iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 0/6] iommu_pgsize() improvements to help towards ->[un]map_pages()
@ 2021-04-01 16:47 Will Deacon
  2021-04-01 16:47 ` [RFC PATCH 1/6] iommu/io-pgtable: Introduce unmap_pages() as a page table op Will Deacon
                   ` (5 more replies)
  0 siblings, 6 replies; 9+ messages in thread
From: Will Deacon @ 2021-04-01 16:47 UTC (permalink / raw)
  To: iommu
  Cc: Isaac J. Manjarres, Will Deacon, Robin Murphy, Pratik Patel,
	linux-arm-kernel

Hi Isaac,

I had a go at removing the loop you have in pgsize_bitmap() over at:

https://lore.kernel.org/r/20210331030042.13348-4-isaacm@codeaurora.org

and I ended up with this. It's _very_ lightly tested, but I thought it
might be useful to you, especially if you're going to be adding support
for '->map_pages' as well.

Cheers,

Will

Cc: "Isaac J. Manjarres" <isaacm@codeaurora.org>
Cc: Pratik Patel <pratikp@codeaurora.org>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Lu Baolu <baolu.lu@linux.intel.com>

--->8

Isaac J. Manjarres (2):
  iommu/io-pgtable: Introduce unmap_pages() as a page table op
  iommu: Add an unmap_pages() op for IOMMU drivers

Will Deacon (4):
  iommu: Use bitmap to calculate page size in iommu_pgsize()
  iommu: Split 'addr_merge' argument to iommu_pgsize() into separate
    parts
  iommu: Hook up '->unmap_pages' driver callback
  iommu: Accomodate larger pages in iommu_pgsize() 'count' calculation

 drivers/iommu/iommu.c      | 87 +++++++++++++++++++++++++++-----------
 include/linux/io-pgtable.h |  4 ++
 include/linux/iommu.h      |  4 ++
 3 files changed, 70 insertions(+), 25 deletions(-)

-- 
2.31.0.291.g576ba9dcdaf-goog

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [RFC PATCH 1/6] iommu/io-pgtable: Introduce unmap_pages() as a page table op
  2021-04-01 16:47 [RFC PATCH 0/6] iommu_pgsize() improvements to help towards ->[un]map_pages() Will Deacon
@ 2021-04-01 16:47 ` Will Deacon
  2021-04-01 16:47 ` [RFC PATCH 2/6] iommu: Add an unmap_pages() op for IOMMU drivers Will Deacon
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: Will Deacon @ 2021-04-01 16:47 UTC (permalink / raw)
  To: iommu
  Cc: Isaac J. Manjarres, Will Deacon, Robin Murphy, Pratik Patel,
	linux-arm-kernel

From: "Isaac J. Manjarres" <isaacm@codeaurora.org>

The io-pgtable code expects to operate on a single block or
granule of memory that is supported by the IOMMU hardware when
unmapping memory.

This means that when a large buffer that consists of multiple
such blocks is unmapped, the io-pgtable code will walk the page
tables to the correct level to unmap each block, even for blocks
that are virtually contiguous and at the same level, which can
incur an overhead in performance.

Introduce the unmap_pages() page table op to express to the
io-pgtable code that it should unmap a number of blocks of
the same size, instead of a single block. Doing so allows
multiple blocks to be unmapped in one call to the io-pgtable
code, reducing the number of page table walks, and indirect
calls.

Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
Suggested-by: Will Deacon <will@kernel.org>
Signed-off-by: Will Deacon <will@kernel.org>
---
 include/linux/io-pgtable.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
index a4c9ca2c31f1..2ed0c057d9e7 100644
--- a/include/linux/io-pgtable.h
+++ b/include/linux/io-pgtable.h
@@ -144,6 +144,7 @@ struct io_pgtable_cfg {
  *
  * @map:          Map a physically contiguous memory region.
  * @unmap:        Unmap a physically contiguous memory region.
+ * @unmap_pages:  Unmap a range of virtually contiguous pages of the same size.
  * @iova_to_phys: Translate iova to physical address.
  *
  * These functions map directly onto the iommu_ops member functions with
@@ -154,6 +155,9 @@ struct io_pgtable_ops {
 		   phys_addr_t paddr, size_t size, int prot, gfp_t gfp);
 	size_t (*unmap)(struct io_pgtable_ops *ops, unsigned long iova,
 			size_t size, struct iommu_iotlb_gather *gather);
+	size_t (*unmap_pages)(struct io_pgtable_ops *ops, unsigned long iova,
+			      size_t pgsize, size_t pgcount,
+			      struct iommu_iotlb_gather *gather);
 	phys_addr_t (*iova_to_phys)(struct io_pgtable_ops *ops,
 				    unsigned long iova);
 };
-- 
2.31.0.291.g576ba9dcdaf-goog

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [RFC PATCH 2/6] iommu: Add an unmap_pages() op for IOMMU drivers
  2021-04-01 16:47 [RFC PATCH 0/6] iommu_pgsize() improvements to help towards ->[un]map_pages() Will Deacon
  2021-04-01 16:47 ` [RFC PATCH 1/6] iommu/io-pgtable: Introduce unmap_pages() as a page table op Will Deacon
@ 2021-04-01 16:47 ` Will Deacon
  2021-04-01 16:47 ` [RFC PATCH 3/6] iommu: Use bitmap to calculate page size in iommu_pgsize() Will Deacon
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: Will Deacon @ 2021-04-01 16:47 UTC (permalink / raw)
  To: iommu
  Cc: Isaac J. Manjarres, Will Deacon, Robin Murphy, Pratik Patel,
	linux-arm-kernel

From: "Isaac J. Manjarres" <isaacm@codeaurora.org>

Add a callback for IOMMU drivers to provide a path for the
IOMMU framework to call into an IOMMU driver, which can call
into the io-pgtable code, to unmap a virtually contiguous
range of pages of the same size.

For IOMMU drivers that do not specify an unmap_pages() callback,
the existing logic of unmapping memory one page block at a time
will be used.

Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
Suggested-by: Will Deacon <will@kernel.org>
Signed-off-by: Will Deacon <will@kernel.org>
---
 include/linux/iommu.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 5e7fe519430a..9cf81242581a 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -193,6 +193,7 @@ struct iommu_iotlb_gather {
  * @detach_dev: detach device from an iommu domain
  * @map: map a physically contiguous memory region to an iommu domain
  * @unmap: unmap a physically contiguous memory region from an iommu domain
+ * @unmap_pages: unmap a number of pages of the same size from an iommu domain
  * @flush_iotlb_all: Synchronously flush all hardware TLBs for this domain
  * @iotlb_sync_map: Sync mappings created recently using @map to the hardware
  * @iotlb_sync: Flush all queued ranges from the hardware TLBs and empty flush
@@ -245,6 +246,9 @@ struct iommu_ops {
 		   phys_addr_t paddr, size_t size, int prot, gfp_t gfp);
 	size_t (*unmap)(struct iommu_domain *domain, unsigned long iova,
 		     size_t size, struct iommu_iotlb_gather *iotlb_gather);
+	size_t (*unmap_pages)(struct iommu_domain *domain, unsigned long iova,
+			      size_t pgsize, size_t pgcount,
+			      struct iommu_iotlb_gather *iotlb_gather);
 	void (*flush_iotlb_all)(struct iommu_domain *domain);
 	void (*iotlb_sync_map)(struct iommu_domain *domain, unsigned long iova,
 			       size_t size);
-- 
2.31.0.291.g576ba9dcdaf-goog

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [RFC PATCH 3/6] iommu: Use bitmap to calculate page size in iommu_pgsize()
  2021-04-01 16:47 [RFC PATCH 0/6] iommu_pgsize() improvements to help towards ->[un]map_pages() Will Deacon
  2021-04-01 16:47 ` [RFC PATCH 1/6] iommu/io-pgtable: Introduce unmap_pages() as a page table op Will Deacon
  2021-04-01 16:47 ` [RFC PATCH 2/6] iommu: Add an unmap_pages() op for IOMMU drivers Will Deacon
@ 2021-04-01 16:47 ` Will Deacon
  2021-04-02  1:39   ` isaacm
  2021-04-01 16:47 ` [RFC PATCH 4/6] iommu: Split 'addr_merge' argument to iommu_pgsize() into separate parts Will Deacon
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 9+ messages in thread
From: Will Deacon @ 2021-04-01 16:47 UTC (permalink / raw)
  To: iommu
  Cc: Isaac J. Manjarres, Will Deacon, Robin Murphy, Pratik Patel,
	linux-arm-kernel

