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

[PATCH v2 00/10] 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 - 2:
  Rename helper function

* Patch 3 - 9:
  Introduce per device GCR3 table and code refactoring

* Patch 10:
  Remove unused variable from protection domain structure

This patch series is based on top of SVA Part 2 [1] :
  https://lore.kernel.org/linux-iommu/673b7081-801b-d24b-ea75-33c5847951b4@amd.com/T/#t

This is also available at github :
  https://github.com/AMDESE/linux/tree/iommu_sva_part3_v2_v6.5_rc1

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

Thank you,
Vasant / Suravee

Suravee Suthikulpanit (9):
  iommu/amd: Use struct protection_domain in helper functions
  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 helper function for attaching / detaching device
  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 (1):
  iommu/amd: Use protection_domain.flags to check page table mode

 drivers/iommu/amd/amd_iommu.h       |  24 ++-
 drivers/iommu/amd/amd_iommu_types.h |  26 +--
 drivers/iommu/amd/io_pgtable_v2.c   |  24 +--
 drivers/iommu/amd/iommu.c           | 257 +++++++++++++++++-----------
 include/linux/iommu.h               |  14 ++
 5 files changed, 213 insertions(+), 132 deletions(-)

-- 
2.31.1

[PATCH v2 01/10] iommu/amd: Use struct protection_domain in helper functions

[Vasant Hegde]

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

To simplify the unnecessary use of container_of() on the struct
iommu_domain to get the container structure.

No functional changes intended.

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     | 13 +++++++++----
 drivers/iommu/amd/io_pgtable_v2.c |  8 ++++----
 drivers/iommu/amd/iommu.c         | 18 ++++--------------
 3 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 8b586effbefc..87739d95ea8b 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -55,15 +55,15 @@ 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);
 
-int amd_iommu_flush_page(struct iommu_domain *dom, u32 pasid, u64 address);
+int amd_iommu_flush_page(struct protection_domain *domain, u32 pasid, u64 address);
 void amd_iommu_update_and_flush_device_table(struct protection_domain *domain);
 void amd_iommu_domain_update(struct protection_domain *domain);
 void amd_iommu_domain_flush_complete(struct protection_domain *domain);
 void amd_iommu_domain_flush_tlb_pde(struct protection_domain *domain);
-int amd_iommu_flush_tlb(struct iommu_domain *dom, u32 pasid);
-int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, u32 pasid,
+int amd_iommu_flush_tlb(struct protection_domain *domain, u32 pasid);
+int amd_iommu_domain_set_gcr3(struct protection_domain *domain, u32 pasid,
 			      unsigned long cr3);
-int amd_iommu_domain_clear_gcr3(struct iommu_domain *dom, u32 pasid);
+int amd_iommu_domain_clear_gcr3(struct protection_domain *domain, u32 pasid);
 
 #ifdef CONFIG_IRQ_REMAP
 int amd_iommu_create_irq_domain(struct amd_iommu *iommu);
@@ -147,6 +147,11 @@ static inline void *alloc_pgtable_page(int nid, gfp_t gfp)
 	return page ? page_address(page) : NULL;
 }
 
+static inline struct protection_domain *to_pdomain(struct iommu_domain *dom)
+{
+	return container_of(dom, struct protection_domain, domain);
+}
+
 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/io_pgtable_v2.c b/drivers/iommu/amd/io_pgtable_v2.c
