Generic IOMMU page table framework
Subsystems (3)
cover Generic IOMMU page table framework
Hi all,
This series introduces a generic IOMMU page table allocation framework, implements support for ARM long-descriptors and then ports the arm-smmu driver over to the new code.
There are a few reasons for doing this:
- Page table code is hard, and I don't enjoy shopping
- A number of IOMMUs actually use the same table format, but currently duplicate the code
- It provides a CPU (and architecture) independent allocator, which may be useful for some systems where the CPU is using a different table format for its own mappings
As illustrated in the final patch, an IOMMU driver interacts with the allocator by passing in a configuration structure describing the input and output address ranges, the supported pages sizes and a set of ops for performing various TLB invalidation and PTE flushing routines.
The LPAE code implements support for 4k/2M/1G, 16k/32M and 64k/512M mappings, but I decided not to implement the contiguous bit in the interest of trying to keep the code semi-readable. This could always be added later, if needed.
I also included some self-tests for the LPAE implementation. Ideally we'd merge these, but I'm also happy to drop them if there are objections.
Tested with the self-tests, but also VFIO + MMU-500 at stage-1 and stage-2. Patches taken against my iommu/devel branch (queued by Joerg for 3.19).
All feedback welcome.
Will
--->8
Will Deacon (4):
iommu: introduce generic page table allocation framework
iommu: add ARM LPAE page table allocator
iommu: add self-consistency tests to ARM LPAE IO page table allocator iommu/arm-smmu: make use of generic LPAE allocator
MAINTAINERS | 1 + arch/arm64/Kconfig | 1 - drivers/iommu/Kconfig | 32 +- drivers/iommu/Makefile | 2 + drivers/iommu/arm-smmu.c | 872 +++++++++++--------------------------- drivers/iommu/io-pgtable-arm.c | 925 +++++++++++++++++++++++++++++++++++++++++ drivers/iommu/io-pgtable.c | 78 ++++ drivers/iommu/io-pgtable.h | 77 ++++ 8 files changed, 1361 insertions(+), 627 deletions(-) create mode 100644 drivers/iommu/io-pgtable-arm.c create mode 100644 drivers/iommu/io-pgtable.c create mode 100644 drivers/iommu/io-pgtable.h
Signature · 1 line
2.1.1
PATCH 1/4 iommu: introduce generic page table allocation framework
This patch introduces a generic framework for allocating page tables for an IOMMU. There are a number of reasons we want to do this:
- It avoids duplication of complex table management code in IOMMU drivers that use the same page table format
- It removes any coupling with the CPU table format (and even the architecture!)
- It defines an API for IOMMU TLB maintenance
--- drivers/iommu/Kconfig | 8 ++++++ drivers/iommu/Makefile | 1 + drivers/iommu/io-pgtable.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++ drivers/iommu/io-pgtable.h | 65 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 145 insertions(+) create mode 100644 drivers/iommu/io-pgtable.c create mode 100644 drivers/iommu/io-pgtable.h
drivers/iommu/Kconfig +8 -0
@@ -13,6 +13,14 @@ menuconfig IOMMU_SUPPORT if IOMMU_SUPPORT +menu "Generic IOMMU Pagetable Support" + +# Selected by the actual pagetable implementations +config IOMMU_IO_PGTABLE + bool + +endmenu + config OF_IOMMU def_bool y depends on OF
drivers/iommu/Makefile +1 -0
@@ -1,6 +1,7 @@ obj-$(CONFIG_IOMMU_API) += iommu.o obj-$(CONFIG_IOMMU_API) += iommu-traces.o obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o +obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o obj-$(CONFIG_OF_IOMMU) += of_iommu.o obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o msm_iommu_dev.o obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o
drivers/iommu/io-pgtable.c new file +71 -0
71 lines not on this page · raw message
drivers/iommu/io-pgtable.h new file +65 -0
65 lines not on this page · raw message
Signature · 1 line
2.1.1
PATCH 1/4 iommu: introduce generic page table allocation framework
Hi Will,
Thank you for the patch.
Quoted 78 lines · On Thursday 27 November 2014 11:51:15 Will Deacon wrote:
This patch introduces a generic framework for allocating page tables for an IOMMU. There are a number of reasons we want to do this:
- It avoids duplication of complex table management code in IOMMU drivers that use the same page table format
- It removes any coupling with the CPU table format (and even the architecture!)
- It defines an API for IOMMU TLB maintenance
Signed-off-by Will Deacon <[email protected]>--- drivers/iommu/Kconfig | 8 ++++++ drivers/iommu/Makefile | 1 + drivers/iommu/io-pgtable.c | 71 +++++++++++++++++++++++++++++++++++++++++++ drivers/iommu/io-pgtable.h | 65 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 145 insertions(+) create mode 100644 drivers/iommu/io-pgtable.c create mode 100644 drivers/iommu/io-pgtable.hdrivers/iommu/Kconfig +8
-0@@ -13,6 +13,14 @@ menuconfig IOMMU_SUPPORT if IOMMU_SUPPORT +menu "Generic IOMMU Pagetable Support" + +# Selected by the actual pagetable implementations +config IOMMU_IO_PGTABLE + bool + +endmenu + config OF_IOMMU def_bool y depends on OFdrivers/iommu/Makefile +1
-0@@ -1,6 +1,7 @@ obj-$(CONFIG_IOMMU_API) += iommu.o obj-$(CONFIG_IOMMU_API) += iommu-traces.o obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o +obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o obj-$(CONFIG_OF_IOMMU) += of_iommu.o obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o msm_iommu_dev.o obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.odrivers/iommu/io-pgtable.c new file +15
-0@@ -0,0 +1,71 @@ +/* +* Generic page table allocator for IOMMUs. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. + *
+ * Copyright (C) 2014 ARM Limited
+ *
+ * Author: Will Deacon <[email protected]>
+ */
drivers/iommu/io-pgtable.c
Quoted 8 lines
drivers/iommu/io-pgtable.c new file +8
-0+ +#include <linux/bug.h> +#include <linux/kernel.h> +#include <linux/types.h> + +#include "io-pgtable.h" + +static struct io_pgtable_init_fns
Any reason not to make the table const ?
Quoted 50 lines
*io_pgtable_init_table[IO_PGTABLE_NUM_FMTS] =
drivers/iommu/io-pgtable.c new file +43
-0+{ +}; + +struct io_pgtable_ops *alloc_io_pgtable_ops (enum io_pgtable_fmt fmt, +struct io_pgtable_cfg *cfg, +void *cookie) +{ +struct io_pgtable *iop; +struct io_pgtable_init_fns *fns; + +if (fmt >=IO_PGTABLE_NUM_FMTS ) +return NULL ; + + fns = io_pgtable_init_table[fmt]; +if (!fns) +return NULL ; + + iop = fns->alloc (cfg, cookie); +if (!iop) +return NULL ; + + iop->fmt = fmt; + iop->cookie = cookie; + iop->cfg = *cfg; + +return &iop->ops; +} + +/* +* It is the IOMMU driver's responsibility to ensure that the page table +* is no longer accessible to the walker by this point. +*/ +void free_io_pgtable_ops (struct io_pgtable_ops *ops) +{ +struct io_pgtable *iop; + +if (!ops) +return ; + + iop = container_of(ops, struct io_pgtable, ops); + iop->cfg.tlb->tlb_flush_all (iop->cookie); + io_pgtable_init_table[iop->fmt]->free (iop); +}drivers/iommu/io-pgtable.h new file +0
-0
drivers/iommu/io-pgtable.h line 5
Quoted 5 lines
drivers/iommu/io-pgtable.h new file +5
-0@@ -0,0 +1,65 @@ +#ifndef __IO_PGTABLE_H +#define __IO_PGTABLE_H + +struct io_pgtable_ops { + int (*map)(struct io_pgtable_ops *ops, unsigned long iova,
How about passing a struct io_pgtable * instead of the ops pointer ? This would require returning a struct io_pgtable from the alloc function, which I suppose you didn't want to do to ensure the caller will not touch the struct io_pgtable fields directly. Do we really need to go that far, or can we simply document struct io_pgtable as being private to the pg alloc framework core and allocators ? Someone who really wants to get hold of the io_pgtable instance could use container_of on the ops anyway, like the allocators do.
Quoted 4 lines
drivers/iommu/io-pgtable.h new file +4
-0+ phys_addr_t paddr, size_t size, int prot); +int (*unmap)(struct io_pgtable_ops *ops,unsigned long iova, +size_t size); + phys_addr_t (*iova_to_phys)(struct io_pgtable_ops *ops,
drivers/iommu/io-pgtable.h line 19
Quoted 10 lines
drivers/iommu/io-pgtable.h new file +10
-0+ unsigned long iova); +}; + +struct iommu_gather_ops { +/* Synchronously invalidate the entire TLB context */ +void (*tlb_flush_all)(void *cookie); + +/* Queue up a TLB invalidation for a virtual address range */ +void (*tlb_add_flush)(unsigned long iova,size_t size,bool leaf, +void *cookie);
Is there a limit to the number of entries that can be queued, or any other kind of restriction ? Implementing a completely generic TLB flush queue can become complex for IOMMU drivers.
I would also document in which context(s) this callback will be called, as IOMMU drivers might be tempted to allocate memory in order to implement a TLB flush queue.
drivers/iommu/io-pgtable.h line 25
Quoted 6 lines
drivers/iommu/io-pgtable.h new file +6
-0+/* Ensure any queued TLB invalidation has taken effect */ +void (*tlb_sync)(void *cookie); + +/* Ensure page tables updates are visible to the IOMMU */ +void (*flush_pgtable)(void *ptr,size_t size,void *cookie); +};
I suppose kerneldoc will come in the next version ;-)
Quoted 11 lines
drivers/iommu/io-pgtable.h new file +11
-0+struct io_pgtable_cfg { +int quirks;/* IO_PGTABLE_QUIRK_* */ +unsigned long pgsize_bitmap; +unsigned int ias; +unsigned int oas; +struct iommu_gather_ops *tlb; + +/* Low-level data specific to the table format */ +union { + }; +};
drivers/iommu/io-pgtable.h line 47
Quoted 10 lines
drivers/iommu/io-pgtable.h new file +10
-0+ +enum io_pgtable_fmt { +IO_PGTABLE_NUM_FMTS , +}; + +struct io_pgtable { +enum io_pgtable_fmt fmt; +void *cookie; +struct io_pgtable_cfg cfg; +struct io_pgtable_ops ops;
This could be turned into a const pointer if we pass struct io_pgtable around instead of the ops.
drivers/iommu/io-pgtable.h line 53
Quoted 6 lines
drivers/iommu/io-pgtable.h new file +6
-0+}; + +struct io_pgtable_init_fns { +struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg,void *cookie); +void (*free)(struct io_pgtable *iop); +};
I would reorder structures into two groups, one clearly marked as private that shouldn't be touched by IOMMU drivers, and then the io_pgtable_fmt enum and the io_pgtable_cfg struct grouped with the two functions below.
Quoted 11 lines
drivers/iommu/io-pgtable.h new file +11
-0+struct io_pgtable_ops *alloc_io_pgtable_ops (enum io_pgtable_fmt fmt, +struct io_pgtable_cfg *cfg, +void *cookie); + +/* +* Free an io_pgtable_ops structure. The caller *must* ensure that the +* page table is no longer live, but the TLB can be dirty. +*/ +void free_io_pgtable_ops (struct io_pgtable_ops *ops); + +#endif /* __IO_PGTABLE_H */
Signature · 3 lines
Regards, Laurent Pinchart
PATCH 1/4 iommu: introduce generic page table allocation framework
Quoted 1 line · Laurent Pinchart
Hi Will,
Hi Laurent,
Quoted 1 line
Thank you for the patch.
Cheers for the review!
Quoted 29 lines · On Thursday 27 November 2014 11:51:15 Will Deacon wrote:
Quoted 28 lines
drivers/iommu/io-pgtable.c new file +15
-0@@ -0,0 +1,71 @@ +/* +* Generic page table allocator for IOMMUs. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. + *
+ * Copyright (C) 2014 ARM Limited
+ *
+ * Author: Will Deacon <[email protected]>
+ */drivers/iommu/io-pgtable.c new file +2
-0+ +#include <linux/bug.h>
Quoted 8 lines
drivers/iommu/io-pgtable.c
Quoted 6 lines
drivers/iommu/io-pgtable.c new file +6
-0+#include <linux/kernel.h> +#include <linux/types.h> + +#include "io-pgtable.h" + +static struct io_pgtable_init_fnsAny reason not to make the table const ?
No reason, I'll give it a go.
Quoted 12 lines
drivers/iommu/io-pgtable.h line 5
Quoted 11 lines
drivers/iommu/io-pgtable.h new file +5
-0@@ -0,0 +1,65 @@ +#ifndef __IO_PGTABLE_H +#define __IO_PGTABLE_H + +struct io_pgtable_ops { + int (*map)(struct io_pgtable_ops *ops, unsigned long iova,
Quoted 7 lines
How about passing a struct
io_pgtable* instead of the ops pointer ? This would require returning a structio_pgtablefrom the alloc function, which I suppose you didn't want to do to ensure the caller will not touch the structio_pgtablefields directly. Do we really need to go that far, or can we simply document structio_pgtableas being private to the pg alloc framework core and allocators ? Someone who really wants to get hold of theio_pgtableinstance could usecontainer_ofon the ops anyway, like the allocators do.
Hmm, currently the struct io_pgtable is private to the page table allocator, so I don't like the IOMMU driver having an explicit handle to that. I also like having the value returned from alloc_io_pgtable_ops being used as the handle to pass around -- it keeps things simple for the caller because there's one structure that you get back and that's the thing you use as a reference.
What do we gain by returning the struct io_pgtable pointer instead?
Quoted 10 lines
Quoted 10 lines
drivers/iommu/io-pgtable.h new file +10
-0+ phys_addr_t paddr, size_t size, int prot); +int (*unmap)(struct io_pgtable_ops *ops,unsigned long iova, +size_t size); +phys_addr_t (*iova_to_phys)(struct io_pgtable_ops *ops, +unsigned long iova); +}; + +struct iommu_gather_ops { +/* Synchronously invalidate the entire TLB context */ +void (*tlb_flush_all)(void *cookie);
Quoted 8 lines
drivers/iommu/io-pgtable.h line 19
Quoted 4 lines
drivers/iommu/io-pgtable.h new file +4
-0+ +/* Queue up a TLB invalidation for a virtual address range */ +void (*tlb_add_flush)(unsigned long iova,size_t size,bool leaf, +void *cookie);Is there a limit to the number of entries that can be queued, or any other kind of restriction ? Implementing a completely generic TLB flush queue can become complex for IOMMU drivers.
I think it's only as complicated as you decide to make it. For example, you could just issue the TLBI directly in the add_flush callback (like I do for the arm-smmu driver), but then don't bother polling the hardware for completion until the sync callback.
Quoted 3 lines
I would also document in which context(s) this callback will be called, as IOMMU drivers might be tempted to allocate memory in order to implement a TLB flush queue.
Good idea.
Quoted 8 lines
drivers/iommu/io-pgtable.h line 25
Quoted 6 lines
drivers/iommu/io-pgtable.h new file +6
-0+/* Ensure any queued TLB invalidation has taken effect */ +void (*tlb_sync)(void *cookie); + +/* Ensure page tables updates are visible to the IOMMU */ +void (*flush_pgtable)(void *ptr,size_t size,void *cookie); +};I suppose kerneldoc will come in the next version ;-)
Bah, ok then, if you insist!
Quoted 25 lines
Quoted 11 lines
drivers/iommu/io-pgtable.h new file +11
-0+struct io_pgtable_cfg { +int quirks;/* IO_PGTABLE_QUIRK_* */ +unsigned long pgsize_bitmap; +unsigned int ias; +unsigned int oas; +struct iommu_gather_ops *tlb; + +/* Low-level data specific to the table format */ +union { + }; +};drivers/iommu/io-pgtable.h line 47
Quoted 10 lines
drivers/iommu/io-pgtable.h new file +10
-0+ +enum io_pgtable_fmt { +IO_PGTABLE_NUM_FMTS , +}; + +struct io_pgtable { +enum io_pgtable_fmt fmt; +void *cookie; +struct io_pgtable_cfg cfg; +struct io_pgtable_ops ops;This could be turned into a const pointer if we pass struct
io_pgtablearound instead of the ops.
Quoted 10 lines
drivers/iommu/io-pgtable.h line 53
Quoted 6 lines
drivers/iommu/io-pgtable.h new file +6
-0+}; + +struct io_pgtable_init_fns { +struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg,void *cookie); +void (*free)(struct io_pgtable *iop); +};I would reorder structures into two groups, one clearly marked as private that shouldn't be touched by IOMMU drivers, and then the
io_pgtable_fmtenum and theio_pgtable_cfgstruct grouped with the two functions below.
Sure.
Thanks again for the review.
Will
PATCH 1/4 iommu: introduce generic page table allocation framework
Hi Will,
Quoted 16 lines · On Monday 01 December 2014 12:13:38 Will Deacon wrote:
Quoted 15 lines · Laurent Pinchart
Quoted 14 lines · On Thursday 27 November 2014 11:51:15 Will Deacon wrote:
drivers/iommu/io-pgtable.c new file +8
-0@@ -0,0 +1,71 @@ +/* +* Generic page table allocator for IOMMUs. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful,
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/iommu/io-pgtable.c new file +7
-0+* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
By the way you can remove this paragraph, we don't want to update all source files the day the FSF decides to move to a new address.
Quoted 39 lines
Quoted 7 lines
Quoted 7 lines
+ *
+ * Copyright (C) 2014 ARM Limited
+ *
+ * Author: Will Deacon <[email protected]>
+ */drivers/iommu/io-pgtable.c new file +2
-0+ +#include <linux/bug.h> Quoted 8 lines
drivers/iommu/io-pgtable.c
Quoted 6 lines
drivers/iommu/io-pgtable.c new file +6
-0+#include <linux/kernel.h> +#include <linux/types.h> + +#include "io-pgtable.h" + +static struct io_pgtable_init_fnsAny reason not to make the table const ?
No reason, I'll give it a go.
Quoted 12 lines
drivers/iommu/io-pgtable.h line 5
Quoted 11 lines
drivers/iommu/io-pgtable.h new file +5
-0@@ -0,0 +1,65 @@ +#ifndef __IO_PGTABLE_H +#define __IO_PGTABLE_H + +struct io_pgtable_ops { + int (*map)(struct io_pgtable_ops *ops, unsigned long iova,Quoted 8 lines
How about passing a struct
io_pgtable* instead of the ops pointer ? This would require returning a structio_pgtablefrom the alloc function, which I suppose you didn't want to do to ensure the caller will not touch the structio_pgtablefields directly. Do we really need to go that far, or can we simply document structio_pgtableas being private to the pg alloc framework core and allocators ? Someone who really wants to get hold of theio_pgtableinstance could usecontainer_ofon the ops anyway, like the allocators do.
Quoted 2 lines
Hmm, currently the struct
io_pgtableis private to the page table allocator, so I don't like the IOMMU driver having an explicit handle to that.
I agree with this, but given that struct io_pgtable is defined in a header used by the IOMMU driver, and given that it directly embeds struct io_pgtable_ops, there's no big difference between the two structures.
Quoted 4 lines
I also like having the value returned from
alloc_io_pgtable_opsbeing used as the handle to pass around -- it keeps things simple for the caller because there's one structure that you get back and that's the thing you use as a reference.
I agree with that as well, my proposal was to return a struct io_pgtable from alloc_io_pgtable_ops.
Quoted 1 line
What do we gain by returning the struct
io_pgtablepointer instead?
The ops structure could be made a const pointer. That's a pretty small optimization, granted.
Quoted 39 lines
Quoted 10 lines
Quoted 10 lines
drivers/iommu/io-pgtable.h new file +10
-0+ phys_addr_t paddr, size_t size, int prot); +int (*unmap)(struct io_pgtable_ops *ops,unsigned long iova, +size_t size); +phys_addr_t (*iova_to_phys)(struct io_pgtable_ops *ops, +unsigned long iova); +}; + +struct iommu_gather_ops { +/* Synchronously invalidate the entire TLB context */ +void (*tlb_flush_all)(void *cookie);Quoted 8 lines
drivers/iommu/io-pgtable.h line 19
Quoted 4 lines
drivers/iommu/io-pgtable.h new file +4
-0+ +/* Queue up a TLB invalidation for a virtual address range */ +void (*tlb_add_flush)(unsigned long iova,size_t size,bool leaf, +void *cookie);Is there a limit to the number of entries that can be queued, or any other kind of restriction ? Implementing a completely generic TLB flush queue can become complex for IOMMU drivers.
I think it's only as complicated as you decide to make it. For example, you could just issue the TLBI directly in the
add_flushcallback (like I do for the arm-smmu driver), but then don't bother polling the hardware for completion until the sync callback.Quoted 3 lines
I would also document in which context(s) this callback will be called, as IOMMU drivers might be tempted to allocate memory in order to implement a TLB flush queue.
Good idea.
Quoted 8 lines
drivers/iommu/io-pgtable.h line 25
Quoted 6 lines
drivers/iommu/io-pgtable.h new file +6
-0+/* Ensure any queued TLB invalidation has taken effect */ +void (*tlb_sync)(void *cookie); + +/* Ensure page tables updates are visible to the IOMMU */ +void (*flush_pgtable)(void *ptr,size_t size,void *cookie); +};I suppose kerneldoc will come in the next version ;-)
Quoted 1 line
Bah, ok then, if you insist!
I'm afraid I do :-)
Quoted 25 lines
Quoted 25 lines
Quoted 11 lines
drivers/iommu/io-pgtable.h new file +11
-0+struct io_pgtable_cfg { +int quirks;/* IO_PGTABLE_QUIRK_* */ +unsigned long pgsize_bitmap; +unsigned int ias; +unsigned int oas; +struct iommu_gather_ops *tlb; + +/* Low-level data specific to the table format */ +union { + }; +};drivers/iommu/io-pgtable.h line 47
Quoted 10 lines
drivers/iommu/io-pgtable.h new file +10
-0+ +enum io_pgtable_fmt { +IO_PGTABLE_NUM_FMTS , +}; + +struct io_pgtable { +enum io_pgtable_fmt fmt; +void *cookie; +struct io_pgtable_cfg cfg; +struct io_pgtable_ops ops;This could be turned into a const pointer if we pass struct
io_pgtablearound instead of the ops.
Quoted 4 lines
Quoted 4 lines
drivers/iommu/io-pgtable.h
Quoted 4 lines
drivers/iommu/io-pgtable.h new file +4
-0+}; + +struct io_pgtable_init_fns { + struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void
*cookie);
Quoted 10 lines
Quoted 6 lines
drivers/iommu/io-pgtable.h line 53
Quoted 2 lines
drivers/iommu/io-pgtable.h new file +2
-0+void (*free)(struct io_pgtable *iop); +};I would reorder structures into two groups, one clearly marked as private that shouldn't be touched by IOMMU drivers, and then the
io_pgtable_fmtenum and theio_pgtable_cfgstruct grouped with the two functions below.Sure.
Thanks again for the review.
You're welcome.
Signature · 3 lines
Regards, Laurent Pinchart
PATCH 1/4 iommu: introduce generic page table allocation framework
Quoted 20 lines · Laurent Pinchart
Quoted 19 lines · On Monday 01 December 2014 12:13:38 Will Deacon wrote:
Quoted 18 lines · Laurent Pinchart
Quoted 17 lines · On Thursday 27 November 2014 11:51:15 Will Deacon wrote:
drivers/iommu/io-pgtable.c new file +11
-0@@ -0,0 +1,71 @@ +/* +* Generic page table allocator for IOMMUs. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details.
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/iommu/io-pgtable.c new file +4
-0+* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
By the way you can remove this paragraph, we don't want to update all source files the day the FSF decides to move to a new address.
Yeah, I missed that one (I fixed the lpae file already).
Quoted 21 lines
Quoted 21 lines
Quoted 20 lines
drivers/iommu/io-pgtable.h line 5
Quoted 11 lines
drivers/iommu/io-pgtable.h new file +5
-0@@ -0,0 +1,65 @@ +#ifndef __IO_PGTABLE_H +#define __IO_PGTABLE_H + +struct io_pgtable_ops { + int (*map)(struct io_pgtable_ops *ops, unsigned long iova,How about passing a struct
io_pgtable* instead of the ops pointer ? This would require returning a structio_pgtablefrom the alloc function, which I suppose you didn't want to do to ensure the caller will not touch the structio_pgtablefields directly. Do we really need to go that far, or can we simply document structio_pgtableas being private to the pg alloc framework core and allocators ? Someone who really wants to get hold of theio_pgtableinstance could usecontainer_ofon the ops anyway, like the allocators do.
Quoted 6 lines
Quoted 2 lines
Hmm, currently the struct
io_pgtableis private to the page table allocator, so I don't like the IOMMU driver having an explicit handle to that.I agree with this, but given that struct
io_pgtableis defined in a header used by the IOMMU driver, and given that it directly embeds structio_pgtable_ops, there's no big difference between the two structures.
Right, but you have to do an explicit container_of and, with the kerneldoc added, it should be clear that it's not a good idea to mess with things like the cookie or the cfg after you've allocated the page tables.
Quoted 5 lines
Quoted 4 lines
I also like having the value returned from
alloc_io_pgtable_opsbeing used as the handle to pass around -- it keeps things simple for the caller because there's one structure that you get back and that's the thing you use as a reference.
Quoted 7 lines
I agree with that as well, my proposal was to return a struct
io_pgtablefromalloc_io_pgtable_ops.Quoted 1 line
What do we gain by returning the struct
io_pgtablepointer instead?The ops structure could be made a const pointer. That's a pretty small optimization, granted.
I still think I'd rather keep things like they are. Let's see how it looks in v2, when I've reordered the structures and documented them.
Will
PATCH 1/4 iommu: introduce generic page table allocation framework
In reply to Will DeaconHi Will,
Please see below for another small comment.
Quoted 20 lines · On Thursday 27 November 2014 11:51:15 Will Deacon wrote:
This patch introduces a generic framework for allocating page tables for an IOMMU. There are a number of reasons we want to do this:
- It avoids duplication of complex table management code in IOMMU drivers that use the same page table format
- It removes any coupling with the CPU table format (and even the architecture!)
- It defines an API for IOMMU TLB maintenance
Signed-off-by Will Deacon <[email protected]>--- drivers/iommu/Kconfig | 8 ++++++ drivers/iommu/Makefile | 1 + drivers/iommu/io-pgtable.c | 71 +++++++++++++++++++++++++++++++++++++++++++ drivers/iommu/io-pgtable.h | 65 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 145 insertions(+) create mode 100644 drivers/iommu/io-pgtable.c create mode 100644 drivers/iommu/io-pgtable.h
[snip]
Quoted 28 lines
drivers/iommu/io-pgtable.h new file +22
-0@@ -0,0 +1,65 @@ +#ifndef __IO_PGTABLE_H +#define __IO_PGTABLE_H + +struct io_pgtable_ops { + int (*map)(struct io_pgtable_ops *ops, unsigned long iova, + phys_addr_t paddr, size_t size, int prot); + int (*unmap)(struct io_pgtable_ops *ops, unsigned long iova, + size_t size); + phys_addr_t (*iova_to_phys)(struct io_pgtable_ops *ops, + unsigned long iova); +}; + +struct iommu_gather_ops { + /* Synchronously invalidate the entire TLB context */ + void (*tlb_flush_all)(void *cookie); + + /* Queue up a TLB invalidation for a virtual address range */ + void (*tlb_add_flush)(unsigned long iova, size_t size, bool leaf, + void *cookie); + /* Ensure any queued TLB invalidation has taken effect */ + void (*tlb_sync)(void *cookie); +
drivers/iommu/io-pgtable.h line 32
Quoted 10 lines
drivers/iommu/io-pgtable.h new file +10
-0+/* Ensure page tables updates are visible to the IOMMU */ +void (*flush_pgtable)(void *ptr,size_t size,void *cookie); +}; + +struct io_pgtable_cfg { +int quirks;/* IO_PGTABLE_QUIRK_* */ +unsigned long pgsize_bitmap; +unsigned int ias; +unsigned int oas; +struct iommu_gather_ops *tlb;
Could you make this pointer const ?
Quoted 32 lines
drivers/iommu/io-pgtable.h new file +32
-0+/* Low-level data specific to the table format */ +union { + }; +}; + +enum io_pgtable_fmt { +IO_PGTABLE_NUM_FMTS , +}; + +struct io_pgtable { +enum io_pgtable_fmt fmt; +void *cookie; +struct io_pgtable_cfg cfg; +struct io_pgtable_ops ops; +}; + +struct io_pgtable_init_fns { +struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg,void *cookie); +void (*free)(struct io_pgtable *iop); +}; + +struct io_pgtable_ops *alloc_io_pgtable_ops (enum io_pgtable_fmt fmt, +struct io_pgtable_cfg *cfg, +void *cookie); + +/* +* Free an io_pgtable_ops structure. The caller *must* ensure that the +* page table is no longer live, but the TLB can be dirty. +*/ +void free_io_pgtable_ops (struct io_pgtable_ops *ops); + +#endif /* __IO_PGTABLE_H */
Signature · 3 lines
Regards, Laurent Pinchart
PATCH 1/4 iommu: introduce generic page table allocation framework
Quoted 8 lines · Laurent Pinchart
drivers/iommu/io-pgtable.h line 32
Quoted 6 lines
drivers/iommu/io-pgtable.h new file +6
-0+struct io_pgtable_cfg { +int quirks;/* IO_PGTABLE_QUIRK_* */ +unsigned long pgsize_bitmap; +unsigned int ias; +unsigned int oas; +struct iommu_gather_ops *tlb;Could you make this pointer const ?
Sure. I'll post a v2 at -rc1 assuming I'm not fishing/drinking (I'm off work from Wednesday this week).
Will
PATCH 2/4 iommu: add ARM LPAE page table allocator
In reply to Will DeaconA number of IOMMUs found in ARM SoCs can walk architecture-compatible page tables.
This patch adds a generic allocator for Stage-1 and Stage-2 v7/v8 long-descriptor page tables. 4k, 16k and 64k pages are supported, with up to 4-levels of walk to cover a 48-bit address space.
--- MAINTAINERS | 1 + drivers/iommu/Kconfig | 9 + drivers/iommu/Makefile | 1 + drivers/iommu/io-pgtable-arm.c | 735 +++++++++++++++++++++++++++++++++++++++++ drivers/iommu/io-pgtable.c | 7 + drivers/iommu/io-pgtable.h | 12 + 6 files changed, 765 insertions(+) create mode 100644 drivers/iommu/io-pgtable-arm.c
MAINTAINERS +1 -0
@@ -1562,6 +1562,7 @@ M: Will Deacon <[email protected]> L: linux-arm-kernel at lists.infradead.org (moderated for non-subscribers) S: Maintained F: drivers/iommu/arm-smmu.c +F: drivers/iommu/io-pgtable-arm.c ARM64 PORT (AARCH64 ARCHITECTURE) M: Catalin Marinas <[email protected]>
drivers/iommu/Kconfig +9 -0
@@ -19,6 +19,15 @@ menu "Generic IOMMU Pagetable Support" config IOMMU_IO_PGTABLE bool +config IOMMU_IO_PGTABLE_LPAE + bool "ARMv7/v8 Long Descriptor Format" + select IOMMU_IO_PGTABLE + help + Enable support for the ARM long descriptor pagetable format. + This allocator supports 4K/2M/1G, 16K/32M and 64K/512M page + sizes at both stage-1 and stage-2, as well as address spaces + up to 48-bits in size. + endmenu config OF_IOMMU
drivers/iommu/Makefile +1 -0
@@ -2,6 +2,7 @@ obj-$(CONFIG_IOMMU_API) += iommu.o obj-$(CONFIG_IOMMU_API) += iommu-traces.o obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o +obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o obj-$(CONFIG_OF_IOMMU) += of_iommu.o obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o msm_iommu_dev.o obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o
drivers/iommu/io-pgtable-arm.c new file +735 -0
735 lines not on this page · raw message
drivers/iommu/io-pgtable.c +7 -0
@@ -25,8 +25,15 @@#include "io-pgtable.h" +extern struct io_pgtable_init_fns io_pgtable_arm_lpae_s1_init_fns; +extern struct io_pgtable_init_fns io_pgtable_arm_lpae_s2_init_fns; +static struct io_pgtable_init_fns *io_pgtable_init_table[IO_PGTABLE_NUM_FMTS ] = { +#ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE + [ARM_LPAE_S1 ] = &io_pgtable_arm_lpae_s1_init_fns, + [ARM_LPAE_S2 ] = &io_pgtable_arm_lpae_s2_init_fns, +#endif }; struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt,
drivers/iommu/io-pgtable.h +12 -0
@@ -33,10 +33,22 @@ struct io_pgtable_cfg {/* Low-level data specific to the table format */ union { +struct { +u64 ttbr[2 ]; +u64 tcr; +u64 mair[2 ]; + } arm_lpae_s1_cfg; + +struct { +u64 vttbr; +u64 vtcr; + } arm_lpae_s2_cfg; }; };enum io_pgtable_fmt { +ARM_LPAE_S1 , +ARM_LPAE_S2 ,IO_PGTABLE_NUM_FMTS , };
Signature · 1 line
2.1.1
PATCH 2/4 iommu: add ARM LPAE page table allocator
Hi Will,
Thank you for the patch.
Quoted 328 lines · On Thursday 27 November 2014 11:51:16 Will Deacon wrote:
A number of IOMMUs found in ARM SoCs can walk architecture-compatible page tables.
This patch adds a generic allocator for Stage-1 and Stage-2 v7/v8 long-descriptor page tables. 4k, 16k and 64k pages are supported, with up to 4-levels of walk to cover a 48-bit address space.
Signed-off-by Will Deacon <[email protected]>--- MAINTAINERS | 1 + drivers/iommu/Kconfig | 9 + drivers/iommu/Makefile | 1 + drivers/iommu/io-pgtable-arm.c | 735 ++++++++++++++++++++++++++++++++++++++ drivers/iommu/io-pgtable.c | 7 + drivers/iommu/io-pgtable.h | 12 + 6 files changed, 765 insertions(+) create mode 100644 drivers/iommu/io-pgtable-arm.cMAINTAINERS +1
-0@@ -1562,6 +1562,7 @@ M: Will Deacon <[email protected]> L: linux-arm-kernel at lists.infradead.org (moderated for non-subscribers) S: Maintained F: drivers/iommu/arm-smmu.c +F: drivers/iommu/io-pgtable-arm.c ARM64 PORT (AARCH64 ARCHITECTURE) M: Catalin Marinas <[email protected]>drivers/iommu/Kconfig +9
-0@@ -19,6 +19,15 @@ menu "Generic IOMMU Pagetable Support" config IOMMU_IO_PGTABLE bool +config IOMMU_IO_PGTABLE_LPAE + bool "ARMv7/v8 Long Descriptor Format" + select IOMMU_IO_PGTABLE + help + Enable support for the ARM long descriptor pagetable format. + This allocator supports 4K/2M/1G, 16K/32M and 64K/512M page + sizes at both stage-1 and stage-2, as well as address spaces + up to 48-bits in size. + endmenu config OF_IOMMUdrivers/iommu/Makefile +1
-0@@ -2,6 +2,7 @@ obj-$(CONFIG_IOMMU_API) += iommu.o obj-$(CONFIG_IOMMU_API) += iommu-traces.o obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o +obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o obj-$(CONFIG_OF_IOMMU) += of_iommu.o obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o msm_iommu_dev.o obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.odrivers/iommu/io-pgtable-arm.c new file +52
-0@@ -0,0 +1,735 @@ +/* +* CPU-agnostic ARM page table allocator. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program. If not, see <http://www.gnu.org/licenses/>. +* +* Copyright (C) 2014 ARM Limited +* +* Author: Will Deacon <[email protected]> +*/ + +#define pr_fmt (fmt) "arm-lpae io-pgtable: " fmt + +#include <linux/iommu.h> +#include <linux/kernel.h> +#include <linux/sizes.h> +#include <linux/slab.h> +#include <linux/types.h> + +#include "io-pgtable.h" + +#define ARM_LPAE_MAX_ADDR_BITS 48 +#define ARM_LPAE_S2_MAX_CONCAT_PAGES 16 +#define ARM_LPAE_MAX_LEVELS 4 + +/* Struct accessors */ +#define io_pgtable_to_data (x) \ + container_of((x), struct arm_lpae_io_pgtable, iop) + +#define io_pgtable_ops_to_pgtable (x) \ + container_of((x), struct io_pgtable, ops) + +#define io_pgtable_ops_to_data (x) \ + io_pgtable_to_data(io_pgtable_ops_to_pgtable(x)) + +/* +* For consistency with the architecture, we always consider +* ARM_LPAE_MAX_LEVELS levels, with the walk starting at level n >=0 +*/ +#define ARM_LPAE_START_LVL (d) (ARM_LPAE_MAX_LEVELS - (d)->levels) + +/* +* Calculate the right shift amount to get to the portion describing level l + * in a virtual address mapped by the pagetable in d.
+ */drivers/iommu/io-pgtable-arm.c new file +163
-0+#define ARM_LPAE_LVL_SHIFT (l,d) \ + ((((d)->levels - ((l) - ARM_LPAE_START_LVL(d) + 1)) \ + * (d)->bits_per_level) + (d)->pg_shift) + +/* +* Calculate the index at level l used to map virtual address a using the +* pagetable in d. +*/ +#define ARM_LPAE_PGD_IDX (l,d) \ + ((l) == ARM_LPAE_START_LVL(d) ? ilog2((d)->pages_per_pgd) : 0) + +#define ARM_LPAE_LVL_IDX (a,l,d) \ + (((a) >> ARM_LPAE_LVL_SHIFT(l,d)) & \ + ((1 << ((d)->bits_per_level + ARM_LPAE_PGD_IDX(l,d))) - 1)) + +/* Calculate the block/page mapping size at level l for pagetable in d. */ +#define ARM_LPAE_BLOCK_SIZE (l,d) \ + (1 << (ilog2(sizeof(arm_lpae_iopte)) + \ + ((ARM_LPAE_MAX_LEVELS - (l)) * (d)->bits_per_level))) + +/* Page table bits */ +#define ARM_LPAE_PTE_TYPE_SHIFT 0 +#define ARM_LPAE_PTE_TYPE_MASK 0x3 + +#define ARM_LPAE_PTE_TYPE_BLOCK 1 +#define ARM_LPAE_PTE_TYPE_TABLE 3 +#define ARM_LPAE_PTE_TYPE_PAGE 3 + +#define ARM_LPAE_PTE_XN (((arm_lpae_iopte)3) << 53) +#define ARM_LPAE_PTE_AF (((arm_lpae_iopte)1) << 10) +#define ARM_LPAE_PTE_SH_NS (((arm_lpae_iopte)0) << 8) +#define ARM_LPAE_PTE_SH_OS (((arm_lpae_iopte)2) << 8) +#define ARM_LPAE_PTE_SH_IS (((arm_lpae_iopte)3) << 8) +#define ARM_LPAE_PTE_VALID (((arm_lpae_iopte)1) << 0) + +#define ARM_LPAE_PTE_ATTR_LO_MASK (((arm_lpae_iopte)0x3ff) << 2) +/* Ignore the contiguous bit for block splitting */ +#define ARM_LPAE_PTE_ATTR_HI_MASK (((arm_lpae_iopte)6) << 52) +#define ARM_LPAE_PTE_ATTR_MASK (ARM_LPAE_PTE_ATTR_LO_MASK | \ + ARM_LPAE_PTE_ATTR_HI_MASK) + +/* Stage-1 PTE */ +#define ARM_LPAE_PTE_AP_UNPRIV (((arm_lpae_iopte)1) << 6) +#define ARM_LPAE_PTE_AP_RDONLY (((arm_lpae_iopte)2) << 6) +#define ARM_LPAE_PTE_ATTRINDX_SHIFT 2 +#define ARM_LPAE_PTE_nG (((arm_lpae_iopte)1) << 11) + +/* Stage-2 PTE */ +#define ARM_LPAE_PTE_HAP_FAULT (((arm_lpae_iopte)0) << 6) +#define ARM_LPAE_PTE_HAP_READ (((arm_lpae_iopte)1) << 6) +#define ARM_LPAE_PTE_HAP_WRITE (((arm_lpae_iopte)2) << 6) +#define ARM_LPAE_PTE_MEMATTR_OIWB (((arm_lpae_iopte)0xf) << 2) +#define ARM_LPAE_PTE_MEMATTR_NC (((arm_lpae_iopte)0x5) << 2) +#define ARM_LPAE_PTE_MEMATTR_DEV (((arm_lpae_iopte)0x1) << 2) + +/* Register bits */ +#define ARM_LPAE_TCR_EAE (1 << 31) + +#define ARM_LPAE_TCR_TG0_4K (0 << 14) +#define ARM_LPAE_TCR_TG0_64K (1 << 14) +#define ARM_LPAE_TCR_TG0_16K (2 << 14) + +#define ARM_LPAE_TCR_SH0_SHIFT 12 +#define ARM_LPAE_TCR_SH0_MASK 0x3 +#define ARM_LPAE_TCR_SH_NS 0 +#define ARM_LPAE_TCR_SH_OS 2 +#define ARM_LPAE_TCR_SH_IS 3 + +#define ARM_LPAE_TCR_ORGN0_SHIFT 10 +#define ARM_LPAE_TCR_IRGN0_SHIFT 8 +#define ARM_LPAE_TCR_RGN_MASK 0x3 +#define ARM_LPAE_TCR_RGN_NC 0 +#define ARM_LPAE_TCR_RGN_WBWA 1 +#define ARM_LPAE_TCR_RGN_WT 2 +#define ARM_LPAE_TCR_RGN_WB 3 + +#define ARM_LPAE_TCR_SL0_SHIFT 6 +#define ARM_LPAE_TCR_SL0_MASK 0x3 + +#define ARM_LPAE_TCR_T0SZ_SHIFT 0 +#define ARM_LPAE_TCR_SZ_MASK 0xf + +#define ARM_LPAE_TCR_PS_SHIFT 16 +#define ARM_LPAE_TCR_PS_MASK 0x7 + +#define ARM_LPAE_TCR_IPS_SHIFT 32 +#define ARM_LPAE_TCR_IPS_MASK 0x7 + +#define ARM_LPAE_TCR_PS_32_BIT 0x0ULL +#define ARM_LPAE_TCR_PS_36_BIT 0x1ULL +#define ARM_LPAE_TCR_PS_40_BIT 0x2ULL +#define ARM_LPAE_TCR_PS_42_BIT 0x3ULL +#define ARM_LPAE_TCR_PS_44_BIT 0x4ULL +#define ARM_LPAE_TCR_PS_48_BIT 0x5ULL + +#define ARM_LPAE_MAIR_ATTR_SHIFT (n) ((n) << 3) +#define ARM_LPAE_MAIR_ATTR_MASK 0xff +#define ARM_LPAE_MAIR_ATTR_DEVICE 0x04 +#define ARM_LPAE_MAIR_ATTR_NC 0x44 +#define ARM_LPAE_MAIR_ATTR_WBRWA 0xff +#define ARM_LPAE_MAIR_ATTR_IDX_NC 0 +#define ARM_LPAE_MAIR_ATTR_IDX_CACHE 1 +#define ARM_LPAE_MAIR_ATTR_IDX_DEV 2 + +/* IOPTE accessors */ +#define iopte_deref (pte,d) \ + (__va((pte) & ((1ULL << ARM_LPAE_MAX_ADDR_BITS) - 1) \ + & ~((1ULL << (d)->pg_shift) - 1))) + +#define iopte_type (pte,l) \ + (((pte) >> ARM_LPAE_PTE_TYPE_SHIFT) & ARM_LPAE_PTE_TYPE_MASK) + +#define iopte_prot (pte) ((pte) & ARM_LPAE_PTE_ATTR_MASK) + +#define iopte_leaf (pte,l) \ + (l == (ARM_LPAE_MAX_LEVELS - 1) ? \ + (iopte_type(pte,l) == ARM_LPAE_PTE_TYPE_PAGE) : \ + (iopte_type(pte,l) == ARM_LPAE_PTE_TYPE_BLOCK)) + +#define iopte_to_pfn (pte,d) \ + (((pte) & ((1ULL << ARM_LPAE_MAX_ADDR_BITS) - 1)) >> (d)->pg_shift) + +#define pfn_to_iopte (pfn,d) \ + (((pfn) << (d)->pg_shift) & ((1ULL << ARM_LPAE_MAX_ADDR_BITS) - 1)) + +struct arm_lpae_io_pgtable { +struct io_pgtable iop; + +int levels; +int pages_per_pgd; +unsigned long pg_shift; +unsigned long bits_per_level; + +void *pgd; +}; + +typedef u64 arm_lpae_iopte ; + +static int arm_lpae_init_pte (struct arm_lpae_io_pgtable *data, +unsigned long iova,phys_addr_t paddr, +arm_lpae_iopte prot,int lvl, +arm_lpae_iopte *ptep) +{ +arm_lpae_iopte pte = prot; + +/* We require an unmap first */ +if (iopte_leaf (*ptep, lvl)) +return -EEXIST ; + +if (lvl ==ARM_LPAE_MAX_LEVELS -1 ) + pte |=ARM_LPAE_PTE_TYPE_PAGE ; +else + pte |=ARM_LPAE_PTE_TYPE_BLOCK ; + + pte |=ARM_LPAE_PTE_AF |ARM_LPAE_PTE_SH_IS ; + pte |=pfn_to_iopte (paddr >> data->pg_shift, data); + + *ptep = pte; + data->iop.cfg.tlb->flush_pgtable (ptep,sizeof (*ptep), data->iop.cookie); +return 0 ; +} + +static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned longiova, +
phys_addr_tpaddr,size_tsize,arm_lpae_iopteprot,drivers/iommu/io-pgtable-arm.c new file +39
-0+ int lvl, arm_lpae_iopte *ptep) +{ + arm_lpae_iopte *cptep, pte; +void *cookie = data->iop.cookie; +size_t block_size =ARM_LPAE_BLOCK_SIZE (lvl, data); + +/* Find our entry at the current level */ + ptep +=ARM_LPAE_LVL_IDX (iova, lvl, data); + +/* If we can install a leaf entry at this level, then do so */ +if (size == block_size && (size & data->iop.cfg.pgsize_bitmap)) +return arm_lpae_init_pte (data, iova, paddr, prot, lvl, ptep); + +/* We can't allocate tables at the final level */ +if (WARN_ON (lvl >=ARM_LPAE_MAX_LEVELS -1 )) +return -EINVAL ; + +/* Grab a pointer to the next level */ + pte = *ptep; +if (!pte) { + cptep =alloc_pages_exact (1UL << data->pg_shift, +GFP_ATOMIC | __GFP_ZERO); +if (!cptep) +return -ENOMEM ; + + data->iop.cfg.tlb->flush_pgtable (cptep,1UL << data->pg_shift, + cookie); + pte =__pa (cptep) |ARM_LPAE_PTE_TYPE_TABLE ; + *ptep = pte; + data->iop.cfg.tlb->flush_pgtable (ptep,sizeof (*ptep), cookie); + }else { + cptep =iopte_deref (pte, data); + } + +/* Rinse, repeat */ +return __arm_lpae_map (data, iova, paddr, size, prot, lvl +1 , cptep); +} + +static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable*data, + int prot)
drivers/iommu/io-pgtable-arm.c new file +2
-0+{ +arm_lpae_iopte pte;
drivers/iommu/io-pgtable-arm.c line 270
Quoted 10 lines
drivers/iommu/io-pgtable-arm.c new file +10
-0+ +if (data->iop.fmt ==ARM_LPAE_S1 ) { + pte =ARM_LPAE_PTE_AP_UNPRIV | ARM_LPAE_PTE_nG; + +if (!(prot &IOMMU_WRITE ) && (prot &IOMMU_READ )) + pte |=ARM_LPAE_PTE_AP_RDONLY ; + +if (prot &IOMMU_CACHE ) + pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE + <<ARM_LPAE_PTE_ATTRINDX_SHIFT );
In my case I'll need to manage the NS bit (here and when allocating tables in __arm_lpae_map). The exact requirements are not exactly clear@the moment I'm afraid, the datasheet doesn't clearly document secure behaviour, but tests showed that setting the NS was necessary.
Given that arm_lpae_init_pte() will unconditionally set the AF and SH_IS bits you could set them here too, but that shouldn't make a big difference.
Quoted 19 lines
drivers/iommu/io-pgtable-arm.c new file +19
-0+ }else { + pte =ARM_LPAE_PTE_HAP_FAULT ; +if (prot &IOMMU_READ ) + pte |=ARM_LPAE_PTE_HAP_READ ; +if (prot &IOMMU_WRITE ) + pte |=ARM_LPAE_PTE_HAP_WRITE ; +if (prot &IOMMU_CACHE ) + pte |=ARM_LPAE_PTE_MEMATTR_OIWB ; +else + pte |=ARM_LPAE_PTE_MEMATTR_NC ; + } + +if (prot &IOMMU_NOEXEC ) + pte |=ARM_LPAE_PTE_XN ; + +return pte; +} + +static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
drivers/iommu/io-pgtable-arm.c line 299
Quoted 10 lines
drivers/iommu/io-pgtable-arm.c new file +10
-0+ phys_addr_t paddr, size_t size, int iommu_prot) +{ + struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops); +arm_lpae_iopte *ptep = data->pgd; +int lvl =ARM_LPAE_START_LVL (data); +arm_lpae_iopte prot; + +/* If no access, then nothing to do */ +if (!(iommu_prot & (IOMMU_READ |IOMMU_WRITE ))) +return 0 ;
Shouldn't this create a faulting entry instead ?
Quoted 310 lines
drivers/iommu/io-pgtable-arm.c new file +5
-0+ prot =arm_lpae_prot_to_pte (data, iommu_prot); +return __arm_lpae_map (data, iova, paddr, size, prot, lvl, ptep); +} + +static void __arm_lpae_free_pgtable(struct arm_lpae_io_pgtable *data, intlvl, +
arm_lpae_iopte*ptep)drivers/iommu/io-pgtable-arm.c new file +248
-0+{ +arm_lpae_iopte *start, *end; +unsigned long table_size; + +/* Only leaf entries at the last level */ +if (lvl ==ARM_LPAE_MAX_LEVELS -1 ) +return ; + + table_size =1UL << data->pg_shift; +if (lvl ==ARM_LPAE_START_LVL (data)) + table_size *= data->pages_per_pgd; + + start = ptep; + end = (void *)ptep + table_size; + +while (ptep != end) { +arm_lpae_iopte pte = *ptep++; + +if (!pte ||iopte_leaf (pte, lvl)) +continue ; + +__arm_lpae_free_pgtable (data, lvl +1 ,iopte_deref (pte, data)); + } + +free_pages_exact (start, table_size); +} + +static void arm_lpae_free_pgtable (struct io_pgtable *iop) +{ +struct arm_lpae_io_pgtable *data =io_pgtable_to_data (iop); + +__arm_lpae_free_pgtable (data,ARM_LPAE_START_LVL (data), data->pgd); +kfree (data); +} + +static int arm_lpae_split_blk_unmap (struct arm_lpae_io_pgtable *data, +unsigned long iova,size_t size, +arm_lpae_iopte prot,int lvl, +arm_lpae_iopte *ptep,size_t blk_size) +{ +unsigned long blk_start, blk_end; +phys_addr_t blk_paddr; +arm_lpae_iopte table =0 ; +void *cookie = data->iop.cookie; +struct iommu_gather_ops *tlb = data->iop.cfg.tlb; + + blk_start = iova & ~(blk_size -1 ); + blk_end = blk_start + blk_size; + blk_paddr =iopte_to_pfn (*ptep, data) << data->pg_shift; + +for (; blk_start < blk_end; blk_start += size, blk_paddr += size) { +arm_lpae_iopte *tablep; + +/* Unmap! */ +if (blk_start == iova) +continue ; + +/* __arm_lpae_map expects a pointer to the start of the table */ + tablep = &table -ARM_LPAE_LVL_IDX (blk_start, lvl, data); +if (__arm_lpae_map (data, blk_start, blk_paddr, size, prot, lvl, + tablep) <0 ) { +if (table) { +/* Free the table we allocated */ + tablep =iopte_deref (table, data); +__arm_lpae_free_pgtable (data, lvl +1 , tablep); + } +return 0 ;/* Bytes unmapped */ + } + } + + *ptep = table; + tlb->flush_pgtable (ptep,sizeof (*ptep), cookie); + iova &= ~(blk_size -1 ); + tlb->tlb_add_flush (iova, blk_size, true, cookie); +return size; +} + +static int __arm_lpae_unmap (struct arm_lpae_io_pgtable *data, +unsigned long iova,size_t size,int lvl, +arm_lpae_iopte *ptep) +{ +arm_lpae_iopte pte; +struct iommu_gather_ops *tlb = data->iop.cfg.tlb; +void *cookie = data->iop.cookie; +size_t blk_size =ARM_LPAE_BLOCK_SIZE (lvl, data); + + ptep +=ARM_LPAE_LVL_IDX (iova, lvl, data); + pte = *ptep; + +/* Something went horribly wrong and we ran out of page table */ +if (WARN_ON (!pte || (lvl ==ARM_LPAE_MAX_LEVELS ))) +return 0 ; + +/* If the size matches this level, we're in the right place */ +if (size == blk_size) { + *ptep =0 ; + tlb->flush_pgtable (ptep,sizeof (*ptep), cookie); + +if (!iopte_leaf (pte, lvl)) { +/* Also flush any partial walks */ + tlb->tlb_add_flush (iova, size, false, cookie); + tlb->tlb_sync (data->iop.cookie); + ptep =iopte_deref (pte, data); +__arm_lpae_free_pgtable (data, lvl +1 , ptep); + }else { + tlb->tlb_add_flush (iova, size, true, cookie); + } + +return size; + }else if (iopte_leaf (pte, lvl)) { +/* +* Insert a table at the next level to map the old region, +* minus the part we want to unmap +*/ +return arm_lpae_split_blk_unmap (data, iova, size, +iopte_prot (pte), lvl, ptep, + blk_size); + } + +/* Keep on walkin' */ + ptep =iopte_deref (pte, data); +return __arm_lpae_unmap (data, iova, size, lvl +1 , ptep); +} + +static int arm_lpae_unmap (struct io_pgtable_ops *ops,unsigned long iova, +size_t size) +{ +size_t unmapped; +struct arm_lpae_io_pgtable *data =io_pgtable_ops_to_data (ops); +struct io_pgtable *iop = &data->iop; +arm_lpae_iopte *ptep = data->pgd; +int lvl =ARM_LPAE_START_LVL (data); + + unmapped =__arm_lpae_unmap (data, iova, size, lvl, ptep); +if (unmapped) + iop->cfg.tlb->tlb_sync (iop->cookie); + +return unmapped; +} + +static phys_addr_t arm_lpae_iova_to_phys (struct io_pgtable_ops *ops, +unsigned long iova) +{ +struct arm_lpae_io_pgtable *data =io_pgtable_ops_to_data (ops); +arm_lpae_iopte pte, *ptep = data->pgd; +int lvl =ARM_LPAE_START_LVL (data); + +do { +/* Valid IOPTE pointer? */ +if (!ptep) +return 0 ; + +/* Grab the IOPTE we're interested in */ + pte = *(ptep +ARM_LPAE_LVL_IDX (iova, lvl, data)); + +/* Valid entry? */ +if (!pte) +return 0 ; + +/* Leaf entry? */ +if (iopte_leaf (pte,lvl)) + goto found_translation; + +/* Take it to the next level */ + ptep =iopte_deref (pte, data); + }while (++lvl <ARM_LPAE_MAX_LEVELS ); + +/* Ran out of page tables to walk */ +return 0 ; + +found_translation: + iova &= ((1 << data->pg_shift) -1 ); +return ((phys_addr_t )iopte_to_pfn (pte,data) << data->pg_shift) | iova; +} + +static void arm_lpae_restrict_pgsizes (struct io_pgtable_cfg *cfg) +{ +unsigned long granule; + +/* +* We need to restrict the supported page sizes to match the +* translation regime for a particular granule. Aim to match +* the CPU page size if possible, otherwise prefer smaller sizes. +* While we're at it, restrict the block sizes to match the +* chosen granule. +*/ +if (cfg->pgsize_bitmap &PAGE_SIZE ) + granule =PAGE_SIZE ; +else if (cfg->pgsize_bitmap & ~PAGE_MASK ) + granule =1UL <<__fls (cfg->pgsize_bitmap & ~PAGE_MASK ); +else if (cfg->pgsize_bitmap &PAGE_MASK ) + granule =1UL <<__ffs (cfg->pgsize_bitmap &PAGE_MASK ); +else + granule =0 ; + +switch (granule) { +case SZ_4K : + cfg->pgsize_bitmap &= (SZ_4K |SZ_2M |SZ_1G ); +break ; +case SZ_16K : + cfg->pgsize_bitmap &= (SZ_16K |SZ_32M ); +break ; +case SZ_64K : + cfg->pgsize_bitmap &= (SZ_64K |SZ_512M ); +break ; +default : + cfg->pgsize_bitmap =0 ; + } +} + +static struct arm_lpae_io_pgtable * +arm_lpae_alloc_pgtable (struct io_pgtable_cfg *cfg) +{ +unsigned long va_bits; +struct arm_lpae_io_pgtable *data; + +arm_lpae_restrict_pgsizes (cfg); + +if (!(cfg->pgsize_bitmap & (SZ_4K |SZ_16K |SZ_64K ))) +return NULL ; + +if (cfg->ias >ARM_LPAE_MAX_ADDR_BITS ) +return NULL ; + +if (cfg->oas >ARM_LPAE_MAX_ADDR_BITS ) +return NULL ; + + data =kmalloc (sizeof (*data),GFP_KERNEL ); +if (!data) +return NULL ; + + data->pages_per_pgd =1 ; + data->pg_shift =__ffs (cfg->pgsize_bitmap); + data->bits_per_level = data->pg_shift -ilog2 (sizeof (arm_lpae_iopte)); + + va_bits = cfg->ias - data->pg_shift; + data->levels =DIV_ROUND_UP (va_bits, data->bits_per_level); + + data->iop.ops = (struct io_pgtable_ops ) { + .map = arm_lpae_map, + .unmap = arm_lpae_unmap, + .iova_to_phys = arm_lpae_iova_to_phys, + }; + +return data; +} + +static struct io_pgtable *arm_lpae_alloc_pgtable_s1(struct io_pgtable_cfg*cfg, + void *cookie)
drivers/iommu/io-pgtable-arm.c new file +55
-0+{ +u64 reg; +struct arm_lpae_io_pgtable *data =arm_lpae_alloc_pgtable (cfg); + +if (!data) +return NULL ; + +/* TCR */ + reg =ARM_LPAE_TCR_EAE | + (ARM_LPAE_TCR_SH_IS <<ARM_LPAE_TCR_SH0_SHIFT ) | + (ARM_LPAE_TCR_RGN_WBWA <<ARM_LPAE_TCR_IRGN0_SHIFT ) | + (ARM_LPAE_TCR_RGN_WBWA <<ARM_LPAE_TCR_ORGN0_SHIFT ); + +switch (1 << data->pg_shift) { +case SZ_4K : + reg |=ARM_LPAE_TCR_TG0_4K ; +break ; +case SZ_16K : + reg |=ARM_LPAE_TCR_TG0_16K ; +break ; +case SZ_64K : + reg |=ARM_LPAE_TCR_TG0_64K ; +break ; + } + +switch (cfg->oas) { +case 32 : + reg |= (ARM_LPAE_TCR_PS_32_BIT <<ARM_LPAE_TCR_IPS_SHIFT ); +break ; +case 36 : + reg |= (ARM_LPAE_TCR_PS_36_BIT <<ARM_LPAE_TCR_IPS_SHIFT ); +break ; +case 40 : + reg |= (ARM_LPAE_TCR_PS_40_BIT <<ARM_LPAE_TCR_IPS_SHIFT ); +break ; +case 42 : + reg |= (ARM_LPAE_TCR_PS_42_BIT <<ARM_LPAE_TCR_IPS_SHIFT ); +break ; +case 44 : + reg |= (ARM_LPAE_TCR_PS_44_BIT <<ARM_LPAE_TCR_IPS_SHIFT ); +break ; +case 48 : + reg |= (ARM_LPAE_TCR_PS_48_BIT <<ARM_LPAE_TCR_IPS_SHIFT ); +break ; +default : + goto out_free_data; + } + + reg |= (64ULL - cfg->ias) <<ARM_LPAE_TCR_T0SZ_SHIFT ; + cfg->arm_lpae_s1_cfg.tcr = reg; + +/* MAIRs */ + reg = (ARM_LPAE_MAIR_ATTR_NC + <<ARM_LPAE_MAIR_ATTR_SHIFT (ARM_LPAE_MAIR_ATTR_IDX_NC )) | + (ARM_LPAE_MAIR_ATTR_WBRWA
drivers/iommu/io-pgtable-arm.c line 620
Quoted 10 lines
drivers/iommu/io-pgtable-arm.c new file +10
-0+ << ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_CACHE)) | + (ARM_LPAE_MAIR_ATTR_DEVICE + <<ARM_LPAE_MAIR_ATTR_SHIFT (ARM_LPAE_MAIR_ATTR_IDX_DEV )); + + cfg->arm_lpae_s1_cfg.mair[0 ] = reg; + cfg->arm_lpae_s1_cfg.mair[1 ] =0 ; + +/* Looking good; allocate a pgd */ + data->pgd =alloc_pages_exact (1UL << data->pg_shift, +GFP_KERNEL | __GFP_ZERO);
data->pg_shift is computed as __ffs(cfg->pgsize_bitmap). 1UL << data->pg_shift will thus be equal to the smallest page size supported by the IOMMU. This will thus allocate 4kB, 16kB or 64kB depending on the IOMMU configuration. However, if I'm not mistaken the top-level directory needs to store one entry per largest supported page size. That's 4, 128 or 8 entries depending on the configuration. You're thus over-allocating.
Quoted 162 lines
drivers/iommu/io-pgtable-arm.c new file +16
-0+if (!data->pgd) + goto out_free_data; + + cfg->tlb->flush_pgtable (data->pgd, (1UL << data->pg_shift), cookie); + +/* TTBRs */ + cfg->arm_lpae_s1_cfg.ttbr[0 ] =virt_to_phys (data->pgd); + cfg->arm_lpae_s1_cfg.ttbr[1 ] =0 ; +return &data->iop; + +out_free_data: +kfree (data); +return NULL ; +} + +static struct io_pgtable *arm_lpae_alloc_pgtable_s2(struct io_pgtable_cfg*cfg,
drivers/iommu/io-pgtable-arm.c new file +99
-0+ void *cookie) +{ +u64 reg, sl; +size_t pgd_size; +struct arm_lpae_io_pgtable *data =arm_lpae_alloc_pgtable (cfg); + +if (!data) +return NULL ; + +/* +* Concatenate PGDs at level 1 if possible in order to reduce +* the depth of the stage-2 walk. +*/ +if (data->levels ==ARM_LPAE_MAX_LEVELS ) { +unsigned long pgd_bits, pgd_pages; +unsigned long va_bits = cfg->ias - data->pg_shift; + + pgd_bits = data->bits_per_level * (data->levels -1 ); + pgd_pages =1 << (va_bits - pgd_bits); +if (pgd_pages <=ARM_LPAE_S2_MAX_CONCAT_PAGES ) { + data->pages_per_pgd = pgd_pages; + data->levels--; + } + } + +/* VTCR */ + reg =ARM_LPAE_TCR_EAE | + (ARM_LPAE_TCR_SH_IS <<ARM_LPAE_TCR_SH0_SHIFT ) | + (ARM_LPAE_TCR_RGN_WBWA <<ARM_LPAE_TCR_IRGN0_SHIFT ) | + (ARM_LPAE_TCR_RGN_WBWA <<ARM_LPAE_TCR_ORGN0_SHIFT ); + + sl =ARM_LPAE_START_LVL (data); + +switch (1 << data->pg_shift) { +case SZ_4K : + reg |=ARM_LPAE_TCR_TG0_4K ; + sl++;/* SL0 format is different for 4K granule size */ +break ; +case SZ_16K : + reg |=ARM_LPAE_TCR_TG0_16K ; +break ; +case SZ_64K : + reg |=ARM_LPAE_TCR_TG0_64K ; +break ; + } + +switch (cfg->oas) { +case 32 : + reg |= (ARM_LPAE_TCR_PS_32_BIT <<ARM_LPAE_TCR_PS_SHIFT ); +break ; +case 36 : + reg |= (ARM_LPAE_TCR_PS_36_BIT <<ARM_LPAE_TCR_PS_SHIFT ); +break ; +case 40 : + reg |= (ARM_LPAE_TCR_PS_40_BIT <<ARM_LPAE_TCR_PS_SHIFT ); +break ; +case 42 : + reg |= (ARM_LPAE_TCR_PS_42_BIT <<ARM_LPAE_TCR_PS_SHIFT ); +break ; +case 44 : + reg |= (ARM_LPAE_TCR_PS_44_BIT <<ARM_LPAE_TCR_PS_SHIFT ); +break ; +case 48 : + reg |= (ARM_LPAE_TCR_PS_48_BIT <<ARM_LPAE_TCR_PS_SHIFT ); +break ; +default : + goto out_free_data; + } + + reg |= (64ULL - cfg->ias) <<ARM_LPAE_TCR_T0SZ_SHIFT ; + reg |= (~sl &ARM_LPAE_TCR_SL0_MASK ) <<ARM_LPAE_TCR_SL0_SHIFT ; + cfg->arm_lpae_s2_cfg.vtcr = reg; + +/* Allocate pgd pages */ + pgd_size = data->pages_per_pgd * (1UL << data->pg_shift); + data->pgd =alloc_pages_exact (pgd_size,GFP_KERNEL | __GFP_ZERO); +if (!data->pgd) + goto out_free_data; + + cfg->tlb->flush_pgtable (data->pgd, pgd_size, cookie); + +/* VTTBR */ + cfg->arm_lpae_s2_cfg.vttbr =virt_to_phys (data->pgd); +return &data->iop; + +out_free_data: +kfree (data); +return NULL ; +} + +struct io_pgtable_init_fns io_pgtable_arm_lpae_s1_init_fns = { + .alloc = arm_lpae_alloc_pgtable_s1, + .free = arm_lpae_free_pgtable, +}; + +struct io_pgtable_init_fns io_pgtable_arm_lpae_s2_init_fns = { + .alloc = arm_lpae_alloc_pgtable_s2, + .free = arm_lpae_free_pgtable, +};drivers/iommu/io-pgtable.c +7
-0@@ -25,8 +25,15 @@#include "io-pgtable.h" +extern struct io_pgtable_init_fns io_pgtable_arm_lpae_s1_init_fns; +extern struct io_pgtable_init_fns io_pgtable_arm_lpae_s2_init_fns; +static struct io_pgtable_init_fns *io_pgtable_init_table[IO_PGTABLE_NUM_FMTS ] = { +#ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE + [ARM_LPAE_S1 ] = &io_pgtable_arm_lpae_s1_init_fns, + [ARM_LPAE_S2 ] = &io_pgtable_arm_lpae_s2_init_fns, +#endif }; struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt,drivers/iommu/io-pgtable.h +12
-0@@ -33,10 +33,22 @@ struct io_pgtable_cfg {/* Low-level data specific to the table format */ union { +struct { +u64 ttbr[2 ]; +u64 tcr; +u64 mair[2 ]; + } arm_lpae_s1_cfg; + +struct { +u64 vttbr; +u64 vtcr; + } arm_lpae_s2_cfg; }; };enum io_pgtable_fmt { +ARM_LPAE_S1 , +ARM_LPAE_S2 ,IO_PGTABLE_NUM_FMTS , };
Signature · 3 lines
Regards, Laurent Pinchart
PATCH 2/4 iommu: add ARM LPAE page table allocator
Quoted 1 line · Laurent Pinchart
Hi Will,
Hello again,
Quoted 12 lines · On Thursday 27 November 2014 11:51:16 Will Deacon wrote:
Quoted 11 lines
+static
arm_lpae_ioptearm_lpae_prot_to_pte(structarm_lpae_io_pgtable
*data, + int prot)drivers/iommu/io-pgtable-arm.c new file +9
-0+{ +arm_lpae_iopte pte; + +if (data->iop.fmt ==ARM_LPAE_S1 ) { + pte =ARM_LPAE_PTE_AP_UNPRIV | ARM_LPAE_PTE_nG; + +if (!(prot &IOMMU_WRITE ) && (prot &IOMMU_READ )) + pte |=ARM_LPAE_PTE_AP_RDONLY ; +
Quoted 8 lines
drivers/iommu/io-pgtable-arm.c line 270
Quoted 3 lines
drivers/iommu/io-pgtable-arm.c new file +3
-0+if (prot &IOMMU_CACHE ) + pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE + <<ARM_LPAE_PTE_ATTRINDX_SHIFT );In my case I'll need to manage the NS bit (here and when allocating tables in
__arm_lpae_map). The exact requirements are not exactly clear at the moment I'm afraid, the datasheet doesn't clearly document secure behaviour, but tests showed that setting the NS was necessary.
Hurrah! You can add a quick to the currently unused quirks field that I have in io_pgtable_cfg :)
Quoted 2 lines
Given that
arm_lpae_init_pte()will unconditionally set the AF andSH_ISbits you could set them here too, but that shouldn't make a big difference.
I prefer to keep only the protection bits handled here (i.e. those bits that map directly to the IOMMU_* prot flags).
Quoted 23 lines
Quoted 23 lines
drivers/iommu/io-pgtable-arm.c new file +23
-0+ }else { + pte =ARM_LPAE_PTE_HAP_FAULT ; +if (prot &IOMMU_READ ) + pte |=ARM_LPAE_PTE_HAP_READ ; +if (prot &IOMMU_WRITE ) + pte |=ARM_LPAE_PTE_HAP_WRITE ; +if (prot &IOMMU_CACHE ) + pte |=ARM_LPAE_PTE_MEMATTR_OIWB ; +else + pte |=ARM_LPAE_PTE_MEMATTR_NC ; + } + +if (prot &IOMMU_NOEXEC ) + pte |=ARM_LPAE_PTE_XN ; + +return pte; +} + +static int arm_lpae_map (struct io_pgtable_ops *ops,unsigned long iova, +phys_addr_t paddr,size_t size,int iommu_prot) +{ +struct arm_lpae_io_pgtable *data =io_pgtable_ops_to_data (ops); +arm_lpae_iopte *ptep = data->pgd;
Quoted 8 lines
drivers/iommu/io-pgtable-arm.c line 299
Quoted 6 lines
drivers/iommu/io-pgtable-arm.c new file +6
-0+int lvl =ARM_LPAE_START_LVL (data); +arm_lpae_iopte prot; + +/* If no access, then nothing to do */ +if (!(iommu_prot & (IOMMU_READ |IOMMU_WRITE ))) +return 0 ;Shouldn't this create a faulting entry instead ?
That's effectively what it does. Calling iommu_map on something that's already mapped is a programming error, so therefore we know that the entry is already faulting by virtue of it being unmapped.
Quoted 66 lines
Quoted 66 lines
+static struct
io_pgtable*arm_lpae_alloc_pgtable_s1(structio_pgtable_cfg
*cfg, + void *cookie)drivers/iommu/io-pgtable-arm.c new file +64
-0+{ +u64 reg; +struct arm_lpae_io_pgtable *data =arm_lpae_alloc_pgtable (cfg); + +if (!data) +return NULL ; + +/* TCR */ + reg =ARM_LPAE_TCR_EAE | + (ARM_LPAE_TCR_SH_IS <<ARM_LPAE_TCR_SH0_SHIFT ) | + (ARM_LPAE_TCR_RGN_WBWA <<ARM_LPAE_TCR_IRGN0_SHIFT ) | + (ARM_LPAE_TCR_RGN_WBWA <<ARM_LPAE_TCR_ORGN0_SHIFT ); + +switch (1 << data->pg_shift) { +case SZ_4K : + reg |=ARM_LPAE_TCR_TG0_4K ; +break ; +case SZ_16K : + reg |=ARM_LPAE_TCR_TG0_16K ; +break ; +case SZ_64K : + reg |=ARM_LPAE_TCR_TG0_64K ; +break ; + } + +switch (cfg->oas) { +case 32 : + reg |= (ARM_LPAE_TCR_PS_32_BIT <<ARM_LPAE_TCR_IPS_SHIFT ); +break ; +case 36 : + reg |= (ARM_LPAE_TCR_PS_36_BIT <<ARM_LPAE_TCR_IPS_SHIFT ); +break ; +case 40 : + reg |= (ARM_LPAE_TCR_PS_40_BIT <<ARM_LPAE_TCR_IPS_SHIFT ); +break ; +case 42 : + reg |= (ARM_LPAE_TCR_PS_42_BIT <<ARM_LPAE_TCR_IPS_SHIFT ); +break ; +case 44 : + reg |= (ARM_LPAE_TCR_PS_44_BIT <<ARM_LPAE_TCR_IPS_SHIFT ); +break ; +case 48 : + reg |= (ARM_LPAE_TCR_PS_48_BIT <<ARM_LPAE_TCR_IPS_SHIFT ); +break ; +default : + goto out_free_data; + } + + reg |= (64ULL - cfg->ias) <<ARM_LPAE_TCR_T0SZ_SHIFT ; + cfg->arm_lpae_s1_cfg.tcr = reg; + +/* MAIRs */ + reg = (ARM_LPAE_MAIR_ATTR_NC + <<ARM_LPAE_MAIR_ATTR_SHIFT (ARM_LPAE_MAIR_ATTR_IDX_NC )) | + (ARM_LPAE_MAIR_ATTR_WBRWA + <<ARM_LPAE_MAIR_ATTR_SHIFT (ARM_LPAE_MAIR_ATTR_IDX_CACHE )) | + (ARM_LPAE_MAIR_ATTR_DEVICE + <<ARM_LPAE_MAIR_ATTR_SHIFT (ARM_LPAE_MAIR_ATTR_IDX_DEV )); + + cfg->arm_lpae_s1_cfg.mair[0 ] = reg; + cfg->arm_lpae_s1_cfg.mair[1 ] =0 ; + +/* Looking good; allocate a pgd */ + data->pgd = alloc_pages_exact(1UL << data->pg_shift,
Quoted 8 lines
Quoted 1 line
+
GFP_KERNEL|__GFP_ZERO);
data->pg_shiftis computed as__ffs(cfg->pgsize_bitmap). 1UL <<data->pg_shiftwill thus be equal to the smallest page size supported by the IOMMU. This will thus allocate 4kB, 16kB or 64kB depending on the IOMMU configuration. However, if I'm not mistaken the top-level directory needs to store one entry per largest supported page size. That's 4, 128 or 8 entries depending on the configuration. You're thus over-allocating.
Yeah, I'll take a closer look at this. The size of the pgd really depends on the TxSZ configuration, which in turn depends on the ias and the page size. There are also alignment constraints to bear in mind, but I'll see what I can do (as it stands, over-allocating will always work).
Thanks,
Will
PATCH 2/4 iommu: add ARM LPAE page table allocator
Hi Will,
Quoted 26 lines · On Monday 01 December 2014 17:23:15 Will Deacon wrote:
Quoted 1 line · Laurent Pinchart
Hi Will,
Hello again,
Quoted 12 lines · On Thursday 27 November 2014 11:51:16 Will Deacon wrote:
Quoted 11 lines
+static
arm_lpae_ioptearm_lpae_prot_to_pte(structarm_lpae_io_pgtable
*data, + int prot)drivers/iommu/io-pgtable-arm.c new file +9
-0+{ +arm_lpae_iopte pte; + +if (data->iop.fmt ==ARM_LPAE_S1 ) { + pte =ARM_LPAE_PTE_AP_UNPRIV | ARM_LPAE_PTE_nG; + +if (!(prot &IOMMU_WRITE ) && (prot &IOMMU_READ )) + pte |=ARM_LPAE_PTE_AP_RDONLY ; +Quoted 8 lines
drivers/iommu/io-pgtable-arm.c line 270
Quoted 3 lines
drivers/iommu/io-pgtable-arm.c new file +3
-0+if (prot &IOMMU_CACHE ) + pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE + <<ARM_LPAE_PTE_ATTRINDX_SHIFT );In my case I'll need to manage the NS bit (here and when allocating tables in
__arm_lpae_map). The exact requirements are not exactly clear at the moment I'm afraid, the datasheet doesn't clearly document secure behaviour, but tests showed that setting the NS was necessary.
Quoted 2 lines
Hurrah! You can add a quick to the currently unused quirks field that I have in
io_pgtable_cfg:)
:-)
Quoted 6 lines
Quoted 3 lines
Given that
arm_lpae_init_pte()will unconditionally set the AF andSH_ISbits you could set them here too, but that shouldn't make a big difference.I prefer to keep only the protection bits handled here (i.e. those bits that map directly to the IOMMU_* prot flags).
I thought so. That's fine with me.
Quoted 32 lines
Quoted 23 lines
Quoted 23 lines
drivers/iommu/io-pgtable-arm.c new file +23
-0+ }else { + pte =ARM_LPAE_PTE_HAP_FAULT ; +if (prot &IOMMU_READ ) + pte |=ARM_LPAE_PTE_HAP_READ ; +if (prot &IOMMU_WRITE ) + pte |=ARM_LPAE_PTE_HAP_WRITE ; +if (prot &IOMMU_CACHE ) + pte |=ARM_LPAE_PTE_MEMATTR_OIWB ; +else + pte |=ARM_LPAE_PTE_MEMATTR_NC ; + } + +if (prot &IOMMU_NOEXEC ) + pte |=ARM_LPAE_PTE_XN ; + +return pte; +} + +static int arm_lpae_map (struct io_pgtable_ops *ops,unsigned long iova, +phys_addr_t paddr,size_t size,int iommu_prot) +{ +struct arm_lpae_io_pgtable *data =io_pgtable_ops_to_data (ops); +arm_lpae_iopte *ptep = data->pgd;Quoted 8 lines
drivers/iommu/io-pgtable-arm.c line 299
Quoted 6 lines
drivers/iommu/io-pgtable-arm.c new file +6
-0+int lvl =ARM_LPAE_START_LVL (data); +arm_lpae_iopte prot; + +/* If no access, then nothing to do */ +if (!(iommu_prot & (IOMMU_READ |IOMMU_WRITE ))) +return 0 ;Shouldn't this create a faulting entry instead ?
Quoted 3 lines
That's effectively what it does. Calling
iommu_mapon something that's already mapped is a programming error, so therefore we know that the entry is already faulting by virtue of it being unmapped.
Indeed.
Quoted 77 lines
Quoted 68 lines
Quoted 68 lines
+static struct
io_pgtable*arm_lpae_alloc_pgtable_s1(structio_pgtable_cfg
*cfg, + void *cookie)drivers/iommu/io-pgtable-arm.c new file +56
-0+{ +u64 reg; +struct arm_lpae_io_pgtable *data =arm_lpae_alloc_pgtable (cfg); + +if (!data) +return NULL ; + +/* TCR */ + reg =ARM_LPAE_TCR_EAE | + (ARM_LPAE_TCR_SH_IS <<ARM_LPAE_TCR_SH0_SHIFT ) | + (ARM_LPAE_TCR_RGN_WBWA <<ARM_LPAE_TCR_IRGN0_SHIFT ) | + (ARM_LPAE_TCR_RGN_WBWA <<ARM_LPAE_TCR_ORGN0_SHIFT ); + +switch (1 << data->pg_shift) { +case SZ_4K : + reg |=ARM_LPAE_TCR_TG0_4K ; +break ; +case SZ_16K : + reg |=ARM_LPAE_TCR_TG0_16K ; +break ; +case SZ_64K : + reg |=ARM_LPAE_TCR_TG0_64K ; +break ; + } + +switch (cfg->oas) { +case 32 : + reg |= (ARM_LPAE_TCR_PS_32_BIT <<ARM_LPAE_TCR_IPS_SHIFT ); +break ; +case 36 : + reg |= (ARM_LPAE_TCR_PS_36_BIT <<ARM_LPAE_TCR_IPS_SHIFT ); +break ; +case 40 : + reg |= (ARM_LPAE_TCR_PS_40_BIT <<ARM_LPAE_TCR_IPS_SHIFT ); +break ; +case 42 : + reg |= (ARM_LPAE_TCR_PS_42_BIT <<ARM_LPAE_TCR_IPS_SHIFT ); +break ; +case 44 : + reg |= (ARM_LPAE_TCR_PS_44_BIT <<ARM_LPAE_TCR_IPS_SHIFT ); +break ; +case 48 : + reg |= (ARM_LPAE_TCR_PS_48_BIT <<ARM_LPAE_TCR_IPS_SHIFT ); +break ; +default : + goto out_free_data; + } + + reg |= (64ULL - cfg->ias) <<ARM_LPAE_TCR_T0SZ_SHIFT ; + cfg->arm_lpae_s1_cfg.tcr = reg; + +/* MAIRs */ + reg = (ARM_LPAE_MAIR_ATTR_NC + <<ARM_LPAE_MAIR_ATTR_SHIFT (ARM_LPAE_MAIR_ATTR_IDX_NC )) | + (ARM_LPAE_MAIR_ATTR_WBRWA + <<ARM_LPAE_MAIR_ATTR_SHIFT (ARM_LPAE_MAIR_ATTR_IDX_CACHE ))|
drivers/iommu/io-pgtable-arm.c new file +8
-0+ (ARM_LPAE_MAIR_ATTR_DEVICE + <<ARM_LPAE_MAIR_ATTR_SHIFT (ARM_LPAE_MAIR_ATTR_IDX_DEV )); + + cfg->arm_lpae_s1_cfg.mair[0 ] = reg; + cfg->arm_lpae_s1_cfg.mair[1 ] =0 ; + +/* Looking good; allocate a pgd */ + data->pgd = alloc_pages_exact(1UL << data->pg_shift,Quoted 8 lines
Quoted 1 line
+
GFP_KERNEL|__GFP_ZERO);
data->pg_shiftis computed as__ffs(cfg->pgsize_bitmap). 1UL <<data->pg_shiftwill thus be equal to the smallest page size supported by the IOMMU. This will thus allocate 4kB, 16kB or 64kB depending on the IOMMU configuration. However, if I'm not mistaken the top-level directory needs to store one entry per largest supported page size. That's 4, 128 or 8 entries depending on the configuration. You're thus over-allocating.
Quoted 4 lines
Yeah, I'll take a closer look at this. The size of the pgd really depends on the TxSZ configuration, which in turn depends on the ias and the page size. There are also alignment constraints to bear in mind, but I'll see what I can do (as it stands, over-allocating will always work).
Beside wasting memory, the code also doesn't reflect the requirements. It works by chance, meaning it could break later. That's why I'd like to see this being fixed. Can't the size be computed with something like
size = (1 << (ias - data->levels * data->pg_shift)) * sizeof(arm_lpae_iopte);
(please add a proper detailed comment to explain the computation, as the meaning is not straightforward)
There might be some corner cases I'm not aware of.
Signature · 3 lines
Regards, Laurent Pinchart
PATCH 2/4 iommu: add ARM LPAE page table allocator
Quoted 14 lines · Laurent Pinchart
Quoted 13 lines · On Monday 01 December 2014 17:23:15 Will Deacon wrote:
Quoted 11 lines · Laurent Pinchart
drivers/iommu/io-pgtable-arm.c line 620
Quoted 3 lines · On Thursday 27 November 2014 11:51:16 Will Deacon wrote:
drivers/iommu/io-pgtable-arm.c new file +3
-0+/* Looking good; allocate a pgd */ + data->pgd =alloc_pages_exact (1UL << data->pg_shift, +GFP_KERNEL | __GFP_ZERO);
data->pg_shiftis computed as__ffs(cfg->pgsize_bitmap). 1UL <<data->pg_shiftwill thus be equal to the smallest page size supported by the IOMMU. This will thus allocate 4kB, 16kB or 64kB depending on the IOMMU configuration. However, if I'm not mistaken the top-level directory needs to store one entry per largest supported page size. That's 4, 128 or 8 entries depending on the configuration. You're thus over-allocating.
Quoted 7 lines
Quoted 4 lines
Yeah, I'll take a closer look at this. The size of the pgd really depends on the TxSZ configuration, which in turn depends on the ias and the page size. There are also alignment constraints to bear in mind, but I'll see what I can do (as it stands, over-allocating will always work).
Beside wasting memory, the code also doesn't reflect the requirements. It works by chance, meaning it could break later.
It won't break, as the maximum size *is* bounded by a page for stage-1 and we already handle stage-2 concatenation correctly.
Quoted 8 lines
That's why I'd like to see this
being fixed. Can't the size be computed with something likesize = (1 << (ias - data->levels * data->pg_shift)) * sizeof(arm_lpae_iopte);(please add a proper detailed comment to explain the computation, as the meaning is not straightforward)
That's actually the easy part. The harder part is getting the correct alignment, which means managing by own kmem_cache on a per-ops basis. That feels like overkill to me and we also need to make sure that we don't screw up the case of concatenated pgds at stage-2. On top of that, since each cache would be per-ops, I'm not even sure we'd save anything (the slab allocators all operate on pages afaict).
If I use alloc_page_exact, we'll still have some wasteage, but it would be less for the case where the CPU page size is smaller than the SMMU page size. Do you think that's worth the extra complexity? We allocate full pages at all levels after the pgd, so the wasteage is relatively small.
An alternative would be preinitialising some caches for `likely' pgd sizes, but that's also horrible, especially if the kernel decides that it doesn't need a bunch of the configurations at runtime.
Will
PATCH 2/4 iommu: add ARM LPAE page table allocator
Hi Will,
Quoted 21 lines · On Tuesday 02 December 2014 09:41:56 Will Deacon wrote:
Quoted 20 lines · Laurent Pinchart
Quoted 18 lines · On Monday 01 December 2014 17:23:15 Will Deacon wrote:
Quoted 5 lines · Laurent Pinchart
drivers/iommu/io-pgtable-arm.c line 620
Quoted 3 lines · On Thursday 27 November 2014 11:51:16 Will Deacon wrote:
drivers/iommu/io-pgtable-arm.c new file +3
-0+/* Looking good; allocate a pgd */ + data->pgd =alloc_pages_exact (1UL << data->pg_shift, +GFP_KERNEL | __GFP_ZERO);Quoted 7 lines
data->pg_shiftis computed as__ffs(cfg->pgsize_bitmap). 1UL <<data->pg_shiftwill thus be equal to the smallest page size supported by the IOMMU. This will thus allocate 4kB, 16kB or 64kB depending on the IOMMU configuration. However, if I'm not mistaken the top-level directory needs to store one entry per largest supported page size. That's 4, 128 or 8 entries depending on the configuration. You're thus over-allocating.Yeah, I'll take a closer look at this. The size of the pgd really depends on the TxSZ configuration, which in turn depends on the ias and the page size. There are also alignment constraints to bear in mind, but I'll see what I can do (as it stands, over-allocating will always work).
Quoted 5 lines
Quoted 2 lines
Beside wasting memory, the code also doesn't reflect the requirements. It works by chance, meaning it could break later.
It won't break, as the maximum size *is* bounded by a page for stage-1 and we already handle stage-2 concatenation correctly.
What I mean is that there's no correlation between the required size and the allocated size in the current code. It happens to work, but if the driver gets extended later to support more IOMMU configurations subtle bugs may crop up.
Quoted 17 lines
Quoted 9 lines
That's why I'd like to see this
being fixed. Can't the size be computed with something likesize = (1 << (ias - data->levels * data->pg_shift)) * sizeof(arm_lpae_iopte);(please add a proper detailed comment to explain the computation, as the meaning is not straightforward)
That's actually the easy part. The harder part is getting the correct alignment, which means managing by own
kmem_cacheon a per-ops basis. That feels like overkill to me and we also need to make sure that we don't screw up the case of concatenated pgds at stage-2. On top of that, since each cache would be per-ops, I'm not even sure we'd save anything (the slab allocators all operate on pages afaict).
Quoted 8 lines
If I use
alloc_page_exact, we'll still have some wasteage, but it would be less for the case where the CPU page size is smaller than the SMMU page size. Do you think that's worth the extra complexity? We allocate full pages at all levels after the pgd, so the wasteage is relatively small.An alternative would be preinitialising some caches for `likely' pgd sizes, but that's also horrible, especially if the kernel decides that it doesn't need a bunch of the configurations at runtime.
How about just computing the right size, align it to a page size, and using alloc_page_exact ? The waste is small, so it doesn't justify anything more complex than that.
Signature · 3 lines
Regards, Laurent Pinchart
PATCH 2/4 iommu: add ARM LPAE page table allocator
Quoted 54 lines · Laurent Pinchart
Quoted 21 lines · On Tuesday 02 December 2014 09:41:56 Will Deacon wrote:
Quoted 20 lines · Laurent Pinchart
Quoted 18 lines · On Monday 01 December 2014 17:23:15 Will Deacon wrote:
Quoted 5 lines · Laurent Pinchart
drivers/iommu/io-pgtable-arm.c line 620
Quoted 3 lines · On Thursday 27 November 2014 11:51:16 Will Deacon wrote:
drivers/iommu/io-pgtable-arm.c new file +3
-0+/* Looking good; allocate a pgd */ + data->pgd =alloc_pages_exact (1UL << data->pg_shift, +GFP_KERNEL | __GFP_ZERO);Quoted 7 lines
data->pg_shiftis computed as__ffs(cfg->pgsize_bitmap). 1UL <<data->pg_shiftwill thus be equal to the smallest page size supported by the IOMMU. This will thus allocate 4kB, 16kB or 64kB depending on the IOMMU configuration. However, if I'm not mistaken the top-level directory needs to store one entry per largest supported page size. That's 4, 128 or 8 entries depending on the configuration. You're thus over-allocating.Yeah, I'll take a closer look at this. The size of the pgd really depends on the TxSZ configuration, which in turn depends on the ias and the page size. There are also alignment constraints to bear in mind, but I'll see what I can do (as it stands, over-allocating will always work).
Quoted 5 lines
Quoted 2 lines
Beside wasting memory, the code also doesn't reflect the requirements. It works by chance, meaning it could break later.
It won't break, as the maximum size *is* bounded by a page for stage-1 and we already handle stage-2 concatenation correctly.
What I mean is that there's no correlation between the required size and the allocated size in the current code. It happens to work, but if the driver gets extended later to support more IOMMU configurations subtle bugs may crop up.
Quoted 22 lines
Quoted 9 lines
That's why I'd like to see this
being fixed. Can't the size be computed with something likesize = (1 << (ias - data->levels * data->pg_shift)) * sizeof(arm_lpae_iopte);(please add a proper detailed comment to explain the computation, as the meaning is not straightforward)
That's actually the easy part. The harder part is getting the correct alignment, which means managing by own
kmem_cacheon a per-ops basis. That feels like overkill to me and we also need to make sure that we don't screw up the case of concatenated pgds at stage-2. On top of that, since each cache would be per-ops, I'm not even sure we'd save anything (the slab allocators all operate on pages afaict).If I use
alloc_page_exact, we'll still have some wasteage, but it would be less for the case where the CPU page size is smaller than the SMMU page size. Do you think that's worth the extra complexity? We allocate full pages at all levels after the pgd, so the wasteage is relatively small.
Quoted 7 lines
Quoted 3 lines
An alternative would be preinitialising some caches for `likely' pgd sizes, but that's also horrible, especially if the kernel decides that it doesn't need a bunch of the configurations at runtime.
How about just computing the right size, align it to a page size, and using
alloc_page_exact? The waste is small, so it doesn't justify anything more complex than that.
Ok, I'll have a go at that.
Will
PATCH 2/4 iommu: add ARM LPAE page table allocator
In reply to Will DeaconQuoted 9 lines · Will Deacon
A number of IOMMUs found in ARM SoCs can walk architecture-compatible page tables.
This patch adds a generic allocator for Stage-1 and Stage-2 v7/v8 long-descriptor page tables. 4k, 16k and 64k pages are supported, with up to 4-levels of walk to cover a 48-bit address space.
Signed-off-by Will Deacon <[email protected]>---
[...]
Quoted 4 lines
drivers/iommu/io-pgtable-arm.c new file +4
-0+static struct io_pgtable *arm_lpae_alloc_pgtable_s1 (struct io_pgtable_cfg *cfg, +void *cookie) +{ +u64 reg;
drivers/iommu/io-pgtable-arm.c line 567
Quoted 10 lines
drivers/iommu/io-pgtable-arm.c new file +10
-0+struct arm_lpae_io_pgtable *data =arm_lpae_alloc_pgtable (cfg); + +if (!data) +return NULL ; + +/* TCR */ + reg =ARM_LPAE_TCR_EAE | + (ARM_LPAE_TCR_SH_IS <<ARM_LPAE_TCR_SH0_SHIFT ) | + (ARM_LPAE_TCR_RGN_WBWA <<ARM_LPAE_TCR_IRGN0_SHIFT ) | + (ARM_LPAE_TCR_RGN_WBWA <<ARM_LPAE_TCR_ORGN0_SHIFT );
TCR has different definitions depending on whether we're using v7l or v8l. For example, bit 31 is TG1[1] (not EAE) when CBA2R.VA64=1. Are we expecting to have an io-pgtable-arm64.c or something? Seems like that would be mostly redundant with this file... (We have this problem in the current arm-smmu driver today).
-Mitch
Signature · 3 lines
Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH 2/4 iommu: add ARM LPAE page table allocator
Quoted 25 lines · Mitchel Humpherys
Quoted 9 lines · Will Deacon
A number of IOMMUs found in ARM SoCs can walk architecture-compatible page tables.
This patch adds a generic allocator for Stage-1 and Stage-2 v7/v8 long-descriptor page tables. 4k, 16k and 64k pages are supported, with up to 4-levels of walk to cover a 48-bit address space.
Signed-off-by Will Deacon <[email protected]>---
[...]
Quoted 12 lines
drivers/iommu/io-pgtable-arm.c new file +12
-0+static struct io_pgtable *arm_lpae_alloc_pgtable_s1 (struct io_pgtable_cfg *cfg, +void *cookie) +{ +u64 reg; +struct arm_lpae_io_pgtable *data =arm_lpae_alloc_pgtable (cfg); + +if (!data) +return NULL ; + +/* TCR */ + reg =ARM_LPAE_TCR_EAE | + (ARM_LPAE_TCR_SH_IS << ARM_LPAE_TCR_SH0_SHIFT) |
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ (ARM_LPAE_TCR_RGN_WBWA << ARM_LPAE_TCR_IRGN0_SHIFT) | + (ARM_LPAE_TCR_RGN_WBWA << ARM_LPAE_TCR_ORGN0_SHIFT);TCR has different definitions depending on whether we're using v7l or v8l. For example, bit 31 is TG1[1] (not EAE) when CBA2R.VA64=1. Are we expecting to have an
io-pgtable-arm64.cor something? Seems like that would be mostly redundant with this file... (We have this problem in the current arm-smmu driver today).
I guess we can add an ARM_LPAE_V7_S1 format or something for this, although we don't actually use TTBR1, so the current code shouldn't cause any issues.
Will
PATCH 2/4 iommu: add ARM LPAE page table allocator
In reply to Will DeaconHi Will,
Please find my comments inline. Search for "varun"
-----Original Message-----
From: Will Deacon [mailto:will.deacon at arm.com]
Sent: Thursday, November 27, 2014 5:21 PM
To: linux-arm-kernel at lists.infradead.org; iommu at lists.linux-foundation.org
Cc: prem.mallappa at broadcom.com; Robin.Murphy at arm.com; lauraa at codeaurora.org; mitchelh at codeaurora.org; laurent.pinchart at ideasonboard.com; joro at 8bytes.org; Sethi Varun-B16395; m.szyprowski at samsung.com; Will Deacon
Subject: [PATCH 2/4] iommu: add ARM LPAE page table allocator
A number of IOMMUs found in ARM SoCs can walk architecture-compatible page tables.
This patch adds a generic allocator for Stage-1 and Stage-2 v7/v8 long-descriptor page tables. 4k, 16k and 64k pages are supported, with up to 4-levels of walk to cover a 48-bit address space.
--- MAINTAINERS | 1 + drivers/iommu/Kconfig | 9 + drivers/iommu/Makefile | 1 + drivers/iommu/io-pgtable-arm.c | 735 +++++++++++++++++++++++++++++++++++++++++ drivers/iommu/io-pgtable.c | 7 + drivers/iommu/io-pgtable.h | 12 + 6 files changed, 765 insertions(+) create mode 100644 drivers/iommu/io-pgtable-arm.c
MAINTAINERS +1 -0
@@ -1562,6 +1562,7 @@ M: Will Deacon <[email protected]> L: linux-arm-kernel at lists.infradead.org (moderated for non-subscribers) S: Maintained F: drivers/iommu/arm-smmu.c +F: drivers/iommu/io-pgtable-arm.c ARM64 PORT (AARCH64 ARCHITECTURE) M: Catalin Marinas <[email protected]>
drivers/iommu/Kconfig +9 -0
@@ -19,6 +19,15 @@ menu "Generic IOMMU Pagetable Support" config IOMMU_IO_PGTABLE bool +config IOMMU_IO_PGTABLE_LPAE + bool "ARMv7/v8 Long Descriptor Format" + select IOMMU_IO_PGTABLE + help + Enable support for the ARM long descriptor pagetable format. + This allocator supports 4K/2M/1G, 16K/32M and 64K/512M page + sizes at both stage-1 and stage-2, as well as address spaces + up to 48-bits in size. + endmenu config OF_IOMMU
drivers/iommu/Makefile +1 -0
@@ -2,6 +2,7 @@ obj-$(CONFIG_IOMMU_API) += iommu.o obj-$(CONFIG_IOMMU_API) += iommu-traces.o obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o +obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o obj-$(CONFIG_OF_IOMMU) += of_iommu.o obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o msm_iommu_dev.o obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c new file mode 100644 index 000000000000..9dbaa2e48424
drivers/iommu/io-pgtable-arm.c new file +203 -0
203 lines not on this page · raw message
[varun] Instead of returning an error, how about displaying a warning and replacing the entry?
+
+ if (lvl == ARM_LPAE_MAX_LEVELS - 1)
+ pte |= ARM_LPAE_PTE_TYPE_PAGE;
+ else
+ pte |= ARM_LPAE_PTE_TYPE_BLOCK;
+
+ pte |= ARM_LPAE_PTE_AF | ARM_LPAE_PTE_SH_IS;
+ pte |= pfn_to_iopte(paddr >> data->pg_shift, data);
+
+ *ptep = pte;
+ data->iop.cfg.tlb->flush_pgtable(ptep, sizeof(*ptep), data->iop.cookie);
+ return 0;
+}
+
+static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long iova,
+ phys_addr_t paddr, size_t size, arm_lpae_iopte prot,
+ int lvl, arm_lpae_iopte *ptep)
+{
+ arm_lpae_iopte *cptep, pte;
+ void *cookie = data->iop.cookie;
+ size_t block_size = ARM_LPAE_BLOCK_SIZE(lvl, data);
+
+ /* Find our entry at the current level */
+ ptep += ARM_LPAE_LVL_IDX(iova, lvl, data);
+
+ /* If we can install a leaf entry at this level, then do so */
+ if (size == block_size && (size & data->iop.cfg.pgsize_bitmap))
+ return arm_lpae_init_pte(data, iova, paddr, prot, lvl, ptep);
+
+ /* We can't allocate tables at the final level */
+ if (WARN_ON(lvl >= ARM_LPAE_MAX_LEVELS - 1))
+ return -EINVAL;
[varun] A warning message would be helpful.
+
+ /* Grab a pointer to the next level */
+ pte = *ptep;
+ if (!pte) {
+ cptep = alloc_pages_exact(1UL << data->pg_shift,
+ GFP_ATOMIC | __GFP_ZERO);
+ if (!cptep)
+ return -ENOMEM;
+
+ data->iop.cfg.tlb->flush_pgtable(cptep, 1UL << data->pg_shift,
+ cookie);
+ pte = __pa(cptep) | ARM_LPAE_PTE_TYPE_TABLE;
+ *ptep = pte;
+ data->iop.cfg.tlb->flush_pgtable(ptep, sizeof(*ptep), cookie);
+ } else {
+ cptep = iopte_deref(pte, data);
+ }
+
+ /* Rinse, repeat */
+ return __arm_lpae_map(data, iova, paddr, size, prot, lvl + 1, cptep);
+}
+
+static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
+ int prot)
+{
+ arm_lpae_iopte pte;
+
+ if (data->iop.fmt == ARM_LPAE_S1) {
+ pte = ARM_LPAE_PTE_AP_UNPRIV | ARM_LPAE_PTE_nG;
+
+ if (!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
+ pte |= ARM_LPAE_PTE_AP_RDONLY;
+
+ if (prot & IOMMU_CACHE)
+ pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
+ << ARM_LPAE_PTE_ATTRINDX_SHIFT);
+ } else {
+ pte = ARM_LPAE_PTE_HAP_FAULT;
+ if (prot & IOMMU_READ)
+ pte |= ARM_LPAE_PTE_HAP_READ;
+ if (prot & IOMMU_WRITE)
+ pte |= ARM_LPAE_PTE_HAP_WRITE;
+ if (prot & IOMMU_CACHE)
+ pte |= ARM_LPAE_PTE_MEMATTR_OIWB;
+ else
+ pte |= ARM_LPAE_PTE_MEMATTR_NC;
+ }
+
+ if (prot & IOMMU_NOEXEC)
+ pte |= ARM_LPAE_PTE_XN;
+
+ return pte;
+}
[[varun]] Do you plan to add a flag to indicate device memory? We had a discussion about this on the patch submitted by me. May be you can include that as a part of this patch. +
+static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
+ phys_addr_t paddr, size_t size, int iommu_prot) {
+ struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
+ arm_lpae_iopte *ptep = data->pgd;
+ int lvl = ARM_LPAE_START_LVL(data);
+ arm_lpae_iopte prot;
+
+ /* If no access, then nothing to do */
+ if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
+ return 0;
+
+ prot = arm_lpae_prot_to_pte(data, iommu_prot);
+ return __arm_lpae_map(data, iova, paddr, size, prot, lvl, ptep); }
+
+static void __arm_lpae_free_pgtable(struct arm_lpae_io_pgtable *data, int lvl,
+ arm_lpae_iopte *ptep)
+{
+ arm_lpae_iopte *start, *end;
+ unsigned long table_size;
+
+ /* Only leaf entries at the last level */
+ if (lvl == ARM_LPAE_MAX_LEVELS - 1)
+ return;
+
+ table_size = 1UL << data->pg_shift;
+ if (lvl == ARM_LPAE_START_LVL(data))
+ table_size *= data->pages_per_pgd;
+
+ start = ptep;
+ end = (void *)ptep + table_size;
+
+ while (ptep != end) {
+ arm_lpae_iopte pte = *ptep++;
+
+ if (!pte || iopte_leaf(pte, lvl))
+ continue;
+
+ __arm_lpae_free_pgtable(data, lvl + 1, iopte_deref(pte, data));
+ }
+
+ free_pages_exact(start, table_size);
+}
+
+static void arm_lpae_free_pgtable(struct io_pgtable *iop) {
+ struct arm_lpae_io_pgtable *data = io_pgtable_to_data(iop);
+
+ __arm_lpae_free_pgtable(data, ARM_LPAE_START_LVL(data), data->pgd);
+ kfree(data);
+}
+
+static int arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
+ unsigned long iova, size_t size,
+ arm_lpae_iopte prot, int lvl,
+ arm_lpae_iopte *ptep, size_t blk_size) {+ unsigned long blk_start, blk_end;
+ phys_addr_t blk_paddr;
+ arm_lpae_iopte table = 0;
+ void *cookie = data->iop.cookie;
+ struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
+
+ blk_start = iova & ~(blk_size - 1);
+ blk_end = blk_start + blk_size;
+ blk_paddr = iopte_to_pfn(*ptep, data) << data->pg_shift;
+
+ for (; blk_start < blk_end; blk_start += size, blk_paddr += size) {
+ arm_lpae_iopte *tablep;
+
+ /* Unmap! */
+ if (blk_start == iova)
+ continue;
+
+ /* __arm_lpae_map expects a pointer to the start of the table */
+ tablep = &table - ARM_LPAE_LVL_IDX(blk_start, lvl, data);
[[varun]] Not clear what's happening here. May be I am missing something, but where is the table allocated?
+ if (__arm_lpae_map(data, blk_start, blk_paddr, size, prot, lvl,
+ tablep) < 0) {
[[varun]] Again not clear how are we unmapping the range. Index at the current level should point to a page table (with contiguous block mappings). Unmap would applied to the mappings@the next level. Unmap can happen anywhere in the contiguous range. It seems that you are just creating a subset of the block mapping.
+ if (table) {
+ /* Free the table we allocated */
+ tablep = iopte_deref(table, data);
+ __arm_lpae_free_pgtable(data, lvl + 1, tablep);
+ }
+ return 0; /* Bytes unmapped */
+ }
+ }
+
+ *ptep = table;
+ tlb->flush_pgtable(ptep, sizeof(*ptep), cookie);
+ iova &= ~(blk_size - 1);
+ tlb->tlb_add_flush(iova, blk_size, true, cookie);
+ return size;
+}
+
+static int __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
+ unsigned long iova, size_t size, int lvl,
+ arm_lpae_iopte *ptep)
+{
+ arm_lpae_iopte pte;
+ struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
+ void *cookie = data->iop.cookie;
+ size_t blk_size = ARM_LPAE_BLOCK_SIZE(lvl, data);
+
+ ptep += ARM_LPAE_LVL_IDX(iova, lvl, data);
+ pte = *ptep;
+
+ /* Something went horribly wrong and we ran out of page table */
+ if (WARN_ON(!pte || (lvl == ARM_LPAE_MAX_LEVELS)))
+ return 0;
+
+ /* If the size matches this level, we're in the right place */
+ if (size == blk_size) {
+ *ptep = 0;
+ tlb->flush_pgtable(ptep, sizeof(*ptep), cookie);
+
+ if (!iopte_leaf(pte, lvl)) {
+ /* Also flush any partial walks */
+ tlb->tlb_add_flush(iova, size, false, cookie);
+ tlb->tlb_sync(data->iop.cookie);
+ ptep = iopte_deref(pte, data);
+ __arm_lpae_free_pgtable(data, lvl + 1, ptep);
+ } else {
+ tlb->tlb_add_flush(iova, size, true, cookie);
+ }
+
+ return size;
+ } else if (iopte_leaf(pte, lvl)) {
+ /*
+ * Insert a table at the next level to map the old region,
+ * minus the part we want to unmap
+ */
[[varun]] Minus could be somwhere in between the contiguous chunk? We should first break the entire block mapping in to a next level page mapping and then unmap a chunk.
+ return arm_lpae_split_blk_unmap(data, iova, size,
+ iopte_prot(pte), lvl, ptep,
+ blk_size);
+ }
+
+ /* Keep on walkin' */
+ ptep = iopte_deref(pte, data);
+ return __arm_lpae_unmap(data, iova, size, lvl + 1, ptep); }
+
+static int arm_lpae_unmap(struct io_pgtable_ops *ops, unsigned long iova,
+ size_t size)
+{
+ size_t unmapped;
+ struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
+ struct io_pgtable *iop = &data->iop;
+ arm_lpae_iopte *ptep = data->pgd;
+ int lvl = ARM_LPAE_START_LVL(data);
+
+ unmapped = __arm_lpae_unmap(data, iova, size, lvl, ptep);
+ if (unmapped)
+ iop->cfg.tlb->tlb_sync(iop->cookie);
+
+ return unmapped;
+}
+
+static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
+ unsigned long iova)
+{
+ struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
+ arm_lpae_iopte pte, *ptep = data->pgd;
+ int lvl = ARM_LPAE_START_LVL(data);
+
+ do {
+ /* Valid IOPTE pointer? */
+ if (!ptep)
+ return 0;
+
+ /* Grab the IOPTE we're interested in */
+ pte = *(ptep + ARM_LPAE_LVL_IDX(iova, lvl, data));
+
+ /* Valid entry? */
+ if (!pte)
+ return 0;
+
+ /* Leaf entry? */
+ if (iopte_leaf(pte,lvl))
+ goto found_translation;
+
+ /* Take it to the next level */
+ ptep = iopte_deref(pte, data);
+ } while (++lvl < ARM_LPAE_MAX_LEVELS);
+
+ /* Ran out of page tables to walk */
+ return 0;
+
+found_translation:
+ iova &= ((1 << data->pg_shift) - 1);
+ return ((phys_addr_t)iopte_to_pfn(pte,data) << data->pg_shift) | iova;
+}
+
+static void arm_lpae_restrict_pgsizes(struct io_pgtable_cfg *cfg) {
+ unsigned long granule;
+
+ /*
+ * We need to restrict the supported page sizes to match the
+ * translation regime for a particular granule. Aim to match
+ * the CPU page size if possible, otherwise prefer smaller sizes.
+ * While we're at it, restrict the block sizes to match the
+ * chosen granule.
+ */
+ if (cfg->pgsize_bitmap & PAGE_SIZE)
+ granule = PAGE_SIZE;
+ else if (cfg->pgsize_bitmap & ~PAGE_MASK)
+ granule = 1UL << __fls(cfg->pgsize_bitmap & ~PAGE_MASK);
+ else if (cfg->pgsize_bitmap & PAGE_MASK)
+ granule = 1UL << __ffs(cfg->pgsize_bitmap & PAGE_MASK);
+ else
+ granule = 0;
+
+ switch (granule) {
+ case SZ_4K:
+ cfg->pgsize_bitmap &= (SZ_4K | SZ_2M | SZ_1G);
+ break;
+ case SZ_16K:
+ cfg->pgsize_bitmap &= (SZ_16K | SZ_32M);
+ break;
+ case SZ_64K:
+ cfg->pgsize_bitmap &= (SZ_64K | SZ_512M);
+ break;
+ default:
+ cfg->pgsize_bitmap = 0;
+ }
+}
+
+static struct arm_lpae_io_pgtable * +arm_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg) {
+ unsigned long va_bits;
+ struct arm_lpae_io_pgtable *data;
+
+ arm_lpae_restrict_pgsizes(cfg);
+
+ if (!(cfg->pgsize_bitmap & (SZ_4K | SZ_16K | SZ_64K)))
+ return NULL;
+
+ if (cfg->ias > ARM_LPAE_MAX_ADDR_BITS)
+ return NULL;
+
+ if (cfg->oas > ARM_LPAE_MAX_ADDR_BITS)
+ return NULL;
+
+ data = kmalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return NULL;
+
+ data->pages_per_pgd = 1;
+ data->pg_shift = __ffs(cfg->pgsize_bitmap);
+ data->bits_per_level = data->pg_shift - ilog2(sizeof(arm_lpae_iopte));
+
+ va_bits = cfg->ias - data->pg_shift;
+ data->levels = DIV_ROUND_UP(va_bits, data->bits_per_level);
[[varun]] Not related to the patch, but this would be applicable to the CPU tables as well i.e, we can't support 48bit VA with 64 KB page tables, right? The AR64 memory maps shows possibility of using 6 bits for the first level page table.
+
+ data->iop.ops = (struct io_pgtable_ops) {
+ .map = arm_lpae_map,
+ .unmap = arm_lpae_unmap,
+ .iova_to_phys = arm_lpae_iova_to_phys,
+ };
+
+ return data;
+}
+
+static struct io_pgtable *arm_lpae_alloc_pgtable_s1(struct io_pgtable_cfg *cfg,
+ void *cookie)
+{
+ u64 reg;
+ struct arm_lpae_io_pgtable *data = arm_lpae_alloc_pgtable(cfg);
+
+ if (!data)
+ return NULL;
+
+ /* TCR */
+ reg = ARM_LPAE_TCR_EAE |
+ (ARM_LPAE_TCR_SH_IS << ARM_LPAE_TCR_SH0_SHIFT) | + (ARM_LPAE_TCR_RGN_WBWA << ARM_LPAE_TCR_IRGN0_SHIFT) | + (ARM_LPAE_TCR_RGN_WBWA << ARM_LPAE_TCR_ORGN0_SHIFT);
+
+ switch (1 << data->pg_shift) {
+ case SZ_4K:
+ reg |= ARM_LPAE_TCR_TG0_4K;
+ break;
+ case SZ_16K:
+ reg |= ARM_LPAE_TCR_TG0_16K;
+ break;
+ case SZ_64K:
+ reg |= ARM_LPAE_TCR_TG0_64K;
+ break;
+ }
+
+ switch (cfg->oas) {
+ case 32:
+ reg |= (ARM_LPAE_TCR_PS_32_BIT << ARM_LPAE_TCR_IPS_SHIFT);
+ break;
+ case 36:
+ reg |= (ARM_LPAE_TCR_PS_36_BIT << ARM_LPAE_TCR_IPS_SHIFT);
+ break;
+ case 40:
+ reg |= (ARM_LPAE_TCR_PS_40_BIT << ARM_LPAE_TCR_IPS_SHIFT);
+ break;
+ case 42:
+ reg |= (ARM_LPAE_TCR_PS_42_BIT << ARM_LPAE_TCR_IPS_SHIFT);
+ break;
+ case 44:
+ reg |= (ARM_LPAE_TCR_PS_44_BIT << ARM_LPAE_TCR_IPS_SHIFT);
+ break;
+ case 48:
+ reg |= (ARM_LPAE_TCR_PS_48_BIT << ARM_LPAE_TCR_IPS_SHIFT);
+ break;
+ default:
+ goto out_free_data;
+ }
+
+ reg |= (64ULL - cfg->ias) << ARM_LPAE_TCR_T0SZ_SHIFT;
+ cfg->arm_lpae_s1_cfg.tcr = reg;
+
+ /* MAIRs */
+ reg = (ARM_LPAE_MAIR_ATTR_NC
+ << ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_NC)) |
+ (ARM_LPAE_MAIR_ATTR_WBRWA
+ << ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_CACHE)) |
+ (ARM_LPAE_MAIR_ATTR_DEVICE
+ << ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_DEV));
+
+ cfg->arm_lpae_s1_cfg.mair[0] = reg;
+ cfg->arm_lpae_s1_cfg.mair[1] = 0;
+
+ /* Looking good; allocate a pgd */
+ data->pgd = alloc_pages_exact(1UL << data->pg_shift,
+ GFP_KERNEL | __GFP_ZERO);
+ if (!data->pgd)
+ goto out_free_data;
+
+ cfg->tlb->flush_pgtable(data->pgd, (1UL << data->pg_shift), cookie);
+
+ /* TTBRs */
+ cfg->arm_lpae_s1_cfg.ttbr[0] = virt_to_phys(data->pgd);
+ cfg->arm_lpae_s1_cfg.ttbr[1] = 0;
+ return &data->iop;
+
+out_free_data:
+ kfree(data);
+ return NULL;
+}
+
+static struct io_pgtable *arm_lpae_alloc_pgtable_s2(struct io_pgtable_cfg *cfg,
+ void *cookie)
+{
+ u64 reg, sl;
+ size_t pgd_size;
+ struct arm_lpae_io_pgtable *data = arm_lpae_alloc_pgtable(cfg);
+
+ if (!data)
+ return NULL;
+
+ /*
+ * Concatenate PGDs at level 1 if possible in order to reduce
+ * the depth of the stage-2 walk.
+ */
+ if (data->levels == ARM_LPAE_MAX_LEVELS) {
+ unsigned long pgd_bits, pgd_pages;
+ unsigned long va_bits = cfg->ias - data->pg_shift;
+
+ pgd_bits = data->bits_per_level * (data->levels - 1);
+ pgd_pages = 1 << (va_bits - pgd_bits);
+ if (pgd_pages <= ARM_LPAE_S2_MAX_CONCAT_PAGES) {
+ data->pages_per_pgd = pgd_pages;
+ data->levels--;
+ }
+ }
+
[[varun]] Can you point me to some documentation regarding stage 2 page concatenation. Not sure why this is required?
+ /* VTCR */
+ reg = ARM_LPAE_TCR_EAE |
+ (ARM_LPAE_TCR_SH_IS << ARM_LPAE_TCR_SH0_SHIFT) | + (ARM_LPAE_TCR_RGN_WBWA << ARM_LPAE_TCR_IRGN0_SHIFT) | + (ARM_LPAE_TCR_RGN_WBWA << ARM_LPAE_TCR_ORGN0_SHIFT);
+
+ sl = ARM_LPAE_START_LVL(data);
+
+ switch (1 << data->pg_shift) {
+ case SZ_4K:
+ reg |= ARM_LPAE_TCR_TG0_4K;
+ sl++; /* SL0 format is different for 4K granule size */
+ break;
+ case SZ_16K:
+ reg |= ARM_LPAE_TCR_TG0_16K;
+ break;
+ case SZ_64K:
+ reg |= ARM_LPAE_TCR_TG0_64K;
+ break;
+ }
+
+ switch (cfg->oas) {
+ case 32:
+ reg |= (ARM_LPAE_TCR_PS_32_BIT << ARM_LPAE_TCR_PS_SHIFT);
+ break;
+ case 36:
+ reg |= (ARM_LPAE_TCR_PS_36_BIT << ARM_LPAE_TCR_PS_SHIFT);
+ break;
+ case 40:
+ reg |= (ARM_LPAE_TCR_PS_40_BIT << ARM_LPAE_TCR_PS_SHIFT);
+ break;
+ case 42:
+ reg |= (ARM_LPAE_TCR_PS_42_BIT << ARM_LPAE_TCR_PS_SHIFT);
+ break;
+ case 44:
+ reg |= (ARM_LPAE_TCR_PS_44_BIT << ARM_LPAE_TCR_PS_SHIFT);
+ break;
+ case 48:
+ reg |= (ARM_LPAE_TCR_PS_48_BIT << ARM_LPAE_TCR_PS_SHIFT);
+ break;
+ default:
+ goto out_free_data;
+ }
+
+ reg |= (64ULL - cfg->ias) << ARM_LPAE_TCR_T0SZ_SHIFT;
+ reg |= (~sl & ARM_LPAE_TCR_SL0_MASK) << ARM_LPAE_TCR_SL0_SHIFT;
+ cfg->arm_lpae_s2_cfg.vtcr = reg;
+
+ /* Allocate pgd pages */
+ pgd_size = data->pages_per_pgd * (1UL << data->pg_shift);
+ data->pgd = alloc_pages_exact(pgd_size, GFP_KERNEL | __GFP_ZERO);
+ if (!data->pgd)
+ goto out_free_data;
+
+ cfg->tlb->flush_pgtable(data->pgd, pgd_size, cookie);
+
+ /* VTTBR */
+ cfg->arm_lpae_s2_cfg.vttbr = virt_to_phys(data->pgd);
+ return &data->iop;
+
+out_free_data:
+ kfree(data);
+ return NULL;
+}
+
+struct io_pgtable_init_fns io_pgtable_arm_lpae_s1_init_fns = {
+ .alloc = arm_lpae_alloc_pgtable_s1,
+ .free = arm_lpae_free_pgtable,
+};
+
+struct io_pgtable_init_fns io_pgtable_arm_lpae_s2_init_fns = {
+ .alloc = arm_lpae_alloc_pgtable_s2,
+ .free = arm_lpae_free_pgtable,
+};
drivers/iommu/io-pgtable.c +19 -1
@@ -25,8 +25,15 @@#include "io-pgtable.h" +extern struct io_pgtable_init_fns io_pgtable_arm_lpae_s1_init_fns; +extern struct io_pgtable_init_fns io_pgtable_arm_lpae_s2_init_fns; +static struct io_pgtable_init_fns *io_pgtable_init_table[IO_PGTABLE_NUM_FMTS ] = { +#ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE + [ARM_LPAE_S1 ] = &io_pgtable_arm_lpae_s1_init_fns, + [ARM_LPAE_S2] = &io_pgtable_arm_lpae_s2_init_fns, #endif }; struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt, diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h index 5ae75d9cae50..c1cff3d045db 100644 --- a/drivers/iommu/io-pgtable.h +++ b/drivers/iommu/io-pgtable.h
@@ -33,10 +33,22 @@ struct io_pgtable_cfg {/* Low-level data specific to the table format */ union { +struct { +u64 ttbr[2 ]; +u64 tcr; +u64 mair[2 ]; + } arm_lpae_s1_cfg; + +struct { +u64 vttbr; +u64 vtcr; + } arm_lpae_s2_cfg; }; };enum io_pgtable_fmt { +ARM_LPAE_S1 , +ARM_LPAE_S2 ,IO_PGTABLE_NUM_FMTS , };
Thanks,
Varun
PATCH 2/4 iommu: add ARM LPAE page table allocator
Quoted 1 line · Varun Sethi
Hi Will,
Hi Varun,
Thanks for the review!
Quoted 10 lines
drivers/iommu/io-pgtable-arm.c new file +10
-0+static int arm_lpae_init_pte (struct arm_lpae_io_pgtable *data, +unsigned long iova,phys_addr_t paddr, +arm_lpae_iopte prot,int lvl, +arm_lpae_iopte *ptep) +{ +arm_lpae_iopte pte = prot; + +/* We require an unmap first */ +if (iopte_leaf (*ptep, lvl)) +return -EEXIST ;
Quoted 2 lines
[varun] Instead of returning an error, how about displaying a warning and replacing the entry?
I'd be ok with displaying a warning, but I don't think we should just continue. It indicates a misuse of the IOMMU API and probably a missing TLBI.
Quoted 19 lines
(quoted diff) +18
-0+static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long iova, + phys_addr_t paddr, size_t size, arm_lpae_iopte prot, + int lvl, arm_lpae_iopte *ptep) +{ + arm_lpae_iopte *cptep, pte; + void *cookie = data->iop.cookie; + size_t block_size = ARM_LPAE_BLOCK_SIZE(lvl, data); + + /* Find our entry at the current level */ + ptep += ARM_LPAE_LVL_IDX(iova, lvl, data); + + /* If we can install a leaf entry at this level, then do so */ + if (size == block_size && (size & data->iop.cfg.pgsize_bitmap)) + return arm_lpae_init_pte(data, iova, paddr, prot, lvl, ptep); + + /* We can't allocate tables at the final level */ + if (WARN_ON(lvl >= ARM_LPAE_MAX_LEVELS - 1)) + return -EINVAL;
Quoted 1 line
[varun] A warning message would be helpful.
Sure, I can add one.
Quoted 31 lines
(quoted diff) +31
-0+static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data, + int prot) +{ + arm_lpae_iopte pte; + + if (data->iop.fmt == ARM_LPAE_S1) { + pte = ARM_LPAE_PTE_AP_UNPRIV | ARM_LPAE_PTE_nG; + + if (!(prot & IOMMU_WRITE) && (prot & IOMMU_READ)) + pte |= ARM_LPAE_PTE_AP_RDONLY; + + if (prot & IOMMU_CACHE) + pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE + << ARM_LPAE_PTE_ATTRINDX_SHIFT); + } else { + pte = ARM_LPAE_PTE_HAP_FAULT; + if (prot & IOMMU_READ) + pte |= ARM_LPAE_PTE_HAP_READ; + if (prot & IOMMU_WRITE) + pte |= ARM_LPAE_PTE_HAP_WRITE; + if (prot & IOMMU_CACHE) + pte |= ARM_LPAE_PTE_MEMATTR_OIWB; + else + pte |= ARM_LPAE_PTE_MEMATTR_NC; + } + + if (prot & IOMMU_NOEXEC) + pte |= ARM_LPAE_PTE_XN; + + return pte; +}
Quoted 3 lines
[[varun]] Do you plan to add a flag to indicate device memory? We had a discussion about this on the patch submitted by me. May be you can include that as a part of this patch.
That needs to go in as a separate patch. I think you should continue to push that separately!
Quoted 25 lines
(quoted diff) +23
-0+static int arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data, + unsigned long iova, size_t size, + arm_lpae_iopte prot, int lvl, + arm_lpae_iopte *ptep, size_t blk_size) { + unsigned long blk_start, blk_end; + phys_addr_t blk_paddr; + arm_lpae_iopte table = 0; + void *cookie = data->iop.cookie; + struct iommu_gather_ops *tlb = data->iop.cfg.tlb; + + blk_start = iova & ~(blk_size - 1); + blk_end = blk_start + blk_size; + blk_paddr = iopte_to_pfn(*ptep, data) << data->pg_shift; + + for (; blk_start < blk_end; blk_start += size, blk_paddr += size) { + arm_lpae_iopte *tablep; + + /* Unmap! */ + if (blk_start == iova) + continue; + + /* __arm_lpae_map expects a pointer to the start of the table */ + tablep = &table - ARM_LPAE_LVL_IDX(blk_start, lvl, data);
Quoted 2 lines
[[varun]] Not clear what's happening here. May be I am missing something, but where is the table allocated?
It is allocated in __arm_lpae_map, because the pte will be 0. The subtraction above is to avoid us having to allocate a whole level, just for a single invalid pte.
Quoted 10 lines
(quoted diff) +2
-0+ if (__arm_lpae_map(data, blk_start, blk_paddr, size, prot, lvl, + tablep) < 0) {[[varun]] Again not clear how are we unmapping the range. Index at the current level should point to a page table (with contiguous block mappings). Unmap would applied to the mappings at the next level. Unmap can happen anywhere in the contiguous range. It seems that you are just creating a subset of the block mapping.
We will be unmapping a single entry at the next level, so we basically create a table, then map everything at the next level apart from the part we need to unmap.
Quoted 37 lines
(quoted diff) +37
-0+static int __arm_lpae_unmap(struct arm_lpae_io_pgtable *data, + unsigned long iova, size_t size, int lvl, + arm_lpae_iopte *ptep) +{ + arm_lpae_iopte pte; + struct iommu_gather_ops *tlb = data->iop.cfg.tlb; + void *cookie = data->iop.cookie; + size_t blk_size = ARM_LPAE_BLOCK_SIZE(lvl, data); + + ptep += ARM_LPAE_LVL_IDX(iova, lvl, data); + pte = *ptep; + + /* Something went horribly wrong and we ran out of page table */ + if (WARN_ON(!pte || (lvl == ARM_LPAE_MAX_LEVELS))) + return 0; + + /* If the size matches this level, we're in the right place */ + if (size == blk_size) { + *ptep = 0; + tlb->flush_pgtable(ptep, sizeof(*ptep), cookie); + + if (!iopte_leaf(pte, lvl)) { + /* Also flush any partial walks */ + tlb->tlb_add_flush(iova, size, false, cookie); + tlb->tlb_sync(data->iop.cookie); + ptep = iopte_deref(pte, data); + __arm_lpae_free_pgtable(data, lvl + 1, ptep); + } else { + tlb->tlb_add_flush(iova, size, true, cookie); + } + + return size; + } else if (iopte_leaf(pte, lvl)) { + /* + * Insert a table at the next level to map the old region, + * minus the part we want to unmap + */
Quoted 3 lines
[[varun]] Minus could be somwhere in between the contiguous chunk? We should first break the entire block mapping in to a next level page mapping and then unmap a chunk.
The amount to unmap will match exactly one entry at the next level -- that's enforced by the IOMMU API (and it will also be aligned as such).
Quoted 27 lines
(quoted diff) +26
-0+static struct arm_lpae_io_pgtable * +arm_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg) { + unsigned long va_bits; + struct arm_lpae_io_pgtable *data; + + arm_lpae_restrict_pgsizes(cfg); + + if (!(cfg->pgsize_bitmap & (SZ_4K | SZ_16K | SZ_64K))) + return NULL; + + if (cfg->ias > ARM_LPAE_MAX_ADDR_BITS) + return NULL; + + if (cfg->oas > ARM_LPAE_MAX_ADDR_BITS) + return NULL; + + data = kmalloc(sizeof(*data), GFP_KERNEL); + if (!data) + return NULL; + + data->pages_per_pgd = 1; + data->pg_shift = __ffs(cfg->pgsize_bitmap); + data->bits_per_level = data->pg_shift - ilog2(sizeof(arm_lpae_iopte)); + + va_bits = cfg->ias - data->pg_shift; + data->levels = DIV_ROUND_UP(va_bits, data->bits_per_level);
Quoted 4 lines
[[varun]] Not related to the patch, but this would be applicable to the CPU tables as well i.e, we can't support 48bit VA with 64 KB page tables, right? The AR64 memory maps shows possibility of using 6 bits for the first level page table.
Sure we can support 48-bit VAs with 64k pages. Why do you think we can't?
Quoted 26 lines
(quoted diff) +26
-0+static struct io_pgtable *arm_lpae_alloc_pgtable_s2(struct io_pgtable_cfg *cfg, + void *cookie) +{ + u64 reg, sl; + size_t pgd_size; + struct arm_lpae_io_pgtable *data = arm_lpae_alloc_pgtable(cfg); + + if (!data) + return NULL; + + /* + * Concatenate PGDs at level 1 if possible in order to reduce + * the depth of the stage-2 walk. + */ + if (data->levels == ARM_LPAE_MAX_LEVELS) { + unsigned long pgd_bits, pgd_pages; + unsigned long va_bits = cfg->ias - data->pg_shift; + + pgd_bits = data->bits_per_level * (data->levels - 1); + pgd_pages = 1 << (va_bits - pgd_bits); + if (pgd_pages <= ARM_LPAE_S2_MAX_CONCAT_PAGES) { + data->pages_per_pgd = pgd_pages; + data->levels--; + } + } +
Quoted 2 lines
[[varun]] Can you point me to some documentation regarding stage 2 page concatenation. Not sure why this is required?
It's all in the ARM ARM. The idea is to reduce the depth of the stage-2 walk, since that can have an impact on performance when it gets too deep (remember that stage-1 table walks are themselves subjected to stage-2 translation).
Will
PATCH 2/4 iommu: add ARM LPAE page table allocator
Hi Will,
Please find my response inline. Search for "varun".
-----Original Message-----
From: Will Deacon [mailto:will.deacon at arm.com]
Sent: Saturday, December 06, 2014 12:18 AM
To: Sethi Varun-B16395
Cc: linux-arm-kernel at lists.infradead.org; iommu at lists.linux-foundation.org; prem.mallappa at broadcom.com; Robin Murphy; lauraa at codeaurora.org; mitchelh at codeaurora.org; laurent.pinchart at ideasonboard.com; joro at 8bytes.org; m.szyprowski at samsung.com
Subject: Re: [PATCH 2/4] iommu: add ARM LPAE page table allocator
Quoted 1 line · Varun Sethi
Hi Will,
Hi Varun,
Thanks for the review!
Quoted 11 lines
drivers/iommu/io-pgtable-arm.c new file +9
-0+static int arm_lpae_init_pte (struct arm_lpae_io_pgtable *data, +unsigned long iova,phys_addr_t paddr, +arm_lpae_iopte prot,int lvl, +arm_lpae_iopte *ptep) { +arm_lpae_iopte pte = prot; + +/* We require an unmap first */ +if (iopte_leaf (*ptep, lvl)) +return -EEXIST ;[varun] Instead of returning an error, how about displaying a warning and replacing the entry?
I'd be ok with displaying a warning, but I don't think we should just continue. It indicates a misuse of the IOMMU API and probably a missing TLBI.
[[varun]] May not apply now, but what if we are dealing with a case where memory is not pinned? It may be possible to hookup (without an unmap) an iova to a different physical address. Offcourse, tlb invalidation would be required. Could this scenario be relevant in case of stall mode?
Quoted 19 lines
(quoted diff) +18
-0+static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long iova, + phys_addr_t paddr, size_t size, arm_lpae_iopte prot, + int lvl, arm_lpae_iopte *ptep) { + arm_lpae_iopte *cptep, pte; + void *cookie = data->iop.cookie; + size_t block_size = ARM_LPAE_BLOCK_SIZE(lvl, data); + + /* Find our entry at the current level */ + ptep += ARM_LPAE_LVL_IDX(iova, lvl, data); + + /* If we can install a leaf entry at this level, then do so */ + if (size == block_size && (size & data->iop.cfg.pgsize_bitmap)) + return arm_lpae_init_pte(data, iova, paddr, prot, lvl, + ptep); + + /* We can't allocate tables at the final level */ + if (WARN_ON(lvl >= ARM_LPAE_MAX_LEVELS - 1)) + return -EINVAL;
Quoted 1 line
[varun] A warning message would be helpful.
Sure, I can add one.
Quoted 30 lines
(quoted diff) +30
-0+static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data, + int prot) { + arm_lpae_iopte pte; + + if (data->iop.fmt == ARM_LPAE_S1) { + pte = ARM_LPAE_PTE_AP_UNPRIV | ARM_LPAE_PTE_nG; + + if (!(prot & IOMMU_WRITE) && (prot & IOMMU_READ)) + pte |= ARM_LPAE_PTE_AP_RDONLY; + + if (prot & IOMMU_CACHE) + pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE + << ARM_LPAE_PTE_ATTRINDX_SHIFT); + } else { + pte = ARM_LPAE_PTE_HAP_FAULT; + if (prot & IOMMU_READ) + pte |= ARM_LPAE_PTE_HAP_READ; + if (prot & IOMMU_WRITE) + pte |= ARM_LPAE_PTE_HAP_WRITE; + if (prot & IOMMU_CACHE) + pte |= ARM_LPAE_PTE_MEMATTR_OIWB; + else + pte |= ARM_LPAE_PTE_MEMATTR_NC; + } + + if (prot & IOMMU_NOEXEC) + pte |= ARM_LPAE_PTE_XN; + + return pte; +}
Quoted 3 lines
[[varun]] Do you plan to add a flag to indicate device memory? We had a discussion about this on the patch submitted by me. May be you can include that as a part of this patch.
That needs to go in as a separate patch. I think you should continue to push that separately!
Quoted 26 lines
(quoted diff) +24
-0+static int arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data, + unsigned long iova, size_t size, + arm_lpae_iopte prot, int lvl, + arm_lpae_iopte *ptep, size_t blk_size) { + unsigned long blk_start, blk_end; + phys_addr_t blk_paddr; + arm_lpae_iopte table = 0; + void *cookie = data->iop.cookie; + struct iommu_gather_ops *tlb = data->iop.cfg.tlb; + + blk_start = iova & ~(blk_size - 1); + blk_end = blk_start + blk_size; + blk_paddr = iopte_to_pfn(*ptep, data) << data->pg_shift; + + for (; blk_start < blk_end; blk_start += size, blk_paddr += size) { + arm_lpae_iopte *tablep; + + /* Unmap! */ + if (blk_start == iova) + continue; + + /* __arm_lpae_map expects a pointer to the start of the table */ + tablep = &table - ARM_LPAE_LVL_IDX(blk_start, lvl, + data);
Quoted 2 lines
[[varun]] Not clear what's happening here. May be I am missing something, but where is the table allocated?
It is allocated in __arm_lpae_map, because the pte will be 0. The subtraction above is to avoid us having to allocate a whole level, just for a single invalid pte.
Quoted 10 lines
(quoted diff) +2
-0+ if (__arm_lpae_map(data, blk_start, blk_paddr, size, prot, lvl, + tablep) < 0) {[[varun]] Again not clear how are we unmapping the range. Index at the current level should point to a page table (with contiguous block mappings). Unmap would applied to the mappings at the next level. Unmap can happen anywhere in the contiguous range. It seems that you are just creating a subset of the block mapping.
We will be unmapping a single entry at the next level, so we basically create a table, then map everything at the next level apart from the part we need to unmap.
[varun] ok, but you could potentially end up splitting mapping to the least possible page size e.g. 4K. You, don't seem to take in to account the possibility of using the block size at the next level. For example, take a case where we have a huge page mapping using 1G page size and we have an unmap request for 4K. We could still split maximum part of the mapping using 2M pages at the next level. The entry where we need to unmap the 4K region would potentially go to the next level.
Quoted 36 lines
(quoted diff) +36
-0+static int __arm_lpae_unmap(struct arm_lpae_io_pgtable *data, + unsigned long iova, size_t size, int lvl, + arm_lpae_iopte *ptep) { + arm_lpae_iopte pte; + struct iommu_gather_ops *tlb = data->iop.cfg.tlb; + void *cookie = data->iop.cookie; + size_t blk_size = ARM_LPAE_BLOCK_SIZE(lvl, data); + + ptep += ARM_LPAE_LVL_IDX(iova, lvl, data); + pte = *ptep; + + /* Something went horribly wrong and we ran out of page table */ + if (WARN_ON(!pte || (lvl == ARM_LPAE_MAX_LEVELS))) + return 0; + + /* If the size matches this level, we're in the right place */ + if (size == blk_size) { + *ptep = 0; + tlb->flush_pgtable(ptep, sizeof(*ptep), cookie); + + if (!iopte_leaf(pte, lvl)) { + /* Also flush any partial walks */ + tlb->tlb_add_flush(iova, size, false, cookie); + tlb->tlb_sync(data->iop.cookie); + ptep = iopte_deref(pte, data); + __arm_lpae_free_pgtable(data, lvl + 1, ptep); + } else { + tlb->tlb_add_flush(iova, size, true, cookie); + } + + return size; + } else if (iopte_leaf(pte, lvl)) { + /* + * Insert a table at the next level to map the old region, + * minus the part we want to unmap + */
Quoted 3 lines
[[varun]] Minus could be somwhere in between the contiguous chunk? We should first break the entire block mapping in to a next level page mapping and then unmap a chunk.
The amount to unmap will match exactly one entry at the next level -- that's enforced by the IOMMU API (and it will also be aligned as such).
Quoted 28 lines
(quoted diff) +27
-0+static struct arm_lpae_io_pgtable * +arm_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg) { + unsigned long va_bits; + struct arm_lpae_io_pgtable *data; + + arm_lpae_restrict_pgsizes(cfg); + + if (!(cfg->pgsize_bitmap & (SZ_4K | SZ_16K | SZ_64K))) + return NULL; + + if (cfg->ias > ARM_LPAE_MAX_ADDR_BITS) + return NULL; + + if (cfg->oas > ARM_LPAE_MAX_ADDR_BITS) + return NULL; + + data = kmalloc(sizeof(*data), GFP_KERNEL); + if (!data) + return NULL; + + data->pages_per_pgd = 1; + data->pg_shift = __ffs(cfg->pgsize_bitmap); + data->bits_per_level = data->pg_shift - + ilog2(sizeof(arm_lpae_iopte)); + + va_bits = cfg->ias - data->pg_shift; + data->levels = DIV_ROUND_UP(va_bits, data->bits_per_level);
Quoted 4 lines
[[varun]] Not related to the patch, but this would be applicable to the CPU tables as well i.e, we can't support 48bit VA with 64 KB page tables, right? The AR64 memory maps shows possibility of using 6 bits for the first level page table.
Sure we can support 48-bit VAs with 64k pages. Why do you think we can't?
[varun] My concern was with respect to the bits per level, which is uneven for the 64 K page sizes. Just wondering how would things work with 64K pages when we do a 3 level page lookup.
Quoted 26 lines
(quoted diff) +26
-0+static struct io_pgtable *arm_lpae_alloc_pgtable_s2(struct io_pgtable_cfg *cfg, + void *cookie) { + u64 reg, sl; + size_t pgd_size; + struct arm_lpae_io_pgtable *data = +arm_lpae_alloc_pgtable(cfg); + + if (!data) + return NULL; + + /* + * Concatenate PGDs at level 1 if possible in order to reduce + * the depth of the stage-2 walk. + */ + if (data->levels == ARM_LPAE_MAX_LEVELS) { + unsigned long pgd_bits, pgd_pages; + unsigned long va_bits = cfg->ias - data->pg_shift; + + pgd_bits = data->bits_per_level * (data->levels - 1); + pgd_pages = 1 << (va_bits - pgd_bits); + if (pgd_pages <= ARM_LPAE_S2_MAX_CONCAT_PAGES) { + data->pages_per_pgd = pgd_pages; + data->levels--; + } + } +
Quoted 2 lines
[[varun]] Can you point me to some documentation regarding stage 2 page concatenation. Not sure why this is required?
It's all in the ARM ARM. The idea is to reduce the depth of the stage-2 walk, since that can have an impact on performance when it gets too deep (remember that stage-1 table walks are themselves subjected to stage-2 translation).
[varun] Just curious, why not consider concatenation at stage 1?
Thanks,
Varun
PATCH 2/4 iommu: add ARM LPAE page table allocator
Quoted 1 line · Varun Sethi
Please find my response inline. Search for "varun".
This is getting fiddly now that you've already replied once. Any chance you could sort your mail client out, please?
Quoted 16 lines
Quoted 11 lines
drivers/iommu/io-pgtable-arm.c new file +9
-0+static int arm_lpae_init_pte (struct arm_lpae_io_pgtable *data, +unsigned long iova,phys_addr_t paddr, +arm_lpae_iopte prot,int lvl, +arm_lpae_iopte *ptep) { +arm_lpae_iopte pte = prot; + +/* We require an unmap first */ +if (iopte_leaf (*ptep, lvl)) +return -EEXIST ;[varun] Instead of returning an error, how about displaying a warning and replacing the entry?
I'd be ok with displaying a warning, but I don't think we should just continue. It indicates a misuse of the IOMMU API and probably a missing TLBI.
Quoted 4 lines
[[varun]] May not apply now, but what if we are dealing with a case where memory is not pinned? It may be possible to hookup (without an unmap) an iova to a different physical address. Offcourse, tlb invalidation would be required. Could this scenario be relevant in case of stall mode?
If we wanted to support that, then we'd need some new functions for grabbing hold of the entry and manipulating it in a similar way to the CPU side (e.g. pte_mkdirty etc).
Quoted 15 lines
Quoted 9 lines
(quoted diff) +2
-0+ if (__arm_lpae_map(data, blk_start, blk_paddr, size, prot, lvl, + tablep) < 0) {[[varun]] Again not clear how are we unmapping the range. Index at the current level should point to a page table (with contiguous block mappings). Unmap would applied to the mappings at the next level. Unmap can happen anywhere in the contiguous range. It seems that you are just creating a subset of the block mapping.
We will be unmapping a single entry at the next level, so we basically create a table, then map everything at the next level apart from the part we need to unmap.
Quoted 7 lines
[varun] ok, but you could potentially end up splitting mapping to the least possible page size e.g. 4K. You, don't seem to take in to account the possibility of using the block size at the next level. For example, take a case where we have a huge page mapping using 1G page size and we have an unmap request for 4K. We could still split maximum part of the mapping using 2M pages at the next level. The entry where we need to unmap the 4K region would potentially go to the next level.
Aha, I see what you mean here, thanks. I'll take a look...
Quoted 78 lines
Quoted 36 lines
(quoted diff) +36
-0+static int __arm_lpae_unmap(struct arm_lpae_io_pgtable *data, + unsigned long iova, size_t size, int lvl, + arm_lpae_iopte *ptep) { + arm_lpae_iopte pte; + struct iommu_gather_ops *tlb = data->iop.cfg.tlb; + void *cookie = data->iop.cookie; + size_t blk_size = ARM_LPAE_BLOCK_SIZE(lvl, data); + + ptep += ARM_LPAE_LVL_IDX(iova, lvl, data); + pte = *ptep; + + /* Something went horribly wrong and we ran out of page table */ + if (WARN_ON(!pte || (lvl == ARM_LPAE_MAX_LEVELS))) + return 0; + + /* If the size matches this level, we're in the right place */ + if (size == blk_size) { + *ptep = 0; + tlb->flush_pgtable(ptep, sizeof(*ptep), cookie); + + if (!iopte_leaf(pte, lvl)) { + /* Also flush any partial walks */ + tlb->tlb_add_flush(iova, size, false, cookie); + tlb->tlb_sync(data->iop.cookie); + ptep = iopte_deref(pte, data); + __arm_lpae_free_pgtable(data, lvl + 1, ptep); + } else { + tlb->tlb_add_flush(iova, size, true, cookie); + } + + return size; + } else if (iopte_leaf(pte, lvl)) { + /* + * Insert a table at the next level to map the old region, + * minus the part we want to unmap + */Quoted 3 lines
[[varun]] Minus could be somwhere in between the contiguous chunk? We should first break the entire block mapping in to a next level page mapping and then unmap a chunk.
The amount to unmap will match exactly one entry at the next level -- that's enforced by the IOMMU API (and it will also be aligned as such).
Quoted 28 lines
(quoted diff) +27
-0+static struct arm_lpae_io_pgtable * +arm_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg) { + unsigned long va_bits; + struct arm_lpae_io_pgtable *data; + + arm_lpae_restrict_pgsizes(cfg); + + if (!(cfg->pgsize_bitmap & (SZ_4K | SZ_16K | SZ_64K))) + return NULL; + + if (cfg->ias > ARM_LPAE_MAX_ADDR_BITS) + return NULL; + + if (cfg->oas > ARM_LPAE_MAX_ADDR_BITS) + return NULL; + + data = kmalloc(sizeof(*data), GFP_KERNEL); + if (!data) + return NULL; + + data->pages_per_pgd = 1; + data->pg_shift = __ffs(cfg->pgsize_bitmap); + data->bits_per_level = data->pg_shift - + ilog2(sizeof(arm_lpae_iopte)); + + va_bits = cfg->ias - data->pg_shift; + data->levels = DIV_ROUND_UP(va_bits, data->bits_per_level);Quoted 4 lines
[[varun]] Not related to the patch, but this would be applicable to the CPU tables as well i.e, we can't support 48bit VA with 64 KB page tables, right? The AR64 memory maps shows possibility of using 6 bits for the first level page table.
Sure we can support 48-bit VAs with 64k pages. Why do you think we can't?
Quoted 3 lines
[varun] My concern was with respect to the bits per level, which is uneven for the 64 K page sizes. Just wondering how would things work with 64K pages when we do a 3 level page lookup.
Well, it's uneven (9) for the 4k case too. Do you actually see an issue here?
48-bit VA with 64k pages gives us:
va_bits = (48 - 16) = 32
bits_per_level = (16 - 3) = 13
levels = ceil(32/13) = 3
so the starting level is 1, which resolves 32-(13*2) = 6 bits.
Does that make sense?
Quoted 34 lines
Quoted 26 lines
(quoted diff) +26
-0+static struct io_pgtable *arm_lpae_alloc_pgtable_s2(struct io_pgtable_cfg *cfg, + void *cookie) { + u64 reg, sl; + size_t pgd_size; + struct arm_lpae_io_pgtable *data = +arm_lpae_alloc_pgtable(cfg); + + if (!data) + return NULL; + + /* + * Concatenate PGDs at level 1 if possible in order to reduce + * the depth of the stage-2 walk. + */ + if (data->levels == ARM_LPAE_MAX_LEVELS) { + unsigned long pgd_bits, pgd_pages; + unsigned long va_bits = cfg->ias - data->pg_shift; + + pgd_bits = data->bits_per_level * (data->levels - 1); + pgd_pages = 1 << (va_bits - pgd_bits); + if (pgd_pages <= ARM_LPAE_S2_MAX_CONCAT_PAGES) { + data->pages_per_pgd = pgd_pages; + data->levels--; + } + } +Quoted 2 lines
[[varun]] Can you point me to some documentation regarding stage 2 page concatenation. Not sure why this is required?
It's all in the ARM ARM. The idea is to reduce the depth of the stage-2 walk, since that can have an impact on performance when it gets too deep (remember that stage-1 table walks are themselves subjected to stage-2 translation).
Quoted 1 line
[varun] Just curious, why not consider concatenation at stage 1?
Well, the architecture doesn't support it. It's not as big a deal at stage 1 anyway, because there is no nesting in that case (unlike stage 2, which is applied to stage 1 table walks).
Will
PATCH 2/4 iommu: add ARM LPAE page table allocator
Quoted 10 lines · Will Deacon
Quoted 7 lines · Varun Sethi
[varun] ok, but you could potentially end up splitting mapping to the least possible page size e.g. 4K. You, don't seem to take in to account the possibility of using the block size at the next level. For example, take a case where we have a huge page mapping using 1G page size and we have an unmap request for 4K. We could still split maximum part of the mapping using 2M pages at the next level. The entry where we need to unmap the 4K region would potentially go to the next level.
Aha, I see what you mean here, thanks. I'll take a look...
Scratch that, I think the code is fine as it is. For the case you highlight, we iterate over the 1GB region remapping it using 4k pages, but skipping the one we want to unmap, so I don't think there's a problem (__arm_lpae_map will create the relevant table entries).
Will
PATCH 2/4 iommu: add ARM LPAE page table allocator
Hi Will,
-----Original Message-----
From: Will Deacon [mailto:will.deacon at arm.com]
Sent: Monday, December 15, 2014 9:13 PM
To: Sethi Varun-B16395
Cc: linux-arm-kernel at lists.infradead.org; iommu at lists.linux-foundation.org; prem.mallappa at broadcom.com; Robin Murphy; lauraa at codeaurora.org; mitchelh at codeaurora.org; laurent.pinchart at ideasonboard.com; joro at 8bytes.org; m.szyprowski at samsung.com
Subject: Re: [PATCH 2/4] iommu: add ARM LPAE page table allocator
Quoted 10 lines · Will Deacon
Quoted 7 lines · Varun Sethi
[varun] ok, but you could potentially end up splitting mapping to
the least possible page size e.g. 4K. You, don't seem to take in to account the possibility of using the block size at the next level. For example, take a case where we have a huge page mapping using 1G page size and we have an unmap request for 4K. We could still split maximum part of the mapping using 2M pages at the next level. The entry where we need to unmap the 4K region would potentially go to the next level.Aha, I see what you mean here, thanks. I'll take a look...
Scratch that, I think the code is fine as it is. For the case you highlight, we iterate over the 1GB region remapping it using 4k pages, but skipping the one we want to unmap, so I don't think there's a problem (__arm_lpae_map will create the relevant table entries).
[[varun]] But you can split 1G in 2M mappings and then split up the unmapped region using 4K pages. In this case you split up the entire region using 4K pages.
Thanks,
Varun
PATCH 2/4 iommu: add ARM LPAE page table allocator
Quoted 25 lines · Varun Sethi
-----Original Message-----
From: Will Deacon [mailto:will.deacon at arm.com]
Sent: Monday, December 15, 2014 9:13 PM
To: Sethi Varun-B16395
Cc: linux-arm-kernel at lists.infradead.org; iommu at lists.linux-foundation.org; prem.mallappa at broadcom.com; Robin Murphy; lauraa at codeaurora.org; mitchelh at codeaurora.org; laurent.pinchart at ideasonboard.com; joro at 8bytes.org; m.szyprowski at samsung.com
Subject: Re: [PATCH 2/4] iommu: add ARM LPAE page table allocatorQuoted 10 lines · Will Deacon
Quoted 7 lines · Varun Sethi
[varun] ok, but you could potentially end up splitting mapping to
the least possible page size e.g. 4K. You, don't seem to take in to account the possibility of using the block size at the next level. For example, take a case where we have a huge page mapping using 1G page size and we have an unmap request for 4K. We could still split maximum part of the mapping using 2M pages at the next level. The entry where we need to unmap the 4K region would potentially go to the next level.Aha, I see what you mean here, thanks. I'll take a look...
Scratch that, I think the code is fine as it is. For the case you highlight, we iterate over the 1GB region remapping it using 4k pages, but skipping the one we want to unmap, so I don't think there's a problem (
__arm_lpae_mapwill create the relevant table entries).
Quoted 3 lines
[[varun]] But you can split 1G in 2M mappings and then split up the unmapped region using 4K pages. In this case you split up the entire region using 4K pages.
True, I miss an optimisation opportunity there, but I don't know that it's common enough to care (in the same way that we don't recreate a 1G mapping if you remapped that 4k page back like it was).
You could add this by making arm_lpae_split_blk_unmap recursive, if you wanted to.
Will
PATCH 2/4 iommu: add ARM LPAE page table allocator
In reply to Will Deacon-----Original Message-----
From: Will Deacon [mailto:will.deacon at arm.com]
Sent: Monday, December 15, 2014 7:00 PM
To: Sethi Varun-B16395
Cc: linux-arm-kernel at lists.infradead.org; iommu at lists.linux-foundation.org; prem.mallappa at broadcom.com; Robin Murphy; lauraa at codeaurora.org; mitchelh at codeaurora.org; laurent.pinchart at ideasonboard.com; joro at 8bytes.org; m.szyprowski at samsung.com
Subject: Re: [PATCH 2/4] iommu: add ARM LPAE page table allocator
Quoted 1 line · Varun Sethi
Please find my response inline. Search for "varun".
This is getting fiddly now that you've already replied once. Any chance you could sort your mail client out, please?
[[varun]] Yes I need to do that, this is painful.
Quoted 18 lines
Quoted 11 lines
(quoted diff) +10
-0+ if (!data) + return NULL; + + data->pages_per_pgd = 1; + data->pg_shift = __ffs(cfg->pgsize_bitmap); + data->bits_per_level = data->pg_shift - + ilog2(sizeof(arm_lpae_iopte)); + + va_bits = cfg->ias - data->pg_shift; + data->levels = DIV_ROUND_UP(va_bits, data->bits_per_level);Quoted 4 lines
[[varun]] Not related to the patch, but this would be applicable to the CPU tables as well i.e, we can't support 48bit VA with 64 KB page tables, right? The AR64 memory maps shows possibility of using 6 bits for the first level page table.
Sure we can support 48-bit VAs with 64k pages. Why do you think we can't?
Quoted 3 lines
[varun] My concern was with respect to the bits per level, which is uneven for the 64 K page sizes. Just wondering how would things work with 64K pages when we do a 3 level page lookup.
Well, it's uneven (9) for the 4k case too. Do you actually see an issue here?
48-bit VA with 64k pages gives us:
va_bits = (48 - 16) = 32
bits_per_level = (16 - 3) = 13
levels = ceil(32/13) = 3
so the starting level is 1, which resolves 32-(13*2) = 6 bits.
Does that make sense?
[[varun]]Yes, but what I meant was, is that in case of 4K pages you have 9 bits per level, but for 64K pages you have 6 bits for the first level and 13 each for second and third. So, bits per level would not work in case of 64 K pages?
Thanks,
Varun
PATCH 2/4 iommu: add ARM LPAE page table allocator
Quoted 14 lines · Varun Sethi
Quoted 5 lines
Sure we can support 48-bit VAs with 64k pages. Why do you think we can't?
[varun] My concern was with respect to the bits per level, which is uneven for the 64 K page sizes. Just wondering how would things work with 64K pages when we do a 3 level page lookup.
Well, it's uneven (9) for the 4k case too. Do you actually see an issue here?
48-bit VA with 64k pages gives us:
va_bits= (48 - 16) = 32
bits_per_level= (16 - 3) = 13
levels = ceil(32/13) = 3
Quoted 8 lines
so the starting level is 1, which resolves 32-(13*2) = 6 bits.
Does that make sense?
[[varun]]Yes, but what I meant was, is that in case of 4K pages you have 9 bits per level, but for 64K pages you have 6 bits for the first level and 13 each for second and third. So, bits per level would not work in case of 64 K pages?
The current code takes this into account.
Will
PATCH 3/4 iommu: add self-consistency tests to ARM LPAE IO page table allocator
In reply to Will DeaconThis patch adds a series of basic self-consistency tests to the ARM LPAE IO page table allocator that exercise corner cases in map/unmap, as well as testing all valid configurations of pagesize, ias and stage.
--- drivers/iommu/Kconfig | 9 ++ drivers/iommu/io-pgtable-arm.c | 190 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 199 insertions(+)
drivers/iommu/Kconfig +9 -0
@@ -28,6 +28,15 @@ config IOMMU_IO_PGTABLE_LPAE sizes at both stage-1 and stage-2, as well as address spaces up to 48-bits in size. +config IOMMU_IO_PGTABLE_LPAE_SELFTEST + bool "LPAE selftests" + depends on IOMMU_IO_PGTABLE_LPAE + help + Enable self-tests for LPAE page table allocator. This performs + a series of page-table consistency checks during boot. + + If unsure, say N here. + endmenu config OF_IOMMU
drivers/iommu/io-pgtable-arm.c +190 -0
193 lines not on this page · raw message
Signature · 1 line
2.1.1
PATCH 4/4 iommu/arm-smmu: make use of generic LPAE allocator
In reply to Will DeaconThe ARM SMMU can walk LPAE page tables, so make use of the generic allocator.
--- arch/arm64/Kconfig | 1 - drivers/iommu/Kconfig | 6 +- drivers/iommu/arm-smmu.c | 872 ++++++++++++++--------------------------------- 3 files changed, 252 insertions(+), 627 deletions(-)
arch/arm64/Kconfig +0 -1
@@ -233,7 +233,6 @@ config ARM64_VA_BITS_42 config ARM64_VA_BITS_48 bool "48-bit" - depends on !ARM_SMMU endchoice
drivers/iommu/Kconfig +3 -3
@@ -317,13 +317,13 @@ config SPAPR_TCE_IOMMU config ARM_SMMU bool "ARM Ltd. System MMU (SMMU) Support" - depends on ARM64 || (ARM_LPAE && OF) + depends on ARM64 || ARM select IOMMU_API + select IOMMU_IO_PGTABLE_LPAE select ARM_DMA_USE_IOMMU if ARM help Support for implementations of the ARM System MMU architecture - versions 1 and 2. The driver supports both v7l and v8l table - formats with 4k and 64k page sizes. + versions 1 and 2. Say Y here if your SoC includes an IOMMU device implementing the ARM SMMU architecture.
drivers/iommu/arm-smmu.c +249 -623
1,208 lines not on this page · raw message
Signature · 1 line
2.1.1
cover Generic IOMMU page table framework
In reply to Will DeaconHi Will,
Thank you for the patches.
Quoted 22 lines · On Thursday 27 November 2014 11:51:14 Will Deacon wrote:
Hi all,
This series introduces a generic IOMMU page table allocation framework, implements support for ARM long-descriptors and then ports the arm-smmu driver over to the new code.
There are a few reasons for doing this:
- Page table code is hard, and I don't enjoy shopping
- A number of IOMMUs actually use the same table format, but currently duplicate the code
- It provides a CPU (and architecture) independent allocator, which may be useful for some systems where the CPU is using a different table format for its own mappings
As illustrated in the final patch, an IOMMU driver interacts with the allocator by passing in a configuration structure describing the input and output address ranges, the supported pages sizes and a set of ops for performing various TLB invalidation and PTE flushing routines.
Quoted 4 lines
The LPAE code implements support for 4k/2M/1G, 16k/32M and 64k/512M mappings, but I decided not to implement the contiguous bit in the interest of trying to keep the code semi-readable. This could always be added later, if needed.
Do you have any idea how much the contiguous bit can improve performances in real use cases ?
Quoted 32 lines
I also included some self-tests for the LPAE implementation. Ideally we'd merge these, but I'm also happy to drop them if there are objections.
Tested with the self-tests, but also VFIO + MMU-500 at stage-1 and stage-2. Patches taken against my iommu/devel branch (queued by Joerg for 3.19).
All feedback welcome.
Will
--->8
Will Deacon (4):
iommu: introduce generic page table allocation framework
iommu: add ARM LPAE page table allocator
iommu: add self-consistency tests to ARM LPAE IO page table allocator iommu/arm-smmu: make use of generic LPAE allocatorMAINTAINERS | 1 + arch/arm64/Kconfig | 1 - drivers/iommu/Kconfig | 32 +- drivers/iommu/Makefile | 2 + drivers/iommu/arm-smmu.c | 872 +++++++++++--------------------------- drivers/iommu/io-pgtable-arm.c | 925 ++++++++++++++++++++++++++++++++++++++ drivers/iommu/io-pgtable.c | 78 ++++ drivers/iommu/io-pgtable.h | 77 ++++ 8 files changed, 1361 insertions(+), 627 deletions(-) create mode 100644 drivers/iommu/io-pgtable-arm.c create mode 100644 drivers/iommu/io-pgtable.c create mode 100644 drivers/iommu/io-pgtable.h
Signature · 3 lines
Regards, Laurent Pinchart
cover Generic IOMMU page table framework
Quoted 1 line · Laurent Pinchart
Hi Will,
Hi Laurent,
Quoted 23 lines · On Thursday 27 November 2014 11:51:14 Will Deacon wrote:
Quoted 22 lines
Hi all,
This series introduces a generic IOMMU page table allocation framework, implements support for ARM long-descriptors and then ports the arm-smmu driver over to the new code.
There are a few reasons for doing this:
- Page table code is hard, and I don't enjoy shopping
- A number of IOMMUs actually use the same table format, but currently duplicate the code
- It provides a CPU (and architecture) independent allocator, which may be useful for some systems where the CPU is using a different table format for its own mappings
As illustrated in the final patch, an IOMMU driver interacts with the allocator by passing in a configuration structure describing the input and output address ranges, the supported pages sizes and a set of ops for performing various TLB invalidation and PTE flushing routines.
Quoted 7 lines
Quoted 4 lines
The LPAE code implements support for 4k/2M/1G, 16k/32M and 64k/512M mappings, but I decided not to implement the contiguous bit in the interest of trying to keep the code semi-readable. This could always be added later, if needed.
Do you have any idea how much the contiguous bit can improve performances in real use cases ?
It depends on the TLB, really. Given that the contiguous sized map directly onto block sizes using different granules, I didn't see that the complexity was worth it.
For example:
4k granule : 16 contiguous entries => {64k, 32M, 16G}
16k granule : 128 contiguous lvl3 entries => 2M
32 contiguous lvl2 entries => 1G
64k granule : 32 contiguous entries => {2M, 16G}
If we use block mappings, then we get:
4k granule : 2M @ lvl2, 1G @ lvl1
16k granule : 32M @ lvl2
64k granule : 512M @ lvl2
so really, we only miss the ability to create 16G mappings. I doubt that hardware even implements that size in the TLB (the contiguous bit is only a hint).
On top of that, the contiguous bit leads to additional expense on unmap, since you have extra TLB invalidation splitting the thing into non-contiguous pages before you can do anything.
Will
cover Generic IOMMU page table framework
Hi Will,
Quoted 42 lines · On Monday 01 December 2014 12:05:34 Will Deacon wrote:
Quoted 22 lines · Laurent Pinchart
Quoted 21 lines · On Thursday 27 November 2014 11:51:14 Will Deacon wrote:
Hi all,
This series introduces a generic IOMMU page table allocation framework, implements support for ARM long-descriptors and then ports the arm-smmu driver over to the new code.
There are a few reasons for doing this:
- Page table code is hard, and I don't enjoy shopping- A number of IOMMUs actually use the same table format, but currently duplicate the code
- It provides a CPU (and architecture) independent allocator, which may be useful for some systems where the CPU is using a different table format for its own mappings
As illustrated in the final patch, an IOMMU driver interacts with the allocator by passing in a configuration structure describing the input and output address ranges, the supported pages sizes and a set of ops for performing various TLB invalidation and PTE flushing routines.
Quoted 7 lines
Quoted 4 lines
The LPAE code implements support for 4k/2M/1G, 16k/32M and 64k/512M mappings, but I decided not to implement the contiguous bit in the interest of trying to keep the code semi-readable. This could always be added later, if needed.
Do you have any idea how much the contiguous bit can improve performances in real use cases ?
It depends on the TLB, really. Given that the contiguous sized map directly onto block sizes using different granules, I didn't see that the complexity was worth it.
For example:
4k granule : 16 contiguous entries => {64k, 32M, 16G}16k granule : 128 contiguous lvl3 entries => 2M
32 contiguous lvl2 entries => 1G
64k granule : 32 contiguous entries => {2M, 16G}
Quoted 7 lines
If we use block mappings, then we get:
4k granule : 2M @ lvl2, 1G @ lvl116k granule : 32M @ lvl2
64k granule : 512M @ lvl2so really, we only miss the ability to create 16G mappings.
In the general case maybe, but as far as I know my IOMMU only supports 4kB granule. Without support for the contiguous bit I loose the ability to create 64kB mappings, which I believe (but haven't test yet) will have a noticeable impact.
Quoted 6 lines
I doubt that hardware even implements that size in the TLB (the contiguous bit is only a hint).
On top of that, the contiguous bit leads to additional expense on unmap, since you have extra TLB invalidation splitting the thing into non- contiguous pages before you can do anything.
That will only be required when doing partial unmaps, which shouldn't be that frequent. When unmapping a 64kB block there's no need to split the mapping beforehand.
Signature · 3 lines
Regards, Laurent Pinchart
cover Generic IOMMU page table framework
Quoted 28 lines · Laurent Pinchart
Quoted 27 lines · On Monday 01 December 2014 12:05:34 Will Deacon wrote:
Quoted 8 lines · Laurent Pinchart
Quoted 4 lines · On Thursday 27 November 2014 11:51:14 Will Deacon wrote:
The LPAE code implements support for 4k/2M/1G, 16k/32M and 64k/512M mappings, but I decided not to implement the contiguous bit in the interest of trying to keep the code semi-readable. This could always be added later, if needed.
Do you have any idea how much the contiguous bit can improve performances in real use cases ?
It depends on the TLB, really. Given that the contiguous sized map directly onto block sizes using different granules, I didn't see that the complexity was worth it.
For example:
4k granule : 16 contiguous entries => {64k, 32M, 16G}16k granule : 128 contiguous lvl3 entries => 2M
32 contiguous lvl2 entries => 1G
64k granule : 32 contiguous entries => {2M, 16G}If we use block mappings, then we get:
4k granule : 2M @ lvl2, 1G @ lvl116k granule : 32M @ lvl2
64k granule : 512M @ lvl2
Quoted 6 lines
Quoted 1 line
so really, we only miss the ability to create 16G mappings.
In the general case maybe, but as far as I know my IOMMU only supports 4kB granule. Without support for the contiguous bit I loose the ability to create 64kB mappings, which I believe (but haven't test yet) will have a noticeable impact.
It would be good if you could confirm that. I'd have thought that you'd end up using 2MB mappings most of the time for DMA buffers.
Quoted 10 lines
Quoted 6 lines
I doubt that hardware even implements that size in the TLB (the contiguous bit is only a hint).
On top of that, the contiguous bit leads to additional expense on unmap, since you have extra TLB invalidation splitting the thing into non- contiguous pages before you can do anything.
That will only be required when doing partial unmaps, which shouldn't be that frequent. When unmapping a 64kB block there's no need to split the mapping beforehand.
Sure. I'm not against having support for the contiguous bit, I just don't plan to implement it myself :)
Will
cover Generic IOMMU page table framework
Hi Will,
Quoted 29 lines · On Tuesday 02 December 2014 13:53:56 Will Deacon wrote:
Quoted 28 lines · Laurent Pinchart
Quoted 26 lines · On Monday 01 December 2014 12:05:34 Will Deacon wrote:
Quoted 8 lines · Laurent Pinchart
Quoted 4 lines · On Thursday 27 November 2014 11:51:14 Will Deacon wrote:
The LPAE code implements support for 4k/2M/1G, 16k/32M and 64k/512M mappings, but I decided not to implement the contiguous bit in the interest of trying to keep the code semi-readable. This could always be added later, if needed.
Do you have any idea how much the contiguous bit can improve performances in real use cases ?
It depends on the TLB, really. Given that the contiguous sized map directly onto block sizes using different granules, I didn't see that the complexity was worth it.
For example:
4k granule : 16 contiguous entries => {64k, 32M, 16G}16k granule : 128 contiguous lvl3 entries => 2M
32 contiguous lvl2 entries => 1G
64k granule : 32 contiguous entries => {2M, 16G}If we use block mappings, then we get:
4k granule : 2M @ lvl2, 1G @ lvl116k granule : 32M @ lvl2
64k granule : 512M @ lvl2so really, we only miss the ability to create 16G mappings.
Quoted 7 lines
Quoted 4 lines
In the general case maybe, but as far as I know my IOMMU only supports 4kB granule. Without support for the contiguous bit I loose the ability to create 64kB mappings, which I believe (but haven't test yet) will have a noticeable impact.
It would be good if you could confirm that. I'd have thought that you'd end up using 2MB mappings most of the time for DMA buffers.
I'll try to gather statistics as soon as I can get TLB flushing working reliably. Without it turning the IOMMU on kills the system pretty fast :-)
Quoted 13 lines
Quoted 10 lines
Quoted 6 lines
I doubt that hardware even implements that size in the TLB (the contiguous bit is only a hint).
On top of that, the contiguous bit leads to additional expense on unmap, since you have extra TLB invalidation splitting the thing into non- contiguous pages before you can do anything.
That will only be required when doing partial unmaps, which shouldn't be that frequent. When unmapping a 64kB block there's no need to split the mapping beforehand.
Sure. I'm not against having support for the contiguous bit, I just don't plan to implement it myself :)
Signature · 3 lines
Regards, Laurent Pinchart
cover Generic IOMMU page table framework
In reply to Will DeaconHi Will,
Quoted 31 lines · On Thursday 27 November 2014 11:51:14 Will Deacon wrote:
Hi all,
This series introduces a generic IOMMU page table allocation framework, implements support for ARM long-descriptors and then ports the arm-smmu driver over to the new code.
There are a few reasons for doing this:
- Page table code is hard, and I don't enjoy shopping
- A number of IOMMUs actually use the same table format, but currently duplicate the code
- It provides a CPU (and architecture) independent allocator, which may be useful for some systems where the CPU is using a different table format for its own mappings
As illustrated in the final patch, an IOMMU driver interacts with the allocator by passing in a configuration structure describing the input and output address ranges, the supported pages sizes and a set of ops for performing various TLB invalidation and PTE flushing routines.
The LPAE code implements support for 4k/2M/1G, 16k/32M and 64k/512M mappings, but I decided not to implement the contiguous bit in the interest of trying to keep the code semi-readable. This could always be added later, if needed.
I also included some self-tests for the LPAE implementation. Ideally we'd merge these, but I'm also happy to drop them if there are objections.
Quoted 5 lines
Tested with the self-tests, but also VFIO + MMU-500 at stage-1 and stage-2. Patches taken against my iommu/devel branch (queued by Joerg for 3.19).
All feedback welcome.
I've successfully tested the patch set with the Renesas IPMMU-VMSA driver with the following extension to the allocator.
Quoted 1 line
From
4bebb7f3a5a48541d4c89ce7c61e6ff66686c3a9Mon Sep 17 00:00:00 2001
From: Laurent Pinchart <[email protected]>
Date: Sun, 14 Dec 2014 23:34:50 +0200
Subject: [PATCH] iommu: io-pgtable-arm: Add Non-Secure quirk
The quirk causes the Non-Secure bit to be set in all page table entries.
--- drivers/iommu/io-pgtable-arm.c | 7 +++++++ drivers/iommu/io-pgtable.h | 3 +++ 2 files changed, 10 insertions(+)
drivers/iommu/io-pgtable-arm.c +7 -0
@@ -80,11 +80,13 @@#define ARM_LPAE_PTE_TYPE_TABLE 3#define ARM_LPAE_PTE_TYPE_PAGE 3 +#define ARM_LPAE_PTE_NSTABLE (((arm_lpae_iopte)1) << 63)#define ARM_LPAE_PTE_XN (((arm_lpae_iopte)3) << 53)#define ARM_LPAE_PTE_AF (((arm_lpae_iopte)1) << 10)#define ARM_LPAE_PTE_SH_NS (((arm_lpae_iopte)0) << 8)#define ARM_LPAE_PTE_SH_OS (((arm_lpae_iopte)2) << 8)#define ARM_LPAE_PTE_SH_IS (((arm_lpae_iopte)3) << 8) +#define ARM_LPAE_PTE_NS (((arm_lpae_iopte)1) << 5)#define ARM_LPAE_PTE_VALID (((arm_lpae_iopte)1) << 0)#define ARM_LPAE_PTE_ATTR_LO_MASK (((arm_lpae_iopte)0x3ff) << 2)
@@ -201,6 +203,9 @@ static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,if (iopte_leaf (*ptep, lvl))return -EEXIST ; +if (data->iop.cfg.quirks &IO_PGTABLE_QUIRK_NON_SECURE ) + pte |=ARM_LPAE_PTE_NS ; +if (lvl ==ARM_LPAE_MAX_LEVELS -1 ) pte |=ARM_LPAE_PTE_TYPE_PAGE ;else
@@ -244,6 +249,8 @@ static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long iova, data->iop.cfg.tlb->flush_pgtable (cptep,1UL << data->pg_shift, cookie); pte =__pa (cptep) |ARM_LPAE_PTE_TYPE_TABLE ; +if (data->iop.cfg.quirks &IO_PGTABLE_QUIRK_NON_SECURE ) + pte |=ARM_LPAE_PTE_NSTABLE ; *ptep = pte; data->iop.cfg.tlb->flush_pgtable (ptep,sizeof (*ptep), cookie); }else {
drivers/iommu/io-pgtable.h +3 -0
@@ -24,6 +24,9 @@ struct iommu_gather_ops {void (*flush_pgtable)(void *ptr,size_t size,void *cookie); }; +/* Set the Non-Secure bit in the PTEs */ +#define IO_PGTABLE_QUIRK_NON_SECURE (1 << 0) +struct io_pgtable_cfg {int quirks;/* IO_PGTABLE_QUIRK_* */ unsigned long pgsize_bitmap;
Quoted 20 lines
--->8
Will Deacon (4):
iommu: introduce generic page table allocation framework
iommu: add ARM LPAE page table allocator
iommu: add self-consistency tests to ARM LPAE IO page table allocator iommu/arm-smmu: make use of generic LPAE allocatorMAINTAINERS | 1 + arch/arm64/Kconfig | 1 - drivers/iommu/Kconfig | 32 +- drivers/iommu/Makefile | 2 + drivers/iommu/arm-smmu.c | 872 +++++++++++--------------------------- drivers/iommu/io-pgtable-arm.c | 925 ++++++++++++++++++++++++++++++++++++++ drivers/iommu/io-pgtable.c | 78 ++++ drivers/iommu/io-pgtable.h | 77 ++++ 8 files changed, 1361 insertions(+), 627 deletions(-) create mode 100644 drivers/iommu/io-pgtable-arm.c create mode 100644 drivers/iommu/io-pgtable.c create mode 100644 drivers/iommu/io-pgtable.h
Signature · 3 lines
Regards, Laurent Pinchart
cover Generic IOMMU page table framework
Quoted 1 line · Laurent Pinchart
Hi Will,
Hi Laurent,
Quoted 4 lines · On Thursday 27 November 2014 11:51:14 Will Deacon wrote:
Quoted 3 lines
This series introduces a generic IOMMU page table allocation framework, implements support for ARM long-descriptors and then ports the arm-smmu driver over to the new code.
[...]
Quoted 6 lines
Quoted 1 line
All feedback welcome.
I've successfully tested the patch set with the Renesas IPMMU-VMSA driver with the following extension to the allocator.
Tested-by Laurent Pinchart <[email protected]>
Wahey, that's really cool, thanks! I have a few minor comments on your patch below. If you don't object, then I can make them locally and include your patch on top of my v2 series?
Quoted 55 lines
From
4bebb7f3a5a48541d4c89ce7c61e6ff66686c3a9Mon Sep 17 00:00:00 2001
From: Laurent Pinchart <[email protected]>
Date: Sun, 14 Dec 2014 23:34:50 +0200
Subject: [PATCH] iommu: io-pgtable-arm: Add Non-Secure quirkThe quirk causes the Non-Secure bit to be set in all page table entries.
Signed-off-by Laurent Pinchart <[email protected]>--- drivers/iommu/io-pgtable-arm.c | 7 +++++++ drivers/iommu/io-pgtable.h | 3 +++ 2 files changed, 10 insertions(+)drivers/iommu/io-pgtable-arm.c +7
-0@@ -80,11 +80,13 @@#define ARM_LPAE_PTE_TYPE_TABLE 3#define ARM_LPAE_PTE_TYPE_PAGE 3 +#define ARM_LPAE_PTE_NSTABLE (((arm_lpae_iopte)1) << 63)#define ARM_LPAE_PTE_XN (((arm_lpae_iopte)3) << 53)#define ARM_LPAE_PTE_AF (((arm_lpae_iopte)1) << 10)#define ARM_LPAE_PTE_SH_NS (((arm_lpae_iopte)0) << 8)#define ARM_LPAE_PTE_SH_OS (((arm_lpae_iopte)2) << 8)#define ARM_LPAE_PTE_SH_IS (((arm_lpae_iopte)3) << 8) +#define ARM_LPAE_PTE_NS (((arm_lpae_iopte)1) << 5)#define ARM_LPAE_PTE_VALID (((arm_lpae_iopte)1) << 0)#define ARM_LPAE_PTE_ATTR_LO_MASK (((arm_lpae_iopte)0x3ff) << 2)@@ -201,6 +203,9 @@ static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,if (iopte_leaf (*ptep, lvl))return -EEXIST ; +if (data->iop.cfg.quirks &IO_PGTABLE_QUIRK_NON_SECURE ) + pte |=ARM_LPAE_PTE_NS ; +if (lvl ==ARM_LPAE_MAX_LEVELS -1 ) pte |=ARM_LPAE_PTE_TYPE_PAGE ;else @@ -244,6 +249,8 @@ static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long iova, data->iop.cfg.tlb->flush_pgtable (cptep,1UL << data->pg_shift, cookie); pte =__pa (cptep) |ARM_LPAE_PTE_TYPE_TABLE ; +if (data->iop.cfg.quirks &IO_PGTABLE_QUIRK_NON_SECURE ) + pte |=ARM_LPAE_PTE_NSTABLE ; *ptep = pte; data->iop.cfg.tlb->flush_pgtable (ptep,sizeof (*ptep), cookie); }else {drivers/iommu/io-pgtable.h +0
-0
drivers/iommu/io-pgtable.h struct iommu_gather_ops { · line 28
Quoted 5 lines
drivers/iommu/io-pgtable.h +2
-0@@ -24,6 +24,9 @@ struct iommu_gather_ops {void (*flush_pgtable)(void *ptr,size_t size,void *cookie); }; +/* Set the Non-Secure bit in the PTEs */ +#define IO_PGTABLE_QUIRK_NON_SECURE (1 << 0)
I think I'd stick an _ARM_ somewhere in here, so maybe IO_PGTABLE_QUIRK_ARM_NS?
drivers/iommu/io-pgtable.h struct iommu_gather_ops { · line 30
Quoted 2 lines
drivers/iommu/io-pgtable.h +1
-0+struct io_pgtable_cfg {
and I'd put the #define here, next to the member.
drivers/iommu/io-pgtable.h struct iommu_gather_ops { · line 32
Quoted 3 lines
drivers/iommu/io-pgtable.h +0
-0int quirks;/* IO_PGTABLE_QUIRK_* */ unsigned long pgsize_bitmap;
Will
cover Generic IOMMU page table framework
Hi Will,
Quoted 10 lines · On Monday 15 December 2014 16:10:52 Will Deacon wrote:
Quoted 4 lines · Laurent Pinchart
Quoted 3 lines · On Thursday 27 November 2014 11:51:14 Will Deacon wrote:
This series introduces a generic IOMMU page table allocation framework, implements support for ARM long-descriptors and then ports the arm-smmu driver over to the new code.
[...]
Quoted 2 lines
Quoted 1 line
All feedback welcome.
Quoted 8 lines
Quoted 4 lines
I've successfully tested the patch set with the Renesas IPMMU-VMSA driver with the following extension to the allocator.
Tested-by Laurent Pinchart <[email protected]>Wahey, that's really cool, thanks! I have a few minor comments on your patch below. If you don't object, then I can make them locally and include your patch on top of my v2 series?
Sure. Please see my reply below.
Quoted 62 lines
Quoted 62 lines
From
4bebb7f3a5a48541d4c89ce7c61e6ff66686c3a9Mon Sep 17 00:00:00 2001
From: Laurent Pinchart <[email protected]>
Date: Sun, 14 Dec 2014 23:34:50 +0200
Subject: [PATCH] iommu: io-pgtable-arm: Add Non-Secure quirkThe quirk causes the Non-Secure bit to be set in all page table entries.
Signed-off-by: Laurent Pinchart
<[email protected]>
---drivers/iommu/io-pgtable-arm.c | 7 +++++++ drivers/iommu/io-pgtable.h | 3 +++ 2 files changed, 10 insertions(+)(quoted diff) +0
-0
b/drivers/iommu/io-pgtable-arm.cindex669e322a83a4..b6910e142734100644drivers/iommu/io-pgtable-arm.c +2
-0@@ -80,11 +80,13 @@#define ARM_LPAE_PTE_TYPE_TABLE 3#define ARM_LPAE_PTE_TYPE_PAGE 3 +#define ARM_LPAE_PTE_NSTABLE (((arm_lpae_iopte)1) << 63)#define ARM_LPAE_PTE_XN (((arm_lpae_iopte)3) << 53)#define ARM_LPAE_PTE_AF (((arm_lpae_iopte)1) << 10)#define ARM_LPAE_PTE_SH_NS (((arm_lpae_iopte)0) << 8)#define ARM_LPAE_PTE_SH_OS (((arm_lpae_iopte)2) << 8)#define ARM_LPAE_PTE_SH_IS (((arm_lpae_iopte)3) << 8) +#define ARM_LPAE_PTE_NS (((arm_lpae_iopte)1) << 5)#define ARM_LPAE_PTE_VALID (((arm_lpae_iopte)1) << 0)#define ARM_LPAE_PTE_ATTR_LO_MASK (((arm_lpae_iopte)0x3ff) << 2)@@ -201,6 +203,9 @@ static int arm_lpae_init_pte(struct
arm_lpae_io_pgtable*data,>drivers/iommu/io-pgtable-arm.c +5
-0if (iopte_leaf (*ptep, lvl))return -EEXIST ; +if (data->iop.cfg.quirks &IO_PGTABLE_QUIRK_NON_SECURE ) + pte |=ARM_LPAE_PTE_NS ; +if (lvl ==ARM_LPAE_MAX_LEVELS -1 ) pte |=ARM_LPAE_PTE_TYPE_PAGE ;else @@ -244,6 +249,8 @@ static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long iova,> data->iop.cfg.tlb->flush_pgtable(cptep, 1UL << data->pg_shift,cookie ); pte =__pa (cptep) |ARM_LPAE_PTE_TYPE_TABLE ; +if (data->iop.cfg.quirks &IO_PGTABLE_QUIRK_NON_SECURE ) + pte |=ARM_LPAE_PTE_NSTABLE ; *ptep = pte; data->iop.cfg.tlb->flush_pgtable (ptep,sizeof (*ptep), cookie);drivers/iommu/io-pgtable-arm.c +0
-0}else {drivers/iommu/io-pgtable.h +0
-0@@ -24,6 +24,9 @@ struct iommu_gather_ops {void (*flush_pgtable)(void *ptr,size_t size,void *cookie); };
Quoted 5 lines
drivers/iommu/io-pgtable.h struct iommu_gather_ops { · line 28
Quoted 2 lines
drivers/iommu/io-pgtable.h +2
-0+/* Set the Non-Secure bit in the PTEs */ +#define IO_PGTABLE_QUIRK_NON_SECURE (1 << 0)I think I'd stick an _ARM_ somewhere in here, so maybe
IO_PGTABLE_QUIRK_ARM_NS?
I'm fine with that.
By the way, I'm only familiar with the Renesas implementation of the VMSA IOMMU, could you double-check whether setting the NSTABLE and NS bits on all levels make sense to you ? It seems to be required by my hardware, even though the ARM spec mentions that setting the NSTABLE bit causes non-secure accesses to page tables for all lower levels regardless of their NSTABLE/NS bits.
Quoted 5 lines
drivers/iommu/io-pgtable.h struct iommu_gather_ops { · line 30
Quoted 3 lines
drivers/iommu/io-pgtable.h +1
-0+struct io_pgtable_cfg {and I'd put the #define here, next to the member.
They're right before the structure so I don't think they're too far away, but if you prefer that coding style that's fine with me.
Quoted 2 lines
Quoted 2 lines
drivers/iommu/io-pgtable.h +0
-0int quirks;/* IO_PGTABLE_QUIRK_* */ unsigned long
pgsize_bitmap;
Signature · 3 lines
Regards, Laurent Pinchart
cover Generic IOMMU page table framework
Quoted 16 lines · Laurent Pinchart
Quoted 9 lines · On Monday 15 December 2014 16:10:52 Will Deacon wrote:
Quoted 8 lines · Laurent Pinchart
drivers/iommu/io-pgtable.h +0
-0@@ -24,6 +24,9 @@ struct iommu_gather_ops {void (*flush_pgtable)(void *ptr,size_t size,void *cookie); };Quoted 5 lines
drivers/iommu/io-pgtable.h struct iommu_gather_ops { · line 28
Quoted 2 lines
drivers/iommu/io-pgtable.h +2
-0+/* Set the Non-Secure bit in the PTEs */ +#define IO_PGTABLE_QUIRK_NON_SECURE (1 << 0)I think I'd stick an _ARM_ somewhere in here, so maybe
IO_PGTABLE_QUIRK_ARM_NS?
Quoted 7 lines
I'm fine with that.
By the way, I'm only familiar with the Renesas implementation of the VMSA IOMMU, could you double-check whether setting the NSTABLE and NS bits on all levels make sense to you ? It seems to be required by my hardware, even though the ARM spec mentions that setting the NSTABLE bit causes non-secure accesses to page tables for all lower levels regardless of their NSTABLE/NS bits.
The ARM ARM is very clear that subsequent levels of lookup must ignore the NSTABLE/NS bits since otherwise you potentially have a security violation where you can use the table walker to access secure memory from non-secure...
So, you might want to check up on that, but given that this is a quirk I'm happy for it to do whatever you need.
Will
cover Generic IOMMU page table framework
Hi Will,
Quoted 28 lines · On Monday 15 December 2014 17:39:11 Will Deacon wrote:
Quoted 18 lines · Laurent Pinchart
Quoted 9 lines · On Monday 15 December 2014 16:10:52 Will Deacon wrote:
Quoted 8 lines · Laurent Pinchart
drivers/iommu/io-pgtable.h +0
-0@@ -24,6 +24,9 @@ struct iommu_gather_ops {void (*flush_pgtable)(void *ptr,size_t size,void *cookie);Quoted 7 lines
drivers/iommu/io-pgtable.h struct iommu_gather_ops { · line 28
Quoted 4 lines
drivers/iommu/io-pgtable.h +2
-0}; +/* Set the Non-Secure bit in the PTEs */ +#define IO_PGTABLE_QUIRK_NON_SECURE (1 << 0)I think I'd stick an _ARM_ somewhere in here, so maybe
IO_PGTABLE_QUIRK_ARM_NS?Quoted 8 lines
I'm fine with that.
By the way, I'm only familiar with the Renesas implementation of the VMSA IOMMU, could you double-check whether setting the NSTABLE and NS bits on all levels make sense to you ? It seems to be required by my hardware, even though the ARM spec mentions that setting the NSTABLE bit causes non-secure accesses to page tables for all lower levels regardless of their NSTABLE/NS bits.
Quoted 7 lines
The ARM ARM is very clear that subsequent levels of lookup must ignore the NSTABLE/NS bits since otherwise you potentially have a security violation where you can use the table walker to access secure memory from non-secure...
So, you might want to check up on that, but given that this is a quirk I'm happy for it to do whatever you need.
Given that the ARM ARM states that subsequent levels must consider the NSTABLE/NS bits to be set, I think it's harmless to actually set them. I just wanted to double-check with you, as we agree let's proceed with the proposed patch.
Signature · 3 lines
Regards, Laurent Pinchart