Re: [PATCH v2 8/8] KVM: arm64: Eagerly switch ZCR_EL{1,2}

[Mark Rutland <mark.rutland@arm.com>]

On Mon, Feb 10, 2025 at 06:20:09PM +0000, Will Deacon wrote:
> On Mon, Feb 10, 2025 at 05:21:59PM +0000, Mark Rutland wrote:
> > 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:
> > > > +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.
> 
> Ah yes, it really all hinges on guest_owns_fp_regs() and so I think a
> comment would be helpful, thanks.
> 
> Just on this, though:
> 
> > 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.
> 
> What about the case where we find that there's an interrupt pending on
> return to the guest? In that case, I think we elide the ERET and go back
> to the host (see the check of isr_el1 in hyp/entry.S).

Ah; I had missed that, and evidently I had not looked at the entry code.

Given that, I think the options are:

(a) Add an ISB after disabling the traps, before returning to the guest.

(b) Add an ISB in fpsimd_lazy_switch_to_host() above.

(c) Add an ISB in that sequence in hyp/entry.S, just before the ret, to
    ensure that __guest_enter() always provides a context
    synchronization event even when it doesn't enter the guest,
    regardless of ARM64_HAS_RAS_EXTN.

I think (c) is probably the nicest, since that avoids the need for
redundant barriers in the common case, and those short-circuited exits
are hopefully rare.

Obviously that would mean adding comments in both __guest_enter() and
fpsimd_lazy_switch_to_host().

Mark.