[PATCH v2 0/6] iommu: Extend changing default domain to normal group

[PATCH v2 0/6] iommu: Extend changing default domain to normal group

[Lu Baolu]

The IOMMU group sysfs interface allows users to change the default
domain of a group. The current implementation uses device_lock() to make
sure that the devices in the group are not bound to any driver and won't
be bound during the process of changing the default domain. In order to
avoid a possible deadlock caused by lock order of device_lock and
group->mutex, it limits the functionality to singleton groups only.

The recently implemented DMA ownership framework can be applied here to
replace device_lock(). In addition, use group->mutex to ensure that the
iommu ops of the devices is always valid during the process of changing
default domain.

With above replacement and enhancement, the device_lock() could be
removed and the singleton-group-only limitation could be removed.

The whole series is also available on github:
https://github.com/LuBaolu/intel-iommu/commits/iommu-sysfs-default-domain-extension-v2

This series is on top of below series from Robin,
https://lore.kernel.org/linux-iommu/cover.1674753627.git.robin.murphy@arm.com/

Please help to review and suggest.

Change log:
v2:
 - Use group->mutex instead of an additional rw lock.

v1: initial post
 - https://lore.kernel.org/linux-iommu/20230213074941.919324-1-baolu.lu@linux.intel.com/

Lu Baolu (6):
  ARM/dma-mapping: Remove iommu_detach_device()
  iommu: Split iommu_group_remove_device() into helpers
  iommu: Same critical region for device release and removal
  iommu: Move lock from iommu_change_dev_def_domain() to its caller
  iommu: Replace device_lock() with group->mutex
  iommu: Cleanup iommu_change_dev_def_domain()

 arch/arm/mm/dma-mapping.c                     |   1 -
 drivers/iommu/iommu.c                         | 255 +++++++-----------
 .../ABI/testing/sysfs-kernel-iommu_groups     |   1 -
 3 files changed, 102 insertions(+), 155 deletions(-)

-- 
2.34.1

[PATCH v2 1/6] ARM/dma-mapping: Remove iommu_detach_device()

[Lu Baolu]

iommu_detach_device() attaches the default domain to the device, or if
default domain is not supported by the IOMMU driver, it calls its
set_platform_dma_ops callback. If the default domain is supported or
the IOMMU driver is not iommu-dma aware, iommu_detach_device() is
actually a noop.

The 64-bit ARM drivers always support default domain and iommu-dma is
even not enabled for 32-bit ARM. This turns out that iommu_detach_device()
is always a noop in arm_iommu_detach_device(). Remove it to avoid dead
code.

The bonus is that it also removes a obstacle of arm_iommu_detach_device()
re-entering the iommu core during release_device. With this removed, the
iommu core code could be simplified a lot.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 arch/arm/mm/dma-mapping.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 8bc01071474a..dcbc2f4586d4 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1699,7 +1699,6 @@ void arm_iommu_detach_device(struct device *dev)
 		return;
 	}
 
-	iommu_detach_device(mapping->domain, dev);
 	kref_put(&mapping->kref, release_iommu_mapping);
 	to_dma_iommu_mapping(dev) = NULL;
 	set_dma_ops(dev, NULL);
-- 
2.34.1

Re: [PATCH v2 1/6] ARM/dma-mapping: Remove iommu_detach_device()

[Jason Gunthorpe]

On Fri, Feb 17, 2023 at 05:47:31PM +0800, Lu Baolu wrote:
> iommu_detach_device() attaches the default domain to the device, or if
> default domain is not supported by the IOMMU driver, it calls its
> set_platform_dma_ops callback. If the default domain is supported or
> the IOMMU driver is not iommu-dma aware, iommu_detach_device() is
> actually a noop.
> 
> The 64-bit ARM drivers always support default domain and iommu-dma is
> even not enabled for 32-bit ARM. This turns out that iommu_detach_device()
> is always a noop in arm_iommu_detach_device(). Remove it to avoid dead
> code.

This isn't entirely right..

The purpose of the iommu_detach_device here should be to make the
iommu driver stop using the domain that it is about to free. It cannot
be a NOP or it is a UAF.

I think things have become confused.

What we decided is that NULL domain is supposed to mean the DMA
translation is controlled by the platform, the concept is modeled
after S390's private iommu implementation. This means the current
IOMMU translation is invisibly set to something that matches the
device's dma_ops.

arm_iommu doesn't work that way, it allocates and assigns domains so
when the platform DMA ops are in control the group->domain is not NULL
- which is the opposite of S390's design. Further when arm_iommu asks
for a NULL domain it doesn't mean "put it back to platform DMA ops" it
really means "park the IOMMU it is not being used anymore"

So.. Blah - we had two meanings for group->domain = NULL and didn't
quite get it right.

IMHO the way to make sense of this is to always have a domain attached
and remove group->domain = NULL and set_platform_dma entirely. If the
driver doesn't want to use dma_iommu then it should provide its own
iommu_domain that it wants attached whenever the iommu API is not
being used.

You can see this in the exynos fix because what it is doing is calling
__sysmmu_disable() from set_platform_dma which is either IDENTITY or
BLOCKING IOMMU behavior.

Then we can document what the idle domain is supposed to be doing in
each of the drivers, but the core code is logical and doesn't have a
confusing overloaded domain = NULL case.

Something like this as a starting idea of how the drivers could look.

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index b0cde22119875e..143d1abcae2641 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -948,6 +948,20 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
 		&pagetable);
 }
 
