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.
>
> Not for commit
>
> Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> ---
>
> diff --git a/drivers/pci/dmar.c b/drivers/pci/dmar.c
> index 9cbeb0b..1d7bc58 100644
> --- a/drivers/pci/dmar.c
> +++ b/drivers/pci/dmar.c
> @@ -59,8 +59,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;
> @@ -72,7 +72,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);
> /*
> @@ -80,82 +80,103 @@ 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)++;
> else
> printk(KERN_WARNING PREFIX
> - "Unsupported device scope\n");
> + "Unsupported device scope\n");
> +
> 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;
> }
>
> @@ -204,9 +225,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);
> @@ -250,10 +270,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);
> }
> @@ -290,10 +310,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);
> }
> @@ -307,6 +326,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;
>
> list_for_each_entry(atsru, &dmar_atsr_units, list) {
> atsr = container_of(atsru->hdr, struct acpi_dmar_atsr, header);
> @@ -325,10 +345,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);
> + }
> }
> }
>
> @@ -475,23 +503,6 @@ parse_dmar_table(void)
> return ret;
> }
>
> -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)
> {
> @@ -504,11 +515,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 e7ff5b8..7d0b095 100644
> --- a/drivers/pci/intel-iommu.c
> +++ b/drivers/pci/intel-iommu.c
> @@ -541,32 +541,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,
> @@ -2250,7 +2252,7 @@ int __init init_dmars(void)
> struct dmar_rmrr_unit *rmrr;
> struct pci_dev *pdev;
> struct intel_iommu *iommu;
> - int i, ret;
> + int ret;
>
> /*
> * for each drhd
> @@ -2397,18 +2399,22 @@ int __init init_dmars(void)
> */
> 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);
> }
> }
>
> @@ -3078,15 +3084,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;
> }
> }
> @@ -3099,20 +3111,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 69a6fba..70b7e3c 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 void 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;
> @@ -203,8 +220,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) \
> @@ -213,8 +230,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-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
