[PATCH v3 05/11] KVM: arm64: Eagerly restore host fpsimd/sve state in pKVM

[Mark Brown]

On Tue, May 28, 2024 at 01:59:08PM +0100, Fuad Tabba wrote:

> +static inline void __hyp_sve_save_host(void)
> +{
> +	struct cpu_sve_state *sve_state = *host_data_ptr(sve_state);
> +
> +	sve_state->zcr_el1 = read_sysreg_el1(SYS_ZCR);
> +	write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL2);

As well as the sync issue Oliver mentioned on the removal of
_cond_update() just doing these updates as a blind write creates a
surprise if we ever get more control bits in ZCR_EL2.

> +static void __hyp_sve_restore_host(void)
> +{
> +	struct cpu_sve_state *sve_state = *host_data_ptr(sve_state);
> +
> +	/*
> +	 * On saving/restoring host sve state, always use the maximum VL for
> +	 * the host. The layout of the data when saving the sve state depends
> +	 * on the VL, so use a consistent (i.e., the maximum) host VL.
> +	 *
> +	 * Setting ZCR_EL2 to ZCR_ELx_LEN_MASK sets the effective length
> +	 * supported by the system (or limited at EL3).
> +	 */
> +	write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL2);

Setting ZCR_ELx_LEN_MASK sets the VL to the maximum supported/configured
for the current PE, not the system.  This will hopefully be the same
since we really hope implementors continue to build symmetric systems
but there is handling for that case in the kernel just in case.  Given
that we record the host's maximum VL should we use it?

> +static void fpsimd_sve_flush(void)
> +{
> +	*host_data_ptr(fp_owner) = FP_STATE_HOST_OWNED;
> +}
> +

That's not what I'd have expected something called fpsimd_sve_flush() to
do, I'd have expected it to save the current state to memory and then
mark it as free (that's what fpsimd_flush_cpu_state() does in the host
kernel).  Perhaps just inline this into the one user?

> +static void fpsimd_sve_sync(struct kvm_vcpu *vcpu)
> +{
> +	if (!guest_owns_fp_regs())
> +		return;
> +
> +	cpacr_clear_set(0, CPACR_ELx_FPEN|CPACR_ELx_ZEN);
> +	isb();

Spaces around |.

>  static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
>  {
>  	struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu;
>  
> +	fpsimd_sve_flush();
> +
>  	hyp_vcpu->vcpu.arch.ctxt	= host_vcpu->arch.ctxt;
>  
>  	hyp_vcpu->vcpu.arch.sve_state	= kern_hyp_va(host_vcpu->arch.sve_state);
> -	hyp_vcpu->vcpu.arch.sve_max_vl	= host_vcpu->arch.sve_max_vl;
> +	hyp_vcpu->vcpu.arch.sve_max_vl	= min(host_vcpu->arch.sve_max_vl, kvm_host_sve_max_vl);

This needs a comment I think.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20240531/86f8d6bb/attachment.sig>