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

[PATCH v2 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 iommu/next (Commit 72e7bac4135e) and a minor patch
to fix protection_domain_free() code path [1].

[1] https://lore.kernel.org/linux-iommu/23a7c339-a7ba-447e-a4fa-935250f771eb@amd.com/T/#m870159022b8e1141d6b9c6ef87c6ee6821d0428d

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


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 v2 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>
---
 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 154f90e7b98e..fc1969011ff9 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 v2 01/10] iommu/amd: Use ida interface to manage protection domain ID

[Joerg Roedel]

On Tue, Sep 10, 2024 at 06:58:03AM +0000, Vasant Hegde wrote:
> Replace custom domain ID allocator with IDA interface.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  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(-)

Reviewed-by: Joerg Roedel <jroedel@suse.de>

[PATCH v2 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>
---
 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 fc1969011ff9..d870aeb654a7 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 v2 02/10] iommu/amd: Remove protection_domain.dev_cnt variable

[Joerg Roedel]

On Tue, Sep 10, 2024 at 06:58:04AM +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>
> ---
>  drivers/iommu/amd/amd_iommu_types.h | 1 -
>  drivers/iommu/amd/iommu.c           | 7 +------
>  2 files changed, 1 insertion(+), 7 deletions(-)
 
Reviewed-by: Joerg Roedel <jroedel@suse.de>

[PATCH v2 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>
---
 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 d870aeb654a7..b9b060219596 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 v2 03/10] iommu/amd: xarray to track protection_domain->iommu list

[Joerg Roedel]

On Tue, Sep 10, 2024 at 06:58:05AM +0000, Vasant Hegde wrote:
> 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>
> ---
>  drivers/iommu/amd/amd_iommu_types.h |  8 ++-
>  drivers/iommu/amd/iommu.c           | 85 ++++++++++++++++++++++-------
>  2 files changed, 72 insertions(+), 21 deletions(-)
 
Reviewed-by: Joerg Roedel <jroedel@suse.de>

[PATCH v2 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>
---
 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 v2 04/10] iommu/amd: Remove unused amd_iommus variable

[Joerg Roedel]

On Tue, Sep 10, 2024 at 06:58:06AM +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>
> ---
>  drivers/iommu/amd/amd_iommu_types.h | 6 ------
>  drivers/iommu/amd/init.c            | 6 ------
>  2 files changed, 12 deletions(-)
 
Reviewed-by: Joerg Roedel <jroedel@suse.de>

[PATCH v2 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>
---
 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 b9b060219596..a4a4fc7ba689 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 v2 05/10] iommu/amd: Do not detach devices in domain free path

[Joerg Roedel]

On Tue, Sep 10, 2024 at 06:58:07AM +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>
> ---
>  drivers/iommu/amd/iommu.c | 26 +-------------------------
>  1 file changed, 1 insertion(+), 25 deletions(-)
 
Reviewed-by: Joerg Roedel <jroedel@suse.de>

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

[Vasant Hegde]

Protection domain lock is required only when updating domain details.
Hence reduce the scope of protection domain lock.

Also 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 a4a4fc7ba689..713d05a85de9 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 v2 06/10] iommu/amd: Reduce domain lock scope in attach device path

[Joerg Roedel]

Hi Vasant,

On Tue, Sep 10, 2024 at 06:58:08AM +0000, Vasant Hegde wrote:
> @@ -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);

This patch inverses the locking order of domain->lock and
dev_data->lock, have you checked all places to make sure it is safe to
do so? Has this change been tested with lockdep enabled?

If so, please document that aspect in the changelog and why it is safe.

Regards,

	Joerg

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

[Vasant Hegde]

Hi Joerg,


On 10/15/2024 2:08 PM, Joerg Roedel wrote:
> Hi Vasant,
> 
> On Tue, Sep 10, 2024 at 06:58:08AM +0000, Vasant Hegde wrote:
>> @@ -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);
> 
> This patch inverses the locking order of domain->lock and
> dev_data->lock, have you checked all places to make sure it is safe to
> do so? Has this change been tested with lockdep enabled?

Sorry. I missed to explain this part in commit message.

With all changes locking sequence will be :
  group->mutex -> dev_data->mutex -> domain->lock

