On Mon, 11 Jul 2022 12:39:51 +0100,
Mark Brown <broonie@xxxxxxxxxx> wrote:
>
> [1 <text/plain; us-ascii (7bit)>]
> On Mon, Jul 11, 2022 at 10:40:50AM +0100, Marc Zyngier wrote:
> > Mark Brown <broonie@xxxxxxxxxx> 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.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
