On Tue, May 30, 2023 at 10:19:05AM +0800, Baolu Lu wrote: > On 5/30/23 3:48 AM, Jason Gunthorpe wrote: > > On Fri, May 19, 2023 at 01:32:22PM -0700, Jacob Pan wrote: > > > > > @@ -4720,25 +4762,99 @@ static void intel_iommu_iotlb_sync_map(struct iommu_domain *domain, > > > static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid) > > > { > > > struct intel_iommu *iommu = device_to_iommu(dev, NULL, NULL); > > > + struct dev_pasid_info *curr, *dev_pasid = NULL; > > > + struct dmar_domain *dmar_domain; > > > struct iommu_domain *domain; > > > + unsigned long flags; > > > - /* Domain type specific cleanup: */ > > > domain = iommu_get_domain_for_dev_pasid(dev, pasid, 0); > > > - if (domain) { > > > - switch (domain->type) { > > > - case IOMMU_DOMAIN_SVA: > > > - intel_svm_remove_dev_pasid(dev, pasid); > > > - break; > > > - default: > > > - /* should never reach here */ > > > - WARN_ON(1); > > > + if (!domain) > > > + goto out_tear_down; > > > + > > > + /* > > > + * The SVA implementation needs to stop mm notification, drain the > > > + * pending page fault requests before tearing down the pasid entry. > > > + * The VT-d spec (section 6.2.3.1) also recommends that software > > > + * could use a reserved domain id for all first-only and pass-through > > > + * translations. Hence there's no need to call domain_detach_iommu() > > > + * in the sva domain case. > > > + */ > > > + if (domain->type == IOMMU_DOMAIN_SVA) { > > > + intel_svm_remove_dev_pasid(dev, pasid); > > > + goto out_tear_down; > > > + } > > > > But why don't you need to do all the other > > intel_pasid_tear_down_entry(), intel_svm_drain_prq() (which is > > misnamed) and other stuff from intel_svm_remove_dev_pasid() ? > > Perhaps, > > if (domain->type == IOMMU_DOMAIN_SVA) { > intel_svm_remove_dev_pasid(dev, pasid); > return; > } > > ? I would expect only stuff directly connected to SVM be in the SVM function. De-initalizing PRI and any other pasid destruction should be in this function. > > There still seems to be waaay too much "SVM" in the PASID code. > > This segment of code is destined to be temporary. From a long-term > perspective, I hope to move SVA specific staffs such as mm notification, > prq draining, etc. to the iommu core. They are generic rather than Intel > iommu specific. Yes, sort of, but.. That is just the mmu notifier bits All the PRI/PASID teardown needs to be unlinked from SVM > > It would be nice if the different domain types had their own ops.. > > Good suggestion! > > We can add a domain ops in the Intel domain structure which is > responsible for how to install an Intel iommu domain onto the VT-d > hardware. We should have seperate iommu_domain_ops at least, I think that would cover alot of it? Jason