Re: [PATCH -fixes] riscv: cpufeature: Do not drop Linux-internal extensions
From: Samuel Holland
Date: Mon Jul 29 2024 - 18:25:28 EST
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@xxxxxxxxxx>
>> ---
>>
>> 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@xxxxxxxxxxxxxxxx>
>
> Thanks,
> drew