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

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.