Re: [PATCH v2 12/12] iommu/vt-d: Retire struct intel_svm

[Jason Gunthorpe]

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