re: [RFC PATCH v2 2/2] iommu/intel: Make use of PCIe requester ID interface

re: [RFC PATCH v2 2/2] iommu/intel: Make use of PCIe requester ID interface

[Andy Burns]

I'm running into an IOMMU crash on CentOS7, it seems this patch series 
aims to fix the same type of issue, any thoughts as to whether it is 
likely to help in my case?

[ 0.451393] IOMMU: Prepare 0-16MiB unity mapping for LPC
[ 0.451400] IOMMU: Setting identity map for device 0000:00:1f.0 [0x0 - 
0xffffff]
[ 0.451455] PCI-DMA: Intel(R) Virtualization Technology for Directed I/O
[ 0.451546] ------------[ cut here ]------------
[ 0.451552] WARNING: at drivers/pci/search.c:46 
pci_find_upstream_pcie_bridge+0x87/0x90()
[ 0.451553] Modules linked in:
[ 0.451557] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 
3.10.0-123.6.3.el7.x86_64 #1
[ 0.451558] Hardware name: To Be Filled By O.E.M. To Be Filled By 
O.E.M./H97 Pro4, BIOS P1.40 07/29/2014
[ 0.451559] 0000000000000000 000000003a87b4a1 ffff880408885cf0 
ffffffff815e20bb
[ 0.451561] ffff880408885d28 ffffffff8105dee1 ffff880408212000 
ffff88040821c000
[ 0.451563] ffff88040821c098 0000000000000000 0000000000000000 
ffff880408885d38
[ 0.451565] Call Trace:
[ 0.451570] [<ffffffff815e20bb>] dump_stack+0x19/0x1b
[ 0.451574] [<ffffffff8105dee1>] warn_slowpath_common+0x61/0x80
[ 0.451576] [<ffffffff8105e00a>] warn_slowpath_null+0x1a/0x20
[ 0.451579] [<ffffffff812ec727>] pci_find_upstream_pcie_bridge+0x87/0x90
[ 0.451582] [<ffffffff814a9d8d>] intel_iommu_add_device+0x4d/0x230
[ 0.451585] [<ffffffff814a0e30>] ? bus_set_iommu+0x50/0x50
[ 0.451588] [<ffffffff814a0e5a>] add_iommu_group+0x2a/0x50
[ 0.451591] [<ffffffff813b4563>] bus_for_each_dev+0x73/0xc0
[ 0.451593] [<ffffffff814a0e28>] bus_set_iommu+0x48/0x50
[ 0.451597] [<ffffffff81a58273>] intel_iommu_init+0x3ea/0x413
[ 0.451601] [<ffffffff81a11c93>] ? memblock_find_dma_reserve+0x147/0x147
[ 0.451603] [<ffffffff81a11ca5>] pci_iommu_init+0x12/0x3c
[ 0.451606] [<ffffffff810020e2>] do_one_initcall+0xe2/0x190
[ 0.451608] [<ffffffff81a09153>] kernel_init_freeable+0x18b/0x22a
[ 0.451610] [<ffffffff81a0892b>] ? do_early_param+0x88/0x88
[ 0.451614] [<ffffffff815c3960>] ? rest_init+0x80/0x80
[ 0.451617] [<ffffffff815c396e>] kernel_init+0xe/0x180
[ 0.451619] [<ffffffff815f26ec>] ret_from_fork+0x7c/0xb0
[ 0.451621] [<ffffffff815c3960>] ? rest_init+0x80/0x80
[ 0.451626] ---[ end trace e5ab4299053e7cdf ]---

I will report back after testing. To save duplication there's info about 
my setup here ...

<http://centos.org/forums/viewtopic.php?f=47&t=48115>

Re: [RFC PATCH v2 2/2] iommu/intel: Make use of PCIe requester ID interface

[Andy Burns]

Andy Burns wrote:

> I'm running into an IOMMU crash on CentOS7, it seems this patch series
> aims to fix the same type of issue

Sorry, I thought I was dragging up a 1 month old thread, not a 13 month 
old thread!

Am I correct that this became part of the 13 patch set of 1st May 2014, 
where I saw reference to

"should also enable motherboards with an onboard ASmedia
ASM1083/1085 PCIe-to-PCI bridge to work with VT-d enabled"

Are any parts of this in mainline yet?

Re: [RFC PATCH v2 2/2] iommu/intel: Make use of PCIe requester ID interface

[Andy Burns]

Andy Burns wrote:

> "should also enable motherboards with an onboard ASmedia
> ASM1083/1085 PCIe-to-PCI bridge to work with VT-d enabled"
>
> Are any parts of this in mainline yet?

