Re: [PATCH RFC v3 09/27] KVM: arm64: Factor SVE guest exit handling out into a function

[Marc Zyngier <maz@kernel.org>]

On Fri, 17 Jan 2025 11:34:09 +0000,
Mark Rutland <mark.rutland@arm.com> wrote:
> 
> On Fri, Dec 20, 2024 at 04:46:34PM +0000, Mark Brown wrote:
> > The SVE portion of kvm_vcpu_put() is quite large, especially given the
> > comments required.  When we add similar handling for SME the function
> > will get even larger, in order to keep things managable factor the SVE
> > portion out of the main kvm_vcpu_put().
> 
> While investigating some problems with SVE I spotted a latent bug in
> this area where I suspect the fix will conflict with / supersede this
> rework. Details below; IIUC the bug was introduced in commit:
> 
>   8c8010d69c132273 ("KVM: arm64: Save/restore SVE state for nVHE")
> 
> > Signed-off-by: Mark Brown <broonie@kernel.org>
> > ---
> >  arch/arm64/kvm/fpsimd.c | 67 +++++++++++++++++++++++++++----------------------
> >  1 file changed, 37 insertions(+), 30 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> > index 09b65abaf9db60cc57dbc554ad2108a80c2dc46b..3c2e0b96877ac5b4f3b9d8dfa38975f11b74b60d 100644
> > --- a/arch/arm64/kvm/fpsimd.c
> > +++ b/arch/arm64/kvm/fpsimd.c
> > @@ -151,6 +151,41 @@ void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
> >  	}
> >  }
> >  
> > +static void kvm_vcpu_put_sve(struct kvm_vcpu *vcpu)
> > +{
> > +	u64 zcr;
> > +
> > +	if (!vcpu_has_sve(vcpu))
> > +		return;
> > +
> > +	zcr = read_sysreg_el1(SYS_ZCR);
> > +
> > +	/*
> > +	 * If the vCPU is in the hyp context then ZCR_EL1 is loaded
> > +	 * with its vEL2 counterpart.
> > +	 */
> > +	__vcpu_sys_reg(vcpu, vcpu_sve_zcr_elx(vcpu)) = zcr;
> > +
> > +	/*
> > +	 * Restore the VL that was saved when bound to the CPU, which
> > +	 * is the maximum VL for the guest. Because the layout of the
> > +	 * data when saving the sve state depends on the VL, we need
> > +	 * to use a consistent (i.e., the maximum) VL.  Note that this
> > +	 * means that at guest exit ZCR_EL1 is not necessarily the
> > +	 * same as on guest entry.
> > +	 *
> > +	 * ZCR_EL2 holds the guest hypervisor's VL when running a
> > +	 * nested guest, which could be smaller than the max for the
> > +	 * vCPU. Similar to above, we first need to switch to a VL
> > +	 * consistent with the layout of the vCPU's SVE state. KVM
> > +	 * support for NV implies VHE, so using the ZCR_EL1 alias is
> > +	 * safe.
> > +	 */
> > +	if (!has_vhe() || (vcpu_has_nv(vcpu) && !is_hyp_ctxt(vcpu)))
> > +		sve_cond_update_zcr_vq(vcpu_sve_max_vq(vcpu) - 1,
> > +				       SYS_ZCR_EL1);
> > +}
> > +
> >  /*
> >   * Write back the vcpu FPSIMD regs if they are dirty, and invalidate the
> >   * cpu FPSIMD regs so that they can't be spuriously reused if this vcpu
> > @@ -179,38 +214,10 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> >  	}
> 
> A little before this context, kvm_arch_vcpu_put_fp() calls
> local_irq_save(), which indicates that IRQs can be taken before this
> point, which is deeply suspicious.
> 
> If IRQs are enabled, then it's possible to take an IRQ and potentially
> run a softirq handler which uses kernel-mode NEON. That means
> kernel_neon_begin() will try to save the live FPSIMD/SVE/SME state via
> fpsimd_save_user_state(), using the live value of ZCR_ELx.LEN, which would not
> be correct per the comment.
> 
> Looking at kvm_arch_vcpu_ioctl_run(), the relevant logic is:
> 
> 	vcpu_load(vcpu); // calls kvm_arch_vcpu_load_fp()
> 
> 	while (ret > 0) {
> 		preempt_disable();
> 		local_irq_disable();
> 
> 		kvm_arch_vcpu_ctxflush_fp();
> 		ret = kvm_arm_vcpu_enter_exit(vcpu);
> 		kvm_arch_vcpu_ctxsync_fp(vcpu);
> 
> 		local_irq_enable();
> 		preempt_enable();
> 	}
> 
> 	vcpu_put(vcpu); // calls kvm_arch_vcpu_put_fp()
> 
> ... and the problem can occur at any point after the vCPU has run where IRQs
> are enabled, i.e, between local_irq_enable() and either local_irq_disable() or
> vcpu_put()'s call to kvm_arch_vcpu_put_fp().
> 
> Note that kernel_neon_begin() calls:
> 
> 	fpsimd_save_user_state();
> 	...
> 	fpsimd_flush_cpu_state();
> 
> ... and fpsimd_save_user_state() will see that the SVE VL is wrong:
> 
> 	if (WARN_ON(sve_get_vl() != vl)) {
> 		force_signal_inject(SIGKILL, SI_KERNEL, 0, 0);
> 		return;
> 	}
> 
> ... pending a SIGKILL for the VMM thread without saving the vCPU's state
> before unbinding the live state via fpsimd_flush_cpu_state(), which'll
> set TIF_FOREIGN_FPSTATE.
> 
> AFAICT it's possible to re-enter the vCPU after that happens, whereupon
> stale vCPU FPSIMD/SVE state will be restored. Upon return to userspace
> the SIGKILL will be delivered, killing the VMM.
> 
> As above, it looks like that's been broken since the nVHE SVE
> save/restore was introduced in commit:
> 
>   8c8010d69c132273 ("KVM: arm64: Save/restore SVE state for nVHE")
> 
> The TL;DR summary is that it's not sufficient for kvm_arch_vcpu_put_fp()
> to fix up ZCR_ELx. Either:
> 
> * That needs to be fixed up while IRQs are masked, e.g. by
>   saving/restoring the host and guest ZCR_EL1 and/or ZCR_ELx values in
>   kvm_arch_vcpu_ctxflush_fp() and kvm_arch_vcpu_ctxsync_fp()
> 
> * The lazy save logic in fpsimd_save_user_state() needs to handle KVM
>   explicitly, saving the guest's ZCR_EL1 and restoring the host's
>   ZCR_ELx.
> 
> I think we need to fix that before we extend this logic for SME.

So save/restore ZCR_ELx eagerly? If that's what it takes, let's do
that now.

Thanks,

	M.

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