[PATCH 0/3] iommu/vt-d: Use ida for domain ID management

[PATCH 0/3] iommu/vt-d: Use ida for domain ID management

[Lu Baolu]

This converts the Intel iommu driver's domain ID management from a
fixed-size bitmap to the dynamic ida allocator. This improves memory
efficiency by only allocating resources for the domain IDs actually in
use, rather than the maximum possible number.

The also includes necessary cleanups after the ida conversion, including
locking adjustment for the ida and code simplification.

Please review and comment.

Lu Baolu (3):
  iommu/vt-d: Use ida to manage domain id
  iommu/vt-d: Replace spin_lock with mutex to protect domain ida
  iommu/vt-d: Simplify domain_attach_iommu()

 drivers/iommu/intel/dmar.c  |   3 +
 drivers/iommu/intel/iommu.c | 113 ++++++++----------------------------
 drivers/iommu/intel/iommu.h |  20 +++++--
 3 files changed, 44 insertions(+), 92 deletions(-)

-- 
2.43.0

[PATCH 1/3] iommu/vt-d: Use ida to manage domain id

[Lu Baolu]

Switch the intel iommu driver to use the ida mechanism for managing domain
IDs, replacing the previous fixed-size bitmap.

The previous approach allocated a bitmap large enough to cover the maximum
number of domain IDs supported by the hardware, regardless of the actual
number of domains in use. This led to unnecessary memory consumption,
especially on systems supporting a large number of iommu units but only
utilizing a small number of domain IDs.

The ida allocator dynamically manages the allocation and freeing of integer
IDs, only consuming memory for the IDs that are currently in use. This
significantly optimizes memory usage compared to the fixed-size bitmap.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/dmar.c  |  2 +
 drivers/iommu/intel/iommu.c | 80 ++++++++-----------------------------
 drivers/iommu/intel/iommu.h | 18 +++++++--
 3 files changed, 33 insertions(+), 67 deletions(-)

diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
index e540092d664d..0afba7434a5c 100644
--- a/drivers/iommu/intel/dmar.c
+++ b/drivers/iommu/intel/dmar.c
@@ -1099,6 +1099,8 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)
 	spin_lock_init(&iommu->device_rbtree_lock);
 	mutex_init(&iommu->iopf_lock);
 	iommu->node = NUMA_NO_NODE;
+	spin_lock_init(&iommu->lock);
+	ida_init(&iommu->domain_ida);
 
 	ver = readl(iommu->reg + DMAR_VER_REG);
 	pr_info("%s: reg_base_addr %llx ver %d:%d cap %llx ecap %llx\n",
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index b29da2d96d0b..76e170922149 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1289,52 +1289,13 @@ static void iommu_disable_translation(struct intel_iommu *iommu)
 	raw_spin_unlock_irqrestore(&iommu->register_lock, flag);
 }
 
