arch/riscv/kernel/cpufeature.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
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.1On 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
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
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
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.
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.
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(-)
© 2016 - 2025 Red Hat, Inc.