[PATCH] iommu: Reorganize __iommu_attach_group()

[PATCH] iommu: Reorganize __iommu_attach_group()

[Jason Gunthorpe via iommu]

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.

Suggested-by: "Tian, Kevin" <kevin.tian@intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 42 +++++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

This goes after "iommu: iommu_group_claim_dma_owner() must always assign a
domain" and simplifies some of the remaining duplication.

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index c1bdec807ea381..09d00be887f924 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -81,9 +81,10 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
 						 unsigned type);
 static int __iommu_attach_device(struct iommu_domain *domain,
 				 struct device *dev);
-static int __iommu_attach_group(struct iommu_domain *domain,
-				struct iommu_group *group);
+static int __iommu_group_attach_domain(struct iommu_group *group,
+				       struct iommu_domain *new_domain);
 static void __iommu_group_attach_core_domain(struct iommu_group *group);
+static bool __iommu_group_is_core_domain(struct iommu_group *group);
 static int iommu_create_device_direct_mappings(struct iommu_group *group,
 					       struct device *dev);
 static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
@@ -1938,10 +1939,11 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
 	 */
 	mutex_lock(&group->mutex);
 	ret = -EINVAL;
-	if (iommu_group_device_count(group) != 1)
+	if (iommu_group_device_count(group) != 1 ||
+	    WARN_ON(!__iommu_group_is_core_domain(group)))
 		goto out_unlock;
 
-	ret = __iommu_attach_group(domain, group);
+	ret = __iommu_group_attach_domain(group, domain);
 
 out_unlock:
 	mutex_unlock(&group->mutex);
@@ -2032,31 +2034,19 @@ static int iommu_group_do_attach_device(struct device *dev, void *data)
 	return __iommu_attach_device(domain, dev);
 }
 
