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

On Mon, Jul 11, 2022 at 03:33:51PM +0100, Marc Zyngier wrote:

Mark Brown [off-list ref] wrote:

quoted

On Mon, Jul 11, 2022 at 10:40:50AM +0100, Marc Zyngier wrote:

quoted

Mark Brown [off-list ref] wrote:

quoted

quoted

quoted

+	enum fp_state *type;

quoted

quoted

For consistency: s/type/fp_type/ ?

quoted

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.

quoted

quoted

quoted

-	    thread_sm_enabled(&task->thread))
+	    thread_sm_enabled(&task->thread)) {
 		sve_to_fpsimd(task);
+		task->thread.fp_type = FP_STATE_FPSIMD;

quoted

quoted

Can you move this assignment into the sve_to_fpsimd() helper?

quoted

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.