Re: [PATCH v4 6/9] KVM: arm64: Eagerly restore host fpsimd/sve state in pKVM

[Marc Zyngier <maz@kernel.org>]

On Tue, 04 Jun 2024 14:13:16 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> [1  <text/plain; utf-8 (quoted-printable)>]
> On Tue, Jun 04, 2024 at 01:03:07PM +0100, Fuad Tabba wrote:
> > On Mon, Jun 3, 2024 at 4:52 PM Mark Brown <broonie@kernel.org> wrote:
> > > On Mon, Jun 03, 2024 at 01:28:48PM +0100, Fuad Tabba wrote:
> 
> > > > +static void fpsimd_sve_flush(void)
> > > > +{
> > > > +     *host_data_ptr(fp_owner) = FP_STATE_HOST_OWNED;
> > > > +}
> 
> > > My previous comments about this being confusing still stand.
> 
> > Sorry, I missed this in my reply to v3.
> 
> > This follows the convention for save/flush in hyp-main.c. Since the
> > act of flushing the fpsimd/sve state is lazy, i.e., only takes place
> > if the guest were to use fpsimd/sve, then the only thing that we need
> > to do to flush is to mark the state as owned by the host.
> 
> I think this needs a comment mentioning what's going on here.
> 
> > You suggested inlining this, but since this is static, I think the
> > compiler would do that. Even though it's only one line, it maintains
> > symmetry with fpsimd_sve_sync().
> 
> The reason I was suggesting inlining was that it removes the need to
> name the function.

You're missing the point.

The name is important, and the current name is correct. We use *flush
(resp. *sync) for operations that need to happen before (resp. after)
the entry into the guest. This is consistent with the rest of the code
base for most of the subsystems KVM/arm64 deals with.

So the current form of this function, as a standalone function with
its current name, stays.

	M.

-- 
Without deviation from the norm, progress is not possible.