Re: [PATCH RFCv2 06/13] iommufd: Make attach_handle generic

[Nicolin Chen]

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