Avoid the potential for shifting values by amounts greater than the
width of their type by using a bitmap to compute page size in
iommu_pgsize().

Signed-off-by: Will Deacon <will@kernel.org>
---
 drivers/iommu/iommu.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d0b0a15dba84..bcd623862bf9 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -8,6 +8,7 @@
 
 #include <linux/device.h>
 #include <linux/kernel.h>
+#include <linux/bits.h>
 #include <linux/bug.h>
 #include <linux/types.h>
 #include <linux/init.h>
@@ -2360,30 +2361,22 @@ static size_t iommu_pgsize(struct iommu_domain *domain,
 			   unsigned long addr_merge, size_t size)
 {
 	unsigned int pgsize_idx;
+	unsigned long pgsizes;
 	size_t pgsize;
 
-	/* Max page size that still fits into 'size' */
-	pgsize_idx = __fls(size);
+	/* Page sizes supported by the hardware and small enough for @size */
+	pgsizes = domain->pgsize_bitmap & GENMASK(__fls(size), 0);
 
-	/* need to consider alignment requirements ? */
-	if (likely(addr_merge)) {
-		/* Max page size allowed by address */
-		unsigned int align_pgsize_idx = __ffs(addr_merge);
-		pgsize_idx = min(pgsize_idx, align_pgsize_idx);
-	}
-
-	/* build a mask of acceptable page sizes */
-	pgsize = (1UL << (pgsize_idx + 1)) - 1;
-
-	/* throw away page sizes not supported by the hardware */
-	pgsize &= domain->pgsize_bitmap;
+	/* Constrain the page sizes further based on the maximum alignment */
+	if (likely(addr_merge))
+		pgsizes &= GENMASK(__ffs(addr_merge), 0);
 
-	/* make sure we're still sane */
-	BUG_ON(!pgsize);
+	/* Make sure we have at least one suitable page size */
+	BUG_ON(!pgsizes);
 
-	/* pick the biggest page */
-	pgsize_idx = __fls(pgsize);
-	pgsize = 1UL << pgsize_idx;
+	/* Pick the biggest page size remaining */
+	pgsize_idx = __fls(pgsizes);
+	pgsize = BIT(pgsize_idx);
 
 	return pgsize;
 }
-- 
2.31.0.291.g576ba9dcdaf-goog

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [RFC PATCH 4/6] iommu: Split 'addr_merge' argument to iommu_pgsize() into separate parts
  2021-04-01 16:47 [RFC PATCH 0/6] iommu_pgsize() improvements to help towards ->[un]map_pages() Will Deacon
                   ` (2 preceding siblings ...)
  2021-04-01 16:47 ` [RFC PATCH 3/6] iommu: Use bitmap to calculate page size in iommu_pgsize() Will Deacon
