Re: [PATCH v2 05/15] KVM: arm64: nv: Load guest hyp's ZCR into EL1 state

[Marc Zyngier <maz@kernel.org>]

On Thu, 13 Jun 2024 21:17:46 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Load the guest hypervisor's ZCR_EL2 into the corresponding EL1 register
> when restoring SVE state, as ZCR_EL2 affects the VL in the hypervisor
> context.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/kvm_host.h       | 4 ++++
>  arch/arm64/kvm/hyp/include/hyp/switch.h | 3 ++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 8170c04fde91..e01e6de414f1 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -844,6 +844,10 @@ struct kvm_vcpu_arch {
>  
>  #define vcpu_sve_max_vq(vcpu)	sve_vq_from_vl((vcpu)->arch.sve_max_vl)
>  
> +#define vcpu_sve_zcr_el1(vcpu)						\
> +	(unlikely(is_hyp_ctxt(vcpu)) ? __vcpu_sys_reg(vcpu, ZCR_EL2) :	\
> +				       __vcpu_sys_reg(vcpu, ZCR_EL1))
> +

I have the feeling this abstracts the access at the wrong level. It's
not that it gives the wrong result, but it hides the register and is
only concerned with the *value*.

In turn, it makes the helper unusable with the *write* side, as shown
in patch 7.

>  #define vcpu_sve_state_size(vcpu) ({					\
>  	size_t __size_ret;						\
>  	unsigned int __vcpu_vq;						\
> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index 5ecd2600d9df..71a93e336a0c 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> @@ -317,7 +317,8 @@ static inline void __hyp_sve_restore_guest(struct kvm_vcpu *vcpu)
>  	sve_cond_update_zcr_vq(vcpu_sve_max_vq(vcpu) - 1, SYS_ZCR_EL2);
>  	__sve_restore_state(vcpu_sve_pffr(vcpu),
>  			    &vcpu->arch.ctxt.fp_regs.fpsr);
> -	write_sysreg_el1(__vcpu_sys_reg(vcpu, ZCR_EL1), SYS_ZCR);
> +
> +	write_sysreg_el1(vcpu_sve_zcr_el1(vcpu), SYS_ZCR);
>  }
>  
>  /*

I would instead propose the following:

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index aeb1c567dfad..2c3eff0031eb 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -845,9 +845,8 @@ struct kvm_vcpu_arch {
 
 #define vcpu_sve_max_vq(vcpu)	sve_vq_from_vl((vcpu)->arch.sve_max_vl)
 
-#define vcpu_sve_zcr_el1(vcpu)						\
-	(unlikely(is_hyp_ctxt(vcpu)) ? __vcpu_sys_reg(vcpu, ZCR_EL2) :	\
-				       __vcpu_sys_reg(vcpu, ZCR_EL1))
+#define vcpu_sve_zcr_elx(vcpu)						\
+	(unlikely(is_hyp_ctxt(vcpu)) ? ZCR_EL2 : ZCR_EL1)
 
 #define vcpu_sve_state_size(vcpu) ({					\
 	size_t __size_ret;						\
diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index bb2ef3166c63..947486a111e1 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c
@@ -179,10 +179,7 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
 			 * If the vCPU is in the hyp context then ZCR_EL1 is
 			 * loaded with its vEL2 counterpart.
 			 */
-			if (is_hyp_ctxt(vcpu))
-				__vcpu_sys_reg(vcpu, ZCR_EL2) = zcr;
-			else
-				__vcpu_sys_reg(vcpu, ZCR_EL1) = zcr;
+			__vcpu_sys_reg(vcpu, vcpu_sve_zcr_elx(vcpu)) = zcr;
 
 			/*
 			 * Restore the VL that was saved when bound to the CPU,
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
index 0a6935a18490..ad8dec0b450b 100644
--- a/arch/arm64/kvm/hyp/include/hyp/switch.h
+++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
@@ -330,7 +330,7 @@ static inline void __hyp_sve_restore_guest(struct kvm_vcpu *vcpu)
 	if (vcpu_has_nv(vcpu) && !is_hyp_ctxt(vcpu))
 		sve_cond_update_zcr_vq(__vcpu_sys_reg(vcpu, ZCR_EL2), SYS_ZCR_EL2);
 
-	write_sysreg_el1(vcpu_sve_zcr_el1(vcpu), SYS_ZCR);
+	write_sysreg_el1(__vcpu_sys_reg(vcpu, vcpu_sve_zcr_elx(vcpu)), SYS_ZCR);
 }
 
 /*

which makes the helper select the correct guest register for the
context, and only that. In turn, the write side is much cleaner and
symmetry is restored.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.