[v3] RISC-V: Ignore V from the riscv,isa DT property on older T-Head CPUs

Commit Message

Conor Dooley Feb. 23, 2024, 11:31 a.m. UTC

From: Palmer Dabbelt <[e-mail]>

Before attempting to support the pre-ratification version of vector
found on older T-Head CPUs, disallow "v" in riscv,isa on these
platforms. The deprecated property has no clear way to communicate
the specific version of vector that is supported and much of the vendor
provided software puts "v" in the isa string. riscv,isa-extensions
should be used instead. This should not be too much of a burden for
these systems, as the vendor shipped devicetrees and firmware do not
work with a mainline kernel and will require updating.

We can limit this restriction to only ignore v in riscv,isa on CPUs
that report T-Head's vendor ID and a zero marchid. Newer T-Head CPUs
that support the ratified version of vector should report non-zero
marchid, according to Guo Ren [1].

Link: https://lore.kernel.org/linux-riscv/CAJF2gTRy5eK73=d6s7CVy9m9pB8p4rAoMHM3cZFwzg=[e-mail]/ [1]
Fixes: dc6667a4e7e3 ("riscv: Extending cpufeature.c to detect V-extension")
Signed-off-by: Palmer Dabbelt <[e-mail]>
Co-developed-by: Conor Dooley <[e-mail]>
Signed-off-by: Conor Dooley <[e-mail]>
---
I'd forgotten about this patch, the plan had been to sit on it until
someone actually reported encountering this, but Palmer asked me to
resend this as part of reviving the attempt to mainline the basic
support for vector on these devices.

I guess the riscv,isa-extensions support actually landed after the v2
was written, so v3 is less complicated since there's full support for
parsing the new property and all we have to do is block V in the !acpi
case when parsing riscv,isa now. As Guo Ren suggested, I decreased the
size of the hammer further, so that the c908, that does implment the
ratified V extension, can set "v" in riscv,isa. Other than that, what
changed is just a rebase on top of the changes since July. v2 is in the
Link: tag above.

cc: [e-mail]
cc: [e-mail]
cc: [e-mail]
cc: [e-mail]
cc: [e-mail]
---
 arch/riscv/kernel/cpufeature.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Guo Ren Feb. 23, 2024, 11:38 a.m. UTC | #1

On Fri, Feb 23, 2024 at 7:31 PM Conor Dooley <[e-mail]> wrote:
>
> From: Palmer Dabbelt <[e-mail]>
>
> Before attempting to support the pre-ratification version of vector
> found on older T-Head CPUs, disallow "v" in riscv,isa on these
> platforms. The deprecated property has no clear way to communicate
> the specific version of vector that is supported and much of the vendor
> provided software puts "v" in the isa string. riscv,isa-extensions
> should be used instead. This should not be too much of a burden for
> these systems, as the vendor shipped devicetrees and firmware do not
> work with a mainline kernel and will require updating.
>
> We can limit this restriction to only ignore v in riscv,isa on CPUs
> that report T-Head's vendor ID and a zero marchid. Newer T-Head CPUs
> that support the ratified version of vector should report non-zero
> marchid, according to Guo Ren [1].
Acked-by: Guo Ren <[e-mail]>

Thx, this helps the newest application disable vector_10 when on the
old XuanTie processor.

>
> Link: https://lore.kernel.org/linux-riscv/CAJF2gTRy5eK73=d6s7CVy9m9pB8p4rAoMHM3cZFwzg=[e-mail]/ [1]
> Fixes: dc6667a4e7e3 ("riscv: Extending cpufeature.c to detect V-extension")
> Signed-off-by: Palmer Dabbelt <[e-mail]>
> Co-developed-by: Conor Dooley <[e-mail]>
> Signed-off-by: Conor Dooley <[e-mail]>
> ---
> I'd forgotten about this patch, the plan had been to sit on it until
> someone actually reported encountering this, but Palmer asked me to
> resend this as part of reviving the attempt to mainline the basic
> support for vector on these devices.
>
> I guess the riscv,isa-extensions support actually landed after the v2
> was written, so v3 is less complicated since there's full support for
> parsing the new property and all we have to do is block V in the !acpi
> case when parsing riscv,isa now. As Guo Ren suggested, I decreased the
> size of the hammer further, so that the c908, that does implment the
> ratified V extension, can set "v" in riscv,isa. Other than that, what
> changed is just a rebase on top of the changes since July. v2 is in the
> Link: tag above.
>
> cc: [e-mail]
> cc: [e-mail]
> cc: [e-mail]
> cc: [e-mail]
> cc: [e-mail]
> ---
>  arch/riscv/kernel/cpufeature.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 89920f84d0a3..3a05af7be510 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -24,6 +24,7 @@
>  #include <asm/hwprobe.h>
>  #include <asm/patch.h>
>  #include <asm/processor.h>
> +#include <asm/sbi.h>
>  #include <asm/vector.h>
>
>  #include "copy-unaligned.h"
> @@ -538,6 +539,20 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>                         set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
>                 }
>
> +               /*
> +                * "V" in ISA strings is ambiguous in practice: it should mean
> +                * just the standard V-1.0 but vendors aren't well behaved.
> +                * Many vendors with T-Head CPU cores which implement the 0.7.1
> +                * version of the vector specification put "v" into their DTs.
> +                * CPU cores with the ratified spec will contain non-zero
> +                * marchid.
> +                */
> +               if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID &&
> +                   riscv_cached_marchid(cpu) == 0x0) {
> +                       this_hwcap &= ~isa2hwcap[RISCV_ISA_EXT_v];
> +                       clear_bit(RISCV_ISA_EXT_v, isainfo->isa);
> +               }
> +
>                 /*
>                  * All "okay" hart should have same isa. Set HWCAP based on
>                  * common capabilities of every "okay" hart, in case they don't
> --
> 2.43.0
>

Palmer Dabbelt Feb. 23, 2024, 5:03 p.m. UTC | #2

On Fri, 23 Feb 2024 03:38:17 PST (-0800), [e-mail] wrote:
> On Fri, Feb 23, 2024 at 7:31 PM Conor Dooley <[e-mail]> wrote:
>>
>> From: Palmer Dabbelt <[e-mail]>
>>
>> Before attempting to support the pre-ratification version of vector
>> found on older T-Head CPUs, disallow "v" in riscv,isa on these
>> platforms. The deprecated property has no clear way to communicate
>> the specific version of vector that is supported and much of the vendor
>> provided software puts "v" in the isa string. riscv,isa-extensions
>> should be used instead. This should not be too much of a burden for
>> these systems, as the vendor shipped devicetrees and firmware do not
>> work with a mainline kernel and will require updating.
>>
>> We can limit this restriction to only ignore v in riscv,isa on CPUs
>> that report T-Head's vendor ID and a zero marchid. Newer T-Head CPUs
>> that support the ratified version of vector should report non-zero
>> marchid, according to Guo Ren [1].
> Acked-by: Guo Ren <[e-mail]>
>
> Thx, this helps the newest application disable vector_10 when on the
> old XuanTie processor.

Thanks.

I queued this up for testing for fixes.  Probably not any meaningful 
coverage there because I just test in QEMU, but it should show up soon.

>
>>
>> Link: https://lore.kernel.org/linux-riscv/CAJF2gTRy5eK73=d6s7CVy9m9pB8p4rAoMHM3cZFwzg=[e-mail]/ [1]
>> Fixes: dc6667a4e7e3 ("riscv: Extending cpufeature.c to detect V-extension")
>> Signed-off-by: Palmer Dabbelt <[e-mail]>
>> Co-developed-by: Conor Dooley <[e-mail]>
>> Signed-off-by: Conor Dooley <[e-mail]>
>> ---
>> I'd forgotten about this patch, the plan had been to sit on it until
>> someone actually reported encountering this, but Palmer asked me to
>> resend this as part of reviving the attempt to mainline the basic
>> support for vector on these devices.
>>
>> I guess the riscv,isa-extensions support actually landed after the v2
>> was written, so v3 is less complicated since there's full support for
>> parsing the new property and all we have to do is block V in the !acpi
>> case when parsing riscv,isa now. As Guo Ren suggested, I decreased the
>> size of the hammer further, so that the c908, that does implment the
>> ratified V extension, can set "v" in riscv,isa. Other than that, what
>> changed is just a rebase on top of the changes since July. v2 is in the
>> Link: tag above.
>>
>> cc: [e-mail]
>> cc: [e-mail]
>> cc: [e-mail]
>> cc: [e-mail]
>> cc: [e-mail]
>> ---
>>  arch/riscv/kernel/cpufeature.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> index 89920f84d0a3..3a05af7be510 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -24,6 +24,7 @@
>>  #include <asm/hwprobe.h>
>>  #include <asm/patch.h>
>>  #include <asm/processor.h>
>> +#include <asm/sbi.h>
>>  #include <asm/vector.h>
>>
>>  #include "copy-unaligned.h"
>> @@ -538,6 +539,20 @@ static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
>>                         set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
>>                 }
>>
>> +               /*
>> +                * "V" in ISA strings is ambiguous in practice: it should mean
>> +                * just the standard V-1.0 but vendors aren't well behaved.
>> +                * Many vendors with T-Head CPU cores which implement the 0.7.1
>> +                * version of the vector specification put "v" into their DTs.
>> +                * CPU cores with the ratified spec will contain non-zero
>> +                * marchid.
>> +                */
>> +               if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID &&
>> +                   riscv_cached_marchid(cpu) == 0x0) {
>> +                       this_hwcap &= ~isa2hwcap[RISCV_ISA_EXT_v];
>> +                       clear_bit(RISCV_ISA_EXT_v, isainfo->isa);
>> +               }
>> +
>>                 /*
>>                  * All "okay" hart should have same isa. Set HWCAP based on
>>                  * common capabilities of every "okay" hart, in case they don't
>> --
>> 2.43.0
>>
>
>
> -- 
> Best Regards
>  Guo Ren
>
> _______________________________________________
> linux-riscv mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

[e-mail] Feb. 23, 2024, 6:40 p.m. UTC | #3

Hello:

This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <[e-mail]>:

On Fri, 23 Feb 2024 11:31:31 +0000 you wrote:
> From: Palmer Dabbelt <[e-mail]>
> 
> Before attempting to support the pre-ratification version of vector
> found on older T-Head CPUs, disallow "v" in riscv,isa on these
> platforms. The deprecated property has no clear way to communicate
> the specific version of vector that is supported and much of the vendor
> provided software puts "v" in the isa string. riscv,isa-extensions
> should be used instead. This should not be too much of a burden for
> these systems, as the vendor shipped devicetrees and firmware do not
> work with a mainline kernel and will require updating.
> 
> [...]

Here is the summary with links:
  - [v3] RISC-V: Ignore V from the riscv,isa DT property on older T-Head CPUs
    https://git.kernel.org/riscv/c/d82f32202e0d

You are awesome, thank you!

Patch

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 89920f84d0a3..3a05af7be510 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -24,6 +24,7 @@ 
 #include <asm/hwprobe.h>
 #include <asm/patch.h>
 #include <asm/processor.h>
+#include <asm/sbi.h>
 #include <asm/vector.h>
 
 #include "copy-unaligned.h"
@@ -538,6 +539,20 @@  static void __init riscv_fill_hwcap_from_isa_string(unsigned long *isa2hwcap)
 			set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
 		}
 
+		/*
+		 * "V" in ISA strings is ambiguous in practice: it should mean
+		 * just the standard V-1.0 but vendors aren't well behaved.
+		 * Many vendors with T-Head CPU cores which implement the 0.7.1
+		 * version of the vector specification put "v" into their DTs.
+		 * CPU cores with the ratified spec will contain non-zero
+		 * marchid.
+		 */
+		if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID &&
+		    riscv_cached_marchid(cpu) == 0x0) {
+			this_hwcap &= ~isa2hwcap[RISCV_ISA_EXT_v];
+			clear_bit(RISCV_ISA_EXT_v, isainfo->isa);
+		}
+
 		/*
 		 * All "okay" hart should have same isa. Set HWCAP based on
 		 * common capabilities of every "okay" hart, in case they don't