[PATCH 0/9] AMD IOMMU: Reduce dependency to struct device

[PATCH 0/9] AMD IOMMU: Reduce dependency to struct device

[Joerg Roedel]

Hi,

this patch series reduces the dependency of the AMD IOMMU driver to the
struct device. In fact, it removes the dependency that every request id
the IOMMU can see must have a PCI device in the system. So this is the
real fix for

	https://bugzilla.kernel.org/show_bug.cgi?id=35592

Any (constructive) feedback appreciated!

Regards,

	Joerg

Diffstat:

 arch/x86/include/asm/amd_iommu_types.h |    9 +-
 arch/x86/kernel/amd_iommu.c            |  291 ++++++++++++++++++--------------
 2 files changed, 175 insertions(+), 125 deletions(-)

Shortlog:

Joerg Roedel (9):
      x86/amd-iommu: Remove redundant device_flush_dte() calls
      x86/amd-iommu: Introduce global dev_data_list
      x86/amd-iommu: Store devid in dev_data
      x86/amd-iommu: Store ATS state in dev_data
      x86/amd-iommu: Use only dev_data for dte and iotlb flushing routines
      x86/amd-iommu: Use only dev_data in low-level domain attach/detach functions
      x86/amd-iommu: Allow dev_data->alias to be NULL
      x86/amd-iommu: Search for existind dev_data before allocting a new one
      x86/amd-iommu: Store device alias as dev_data pointer

[PATCH 1/9] x86/amd-iommu: Remove redundant device_flush_dte() calls

[Joerg Roedel]

Remove these function calls from places where the function
has already been called by another function.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/amd_iommu.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index 7c3a95e..cc6f6da6 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -1798,7 +1798,6 @@ static int device_change_notifier(struct notifier_block *nb,
 		goto out;
 	}
 
-	device_flush_dte(dev);
 	iommu_completion_wait(iommu);
 
 out:
@@ -2605,7 +2604,6 @@ static void amd_iommu_detach_device(struct iommu_domain *dom,
 	if (!iommu)
 		return;
 
-	device_flush_dte(dev);
 	iommu_completion_wait(iommu);
 }
 
-- 
1.7.4.1

[PATCH 2/9] x86/amd-iommu: Introduce global dev_data_list

[Joerg Roedel]

This list keeps all allocated iommu_dev_data structs in a
list together. This is needed for instances that have no
associated device.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/amd_iommu_types.h |    1 +
 arch/x86/kernel/amd_iommu.c            |   56 ++++++++++++++++++++++++++-----
 2 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/amd_iommu_types.h b/arch/x86/include/asm/amd_iommu_types.h
