[PATCH 0/2] KVM: nVMX: Fix an SVI update bug with passthrough APIC

[PATCH 0/2] KVM: nVMX: Fix an SVI update bug with passthrough APIC

[Sean Christopherson]

Defer updating SVI (i.e. the VMCS's highest ISR cache) when L2 is active,
but L1 has not enabled virtual interrupt delivery for L2, as an EOI that
is emulated _by KVM_ in such a case acts on L1's ISR, i.e. vmcs01 needs to
reflect the updated ISR when L1 is next run.

Note, L1's ISR is also effectively L2's ISR in such a setup, but because
virtual interrupt deliver is disable for L2, there's no need to update
SVI in vmcs02, because it will never be used.

Chao Gao (1):
  KVM: nVMX: Defer SVI update to vmcs01 on EOI when L2 is active w/o VID

Sean Christopherson (1):
  KVM: x86: Plumb in the vCPU to kvm_x86_ops.hwapic_isr_update()

 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/lapic.c            | 22 ++++++++++++++++------
 arch/x86/kvm/lapic.h            |  1 +
 arch/x86/kvm/vmx/nested.c       |  5 +++++
 arch/x86/kvm/vmx/vmx.c          | 19 ++++++++++++++++++-
 arch/x86/kvm/vmx/vmx.h          |  1 +
 arch/x86/kvm/vmx/x86_ops.h      |  2 +-
 7 files changed, 43 insertions(+), 9 deletions(-)


base-commit: e466901b947d529f7b091a3b00b19d2bdee206ee
-- 
2.47.0.163.g1226f6d8fa-goog

[PATCH 1/2] KVM: x86: Plumb in the vCPU to kvm_x86_ops.hwapic_isr_update()

[Sean Christopherson]

Pass the target vCPU to the hwapic_isr_update() vendor hook so that VMX
can defer the update until after nested VM-Exit if an EOI for L1's vAPIC
occurs while L2 is active.

No functional change intended.

Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/lapic.c            | 11 +++++------
 arch/x86/kvm/vmx/vmx.c          |  2 +-
 arch/x86/kvm/vmx/x86_ops.h      |  2 +-
 4 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 70c7ed0ef184..3f3de047cbfd 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1734,7 +1734,7 @@ struct kvm_x86_ops {
 	bool allow_apicv_in_x2apic_without_x2apic_virtualization;
 	void (*refresh_apicv_exec_ctrl)(struct kvm_vcpu *vcpu);
 	void (*hwapic_irr_update)(struct kvm_vcpu *vcpu, int max_irr);
-	void (*hwapic_isr_update)(int isr);
+	void (*hwapic_isr_update)(struct kvm_vcpu *vcpu, int isr);
 	void (*load_eoi_exitmap)(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap);
 	void (*set_virtual_apic_mode)(struct kvm_vcpu *vcpu);
 	void (*set_apic_access_page_addr)(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 65412640cfc7..5be2be44a188 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -763,7 +763,7 @@ static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
 	 * just set SVI.
 	 */
 	if (unlikely(apic->apicv_active))
-		kvm_x86_call(hwapic_isr_update)(vec);
+		kvm_x86_call(hwapic_isr_update)(apic->vcpu, vec);
 	else {
 		++apic->isr_count;
 		BUG_ON(apic->isr_count > MAX_APIC_VECTOR);
@@ -808,7 +808,7 @@ static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
 	 * and must be left alone.
 	 */
 	if (unlikely(apic->apicv_active))
-		kvm_x86_call(hwapic_isr_update)(apic_find_highest_isr(apic));
+		kvm_x86_call(hwapic_isr_update)(apic->vcpu, apic_find_highest_isr(apic));
 	else {
 		--apic->isr_count;
 		BUG_ON(apic->isr_count < 0);
@@ -2767,7 +2767,7 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event)
 	if (apic->apicv_active) {
 		kvm_x86_call(apicv_post_state_restore)(vcpu);
 		kvm_x86_call(hwapic_irr_update)(vcpu, -1);
-		kvm_x86_call(hwapic_isr_update)(-1);
+		kvm_x86_call(hwapic_isr_update)(vcpu, -1);
 	}
 
 	vcpu->arch.apic_arb_prio = 0;
@@ -3083,9 +3083,8 @@ int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s)
 	kvm_apic_update_apicv(vcpu);
 	if (apic->apicv_active) {
 		kvm_x86_call(apicv_post_state_restore)(vcpu);
-		kvm_x86_call(hwapic_irr_update)(vcpu,
-						apic_find_highest_irr(apic));
-		kvm_x86_call(hwapic_isr_update)(apic_find_highest_isr(apic));
+		kvm_x86_call(hwapic_irr_update)(vcpu, apic_find_highest_irr(apic));
+		kvm_x86_call(hwapic_isr_update)(vcpu, apic_find_highest_isr(apic));
 	}
 	kvm_make_request(KVM_REQ_EVENT, vcpu);
 	if (ioapic_in_kernel(vcpu->kvm))
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 6ed801ffe33f..fe9887a5fa4a 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6868,7 +6868,7 @@ void vmx_set_apic_access_page_addr(struct kvm_vcpu *vcpu)
 	read_unlock(&vcpu->kvm->mmu_lock);
 }
 
-void vmx_hwapic_isr_update(int max_isr)
+void vmx_hwapic_isr_update(struct kvm_vcpu *vcpu, int max_isr)
 {
 	u16 status;
 	u8 old;
diff --git a/arch/x86/kvm/vmx/x86_ops.h b/arch/x86/kvm/vmx/x86_ops.h
index a55981c5216e..48dc76bf0ec0 100644
--- a/arch/x86/kvm/vmx/x86_ops.h
+++ b/arch/x86/kvm/vmx/x86_ops.h
@@ -48,7 +48,7 @@ void vmx_migrate_timers(struct kvm_vcpu *vcpu);
 void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu);
 void vmx_apicv_pre_state_restore(struct kvm_vcpu *vcpu);
 void vmx_hwapic_irr_update(struct kvm_vcpu *vcpu, int max_irr);
-void vmx_hwapic_isr_update(int max_isr);
+void vmx_hwapic_isr_update(struct kvm_vcpu *vcpu, int max_isr);
 int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu);
 void vmx_deliver_interrupt(struct kvm_lapic *apic, int delivery_mode,
 			   int trig_mode, int vector);
-- 
2.47.0.163.g1226f6d8fa-goog

Re: [PATCH 1/2] KVM: x86: Plumb in the vCPU to kvm_x86_ops.hwapic_isr_update()

[Chao Gao]

On Fri, Nov 01, 2024 at 12:21:13PM -0700, Sean Christopherson wrote:
>Pass the target vCPU to the hwapic_isr_update() vendor hook so that VMX
>can defer the update until after nested VM-Exit if an EOI for L1's vAPIC
>occurs while L2 is active.
>
>No functional change intended.
>
>Cc: stable@vger.kernel.org
>Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Chao Gao <chao.gao@intel.com>

>---
> arch/x86/include/asm/kvm_host.h |  2 +-
> arch/x86/kvm/lapic.c            | 11 +++++------
> arch/x86/kvm/vmx/vmx.c          |  2 +-
> arch/x86/kvm/vmx/x86_ops.h      |  2 +-
> 4 files changed, 8 insertions(+), 9 deletions(-)
>
>diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>index 70c7ed0ef184..3f3de047cbfd 100644
>--- a/arch/x86/include/asm/kvm_host.h
>+++ b/arch/x86/include/asm/kvm_host.h
>@@ -1734,7 +1734,7 @@ struct kvm_x86_ops {
> 	bool allow_apicv_in_x2apic_without_x2apic_virtualization;
> 	void (*refresh_apicv_exec_ctrl)(struct kvm_vcpu *vcpu);
> 	void (*hwapic_irr_update)(struct kvm_vcpu *vcpu, int max_irr);
>-	void (*hwapic_isr_update)(int isr);
>+	void (*hwapic_isr_update)(struct kvm_vcpu *vcpu, int isr);
> 	void (*load_eoi_exitmap)(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap);
> 	void (*set_virtual_apic_mode)(struct kvm_vcpu *vcpu);
> 	void (*set_apic_access_page_addr)(struct kvm_vcpu *vcpu);
>diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>index 65412640cfc7..5be2be44a188 100644
>--- a/arch/x86/kvm/lapic.c
>+++ b/arch/x86/kvm/lapic.c
>@@ -763,7 +763,7 @@ static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
> 	 * just set SVI.
> 	 */
> 	if (unlikely(apic->apicv_active))
>-		kvm_x86_call(hwapic_isr_update)(vec);
>+		kvm_x86_call(hwapic_isr_update)(apic->vcpu, vec);

Both branches need braces here. So, maybe take the opportunity to fix the coding style
issue.

> 	else {
> 		++apic->isr_count;
> 		BUG_ON(apic->isr_count > MAX_APIC_VECTOR);
>@@ -808,7 +808,7 @@ static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
> 	 * and must be left alone.
> 	 */
> 	if (unlikely(apic->apicv_active))
>-		kvm_x86_call(hwapic_isr_update)(apic_find_highest_isr(apic));
>+		kvm_x86_call(hwapic_isr_update)(apic->vcpu, apic_find_highest_isr(apic));

ditto

> 	else {
> 		--apic->isr_count;
> 		BUG_ON(apic->isr_count < 0);

Re: [PATCH 1/2] KVM: x86: Plumb in the vCPU to kvm_x86_ops.hwapic_isr_update()

[Sean Christopherson]

On Mon, Nov 04, 2024, Chao Gao wrote:
> On Fri, Nov 01, 2024 at 12:21:13PM -0700, Sean Christopherson wrote:
> >Pass the target vCPU to the hwapic_isr_update() vendor hook so that VMX
> >can defer the update until after nested VM-Exit if an EOI for L1's vAPIC
> >occurs while L2 is active.
> >
> >No functional change intended.
> >
> >Cc: stable@vger.kernel.org
> >Signed-off-by: Sean Christopherson <seanjc@google.com>
> 
> Reviewed-by: Chao Gao <chao.gao@intel.com>
> 
> >---
> > arch/x86/include/asm/kvm_host.h |  2 +-
> > arch/x86/kvm/lapic.c            | 11 +++++------
> > arch/x86/kvm/vmx/vmx.c          |  2 +-
> > arch/x86/kvm/vmx/x86_ops.h      |  2 +-
> > 4 files changed, 8 insertions(+), 9 deletions(-)
> >
> >diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> >index 70c7ed0ef184..3f3de047cbfd 100644
> >--- a/arch/x86/include/asm/kvm_host.h
> >+++ b/arch/x86/include/asm/kvm_host.h
> >@@ -1734,7 +1734,7 @@ struct kvm_x86_ops {
> > 	bool allow_apicv_in_x2apic_without_x2apic_virtualization;
> > 	void (*refresh_apicv_exec_ctrl)(struct kvm_vcpu *vcpu);
> > 	void (*hwapic_irr_update)(struct kvm_vcpu *vcpu, int max_irr);
> >-	void (*hwapic_isr_update)(int isr);
> >+	void (*hwapic_isr_update)(struct kvm_vcpu *vcpu, int isr);
> > 	void (*load_eoi_exitmap)(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap);
> > 	void (*set_virtual_apic_mode)(struct kvm_vcpu *vcpu);
> > 	void (*set_apic_access_page_addr)(struct kvm_vcpu *vcpu);
> >diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> >index 65412640cfc7..5be2be44a188 100644
> >--- a/arch/x86/kvm/lapic.c
> >+++ b/arch/x86/kvm/lapic.c
> >@@ -763,7 +763,7 @@ static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
> > 	 * just set SVI.
> > 	 */
> > 	if (unlikely(apic->apicv_active))
> >-		kvm_x86_call(hwapic_isr_update)(vec);
> >+		kvm_x86_call(hwapic_isr_update)(apic->vcpu, vec);
> 
> Both branches need braces here. So, maybe take the opportunity to fix the
> coding style issue.

Very tempting, but since this is destined for stable, I'll go with a minimal patch
to reduce the odds of creating a conflict.

> > 	else {
> > 		++apic->isr_count;
> > 		BUG_ON(apic->isr_count > MAX_APIC_VECTOR);
> >@@ -808,7 +808,7 @@ static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
> > 	 * and must be left alone.
> > 	 */
> > 	if (unlikely(apic->apicv_active))
> >-		kvm_x86_call(hwapic_isr_update)(apic_find_highest_isr(apic));
> >+		kvm_x86_call(hwapic_isr_update)(apic->vcpu, apic_find_highest_isr(apic));

Re: [PATCH 1/2] KVM: x86: Plumb in the vCPU to kvm_x86_ops.hwapic_isr_update()

[Sean Christopherson]

On Fri, Nov 01, 2024, Sean Christopherson wrote:
> Pass the target vCPU to the hwapic_isr_update() vendor hook so that VMX
> can defer the update until after nested VM-Exit if an EOI for L1's vAPIC
> occurs while L2 is active.
> 
> No functional change intended.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  2 +-
>  arch/x86/kvm/lapic.c            | 11 +++++------
>  arch/x86/kvm/vmx/vmx.c          |  2 +-
>  arch/x86/kvm/vmx/x86_ops.h      |  2 +-
>  4 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 70c7ed0ef184..3f3de047cbfd 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1734,7 +1734,7 @@ struct kvm_x86_ops {
>  	bool allow_apicv_in_x2apic_without_x2apic_virtualization;
>  	void (*refresh_apicv_exec_ctrl)(struct kvm_vcpu *vcpu);
>  	void (*hwapic_irr_update)(struct kvm_vcpu *vcpu, int max_irr);
> -	void (*hwapic_isr_update)(int isr);
> +	void (*hwapic_isr_update)(struct kvm_vcpu *vcpu, int isr);

Oh, the hilarity.  Got that one wrong.
 
  d39850f57d21 ("KVM: x86: Drop @vcpu parameter from kvm_x86_ops.hwapic_isr_update()")

Not entirely sure what cleanups were made possible by dropping @vcpu at the time.
I assume the end goal was ce0a58f4756c ("KVM: x86: Move "apicv_active" into "struct
kvm_lapic""), but that should have been possible, if slightly more annoying, without
modifying hwapic_isr_update().  *sigh*

[PATCH 2/2] KVM: nVMX: Defer SVI update to vmcs01 on EOI when L2 is active w/o VID

[Sean Christopherson]

From: Chao Gao <chao.gao@intel.com>

If KVM emulates an EOI for L1's virtual APIC while L2 is active, defer
updating GUEST_INTERUPT_STATUS.SVI, i.e. the VMCS's cache of the highest
in-service IRQ, until L1 is active, as vmcs01, not vmcs02, needs to track
vISR.  The missed SVI update for vmcs01 can result in L1 interrupts being
incorrectly blocked, e.g. if there is a pending interrupt with lower
priority than the interrupt that was EOI'd.

This bug only affects use cases where L1's vAPIC is effectively passed
through to L2, e.g. in a pKVM scenario where L2 is L1's depriveleged host,
as KVM will only emulate an EOI for L1's vAPIC if Virtual Interrupt
Delivery (VID) is disabled in vmc12, and L1 isn't intercepting L2 accesses
to its (virtual) APIC page (or if x2APIC is enabled, the EOI MSR).

WARN() if KVM updates L1's ISR while L2 is active with VID enabled, as an
EOI from L2 is supposed to affect L2's vAPIC, but still defer the update,
to try to keep L1 alive.  Specifically, KVM forwards all APICv-related
VM-Exits to L1 via nested_vmx_l1_wants_exit():

	case EXIT_REASON_APIC_ACCESS:
	case EXIT_REASON_APIC_WRITE:
	case EXIT_REASON_EOI_INDUCED:
		/*
		 * The controls for "virtualize APIC accesses," "APIC-
		 * register virtualization," and "virtual-interrupt
		 * delivery" only come from vmcs12.
		 */
		return true;

Fixes: c7c9c56ca26f ("x86, apicv: add virtual interrupt delivery support")
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/kvm/20230312180048.1778187-1-jason.cj.chen@intel.com
Reported-by: Markku Ahvenjärvi <mankku@gmail.com>
Closes: https://lore.kernel.org/all/20240920080012.74405-1-mankku@gmail.com
Cc: Janne Karhunen <janne.karhunen@gmail.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
[sean: drop request, handle in VMX, write changelog]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/lapic.c      | 11 +++++++++++
 arch/x86/kvm/lapic.h      |  1 +
 arch/x86/kvm/vmx/nested.c |  5 +++++
 arch/x86/kvm/vmx/vmx.c    | 17 +++++++++++++++++
 arch/x86/kvm/vmx/vmx.h    |  1 +
 5 files changed, 35 insertions(+)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 5be2be44a188..66751bf9d4f4 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -816,6 +816,17 @@ static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
 	}
 }
 
+void kvm_apic_update_hwapic_isr(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	if (WARN_ON_ONCE(!lapic_in_kernel(vcpu)) || !apic->apicv_active)
+		return;
+
+	kvm_x86_call(hwapic_isr_update)(apic->vcpu, apic_find_highest_isr(apic));
+}
+EXPORT_SYMBOL_GPL(kvm_apic_update_hwapic_isr);
+
 int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
 {
 	/* This may race with setting of irr in __apic_accept_irq() and
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 1b8ef9856422..469a6f20e2db 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -122,6 +122,7 @@ int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
 int kvm_apic_get_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s);
 int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s);
 enum lapic_mode kvm_get_apic_mode(struct kvm_vcpu *vcpu);
+void kvm_apic_update_hwapic_isr(struct kvm_vcpu *vcpu);
 int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu);
 
 u64 kvm_get_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 746cb41c5b98..0111539fcea1 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5036,6 +5036,11 @@ void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 vm_exit_reason,
 		kvm_make_request(KVM_REQ_APICV_UPDATE, vcpu);
 	}
 
+	if (vmx->nested.update_vmcs01_hwapic_isr) {
+		vmx->nested.update_vmcs01_hwapic_isr = false;
+		kvm_apic_update_hwapic_isr(vcpu);
+	}
+
 	if ((vm_exit_reason != -1) &&
 	    (enable_shadow_vmcs || nested_vmx_is_evmptr12_valid(vmx)))
 		vmx->nested.need_vmcs12_to_shadow_sync = true;
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index fe9887a5fa4a..a3513fc05a01 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6873,6 +6873,23 @@ void vmx_hwapic_isr_update(struct kvm_vcpu *vcpu, int max_isr)
 	u16 status;
 	u8 old;
 
+	/*
+	 * If L2 is active, defer the SVI update until vmcs01 is loaded, as SVI
+	 * is only relevant for if and only if Virtual Interrupt Delivery is
+	 * enabled in vmcs12, and if VID is enabled then L2 EOIs affect L2's
+	 * vAPIC, not L1's vAPIC.  KVM must update vmcs01 on the next nested
+	 * VM-Exit, otherwise L1 with run with a stale SVI.
+	 */
+	if (is_guest_mode(vcpu)) {
+		/*
+		 * KVM is supposed to forward intercepted L2 EOIs to L1 if VID
+		 * is enabled in vmcs12; as above, the EOIs affect L2's vAPIC.
+		 */
+		WARN_ON_ONCE(nested_cpu_has_vid(get_vmcs12(vcpu)));
+		to_vmx(vcpu)->nested.update_vmcs01_hwapic_isr = true;
+		return;
+	}
+
 	if (max_isr == -1)
 		max_isr = 0;
 
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 43f573f6ca46..892302022094 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -176,6 +176,7 @@ struct nested_vmx {
 	bool reload_vmcs01_apic_access_page;
 	bool update_vmcs01_cpu_dirty_logging;
 	bool update_vmcs01_apicv_status;
+	bool update_vmcs01_hwapic_isr;
 
 	/*
 	 * Enlightened VMCS has been enabled. It does not mean that L1 has to
-- 
2.47.0.163.g1226f6d8fa-goog

Re: [PATCH 2/2] KVM: nVMX: Defer SVI update to vmcs01 on EOI when L2 is active w/o VID

[Chao Gao]

>@@ -6873,6 +6873,23 @@ void vmx_hwapic_isr_update(struct kvm_vcpu *vcpu, int max_isr)
> 	u16 status;
> 	u8 old;
> 
>+	/*
>+	 * If L2 is active, defer the SVI update until vmcs01 is loaded, as SVI
>+	 * is only relevant for if and only if Virtual Interrupt Delivery is
>+	 * enabled in vmcs12, and if VID is enabled then L2 EOIs affect L2's
>+	 * vAPIC, not L1's vAPIC.  KVM must update vmcs01 on the next nested
>+	 * VM-Exit, otherwise L1 with run with a stale SVI.
>+	 */
>+	if (is_guest_mode(vcpu)) {
>+		/*
>+		 * KVM is supposed to forward intercepted L2 EOIs to L1 if VID
>+		 * is enabled in vmcs12; as above, the EOIs affect L2's vAPIC.
>+		 */
>+		WARN_ON_ONCE(nested_cpu_has_vid(get_vmcs12(vcpu)));

This function can be called in other scenarios, e.g., from
kvm_apic_set_state(). Is it possible for the userspace VMM to set the APIC
state while L2 is running and VID is enabled for L2? If so, this warning could
be triggered by the userspace VMM.

Anyway, I verified this patch can fix the reported issue. So,

Tested-by: Chao Gao <chao.gao@intel.com>

>+		to_vmx(vcpu)->nested.update_vmcs01_hwapic_isr = true;
>+		return;
>+	}
>+
> 	if (max_isr == -1)
> 		max_isr = 0;

Re: [PATCH 2/2] KVM: nVMX: Defer SVI update to vmcs01 on EOI when L2 is active w/o VID

[Sean Christopherson]

On Mon, Nov 04, 2024, Chao Gao wrote:
> >@@ -6873,6 +6873,23 @@ void vmx_hwapic_isr_update(struct kvm_vcpu *vcpu, int max_isr)
> > 	u16 status;
> > 	u8 old;
> > 
> >+	/*
> >+	 * If L2 is active, defer the SVI update until vmcs01 is loaded, as SVI
> >+	 * is only relevant for if and only if Virtual Interrupt Delivery is
> >+	 * enabled in vmcs12, and if VID is enabled then L2 EOIs affect L2's
> >+	 * vAPIC, not L1's vAPIC.  KVM must update vmcs01 on the next nested
> >+	 * VM-Exit, otherwise L1 with run with a stale SVI.
> >+	 */
> >+	if (is_guest_mode(vcpu)) {
> >+		/*
> >+		 * KVM is supposed to forward intercepted L2 EOIs to L1 if VID
> >+		 * is enabled in vmcs12; as above, the EOIs affect L2's vAPIC.
> >+		 */
> >+		WARN_ON_ONCE(nested_cpu_has_vid(get_vmcs12(vcpu)));
> 
> This function can be called in other scenarios, e.g., from
> kvm_apic_set_state(). Is it possible for the userspace VMM to set the APIC
> state while L2 is running and VID is enabled for L2? If so, this warning could
> be triggered by the userspace VMM.

Ugh, yeah, that's reachable.  Bummer.  Ooh!  I think we can use the recently
added "wants_to_run" to filter out state stuffing.  I.e. this

		WARN_ON_ONCE(vcpu->wants_to_run &&
			     nested_cpu_has_vid(get_vmcs12(vcpu)));

plus an updated comment.