Message ID | 20240412-zve-detection-v4-0-e0c45bb6b253@sifive.com |
---|---|
Headers | show |
Series | Support Zve32[xf] and Zve64[xfd] Vector subextensions | expand |
On Fri, Apr 12, 2024 at 02:48:57PM +0800, Andy Chiu wrote: > The function would fail when it detects the calling hart's vlen doesn't > match the first one's. The boot hart is the first hart calling this > function during riscv_fill_hwcap, so it is impossible to fail here. Add > a comment about this behavior. > > Signed-off-by: Andy Chiu <andy.chiu@sifive.com> > --- > Changelog v2: > - update the comment (Conor) > --- > arch/riscv/kernel/cpufeature.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index 3ed2359eae35..d22b12072579 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -683,6 +683,10 @@ void __init riscv_fill_hwcap(void) > } > > if (elf_hwcap & COMPAT_HWCAP_ISA_V) { > + /* > + * This callsite can't fail here. It cannot fail when called on > + * the boot hart. I am loathe to comment on this again, so Reviewed-by: Conor Dooley <conor.dooley@microchip.com> but you could just write this as "This cannot fail when called on the boot hart." Cheers, Conor. > + */ > riscv_v_setup_vsize(); > /* > * ISA string in device tree might have 'v' flag, but > > -- > 2.44.0.rc2 >
On Fri, Apr 12, 2024 at 02:49:01PM +0800, Andy Chiu wrote: > Add description for Zve32x Zve32f Zve64x Zve64f Zve64d ISA extensions. > > Signed-off-by: Andy Chiu <andy.chiu@sifive.com> Technically this should be before the patch using them in the kernel, but Acked-by: Conor Dooley <conor.dooley@microchip.com> Cheers, Conor.
On Fri, Apr 12, 2024 at 02:48:59PM +0800, Andy Chiu wrote: > Single-letter extensions may also imply multiple subextensions. For > example, Vector extension implies zve64d, and zve64d implies zve64f. > > Extension parsing for "riscv,isa-extensions" has the ability to resolve > the dependency by calling match_isa_ext(). This patch makes deprecated > parser call the same function for single letter extensions. Sure, why not.. Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Thanks, Conor.
+CC Eric, Jerry On Fri, Apr 12, 2024 at 02:49:03PM +0800, Andy Chiu wrote: > Make has_vector take one argument. This argument represents the minimum > Vector subextension that the following Vector actions assume. > > Also, change riscv_v_first_use_handler(), and boot code that calls > riscv_v_setup_vsize() to accept the minimum Vector sub-extension, > ZVE32X. > > Most kernel/user interfaces requires minimum of ZVE32X. Thus, programs > compiled and run with ZVE32X should be supported by the kernel on most > aspects. This includes context-switch, signal, ptrace, prctl, and > hwprobe. > > One exception is that ELF_HWCAP returns 'V' only if full V is supported > on the platform. This means that the system without a full V must not > rely on ELF_HWCAP to tell whether it is allowable to execute Vector > without first invoking a prctl() check. > > Signed-off-by: Andy Chiu <andy.chiu@sifive.com> > Acked-by: Joel Granados <j.granados@samsung.com> I'm not sure that I like this patch to be honest. As far as I can tell, every user here of has_vector(ext) is ZVE32X, so why bother actually having an argument? Could we just document that has_vector() is just a tyre kick of "is there a vector unit and are we allowed to use it", and anything requiring more than the bare-minimum (so zve32x?)must explicitly check for that form of vector using riscv_has_extension_[un]likely()? Finally, the in-kernel crypto stuff or other things that use can_use_simd() to check for vector support - do they all function correctly with all of the vector flavours? I don't understand the vector extensions well enough to evaluate that - I know that they do check for the individual extensions like Zvkb during probe but don't have anything for the vector version (at least in the chacha20 and sha256 glue code). If they don't, then we need to make sure those drivers do not probe with the cut-down variants. Eric/Jerry (although read the previous paragraph too): I noticed that the sha256 glue code calls crypto_simd_usable(), and in turn may_use_simd() before kernel_vector_begin(). The chacha20 glue code does not call either, which seems to violate the edict in kernel_vector_begin()'s kerneldoc: "Must not be called unless may_use_simd() returns true." What am I missing there? Cheers, Conor. > diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c > index c8219b82fbfc..e7c3fcac62a1 100644 > --- a/arch/riscv/kernel/sys_hwprobe.c > +++ b/arch/riscv/kernel/sys_hwprobe.c > @@ -69,7 +69,7 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, > if (riscv_isa_extension_available(NULL, c)) > pair->value |= RISCV_HWPROBE_IMA_C; > > - if (has_vector()) > + if (has_vector(v)) > pair->value |= RISCV_HWPROBE_IMA_V; > > /* > @@ -112,7 +112,11 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair, > EXT_KEY(ZACAS); > EXT_KEY(ZICOND); > > - if (has_vector()) { > + /* > + * Vector crypto and ZVE* extensions are supported only if > + * kernel has minimum V support of ZVE32X. > + */ > + if (has_vector(ZVE32X)) { > EXT_KEY(ZVE32X); > EXT_KEY(ZVE32F); > EXT_KEY(ZVE64X); I find this to be an indicate of the new has_vector() being a poor API, as it is confusing that a check > diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c > index 6727d1d3b8f2..e8a47fa72351 100644 > --- a/arch/riscv/kernel/vector.c > +++ b/arch/riscv/kernel/vector.c > @@ -53,7 +53,7 @@ int riscv_v_setup_vsize(void) > > void __init riscv_v_setup_ctx_cache(void) > { > - if (!has_vector()) > + if (!has_vector(ZVE32X)) > return; > > riscv_v_user_cachep = kmem_cache_create_usercopy("riscv_vector_ctx", > @@ -173,8 +173,11 @@ bool riscv_v_first_use_handler(struct pt_regs *regs) > u32 __user *epc = (u32 __user *)regs->epc; > u32 insn = (u32)regs->badaddr; > > + if (!has_vector(ZVE32X)) > + return false; > + > /* Do not handle if V is not supported, or disabled */ > - if (!(ELF_HWCAP & COMPAT_HWCAP_ISA_V)) > + if (!riscv_v_vstate_ctrl_user_allowed()) > return false; > > /* If V has been enabled then it is not the first-use trap */ > @@ -213,7 +216,7 @@ void riscv_v_vstate_ctrl_init(struct task_struct *tsk) > bool inherit; > int cur, next; > > - if (!has_vector()) > + if (!has_vector(ZVE32X)) > return; > > next = riscv_v_ctrl_get_next(tsk); > @@ -235,7 +238,7 @@ void riscv_v_vstate_ctrl_init(struct task_struct *tsk) > > long riscv_v_vstate_ctrl_get_current(void) > { > - if (!has_vector()) > + if (!has_vector(ZVE32X)) > return -EINVAL; > > return current->thread.vstate_ctrl & PR_RISCV_V_VSTATE_CTRL_MASK; > @@ -246,7 +249,7 @@ long riscv_v_vstate_ctrl_set_current(unsigned long arg) > bool inherit; > int cur, next; > > - if (!has_vector()) > + if (!has_vector(ZVE32X)) > return -EINVAL; > > if (arg & ~PR_RISCV_V_VSTATE_CTRL_MASK) > @@ -296,7 +299,7 @@ static struct ctl_table riscv_v_default_vstate_table[] = { > > static int __init riscv_v_sysctl_init(void) > { > - if (has_vector()) > + if (has_vector(ZVE32X)) > if (!register_sysctl("abi", riscv_v_default_vstate_table)) > return -EINVAL; > return 0; > diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S > index bc22c078aba8..bbe143bb32a0 100644 > --- a/arch/riscv/lib/uaccess.S > +++ b/arch/riscv/lib/uaccess.S > @@ -14,7 +14,7 @@ > > SYM_FUNC_START(__asm_copy_to_user) > #ifdef CONFIG_RISCV_ISA_V > - ALTERNATIVE("j fallback_scalar_usercopy", "nop", 0, RISCV_ISA_EXT_v, CONFIG_RISCV_ISA_V) > + ALTERNATIVE("j fallback_scalar_usercopy", "nop", 0, RISCV_ISA_EXT_ZVE32X, CONFIG_RISCV_ISA_V) > REG_L t0, riscv_v_usercopy_threshold > bltu a2, t0, fallback_scalar_usercopy > tail enter_vector_usercopy > > -- > 2.44.0.rc2 >
Hi Conor, On Thu, Apr 18, 2024 at 12:02:10PM +0100, Conor Dooley wrote: > +CC Eric, Jerry > > On Fri, Apr 12, 2024 at 02:49:03PM +0800, Andy Chiu wrote: > > Make has_vector take one argument. This argument represents the minimum > > Vector subextension that the following Vector actions assume. > > > > Also, change riscv_v_first_use_handler(), and boot code that calls > > riscv_v_setup_vsize() to accept the minimum Vector sub-extension, > > ZVE32X. > > > > Most kernel/user interfaces requires minimum of ZVE32X. Thus, programs > > compiled and run with ZVE32X should be supported by the kernel on most > > aspects. This includes context-switch, signal, ptrace, prctl, and > > hwprobe. > > > > One exception is that ELF_HWCAP returns 'V' only if full V is supported > > on the platform. This means that the system without a full V must not > > rely on ELF_HWCAP to tell whether it is allowable to execute Vector > > without first invoking a prctl() check. > > > > Signed-off-by: Andy Chiu <andy.chiu@sifive.com> > > Acked-by: Joel Granados <j.granados@samsung.com> > > I'm not sure that I like this patch to be honest. As far as I can tell, > every user here of has_vector(ext) is ZVE32X, so why bother actually > having an argument? > > Could we just document that has_vector() is just a tyre kick of "is > there a vector unit and are we allowed to use it", and anything > requiring more than the bare-minimum (so zve32x?)must explicitly check > for that form of vector using riscv_has_extension_[un]likely()? > > Finally, the in-kernel crypto stuff or other things that use > can_use_simd() to check for vector support - do they all function correctly > with all of the vector flavours? I don't understand the vector > extensions well enough to evaluate that - I know that they do check for > the individual extensions like Zvkb during probe but don't have anything > for the vector version (at least in the chacha20 and sha256 glue code). > If they don't, then we need to make sure those drivers do not probe with > the cut-down variants. As far as I know, none of the RISC-V vector crypto code has been tested with Zve* yet. Currently it always checks for VLEN >= 128, which should exclude most Zve* implementations. Currently it doesn't check for EEW >= 64, even though it sometimes assumes that. It looks like a check for EEW >= 64 needs to be added in order to exclude Zve32x and Zve32f implementations that don't support EEW == 64. If it would be useful to do so, we should be able to enable some of the code with a smaller VLEN and/or EEW once it has been tested in those configurations. Some of it should work, but some of it won't be able to work. (For example, the SHA512 instructions require EEW==64.) Also note that currently all the RISC-V vector crypto code only supports riscv64 (XLEN=64). Similarly, that could be relaxed in the future if people really need the vector crypto acceleration on 32-bit CPUs... But similarly, the code would need to be revised and tested in that configuration. > Eric/Jerry (although read the previous paragraph too): > I noticed that the sha256 glue code calls crypto_simd_usable(), and in > turn may_use_simd() before kernel_vector_begin(). The chacha20 glue code > does not call either, which seems to violate the edict in > kernel_vector_begin()'s kerneldoc: > "Must not be called unless may_use_simd() returns true." skcipher algorithms can only be invoked in process and softirq context. This differs from shash algorithms which can be invoked in any context. My understanding is that, like arm64, RISC-V always allows non-nested kernel-mode vector to be used in process and softirq context -- and in fact, this was intentionally done in order to support use cases like this. So that's why the RISC-V skcipher algorithms don't check for may_use_simd() before calling kernel_vector_begin(). Has that changed? If so, why? Some architectures like x86 do provide no-SIMD fallbacks for all skcipher algorithms, but it's very annoying to do. We were hoping to avoid that in RISC-V. - Eric
On Thu, Apr 18, 2024 at 08:52:56AM -0700, Eric Biggers wrote: > Hi Conor, > > On Thu, Apr 18, 2024 at 12:02:10PM +0100, Conor Dooley wrote: > > +CC Eric, Jerry > > > > On Fri, Apr 12, 2024 at 02:49:03PM +0800, Andy Chiu wrote: > > > Make has_vector take one argument. This argument represents the minimum > > > Vector subextension that the following Vector actions assume. > > > > > > Also, change riscv_v_first_use_handler(), and boot code that calls > > > riscv_v_setup_vsize() to accept the minimum Vector sub-extension, > > > ZVE32X. > > > > > > Most kernel/user interfaces requires minimum of ZVE32X. Thus, programs > > > compiled and run with ZVE32X should be supported by the kernel on most > > > aspects. This includes context-switch, signal, ptrace, prctl, and > > > hwprobe. > > > > > > One exception is that ELF_HWCAP returns 'V' only if full V is supported > > > on the platform. This means that the system without a full V must not > > > rely on ELF_HWCAP to tell whether it is allowable to execute Vector > > > without first invoking a prctl() check. > > > > > > Signed-off-by: Andy Chiu <andy.chiu@sifive.com> > > > Acked-by: Joel Granados <j.granados@samsung.com> > > > > I'm not sure that I like this patch to be honest. As far as I can tell, > > every user here of has_vector(ext) is ZVE32X, so why bother actually > > having an argument? > > > > Could we just document that has_vector() is just a tyre kick of "is > > there a vector unit and are we allowed to use it", and anything > > requiring more than the bare-minimum (so zve32x?)must explicitly check > > for that form of vector using riscv_has_extension_[un]likely()? > > > > Finally, the in-kernel crypto stuff or other things that use > > can_use_simd() to check for vector support - do they all function correctly > > with all of the vector flavours? I don't understand the vector > > extensions well enough to evaluate that - I know that they do check for > > the individual extensions like Zvkb during probe but don't have anything > > for the vector version (at least in the chacha20 and sha256 glue code). > > If they don't, then we need to make sure those drivers do not probe with > > the cut-down variants. > > As far as I know, none of the RISC-V vector crypto code has been tested with > Zve* yet. Currently it always checks for VLEN >= 128, which should exclude most > Zve* implementations. > > Currently it doesn't check for EEW >= 64, even though it sometimes assumes that. > It looks like a check for EEW >= 64 needs to be added in order to exclude Zve32x > and Zve32f implementations that don't support EEW == 64. Cool, glad I asked then :) > If it would be useful to do so, we should be able to enable some of the code > with a smaller VLEN and/or EEW once it has been tested in those configurations. > Some of it should work, but some of it won't be able to work. (For example, the > SHA512 instructions require EEW==64.) > > Also note that currently all the RISC-V vector crypto code only supports riscv64 > (XLEN=64). Similarly, that could be relaxed in the future if people really need > the vector crypto acceleration on 32-bit CPUs... But similarly, the code would > need to be revised and tested in that configuration. > > > Eric/Jerry (although read the previous paragraph too): > > I noticed that the sha256 glue code calls crypto_simd_usable(), and in > > turn may_use_simd() before kernel_vector_begin(). The chacha20 glue code > > does not call either, which seems to violate the edict in > > kernel_vector_begin()'s kerneldoc: > > "Must not be called unless may_use_simd() returns true." > > skcipher algorithms can only be invoked in process and softirq context. This > differs from shash algorithms which can be invoked in any context. > > My understanding is that, like arm64, RISC-V always allows non-nested > kernel-mode vector to be used in process and softirq context -- and in fact, > this was intentionally done in order to support use cases like this. So that's > why the RISC-V skcipher algorithms don't check for may_use_simd() before calling > kernel_vector_begin(). I see, thanks for explaining that. I think you should probably check somewhere if has_vector() returns true in that driver though before using vector instructions. Only checking vlen seems to me like relying on an implementation detail and if we set vlen for the T-Head/0.7.1 vector it'd be fooled. That said, I don't think that any of the 0.7.1 vector systems actually support Zvkb, but I hope you get my drift. Thanks, Conor.
On Thu, Apr 18, 2024 at 05:53:55PM +0100, Conor Dooley wrote: > > If it would be useful to do so, we should be able to enable some of the code > > with a smaller VLEN and/or EEW once it has been tested in those configurations. > > Some of it should work, but some of it won't be able to work. (For example, the > > SHA512 instructions require EEW==64.) > > > > Also note that currently all the RISC-V vector crypto code only supports riscv64 > > (XLEN=64). Similarly, that could be relaxed in the future if people really need > > the vector crypto acceleration on 32-bit CPUs... But similarly, the code would > > need to be revised and tested in that configuration. > > > > > Eric/Jerry (although read the previous paragraph too): > > > I noticed that the sha256 glue code calls crypto_simd_usable(), and in > > > turn may_use_simd() before kernel_vector_begin(). The chacha20 glue code > > > does not call either, which seems to violate the edict in > > > kernel_vector_begin()'s kerneldoc: > > > "Must not be called unless may_use_simd() returns true." > > > > skcipher algorithms can only be invoked in process and softirq context. This > > differs from shash algorithms which can be invoked in any context. > > > > My understanding is that, like arm64, RISC-V always allows non-nested > > kernel-mode vector to be used in process and softirq context -- and in fact, > > this was intentionally done in order to support use cases like this. So that's > > why the RISC-V skcipher algorithms don't check for may_use_simd() before calling > > kernel_vector_begin(). > > I see, thanks for explaining that. I think you should probably check > somewhere if has_vector() returns true in that driver though before > using vector instructions. Only checking vlen seems to me like relying on > an implementation detail and if we set vlen for the T-Head/0.7.1 vector > it'd be fooled. That said, I don't think that any of the 0.7.1 vector > systems actually support Zvkb, but I hope you get my drift. All the algorithms check for at least one of the vector crypto extensions being supported, for example Zvkb. 'if (riscv_isa_extension_available(NULL, ZVKB))' should return whether the ratified version of Zvkb is supported, and likewise for the other vector crypto extensions. The ratified version of the vector crypto extensions depends on the ratified version of the vector extension. So there should be no issue. If there is, the RISC-V core architecture code needs to be fixed to not declare that extensions are supported when they are actually incompatible non-standard versions of those extensions. Incompatible non-standard extensions should be represented as separate extensions. - Eric
On Thu, Apr 18, 2024 at 10:32:03AM -0700, Eric Biggers wrote: > On Thu, Apr 18, 2024 at 05:53:55PM +0100, Conor Dooley wrote: > > > If it would be useful to do so, we should be able to enable some of the code > > > with a smaller VLEN and/or EEW once it has been tested in those configurations. > > > Some of it should work, but some of it won't be able to work. (For example, the > > > SHA512 instructions require EEW==64.) > > > > > > Also note that currently all the RISC-V vector crypto code only supports riscv64 > > > (XLEN=64). Similarly, that could be relaxed in the future if people really need > > > the vector crypto acceleration on 32-bit CPUs... But similarly, the code would > > > need to be revised and tested in that configuration. > > > > > > > Eric/Jerry (although read the previous paragraph too): > > > > I noticed that the sha256 glue code calls crypto_simd_usable(), and in > > > > turn may_use_simd() before kernel_vector_begin(). The chacha20 glue code > > > > does not call either, which seems to violate the edict in > > > > kernel_vector_begin()'s kerneldoc: > > > > "Must not be called unless may_use_simd() returns true." > > > > > > skcipher algorithms can only be invoked in process and softirq context. This > > > differs from shash algorithms which can be invoked in any context. > > > > > > My understanding is that, like arm64, RISC-V always allows non-nested > > > kernel-mode vector to be used in process and softirq context -- and in fact, > > > this was intentionally done in order to support use cases like this. So that's > > > why the RISC-V skcipher algorithms don't check for may_use_simd() before calling > > > kernel_vector_begin(). > > > > I see, thanks for explaining that. I think you should probably check > > somewhere if has_vector() returns true in that driver though before > > using vector instructions. Only checking vlen seems to me like relying on > > an implementation detail and if we set vlen for the T-Head/0.7.1 vector > > it'd be fooled. That said, I don't think that any of the 0.7.1 vector > > systems actually support Zvkb, but I hope you get my drift. > > All the algorithms check for at least one of the vector crypto extensions being > supported, for example Zvkb. 'if (riscv_isa_extension_available(NULL, ZVKB))' > should return whether the ratified version of Zvkb is supported, and likewise > for the other vector crypto extensions. The ratified version of the vector > crypto extensions depends on the ratified version of the vector extension. So > there should be no issue. If there is, the RISC-V core architecture code needs > to be fixed to not declare that extensions are supported when they are actually > incompatible non-standard versions of those extensions. Incompatible > non-standard extensions should be represented as separate extensions. > It probably makes sense to check has_vector() to exclude Zve* for now, though. I am just concerned about how you're suggesting that non-standard extensions might be pretending to be standard ones and individual users of kernel-mode vector would need to work around that. I think that neither has_vector() nor 'if (riscv_isa_extension_available(NULL, ZVKB))' should return true if the CPU's vector extension is non-standard. - Eric
On Thu, Apr 18, 2024 at 10:39:46AM -0700, Eric Biggers wrote: > On Thu, Apr 18, 2024 at 10:32:03AM -0700, Eric Biggers wrote: > > On Thu, Apr 18, 2024 at 05:53:55PM +0100, Conor Dooley wrote: > > > > If it would be useful to do so, we should be able to enable some of the code > > > > with a smaller VLEN and/or EEW once it has been tested in those configurations. > > > > Some of it should work, but some of it won't be able to work. (For example, the > > > > SHA512 instructions require EEW==64.) > > > > > > > > Also note that currently all the RISC-V vector crypto code only supports riscv64 > > > > (XLEN=64). Similarly, that could be relaxed in the future if people really need > > > > the vector crypto acceleration on 32-bit CPUs... But similarly, the code would > > > > need to be revised and tested in that configuration. > > > > > > > > > Eric/Jerry (although read the previous paragraph too): > > > > > I noticed that the sha256 glue code calls crypto_simd_usable(), and in > > > > > turn may_use_simd() before kernel_vector_begin(). The chacha20 glue code > > > > > does not call either, which seems to violate the edict in > > > > > kernel_vector_begin()'s kerneldoc: > > > > > "Must not be called unless may_use_simd() returns true." > > > > > > > > skcipher algorithms can only be invoked in process and softirq context. This > > > > differs from shash algorithms which can be invoked in any context. > > > > > > > > My understanding is that, like arm64, RISC-V always allows non-nested > > > > kernel-mode vector to be used in process and softirq context -- and in fact, > > > > this was intentionally done in order to support use cases like this. So that's > > > > why the RISC-V skcipher algorithms don't check for may_use_simd() before calling > > > > kernel_vector_begin(). > > > > > > I see, thanks for explaining that. I think you should probably check > > > somewhere if has_vector() returns true in that driver though before > > > using vector instructions. Only checking vlen seems to me like relying on > > > an implementation detail and if we set vlen for the T-Head/0.7.1 vector > > > it'd be fooled. That said, I don't think that any of the 0.7.1 vector > > > systems actually support Zvkb, but I hope you get my drift. > > > > All the algorithms check for at least one of the vector crypto extensions being > > supported, for example Zvkb. 'if (riscv_isa_extension_available(NULL, ZVKB))' > > should return whether the ratified version of Zvkb is supported, and likewise > > for the other vector crypto extensions. The ratified version of the vector > > crypto extensions depends on the ratified version of the vector extension. That's great if it does require that the version of the vector extension must be standard. Higher quality spec than most if it does. But "supported" in the context of riscv_isa_extension_available() means that the hardware supports it (or set of harts), not that the currently running kernel does. The Kconfig deps that must be met for the code to be built at least mean the kernel is built with vector support, leaving only "the kernel was built with vector support and the hardware supports vector but for $reason the kernel refused to enable it". I'm not sure if that final condition is actually possible with the system ending up in a broken state, however - I'm not sure that we ever do turn off access to the VPU at present (after we mark it usable), and if we do it doesn't get reflected in has_vector() so the kernel and userspace would both break, with what a crypto driver does probably being the least of your worries. > > So > > there should be no issue. If there is, the RISC-V core architecture code needs > > to be fixed to not declare that extensions are supported when they are actually > > incompatible non-standard versions of those extensions. Incompatible > > non-standard extensions should be represented as separate extensions. > > > > It probably makes sense to check has_vector() to exclude Zve* for now, though. I think you might actually be better served at present, given the code can only be built if the core vector code is, by using riscv_isa_extension_available(NULL, v). That way you know for sure that you're getting the ratified extension and nothing else. Prior to this conversation I thought that has_vector() should return true if there's a standard compliant vector unit available - given all users Andy added only need Zve32x. > I am just concerned about how you're suggesting that non-standard extensions > might be pretending to be standard ones and individual users of kernel-mode > vector would need to work around that. I am absolutely not suggesting that non-standard extensions should masquerade as standard ones, I don't know where you got that from. What I said was that a non-standard vector extension could reuse riscv_v_vlen (and should IMO for simplicity reasons), not that any of the APIs we have for checking extension availability would lie and say it was standard. riscv_v_vlen having a value greater than 128 is not one of those APIs ;) > I think that neither has_vector() nor > 'if (riscv_isa_extension_available(NULL, ZVKB))' should return true if the CPU's > vector extension is non-standard. riscv_isa_extension_available(NULL, ZVKB) only checks whether the extension was present in DT or ACPI for all harts. It doesn't check whether or not the required config option for vector has been set or anything related to dependencies. has_vector() at least checks that the vector core has been enabled (and uses the alternative-patched version of the check given it is used in some hotter paths). That's kinda moot for code that's only built if the vector core stuff is enabled as I said above though. We could of course make riscv_isa_extension_available() check extension dependencies, but I'd rather leave dt validation to the dt tooling (apparently ACPI tables are never wrong...). Either would allow you to rely on the crypto extensions present only when the standard vector extensions unless someone's DT/ACPI stuff is shite, but then they keep the pieces IMO :) Hope that makes sense? Conor.
On Thu, Apr 18, 2024 at 07:26:00PM +0100, Conor Dooley wrote: > On Thu, Apr 18, 2024 at 10:39:46AM -0700, Eric Biggers wrote: > > On Thu, Apr 18, 2024 at 10:32:03AM -0700, Eric Biggers wrote: > > > On Thu, Apr 18, 2024 at 05:53:55PM +0100, Conor Dooley wrote: > > > > > If it would be useful to do so, we should be able to enable some of the code > > > > > with a smaller VLEN and/or EEW once it has been tested in those configurations. > > > > > Some of it should work, but some of it won't be able to work. (For example, the > > > > > SHA512 instructions require EEW==64.) > > > > > > > > > > Also note that currently all the RISC-V vector crypto code only supports riscv64 > > > > > (XLEN=64). Similarly, that could be relaxed in the future if people really need > > > > > the vector crypto acceleration on 32-bit CPUs... But similarly, the code would > > > > > need to be revised and tested in that configuration. > > > > > > > > > > > Eric/Jerry (although read the previous paragraph too): > > > > > > I noticed that the sha256 glue code calls crypto_simd_usable(), and in > > > > > > turn may_use_simd() before kernel_vector_begin(). The chacha20 glue code > > > > > > does not call either, which seems to violate the edict in > > > > > > kernel_vector_begin()'s kerneldoc: > > > > > > "Must not be called unless may_use_simd() returns true." > > > > > > > > > > skcipher algorithms can only be invoked in process and softirq context. This > > > > > differs from shash algorithms which can be invoked in any context. > > > > > > > > > > My understanding is that, like arm64, RISC-V always allows non-nested > > > > > kernel-mode vector to be used in process and softirq context -- and in fact, > > > > > this was intentionally done in order to support use cases like this. So that's > > > > > why the RISC-V skcipher algorithms don't check for may_use_simd() before calling > > > > > kernel_vector_begin(). > > > > > > > > I see, thanks for explaining that. I think you should probably check > > > > somewhere if has_vector() returns true in that driver though before > > > > using vector instructions. Only checking vlen seems to me like relying on > > > > an implementation detail and if we set vlen for the T-Head/0.7.1 vector > > > > it'd be fooled. That said, I don't think that any of the 0.7.1 vector > > > > systems actually support Zvkb, but I hope you get my drift. > > > > > > All the algorithms check for at least one of the vector crypto extensions being > > > supported, for example Zvkb. 'if (riscv_isa_extension_available(NULL, ZVKB))' > > > should return whether the ratified version of Zvkb is supported, and likewise > > > for the other vector crypto extensions. The ratified version of the vector > > > crypto extensions depends on the ratified version of the vector extension. > > That's great if it does require that the version of the vector extension > must be standard. Higher quality spec than most if it does. But > "supported" in the context of riscv_isa_extension_available() means that > the hardware supports it (or set of harts), not that the currently > running kernel does. The Kconfig deps that must be met for the code to be > built at least mean the kernel is built with vector support, leaving only > "the kernel was built with vector support and the hardware supports vector > but for $reason the kernel refused to enable it". > > I'm not sure if that final condition is actually possible with the system > ending up in a broken state, however - I'm not sure that we ever do turn > off access to the VPU at present (after we mark it usable), and if we do > it doesn't get reflected in has_vector() so the kernel and userspace would > both break, with what a crypto driver does probably being the least of > your worries. > > > > So > > > there should be no issue. If there is, the RISC-V core architecture code needs > > > to be fixed to not declare that extensions are supported when they are actually > > > incompatible non-standard versions of those extensions. Incompatible > > > non-standard extensions should be represented as separate extensions. > > > > > > > It probably makes sense to check has_vector() to exclude Zve* for now, though. > > I think you might actually be better served at present, given the code can > only be built if the core vector code is, by using > riscv_isa_extension_available(NULL, v). That way you know for sure that > you're getting the ratified extension and nothing else. Poor choice of wording here - I meant, of course, the "main" vector extension, rather than the Zve* variants. > Prior to this conversation I thought that has_vector() should return true > if there's a standard compliant vector unit available - given all users > Andy added only need Zve32x. > > > I am just concerned about how you're suggesting that non-standard extensions > > might be pretending to be standard ones and individual users of kernel-mode > > vector would need to work around that. > > I am absolutely not suggesting that non-standard extensions should > masquerade as standard ones, I don't know where you got that from. What > I said was that a non-standard vector extension could reuse riscv_v_vlen > (and should IMO for simplicity reasons), not that any of the APIs we have > for checking extension availability would lie and say it was standard. > riscv_v_vlen having a value greater than 128 is not one of those APIs ;) > > > I think that neither has_vector() nor > > 'if (riscv_isa_extension_available(NULL, ZVKB))' should return true if the CPU's > > vector extension is non-standard. > > riscv_isa_extension_available(NULL, ZVKB) only checks whether the extension > was present in DT or ACPI for all harts. It doesn't check whether or not > the required config option for vector has been set or anything related > to dependencies. has_vector() at least checks that the vector core has > been enabled (and uses the alternative-patched version of the check > given it is used in some hotter paths). That's kinda moot for code > that's only built if the vector core stuff is enabled as I said above > though. > > We could of course make riscv_isa_extension_available() check > extension dependencies, but I'd rather leave dt validation to the dt > tooling (apparently ACPI tables are never wrong...). Either would allow > you to rely on the crypto extensions present only when the standard vector > extensions unless someone's DT/ACPI stuff is shite, but then they keep the > pieces IMO :) > > Hope that makes sense? > Conor.
On Thu, Apr 18, 2024 at 07:26:00PM +0100, Conor Dooley wrote: > That's great if it does require that the version of the vector extension > must be standard. Higher quality spec than most if it does. But > "supported" in the context of riscv_isa_extension_available() means that > the hardware supports it (or set of harts), not that the currently > running kernel does. The Kconfig deps that must be met for the code to be > built at least mean the kernel is built with vector support, leaving only > "the kernel was built with vector support and the hardware supports vector > but for $reason the kernel refused to enable it". > > I'm not sure if that final condition is actually possible with the system > ending up in a broken state, however - I'm not sure that we ever do turn > off access to the VPU at present (after we mark it usable), and if we do > it doesn't get reflected in has_vector() so the kernel and userspace would > both break, with what a crypto driver does probably being the least of > your worries. > > > I am just concerned about how you're suggesting that non-standard extensions > > might be pretending to be standard ones and individual users of kernel-mode > > vector would need to work around that. > > I am absolutely not suggesting that non-standard extensions should > masquerade as standard ones, I don't know where you got that from. What > I said was that a non-standard vector extension could reuse riscv_v_vlen > (and should IMO for simplicity reasons), not that any of the APIs we have > for checking extension availability would lie and say it was standard. > riscv_v_vlen having a value greater than 128 is not one of those APIs ;) It sounded like you were suggesting that a CPU could plausibly have a pre-standard version of the vector extension but also have standard Zvkb. I don't think this makes sense, due to the dependency. > > I think that neither has_vector() nor > > 'if (riscv_isa_extension_available(NULL, ZVKB))' should return true if the CPU's > > vector extension is non-standard. > > riscv_isa_extension_available(NULL, ZVKB) only checks whether the extension > was present in DT or ACPI for all harts. It doesn't check whether or not > the required config option for vector has been set or anything related > to dependencies. has_vector() at least checks that the vector core has > been enabled (and uses the alternative-patched version of the check > given it is used in some hotter paths). That's kinda moot for code > that's only built if the vector core stuff is enabled as I said above > though. > > We could of course make riscv_isa_extension_available() check > extension dependencies, but I'd rather leave dt validation to the dt > tooling (apparently ACPI tables are never wrong...). Either would allow > you to rely on the crypto extensions present only when the standard vector > extensions unless someone's DT/ACPI stuff is shite, but then they keep the > pieces IMO :) > > Hope that makes sense? If the RISC-V kernel ever disables V, then it should also disable everything that depends on V. This would be similar to how on x86, if the kernel decides to disable AVX to mitigate the Gather Data Sampling vulnerability, it also disables AVX2, AVX512, VAES, VPCLMULQDQ, etc. See cpuid_deps[] in arch/x86/kernel/cpu/cpuid-deps.c. Sometimes CPU features depend on other ones. That's just the way things work. Whenever possible that should be handled centrally, not pushed down to every user both in-kernel and userspace. - Eric
On Thu, Apr 18, 2024 at 11:41:29AM -0700, Eric Biggers wrote: > If the RISC-V kernel ever disables V, then it should also disable everything > that depends on V. > > This would be similar to how on x86, if the kernel decides to disable AVX to > mitigate the Gather Data Sampling vulnerability, it also disables AVX2, AVX512, > VAES, VPCLMULQDQ, etc. See cpuid_deps[] in arch/x86/kernel/cpu/cpuid-deps.c. > > Sometimes CPU features depend on other ones. That's just the way things work. 🤨 > Whenever possible that should be handled centrally, not pushed down to every > user both in-kernel and userspace. FWIW, anything detected but rejected while probing the DT or ACPI tables will cause riscv_isa_extension_available() (or other APIs) to return false.
Hi Conor, On Fri, Apr 19, 2024 at 2:26 AM Conor Dooley <conor@kernel.org> wrote: > > On Thu, Apr 18, 2024 at 10:39:46AM -0700, Eric Biggers wrote: > > On Thu, Apr 18, 2024 at 10:32:03AM -0700, Eric Biggers wrote: > > > On Thu, Apr 18, 2024 at 05:53:55PM +0100, Conor Dooley wrote: > > > > > If it would be useful to do so, we should be able to enable some of the code > > > > > with a smaller VLEN and/or EEW once it has been tested in those configurations. > > > > > Some of it should work, but some of it won't be able to work. (For example, the > > > > > SHA512 instructions require EEW==64.) > > > > > > > > > > Also note that currently all the RISC-V vector crypto code only supports riscv64 > > > > > (XLEN=64). Similarly, that could be relaxed in the future if people really need > > > > > the vector crypto acceleration on 32-bit CPUs... But similarly, the code would > > > > > need to be revised and tested in that configuration. > > > > > > > > > > > Eric/Jerry (although read the previous paragraph too): > > > > > > I noticed that the sha256 glue code calls crypto_simd_usable(), and in > > > > > > turn may_use_simd() before kernel_vector_begin(). The chacha20 glue code > > > > > > does not call either, which seems to violate the edict in > > > > > > kernel_vector_begin()'s kerneldoc: > > > > > > "Must not be called unless may_use_simd() returns true." > > > > > > > > > > skcipher algorithms can only be invoked in process and softirq context. This > > > > > differs from shash algorithms which can be invoked in any context. > > > > > > > > > > My understanding is that, like arm64, RISC-V always allows non-nested > > > > > kernel-mode vector to be used in process and softirq context -- and in fact, > > > > > this was intentionally done in order to support use cases like this. So that's > > > > > why the RISC-V skcipher algorithms don't check for may_use_simd() before calling > > > > > kernel_vector_begin(). > > > > > > > > I see, thanks for explaining that. I think you should probably check > > > > somewhere if has_vector() returns true in that driver though before > > > > using vector instructions. Only checking vlen seems to me like relying on > > > > an implementation detail and if we set vlen for the T-Head/0.7.1 vector > > > > it'd be fooled. That said, I don't think that any of the 0.7.1 vector > > > > systems actually support Zvkb, but I hope you get my drift. > > > > > > All the algorithms check for at least one of the vector crypto extensions being > > > supported, for example Zvkb. 'if (riscv_isa_extension_available(NULL, ZVKB))' > > > should return whether the ratified version of Zvkb is supported, and likewise > > > for the other vector crypto extensions. The ratified version of the vector > > > crypto extensions depends on the ratified version of the vector extension. > > That's great if it does require that the version of the vector extension > must be standard. Higher quality spec than most if it does. But > "supported" in the context of riscv_isa_extension_available() means that > the hardware supports it (or set of harts), not that the currently > running kernel does. The Kconfig deps that must be met for the code to be > built at least mean the kernel is built with vector support, leaving only > "the kernel was built with vector support and the hardware supports vector > but for $reason the kernel refused to enable it". > > I'm not sure if that final condition is actually possible with the system > ending up in a broken state, however - I'm not sure that we ever do turn > off access to the VPU at present (after we mark it usable), and if we do > it doesn't get reflected in has_vector() so the kernel and userspace would > both break, with what a crypto driver does probably being the least of > your worries. > > > > So > > > there should be no issue. If there is, the RISC-V core architecture code needs > > > to be fixed to not declare that extensions are supported when they are actually > > > incompatible non-standard versions of those extensions. Incompatible > > > non-standard extensions should be represented as separate extensions. > > > > > > > It probably makes sense to check has_vector() to exclude Zve* for now, though. > > I think you might actually be better served at present, given the code can > only be built if the core vector code is, by using > riscv_isa_extension_available(NULL, v). That way you know for sure that > you're getting the ratified extension and nothing else. > > Prior to this conversation I thought that has_vector() should return true > if there's a standard compliant vector unit available - given all users > Andy added only need Zve32x. > > > I am just concerned about how you're suggesting that non-standard extensions > > might be pretending to be standard ones and individual users of kernel-mode > > vector would need to work around that. > > I am absolutely not suggesting that non-standard extensions should > masquerade as standard ones, I don't know where you got that from. What > I said was that a non-standard vector extension could reuse riscv_v_vlen > (and should IMO for simplicity reasons), not that any of the APIs we have > for checking extension availability would lie and say it was standard. > riscv_v_vlen having a value greater than 128 is not one of those APIs ;) > > > I think that neither has_vector() nor > > 'if (riscv_isa_extension_available(NULL, ZVKB))' should return true if the CPU's > > vector extension is non-standard. > > riscv_isa_extension_available(NULL, ZVKB) only checks whether the extension > was present in DT or ACPI for all harts. It doesn't check whether or not > the required config option for vector has been set or anything related > to dependencies. has_vector() at least checks that the vector core has > been enabled (and uses the alternative-patched version of the check > given it is used in some hotter paths). That's kinda moot for code > that's only built if the vector core stuff is enabled as I said above > though. > > We could of course make riscv_isa_extension_available() check > extension dependencies, but I'd rather leave dt validation to the dt > tooling (apparently ACPI tables are never wrong...). Either would allow > you to rely on the crypto extensions present only when the standard vector > extensions unless someone's DT/ACPI stuff is shite, but then they keep the > pieces IMO :) Should we check if "v" presents for vector crypto extensions in riscv_isa_extension_check()? We are not checking this for now. So a kernel compiled with RISCV_ISA_V still has a problem if its isa-string includes any of vector crypto ("zvbb, zvkg, etc") but not "v". > > Hope that makes sense? > Conor. Cheers, Andy
On Thu, May 09, 2024 at 02:56:30PM +0800, Andy Chiu wrote: > Hi Conor, > > Should we check if "v" presents for vector crypto extensions in > riscv_isa_extension_check()? We are not checking this for now. So a > kernel compiled with RISCV_ISA_V still has a problem if its isa-string > includes any of vector crypto ("zvbb, zvkg, etc") but not "v". Yeah, one of the things I took away from this discussion is that we need to improve the implementation of both the methods we have at the moment for drivers etc to check if extensions are present and usable. In general, I don't think checks like that are "safe" to do in riscv_isa_extension_check(), because the dependencies may not all have been resolved when we probe an extension (Clement's current Zca etc series improves the situation though by only calling the checks after we probe all extensions). The simple V cases are all fine though - the DT binding and ACPI rules for compatible strings all mandate that single-letter extensions must come before multi-letter ones. For riscv,isa-extensions we control the probe ordering and probe V before any multi-letter stuff. Additionally, we should make it a requirement for V to be present if things that depend on it are. That said, is it permitted by the specs to have any of the extensions you mention without the full V extension, but with one of the cut-down variants you mention here? If not, I'd be more interested in figuring out the non-extension dependencies: whether or not the kernel itself supports vector and if the kernel has opted to disable vector due to detecting that harts have mismatching vector lengths. TL;DR: I think we should add some checks in riscv_isa_extension_check(). Thanks, Conor.
On Thu, May 09, 2024 at 08:48:09AM +0100, Conor Dooley wrote: > On Thu, May 09, 2024 at 02:56:30PM +0800, Andy Chiu wrote: > > Hi Conor, > > > > Should we check if "v" presents for vector crypto extensions in > > riscv_isa_extension_check()? We are not checking this for now. So a > > kernel compiled with RISCV_ISA_V still has a problem if its isa-string > > includes any of vector crypto ("zvbb, zvkg, etc") but not "v". > > > Yeah, one of the things I took away from this discussion is that we need > to improve the implementation of both the methods we have at the moment > for drivers etc to check if extensions are present and usable. > In general, I don't think checks like that are "safe" to do in > riscv_isa_extension_check(), because the dependencies may not all have > been resolved when we probe an extension (Clement's current Zca etc > series improves the situation though by only calling the checks after > we probe all extensions). > > The simple V cases are all fine though - the DT binding and ACPI rules > for compatible strings all mandate that single-letter extensions must > come before multi-letter ones. For riscv,isa-extensions we control the > probe ordering and probe V before any multi-letter stuff. Additionally, > we should make it a requirement for V to be present if things that > depend on it are. > > That said, is it permitted by the specs to have any of the extensions > you mention without the full V extension, but with one of the cut-down > variants you mention here? If not, I'd be more interested in figuring > out the non-extension dependencies: whether or not the kernel itself > supports vector and if the kernel has opted to disable vector due to > detecting that harts have mismatching vector lengths. > > TL;DR: I think we should add some checks in riscv_isa_extension_check(). Also, unless this only becomes a problem with this series that adds the cut-down forms of vector, I think this is a separate problem to solve and I can send some patches for it (along with some other cleanup I'd like to do as a result of Eric's comments) and you can just submit the v2 you were planning to without it. I can't, off the top of my head, think of why this particular series would break the vector crypto stuff though, the problems with enabling extensions seem underlying. Thanks, Conor.
On Thu, May 09, 2024 at 09:25:25AM +0100, Conor Dooley wrote: > On Thu, May 09, 2024 at 08:48:09AM +0100, Conor Dooley wrote: > > On Thu, May 09, 2024 at 02:56:30PM +0800, Andy Chiu wrote: > > > Hi Conor, > > > > > > Should we check if "v" presents for vector crypto extensions in > > > riscv_isa_extension_check()? We are not checking this for now. So a > > > kernel compiled with RISCV_ISA_V still has a problem if its isa-string > > > includes any of vector crypto ("zvbb, zvkg, etc") but not "v". > > > > > > Yeah, one of the things I took away from this discussion is that we need > > to improve the implementation of both the methods we have at the moment > > for drivers etc to check if extensions are present and usable. > > In general, I don't think checks like that are "safe" to do in > > riscv_isa_extension_check(), because the dependencies may not all have > > been resolved when we probe an extension (Clement's current Zca etc > > series improves the situation though by only calling the checks after > > we probe all extensions). > > > > The simple V cases are all fine though - the DT binding and ACPI rules > > for compatible strings all mandate that single-letter extensions must > > come before multi-letter ones. For riscv,isa-extensions we control the > > probe ordering and probe V before any multi-letter stuff. Additionally, > > we should make it a requirement for V to be present if things that > > depend on it are. > > > > That said, is it permitted by the specs to have any of the extensions > > you mention without the full V extension, but with one of the cut-down > > variants you mention here? If not, I'd be more interested in figuring > > out the non-extension dependencies: whether or not the kernel itself > > supports vector and if the kernel has opted to disable vector due to > > detecting that harts have mismatching vector lengths. > > > > TL;DR: I think we should add some checks in riscv_isa_extension_check(). > > Also, unless this only becomes a problem with this series that adds the > cut-down forms of vector, I think this is a separate problem to solve > and I can send some patches for it (along with some other cleanup I'd like > to do as a result of Eric's comments) and you can just submit the v2 you > were planning to without it. I can't, off the top of my head, think of > why this particular series would break the vector crypto stuff though, > the problems with enabling extensions seem underlying. Here's something buggy that I chucked together as an idea of what I meant: https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=riscv-check_vector Beware, it is entirely untested :) It's based on both this series and patches 2 & 3 of Charlie's series doing the T-Head vector stuff. It really needs Clement's extension_check() rework that I mentioned 2 mails ago to function correctly for any of these vector subsets. Without Clement's stuff, it'll have "random" behaviour depending on probe order for riscv,isa and a determinate, but incorrect, behaviour otherwise. Cheers, Conor.