@ 2021-04-01 16:47 ` Will Deacon
  2021-04-01 16:47 ` [RFC PATCH 5/6] iommu: Hook up '->unmap_pages' driver callback Will Deacon
  2021-04-01 16:47 ` [RFC PATCH 6/6] iommu: Accomodate larger pages in iommu_pgsize() 'count' calculation Will Deacon
  5 siblings, 0 replies; 9+ messages in thread
From: Will Deacon @ 2021-04-01 16:47 UTC (permalink / raw)
  To: iommu
  Cc: Isaac J. Manjarres, Will Deacon, Robin Murphy, Pratik Patel,
	linux-arm-kernel

The 'addr_merge' parameter to iommu_pgsize() is a fabricated address
intended to describe the alignment requirements to consider when
choosing an appropriate page size. On the iommu_map() path, this address
is the logical OR of the virtual and physical addresses.

Subsequent improvements to iommu_pgsize() will need to check the
alignment of the virtual and physical components of 'addr_merge'
independently, so pass them in as separate parameters and reconstruct
'addr_merge' locally.

No functional change.

Signed-off-by: Will Deacon <will@kernel.org>
---
 drivers/iommu/iommu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index bcd623862bf9..ab689611a03b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2357,12 +2357,13 @@ phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova)
 }
 EXPORT_SYMBOL_GPL(iommu_iova_to_phys);
 
-static size_t iommu_pgsize(struct iommu_domain *domain,
-			   unsigned long addr_merge, size_t size)
+static size_t iommu_pgsize(struct iommu_domain *domain, unsigned long iova,
+			   phys_addr_t paddr, size_t size)
 {
 	unsigned int pgsize_idx;
 	unsigned long pgsizes;
 	size_t pgsize;
+	phys_addr_t addr_merge = paddr | iova;
 
 	/* Page sizes supported by the hardware and small enough for @size */
 	pgsizes = domain->pgsize_bitmap & GENMASK(__fls(size), 0);
@@ -2415,7 +2416,7 @@ static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
 	pr_debug("map: iova 0x%lx pa %pa size 0x%zx\n", iova, &paddr, size);
 
 	while (size) {
-		size_t pgsize = iommu_pgsize(domain, iova | paddr, size);
+		size_t pgsize = iommu_pgsize(domain, iova, paddr, size);
 
 		pr_debug("mapping: iova 0x%lx pa %pa pgsize 0x%zx\n",
 			 iova, &paddr, pgsize);
@@ -2503,8 +2504,9 @@ static size_t __iommu_unmap(struct iommu_domain *domain,
 	 * or we hit an area that isn't mapped.
 	 */
 	while (unmapped < size) {
-		size_t pgsize = iommu_pgsize(domain, iova, size - unmapped);
+		size_t pgsize;
 
+		pgsize = iommu_pgsize(domain, iova, iova, size - unmapped);
 		unmapped_page = ops->unmap(domain, iova, pgsize, iotlb_gather);
 		if (!unmapped_page)
 			break;
-- 
2.31.0.291.g576ba9dcdaf-goog

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [RFC PATCH 5/6] iommu: Hook up '->unmap_pages' driver callback
  2021-04-01 16:47 [RFC PATCH 0/6] iommu_pgsize() improvements to help towards ->[un]map_pages() Will Deacon
                   ` (3 preceding siblings ...)
  2021-04-01 16:47 ` [RFC PATCH 4/6] iommu: Split 'addr_merge' argument to iommu_pgsize() into separate parts Will Deacon
@ 2021-04-01 16:47 ` Will Deacon
  2021-04-01 16:47 ` [RFC PATCH 6/6] iommu: Accomodate larger pages in iommu_pgsize() 'count' calculation Will Deacon
  5 siblings, 0 replies; 9+ messages in thread
From: Will Deacon @ 2021-04-01 16:47 UTC (permalink / raw)
  To: iommu
  Cc: Isaac J. Manjarres, Will Deacon, Robin Murphy, Pratik Patel,
	linux-arm-kernel

Extend iommu_pgsize() to populate an optional 'count' paramater so that
we can direct unmapping operation to the ->unmap_pages callback if it
has been provided by the driver.

Signed-off-by: Will Deacon <will@kernel.org>
---
 drivers/iommu/iommu.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index ab689611a03b..fe186691fc21 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2358,7 +2358,7 @@ phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova)
 EXPORT_SYMBOL_GPL(iommu_iova_to_phys);
 
 static size_t iommu_pgsize(struct iommu_domain *domain, unsigned long iova,
-			   phys_addr_t paddr, size_t size)
+			   phys_addr_t paddr, size_t size, size_t *count)
 {
 	unsigned int pgsize_idx;
 	unsigned long pgsizes;
@@ -2379,6 +2379,8 @@ static size_t iommu_pgsize(struct iommu_domain *domain, unsigned long iova,
 	pgsize_idx = __fls(pgsizes);
 	pgsize = BIT(pgsize_idx);
 
+	if (count)
+		*count = size >> pgsize_idx;
 	return pgsize;
 }
 
@@ -2416,7 +2418,7 @@ static int __iommu_map(struct iommu_domain *domain, unsigned long iova,
 	pr_debug("map: iova 0x%lx pa %pa size 0x%zx\n", iova, &paddr, size);
 
 	while (size) {
-		size_t pgsize = iommu_pgsize(domain, iova, paddr, size);
+		size_t pgsize = iommu_pgsize(domain, iova, paddr, size, NULL);
 
 		pr_debug("mapping: iova 0x%lx pa %pa pgsize 0x%zx\n",
 			 iova, &paddr, pgsize);
@@ -2467,6 +2469,19 @@ int iommu_map_atomic(struct iommu_domain *domain, unsigned long iova,
 }
 EXPORT_SYMBOL_GPL(iommu_map_atomic);
 
+static size_t __iommu_unmap_pages(struct iommu_domain *domain,
+				  unsigned long iova, size_t size,
+				  struct iommu_iotlb_gather *iotlb_gather)
+{
+	const struct iommu_ops *ops = domain->ops;
+	size_t pgsize, count;
+
+	pgsize = iommu_pgsize(domain, iova, iova, size, &count);
+	return ops->unmap_pages ?
+	       ops->unmap_pages(domain, iova, pgsize, count, iotlb_gather) :
+	       ops->unmap(domain, iova, pgsize, iotlb_gather);
+}
+
 static size_t __iommu_unmap(struct iommu_domain *domain,
 			    unsigned long iova, size_t size,
 			    struct iommu_iotlb_gather *iotlb_gather)
@@ -2504,10 +2519,9 @@ static size_t __iommu_unmap(struct iommu_domain *domain,
 	 * or we hit an area that isn't mapped.
 	 */
 	while (unmapped < size) {
-		size_t pgsize;
-
-		pgsize = iommu_pgsize(domain, iova, iova, size - unmapped);
-		unmapped_page = ops->unmap(domain, iova, pgsize, iotlb_gather);
+		unmapped_page = __iommu_unmap_pages(domain, iova,
+						    size - unmapped,
+						    iotlb_gather);
 		if (!unmapped_page)
 			break;
 
-- 
2.31.0.291.g576ba9dcdaf-goog

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [RFC PATCH 6/6] iommu: Accomodate larger pages in iommu_pgsize() 'count' calculation
  2021-04-01 16:47 [RFC PATCH 0/6] iommu_pgsize() improvements to help towards ->[un]map_pages() Will Deacon
                   ` (4 preceding siblings ...)
  2021-04-01 16:47 ` [RFC PATCH 5/6] iommu: Hook up '->unmap_pages' driver callback Will Deacon
@ 2021-04-01 16:47 ` Will Deacon
  5 siblings, 0 replies; 9+ messages in thread
