iommu/intel: Quirk non-compliant PCIe-to-PCI bridges

Some PCIe-to-PCI bridges are not fully compliant with the PCIe spec and do not include a PCIe capability. pci_is_pcie() is not useful on these devices, making it difficult to determine where we transition from a legacy PCI bus to a PCIe link. PCI-core doesn't want a quirked bridge test for fear of confusion that a PCIe capability would be expected, so implement it in IOMMU code.

Signed-off-by Alex Williamson <[email protected]>

Quoted 7 lines · On Tue, 28 May 2013 12:40:20 -0600 Alex Williamson…

Some PCIe-to-PCI bridges are not fully compliant with the PCIe spec and do not include a PCIe capability. pci_is_pcie() is not useful on these devices, making it difficult to determine where we transition from a legacy PCI bus to a PCIe link. PCI-core doesn't want a quirked bridge test for fear of confusion that a PCIe capability would be expected, so implement it in IOMMU code.

Quoted 1 line

Signed-off-by Alex Williamson <[email protected]>

This won't work for me because I run without IOMMU.
The overhead of IOMMU setup per-packet significantly impacts network performance.

Quoted 10 lines · Stephen Hemminger

Quoted 7 lines · On Tue, 28 May 2013 12:40:20 -0600 Alex Williamson…

Some PCIe-to-PCI bridges are not fully compliant with the PCIe spec and do not include a PCIe capability. pci_is_pcie() is not useful on these devices, making it difficult to determine where we transition from a legacy PCI bus to a PCIe link. PCI-core doesn't want a quirked bridge test for fear of confusion that a PCIe capability would be expected, so implement it in IOMMU code.

Quoted 4 lines

Quoted 1 line

Signed-off-by Alex Williamson <[email protected]>

This won't work for me because I run without IOMMU.
The overhead of IOMMU setup per-packet significantly impacts network performance.

You were one of the reporters of this issue:

http://www.spinics.net/lists/linux-pci/msg19579.html

Does this not work for you or are you saying this is not relevant to you because you no longer attempt to enable the IOMMU? Thanks,

Alex

Quoted 18 lines · On Tue, 28 May 2013 13:53:10 -0600 Alex Williamson…

Quoted 10 lines · Stephen Hemminger

Quoted 7 lines · On Tue, 28 May 2013 12:40:20 -0600 Alex Williamson…

Some PCIe-to-PCI bridges are not fully compliant with the PCIe spec and do not include a PCIe capability. pci_is_pcie() is not useful on these devices, making it difficult to determine where we transition from a legacy PCI bus to a PCIe link. PCI-core doesn't want a quirked bridge test for fear of confusion that a PCIe capability would be expected, so implement it in IOMMU code.

Quoted 4 lines

Quoted 1 line

Signed-off-by Alex Williamson <[email protected]>

This won't work for me because I run without IOMMU.
The overhead of IOMMU setup per-packet significantly impacts network performance.

You were one of the reporters of this issue:

Quoted 5 lines

http://www.spinics.net/lists/linux-pci/msg19579.html

Does this not work for you or are you saying this is not relevant to you because you no longer attempt to enable the IOMMU? Thanks,

For most usage, I gave up on using IOMMU/SR-IOV.

Quoted 27 lines · Stephen Hemminger

Quoted 23 lines · On Tue, 28 May 2013 13:53:10 -0600 Alex Williamson…

Quoted 10 lines · Stephen Hemminger

Quoted 7 lines · On Tue, 28 May 2013 12:40:20 -0600 Alex Williamson…

Some PCIe-to-PCI bridges are not fully compliant with the PCIe spec and do not include a PCIe capability. pci_is_pcie() is not useful on these devices, making it difficult to determine where we transition from a legacy PCI bus to a PCIe link. PCI-core doesn't want a quirked bridge test for fear of confusion that a PCIe capability would be expected, so implement it in IOMMU code.

Quoted 4 lines

Quoted 1 line

Signed-off-by Alex Williamson <[email protected]>

This won't work for me because I run without IOMMU.
The overhead of IOMMU setup per-packet significantly impacts network performance.

You were one of the reporters of this issue:

http://www.spinics.net/lists/linux-pci/msg19579.html

Does this not work for you or are you saying this is not relevant to you because you no longer attempt to enable the IOMMU? Thanks,

Quoted 1 line

For most usage, I gave up on using IOMMU/SR-IOV.

Ok, let's call that a "don't care" rather than "won't work" for the purposes of these patches. I'll take you off the CC if there's another version. Thanks,

Alex

drivers/iommu/pci.c new file +14 -0

