[PATCH] lib: sbi: Ensure SBI extension is available

[Andrew Jones]

On Fri, Apr 28, 2023 at 09:35:45AM +0530, Anup Patel wrote:
> On Wed, Apr 26, 2023 at 10:58 PM Andrew Jones <ajones at ventanamicro.com> wrote:
> >
> > Ensure attempts to invoke SBI extension functions fail with
> > SBI_ERR_NOT_SUPPORTED when the extension's probe function has
> > reported that the extension is not available. By adding a new
> > status member to the extension which has three states
> > (uninitialized, available, unavailable), we ensure that the
> > probe function is only invoked once, lazily, upon first use.
> >
> > Signed-off-by: Andrew Jones <ajones at ventanamicro.com>
> > ---
> >  include/sbi/sbi_ecall.h |  6 ++++++
> >  lib/sbi/sbi_ecall.c     | 18 ++++++++++++++----
> >  2 files changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/sbi/sbi_ecall.h b/include/sbi/sbi_ecall.h
> > index ff9bf8e2b435..8e31d0d91620 100644
> > --- a/include/sbi/sbi_ecall.h
> > +++ b/include/sbi/sbi_ecall.h
> > @@ -20,10 +20,16 @@
> >  struct sbi_trap_regs;
> >  struct sbi_trap_info;
> >
> > +enum sbi_ext_status {
> > +       SBI_EXT_AVAILABLE = 1,
> > +       SBI_EXT_UNAVAILABLE,
> > +};
> > +
> >  struct sbi_ecall_extension {
> >         struct sbi_dlist head;
> >         unsigned long extid_start;
> >         unsigned long extid_end;
> > +       enum sbi_ext_status status;
> >         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 76a1ae9ab733..20ff06f32dad 100644
> > --- a/lib/sbi/sbi_ecall.c
> > +++ b/lib/sbi/sbi_ecall.c
> > @@ -42,16 +42,26 @@ static SBI_LIST_HEAD(ecall_exts_list);
> >
> >  struct sbi_ecall_extension *sbi_ecall_find_extension(unsigned long extid)
> >  {
> > -       struct sbi_ecall_extension *t, *ret = NULL;
> > +       struct sbi_ecall_extension *t;
> > +       unsigned long out_val;
> >
> >         sbi_list_for_each_entry(t, &ecall_exts_list, head) {
> >                 if (t->extid_start <= extid && extid <= t->extid_end) {
> > -                       ret = t;
> > -                       break;
> > +                       if (t->status == SBI_EXT_AVAILABLE)
> > +                               return t;
> > +                       if (t->status == SBI_EXT_UNAVAILABLE)
> > +                               return NULL;
> > +                       if (t->probe && (t->probe(extid, &out_val) || !out_val)) {
> > +                               t->status = SBI_EXT_UNAVAILABLE;
> 
> Setting this `status` flag is a bit racey because two HARTs can end-up
> in sbi_ecall_find_extension() at the same time.

It's racy, but it shouldn't matter, since the result will be the
same whether a second hart ends up calling probe itself or not.

> 
> I suggest an alternate approach whereby we don't need the `status`
> flag and sbi_ecall_find_extension() also does not need to change:
> 
> 1) Rename `probe()` callback to `init()` callback
> 2) Remove sbi_ecall_register_extension() call from sbi_ecall_init()
>     and instead call `init()` for each extension
> 3) Implement `init()` for each extension such that each extension
>     explicitly register itself using sbi_ecall_register_extension()
>     only if the extension is actually available.
> 
> In other words, the alternate approach treats SBI extensions are
> driver instances which need to be registered by some init() call.

I mostly like that, but probe is specified as "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.",
which means a future extension may want to return some nonzero value
from its probe function to inform the caller of something. So, maybe
we keep probe, add init, and call probe from init to decide if we
register the extension?

Thanks,
drew