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

[Suravee Suthikulanit <suravee.suthikulpanit@amd.com>]

On 6/10/2013 8:59 AM, Jan Beulich wrote:
>>>> On 10.06.13 at 15:53, Suravee Suthikulanit <suravee.suthikulpanit@amd.com>
> wrote:
>> On 6/10/2013 5:53 AM, Jan Beulich wrote:
>>>>>> On 10.06.13 at 12:40, Tim Deegan <tim@xen.org> wrote:
>>>> At 10:47 +0100 on 10 Jun (1370861263), Jan Beulich wrote:
>>>>>>>> On 10.06.13 at 11:35, Tim Deegan <tim@xen.org> wrote:
>>>>>> At 00:05 -0500 on 10 Jun (1370822751), suravee.suthikulpanit@amd.com wrote:
>>>>>>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>>>>>>
>>>>>>> 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
>
I am sure this is what the behavior of the hardware.  Besides, only the 
hardware can set this bit.
I have also tested by not clearing the bit, and basically I did not see 
additional interrupts.

Suravee