+static struct iommu_domain_ops exynos_private_ops = {
+	.attach_dev_nofail = &exynos_iommu_detach_device,
+};
+
+static struct iommu_domain exynos_private_domain = {
+	/*
+	 * This is private because nobody knew what __sysmmu_disable() does.
+	 * When someone figures that out this should be made blocking or
+	 * identity
+	 */
+	.type = IOMMU_DOMAN_PRIVATE,
+	.ops = &exynos_private_ops,
+};
+
 static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
 				   struct device *dev)
 {
@@ -1400,11 +1414,11 @@ static const struct iommu_ops exynos_iommu_ops = {
 	.device_group = generic_device_group,
 	.probe_device = exynos_iommu_probe_device,
 	.release_device = exynos_iommu_release_device,
+	.idle_domain = IS_ENABLED(CONFIG_ARM) ? &exynos_private_domain : NULL,
 	.pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
 	.of_xlate = exynos_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= exynos_iommu_attach_device,
-		.detach_dev	= exynos_iommu_detach_device,
 		.map		= exynos_iommu_map,
 		.unmap		= exynos_iommu_unmap,
 		.iova_to_phys	= exynos_iommu_iova_to_phys,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index de91dd88705bd3..b71a1667f43bd5 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -387,14 +387,13 @@ int iommu_probe_device(struct device *dev)
 		goto err_release;
 	}
 
-	/*
-	 * Try to allocate a default domain - needs support from the
-	 * IOMMU driver. There are still some drivers which don't
-	 * support default domains, so the return value is not yet
-	 * checked.
-	 */
 	mutex_lock(&group->mutex);
-	iommu_alloc_default_domain(group, dev);
+	ret = iommu_alloc_default_domain(group, dev);
+	if (ret) {
+		mutex_unlock(&group->mutex);
+		iommu_group_put(group);
+		goto err_release;
+	}
 
 	/*
 	 * If device joined an existing group which has been claimed, don't
@@ -1645,7 +1644,15 @@ static int iommu_alloc_default_domain(struct iommu_group *group,
 
 	type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type;
 
-	return iommu_group_alloc_default_domain(dev->bus, group, type);
+	if (!iommu_group_alloc_default_domain(dev->bus, group, type))
+		return 0;
+
+	/*
+	 * Driver must support default domains or provide an legacy idle domain
+	 */
+	if (WARN_ON(!dev_iommu_ops(dev)->idle_domain))
+		return -EINVAL;
+	return 0;
 }
 
 /**
@@ -2172,17 +2179,22 @@ static int __iommu_group_set_domain(struct iommu_group *group,
 		return 0;
 
 	/*
-	 * New drivers should support default domains and so the detach_dev() op
-	 * will never be called. Otherwise the NULL domain represents some
-	 * platform specific behavior.
+	 * New drivers should support default domains, otherwise the driver
+	 * must provide a domain to be attached when the iommu subsystem
+	 * is not using the device. The purpose of this domain depends
+	 * on how the iommu driver is operating the dma_ops.
 	 */
 	if (!new_domain) {
-		if (WARN_ON(!group->domain->ops->detach_dev))
+		struct group_device *grp_dev;
+		const struct iommu_ops *ops;
+
+		grp_dev = list_first_entry(&group->devices, struct group_device,
+					   list);
+		ops = dev_iommu_ops(grp_dev->dev);
+
+		if (WARN_ON(!ops->idle_domain))
 			return -EINVAL;
-		__iommu_group_for_each_dev(group, group->domain,
-					   iommu_group_do_detach_device);
-		group->domain = NULL;
-		return 0;
+		new_domain = ops->idle_domain;
 	}
 
 	/*
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 46e1347bfa2286..a17069368d70d9 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -91,6 +91,7 @@ struct iommu_domain_geometry {
 				 __IOMMU_DOMAIN_DMA_API |	\
 				 __IOMMU_DOMAIN_DMA_FQ)
 #define IOMMU_DOMAIN_SVA	(__IOMMU_DOMAIN_SVA)
+#define IOMMU_DOMAN_PRIVATE	(1 << 5)
 
 struct iommu_domain {
 	unsigned type;
@@ -272,7 +273,18 @@ struct iommu_ops {
 			     struct iommu_fault_event *evt,
 			     struct iommu_page_response *msg);
 
+	/*
+	 * driver wants to use dma_ops provided by dma-iommu.c, return
+	 * the recommended initial IDENTITY/DMA domain.
+	 */
 	int (*def_domain_type)(struct device *dev);
+
+	/*
+	 * Driver is not using dma-iommu.c, when the iommu core is not
+	 * using the device it will attach it to this domain.
+	 */
+	struct iommu_domain *idle_domain;
+
 	void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid);
 
 	const struct iommu_domain_ops *default_domain_ops;