In defer attach path (iommu_deferred_attach()), core call attach_device() w/o
group->mutex lock. So I retained dev_data lock.

I have manually audited this change and also tested with LOCKDEP. Its all fine.


> 
> If so, please document that aspect in the changelog and why it is safe.


Sure. Will update in next version.

-Vasant

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

[Jason Gunthorpe]

On Tue, Oct 15, 2024 at 06:00:25PM +0530, Vasant Hegde wrote:
> Hi Joerg,
> 
> 
> On 10/15/2024 2:08 PM, Joerg Roedel wrote:
> > Hi Vasant,
> > 
> > On Tue, Sep 10, 2024 at 06:58:08AM +0000, Vasant Hegde wrote:
> >> @@ -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);
> > 
> > This patch inverses the locking order of domain->lock and
> > dev_data->lock, have you checked all places to make sure it is safe to
> > do so? Has this change been tested with lockdep enabled?
> 
> Sorry. I missed to explain this part in commit message.
> 
> With all changes locking sequence will be :
>   group->mutex -> dev_data->mutex -> domain->lock
> 
> In defer attach path (iommu_deferred_attach()), core call attach_device() w/o
> group->mutex lock. So I retained dev_data lock.

deferred attach is broken to not have the core code take the lock, all
most all drivers depend on group_mutex for correctness.

I think we should fix that, not retain unnecessary locks in drivers :\

Jason

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

[Vasant Hegde]

Jason,

On 10/15/2024 9:42 PM, Jason Gunthorpe wrote:
> On Tue, Oct 15, 2024 at 06:00:25PM +0530, Vasant Hegde wrote:
>> Hi Joerg,
>>
>>
>> On 10/15/2024 2:08 PM, Joerg Roedel wrote:
>>> Hi Vasant,
>>>
>>> On Tue, Sep 10, 2024 at 06:58:08AM +0000, Vasant Hegde wrote:
>>>> @@ -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);
>>>
>>> This patch inverses the locking order of domain->lock and
>>> dev_data->lock, have you checked all places to make sure it is safe to
>>> do so? Has this change been tested with lockdep enabled?
>>
>> Sorry. I missed to explain this part in commit message.
>>
>> With all changes locking sequence will be :
>>   group->mutex -> dev_data->mutex -> domain->lock
>>
>> In defer attach path (iommu_deferred_attach()), core call attach_device() w/o
>> group->mutex lock. So I retained dev_data lock.
> 
> deferred attach is broken to not have the core code take the lock, all
> most all drivers depend on group_mutex for correctness.
> 
> I think we should fix that, not retain unnecessary locks in drivers :\

If you recall, I had a patch to fix core code [1]. Robin had a concern and hence
that patch was dropped.

[1]
https://lore.kernel.org/linux-iommu/aaf7b619-1826-4db2-aaa6-235a25f46299@arm.com/

So for now I will retain this lock. and if we fix core layer, I can have patch
to drop dev_data->mutex.

-Vasant

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

[Jason Gunthorpe]

On Tue, Oct 15, 2024 at 10:15:30PM +0530, Vasant Hegde wrote:
> >> With all changes locking sequence will be :
> >>   group->mutex -> dev_data->mutex -> domain->lock
> >>
> >> In defer attach path (iommu_deferred_attach()), core call attach_device() w/o
> >> group->mutex lock. So I retained dev_data lock.
> > 
> > deferred attach is broken to not have the core code take the lock, all
> > most all drivers depend on group_mutex for correctness.
> > 
> > I think we should fix that, not retain unnecessary locks in drivers :\
> 
> If you recall, I had a patch to fix core code [1]. Robin had a concern and hence
> that patch was dropped.

I do, but I think the concern was addressed.

There might have been a small argument if AMD had a non-sleeping
attach path (at least in some case), but here you are switching to a
mutex so that is out the window.

It is one thing to say we don't want the group mutex in core because
it is just documentation, it is quite another to say you will keep a
fully redundant mutex in the driver!!

Drivers are relying on the group mutex, we have a lockdep assertion
helper now, we need to hold it consistently when invoking the
attach_dev and related ops.

But I agree we should deal with this in a seperate series

Jason

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

[Vasant Hegde]

Jason,


