On 2022-04-08 13:18, Jason Gunthorpe wrote:
On Thu, Apr 07, 2022 at 08:27:05PM +0100, Robin Murphy wrote:
On 2022-04-07 20:08, Jason Gunthorpe wrote:
On Thu, Apr 07, 2022 at 07:02:03PM +0100, Robin Murphy wrote:
On 2022-04-07 18:43, Jason Gunthorpe wrote:
On Thu, Apr 07, 2022 at 06:03:37PM +0100, Robin Murphy wrote:
At a glance, this all looks about the right shape to me now, thanks!
Thanks!
Ideally I'd hope patch #4 could go straight to device_iommu_capable() from
my Thunderbolt series, but we can figure that out in a couple of weeks once
Yes, this does helps that because now the only iommu_capable call is
in a context where a device is available :)
Derp, of course I have *two* VFIO patches waiting, the other one touching
the iommu_capable() calls (there's still IOMMU_CAP_INTR_REMAP, which, much
as I hate it and would love to boot all that stuff over to
drivers/irqchip,
Oh me too...
it's not in my way so I'm leaving it be for now). I'll have to rebase that
anyway, so merging this as-is is absolutely fine!
This might help your effort - after this series and this below there
are no 'bus' users of iommu_capable left at all.
Thanks, but I still need a device for the iommu_domain_alloc() as well, so
at that point the interrupt check is OK to stay where it is.
It is a simple enough change that could avoid introducing the
device_iommu_capable() at all perhaps.
I figured out a locking strategy per my original idea that seems
pretty clean, it just needs vfio_group_viable() to go away first:
I think this should be more like:
struct vfio_device *vdev;
mutex_lock(&group->device_lock);
vdev = list_first_entry(group->device_list, struct vfio_device, group_next);
ret = driver->ops->attach_group(data, group->iommu_group,
group->type,
vdev->dev);
mutex_unlock(&group->device_lock);
Then don't do the iommu_group_for_each_dev() at all.
The problem with iommu_group_for_each_dev() is that it may select a
struct device that does not have a vfio_device bound to it, so we
would be using a random struct device that is not protected by any
VFIO device_driver.
Yeah, I was just looking at the final puzzle piece of making sure to nab
the actual VFIO device rather than some unbound device that's just along
for the ride... If I can't come up with anything more self-contained
I'll take this suggestion, thanks.
However, this creates an oddball situation where the vfio_device and
it's struct device could become unplugged from the system while the
domain that the struct device spawned continues to exist and remains
attached to other devices in the same group. ie the iommu driver has
to be careful not to retain the struct device input..
Oh, I rather assumed that VFIO might automatically tear down the
container/domain when the last real user disappears. I don't see there
being an obvious problem if the domain does hang around with residual
non-VFIO devices attached until userspace explicitly closes all the
relevant handles, as long as we take care not to release DMA ownership
until that point also. As you say, it just looks a bit funny.
I suppose that is inevitable to have sharing of domains across
devices, so the iommu drivers will have to accommodate this.
I think domain lifecycle management is already entirely up to the users
and not something that IOMMU drivers need to worry about. Drivers should
only need to look at per-device data in attach/detach (and, once I've
finished, alloc) from the device argument which can be assumed to be
valid at that point. Otherwise, all the relevant internal data for
domain ops should belong to the domain already.
Cheers,
Robin.
