On Wed, Jan 29, 2025 at 02:14:47PM +0100, Eric Auger wrote:
> > @@ -352,6 +352,111 @@ iommufd_device_attach_reserved_iova(struct
> > iommufd_device *idev,
> > return 0;
> > }
> >
> > +/* The device attach/detach/replace helpers for attach_handle */
> > +
> > +static int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
> > + struct iommufd_device *idev)
> > +{
> > + struct iommufd_attach_handle *handle;
> > + int rc;
> > +
> > + if (hwpt->fault) {
> > + rc = iommufd_fault_domain_attach_dev(hwpt, idev, true);
> why don't we simply call iommufd_fault_iopf_enable(idev)
> also it looks there is a redundant check of hwpt_fault here and in
>
> iommufd_fault_domain_attach_dev
>
> Besides the addition of enable_iopf param is not documented anywhere
OK. I will try unwrapping that.
> > +static void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt,
> > + struct iommufd_device *idev)
> > +{
> > + struct iommufd_attach_handle *handle;
> > +
> > + handle = iommufd_device_get_attach_handle(idev);
> > + iommu_detach_group_handle(hwpt->domain, idev->igroup->group);
> > + if (hwpt->fault)
> > + iommufd_fault_domain_detach_dev(hwpt, idev, handle, true);
> same here, pretty difficult to understand what this
>
> iommufd_fault_domain_detach_dev does
> To me calling iommufd_auto_response_faults and iommufd_fault_iopf_disable
> would be more readable or rename iommufd_fault_domain_detach_dev().
> Also compared to the original code,
This is basically a cleanup call for the fault specific items as
the patch's commit message describes. And you read it correct..
I will see what I can do with the naming.
> there is a new check on handle. Why is it necessary.
It was to avoid the error path that has a handle=NULL entering the
auto response function. We can change that a bit to drop the check
to make it slightly clearer, though it would waste some extra CPU
cycles on scanning the two fault lists against an empty handle.
> Globally I feel that patch pretty hard to read. Would be nice to split if
> possible to ease the review process.
This patch is needed by both this series and Yi's PASID series too,
so I was planning to send it individually. I will see what I can do
to make it easy to read.
Thanks
Nicolin