@@ -315,6 +327,7 @@ struct iommu_ops {
  * @free: Release the domain after use.
  */
 struct iommu_domain_ops {
+	void (*attach_dev_nofail)(struct iommu_domain *domain, struct device *dev);
 	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
 	void (*detach_dev)(struct iommu_domain *domain, struct device *dev);
 	int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev,

Re: [PATCH v2 1/6] ARM/dma-mapping: Remove iommu_detach_device()

[Baolu Lu]

On 2/17/23 11:39 PM, Jason Gunthorpe wrote:
> On Fri, Feb 17, 2023 at 05:47:31PM +0800, Lu Baolu wrote:
>> iommu_detach_device() attaches the default domain to the device, or if
>> default domain is not supported by the IOMMU driver, it calls its
>> set_platform_dma_ops callback. If the default domain is supported or
>> the IOMMU driver is not iommu-dma aware, iommu_detach_device() is
>> actually a noop.
>>
>> The 64-bit ARM drivers always support default domain and iommu-dma is
>> even not enabled for 32-bit ARM. This turns out that iommu_detach_device()
>> is always a noop in arm_iommu_detach_device(). Remove it to avoid dead
>> code.
> 
> This isn't entirely right..
> 
> The purpose of the iommu_detach_device here should be to make the
> iommu driver stop using the domain that it is about to free. It cannot
> be a NOP or it is a UAF.
> 
> I think things have become confused.
> 
> What we decided is that NULL domain is supposed to mean the DMA
> translation is controlled by the platform, the concept is modeled
> after S390's private iommu implementation. This means the current
> IOMMU translation is invisibly set to something that matches the
> device's dma_ops.

Yes. This matches what I understood.

> 
> arm_iommu doesn't work that way, it allocates and assigns domains so
> when the platform DMA ops are in control the group->domain is not NULL

This is what the iommu core assumes, right? Any iommu group should
always has a domain attached, default domain, blocking domain or driver-
owned unmanaged domain. The iommu core just switches between different
domains.

> - which is the opposite of S390's design. Further when arm_iommu asks
> for a NULL domain it doesn't mean "put it back to platform DMA ops" it
> really means "park the IOMMU it is not being used anymore"

This is what identity domain and blocking domains were designed to do,
right?

If my understanding is right, ARM presumably could implement the
identity default domain and blocking domain. With that implemented,
iommu_attach/detac_device() could be removed from drivers and everything
then could go through the iommu core.

Best regards,
baolu

> 
> So.. Blah - we had two meanings for group->domain = NULL and didn't
> quite get it right.
> 
> IMHO the way to make sense of this is to always have a domain attached
> and remove group->domain = NULL and set_platform_dma entirely. If the
> driver doesn't want to use dma_iommu then it should provide its own
> iommu_domain that it wants attached whenever the iommu API is not
> being used.
> 
> You can see this in the exynos fix because what it is doing is calling
> __sysmmu_disable() from set_platform_dma which is either IDENTITY or
> BLOCKING IOMMU behavior.
> 
> Then we can document what the idle domain is supposed to be doing in
> each of the drivers, but the core code is logical and doesn't have a
> confusing overloaded domain = NULL case.
> 
> Something like this as a starting idea of how the drivers could look.
> 
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index b0cde22119875e..143d1abcae2641 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -948,6 +948,20 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
>   		&pagetable);
>   }
>   
> +static struct iommu_domain_ops exynos_private_ops = {
> +	.attach_dev_nofail = &exynos_iommu_detach_device,
> +};
> +
> +static struct iommu_domain exynos_private_domain = {
> +	/*
> +	 * This is private because nobody knew what __sysmmu_disable() does.
> +	 * When someone figures that out this should be made blocking or
> +	 * identity
> +	 */
> +	.type = IOMMU_DOMAN_PRIVATE,
> +	.ops = &exynos_private_ops,
> +};
> +
>   static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
>   				   struct device *dev)
>   {
> @@ -1400,11 +1414,11 @@ static const struct iommu_ops exynos_iommu_ops = {
>   	.device_group = generic_device_group,
>   	.probe_device = exynos_iommu_probe_device,
>   	.release_device = exynos_iommu_release_device,
> +	.idle_domain = IS_ENABLED(CONFIG_ARM) ? &exynos_private_domain : NULL,
>   	.pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
>   	.of_xlate = exynos_iommu_of_xlate,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
>   		.attach_dev	= exynos_iommu_attach_device,
> -		.detach_dev	= exynos_iommu_detach_device,
>   		.map		= exynos_iommu_map,
>   		.unmap		= exynos_iommu_unmap,
>   		.iova_to_phys	= exynos_iommu_iova_to_phys,
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index de91dd88705bd3..b71a1667f43bd5 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -387,14 +387,13 @@ int iommu_probe_device(struct device *dev)
>   		goto err_release;
>   	}
>   
> -	/*
> -	 * Try to allocate a default domain - needs support from the
> -	 * IOMMU driver. There are still some drivers which don't
> -	 * support default domains, so the return value is not yet
> -	 * checked.
> -	 */
>   	mutex_lock(&group->mutex);
> -	iommu_alloc_default_domain(group, dev);
> +	ret = iommu_alloc_default_domain(group, dev);
> +	if (ret) {
> +		mutex_unlock(&group->mutex);
> +		iommu_group_put(group);
> +		goto err_release;
> +	}
>   
>   	/*
>   	 * If device joined an existing group which has been claimed, don't
> @@ -1645,7 +1644,15 @@ static int iommu_alloc_default_domain(struct iommu_group *group,
>   
>   	type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type;
>   
> -	return iommu_group_alloc_default_domain(dev->bus, group, type);
> +	if (!iommu_group_alloc_default_domain(dev->bus, group, type))
> +		return 0;
> +
> +	/*
> +	 * Driver must support default domains or provide an legacy idle domain
> +	 */
> +	if (WARN_ON(!dev_iommu_ops(dev)->idle_domain))
> +		return -EINVAL;
> +	return 0;
>   }
>   
>   /**
> @@ -2172,17 +2179,22 @@ static int __iommu_group_set_domain(struct iommu_group *group,
>   		return 0;
>   
>   	/*
> -	 * New drivers should support default domains and so the detach_dev() op
> -	 * will never be called. Otherwise the NULL domain represents some
> -	 * platform specific behavior.
> +	 * New drivers should support default domains, otherwise the driver
> +	 * must provide a domain to be attached when the iommu subsystem
> +	 * is not using the device. The purpose of this domain depends
> +	 * on how the iommu driver is operating the dma_ops.
>   	 */
>   	if (!new_domain) {
> -		if (WARN_ON(!group->domain->ops->detach_dev))
> +		struct group_device *grp_dev;
> +		const struct iommu_ops *ops;
> +
> +		grp_dev = list_first_entry(&group->devices, struct group_device,
> +					   list);
> +		ops = dev_iommu_ops(grp_dev->dev);
> +
> +		if (WARN_ON(!ops->idle_domain))
>   			return -EINVAL;
> -		__iommu_group_for_each_dev(group, group->domain,
> -					   iommu_group_do_detach_device);
> -		group->domain = NULL;
> -		return 0;
> +		new_domain = ops->idle_domain;
>   	}
>   
>   	/*
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 46e1347bfa2286..a17069368d70d9 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -91,6 +91,7 @@ struct iommu_domain_geometry {
>   				 __IOMMU_DOMAIN_DMA_API |	\
>   				 __IOMMU_DOMAIN_DMA_FQ)
>   #define IOMMU_DOMAIN_SVA	(__IOMMU_DOMAIN_SVA)
> +#define IOMMU_DOMAN_PRIVATE	(1 << 5)
>   
>   struct iommu_domain {
>   	unsigned type;
> @@ -272,7 +273,18 @@ struct iommu_ops {
>   			     struct iommu_fault_event *evt,
>   			     struct iommu_page_response *msg);
>   
> +	/*
> +	 * driver wants to use dma_ops provided by dma-iommu.c, return
> +	 * the recommended initial IDENTITY/DMA domain.
> +	 */
>   	int (*def_domain_type)(struct device *dev);
> +
> +	/*
> +	 * Driver is not using dma-iommu.c, when the iommu core is not
> +	 * using the device it will attach it to this domain.
> +	 */
> +	struct iommu_domain *idle_domain;
> +
>   	void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid);
>   
>   	const struct iommu_domain_ops *default_domain_ops;
> @@ -315,6 +327,7 @@ struct iommu_ops {
>    * @free: Release the domain after use.
>    */
>   struct iommu_domain_ops {
> +	void (*attach_dev_nofail)(struct iommu_domain *domain, struct device *dev);
>   	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
>   	void (*detach_dev)(struct iommu_domain *domain, struct device *dev);
>   	int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev,

Re: [PATCH v2 1/6] ARM/dma-mapping: Remove iommu_detach_device()

[Jason Gunthorpe]

On Sat, Feb 18, 2023 at 02:59:16PM +0800, Baolu Lu wrote:

> > arm_iommu doesn't work that way, it allocates and assigns domains so
> > when the platform DMA ops are in control the group->domain is not NULL
> 
> This is what the iommu core assumes, right? Any iommu group should
> always has a domain attached, default domain, blocking domain or driver-
> owned unmanaged domain. The iommu core just switches between different
> domains.

That would be nice, but we still have NULL domains in some cases right
now.
 
> > - which is the opposite of S390's design. Further when arm_iommu asks
> > for a NULL domain it doesn't mean "put it back to platform DMA ops" it
> > really means "park the IOMMU it is not being used anymore"
> 
> This is what identity domain and blocking domains were designed to do,
> right?
>
> If my understanding is right, ARM presumably could implement the
> identity default domain and blocking domain. With that implemented,
> iommu_attach/detac_device() could be removed from drivers and everything
> then could go through the iommu core.

Yes, ideally, but I have no idea what the few reamining drivers do
with their code to properly classify it. So what I showed in the
little sketch was to just mark it DOMAIN_PRIVATE and if someone knows
the right answer they can change it to blocking/identity someday.

In the mean time we can get rid of the NULL domain situation. The core
code would immediately attach either the default or 'idle' domain on
probe and a device will always have a domain attached until release.

I still don't entirely understand how exynox works. In ARM64 mode it
should have default domains, but since it doesn't supply a
def_domain_type() op it only gets the default set of default domains
eg PCI devices.

So on ARM64 some devices don't get default domains, and on ARM32 those
devices use arm_iommu but crashed because of a lack of default domain?

It is confusing. It is harmelss to create a DMA default domain on
ARM32, at worst it wastes a bit of memory. So maybe the fact it
crashes on ARM32 indicates a bug on ARM64 where some devices are not
properly using default domains??

Jason

Re: [PATCH v2 1/6] ARM/dma-mapping: Remove iommu_detach_device()

[Robin Murphy]

On 2023-02-17 09:47, Lu Baolu wrote:
> iommu_detach_device() attaches the default domain to the device, or if
> default domain is not supported by the IOMMU driver, it calls its
> set_platform_dma_ops callback. If the default domain is supported or
> the IOMMU driver is not iommu-dma aware, iommu_detach_device() is
> actually a noop.
> 
> The 64-bit ARM drivers always support default domain and iommu-dma is
> even not enabled for 32-bit ARM. This turns out that iommu_detach_device()
> is always a noop in arm_iommu_detach_device(). Remove it to avoid dead
> code.

Huh? This call clearly balances the iommu_attach_device() call in 
arm_iommu_attach_device() - it has nothing to do with default domains.

> The bonus is that it also removes a obstacle of arm_iommu_detach_device()
> re-entering the iommu core during release_device. With this removed, the
> iommu core code could be simplified a lot.

That needs to be worked around in those release paths, not by breaking 
the public API. Should probably just be a case of doing as much "detach" 
as necessary directly, then calling arm_iommu_release_mapping(). Just 
beware that arm_teardown_iommu_dma_ops() may or may not have done some 
of it already, depending on whether a driver ever bound to the device.

Thanks,
Robin.

> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>   arch/arm/mm/dma-mapping.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 8bc01071474a..dcbc2f4586d4 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1699,7 +1699,6 @@ void arm_iommu_detach_device(struct device *dev)
>   		return;
>   	}
>   
> -	iommu_detach_device(mapping->domain, dev);
>   	kref_put(&mapping->kref, release_iommu_mapping);
>   	to_dma_iommu_mapping(dev) = NULL;
>   	set_dma_ops(dev, NULL);

[PATCH v2 2/6] iommu: Split iommu_group_remove_device() into helpers

[Lu Baolu]

So that code could be re-used by iommu_release_device() in the subsequent
change. No intention for functionality change.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 64 +++++++++++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 20 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 28c6b088aedd..6247883991e2 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1069,6 +1069,46 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 }
 EXPORT_SYMBOL_GPL(iommu_group_add_device);
 
