| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 8 Apr 2013 09:07:42 -0500
From: Suravee Suthikulanit <suravee.suthikulpanit@....com>
To: <suravee.suthikulpanit@....com>
CC: <iommu@...ts.linux-foundation.org>, <joro@...tes.org>,
<linux-kernel@...r.kernel.org>
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@....com wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@....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@....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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists