Messages in this thread | | | Date | Mon, 8 Apr 2013 09:07:42 -0500 | From | Suravee Suthikulanit <> | Subject | Re: [PATCH] iommu/amd: Add workaround to propery clearing IOMMU status register |
| |
Joerg,
Do you have any feedback about this patch?
Thanks,
Suravee
On 4/3/2013 6:19 PM, 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. > > In the current interrupt handling scheme, there are as many threads as > the number of IOMMUs. Each thread is created and assigned to an IOMMU at > the time of registering interrupt handlers (request_threaded_irq). > When an IOMMU HW generates an interrupt, the irq handler (top half) wakes up > the corresponding thread to process event and PPR logs of all IOMMUs > starting from the 1st IOMMU. > > In the system with multiple IOMMU,this handling scheme complicates the > synchronization of the IOMMU data structures and status registers as > there could be multiple threads competing for the same IOMMU while > the other IOMMU could be left unhandled. > > In order to simplify the implementation of the workaround, this patch is > proposing a different interrupt handling scheme by having each thread only > managing interrupts of the corresponding IOMMU. This can be achieved by passing > the struct amd_iommu when registering the interrupt handlers. This structure is > unique for each IOMMU and can be used by the bottom half thread to identify > the IOMMU to be handled instead of calling for_each_iommu. Besides this also > eliminate the needs to lock the IOMMU for processing event and PPR logs. > > Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> > --- > drivers/iommu/amd_iommu.c | 59 ++++++++++++++++++++-------------------- > drivers/iommu/amd_iommu_init.c | 2 +- > 2 files changed, 31 insertions(+), 30 deletions(-) > > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c > index e5db3e5..3548d63 100644 > --- a/drivers/iommu/amd_iommu.c > +++ b/drivers/iommu/amd_iommu.c > @@ -803,12 +803,6 @@ retry: > static void iommu_poll_events(struct amd_iommu *iommu) > { > u32 head, tail; > - unsigned long flags; > - > - /* enable event interrupts again */ > - writel(MMIO_STATUS_EVT_INT_MASK, iommu->mmio_base + MMIO_STATUS_OFFSET); > - > - spin_lock_irqsave(&iommu->lock, flags); > > head = readl(iommu->mmio_base + MMIO_EVT_HEAD_OFFSET); > tail = readl(iommu->mmio_base + MMIO_EVT_TAIL_OFFSET); > @@ -819,8 +813,6 @@ static void iommu_poll_events(struct amd_iommu *iommu) > } > > writel(head, iommu->mmio_base + MMIO_EVT_HEAD_OFFSET); > - > - spin_unlock_irqrestore(&iommu->lock, flags); > } > > static void iommu_handle_ppr_entry(struct amd_iommu *iommu, u64 *raw) > @@ -845,17 +837,11 @@ static void iommu_handle_ppr_entry(struct amd_iommu *iommu, u64 *raw) > > static void iommu_poll_ppr_log(struct amd_iommu *iommu) > { > - unsigned long flags; > u32 head, tail; > > if (iommu->ppr_log == NULL) > return; > > - /* enable ppr interrupts again */ > - writel(MMIO_STATUS_PPR_INT_MASK, iommu->mmio_base + MMIO_STATUS_OFFSET); > - > - spin_lock_irqsave(&iommu->lock, flags); > - > head = readl(iommu->mmio_base + MMIO_PPR_HEAD_OFFSET); > tail = readl(iommu->mmio_base + MMIO_PPR_TAIL_OFFSET); > > @@ -891,34 +877,49 @@ static void iommu_poll_ppr_log(struct amd_iommu *iommu) > head = (head + PPR_ENTRY_SIZE) % PPR_LOG_SIZE; > writel(head, iommu->mmio_base + MMIO_PPR_HEAD_OFFSET); > > - /* > - * Release iommu->lock because ppr-handling might need to > - * re-acquire it > - */ > - spin_unlock_irqrestore(&iommu->lock, flags); > - > /* Handle PPR entry */ > iommu_handle_ppr_entry(iommu, entry); > > - spin_lock_irqsave(&iommu->lock, flags); > - > /* Refresh ring-buffer information */ > head = readl(iommu->mmio_base + MMIO_PPR_HEAD_OFFSET); > tail = readl(iommu->mmio_base + MMIO_PPR_TAIL_OFFSET); > } > - > - spin_unlock_irqrestore(&iommu->lock, flags); > } > > irqreturn_t amd_iommu_int_thread(int irq, void *data) > { > - struct amd_iommu *iommu; > + struct amd_iommu *iommu = (struct amd_iommu *) data; > + u32 status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET); > > - for_each_iommu(iommu) { > - iommu_poll_events(iommu); > - iommu_poll_ppr_log(iommu); > - } > + while (status & (MMIO_STATUS_EVT_INT_MASK | MMIO_STATUS_PPR_INT_MASK)) { > + if (status & MMIO_STATUS_EVT_INT_MASK) { > + pr_devel("AMD-Vi: Processing IOMMU Event Log\n"); > + iommu_poll_events(iommu); > + } > > + if (status & MMIO_STATUS_PPR_INT_MASK) { > + pr_devel("AMD-Vi: Processing IOMMU PPR Log\n"); > + iommu_poll_ppr_log(iommu); > + } > + > + /* Enable EVT and PPR interrupts again */ > + writel((MMIO_STATUS_EVT_INT_MASK | MMIO_STATUS_PPR_INT_MASK), > + iommu->mmio_base + MMIO_STATUS_OFFSET); > + > + /* > + * Hardware bug: When re-enabling interrupt (by writing 1 > + * to clear the bit), the hardware might also try to set > + * the interrupt bit in the event status register. > + * In this scenario, the bit will be set, and disable > + * subsequent interrupts. > + * > + * Workaround: The IOMMU driver should read back the > + * status register and check if the interrupt bits are cleared. > + * If not, driver will need to go through the interrupt handler > + * again and re-clear the bits > + */ > + status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET); > + } > return IRQ_HANDLED; > } > > diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c > index e3c2d74..e4120bb 100644 > --- a/drivers/iommu/amd_iommu_init.c > +++ b/drivers/iommu/amd_iommu_init.c > @@ -1275,7 +1275,7 @@ static int iommu_setup_msi(struct amd_iommu *iommu) > amd_iommu_int_handler, > amd_iommu_int_thread, > 0, "AMD-Vi", > - iommu->dev); > + iommu); > > if (r) { > pci_disable_msi(iommu->dev);
| |