Re: [Patch Part2 V1 03/14] iommu/vt-d: simplify function get_domain_for_dev()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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.

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

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




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux