From: Lu Baolu <baolu.lu@linux.intel.com>
To: James Sewart <jamessewart@arista.com>, iommu@lists.linux-foundation.org
Cc: baolu.lu@linux.intel.com, Tom Murphy <tmurphy@arista.com>,
Dmitry Safonov <dima@arista.com>,
Jacob Pan <jacob.jun.pan@linux.intel.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/4] iommu/vt-d: Remove lazy allocation of domains
Date: Tue, 5 Mar 2019 14:59:48 +0800 [thread overview]
Message-ID: <fdd74d83-acd4-23da-6c0c-b52573d999bb@linux.intel.com> (raw)
In-Reply-To: <7F6B5F6A-EC76-4A9F-8EB6-AEAB9994D91A@arista.com>
Hi,
It's hard for me to understand why do we remove the rmrr related
code in this patch.
And, now we have two places to hold a domain for a device: group and
dev->info. We can consider to optimize the use of per device iommu
arch data. This should be later work anyway.
More comments inline.
On 3/4/19 11:47 PM, James Sewart wrote:
> The generic IOMMU code will allocate and attach a dma ops domain to each
> device that comes online, replacing any lazy allocated domain. Removes
> RMRR application at iommu init time as we won't have a domain attached
> to any device. iommu.c will do this after attaching a device using
> create_direct_mappings.
>
> Signed-off-by: James Sewart <jamessewart@arista.com>
> ---
> drivers/iommu/intel-iommu.c | 202 ++----------------------------------
> 1 file changed, 8 insertions(+), 194 deletions(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 71cd6bbfec05..282257e2628d 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -2595,118 +2595,6 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
> return domain;
> }
>
> -static int get_last_alias(struct pci_dev *pdev, u16 alias, void *opaque)
> -{
> - *(u16 *)opaque = alias;
> - return 0;
> -}
> -
> -static struct dmar_domain *find_or_alloc_domain(struct device *dev, int gaw)
> -{
> - struct device_domain_info *info = NULL;
> - struct dmar_domain *domain = NULL;
> - struct intel_iommu *iommu;
> - u16 dma_alias;
> - unsigned long flags;
> - u8 bus, devfn;
> -
> - iommu = device_to_iommu(dev, &bus, &devfn);
> - if (!iommu)
> - return NULL;
> -
> - if (dev_is_pci(dev)) {
> - struct pci_dev *pdev = to_pci_dev(dev);
> -
> - pci_for_each_dma_alias(pdev, get_last_alias, &dma_alias);
> -
> - spin_lock_irqsave(&device_domain_lock, flags);
> - info = dmar_search_domain_by_dev_info(pci_domain_nr(pdev->bus),
> - PCI_BUS_NUM(dma_alias),
> - dma_alias & 0xff);
> - if (info) {
> - iommu = info->iommu;
> - domain = info->domain;
> - }
> - spin_unlock_irqrestore(&device_domain_lock, flags);
> -
> - /* DMA alias already has a domain, use it */
> - if (info)
> - goto out;
> - }
> -
> - /* Allocate and initialize new domain for the device */
> - domain = alloc_domain(0);
> - if (!domain)
> - return NULL;
> - if (domain_init(domain, iommu, gaw)) {
> - domain_exit(domain);
> - return NULL;
> - }
> -
> -out:
> -
> - return domain;
> -}
> -
> -static struct dmar_domain *set_domain_for_dev(struct device *dev,
> - struct dmar_domain *domain)
> -{
> - struct intel_iommu *iommu;
> - struct dmar_domain *tmp;
> - u16 req_id, dma_alias;
> - u8 bus, devfn;
> -
> - iommu = device_to_iommu(dev, &bus, &devfn);
> - if (!iommu)
> - return NULL;
> -
> - req_id = ((u16)bus << 8) | devfn;
> -
> - if (dev_is_pci(dev)) {
> - struct pci_dev *pdev = to_pci_dev(dev);
> -
> - pci_for_each_dma_alias(pdev, get_last_alias, &dma_alias);
> -
> - /* register PCI DMA alias device */
> - if (req_id != dma_alias) {
> - tmp = dmar_insert_one_dev_info(iommu, PCI_BUS_NUM(dma_alias),
> - dma_alias & 0xff, NULL, domain);
> -
> - if (!tmp || tmp != domain)
> - return tmp;
> - }
> - }
> -
> - tmp = dmar_insert_one_dev_info(iommu, bus, devfn, dev, domain);
> - if (!tmp || tmp != domain)
> - return tmp;
> -
> - return domain;
> -}
> -
> -static struct dmar_domain *get_domain_for_dev(struct device *dev, int gaw)
> -{
> - struct dmar_domain *domain, *tmp;
> -
> - domain = find_domain(dev);
> - if (domain)
> - goto out;
> -
> - domain = find_or_alloc_domain(dev, gaw);
> - if (!domain)
> - goto out;
> -
> - tmp = set_domain_for_dev(dev, domain);
> - if (!tmp || domain != tmp) {
> - domain_exit(domain);
> - domain = tmp;
> - }
> -
> -out:
> -
> - return domain;
> -}
> -
> static int iommu_domain_identity_map(struct dmar_domain *domain,
> unsigned long long start,
> unsigned long long end)
> @@ -2779,7 +2667,7 @@ static int iommu_prepare_identity_map(struct device *dev,
> struct dmar_domain *domain;
> int ret;
>
> - domain = get_domain_for_dev(dev, DEFAULT_DOMAIN_ADDRESS_WIDTH);
> + domain = find_domain(dev);
> if (!domain)
> return -ENOMEM;
>
> @@ -3301,11 +3189,9 @@ static int copy_translation_tables(struct intel_iommu *iommu)
> static int __init init_dmars(void)
> {
> struct dmar_drhd_unit *drhd;
> - struct dmar_rmrr_unit *rmrr;
> bool copied_tables = false;
> - struct device *dev;
> struct intel_iommu *iommu;
> - int i, ret;
> + int ret;
>
> /*
> * for each drhd
> @@ -3466,32 +3352,6 @@ static int __init init_dmars(void)
> goto free_iommu;
> }
> }
> - /*
> - * For each rmrr
> - * for each dev attached to rmrr
> - * do
> - * locate drhd for dev, alloc domain for dev
> - * allocate free domain
> - * allocate page table entries for rmrr
> - * if context not allocated for bus
> - * allocate and init context
> - * set present in root table for this bus
> - * init context with domain, translation etc
> - * endfor
> - * endfor
> - */
> - pr_info("Setting RMRR:\n");
> - for_each_rmrr_units(rmrr) {
> - /* some BIOS lists non-exist devices in DMAR table. */
> - for_each_active_dev_scope(rmrr->devices, rmrr->devices_cnt,
> - i, dev) {
> - ret = iommu_prepare_rmrr_dev(rmrr, dev);
> - if (ret)
> - pr_err("Mapping reserved region failed\n");
> - }
> - }
> -
> - iommu_prepare_isa();
Why do you want to remove this segment of code?
>
> domains_done:
>
> @@ -3580,53 +3440,6 @@ static unsigned long intel_alloc_iova(struct device *dev,
> return iova_pfn;
> }
>
> -struct dmar_domain *get_valid_domain_for_dev(struct device *dev)
> -{
> - struct dmar_domain *domain, *tmp;
> - struct dmar_rmrr_unit *rmrr;
> - struct device *i_dev;
> - int i, ret;
> -
> - domain = find_domain(dev);
> - if (domain)
> - goto out;
> -
> - domain = find_or_alloc_domain(dev, DEFAULT_DOMAIN_ADDRESS_WIDTH);
> - if (!domain)
> - goto out;
> -
> - /* We have a new domain - setup possible RMRRs for the device */
> - rcu_read_lock();
> - for_each_rmrr_units(rmrr) {
> - for_each_active_dev_scope(rmrr->devices, rmrr->devices_cnt,
> - i, i_dev) {
> - if (i_dev != dev)
> - continue;
> -
> - ret = domain_prepare_identity_map(dev, domain,
> - rmrr->base_address,
> - rmrr->end_address);
> - if (ret)
> - dev_err(dev, "Mapping reserved region failed\n");
We can't simply remove this segment of code, right? At least it should
be moved to the domain allocation interface.
> - }
> - }
> - rcu_read_unlock();
> -
> - tmp = set_domain_for_dev(dev, domain);
> - if (!tmp || domain != tmp) {
> - domain_exit(domain);
> - domain = tmp;
> - }
> -
> -out:
> -
> - if (!domain)
> - pr_err("Allocating domain for %s failed\n", dev_name(dev));
> -
> -
> - return domain;
> -}
> -
> /* Check if the dev needs to go through non-identity map and unmap process.*/
> static int iommu_no_mapping(struct device *dev)
> {
> @@ -3689,7 +3502,7 @@ static dma_addr_t __intel_map_page(struct device *dev, struct page *page,
> if (iommu_no_mapping(dev))
> return paddr;
>
> - domain = get_valid_domain_for_dev(dev);
> + domain = find_domain(dev);
> if (!domain)
> return DMA_MAPPING_ERROR;
>
> @@ -3753,7 +3566,8 @@ static void intel_unmap(struct device *dev, dma_addr_t dev_addr, size_t size)
> return;
>
> domain = find_domain(dev);
> - BUG_ON(!domain);
> + if (!domain)
> + return;
>
This is not related to this patch. Let's do it in a separated patch.
> iommu = domain_get_iommu(domain);
>
> @@ -3899,7 +3713,7 @@ static int intel_map_sg(struct device *dev, struct scatterlist *sglist, int nele
> if (iommu_no_mapping(dev))
> return intel_nontranslate_map_sg(dev, sglist, nelems, dir);
>
> - domain = get_valid_domain_for_dev(dev);
> + domain = find_domain(dev);
> if (!domain)
> return 0;
>
> @@ -5377,9 +5191,9 @@ int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct intel_svm_dev *sd
> u64 ctx_lo;
> int ret;
>
> - domain = get_valid_domain_for_dev(sdev->dev);
> + domain = find_domain(sdev->dev);
> if (!domain)
> - return -EINVAL;
> + return -ENOMEM;
This is not related to this patch. Let's do it in a separated patch.
>
> spin_lock_irqsave(&device_domain_lock, flags);
> spin_lock(&iommu->lock);
>
Best regards,
Lu Baolu
next prev parent reply other threads:[~2019-03-05 7:05 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-04 15:41 [PATCH 0/4] iommu/vt-d: Fix-up device-domain relationship by refactoring to use iommu group default domain James Sewart
2019-03-04 15:45 ` [PATCH 1/4] iommu: Move iommu_group_create_direct_mappings to after device_attach James Sewart
2019-03-04 15:46 ` [PATCH 2/4] iommu/vt-d: Implement apply_resv_region for reserving IOVA ranges James Sewart
2019-03-04 15:46 ` [PATCH 3/4] iommu/vt-d: Allow IOMMU_DOMAIN_DMA and IOMMU_DOMAIN_IDENTITY to be allocated James Sewart
2019-03-04 15:47 ` [PATCH 4/4] iommu/vt-d: Remove lazy allocation of domains James Sewart
2019-03-05 6:59 ` Lu Baolu [this message]
2019-03-05 11:46 ` James Sewart
2019-03-06 7:00 ` Lu Baolu
2019-03-06 18:08 ` James Sewart
2019-03-07 6:31 ` Lu Baolu
2019-03-07 10:21 ` James Sewart
2019-03-08 1:09 ` Lu Baolu
2019-03-08 3:09 ` Lu Baolu
2019-03-08 16:57 ` James Sewart
2019-03-09 1:53 ` Lu Baolu
2019-03-09 11:49 ` James Sewart
2019-03-10 2:51 ` Lu Baolu
2019-03-05 6:46 ` [PATCH 3/4] iommu/vt-d: Allow IOMMU_DOMAIN_DMA and IOMMU_DOMAIN_IDENTITY to be allocated Lu Baolu
2019-03-05 11:34 ` James Sewart
2019-03-08 1:20 ` Dmitry Safonov
2019-03-09 11:57 ` James Sewart
2019-03-05 6:05 ` [PATCH 0/4] iommu/vt-d: Fix-up device-domain relationship by refactoring to use iommu group default domain Lu Baolu
2019-03-05 11:14 ` James Sewart
2019-03-06 6:27 ` Lu Baolu
2019-03-14 11:56 ` [PATCH v2 0/7] " James Sewart
2019-03-14 11:57 ` [PATCH v2 1/7] iommu: Move iommu_group_create_direct_mappings to after device_attach James Sewart
2019-03-14 11:58 ` [PATCH v2 2/7] iommu/vt-d: Implement apply_resv_region for reserving IOVA ranges James Sewart
2019-03-14 11:58 ` [PATCH v2 3/7] iommu/vt-d: Expose ISA direct mapping region via iommu_get_resv_regions James Sewart
2019-03-15 2:19 ` Lu Baolu
2019-03-22 9:57 ` James Sewart
2019-03-25 2:03 ` Lu Baolu
2019-03-25 12:57 ` James Sewart
2019-03-26 1:10 ` Lu Baolu
2019-03-26 1:24 ` Lu Baolu
2019-03-28 18:37 ` James Sewart
2019-03-29 15:26 ` James Sewart
2019-04-04 6:49 ` Lu Baolu
2019-04-05 18:02 ` James Sewart
2019-04-08 2:43 ` Lu Baolu
2019-04-10 5:22 ` Lu Baolu
2019-04-15 14:16 ` James Sewart
2019-04-16 2:18 ` Lu Baolu
2019-04-24 23:47 ` Tom Murphy
2019-04-25 1:15 ` Lu Baolu
2019-03-14 11:58 ` [PATCH v2 4/7] iommu/vt-d: Ignore domain parameter in attach_device if device requires identity map James Sewart
2019-03-15 2:30 ` Lu Baolu
2019-03-14 11:58 ` [PATCH v2 5/7] iommu/vt-d: Enable DMA remapping after rmrr mapped James Sewart
2019-03-14 11:59 ` [PATCH v2 6/7] iommu/vt-d: Allow IOMMU_DOMAIN_DMA to be allocated by iommu_ops James Sewart
2019-03-14 11:59 ` [PATCH v2 7/7] iommu/vt-d: Remove lazy allocation of domains James Sewart
2019-03-14 23:35 ` Jacob Pan
2019-03-22 10:07 ` James Sewart
2019-03-15 3:13 ` [PATCH v2 0/7] iommu/vt-d: Fix-up device-domain relationship by refactoring to use iommu group default domain Lu Baolu
2019-03-19 13:35 ` James Sewart
2019-03-20 1:26 ` Lu Baolu
2019-03-22 10:05 ` James Sewart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fdd74d83-acd4-23da-6c0c-b52573d999bb@linux.intel.com \
--to=baolu.lu@linux.intel.com \
--cc=dima@arista.com \
--cc=iommu@lists.linux-foundation.org \
--cc=jacob.jun.pan@linux.intel.com \
--cc=jamessewart@arista.com \
--cc=linux-kernel@vger.kernel.org \
--cc=tmurphy@arista.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).