On 2014/1/8 14:48, Kai Huang wrote: > On Wed, Jan 8, 2014 at 2:31 PM, Jiang Liu <jiang.liu@xxxxxxxxxxxxxxx> wrote: >> >> >> 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. >> > Looks domain_get_iommu just returns the first IOMMU in the bitmap, so > it can't return the IOMMU that *really* handles the device in > hardware. But I guess from domain's view, probably we don't care which > IOMMU really handles the device, cause if multiple IOMMUs share the > same domain, the IOMMUs should have the same (or very similar) > capabilities, so referencing the first IOMMU to check some > capabilities (that's what I see in code so far) should work for the > domain layer API. > > But looks it's safe to set info->iommu in get_domain_for_dev anyway > (and remove it in iommu_support_dev_iotlb)? In this case it's easier > when we want to get IOMMU from info. > > Of course, it's just my opinion, and probably is wrong, it's totally > up to you if want to do this or not. :) Actually I'm on your side, I think it's better to initialize info->iommu in function dmar_insert_dev_info() too. Will try that way. > > -Kai > >>> >>> -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
