Re: [Xen-devel] [PATCH 2/2 V2] iommu/amd: Workaround for erratum 787

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.06.13 at 15:53, Suravee Suthikulanit <suravee.suthikulpanit@xxxxxxx>
wrote:
> On 6/10/2013 5:53 AM, Jan Beulich wrote:
>>>>> On 10.06.13 at 12:40, Tim Deegan <tim@xxxxxxx> wrote:
>>> At 10:47 +0100 on 10 Jun (1370861263), Jan Beulich wrote:
>>>>>>> On 10.06.13 at 11:35, Tim Deegan <tim@xxxxxxx> wrote:
>>>>> At 00:05 -0500 on 10 Jun (1370822751), suravee.suthikulpanit@xxxxxxx 
>>>>> wrote:
>>>>>> From: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
>>>>>>
>>>>>> The IOMMU interrupt handling in bottom half must clear the PPR log
>>> interrupt
>>>>>> and event log interrupt bits to re-enable the interrupt. This is done by
>>>>>> writing 1 to the memory mapped register to clear the bit. Due to hardware
>>>>> bug,
>>>>>> if the driver tries to clear this bit while the IOMMU hardware also 
>>>>>> setting
>>>>>> this bit, the conflict will result with the bit being set. If the 
>>>>>> interrupt
>>>>>> handling code does not make sure to clear this bit, subsequent changes in
>>>>> the
>>>>>> event/PPR logs will no longer generating interrupts, and would result if
>>>>>> buffer overflow. After clearing the bits, the driver must read back
>>>>>> the register to verify.
>>>>> Is there a risk of livelock here?  That is, if some device is causing a
>>>>> lot of IOMMU faults, a CPU could get stuck in this loop re-enabling
>>>>> interrupts as fast as they are raised.
>>>>>
>>>>> The solution suggested in the erratum seems better: if the bit is set
>>>>> after clearing, process the interrupts again (i.e. run/schedule the
>>>>> top-half handler).  That way the bottom-half handler will definitely
>>>>> terminate and the system can make some progress.
>>>> That's what's being done really: The actual interrupt handler disables
>>>> the interrupt sources, and the tasklet re-enables them (or at least is
>>>> supposed to do so - patch 1 isn't really correct in the respect).
>>> Oh I see, the idea is that we use the control register to mask
>>> interrupts instead of relying on the status register?  That seems
>>> better.  But doesn't this IOMMU already mask further interrupts when the
>>> pending bit in the status register is set?  I can't see any wording
>>> about that in the IOMMU doc but the erratum implies it.  Suravee, do you
>>> know if this is the case?
>> Actually, the documentation has a subtle but perhaps important
>> difference int the wording here: For EventLogInt and ComWaitInt
>> is read "An interrupt is generated when <status bit> = 1b and MMIO
>> Offset 0018h[<control bit>] = 1b", where as for PPRInt and GAInt
>> it says "An interrupt is generated when <status bit> changes from 0b
>> to 1b and MMIO Offset 0018h[<control bit>] = 1b".
>>
>> So I'd like to be one the safe side and assume further interrupts can
>> be generated in all cases - see also the emulation behavior in
>> iommu_guest.c which - afaict - always raises an interrupt, not just on
>> a 0 -> 1 transition.
> 
> The behavior should be that the interrupt will be generated if the "xxxInt"
> bit is 0.  Once generated, it will be set to "1" by the hardware.  If this
> bit is 1, events will be added to the log but interrupt will not be 
> generated.

"Should be" isn't enough here, even more so given the mentioned
wording differences in your documentation. We need to know how
actual (past, current, and future) hardware behaves.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel