[PATCH] lib: sbi: sbi_ecall: Improve return SBI error

[Anup Patel]

On Tue, Feb 21, 2023 at 11:37 PM Andrew Jones <ajones at ventanamicro.com> wrote:
>
> On Tue, Feb 21, 2023 at 10:53:31PM +0530, Anup Patel wrote:
> > On Tue, Feb 21, 2023 at 10:51 PM Andrew Jones <ajones at ventanamicro.com> wrote:
> > >
> > > On Tue, Feb 21, 2023 at 06:47:56PM +0800, Yu Chien Peter Lin wrote:
> > > > We should also check if the return error code is greater than 0
> > > > (SBI_SUCCESS), as this is an invalid error.
> > > >
> > > > Also rename the variable returned via a0 to 'out_err' and use
> > > > SBI_ERR_NOT_SUPPORTED when the extension does not have a handler
> > > > for consistency with the standard SBI errors defined in Table 1
> > > > of the SBI specification.
> > >
> > > It's best to do renames separate from functional changes, and, in
> > > this case, the ret -> out_err rename feels like unnecessary churn.
> >
> > Maybe we can do something similar to KVM RISC-V ?
>
> We could if we have three types of return values to manage like we do
> with KVM. For anyone who isn't familiar with how KVM will manage things,
> once the PMU patches are merged, it'll work like this:
>
>  1. Linux error while emulating an SBI call (e.g. ENOMEM)
>  2. 'sbiret.error' generated during emulation
>  3. 'sbiret.value' generated during emulation
>
> (1) is the return value of an SBI emulation function,
>
>     int sbi_func(struct kvm_vcpu_sbi_return *retdata)
>
> (2) is kvm_vcpu_sbi_return.out_val
> (3) is kvm_vcpu_sbi_return.err_val
>
>
> But don't we only have (2) and (3) here?

We do have (1) as well because SBI_Exxx are supposed
to be errors internal to OpenSBI whereas SBI_ERR_xxx
are standard SBI errors.

Maybe as a separate series we can improve error returned
by SBI calls in OpenSBI?

Regards,
Anup

>
> Thanks,
> drew
>
> >
> > Regards,
> > Anup
> >
> > >
> > > Thanks,
> > > drew
> > >
> > > >
> > > > Signed-off-by: Yu Chien Peter Lin <peterlin at andestech.com>
> > > > ---
> > > >  lib/sbi/sbi_ecall.c | 18 +++++++++---------
> > > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/lib/sbi/sbi_ecall.c b/lib/sbi/sbi_ecall.c
> > > > index 27ce5d49..8accf675 100644
> > > > --- a/lib/sbi/sbi_ecall.c
> > > > +++ b/lib/sbi/sbi_ecall.c
> > > > @@ -97,34 +97,34 @@ void sbi_ecall_unregister_extension(struct sbi_ecall_extension *ext)
> > > >
> > > >  int sbi_ecall_handler(struct sbi_trap_regs *regs)
> > > >  {
> > > > -     int ret = 0;
> > > > +     int out_err = 0;
> > > > +     unsigned long out_val = 0;
> > > >       struct sbi_ecall_extension *ext;
> > > >       unsigned long extension_id = regs->a7;
> > > >       unsigned long func_id = regs->a6;
> > > >       struct sbi_trap_info trap = {0};
> > > > -     unsigned long out_val = 0;
> > > >       bool is_0_1_spec = 0;
> > > >
> > > >       ext = sbi_ecall_find_extension(extension_id);
> > > >       if (ext && ext->handle) {
> > > > -             ret = ext->handle(extension_id, func_id,
> > > > +             out_err = ext->handle(extension_id, func_id,
> > > >                                 regs, &out_val, &trap);
> > > >               if (extension_id >= SBI_EXT_0_1_SET_TIMER &&
> > > >                   extension_id <= SBI_EXT_0_1_SHUTDOWN)
> > > >                       is_0_1_spec = 1;
> > > >       } else {
> > > > -             ret = SBI_ENOTSUPP;
> > > > +             out_err = SBI_ERR_NOT_SUPPORTED;
> > > >       }
> > > >
> > > > -     if (ret == SBI_ETRAP) {
> > > > +     if (out_err == SBI_ETRAP) {
> > > >               trap.epc = regs->mepc;
> > > >               sbi_trap_redirect(regs, &trap);
> > > >       } else {
> > > > -             if (ret < SBI_LAST_ERR) {
> > > > +             if (out_err < SBI_LAST_ERR || SBI_SUCCESS < out_err) {
> > > >                       sbi_printf("%s: Invalid error %d for ext=0x%lx "
> > > > -                                "func=0x%lx\n", __func__, ret,
> > > > +                                "func=0x%lx\n", __func__, out_err,
> > > >                                  extension_id, func_id);
> > > > -                     ret = SBI_ERR_FAILED;
> > > > +                     out_err = SBI_ERR_FAILED;
> > > >               }
> > > >
> > > >               /*
> > > > @@ -136,7 +136,7 @@ int sbi_ecall_handler(struct sbi_trap_regs *regs)
> > > >                * case should be handled differently.
> > > >                */
> > > >               regs->mepc += 4;
> > > > -             regs->a0 = ret;
> > > > +             regs->a0 = out_err;
> > > >               if (!is_0_1_spec)
> > > >                       regs->a1 = out_val;
> > > >       }
> > > > --
> > > > 2.34.1
> > > >
> > > >
> > > > --
> > > > opensbi mailing list
> > > > opensbi at lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/opensbi
> > >
> > > --
> > > opensbi mailing list
> > > opensbi at lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/opensbi