Re: [PATCH v2 10/17] iommufd: Reorganize iommufd_device_attach into iommufd_device_change_pt

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Mar 10, 2023 at 11:26:42AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Wednesday, March 8, 2023 8:36 AM
> > 
> > @@ -379,52 +388,57 @@ static int
> > iommufd_device_auto_get_domain(struct iommufd_device *idev,
> > 
> >  		if (!iommufd_lock_obj(&hwpt->obj))
> >  			continue;
> > -		rc = iommufd_device_do_attach(idev, hwpt);
> > -		iommufd_put_object(&hwpt->obj);
> > -
> > -		/*
> > -		 * -EINVAL means the domain is incompatible with the device.
> > -		 * Other error codes should propagate to userspace as failure.
> > -		 * Success means the domain is attached.
> > -		 */
> > -		if (rc == -EINVAL)
> > -			continue;
> > +		destroy_hwpt = (*do_attach)(idev, hwpt);
> >  		*pt_id = hwpt->obj.id;
> 
> only when succeed?

It isn't necessary, but it can be, it is just more ugly

@@ -461,9 +468,8 @@ iommufd_device_auto_get_domain(struct iommufd_device *idev,
                if (!iommufd_lock_obj(&hwpt->obj))
                        continue;
                destroy_hwpt = (*do_attach)(idev, hwpt);
-               *pt_id = hwpt->obj.id;
-               iommufd_put_object(&hwpt->obj);
                if (IS_ERR(destroy_hwpt)) {
+                       iommufd_put_object(&hwpt->obj);
                        /*
                         * -EINVAL means the domain is incompatible with the
                         * device. Other error codes should propagate to
@@ -474,6 +480,8 @@ iommufd_device_auto_get_domain(struct iommufd_device *idev,
                                continue;
                        goto out_unlock;
                }
+               *pt_id = hwpt->obj.id;
+               iommufd_put_object(&hwpt->obj);
                goto out_unlock;
        }

but sure lets do it

> > +		if (IS_ERR(destroy_hwpt)) {
> > +			/*
> > +			 * -EINVAL means the domain is incompatible with
> > the
> > +			 * device. Other error codes should propagate to
> > +			 * userspace as failure. Success means the domain is
> > +			 * attached.
> > +			 */
> > +			if (PTR_ERR(destroy_hwpt) == -EINVAL)
> > +				continue;
> > +			goto out_unlock;
> > +		}
> >  		goto out_unlock;
> 
> two goto's can be merged, if you still want to keep pt_id assignment
> in original place.

Ah, I don't like that so much stylistically. 
 
> >  	}
> > 
> > -	hwpt = iommufd_hw_pagetable_alloc(idev->ictx, ioas, idev, true);
> > +	hwpt = iommufd_hw_pagetable_alloc(idev->ictx, ioas, idev,
> > +					  immediate_attach);
> >  	if (IS_ERR(hwpt)) {
> > -		rc = PTR_ERR(hwpt);
> > +		destroy_hwpt = ERR_CAST(hwpt);
> >  		goto out_unlock;
> >  	}
> > +
> > +	if (!immediate_attach) {
> > +		destroy_hwpt = (*do_attach)(idev, hwpt);
> > +		if (IS_ERR(destroy_hwpt))
> > +			goto out_abort;
> > +	} else {
> > +		destroy_hwpt = NULL;
> > +	}
> > +
> 
> Above is a bit confusing.
> 
> On one hand we have immediate_attach for drivers which must
> complete attach before we can add the domain to iopt. From
> this angle it should always be set when calling
> iommufd_hw_pagetable_alloc() no matter it's attach or replace.

I looked at it for a while if we could make replace follow the same
immediate_attach flow, and it doesn't work right. The problem is we
can fail at iopt_table_add_domain() which would be after replace is
done and at that point we are pretty stuck.

The design of replace is that iommu_group_replace_domain() is the last
failable function in the process.

> On the other hand we assume *replace* doesn't work with
> driver which requires immediate_attach so it's done outside of
> iommufd_hw_pagetable_alloc().

Replace with an IOAS doesn't work on those drivers. It works OK with a
HWPT.

> I'm unsure any better way of handling this transition phase, but
> at least some comment would be useful in this part.

	/*
	 * iommufd_hw_pagetable_attach() is called by
	 * iommufd_hw_pagetable_alloc() in immediate attachment mode, same as
	 * iommufd_device_do_attach(). So if we are in this mode then we prefer
	 * to use the immediate_attach path as it supports drivers that can't
	 * directly allocate a domain.
	 */
	bool immediate_attach = do_attach == iommufd_device_do_attach;

Jason