On 2022-05-06 14:21, Jason Gunthorpe wrote:
On Fri, May 06, 2022 at 10:12:29AM +0100, Robin Murphy wrote:
On 2022-05-05 17:15, Jason Gunthorpe via iommu wrote:
Call iommu_group_do_attach_device() only from
__iommu_group_attach_domain() which should be used to attach any domain to
the group.
The only unique thing __iommu_attach_group() does is to check if the group
is already attached to some caller specified group. Put this test into
__iommu_group_is_core_domain(), matching the
__iommu_group_attach_core_domain() nomenclature.
Change the two callers to directly call __iommu_group_attach_domain() and
test __iommu_group_is_core_domain().
iommu_attach_device() should trigger a WARN_ON if the group is attached as
the caller is using the function wrong.
Nit: if that's true then it's equally true for iommu_attach_group() as well.
Is it? I didn't check this as closely..
Well, there certainly seems no obvious reason for one to WARN where the
other fails quietly. Either it's an egregious loss of internal
consistency to not know whether your thing is already attached or it
isn't, but I don't see why the exact flavour of attach API called should
matter.
@@ -2110,6 +2100,12 @@ static int __iommu_group_attach_domain(struct
iommu_group *group,
return 0;
}
+static bool __iommu_group_is_core_domain(struct iommu_group *group)
I can see the thought process behind it, but once we've had some time away
from actively working on this area, this is clearly going to be a terrible
name.
We don't have a name for the set of domains owned by the core code vs a
domain set externally.
If you don't like core how about internal/external?
Oh no, I rather like the "core domain" nomenclature, it's the
"iommu_group_is_..." part that then fails to parse sensibly.
__iommu_group_set_internal_domain()
__iommu_group_internal_domain_attached() /
__iommu_group_external_domain_attached() ?
I wanted to use the word 'user' here (ie kernel user of the iommu
kAPI) for external domain but that felt confusing as well given we are
talking about introducing userspace domains for nesting.
Even getting past that, does it make sense to say NULL
is a core domain? I'm not convinced.
NULL is not an externally imposed domain so it is definately a
core/internal domain in this logic.
So if it *is* a domain then I can call NULL->attach_dev() and...? ;)
It's true that it's not a non-core domain, but IMO the two negatives
don't simply cancel out. Hence why I think it's easier to try framing it
in terms of what the *result* implies, rather than wrestle with trying
to concisely capture that the check itself is approximately
iommu_group_is_not_already_attached_to_user_allocated_domain().
For the sake of future readability, I'd
prefer to call this something more purpose-related like
__iommu_group_can_attach()
That is not the case - we can always attach any domain.
No, in the context of the iommu_attach_{device,group}() APIs where this
helper is relevant, after a successful attach, it has long been required
to detach before another attach can be performed. That's literally the
code you're moving around in this patch.
(Prior to default domains that wasn't actually enforced, but I doubt all
the drivers would have handled it correctly.)
This is precisely why I'm starting to think it would be beneficial to
differentiate the internal interface now that it's firmly moving away
from the attach/detach paradigm still exposed externally.
This is only used as a santity check to see if an externally imposed
domain is currently attached or not.
We could also just delete the check..
(and also just define it earlier to avoid the forward-declaration).
I put it here deliberately because the function directly below is
__iommu_group_attach_core_domain() - which causes
__iommu_group_is_core_domain() to become true. The symmetry helps
explain how the two functions relate.
This whole file is out of order, it seems to be a style or something?
FWIW I think the declarations we do have there are all things that got
refactored out of existing code. I don't see any having been added with
entirely new functions where easily avoidable.
In fact at that point I'd also be tempted to rename
__iommu_group_attach_domain() to __iommu_group_set_domain(), to further
clarify that attach/detach still reflects the external API, but the internal
mechanism is really a bit different since default/core domains came in.
It make sense - weird that set_domain is the only way to call
attach_dev, but OK :) I'll do that in the prior patch
Heh, if we can ever get to the point where we don't have differing old
and new semantics side-by-side at almost every level, maybe then we can
find the right name for *everything*. Besides, it's arguably even
weirder for attach_domain to be the only way to call detach_dev, so
there's that.
Please give me your thought on external/internal as naming instead I
can adjust the prior patch as well.
I certainly don't mind internal/external either, but as I say I'm also
equally happy with core/user, noting that the latter win on typing
effort too (and I'll continue to argue that there's not much appreciable
difference between kernel users and userspace users, since half the time
it's just going to be semantics of whether userspace has control of
mappings via IOMMUFD vs. some private driver interface).
Thanks,
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
