Re: [PATCH] iommu: Reorganize __iommu_attach_group()

[Robin Murphy <robin.murphy@arm.com>]

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