| Message ID | 20240502-dev-charlie-support_thead_vector_6_9-v5-3-d1b5c013a966@rivosinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: Support vendor extensions and xtheadvector | expand |
On Thu, May 02, 2024 at 09:46:38PM -0700, Charlie Jenkins wrote: > If vlenb is provided in the device tree, prefer that over reading the > vlenb csr. > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > --- > arch/riscv/include/asm/cpufeature.h | 2 ++ > arch/riscv/kernel/cpufeature.c | 43 +++++++++++++++++++++++++++++++++++++ > arch/riscv/kernel/vector.c | 12 ++++++++++- > 3 files changed, 56 insertions(+), 1 deletion(-) > > diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h > index 347805446151..0c4f08577015 100644 > --- a/arch/riscv/include/asm/cpufeature.h > +++ b/arch/riscv/include/asm/cpufeature.h > @@ -31,6 +31,8 @@ DECLARE_PER_CPU(struct riscv_cpuinfo, riscv_cpuinfo); > /* Per-cpu ISA extensions. */ > extern struct riscv_isainfo hart_isa[NR_CPUS]; > > +extern u32 riscv_vlenb_of; > + > void riscv_user_isa_enable(void); > > #if defined(CONFIG_RISCV_MISALIGNED) > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index 3ed2359eae35..12c79db0b0bb 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -35,6 +35,8 @@ static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly; > /* Per-cpu ISA extensions. */ > struct riscv_isainfo hart_isa[NR_CPUS]; > > +u32 riscv_vlenb_of; > + > /** > * riscv_isa_extension_base() - Get base extension word > * > @@ -648,6 +650,42 @@ static int __init riscv_isa_fallback_setup(char *__unused) > early_param("riscv_isa_fallback", riscv_isa_fallback_setup); > #endif > > +static int has_riscv_homogeneous_vlenb(void) > +{ > + int cpu; > + u32 prev_vlenb = 0; > + u32 vlenb; > + > + for_each_possible_cpu(cpu) { > + struct device_node *cpu_node; > + > + cpu_node = of_cpu_device_node_get(cpu); > + if (!cpu_node) { > + pr_warn("Unable to find cpu node\n"); > + return -ENOENT; > + } > + > + if (of_property_read_u32(cpu_node, "riscv,vlenb", &vlenb)) { > + of_node_put(cpu_node); > + > + if (prev_vlenb) > + return -ENOENT; > + continue; > + } > + > + if (prev_vlenb && vlenb != prev_vlenb) { > + of_node_put(cpu_node); > + return -ENOENT; > + } > + > + prev_vlenb = vlenb; > + of_node_put(cpu_node); > + } > + > + riscv_vlenb_of = vlenb; > + return 0; > +} > + > void __init riscv_fill_hwcap(void) > { > char print_str[NUM_ALPHA_EXTS + 1]; > @@ -671,6 +709,11 @@ void __init riscv_fill_hwcap(void) > pr_info("Falling back to deprecated \"riscv,isa\"\n"); > riscv_fill_hwcap_from_isa_string(isa2hwcap); > } > + > + if (elf_hwcap & COMPAT_HWCAP_ISA_V && has_riscv_homogeneous_vlenb() < 0) { I still think this isn't quite right, as it will emit a warning when RISCV_ISA_V is disabled. The simplest thing to do probably is just add an `if (IS_ENABLED(CONFIG_RISCV_ISA_V) return 0` shortcut the to function? It'll get disabled a few lines later so I think a zero is safe. > + pr_warn("Unsupported heterogeneous vlen detected, vector extension disabled.\n"); > + elf_hwcap &= ~COMPAT_HWCAP_ISA_V; > + } > } > > /* > diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c > index 6727d1d3b8f2..e04586cdb7f0 100644 > --- a/arch/riscv/kernel/vector.c > +++ b/arch/riscv/kernel/vector.c > @@ -33,7 +33,17 @@ int riscv_v_setup_vsize(void) > { > unsigned long this_vsize; > > - /* There are 32 vector registers with vlenb length. */ > + /* > + * There are 32 vector registers with vlenb length. > + * > + * If the riscv,vlenb property was provided by the firmware, use that > + * instead of probing the CSRs. > + */ > + if (riscv_vlenb_of) { > + this_vsize = riscv_vlenb_of * 32; > + return 0; > + } > + > riscv_v_enable(); > this_vsize = csr_read(CSR_VLENB) * 32; > riscv_v_disable(); > > -- > 2.44.0 >
On Fri, May 03, 2024 at 05:59:33PM +0100, Conor Dooley wrote: > On Thu, May 02, 2024 at 09:46:38PM -0700, Charlie Jenkins wrote: > > If vlenb is provided in the device tree, prefer that over reading the > > vlenb csr. > > > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > > --- > > arch/riscv/include/asm/cpufeature.h | 2 ++ > > arch/riscv/kernel/cpufeature.c | 43 +++++++++++++++++++++++++++++++++++++ > > arch/riscv/kernel/vector.c | 12 ++++++++++- > > 3 files changed, 56 insertions(+), 1 deletion(-) > > > > diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h > > index 347805446151..0c4f08577015 100644 > > --- a/arch/riscv/include/asm/cpufeature.h > > +++ b/arch/riscv/include/asm/cpufeature.h > > @@ -31,6 +31,8 @@ DECLARE_PER_CPU(struct riscv_cpuinfo, riscv_cpuinfo); > > /* Per-cpu ISA extensions. */ > > extern struct riscv_isainfo hart_isa[NR_CPUS]; > > > > +extern u32 riscv_vlenb_of; > > + > > void riscv_user_isa_enable(void); > > > > #if defined(CONFIG_RISCV_MISALIGNED) > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > index 3ed2359eae35..12c79db0b0bb 100644 > > --- a/arch/riscv/kernel/cpufeature.c > > +++ b/arch/riscv/kernel/cpufeature.c > > @@ -35,6 +35,8 @@ static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly; > > /* Per-cpu ISA extensions. */ > > struct riscv_isainfo hart_isa[NR_CPUS]; > > > > +u32 riscv_vlenb_of; > > + > > /** > > * riscv_isa_extension_base() - Get base extension word > > * > > @@ -648,6 +650,42 @@ static int __init riscv_isa_fallback_setup(char *__unused) > > early_param("riscv_isa_fallback", riscv_isa_fallback_setup); > > #endif > > > > +static int has_riscv_homogeneous_vlenb(void) > > +{ > > + int cpu; > > + u32 prev_vlenb = 0; > > + u32 vlenb; > > + > > + for_each_possible_cpu(cpu) { > > + struct device_node *cpu_node; > > + > > + cpu_node = of_cpu_device_node_get(cpu); > > + if (!cpu_node) { > > + pr_warn("Unable to find cpu node\n"); > > + return -ENOENT; > > + } > > + > > + if (of_property_read_u32(cpu_node, "riscv,vlenb", &vlenb)) { > > + of_node_put(cpu_node); > > + > > + if (prev_vlenb) > > + return -ENOENT; > > + continue; > > + } > > + > > + if (prev_vlenb && vlenb != prev_vlenb) { > > + of_node_put(cpu_node); > > + return -ENOENT; > > + } > > + > > + prev_vlenb = vlenb; > > + of_node_put(cpu_node); > > + } > > + > > + riscv_vlenb_of = vlenb; > > + return 0; > > +} > > + > > void __init riscv_fill_hwcap(void) > > { > > char print_str[NUM_ALPHA_EXTS + 1]; > > @@ -671,6 +709,11 @@ void __init riscv_fill_hwcap(void) > > pr_info("Falling back to deprecated \"riscv,isa\"\n"); > > riscv_fill_hwcap_from_isa_string(isa2hwcap); > > } > > + > > + if (elf_hwcap & COMPAT_HWCAP_ISA_V && has_riscv_homogeneous_vlenb() < 0) { > > I still think this isn't quite right, as it will emit a warning when > RISCV_ISA_V is disabled. The simplest thing to do probably is just > add an `if (IS_ENABLED(CONFIG_RISCV_ISA_V) return 0` shortcut the to > function? It'll get disabled a few lines later so I think a zero is > safe. That seems like a good idea. It is weird to throw a warning about this even when they have V disabled in the kernel. The DT is improperly formatted since it has heterogeneous vlenb entries and has V enabled, but since the user disabled V in the kernel skipping the warning is reasonable. - Charlie > > > + pr_warn("Unsupported heterogeneous vlen detected, vector extension disabled.\n"); > > + elf_hwcap &= ~COMPAT_HWCAP_ISA_V; > > + } > > } > > > > /* > > diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c > > index 6727d1d3b8f2..e04586cdb7f0 100644 > > --- a/arch/riscv/kernel/vector.c > > +++ b/arch/riscv/kernel/vector.c > > @@ -33,7 +33,17 @@ int riscv_v_setup_vsize(void) > > { > > unsigned long this_vsize; > > > > - /* There are 32 vector registers with vlenb length. */ > > + /* > > + * There are 32 vector registers with vlenb length. > > + * > > + * If the riscv,vlenb property was provided by the firmware, use that > > + * instead of probing the CSRs. > > + */ > > + if (riscv_vlenb_of) { > > + this_vsize = riscv_vlenb_of * 32; > > + return 0; > > + } > > + > > riscv_v_enable(); > > this_vsize = csr_read(CSR_VLENB) * 32; > > riscv_v_disable(); > > > > -- > > 2.44.0 > >
On Fri, May 03, 2024 at 10:15:16AM -0700, Charlie Jenkins wrote: > The DT is improperly > formatted since it has heterogeneous vlenb entries and has V enabled, > but since the user disabled V in the kernel skipping the warning is > reasonable. I wouldn't go as far as "improperly formatted", as if the harts really do have differing vector lengths, it's correctly formatted. It's just not something we support in Linux.
On Fri, May 03, 2024 at 06:26:58PM +0100, Conor Dooley wrote: > On Fri, May 03, 2024 at 10:15:16AM -0700, Charlie Jenkins wrote: > > The DT is improperly > > formatted since it has heterogeneous vlenb entries and has V enabled, > > but since the user disabled V in the kernel skipping the warning is > > reasonable. > > I wouldn't go as far as "improperly formatted", as if the harts really > do have differing vector lengths, it's correctly formatted. It's just > not something we support in Linux. Fair enough, not supported is a better term here. - Charlie
diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h index 347805446151..0c4f08577015 100644 --- a/arch/riscv/include/asm/cpufeature.h +++ b/arch/riscv/include/asm/cpufeature.h @@ -31,6 +31,8 @@ DECLARE_PER_CPU(struct riscv_cpuinfo, riscv_cpuinfo); /* Per-cpu ISA extensions. */ extern struct riscv_isainfo hart_isa[NR_CPUS]; +extern u32 riscv_vlenb_of; + void riscv_user_isa_enable(void); #if defined(CONFIG_RISCV_MISALIGNED) diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index 3ed2359eae35..12c79db0b0bb 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -35,6 +35,8 @@ static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly; /* Per-cpu ISA extensions. */ struct riscv_isainfo hart_isa[NR_CPUS]; +u32 riscv_vlenb_of; + /** * riscv_isa_extension_base() - Get base extension word * @@ -648,6 +650,42 @@ static int __init riscv_isa_fallback_setup(char *__unused) early_param("riscv_isa_fallback", riscv_isa_fallback_setup); #endif +static int has_riscv_homogeneous_vlenb(void) +{ + int cpu; + u32 prev_vlenb = 0; + u32 vlenb; + + for_each_possible_cpu(cpu) { + struct device_node *cpu_node; + + cpu_node = of_cpu_device_node_get(cpu); + if (!cpu_node) { + pr_warn("Unable to find cpu node\n"); + return -ENOENT; + } + + if (of_property_read_u32(cpu_node, "riscv,vlenb", &vlenb)) { + of_node_put(cpu_node); + + if (prev_vlenb) + return -ENOENT; + continue; + } + + if (prev_vlenb && vlenb != prev_vlenb) { + of_node_put(cpu_node); + return -ENOENT; + } + + prev_vlenb = vlenb; + of_node_put(cpu_node); + } + + riscv_vlenb_of = vlenb; + return 0; +} + void __init riscv_fill_hwcap(void) { char print_str[NUM_ALPHA_EXTS + 1]; @@ -671,6 +709,11 @@ void __init riscv_fill_hwcap(void) pr_info("Falling back to deprecated \"riscv,isa\"\n"); riscv_fill_hwcap_from_isa_string(isa2hwcap); } + + if (elf_hwcap & COMPAT_HWCAP_ISA_V && has_riscv_homogeneous_vlenb() < 0) { + pr_warn("Unsupported heterogeneous vlen detected, vector extension disabled.\n"); + elf_hwcap &= ~COMPAT_HWCAP_ISA_V; + } } /* diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c index 6727d1d3b8f2..e04586cdb7f0 100644 --- a/arch/riscv/kernel/vector.c +++ b/arch/riscv/kernel/vector.c @@ -33,7 +33,17 @@ int riscv_v_setup_vsize(void) { unsigned long this_vsize; - /* There are 32 vector registers with vlenb length. */ + /* + * There are 32 vector registers with vlenb length. + * + * If the riscv,vlenb property was provided by the firmware, use that + * instead of probing the CSRs. + */ + if (riscv_vlenb_of) { + this_vsize = riscv_vlenb_of * 32; + return 0; + } + riscv_v_enable(); this_vsize = csr_read(CSR_VLENB) * 32; riscv_v_disable();
If vlenb is provided in the device tree, prefer that over reading the vlenb csr. Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> --- arch/riscv/include/asm/cpufeature.h | 2 ++ arch/riscv/kernel/cpufeature.c | 43 +++++++++++++++++++++++++++++++++++++ arch/riscv/kernel/vector.c | 12 ++++++++++- 3 files changed, 56 insertions(+), 1 deletion(-)