[PATCH v2 0/4] iommu/amd: avoid irqs_disabled() check

[PATCH v2 0/4] iommu/amd: avoid irqs_disabled() check

[Anna-Maria Gleixner]

Primary motivation was to get rid of the "WARN_ON(!irqs_disabled());"
check. The second patch avoids a possible loop (if cleanup_domain() is
invoked `entry->domain == NULL' then it loops for ever). The
irqs_disabled() check has been replaced with a lockdep_assert_held() check.

v1..v2:
- Fix the grammar of comments as suggested by Gary Hook. Put the cleanup
  fix at the begin of the queue.

Anna-Maria

[PATCH v2 1/4] iommu/amd: Fix grammar of comments

[Anna-Maria Gleixner]

Suggested-by: Gary R Hook <gary.hook-5C7GfCeVMHo@public.gmane.org>
Signed-off-by: Anna-Maria Gleixner <anna-maria-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
 drivers/iommu/amd_iommu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 8fb8c737fffe..6f13c03eba62 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1939,8 +1939,8 @@ static void do_detach(struct iommu_dev_data *dev_data)
 }
 
 /*
- * If a device is not yet associated with a domain, this function does
- * assigns it visible for the hardware
+ * If a device is not yet associated with a domain, this function makes the
+ * device visible in the domain
  */
 static int __attach_device(struct iommu_dev_data *dev_data,
 			   struct protection_domain *domain)
@@ -2061,8 +2061,8 @@ static bool pci_pri_tlp_required(struct pci_dev *pdev)
 }
 
 /*
- * If a device is not yet associated with a domain, this function
- * assigns it visible for the hardware
+ * If a device is not yet associated with a domain, this function makes the
+ * device visible in the domain
  */
 static int attach_device(struct device *dev,
 			 struct protection_domain *domain)
-- 
2.15.1

Re: [PATCH v2 1/4] iommu/amd: Fix grammar of comments

[Joerg Roedel]

On Mon, May 07, 2018 at 02:53:26PM +0200, Anna-Maria Gleixner wrote:
> Suggested-by: Gary R Hook <gary.hook-5C7GfCeVMHo@public.gmane.org>
> Signed-off-by: Anna-Maria Gleixner <anna-maria-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> ---
>  drivers/iommu/amd_iommu.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Applied, thanks.

[PATCH v2 2/4] iommu/amd: Prevent possible null pointer dereference and infinite loop

[Anna-Maria Gleixner]

The check for !dev_data->domain in __detach_device() emits a warning and
returns. The calling code in detach_device() dereferences dev_data->domain
afterwards unconditionally, so in case that dev_data->domain is NULL the
warning will be immediately followed by a NULL pointer dereference.

The calling code in cleanup_domain() loops infinite when !dev_data->domain
and the check in __detach_device() returns immediately because dev_list is
not changed.

do_detach() duplicates this check without throwing a warning.

Move the check with the explanation of the do_detach() code into the caller
detach_device() and return immediately. Throw an error, when hitting the
condition in cleanup_domain().

Signed-off-by: Anna-Maria Gleixner <anna-maria-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
 drivers/iommu/amd_iommu.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 6f13c03eba62..a632cc2daada 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1911,15 +1911,6 @@ static void do_detach(struct iommu_dev_data *dev_data)
 	struct amd_iommu *iommu;
 	u16 alias;
 
-	/*
-	 * First check if the device is still attached. It might already
-	 * be detached from its domain because the generic
-	 * iommu_detach_group code detached it and we try again here in
-	 * our alias handling.
-	 */
-	if (!dev_data->domain)
-		return;
-
 	iommu = amd_iommu_rlookup_table[dev_data->devid];
 	alias = dev_data->alias;
 
@@ -2124,9 +2115,6 @@ static void __detach_device(struct iommu_dev_data *dev_data)
 	 */
 	WARN_ON(!irqs_disabled());
 
-	if (WARN_ON(!dev_data->domain))
-		return;
-
 	domain = dev_data->domain;
 
 	spin_lock(&domain->lock);
@@ -2148,6 +2136,15 @@ static void detach_device(struct device *dev)
 	dev_data = get_dev_data(dev);
 	domain   = dev_data->domain;
 
+	/*
+	 * First check if the device is still attached. It might already
+	 * be detached from its domain because the generic
+	 * iommu_detach_group code detached it and we try again here in
+	 * our alias handling.
+	 */
+	if (WARN_ON(!dev_data->domain))
+		return;
+
 	/* lock device table */
 	spin_lock_irqsave(&amd_iommu_devtable_lock, flags);
 	__detach_device(dev_data);
@@ -2793,6 +2790,7 @@ static void cleanup_domain(struct protection_domain *domain)
 	while (!list_empty(&domain->dev_list)) {
 		entry = list_first_entry(&domain->dev_list,
 					 struct iommu_dev_data, list);
+		BUG_ON(!entry->domain);
 		__detach_device(entry);
 	}
 
-- 
2.15.1

Re: [PATCH v2 2/4] iommu/amd: Prevent possible null pointer dereference and infinite loop

[Joerg Roedel]

On Mon, May 07, 2018 at 02:53:27PM +0200, Anna-Maria Gleixner wrote:
> @@ -2793,6 +2790,7 @@ static void cleanup_domain(struct protection_domain *domain)
>  	while (!list_empty(&domain->dev_list)) {
>  		entry = list_first_entry(&domain->dev_list,
>  					 struct iommu_dev_data, list);
> +		BUG_ON(!entry->domain);

I am not happy with that BUG_ON, and it is unnecessary because we would
run into a NULL-ptr deref later if we don't BUG_ON here. So this makes
it easier to debug if it ever happens, but changing the code so that
this can be turned into a WARN_ON would be nicer.

Anyway, applied for now.



	Joerg

[PATCH v2 3/4] iommu/amd: Cleanup locking in __attach/detach_device()

[Anna-Maria Gleixner]

Since introduction of the pd_bitmap_lock in commit 2bc001808904
("iommu/amd: Split domain id out of amd_iommu_devtable_lock")
amd_iommu_devtable_lock is only taken around __detach_device() and
__attach_device() calls.

The lock is not protecting anything as all operations are domain specific
and protected by domain->lock in __detach_device() and __attach_device(),
so amd_iommu_devtable_lock has no real purpose anymore.

Lock domain->lock before calling into __detach_device() and
__attach_device() and simplify the implementation of those functions. Add
lockdep checks where appropriate.

Signed-off-by: Anna-Maria Gleixner <anna-maria-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
 drivers/iommu/amd_iommu.c | 70 ++++++++++-------------------------------------
 1 file changed, 15 insertions(+), 55 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index a632cc2daada..38ec7cae0024 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -81,7 +81,6 @@
  */
 #define AMD_IOMMU_PGSIZES	((~0xFFFUL) & ~(2ULL << 38))
 
-static DEFINE_SPINLOCK(amd_iommu_devtable_lock);
 static DEFINE_SPINLOCK(pd_bitmap_lock);
 
 /* List of all available dev_data structures */
@@ -1886,6 +1885,8 @@ static void do_attach(struct iommu_dev_data *dev_data,
 	u16 alias;
 	bool ats;
 
+	lockdep_assert_held(&domain->lock);
+
 	iommu = amd_iommu_rlookup_table[dev_data->devid];
 	alias = dev_data->alias;
 	ats   = dev_data->ats.enabled;
@@ -1906,11 +1907,13 @@ static void do_attach(struct iommu_dev_data *dev_data,
 	device_flush_dte(dev_data);
 }
 
-static void do_detach(struct iommu_dev_data *dev_data)
+static void __detach_device(struct iommu_dev_data *dev_data)
 {
 	struct amd_iommu *iommu;
 	u16 alias;
 
+	lockdep_assert_held(&dev_data->domain->lock);
+
 	iommu = amd_iommu_rlookup_table[dev_data->devid];
 	alias = dev_data->alias;
 
@@ -1936,32 +1939,13 @@ static void do_detach(struct iommu_dev_data *dev_data)
 static int __attach_device(struct iommu_dev_data *dev_data,
 			   struct protection_domain *domain)
 {
-	int ret;
-
-	/*
-	 * Must be called with IRQs disabled. Warn here to detect early
-	 * when its not.
-	 */
-	WARN_ON(!irqs_disabled());
-
-	/* lock domain */
-	spin_lock(&domain->lock);
-
-	ret = -EBUSY;
 	if (dev_data->domain != NULL)
-		goto out_unlock;
+		return -EBUSY;
 
 	/* Attach alias group root */
 	do_attach(dev_data, domain);
 
-	ret = 0;
-
-out_unlock:
-
-	/* ready */
-	spin_unlock(&domain->lock);
-
-	return ret;
+	return 0;
 }
 
 
@@ -2088,9 +2072,10 @@ static int attach_device(struct device *dev,
 	}
 
 skip_ats_check:
-	spin_lock_irqsave(&amd_iommu_devtable_lock, flags);
+
+	spin_lock_irqsave(&domain->lock, flags);
 	ret = __attach_device(dev_data, domain);
-	spin_unlock_irqrestore(&amd_iommu_devtable_lock, flags);
+	spin_unlock_irqrestore(&domain->lock, flags);
 
 	/*
 	 * We might boot into a crash-kernel here. The crashed kernel
@@ -2103,29 +2088,7 @@ static int attach_device(struct device *dev,
 }
 
 /*
- * Removes a device from a protection domain (unlocked)
- */
-static void __detach_device(struct iommu_dev_data *dev_data)
-{
-	struct protection_domain *domain;
-
-	/*
-	 * Must be called with IRQs disabled. Warn here to detect early
-	 * when its not.
-	 */
-	WARN_ON(!irqs_disabled());
-
-	domain = dev_data->domain;
-
-	spin_lock(&domain->lock);
-
-	do_detach(dev_data);
-
-	spin_unlock(&domain->lock);
-}
-
-/*
- * Removes a device from a protection domain (with devtable_lock held)
+ * Removes a device from a protection domain
  */
 static void detach_device(struct device *dev)
 {
@@ -2145,10 +2108,9 @@ static void detach_device(struct device *dev)
 	if (WARN_ON(!dev_data->domain))
 		return;
 
-	/* lock device table */
-	spin_lock_irqsave(&amd_iommu_devtable_lock, flags);
+	spin_lock_irqsave(&domain->lock, flags);
 	__detach_device(dev_data);
-	spin_unlock_irqrestore(&amd_iommu_devtable_lock, flags);
+	spin_unlock_irqrestore(&domain->lock, flags);
 
 	if (!dev_is_pci(dev))
 		return;
@@ -2785,16 +2747,14 @@ static void cleanup_domain(struct protection_domain *domain)
 	struct iommu_dev_data *entry;
 	unsigned long flags;
 
-	spin_lock_irqsave(&amd_iommu_devtable_lock, flags);
-
+	spin_lock_irqsave(&domain->lock, flags);
 	while (!list_empty(&domain->dev_list)) {
 		entry = list_first_entry(&domain->dev_list,
 					 struct iommu_dev_data, list);
 		BUG_ON(!entry->domain);
 		__detach_device(entry);
 	}
-
-	spin_unlock_irqrestore(&amd_iommu_devtable_lock, flags);
+	spin_unlock_irqrestore(&domain->lock, flags);
 }
 
 static void protection_domain_free(struct protection_domain *domain)
-- 
2.15.1

Re: [PATCH v2 3/4] iommu/amd: Cleanup locking in __attach/detach_device()

[Joerg Roedel]

On Mon, May 07, 2018 at 02:53:28PM +0200, Anna-Maria Gleixner wrote:
> Since introduction of the pd_bitmap_lock in commit 2bc001808904
> ("iommu/amd: Split domain id out of amd_iommu_devtable_lock")
> amd_iommu_devtable_lock is only taken around __detach_device() and
> __attach_device() calls.
> 
> The lock is not protecting anything as all operations are domain specific
> and protected by domain->lock in __detach_device() and __attach_device(),
> so amd_iommu_devtable_lock has no real purpose anymore.

There is a subtle problem with this, as the lock you remove here
protects changes to the device table. Now one could argue that we never
check what we actually overwrite there, so it doesn't matter and the
lock can be removed. That is true, but it leaves an existing
race-condition in the code when a two cores try to assign the same
device to different domains.

So while at it, can you look into fixing that too before removing the
devtable_lock?


	Joerg

[PATCH v2 4/4] iommu/amd: Do not flush when device is busy

[Anna-Maria Gleixner]

When device is already attached to a domain, there is no need to execute
the domain_flush_tlb_pde(). Therefore move the check if the domain is set
into attach_device().

Signed-off-by: Anna-Maria Gleixner <anna-maria-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
 drivers/iommu/amd_iommu.c | 32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 38ec7cae0024..f9207e5b5901 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1878,8 +1878,11 @@ static void clear_dte_entry(u16 devid)
 	amd_iommu_apply_erratum_63(devid);
 }
 
-static void do_attach(struct iommu_dev_data *dev_data,
-		      struct protection_domain *domain)
+/*
+ * This function makes the device visible in the domain
+ */
+static void __attach_device(struct iommu_dev_data *dev_data,
+			    struct protection_domain *domain)
 {
 	struct amd_iommu *iommu;
 	u16 alias;
@@ -1932,23 +1935,6 @@ static void __detach_device(struct iommu_dev_data *dev_data)
 	device_flush_dte(dev_data);
 }
 
-/*
- * If a device is not yet associated with a domain, this function makes the
- * device visible in the domain
- */
-static int __attach_device(struct iommu_dev_data *dev_data,
-			   struct protection_domain *domain)
-{
-	if (dev_data->domain != NULL)
-		return -EBUSY;
-
-	/* Attach alias group root */
-	do_attach(dev_data, domain);
-
-	return 0;
-}
-
-
 static void pdev_iommuv2_disable(struct pci_dev *pdev)
 {
 	pci_disable_ats(pdev);
@@ -2045,7 +2031,6 @@ static int attach_device(struct device *dev,
 	struct pci_dev *pdev;
 	struct iommu_dev_data *dev_data;
 	unsigned long flags;
-	int ret;
 
 	dev_data = get_dev_data(dev);
 
@@ -2073,8 +2058,11 @@ static int attach_device(struct device *dev,
 
 skip_ats_check:
 
+	if (dev_data->domain != NULL)
+		return -EBUSY;
+
 	spin_lock_irqsave(&domain->lock, flags);
-	ret = __attach_device(dev_data, domain);
+	__attach_device(dev_data, domain);
 	spin_unlock_irqrestore(&domain->lock, flags);
 
 	/*
@@ -2084,7 +2072,7 @@ static int attach_device(struct device *dev,
 	 */
 	domain_flush_tlb_pde(domain);
 
-	return ret;
+	return 0;
 }
 
 /*
-- 
2.15.1