index f818a7e254d4..c17cda83bca5 100644
--- a/drivers/iommu/amd/io_pgtable_v2.c
+++ b/drivers/iommu/amd/io_pgtable_v2.c
@@ -274,9 +274,9 @@ static int iommu_v2_map_pages(struct io_pgtable_ops *ops, unsigned long iova,
 out:
 	if (updated) {
 		if (count > 1)
-			amd_iommu_flush_tlb(&pdom->domain, 0);
+			amd_iommu_flush_tlb(pdom, 0);
 		else
-			amd_iommu_flush_page(&pdom->domain, 0, o_iova);
+			amd_iommu_flush_page(pdom, 0, o_iova);
 	}
 
 	if (mapped)
@@ -364,7 +364,7 @@ static void v2_free_pgtable(struct io_pgtable *iop)
 		return;
 
 	/* Clear gcr3 entry */
-	amd_iommu_domain_clear_gcr3(&pdom->domain, 0);
+	amd_iommu_domain_clear_gcr3(pdom, 0);
 
 	/* Make changes visible to IOMMUs */
 	amd_iommu_domain_update(pdom);
@@ -384,7 +384,7 @@ static struct io_pgtable *v2_alloc_pgtable(struct io_pgtable_cfg *cfg, void *coo
 	if (!pgtable->pgd)
 		return NULL;
 
-	ret = amd_iommu_domain_set_gcr3(&pdom->domain, 0, iommu_virt_to_phys(pgtable->pgd));
+	ret = amd_iommu_domain_set_gcr3(pdom, 0, iommu_virt_to_phys(pgtable->pgd));
 	if (ret)
 		goto err_free_pgd;
 
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 8e4f52d8bc9d..fca6e6ceeef3 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -173,11 +173,6 @@ static struct amd_iommu *rlookup_amd_iommu(struct device *dev)
 	return __rlookup_amd_iommu(seg, PCI_SBDF_TO_DEVID(devid));
 }
 
-static struct protection_domain *to_pdomain(struct iommu_domain *dom)
-{
-	return container_of(dom, struct protection_domain, domain);
-}
-
 static struct iommu_dev_data *alloc_dev_data(struct amd_iommu *iommu, u16 devid)
 {
 	struct iommu_dev_data *dev_data;
@@ -2592,10 +2587,8 @@ static int __amd_iommu_flush_page(struct protection_domain *domain, u32 pasid,
 	return __flush_pasid(domain, pasid, address, false);
 }
 
-int amd_iommu_flush_page(struct iommu_domain *dom, u32 pasid,
-			 u64 address)
+int amd_iommu_flush_page(struct protection_domain *domain, u32 pasid, u64 address)
 {
-	struct protection_domain *domain = to_pdomain(dom);
 	unsigned long flags;
 	int ret;
 
@@ -2612,9 +2605,8 @@ static int __amd_iommu_flush_tlb(struct protection_domain *domain, u32 pasid)
 			     true);
 }
 
-int amd_iommu_flush_tlb(struct iommu_domain *dom, u32 pasid)
+int amd_iommu_flush_tlb(struct protection_domain *domain, u32 pasid)
 {
-	struct protection_domain *domain = to_pdomain(dom);
 	unsigned long flags;
 	int ret;
 
@@ -2690,10 +2682,9 @@ static int __clear_gcr3(struct protection_domain *domain, u32 pasid)
 	return __amd_iommu_flush_tlb(domain, pasid);
 }
 
-int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, u32 pasid,
+int amd_iommu_domain_set_gcr3(struct protection_domain *domain, u32 pasid,
 			      unsigned long cr3)
 {
-	struct protection_domain *domain = to_pdomain(dom);
 	unsigned long flags;
 	int ret;
 
@@ -2704,9 +2695,8 @@ int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, u32 pasid,
 	return ret;
 }
 
-int amd_iommu_domain_clear_gcr3(struct iommu_domain *dom, u32 pasid)
+int amd_iommu_domain_clear_gcr3(struct protection_domain *domain, u32 pasid)
 {
-	struct protection_domain *domain = to_pdomain(dom);
 	unsigned long flags;
 	int ret;
 
-- 
2.31.1

[PATCH v2 02/10] 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 |  7 +++++++
 drivers/iommu/amd/iommu.c     | 24 ++++++++++++------------
 include/linux/iommu.h         | 14 ++++++++++++++
 3 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 87739d95ea8b..639d82295797 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -152,6 +152,13 @@ static inline struct protection_domain *to_pdomain(struct iommu_domain *dom)
 	return container_of(dom, struct protection_domain, domain);
 }
 
+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 fca6e6ceeef3..002b29048813 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -217,7 +217,7 @@ static int clone_alias(struct pci_dev *pdev, u16 alias, void *data)
 	if (devid == alias)
 		return 0;
 
-	iommu = rlookup_amd_iommu(&pdev->dev);
+	iommu = get_amd_iommu_from_dev(&pdev->dev);
 	if (!iommu)
 		return 0;
 
@@ -1407,7 +1407,7 @@ static int device_flush_iotlb(struct iommu_dev_data *dev_data,
 	int qdep;
 
 	qdep     = dev_data->ats_qdep;
-	iommu    = rlookup_amd_iommu(dev_data->dev);
+	iommu    = get_amd_iommu_from_dev(dev_data->dev);
 	if (!iommu)
 		return -EINVAL;
 
@@ -1434,7 +1434,7 @@ static int device_flush_dte(struct iommu_dev_data *dev_data)
 	u16 alias;
 	int ret;
 
-	iommu = rlookup_amd_iommu(dev_data->dev);
+	iommu = get_amd_iommu_from_dev(dev_data->dev);
 	if (!iommu)
 		return -EINVAL;
 
@@ -1805,7 +1805,7 @@ static void do_attach(struct iommu_dev_data *dev_data,
 	struct amd_iommu *iommu;
 	bool ats;
 
-	iommu = rlookup_amd_iommu(dev_data->dev);
+	iommu = get_amd_iommu_from_dev(dev_data->dev);
 	if (!iommu)
 		return;
 	ats   = dev_data->ats_enabled;
@@ -1835,7 +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);
+	iommu = get_amd_iommu_from_dev(dev_data->dev);
 	if (!iommu)
 		return;
 
@@ -1989,7 +1989,7 @@ static void amd_iommu_release_device(struct device *dev)
 	if (!check_device(dev))
 		return;
 
-	iommu = rlookup_amd_iommu(dev);
+	iommu = get_amd_iommu_from_dev(dev);
 	if (!iommu)
 		return;
 
@@ -2016,7 +2016,7 @@ 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;
@@ -2237,7 +2237,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;
 
 	/*
@@ -2388,7 +2388,7 @@ static void amd_iommu_get_resv_regions(struct device *dev,
 		return;
 
 	devid = PCI_SBDF_TO_DEVID(sbdf);
-	iommu = rlookup_amd_iommu(dev);
+	iommu = get_amd_iommu_from_dev(dev);
 	if (!iommu)
 		return;
 	pci_seg = iommu->pci_seg;
@@ -2560,7 +2560,7 @@ static int __flush_pasid(struct protection_domain *domain, u32 pasid,
 			continue;
 
 		qdep  = dev_data->ats_qdep;
-		iommu = rlookup_amd_iommu(dev_data->dev);
+		iommu = get_amd_iommu_from_dev(dev_data->dev);
 		if (!iommu)
 			continue;
 		build_inv_iotlb_pasid(&cmd, dev_data->devid, pasid,
@@ -2715,7 +2715,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);
+	iommu    = get_amd_iommu_from_dev(&pdev->dev);
 	if (!iommu)
 		return -ENODEV;
 
@@ -2835,7 +2835,7 @@ static int set_remap_table_entry_alias(struct pci_dev *pdev, u16 alias,
 {
 	struct irq_remap_table *table = data;
 	struct amd_iommu_pci_seg *pci_seg;
-	struct amd_iommu *iommu = rlookup_amd_iommu(&pdev->dev);
+	struct amd_iommu *iommu = get_amd_iommu_from_dev(&pdev->dev);
 
 	if (!iommu)
 		return -EINVAL;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index f1e18e81fca7..5a44567678ce 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -446,6 +446,20 @@ 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 of of 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 v2 03/10] 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>
---
 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 e742006f2885..eb32df5371c4 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -538,6 +538,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.
@@ -553,6 +558,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 */
 	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 002b29048813..bfacad9ddc14 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2102,6 +2102,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;
@@ -2110,6 +2111,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 v2 04/10] 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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index eb32df5371c4..1c4672ed90f4 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -530,6 +530,13 @@ 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 */
+	int	pasid_cnt;	/* Track attached PASIDs */
+	bool	giov;		/* GIOV bit support */
+};
+
 struct amd_io_pgtable {
 	struct io_pgtable_cfg	pgtbl_cfg;
 	struct io_pgtable	iop;
@@ -810,6 +817,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 v2 05/10] 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>
---
 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 bfacad9ddc14..ca092b128402 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2289,7 +2289,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;
 
@@ -2335,7 +2335,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 v2 06/10] 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.

  * Add internal functions which will be used by subsequent patches
    to set/clear default 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.

  * 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 | 20 ++-------
 drivers/iommu/amd/iommu.c         | 75 ++++++++++++++++++-------------
 3 files changed, 53 insertions(+), 50 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 639d82295797..92fba9bda3d2 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -55,15 +55,17 @@ 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,
