Marcelo Tosatti wrote on 2013-02-25: > On Sun, Feb 24, 2013 at 01:17:59PM +0000, Zhang, Yang Z wrote: >> Marcelo Tosatti wrote on 2013-02-24: >>> On Sat, Feb 23, 2013 at 03:16:11PM +0000, Zhang, Yang Z wrote: >>>> Marcelo Tosatti wrote on 2013-02-23: >>>>> On Sat, Feb 23, 2013 at 02:05:28PM +0000, Zhang, Yang Z wrote: >>>>>> Marcelo Tosatti wrote on 2013-02-23: >>>>>>> >>>>>>> On Fri, Feb 22, 2013 at 09:42:32PM +0800, Yang Zhang wrote: >>>>>>>> From: Yang Zhang <yang.z.zh...@intel.com> >>>>>>>> >>>>>>>> Posted Interrupt allows APIC interrupts to inject into guest directly >>>>>>>> without any vmexit. >>>>>>>> >>>>>>>> - When delivering a interrupt to guest, if target vcpu is running, >>>>>>>> update Posted-interrupt requests bitmap and send a notification >>>>>>>> event to the vcpu. Then the vcpu will handle this interrupt >>>>>>>> automatically, without any software involvemnt. - If target vcpu is >>>>>>>> not running or there already a notification event pending in the >>>>>>>> vcpu, do nothing. The interrupt will be handled by next vm entry >>>>>>>> Signed-off-by: Yang Zhang <yang.z.zh...@intel.com> >>>>>>>> --- >>>>>>>> >>>>>>>> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c >>>>>>>> index e4595f1..64616cc 100644 >>>>>>>> --- a/arch/x86/kernel/irq.c >>>>>>>> +++ b/arch/x86/kernel/irq.c >>>>>>>> @@ -228,6 +228,26 @@ void smp_x86_platform_ipi(struct pt_regs > *regs) >>>>>>>> set_irq_regs(old_regs); >>>>>>>> } >>>>>>>> +#ifdef CONFIG_HAVE_KVM >>>>>>>> +/* >>>>>>>> + * Handler for POSTED_INTERRUPT_VECTOR. >>>>>>>> + */ >>>>>>>> +void smp_kvm_posted_intr_ipi(struct pt_regs *regs) >>>>>>>> +{ >>>>>>>> + struct pt_regs *old_regs = set_irq_regs(regs); >>>>>>>> + >>>>>>>> + ack_APIC_irq(); >>>>>>>> + >>>>>>>> + irq_enter(); >>>>>>>> + >>>>>>>> + exit_idle(); >>>>>>>> + >>>>>>>> + irq_exit(); >>>>>>>> + >>>>>>>> + set_irq_regs(old_regs); >>>>>>>> +} >>>>>>>> +#endif >>>>>>>> + >>>>>>> >>>>>>> Add per-cpu counters, similarly to other handlers in the same file. >>>>>> Sure. >>>>>> >>>>>>>> @@ -379,6 +398,7 @@ static inline int apic_find_highest_irr(struct >>> kvm_lapic >>>>>>> *apic) >>>>>>>> if (!apic->irr_pending) return -1; >>>>>>>> + kvm_x86_ops->sync_pir_to_irr(apic->vcpu); result = >>>>>>>> apic_search_irr(apic); ASSERT(result == -1 || result >= 16); >>>>>>> >>>>>>> kvm_x86_ops->sync_pir_to_irr() should be called from >>>>>>> vcpu_enter_guest, before inject_pending_event. >>>>>>> >>>>>>> if (kvm_check_request(KVM_REQ_EVENT, vcpu) || > req_int_win) >>> { >>>>>>> -> >>>>>>> inject_pending_event >>>>>>> ... >>>>>>> } >>>>>> Some other places will search irr to do something, like >>>>> kvm_cpu_has_interrupt(). So we must to sync pir to irr before touch >>>>> irr, not just before enter guest. >>>>> >>>>> I see. The only call site that needs IRR/PIR information with posted >>>>> interrupt enabled is kvm_arch_vcpu_runnable, correct? >>>> Yes. >>>> >>>>> If so, can we contain reading PIR to only that location? >>>> Yes, we can do it. Actually, why I do pir->irr here is to avoid the >>>> wrong usage in future of check pending interrupt just by call >>>> kvm_vcpu_has_interrupt(). >>> >>> Yes, i see. >>> >>>> Also, there may need an extra callback to check pir. >>>> So you think your suggestions is better? >>> >>> Because it respects standard request processing mechanism, yes. >> Sure. Will change it in next patch. >> >>>>> And have PIR->IRR sync at KVM_REQ_EVENT processing only (to respect >>>>> the standard way of event processing and also reduce reading the >>>>> PIR). >>>> Since we will check ON bit before each reading PIR, the cost can be >>>> ignored. >>> >>> Note reading ON bit is not OK, because if injection path did not see >>> vcpu->mode == IN_GUEST_MODE, ON bit will not be set. >> In my patch, It will set ON bit before check vcpu->mode. So it's ok. >> Actually, I think the ON bit must be set unconditionally after change >> PIR regardless of vcpu->mode. >> >>>>> >>>>> So it is possible that a PIR IPI is delivered and handled before guest >>>>> entry. >>>>> >>>>> By clearing PIR notification bit after local_irq_disable, but before the >>>>> last check for vcpu->requests, we guarantee that a PIR IPI is never lost. >>>>> >>>>> Makes sense? (please check the logic, it might be flawed). >>>> I am not sure whether I understand you. But I don't think the IPI will >>>> lost in current patch: >>>> >>>> if (!pi_test_and_set_on(&vmx->pi_desc) && (vcpu->mode == >>> IN_GUEST_MODE)) { >>>> kvm_make_request(KVM_REQ_EVENT, vcpu); >>>> apic->send_IPI_mask(get_cpu_mask(vcpu->cpu), >>>> POSTED_INTR_VECTOR); >>>> *delivered = true; >>>> } >>>> >>>> vcpu entry has: >>>> vcpu->mode = GUEST_MODE >>>> local irq disable >>>> check request >>>> >>>> We will send the IPI after making request, if IPI is received after set >>> guest_mode before disable irq, then it still will be handled by the >>> following check request. >>>> Please correct me if I am wrong. > > p1) > >>> 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 > > p2) > >> No, this exists with your implementation not mine. >> As I said, in this patch, it will make request after vcpu ==guest mode, then >> send > ipi. Even the ipi is lost, but the request still will be tracked. Because we > have the > follow check: >> vcpu->mode = GUEST_MODE >> (ipi may arrived here and lost) >> local irq disable >> check request (this will ensure the pir modification will be picked up by >> vcpu > before vmentry) > > Please read the sequence above again, between p1) and p2). Yes, if the > IPI is lost, the request _for the CPU which sent PIR IPI_ is guaranteed > to be processed, but not the request for another cpu (cpu1). If no PIR IPI sent in cpu1, the delivered is false, and it will roll back to old logic: __apic_accept_irq(): if (!delivered) { kvm_make_request(KVM_REQ_EVENT, vcpu); kvm_vcpu_kick(vcpu); } This can solve the problem you mentioned above. Right?
> If in fact the scenario is not possible, then please point out between > p1) and p2) where the error in representation is. > > Note there are 3 contexes: CPU0, CPU1, VCPU0 (virtual CPU on some CPU != 0 > and != > 1). >>>>> Sync PIR->IRR uses xchg, which is locked and atomic, so can't see the >>>>> need for the spinlock there. >>>> In function sync_pir_to_irr(): >>>> tmp_pir=xchg(pir,0) >>>> xchg(tmp_pir, irr) >>>> >>>> It is not atomically, the lock still is needed. >>> >>> IRR is only accessed by local vcpu context, only PIR is accessed >>> concurrently, >>> therefore only PIR accesses must be atomic. xchg instruction is >>> locked and atomic. >> There has same problem as we discussed before. Consider this: >> Before delivery poste ipi, the irr is 0. then: >> >> cpu0 cpu1 vcpu0 >> delivery_posted_intr() >> sync_pir_toirr(): >> tmp_pir=xchg(pir,0)(pir is cleared) >> check pir(pass) >> check irr(pass) >> xchg(tmp_pir, irr) >> injected= true >> set pir >> >> Same problem: both pir and irr is set, but it reported as injected. Still >> need the > lock to protect it. > > See: > > cpu0 > > check pir(pass) > check irr(pass) > injected = !test_and_set_bit(pir) > > versus > > cpu1 >xchg(pir) > > No information can be lost because all accesses to shared data are > atomic. Sorry, I cannot get your point. Why 'xchg(pir)' can solve the problem I mentioned above? Can you elaborate it? The spinlock I used is trying to protect the whole procedure of sync pir to irr, not just access pir. Best regards, Yang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html