[PATCH] iommu/amd: Remove redundant WARN_ON()
The WARN_ON() was introduced in commit 272e4f99e966 ("iommu/amd: WARN
when __[attach|detach]_device are called with irqs enabled") to ensure
that the domain->lock is taken in proper irqs disabled context. This
is required, because the domain->lock is taken as well in irq
context.
The proper context check by the WARN_ON() is redundant, because it is
already covered by LOCKDEP. When working with locks and changing
context, a run with LOCKDEP is required anyway and would detect the
wrong lock context.
Furthermore all callers for those functions are within the same file
and all callers acquire another lock which already disables interrupts.
Signed-off-by: Anna-Maria Gleixner
---
drivers/iommu/amd_iommu.c | 12
1 file changed, 12 deletions(-)
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 596b95c50051..b5f39bffd684 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1944,12 +1944,6 @@ static int __attach_device(struct iommu_dev_data
*dev_data,
{
int ret;
- /*
-* Must be called with IRQs disabled. Warn here to detect early
-* when its not.
-*/
- WARN_ON(!irqs_disabled());
-
/* lock domain */
spin_lock(>lock);
@@ -2115,12 +2109,6 @@ 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(>lock);
--
2.18.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 0/4] iommu/amd: avoid irqs_disabled() check
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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 4/4] iommu/amd: Do not flush when device is busy
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-ma...@linutronix.de>
---
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(>lock, flags);
- ret = __attach_device(dev_data, domain);
+ __attach_device(dev_data, domain);
spin_unlock_irqrestore(>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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 3/4] iommu/amd: Cleanup locking in __attach/detach_device()
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-ma...@linutronix.de>
---
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(>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(_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(>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(>lock);
-
- return ret;
+ return 0;
}
@@ -2088,9 +2072,10 @@ static int attach_device(struct device *dev,
}
skip_ats_check:
- spin_lock_irqsave(_iommu_devtable_lock, flags);
+
+ spin_lock_irqsave(>lock, flags);
ret = __attach_device(dev_data, domain);
- spin_unlock_irqrestore(_iommu_devtable_lock, flags);
+ spin_unlock_irqrestore(>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(>lock);
-
- do_detach(dev_data);
-
- spin_unlock(>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(_iommu_devtable_lock, flags);
+ spin_lock_irqsave(>lock, flags);
__detach_device(dev_data);
- spin_unlock_irqrestore(_iommu_devtable_lock, flags);
+ spin_unlock_irqrestore(>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(_iommu_devtable_lock, flags);
-
+ spin_lock_irqsave(>lock, flags);
while (!list_empty(>dev_list)) {
entry = list_first_entry(>dev_list,
struct iommu_dev_data, list);
BUG_ON(!entry->domain);
__detach_device(entry);
}
-
- spin_unlock_irqrestore(_iommu_devtable_lock, flags);
+ spin_unlock_irqrestore
[PATCH v2 2/4] iommu/amd: Prevent possible null pointer dereference and infinite loop
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-ma...@linutronix.de>
---
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(>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(_iommu_devtable_lock, flags);
__detach_device(dev_data);
@@ -2793,6 +2790,7 @@ static void cleanup_domain(struct protection_domain
*domain)
while (!list_empty(>dev_list)) {
entry = list_first_entry(>dev_list,
struct iommu_dev_data, list);
+ BUG_ON(!entry->domain);
__detach_device(entry);
}
--
2.15.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 1/4] iommu/amd: Fix grammar of comments
Suggested-by: Gary R Hook <gary.h...@amd.com> Signed-off-by: Anna-Maria Gleixner <anna-ma...@linutronix.de> --- 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/3] iommu/amd: Do not flush when device is busy
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-ma...@linutronix.de> > > > > 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-ma...@linutronix.de> > > Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de> > > --- > > 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 > > + */ > > > > 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? > > > Will fix it in a separate patch - but I only found 2 places where the prior version of this comment appears. Anna-Maria ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
