[PATCH 0/4] iommu/amd: Enable page-selective flushes

[PATCH 0/4] iommu/amd: Enable page-selective flushes

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

The previous patch, commit 268aa4548277 ("iommu/amd: Page-specific
invalidations for more than one page") was supposed to enable
page-selective IOTLB flushes on AMD.

The patch had an embaressing bug, and I apologize for it.

Analysis as for why this bug did not result in failures raised
additional issues that caused at least most of the IOTLB flushes not to
be page-selective ones.

The first patch corrects the bug from the previous patch. The next
patches enable page-selective invalidations, which were not enabled
despite the previous patch.

Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will@kernel.org>
Cc: Jiajun Cao <caojiajun@vmware.com>
Cc: iommu@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org

Nadav Amit (4):
  iommu/amd: Fix wrong parentheses on page-specific invalidations
  iommu/amd: Selective flush on unmap
  iommu/amd: Do not sync on page size changes
  iommu/amd: Do not use flush-queue when NpCache is on

 drivers/iommu/amd/init.c  |  7 ++++++-
 drivers/iommu/amd/iommu.c | 18 +++++++++++++++---
 include/linux/iommu.h     |  3 ++-
 3 files changed, 23 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH 0/4] iommu/amd: Enable page-selective flushes

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

The previous patch, commit 268aa4548277 ("iommu/amd: Page-specific
invalidations for more than one page") was supposed to enable
page-selective IOTLB flushes on AMD.

The patch had an embaressing bug, and I apologize for it.

Analysis as for why this bug did not result in failures raised
additional issues that caused at least most of the IOTLB flushes not to
be page-selective ones.

The first patch corrects the bug from the previous patch. The next
patches enable page-selective invalidations, which were not enabled
despite the previous patch.

Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will@kernel.org>
Cc: Jiajun Cao <caojiajun@vmware.com>
Cc: iommu@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org

Nadav Amit (4):
  iommu/amd: Fix wrong parentheses on page-specific invalidations
  iommu/amd: Selective flush on unmap
  iommu/amd: Do not sync on page size changes
  iommu/amd: Do not use flush-queue when NpCache is on

 drivers/iommu/amd/init.c  |  7 ++++++-
 drivers/iommu/amd/iommu.c | 18 +++++++++++++++---
 include/linux/iommu.h     |  3 ++-
 3 files changed, 23 insertions(+), 5 deletions(-)

-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 1/4] iommu/amd: Fix wrong parentheses on page-specific invalidations

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

The logic to determine the mask of page-specific invalidations was
tested in userspace. As the code was copied into the kernel, the
parentheses were mistakenly set in the wrong place, resulting in the
wrong mask.

Fix it.

Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will@kernel.org>
Cc: Jiajun Cao <caojiajun@vmware.com>
Cc: iommu@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
Fixes: 268aa4548277 ("iommu/amd: Page-specific invalidations for more than one page")
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 drivers/iommu/amd/iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 80e8e1916dd1..6723cbcf4030 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -884,7 +884,7 @@ static inline u64 build_inv_address(u64 address, size_t size)
 		 * The msb-bit must be clear on the address. Just set all the
 		 * lower bits.
 		 */
-		address |= 1ull << (msb_diff - 1);
+		address |= (1ull << msb_diff) - 1;
 	}
 
 	/* Clear bits 11:0 */
-- 
2.25.1

[PATCH 1/4] iommu/amd: Fix wrong parentheses on page-specific invalidations

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

The logic to determine the mask of page-specific invalidations was
tested in userspace. As the code was copied into the kernel, the
parentheses were mistakenly set in the wrong place, resulting in the
wrong mask.

Fix it.

Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will@kernel.org>
Cc: Jiajun Cao <caojiajun@vmware.com>
Cc: iommu@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
Fixes: 268aa4548277 ("iommu/amd: Page-specific invalidations for more than one page")
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 drivers/iommu/amd/iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 80e8e1916dd1..6723cbcf4030 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -884,7 +884,7 @@ static inline u64 build_inv_address(u64 address, size_t size)
 		 * The msb-bit must be clear on the address. Just set all the
 		 * lower bits.
 		 */
-		address |= 1ull << (msb_diff - 1);
+		address |= (1ull << msb_diff) - 1;
 	}
 
 	/* Clear bits 11:0 */
-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 2/4] iommu/amd: Do not sync on page size changes

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

Some IOMMU architectures perform invalidations regardless of the page
size. In such architectures there is no need to sync when the page size
changes. In such architecture, there is no need to regard pgsize when
making interim flush in iommu_iotlb_gather_add_page().

