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 12:13:11PM -0300, Marcelo Tosatti wrote:
> On Tue, Mar 19, 2013 at 03:29:24PM +0200, Gleb Natapov wrote:
> > 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:
> > 
> > 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?
> 
> You should not ever enter guest mode with ON bit set on PIR (because that will
> prevent PIR IPI from waking up interrupt injection logic). This is why
> the ON bit should be cleared on VM entry.
I agree that we should not, but I claim that this will not happen since
KVM_REQ_EVENT will prevent guest entry with ON bit set.

> 
> > > > May be move vcpu->mode = IN_GUEST_MODE after local_irq_disable()?
> > > Yes, this will solve it. But I am not sure whether it will introduce any regressions. Is there any check relies on this sequence?
> > >  	
> > Do not think so.
> 
> Can't see what is the problem with the code as it is?
> 
Just the useless IPI that can be prevented. I agree that this is not a
big deal unless shown otherwise.

--
			Gleb.