-static int iommu_init_domains(struct intel_iommu *iommu)
-{
-	u32 ndomains;
-
-	ndomains = cap_ndoms(iommu->cap);
-	pr_debug("%s: Number of Domains supported <%d>\n",
-		 iommu->name, ndomains);
-
-	spin_lock_init(&iommu->lock);
-
-	iommu->domain_ids = bitmap_zalloc(ndomains, GFP_KERNEL);
-	if (!iommu->domain_ids)
-		return -ENOMEM;
-
-	/*
-	 * If Caching mode is set, then invalid translations are tagged
-	 * with domain-id 0, hence we need to pre-allocate it. We also
-	 * use domain-id 0 as a marker for non-allocated domain-id, so
-	 * make sure it is not used for a real domain.
-	 */
-	set_bit(0, iommu->domain_ids);
-
-	/*
-	 * Vt-d spec rev3.0 (section 6.2.3.1) requires that each pasid
-	 * entry for first-level or pass-through translation modes should
-	 * be programmed with a domain id different from those used for
-	 * second-level or nested translation. We reserve a domain id for
-	 * this purpose. This domain id is also used for identity domain
-	 * in legacy mode.
-	 */
-	set_bit(FLPT_DEFAULT_DID, iommu->domain_ids);
-
-	return 0;
-}
-
 static void disable_dmar_iommu(struct intel_iommu *iommu)
 {
-	if (!iommu->domain_ids)
-		return;
-
 	/*
 	 * All iommu domains must have been detached from the devices,
 	 * hence there should be no domain IDs in use.
 	 */
-	if (WARN_ON(bitmap_weight(iommu->domain_ids, cap_ndoms(iommu->cap))
-		    > NUM_RESERVED_DID))
+	if (WARN_ON(!ida_is_empty(&iommu->domain_ida)))
 		return;
 
 	if (iommu->gcmd & DMA_GCMD_TE)
@@ -1343,10 +1304,7 @@ static void disable_dmar_iommu(struct intel_iommu *iommu)
 
 static void free_dmar_iommu(struct intel_iommu *iommu)
 {
-	if (iommu->domain_ids) {
-		bitmap_free(iommu->domain_ids);
-		iommu->domain_ids = NULL;
-	}
+	ida_destroy(&iommu->domain_ida);
 
 	if (iommu->copied_tables) {
 		bitmap_free(iommu->copied_tables);
@@ -1380,7 +1338,6 @@ static bool first_level_by_default(struct intel_iommu *iommu)
 int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
 {
 	struct iommu_domain_info *info, *curr;
-	unsigned long ndomains;
 	int num, ret = -ENOSPC;
 
 	if (domain->domain.type == IOMMU_DOMAIN_SVA)
@@ -1399,14 +1356,13 @@ int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
 		return 0;
 	}
 
-	ndomains = cap_ndoms(iommu->cap);
-	num = find_first_zero_bit(iommu->domain_ids, ndomains);
-	if (num >= ndomains) {
+	num = ida_alloc_range(&iommu->domain_ida, FLPT_DEFAULT_DID + 1,
+			      cap_ndoms(iommu->cap) - 1, GFP_ATOMIC);
+	if (num < 0) {
 		pr_err("%s: No free domain ids\n", iommu->name);
 		goto err_unlock;
 	}
 
-	set_bit(num, iommu->domain_ids);
 	info->refcnt	= 1;
 	info->did	= num;
 	info->iommu	= iommu;
@@ -1421,7 +1377,7 @@ int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
 	return 0;
 
 err_clear:
-	clear_bit(info->did, iommu->domain_ids);
+	ida_free(&iommu->domain_ida, info->did);
 err_unlock:
 	spin_unlock(&iommu->lock);
 	kfree(info);
@@ -1438,7 +1394,7 @@ void domain_detach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
 	spin_lock(&iommu->lock);
 	info = xa_load(&domain->iommu_array, iommu->seq_id);
 	if (--info->refcnt == 0) {
-		clear_bit(info->did, iommu->domain_ids);
+		ida_free(&iommu->domain_ida, info->did);
 		xa_erase(&domain->iommu_array, iommu->seq_id);
 		domain->nid = NUMA_NO_NODE;
 		kfree(info);
@@ -2042,7 +1998,7 @@ static int copy_context_table(struct intel_iommu *iommu,
 
 		did = context_domain_id(&ce);
 		if (did >= 0 && did < cap_ndoms(iommu->cap))
-			set_bit(did, iommu->domain_ids);
+			ida_alloc_range(&iommu->domain_ida, did, did + 1, GFP_KERNEL);
 
 		set_context_copied(iommu, bus, devfn);
 		new_ce[idx] = ce;
@@ -2169,11 +2125,6 @@ static int __init init_dmars(void)
 		}
 
 		intel_iommu_init_qi(iommu);
-
-		ret = iommu_init_domains(iommu);
-		if (ret)
-			goto free_iommu;
-
 		init_translation_status(iommu);
 
 		if (translation_pre_enabled(iommu) && !is_kdump_kernel()) {
@@ -2651,9 +2602,7 @@ static int intel_iommu_add(struct dmar_drhd_unit *dmaru)
 	if (iommu->gcmd & DMA_GCMD_TE)
 		iommu_disable_translation(iommu);
 
-	ret = iommu_init_domains(iommu);
-	if (ret == 0)
-		ret = iommu_alloc_root_entry(iommu);
+	ret = iommu_alloc_root_entry(iommu);
 	if (ret)
 		goto out;
 
@@ -2972,9 +2921,14 @@ static ssize_t domains_used_show(struct device *dev,
 				 struct device_attribute *attr, char *buf)
 {
 	struct intel_iommu *iommu = dev_to_intel_iommu(dev);
-	return sysfs_emit(buf, "%d\n",
-			  bitmap_weight(iommu->domain_ids,
-					cap_ndoms(iommu->cap)));
+	unsigned int count = 0;
+	int id;
+
+	for (id = 0; id < cap_ndoms(iommu->cap); id++)
+		if (ida_exists(&iommu->domain_ida, id))
+			count++;
+
+	return sysfs_emit(buf, "%d\n", count);
 }
 static DEVICE_ATTR_RO(domains_used);
 
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index c4916886da5a..993232292400 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -722,7 +722,7 @@ struct intel_iommu {
 	unsigned char	name[16];    /* Device Name */
 
 #ifdef CONFIG_INTEL_IOMMU
-	unsigned long 	*domain_ids; /* bitmap of domains */
+	struct ida	domain_ida; /* domain id allocator */
 	unsigned long	*copied_tables; /* bitmap of copied tables */
 	spinlock_t	lock; /* protect context, domain ids */
 	struct root_entry *root_entry; /* virtual address */
@@ -809,11 +809,21 @@ static inline struct dmar_domain *to_dmar_domain(struct iommu_domain *dom)
 }
 
 /*
- * Domain ID reserved for pasid entries programmed for first-level
- * only and pass-through transfer modes.
+ * Domain ID 0 and 1 are reserved:
+ *
+ * If Caching mode is set, then invalid translations are tagged
+ * with domain-id 0, hence we need to pre-allocate it. We also
+ * use domain-id 0 as a marker for non-allocated domain-id, so
+ * make sure it is not used for a real domain.
+ *
+ * Vt-d spec rev3.0 (section 6.2.3.1) requires that each pasid
+ * entry for first-level or pass-through translation modes should
+ * be programmed with a domain id different from those used for
+ * second-level or nested translation. We reserve a domain id for
+ * this purpose. This domain id is also used for identity domain
+ * in legacy mode.
  */
 #define FLPT_DEFAULT_DID		1
-#define NUM_RESERVED_DID		2
 
 /* Retrieve the domain ID which has allocated to the domain */
 static inline u16
-- 
2.43.0

RE: [PATCH 1/3] iommu/vt-d: Use ida to manage domain id

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Wednesday, April 23, 2025 11:10 AM
> 
>  static void free_dmar_iommu(struct intel_iommu *iommu)
>  {
> -	if (iommu->domain_ids) {
> -		bitmap_free(iommu->domain_ids);
> -		iommu->domain_ids = NULL;
> -	}
> +	ida_destroy(&iommu->domain_ida);

since ida_init() is in alloc_iommu() now, the destroy can be
moved to free_iommu().

> @@ -1399,14 +1356,13 @@ int domain_attach_iommu(struct dmar_domain
> *domain, struct intel_iommu *iommu)
>  		return 0;
>  	}
> 
> -	ndomains = cap_ndoms(iommu->cap);
> -	num = find_first_zero_bit(iommu->domain_ids, ndomains);
> -	if (num >= ndomains) {
> +	num = ida_alloc_range(&iommu->domain_ida, FLPT_DEFAULT_DID +
> 1,
> +			      cap_ndoms(iommu->cap) - 1, GFP_ATOMIC);

let's define a macro e.g. FIRST_DID for min.

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

Re: [PATCH 1/3] iommu/vt-d: Use ida to manage domain id

[Baolu Lu]

On 4/24/2025 3:37 PM, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Wednesday, April 23, 2025 11:10 AM
>>
>>   static void free_dmar_iommu(struct intel_iommu *iommu)
>>   {
>> -	if (iommu->domain_ids) {
>> -		bitmap_free(iommu->domain_ids);
>> -		iommu->domain_ids = NULL;
>> -	}
>> +	ida_destroy(&iommu->domain_ida);
> 
> since ida_init() is in alloc_iommu() now, the destroy can be
> moved to free_iommu().

Sure.

diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
index e25e8d3dedcb..9e17e8e56308 100644
--- a/drivers/iommu/intel/dmar.c
+++ b/drivers/iommu/intel/dmar.c
@@ -1198,6 +1198,7 @@ static void free_iommu(struct intel_iommu *iommu)
         if (iommu->reg)
                 unmap_iommu(iommu);

+       ida_destroy(&iommu->domain_ida);
         ida_free(&dmar_seq_ids, iommu->seq_id);
         kfree(iommu);
  }
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index f784df122d7a..0002511c561f 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1304,8 +1304,6 @@ static void disable_dmar_iommu(struct intel_iommu 
*iommu)

  static void free_dmar_iommu(struct intel_iommu *iommu)
  {
-       ida_destroy(&iommu->domain_ida);
-
         if (iommu->copied_tables) {
                 bitmap_free(iommu->copied_tables);
                 iommu->copied_tables = NULL;

> 
>> @@ -1399,14 +1356,13 @@ int domain_attach_iommu(struct dmar_domain
>> *domain, struct intel_iommu *iommu)
>>   		return 0;
>>   	}
>>
>> -	ndomains = cap_ndoms(iommu->cap);
>> -	num = find_first_zero_bit(iommu->domain_ids, ndomains);
>> -	if (num >= ndomains) {
>> +	num = ida_alloc_range(&iommu->domain_ida, FLPT_DEFAULT_DID +
>> 1,
>> +			      cap_ndoms(iommu->cap) - 1, GFP_ATOMIC);
> 
> let's define a macro e.g. FIRST_DID for min.

Okay, how about IDA_START_DID?

Thanks,
baolu

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

Re: [PATCH 1/3] iommu/vt-d: Use ida to manage domain id

[Baolu Lu]

On 4/23/2025 11:10 AM, Lu Baolu wrote:
> @@ -2042,7 +1998,7 @@ static int copy_context_table(struct intel_iommu *iommu,
>   
>   		did = context_domain_id(&ce);
>   		if (did >= 0 && did < cap_ndoms(iommu->cap))
> -			set_bit(did, iommu->domain_ids);
> +			ida_alloc_range(&iommu->domain_ida, did, did + 1, GFP_KERNEL);

ida allocation range is inclusive, thus here it should be,

ida_alloc_range(&iommu->domain_ida, did, did, GFP_KERNEL);

>   
>   		set_context_copied(iommu, bus, devfn);
>   		new_ce[idx] = ce;

Thanks,
baolu

[PATCH 2/3] iommu/vt-d: Replace spin_lock with mutex to protect domain ida

[Lu Baolu]

The domain ID allocator is currently protected by a spin_lock. However,
ida_alloc_range can potentially block if it needs to allocate memory to
grow its internal structures.

Replace the spin_lock with a mutex which allows sleep on block. Thus,
the memory allocation flags can be updated from GFP_ATOMIC to GFP_KERNEL
to allow blocking memory allocations if necessary.

Introduce a new mutex, did_lock, specifically for protecting the domain
ida. The existing spinlock will remain for protecting other intel_iommu
fields.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/dmar.c  |  1 +
 drivers/iommu/intel/iommu.c | 12 ++++--------
 drivers/iommu/intel/iommu.h |  2 ++
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
index 0afba7434a5c..e25e8d3dedcb 100644
--- a/drivers/iommu/intel/dmar.c
+++ b/drivers/iommu/intel/dmar.c
@@ -1101,6 +1101,7 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)
 	iommu->node = NUMA_NO_NODE;
 	spin_lock_init(&iommu->lock);
 	ida_init(&iommu->domain_ida);
+	mutex_init(&iommu->did_lock);
 
 	ver = readl(iommu->reg + DMAR_VER_REG);
 	pr_info("%s: reg_base_addr %llx ver %d:%d cap %llx ecap %llx\n",
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 76e170922149..3a9ea0ad2cd3 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1347,17 +1347,16 @@ int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
 	if (!info)
 		return -ENOMEM;
 
-	spin_lock(&iommu->lock);
+	guard(mutex)(&iommu->did_lock);
 	curr = xa_load(&domain->iommu_array, iommu->seq_id);
 	if (curr) {
 		curr->refcnt++;
-		spin_unlock(&iommu->lock);
 		kfree(info);
 		return 0;
 	}
 
 	num = ida_alloc_range(&iommu->domain_ida, FLPT_DEFAULT_DID + 1,
-			      cap_ndoms(iommu->cap) - 1, GFP_ATOMIC);
+			      cap_ndoms(iommu->cap) - 1, GFP_KERNEL);
 	if (num < 0) {
 		pr_err("%s: No free domain ids\n", iommu->name);
 		goto err_unlock;
@@ -1367,19 +1366,17 @@ int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
 	info->did	= num;
 	info->iommu	= iommu;
 	curr = xa_cmpxchg(&domain->iommu_array, iommu->seq_id,
-			  NULL, info, GFP_ATOMIC);
+			  NULL, info, GFP_KERNEL);
 	if (curr) {
 		ret = xa_err(curr) ? : -EBUSY;
 		goto err_clear;
 	}
 
-	spin_unlock(&iommu->lock);
 	return 0;
 
 err_clear:
 	ida_free(&iommu->domain_ida, info->did);
 err_unlock:
-	spin_unlock(&iommu->lock);
 	kfree(info);
 	return ret;
 }
@@ -1391,7 +1388,7 @@ void domain_detach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
 	if (domain->domain.type == IOMMU_DOMAIN_SVA)
 		return;
 
-	spin_lock(&iommu->lock);
+	guard(mutex)(&iommu->did_lock);
 	info = xa_load(&domain->iommu_array, iommu->seq_id);
 	if (--info->refcnt == 0) {
 		ida_free(&iommu->domain_ida, info->did);
@@ -1399,7 +1396,6 @@ void domain_detach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
 		domain->nid = NUMA_NO_NODE;
 		kfree(info);
 	}
-	spin_unlock(&iommu->lock);
 }
 
 static void domain_exit(struct dmar_domain *domain)
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 993232292400..1e98c10ca553 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -722,6 +722,8 @@ struct intel_iommu {
 	unsigned char	name[16];    /* Device Name */
 
 #ifdef CONFIG_INTEL_IOMMU
+	/* mutex to protect domain_ida */
+	struct mutex	did_lock;
 	struct ida	domain_ida; /* domain id allocator */
 	unsigned long	*copied_tables; /* bitmap of copied tables */
 	spinlock_t	lock; /* protect context, domain ids */
-- 
2.43.0

RE: [PATCH 2/3] iommu/vt-d: Replace spin_lock with mutex to protect domain ida

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Wednesday, April 23, 2025 11:10 AM
> 
> The domain ID allocator is currently protected by a spin_lock. However,
> ida_alloc_range can potentially block if it needs to allocate memory to
> grow its internal structures.
> 
> Replace the spin_lock with a mutex which allows sleep on block. Thus,
> the memory allocation flags can be updated from GFP_ATOMIC to
> GFP_KERNEL
> to allow blocking memory allocations if necessary.
> 
> Introduce a new mutex, did_lock, specifically for protecting the domain
> ida. The existing spinlock will remain for protecting other intel_iommu
> fields.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>

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

[PATCH 3/3] iommu/vt-d: Simplify domain_attach_iommu()

[Lu Baolu]

Use the __free(kfree) attribute with kzalloc() to automatically handle
the freeing of the allocated struct iommu_domain_info on error or early
exit paths, eliminating the need for explicit kfree() calls in error
handling branches.

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 3a9ea0ad2cd3..12382c85495f 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1337,13 +1337,14 @@ static bool first_level_by_default(struct intel_iommu *iommu)
 
 int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
 {
-	struct iommu_domain_info *info, *curr;
-	int num, ret = -ENOSPC;
+	struct iommu_domain_info *curr;
+	int num;
 
 	if (domain->domain.type == IOMMU_DOMAIN_SVA)
 		return 0;
 
-	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	struct iommu_domain_info *info __free(kfree) =
+		kzalloc(sizeof(*info), GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
 
@@ -1351,34 +1352,20 @@ int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
 	curr = xa_load(&domain->iommu_array, iommu->seq_id);
 	if (curr) {
 		curr->refcnt++;
-		kfree(info);
 		return 0;
 	}
 
 	num = ida_alloc_range(&iommu->domain_ida, FLPT_DEFAULT_DID + 1,
 			      cap_ndoms(iommu->cap) - 1, GFP_KERNEL);
-	if (num < 0) {
-		pr_err("%s: No free domain ids\n", iommu->name);
-		goto err_unlock;
-	}
+	if (num < 0)
+		return num;
 
 	info->refcnt	= 1;
 	info->did	= num;
 	info->iommu	= iommu;
-	curr = xa_cmpxchg(&domain->iommu_array, iommu->seq_id,
-			  NULL, info, GFP_KERNEL);
-	if (curr) {
-		ret = xa_err(curr) ? : -EBUSY;
-		goto err_clear;
-	}
 
-	return 0;
-
-err_clear:
-	ida_free(&iommu->domain_ida, info->did);
-err_unlock:
-	kfree(info);
-	return ret;
+	return xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
+			       no_free_ptr(info), GFP_KERNEL));
 }
 
 void domain_detach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
-- 
2.43.0

RE: [PATCH 3/3] iommu/vt-d: Simplify domain_attach_iommu()

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Wednesday, April 23, 2025 11:10 AM
> 
>  	num = ida_alloc_range(&iommu->domain_ida, FLPT_DEFAULT_DID +
> 1,
>  			      cap_ndoms(iommu->cap) - 1, GFP_KERNEL);
> -	if (num < 0) {
> -		pr_err("%s: No free domain ids\n", iommu->name);

this error message could be kept.

> -		goto err_unlock;
> -	}
> +	if (num < 0)
> +		return num;
> 
>  	info->refcnt	= 1;
>  	info->did	= num;
>  	info->iommu	= iommu;
> -	curr = xa_cmpxchg(&domain->iommu_array, iommu->seq_id,
> -			  NULL, info, GFP_KERNEL);
> -	if (curr) {
> -		ret = xa_err(curr) ? : -EBUSY;
> -		goto err_clear;
> -	}
> 
> -	return 0;
> -
> -err_clear:
> -	ida_free(&iommu->domain_ida, info->did);
> -err_unlock:
> -	kfree(info);
> -	return ret;
> +	return xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
> +			       no_free_ptr(info), GFP_KERNEL));
>  }

no_free_ptr() should be used before successful return. Here xa_store()
could return error but at that point no auto free as no_free_ptr() already
changes 'info' to NULL. then memory leak.

Re: [PATCH 3/3] iommu/vt-d: Simplify domain_attach_iommu()

[Baolu Lu]

On 4/24/2025 3:46 PM, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Wednesday, April 23, 2025 11:10 AM
>>
>>   	num = ida_alloc_range(&iommu->domain_ida, FLPT_DEFAULT_DID +
>> 1,
>>   			      cap_ndoms(iommu->cap) - 1, GFP_KERNEL);
>> -	if (num < 0) {
>> -		pr_err("%s: No free domain ids\n", iommu->name);
> 
> this error message could be kept.

Okay.

> 
>> -		goto err_unlock;
>> -	}
>> +	if (num < 0)
>> +		return num;
>>
>>   	info->refcnt	= 1;
>>   	info->did	= num;
>>   	info->iommu	= iommu;
>> -	curr = xa_cmpxchg(&domain->iommu_array, iommu->seq_id,
>> -			  NULL, info, GFP_KERNEL);
>> -	if (curr) {
>> -		ret = xa_err(curr) ? : -EBUSY;
>> -		goto err_clear;
>> -	}
>>
>> -	return 0;
>> -
>> -err_clear:
>> -	ida_free(&iommu->domain_ida, info->did);
>> -err_unlock:
>> -	kfree(info);
>> -	return ret;
>> +	return xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
>> +			       no_free_ptr(info), GFP_KERNEL));
>>   }
> 
> no_free_ptr() should be used before successful return. Here xa_store()
> could return error but at that point no auto free as no_free_ptr() already
> changes 'info' to NULL. then memory leak.
Hmm, I've considered this. My thought was that xa_store() failure only
occurs due to the system running out of memory, and the Linux kernel
can't recover from it. In that case, the system is already broken;
hence, handling the failure case here doesn't make things better.

Thanks,
baolu

Re: [PATCH 3/3] iommu/vt-d: Simplify domain_attach_iommu()

[Jason Gunthorpe]

On Thu, Apr 24, 2025 at 05:22:48PM +0800, Baolu Lu wrote:

> > > -err_clear:
> > > -	ida_free(&iommu->domain_ida, info->did);
> > > -err_unlock:
> > > -	kfree(info);
> > > -	return ret;
> > > +	return xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
> > > +			       no_free_ptr(info), GFP_KERNEL));
> > >   }
> > 
> > no_free_ptr() should be used before successful return. Here xa_store()
> > could return error but at that point no auto free as no_free_ptr() already
> > changes 'info' to NULL. then memory leak.
>
> Hmm, I've considered this. My thought was that xa_store() failure only
> occurs due to the system running out of memory, and the Linux kernel
> can't recover from it. In that case, the system is already broken;
> hence, handling the failure case here doesn't make things better.

