Re: [PATCH v1 1/8] iommu: Add iommu_group_replace_domain()

[Jason Gunthorpe]

On Mon, Feb 14, 2022 at 02:10:19PM +0000, Robin Murphy wrote:
> On 2022-02-14 12:45, Jason Gunthorpe wrote:
> > On Mon, Feb 14, 2022 at 12:09:36PM +0000, Robin Murphy wrote:
> > > On 2022-01-06 02:20, Lu Baolu wrote:
> > > > Expose an interface to replace the domain of an iommu group for frameworks
> > > > like vfio which claims the ownership of the whole iommu group.
> > > 
> > > But if the underlying point is the new expectation that
> > > iommu_{attach,detach}_device() operate on the device's whole group where
> > > relevant, why should we invent some special mechanism for VFIO to be
> > > needlessly inconsistent?
> > > 
> > > I said before that it's trivial for VFIO to resolve a suitable device if it
> > > needs to; by now I've actually written the patch ;)
> > > 
> > > https://gitlab.arm.com/linux-arm/linux-rm/-/commit/9f37d8c17c9b606abc96e1f1001c0b97c8b93ed5
> > 
> > Er, how does locking work there? What keeps busdev from being
> > concurrently unplugged?
> 
> Same thing that prevents the bus pointer from suddenly becoming invalid in
> the current code, I guess :)

Oooh, yes, that does look broken now too. :(

> > How can iommu_group_get() be safely called on
> > this pointer?
> 
> What matters is being able to call *other* device-based IOMMU API
> interfaces in the long term.

Yes, this is what I mean, those are the ones that call
iommu_group_get().

> > All of the above only works normally inside a probe/remove context
> > where the driver core is blocking concurrent unplug and descruction.
> > 
> > I think I said this last time you brought it up that lifetime was the
> > challenge with this idea.
> 
> Indeed, but it's a challenge that needs tackling, because the bus-based
> interfaces need to go away. So either we figure it out now and let this
> attach interface rework benefit immediately, or I spend three times as long

IMHO your path is easier if you let VFIO stay with the group interface
and use something like:

   domain = iommu_group_alloc_domain(group)

Which is what VFIO is trying to accomplish. Since Lu removed the only
other user of iommu_group_for_each_dev() it means we can de-export
that interface.

This works better because the iommu code can hold the internal group
while it finds the bus/device and then invokes the driver op. We don't
have a lifetime problem anymore under that lock.

The remaining VFIO use of bus for iommu_capable() is better done
against the domain or the group object, as appropriate.

In the bigger picture, VFIO should stop doing
'iommu_group_alloc_domain' by moving the domain alloc to
VFIO_GROUP_GET_DEVICE_FD where we have a struct device to use.

We've already been experimenting with this for iommufd and the subtle
difference in the uapi doesn't seem relevant.

> solving it on my own and end up deleting
> iommu_group_replace_domain() in about 6 months' time anyway.

I expect this API to remain until we figure out a solution to the PPC
problem, and come up with an alternative way to change the attached
domain on the fly.

Jason