| 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: <CAOtp4KpVbS6twWHukFWODDuwujG8BX6zYXOZiGRQM17f49UQ3w@mail.gmail.com>
Date: Wed, 8 Jan 2014 14:48:57 +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 2:31 PM, Jiang Liu <jiang.liu@...ux.intel.com> wrote:
>
>
> On 2014/1/8 14:06, Kai Huang wrote:
>> 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.
> 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. :)
-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@...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