Re: [PATCH v4 7/9] riscv: vector: adjust minimum Vector requirement to ZVE32X

[Conor Dooley <conor.dooley@microchip.com>]

[-- Attachment #1: Type: text/plain, Size: 5922 bytes --]

+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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]