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

[Mark Brown <broonie@kernel.org>]

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

On Mon, Jul 11, 2022 at 03:33:51PM +0100, Marc Zyngier wrote:
> Mark Brown <broonie@kernel.org> wrote:
> > 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

Ah, sure I can do that.  I had thought this being in the FP last state
structure made things clear here.

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

My experience trying to follow and update this code has been that
layering on more helpers just shifts the potential for bugs around -
it's easy to have the calling context using the wrong helper and looking
correct, or to spend time cross checking if the helper in a particular
context is the right one.  Sometimes this happens because something
about the calling context changed rather than due to writing a new use.
Yes, someone might forget to update the state type but my experience
with this code is that it's a lot easier to spot "this is writing new
state, did it update the state type?" than "this is writing new state,
did it call the helper that implicitly updates the state type or the
other one?".

Since these callers are already explicitly peering into the data in one
form or another (like reading or writing the actual register values, and
including for some checking the type information) it seems reasonable
for them to also be doing updates to the type selection explicitly.  It
does also make the error handling a little neater, if we are switching
between state types then in the case of error we just leave things using
the old, unmodified state.

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