[PATCH 0/9] AMD IOMMUv2 driver updates and fixes

[PATCH 0/9] AMD IOMMUv2 driver updates and fixes

[Joerg Roedel]

Hi,

here is a queue of patches for the AMD IOMMUv2 driver that came up
during development of the radeon_kfd driver. The enhance the semantics
and fix a number of bugs found in the code.


	Joerg

Diffstat:

 drivers/iommu/amd_iommu_v2.c |  104 ++++++++++++++++++++++++++++--------------
 1 file changed, 71 insertions(+), 33 deletions(-)

Shortlog:

Joerg Roedel (9):
      iommu/amd: Fix typo in amd_iommu_v2 driver
      iommu/amd: Don't call mmu_notifer_unregister in __unbind_pasid
      iommu/amd: Don't free pasid_state in mn_release path
      iommu/amd: Get rid of __unbind_pasid
      iommu/amd: Drop pasid_state reference in ppr_notifer error path
      iommu/amd: Add pasid_state->invalid flag
      iommu/amd: Don't hold a reference to mm_struct
      iommu/amd: Don't hold a reference to task_struct
      iommu/amd: Don't call the inv_ctx_cb when pasid is not set up

[PATCH 2/9] iommu/amd: Don't call mmu_notifer_unregister in __unbind_pasid

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

This function is called only in the mn_release() path, so
there is no need to unregister the mmu_notifer here.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
Tested-by: Oded Gabbay <Oded.Gabbay@amd.com>
---
 drivers/iommu/amd_iommu_v2.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/iommu/amd_iommu_v2.c b/drivers/iommu/amd_iommu_v2.c
index 0e29f6f..1fdd22c 100644
--- a/drivers/iommu/amd_iommu_v2.c
+++ b/drivers/iommu/amd_iommu_v2.c
@@ -313,8 +313,6 @@ static void __unbind_pasid(struct pasid_state *pasid_state)
 	/* Make sure no more pending faults are in the queue */
 	flush_workqueue(iommu_wq);
 
-	mmu_notifier_unregister(&pasid_state->mn, pasid_state->mm);
-
 	put_pasid_state(pasid_state); /* Reference taken in bind() function */
 }
 
-- 
1.7.9.5

[PATCH 1/9] iommu/amd: Fix typo in amd_iommu_v2 driver

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

Fix typo in a comment.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 drivers/iommu/amd_iommu_v2.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/amd_iommu_v2.c b/drivers/iommu/amd_iommu_v2.c
index 92fb77c..0e29f6f 100644
--- a/drivers/iommu/amd_iommu_v2.c
+++ b/drivers/iommu/amd_iommu_v2.c
@@ -49,7 +49,7 @@ struct pasid_state {
 						   calls */
 	struct task_struct *task;		/* Task bound to this PASID */
 	struct mm_struct *mm;			/* mm_struct for the faults */
-	struct mmu_notifier mn;                 /* mmu_otifier handle */
+	struct mmu_notifier mn;                 /* mmu_notifier handle */
 	struct pri_queue pri[PRI_QUEUE_SIZE];	/* PRI tag states */
 	struct device_state *device_state;	/* Link to our device_state */
 	int pasid;				/* PASID index */
-- 
1.7.9.5

[PATCH 3/9] iommu/amd: Don't free pasid_state in mn_release path

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

The mmu_notifier state is part of pasid_state so it can't be
freed in the mn_release path. Free the pasid_state after
mmu_notifer_unregister has completed.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
Tested-by: Oded Gabbay <Oded.Gabbay-5C7GfCeVMHo@public.gmane.org>
---
 drivers/iommu/amd_iommu_v2.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd_iommu_v2.c b/drivers/iommu/amd_iommu_v2.c
index 1fdd22c..a621552 100644
--- a/drivers/iommu/amd_iommu_v2.c
+++ b/drivers/iommu/amd_iommu_v2.c
@@ -312,8 +312,6 @@ static void __unbind_pasid(struct pasid_state *pasid_state)
 
 	/* Make sure no more pending faults are in the queue */
 	flush_workqueue(iommu_wq);
-
-	put_pasid_state(pasid_state); /* Reference taken in bind() function */
 }
 
 static void unbind_pasid(struct device_state *dev_state, int pasid)
@@ -325,7 +323,7 @@ static void unbind_pasid(struct device_state *dev_state, int pasid)
 		return;
 
 	__unbind_pasid(pasid_state);
-	put_pasid_state_wait(pasid_state); /* Reference taken in this function */
+	put_pasid_state(pasid_state); /* Reference taken in this function */
 }
 
 static void free_pasid_states_level1(struct pasid_state **tbl)
