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

[Tim Deegan <tim@xxxxxxx>]

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?

If that _is_ the case then the correct handling logic is much simpler:
don't touch any IOMMU registers at all in the interrupt handler; clear
the interrupt-pending bits in the tasklet.

In either case, the while () loop worries me; I think it would be better
to schedule the tasklet again if we see the bit is set; a 'while
(pending is set) { clear pending bit; }' loop might never exit the
tasklet at all.

Cheers,

Tim.

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