Re: [PATCH v6 3/4] iommu/vt-d: Add set_dev_pasid callback for dma domain

[Baolu Lu]

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;
	}

?

> 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.

After the code consolidation done, the code here will become simpler and
appealing. We just need to tear down the pasid entry.

> > +static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
> > +				     struct device *dev, ioasid_t pasid)
> > +{
> > +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> > +	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> > +	struct intel_iommu *iommu = info->iommu;
> > +	struct dev_pasid_info *dev_pasid;
> > +	unsigned long flags;
> > +	int ret;
> > +
> > +	if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
> > +		return -EOPNOTSUPP;
> > +
> > +	if (context_copied(iommu, info->bus, info->devfn))
> > +		return -EBUSY;
> > +
> > +	ret = prepare_domain_attach_device(domain, dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
> > +	if (!dev_pasid)
> > +		return -ENOMEM;
> > +
> > +	ret = domain_attach_iommu(dmar_domain, iommu);
> > +	if (ret)
> > +		goto out_free;
> > +
> > +	if (domain_type_is_si(dmar_domain))
> > +		ret = intel_pasid_setup_pass_through(iommu, dmar_domain,
> > +						     dev, pasid);
> > +	else if (dmar_domain->use_first_level)
> > +		ret = domain_setup_first_level(iommu, dmar_domain,
> > +					       dev, pasid);
> > +	else
> > +		ret = intel_pasid_setup_second_level(iommu, dmar_domain,
> > +						     dev, pasid);
>
> 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.

It worth a separated refactoring series. Let me do it afterward.

Best regards,
baolu