Message-ID: <20240906043413.1049633-1-seanjc@google.com>
Date: Thu,  5 Sep 2024 21:34:06 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Sean Christopherson <seanjc@...gle.com>, Paolo Bonzini <pbonzini@...hat.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Nathan Chancellor <nathan@...nel.org>, Chao Gao <chao.gao@...el.com>, 
	Zeng Guang <guang.zeng@...el.com>
Subject: [PATCH v2 0/7] KVM: nVMX: Fix IPIv vs. nested posted interrupts

Fix a bug where KVM injects L2's nested posted interrupt into L1 as a
nested VM-Exit instead of triggering PI processing.  The actual bug is
technically a generic nested posted interrupts problem, but due to the
way that KVM handles interrupt delivery, the issue is mostly limited to
to IPI virtualization being enabled.

Found by the nested posted interrupt KUT test on SPR.

If it weren't for an annoying TOCTOU bug waiting to happen, the fix would
be quite simple, e.g. it's really just:

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index f7dde74ff565..b07805daedf5 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -4288,6 +4288,15 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)
                        return -EBUSY;
                if (!nested_exit_on_intr(vcpu))
                        goto no_vmexit;
+
+               if (nested_cpu_has_posted_intr(get_vmcs12(vcpu)) &&
+                   kvm_apic_has_interrupt(vcpu) == vmx->nested.posted_intr_nv) {
+                       vmx->nested.pi_pending = true;
+                       kvm_apic_clear_irr(vcpu, vmx->nested.posted_intr_nv);
+                       goto no_vmexit;
+               }
+
                nested_vmx_vmexit(vcpu, EXIT_REASON_EXTERNAL_INTERRUPT, 0, 0);
                return 0;
        }

v2:
 - Split kvm_get_apic_interrupt() into has+ack to avoid marking the IRQ as
   in-service in vmcs02 instead of vmcs01. [Nathan]
 - Gather reviews, but only for the patches that didn't meaningful change (all
   two of them). [Chao]
 - Drop Cc: stable@ from all patches.  For real world hypervisors, this is
   unlikely to cause functional issues, only loss of IPI virtualization
   performance due to the unnecessary VM-Exit.  Whereas evidenced by my screwup
   in v1, this code is plenty subtle enough to introduce bugs.
 - Drop the patch to store nested.posted_intr_nv as an int, as there is no need
   to explicitly match -1 (as a signed int) in this approach.
 - Add a patch to assert vcpu->mutex is held when getting vmcs12, as I was
   "this" close to yanking out nested.posted_intr_nv, until I realized that
   accessing a different vCPU's vmcs12 in the IPI path is unsafe.

v1: https://lore.kernel.org/all/20240720000138.3027780-1-seanjc@google.com

Sean Christopherson (7):
  KVM: x86: Move "ack" phase of local APIC IRQ delivery to separate API
  KVM: nVMX: Get to-be-acknowledge IRQ for nested VM-Exit at injection
    site
  KVM: nVMX: Suppress external interrupt VM-Exit injection if there's no
    IRQ
  KVM: nVMX: Detect nested posted interrupt NV at nested VM-Exit
    injection
  KVM: x86: Fold kvm_get_apic_interrupt() into kvm_cpu_get_interrupt()
  KVM: nVMX: Explicitly invalidate posted_intr_nv if PI is disabled at
    VM-Enter
  KVM: nVMX: Assert that vcpu->mutex is held when accessing secondary
    VMCSes

 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/irq.c              | 10 ++++--
 arch/x86/kvm/lapic.c            |  9 +++---
 arch/x86/kvm/lapic.h            |  2 +-
 arch/x86/kvm/vmx/nested.c       | 57 ++++++++++++++++++++++++++-------
 arch/x86/kvm/vmx/nested.h       |  6 ++++
 arch/x86/kvm/vmx/vmx.c          |  7 ++++
 7 files changed, 72 insertions(+), 20 deletions(-)


base-commit: 332d2c1d713e232e163386c35a3ba0c1b90df83f
-- 
2.46.0.469.g59c65b2a67-goog

Powered by blists - more mailing lists