@@ -371,6 +369,9 @@ static void free_pasid_states(struct device_state *dev_state)
 		 * unbind the PASID
 		 */
 		mmu_notifier_unregister(&pasid_state->mn, pasid_state->mm);
+
+		put_pasid_state_wait(pasid_state); /* Reference taken in
+						      amd_iommu_pasid_bind */
 	}
 
 	if (dev_state->pasid_levels == 2)
@@ -690,6 +691,7 @@ out_unregister:
 	mmu_notifier_unregister(&pasid_state->mn, pasid_state->mm);
 
 out_free:
+	mmput(pasid_state->mm);
 	free_pasid_state(pasid_state);
 
 out:
@@ -730,6 +732,8 @@ void amd_iommu_unbind_pasid(struct pci_dev *pdev, int pasid)
 	/* This will call the mn_release function and unbind the PASID */
 	mmu_notifier_unregister(&pasid_state->mn, pasid_state->mm);
 
+	put_pasid_state_wait(pasid_state); /* Reference taken in
+					      amd_iommu_pasid_bind */
 out:
 	put_device_state(dev_state);
 }
-- 
1.7.9.5

[PATCH 4/9] iommu/amd: Get rid of __unbind_pasid

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

Unbind_pasid is only called from mn_release which already
has the pasid_state. Use this to simplify the unbind_pasid
path and get rid of __unbind_pasid.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
Tested-by: Oded Gabbay <Oded.Gabbay-5C7GfCeVMHo@public.gmane.org>
---
 drivers/iommu/amd_iommu_v2.c |   17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/amd_iommu_v2.c b/drivers/iommu/amd_iommu_v2.c
index a621552..574c71b 100644
--- a/drivers/iommu/amd_iommu_v2.c
+++ b/drivers/iommu/amd_iommu_v2.c
@@ -99,7 +99,6 @@ static struct workqueue_struct *iommu_wq;
 static u64 *empty_page_table;
 
 static void free_pasid_states(struct device_state *dev_state);
-static void unbind_pasid(struct device_state *dev_state, int pasid);
 
 static u16 device_id(struct pci_dev *pdev)
 {
@@ -301,7 +300,7 @@ static void put_pasid_state_wait(struct pasid_state *pasid_state)
 	free_pasid_state(pasid_state);
 }
 
-static void __unbind_pasid(struct pasid_state *pasid_state)
+static void unbind_pasid(struct pasid_state *pasid_state)
 {
 	struct iommu_domain *domain;
 
@@ -314,18 +313,6 @@ static void __unbind_pasid(struct pasid_state *pasid_state)
 	flush_workqueue(iommu_wq);
 }
 
