| Message ID | 20240718213011.2600150-1-samuel.holland@sifive.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b75a22e7d4f23dcd4f78ed2ff368a3d2a4556c0c |
| Headers | show |
| Series | [-fixes] riscv: cpufeature: Do not drop Linux-internal extensions | expand |
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(-)
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
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.
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
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!
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))); }
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(-)