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);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 withthe
* 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;}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);
rc = PTR_ERR(hwpt);
destroy_hwpt = ERR_CAST(hwpt);- 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