Re: [PATCH v2 5/8] KVM: arm64: Refactor CPTR trap deactivation

[Will Deacon <will@kernel.org>]

On Thu, Feb 06, 2025 at 02:10:59PM +0000, Mark Rutland wrote:
> For historical reasons, the VHE and nVHE/hVHE implementations of
> __activate_cptr_traps() pair with a common implementation of
> __kvm_reset_cptr_el2(), which ideally would be named
> __deactivate_cptr_traps().
> 
> Rename __kvm_reset_cptr_el2() to __deactivate_cptr_traps(), and split it
> into separate VHE and nVHE/hVHE variants so that each can be paired with
> its corresponding implementation of __activate_cptr_traps().
> 
> At the same time, fold kvm_write_cptr_el2() into its callers. This
> makes it clear in-context whether a write is made to the CPACR_EL1
> encoding or the CPTR_EL2 encoding, and removes the possibility of
> confusion as to whether kvm_write_cptr_el2() reformats the sysreg fields
> as cpacr_clear_set() does.
> 
> In the nVHE/hVHE implementation of __activate_cptr_traps(), placing the
> sysreg writes within the if-else blocks requires that the call to
> __activate_traps_fpsimd32() is moved earlier, but as this was always
> called before writing to CPTR_EL2/CPACR_EL1, this should not result in a
> functional change.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Fuad Tabba <tabba@google.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Oliver Upton <oliver.upton@linux.dev>
> Cc: Will Deacon <will@kernel.org>

[...]

> diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
> index 7a2d189176249..5d79f63a4f861 100644
> --- a/arch/arm64/kvm/hyp/nvhe/switch.c
> +++ b/arch/arm64/kvm/hyp/nvhe/switch.c
> @@ -39,6 +39,9 @@ static void __activate_cptr_traps(struct kvm_vcpu *vcpu)
>  {
>  	u64 val = CPTR_EL2_TAM;	/* Same bit irrespective of E2H */
>  
> +	if (!guest_owns_fp_regs())
> +		__activate_traps_fpsimd32(vcpu);
> +
>  	if (has_hvhe()) {
>  		val |= CPACR_EL1_TTA;
>  
> @@ -47,6 +50,8 @@ static void __activate_cptr_traps(struct kvm_vcpu *vcpu)
>  			if (vcpu_has_sve(vcpu))
>  				val |= CPACR_EL1_ZEN;
>  		}
> +
> +		write_sysreg(val, cpacr_el1);
>  	} else {
>  		val |= CPTR_EL2_TTA | CPTR_NVHE_EL2_RES1;
>  
> @@ -61,12 +66,34 @@ static void __activate_cptr_traps(struct kvm_vcpu *vcpu)
>  
>  		if (!guest_owns_fp_regs())
>  			val |= CPTR_EL2_TFP;
> +
> +		write_sysreg(val, cptr_el2);
>  	}
> +}
>  
> -	if (!guest_owns_fp_regs())
> -		__activate_traps_fpsimd32(vcpu);
> +static void __deactivate_cptr_traps(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm *kvm = kern_hyp_va(vcpu->kvm);

nit: You could lose the local if you used vcpu_has_sve(vcpu) instead.

However, given that this gets removed _anyway_ when we eagerly switch
ZCR later on:

Acked-by: Will Deacon <will@kernel.org>

Will