Date:	Tue, 24 May 2011 13:24:38 -0700
From:	Yinghai Lu <yinghai@...nel.org>
To:	Alex Williamson <alex.williamson@...hat.com>
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 05/24/2011 01:07 PM, Alex Williamson wrote:
> 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,

can not find anywhere to call flush_unmaps_timeout(0).

so what do you mean flush patch?

> 
> 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