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