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] [day] [month] [year] [list]

Message-ID: <ce2b8cee-7ce0-4675-8b74-f455a206d7c8@sirena.org.uk>
Date: Tue, 27 Feb 2024 12:19:17 +0000
From: Mark Brown <broonie@...nel.org>
To: Oliver Upton <oliver.upton@...ux.dev>
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

On Mon, Feb 26, 2024 at 11:07:55PM -0800, Oliver Upton wrote:
> On Mon, Feb 26, 2024 at 08:44:11PM +0000, Mark Brown wrote:

> >       vcpu->arch.fp_owner = FP_STATE_FREE;

..

> > +	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.

It's just reflecting the existing structure of the code, we already
split the initialisation between _create() and the SVE setup so this is
just following along from that.

I'm also happier with doing whatever initialisation we can prior to
userspace getting involved, it wouldn't be an issue currently but this
state is visible to userspace and it feels error prone to have it
partially initialised when we aren't compelled to like we are with the
configurability of the vector lengths.  Someone could too easily not
notice that there's a window where the state is not yet initialised but
userspace can try to access it when adding to the initisation function
during future work.

> > -		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...

Indeed, that was actually fixed locally - looks like that didn't make it
into what got sent out somehow.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)