+		       u32 pasid, unsigned long gcr3);
+int amd_iommu_clear_gcr3(struct iommu_dev_data *dev_data, u32 pasid);
+
 int amd_iommu_flush_page(struct protection_domain *domain, u32 pasid, u64 address);
 void amd_iommu_update_and_flush_device_table(struct protection_domain *domain);
 void amd_iommu_domain_update(struct protection_domain *domain);
 void amd_iommu_domain_flush_complete(struct protection_domain *domain);
 void amd_iommu_domain_flush_tlb_pde(struct protection_domain *domain);
 int amd_iommu_flush_tlb(struct protection_domain *domain, u32 pasid);
-int amd_iommu_domain_set_gcr3(struct protection_domain *domain, u32 pasid,
-			      unsigned long cr3);
-int amd_iommu_domain_clear_gcr3(struct protection_domain *domain, 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 c17cda83bca5..8c588f93cbed 100644
--- a/drivers/iommu/amd/io_pgtable_v2.c
+++ b/drivers/iommu/amd/io_pgtable_v2.c
@@ -360,34 +360,25 @@ static void v2_free_pgtable(struct io_pgtable *iop)
 	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))
-		return;
-
-	/* Clear gcr3 entry */
-	amd_iommu_domain_clear_gcr3(pdom, 0);
 
