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

On Mon, 11 Jul 2022 12:39:51 +0100,
Mark Brown [off-list ref] wrote:

[1  <text/plain; us-ascii (7bit)>]
On Mon, Jul 11, 2022 at 10:40:50AM +0100, Marc Zyngier wrote:

quoted

Mark Brown [off-list ref] wrote:

quoted

quoted

+	enum fp_state *type;

quoted

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.

quoted

quoted

 	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;

quoted

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.

quoted

quoted

 	} else {
 		fpsimd_to_sve(current);
+		current->thread.fp_type = FP_STATE_SVE;

quoted

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