On 2014/1/8 14:06, Kai Huang wrote: > On Wed, Jan 8, 2014 at 1:48 PM, Jiang Liu <jiang.liu@xxxxxxxxxxxxxxx> wrote: >> >> >> On 2014/1/8 11:06, Kai Huang wrote: >>> >>> >>> >>> On Tue, Jan 7, 2014 at 5:00 PM, Jiang Liu <jiang.liu@xxxxxxxxxxxxxxx >>> <mailto:jiang.liu@xxxxxxxxxxxxxxx>> wrote: >>> >>> Function get_domain_for_dev() is a little complex, simplify it >>> by factoring out dmar_search_domain_by_dev_info() and >>> dmar_insert_dev_info(). >>> >>> Signed-off-by: Jiang Liu <jiang.liu@xxxxxxxxxxxxxxx >>> <mailto:jiang.liu@xxxxxxxxxxxxxxx>> >>> --- >>> drivers/iommu/intel-iommu.c | 161 >>> ++++++++++++++++++++----------------------- >>> 1 file changed, 75 insertions(+), 86 deletions(-) >>> >>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c >>> index 1704e97..da65884 100644 >>> --- a/drivers/iommu/intel-iommu.c >>> +++ b/drivers/iommu/intel-iommu.c >>> @@ -1957,18 +1957,70 @@ find_domain(struct pci_dev *pdev) >>> return NULL; >>> } >>> >>> +static inline struct dmar_domain * >>> +dmar_search_domain_by_dev_info(int segment, int bus, int devfn) >>> +{ >>> + struct device_domain_info *info; >>> + >>> + list_for_each_entry(info, &device_domain_list, global) >>> + if (info->segment == segment && info->bus == bus && >>> + info->devfn == devfn) >>> + return info->domain; >>> + >>> + return NULL; >>> +} >>> + >>> +static int dmar_insert_dev_info(int segment, int bus, int devfn, >>> + struct pci_dev *dev, struct >>> dmar_domain **domp) >>> +{ >>> + struct dmar_domain *found, *domain = *domp; >>> + struct device_domain_info *info; >>> + unsigned long flags; >>> + >>> + info = alloc_devinfo_mem(); >>> + if (!info) >>> + return -ENOMEM; >>> + >>> + info->segment = segment; >>> + info->bus = bus; >>> + info->devfn = devfn; >>> + info->dev = dev; >>> + info->domain = domain; >>> + if (!dev) >>> + domain->flags |= DOMAIN_FLAG_P2P_MULTIPLE_DEVICES; >>> + >>> + spin_lock_irqsave(&device_domain_lock, flags); >>> + if (dev) >>> + found = find_domain(dev); >>> + else >>> + found = dmar_search_domain_by_dev_info(segment, bus, >>> devfn); >>> + if (found) { >>> + spin_unlock_irqrestore(&device_domain_lock, flags); >>> + free_devinfo_mem(info); >>> + if (found != domain) { >>> + domain_exit(domain); >>> + *domp = found; >>> + } >>> + } else { >>> + list_add(&info->link, &domain->devices); >>> + list_add(&info->global, &device_domain_list); >>> + if (dev) >>> + dev->dev.archdata.iommu = info; >>> + spin_unlock_irqrestore(&device_domain_lock, flags); >>> + } >>> + >>> + return 0; >>> +} >>> >>> >>> I am a little bit confused about the "alloc before check" sequence in >>> above function. I believe the purpose of allocating the >>> device_domain_info before searching the domain with spin_lock hold is to >>> avoid the memory allocation in the spin_lock? In this case, why can't we >>> use mutex instead of spin_lock? In my understanding, if we use mutex, we >>> can first check if the domain exists or not before we really allocate >>> device_domain_info, right? >> Hi Kai, >> Thanks for review! >> The domain->devices list may be access in interrupt context, >> so we need to protect it with spin_lock_irqsave(). Otherwise it may >> case deadlock. >> > > Thanks. That's what I am thinking. I observed lots of other iommu > functions also use spin_lock. > >>> >>> Another question is when is info->iommu field initialized? Looks it is >>> not initialized here? >> It's set in function iommu_support_dev_iotlb() for devices supporting >> device IOTLB. > > Thanks. I see in iommu_support_dev_iotlb, info->iommu won't be set > unless the device supports iotlb and ATS. Does this mean the > info->iommu won't be used if the device doesn't support iotlb? > > If this is true, looks it's not convenient to find the IOMMU that > handles the device via info, as it's possible that different IOMMUs > share the same domain, and we don't know which IOMMU actually handles > this device if we try to find it via the info->domain. It's a little heavy to find info by walking the list too:) Please refer to domain_get_iommu() for the way to find iommu associated with a domain. > > -Kai > >> >>> >>> -Kai >>> >>> + >>> /* domain is initialized */ >>> static struct dmar_domain *get_domain_for_dev(struct pci_dev *pdev, >>> int gaw) >>> { >>> - struct dmar_domain *domain, *found = NULL; >>> + struct dmar_domain *domain; >>> struct intel_iommu *iommu; >>> struct dmar_drhd_unit *drhd; >>> - struct device_domain_info *info, *tmp; >>> - struct pci_dev *dev_tmp; >>> + struct pci_dev *bridge; >>> unsigned long flags; >>> - int bus = 0, devfn = 0; >>> - int segment; >>> - int ret; >>> + int segment, bus, devfn; >>> >>> domain = find_domain(pdev); >>> if (domain) >>> @@ -1976,119 +2028,56 @@ 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; >>> + bridge = pci_find_upstream_pcie_bridge(pdev); >>> + if (bridge) { >>> + if (pci_is_pcie(bridge)) { >>> + bus = bridge->subordinate->number; >>> devfn = 0; >>> } else { >>> - bus = dev_tmp->bus->number; >>> - devfn = dev_tmp->devfn; >>> + bus = bridge->bus->number; >>> + devfn = bridge->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; >>> - } >>> - } >>> + domain = dmar_search_domain_by_dev_info(segment, >>> bus, devfn); >>> spin_unlock_irqrestore(&device_domain_lock, flags); >>> /* pcie-pci bridge already has a domain, uses it */ >>> - if (found) { >>> - domain = found; >>> + if (domain) >>> goto found_domain; >>> - } >>> } >>> >>> - domain = alloc_domain(); >>> - if (!domain) >>> - goto error; >>> - >>> - /* Allocate new domain for the device */ >>> drhd = dmar_find_matched_drhd_unit(pdev); >>> if (!drhd) { >>> printk(KERN_ERR "IOMMU: can't find DMAR for device >>> %s\n", >>> pci_name(pdev)); >>> - free_domain_mem(domain); >>> return NULL; >>> } >>> iommu = drhd->iommu; >>> >>> - ret = iommu_attach_domain(domain, iommu); >>> - if (ret) { >>> + /* Allocate new domain for the device */ >>> + domain = alloc_domain(); >>> + if (!domain) >>> + goto error; >>> + if (iommu_attach_domain(domain, iommu)) { >>> free_domain_mem(domain); >>> goto error; >>> } >>> - >>> if (domain_init(domain, gaw)) { >>> domain_exit(domain); >>> goto error; >>> } >>> >>> /* register pcie-to-pci device */ >>> - if (dev_tmp) { >>> - info = alloc_devinfo_mem(); >>> - if (!info) { >>> + if (bridge) { >>> + if (dmar_insert_dev_info(segment, bus, devfn, NULL, >>> &domain)) { >>> domain_exit(domain); >>> goto error; >>> } >>> - info->segment = segment; >>> - info->bus = bus; >>> - info->devfn = devfn; >>> - info->dev = NULL; >>> - info->domain = domain; >>> - /* This domain is shared by devices under p2p bridge */ >>> - domain->flags |= DOMAIN_FLAG_P2P_MULTIPLE_DEVICES; >>> - >>> - /* pcie-to-pci bridge already has a domain, uses it */ >>> - found = NULL; >>> - spin_lock_irqsave(&device_domain_lock, flags); >>> - list_for_each_entry(tmp, &device_domain_list, global) { >>> - if (tmp->segment == segment && >>> - tmp->bus == bus && tmp->devfn == devfn) { >>> - found = tmp->domain; >>> - break; >>> - } >>> - } >>> - if (found) { >>> - spin_unlock_irqrestore(&device_domain_lock, >>> flags); >>> - free_devinfo_mem(info); >>> - domain_exit(domain); >>> - domain = found; >>> - } else { >>> - list_add(&info->link, &domain->devices); >>> - list_add(&info->global, &device_domain_list); >>> - spin_unlock_irqrestore(&device_domain_lock, >>> flags); >>> - } >>> } >>> >>> found_domain: >>> - info = alloc_devinfo_mem(); >>> - if (!info) >>> - goto error; >>> - info->segment = segment; >>> - info->bus = pdev->bus->number; >>> - info->devfn = pdev->devfn; >>> - info->dev = pdev; >>> - info->domain = domain; >>> - spin_lock_irqsave(&device_domain_lock, flags); >>> - /* somebody is fast */ >>> - found = find_domain(pdev); >>> - if (found != NULL) { >>> - spin_unlock_irqrestore(&device_domain_lock, flags); >>> - if (found != domain) { >>> - domain_exit(domain); >>> - domain = found; >>> - } >>> - free_devinfo_mem(info); >>> + if (dmar_insert_dev_info(segment, pdev->bus->number, >>> pdev->devfn, >>> + pdev, &domain) == 0) >>> return domain; >>> - } >>> - list_add(&info->link, &domain->devices); >>> - list_add(&info->global, &device_domain_list); >>> - pdev->dev.archdata.iommu = info; >>> - spin_unlock_irqrestore(&device_domain_lock, flags); >>> - return domain; >>> error: >>> /* recheck it here, maybe others set it */ >>> return find_domain(pdev); >>> -- >>> 1.7.10.4 >>> >>> _______________________________________________ >>> iommu mailing list >>> iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx >>> <mailto:iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx> >>> https://lists.linuxfoundation.org/mailman/listinfo/iommu >>> >>> -- 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
