Re: [PATCH v3 1/7] iommu: Introduce a replace API for device pasid

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Aug 16, 2024 at 05:43:18PM +0800, Yi Liu wrote:
> On 2024/7/18 16:27, Tian, Kevin wrote:
> > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > Sent: Friday, June 28, 2024 5:06 PM
> > > 
> > > @@ -3289,7 +3290,20 @@ static int __iommu_set_group_pasid(struct
> > > iommu_domain *domain,
> > > 
> > >   		if (device == last_gdev)
> > >   			break;
> > > -		ops->remove_dev_pasid(device->dev, pasid, domain);
> > > +		/* If no old domain, undo the succeeded devices/pasid */
> > > +		if (!old) {
> > > +			ops->remove_dev_pasid(device->dev, pasid, domain);
> > > +			continue;
> > > +		}
> > > +
> > > +		/*
> > > +		 * Rollback the succeeded devices/pasid to the old domain.
> > > +		 * And it is a driver bug to fail attaching with a previously
> > > +		 * good domain.
> > > +		 */
> > > +		if (WARN_ON(old->ops->set_dev_pasid(old, device->dev,
> > > +						    pasid, domain)))
> > > +			ops->remove_dev_pasid(device->dev, pasid, domain);
> > 
> > I wonder whether @remove_dev_pasid() can be replaced by having
> > blocking_domain support @set_dev_pasid?
> 
> how about your thought, @Jason?

I think we talked about doing that once before, I forget why it was
not done. Maybe there was an issue?

But it seems worth trying.

I would like to see set_dev_pasid pass in the old domain first:

	int (*set_dev_pasid)(struct iommu_domain *new_domain,
                             struct iommu_domain *old_domain,
                             struct device *dev,
			     ioasid_t pasid);

Replace includes the old_domain as an argument and it is necessary
information..

A quick try on SMMUv3 seems reasonable:

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 9bc50bded5af72..f512bfe5cd202c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2931,13 +2931,12 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
        return ret;
 }
 
-static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
-                                     struct iommu_domain *domain)
+static void arm_smmu_blocking_set_dev_pasid(struct iommu_domain *new_domain,
+                                           struct iommu_domain *old_domain,
+                                           struct device *dev, ioasid_t pasid)
 {
        struct arm_smmu_master *master = dev_iommu_priv_get(dev);
-       struct arm_smmu_domain *smmu_domain;
-
-       smmu_domain = to_smmu_domain(domain);
+       struct arm_smmu_domain *smmu_domain = to_smmu_domain(old_domain);
 
        mutex_lock(&arm_smmu_asid_lock);
        arm_smmu_clear_cd(master, pasid);
@@ -3039,6 +3038,7 @@ static int arm_smmu_attach_dev_blocked(struct iommu_domain *domain,
 
 static const struct iommu_domain_ops arm_smmu_blocked_ops = {
        .attach_dev = arm_smmu_attach_dev_blocked,
+       .set_dev_pasid = arm_smmu_blocked_set_dev_pasid,
 };
 
 static struct iommu_domain arm_smmu_blocked_domain = {
@@ -3487,7 +3487,6 @@ static struct iommu_ops arm_smmu_ops = {
        .device_group           = arm_smmu_device_group,
        .of_xlate               = arm_smmu_of_xlate,
        .get_resv_regions       = arm_smmu_get_resv_regions,
-       .remove_dev_pasid       = arm_smmu_remove_dev_pasid,
        .dev_enable_feat        = arm_smmu_dev_enable_feature,
        .dev_disable_feat       = arm_smmu_dev_disable_feature,
        .page_response          = arm_smmu_page_response,

Jason