Re: [PATCH 2/2] KVM: arm64: Reuse struct cpu_fp_state to track the guest FP state

From: Oliver Upton <oliver.upton@linux.dev>
To: Mark Brown <broonie@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>, James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] KVM: arm64: Reuse struct cpu_fp_state to track the guest FP state
Date: Mon, 26 Feb 2024 23:07:55 -0800	[thread overview]
Message-ID: <Zd2KS3sVI-vPxurg@linux.dev> (raw)
In-Reply-To: <20240226-kvm-arm64-group-fp-data-v1-2-07d13759517e@kernel.org>

Hey broonie,

On Mon, Feb 26, 2024 at 08:44:11PM +0000, Mark Brown wrote:
> At present we store the various bits of floating point state individually
> in struct kvm_vpcu_arch and construct a struct cpu_fp_state to share with

typo: kvm_vcpu_arch

> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index a2cba18effb2..84cc0dbd9b14 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -379,6 +379,18 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  	 */
>  	vcpu->arch.fp_owner = FP_STATE_FREE;
>  
> +	/*
> +	 * Initial setup for FP state for sharing with host, if SVE is
> +	 * enabled additional configuration will be done.
> +	 *
> +	 * Currently we do not support SME guests so SVCR is always 0
> +	 * and we just need a variable to point to.
> +	 */
> +	vcpu->arch.fp_state.st = &vcpu->arch.ctxt.fp_regs;
> +	vcpu->arch.fp_state.fp_type = &vcpu->arch.fp_type;
> +	vcpu->arch.fp_state.svcr = &vcpu->arch.svcr;
> +	vcpu->arch.fp_state.to_save = FP_STATE_FPSIMD;
> +

I'm not too big of a fan of scattering the initialization in various
places... Why can't we have a unified helper for priming cpu_fp_state once
we know what we're dealing with?

That can be called from either kvm_setup_vcpu() or kvm_vcpu_finalize_sve()
depending on whether userspace signed up for SVE or not.

>  	/* Set up the timer */
>  	kvm_timer_vcpu_init(vcpu);
>  
> diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> index 8dbd62d1e677..45fe4a942992 100644
> --- a/arch/arm64/kvm/fpsimd.c
> +++ b/arch/arm64/kvm/fpsimd.c
> @@ -143,24 +143,7 @@ void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
>  	WARN_ON_ONCE(!irqs_disabled());
>  
>  	if (vcpu->arch.fp_owner == FP_STATE_GUEST_OWNED) {
> -
> -		/*
> -		 * Currently we do not support SME guests so SVCR is
> -		 * always 0 and we just need a variable to point to.
> -		 */
> -		fp_state.st = &vcpu->arch.ctxt.fp_regs;
> -		fp_state.sve_state = vcpu->arch.sve_state;
> -		fp_state.sve_vl = vcpu->arch.sve_max_vl;
> -		fp_state.sme_state = NULL;
> -		fp_state.svcr = &vcpu->arch.svcr;
> -		fp_state.fp_type = &vcpu->arch.fp_type;
> -
> -		if (vcpu_has_sve(vcpu))
> -			fp_state.to_save = FP_STATE_SVE;
> -		else
> -			fp_state.to_save = FP_STATE_FPSIMD;
> -
> -		fpsimd_bind_state_to_cpu(&fp_state);
> +		fpsimd_bind_state_to_cpu(&vcpu->arch.fp_state);

Shouldn't we get rid of the fp_state local at this point? I'm pretty
sure a compiler would emit a warning here...

-- 
Thanks,
Oliver