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

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

[Samuel Holland]

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.

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

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

[Samuel Holland]

On 2024-07-18 4:29 PM, 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.
> 
> Fixes: 625034abd52a ("riscv: add ISA extensions validation callback")

Apologies for the incorrect subject line. This fixes a commit in for-next, so
this patch is targeting for-next.

> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> ---
> 
>  arch/riscv/kernel/cpufeature.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)

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

[Andrew Jones]

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
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;
    }
 }

> 
> 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

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

[Clément Léger]

On 18/07/2024 23:57, 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
> 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;
>     }
>  }

I would rather add it to the riscv_isa_ext[] array and avoid handling it
differently. There is already the xandespmu extension in this array so
xlinuxenvcfg can be added as well.

Thanks,

Clément

> 
>>
>> 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

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1615 bytes --]

On Fri, Jul 19, 2024 at 09:11:20AM +0200, Clément Léger wrote:
> 
> 
> On 18/07/2024 23:57, 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
> > 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;
> >     }
> >  }
> 
> I would rather add it to the riscv_isa_ext[] array and avoid handling it
> differently. There is already the xandespmu extension in this array so
> xlinuxenvcfg can be added as well.

xandespmu and xlinuxenvcfg are fundamentally different, the former is
parsed from devicetree and is a real extension. xlinuxengcfg is an
internal flag. I don't think we should be printing it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Clément Léger]

On 19/07/2024 10:25, Conor Dooley wrote:
> On Fri, Jul 19, 2024 at 09:11:20AM +0200, Clément Léger wrote:
>>
>>
>> On 18/07/2024 23:57, 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
>>> 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;
>>>     }
>>>  }
>>
>> I would rather add it to the riscv_isa_ext[] array and avoid handling it
>> differently. There is already the xandespmu extension in this array so
>> xlinuxenvcfg can be added as well.
> 
> xandespmu and xlinuxenvcfg are fundamentally different, the former is
> parsed from devicetree and is a real extension. xlinuxengcfg is an
> internal flag. I don't think we should be printing it.

Sorry, I did not meant it was to be displayed. Only added to
riscv_isa_ext[] as pointed out by Andrew.

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

[Samuel Holland]

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

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

[patchwork-bot+linux-riscv]

Hello:

This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Thu, 18 Jul 2024 14:29:59 -0700 you 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.
> 
> [...]

Here is the summary with links:
  - [-fixes] riscv: cpufeature: Do not drop Linux-internal extensions
    https://git.kernel.org/riscv/c/b75a22e7d4f2

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html