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

[Nicolin Chen <nicolinc@nvidia.com>]

On Fri, Jan 17, 2025 at 08:33:43PM +0800, Yi Liu wrote:
> On 2025/1/9 15:44, Tian, Kevin wrote:
> > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > Sent: Thursday, December 19, 2024 9:28 PM
> > > 
> > >   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;
> > > +
> > > +	kfree(curr);
> > > +	return 0;
> > >   }
> > > 
> > 
> > It's not balanced to have handle freed in different locations (one in
> > Fault domain specific handler and the other here).
> > 
> > It sounds clearer to remove the fault domain specific helper and have
> > everything together in this function when you move it to a C file.
> 
> sure. I'm also coordinating with Nic about this since he also needs to
> make the handle code to be generic in both fault and non-fault path.

Yea, it would look like the one in the MSI series [1]. A common
flow attaches/detaches/replaces the handles. If it's on a fault
patch, do the extra to enable/disable the fault specific stuff.

[1] https://lore.kernel.org/kvm/c708aedc678c63e2466b43ab9d4f8ac876e49aa1.1736550979.git.nicolinc@nvidia.com/