Re: [PATCH v3 05/11] KVM: arm64: Eagerly restore host fpsimd/sve state in pKVM

[Marc Zyngier <maz@kernel.org>]

On Mon, 03 Jun 2024 14:27:07 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> On Mon, Jun 03, 2024 at 09:37:16AM +0100, Fuad Tabba wrote:
> > On Fri, May 31, 2024 at 3:09 PM Mark Brown <broonie@kernel.org> wrote:
> > > On Tue, May 28, 2024 at 01:59:08PM +0100, Fuad Tabba wrote:
> 
> > > > +     write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL2);
> 
> > > As well as the sync issue Oliver mentioned on the removal of
> > > _cond_update() just doing these updates as a blind write creates a
> > > surprise if we ever get more control bits in ZCR_EL2.
> 
> > I'm not sure it does. The other bits are RES0, and this is always
> > setting the length. So even if new control bits are added, this
> > shouldn't matter.
> 
> The surprise would be that if new control bits were added this would
> result in clearing them on restore.

And as far as I can tell, there is no in-flight architectural change
that touches this class of registers. And should this eventually
happens, we will have to audit *all* the spots when ZCR_ELx is touched
and turn them all into RMW accesses. Having an extra spot here isn't
going to change this in a material way.

> 
> > Also, one of the concerns in terms of performance is now with
> > nested-virt support being added, and the overhead of doing the
> > conditional update when we know that it's unlikely that anyone is
> > implementing vectors as big as the max.
> 
> I guess there's the option of doing a restore of a value fixed during
> initialisation instead?

And what do we gain from that?

> 
> > > > +     /*
> > > > +      * On saving/restoring host sve state, always use the maximum VL for
> > > > +      * the host. The layout of the data when saving the sve state depends
> > > > +      * on the VL, so use a consistent (i.e., the maximum) host VL.
> > > > +      *
> > > > +      * Setting ZCR_EL2 to ZCR_ELx_LEN_MASK sets the effective length
> > > > +      * supported by the system (or limited at EL3).
> > > > +      */
> > > > +     write_sysreg_s(ZCR_ELx_LEN_MASK, SYS_ZCR_EL2);
> 
> > > Setting ZCR_ELx_LEN_MASK sets the VL to the maximum supported/configured
> > > for the current PE, not the system.  This will hopefully be the same
> > > since we really hope implementors continue to build symmetric systems
> > > but there is handling for that case in the kernel just in case.  Given
> > > that we record the host's maximum VL should we use it?
> 
> > You're right, but even if the current PE had a different vector
> > length, ZCR_ELx_LEN_MASK is the default value for ZCR_EL2 when the
> > host is running (this is the existing behavior before this patch
> > series). It is also the value this patch series uses when saving the
> > host SVE state. So since we are consistent I think this is correct.
> 
> The reason we just set all bits in ZCR_EL2.LEN is that we don't
> currently use SVE at EL2 so we're just passing everything through to EL1
> and letting it worry about things.  As we start adding more SVE code at
> EL2 we need to care more and I think we should start explicitly
> programming what we think we're using to use to avoid surprises.  For
> example in this series we allocate the buffer used to store the host SVE
> state based on the probed maximum usable VL for the system but here we
> use whatever the PE has as the maximum VL.  This means that in the
> (hopefully unlikely) case where the probed value is lower than the PE
> value we'll overflow the buffer.

In that case, we need the *real* maximum across all CPUs, not the
maximum usable.

	M.

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