On Fri, Mar 10, 2023 at 11:26:42AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > 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
