| 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
| ||
|
Date: Wed, 8 Jan 2014 14:56:47 +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:48 PM, Kai Huang <dev.kai.huang@...il.com> wrote:
> 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.
>
And just realized the domain_get_iommu just takes the domain as
argument, so it's really not used to get the IOMMU from device. I am
not sure in current code if there's such interface like
get_iommu_from_device(struct pci_dev *dev) or not, probably we don't
need it. :)
-Kai
> 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