Gleb Natapov wrote on 2013-02-05:
> On Tue, Feb 05, 2013 at 01:26:42PM +0000, Zhang, Yang Z wrote:
>> Gleb Natapov wrote on 2013-02-05:
>>> On Tue, Feb 05, 2013 at 10:58:28AM +0000, Zhang, Yang Z wrote:
>>>> Gleb Natapov wrote on 2013-02-05:
>>>>> On Tue, Feb 05, 2013 at 10:35:55AM +0000, Zhang, Yang Z wrote:
>>>>>> Gleb Natapov wrote on 2013-02-05:
>>>>>>> On Tue, Feb 05, 2013 at 05:57:14AM +0000, Zhang, Yang Z wrote:
>>>>>>>> Marcelo Tosatti wrote on 2013-02-05:
>>>>>>>>> On Mon, Feb 04, 2013 at 05:59:52PM -0200, Marcelo Tosatti wrote:
>>>>>>>>>> On Mon, Feb 04, 2013 at 07:13:01PM +0200, Gleb Natapov wrote:
>>>>>>>>>>> On Mon, Feb 04, 2013 at 12:43:45PM -0200, Marcelo Tosatti wrote:
>>>>>>>>>>>>>> Any example how software relies on such
>>>>>>>>> two-interrupts-queued-in-IRR/ISR behaviour?
>>>>>>>>>>>>> Don't know about guests, but KVM relies on it to detect
>>>>>>>>>>>>> interrupt coalescing. So if interrupt is set in IRR but not in
>>>>>>>>>>>>> PIR interrupt will not be reported as coalesced, but it will
>>>>>>>>>>>>> be coalesced during PIR->IRR merge.
>>>>>>>>>>>>
>>>>>>>>>>>> Yes, so:
>>>>>>>>>>>>
>>>>>>>>>>>> 1. IRR=1, ISR=0, PIR=0. Event: set_irq, coalesced=no.
>>>>>>>>>>>> 2. IRR=0, ISR=1, PIR=0. Event: IRR->ISR transfer.
>>>>>>>>>>>> 3. vcpu outside of guest mode.
>>>>>>>>>>>> 4. IRR=1, ISR=1, PIR=0. Event: set_irq, coalesced=no.
>>>>>>>>>>>> 5. vcpu enters guest mode.
>>>>>>>>>>>> 6. IRR=1, ISR=1, PIR=1. Event: set_irq, coalesced=no.
>>>>>>>>>>>> 7. HW transfers PIR into IRR.
>>>>>>>>>>>>
>>>>>>>>>>>> set_irq return value at 7 is incorrect, interrupt event was _not_
>>>>>>>>>>>> queued.
>>>>>>>>>>> Not sure I understand the flow of events in your description
>>>>>>>>>>> correctly. As I understand it at 4 set_irq() will return incorrect
>>>>>>>>>>> result. Basically when PIR is set to 1 while IRR has 1 for the
>>>>>>>>>>> vector the value of set_irq() will be incorrect.
>>>>>>>>>>
>>>>>>>>>> At 4 it has not been coalesced: it has been queued to IRR.
>>>>>>>>>> At 6 it has been coalesced: PIR bit merged into IRR bit.
>>>>>>>>>>
>>>>>>>>>>> Frankly I do not see how it can be fixed
>>>>>>>>>>> without any race with present HW PIR design.
>>>>>>>>>>
>>>>>>>>>> At kvm_accept_apic_interrupt, check IRR before setting PIR bit,
>>>>>>>>>> if IRR already set, don't set PIR.
>>>>>>>>>
>>>>>>>>> Or:
>>>>>>>>>
>>>>>>>>> apic_accept_interrupt() {
>>>>>>>>>
>>>>>>>>> 1. Read ORIG_PIR=PIR, ORIG_IRR=IRR.
>>>>>>>>> Never set IRR when HWAPIC enabled, even if outside of guest mode.
>>>>>>>>> 2. Set PIR and let HW or SW VM-entry transfer it to IRR.
>>>>>>>>> 3. set_irq return value: (ORIG_PIR or ORIG_IRR set).
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> Two or more concurrent set_irq can race with each other, though. Can
>>>>>>>>> either document the race or add a lock.
>>>>>>>> According the SDM, software should not touch the IRR when target
> vcpu
>>> is
>>>>>>> running. Instead, use locked way to access PIR. So your solution may
>>>>>>> wrong. Then your apicv patches are broken, because they do exactly
>>>>>>> that.
>>>>>> Which code is broken?
>>>>>>
>>>>> The one that updates IRR directly on the apic page.
>>>> No, all the updates are ensuring the target vcpu is not running. So
>>>> it's safe to touch IRR.
>>>>
>>> Not at all. Read the code.
>> Sorry. I still cannot figure out which code is wrong. All the places
>> call sync_pir_to_irr() are on target vcpu. Can you point out the code?
>> Thanks.
>>
> I am taking about vapic patches which are already in, not pir patches.
Yes, but the issue will be fixed with pir patches. With posted interrupt, it
will touch PIR instead IRR and access PIR is allowed by HW.
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
