Re: [PATCH v6 5/5] KVM : VMX: Use posted interrupt to deliver virtual interrupt

[Gleb Natapov <gleb@redhat.com>]

On Tue, Mar 19, 2013 at 04:51:04PM +0200, Gleb Natapov wrote:
> On Tue, Mar 19, 2013 at 01:59:21PM +0000, Zhang, Yang Z wrote:
> > Gleb Natapov wrote on 2013-03-19:
> > > On Tue, Mar 19, 2013 at 12:42:01PM +0000, Zhang, Yang Z wrote:
> > >>>>>>  	local_irq_disable();
> > >>>>>> +	kvm_x86_ops->posted_intr_clear_on(vcpu);
> > >>>>>> +
> > >>>>> Why is this separate from pir_to_irr syncing?
> > >>>> This is the result of discussion with Marcelo. It is more reasonable to
> > >>>> put it here to avoid unnecessary posted interrupt between:
> > >>>> 
> > >>>> vcpu->mode = IN_GUEST_MODE;
> > >>>> 
> > >>>> <--interrupt may arrived here and this is unnecessary.
> > >>>> 
> > >>>> local_irq_disable();
> > >>>> 
> > >>> 
> > >>> But this still can happen as far as I see:
> > >>> 
> > >>> vcpu0                                         vcpu1:
> > >>> pi_test_and_set_pir() kvm_make_request(KVM_REQ_EVENT)
> > >>>                                             if (KVM_REQ_EVENT)
> > >>>                                                    sync_pir_to_irr()
> > >>>                                             vcpu->mode =
> > >>> IN_GUEST_MODE;
> > >>> if (vcpu->mode == IN_GUEST_MODE)
> > >>>   if (!pi_test_and_set_on())
> > >>>     apic->send_IPI_mask()
> > >>>                                             --> IPI arrives here
> > >>>                                             local_irq_disable();
> > >>>                                             posted_intr_clear_on()
> > >> Current solution is trying to block other Posted Interrupt from other VCPUs at
> > > same time. It only mitigates it but cannot solve it. The case you mentioned still
> > > exists but it should be rare.
> > >> 
> > > I am not sure I follow. What scenario exactly are you talking about. I
> > > looked over past discussion about it and saw that Marcelo gives an
> > > example how IPI can be lost, but I think that's because we set "on" bit
> > > after KVM_REQ_EVENT:
> > The IPI will not lost in his example(he misread the patch). 
> > 
> > > cpu0                                    cpu1            vcpu0
> > > test_and_set_bit(PIR-A) set KVM_REQ_EVENT
> > >                                                         process
> > >                                                         REQ_EVENT
> > >                                                         PIR-A->IRR
> > > 
> > > vcpu->mode=IN_GUEST
> > > 
> > > if (vcpu0->guest_mode)
> > >         if (!t_a_s_bit(PIR notif))
> > >                 send IPI
> > > linux_pir_handler
> > > 
> > >                                         t_a_s_b(PIR-B)=1
> > >                                         no PIR IPI sent
> > > 
> > > But what if on delivery we do:
> > > pi_test_and_set_pir()
> > > r = pi_test_and_set_on()
> > > kvm_make_request(KVM_REQ_EVENT)
> > > if (!r)
> > >    send_IPI_mask() else kvm_vcpu_kick()
> > > And on vcpu entry we do:
> > > if (kvm_check_request(KVM_REQ_EVENT)
> > >  if (test_and_clear_bit(on))
> > >    kvm_apic_update_irr()
> > > What are the downsides? Can we lost interrupts this way?
> > Need to check guest mode before sending IPI. Otherwise hypervisor may receive IPI.
> Of course, forget it. So if (!r) should be if (!r && mode == IN_GUEST)
> 
> > I think current logic is ok. Only problem is that when to clear Outstanding Notification bit. Actually I prefer your suggestion to clear it before sync_pir_irr. But Marcelo prefer to clear ON bit after disabling irq.
> Marcelo what is the problem with the logic above?
> 
Just to clarify the advantages that I see are: one less callback, no
need to sync pir to irr on each event and, arguably, a little bit
simpler logic.

--
			Gleb.