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

[Andrew Jones]

On Fri, May 12, 2023 at 10:17:35PM +0530, Anup Patel wrote:
> 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.

Sounds good. I'll add a patch documenting the probe callback's
purpose.

> 
> >
> > >
> > > 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;

Works for me.

Thanks,
drew