Re: [PATCH -fixes] riscv: cpufeature: Do not drop Linux-internal extensions

[Samuel Holland <samuel.holland@sifive.com>]

Hi Drew, Clément,

On 2024-07-18 4:57 PM, Andrew Jones wrote:
> On Thu, Jul 18, 2024 at 02:29:59PM GMT, Samuel Holland wrote:
>> The Linux-internal Xlinuxenvcfg ISA extension is omitted from the
>> riscv_isa_ext array because it has no DT binding and should not appear
>> in /proc/cpuinfo. The logic added in commit 625034abd52a ("riscv: add
>> ISA extensions validation callback") assumes all extensions are included
>> in riscv_isa_ext, and so riscv_resolve_isa() wrongly drops Xlinuxenvcfg
>> from the final ISA string. Instead, accept such Linux-internal ISA
>> extensions as if they have no validation callback.
> 
> This assumes we'll never need a validation callback for a Linux-internal
> ISA extension. We can make that assumption now and change our mind

I think that's a reasonable assumption. An internal extension can only be
added by implication, so any validation would happen in the context of the
superset extension which is actually in the DT/ISA string.

> later, but we could also add Xlinuxenvcfg to riscv_isa_ext now and
> modify the places where it matters (just print_isa?). If we add
> Xlinuxenvcfg to the array with a NULL name then we could do something
> like
> 
>  print_isa()
>  {
>     for (...) {
>         ...
>         if (!riscv_isa_ext[i].name)
> 	   continue;
>     }
>  }

This strategy ends up looking like the following, which seems like a lot of
extra code to do nothing with the array entry. But if this is what you all
prefer, I can send it as v2.

Regards,
Samuel

diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
index ce9a995730c1..cbf8d9a88f45 100644
--- a/arch/riscv/include/asm/cpufeature.h
+++ b/arch/riscv/include/asm/cpufeature.h
@@ -58,6 +58,10 @@ void __init riscv_user_isa_enable(void);
 #define __RISCV_ISA_EXT_SUPERSET_VALIDATE(_name, _id, _sub_exts, _validate) \
 	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), _validate)
 
+#define __RISCV_ISA_EXT_INTERNAL(_id) {							\
+	.id = _id,									\
+}
+
 #if defined(CONFIG_RISCV_MISALIGNED)
 bool check_unaligned_access_emulated_all_cpus(void);
 void unaligned_emulation_finish(void);
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index f6b13e9f5e6c..59562ed13986 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -271,6 +271,9 @@ static void print_isa(struct seq_file *f, const unsigned long *isa_bitmap, int c
 		seq_write(f, "rv64", 4);
 
 	for (int i = 0; i < riscv_isa_ext_count; i++) {
+		if (!riscv_isa_ext[i].name)
+			continue;
+
 		if (!__riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].id))
 			continue;
 
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 68b8f24b630d..e4dd837d7bea 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -387,6 +387,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
 	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
 	__RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT),
 	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
+	__RISCV_ISA_EXT_INTERNAL(RISCV_ISA_EXT_XLINUXENVCFG),
 };
 
 const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
@@ -469,7 +470,8 @@ static void __init match_isa_ext(const char *name, const char *name_end, unsigne
 	for (int i = 0; i < riscv_isa_ext_count; i++) {
 		const struct riscv_isa_ext_data *ext = &riscv_isa_ext[i];
 
-		if ((name_end - name == strlen(ext->name)) &&
+		if (ext->name &&
+		    (name_end - name == strlen(ext->name)) &&
 		    !strncasecmp(name, ext->name, name_end - name)) {
 			riscv_isa_set_ext(ext, bitmap);
 			break;
@@ -734,6 +736,9 @@ static void __init riscv_fill_cpu_vendor_ext(struct device_node *cpu_node, int c
 			const struct riscv_isa_ext_data ext = ext_list->ext_data[j];
 			struct riscv_isavendorinfo *isavendorinfo = &ext_list->per_hart_isa_bitmap[cpu];
 
+			if (!ext.property)
+				continue;
+
 			if (of_property_match_string(cpu_node, "riscv,isa-extensions",
 						     ext.property) < 0)
 				continue;
@@ -801,6 +806,9 @@ static int __init riscv_fill_hwcap_from_ext_list(unsigned long *isa2hwcap)
 		for (int i = 0; i < riscv_isa_ext_count; i++) {
 			const struct riscv_isa_ext_data *ext = &riscv_isa_ext[i];
 
+			if (!ext->property)
+				continue;
+
 			if (of_property_match_string(cpu_node, "riscv,isa-extensions",
 						     ext->property) < 0)
 				continue;


>>
>> Fixes: 625034abd52a ("riscv: add ISA extensions validation callback")
>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>> ---
>>
>>  arch/riscv/kernel/cpufeature.c | 14 ++++++--------
>>  1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> index 0366dc3baf33..dd25677d60de 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -457,28 +457,26 @@ 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;
>>  					continue;
>>  				} else if (ret) {
>>  					/* Disable the extension entirely */
>> -					clear_bit(ext->id, source_isa);
>> +					clear_bit(bit, source_isa);
>>  					continue;
>>  				}
>>  			}
>>  
>> -			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.45.1
>>
> 
> If we'd rather leave Xlinuxenvcfg out of the array (and generally support
> extensions not in the array), then LGTM
> 
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> 
> Thanks,
> drew


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