linux-kernel - Re: [PART2 PATCH v5 06/12] iommu/amd: Adding GALOG interrupt handler

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160809144347.GC1437@8bytes.org>
Date:	Tue, 9 Aug 2016 16:43:47 +0200
From:	Joerg Roedel <joro@...tes.org>
To:	Suravee Suthikulpanit <suravee.suthikulpanit@....com>
Cc:	rkrcmar@...hat.com, pbonzini@...hat.com,
	alex.williamson@...hat.com, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org, sherry.hurwitz@....com
Subject: Re: [PART2 PATCH v5 06/12] iommu/amd: Adding GALOG interrupt handler

On Mon, Jul 25, 2016 at 04:32:05AM -0500, Suthikulpanit, Suravee wrote:
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
> 
> This patch adds AMD IOMMU guest virtual APIC log (GALOG) handler.
> When IOMMU hardware receives an interrupt targeting a blocking vcpu,
> it creates an entry in the GALOG, and generates an interrupt to notify
> the AMD IOMMU driver.
> 
> At this point, the driver processes the log entry, and notify the SVM
> driver via the registered iommu_ga_log_notifier function.
> 
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
> ---
>  drivers/iommu/amd_iommu.c | 77 +++++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/amd-iommu.h | 20 ++++++++++--
>  2 files changed, 91 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index abfb2b7..861d723 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -741,14 +741,78 @@ static void iommu_poll_ppr_log(struct amd_iommu *iommu)
>  	}
>  }
>  
> +#ifdef CONFIG_IRQ_REMAP
> +static int (*iommu_ga_log_notifier)(u32);
> +
> +int amd_iommu_register_ga_log_notifier(int (*notifier)(u32))
> +{
> +	iommu_ga_log_notifier = notifier;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(amd_iommu_register_ga_log_notifier);
> +
> +static void iommu_poll_ga_log(struct amd_iommu *iommu)
> +{
> +	u32 head, tail, cnt = 0;
> +
> +	if (iommu->ga_log == NULL)
> +		return;
> +
> +	head = readl(iommu->mmio_base + MMIO_GA_HEAD_OFFSET);
> +	tail = readl(iommu->mmio_base + MMIO_GA_TAIL_OFFSET);
> +
> +	while (head != tail) {
> +		volatile u64 *raw;
> +		u64 log_entry;
> +
> +		raw = (u64 *)(iommu->ga_log + head);
> +		cnt++;
> +
> +		/* Avoid memcpy function-call overhead */
> +		log_entry = *raw;
> +
> +		/* Update head pointer of hardware ring-buffer */
> +		head = (head + GA_ENTRY_SIZE) % GA_LOG_SIZE;
> +		writel(head, iommu->mmio_base + MMIO_GA_HEAD_OFFSET);
> +
> +		/* Handle GA entry */
> +		switch (GA_REQ_TYPE(log_entry)) {
> +		case GA_GUEST_NR:
> +			if (!iommu_ga_log_notifier)
> +				break;
> +
> +			pr_debug("AMD-Vi: %s: devid=%#x, ga_tag=%#x\n",
> +				 __func__, GA_DEVID(log_entry),
> +				 GA_TAG(log_entry));
> +
> +			if (iommu_ga_log_notifier(GA_TAG(log_entry)) != 0)
> +				pr_err("AMD-Vi: GA log notifier failed.\n");
> +			break;
> +		default:
> +			break;
> +		}
> +
> +		/* Refresh ring-buffer information */
> +		head = readl(iommu->mmio_base + MMIO_GA_HEAD_OFFSET);
> +		tail = readl(iommu->mmio_base + MMIO_GA_TAIL_OFFSET);

Couldn't that cause an endless-loop in case of an interrupt storm from a
device? I think it is better to just read head and tail once before the
loop and update head after we get out of the loop. Any new entries
could be handled by the next iommu interrupt. This avoids any
soft-lockups that might happen when the loop runs for too long.