Re: [PATCH] KVM: nVMX: Fix races when sending nested PI while dest enters/leaves L2

["Radim Krčmář" <rkrcmar@redhat.com>]

2017-11-10 22:40+0200, Liran Alon:
> On 10/11/17 20:06, Radim Krčmář wrote:
> > 2017-11-10 18:06+0100, Paolo Bonzini:
> > > On 09/11/2017 19:27, Liran Alon wrote:
> > > 		/*
> > > 		 * 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);
> > > 		kvm_vcpu_kick(vcpu);
> > > 	}
> > > 
> > > See the comments around the setting of IN_GUEST_MODE, introduced by
> > > commit b95234c84004 ("kvm: x86: do not use KVM_REQ_EVENT for APICv
> > > interrupt injection", 2017-02-15).
> > > 
> > > Even though nested PI must use KVM_REQ_EVENT, the reasoning behind the
> > > ordering of cli and vcpu->mode = IN_GUEST_MODE should hold in both cases.
> > 
> > I think the fix is correct, but the code is using very awkward
> > synchronization through vmx->nested.pi_pending and slaps KVM_REQ_EVENT
> > on top.
> > If we checked vmx->nested.pi_pending after cli, we could get rid of
> > KVM_REQ_EVENT and if we want to support nested posted interrupts from
> > PCI devices, we should explicitly check for pi.on during VM entry.
> I like this suggestion. If I understand correctly, these are the changes I
> will do for v2 of this patch:
> 
> 1. I Will change vmx_deliver_nested_posted_interrupt() to first set
> pi_pending=true and then call kvm_vcpu_trigger_posted_interrupt(). No need
> to set KVM_REQ_EVENT and no need to check
> kvm_vcpu_trigger_posted_interrupt() ret-val.
> 
> 2. I will create a kvm_x86_ops->complete_nested_posted_interrupt() that will
> be called from vcpu_enter_guest() just after sync_pir_to_irr() is called but
> outside the "if" for kvm_lapic_enabled().

I think that we can't have nested posted interrupts without
kvm_lapic_enabled() or kvm_vcpu_apicv_active(), so we could add nested
handling to sync_pir_to_irr and save x86 op.

vmx_complete_nested_posted_interrupt will need some changes as it is not
called with disabled interrupts -- kmap() and probably also
nested_mark_vmcs12_pages_dirty() should be done outside;  I guess that
other code is already be taking care of both, but better check.

> 3. I will remove call to vmx_complete_nested_posted_interrupt() from
> vmx_check_nested_events(). That call-site had a bug anyway that I fixed in
> another patch-series I posted here. This change will make that patch (not
> the series) redundant. See redundant patch here:
> https://marc.info/?l=kvm&m=150989090007281&w=2

I'll ignore that patch,

> I will prepare a v2 patch by this suggestion and send it.

thanks.