[v1,2/8] iommu: Introduce a new iommu_group_replace_domain() API

  qemu has a need to replace the translations associated with a domain
when the guest does large-scale operations like switching between an
IDENTITY domain and, say, dma-iommu.c.

Currently, it does this by replacing all the mappings in a single
domain, but this is very inefficient and means that domains have to be
per-device rather than per-translation.

Provide a high-level API to allow replacements of one domain with
another. This is similar to a detach/attach cycle except it doesn't
force the group to go to the blocking domain in-between.

By removing this forced blocking domain the iommu driver has the
opportunity to implement an atomic replacement of the domains to the
greatest extent its hardware allows.

Atomic replacement allows the qemu emulation of the viommu to be more
complete, as real hardware has this ability.

All drivers are already required to support changing between active
UNMANAGED domains when using their attach_dev ops.

This API is expected to be used by IOMMUFD, so add to the iommu-priv
header and mark it as IOMMUFD_INTERNAL.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommu-priv.h |  4 ++++
 drivers/iommu/iommu.c      | 30 ++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)
  

On 2023/2/2 15:05, Nicolin Chen wrote:
> +/**
> + * iommu_group_replace_domain - replace the domain that a group is attached to
> + * @new_domain: new IOMMU domain to replace with
> + * @group: IOMMU group that will be attached to the new domain
> + *
> + * This API allows the group to switch domains without being forced to go to
> + * the blocking domain in-between.
> + *
> + * If the attached domain is a core domain (e.g. a default_domain), it will act
> + * just like the iommu_attach_group().

I am not following above two lines. Why and how could iommufd set a
core domain to an iommu_group?

> + */
> +int iommu_group_replace_domain(struct iommu_group *group,
> +			       struct iommu_domain *new_domain)
> +{
> +	int ret;
> +
> +	if (!new_domain)
> +		return -EINVAL;
> +
> +	mutex_lock(&group->mutex);
> +	ret = __iommu_group_set_domain(group, new_domain);
> +	if (ret) {
> +		if (__iommu_group_set_domain(group, group->domain))
> +			__iommu_group_set_core_domain(group);
> +	}
> +	mutex_unlock(&group->mutex);
> +	return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(iommu_group_replace_domain, IOMMUFD_INTERNAL);

Best regards,
baolu
  

On Thu, Feb 02, 2023 at 06:21:20PM +0800, Baolu Lu wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 2023/2/2 15:05, Nicolin Chen wrote:
> > +/**
> > + * iommu_group_replace_domain - replace the domain that a group is attached to
> > + * @new_domain: new IOMMU domain to replace with
> > + * @group: IOMMU group that will be attached to the new domain
> > + *
> > + * This API allows the group to switch domains without being forced to go to
> > + * the blocking domain in-between.
> > + *
> > + * If the attached domain is a core domain (e.g. a default_domain), it will act
> > + * just like the iommu_attach_group().
> 
> I am not following above two lines. Why and how could iommufd set a
> core domain to an iommu_group?

Perhaps this isn't the best narrative. What it's supposed to say
is that this function acts as an iommu_attach_group() call if the
device is "detached", yet we have changed the semantics about the
word "detach". So, what should the correct way to write such a
note?

Thanks
Nic
  

On 2023/2/3 3:14, Nicolin Chen wrote:
> On Thu, Feb 02, 2023 at 06:21:20PM +0800, Baolu Lu wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 2023/2/2 15:05, Nicolin Chen wrote:
>>> +/**
>>> + * iommu_group_replace_domain - replace the domain that a group is attached to
>>> + * @new_domain: new IOMMU domain to replace with
>>> + * @group: IOMMU group that will be attached to the new domain
>>> + *
>>> + * This API allows the group to switch domains without being forced to go to
>>> + * the blocking domain in-between.
>>> + *
>>> + * If the attached domain is a core domain (e.g. a default_domain), it will act
>>> + * just like the iommu_attach_group().
>> I am not following above two lines. Why and how could iommufd set a
>> core domain to an iommu_group?
> Perhaps this isn't the best narrative. What it's supposed to say
> is that this function acts as an iommu_attach_group() call if the
> device is "detached", yet we have changed the semantics about the
> word "detach". So, what should the correct way to write such a
> note?

How could this interface be used as detaching a domain from a group?
Even it could be used, doesn't it act as an iommu_detach_group()?

Best regards,
baolu
  

On Fri, Feb 03, 2023 at 09:33:44AM +0800, Baolu Lu wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 2023/2/3 3:14, Nicolin Chen wrote:
> > On Thu, Feb 02, 2023 at 06:21:20PM +0800, Baolu Lu wrote:
> > > External email: Use caution opening links or attachments
> > > 
> > > 
> > > On 2023/2/2 15:05, Nicolin Chen wrote:
> > > > +/**
> > > > + * iommu_group_replace_domain - replace the domain that a group is attached to
> > > > + * @new_domain: new IOMMU domain to replace with
> > > > + * @group: IOMMU group that will be attached to the new domain
> > > > + *
> > > > + * This API allows the group to switch domains without being forced to go to
> > > > + * the blocking domain in-between.
> > > > + *
> > > > + * If the attached domain is a core domain (e.g. a default_domain), it will act
> > > > + * just like the iommu_attach_group().
> > > I am not following above two lines. Why and how could iommufd set a
> > > core domain to an iommu_group?
> > Perhaps this isn't the best narrative. What it's supposed to say
> > is that this function acts as an iommu_attach_group() call if the
> > device is "detached", yet we have changed the semantics about the
> > word "detach". So, what should the correct way to write such a
> > note?
> 
> How could this interface be used as detaching a domain from a group?
> Even it could be used, doesn't it act as an iommu_detach_group()?

No. I didn't say that. It doesn't act as detach(), but attach()
when a device is already "detached".

The original statement is saying, "if the attached domain is a
core domain", i.e. the device is detach()-ed, "it will act just
like the iommu_attach_group()".

Thanks
Nic
  

On 2023/2/3 9:41, Nicolin Chen wrote:
> On Fri, Feb 03, 2023 at 09:33:44AM +0800, Baolu Lu wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 2023/2/3 3:14, Nicolin Chen wrote:
>>> On Thu, Feb 02, 2023 at 06:21:20PM +0800, Baolu Lu wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 2023/2/2 15:05, Nicolin Chen wrote:
>>>>> +/**
>>>>> + * iommu_group_replace_domain - replace the domain that a group is attached to
>>>>> + * @new_domain: new IOMMU domain to replace with
>>>>> + * @group: IOMMU group that will be attached to the new domain
>>>>> + *
>>>>> + * This API allows the group to switch domains without being forced to go to
>>>>> + * the blocking domain in-between.
>>>>> + *
>>>>> + * If the attached domain is a core domain (e.g. a default_domain), it will act
>>>>> + * just like the iommu_attach_group().
>>>> I am not following above two lines. Why and how could iommufd set a
>>>> core domain to an iommu_group?
>>> Perhaps this isn't the best narrative. What it's supposed to say
>>> is that this function acts as an iommu_attach_group() call if the
>>> device is "detached", yet we have changed the semantics about the
>>> word "detach". So, what should the correct way to write such a
>>> note?
>> How could this interface be used as detaching a domain from a group?
>> Even it could be used, doesn't it act as an iommu_detach_group()?
> No. I didn't say that. It doesn't act as detach(), but attach()
> when a device is already "detached".
> 
> The original statement is saying, "if the attached domain is a
> core domain", i.e. the device is detach()-ed, "it will act just
> like the iommu_attach_group()".

Oh! My bad. I misunderstood it. Sorry for the noise. :-)

Best regards,
baolu
  

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, February 2, 2023 3:05 PM
> 
> All drivers are already required to support changing between active
> UNMANAGED domains when using their attach_dev ops.

All drivers which don't have *broken* UNMANAGED domain?

> 
> +/**
> + * iommu_group_replace_domain - replace the domain that a group is
> attached to
> + * @new_domain: new IOMMU domain to replace with
> + * @group: IOMMU group that will be attached to the new domain
> + *
> + * This API allows the group to switch domains without being forced to go to
> + * the blocking domain in-between.
> + *
> + * If the attached domain is a core domain (e.g. a default_domain), it will act
> + * just like the iommu_attach_group().

I think you meant "the currently-attached domain", which implies a
'detached' state as you replied to Baolu.

> + */
> +int iommu_group_replace_domain(struct iommu_group *group,
> +			       struct iommu_domain *new_domain)

what actual value does 'replace' give us? It's just a wrapper of
__iommu_group_set_domain() then calling it set_domain is
probably clearer. You can clarify the 'replace' behavior in the
comment.

> +{
> +	int ret;
> +
> +	if (!new_domain)
> +		return -EINVAL;
> +
> +	mutex_lock(&group->mutex);
> +	ret = __iommu_group_set_domain(group, new_domain);
> +	if (ret) {
> +		if (__iommu_group_set_domain(group, group->domain))
> +			__iommu_group_set_core_domain(group);
> +	}

Can you elaborate the error handling here? Ideally if
__iommu_group_set_domain() fails then group->domain shouldn't
be changed. Why do we need further housekeeping here?
  

On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Sent: Thursday, February 2, 2023 3:05 PM
> > 
> > All drivers are already required to support changing between active
> > UNMANAGED domains when using their attach_dev ops.
> 
> All drivers which don't have *broken* UNMANAGED domain?

No, all drivers.. It has always been used by VFIO.

> > + */
> > +int iommu_group_replace_domain(struct iommu_group *group,
> > +			       struct iommu_domain *new_domain)
> 
> what actual value does 'replace' give us? It's just a wrapper of
> __iommu_group_set_domain() then calling it set_domain is
> probably clearer. You can clarify the 'replace' behavior in the
> comment.

As the APIs are setup:

attach demands that no domain is currently set (eg the domain must be blocking)

replace permits the domain to be currently set

'set' vs 'attach' is really unclear what the intended difference is.

We could also address this by simply removing the protection from
attach, but it is not so clear if that is safe for the few users.

> > +{
> > +	int ret;
> > +
> > +	if (!new_domain)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&group->mutex);
> > +	ret = __iommu_group_set_domain(group, new_domain);
> > +	if (ret) {
> > +		if (__iommu_group_set_domain(group, group->domain))
> > +			__iommu_group_set_core_domain(group);
> > +	}
> 
> Can you elaborate the error handling here? Ideally if
> __iommu_group_set_domain() fails then group->domain shouldn't
> be changed. 

That isn't what it implements though. The internal helper leaves
things in a mess, it is for the caller to fix it, and it depends on
the caller what that means.

In this case the API cannot retain a hidden reference to the new
domain, so it must be purged, one way or another.

Jason
  

On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote:

> > +/**
> > + * iommu_group_replace_domain - replace the domain that a group is
> > attached to
> > + * @new_domain: new IOMMU domain to replace with
> > + * @group: IOMMU group that will be attached to the new domain
> > + *
> > + * This API allows the group to switch domains without being forced to go to
> > + * the blocking domain in-between.
> > + *
> > + * If the attached domain is a core domain (e.g. a default_domain), it will act
> > + * just like the iommu_attach_group().
> 
> I think you meant "the currently-attached domain", which implies a
> 'detached' state as you replied to Baolu.

Hmm, I don't see an implication, since we only have either
"the attached domain" or "a new domain" in the context?

With that being said, I can add "currently" in v2:
 * If the currently attached domain is a core domain (e.g. a default_domain),
 * it will act just like the iommu_attach_group().

Thanks
Nic
  

On Fri, Feb 03, 2023 at 11:03:20AM -0400, Jason Gunthorpe wrote:
> > > + */
> > > +int iommu_group_replace_domain(struct iommu_group *group,
> > > +			       struct iommu_domain *new_domain)
> > 
> > what actual value does 'replace' give us? It's just a wrapper of
> > __iommu_group_set_domain() then calling it set_domain is
> > probably clearer. You can clarify the 'replace' behavior in the
> > comment.
> 
> As the APIs are setup:
> 
> attach demands that no domain is currently set (eg the domain must be blocking)
> 
> replace permits the domain to be currently set
> 
> 'set' vs 'attach' is really unclear what the intended difference is.
> 
> We could also address this by simply removing the protection from
> attach, but it is not so clear if that is safe for the few users.

I can add a couple of lines to the commit message to make things
clear.

> > > +{
> > > +	int ret;
> > > +
> > > +	if (!new_domain)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&group->mutex);
> > > +	ret = __iommu_group_set_domain(group, new_domain);
> > > +	if (ret) {
> > > +		if (__iommu_group_set_domain(group, group->domain))
> > > +			__iommu_group_set_core_domain(group);
> > > +	}
> > 
> > Can you elaborate the error handling here? Ideally if
> > __iommu_group_set_domain() fails then group->domain shouldn't
> > be changed. 
> 
> That isn't what it implements though. The internal helper leaves
> things in a mess, it is for the caller to fix it, and it depends on
> the caller what that means.
> 
> In this case the API cannot retain a hidden reference to the new
> domain, so it must be purged, one way or another.

Considering it is a bit different than the existing APIs like
iommu_attach_group(), perhaps I should add a line of comments
in the fallback routine.

Thanks
Nic
  

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Saturday, February 4, 2023 1:45 AM
> 
> On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote:
> 
> > > +/**
> > > + * iommu_group_replace_domain - replace the domain that a group is
> > > attached to
> > > + * @new_domain: new IOMMU domain to replace with
> > > + * @group: IOMMU group that will be attached to the new domain
> > > + *
> > > + * This API allows the group to switch domains without being forced to
> go to
> > > + * the blocking domain in-between.
> > > + *
> > > + * If the attached domain is a core domain (e.g. a default_domain), it will
> act
> > > + * just like the iommu_attach_group().
> >
> > I think you meant "the currently-attached domain", which implies a
> > 'detached' state as you replied to Baolu.
> 
> Hmm, I don't see an implication, since we only have either
> "the attached domain" or "a new domain" in the context?

probably just me reading it as "the newly attached domain". 😊

> 
> With that being said, I can add "currently" in v2:
>  * If the currently attached domain is a core domain (e.g. a default_domain),
>  * it will act just like the iommu_attach_group().
> 

this is clearer.
  

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, February 3, 2023 11:03 PM
> 
> On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote:
> > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > Sent: Thursday, February 2, 2023 3:05 PM
> > >
> > > All drivers are already required to support changing between active
> > > UNMANAGED domains when using their attach_dev ops.
> >
> > All drivers which don't have *broken* UNMANAGED domain?
> 
> No, all drivers.. It has always been used by VFIO.

existing iommu_attach_group() doesn't support changing between
two UNMANAGED domains. only from default->unmanaged or
blocking->unmanaged.

Above statement is true only if this series is based on your unmerged
work removing DMA domain types.

> 
> > > +{
> > > +	int ret;
> > > +
> > > +	if (!new_domain)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&group->mutex);
> > > +	ret = __iommu_group_set_domain(group, new_domain);
> > > +	if (ret) {
> > > +		if (__iommu_group_set_domain(group, group->domain))
> > > +			__iommu_group_set_core_domain(group);
> > > +	}
> >
> > Can you elaborate the error handling here? Ideally if
> > __iommu_group_set_domain() fails then group->domain shouldn't
> > be changed.
> 
> That isn't what it implements though. The internal helper leaves
> things in a mess, it is for the caller to fix it, and it depends on
> the caller what that means.

I didn't see any warning of the mess and the caller's responsibility
in __iommu_group_set_domain(). Can it be documented clearly
so if someone wants to add a new caller on it he can clearly know
what to do?

and why doesn't iommu_attach_group() need to do anything
when an attach attempt fails? In the end it calls the same
iommu_group_do_attach_device() as __iommu_group_set_domain()
does.

btw looking at the code __iommu_group_set_domain():

	 * Note that this is called in error unwind paths, attaching to a
	 * domain that has already been attached cannot fail.
	 */
	ret = __iommu_group_for_each_dev(group, new_domain,
				iommu_group_do_attach_device);

with that we don't need fall back to core domain in above error
unwinding per this comment.

> 
> In this case the API cannot retain a hidden reference to the new
> domain, so it must be purged, one way or another.
> 

Could you elaborate where the hidden reference is retained?
  

On Mon, Feb 06, 2023 at 06:57:35AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Friday, February 3, 2023 11:03 PM
> > 
> > On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote:
> > > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > > Sent: Thursday, February 2, 2023 3:05 PM
> > > >
> > > > All drivers are already required to support changing between active
> > > > UNMANAGED domains when using their attach_dev ops.
> > >
> > > All drivers which don't have *broken* UNMANAGED domain?
> > 
> > No, all drivers.. It has always been used by VFIO.
> 
> existing iommu_attach_group() doesn't support changing between
> two UNMANAGED domains. only from default->unmanaged or
> blocking->unmanaged.

Yes, but before we added the blocking domains VFIO was changing
between unmanaged domains. Blocking domains are so new that no driver
could have suddenly started to depend on this.

> > > Can you elaborate the error handling here? Ideally if
> > > __iommu_group_set_domain() fails then group->domain shouldn't
> > > be changed.
> > 
> > That isn't what it implements though. The internal helper leaves
> > things in a mess, it is for the caller to fix it, and it depends on
> > the caller what that means.
> 
> I didn't see any warning of the mess and the caller's responsibility
> in __iommu_group_set_domain(). Can it be documented clearly
> so if someone wants to add a new caller on it he can clearly know
> what to do?

That would be nice..
 
> and why doesn't iommu_attach_group() need to do anything
> when an attach attempt fails? In the end it calls the same
> iommu_group_do_attach_device() as __iommu_group_set_domain()
> does.

That's a bug for sure.

 
> btw looking at the code __iommu_group_set_domain():
> 
> 	 * Note that this is called in error unwind paths, attaching to a
> 	 * domain that has already been attached cannot fail.
> 	 */
> 	ret = __iommu_group_for_each_dev(group, new_domain,
> 				iommu_group_do_attach_device);
> 
> with that we don't need fall back to core domain in above error
> unwinding per this comment.

That does make some sense.

I tried to make a patch to consolidate all this error handling once,
that would be the better way to approach this.

> > In this case the API cannot retain a hidden reference to the new
> > domain, so it must be purged, one way or another.
> 
> Could you elaborate where the hidden reference is retained?

Inside the driver, it can keep track of the domain pointer if
attach_dev succeeds

Jason
  

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Monday, February 6, 2023 9:25 PM
> 
> On Mon, Feb 06, 2023 at 06:57:35AM +0000, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > Sent: Friday, February 3, 2023 11:03 PM
> > >
> > > On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote:
> > > > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > > > Sent: Thursday, February 2, 2023 3:05 PM
> > > > >
> > > > > All drivers are already required to support changing between active
> > > > > UNMANAGED domains when using their attach_dev ops.
> > > >
> > > > All drivers which don't have *broken* UNMANAGED domain?
> > >
> > > No, all drivers.. It has always been used by VFIO.
> >
> > existing iommu_attach_group() doesn't support changing between
> > two UNMANAGED domains. only from default->unmanaged or
> > blocking->unmanaged.
> 
> Yes, but before we added the blocking domains VFIO was changing
> between unmanaged domains. Blocking domains are so new that no driver
> could have suddenly started to depend on this.

In legacy VFIO unmanaged domain was 1:1 associated with vfio
container. I didn't say how a group can switch between two
containers w/o going through transition to/from the default
domain, i.e. detach from 1st container and then attach to the 2nd.

> > btw looking at the code __iommu_group_set_domain():
> >
> > 	 * Note that this is called in error unwind paths, attaching to a
> > 	 * domain that has already been attached cannot fail.
> > 	 */
> > 	ret = __iommu_group_for_each_dev(group, new_domain,
> > 				iommu_group_do_attach_device);
> >
> > with that we don't need fall back to core domain in above error
> > unwinding per this comment.
> 
> That does make some sense.
> 
> I tried to make a patch to consolidate all this error handling once,
> that would be the better way to approach this.

that would be good.

> 
> > > In this case the API cannot retain a hidden reference to the new
> > > domain, so it must be purged, one way or another.
> >
> > Could you elaborate where the hidden reference is retained?
> 
> Inside the driver, it can keep track of the domain pointer if
> attach_dev succeeds
> 

Are you referring to no error unwinding in __iommu_group_for_each_dev()
so if it is failed some devices may have attach_dev succeeds then simply
recovering group->domain in __iommu_attach_group() is insufficient?
  

On Tue, Feb 07, 2023 at 12:32:50AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Monday, February 6, 2023 9:25 PM
> > 
> > On Mon, Feb 06, 2023 at 06:57:35AM +0000, Tian, Kevin wrote:
> > > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > > Sent: Friday, February 3, 2023 11:03 PM
> > > >
> > > > On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote:
> > > > > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > > > > Sent: Thursday, February 2, 2023 3:05 PM
> > > > > >
> > > > > > All drivers are already required to support changing between active
> > > > > > UNMANAGED domains when using their attach_dev ops.
> > > > >
> > > > > All drivers which don't have *broken* UNMANAGED domain?
> > > >
> > > > No, all drivers.. It has always been used by VFIO.
> > >
> > > existing iommu_attach_group() doesn't support changing between
> > > two UNMANAGED domains. only from default->unmanaged or
> > > blocking->unmanaged.
> > 
> > Yes, but before we added the blocking domains VFIO was changing
> > between unmanaged domains. Blocking domains are so new that no driver
> > could have suddenly started to depend on this.
> 
> In legacy VFIO unmanaged domain was 1:1 associated with vfio
> container. I didn't say how a group can switch between two
> containers w/o going through transition to/from the default
> domain, i.e. detach from 1st container and then attach to the 2nd.

Yes, in the past we went through the default domain which is basically
another unmanaged domain type. So unmanaged -> unmanaged is OK.

> > Inside the driver, it can keep track of the domain pointer if
> > attach_dev succeeds
> 
> Are you referring to no error unwinding in __iommu_group_for_each_dev()
> so if it is failed some devices may have attach_dev succeeds then simply
> recovering group->domain in __iommu_attach_group() is insufficient?

Yes

Jason
  

On Mon, Feb 06, 2023 at 09:25:17AM -0400, Jason Gunthorpe wrote:

> > > > Can you elaborate the error handling here? Ideally if
> > > > __iommu_group_set_domain() fails then group->domain shouldn't
> > > > be changed.
> > > 
> > > That isn't what it implements though. The internal helper leaves
> > > things in a mess, it is for the caller to fix it, and it depends on
> > > the caller what that means.
> > 
> > I didn't see any warning of the mess and the caller's responsibility
> > in __iommu_group_set_domain(). Can it be documented clearly
> > so if someone wants to add a new caller on it he can clearly know
> > what to do?
> 
> That would be nice..

I'd expect the doc to come with some other patch/series than this
replace series, so I think we should be fine without adding a line
of comments in this patch?

> > btw looking at the code __iommu_group_set_domain():
> > 
> > 	 * Note that this is called in error unwind paths, attaching to a
> > 	 * domain that has already been attached cannot fail.
> > 	 */
> > 	ret = __iommu_group_for_each_dev(group, new_domain,
> > 				iommu_group_do_attach_device);
> > 
> > with that we don't need fall back to core domain in above error
> > unwinding per this comment.
> 
> That does make some sense.
> 
> I tried to make a patch to consolidate all this error handling once,
> that would be the better way to approach this.

Then, I'll drop the core-domain line. Combining my reply above:

+	mutex_lock(&group->mutex);
+	ret = __iommu_group_set_domain(group, new_domain);
+	if (ret)
+		__iommu_group_set_domain(group, group->domain);
+	mutex_unlock(&group->mutex);

Will wrap things up and send v2 today.

Thanks
Nic
  

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, February 7, 2023 8:23 PM
> 
> On Tue, Feb 07, 2023 at 12:32:50AM +0000, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > Sent: Monday, February 6, 2023 9:25 PM
> > >
> > > On Mon, Feb 06, 2023 at 06:57:35AM +0000, Tian, Kevin wrote:
> > > > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > > > Sent: Friday, February 3, 2023 11:03 PM
> > > > >
> > > > > On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote:
> > > > > > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > > > > > Sent: Thursday, February 2, 2023 3:05 PM
> > > > > > >
> > > > > > > All drivers are already required to support changing between
> active
> > > > > > > UNMANAGED domains when using their attach_dev ops.
> > > > > >
> > > > > > All drivers which don't have *broken* UNMANAGED domain?
> > > > >
> > > > > No, all drivers.. It has always been used by VFIO.
> > > >
> > > > existing iommu_attach_group() doesn't support changing between
> > > > two UNMANAGED domains. only from default->unmanaged or
> > > > blocking->unmanaged.
> > >
> > > Yes, but before we added the blocking domains VFIO was changing
> > > between unmanaged domains. Blocking domains are so new that no
> driver
> > > could have suddenly started to depend on this.
> >
> > In legacy VFIO unmanaged domain was 1:1 associated with vfio
> > container. I didn't say how a group can switch between two
> > containers w/o going through transition to/from the default
> > domain, i.e. detach from 1st container and then attach to the 2nd.
> 
> Yes, in the past we went through the default domain which is basically
> another unmanaged domain type. So unmanaged -> unmanaged is OK.
> 

it's right in concept but confusing in current context whether DMA
domain still has its own type. That's why I replied earlier the statement
makes more sense after your patch which cleans up the domain type
is merged.
  

On Wed, Feb 08, 2023 at 04:25:58AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Tuesday, February 7, 2023 8:23 PM
> > 
> > On Tue, Feb 07, 2023 at 12:32:50AM +0000, Tian, Kevin wrote:
> > > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > > Sent: Monday, February 6, 2023 9:25 PM
> > > >
> > > > On Mon, Feb 06, 2023 at 06:57:35AM +0000, Tian, Kevin wrote:
> > > > > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > > > > Sent: Friday, February 3, 2023 11:03 PM
> > > > > >
> > > > > > On Fri, Feb 03, 2023 at 08:26:44AM +0000, Tian, Kevin wrote:
> > > > > > > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > > > > > > Sent: Thursday, February 2, 2023 3:05 PM
> > > > > > > >
> > > > > > > > All drivers are already required to support changing between
> > active
> > > > > > > > UNMANAGED domains when using their attach_dev ops.
> > > > > > >
> > > > > > > All drivers which don't have *broken* UNMANAGED domain?
> > > > > >
> > > > > > No, all drivers.. It has always been used by VFIO.
> > > > >
> > > > > existing iommu_attach_group() doesn't support changing between
> > > > > two UNMANAGED domains. only from default->unmanaged or
> > > > > blocking->unmanaged.
> > > >
> > > > Yes, but before we added the blocking domains VFIO was changing
> > > > between unmanaged domains. Blocking domains are so new that no
> > driver
> > > > could have suddenly started to depend on this.
> > >
> > > In legacy VFIO unmanaged domain was 1:1 associated with vfio
> > > container. I didn't say how a group can switch between two
> > > containers w/o going through transition to/from the default
> > > domain, i.e. detach from 1st container and then attach to the 2nd.
> > 
> > Yes, in the past we went through the default domain which is basically
> > another unmanaged domain type. So unmanaged -> unmanaged is OK.
> > 
> 
> it's right in concept but confusing in current context whether DMA
> domain still has its own type. That's why I replied earlier the statement
> makes more sense after your patch which cleans up the domain type
> is merged.

We are just reasoning about why the existing drivers are safe with
this - and my draft patch to remove DMA simply demonstrates that DMA
== UNMANAGED, and the above reasoning about VFIO in the past
demonstrates that this has historically be expected of drivers.

So I'm not so worried about patch order as long as things do work

Jason