On Fri, 17 Jan 2025 11:34:09 +0000,
Mark Rutland <mark.rutland@xxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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.