That's not the kernel pattern, you are supposed to unwind correctly in
those failures

I think you should not use cleanup.h for something complicated like
this..

Jason

Re: [PATCH 3/3] iommu/vt-d: Simplify domain_attach_iommu()

[Baolu Lu]

On 4/24/2025 9:37 PM, Jason Gunthorpe wrote:
> On Thu, Apr 24, 2025 at 05:22:48PM +0800, Baolu Lu wrote:
> 
>>>> -err_clear:
>>>> -	ida_free(&iommu->domain_ida, info->did);
>>>> -err_unlock:
>>>> -	kfree(info);
>>>> -	return ret;
>>>> +	return xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
>>>> +			       no_free_ptr(info), GFP_KERNEL));
>>>>    }
>>> no_free_ptr() should be used before successful return. Here xa_store()
>>> could return error but at that point no auto free as no_free_ptr() already
>>> changes 'info' to NULL. then memory leak.
>> Hmm, I've considered this. My thought was that xa_store() failure only
>> occurs due to the system running out of memory, and the Linux kernel
>> can't recover from it. In that case, the system is already broken;
>> hence, handling the failure case here doesn't make things better.
> That's not the kernel pattern, you are supposed to unwind correctly in
> those failures
> 
> I think you should not use cleanup.h for something complicated like
> this..