+bool iommu_pci_is_pcie_bridge(struct pci_dev *pdev)
+{
+	if (!pdev->subordinate)
+		return false;
+
+	if (pci_is_pcie(pdev))
+		return true;
+
+#ifdef CONFIG_PCI_QUIRKS
+	/*
+	 * If we're not on the root bus, look one device upstream of the
+	 * current device.  If that device is PCIe and is not a PCIe-to-PCI
+	 * bridge, then the current device is effectively PCIe as it must
+	 * be the PCIe-to-PCI bridge.  This handles several bridges that

Quoted 18 lines · Alex Williamson

drivers/iommu/pci.c new file +18 -0

+bool iommu_pci_is_pcie_bridge(struct pci_dev *pdev)
+{
+	if (!pdev->subordinate)
+		return false;
+
+	if (pci_is_pcie(pdev))
+		return true;
+
+#ifdef CONFIG_PCI_QUIRKS
+	/*
+	 * If we're not on the root bus, look one device upstream of the
+	 * current device.  If that device is PCIe and is not a PCIe-to-PCI
+	 * bridge, then the current device is effectively PCIe as it must
+	 * be the PCIe-to-PCI bridge.  This handles several bridges that
+	 * violate the PCIe spec by not exposing a PCIe capability:
+	 * https://bugzilla.kernel.org/show_bug.cgi?id=44881
+	 */
+	if (!pci_is_root_bus(pdev->bus)) {

drivers/iommu/pci.c line 44

Quoted 6 lines

drivers/iommu/pci.c new file +6 -0

+		struct pci_dev *parent = pdev->bus->self;
+
+		if (pci_is_pcie(parent) &&
+		    pci_pcie_type(parent) != PCI_EXP_TYPE_PCI_BRIDGE)
+			return true;
+	}

Hmm, that looks a bit dangerous.

Do we have a list of PCI
vendor/device-ids of these broken bridges to match against instead of some open-coded heuristics?

That would probably also help to bring this into the PCI code.

Quoted 10 lines · Joerg Roedel

drivers/iommu/pci.c line 44

Quoted 7 lines · Alex Williamson

drivers/iommu/pci.c new file +7 -0

+	if (!pci_is_root_bus(pdev->bus)) {
+		struct pci_dev *parent = pdev->bus->self;
+
+		if (pci_is_pcie(parent) &&
+		    pci_pcie_type(parent) != PCI_EXP_TYPE_PCI_BRIDGE)
+			return true;
+	}

Hmm, that looks a bit dangerous.

How so? The algorithm seems pretty simple and logical.

Actually, I believe Bjorn rejected the idea of a fixed list because this problem is detectable. He also doesn't want me messing with quirks to pci_is_pcie() in PCI because he wants a 1:1 relation between that and having a PCIe capability. So, I'm stuck and this is where it's ended up. Thanks,

Quoted 10 lines · Alex Williamson

Quoted 9 lines · Joerg Roedel

Quoted 7 lines · Alex Williamson

+ if (!pci_is_root_bus(pdev->bus)) {

drivers/iommu/pci.c new file +6 -0

+         struct pci_dev *parent = pdev->bus->self;
+
+         if (pci_is_pcie(parent) &&
+             pci_pcie_type(parent) != PCI_EXP_TYPE_PCI_BRIDGE)
+                 return true;
+ }

Quoted 3 lines

Quoted 1 line

Hmm, that looks a bit dangerous.

How so? The algorithm seems pretty simple and logical.

It is simple, but it is still a heuristic that may fail at some point, no?

Quoted 5 lines

Actually, I believe Bjorn rejected the idea of a fixed list because this problem is detectable. He also doesn't want me messing with quirks to pci_is_pcie() in PCI because he wants a 1:1 relation between that and having a PCIe capability. So, I'm stuck and this is where it's ended up. Thanks,

I think implementing such a list is much safer.

Bjorn, why didn't you like that idea?

Quoted 24 lines · Joerg Roedel

Quoted 10 lines · Alex Williamson

Quoted 9 lines · Joerg Roedel

Quoted 7 lines · Alex Williamson

+ if (!pci_is_root_bus(pdev->bus)) {

(quoted diff) +6 -0

+         struct pci_dev *parent = pdev->bus->self;
+
+         if (pci_is_pcie(parent) &&
+             pci_pcie_type(parent) != PCI_EXP_TYPE_PCI_BRIDGE)
+                 return true;
+ }

Quoted 3 lines

Quoted 1 line

Hmm, that looks a bit dangerous.

How so? The algorithm seems pretty simple and logical.

It is simple, but it is still a heuristic that may fail at some point, no?

Quoted 5 lines

Actually, I believe Bjorn rejected the idea of a fixed list because this problem is detectable. He also doesn't want me messing with quirks to pci_is_pcie() in PCI because he wants a 1:1 relation between that and having a PCIe capability. So, I'm stuck and this is where it's ended up. Thanks,

Quoted 3 lines

I think implementing such a list is much safer.

Bjorn, why didn't you like that idea?

Sorry, I can't remember, and I haven't been able to find the discussion where I said that. I think the current patches are all in drivers/iommu, and if a list makes sense there, it's fine with me.

Quoted 25 lines · Bjorn Helgaas

Quoted 24 lines · Joerg Roedel

Quoted 10 lines · Alex Williamson

Quoted 9 lines · Joerg Roedel

Quoted 7 lines · Alex Williamson

+ if (!pci_is_root_bus(pdev->bus)) {

(quoted diff) +6 -0

+         struct pci_dev *parent = pdev->bus->self;
+
+         if (pci_is_pcie(parent) &&
+             pci_pcie_type(parent) != PCI_EXP_TYPE_PCI_BRIDGE)
+                 return true;
+ }

Quoted 3 lines

Quoted 1 line

Hmm, that looks a bit dangerous.

How so? The algorithm seems pretty simple and logical.

It is simple, but it is still a heuristic that may fail at some point, no?

Quoted 5 lines

Actually, I believe Bjorn rejected the idea of a fixed list because this problem is detectable. He also doesn't want me messing with quirks to pci_is_pcie() in PCI because he wants a 1:1 relation between that and having a PCIe capability. So, I'm stuck and this is where it's ended up. Thanks,

Quoted 7 lines

Quoted 3 lines

I think implementing such a list is much safer.

Bjorn, why didn't you like that idea?

Sorry, I can't remember, and I haven't been able to find the discussion where I said that. I think the current patches are all in drivers/iommu, and if a list makes sense there, it's fine with me.

Here's the comment I remember

https://bugzilla.kernel.org/show_bug.cgi?id=44881#c7

Comment #7 From Bjorn Helgaas 2012-08-23 15:58:39 [snip]
I doubt the upstream device is at fault. More likely the downstream device is really a PCIe device (a PCIe-to-PCI bridge) but just fails to report a PCIe capability. I think this situation is likely too common to deal with via quirks, so we'll have to figure out a way to just make this work.

This series tries to address:

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

Where pci_find_upstream_pcie_bridge() gets lost trying to find the upstream PCIe-to-PCI bridge for a device because the bridge doesn't expose a PCIe capability. To do this, we add a iommu_pci_is_pcie_bridge function which includes a quirk to look to the next upstream device as a sanity check. We can then replace pci_find_upstream_pcie_bridge with a function that's a bit more generic and less tied to intel-iommu eccentricities.

v2 uses the same logic as v1, but moves the search and match code to IOMMU-core since PCI-core doesn't want it. v1 has several reports from users that this solves the problem they have in the above bz. Thanks,

Alex

---

Alex Williamson (2):
iommu: Quirked PCIe bridge test and search function
intel-iommu: Convert to iommu_pci_find_upstream + iommu_pci_is_pcie_bridge

 drivers/iommu/Kconfig               |    5 ++
 drivers/iommu/Makefile              |    1
 drivers/iommu/intel-iommu.c         |   77 ++++++++++++++++++++++-------------
 drivers/iommu/intel_irq_remapping.c |   15 +++++--
 drivers/iommu/pci.c                 |   69 +++++++++++++++++++++++++++++++
 drivers/iommu/pci.h                 |   23 ++++++++++
 6 files changed, 157 insertions(+), 33 deletions(-)
 create mode 100644 drivers/iommu/pci.c

This series tries to address:

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

Where pci_find_upstream_pcie_bridge() gets lost trying to find the upstream PCIe-to-PCI bridge for a device because the bridge doesn't expose a PCIe capability. To do this, we add a iommu_pci_is_pcie_bridge function which includes a quirk to look to the next upstream device as a sanity check. We can then replace pci_find_upstream_pcie_bridge with a function that's a bit more generic and less tied to intel-iommu eccentricities.

v2 uses the same logic as v1, but moves the search and match code to IOMMU-core since PCI-core doesn't want it. v1 has several reports from users that this solves the problem they have in the above bz. Thanks,

Alex

---

Alex Williamson (2):
iommu: Quirked PCIe bridge test and search function
intel-iommu: Convert to iommu_pci_find_upstream + iommu_pci_is_pcie_bridge

 drivers/iommu/Kconfig               |    5 ++
 drivers/iommu/Makefile              |    1 
 drivers/iommu/intel-iommu.c         |   77 ++++++++++++++++++++++-------------
 drivers/iommu/intel_irq_remapping.c |   15 +++++--
 drivers/iommu/pci.c                 |   69 +++++++++++++++++++++++++++++++
 drivers/iommu/pci.h                 |   23 ++++++++++
 6 files changed, 157 insertions(+), 33 deletions(-)
 create mode 100644 drivers/iommu/pci.c

Joerg,

Where do we stand on this series? You had a concern that the heuristic used in patch 1/ could be dangerous. The suggestion for detecting the issue was actually from Bjorn who replied with his rationale. Do you want to go in the direction of a fixed whitelist or do you agree that even if the heuristic breaks it provides better behavior than what we have now? Thanks,

Quoted 8 lines · Alex Williamson

Joerg,

Where do we stand on this series? You had a concern that the heuristic used in patch 1/ could be dangerous. The suggestion for detecting the issue was actually from Bjorn who replied with his rationale. Do you want to go in the direction of a fixed whitelist or do you agree that even if the heuristic breaks it provides better behavior than what we have now? Thanks,

I'm trying to take a step back and look at the overall design, not these specific patches.

IOMMUs translate addresses based on their source, i.e., a PCIe requester ID. This is made more complicated by the fact that some bridges "take ownership" (change the requester ID as they forward transactions upstream), as well as the fact that conventional PCI has no requester ID at all. And some broken devices apparently generate DMA requests using the requester ID of another device.

We currently deal with this using the pci_find_upstream_pcie_bridge() and pci_get_dma_source() interfaces, but I think there's too much assembly required by their users. pci_find_upstream_pcie_bridge() callers normally loop through all the bridges between the "upstream PCIe bridge" and the device, checking for bridges that might take ownership. They probably also ought to use pci_get_dma_source() to account for the broken devices, but most callers don't.

Most of this is PCI-specific stuff that should be of interest to all IOMMU drivers, and the overall structure of calls and looping should be the same for all of them, so it would be nice to factor it out somehow.

The attached patch is guaranteed not to even compile; it's just to make this idea more concrete. The basic idea is that since the IOMMU driver wants to perform some action for each possible requester ID the IOMMU might see, PCI could provide an iterator ("pci_for_each_requester_id()") to do that.

Bjorn

commit afad51492c6672b96c2b0735600d5695e30f7180

Author: Bjorn Helgaas <[email protected]>
Date:   Wed Jul 3 16:04:26 2013 -0600

pci-add-for-each-requester-id

drivers/pci/search.c +7 -0

index d0627fa..380eb03 100644
@@ -17,6 +17,89 @@
 DECLARE_RWSEM(pci_bus_sem);
 EXPORT_SYMBOL_GPL(pci_bus_sem);
 
+#define PCI_REQUESTER_ID(dev)	(((dev)->bus->number << 8) | (dev)->devfn)
+#define PCI_BRIDGE_REQUESTER_ID(bridge)	((bridge)->subordinate->number << 8)
+
+static inline bool pci_is_pcix(struct pci_dev *dev)
+{
+	return !!pci_pcix_cap(dev);	/* XXX not implemented */
+}

drivers/pci/search.c +34 -0

+
+	/*
+	 * A PCI-X to PCI-X bridge need not take ownership because there
+	 * are requester IDs on the secondary PCI-X bus.  However, if a PCI
+	 * device is added on the secondary bus, the bridge must revert to
+	 * being a PCI-X to PCI bridge, and it then *would* take ownership.
+	 * Assuming a PCI-X to PCI-X bridge takes ownership means we can
+	 * tolerate a future hot-add without having to change existing
+	 * IOMMU mappings.
+	 */
+	if (pci_is_pcix(bridge))
+		return true;
+
+	return false;
+}
+
+static struct pci_dev *pci_bridge_to_dev(struct pci_bus *bus,
+					 struct pci_dev *dev)
+{
+	struct pci_dev *bridge;
+	struct pci_bus *child_bus;
+	u8 secondary, subordinate, busn = dev->bus->number;
+
+	if (dev->bus == bus)
+		return dev;
+
+	/*
+	 * There may be several devices on "bus".  Find the one that is a
+	 * bridge leading to "dev".
+	 */
+	list_for_each_entry(bridge, &bus->devices, bus_list) {
+		child_bus = bridge->subordinate;
+		if (child_bus) {
+			secondary = child_bus->busn_res.start;

drivers/pci/search.c +15 -0

+		if (pci_bridge_may_take_ownership(bridge)) {
+			ret = fn(dev, PCI_BRIDGE_REQUESTER_ID(bridge), data);
+			if (ret)
+				return ret;
+		}
+		bridge = pci_bridge_to_dev(bridge->subordinate, dev);
+	}
+
+	if (pci_is_pcie(dev) || pci_is_pcix(dev))
+		return fn(dev, PCI_REQUESTER_ID(dev), data);
+
+	/* Conventional PCI has no requester ID */
+	return 0;
+}
+
 /*
  * find the upstream PCIe-to-PCI bridge of a PCI device
  * if the device is PCIE, return NULL

include/linux/pci.h +2 -0

index 3a24e4f..cbcc82f 100644
@@ -794,6 +794,8 @@ static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus,
 }
 struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from);
 int pci_dev_present(const struct pci_device_id *ids);
+void pci_for_each_requester_id(struct pci_dev *bridge, struct pci_dev *dev,
+			       int (*fn)(struct pci_dev *, void *), void *data);
 
 int pci_bus_read_config_byte(struct pci_bus *bus, unsigned int devfn,
 			     int where, u8 *val);

commit 6e770d7bf4ef95207df4ac4c42ef4406ff7bc54e

Author: Bjorn Helgaas <[email protected]>
Date:   Wed Jul 3 12:02:15 2013 -0600

intel-iommu-upstream-pcie

drivers/iommu/intel-iommu.c +7 -2

+
+	return domain_context_mapping_one(data->domain, segment,
+					  bus, devfn, data->translation);
+}
+
 static int domain_context_mapping(struct dmar_domain *domain,
 				  struct pci_dev *pdev, int translation)
 {
-	int ret;
-	struct pci_dev *tmp, *parent;
+	struct domain_context_info data;
+	struct pci_dev *bridge;

drivers/iommu/intel-iommu.c +65 -92

+	return pci_for_each_requester_id(bridge, pdev, context_mapping, &data);
+}
 
-	/* dependent device mapping */
-	tmp = pci_find_upstream_pcie_bridge(pdev);
-	if (!tmp)
-		return 0;
-	/* Secondary interface's bus number and devfn 0 */
-	parent = pdev->bus->self;
-	while (parent != tmp) {
-		ret = domain_context_mapping_one(domain,
-						 pci_domain_nr(parent->bus),
-						 parent->bus->number,
-						 parent->devfn, translation);
-		if (ret)
-			return ret;
-		parent = parent->bus->self;
-	}
-	if (pci_is_pcie(tmp)) /* this is a PCIe-to-PCI bridge */
-		return domain_context_mapping_one(domain,
-					pci_domain_nr(tmp->subordinate),
-					tmp->subordinate->number, 0,
-					translation);
-	else /* this is a legacy PCI bridge */
-		return domain_context_mapping_one(domain,
-						  pci_domain_nr(tmp->bus),
-						  tmp->bus->number,
-						  tmp->devfn,
-						  translation);
+static int is_context_mapped(struct pci_dev *dev, u16 requester_id, void *data)
+{
+	struct domain_context_info *dev_info = data;
+	u8 bus = data->requester_id >> 8;
+	u8 devfn = data->requester_id & 0xff;
+	bool ret;
+
+	ret = device_context_mapped(dev_info->iommu, bus, devfn);
+	if (!ret)
+		dev_info->mapped = false;
+
+	return 0;
 }
 
 static bool domain_context_mapped(struct pci_dev *pdev)
 {
-	bool ret;
-	struct pci_dev *tmp, *parent;
+	struct domain_context_info data;
 	struct intel_iommu *iommu;
+	struct pci_dev *bridge;
 
 	iommu = device_to_iommu(pci_domain_nr(pdev->bus), pdev->bus->number,
 				pdev->devfn);
 	if (!iommu)
 		return false;
 
-	ret = device_context_mapped(iommu, pdev->bus->number, pdev->devfn);
-	if (!ret)
-		return false;
-	/* dependent device mapping */
-	tmp = pci_find_upstream_pcie_bridge(pdev);
-	if (!tmp)
-		return true;
-	/* Secondary interface's bus number and devfn 0 */
-	parent = pdev->bus->self;
-	while (parent != tmp) {
-		ret = device_context_mapped(iommu, parent->bus->number,
-					    parent->devfn);
-		if (!ret)
-			return false;
-		parent = parent->bus->self;
-	}
-	if (pci_is_pcie(tmp))
-		return device_context_mapped(iommu, tmp->subordinate->number,
-					     0);
-	else
-		return device_context_mapped(iommu, tmp->bus->number,
-					     tmp->devfn);
+	data.iommu = iommu;
+	data.mapped = true;
+	bridge = xx;
+	pci_for_each_requester_id(bridge, pdev, is_context_mapped, &data);
+	return data.mapped;
 }
 
 /* Returns a number of VTD pages, but aligned to MM page size */

@@ -1961,6 +1950,27 @@ static struct dmar_domain *find_domain(struct pci_dev *pdev)
 	return NULL;
 }
 
