[PATCH 0/5] AMD IOMMU Fixes for v4.2-rc4

[PATCH 0/5] AMD IOMMU Fixes for v4.2-rc4

[Joerg Roedel]

Hi,

here are a couple of fixes for the AMD IOMMU driver for
issues found recently. The issues were introduced by the
default-domain conversion in this development cycle. I plan
to send these fixes upstream by the end of the week.

Please review!

Thanks,

	Joerg

Joerg Roedel (5):
  iommu/amd: Use iommu_attach_group()
  iommu/amd: Use iommu core for passthrough mode
  iommu/amd: Allow non-IOMMUv2 devices in IOMMUv2 domains
  iommu/amd: Use swiotlb in passthrough mode
  iommu/amd: Set global dma_ops if swiotlb is disabled

 drivers/iommu/amd_iommu.c      | 91 +++++++++++-------------------------------
 drivers/iommu/amd_iommu_init.c | 10 +----
 drivers/iommu/amd_iommu_v2.c   | 24 +++++++++--
 3 files changed, 45 insertions(+), 80 deletions(-)

-- 
1.9.1

[PATCH 1/5] iommu/amd: Use iommu_attach_group()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Since the conversion to default domains the
iommu_attach_device function only works for devices with
their own group. But this isn't always true for current
IOMMUv2 capable devices, so use iommu_attach_group instead.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd_iommu_v2.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd_iommu_v2.c b/drivers/iommu/amd_iommu_v2.c
index 3465faf..f7b875b 100644
--- a/drivers/iommu/amd_iommu_v2.c
+++ b/drivers/iommu/amd_iommu_v2.c
@@ -132,11 +132,19 @@ static struct device_state *get_device_state(u16 devid)
 
 static void free_device_state(struct device_state *dev_state)
 {
+	struct iommu_group *group;
+
 	/*
 	 * First detach device from domain - No more PRI requests will arrive
 	 * from that device after it is unbound from the IOMMUv2 domain.
 	 */
-	iommu_detach_device(dev_state->domain, &dev_state->pdev->dev);
+	group = iommu_group_get(&dev_state->pdev->dev);
+	if (WARN_ON(!group))
+		return;
+
+	iommu_detach_group(dev_state->domain, group);
+
+	iommu_group_put(group);
 
 	/* Everything is down now, free the IOMMUv2 domain */
 	iommu_domain_free(dev_state->domain);
@@ -731,6 +739,7 @@ EXPORT_SYMBOL(amd_iommu_unbind_pasid);
 int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
 {
 	struct device_state *dev_state;
+	struct iommu_group *group;
 	unsigned long flags;
 	int ret, tmp;
 	u16 devid;
@@ -776,10 +785,16 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
 	if (ret)
 		goto out_free_domain;
 
-	ret = iommu_attach_device(dev_state->domain, &pdev->dev);
-	if (ret != 0)
+	group = iommu_group_get(&pdev->dev);
+	if (!group)
 		goto out_free_domain;
 
+	ret = iommu_attach_group(dev_state->domain, group);
+	if (ret != 0)
+		goto out_drop_group;
+
+	iommu_group_put(group);
+
 	spin_lock_irqsave(&state_lock, flags);
 
 	if (__get_device_state(devid) != NULL) {
@@ -794,6 +809,9 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
 
 	return 0;
 
+out_drop_group:
+	iommu_group_put(group);
+
 out_free_domain:
 	iommu_domain_free(dev_state->domain);
 
-- 
1.9.1

[PATCH 2/5] iommu/amd: Use iommu core for passthrough mode

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Remove the AMD IOMMU driver implementation for passthrough
mode and rely on the new iommu core features for that.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd_iommu.c      | 58 ++----------------------------------------
 drivers/iommu/amd_iommu_init.c | 10 +-------
 2 files changed, 3 insertions(+), 65 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index a57e9b7..89e6d4b 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -76,8 +76,6 @@ LIST_HEAD(hpet_map);
  * Domain for untranslated devices - only allocated
  * if iommu=pt passed on kernel cmd line.
  */
-static struct protection_domain *pt_domain;
-
 static const struct iommu_ops amd_iommu_ops;
 
 static ATOMIC_NOTIFIER_HEAD(ppr_notifier);
@@ -96,7 +94,7 @@ struct iommu_dev_data {
 	struct protection_domain *domain; /* Domain the device is bound to */
 	u16 devid;			  /* PCI Device ID */
 	bool iommu_v2;			  /* Device can make use of IOMMUv2 */
-	bool passthrough;		  /* Default for device is pt_domain */
+	bool passthrough;		  /* Device is identity mapped */
 	struct {
 		bool enabled;
 		int qdep;
@@ -116,7 +114,6 @@ struct iommu_cmd {
 struct kmem_cache *amd_iommu_irq_cache;
 
 static void update_domain(struct protection_domain *domain);
-static int alloc_passthrough_domain(void);
 static int protection_domain_init(struct protection_domain *domain);
 
 /****************************************************************************
@@ -2221,15 +2218,6 @@ static void __detach_device(struct iommu_dev_data *dev_data)
 	do_detach(head);
 
 	spin_unlock_irqrestore(&domain->lock, flags);
-
-	/*
-	 * If we run in passthrough mode the device must be assigned to the
-	 * passthrough domain if it is detached from any other domain.
-	 * Make sure we can deassign from the pt_domain itself.
-	 */
-	if (dev_data->passthrough &&
-	    (dev_data->domain == NULL && domain != pt_domain))
-		__attach_device(dev_data, pt_domain);
 }
 
 /*
@@ -2287,7 +2275,7 @@ static int amd_iommu_add_device(struct device *dev)
 
 	BUG_ON(!dev_data);
 
-	if (dev_data->iommu_v2)
+	if (iommu_pass_through || dev_data->iommu_v2)
 		iommu_request_dm_for_dev(dev);
 
 	/* Domains are initialized for this device - have a look what we ended up with */
@@ -2947,21 +2935,6 @@ out_err:
 	return NULL;
 }
 
-static int alloc_passthrough_domain(void)
-{
-	if (pt_domain != NULL)
-		return 0;
-
-	/* allocate passthrough domain */
-	pt_domain = protection_domain_alloc();
-	if (!pt_domain)
-		return -ENOMEM;
-
-	pt_domain->mode = PAGE_MODE_NONE;
-
-	return 0;
-}
-
 static struct iommu_domain *amd_iommu_domain_alloc(unsigned type)
 {
 	struct protection_domain *pdomain;
@@ -3222,33 +3195,6 @@ static const struct iommu_ops amd_iommu_ops = {
  *
  *****************************************************************************/
 
-int __init amd_iommu_init_passthrough(void)
-{
-	struct iommu_dev_data *dev_data;
-	struct pci_dev *dev = NULL;
-	int ret;
-
-	ret = alloc_passthrough_domain();
-	if (ret)
-		return ret;
-
-	for_each_pci_dev(dev) {
-		if (!check_device(&dev->dev))
-			continue;
-
-		dev_data = get_dev_data(&dev->dev);
-		dev_data->passthrough = true;
-
-		attach_device(&dev->dev, pt_domain);
-	}
-
-	amd_iommu_stats_init();
-
-	pr_info("AMD-Vi: Initialized for Passthrough Mode\n");
-
-	return 0;
-}
-
 /* IOMMUv2 specific functions */
 int amd_iommu_register_ppr_notifier(struct notifier_block *nb)
 {
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index dbda9ae..a24495e 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -2026,14 +2026,6 @@ static bool detect_ivrs(void)
 	return true;
 }
 
-static int amd_iommu_init_dma(void)
-{
-	if (iommu_pass_through)
-		return amd_iommu_init_passthrough();
-	else
-		return amd_iommu_init_dma_ops();
-}
-
 /****************************************************************************
  *
  * AMD IOMMU Initialization State Machine
@@ -2073,7 +2065,7 @@ static int __init state_next(void)
 		init_state = ret ? IOMMU_INIT_ERROR : IOMMU_INTERRUPTS_EN;
 		break;
 	case IOMMU_INTERRUPTS_EN:
-		ret = amd_iommu_init_dma();
+		ret = amd_iommu_init_dma_ops();
 		init_state = ret ? IOMMU_INIT_ERROR : IOMMU_DMA_OPS;
 		break;
 	case IOMMU_DMA_OPS:
-- 
1.9.1

[PATCH 3/5] iommu/amd: Allow non-IOMMUv2 devices in IOMMUv2 domains

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Since devices with IOMMUv2 functionality might be in the
same group as devices without it, allow those devices in
IOMMUv2 domains too.
Otherwise attaching the group with the IOMMUv2 device to the
domain will fail.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd_iommu.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 89e6d4b..6d3dae9 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2164,15 +2164,17 @@ static int attach_device(struct device *dev,
 	dev_data = get_dev_data(dev);
 
 	if (domain->flags & PD_IOMMUV2_MASK) {
-		if (!dev_data->iommu_v2 || !dev_data->passthrough)
+		if (!dev_data->passthrough)
 			return -EINVAL;
 
-		if (pdev_iommuv2_enable(pdev) != 0)
-			return -EINVAL;
+		if (dev_data->iommu_v2) {
+			if (pdev_iommuv2_enable(pdev) != 0)
+				return -EINVAL;
 
-		dev_data->ats.enabled = true;
-		dev_data->ats.qdep    = pci_ats_queue_depth(pdev);
-		dev_data->pri_tlp     = pci_pri_tlp_required(pdev);
+			dev_data->ats.enabled = true;
+			dev_data->ats.qdep    = pci_ats_queue_depth(pdev);
+			dev_data->pri_tlp     = pci_pri_tlp_required(pdev);
+		}
 	} else if (amd_iommu_iotlb_sup &&
 		   pci_enable_ats(pdev, PAGE_SHIFT) == 0) {
 		dev_data->ats.enabled = true;
@@ -2237,7 +2239,7 @@ static void detach_device(struct device *dev)
 	__detach_device(dev_data);
 	write_unlock_irqrestore(&amd_iommu_devtable_lock, flags);
 
-	if (domain->flags & PD_IOMMUV2_MASK)
+	if (domain->flags & PD_IOMMUV2_MASK && dev_data->iommu_v2)
 		pdev_iommuv2_disable(to_pci_dev(dev));
 	else if (dev_data->ats.enabled)
 		pci_disable_ats(to_pci_dev(dev));
-- 
1.9.1

[PATCH 4/5] iommu/amd: Use swiotlb in passthrough mode

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

In passthrough mode (iommu=pt) all devices are identity
mapped. If a device does not support 64bit DMA it might
still need remapping. Make sure swiotlb is initialized to
provide this remapping.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd_iommu.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 6d3dae9..e29baa6 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2282,12 +2282,10 @@ static int amd_iommu_add_device(struct device *dev)
 
 	/* Domains are initialized for this device - have a look what we ended up with */
 	domain = iommu_get_domain_for_dev(dev);
-	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
+	if (domain->type == IOMMU_DOMAIN_IDENTITY)
 		dev_data->passthrough = true;
-		dev->archdata.dma_ops = &nommu_dma_ops;
-	} else {
+	else
 		dev->archdata.dma_ops = &amd_iommu_dma_ops;
-	}
 
 out:
 	iommu_completion_wait(iommu);
@@ -2852,8 +2850,8 @@ int __init amd_iommu_init_api(void)
 
 int __init amd_iommu_init_dma_ops(void)
 {
+	swiotlb        = iommu_pass_through ? 1 : 0;
 	iommu_detected = 1;
-	swiotlb = 0;
 
 	amd_iommu_stats_init();
 
-- 
1.9.1

[PATCH 5/5] iommu/amd: Set global dma_ops if swiotlb is disabled

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Some AMD systems also have non-PCI devices which can do DMA.
Those can't be handled by the AMD IOMMU, as the hardware can
only handle PCI. These devices would end up with no dma_ops,
as neither the per-device nor the global dma_ops will get
set. SWIOTLB provides global dma_ops when it is active, so
make sure there are global dma_ops too when swiotlb is
disabled.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd_iommu.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index e29baa6..fa9508b 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2853,6 +2853,15 @@ int __init amd_iommu_init_dma_ops(void)
 	swiotlb        = iommu_pass_through ? 1 : 0;
 	iommu_detected = 1;
 
+	/*
+	 * In case we don't initialize SWIOTLB (actually the common case
+	 * when AMD IOMMU is enabled), make sure there are global
+	 * dma_ops set as a fall-back for devices not handled by this
+	 * driver (for example non-PCI devices).
+	 */
+	if (!swiotlb)
+		dma_ops = &nommu_dma_ops;
+
 	amd_iommu_stats_init();
 
 	if (amd_iommu_unmap_flush)
-- 
1.9.1

Re: [PATCH 0/5] AMD IOMMU Fixes for v4.2-rc4

[Oded Gabbay]

Hi Joerg,

I tested your patches on Kaveri, and while they do fix the initial
setup between amdkfd and IOMMU, the kernel crashes when an HSA process
tries to do any simple action that involves the kernel, such as to
create a compute queue.

So while I suppose the above patches should be included, the new IOMMU
feature as a whole is still broken for amdkfd.

This is the log I get from dmesg:

<snip>
[  116.592829] systemd-logind[2221]: New session 1 of user ogabbay.
[  136.933560] ------------[ cut here ]------------
[  136.933845] kernel BUG at drivers/iommu/amd_iommu.c:3321!
[  136.934168] invalid opcode: 0000 [#1] SMP
[  136.934424] Modules linked in: amdkfd amd_iommu_v2 radeon
drm_kms_helper ttm fuse
[  136.934919] CPU: 1 PID: 3894 Comm: kfdtest Not tainted 4.2.0-rc3-kfd+ #9
[  136.935319] Hardware name: AMD BALLINA/Ballina, BIOS
WBL3B20N_Weekly_13_11_2 11/20/2013
[  136.935796] task: ffff8800a0c48000 ti: ffff880095f28000 task.ti:
ffff880095f28000
[  136.936242] RIP: 0010:[<ffffffff81424220>]  [<ffffffff81424220>]
__flush_pasid+0xd0/0x153
[  136.936740] RSP: 0018:ffff880095f2bb98  EFLAGS: 00010046
[  136.937055] RAX: 0000000000000000 RBX: ffff8800bd89b138 RCX: 000000004712e4da
[  136.937480] RDX: ffff8800ba7c4d40 RSI: 0000000000000001 RDI: 0000000000000a88
[  136.937906] RBP: ffff880095f2bbf8 R08: 000000004712f017 R09: 0000000000000b40
[  136.938331] R10: ffff8800a0c48000 R11: ffff8800a0c48688 R12: 00000000fffff000
[  136.938755] R13: ffff8800bd89b148 R14: 000000007fffffff R15: ffff8800bd89b101
[  136.939180] FS:  00007f2dbbb14780(0000) GS:ffff88011ec80000(0000)
knlGS:0000000000000000
[  136.939662] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  136.940003] CR2: 0000000000b77000 CR3: 0000000095e15000 CR4: 00000000000406e0
[  136.940427] Stack:
[  136.940544]  ffff880095f2bc18 00000000bd89b190 ffff880000010000
ffff8800bd89b190
[  136.941009]  3000000e00000001 7ffffffffffff007 ffff880095f2bc08
0000000095e15001
[  136.941474]  ffff8800bd89b190 0000000000000001 0000000000000296
ffff8800bd89b138
[  136.941937] Call Trace:
[  136.942084]  [<ffffffff81424327>] __amd_iommu_flush_tlb+0x24/0x26
[  136.942447]  [<ffffffff814243fd>] amd_iommu_domain_set_gcr3+0x85/0xa5
[  136.942833]  [<ffffffff8175a844>] ? _raw_spin_unlock_irqrestore+0x41/0x46
[  136.943241]  [<ffffffffa01c4a9d>] amd_iommu_bind_pasid+0x197/0x20c
[amd_iommu_v2]
[  136.943695]  [<ffffffffa01cfbb3>]
kfd_bind_process_to_device+0x59/0x74 [amdkfd]
[  136.944144]  [<ffffffffa01cc406>] ?
kfd_ioctl_create_queue+0x375/0x4e8 [amdkfd]
[  136.944583]  [<ffffffffa01cc43f>] kfd_ioctl_create_queue+0x3ae/0x4e8 [amdkfd]
[  136.945013]  [<ffffffffa01cc091>] ?
kfd_ioctl_destroy_queue+0x73/0x73 [amdkfd]
[  136.945448]  [<ffffffffa01cc737>] kfd_ioctl+0x1be/0x2b5 [amdkfd]
[  136.945808]  [<ffffffff81325860>] ? inode_has_perm+0x2b/0x30
[  136.946146]  [<ffffffff8115a4bf>] do_vfs_ioctl+0x455/0x4dd
[  136.946474]  [<ffffffff81329122>] ? selinux_file_ioctl+0xaf/0xb4
[  136.946835]  [<ffffffff81323001>] ? security_file_ioctl+0x35/0x49
[  136.947198]  [<ffffffff8115a5a1>] SyS_ioctl+0x5a/0x7f
[  136.947499]  [<ffffffff8175ae57>] entry_SYSCALL_64_fastpath+0x12/0x6f
[  136.947881] Code: b6 6d b0 48 8b 45 b0 48 8b 53 10 0f b6 c4 41 c1
e5 10 c1 e0 10 44 89 6d b0 4c 8d 6b 10 89 45 ac 4c 39 ea 74 68 80 7a
44 00 75 02 <0f> 0b 0f b7 72 40 48 8b 0d a3 57 76 01 48 8b 3c f1 8b 4a
48 48
[  136.949408] RIP  [<ffffffff81424220>] __flush_pasid+0xd0/0x153
[  136.949762]  RSP <ffff880095f2bb98>
[  137.116301] ---[ end trace 2b10f64971a81bd2 ]---

Oded

On Tue, Jul 28, 2015 at 5:58 PM, Joerg Roedel <joro@8bytes.org> wrote:
> Hi,
>
> here are a couple of fixes for the AMD IOMMU driver for
> issues found recently. The issues were introduced by the
> default-domain conversion in this development cycle. I plan
> to send these fixes upstream by the end of the week.
>
> Please review!
>
> Thanks,
>
>         Joerg
>
> Joerg Roedel (5):
>   iommu/amd: Use iommu_attach_group()
>   iommu/amd: Use iommu core for passthrough mode
>   iommu/amd: Allow non-IOMMUv2 devices in IOMMUv2 domains
>   iommu/amd: Use swiotlb in passthrough mode
>   iommu/amd: Set global dma_ops if swiotlb is disabled
>
>  drivers/iommu/amd_iommu.c      | 91 +++++++++++-------------------------------
>  drivers/iommu/amd_iommu_init.c | 10 +----
>  drivers/iommu/amd_iommu_v2.c   | 24 +++++++++--
>  3 files changed, 45 insertions(+), 80 deletions(-)
>
> --
> 1.9.1
>
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/5] AMD IOMMU Fixes for v4.2-rc4

[Oded Gabbay]

On Tue, Jul 28, 2015 at 5:58 PM, Joerg Roedel <joro@8bytes.org> wrote:
> Hi,
>
> here are a couple of fixes for the AMD IOMMU driver for
> issues found recently. The issues were introduced by the
> default-domain conversion in this development cycle. I plan
> to send these fixes upstream by the end of the week.
>
> Please review!
>
> Thanks,
>
>         Joerg
>
> Joerg Roedel (5):
>   iommu/amd: Use iommu_attach_group()
>   iommu/amd: Use iommu core for passthrough mode
>   iommu/amd: Allow non-IOMMUv2 devices in IOMMUv2 domains
>   iommu/amd: Use swiotlb in passthrough mode
>   iommu/amd: Set global dma_ops if swiotlb is disabled
>
>  drivers/iommu/amd_iommu.c      | 91 +++++++++++-------------------------------
>  drivers/iommu/amd_iommu_init.c | 10 +----
>  drivers/iommu/amd_iommu_v2.c   | 24 +++++++++--
>  3 files changed, 45 insertions(+), 80 deletions(-)
>
> --
> 1.9.1
>
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

This series, with the additional patch, is:
Tested-by: "Oded Gabbay <oded.gabbay@gmail.com>"