[PATCH] riscv: fix isa validation for virtual [ms]envcfg extension

[PATCH] riscv: fix isa validation for virtual [ms]envcfg extension

[Andy Chiu]

The exntension RISCV_ISA_EXT_XLINUXENVCFG was accidentally turned off
after introducing ISA validation. The reason is that this extension is
not in the riscv_isa_ext[] list, causing riscv_get_isa_ext_data() fail
to find a corresponding riscv_isa_ext_data pointer. As a result, the
kernel skipped setting resolved_isa bitmask. To fix this, we proceed
with the bit information obtained from source_isa bitmask. This means
that the bit in resolved_isa is set unconditionally, regardless of
whether we find an entry from riscv_isa_ext[] list. This should be safe
as the kernel already parse isa string before writing into source_isa.

Fixes: 625034abd52a ("riscv: add ISA extensions validation callback")
Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
---
 arch/riscv/kernel/cpufeature.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 8f20607adb40..3f91a1f39a50 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -432,10 +432,7 @@ static void __init riscv_resolve_isa(unsigned long *source_isa,
 		bitmap_copy(prev_resolved_isa, resolved_isa, RISCV_ISA_EXT_MAX);
 		for_each_set_bit(bit, source_isa, RISCV_ISA_EXT_MAX) {
 			ext = riscv_get_isa_ext_data(bit);
-			if (!ext)
-				continue;
-
-			if (ext->validate) {
+			if (ext && ext->validate) {
 				ret = ext->validate(ext, resolved_isa);
 				if (ret == -EPROBE_DEFER) {
 					loop = true;
@@ -447,13 +444,13 @@ static void __init riscv_resolve_isa(unsigned long *source_isa,
 				}
 			}
 
-			set_bit(ext->id, resolved_isa);
+			set_bit(bit, resolved_isa);
 			/* No need to keep it in source isa now that it is enabled */
-			clear_bit(ext->id, source_isa);
+			clear_bit(bit, source_isa);
 
 			/* Single letter extensions get set in hwcap */
-			if (ext->id < RISCV_ISA_EXT_BASE)
-				*this_hwcap |= isa2hwcap[ext->id];
+			if (bit < RISCV_ISA_EXT_BASE)
+				*this_hwcap |= isa2hwcap[bit];
 		}
 	} while (loop && memcmp(prev_resolved_isa, resolved_isa, sizeof(prev_resolved_isa)));
 }
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: fix isa validation for virtual [ms]envcfg extension

[Andrew Jones]

On Tue, Jul 30, 2024 at 03:38:45PM GMT, Andy Chiu wrote:
> The exntension RISCV_ISA_EXT_XLINUXENVCFG was accidentally turned off
> after introducing ISA validation. The reason is that this extension is
> not in the riscv_isa_ext[] list, causing riscv_get_isa_ext_data() fail
> to find a corresponding riscv_isa_ext_data pointer. As a result, the
> kernel skipped setting resolved_isa bitmask. To fix this, we proceed
> with the bit information obtained from source_isa bitmask. This means
> that the bit in resolved_isa is set unconditionally, regardless of
> whether we find an entry from riscv_isa_ext[] list. This should be safe
> as the kernel already parse isa string before writing into source_isa.
> 
> Fixes: 625034abd52a ("riscv: add ISA extensions validation callback")
> Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
> ---
>  arch/riscv/kernel/cpufeature.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 8f20607adb40..3f91a1f39a50 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -432,10 +432,7 @@ static void __init riscv_resolve_isa(unsigned long *source_isa,
>  		bitmap_copy(prev_resolved_isa, resolved_isa, RISCV_ISA_EXT_MAX);
>  		for_each_set_bit(bit, source_isa, RISCV_ISA_EXT_MAX) {
>  			ext = riscv_get_isa_ext_data(bit);
> -			if (!ext)
> -				continue;
> -
> -			if (ext->validate) {
> +			if (ext && ext->validate) {
>  				ret = ext->validate(ext, resolved_isa);
>  				if (ret == -EPROBE_DEFER) {
>  					loop = true;
> @@ -447,13 +444,13 @@ static void __init riscv_resolve_isa(unsigned long *source_isa,
>  				}
>  			}
>  
> -			set_bit(ext->id, resolved_isa);
> +			set_bit(bit, resolved_isa);
>  			/* No need to keep it in source isa now that it is enabled */
> -			clear_bit(ext->id, source_isa);
> +			clear_bit(bit, source_isa);
>  
>  			/* Single letter extensions get set in hwcap */
> -			if (ext->id < RISCV_ISA_EXT_BASE)
> -				*this_hwcap |= isa2hwcap[ext->id];
> +			if (bit < RISCV_ISA_EXT_BASE)
> +				*this_hwcap |= isa2hwcap[bit];
>  		}
>  	} while (loop && memcmp(prev_resolved_isa, resolved_isa, sizeof(prev_resolved_isa)));
>  }
> -- 
> 2.43.0
>

Hi Andy,

This looks like the same solution that Samuel sent which is currently
under discussion[1].

[1] https://lore.kernel.org/all/20240718213011.2600150-1-samuel.holland@sifive.com/

Thanks,
drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv