Received-SPF: pass (google.com: domain of linux-kernel+bounces-140483-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 11 Apr 2024 10:07:59 -0300
From: Jason Gunthorpe <jgg@ziepe.ca>
To: Baolu Lu <baolu.lu@linux.intel.com>
Cc: Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Kevin Tian <kevin.tian@intel.com>,
	Tina Zhang <tina.zhang@intel.com>, Yi Liu <yi.l.liu@intel.com>,
	iommu@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 12/12] iommu/vt-d: Retire struct intel_svm
Message-ID: <20240411130759.GJ223006@ziepe.ca>
References: <20240410020844.253535-1-baolu.lu@linux.intel.com>
 <20240410020844.253535-13-baolu.lu@linux.intel.com>
 <20240410154951.GH223006@ziepe.ca>
 <b24f3380-1ac3-41c4-9163-56c1dcff6297@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b24f3380-1ac3-41c4-9163-56c1dcff6297@linux.intel.com>

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