From: Will Deacon @ 2021-04-01 16:47 UTC (permalink / raw)
  To: iommu
  Cc: Isaac J. Manjarres, Will Deacon, Robin Murphy, Pratik Patel,
	linux-arm-kernel

Extend the calculation of 'count' in iommu_pgsize() so that it takes
larger page sizes into consideration and returns a value which will
allow a larger page size to be used on the next call.

Signed-off-by: Will Deacon <will@kernel.org>
---
 drivers/iommu/iommu.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index fe186691fc21..0572a4dcd9e2 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2360,9 +2360,9 @@ EXPORT_SYMBOL_GPL(iommu_iova_to_phys);
 static size_t iommu_pgsize(struct iommu_domain *domain, unsigned long iova,
 			   phys_addr_t paddr, size_t size, size_t *count)
 {
-	unsigned int pgsize_idx;
+	unsigned int pgsize_idx, pgsize_idx_next;
 	unsigned long pgsizes;
-	size_t pgsize;
+	size_t offset, pgsize, pgsize_next;
 	phys_addr_t addr_merge = paddr | iova;
 
 	/* Page sizes supported by the hardware and small enough for @size */
@@ -2378,9 +2378,37 @@ static size_t iommu_pgsize(struct iommu_domain *domain, unsigned long iova,
 	/* Pick the biggest page size remaining */
 	pgsize_idx = __fls(pgsizes);
 	pgsize = BIT(pgsize_idx);
+	if (!count)
+		return pgsize;
 
-	if (count)
-		*count = size >> pgsize_idx;
+
+	/* Find the next biggest support page size, if it exists */
+	pgsizes = domain->pgsize_bitmap & ~GENMASK(pgsize_idx, 0);
+	if (!pgsizes)
+		goto out_set_count;
+
+	pgsize_idx_next = __ffs(pgsizes);
+	pgsize_next = BIT(pgsize_idx_next);
+
+	/*
+	 * There's no point trying a bigger page size unless the virtual
+	 * and physical addresses are similarly offset within the larger page.
+	 */
+	if ((iova ^ paddr) & (pgsize_next - 1))
+		goto out_set_count;
+
+	/* Calculate the offset to the next page size alignment boundary */
+	offset = pgsize_next - (addr_merge & (pgsize_next - 1));
+
+	/*
+	 * If size is big enough to accomodate the larger page, reduce
+	 * the number of smaller pages.
+	 */
+	if (offset + pgsize_next <= size)
+		size = offset;
+
+out_set_count:
+	*count = size >> pgsize_idx;
 	return pgsize;
 }
 
-- 
2.31.0.291.g576ba9dcdaf-goog

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH 3/6] iommu: Use bitmap to calculate page size in iommu_pgsize()
  2021-04-01 16:47 ` [RFC PATCH 3/6] iommu: Use bitmap to calculate page size in iommu_pgsize() Will Deacon
@ 2021-04-02  1:39   ` isaacm
  2021-04-06 11:40     ` Will Deacon
  0 siblings, 1 reply; 9+ messages in thread
From: isaacm @ 2021-04-02  1:39 UTC (permalink / raw)
  To: Will Deacon; +Cc: Robin Murphy, iommu, Pratik Patel, linux-arm-kernel

On 2021-04-01 09:47, Will Deacon wrote:
> Avoid the potential for shifting values by amounts greater than the
> width of their type by using a bitmap to compute page size in
> iommu_pgsize().
> 
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  drivers/iommu/iommu.c | 31 ++++++++++++-------------------
>  1 file changed, 12 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index d0b0a15dba84..bcd623862bf9 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -8,6 +8,7 @@
> 
>  #include <linux/device.h>
>  #include <linux/kernel.h>
> +#include <linux/bits.h>
>  #include <linux/bug.h>
>  #include <linux/types.h>
>  #include <linux/init.h>
> @@ -2360,30 +2361,22 @@ static size_t iommu_pgsize(struct iommu_domain 
> *domain,
>  			   unsigned long addr_merge, size_t size)
>  {
>  	unsigned int pgsize_idx;
> +	unsigned long pgsizes;
>  	size_t pgsize;
> 
> -	/* Max page size that still fits into 'size' */
> -	pgsize_idx = __fls(size);
> +	/* Page sizes supported by the hardware and small enough for @size */
> +	pgsizes = domain->pgsize_bitmap & GENMASK(__fls(size), 0);
I've fixed this in the latest RFC for the iommu_map/unmap optimization 
patches,
but for the sake of completeness: I think this should be GENMASK_ULL, in 
case
__fls(size) >= 32.

Thank you for these patches, by the way. I've looked through them and 
they
make sense/seem correct. I've integrated them into the latest RFC: 
https://lore.kernel.org/linux-iommu/20210402013452.4013-1-isaacm@codeaurora.org/T/#t.

Thanks,
Isaac
> 
> -	/* need to consider alignment requirements ? */
> -	if (likely(addr_merge)) {
> -		/* Max page size allowed by address */
> -		unsigned int align_pgsize_idx = __ffs(addr_merge);
> -		pgsize_idx = min(pgsize_idx, align_pgsize_idx);
> -	}
> -
> -	/* build a mask of acceptable page sizes */
> -	pgsize = (1UL << (pgsize_idx + 1)) - 1;
> -
> -	/* throw away page sizes not supported by the hardware */
> -	pgsize &= domain->pgsize_bitmap;
> +	/* Constrain the page sizes further based on the maximum alignment */
> +	if (likely(addr_merge))
> +		pgsizes &= GENMASK(__ffs(addr_merge), 0);
> 
> -	/* make sure we're still sane */
> -	BUG_ON(!pgsize);
> +	/* Make sure we have at least one suitable page size */
> +	BUG_ON(!pgsizes);
> 
> -	/* pick the biggest page */
> -	pgsize_idx = __fls(pgsize);
> -	pgsize = 1UL << pgsize_idx;
> +	/* Pick the biggest page size remaining */
> +	pgsize_idx = __fls(pgsizes);
> +	pgsize = BIT(pgsize_idx);
> 
>  	return pgsize;
>  }
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH 3/6] iommu: Use bitmap to calculate page size in iommu_pgsize()
  2021-04-02  1:39   ` isaacm