+/*
+ * Remove a device from a group's device list and return the group device
+ * if successful.
+ */
+static struct group_device *
+__iommu_group_remove_device(struct iommu_group *group, struct device *dev)
+{
+	struct group_device *device;
+
+	lockdep_assert_held(&group->mutex);
+	list_for_each_entry(device, &group->devices, list) {
+		if (device->dev == dev) {
+			list_del(&device->list);
+			return device;
+		}
+	}
+
+	return NULL;
+}
+
+/*
+ * Release a device from its group and decrements the iommu group reference
+ * count.
+ */
+static void __iommu_group_release_device(struct iommu_group *group,
+					 struct group_device *grp_dev)
+{
+	struct device *dev = grp_dev->dev;
+
+	sysfs_remove_link(group->devices_kobj, grp_dev->name);
+	sysfs_remove_link(&dev->kobj, "iommu_group");
+
+	trace_remove_device_from_group(group->id, dev);
+
+	kfree(grp_dev->name);
+	kfree(grp_dev);
+	dev->iommu_group = NULL;
+	kobject_put(group->devices_kobj);
+}
+
 /**
  * iommu_group_remove_device - remove a device from it's current group
  * @dev: device to be removed
@@ -1079,7 +1119,7 @@ EXPORT_SYMBOL_GPL(iommu_group_add_device);
 void iommu_group_remove_device(struct device *dev)
 {
 	struct iommu_group *group = dev->iommu_group;
-	struct group_device *tmp_device, *device = NULL;
+	struct group_device *device;
 
 	if (!group)
 		return;
@@ -1087,27 +1127,11 @@ void iommu_group_remove_device(struct device *dev)
 	dev_info(dev, "Removing from iommu group %d\n", group->id);
 
 	mutex_lock(&group->mutex);
-	list_for_each_entry(tmp_device, &group->devices, list) {
-		if (tmp_device->dev == dev) {
-			device = tmp_device;
-			list_del(&device->list);
-			break;
-		}
-	}
+	device = __iommu_group_remove_device(group, dev);
 	mutex_unlock(&group->mutex);
 
-	if (!device)
-		return;
-
-	sysfs_remove_link(group->devices_kobj, device->name);
-	sysfs_remove_link(&dev->kobj, "iommu_group");
-
-	trace_remove_device_from_group(group->id, dev);
-
-	kfree(device->name);
-	kfree(device);
-	dev->iommu_group = NULL;
-	kobject_put(group->devices_kobj);
+	if (device)
+		__iommu_group_release_device(group, device);
 }
 EXPORT_SYMBOL_GPL(iommu_group_remove_device);
 
-- 
2.34.1

[PATCH v2 3/6] iommu: Same critical region for device release and removal

[Lu Baolu]

In a non-driver context, it is crucial to ensure the consistency of a
device's iommu ops. Otherwise, it may result in a situation where a
device is released but it's iommu ops are still used.

Put the ops->release_device and __iommu_group_remove_device() in a some
group->mutext critical region, so that, as long as group->mutex is held
and the device is in its group's device list, its iommu ops are always
consistent.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 6247883991e2..093692308b80 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -101,6 +101,10 @@ static int iommu_create_device_direct_mappings(struct iommu_group *group,
 static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
 static ssize_t iommu_group_store_type(struct iommu_group *group,
 				      const char *buf, size_t count);
+static struct group_device *
+__iommu_group_remove_device(struct iommu_group *group, struct device *dev);
+static void __iommu_group_release_device(struct iommu_group *group,
+					 struct group_device *grp_dev);
 
 #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)		\
 struct iommu_group_attribute iommu_group_attr_##_name =		\
@@ -466,18 +470,25 @@ int iommu_probe_device(struct device *dev)
 
 void iommu_release_device(struct device *dev)
 {
+	struct iommu_group *group = dev->iommu_group;
+	struct group_device *device;
 	const struct iommu_ops *ops;
 
-	if (!dev->iommu)
+	if (!dev->iommu || !group)
 		return;
 
 	iommu_device_unlink(dev->iommu->iommu_dev, dev);
 
+	mutex_lock(&group->mutex);
 	ops = dev_iommu_ops(dev);
 	if (ops->release_device)
 		ops->release_device(dev);
+	device = __iommu_group_remove_device(group, dev);
+	mutex_unlock(&group->mutex);
+
+	if (device)
+		__iommu_group_release_device(group, device);
 
-	iommu_group_remove_device(dev);
 	module_put(ops->owner);
 	dev_iommu_free(dev);
 }
-- 
2.34.1

Re: [PATCH v2 3/6] iommu: Same critical region for device release and removal

[Jason Gunthorpe]

On Fri, Feb 17, 2023 at 05:47:33PM +0800, Lu Baolu wrote:
> In a non-driver context, it is crucial to ensure the consistency of a
> device's iommu ops. Otherwise, it may result in a situation where a
> device is released but it's iommu ops are still used.
> 
> Put the ops->release_device and __iommu_group_remove_device() in a some
> group->mutext critical region, so that, as long as group->mutex is held
> and the device is in its group's device list, its iommu ops are always
> consistent.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 6247883991e2..093692308b80 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -101,6 +101,10 @@ static int iommu_create_device_direct_mappings(struct iommu_group *group,
>  static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
>  static ssize_t iommu_group_store_type(struct iommu_group *group,
>  				      const char *buf, size_t count);
> +static struct group_device *
> +__iommu_group_remove_device(struct iommu_group *group, struct device *dev);
> +static void __iommu_group_release_device(struct iommu_group *group,
> +					 struct group_device *grp_dev);

Seems like a hunk is missing from this patch?

Jason

Re: [PATCH v2 3/6] iommu: Same critical region for device release and removal

[Baolu Lu]

On 2/17/23 11:40 PM, Jason Gunthorpe wrote:
> On Fri, Feb 17, 2023 at 05:47:33PM +0800, Lu Baolu wrote:
>> In a non-driver context, it is crucial to ensure the consistency of a
>> device's iommu ops. Otherwise, it may result in a situation where a
>> device is released but it's iommu ops are still used.
>>
>> Put the ops->release_device and __iommu_group_remove_device() in a some
>> group->mutext critical region, so that, as long as group->mutex is held
>> and the device is in its group's device list, its iommu ops are always
>> consistent.
>>
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/iommu.c | 15 +++++++++++++--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 6247883991e2..093692308b80 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -101,6 +101,10 @@ static int iommu_create_device_direct_mappings(struct iommu_group *group,
>>   static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
>>   static ssize_t iommu_group_store_type(struct iommu_group *group,
>>   				      const char *buf, size_t count);
>> +static struct group_device *
>> +__iommu_group_remove_device(struct iommu_group *group, struct device *dev);
>> +static void __iommu_group_release_device(struct iommu_group *group,
>> +					 struct group_device *grp_dev);
> Seems like a hunk is missing from this patch?

Did you mean below block of change? If so, I will add it in the next
version.

+
+	/*
+	 * If the group has become empty then ownership must have been released,
+	 * and the current domain must be set back to NULL or the default
+	 * domain.
+	 */
+	if (list_empty(&group->devices))
+		WARN_ON(group->owner_cnt ||
+			group->domain != group->default_domain);
+
+	/*
+	 * release_device() must stop using any attached domain on the device.
+	 * If there are still other devices in the group they are not effected
+	 * by this callback.
+	 *
+	 * The IOMMU driver must set the device to either an identity or
+	 * blocking translation and stop using any domain pointer, as it is
+	 * going to be freed.
+	 */
  	if (ops->release_device)
  		ops->release_device(dev);
