On Mon, Feb 10, 2025 at 04:53:27PM +0000, Will Deacon wrote:
> On Thu, Feb 06, 2025 at 02:11:02PM +0000, Mark Rutland wrote:
> > Fix this and make this a bit easier to reason about by always eagerly
> > switching ZCR_EL{1,2} at hyp during guest<->host transitions. With this
> > happening, there's no need to trap host SVE usage, and the nVHE/nVHVE
>
> nit: nVHVE?
>
> (also, note to Fuad: I think we're trapping FPSIMD/SVE from the host with
> pKVM in Android, so we'll want to fix that when we take this patch via
> -stable)
>
> > __deactivate_cptr_traps() logic can be simplified enable host access to
>
> nit: to enable
>
> > all present FPSIMD/SVE/SME features.
> >
> > In protected nVHE/hVHVE modes, the host's state is always saved/restored
>
> nit: hVHVE
>
> (something tells me these acronyms aren't particularly friendly!)
Aargh, sorry about those -- I've fixed those up and I'll give the series
another once-over.
[...]
> > +static inline void fpsimd_lazy_switch_to_guest(struct kvm_vcpu *vcpu)
> > +{
> > + u64 zcr_el1, zcr_el2;
> > +
> > + if (!guest_owns_fp_regs())
> > + return;
> > +
> > + if (vcpu_has_sve(vcpu)) {
> > + /* A guest hypervisor may restrict the effective max VL. */
> > + if (vcpu_has_nv(vcpu) && !is_hyp_ctxt(vcpu))
> > + zcr_el2 = __vcpu_sys_reg(vcpu, ZCR_EL2);
> > + else
> > + zcr_el2 = vcpu_sve_max_vq(vcpu) - 1;
> > +
> > + write_sysreg_el2(zcr_el2, SYS_ZCR);
> > +
> > + zcr_el1 = __vcpu_sys_reg(vcpu, vcpu_sve_zcr_elx(vcpu));
> > + write_sysreg_el1(zcr_el1, SYS_ZCR);
> > + }
> > +}
> > +
> > +static inline void fpsimd_lazy_switch_to_host(struct kvm_vcpu *vcpu)
> > +{
> > + u64 zcr_el1, zcr_el2;
> > +
> > + if (!guest_owns_fp_regs())
> > + return;
> > +
> > + if (vcpu_has_sve(vcpu)) {
> > + zcr_el1 = read_sysreg_el1(SYS_ZCR);
> > + __vcpu_sys_reg(vcpu, vcpu_sve_zcr_elx(vcpu)) = zcr_el1;
> > +
> > + /*
> > + * The guest's state is always saved using the guest's max VL.
> > + * Ensure that the host has the guest's max VL active such that
> > + * the host can save the guest's state lazily, but don't
> > + * artificially restrict the host to the guest's max VL.
> > + */
> > + if (has_vhe()) {
> > + zcr_el2 = vcpu_sve_max_vq(vcpu) - 1;
> > + write_sysreg_el2(zcr_el2, SYS_ZCR);
> > + } else {
> > + zcr_el2 = sve_vq_from_vl(kvm_host_sve_max_vl) - 1;
> > + write_sysreg_el2(zcr_el2, SYS_ZCR);
> > +
> > + zcr_el1 = vcpu_sve_max_vq(vcpu) - 1;
> > + write_sysreg_el1(zcr_el1, SYS_ZCR);
>
> Do we need an ISB before this to make sure that the CPTR traps have been
> deactivated properly?
Sorry, I had meant to add a comment here that this relies upon a
subtlety that avoids the need for the ISB.
When the guest owns the FP regs here, we know:
* If the guest doesn't have SVE, then we're not poking anything, and so
no ISB is necessary.
* If the guest has SVE, then either:
- The guest owned the FP regs when it was entered.
- The guest *didn't* own the FP regs when it was entered, but acquired
ownership via a trap which executed kvm_hyp_handle_fpsimd().
... and in either case, *after* disabling the traps there's been an
ERET to the guest and an exception back to hyp, either of which
provides the necessary context synchronization such that the traps are
disabled here.
Does that make sense to you?
Mark.
