Re: [PATCH v2 3/3] iommufd/device: Change iommufd_hw_pagetable_has_group to device centric

[Nicolin Chen]

On Wed, Feb 01, 2023 at 06:59:21AM +0000, Tian, Kevin wrote:
> External email: Use caution opening links or attachments
> 
> 
> > From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
> > Sent: Wednesday, February 1, 2023 2:49 PM
> >
> > On 2023/2/1 11:07, Tian, Kevin wrote:
> > >> From: Nicolin Chen <nicolinc@xxxxxxxxxx>
> > >> Sent: Monday, January 30, 2023 6:22 PM
> > >>
> > >> On Sun, Jan 29, 2023 at 02:38:55AM -0800, Nicolin Chen wrote:
> > >>
> > >>>>> @@ -385,10 +372,8 @@ void iommufd_device_detach(struct
> > >>>>> iommufd_device *idev)
> > >>>>>        struct iommufd_hw_pagetable *hwpt = idev->hwpt;
> > >>>>>
> > >>>>>        mutex_lock(&hwpt->ioas->mutex);
> > >>>>> -     mutex_lock(&hwpt->devices_lock);
> > >>>>>        refcount_dec(hwpt->devices_users);
> > >>>>> -     list_del(&idev->devices_item);
> > >>>>> -     if (!iommufd_hw_pagetable_has_group(hwpt, idev->group)) {
> > >>>>> +     if (iommufd_hw_pagetable_has_device(hwpt, idev->dev)) {
> > >>>>>                if (refcount_read(hwpt->devices_users) == 1) {
> > >>>>>                        iopt_table_remove_domain(&hwpt->ioas->iopt,
> > >>>>>                                                 hwpt->domain);
> > >>>>> @@ -397,7 +382,6 @@ void iommufd_device_detach(struct
> > >> iommufd_device
> > >>>>> *idev)
> > >>>>>                iommu_detach_group(hwpt->domain, idev->group);
> > >>>>>        }
> > >>>>
> > >>>> emmm how do we track last device detach in a group? Here the first
> > >>>> device detach already leads to group detach...
> > >>>
> > >>> Oh no. That's a bug. Thanks for catching it.
> > >>>
> > >>> We need an additional refcount somewhere to track the number of
> > >>> attached devices in the iommu_group.
> > >>
> > >> Wondering if we can let iommu_attach/detach_device handle this:
> > >>
> > >
> > > that is the desired way to fully remove group awareness in iommufd.
> > >
> > > but iirc there were some concerns on changing their semantics. But
> > > I don't remember the detail now. Jason might know. also +Baolu/Robin.
> > >
> > > otherwise as long as the group attach/detach continues to be used
> > > then identifying last device in the group always needs some hack
> > > within iommufd itself.
> >
> > I have tried to solve this problem.
> >
> > https://lore.kernel.org/linux-iommu/20220106022053.2406748-1-
> > baolu.lu@xxxxxxxxxxxxxxx/
> >
> > I may need to review the original discussion to see if I can update a
> > new version.
> >
> 
> emm looks there are quite some discussions to catch up.
> 
> anyway assuming this will happen, Nicolin do we still want this
> preparatory series for coming nesting support?
>
> iiuc the main motivation was on the complexity of s2 attaching
> but with your discussion with Jason looks it's solvable. Then if this
> group hack can be separated from the nesting work it avoids
> unnecessary dependency in-between.

The device list is going to overcomplicate either the nesting
series or the replace series. And Jason asked me to send the
replace series prior to our nesting series.

Hoping that we can eliminate the dependencies between those two
series, I took these patches out and sent separately to clean
up a way. Yet, I was naive by thinking that I could do it with
a smaller pathset.

So, assuming we drop this series and move the first two patches
back to the nesting series or the replace series, one of them
would end up doing something ugly:

	if (cur_hwpt != hwpt)
		mutex_lock(&cur_hwpt->device_lock);
	mutex_lock(&hwpt->device_lock);
	...
	mutex_unlock(&hwpt->device_lock);
	if (cur_hwpt != hwpt)
		mutex_unlock(&cur_hwpt->device_lock);

So, perhaps we should discuss about which way we want to choose.

Btw, Baolu's version has a similar patch as mine changing the
iommu_attach/detach_device(), yet also touches _group(). Could
we bisect that series into _device() first and _group() later?
Given that we only need a device-centric API at this moment...

Thanks
Nic