On 10/15/2024 10:31 PM, Jason Gunthorpe wrote:
> On Tue, Oct 15, 2024 at 10:15:30PM +0530, Vasant Hegde wrote:
>>>> With all changes locking sequence will be :
>>>>   group->mutex -> dev_data->mutex -> domain->lock
>>>>
>>>> In defer attach path (iommu_deferred_attach()), core call attach_device() w/o
>>>> group->mutex lock. So I retained dev_data lock.
>>>
>>> deferred attach is broken to not have the core code take the lock, all
>>> most all drivers depend on group_mutex for correctness.
>>>
>>> I think we should fix that, not retain unnecessary locks in drivers :\
>>
>> If you recall, I had a patch to fix core code [1]. Robin had a concern and hence
>> that patch was dropped.
> 
> I do, but I think the concern was addressed.
> 
> There might have been a small argument if AMD had a non-sleeping
> attach path (at least in some case), but here you are switching to a
> mutex so that is out the window.
> 
> It is one thing to say we don't want the group mutex in core because
> it is just documentation, it is quite another to say you will keep a
> fully redundant mutex in the driver!!
> 
> Drivers are relying on the group mutex, we have a lockdep assertion
> helper now, we need to hold it consistently when invoking the
> attach_dev and related ops.

Yeah. Its redundant mutex lock. I am happy to get rid of it.
I will look into it as separate fix.

-Vasant

[PATCH v2 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>
---
 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 713d05a85de9..ab8bedf55459 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 v2 07/10] iommu/amd: Rearrange attach device code

[Joerg Roedel]

On Tue, Sep 10, 2024 at 06:58:09AM +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>
> ---
>  drivers/iommu/amd/iommu.c | 85 +++++++++++++++------------------------
>  1 file changed, 33 insertions(+), 52 deletions(-)

Reviewed-by: Joerg Roedel <jroedel@suse.de>

[PATCH v2 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>
---
 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 ab8bedf55459..0f83e1b3fb0c 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 v2 08/10] iommu/amd: Convert dev_data lock from spinlock to mutex

[Joerg Roedel]

On Tue, Sep 10, 2024 at 06:58:10AM +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>
> ---
>  drivers/iommu/amd/amd_iommu_types.h |  2 +-
>  drivers/iommu/amd/iommu.c           | 14 +++++++-------
>  2 files changed, 8 insertions(+), 8 deletions(-)

Pending on the clarification for the lock inversion change:

Reviewed-by: Joerg Roedel <jroedel@suse.de>

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

[Vasant Hegde]

Previous patch converted dev_data lock to mutex. Move 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 0f83e1b3fb0c..e7fc59e30387 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 v2 09/10] iommu/amd: Reorder attach device code

[Joerg Roedel]

On Tue, Sep 10, 2024 at 06:58:11AM +0000, Vasant Hegde wrote:
> Previous patch converted dev_data lock to mutex. Move 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.

This patch description lacks the 'Why?' part.

> 
> 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 0f83e1b3fb0c..e7fc59e30387 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 v2 09/10] iommu/amd: Reorder attach device code

[Vasant Hegde]

Hi Joerg,

On 10/15/2024 2:14 PM, Joerg Roedel wrote:
> On Tue, Sep 10, 2024 at 06:58:11AM +0000, Vasant Hegde wrote:
>> Previous patch converted dev_data lock to mutex. Move 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.
> 
> This patch description lacks the 'Why?' part.

Agree. I have updated it.

-Vasant

[PATCH v2 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>
---
 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 e7fc59e30387..15895ddf1413 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 v2 10/10] iommu/amd: Improve amd_iommu_release_device()

[Joerg Roedel]

On Tue, Sep 10, 2024 at 06:58:12AM +0000, Vasant Hegde wrote:
> 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>
> ---
>  drivers/iommu/amd/iommu.c | 32 +++++++++++++-------------------
>  1 file changed, 13 insertions(+), 19 deletions(-)

Reviewed-by: Joerg Roedel <jroedel@suse.de>

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

[Vasant Hegde]

Joerg,


On 9/10/2024 12:28 PM, Vasant Hegde wrote:
> 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

Ping.

This applies cleanly on top of v6.12-rc1. I have tested on to of v6.12-rc1 and
its working fine.

-Vasant