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

[Andrew Jones]

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?

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