[PATCH v3 00/10] iommu/amd: Improve domain allocator and device attach code path

[PATCH v3 00/10] iommu/amd: Improve domain allocator and device attach code path

[Vasant Hegde]

This series aims to improve domain allocator and attach device code path.
  - Replace custom domain ID allocator with IDA allocator.
  - Improve protection domain data structure
  - Improve attach device code path and replace dev_data spinlock with mutex

This series is on top of v6.12-rc3.

This is also available at github :
  https://github.com/AMDESE/linux-iommu/tree/iommu_rework_attach_dev_v3

Changes from v2 -> v3:
  - Rebased on top of v6.12-rc3
  - Added Reviewed-by tags
  - Updated patch description

V2 : https://lore.kernel.org/linux-iommu/20240910065812.6091-1-vasant.hegde@amd.com/T/#t

Changes from v1 -> v2:
  - Rebased on top of iommu/next
  - Minor fixes.


V1 : https://lore.kernel.org/linux-iommu/20240828134317.6239-1-vasant.hegde@amd.com/T/#t

-Vasant

Vasant Hegde (10):
  iommu/amd: Use ida interface to manage protection domain ID
  iommu/amd: Remove protection_domain.dev_cnt variable
  iommu/amd: xarray to track protection_domain->iommu list
  iommu/amd: Remove unused amd_iommus variable
  iommu/amd: Do not detach devices in domain free path
  iommu/amd: Reduce domain lock scope in attach device path
  iommu/amd: Rearrange attach device code
  iommu/amd: Convert dev_data lock from spinlock to mutex
  iommu/amd: Reorder attach device code
  iommu/amd: Improve amd_iommu_release_device()

 drivers/iommu/amd/amd_iommu.h       |   2 +
 drivers/iommu/amd/amd_iommu_types.h |  20 +-
 drivers/iommu/amd/init.c            |  35 +--
 drivers/iommu/amd/iommu.c           | 349 +++++++++++++---------------
 4 files changed, 188 insertions(+), 218 deletions(-)

-- 
2.31.1

[PATCH v3 01/10] iommu/amd: Use ida interface to manage protection domain ID

[Vasant Hegde]

Replace custom domain ID allocator with IDA interface.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd/amd_iommu.h       |  2 +
 drivers/iommu/amd/amd_iommu_types.h |  3 --
 drivers/iommu/amd/init.c            | 29 +++++----------
 drivers/iommu/amd/iommu.c           | 58 ++++++++++++++---------------
 4 files changed, 40 insertions(+), 52 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 6386fa4556d9..6f28f435ed62 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -46,6 +46,8 @@ extern int amd_iommu_gpt_level;
 extern unsigned long amd_iommu_pgsize_bitmap;
 
 /* Protection domain ops */
