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

[Marc Zyngier <maz@kernel.org>]

On Mon, 11 Jul 2022 16:53:43 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> 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?".

My experience in maintaining the KVM code is that the least state
leaks outside of this sort of helpers, the least problematic they
are. I'd rather have multiple helpers that have different *documented*
behaviours than expecting the random hacker to know (or in this case,
*guess*) when or not to add some extra state-twiddling. It also makes
the code far more readable because it is self-contained.

If this series is supposed to help making things more maintainable,
then this is one way to do it.

	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