@ 2021-04-06 11:40     ` Will Deacon
  0 siblings, 0 replies; 9+ messages in thread
From: Will Deacon @ 2021-04-06 11:40 UTC (permalink / raw)
  To: isaacm; +Cc: Robin Murphy, iommu, Pratik Patel, linux-arm-kernel

On Thu, Apr 01, 2021 at 06:39:35PM -0700, isaacm@codeaurora.org wrote:
> On 2021-04-01 09:47, Will Deacon wrote:
> > Avoid the potential for shifting values by amounts greater than the
> > width of their type by using a bitmap to compute page size in
> > iommu_pgsize().
> > 
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> >  drivers/iommu/iommu.c | 31 ++++++++++++-------------------
> >  1 file changed, 12 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index d0b0a15dba84..bcd623862bf9 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -8,6 +8,7 @@
> > 
> >  #include <linux/device.h>
> >  #include <linux/kernel.h>
> > +#include <linux/bits.h>
> >  #include <linux/bug.h>
> >  #include <linux/types.h>
> >  #include <linux/init.h>
> > @@ -2360,30 +2361,22 @@ static size_t iommu_pgsize(struct iommu_domain
> > *domain,
> >  			   unsigned long addr_merge, size_t size)
> >  {
> >  	unsigned int pgsize_idx;
> > +	unsigned long pgsizes;
> >  	size_t pgsize;
> > 
> > -	/* Max page size that still fits into 'size' */
> > -	pgsize_idx = __fls(size);
> > +	/* Page sizes supported by the hardware and small enough for @size */
> > +	pgsizes = domain->pgsize_bitmap & GENMASK(__fls(size), 0);
> I've fixed this in the latest RFC for the iommu_map/unmap optimization
> patches,
> but for the sake of completeness: I think this should be GENMASK_ULL, in
> case
> __fls(size) >= 32.

Hmm, but 'size' is a size_t; which architectures have sizeof(size_t) >
sizeof(unsigned long)?

> > -	/* need to consider alignment requirements ? */
> > -	if (likely(addr_merge)) {
> > -		/* Max page size allowed by address */
> > -		unsigned int align_pgsize_idx = __ffs(addr_merge);
> > -		pgsize_idx = min(pgsize_idx, align_pgsize_idx);
> > -	}
> > -
> > -	/* build a mask of acceptable page sizes */
> > -	pgsize = (1UL << (pgsize_idx + 1)) - 1;
> > -
> > -	/* throw away page sizes not supported by the hardware */
> > -	pgsize &= domain->pgsize_bitmap;
> > +	/* Constrain the page sizes further based on the maximum alignment */
> > +	if (likely(addr_merge))
> > +		pgsizes &= GENMASK(__ffs(addr_merge), 0);

This one looks like more of an issue, though, as addr_merge is a
phys_addr_t, which certainly can be 64-bit where unsigned long is 32-bit
(e.g. Armv7 + LPAE)

Rather than make everything _ULL, please can you just do it where we're
actually using the larger types?

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-04-06 11:40 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-01 16:47 [RFC PATCH 0/6] iommu_pgsize() improvements to help towards ->[un]map_pages() Will Deacon
2021-04-01 16:47 ` [RFC PATCH 1/6] iommu/io-pgtable: Introduce unmap_pages() as a page table op Will Deacon
2021-04-01 16:47 ` [RFC PATCH 2/6] iommu: Add an unmap_pages() op for IOMMU drivers Will Deacon
2021-04-01 16:47 ` [RFC PATCH 3/6] iommu: Use bitmap to calculate page size in iommu_pgsize() Will Deacon
2021-04-02  1:39   ` isaacm
2021-04-06 11:40     ` Will Deacon
2021-04-01 16:47 ` [RFC PATCH 4/6] iommu: Split 'addr_merge' argument to iommu_pgsize() into separate parts Will Deacon
2021-04-01 16:47 ` [RFC PATCH 5/6] iommu: Hook up '->unmap_pages' driver callback Will Deacon
2021-04-01 16:47 ` [RFC PATCH 6/6] iommu: Accomodate larger pages in iommu_pgsize() 'count' calculation Will Deacon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).