Re: [PATCH v2 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 v2 2/7] arm64/fpsimd: Track the saved FPSIMD state type separately to TIF_SVE
Date: Mon, 11 Jul 2022 15:33:51 +0100	[thread overview]
Message-ID: <87h73nlnw0.wl-maz@kernel.org> (raw)
In-Reply-To: <YswMB2v9TqPJHDM1@sirena.org.uk>

On Mon, 11 Jul 2022 12:39:51 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> [1  <text/plain; us-ascii (7bit)>]
> On Mon, Jul 11, 2022 at 10:40:50AM +0100, Marc Zyngier wrote:
> > Mark Brown <broonie@kernel.org> wrote:
> 
> > > +	enum fp_state *type;
> 
> > For consistency: s/type/fp_type/ ?
> 
> Sure if nobody else wants a different bikeshed.  It really needs a
> longer name like fp_state_t or something but that had it's own problems
> with non-idiomaticness.

I'm not talking about the name of the type, but about the name of the
member in the struct fpsimd_last_state_struct. I'd like it to be
homogeneous to the name you use in struct kvm_vcpu_arch. 'type' is way
too vague a name, and maybe it should be fp_reg_type, as that's what
it actually indicates.

> 
> > >  	if (test_and_clear_tsk_thread_flag(task, TIF_SVE) ||
> > > -	    thread_sm_enabled(&task->thread))
> > > +	    thread_sm_enabled(&task->thread)) {
> > >  		sve_to_fpsimd(task);
> > > +		task->thread.fp_type = FP_STATE_FPSIMD;
> 
> > Can you move this assignment into the sve_to_fpsimd() helper?
> 
> There are cases where we want a FPSIMD version of the state for
> reading but don't want to affect the actual state of the process
> (eg, if someone reads the FPSIMD registers via ptrace) so we don't
> want to change the active register state just because we converted
> it.  Adding another API that does the convert and update didn't feel
> like it was helping since you then have to remember which API does
> what and we already have lots of similarly named functions for
> slightly different contexts.

I still think the state conversion should be self contained.
Sprinkling this context tracking is bound to end-up with a bug, while
documenting what is to be used when, or with a helper named
explicitly enough ("extract_fp_from_sve()" springs to mind) for
ptrace.

> > >  	} else {
> > >  		fpsimd_to_sve(current);
> > > +		current->thread.fp_type = FP_STATE_SVE;
> 
> > Same thing here.
> 
> There's not the same issue with reading FPSIMD state via the SVE APIs
> but for consistency it seems best to always leave these updates in the
> callers.

I disagree again. I really want these things to be self-contained, and
be able to reason about what they do from their name and the arguments
they take (and even some documentation). Relying on some extra updates
adds complexity to a difficult part of the kernel, and an even more
difficult part of the architecture.

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