[PATCH v2 0/2] KVM: nVMX: fix nested APICv emulation for windows guests

[PATCH v2 0/2] KVM: nVMX: fix nested APICv emulation for windows guests

[Maxim Levitsky]

Fix two problems that were recently discovered that cause nested APICv
to malfunction on windows with hyperv enabled.

The first problem is that unlike KVM, windows doesn't set PIR.ON when
sending posted interrupts. X86 spec actually allows this.

The second problem is that unlilke KVM, which mostly ignores them,
windows depends on accurate delivery of the posted notification
interrupts, and currently it is possible for a posted notification
to get lost it it arrives at the same time as the nested VM exit.

V2: to avoid spurious notification in L1, process the posted
notification in L2, just before VM exit.

PS: I am not 100% sure if I got the barriers right, plus I am not sure
about what to do if vmx_complete_nested_posted_interrupt can't write
to the nested PIR, in this it calls kvm_handle_memory_failure, and
I don't know 100% its interactions with nested VM exit.

Best regards,
	Maxim Levitsky

Maxim Levitsky (2):
  KVM: nVMX: don't check PIR.ON when processing nested posted interrupts
  KVM: nVMX: avoid losing the posted notification interrupt when exiting
    L2

 arch/x86/kvm/vmx/nested.c | 22 ++++++++++++++++++----
 arch/x86/kvm/vmx/vmx.c    |  5 +++++
 2 files changed, 23 insertions(+), 4 deletions(-)

-- 
2.54.0

[PATCH v2 1/2] KVM: nVMX: don't check PIR.ON when processing nested posted interrupts

[Maxim Levitsky]

Despite the suggestion to test and set the PIR.ON and avoid sending
a posted interrupt if it is already set, there is no requirement
for this bit to be set for posted interrupt processing to happen
when the target CPU receives the posted notification vector.

See section 30.6 POSTED-INTERRUPT PROCESSING:

   "The processor clears the outstanding-notification bit in
    the posted-interrupt descriptor. This is done atomically so as to leave
    the remainder of the descriptor unmodified
    (e.g., with a locked AND operation)."

Apparently Windows' implementation of APICv does exactly this:
it sets bits in PIR, without bothering to also set the PIR.ON.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/vmx/nested.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 151873407abd..8504a2c12d9d 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -4041,8 +4041,16 @@ static int vmx_complete_nested_posted_interrupt(struct kvm_vcpu *vcpu)
 
 	vmx->nested.pi_pending = false;
 