-	/* Make changes visible to IOMMUs */
-	amd_iommu_domain_update(pdom);
+	if (!pgtable->pgd)
+		return;
 
 	/* 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, 0, iommu_virt_to_phys(pgtable->pgd));
-	if (ret)
-		goto err_free_pgd;
-
 	if (get_pgtable_level() == PAGE_MODE_5_LEVEL)
 		ias = 57;
 
@@ -401,11 +392,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 ca092b128402..b54d4a0e2f4c 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -77,6 +77,11 @@ struct kmem_cache *amd_iommu_irq_cache;
 
 static void detach_device(struct device *dev);
 
+static int __set_gcr3(struct iommu_dev_data *dev_data,
+		       u32 pasid, unsigned long gcr3);
+
+static int __clear_gcr3(struct iommu_dev_data *dev_data, u32 pasid);
+
 /****************************************************************************
  *
  * Helper functions
@@ -2651,61 +2656,71 @@ static u64 *__get_gcr3_pte(u64 *root, int level, u32 pasid, bool alloc)
 	return pte;
 }
 
-static int __set_gcr3(struct protection_domain *domain, u32 pasid,
-		      unsigned long cr3)
+static int __set_gcr3(struct iommu_dev_data *dev_data,
+		      u32 pasid, unsigned long gcr3)
 {
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
 	u64 *pte;
 
-	if (domain->iop.mode != PAGE_MODE_NONE)
-		return -EINVAL;
+	lockdep_assert_held(&dev_data->lock);
 
-	pte = __get_gcr3_pte(domain->gcr3_tbl, domain->glx, pasid, true);
+	pte = __get_gcr3_pte(gcr3_info->gcr3_tbl,
+			     gcr3_info->glx, pasid, true);
 	if (pte == NULL)
 		return -ENOMEM;
 
-	*pte = (cr3 & PAGE_MASK) | GCR3_VALID;
+	*pte = (gcr3 & PAGE_MASK) | GCR3_VALID;
+	__amd_iommu_flush_tlb(dev_data->domain, pasid);
 
-	return __amd_iommu_flush_tlb(domain, pasid);
+	return 0;
 }
 
-static int __clear_gcr3(struct protection_domain *domain, u32 pasid)
+int amd_iommu_set_gcr3(struct iommu_dev_data *dev_data, u32 pasid,
+		       unsigned long gcr3)
 {
-	u64 *pte;
-
-	if (domain->iop.mode != PAGE_MODE_NONE)
-		return -EINVAL;
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
+	int ret;
 
-	pte = __get_gcr3_pte(domain->gcr3_tbl, domain->glx, pasid, false);
-	if (pte == NULL)
-		return 0;
+	spin_lock(&dev_data->lock);
 
-	*pte = 0;
+	ret = __set_gcr3(dev_data, pasid, gcr3);
+	if (!ret)
+		gcr3_info->pasid_cnt++;
 
-	return __amd_iommu_flush_tlb(domain, pasid);
+	spin_unlock(&dev_data->lock);
+	return ret;
 }
 
-int amd_iommu_domain_set_gcr3(struct protection_domain *domain, u32 pasid,
-			      unsigned long cr3)
+static int __clear_gcr3(struct iommu_dev_data *dev_data, u32 pasid)
 {
-	unsigned long flags;
-	int ret;
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
+	u64 *pte;
 
-	spin_lock_irqsave(&domain->lock, flags);
-	ret = __set_gcr3(domain, pasid, cr3);
-	spin_unlock_irqrestore(&domain->lock, flags);
+	lockdep_assert_held(&dev_data->lock);
 
-	return ret;
+	pte = __get_gcr3_pte(gcr3_info->gcr3_tbl,
+			     gcr3_info->glx, pasid, false);
+	if (pte == NULL)
+		return -EINVAL;
+
+	*pte = 0;
+	__amd_iommu_flush_tlb(dev_data->domain, pasid);
+
+	return 0;
 }
 
-int amd_iommu_domain_clear_gcr3(struct protection_domain *domain, u32 pasid)
+int amd_iommu_clear_gcr3(struct iommu_dev_data *dev_data, u32 pasid)
 {
-	unsigned long flags;
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
 	int ret;
 
-	spin_lock_irqsave(&domain->lock, flags);
-	ret = __clear_gcr3(domain, pasid);
-	spin_unlock_irqrestore(&domain->lock, flags);
+	spin_lock(&dev_data->lock);
 
+	ret = __clear_gcr3(dev_data, pasid);
+	if (!ret)
+		gcr3_info->pasid_cnt--;
+
+	spin_unlock(&dev_data->lock);
 	return ret;
 }
 
-- 
2.31.1

[PATCH v2 07/10] iommu/amd: Refactor helper function for attaching / detaching device

[Vasant Hegde]

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

To use the new helper function for setting up GCR3 table.

If system is booted 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.

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 | 51 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index b54d4a0e2f4c..421f827fc24b 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1804,15 +1804,38 @@ 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 _init_gcr3_tbl(struct iommu_dev_data *dev_data)
+{
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
+
+	/* By default, GCR3 is set to support non-PASID devices. */
+	gcr3_info->giov = true;
+
+	/*
+	 * By default, setup GCR3 table to support MAX PASIDs
+	 * support by the IOMMU HW.
+	 */
+	return setup_gcr3_table(dev_data->domain, -1);
+}
+
+static inline void _destroy_gcr3_tbl(struct iommu_dev_data *dev_data)
+{
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
+
+	gcr3_info->giov = false;
+	free_gcr3_table(dev_data->domain);
+}
+
+static int do_attach(struct iommu_dev_data *dev_data,
+		     struct protection_domain *domain)
 {
 	struct amd_iommu *iommu;
 	bool ats;
+	int ret = 0;
 
 	iommu = get_amd_iommu_from_dev(dev_data->dev);
 	if (!iommu)
-		return;
+		return -EINVAL;
 	ats   = dev_data->ats_enabled;
 
 	/* Update data structures */
@@ -1827,12 +1850,27 @@ static void do_attach(struct iommu_dev_data *dev_data,
 	domain->dev_iommu[iommu->index] += 1;
 	domain->dev_cnt                 += 1;
 
+	if (domain->pd_mode == PD_MODE_V2) {
+		ret = _init_gcr3_tbl(dev_data);
+		if (ret)
+			return ret;
+
+		ret = __set_gcr3(dev_data, 0,
+				 iommu_virt_to_phys(domain->iop.pgd));
+		if (ret) {
+			_destroy_gcr3_tbl(dev_data);
+			return ret;
+		}
+	}
+
 	/* Update device table */
 	set_dte_entry(iommu, dev_data->devid, domain,
 		      ats, dev_data->ppr);
 	clone_aliases(iommu, dev_data->dev);
 
 	device_flush_dte(dev_data);
+
+	return ret;
 }
 
 static void do_detach(struct iommu_dev_data *dev_data)
@@ -1844,6 +1882,11 @@ static void do_detach(struct iommu_dev_data *dev_data)
 	if (!iommu)
 		return;
 
+	if (domain->pd_mode == PD_MODE_V2) {
+		__clear_gcr3(dev_data, 0);
+		_destroy_gcr3_tbl(dev_data);
+	}
+
 	/* Update data structures */
 	dev_data->domain = NULL;
 	list_del(&dev_data->list);
@@ -1889,7 +1932,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);
 
 	/*
 	 * We might boot into a crash-kernel here. The crashed kernel
-- 
2.31.1

[PATCH v2 08/10] 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>
---
 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 421f827fc24b..be68d77861f5 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2126,9 +2126,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);
 
@@ -2156,15 +2153,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 v2 09/10] 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. 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 | 76 ++++++++++++++++++++++++++-------------
 1 file changed, 51 insertions(+), 25 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index be68d77861f5..8d3d520a525a 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -77,6 +77,11 @@ struct kmem_cache *amd_iommu_irq_cache;
 
 static void detach_device(struct device *dev);
 
+static void set_dte_entry(struct amd_iommu *iommu, u16 devid,
+			  struct gcr3_tbl_info *gcr3_info,
+			  struct protection_domain *domain, bool ats,
+			  bool ppr);
+
 static int __set_gcr3(struct iommu_dev_data *dev_data,
 		       u32 pasid, unsigned long gcr3);
 
@@ -1659,16 +1664,27 @@ 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;
+	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
+
+	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);
 
-	free_page((unsigned long)domain->gcr3_tbl);
+	gcr3_info->glx = 0;
+
+	set_dte_entry(iommu, dev_data->devid, NULL, dev_data->domain,
+		      dev_data->ats_enabled, dev_data->ppr);
+	clone_aliases(iommu, dev_data->dev);
+	device_flush_dte(dev_data);
+
+	free_page((unsigned long)gcr3_info->gcr3_tbl);
+	gcr3_info->gcr3_tbl = NULL;
 }
 
 /*
@@ -1687,28 +1703,37 @@ 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;
+	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
 	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)
+	if (gcr3_info->gcr3_tbl)
+		return -EBUSY;
+
+	gcr3_info->gcr3_tbl = alloc_pgtable_page(dev_to_node(dev_data->dev),
+						 GFP_ATOMIC);
+	if (gcr3_info->gcr3_tbl == NULL)
 		return -ENOMEM;
 
-	domain->glx      = levels;
-	domain->flags   |= PD_IOMMUV2_MASK;
+	gcr3_info->glx = levels;
 
-	amd_iommu_domain_update(domain);
+	set_dte_entry(iommu, dev_data->devid, &dev_data->gcr3_info,
+		      dev_data->domain, dev_data->ats_enabled, dev_data->ppr);
+	clone_aliases(iommu, dev_data->dev);
+	device_flush_dte(dev_data);
 
 	return 0;
 }
 
 static void set_dte_entry(struct amd_iommu *iommu, u16 devid,
-			  struct protection_domain *domain, bool ats, bool ppr)
+			  struct gcr3_tbl_info *gcr3_info,
+			  struct protection_domain *domain, bool ats,
+			  bool ppr)
 {
 	u64 pte_root = 0;
 	u64 flags = 0;
@@ -1738,9 +1763,9 @@ static void set_dte_entry(struct amd_iommu *iommu, u16 devid,
 	if (ppr)
 		pte_root |= 1ULL << DEV_ENTRY_PPR;
 
-	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;
@@ -1768,7 +1793,7 @@ static void set_dte_entry(struct amd_iommu *iommu, u16 devid,
 				((u64)GUEST_PGTABLE_5_LEVEL << DTE_GPT_LEVEL_SHIFT);
 		}
 
-		if (domain->flags & PD_GIOV_MASK)
+		if (gcr3_info->giov)
 			pte_root |= DTE_FLAG_GIOV;
 	}
 
@@ -1815,7 +1840,7 @@ static int _init_gcr3_tbl(struct iommu_dev_data *dev_data)
 	 * By default, setup GCR3 table to support MAX PASIDs
 	 * support by the IOMMU HW.
 	 */
-	return setup_gcr3_table(dev_data->domain, -1);
+	return setup_gcr3_table(dev_data, -1);
 }
 
 static inline void _destroy_gcr3_tbl(struct iommu_dev_data *dev_data)
@@ -1823,7 +1848,7 @@ static inline void _destroy_gcr3_tbl(struct iommu_dev_data *dev_data)
 	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
 
 	gcr3_info->giov = false;
