On Tue, Mar 19, 2013 at 12:19:55PM -0300, Marcelo Tosatti wrote:
> On Tue, Mar 19, 2013 at 05:12:55PM +0200, Gleb Natapov wrote:
> > 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.
>
> See the previous argument: should never enter guest mode with PIR ON bit
> set. With logic above:
>
> context1 context2 context3
> set_bit(PIR-1)
> r = pi_test_and_set_on()
> set_bit(PIR-40)
> set_bit(KVM_REQ_EVENT)
> if (kvm_check_request(KVM_REQ_EVENT)
> if (test_and_clear_bit(on))
>
> kvm_apic_update_irr() r =
> pi_test_and_set_on()
>
> guest entry with PIR ON=1
>
>
But that's OK since context3 will send IPI and PIR will be processed by
CPU. Let me amend myself: we should not enter guest mode with ON bit set
and no PI IPI pending in LAPIC.
> Thats the reason for unconditional clearing on guest entry: it is easy
> to verify its correct. I understand and agree the callback (and VMWRITE)
> is not nice.
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