+static int find_domain2(struct pci_dev *dev, u16 requester_id, void *data)
+{
+	struct domain_context_info *dev_info = data;
+	int segment = pci_domain_nr(data->dev);
+	u8 bus = data->requester_id >> 8;
+	u8 devfn = data->requester_id & 0xff;
+	unsigned long flags;
+	struct device_domain_info *info;
+
+	spin_lock_irqsave(&device_domain_lock, flags);
+	list_for_each_entry(info, &device_domain_list, global) {
+		if (info->segment == segment &&
+		    info->bus == bus && info->devfn == devfn) {
+			data->domain = info->domain;
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&device_domain_lock, flags);
+	return 0;
+}
+
 /* domain is initialized */
 static struct dmar_domain *get_domain_for_dev(struct pci_dev *pdev, int gaw)
 {

@@ -1968,11 +1978,11 @@ static struct dmar_domain *get_domain_for_dev(struct pci_dev *pdev, int gaw)
 	struct intel_iommu *iommu;
 	struct dmar_drhd_unit *drhd;
 	struct device_domain_info *info, *tmp;
-	struct pci_dev *dev_tmp;
 	unsigned long flags;
 	int bus = 0, devfn = 0;
 	int segment;
 	int ret;
+	struct domain_context_info data;
 
 	domain = find_domain(pdev);
 	if (domain)

@@ -1980,29 +1990,13 @@ static struct dmar_domain *get_domain_for_dev(struct pci_dev *pdev, int gaw)
 
 	segment = pci_domain_nr(pdev->bus);
 
-	dev_tmp = pci_find_upstream_pcie_bridge(pdev);
-	if (dev_tmp) {
-		if (pci_is_pcie(dev_tmp)) {
-			bus = dev_tmp->subordinate->number;
-			devfn = 0;
-		} else {
-			bus = dev_tmp->bus->number;
-			devfn = dev_tmp->devfn;
-		}
-		spin_lock_irqsave(&device_domain_lock, flags);
-		list_for_each_entry(info, &device_domain_list, global) {
-			if (info->segment == segment &&
-			    info->bus == bus && info->devfn == devfn) {
-				found = info->domain;
-				break;
-			}
-		}
-		spin_unlock_irqrestore(&device_domain_lock, flags);
-		/* pcie-pci bridge already has a domain, uses it */
-		if (found) {
-			domain = found;
-			goto found_domain;
-		}
+	data.domain = NULL;
+	data.dev = pdev;
+	bridge = xx;
+	pci_for_each_requester_id(bridge, pdev, find_domain2, &data);
+	if (data.domain) {
+		domain = data.domain;
+		goto found_domain;
 	}
 
 	domain = alloc_domain();

@@ -3734,31 +3728,29 @@ int __init intel_iommu_init(void)
 	return 0;
 }
 
+static int detach_requester(struct pci_dev *dev, u16 requester_id, void *data)
+{
+	struct domain_context_info *dev_info = data;
+	struct intel_iommu *iommu = data->iommu;
+	u8 bus = data->requester_id >> 8;
+	u8 devfn = data->requester_id & 0xff;
+
+	iommu_detach_dev(iommu, bus, devfn);
+	return 0;
+}
+
 static void iommu_detach_dependent_devices(struct intel_iommu *iommu,
 					   struct pci_dev *pdev)
 {
-	struct pci_dev *tmp, *parent;
+	struct domain_context_info data;
 
 	if (!iommu || !pdev)
 		return;
 
 	/* dependent device detach */
-	tmp = pci_find_upstream_pcie_bridge(pdev);
-	/* Secondary interface's bus number and devfn 0 */
-	if (tmp) {
-		parent = pdev->bus->self;
-		while (parent != tmp) {
-			iommu_detach_dev(iommu, parent->bus->number,
-					 parent->devfn);
-			parent = parent->bus->self;
-		}
-		if (pci_is_pcie(tmp)) /* this is a PCIe-to-PCI bridge */
-			iommu_detach_dev(iommu,
-				tmp->subordinate->number, 0);
-		else /* this is a legacy PCI bridge */
-			iommu_detach_dev(iommu, tmp->bus->number,
-					 tmp->devfn);
-	}
+	data.iommu = iommu;
+	bridge = xx;
+	pci_for_each_requester_id(bridge, pdev, detach_requester, &data);
 }
 
 static void domain_remove_one_dev_info(struct dmar_domain *domain,
--

To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 68 lines · Bjorn Helgaas

Quoted 8 lines · Alex Williamson

Joerg,

Where do we stand on this series? You had a concern that the heuristic used in patch 1/ could be dangerous. The suggestion for detecting the issue was actually from Bjorn who replied with his rationale. Do you want to go in the direction of a fixed whitelist or do you agree that even if the heuristic breaks it provides better behavior than what we have now? Thanks,

I'm trying to take a step back and look at the overall design, not these specific patches.

IOMMUs translate addresses based on their source, i.e., a PCIe requester ID. This is made more complicated by the fact that some bridges "take ownership" (change the requester ID as they forward transactions upstream), as well as the fact that conventional PCI has no requester ID at all. And some broken devices apparently generate DMA requests using the requester ID of another device.

We currently deal with this using the pci_find_upstream_pcie_bridge() and pci_get_dma_source() interfaces, but I think there's too much assembly required by their users. pci_find_upstream_pcie_bridge() callers normally loop through all the bridges between the "upstream PCIe bridge" and the device, checking for bridges that might take ownership. They probably also ought to use pci_get_dma_source() to account for the broken devices, but most callers don't.

Most of this is PCI-specific stuff that should be of interest to all IOMMU drivers, and the overall structure of calls and looping should be the same for all of them, so it would be nice to factor it out somehow.

The attached patch is guaranteed not to even compile; it's just to make this idea more concrete. The basic idea is that since the IOMMU driver wants to perform some action for each possible requester ID the IOMMU might see, PCI could provide an iterator ("pci_for_each_requester_id()") to do that.

Bjorn

commit afad51492c6672b96c2b0735600d5695e30f7180

Author: Bjorn Helgaas <[email protected]>
Date:   Wed Jul 3 16:04:26 2013 -0600

pci-add-for-each-requester-id

drivers/pci/search.c +12 -0

index d0627fa..380eb03 100644
@@ -17,6 +17,89 @@
 DECLARE_RWSEM(pci_bus_sem);
 EXPORT_SYMBOL_GPL(pci_bus_sem);
 
+#define PCI_REQUESTER_ID(dev)	(((dev)->bus->number << 8) | (dev)->devfn)
+#define PCI_BRIDGE_REQUESTER_ID(bridge)	((bridge)->subordinate->number << 8)
+
+static inline bool pci_is_pcix(struct pci_dev *dev)
+{
+	return !!pci_pcix_cap(dev);	/* XXX not implemented */
+}
+
+static bool pci_bridge_may_take_ownership(struct pci_dev *bridge)
+{
+	/*
+	 * A PCIe to PCI/PCI-X bridge may take ownership per PCIe Bridge

drivers/pci/search.c line 36

Quoted 5 lines

drivers/pci/search.c +5 -0

+	 * Spec v1.0, sec 2.3.
+	 */
+	if (pci_is_pcie(bridge) &&
+	    pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)
+		return true;

Assume we still need a quirk here, PCIe-to-PCI bridges without a PCIe capability are exactly what causes the current bug.

Quoted 34 lines

drivers/pci/search.c +34 -0

+
+	/*
+	 * A PCI-X to PCI-X bridge need not take ownership because there
+	 * are requester IDs on the secondary PCI-X bus.  However, if a PCI
+	 * device is added on the secondary bus, the bridge must revert to
+	 * being a PCI-X to PCI bridge, and it then *would* take ownership.
+	 * Assuming a PCI-X to PCI-X bridge takes ownership means we can
+	 * tolerate a future hot-add without having to change existing
+	 * IOMMU mappings.
+	 */
+	if (pci_is_pcix(bridge))
+		return true;
+
+	return false;
+}
+
+static struct pci_dev *pci_bridge_to_dev(struct pci_bus *bus,
+					 struct pci_dev *dev)
+{
+	struct pci_dev *bridge;
+	struct pci_bus *child_bus;
+	u8 secondary, subordinate, busn = dev->bus->number;
+
+	if (dev->bus == bus)
+		return dev;
+
+	/*
+	 * There may be several devices on "bus".  Find the one that is a
+	 * bridge leading to "dev".
+	 */
+	list_for_each_entry(bridge, &bus->devices, bus_list) {
+		child_bus = bridge->subordinate;
+		if (child_bus) {
+			secondary = child_bus->busn_res.start;

drivers/pci/search.c line 80

Quoted 10 lines

drivers/pci/search.c +10 -0

+			subordinate = child_bus->busn_res.end;
+			if (secondary <= busn && busn <= subordinate)
+				return bridge;
+		}
+	}
+	return NULL;
+}
+
+int pci_for_each_requester_id(struct pci_dev *bridge, struct pci_dev *dev,
+			      int (*fn)(struct pci_dev *, void *),

u16 requester_id

drivers/pci/search.c line 85

Quoted 5 lines

drivers/pci/search.c +5 -0

+			      void *data)
+{
+	int ret;
+
+	dev = pci_get_dma_source(dev);	/* XXX ref count screwup */

It's unfortunate that our dma quirk still seems to only get called for the end device where we could have bridges that need it too.

drivers/pci/search.c line 87

Quoted 2 lines

drivers/pci/search.c +2 -0

+
+	while (bridge != dev) {

I'm having a hard time understanding this function since bridge is never defined in any of the below sample code. Would bridge be the root port above a device? And what about devices connected to the RC? Seems like we'd need to traverse up the bus first, so there's a bit of missing magic.

Quoted 32 lines

drivers/pci/search.c +15 -0

+		if (pci_bridge_may_take_ownership(bridge)) {
+			ret = fn(dev, PCI_BRIDGE_REQUESTER_ID(bridge), data);
+			if (ret)
+				return ret;
+		}
+		bridge = pci_bridge_to_dev(bridge->subordinate, dev);
+	}
+
+	if (pci_is_pcie(dev) || pci_is_pcix(dev))
+		return fn(dev, PCI_REQUESTER_ID(dev), data);
+
+	/* Conventional PCI has no requester ID */
+	return 0;
+}
+
 /*
  * find the upstream PCIe-to-PCI bridge of a PCI device
  * if the device is PCIE, return NULL

include/linux/pci.h +2 -0

index 3a24e4f..cbcc82f 100644
@@ -794,6 +794,8 @@ static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus,
 }
 struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from);
 int pci_dev_present(const struct pci_device_id *ids);
+void pci_for_each_requester_id(struct pci_dev *bridge, struct pci_dev *dev,
+			       int (*fn)(struct pci_dev *, void *), void *data);
 
 int pci_bus_read_config_byte(struct pci_bus *bus, unsigned int devfn,
 			     int where, u8 *val);

Quoted 5 lines

commit 6e770d7bf4ef95207df4ac4c42ef4406ff7bc54e

Author: Bjorn Helgaas <[email protected]>
Date:   Wed Jul 3 12:02:15 2013 -0600

intel-iommu-upstream-pcie

I assume this one would is also going to apply dma quirks to dma_ops, which I'm all for.

drivers/iommu/intel-iommu.c static int domain_context_mapping_one(struct dmar_domain *domain, int segment, · line 1676

Quoted 13 lines

drivers/iommu/intel-iommu.c +5 -0

index b91564d..bdb6e6a 100644
@@ -1669,79 +1669,68 @@ static int domain_context_mapping_one(struct dmar_domain *domain, int segment,
 	return 0;
 }
 
+struct domain_context_info {
+	struct dmar_domain *domain;
+	struct pci_dev *dev;
+	int translation;
+	struct intel_iommu *iomumu;

s/iomumu/iommu/

drivers/iommu/intel-iommu.c static int domain_context_mapping_one(struct dmar_domain *domain, int segment, · line 1683

Quoted 7 lines

drivers/iommu/intel-iommu.c +7 -0

+	bool mapped;
+};
+
+static int context_mapping(struct pci_dev *dev, u16 requester_id, void *data)
+{
+	struct domain_context_info *dev_info = data;
+	int segment = pci_domain_nr(data->dev);

dev_info->dev

drivers/iommu/intel-iommu.c static int domain_context_mapping_one(struct dmar_domain *domain, int segment, · line 1684

Quoted 1 line

drivers/iommu/intel-iommu.c +1 -0

+	u8 bus = data->requester_id >> 8;

s/data->//

drivers/iommu/intel-iommu.c static int domain_context_mapping_one(struct dmar_domain *domain, int segment, · line 1685

Quoted 1 line

drivers/iommu/intel-iommu.c +1 -0

+	u8 devfn = data->requester_id & 0xff;

s/data->//

Quoted 12 lines

drivers/iommu/intel-iommu.c +7 -2

+
+	return domain_context_mapping_one(data->domain, segment,
+					  bus, devfn, data->translation);
+}
+
 static int domain_context_mapping(struct dmar_domain *domain,
 				  struct pci_dev *pdev, int translation)
 {
-	int ret;
-	struct pci_dev *tmp, *parent;
+	struct domain_context_info data;
+	struct pci_dev *bridge;

drivers/iommu/intel-iommu.c static int domain_context_mapping_one(struct dmar_domain *domain, int segment, · line 1700

Quoted 10 lines

drivers/iommu/intel-iommu.c +4 -5

 
-	ret = domain_context_mapping_one(domain, pci_domain_nr(pdev->bus),
-					 pdev->bus->number, pdev->devfn,
-					 translation);
-	if (ret)
-		return ret;
+	data.domain = domain;
+	data.dev = pdev;
+	data.translation = translation;
+	bridge = xx;

Darn, I was hoping you were going to reveal where bridge comes from.

Quoted 215 lines

drivers/iommu/intel-iommu.c +65 -92

+	return pci_for_each_requester_id(bridge, pdev, context_mapping, &data);
+}
 
-	/* dependent device mapping */
-	tmp = pci_find_upstream_pcie_bridge(pdev);
-	if (!tmp)
-		return 0;
-	/* Secondary interface's bus number and devfn 0 */
-	parent = pdev->bus->self;
-	while (parent != tmp) {
-		ret = domain_context_mapping_one(domain,
-						 pci_domain_nr(parent->bus),
-						 parent->bus->number,
-						 parent->devfn, translation);
-		if (ret)
-			return ret;
-		parent = parent->bus->self;
-	}
-	if (pci_is_pcie(tmp)) /* this is a PCIe-to-PCI bridge */
-		return domain_context_mapping_one(domain,
-					pci_domain_nr(tmp->subordinate),
-					tmp->subordinate->number, 0,
-					translation);
-	else /* this is a legacy PCI bridge */
-		return domain_context_mapping_one(domain,
-						  pci_domain_nr(tmp->bus),
-						  tmp->bus->number,
-						  tmp->devfn,
-						  translation);
+static int is_context_mapped(struct pci_dev *dev, u16 requester_id, void *data)
+{
+	struct domain_context_info *dev_info = data;
+	u8 bus = data->requester_id >> 8;
+	u8 devfn = data->requester_id & 0xff;
+	bool ret;
+
+	ret = device_context_mapped(dev_info->iommu, bus, devfn);
+	if (!ret)
+		dev_info->mapped = false;
+
+	return 0;
 }
 
 static bool domain_context_mapped(struct pci_dev *pdev)
 {
-	bool ret;
-	struct pci_dev *tmp, *parent;
+	struct domain_context_info data;
 	struct intel_iommu *iommu;
+	struct pci_dev *bridge;
 
 	iommu = device_to_iommu(pci_domain_nr(pdev->bus), pdev->bus->number,
 				pdev->devfn);
 	if (!iommu)
 		return false;
 
-	ret = device_context_mapped(iommu, pdev->bus->number, pdev->devfn);
-	if (!ret)
-		return false;
-	/* dependent device mapping */
-	tmp = pci_find_upstream_pcie_bridge(pdev);
-	if (!tmp)
-		return true;
-	/* Secondary interface's bus number and devfn 0 */
-	parent = pdev->bus->self;
-	while (parent != tmp) {
-		ret = device_context_mapped(iommu, parent->bus->number,
-					    parent->devfn);
-		if (!ret)
-			return false;
-		parent = parent->bus->self;
-	}
-	if (pci_is_pcie(tmp))
-		return device_context_mapped(iommu, tmp->subordinate->number,
-					     0);
-	else
-		return device_context_mapped(iommu, tmp->bus->number,
-					     tmp->devfn);
+	data.iommu = iommu;
+	data.mapped = true;
+	bridge = xx;
+	pci_for_each_requester_id(bridge, pdev, is_context_mapped, &data);
+	return data.mapped;
 }
 
 /* Returns a number of VTD pages, but aligned to MM page size */

@@ -1961,6 +1950,27 @@ static struct dmar_domain *find_domain(struct pci_dev *pdev)
 	return NULL;
 }
 
+static int find_domain2(struct pci_dev *dev, u16 requester_id, void *data)
+{
+	struct domain_context_info *dev_info = data;
+	int segment = pci_domain_nr(data->dev);
+	u8 bus = data->requester_id >> 8;
+	u8 devfn = data->requester_id & 0xff;
+	unsigned long flags;
+	struct device_domain_info *info;
+
+	spin_lock_irqsave(&device_domain_lock, flags);
+	list_for_each_entry(info, &device_domain_list, global) {
+		if (info->segment == segment &&
+		    info->bus == bus && info->devfn == devfn) {
+			data->domain = info->domain;
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&device_domain_lock, flags);
+	return 0;
+}
+
 /* domain is initialized */
 static struct dmar_domain *get_domain_for_dev(struct pci_dev *pdev, int gaw)
 {

@@ -1968,11 +1978,11 @@ static struct dmar_domain *get_domain_for_dev(struct pci_dev *pdev, int gaw)
 	struct intel_iommu *iommu;
 	struct dmar_drhd_unit *drhd;
 	struct device_domain_info *info, *tmp;
-	struct pci_dev *dev_tmp;
 	unsigned long flags;
 	int bus = 0, devfn = 0;
 	int segment;
 	int ret;
+	struct domain_context_info data;
 
 	domain = find_domain(pdev);
 	if (domain)

@@ -1980,29 +1990,13 @@ static struct dmar_domain *get_domain_for_dev(struct pci_dev *pdev, int gaw)
 
 	segment = pci_domain_nr(pdev->bus);
 
-	dev_tmp = pci_find_upstream_pcie_bridge(pdev);
-	if (dev_tmp) {
-		if (pci_is_pcie(dev_tmp)) {
-			bus = dev_tmp->subordinate->number;
-			devfn = 0;
-		} else {
-			bus = dev_tmp->bus->number;
-			devfn = dev_tmp->devfn;
-		}
-		spin_lock_irqsave(&device_domain_lock, flags);
-		list_for_each_entry(info, &device_domain_list, global) {
-			if (info->segment == segment &&
-			    info->bus == bus && info->devfn == devfn) {
-				found = info->domain;
-				break;
-			}
-		}
-		spin_unlock_irqrestore(&device_domain_lock, flags);
-		/* pcie-pci bridge already has a domain, uses it */
-		if (found) {
-			domain = found;
-			goto found_domain;
-		}
+	data.domain = NULL;
+	data.dev = pdev;
+	bridge = xx;
+	pci_for_each_requester_id(bridge, pdev, find_domain2, &data);
+	if (data.domain) {
+		domain = data.domain;
+		goto found_domain;
 	}
 
 	domain = alloc_domain();

@@ -3734,31 +3728,29 @@ int __init intel_iommu_init(void)
 	return 0;
 }
 
+static int detach_requester(struct pci_dev *dev, u16 requester_id, void *data)
+{
+	struct domain_context_info *dev_info = data;
+	struct intel_iommu *iommu = data->iommu;
+	u8 bus = data->requester_id >> 8;
+	u8 devfn = data->requester_id & 0xff;
+
+	iommu_detach_dev(iommu, bus, devfn);
+	return 0;
+}
+
 static void iommu_detach_dependent_devices(struct intel_iommu *iommu,
 					   struct pci_dev *pdev)
 {
-	struct pci_dev *tmp, *parent;
+	struct domain_context_info data;
 
 	if (!iommu || !pdev)
 		return;
 
 	/* dependent device detach */
-	tmp = pci_find_upstream_pcie_bridge(pdev);
-	/* Secondary interface's bus number and devfn 0 */
-	if (tmp) {
-		parent = pdev->bus->self;
-		while (parent != tmp) {
-			iommu_detach_dev(iommu, parent->bus->number,
-					 parent->devfn);
-			parent = parent->bus->self;
-		}
-		if (pci_is_pcie(tmp)) /* this is a PCIe-to-PCI bridge */
-			iommu_detach_dev(iommu,
-				tmp->subordinate->number, 0);
-		else /* this is a legacy PCI bridge */
-			iommu_detach_dev(iommu, tmp->bus->number,
-					 tmp->devfn);
-	}
+	data.iommu = iommu;
+	bridge = xx;
+	pci_for_each_requester_id(bridge, pdev, detach_requester, &data);
 }
 
 static void domain_remove_one_dev_info(struct dmar_domain *domain,
--

To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 69 lines · Alex Williamson

Quoted 68 lines · Bjorn Helgaas

Quoted 8 lines · Alex Williamson

Joerg,

Where do we stand on this series? You had a concern that the heuristic used in patch 1/ could be dangerous. The suggestion for detecting the issue was actually from Bjorn who replied with his rationale. Do you want to go in the direction of a fixed whitelist or do you agree that even if the heuristic breaks it provides better behavior than what we have now? Thanks,

I'm trying to take a step back and look at the overall design, not these specific patches.

IOMMUs translate addresses based on their source, i.e., a PCIe requester ID. This is made more complicated by the fact that some bridges "take ownership" (change the requester ID as they forward transactions upstream), as well as the fact that conventional PCI has no requester ID at all. And some broken devices apparently generate DMA requests using the requester ID of another device.

We currently deal with this using the pci_find_upstream_pcie_bridge() and pci_get_dma_source() interfaces, but I think there's too much assembly required by their users. pci_find_upstream_pcie_bridge() callers normally loop through all the bridges between the "upstream PCIe bridge" and the device, checking for bridges that might take ownership. They probably also ought to use pci_get_dma_source() to account for the broken devices, but most callers don't.

Most of this is PCI-specific stuff that should be of interest to all IOMMU drivers, and the overall structure of calls and looping should be the same for all of them, so it would be nice to factor it out somehow.

The attached patch is guaranteed not to even compile; it's just to make this idea more concrete. The basic idea is that since the IOMMU driver wants to perform some action for each possible requester ID the IOMMU might see, PCI could provide an iterator ("pci_for_each_requester_id()") to do that.

Bjorn

commit afad51492c6672b96c2b0735600d5695e30f7180

Author: Bjorn Helgaas <[email protected]>
Date:   Wed Jul 3 16:04:26 2013 -0600

pci-add-for-each-requester-id

drivers/pci/search.c +12 -0

index d0627fa..380eb03 100644
@@ -17,6 +17,89 @@
 DECLARE_RWSEM(pci_bus_sem);
 EXPORT_SYMBOL_GPL(pci_bus_sem);
 
+#define PCI_REQUESTER_ID(dev)	(((dev)->bus->number << 8) | (dev)->devfn)
+#define PCI_BRIDGE_REQUESTER_ID(bridge)	((bridge)->subordinate->number << 8)
+
+static inline bool pci_is_pcix(struct pci_dev *dev)
+{
+	return !!pci_pcix_cap(dev);	/* XXX not implemented */
+}
+
+static bool pci_bridge_may_take_ownership(struct pci_dev *bridge)
+{
+	/*
+	 * A PCIe to PCI/PCI-X bridge may take ownership per PCIe Bridge

Quoted 8 lines

drivers/pci/search.c line 36

Quoted 5 lines

drivers/pci/search.c +5 -0

+	 * Spec v1.0, sec 2.3.
+	 */
+	if (pci_is_pcie(bridge) &&
+	    pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)
+		return true;

Assume we still need a quirk here, PCIe-to-PCI bridges without a PCIe capability are exactly what causes the current bug.

Likely so. We might be able to identify that as "secondary bus is not PCIe" or something.

Quoted 47 lines

Quoted 34 lines

drivers/pci/search.c +34 -0

+
+	/*
+	 * A PCI-X to PCI-X bridge need not take ownership because there
+	 * are requester IDs on the secondary PCI-X bus.  However, if a PCI
+	 * device is added on the secondary bus, the bridge must revert to
+	 * being a PCI-X to PCI bridge, and it then *would* take ownership.
+	 * Assuming a PCI-X to PCI-X bridge takes ownership means we can
+	 * tolerate a future hot-add without having to change existing
+	 * IOMMU mappings.
+	 */
+	if (pci_is_pcix(bridge))
+		return true;
+
+	return false;
+}
+
+static struct pci_dev *pci_bridge_to_dev(struct pci_bus *bus,
+					 struct pci_dev *dev)
+{
+	struct pci_dev *bridge;
+	struct pci_bus *child_bus;
+	u8 secondary, subordinate, busn = dev->bus->number;
+
+	if (dev->bus == bus)
+		return dev;
+
+	/*
+	 * There may be several devices on "bus".  Find the one that is a
+	 * bridge leading to "dev".
+	 */
+	list_for_each_entry(bridge, &bus->devices, bus_list) {
+		child_bus = bridge->subordinate;
+		if (child_bus) {
+			secondary = child_bus->busn_res.start;

drivers/pci/search.c line 80

Quoted 10 lines

drivers/pci/search.c +10 -0

+			subordinate = child_bus->busn_res.end;
+			if (secondary <= busn && busn <= subordinate)
+				return bridge;
+		}
+	}
+	return NULL;
+}
+
+int pci_for_each_requester_id(struct pci_dev *bridge, struct pci_dev *dev,
+			      int (*fn)(struct pci_dev *, void *),

u16 requester_id

Quoted 8 lines

drivers/pci/search.c line 85

Quoted 5 lines

drivers/pci/search.c +5 -0

+			      void *data)
+{
+	int ret;
+
+	dev = pci_get_dma_source(dev);	/* XXX ref count screwup */

It's unfortunate that our dma quirk still seems to only get called for the end device where we could have bridges that need it too.

Sure, we can do that if necessary.

I'm not trying to present this as any kind of finished solution. The point is to figure out if we can make an interface that's a little more abstract so we can pull the PCI topology issues out of the IOMMU drivers, and I just thought something that looked like code would be more concrete and easier to discuss.

Quoted 9 lines

drivers/pci/search.c line 87

Quoted 2 lines

drivers/pci/search.c +2 -0

+
+	while (bridge != dev) {

I'm having a hard time understanding this function since bridge is never defined in any of the below sample code. Would bridge be the root port above a device? And what about devices connected to the RC? Seems like we'd need to traverse up the bus first, so there's a bit of missing magic.

I was assuming that an IOMMU could be either at the root of a PCI hierarchy or buried farther down. If that's true, the IOMMU driver might need to supply the top of the PCI hierarchy for which it translates. That's what I'm trying to get at with the "bridge" argument.

If the IOMMU is buried, I assumed there was no point in iterating over bridges upstream of the IOMMU because it wouldn't see requester IDs of those upstream bridges. PCI wouldn't have any way to figure out how far up to go, hence the argument.

Quoted 67 lines

Quoted 32 lines

drivers/pci/search.c +15 -0

+		if (pci_bridge_may_take_ownership(bridge)) {
+			ret = fn(dev, PCI_BRIDGE_REQUESTER_ID(bridge), data);
+			if (ret)
+				return ret;
+		}
+		bridge = pci_bridge_to_dev(bridge->subordinate, dev);
+	}
+
+	if (pci_is_pcie(dev) || pci_is_pcix(dev))
+		return fn(dev, PCI_REQUESTER_ID(dev), data);
+
+	/* Conventional PCI has no requester ID */
+	return 0;
+}
+
 /*
  * find the upstream PCIe-to-PCI bridge of a PCI device
  * if the device is PCIE, return NULL

include/linux/pci.h +2 -0

index 3a24e4f..cbcc82f 100644
@@ -794,6 +794,8 @@ static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus,
 }
 struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from);
 int pci_dev_present(const struct pci_device_id *ids);
+void pci_for_each_requester_id(struct pci_dev *bridge, struct pci_dev *dev,
+			       int (*fn)(struct pci_dev *, void *), void *data);
 
 int pci_bus_read_config_byte(struct pci_bus *bus, unsigned int devfn,
 			     int where, u8 *val);

Quoted 5 lines

commit 6e770d7bf4ef95207df4ac4c42ef4406ff7bc54e

Author: Bjorn Helgaas <[email protected]>
Date:   Wed Jul 3 12:02:15 2013 -0600

intel-iommu-upstream-pcie

I assume this one would is also going to apply dma quirks to dma_ops, which I'm all for.

drivers/iommu/intel-iommu.c static int domain_context_mapping_one(struct dmar_domain *domain, int segment, · line 1676

Quoted 13 lines

drivers/iommu/intel-iommu.c +5 -0

index b91564d..bdb6e6a 100644
@@ -1669,79 +1669,68 @@ static int domain_context_mapping_one(struct dmar_domain *domain, int segment,
 	return 0;
 }
 
+struct domain_context_info {
+	struct dmar_domain *domain;
+	struct pci_dev *dev;
+	int translation;
+	struct intel_iommu *iomumu;

s/iomumu/iommu/

drivers/iommu/intel-iommu.c static int domain_context_mapping_one(struct dmar_domain *domain, int segment, · line 1683

Quoted 7 lines

drivers/iommu/intel-iommu.c +7 -0

+	bool mapped;
+};
+
+static int context_mapping(struct pci_dev *dev, u16 requester_id, void *data)
+{
+	struct domain_context_info *dev_info = data;
+	int segment = pci_domain_nr(data->dev);

dev_info->dev

Quoted 7 lines

drivers/iommu/intel-iommu.c static int domain_context_mapping_one(struct dmar_domain *domain, int segment, · line 1684

Quoted 1 line

drivers/iommu/intel-iommu.c +1 -0

+	u8 bus = data->requester_id >> 8;

s/data->//

drivers/iommu/intel-iommu.c static int domain_context_mapping_one(struct dmar_domain *domain, int segment, · line 1685

Quoted 1 line

drivers/iommu/intel-iommu.c +1 -0

+	u8 devfn = data->requester_id & 0xff;

s/data->//

I told you it wouldn't even compile :) I'm only trying to present a possibility, not all the nitty-gritty details.

Quoted 243 lines

Quoted 12 lines

drivers/iommu/intel-iommu.c +7 -2

+
+	return domain_context_mapping_one(data->domain, segment,
+					  bus, devfn, data->translation);
+}
+
 static int domain_context_mapping(struct dmar_domain *domain,
 				  struct pci_dev *pdev, int translation)
 {
-	int ret;
-	struct pci_dev *tmp, *parent;
+	struct domain_context_info data;
+	struct pci_dev *bridge;

drivers/iommu/intel-iommu.c static int domain_context_mapping_one(struct dmar_domain *domain, int segment, · line 1700

Quoted 10 lines

drivers/iommu/intel-iommu.c +4 -5

 
-	ret = domain_context_mapping_one(domain, pci_domain_nr(pdev->bus),
-					 pdev->bus->number, pdev->devfn,
-					 translation);
-	if (ret)
-		return ret;
+	data.domain = domain;
+	data.dev = pdev;
+	data.translation = translation;
+	bridge = xx;

Darn, I was hoping you were going to reveal where bridge comes from.

Quoted 215 lines

drivers/iommu/intel-iommu.c +65 -92

+	return pci_for_each_requester_id(bridge, pdev, context_mapping, &data);
+}
 
-	/* dependent device mapping */
-	tmp = pci_find_upstream_pcie_bridge(pdev);
-	if (!tmp)
-		return 0;
-	/* Secondary interface's bus number and devfn 0 */
-	parent = pdev->bus->self;
-	while (parent != tmp) {
-		ret = domain_context_mapping_one(domain,
-						 pci_domain_nr(parent->bus),
-						 parent->bus->number,
-						 parent->devfn, translation);
-		if (ret)
-			return ret;
-		parent = parent->bus->self;
-	}
-	if (pci_is_pcie(tmp)) /* this is a PCIe-to-PCI bridge */
-		return domain_context_mapping_one(domain,
-					pci_domain_nr(tmp->subordinate),
-					tmp->subordinate->number, 0,
-					translation);
-	else /* this is a legacy PCI bridge */
-		return domain_context_mapping_one(domain,
-						  pci_domain_nr(tmp->bus),
-						  tmp->bus->number,
-						  tmp->devfn,
-						  translation);
+static int is_context_mapped(struct pci_dev *dev, u16 requester_id, void *data)
+{
+	struct domain_context_info *dev_info = data;
+	u8 bus = data->requester_id >> 8;
+	u8 devfn = data->requester_id & 0xff;
+	bool ret;
+
+	ret = device_context_mapped(dev_info->iommu, bus, devfn);
+	if (!ret)
+		dev_info->mapped = false;
+
+	return 0;
 }
 
 static bool domain_context_mapped(struct pci_dev *pdev)
 {
-	bool ret;
-	struct pci_dev *tmp, *parent;
+	struct domain_context_info data;
 	struct intel_iommu *iommu;
+	struct pci_dev *bridge;
 
 	iommu = device_to_iommu(pci_domain_nr(pdev->bus), pdev->bus->number,
 				pdev->devfn);
 	if (!iommu)
 		return false;
 
-	ret = device_context_mapped(iommu, pdev->bus->number, pdev->devfn);
-	if (!ret)
-		return false;
-	/* dependent device mapping */
-	tmp = pci_find_upstream_pcie_bridge(pdev);
-	if (!tmp)
-		return true;
-	/* Secondary interface's bus number and devfn 0 */
-	parent = pdev->bus->self;
-	while (parent != tmp) {
-		ret = device_context_mapped(iommu, parent->bus->number,
-					    parent->devfn);
-		if (!ret)
-			return false;
-		parent = parent->bus->self;
-	}
-	if (pci_is_pcie(tmp))
-		return device_context_mapped(iommu, tmp->subordinate->number,
-					     0);
-	else
-		return device_context_mapped(iommu, tmp->bus->number,
-					     tmp->devfn);
+	data.iommu = iommu;
+	data.mapped = true;
+	bridge = xx;
+	pci_for_each_requester_id(bridge, pdev, is_context_mapped, &data);
+	return data.mapped;
 }
 
 /* Returns a number of VTD pages, but aligned to MM page size */

@@ -1961,6 +1950,27 @@ static struct dmar_domain *find_domain(struct pci_dev *pdev)
 	return NULL;
 }
 
+static int find_domain2(struct pci_dev *dev, u16 requester_id, void *data)
+{
+	struct domain_context_info *dev_info = data;
+	int segment = pci_domain_nr(data->dev);
+	u8 bus = data->requester_id >> 8;
+	u8 devfn = data->requester_id & 0xff;
+	unsigned long flags;
+	struct device_domain_info *info;
+
+	spin_lock_irqsave(&device_domain_lock, flags);
+	list_for_each_entry(info, &device_domain_list, global) {
+		if (info->segment == segment &&
+		    info->bus == bus && info->devfn == devfn) {
+			data->domain = info->domain;
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&device_domain_lock, flags);
+	return 0;
+}
+
 /* domain is initialized */
 static struct dmar_domain *get_domain_for_dev(struct pci_dev *pdev, int gaw)
 {

@@ -1968,11 +1978,11 @@ static struct dmar_domain *get_domain_for_dev(struct pci_dev *pdev, int gaw)
 	struct intel_iommu *iommu;
 	struct dmar_drhd_unit *drhd;
 	struct device_domain_info *info, *tmp;
-	struct pci_dev *dev_tmp;
 	unsigned long flags;
 	int bus = 0, devfn = 0;
 	int segment;
 	int ret;
+	struct domain_context_info data;
 
 	domain = find_domain(pdev);
 	if (domain)

@@ -1980,29 +1990,13 @@ static struct dmar_domain *get_domain_for_dev(struct pci_dev *pdev, int gaw)
 
 	segment = pci_domain_nr(pdev->bus);
 
-	dev_tmp = pci_find_upstream_pcie_bridge(pdev);
-	if (dev_tmp) {
-		if (pci_is_pcie(dev_tmp)) {
-			bus = dev_tmp->subordinate->number;
-			devfn = 0;
-		} else {
-			bus = dev_tmp->bus->number;
-			devfn = dev_tmp->devfn;
-		}
-		spin_lock_irqsave(&device_domain_lock, flags);
-		list_for_each_entry(info, &device_domain_list, global) {
-			if (info->segment == segment &&
-			    info->bus == bus && info->devfn == devfn) {
-				found = info->domain;
-				break;
-			}
-		}
-		spin_unlock_irqrestore(&device_domain_lock, flags);
-		/* pcie-pci bridge already has a domain, uses it */
-		if (found) {
-			domain = found;
-			goto found_domain;
-		}
+	data.domain = NULL;
+	data.dev = pdev;
+	bridge = xx;
+	pci_for_each_requester_id(bridge, pdev, find_domain2, &data);
+	if (data.domain) {
+		domain = data.domain;
+		goto found_domain;
 	}
 
 	domain = alloc_domain();

@@ -3734,31 +3728,29 @@ int __init intel_iommu_init(void)
 	return 0;
 }
 
+static int detach_requester(struct pci_dev *dev, u16 requester_id, void *data)
+{
+	struct domain_context_info *dev_info = data;
+	struct intel_iommu *iommu = data->iommu;
+	u8 bus = data->requester_id >> 8;
+	u8 devfn = data->requester_id & 0xff;
+
+	iommu_detach_dev(iommu, bus, devfn);
+	return 0;
+}
+
 static void iommu_detach_dependent_devices(struct intel_iommu *iommu,
 					   struct pci_dev *pdev)
 {
-	struct pci_dev *tmp, *parent;
+	struct domain_context_info data;
 
 	if (!iommu || !pdev)
 		return;
 
 	/* dependent device detach */
-	tmp = pci_find_upstream_pcie_bridge(pdev);
-	/* Secondary interface's bus number and devfn 0 */
-	if (tmp) {
-		parent = pdev->bus->self;
-		while (parent != tmp) {
-			iommu_detach_dev(iommu, parent->bus->number,
-					 parent->devfn);
-			parent = parent->bus->self;
-		}
-		if (pci_is_pcie(tmp)) /* this is a PCIe-to-PCI bridge */
-			iommu_detach_dev(iommu,
-				tmp->subordinate->number, 0);
-		else /* this is a legacy PCI bridge */
-			iommu_detach_dev(iommu, tmp->bus->number,
-					 tmp->devfn);
-	}
+	data.iommu = iommu;
+	bridge = xx;
+	pci_for_each_requester_id(bridge, pdev, detach_requester, &data);
 }
 
 static void domain_remove_one_dev_info(struct dmar_domain *domain,
--

To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 162 lines · Bjorn Helgaas

Quoted 69 lines · Alex Williamson

Quoted 68 lines · Bjorn Helgaas

Quoted 8 lines · Alex Williamson

Joerg,

Where do we stand on this series? You had a concern that the heuristic used in patch 1/ could be dangerous. The suggestion for detecting the issue was actually from Bjorn who replied with his rationale. Do you want to go in the direction of a fixed whitelist or do you agree that even if the heuristic breaks it provides better behavior than what we have now? Thanks,

I'm trying to take a step back and look at the overall design, not these specific patches.

IOMMUs translate addresses based on their source, i.e., a PCIe requester ID. This is made more complicated by the fact that some bridges "take ownership" (change the requester ID as they forward transactions upstream), as well as the fact that conventional PCI has no requester ID at all. And some broken devices apparently generate DMA requests using the requester ID of another device.

We currently deal with this using the pci_find_upstream_pcie_bridge() and pci_get_dma_source() interfaces, but I think there's too much assembly required by their users. pci_find_upstream_pcie_bridge() callers normally loop through all the bridges between the "upstream PCIe bridge" and the device, checking for bridges that might take ownership. They probably also ought to use pci_get_dma_source() to account for the broken devices, but most callers don't.

Most of this is PCI-specific stuff that should be of interest to all IOMMU drivers, and the overall structure of calls and looping should be the same for all of them, so it would be nice to factor it out somehow.

The attached patch is guaranteed not to even compile; it's just to make this idea more concrete. The basic idea is that since the IOMMU driver wants to perform some action for each possible requester ID the IOMMU might see, PCI could provide an iterator ("pci_for_each_requester_id()") to do that.

Bjorn

commit afad51492c6672b96c2b0735600d5695e30f7180

Author: Bjorn Helgaas <[email protected]>
Date:   Wed Jul 3 16:04:26 2013 -0600

pci-add-for-each-requester-id

drivers/pci/search.c +12 -0

index d0627fa..380eb03 100644
@@ -17,6 +17,89 @@
 DECLARE_RWSEM(pci_bus_sem);
 EXPORT_SYMBOL_GPL(pci_bus_sem);
 
+#define PCI_REQUESTER_ID(dev)    (((dev)->bus->number << 8) | (dev)->devfn)
+#define PCI_BRIDGE_REQUESTER_ID(bridge)  ((bridge)->subordinate->number << 8)
+
+static inline bool pci_is_pcix(struct pci_dev *dev)
+{
+ return !!pci_pcix_cap(dev);     /* XXX not implemented */
+}
+
+static bool pci_bridge_may_take_ownership(struct pci_dev *bridge)
+{
+ /*
+  * A PCIe to PCI/PCI-X bridge may take ownership per PCIe Bridge

Quoted 8 lines

drivers/pci/search.c line 36

Quoted 5 lines

drivers/pci/search.c +5 -0

+  * Spec v1.0, sec 2.3.
+  */
+ if (pci_is_pcie(bridge) &&
+     pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)
+         return true;

Assume we still need a quirk here, PCIe-to-PCI bridges without a PCIe capability are exactly what causes the current bug.

Likely so. We might be able to identify that as "secondary bus is not PCIe" or something.

Quoted 47 lines

Quoted 34 lines

drivers/pci/search.c +34 -0

+
+ /*
+  * A PCI-X to PCI-X bridge need not take ownership because there
+  * are requester IDs on the secondary PCI-X bus.  However, if a PCI
+  * device is added on the secondary bus, the bridge must revert to
+  * being a PCI-X to PCI bridge, and it then *would* take ownership.
+  * Assuming a PCI-X to PCI-X bridge takes ownership means we can
+  * tolerate a future hot-add without having to change existing
+  * IOMMU mappings.
+  */
+ if (pci_is_pcix(bridge))
+         return true;
+
+ return false;
+}
+
+static struct pci_dev *pci_bridge_to_dev(struct pci_bus *bus,
+                                  struct pci_dev *dev)
+{
+ struct pci_dev *bridge;
+ struct pci_bus *child_bus;
+ u8 secondary, subordinate, busn = dev->bus->number;
+
+ if (dev->bus == bus)
+         return dev;
+
+ /*
+  * There may be several devices on "bus".  Find the one that is a
+  * bridge leading to "dev".
+  */
+ list_for_each_entry(bridge, &bus->devices, bus_list) {
+         child_bus = bridge->subordinate;
+         if (child_bus) {
+                 secondary = child_bus->busn_res.start;

drivers/pci/search.c line 80

Quoted 10 lines

drivers/pci/search.c +10 -0

+                 subordinate = child_bus->busn_res.end;
+                 if (secondary <= busn && busn <= subordinate)
+                         return bridge;
+         }
+ }
+ return NULL;
+}
+
+int pci_for_each_requester_id(struct pci_dev *bridge, struct pci_dev *dev,
+                       int (*fn)(struct pci_dev *, void *),

u16 requester_id

Quoted 8 lines

drivers/pci/search.c line 85

Quoted 5 lines

drivers/pci/search.c +5 -0

+                       void *data)
+{
+ int ret;
+
+ dev = pci_get_dma_source(dev);  /* XXX ref count screwup */

It's unfortunate that our dma quirk still seems to only get called for the end device where we could have bridges that need it too.

Sure, we can do that if necessary.

I'm not trying to present this as any kind of finished solution. The point is to figure out if we can make an interface that's a little more abstract so we can pull the PCI topology issues out of the IOMMU drivers, and I just thought something that looked like code would be more concrete and easier to discuss.

Quoted 9 lines

drivers/pci/search.c line 87

Quoted 2 lines

drivers/pci/search.c +2 -0

+
+ while (bridge != dev) {

I'm having a hard time understanding this function since bridge is never defined in any of the below sample code. Would bridge be the root port above a device? And what about devices connected to the RC? Seems like we'd need to traverse up the bus first, so there's a bit of missing magic.

I was assuming that an IOMMU could be either at the root of a PCI hierarchy or buried farther down. If that's true, the IOMMU driver might need to supply the top of the PCI hierarchy for which it translates. That's what I'm trying to get at with the "bridge" argument.

Quoted 4 lines

If the IOMMU is buried, I assumed there was no point in iterating over bridges upstream of the IOMMU because it wouldn't see requester IDs of those upstream bridges. PCI wouldn't have any way to figure out how far up to go, hence the argument.

I don't think that getting the bridge is going to be quite so simple. VT-d can have multiple IOMMUs, each handling a specific device or hierarchy. One IOMMU per domain can be default IOMMU for any device not otherwise specified. The IOMMU is not a PCI enumerable device. AMD-Vi does have a PCI visible IOMMU, but it's an endpoint on the root complex, not the top of a hierarchy.

I'm warming up to your approach, but I think I'd rather see something like:

struct pci_dev *pci_get_requester_id(struct pci_dev *pdev)

{
        do {
                /* Apply DMA quirks at every step */
                pdev = pci_get_dma_source(pdev);

                /* Done for PCIe devices or broken bridges, PCI-X TBD */
                if (pci_is_pcie(pdev) || /* broken pcie bridge test */)
                        break;

        } while (!pci_is_root_bus(pdev->bus) && (pdev = pdev->bus->self));

        return pdev;

}

/* Start with requester ID pdev and pdev->bus, this way we handle both peer
* and parent requester IDs */
int pci_for_each_requester_id(struct pci_dev *pdev, struct pci_bus *bus,
int (*fn)(struct pci_dev *, void *), void *data)

{
        struct pci_dev *tmp;
        int ret;

        list_for_each_entry(tmp, &bus->devices, bus_list) {
                if (pci_get_requester_id(tmp) == pdev) {
                        ret = fn(tmp, data);

                        if (ret)
                                ...

                        if (tmp->subordinate) {
                                ret = pci_for_each_requester_id(pdev, tmp->subordinate, fn);
                                if (ret)
                                        ...
                        }
                }
        }

        return ret;
}

Seems like that would be sufficient to leverage the intel-iommu changes you've drafted. Thanks,

Alex

Quoted 321 lines

Quoted 67 lines

Quoted 32 lines

drivers/pci/search.c +15 -0

+         if (pci_bridge_may_take_ownership(bridge)) {
+                 ret = fn(dev, PCI_BRIDGE_REQUESTER_ID(bridge), data);
+                 if (ret)
+                         return ret;
+         }
+         bridge = pci_bridge_to_dev(bridge->subordinate, dev);
+ }
+
+ if (pci_is_pcie(dev) || pci_is_pcix(dev))
+         return fn(dev, PCI_REQUESTER_ID(dev), data);
+
+ /* Conventional PCI has no requester ID */
+ return 0;
+}
+
 /*
  * find the upstream PCIe-to-PCI bridge of a PCI device
  * if the device is PCIE, return NULL

include/linux/pci.h +2 -0

index 3a24e4f..cbcc82f 100644
@@ -794,6 +794,8 @@ static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus,
 }
 struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from);
 int pci_dev_present(const struct pci_device_id *ids);
+void pci_for_each_requester_id(struct pci_dev *bridge, struct pci_dev *dev,
+                        int (*fn)(struct pci_dev *, void *), void *data);
 
 int pci_bus_read_config_byte(struct pci_bus *bus, unsigned int devfn,
                       int where, u8 *val);

Quoted 5 lines

commit 6e770d7bf4ef95207df4ac4c42ef4406ff7bc54e

Author: Bjorn Helgaas <[email protected]>
Date:   Wed Jul 3 12:02:15 2013 -0600

intel-iommu-upstream-pcie

I assume this one would is also going to apply dma quirks to dma_ops, which I'm all for.

drivers/iommu/intel-iommu.c static int domain_context_mapping_one(struct dmar_domain *domain, int segment, · line 1676

Quoted 13 lines

drivers/iommu/intel-iommu.c +5 -0

index b91564d..bdb6e6a 100644
@@ -1669,79 +1669,68 @@ static int domain_context_mapping_one(struct dmar_domain *domain, int segment,
  return 0;
 }
 
+struct domain_context_info {
+ struct dmar_domain *domain;
+ struct pci_dev *dev;
+ int translation;
+ struct intel_iommu *iomumu;

s/iomumu/iommu/

drivers/iommu/intel-iommu.c static int domain_context_mapping_one(struct dmar_domain *domain, int segment, · line 1683

Quoted 7 lines

drivers/iommu/intel-iommu.c +7 -0

+ bool mapped;
+};
+
+static int context_mapping(struct pci_dev *dev, u16 requester_id, void *data)
+{
+ struct domain_context_info *dev_info = data;
+ int segment = pci_domain_nr(data->dev);

dev_info->dev

Quoted 7 lines

drivers/iommu/intel-iommu.c static int domain_context_mapping_one(struct dmar_domain *domain, int segment, · line 1684

Quoted 1 line

drivers/iommu/intel-iommu.c +1 -0

+ u8 bus = data->requester_id >> 8;

s/data->//

drivers/iommu/intel-iommu.c static int domain_context_mapping_one(struct dmar_domain *domain, int segment, · line 1685

Quoted 1 line

drivers/iommu/intel-iommu.c +1 -0

+ u8 devfn = data->requester_id & 0xff;

s/data->//

I told you it wouldn't even compile :) I'm only trying to present a possibility, not all the nitty-gritty details.

Quoted 243 lines

Quoted 12 lines

drivers/iommu/intel-iommu.c +7 -2

+
+ return domain_context_mapping_one(data->domain, segment,
+                                   bus, devfn, data->translation);
+}
+
 static int domain_context_mapping(struct dmar_domain *domain,
                            struct pci_dev *pdev, int translation)
 {
- int ret;
- struct pci_dev *tmp, *parent;
+ struct domain_context_info data;
+ struct pci_dev *bridge;

drivers/iommu/intel-iommu.c static int domain_context_mapping_one(struct dmar_domain *domain, int segment, · line 1700

Quoted 10 lines

drivers/iommu/intel-iommu.c +4 -5

 
- ret = domain_context_mapping_one(domain, pci_domain_nr(pdev->bus),
-                                  pdev->bus->number, pdev->devfn,
-                                  translation);
- if (ret)
-         return ret;
+ data.domain = domain;
+ data.dev = pdev;
+ data.translation = translation;
+ bridge = xx;

Darn, I was hoping you were going to reveal where bridge comes from.

Quoted 215 lines

drivers/iommu/intel-iommu.c +65 -92

+ return pci_for_each_requester_id(bridge, pdev, context_mapping, &data);
+}
 
- /* dependent device mapping */
- tmp = pci_find_upstream_pcie_bridge(pdev);
- if (!tmp)
-         return 0;
- /* Secondary interface's bus number and devfn 0 */
- parent = pdev->bus->self;
- while (parent != tmp) {
-         ret = domain_context_mapping_one(domain,
-                                          pci_domain_nr(parent->bus),
-                                          parent->bus->number,
-                                          parent->devfn, translation);
-         if (ret)
-                 return ret;
-         parent = parent->bus->self;
- }
- if (pci_is_pcie(tmp)) /* this is a PCIe-to-PCI bridge */
-         return domain_context_mapping_one(domain,
-                                 pci_domain_nr(tmp->subordinate),
-                                 tmp->subordinate->number, 0,
-                                 translation);
- else /* this is a legacy PCI bridge */
-         return domain_context_mapping_one(domain,
-                                           pci_domain_nr(tmp->bus),
-                                           tmp->bus->number,
-                                           tmp->devfn,
-                                           translation);
+static int is_context_mapped(struct pci_dev *dev, u16 requester_id, void *data)
+{
+ struct domain_context_info *dev_info = data;
+ u8 bus = data->requester_id >> 8;
+ u8 devfn = data->requester_id & 0xff;
+ bool ret;
+
+ ret = device_context_mapped(dev_info->iommu, bus, devfn);
+ if (!ret)
+         dev_info->mapped = false;
+
+ return 0;
 }
 
 static bool domain_context_mapped(struct pci_dev *pdev)
 {
- bool ret;
- struct pci_dev *tmp, *parent;
+ struct domain_context_info data;
  struct intel_iommu *iommu;
+ struct pci_dev *bridge;
 
  iommu = device_to_iommu(pci_domain_nr(pdev->bus), pdev->bus->number,
                          pdev->devfn);
  if (!iommu)
          return false;
 
- ret = device_context_mapped(iommu, pdev->bus->number, pdev->devfn);
- if (!ret)
-         return false;
- /* dependent device mapping */
- tmp = pci_find_upstream_pcie_bridge(pdev);
- if (!tmp)
-         return true;
- /* Secondary interface's bus number and devfn 0 */
- parent = pdev->bus->self;
- while (parent != tmp) {
-         ret = device_context_mapped(iommu, parent->bus->number,
-                                     parent->devfn);
-         if (!ret)
-                 return false;
-         parent = parent->bus->self;
- }
- if (pci_is_pcie(tmp))
-         return device_context_mapped(iommu, tmp->subordinate->number,
-                                      0);
- else
-         return device_context_mapped(iommu, tmp->bus->number,
-                                      tmp->devfn);
+ data.iommu = iommu;
+ data.mapped = true;
+ bridge = xx;
+ pci_for_each_requester_id(bridge, pdev, is_context_mapped, &data);
+ return data.mapped;
 }
 
 /* Returns a number of VTD pages, but aligned to MM page size */

@@ -1961,6 +1950,27 @@ static struct dmar_domain *find_domain(struct pci_dev *pdev)
  return NULL;
 }
 
+static int find_domain2(struct pci_dev *dev, u16 requester_id, void *data)
+{
+ struct domain_context_info *dev_info = data;
+ int segment = pci_domain_nr(data->dev);
+ u8 bus = data->requester_id >> 8;
+ u8 devfn = data->requester_id & 0xff;
+ unsigned long flags;
+ struct device_domain_info *info;
+
+ spin_lock_irqsave(&device_domain_lock, flags);
+ list_for_each_entry(info, &device_domain_list, global) {
+         if (info->segment == segment &&
+             info->bus == bus && info->devfn == devfn) {
+                 data->domain = info->domain;
+                 break;
+         }
+ }
+ spin_unlock_irqrestore(&device_domain_lock, flags);
+ return 0;
+}
+
 /* domain is initialized */
 static struct dmar_domain *get_domain_for_dev(struct pci_dev *pdev, int gaw)
 {

@@ -1968,11 +1978,11 @@ static struct dmar_domain *get_domain_for_dev(struct pci_dev *pdev, int gaw)
  struct intel_iommu *iommu;
  struct dmar_drhd_unit *drhd;
  struct device_domain_info *info, *tmp;
- struct pci_dev *dev_tmp;
  unsigned long flags;
  int bus = 0, devfn = 0;
  int segment;
  int ret;
+ struct domain_context_info data;
 
  domain = find_domain(pdev);
  if (domain)

@@ -1980,29 +1990,13 @@ static struct dmar_domain *get_domain_for_dev(struct pci_dev *pdev, int gaw)
 
  segment = pci_domain_nr(pdev->bus);
 
- dev_tmp = pci_find_upstream_pcie_bridge(pdev);
- if (dev_tmp) {
-         if (pci_is_pcie(dev_tmp)) {
-                 bus = dev_tmp->subordinate->number;
-                 devfn = 0;
-         } else {
-                 bus = dev_tmp->bus->number;
-                 devfn = dev_tmp->devfn;
-         }
-         spin_lock_irqsave(&device_domain_lock, flags);
-         list_for_each_entry(info, &device_domain_list, global) {
-                 if (info->segment == segment &&
-                     info->bus == bus && info->devfn == devfn) {
-                         found = info->domain;
-                         break;
-                 }
-         }
-         spin_unlock_irqrestore(&device_domain_lock, flags);
-         /* pcie-pci bridge already has a domain, uses it */
-         if (found) {
-                 domain = found;
-                 goto found_domain;
-         }
+ data.domain = NULL;
+ data.dev = pdev;
+ bridge = xx;
+ pci_for_each_requester_id(bridge, pdev, find_domain2, &data);
+ if (data.domain) {
+         domain = data.domain;
+         goto found_domain;
  }
 
  domain = alloc_domain();

@@ -3734,31 +3728,29 @@ int __init intel_iommu_init(void)
  return 0;
 }
 
+static int detach_requester(struct pci_dev *dev, u16 requester_id, void *data)
+{
+ struct domain_context_info *dev_info = data;
+ struct intel_iommu *iommu = data->iommu;
+ u8 bus = data->requester_id >> 8;
+ u8 devfn = data->requester_id & 0xff;
+
+ iommu_detach_dev(iommu, bus, devfn);
+ return 0;
+}
+
 static void iommu_detach_dependent_devices(struct intel_iommu *iommu,
                                     struct pci_dev *pdev)
 {
- struct pci_dev *tmp, *parent;
+ struct domain_context_info data;
 
  if (!iommu || !pdev)
          return;
 
  /* dependent device detach */
- tmp = pci_find_upstream_pcie_bridge(pdev);
- /* Secondary interface's bus number and devfn 0 */
- if (tmp) {
-         parent = pdev->bus->self;
-         while (parent != tmp) {
-                 iommu_detach_dev(iommu, parent->bus->number,
-                                  parent->devfn);
-                 parent = parent->bus->self;
-         }
-         if (pci_is_pcie(tmp)) /* this is a PCIe-to-PCI bridge */
-                 iommu_detach_dev(iommu,
-                         tmp->subordinate->number, 0);
-         else /* this is a legacy PCI bridge */
-                 iommu_detach_dev(iommu, tmp->bus->number,
-                                  tmp->devfn);
- }
+ data.iommu = iommu;
+ bridge = xx;
+ pci_for_each_requester_id(bridge, pdev, detach_requester, &data);
 }
 
 static void domain_remove_one_dev_info(struct dmar_domain *domain,
--

To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Where do we stand on this series? You had a concern that the heuristic used in patch 1/ could be dangerous. The suggestion for detecting the issue was actually from Bjorn who replied with his rationale. Do you want to go in the direction of a fixed whitelist or do you agree that even if the heuristic breaks it provides better behavior than what we have now? Thanks,