Date:	Tue, 24 May 2011 14:07:19 -0600
From:	Alex Williamson <alex.williamson@...hat.com>
To:	Yinghai Lu <yinghai@...nel.org>
Cc:	David Woodhouse <dwmw2@...radead.org>,
	Vinod Koul <vinod.koul@...el.com>, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Jesse Barnes <jbarnes@...tuousgeek.org>,
	iommu@...ts.linux-foundation.org,
	Dan Williams <dan.j.williams@...el.com>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] pci, dmar: Update dmar units devices list during
 hotplug

On Tue, 2011-05-24 at 12:34 -0700, Yinghai Lu wrote:
> On 05/24/2011 10:42 AM, Alex Williamson wrote:
> > On Tue, 2011-05-24 at 11:58 +0100, David Woodhouse wrote:
> >> On Thu, 2011-05-19 at 16:15 -0600, Alex Williamson wrote:
> >>> I think I'd vote for saving some kind of representation of the bus
> >>> hierarchy, we probably don't need to list every possible individual
> >>> device.  Leaving a broken pointer around to be matched up and restored
> >>> later just seems like a continuation of an idea that was bad to begin
> >>> with.  Thanks, 
> >>
> >> I agree. We should just process the original ATSR information in
> >> dmar_find_matched_drhd_unit(), rather than comparing with a list of
> >> possibly stale pointers.
> >>
> >> I don't quite understand why the list of PCI devices was *ever* done
> >> like that.
> > 
> > Yinghai,
> > 
> > I thought I might be running into something similar so spent some time
> > taking a different slant coding up the bug you found.  Turns out I
> > should have tested your patch first because I wasn't hitting that bug at
> > all.  The patch below is a work-in-progress that I think fixes the bug
> > by providing a quick means of re-parsing the scope as needed to match
> > current struct pci_devs.  It needs testing and cleanup, but feel free to
> > run with it (or ignore).  Just figured its better to post than waste the
> > code if you end up doing something similar.  Thanks,
> > 
> > Alex
> > 
> 
> it does not apply to current linus tree cleanly.

Sorry, for some reason I started hacking on this against a rhel kernel.
Here's the compile tested-only forward port to 2.6.39 (plus the
domain_exit flush patch).  Thanks,

Alex


Not for commit

Signed-off-by: Alex Williamson <alex.williamson@...hat.com>
---

 drivers/pci/dmar.c        |  162 ++++++++++++++++++++++++---------------------
 drivers/pci/intel-iommu.c |   94 ++++++++++++++++----------
 include/linux/dmar.h      |   29 ++++++--
 3 files changed, 166 insertions(+), 119 deletions(-)


diff --git a/drivers/pci/dmar.c b/drivers/pci/dmar.c
index 12e02bf..47e4f09 100644
--- a/drivers/pci/dmar.c
+++ b/drivers/pci/dmar.c
@@ -61,8 +61,8 @@ static void __init dmar_register_drhd_unit(struct dmar_drhd_unit *drhd)
 		list_add(&drhd->list, &dmar_drhd_units);
 }
 
-static int __init dmar_parse_one_dev_scope(struct acpi_dmar_device_scope *scope,
-					   struct pci_dev **dev, u16 segment)
+struct pci_dev *dmar_get_scope_dev(struct acpi_dmar_device_scope *scope,
+				   u16 segment)
 {
 	struct pci_bus *bus;
 	struct pci_dev *pdev = NULL;
@@ -74,7 +74,7 @@ static int __init dmar_parse_one_dev_scope(struct acpi_dmar_device_scope *scope,
 	count = (scope->length - sizeof(struct acpi_dmar_device_scope))
 		/ sizeof(struct acpi_dmar_pci_path);
 
-	while (count) {
+	for (; count; path++, count--, bus = pdev->subordinate) {
 		if (pdev)
 			pci_dev_put(pdev);
 		/*
@@ -82,53 +82,77 @@ static int __init dmar_parse_one_dev_scope(struct acpi_dmar_device_scope *scope,
 		 * ignore it
 		 */
 		if (!bus) {
-			printk(KERN_WARNING
-			PREFIX "Device scope bus [%d] not found\n",
-			scope->bus);
-			break;
+			printk(KERN_WARNING PREFIX
+			       "Device scope bus [%d] not found\n", scope->bus);
+			return NULL;
 		}
 		pdev = pci_get_slot(bus, PCI_DEVFN(path->dev, path->fn));
 		if (!pdev) {
 			printk(KERN_WARNING PREFIX
-			"Device scope device [%04x:%02x:%02x.%02x] not found\n",
-				segment, bus->number, path->dev, path->fn);
-			break;
+			       "Device scope device [%04x:%02x:%02x.%02x] not found\n",
+			       segment, bus->number, path->dev, path->fn);
+			return NULL;
 		}
-		path ++;
-		count --;
-		bus = pdev->subordinate;
 	}
-	if (!pdev) {
-		printk(KERN_WARNING PREFIX
-		"Device scope device [%04x:%02x:%02x.%02x] not found\n",
-		segment, scope->bus, path->dev, path->fn);
-		*dev = NULL;
+
+	return pdev;
+}
+
+static int dmar_match_scope_one(struct acpi_dmar_device_scope *scope,
+				struct pci_dev *dev, u16 segment)
+{
+	struct pci_dev *pdev;
+	int ret = 0;
+
+	if (segment != pci_domain_nr(dev->bus))
+		return 0;
+
+	pdev = dmar_get_scope_dev(scope, segment);
+	if (!pdev)
 		return 0;
+
+	if (scope->entry_type == ACPI_DMAR_SCOPE_TYPE_ENDPOINT) {
+		if (dev == pdev)
+			ret = 1;
+	} else {
+		while (dev) {
+			if (dev == pdev) {
+				ret = 1;
+				break;
+			}
+			dev = dev->bus->self;
+		}
 	}
-	if ((scope->entry_type == ACPI_DMAR_SCOPE_TYPE_ENDPOINT && \
-			pdev->subordinate) || (scope->entry_type == \
-			ACPI_DMAR_SCOPE_TYPE_BRIDGE && !pdev->subordinate)) {
-		pci_dev_put(pdev);
-		printk(KERN_WARNING PREFIX
-			"Device scope type does not match for %s\n",
-			 pci_name(pdev));
-		return -EINVAL;
+
+	pci_dev_put(pdev);
+
+	return ret;
+}
+
+int dmar_match_scope(struct acpi_dmar_device_scope **scopes, int cnt,
+		     struct pci_dev *dev, u16 segment)
+{
+	int i;
+
+	for (i = 0; i < cnt; i++) {
+		if (dmar_match_scope_one(scopes[i], dev, segment))
+			return 1;
 	}
-	*dev = pdev;
 	return 0;
 }
 
 static int __init dmar_parse_dev_scope(void *start, void *end, int *cnt,
-				       struct pci_dev ***devices, u16 segment)
+				       struct acpi_dmar_device_scope ***scopes)
 {
 	struct acpi_dmar_device_scope *scope;
-	void * tmp = start;
-	int index;
-	int ret;
+	void *tmp = start;
+	int index = 0;
 
 	*cnt = 0;
+
 	while (start < end) {
 		scope = start;
+
 		if (scope->entry_type == ACPI_DMAR_SCOPE_TYPE_ENDPOINT ||
 		    scope->entry_type == ACPI_DMAR_SCOPE_TYPE_BRIDGE)
 			(*cnt)++;
@@ -138,27 +162,23 @@ static int __init dmar_parse_dev_scope(void *start, void *end, int *cnt,
 		}
 		start += scope->length;
 	}
+
 	if (*cnt == 0)
 		return 0;
 
-	*devices = kcalloc(*cnt, sizeof(struct pci_dev *), GFP_KERNEL);
-	if (!*devices)
+	*scopes = kcalloc(*cnt, sizeof(struct acpi_dmar_device_scope *),
+                          GFP_KERNEL);
+	if (!*scopes)
 		return -ENOMEM;
 
 	start = tmp;
-	index = 0;
 	while (start < end) {
 		scope = start;
+
 		if (scope->entry_type == ACPI_DMAR_SCOPE_TYPE_ENDPOINT ||
-		    scope->entry_type == ACPI_DMAR_SCOPE_TYPE_BRIDGE) {
-			ret = dmar_parse_one_dev_scope(scope,
-				&(*devices)[index], segment);
-			if (ret) {
-				kfree(*devices);
-				return ret;
-			}
-			index ++;
-		}
+		    scope->entry_type == ACPI_DMAR_SCOPE_TYPE_BRIDGE)
+			(*scopes)[index++] = scope;
+
 		start += scope->length;
 	}
 
@@ -207,9 +227,8 @@ static int __init dmar_parse_dev(struct dmar_drhd_unit *dmaru)
 		return 0;
 
 	ret = dmar_parse_dev_scope((void *)(drhd + 1),
-				((void *)drhd) + drhd->header.length,
-				&dmaru->devices_cnt, &dmaru->devices,
-				drhd->segment);
+				   ((void *)drhd) + drhd->header.length,
+				   &dmaru->scopes_cnt, &dmaru->scopes);
 	if (ret) {
 		list_del(&dmaru->list);
 		kfree(dmaru);
@@ -253,10 +272,10 @@ rmrr_parse_dev(struct dmar_rmrr_unit *rmrru)
 
 	rmrr = (struct acpi_dmar_reserved_memory *) rmrru->hdr;
 	ret = dmar_parse_dev_scope((void *)(rmrr + 1),
-		((void *)rmrr) + rmrr->header.length,
-		&rmrru->devices_cnt, &rmrru->devices, rmrr->segment);
+				   ((void *)rmrr) + rmrr->header.length,
+				   &rmrru->scopes_cnt, &rmrru->scopes);
 
-	if (ret || (rmrru->devices_cnt == 0)) {
+	if (ret || (rmrru->scopes_cnt == 0)) {
 		list_del(&rmrru->list);
 		kfree(rmrru);
 	}
@@ -293,10 +312,9 @@ static int __init atsr_parse_dev(struct dmar_atsr_unit *atsru)
 
 	atsr = container_of(atsru->hdr, struct acpi_dmar_atsr, header);
 	rc = dmar_parse_dev_scope((void *)(atsr + 1),
-				(void *)atsr + atsr->header.length,
-				&atsru->devices_cnt, &atsru->devices,
-				atsr->segment);
-	if (rc || !atsru->devices_cnt) {
+				  (void *)atsr + atsr->header.length,
+				  &atsru->scopes_cnt, &atsru->scopes);
+	if (rc || !atsru->scopes_cnt) {
 		list_del(&atsru->list);
 		kfree(atsru);
 	}
@@ -310,6 +328,7 @@ int dmar_find_matched_atsr_unit(struct pci_dev *dev)
 	struct pci_bus *bus;
 	struct acpi_dmar_atsr *atsr;
 	struct dmar_atsr_unit *atsru;
+	struct pci_dev *pdev;
 
 	dev = pci_physfn(dev);
 
@@ -330,10 +349,18 @@ found:
 			return 0;
 
 		if (bridge->pcie_type == PCI_EXP_TYPE_ROOT_PORT) {
-			for (i = 0; i < atsru->devices_cnt; i++)
-				if (atsru->devices[i] == bridge)
+			for (i = 0; i < atsru->scopes_cnt; i++) {
+				pdev = dmar_get_scope_dev(atsru->scopes[i],
+							  atsr->segment);
+				if (!pdev)
+					continue;
+
+				if (pdev == bridge) {
+					pci_dev_put(pdev);
 					return 1;
-			break;
+				}
+				pci_dev_put(pdev);
+			}
 		}
 	}
 
@@ -513,23 +540,6 @@ parse_dmar_table(void)
 	return ret;
 }
 
