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

[Tian, Kevin]

> 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.