index 4c99829..35520e3 100644
--- a/arch/x86/include/asm/amd_iommu_types.h
+++ b/arch/x86/include/asm/amd_iommu_types.h
@@ -310,6 +310,7 @@ struct protection_domain {
  */
 struct iommu_dev_data {
 	struct list_head list;		  /* For domain->dev_list */
+	struct list_head dev_data_list;	  /* For global dev_data_list */
 	struct device *dev;		  /* Device this data belong to */
 	struct device *alias;		  /* The Alias Device */
 	struct protection_domain *domain; /* Domain the device is bound to */
diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index cc6f6da6..8b84890 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -45,6 +45,10 @@ static DEFINE_RWLOCK(amd_iommu_devtable_lock);
 static LIST_HEAD(iommu_pd_list);
 static DEFINE_SPINLOCK(iommu_pd_list_lock);
 
+/* List of all available dev_data structures */
+static LIST_HEAD(dev_data_list);
+static DEFINE_SPINLOCK(dev_data_list_lock);
+
 /*
  * Domain for untranslated devices - only allocated
  * if iommu=pt passed on kernel cmd line.
@@ -68,6 +72,35 @@ static void update_domain(struct protection_domain *domain);
  *
  ****************************************************************************/
 
+static struct iommu_dev_data *alloc_dev_data(void)
+{
+	struct iommu_dev_data *dev_data;
+	unsigned long flags;
+
+	dev_data = kzalloc(sizeof(*dev_data), GFP_KERNEL);
+	if (!dev_data)
+		return NULL;
+
+	atomic_set(&dev_data->bind, 0);
+
+	spin_lock_irqsave(&dev_data_list_lock, flags);
+	list_add_tail(&dev_data->dev_data_list, &dev_data_list);
+	spin_unlock_irqrestore(&dev_data_list_lock, flags);
+
+	return dev_data;
+}
+
+static void free_dev_data(struct iommu_dev_data *dev_data)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&dev_data_list_lock, flags);
+	list_del(&dev_data->dev_data_list);
+	spin_unlock_irqrestore(&dev_data_list_lock, flags);
+
+	kfree(dev_data);
+}
+
 static inline u16 get_device_id(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
@@ -139,32 +172,28 @@ static int iommu_init_device(struct device *dev)
 {
 	struct iommu_dev_data *dev_data;
 	struct pci_dev *pdev;
-	u16 devid, alias;
+	u16 alias;
 
 	if (dev->archdata.iommu)
 		return 0;
 
-	dev_data = kzalloc(sizeof(*dev_data), GFP_KERNEL);
+	dev_data = alloc_dev_data();
 	if (!dev_data)
 		return -ENOMEM;
 
 	dev_data->dev = dev;
 
-	devid = get_device_id(dev);
-	alias = amd_iommu_alias_table[devid];
+	alias = amd_iommu_alias_table[get_device_id(dev)];
 	pdev = pci_get_bus_and_slot(PCI_BUS(alias), alias & 0xff);
 	if (pdev)
 		dev_data->alias = &pdev->dev;
 	else {
-		kfree(dev_data);
+		free_dev_data(dev_data);
 		return -ENOTSUPP;
 	}
 
-	atomic_set(&dev_data->bind, 0);
-
 	dev->archdata.iommu = dev_data;
 
-
 	return 0;
 }
 
@@ -184,11 +213,16 @@ static void iommu_ignore_device(struct device *dev)
 
 static void iommu_uninit_device(struct device *dev)
 {
-	kfree(dev->archdata.iommu);
+	/*
+	 * Nothing to do here - we keep dev_data around for unplugged devices
+	 * and reuse it when the device is re-plugged - not doing so would
+	 * introduce a ton of races.
+	 */
 }
 
 void __init amd_iommu_uninit_devices(void)
 {
+	struct iommu_dev_data *dev_data, *n;
 	struct pci_dev *pdev = NULL;
 
 	for_each_pci_dev(pdev) {
@@ -198,6 +232,10 @@ void __init amd_iommu_uninit_devices(void)
 
 		iommu_uninit_device(&pdev->dev);
 	}
+
+	/* Free all of our dev_data structures */
+	list_for_each_entry_safe(dev_data, n, &dev_data_list, dev_data_list)
+		free_dev_data(dev_data);
 }
 
 int __init amd_iommu_init_devices(void)
-- 
1.7.4.1

Re: [PATCH 2/9] x86/amd-iommu: Introduce global dev_data_list

[Chris Wright]

* Joerg Roedel (joerg.roedel@amd.com) wrote:
> +static struct iommu_dev_data *alloc_dev_data(void)
> +{
> +	struct iommu_dev_data *dev_data;
> +	unsigned long flags;
> +
> +	dev_data = kzalloc(sizeof(*dev_data), GFP_KERNEL);
> +	if (!dev_data)
> +		return NULL;
> +
> +	atomic_set(&dev_data->bind, 0);
> +
> +	spin_lock_irqsave(&dev_data_list_lock, flags);
> +	list_add_tail(&dev_data->dev_data_list, &dev_data_list);
> +	spin_unlock_irqrestore(&dev_data_list_lock, flags);

Globally visible but only paritially initiailized.  I didn't see any, but
would this ever cause an issue?

> +static void free_dev_data(struct iommu_dev_data *dev_data)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dev_data_list_lock, flags);
> +	list_del(&dev_data->dev_data_list);
> +	spin_unlock_irqrestore(&dev_data_list_lock, flags);
> +
> +	kfree(dev_data);
<snip>
> +	/* Free all of our dev_data structures */
> +	list_for_each_entry_safe(dev_data, n, &dev_data_list, dev_data_list)
> +		free_dev_data(dev_data);

Given that it's not actually contended in early init, should be fine...but
typically full list traversal would be protected by lock rather than
repeatedly acquiring and releasing the lock.

Re: [PATCH 2/9] x86/amd-iommu: Introduce global dev_data_list

[Roedel, Joerg]

On Fri, Jun 10, 2011 at 01:36:04PM -0400, Chris Wright wrote:
> * Joerg Roedel (joerg.roedel@amd.com) wrote:
> > +static struct iommu_dev_data *alloc_dev_data(void)
> > +{
> > +	struct iommu_dev_data *dev_data;
> > +	unsigned long flags;
> > +
> > +	dev_data = kzalloc(sizeof(*dev_data), GFP_KERNEL);
> > +	if (!dev_data)
> > +		return NULL;
> > +
> > +	atomic_set(&dev_data->bind, 0);
> > +
> > +	spin_lock_irqsave(&dev_data_list_lock, flags);
> > +	list_add_tail(&dev_data->dev_data_list, &dev_data_list);
> > +	spin_unlock_irqrestore(&dev_data_list_lock, flags);
> 
> Globally visible but only paritially initiailized.  I didn't see any, but
> would this ever cause an issue?

I don't think so. Everything allocated here goes into iommu_init_device
where the alias-part of the struct is initialized. As long as this is
the only place that calls find_dev_data everything is fine.

> > +static void free_dev_data(struct iommu_dev_data *dev_data)
> > +{
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&dev_data_list_lock, flags);
> > +	list_del(&dev_data->dev_data_list);
> > +	spin_unlock_irqrestore(&dev_data_list_lock, flags);
> > +
> > +	kfree(dev_data);
> <snip>
> > +	/* Free all of our dev_data structures */
> > +	list_for_each_entry_safe(dev_data, n, &dev_data_list, dev_data_list)
> > +		free_dev_data(dev_data);
> 
> Given that it's not actually contended in early init, should be fine...but
> typically full list traversal would be protected by lock rather than
> repeatedly acquiring and releasing the lock.

Well, the lock shouldn't be necessary here at all. This function is
called in the error-path to clean up. Otherwise the dev_data structures
are never freed, so the lock can probably be removed from free_dev_data.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[PATCH 3/9] x86/amd-iommu: Store devid in dev_data

[Joerg Roedel]

This allows to use dev_data independent of struct device
later.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/amd_iommu_types.h |    1 +
 arch/x86/kernel/amd_iommu.c            |   39 ++++++++++++-------------------
 2 files changed, 16 insertions(+), 24 deletions(-)

diff --git a/arch/x86/include/asm/amd_iommu_types.h b/arch/x86/include/asm/amd_iommu_types.h
index 35520e3..2833862 100644
--- a/arch/x86/include/asm/amd_iommu_types.h
+++ b/arch/x86/include/asm/amd_iommu_types.h
@@ -315,6 +315,7 @@ struct iommu_dev_data {
 	struct device *alias;		  /* The Alias Device */
 	struct protection_domain *domain; /* Domain the device is bound to */
 	atomic_t bind;			  /* Domain attach reverent count */
+	u16 devid;			  /* PCI Device ID */
 };
 
 /*
diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index 8b84890..4e8b176 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -72,7 +72,7 @@ static void update_domain(struct protection_domain *domain);
  *
  ****************************************************************************/
 
-static struct iommu_dev_data *alloc_dev_data(void)
+static struct iommu_dev_data *alloc_dev_data(u16 devid)
 {
 	struct iommu_dev_data *dev_data;
 	unsigned long flags;
@@ -81,6 +81,7 @@ static struct iommu_dev_data *alloc_dev_data(void)
 	if (!dev_data)
 		return NULL;
 
+	dev_data->devid = devid;
 	atomic_set(&dev_data->bind, 0);
 
 	spin_lock_irqsave(&dev_data_list_lock, flags);
@@ -177,13 +178,13 @@ static int iommu_init_device(struct device *dev)
 	if (dev->archdata.iommu)
 		return 0;
 
-	dev_data = alloc_dev_data();
+	dev_data = alloc_dev_data(get_device_id(dev));
 	if (!dev_data)
 		return -ENOMEM;
 
 	dev_data->dev = dev;
 
-	alias = amd_iommu_alias_table[get_device_id(dev)];
+	alias = amd_iommu_alias_table[dev_data->devid];
 	pdev = pci_get_bus_and_slot(PCI_BUS(alias), alias & 0xff);
 	if (pdev)
 		dev_data->alias = &pdev->dev;
@@ -714,16 +715,16 @@ static int device_flush_iotlb(struct device *dev, u64 address, size_t size)
  */
 static int device_flush_dte(struct device *dev)
 {
+	struct iommu_dev_data *dev_data;
 	struct amd_iommu *iommu;
 	struct pci_dev *pdev;
-	u16 devid;
 	int ret;
 
-	pdev  = to_pci_dev(dev);
-	devid = get_device_id(dev);
-	iommu = amd_iommu_rlookup_table[devid];
+	pdev     = to_pci_dev(dev);
+	dev_data = get_dev_data(dev);
+	iommu    = amd_iommu_rlookup_table[dev_data->devid];
 
-	ret = iommu_flush_dte(iommu, devid);
+	ret = iommu_flush_dte(iommu, dev_data->devid);
 	if (ret)
 		return ret;
 
@@ -1570,11 +1571,9 @@ static void do_attach(struct device *dev, struct protection_domain *domain)
 	struct amd_iommu *iommu;
 	struct pci_dev *pdev;
 	bool ats = false;
-	u16 devid;
 
-	devid    = get_device_id(dev);
-	iommu    = amd_iommu_rlookup_table[devid];
 	dev_data = get_dev_data(dev);
+	iommu    = amd_iommu_rlookup_table[dev_data->devid];
 	pdev     = to_pci_dev(dev);
 
 	if (amd_iommu_iotlb_sup)
@@ -1583,7 +1582,7 @@ static void do_attach(struct device *dev, struct protection_domain *domain)
 	/* Update data structures */
 	dev_data->domain = domain;
 	list_add(&dev_data->list, &domain->dev_list);
-	set_dte_entry(devid, domain, ats);
+	set_dte_entry(dev_data->devid, domain, ats);
 
 	/* Do reference counting */
 	domain->dev_iommu[iommu->index] += 1;
@@ -1597,11 +1596,9 @@ static void do_detach(struct device *dev)
 {
 	struct iommu_dev_data *dev_data;
 	struct amd_iommu *iommu;
-	u16 devid;
 
-	devid    = get_device_id(dev);
-	iommu    = amd_iommu_rlookup_table[devid];
 	dev_data = get_dev_data(dev);
+	iommu    = amd_iommu_rlookup_table[dev_data->devid];
 
 	/* decrease reference counters */
 	dev_data->domain->dev_iommu[iommu->index] -= 1;
@@ -1610,7 +1607,7 @@ static void do_detach(struct device *dev)
 	/* Update data structures */
 	dev_data->domain = NULL;
 	list_del(&dev_data->list);
-	clear_dte_entry(devid);
+	clear_dte_entry(dev_data->devid);
 
 	/* Flush the DTE entry */
 	device_flush_dte(dev);
@@ -1760,9 +1757,7 @@ static struct protection_domain *domain_for_device(struct device *dev)
 	struct protection_domain *dom;
 	struct iommu_dev_data *dev_data, *alias_data;
 	unsigned long flags;
-	u16 devid;
 
-	devid      = get_device_id(dev);
 	dev_data   = get_dev_data(dev);
 	alias_data = get_dev_data(dev_data->alias);
 	if (!alias_data)
@@ -1897,8 +1892,7 @@ static void update_device_table(struct protection_domain *domain)
 
 	list_for_each_entry(dev_data, &domain->dev_list, list) {
 		struct pci_dev *pdev = to_pci_dev(dev_data->dev);
-		u16 devid = get_device_id(dev_data->dev);
-		set_dte_entry(devid, domain, pci_ats_enabled(pdev));
+		set_dte_entry(dev_data->devid, domain, pci_ats_enabled(pdev));
 	}
 }
 
@@ -2652,16 +2646,13 @@ static int amd_iommu_attach_device(struct iommu_domain *dom,
 	struct iommu_dev_data *dev_data;
 	struct amd_iommu *iommu;
 	int ret;
-	u16 devid;
 
 	if (!check_device(dev))
 		return -EINVAL;
 
 	dev_data = dev->archdata.iommu;
 
-	devid = get_device_id(dev);
-
-	iommu = amd_iommu_rlookup_table[devid];
+	iommu = amd_iommu_rlookup_table[dev_data->devid];
 	if (!iommu)
 		return -EINVAL;
 
-- 
1.7.4.1

[PATCH 4/9] x86/amd-iommu: Store ATS state in dev_data

[Joerg Roedel]

This allows the low-level functions to operate on dev_data
exclusivly later.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/amd_iommu_types.h |    4 +++
 arch/x86/kernel/amd_iommu.c            |   44 +++++++++++++++++--------------
 2 files changed, 28 insertions(+), 20 deletions(-)

diff --git a/arch/x86/include/asm/amd_iommu_types.h b/arch/x86/include/asm/amd_iommu_types.h
index 2833862..9fc045e 100644
--- a/arch/x86/include/asm/amd_iommu_types.h
+++ b/arch/x86/include/asm/amd_iommu_types.h
@@ -316,6 +316,10 @@ struct iommu_dev_data {
 	struct protection_domain *domain; /* Domain the device is bound to */
 	atomic_t bind;			  /* Domain attach reverent count */
 	u16 devid;			  /* PCI Device ID */
+	struct {
+		bool enabled;
+		int qdep;
+	} ats;				  /* ATS state */
 };
 
 /*
diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index 4e8b176..9e07ef6 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -695,17 +695,16 @@ void iommu_flush_all_caches(struct amd_iommu *iommu)
  */
 static int device_flush_iotlb(struct device *dev, u64 address, size_t size)
 {
-	struct pci_dev *pdev = to_pci_dev(dev);
+	struct iommu_dev_data *dev_data;
 	struct amd_iommu *iommu;
 	struct iommu_cmd cmd;
-	u16 devid;
 	int qdep;
 
-	qdep  = pci_ats_queue_depth(pdev);
-	devid = get_device_id(dev);
-	iommu = amd_iommu_rlookup_table[devid];
+	dev_data = get_dev_data(dev);
+	qdep     = dev_data->ats.qdep;
+	iommu    = amd_iommu_rlookup_table[dev_data->devid];
 
-	build_inv_iotlb_pages(&cmd, devid, qdep, address, size);
+	build_inv_iotlb_pages(&cmd, dev_data->devid, qdep, address, size);
 
 	return iommu_queue_command(iommu, &cmd);
 }
@@ -728,7 +727,7 @@ static int device_flush_dte(struct device *dev)
 	if (ret)
 		return ret;
 
-	if (pci_ats_enabled(pdev))
+	if (dev_data->ats.enabled)
 		ret = device_flush_iotlb(dev, 0, ~0UL);
 
 	return ret;
@@ -760,9 +759,8 @@ static void __domain_flush_pages(struct protection_domain *domain,
 	}
 
 	list_for_each_entry(dev_data, &domain->dev_list, list) {
-		struct pci_dev *pdev = to_pci_dev(dev_data->dev);
 
-		if (!pci_ats_enabled(pdev))
+		if (!dev_data->ats.enabled)
 			continue;
 
 		ret |= device_flush_iotlb(dev_data->dev, address, size);
@@ -1576,8 +1574,7 @@ static void do_attach(struct device *dev, struct protection_domain *domain)
 	iommu    = amd_iommu_rlookup_table[dev_data->devid];
 	pdev     = to_pci_dev(dev);
 
-	if (amd_iommu_iotlb_sup)
-		ats = pci_ats_enabled(pdev);
+	ats = dev_data->ats.enabled;
 
 	/* Update data structures */
 	dev_data->domain = domain;
@@ -1674,11 +1671,16 @@ static int attach_device(struct device *dev,
 			 struct protection_domain *domain)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
+	struct iommu_dev_data *dev_data;
 	unsigned long flags;
 	int ret;
 
-	if (amd_iommu_iotlb_sup)
-		pci_enable_ats(pdev, PAGE_SHIFT);
+	dev_data = get_dev_data(dev);
+
+	if (amd_iommu_iotlb_sup && pci_enable_ats(pdev, PAGE_SHIFT) == 0) {
+		dev_data->ats.enabled = true;
+		dev_data->ats.qdep    = pci_ats_queue_depth(pdev);
+	}
 
 	write_lock_irqsave(&amd_iommu_devtable_lock, flags);
 	ret = __attach_device(dev, domain);
@@ -1736,7 +1738,7 @@ static void __detach_device(struct device *dev)
  */
 static void detach_device(struct device *dev)
 {
-	struct pci_dev *pdev = to_pci_dev(dev);
+	struct iommu_dev_data *dev_data;
 	unsigned long flags;
 
 	/* lock device table */
@@ -1744,8 +1746,12 @@ static void detach_device(struct device *dev)
 	__detach_device(dev);
 	write_unlock_irqrestore(&amd_iommu_devtable_lock, flags);
 
-	if (amd_iommu_iotlb_sup && pci_ats_enabled(pdev))
-		pci_disable_ats(pdev);
+	dev_data = get_dev_data(dev);
+
+	if (dev_data->ats.enabled) {
+		pci_disable_ats(to_pci_dev(dev));
+		dev_data->ats.enabled = false;
+	}
 }
 
 /*
@@ -1890,10 +1896,8 @@ static void update_device_table(struct protection_domain *domain)
 {
 	struct iommu_dev_data *dev_data;
 
-	list_for_each_entry(dev_data, &domain->dev_list, list) {
-		struct pci_dev *pdev = to_pci_dev(dev_data->dev);
-		set_dte_entry(dev_data->devid, domain, pci_ats_enabled(pdev));
-	}
+	list_for_each_entry(dev_data, &domain->dev_list, list)
+		set_dte_entry(dev_data->devid, domain, dev_data->ats.enabled);
 }
 
 static void update_domain(struct protection_domain *domain)
-- 
1.7.4.1

[PATCH 5/9] x86/amd-iommu: Use only dev_data for dte and iotlb flushing routines

[Joerg Roedel]

This patch make the functions flushing the DTE and IOTLBs
only take the dev_data structure instead of the struct
device directly.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/amd_iommu.c |   23 +++++++++--------------
 1 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index 9e07ef6..0a5b465 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -693,14 +693,13 @@ void iommu_flush_all_caches(struct amd_iommu *iommu)
 /*
  * Command send function for flushing on-device TLB
  */
-static int device_flush_iotlb(struct device *dev, u64 address, size_t size)
+static int device_flush_iotlb(struct iommu_dev_data *dev_data,
+			      u64 address, size_t size)
 {
-	struct iommu_dev_data *dev_data;
 	struct amd_iommu *iommu;
 	struct iommu_cmd cmd;
 	int qdep;
 
-	dev_data = get_dev_data(dev);
 	qdep     = dev_data->ats.qdep;
 	iommu    = amd_iommu_rlookup_table[dev_data->devid];
 
@@ -712,23 +711,19 @@ static int device_flush_iotlb(struct device *dev, u64 address, size_t size)
 /*
  * Command send function for invalidating a device table entry
  */
-static int device_flush_dte(struct device *dev)
+static int device_flush_dte(struct iommu_dev_data *dev_data)
 {
-	struct iommu_dev_data *dev_data;
 	struct amd_iommu *iommu;
-	struct pci_dev *pdev;
 	int ret;
 
-	pdev     = to_pci_dev(dev);
-	dev_data = get_dev_data(dev);
-	iommu    = amd_iommu_rlookup_table[dev_data->devid];
+	iommu = amd_iommu_rlookup_table[dev_data->devid];
 
 	ret = iommu_flush_dte(iommu, dev_data->devid);
 	if (ret)
 		return ret;
 
 	if (dev_data->ats.enabled)
-		ret = device_flush_iotlb(dev, 0, ~0UL);
+		ret = device_flush_iotlb(dev_data, 0, ~0UL);
 
 	return ret;
 }
@@ -763,7 +758,7 @@ static void __domain_flush_pages(struct protection_domain *domain,
 		if (!dev_data->ats.enabled)
 			continue;
 
-		ret |= device_flush_iotlb(dev_data->dev, address, size);
+		ret |= device_flush_iotlb(dev_data, address, size);
 	}
 
 	WARN_ON(ret);
@@ -815,7 +810,7 @@ static void domain_flush_devices(struct protection_domain *domain)
 	spin_lock_irqsave(&domain->lock, flags);
 
 	list_for_each_entry(dev_data, &domain->dev_list, list)
-		device_flush_dte(dev_data->dev);
+		device_flush_dte(dev_data);
 
 	spin_unlock_irqrestore(&domain->lock, flags);
 }
@@ -1586,7 +1581,7 @@ static void do_attach(struct device *dev, struct protection_domain *domain)
 	domain->dev_cnt                 += 1;
 
 	/* Flush the DTE entry */
-	device_flush_dte(dev);
+	device_flush_dte(dev_data);
 }
 
 static void do_detach(struct device *dev)
@@ -1607,7 +1602,7 @@ static void do_detach(struct device *dev)
 	clear_dte_entry(dev_data->devid);
 
 	/* Flush the DTE entry */
-	device_flush_dte(dev);
+	device_flush_dte(dev_data);
 }
 
 /*
-- 
1.7.4.1

[PATCH 6/9] x86/amd-iommu: Use only dev_data in low-level domain attach/detach functions

[Joerg Roedel]

With this patch the low-level attach/detach functions only
work on dev_data structures. This allows to remove the
dev_data->dev pointer.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/amd_iommu_types.h |    1 -
 arch/x86/kernel/amd_iommu.c            |   61 ++++++++++++-------------------
 2 files changed, 24 insertions(+), 38 deletions(-)

diff --git a/arch/x86/include/asm/amd_iommu_types.h b/arch/x86/include/asm/amd_iommu_types.h
index 9fc045e..fffbe3c 100644
--- a/arch/x86/include/asm/amd_iommu_types.h
+++ b/arch/x86/include/asm/amd_iommu_types.h
@@ -311,7 +311,6 @@ struct protection_domain {
 struct iommu_dev_data {
 	struct list_head list;		  /* For domain->dev_list */
 	struct list_head dev_data_list;	  /* For global dev_data_list */
-	struct device *dev;		  /* Device this data belong to */
 	struct device *alias;		  /* The Alias Device */
 	struct protection_domain *domain; /* Domain the device is bound to */
 	atomic_t bind;			  /* Domain attach reverent count */
diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index 0a5b465..a15a172 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -182,8 +182,6 @@ static int iommu_init_device(struct device *dev)
 	if (!dev_data)
 		return -ENOMEM;
 
-	dev_data->dev = dev;
-
 	alias = amd_iommu_alias_table[dev_data->devid];
 	pdev = pci_get_bus_and_slot(PCI_BUS(alias), alias & 0xff);
 	if (pdev)
@@ -1558,18 +1556,14 @@ static void clear_dte_entry(u16 devid)
 	amd_iommu_apply_erratum_63(devid);
 }
 
-static void do_attach(struct device *dev, struct protection_domain *domain)
+static void do_attach(struct iommu_dev_data *dev_data,
+		      struct protection_domain *domain)
 {
-	struct iommu_dev_data *dev_data;
 	struct amd_iommu *iommu;
-	struct pci_dev *pdev;
-	bool ats = false;
-
-	dev_data = get_dev_data(dev);
-	iommu    = amd_iommu_rlookup_table[dev_data->devid];
-	pdev     = to_pci_dev(dev);
+	bool ats;
 
-	ats = dev_data->ats.enabled;
+	iommu = amd_iommu_rlookup_table[dev_data->devid];
+	ats   = dev_data->ats.enabled;
 
 	/* Update data structures */
 	dev_data->domain = domain;
@@ -1584,13 +1578,11 @@ static void do_attach(struct device *dev, struct protection_domain *domain)
 	device_flush_dte(dev_data);
 }
 
-static void do_detach(struct device *dev)
+static void do_detach(struct iommu_dev_data *dev_data)
 {
-	struct iommu_dev_data *dev_data;
 	struct amd_iommu *iommu;
 
-	dev_data = get_dev_data(dev);
-	iommu    = amd_iommu_rlookup_table[dev_data->devid];
+	iommu = amd_iommu_rlookup_table[dev_data->devid];
 
 	/* decrease reference counters */
 	dev_data->domain->dev_iommu[iommu->index] -= 1;
@@ -1609,13 +1601,12 @@ static void do_detach(struct device *dev)
  * If a device is not yet associated with a domain, this function does
  * assigns it visible for the hardware
  */
-static int __attach_device(struct device *dev,
+static int __attach_device(struct iommu_dev_data *dev_data,
 			   struct protection_domain *domain)
 {
-	struct iommu_dev_data *dev_data, *alias_data;
+	struct iommu_dev_data *alias_data;
 	int ret;
 
-	dev_data   = get_dev_data(dev);
 	alias_data = get_dev_data(dev_data->alias);
 
 	if (!alias_data)
@@ -1635,16 +1626,15 @@ static int __attach_device(struct device *dev,
 		goto out_unlock;
 
 	/* Do real assignment */
-	if (dev_data->alias != dev) {
-		alias_data = get_dev_data(dev_data->alias);
+	if (dev_data->devid != alias_data->devid) {
 		if (alias_data->domain == NULL)
-			do_attach(dev_data->alias, domain);
+			do_attach(alias_data, domain);
 
 		atomic_inc(&alias_data->bind);
 	}
 
 	if (dev_data->domain == NULL)
-		do_attach(dev, domain);
+		do_attach(dev_data, domain);
 
 	atomic_inc(&dev_data->bind);
 
@@ -1678,7 +1668,7 @@ static int attach_device(struct device *dev,
 	}
 
 	write_lock_irqsave(&amd_iommu_devtable_lock, flags);
-	ret = __attach_device(dev, domain);
+	ret = __attach_device(dev_data, domain);
 	write_unlock_irqrestore(&amd_iommu_devtable_lock, flags);
 
 	/*
@@ -1694,9 +1684,8 @@ static int attach_device(struct device *dev,
 /*
  * Removes a device from a protection domain (unlocked)
  */
-static void __detach_device(struct device *dev)
+static void __detach_device(struct iommu_dev_data *dev_data)
 {
-	struct iommu_dev_data *dev_data = get_dev_data(dev);
 	struct iommu_dev_data *alias_data;
 	struct protection_domain *domain;
 	unsigned long flags;
@@ -1707,14 +1696,14 @@ static void __detach_device(struct device *dev)
 
 	spin_lock_irqsave(&domain->lock, flags);
 
-	if (dev_data->alias != dev) {
-		alias_data = get_dev_data(dev_data->alias);
+	alias_data = get_dev_data(dev_data->alias);
+	if (dev_data->devid != alias_data->devid) {
 		if (atomic_dec_and_test(&alias_data->bind))
-			do_detach(dev_data->alias);
+			do_detach(alias_data);
 	}
 
 	if (atomic_dec_and_test(&dev_data->bind))
-		do_detach(dev);
+		do_detach(dev_data);
 
 	spin_unlock_irqrestore(&domain->lock, flags);
 
@@ -1725,7 +1714,7 @@ static void __detach_device(struct device *dev)
 	 */
 	if (iommu_pass_through &&
 	    (dev_data->domain == NULL && domain != pt_domain))
-		__attach_device(dev, pt_domain);
+		__attach_device(dev_data, pt_domain);
 }
 
 /*
@@ -1736,13 +1725,13 @@ static void detach_device(struct device *dev)
 	struct iommu_dev_data *dev_data;
 	unsigned long flags;
 
+	dev_data = get_dev_data(dev);
+
 	/* lock device table */
 	write_lock_irqsave(&amd_iommu_devtable_lock, flags);
-	__detach_device(dev);
+	__detach_device(dev_data);
 	write_unlock_irqrestore(&amd_iommu_devtable_lock, flags);
 
-	dev_data = get_dev_data(dev);
-
 	if (dev_data->ats.enabled) {
 		pci_disable_ats(to_pci_dev(dev));
 		dev_data->ats.enabled = false;
@@ -1768,7 +1757,7 @@ static struct protection_domain *domain_for_device(struct device *dev)
 	dom = dev_data->domain;
 	if (dom == NULL &&
 	    alias_data->domain != NULL) {
-		__attach_device(dev, alias_data->domain);
+		__attach_device(dev_data, alias_data->domain);
 		dom = alias_data->domain;
 	}
 
@@ -2527,9 +2516,7 @@ static void cleanup_domain(struct protection_domain *domain)
 	write_lock_irqsave(&amd_iommu_devtable_lock, flags);
 
 	list_for_each_entry_safe(dev_data, next, &domain->dev_list, list) {
-		struct device *dev = dev_data->dev;
-
-		__detach_device(dev);
+		__detach_device(dev_data);
 		atomic_set(&dev_data->bind, 0);
 	}
 
-- 
1.7.4.1

[PATCH 7/9] x86/amd-iommu: Allow dev_data->alias to be NULL

[Joerg Roedel]

Let dev_data->alias be just NULL if the device has no alias.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/amd_iommu.c |   77 +++++++++++++++++++++++--------------------
 1 files changed, 41 insertions(+), 36 deletions(-)

diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index a15a172..2d914a4 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -183,12 +183,14 @@ static int iommu_init_device(struct device *dev)
 		return -ENOMEM;
 
 	alias = amd_iommu_alias_table[dev_data->devid];
-	pdev = pci_get_bus_and_slot(PCI_BUS(alias), alias & 0xff);
-	if (pdev)
-		dev_data->alias = &pdev->dev;
-	else {
-		free_dev_data(dev_data);
-		return -ENOTSUPP;
+	if (alias != dev_data->devid) {
+		pdev = pci_get_bus_and_slot(PCI_BUS(alias), alias & 0xff);
+		if (pdev)
+			dev_data->alias = &pdev->dev;
+		else {
+			free_dev_data(dev_data);
+			return -ENOTSUPP;
+		}
 	}
 
 	dev->archdata.iommu = dev_data;
@@ -1604,29 +1606,31 @@ static void do_detach(struct iommu_dev_data *dev_data)
 static int __attach_device(struct iommu_dev_data *dev_data,
 			   struct protection_domain *domain)
 {
-	struct iommu_dev_data *alias_data;
 	int ret;
 
-	alias_data = get_dev_data(dev_data->alias);
-
-	if (!alias_data)
-		return -EINVAL;
-
 	/* lock domain */
 	spin_lock(&domain->lock);
 
-	/* Some sanity checks */
-	ret = -EBUSY;
-	if (alias_data->domain != NULL &&
-	    alias_data->domain != domain)
-		goto out_unlock;
+	if (dev_data->alias != NULL) {
+		struct iommu_dev_data *alias_data;
+
+		alias_data = get_dev_data(dev_data->alias);
+
+		ret = -EINVAL;
+		if (!alias_data)
+			goto out_unlock;
 
-	if (dev_data->domain != NULL &&
-	    dev_data->domain != domain)
-		goto out_unlock;
+		/* Some sanity checks */
+		ret = -EBUSY;
+		if (alias_data->domain != NULL &&
+				alias_data->domain != domain)
+			goto out_unlock;
 
-	/* Do real assignment */
-	if (dev_data->devid != alias_data->devid) {
+		if (dev_data->domain != NULL &&
+				dev_data->domain != domain)
+			goto out_unlock;
+
+		/* Do real assignment */
 		if (alias_data->domain == NULL)
 			do_attach(alias_data, domain);
 
@@ -1696,8 +1700,8 @@ static void __detach_device(struct iommu_dev_data *dev_data)
 
 	spin_lock_irqsave(&domain->lock, flags);
 
-	alias_data = get_dev_data(dev_data->alias);
-	if (dev_data->devid != alias_data->devid) {
+	if (dev_data->alias) {
+		alias_data = get_dev_data(dev_data->alias);
 		if (atomic_dec_and_test(&alias_data->bind))
 			do_detach(alias_data);
 	}
@@ -1744,24 +1748,25 @@ static void detach_device(struct device *dev)
  */
 static struct protection_domain *domain_for_device(struct device *dev)
 {
-	struct protection_domain *dom;
 	struct iommu_dev_data *dev_data, *alias_data;
+	struct protection_domain *dom = NULL;
 	unsigned long flags;
 
 	dev_data   = get_dev_data(dev);
-	alias_data = get_dev_data(dev_data->alias);
-	if (!alias_data)
-		return NULL;
 
-	read_lock_irqsave(&amd_iommu_devtable_lock, flags);
-	dom = dev_data->domain;
-	if (dom == NULL &&
-	    alias_data->domain != NULL) {
-		__attach_device(dev_data, alias_data->domain);
-		dom = alias_data->domain;
-	}
+	if (dev_data->domain)
+		return dev_data->domain;
 
-	read_unlock_irqrestore(&amd_iommu_devtable_lock, flags);
+	if (dev_data->alias != NULL) {
+		alias_data = get_dev_data(dev_data->alias);
+
+		read_lock_irqsave(&amd_iommu_devtable_lock, flags);
+		if (alias_data->domain != NULL) {
+			__attach_device(dev_data, alias_data->domain);
+			dom = alias_data->domain;
+		}
+		read_unlock_irqrestore(&amd_iommu_devtable_lock, flags);
+	}
 
 	return dom;
 }
-- 
1.7.4.1

[PATCH 8/9] x86/amd-iommu: Search for existind dev_data before allocting a new one

[Joerg Roedel]

Search for existing dev_data first will allow to switch
dev_data->alias to just store dev_data instead of struct
device.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/amd_iommu.c |   33 ++++++++++++++++++++++++++++++++-
 1 files changed, 32 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index 2d914a4..f647998 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -102,6 +102,37 @@ static void free_dev_data(struct iommu_dev_data *dev_data)
 	kfree(dev_data);
 }
 
+static struct iommu_dev_data *search_dev_data(u16 devid)
+{
+	struct iommu_dev_data *dev_data;
+	unsigned long flags;
+
+	spin_lock_irqsave(&dev_data_list_lock, flags);
+	list_for_each_entry(dev_data, &dev_data_list, dev_data_list) {
+		if (dev_data->devid == devid)
+			goto out_unlock;
+	}
+
+	dev_data = NULL;
+
+out_unlock:
+	spin_unlock_irqrestore(&dev_data_list_lock, flags);
+
+	return dev_data;
+}
+
+static struct iommu_dev_data *find_dev_data(u16 devid)
+{
+	struct iommu_dev_data *dev_data;
+
+	dev_data = search_dev_data(devid);
+
+	if (dev_data == NULL)
+		dev_data = alloc_dev_data(devid);
+
+	return dev_data;
+}
+
 static inline u16 get_device_id(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
@@ -178,7 +209,7 @@ static int iommu_init_device(struct device *dev)
 	if (dev->archdata.iommu)
 		return 0;
 
-	dev_data = alloc_dev_data(get_device_id(dev));
+	dev_data = find_dev_data(get_device_id(dev));
 	if (!dev_data)
 		return -ENOMEM;
 
-- 
1.7.4.1

Re: [PATCH 8/9] x86/amd-iommu: Search for existind dev_data before allocting a new one

[Chris Wright]

* Joerg Roedel (joerg.roedel@amd.com) wrote:
> +static struct iommu_dev_data *search_dev_data(u16 devid)
> +{
> +     struct iommu_dev_data *dev_data;
> +     unsigned long flags;
> +
> +	spin_lock_irqsave(&dev_data_list_lock, flags);
> +	list_for_each_entry(dev_data, &dev_data_list, dev_data_list) {
> +		if (dev_data->devid == devid)

This devid match on a global list is not going to work in a multi-segment
machine.  I guess non-zero segment function would either not exist or go
through swiotlb since it's not supported by hw def'n, so only a concern
for the future (along with the other devid based lookup tables).

thanks,
-chris

Re: [PATCH 8/9] x86/amd-iommu: Search for existind dev_data before allocting a new one

[Roedel, Joerg]

On Fri, Jun 10, 2011 at 04:52:26PM -0400, Chris Wright wrote:
> * Joerg Roedel (joerg.roedel@amd.com) wrote:
> > +static struct iommu_dev_data *search_dev_data(u16 devid)
> > +{
> > +     struct iommu_dev_data *dev_data;
> > +     unsigned long flags;
> > +
> > +	spin_lock_irqsave(&dev_data_list_lock, flags);
> > +	list_for_each_entry(dev_data, &dev_data_list, dev_data_list) {
> > +		if (dev_data->devid == devid)

Right, multiple segments are not handled at all by the driver. The ACPI
tables support them in theory, but there are no systems to test those
things on.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[PATCH 9/9] x86/amd-iommu: Store device alias as dev_data pointer

[Joerg Roedel]

This finally allows PCI-Device-IDs to be handled by the
IOMMU driver that have no corresponding struct device
present in the system.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/amd_iommu_types.h |    2 +-
 arch/x86/kernel/amd_iommu.c            |   34 ++++++++++++++-----------------
 2 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/arch/x86/include/asm/amd_iommu_types.h b/arch/x86/include/asm/amd_iommu_types.h
index fffbe3c..5b9c507 100644
--- a/arch/x86/include/asm/amd_iommu_types.h
+++ b/arch/x86/include/asm/amd_iommu_types.h
@@ -311,7 +311,7 @@ struct protection_domain {
 struct iommu_dev_data {
 	struct list_head list;		  /* For domain->dev_list */
 	struct list_head dev_data_list;	  /* For global dev_data_list */
-	struct device *alias;		  /* The Alias Device */
+	struct iommu_dev_data *alias_data;/* The alias dev_data */
 	struct protection_domain *domain; /* Domain the device is bound to */
 	atomic_t bind;			  /* Domain attach reverent count */
 	u16 devid;			  /* PCI Device ID */
diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index f647998..0ed1c94 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -203,7 +203,6 @@ static bool check_device(struct device *dev)
 static int iommu_init_device(struct device *dev)
 {
 	struct iommu_dev_data *dev_data;
-	struct pci_dev *pdev;
 	u16 alias;
 
 	if (dev->archdata.iommu)
@@ -215,13 +214,16 @@ static int iommu_init_device(struct device *dev)
 
 	alias = amd_iommu_alias_table[dev_data->devid];
 	if (alias != dev_data->devid) {
-		pdev = pci_get_bus_and_slot(PCI_BUS(alias), alias & 0xff);
-		if (pdev)
-			dev_data->alias = &pdev->dev;
-		else {
+		struct iommu_dev_data *alias_data;
+
+		alias_data = find_dev_data(alias);
+		if (alias_data == NULL) {
+			pr_err("AMD-Vi: Warning: Unhandled device %s\n",
+					dev_name(dev));
 			free_dev_data(dev_data);
 			return -ENOTSUPP;
 		}
+		dev_data->alias_data = alias_data;
 	}
 
 	dev->archdata.iommu = dev_data;
@@ -1642,14 +1644,8 @@ static int __attach_device(struct iommu_dev_data *dev_data,
 	/* lock domain */
 	spin_lock(&domain->lock);
 
-	if (dev_data->alias != NULL) {
-		struct iommu_dev_data *alias_data;
-
-		alias_data = get_dev_data(dev_data->alias);
-
-		ret = -EINVAL;
-		if (!alias_data)
-			goto out_unlock;
+	if (dev_data->alias_data != NULL) {
+		struct iommu_dev_data *alias_data = dev_data->alias_data;
 
 		/* Some sanity checks */
 		ret = -EBUSY;
@@ -1721,7 +1717,6 @@ static int attach_device(struct device *dev,
  */
 static void __detach_device(struct iommu_dev_data *dev_data)
 {
-	struct iommu_dev_data *alias_data;
 	struct protection_domain *domain;
 	unsigned long flags;
 
@@ -1731,8 +1726,9 @@ static void __detach_device(struct iommu_dev_data *dev_data)
 
 	spin_lock_irqsave(&domain->lock, flags);
 
-	if (dev_data->alias) {
-		alias_data = get_dev_data(dev_data->alias);
+	if (dev_data->alias_data != NULL) {
+		struct iommu_dev_data *alias_data = dev_data->alias_data;
+
 		if (atomic_dec_and_test(&alias_data->bind))
 			do_detach(alias_data);
 	}
@@ -1779,7 +1775,7 @@ static void detach_device(struct device *dev)
  */
 static struct protection_domain *domain_for_device(struct device *dev)
 {
-	struct iommu_dev_data *dev_data, *alias_data;
+	struct iommu_dev_data *dev_data;
 	struct protection_domain *dom = NULL;
 	unsigned long flags;
 
@@ -1788,8 +1784,8 @@ static struct protection_domain *domain_for_device(struct device *dev)
 	if (dev_data->domain)
 		return dev_data->domain;
 
-	if (dev_data->alias != NULL) {
-		alias_data = get_dev_data(dev_data->alias);
+	if (dev_data->alias_data != NULL) {
+		struct iommu_dev_data *alias_data = dev_data->alias_data;
 
 		read_lock_irqsave(&amd_iommu_devtable_lock, flags);
 		if (alias_data->domain != NULL) {
-- 
1.7.4.1