Okay, so let me drop this patch.

Thanks,
baolu

Re: [PATCH 3/3] iommu/vt-d: Simplify domain_attach_iommu()

[Dan Williams]

Lu Baolu wrote:
> Use the __free(kfree) attribute with kzalloc() to automatically handle
> the freeing of the allocated struct iommu_domain_info on error or early
> exit paths, eliminating the need for explicit kfree() calls in error
> handling branches.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 29 ++++++++---------------------
>  1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 3a9ea0ad2cd3..12382c85495f 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1337,13 +1337,14 @@ static bool first_level_by_default(struct intel_iommu *iommu)
>  
>  int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
>  {
> -	struct iommu_domain_info *info, *curr;
> -	int num, ret = -ENOSPC;
> +	struct iommu_domain_info *curr;
> +	int num;
>  
>  	if (domain->domain.type == IOMMU_DOMAIN_SVA)
>  		return 0;
>  
> -	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	struct iommu_domain_info *info __free(kfree) =
> +		kzalloc(sizeof(*info), GFP_KERNEL);
>  	if (!info)
>  		return -ENOMEM;
>  
> @@ -1351,34 +1352,20 @@ int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
[..]
> -err_clear:
> -	ida_free(&iommu->domain_ida, info->did);
> -err_unlock:
> -	kfree(info);
> -	return ret;
> +	return xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
> +			       no_free_ptr(info), GFP_KERNEL));
>  }

This pattern looks like it wants a "xa_store_or_{reset,kfree}()" helper that
handles both canceling a scope based cleanup and taking responsibility for
error-exit-freeing @info in one statement.

I.e. this is similar to a:

	"return devm_add_action_or_reset(..., no_free_ptr(obj), ...)"

...pattern.

Re: [PATCH 3/3] iommu/vt-d: Simplify domain_attach_iommu()

[Baolu Lu]

On 4/26/25 02:49, Dan Williams wrote:
> Lu Baolu wrote:
>> Use the __free(kfree) attribute with kzalloc() to automatically handle
>> the freeing of the allocated struct iommu_domain_info on error or early
>> exit paths, eliminating the need for explicit kfree() calls in error
>> handling branches.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c | 29 ++++++++---------------------
>>   1 file changed, 8 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 3a9ea0ad2cd3..12382c85495f 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -1337,13 +1337,14 @@ static bool first_level_by_default(struct intel_iommu *iommu)
>>   
>>   int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
>>   {
>> -	struct iommu_domain_info *info, *curr;
>> -	int num, ret = -ENOSPC;
>> +	struct iommu_domain_info *curr;
>> +	int num;
>>   
>>   	if (domain->domain.type == IOMMU_DOMAIN_SVA)
>>   		return 0;
>>   
>> -	info = kzalloc(sizeof(*info), GFP_KERNEL);
>> +	struct iommu_domain_info *info __free(kfree) =
>> +		kzalloc(sizeof(*info), GFP_KERNEL);
>>   	if (!info)
>>   		return -ENOMEM;
>>   
>> @@ -1351,34 +1352,20 @@ int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
> [..]
>> -err_clear:
>> -	ida_free(&iommu->domain_ida, info->did);
>> -err_unlock:
>> -	kfree(info);
>> -	return ret;
>> +	return xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
>> +			       no_free_ptr(info), GFP_KERNEL));
>>   }
> 
> This pattern looks like it wants a "xa_store_or_{reset,kfree}()" helper that
> handles both canceling a scope based cleanup and taking responsibility for
> error-exit-freeing @info in one statement.
> 
> I.e. this is similar to a:
> 
> 	"return devm_add_action_or_reset(..., no_free_ptr(obj), ...)"
> 
> ...pattern.
> 

