[PATCH v1 00/13] Improve TLB invalidation logic

[PATCH v1 00/13] Improve TLB invalidation logic

[Vasant Hegde]

Current code invalidates single page and entire range for the given
domain ID. IOMMU hardware supports multiple page invalidation.
This series adds support to invalidate range of pages.

Also consolidate various invalidation functions, remove unused functions,
re-arranges invalidation related functions.

Note that this series doesn't add support for PASID based invalidation (except
PASID 0 for v2 page table). We will include necessary changes in SVA series.

Testing:
  - This series is tested with v1 and v2 page table and it works fine.

This patch series is based on top of SVA Part 2 [1] which in turn based on
top of iommu/next branch (Commit 8e5ab3f54a10).
  [1] https://lore.kernel.org/linux-iommu/20231006095706.5694-1-vasant.hegde@amd.com/T/#t

This is also available at github :
   https://github.com/AMDESE/linux/tree/iommu_flush_improvement_v1_v6.6_rc1


Thank you,
Vasant


Vasant Hegde (13):
  iommu/amd: Rename iommu_flush_all_caches() -> amd_iommu_flush_all_caches()
  iommu/amd: Remove redundant domain flush from attach_device()
  iommu/amd: Remove redundant passing of PDE bit
  iommu/amd: Add support to invalidate multiple guest pages
  iommu/amd: Refactor IOMMU tlb invalidation code
  iommu/amd: Refactor device iotlb invalidation code
  iommu/amd: Consolidate device IOTLB flush code
  iommu/amd: Consolidate amd_iommu_domain_flush_complete() call
  iommu/amd: Refactor domain flush global function
  iommu/amd: Consolidate domain flush logic
  iommu/amd/pgtbl_v2: Invalidate updated page ranges only
  iommu/amd: Remove unused flush pasid functions
  iommu/amd: Rearrange device flush code

 drivers/iommu/amd/amd_iommu.h       |  12 +-
 drivers/iommu/amd/amd_iommu_types.h |   6 -
 drivers/iommu/amd/init.c            |   8 +-
 drivers/iommu/amd/io_pgtable.c      |   5 +-
 drivers/iommu/amd/io_pgtable_v2.c   |  10 +-
 drivers/iommu/amd/iommu.c           | 417 ++++++++++++----------------
 6 files changed, 189 insertions(+), 269 deletions(-)

-- 
2.31.1

[PATCH v1 01/13] iommu/amd: Rename iommu_flush_all_caches() -> amd_iommu_flush_all_caches()

[Vasant Hegde]

Rename function inline with driver naming convention.

No functional changes.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu.h       | 5 +++++
 drivers/iommu/amd/amd_iommu_types.h | 6 ------
 drivers/iommu/amd/init.c            | 8 ++++----
 drivers/iommu/amd/iommu.c           | 2 +-
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 86be1edd50ee..234db57cd320 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -53,6 +53,11 @@ 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);
+/*
+ * This function flushes all internal caches of
+ * the IOMMU used by this driver.
+ */
+void amd_iommu_flush_all_caches(struct amd_iommu *iommu);
 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);
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index e742006f2885..ba40e5066300 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -890,12 +890,6 @@ extern int amd_iommu_max_glx_val;
 extern u64 amd_iommu_efr;
 extern u64 amd_iommu_efr2;
 
-/*
- * This function flushes all internal caches of
- * the IOMMU used by this driver.
- */
-void iommu_flush_all_caches(struct amd_iommu *iommu);
-
 static inline int get_ioapic_devid(int id)
 {
 	struct devid_map *entry;
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 64bcf3df37ee..c83bd0c2a1c9 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -2223,7 +2223,7 @@ static int __init amd_iommu_init_pci(void)
 		init_device_table_dma(pci_seg);
 
 	for_each_iommu(iommu)
-		iommu_flush_all_caches(iommu);
+		amd_iommu_flush_all_caches(iommu);
 
 	print_iommu_info();
 
@@ -2773,7 +2773,7 @@ static void early_enable_iommu(struct amd_iommu *iommu)
 	iommu_enable_xt(iommu);
 	iommu_enable_irtcachedis(iommu);
 	iommu_enable(iommu);
-	iommu_flush_all_caches(iommu);
+	amd_iommu_flush_all_caches(iommu);
 }
 
 /*
@@ -2829,7 +2829,7 @@ static void early_enable_iommus(void)
 			iommu_enable_xt(iommu);
 			iommu_enable_irtcachedis(iommu);
 			iommu_set_device_table(iommu);
-			iommu_flush_all_caches(iommu);
+			amd_iommu_flush_all_caches(iommu);
 		}
 	}
 }
@@ -3293,7 +3293,7 @@ static int __init state_next(void)
 				uninit_device_table_dma(pci_seg);
 
 			for_each_iommu(iommu)
-				iommu_flush_all_caches(iommu);
+				amd_iommu_flush_all_caches(iommu);
 		}
 	}
 	return ret;
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 45b74dfedce9..fd4154c2e770 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1390,7 +1390,7 @@ static void amd_iommu_flush_irt_all(struct amd_iommu *iommu)
 	iommu_completion_wait(iommu);
 }
 
-void iommu_flush_all_caches(struct amd_iommu *iommu)
+void amd_iommu_flush_all_caches(struct amd_iommu *iommu)
 {
 	if (check_feature(FEATURE_IA)) {
 		amd_iommu_flush_all(iommu);
-- 
2.31.1

Re: [PATCH v1 01/13] iommu/amd: Rename iommu_flush_all_caches() -> amd_iommu_flush_all_caches()

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 10:16:12AM +0000, Vasant Hegde wrote:
> Rename function inline with driver naming convention.
> 
> No functional changes.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu.h       | 5 +++++
>  drivers/iommu/amd/amd_iommu_types.h | 6 ------
>  drivers/iommu/amd/init.c            | 8 ++++----
>  drivers/iommu/amd/iommu.c           | 2 +-
>  4 files changed, 10 insertions(+), 11 deletions(-)

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

Jason

[PATCH v1 02/13] iommu/amd: Remove redundant domain flush from attach_device()

[Vasant Hegde]

Domain flush was introduced in attach_device() path to handle kdump
scenario. Later init code was enhanced to handle kdump scenario where
it also takes care of flushing everything including TLB
(see early_enable_iommus()).

Hence remove redundant flush from attach_device() function.

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

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index fd4154c2e770..bbbb7204b635 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1891,15 +1891,6 @@ static int attach_device(struct device *dev,
 
 	do_attach(dev_data, domain);
 
-	/*
-	 * We might boot into a crash-kernel here. The crashed kernel
-	 * left the caches in the IOMMU dirty. So we have to flush
-	 * here to evict all dirty stuff.
-	 */
-	amd_iommu_domain_flush_tlb_pde(domain);
-
-	amd_iommu_domain_flush_complete(domain);
-
 out:
 	spin_unlock(&dev_data->lock);
 
-- 
2.31.1

Re: [PATCH v1 02/13] iommu/amd: Remove redundant domain flush from attach_device()

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 10:16:13AM +0000, Vasant Hegde wrote:
> Domain flush was introduced in attach_device() path to handle kdump
> scenario. Later init code was enhanced to handle kdump scenario where
> it also takes care of flushing everything including TLB
> (see early_enable_iommus()).

Yeah, this is the right thing to do the caches should be cleaned
when the driver starts up.

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

Jason

[PATCH v1 03/13] iommu/amd: Remove redundant passing of PDE bit

[Vasant Hegde]

Current code always sets PDE bit in INVALIDATE_IOMMU_PAGES command.
Hence get rid of 'pde' variable across functions.

We can re-introduce this bit whenever its needed.

Suggested-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index bbbb7204b635..94c230bd3fba 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1122,7 +1122,7 @@ static inline u64 build_inv_address(u64 address, size_t size)
 }
 
 static void build_inv_iommu_pages(struct iommu_cmd *cmd, u64 address,
-				  size_t size, u16 domid, int pde)
+				  size_t size, u16 domid)
 {
 	u64 inv_address = build_inv_address(address, size);
 
@@ -1131,8 +1131,8 @@ static void build_inv_iommu_pages(struct iommu_cmd *cmd, u64 address,
 	cmd->data[2]  = lower_32_bits(inv_address);
 	cmd->data[3]  = upper_32_bits(inv_address);
 	CMD_SET_TYPE(cmd, CMD_INV_IOMMU_PAGES);
-	if (pde) /* PDE bit - we want to flush everything, not only the PTEs */
-		cmd->data[2] |= CMD_INV_IOMMU_PAGES_PDE_MASK;
+	/* PDE bit - we want to flush everything, not only the PTEs */
+	cmd->data[2] |= CMD_INV_IOMMU_PAGES_PDE_MASK;
 }
 
 static void build_inv_iotlb_pages(struct iommu_cmd *cmd, u16 devid, int qdep,
@@ -1339,7 +1339,7 @@ static void amd_iommu_flush_tlb_all(struct amd_iommu *iommu)
 	for (dom_id = 0; dom_id <= last_bdf; ++dom_id) {
 		struct iommu_cmd cmd;
 		build_inv_iommu_pages(&cmd, 0, CMD_INV_IOMMU_ALL_PAGES_ADDRESS,
-				      dom_id, 1);
+				      dom_id);
 		iommu_queue_command(iommu, &cmd);
 	}
 
@@ -1350,8 +1350,7 @@ static void amd_iommu_flush_tlb_domid(struct amd_iommu *iommu, u32 dom_id)
 {
 	struct iommu_cmd cmd;
 
-	build_inv_iommu_pages(&cmd, 0, CMD_INV_IOMMU_ALL_PAGES_ADDRESS,
-			      dom_id, 1);
+	build_inv_iommu_pages(&cmd, 0, CMD_INV_IOMMU_ALL_PAGES_ADDRESS, dom_id);
 	iommu_queue_command(iommu, &cmd);
 
 	iommu_completion_wait(iommu);
@@ -1474,13 +1473,13 @@ static int device_flush_dte(struct iommu_dev_data *dev_data)
  * page. Otherwise it flushes the whole TLB of the IOMMU.
  */
 static void __domain_flush_pages(struct protection_domain *domain,
-				 u64 address, size_t size, int pde)
+				 u64 address, size_t size)
 {
 	struct iommu_dev_data *dev_data;
 	struct iommu_cmd cmd;
 	int ret = 0, i;
 
-	build_inv_iommu_pages(&cmd, address, size, domain->id, pde);
+	build_inv_iommu_pages(&cmd, address, size, domain->id);
 
 	for (i = 0; i < amd_iommu_get_num_iommus(); ++i) {
 		if (!domain->dev_iommu[i])
@@ -1505,10 +1504,10 @@ static void __domain_flush_pages(struct protection_domain *domain,
 }
 
 static void domain_flush_pages(struct protection_domain *domain,
-			       u64 address, size_t size, int pde)
+			       u64 address, size_t size)
 {
 	if (likely(!amd_iommu_np_cache)) {
-		__domain_flush_pages(domain, address, size, pde);
+		__domain_flush_pages(domain, address, size);
 		return;
 	}
 
@@ -1541,7 +1540,7 @@ static void domain_flush_pages(struct protection_domain *domain,
 
 		flush_size = 1ul << min_alignment;
 
-		__domain_flush_pages(domain, address, flush_size, pde);
+		__domain_flush_pages(domain, address, flush_size);
 		address += flush_size;
 		size -= flush_size;
 	}
@@ -1550,7 +1549,7 @@ static void domain_flush_pages(struct protection_domain *domain,
 /* Flush the whole IO/TLB for a given protection domain - including PDE */
 void amd_iommu_domain_flush_tlb_pde(struct protection_domain *domain)
 {
-	domain_flush_pages(domain, 0, CMD_INV_IOMMU_ALL_PAGES_ADDRESS, 1);
+	domain_flush_pages(domain, 0, CMD_INV_IOMMU_ALL_PAGES_ADDRESS);
 }
 
 void amd_iommu_domain_flush_complete(struct protection_domain *domain)
@@ -1577,7 +1576,7 @@ static void domain_flush_np_cache(struct protection_domain *domain,
 		unsigned long flags;
 
 		spin_lock_irqsave(&domain->lock, flags);
-		domain_flush_pages(domain, iova, size, 1);
+		domain_flush_pages(domain, iova, size);
 		amd_iommu_domain_flush_complete(domain);
 		spin_unlock_irqrestore(&domain->lock, flags);
 	}
@@ -2457,7 +2456,7 @@ static void amd_iommu_iotlb_sync(struct iommu_domain *domain,
 	unsigned long flags;
 
 	spin_lock_irqsave(&dom->lock, flags);
-	domain_flush_pages(dom, gather->start, gather->end - gather->start + 1, 1);
+	domain_flush_pages(dom, gather->start, gather->end - gather->start + 1);
 	amd_iommu_domain_flush_complete(dom);
 	spin_unlock_irqrestore(&dom->lock, flags);
 }
-- 
2.31.1

Re: [PATCH v1 03/13] iommu/amd: Remove redundant passing of PDE bit

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 10:16:14AM +0000, Vasant Hegde wrote:
> Current code always sets PDE bit in INVALIDATE_IOMMU_PAGES command.
> Hence get rid of 'pde' variable across functions.
> 
> We can re-introduce this bit whenever its needed.
> 
> Suggested-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)

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

'bool pde' if you do decide to bring it back

Jason

[PATCH v1 04/13] iommu/amd: Add support to invalidate multiple guest pages

[Vasant Hegde]

Current interface supports invalidating single page or entire guest
translation information for a single process address space.

IOMMU CMD_INV_IOMMU_PAGES and CMD_INV_IOTLB_PAGES commands supports
invalidating range of pages. Add support to invalidate multiple pages.

This is preparatory patch before consolidating host and guest
invalidation code into single function. Following patches will
consolidation tlb invalidation code.

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

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 94c230bd3fba..ee81ce65d25e 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1150,40 +1150,36 @@ static void build_inv_iotlb_pages(struct iommu_cmd *cmd, u16 devid, int qdep,
 }
 
 static void build_inv_iommu_pasid(struct iommu_cmd *cmd, u16 domid, u32 pasid,
-				  u64 address, bool size)
+				  u64 address, size_t size)
 {
-	memset(cmd, 0, sizeof(*cmd));
+	u64 inv_address = build_inv_address(address, size);
 
-	address &= ~(0xfffULL);
+	memset(cmd, 0, sizeof(*cmd));
 
 	cmd->data[0]  = pasid;
 	cmd->data[1]  = domid;
-	cmd->data[2]  = lower_32_bits(address);
-	cmd->data[3]  = upper_32_bits(address);
+	cmd->data[2]  = lower_32_bits(inv_address);
+	cmd->data[3]  = upper_32_bits(inv_address);
 	cmd->data[2] |= CMD_INV_IOMMU_PAGES_PDE_MASK;
 	cmd->data[2] |= CMD_INV_IOMMU_PAGES_GN_MASK;
-	if (size)
-		cmd->data[2] |= CMD_INV_IOMMU_PAGES_SIZE_MASK;
 	CMD_SET_TYPE(cmd, CMD_INV_IOMMU_PAGES);
 }
 
 static void build_inv_iotlb_pasid(struct iommu_cmd *cmd, u16 devid, u32 pasid,
-				  int qdep, u64 address, bool size)
+				  int qdep, u64 address, size_t size)
 {
-	memset(cmd, 0, sizeof(*cmd));
+	u64 inv_address = build_inv_address(address, size);
 
-	address &= ~(0xfffULL);
+	memset(cmd, 0, sizeof(*cmd));
 
 	cmd->data[0]  = devid;
 	cmd->data[0] |= ((pasid >> 8) & 0xff) << 16;
 	cmd->data[0] |= (qdep  & 0xff) << 24;
 	cmd->data[1]  = devid;
 	cmd->data[1] |= (pasid & 0xff) << 16;
-	cmd->data[2]  = lower_32_bits(address);
+	cmd->data[2]  = lower_32_bits(inv_address);
 	cmd->data[2] |= CMD_INV_IOMMU_PAGES_GN_MASK;
-	cmd->data[3]  = upper_32_bits(address);
-	if (size)
-		cmd->data[2] |= CMD_INV_IOMMU_PAGES_SIZE_MASK;
+	cmd->data[3]  = upper_32_bits(inv_address);
 	CMD_SET_TYPE(cmd, CMD_INV_IOTLB_PAGES);
 }
 
@@ -2516,7 +2512,7 @@ const struct iommu_ops amd_iommu_ops = {
 };
 
 static int __flush_pasid(struct protection_domain *domain, u32 pasid,
-			 u64 address, bool size)
+			 u64 address, size_t size)
 {
 	struct iommu_dev_data *dev_data;
 	struct iommu_cmd cmd;
@@ -2580,7 +2576,7 @@ static int __flush_pasid(struct protection_domain *domain, u32 pasid,
 static int __amd_iommu_flush_page(struct protection_domain *domain, u32 pasid,
 				  u64 address)
 {
-	return __flush_pasid(domain, pasid, address, false);
+	return __flush_pasid(domain, pasid, address, PAGE_SIZE);
 }
 
 int amd_iommu_flush_page(struct iommu_domain *dom, u32 pasid,
@@ -2599,8 +2595,7 @@ int amd_iommu_flush_page(struct iommu_domain *dom, u32 pasid,
 
 static int __amd_iommu_flush_tlb(struct protection_domain *domain, u32 pasid)
 {
-	return __flush_pasid(domain, pasid, CMD_INV_IOMMU_ALL_PAGES_ADDRESS,
-			     true);
+	return __flush_pasid(domain, pasid, 0, CMD_INV_IOMMU_ALL_PAGES_ADDRESS);
 }
 
 int amd_iommu_flush_tlb(struct iommu_domain *dom, u32 pasid)
-- 
2.31.1

Re: [PATCH v1 04/13] iommu/amd: Add support to invalidate multiple guest pages

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 10:16:15AM +0000, Vasant Hegde wrote:
> @@ -2580,7 +2576,7 @@ static int __flush_pasid(struct protection_domain *domain, u32 pasid,
>  static int __amd_iommu_flush_page(struct protection_domain *domain, u32 pasid,
>  				  u64 address)
>  {
> -	return __flush_pasid(domain, pasid, address, false);
> +	return __flush_pasid(domain, pasid, address, PAGE_SIZE);
>  }

The idea of passing the size to flush seems fine, but an API to
invalidate a single page is really strange, we never have that
workflow in the iommu drivers.

The only caller is iommu_v2_map_pages and it has a length > PAGE_SIZE
available.

But the other parts are fine

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

Jason

[PATCH v1 05/13] iommu/amd: Refactor IOMMU tlb invalidation code

[Vasant Hegde]

build_inv_iommu_pages() and build_inv_iommu_pasid() pretty much
duplicates the code. Hence enhance build_inv_iommu_pages() to
invalidate guest pages as well. And remove build_inv_iommu_pasid().

Suggested-by: Kishon Vijay Abraham I <kvijayab@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu.c | 36 ++++++++++++++----------------------
 1 file changed, 14 insertions(+), 22 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index ee81ce65d25e..aa1bd45f18b9 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1122,17 +1122,23 @@ static inline u64 build_inv_address(u64 address, size_t size)
 }
 
 static void build_inv_iommu_pages(struct iommu_cmd *cmd, u64 address,
-				  size_t size, u16 domid)
+				  size_t size, u16 domid,
+				  ioasid_t pasid, bool gn)
 {
 	u64 inv_address = build_inv_address(address, size);
 
 	memset(cmd, 0, sizeof(*cmd));
+
 	cmd->data[1] |= domid;
 	cmd->data[2]  = lower_32_bits(inv_address);
 	cmd->data[3]  = upper_32_bits(inv_address);
-	CMD_SET_TYPE(cmd, CMD_INV_IOMMU_PAGES);
 	/* PDE bit - we want to flush everything, not only the PTEs */
 	cmd->data[2] |= CMD_INV_IOMMU_PAGES_PDE_MASK;
+	if (gn) {
+		cmd->data[0] |= pasid;
+		cmd->data[2] |= CMD_INV_IOMMU_PAGES_GN_MASK;
+	}
+	CMD_SET_TYPE(cmd, CMD_INV_IOMMU_PAGES);
 }
 
 static void build_inv_iotlb_pages(struct iommu_cmd *cmd, u16 devid, int qdep,
@@ -1149,22 +1155,6 @@ static void build_inv_iotlb_pages(struct iommu_cmd *cmd, u16 devid, int qdep,
 	CMD_SET_TYPE(cmd, CMD_INV_IOTLB_PAGES);
 }
 
-static void build_inv_iommu_pasid(struct iommu_cmd *cmd, u16 domid, u32 pasid,
-				  u64 address, size_t size)
-{
-	u64 inv_address = build_inv_address(address, size);
-
-	memset(cmd, 0, sizeof(*cmd));
-
-	cmd->data[0]  = pasid;
-	cmd->data[1]  = domid;
-	cmd->data[2]  = lower_32_bits(inv_address);
-	cmd->data[3]  = upper_32_bits(inv_address);
-	cmd->data[2] |= CMD_INV_IOMMU_PAGES_PDE_MASK;
-	cmd->data[2] |= CMD_INV_IOMMU_PAGES_GN_MASK;
-	CMD_SET_TYPE(cmd, CMD_INV_IOMMU_PAGES);
-}
-
 static void build_inv_iotlb_pasid(struct iommu_cmd *cmd, u16 devid, u32 pasid,
 				  int qdep, u64 address, size_t size)
 {
@@ -1335,7 +1325,7 @@ static void amd_iommu_flush_tlb_all(struct amd_iommu *iommu)
 	for (dom_id = 0; dom_id <= last_bdf; ++dom_id) {
 		struct iommu_cmd cmd;
 		build_inv_iommu_pages(&cmd, 0, CMD_INV_IOMMU_ALL_PAGES_ADDRESS,
-				      dom_id);
+				      dom_id, IOMMU_NO_PASID, false);
 		iommu_queue_command(iommu, &cmd);
 	}
 
@@ -1346,7 +1336,8 @@ static void amd_iommu_flush_tlb_domid(struct amd_iommu *iommu, u32 dom_id)
 {
 	struct iommu_cmd cmd;
 
-	build_inv_iommu_pages(&cmd, 0, CMD_INV_IOMMU_ALL_PAGES_ADDRESS, dom_id);
+	build_inv_iommu_pages(&cmd, 0, CMD_INV_IOMMU_ALL_PAGES_ADDRESS,
+			      dom_id, IOMMU_NO_PASID, false);
 	iommu_queue_command(iommu, &cmd);
 
 	iommu_completion_wait(iommu);
@@ -1475,7 +1466,8 @@ static void __domain_flush_pages(struct protection_domain *domain,
 	struct iommu_cmd cmd;
 	int ret = 0, i;
 
-	build_inv_iommu_pages(&cmd, address, size, domain->id);
+	build_inv_iommu_pages(&cmd, address, size, domain->id,
+			      IOMMU_NO_PASID, false);
 
 	for (i = 0; i < amd_iommu_get_num_iommus(); ++i) {
 		if (!domain->dev_iommu[i])
@@ -2521,7 +2513,7 @@ static int __flush_pasid(struct protection_domain *domain, u32 pasid,
 	if (!(domain->flags & PD_IOMMUV2_MASK))
 		return -EINVAL;
 
-	build_inv_iommu_pasid(&cmd, domain->id, pasid, address, size);
+	build_inv_iommu_pages(&cmd, address, size, domain->id, pasid, true);
 
 	/*
 	 * IOMMU TLB needs to be flushed before Device TLB to
-- 
2.31.1

Re: [PATCH v1 05/13] iommu/amd: Refactor IOMMU tlb invalidation code

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 10:16:16AM +0000, Vasant Hegde wrote:
> build_inv_iommu_pages() and build_inv_iommu_pasid() pretty much
> duplicates the code. Hence enhance build_inv_iommu_pages() to
> invalidate guest pages as well. And remove build_inv_iommu_pasid().
> 
> Suggested-by: Kishon Vijay Abraham I <kvijayab@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 36 ++++++++++++++----------------------
>  1 file changed, 14 insertions(+), 22 deletions(-)

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

Jason

[PATCH v1 06/13] iommu/amd: Refactor device iotlb invalidation code

[Vasant Hegde]

build_inv_iotlb_pages() and build_inv_iotlb_pasid() pretty much duplicates
the code. Enhance build_inv_iotlb_pages() to invalidate guest IOTLB as
well. And remove build_inv_iotlb_pasid() function.

Suggested-by: Kishon Vijay Abraham I <kvijayab@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index aa1bd45f18b9..2e7360fbc3c4 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1142,34 +1142,24 @@ static void build_inv_iommu_pages(struct iommu_cmd *cmd, u64 address,
 }
 
 static void build_inv_iotlb_pages(struct iommu_cmd *cmd, u16 devid, int qdep,
-				  u64 address, size_t size)
+				  u64 address, size_t size,
+				  ioasid_t pasid, bool gn)
 {
 	u64 inv_address = build_inv_address(address, size);
 
 	memset(cmd, 0, sizeof(*cmd));
+
 	cmd->data[0]  = devid;
 	cmd->data[0] |= (qdep & 0xff) << 24;
 	cmd->data[1]  = devid;
 	cmd->data[2]  = lower_32_bits(inv_address);
 	cmd->data[3]  = upper_32_bits(inv_address);
-	CMD_SET_TYPE(cmd, CMD_INV_IOTLB_PAGES);
-}
-
-static void build_inv_iotlb_pasid(struct iommu_cmd *cmd, u16 devid, u32 pasid,
-				  int qdep, u64 address, size_t size)
-{
-	u64 inv_address = build_inv_address(address, size);
-
-	memset(cmd, 0, sizeof(*cmd));
+	if (gn) {
+		cmd->data[0] |= ((pasid >> 8) & 0xff) << 16;
+		cmd->data[1] |= (pasid & 0xff) << 16;
+		cmd->data[2] |= CMD_INV_IOMMU_PAGES_GN_MASK;
+	}
 
-	cmd->data[0]  = devid;
-	cmd->data[0] |= ((pasid >> 8) & 0xff) << 16;
-	cmd->data[0] |= (qdep  & 0xff) << 24;
-	cmd->data[1]  = devid;
-	cmd->data[1] |= (pasid & 0xff) << 16;
-	cmd->data[2]  = lower_32_bits(inv_address);
-	cmd->data[2] |= CMD_INV_IOMMU_PAGES_GN_MASK;
-	cmd->data[3]  = upper_32_bits(inv_address);
 	CMD_SET_TYPE(cmd, CMD_INV_IOTLB_PAGES);
 }
 
@@ -1402,7 +1392,8 @@ static int device_flush_iotlb(struct iommu_dev_data *dev_data,
 	if (!iommu)
 		return -EINVAL;
 
-	build_inv_iotlb_pages(&cmd, dev_data->devid, qdep, address, size);
+	build_inv_iotlb_pages(&cmd, dev_data->devid, qdep, address,
+			      size, IOMMU_NO_PASID, false);
 
 	return iommu_queue_command(iommu, &cmd);
 }
@@ -2547,8 +2538,8 @@ static int __flush_pasid(struct protection_domain *domain, u32 pasid,
 		iommu = rlookup_amd_iommu(dev_data->dev);
 		if (!iommu)
 			continue;
-		build_inv_iotlb_pasid(&cmd, dev_data->devid, pasid,
-				      qdep, address, size);
+		build_inv_iotlb_pages(&cmd, dev_data->devid, qdep,
+				      address, size, pasid, true);
 
 		ret = iommu_queue_command(iommu, &cmd);
 		if (ret != 0)
-- 
2.31.1

Re: [PATCH v1 06/13] iommu/amd: Refactor device iotlb invalidation code

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 10:16:17AM +0000, Vasant Hegde wrote:
> build_inv_iotlb_pages() and build_inv_iotlb_pasid() pretty much duplicates
> the code. Enhance build_inv_iotlb_pages() to invalidate guest IOTLB as
> well. And remove build_inv_iotlb_pasid() function.
> 
> Suggested-by: Kishon Vijay Abraham I <kvijayab@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 33 ++++++++++++---------------------
>  1 file changed, 12 insertions(+), 21 deletions(-)

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

Jason

[PATCH v1 07/13] iommu/amd: Consolidate device IOTLB flush code

[Vasant Hegde]

Currently we have separate code path to flush device IOTLB for host and
guest page table (PASID table). Consolidate device IOTLB flush code in
device_flush_iotlb_range().

Introduce helper function to find page table mode, decide whether to
flush host/guest page table.

Finally rename device_flush_iotlb() -> device_flush_iotlb_range().

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

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 2e7360fbc3c4..9a7f0f571862 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -83,6 +83,22 @@ static void detach_device(struct device *dev);
  *
  ****************************************************************************/
 
+/*
+ * For invalidation request without PASID, get the pasid based on
+ * domain page table mode.
+ */
+static inline ioasid_t pdom_get_default_pasid(struct protection_domain *pdom)
+{
+	return (pdom && (pdom->flags & PD_IOMMUV2_MASK)) ?
+		IOMMU_NO_PASID : IOMMU_PASID_INVALID;
+}
+
+/* Used to select host/guest page table for invalidation */
+static inline bool is_pasid_valid(ioasid_t pasid)
+{
+	return (pasid != IOMMU_PASID_INVALID) ? true : false;
+}
+
 static inline int get_acpihid_device_id(struct device *dev,
 					struct acpihid_map_entry **entry)
 {
@@ -1380,12 +1396,13 @@ void amd_iommu_flush_all_caches(struct amd_iommu *iommu)
 /*
  * Command send function for flushing on-device TLB
  */
-static int device_flush_iotlb(struct iommu_dev_data *dev_data,
-			      u64 address, size_t size)
+static int device_flush_iotlb_range(struct iommu_dev_data *dev_data,
+				    ioasid_t pasid, u64 address, size_t size)
 {
 	struct amd_iommu *iommu;
 	struct iommu_cmd cmd;
 	int qdep;
+	bool gn = is_pasid_valid(pasid);
 
 	qdep     = dev_data->ats_qdep;
 	iommu    = rlookup_amd_iommu(dev_data->dev);
@@ -1393,7 +1410,7 @@ static int device_flush_iotlb(struct iommu_dev_data *dev_data,
 		return -EINVAL;
 
 	build_inv_iotlb_pages(&cmd, dev_data->devid, qdep, address,
-			      size, IOMMU_NO_PASID, false);
+			      size, pasid, gn);
 
 	return iommu_queue_command(iommu, &cmd);
 }
@@ -1439,8 +1456,11 @@ static int device_flush_dte(struct iommu_dev_data *dev_data)
 			return ret;
 	}
 
-	if (dev_data->ats_enabled)
-		ret = device_flush_iotlb(dev_data, 0, ~0UL);
+	if (dev_data->ats_enabled) {
+		ioasid_t pasid = pdom_get_default_pasid(dev_data->domain);
+
+		ret = device_flush_iotlb_range(dev_data, pasid, 0, ~0UL);
+	}
 
 	return ret;
 }
@@ -1476,7 +1496,8 @@ static void __domain_flush_pages(struct protection_domain *domain,
 		if (!dev_data->ats_enabled)
 			continue;
 
-		ret |= device_flush_iotlb(dev_data, address, size);
+		ret |= device_flush_iotlb_range(dev_data, IOMMU_PASID_INVALID,
+						address, size);
 	}
 
 	WARN_ON(ret);
-- 
2.31.1

Re: [PATCH v1 07/13] iommu/amd: Consolidate device IOTLB flush code

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 10:16:18AM +0000, Vasant Hegde wrote:

> @@ -1380,12 +1396,13 @@ void amd_iommu_flush_all_caches(struct amd_iommu *iommu)
>  /*
>   * Command send function for flushing on-device TLB
>   */
> -static int device_flush_iotlb(struct iommu_dev_data *dev_data,
> -			      u64 address, size_t size)
> +static int device_flush_iotlb_range(struct iommu_dev_data *dev_data,
> +				    ioasid_t pasid, u64 address, size_t size)
>  {
>  	struct amd_iommu *iommu;
>  	struct iommu_cmd cmd;
>  	int qdep;
> +	bool gn = is_pasid_valid(pasid);

I've read this a few times, and this just looks really ugly..

Something has gone off if you have to encode the type of invalidation
to do inside the PASID.

I don't have a good advice for you, but I suspect this is a
consequence of not having robust datastructures. There is not a single
handle you can use here to refer to the thing that needs to be
invalidated.

I'm not sure unifying the v1/v2 flows is actually an improvement in
clarity. In all cases the caller should already know if it is a v1/v2
object, calling a specific function is not such a burden.

Regardless, don't let this remark discrouage Joerg from merging it,
just something to think about.

Jason

Re: [PATCH v1 07/13] iommu/amd: Consolidate device IOTLB flush code

[Vasant Hegde]

On 11/4/2023 12:14 AM, Jason Gunthorpe wrote:
> On Fri, Oct 06, 2023 at 10:16:18AM +0000, Vasant Hegde wrote:
> 
>> @@ -1380,12 +1396,13 @@ void amd_iommu_flush_all_caches(struct amd_iommu *iommu)
>>  /*
>>   * Command send function for flushing on-device TLB
>>   */
>> -static int device_flush_iotlb(struct iommu_dev_data *dev_data,
>> -			      u64 address, size_t size)
>> +static int device_flush_iotlb_range(struct iommu_dev_data *dev_data,
>> +				    ioasid_t pasid, u64 address, size_t size)
>>  {
>>  	struct amd_iommu *iommu;
>>  	struct iommu_cmd cmd;
>>  	int qdep;
>> +	bool gn = is_pasid_valid(pasid);
> 
> I've read this a few times, and this just looks really ugly..
> 
> Something has gone off if you have to encode the type of invalidation
> to do inside the PASID.

I wanted to push all these checks to low level functions rather than caller side
adding checks. Also I wanted to avoid passing protection domain IOTLB flush
functions.

> 
> I don't have a good advice for you, but I suspect this is a
> consequence of not having robust datastructures. There is not a single
> handle you can use here to refer to the thing that needs to be
> invalidated.
> 
> I'm not sure unifying the v1/v2 flows is actually an improvement in
> clarity. In all cases the caller should already know if it is a v1/v2
> object, calling a specific function is not such a burden.

Yeah. I can introduce few more functions and few extra checks in caller side if
that makes it easy.

-Vasant

[PATCH v1 08/13] iommu/amd: Consolidate amd_iommu_domain_flush_complete() call

[Vasant Hegde]

Call amd_iommu_domain_flush_complete() from domain_flush_pages().
That way we can remove explicit call of amd_iommu_domain_flush_complete()
from various places.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/io_pgtable.c |  1 -
 drivers/iommu/amd/iommu.c      | 16 ++++++++--------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/amd/io_pgtable.c b/drivers/iommu/amd/io_pgtable.c
index 2892aa1b4dc1..2f072c434fd9 100644
--- a/drivers/iommu/amd/io_pgtable.c
+++ b/drivers/iommu/amd/io_pgtable.c
@@ -425,7 +425,6 @@ static int iommu_v1_map_pages(struct io_pgtable_ops *ops, unsigned long iova,
 		 * increase_address_space().
 		 */
 		amd_iommu_domain_flush_tlb_pde(dom);
-		amd_iommu_domain_flush_complete(dom);
 		spin_unlock_irqrestore(&dom->lock, flags);
 	}
 
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 9a7f0f571862..1f695dd50fec 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1508,6 +1508,10 @@ static void domain_flush_pages(struct protection_domain *domain,
 {
 	if (likely(!amd_iommu_np_cache)) {
 		__domain_flush_pages(domain, address, size);
+
+		/* Wait until IOMMU TLB and all device IOTLB flushes are complete */
+		amd_iommu_domain_flush_complete(domain);
+
 		return;
 	}
 
@@ -1544,6 +1548,9 @@ static void domain_flush_pages(struct protection_domain *domain,
 		address += flush_size;
 		size -= flush_size;
 	}
+
+	/* Wait until IOMMU TLB and all device IOTLB flushes are complete */
+	amd_iommu_domain_flush_complete(domain);
 }
 
 /* Flush the whole IO/TLB for a given protection domain - including PDE */
@@ -1577,7 +1584,6 @@ static void domain_flush_np_cache(struct protection_domain *domain,
 
 		spin_lock_irqsave(&domain->lock, flags);
 		domain_flush_pages(domain, iova, size);
-		amd_iommu_domain_flush_complete(domain);
 		spin_unlock_irqrestore(&domain->lock, flags);
 	}
 }
@@ -1852,12 +1858,9 @@ static void do_detach(struct iommu_dev_data *dev_data)
 	/* Flush the DTE entry */
 	device_flush_dte(dev_data);
 
-	/* Flush IOTLB */
+	/* Flush IOTLB and wait for the flushes to finish */
 	amd_iommu_domain_flush_tlb_pde(domain);
 
-	/* Wait for the flushes to finish */
-	amd_iommu_domain_flush_complete(domain);
-
 	/* decrease reference counters - needs to happen after the flushes */
 	domain->dev_iommu[iommu->index] -= 1;
 	domain->dev_cnt                 -= 1;
@@ -2034,7 +2037,6 @@ void amd_iommu_domain_update(struct protection_domain *domain)
 
 	/* Flush domain TLB(s) and wait for completion */
 	amd_iommu_domain_flush_tlb_pde(domain);
-	amd_iommu_domain_flush_complete(domain);
 }
 
 /*****************************************************************************
@@ -2445,7 +2447,6 @@ static void amd_iommu_flush_iotlb_all(struct iommu_domain *domain)
 
 	spin_lock_irqsave(&dom->lock, flags);
 	amd_iommu_domain_flush_tlb_pde(dom);
-	amd_iommu_domain_flush_complete(dom);
 	spin_unlock_irqrestore(&dom->lock, flags);
 }
 
@@ -2457,7 +2458,6 @@ static void amd_iommu_iotlb_sync(struct iommu_domain *domain,
 
 	spin_lock_irqsave(&dom->lock, flags);
 	domain_flush_pages(dom, gather->start, gather->end - gather->start + 1);
-	amd_iommu_domain_flush_complete(dom);
 	spin_unlock_irqrestore(&dom->lock, flags);
 }
 
-- 
2.31.1

Re: [PATCH v1 08/13] iommu/amd: Consolidate amd_iommu_domain_flush_complete() call

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 10:16:19AM +0000, Vasant Hegde wrote:
> Call amd_iommu_domain_flush_complete() from domain_flush_pages().
> That way we can remove explicit call of amd_iommu_domain_flush_complete()
> from various places.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/io_pgtable.c |  1 -
>  drivers/iommu/amd/iommu.c      | 16 ++++++++--------
>  2 files changed, 8 insertions(+), 9 deletions(-)

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

Jason

[PATCH v1 09/13] iommu/amd: Refactor domain flush global function

[Vasant Hegde]

- Add new function (amd_iommu_domain_flush_pages()) to invalidate range
  of pages. Current patch adds support to flush host page table.
  Following patch will enhance this function to flush guest page table
  with default PASID. So that we can use same function to flush v2 page
  table as well.

- Convert v1 page table (io_pgtble.c) to use newly introduced functions.
  So that it invalidates updated page ranges only instead of invalidating
  everything.

- Rename amd_iommu_domain_flush_tlb_pde() -> amd_iommu_domain_flush_all().
  It will reflect its usage as PDE is not passed explicitly. Also make it
  static as its used inside iommu.c only.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu.h  |  4 +++-
 drivers/iommu/amd/io_pgtable.c |  4 +++-
 drivers/iommu/amd/iommu.c      | 23 ++++++++++++++++-------
 3 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 234db57cd320..088890f9618a 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -52,6 +52,7 @@ 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);
 
+/* TLB flush */
 int amd_iommu_flush_page(struct iommu_domain *dom, u32 pasid, u64 address);
 /*
  * This function flushes all internal caches of
@@ -61,7 +62,8 @@ void amd_iommu_flush_all_caches(struct amd_iommu *iommu);
 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);
+void amd_iommu_domain_flush_pages(struct protection_domain *pdom,
+				  u64 address, size_t size);
 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/io_pgtable.c b/drivers/iommu/amd/io_pgtable.c
index 2f072c434fd9..35bf829d1fb4 100644
--- a/drivers/iommu/amd/io_pgtable.c
+++ b/drivers/iommu/amd/io_pgtable.c
@@ -369,6 +369,8 @@ static int iommu_v1_map_pages(struct io_pgtable_ops *ops, unsigned long iova,
 	bool updated = false;
 	u64 __pte, *pte;
 	int ret, i, count;
+	size_t size = pgcount << __ffs(pgsize);
+	unsigned long o_iova = iova;
 
 	BUG_ON(!IS_ALIGNED(iova, pgsize));
 	BUG_ON(!IS_ALIGNED(paddr, pgsize));
@@ -424,7 +426,7 @@ static int iommu_v1_map_pages(struct io_pgtable_ops *ops, unsigned long iova,
 		 * Updates and flushing already happened in
 		 * increase_address_space().
 		 */
-		amd_iommu_domain_flush_tlb_pde(dom);
+		amd_iommu_domain_flush_pages(dom, o_iova, size);
 		spin_unlock_irqrestore(&dom->lock, flags);
 	}
 
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 1f695dd50fec..c30b08e2a939 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1553,10 +1553,18 @@ static void domain_flush_pages(struct protection_domain *domain,
 	amd_iommu_domain_flush_complete(domain);
 }
 
+/* Flush range of IO/TLB for a given protection domain */
+void amd_iommu_domain_flush_pages(struct protection_domain *pdom,
+				  u64 address, size_t size)
+{
+	return domain_flush_pages(pdom, address, size);
+}
+
 /* Flush the whole IO/TLB for a given protection domain - including PDE */
-void amd_iommu_domain_flush_tlb_pde(struct protection_domain *domain)
+static void amd_iommu_domain_flush_all(struct protection_domain *pdom)
 {
-	domain_flush_pages(domain, 0, CMD_INV_IOMMU_ALL_PAGES_ADDRESS);
+	return amd_iommu_domain_flush_pages(pdom,  0,
+					    CMD_INV_IOMMU_ALL_PAGES_ADDRESS);
 }
 
 void amd_iommu_domain_flush_complete(struct protection_domain *domain)
@@ -1583,7 +1591,7 @@ static void domain_flush_np_cache(struct protection_domain *domain,
 		unsigned long flags;
 
 		spin_lock_irqsave(&domain->lock, flags);
-		domain_flush_pages(domain, iova, size);
+		amd_iommu_domain_flush_pages(domain, iova, size);
 		spin_unlock_irqrestore(&domain->lock, flags);
 	}
 }
@@ -1859,7 +1867,7 @@ static void do_detach(struct iommu_dev_data *dev_data)
 	device_flush_dte(dev_data);
 
 	/* Flush IOTLB and wait for the flushes to finish */
-	amd_iommu_domain_flush_tlb_pde(domain);
+	amd_iommu_domain_flush_all(domain);
 
 	/* decrease reference counters - needs to happen after the flushes */
 	domain->dev_iommu[iommu->index] -= 1;
@@ -2036,7 +2044,7 @@ void amd_iommu_domain_update(struct protection_domain *domain)
 	amd_iommu_update_and_flush_device_table(domain);
 
 	/* Flush domain TLB(s) and wait for completion */
-	amd_iommu_domain_flush_tlb_pde(domain);
+	amd_iommu_domain_flush_all(domain);
 }
 
 /*****************************************************************************
@@ -2446,7 +2454,7 @@ static void amd_iommu_flush_iotlb_all(struct iommu_domain *domain)
 	unsigned long flags;
 
 	spin_lock_irqsave(&dom->lock, flags);
-	amd_iommu_domain_flush_tlb_pde(dom);
+	amd_iommu_domain_flush_all(dom);
 	spin_unlock_irqrestore(&dom->lock, flags);
 }
 
@@ -2457,7 +2465,8 @@ static void amd_iommu_iotlb_sync(struct iommu_domain *domain,
 	unsigned long flags;
 
 	spin_lock_irqsave(&dom->lock, flags);
-	domain_flush_pages(dom, gather->start, gather->end - gather->start + 1);
+	amd_iommu_domain_flush_pages(dom, gather->start,
+				     gather->end - gather->start + 1);
 	spin_unlock_irqrestore(&dom->lock, flags);
 }
 
-- 
2.31.1

Re: [PATCH v1 09/13] iommu/amd: Refactor domain flush global function

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 10:16:20AM +0000, Vasant Hegde wrote:

> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 1f695dd50fec..c30b08e2a939 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -1553,10 +1553,18 @@ static void domain_flush_pages(struct protection_domain *domain,
>  	amd_iommu_domain_flush_complete(domain);
>  }
>  
> +/* Flush range of IO/TLB for a given protection domain */
> +void amd_iommu_domain_flush_pages(struct protection_domain *pdom,
> +				  u64 address, size_t size)
> +{
> +	return domain_flush_pages(pdom, address, size);
> +}

What is the point of having this maze of functions? Just rename
domain_flush_pages?

> @@ -1859,7 +1867,7 @@ static void do_detach(struct iommu_dev_data *dev_data)
>  	device_flush_dte(dev_data);
>  
>  	/* Flush IOTLB and wait for the flushes to finish */
> -	amd_iommu_domain_flush_tlb_pde(domain);
> +	amd_iommu_domain_flush_all(domain);

This is weird.. The cache tag for a domain shouldn't necessarily be
flushed from the iotlb when a domain is removed from a dte, should it?

ATC and IOTLB flushing should be seperate. The IOTLB should be flushed
immediately before some event that causes the cache tag to become
invalid, eg returning the tag to an allocator.

It is a minor point, but it does make things more understandable if
stuff is done in the right places.

Jason

Re: [PATCH v1 09/13] iommu/amd: Refactor domain flush global function

[Vasant Hegde]

On 11/5/2023 11:22 PM, Jason Gunthorpe wrote:
> On Fri, Oct 06, 2023 at 10:16:20AM +0000, Vasant Hegde wrote:
> 
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 1f695dd50fec..c30b08e2a939 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -1553,10 +1553,18 @@ static void domain_flush_pages(struct protection_domain *domain,
>>  	amd_iommu_domain_flush_complete(domain);
>>  }
>>  
>> +/* Flush range of IO/TLB for a given protection domain */
>> +void amd_iommu_domain_flush_pages(struct protection_domain *pdom,
>> +				  u64 address, size_t size)
>> +{
>> +	return domain_flush_pages(pdom, address, size);
>> +}
> 
> What is the point of having this maze of functions? Just rename
> domain_flush_pages?

Later patch adds PASID check to this function.

> 
>> @@ -1859,7 +1867,7 @@ static void do_detach(struct iommu_dev_data *dev_data)
>>  	device_flush_dte(dev_data);
>>  
>>  	/* Flush IOTLB and wait for the flushes to finish */
>> -	amd_iommu_domain_flush_tlb_pde(domain);
>> +	amd_iommu_domain_flush_all(domain);
> 
> This is weird.. The cache tag for a domain shouldn't necessarily be
> flushed from the iotlb when a domain is removed from a dte, should it?


In this path we need to flush IOMMU cache along with device IOTLB. That's why
original code called domain flush. May be we can split this into separate piece
where DTE update path will flush the IOMMU TLB and then in do_detach() path we
can just flush IOTLB. I think this needs more investigation to make sure we
don't break things.

Given the amount of changes we are doing in this space I'd prefer not to touch
this now. I will add it into my TODO list. Will revisit along with other
invalidation improvement that I planned to do later (like re-introducing PDE
bit, etc.).


-Vasant

> 
> ATC and IOTLB flushing should be seperate. The IOTLB should be flushed
> immediately before some event that causes the cache tag to become
> invalid, eg returning the tag to an allocator.
> 
> It is a minor point, but it does make things more understandable if
> stuff is done in the right places.
> 
> Jason

Re: [PATCH v1 09/13] iommu/amd: Refactor domain flush global function

[Jason Gunthorpe]

On Mon, Nov 06, 2023 at 04:23:10PM +0530, Vasant Hegde wrote:
> 
> >> @@ -1553,10 +1553,18 @@ static void domain_flush_pages(struct protection_domain *domain,
> >>  	amd_iommu_domain_flush_complete(domain);
> >>  }
> >>  
> >> +/* Flush range of IO/TLB for a given protection domain */
> >> +void amd_iommu_domain_flush_pages(struct protection_domain *pdom,
> >> +				  u64 address, size_t size)
> >> +{
> >> +	return domain_flush_pages(pdom, address, size);
> >> +}
> > 
> > What is the point of having this maze of functions? Just rename
> > domain_flush_pages?
> 
> Later patch adds PASID check to this function.

I saw that, I still think it is a maze.

> >> @@ -1859,7 +1867,7 @@ static void do_detach(struct iommu_dev_data *dev_data)
> >>  	device_flush_dte(dev_data);
> >>  
> >>  	/* Flush IOTLB and wait for the flushes to finish */
> >> -	amd_iommu_domain_flush_tlb_pde(domain);
> >> +	amd_iommu_domain_flush_all(domain);
> > 
> > This is weird.. The cache tag for a domain shouldn't necessarily be
> > flushed from the iotlb when a domain is removed from a dte, should it?
>
> In this path we need to flush IOMMU cache along with device IOTLB.

The iommu cache only needs to be flushed if the IOPTEs associated with
the cache tag change, and that doesn't happen during "detach".

Some of this is confusing because we've largely removed the word
'detach' from the iommu language, we now just have attach, eg replace.

When the driver has to replace an iommu_domain translation to a PCI
RID with a new translation it should run a sequence depending on how
it is using cache tags.

Cache tag shared in the domain - eg the cache tag is stored in the
iommu_domain struct:

 - Load the DTE/GCR3/etc with the new translation
 - Invalidate any RID -> Cache tag caches (at this point ATS requests
    see the new translation)
 - Invalidate the ATC
 
 The cache tag is flushed when the domain is freed and the tag
 returned to the allocator

Cache tag is unique to the device, replacing the translation continues
to use the existing tag:

 - Load the DTE/GCR3/etc with the new translation
 - Invalidate any RID -> DTE/Cache tag caches (at this point ATS requests
    see the new translation)
 - Invalidate the cache tag, since we changed the translation
 - Invalidate the ATC

 The cache tag is flushed when the GCR3 table is freed and the tag
 returned to the allocator

I agree it does not need to be in this series, but there is a logic to
how all this should flow that should be captured in the APIs. Bundling
all kinds of IOTLB and ATC invalidation into one call chain is not
consistent with where things should end up.

The attach/replace ops should implement the exact sequence of
invalidations they need for what they are doing in a clear sequence.

Jason

Re: [PATCH v1 09/13] iommu/amd: Refactor domain flush global function

[Vasant Hegde]

On 11/6/2023 6:31 PM, Jason Gunthorpe wrote:
> On Mon, Nov 06, 2023 at 04:23:10PM +0530, Vasant Hegde wrote:
>>
>>>> @@ -1553,10 +1553,18 @@ static void domain_flush_pages(struct protection_domain *domain,
>>>>  	amd_iommu_domain_flush_complete(domain);
>>>>  }
>>>>  
>>>> +/* Flush range of IO/TLB for a given protection domain */
>>>> +void amd_iommu_domain_flush_pages(struct protection_domain *pdom,
>>>> +				  u64 address, size_t size)
>>>> +{
>>>> +	return domain_flush_pages(pdom, address, size);
>>>> +}
>>>
>>> What is the point of having this maze of functions? Just rename
>>> domain_flush_pages?
>>
>> Later patch adds PASID check to this function.
> 
> I saw that, I still think it is a maze.
> 
>>>> @@ -1859,7 +1867,7 @@ static void do_detach(struct iommu_dev_data *dev_data)
>>>>  	device_flush_dte(dev_data);
>>>>  
>>>>  	/* Flush IOTLB and wait for the flushes to finish */
>>>> -	amd_iommu_domain_flush_tlb_pde(domain);
>>>> +	amd_iommu_domain_flush_all(domain);
>>>
>>> This is weird.. The cache tag for a domain shouldn't necessarily be
>>> flushed from the iotlb when a domain is removed from a dte, should it?
>>
>> In this path we need to flush IOMMU cache along with device IOTLB.
> 
> The iommu cache only needs to be flushed if the IOPTEs associated with
> the cache tag change, and that doesn't happen during "detach".
> 
> Some of this is confusing because we've largely removed the word
> 'detach' from the iommu language, we now just have attach, eg replace.

But we still need detach() function inside our driver so that we can safely
remove it from current domain and attach it to new domain.

> 
> When the driver has to replace an iommu_domain translation to a PCI
> RID with a new translation it should run a sequence depending on how
> it is using cache tags.
> 
> Cache tag shared in the domain - eg the cache tag is stored in the
> iommu_domain struct:
> 
>  - Load the DTE/GCR3/etc with the new translation
>  - Invalidate any RID -> Cache tag caches (at this point ATS requests
>     see the new translation)
>  - Invalidate the ATC
>  
>  The cache tag is flushed when the domain is freed and the tag
>  returned to the allocator
> 
> Cache tag is unique to the device, replacing the translation continues
> to use the existing tag:
> 
>  - Load the DTE/GCR3/etc with the new translation
>  - Invalidate any RID -> DTE/Cache tag caches (at this point ATS requests
>     see the new translation)
>  - Invalidate the cache tag, since we changed the translation
>  - Invalidate the ATC
> 
>  The cache tag is flushed when the GCR3 table is freed and the tag
>  returned to the allocator
> 
> I agree it does not need to be in this series, but there is a logic to
> how all this should flow that should be captured in the APIs. Bundling
> all kinds of IOTLB and ATC invalidation into one call chain is not
> consistent with where things should end up.

As mentioned earlier this needs investigation and will look into it later.

-Vasant

Re: [PATCH v1 09/13] iommu/amd: Refactor domain flush global function

[Jason Gunthorpe]

On Tue, Nov 07, 2023 at 10:23:16AM +0530, Vasant Hegde wrote:
> >>>> @@ -1859,7 +1867,7 @@ static void do_detach(struct iommu_dev_data *dev_data)
> >>>>  	device_flush_dte(dev_data);
> >>>>  
> >>>>  	/* Flush IOTLB and wait for the flushes to finish */
> >>>> -	amd_iommu_domain_flush_tlb_pde(domain);
> >>>> +	amd_iommu_domain_flush_all(domain);
> >>>
> >>> This is weird.. The cache tag for a domain shouldn't necessarily be
> >>> flushed from the iotlb when a domain is removed from a dte, should it?
> >>
> >> In this path we need to flush IOMMU cache along with device IOTLB.
> > 
> > The iommu cache only needs to be flushed if the IOPTEs associated with
> > the cache tag change, and that doesn't happen during "detach".
> > 
> > Some of this is confusing because we've largely removed the word
> > 'detach' from the iommu language, we now just have attach, eg replace.
> 
> But we still need detach() function inside our driver so that we can safely
> remove it from current domain and attach it to new domain.

No, you don't. It is like that today, but it is a functional bug that
it works like this. The driver can't meet the core API expectations
with this design. Review the smmuv3 series I sent to undertand this
fully.

Please try to organize things so we are moving toward a direction of
correctness and not gaining more problems.

Jason

[PATCH v1 10/13] iommu/amd: Consolidate domain flush logic

[Vasant Hegde]

Enhance domain_flush_pages() to flush guest page table as well. So that
we have single function to flush TLB/IOTLB. Also move IOMMU TLB and device
IOTLB invalidation to separate functions. This is useful when we introduce
PASID invalidation support.

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

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index c30b08e2a939..8da445e51251 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1465,23 +1465,42 @@ static int device_flush_dte(struct iommu_dev_data *dev_data)
 	return ret;
 }
 
+/* Invalidate device IOTLB for all devices in the protection domain */
+static int domain_flush_dev_iotlb_range(struct protection_domain *pdom,
+					ioasid_t pasid, u64 address, size_t size)
+{
+	struct iommu_dev_data *dev_data;
+	int ret = 0;
+
+	list_for_each_entry(dev_data, &pdom->dev_list, list) {
+		/*
+		 * There might be non-ATS capable devices in the same
+		 * protection domain.
+		 */
+		if (!dev_data->ats_enabled)
+			continue;
+
+		ret |= device_flush_iotlb_range(dev_data, pasid, address, size);
+	}
+
+	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.
+ * IOMMU TLB needs to be flushed before Device TLB to prevent device
+ * TLB refill from IOMMU TLB
  */
-static void __domain_flush_pages(struct protection_domain *domain,
-				 u64 address, size_t size)
+static int domain_flush_tlb_range(struct protection_domain *pdom,
+				  ioasid_t pasid, u64 address, size_t size)
 {
-	struct iommu_dev_data *dev_data;
 	struct iommu_cmd cmd;
-	int ret = 0, i;
+	int i, ret = 0;
+	bool gn = is_pasid_valid(pasid);
 
-	build_inv_iommu_pages(&cmd, address, size, domain->id,
-			      IOMMU_NO_PASID, false);
+	build_inv_iommu_pages(&cmd, address, size, pdom->id, pasid, gn);
 
 	for (i = 0; i < amd_iommu_get_num_iommus(); ++i) {
-		if (!domain->dev_iommu[i])
+		if (!pdom->dev_iommu[i])
 			continue;
 
 		/*
@@ -1491,23 +1510,30 @@ static void __domain_flush_pages(struct protection_domain *domain,
 		ret |= iommu_queue_command(amd_iommus[i], &cmd);
 	}
 
-	list_for_each_entry(dev_data, &domain->dev_list, list) {
+	return ret;
+}
 
-		if (!dev_data->ats_enabled)
-			continue;
+/*
+ * 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 *pdom,
+				 ioasid_t pasid, u64 address, size_t size)
+{
+	int ret;
 
-		ret |= device_flush_iotlb_range(dev_data, IOMMU_PASID_INVALID,
-						address, size);
-	}
+	ret = domain_flush_tlb_range(pdom, pasid, address, size);
+
+	ret |= domain_flush_dev_iotlb_range(pdom, pasid, address, size);
 
 	WARN_ON(ret);
 }
 
 static void domain_flush_pages(struct protection_domain *domain,
-			       u64 address, size_t size)
+			       ioasid_t pasid, u64 address, size_t size)
 {
 	if (likely(!amd_iommu_np_cache)) {
-		__domain_flush_pages(domain, address, size);
+		__domain_flush_pages(domain, pasid, address, size);
 
 		/* Wait until IOMMU TLB and all device IOTLB flushes are complete */
 		amd_iommu_domain_flush_complete(domain);
@@ -1544,7 +1570,7 @@ static void domain_flush_pages(struct protection_domain *domain,
 
 		flush_size = 1ul << min_alignment;
 
-		__domain_flush_pages(domain, address, flush_size);
+		__domain_flush_pages(domain, pasid, address, flush_size);
 		address += flush_size;
 		size -= flush_size;
 	}
@@ -1557,7 +1583,9 @@ static void domain_flush_pages(struct protection_domain *domain,
 void amd_iommu_domain_flush_pages(struct protection_domain *pdom,
 				  u64 address, size_t size)
 {
-	return domain_flush_pages(pdom, address, size);
+	ioasid_t pasid = pdom_get_default_pasid(pdom);
+
+	return domain_flush_pages(pdom, pasid, address, size);
 }
 
 /* Flush the whole IO/TLB for a given protection domain - including PDE */
-- 
2.31.1

Re: [PATCH v1 10/13] iommu/amd: Consolidate domain flush logic

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 10:16:21AM +0000, Vasant Hegde wrote:

> @@ -1557,7 +1583,9 @@ static void domain_flush_pages(struct protection_domain *domain,
>  void amd_iommu_domain_flush_pages(struct protection_domain *pdom,
>  				  u64 address, size_t size)
>  {
> -	return domain_flush_pages(pdom, address, size);
> +	ioasid_t pasid = pdom_get_default_pasid(pdom);
> +
> +	return domain_flush_pages(pdom, pasid, address, size);
>  }

There should be no such thing as a pasid for a domain. There should
not be an api called pdom_get_default_pasid(), it is nonsensical.

The callers know they are being called on a RID because they are RID
ops. They can invoke the actual flusher with the RID PASID directly.

Jason

Re: [PATCH v1 10/13] iommu/amd: Consolidate domain flush logic

[Vasant Hegde]

On 11/5/2023 11:25 PM, Jason Gunthorpe wrote:
> On Fri, Oct 06, 2023 at 10:16:21AM +0000, Vasant Hegde wrote:
> 
>> @@ -1557,7 +1583,9 @@ static void domain_flush_pages(struct protection_domain *domain,
>>  void amd_iommu_domain_flush_pages(struct protection_domain *pdom,
>>  				  u64 address, size_t size)
>>  {
>> -	return domain_flush_pages(pdom, address, size);
>> +	ioasid_t pasid = pdom_get_default_pasid(pdom);
>> +
>> +	return domain_flush_pages(pdom, pasid, address, size);
>>  }
> 
> There should be no such thing as a pasid for a domain. There should

We do have default PASID (zero) when we boot in v2 page table mode.

> not be an api called pdom_get_default_pasid(), it is nonsensical.

But we have single command to deal with all IOMMU TLB flush.
I have a choice of doing all checks in low level function (what this patch does)
 -OR- add checks in all API path (like flush_iotlb_all, iotlb_sync, etc). If
that's what preferred I can do that.

-Vasant

Re: [PATCH v1 10/13] iommu/amd: Consolidate domain flush logic

[Jason Gunthorpe]

On Mon, Nov 06, 2023 at 04:42:17PM +0530, Vasant Hegde wrote:
> 
> 
> On 11/5/2023 11:25 PM, Jason Gunthorpe wrote:
> > On Fri, Oct 06, 2023 at 10:16:21AM +0000, Vasant Hegde wrote:
> > 
> >> @@ -1557,7 +1583,9 @@ static void domain_flush_pages(struct protection_domain *domain,
> >>  void amd_iommu_domain_flush_pages(struct protection_domain *pdom,
> >>  				  u64 address, size_t size)
> >>  {
> >> -	return domain_flush_pages(pdom, address, size);
> >> +	ioasid_t pasid = pdom_get_default_pasid(pdom);
> >> +
> >> +	return domain_flush_pages(pdom, pasid, address, size);
> >>  }
> > 
> > There should be no such thing as a pasid for a domain. There should
> 
> We do have default PASID (zero) when we boot in v2 page table mode.

That is the PASID for a RID, not the PASID or a domain.

> > not be an api called pdom_get_default_pasid(), it is nonsensical.
> 
> But we have single command to deal with all IOMMU TLB flush.

This is probably a mistake..

> I have a choice of doing all checks in low level function (what this patch does)
>  -OR- add checks in all API path (like flush_iotlb_all, iotlb_sync, etc). If
> that's what preferred I can do that.

Well, you should not have concepts like the 'pasid of a domain' and
you should not encode the v1/v2 type into the pasid. I didn't look
closely how to sort that out to say exactly.

I can say that in SMMUv3 structuring it so that the attach op at the
top of the call chain knows what to do, and called a bunch of specific
helpers was reasonable. We know at the op level if we are going to do
a V1 or V2 invalidation because the OP itself already knows if it is
working on a V1/V2 iommu domain due to how it has to load the domain
into the DTE/GCR3.

So if you have:

   invalidate_iotlb_range/all_v1(domain)
   invalidate_iotlb_range/all_v2(device, pasid)
   invalidate_iotlb_full_v2(device)  // Clean every pasid
   invalidate_atc_range/all(domain)
     invalidate_atc_device_range_all(device, pasid)
   invalidate_atc_full(device) // Clean every pasid

As working functions then the ops should be writable to know exactly
which of those call based on the op.

For instance, if you have a v1 page table then you should have a v1
specific flush op that does:

   invalidate_iotlb_range/all_v1(domain)
   if (domain->ats in use)
     invalidate_atc_range/all(domain)

No maze.

Jason

Re: [PATCH v1 10/13] iommu/amd: Consolidate domain flush logic

[Vasant Hegde]

On 11/6/2023 6:43 PM, Jason Gunthorpe wrote:
> On Mon, Nov 06, 2023 at 04:42:17PM +0530, Vasant Hegde wrote:
>>
>>
>> On 11/5/2023 11:25 PM, Jason Gunthorpe wrote:
>>> On Fri, Oct 06, 2023 at 10:16:21AM +0000, Vasant Hegde wrote:
>>>
>>>> @@ -1557,7 +1583,9 @@ static void domain_flush_pages(struct protection_domain *domain,
>>>>  void amd_iommu_domain_flush_pages(struct protection_domain *pdom,
>>>>  				  u64 address, size_t size)
>>>>  {
>>>> -	return domain_flush_pages(pdom, address, size);
>>>> +	ioasid_t pasid = pdom_get_default_pasid(pdom);
>>>> +
>>>> +	return domain_flush_pages(pdom, pasid, address, size);
>>>>  }
>>>
>>> There should be no such thing as a pasid for a domain. There should
>>
>> We do have default PASID (zero) when we boot in v2 page table mode.
> 
> That is the PASID for a RID, not the PASID or a domain.
> 
>>> not be an api called pdom_get_default_pasid(), it is nonsensical.
>>
>> But we have single command to deal with all IOMMU TLB flush.
> 
> This is probably a mistake..
> 
>> I have a choice of doing all checks in low level function (what this patch does)
>>  -OR- add checks in all API path (like flush_iotlb_all, iotlb_sync, etc). If
>> that's what preferred I can do that.
> 
> Well, you should not have concepts like the 'pasid of a domain' and
> you should not encode the v1/v2 type into the pasid. I didn't look
> closely how to sort that out to say exactly.
> 
> I can say that in SMMUv3 structuring it so that the attach op at the
> top of the call chain knows what to do, and called a bunch of specific
> helpers was reasonable. We know at the op level if we are going to do
> a V1 or V2 invalidation because the OP itself already knows if it is
> working on a V1/V2 iommu domain due to how it has to load the domain
> into the DTE/GCR3.


AMD IOMMU invalidation is based on domain and we have single command which takes
PASID as well. Current code does have separate functions but that made code
duplicate. So I try to consolidate all low level functions.

> 
> So if you have:
> 
>    invalidate_iotlb_range/all_v1(domain)
>    invalidate_iotlb_range/all_v2(device, pasid)
>    invalidate_iotlb_full_v2(device)  // Clean every pasid
>    invalidate_atc_range/all(domain)
>      invalidate_atc_device_range_all(device, pasid)
>    invalidate_atc_full(device) // Clean every pasid


I am going to have separate functions for V1 and V2 domain flush.

> 
> As working functions then the ops should be writable to know exactly
> which of those call based on the op.
> 
> For instance, if you have a v1 page table then you should have a v1
> specific flush op that does:
> 
>    invalidate_iotlb_range/all_v1(domain)
>    if (domain->ats in use)
>      invalidate_atc_range/all(domain)

That's exactly domain_flush_page() does (flush IOMMU TLB and then flush device
IOTLB). Just that I need to fix upper layer function and pass few extra params
to low level functions.

-Vasant

Re: [PATCH v1 10/13] iommu/amd: Consolidate domain flush logic

[Jason Gunthorpe]

On Tue, Nov 07, 2023 at 10:14:17AM +0530, Vasant Hegde wrote:

> > I can say that in SMMUv3 structuring it so that the attach op at the
> > top of the call chain knows what to do, and called a bunch of specific
> > helpers was reasonable. We know at the op level if we are going to do
> > a V1 or V2 invalidation because the OP itself already knows if it is
> > working on a V1/V2 iommu domain due to how it has to load the domain
> > into the DTE/GCR3.
> 
> AMD IOMMU invalidation is based on domain and we have single command which takes
> PASID as well. Current code does have separate functions but that made code
> duplicate. So I try to consolidate all low level functions.

It is really not different from SMMU, invalidation commands for S1 or
S2 tables are constructed differently.

I would say the driver should have a way to issue the commands and a
way to construct the commands. Pass the actual command in instead of
passing in a mess of arguments to try to guess what command the caller
wanted.

> > As working functions then the ops should be writable to know exactly
> > which of those call based on the op.
> > 
> > For instance, if you have a v1 page table then you should have a v1
> > specific flush op that does:
> > 
> >    invalidate_iotlb_range/all_v1(domain)
> >    if (domain->ats in use)
> >      invalidate_atc_range/all(domain)
> 
> That's exactly domain_flush_page() does (flush IOMMU TLB and then flush device
> IOTLB). Just that I need to fix upper layer function and pass few extra params
> to low level functions.

Yes, but that is not always helpful to bury it like this because the
ATC invalidation needs to be done in a few places seperated from the
IOTLB invalidation, and sometimes done without a domain.

Jason

Re: [PATCH v1 10/13] iommu/amd: Consolidate domain flush logic

[Vasant Hegde]

On 11/7/2023 6:39 PM, Jason Gunthorpe wrote:
> On Tue, Nov 07, 2023 at 10:14:17AM +0530, Vasant Hegde wrote:
> 
>>> I can say that in SMMUv3 structuring it so that the attach op at the
>>> top of the call chain knows what to do, and called a bunch of specific
>>> helpers was reasonable. We know at the op level if we are going to do
>>> a V1 or V2 invalidation because the OP itself already knows if it is
>>> working on a V1/V2 iommu domain due to how it has to load the domain
>>> into the DTE/GCR3.
>>
>> AMD IOMMU invalidation is based on domain and we have single command which takes
>> PASID as well. Current code does have separate functions but that made code
>> duplicate. So I try to consolidate all low level functions.
> 
> It is really not different from SMMU, invalidation commands for S1 or
> S2 tables are constructed differently.

I don't understand ARM completely. So I don't think I understood the entire code.

Looking into https://github.com/jgunthorpe/linux/commits/smmuv3_newapi tree,

As I understand you have S1/S2. I guess that's something similar to V1 and V2
page table. arm_smmu_tlb_inv_range_domain() function basically checks for S1 or S2.

I will replace pdom_get_default_pasid(pdom) with pdom_is_v2_pgtbl_mode(). So
code will be something like

domain_flush_pages_v1()
{
	for (i = 0; i < amd_iommu_get_num_iommus(); ++i)
		iommu_queue_command()
}

domain_flush_pages_v2()
{
	list_for_each_entry(dev_data, &domain->dev_list, list)
		iommu_queue_command()
}

/* Invalidation based on domain */
amd_iommu_domain_flush_pages(pdom, addr, size)
{
	pasid = INVALID;
	gn = false;

	if (pdom_is_v2_pgtbl_mode(pdom)) {
		pasid = 0; gn = true
		domain_flush_pages_v2()
	} else
		domain_flush_pages_v1()

	list_for_each_entry(dev_data, &domain->dev_list, list)
		__device_flush_iotlb(dev_data, addr, size, pasid, gn)
		
}

/* Invalidation based on device */

amd_iommu_dev_flush_pasid_pages()
{
	iommu_flush_tlb_range(iommu, domid, pasid, addr, size)

	device_flush_iotlb_pasid(dev_data, pasid, addr, size)
}


> 
> I would say the driver should have a way to issue the commands and a
> way to construct the commands. Pass the actual command in instead of
> passing in a mess of arguments to try to guess what command the caller
> wanted.

We have helper functions (build_inv_*) functions which builds the actual command.

-Vasant

Re: [PATCH v1 10/13] iommu/amd: Consolidate domain flush logic

[Jason Gunthorpe]

On Thu, Nov 09, 2023 at 07:22:09PM +0530, Vasant Hegde wrote:
> 
> 
> On 11/7/2023 6:39 PM, Jason Gunthorpe wrote:
> > On Tue, Nov 07, 2023 at 10:14:17AM +0530, Vasant Hegde wrote:
> > 
> >>> I can say that in SMMUv3 structuring it so that the attach op at the
> >>> top of the call chain knows what to do, and called a bunch of specific
> >>> helpers was reasonable. We know at the op level if we are going to do
> >>> a V1 or V2 invalidation because the OP itself already knows if it is
> >>> working on a V1/V2 iommu domain due to how it has to load the domain
> >>> into the DTE/GCR3.
> >>
> >> AMD IOMMU invalidation is based on domain and we have single command which takes
> >> PASID as well. Current code does have separate functions but that made code
> >> duplicate. So I try to consolidate all low level functions.
> > 
> > It is really not different from SMMU, invalidation commands for S1 or
> > S2 tables are constructed differently.
> 
> I don't understand ARM completely. So I don't think I understood the entire code.

It is really similar, of the three AMD and ARM are almost twins. VT-D
is completely different.

> Looking into https://github.com/jgunthorpe/linux/commits/smmuv3_newapi tree,
> 
> As I understand you have S1/S2. I guess that's something similar to V1 and V2
> page table. 

Yes

> arm_smmu_tlb_inv_range_domain() function basically checks for S1 or S2.

Yes, I haven't touched that yet.

The function is always called from an OP callback:

 iommu_flush_ops -> arm_smmu_tlb_inv_walk -> arm_smmu_tlb_inv_range_domain
 iotlb_sync -> arm_smmu_tlb_inv_range_domain

And in both cases we can arrange things so that there are S1 and S2
specific ops that already know exactly what they should be doing.

> domain_flush_pages_v1()
> {
> 	for (i = 0; i < amd_iommu_get_num_iommus(); ++i)
> 		iommu_queue_command()
> }
> 
> domain_flush_pages_v2()
> {
> 	list_for_each_entry(dev_data, &domain->dev_list, list)
> 		iommu_queue_command()
> }
> 
> /* Invalidation based on domain */
> amd_iommu_domain_flush_pages(pdom, addr, size)
> {
> 	pasid = INVALID;
> 	gn = false;
> 
> 	if (pdom_is_v2_pgtbl_mode(pdom)) {
> 		pasid = 0; gn = true
> 		domain_flush_pages_v2()
> 	} else
> 		domain_flush_pages_v1()
> 
> 	list_for_each_entry(dev_data, &domain->dev_list, list)
> 		__device_flush_iotlb(dev_data, addr, size, pasid, gn)
> 		
> }

Looks logical

> /* Invalidation based on device */
>
> amd_iommu_dev_flush_pasid_pages()
> {
> 	iommu_flush_tlb_range(iommu, domid, pasid, addr, size)
> 
> 	device_flush_iotlb_pasid(dev_data, pasid, addr, size)
> }

When does this happen? There were only two places in ARM land where we
needed a non-domain invalidation:
 - Flushing a PCI device ATC in an attach op callback due to a
   translation change or ATS enable/disable
 - Clearing an entire cache tag before returning it to the tag
   allocator

So I would not expect to see a range here, or to have the ATC in the
same function..

Jason

Re: [PATCH v1 10/13] iommu/amd: Consolidate domain flush logic

[Vasant Hegde]

On 11/9/2023 7:40 PM, Jason Gunthorpe wrote:
> On Thu, Nov 09, 2023 at 07:22:09PM +0530, Vasant Hegde wrote:
>>
>>
>> On 11/7/2023 6:39 PM, Jason Gunthorpe wrote:
>>> On Tue, Nov 07, 2023 at 10:14:17AM +0530, Vasant Hegde wrote:
>>>
>>>>> I can say that in SMMUv3 structuring it so that the attach op at the
>>>>> top of the call chain knows what to do, and called a bunch of specific
>>>>> helpers was reasonable. We know at the op level if we are going to do
>>>>> a V1 or V2 invalidation because the OP itself already knows if it is
>>>>> working on a V1/V2 iommu domain due to how it has to load the domain
>>>>> into the DTE/GCR3.
>>>>
>>>> AMD IOMMU invalidation is based on domain and we have single command which takes
>>>> PASID as well. Current code does have separate functions but that made code
>>>> duplicate. So I try to consolidate all low level functions.
>>>
>>> It is really not different from SMMU, invalidation commands for S1 or
>>> S2 tables are constructed differently.
>>
>> I don't understand ARM completely. So I don't think I understood the entire code.
> 
> It is really similar, of the three AMD and ARM are almost twins. VT-D
> is completely different.
> 
>> Looking into https://github.com/jgunthorpe/linux/commits/smmuv3_newapi tree,
>>
>> As I understand you have S1/S2. I guess that's something similar to V1 and V2
>> page table. 
> 
> Yes
> 
>> arm_smmu_tlb_inv_range_domain() function basically checks for S1 or S2.
> 
> Yes, I haven't touched that yet.
> 
> The function is always called from an OP callback:
> 
>  iommu_flush_ops -> arm_smmu_tlb_inv_walk -> arm_smmu_tlb_inv_range_domain
>  iotlb_sync -> arm_smmu_tlb_inv_range_domain
> 
> And in both cases we can arrange things so that there are S1 and S2
> specific ops that already know exactly what they should be doing.
> 
>> domain_flush_pages_v1()
>> {
>> 	for (i = 0; i < amd_iommu_get_num_iommus(); ++i)
>> 		iommu_queue_command()
>> }
>>
>> domain_flush_pages_v2()
>> {
>> 	list_for_each_entry(dev_data, &domain->dev_list, list)
>> 		iommu_queue_command()
>> }
>>
>> /* Invalidation based on domain */
>> amd_iommu_domain_flush_pages(pdom, addr, size)
>> {
>> 	pasid = INVALID;
>> 	gn = false;
>>
>> 	if (pdom_is_v2_pgtbl_mode(pdom)) {
>> 		pasid = 0; gn = true
>> 		domain_flush_pages_v2()
>> 	} else
>> 		domain_flush_pages_v1()
>>
>> 	list_for_each_entry(dev_data, &domain->dev_list, list)
>> 		__device_flush_iotlb(dev_data, addr, size, pasid, gn)
>> 		
>> }
> 
> Looks logical
> 
>> /* Invalidation based on device */
>>
>> amd_iommu_dev_flush_pasid_pages()
>> {
>> 	iommu_flush_tlb_range(iommu, domid, pasid, addr, size)
>>
>> 	device_flush_iotlb_pasid(dev_data, pasid, addr, size)
>> }
> 
> When does this happen? There were only two places in ARM land where we
> needed a non-domain invalidation:

This is for handling mmu notifier callbacks for SVA.

-Vasant


>  - Flushing a PCI device ATC in an attach op callback due to a
>    translation change or ATS enable/disable
>  - Clearing an entire cache tag before returning it to the tag
>    allocator
> 
> So I would not expect to see a range here, or to have the ATC in the
> same function..
> 
> Jason

Re: [PATCH v1 10/13] iommu/amd: Consolidate domain flush logic

[Jason Gunthorpe]

On Fri, Nov 10, 2023 at 10:58:27AM +0530, Vasant Hegde wrote:
> >> /* Invalidation based on device */
> >>
> >> amd_iommu_dev_flush_pasid_pages()
> >> {
> >> 	iommu_flush_tlb_range(iommu, domid, pasid, addr, size)
> >>
> >> 	device_flush_iotlb_pasid(dev_data, pasid, addr, size)
> >> }
> > 
> > When does this happen? There were only two places in ARM land where we
> > needed a non-domain invalidation:
> 
> This is for handling mmu notifier callbacks for SVA.

SVA always has a single domain, it should use the domain invalidation
functions.

Jason

[PATCH v1 11/13] iommu/amd/pgtbl_v2: Invalidate updated page ranges only

[Vasant Hegde]

amd_iommu_domain_flush_pages() uses protection domain page table mode
to detect/get default PASID. Hence we can use this function to
invalidate v2 page table as well.

Also in __set_gcr3()/__clear_gcr3() path call domain_flush_pages()
directly. So that we can remove __amd_iommu_flush_tlb() and related
functions. This works fine as we have GCR3 table per domain.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/io_pgtable_v2.c | 10 ++--------
 drivers/iommu/amd/iommu.c         |  6 ++++--
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/amd/io_pgtable_v2.c b/drivers/iommu/amd/io_pgtable_v2.c
index f818a7e254d4..6d69ba60744f 100644
--- a/drivers/iommu/amd/io_pgtable_v2.c
+++ b/drivers/iommu/amd/io_pgtable_v2.c
@@ -244,7 +244,6 @@ static int iommu_v2_map_pages(struct io_pgtable_ops *ops, unsigned long iova,
 	unsigned long mapped_size = 0;
 	unsigned long o_iova = iova;
 	size_t size = pgcount << __ffs(pgsize);
-	int count = 0;
 	int ret = 0;
 	bool updated = false;
 
@@ -265,19 +264,14 @@ static int iommu_v2_map_pages(struct io_pgtable_ops *ops, unsigned long iova,
 
 		*pte = set_pte_attr(paddr, map_size, prot);
 
-		count++;
 		iova += map_size;
 		paddr += map_size;
 		mapped_size += map_size;
 	}
 
 out:
-	if (updated) {
-		if (count > 1)
-			amd_iommu_flush_tlb(&pdom->domain, 0);
-		else
-			amd_iommu_flush_page(&pdom->domain, 0, o_iova);
-	}
+	if (updated)
+		amd_iommu_domain_flush_pages(pdom, o_iova, size);
 
 	if (mapped)
 		*mapped += mapped_size;
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 8da445e51251..764a5656b56b 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2698,7 +2698,8 @@ static int __set_gcr3(struct protection_domain *domain, u32 pasid,
 
 	*pte = (cr3 & PAGE_MASK) | GCR3_VALID;
 
-	return __amd_iommu_flush_tlb(domain, pasid);
+	domain_flush_pages(domain, pasid, 0, CMD_INV_IOMMU_ALL_PAGES_ADDRESS);
+	return 0;
 }
 
 static int __clear_gcr3(struct protection_domain *domain, u32 pasid)
@@ -2714,7 +2715,8 @@ static int __clear_gcr3(struct protection_domain *domain, u32 pasid)
 
 	*pte = 0;
 
-	return __amd_iommu_flush_tlb(domain, pasid);
+	domain_flush_pages(domain, pasid, 0, CMD_INV_IOMMU_ALL_PAGES_ADDRESS);
+	return 0;
 }
 
 int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, u32 pasid,
-- 
2.31.1

Re: [PATCH v1 11/13] iommu/amd/pgtbl_v2: Invalidate updated page ranges only

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 10:16:22AM +0000, Vasant Hegde wrote:
> amd_iommu_domain_flush_pages() uses protection domain page table mode
> to detect/get default PASID. Hence we can use this function to
> invalidate v2 page table as well.
> 
> Also in __set_gcr3()/__clear_gcr3() path call domain_flush_pages()
> directly. So that we can remove __amd_iommu_flush_tlb() and related
> functions. This works fine as we have GCR3 table per domain.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/io_pgtable_v2.c | 10 ++--------
>  drivers/iommu/amd/iommu.c         |  6 ++++--
>  2 files changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iommu/amd/io_pgtable_v2.c b/drivers/iommu/amd/io_pgtable_v2.c
> index f818a7e254d4..6d69ba60744f 100644
> --- a/drivers/iommu/amd/io_pgtable_v2.c
> +++ b/drivers/iommu/amd/io_pgtable_v2.c
> @@ -244,7 +244,6 @@ static int iommu_v2_map_pages(struct io_pgtable_ops *ops, unsigned long iova,
>  	unsigned long mapped_size = 0;
>  	unsigned long o_iova = iova;
>  	size_t size = pgcount << __ffs(pgsize);
> -	int count = 0;
>  	int ret = 0;
>  	bool updated = false;
>  
> @@ -265,19 +264,14 @@ static int iommu_v2_map_pages(struct io_pgtable_ops *ops, unsigned long iova,
>  
>  		*pte = set_pte_attr(paddr, map_size, prot);
>  
> -		count++;
>  		iova += map_size;
>  		paddr += map_size;
>  		mapped_size += map_size;
>  	}
>  
>  out:
> -	if (updated) {
> -		if (count > 1)
> -			amd_iommu_flush_tlb(&pdom->domain, 0);
> -		else
> -			amd_iommu_flush_page(&pdom->domain, 0, o_iova);
> -	}
> +	if (updated)
> +		amd_iommu_domain_flush_pages(pdom, o_iova, size);
>  
>  	if (mapped)
>  		*mapped += mapped_size;
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 8da445e51251..764a5656b56b 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2698,7 +2698,8 @@ static int __set_gcr3(struct protection_domain *domain, u32 pasid,
>  
>  	*pte = (cr3 & PAGE_MASK) | GCR3_VALID;
>  
> -	return __amd_iommu_flush_tlb(domain, pasid);
> +	domain_flush_pages(domain, pasid, 0, CMD_INV_IOMMU_ALL_PAGES_ADDRESS);
> +	return 0;
>  }
>  
>  static int __clear_gcr3(struct protection_domain *domain, u32 pasid)
> @@ -2714,7 +2715,8 @@ static int __clear_gcr3(struct protection_domain *domain, u32 pasid)
>  
>  	*pte = 0;
>  
> -	return __amd_iommu_flush_tlb(domain, pasid);
> +	domain_flush_pages(domain, pasid, 0, CMD_INV_IOMMU_ALL_PAGES_ADDRESS);
> +	return 0;
>  }

You should make the other "flush all" wrapper accept a PASID and call
it here as well instead of open coding it, or do away with the flush
all wraper completely.

Jason

Re: [PATCH v1 11/13] iommu/amd/pgtbl_v2: Invalidate updated page ranges only

[Vasant Hegde]

On 11/5/2023 11:27 PM, Jason Gunthorpe wrote:
> On Fri, Oct 06, 2023 at 10:16:22AM +0000, Vasant Hegde wrote:
>> amd_iommu_domain_flush_pages() uses protection domain page table mode
>> to detect/get default PASID. Hence we can use this function to
>> invalidate v2 page table as well.
>>
>> Also in __set_gcr3()/__clear_gcr3() path call domain_flush_pages()
>> directly. So that we can remove __amd_iommu_flush_tlb() and related
>> functions. This works fine as we have GCR3 table per domain.
>>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/io_pgtable_v2.c | 10 ++--------
>>  drivers/iommu/amd/iommu.c         |  6 ++++--
>>  2 files changed, 6 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/io_pgtable_v2.c b/drivers/iommu/amd/io_pgtable_v2.c
>> index f818a7e254d4..6d69ba60744f 100644
>> --- a/drivers/iommu/amd/io_pgtable_v2.c
>> +++ b/drivers/iommu/amd/io_pgtable_v2.c
>> @@ -244,7 +244,6 @@ static int iommu_v2_map_pages(struct io_pgtable_ops *ops, unsigned long iova,
>>  	unsigned long mapped_size = 0;
>>  	unsigned long o_iova = iova;
>>  	size_t size = pgcount << __ffs(pgsize);
>> -	int count = 0;
>>  	int ret = 0;
>>  	bool updated = false;
>>  
>> @@ -265,19 +264,14 @@ static int iommu_v2_map_pages(struct io_pgtable_ops *ops, unsigned long iova,
>>  
>>  		*pte = set_pte_attr(paddr, map_size, prot);
>>  
>> -		count++;
>>  		iova += map_size;
>>  		paddr += map_size;
>>  		mapped_size += map_size;
>>  	}
>>  
>>  out:
>> -	if (updated) {
>> -		if (count > 1)
>> -			amd_iommu_flush_tlb(&pdom->domain, 0);
>> -		else
>> -			amd_iommu_flush_page(&pdom->domain, 0, o_iova);
>> -	}
>> +	if (updated)
>> +		amd_iommu_domain_flush_pages(pdom, o_iova, size);
>>  
>>  	if (mapped)
>>  		*mapped += mapped_size;
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 8da445e51251..764a5656b56b 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -2698,7 +2698,8 @@ static int __set_gcr3(struct protection_domain *domain, u32 pasid,
>>  
>>  	*pte = (cr3 & PAGE_MASK) | GCR3_VALID;
>>  
>> -	return __amd_iommu_flush_tlb(domain, pasid);
>> +	domain_flush_pages(domain, pasid, 0, CMD_INV_IOMMU_ALL_PAGES_ADDRESS);
>> +	return 0;
>>  }
>>  
>>  static int __clear_gcr3(struct protection_domain *domain, u32 pasid)
>> @@ -2714,7 +2715,8 @@ static int __clear_gcr3(struct protection_domain *domain, u32 pasid)
>>  
>>  	*pte = 0;
>>  
>> -	return __amd_iommu_flush_tlb(domain, pasid);
>> +	domain_flush_pages(domain, pasid, 0, CMD_INV_IOMMU_ALL_PAGES_ADDRESS);
>> +	return 0;
>>  }
> 
> You should make the other "flush all" wrapper accept a PASID and call
> it here as well instead of open coding it, or do away with the flush
> all wraper completely.

I'd say this is just a temporary fix to make sure we don't break things... as
next series (SVA Part3) completely changed these functions.

-Vasant

[PATCH v1 12/13] 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. PASID related functions are not used.

Also when we add SVA support we will be moving to per device GCR3 table.
We cannot use current flush pasid functions as is. 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, 1 insertion(+), 102 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 088890f9618a..38b3f4562f3b 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -53,7 +53,6 @@ int amd_iommu_pdev_enable_cap_pri(struct pci_dev *pdev);
 void amd_iommu_pdev_disable_cap_pri(struct pci_dev *pdev);
 
 /* TLB flush */
-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.
@@ -64,7 +63,7 @@ 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 *pdom,
 				  u64 address, size_t size);
-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);
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 764a5656b56b..fca1f969845d 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2552,106 +2552,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;
-}
-
 static u64 *__get_gcr3_pte(u64 *root, int level, u32 pasid, bool alloc)
 {
 	int index;
-- 
2.31.1

Re: [PATCH v1 12/13] iommu/amd: Remove unused flush pasid functions

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 10:16:23AM +0000, Vasant Hegde wrote:
> We have removed iommu_v2 module and converted v2 page table to use
> common flush functions. PASID related functions are not used.
> 
> Also when we add SVA support we will be moving to per device GCR3 table.
> We cannot use current flush pasid functions as is. 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, 1 insertion(+), 102 deletions(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
> index 088890f9618a..38b3f4562f3b 100644
> --- a/drivers/iommu/amd/amd_iommu.h
> +++ b/drivers/iommu/amd/amd_iommu.h
> @@ -53,7 +53,6 @@ int amd_iommu_pdev_enable_cap_pri(struct pci_dev *pdev);
>  void amd_iommu_pdev_disable_cap_pri(struct pci_dev *pdev);

Maybe this addresses my earlier remark about _pages?

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

Jason

Re: [PATCH v1 12/13] iommu/amd: Remove unused flush pasid functions

[Vasant Hegde]

On 11/5/2023 11:28 PM, Jason Gunthorpe wrote:
> On Fri, Oct 06, 2023 at 10:16:23AM +0000, Vasant Hegde wrote:
>> We have removed iommu_v2 module and converted v2 page table to use
>> common flush functions. PASID related functions are not used.
>>
>> Also when we add SVA support we will be moving to per device GCR3 table.
>> We cannot use current flush pasid functions as is. 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, 1 insertion(+), 102 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
>> index 088890f9618a..38b3f4562f3b 100644
>> --- a/drivers/iommu/amd/amd_iommu.h
>> +++ b/drivers/iommu/amd/amd_iommu.h
>> @@ -53,7 +53,6 @@ int amd_iommu_pdev_enable_cap_pri(struct pci_dev *pdev);
>>  void amd_iommu_pdev_disable_cap_pri(struct pci_dev *pdev);
> 
> Maybe this addresses my earlier remark about _pages?

Yes.

-Vasant

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

[PATCH v1 13/13] iommu/amd: Rearrange device flush code

[Vasant Hegde]

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

No functional changes intended.

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

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index fca1f969845d..2e90f7807168 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -77,6 +77,9 @@ 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 protection_domain *domain, bool ats, bool ppr);
+
 /****************************************************************************
  *
  * Helper functions
@@ -1636,6 +1639,54 @@ 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);
+
+		if (!iommu)
+			continue;
+		set_dte_entry(iommu, dev_data->devid, domain,
+			      dev_data->ats_enabled, dev_data->ppr);
+		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    = rlookup_amd_iommu(&pdev->dev);
+	if (!iommu)
+		return -ENODEV;
+
+	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
@@ -2039,42 +2090,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 = rlookup_amd_iommu(dev_data->dev);
-
-		if (!iommu)
-			continue;
-		set_dte_entry(iommu, dev_data->devid, domain,
-			      dev_data->ats_enabled, dev_data->ppr);
-		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
@@ -2646,24 +2661,6 @@ int amd_iommu_domain_clear_gcr3(struct iommu_domain *dom, u32 pasid)
 	return ret;
 }
 
-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    = rlookup_amd_iommu(&pdev->dev);
-	if (!iommu)
-		return -ENODEV;
-
-	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

Re: [PATCH v1 13/13] iommu/amd: Rearrange device flush code

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 10:16:24AM +0000, Vasant Hegde wrote:
> Consolidate all flush related code in one place so that its easy
> to mantain.
> 
> No functional changes intended.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 105 ++++++++++++++++++--------------------
>  1 file changed, 51 insertions(+), 54 deletions(-)

On one hand I wish to do this sort of code motion quite alot and agree
it helps maintainability to have like with like

But, in Linux I generally discourage it because it overly harms
backporting for quite a minimal gain.

Jason

Re: [PATCH v1 13/13] iommu/amd: Rearrange device flush code

[Vasant Hegde]

On 11/5/2023 11:29 PM, Jason Gunthorpe wrote:
> On Fri, Oct 06, 2023 at 10:16:24AM +0000, Vasant Hegde wrote:
>> Consolidate all flush related code in one place so that its easy
>> to mantain.
>>
>> No functional changes intended.
>>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/iommu.c | 105 ++++++++++++++++++--------------------
>>  1 file changed, 51 insertions(+), 54 deletions(-)
> 
> On one hand I wish to do this sort of code motion quite alot and agree
> it helps maintainability to have like with like
> 
> But, in Linux I generally discourage it because it overly harms
> backporting for quite a minimal gain.

In general yes. I don't prefer to have a patch like this.

With removing iommu_v2 module and consolidating all flush code, I felt this
patch is necessary for long term maintainability of the code.

-Vasant

Re: [PATCH v1 00/13] Improve TLB invalidation logic

[Suthikulpanit, Suravee]

On 10/6/2023 5:16 PM, Vasant Hegde wrote:
> Current code invalidates single page and entire range for the given
> domain ID. IOMMU hardware supports multiple page invalidation.
> This series adds support to invalidate range of pages.
> 
> Also consolidate various invalidation functions, remove unused functions,
> re-arranges invalidation related functions.
> 
> Note that this series doesn't add support for PASID based invalidation (except
> PASID 0 for v2 page table). We will include necessary changes in SVA series.
> 
> Testing:
>    - This series is tested with v1 and v2 page table and it works fine.
> 
> This patch series is based on top of SVA Part 2 [1] which in turn based on
> top of iommu/next branch (Commit 8e5ab3f54a10).
>    [1] https://lore.kernel.org/linux-iommu/20231006095706.5694-1-vasant.hegde@amd.com/T/#t
> 
> This is also available at github :
>     https://github.com/AMDESE/linux/tree/iommu_flush_improvement_v1_v6.6_rc1
> 
> 
> Thank you,
> Vasant

Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Thanks,
Suravee

Re: [PATCH v1 00/13] Improve TLB invalidation logic

[Vasant Hegde]

Hi Joerg,

On 10/6/2023 3:46 PM, Vasant Hegde wrote:
> Current code invalidates single page and entire range for the given
> domain ID. IOMMU hardware supports multiple page invalidation.
> This series adds support to invalidate range of pages.
> 

Ping. Did you get a chance to look into this series?

-Vasant

> Also consolidate various invalidation functions, remove unused functions,
> re-arranges invalidation related functions.
> 
> Note that this series doesn't add support for PASID based invalidation (except
> PASID 0 for v2 page table). We will include necessary changes in SVA series.
> 
> Testing:
>   - This series is tested with v1 and v2 page table and it works fine.
> 
> This patch series is based on top of SVA Part 2 [1] which in turn based on
> top of iommu/next branch (Commit 8e5ab3f54a10).
>   [1] https://lore.kernel.org/linux-iommu/20231006095706.5694-1-vasant.hegde@amd.com/T/#t
> 
> This is also available at github :
>    https://github.com/AMDESE/linux/tree/iommu_flush_improvement_v1_v6.6_rc1
> 
> 
> Thank you,
> Vasant
> 
> 
> Vasant Hegde (13):
>   iommu/amd: Rename iommu_flush_all_caches() -> amd_iommu_flush_all_caches()
>   iommu/amd: Remove redundant domain flush from attach_device()
>   iommu/amd: Remove redundant passing of PDE bit
>   iommu/amd: Add support to invalidate multiple guest pages
>   iommu/amd: Refactor IOMMU tlb invalidation code
>   iommu/amd: Refactor device iotlb invalidation code
>   iommu/amd: Consolidate device IOTLB flush code
>   iommu/amd: Consolidate amd_iommu_domain_flush_complete() call
>   iommu/amd: Refactor domain flush global function
>   iommu/amd: Consolidate domain flush logic
>   iommu/amd/pgtbl_v2: Invalidate updated page ranges only
>   iommu/amd: Remove unused flush pasid functions
>   iommu/amd: Rearrange device flush code
> 
>  drivers/iommu/amd/amd_iommu.h       |  12 +-
>  drivers/iommu/amd/amd_iommu_types.h |   6 -
>  drivers/iommu/amd/init.c            |   8 +-
>  drivers/iommu/amd/io_pgtable.c      |   5 +-
>  drivers/iommu/amd/io_pgtable_v2.c   |  10 +-
>  drivers/iommu/amd/iommu.c           | 417 ++++++++++++----------------
>  6 files changed, 189 insertions(+), 269 deletions(-)
>