linux-kernel - Re: [PATCH 2/2] KVM: arm64: Reuse struct cpu_fp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zd2KS3sVI-vPxurg@linux.dev>
Date: Mon, 26 Feb 2024 23:07:55 -0800
From: Oliver Upton <oliver.upton@...ux.dev>
To: Mark Brown <broonie@...nel.org>
Cc: Marc Zyngier <maz@...nel.org>, James Morse <james.morse@....com>,
	Suzuki K Poulose <suzuki.poulose@....com>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, linux-arm-kernel@...ts.infradead.org,
	kvmarm@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] KVM: arm64: Reuse struct cpu_fp_state to track the
 guest FP state

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