[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241011162703.GL762027@ziepe.ca>
Date: Fri, 11 Oct 2024 13:27:03 -0300
From: Jason Gunthorpe <jgg@...pe.ca>
To: Lu Baolu <baolu.lu@...ux.intel.com>
Cc: iommu@...ts.linux.dev, Joerg Roedel <joro@...tes.org>,
Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>,
Kevin Tian <kevin.tian@...el.com>, Yi Liu <yi.l.liu@...el.com>,
Vasant Hegde <vasant.hegde@....com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/7] iommu/vt-d: Enhance compatibility check for paging
domain attach
On Fri, Oct 11, 2024 at 12:27:18PM +0800, Lu Baolu wrote:
> @@ -1623,27 +1623,15 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
>
> copied_context_tear_down(iommu, context, bus, devfn);
> context_clear_entry(context);
> -
> context_set_domain_id(context, did);
>
> - /*
> - * Skip top levels of page tables for iommu which has
> - * less agaw than default. Unnecessary for PT mode.
> - */
> - for (agaw = domain->agaw; agaw > iommu->agaw; agaw--) {
> - ret = -ENOMEM;
> - pgd = phys_to_virt(dma_pte_addr(pgd));
> - if (!dma_pte_present(pgd))
> - goto out_unlock;
> - }
Yikes, this is nasty racy stuff, glad to see it go
But should the agaw stuff be in its own patch?
> @@ -3506,27 +3483,26 @@ int prepare_domain_attach_device(struct iommu_domain *domain,
> if (domain->dirty_ops && !ssads_supported(iommu))
> return -EINVAL;
>
> - /* check if this iommu agaw is sufficient for max mapped address */
> - addr_width = agaw_to_width(iommu->agaw);
> - if (addr_width > cap_mgaw(iommu->cap))
> - addr_width = cap_mgaw(iommu->cap);
> -
> - if (dmar_domain->max_addr > (1LL << addr_width))
> + if (dmar_domain->iommu_coherency !=
> + iommu_paging_structure_coherency(iommu))
> return -EINVAL;
> - dmar_domain->gaw = addr_width;
> -
> - /*
> - * Knock out extra levels of page tables if necessary
> - */
> - while (iommu->agaw < dmar_domain->agaw) {
> - struct dma_pte *pte;
> -
> - pte = dmar_domain->pgd;
> - if (dma_pte_present(pte)) {
> - dmar_domain->pgd = phys_to_virt(dma_pte_addr(pte));
> - iommu_free_page(pte);
> - }
> - dmar_domain->agaw--;
> +
> + if (domain->type & __IOMMU_DOMAIN_PAGING) {
It looks like this entire function is already never called for
anything but paging?
The only three callers are:
.default_domain_ops = &(const struct iommu_domain_ops) {
.attach_dev = intel_iommu_attach_device,
.set_dev_pasid = intel_iommu_set_dev_pasid,
and
static const struct iommu_domain_ops intel_nested_domain_ops = {
.attach_dev = intel_nested_attach_dev,
And none of those cases can be anything except a paging domain by
definition.
So this if should go away, or be turned into a WARN_ON.
Jason
Powered by blists - more mailing lists