-	free_gcr3_table(dev_data->domain);
+	free_gcr3_table(dev_data);
 }
 
 static int do_attach(struct iommu_dev_data *dev_data,
@@ -1864,8 +1889,8 @@ static int do_attach(struct iommu_dev_data *dev_data,
 	}
 
 	/* Update device table */
-	set_dte_entry(iommu, dev_data->devid, domain,
-		      ats, dev_data->ppr);
+	set_dte_entry(iommu, dev_data->devid, &dev_data->gcr3_info,
+		      domain, ats, dev_data->ppr);
 	clone_aliases(iommu, dev_data->dev);
 
 	device_flush_dte(dev_data);
@@ -2068,8 +2093,9 @@ 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->devid, &dev_data->gcr3_info,
+			      domain, dev_data->ats_enabled, dev_data->ppr);
 		clone_aliases(iommu, dev_data->dev);
 	}
 }
-- 
2.31.1

[PATCH v2 10/10] 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.

Also remove V2 page table check in __flush_pasid() as this gets called
only when we have PASIDs enabled.

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 | 12 ------------
 drivers/iommu/amd/iommu.c           |  3 ---
 2 files changed, 15 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 1c4672ed90f4..b1a54d8c7506 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -442,15 +442,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
@@ -561,10 +552,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 */
 	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 8d3d520a525a..15fce04ac59b 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2594,9 +2594,6 @@ static int __flush_pasid(struct protection_domain *domain, u32 pasid,
 	struct iommu_cmd cmd;
 	int i, ret;
 
-	if (!(domain->flags & PD_IOMMUV2_MASK))
-		return -EINVAL;
-
 	build_inv_iommu_pasid(&cmd, domain->id, pasid, address, size);
 
 	/*
-- 
2.31.1

Re: [PATCH v2 01/10] iommu/amd: Use struct protection_domain in helper functions

[Jason Gunthorpe]

On Wed, Aug 16, 2023 at 05:40:22PM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> To simplify the unnecessary use of container_of() on the struct
> iommu_domain to get the container structure.
> 
> No functional changes intended.
> 
> 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     | 13 +++++++++----
>  drivers/iommu/amd/io_pgtable_v2.c |  8 ++++----
>  drivers/iommu/amd/iommu.c         | 18 ++++--------------
>  3 files changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
> index 8b586effbefc..87739d95ea8b 100644
> --- a/drivers/iommu/amd/amd_iommu.h
> +++ b/drivers/iommu/amd/amd_iommu.h
> @@ -55,15 +55,15 @@ 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);
>  
> -int amd_iommu_flush_page(struct iommu_domain *dom, u32 pasid, u64 address);
> +int amd_iommu_flush_page(struct protection_domain *domain, u32 pasid, u64 address);
>  void amd_iommu_update_and_flush_device_table(struct protection_domain *domain);
>  void amd_iommu_domain_update(struct protection_domain *domain);
>  void amd_iommu_domain_flush_complete(struct protection_domain *domain);
>  void amd_iommu_domain_flush_tlb_pde(struct protection_domain *domain);
> -int amd_iommu_flush_tlb(struct iommu_domain *dom, u32 pasid);
> -int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, u32 pasid,
> +int amd_iommu_flush_tlb(struct protection_domain *domain, u32 pasid);
> +int amd_iommu_domain_set_gcr3(struct protection_domain *domain, u32 pasid,
>  			      unsigned long cr3);
> -int amd_iommu_domain_clear_gcr3(struct iommu_domain *dom, u32 pasid);
> +int amd_iommu_domain_clear_gcr3(struct protection_domain *domain, u32 pasid);
>  
>  #ifdef CONFIG_IRQ_REMAP
>  int amd_iommu_create_irq_domain(struct amd_iommu *iommu);
> @@ -147,6 +147,11 @@ static inline void *alloc_pgtable_page(int nid, gfp_t gfp)
>  	return page ? page_address(page) : NULL;
>  }
>  
> +static inline struct protection_domain *to_pdomain(struct iommu_domain *dom)
> +{
> +	return container_of(dom, struct protection_domain, domain);
> +}
> +

The change looks fine, but why did this patch move this into a header?

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

Jason

Re: [PATCH v2 01/10] iommu/amd: Use struct protection_domain in helper functions

[Vasant Hegde]

Jason,


On 8/23/2023 10:12 PM, Jason Gunthorpe wrote:
> On Wed, Aug 16, 2023 at 05:40:22PM +0000, Vasant Hegde wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> To simplify the unnecessary use of container_of() on the struct
>> iommu_domain to get the container structure.
>>
>> No functional changes intended.
>>
>> 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     | 13 +++++++++----
>>  drivers/iommu/amd/io_pgtable_v2.c |  8 ++++----
>>  drivers/iommu/amd/iommu.c         | 18 ++++--------------
>>  3 files changed, 17 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
>> index 8b586effbefc..87739d95ea8b 100644
>> --- a/drivers/iommu/amd/amd_iommu.h
>> +++ b/drivers/iommu/amd/amd_iommu.h
>> @@ -55,15 +55,15 @@ 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);
>>  
>> -int amd_iommu_flush_page(struct iommu_domain *dom, u32 pasid, u64 address);
>> +int amd_iommu_flush_page(struct protection_domain *domain, u32 pasid, u64 address);
>>  void amd_iommu_update_and_flush_device_table(struct protection_domain *domain);
>>  void amd_iommu_domain_update(struct protection_domain *domain);
>>  void amd_iommu_domain_flush_complete(struct protection_domain *domain);
>>  void amd_iommu_domain_flush_tlb_pde(struct protection_domain *domain);
>> -int amd_iommu_flush_tlb(struct iommu_domain *dom, u32 pasid);
>> -int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, u32 pasid,
>> +int amd_iommu_flush_tlb(struct protection_domain *domain, u32 pasid);
>> +int amd_iommu_domain_set_gcr3(struct protection_domain *domain, u32 pasid,
>>  			      unsigned long cr3);
>> -int amd_iommu_domain_clear_gcr3(struct iommu_domain *dom, u32 pasid);
>> +int amd_iommu_domain_clear_gcr3(struct protection_domain *domain, u32 pasid);
>>  
>>  #ifdef CONFIG_IRQ_REMAP
>>  int amd_iommu_create_irq_domain(struct amd_iommu *iommu);
>> @@ -147,6 +147,11 @@ static inline void *alloc_pgtable_page(int nid, gfp_t gfp)
>>  	return page ? page_address(page) : NULL;
>>  }
>>  
>> +static inline struct protection_domain *to_pdomain(struct iommu_domain *dom)
>> +{
>> +	return container_of(dom, struct protection_domain, domain);
>> +}
>> +
> 
> The change looks fine, but why did this patch move this into a header?

We wanted to use this function in other files as well. But lot of code changed
now. Now its only iommu.c is using this function. I will revert above change for
now.

-Vasant

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

Re: [PATCH v2 02/10] iommu/amd: Introduce get_amd_iommu_from_dev()

[Jason Gunthorpe]

On Wed, Aug 16, 2023 at 05:40:23PM +0000, Vasant Hegde wrote:
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index f1e18e81fca7..5a44567678ce 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -446,6 +446,20 @@ 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

No space before kdoc parameters

> + *
> + * Note that this function must be called from of of 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;
> +}

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

Jason

Re: [PATCH v2 03/10] iommu/amd: Introduce struct protection_domain.pd_mode

[Jason Gunthorpe]

On Wed, Aug 16, 2023 at 05:40:24PM +0000, Vasant Hegde wrote:
> 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>
> ---
>  drivers/iommu/amd/amd_iommu_types.h | 6 ++++++
>  drivers/iommu/amd/iommu.c           | 2 ++
>  2 files changed, 8 insertions(+)

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

Jason

Re: [PATCH v2 04/10] iommu/amd: Introduce per-device GCR3 table

[Jason Gunthorpe]

On Wed, Aug 16, 2023 at 05:40:25PM +0000, Vasant Hegde wrote:
> 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 | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index eb32df5371c4..1c4672ed90f4 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -530,6 +530,13 @@ 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 */
> +	int	pasid_cnt;	/* Track attached PASIDs */

These should be unsigned values since they can't be negative

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

Jason

Re: [PATCH v2 05/10] iommu/amd: Use protection_domain.flags to check page table mode

[Jason Gunthorpe]

On Wed, Aug 16, 2023 at 05:40:26PM +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>
> ---
>  drivers/iommu/amd/iommu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

Jason

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

[Jason Gunthorpe]

On Wed, Aug 16, 2023 at 05:40:27PM +0000, Vasant Hegde wrote:
> @@ -2651,61 +2656,71 @@ static u64 *__get_gcr3_pte(u64 *root, int level, u32 pasid, bool alloc)
>  	return pte;
>  }
>  
> -static int __set_gcr3(struct protection_domain *domain, u32 pasid,
> -		      unsigned long cr3)
> +static int __set_gcr3(struct iommu_dev_data *dev_data,
> +		      u32 pasid, unsigned long gcr3)
>  {
> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
>  	u64 *pte;
>  
> -	if (domain->iop.mode != PAGE_MODE_NONE)
> -		return -EINVAL;
> +	lockdep_assert_held(&dev_data->lock);
>  
> -	pte = __get_gcr3_pte(domain->gcr3_tbl, domain->glx, pasid, true);
> +	pte = __get_gcr3_pte(gcr3_info->gcr3_tbl,
> +			     gcr3_info->glx, pasid, true);

It would be a nice touch to change these kinds of functions to take in
the struct gcr3_tbl_info instead of two parameters

>  	if (pte == NULL)
>  		return -ENOMEM;
>  
> -	*pte = (cr3 & PAGE_MASK) | GCR3_VALID;
> +	*pte = (gcr3 & PAGE_MASK) | GCR3_VALID;
> +	__amd_iommu_flush_tlb(dev_data->domain, pasid);

This flushing doesn't seem to make sense to me, it shouldn't take in a
domain parameter.

At this point we changed the gcr3 table for a single iommu_dev_data
and so this change has exactly one iommu, device, and cache tag that
needs flushing.

I'm reading the spec here so I may get this wrong but.. It looks like
the IOTLB cache is tagged by (DTE.domain_id, PASID)?

So in v1 mode PASID is always zero and DTE.domain_id should refer to
a per-domain ID.

In v2 mode PASID can vary and many DTEs in the system can have
aliasing PASIDs. ie DTE A and B may have different translations for
PASID 0.

Understand that the core iommu code does not provide a guarentee that
PASID is non-aliasing. It is perfectly allowed that the same PASID can
point to different translations on different devices.

So this looks broken to me. The RID domain should not provide
DTE.domain_id in v2 mode. DTE.domain_id needs to reflect a global set
of non-aliasing PASIDs.

The naive way to make this work is to have DTE.domain_id be per-GCR3
table (eg per-device) when in v2 mode. This will obviously make the
cache tag work correctly.

This HW has a similar, though IMHO worse, cache tag misdesign as
Intel's does. The choice of cache tags makes very little sense for the
SW model we have. ARM got this correct where (in AMD language) there
is a per-GCR3 entry cache tag (ASID) and a per DTE entry cache tag
(VMID) for the v1 host translation. The IOTLB never uses PASID as a
tag.

Jason

Re: [PATCH v2 02/10] iommu/amd: Introduce get_amd_iommu_from_dev()

[Vasant Hegde]

Jason,


On 9/13/2023 8:12 PM, Jason Gunthorpe wrote:
> On Wed, Aug 16, 2023 at 05:40:23PM +0000, Vasant Hegde wrote:
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index f1e18e81fca7..5a44567678ce 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -446,6 +446,20 @@ 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
> 
> No space before kdoc parameters

Fixed.

Thanks
-Vasant

Re: [PATCH v2 04/10] iommu/amd: Introduce per-device GCR3 table

[Vasant Hegde]

On 9/13/2023 8:13 PM, Jason Gunthorpe wrote:
> On Wed, Aug 16, 2023 at 05:40:25PM +0000, Vasant Hegde wrote:
>> 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 | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
>> index eb32df5371c4..1c4672ed90f4 100644
>> --- a/drivers/iommu/amd/amd_iommu_types.h
>> +++ b/drivers/iommu/amd/amd_iommu_types.h
>> @@ -530,6 +530,13 @@ 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 */
>> +	int	pasid_cnt;	/* Track attached PASIDs */
> 
> These should be unsigned values since they can't be negative

Fixed.

Thanks
Vasant

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

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

[Vasant Hegde]

Jason,


On 9/13/2023 8:42 PM, Jason Gunthorpe wrote:
> On Wed, Aug 16, 2023 at 05:40:27PM +0000, Vasant Hegde wrote:
>> @@ -2651,61 +2656,71 @@ static u64 *__get_gcr3_pte(u64 *root, int level, u32 pasid, bool alloc)
>>  	return pte;
>>  }
>>  
>> -static int __set_gcr3(struct protection_domain *domain, u32 pasid,
>> -		      unsigned long cr3)
>> +static int __set_gcr3(struct iommu_dev_data *dev_data,
>> +		      u32 pasid, unsigned long gcr3)
>>  {
>> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
>>  	u64 *pte;
>>  
>> -	if (domain->iop.mode != PAGE_MODE_NONE)
>> -		return -EINVAL;
>> +	lockdep_assert_held(&dev_data->lock);
>>  
>> -	pte = __get_gcr3_pte(domain->gcr3_tbl, domain->glx, pasid, true);
>> +	pte = __get_gcr3_pte(gcr3_info->gcr3_tbl,
>> +			     gcr3_info->glx, pasid, true);
> 
> It would be a nice touch to change these kinds of functions to take in
> the struct gcr3_tbl_info instead of two parameters

Agree. But that is kind of not relevant to this patch. Hence I didn't make that
change. I intend to re-arrange the code within this file little bit after this
series. I will address this along with re-arranging code.


> 
>>  	if (pte == NULL)
>>  		return -ENOMEM;
>>  
>> -	*pte = (cr3 & PAGE_MASK) | GCR3_VALID;
>> +	*pte = (gcr3 & PAGE_MASK) | GCR3_VALID;
>> +	__amd_iommu_flush_tlb(dev_data->domain, pasid);
> 
> This flushing doesn't seem to make sense to me, it shouldn't take in a
> domain parameter.
>
> I'm reading the spec here so I may get this wrong but.. It looks like
> the IOTLB cache is tagged by (DTE.domain_id, PASID)?
> 

Yeah. It works, but name is confusing.
I don't wanted to touch invalidation code in this series. But on second thought
I am thinking of getting invalidation series on top of Part2 and then rebase
this series on top of that. I will add function to something like

amd_iommu_device_flush_pasid_all(dev_data, pasid)
{
	// Flush domain
	// Flush single device ATS
}


> So in v1 mode PASID is always zero and DTE.domain_id should refer to
> a per-domain ID.
> 
> In v2 mode PASID can vary and many DTEs in the system can have
> aliasing PASIDs. ie DTE A and B may have different translations for
> PASID 0.

PASID 0 GCR3 is shared by all devices in the domain. So it should be fine.

> 
> Understand that the core iommu code does not provide a guarentee that
> PASID is non-aliasing. It is perfectly allowed that the same PASID can
> point to different translations on different devices.

Sure.

> 
> So this looks broken to me. The RID domain should not provide
> DTE.domain_id in v2 mode. DTE.domain_id needs to reflect a global set
> of non-aliasing PASIDs.

In our case domain ID is per device, not per PASID. Also when we enable PASID we
will have single PASID capable device in the group (ACS requirement). So it
works fine.


> 
> The naive way to make this work is to have DTE.domain_id be per-GCR3
> table (eg per-device) when in v2 mode. This will obviously make the
> cache tag work correctly.

This will impact the performance as have to do unnecessary invalidations.

-Vasant

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

[Jason Gunthorpe]

On Wed, Sep 27, 2023 at 11:51:20AM +0530, Vasant Hegde wrote:
> >>  	if (pte == NULL)
> >>  		return -ENOMEM;
> >>  
> >> -	*pte = (cr3 & PAGE_MASK) | GCR3_VALID;
> >> +	*pte = (gcr3 & PAGE_MASK) | GCR3_VALID;
> >> +	__amd_iommu_flush_tlb(dev_data->domain, pasid);
> > 
> > This flushing doesn't seem to make sense to me, it shouldn't take in a
> > domain parameter.
> >
> > I'm reading the spec here so I may get this wrong but.. It looks like
> > the IOTLB cache is tagged by (DTE.domain_id, PASID)?
> > 
> 
> Yeah. It works, but name is confusing.

How does it work? You haven't explained anything about why does it
work.

Nothing guarantees that domain->id is per-device. It is emphatically
not, it is per-domain. It happens to act like it is per-device when
using the DMA API, as DMA API does not share domains across
groups. However iommufd does share domains and so this is not a
correct assumption.

You said below that you think the DTE.domain_id is per-device. I agree
that would fix this bug, if true. If it is to be per-device it should
come from the struct iommu_dev_data, not from the domain->id. This
should happen any time a GCR3 table is in-use. v1 mode is fine to use
a per-domain 'domain_id'.

> amd_iommu_device_flush_pasid_all(dev_data, pasid)
> {
>         // Flush domain
>        // Flush single device ATS
> }

That would make more logical sense, but it does not resolve the issue
of correctly choosing DTE.domain_id so the cache tags don't alias.

Jason

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

[Vasant Hegde]

On 9/27/2023 10:15 PM, Jason Gunthorpe wrote:
> On Wed, Sep 27, 2023 at 11:51:20AM +0530, Vasant Hegde wrote:
>>>>  	if (pte == NULL)
>>>>  		return -ENOMEM;
>>>>  
>>>> -	*pte = (cr3 & PAGE_MASK) | GCR3_VALID;
>>>> +	*pte = (gcr3 & PAGE_MASK) | GCR3_VALID;
>>>> +	__amd_iommu_flush_tlb(dev_data->domain, pasid);
>>>
>>> This flushing doesn't seem to make sense to me, it shouldn't take in a
>>> domain parameter.
>>>
>>> I'm reading the spec here so I may get this wrong but.. It looks like
>>> the IOTLB cache is tagged by (DTE.domain_id, PASID)?
>>>
>>
>> Yeah. It works, but name is confusing.
> 
> How does it work? You haven't explained anything about why does it
> work.

__amd_iommu_flush_tlb() flushes both domain ID and all devices within the domain
for the given PASID. So it works fine. All these functions are reworked as part
of invalidation series. Hence this function is not relevant anymore.


> 
> Nothing guarantees that domain->id is per-device. It is emphatically
> not, it is per-domain. It happens to act like it is per-device when
> using the DMA API, as DMA API does not share domains across
> groups. However iommufd does share domains and so this is not a
> correct assumption.

That's exactly why I have been saying for SVA I don't need per-device-domain ID.
Current code works fine. Anyway, I will add this support in v3.


-Vasant

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

[Jason Gunthorpe]

On Tue, Oct 10, 2023 at 11:32:53AM +0530, Vasant Hegde wrote:

> __amd_iommu_flush_tlb() flushes both domain ID and all devices within the domain
> for the given PASID. So it works fine. All these functions are reworked as part
> of invalidation series. Hence this function is not relevant anymore.

But the replacement has the same issue:

static int domain_flush_tlb_range(struct protection_domain *pdom,
                                  ioasid_t pasid, u64 address, size_t size)
{
        struct iommu_cmd cmd;
        int i, ret = 0;
        bool gn = is_pasid_valid(pasid);

        build_inv_iommu_pages(&cmd, address, size, pdom->id, pasid, gn);
                                              ^^^^^^^^^^^^^^^

(btw I have no issue/comments with the invalidate series, it just
doesn't go far enough to correctly support the iommu core's PASID ops)

> > Nothing guarantees that domain->id is per-device. It is emphatically
> > not, it is per-domain. It happens to act like it is per-device when
> > using the DMA API, as DMA API does not share domains across
> > groups. However iommufd does share domains and so this is not a
> > correct assumption.
> 
> That's exactly why I have been saying for SVA I don't need
> per-device-domain ID.  Current code works fine. Anyway, I will add
> this support in v3.

The threshold for upstream is more than "works fine" in some limited
testing.

It needs to correctly implement the driver op APIs we have defined in
the core code. In this regard it does not "work fine", you can see it
by code inspection that it is not producing a correct cache tag in the
IOTLB for domains attached via op->set_dev_pasid()

So, lets see v3.

Jason

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

[Vasant Hegde]

On 10/10/2023 8:08 PM, Jason Gunthorpe wrote:
> On Tue, Oct 10, 2023 at 11:32:53AM +0530, Vasant Hegde wrote:
> 
>> __amd_iommu_flush_tlb() flushes both domain ID and all devices within the domain
>> for the given PASID. So it works fine. All these functions are reworked as part
>> of invalidation series. Hence this function is not relevant anymore.
> 
> But the replacement has the same issue:
> 
> static int domain_flush_tlb_range(struct protection_domain *pdom,
>                                   ioasid_t pasid, u64 address, size_t size)
> {
>         struct iommu_cmd cmd;
>         int i, ret = 0;
>         bool gn = is_pasid_valid(pasid);
> 
>         build_inv_iommu_pages(&cmd, address, size, pdom->id, pasid, gn);
>                                               ^^^^^^^^^^^^^^^
> 
> (btw I have no issue/comments with the invalidate series, it just
> doesn't go far enough to correctly support the iommu core's PASID ops)

Invalidation series [1] is cleaning up existing code. Device oriented flushing
logic will be introduced in later series.

[1]
https://lore.kernel.org/linux-iommu/20231006101624.5912-1-vasant.hegde@amd.com/T/#t

> 
>>> Nothing guarantees that domain->id is per-device. It is emphatically
>>> not, it is per-domain. It happens to act like it is per-device when
>>> using the DMA API, as DMA API does not share domains across
>>> groups. However iommufd does share domains and so this is not a
>>> correct assumption.
>>
>> That's exactly why I have been saying for SVA I don't need
>> per-device-domain ID.  Current code works fine. Anyway, I will add
>> this support in v3.
> 
> The threshold for upstream is more than "works fine" in some limited
> testing.

I know that.

> 
> It needs to correctly implement the driver op APIs we have defined in
> the core code. In this regard it does not "work fine", you can see it
> by code inspection that it is not producing a correct cache tag in the
> IOTLB for domains attached via op->set_dev_pasid()

Domain centered approach works fine (not just limited testing) for enabling SVA
with DMA API mode in host. Going to device specific domain ID will introduce
performance penalty.

When we support PASID with UNMANAGED domain and vIOMMU we do need
per-device-domain-id. Anyway for now I have introduced per-device-domain-ID for
as soon as I put device in V2 page table mode [2].

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

-Vasant

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

[Jason Gunthorpe]

On Fri, Oct 13, 2023 at 09:15:19PM +0530, Vasant Hegde wrote:
> 
> 
> On 10/10/2023 8:08 PM, Jason Gunthorpe wrote:
> > On Tue, Oct 10, 2023 at 11:32:53AM +0530, Vasant Hegde wrote:
> > 
> >> __amd_iommu_flush_tlb() flushes both domain ID and all devices within the domain
> >> for the given PASID. So it works fine. All these functions are reworked as part
> >> of invalidation series. Hence this function is not relevant anymore.
> > 
> > But the replacement has the same issue:
> > 
> > static int domain_flush_tlb_range(struct protection_domain *pdom,
> >                                   ioasid_t pasid, u64 address, size_t size)
> > {
> >         struct iommu_cmd cmd;
> >         int i, ret = 0;
> >         bool gn = is_pasid_valid(pasid);
> > 
> >         build_inv_iommu_pages(&cmd, address, size, pdom->id, pasid, gn);
> >                                               ^^^^^^^^^^^^^^^
> > 
> > (btw I have no issue/comments with the invalidate series, it just
> > doesn't go far enough to correctly support the iommu core's PASID ops)
> 
> Invalidation series [1] is cleaning up existing code. Device oriented flushing
> logic will be introduced in later series.
> 
> [1]
> https://lore.kernel.org/linux-iommu/20231006101624.5912-1-vasant.hegde@amd.com/T/#t

I'll look

> > It needs to correctly implement the driver op APIs we have defined in
> > the core code. In this regard it does not "work fine", you can see it
> > by code inspection that it is not producing a correct cache tag in the
> > IOTLB for domains attached via op->set_dev_pasid()
> 
> Domain centered approach works fine (not just limited testing) for enabling SVA
> with DMA API mode in host. Going to device specific domain ID will introduce
> performance penalty.

That is exactly what I mean by limited testing. "DMA API mode" can
only excercise a small portion of the defined API.

We can discuss ways to optimize in the driver, but for now correctness
is more important. (I also don't see the performance penalty given DMA
API is already creating per-device domain IDs)

> When we support PASID with UNMANAGED domain and vIOMMU we do need
> per-device-domain-id.

You need it now before you implement any PASID support, it is part of
the API.

iommufd will support SVA and iommufd will not be limited like DMA API
mode. A driver must implement the defined API correctly from the
start. This is what I'm trying to explain to you as the threshold.

> Anyway for now I have introduced per-device-domain-ID for
> as soon as I put device in V2 page table mode [2].

Great, thanks!

Jason