-static void unbind_pasid(struct device_state *dev_state, int pasid)
-{
-	struct pasid_state *pasid_state;
-
-	pasid_state = get_pasid_state(dev_state, pasid);
-	if (pasid_state == NULL)
-		return;
-
-	__unbind_pasid(pasid_state);
-	put_pasid_state(pasid_state); /* Reference taken in this function */
-}
-
 static void free_pasid_states_level1(struct pasid_state **tbl)
 {
 	int i;
@@ -480,7 +467,7 @@ static void mn_release(struct mmu_notifier *mn, struct mm_struct *mm)
 	if (pasid_state->device_state->inv_ctx_cb)
 		dev_state->inv_ctx_cb(dev_state->pdev, pasid_state->pasid);
 
-	unbind_pasid(dev_state, pasid_state->pasid);
+	unbind_pasid(pasid_state);
 }
 
 static struct mmu_notifier_ops iommu_mn = {
-- 
1.7.9.5

[PATCH 5/9] iommu/amd: Drop pasid_state reference in ppr_notifer error path

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

In case we are not able to allocate a fault structure a
reference to the pasid_state will be leaked. Fix that by
dropping the reference in the error path in case we hold
one.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
Tested-by: Oded Gabbay <Oded.Gabbay-5C7GfCeVMHo@public.gmane.org>
---
 drivers/iommu/amd_iommu_v2.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/iommu/amd_iommu_v2.c b/drivers/iommu/amd_iommu_v2.c
index 574c71b..6ba707b 100644
--- a/drivers/iommu/amd_iommu_v2.c
+++ b/drivers/iommu/amd_iommu_v2.c
@@ -607,6 +607,10 @@ static int ppr_notifier(struct notifier_block *nb, unsigned long e, void *data)
 	ret = NOTIFY_OK;
 
 out_drop_state:
+
+	if (ret != NOTIFY_OK && pasid_state)
+		put_pasid_state(pasid_state);
+
 	put_device_state(dev_state);
 
 out:
-- 
1.7.9.5

[PATCH 6/9] iommu/amd: Add pasid_state->invalid flag

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

This is used to signal the ppr_notifer function that no more
faults should be processes on this pasid_state. This way we
can keep the pasid_state safely in the state-table so that
it can be freed in the amd_iommu_unbind_pasid() function.

This allows us to not hold a reference to the mm_struct
during the whole pasid-binding-time.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
Tested-by: Oded Gabbay <Oded.Gabbay-5C7GfCeVMHo@public.gmane.org>
---
 drivers/iommu/amd_iommu_v2.c |   18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd_iommu_v2.c b/drivers/iommu/amd_iommu_v2.c
index 6ba707b..69a46f1 100644
--- a/drivers/iommu/amd_iommu_v2.c
+++ b/drivers/iommu/amd_iommu_v2.c
@@ -53,6 +53,7 @@ struct pasid_state {
 	struct pri_queue pri[PRI_QUEUE_SIZE];	/* PRI tag states */
 	struct device_state *device_state;	/* Link to our device_state */
 	int pasid;				/* PASID index */
+	bool invalid;				/* Used during teardown */
 	spinlock_t lock;			/* Protect pri_queues and
 						   mmu_notifer_count */
 	wait_queue_head_t wq;			/* To wait for count == 0 */
@@ -306,8 +307,17 @@ static void unbind_pasid(struct pasid_state *pasid_state)
 
 	domain = pasid_state->device_state->domain;
 
+	/*
+	 * Mark pasid_state as invalid, no more faults will we added to the
+	 * work queue after this is visible everywhere.
+	 */
+	pasid_state->invalid = true;
+
+	/* Make sure this is visible */
+	smp_wmb();
+
+	/* After this the device/pasid can't access the mm anymore */
 	amd_iommu_domain_clear_gcr3(domain, pasid_state->pasid);
-	clear_pasid_state(pasid_state->device_state, pasid_state->pasid);
 
 	/* Make sure no more pending faults are in the queue */
 	flush_workqueue(iommu_wq);
@@ -573,7 +583,7 @@ static int ppr_notifier(struct notifier_block *nb, unsigned long e, void *data)
 		goto out;
 
 	pasid_state = get_pasid_state(dev_state, iommu_fault->pasid);
-	if (pasid_state == NULL) {
+	if (pasid_state == NULL || pasid_state->invalid) {
 		/* We know the device but not the PASID -> send INVALID */
 		amd_iommu_complete_ppr(dev_state->pdev, iommu_fault->pasid,
 				       PPR_INVALID, tag);
@@ -657,6 +667,7 @@ int amd_iommu_bind_pasid(struct pci_dev *pdev, int pasid,
 	pasid_state->mm           = get_task_mm(task);
 	pasid_state->device_state = dev_state;
 	pasid_state->pasid        = pasid;
+	pasid_state->invalid      = false;
 	pasid_state->mn.ops       = &iommu_mn;
 
 	if (pasid_state->mm == NULL)
@@ -720,6 +731,9 @@ void amd_iommu_unbind_pasid(struct pci_dev *pdev, int pasid)
 	 */
 	put_pasid_state(pasid_state);
 
+	/* Clear the pasid state so that the pasid can be re-used */
+	clear_pasid_state(dev_state, pasid_state->pasid);
+
 	/* This will call the mn_release function and unbind the PASID */
 	mmu_notifier_unregister(&pasid_state->mn, pasid_state->mm);
 
-- 
1.7.9.5

[PATCH 7/9] iommu/amd: Don't hold a reference to mm_struct

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

With mmu_notifiers we don't need to hold a reference to the
mm_struct during the time the pasid is bound to a device. We
can rely on the .mn_release call back to inform us when the
mm_struct goes away.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
Tested-by: Oded Gabbay <Oded.Gabbay-5C7GfCeVMHo@public.gmane.org>
---
 drivers/iommu/amd_iommu_v2.c |   40 +++++++++++++++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/amd_iommu_v2.c b/drivers/iommu/amd_iommu_v2.c
index 69a46f1..2b848c0 100644
--- a/drivers/iommu/amd_iommu_v2.c
+++ b/drivers/iommu/amd_iommu_v2.c
@@ -297,7 +297,6 @@ static void put_pasid_state_wait(struct pasid_state *pasid_state)
 		schedule();
 
 	finish_wait(&pasid_state->wq, &wait);
-	mmput(pasid_state->mm);
 	free_pasid_state(pasid_state);
 }
 
@@ -321,6 +320,13 @@ static void unbind_pasid(struct pasid_state *pasid_state)
 
 	/* Make sure no more pending faults are in the queue */
 	flush_workqueue(iommu_wq);
+
+	/*
+	 * No more faults are in the work queue and no new faults will be queued
+	 * from here on. We can safely set pasid_state->mm to NULL now as the
+	 * mm_struct might go away after we return.
+	 */
+	pasid_state->mm = NULL;
 }
 
 static void free_pasid_states_level1(struct pasid_state **tbl)
@@ -636,6 +642,7 @@ int amd_iommu_bind_pasid(struct pci_dev *pdev, int pasid,
 {
 	struct pasid_state *pasid_state;
 	struct device_state *dev_state;
+	struct mm_struct *mm;
 	u16 devid;
 	int ret;
 
@@ -659,12 +666,14 @@ int amd_iommu_bind_pasid(struct pci_dev *pdev, int pasid,
 	if (pasid_state == NULL)
 		goto out;
 
+
 	atomic_set(&pasid_state->count, 1);
 	init_waitqueue_head(&pasid_state->wq);
 	spin_lock_init(&pasid_state->lock);
 
+	mm                        = get_task_mm(task);
 	pasid_state->task         = task;
-	pasid_state->mm           = get_task_mm(task);
+	pasid_state->mm           = mm;
 	pasid_state->device_state = dev_state;
 	pasid_state->pasid        = pasid;
 	pasid_state->invalid      = false;
@@ -673,7 +682,7 @@ int amd_iommu_bind_pasid(struct pci_dev *pdev, int pasid,
 	if (pasid_state->mm == NULL)
 		goto out_free;
 
-	mmu_notifier_register(&pasid_state->mn, pasid_state->mm);
+	mmu_notifier_register(&pasid_state->mn, mm);
 
 	ret = set_pasid_state(dev_state, pasid_state, pasid);
 	if (ret)
@@ -684,16 +693,23 @@ int amd_iommu_bind_pasid(struct pci_dev *pdev, int pasid,
 	if (ret)
 		goto out_clear_state;
 
+	/*
+	 * Drop the reference to the mm_struct here. We rely on the
+	 * mmu_notifier release call-back to inform us when the mm
+	 * is going away.
+	 */
+	mmput(mm);
+
 	return 0;
 
 out_clear_state:
 	clear_pasid_state(dev_state, pasid);
 
 out_unregister:
-	mmu_notifier_unregister(&pasid_state->mn, pasid_state->mm);
+	mmu_notifier_unregister(&pasid_state->mn, mm);
 
 out_free:
-	mmput(pasid_state->mm);
+	mmput(mm);
 	free_pasid_state(pasid_state);
 
 out:
@@ -734,8 +750,18 @@ void amd_iommu_unbind_pasid(struct pci_dev *pdev, int pasid)
 	/* Clear the pasid state so that the pasid can be re-used */
 	clear_pasid_state(dev_state, pasid_state->pasid);
 
-	/* This will call the mn_release function and unbind the PASID */
-	mmu_notifier_unregister(&pasid_state->mn, pasid_state->mm);
+	/*
+	 * Check if pasid_state->mm is still valid. If mn_release has already
+	 * run it will be NULL and we can't (and don't need to) call
+	 * mmu_notifier_unregister() on it anymore.
+	 */
+	if (pasid_state->mm) {
+		/*
+		 * This will call the mn_release function and unbind
+		 * the PASID.
+		 */
+		mmu_notifier_unregister(&pasid_state->mn, pasid_state->mm);
+	}
 
 	put_pasid_state_wait(pasid_state); /* Reference taken in
 					      amd_iommu_pasid_bind */
-- 
1.7.9.5

[PATCH 8/9] iommu/amd: Don't hold a reference to task_struct

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

Since we are only caring about the lifetime of the mm_struct
and not the task we can't safely keep a reference to it. The
reference is also not needed anymore, so remove that code
entirely.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
Tested-by: Oded Gabbay <Oded.Gabbay-5C7GfCeVMHo@public.gmane.org>
---
 drivers/iommu/amd_iommu_v2.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iommu/amd_iommu_v2.c b/drivers/iommu/amd_iommu_v2.c
index 2b848c0..f7ca009 100644
--- a/drivers/iommu/amd_iommu_v2.c
+++ b/drivers/iommu/amd_iommu_v2.c
@@ -47,7 +47,6 @@ struct pasid_state {
 	atomic_t count;				/* Reference count */
 	unsigned mmu_notifier_count;		/* Counting nested mmu_notifier
 						   calls */
-	struct task_struct *task;		/* Task bound to this PASID */
 	struct mm_struct *mm;			/* mm_struct for the faults */
 	struct mmu_notifier mn;                 /* mmu_notifier handle */
 	struct pri_queue pri[PRI_QUEUE_SIZE];	/* PRI tag states */
@@ -531,7 +530,7 @@ static void do_fault(struct work_struct *work)
 	write = !!(fault->flags & PPR_FAULT_WRITE);
 
 	down_read(&fault->state->mm->mmap_sem);
-	npages = get_user_pages(fault->state->task, fault->state->mm,
+	npages = get_user_pages(NULL, fault->state->mm,
 				fault->address, 1, write, 0, &page, NULL);
 	up_read(&fault->state->mm->mmap_sem);
 
@@ -672,7 +671,6 @@ int amd_iommu_bind_pasid(struct pci_dev *pdev, int pasid,
 	spin_lock_init(&pasid_state->lock);
 
 	mm                        = get_task_mm(task);
-	pasid_state->task         = task;
 	pasid_state->mm           = mm;
 	pasid_state->device_state = dev_state;
 	pasid_state->pasid        = pasid;
-- 
1.7.9.5

[PATCH 9/9] iommu/amd: Don't call the inv_ctx_cb when pasid is not set up

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

On the error path of amd_iommu_bind_pasid() we call
mmu_notifier_unregister() for cleanup. This calls
mn_release() which calls the users inv_ctx_cb function if
one is available. Since the pasid is not set up yet there is
nothing the user can to tear down in this call-back. So
don't call inv_ctx_cb on the error path of
amd_iommu_unbind_pasid() and make life of the users simpler.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
Tested-by: Oded Gabbay <Oded.Gabbay-5C7GfCeVMHo@public.gmane.org>
---
 drivers/iommu/amd_iommu_v2.c |   17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/amd_iommu_v2.c b/drivers/iommu/amd_iommu_v2.c
index f7ca009..a195c78 100644
--- a/drivers/iommu/amd_iommu_v2.c
+++ b/drivers/iommu/amd_iommu_v2.c
@@ -52,7 +52,8 @@ struct pasid_state {
 	struct pri_queue pri[PRI_QUEUE_SIZE];	/* PRI tag states */
 	struct device_state *device_state;	/* Link to our device_state */
 	int pasid;				/* PASID index */
-	bool invalid;				/* Used during teardown */
+	bool invalid;				/* Used during setup and
+						   teardown of the pasid */
 	spinlock_t lock;			/* Protect pri_queues and
 						   mmu_notifer_count */
 	wait_queue_head_t wq;			/* To wait for count == 0 */
@@ -473,13 +474,15 @@ static void mn_release(struct mmu_notifier *mn, struct mm_struct *mm)
 {
 	struct pasid_state *pasid_state;
 	struct device_state *dev_state;
+	bool run_inv_ctx_cb;
 
 	might_sleep();
 
-	pasid_state = mn_to_state(mn);
-	dev_state   = pasid_state->device_state;
+	pasid_state    = mn_to_state(mn);
+	dev_state      = pasid_state->device_state;
+	run_inv_ctx_cb = !pasid_state->invalid;
 
-	if (pasid_state->device_state->inv_ctx_cb)
+	if (run_inv_ctx_cb && pasid_state->device_state->inv_ctx_cb)
 		dev_state->inv_ctx_cb(dev_state->pdev, pasid_state->pasid);
 
 	unbind_pasid(pasid_state);
@@ -674,7 +677,8 @@ int amd_iommu_bind_pasid(struct pci_dev *pdev, int pasid,
 	pasid_state->mm           = mm;
 	pasid_state->device_state = dev_state;
 	pasid_state->pasid        = pasid;
-	pasid_state->invalid      = false;
+	pasid_state->invalid      = true; /* Mark as valid only if we are
+					     done with setting up the pasid */
 	pasid_state->mn.ops       = &iommu_mn;
 
 	if (pasid_state->mm == NULL)
@@ -691,6 +695,9 @@ int amd_iommu_bind_pasid(struct pci_dev *pdev, int pasid,
 	if (ret)
 		goto out_clear_state;
 
+	/* Now we are ready to handle faults */
+	pasid_state->invalid = false;
+
 	/*
 	 * Drop the reference to the mm_struct here. We rely on the
 	 * mmu_notifier release call-back to inform us when the mm
-- 
1.7.9.5