Add a "no_sync_on_pgsize_change" property for each IOMMU ops to decide
whether gather's pgsize is tracked and triggers a flush.

Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will@kernel.org>
Cc: Jiajun Cao <caojiajun@vmware.com>
Cc: iommu@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 drivers/iommu/amd/iommu.c | 1 +
 include/linux/iommu.h     | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 6723cbcf4030..8a71ad477c34 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2204,6 +2204,7 @@ const struct iommu_ops amd_iommu_ops = {
 	.put_resv_regions = generic_iommu_put_resv_regions,
 	.is_attach_deferred = amd_iommu_is_attach_deferred,
 	.pgsize_bitmap	= AMD_IOMMU_PGSIZES,
+	.ignore_gather_pgsize = true,
 	.flush_iotlb_all = amd_iommu_flush_iotlb_all,
 	.iotlb_sync = amd_iommu_iotlb_sync,
 	.def_domain_type = amd_iommu_def_domain_type,
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 32d448050bf7..1fb2695418e9 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -284,6 +284,7 @@ struct iommu_ops {
 	int (*def_domain_type)(struct device *dev);
 
 	unsigned long pgsize_bitmap;
+	bool ignore_gather_pgsize;
 	struct module *owner;
 };
 
@@ -508,7 +509,7 @@ static inline void iommu_iotlb_gather_add_page(struct iommu_domain *domain,
 	 * a different granularity, then sync the TLB so that the gather
 	 * structure can be rewritten.
 	 */
-	if (gather->pgsize != size ||
+	if ((gather->pgsize != size && !domain->ops->ignore_gather_pgsize) ||
 	    end + 1 < gather->start || start > gather->end + 1) {
 		if (gather->pgsize)
 			iommu_iotlb_sync(domain, gather);
-- 
2.25.1

[PATCH 2/4] iommu/amd: Do not sync on page size changes

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

Some IOMMU architectures perform invalidations regardless of the page
size. In such architectures there is no need to sync when the page size
changes. In such architecture, there is no need to regard pgsize when
making interim flush in iommu_iotlb_gather_add_page().

Add a "no_sync_on_pgsize_change" property for each IOMMU ops to decide
whether gather's pgsize is tracked and triggers a flush.

Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will@kernel.org>
Cc: Jiajun Cao <caojiajun@vmware.com>
Cc: iommu@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 drivers/iommu/amd/iommu.c | 1 +
 include/linux/iommu.h     | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 6723cbcf4030..8a71ad477c34 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2204,6 +2204,7 @@ const struct iommu_ops amd_iommu_ops = {
 	.put_resv_regions = generic_iommu_put_resv_regions,
 	.is_attach_deferred = amd_iommu_is_attach_deferred,
 	.pgsize_bitmap	= AMD_IOMMU_PGSIZES,
+	.ignore_gather_pgsize = true,
 	.flush_iotlb_all = amd_iommu_flush_iotlb_all,
 	.iotlb_sync = amd_iommu_iotlb_sync,
 	.def_domain_type = amd_iommu_def_domain_type,
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 32d448050bf7..1fb2695418e9 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -284,6 +284,7 @@ struct iommu_ops {
 	int (*def_domain_type)(struct device *dev);
 
 	unsigned long pgsize_bitmap;
+	bool ignore_gather_pgsize;
 	struct module *owner;
 };
 
@@ -508,7 +509,7 @@ static inline void iommu_iotlb_gather_add_page(struct iommu_domain *domain,
 	 * a different granularity, then sync the TLB so that the gather
 	 * structure can be rewritten.
 	 */
-	if (gather->pgsize != size ||
+	if ((gather->pgsize != size && !domain->ops->ignore_gather_pgsize) ||
 	    end + 1 < gather->start || start > gather->end + 1) {
 		if (gather->pgsize)
 			iommu_iotlb_sync(domain, gather);
-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 2/4] iommu/amd: Selective flush on unmap

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

Recent patch attempted to enable selective page flushes on AMD IOMMU but
neglected to adapt amd_iommu_iotlb_sync() to use the selective flushes.

Adapt amd_iommu_iotlb_sync() to use selective flushes and change
amd_iommu_unmap() to collect the flushes. As a defensive measure, to
avoid potential issues as those that the Intel IOMMU driver encountered
recently, flush the page-walk caches by always setting the "pde"
parameter. This can be removed later.

Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will@kernel.org>
Cc: Jiajun Cao <caojiajun@vmware.com>
Cc: iommu@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 drivers/iommu/amd/iommu.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 6723cbcf4030..b8cabbbeed71 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2057,12 +2057,17 @@ static size_t amd_iommu_unmap(struct iommu_domain *dom, unsigned long iova,
 {
 	struct protection_domain *domain = to_pdomain(dom);
 	struct io_pgtable_ops *ops = &domain->iop.iop.ops;
+	size_t r;
 
 	if ((amd_iommu_pgtable == AMD_IOMMU_V1) &&
 	    (domain->iop.mode == PAGE_MODE_NONE))
 		return 0;
 
-	return (ops->unmap) ? ops->unmap(ops, iova, page_size, gather) : 0;
+	r = (ops->unmap) ? ops->unmap(ops, iova, page_size, gather) : 0;
+
+	iommu_iotlb_gather_add_page(dom, gather, iova, page_size);
+
+	return r;
 }
 
 static phys_addr_t amd_iommu_iova_to_phys(struct iommu_domain *dom,
@@ -2165,7 +2170,13 @@ static void amd_iommu_flush_iotlb_all(struct iommu_domain *domain)
 static void amd_iommu_iotlb_sync(struct iommu_domain *domain,
 				 struct iommu_iotlb_gather *gather)
 {
-	amd_iommu_flush_iotlb_all(domain);
+	struct protection_domain *dom = to_pdomain(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_complete(dom);
+	spin_unlock_irqrestore(&dom->lock, flags);
 }
 
 static int amd_iommu_def_domain_type(struct device *dev)
-- 
2.25.1

[PATCH 2/4] iommu/amd: Selective flush on unmap

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

Recent patch attempted to enable selective page flushes on AMD IOMMU but
neglected to adapt amd_iommu_iotlb_sync() to use the selective flushes.

Adapt amd_iommu_iotlb_sync() to use selective flushes and change
amd_iommu_unmap() to collect the flushes. As a defensive measure, to
avoid potential issues as those that the Intel IOMMU driver encountered
recently, flush the page-walk caches by always setting the "pde"
parameter. This can be removed later.

Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will@kernel.org>
Cc: Jiajun Cao <caojiajun@vmware.com>
Cc: iommu@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 drivers/iommu/amd/iommu.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 6723cbcf4030..b8cabbbeed71 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2057,12 +2057,17 @@ static size_t amd_iommu_unmap(struct iommu_domain *dom, unsigned long iova,
 {
 	struct protection_domain *domain = to_pdomain(dom);
 	struct io_pgtable_ops *ops = &domain->iop.iop.ops;
+	size_t r;
 
 	if ((amd_iommu_pgtable == AMD_IOMMU_V1) &&
 	    (domain->iop.mode == PAGE_MODE_NONE))
 		return 0;
 
-	return (ops->unmap) ? ops->unmap(ops, iova, page_size, gather) : 0;
+	r = (ops->unmap) ? ops->unmap(ops, iova, page_size, gather) : 0;
+
+	iommu_iotlb_gather_add_page(dom, gather, iova, page_size);
+
+	return r;
 }
 
 static phys_addr_t amd_iommu_iova_to_phys(struct iommu_domain *dom,
@@ -2165,7 +2170,13 @@ static void amd_iommu_flush_iotlb_all(struct iommu_domain *domain)
 static void amd_iommu_iotlb_sync(struct iommu_domain *domain,
 				 struct iommu_iotlb_gather *gather)
 {
-	amd_iommu_flush_iotlb_all(domain);
+	struct protection_domain *dom = to_pdomain(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_complete(dom);
+	spin_unlock_irqrestore(&dom->lock, flags);
 }
 
 static int amd_iommu_def_domain_type(struct device *dev)
-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 3/4] iommu/amd: Do not sync on page size changes

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

Some IOMMU architectures perform invalidations regardless of the page
size. In such architectures there is no need to sync when the page size
changes or to regard pgsize when making interim flush in
iommu_iotlb_gather_add_page().

Add a "ignore_gather_pgsize" property for each IOMMU-ops to decide
whether gather's pgsize is tracked and triggers a flush.

Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will@kernel.org>
Cc: Jiajun Cao <caojiajun@vmware.com>
Cc: iommu@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 drivers/iommu/amd/iommu.c | 1 +
 include/linux/iommu.h     | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index b8cabbbeed71..1849b53f2149 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2215,6 +2215,7 @@ const struct iommu_ops amd_iommu_ops = {
 	.put_resv_regions = generic_iommu_put_resv_regions,
 	.is_attach_deferred = amd_iommu_is_attach_deferred,
 	.pgsize_bitmap	= AMD_IOMMU_PGSIZES,
+	.ignore_gather_pgsize = true,
 	.flush_iotlb_all = amd_iommu_flush_iotlb_all,
 	.iotlb_sync = amd_iommu_iotlb_sync,
 	.def_domain_type = amd_iommu_def_domain_type,
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 32d448050bf7..1fb2695418e9 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -284,6 +284,7 @@ struct iommu_ops {
 	int (*def_domain_type)(struct device *dev);
 
 	unsigned long pgsize_bitmap;
+	bool ignore_gather_pgsize;
 	struct module *owner;
 };
 
@@ -508,7 +509,7 @@ static inline void iommu_iotlb_gather_add_page(struct iommu_domain *domain,
 	 * a different granularity, then sync the TLB so that the gather
 	 * structure can be rewritten.
 	 */
-	if (gather->pgsize != size ||
+	if ((gather->pgsize != size && !domain->ops->ignore_gather_pgsize) ||
 	    end + 1 < gather->start || start > gather->end + 1) {
 		if (gather->pgsize)
 			iommu_iotlb_sync(domain, gather);
-- 
2.25.1

[PATCH 3/4] iommu/amd: Do not sync on page size changes

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

Some IOMMU architectures perform invalidations regardless of the page
size. In such architectures there is no need to sync when the page size
changes or to regard pgsize when making interim flush in
iommu_iotlb_gather_add_page().

Add a "ignore_gather_pgsize" property for each IOMMU-ops to decide
whether gather's pgsize is tracked and triggers a flush.

Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will@kernel.org>
Cc: Jiajun Cao <caojiajun@vmware.com>
Cc: iommu@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 drivers/iommu/amd/iommu.c | 1 +
 include/linux/iommu.h     | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index b8cabbbeed71..1849b53f2149 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2215,6 +2215,7 @@ const struct iommu_ops amd_iommu_ops = {
 	.put_resv_regions = generic_iommu_put_resv_regions,
 	.is_attach_deferred = amd_iommu_is_attach_deferred,
 	.pgsize_bitmap	= AMD_IOMMU_PGSIZES,
+	.ignore_gather_pgsize = true,
 	.flush_iotlb_all = amd_iommu_flush_iotlb_all,
 	.iotlb_sync = amd_iommu_iotlb_sync,
 	.def_domain_type = amd_iommu_def_domain_type,
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 32d448050bf7..1fb2695418e9 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -284,6 +284,7 @@ struct iommu_ops {
 	int (*def_domain_type)(struct device *dev);
 
 	unsigned long pgsize_bitmap;
+	bool ignore_gather_pgsize;
 	struct module *owner;
 };
 
@@ -508,7 +509,7 @@ static inline void iommu_iotlb_gather_add_page(struct iommu_domain *domain,
 	 * a different granularity, then sync the TLB so that the gather
 	 * structure can be rewritten.
 	 */
-	if (gather->pgsize != size ||
+	if ((gather->pgsize != size && !domain->ops->ignore_gather_pgsize) ||
 	    end + 1 < gather->start || start > gather->end + 1) {
 		if (gather->pgsize)
 			iommu_iotlb_sync(domain, gather);
-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 3/4] iommu/amd: Selective flush on unmap

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

Recent patch attempted to enable selective page flushes on AMD IOMMU but
neglected to adapt amd_iommu_iotlb_sync() to use the selective flushes.

Adapt amd_iommu_iotlb_sync() to use selective flushes and change
amd_iommu_unmap() to collect the flushes. As a defensive measure, to
avoid potential issues as those that the Intel IOMMU driver encountered
recently, flush the page-walk caches by always setting the "pde"
parameter. This can be removed later.

Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will@kernel.org>
Cc: Jiajun Cao <caojiajun@vmware.com>
Cc: iommu@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 drivers/iommu/amd/iommu.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 8a71ad477c34..1849b53f2149 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2057,12 +2057,17 @@ static size_t amd_iommu_unmap(struct iommu_domain *dom, unsigned long iova,
 {
 	struct protection_domain *domain = to_pdomain(dom);
 	struct io_pgtable_ops *ops = &domain->iop.iop.ops;
+	size_t r;
 
 	if ((amd_iommu_pgtable == AMD_IOMMU_V1) &&
 	    (domain->iop.mode == PAGE_MODE_NONE))
 		return 0;
 
-	return (ops->unmap) ? ops->unmap(ops, iova, page_size, gather) : 0;
+	r = (ops->unmap) ? ops->unmap(ops, iova, page_size, gather) : 0;
+
+	iommu_iotlb_gather_add_page(dom, gather, iova, page_size);
+
+	return r;
 }
 
 static phys_addr_t amd_iommu_iova_to_phys(struct iommu_domain *dom,
@@ -2165,7 +2170,13 @@ static void amd_iommu_flush_iotlb_all(struct iommu_domain *domain)
 static void amd_iommu_iotlb_sync(struct iommu_domain *domain,
 				 struct iommu_iotlb_gather *gather)
 {
-	amd_iommu_flush_iotlb_all(domain);
+	struct protection_domain *dom = to_pdomain(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_complete(dom);
+	spin_unlock_irqrestore(&dom->lock, flags);
 }
 
 static int amd_iommu_def_domain_type(struct device *dev)
-- 
2.25.1

[PATCH 3/4] iommu/amd: Selective flush on unmap

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

Recent patch attempted to enable selective page flushes on AMD IOMMU but
neglected to adapt amd_iommu_iotlb_sync() to use the selective flushes.

Adapt amd_iommu_iotlb_sync() to use selective flushes and change
amd_iommu_unmap() to collect the flushes. As a defensive measure, to
avoid potential issues as those that the Intel IOMMU driver encountered
recently, flush the page-walk caches by always setting the "pde"
parameter. This can be removed later.

Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will@kernel.org>
Cc: Jiajun Cao <caojiajun@vmware.com>
Cc: iommu@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 drivers/iommu/amd/iommu.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 8a71ad477c34..1849b53f2149 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2057,12 +2057,17 @@ static size_t amd_iommu_unmap(struct iommu_domain *dom, unsigned long iova,
 {
 	struct protection_domain *domain = to_pdomain(dom);
 	struct io_pgtable_ops *ops = &domain->iop.iop.ops;
+	size_t r;
 
 	if ((amd_iommu_pgtable == AMD_IOMMU_V1) &&
 	    (domain->iop.mode == PAGE_MODE_NONE))
 		return 0;
 
-	return (ops->unmap) ? ops->unmap(ops, iova, page_size, gather) : 0;
+	r = (ops->unmap) ? ops->unmap(ops, iova, page_size, gather) : 0;
+
+	iommu_iotlb_gather_add_page(dom, gather, iova, page_size);
+
+	return r;
 }
 
 static phys_addr_t amd_iommu_iova_to_phys(struct iommu_domain *dom,
@@ -2165,7 +2170,13 @@ static void amd_iommu_flush_iotlb_all(struct iommu_domain *domain)
 static void amd_iommu_iotlb_sync(struct iommu_domain *domain,
 				 struct iommu_iotlb_gather *gather)
 {
-	amd_iommu_flush_iotlb_all(domain);
+	struct protection_domain *dom = to_pdomain(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_complete(dom);
+	spin_unlock_irqrestore(&dom->lock, flags);
 }
 
 static int amd_iommu_def_domain_type(struct device *dev)
-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 4/4] iommu/amd: Do not use flush-queue when NpCache is on

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

Do not use flush-queue on virtualized environments, where the NpCache
capability of the IOMMU is set. This is required to reduce
virtualization overheads.

This change follows a similar change to Intel's VT-d and a detailed
explanation as for the rationale is described in commit 29b32839725f
("iommu/vt-d: Do not use flush-queue when caching-mode is on").

Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will@kernel.org>
Cc: Jiajun Cao <caojiajun@vmware.com>
Cc: iommu@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 drivers/iommu/amd/init.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index d006724f4dc2..ba3b76ed776d 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1850,8 +1850,13 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
 	if (ret)
 		return ret;
 
-	if (iommu->cap & (1UL << IOMMU_CAP_NPCACHE))
+	if (iommu->cap & (1UL << IOMMU_CAP_NPCACHE)) {
+		if (!amd_iommu_unmap_flush)
+			pr_warn_once("IOMMU batching is disabled due to virtualization");
+
 		amd_iommu_np_cache = true;
+		amd_iommu_unmap_flush = true;
+	}
 
 	init_iommu_perf_ctr(iommu);
 
-- 
2.25.1

[PATCH 4/4] iommu/amd: Do not use flush-queue when NpCache is on

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

Do not use flush-queue on virtualized environments, where the NpCache
capability of the IOMMU is set. This is required to reduce
virtualization overheads.

This change follows a similar change to Intel's VT-d and a detailed
explanation as for the rationale is described in commit 29b32839725f
("iommu/vt-d: Do not use flush-queue when caching-mode is on").

Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will@kernel.org>
Cc: Jiajun Cao <caojiajun@vmware.com>
Cc: iommu@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 drivers/iommu/amd/init.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index d006724f4dc2..ba3b76ed776d 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1850,8 +1850,13 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
 	if (ret)
 		return ret;
 
-	if (iommu->cap & (1UL << IOMMU_CAP_NPCACHE))
+	if (iommu->cap & (1UL << IOMMU_CAP_NPCACHE)) {
+		if (!amd_iommu_unmap_flush)
+			pr_warn_once("IOMMU batching is disabled due to virtualization");
+
 		amd_iommu_np_cache = true;
+		amd_iommu_unmap_flush = true;
+	}
 
 	init_iommu_perf_ctr(iommu);
 
-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/4] iommu/amd: Fix wrong parentheses on page-specific invalidations

[Joerg Roedel]

On Sat, May 01, 2021 at 11:59:56PM -0700, Nadav Amit wrote:
> From: Nadav Amit <namit@vmware.com>
> 
> The logic to determine the mask of page-specific invalidations was
> tested in userspace. As the code was copied into the kernel, the
> parentheses were mistakenly set in the wrong place, resulting in the
> wrong mask.
> 
> Fix it.
> 
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Will Deacon <will@kernel.org>
> Cc: Jiajun Cao <caojiajun@vmware.com>
> Cc: iommu@lists.linux-foundation.org
> Cc: linux-kernel@vger.kernel.org
> Fixes: 268aa4548277 ("iommu/amd: Page-specific invalidations for more than one page")
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
>  drivers/iommu/amd/iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied this one for v5.13, thanks Nadav.

Somehow the rest of the patch-set got screwed up during sending or so,
at least I see some patches twice in my inbox and with different
subjects.

Can you please re-send patches 2-4 when -rc3 it out?

Thanks,

	Joerg

Re: [PATCH 1/4] iommu/amd: Fix wrong parentheses on page-specific invalidations

[Joerg Roedel]

On Sat, May 01, 2021 at 11:59:56PM -0700, Nadav Amit wrote:
> From: Nadav Amit <namit@vmware.com>
> 
> The logic to determine the mask of page-specific invalidations was
> tested in userspace. As the code was copied into the kernel, the
> parentheses were mistakenly set in the wrong place, resulting in the
> wrong mask.
> 
> Fix it.
> 
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Will Deacon <will@kernel.org>
> Cc: Jiajun Cao <caojiajun@vmware.com>
> Cc: iommu@lists.linux-foundation.org
> Cc: linux-kernel@vger.kernel.org
> Fixes: 268aa4548277 ("iommu/amd: Page-specific invalidations for more than one page")
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
>  drivers/iommu/amd/iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied this one for v5.13, thanks Nadav.

Somehow the rest of the patch-set got screwed up during sending or so,
at least I see some patches twice in my inbox and with different
subjects.

Can you please re-send patches 2-4 when -rc3 it out?

Thanks,

	Joerg
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/4] iommu/amd: Fix wrong parentheses on page-specific invalidations

[Nadav Amit]

[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]



> On May 18, 2021, at 2:23 AM, Joerg Roedel <joro@8bytes.org> wrote:
> 
> On Sat, May 01, 2021 at 11:59:56PM -0700, Nadav Amit wrote:
>> From: Nadav Amit <namit@vmware.com>
>> 
>> The logic to determine the mask of page-specific invalidations was
>> tested in userspace. As the code was copied into the kernel, the
>> parentheses were mistakenly set in the wrong place, resulting in the
>> wrong mask.
>> 
>> Fix it.
>> 
>> Cc: Joerg Roedel <joro@8bytes.org>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Jiajun Cao <caojiajun@vmware.com>
>> Cc: iommu@lists.linux-foundation.org
>> Cc: linux-kernel@vger.kernel.org
>> Fixes: 268aa4548277 ("iommu/amd: Page-specific invalidations for more than one page")
>> Signed-off-by: Nadav Amit <namit@vmware.com>
>> ---
>> drivers/iommu/amd/iommu.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Applied this one for v5.13, thanks Nadav.
> 
> Somehow the rest of the patch-set got screwed up during sending or so,
> at least I see some patches twice in my inbox and with different
> subjects.
> 
> Can you please re-send patches 2-4 when -rc3 it out?

Joerg,

Thanks for your understanding. I sent a version based on -rc3 a week
ago.

I noticed that there was some confusion regarding rc numbers. Do you
need a new version based on rc4 or can you apply the version I sent?

Thanks,
Nadav


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/4] iommu/amd: Fix wrong parentheses on page-specific invalidations

[Nadav Amit]

[-- Attachment #1.1: Type: text/plain, Size: 1386 bytes --]



> On May 18, 2021, at 2:23 AM, Joerg Roedel <joro@8bytes.org> wrote:
> 
> On Sat, May 01, 2021 at 11:59:56PM -0700, Nadav Amit wrote:
>> From: Nadav Amit <namit@vmware.com>
>> 
>> The logic to determine the mask of page-specific invalidations was
>> tested in userspace. As the code was copied into the kernel, the
>> parentheses were mistakenly set in the wrong place, resulting in the
>> wrong mask.
>> 
>> Fix it.
>> 
>> Cc: Joerg Roedel <joro@8bytes.org>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Jiajun Cao <caojiajun@vmware.com>
>> Cc: iommu@lists.linux-foundation.org
>> Cc: linux-kernel@vger.kernel.org
>> Fixes: 268aa4548277 ("iommu/amd: Page-specific invalidations for more than one page")
>> Signed-off-by: Nadav Amit <namit@vmware.com>
>> ---
>> drivers/iommu/amd/iommu.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Applied this one for v5.13, thanks Nadav.
> 
> Somehow the rest of the patch-set got screwed up during sending or so,
> at least I see some patches twice in my inbox and with different
> subjects.
> 
> Can you please re-send patches 2-4 when -rc3 it out?

Joerg,

Thanks for your understanding. I sent a version based on -rc3 a week
ago.

I noticed that there was some confusion regarding rc numbers. Do you
need a new version based on rc4 or can you apply the version I sent?

Thanks,
Nadav


[-- Attachment #1.2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] iommu/amd: Do not sync on page size changes

[Robin Murphy]

On 2021-05-02 07:59, Nadav Amit wrote:
> From: Nadav Amit <namit@vmware.com>
> 
> Some IOMMU architectures perform invalidations regardless of the page
> size. In such architectures there is no need to sync when the page size
> changes or to regard pgsize when making interim flush in
> iommu_iotlb_gather_add_page().
> 
> Add a "ignore_gather_pgsize" property for each IOMMU-ops to decide
> whether gather's pgsize is tracked and triggers a flush.

I've objected before[1], and I'll readily object again ;)

I still think it's very silly to add a bunch of indirection all over the 
place to make a helper function not do the main thing it's intended to 
help with. If you only need trivial address gathering then it's far 
simpler to just implement trivial address gathering. I suppose if you 
really want to you could factor out another helper to share the 5 lines 
of code which ended up in mtk-iommu (see commit f21ae3b10084).

Robin.

[1] 
https://lore.kernel.org/linux-iommu/49bae447-d662-e6cf-7500-ab78e3b75dc4@arm.com/

> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Will Deacon <will@kernel.org>
> Cc: Jiajun Cao <caojiajun@vmware.com>
> Cc: iommu@lists.linux-foundation.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
>   drivers/iommu/amd/iommu.c | 1 +
>   include/linux/iommu.h     | 3 ++-
>   2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index b8cabbbeed71..1849b53f2149 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2215,6 +2215,7 @@ const struct iommu_ops amd_iommu_ops = {
>   	.put_resv_regions = generic_iommu_put_resv_regions,
>   	.is_attach_deferred = amd_iommu_is_attach_deferred,
>   	.pgsize_bitmap	= AMD_IOMMU_PGSIZES,
> +	.ignore_gather_pgsize = true,
>   	.flush_iotlb_all = amd_iommu_flush_iotlb_all,
>   	.iotlb_sync = amd_iommu_iotlb_sync,
>   	.def_domain_type = amd_iommu_def_domain_type,
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 32d448050bf7..1fb2695418e9 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -284,6 +284,7 @@ struct iommu_ops {
>   	int (*def_domain_type)(struct device *dev);
>   
>   	unsigned long pgsize_bitmap;
> +	bool ignore_gather_pgsize;
>   	struct module *owner;
>   };
>   
> @@ -508,7 +509,7 @@ static inline void iommu_iotlb_gather_add_page(struct iommu_domain *domain,
>   	 * a different granularity, then sync the TLB so that the gather
>   	 * structure can be rewritten.
>   	 */
> -	if (gather->pgsize != size ||
> +	if ((gather->pgsize != size && !domain->ops->ignore_gather_pgsize) ||
>   	    end + 1 < gather->start || start > gather->end + 1) {
>   		if (gather->pgsize)
>   			iommu_iotlb_sync(domain, gather);
> 

Re: [PATCH 3/4] iommu/amd: Do not sync on page size changes

[Robin Murphy]

On 2021-05-02 07:59, Nadav Amit wrote:
> From: Nadav Amit <namit@vmware.com>
> 
> Some IOMMU architectures perform invalidations regardless of the page
> size. In such architectures there is no need to sync when the page size
> changes or to regard pgsize when making interim flush in
> iommu_iotlb_gather_add_page().
> 
> Add a "ignore_gather_pgsize" property for each IOMMU-ops to decide
> whether gather's pgsize is tracked and triggers a flush.

I've objected before[1], and I'll readily object again ;)

I still think it's very silly to add a bunch of indirection all over the 
place to make a helper function not do the main thing it's intended to 
help with. If you only need trivial address gathering then it's far 
simpler to just implement trivial address gathering. I suppose if you 
really want to you could factor out another helper to share the 5 lines 
of code which ended up in mtk-iommu (see commit f21ae3b10084).

Robin.

[1] 
https://lore.kernel.org/linux-iommu/49bae447-d662-e6cf-7500-ab78e3b75dc4@arm.com/

> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Will Deacon <will@kernel.org>
> Cc: Jiajun Cao <caojiajun@vmware.com>
> Cc: iommu@lists.linux-foundation.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
>   drivers/iommu/amd/iommu.c | 1 +
>   include/linux/iommu.h     | 3 ++-
>   2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index b8cabbbeed71..1849b53f2149 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2215,6 +2215,7 @@ const struct iommu_ops amd_iommu_ops = {
>   	.put_resv_regions = generic_iommu_put_resv_regions,
>   	.is_attach_deferred = amd_iommu_is_attach_deferred,
>   	.pgsize_bitmap	= AMD_IOMMU_PGSIZES,
> +	.ignore_gather_pgsize = true,
>   	.flush_iotlb_all = amd_iommu_flush_iotlb_all,
>   	.iotlb_sync = amd_iommu_iotlb_sync,
>   	.def_domain_type = amd_iommu_def_domain_type,
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 32d448050bf7..1fb2695418e9 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -284,6 +284,7 @@ struct iommu_ops {
>   	int (*def_domain_type)(struct device *dev);
>   
>   	unsigned long pgsize_bitmap;
> +	bool ignore_gather_pgsize;
>   	struct module *owner;
>   };
>   
> @@ -508,7 +509,7 @@ static inline void iommu_iotlb_gather_add_page(struct iommu_domain *domain,
>   	 * a different granularity, then sync the TLB so that the gather
>   	 * structure can be rewritten.
>   	 */
> -	if (gather->pgsize != size ||
> +	if ((gather->pgsize != size && !domain->ops->ignore_gather_pgsize) ||
>   	    end + 1 < gather->start || start > gather->end + 1) {
>   		if (gather->pgsize)
>   			iommu_iotlb_sync(domain, gather);
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] iommu/amd: Do not sync on page size changes

[Nadav Amit]

> On Jun 1, 2021, at 8:59 AM, Robin Murphy <robin.murphy@arm.com> wrote:
> 
> On 2021-05-02 07:59, Nadav Amit wrote:
>> From: Nadav Amit <namit@vmware.com>
>> Some IOMMU architectures perform invalidations regardless of the page
>> size. In such architectures there is no need to sync when the page size
>> changes or to regard pgsize when making interim flush in
>> iommu_iotlb_gather_add_page().
>> Add a "ignore_gather_pgsize" property for each IOMMU-ops to decide
>> whether gather's pgsize is tracked and triggers a flush.
> 
> I've objected before[1], and I'll readily object again ;)
> 
> I still think it's very silly to add a bunch of indirection all over the place to make a helper function not do the main thing it's intended to help with. If you only need trivial address gathering then it's far simpler to just implement trivial address gathering. I suppose if you really want to you could factor out another helper to share the 5 lines of code which ended up in mtk-iommu (see commit f21ae3b10084).

Thanks, Robin.

I read your comments but I cannot fully understand the alternative that you propose, although I do understand your objection to the indirection “ignore_gather_pgsize”. Would it be ok if “ignore_gather_pgsize" was provided as an argument for iommu_iotlb_gather_add_page()?

In general, I can live without this patch. It probably would have negligent impact on performance anyhow.

Regards,
Nadav

Re: [PATCH 3/4] iommu/amd: Do not sync on page size changes

[Nadav Amit]

> On Jun 1, 2021, at 8:59 AM, Robin Murphy <robin.murphy@arm.com> wrote:
> 
> On 2021-05-02 07:59, Nadav Amit wrote:
>> From: Nadav Amit <namit@vmware.com>
>> Some IOMMU architectures perform invalidations regardless of the page
>> size. In such architectures there is no need to sync when the page size
>> changes or to regard pgsize when making interim flush in
>> iommu_iotlb_gather_add_page().
>> Add a "ignore_gather_pgsize" property for each IOMMU-ops to decide
>> whether gather's pgsize is tracked and triggers a flush.
> 
> I've objected before[1], and I'll readily object again ;)
> 
> I still think it's very silly to add a bunch of indirection all over the place to make a helper function not do the main thing it's intended to help with. If you only need trivial address gathering then it's far simpler to just implement trivial address gathering. I suppose if you really want to you could factor out another helper to share the 5 lines of code which ended up in mtk-iommu (see commit f21ae3b10084).

Thanks, Robin.

I read your comments but I cannot fully understand the alternative that you propose, although I do understand your objection to the indirection “ignore_gather_pgsize”. Would it be ok if “ignore_gather_pgsize" was provided as an argument for iommu_iotlb_gather_add_page()?

In general, I can live without this patch. It probably would have negligent impact on performance anyhow.

Regards,
Nadav


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] iommu/amd: Do not sync on page size changes

[Robin Murphy]

On 2021-06-01 17:39, Nadav Amit wrote:
> 
> 
>> On Jun 1, 2021, at 8:59 AM, Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2021-05-02 07:59, Nadav Amit wrote:
>>> From: Nadav Amit <namit@vmware.com>
>>> Some IOMMU architectures perform invalidations regardless of the page
>>> size. In such architectures there is no need to sync when the page size
>>> changes or to regard pgsize when making interim flush in
>>> iommu_iotlb_gather_add_page().
>>> Add a "ignore_gather_pgsize" property for each IOMMU-ops to decide
>>> whether gather's pgsize is tracked and triggers a flush.
>>
>> I've objected before[1], and I'll readily object again ;)
>>
>> I still think it's very silly to add a bunch of indirection all over the place to make a helper function not do the main thing it's intended to help with. If you only need trivial address gathering then it's far simpler to just implement trivial address gathering. I suppose if you really want to you could factor out another helper to share the 5 lines of code which ended up in mtk-iommu (see commit f21ae3b10084).
> 
> Thanks, Robin.
> 
> I read your comments but I cannot fully understand the alternative that you propose, although I do understand your objection to the indirection “ignore_gather_pgsize”. Would it be ok if “ignore_gather_pgsize" was provided as an argument for iommu_iotlb_gather_add_page()?

No, I mean if iommu_iotlb_gather_add_page() doesn't have the behaviour 
your driver wants, just don't call it. Write or factor out a suitable 
helper that *does* do what you want and call that, or just implement the 
logic directly inline. Indirect argument or not, it just doesn't make 
much sense to have a helper function call which says "do this except 
don't do most of it".

> In general, I can live without this patch. It probably would have negligent impact on performance anyhow.

As I implied, it sounds like your needs are the same as the Mediatek 
driver had, so you could readily factor out a new page-size-agnostic 
gather helper from that. I fully support making the functional change to 
amd-iommu *somehow* - nobody likes unnecessary syncs - just not with 
this particular implementation :)

Robin.

Re: [PATCH 3/4] iommu/amd: Do not sync on page size changes

[Robin Murphy]

On 2021-06-01 17:39, Nadav Amit wrote:
> 
> 
>> On Jun 1, 2021, at 8:59 AM, Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2021-05-02 07:59, Nadav Amit wrote:
>>> From: Nadav Amit <namit@vmware.com>
>>> Some IOMMU architectures perform invalidations regardless of the page
>>> size. In such architectures there is no need to sync when the page size
>>> changes or to regard pgsize when making interim flush in
>>> iommu_iotlb_gather_add_page().
>>> Add a "ignore_gather_pgsize" property for each IOMMU-ops to decide
>>> whether gather's pgsize is tracked and triggers a flush.
>>
>> I've objected before[1], and I'll readily object again ;)
>>
>> I still think it's very silly to add a bunch of indirection all over the place to make a helper function not do the main thing it's intended to help with. If you only need trivial address gathering then it's far simpler to just implement trivial address gathering. I suppose if you really want to you could factor out another helper to share the 5 lines of code which ended up in mtk-iommu (see commit f21ae3b10084).
> 
> Thanks, Robin.
> 
> I read your comments but I cannot fully understand the alternative that you propose, although I do understand your objection to the indirection “ignore_gather_pgsize”. Would it be ok if “ignore_gather_pgsize" was provided as an argument for iommu_iotlb_gather_add_page()?

No, I mean if iommu_iotlb_gather_add_page() doesn't have the behaviour 
your driver wants, just don't call it. Write or factor out a suitable 
helper that *does* do what you want and call that, or just implement the 
logic directly inline. Indirect argument or not, it just doesn't make 
much sense to have a helper function call which says "do this except 
don't do most of it".

> In general, I can live without this patch. It probably would have negligent impact on performance anyhow.

As I implied, it sounds like your needs are the same as the Mediatek 
driver had, so you could readily factor out a new page-size-agnostic 
gather helper from that. I fully support making the functional change to 
amd-iommu *somehow* - nobody likes unnecessary syncs - just not with 
this particular implementation :)

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] iommu/amd: Do not sync on page size changes

[Nadav Amit]

[-- Attachment #1: Type: text/plain, Size: 5774 bytes --]



> On Jun 1, 2021, at 10:27 AM, Robin Murphy <robin.murphy@arm.com> wrote:
> 
> On 2021-06-01 17:39, Nadav Amit wrote:
>>> On Jun 1, 2021, at 8:59 AM, Robin Murphy <robin.murphy@arm.com> wrote:
>>> 
>>> On 2021-05-02 07:59, Nadav Amit wrote:
>>>> From: Nadav Amit <namit@vmware.com>
>>>> Some IOMMU architectures perform invalidations regardless of the page
>>>> size. In such architectures there is no need to sync when the page size
>>>> changes or to regard pgsize when making interim flush in
>>>> iommu_iotlb_gather_add_page().
>>>> Add a "ignore_gather_pgsize" property for each IOMMU-ops to decide
>>>> whether gather's pgsize is tracked and triggers a flush.
>>> 
>>> I've objected before[1], and I'll readily object again ;)
>>> 
>>> I still think it's very silly to add a bunch of indirection all over the place to make a helper function not do the main thing it's intended to help with. If you only need trivial address gathering then it's far simpler to just implement trivial address gathering. I suppose if you really want to you could factor out another helper to share the 5 lines of code which ended up in mtk-iommu (see commit f21ae3b10084).
>> Thanks, Robin.
>> I read your comments but I cannot fully understand the alternative that you propose, although I do understand your objection to the indirection “ignore_gather_pgsize”. Would it be ok if “ignore_gather_pgsize" was provided as an argument for iommu_iotlb_gather_add_page()?
> 
> No, I mean if iommu_iotlb_gather_add_page() doesn't have the behaviour your driver wants, just don't call it. Write or factor out a suitable helper that *does* do what you want and call that, or just implement the logic directly inline. Indirect argument or not, it just doesn't make much sense to have a helper function call which says "do this except don't do most of it".
> 
>> In general, I can live without this patch. It probably would have negligent impact on performance anyhow.
> 
> As I implied, it sounds like your needs are the same as the Mediatek driver had, so you could readily factor out a new page-size-agnostic gather helper from that. I fully support making the functional change to amd-iommu *somehow* - nobody likes unnecessary syncs - just not with this particular implementation :)

Hm… avoid code duplication I need to extract some common code to another function.

Is the following resembles what you had in mind (untested)?

-- >8 --

Subject: [PATCH] iommu: add iommu_iotlb_gather_add_page_ignore_pgsize()

---
 drivers/iommu/mtk_iommu.c |  7 ++---
 include/linux/iommu.h     | 55 ++++++++++++++++++++++++++++++---------
 2 files changed, 44 insertions(+), 18 deletions(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index e168a682806a..5890e745bed3 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -520,12 +520,9 @@ static size_t mtk_iommu_unmap(struct iommu_domain *domain,
 			      struct iommu_iotlb_gather *gather)
 {
 	struct mtk_iommu_domain *dom = to_mtk_domain(domain);
-	unsigned long end = iova + size - 1;

-	if (gather->start > iova)
-		gather->start = iova;
-	if (gather->end < end)
-		gather->end = end;
+	iommu_iotlb_gather_update_range(gather, iova, size);
+
 	return dom->iop->unmap(dom->iop, iova, size, gather);
 }

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 9ca6e6b8084d..037434b6eb4c 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -535,29 +535,58 @@ static inline void iommu_iotlb_sync(struct iommu_domain *domain,
 	iommu_iotlb_gather_init(iotlb_gather);
 }

-static inline void iommu_iotlb_gather_add_page(struct iommu_domain *domain,
+static inline
+void iommu_iotlb_gather_update_range(struct iommu_iotlb_gather *gather,
+				     unsigned long iova, size_t size)
+{
+	unsigned long start = iova, end = start + size - 1;
+
+	if (gather->end < end)
+		gather->end = end;
+
+	if (gather->start > start)
+		gather->start = start;
+
+	gather->pgsize = size;
+}
+
+static inline
+bool iommu_iotlb_gather_is_disjoint(struct iommu_iotlb_gather *gather,
+				    unsigned long iova, size_t size)
+{
+	return iova + size < gather->start || iova > gather->end + 1;
+}
+
+static inline
+void iommu_iotlb_gather_add_page_ignore_pgsize(struct iommu_domain *domain,
 					       struct iommu_iotlb_gather *gather,
 					       unsigned long iova, size_t size)
 {
-	unsigned long start = iova, end = start + size - 1;
+	/*
+	 * Only if the new page is disjoint from the current range, then sync
+	 * the TLB so that the gather structure can be rewritten.
+	 */
+	if (iommu_iotlb_gather_is_disjoint(gather, iova, size) && gather->pgsize)
+		iommu_iotlb_sync(domain, gather);
+
+	iommu_iotlb_gather_update_range(gather, iova, size);
+}

+static inline void iommu_iotlb_gather_add_page(struct iommu_domain *domain,
+					       struct iommu_iotlb_gather *gather,
+					       unsigned long iova, size_t size)
+{
 	/*
 	 * If the new page is disjoint from the current range or is mapped at
 	 * a different granularity, then sync the TLB so that the gather
 	 * structure can be rewritten.
 	 */
-	if (gather->pgsize != size ||
-	    end + 1 < gather->start || start > gather->end + 1) {
-		if (gather->pgsize)
-			iommu_iotlb_sync(domain, gather);
-		gather->pgsize = size;
-	}
-
-	if (gather->end < end)
-		gather->end = end;
+	if ((gather->pgsize != size ||
+	     iommu_iotlb_gather_is_disjoint(gather, iova, size)) &&
+	    gather->pgsize)
+		iommu_iotlb_sync(domain, gather);

-	if (gather->start > start)
-		gather->start = start;
+	iommu_iotlb_gather_update_range(gather, iova, size);
 }

 /* PCI device grouping function */
--
2.25.1

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/4] iommu/amd: Do not sync on page size changes

[Nadav Amit]

[-- Attachment #1.1: Type: text/plain, Size: 5774 bytes --]



> On Jun 1, 2021, at 10:27 AM, Robin Murphy <robin.murphy@arm.com> wrote:
> 
> On 2021-06-01 17:39, Nadav Amit wrote:
>>> On Jun 1, 2021, at 8:59 AM, Robin Murphy <robin.murphy@arm.com> wrote:
>>> 
>>> On 2021-05-02 07:59, Nadav Amit wrote:
>>>> From: Nadav Amit <namit@vmware.com>
>>>> Some IOMMU architectures perform invalidations regardless of the page
>>>> size. In such architectures there is no need to sync when the page size
>>>> changes or to regard pgsize when making interim flush in
>>>> iommu_iotlb_gather_add_page().
>>>> Add a "ignore_gather_pgsize" property for each IOMMU-ops to decide
>>>> whether gather's pgsize is tracked and triggers a flush.
>>> 
>>> I've objected before[1], and I'll readily object again ;)
>>> 
>>> I still think it's very silly to add a bunch of indirection all over the place to make a helper function not do the main thing it's intended to help with. If you only need trivial address gathering then it's far simpler to just implement trivial address gathering. I suppose if you really want to you could factor out another helper to share the 5 lines of code which ended up in mtk-iommu (see commit f21ae3b10084).
>> Thanks, Robin.
>> I read your comments but I cannot fully understand the alternative that you propose, although I do understand your objection to the indirection “ignore_gather_pgsize”. Would it be ok if “ignore_gather_pgsize" was provided as an argument for iommu_iotlb_gather_add_page()?
> 
> No, I mean if iommu_iotlb_gather_add_page() doesn't have the behaviour your driver wants, just don't call it. Write or factor out a suitable helper that *does* do what you want and call that, or just implement the logic directly inline. Indirect argument or not, it just doesn't make much sense to have a helper function call which says "do this except don't do most of it".
> 
>> In general, I can live without this patch. It probably would have negligent impact on performance anyhow.
> 
> As I implied, it sounds like your needs are the same as the Mediatek driver had, so you could readily factor out a new page-size-agnostic gather helper from that. I fully support making the functional change to amd-iommu *somehow* - nobody likes unnecessary syncs - just not with this particular implementation :)

Hm… avoid code duplication I need to extract some common code to another function.

Is the following resembles what you had in mind (untested)?

-- >8 --

Subject: [PATCH] iommu: add iommu_iotlb_gather_add_page_ignore_pgsize()

---
 drivers/iommu/mtk_iommu.c |  7 ++---
 include/linux/iommu.h     | 55 ++++++++++++++++++++++++++++++---------
 2 files changed, 44 insertions(+), 18 deletions(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index e168a682806a..5890e745bed3 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -520,12 +520,9 @@ static size_t mtk_iommu_unmap(struct iommu_domain *domain,
 			      struct iommu_iotlb_gather *gather)
 {
 	struct mtk_iommu_domain *dom = to_mtk_domain(domain);
-	unsigned long end = iova + size - 1;

-	if (gather->start > iova)
-		gather->start = iova;
-	if (gather->end < end)
-		gather->end = end;
+	iommu_iotlb_gather_update_range(gather, iova, size);
+
 	return dom->iop->unmap(dom->iop, iova, size, gather);
 }

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 9ca6e6b8084d..037434b6eb4c 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -535,29 +535,58 @@ static inline void iommu_iotlb_sync(struct iommu_domain *domain,
 	iommu_iotlb_gather_init(iotlb_gather);
 }

-static inline void iommu_iotlb_gather_add_page(struct iommu_domain *domain,
+static inline
+void iommu_iotlb_gather_update_range(struct iommu_iotlb_gather *gather,
+				     unsigned long iova, size_t size)
+{
+	unsigned long start = iova, end = start + size - 1;
+
+	if (gather->end < end)
+		gather->end = end;
+
+	if (gather->start > start)
+		gather->start = start;
+
+	gather->pgsize = size;
+}
+
+static inline
+bool iommu_iotlb_gather_is_disjoint(struct iommu_iotlb_gather *gather,
+				    unsigned long iova, size_t size)
+{
+	return iova + size < gather->start || iova > gather->end + 1;
+}
+
+static inline
+void iommu_iotlb_gather_add_page_ignore_pgsize(struct iommu_domain *domain,
 					       struct iommu_iotlb_gather *gather,
 					       unsigned long iova, size_t size)
 {
-	unsigned long start = iova, end = start + size - 1;
+	/*
+	 * Only if the new page is disjoint from the current range, then sync
+	 * the TLB so that the gather structure can be rewritten.
+	 */
+	if (iommu_iotlb_gather_is_disjoint(gather, iova, size) && gather->pgsize)
+		iommu_iotlb_sync(domain, gather);
+
+	iommu_iotlb_gather_update_range(gather, iova, size);
+}

+static inline void iommu_iotlb_gather_add_page(struct iommu_domain *domain,
+					       struct iommu_iotlb_gather *gather,
+					       unsigned long iova, size_t size)
+{
 	/*
 	 * If the new page is disjoint from the current range or is mapped at
 	 * a different granularity, then sync the TLB so that the gather
 	 * structure can be rewritten.
 	 */
-	if (gather->pgsize != size ||
-	    end + 1 < gather->start || start > gather->end + 1) {
-		if (gather->pgsize)
-			iommu_iotlb_sync(domain, gather);
-		gather->pgsize = size;
-	}
-
-	if (gather->end < end)
-		gather->end = end;
+	if ((gather->pgsize != size ||
+	     iommu_iotlb_gather_is_disjoint(gather, iova, size)) &&
+	    gather->pgsize)
+		iommu_iotlb_sync(domain, gather);

-	if (gather->start > start)
-		gather->start = start;
+	iommu_iotlb_gather_update_range(gather, iova, size);
 }

 /* PCI device grouping function */
--
2.25.1

[-- Attachment #1.2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu