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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3060 bytes --]

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.

> > +	/*
> > +	 * 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.

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.

> >  void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
> >  {
> > +	enum fp_state fp_type;
> > +
> >  	WARN_ON_ONCE(!irqs_disabled());
> >  
> >  	if (vcpu->arch.fp_state == FP_STATE_GUEST_OWNED) {
> > +		if (vcpu_has_sve(vcpu))
> > +			fp_type = FP_STATE_SVE;

> Eventually, I'd like to relax this, and start tracking the actual use
> of the guest rather than assuming that SVE guest use SVE at all times
> (odds are they won't).

> I hope this series still leaves us with this option.

Yes, it probably makes it more tractable with KVM being able to
just say what type of state it wants to save so there's less to
take care of syncing with the host task so the code is a lot more
direct - it will just be a case of setting the desired fp_type
whenever a decision is made about what state type to save.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 151 bytes --]

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm