RE: [PATCH 2/2] x86, apicv: Add Posted Interrupt supporting

Zhang, Yang Z
2013-02-05T05:57:18Z

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.
The only problem is the step 6, but at that point, there already an interrupt pending in IRR. This means the interrupt will be handled not lost. And even in real hardware, this case do exist. So I think it should not be a problem.

Best regards,
Yang