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

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

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.

Suravee