Re: [PATCH v3 11/11] KVM: nVMX: Wake L2 from HLT when nested posted-interrupt pending

[Sean Christopherson <seanjc@google.com>]

On Tue, Nov 24, 2020, Paolo Bonzini wrote:
> On 24/11/20 02:55, Sean Christopherson wrote:
> > > > I believe there is a 1-to-many relationship here, which is why I said
> > > > each CPU would need to maintain a linked list of possible vCPUs to
> > > > iterate and find the intended recipient.
> > 
> > Ya, the concern is that it's theoretically possible for the PINV to arrive in L0
> > after a different vCPU has been loaded (or even multiple different vCPUs).
> > E.g. if the sending pCPU is hit with an NMI after checking vcpu->mode, and the
> > NMI runs for some absurd amount of time.  If that happens, the PINV handler
> > won't know which vCPU(s) should get an IRQ injected into L1 without additional
> > tracking.  KVM would need to set something like nested.pi_pending before doing
> > kvm_vcpu_trigger_posted_interrupt(), i.e. nothing really changes, it just gets
> > more complex.
> 
> Ah, gotcha.  What if IN_GUEST_MODE/OUTSIDE_GUEST_MODE was replaced by a
> generation count?  Then you reread vcpu->mode after sending the IPI, and
> retry if it does not match.
> 
> To guarantee atomicity, the OUTSIDE_GUEST_MODE IN_GUEST_MODE
> EXITING_GUEST_MODE READING_SHADOW_PAGE_TABLES values would remain in the
> bottom 2 bits.  That is, the vcpu->mode accesses like
> 
> 	vcpu->mode = IN_GUEST_MODE;
> 
> 	vcpu->mode = OUTSIDE_GUEST_MODE;
> 
> 	smp_store_mb(vcpu->mode, READING_SHADOW_PAGE_TABLES);
> 
> 	smp_store_release(&vcpu->mode, OUTSIDE_GUEST_MODE);
> 
> 	return cmpxchg(&vcpu->mode, IN_GUEST_MODE, EXITING_GUEST_MODE);
> 
> becoming
> 
> 	enum {
> 		OUTSIDE_GUEST_MODE,
> 		IN_GUEST_MODE,
> 		EXITING_GUEST_MODE,
> 		READING_SHADOW_PAGE_TABLES,
> 		GUEST_MODE_MASK = 3,
> 	};
> 
> 	vcpu->mode = (vcpu->mode | GUEST_MODE_MASK) + 1 + IN_GUEST_MODE;
> 
> 	vcpu->mode &= ~GUEST_MODE_MASK;
> 
> 	smp_store_mb(vcpu->mode, vcpu->mode|READING_SHADOW_PAGE_TABLES);
> 
> 	smp_store_release(&vcpu->mode, vcpu->mode & ~GUEST_MODE_MASK);
> 
> 	int x = READ_ONCE(vcpu->mode);
> 	do {
> 		if ((x & GUEST_MODE_MASK) != IN_GUEST_MODE)
> 			return x & GUEST_MODE_MASK;
> 	} while (!try_cmpxchg(&vcpu->mode, &x,
> 			      x ^ IN_GUEST_MODE ^ EXITING_GUEST_MODE))
> 	return IN_GUEST_MODE;
> 
> You could still get spurious posted interrupt IPIs, but the IPI handler need
> not do anything at all and that is much better.

This doesn't handle the case where the PINV arrives in L0 after VM-Exit but
before the vCPU clears IN_GUEST_MODE.  The sender will have seen IN_GUEST_MODE
and so won't retry the IPI, but hardware didn't process the PINV as a
posted-interrupt.  I.e. the L0 PINV handler still needs an indicator a la
nested.pi_pending to know that it should stuff an IRQ into L1's vIRR.

> > if we're ok with KVM
> > processing virtual interrupts that technically shouldn't happen, yet.  E.g. if
> > the L0 PINV handler consumes vIRR bits that were set after the last PI from L1.
> 
> I actually find it curious that the spec promises posted interrupt
> processing to be triggered only by the arrival of the posted interrupt IPI.
> Why couldn't the processor in principle snoop for the address of the ON bit
> instead, similar to an MWAIT?

It would lead to false positives and missed IRQs.  PI processing would fire on
writing the PI _cache line_, not just on writes to PI.ON.  I suspect MONITOR is
also triggered on request-for-EXLUSIVE and not just writes, i.e. on speculative
behavior, but I forget if that's actually the case.

Regardless, a write to any part of the PI would trip the monitoring, and then
all subsequent writes would be missed, e.g. other package writes PI.IRR then
PI.ON, CPU PI processing triggers on the PI.IRR write but not PI.ON write.  The
target CPU (the running vCPU) would have to constantly rearm the monitor, but
even then there would always be a window where a write would get missed.

> But even without that, I don't think the spec promises that kind of strict
> ordering with respect to what goes on in the source.  Even though posted
> interrupt processing is atomic with the acknowledgement of the posted
> interrupt IPI, the spec only promises that the PINV triggers an _eventual_
> scan of PID.PIR when the interrupt controller delivers an unmasked external
> interrupt to the destination CPU.  You can still have something like
> 
> 	set PID.PIR[100]
> 	set PID.ON
> 					processor starts executing a
> 					 very slow instruction...
> 	send PINV
> 	set PID.PIR[200]
> 					acknowledge PINV
> 
> and then vector 200 would be delivered before vector 100.  Of course with
> nested PI the effect would be amplified, but it's possible even on bare
> metal.

Jim was concerned that L1 could poll the PID to determine whether or not
PID.PIR[200] should be seen in L2.  The whole PIR is copied to the vIRR after
PID.ON is cleared the auto-EOI is done, and the read->clear is atomic.  So the
above sequence where PINV is acknowledge after PID.PIR[200] is legal, but
processing PIR bits that are set after the PIR is observed to be cleared would
be illegal.  E.g. if L1 did this

	set PID.PIR[100]
	set PID.ON
	send PINV
	while (PID.PIR)
	set PID.PIR[200]
	set PID.ON

then L2 should never observe vector 200.  KVM violates this because
nested.pi_pending is left set even if PINV is handled as a posted interrupt, and
KVM's processing of nested.pi_pending will see the second PID.ON and incorrectly
do PI processing in software.  This is the part that is likely impossible to
solve without shadowing the PID (which, for the record, I have zero desire to do).

It seems extremely unlikely any guest will puke on the above, I can't imagine
there's for setting a PID.PIR + PID.ON without triggering PINV, but it's
technically bad behavior in KVM.