I tried with an elrepo 3.17-rc1 kernel (which presumably includes 
dma-alias-v4 patches) and the warning from pci_find_upstream_pcie_bridge 
is now gone.

I still have to learn a little about how to pass through the whole group 
of devices using VT-d, to get PCI passthrough working.

Thanks.

[RFC PATCH v2 2/2] iommu/intel: Make use of PCIe requester ID interface

[Alex Williamson]

This eliminates uses of pci_find_upstream_pcie_bridge() and
incorporates DMA quirks into dma_ops path.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/iommu/intel-iommu.c         |  164 ++++++++++++++---------------------
 drivers/iommu/intel_irq_remapping.c |    2 
 2 files changed, 65 insertions(+), 101 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index b4f0e28..51488cb 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1675,80 +1675,59 @@ static int domain_context_mapping_one(struct dmar_domain *domain, int segment,
 	return 0;
 }
 
-static int
-domain_context_mapping(struct dmar_domain *domain, struct pci_dev *pdev,
-			int translation)
+struct context_mapping_info {
+	struct dmar_domain *domain;
+	int translation;
+};
+
+static int context_mapping(struct pci_dev *dev, u16 requester_id, void *data)
 {
-	int ret;
-	struct pci_dev *tmp, *parent;
+	struct context_mapping_info *info = data;
+	u8 bus = requester_id >> 8;
+	u8 devfn = requester_id & 0xFF;
 
-	ret = domain_context_mapping_one(domain, pci_domain_nr(pdev->bus),
-					 pdev->bus->number, pdev->devfn,
-					 translation);
-	if (ret)
-		return ret;
+	return domain_context_mapping_one(info->domain, pci_domain_nr(dev->bus),
+					  bus, devfn, info->translation);
+}
 
-	/* 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 domain_context_mapping(struct dmar_domain *domain,
+				  struct pci_dev *pdev, int translation)
+{
+	struct context_mapping_info info = {
+		.domain = domain,
+		.translation = translation,
+	};
+
+	return pcie_for_each_requester(pdev, NULL, context_mapping, &info);
+}
+
+static int is_context_mapped(struct pci_dev *dev, u16 requester_id, void *data)
+{
+	struct intel_iommu *iommu = data;
+	u8 bus = requester_id >> 8;
+	u8 devfn = requester_id & 0xFF;
+
+	if (!device_context_mapped(iommu, bus, devfn))
+		return 1; /* stop */
+
+	return 0;
 }
 
 static int domain_context_mapped(struct pci_dev *pdev)
 {
-	int ret;
-	struct pci_dev *tmp, *parent;
 	struct intel_iommu *iommu;
+	int ret;
 
-	iommu = device_to_iommu(pci_domain_nr(pdev->bus), pdev->bus->number,
-				pdev->devfn);
+	iommu = device_to_iommu(pci_domain_nr(pdev->bus),
+				pdev->bus->number, pdev->devfn);
 	if (!iommu)
 		return -ENODEV;
 
-	ret = device_context_mapped(iommu, pdev->bus->number, pdev->devfn);
-	if (!ret)
-		return ret;
-	/* dependent device mapping */
-	tmp = pci_find_upstream_pcie_bridge(pdev);
-	if (!tmp)
+	ret = pcie_for_each_requester(pdev, NULL, is_context_mapped, iommu);
+	if (ret < 0)
 		return ret;
-	/* 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 ret;
-		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);
+
+	return (ret == 0);
 }
 
 /* Returns a number of VTD pages, but aligned to MM page size */
@@ -1975,6 +1954,7 @@ static struct dmar_domain *get_domain_for_dev(struct pci_dev *pdev, int gaw)
 	struct dmar_drhd_unit *drhd;
 	struct device_domain_info *info, *tmp;
 	struct pci_dev *dev_tmp;
+	u16 requester_id;
 	unsigned long flags;
 	int bus = 0, devfn = 0;
 	int segment;
@@ -1986,15 +1966,11 @@ 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;
-		}
+	dev_tmp = pci_get_visible_pcie_requester(pdev, NULL, &requester_id);
+	if (dev_tmp && dev_tmp != pdev) {
+		bus = requester_id >> 8;
+		devfn = requester_id & 0xFF;
+
 		spin_lock_irqsave(&device_domain_lock, flags);
 		list_for_each_entry(info, &device_domain_list, global) {
 			if (info->segment == segment &&
@@ -3749,31 +3725,24 @@ int __init intel_iommu_init(void)
 	return 0;
 }
 
+static int detach_requester(struct pci_dev *dev, u16 requester_id, void *data)
+{
+	struct intel_iommu *iommu = data;
+	u8 bus = requester_id >> 8;
+	u8 devfn = 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;
-
 	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);
-	}
+	/* XXX What if there's something else using his path? */
+	pcie_for_each_requester(pdev, NULL, detach_requester, iommu);
 }
 
 static void domain_remove_one_dev_info(struct dmar_domain *domain,
@@ -4158,7 +4127,7 @@ static int intel_iommu_domain_has_cap(struct iommu_domain *domain,
 static int intel_iommu_add_device(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	struct pci_dev *bridge, *dma_pdev = NULL;
+	struct pci_dev *dma_pdev = NULL;
 	struct iommu_group *group;
 	int ret;
 
@@ -4166,16 +4135,11 @@ static int intel_iommu_add_device(struct device *dev)
 			     pdev->bus->number, pdev->devfn))
 		return -ENODEV;
 
-	bridge = pci_find_upstream_pcie_bridge(pdev);
-	if (bridge) {
-		if (pci_is_pcie(bridge))
-			dma_pdev = pci_get_domain_bus_and_slot(
-						pci_domain_nr(pdev->bus),
-						bridge->subordinate->number, 0);
-		if (!dma_pdev)
-			dma_pdev = pci_dev_get(bridge);
-	} else
-		dma_pdev = pci_dev_get(pdev);
+	dma_pdev = pci_get_visible_pcie_requester(pdev, NULL, NULL);
+	if (!dma_pdev)
+		return -EINVAL;
+
+	dma_pdev = pci_dev_get(dma_pdev);
 
 	/* Account for quirked devices */
 	swap_pci_ref(&dma_pdev, pci_get_dma_source(dma_pdev));
diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
index 5b19b2d..31214fe 100644
--- a/drivers/iommu/intel_irq_remapping.c
+++ b/drivers/iommu/intel_irq_remapping.c
@@ -385,7 +385,7 @@ static int set_msi_sid(struct irte *irte, struct pci_dev *dev)
 		return 0;
 	}
 
