[v2] RISC-V: Don't trust V from the riscv,isa DT property on T-Head CPUs

Commit Message

Conor Dooley July 12, 2023, 5:48 p.m. UTC

From: Palmer Dabbelt <[e-mail]>

The last merge window contained both V support and the deprecation of
the riscv,isa DT property, with the V implementation reading riscv,isa
to determine the presence of the V extension.  At the time that was the
only way to do it, but there's a lot of ambiguity around V in ISA
strings.  In particular, there is a lot of firmware in the wild that
uses "v" in the riscv,isa DT property to communicate support for the
0.7.1 version of the Vector specification implemented by T-Head CPU
cores.

Rather than forcing use of the newly added interface that has strict
meanings for extensions to detect the presence of vector support, as
that would penalise those who have behaved, only ignore v in riscv,isa
on CPUs that report T-Head's vendor ID.

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]>
---
Changes in v2:
- Use my version of the patch that touches hwcap and isainfo uniformly
- Don't penalise those who behaved
---
 arch/riscv/kernel/cpufeature.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Jisheng Zhang July 13, 2023, 4:36 p.m. UTC | #1

On Wed, Jul 12, 2023 at 06:48:02PM +0100, Conor Dooley wrote:
> From: Palmer Dabbelt <[e-mail]>
> 
> The last merge window contained both V support and the deprecation of
> the riscv,isa DT property, with the V implementation reading riscv,isa
> to determine the presence of the V extension.  At the time that was the
> only way to do it, but there's a lot of ambiguity around V in ISA
> strings.  In particular, there is a lot of firmware in the wild that
> uses "v" in the riscv,isa DT property to communicate support for the
> 0.7.1 version of the Vector specification implemented by T-Head CPU
> cores.

Add Guo

Hi Conor, Palmer,

FWICT, new T-HEAD's riscv cores such as C908 support standard RVV-1.0,
this patch looks like a big hammer for T-HEAD. I do understand why
this patch is provided, but can we mitigate the situation by carefully
review the DTs? Per my understanding, dts is also part of linux kernel.

Thanks