-static int dmar_pci_device_match(struct pci_dev *devices[], int cnt,
-			  struct pci_dev *dev)
-{
-	int index;
-
-	while (dev) {
-		for (index = 0; index < cnt; index++)
-			if (dev == devices[index])
-				return 1;
-
-		/* Check our parent */
-		dev = dev->bus->self;
-	}
-
-	return 0;
-}
-
 struct dmar_drhd_unit *
 dmar_find_matched_drhd_unit(struct pci_dev *dev)
 {
@@ -544,11 +554,11 @@ dmar_find_matched_drhd_unit(struct pci_dev *dev)
 				    header);
 
 		if (dmaru->include_all &&
-		    drhd->segment == pci_domain_nr(dev->bus))
+		    dmaru->segment == pci_domain_nr(dev->bus))
 			return dmaru;
 
-		if (dmar_pci_device_match(dmaru->devices,
-					  dmaru->devices_cnt, dev))
+		if (dmar_match_scope(dmaru->scopes, dmaru->scopes_cnt,
+				     dev, dmaru->segment))
 			return dmaru;
 	}
 
diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
index b04f84e..d1d542a 100644
--- a/drivers/pci/intel-iommu.c
+++ b/drivers/pci/intel-iommu.c
@@ -563,32 +563,34 @@ static void domain_update_iommu_cap(struct dmar_domain *domain)
 
 static struct intel_iommu *device_to_iommu(int segment, u8 bus, u8 devfn)
 {
-	struct dmar_drhd_unit *drhd = NULL;
-	int i;
+	struct dmar_drhd_unit *dmaru = NULL;
+	struct pci_dev *pdev;
+	struct intel_iommu *found = NULL;
 
-	for_each_drhd_unit(drhd) {
-		if (drhd->ignored)
+	pdev = pci_get_domain_bus_and_slot(segment, bus, devfn);
+
+	for_each_drhd_unit(dmaru) {
+		if (dmaru->ignored)
 			continue;
-		if (segment != drhd->segment)
+		if (segment != dmaru->segment)
 			continue;
 
-		for (i = 0; i < drhd->devices_cnt; i++) {
-			if (drhd->devices[i] &&
-			    drhd->devices[i]->bus->number == bus &&
-			    drhd->devices[i]->devfn == devfn)
-				return drhd->iommu;
-			if (drhd->devices[i] &&
-			    drhd->devices[i]->subordinate &&
-			    drhd->devices[i]->subordinate->number <= bus &&
-			    drhd->devices[i]->subordinate->subordinate >= bus)
-				return drhd->iommu;
+		if (dmaru->include_all) {
+			found = dmaru->iommu;
+			break;
+		}
+
+		if (dmar_match_scope(dmaru->scopes, dmaru->scopes_cnt,
+				     pdev, dmaru->segment)) {
+			found = dmaru->iommu;
+			break;
 		}
 
-		if (drhd->include_all)
-			return drhd->iommu;
 	}
 
-	return NULL;
+	pci_dev_put(pdev);
+
+	return found;
 }
 
 static void domain_flush_cache(struct dmar_domain *domain,
@@ -2227,7 +2229,7 @@ static int __init init_dmars(int force_on)
 	struct dmar_rmrr_unit *rmrr;
 	struct pci_dev *pdev;
 	struct intel_iommu *iommu;
-	int i, ret;
+	int ret;
 
 	/*
 	 * for each drhd
@@ -2376,18 +2378,22 @@ static int __init init_dmars(int force_on)
 	 */
 	printk(KERN_INFO "IOMMU: Setting RMRR:\n");
 	for_each_rmrr_units(rmrr) {
-		for (i = 0; i < rmrr->devices_cnt; i++) {
-			pdev = rmrr->devices[i];
-			/*
-			 * some BIOS lists non-exist devices in DMAR
-			 * table.
-			 */
+		struct acpi_dmar_reserved_memory *rmrrh;
+		int i;
+
+		rmrrh = container_of(rmrr->hdr,
+				     struct acpi_dmar_reserved_memory, header);
+
+		for (i = 0; i < rmrr->scopes_cnt; i++) {
+			pdev = dmar_get_scope_dev(rmrr->scopes[i],
+						  rmrrh->segment);
 			if (!pdev)
 				continue;
-			ret = iommu_prepare_rmrr_dev(rmrr, pdev);
-			if (ret)
+
+			if (iommu_prepare_rmrr_dev(rmrr, pdev))
 				printk(KERN_ERR
 				       "IOMMU: mapping reserved region failed\n");
+			pci_dev_put(pdev);
 		}
 	}
 
@@ -3072,15 +3078,21 @@ DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_IOAT_SNB, quir
 static void __init init_no_remapping_devices(void)
 {
 	struct dmar_drhd_unit *drhd;
+	struct pci_dev *pdev;
 
 	for_each_drhd_unit(drhd) {
 		if (!drhd->include_all) {
 			int i;
-			for (i = 0; i < drhd->devices_cnt; i++)
-				if (drhd->devices[i] != NULL)
+			for (i = 0; i < drhd->scopes_cnt; i++) {
+				pdev = dmar_get_scope_dev(drhd->scopes[i],
+							  drhd->segment);
+				if (pdev) {
+					pci_dev_put(pdev);
 					break;
+				}
+			}
 			/* ignore DMAR unit if no pci devices exist */
-			if (i == drhd->devices_cnt)
+			if (i == drhd->scopes_cnt)
 				drhd->ignored = 1;
 		}
 	}
@@ -3093,20 +3105,28 @@ static void __init init_no_remapping_devices(void)
 		if (drhd->ignored || drhd->include_all)
 			continue;
 
-		for (i = 0; i < drhd->devices_cnt; i++)
-			if (drhd->devices[i] &&
-				!IS_GFX_DEVICE(drhd->devices[i]))
+		for (i = 0; i < drhd->scopes_cnt; i++) {
+			pdev = dmar_get_scope_dev(drhd->scopes[i],
+						  drhd->segment);
+			if (pdev && !IS_GFX_DEVICE(pdev)) {
+				pci_dev_put(pdev);
 				break;
+			}
+			pci_dev_put(pdev);
+		}
 
-		if (i < drhd->devices_cnt)
+		if (i < drhd->scopes_cnt)
 			continue;
 
 		/* bypass IOMMU if it is just for gfx devices */
 		drhd->ignored = 1;
-		for (i = 0; i < drhd->devices_cnt; i++) {
-			if (!drhd->devices[i])
+		for (i = 0; i < drhd->scopes_cnt; i++) {
+			pdev = dmar_get_scope_dev(drhd->scopes[i],
+						  drhd->segment);
+			if (!pdev)
 				continue;
-			drhd->devices[i]->dev.archdata.iommu = DUMMY_DEVICE_DOMAIN_INFO;
+			pdev->dev.archdata.iommu = DUMMY_DEVICE_DOMAIN_INFO;
+			pci_dev_put(pdev);
 		}
 	}
 }
diff --git a/include/linux/dmar.h b/include/linux/dmar.h
index 7b776d7..cf071f9 100644
--- a/include/linux/dmar.h
+++ b/include/linux/dmar.h
@@ -32,8 +32,8 @@ struct dmar_drhd_unit {
 	struct list_head list;		/* list of drhd units	*/
 	struct  acpi_dmar_header *hdr;	/* ACPI header		*/
 	u64	reg_base_addr;		/* register base address*/
-	struct	pci_dev **devices; 	/* target device array	*/
-	int	devices_cnt;		/* target device count	*/
+	struct	acpi_dmar_device_scope **scopes; /* target scope array	*/
+	int	scopes_cnt;		/* target scope count	*/
 	u16	segment;		/* PCI domain		*/
 	u8	ignored:1; 		/* ignore drhd		*/
 	u8	include_all:1;
@@ -55,6 +55,9 @@ extern struct list_head dmar_drhd_units;
 
 extern int dmar_table_init(void);
 extern int dmar_dev_scope_init(void);
+extern int dmar_match_scope(struct acpi_dmar_device_scope **, int,
+			    struct pci_dev *, u16);
+extern struct pci_dev *dmar_get_scope_dev(struct acpi_dmar_device_scope *, u16);
 
 /* Intel IOMMU detection */
 extern int detect_intel_iommu(void);
@@ -72,6 +75,20 @@ static inline int dmar_table_init(void)
 {
 	return -ENODEV;
 }
+
+static inline int dmar_match_scope(struct acpi_dmar_device_scope **scopes,
+				   int cnt, struct pci_dev *dev, u16 segment)
+{
+	return 0;
+}
+
+static inline struct pci_dev *dmar_get_scope_dev(
+					struct acpi_dmar_device_scope *scope,
+					u16 segment)
+{
+	return NULL;
+}
+
 static inline int enable_drhd_fault_handling(void)
 {
 	return -1;
@@ -212,8 +229,8 @@ struct dmar_rmrr_unit {
 	struct acpi_dmar_header *hdr;	/* ACPI header		*/
 	u64	base_address;		/* reserved base address*/
 	u64	end_address;		/* reserved end address */
-	struct pci_dev **devices;	/* target devices */
-	int	devices_cnt;		/* target device count */
+	struct	acpi_dmar_device_scope **scopes; /* target scope array */
+	int	scopes_cnt;		/* target scope count */
 };
 
 #define for_each_rmrr_units(rmrr) \
@@ -222,8 +239,8 @@ struct dmar_rmrr_unit {
 struct dmar_atsr_unit {
 	struct list_head list;		/* list of ATSR units */
 	struct acpi_dmar_header *hdr;	/* ACPI header */
-	struct pci_dev **devices;	/* target devices */
-	int devices_cnt;		/* target device count */
+	struct acpi_dmar_device_scope **scopes;	/* target scope array */
+	int scopes_cnt;		/* target scope count */
 	u8 include_all:1;		/* include all ports */
 };
 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists