iommu/amd: avoid irqs_disabled() check

iommu/amd: avoid irqs_disabled() check

[Sebastian Andrzej Siewior]

Primary motivation was to get rid of the "WARN_ON(!irqs_disabled());"
check. The first 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.

Sebastian

[PATCH 1/3] iommu/amd: Prevent possible null pointer dereference and infinite loop

[Sebastian Andrzej Siewior]

From: Anna-Maria Gleixner <anna-maria-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>

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>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy-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 8fb8c737fffe..e484275a4c69 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.17.0

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

[Sebastian Andrzej Siewior]

From: Anna-Maria Gleixner <anna-maria-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>

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>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy-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 e484275a4c69..f66a5d0b7c62 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.17.0

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

[Sebastian Andrzej Siewior]

From: Anna-Maria Gleixner <anna-maria-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>

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>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy-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 f66a5d0b7c62..a801678ae1b4 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 does assigns the device visible for the hardware
+ */
+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 does
- * assigns it visible for the hardware
- */
-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.17.0

Re: [PATCH 3/3] iommu/amd: Do not flush when device is busy

[Gary R Hook]

On 05/04/2018 11:22 AM, Sebastian Andrzej Siewior wrote:
> From: Anna-Maria Gleixner <anna-maria-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> 
> 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>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy-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 f66a5d0b7c62..a801678ae1b4 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 does assigns the device visible for the hardware
> + */

<grammar cop>

The prior version of this comment appears 3 times in the file, and is 
grammatically problematic every time. Can we simplify it to say

  * This function makes the device visible in the domain

Or some such? I.e. tidy up the two remaining comments?

</grammar cop>

Re: [PATCH 3/3] iommu/amd: Do not flush when device is busy

[Anna-Maria Gleixner]

On Fri, 4 May 2018, Gary R Hook wrote:

> On 05/04/2018 11:22 AM, Sebastian Andrzej Siewior wrote:
> > From: Anna-Maria Gleixner <anna-maria-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> > 
> > 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>
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy-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 f66a5d0b7c62..a801678ae1b4 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 does assigns the device visible for the hardware
> > + */
> 
> <grammar cop>
> 
> The prior version of this comment appears 3 times in the file, and is
> grammatically problematic every time. Can we simplify it to say
> 
>  * This function makes the device visible in the domain
> 
> Or some such? I.e. tidy up the two remaining comments?
> 
> </grammar cop>
> 

Will fix it in a separate patch - but I only found 2 places where the
prior version of this comment appears.

Anna-Maria

Re: [PATCH 3/3] iommu/amd: Do not flush when device is busy

[Gary R Hook]

On 05/07/2018 07:38 AM, Anna-Maria Gleixner wrote:
> On Fri, 4 May 2018, Gary R Hook wrote:
>>
>> <grammar cop>
>>
>> The prior version of this comment appears 3 times in the file, and is
>> grammatically problematic every time. Can we simplify it to say
>>
>>   * This function makes the device visible in the domain
>>
>> Or some such? I.e. tidy up the two remaining comments?
>>
>> </grammar cop>
>>
> 
> Will fix it in a separate patch - but I only found 2 places where the
> prior version of this comment appears.

My mistake. You are, of course, correct.