iommu_pgsize() improvements to help towards ->[un]map_pages()
Will Deacon
maintainercover iommu_pgsize() improvements to help towards ->[un]map_pages()
Hi Isaac,
I had a go at removing the loop you have in pgsize_bitmap() over at:
https://lore.kernel.org/r/[email protected]
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
--->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(-)
Signature · 6 lines
2.31.0.291.g576ba9dcdaf-goog _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
Will Deacon
maintainerRFC PATCH 1/6 iommu/io-pgtable: Introduce unmap_pages() as a page table op
From: "Isaac J. Manjarres" <[email protected]>
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.
--- include/linux/io-pgtable.h | 4 ++++ 1 file changed, 4 insertions(+)
include/linux/io-pgtable.h +4 -0
@@ -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); };
Signature · 6 lines
2.31.0.291.g576ba9dcdaf-goog _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
Will Deacon
maintainerRFC PATCH 2/6 iommu: Add an unmap_pages() op for IOMMU drivers
In reply to Will DeaconFrom: "Isaac J. Manjarres" <[email protected]>
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.
--- include/linux/iommu.h | 4 ++++ 1 file changed, 4 insertions(+)
include/linux/iommu.h +4 -0
@@ -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);
Signature · 6 lines
2.31.0.291.g576ba9dcdaf-goog _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
Will Deacon
maintainerRFC PATCH 3/6 iommu: Use bitmap to calculate page size in iommu_pgsize()
In reply to Will DeaconAvoid 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().
--- drivers/iommu/iommu.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-)
drivers/iommu/iommu.c +12 -19
@@ -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; }
Signature · 6 lines
2.31.0.291.g576ba9dcdaf-goog _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 3/6] iommu: Use bitmap to calculate page size in iommu_pgsize()
Quoted 23 lines · Will Deacon
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 <[email protected]>--- drivers/iommu/iommu.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-)drivers/iommu/iommu.c +1
-0@@ -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,
drivers/iommu/iommu.c static size_t iommu_pgsize(struct iommu_domain · line 2369
Quoted 10 lines
drivers/iommu/iommu.c +3
-2unsigned 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/[email protected]/T/#t.
Thanks,
Isaac
Quoted 21 lines
drivers/iommu/iommu.c +5
-14-/* 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);
drivers/iommu/iommu.c static size_t iommu_pgsize(struct iommu_domain *domain, · line 2382
Quoted 10 lines
drivers/iommu/iommu.c +3
-3-/* 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
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Will Deacon
maintainerRe: [RFC PATCH 3/6] iommu: Use bitmap to calculate page size in iommu_pgsize()
Quoted 30 lines · On Thu, Apr 01, 2021 at 06:39:35PM -0700, [email protected] wrote:
Quoted 29 lines · Will Deacon
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 <[email protected]>--- drivers/iommu/iommu.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-)drivers/iommu/iommu.c +2
-0@@ -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;
Quoted 9 lines
drivers/iommu/iommu.c static size_t iommu_pgsize(struct iommu_domain *domain, · line 2368
Quoted 4 lines
drivers/iommu/iommu.c +2
-2-/* 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 beGENMASK_ULL, in case__fls(size)>= 32.
Hmm, but 'size' is a size_t; which architectures have sizeof(size_t) > sizeof(unsigned long)?
Quoted 7 lines
Quoted 7 lines
drivers/iommu/iommu.c +0
-7-/* 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); - } -
Quoted 8 lines
drivers/iommu/iommu.c static size_t iommu_pgsize(struct iommu_domain *domain, · line 2372
Quoted 8 lines
drivers/iommu/iommu.c +3
-5-/* 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
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Will Deacon
maintainerRFC PATCH 4/6 iommu: Split 'addr_merge' argument to iommu_pgsize() into separate parts
In reply to Will DeaconThe '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.
--- drivers/iommu/iommu.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/iommu/iommu.c +6 -4
@@ -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 ;
Signature · 6 lines
2.31.0.291.g576ba9dcdaf-goog _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
Will Deacon
maintainerRFC PATCH 5/6 iommu: Hook up '->unmap_pages' driver callback
In reply to Will DeaconExtend 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.
--- drivers/iommu/iommu.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)
drivers/iommu/iommu.c +20 -6
@@ -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 ;
Signature · 6 lines
2.31.0.291.g576ba9dcdaf-goog _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
Will Deacon
maintainerRFC PATCH 6/6 iommu: Accomodate larger pages in iommu_pgsize() 'count' calculation
In reply to Will DeaconExtend 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.
--- drivers/iommu/iommu.c | 36 ++++++++++++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 4 deletions(-)
drivers/iommu/iommu.c +32 -4
@@ -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; }
Signature · 6 lines
2.31.0.291.g576ba9dcdaf-goog _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu