Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 11 Apr 2024 10:07:59 -0300 | From | Jason Gunthorpe <> | 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
| ![\](/images/icornerr.gif) |