Re: [PATCH v2 1/2] KVM: riscv: Eliminate redundancy in get_reg function via pointers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] KVM: riscv: runtime math replaces constants in kvm_riscv_vcpu_get_reg_fp()
- [Low] KVM: riscv: magic numbers and untyped math in kvm_riscv_vcpu_get_reg_fp()
--

commit 6aba685edaa53c11084934b49cbe82d9d80cee01
Author: Paul White <paul.white.kernel@gmail.com>

KVM: riscv: Eliminate redundancy in get_reg function via pointers

This commit refactors kvm_riscv_vcpu_get_reg_fp() in arch/riscv/kvm/vcpu_fp.c
to store floating-point mode parameters in local variables and compute
register addresses using byte-level pointer arithmetic.

> diff --git a/arch/riscv/kvm/vcpu_fp.c b/arch/riscv/kvm/vcpu_fp.c
> index bb11e6757d349..da4f83be6b19f 100644
> --- a/arch/riscv/kvm/vcpu_fp.c
> +++ b/arch/riscv/kvm/vcpu_fp.c
> @@ -83,41 +83,51 @@ int kvm_riscv_vcpu_get_reg_fp(struct kvm_vcpu *vcpu,
>  	struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
>  	unsigned long __user *uaddr =
>  			(unsigned long __user *)(unsigned long)reg->addr;
> -	unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
> +	unsigned long token_reg_num = reg->id & ~(KVM_REG_ARCH_MASK |
>  					    KVM_REG_SIZE_MASK |
>  					    rtype);
>  	void *reg_val;
>  
> +	u32 *fcsr_addr;
> +	void *f_array_addr;
> +	size_t byte_offset = 0;
> +	unsigned long offical_fcsr_num = 0;
> +	unsigned long offical_first_reg = 0;
> +	unsigned long offical_last_reg = 0;
> +
>  	if ((rtype == KVM_REG_RISCV_FP_F) &&
>  	    riscv_isa_extension_available(vcpu->arch.isa, F)) {
> +		fcsr_addr = &cntx->fp.f.fcsr;
> +		f_array_addr = &cntx->fp.f.f;
> +		byte_offset = 4;
> +		offical_fcsr_num = KVM_REG_RISCV_FP_F_REG(fcsr);
> +		offical_first_reg = KVM_REG_RISCV_FP_F_REG(f[0]);
> +		offical_last_reg = KVM_REG_RISCV_FP_F_REG(f[31]);
> +	} else if  ((rtype == KVM_REG_RISCV_FP_D) &&
> +			   riscv_isa_extension_available(vcpu->arch.isa, D)) {
> +		fcsr_addr = &cntx->fp.d.fcsr;
> +		f_array_addr = &cntx->fp.d.f;
> +		byte_offset = 8;
> +		offical_fcsr_num = KVM_REG_RISCV_FP_D_REG(fcsr);
> +		offical_first_reg = KVM_REG_RISCV_FP_D_REG(f[0]);
> +		offical_last_reg = KVM_REG_RISCV_FP_D_REG(f[31]);
> +	} else {
> +		return -ENOENT;
> +	}
> +
> +	if (token_reg_num == offical_fcsr_num) {
>  		if (KVM_REG_SIZE(reg->id) != sizeof(u32))
>  			return -EINVAL;
> -		if (reg_num == KVM_REG_RISCV_FP_F_REG(fcsr))
> -			reg_val = &cntx->fp.f.fcsr;
> -		else if ((KVM_REG_RISCV_FP_F_REG(f[0]) <= reg_num) &&
> -			  reg_num <= KVM_REG_RISCV_FP_F_REG(f[31])) {
> -			reg_num = array_index_nospec(reg_num,
> -					ARRAY_SIZE(cntx->fp.f.f));
> -			reg_val = &cntx->fp.f.f[reg_num];
> -		} else
> -			return -ENOENT;
> -	} else if ((rtype == KVM_REG_RISCV_FP_D) &&
> -		   riscv_isa_extension_available(vcpu->arch.isa, D)) {
> -		if (reg_num == KVM_REG_RISCV_FP_D_REG(fcsr)) {
> -			if (KVM_REG_SIZE(reg->id) != sizeof(u32))
> -				return -EINVAL;
> -			reg_val = &cntx->fp.d.fcsr;
> -		} else if ((KVM_REG_RISCV_FP_D_REG(f[0]) <= reg_num) &&
> -			   reg_num <= KVM_REG_RISCV_FP_D_REG(f[31])) {
> -			if (KVM_REG_SIZE(reg->id) != sizeof(u64))
> -				return -EINVAL;
> -			reg_num = array_index_nospec(reg_num,
> -					ARRAY_SIZE(cntx->fp.d.f));
> -			reg_val = &cntx->fp.d.f[reg_num];
> -		} else
> -			return -ENOENT;
> -	} else
> +		reg_val = fcsr_addr;
> +	} else if ((offical_first_reg <= token_reg_num) && (token_reg_num <= offical_last_reg)) {
> +		if (KVM_REG_SIZE(reg->id) != byte_offset)
> +			return -EINVAL;
> +		token_reg_num = array_index_nospec(token_reg_num,
> +						   32);
> +		reg_val = (u8 *)f_array_addr + (token_reg_num * byte_offset);

[Severity: Low]
Does this refactoring in kvm_riscv_vcpu_get_reg_fp() actually reduce branch
decisions or improve address calculation when userspace invokes the
KVM_GET_ONE_REG ioctl for F or D extension registers?

In the original code, the KVM_REG_RISCV_FP_F and KVM_REG_RISCV_FP_D branches
are mutually exclusive, so the CPU evaluates the same number of conditional
checks per call. In C, struct member access and typed array indexing such as
&cntx->fp.f.f[reg_num] and &cntx->fp.d.f[reg_num] resolve struct offsets at
compile time and scale the index by a constant power of two.

Moving byte_offset, offical_fcsr_num, offical_first_reg, and offical_last_reg
into runtime local variables replaces compile-time constants (sizeof(u32),
sizeof(u64), and KVM_REG_RISCV_FP_F_REG / KVM_REG_RISCV_FP_D_REG) with
runtime state and variable multiplication (token_reg_num * byte_offset).

> +	} else {
>  		return -ENOENT;
> +	}
>  
>  	if (copy_to_user(uaddr, reg_val, KVM_REG_SIZE(reg->id)))
>  		return -EFAULT;
> @@ -144,9 +154,9 @@ int kvm_riscv_vcpu_set_reg_fp(struct kvm_vcpu *vcpu,
>  		if (reg_num == KVM_REG_RISCV_FP_F_REG(fcsr))
>  			reg_val = &cntx->fp.f.fcsr;
>  		else if ((KVM_REG_RISCV_FP_F_REG(f[0]) <= reg_num) &&
> -			  reg_num <= KVM_REG_RISCV_FP_F_REG(f[31])) {
> +			 reg_num <= KVM_REG_RISCV_FP_F_REG(f[31])) {
>  			reg_num = array_index_nospec(reg_num,
> -					ARRAY_SIZE(cntx->fp.f.f));
> +						     ARRAY_SIZE(cntx->fp.f.f));
>  			reg_val = &cntx->fp.f.f[reg_num];
>  		} else
>  			return -ENOENT;
> @@ -161,7 +171,7 @@ int kvm_riscv_vcpu_set_reg_fp(struct kvm_vcpu *vcpu,
>  			if (KVM_REG_SIZE(reg->id) != sizeof(u64))
>  				return -EINVAL;
>  			reg_num = array_index_nospec(reg_num,
> -					ARRAY_SIZE(cntx->fp.d.f));
> +						     ARRAY_SIZE(cntx->fp.d.f));

[Severity: Low]
This isn't a bug, but there are several code quality issues in
arch/riscv/kvm/vcpu_fp.c:

In kvm_riscv_vcpu_get_reg_fp(), sizeof(u32), sizeof(u64), and ARRAY_SIZE()
are replaced with hardcoded magic numbers (4, 8, and 32), and typed array
access is replaced with untyped void * and u8 * pointer arithmetic.

In addition, while offical_first_reg and offical_last_reg are introduced to
abstract the register range check, the array indexing does not subtract
offical_first_reg and hardcodes 32 in array_index_nospec(token_reg_num, 32).
Would (u8 *)f_array_addr + (token_reg_num * byte_offset) compute the wrong
offset if a future floating-point mode has a non-zero first register offset
or a different register count?

Also, the new variable names have a typo (offical_fcsr_num,
offical_first_reg, and offical_last_reg instead of official_*), and this
patch includes unrelated whitespace changes in kvm_riscv_vcpu_set_reg_fp().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793223506.git.paul.white.kernel@gmail.com?part=1