-	bridge = pci_find_upstream_pcie_bridge(dev);
+	bridge = pci_get_visible_pcie_requester(dev, NULL, NULL);
 	if (bridge) {
 		if (pci_is_pcie(bridge))/* this is a PCIe-to-PCI/PCIX bridge */
 			set_irte_sid(irte, SVT_VERIFY_BUS, SQ_ALL_16,

[RFC PATCH v2 2/2] iommu/intel: Make use of PCIe requester ID interface

[Alex Williamson]

This eliminates uses of pci_find_upstream_pcie_bridge() and
incorporates DMA quirks into dma_ops path.

Suggested-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/intel-iommu.c         |  164 ++++++++++++++---------------------
 drivers/iommu/intel_irq_remapping.c |    2 
 2 files changed, 65 insertions(+), 101 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index b4f0e28..51488cb 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1675,80 +1675,59 @@ static int domain_context_mapping_one(struct dmar_domain *domain, int segment,
 	return 0;
 }
 
-static int
-domain_context_mapping(struct dmar_domain *domain, struct pci_dev *pdev,
-			int translation)
+struct context_mapping_info {
+	struct dmar_domain *domain;
+	int translation;
+};
+
+static int context_mapping(struct pci_dev *dev, u16 requester_id, void *data)
 {
-	int ret;
-	struct pci_dev *tmp, *parent;
+	struct context_mapping_info *info = data;
+	u8 bus = requester_id >> 8;
+	u8 devfn = requester_id & 0xFF;
 
-	ret = domain_context_mapping_one(domain, pci_domain_nr(pdev->bus),
-					 pdev->bus->number, pdev->devfn,
-					 translation);
-	if (ret)
-		return ret;
+	return domain_context_mapping_one(info->domain, pci_domain_nr(dev->bus),
+					  bus, devfn, info->translation);
+}
 
-	/* 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 domain_context_mapping(struct dmar_domain *domain,
+				  struct pci_dev *pdev, int translation)
+{
+	struct context_mapping_info info = {
+		.domain = domain,
+		.translation = translation,
+	};
+
+	return pcie_for_each_requester(pdev, NULL, context_mapping, &info);
+}
+
+static int is_context_mapped(struct pci_dev *dev, u16 requester_id, void *data)
+{
+	struct intel_iommu *iommu = data;
+	u8 bus = requester_id >> 8;
+	u8 devfn = requester_id & 0xFF;
+
+	if (!device_context_mapped(iommu, bus, devfn))
+		return 1; /* stop */
+
+	return 0;
 }
 
 static int domain_context_mapped(struct pci_dev *pdev)
 {
-	int ret;
-	struct pci_dev *tmp, *parent;
 	struct intel_iommu *iommu;
+	int ret;
 
-	iommu = device_to_iommu(pci_domain_nr(pdev->bus), pdev->bus->number,
-				pdev->devfn);
+	iommu = device_to_iommu(pci_domain_nr(pdev->bus),
+				pdev->bus->number, pdev->devfn);
 	if (!iommu)
 		return -ENODEV;
 
-	ret = device_context_mapped(iommu, pdev->bus->number, pdev->devfn);
-	if (!ret)
-		return ret;
-	/* dependent device mapping */
-	tmp = pci_find_upstream_pcie_bridge(pdev);
-	if (!tmp)
+	ret = pcie_for_each_requester(pdev, NULL, is_context_mapped, iommu);
+	if (ret < 0)
 		return ret;
-	/* 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 ret;
-		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);
+
+	return (ret == 0);
 }
 
 /* Returns a number of VTD pages, but aligned to MM page size */
@@ -1975,6 +1954,7 @@ static struct dmar_domain *get_domain_for_dev(struct pci_dev *pdev, int gaw)
 	struct dmar_drhd_unit *drhd;
 	struct device_domain_info *info, *tmp;
 	struct pci_dev *dev_tmp;
+	u16 requester_id;
 	unsigned long flags;
 	int bus = 0, devfn = 0;
 	int segment;
@@ -1986,15 +1966,11 @@ 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;
-		}
+	dev_tmp = pci_get_visible_pcie_requester(pdev, NULL, &requester_id);
+	if (dev_tmp && dev_tmp != pdev) {
+		bus = requester_id >> 8;
+		devfn = requester_id & 0xFF;
+
 		spin_lock_irqsave(&device_domain_lock, flags);
 		list_for_each_entry(info, &device_domain_list, global) {
 			if (info->segment == segment &&
@@ -3749,31 +3725,24 @@ int __init intel_iommu_init(void)
 	return 0;
 }
 
+static int detach_requester(struct pci_dev *dev, u16 requester_id, void *data)
+{
+	struct intel_iommu *iommu = data;
+	u8 bus = requester_id >> 8;
+	u8 devfn = 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;
-
 	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);
-	}
+	/* XXX What if there's something else using his path? */
+	pcie_for_each_requester(pdev, NULL, detach_requester, iommu);
 }
 
 static void domain_remove_one_dev_info(struct dmar_domain *domain,
@@ -4158,7 +4127,7 @@ static int intel_iommu_domain_has_cap(struct iommu_domain *domain,
 static int intel_iommu_add_device(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	struct pci_dev *bridge, *dma_pdev = NULL;
+	struct pci_dev *dma_pdev = NULL;
 	struct iommu_group *group;
 	int ret;
 
@@ -4166,16 +4135,11 @@ static int intel_iommu_add_device(struct device *dev)
 			     pdev->bus->number, pdev->devfn))
 		return -ENODEV;
 
-	bridge = pci_find_upstream_pcie_bridge(pdev);
-	if (bridge) {
-		if (pci_is_pcie(bridge))
-			dma_pdev = pci_get_domain_bus_and_slot(
-						pci_domain_nr(pdev->bus),
-						bridge->subordinate->number, 0);
-		if (!dma_pdev)
-			dma_pdev = pci_dev_get(bridge);
-	} else
-		dma_pdev = pci_dev_get(pdev);
+	dma_pdev = pci_get_visible_pcie_requester(pdev, NULL, NULL);
+	if (!dma_pdev)
+		return -EINVAL;
+
+	dma_pdev = pci_dev_get(dma_pdev);
 
 	/* Account for quirked devices */
 	swap_pci_ref(&dma_pdev, pci_get_dma_source(dma_pdev));
diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
index 5b19b2d..31214fe 100644
--- a/drivers/iommu/intel_irq_remapping.c
+++ b/drivers/iommu/intel_irq_remapping.c
@@ -385,7 +385,7 @@ static int set_msi_sid(struct irte *irte, struct pci_dev *dev)
 		return 0;
 	}
 
-	bridge = pci_find_upstream_pcie_bridge(dev);
+	bridge = pci_get_visible_pcie_requester(dev, NULL, NULL);
 	if (bridge) {
 		if (pci_is_pcie(bridge))/* this is a PCIe-to-PCI/PCIX bridge */
 			set_irte_sid(irte, SVT_VERIFY_BUS, SQ_ALL_16,