Re: [PATCH v3 2/7] arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE

From: Marc Zyngier <maz@kernel.org>
To: Mark Brown <broonie@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Zhang Lei <zhang.lei@jp.fujitsu.com>,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Andre Przywara <andre.przywara@arm.com>,
	kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 2/7] arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE
Date: Tue, 20 Sep 2022 19:30:18 +0100	[thread overview]
Message-ID: <87sfklkif9.wl-maz@kernel.org> (raw)
In-Reply-To: <YyoBy+HuHj5XRXKG@sirena.org.uk>

On Tue, 20 Sep 2022 19:09:15 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> [1  <text/plain; us-ascii (7bit)>]
> On Tue, Sep 20, 2022 at 06:14:13PM +0100, Marc Zyngier wrote:
> > Mark Brown <broonie@kernel.org> wrote:
> 
> > > When we save the state for the floating point registers this can be done
> > > in the form visible through either the FPSIMD V registers or the SVE Z and
> > > P registers. At present we track which format is currently used based on
> > > TIF_SVE and the SME streaming mode state but particularly in the SVE case
> > > this limits our options for optimising things, especially around syscalls.
> > > Introduce a new enum in thread_struct which explicitly states which format
> > > is active and keep it up to date when we change it.
> 
> > > At present we do not use this state except to verify that it has the
> > > expected value when loading the state, future patches will introduce
> > > functional changes.
> 
> > > +	enum fp_state fp_type;
> 
> > Is it a state or a type? Some consistency would help. Also, what does
> 
> We can bikeshed this either way - the state currently stored is
> of a particular type.  I'll probably go for type.

Then please do it consistently. At the moment, this is a bizarre mix
of the two, and this is already hard enough to reason about this that
we don't need extra complexity!

> 
> > this represent? Your commit message keeps talking about the FP/SVE
> > state for the host, but this is obviously a guest-related structure.
> > How do the two relate?
> 
> The commit message talks about saving the floating point state in
> general which is something we do for both the host and the guest.
> The optimisation cases I am focusing on right now are more on
> host usage but the complexity with tracking that currently blocks
> them crosses both host and guest, indeed the biggest improvement
> overall is probably that tracking the guest state stops requiring
> us to fiddle with the host task's state which to me at least
> makes things clearer.

At least for the KVM part, I want a clear comment explaining what this
tracks and how this is used, because at the moment, I'm only guessing.
And I've had enough guessing with this code...

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel