linux-kernel - Re: [PATCH v2 12/12] iommu/vt-d: Retire struct intel

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240411130759.GJ223006@ziepe.ca>
Date: Thu, 11 Apr 2024 10:07:59 -0300
From: Jason Gunthorpe <jgg@...pe.ca>
To: Baolu Lu <baolu.lu@...ux.intel.com>
Cc: Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
	Robin Murphy <robin.murphy@....com>,
	Kevin Tian <kevin.tian@...el.com>,
	Tina Zhang <tina.zhang@...el.com>, Yi Liu <yi.l.liu@...el.com>,
	iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 12/12] iommu/vt-d: Retire struct intel_svm

On Thu, Apr 11, 2024 at 03:55:50PM +0800, Baolu Lu wrote:
> > > @@ -4388,14 +4386,8 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
> > >   	WARN_ON_ONCE(!dev_pasid);
> > >   	spin_unlock_irqrestore(&dmar_domain->lock, flags);
> > > -	/*
> > > -	 * The SVA implementation needs to handle its own stuffs like the mm
> > > -	 * notification. Before consolidating that code into iommu core, let
> > > -	 * the intel sva code handle it.
> > > -	 */
> > >   	if (domain->type == IOMMU_DOMAIN_SVA) {
> > >   		cache_tag_unassign_domain(dmar_domain, FLPT_DEFAULT_DID, dev, pasid);
> > > -		intel_svm_remove_dev_pasid(domain);
> > >   	} else {
> > >   		did = domain_id_iommu(dmar_domain, iommu);
> > >   		cache_tag_unassign_domain(dmar_domain, did, dev, pasid);
> > 
> > It seems very strange that SVA has a different DID scheme, why is
> > this? PASID and SVA should not be different at this layer.
> 
> The VT-d spec recommends that all SVA domains share a single domain ID.
> The PASID is unique to each SVA domain, hence the cache tags are unique.
> Currently, the Intel IOMMU driver assigns different domain IDs for all
> domains except the SVA type.
> 
> Sharing a domain ID is not specific to SVA. In general, for devices
> under a single IOMMU, domains with unique PASIDs can share the same
> domain ID.
> 
> In the long term (also on my task list), we will extend the cache tag
> code to support sharing domain IDs and remove the domain type check from
> the main code. This will also benefit the nesting case, where user
> domains nested on the same parent could share a domain ID.

Okay, that makes sense

> +static void intel_mm_free_notifier(struct mmu_notifier *mn)
> +{
> +       kfree(container_of(mn, struct dmar_domain, notifier));
> +}
> +
>  static const struct mmu_notifier_ops intel_mmuops = {
>         .release = intel_mm_release,
>         .arch_invalidate_secondary_tlbs =
> intel_arch_invalidate_secondary_tlbs,
> +       .free_notifier = intel_mm_free_notifier,
>  };
> 
>  static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
> @@ -598,10 +604,8 @@ static void intel_svm_domain_free(struct iommu_domain
> *domain)
>  {
>         struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> 
> -       if (dmar_domain->notifier.ops)
> -               mmu_notifier_unregister(&dmar_domain->notifier, domain->mm);
> -
> -       kfree(dmar_domain);
> +       /* dmar_domain free is defered to the mmu free_notifier callback. */
> +       mmu_notifier_put(&dmar_domain->notifier);
>  }

Yeah, that is better.

Also you need to have mmu notifier call on module unload when using
this scheme.

Jason