Re: [PATCH 6/6] KVM: nVMX: Detect nested posted interrupt NV at nested VM-Exit injection

[Sean Christopherson <seanjc@google.com>]

On Tue, Jul 23, 2024, Chao Gao wrote:
> On Fri, Jul 19, 2024 at 05:01:38PM -0700, Sean Christopherson wrote:
> >When synthensizing a nested VM-Exit due to an external interrupt, pend a
> >nested posted interrupt if the external interrupt vector matches L2's PI
> >notification vector, i.e. if the interrupt is a PI notification for L2.
> >This fixes a bug where KVM will incorrectly inject VM-Exit instead of
> >processing nested posted interrupt when IPI virtualization is enabled.
> >
> >Per the SDM, detection of the notification vector doesn't occur until the
> >interrupt is acknowledge and deliver to the CPU core.
> >
> >  If the external-interrupt exiting VM-execution control is 1, any unmasked
> >  external interrupt causes a VM exit (see Section 26.2). If the "process
> >  posted interrupts" VM-execution control is also 1, this behavior is
> >  changed and the processor handles an external interrupt as follows:
> >
> >    1. The local APIC is acknowledged; this provides the processor core
> >       with an interrupt vector, called here the physical vector.
> >    2. If the physical vector equals the posted-interrupt notification
> >       vector, the logical processor continues to the next step. Otherwise,
> >       a VM exit occurs as it would normally due to an external interrupt;
> >       the vector is saved in the VM-exit interruption-information field.
> >
> >For the most part, KVM has avoided problems because a PI NV for L2 that
> >arrives will L2 is active will be processed by hardware, and KVM checks
> >for a pending notification vector during nested VM-Enter.
> 
> With this series in place, I wonder if we can remove the check for a pending
> notification vector during nested VM-Enter.
> 
> 	/* Emulate processing of posted interrupts on VM-Enter. */
> 	if (nested_cpu_has_posted_intr(vmcs12) &&
> 	    kvm_apic_has_interrupt(vcpu) == vmx->nested.posted_intr_nv) {
> 		vmx->nested.pi_pending = true;
> 		kvm_make_request(KVM_REQ_EVENT, vcpu);
> 		kvm_apic_clear_irr(vcpu, vmx->nested.posted_intr_nv);
> 	}
> 
> I believe the check is arguably incorrect because:
> 
> 1. nested_vmx_run() may set pi_pending and clear the IRR bit of the notification
> vector, but this doesn't guarantee that vmx_complete_nested_posted_interrupt()
> will be called later in vmx_check_nested_events(). This could lead to partial
> posted interrupt processing, where the IRR bit is cleared but PIR isn't copied
> into VIRR. This might confuse L1 since, from L1's perspective, posted interrupt
> processing should be atomic. Per the SDM, the logical processor performs
> posted-interrupt processing "in an uninterruptible manner".

vmx_deliver_nested_posted_interrupt() is also broken in this regard.  I don't see
a sane way to handle that though, at least not without completely losing the value
of posted interrupts.  Ooh, maybe we could call vmx_complete_nested_posted_interrupt()
from nested_vmx_vmexit()?  That is a little scary, but probably worth trying?

> 2. The check doesn't respect event priority. For example, if a higher-priority
> event (preemption timer exit or NMI-window exit) causes an immediate nested
> VM-exit, the notification vector should remain pending after the nested VM-exit.

Ah, right, because block_nested_events would be true due to the pending nested
VM-Enter, which would ensure KVM enters L2 and trips NMI/IRQ window exiting.

The downside is that removing that code would regress performance for the more
common case (no NMI/IRQ window), as KVM would need to complete the nested
VM-Enter before consuming the IRQ, i.e. would need to do a VM-Enter and force a
VM-Exit.  But as you say, that's the architecturally correct behavior.