+	mutex_unlock(&group->mutex);

By the way, can I add you signed-off-by when I use the code you posted
in the discussion thread?

Best regards,
baolu

Re: [PATCH v2 3/6] iommu: Same critical region for device release and removal

[Jason Gunthorpe]

On Sat, Feb 18, 2023 at 03:29:12PM +0800, Baolu Lu wrote:
> On 2/17/23 11:40 PM, Jason Gunthorpe wrote:
> > On Fri, Feb 17, 2023 at 05:47:33PM +0800, Lu Baolu wrote:
> > > In a non-driver context, it is crucial to ensure the consistency of a
> > > device's iommu ops. Otherwise, it may result in a situation where a
> > > device is released but it's iommu ops are still used.
> > > 
> > > Put the ops->release_device and __iommu_group_remove_device() in a some
> > > group->mutext critical region, so that, as long as group->mutex is held
> > > and the device is in its group's device list, its iommu ops are always
> > > consistent.
> > > 
> > > Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
> > > ---
> > >   drivers/iommu/iommu.c | 15 +++++++++++++--
> > >   1 file changed, 13 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index 6247883991e2..093692308b80 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -101,6 +101,10 @@ static int iommu_create_device_direct_mappings(struct iommu_group *group,
> > >   static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
> > >   static ssize_t iommu_group_store_type(struct iommu_group *group,
> > >   				      const char *buf, size_t count);
> > > +static struct group_device *
> > > +__iommu_group_remove_device(struct iommu_group *group, struct device *dev);
> > > +static void __iommu_group_release_device(struct iommu_group *group,
> > > +					 struct group_device *grp_dev);
> > Seems like a hunk is missing from this patch?
> 
> Did you mean below block of change? If so, I will add it in the next
> version.

I  mean this changed the protoype but I didn't see a change in the
actual funtion?

> By the way, can I add you signed-off-by when I use the code you posted
> in the discussion thread?

Yes

Jason

[PATCH v2 4/6] iommu: Move lock from iommu_change_dev_def_domain() to its caller

[Lu Baolu]

The intention is to make it possible to put group ownership check and
default domain change in a same critical region protected by the group's
mutex lock. No intentional functional change.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 093692308b80..e1ae1eb4faf0 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2892,7 +2892,7 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
 	int ret, dev_def_dom;
 	struct device *dev;
 
-	mutex_lock(&group->mutex);
+	lockdep_assert_held(&group->mutex);
 
 	if (group->default_domain != group->domain) {
 		dev_err_ratelimited(prev_dev, "Group not assigned to default domain\n");
@@ -2980,28 +2980,15 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
 		goto free_new_domain;
 
 	group->domain = group->default_domain;
-
-	/*
-	 * Release the mutex here because ops->probe_finalize() call-back of
-	 * some vendor IOMMU drivers calls arm_iommu_attach_device() which
-	 * in-turn might call back into IOMMU core code, where it tries to take
-	 * group->mutex, resulting in a deadlock.
-	 */
-	mutex_unlock(&group->mutex);
-
-	/* Make sure dma_ops is appropriatley set */
-	iommu_group_do_probe_finalize(dev, group->default_domain);
 	iommu_domain_free(prev_dom);
+
 	return 0;
 
 free_new_domain:
 	iommu_domain_free(group->default_domain);
 	group->default_domain = prev_dom;
 	group->domain = prev_dom;
-
 out:
-	mutex_unlock(&group->mutex);
-
 	return ret;
 }
 
@@ -3089,7 +3076,19 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
 		goto out;
 	}
 
