Re: [PATCH v3 3/7] arm64/fpsimd: Have KVM explicitly say which FP registers to save

[Marc Zyngier <maz@kernel.org>]

On Tue, 20 Sep 2022 19:32:49 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> [1  <text/plain; us-ascii (quoted-printable)>]
> On Tue, Sep 20, 2022 at 06:52:59PM +0100, Marc Zyngier wrote:
> > On Mon, 15 Aug 2022 23:55:25 +0100,
> > Mark Brown <broonie@kernel.org> wrote:
> 
> > >  enum fp_state {
> > > +	FP_STATE_TASK,		/* Save based on current, invalid as fp_type */
> 
> > How is that related to the FP_TYPE_TASK in the commit message? What
> 
> TYPE in the commit message should be STATE.
> 
> > does this 'invalid as fp_type' mean?
> 
> It means that using FP_STATE_TASK as a value for the fp_type
> member of the task struck recording what type of state is
> currently stored for the task is not valid, one of the other two
> values representing what was actually saved must be chosen.

Then this definitely represents something else, and shouldn't be a
state or a type, whatever you decide to call it in the end. There is
the state of the FP/SVE unit, and what some piece of SW wants to
save. They match in some cases, and differ in other (the TASK
value). I'd rather you encode them as them as different types to lift
the ambiguity.

> 
> > > +	/*
> > > +	 * For now we're just validating that the requested state is
> > > +	 * consistent with what we'd otherwise work out.
> 
> > Nit: work out? or worked out? the "we'd" doesn't help disambiguate it
> > for a non-native speaker.
> 
> we'd == we would so work out to match the tense.
> 
> > >  void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st, void *sve_state,
> > >  			      unsigned int sve_vl, void *za_state,
> > >  			      unsigned int sme_vl, u64 *svcr,
> > > -			      enum fp_state *type)
> > > +			      enum fp_state *type, enum fp_state to_save)
> 
> > OK, how many discrete arguments are we going to pass to this function,
> > which most of them are part the vcpu structure? It really feels like
> > what you want is a getter for the per-cpu structure, and let the KVM
> > code do the actual business. If this function was supposed to provide
> > some level of abstraction, well, it's a fail.
> 
> I agree that this is not an ideal interface, I am merely
> following the previously chosen idiom since I haven't been able
> to figure out why we were doing it in the first place and with a
> lot of these things it turns out that there's some actual reason.

Huh. If we're changing anything around this code, we'd better
understand what we are doing...

> It's not even like fpsimd_bind_task_to_cpu() has ever been
> written in terms of this function, there's two parallel
> implementations.  My best guess was that it was some combination
> of not peering at KVM internals and keeping struct
> fpsimd_last_state_struct internal to fpsimd.c (since we're
> effectively just passing one of those in in a more verbose form)
> but never anything solid enough to be sure.

Up to you, but adding extra parameters to this function really feels
like the wrong thing to do.

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm