[PATCH v4 00/16] iommu/amd: SVA Support (part 3) - refactor support for GCR3 table

[PATCH v4 00/16] iommu/amd: SVA Support (part 3) - refactor support for GCR3 table

[Vasant Hegde]

This is part 3 of the 4-part series to introduce Share Virtual
Address (SVA) support, which focuses on refactoring GCR3 table.
This moves GCR3 related information from protection domain
structure to iommu_dev_data (per device) structure.

It contains the following enhancements:

* Patch 1 - 3:
  Cleanup and introduce helper function

* Patch 4 - 13:
  Introduce per device GCR3 table, per device domain ID, invalidation based
  on per device and code refactoring

* Patch 14 - 16:
  Remove unused variable, rearrange functions to make it easy to maintain

This patch series is based on top of iommu/next branch (commit bd19c7637024).

This is also available at github :
  https://github.com/AMDESE/linux/tree/iommu_sva_part3_v4_v6.7_rc5

Thanks Jason for reviewing previous versions and providing valuable feedbacks.


Changes from v3 -> v4:
  - Added new patch to enable IOMMU GT feature early
  - Fixed dirty tracking path to use get_amd_iommu_from_dev()
  - Fixed duplicate flush issue in do_attach() path
  - Removed gcr3_info->giov variable. Instead we use domain page table mode to decide
    DTE[GIOV] bit.
  - Free gcr3_table before calling set_dte_entry (otherwise there is a possibility
    of its setting up gcr3 table with glx 0)
  - Adjusted gfp_t flag for GCR3 table allocation (GPF_ATOMIC -> GFP_KERNEL)
  - Moved below patches from invalidation improvement series to this series
    * Remove unused flush_pasid() related functions
      (Originally it was part of Invalidation improvement series, I had to push it to
       this series as set_gcr3() was still using __amd_iommu_flush_tlb()).
    * Added patch to Rearrange device flush code

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

Changes from v2 -> v3:
  - Dropped "iommu/amd: Use struct protection_domain in helper functions" as flush related
    functions are already fixed and set_gcr3 related functions is reworked in this series.
  - Removed moving to_pdomain() function from this series. It will be introduced
    in SVA series where it will be used.
  - Added per device domain ID and per device invalidation functions
  - Added functions to flush TLB for the given device
  - Added Review-by tags

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

Changes from v1 - v2:
  - Dropped iommu_v2 module related support as newly introduced Part2
    removed iommu_v2 module.
  - Moved Patch 'Use struct protection_domain in helper functions' from Part1 to
    Part3
  - Updated get_amd_iommu_from_dev() to retrieve iommu from device structure
  - Removed 'PD_MODE_PT'

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

Suravee Suthikulpanit (8):
  iommu/amd: Introduce get_amd_iommu_from_dev()
  iommu/amd: Introduce struct protection_domain.pd_mode
  iommu/amd: Introduce per-device GCR3 table
  iommu/amd: Refactor helper function for setting / clearing GCR3
  iommu/amd: Refactor attaching / detaching device functions
  iommu/amd: Refactor protection_domain helper functions
  iommu/amd: Refactor GCR3 table helper functions
  iommu/amd: Remove unused GCR3 table parameters from struct
    protection_domain

Vasant Hegde (8):
  iommu/amd: Pass struct iommu_dev_data to set_dte_entry()
  iommu/amd: Enable Guest Translation before registering devices
  iommu/amd: Use protection_domain.flags to check page table mode
  iommu/amd: Introduce per-device domain ID to workaround potential TLB
    aliasing issue
  iommu/amd: Add support for device based TLB invalidation
  iommu/amd: Rearrange GCR3 table setup code
  iommu/amd: Remove unused flush pasid functions
  iommu/amd: Rearrange device flush code

 drivers/iommu/amd/amd_iommu.h       |  25 +-
 drivers/iommu/amd/amd_iommu_types.h |  27 +-
 drivers/iommu/amd/init.c            |   6 +-
 drivers/iommu/amd/io_pgtable_v2.c   |  21 +-
 drivers/iommu/amd/iommu.c           | 646 ++++++++++++++--------------
 include/linux/iommu.h               |  13 +
 6 files changed, 371 insertions(+), 367 deletions(-)

-- 
2.31.1

[PATCH v4 01/16] iommu/amd: Pass struct iommu_dev_data to set_dte_entry()

[Vasant Hegde]

Pass iommu_dev_data structure instead of passing indivisual variables.

No functional changes intended.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/iommu.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 255ea754c0cd..474c073a422c 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1706,12 +1706,14 @@ static int setup_gcr3_table(struct protection_domain *domain, int pasids)
 	return 0;
 }
 
-static void set_dte_entry(struct amd_iommu *iommu, u16 devid,
-			  struct protection_domain *domain, bool ats, bool ppr)
+static void set_dte_entry(struct amd_iommu *iommu,
+			  struct iommu_dev_data *dev_data)
 {
 	u64 pte_root = 0;
 	u64 flags = 0;
 	u32 old_domid;
+	u16 devid = dev_data->devid;
+	struct protection_domain *domain = dev_data->domain;
 	struct dev_table_entry *dev_table = get_dev_table(iommu);
 
 	if (domain->iop.mode != PAGE_MODE_NONE)
@@ -1731,10 +1733,10 @@ static void set_dte_entry(struct amd_iommu *iommu, u16 devid,
 
 	flags = dev_table[devid].data[1];
 
-	if (ats)
+	if (dev_data->ats_enabled)
 		flags |= DTE_FLAG_IOTLB;
 
-	if (ppr)
+	if (dev_data->ppr)
 		pte_root |= 1ULL << DEV_ENTRY_PPR;
 
 	if (domain->dirty_tracking)
@@ -1810,12 +1812,10 @@ static void do_attach(struct iommu_dev_data *dev_data,
 		      struct protection_domain *domain)
 {
 	struct amd_iommu *iommu;
-	bool ats;
 
 	iommu = rlookup_amd_iommu(dev_data->dev);
 	if (!iommu)
 		return;
-	ats   = dev_data->ats_enabled;
 
 	/* Update data structures */
 	dev_data->domain = domain;
@@ -1830,8 +1830,7 @@ static void do_attach(struct iommu_dev_data *dev_data,
 	domain->dev_cnt                 += 1;
 
 	/* Update device table */
-	set_dte_entry(iommu, dev_data->devid, domain,
-		      ats, dev_data->ppr);
+	set_dte_entry(iommu, dev_data);
 	clone_aliases(iommu, dev_data->dev);
 
 	device_flush_dte(dev_data);
@@ -2015,8 +2014,7 @@ static void update_device_table(struct protection_domain *domain)
 
 		if (!iommu)
 			continue;
-		set_dte_entry(iommu, dev_data->devid, domain,
-			      dev_data->ats_enabled, dev_data->ppr);
+		set_dte_entry(iommu, dev_data);
 		clone_aliases(iommu, dev_data->dev);
 	}
 }
-- 
2.31.1

[PATCH v4 02/16] iommu/amd: Enable Guest Translation before registering devices

[Vasant Hegde]

IOMMU Guest Translation (GT) feature needs to be enabled before
invalidating guest translations (CMD_INV_IOMMU_PAGES with GN=1).

Currently GT feature is enabled after setting up interrupt handler.
So far it was fine as we were not invalidating guest page table
before this point.

Upcoming series will introduce per device GCR3 table and it will
invalidate guest pages after configuring. Hence move GT feature
enablement to early_enable_iommu().

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

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index c83bd0c2a1c9..959820ccfbcc 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -2769,6 +2769,7 @@ static void early_enable_iommu(struct amd_iommu *iommu)
 	iommu_enable_command_buffer(iommu);
 	iommu_enable_event_buffer(iommu);
 	iommu_set_exclusion_range(iommu);
+	iommu_enable_gt(iommu);
 	iommu_enable_ga(iommu);
 	iommu_enable_xt(iommu);
 	iommu_enable_irtcachedis(iommu);
@@ -2825,6 +2826,7 @@ static void early_enable_iommus(void)
 			iommu_disable_irtcachedis(iommu);
 			iommu_enable_command_buffer(iommu);
 			iommu_enable_event_buffer(iommu);
+			iommu_enable_gt(iommu);
 			iommu_enable_ga(iommu);
 			iommu_enable_xt(iommu);
 			iommu_enable_irtcachedis(iommu);
@@ -2838,10 +2840,8 @@ static void enable_iommus_v2(void)
 {
 	struct amd_iommu *iommu;
 
-	for_each_iommu(iommu) {
+	for_each_iommu(iommu)
 		iommu_enable_ppr_log(iommu);
-		iommu_enable_gt(iommu);
-	}
 }
 
 static void enable_iommus_vapic(void)
-- 
2.31.1

[PATCH v4 03/16] iommu/amd: Introduce get_amd_iommu_from_dev()

[Vasant Hegde]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

And replace rlookup_amd_iommu() with the new helper function where
applicable to avoid unnecessary loop to look up struct amd_iommu from
struct device.

Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu.h | 11 +++++++++
 drivers/iommu/amd/iommu.c     | 43 ++++++++++-------------------------
 include/linux/iommu.h         | 13 +++++++++++
 3 files changed, 36 insertions(+), 31 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 8b3601f285fd..4edadace486a 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -150,6 +150,17 @@ static inline void *alloc_pgtable_page(int nid, gfp_t gfp)
 	return page ? page_address(page) : NULL;
 }
 
+/*
+ * This must be called after device probe completes. During probe
+ * use rlookup_amd_iommu() get the iommu.
+ */
+static inline struct amd_iommu *get_amd_iommu_from_dev(struct device *dev)
+{
+	struct iommu_device *iommu = iommu_get_iommu_dev(dev);
+
+	return container_of(iommu, struct amd_iommu, iommu);
+}
+
 bool translation_pre_enabled(struct amd_iommu *iommu);
 bool amd_iommu_is_attach_deferred(struct device *dev);
 int __init add_special_device(u8 type, u8 id, u32 *devid, bool cmd_line);
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 474c073a422c..6d0ba9a35489 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1395,9 +1395,7 @@ static int device_flush_iotlb(struct iommu_dev_data *dev_data, u64 address,
 	int qdep;
 
 	qdep     = dev_data->ats_qdep;
-	iommu    = rlookup_amd_iommu(dev_data->dev);
-	if (!iommu)
-		return -EINVAL;
+	iommu    = get_amd_iommu_from_dev(dev_data->dev);
 
 	build_inv_iotlb_pages(&cmd, dev_data->devid, qdep, address,
 			      size, pasid, gn);
@@ -1423,9 +1421,7 @@ static int device_flush_dte(struct iommu_dev_data *dev_data)
 	u16 alias;
 	int ret;
 
-	iommu = rlookup_amd_iommu(dev_data->dev);
-	if (!iommu)
-		return -EINVAL;
+	iommu = get_amd_iommu_from_dev(dev_data->dev);
 
 	if (dev_is_pci(dev_data->dev))
 		pdev = to_pci_dev(dev_data->dev);
@@ -1813,9 +1809,7 @@ static void do_attach(struct iommu_dev_data *dev_data,
 {
 	struct amd_iommu *iommu;
 
-	iommu = rlookup_amd_iommu(dev_data->dev);
-	if (!iommu)
-		return;
+	iommu = get_amd_iommu_from_dev(dev_data->dev);
 
 	/* Update data structures */
 	dev_data->domain = domain;
@@ -1841,9 +1835,7 @@ static void do_detach(struct iommu_dev_data *dev_data)
 	struct protection_domain *domain = dev_data->domain;
 	struct amd_iommu *iommu;
 
-	iommu = rlookup_amd_iommu(dev_data->dev);
-	if (!iommu)
-		return;
+	iommu = get_amd_iommu_from_dev(dev_data->dev);
 
 	/* Update data structures */
 	dev_data->domain = NULL;
@@ -2010,10 +2002,8 @@ static void update_device_table(struct protection_domain *domain)
 	struct iommu_dev_data *dev_data;
 
 	list_for_each_entry(dev_data, &domain->dev_list, list) {
-		struct amd_iommu *iommu = rlookup_amd_iommu(dev_data->dev);
+		struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
 
-		if (!iommu)
-			continue;
 		set_dte_entry(iommu, dev_data);
 		clone_aliases(iommu, dev_data->dev);
 	}
@@ -2194,11 +2184,8 @@ static struct iommu_domain *do_iommu_domain_alloc(unsigned int type,
 	struct protection_domain *domain;
 	struct amd_iommu *iommu = NULL;
 
-	if (dev) {
-		iommu = rlookup_amd_iommu(dev);
-		if (!iommu)
-			return ERR_PTR(-ENODEV);
-	}
+	if (dev)
+		iommu = get_amd_iommu_from_dev(dev);
 
 	/*
 	 * Since DTE[Mode]=0 is prohibited on SNP-enabled system,
@@ -2279,7 +2266,7 @@ 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 = rlookup_amd_iommu(dev);
+	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
 	int ret;
 
 	/*
@@ -2418,7 +2405,7 @@ static bool amd_iommu_capable(struct device *dev, enum iommu_cap cap)
 	case IOMMU_CAP_DEFERRED_FLUSH:
 		return true;
 	case IOMMU_CAP_DIRTY_TRACKING: {
-		struct amd_iommu *iommu = rlookup_amd_iommu(dev);
+		struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
 
 		return amd_iommu_hd_support(iommu);
 	}
@@ -2447,9 +2434,7 @@ static int amd_iommu_set_dirty_tracking(struct iommu_domain *domain,
 	}
 
 	list_for_each_entry(dev_data, &pdomain->dev_list, list) {
-		iommu = rlookup_amd_iommu(dev_data->dev);
-		if (!iommu)
-			continue;
+		iommu = get_amd_iommu_from_dev(dev_data->dev);
 
 		dev_table = get_dev_table(iommu);
 		pte_root = dev_table[dev_data->devid].data[0];
@@ -2509,9 +2494,7 @@ static void amd_iommu_get_resv_regions(struct device *dev,
 		return;
 
 	devid = PCI_SBDF_TO_DEVID(sbdf);
-	iommu = rlookup_amd_iommu(dev);
-	if (!iommu)
-		return;
+	iommu = get_amd_iommu_from_dev(dev);
 	pci_seg = iommu->pci_seg;
 
 	list_for_each_entry(entry, &pci_seg->unity_map, list) {
@@ -2845,9 +2828,7 @@ int amd_iommu_complete_ppr(struct pci_dev *pdev, u32 pasid,
 	struct iommu_cmd cmd;
 
 	dev_data = dev_iommu_priv_get(&pdev->dev);
-	iommu    = rlookup_amd_iommu(&pdev->dev);
-	if (!iommu)
-		return -ENODEV;
+	iommu    = get_amd_iommu_from_dev(&pdev->dev);
 
 	build_complete_ppr(&cmd, dev_data->devid, pasid, status,
 			   tag, dev_data->pri_tlp);
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 8d4f74ff2a8b..44fe09162b21 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -670,6 +670,19 @@ static inline struct iommu_device *dev_to_iommu_device(struct device *dev)
 	return (struct iommu_device *)dev_get_drvdata(dev);
 }
 
+/**
+ * iommu_get_iommu_dev - Get iommu_device for a device
+ * @dev: an end-point device
+ *
+ * Note that this function must be called from the iommu_ops
+ * to retrieve the iommu_device for a device, which the core code
+ * guarentees it will not invoke the op without an attached iommu.
+ */
+static inline struct iommu_device *iommu_get_iommu_dev(struct device *dev)
+{
+	return dev->iommu->iommu_dev;
+}
+
 static inline void iommu_iotlb_gather_init(struct iommu_iotlb_gather *gather)
 {
 	*gather = (struct iommu_iotlb_gather) {
-- 
2.31.1

[PATCH v4 04/16] iommu/amd: Introduce struct protection_domain.pd_mode

[Vasant Hegde]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

This enum variable is used to track the type of page table used by the
protection domain. It will replace the protection_domain.flags in
subsequent series.

Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/amd_iommu_types.h | 6 ++++++
 drivers/iommu/amd/iommu.c           | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 809d74faa1a5..934a63f64839 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -549,6 +549,11 @@ struct amd_io_pgtable {
 	u64			*pgd;		/* v2 pgtable pgd pointer */
 };
 
+enum protection_domain_mode {
+	PD_MODE_V1 = 1,
+	PD_MODE_V2,
+};
+
 /*
  * This structure contains generic data for  IOMMU protection domains
  * independent of their use.
@@ -564,6 +569,7 @@ struct protection_domain {
 	int nid;		/* Node ID */
 	u64 *gcr3_tbl;		/* Guest CR3 table */
 	unsigned long flags;	/* flags to find out type of domain */
+	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 */
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 6d0ba9a35489..9233f476d4a4 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2084,6 +2084,7 @@ static int protection_domain_init_v1(struct protection_domain *domain, int mode)
 			return -ENOMEM;
 	}
 
+	domain->pd_mode = PD_MODE_V1;
 	amd_iommu_domain_set_pgtable(domain, pt_root, mode);
 
 	return 0;
@@ -2092,6 +2093,7 @@ static int protection_domain_init_v1(struct protection_domain *domain, int mode)
 static int protection_domain_init_v2(struct protection_domain *domain)
 {
 	domain->flags |= PD_GIOV_MASK;
+	domain->pd_mode = PD_MODE_V2;
 
 	domain->domain.pgsize_bitmap = AMD_IOMMU_PGSIZES_V2;
 
-- 
2.31.1

[PATCH v4 05/16] iommu/amd: Introduce per-device GCR3 table

[Vasant Hegde]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

AMD IOMMU GCR3 table is indexed by PASID. Each entry stores guest CR3
register value, which is an address to the root of guest IO page table.
The GCR3 table can be programmed per-device. However, Linux AMD IOMMU
driver currently managing the table on a per-domain basis.

PASID is a device feature. When SVA is enabled it will bind PASID to
device, not domain. Hence it makes sense to have per device GCR3 table.

Introduce struct iommu_dev_data.gcr3_tbl_info to keep track of GCR3 table
configuration. This will eventually replaces gcr3 related variables in
protection_domain structure.

Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu_types.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 934a63f64839..fead9033796f 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -541,6 +541,12 @@ struct amd_irte_ops;
 #define io_pgtable_cfg_to_data(x) \
 	container_of((x), struct amd_io_pgtable, pgtbl_cfg)
 
+struct gcr3_tbl_info {
+	u64	*gcr3_tbl;	/* Guest CR3 table */
+	int	glx;		/* Number of levels for GCR3 table */
+	u32	pasid_cnt;	/* Track attached PASIDs */
+};
+
 struct amd_io_pgtable {
 	struct io_pgtable_cfg	pgtbl_cfg;
 	struct io_pgtable	iop;
@@ -822,6 +828,7 @@ struct iommu_dev_data {
 	struct list_head list;		  /* For domain->dev_list */
 	struct llist_node dev_data_list;  /* For global dev_data_list */
 	struct protection_domain *domain; /* Domain the device is bound to */
+	struct gcr3_tbl_info gcr3_info;   /* Per-device GCR3 table */
 	struct device *dev;
 	u16 devid;			  /* PCI Device ID */
 
-- 
2.31.1

[PATCH v4 06/16] iommu/amd: Use protection_domain.flags to check page table mode

[Vasant Hegde]

Page table mode (v1, v2 or pt) is per domain property. Recently we have
enhanced protection_domain.pd_mode to track per domain page table mode.
Use that variable to check the page table mode instead of global
'amd_iommu_pgtable' in {map/unmap}_pages path.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 9233f476d4a4..93d436bc3e8e 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2326,7 +2326,7 @@ static int amd_iommu_map_pages(struct iommu_domain *dom, unsigned long iova,
 	int prot = 0;
 	int ret = -EINVAL;
 
-	if ((amd_iommu_pgtable == AMD_IOMMU_V1) &&
+	if ((domain->pd_mode == PD_MODE_V1) &&
 	    (domain->iop.mode == PAGE_MODE_NONE))
 		return -EINVAL;
 
@@ -2372,7 +2372,7 @@ static size_t amd_iommu_unmap_pages(struct iommu_domain *dom, unsigned long iova
 	struct io_pgtable_ops *ops = &domain->iop.iop.ops;
 	size_t r;
 
-	if ((amd_iommu_pgtable == AMD_IOMMU_V1) &&
+	if ((domain->pd_mode == PD_MODE_V1) &&
 	    (domain->iop.mode == PAGE_MODE_NONE))
 		return 0;
 
-- 
2.31.1

[PATCH v4 07/16] iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue

[Vasant Hegde]

With v1 page table, the AMD IOMMU spec states that the hardware must use
the domain ID to tag its internal translation caches. I/O devices with
different v1 page tables must be given different domain IDs. I/O devices
that share the same v1 page table __may__ be given the same domain ID.
This domain ID management policy is currently implemented by the AMD
IOMMU driver. In this case, only the domain ID is needed when issuing the
INVALIDATE_IOMMU_PAGES command to invalidate the IOMMU translation cache
(TLB).

With v2 page table, the hardware uses domain ID and PASID as parameters
to tag and issue the INVALIDATE_IOMMU_PAGES command. Since the GCR3 table
is setup per-device, and there is no guarantee for PASID to be unique
across multiple devices. The same PASID for different devices could
have different v2 page tables. In such case, if multiple devices share the
same domain ID, IOMMU translation cache for these devices would be polluted
due to TLB aliasing.

Hence, avoid the TLB aliasing issue with v2 page table by allocating unique
domain ID for each device even when multiple devices are sharing the same v1
page table. Please note that this workaround would result in multiple
INVALIDATE_IOMMU_PAGES commands (one per domain id) when unmapping a
translation on the shared v1 page table.

Domain ID can be shared until device starts using PASID. We will enhance this
code later where we will allocate per device domain ID only when its needed.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu_types.h |  2 +
 drivers/iommu/amd/iommu.c           | 83 +++++++++++++++++++++++------
 2 files changed, 70 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index fead9033796f..51daf5dd4729 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -842,6 +842,8 @@ struct iommu_dev_data {
 	u8 ppr          :1;		  /* Enable device PPR support */
 	bool use_vapic;			  /* Enable device to use vapic mode */
 	bool defer_attach;
+	/* Per device domain ID. Used with V2 page table */
+	u16 domid;
 
 	struct ratelimit_state rs;        /* Ratelimit IOPF messages */
 };
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 93d436bc3e8e..e497ce58b24a 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -90,6 +90,14 @@ static inline bool pdom_is_v2_pgtbl_mode(struct protection_domain *pdom)
 	return (pdom && (pdom->flags & PD_IOMMUV2_MASK));
 }
 
+/*
+ * Allocate per device domain ID when using V2 page table
+ */
+static inline bool domain_id_is_per_dev(struct protection_domain *pdom)
+{
+	return (pdom && pdom->pd_mode != PD_MODE_V1);
+}
+
 static inline int get_acpihid_device_id(struct device *dev,
 					struct acpihid_map_entry **entry)
 {
@@ -1451,27 +1459,36 @@ static int device_flush_dte(struct iommu_dev_data *dev_data)
 	return ret;
 }
 
-/*
- * TLB invalidation function which is called from the mapping functions.
- * It invalidates a single PTE if the range to flush is within a single
- * page. Otherwise it flushes the whole TLB of the IOMMU.
- */
-static void __domain_flush_pages(struct protection_domain *domain,
+static int domain_flush_pages_v2(struct protection_domain *pdom,
 				 u64 address, size_t size)
 {
 	struct iommu_dev_data *dev_data;
 	struct iommu_cmd cmd;
-	int ret = 0, i;
-	ioasid_t pasid = IOMMU_NO_PASID;
-	bool gn = false;
+	int ret = 0;
 
-	if (pdom_is_v2_pgtbl_mode(domain))
-		gn = true;
+	list_for_each_entry(dev_data, &pdom->dev_list, list) {
+		struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
+
+		build_inv_iommu_pages(&cmd, address, size,
+				      dev_data->domid, IOMMU_NO_PASID, true);
+
+		ret |= iommu_queue_command(iommu, &cmd);
+	}
+
+	return ret;
+}
+
+static int domain_flush_pages_v1(struct protection_domain *pdom,
+				 u64 address, size_t size)
+{
+	struct iommu_cmd cmd;
+	int ret = 0, i;
 
-	build_inv_iommu_pages(&cmd, address, size, domain->id, pasid, gn);
+	build_inv_iommu_pages(&cmd, address, size,
+			      pdom->id, IOMMU_NO_PASID, false);
 
 	for (i = 0; i < amd_iommu_get_num_iommus(); ++i) {
-		if (!domain->dev_iommu[i])
+		if (!pdom->dev_iommu[i])
 			continue;
 
 		/*
@@ -1481,6 +1498,28 @@ static void __domain_flush_pages(struct protection_domain *domain,
 		ret |= iommu_queue_command(amd_iommus[i], &cmd);
 	}
 
+	return ret;
+}
+
+/*
+ * TLB invalidation function which is called from the mapping functions.
+ * It flushes range of PTEs of the domain.
+ */
+static void __domain_flush_pages(struct protection_domain *domain,
+				 u64 address, size_t size)
+{
+	struct iommu_dev_data *dev_data;
+	int ret = 0;
+	ioasid_t pasid = IOMMU_NO_PASID;
+	bool gn = false;
+
+	if (pdom_is_v2_pgtbl_mode(domain)) {
+		gn = true;
+		ret = domain_flush_pages_v2(domain, address, size);
+	} else {
+		ret = domain_flush_pages_v1(domain, address, size);
+	}
+
 	list_for_each_entry(dev_data, &domain->dev_list, list) {
 
 		if (!dev_data->ats_enabled)
@@ -1709,9 +1748,15 @@ static void set_dte_entry(struct amd_iommu *iommu,
 	u64 flags = 0;
 	u32 old_domid;
 	u16 devid = dev_data->devid;
+	u16 domid;
 	struct protection_domain *domain = dev_data->domain;
 	struct dev_table_entry *dev_table = get_dev_table(iommu);
 
+	if (domain_id_is_per_dev(domain))
+		domid = dev_data->domid;
+	else
+		domid = domain->id;
+
 	if (domain->iop.mode != PAGE_MODE_NONE)
 		pte_root = iommu_virt_to_phys(domain->iop.root);
 
@@ -1724,7 +1769,7 @@ static void set_dte_entry(struct amd_iommu *iommu,
 	 * When SNP is enabled, Only set TV bit when IOMMU
 	 * page translation is in use.
 	 */
-	if (!amd_iommu_snp_en || (domain->id != 0))
+	if (!amd_iommu_snp_en || (domid != 0))
 		pte_root |= DTE_FLAG_TV;
 
 	flags = dev_table[devid].data[1];
@@ -1773,7 +1818,7 @@ static void set_dte_entry(struct amd_iommu *iommu,
 	}
 
 	flags &= ~DEV_DOMID_MASK;
-	flags |= domain->id;
+	flags |= domid;
 
 	old_domid = dev_table[devid].data[1] & DEV_DOMID_MASK;
 	dev_table[devid].data[1]  = flags;
@@ -1823,6 +1868,10 @@ static void do_attach(struct iommu_dev_data *dev_data,
 	domain->dev_iommu[iommu->index] += 1;
 	domain->dev_cnt                 += 1;
 
+	/* Allocate per device domain ID */
+	if (domain_id_is_per_dev(domain))
+		dev_data->domid = domain_id_alloc();
+
 	/* Update device table */
 	set_dte_entry(iommu, dev_data);
 	clone_aliases(iommu, dev_data->dev);
@@ -1852,6 +1901,10 @@ 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;
+
+	/* Free per device domain ID */
+	if (domain_id_is_per_dev(domain))
+		domain_id_free(dev_data->domid);
 }
 
 /*
-- 
2.31.1

[PATCH v4 08/16] iommu/amd: Add support for device based TLB invalidation

[Vasant Hegde]

Add support to invalidate TLB/IOTLB for the given device.

These functions will be used in subsequent patches where we will
introduce per device GCR3 table and SVA support.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu.h |  5 +++++
 drivers/iommu/amd/iommu.c     | 23 +++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 4edadace486a..ac95481f2339 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -63,6 +63,11 @@ void amd_iommu_domain_update(struct protection_domain *domain);
 void amd_iommu_domain_flush_complete(struct protection_domain *domain);
 void amd_iommu_domain_flush_pages(struct protection_domain *domain,
 				  u64 address, size_t size);
+void amd_iommu_dev_flush_pasid_pages(struct iommu_dev_data *dev_data,
+				     ioasid_t pasid, u64 address, size_t size);
+void amd_iommu_dev_flush_pasid_all(struct iommu_dev_data *dev_data,
+				   ioasid_t pasid);
+
 int amd_iommu_flush_tlb(struct iommu_domain *dom, u32 pasid);
 int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, u32 pasid,
 			      unsigned long cr3);
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index e497ce58b24a..578462a2325b 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1588,6 +1588,29 @@ static void amd_iommu_domain_flush_all(struct protection_domain *domain)
 				     CMD_INV_IOMMU_ALL_PAGES_ADDRESS);
 }
 
+void amd_iommu_dev_flush_pasid_pages(struct iommu_dev_data *dev_data,
+				     ioasid_t pasid, u64 address, size_t size)
+{
+	struct iommu_cmd cmd;
+	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
+
+	build_inv_iommu_pages(&cmd, address, size,
+			      dev_data->domid, pasid, true);
+	iommu_queue_command(iommu, &cmd);
+
+	if (dev_data->ats_enabled)
+		device_flush_iotlb(dev_data, address, size, pasid, true);
+
+	iommu_completion_wait(iommu);
+}
+
+void amd_iommu_dev_flush_pasid_all(struct iommu_dev_data *dev_data,
+				   ioasid_t pasid)
+{
+	amd_iommu_dev_flush_pasid_pages(dev_data, 0,
+					CMD_INV_IOMMU_ALL_PAGES_ADDRESS, pasid);
+}
+
 void amd_iommu_domain_flush_complete(struct protection_domain *domain)
 {
 	int i;
-- 
2.31.1

[PATCH v4 09/16] iommu/amd: Rearrange GCR3 table setup code

[Vasant Hegde]

Consolidate GCR3 table related code in one place so that its easy
to maintain.

Note that this patch doesn't move __set_gcr3/__clear_gcr3. We are moving
GCR3 table from per domain to per device. Following series will rework
these functions. During that time I will move these functions as well.

No functional changes intended.

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

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 578462a2325b..85a30b1614de 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1764,6 +1764,38 @@ static int setup_gcr3_table(struct protection_domain *domain, int pasids)
 	return 0;
 }
 
+static u64 *__get_gcr3_pte(u64 *root, int level, u32 pasid, bool alloc)
+{
+	int index;
+	u64 *pte;
+
+	while (true) {
+
+		index = (pasid >> (9 * level)) & 0x1ff;
+		pte   = &root[index];
+
+		if (level == 0)
+			break;
+
+		if (!(*pte & GCR3_VALID)) {
+			if (!alloc)
+				return NULL;
+
+			root = (void *)get_zeroed_page(GFP_ATOMIC);
+			if (root == NULL)
+				return NULL;
+
+			*pte = iommu_virt_to_phys(root) | GCR3_VALID;
+		}
+
+		root = iommu_phys_to_virt(*pte & PAGE_MASK);
+
+		level -= 1;
+	}
+
+	return pte;
+}
+
 static void set_dte_entry(struct amd_iommu *iommu,
 			  struct iommu_dev_data *dev_data)
 {
@@ -2806,38 +2838,6 @@ int amd_iommu_flush_tlb(struct iommu_domain *dom, u32 pasid)
 	return ret;
 }
 
-static u64 *__get_gcr3_pte(u64 *root, int level, u32 pasid, bool alloc)
-{
-	int index;
-	u64 *pte;
-
-	while (true) {
-
-		index = (pasid >> (9 * level)) & 0x1ff;
-		pte   = &root[index];
-
-		if (level == 0)
-			break;
-
-		if (!(*pte & GCR3_VALID)) {
-			if (!alloc)
-				return NULL;
-
-			root = (void *)get_zeroed_page(GFP_ATOMIC);
-			if (root == NULL)
-				return NULL;
-
-			*pte = iommu_virt_to_phys(root) | GCR3_VALID;
-		}
-
-		root = iommu_phys_to_virt(*pte & PAGE_MASK);
-
-		level -= 1;
-	}
-
-	return pte;
-}
-
 static int __set_gcr3(struct protection_domain *domain, u32 pasid,
 		      unsigned long cr3)
 {
-- 
2.31.1

[PATCH v4 10/16] iommu/amd: Refactor helper function for setting / clearing GCR3

[Vasant Hegde]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Refactor GCR3 helper functions in preparation to use per device
GCR3 table.
  * Use new per device GCR3 table to set/clear the gcr3 entries.

  * Remove per domain default GCR3 setup during v2 page table allocation.
    Subsequent patch will add support to setup default gcr3 while
    attaching device to domain.

  * Remove amd_iommu_domain_update() from V2 page table path as device
    detach path will take care of updating the domain.

  * Consolidate GCR3 table related code in one place so that its easy
    to maintain.

  * Rename functions to reflect its usage.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu.h     |   8 +-
 drivers/iommu/amd/io_pgtable_v2.c |  21 +----
 drivers/iommu/amd/iommu.c         | 131 ++++++++++++++++--------------
 3 files changed, 77 insertions(+), 83 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index ac95481f2339..c712cd086286 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -52,6 +52,11 @@ int amd_iommu_pc_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
 int amd_iommu_pdev_enable_cap_pri(struct pci_dev *pdev);
 void amd_iommu_pdev_disable_cap_pri(struct pci_dev *pdev);
 
+/* GCR3 setup */
+int amd_iommu_set_gcr3(struct iommu_dev_data *dev_data,
+		       ioasid_t pasid, unsigned long gcr3);
+int amd_iommu_clear_gcr3(struct iommu_dev_data *dev_data, ioasid_t pasid);
+
 int amd_iommu_flush_page(struct iommu_domain *dom, u32 pasid, u64 address);
 /*
  * This function flushes all internal caches of
@@ -69,9 +74,6 @@ void amd_iommu_dev_flush_pasid_all(struct iommu_dev_data *dev_data,
 				   ioasid_t pasid);
 
 int amd_iommu_flush_tlb(struct iommu_domain *dom, u32 pasid);
-int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, u32 pasid,
-			      unsigned long cr3);
-int amd_iommu_domain_clear_gcr3(struct iommu_domain *dom, u32 pasid);
 
 #ifdef CONFIG_IRQ_REMAP
 int amd_iommu_create_irq_domain(struct amd_iommu *iommu);
diff --git a/drivers/iommu/amd/io_pgtable_v2.c b/drivers/iommu/amd/io_pgtable_v2.c
index 6d69ba60744f..93489d2db4e8 100644
--- a/drivers/iommu/amd/io_pgtable_v2.c
+++ b/drivers/iommu/amd/io_pgtable_v2.c
@@ -350,38 +350,26 @@ static const struct iommu_flush_ops v2_flush_ops = {
 
 static void v2_free_pgtable(struct io_pgtable *iop)
 {
-	struct protection_domain *pdom;
 	struct amd_io_pgtable *pgtable = container_of(iop, struct amd_io_pgtable, iop);
 
-	pdom = container_of(pgtable, struct protection_domain, iop);
-	if (!(pdom->flags & PD_IOMMUV2_MASK))
+	if (!pgtable || !pgtable->pgd)
 		return;
 
-	/* Clear gcr3 entry */
-	amd_iommu_domain_clear_gcr3(&pdom->domain, 0);
-
-	/* Make changes visible to IOMMUs */
-	amd_iommu_domain_update(pdom);
-
 	/* Free page table */
 	free_pgtable(pgtable->pgd, get_pgtable_level());
+	pgtable->pgd = NULL;
 }
 
 static struct io_pgtable *v2_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
 {
 	struct amd_io_pgtable *pgtable = io_pgtable_cfg_to_data(cfg);
 	struct protection_domain *pdom = (struct protection_domain *)cookie;
-	int ret;
 	int ias = IOMMU_IN_ADDR_BIT_SIZE;
 
 	pgtable->pgd = alloc_pgtable_page(pdom->nid, GFP_ATOMIC);
 	if (!pgtable->pgd)
 		return NULL;
 
-	ret = amd_iommu_domain_set_gcr3(&pdom->domain, 0, iommu_virt_to_phys(pgtable->pgd));
-	if (ret)
-		goto err_free_pgd;
-
 	if (get_pgtable_level() == PAGE_MODE_5_LEVEL)
 		ias = 57;
 
@@ -395,11 +383,6 @@ static struct io_pgtable *v2_alloc_pgtable(struct io_pgtable_cfg *cfg, void *coo
 	cfg->tlb           = &v2_flush_ops;
 
 	return &pgtable->iop;
-
-err_free_pgd:
-	free_pgtable_page(pgtable->pgd);
-
-	return NULL;
 }
 
 struct io_pgtable_init_fns io_pgtable_amd_iommu_v2_init_fns = {
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 85a30b1614de..834c0a79fc17 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1764,10 +1764,13 @@ static int setup_gcr3_table(struct protection_domain *domain, int pasids)
 	return 0;
 }
 
-static u64 *__get_gcr3_pte(u64 *root, int level, u32 pasid, bool alloc)
+static u64 *__get_gcr3_pte(struct gcr3_tbl_info *gcr3_info,
+			   ioasid_t pasid, bool alloc)
 {
 	int index;
 	u64 *pte;
+	u64 *root = gcr3_info->gcr3_tbl;
+	int level = gcr3_info->glx;
 
 	while (true) {
 
@@ -1796,6 +1799,72 @@ static u64 *__get_gcr3_pte(u64 *root, int level, u32 pasid, bool alloc)
 	return pte;
 }
 
+static int __set_gcr3(struct iommu_dev_data *dev_data,
+		      ioasid_t pasid, unsigned long gcr3)
+{
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
+	u64 *pte;
+
+	lockdep_assert_held(&dev_data->lock);
+
+	pte = __get_gcr3_pte(gcr3_info, pasid, true);
+	if (pte == NULL)
+		return -ENOMEM;
+
+	*pte = (gcr3 & PAGE_MASK) | GCR3_VALID;
+	amd_iommu_dev_flush_pasid_all(dev_data, pasid);
+
+	return 0;
+}
+
+int amd_iommu_set_gcr3(struct iommu_dev_data *dev_data, ioasid_t pasid,
+		       unsigned long gcr3)
+{
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
+	int ret;
+
+	spin_lock(&dev_data->lock);
+
+	ret = __set_gcr3(dev_data, pasid, gcr3);
+	if (!ret)
+		gcr3_info->pasid_cnt++;
+
+	spin_unlock(&dev_data->lock);
+	return ret;
+}
+
+static int __clear_gcr3(struct iommu_dev_data *dev_data, ioasid_t pasid)
+{
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
+	u64 *pte;
+
+	lockdep_assert_held(&dev_data->lock);
+
+	pte = __get_gcr3_pte(gcr3_info, pasid, false);
+	if (pte == NULL)
+		return -EINVAL;
+
+	*pte = 0;
+	amd_iommu_dev_flush_pasid_all(dev_data, pasid);
+
+	return 0;
+}
+
+int amd_iommu_clear_gcr3(struct iommu_dev_data *dev_data, ioasid_t pasid)
+{
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
+	int ret;
+
+	spin_lock(&dev_data->lock);
+
+	ret = __clear_gcr3(dev_data, pasid);
+	if (!ret)
+		gcr3_info->pasid_cnt--;
+
+	spin_unlock(&dev_data->lock);
+	return ret;
+}
+
 static void set_dte_entry(struct amd_iommu *iommu,
 			  struct iommu_dev_data *dev_data)
 {
@@ -2838,66 +2907,6 @@ int amd_iommu_flush_tlb(struct iommu_domain *dom, u32 pasid)
 	return ret;
 }
 
-static int __set_gcr3(struct protection_domain *domain, u32 pasid,
-		      unsigned long cr3)
-{
-	u64 *pte;
-
-	if (domain->iop.mode != PAGE_MODE_NONE)
-		return -EINVAL;
-
-	pte = __get_gcr3_pte(domain->gcr3_tbl, domain->glx, pasid, true);
-	if (pte == NULL)
-		return -ENOMEM;
-
-	*pte = (cr3 & PAGE_MASK) | GCR3_VALID;
-
-	return __amd_iommu_flush_tlb(domain, pasid);
-}
-
-static int __clear_gcr3(struct protection_domain *domain, u32 pasid)
-{
-	u64 *pte;
-
-	if (domain->iop.mode != PAGE_MODE_NONE)
-		return -EINVAL;
-
-	pte = __get_gcr3_pte(domain->gcr3_tbl, domain->glx, pasid, false);
-	if (pte == NULL)
-		return 0;
-
-	*pte = 0;
-
-	return __amd_iommu_flush_tlb(domain, pasid);
-}
-
-int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, u32 pasid,
-			      unsigned long cr3)
-{
-	struct protection_domain *domain = to_pdomain(dom);
-	unsigned long flags;
-	int ret;
-
-	spin_lock_irqsave(&domain->lock, flags);
-	ret = __set_gcr3(domain, pasid, cr3);
-	spin_unlock_irqrestore(&domain->lock, flags);
-
-	return ret;
-}
-
-int amd_iommu_domain_clear_gcr3(struct iommu_domain *dom, u32 pasid)
-{
-	struct protection_domain *domain = to_pdomain(dom);
-	unsigned long flags;
-	int ret;
-
-	spin_lock_irqsave(&domain->lock, flags);
-	ret = __clear_gcr3(domain, pasid);
-	spin_unlock_irqrestore(&domain->lock, flags);
-
-	return ret;
-}
-
 int amd_iommu_complete_ppr(struct pci_dev *pdev, u32 pasid,
 			   int status, int tag)
 {
-- 
2.31.1

[PATCH v4 11/16] iommu/amd: Refactor attaching / detaching device functions

[Vasant Hegde]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

If default domain is configured with V2 page table then setup default GCR3
with domain GCR3 pointer. So that all devices in the domain uses same page
table for translation. Also return page table setup status from do_attach()
function.

Note that current patch configures GCR3 table with MAX supported PASIDs.
Ideally it should use 1 level PASID table as its using PASID zero only.
Later in SVA enable path it should be reconfigured to support MAX PASIDs.
This will be fixed later once SVA support is upstreamed.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 834c0a79fc17..2f4ac415e8d8 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1973,10 +1973,11 @@ static void clear_dte_entry(struct amd_iommu *iommu, u16 devid)
 	amd_iommu_apply_erratum_63(iommu, devid);
 }
 
-static void do_attach(struct iommu_dev_data *dev_data,
-		      struct protection_domain *domain)
+static int do_attach(struct iommu_dev_data *dev_data,
+		     struct protection_domain *domain)
 {
 	struct amd_iommu *iommu;
+	int ret = 0;
 
 	iommu = get_amd_iommu_from_dev(dev_data->dev);
 
@@ -1996,11 +1997,31 @@ static void do_attach(struct iommu_dev_data *dev_data,
 	if (domain_id_is_per_dev(domain))
 		dev_data->domid = domain_id_alloc();
 
+	/* Init GCR3 table and update device table */
+	if (domain->pd_mode == PD_MODE_V2) {
+		/*
+		 * By default, setup GCR3 table to support MAX PASIDs
+		 * support by the IOMMU HW.
+		 */
+		ret = setup_gcr3_table(dev_data->domain, -1);
+		if (ret)
+			return ret;
+
+		ret = __set_gcr3(dev_data, 0,
+				 iommu_virt_to_phys(domain->iop.pgd));
+		if (ret) {
+			free_gcr3_table(dev_data->domain);
+			return ret;
+		}
+	}
+
 	/* Update device table */
 	set_dte_entry(iommu, dev_data);
 	clone_aliases(iommu, dev_data->dev);
 
 	device_flush_dte(dev_data);
+
+	return ret;
 }
 
 static void do_detach(struct iommu_dev_data *dev_data)
@@ -2010,6 +2031,12 @@ static void do_detach(struct iommu_dev_data *dev_data)
 
 	iommu = get_amd_iommu_from_dev(dev_data->dev);
 
+	/* Clear GCR3 table */
+	if (domain->pd_mode == PD_MODE_V2) {
+		__clear_gcr3(dev_data, 0);
+		free_gcr3_table(dev_data->domain);
+	}
+
 	/* Update data structures */
 	dev_data->domain = NULL;
 	list_del(&dev_data->list);
@@ -2056,7 +2083,7 @@ static int attach_device(struct device *dev,
 	if (dev_is_pci(dev))
 		pdev_enable_caps(to_pci_dev(dev));
 
-	do_attach(dev_data, domain);
+	ret = do_attach(dev_data, domain);
 
 out:
 	spin_unlock(&dev_data->lock);
-- 
2.31.1

[PATCH v4 12/16] iommu/amd: Refactor protection_domain helper functions

[Vasant Hegde]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

To removes the code to setup GCR3 table, and only handle domain
create / destroy, since GCR3 is no longer part of a domain.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/iommu.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 2f4ac415e8d8..584608b98f26 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2264,9 +2264,6 @@ static void protection_domain_free(struct protection_domain *domain)
 	if (domain->iop.pgtbl_cfg.tlb)
 		free_io_pgtable_ops(&domain->iop.iop.ops);
 
-	if (domain->flags & PD_IOMMUV2_MASK)
-		free_gcr3_table(domain);
-
 	if (domain->iop.root)
 		free_page((unsigned long)domain->iop.root);
 
@@ -2294,15 +2291,10 @@ static int protection_domain_init_v1(struct protection_domain *domain, int mode)
 	return 0;
 }
 
-static int protection_domain_init_v2(struct protection_domain *domain)
+static int protection_domain_init_v2(struct protection_domain *pdom)
 {
-	domain->flags |= PD_GIOV_MASK;
-	domain->pd_mode = PD_MODE_V2;
-
-	domain->domain.pgsize_bitmap = AMD_IOMMU_PGSIZES_V2;
-
-	if (setup_gcr3_table(domain, 1))
-		return -ENOMEM;
+	pdom->pd_mode = PD_MODE_V2;
+	pdom->domain.pgsize_bitmap = AMD_IOMMU_PGSIZES_V2;
 
 	return 0;
 }
-- 
2.31.1

[PATCH v4 13/16] iommu/amd: Refactor GCR3 table helper functions

[Vasant Hegde]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

To use the new per-device struct gcr3_tbl_info. Use GFP_KERNEL flag
instead of GFP_ATOMIC for GCR3 table allocation. Also modify
set_dte_entry() to use new per device GCR3 table.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu.c | 55 +++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 22 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 584608b98f26..7e5f32817931 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -79,6 +79,9 @@ 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);
+
 /****************************************************************************
  *
  * Helper functions
@@ -1716,16 +1719,21 @@ static void free_gcr3_tbl_level2(u64 *tbl)
 	}
 }
 
-static void free_gcr3_table(struct protection_domain *domain)
+static void free_gcr3_table(struct iommu_dev_data *dev_data)
 {
-	if (domain->glx == 2)
-		free_gcr3_tbl_level2(domain->gcr3_tbl);
-	else if (domain->glx == 1)
-		free_gcr3_tbl_level1(domain->gcr3_tbl);
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
+
+	if (gcr3_info->glx == 2)
+		free_gcr3_tbl_level2(gcr3_info->gcr3_tbl);
+	else if (gcr3_info->glx == 1)
+		free_gcr3_tbl_level1(gcr3_info->gcr3_tbl);
 	else
-		BUG_ON(domain->glx != 0);
+		WARN_ON_ONCE(gcr3_info->glx != 0);
+
+	gcr3_info->glx = 0;
 
-	free_page((unsigned long)domain->gcr3_tbl);
+	free_page((unsigned long)gcr3_info->gcr3_tbl);
+	gcr3_info->gcr3_tbl = NULL;
 }
 
 /*
@@ -1744,22 +1752,23 @@ static int get_gcr3_levels(int pasids)
 	return levels ? (DIV_ROUND_UP(levels, 9) - 1) : levels;
 }
 
-/* Note: This function expects iommu_domain->lock to be held prior calling the function. */
-static int setup_gcr3_table(struct protection_domain *domain, int pasids)
+static int setup_gcr3_table(struct iommu_dev_data *dev_data, int pasids)
 {
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
 	int levels = get_gcr3_levels(pasids);
 
 	if (levels > amd_iommu_max_glx_val)
 		return -EINVAL;
 
-	domain->gcr3_tbl = alloc_pgtable_page(domain->nid, GFP_ATOMIC);
-	if (domain->gcr3_tbl == NULL)
-		return -ENOMEM;
+	if (gcr3_info->gcr3_tbl)
+		return -EBUSY;
 
-	domain->glx      = levels;
-	domain->flags   |= PD_IOMMUV2_MASK;
+	gcr3_info->gcr3_tbl = alloc_pgtable_page(dev_to_node(dev_data->dev),
+						 GFP_KERNEL);
+	if (gcr3_info->gcr3_tbl == NULL)
+		return -ENOMEM;
 
-	amd_iommu_domain_update(domain);
+	gcr3_info->glx = levels;
 
 	return 0;
 }
@@ -1875,6 +1884,7 @@ static void set_dte_entry(struct amd_iommu *iommu,
 	u16 domid;
 	struct protection_domain *domain = dev_data->domain;
 	struct dev_table_entry *dev_table = get_dev_table(iommu);
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
 
 	if (domain_id_is_per_dev(domain))
 		domid = dev_data->domid;
@@ -1907,9 +1917,9 @@ static void set_dte_entry(struct amd_iommu *iommu,
 	if (domain->dirty_tracking)
 		pte_root |= DTE_FLAG_HAD;
 
-	if (domain->flags & PD_IOMMUV2_MASK) {
-		u64 gcr3 = iommu_virt_to_phys(domain->gcr3_tbl);
-		u64 glx  = domain->glx;
+	if (gcr3_info && gcr3_info->gcr3_tbl) {
+		u64 gcr3 = iommu_virt_to_phys(gcr3_info->gcr3_tbl);
+		u64 glx  = gcr3_info->glx;
 		u64 tmp;
 
 		pte_root |= DTE_FLAG_GV;
@@ -1937,7 +1947,8 @@ static void set_dte_entry(struct amd_iommu *iommu,
 				((u64)GUEST_PGTABLE_5_LEVEL << DTE_GPT_LEVEL_SHIFT);
 		}
 
-		if (domain->flags & PD_GIOV_MASK)
+		/* GIOV is supported with V2 page table mode only */
+		if (pdom_is_v2_pgtbl_mode(domain))
 			pte_root |= DTE_FLAG_GIOV;
 	}
 
@@ -2003,14 +2014,14 @@ static int do_attach(struct iommu_dev_data *dev_data,
 		 * By default, setup GCR3 table to support MAX PASIDs
 		 * support by the IOMMU HW.
 		 */
-		ret = setup_gcr3_table(dev_data->domain, -1);
+		ret = setup_gcr3_table(dev_data, -1);
 		if (ret)
 			return ret;
 
 		ret = __set_gcr3(dev_data, 0,
 				 iommu_virt_to_phys(domain->iop.pgd));
 		if (ret) {
-			free_gcr3_table(dev_data->domain);
+			free_gcr3_table(dev_data);
 			return ret;
 		}
 	}
@@ -2034,7 +2045,7 @@ static void do_detach(struct iommu_dev_data *dev_data)
 	/* Clear GCR3 table */
 	if (domain->pd_mode == PD_MODE_V2) {
 		__clear_gcr3(dev_data, 0);
-		free_gcr3_table(dev_data->domain);
+		free_gcr3_table(dev_data);
 	}
 
 	/* Update data structures */
-- 
2.31.1

[PATCH v4 14/16] iommu/amd: Remove unused flush pasid functions

[Vasant Hegde]

We have removed iommu_v2 module and converted v2 page table to use
common flush functions. Also we have moved GCR3 table to per device.
PASID related functions are not used. Hence remove these unused
functions.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu.h |   3 -
 drivers/iommu/amd/iommu.c     | 100 ----------------------------------
 2 files changed, 103 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index c712cd086286..bbed268e8abc 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -57,7 +57,6 @@ int amd_iommu_set_gcr3(struct iommu_dev_data *dev_data,
 		       ioasid_t pasid, unsigned long gcr3);
 int amd_iommu_clear_gcr3(struct iommu_dev_data *dev_data, ioasid_t pasid);
 
-int amd_iommu_flush_page(struct iommu_domain *dom, u32 pasid, u64 address);
 /*
  * This function flushes all internal caches of
  * the IOMMU used by this driver.
@@ -73,8 +72,6 @@ void amd_iommu_dev_flush_pasid_pages(struct iommu_dev_data *dev_data,
 void amd_iommu_dev_flush_pasid_all(struct iommu_dev_data *dev_data,
 				   ioasid_t pasid);
 
-int amd_iommu_flush_tlb(struct iommu_domain *dom, u32 pasid);
-
 #ifdef CONFIG_IRQ_REMAP
 int amd_iommu_create_irq_domain(struct amd_iommu *iommu);
 #else
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 7e5f32817931..13cfea126800 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2837,106 +2837,6 @@ const struct iommu_ops amd_iommu_ops = {
 	}
 };
 
-static int __flush_pasid(struct protection_domain *domain, u32 pasid,
-			 u64 address, size_t size)
-{
-	struct iommu_dev_data *dev_data;
-	struct iommu_cmd cmd;
-	int i, ret;
-
-	if (!(domain->flags & PD_IOMMUV2_MASK))
-		return -EINVAL;
-
-	build_inv_iommu_pages(&cmd, address, size, domain->id, pasid, true);
-
-	/*
-	 * IOMMU TLB needs to be flushed before Device TLB to
-	 * prevent device TLB refill from IOMMU TLB
-	 */
-	for (i = 0; i < amd_iommu_get_num_iommus(); ++i) {
-		if (domain->dev_iommu[i] == 0)
-			continue;
-
-		ret = iommu_queue_command(amd_iommus[i], &cmd);
-		if (ret != 0)
-			goto out;
-	}
-
-	/* Wait until IOMMU TLB flushes are complete */
-	amd_iommu_domain_flush_complete(domain);
-
-	/* Now flush device TLBs */
-	list_for_each_entry(dev_data, &domain->dev_list, list) {
-		struct amd_iommu *iommu;
-		int qdep;
-
-		/*
-		   There might be non-IOMMUv2 capable devices in an IOMMUv2
-		 * domain.
-		 */
-		if (!dev_data->ats_enabled)
-			continue;
-
-		qdep  = dev_data->ats_qdep;
-		iommu = rlookup_amd_iommu(dev_data->dev);
-		if (!iommu)
-			continue;
-		build_inv_iotlb_pages(&cmd, dev_data->devid, qdep,
-				      address, size, pasid, true);
-
-		ret = iommu_queue_command(iommu, &cmd);
-		if (ret != 0)
-			goto out;
-	}
-
-	/* Wait until all device TLBs are flushed */
-	amd_iommu_domain_flush_complete(domain);
-
-	ret = 0;
-
-out:
-
-	return ret;
-}
-
-static int __amd_iommu_flush_page(struct protection_domain *domain, u32 pasid,
-				  u64 address)
-{
-	return __flush_pasid(domain, pasid, address, PAGE_SIZE);
-}
-
-int amd_iommu_flush_page(struct iommu_domain *dom, u32 pasid,
-			 u64 address)
-{
-	struct protection_domain *domain = to_pdomain(dom);
-	unsigned long flags;
-	int ret;
-
-	spin_lock_irqsave(&domain->lock, flags);
-	ret = __amd_iommu_flush_page(domain, pasid, address);
-	spin_unlock_irqrestore(&domain->lock, flags);
-
-	return ret;
-}
-
-static int __amd_iommu_flush_tlb(struct protection_domain *domain, u32 pasid)
-{
-	return __flush_pasid(domain, pasid, 0, CMD_INV_IOMMU_ALL_PAGES_ADDRESS);
-}
-
-int amd_iommu_flush_tlb(struct iommu_domain *dom, u32 pasid)
-{
-	struct protection_domain *domain = to_pdomain(dom);
-	unsigned long flags;
-	int ret;
-
-	spin_lock_irqsave(&domain->lock, flags);
-	ret = __amd_iommu_flush_tlb(domain, pasid);
-	spin_unlock_irqrestore(&domain->lock, flags);
-
-	return ret;
-}
-
 int amd_iommu_complete_ppr(struct pci_dev *pdev, u32 pasid,
 			   int status, int tag)
 {
-- 
2.31.1

[PATCH v4 15/16] iommu/amd: Rearrange device flush code

[Vasant Hegde]

Consolidate all flush related code in one place so that its easy
to maintain.

No functional changes intended.

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

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 13cfea126800..f2a748904eed 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1655,6 +1655,49 @@ static void domain_flush_devices(struct protection_domain *domain)
 		device_flush_dte(dev_data);
 }
 
+static void update_device_table(struct protection_domain *domain)
+{
+	struct iommu_dev_data *dev_data;
+
+	list_for_each_entry(dev_data, &domain->dev_list, list) {
+		struct amd_iommu *iommu = rlookup_amd_iommu(dev_data->dev);
+
+		set_dte_entry(iommu, dev_data);
+		clone_aliases(iommu, dev_data->dev);
+	}
+}
+
+void amd_iommu_update_and_flush_device_table(struct protection_domain *domain)
+{
+	update_device_table(domain);
+	domain_flush_devices(domain);
+}
+
+void amd_iommu_domain_update(struct protection_domain *domain)
+{
+	/* Update device table */
+	amd_iommu_update_and_flush_device_table(domain);
+
+	/* Flush domain TLB(s) and wait for completion */
+	amd_iommu_domain_flush_all(domain);
+}
+
+int amd_iommu_complete_ppr(struct pci_dev *pdev, u32 pasid,
+			   int status, int tag)
+{
+	struct iommu_dev_data *dev_data;
+	struct amd_iommu *iommu;
+	struct iommu_cmd cmd;
+
+	dev_data = dev_iommu_priv_get(&pdev->dev);
+	iommu    = get_amd_iommu_from_dev(&pdev->dev);
+
+	build_complete_ppr(&cmd, dev_data->devid, pasid, status,
+			   tag, dev_data->pri_tlp);
+
+	return iommu_queue_command(iommu, &cmd);
+}
+
 /****************************************************************************
  *
  * The next functions belong to the domain allocation. A domain is
@@ -2206,39 +2249,6 @@ static struct iommu_group *amd_iommu_device_group(struct device *dev)
 	return acpihid_device_group(dev);
 }
 
-/*****************************************************************************
- *
- * The next functions belong to the dma_ops mapping/unmapping code.
- *
- *****************************************************************************/
-
-static void update_device_table(struct protection_domain *domain)
-{
-	struct iommu_dev_data *dev_data;
-
-	list_for_each_entry(dev_data, &domain->dev_list, list) {
-		struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
-
-		set_dte_entry(iommu, dev_data);
-		clone_aliases(iommu, dev_data->dev);
-	}
-}
-
-void amd_iommu_update_and_flush_device_table(struct protection_domain *domain)
-{
-	update_device_table(domain);
-	domain_flush_devices(domain);
-}
-
-void amd_iommu_domain_update(struct protection_domain *domain)
-{
-	/* Update device table */
-	amd_iommu_update_and_flush_device_table(domain);
-
-	/* Flush domain TLB(s) and wait for completion */
-	amd_iommu_domain_flush_all(domain);
-}
-
 /*****************************************************************************
  *
  * The following functions belong to the exported interface of AMD IOMMU
@@ -2837,22 +2847,6 @@ const struct iommu_ops amd_iommu_ops = {
 	}
 };
 
-int amd_iommu_complete_ppr(struct pci_dev *pdev, u32 pasid,
-			   int status, int tag)
-{
-	struct iommu_dev_data *dev_data;
-	struct amd_iommu *iommu;
-	struct iommu_cmd cmd;
-
-	dev_data = dev_iommu_priv_get(&pdev->dev);
-	iommu    = get_amd_iommu_from_dev(&pdev->dev);
-
-	build_complete_ppr(&cmd, dev_data->devid, pasid, status,
-			   tag, dev_data->pri_tlp);
-
-	return iommu_queue_command(iommu, &cmd);
-}
-
 #ifdef CONFIG_IRQ_REMAP
 
 /*****************************************************************************
-- 
2.31.1

[PATCH v4 16/16] iommu/amd: Remove unused GCR3 table parameters from struct protection_domain

[Vasant Hegde]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Since they are moved to struct iommu_dev_data, and the driver has been
ported to use them.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/amd_iommu_types.h | 12 ------------
 drivers/iommu/amd/iommu.c           |  2 +-
 2 files changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 51daf5dd4729..3dc39bbc05fc 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -453,15 +453,6 @@
 
 #define MAX_DOMAIN_ID 65536
 
-/* Protection domain flags */
-#define PD_DMA_OPS_MASK		BIT(0) /* domain used for dma_ops */
-#define PD_DEFAULT_MASK		BIT(1) /* domain is a default dma_ops
-					      domain for an IOMMU */
-#define PD_PASSTHROUGH_MASK	BIT(2) /* domain has no page
-					      translation */
-#define PD_IOMMUV2_MASK		BIT(3) /* domain has gcr3 table */
-#define PD_GIOV_MASK		BIT(4) /* domain enable GIOV support */
-
 /* Timeout stuff */
 #define LOOP_TIMEOUT		100000
 #define MMIO_STATUS_TIMEOUT	2000000
@@ -571,10 +562,7 @@ struct protection_domain {
 	struct amd_io_pgtable iop;
 	spinlock_t lock;	/* mostly used to lock the page table*/
 	u16 id;			/* the domain id written to the device table */
-	int glx;		/* Number of levels for GCR3 table */
 	int nid;		/* Node ID */
-	u64 *gcr3_tbl;		/* Guest CR3 table */
-	unsigned long flags;	/* flags to find out type of domain */
 	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 */
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index f2a748904eed..4e4ff1550cf3 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -90,7 +90,7 @@ static void set_dte_entry(struct amd_iommu *iommu,
 
 static inline bool pdom_is_v2_pgtbl_mode(struct protection_domain *pdom)
 {
-	return (pdom && (pdom->flags & PD_IOMMUV2_MASK));
+	return (pdom && (pdom->pd_mode == PD_MODE_V2));
 }
 
 /*
-- 
2.31.1

Re: [PATCH v4 02/16] iommu/amd: Enable Guest Translation before registering devices

[Jason Gunthorpe]

On Tue, Dec 12, 2023 at 08:52:10AM +0000, Vasant Hegde wrote:
> IOMMU Guest Translation (GT) feature needs to be enabled before
> invalidating guest translations (CMD_INV_IOMMU_PAGES with GN=1).
> 
> Currently GT feature is enabled after setting up interrupt handler.
> So far it was fine as we were not invalidating guest page table
> before this point.
> 
> Upcoming series will introduce per device GCR3 table and it will
> invalidate guest pages after configuring. Hence move GT feature
> enablement to early_enable_iommu().
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/init.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

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

Jason

Re: [PATCH v4 03/16] iommu/amd: Introduce get_amd_iommu_from_dev()

[Jason Gunthorpe]

On Tue, Dec 12, 2023 at 08:52:11AM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> And replace rlookup_amd_iommu() with the new helper function where
> applicable to avoid unnecessary loop to look up struct amd_iommu from
> struct device.
> 
> Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu.h | 11 +++++++++
>  drivers/iommu/amd/iommu.c     | 43 ++++++++++-------------------------
>  include/linux/iommu.h         | 13 +++++++++++
>  3 files changed, 36 insertions(+), 31 deletions(-)

This is basically fine

But can you improve the core helper a bit so everyone else can easially use it too:

static inline struct iommu_device *__iommu_get_iommu_dev(struct device *dev)
{
	return dev->iommu->iommu_dev;
}

#define iommu_get_iommu_dev(dev, type, member) \
	container_of(__iommu_get_iommu_dev(dev), type, member)

static inline struct amd_iommu *get_amd_iommu_from_dev(struct device *dev)
{
	return iommu_get_iommu_dev(dev, struct amd_iommu, iommu);
}

static inline struct amd_iommu *get_amd_iommu_from_dev_data(struct iommu_dev_data *dev_data)
{
	return iommu_get_iommu_dev(dev_data->dev, struct amd_iommu, iommu);
}

> @@ -1813,9 +1809,7 @@ static void do_attach(struct iommu_dev_data *dev_data,
>  {
>  	struct amd_iommu *iommu;
>  
> -	iommu = rlookup_amd_iommu(dev_data->dev);
> -	if (!iommu)
> -		return;
> +	iommu = get_amd_iommu_from_dev(dev_data->dev);

And I suggest to stylistically keep with the common pattern of
initializing the casts at the top of the function

	     struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
	     struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);

Jason

Re: [PATCH v4 06/16] iommu/amd: Use protection_domain.flags to check page table mode

[Jason Gunthorpe]

On Tue, Dec 12, 2023 at 08:52:14AM +0000, Vasant Hegde wrote:
> Page table mode (v1, v2 or pt) is per domain property. Recently we have
> enhanced protection_domain.pd_mode to track per domain page table mode.
> Use that variable to check the page table mode instead of global
> 'amd_iommu_pgtable' in {map/unmap}_pages path.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/amd/iommu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

Jason

Re: [PATCH v4 07/16] iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue

[Jason Gunthorpe]

On Tue, Dec 12, 2023 at 08:52:15AM +0000, Vasant Hegde wrote:
> With v1 page table, the AMD IOMMU spec states that the hardware must use
> the domain ID to tag its internal translation caches. I/O devices with
> different v1 page tables must be given different domain IDs. I/O devices
> that share the same v1 page table __may__ be given the same domain ID.
> This domain ID management policy is currently implemented by the AMD
> IOMMU driver. In this case, only the domain ID is needed when issuing the
> INVALIDATE_IOMMU_PAGES command to invalidate the IOMMU translation cache
> (TLB).
> 
> With v2 page table, the hardware uses domain ID and PASID as parameters
> to tag and issue the INVALIDATE_IOMMU_PAGES command. Since the GCR3 table
> is setup per-device, and there is no guarantee for PASID to be unique
> across multiple devices. The same PASID for different devices could
> have different v2 page tables. In such case, if multiple devices share the
> same domain ID, IOMMU translation cache for these devices would be polluted
> due to TLB aliasing.
> 
> Hence, avoid the TLB aliasing issue with v2 page table by allocating unique
> domain ID for each device even when multiple devices are sharing the same v1
> page table. Please note that this workaround would result in multiple
> INVALIDATE_IOMMU_PAGES commands (one per domain id) when unmapping a
> translation on the shared v1 page table.

Huh? This is a typo? "the same v2 page table"  "unmapping a
translation on the shared v2 page table" ??

The code seems to be fine, domain_flush_pages_v1() looks optimal?

> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index fead9033796f..51daf5dd4729 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -842,6 +842,8 @@ struct iommu_dev_data {
>  	u8 ppr          :1;		  /* Enable device PPR support */
>  	bool use_vapic;			  /* Enable device to use vapic mode */
>  	bool defer_attach;
> +	/* Per device domain ID. Used with V2 page table */
> +	u16 domid;

This should really be put into the 'struct gcr3_tbl_info' - logically
that is the struct the HW cache tag is linked to. ie if the gcr3 table
is the same pointer then the cache tag can be re-used by the HW.

Then when you want to optimize for the no-pasid case then the right
way to do it is putting a 'struct gcr3_tbl_info' inside the v2
protection_domain.

The DTE will point at the v2 protection_domain's version of the gcr3
if the PASID table is empty, otherwise the DTE will point at the
struct iommu_dev_data version of the gcr3 table.

Naturally this will optimize the lifecycle of the domain_id.

Then put the domain_id alloc/dealloc inside the functions that
alloc/free the memory under the struct gcr3_tbl_info - ie it is part
of the gcr3 layer.

> +/*
> + * Allocate per device domain ID when using V2 page table
> + */
> +static inline bool domain_id_is_per_dev(struct protection_domain *pdom)
> +{
> +	return (pdom && pdom->pd_mode != PD_MODE_V1);
> +}

Under that view this function probably would make more sense as:

domain_requires_gcr3(pdom)

But frankly I'd just stick with pdom_is_v2_pgtbl_mode().

Also pdom should never be null when this is called, right?

> -/*
> - * TLB invalidation function which is called from the mapping functions.
> - * It invalidates a single PTE if the range to flush is within a single
> - * page. Otherwise it flushes the whole TLB of the IOMMU.
> - */
> -static void __domain_flush_pages(struct protection_domain *domain,
> +static int domain_flush_pages_v2(struct protection_domain *pdom,
>  				 u64 address, size_t size)
>  {
>  	struct iommu_dev_data *dev_data;
>  	struct iommu_cmd cmd;
> -	int ret = 0, i;
> -	ioasid_t pasid = IOMMU_NO_PASID;
> -	bool gn = false;
> +	int ret = 0;
>  
> -	if (pdom_is_v2_pgtbl_mode(domain))
> -		gn = true;
> +	list_for_each_entry(dev_data, &pdom->dev_list, list) {
> +		struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
> +
> +		build_inv_iommu_pages(&cmd, address, size,
> +				      dev_data->domid, IOMMU_NO_PASID, true);
> +
> +		ret |= iommu_queue_command(iommu, &cmd);
> +	}

This is fine for where things are here, but what you want to get to
long term is what I've been talking about of having the attachment
list on the protection_domain.

Each attachment entry would hold:
  struct device *dev;
  unsigned int pasid;

Keep the list sorted by (iommu, gcr.domain_id).

Then optimized invalidation is simply this:

for_each:
  if (get_amd_iommu_from_dev(entry->dev) == last_iommu &&
      get_gcr(entry, pdom)->domain_id == last_domain_id)
     continue;
  build_inv_iommu_pages(..)
  last_iommu = get_amd_iommu_from_dev(entry->dev)
  domain_id = get_gcr(entry, pdom)->domain_id;

And this algorithm matches what Intel and SMMU need and I'm strongly
thinking about making a driver utility library to handle it, so
we can revisit this later on.

> +static int domain_flush_pages_v1(struct protection_domain *pdom,
> +				 u64 address, size_t size)
> +{
> +	struct iommu_cmd cmd;
> +	int ret = 0, i;
>  
> -	build_inv_iommu_pages(&cmd, address, size, domain->id, pasid, gn);
> +	build_inv_iommu_pages(&cmd, address, size,
> +			      pdom->id, IOMMU_NO_PASID, false);
>  
>  	for (i = 0; i < amd_iommu_get_num_iommus(); ++i) {
> -		if (!domain->dev_iommu[i])
> +		if (!pdom->dev_iommu[i])
>  			continue;

Right, iterate over every iommu

Jason

Re: [PATCH v4 08/16] iommu/amd: Add support for device based TLB invalidation

[Jason Gunthorpe]

On Tue, Dec 12, 2023 at 08:52:16AM +0000, Vasant Hegde wrote:
> Add support to invalidate TLB/IOTLB for the given device.
> 
> These functions will be used in subsequent patches where we will
> introduce per device GCR3 table and SVA support.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu.h |  5 +++++
>  drivers/iommu/amd/iommu.c     | 23 +++++++++++++++++++++++
>  2 files changed, 28 insertions(+)

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

Jason

Re: [PATCH v4 09/16] iommu/amd: Rearrange GCR3 table setup code

[Jason Gunthorpe]

On Tue, Dec 12, 2023 at 08:52:17AM +0000, Vasant Hegde wrote:
> Consolidate GCR3 table related code in one place so that its easy
> to maintain.
> 
> Note that this patch doesn't move __set_gcr3/__clear_gcr3. We are moving
> GCR3 table from per domain to per device. Following series will rework
> these functions. During that time I will move these functions as well.
> 
> No functional changes intended.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 64 +++++++++++++++++++--------------------
>  1 file changed, 32 insertions(+), 32 deletions(-)

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

Usual comment about doing code motion without a good reason..

Jason

Re: [PATCH v4 10/16] iommu/amd: Refactor helper function for setting / clearing GCR3

[Jason Gunthorpe]

On Tue, Dec 12, 2023 at 08:52:18AM +0000, Vasant Hegde wrote:
> +static int __set_gcr3(struct iommu_dev_data *dev_data,
> +		      ioasid_t pasid, unsigned long gcr3)
> +{

IMHO you've got the ATS layering wrong here.. The ATS invalidation
should be pushed out by attach/detach functions and has to be
carefully sequenced with the ATS disable bit in the PCI control
register. I don't think you can do all of this right with things
organized like this. Indeed this looks like it over invalidates the
ATS quite a bit.

You should only need to invalidate prior to doing the enable and when
a GCR3 value is changed while ATS is turned on, which is something
that the attach op can caculate.

So, these functions should have a signature of:

(struct amd_iommu *iommu, struct struct gcr3_tbl_info *gcr3_info, ...)

[and the locking can implicitly rely on the core's group lock, don't
need more locks]

> +static int __clear_gcr3(struct iommu_dev_data *dev_data, ioasid_t pasid)
> +{
> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
> +	u64 *pte;
> +
> +	lockdep_assert_held(&dev_data->lock);
> +
> +	pte = __get_gcr3_pte(gcr3_info, pasid, false);
> +	if (pte == NULL)
> +		return -EINVAL;
> +
> +	*pte = 0;
> +	amd_iommu_dev_flush_pasid_all(dev_data, pasid);
> +
> +	return 0;
> +}

What is the point of clear? By the time the attach ops will want to do
clear it is already certain that a non-zero value was installed in
pasid, and this doesn't free any memory, so what is the point of
'alloc=false'?

Just do set with a 0 value?

Jason

Re: [PATCH v4 11/16] iommu/amd: Refactor attaching / detaching device functions

[Jason Gunthorpe]

On Tue, Dec 12, 2023 at 08:52:19AM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> If default domain is configured with V2 page table then setup default GCR3
> with domain GCR3 pointer. So that all devices in the domain uses same page
> table for translation. Also return page table setup status from do_attach()
> function.
> 
> Note that current patch configures GCR3 table with MAX supported PASIDs.
> Ideally it should use 1 level PASID table as its using PASID zero only.
> Later in SVA enable path it should be reconfigured to support MAX PASIDs.
> This will be fixed later once SVA support is upstreamed.

??

Given this driver doesn't support PASID at this point you should just
allocate a 1 entry table in this patch.

When PASID support is added then you have to allocate a MAX pasid
table or implement atomic resize.

The patch looks fine otherwise

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

Jason

Re: [PATCH v4 13/16] iommu/amd: Refactor GCR3 table helper functions

[Jason Gunthorpe]

On Tue, Dec 12, 2023 at 08:52:21AM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> To use the new per-device struct gcr3_tbl_info. Use GFP_KERNEL flag
> instead of GFP_ATOMIC for GCR3 table allocation. Also modify
> set_dte_entry() to use new per device GCR3 table.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 55 +++++++++++++++++++++++----------------
>  1 file changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 584608b98f26..7e5f32817931 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -79,6 +79,9 @@ 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);
> +
>  /****************************************************************************
>   *
>   * Helper functions
> @@ -1716,16 +1719,21 @@ static void free_gcr3_tbl_level2(u64 *tbl)
>  	}
>  }
>  
> -static void free_gcr3_table(struct protection_domain *domain)
> +static void free_gcr3_table(struct iommu_dev_data *dev_data)
>  {

Pass in struct gcr3_tbl_info not dev_data

> -/* Note: This function expects iommu_domain->lock to be held prior calling the function. */
> -static int setup_gcr3_table(struct protection_domain *domain, int pasids)
> +static int setup_gcr3_table(struct iommu_dev_data *dev_data, int pasids)
>  {
> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
>  	int levels = get_gcr3_levels(pasids);

Pass in struct gcr3_tbl_info not dev_data

I don't think it is worth re-doing at this point - but I probably
would have tried to structure this series as creating the struct
gcr3_tbl_info and migrating the protection_domain to use it, then
streamlined the APIs to have a clean gcr3_tbl_info layer, finally
adding a gcr3_tbl_info to the iommu_dev_data and using it when
appropriate.

Jason

Re: [PATCH v4 14/16] iommu/amd: Remove unused flush pasid functions

[Jason Gunthorpe]

On Tue, Dec 12, 2023 at 08:52:22AM +0000, Vasant Hegde wrote:
> We have removed iommu_v2 module and converted v2 page table to use
> common flush functions. Also we have moved GCR3 table to per device.
> PASID related functions are not used. Hence remove these unused
> functions.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu.h |   3 -
>  drivers/iommu/amd/iommu.c     | 100 ----------------------------------
>  2 files changed, 103 deletions(-)

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

Jason

Re: [PATCH v4 15/16] iommu/amd: Rearrange device flush code

[Jason Gunthorpe]

On Tue, Dec 12, 2023 at 08:52:23AM +0000, Vasant Hegde wrote:
> Consolidate all flush related code in one place so that its easy
> to maintain.
> 
> No functional changes intended.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 92 ++++++++++++++++++---------------------
>  1 file changed, 43 insertions(+), 49 deletions(-)

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

Usual remark about not moving code without good reason..

Jason

Re: [PATCH v4 03/16] iommu/amd: Introduce get_amd_iommu_from_dev()

[Vasant Hegde]

Jason,

On 1/5/2024 11:57 PM, Jason Gunthorpe wrote:
> On Tue, Dec 12, 2023 at 08:52:11AM +0000, Vasant Hegde wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> And replace rlookup_amd_iommu() with the new helper function where
>> applicable to avoid unnecessary loop to look up struct amd_iommu from
>> struct device.
>>
>> Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/amd_iommu.h | 11 +++++++++
>>  drivers/iommu/amd/iommu.c     | 43 ++++++++++-------------------------
>>  include/linux/iommu.h         | 13 +++++++++++
>>  3 files changed, 36 insertions(+), 31 deletions(-)
> 
> This is basically fine
> 
> But can you improve the core helper a bit so everyone else can easially use it too:>
> static inline struct iommu_device *__iommu_get_iommu_dev(struct device *dev)
> {
> 	return dev->iommu->iommu_dev;
> }
> 
> #define iommu_get_iommu_dev(dev, type, member) \
> 	container_of(__iommu_get_iommu_dev(dev), type, member)
> 
> static inline struct amd_iommu *get_amd_iommu_from_dev(struct device *dev)
> {
> 	return iommu_get_iommu_dev(dev, struct amd_iommu, iommu);
> }
> 
> static inline struct amd_iommu *get_amd_iommu_from_dev_data(struct iommu_dev_data *dev_data)
> {
> 	return iommu_get_iommu_dev(dev_data->dev, struct amd_iommu, iommu);
> }

Makes sense. Fixed in v5.

-Vasant

Re: [PATCH v4 13/16] iommu/amd: Refactor GCR3 table helper functions

[Vasant Hegde]

Jason,


On 1/6/2024 12:51 AM, Jason Gunthorpe wrote:
> On Tue, Dec 12, 2023 at 08:52:21AM +0000, Vasant Hegde wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> To use the new per-device struct gcr3_tbl_info. Use GFP_KERNEL flag
>> instead of GFP_ATOMIC for GCR3 table allocation. Also modify
>> set_dte_entry() to use new per device GCR3 table.
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/iommu.c | 55 +++++++++++++++++++++++----------------
>>  1 file changed, 33 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 584608b98f26..7e5f32817931 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -79,6 +79,9 @@ 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);
>> +
>>  /****************************************************************************
>>   *
>>   * Helper functions
>> @@ -1716,16 +1719,21 @@ static void free_gcr3_tbl_level2(u64 *tbl)
>>  	}
>>  }
>>  
>> -static void free_gcr3_table(struct protection_domain *domain)
>> +static void free_gcr3_table(struct iommu_dev_data *dev_data)
>>  {
> 
> Pass in struct gcr3_tbl_info not dev_data

Fixed.

> 
>> -/* Note: This function expects iommu_domain->lock to be held prior calling the function. */
>> -static int setup_gcr3_table(struct protection_domain *domain, int pasids)
>> +static int setup_gcr3_table(struct iommu_dev_data *dev_data, int pasids)
>>  {
>> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
>>  	int levels = get_gcr3_levels(pasids);
> 
> Pass in struct gcr3_tbl_info not dev_data

Fixed.

-Vasant

> 
> I don't think it is worth re-doing at this point - but I probably
> would have tried to structure this series as creating the struct
> gcr3_tbl_info and migrating the protection_domain to use it, then
> streamlined the APIs to have a clean gcr3_tbl_info layer, finally
> adding a gcr3_tbl_info to the iommu_dev_data and using it when
> appropriate.
> 
> Jason

Re: [PATCH v4 11/16] iommu/amd: Refactor attaching / detaching device functions

[Vasant Hegde]

On 1/6/2024 12:44 AM, Jason Gunthorpe wrote:
> On Tue, Dec 12, 2023 at 08:52:19AM +0000, Vasant Hegde wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> If default domain is configured with V2 page table then setup default GCR3
>> with domain GCR3 pointer. So that all devices in the domain uses same page
>> table for translation. Also return page table setup status from do_attach()
>> function.
>>
>> Note that current patch configures GCR3 table with MAX supported PASIDs.
>> Ideally it should use 1 level PASID table as its using PASID zero only.
>> Later in SVA enable path it should be reconfigured to support MAX PASIDs.
>> This will be fixed later once SVA support is upstreamed.
> 
> ??
> 
> Given this driver doesn't support PASID at this point you should just
> allocate a 1 entry table in this patch.
> 
> When PASID support is added then you have to allocate a MAX pasid
> table or implement atomic resize.\

Sure. Its just fixing param. Fixed in next iteration.



> 
> The patch looks fine otherwise
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Thanks
-Vasant

Re: [PATCH v4 07/16] iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue

[Vasant Hegde]

Joson,

[+ Baolu - adding as I have some question around PASID support for UNMANAGED domain]

On 1/6/2024 12:25 AM, Jason Gunthorpe wrote:
> On Tue, Dec 12, 2023 at 08:52:15AM +0000, Vasant Hegde wrote:
>> With v1 page table, the AMD IOMMU spec states that the hardware must use
>> the domain ID to tag its internal translation caches. I/O devices with
>> different v1 page tables must be given different domain IDs. I/O devices
>> that share the same v1 page table __may__ be given the same domain ID.
>> This domain ID management policy is currently implemented by the AMD
>> IOMMU driver. In this case, only the domain ID is needed when issuing the
>> INVALIDATE_IOMMU_PAGES command to invalidate the IOMMU translation cache
>> (TLB).
>>
>> With v2 page table, the hardware uses domain ID and PASID as parameters
>> to tag and issue the INVALIDATE_IOMMU_PAGES command. Since the GCR3 table
>> is setup per-device, and there is no guarantee for PASID to be unique
>> across multiple devices. The same PASID for different devices could
>> have different v2 page tables. In such case, if multiple devices share the
>> same domain ID, IOMMU translation cache for these devices would be polluted
>> due to TLB aliasing.
>>
>> Hence, avoid the TLB aliasing issue with v2 page table by allocating unique
>> domain ID for each device even when multiple devices are sharing the same v1
>> page table. Please note that this workaround would result in multiple
>> INVALIDATE_IOMMU_PAGES commands (one per domain id) when unmapping a
>> translation on the shared v1 page table.
> 
> Huh? This is a typo? "the same v2 page table"  "unmapping a
> translation on the shared v2 page table" ??

Its typo. Will fix it.

> 
> The code seems to be fine, domain_flush_pages_v1() looks optimal?
> 

I'd say its optimal for given state. I have a patch to move dev_iommu[] to
xarray. I am planning to fine tune and post those patches after SVA. With that
changes it will be better.


>> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
>> index fead9033796f..51daf5dd4729 100644
>> --- a/drivers/iommu/amd/amd_iommu_types.h
>> +++ b/drivers/iommu/amd/amd_iommu_types.h
>> @@ -842,6 +842,8 @@ struct iommu_dev_data {
>>  	u8 ppr          :1;		  /* Enable device PPR support */
>>  	bool use_vapic;			  /* Enable device to use vapic mode */
>>  	bool defer_attach;
>> +	/* Per device domain ID. Used with V2 page table */
>> +	u16 domid;
> 
> This should really be put into the 'struct gcr3_tbl_info' - logically
> that is the struct the HW cache tag is linked to. ie if the gcr3 table
> is the same pointer then the cache tag can be re-used by the HW.
> 

The reason we put it in dev_data is because its per device ID, not specific to
GCR3 table.

> Then when you want to optimize for the no-pasid case then the right
> way to do it is putting a 'struct gcr3_tbl_info' inside the v2
> protection_domain.
> 
> The DTE will point at the v2 protection_domain's version of the gcr3
> if the PASID table is empty, otherwise the DTE will point at the
> struct iommu_dev_data version of the gcr3 table.

This makese sense if we are sure we will do per-device-domain-ID only with V2
page table. I still need to see how SVA support with vIOMMU works. For now I
will keep this in my list. Once I have better picture I will fine tune.

> 
> Naturally this will optimize the lifecycle of the domain_id.
> 
> Then put the domain_id alloc/dealloc inside the functions that
> alloc/free the memory under the struct gcr3_tbl_info - ie it is part
> of the gcr3 layer.

Domain ID is part of domain/device not specific to gcr3 layer. In V1 we don't
have GCR3 stuff.

> 
>> +/*
>> + * Allocate per device domain ID when using V2 page table
>> + */
>> +static inline bool domain_id_is_per_dev(struct protection_domain *pdom)
>> +{
>> +	return (pdom && pdom->pd_mode != PD_MODE_V1);
>> +}
> 
> Under that view this function probably would make more sense as:
> 
> domain_requires_gcr3(pdom)
> 
> But frankly I'd just stick with pdom_is_v2_pgtbl_mode().
> 
> Also pdom should never be null when this is called, right?

Right. Will fix it.

> 
>> -/*
>> - * TLB invalidation function which is called from the mapping functions.
>> - * It invalidates a single PTE if the range to flush is within a single
>> - * page. Otherwise it flushes the whole TLB of the IOMMU.
>> - */
>> -static void __domain_flush_pages(struct protection_domain *domain,
>> +static int domain_flush_pages_v2(struct protection_domain *pdom,
>>  				 u64 address, size_t size)
>>  {
>>  	struct iommu_dev_data *dev_data;
>>  	struct iommu_cmd cmd;
>> -	int ret = 0, i;
>> -	ioasid_t pasid = IOMMU_NO_PASID;
>> -	bool gn = false;
>> +	int ret = 0;
>>  
>> -	if (pdom_is_v2_pgtbl_mode(domain))
>> -		gn = true;
>> +	list_for_each_entry(dev_data, &pdom->dev_list, list) {
>> +		struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
>> +
>> +		build_inv_iommu_pages(&cmd, address, size,
>> +				      dev_data->domid, IOMMU_NO_PASID, true);
>> +
>> +		ret |= iommu_queue_command(iommu, &cmd);
>> +	}
> 
> This is fine for where things are here, but what you want to get to
> long term is what I've been talking about of having the attachment
> list on the protection_domain.
> 
> Each attachment entry would hold:
>   struct device *dev;
>   unsigned int pasid;
> 
> Keep the list sorted by (iommu, gcr.domain_id).
> 
> Then optimized invalidation is simply this:
> 
> for_each:
>   if (get_amd_iommu_from_dev(entry->dev) == last_iommu &&
>       get_gcr(entry, pdom)->domain_id == last_domain_id)
>      continue;
>   build_inv_iommu_pages(..)
>   last_iommu = get_amd_iommu_from_dev(entry->dev)
>   domain_id = get_gcr(entry, pdom)->domain_id;


I have done a prototype something in this line. Basically we will eventually
have single API for invalidation :
  domain_flush_pages(pdom, addr, size)

.. and internally it will decide to flush host/guest page table.


[Slightly unrelated topic]

UNAMANGED Domain and PASID support:
  - I was considering this scenario as well. I don't think I understood the use
case of and how invalidation is suppose to work here.

   Can you explain (again?) the use case and how invalidation is suppose to work?

  - For PASID capable device we will have per-device-domain-ID
  - We will have default page table setup (PASID zero in our case) during domain
initialization.
  - We attach PASIDs to same domain. We can add this to list (protection domain
device List info - which will have dev_data/PASID). So set/remove PASIDs is fine.

  - iommu_ops->iotlb_sync_map/flush_iotlb_all will flush PASID zero.
    Looking into intel driver they seems to be invalidating all PASIDs in this
path. I didn't get why it has to flush all PASIDs here.

  - For other PASIDs do we have mmu notifier to invalidate as its attached to
some process?


> 
> And this algorithm matches what Intel and SMMU need and I'm strongly
> thinking about making a driver utility library to handle it, so
> we can revisit this later on.

If things are common across drivers then it makes sense. We can consider common
set of functions.


> 
>> +static int domain_flush_pages_v1(struct protection_domain *pdom,
>> +				 u64 address, size_t size)
>> +{
>> +	struct iommu_cmd cmd;
>> +	int ret = 0, i;
>>  
>> -	build_inv_iommu_pages(&cmd, address, size, domain->id, pasid, gn);
>> +	build_inv_iommu_pages(&cmd, address, size,
>> +			      pdom->id, IOMMU_NO_PASID, false);
>>  
>>  	for (i = 0; i < amd_iommu_get_num_iommus(); ++i) {
>> -		if (!domain->dev_iommu[i])
>> +		if (!pdom->dev_iommu[i])
>>  			continue;
> 
> Right, iterate over every iommu

As explained above I believe this will be better once I move this to xarray.

-Vasant

Re: [PATCH v4 10/16] iommu/amd: Refactor helper function for setting / clearing GCR3

[Vasant Hegde]

On 1/6/2024 12:42 AM, Jason Gunthorpe wrote:
> On Tue, Dec 12, 2023 at 08:52:18AM +0000, Vasant Hegde wrote:
>> +static int __set_gcr3(struct iommu_dev_data *dev_data,
>> +		      ioasid_t pasid, unsigned long gcr3)
>> +{
> 
> IMHO you've got the ATS layering wrong here.. The ATS invalidation
> should be pushed out by attach/detach functions and has to be
> carefully sequenced with the ATS disable bit in the PCI control
> register. I don't think you can do all of this right with things
> organized like this. Indeed this looks like it over invalidates the
> ATS quite a bit.

Currently if device is capable of ATS we just enable it. Then we invalidate
while setting/clearing GCR3. This is extra invalidation if we always clear the
PASID before using it (as clear path would have invalidated IOTLB).


> 
> You should only need to invalidate prior to doing the enable and when
> a GCR3 value is changed while ATS is turned on, which is something
> that the attach op can caculate.
> 
> So, these functions should have a signature of:
> 
> (struct amd_iommu *iommu, struct struct gcr3_tbl_info *gcr3_info, ...)

Not sure I understood this. This path is adding PASID to devices GCR3 table.
Hence I pass device_data. It will set GCR[PASID] and invalidates TLB (Because
its per device things, we can get the iommu details from dev_data itself).

> 
> [and the locking can implicitly rely on the core's group lock, don't
> need more locks]

I was waiting for group->mutex to export so that I can add lockdep_asset with
that and remove device lock here. May be I can just remove it and expand the
description.

> 
>> +static int __clear_gcr3(struct iommu_dev_data *dev_data, ioasid_t pasid)
>> +{
>> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
>> +	u64 *pte;
>> +
>> +	lockdep_assert_held(&dev_data->lock);
>> +
>> +	pte = __get_gcr3_pte(gcr3_info, pasid, false);
>> +	if (pte == NULL)
>> +		return -EINVAL;
>> +
>> +	*pte = 0;
>> +	amd_iommu_dev_flush_pasid_all(dev_data, pasid);
>> +
>> +	return 0;
>> +}
> 
> What is the point of clear? By the time the attach ops will want to do
> clear it is already certain that a non-zero value was installed in
> pasid, and this doesn't free any memory, so what is the point of
> 'alloc=false'?

This will fetch GCR3[PASID] and clears it.

We use same __get_gcr3_pte() in set/clear path. Hence alloc=false is passed.

-Vasant

> 
> Just do set with a 0 value?
> 
> Jason

Re: [PATCH v4 10/16] iommu/amd: Refactor helper function for setting / clearing GCR3

[Jason Gunthorpe]

On Thu, Jan 11, 2024 at 05:22:02PM +0530, Vasant Hegde wrote:
> 
> 
> On 1/6/2024 12:42 AM, Jason Gunthorpe wrote:
> > On Tue, Dec 12, 2023 at 08:52:18AM +0000, Vasant Hegde wrote:
> >> +static int __set_gcr3(struct iommu_dev_data *dev_data,
> >> +		      ioasid_t pasid, unsigned long gcr3)
> >> +{
> > 
> > IMHO you've got the ATS layering wrong here.. The ATS invalidation
> > should be pushed out by attach/detach functions and has to be
> > carefully sequenced with the ATS disable bit in the PCI control
> > register. I don't think you can do all of this right with things
> > organized like this. Indeed this looks like it over invalidates the
> > ATS quite a bit.
> 
> Currently if device is capable of ATS we just enable it. Then we invalidate
> while setting/clearing GCR3. This is extra invalidation if we always clear the
> PASID before using it (as clear path would have invalidated IOTLB).

It is simpler if ATS can just be left on, but I'm not sure you can
actually do that when you get to nesting.

The guest and the hypervisor need to agree on the ATS state, if the
guest thinks ATS is off then the guest will not generate ATC
invalidations, which means the physical ATS has to be off too.

Thus all this needs to be carefully sequenced to be dynamic, and this
doesn't look layered well for that.

> > You should only need to invalidate prior to doing the enable and when
> > a GCR3 value is changed while ATS is turned on, which is something
> > that the attach op can caculate.
> > 
> > So, these functions should have a signature of:
> > 
> > (struct amd_iommu *iommu, struct struct gcr3_tbl_info *gcr3_info, ...)
> 
> Not sure I understood this. This path is adding PASID to devices GCR3 table.
> Hence I pass device_data. It will set GCR[PASID] and invalidates TLB (Because
> its per device things, we can get the iommu details from dev_data itself).

I think it is wrong layering to make the GCR3 table linked to the
device, it should be an object indepdent of the device. Any place you
are passing a device into a gcr3 layer function looks suspect to me.

This is why I said you should put the domain_id in the gcr3 table,
because that is the proper layers for the objects and data.

> > [and the locking can implicitly rely on the core's group lock, don't
> > need more locks]
> 
> I was waiting for group->mutex to export so that I can add lockdep_asset with
> that and remove device lock here. May be I can just remove it and expand the
> description.

If you want to use it just add a 'iommu_group_mutex_assert(dev)'. It
is a couple of lines

> >> +static int __clear_gcr3(struct iommu_dev_data *dev_data, ioasid_t pasid)
> >> +{
> >> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
> >> +	u64 *pte;
> >> +
> >> +	lockdep_assert_held(&dev_data->lock);
> >> +
> >> +	pte = __get_gcr3_pte(gcr3_info, pasid, false);
> >> +	if (pte == NULL)
> >> +		return -EINVAL;
> >> +
> >> +	*pte = 0;
> >> +	amd_iommu_dev_flush_pasid_all(dev_data, pasid);
> >> +
> >> +	return 0;
> >> +}
> > 
> > What is the point of clear? By the time the attach ops will want to do
> > clear it is already certain that a non-zero value was installed in
> > pasid, and this doesn't free any memory, so what is the point of
> > 'alloc=false'?
> 
> This will fetch GCR3[PASID] and clears it.
> 
> We use same __get_gcr3_pte() in set/clear path. Hence alloc=false is passed.

Just call set with 0 - again there is no point in having alloc=false
since this isn't called in any case where we don't already know the
that the entry was already allocated.

Also, most likely this eventally needs splitting up like SMMU did so
that allocating memory to get an entry pointer is separate from
setting that entry pointer as a driver should strive for "fail means
no change" implemenation of their domain attach function.

Jason

Re: [PATCH v4 07/16] iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue

[Jason Gunthorpe]

On Thu, Jan 11, 2024 at 04:48:01PM +0530, Vasant Hegde wrote:

> > The code seems to be fine, domain_flush_pages_v1() looks optimal?
> 
> I'd say its optimal for given state. I have a patch to move dev_iommu[] to
> xarray. I am planning to fine tune and post those patches after SVA. With that
> changes it will be better.

I'm not keen on an xarray, that isn't fully addressing the problem and
when Intel did it they messed up the locking..

I would like to make a shared helper to solve this problem. A rcu
backed linked list, with a helper API to iterate once per-iommu.
The RCU is a bit tricky.

> >> --- a/drivers/iommu/amd/amd_iommu_types.h
> >> +++ b/drivers/iommu/amd/amd_iommu_types.h
> >> @@ -842,6 +842,8 @@ struct iommu_dev_data {
> >>  	u8 ppr          :1;		  /* Enable device PPR support */
> >>  	bool use_vapic;			  /* Enable device to use vapic mode */
> >>  	bool defer_attach;
> >> +	/* Per device domain ID. Used with V2 page table */
> >> +	u16 domid;
> > 
> > This should really be put into the 'struct gcr3_tbl_info' - logically
> > that is the struct the HW cache tag is linked to. ie if the gcr3 table
> > is the same pointer then the cache tag can be re-used by the HW.
> > 
> 
> The reason we put it in dev_data is because its per device ID, not specific to
> GCR3 table.

But from a HW perspective it is actually linked to the GCR3 table as
that is the data pointer that is being cached. Two devices that share
a GCR3 pointer can share a domain ID.

If you want to optimize the domain ID tagging the logical way to do it
is to add code to share the GCR3 tables across devices. If the GCR3
table is the same then the domain ID can be the same.

> > Then when you want to optimize for the no-pasid case then the right
> > way to do it is putting a 'struct gcr3_tbl_info' inside the v2
> > protection_domain.
> > 
> > The DTE will point at the v2 protection_domain's version of the gcr3
> > if the PASID table is empty, otherwise the DTE will point at the
> > struct iommu_dev_data version of the gcr3 table.
> 
> This makese sense if we are sure we will do per-device-domain-ID only with V2
> page table. I still need to see how SVA support with vIOMMU works. For now I
> will keep this in my list. Once I have better picture I will fine tune.

SVA support is ugly on AMD - you need to flow through the virtual
domain ID to a consistent physical domain ID even though the guest may
have a DTEs tagged with the same domain ID but different GCR3
pointers.

Most likely the implementation will have the physical domain ID be
part of the nesting domain to achieve this.

> UNAMANGED Domain and PASID support:
>   - I was considering this scenario as well. I don't think I understood the use
> case of and how invalidation is suppose to work here.
> 
>    Can you explain (again?) the use case and how invalidation is suppose to work?

Invalidation is no different than any other V2 domain use case. The
domain ops trigger invalidation. In AMD HW you need to record that an
unmanaged domain is connected to a domain ID & PASID and push the
right invalidate. The driver can't just assume the PASID is 0 for V2
unmanaged domains.

So the implementation is to track the attacked devices, iommus and
PASIDs in a linked list and use that linked list to generate
invalidations. Intel and SMMU (after my patches) both have
implementations of this. I would like to unify them to a helper
because they are both kind of bad.

There are many use cases for PASID mappings without SVA, including
SIOV-like devices and virtualization modes with non-SVA PRI.

>   - For PASID capable device we will have per-device-domain-ID
>   - We will have default page table setup (PASID zero in our case) during domain
> initialization.

Domain initialization???

>   - We attach PASIDs to same domain. We can add this to list (protection domain
> device List info - which will have dev_data/PASID). So set/remove PASIDs is fine.

Right

>   - iommu_ops->iotlb_sync_map/flush_iotlb_all will flush PASID zero.
>     Looking into intel driver they seems to be invalidating all PASIDs in this
> path. I didn't get why it has to flush all PASIDs here.

You iterate ove the list above and flush every PASID in the list. I
don't know what Intel is doing, fush all PASID on domain invalidation
sounds like overkill.
 
>   - For other PASIDs do we have mmu notifier to invalidate as its attached to
> some process?

mmu notifier has nothing to do with PASID.

The driver should have a general scheme to keep track of a
iommu_domain's cache tags. This is the per-domain linked list of
pasid/device/iommu.

When the page table changes (however that page table is stored) it
walks that linked list and pushes invalidation commands.

For an unmanaged domain this common code is called by the
iommu_domain ops unmap/flush/etc.

For a SVA domain this common code is called by the MMU notifier
arch_invalidate_range

> > And this algorithm matches what Intel and SMMU need and I'm strongly
> > thinking about making a driver utility library to handle it, so
> > we can revisit this later on.
> 
> If things are common across drivers then it makes sense. We can consider common
> set of functions.

I have been meaning to try to make something, lets see next week

Jason

Re: [PATCH v4 10/16] iommu/amd: Refactor helper function for setting / clearing GCR3

[Vasant Hegde]

Jason,


On 1/11/2024 6:56 PM, Jason Gunthorpe wrote:
> On Thu, Jan 11, 2024 at 05:22:02PM +0530, Vasant Hegde wrote:
>>
>>
>> On 1/6/2024 12:42 AM, Jason Gunthorpe wrote:
>>> On Tue, Dec 12, 2023 at 08:52:18AM +0000, Vasant Hegde wrote:
>>>> +static int __set_gcr3(struct iommu_dev_data *dev_data,
>>>> +		      ioasid_t pasid, unsigned long gcr3)
>>>> +{
>>>
>>> IMHO you've got the ATS layering wrong here.. The ATS invalidation
>>> should be pushed out by attach/detach functions and has to be
>>> carefully sequenced with the ATS disable bit in the PCI control
>>> register. I don't think you can do all of this right with things
>>> organized like this. Indeed this looks like it over invalidates the
>>> ATS quite a bit.
>>
>> Currently if device is capable of ATS we just enable it. Then we invalidate
>> while setting/clearing GCR3. This is extra invalidation if we always clear the
>> PASID before using it (as clear path would have invalidated IOTLB).
> 
> It is simpler if ATS can just be left on, but I'm not sure you can
> actually do that when you get to nesting.
> 
> The guest and the hypervisor need to agree on the ATS state, if the
> guest thinks ATS is off then the guest will not generate ATC
> invalidations, which means the physical ATS has to be off too.
> 
> Thus all this needs to be carefully sequenced to be dynamic, and this
> doesn't look layered well for that.

Yeah. That's another complicated stuff. We will look into it later.


> 
>>> You should only need to invalidate prior to doing the enable and when
>>> a GCR3 value is changed while ATS is turned on, which is something
>>> that the attach op can caculate.
>>>
>>> So, these functions should have a signature of:
>>>
>>> (struct amd_iommu *iommu, struct struct gcr3_tbl_info *gcr3_info, ...)
>>
>> Not sure I understood this. This path is adding PASID to devices GCR3 table.
>> Hence I pass device_data. It will set GCR[PASID] and invalidates TLB (Because
>> its per device things, we can get the iommu details from dev_data itself).
> 
> I think it is wrong layering to make the GCR3 table linked to the
> device, it should be an object indepdent of the device. Any place you
> are passing a device into a gcr3 layer function looks suspect to me.
> 
> This is why I said you should put the domain_id in the gcr3 table,
> because that is the proper layers for the objects and data.
> 
>>> [and the locking can implicitly rely on the core's group lock, don't
>>> need more locks]
>>
>> I was waiting for group->mutex to export so that I can add lockdep_asset with
>> that and remove device lock here. May be I can just remove it and expand the
>> description.
> 
> If you want to use it just add a 'iommu_group_mutex_assert(dev)'. It
> is a couple of lines

Done. I have added a patch to export group mutex from core layer and dropped
dev_data lock.

> 
>>>> +static int __clear_gcr3(struct iommu_dev_data *dev_data, ioasid_t pasid)
>>>> +{
>>>> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
>>>> +	u64 *pte;
>>>> +
>>>> +	lockdep_assert_held(&dev_data->lock);
>>>> +
>>>> +	pte = __get_gcr3_pte(gcr3_info, pasid, false);
>>>> +	if (pte == NULL)
>>>> +		return -EINVAL;
>>>> +
>>>> +	*pte = 0;
>>>> +	amd_iommu_dev_flush_pasid_all(dev_data, pasid);
>>>> +
>>>> +	return 0;
>>>> +}
>>>
>>> What is the point of clear? By the time the attach ops will want to do
>>> clear it is already certain that a non-zero value was installed in
>>> pasid, and this doesn't free any memory, so what is the point of
>>> 'alloc=false'?
>>
>> This will fetch GCR3[PASID] and clears it.
>>
>> We use same __get_gcr3_pte() in set/clear path. Hence alloc=false is passed.
> 
> Just call set with 0 - again there is no point in having alloc=false
> since this isn't called in any case where we don't already know the
> that the entry was already allocated.

I will replace set/clear function with update_gcr3 for now.

> 
> Also, most likely this eventally needs splitting up like SMMU did so
> that allocating memory to get an entry pointer is separate from
> setting that entry pointer as a driver should strive for "fail means
> no change" implemenation of their domain attach function.
> 

That needs some more rework in attach device path as well. We will look into it
later.

-Vasant

Re: [PATCH v4 07/16] iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue

[Vasant Hegde]

On 1/11/2024 7:29 PM, Jason Gunthorpe wrote:
> On Thu, Jan 11, 2024 at 04:48:01PM +0530, Vasant Hegde wrote:
> 
>>> The code seems to be fine, domain_flush_pages_v1() looks optimal?
>>
>> I'd say its optimal for given state. I have a patch to move dev_iommu[] to
>> xarray. I am planning to fine tune and post those patches after SVA. With that
>> changes it will be better.
> 
> I'm not keen on an xarray, that isn't fully addressing the problem and
> when Intel did it they messed up the locking..

In our case it will be simple as we just need to track attached IOMMUs and we
can build list/xarray under protection domain lock. Either xarray or even list
will do.

> 
> I would like to make a shared helper to solve this problem. A rcu
> backed linked list, with a helper API to iterate once per-iommu.
> The RCU is a bit tricky.
> 
>>>> --- a/drivers/iommu/amd/amd_iommu_types.h
>>>> +++ b/drivers/iommu/amd/amd_iommu_types.h
>>>> @@ -842,6 +842,8 @@ struct iommu_dev_data {
>>>>  	u8 ppr          :1;		  /* Enable device PPR support */
>>>>  	bool use_vapic;			  /* Enable device to use vapic mode */
>>>>  	bool defer_attach;
>>>> +	/* Per device domain ID. Used with V2 page table */
>>>> +	u16 domid;
>>>
>>> This should really be put into the 'struct gcr3_tbl_info' - logically
>>> that is the struct the HW cache tag is linked to. ie if the gcr3 table
>>> is the same pointer then the cache tag can be re-used by the HW.
>>>
>>
>> The reason we put it in dev_data is because its per device ID, not specific to
>> GCR3 table.
> 
> But from a HW perspective it is actually linked to the GCR3 table as
> that is the data pointer that is being cached. Two devices that share
> a GCR3 pointer can share a domain ID.
> 
> If you want to optimize the domain ID tagging the logical way to do it
> is to add code to share the GCR3 tables across devices. If the GCR3
> table is the same then the domain ID can be the same.


Right. It makes sense when using V2 page table. But when we are in V1 page table
we don't have GCR3. We will revisit once we finalize some of the vIOMMU stuff.


> 
>>> Then when you want to optimize for the no-pasid case then the right
>>> way to do it is putting a 'struct gcr3_tbl_info' inside the v2
>>> protection_domain.
>>>
>>> The DTE will point at the v2 protection_domain's version of the gcr3
>>> if the PASID table is empty, otherwise the DTE will point at the
>>> struct iommu_dev_data version of the gcr3 table.
>>
>> This makese sense if we are sure we will do per-device-domain-ID only with V2
>> page table. I still need to see how SVA support with vIOMMU works. For now I
>> will keep this in my list. Once I have better picture I will fine tune.
> 
> SVA support is ugly on AMD - you need to flow through the virtual
> domain ID to a consistent physical domain ID even though the guest may
> have a DTEs tagged with the same domain ID but different GCR3
> pointers.
> 
> Most likely the implementation will have the physical domain ID be
> part of the nesting domain to achieve this.

Yeah. This is something to figureout after getting SVA upstream.

> 
>> UNAMANGED Domain and PASID support:
>>   - I was considering this scenario as well. I don't think I understood the use
>> case of and how invalidation is suppose to work here.
>>
>>    Can you explain (again?) the use case and how invalidation is suppose to work?
> 
> Invalidation is no different than any other V2 domain use case. The
> domain ops trigger invalidation. In AMD HW you need to record that an
> unmanaged domain is connected to a domain ID & PASID and push the
> right invalidate. The driver can't just assume the PASID is 0 for V2
> unmanaged domains.
> 
> So the implementation is to track the attacked devices, iommus and
> PASIDs in a linked list and use that linked list to generate
> invalidations. Intel and SMMU (after my patches) both have
> implementations of this. I would like to unify them to a helper
> because they are both kind of bad.
> 
> There are many use cases for PASID mappings without SVA, including
> SIOV-like devices and virtualization modes with non-SVA PRI.

What kind of page table will be attached with non zero PASID? like KVM page table?

> 
>>   - For PASID capable device we will have per-device-domain-ID
>>   - We will have default page table setup (PASID zero in our case) during domain
>> initialization.
> 
> Domain initialization???

Sorry. Its typo. I meant during device attach path.

> 
>>   - We attach PASIDs to same domain. We can add this to list (protection domain
>> device List info - which will have dev_data/PASID). So set/remove PASIDs is fine.
> 
> Right
> 
>>   - iommu_ops->iotlb_sync_map/flush_iotlb_all will flush PASID zero.
>>     Looking into intel driver they seems to be invalidating all PASIDs in this
>> path. I didn't get why it has to flush all PASIDs here.
> 
> You iterate ove the list above and flush every PASID in the list. I
> don't know what Intel is doing, fush all PASID on domain invalidation
> sounds like overkill.

IIUC with UNMANAGED domain with PASID, we will have device/pasid list with
different PASIDs pointing to different page tables.
  ex: PASID 0 with DMA-API mode , PASID1 pointing to some other page table.

This is where the confusion is. Current iommu ops doesn't take pasid as
parameter. So if we go over entire dev/pasid list and flush it becomes overkill
right? -OR- are we planning to add more params to these ops?
Am I missing something here?


>  
>>   - For other PASIDs do we have mmu notifier to invalidate as its attached to
>> some process?
> 
> mmu notifier has nothing to do with PASID.
> 
> The driver should have a general scheme to keep track of a
> iommu_domain's cache tags. This is the per-domain linked list of
> pasid/device/iommu.
> 
> When the page table changes (however that page table is stored) it
> walks that linked list and pushes invalidation commands.
> 
> For an unmanaged domain this common code is called by the
> iommu_domain ops unmap/flush/etc.
> 
> For a SVA domain this common code is called by the MMU notifier
> arch_invalidate_range

Yeah. I got the overall flow.


-Vasant

Re: [PATCH v4 07/16] iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue

[Jason Gunthorpe]

On Fri, Jan 12, 2024 at 06:15:07PM +0530, Vasant Hegde wrote:

> Right. It makes sense when using V2 page table. But when we are in V1 page table
> we don't have GCR3. We will revisit once we finalize some of the vIOMMU stuff.

But in v1 mode the domain_id comes from the iommu_domain struct, there
is no case where it logically is part of the device.

> > Invalidation is no different than any other V2 domain use case. The
> > domain ops trigger invalidation. In AMD HW you need to record that an
> > unmanaged domain is connected to a domain ID & PASID and push the
> > right invalidate. The driver can't just assume the PASID is 0 for V2
> > unmanaged domains.
> > 
> > So the implementation is to track the attacked devices, iommus and
> > PASIDs in a linked list and use that linked list to generate
> > invalidations. Intel and SMMU (after my patches) both have
> > implementations of this. I would like to unify them to a helper
> > because they are both kind of bad.
> > 
> > There are many use cases for PASID mappings without SVA, including
> > SIOV-like devices and virtualization modes with non-SVA PRI.
> 
> What kind of page table will be attached with non zero PASID? like KVM page table?

Probably at copy of the KVM page table in some for, certainly today
starting with an UNAMANGED domain as is normal. I think people will
want to do different things here..

> >>   - iommu_ops->iotlb_sync_map/flush_iotlb_all will flush PASID zero.
> >>     Looking into intel driver they seems to be invalidating all PASIDs in this
> >> path. I didn't get why it has to flush all PASIDs here.
> > 
> > You iterate ove the list above and flush every PASID in the list. I
> > don't know what Intel is doing, fush all PASID on domain invalidation
> > sounds like overkill.
> 
> IIUC with UNMANAGED domain with PASID, we will have device/pasid list with
> different PASIDs pointing to different page tables.
>   ex: PASID 0 with DMA-API mode , PASID1 pointing to some other page table.

The *device* has a list of PASID's that point to iommu_domains. This
is stored in an xarray inside the iommu_group.

The *iommu_domain* has a list of *devices & PASIDs* that can use this
domain for translation (ie that it was attached to)

The RID attach is just PASID 0.

> This is where the confusion is. Current iommu ops doesn't take pasid as
> parameter. So if we go over entire dev/pasid list and flush it becomes overkill
> right?

If I have a v2 paging iommu domain (unmanaged) and I change the IOPTE
to effect a certain IOVA range then I have to invalidate it at every
place that is caching it.

For ATC that means I need to issue an invalidation to every
(iommu, RID, PASID) combination that has it in cache.

For V2 AMD IOTLB I need to issue an invalidation for every
(iommu, device->gcr3->domain_id, PASID) that has it in the cache.

For V1 AMD IOTLB I need to invalidate every
(iommu, domain->domain_id) combination.

It is a data structure problem to store a minimal list of those
things off of the iommu_domain.

No new op parameters are needed.

ATC is a list of every attached struct device and PASID, populated by
ops->attach_dev (PASID=0) or ops->set_dev_pasid.

V2 IOTLB is that same list with duplicate device->gcr3->domain_id's
removed

IOMMU is that same list with duplicate device->amd_iommu's removed,
which is the list V1 IOTLB needs.

So, store one linked list for ATC in the iommu_domain.

Sort it by (device->amd_iommu, device->gcr3->domain_id, device->id)

Skip consecutive runs of same domain_id or same iommu when generating
the invalidation sequences.

Use RCU to lock the list so the invalidation fast path is
lockless. (this is tricky)

Jason

Re: [PATCH v4 07/16] iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue

[Vasant Hegde]

Jason,


On 1/12/2024 8:29 PM, Jason Gunthorpe wrote:
> On Fri, Jan 12, 2024 at 06:15:07PM +0530, Vasant Hegde wrote:
> 
>> Right. It makes sense when using V2 page table. But when we are in V1 page table
>> we don't have GCR3. We will revisit once we finalize some of the vIOMMU stuff.
> 
> But in v1 mode the domain_id comes from the iommu_domain struct, there
> is no case where it logically is part of the device.

currently yes, for v1 it comes from protection domain data structure. In future
we may have to do per device domain ID. Anyway for now I can put it in gcr3_info
table and if required I will move it back to dev_data structure later.

> 
>>> Invalidation is no different than any other V2 domain use case. The
>>> domain ops trigger invalidation. In AMD HW you need to record that an
>>> unmanaged domain is connected to a domain ID & PASID and push the
>>> right invalidate. The driver can't just assume the PASID is 0 for V2
>>> unmanaged domains.
>>>
>>> So the implementation is to track the attacked devices, iommus and
>>> PASIDs in a linked list and use that linked list to generate
>>> invalidations. Intel and SMMU (after my patches) both have
>>> implementations of this. I would like to unify them to a helper
>>> because they are both kind of bad.
>>>
>>> There are many use cases for PASID mappings without SVA, including
>>> SIOV-like devices and virtualization modes with non-SVA PRI.
>>
>> What kind of page table will be attached with non zero PASID? like KVM page table?
> 
> Probably at copy of the KVM page table in some for, certainly today
> starting with an UNAMANGED domain as is normal. I think people will
> want to do different things here..
> 
>>>>   - iommu_ops->iotlb_sync_map/flush_iotlb_all will flush PASID zero.
>>>>     Looking into intel driver they seems to be invalidating all PASIDs in this
>>>> path. I didn't get why it has to flush all PASIDs here.
>>>
>>> You iterate ove the list above and flush every PASID in the list. I
>>> don't know what Intel is doing, fush all PASID on domain invalidation
>>> sounds like overkill.
>>
>> IIUC with UNMANAGED domain with PASID, we will have device/pasid list with
>> different PASIDs pointing to different page tables.
>>   ex: PASID 0 with DMA-API mode , PASID1 pointing to some other page table.
> 
> The *device* has a list of PASID's that point to iommu_domains. This
> is stored in an xarray inside the iommu_group.
> 
> The *iommu_domain* has a list of *devices & PASIDs* that can use this
> domain for translation (ie that it was attached to)

IIUC domain will be having device/PASID combination something like below:

UNMANAGED_DOMAIN_A with IO Page table :
  - DevA + PASID zero (say non PASID capable device)
  - devB + PASID <M>
  - devC + PASID <N>

We will *not* have same device with different PASID in same domain like
  - DevB + PASID M
  - DevB + PASID N

Is that correct?


> 
> The RID attach is just PASID 0.
> 
>> This is where the confusion is. Current iommu ops doesn't take pasid as
>> parameter. So if we go over entire dev/pasid list and flush it becomes overkill
>> right?
> 
> If I have a v2 paging iommu domain (unmanaged) and I change the IOPTE
> to effect a certain IOVA range then I have to invalidate it at every
> place that is caching it.

Below flow is fine. We mostly have it and we should be able to refine it.

-Vasant


> 
> For ATC that means I need to issue an invalidation to every
> (iommu, RID, PASID) combination that has it in cache.
> 
> For V2 AMD IOTLB I need to issue an invalidation for every
> (iommu, device->gcr3->domain_id, PASID) that has it in the cache.
> 
> For V1 AMD IOTLB I need to invalidate every
> (iommu, domain->domain_id) combination.
> 
> It is a data structure problem to store a minimal list of those
> things off of the iommu_domain.
> 
> No new op parameters are needed.
> 
> ATC is a list of every attached struct device and PASID, populated by
> ops->attach_dev (PASID=0) or ops->set_dev_pasid.
> 
> V2 IOTLB is that same list with duplicate device->gcr3->domain_id's
> removed
> 
> IOMMU is that same list with duplicate device->amd_iommu's removed,
> which is the list V1 IOTLB needs.
> 
> So, store one linked list for ATC in the iommu_domain.
> 
> Sort it by (device->amd_iommu, device->gcr3->domain_id, device->id)
> 
> Skip consecutive runs of same domain_id or same iommu when generating
> the invalidation sequences.
> 
> Use RCU to lock the list so the invalidation fast path is
> lockless. (this is tricky)
> 
> Jason

Re: [PATCH v4 07/16] iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue

[Jason Gunthorpe]

On Tue, Jan 16, 2024 at 04:22:16PM +0530, Vasant Hegde wrote:

> > The *device* has a list of PASID's that point to iommu_domains. This
> > is stored in an xarray inside the iommu_group.
> > 
> > The *iommu_domain* has a list of *devices & PASIDs* that can use this
> > domain for translation (ie that it was attached to)
> 
> IIUC domain will be having device/PASID combination something like below:
> 
> UNMANAGED_DOMAIN_A with IO Page table :
>   - DevA + PASID zero (say non PASID capable device)
>   - devB + PASID <M>
>   - devC + PASID <N>
> 
> We will *not* have same device with different PASID in same domain like
>   - DevB + PASID M
>   - DevB + PASID N
> 
> Is that correct?

No, all combinations are allowed in the API.

Jason

Re: [PATCH v4 07/16] iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue

[Vasant Hegde]

Joson,


On 1/16/2024 7:30 PM, Jason Gunthorpe wrote:
> On Tue, Jan 16, 2024 at 04:22:16PM +0530, Vasant Hegde wrote:
> 
>>> The *device* has a list of PASID's that point to iommu_domains. This
>>> is stored in an xarray inside the iommu_group.
>>>
>>> The *iommu_domain* has a list of *devices & PASIDs* that can use this
>>> domain for translation (ie that it was attached to)
>>
>> IIUC domain will be having device/PASID combination something like below:
>>
>> UNMANAGED_DOMAIN_A with IO Page table :
>>   - DevA + PASID zero (say non PASID capable device)
>>   - devB + PASID <M>
>>   - devC + PASID <N>
>>
>> We will *not* have same device with different PASID in same domain like
>>   - DevB + PASID M
>>   - DevB + PASID N
>>
>> Is that correct?
> 
> No, all combinations are allowed in the API.

What is the use case for same device with two or more different PASID to point
to same page table?

-Vasant

Re: [PATCH v4 07/16] iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue

[Jason Gunthorpe]

On Tue, Jan 16, 2024 at 10:38:16PM +0530, Vasant Hegde wrote:
> Joson,
> 
> 
> On 1/16/2024 7:30 PM, Jason Gunthorpe wrote:
> > On Tue, Jan 16, 2024 at 04:22:16PM +0530, Vasant Hegde wrote:
> > 
> >>> The *device* has a list of PASID's that point to iommu_domains. This
> >>> is stored in an xarray inside the iommu_group.
> >>>
> >>> The *iommu_domain* has a list of *devices & PASIDs* that can use this
> >>> domain for translation (ie that it was attached to)
> >>
> >> IIUC domain will be having device/PASID combination something like below:
> >>
> >> UNMANAGED_DOMAIN_A with IO Page table :
> >>   - DevA + PASID zero (say non PASID capable device)
> >>   - devB + PASID <M>
> >>   - devC + PASID <N>
> >>
> >> We will *not* have same device with different PASID in same domain like
> >>   - DevB + PASID M
> >>   - DevB + PASID N
> >>
> >> Is that correct?
> > 
> > No, all combinations are allowed in the API.
> 
> What is the use case for same device with two or more different PASID to point
> to same page table?

Possibly something with SIOV like techniques where you have two
virtual functions, each being DMA isolated with PASID, installed into
the same VM. The guest physical map would be the same iommu_domain but
the PASID would be different.

Regardless, the API doesn't require this and I don't want to see
drivers inventing their own restrictions without a reason..

Jason