* [PATCH] lib: sbi: sbi_ecall: Improve return SBI error
@ 2023-02-21 10:47 Yu Chien Peter Lin
2023-02-21 11:34 ` Xiang W
2023-02-21 17:21 ` Andrew Jones
0 siblings, 2 replies; 7+ messages in thread
From: Yu Chien Peter Lin @ 2023-02-21 10:47 UTC (permalink / raw)
To: opensbi
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.
Signed-off-by: Yu Chien Peter Lin <peterlin@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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] lib: sbi: sbi_ecall: Improve return SBI error
2023-02-21 10:47 [PATCH] lib: sbi: sbi_ecall: Improve return SBI error Yu Chien Peter Lin
@ 2023-02-21 11:34 ` Xiang W
2023-02-22 8:42 ` Yu-Chien Peter Lin
2023-02-21 17:21 ` Andrew Jones
1 sibling, 1 reply; 7+ messages in thread
From: Xiang W @ 2023-02-21 11:34 UTC (permalink / raw)
To: opensbi
? 2023-02-21???? 18:47 +0800?Yu Chien Peter Lin???
> 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.
>
> Signed-off-by: Yu Chien Peter Lin <peterlin@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;
There is no need to rename ret to out_err because a0 is not always an
error code in legacy extensions
Other than the above, it looks good to me.
Reviewed-by: Xiang W <wxjstz@126.com>
> +???????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
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] lib: sbi: sbi_ecall: Improve return SBI error
2023-02-21 10:47 [PATCH] lib: sbi: sbi_ecall: Improve return SBI error Yu Chien Peter Lin
2023-02-21 11:34 ` Xiang W
@ 2023-02-21 17:21 ` Andrew Jones
2023-02-21 17:23 ` Anup Patel
1 sibling, 1 reply; 7+ messages in thread
From: Andrew Jones @ 2023-02-21 17:21 UTC (permalink / raw)
To: opensbi
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.
Thanks,
drew
>
> Signed-off-by: Yu Chien Peter Lin <peterlin@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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] lib: sbi: sbi_ecall: Improve return SBI error
2023-02-21 17:21 ` Andrew Jones
@ 2023-02-21 17:23 ` Anup Patel
2023-02-21 18:07 ` Andrew Jones
0 siblings, 1 reply; 7+ messages in thread
From: Anup Patel @ 2023-02-21 17:23 UTC (permalink / raw)
To: opensbi
On Tue, Feb 21, 2023 at 10:51 PM Andrew Jones <ajones@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 ?
Regards,
Anup
>
> Thanks,
> drew
>
> >
> > Signed-off-by: Yu Chien Peter Lin <peterlin@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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] lib: sbi: sbi_ecall: Improve return SBI error
2023-02-21 17:23 ` Anup Patel
@ 2023-02-21 18:07 ` Andrew Jones
2023-02-22 3:29 ` Anup Patel
0 siblings, 1 reply; 7+ messages in thread
From: Andrew Jones @ 2023-02-21 18:07 UTC (permalink / raw)
To: opensbi
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@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@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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] lib: sbi: sbi_ecall: Improve return SBI error
2023-02-21 18:07 ` Andrew Jones
@ 2023-02-22 3:29 ` Anup Patel
0 siblings, 0 replies; 7+ messages in thread
From: Anup Patel @ 2023-02-22 3:29 UTC (permalink / raw)
To: opensbi
On Tue, Feb 21, 2023 at 11:37 PM Andrew Jones <ajones@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@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@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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] lib: sbi: sbi_ecall: Improve return SBI error
2023-02-21 11:34 ` Xiang W
@ 2023-02-22 8:42 ` Yu-Chien Peter Lin
0 siblings, 0 replies; 7+ messages in thread
From: Yu-Chien Peter Lin @ 2023-02-22 8:42 UTC (permalink / raw)
To: opensbi
On Tue, Feb 21, 2023 at 07:34:46PM +0800, Xiang W wrote:
> ? 2023-02-21???? 18:47 +0800?Yu Chien Peter Lin???
> > 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.
> >
> > Signed-off-by: Yu Chien Peter Lin <peterlin@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;
> There is no need to rename ret to out_err because a0 is not always an
> error code in legacy extensions
> Other than the above, it looks good to me.
>
> Reviewed-by: Xiang W <wxjstz@126.com>
Hi Xiang, Andrew,
Thanks for the review,
Sure I'll use 'ret' as it is in next patch.
Best regards,
Peter Lin
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-02-22 8:42 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-21 10:47 [PATCH] lib: sbi: sbi_ecall: Improve return SBI error Yu Chien Peter Lin
2023-02-21 11:34 ` Xiang W
2023-02-22 8:42 ` Yu-Chien Peter Lin
2023-02-21 17:21 ` Andrew Jones
2023-02-21 17:23 ` Anup Patel
2023-02-21 18:07 ` Andrew Jones
2023-02-22 3:29 ` Anup Patel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.