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

[Fuad Tabba]

Hi Mark,

On Fri, May 31, 2024 at 3:09 PM Mark Brown <broonie at kernel.org> wrote:
>
> On Tue, May 28, 2024 at 01:59:08PM +0100, Fuad Tabba wrote:
>
> > +static inline void __hyp_sve_save_host(void)
> > +{
> > +     struct cpu_sve_state *sve_state = *host_data_ptr(sve_state);
> > +
> > +     sve_state->zcr_el1 = read_sysreg_el1(SYS_ZCR);
> > +     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.

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.

>
> > +static void __hyp_sve_restore_host(void)
> > +{
> > +     struct cpu_sve_state *sve_state = *host_data_ptr(sve_state);
> > +
> > +     /*
> > +      * 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.

> > +static void fpsimd_sve_flush(void)
> > +{
> > +     *host_data_ptr(fp_owner) = FP_STATE_HOST_OWNED;
> > +}
> > +
>
> That's not what I'd have expected something called fpsimd_sve_flush() to
> do, I'd have expected it to save the current state to memory and then
> mark it as free (that's what fpsimd_flush_cpu_state() does in the host
> kernel).  Perhaps just inline this into the one user?
>
> > +static void fpsimd_sve_sync(struct kvm_vcpu *vcpu)
> > +{
> > +     if (!guest_owns_fp_regs())
> > +             return;
> > +
> > +     cpacr_clear_set(0, CPACR_ELx_FPEN|CPACR_ELx_ZEN);
> > +     isb();
>
> Spaces around |.

Will do.

> >  static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
> >  {
> >       struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu;
> >
> > +     fpsimd_sve_flush();
> > +
> >       hyp_vcpu->vcpu.arch.ctxt        = host_vcpu->arch.ctxt;
> >
> >       hyp_vcpu->vcpu.arch.sve_state   = kern_hyp_va(host_vcpu->arch.sve_state);
> > -     hyp_vcpu->vcpu.arch.sve_max_vl  = host_vcpu->arch.sve_max_vl;
> > +     hyp_vcpu->vcpu.arch.sve_max_vl  = min(host_vcpu->arch.sve_max_vl, kvm_host_sve_max_vl);
>
> This needs a comment I think.

Will do.

Thanks!
/fuad