Yes. Perhaps adding a xa_store variant would be beneficial in all
places that require this pattern.

Something like this?

diff --git a/include/linux/xarray.h b/include/linux/xarray.h
index 78eede109b1a..efbdff7ebda4 100644
--- a/include/linux/xarray.h
+++ b/include/linux/xarray.h
@@ -626,6 +626,35 @@ static inline void *xa_store_irq(struct xarray *xa, 
unsigned long index,
         return curr;
  }

+/**
+ * xa_store_or_kfree() - Store this entry in the XArray.
+ * @xa: XArray.
+ * @index: Index into array.
+ * @entry: New entry.
+ * @gfp: Memory allocation flags.
+ *
+ * This function is like calling xa_store() except it kfrees the new
+ * entry if an error happened.
+ *
+ * Context: Process context. Any context. Takes and releases the xa_lock.
+ * May sleep if the @gfp flags permit.
+ * Return: The old entry at this index or xa_err() if an error happened.
+ */
+static inline void *xa_store_or_kfree(struct xarray *xa, unsigned long 
index,
+               void *entry, gfp_t gfp)
+{
+       void *curr;
+
+       xa_lock(xa);
+       curr = __xa_store(xa, index, entry, gfp);
+       xa_unlock(xa);
+
+       if (xa_err(curr))
+               kfree(entry);
+
+       return curr;
+}
+
  /**
   * xa_erase_bh() - Erase this entry from the XArray.
   * @xa: XArray.

Thanks,
baolu