Re: [PATCH v6 04/14] iommufd: Always pass iommu_attach_handle to iommu core

[Nicolin Chen <nicolinc@nvidia.com>]

On Fri, Dec 20, 2024 at 02:40:39PM +0800, Yi Liu wrote:
> > >   static inline void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt,
> > >   					      struct iommufd_device *idev)
> > >   {
> > > +	struct iommufd_attach_handle *handle;
> > > +
> > >   	if (hwpt->fault) {
> > >   		iommufd_fault_domain_detach_dev(hwpt, idev);
> > >   		return;
> > >   	}
> > > -	iommu_detach_group(hwpt->domain, idev->igroup->group);
> > > +	handle = iommufd_device_get_attach_handle(idev);
> > > +	iommu_detach_group_handle(hwpt->domain, idev->igroup->group);
> > > +	kfree(handle);
> > >   }
> > >   static inline int iommufd_hwpt_replace_device(struct iommufd_device *idev,
> > >   					      struct iommufd_hw_pagetable *hwpt,
> > >   					      struct iommufd_hw_pagetable *old)
> > >   {
> > > +	struct iommufd_attach_handle *curr;
> > > +	int ret;
> > > +
> > >   	if (old->fault || hwpt->fault)
> > >   		return iommufd_fault_domain_replace_dev(idev, hwpt, old);
> > > -	return iommu_group_replace_domain(idev->igroup->group, hwpt->domain);
> > > +	curr = iommufd_device_get_attach_handle(idev);
> > > +
> > > +	ret = iommufd_dev_replace_handle(idev, hwpt, old);
> > > +	if (ret)
> > > +		return ret;
> > 
> > These inline functions feel heavier after this rework..
> > 
> > I actually have the same patch for a different reason, yet haven't
> > sent yet:
> > https://github.com/nicolinc/iommufd/commits/wip/iommufd_msi_p1-v1
> >     iommu: Turn fault_data to iommufd private pointer
> >   =>iommufd: Make attach_handle generic
> > 
> > I think we can align with each other on these functions with one
> > common patch: my series requires the fault_data and attach_handle
> > are common to all HWPTs, v.s. exclusive to an hwpt->fault.
> 
> yeah, I'm ok to move the helpers to be in device.c to avoid inline.
> I've three remark on your patch [1]. I'm now also considering to do
> the work in one patch, hence it might be better for review w.r.t the
> comment from Baolu [2].

I replied: I moved attach_handle out of fault functions, leaving
them to only handle fault specific routines.

Yet, you might want to fix the iommufd_device_get_attach_handle()
in my patch to a normal function where NULL will be returned upon
an IS_ERR(attach_handle) v.s. just a to_iommufd_handle, which I
just realized a minute ago..

Thanks
Nic