Marcelo Tosatti wrote on 2013-02-25:
> On Mon, Feb 25, 2013 at 06:55:58AM +0000, Zhang, Yang Z wrote:
>>>
>>> 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?
>
> Should not be doing this kick in the first place. One result of it is
> that you'll always take a VM-exit if a second injection happens while a VCPU
> has not handled the first one.
You are right.
> What is the drawback of the suggestion to unconditionally clear PIR
> notification bit on VM-entry?
The only thing is we need to traverse the whole pir when calling sync_pir_to_irr even the pir is empty.
>
> We can avoid it, but lets get it correct first (BTW can stick a comment
> saying FIXME: optimize) on that PIR clearing.
Ok, I will adopt your suggestion. BTW, Where should the comment be add? on sync_pir_to_irr()?
>
>>> 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.
>
> You're right, its the same problem as with the hardware update.
Best regards,
Yang
--
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