-	if (!pi_test_and_clear_on(vmx->nested.pi_desc))
-		return 0;
+	/*
+	 * Don't test the value of PID.ON.
+	 * It is valid to trigger a posted interrupt without setting this bit
+	 */
+	pi_clear_on(vmx->nested.pi_desc);
+
+	/* Ensure that guests sees the change to PIR.ON before KVM harvest
+	 * the PIR
+	 */
+	__smp_mb__after_atomic();
 
 	max_irr = pi_find_highest_vector(vmx->nested.pi_desc);
 	if (max_irr > 0) {
@@ -4209,8 +4217,7 @@ static bool vmx_has_nested_events(struct kvm_vcpu *vcpu, bool for_injection)
 	if ((max_irr & 0xf0) > (vppr & 0xf0))
 		return true;
 
-	if (vmx->nested.pi_pending && vmx->nested.pi_desc &&
-	    pi_test_on(vmx->nested.pi_desc)) {
+	if (vmx->nested.pi_pending && vmx->nested.pi_desc) {
 		max_irr = pi_find_highest_vector(vmx->nested.pi_desc);
 		if (max_irr > 0 && (max_irr & 0xf0) > (vppr & 0xf0))
 			return true;
-- 
2.54.0

Re: [PATCH v2 1/2] KVM: nVMX: don't check PIR.ON when processing nested posted interrupts

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] x86/kvm: direct usage of __smp_mb__after_atomic() bypasses KCSAN
--

commit b3864bd32325846a2ccca43a4ce66f3b92dcb341
Author: Maxim Levitsky <mlevitsk@redhat.com>

KVM: nVMX: don't check PIR.ON when processing nested posted interrupts
    
Despite the suggestion to test and set the PIR.ON and avoid sending
a posted interrupt if it is already set, there is no requirement
for this bit to be set for posted interrupt processing to happen
when the target CPU receives the posted notification vector.

> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index ddf6df7bee93b..50548e3483181 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -4033,8 +4033,16 @@ static int vmx_complete_nested_posted_interrupt(struct kvm_vcpu *vcpu)
>  
>  	vmx->nested.pi_pending = false;
>  
> -	if (!pi_test_and_clear_on(vmx->nested.pi_desc))
> -		return 0;
> +	/*
> +	 * Don't test the value of PID.ON.
> +	 * It is valid to trigger a posted interrupt without setting this bit
> +	 */
> +	pi_clear_on(vmx->nested.pi_desc);
> +
> +	/* Ensure that guests sees the change to PIR.ON before KVM harvest
> +	 * the PIR
> +	 */
> +	__smp_mb__after_atomic();

[Severity: Low]
Does this code intentionally bypass KCSAN instrumentation?

By calling the architecture-internal macro __smp_mb__after_atomic() directly
instead of the generic smp_mb__after_atomic() API, KCSAN instrumentation
(provided by kcsan_mb()) is skipped.

While this does not cause a functional memory ordering bug on x86, it violates
kernel API conventions and could lead to false positives when running KCSAN.

>  
>  	max_irr = pi_find_highest_vector(vmx->nested.pi_desc);
>  	if (max_irr > 0) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011001303.723721-1-mlevitsk@redhat.com?part=1

[PATCH v2 2/2] KVM: nVMX: avoid losing the posted notification interrupt when exiting L2

[Maxim Levitsky]

While delivering a nested posted interrupt notification
(see vmx_deliver_nested_posted_interrupt), KVM assumes that,
as long as the target vCPU is in the guest mode, KVM can send the
special POSTED_INTR_NESTED_VECTOR, which will either trigger APICv ucode
to inject all interrupts into L2 or cause a VM exit, after which
vmx_complete_nested_posted_interrupt is supposed to finish the job.

However if the target vCPU was about to exit the L2 when
nested posted interrupt notification was delivered in this way,
it is possible that it will be lost.

Fix this by running vmx_complete_nested_posted_interrupt just before
nested VM exit.

Detect this case in the nested VM exit path and act accordingly.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/vmx/nested.c | 7 +++++++
 arch/x86/kvm/vmx/vmx.c    | 5 +++++
 2 files changed, 12 insertions(+)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 8504a2c12d9d..1b2f55a13993 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5114,8 +5114,15 @@ void __nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 vm_exit_reason,
 	if (enable_ept && is_pae_paging(vcpu))
 		vmx_ept_load_pdptrs(vcpu);
 
+
 	leave_guest_mode(vcpu);
 
+	/* pairs with barrier in vmx_deliver_nested_posted_interrupt */
+	smp_wmb();
+
+	if (vmx->nested.pi_pending)
+		vmx_complete_nested_posted_interrupt(vcpu);
+
 	if (nested_cpu_has_preemption_timer(vmcs12))
 		hrtimer_cancel(&to_vmx(vcpu)->nested.preemption_timer);
 
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 612ab07d4100..21f2f8e4c4f0 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4381,10 +4381,15 @@ static int vmx_deliver_nested_posted_interrupt(struct kvm_vcpu *vcpu,
 	 */
 	if (is_guest_mode(vcpu) &&
 	    vector == vmx->nested.posted_intr_nv) {
+
+		/* pairs with barrier in __nested_vmx_vmexit */
+		smp_rmb();
+
 		/*
 		 * If a posted intr is not recognized by hardware,
 		 * we will accomplish it in the next vmentry.
 		 */
+
 		vmx->nested.pi_pending = true;
 		kvm_make_request(KVM_REQ_EVENT, vcpu);
 
-- 
2.54.0

Re: [PATCH v2 2/2] KVM: nVMX: avoid losing the posted notification interrupt when exiting L2

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] kvm: nVMX: incorrect memory barriers (smp_wmb/smp_rmb) fail to prevent Store-Load race
- [High] kvm: nVMX: ignored return value in __nested_vmx_vmexit swallows memory failure
--

commit 8287f538318e13160edd8d2bb1ae4a6ae79766ff
Author: Maxim Levitsky <mlevitsk@redhat.com>

KVM: nVMX: avoid losing the posted notification interrupt when exiting L2

This commit runs vmx_complete_nested_posted_interrupt just before a nested
VM exit to prevent losing nested posted interrupt notifications when the
target vCPU is exiting L2. It also adds memory barriers between
vmx_deliver_nested_posted_interrupt and __nested_vmx_vmexit.

> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 50548e3483181..92d5ade58dc9c 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -5103,8 +5103,15 @@ void __nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 vm_exit_reason,
>  	if (enable_ept && is_pae_paging(vcpu))
>  		vmx_ept_load_pdptrs(vcpu);
>  
> +
>  	leave_guest_mode(vcpu);
>  
> +	/* pairs with barrier in vmx_deliver_nested_posted_interrupt */
> +	smp_wmb();
> +
> +	if (vmx->nested.pi_pending)
> +		vmx_complete_nested_posted_interrupt(vcpu);

[Severity: High]
Does this ignored return value swallow memory failures?

When vmx_complete_nested_posted_interrupt() encounters an error mapping the
virtual APIC page, it calls kvm_handle_memory_failure() to set
vcpu->run->exit_reason to KVM_EXIT_INTERNAL_ERROR and returns -ENXIO to abort
the current VM run.

Since __nested_vmx_vmexit() returns void and ignores this return value, is the
KVM_EXIT_INTERNAL_ERROR silently overwritten by the next VM-exit, preventing
userspace from receiving the memory failure error?

[Severity: High]
Do these memory barriers properly prevent a Store-Load race?

In __nested_vmx_vmexit(), we store to vcpu->arch.hflags (via leave_guest_mode)
and then load vmx->nested.pi_pending.

In vmx_deliver_nested_posted_interrupt(), we load vcpu->arch.hflags (via
is_guest_mode) and then store to vmx->nested.pi_pending.

This is a Store-Load sequence on the receiver and a Load-Store sequence on the
sender.

On x86, smp_wmb() acts only as a compiler barrier and does not prevent the CPU
store buffer from reordering the load of pi_pending before the store to hflags
becomes globally visible. As a result, the target vCPU can still read
pi_pending == false before its exit from guest mode is visible to the sender.
The sender will then see is_guest_mode == true and set pi_pending = true,
leaving the nested posted interrupt lost.

Is a full memory barrier (smp_mb()) required here to prevent Store-Load
reordering?

> +
>  	if (nested_cpu_has_preemption_timer(vmcs12))
>  		hrtimer_cancel(&to_vmx(vcpu)->nested.preemption_timer);
>  

[ ... ]

> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index cc75feec05daa..a40791167b10e 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4362,10 +4362,15 @@ static int vmx_deliver_nested_posted_interrupt(struct kvm_vcpu *vcpu,
>  	 */
>  	if (is_guest_mode(vcpu) &&
>  	    vector == vmx->nested.posted_intr_nv) {
> +
> +		/* pairs with barrier in __nested_vmx_vmexit */
> +		smp_rmb();

[Severity: High]
Does an smp_rmb() here correctly pair with the barrier in __nested_vmx_vmexit?

Since the sequence in vmx_deliver_nested_posted_interrupt() involves a load
(is_guest_mode) followed by a store (pi_pending = true), and the sequence in
__nested_vmx_vmexit() is a Store-Load, is a different synchronization primitive
or stronger barrier pairing needed to prevent the race?

> +
>  		/*
>  		 * If a posted intr is not recognized by hardware,
>  		 * we will accomplish it in the next vmentry.
>  		 */
> +
>  		vmx->nested.pi_pending = true;
>  		kvm_make_request(KVM_REQ_EVENT, vcpu);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011001303.723721-1-mlevitsk@redhat.com?part=2

Re: [PATCH v2 0/2] KVM: nVMX: fix nested APICv emulation for windows guests

[Paolo Bonzini]

On 10/11/26 02:13, Maxim Levitsky wrote:
> Fix two problems that were recently discovered that cause nested APICv
> to malfunction on windows with hyperv enabled.
> 
> The first problem is that unlike KVM, windows doesn't set PIR.ON when
> sending posted interrupts. X86 spec actually allows this.
> 
> The second problem is that unlilke KVM, which mostly ignores them,
> windows depends on accurate delivery of the posted notification
> interrupts, and currently it is possible for a posted notification
> to get lost it it arrives at the same time as the nested VM exit.
> 
> V2: to avoid spurious notification in L1, process the posted
> notification in L2, just before VM exit.
> 
> PS: I am not 100% sure if I got the barriers right, plus I am not sure
> about what to do if vmx_complete_nested_posted_interrupt can't write
> to the nested PIR, in this it calls kvm_handle_memory_failure, and
> I don't know 100% its interactions with nested VM exit.

Yeah, the first commit is fine but the barriers are still incomplete 
here.  It seems to me that it really works by chance: we're executing on 
the sending side, where there is no ordering with the setting of 
posted_intr_nv, but in real hardware the check happens during interrupt 
acknowledgement on the receiving side.

However, doing it on the sending side is the key to accelerating nested 
posted interrupts.  We need to check atomically for is_guest_mode and 
posted_intr_nv (which has 257 possible values - "-1" for disabled and 
0..255).  The pattern we are trying to use here is, in pseudocode:

	if (atomic {
		// really an xchg
		pi_active = false;
		pi_pending
	})
		complete nested PI

	--

	if (atomic {
		// really a cmpxchg loop
		pi_pending = true;
		pi_active && vector == posted_intr_nv
	})
		trigger L0's nested PI vector
	else
		set bit in IRR and KVM_REQ_EVENT // deliver to L1

(In reality, pi_pending need not be set unless pi_active is true. If 
pi_active is false, no PI is made pending because the PI vector will be 
delivered to L1).

This means the current code is missing two bits:

- pi_hw_delivered (now covered albeit incorrectly by is_guest_mode) and 
pi_pending must be accessed atomically;

- in the current code all this is enclosed in a check for the *remote* 
posted_intr_nv which is completely wrong, because there's no way to make 
those reads not racy.

This means adding a cmpxchg-based state machine that includes 
pi_hw_delivered, pi_pending, and the vector; which in turn means 
promoting guest_mode from being just a bit (HF_GUEST_MASK) to an int:

#define KVM_NESTED_PI_VECTOR_MASK	0xFF
#define KVM_NESTED_IN_GUEST		BIT(8)
#define KVM_NESTED_PI_ACTIVE		BIT(9)
#define KVM_NESTED_PI_PENDING		BIT(10)

void enter_guest_mode(struct kvm_vcpu *vcpu);
int leave_guest_mode(struct kvm_vcpu *vcpu);
/* not vmx specific anymore, move to irq.c (!) */
int kvm_deliver_nested_posted_interrupt(struct kvm_vcpu *vcpu,
					int vector);



On vmentry you can publish the state as soon as you've committed to not 
fail:

	/* which does
	 *   atomic_set(&arch.nested_guest_state, KVM_NESTED_IN_GUEST);
	 */
	enter_guest_mode(vcpu);

	...

	if (from_vmentry) {
		...
		/* and another copy of this in
		 * vmx_get_nested_state_pages(), called if
		 * nested_get_vmcs12_pages() succeeds, so maybe this
		 * must be a separate function */
		if (nested_cpu_has_posted_intr(vmcs12))
			atomic_or(
			   KVM_NESTED_PI_ACTIVE |
			   (vmcs12->posted_intr_nv &
			    KVM_NESTED_PI_VECTOR_MASK),
			   &vcpu->arch.nested_guest_state);
	} else {
		kvm_make_request(KVM_REQ_GET_NESTED_STATE_PAGES, vcpu);
	}

	vmx_complete_nested_posted_interrupt();

On nested vmexit:

	/*
	 * clear PI_ACTIVE to block nested posted interrupt delivery,
	 * and take one last opportunity to read KVM_NESTED_PI_PENDING.
	 *
	 * leave_guest_mode does
	 *	atomic_xchg(&vcpu->arch.nested_guest_state, 0);
	 */
	old = leave_guest_mode(vcpu);
	__vmx_complete_nested_posted_interrupt(vcpu, old);

	/*
	 * only one read is enough - after it, the sender vCPU is
	 * guaranteed to not see PI_ACTIVE after the cmpxchg() loop,
	 * making it fall back to delivery via LAPIC
	 */

And __vmx_complete_nested_posted_interrupt checks and clears 
KVM_NESTED_PI_PENDING:

	/* write other bits of state before reading pi_pending */
	if (!(old & KVM_NESTED_PI_PENDING))
		return 0;

	if (!vmx->nested.pi_desc)
		goto mmio_needed;

	atomic_andnot(KVM_NESTED_PI_PENDING,
		      &vcpu->arch.nested_guest_state);

vmx_complete_nested_posted_interrupt is a simple wrapper for the __ version:

	/* write other bits of state before reading pi_pending */
	smp_mb();
	return __vmx_complete_nested_posted_interrupt(vcpu,
		atomic_read(&vcpu->arch.nested_guest_state));


And to deliver:

for (;;) {
	old = atomic_read(&vcpu->arch.nested_guest_state);
	if ((old & (KVM_NESTED_PI_ACTIVE | KVM_NESTED_PI_VECTOR_MASK))
	     != (KVM_NESTED_PI_ACTIVE | vector))
		return -1;

	WARN_ON_ONCE(!(old & KVM_NESTED_IN_GUEST));
	if (atomic_try_cmpxchg(&vcpu->arch.nested_guest_state, &old,
			       old | KVM_NESTED_PI_PENDING))
		break;
}

kvm_make_request(KVM_REQ_EVENT, vcpu);
smp_mb__after_atomic();
kvm_vcpu_trigger_posted_interrupt(vcpu, POSTED_INTR_NESTED_VECTOR);


Paolo