-static int __iommu_attach_group(struct iommu_domain *domain,
-				struct iommu_group *group)
-{
-	int ret;
-
-	if (group->domain && group->domain != group->default_domain &&
-	    group->domain != group->blocking_domain)
-		return -EBUSY;
-
-	ret = __iommu_group_for_each_dev(group, domain,
-					 iommu_group_do_attach_device);
-	if (ret == 0)
-		group->domain = domain;
-
-	return ret;
-}
-
 int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
 {
 	int ret;
 
 	mutex_lock(&group->mutex);
-	ret = __iommu_attach_group(domain, group);
-	mutex_unlock(&group->mutex);
+	if (!__iommu_group_is_core_domain(group)) {
+		ret = -EBUSY;
+		goto out_unlock;
+	}
 
+	ret = __iommu_group_attach_domain(group, domain);
+out_unlock:
+	mutex_unlock(&group->mutex);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(iommu_attach_group);
@@ -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)
+{
+	return !group->domain || group->domain == group->default_domain ||
+	       group->domain == group->blocking_domain;
+}
+
 /*
  * Put the group's domain back to the appropriate core-owned domain - either the
  * standard kernel-mode DMA configuration or an all-DMA-blocked domain.

base-commit: f9169ee95787fe691287eeed1a1c269ea72c8fb4
-- 
2.36.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH] iommu: Reorganize __iommu_attach_group()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, May 6, 2022 12:16 AM
> 
> 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.
> 
> Suggested-by: "Tian, Kevin" <kevin.tian@intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> ---
>  drivers/iommu/iommu.c | 42 +++++++++++++++++++-----------------------
>  1 file changed, 19 insertions(+), 23 deletions(-)
> 
> This goes after "iommu: iommu_group_claim_dma_owner() must always
> assign a
> domain" and simplifies some of the remaining duplication.
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index c1bdec807ea381..09d00be887f924 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -81,9 +81,10 @@ static struct iommu_domain
> *__iommu_domain_alloc(struct bus_type *bus,
>  						 unsigned type);
>  static int __iommu_attach_device(struct iommu_domain *domain,
>  				 struct device *dev);
> -static int __iommu_attach_group(struct iommu_domain *domain,
> -				struct iommu_group *group);
> +static int __iommu_group_attach_domain(struct iommu_group *group,
> +				       struct iommu_domain *new_domain);
>  static void __iommu_group_attach_core_domain(struct iommu_group
> *group);
> +static bool __iommu_group_is_core_domain(struct iommu_group *group);
>  static int iommu_create_device_direct_mappings(struct iommu_group
> *group,
>  					       struct device *dev);
>  static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
> @@ -1938,10 +1939,11 @@ int iommu_attach_device(struct iommu_domain
> *domain, struct device *dev)
>  	 */
>  	mutex_lock(&group->mutex);
>  	ret = -EINVAL;
> -	if (iommu_group_device_count(group) != 1)
> +	if (iommu_group_device_count(group) != 1 ||
> +	    WARN_ON(!__iommu_group_is_core_domain(group)))
>  		goto out_unlock;
> 
> -	ret = __iommu_attach_group(domain, group);
> +	ret = __iommu_group_attach_domain(group, domain);
> 
>  out_unlock:
>  	mutex_unlock(&group->mutex);
> @@ -2032,31 +2034,19 @@ static int iommu_group_do_attach_device(struct
> device *dev, void *data)
>  	return __iommu_attach_device(domain, dev);
>  }
> 
> -static int __iommu_attach_group(struct iommu_domain *domain,
> -				struct iommu_group *group)
> -{
> -	int ret;
> -
> -	if (group->domain && group->domain != group->default_domain &&
> -	    group->domain != group->blocking_domain)
> -		return -EBUSY;
> -
> -	ret = __iommu_group_for_each_dev(group, domain,
> -					 iommu_group_do_attach_device);
> -	if (ret == 0)
> -		group->domain = domain;
> -
> -	return ret;
> -}
> -
>  int iommu_attach_group(struct iommu_domain *domain, struct
> iommu_group *group)
>  {
>  	int ret;
> 
>  	mutex_lock(&group->mutex);
> -	ret = __iommu_attach_group(domain, group);
> -	mutex_unlock(&group->mutex);
> +	if (!__iommu_group_is_core_domain(group)) {
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> 
> +	ret = __iommu_group_attach_domain(group, domain);
> +out_unlock:
> +	mutex_unlock(&group->mutex);
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(iommu_attach_group);
> @@ -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)
> +{
> +	return !group->domain || group->domain == group-
> >default_domain ||
> +	       group->domain == group->blocking_domain;
> +}
> +
>  /*
>   * Put the group's domain back to the appropriate core-owned domain -
> either the
>   * standard kernel-mode DMA configuration or an all-DMA-blocked domain.
> 
> base-commit: f9169ee95787fe691287eeed1a1c269ea72c8fb4
> --
> 2.36.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu: Reorganize __iommu_attach_group()

[Robin Murphy]

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.

> Suggested-by: "Tian, Kevin" <kevin.tian@intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/iommu.c | 42 +++++++++++++++++++-----------------------
>   1 file changed, 19 insertions(+), 23 deletions(-)
> 
> This goes after "iommu: iommu_group_claim_dma_owner() must always assign a
> domain" and simplifies some of the remaining duplication.
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index c1bdec807ea381..09d00be887f924 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -81,9 +81,10 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
>   						 unsigned type);
>   static int __iommu_attach_device(struct iommu_domain *domain,
>   				 struct device *dev);
> -static int __iommu_attach_group(struct iommu_domain *domain,
> -				struct iommu_group *group);
> +static int __iommu_group_attach_domain(struct iommu_group *group,
> +				       struct iommu_domain *new_domain);
>   static void __iommu_group_attach_core_domain(struct iommu_group *group);
> +static bool __iommu_group_is_core_domain(struct iommu_group *group);
>   static int iommu_create_device_direct_mappings(struct iommu_group *group,
>   					       struct device *dev);
>   static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
> @@ -1938,10 +1939,11 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
>   	 */
>   	mutex_lock(&group->mutex);
>   	ret = -EINVAL;
> -	if (iommu_group_device_count(group) != 1)
> +	if (iommu_group_device_count(group) != 1 ||
> +	    WARN_ON(!__iommu_group_is_core_domain(group)))
>   		goto out_unlock;
>   
> -	ret = __iommu_attach_group(domain, group);
> +	ret = __iommu_group_attach_domain(group, domain);
>   
>   out_unlock:
>   	mutex_unlock(&group->mutex);
> @@ -2032,31 +2034,19 @@ static int iommu_group_do_attach_device(struct device *dev, void *data)
>   	return __iommu_attach_device(domain, dev);
>   }
>   
> -static int __iommu_attach_group(struct iommu_domain *domain,
> -				struct iommu_group *group)
> -{
> -	int ret;
> -
> -	if (group->domain && group->domain != group->default_domain &&
> -	    group->domain != group->blocking_domain)
> -		return -EBUSY;
> -
> -	ret = __iommu_group_for_each_dev(group, domain,
> -					 iommu_group_do_attach_device);
> -	if (ret == 0)
> -		group->domain = domain;
> -
> -	return ret;
> -}
> -
>   int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
>   {
>   	int ret;
>   
>   	mutex_lock(&group->mutex);
> -	ret = __iommu_attach_group(domain, group);
> -	mutex_unlock(&group->mutex);
> +	if (!__iommu_group_is_core_domain(group)) {
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
>   
> +	ret = __iommu_group_attach_domain(group, domain);
> +out_unlock:
> +	mutex_unlock(&group->mutex);
>   	return ret;
>   }
>   EXPORT_SYMBOL_GPL(iommu_attach_group);
> @@ -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. "Is this group a core domain? Er, no, it's a group; what 
an odd question to ask :/" Even getting past that, does it make sense to 
say NULL is a core domain? I'm not convinced. For the sake of future 
readability, I'd prefer to call this something more purpose-related like 
__iommu_group_can_attach() (and also just define it earlier to avoid the 
forward-declaration).

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.

Thanks,
Robin.

> +{
> +	return !group->domain || group->domain == group->default_domain ||
> +	       group->domain == group->blocking_domain;
> +}
> +
>   /*
>    * Put the group's domain back to the appropriate core-owned domain - either the
>    * standard kernel-mode DMA configuration or an all-DMA-blocked domain.
> 
> base-commit: f9169ee95787fe691287eeed1a1c269ea72c8fb4
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu: Reorganize __iommu_attach_group()

[Jason Gunthorpe via iommu]

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
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu: Reorganize __iommu_attach_group()

[Robin Murphy]

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

Re: [PATCH] iommu: Reorganize __iommu_attach_group()

[Jason Gunthorpe via iommu]

On Fri, May 06, 2022 at 05:44:11PM +0100, Robin Murphy wrote:

> > > 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.

I will try to check

> > 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.

Oh Ok

> > > 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...? ;)

You can call iommu_group_set_domain(group, NULL) and it will work.

As I said, it must have this symmetry:

 __iommu_group_attach_core_domain()
 assert(__iommu_group_is_core_domain())

This is why I choose the name, because it is a clear pairing with
__iommu_group_attach_core_domain().

How about __iommu_group_is_core_domain_attached() ? Solves the grammer
issue

> > > 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.


> 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.

I'm viewing the API family of __iommu_group_attach_core_domain /
__iommu_group_set_domain / __iommu_group_is_core_domain_attached()
a layering point - the other APIs are being built on top of this
family.

It is now exposing a semantic the same as the default-domain enabled
driver ops, but is using different nomenclature.

Should it be __iommu_group_set_core_domain() ?

This naming seems way harder than it really should be.

> 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.

It is because the ops are misnamed for what they now do.

 attach_dev is really set_domain_for_dev()
 detach_dev is really reset_dev_to_platform()

Which definitely is the root cause of the confusion that created this
bug in the first place.

Anyhow, I'll post the fixing patch again with the new comment and we
can decide how to revise the language separately, no need to delay
getting Lu's series back into the testing stream.

Thanks,
Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH] iommu: Reorganize __iommu_attach_group()

[Tian, Kevin]

> From: Jason Gunthorpe
> Sent: Saturday, May 7, 2022 1:55 AM
> 
> On Fri, May 06, 2022 at 05:44:11PM +0100, Robin Murphy wrote:
> >
> > So if it *is* a domain then I can call NULL->attach_dev() and...? ;)
> 
> You can call iommu_group_set_domain(group, NULL) and it will work.
> 
> As I said, it must have this symmetry:
> 
>  __iommu_group_attach_core_domain()
>  assert(__iommu_group_is_core_domain())
> 
> This is why I choose the name, because it is a clear pairing with
> __iommu_group_attach_core_domain().
> 
> How about __iommu_group_is_core_domain_attached() ? Solves the
> grammer
> issue

Or just __iommu_group_is_core_managed() to avoid the confusion
on whether NULL domain is considered as 'attached'? 'core managed'
can cover NULL domain as a policy in iommu core.

Alternatively we can also keep current name but moving the NULL domain
check out, i.e.:

assert(!group->domain || __iommu_group_is_core_domain(group));

This actually better pairs with __iommu_group_attach_core_domain()
as the latter is clearly defined for non-NULL domains:

+/*
+ * Put the group's domain back to the appropriate core-owned domain - either the
+ * standard kernel-mode DMA configuration or an all-DMA-blocked domain.
+ */
+static void __iommu_group_set_core_domain(struct iommu_group *group)

Thanks
Kevin
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu