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