+int amd_iommu_pdom_id_alloc(int min, int max);
+void amd_iommu_pdom_id_destroy(void);
 struct protection_domain *protection_domain_alloc(unsigned int type, int nid);
 void protection_domain_free(struct protection_domain *domain);
 struct iommu_domain *amd_iommu_domain_alloc_sva(struct device *dev,
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 601fb4ee6900..d32a86434de3 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -912,9 +912,6 @@ struct unity_map_entry {
 /* size of the dma_ops aperture as power of 2 */
 extern unsigned amd_iommu_aperture_order;
 
-/* allocation bitmap for domain ids */
-extern unsigned long *amd_iommu_pd_alloc_bitmap;
-
 extern bool amd_iommu_force_isolation;
 
 /* Max levels of glxval supported */
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 43131c3a2172..a1156d928bc4 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -194,12 +194,6 @@ bool amd_iommu_force_isolation __read_mostly;
 
 unsigned long amd_iommu_pgsize_bitmap __ro_after_init = AMD_IOMMU_PGSIZES;
 
-/*
- * AMD IOMMU allows up to 2^16 different protection domains. This is a bitmap
- * to know which ones are already in use.
- */
-unsigned long *amd_iommu_pd_alloc_bitmap;
-
 enum iommu_init_state {
 	IOMMU_START_STATE,
 	IOMMU_IVRS_DETECTED,
@@ -1082,7 +1076,11 @@ static bool __copy_device_table(struct amd_iommu *iommu)
 		if (dte_v && dom_id) {
 			pci_seg->old_dev_tbl_cpy[devid].data[0] = old_devtb[devid].data[0];
 			pci_seg->old_dev_tbl_cpy[devid].data[1] = old_devtb[devid].data[1];
-			__set_bit(dom_id, amd_iommu_pd_alloc_bitmap);
+			if (amd_iommu_pdom_id_alloc(dom_id, dom_id) != dom_id) {
+				pr_err("Failed to reserve domain ID 0x%x\n", dom_id);
+				memunmap(old_devtb);
+				return false;
+			}
 			/* If gcr3 table existed, mask it out */
 			if (old_devtb[devid].data[0] & DTE_FLAG_GV) {
 				tmp = DTE_GCR3_VAL_B(~0ULL) << DTE_GCR3_SHIFT_B;
@@ -2994,9 +2992,7 @@ static bool __init check_ioapic_information(void)
 
 static void __init free_dma_resources(void)
 {
-	iommu_free_pages(amd_iommu_pd_alloc_bitmap,
-			 get_order(MAX_DOMAIN_ID / 8));
-	amd_iommu_pd_alloc_bitmap = NULL;
+	amd_iommu_pdom_id_destroy();
 
 	free_unity_maps();
 }
@@ -3064,19 +3060,14 @@ static int __init early_amd_iommu_init(void)
 	amd_iommu_target_ivhd_type = get_highest_supported_ivhd_type(ivrs_base);
 	DUMP_printk("Using IVHD type %#x\n", amd_iommu_target_ivhd_type);
 
-	/* Device table - directly used by all IOMMUs */
-	ret = -ENOMEM;
-
-	amd_iommu_pd_alloc_bitmap = iommu_alloc_pages(GFP_KERNEL,
-						      get_order(MAX_DOMAIN_ID / 8));
-	if (amd_iommu_pd_alloc_bitmap == NULL)
-		goto out;
-
 	/*
 	 * never allocate domain 0 because its used as the non-allocated and
 	 * error value placeholder
 	 */
-	__set_bit(0, amd_iommu_pd_alloc_bitmap);
+	if (amd_iommu_pdom_id_alloc(0, 0) != 0) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	/*
 	 * now the data structures are allocated and basically initialized
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 8364cd6fa47d..3a8a77bcd2c1 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -18,6 +18,7 @@
 #include <linux/scatterlist.h>
 #include <linux/dma-map-ops.h>
 #include <linux/dma-direct.h>
+#include <linux/idr.h>
 #include <linux/iommu-helper.h>
 #include <linux/delay.h>
 #include <linux/amd-iommu.h>
@@ -52,8 +53,6 @@
 #define HT_RANGE_START		(0xfd00000000ULL)
 #define HT_RANGE_END		(0xffffffffffULL)
 
-static DEFINE_SPINLOCK(pd_bitmap_lock);
-
 LIST_HEAD(ioapic_map);
 LIST_HEAD(hpet_map);
 LIST_HEAD(acpihid_map);
@@ -70,6 +69,12 @@ struct iommu_cmd {
 	u32 data[4];
 };
 
+/*
+ * AMD IOMMU allows up to 2^16 different protection domains. This is a bitmap
+ * to know which ones are already in use.
+ */
+static DEFINE_IDA(pdom_ids);
+
 struct kmem_cache *amd_iommu_irq_cache;
 
 static void detach_device(struct device *dev);
@@ -1640,31 +1645,19 @@ int amd_iommu_complete_ppr(struct device *dev, u32 pasid, int status, int tag)
  *
  ****************************************************************************/
 
-static u16 domain_id_alloc(void)
+int amd_iommu_pdom_id_alloc(int min, int max)
 {
-	unsigned long flags;
-	int id;
-
-	spin_lock_irqsave(&pd_bitmap_lock, flags);
-	id = find_first_zero_bit(amd_iommu_pd_alloc_bitmap, MAX_DOMAIN_ID);
-	BUG_ON(id == 0);
-	if (id > 0 && id < MAX_DOMAIN_ID)
-		__set_bit(id, amd_iommu_pd_alloc_bitmap);
-	else
-		id = 0;
-	spin_unlock_irqrestore(&pd_bitmap_lock, flags);
-
-	return id;
+	return ida_alloc_range(&pdom_ids, min, max, GFP_ATOMIC);
 }
 
-static void domain_id_free(int id)
+void amd_iommu_pdom_id_destroy(void)
 {
-	unsigned long flags;
+	ida_destroy(&pdom_ids);
+}
 
-	spin_lock_irqsave(&pd_bitmap_lock, flags);
-	if (id > 0 && id < MAX_DOMAIN_ID)
-		__clear_bit(id, amd_iommu_pd_alloc_bitmap);
-	spin_unlock_irqrestore(&pd_bitmap_lock, flags);
+static void pdom_id_free(int id)
+{
+	ida_free(&pdom_ids, id);
 }
 
 static void free_gcr3_tbl_level1(u64 *tbl)
@@ -1709,7 +1702,7 @@ static void free_gcr3_table(struct gcr3_tbl_info *gcr3_info)
 	gcr3_info->glx = 0;
 
 	/* Free per device domain ID */
-	domain_id_free(gcr3_info->domid);
+	pdom_id_free(gcr3_info->domid);
 
 	iommu_free_page(gcr3_info->gcr3_tbl);
 	gcr3_info->gcr3_tbl = NULL;
@@ -1736,6 +1729,7 @@ static int setup_gcr3_table(struct gcr3_tbl_info *gcr3_info,
 {
 	int levels = get_gcr3_levels(pasids);
 	int nid = iommu ? dev_to_node(&iommu->dev->dev) : NUMA_NO_NODE;
+	int domid;
 
 	if (levels > amd_iommu_max_glx_val)
 		return -EINVAL;
@@ -1744,11 +1738,14 @@ static int setup_gcr3_table(struct gcr3_tbl_info *gcr3_info,
 		return -EBUSY;
 
 	/* Allocate per device domain ID */
-	gcr3_info->domid = domain_id_alloc();
+	domid = amd_iommu_pdom_id_alloc(1, MAX_DOMAIN_ID - 1);
+	if (domid <= 0)
+		return -ENOSPC;
+	gcr3_info->domid = domid;
 
 	gcr3_info->gcr3_tbl = iommu_alloc_page_node(nid, GFP_ATOMIC);
 	if (gcr3_info->gcr3_tbl == NULL) {
-		domain_id_free(gcr3_info->domid);
+		pdom_id_free(domid);
 		return -ENOMEM;
 	}
 
@@ -2259,7 +2256,7 @@ void protection_domain_free(struct protection_domain *domain)
 	WARN_ON(!list_empty(&domain->dev_list));
 	if (domain->domain.type & __IOMMU_DOMAIN_PAGING)
 		free_io_pgtable_ops(&domain->iop.pgtbl.ops);
-	domain_id_free(domain->id);
+	pdom_id_free(domain->id);
 	kfree(domain);
 }
 
@@ -2267,15 +2264,16 @@ struct protection_domain *protection_domain_alloc(unsigned int type, int nid)
 {
 	struct io_pgtable_ops *pgtbl_ops;
 	struct protection_domain *domain;
-	int pgtable;
+	int pgtable, domid;
 
 	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
 	if (!domain)
 		return NULL;
 
-	domain->id = domain_id_alloc();
-	if (!domain->id)
+	domid = amd_iommu_pdom_id_alloc(1, MAX_DOMAIN_ID - 1);
+	if (domid <= 0)
 		goto err_free;
+	domain->id = domid;
 
 	spin_lock_init(&domain->lock);
 	INIT_LIST_HEAD(&domain->dev_list);
@@ -2319,7 +2317,7 @@ struct protection_domain *protection_domain_alloc(unsigned int type, int nid)
 
 	return domain;
 err_id:
-	domain_id_free(domain->id);
+	pdom_id_free(domain->id);
 err_free:
 	kfree(domain);
 	return NULL;
-- 
2.31.1

Re: [PATCH v3 01/10] iommu/amd: Use ida interface to manage protection domain ID

[Jason Gunthorpe]

On Wed, Oct 16, 2024 at 05:34:52AM +0000, Vasant Hegde wrote:
>  	/*
>  	 * never allocate domain 0 because its used as the non-allocated and
>  	 * error value placeholder
>  	 */
> -	__set_bit(0, amd_iommu_pd_alloc_bitmap);
> +	if (amd_iommu_pdom_id_alloc(0, 0) != 0) {

The IDA interfaces have a min, so you don't need to reserve 0, just
make min 1 in all calls, which is being done anyhow:

> +	domid = amd_iommu_pdom_id_alloc(1, MAX_DOMAIN_ID - 1);

Suggest to just drop allocating 0 and redo amd_iommu_pdom_id_alloc()
to have no arguments and set the min/max correctly internally to the
function.

Open code the special ida manipulation in __copy_device_table().

Jason

Re: [PATCH v3 01/10] iommu/amd: Use ida interface to manage protection domain ID

[Vasant Hegde]

Jason,


On 10/17/2024 6:44 PM, Jason Gunthorpe wrote:
> On Wed, Oct 16, 2024 at 05:34:52AM +0000, Vasant Hegde wrote:
>>  	/*
>>  	 * never allocate domain 0 because its used as the non-allocated and
>>  	 * error value placeholder
>>  	 */
>> -	__set_bit(0, amd_iommu_pd_alloc_bitmap);
>> +	if (amd_iommu_pdom_id_alloc(0, 0) != 0) {
> 
> The IDA interfaces have a min, so you don't need to reserve 0, just
> make min 1 in all calls, which is being done anyhow:
> 
>> +	domid = amd_iommu_pdom_id_alloc(1, MAX_DOMAIN_ID - 1);
> 
> Suggest to just drop allocating 0 and redo amd_iommu_pdom_id_alloc()
> to have no arguments and set the min/max correctly internally to the
> function.
> 
> Open code the special ida manipulation in __copy_device_table().

Sure. will fix it.

-Vasant

[PATCH v3 02/10] iommu/amd: Remove protection_domain.dev_cnt variable

[Vasant Hegde]

protection_domain->dev_list tracks list of attached devices to
domain. We can use list_* functions on dev_list to get device count.
Hence remove 'dev_cnt' variable.

No functional change intended.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Reviewed-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd/amd_iommu_types.h | 1 -
 drivers/iommu/amd/iommu.c           | 7 +------
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index d32a86434de3..418c14aa4ec9 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -578,7 +578,6 @@ struct protection_domain {
 	u16 id;			/* the domain id written to the device table */
 	enum protection_domain_mode pd_mode; /* Track page table type */
 	bool dirty_tracking;	/* dirty tracking is enabled in the domain */
-	unsigned dev_cnt;	/* devices assigned to this domain */
 	unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference count */
 
 	struct mmu_notifier mn;	/* mmu notifier for the SVA domain */
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 3a8a77bcd2c1..c9514284382e 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2033,7 +2033,6 @@ static int do_attach(struct iommu_dev_data *dev_data,
 
 	/* Do reference counting */
 	domain->dev_iommu[iommu->index] += 1;
-	domain->dev_cnt                 += 1;
 
 	/* Setup GCR3 table */
 	if (pdom_is_sva_capable(domain)) {
@@ -2066,7 +2065,6 @@ static void do_detach(struct iommu_dev_data *dev_data)
 
 	/* decrease reference counters - needs to happen after the flushes */
 	domain->dev_iommu[iommu->index] -= 1;
-	domain->dev_cnt                 -= 1;
 }
 
 /*
@@ -2239,16 +2237,13 @@ static void cleanup_domain(struct protection_domain *domain)
 
 	lockdep_assert_held(&domain->lock);
 
-	if (!domain->dev_cnt)
-		return;
-
 	while (!list_empty(&domain->dev_list)) {
 		entry = list_first_entry(&domain->dev_list,
 					 struct iommu_dev_data, list);
 		BUG_ON(!entry->domain);
 		do_detach(entry);
 	}
-	WARN_ON(domain->dev_cnt != 0);
+	WARN_ON(!list_empty(&domain->dev_list));
 }
 
 void protection_domain_free(struct protection_domain *domain)
-- 
2.31.1

Re: [PATCH v3 02/10] iommu/amd: Remove protection_domain.dev_cnt variable

[Jason Gunthorpe]

On Wed, Oct 16, 2024 at 05:34:53AM +0000, Vasant Hegde wrote:
> protection_domain->dev_list tracks list of attached devices to
> domain. We can use list_* functions on dev_list to get device count.
> Hence remove 'dev_cnt' variable.
> 
> No functional change intended.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Reviewed-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/amd/amd_iommu_types.h | 1 -
>  drivers/iommu/amd/iommu.c           | 7 +------
>  2 files changed, 1 insertion(+), 7 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

[PATCH v3 03/10] iommu/amd: xarray to track protection_domain->iommu list

[Vasant Hegde]

Use xarray to track IOMMU attached to protection domain instead of
static array of MAX_IOMMUS.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd/amd_iommu_types.h |  8 ++-
 drivers/iommu/amd/iommu.c           | 85 ++++++++++++++++++++++-------
 2 files changed, 72 insertions(+), 21 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 418c14aa4ec9..0f030a61ceb2 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -565,6 +565,12 @@ struct pdom_dev_data {
 	struct list_head list;
 };
 
+/* Keeps track of the IOMMUs attached to protection domain */
+struct pdom_iommu_info {
+	struct amd_iommu *iommu; /* IOMMUs attach to protection domain */
+	u32 refcnt;	/* Count of attached dev/pasid per domain/IOMMU */
+};
+
 /*
  * This structure contains generic data for  IOMMU protection domains
  * independent of their use.
@@ -578,7 +584,7 @@ struct protection_domain {
 	u16 id;			/* the domain id written to the device table */
 	enum protection_domain_mode pd_mode; /* Track page table type */
 	bool dirty_tracking;	/* dirty tracking is enabled in the domain */
-	unsigned dev_iommu[MAX_IOMMUS]; /* per-IOMMU reference count */
+	struct xarray iommu_array;	/* per-IOMMU reference count */
 
 	struct mmu_notifier mn;	/* mmu notifier for the SVA domain */
 	struct list_head dev_data_list; /* List of pdom_dev_data */
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index c9514284382e..76af6304cc1b 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1254,18 +1254,15 @@ static int iommu_completion_wait(struct amd_iommu *iommu)
 
 static void domain_flush_complete(struct protection_domain *domain)
 {
-	int i;
-
-	for (i = 0; i < amd_iommu_get_num_iommus(); ++i) {
-		if (domain && !domain->dev_iommu[i])
-			continue;
+	struct pdom_iommu_info *pdom_iommu_info;
+	unsigned long i;
 
-		/*
-		 * Devices of this domain are behind this IOMMU
-		 * We need to wait for completion of all commands.
-		 */
-		iommu_completion_wait(amd_iommus[i]);
-	}
+	/*
+	 * Devices of this domain are behind this IOMMU
+	 * We need to wait for completion of all commands.
+	 */
+	 xa_for_each(&domain->iommu_array, i, pdom_iommu_info)
+		iommu_completion_wait(pdom_iommu_info->iommu);
 }
 
 static int iommu_flush_dte(struct amd_iommu *iommu, u16 devid)
@@ -1447,21 +1444,20 @@ static int domain_flush_pages_v2(struct protection_domain *pdom,
 static int domain_flush_pages_v1(struct protection_domain *pdom,
 				 u64 address, size_t size)
 {
+	struct pdom_iommu_info *pdom_iommu_info;
 	struct iommu_cmd cmd;
-	int ret = 0, i;
+	int ret = 0;
+	unsigned long i;
 
 	build_inv_iommu_pages(&cmd, address, size,
 			      pdom->id, IOMMU_NO_PASID, false);
 
-	for (i = 0; i < amd_iommu_get_num_iommus(); ++i) {
-		if (!pdom->dev_iommu[i])
-			continue;
-
+	xa_for_each(&pdom->iommu_array, i, pdom_iommu_info) {
 		/*
 		 * Devices of this domain are behind this IOMMU
 		 * We need a TLB flush
 		 */
-		ret |= iommu_queue_command(amd_iommus[i], &cmd);
+		ret |= iommu_queue_command(pdom_iommu_info->iommu, &cmd);
 	}
 
 	return ret;
@@ -2016,6 +2012,50 @@ static void destroy_gcr3_table(struct iommu_dev_data *dev_data,
 	free_gcr3_table(gcr3_info);
 }
 
+static int pdom_attach_iommu(struct amd_iommu *iommu,
+			     struct protection_domain *pdom)
+{
+	struct pdom_iommu_info *pdom_iommu_info, *curr;
+
+	pdom_iommu_info = xa_load(&pdom->iommu_array, iommu->index);
+	if (pdom_iommu_info) {
+		pdom_iommu_info->refcnt++;
+		return 0;
+	}
+
+	pdom_iommu_info = kzalloc(sizeof(*pdom_iommu_info), GFP_ATOMIC);
+	if (!pdom_iommu_info)
+		return -ENOMEM;
+
+	pdom_iommu_info->iommu = iommu;
+	pdom_iommu_info->refcnt = 1;
+
+	curr = xa_cmpxchg(&pdom->iommu_array, iommu->index,
+			  NULL, pdom_iommu_info, GFP_ATOMIC);
+	if (curr) {
+		kfree(pdom_iommu_info);
+		return -ENOSPC;
+	}
+
+	return 0;
+}
+
+static void pdom_detach_iommu(struct amd_iommu *iommu,
+			      struct protection_domain *pdom)
+{
+	struct pdom_iommu_info *pdom_iommu_info;
+
+	pdom_iommu_info = xa_load(&pdom->iommu_array, iommu->index);
+	if (!pdom_iommu_info)
+		return;
+
+	pdom_iommu_info->refcnt--;
+	if (pdom_iommu_info->refcnt == 0) {
+		xa_erase(&pdom->iommu_array, iommu->index);
+		kfree(pdom_iommu_info);
+	}
+}
+
 static int do_attach(struct iommu_dev_data *dev_data,
 		     struct protection_domain *domain)
 {
@@ -2032,13 +2072,17 @@ static int do_attach(struct iommu_dev_data *dev_data,
 		cfg->amd.nid = dev_to_node(dev_data->dev);
 
 	/* Do reference counting */
-	domain->dev_iommu[iommu->index] += 1;
+	ret = pdom_attach_iommu(iommu, domain);
+	if (ret)
+		return ret;
 
 	/* Setup GCR3 table */
 	if (pdom_is_sva_capable(domain)) {
 		ret = init_gcr3_table(dev_data, domain);
-		if (ret)
+		if (ret) {
+			pdom_detach_iommu(iommu, domain);
 			return ret;
+		}
 	}
 
 	return ret;
@@ -2064,7 +2108,7 @@ static void do_detach(struct iommu_dev_data *dev_data)
 	list_del(&dev_data->list);
 
 	/* decrease reference counters - needs to happen after the flushes */
-	domain->dev_iommu[iommu->index] -= 1;
+	pdom_detach_iommu(iommu, domain);
 }
 
 /*
@@ -2273,6 +2317,7 @@ struct protection_domain *protection_domain_alloc(unsigned int type, int nid)
 	spin_lock_init(&domain->lock);
 	INIT_LIST_HEAD(&domain->dev_list);
 	INIT_LIST_HEAD(&domain->dev_data_list);
+	xa_init(&domain->iommu_array);
 	domain->iop.pgtbl.cfg.amd.nid = nid;
 
 	switch (type) {
-- 
2.31.1

Re: [PATCH v3 03/10] iommu/amd: xarray to track protection_domain->iommu list

[Jason Gunthorpe]

On Wed, Oct 16, 2024 at 05:34:54AM +0000, Vasant Hegde wrote:

> +	/*
> +	 * Devices of this domain are behind this IOMMU
> +	 * We need to wait for completion of all commands.
> +	 */
> +	 xa_for_each(&domain->iommu_array, i, pdom_iommu_info)
> +		iommu_completion_wait(pdom_iommu_info->iommu);

Please add lockdep assertions to all the functions touching the
xarray, the domain->lock must be held for this to be correct.

Some of these are many nested function calls deep from the lock, I
didn't notice a missed lock but I didn't check every path.

Other than that, the logic looks OK

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v3 03/10] iommu/amd: xarray to track protection_domain->iommu list

[Vasant Hegde]

Jason,


On 10/17/2024 6:49 PM, Jason Gunthorpe wrote:
> On Wed, Oct 16, 2024 at 05:34:54AM +0000, Vasant Hegde wrote:
> 
>> +	/*
>> +	 * Devices of this domain are behind this IOMMU
>> +	 * We need to wait for completion of all commands.
>> +	 */
>> +	 xa_for_each(&domain->iommu_array, i, pdom_iommu_info)
>> +		iommu_completion_wait(pdom_iommu_info->iommu);
> 
> Please add lockdep assertions to all the functions touching the
> xarray, the domain->lock must be held for this to be correct.
> 
> Some of these are many nested function calls deep from the lock, I
> didn't notice a missed lock but I didn't check every path.

Sure. Will audit the code path and will add lockdep.

-Vasant

[PATCH v3 04/10] iommu/amd: Remove unused amd_iommus variable

[Vasant Hegde]

protection_domain structure is updated to use xarray to track the IOMMUs
attached to the domain. Now domain flush code is not using amd_iommus.
Hence remove this unused variable.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd/amd_iommu_types.h | 6 ------
 drivers/iommu/amd/init.c            | 6 ------
 2 files changed, 12 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 0f030a61ceb2..ad6ce13c0b7b 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -877,12 +877,6 @@ extern struct list_head amd_iommu_pci_seg_list;
  */
 extern struct list_head amd_iommu_list;
 
-/*
- * Array with pointers to each IOMMU struct
- * The indices are referenced in the protection domains
- */
-extern struct amd_iommu *amd_iommus[MAX_IOMMUS];
-
 /*
  * Structure defining one entry in the device table
  */
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index a1156d928bc4..4c6ecef149fd 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -177,9 +177,6 @@ LIST_HEAD(amd_iommu_pci_seg_list);	/* list of all PCI segments */
 LIST_HEAD(amd_iommu_list);		/* list of all AMD IOMMUs in the
 					   system */
 
-/* Array to assign indices to IOMMUs*/
-struct amd_iommu *amd_iommus[MAX_IOMMUS];
-
 /* Number of IOMMUs present in the system */
 static int amd_iommus_present;
 
@@ -1742,9 +1739,6 @@ static int __init init_iommu_one(struct amd_iommu *iommu, struct ivhd_header *h,
 		return -ENOSYS;
 	}
 
-	/* Index is fine - add IOMMU to the array */
-	amd_iommus[iommu->index] = iommu;
-
 	/*
 	 * Copy data from ACPI table entry to the iommu struct
 	 */
-- 
2.31.1

Re: [PATCH v3 04/10] iommu/amd: Remove unused amd_iommus variable

[Jason Gunthorpe]

On Wed, Oct 16, 2024 at 05:34:55AM +0000, Vasant Hegde wrote:
> protection_domain structure is updated to use xarray to track the IOMMUs
> attached to the domain. Now domain flush code is not using amd_iommus.
> Hence remove this unused variable.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> Reviewed-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/amd/amd_iommu_types.h | 6 ------
>  drivers/iommu/amd/init.c            | 6 ------
>  2 files changed, 12 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

[PATCH v3 05/10] iommu/amd: Do not detach devices in domain free path

[Vasant Hegde]

All devices attached to a protection domain must be freed before
calling domain free. Hence do not try to free devices in domain
free path. Continue to throw warning if pdom->dev_list is not empty
so that any potential issues can be fixed.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd/iommu.c | 26 +-------------------------
 1 file changed, 1 insertion(+), 25 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 76af6304cc1b..d74d3b65c939 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2275,21 +2275,6 @@ static struct iommu_group *amd_iommu_device_group(struct device *dev)
  *
  *****************************************************************************/
 
-static void cleanup_domain(struct protection_domain *domain)
-{
-	struct iommu_dev_data *entry;
-
-	lockdep_assert_held(&domain->lock);
-
-	while (!list_empty(&domain->dev_list)) {
-		entry = list_first_entry(&domain->dev_list,
-					 struct iommu_dev_data, list);
-		BUG_ON(!entry->domain);
-		do_detach(entry);
-	}
-	WARN_ON(!list_empty(&domain->dev_list));
-}
-
 void protection_domain_free(struct protection_domain *domain)
 {
 	WARN_ON(!list_empty(&domain->dev_list));
@@ -2445,16 +2430,7 @@ amd_iommu_domain_alloc_user(struct device *dev, u32 flags,
 
 void amd_iommu_domain_free(struct iommu_domain *dom)
 {
-	struct protection_domain *domain;
-	unsigned long flags;
-
-	domain = to_pdomain(dom);
-
-	spin_lock_irqsave(&domain->lock, flags);
-
-	cleanup_domain(domain);
-
-	spin_unlock_irqrestore(&domain->lock, flags);
+	struct protection_domain *domain = to_pdomain(dom);
 
 	protection_domain_free(domain);
 }
-- 
2.31.1

Re: [PATCH v3 05/10] iommu/amd: Do not detach devices in domain free path

[Jason Gunthorpe]

On Wed, Oct 16, 2024 at 05:34:56AM +0000, Vasant Hegde wrote:
> All devices attached to a protection domain must be freed before
> calling domain free. Hence do not try to free devices in domain
> free path. Continue to throw warning if pdom->dev_list is not empty
> so that any potential issues can be fixed.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> Reviewed-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/amd/iommu.c | 26 +-------------------------
>  1 file changed, 1 insertion(+), 25 deletions(-)

Yes!

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

>  void protection_domain_free(struct protection_domain *domain)
>  {
>  	WARN_ON(!list_empty(&domain->dev_list));

Probably should also check that the new xarray is empty here too??
Otherwise it will leak struct pdom_iommu_info memory.

Jason

[PATCH v3 06/10] iommu/amd: Reduce domain lock scope in attach device path

[Vasant Hegde]

Currently attach device path takes protection domain lock followed by
dev_data lock. Most of the operations in this function is specific to
device data except pdom_attach_iommu() where it updates protection
domain structure. Hence reduce the scope of protection domain lock.

Note that this changes the locking order. Now it takes device lock
before taking domain lock (group->mutex -> dev_data->lock ->
pdom->lock). dev_data->lock is used only in device attachment path.
So changing order is fine. It will not create any issue.

Finally move numa node assignment to pdom_attach_iommu().

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu.c | 49 +++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 22 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index d74d3b65c939..a738d2d7f0c4 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2016,16 +2016,23 @@ static int pdom_attach_iommu(struct amd_iommu *iommu,
 			     struct protection_domain *pdom)
 {
 	struct pdom_iommu_info *pdom_iommu_info, *curr;
+	struct io_pgtable_cfg *cfg = &pdom->iop.pgtbl.cfg;
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&pdom->lock, flags);
 
 	pdom_iommu_info = xa_load(&pdom->iommu_array, iommu->index);
 	if (pdom_iommu_info) {
 		pdom_iommu_info->refcnt++;
-		return 0;
+		goto out_unlock;
 	}
 
 	pdom_iommu_info = kzalloc(sizeof(*pdom_iommu_info), GFP_ATOMIC);
-	if (!pdom_iommu_info)
-		return -ENOMEM;
+	if (!pdom_iommu_info) {
+		ret = -ENOMEM;
+		goto out_unlock;
+	}
 
 	pdom_iommu_info->iommu = iommu;
 	pdom_iommu_info->refcnt = 1;
@@ -2034,43 +2041,52 @@ static int pdom_attach_iommu(struct amd_iommu *iommu,
 			  NULL, pdom_iommu_info, GFP_ATOMIC);
 	if (curr) {
 		kfree(pdom_iommu_info);
-		return -ENOSPC;
+		ret = -ENOSPC;
+		goto out_unlock;
 	}
 
-	return 0;
+	/* Update NUMA Node ID */
+	if (cfg->amd.nid == NUMA_NO_NODE)
+		cfg->amd.nid = dev_to_node(&iommu->dev->dev);
+
+out_unlock:
+	spin_unlock_irqrestore(&pdom->lock, flags);
+	return ret;
 }
 
 static void pdom_detach_iommu(struct amd_iommu *iommu,
 			      struct protection_domain *pdom)
 {
 	struct pdom_iommu_info *pdom_iommu_info;
+	unsigned long flags;
+
+	spin_lock_irqsave(&pdom->lock, flags);
 
 	pdom_iommu_info = xa_load(&pdom->iommu_array, iommu->index);
-	if (!pdom_iommu_info)
+	if (!pdom_iommu_info) {
+		spin_unlock_irqrestore(&pdom->lock, flags);
 		return;
+	}
 
 	pdom_iommu_info->refcnt--;
 	if (pdom_iommu_info->refcnt == 0) {
 		xa_erase(&pdom->iommu_array, iommu->index);
 		kfree(pdom_iommu_info);
 	}
+
+	spin_unlock_irqrestore(&pdom->lock, flags);
 }
 
 static int do_attach(struct iommu_dev_data *dev_data,
 		     struct protection_domain *domain)
 {
 	struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
-	struct io_pgtable_cfg *cfg = &domain->iop.pgtbl.cfg;
 	int ret = 0;
 
 	/* Update data structures */
 	dev_data->domain = domain;
 	list_add(&dev_data->list, &domain->dev_list);
 
-	/* Update NUMA Node ID */
-	if (cfg->amd.nid == NUMA_NO_NODE)
-		cfg->amd.nid = dev_to_node(dev_data->dev);
-
 	/* Do reference counting */
 	ret = pdom_attach_iommu(iommu, domain);
 	if (ret)
@@ -2119,11 +2135,8 @@ static int attach_device(struct device *dev,
 			 struct protection_domain *domain)
 {
 	struct iommu_dev_data *dev_data;
-	unsigned long flags;
 	int ret = 0;
 
-	spin_lock_irqsave(&domain->lock, flags);
-
 	dev_data = dev_iommu_priv_get(dev);
 
 	spin_lock(&dev_data->lock);
@@ -2138,8 +2151,6 @@ static int attach_device(struct device *dev,
 out:
 	spin_unlock(&dev_data->lock);
 
-	spin_unlock_irqrestore(&domain->lock, flags);
-
 	return ret;
 }
 
@@ -2149,13 +2160,9 @@ static int attach_device(struct device *dev,
 static void detach_device(struct device *dev)
 {
 	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
-	struct protection_domain *domain = dev_data->domain;
 	struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
-	unsigned long flags;
 	bool ppr = dev_data->ppr;
 
-	spin_lock_irqsave(&domain->lock, flags);
-
 	spin_lock(&dev_data->lock);
 
 	/*
@@ -2179,8 +2186,6 @@ static void detach_device(struct device *dev)
 out:
 	spin_unlock(&dev_data->lock);
 
-	spin_unlock_irqrestore(&domain->lock, flags);
-
 	/* Remove IOPF handler */
 	if (ppr)
 		amd_iommu_iopf_remove_device(iommu, dev_data);
-- 
2.31.1

Re: [PATCH v3 06/10] iommu/amd: Reduce domain lock scope in attach device path

[Jason Gunthorpe]

On Wed, Oct 16, 2024 at 05:34:57AM +0000, Vasant Hegde wrote:
> Currently attach device path takes protection domain lock followed by
> dev_data lock. Most of the operations in this function is specific to
> device data except pdom_attach_iommu() where it updates protection
> domain structure. Hence reduce the scope of protection domain lock.
> 
> Note that this changes the locking order. Now it takes device lock
> before taking domain lock (group->mutex -> dev_data->lock ->
> pdom->lock). dev_data->lock is used only in device attachment path.
> So changing order is fine. It will not create any issue.
> 
> Finally move numa node assignment to pdom_attach_iommu().

numa node assignment should only be done during domain
allocation. This is important because the domain can be mapped prior
to being attached and without the right nid table levels will be mis
allocated.

However, that needs your other series, so let's just leave this as an
future direction note..

> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index d74d3b65c939..a738d2d7f0c4 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2016,16 +2016,23 @@ static int pdom_attach_iommu(struct amd_iommu *iommu,
>  			     struct protection_domain *pdom)
>  {
>  	struct pdom_iommu_info *pdom_iommu_info, *curr;
> +	struct io_pgtable_cfg *cfg = &pdom->iop.pgtbl.cfg;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	spin_lock_irqsave(&pdom->lock, flags);
>  
>  	pdom_iommu_info = xa_load(&pdom->iommu_array, iommu->index);

It would probably make sense to use the xa_lock to protect the xa
instead of overloading the pdom->lock. Then you don't get forced into
using GFP_ATOMIC here. That could be a future direction.

Anyhow, it is already a big improvement to narrow the scope of this
lock quite a bit

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v3 06/10] iommu/amd: Reduce domain lock scope in attach device path

[Baolu Lu]

On 2024/10/17 21:32, Jason Gunthorpe wrote:
> On Wed, Oct 16, 2024 at 05:34:57AM +0000, Vasant Hegde wrote:
>> Currently attach device path takes protection domain lock followed by
>> dev_data lock. Most of the operations in this function is specific to
>> device data except pdom_attach_iommu() where it updates protection
>> domain structure. Hence reduce the scope of protection domain lock.
>>
>> Note that this changes the locking order. Now it takes device lock
>> before taking domain lock (group->mutex -> dev_data->lock ->
>> pdom->lock). dev_data->lock is used only in device attachment path.
>> So changing order is fine. It will not create any issue.
>>
>> Finally move numa node assignment to pdom_attach_iommu().
> numa node assignment should only be done during domain
> allocation. This is important because the domain can be mapped prior
> to being attached and without the right nid table levels will be mis
> allocated.

Sorry for off the topic, just a question about the domain numa ID.

When a domain is about to be attached to a device on a different numa
node, what should the iommu driver do? Should it:

- Ignore the difference (probably log an informational message);

or

- Return -EINVAL to force the upper layer to allocate a different domain
   for the device?

Thanks,
baolu

Re: [PATCH v3 06/10] iommu/amd: Reduce domain lock scope in attach device path

[Jason Gunthorpe]

On Fri, Oct 18, 2024 at 10:38:05AM +0800, Baolu Lu wrote:
> On 2024/10/17 21:32, Jason Gunthorpe wrote:
> > On Wed, Oct 16, 2024 at 05:34:57AM +0000, Vasant Hegde wrote:
> > > Currently attach device path takes protection domain lock followed by
> > > dev_data lock. Most of the operations in this function is specific to
> > > device data except pdom_attach_iommu() where it updates protection
> > > domain structure. Hence reduce the scope of protection domain lock.
> > > 
> > > Note that this changes the locking order. Now it takes device lock
> > > before taking domain lock (group->mutex -> dev_data->lock ->
> > > pdom->lock). dev_data->lock is used only in device attachment path.
> > > So changing order is fine. It will not create any issue.
> > > 
> > > Finally move numa node assignment to pdom_attach_iommu().
> > numa node assignment should only be done during domain
> > allocation. This is important because the domain can be mapped prior
> > to being attached and without the right nid table levels will be mis
> > allocated.
> 
> Sorry for off the topic, just a question about the domain numa ID.
> 
> When a domain is about to be attached to a device on a different numa
> node, what should the iommu driver do? Should it:
> 
> - Ignore the difference (probably log an informational message);

I would say silently ignore it..

The only case that probably does this is VFIO, and VFIO is probably
doing it deliberately to save memory.

> - Return -EINVAL to force the upper layer to allocate a different domain
>   for the device?

This seems extreme..

I think if qemu or something wants to optimize this we need to return
numa information in the get_infos to let it make the right decisions
about domain allocation without being forced into anything.

Jason

Re: [PATCH v3 06/10] iommu/amd: Reduce domain lock scope in attach device path

[Baolu Lu]

On 2024/10/18 19:54, Jason Gunthorpe wrote:
> On Fri, Oct 18, 2024 at 10:38:05AM +0800, Baolu Lu wrote:
>> On 2024/10/17 21:32, Jason Gunthorpe wrote:
>>> On Wed, Oct 16, 2024 at 05:34:57AM +0000, Vasant Hegde wrote:
>>>> Currently attach device path takes protection domain lock followed by
>>>> dev_data lock. Most of the operations in this function is specific to
>>>> device data except pdom_attach_iommu() where it updates protection
>>>> domain structure. Hence reduce the scope of protection domain lock.
>>>>
>>>> Note that this changes the locking order. Now it takes device lock
>>>> before taking domain lock (group->mutex -> dev_data->lock ->
>>>> pdom->lock). dev_data->lock is used only in device attachment path.
>>>> So changing order is fine. It will not create any issue.
>>>>
>>>> Finally move numa node assignment to pdom_attach_iommu().
>>> numa node assignment should only be done during domain
>>> allocation. This is important because the domain can be mapped prior
>>> to being attached and without the right nid table levels will be mis
>>> allocated.
>> Sorry for off the topic, just a question about the domain numa ID.
>>
>> When a domain is about to be attached to a device on a different numa
>> node, what should the iommu driver do? Should it:
>>
>> - Ignore the difference (probably log an informational message);
> I would say silently ignore it..
> 
> The only case that probably does this is VFIO, and VFIO is probably
> doing it deliberately to save memory.
> 
>> - Return -EINVAL to force the upper layer to allocate a different domain
>>    for the device?
> This seems extreme..
> 
> I think if qemu or something wants to optimize this we need to return
> numa information in the get_infos to let it make the right decisions
> about domain allocation without being forced into anything.

Yea, thanks! This aligns with what we have in the intel driver.

Thanks,
baolu

Re: [PATCH v3 06/10] iommu/amd: Reduce domain lock scope in attach device path

[Vasant Hegde]

Jason,


On 10/17/2024 7:02 PM, Jason Gunthorpe wrote:
> On Wed, Oct 16, 2024 at 05:34:57AM +0000, Vasant Hegde wrote:
>> Currently attach device path takes protection domain lock followed by
>> dev_data lock. Most of the operations in this function is specific to
>> device data except pdom_attach_iommu() where it updates protection
>> domain structure. Hence reduce the scope of protection domain lock.
>>
>> Note that this changes the locking order. Now it takes device lock
>> before taking domain lock (group->mutex -> dev_data->lock ->
>> pdom->lock). dev_data->lock is used only in device attachment path.
>> So changing order is fine. It will not create any issue.
>>
>> Finally move numa node assignment to pdom_attach_iommu().
> 
> numa node assignment should only be done during domain
> allocation. This is important because the domain can be mapped prior
> to being attached and without the right nid table levels will be mis
> allocated.
> 
> However, that needs your other series, so let's just leave this as an
> future direction note..

Correct. Once these two series gets merged will remove this check.

> 
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index d74d3b65c939..a738d2d7f0c4 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -2016,16 +2016,23 @@ static int pdom_attach_iommu(struct amd_iommu *iommu,
>>  			     struct protection_domain *pdom)
>>  {
>>  	struct pdom_iommu_info *pdom_iommu_info, *curr;
>> +	struct io_pgtable_cfg *cfg = &pdom->iop.pgtbl.cfg;
>> +	unsigned long flags;
>> +	int ret = 0;
>> +
>> +	spin_lock_irqsave(&pdom->lock, flags);
>>  
>>  	pdom_iommu_info = xa_load(&pdom->iommu_array, iommu->index);
> 
> It would probably make sense to use the xa_lock to protect the xa
> instead of overloading the pdom->lock. Then you don't get forced into
> using GFP_ATOMIC here. That could be a future direction.

Right. We can do follow up series.

> 
> Anyhow, it is already a big improvement to narrow the scope of this
> lock quite a bit
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Thanks
-Vasant

[PATCH v3 07/10] iommu/amd: Rearrange attach device code

[Vasant Hegde]

attach_device() is just holding lock and calling do_attach(). There is
not need to have another function. Just move do_attach() code to
attach_device(). Similary move do_detach() code to detach_device().

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd/iommu.c | 85 +++++++++++++++------------------------
 1 file changed, 33 insertions(+), 52 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index a738d2d7f0c4..dd1fb3bedd33 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2077,12 +2077,24 @@ static void pdom_detach_iommu(struct amd_iommu *iommu,
 	spin_unlock_irqrestore(&pdom->lock, flags);
 }
 
-static int do_attach(struct iommu_dev_data *dev_data,
-		     struct protection_domain *domain)
+/*
+ * If a device is not yet associated with a domain, this function makes the
+ * device visible in the domain
+ */
+static int attach_device(struct device *dev,
+			 struct protection_domain *domain)
 {
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
 	struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
 	int ret = 0;
 
+	spin_lock(&dev_data->lock);
+
+	if (dev_data->domain != NULL) {
+		ret = -EBUSY;
+		goto out;
+	}
+
 	/* Update data structures */
 	dev_data->domain = domain;
 	list_add(&dev_data->list, &domain->dev_list);
@@ -2090,64 +2102,17 @@ static int do_attach(struct iommu_dev_data *dev_data,
 	/* Do reference counting */
 	ret = pdom_attach_iommu(iommu, domain);
 	if (ret)
-		return ret;
+		goto out;
 
 	/* Setup GCR3 table */
 	if (pdom_is_sva_capable(domain)) {
 		ret = init_gcr3_table(dev_data, domain);
 		if (ret) {
 			pdom_detach_iommu(iommu, domain);
-			return ret;
+			goto out;
 		}
 	}
 
-	return ret;
-}
-
-static void do_detach(struct iommu_dev_data *dev_data)
-{
-	struct protection_domain *domain = dev_data->domain;
-	struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
-
-	/* Clear DTE and flush the entry */
-	dev_update_dte(dev_data, false);
-
-	/* Flush IOTLB and wait for the flushes to finish */
-	amd_iommu_domain_flush_all(domain);
-
-	/* Clear GCR3 table */
-	if (pdom_is_sva_capable(domain))
-		destroy_gcr3_table(dev_data, domain);
-
-	/* Update data structures */
-	dev_data->domain = NULL;
-	list_del(&dev_data->list);
-
-	/* decrease reference counters - needs to happen after the flushes */
-	pdom_detach_iommu(iommu, domain);
-}
-
-/*
- * If a device is not yet associated with a domain, this function makes the
- * device visible in the domain
- */
-static int attach_device(struct device *dev,
-			 struct protection_domain *domain)
-{
-	struct iommu_dev_data *dev_data;
-	int ret = 0;
-
-	dev_data = dev_iommu_priv_get(dev);
-
-	spin_lock(&dev_data->lock);
-
-	if (dev_data->domain != NULL) {
-		ret = -EBUSY;
-		goto out;
-	}
-
-	ret = do_attach(dev_data, domain);
-
 out:
 	spin_unlock(&dev_data->lock);
 
@@ -2161,6 +2126,7 @@ static void detach_device(struct device *dev)
 {
 	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
 	struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
+	struct protection_domain *domain = dev_data->domain;
 	bool ppr = dev_data->ppr;
 
 	spin_lock(&dev_data->lock);
@@ -2181,7 +2147,22 @@ static void detach_device(struct device *dev)
 		dev_data->ppr = false;
 	}
 
-	do_detach(dev_data);
+	/* Clear DTE and flush the entry */
+	dev_update_dte(dev_data, false);
+
+	/* Flush IOTLB and wait for the flushes to finish */
+	amd_iommu_domain_flush_all(domain);
+
+	/* Clear GCR3 table */
+	if (pdom_is_sva_capable(domain))
+		destroy_gcr3_table(dev_data, domain);
+
+	/* Update data structures */
+	dev_data->domain = NULL;
+	list_del(&dev_data->list);
+
+	/* decrease reference counters - needs to happen after the flushes */
+	pdom_detach_iommu(iommu, domain);
 
 out:
 	spin_unlock(&dev_data->lock);
-- 
2.31.1

Re: [PATCH v3 07/10] iommu/amd: Rearrange attach device code

[Jason Gunthorpe]

On Wed, Oct 16, 2024 at 05:34:58AM +0000, Vasant Hegde wrote:
> attach_device() is just holding lock and calling do_attach(). There is
> not need to have another function. Just move do_attach() code to
> attach_device(). Similary move do_detach() code to detach_device().
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> Reviewed-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/amd/iommu.c | 85 +++++++++++++++------------------------
>  1 file changed, 33 insertions(+), 52 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

[PATCH v3 08/10] iommu/amd: Convert dev_data lock from spinlock to mutex

[Vasant Hegde]

Currently in attach device path it takes dev_data->spinlock. But as per
design attach device path can sleep. Also if device is PRI capable then
it adds device to IOMMU fault handler queue which takes mutex. Hence
currently PRI enablement is done outside dev_data lock.

Covert dev_data lock from spinlock to mutex so that it follows the
design and also PRI enablement can be done properly.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd/amd_iommu_types.h |  2 +-
 drivers/iommu/amd/iommu.c           | 14 +++++++-------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index ad6ce13c0b7b..a13c347baffd 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -836,7 +836,7 @@ struct devid_map {
  */
 struct iommu_dev_data {
 	/*Protect against attach/detach races */
-	spinlock_t lock;
+	struct mutex mutex;
 
 	struct list_head list;		  /* For domain->dev_list */
 	struct llist_node dev_data_list;  /* For global dev_data_list */
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index dd1fb3bedd33..2fefb7abb9ab 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -207,7 +207,7 @@ static struct iommu_dev_data *alloc_dev_data(struct amd_iommu *iommu, u16 devid)
 	if (!dev_data)
 		return NULL;
 
-	spin_lock_init(&dev_data->lock);
+	mutex_init(&dev_data->mutex);
 	dev_data->devid = devid;
 	ratelimit_default_init(&dev_data->rs);
 
@@ -2088,7 +2088,7 @@ static int attach_device(struct device *dev,
 	struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
 	int ret = 0;
 
-	spin_lock(&dev_data->lock);
+	mutex_lock(&dev_data->mutex);
 
 	if (dev_data->domain != NULL) {
 		ret = -EBUSY;
@@ -2114,7 +2114,7 @@ static int attach_device(struct device *dev,
 	}
 
 out:
-	spin_unlock(&dev_data->lock);
+	mutex_unlock(&dev_data->mutex);
 
 	return ret;
 }
@@ -2129,7 +2129,7 @@ static void detach_device(struct device *dev)
 	struct protection_domain *domain = dev_data->domain;
 	bool ppr = dev_data->ppr;
 
-	spin_lock(&dev_data->lock);
+	mutex_lock(&dev_data->mutex);
 
 	/*
 	 * First check if the device is still attached. It might already
@@ -2165,7 +2165,7 @@ static void detach_device(struct device *dev)
 	pdom_detach_iommu(iommu, domain);
 
 out:
-	spin_unlock(&dev_data->lock);
+	mutex_unlock(&dev_data->mutex);
 
 	/* Remove IOPF handler */
 	if (ppr)
@@ -2430,9 +2430,9 @@ static int blocked_domain_attach_device(struct iommu_domain *domain,
 		detach_device(dev);
 
 	/* Clear DTE and flush the entry */
-	spin_lock(&dev_data->lock);
+	mutex_lock(&dev_data->mutex);
 	dev_update_dte(dev_data, false);
-	spin_unlock(&dev_data->lock);
+	mutex_unlock(&dev_data->mutex);
 
 	return 0;
 }
-- 
2.31.1

Re: [PATCH v3 08/10] iommu/amd: Convert dev_data lock from spinlock to mutex

[Jason Gunthorpe]

On Wed, Oct 16, 2024 at 05:34:59AM +0000, Vasant Hegde wrote:
> Currently in attach device path it takes dev_data->spinlock. But as per
> design attach device path can sleep. Also if device is PRI capable then
> it adds device to IOMMU fault handler queue which takes mutex. Hence
> currently PRI enablement is done outside dev_data lock.
> 
> Covert dev_data lock from spinlock to mutex so that it follows the
> design and also PRI enablement can be done properly.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> Reviewed-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/amd/amd_iommu_types.h |  2 +-
>  drivers/iommu/amd/iommu.c           | 14 +++++++-------
>  2 files changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

But let's still aspire to removing this lock and using the group
mutex.

Jason

[PATCH v3 09/10] iommu/amd: Reorder attach device code

[Vasant Hegde]

Ideally in attach device path, it should take dev_data lock before
making changes to device data including IOPF enablement. So far dev_data
was using spinlock and it was hitting lock order issue when it tries to
enable IOPF. Hence Commit 526606b0a199 ("iommu/amd: Fix Invalid wait
context issue") moved IOPF enablement outside dev_data->lock.

Previous patch converted dev_data lock to mutex. Now its safe to call
amd_iommu_iopf_add_device() with dev_data->mutex. Hence move back PCI
device capability enablement (ATS, PRI, PASID) and IOPF enablement code
inside the lock. Also in attach_device(), update 'dev_data->domain' at
the end so that error handling becomes simple.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu.c | 65 +++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 36 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 2fefb7abb9ab..cd2371819635 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2086,6 +2086,7 @@ static int attach_device(struct device *dev,
 {
 	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
 	struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
+	struct pci_dev *pdev;
 	int ret = 0;
 
 	mutex_lock(&dev_data->mutex);
@@ -2095,10 +2096,6 @@ static int attach_device(struct device *dev,
 		goto out;
 	}
 
-	/* Update data structures */
-	dev_data->domain = domain;
-	list_add(&dev_data->list, &domain->dev_list);
-
 	/* Do reference counting */
 	ret = pdom_attach_iommu(iommu, domain);
 	if (ret)
@@ -2113,6 +2110,28 @@ static int attach_device(struct device *dev,
 		}
 	}
 
+	pdev = dev_is_pci(dev_data->dev) ? to_pci_dev(dev_data->dev) : NULL;
+	if (pdev && pdom_is_sva_capable(domain)) {
+		pdev_enable_caps(pdev);
+
+		/*
+		 * Device can continue to function even if IOPF
+		 * enablement failed. Hence in error path just
+		 * disable device PRI support.
+		 */
+		if (amd_iommu_iopf_add_device(iommu, dev_data))
+			pdev_disable_cap_pri(pdev);
+	} else if (pdev) {
+		pdev_enable_cap_ats(pdev);
+	}
+
+	/* Update data structures */
+	dev_data->domain = domain;
+	list_add(&dev_data->list, &domain->dev_list);
+
+	/* Update device table */
+	dev_update_dte(dev_data, true);
+
 out:
 	mutex_unlock(&dev_data->mutex);
 
@@ -2127,7 +2146,6 @@ static void detach_device(struct device *dev)
 	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
 	struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
 	struct protection_domain *domain = dev_data->domain;
-	bool ppr = dev_data->ppr;
 
 	mutex_lock(&dev_data->mutex);
 
@@ -2140,13 +2158,15 @@ static void detach_device(struct device *dev)
 	if (WARN_ON(!dev_data->domain))
 		goto out;
 
-	if (ppr) {
+	/* Remove IOPF handler */
+	if (dev_data->ppr) {
 		iopf_queue_flush_dev(dev);
-
-		/* Updated here so that it gets reflected in DTE */
-		dev_data->ppr = false;
+		amd_iommu_iopf_remove_device(iommu, dev_data);
 	}
 
+	if (dev_is_pci(dev))
+		pdev_disable_caps(to_pci_dev(dev));
+
 	/* Clear DTE and flush the entry */
 	dev_update_dte(dev_data, false);
 
@@ -2166,14 +2186,6 @@ static void detach_device(struct device *dev)
 
 out:
 	mutex_unlock(&dev_data->mutex);
-
-	/* Remove IOPF handler */
-	if (ppr)
-		amd_iommu_iopf_remove_device(iommu, dev_data);
-
-	if (dev_is_pci(dev))
-		pdev_disable_caps(to_pci_dev(dev));
-
 }
 
 static struct iommu_device *amd_iommu_probe_device(struct device *dev)
@@ -2450,7 +2462,6 @@ static int amd_iommu_attach_device(struct iommu_domain *dom,
 	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
 	struct protection_domain *domain = to_pdomain(dom);
 	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
-	struct pci_dev *pdev;
 	int ret;
 
 	/*
@@ -2483,24 +2494,6 @@ static int amd_iommu_attach_device(struct iommu_domain *dom,
 	}
 #endif
 
-	pdev = dev_is_pci(dev_data->dev) ? to_pci_dev(dev_data->dev) : NULL;
-	if (pdev && pdom_is_sva_capable(domain)) {
-		pdev_enable_caps(pdev);
-
-		/*
-		 * Device can continue to function even if IOPF
-		 * enablement failed. Hence in error path just
-		 * disable device PRI support.
-		 */
-		if (amd_iommu_iopf_add_device(iommu, dev_data))
-			pdev_disable_cap_pri(pdev);
-	} else if (pdev) {
-		pdev_enable_cap_ats(pdev);
-	}
-
-	/* Update device table */
-	dev_update_dte(dev_data, true);
-
 	return ret;
 }
 
-- 
2.31.1

Re: [PATCH v3 09/10] iommu/amd: Reorder attach device code

[Jason Gunthorpe]

On Wed, Oct 16, 2024 at 05:35:00AM +0000, Vasant Hegde wrote:
> Ideally in attach device path, it should take dev_data lock before
> making changes to device data including IOPF enablement. So far dev_data
> was using spinlock and it was hitting lock order issue when it tries to
> enable IOPF. Hence Commit 526606b0a199 ("iommu/amd: Fix Invalid wait
> context issue") moved IOPF enablement outside dev_data->lock.
> 
> Previous patch converted dev_data lock to mutex. Now its safe to call
> amd_iommu_iopf_add_device() with dev_data->mutex. Hence move back PCI
> device capability enablement (ATS, PRI, PASID) and IOPF enablement code
> inside the lock. Also in attach_device(), update 'dev_data->domain' at
> the end so that error handling becomes simple.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 65 +++++++++++++++++----------------------
>  1 file changed, 29 insertions(+), 36 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

[PATCH v3 10/10] iommu/amd: Improve amd_iommu_release_device()

[Vasant Hegde]

No need to have separate amd_iommu_uninit_device() just to check
dev_data and call detach domain. Hence move these checks to
amd_iommu_release_device() itself.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Reviewed-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd/iommu.c | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index cd2371819635..34a3b5eb8153 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -77,8 +77,6 @@ static DEFINE_IDA(pdom_ids);
 
 struct kmem_cache *amd_iommu_irq_cache;
 
-static void detach_device(struct device *dev);
-
 static void set_dte_entry(struct amd_iommu *iommu,
 			  struct iommu_dev_data *dev_data);
 
@@ -560,22 +558,6 @@ static void iommu_ignore_device(struct amd_iommu *iommu, struct device *dev)
 	setup_aliases(iommu, dev);
 }
 
-static void amd_iommu_uninit_device(struct device *dev)
-{
-	struct iommu_dev_data *dev_data;
-
-	dev_data = dev_iommu_priv_get(dev);
-	if (!dev_data)
-		return;
-
-	if (dev_data->domain)
-		detach_device(dev);
-
-	/*
-	 * We keep dev_data around for unplugged devices and reuse it when the
-	 * device is re-plugged - not doing so would introduce a ton of races.
-	 */
-}
 
 /****************************************************************************
  *
@@ -2242,6 +2224,7 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)
 
 static void amd_iommu_release_device(struct device *dev)
 {
+	struct iommu_dev_data *dev_data;
 	struct amd_iommu *iommu;
 
 	if (!check_device(dev))
@@ -2251,7 +2234,18 @@ static void amd_iommu_release_device(struct device *dev)
 	if (!iommu)
 		return;
 
-	amd_iommu_uninit_device(dev);
+	dev_data = dev_iommu_priv_get(dev);
+	if (!dev_data)
+		return;
+
+	if (dev_data->domain)
+		detach_device(dev);
+
+	/*
+	 * We keep dev_data around for unplugged devices and reuse it when the
+	 * device is re-plugged - not doing so would introduce a ton of races.
+	 */
+
 	iommu_completion_wait(iommu);
 }
 
-- 
2.31.1

Re: [PATCH v3 10/10] iommu/amd: Improve amd_iommu_release_device()

[Jason Gunthorpe]

On Wed, Oct 16, 2024 at 05:35:01AM +0000, Vasant Hegde wrote:

> @@ -2251,7 +2234,18 @@ static void amd_iommu_release_device(struct device *dev)
>  	if (!iommu)
>  		return;
>  
> -	amd_iommu_uninit_device(dev);
> +	dev_data = dev_iommu_priv_get(dev);
> +	if (!dev_data)
> +		return;

This can never happen, the core code never calls an op unless
probe_device succeeded.

> +	if (dev_data->domain)
> +		detach_device(dev);

This is also can't happen. The driver sets release_domain then the
core code never calls this with anything other than release_domain
attached.

Suggest just 

 struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);

 WARN_ON(dev->data->domain);

> +	/*
> +	 * We keep dev_data around for unplugged devices and reuse it when the
> +	 * device is re-plugged - not doing so would introduce a ton of races.
> +	 */
> +

Yikes.. Sounds sketchy. Why doesn't any other driver have these races?

Jason

Re: [PATCH v3 10/10] iommu/amd: Improve amd_iommu_release_device()

[Vasant Hegde]

Jason,

On 10/17/2024 7:09 PM, Jason Gunthorpe wrote:
> On Wed, Oct 16, 2024 at 05:35:01AM +0000, Vasant Hegde wrote:
> 
>> @@ -2251,7 +2234,18 @@ static void amd_iommu_release_device(struct device *dev)
>>  	if (!iommu)
>>  		return;
>>  
>> -	amd_iommu_uninit_device(dev);
>> +	dev_data = dev_iommu_priv_get(dev);
>> +	if (!dev_data)
>> +		return;
> 
> This can never happen, the core code never calls an op unless
> probe_device succeeded.

Correct. Let me remove this check.

> 
>> +	if (dev_data->domain)
>> +		detach_device(dev);
> 
> This is also can't happen. The driver sets release_domain then the
> core code never calls this with anything other than release_domain
> attached.

I wanted to implement ops->release_domain  after this series. May be I can put
it as part of this series itself. We already have all the support. Should be
easy to add.


> 
> Suggest just 
> 
>  struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> 
>  WARN_ON(dev->data->domain);
> 
>> +	/*
>> +	 * We keep dev_data around for unplugged devices and reuse it when the
>> +	 * device is re-plugged - not doing so would introduce a ton of races.
>> +	 */
>> +
> 
> Yikes.. Sounds sketchy. Why doesn't any other driver have these races?

I don't know. Suravee/Joerg may know have historical details. I just moved this
from amd_iommu_uninit_device() to here.

-Vasant