Hi Robin and Jason,
On 12/23/21 8:57 AM, Jason Gunthorpe wrote:
On Wed, Dec 22, 2021 at 08:26:34PM +0000, Robin Murphy wrote:
On 21/12/2021 6:46 pm, Jason Gunthorpe wrote:
On Tue, Dec 21, 2021 at 04:50:56PM +0000, Robin Murphy wrote:
this proposal is the worst of both worlds, in that drivers still have to be
just as aware of groups in order to know whether to call the _shared
interface or not, except it's now entirely implicit and non-obvious.
Drivers are not aware of groups, where did you see that?
`git grep iommu_attach_group -- :^drivers/iommu :^include`
Did I really have to explain that?
Well, yes you did, because it shows you haven't understood my
question. After this series we deleted all those calls (though Lu, we
missed one of the tegra ones in staging, let's get it for the next
posting)
Yes, I will.
So, after this series, where do you see drivers being aware of groups?
If things are missed lets expect to fix them.
If the driver uses multiple struct devices and intends to connect them
all to the same domain then it uses the _shared variant. The only
difference between the two is the _shared varient lacks some of the
protections against driver abuse of the API.
You've lost me again; how are those intentions any different? Attaching one
device to a private domain is a literal subset of attaching more than one
device to a private domain.
Yes it is a subset, but drivers will malfunction if they are not
designed to have multi-attachment and wrongly get it, and there is
only one driver that does actually need this.
I maintain a big driver subsystem and have learned that grepability of
the driver mess for special cases is quite a good thing to
have. Forcing drivers to mark in code when they do something weird is
an advantage, even if it causes some small API redundancy.
However, if you really feel strongly this should really be one API
with the _shared implementation I won't argue it any further.
So then we have the iommu_attach_group() interface for new code (and still
nobody has got round to updating the old code to it yet), for which
the
This series is going in the direction of eliminating
iommu_attach_group() as part of the driver
interface. iommu_attach_group() is repurposed to only be useful for
VFIO.
We can also remove iommu_attach_group() in VFIO because it is
essentially equivalent to
iommu_group_for_each_dev(group, iommu_attach_device(dev))
properly, or iommu_attach_group() with a potentially better interface and
actual safety. The former is still more prevalent (and the interface
argument compelling), so if we put the new implementation behind that, with
the one tweak of having it set DMA_OWNER_PRIVATE_DOMAIN automatically, kill
off iommu_attach_group() by converting its couple of users,
This is what we did, iommu_attach_device() & _shared() are to be the
only interface for the drivers, and we killed off the
iommu_attach_group() couple of users except VFIO (the miss of
drivers/staging excepted)
and not only have we solved the VFIO problem but we've also finally
updated all the legacy code for free! Of course you can have a
separate version for VFIO to attach with
DMA_OWNER_PRIVATE_DOMAIN_USER if you like, although I still fail to
understand the necessity of the distinction.
And the seperate version for VFIO is called 'iommu_attach_group()'.
Lu, it is probably a good idea to add an assertion here that the group
is in DMA_OWNER_PRIVATE_DOMAIN_USER to make it clear that
iommu_attach_group() is only for VFIO.
VFIO has a special requirement that it be able to do:
+ ret = iommu_group_set_dma_owner(group->iommu_group,
+ DMA_OWNER_PRIVATE_DOMAIN_USER, f.file);
Without having a iommu_domain to attach.
This is because of the giant special case that PPC made of VFIO's
IOMMU code. PPC (aka vfio_iommu_spapr_tce.c) requires the group
isolation that iommu_group_set_dma_owner() provides, but does not
actually have an iommu_domain and can not/does not call
iommu_attach_group().
Fixing this is a whole other giant adventure I'm hoping David will
help me unwind next year..
This series solves this problem by using the two step sequence of
iommu_group_set_dma_owner()/iommu_attach_group() and conceptually
redefining how iommu_attach_group() works to require the external
caller to have done the iommu_group_set_dma_owner() for it. This is
why the series has three APIs, because the VFIO special one assumes
external iommu_group_set_dma_owner(). It just happens that is exactly
the same code as iommu_attach_group() today.
As for why does DMA_OWNER_PRIVATE_DOMAIN_USER exist? VFIO doesn't have
an iommu_domain at this point but it still needs the iommu core to
detatch the default domain. This is what the _USER does.
There is also a contract that after the USER ownership is claimed the
device could be accessed by userspace through the MMIO registers. So,
a device could be accessible by userspace before a user-space I/O
address is attached.
Soo..
There is another way to organize this and perhaps it does make more
sense. I will try to sketch briefly in email, try to imagine the
gaps..
API family (== compares to this series):
iommu_device_use_dma_api(dev);
== iommu_device_set_dma_owner(dev, DMA_OWNER_DMA_API, NULL);
iommu_group_set_dma_owner(group, file);
== iommu_device_set_dma_owner(dev, DMA_OWNER_PRIVATE_DOMAIN_USER,
file);
Always detaches all domains from the group
I hope we can drop all group variant APIs as we already have the per-
device interfaces, just iterate all device in the group and call the
device API.
iommu_attach_device(domain, dev)
== as is in this patch
dev and domain are 1:1
iommu_attach_device_shared(domain, dev)
== as is in this patch
dev and domain are N:1
* could just be the same as iommu_attach_device
iommu_replace_group_domain(group, old_domain, new_domain)
Makes group point at new_domain. new_domain can be NULL.
iommu_device_unuse_dma_api(dev)
== iommu_device_release_dma_owner() in this patch
iommu_group_release_dma_owner(group)
== iommu_detatch_group() && iommu_group_release_dma_owner()
VFIO would use the sequence:
iommu_group_set_dma_owner(group, file);
iommu_replace_group_domain(group, NULL, domain_1);
iommu_replace_group_domain(group, domain_1, domain_2);
iommu_group_release_dma_owner(group);
Simple devices would use
iommu_attach_device(domain, dev);
iommu_detatch_device(domain, dev);
Tegra would use:
iommu_attach_device_shared(domain, dev);
iommu_detatch_device_shared(domain, dev);
// Or not, if people agree we should not mark this
DMA API would have the driver core dma_configure do:
iommu_device_use_dma_api(dev);
dev->driver->probe()
iommu_device_unuse_dma_api(dev);
It is more APIs overall, but perhaps they have a much clearer
purpose.
I think it would be clear why iommu_group_set_dma_owner(), which
actually does detatch, is not the same thing as iommu_attach_device().
iommu_device_set_dma_owner() will eventually call
iommu_group_set_dma_owner(). I didn't get why
iommu_group_set_dma_owner() is special and need to keep.
I'm not sure if this entirely eliminates
DMA_OWNER_PRIVATE_DOMAIN_USER, or not, but at least it isn't in the
API.
Is it better?
Perhaps I missed anything. I have a simpler idea. We only need to have
below interfaces:
iommu_device_set_dma_owner(dev, owner);
iommu_device_release_dma_owner(dev, owner);
iommu_attach_device(domain, dev, owner);
iommu_detach_device(domain, dev);
All existing drivers calling iommu_attach_device() remain unchanged
since we already have singleton group enforcement. We only need to add
a default owner type.
For multiple-device group, like drm/tegra, the drivers should claim the
PRIVATE_DOMAIN ownership and call iommu_attach_device(domain, dev,
PRIVATE_DOMAIN) explicitly.
The new iommu_attach_device(domain, dev, owner) is a mix of the existing
iommu_attach_device() and the new iommu_attach_device_shared(). That
means,
if (group_is_singleton(group))
__iommu_atttach_device(domain, dev)
else
__iommu_attach_device_shared(domain, dev, owner)
The group variant interfaces will be deprecated and replace with the
device ones.
Sorry if I missed anything.
What VFIO wants is (conceptually[1]) "attach this device to my domain,
provided it and any other devices in its group are managed by a driver I
approve of."
Yes, sure, "conceptually". But, there are troublesome details.
VFIO will also need a struct device anyway, because once I get back from my
holiday in the new year I need to start working with Simon on evolving the
rest of the API away from bus->iommu_ops to dev->iommu so we can finally
support IOMMU drivers coexisting[2].
For VFIO it would be much easier to get the ops from the struct
iommu_group (eg via iommu_group->default_domain->ops, or whatever).
Indeed I agree with that second point, I'm just increasingly baffled how
it's not clear to you that there is only one fundamental use-case here.
Perhaps I'm too familiar with the history to objectively see how unclear the
current state of things might be :/
I think it is because you are just not familiar with the dark corners
of VFIO.
VFIO has a special case, I outlined above.
This is taking 426a to it's logical conclusion and *removing* the
group API from the drivers entirely. This is desirable because drivers
cannot do anything sane with the group.
I am in complete agreement with that (to the point of also not liking patch
#6).
Unfortunately patch #6 is only because of VFIO needing to use the
group as a handle.
Jason
Best regards,
baolu
