[PATCH v4 2/3] lib: sbi: Introduce register_extensions extension callback

[Anup Patel]

On Fri, May 12, 2023 at 8:36 PM Andrew Jones <ajones at ventanamicro.com> wrote:
>
> On Fri, May 12, 2023 at 07:55:56PM +0530, Anup Patel wrote:
> > On Thu, May 11, 2023 at 9:41 PM Andrew Jones <ajones at ventanamicro.com> wrote:
> > >
> > > The register_extensions callback is called from sbi_ecall_init() for
> > > any extension that provides it. It's purpose is to register the
> > > extension with one or more sbi_ecall_register_extension() calls after
> > > possibly narrowing the range of extension IDs that should be handled.
> > > More than one call of sbi_ecall_register_extension() is necessary
> > > when the supported extension ID ranges have gaps. Additionally, when
> > > an extension may not be available and has a probe function, then the
> > > probe function should be consulted as to whether or not the extension
> > > should be registered. In summary, when register_extensions() returns,
> > > only valid extension IDs from the initial range, which are also
> > > available, have been registered.
> > >
> > > Signed-off-by: Andrew Jones <ajones at ventanamicro.com>
> > > ---
> > >  include/sbi/sbi_ecall.h | 1 +
> > >  lib/sbi/sbi_ecall.c     | 8 ++++++--
> > >  2 files changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/sbi/sbi_ecall.h b/include/sbi/sbi_ecall.h
> > > index ff9bf8e2b435..fac26429cf5d 100644
> > > --- a/include/sbi/sbi_ecall.h
> > > +++ b/include/sbi/sbi_ecall.h
> > > @@ -24,6 +24,7 @@ struct sbi_ecall_extension {
> > >         struct sbi_dlist head;
> > >         unsigned long extid_start;
> > >         unsigned long extid_end;
> > > +       int (* register_extensions)(void);
> > >         int (* probe)(unsigned long extid, unsigned long *out_val);
> > >         int (* handle)(unsigned long extid, unsigned long funcid,
> > >                        const struct sbi_trap_regs *regs,
> > > diff --git a/lib/sbi/sbi_ecall.c b/lib/sbi/sbi_ecall.c
> > > index 5a301fb7d403..1c4a26fde438 100644
> > > --- a/lib/sbi/sbi_ecall.c
> > > +++ b/lib/sbi/sbi_ecall.c
> > > @@ -154,8 +154,12 @@ int sbi_ecall_init(void)
> > >
> > >         for (i = 0; i < sbi_ecall_exts_size; i++) {
> > >                 ext = sbi_ecall_exts[i];
> > > -               if (ext->extid_start != ext->extid_end || !ext->probe ||
> > > -                   (!ext->probe(ext->extid_end, &out_val) && out_val)) {
> > > +               if (ext->register_extensions) {
> > > +                       ret = ext->register_extensions();
> > > +                       if (ret)
> > > +                               return ret;
> > > +               } else if (ext->extid_start != ext->extid_end || !ext->probe ||
> > > +                          (!ext->probe(ext->extid_end, &out_val) && out_val)) {
> >
> > We should replace probe() callback with register_extensions()
> > and code over here can be something like below:
>
> We need to keep probe in case an extension wants to implement a
> nonzero out_val, which can be inspected by S-mode with the Base
> extension probe function:
>
> """
> Probe SBI extension (FID #3)
>
> Returns 0 if the given SBI extension ID (EID) is not available, or
> 1 if it is available unless defined as any other non-zero value by
> the implementation.
> """

Fair enough, we can keep the probe() callback for custom
return values but none of the existing SBI extensions have
custom probe return values so don't need probe existing
SBI extensions.

>
> >
> > if (ext->register_extensions)
> >      ret = ext->register_extensions();
> > else if (ext->extid_start == ext->extid_end)
> >      ret = sbi_ecall_register_extension(ext);
>
> Also here, if we don't have probe, then we can't know if the single
> ID extension should be added or not. Not adding the extensions which
> fail probe to the list, so sbi_ecall_find_extension() can't find
> them when their functions are invoked, is the main motivation of the
> series.

We should just implemetn register_extensions() for all
existing SBI extensions and remove probe() implementation
so the code over here will be a simple if-else:

if (ext->register_extensions)
    ret = ext->register_extensions();
else
    ret = SBI_ENODEV;
if (ret)
    return ret;

>
> > else
> >      ret = SBI_ENODEV;
> > if (ret)
> >     return ret;
> >
> > Basically, register_extensions() is optional for single extension ID
> > but mandatory for extension ID range.
>
> Oh, right, register_extensions() needs to be mandatory for multi-ID
> extensions, otherwise we would still need a change in
> sbi_ecall_find_extension() to ensure we don't return the extension for
> IDs which aren't available. However, if we make it mandatory for all
> multi-ID extensions, then the legacy extensions will need to implement
> it, even though it will just call sbi_ecall_register_extension() without
> making any changes first. I guess that's fine.
>
> I'll go ahead and make register_extensions() mandatory for multi-ID
> extensions and add the callback to the legacy range. So, assuming we
> keep probe for the reasons I stated above, then we'll have this
>
>  if (ext->register_extensions) {
>      ret = ext->register_extensions();
>  else if (ext->extid_start == ext->extid_end)
>      if (!ext->probe || (!ext->probe(ext->extid_end, &out_val) && out_val))
>          ret = sbi_ecall_register_extension(ext);
>  else
>      ret = SBI_ENODEV;
>
>  if (ret)
>      return ret;
>
>
> Thanks,
> drew

Regards,
Anup