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

From: Mark Brown <broonie@kernel.org>
To: Marc Zyngier <maz@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Zhang Lei <zhang.lei@jp.fujitsu.com>,
	Andre Przywara <andre.przywara@arm.com>,
	Will Deacon <will@kernel.org>,
	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 12:39:51 +0100	[thread overview]
Message-ID: <YswMB2v9TqPJHDM1@sirena.org.uk> (raw)
In-Reply-To: <87ilo4kmvx.wl-maz@kernel.org>

[-- Attachment #1.1: Type: text/plain, Size: 1925 bytes --]

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.

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

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

> > --- a/arch/arm64/kernel/ptrace.c
> > +++ b/arch/arm64/kernel/ptrace.c
> > @@ -892,8 +892,7 @@ static int sve_set_common(struct task_struct *target,
> >  		ret = __fpr_set(target, regset, pos, count, kbuf, ubuf,
> >  				SVE_PT_FPSIMD_OFFSET);
> >  		clear_tsk_thread_flag(target, TIF_SVE);
> > -		if (type == ARM64_VEC_SME)
> > -			fpsimd_force_sync_to_sve(target);

> I don't get this particular change. Can you please clarify?

That should probably be shifted to a later patch in the series, I think
I just rebased it to the wrong place.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 151 bytes --]

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm