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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