> 
> Rather than forcing use of the newly added interface that has strict
> meanings for extensions to detect the presence of vector support, as
> that would penalise those who have behaved, only ignore v in riscv,isa
> on CPUs that report T-Head's vendor ID.
> 
> 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]>
> ---
> Changes in v2:
> - Use my version of the patch that touches hwcap and isainfo uniformly
> - Don't penalise those who behaved
> ---
>  arch/riscv/kernel/cpufeature.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index bdcf460ea53d..05362715e1b7 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -21,6 +21,7 @@
>  #include <asm/hwcap.h>
>  #include <asm/patch.h>
>  #include <asm/processor.h>
> +#include <asm/sbi.h>
>  #include <asm/vector.h>
>  
>  #define NUM_ALPHA_EXTS ('z' - 'a' + 1)
> @@ -334,6 +335,27 @@ void __init riscv_fill_hwcap(void)
>  			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
> +		 * and no T-Head CPU cores with the standard version of vector
> +		 * are in circulation yet.
> +		 * Platforms with T-Head CPU cores that support the standard
> +		 * version of vector must provide the explicit V property,
> +		 * which is well defined.
> +		 */
> +		if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID) {
> +			if (of_property_match_string(node, "riscv,isa-extensions", "v") >= 0) {
> +				this_hwcap |= isa2hwcap[RISCV_ISA_EXT_v];
> +				set_bit(RISCV_ISA_EXT_v, isainfo->isa);
> +			} else {
> +				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.39.2
> 
> 
> _______________________________________________
> linux-riscv mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Palmer Dabbelt July 13, 2023, 4:56 p.m. UTC | #2

On Thu, 13 Jul 2023 09:36:49 PDT (-0700), [e-mail] wrote:
> On Wed, Jul 12, 2023 at 06:48:02PM +0100, Conor Dooley wrote:
>> From: Palmer Dabbelt <[e-mail]>
>>
>> The last merge window contained both V support and the deprecation of
>> the riscv,isa DT property, with the V implementation reading riscv,isa
>> to determine the presence of the V extension.  At the time that was the
>> only way to do it, but there's a lot of ambiguity around V in ISA
>> strings.  In particular, there is a lot of firmware in the wild that
>> uses "v" in the riscv,isa DT property to communicate support for the
>> 0.7.1 version of the Vector specification implemented by T-Head CPU
>> cores.
>
> Add Guo
>
> Hi Conor, Palmer,
>
> FWICT, new T-HEAD's riscv cores such as C908 support standard RVV-1.0,
> this patch looks like a big hammer for T-HEAD. I do understand why

Ya, it's a big hammer.  There's no extant systems with the C908, though, 
and given that the C906 and C910 alias marchid/mimplid it's kind of hard 
to trust any of those values for T-Head systems.  We could check for the 
0s and hope T-Head starts setting something else, but I'm not sure 
that's a net win (we've also got the C920 in the Sophgo chip, which IIUC 
is V-0.7.1 too).

> this patch is provided, but can we mitigate the situation by carefully
> review the DTs? Per my understanding, dts is also part of linux kernel.

That would break compatibility with existing firmware.  It's certainly 
something that has happened before, but we try to avoid it where 
possible.

In this case new T-Head systems that have standard V would just need to 
provide the new DT properties instead of the ISA string property.  We've 
deprecated the ISA string property already so that's what new systems 
should be doing anyway, and given that this only applies to new systems 
it doesn't seem like a big ask.

> Thanks
>
>>
>> Rather than forcing use of the newly added interface that has strict
>> meanings for extensions to detect the presence of vector support, as
>> that would penalise those who have behaved, only ignore v in riscv,isa
>> on CPUs that report T-Head's vendor ID.
>>
>> 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]>
>> ---
>> Changes in v2:
>> - Use my version of the patch that touches hwcap and isainfo uniformly
>> - Don't penalise those who behaved
>> ---
>>  arch/riscv/kernel/cpufeature.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> index bdcf460ea53d..05362715e1b7 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -21,6 +21,7 @@
>>  #include <asm/hwcap.h>
>>  #include <asm/patch.h>
>>  #include <asm/processor.h>
>> +#include <asm/sbi.h>
>>  #include <asm/vector.h>
>>
>>  #define NUM_ALPHA_EXTS ('z' - 'a' + 1)
>> @@ -334,6 +335,27 @@ void __init riscv_fill_hwcap(void)
>>  			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
>> +		 * and no T-Head CPU cores with the standard version of vector
>> +		 * are in circulation yet.
>> +		 * Platforms with T-Head CPU cores that support the standard
>> +		 * version of vector must provide the explicit V property,
>> +		 * which is well defined.
>> +		 */
>> +		if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID) {
>> +			if (of_property_match_string(node, "riscv,isa-extensions", "v") >= 0) {
>> +				this_hwcap |= isa2hwcap[RISCV_ISA_EXT_v];
>> +				set_bit(RISCV_ISA_EXT_v, isainfo->isa);
>> +			} else {
>> +				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.39.2
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> [e-mail]
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

Conor Dooley July 13, 2023, 5:04 p.m. UTC | #3

Jumping on top of Palmer's reply cos I had already started replying...

On Thu, Jul 13, 2023 at 09:56:34AM -0700, Palmer Dabbelt wrote:
> On Thu, 13 Jul 2023 09:36:49 PDT (-0700), [e-mail] wrote:
> > On Wed, Jul 12, 2023 at 06:48:02PM +0100, Conor Dooley wrote:
> > > From: Palmer Dabbelt <[e-mail]>
> > > 
> > > The last merge window contained both V support and the deprecation of
> > > the riscv,isa DT property, with the V implementation reading riscv,isa
> > > to determine the presence of the V extension.  At the time that was the
> > > only way to do it, but there's a lot of ambiguity around V in ISA
> > > strings.  In particular, there is a lot of firmware in the wild that
> > > uses "v" in the riscv,isa DT property to communicate support for the
> > > 0.7.1 version of the Vector specification implemented by T-Head CPU
> > > cores.
> > 
> > Add Guo
> > 
> > Hi Conor, Palmer,
> > 
> > FWICT, new T-HEAD's riscv cores such as C908 support standard RVV-1.0,
> > this patch looks like a big hammer for T-HEAD. I do understand why
> 
> Ya, it's a big hammer.  There's no extant systems with the C908, though, and
> given that the C906 and C910 alias marchid/mimplid it's kind of hard to
> trust any of those values for T-Head systems.  We could check for the 0s and
> hope T-Head starts setting something else, but I'm not sure that's a net win
> (we've also got the C920 in the Sophgo chip, which IIUC is V-0.7.1 too).

(In reply to Jisheng mostly)
It is most definitely a big hammer. And yes, we did talk about the c908
& its standard implementation of vector before submitting this. Unless
Guo can confirm that the c908 (and later CPU cores) will start setting
mimpid & mvendorid, I don't really see what the alternatives are? *
Whacking in a list of DT compatibles to blacklist? That doesn't seem
like something that would scale.
Open to ideas on that front for sure, smaller hammers are always better!

@Palmer, from what I am told, the c920 does put zeros in those CSRs,
so we are okay on that front.

* If they do do something other than 0s, the errata handling will need
  an update anyway, so the big hammer could be revised in tandem...

> > this patch is provided, but can we mitigate the situation by carefully
> > review the DTs? Per my understanding, dts is also part of linux kernel.
> 
> That would break compatibility with existing firmware.  It's certainly
> something that has happened before, but we try to avoid it where possible.

(Mostly in reply to Jisheng again)
Sure, some devicetrees are part of the kernel, but not all are - they may
be passed up from U-Boot or OpenSBI etc & contain "v" in riscv,isa.
The intent of this patch (and Palmer's v1) is to make sure such systems
do not tell the kernel they support the standard version of v, when they
do not do so.

> In this case new T-Head systems that have standard V would just need to
> provide the new DT properties instead of the ISA string property.  We've
> deprecated the ISA string property already so that's what new systems should
> be doing anyway, and given that this only applies to new systems it doesn't
> seem like a big ask.

Aye, AFAIK there are no extant systems with a c908 outside of vendors?
At least, from what searching I did I could not find a way to acquire
one... If you read the patch, you will see that there is in fact a way
out being provided & it is not a "no T-Head can never have vector"
situation - just slightly earlier use of the new properties in the
kernel than we perhaps intended.

Thanks,
Conor.

> > > Rather than forcing use of the newly added interface that has strict
> > > meanings for extensions to detect the presence of vector support, as
> > > that would penalise those who have behaved, only ignore v in riscv,isa
> > > on CPUs that report T-Head's vendor ID.
> > > 
> > > 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]>
> > > ---
> > > Changes in v2:
> > > - Use my version of the patch that touches hwcap and isainfo uniformly
> > > - Don't penalise those who behaved
> > > ---
> > >  arch/riscv/kernel/cpufeature.c | 22 ++++++++++++++++++++++
> > >  1 file changed, 22 insertions(+)
> > > 
> > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > > index bdcf460ea53d..05362715e1b7 100644
> > > --- a/arch/riscv/kernel/cpufeature.c
> > > +++ b/arch/riscv/kernel/cpufeature.c
> > > @@ -21,6 +21,7 @@
> > >  #include <asm/hwcap.h>
> > >  #include <asm/patch.h>
> > >  #include <asm/processor.h>
> > > +#include <asm/sbi.h>
> > >  #include <asm/vector.h>
> > > 
> > >  #define NUM_ALPHA_EXTS ('z' - 'a' + 1)
> > > @@ -334,6 +335,27 @@ void __init riscv_fill_hwcap(void)
> > >  			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
> > > +		 * and no T-Head CPU cores with the standard version of vector
> > > +		 * are in circulation yet.
> > > +		 * Platforms with T-Head CPU cores that support the standard
> > > +		 * version of vector must provide the explicit V property,
> > > +		 * which is well defined.
> > > +		 */
> > > +		if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID) {
> > > +			if (of_property_match_string(node, "riscv,isa-extensions", "v") >= 0) {
> > > +				this_hwcap |= isa2hwcap[RISCV_ISA_EXT_v];
> > > +				set_bit(RISCV_ISA_EXT_v, isainfo->isa);
> > > +			} else {
> > > +				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.39.2
> > > 
> > > 
> > > _______________________________________________
> > > linux-riscv mailing list
> > > [e-mail]
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv

Jisheng Zhang July 13, 2023, 5:12 p.m. UTC | #4

On Thu, Jul 13, 2023 at 06:04:22PM +0100, Conor Dooley wrote:
> Jumping on top of Palmer's reply cos I had already started replying...
> 
> On Thu, Jul 13, 2023 at 09:56:34AM -0700, Palmer Dabbelt wrote:
> > On Thu, 13 Jul 2023 09:36:49 PDT (-0700), [e-mail] wrote:
> > > On Wed, Jul 12, 2023 at 06:48:02PM +0100, Conor Dooley wrote:
> > > > From: Palmer Dabbelt <[e-mail]>
> > > > 
> > > > The last merge window contained both V support and the deprecation of
> > > > the riscv,isa DT property, with the V implementation reading riscv,isa
> > > > to determine the presence of the V extension.  At the time that was the
> > > > only way to do it, but there's a lot of ambiguity around V in ISA
> > > > strings.  In particular, there is a lot of firmware in the wild that
> > > > uses "v" in the riscv,isa DT property to communicate support for the
> > > > 0.7.1 version of the Vector specification implemented by T-Head CPU
> > > > cores.
> > > 
> > > Add Guo
> > > 
> > > Hi Conor, Palmer,
> > > 
> > > FWICT, new T-HEAD's riscv cores such as C908 support standard RVV-1.0,
> > > this patch looks like a big hammer for T-HEAD. I do understand why
> > 
> > Ya, it's a big hammer.  There's no extant systems with the C908, though, and
> > given that the C906 and C910 alias marchid/mimplid it's kind of hard to
> > trust any of those values for T-Head systems.  We could check for the 0s and
> > hope T-Head starts setting something else, but I'm not sure that's a net win
> > (we've also got the C920 in the Sophgo chip, which IIUC is V-0.7.1 too).
> 
> (In reply to Jisheng mostly)
> It is most definitely a big hammer. And yes, we did talk about the c908
> & its standard implementation of vector before submitting this. Unless
> Guo can confirm that the c908 (and later CPU cores) will start setting
> mimpid & mvendorid, I don't really see what the alternatives are? *

In mainline kernel, three SoCs which powered by T-HEAD cpu are
supported: D1, D1s and TH1520, they don't contain the "v" in riscv,isa
dt property.

> Whacking in a list of DT compatibles to blacklist? That doesn't seem
> like something that would scale.
> Open to ideas on that front for sure, smaller hammers are always better!
> 
> @Palmer, from what I am told, the c920 does put zeros in those CSRs,
> so we are okay on that front.
> 
> * If they do do something other than 0s, the errata handling will need
>   an update anyway, so the big hammer could be revised in tandem...
> 
> > > this patch is provided, but can we mitigate the situation by carefully
> > > review the DTs? Per my understanding, dts is also part of linux kernel.
> > 
> > That would break compatibility with existing firmware.  It's certainly
> > something that has happened before, but we try to avoid it where possible.
> 
> (Mostly in reply to Jisheng again)
> Sure, some devicetrees are part of the kernel, but not all are - they may
> be passed up from U-Boot or OpenSBI etc & contain "v" in riscv,isa.

If so this looks like a bug of u-boot and opensbi.

PS: does u-boot/opensbi modify "riscv,isa" property dynamically? Or
there's below usage case:
mainline linux kernel + dtb which is built from u-boot/opensbi source
code rather than linux kernel.


> The intent of this patch (and Palmer's v1) is to make sure such systems
> do not tell the kernel they support the standard version of v, when they
> do not do so.
> 
> > In this case new T-Head systems that have standard V would just need to
> > provide the new DT properties instead of the ISA string property.  We've
> > deprecated the ISA string property already so that's what new systems should
> > be doing anyway, and given that this only applies to new systems it doesn't
> > seem like a big ask.
> 
> Aye, AFAIK there are no extant systems with a c908 outside of vendors?
> At least, from what searching I did I could not find a way to acquire
> one... If you read the patch, you will see that there is in fact a way
> out being provided & it is not a "no T-Head can never have vector"
> situation - just slightly earlier use of the new properties in the
> kernel than we perhaps intended.

This new properties look like a solution for T-HEAD vector support.
Guo may have comments.

Thank you.

> 
> Thanks,
> Conor.
> 
> > > > Rather than forcing use of the newly added interface that has strict
> > > > meanings for extensions to detect the presence of vector support, as
> > > > that would penalise those who have behaved, only ignore v in riscv,isa
> > > > on CPUs that report T-Head's vendor ID.
> > > > 
> > > > 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]>
> > > > ---
> > > > Changes in v2:
> > > > - Use my version of the patch that touches hwcap and isainfo uniformly
> > > > - Don't penalise those who behaved
> > > > ---
> > > >  arch/riscv/kernel/cpufeature.c | 22 ++++++++++++++++++++++
> > > >  1 file changed, 22 insertions(+)
> > > > 
> > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > > > index bdcf460ea53d..05362715e1b7 100644
> > > > --- a/arch/riscv/kernel/cpufeature.c
> > > > +++ b/arch/riscv/kernel/cpufeature.c
> > > > @@ -21,6 +21,7 @@
> > > >  #include <asm/hwcap.h>
> > > >  #include <asm/patch.h>
> > > >  #include <asm/processor.h>
> > > > +#include <asm/sbi.h>
> > > >  #include <asm/vector.h>
> > > > 
> > > >  #define NUM_ALPHA_EXTS ('z' - 'a' + 1)
> > > > @@ -334,6 +335,27 @@ void __init riscv_fill_hwcap(void)
> > > >  			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
> > > > +		 * and no T-Head CPU cores with the standard version of vector
> > > > +		 * are in circulation yet.
> > > > +		 * Platforms with T-Head CPU cores that support the standard
> > > > +		 * version of vector must provide the explicit V property,
> > > > +		 * which is well defined.
> > > > +		 */
> > > > +		if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID) {
> > > > +			if (of_property_match_string(node, "riscv,isa-extensions", "v") >= 0) {
> > > > +				this_hwcap |= isa2hwcap[RISCV_ISA_EXT_v];
> > > > +				set_bit(RISCV_ISA_EXT_v, isainfo->isa);
> > > > +			} else {
> > > > +				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.39.2
> > > > 
> > > > 
> > > > _______________________________________________
> > > > linux-riscv mailing list
> > > > [e-mail]
> > > > http://lists.infradead.org/mailman/listinfo/linux-riscv

Guo Ren July 13, 2023, 5:33 p.m. UTC | #5

On Thu, Jul 13, 2023 at 12:48 PM Jisheng Zhang <[e-mail]> wrote:
>
> On Wed, Jul 12, 2023 at 06:48:02PM +0100, Conor Dooley wrote:
> > From: Palmer Dabbelt <[e-mail]>
> >
> > The last merge window contained both V support and the deprecation of
> > the riscv,isa DT property, with the V implementation reading riscv,isa
> > to determine the presence of the V extension.  At the time that was the
> > only way to do it, but there's a lot of ambiguity around V in ISA
> > strings.  In particular, there is a lot of firmware in the wild that
> > uses "v" in the riscv,isa DT property to communicate support for the
> > 0.7.1 version of the Vector specification implemented by T-Head CPU
> > cores.
>
> Add Guo
>
> Hi Conor, Palmer,
>
> FWICT, new T-HEAD's riscv cores such as C908 support standard RVV-1.0,
> this patch looks like a big hammer for T-HEAD. I do understand why
> this patch is provided, but can we mitigate the situation by carefully
> review the DTs? Per my understanding, dts is also part of linux kernel.
>
> Thanks
>
> >
> > Rather than forcing use of the newly added interface that has strict
> > meanings for extensions to detect the presence of vector support, as
> > that would penalise those who have behaved, only ignore v in riscv,isa
> > on CPUs that report T-Head's vendor ID.
> >
> > 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]>
> > ---
> > Changes in v2:
> > - Use my version of the patch that touches hwcap and isainfo uniformly
> > - Don't penalise those who behaved
> > ---
> >  arch/riscv/kernel/cpufeature.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index bdcf460ea53d..05362715e1b7 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -21,6 +21,7 @@
> >  #include <asm/hwcap.h>
> >  #include <asm/patch.h>
> >  #include <asm/processor.h>
> > +#include <asm/sbi.h>
> >  #include <asm/vector.h>
> >
> >  #define NUM_ALPHA_EXTS ('z' - 'a' + 1)
> > @@ -334,6 +335,27 @@ void __init riscv_fill_hwcap(void)
> >                       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
> > +              * and no T-Head CPU cores with the standard version of vector
> > +              * are in circulation yet.
T-HEAD's vector 1.0 SoCs is in circulation. Kendryte K230 is the
shipped SoC chip, which vendor id = THEAD_VENDOR_ID and with vector
1.0.

> > +              * Platforms with T-Head CPU cores that support the standard
> > +              * version of vector must provide the explicit V property,
> > +              * which is well defined.
> > +              */
> > +             if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID) {
If you insist on doing this, please:

if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID &&
riscv_cached_marchid(cpu) == 0 && riscv_cached_mimpid(cpu) == 0) {

> > +                     if (of_property_match_string(node, "riscv,isa-extensions", "v") >= 0) {
> > +                             this_hwcap |= isa2hwcap[RISCV_ISA_EXT_v];
> > +                             set_bit(RISCV_ISA_EXT_v, isainfo->isa);
> > +                     } else {
> > +                             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.39.2
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > [e-mail]
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

Conor Dooley July 13, 2023, 5:36 p.m. UTC | #6

On Fri, Jul 14, 2023 at 01:12:32AM +0800, Jisheng Zhang wrote:
> On Thu, Jul 13, 2023 at 06:04:22PM +0100, Conor Dooley wrote:
> > Jumping on top of Palmer's reply cos I had already started replying...
> > On Thu, Jul 13, 2023 at 09:56:34AM -0700, Palmer Dabbelt wrote:
> > > On Thu, 13 Jul 2023 09:36:49 PDT (-0700), [e-mail] wrote:
> > > > On Wed, Jul 12, 2023 at 06:48:02PM +0100, Conor Dooley wrote:

> > > > FWICT, new T-HEAD's riscv cores such as C908 support standard RVV-1.0,
> > > > this patch looks like a big hammer for T-HEAD. I do understand why
> > > 
> > > Ya, it's a big hammer.  There's no extant systems with the C908, though, and
> > > given that the C906 and C910 alias marchid/mimplid it's kind of hard to
> > > trust any of those values for T-Head systems.  We could check for the 0s and
> > > hope T-Head starts setting something else, but I'm not sure that's a net win
> > > (we've also got the C920 in the Sophgo chip, which IIUC is V-0.7.1 too).
> > 
> > (In reply to Jisheng mostly)
> > It is most definitely a big hammer. And yes, we did talk about the c908
> > & its standard implementation of vector before submitting this. Unless
> > Guo can confirm that the c908 (and later CPU cores) will start setting
> > mimpid & mvendorid, I don't really see what the alternatives are? *
> 
> In mainline kernel, three SoCs which powered by T-HEAD cpu are
> supported: D1, D1s and TH1520, they don't contain the "v" in riscv,isa
> dt property.

Yup, and they will stay that way ;)

> > Whacking in a list of DT compatibles to blacklist? That doesn't seem
> > like something that would scale.
> > Open to ideas on that front for sure, smaller hammers are always better!
> > 
> > @Palmer, from what I am told, the c920 does put zeros in those CSRs,
> > so we are okay on that front.
> > 
> > * If they do do something other than 0s, the errata handling will need
> >   an update anyway, so the big hammer could be revised in tandem...
> > 
> > > > this patch is provided, but can we mitigate the situation by carefully
> > > > review the DTs? Per my understanding, dts is also part of linux kernel.
> > > 
> > > That would break compatibility with existing firmware.  It's certainly
> > > something that has happened before, but we try to avoid it where possible.
> > 
> > (Mostly in reply to Jisheng again)
> > Sure, some devicetrees are part of the kernel, but not all are - they may
> > be passed up from U-Boot or OpenSBI etc & contain "v" in riscv,isa.
> 
> If so this looks like a bug of u-boot and opensbi.
> 
> PS: does u-boot/opensbi modify "riscv,isa" property dynamically? Or
> there's below usage case:
> mainline linux kernel + dtb which is built from u-boot/opensbi source
> code rather than linux kernel.

Its the latter I am thinking of. If someone wants to go and double check
that there are no vendors shipping T-Head cores with firmware that
behaves that way, then the patch could I suppose be dropped.

Palmer Dabbelt July 13, 2023, 5:43 p.m. UTC | #7

On Thu, 13 Jul 2023 10:33:56 PDT (-0700), [e-mail] wrote:
> On Thu, Jul 13, 2023 at 12:48 PM Jisheng Zhang <[e-mail]> wrote:
>>
>> On Wed, Jul 12, 2023 at 06:48:02PM +0100, Conor Dooley wrote:
>> > From: Palmer Dabbelt <[e-mail]>
>> >
>> > The last merge window contained both V support and the deprecation of
>> > the riscv,isa DT property, with the V implementation reading riscv,isa
>> > to determine the presence of the V extension.  At the time that was the
>> > only way to do it, but there's a lot of ambiguity around V in ISA
>> > strings.  In particular, there is a lot of firmware in the wild that
>> > uses "v" in the riscv,isa DT property to communicate support for the
>> > 0.7.1 version of the Vector specification implemented by T-Head CPU
>> > cores.
>>
>> Add Guo
>>
>> Hi Conor, Palmer,
>>
>> FWICT, new T-HEAD's riscv cores such as C908 support standard RVV-1.0,
>> this patch looks like a big hammer for T-HEAD. I do understand why
>> this patch is provided, but can we mitigate the situation by carefully
>> review the DTs? Per my understanding, dts is also part of linux kernel.
>>
>> Thanks
>>
>> >
>> > Rather than forcing use of the newly added interface that has strict
>> > meanings for extensions to detect the presence of vector support, as
>> > that would penalise those who have behaved, only ignore v in riscv,isa
>> > on CPUs that report T-Head's vendor ID.
>> >
>> > 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]>
>> > ---
>> > Changes in v2:
>> > - Use my version of the patch that touches hwcap and isainfo uniformly
>> > - Don't penalise those who behaved
>> > ---
>> >  arch/riscv/kernel/cpufeature.c | 22 ++++++++++++++++++++++
>> >  1 file changed, 22 insertions(+)
>> >
>> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> > index bdcf460ea53d..05362715e1b7 100644
>> > --- a/arch/riscv/kernel/cpufeature.c
>> > +++ b/arch/riscv/kernel/cpufeature.c
>> > @@ -21,6 +21,7 @@
>> >  #include <asm/hwcap.h>
>> >  #include <asm/patch.h>
>> >  #include <asm/processor.h>
>> > +#include <asm/sbi.h>
>> >  #include <asm/vector.h>
>> >
>> >  #define NUM_ALPHA_EXTS ('z' - 'a' + 1)
>> > @@ -334,6 +335,27 @@ void __init riscv_fill_hwcap(void)
>> >                       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
>> > +              * and no T-Head CPU cores with the standard version of vector
>> > +              * are in circulation yet.
> T-HEAD's vector 1.0 SoCs is in circulation. Kendryte K230 is the
> shipped SoC chip, which vendor id = THEAD_VENDOR_ID and with vector
> 1.0.

Do you have a pointer to where they're availiable?  Kendryte's website 
doesn't list a K230 yet: https://www.canaan.io/product/kendryteai## .  
They'd be the first V-1.0 implementation in the wild, I bet a lot of 
people would want one to play with...

>
>> > +              * Platforms with T-Head CPU cores that support the standard
>> > +              * version of vector must provide the explicit V property,
>> > +              * which is well defined.
>> > +              */
>> > +             if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID) {
> If you insist on doing this, please:
>
> if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID &&
> riscv_cached_marchid(cpu) == 0 && riscv_cached_mimpid(cpu) == 0) {

From above it sounds like that's OK for the C920.  Does the C908 set a 
non-zero m{arch,impl}id?  If not, then this won't help anything.

Also: does the K230 work with upstream kernels yet?  I don't see any DT 
stuff for it, so aside from the unlikely event it's quirk-free then it'd 
need DTs changed by upstream review and thus would be forced to use the 
new properties anyway.

>> > +                     if (of_property_match_string(node, "riscv,isa-extensions", "v") >= 0) {
>> > +                             this_hwcap |= isa2hwcap[RISCV_ISA_EXT_v];
>> > +                             set_bit(RISCV_ISA_EXT_v, isainfo->isa);
>> > +                     } else {
>> > +                             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.39.2
>> >
>> >
>> > _______________________________________________
>> > linux-riscv mailing list
>> > [e-mail]
>> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
>
>
> -- 
> Best Regards
>  Guo Ren

Conor Dooley July 13, 2023, 5:45 p.m. UTC | #8

On Thu, Jul 13, 2023 at 01:33:56PM -0400, Guo Ren wrote:
> On Thu, Jul 13, 2023 at 12:48 PM Jisheng Zhang <[e-mail]> wrote:
> > On Wed, Jul 12, 2023 at 06:48:02PM +0100, Conor Dooley wrote:

> > > +             /*
> > > +              * "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
> > > +              * and no T-Head CPU cores with the standard version of vector
> > > +              * are in circulation yet.

> T-HEAD's vector 1.0 SoCs is in circulation. Kendryte K230 is the
> shipped SoC chip, which vendor id = THEAD_VENDOR_ID and with vector
> 1.0.

Where can I buy one, if it is in circulation?
Googling in English might not be the best thing to do, but doing so I
could find basically no information on the k230 - I did know it existed
and tried to find some info on it before sending the patch.
If it is not in circulation, then the comment is not inaccurate & when
they do arrive they can use the new dedicated property to convey support
for vector.

> > > +              * Platforms with T-Head CPU cores that support the standard
> > > +              * version of vector must provide the explicit V property,
> > > +              * which is well defined.
> > > +              */
> > > +             if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID) {
> If you insist on doing this, please:
> 
> if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID &&
> riscv_cached_marchid(cpu) == 0 && riscv_cached_mimpid(cpu) == 0) {

Why? Does the c908 report non-zero mimpid/marchid?
If yes, does it need either the errata for CMOs or the page tables?

Thanks,
Conor.

Conor Dooley July 13, 2023, 6:20 p.m. UTC | #9

On Thu, Jul 13, 2023 at 06:36:56PM +0100, Conor Dooley wrote:
> On Fri, Jul 14, 2023 at 01:12:32AM +0800, Jisheng Zhang wrote:
> > On Thu, Jul 13, 2023 at 06:04:22PM +0100, Conor Dooley wrote:
> > > Jumping on top of Palmer's reply cos I had already started replying...
> > > On Thu, Jul 13, 2023 at 09:56:34AM -0700, Palmer Dabbelt wrote:
> > > > On Thu, 13 Jul 2023 09:36:49 PDT (-0700), [e-mail] wrote:
> > > > > On Wed, Jul 12, 2023 at 06:48:02PM +0100, Conor Dooley wrote:

> > > > > this patch is provided, but can we mitigate the situation by carefully
> > > > > review the DTs? Per my understanding, dts is also part of linux kernel.
> > > > 
> > > > That would break compatibility with existing firmware.  It's certainly
> > > > something that has happened before, but we try to avoid it where possible.
> > > 
> > > (Mostly in reply to Jisheng again)
> > > Sure, some devicetrees are part of the kernel, but not all are - they may
> > > be passed up from U-Boot or OpenSBI etc & contain "v" in riscv,isa.
> > 
> > If so this looks like a bug of u-boot and opensbi.
> > 
> > PS: does u-boot/opensbi modify "riscv,isa" property dynamically? Or
> > there's below usage case:
> > mainline linux kernel + dtb which is built from u-boot/opensbi source
> > code rather than linux kernel.
> 
> Its the latter I am thinking of. If someone wants to go and double check
> that there are no vendors shipping T-Head cores with firmware that
> behaves that way, then the patch could I suppose be dropped.

I don't have one (yet), what is the boot flow in that regard with the
vendor shipped software on the lichee pi4a?
The D1 from the factory is a mess & I use Samuel's firmware/bootloaders,
so it is well behaved :)

Rémi Denis-Courmont July 13, 2023, 6:47 p.m. UTC | #10

Le torstaina 13. heinäkuuta 2023, 19.36.49 EEST Jisheng Zhang a écrit :
> On Wed, Jul 12, 2023 at 06:48:02PM +0100, Conor Dooley wrote:
> > From: Palmer Dabbelt <[e-mail]>
> > 
> > The last merge window contained both V support and the deprecation of
> > the riscv,isa DT property, with the V implementation reading riscv,isa
> > to determine the presence of the V extension.  At the time that was the
> > only way to do it, but there's a lot of ambiguity around V in ISA
> > strings.  In particular, there is a lot of firmware in the wild that
> > uses "v" in the riscv,isa DT property to communicate support for the
> > 0.7.1 version of the Vector specification implemented by T-Head CPU
> > cores.
> 
> Add Guo
> 
> Hi Conor, Palmer,
> 
> FWICT, new T-HEAD's riscv cores such as C908 support standard RVV-1.0,
> this patch looks like a big hammer for T-HEAD. I do understand why
> this patch is provided, but can we mitigate the situation by carefully
> review the DTs?

There are many tricks to detect that RVV is present but noncomformant at run-
time. If the kernel can at least temporarily enable V to make the check, the 
example below works. Converting the user-space console program to a kernel 
function is left as an exercise to the reader:

----8<--------8<--------8<--------8<--------8<--------8<--------8<----
#include <stdio.h>

int main(void)
{
        long vtype;

        __asm__ (
                "vsetvli zero, zero, e8, m1, ta, ma\n"
                "csrr   %1, vtype\n" : "=r" (vtype) : "r" (0xC0));

        if (vtype < 0)
                puts("RVV 0.7.1");
        else
                puts("RVV 1.0+");

        return 0;
}
----8<--------8<--------8<--------8<--------8<--------8<--------8<----

The trick here is that the vector configuration is valid for RVV 1.0, but not 
for RVV 0.7.1 so the sign/illegal bit gets.

Rémi Denis-Courmont July 13, 2023, 6:50 p.m. UTC | #11

Le torstaina 13. heinäkuuta 2023, 21.47.29 EEST Rémi Denis-Courmont a écrit :
>         __asm__ (
>                 "vsetvli zero, zero, e8, m1, ta, ma\n"
>                 "csrr   %1, vtype\n" : "=r" (vtype) : "r" (0xC0));

The input operand is obviously bogus left-over, sorry.

Guo Ren July 14, 2023, 6:07 a.m. UTC | #12

On Fri, Jul 14, 2023 at 1:43 AM Palmer Dabbelt <[e-mail]> wrote:
>
> On Thu, 13 Jul 2023 10:33:56 PDT (-0700), [e-mail] wrote:
> > On Thu, Jul 13, 2023 at 12:48 PM Jisheng Zhang <[e-mail]> wrote:
> >>
> >> On Wed, Jul 12, 2023 at 06:48:02PM +0100, Conor Dooley wrote:
> >> > From: Palmer Dabbelt <[e-mail]>
> >> >
> >> > The last merge window contained both V support and the deprecation of
> >> > the riscv,isa DT property, with the V implementation reading riscv,isa
> >> > to determine the presence of the V extension.  At the time that was the
> >> > only way to do it, but there's a lot of ambiguity around V in ISA
> >> > strings.  In particular, there is a lot of firmware in the wild that
> >> > uses "v" in the riscv,isa DT property to communicate support for the
> >> > 0.7.1 version of the Vector specification implemented by T-Head CPU
> >> > cores.
> >>
> >> Add Guo
> >>
> >> Hi Conor, Palmer,
> >>
> >> FWICT, new T-HEAD's riscv cores such as C908 support standard RVV-1.0,
> >> this patch looks like a big hammer for T-HEAD. I do understand why
> >> this patch is provided, but can we mitigate the situation by carefully
> >> review the DTs? Per my understanding, dts is also part of linux kernel.
> >>
> >> Thanks
> >>
> >> >
> >> > Rather than forcing use of the newly added interface that has strict
> >> > meanings for extensions to detect the presence of vector support, as
> >> > that would penalise those who have behaved, only ignore v in riscv,isa
> >> > on CPUs that report T-Head's vendor ID.
> >> >
> >> > 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]>
> >> > ---
> >> > Changes in v2:
> >> > - Use my version of the patch that touches hwcap and isainfo uniformly
> >> > - Don't penalise those who behaved
> >> > ---
> >> >  arch/riscv/kernel/cpufeature.c | 22 ++++++++++++++++++++++
> >> >  1 file changed, 22 insertions(+)
> >> >
> >> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> >> > index bdcf460ea53d..05362715e1b7 100644
> >> > --- a/arch/riscv/kernel/cpufeature.c
> >> > +++ b/arch/riscv/kernel/cpufeature.c
> >> > @@ -21,6 +21,7 @@
> >> >  #include <asm/hwcap.h>
> >> >  #include <asm/patch.h>
> >> >  #include <asm/processor.h>
> >> > +#include <asm/sbi.h>
> >> >  #include <asm/vector.h>
> >> >
> >> >  #define NUM_ALPHA_EXTS ('z' - 'a' + 1)
> >> > @@ -334,6 +335,27 @@ void __init riscv_fill_hwcap(void)
> >> >                       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
> >> > +              * and no T-Head CPU cores with the standard version of vector
> >> > +              * are in circulation yet.
> > T-HEAD's vector 1.0 SoCs is in circulation. Kendryte K230 is the
> > shipped SoC chip, which vendor id = THEAD_VENDOR_ID and with vector
> > 1.0.
>
> Do you have a pointer to where they're availiable?  Kendryte's website
> doesn't list a K230 yet: https://www.canaan.io/product/kendryteai## .
> They'd be the first V-1.0 implementation in the wild, I bet a lot of
> people would want one to play with...
Here is sales' link, the chips & EVB boards:
https://www.canaan-creative.com/product/k230

>
> >
> >> > +              * Platforms with T-Head CPU cores that support the standard
> >> > +              * version of vector must provide the explicit V property,
> >> > +              * which is well defined.
> >> > +              */
> >> > +             if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID) {
> > If you insist on doing this, please:
> >
> > if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID &&
> > riscv_cached_marchid(cpu) == 0 && riscv_cached_mimpid(cpu) == 0) {
>
> From above it sounds like that's OK for the C920.  Does the C908 set a
> non-zero m{arch,impl}id?  If not, then this won't help anything.
The c908 {arch,impl}id in k230 is non-zero, so it would help. Treat
all T-HEAD cores as 0.7.1 vector is not truth, only {arch,impl}id = 0
cores are vector 0.7.1.

And the all erratas of T-HEAD has the below code:
if (arch_id != 0 || impid != 0)
    return false;

So, we should do the same here.

>
> Also: does the K230 work with upstream kernels yet?  I don't see any DT
> stuff for it, so aside from the unlikely event it's quirk-free then it'd
> need DTs changed by upstream review and thus would be forced to use the
> new properties anyway.
>
> >> > +                     if (of_property_match_string(node, "riscv,isa-extensions", "v") >= 0) {
> >> > +                             this_hwcap |= isa2hwcap[RISCV_ISA_EXT_v];
> >> > +                             set_bit(RISCV_ISA_EXT_v, isainfo->isa);
> >> > +                     } else {
> >> > +                             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.39.2
> >> >
> >> >
> >> > _______________________________________________
> >> > linux-riscv mailing list
> >> > [e-mail]
> >> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >
> >
> >
> > --
> > Best Regards
> >  Guo Ren

Guo Ren July 14, 2023, 6:14 a.m. UTC | #13

On Fri, Jul 14, 2023 at 1:45 AM Conor Dooley <[e-mail]> wrote:
>
> On Thu, Jul 13, 2023 at 01:33:56PM -0400, Guo Ren wrote:
> > On Thu, Jul 13, 2023 at 12:48 PM Jisheng Zhang <[e-mail]> wrote:
> > > On Wed, Jul 12, 2023 at 06:48:02PM +0100, Conor Dooley wrote:
>
> > > > +             /*
> > > > +              * "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
> > > > +              * and no T-Head CPU cores with the standard version of vector
> > > > +              * are in circulation yet.
>
> > T-HEAD's vector 1.0 SoCs is in circulation. Kendryte K230 is the
> > shipped SoC chip, which vendor id = THEAD_VENDOR_ID and with vector
> > 1.0.
>
> Where can I buy one, if it is in circulation?
> Googling in English might not be the best thing to do, but doing so I
> could find basically no information on the k230 - I did know it existed
> and tried to find some info on it before sending the patch.
> If it is not in circulation, then the comment is not inaccurate & when
> they do arrive they can use the new dedicated property to convey support
> for vector.
>
> > > > +              * Platforms with T-Head CPU cores that support the standard
> > > > +              * version of vector must provide the explicit V property,
> > > > +              * which is well defined.
> > > > +              */
> > > > +             if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID) {
> > If you insist on doing this, please:
> >
> > if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID &&
> > riscv_cached_marchid(cpu) == 0 && riscv_cached_mimpid(cpu) == 0) {
>
> Why? Does the c908 report non-zero mimpid/marchid?
Yes

> If yes, does it need either the errata for CMOs or the page tables?
C908 is compatible to RVA22 Profile, here is the detail link:
https://xrvm.com/cpu-details?id=4107904466789928960

>
> Thanks,
> Conor.

Conor Dooley July 14, 2023, 11:10 a.m. UTC | #14

On Fri, Jul 14, 2023 at 02:14:45PM +0800, Guo Ren wrote:
> On Fri, Jul 14, 2023 at 1:45 AM Conor Dooley <[e-mail]> wrote:
> >
> > On Thu, Jul 13, 2023 at 01:33:56PM -0400, Guo Ren wrote:
> > > On Thu, Jul 13, 2023 at 12:48 PM Jisheng Zhang <[e-mail]> wrote:
> > > > On Wed, Jul 12, 2023 at 06:48:02PM +0100, Conor Dooley wrote:
> >
> > > > > +             /*
> > > > > +              * "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
> > > > > +              * and no T-Head CPU cores with the standard version of vector
> > > > > +              * are in circulation yet.
> >
> > > T-HEAD's vector 1.0 SoCs is in circulation. Kendryte K230 is the
> > > shipped SoC chip, which vendor id = THEAD_VENDOR_ID and with vector
> > > 1.0.
> >
> > Where can I buy one, if it is in circulation?
> > Googling in English might not be the best thing to do, but doing so I
> > could find basically no information on the k230 - I did know it existed
> > and tried to find some info on it before sending the patch.
> > If it is not in circulation, then the comment is not inaccurate & when
> > they do arrive they can use the new dedicated property to convey support
> > for vector.

I saw you sent to Palmer a link to the k230. When I clicked the link,
it gave (per google translate) purchase options for k210 and k510 only.
I figure that means there is nothing _publicly_ available?

> > > > > +              * Platforms with T-Head CPU cores that support the standard
> > > > > +              * version of vector must provide the explicit V property,
> > > > > +              * which is well defined.
> > > > > +              */
> > > > > +             if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID) {
> > > If you insist on doing this, please:
> > >
> > > if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID &&
> > > riscv_cached_marchid(cpu) == 0 && riscv_cached_mimpid(cpu) == 0) {
> >
> > Why? Does the c908 report non-zero mimpid/marchid?

> Yes

> > If yes, does it need either the errata for CMOs or the page tables?

> C908 is compatible to RVA22 Profile, here is the detail link:
> https://xrvm.com/cpu-details?id=4107904466789928960

That's fantastic news! I'll submit a v3 of this with the hammer reduced,
as you have suggested above.

Thanks,
Conor.

Conor Dooley July 14, 2023, 6:45 p.m. UTC | #15

On Fri, Jul 14, 2023 at 12:10:24PM +0100, Conor Dooley wrote:
> On Fri, Jul 14, 2023 at 02:14:45PM +0800, Guo Ren wrote:
> > On Fri, Jul 14, 2023 at 1:45 AM Conor Dooley <[e-mail]> wrote:
> > >
> > > On Thu, Jul 13, 2023 at 01:33:56PM -0400, Guo Ren wrote:
> > > > On Thu, Jul 13, 2023 at 12:48 PM Jisheng Zhang <[e-mail]> wrote:
> > > > > On Wed, Jul 12, 2023 at 06:48:02PM +0100, Conor Dooley wrote:
> > >
> > > > > > +             /*
> > > > > > +              * "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
> > > > > > +              * and no T-Head CPU cores with the standard version of vector
> > > > > > +              * are in circulation yet.
> > >
> > > > T-HEAD's vector 1.0 SoCs is in circulation. Kendryte K230 is the
> > > > shipped SoC chip, which vendor id = THEAD_VENDOR_ID and with vector
> > > > 1.0.
> > >
> > > Where can I buy one, if it is in circulation?
> > > Googling in English might not be the best thing to do, but doing so I
> > > could find basically no information on the k230 - I did know it existed
> > > and tried to find some info on it before sending the patch.
> > > If it is not in circulation, then the comment is not inaccurate & when
> > > they do arrive they can use the new dedicated property to convey support
> > > for vector.
> 
> I saw you sent to Palmer a link to the k230. When I clicked the link,
> it gave (per google translate) purchase options for k210 and k510 only.
> I figure that means there is nothing _publicly_ available?
> 
> > > > > > +              * Platforms with T-Head CPU cores that support the standard
> > > > > > +              * version of vector must provide the explicit V property,
> > > > > > +              * which is well defined.
> > > > > > +              */
> > > > > > +             if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID) {
> > > > If you insist on doing this, please:
> > > >
> > > > if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID &&
> > > > riscv_cached_marchid(cpu) == 0 && riscv_cached_mimpid(cpu) == 0) {
> > >
> > > Why? Does the c908 report non-zero mimpid/marchid?
> 
> > Yes
> 
> > > If yes, does it need either the errata for CMOs or the page tables?
> 
> > C908 is compatible to RVA22 Profile, here is the detail link:
> > https://xrvm.com/cpu-details?id=4107904466789928960
> 
> That's fantastic news! I'll submit a v3 of this with the hammer reduced,
> as you have suggested above.

From some chat on IRC, I realised that this xrvm.com link mentions that
the c908 supports both XMAE and Svpbmt.
If it does support both, could you explain about how that works?
Is there some CSR that allows to switch between them?
How do you intend communicating to s-mode etc which of them is in use?

Thanks,
Conor.

Conor Dooley July 14, 2023, 7:21 p.m. UTC | #16

On Wed, Jul 12, 2023 at 06:48:02PM +0100, Conor Dooley wrote:
> From: Palmer Dabbelt <[e-mail]>
> 
> The last merge window contained both V support and the deprecation of
> the riscv,isa DT property, with the V implementation reading riscv,isa
> to determine the presence of the V extension.  At the time that was the
> only way to do it, but there's a lot of ambiguity around V in ISA
> strings.  In particular, there is a lot of firmware in the wild that
> uses "v" in the riscv,isa DT property to communicate support for the
> 0.7.1 version of the Vector specification implemented by T-Head CPU
> cores.
> 
> Rather than forcing use of the newly added interface that has strict
> meanings for extensions to detect the presence of vector support, as
> that would penalise those who have behaved, only ignore v in riscv,isa
> on CPUs that report T-Head's vendor ID.
> 
> 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]>

From speaking to various people on IRC, and Guo Ren's information that
the c908, which supports the standard version of vector, has non-zero
marchid, we may not need this patch for now.
There's no real urgency to prevent a future regression in support since
marchid will differ between the c908 and the existing cores that only
support the v0.7.1 version of vector, so this could be applied at our
leisure IFF an issue does actually crop up. I've marked it as Changes
Requested on patchwork.

Guo Ren July 15, 2023, 6:07 a.m. UTC | #17

On Fri, Jul 14, 2023 at 7:10 PM Conor Dooley <[e-mail]> wrote:
>
> On Fri, Jul 14, 2023 at 02:14:45PM +0800, Guo Ren wrote:
> > On Fri, Jul 14, 2023 at 1:45 AM Conor Dooley <[e-mail]> wrote:
> > >
> > > On Thu, Jul 13, 2023 at 01:33:56PM -0400, Guo Ren wrote:
> > > > On Thu, Jul 13, 2023 at 12:48 PM Jisheng Zhang <[e-mail]> wrote:
> > > > > On Wed, Jul 12, 2023 at 06:48:02PM +0100, Conor Dooley wrote:
> > >
> > > > > > +             /*
> > > > > > +              * "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
> > > > > > +              * and no T-Head CPU cores with the standard version of vector
> > > > > > +              * are in circulation yet.
> > >
> > > > T-HEAD's vector 1.0 SoCs is in circulation. Kendryte K230 is the
> > > > shipped SoC chip, which vendor id = THEAD_VENDOR_ID and with vector
> > > > 1.0.
> > >
> > > Where can I buy one, if it is in circulation?
> > > Googling in English might not be the best thing to do, but doing so I
> > > could find basically no information on the k230 - I did know it existed
> > > and tried to find some info on it before sending the patch.
> > > If it is not in circulation, then the comment is not inaccurate & when
> > > they do arrive they can use the new dedicated property to convey support
> > > for vector.
>
> I saw you sent to Palmer a link to the k230. When I clicked the link,
> it gave (per google translate) purchase options for k210 and k510 only.
> I figure that means there is nothing _publicly_ available?
The current is in the early stage. If you have interesting to buy
chips / EVBs, don't hesitate to email them.
Chips are ready for sale, and you may buy the development boards from
Taobao/... in Q4 of this year.

>
> > > > > > +              * Platforms with T-Head CPU cores that support the standard
> > > > > > +              * version of vector must provide the explicit V property,
> > > > > > +              * which is well defined.
> > > > > > +              */
> > > > > > +             if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID) {
> > > > If you insist on doing this, please:
> > > >
> > > > if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID &&
> > > > riscv_cached_marchid(cpu) == 0 && riscv_cached_mimpid(cpu) == 0) {
> > >
> > > Why? Does the c908 report non-zero mimpid/marchid?
>
> > Yes
>
> > > If yes, does it need either the errata for CMOs or the page tables?
>
> > C908 is compatible to RVA22 Profile, here is the detail link:
> > https://xrvm.com/cpu-details?id=4107904466789928960
>
> That's fantastic news! I'll submit a v3 of this with the hammer reduced,
> as you have suggested above.
>
> Thanks,
> Conor.

Guo Ren July 15, 2023, 6:11 a.m. UTC | #18

On Sat, Jul 15, 2023 at 2:45 AM Conor Dooley <[e-mail]> wrote:
>
> On Fri, Jul 14, 2023 at 12:10:24PM +0100, Conor Dooley wrote:
> > On Fri, Jul 14, 2023 at 02:14:45PM +0800, Guo Ren wrote:
> > > On Fri, Jul 14, 2023 at 1:45 AM Conor Dooley <[e-mail]> wrote:
> > > >
> > > > On Thu, Jul 13, 2023 at 01:33:56PM -0400, Guo Ren wrote:
> > > > > On Thu, Jul 13, 2023 at 12:48 PM Jisheng Zhang <[e-mail]> wrote:
> > > > > > On Wed, Jul 12, 2023 at 06:48:02PM +0100, Conor Dooley wrote:
> > > >
> > > > > > > +             /*
> > > > > > > +              * "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
> > > > > > > +              * and no T-Head CPU cores with the standard version of vector
> > > > > > > +              * are in circulation yet.
> > > >
> > > > > T-HEAD's vector 1.0 SoCs is in circulation. Kendryte K230 is the
> > > > > shipped SoC chip, which vendor id = THEAD_VENDOR_ID and with vector
> > > > > 1.0.
> > > >
> > > > Where can I buy one, if it is in circulation?
> > > > Googling in English might not be the best thing to do, but doing so I
> > > > could find basically no information on the k230 - I did know it existed
> > > > and tried to find some info on it before sending the patch.
> > > > If it is not in circulation, then the comment is not inaccurate & when
> > > > they do arrive they can use the new dedicated property to convey support
> > > > for vector.
> >
> > I saw you sent to Palmer a link to the k230. When I clicked the link,
> > it gave (per google translate) purchase options for k210 and k510 only.
> > I figure that means there is nothing _publicly_ available?
> >
> > > > > > > +              * Platforms with T-Head CPU cores that support the standard
> > > > > > > +              * version of vector must provide the explicit V property,
> > > > > > > +              * which is well defined.
> > > > > > > +              */
> > > > > > > +             if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID) {
> > > > > If you insist on doing this, please:
> > > > >
> > > > > if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID &&
> > > > > riscv_cached_marchid(cpu) == 0 && riscv_cached_mimpid(cpu) == 0) {
> > > >
> > > > Why? Does the c908 report non-zero mimpid/marchid?
> >
> > > Yes
> >
> > > > If yes, does it need either the errata for CMOs or the page tables?
> >
> > > C908 is compatible to RVA22 Profile, here is the detail link:
> > > https://xrvm.com/cpu-details?id=4107904466789928960
> >
> > That's fantastic news! I'll submit a v3 of this with the hammer reduced,
> > as you have suggested above.
>
> From some chat on IRC, I realised that this xrvm.com link mentions that
> the c908 supports both XMAE and Svpbmt.
> If it does support both, could you explain about how that works?
> Is there some CSR that allows to switch between them?
> How do you intend communicating to s-mode etc which of them is in use?
We still keep XMAE as a backup solution, and our custom m-mode csr
register could switch it. So it wouldn't affect the current Linux
code. Our next generation of cores would abandon it gradually.

>
> Thanks,
> Conor.

Guo Ren July 15, 2023, 6:12 a.m. UTC | #19

On Sat, Jul 15, 2023 at 3:21 AM Conor Dooley <[e-mail]> wrote:
>
> On Wed, Jul 12, 2023 at 06:48:02PM +0100, Conor Dooley wrote:
> > From: Palmer Dabbelt <[e-mail]>
> >
> > The last merge window contained both V support and the deprecation of
> > the riscv,isa DT property, with the V implementation reading riscv,isa
> > to determine the presence of the V extension.  At the time that was the
> > only way to do it, but there's a lot of ambiguity around V in ISA
> > strings.  In particular, there is a lot of firmware in the wild that
> > uses "v" in the riscv,isa DT property to communicate support for the
> > 0.7.1 version of the Vector specification implemented by T-Head CPU
> > cores.
> >
> > Rather than forcing use of the newly added interface that has strict
> > meanings for extensions to detect the presence of vector support, as
> > that would penalise those who have behaved, only ignore v in riscv,isa
> > on CPUs that report T-Head's vendor ID.
> >
> > 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]>
>
> From speaking to various people on IRC, and Guo Ren's information that
> the c908, which supports the standard version of vector, has non-zero
> marchid, we may not need this patch for now.
> There's no real urgency to prevent a future regression in support since
> marchid will differ between the c908 and the existing cores that only
> support the v0.7.1 version of vector, so this could be applied at our
> leisure IFF an issue does actually crop up. I've marked it as Changes
> Requested on patchwork.
Thx! We agree that standard vector 1.0 is the correct direction for the future.

Patch

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index bdcf460ea53d..05362715e1b7 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -21,6 +21,7 @@ 
 #include <asm/hwcap.h>
 #include <asm/patch.h>
 #include <asm/processor.h>
+#include <asm/sbi.h>
 #include <asm/vector.h>
 
 #define NUM_ALPHA_EXTS ('z' - 'a' + 1)
@@ -334,6 +335,27 @@  void __init riscv_fill_hwcap(void)
 			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
+		 * and no T-Head CPU cores with the standard version of vector
+		 * are in circulation yet.
+		 * Platforms with T-Head CPU cores that support the standard
+		 * version of vector must provide the explicit V property,
+		 * which is well defined.
+		 */
+		if (acpi_disabled && riscv_cached_mvendorid(cpu) == THEAD_VENDOR_ID) {
+			if (of_property_match_string(node, "riscv,isa-extensions", "v") >= 0) {
+				this_hwcap |= isa2hwcap[RISCV_ISA_EXT_v];
+				set_bit(RISCV_ISA_EXT_v, isainfo->isa);
+			} else {
+				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