From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulanit Subject: Re: [PATCH 2/2 V2] iommu/amd: Workaround for erratum 787 Date: Mon, 10 Jun 2013 10:11:57 -0500 Message-ID: <51B5ECBD.4070508@amd.com> References: <1370840751-11277-1-git-send-email-suravee.suthikulpanit@amd.com> <1370840751-11277-2-git-send-email-suravee.suthikulpanit@amd.com> <20130610093532.GA8802@ocelot.phlegethon.org> <51B5BCDF02000078000DC94E@nat28.tlf.novell.com> <20130610104008.GC8802@ocelot.phlegethon.org> <51B5CC4902000078000DC9A1@nat28.tlf.novell.com> <51B5DA3C.2040606@amd.com> <51B5F7CA02000078000DCB26@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51B5F7CA02000078000DCB26@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Tim Deegan , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 6/10/2013 8:59 AM, Jan Beulich wrote: >>>> On 10.06.13 at 15:53, Suravee Suthikulanit > wrote: >> On 6/10/2013 5:53 AM, Jan Beulich wrote: >>>>>> On 10.06.13 at 12:40, Tim Deegan wrote: >>>> At 10:47 +0100 on 10 Jun (1370861263), Jan Beulich wrote: >>>>>>>> On 10.06.13 at 11:35, Tim Deegan wrote: >>>>>> At 00:05 -0500 on 10 Jun (1370822751), suravee.suthikulpanit@amd.com wrote: >>>>>>> From: Suravee Suthikulpanit >>>>>>> >>>>>>> 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 = 1b and MMIO >>> Offset 0018h[] = 1b", where as for PPRInt and GAInt >>> it says "An interrupt is generated when changes from 0b >>> to 1b and MMIO Offset 0018h[] = 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