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

[Jason Gunthorpe]

On Mon, Feb 14, 2022 at 04:38:23PM +0000, Robin Murphy wrote:

> > 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.
> 
> That's certainly one of the cleaner possibilities - per the theme of this
> thread I'm not hugely keen on proliferating special VFIO-specific
> versions

IMHO this is still a net better than VFIO open coding buggy versions
as it has done.

> of IOMMU APIs, but trying to take the dev->mutex might be a bit heavy-handed
> and risky,

The container->group lock is held during this code, and the
container->group_lock is taken during probe under the
dev_mutex. Acquiring the dev_mutex inside the group_lock should not be
done.

> and getting at the vfio_group->device_lock a bit fiddly, so if I
> can't come up with anything nicer or more general it might be a fair
> compromise.

Actually that doesn't look so bad. A 'vfio allocate domain from group'
function that used the above trick looks OK to me right now.

If we could move the iommu_capable() test to a domain that would make
this pretty nice - getting the bus safely is a bit more of a PITA -
I'm much less keen on holding the device_lock for the whole function.

> > The remaining VFIO use of bus for iommu_capable() is better done
> > against the domain or the group object, as appropriate.
> 
> Indeed, although half the implementations of .capable are nonsense already,
> so I'm treating that one as a secondary priority for the moment (with an aim
> to come back afterwards and just try to kill it off as far as possible).
> RDMA and VFIO shouldn't be a serious concern for the kind of systems with
> heterogeneous IOMMUs at this point.

Well, lets see:

drivers/infiniband/hw/usnic/usnic_uiom.c:       if (!iommu_capable(dev->bus, IOMMU_CAP_CACHE_COHERENCY)) {
drivers/vhost/vdpa.c:   if (!iommu_capable(bus, IOMMU_CAP_CACHE_COHERENCY))

These are kind of hacky ways to say "userspace can actually do DMA
because we don't need privileged cache flush instructions on this
platform". I would love it if these could be moved to some struct
device API - I've aruged with Christoph a couple of times we need
something like that..

drivers/vfio/vfio_iommu_type1.c:        if (iommu_capable(bus, IOMMU_CAP_CACHE_COHERENCY))

This is doing the above, and also the no-snoop mess that Intel has
mixed in. How to exactly properly expose their special no-snoop
behavior is definitely something that should be on the domain.

drivers/pci/controller/vmd.c:   if (iommu_capable(vmd->dev->dev.bus, IOMMU_CAP_INTR_REMAP) ||
drivers/vfio/vfio_iommu_type1.c:                    iommu_capable(bus, IOMMU_CAP_INTR_REMAP);

Not sure about VMD, but the VFIO one is a security statement. It could
be quite happy as a domain query, or a flag 'require secure MSI
interrupts' as input to attach_domain.

> > > 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.
> 
> I though PPC wasn't using the IOMMU API at all... or is that the problem?

It needs it both ways, a way to get all the DMA security properties
from Lu's series without currently using an iommu_domain to get
them. So the design is to attach a NULL domain for PPC and leave it
like that.

It is surely eventually fixable to introduce a domain to PPC, I would
just prefer we not make anything contingent on actually doing that. :\

Jason