| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAOtp4Kqo5m-uOKfr8WDwH1v3+23iSv9_329xS=K76Kpq-QXdVw@mail.gmail.com>
Date: Wed, 8 Jan 2014 14:06:36 +0800
From: Kai Huang <dev.kai.huang@...il.com>
To: Jiang Liu <jiang.liu@...ux.intel.com>
Cc: Joerg Roedel <joro@...tes.org>,
David Woodhouse <dwmw2@...radead.org>,
Yinghai Lu <yinghai@...nel.org>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Dan Williams <dan.j.williams@...el.com>,
Vinod Koul <vinod.koul@...el.com>,
Tony Luck <tony.luck@...el.com>, linux-pci@...r.kernel.org,
linux-kernel@...r.kernel.org, dmaengine@...r.kernel.org,
iommu@...ts.linux-foundation.org
Subject: Re: [Patch Part2 V1 03/14] iommu/vt-d: simplify function get_domain_for_dev()
On Wed, Jan 8, 2014 at 1:48 PM, Jiang Liu <jiang.liu@...ux.intel.com> wrote:
>
>
> On 2014/1/8 11:06, Kai Huang wrote:
>>
>>
>>
>> On Tue, Jan 7, 2014 at 5:00 PM, Jiang Liu <jiang.liu@...ux.intel.com
>> <mailto:jiang.liu@...ux.intel.com>> 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@...ux.intel.com
>> <mailto:jiang.liu@...ux.intel.com>>
>> ---
>> 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.
-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@...ts.linux-foundation.org
>> <mailto:iommu@...ts.linux-foundation.org>
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
>>
--
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