+	mutex_lock(&group->mutex);
 	ret = iommu_change_dev_def_domain(group, dev, req_type);
+	/*
+	 * Release the mutex here because ops->probe_finalize() call-back of
+	 * some vendor IOMMU drivers calls arm_iommu_attach_device() which
+	 * in-turn might call back into IOMMU core code, where it tries to take
+	 * group->mutex, resulting in a deadlock.
+	 */
+	mutex_unlock(&group->mutex);
+
+	/* Make sure dma_ops is appropriatley set */
+	if (!ret)
+		iommu_group_do_probe_finalize(dev, group->default_domain);
 	ret = ret ?: count;
 
 out:
-- 
2.34.1

[PATCH v2 5/6] iommu: Replace device_lock() with group->mutex

[Lu Baolu]

device_lock() was used in iommu_group_store_type() to prevent the
devices in an iommu group from being attached by any device driver.
On the other hand, in order to avoid lock race between group->mutex
and device_lock(), it limited the usage scenario to the singleton
groups.

We already have the DMA ownership scheme to avoid driver attachment
and group->mutex ensures that device ops are always valid, there's
no need for device_lock() anymore. Remove device_lock() and the
singleton group limitation.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 78 +++++++++----------------------------------
 1 file changed, 16 insertions(+), 62 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index e1ae1eb4faf0..18dac155a178 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2958,14 +2958,6 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
 		goto out;
 	}
 
-	/* We can bring up a flush queue without tearing down the domain */
-	if (type == IOMMU_DOMAIN_DMA_FQ && prev_dom->type == IOMMU_DOMAIN_DMA) {
-		ret = iommu_dma_init_fq(prev_dom);
-		if (!ret)
-			prev_dom->type = IOMMU_DOMAIN_DMA_FQ;
-		goto out;
-	}
-
 	/* Sets group->default_domain to the newly allocated domain */
 	ret = iommu_group_alloc_default_domain(group, dev, type);
 	if (ret)
@@ -2998,7 +2990,7 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
  * transition. Return failure if this isn't met.
  *
  * We need to consider the race between this and the device release path.
- * device_lock(dev) is used here to guarantee that the device release path
+ * group->mutex is used here to guarantee that the device release path
  * will not be entered at the same time.
  */
 static ssize_t iommu_group_store_type(struct iommu_group *group,
@@ -3024,60 +3016,27 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
 	else
 		return -EINVAL;
 
-	/*
-	 * Lock/Unlock the group mutex here before device lock to
-	 * 1. Make sure that the iommu group has only one device (this is a
-	 *    prerequisite for step 2)
-	 * 2. Get struct *dev which is needed to lock device
-	 */
 	mutex_lock(&group->mutex);
-	if (iommu_group_device_count(group) != 1) {
+	/* We can bring up a flush queue without tearing down the domain. */
+	if (req_type == IOMMU_DOMAIN_DMA_FQ &&
+	    group->default_domain->type == IOMMU_DOMAIN_DMA) {
+		ret = iommu_dma_init_fq(group->default_domain);
+		if (!ret)
+			group->default_domain->type = IOMMU_DOMAIN_DMA_FQ;
 		mutex_unlock(&group->mutex);
-		pr_err_ratelimited("Cannot change default domain: Group has more than one device\n");
-		return -EINVAL;
+
+		return ret ?: count;
 	}
 
-	/* Since group has only one device */
+	/* Otherwise, ensure that device exists and no driver is bound. */
 	dev = iommu_group_first_dev(group);
-	get_device(dev);
-
-	/*
-	 * Don't hold the group mutex because taking group mutex first and then
-	 * the device lock could potentially cause a deadlock as below. Assume
-	 * two threads T1 and T2. T1 is trying to change default domain of an
-	 * iommu group and T2 is trying to hot unplug a device or release [1] VF
-	 * of a PCIe device which is in the same iommu group. T1 takes group
-	 * mutex and before it could take device lock assume T2 has taken device
-	 * lock and is yet to take group mutex. Now, both the threads will be
-	 * waiting for the other thread to release lock. Below, lock order was
-	 * suggested.
-	 * device_lock(dev);
-	 *	mutex_lock(&group->mutex);
-	 *		iommu_change_dev_def_domain();
-	 *	mutex_unlock(&group->mutex);
-	 * device_unlock(dev);
-	 *
-	 * [1] Typical device release path
-	 * device_lock() from device/driver core code
-	 *  -> bus_notifier()
-	 *   -> iommu_bus_notifier()
-	 *    -> iommu_release_device()
-	 *     -> ops->release_device() vendor driver calls back iommu core code
-	 *      -> mutex_lock() from iommu core code
-	 */
-	mutex_unlock(&group->mutex);
-
-	/* Check if the device in the group still has a driver bound to it */
-	device_lock(dev);
-	if (device_is_bound(dev) && !(req_type == IOMMU_DOMAIN_DMA_FQ &&
-	    group->default_domain->type == IOMMU_DOMAIN_DMA)) {
-		pr_err_ratelimited("Device is still bound to driver\n");
-		ret = -EBUSY;
-		goto out;
+	if (!dev || group->owner_cnt) {
+		mutex_unlock(&group->mutex);
+		return -EPERM;
 	}
 
-	mutex_lock(&group->mutex);
 	ret = iommu_change_dev_def_domain(group, dev, req_type);
+
 	/*
 	 * Release the mutex here because ops->probe_finalize() call-back of
 	 * some vendor IOMMU drivers calls arm_iommu_attach_device() which
@@ -3088,14 +3047,9 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
 
 	/* Make sure dma_ops is appropriatley set */
 	if (!ret)
-		iommu_group_do_probe_finalize(dev, group->default_domain);
-	ret = ret ?: count;
-
-out:
-	device_unlock(dev);
-	put_device(dev);
+		__iommu_group_dma_finalize(group);
 
-	return ret;
+	return ret ?: count;
 }
 
 static bool iommu_is_default_domain(struct iommu_group *group)
-- 
2.34.1

[PATCH v2 6/6] iommu: Cleanup iommu_change_dev_def_domain()

[Lu Baolu]

As the singleton group limitation has been removed, cleanup the code
in iommu_change_dev_def_domain() accordingly.

Documentation is also updated.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c                         | 81 +++++--------------
 .../ABI/testing/sysfs-kernel-iommu_groups     |  1 -
 2 files changed, 21 insertions(+), 61 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 18dac155a178..c06757224bec 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2871,11 +2871,10 @@ int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features feat)
 EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
 
 /*
- * Changes the default domain of an iommu group that has *only* one device
+ * Changes the default domain of an iommu group
  *
  * @group: The group for which the default domain should be changed
- * @prev_dev: The device in the group (this is used to make sure that the device
- *	 hasn't changed after the caller has called this function)
+ * @dev: The first device in the group
  * @type: The type of the new default domain that gets associated with the group
  *
  * Returns 0 on success and error code on failure
@@ -2886,101 +2885,63 @@ EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
  *    Please take a closer look if intended to use for other purposes.
  */
 static int iommu_change_dev_def_domain(struct iommu_group *group,
-				       struct device *prev_dev, int type)
+				       struct device *dev, int type)
 {
+	struct __group_domain_type gtype = {NULL, 0};
 	struct iommu_domain *prev_dom;
-	int ret, dev_def_dom;
-	struct device *dev;
+	int ret;
 
 	lockdep_assert_held(&group->mutex);
 
-	if (group->default_domain != group->domain) {
-		dev_err_ratelimited(prev_dev, "Group not assigned to default domain\n");
-		ret = -EBUSY;
-		goto out;
-	}
-
-	/*
-	 * iommu group wasn't locked while acquiring device lock in
-	 * iommu_group_store_type(). So, make sure that the device count hasn't
-	 * changed while acquiring device lock.
-	 *
-	 * Changing default domain of an iommu group with two or more devices
-	 * isn't supported because there could be a potential deadlock. Consider
-	 * the following scenario. T1 is trying to acquire device locks of all
-	 * the devices in the group and before it could acquire all of them,
-	 * there could be another thread T2 (from different sub-system and use
-	 * case) that has already acquired some of the device locks and might be
-	 * waiting for T1 to release other device locks.
-	 */
-	if (iommu_group_device_count(group) != 1) {
-		dev_err_ratelimited(prev_dev, "Cannot change default domain: Group has more than one device\n");
-		ret = -EINVAL;
-		goto out;
-	}
-
-	/* Since group has only one device */
-	dev = iommu_group_first_dev(group);
-
-	if (prev_dev != dev) {
-		dev_err_ratelimited(prev_dev, "Cannot change default domain: Device has been changed\n");
-		ret = -EBUSY;
-		goto out;
-	}
-
 	prev_dom = group->default_domain;
-	if (!prev_dom) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	dev_def_dom = iommu_get_def_domain_type(dev);
+	__iommu_group_for_each_dev(group, &gtype,
+				   probe_get_default_domain_type);
 	if (!type) {
 		/*
 		 * If the user hasn't requested any specific type of domain and
 		 * if the device supports both the domains, then default to the
 		 * domain the device was booted with
 		 */
-		type = dev_def_dom ? : iommu_def_domain_type;
-	} else if (dev_def_dom && type != dev_def_dom) {
-		dev_err_ratelimited(prev_dev, "Device cannot be in %s domain\n",
+		type = gtype.type ? : iommu_def_domain_type;
+	} else if (gtype.type && type != gtype.type) {
+		dev_err_ratelimited(dev, "Device cannot be in %s domain\n",
 				    iommu_domain_type_str(type));
-		ret = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
 	/*
 	 * Switch to a new domain only if the requested domain type is different
 	 * from the existing default domain type
 	 */
-	if (prev_dom->type == type) {
-		ret = 0;
-		goto out;
-	}
+	if (prev_dom->type == type)
+		return 0;
+
+	group->default_domain = NULL;
+	group->domain = NULL;
 
 	/* Sets group->default_domain to the newly allocated domain */
 	ret = iommu_group_alloc_default_domain(group, dev, type);
 	if (ret)
-		goto out;
+		goto restore_old_domain;
 
-	ret = iommu_create_device_direct_mappings(group, dev);
+	ret = iommu_group_create_direct_mappings(group);
 	if (ret)
 		goto free_new_domain;
 
-	ret = __iommu_attach_device(group->default_domain, dev);
+	ret = __iommu_attach_group(group->default_domain, group);
 	if (ret)
 		goto free_new_domain;
 
-	group->domain = group->default_domain;
 	iommu_domain_free(prev_dom);
 
 	return 0;
 
 free_new_domain:
 	iommu_domain_free(group->default_domain);
+restore_old_domain:
 	group->default_domain = prev_dom;
 	group->domain = prev_dom;
-out:
+
 	return ret;
 }
 
diff --git a/Documentation/ABI/testing/sysfs-kernel-iommu_groups b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
index b15af6a5bc08..a42d4383d999 100644
--- a/Documentation/ABI/testing/sysfs-kernel-iommu_groups
+++ b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
@@ -53,7 +53,6 @@ Description:	/sys/kernel/iommu_groups/<grp_id>/type shows the type of default
 
 		The default domain type of a group may be modified only when
 
-		- The group has only one device.
 		- The device in the group is not bound to any device driver.
 		  So, the users must unbind the appropriate driver before
 		  changing the default domain type.
-- 
2.34.1

Re: [PATCH v2 0/6] iommu: Extend changing default domain to normal group

[Jason Gunthorpe]

On Fri, Feb 17, 2023 at 05:47:30PM +0800, Lu Baolu wrote:
> The IOMMU group sysfs interface allows users to change the default
> domain of a group. The current implementation uses device_lock() to make
> sure that the devices in the group are not bound to any driver and won't
> be bound during the process of changing the default domain. In order to
> avoid a possible deadlock caused by lock order of device_lock and
> group->mutex, it limits the functionality to singleton groups only.
> 
> The recently implemented DMA ownership framework can be applied here to
> replace device_lock(). In addition, use group->mutex to ensure that the
> iommu ops of the devices is always valid during the process of changing
> default domain.
> 
> With above replacement and enhancement, the device_lock() could be
> removed and the singleton-group-only limitation could be removed.
> 
> The whole series is also available on github:
> https://github.com/LuBaolu/intel-iommu/commits/iommu-sysfs-default-domain-extension-v2
> 
> This series is on top of below series from Robin,
> https://lore.kernel.org/linux-iommu/cover.1674753627.git.robin.murphy@arm.com/
> 
> Please help to review and suggest.

Given the overall situation, I think my suggestion to use
arm_iommu_release_device() might be more short term practical.

Jason

Re: [PATCH v2 0/6] iommu: Extend changing default domain to normal group

[Baolu Lu]

On 2/17/23 11:47 PM, Jason Gunthorpe wrote:
> On Fri, Feb 17, 2023 at 05:47:30PM +0800, Lu Baolu wrote:
>> The IOMMU group sysfs interface allows users to change the default
>> domain of a group. The current implementation uses device_lock() to make
>> sure that the devices in the group are not bound to any driver and won't
>> be bound during the process of changing the default domain. In order to
>> avoid a possible deadlock caused by lock order of device_lock and
>> group->mutex, it limits the functionality to singleton groups only.
>>
>> The recently implemented DMA ownership framework can be applied here to
>> replace device_lock(). In addition, use group->mutex to ensure that the
>> iommu ops of the devices is always valid during the process of changing
>> default domain.
>>
>> With above replacement and enhancement, the device_lock() could be
>> removed and the singleton-group-only limitation could be removed.
>>
>> The whole series is also available on github:
>> https://github.com/LuBaolu/intel-iommu/commits/iommu-sysfs-default-domain-extension-v2
>>
>> This series is on top of below series from Robin,
>> https://lore.kernel.org/linux-iommu/cover.1674753627.git.robin.murphy@arm.com/
>>
>> Please help to review and suggest.
> Given the overall situation, I think my suggestion to use
> arm_iommu_release_device() might be more short term practical.

Yes. Fair enough.

Best regards,
baolu