| Message ID | 20240503-dev-charlie-support_thead_vector_6_9-v6-3-cb7624e65d82@rivosinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | riscv: Support vendor extensions and xtheadvector | expand |
Hey Charlie, Just me being a pain again... On Fri, May 03, 2024 at 11:18:18AM -0700, Charlie Jenkins wrote: > @@ -671,6 +713,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; > + } > } After replying to Andy this morning about doing some dependency checking for vector-reliant extensions I realised that we're actually doing this check too late to be useful for that case. Say for Zvkb we want to check if vector has been enabled before marking the extension available, and without Clement's series that re-works the riscv_isa_extension_check(), we need to ensure that vector support isn't gonna get turned after we've already marked Zvkb as usable. If we could move the riscv_has_homogeneous_vlenb() call before probing extensions we could then examine the result in riscv_isa_extension_check() for V and make sure it doesn't get enabled in the first place, rather than clearing it after the fact. There's a whole load of moving pieces between different series here at the moment though, I wish some of it would land so that I could send some cleanup patches for what I think is inconsistency on top :) I wouldn't mind if this landed as-is, it's still an improvement and the user I mention above doesn't actually exist yet. Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Cheers, Conor.
On Sat, May 4, 2024 at 2:21 AM Charlie Jenkins <charlie@rivosinc.com> wrote: > > If vlenb is provided in the device tree, prefer that over reading the > vlenb csr. > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> I agree with Conor that we need a mechanism to turn off v and all depending extensions with has_riscv_homogeneous_vlenb(). And that can come after this. Thanks for adding the homogeneous vlen checking! Reviewed-by: Andy Chiu <andy.chiu@sifive.com> > --- > arch/riscv/include/asm/cpufeature.h | 2 ++ > arch/riscv/kernel/cpufeature.c | 47 +++++++++++++++++++++++++++++++++++++ > arch/riscv/kernel/vector.c | 12 +++++++++- > 3 files changed, 60 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..6c143ea9592b 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,46 @@ 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; > + > + /* Ignore vlenb if vector is not enabled in the kernel */ > + if (!IS_ENABLED(CONFIG_RISCV_ISA_V)) > + return 0; > + > + 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 +713,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(); > > -- > 2.44.0 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
Sorry Charlie, I forgot to include the mailing list. Here is the same as what I sent in the private message. On Sat, May 4, 2024 at 2:21 AM Charlie Jenkins <charlie@rivosinc.com> 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 | 47 +++++++++++++++++++++++++++++++++++++ > arch/riscv/kernel/vector.c | 12 +++++++++- > 3 files changed, 60 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..6c143ea9592b 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,46 @@ 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; > + > + /* Ignore vlenb if vector is not enabled in the kernel */ > + if (!IS_ENABLED(CONFIG_RISCV_ISA_V)) > + return 0; > + > + 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 +713,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.\> + elf_hwcap &= ~COMPAT_HWCAP_ISA_V; > + } We only touch COMPAT_HWCAP_ISA_V and the failed case only turns off the rectified V. So here we have nothing to do with the Xtheadvector. However, I am still confused because I think Xtheadvector would also need to call into this check, so as to setup vlenb. Apart from that, it seems like some vendor stating Xtheadvector is actually vector-0.7. Please correct me if I speak anything wrong. One thing I noticed is that Xtheadvector wouldn't trap on reading th.vlenb but vector-0.7 would. If that is the case, should we require Xtheadvector to specify `riscv,vlenb` on the device tree? > } > > /* > 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 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv Thanks, Andy
On Thu, May 16, 2024 at 10:00:12PM +0800, Andy Chiu wrote: > On Sat, May 4, 2024 at 2:21 AM Charlie Jenkins <charlie@rivosinc.com> wrote: > > + if (elf_hwcap & COMPAT_HWCAP_ISA_V && has_riscv_homogeneous_vlenb() < 0) { > > + pr_warn("Unsupported heterogeneous vlen detected, vector extension disabled.\ > > + elf_hwcap &= ~COMPAT_HWCAP_ISA_V; > > + } > > We only touch COMPAT_HWCAP_ISA_V and the failed case only turns off the > rectified V. So here we have nothing to do with the Xtheadvector. There's nothing t-head related in the tree at this point, so doing anything with it would cause build issues. > However, I am still confused because I think Xtheadvector would also > need to call into this check, so as to setup vlenb. > Apart from that, it seems like some vendor stating Xtheadvector is > actually vector-0.7. The T-Head implementation is 0.7.x, but I am not really sure what you mean by this comment. > Please correct me if I speak anything wrong. One > thing I noticed is that Xtheadvector wouldn't trap on reading > th.vlenb but vector-0.7 would. If that is the case, should we require > Xtheadvector to specify `riscv,vlenb` on the device tree? In the world of Linux, "vector-0.7" isn't a thing. There's only 1.0, and after this patchset, "xtheadvector". My understanding, from discussion on earlier versions of this series the trap is actually accessing th.vlenb register, despite the documentation stating that it is unprivileged: https://github.com/T-head-Semi/thead-extension-spec/blob/master/xtheadvector.adoc I assume Charlie tried it but was trapping, as v1 had a comment: + * Although xtheadvector states that th.vlenb exists and + * overlaps with the vector 1.0 extension overlaps, an illegal + * instruction is raised if read. These systems all currently + * have a fixed vector length of 128, so hardcode that value. Cheers, Conor.
On Thu, May 16, 2024 at 05:24:25PM +0100, Conor Dooley wrote: > On Thu, May 16, 2024 at 10:00:12PM +0800, Andy Chiu wrote: > > On Sat, May 4, 2024 at 2:21 AM Charlie Jenkins <charlie@rivosinc.com> wrote: > > > > + if (elf_hwcap & COMPAT_HWCAP_ISA_V && has_riscv_homogeneous_vlenb() < 0) { > > > + pr_warn("Unsupported heterogeneous vlen detected, vector extension disabled.\ > > > + elf_hwcap &= ~COMPAT_HWCAP_ISA_V; > > > + } > > > > We only touch COMPAT_HWCAP_ISA_V and the failed case only turns off the > > rectified V. So here we have nothing to do with the Xtheadvector. > > There's nothing t-head related in the tree at this point, so doing > anything with it would cause build issues. > > > However, I am still confused because I think Xtheadvector would also > > need to call into this check, so as to setup vlenb. > > > > Apart from that, it seems like some vendor stating Xtheadvector is > > actually vector-0.7. > > The T-Head implementation is 0.7.x, but I am not really sure what you > mean by this comment. Andy, the idea of this patch was to be able to support this binding on more than just xtheadvector. You are correct though Andy, this is a problem that a later patch in this series doesn't disable xtheadvector when vlenb is not homogeneous. I am going to wait to send out any more versions until after this merge window but I will fix this in the next version. Thank you! > > > Please correct me if I speak anything wrong. One > > thing I noticed is that Xtheadvector wouldn't trap on reading > > th.vlenb but vector-0.7 would. If that is the case, should we require > > Xtheadvector to specify `riscv,vlenb` on the device tree? > > In the world of Linux, "vector-0.7" isn't a thing. There's only 1.0, and > after this patchset, "xtheadvector". My understanding, from discussion > on earlier versions of this series the trap is actually accessing > th.vlenb register, despite the documentation stating that it is > unprivileged: > https://github.com/T-head-Semi/thead-extension-spec/blob/master/xtheadvector.adoc > I assume Charlie tried it but was trapping, as v1 had a comment: > + * Although xtheadvector states that th.vlenb exists and > + * overlaps with the vector 1.0 extension overlaps, an illegal > + * instruction is raised if read. These systems all currently > + * have a fixed vector length of 128, so hardcode that value. On my board with a c906 attempting to read th.vlenb (which is supposed to have the same encoding as vlenb) raises an illegal instruction exception from S-mode even though the documentation states that it shouldn't. Because the documentation states that vlenb is available, I haven't made it required for xtheadvector, I am not sure the proper solution for that. - Charlie > > Cheers, > Conor.
On Thu, May 16, 2024 at 01:28:45PM -0700, Charlie Jenkins wrote: > On Thu, May 16, 2024 at 05:24:25PM +0100, Conor Dooley wrote: > > On Thu, May 16, 2024 at 10:00:12PM +0800, Andy Chiu wrote: > > > On Sat, May 4, 2024 at 2:21 AM Charlie Jenkins <charlie@rivosinc.com> wrote: > > > > > > + if (elf_hwcap & COMPAT_HWCAP_ISA_V && has_riscv_homogeneous_vlenb() < 0) { > > > > + pr_warn("Unsupported heterogeneous vlen detected, vector extension disabled.\ > > > > + elf_hwcap &= ~COMPAT_HWCAP_ISA_V; > > > > + } > > > > > > We only touch COMPAT_HWCAP_ISA_V and the failed case only turns off the > > > rectified V. So here we have nothing to do with the Xtheadvector. > > > > There's nothing t-head related in the tree at this point, so doing > > anything with it would cause build issues. > > > > > However, I am still confused because I think Xtheadvector would also > > > need to call into this check, so as to setup vlenb. > > > > > > > Apart from that, it seems like some vendor stating Xtheadvector is > > > actually vector-0.7. > > > > The T-Head implementation is 0.7.x, but I am not really sure what you > > mean by this comment. > > Andy, the idea of this patch was to be able to support this binding on > more than just xtheadvector. > > You are correct though Andy, this is a problem that a later patch in > this series doesn't disable xtheadvector when vlenb is not homogeneous. > I am going to wait to send out any more versions until after this merge > window but I will fix this in the next version. Thank you! Agreed on all counts :) > > > Please correct me if I speak anything wrong. One > > > thing I noticed is that Xtheadvector wouldn't trap on reading > > > th.vlenb but vector-0.7 would. If that is the case, should we require > > > Xtheadvector to specify `riscv,vlenb` on the device tree? > > > > In the world of Linux, "vector-0.7" isn't a thing. There's only 1.0, and > > after this patchset, "xtheadvector". My understanding, from discussion > > on earlier versions of this series the trap is actually accessing > > th.vlenb register, despite the documentation stating that it is > > unprivileged: > > https://github.com/T-head-Semi/thead-extension-spec/blob/master/xtheadvector.adoc > > I assume Charlie tried it but was trapping, as v1 had a comment: > > + * Although xtheadvector states that th.vlenb exists and > > + * overlaps with the vector 1.0 extension overlaps, an illegal > > + * instruction is raised if read. These systems all currently > > + * have a fixed vector length of 128, so hardcode that value. > > On my board with a c906 attempting to read th.vlenb (which is supposed > to have the same encoding as vlenb) raises an illegal instruction > exception from S-mode even though the documentation states that it > shouldn't. Because the documentation states that vlenb is available, I > haven't made it required for xtheadvector, I am not sure the proper > solution for that. Would you mind raising an issue on the T-Head extension spec repo about this? Thanks, Conor.
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..6c143ea9592b 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,46 @@ 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; + + /* Ignore vlenb if vector is not enabled in the kernel */ + if (!IS_ENABLED(CONFIG_RISCV_ISA_V)) + return 0; + + 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 +713,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 | 47 +++++++++++++++++++++++++++++++++++++ arch/riscv/kernel/vector.c | 12 +++++++++- 3 files changed, 60 insertions(+), 1 deletion(-)