RISC-V: Align SBI probe implementation with spec
Subsystems (6)
PATCH 1/1 RISC-V: Align SBI probe implementation with spec
sbi_probe_extension() is specified with "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." Additionally, sbiret.value is a long. Fix the implementation to ensure any nonzero long value is considered a success, rather than only positive int values.
--- arch/riscv/include/asm/sbi.h | 2 +- arch/riscv/kernel/cpu_ops.c | 2 +- arch/riscv/kernel/sbi.c | 17 ++++++++--------- arch/riscv/kvm/main.c | 2 +- drivers/cpuidle/cpuidle-riscv-sbi.c | 2 +- drivers/perf/riscv_pmu_sbi.c | 2 +- 6 files changed, 13 insertions(+), 14 deletions(-)
arch/riscv/include/asm/sbi.h +1 -1
@@ -296,7 +296,7 @@ int sbi_remote_hfence_vvma_asid(const struct cpumask *cpu_mask,unsigned long start, unsigned long size, unsigned long asid); -int sbi_probe_extension (int ext); +long sbi_probe_extension (int ext);/* Check if current SBI specification version is 0.1 or not */ static inline int sbi_spec_is_0_1 (void )
arch/riscv/kernel/cpu_ops.c +1 -1
@@ -27,7 +27,7 @@ const struct cpu_operations cpu_ops_spinwait = {void __init cpu_set_ops (int cpuid) {#if IS_ENABLED (CONFIG_RISCV_SBI ) -if (sbi_probe_extension (SBI_EXT_HSM ) >0 ) { +if (sbi_probe_extension (SBI_EXT_HSM )) {if (!cpuid)pr_info ("SBI HSM extension detected\n" ); cpu_ops[cpuid] = &cpu_ops_sbi;
arch/riscv/kernel/sbi.c +8 -9
@@ -581,19 +581,18 @@ static void sbi_srst_power_off(void)* sbi_probe_extension() - Check if an SBI extension ID is supported or not. * @extid: The extension ID to be probed. * -* Return: Extension specific nonzero value f yes, -ENOTSUPP otherwise. +* Return: 1 or an extension specific nonzero value if yes, 0 otherwise. */ -int sbi_probe_extension (int extid) +long sbi_probe_extension (int extid) {struct sbiret ret; ret =sbi_ecall (SBI_EXT_BASE ,SBI_EXT_BASE_PROBE_EXT , extid,0 ,0 ,0 ,0 ,0 );if (!ret.error) -if (ret.value) -return ret.value; +return ret.value; -return -ENOTSUPP ; +return 0 ; }EXPORT_SYMBOL (sbi_probe_extension);
@@ -665,26 +664,26 @@ void __init sbi_init(void)if (!sbi_spec_is_0_1 ()) {pr_info ("SBI implementation ID=0x%lx Version=0x%lx\n" ,sbi_get_firmware_id (),sbi_get_firmware_version ()); -if (sbi_probe_extension (SBI_EXT_TIME ) >0 ) { +if (sbi_probe_extension (SBI_EXT_TIME )) { __sbi_set_timer = __sbi_set_timer_v02;pr_info ("SBI TIME extension detected\n" ); }else { __sbi_set_timer = __sbi_set_timer_v01; } -if (sbi_probe_extension (SBI_EXT_IPI ) >0 ) { +if (sbi_probe_extension (SBI_EXT_IPI )) { __sbi_send_ipi = __sbi_send_ipi_v02;pr_info ("SBI IPI extension detected\n" ); }else { __sbi_send_ipi = __sbi_send_ipi_v01; } -if (sbi_probe_extension (SBI_EXT_RFENCE ) >0 ) { +if (sbi_probe_extension (SBI_EXT_RFENCE )) { __sbi_rfence = __sbi_rfence_v02;pr_info ("SBI RFENCE extension detected\n" ); }else { __sbi_rfence = __sbi_rfence_v01; }if ((sbi_spec_version >=sbi_mk_version (0 ,3 )) && - (sbi_probe_extension (SBI_EXT_SRST ) >0 )) { +sbi_probe_extension (SBI_EXT_SRST )) {pr_info ("SBI SRST extension detected\n" ); pm_power_off = sbi_srst_power_off; sbi_srst_reboot_nb.notifier_call = sbi_srst_reboot;
arch/riscv/kvm/main.c +1 -1
@@ -75,7 +75,7 @@ static int __init riscv_kvm_init(void)return -ENODEV ; } -if (sbi_probe_extension (SBI_EXT_RFENCE ) <=0 ) { +if (!sbi_probe_extension (SBI_EXT_RFENCE )) {kvm_info ("require SBI RFENCE extension\n" );return -ENODEV ; }
drivers/cpuidle/cpuidle-riscv-sbi.c +1 -1
@@ -613,7 +613,7 @@ static int __init sbi_cpuidle_init(void)* 2) SBI HSM extension is available */ if ((sbi_spec_version <sbi_mk_version (0 ,3 )) || -sbi_probe_extension (SBI_EXT_HSM ) <=0 ) { + !sbi_probe_extension (SBI_EXT_HSM )) {pr_info ("HSM suspend not available\n" );return 0 ; }
drivers/perf/riscv_pmu_sbi.c +1 -1
@@ -924,7 +924,7 @@ static int __init pmu_sbi_devinit(void)struct platform_device *pdev;if (sbi_spec_version <sbi_mk_version (0 ,3 ) || -sbi_probe_extension (SBI_EXT_PMU ) <=0 ) { + !sbi_probe_extension (SBI_EXT_PMU )) {return 0 ; }
Signature · 1 line
2.39.2
PATCH 1/1 RISC-V: Align SBI probe implementation with spec
Quoted 6 lines · Andrew Jones
sbi_probe_extension()is specified with "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." Additionally, sbiret.value is a long. Fix the implementation to ensure any nonzero long value is considered a success, rather than only positive int values.
Does this need a fixes tag (or tags) then, since we could easily return a negative value as things stand if the SBI implementation decides to return 0b1...1 for success?
Quoted 51 lines
Signed-off-by Andrew Jones <[email protected]>--- arch/riscv/include/asm/sbi.h | 2 +- arch/riscv/kernel/cpu_ops.c | 2 +- arch/riscv/kernel/sbi.c | 17 ++++++++--------- arch/riscv/kvm/main.c | 2 +- drivers/cpuidle/cpuidle-riscv-sbi.c | 2 +- drivers/perf/riscv_pmu_sbi.c | 2 +- 6 files changed, 13 insertions(+), 14 deletions(-)arch/riscv/include/asm/sbi.h +1
-1@@ -296,7 +296,7 @@ int sbi_remote_hfence_vvma_asid(const struct cpumask *cpu_mask,unsigned long start, unsigned long size, unsigned long asid); -int sbi_probe_extension (int ext); +long sbi_probe_extension (int ext);/* Check if current SBI specification version is 0.1 or not */ static inline int sbi_spec_is_0_1 (void )arch/riscv/kernel/cpu_ops.c +1
-1@@ -27,7 +27,7 @@ const struct cpu_operations cpu_ops_spinwait = {void __init cpu_set_ops (int cpuid) {#if IS_ENABLED (CONFIG_RISCV_SBI ) -if (sbi_probe_extension (SBI_EXT_HSM ) >0 ) { +if (sbi_probe_extension (SBI_EXT_HSM )) {if (!cpuid)pr_info ("SBI HSM extension detected\n" ); cpu_ops[cpuid] = &cpu_ops_sbi;arch/riscv/kernel/sbi.c +2
-2@@ -581,19 +581,18 @@ static void sbi_srst_power_off(void)* sbi_probe_extension() - Check if an SBI extension ID is supported or not. * @extid: The extension ID to be probed. * -* Return: Extension specific nonzero value f yes, -ENOTSUPP otherwise. +* Return: 1 or an extension specific nonzero value if yes, 0 otherwise. */ -int sbi_probe_extension (int extid) +long sbi_probe_extension (int extid) {struct sbiret ret;
arch/riscv/kernel/sbi.c static void sbi_srst_power_off(void) · line 595
Quoted 10 lines
arch/riscv/kernel/sbi.c +2
-3ret =sbi_ecall (SBI_EXT_BASE ,SBI_EXT_BASE_PROBE_EXT , extid,0 ,0 ,0 ,0 ,0 );if (!ret.error) -if (ret.value) -return ret.value; +return ret.value; -return -ENOTSUPP ; +return 0 ;
Why not make it return -ENOTSUPP for failures and 0 for success instead, as it does not appear that any users actually care what the return value is, once it is not an error?
Concerned that it'll look weird to have
if(!sbi_probe_extension(foo))
print(foo is available!)
since it looks a bit weird that the !case is success?
If so, perhaps it could just return a bool instead, unless of course I am missing some pending user that make some decision on the actual value returned here is.
Cheers,
Conor.
Quoted 76 lines
arch/riscv/kernel/sbi.c +4
-4}EXPORT_SYMBOL (sbi_probe_extension);@@ -665,26 +664,26 @@ void __init sbi_init(void)if (!sbi_spec_is_0_1 ()) {pr_info ("SBI implementation ID=0x%lx Version=0x%lx\n" ,sbi_get_firmware_id (),sbi_get_firmware_version ()); -if (sbi_probe_extension (SBI_EXT_TIME ) >0 ) { +if (sbi_probe_extension (SBI_EXT_TIME )) { __sbi_set_timer = __sbi_set_timer_v02;pr_info ("SBI TIME extension detected\n" ); }else { __sbi_set_timer = __sbi_set_timer_v01; } -if (sbi_probe_extension (SBI_EXT_IPI ) >0 ) { +if (sbi_probe_extension (SBI_EXT_IPI )) { __sbi_send_ipi = __sbi_send_ipi_v02;pr_info ("SBI IPI extension detected\n" ); }else { __sbi_send_ipi = __sbi_send_ipi_v01; } -if (sbi_probe_extension (SBI_EXT_RFENCE ) >0 ) { +if (sbi_probe_extension (SBI_EXT_RFENCE )) { __sbi_rfence = __sbi_rfence_v02;pr_info ("SBI RFENCE extension detected\n" ); }else { __sbi_rfence = __sbi_rfence_v01; }if ((sbi_spec_version >=sbi_mk_version (0 ,3 )) && - (sbi_probe_extension (SBI_EXT_SRST ) >0 )) { +sbi_probe_extension (SBI_EXT_SRST )) {pr_info ("SBI SRST extension detected\n" ); pm_power_off = sbi_srst_power_off; sbi_srst_reboot_nb.notifier_call = sbi_srst_reboot;arch/riscv/kvm/main.c +1
-1@@ -75,7 +75,7 @@ static int __init riscv_kvm_init(void)return -ENODEV ; } -if (sbi_probe_extension (SBI_EXT_RFENCE ) <=0 ) { +if (!sbi_probe_extension (SBI_EXT_RFENCE )) {kvm_info ("require SBI RFENCE extension\n" );return -ENODEV ; }drivers/cpuidle/cpuidle-riscv-sbi.c +1
-1@@ -613,7 +613,7 @@ static int __init sbi_cpuidle_init(void)* 2) SBI HSM extension is available */ if ((sbi_spec_version <sbi_mk_version (0 ,3 )) || -sbi_probe_extension (SBI_EXT_HSM ) <=0 ) { + !sbi_probe_extension (SBI_EXT_HSM )) {pr_info ("HSM suspend not available\n" );return 0 ; }drivers/perf/riscv_pmu_sbi.c +1
-1@@ -924,7 +924,7 @@ static int __init pmu_sbi_devinit(void)struct platform_device *pdev;if (sbi_spec_version <sbi_mk_version (0 ,3 ) || -sbi_probe_extension (SBI_EXT_PMU ) <=0 ) { + !sbi_probe_extension (SBI_EXT_PMU )) {return 0 ; }Signature · 1 line
2.39.2
PATCH 1/1 RISC-V: Align SBI probe implementation with spec
Quoted 11 lines · Conor Dooley
Quoted 6 lines · Andrew Jones
sbi_probe_extension()is specified with "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." Additionally, sbiret.value is a long. Fix the implementation to ensure any nonzero long value is considered a success, rather than only positive int values.Does this need a fixes tag (or tags) then, since we could easily return a negative value as things stand if the SBI implementation decides to return 0b1...1 for success?
Nothing is getting fixed when only considering the current generic opensbi platform, but there could be other SBI implementations Linux isn't handling correctly by only considering success to be greater than zero or by truncating the return value from long to int. I suppose that possibility does warrant a fix tag, which would be
Quoted 60 lines
Quoted 60 lines
Signed-off-by Andrew Jones <[email protected]>--- arch/riscv/include/asm/sbi.h | 2 +- arch/riscv/kernel/cpu_ops.c | 2 +- arch/riscv/kernel/sbi.c | 17 ++++++++--------- arch/riscv/kvm/main.c | 2 +- drivers/cpuidle/cpuidle-riscv-sbi.c | 2 +- drivers/perf/riscv_pmu_sbi.c | 2 +- 6 files changed, 13 insertions(+), 14 deletions(-)arch/riscv/include/asm/sbi.h +1
-1@@ -296,7 +296,7 @@ int sbi_remote_hfence_vvma_asid(const struct cpumask *cpu_mask,unsigned long start, unsigned long size, unsigned long asid); -int sbi_probe_extension (int ext); +long sbi_probe_extension (int ext);/* Check if current SBI specification version is 0.1 or not */ static inline int sbi_spec_is_0_1 (void )arch/riscv/kernel/cpu_ops.c +1
-1@@ -27,7 +27,7 @@ const struct cpu_operations cpu_ops_spinwait = {void __init cpu_set_ops (int cpuid) {#if IS_ENABLED (CONFIG_RISCV_SBI ) -if (sbi_probe_extension (SBI_EXT_HSM ) >0 ) { +if (sbi_probe_extension (SBI_EXT_HSM )) {if (!cpuid)pr_info ("SBI HSM extension detected\n" ); cpu_ops[cpuid] = &cpu_ops_sbi;arch/riscv/kernel/sbi.c +3
-4@@ -581,19 +581,18 @@ static void sbi_srst_power_off(void)* sbi_probe_extension() - Check if an SBI extension ID is supported or not. * @extid: The extension ID to be probed. * -* Return: Extension specific nonzero value f yes, -ENOTSUPP otherwise. +* Return: 1 or an extension specific nonzero value if yes, 0 otherwise. */ -int sbi_probe_extension (int extid) +long sbi_probe_extension (int extid) {struct sbiret ret; ret =sbi_ecall (SBI_EXT_BASE ,SBI_EXT_BASE_PROBE_EXT , extid,0 ,0 ,0 ,0 ,0 );if (!ret.error) -if (ret.value) -return ret.value; +return ret.value;
Quoted 6 lines
arch/riscv/kernel/sbi.c static void sbi_srst_power_off(void) · line 595
Quoted 2 lines
arch/riscv/kernel/sbi.c +1
-1-return -ENOTSUPP ; +return 0 ;Why not make it return
-ENOTSUPPfor failures and 0 for success instead, as it does not appear that any users actually care what the return value is, once it is not an error?
Just to prepare for theoretical new uses.
Quoted 4 lines
Concerned that it'll look weird to have
if(!sbi_probe_extension(foo))
print(foo is available!)
since it looks a bit weird that the !case is success?
No, that's pretty par for the course in the kernel. I'd just prefer that sbi_probe_extension() be a simple wrapper around the SBI call, one that doesn't change the SBI call's return value semantics.
Quoted 4 lines
If so, perhaps it could just return a bool instead, unless of course I am missing some pending user that make some decision on the actual value returned here is.
No pending user that I'm aware of, but it's not too awkward, IMO, to implement this as the spec says, so any pending user that comes along will be happy from the start. If a boolean function seems better for everyone, then I'd probably extend this patch to also rename this sbi_probe_extension to __sbi_probe_extension, make it static, and then add
bool sbi_probe_extension(int extid)
{
return __sbi_probe_extension(extid);
}
just to feel good about the wrapper!
Thanks,
drew
PATCH 1/1 RISC-V: Align SBI probe implementation with spec
Quoted 13 lines · Andrew Jones
Quoted 11 lines · Conor Dooley
Quoted 6 lines · Andrew Jones
sbi_probe_extension()is specified with "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." Additionally, sbiret.value is a long. Fix the implementation to ensure any nonzero long value is considered a success, rather than only positive int values.Does this need a fixes tag (or tags) then, since we could easily return a negative value as things stand if the SBI implementation decides to return 0b1...1 for success?
Quoted 5 lines
Nothing is getting fixed when only considering the current generic opensbi platform, but there could be other SBI implementations Linux isn't handling correctly by only considering success to be greater than zero or by truncating the return value from long to int. I suppose that possibility does warrant a fix tag, which would be
Yah, I figured that something like opensbi would do the "sane" thing here, but there's no accounting for taste and all that.
Quoted 81 lines
Fixes b9dcd9e41587 ("RISC-V: Add basic support for SBI v0.2")Quoted 60 lines
Quoted 60 lines
Signed-off-by Andrew Jones <[email protected]>--- arch/riscv/include/asm/sbi.h | 2 +- arch/riscv/kernel/cpu_ops.c | 2 +- arch/riscv/kernel/sbi.c | 17 ++++++++--------- arch/riscv/kvm/main.c | 2 +- drivers/cpuidle/cpuidle-riscv-sbi.c | 2 +- drivers/perf/riscv_pmu_sbi.c | 2 +- 6 files changed, 13 insertions(+), 14 deletions(-)arch/riscv/include/asm/sbi.h +1
-1@@ -296,7 +296,7 @@ int sbi_remote_hfence_vvma_asid(const struct cpumask *cpu_mask,unsigned long start, unsigned long size, unsigned long asid); -int sbi_probe_extension (int ext); +long sbi_probe_extension (int ext);/* Check if current SBI specification version is 0.1 or not */ static inline int sbi_spec_is_0_1 (void )arch/riscv/kernel/cpu_ops.c +1
-1@@ -27,7 +27,7 @@ const struct cpu_operations cpu_ops_spinwait = {void __init cpu_set_ops (int cpuid) {#if IS_ENABLED (CONFIG_RISCV_SBI ) -if (sbi_probe_extension (SBI_EXT_HSM ) >0 ) { +if (sbi_probe_extension (SBI_EXT_HSM )) {if (!cpuid)pr_info ("SBI HSM extension detected\n" ); cpu_ops[cpuid] = &cpu_ops_sbi;arch/riscv/kernel/sbi.c +3
-4@@ -581,19 +581,18 @@ static void sbi_srst_power_off(void)* sbi_probe_extension() - Check if an SBI extension ID is supported or not. * @extid: The extension ID to be probed. * -* Return: Extension specific nonzero value f yes, -ENOTSUPP otherwise. +* Return: 1 or an extension specific nonzero value if yes, 0 otherwise. */ -int sbi_probe_extension (int extid) +long sbi_probe_extension (int extid) {struct sbiret ret; ret =sbi_ecall (SBI_EXT_BASE ,SBI_EXT_BASE_PROBE_EXT , extid,0 ,0 ,0 ,0 ,0 );if (!ret.error) -if (ret.value) -return ret.value; +return ret.value;Quoted 6 lines
arch/riscv/kernel/sbi.c static void sbi_srst_power_off(void) · line 595
Quoted 2 lines
arch/riscv/kernel/sbi.c +1
-1-return -ENOTSUPP ; +return 0 ;Why not make it return
-ENOTSUPPfor failures and 0 for success instead, as it does not appear that any users actually care what the return value is, once it is not an error?Just to prepare for theoretical new uses.
Quoted 4 lines
Concerned that it'll look weird to have
if(!sbi_probe_extension(foo))
print(foo is available!)
since it looks a bit weird that the !case is success?No, that's pretty par for the course in the kernel. I'd just prefer that
sbi_probe_extension()be a simple wrapper around the SBI call, one that doesn't change the SBI call's return value semantics.Quoted 1 line
Quoted 8 lines
Quoted 3 lines
If so, perhaps it could just return a bool instead, unless of course I am missing some pending user that make some decision on the actual value returned here is.
No pending user that I'm aware of, but it's not too awkward, IMO, to implement this as the spec says, so any pending user that comes along will be happy from the start. If a boolean function seems better for everyone,
I don't really mind which way you do it to be honest.
I see reasons for both wanting alignment with what the SBI spec has & for returning the simplest type that fits the usage.
Both are better than the current, potentially buggy, implementation.
Can put a
on it either way.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/kvm-riscv/attachments/20230427/552cc251/attachment-0001.sig>