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.. > > @@ -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? __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. > 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. 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? > 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 Please give me your thought on external/internal as naming instead I can adjust the prior patch as well. Jason _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
