io-pgtable ARM short descriptor format

PATCH New Robin Murphy 5 participants 23 messages Jan 26, 2016 – Mar 1, 2016

Subsystems (2)

IOMMU Arm SMMU drivers

lore

Share & tools

lore

Robin Murphy

author reviewer

2016-01-26 17:13 UTC #0 lore raw

v3 cover io-pgtable ARM short descriptor format

Hi all,

As promised, here's v3 based on 4.5-rc1, with all the previous comments addressed other than the logic of split_blk_unmap() - having looked into it, trying to simplify that essentially just pushes the complexity out into map() and complicates the general case. Ultimately it's an awkward edge case to handle however it's implemented, and keeping it as close as possible to the LPAE version makes it somewhat easier to reason about.

This is currently just a dependency for the Mediatek IOMMU series[1] - the ARM SMMU driver support I've been testing with is now much closer to "proper patch" than "horrible development hack", but still depends on a bunch of other work in that driver I need to post first.

Thanks,
Robin.

Changes since v2[2]:
- #include dma-mapping.h before I break ARC again...
- Fix PTE type bit swizzling to not collide with XN bit in cont_to_pte().
- Correct init_pte to increment the IOVA, rather than the table pointer, when freeing L2 tables being replaced by a supersection entry.
- Rename sync_flag to the much better tlb_sync_pending.
- Acks on patches 2 and 3.

[1]:http://thread.gmane.org/gmane.linux.kernel.iommu/11974 [2]:http://thread.gmane.org/gmane.linux.kernel.iommu/11752

Robin Murphy (3):
iommu/io-pgtable: Add ARMv7 short descriptor support iommu/io-pgtable: Add helper functions for TLB ops iommu/io-pgtable: Avoid redundant TLB syncs

 drivers/iommu/Kconfig              |  19 +
 drivers/iommu/Makefile             |   1 +
 drivers/iommu/io-pgtable-arm-v7s.c | 841 +++++++++++++++++++++++++++++++++++++
 drivers/iommu/io-pgtable-arm.c     |  21 +-
 drivers/iommu/io-pgtable.c         |   5 +-
 drivers/iommu/io-pgtable.h         |  37 +-
 6 files changed, 910 insertions(+), 14 deletions(-)
 create mode 100644 drivers/iommu/io-pgtable-arm-v7s.c

Signature · 1 line

2.7.0.25.gfc10eb5

Robin Murphy

author reviewer

2016-01-26 17:13 UTC #1 lore raw

PATCH v3 1/3 iommu/io-pgtable: Add ARMv7 short descriptor support

Add a nearly-complete ARMv7 short descriptor implementation, omitting only a few legacy and CPU-centric aspects which shouldn't be necessary for IOMMU API use anyway.

Signed-off-by Yong Wu <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/Kconfig              |  19 +
 drivers/iommu/Makefile             |   1 +
 drivers/iommu/io-pgtable-arm-v7s.c | 849 +++++++++++++++++++++++++++++++++++++
 drivers/iommu/io-pgtable.c         |   3 +
 drivers/iommu/io-pgtable.h         |  14 +-
 5 files changed, 885 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iommu/io-pgtable-arm-v7s.c

drivers/iommu/Kconfig +19 -0

index a1e75cb..dc1aaa5 100644
@@ -39,6 +39,25 @@ config IOMMU_IO_PGTABLE_LPAE_SELFTEST
 
 	  If unsure, say N here.
 
+config IOMMU_IO_PGTABLE_ARMV7S
+	bool "ARMv7/v8 Short Descriptor Format"
+	select IOMMU_IO_PGTABLE
+	depends on HAS_DMA && (ARM || ARM64 || COMPILE_TEST)
+	help
+	  Enable support for the ARM Short-descriptor pagetable format.
+	  This supports 32-bit virtual and physical addresses mapped using
+	  2-level tables with 4KB pages/1MB sections, and contiguous entries
+	  for 64KB pages/16MB supersections if indicated by the IOMMU driver.
+
+config IOMMU_IO_PGTABLE_ARMV7S_SELFTEST
+	bool "ARMv7s selftests"
+	depends on IOMMU_IO_PGTABLE_ARMV7S
+	help
+	  Enable self-tests for ARMv7s page table allocator. This performs
+	  a series of page-table consistency checks during boot.
+
+	  If unsure, say N here.
+
 endmenu
 
 config IOMMU_IOVA

drivers/iommu/Makefile +1 -0

index 42fc0c2..2f9bfbc 100644
@@ -3,6 +3,7 @@ obj-$(CONFIG_IOMMU_API) += iommu-traces.o
 obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
 obj-$(CONFIG_IOMMU_DMA) += dma-iommu.o
 obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
+obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
 obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o
 obj-$(CONFIG_IOMMU_IOVA) += iova.o
 obj-$(CONFIG_OF_IOMMU)	+= of_iommu.o

drivers/iommu/io-pgtable-arm-v7s.c new file +849 -0

849 lines not on this page · raw message

drivers/iommu/io-pgtable.c +3 -0

index 6f2e319..8c615b7 100644
@@ -33,6 +33,9 @@ io_pgtable_init_table[IO_PGTABLE_NUM_FMTS] =
 	[ARM_64_LPAE_S1] = &io_pgtable_arm_64_lpae_s1_init_fns,
 	[ARM_64_LPAE_S2] = &io_pgtable_arm_64_lpae_s2_init_fns,
 #endif
+#ifdef CONFIG_IOMMU_IO_PGTABLE_ARMV7S
+	[ARM_V7S] = &io_pgtable_arm_v7s_init_fns,
+#endif
 };
 
 struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt,

drivers/iommu/io-pgtable.h +13 -1

index 36673c8..aa57073 100644
@@ -1,5 +1,6 @@
 #ifndef __IO_PGTABLE_H
 #define __IO_PGTABLE_H
+#include <linux/bitops.h>
 
 /*
  * Public API for use by IOMMU drivers

@@ -9,6 +10,7 @@ enum io_pgtable_fmt {
 	ARM_32_LPAE_S2,
 	ARM_64_LPAE_S1,
 	ARM_64_LPAE_S2,
+	ARM_V7S,
 	IO_PGTABLE_NUM_FMTS,
 };

@@ -45,7 +47,9 @@ struct iommu_gather_ops {
  *                 page table walker.
  */
 struct io_pgtable_cfg {
-	#define IO_PGTABLE_QUIRK_ARM_NS	(1 << 0)	/* Set NS bit in PTEs */
+	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0) /* Set NS bit in PTEs */
+	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1) /* No AP/XN bits */
+	#define IO_PGTABLE_QUIRK_TLBI_ON_MAP	BIT(2) /* TLB Inv. on map */
 	int				quirks;
 	unsigned long			pgsize_bitmap;
 	unsigned int			ias;

@@ -65,6 +69,13 @@ struct io_pgtable_cfg {
 			u64	vttbr;
 			u64	vtcr;
 		} arm_lpae_s2_cfg;
+
+		struct {
+			u32	ttbr[2];
+			u32	tcr;
+			u32	nmrr;
+			u32	prrr;
+		} arm_v7s_cfg;
 	};
 };

@@ -149,5 +160,6 @@ extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns;
 extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns;
 extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns;
 extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns;
+extern struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns;
 
 #endif /* __IO_PGTABLE_H */

Signature · 1 line

2.7.0.25.gfc10eb5

Yong Wu

2016-01-27 01:16 UTC #2 lore raw

PATCH v3 1/3 iommu/io-pgtable: Add ARMv7 short descriptor support

Quoted 6 lines · Robin Murphy

Add a nearly-complete ARMv7 short descriptor implementation, omitting only a few legacy and CPU-centric aspects which shouldn't be necessary for IOMMU API use anyway.
Signed-off-by Yong Wu <[email protected]>
Signed-off-by Robin Murphy <[email protected]>

This looks good to me.
And actually our disp/vdec/venc works well on your v2.

You could choose the two below,

Reviewed-by Yong Wu <[email protected]>

Tested-by Yong Wu <[email protected]>

Thanks.

Quoted 8 lines

---
 drivers/iommu/Kconfig              |  19 +
 drivers/iommu/Makefile             |   1 +
 drivers/iommu/io-pgtable-arm-v7s.c | 849 +++++++++++++++++++++++++++++++++++++
 drivers/iommu/io-pgtable.c         |   3 +
 drivers/iommu/io-pgtable.h         |  14 +-
 5 files changed, 885 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iommu/io-pgtable-arm-v7s.c

Robin Murphy

author reviewer

2016-01-28 13:41 UTC #3 lore raw

PATCH v3 1/3 iommu/io-pgtable: Add ARMv7 short descriptor support

Quoted 8 lines · Yong Wu

Quoted 6 lines · Robin Murphy
Add a nearly-complete ARMv7 short descriptor implementation, omitting only a few legacy and CPU-centric aspects which shouldn't be necessary for IOMMU API use anyway.
Signed-off-by Yong Wu <[email protected]>
Signed-off-by Robin Murphy <[email protected]>

Quoted 6 lines

This looks good to me.
And actually our disp/vdec/venc works well on your v2.
You could choose the two below,
Reviewed-by Yong Wu <[email protected]>
Tested-by Yong Wu <[email protected]>

Great, thanks!

Will, it seems to make the most sense for Joerg to pick these patches up directly with the MTK series that depends on them - any objections?

Robin.

Quoted 14 lines

Thanks.

Quoted 8 lines

---
  drivers/iommu/Kconfig              |  19 +
  drivers/iommu/Makefile             |   1 +
  drivers/iommu/io-pgtable-arm-v7s.c | 849 +++++++++++++++++++++++++++++++++++++
  drivers/iommu/io-pgtable.c         |   3 +
  drivers/iommu/io-pgtable.h         |  14 +-
  5 files changed, 885 insertions(+), 1 deletion(-)

create mode 100644 drivers/iommu/io-pgtable-arm-v7s.c

Will Deacon

2016-02-17 14:31 UTC #4 lore raw

PATCH v3 1/3 iommu/io-pgtable: Add ARMv7 short descriptor support

Hi Robin,

Quoted 12 lines · Robin Murphy

Quoted 11 lines · Yong Wu
Quoted 6 lines · Robin Murphy
Add a nearly-complete ARMv7 short descriptor implementation, omitting only a few legacy and CPU-centric aspects which shouldn't be necessary for IOMMU API use anyway.
Signed-off-by Yong Wu <[email protected]>
Signed-off-by Robin Murphy <[email protected]>
This looks good to me.
And actually our disp/vdec/venc works well on your v2.

Quoted 8 lines

Quoted 3 lines
You could choose the two below,
Reviewed-by Yong Wu <[email protected]>
Tested-by Yong Wu <[email protected]>
Great, thanks!
Will, it seems to make the most sense for Joerg to pick these patches up directly with the MTK series that depends on them - any objections?

As discussed with Joerg, I'll pick these up and send him a pull request containing all the pgtable bits. I've taken the liberty of updating MAINTAINERS too, assuming you don't mind being a reviewer for this?

Will

--->8

Quoted 1 line

From bccb08d096703bd790fcf2f934553dd9c8503fc9 Mon Sep 17 00:00:00 2001

From: Will Deacon <[email protected]>
Date: Wed, 17 Feb 2016 14:18:00 +0000
Subject: [PATCH] MAINTAINERS: update ARM SMMU entry

Ensure that the short-descriptor page table code is included under the SMMU entry, and add Robin as a designated reviewer.

Cc Robin Murphy <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

MAINTAINERS +2 -0

index cc2f753cb357..a01bae148b89 100644
@@ -1793,11 +1793,13 @@ F:	drivers/edac/synopsys_edac.c
 
 ARM SMMU DRIVERS
 M:	Will Deacon <[email protected]>
+R:	Robin Murphy <[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/arm-smmu-v3.c
 F:	drivers/iommu/io-pgtable-arm.c
+F:	drivers/iommu/io-pgtable-arm-v7s.c
 
 ARM64 PORT (AARCH64 ARCHITECTURE)
 M:	Catalin Marinas <[email protected]>

Signature · 1 line

2.1.4

Robin Murphy

author reviewer

2016-02-17 14:34 UTC #5 lore raw

PATCH v3 1/3 iommu/io-pgtable: Add ARMv7 short descriptor support

Quoted 19 lines · Will Deacon

Hi Robin,
Quoted 16 lines · Robin Murphy
Quoted 14 lines · Yong Wu
Quoted 6 lines · Robin Murphy
Add a nearly-complete ARMv7 short descriptor implementation, omitting only a few legacy and CPU-centric aspects which shouldn't be necessary for IOMMU API use anyway.
Signed-off-by Yong Wu <[email protected]>
Signed-off-by Robin Murphy <[email protected]>
This looks good to me.
And actually our disp/vdec/venc works well on your v2.
You could choose the two below,
Reviewed-by Yong Wu <[email protected]>
Tested-by Yong Wu <[email protected]>

Quoted 8 lines

Quoted 4 lines
Great, thanks!
Will, it seems to make the most sense for Joerg to pick these patches up directly with the MTK series that depends on them - any objections?
As discussed with Joerg, I'll pick these up and send him a pull request containing all the pgtable bits. I've taken the liberty of updating MAINTAINERS too, assuming you don't mind being a reviewer for this?

Sure, that's fine by me.

Quoted 9 lines

Will
--->8
From bccb08d096703bd790fcf2f934553dd9c8503fc9 Mon Sep 17 00:00:00 2001
From: Will Deacon <[email protected]>
Date: Wed, 17 Feb 2016 14:18:00 +0000
Subject: [PATCH] MAINTAINERS: update ARM SMMU entry

Quoted 4 lines

Ensure that the short-descriptor page table code is included under the SMMU entry, and add Robin as a designated reviewer.
Cc Robin Murphy <[email protected]>

s/Cc/Acked-by/

Robin.

Quoted 24 lines

Signed-off-by Will Deacon <[email protected]>

---
  MAINTAINERS | 2 ++
  1 file changed, 2 insertions(+)

MAINTAINERS +2 -0

index cc2f753cb357..a01bae148b89 100644
@@ -1793,11 +1793,13 @@ F:	drivers/edac/synopsys_edac.c
 
  ARM SMMU DRIVERS
  M:	Will Deacon <[email protected]>
+R:	Robin Murphy <[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/arm-smmu-v3.c
  F:	drivers/iommu/io-pgtable-arm.c
+F:	drivers/iommu/io-pgtable-arm-v7s.c
 
  ARM64 PORT (AARCH64 ARCHITECTURE)
  M:	Catalin Marinas <[email protected]>

Will Deacon

2016-02-10 15:48 UTC #6 lore raw

PATCH v3 1/3 iommu/io-pgtable: Add ARMv7 short descriptor support

In reply to Robin Murphy

Hi Robin,

Quoted 6 lines · Robin Murphy

Add a nearly-complete ARMv7 short descriptor implementation, omitting only a few legacy and CPU-centric aspects which shouldn't be necessary for IOMMU API use anyway.
Signed-off-by Yong Wu <[email protected]>
Signed-off-by Robin Murphy <[email protected]>

Quoted 8 lines

---
 drivers/iommu/Kconfig              |  19 +
 drivers/iommu/Makefile             |   1 +
 drivers/iommu/io-pgtable-arm-v7s.c | 849 +++++++++++++++++++++++++++++++++++++
 drivers/iommu/io-pgtable.c         |   3 +
 drivers/iommu/io-pgtable.h         |  14 +-
 5 files changed, 885 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iommu/io-pgtable-arm-v7s.c

This looks good to me, with one minor comment below.

Quoted 20 lines

drivers/iommu/io-pgtable.h +2 -0

index 36673c8..aa57073 100644
@@ -1,5 +1,6 @@
 #ifndef __IO_PGTABLE_H
 #define __IO_PGTABLE_H
+#include <linux/bitops.h>
 
 /*
  * Public API for use by IOMMU drivers

@@ -9,6 +10,7 @@ enum io_pgtable_fmt {
 	ARM_32_LPAE_S2,
 	ARM_64_LPAE_S1,
 	ARM_64_LPAE_S2,
+	ARM_V7S,
 	IO_PGTABLE_NUM_FMTS,
 };

drivers/iommu/io-pgtable.h struct iommu_gather_ops { · line 52

Quoted 7 lines

drivers/iommu/io-pgtable.h +3 -1

@@ -45,7 +47,9 @@ struct iommu_gather_ops {
  *                 page table walker.
  */
 struct io_pgtable_cfg {
-	#define IO_PGTABLE_QUIRK_ARM_NS	(1 << 0)	/* Set NS bit in PTEs */
+	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0) /* Set NS bit in PTEs */
+	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1) /* No AP/XN bits */
+	#define IO_PGTABLE_QUIRK_TLBI_ON_MAP	BIT(2) /* TLB Inv. on map */

We should either try to keep these generic or add _ARM_ in the names. I don't see why we couldn't have them as generic quicks, but that means you should fix the AP/XN comment to be architecture agnostic and also hack the lpae iopgtable alloc code to reject a cfg with unsupported quirks.

With that change, I'm happy to queue this via my tree or whatever is easiest for Yong/Joerg.

Will

Robin Murphy

author reviewer

2016-02-11 16:13 UTC #7 lore raw

[PATCH 4/3] iommu/io-pgtable: Rationalise quirk handling

As the number of io-pgtable implementations grows beyond 1, it's time to rationalise the quirks mechanism before things have a chance to start getting really ugly and out-of-hand.

To that end:
- Indicate exactly which quirks each format can/does support.
- Fail creating a table if a caller wants unsupported quirks.
- Properly document where each quirk applies and why.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/io-pgtable-arm-v7s.c |  3 +++
 drivers/iommu/io-pgtable-arm.c     |  2 ++
 drivers/iommu/io-pgtable.c         |  3 +++
 drivers/iommu/io-pgtable.h         | 24 ++++++++++++++++++++----
 4 files changed, 28 insertions(+), 4 deletions(-)

drivers/iommu/io-pgtable-arm-v7s.c +3 -0

index d39a021..9bc607a 100644
@@ -690,6 +690,9 @@ out_free_data:
 struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = {
 	.alloc	= arm_v7s_alloc_pgtable,
 	.free	= arm_v7s_free_pgtable,
+	.supported_quirks = IO_PGTABLE_QUIRK_ARM_NS |
+			    IO_PGTABLE_QUIRK_NO_PERMS |
+			    IO_PGTABLE_QUIRK_TLBI_ON_MAP,
 };
 
 #ifdef CONFIG_IOMMU_IO_PGTABLE_ARMV7S_SELFTEST

drivers/iommu/io-pgtable-arm.c +2 -0

index 4095af2..32f94f1 100644
@@ -863,6 +863,7 @@ arm_32_lpae_alloc_pgtable_s2(struct io_pgtable_cfg *cfg, void *cookie)
 struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns = {
 	.alloc	= arm_64_lpae_alloc_pgtable_s1,
 	.free	= arm_lpae_free_pgtable,
+	.supported_quirks = IO_PGTABLE_QUIRK_ARM_NS,
 };
 
 struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns = {

@@ -873,6 +874,7 @@ struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns = {
 struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns = {
 	.alloc	= arm_32_lpae_alloc_pgtable_s1,
 	.free	= arm_lpae_free_pgtable,
+	.supported_quirks = IO_PGTABLE_QUIRK_ARM_NS,
 };
 
 struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns = {

drivers/iommu/io-pgtable.c +3 -0

index 876f6a7..0f57a45 100644
@@ -52,6 +52,9 @@ struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt,
 	if (!fns)
 		return NULL;
 
+	if (cfg->quirks & ~fns->supported_quirks)
+		return NULL;
+
 	iop = fns->alloc(cfg, cookie);
 	if (!iop)
 		return NULL;

drivers/iommu/io-pgtable.h +20 -4

index 4faee7d..6e7f11e 100644
@@ -47,10 +47,24 @@ struct iommu_gather_ops {
  *                 page table walker.
  */
 struct io_pgtable_cfg {
-	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0) /* Set NS bit in PTEs */
-	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1) /* No AP/XN bits */
-	#define IO_PGTABLE_QUIRK_TLBI_ON_MAP	BIT(2) /* TLB Inv. on map */
-	int				quirks;
+	/*
+	 * IO_PGTABLE_QUIRK_ARM_NS: (ARM formats) Set NS and NSTABLE bits in
+	 *	stage 1 PTEs, for hardware which insists on validating them
+	 *	even in	non-secure state where they should normally be ignored.
+	 *
+	 * IO_PGTABLE_QUIRK_NO_PERMS: Ignore the IOMMU_READ, IOMMU_WRITE and
+	 *	IOMMU_NOEXEC flags and map everything with full access, for
+	 *	hardware which does not implement the permissions of a given
+	 *	format, and/or requires some format-specific default value.
+	 *
+	 * IO_PGTABLE_QUIRK_TLBI_ON_MAP: If the format forbids caching invalid
+	 *	(unmapped) entries but the hardware might do so anyway, perform
+	 *	TLB maintenance when mapping as well as when unmapping.
+	 */
+	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0)
+	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1)
+	#define IO_PGTABLE_QUIRK_TLBI_ON_MAP	BIT(2)
+	unsigned int			quirks;
 	unsigned long			pgsize_bitmap;
 	unsigned int			ias;
 	unsigned int			oas;

@@ -173,10 +187,12 @@ static inline void io_pgtable_tlb_sync(struct io_pgtable *iop)
  *
  * @alloc: Allocate a set of page tables described by cfg.
  * @free:  Free the page tables associated with iop.
+ * @supported_quirks: Bitmap of quirks supported by the format.
  */
 struct io_pgtable_init_fns {
 	struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void *cookie);
 	void (*free)(struct io_pgtable *iop);
+	unsigned int supported_quirks;
 };
 
 extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns;

Signature · 1 line

2.7.0.25.gfc10eb5.dirty

Laurent Pinchart

Reviewed-by

2016-02-11 20:39 UTC #8 lore raw

[PATCH 4/3] iommu/io-pgtable: Rationalise quirk handling

Hi Robin,

Thank you for the patch.

Quoted 10 lines · On Thursday 11 February 2016 16:13:45 Robin Murphy wrote:

As the number of io-pgtable implementations grows beyond 1, it's time to rationalise the quirks mechanism before things have a chance to start getting really ugly and out-of-hand.
To that end:
- Indicate exactly which quirks each format can/does support.
- Fail creating a table if a caller wants unsupported quirks.
- Properly document where each quirk applies and why.
Signed-off-by Robin Murphy <[email protected]>

Reviewed-by Laurent Pinchart <[email protected]>

Quoted 102 lines

---
 drivers/iommu/io-pgtable-arm-v7s.c |  3 +++
 drivers/iommu/io-pgtable-arm.c     |  2 ++
 drivers/iommu/io-pgtable.c         |  3 +++
 drivers/iommu/io-pgtable.h         | 24 ++++++++++++++++++++----
 4 files changed, 28 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/iommu/io-pgtable-arm-v7s.c index d39a021..9bc607a 100644

drivers/iommu/io-pgtable-arm-v7s.c +3 -0

@@ -690,6 +690,9 @@ out_free_data:
 struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = {
 	.alloc	= arm_v7s_alloc_pgtable,
 	.free	= arm_v7s_free_pgtable,
+	.supported_quirks = IO_PGTABLE_QUIRK_ARM_NS |
+			    IO_PGTABLE_QUIRK_NO_PERMS |
+			    IO_PGTABLE_QUIRK_TLBI_ON_MAP,
 };
 
 #ifdef CONFIG_IOMMU_IO_PGTABLE_ARMV7S_SELFTEST

drivers/iommu/io-pgtable-arm.c +0 -0

index 4095af2..32f94f1 100644
@@ -863,6 +863,7 @@ arm_32_lpae_alloc_pgtable_s2(struct io_pgtable_cfg *cfg,

void *cookie) struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns
= { .alloc = arm_64_lpae_alloc_pgtable_s1,

drivers/iommu/io-pgtable-arm.c +1 -0

 	.free	= arm_lpae_free_pgtable,
+	.supported_quirks = IO_PGTABLE_QUIRK_ARM_NS,
 };
 
 struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns = {

@@ -873,6 +874,7 @@ struct io_pgtable_init_fns

io_pgtable_arm_64_lpae_s2_init_fns = { struct io_pgtable_init_fns
io_pgtable_arm_32_lpae_s1_init_fns = { .alloc =
arm_32_lpae_alloc_pgtable_s1,

drivers/iommu/io-pgtable-arm.c +1 -0

 	.free	= arm_lpae_free_pgtable,
+	.supported_quirks = IO_PGTABLE_QUIRK_ARM_NS,
 };
 
 struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns = {

drivers/iommu/io-pgtable.c +3 -0

index 876f6a7..0f57a45 100644
@@ -52,6 +52,9 @@ struct io_pgtable_ops *alloc_io_pgtable_ops(enum
 io_pgtable_fmt fmt, if (!fns)
 		return NULL;
 
+	if (cfg->quirks & ~fns->supported_quirks)
+		return NULL;
+
 	iop = fns->alloc(cfg, cookie);
 	if (!iop)
 		return NULL;

drivers/iommu/io-pgtable.h +18 -4

index 4faee7d..6e7f11e 100644
@@ -47,10 +47,24 @@ struct iommu_gather_ops {
  *                 page table walker.
  */
 struct io_pgtable_cfg {
-	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0) /* Set NS bit in PTEs */
-	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1) /* No AP/XN bits */
-	#define IO_PGTABLE_QUIRK_TLBI_ON_MAP	BIT(2) /* TLB Inv. on map */
-	int				quirks;
+	/*
+	 * IO_PGTABLE_QUIRK_ARM_NS: (ARM formats) Set NS and NSTABLE bits in
+	 *	stage 1 PTEs, for hardware which insists on validating them
+	 *	even in	non-secure state where they should normally be ignored.
+	 *
+	 * IO_PGTABLE_QUIRK_NO_PERMS: Ignore the IOMMU_READ, IOMMU_WRITE and
+	 *	IOMMU_NOEXEC flags and map everything with full access, for
+	 *	hardware which does not implement the permissions of a given
+	 *	format, and/or requires some format-specific default value.
+	 *
+	 * IO_PGTABLE_QUIRK_TLBI_ON_MAP: If the format forbids caching invalid
+	 *	(unmapped) entries but the hardware might do so anyway, perform
+	 *	TLB maintenance when mapping as well as when unmapping.
+	 */
+	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0)
+	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1)
+	#define IO_PGTABLE_QUIRK_TLBI_ON_MAP	BIT(2)
+	unsigned int			quirks;
 	unsigned long			pgsize_bitmap;
 	unsigned int			ias;
 	unsigned int			oas;

@@ -173,10 +187,12 @@ static inline void io_pgtable_tlb_sync(struct

io_pgtable *iop) *
* @alloc: Allocate a set of page tables described by cfg.
* @free: Free the page tables associated with iop.
+ * @supported_quirks: Bitmap of quirks supported by the format. */
struct io_pgtable_init_fns {

drivers/iommu/io-pgtable.h +1 -0

 	struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void *cookie);
 	void (*free)(struct io_pgtable *iop);
+	unsigned int supported_quirks;
 };
 
 extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns;

Signature · 3 lines

Regards,

Laurent Pinchart

Will Deacon

2016-02-12 12:08 UTC #9 lore raw

[PATCH 4/3] iommu/io-pgtable: Rationalise quirk handling

In reply to Robin Murphy

Quoted 9 lines · Robin Murphy

As the number of io-pgtable implementations grows beyond 1, it's time to rationalise the quirks mechanism before things have a chance to start getting really ugly and out-of-hand.
To that end:
- Indicate exactly which quirks each format can/does support.
- Fail creating a table if a caller wants unsupported quirks.
- Properly document where each quirk applies and why.

Quoted 7 lines

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/io-pgtable-arm-v7s.c |  3 +++
 drivers/iommu/io-pgtable-arm.c     |  2 ++
 drivers/iommu/io-pgtable.c         |  3 +++
 drivers/iommu/io-pgtable.h         | 24 ++++++++++++++++++++----
 4 files changed, 28 insertions(+), 4 deletions(-)

Note that I can't merge this until we've figured out the plan for Yong's driver.

Quoted 64 lines

drivers/iommu/io-pgtable-arm-v7s.c +3 -0

index d39a021..9bc607a 100644
@@ -690,6 +690,9 @@ out_free_data:
 struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = {
 	.alloc	= arm_v7s_alloc_pgtable,
 	.free	= arm_v7s_free_pgtable,
+	.supported_quirks = IO_PGTABLE_QUIRK_ARM_NS |
+			    IO_PGTABLE_QUIRK_NO_PERMS |
+			    IO_PGTABLE_QUIRK_TLBI_ON_MAP,
 };
 
 #ifdef CONFIG_IOMMU_IO_PGTABLE_ARMV7S_SELFTEST

drivers/iommu/io-pgtable-arm.c +2 -0

index 4095af2..32f94f1 100644
@@ -863,6 +863,7 @@ arm_32_lpae_alloc_pgtable_s2(struct io_pgtable_cfg *cfg, void *cookie)
 struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns = {
 	.alloc	= arm_64_lpae_alloc_pgtable_s1,
 	.free	= arm_lpae_free_pgtable,
+	.supported_quirks = IO_PGTABLE_QUIRK_ARM_NS,
 };
 
 struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns = {

@@ -873,6 +874,7 @@ struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns = {
 struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns = {
 	.alloc	= arm_32_lpae_alloc_pgtable_s1,
 	.free	= arm_lpae_free_pgtable,
+	.supported_quirks = IO_PGTABLE_QUIRK_ARM_NS,
 };
 
 struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns = {

drivers/iommu/io-pgtable.c +3 -0

index 876f6a7..0f57a45 100644
@@ -52,6 +52,9 @@ struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt,
 	if (!fns)
 		return NULL;
 
+	if (cfg->quirks & ~fns->supported_quirks)
+		return NULL;
+
 	iop = fns->alloc(cfg, cookie);
 	if (!iop)
 		return NULL;

drivers/iommu/io-pgtable.h +4 -4

index 4faee7d..6e7f11e 100644
@@ -47,10 +47,24 @@ struct iommu_gather_ops {
  *                 page table walker.
  */
 struct io_pgtable_cfg {
-	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0) /* Set NS bit in PTEs */
-	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1) /* No AP/XN bits */
-	#define IO_PGTABLE_QUIRK_TLBI_ON_MAP	BIT(2) /* TLB Inv. on map */
-	int				quirks;
+	/*
+	 * IO_PGTABLE_QUIRK_ARM_NS: (ARM formats) Set NS and NSTABLE bits in
+	 *	stage 1 PTEs, for hardware which insists on validating them
+	 *	even in	non-secure state where they should normally be ignored.

drivers/iommu/io-pgtable.h struct iommu_gather_ops { · line 63

Quoted 10 lines

drivers/iommu/io-pgtable.h +10 -0

+	 *
+	 * IO_PGTABLE_QUIRK_NO_PERMS: Ignore the IOMMU_READ, IOMMU_WRITE and
+	 *	IOMMU_NOEXEC flags and map everything with full access, for
+	 *	hardware which does not implement the permissions of a given
+	 *	format, and/or requires some format-specific default value.
+	 *
+	 * IO_PGTABLE_QUIRK_TLBI_ON_MAP: If the format forbids caching invalid
+	 *	(unmapped) entries but the hardware might do so anyway, perform
+	 *	TLB maintenance when mapping as well as when unmapping.
+	 */

Much better, cheers.

drivers/iommu/io-pgtable.h struct iommu_gather_ops { · line 67

Quoted 4 lines

drivers/iommu/io-pgtable.h +4 -0

+	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0)
+	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1)
+	#define IO_PGTABLE_QUIRK_TLBI_ON_MAP	BIT(2)
+	unsigned int			quirks;

Let's just make this unsigned long.

Quoted 4 lines

drivers/iommu/io-pgtable.h +0 -0

 	unsigned long			pgsize_bitmap;
 	unsigned int			ias;
 	unsigned int			oas;

drivers/iommu/io-pgtable.h static inline void io_pgtable_tlb_sync(struct io_pgtable *iop) · line 196

Quoted 10 lines

drivers/iommu/io-pgtable.h +2 -0

@@ -173,10 +187,12 @@ static inline void io_pgtable_tlb_sync(struct io_pgtable *iop)
  *
  * @alloc: Allocate a set of page tables described by cfg.
  * @free:  Free the page tables associated with iop.
+ * @supported_quirks: Bitmap of quirks supported by the format.
  */
 struct io_pgtable_init_fns {
 	struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void *cookie);
 	void (*free)(struct io_pgtable *iop);
+	unsigned int supported_quirks;
 };

My only gripe here is that this is supposed to be a struct of function pointers... get_supported_quirks() ?

Will

Laurent Pinchart

2016-02-12 12:32 UTC #10 lore raw

[PATCH 4/3] iommu/io-pgtable: Rationalise quirk handling

Quoted 101 lines · On Friday 12 February 2016 12:08:58 Will Deacon wrote:

Quoted 9 lines · Robin Murphy
As the number of io-pgtable implementations grows beyond 1, it's time to rationalise the quirks mechanism before things have a chance to start getting really ugly and out-of-hand.
To that end:
- Indicate exactly which quirks each format can/does support.
- Fail creating a table if a caller wants unsupported quirks.
- Properly document where each quirk applies and why.
Quoted 8 lines
Signed-off-by Robin Murphy <[email protected]>
---
 drivers/iommu/io-pgtable-arm-v7s.c |  3 +++
 drivers/iommu/io-pgtable-arm.c     |  2 ++
 drivers/iommu/io-pgtable.c         |  3 +++
 drivers/iommu/io-pgtable.h         | 24 ++++++++++++++++++++----
 4 files changed, 28 insertions(+), 4 deletions(-)
Note that I can't merge this until we've figured out the plan for Yong's driver.
Quoted 79 lines
(quoted diff) +0 -0
b/drivers/iommu/io-pgtable-arm-v7s.c index d39a021..9bc607a 100644
drivers/iommu/io-pgtable-arm-v7s.c +3 -0
@@ -690,6 +690,9 @@ out_free_data:
 struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = {
 
 	.alloc	= arm_v7s_alloc_pgtable,
 	.free	= arm_v7s_free_pgtable,
 
+	.supported_quirks = IO_PGTABLE_QUIRK_ARM_NS |
+			    IO_PGTABLE_QUIRK_NO_PERMS |
+			    IO_PGTABLE_QUIRK_TLBI_ON_MAP,
 
};
#ifdef CONFIG_IOMMU_IO_PGTABLE_ARMV7S_SELFTEST
(quoted diff) +0 -0
b/drivers/iommu/io-pgtable-arm.c index 4095af2..32f94f1 100644
drivers/iommu/io-pgtable-arm.c +1 -0
@@ -863,6 +863,7 @@ arm_32_lpae_alloc_pgtable_s2(struct io_pgtable_cfg
 *cfg, void *cookie)> 
 struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns = {
 
 	.alloc	= arm_64_lpae_alloc_pgtable_s1,
 	.free	= arm_lpae_free_pgtable,
 
+	.supported_quirks = IO_PGTABLE_QUIRK_ARM_NS,
};
struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns = {
drivers/iommu/io-pgtable-arm.c +0 -0
@@ -873,6 +874,7 @@ struct io_pgtable_init_fns
io_pgtable_arm_64_lpae_s2_init_fns = {>
struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns = {
drivers/iommu/io-pgtable-arm.c +1 -0
 	.alloc	= arm_32_lpae_alloc_pgtable_s1,
 	.free	= arm_lpae_free_pgtable,
 
+	.supported_quirks = IO_PGTABLE_QUIRK_ARM_NS,
 
 };
 
 struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns = {
drivers/iommu/io-pgtable.c +0 -0
index 876f6a7..0f57a45 100644
@@ -52,6 +52,9 @@ struct io_pgtable_ops *alloc_io_pgtable_ops(enum
io_pgtable_fmt fmt,>
drivers/iommu/io-pgtable.c +3 -0
 	if (!fns)
 
 		return NULL;
 
+	if (cfg->quirks & ~fns->supported_quirks)
+		return NULL;
+
 
 	iop = fns->alloc(cfg, cookie);
 	if (!iop)
 
 		return NULL;
drivers/iommu/io-pgtable.h +0 -0
index 4faee7d..6e7f11e 100644
@@ -47,10 +47,24 @@ struct iommu_gather_ops {
 
  *                 page table walker.
  */
 
 struct io_pgtable_cfg {
 

Quoted 15 lines

Quoted 5 lines

drivers/iommu/io-pgtable.h +1 -4

-	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0) /* Set NS bit in PTEs */
-	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1) /* No AP/XN bits */
-	#define IO_PGTABLE_QUIRK_TLBI_ON_MAP	BIT(2) /* TLB Inv. on map */
-	int				quirks;
+	/*

drivers/iommu/io-pgtable.h struct iommu_gather_ops {

Quoted 10 lines

drivers/iommu/io-pgtable.h +10 -0

+	 * IO_PGTABLE_QUIRK_ARM_NS: (ARM formats) Set NS and NSTABLE bits in
+	 *	stage 1 PTEs, for hardware which insists on validating them
+	 *	even in	non-secure state where they should normally be ignored.
+	 *
+	 * IO_PGTABLE_QUIRK_NO_PERMS: Ignore the IOMMU_READ, IOMMU_WRITE and
+	 *	IOMMU_NOEXEC flags and map everything with full access, for
+	 *	hardware which does not implement the permissions of a given
+	 *	format, and/or requires some format-specific default value.
+	 *
+	 * IO_PGTABLE_QUIRK_TLBI_ON_MAP: If the format forbids caching

invalid

Quoted 17 lines

drivers/iommu/io-pgtable.h struct iommu_gather_ops { · line 63
Quoted 3 lines
drivers/iommu/io-pgtable.h +3 -0
+	 *	(unmapped) entries but the hardware might do so anyway, perform
+	 *	TLB maintenance when mapping as well as when unmapping.
+	 */
Much better, cheers.
drivers/iommu/io-pgtable.h struct iommu_gather_ops { · line 67
Quoted 4 lines
drivers/iommu/io-pgtable.h +4 -0
+	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0)
+	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1)
+	#define IO_PGTABLE_QUIRK_TLBI_ON_MAP	BIT(2)
+	unsigned int			quirks;
Let's just make this unsigned long.
Quoted 4 lines
drivers/iommu/io-pgtable.h +0 -0
 	unsigned long			pgsize_bitmap;
 	unsigned int			ias;
 	unsigned int			oas;

Quoted 9 lines

drivers/iommu/io-pgtable.h static inline void io_pgtable_tlb_sync(struct · line 194

Quoted 9 lines

drivers/iommu/io-pgtable.h +1 -0

@@ -173,10 +187,12 @@ static inline void io_pgtable_tlb_sync(struct
 io_pgtable *iop)
  *
  * @alloc: Allocate a set of page tables described by cfg.
  * @free:  Free the page tables associated with iop.
+ * @supported_quirks: Bitmap of quirks supported by the format.
  */
 struct io_pgtable_init_fns {
 	struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void

*cookie);

Quoted 6 lines

drivers/iommu/io-pgtable.h static inline void io_pgtable_tlb_sync(struct io_pgtable *iop) · line 196
Quoted 3 lines
drivers/iommu/io-pgtable.h +1 -0
 	void (*free)(struct io_pgtable *iop);
+	unsigned int supported_quirks;
 };
My only gripe here is that this is supposed to be a struct of function pointers... get_supported_quirks() ?

Can't we instead say it's a struct of function pointers and static data ? :-) A function to retrieve the supported quirks will just contribute to kernel bloat without adding any extra benefit.

Signature · 3 lines

Regards,

Laurent Pinchart

Robin Murphy

author reviewer

2016-02-12 15:23 UTC #11 lore raw

[PATCH 4/3] iommu/io-pgtable: Rationalise quirk handling

Quoted 14 lines · Laurent Pinchart

Quoted 13 lines · On Friday 12 February 2016 12:08:58 Will Deacon wrote:
Quoted 12 lines · Robin Murphy
As the number of io-pgtable implementations grows beyond 1, it's time to rationalise the quirks mechanism before things have a chance to start getting really ugly and out-of-hand.
To that end:
- Indicate exactly which quirks each format can/does support.
- Fail creating a table if a caller wants unsupported quirks.
- Properly document where each quirk applies and why.
Signed-off-by Robin Murphy <[email protected]>
---

Quoted 8 lines

Quoted 5 lines

  drivers/iommu/io-pgtable-arm-v7s.c |  3 +++
  drivers/iommu/io-pgtable-arm.c     |  2 ++
  drivers/iommu/io-pgtable.c         |  3 +++
  drivers/iommu/io-pgtable.h         | 24 ++++++++++++++++++++----
  4 files changed, 28 insertions(+), 4 deletions(-)

Note that I can't merge this until we've figured out the plan for Yong's driver.

Indeed (hence 4/3) - it just ended up seeming like a significant enough change to warrant its own patch, but not critical enough to warrant splitting it for the sake of applying the parts touching the existing code right now.

Quoted 129 lines

Quoted 79 lines
Quoted 79 lines
(quoted diff) +0 -0
b/drivers/iommu/io-pgtable-arm-v7s.c index d39a021..9bc607a 100644
drivers/iommu/io-pgtable-arm-v7s.c +3 -0
@@ -690,6 +690,9 @@ out_free_data:
  struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = {
 
  	.alloc	= arm_v7s_alloc_pgtable,
  	.free	= arm_v7s_free_pgtable,
 
+	.supported_quirks = IO_PGTABLE_QUIRK_ARM_NS |
+			    IO_PGTABLE_QUIRK_NO_PERMS |
+			    IO_PGTABLE_QUIRK_TLBI_ON_MAP,
 
  };

  #ifdef CONFIG_IOMMU_IO_PGTABLE_ARMV7S_SELFTEST
(quoted diff) +0 -0
b/drivers/iommu/io-pgtable-arm.c index 4095af2..32f94f1 100644
drivers/iommu/io-pgtable-arm.c +1 -0
@@ -863,6 +863,7 @@ arm_32_lpae_alloc_pgtable_s2(struct io_pgtable_cfg
 *cfg, void *cookie)>
  struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns = {
 
  	.alloc	= arm_64_lpae_alloc_pgtable_s1,
  	.free	= arm_lpae_free_pgtable,
 
+	.supported_quirks = IO_PGTABLE_QUIRK_ARM_NS,
  };

  struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns = {
drivers/iommu/io-pgtable-arm.c +0 -0
@@ -873,6 +874,7 @@ struct io_pgtable_init_fns
io_pgtable_arm_64_lpae_s2_init_fns = {>
struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns = {
.alloc = arm_32_lpae_alloc_pgtable_s1,
.free = arm_lpae_free_pgtable,
drivers/iommu/io-pgtable-arm.c +1 -0
+	.supported_quirks = IO_PGTABLE_QUIRK_ARM_NS,
 
  };
 
  struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns = {
drivers/iommu/io-pgtable.c +0 -0
index 876f6a7..0f57a45 100644
@@ -52,6 +52,9 @@ struct io_pgtable_ops *alloc_io_pgtable_ops(enum
io_pgtable_fmt fmt,>
if (!fns)
return NULL;
drivers/iommu/io-pgtable.c +3 -0
+	if (cfg->quirks & ~fns->supported_quirks)
+		return NULL;
+
 
  	iop = fns->alloc(cfg, cookie);
  	if (!iop)
 
  		return NULL;
drivers/iommu/io-pgtable.h +0 -0
index 4faee7d..6e7f11e 100644
@@ -47,10 +47,24 @@ struct iommu_gather_ops {
 
   *                 page table walker.
   */
 
  struct io_pgtable_cfg {
 
Quoted 15 lines
Quoted 5 lines
drivers/iommu/io-pgtable.h +1 -4
-	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0) /* Set NS bit in PTEs */
-	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1) /* No AP/XN bits */
-	#define IO_PGTABLE_QUIRK_TLBI_ON_MAP	BIT(2) /* TLB Inv. on map */
-	int				quirks;
+	/*
drivers/iommu/io-pgtable.h struct iommu_gather_ops {
Quoted 10 lines
drivers/iommu/io-pgtable.h +10 -0
+	 * IO_PGTABLE_QUIRK_ARM_NS: (ARM formats) Set NS and NSTABLE bits in
+	 *	stage 1 PTEs, for hardware which insists on validating them
+	 *	even in	non-secure state where they should normally be ignored.
+	 *
+	 * IO_PGTABLE_QUIRK_NO_PERMS: Ignore the IOMMU_READ, IOMMU_WRITE and
+	 *	IOMMU_NOEXEC flags and map everything with full access, for
+	 *	hardware which does not implement the permissions of a given
+	 *	format, and/or requires some format-specific default value.
+	 *
+	 * IO_PGTABLE_QUIRK_TLBI_ON_MAP: If the format forbids caching
invalid
Quoted 17 lines
drivers/iommu/io-pgtable.h struct iommu_gather_ops { · line 63
Quoted 3 lines
drivers/iommu/io-pgtable.h +3 -0
+	 *	(unmapped) entries but the hardware might do so anyway, perform
+	 *	TLB maintenance when mapping as well as when unmapping.
+	 */
Much better, cheers.
drivers/iommu/io-pgtable.h struct iommu_gather_ops { · line 67
Quoted 4 lines
drivers/iommu/io-pgtable.h +4 -0
+	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0)
+	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1)
+	#define IO_PGTABLE_QUIRK_TLBI_ON_MAP	BIT(2)
+	unsigned int			quirks;
Let's just make this unsigned long.
Quoted 4 lines
unsigned long pgsize_bitmap;
unsigned int ias;
unsigned int oas;
Quoted 9 lines
drivers/iommu/io-pgtable.h static inline void io_pgtable_tlb_sync(struct · line 194
Quoted 9 lines
drivers/iommu/io-pgtable.h +1 -0
@@ -173,10 +187,12 @@ static inline void io_pgtable_tlb_sync(struct
 io_pgtable *iop)
   *
   * @alloc: Allocate a set of page tables described by cfg.
   * @free:  Free the page tables associated with iop.
+ * @supported_quirks: Bitmap of quirks supported by the format.
   */
  struct io_pgtable_init_fns {
  	struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void
*cookie);
Quoted 6 lines
drivers/iommu/io-pgtable.h static inline void io_pgtable_tlb_sync(struct io_pgtable *iop) · line 196
Quoted 3 lines
drivers/iommu/io-pgtable.h +1 -0
  	void (*free)(struct io_pgtable *iop);
+	unsigned int supported_quirks;
  };
My only gripe here is that this is supposed to be a struct of function pointers... get_supported_quirks() ?

Quoted 3 lines

Can't we instead say it's a struct of function pointers and static data ? :-) A function to retrieve the supported quirks will just contribute to kernel bloat without adding any extra benefit.

Bleh - personally I think it's marginally more obvious and readable to have the information looking like data, but I'd much sooner just have a hard-coded "if (cfq->quirks & ..." check in every format's alloc function than introduce a whole new callback. Does that sound like a reasonable compromise?

Robin.

Will Deacon

2016-02-12 15:55 UTC #12 lore raw

[PATCH 4/3] iommu/io-pgtable: Rationalise quirk handling

Quoted 24 lines · Robin Murphy

Quoted 19 lines · Laurent Pinchart
Quoted 10 lines · On Friday 12 February 2016 12:08:58 Will Deacon wrote:
drivers/iommu/io-pgtable.h static inline void io_pgtable_tlb_sync(struct · line 194
Quoted 9 lines · Robin Murphy
drivers/iommu/io-pgtable.h +1 -0
@@ -173,10 +187,12 @@ static inline void io_pgtable_tlb_sync(struct
 io_pgtable *iop)
  *
  * @alloc: Allocate a set of page tables described by cfg.
  * @free:  Free the page tables associated with iop.
+ * @supported_quirks: Bitmap of quirks supported by the format.
  */
 struct io_pgtable_init_fns {
 	struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void
*cookie);
Quoted 6 lines
drivers/iommu/io-pgtable.h static inline void io_pgtable_tlb_sync(struct io_pgtable *iop) · line 196
Quoted 3 lines
drivers/iommu/io-pgtable.h +1 -0
 	void (*free)(struct io_pgtable *iop);
+	unsigned int supported_quirks;
 };
My only gripe here is that this is supposed to be a struct of function pointers... get_supported_quirks() ?
Quoted 3 lines
Can't we instead say it's a struct of function pointers and static data ? :-) A function to retrieve the supported quirks will just contribute to kernel bloat without adding any extra benefit.

Quoted 5 lines

Bleh - personally I think it's marginally more obvious and readable to have the information looking like data, but I'd much sooner just have a hard-coded "if (cfq->quirks & ..." check in every format's alloc function than introduce a whole new callback. Does that sound like a reasonable compromise?

Yeah, do that. You never know, there could be quirks that are only supported based on other things in the cfg, so it's more extensible that way too.

Will

Geert Uytterhoeven

2016-03-01 12:01 UTC #13 lore raw

PATCH v3 1/3 iommu/io-pgtable: Add ARMv7 short descriptor support

In reply to Robin Murphy

Hi Robin,

Quoted 32 lines · Robin Murphy

Add a nearly-complete ARMv7 short descriptor implementation, omitting only a few legacy and CPU-centric aspects which shouldn't be necessary for IOMMU API use anyway.

Signed-off-by Yong Wu <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/Kconfig              |  19 +
 drivers/iommu/Makefile             |   1 +
 drivers/iommu/io-pgtable-arm-v7s.c | 849 +++++++++++++++++++++++++++++++++++++
 drivers/iommu/io-pgtable.c         |   3 +
 drivers/iommu/io-pgtable.h         |  14 +-
 5 files changed, 885 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iommu/io-pgtable-arm-v7s.c

drivers/iommu/Kconfig +9 -0

index a1e75cb..dc1aaa5 100644
@@ -39,6 +39,25 @@ config IOMMU_IO_PGTABLE_LPAE_SELFTEST
 
          If unsure, say N here.
 
+config IOMMU_IO_PGTABLE_ARMV7S
+       bool "ARMv7/v8 Short Descriptor Format"
+       select IOMMU_IO_PGTABLE
+       depends on HAS_DMA && (ARM || ARM64 || COMPILE_TEST)
+       help
+         Enable support for the ARM Short-descriptor pagetable format.
+         This supports 32-bit virtual and physical addresses mapped using
+         2-level tables with 4KB pages/1MB sections, and contiguous entries
+         for 64KB pages/16MB supersections if indicated by the IOMMU driver.

drivers/iommu/Kconfig config IOMMU_IO_PGTABLE_LPAE_SELFTEST · line 60

Quoted 10 lines

drivers/iommu/Kconfig +10 -0

+
+config IOMMU_IO_PGTABLE_ARMV7S_SELFTEST
+       bool "ARMv7s selftests"
+       depends on IOMMU_IO_PGTABLE_ARMV7S
+       help
+         Enable self-tests for ARMv7s page table allocator. This performs
+         a series of page-table consistency checks during boot.
+
+         If unsure, say N here.
+

When enabling IOMMU_IO_PGTABLE_ARMV7S_SELFTEST on r8a7791 (dual Cortex A15), the kernel crashes with:

kernel BUG at mm/slab.c:2536!

Internal error: Oops - BUG: 0 [#1] SMP ARM
Modules linked in:[    1.299311] Modules linked in:

CPU: 1 PID: 1 Comm: swapper/0 Not tainted
4.5.0-rc6-koelsch-05892-ge7e45ad53ab6795e #2270
Hardware name: Generic R8A7791 (Flattened Device Tree)
task: ef422040 ti: ef442000 task.ti: ef442000
PC is at cache_alloc_refill+0x2a0/0x530
LR is at _raw_spin_unlock+0x8/0xc
pc : [<c02c6928>]    lr : [<c067f97c>]    psr: 00000093

sp : ef443d88 ip : d0a7b1f0 fp : 02000020
r10: 00000010 r9 : 00000000 r8 : 02088020
r7 : 00000000 r6 : ef5172c0 r5 : 00000000 r4 : ef5d7f40 r3 : 00004001 r2 : 00000001 r1 : 00020001 r0 : ef5172c0
Flags: nzcv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel
Control: 30c5387d Table: 40003000 DAC: 55555555
Process swapper/0 (pid: 1, stack limit = 0xef442210)

Stack: (0xef443d88 to 0xef444000)

3d80: ef48b4c0 00000000 ef5172d0 c0e38280 00000000 00001000
3da0: 02088020 60000013 ef5d7f40 c0445010 00000000 ef517340 00001000 c02c6630 3dc0: 00000008 00001000 00000000 00000400 00000002 c04444bc 00000000 00000000 3de0: 024000c0 ef517340 00000000 00004000 00000001 00000001 c0e3b000 ef517340 3e00: 00000000 00001000 00000000 ef5f8000 ef517340 c0445010 00000000 0000000f 3e20: 00001000 c0444e1c 00000000 00000000 ef517350 00000000 ef520f15 ef517340 3e40: ef443ea0 ef517390 00000000 00000000 00001000 c0445010 00000000 ef517390 3e60: 00001000 c0445044 00001000 0000000f ef5f8000 ef443ea0 c0e92a00 c0e92a00 3e80: 00000000 00000000 0000000c c0c18ee4 00001000 0000000f 00000001 00000014 3ea0: 00000001 01111000 00000020 00000020 c0e25bc8 00000000 6f5f806a 00000000 3ec0: 00000020 40004000 800b8204 00000000 00000000 00000000 00000000 00000000 3ee0: c0c3ce94 ef520f00 c0c18da4 00000000 c0e05550 c0e05550 c0e3b000 c0c2d848 3f00: 00000000 c0201760 ef5036c0 c0804b44 c0e5fdd0 00000000 c0e0be00 c0e0be7c 3f20: 00000000 60000013 00000000 efffec51 efffec59 c023893c c09dc204 00000004 3f40: 00000004 c09dce98 000000a0 c09dce98 c0e09e78 00000004 c0c2d834 00000004 3f60: c0c2d838 000000a1 c0c3ce94 c0e3b000 c0e3b000 c0c00d4c 00000004 00000004 3f80: 00000000 c0c0058c 00000000 c067a35c 00000000 00000000 00000000 00000000 3fa0: 00000000 c067a364 00000000 c0206b68 00000000 00000000 00000000 00000000 3fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 3fe0: 00000000 00000000 00000000 00000000 00000013 00000000 ef443ff4 00000000 [<c02c6928>] (cache_alloc_refill) from [<c02c6630>] (kmem_cache_alloc+0x7c/0xd4) [<c02c6630>] (kmem_cache_alloc) from [<c04444bc>] (__arm_v7s_alloc_table+0x5c/0x278)
[<c04444bc>] (__arm_v7s_alloc_table) from [<c0444e1c>] (__arm_v7s_map.constprop.6+0x68/0x25c)
[<c0444e1c>] (__arm_v7s_map.constprop.6) from [<c0445044>] (arm_v7s_map+0x34/0xa4)
[<c0445044>] (arm_v7s_map) from [<c0c18ee4>] (arm_v7s_do_selftests+0x140/0x418) [<c0c18ee4>] (arm_v7s_do_selftests) from [<c0201760>] (do_one_initcall+0x100/0x1b4)
[<c0201760>] (do_one_initcall) from [<c0c00d4c>] (kernel_init_freeable+0x120/0x1e8)
[<c0c00d4c>] (kernel_init_freeable) from [<c067a364>] (kernel_init+0x8/0xec) [<c067a364>] (kernel_init) from [<c0206b68>] (ret_from_fork+0x14/0x2c)

Code: 1a000003 e7f001f2 e3130001 0a000000 (e7f001f2)
---[ end trace 190f6f6b84352efd ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b

I don't know if r8a7791 supports v7s. Still, I don't expect a crash. The LPAE selftest runs fine.

On r8a7795 (arm64), both the LPAE and V7S selftests run fine.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Laurent Pinchart

2016-03-01 13:13 UTC #14 lore raw

PATCH v3 1/3 iommu/io-pgtable: Add ARMv7 short descriptor support

Hi Geert,

Quoted 105 lines · On Tuesday 01 March 2016 13:01:39 Geert Uytterhoeven wrote:

Quoted 34 lines · Robin Murphy
Add a nearly-complete ARMv7 short descriptor implementation, omitting only a few legacy and CPU-centric aspects which shouldn't be necessary for IOMMU API use anyway.
Signed-off-by Yong Wu <[email protected]>
Signed-off-by Robin Murphy <[email protected]>
---
 drivers/iommu/Kconfig              |  19 +
 drivers/iommu/Makefile             |   1 +
 drivers/iommu/io-pgtable-arm-v7s.c | 849 ++++++++++++++++++++++++++++++++
 drivers/iommu/io-pgtable.c         |   3 +
 drivers/iommu/io-pgtable.h         |  14 +-
 5 files changed, 885 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iommu/io-pgtable-arm-v7s.c
drivers/iommu/Kconfig +8 -0
index a1e75cb..dc1aaa5 100644
@@ -39,6 +39,25 @@ config IOMMU_IO_PGTABLE_LPAE_SELFTEST
 
          If unsure, say N here.
 
+config IOMMU_IO_PGTABLE_ARMV7S
+       bool "ARMv7/v8 Short Descriptor Format"
+       select IOMMU_IO_PGTABLE
+       depends on HAS_DMA && (ARM || ARM64 || COMPILE_TEST)
+       help
+         Enable support for the ARM Short-descriptor pagetable format.
+         This supports 32-bit virtual and physical addresses mapped using
+         2-level tables with 4KB pages/1MB sections, and contiguous
entries + for 64KB pages/16MB supersections if indicated by the
IOMMU driver. +
drivers/iommu/Kconfig config IOMMU_IO_PGTABLE_LPAE_SELFTEST · line 60
Quoted 9 lines
drivers/iommu/Kconfig +9 -0
+config IOMMU_IO_PGTABLE_ARMV7S_SELFTEST
+       bool "ARMv7s selftests"
+       depends on IOMMU_IO_PGTABLE_ARMV7S
+       help
+         Enable self-tests for ARMv7s page table allocator. This performs
+         a series of page-table consistency checks during boot.
+
+         If unsure, say N here.
+
When enabling IOMMU_IO_PGTABLE_ARMV7S_SELFTEST on r8a7791 (dual Cortex A15), the kernel crashes with:
kernel BUG at mm/slab.c:2536!
Internal error: Oops - BUG: 0 [#1] SMP ARM
Modules linked in:[    1.299311] Modules linked in:

CPU: 1 PID: 1 Comm: swapper/0 Not tainted
4.5.0-rc6-koelsch-05892-ge7e45ad53ab6795e #2270
Hardware name: Generic R8A7791 (Flattened Device Tree)
task: ef422040 ti: ef442000 task.ti: ef442000
PC is at cache_alloc_refill+0x2a0/0x530
LR is at _raw_spin_unlock+0x8/0xc
pc : [<c02c6928>]    lr : [<c067f97c>]    psr: 00000093
sp : ef443d88 ip : d0a7b1f0 fp : 02000020
r10: 00000010 r9 : 00000000 r8 : 02088020
r7 : 00000000 r6 : ef5172c0 r5 : 00000000 r4 : ef5d7f40 r3 : 00004001 r2 : 00000001 r1 : 00020001 r0 : ef5172c0
Flags: nzcv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel
Control: 30c5387d Table: 40003000 DAC: 55555555
Process swapper/0 (pid: 1, stack limit = 0xef442210)
Stack: (0xef443d88 to 0xef444000)
3d80: ef48b4c0 00000000 ef5172d0 c0e38280 00000000
00001000 3da0: 02088020 60000013 ef5d7f40 c0445010 00000000 ef517340 00001000 c02c6630 3dc0: 00000008 00001000 00000000 00000400 00000002 c04444bc 00000000 00000000 3de0: 024000c0 ef517340 00000000 00004000 00000001 00000001 c0e3b000 ef517340 3e00: 00000000 00001000 00000000 ef5f8000 ef517340 c0445010 00000000 0000000f 3e20: 00001000 c0444e1c 00000000 00000000 ef517350 00000000 ef520f15 ef517340 3e40: ef443ea0 ef517390 00000000 00000000 00001000 c0445010 00000000 ef517390 3e60:
00001000 c0445044 00001000 0000000f ef5f8000 ef443ea0 c0e92a00 c0e92a00 3e80: 00000000 00000000 0000000c c0c18ee4 00001000 0000000f 00000001 00000014 3ea0: 00000001 01111000 00000020 00000020 c0e25bc8 00000000 6f5f806a 00000000 3ec0: 00000020 40004000 800b8204 00000000 00000000 00000000 00000000 00000000 3ee0: c0c3ce94 ef520f00 c0c18da4 00000000 c0e05550 c0e05550 c0e3b000 c0c2d848 3f00: 00000000 c0201760 ef5036c0 c0804b44 c0e5fdd0 00000000 c0e0be00 c0e0be7c 3f20: 00000000 60000013 00000000 efffec51 efffec59 c023893c c09dc204 00000004 3f40: 00000004 c09dce98 000000a0 c09dce98 c0e09e78 00000004 c0c2d834 00000004 3f60:
c0c2d838 000000a1 c0c3ce94 c0e3b000 c0e3b000 c0c00d4c 00000004 00000004 3f80: 00000000 c0c0058c 00000000 c067a35c 00000000 00000000 00000000 00000000 3fa0: 00000000 c067a364 00000000 c0206b68 00000000 00000000 00000000 00000000 3fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 3fe0: 00000000 00000000 00000000 00000000 00000013 00000000 ef443ff4 00000000 [<c02c6928>] (cache_alloc_refill) from [<c02c6630>] (kmem_cache_alloc+0x7c/0xd4) [<c02c6630>] (kmem_cache_alloc) from [<c04444bc>]
(__arm_v7s_alloc_table+0x5c/0x278)
[<c04444bc>] (__arm_v7s_alloc_table) from [<c0444e1c>] (__arm_v7s_map.constprop.6+0x68/0x25c)
[<c0444e1c>] (__arm_v7s_map.constprop.6) from [<c0445044>] (arm_v7s_map+0x34/0xa4)
[<c0445044>] (arm_v7s_map) from [<c0c18ee4>] (arm_v7s_do_selftests+0x140/0x418) [<c0c18ee4>] (arm_v7s_do_selftests) from [<c0201760>]
(do_one_initcall+0x100/0x1b4)
[<c0201760>] (do_one_initcall) from [<c0c00d4c>] (kernel_init_freeable+0x120/0x1e8)
[<c0c00d4c>] (kernel_init_freeable) from [<c067a364>] (kernel_init+0x8/0xec) [<c067a364>] (kernel_init) from [<c0206b68>] (ret_from_fork+0x14/0x2c)

Quoted 5 lines

Code: 1a000003 e7f001f2 e3130001 0a000000 (e7f001f2)
---[ end trace 190f6f6b84352efd ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b

I don't know if r8a7791 supports v7s.

The IPMMU does, but that's not implemented in the driver at the moment.

Quoted 1 line

Still, I don't expect a crash.

I agree.

Quoted 3 lines

The LPAE selftest runs fine.
On r8a7795 (arm64), both the LPAE and V7S selftests run fine.

Signature · 3 lines

Regards,

Laurent Pinchart

Robin Murphy

author reviewer

2016-03-01 13:51 UTC #15 lore raw

PATCH v3 1/3 iommu/io-pgtable: Add ARMv7 short descriptor support

In reply to Geert Uytterhoeven

Hi Geert,

Quoted 102 lines · Geert Uytterhoeven

Hi Robin,
Quoted 32 lines · Robin Murphy
Add a nearly-complete ARMv7 short descriptor implementation, omitting only a few legacy and CPU-centric aspects which shouldn't be necessary for IOMMU API use anyway.
Signed-off-by Yong Wu <[email protected]>
Signed-off-by Robin Murphy <[email protected]>
---
  drivers/iommu/Kconfig              |  19 +
  drivers/iommu/Makefile             |   1 +
  drivers/iommu/io-pgtable-arm-v7s.c | 849 +++++++++++++++++++++++++++++++++++++
  drivers/iommu/io-pgtable.c         |   3 +
  drivers/iommu/io-pgtable.h         |  14 +-
  5 files changed, 885 insertions(+), 1 deletion(-)
create mode 100644 drivers/iommu/io-pgtable-arm-v7s.c
drivers/iommu/Kconfig +9 -0
index a1e75cb..dc1aaa5 100644
@@ -39,6 +39,25 @@ config IOMMU_IO_PGTABLE_LPAE_SELFTEST
 
           If unsure, say N here.
 
+config IOMMU_IO_PGTABLE_ARMV7S
+       bool "ARMv7/v8 Short Descriptor Format"
+       select IOMMU_IO_PGTABLE
+       depends on HAS_DMA && (ARM || ARM64 || COMPILE_TEST)
+       help
+         Enable support for the ARM Short-descriptor pagetable format.
+         This supports 32-bit virtual and physical addresses mapped using
+         2-level tables with 4KB pages/1MB sections, and contiguous entries
+         for 64KB pages/16MB supersections if indicated by the IOMMU driver.
drivers/iommu/Kconfig config IOMMU_IO_PGTABLE_LPAE_SELFTEST · line 60
Quoted 10 lines
drivers/iommu/Kconfig +10 -0
+
+config IOMMU_IO_PGTABLE_ARMV7S_SELFTEST
+       bool "ARMv7s selftests"
+       depends on IOMMU_IO_PGTABLE_ARMV7S
+       help
+         Enable self-tests for ARMv7s page table allocator. This performs
+         a series of page-table consistency checks during boot.
+
+         If unsure, say N here.
+
When enabling IOMMU_IO_PGTABLE_ARMV7S_SELFTEST on r8a7791 (dual Cortex A15), the kernel crashes with:
kernel BUG at mm/slab.c:2536!
Internal error: Oops - BUG: 0 [#1] SMP ARM
Modules linked in:[    1.299311] Modules linked in:

CPU: 1 PID: 1 Comm: swapper/0 Not tainted
4.5.0-rc6-koelsch-05892-ge7e45ad53ab6795e #2270
Hardware name: Generic R8A7791 (Flattened Device Tree)
task: ef422040 ti: ef442000 task.ti: ef442000
PC is at cache_alloc_refill+0x2a0/0x530
LR is at _raw_spin_unlock+0x8/0xc
pc : [<c02c6928>]    lr : [<c067f97c>]    psr: 00000093
sp : ef443d88 ip : d0a7b1f0 fp : 02000020
r10: 00000010 r9 : 00000000 r8 : 02088020
r7 : 00000000 r6 : ef5172c0 r5 : 00000000 r4 : ef5d7f40 r3 : 00004001 r2 : 00000001 r1 : 00020001 r0 : ef5172c0
Flags: nzcv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel
Control: 30c5387d Table: 40003000 DAC: 55555555
Process swapper/0 (pid: 1, stack limit = 0xef442210)
Stack: (0xef443d88 to 0xef444000)
3d80: ef48b4c0 00000000 ef5172d0 c0e38280 00000000 00001000
3da0: 02088020 60000013 ef5d7f40 c0445010 00000000 ef517340 00001000 c02c6630 3dc0: 00000008 00001000 00000000 00000400 00000002 c04444bc 00000000 00000000 3de0: 024000c0 ef517340 00000000 00004000 00000001 00000001 c0e3b000 ef517340 3e00: 00000000 00001000 00000000 ef5f8000 ef517340 c0445010 00000000 0000000f 3e20: 00001000 c0444e1c 00000000 00000000 ef517350 00000000 ef520f15 ef517340 3e40: ef443ea0 ef517390 00000000 00000000 00001000 c0445010 00000000 ef517390 3e60: 00001000 c0445044 00001000 0000000f ef5f8000 ef443ea0 c0e92a00 c0e92a00 3e80: 00000000 00000000 0000000c c0c18ee4 00001000 0000000f 00000001 00000014 3ea0: 00000001 01111000 00000020 00000020 c0e25bc8 00000000 6f5f806a 00000000 3ec0: 00000020 40004000 800b8204 00000000 00000000 00000000 00000000 00000000 3ee0: c0c3ce94 ef520f00 c0c18da4 00000000 c0e05550 c0e05550 c0e3b000 c0c2d848 3f00: 00000000 c0201760 ef5036c0 c0804b44 c0e5fdd0 00000000 c0e0be00 c0e0be7c 3f20: 00000000 60000013 00000000 efffec51 efffec59 c023893c c09dc204 00000004 3f40: 00000004 c09dce98 000000a0 c09dce98 c0e09e78 00000004 c0c2d834 00000004 3f60: c0c2d838 000000a1 c0c3ce94 c0e3b000 c0e3b000 c0c00d4c 00000004 00000004 3f80: 00000000 c0c0058c 00000000 c067a35c 00000000 00000000 00000000 00000000 3fa0: 00000000 c067a364 00000000 c0206b68 00000000 00000000 00000000 00000000 3fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 3fe0: 00000000 00000000 00000000 00000000 00000013 00000000 ef443ff4 00000000 [<c02c6928>] (cache_alloc_refill) from [<c02c6630>] (kmem_cache_alloc+0x7c/0xd4) [<c02c6630>] (kmem_cache_alloc) from [<c04444bc>] (__arm_v7s_alloc_table+0x5c/0x278)
[<c04444bc>] (__arm_v7s_alloc_table) from [<c0444e1c>] (__arm_v7s_map.constprop.6+0x68/0x25c)
[<c0444e1c>] (__arm_v7s_map.constprop.6) from [<c0445044>] (arm_v7s_map+0x34/0xa4)
[<c0445044>] (arm_v7s_map) from [<c0c18ee4>] (arm_v7s_do_selftests+0x140/0x418) [<c0c18ee4>] (arm_v7s_do_selftests) from [<c0201760>] (do_one_initcall+0x100/0x1b4)
[<c0201760>] (do_one_initcall) from [<c0c00d4c>] (kernel_init_freeable+0x120/0x1e8)
[<c0c00d4c>] (kernel_init_freeable) from [<c067a364>] (kernel_init+0x8/0xec) [<c067a364>] (kernel_init) from [<c0206b68>] (ret_from_fork+0x14/0x2c)

Quoted 8 lines

Code: 1a000003 e7f001f2 e3130001 0a000000 (e7f001f2)
---[ end trace 190f6f6b84352efd ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
I don't know if r8a7791 supports v7s. Still, I don't expect a crash. The LPAE selftest runs fine.
On r8a7795 (arm64), both the LPAE and V7S selftests run fine.

The hardware itself shouldn't matter, as the selftests run without reference to any device at all. At a glance this looks like a difference in behaviour between SLAB and SLUB - I see shmobile_defconfig sets CONFIG_SLAB=y. I'll reproduce it locally to make sure.

Thanks for the report,
Robin.

Quoted 11 lines

Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Robin Murphy

author reviewer

2016-01-26 17:13 UTC #16 lore raw

PATCH v3 2/3 iommu/io-pgtable: Add helper functions for TLB ops

In reply to Robin Murphy

Add some simple wrappers to avoid having the guts of the TLB operations spilled all over the page table implementations, and to provide a point to implement extra common functionality.

Acked-by Will Deacon <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/io-pgtable-arm-v7s.c | 48 ++++++++++++++++----------------------
 drivers/iommu/io-pgtable-arm.c     | 21 +++++++----------
 drivers/iommu/io-pgtable.c         |  2 +-
 drivers/iommu/io-pgtable.h         | 16 +++++++++++++
 4 files changed, 46 insertions(+), 41 deletions(-)

drivers/iommu/io-pgtable-arm-v7s.c +20 ~~-28~~

index efc7d1e..d39a021 100644
@@ -417,9 +417,7 @@ static int arm_v7s_map(struct io_pgtable_ops *ops, unsigned long iova,
 			phys_addr_t paddr, size_t size, int prot)
 {
 	struct arm_v7s_io_pgtable *data = io_pgtable_ops_to_data(ops);
-	struct io_pgtable_cfg *cfg = &data->iop.cfg;
-	const struct iommu_gather_ops *tlb = cfg->tlb;
-	void *cookie = data->iop.cookie;
+	struct io_pgtable *iop = &data->iop;
 	int ret;
 
 	/* If no access, then nothing to do */

@@ -431,10 +429,10 @@ static int arm_v7s_map(struct io_pgtable_ops *ops, unsigned long iova,
 	 * Synchronise all PTE updates for the new mapping before there's
 	 * a chance for anything to kick off a table walk for the new iova.
 	 */
-	if (cfg->quirks & IO_PGTABLE_QUIRK_TLBI_ON_MAP) {
-		tlb->tlb_add_flush(iova, size, ARM_V7S_BLOCK_SIZE(2), false,
-				   cookie);
-		tlb->tlb_sync(cookie);
+	if (iop->cfg.quirks & IO_PGTABLE_QUIRK_TLBI_ON_MAP) {
+		io_pgtable_tlb_add_flush(iop, iova, size,
+					 ARM_V7S_BLOCK_SIZE(2), false);
+		io_pgtable_tlb_sync(iop);
 	} else {
 		wmb();
 	}

@@ -462,8 +460,7 @@ static void arm_v7s_split_cont(struct arm_v7s_io_pgtable *data,
 			       unsigned long iova, int idx, int lvl,
 			       arm_v7s_iopte *ptep)
 {
-	struct io_pgtable_cfg *cfg = &data->iop.cfg;
-	void *cookie = data->iop.cookie;
+	struct io_pgtable *iop = &data->iop;
 	arm_v7s_iopte pte;
 	size_t size = ARM_V7S_BLOCK_SIZE(lvl);
 	int i;

@@ -475,11 +472,11 @@ static void arm_v7s_split_cont(struct arm_v7s_io_pgtable *data,
 		pte += size;
 	}
 
-	__arm_v7s_pte_sync(ptep, ARM_V7S_CONT_PAGES, cfg);
+	__arm_v7s_pte_sync(ptep, ARM_V7S_CONT_PAGES, &iop->cfg);
 
 	size *= ARM_V7S_CONT_PAGES;
-	cfg->tlb->tlb_add_flush(iova, size, size, true, cookie);
-	cfg->tlb->tlb_sync(cookie);
+	io_pgtable_tlb_add_flush(iop, iova, size, size, true);
+	io_pgtable_tlb_sync(iop);
 }
 
 static int arm_v7s_split_blk_unmap(struct arm_v7s_io_pgtable *data,

@@ -489,7 +486,6 @@ static int arm_v7s_split_blk_unmap(struct arm_v7s_io_pgtable *data,
 	unsigned long blk_start, blk_end, blk_size;
 	phys_addr_t blk_paddr;
 	arm_v7s_iopte table = 0;
-	struct io_pgtable_cfg *cfg = &data->iop.cfg;
 	int prot = arm_v7s_pte_to_prot(*ptep, 1);
 
 	blk_size = ARM_V7S_BLOCK_SIZE(1);

@@ -517,9 +513,9 @@ static int arm_v7s_split_blk_unmap(struct arm_v7s_io_pgtable *data,
 		}
 	}
 
-	__arm_v7s_set_pte(ptep, table, 1, cfg);
+	__arm_v7s_set_pte(ptep, table, 1, &data->iop.cfg);
 	iova &= ~(blk_size - 1);
-	cfg->tlb->tlb_add_flush(iova, blk_size, blk_size, true, data->iop.cookie);
+	io_pgtable_tlb_add_flush(&data->iop, iova, blk_size, blk_size, true);
 	return size;
 }

@@ -528,9 +524,7 @@ static int __arm_v7s_unmap(struct arm_v7s_io_pgtable *data,
 			    arm_v7s_iopte *ptep)
 {
 	arm_v7s_iopte pte[ARM_V7S_CONT_PAGES];
-	struct io_pgtable_cfg *cfg = &data->iop.cfg;
-	const struct iommu_gather_ops *tlb = cfg->tlb;
-	void *cookie = data->iop.cookie;
+	struct io_pgtable *iop = &data->iop;
 	int idx, i = 0, num_entries = size >> ARM_V7S_LVL_SHIFT(lvl);
 
 	/* Something went horribly wrong and we ran out of page table */

@@ -556,20 +550,19 @@ static int __arm_v7s_unmap(struct arm_v7s_io_pgtable *data,
 	if (num_entries) {
 		size_t blk_size = ARM_V7S_BLOCK_SIZE(lvl);
 
-		__arm_v7s_set_pte(ptep, 0, num_entries, cfg);
+		__arm_v7s_set_pte(ptep, 0, num_entries, &iop->cfg);
 
 		for (i = 0; i < num_entries; i++) {
 			if (ARM_V7S_PTE_IS_TABLE(pte[i], lvl)) {
 				/* Also flush any partial walks */
-				tlb->tlb_add_flush(iova, blk_size,
-						   ARM_V7S_BLOCK_SIZE(lvl + 1),
-						   false, cookie);
-				tlb->tlb_sync(cookie);
+				io_pgtable_tlb_add_flush(iop, iova, blk_size,
+					ARM_V7S_BLOCK_SIZE(lvl + 1), false);
+				io_pgtable_tlb_sync(iop);
 				ptep = iopte_deref(pte[i], lvl);
 				__arm_v7s_free_table(ptep, lvl + 1, data);
 			} else {
-				tlb->tlb_add_flush(iova, blk_size, blk_size,
-						   true, cookie);
+				io_pgtable_tlb_add_flush(iop, iova, blk_size,
+							 blk_size, true);
 			}
 			iova += blk_size;
 		}

@@ -590,13 +583,12 @@ static int __arm_v7s_unmap(struct arm_v7s_io_pgtable *data,
 static int arm_v7s_unmap(struct io_pgtable_ops *ops, unsigned long iova,
 			 size_t size)
 {
-	size_t unmapped;
 	struct arm_v7s_io_pgtable *data = io_pgtable_ops_to_data(ops);
-	struct io_pgtable *iop = &data->iop;
+	size_t unmapped;
 
 	unmapped = __arm_v7s_unmap(data, iova, size, 1, data->pgd);
 	if (unmapped)
-		iop->cfg.tlb->tlb_sync(iop->cookie);
+		io_pgtable_tlb_sync(&data->iop);
 
 	return unmapped;
 }

drivers/iommu/io-pgtable-arm.c +9 ~~-12~~

index 8bbcbfe..4095af2 100644
@@ -445,7 +445,6 @@ static int arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
 	unsigned long blk_start, blk_end;
 	phys_addr_t blk_paddr;
 	arm_lpae_iopte table = 0;
-	struct io_pgtable_cfg *cfg = &data->iop.cfg;
 
 	blk_start = iova & ~(blk_size - 1);
 	blk_end = blk_start + blk_size;

@@ -471,9 +470,9 @@ static int arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
 		}
 	}
 
-	__arm_lpae_set_pte(ptep, table, cfg);
+	__arm_lpae_set_pte(ptep, table, &data->iop.cfg);
 	iova &= ~(blk_size - 1);
-	cfg->tlb->tlb_add_flush(iova, blk_size, blk_size, true, data->iop.cookie);
+	io_pgtable_tlb_add_flush(&data->iop, iova, blk_size, blk_size, true);
 	return size;
 }

@@ -482,8 +481,7 @@ static int __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
 			    arm_lpae_iopte *ptep)
 {
 	arm_lpae_iopte pte;
-	const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
-	void *cookie = data->iop.cookie;
+	struct io_pgtable *iop = &data->iop;
 	size_t blk_size = ARM_LPAE_BLOCK_SIZE(lvl, data);
 
 	/* Something went horribly wrong and we ran out of page table */

@@ -497,17 +495,17 @@ static int __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
 
 	/* If the size matches this level, we're in the right place */
 	if (size == blk_size) {
-		__arm_lpae_set_pte(ptep, 0, &data->iop.cfg);
+		__arm_lpae_set_pte(ptep, 0, &iop->cfg);
 
 		if (!iopte_leaf(pte, lvl)) {
 			/* Also flush any partial walks */
-			tlb->tlb_add_flush(iova, size, ARM_LPAE_GRANULE(data),
-					   false, cookie);
-			tlb->tlb_sync(cookie);
+			io_pgtable_tlb_add_flush(iop, iova, size,
+						ARM_LPAE_GRANULE(data), false);
+			io_pgtable_tlb_sync(iop);
 			ptep = iopte_deref(pte, data);
 			__arm_lpae_free_pgtable(data, lvl + 1, ptep);
 		} else {
-			tlb->tlb_add_flush(iova, size, size, true, cookie);
+			io_pgtable_tlb_add_flush(iop, iova, size, size, true);
 		}
 
 		return size;

@@ -531,13 +529,12 @@ static int arm_lpae_unmap(struct io_pgtable_ops *ops, unsigned long iova,
 {
 	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);
+		io_pgtable_tlb_sync(&data->iop);
 
 	return unmapped;
 }

drivers/iommu/io-pgtable.c +1 -1

index 8c615b7..876f6a7 100644
@@ -75,6 +75,6 @@ void free_io_pgtable_ops(struct io_pgtable_ops *ops)
 		return;
 
 	iop = container_of(ops, struct io_pgtable, ops);
-	iop->cfg.tlb->tlb_flush_all(iop->cookie);
+	io_pgtable_tlb_flush_all(iop);
 	io_pgtable_init_table[iop->fmt]->free(iop);
 }

drivers/iommu/io-pgtable.h +16 -0

index aa57073..95c5565 100644
@@ -144,6 +144,22 @@ struct io_pgtable {
 
 #define io_pgtable_ops_to_pgtable(x) container_of((x), struct io_pgtable, ops)
 
+static inline void io_pgtable_tlb_flush_all(struct io_pgtable *iop)
+{
+	iop->cfg.tlb->tlb_flush_all(iop->cookie);
+}
+
+static inline void io_pgtable_tlb_add_flush(struct io_pgtable *iop,
+		unsigned long iova, size_t size, size_t granule, bool leaf)
+{
+	iop->cfg.tlb->tlb_add_flush(iova, size, granule, leaf, iop->cookie);
+}
+
+static inline void io_pgtable_tlb_sync(struct io_pgtable *iop)
+{
+	iop->cfg.tlb->tlb_sync(iop->cookie);
+}
+
 /**
  * struct io_pgtable_init_fns - Alloc/free a set of page tables for a
  *                              particular format.

Signature · 1 line

2.7.0.25.gfc10eb5

Robin Murphy

author reviewer

2016-01-26 17:13 UTC #17 lore raw

PATCH v3 3/3 iommu/io-pgtable: Avoid redundant TLB syncs

In reply to Robin Murphy

In certain unmapping situations it is quite possible to end up issuing back-to-back TLB synchronisations, which at best is a waste of time and effort, and at worst causes some hardware to get rather confused. Whilst the pagetable implementations, or the IOMMU drivers, or both, could keep track of things to avoid this happening, it seems to make the most sense to prevent code duplication and add some simple state tracking in the common interface between the two.

Reviewed-by Laurent Pinchart <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/io-pgtable.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

drivers/iommu/io-pgtable.h +8 -1

index 95c5565..4faee7d 100644
@@ -132,12 +132,14 @@ void free_io_pgtable_ops(struct io_pgtable_ops *ops);
  * @fmt:    The page table format.
  * @cookie: An opaque token provided by the IOMMU driver and passed back to
  *          any callback routines.
+ * @tlb_sync_pending: Private flag for optimising out redundant syncs.
  * @cfg:    A copy of the page table configuration.
  * @ops:    The page table operations in use for this set of page tables.
  */
 struct io_pgtable {
 	enum io_pgtable_fmt	fmt;
 	void			*cookie;
+	bool			tlb_sync_pending;
 	struct io_pgtable_cfg	cfg;
 	struct io_pgtable_ops	ops;
 };

@@ -147,17 +149,22 @@ struct io_pgtable {
 static inline void io_pgtable_tlb_flush_all(struct io_pgtable *iop)
 {
 	iop->cfg.tlb->tlb_flush_all(iop->cookie);
+	iop->tlb_sync_pending = true;
 }
 
 static inline void io_pgtable_tlb_add_flush(struct io_pgtable *iop,
 		unsigned long iova, size_t size, size_t granule, bool leaf)
 {
 	iop->cfg.tlb->tlb_add_flush(iova, size, granule, leaf, iop->cookie);
+	iop->tlb_sync_pending = true;
 }
 
 static inline void io_pgtable_tlb_sync(struct io_pgtable *iop)
 {
-	iop->cfg.tlb->tlb_sync(iop->cookie);
+	if (iop->tlb_sync_pending) {
+		iop->cfg.tlb->tlb_sync(iop->cookie);
+		iop->tlb_sync_pending = false;
+	}
 }
 
 /**

Signature · 1 line

2.7.0.25.gfc10eb5

Will Deacon

2016-02-10 14:21 UTC #18 lore raw

PATCH v3 3/3 iommu/io-pgtable: Avoid redundant TLB syncs

Quoted 8 lines · Robin Murphy

In certain unmapping situations it is quite possible to end up issuing back-to-back TLB synchronisations, which at best is a waste of time and effort, and at worst causes some hardware to get rather confused. Whilst the pagetable implementations, or the IOMMU drivers, or both, could keep track of things to avoid this happening, it seems to make the most sense to prevent code duplication and add some simple state tracking in the common interface between the two.

Quoted 5 lines

Reviewed-by Laurent Pinchart <[email protected]>
Signed-off-by Robin Murphy <[email protected]>
---
 drivers/iommu/io-pgtable.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Acked-by Will Deacon <[email protected]>

Will

Robin Murphy

author reviewer

2016-02-12 17:09 UTC #19 lore raw

PATCH v2 1/1 iommu/io-pgtable: Rationalise quirk handling

In reply to Robin Murphy

As the number of io-pgtable implementations grows beyond 1, it's time to rationalise the quirks mechanism before things have a chance to start getting really ugly and out-of-hand.

To that end:
- Indicate exactly which quirks each format can/does support.
- Fail creating a table if a caller wants unsupported quirks.
- Properly document where each quirk applies and why.

Signed-off-by Robin Murphy <[email protected]>

---

Just to allay any confusion, I intend this as a followup to the short-descriptor[1] and Mediatek IOMMU[2] patches, rather than a crucial addition, so it can either get picked up along with those or wait 'til later.

v2:
- Use unsigned long for the bitmap.
- Do format-specific checks inline.
- Enough of a difference that I don't feel comfortable putting Laurent's reviewed-by from v1 on it...

Robin.

[1]:http://thread.gmane.org/gmane.linux.kernel.iommu/12007 [2]:http://thread.gmane.org/gmane.linux.kernel.iommu/11974

 drivers/iommu/io-pgtable-arm-v7s.c |  5 +++++
 drivers/iommu/io-pgtable-arm.c     | 13 +++++++++++--
 drivers/iommu/io-pgtable.h         | 22 ++++++++++++++++++----
 3 files changed, 34 insertions(+), 6 deletions(-)

drivers/iommu/io-pgtable-arm-v7s.c +5 -0

index d39a021..9fcceb1 100644
@@ -623,6 +623,11 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,
 	if (cfg->ias > ARM_V7S_ADDR_BITS || cfg->oas > ARM_V7S_ADDR_BITS)
 		return NULL;
 
+	if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS |
+			    IO_PGTABLE_QUIRK_NO_PERMS |
+			    IO_PGTABLE_QUIRK_TLBI_ON_MAP))
+		return NULL;
+
 	data = kmalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return NULL;

drivers/iommu/io-pgtable-arm.c +11 -2

index 4095af2..debdf04 100644
@@ -658,8 +658,12 @@ static struct io_pgtable *
 arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg *cfg, void *cookie)
 {
 	u64 reg;
-	struct arm_lpae_io_pgtable *data = arm_lpae_alloc_pgtable(cfg);
+	struct arm_lpae_io_pgtable *data;
 
+	if (cfg->quirks & ~IO_PGTABLE_QUIRK_ARM_NS)
+		return NULL;
+
+	data = arm_lpae_alloc_pgtable(cfg);
 	if (!data)
 		return NULL;

@@ -742,8 +746,13 @@ static struct io_pgtable *
 arm_64_lpae_alloc_pgtable_s2(struct io_pgtable_cfg *cfg, void *cookie)
 {
 	u64 reg, sl;
-	struct arm_lpae_io_pgtable *data = arm_lpae_alloc_pgtable(cfg);
+	struct arm_lpae_io_pgtable *data;
 
+	/* The NS quirk doesn't apply at stage 2 */
+	if (cfg->quirks)
+		return NULL;
+
+	data = arm_lpae_alloc_pgtable(cfg);
 	if (!data)
 		return NULL;

drivers/iommu/io-pgtable.h +18 -4

index 4faee7d..d4f5027 100644
@@ -47,10 +47,24 @@ struct iommu_gather_ops {
  *                 page table walker.
  */
 struct io_pgtable_cfg {
-	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0) /* Set NS bit in PTEs */
-	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1) /* No AP/XN bits */
-	#define IO_PGTABLE_QUIRK_TLBI_ON_MAP	BIT(2) /* TLB Inv. on map */
-	int				quirks;
+	/*
+	 * IO_PGTABLE_QUIRK_ARM_NS: (ARM formats) Set NS and NSTABLE bits in
+	 *	stage 1 PTEs, for hardware which insists on validating them
+	 *	even in	non-secure state where they should normally be ignored.
+	 *
+	 * IO_PGTABLE_QUIRK_NO_PERMS: Ignore the IOMMU_READ, IOMMU_WRITE and
+	 *	IOMMU_NOEXEC flags and map everything with full access, for
+	 *	hardware which does not implement the permissions of a given
+	 *	format, and/or requires some format-specific default value.
+	 *
+	 * IO_PGTABLE_QUIRK_TLBI_ON_MAP: If the format forbids caching invalid
+	 *	(unmapped) entries but the hardware might do so anyway, perform
+	 *	TLB maintenance when mapping as well as when unmapping.
+	 */
+	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0)
+	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1)
+	#define IO_PGTABLE_QUIRK_TLBI_ON_MAP	BIT(2)
+	unsigned long			quirks;
 	unsigned long			pgsize_bitmap;
 	unsigned int			ias;
 	unsigned int			oas;

Signature · 1 line

2.7.0.25.gfc10eb5.dirty

Laurent Pinchart

2016-02-12 17:24 UTC #20 lore raw

PATCH v2 1/1 iommu/io-pgtable: Rationalise quirk handling

Hi Robin,

Thank you for the patch.

Quoted 17 lines · On Friday 12 February 2016 17:09:46 Robin Murphy wrote:

As the number of io-pgtable implementations grows beyond 1, it's time to rationalise the quirks mechanism before things have a chance to start getting really ugly and out-of-hand.
To that end:
- Indicate exactly which quirks each format can/does support.
- Fail creating a table if a caller wants unsupported quirks.
- Properly document where each quirk applies and why.
Signed-off-by Robin Murphy <[email protected]>
---
Just to allay any confusion, I intend this as a followup to the short-descriptor[1] and Mediatek IOMMU[2] patches, rather than a crucial addition, so it can either get picked up along with those or wait 'til later.

Quoted 5 lines

v2:
- Use unsigned long for the bitmap.
- Do format-specific checks inline.
- Enough of a difference that I don't feel comfortable putting Laurent's reviewed-by from v1 on it...

Here it is again.

Reviewed-by Laurent Pinchart <[email protected]>

Quoted 90 lines

[1]:http://thread.gmane.org/gmane.linux.kernel.iommu/12007 [2]:http://thread.gmane.org/gmane.linux.kernel.iommu/11974

 drivers/iommu/io-pgtable-arm-v7s.c |  5 +++++
 drivers/iommu/io-pgtable-arm.c     | 13 +++++++++++--
 drivers/iommu/io-pgtable.h         | 22 ++++++++++++++++++----
 3 files changed, 34 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/drivers/iommu/io-pgtable-arm-v7s.c index d39a021..9fcceb1 100644

drivers/iommu/io-pgtable-arm-v7s.c +0 -0

@@ -623,6 +623,11 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct

io_pgtable_cfg *cfg, if (cfg->ias > ARM_V7S_ADDR_BITS || cfg->oas > ARM_V7S_ADDR_BITS) return NULL;

drivers/iommu/io-pgtable-arm-v7s.c +5 -0

+	if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS |
+			    IO_PGTABLE_QUIRK_NO_PERMS |
+			    IO_PGTABLE_QUIRK_TLBI_ON_MAP))
+		return NULL;
+
 	data = kmalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return NULL;

drivers/iommu/io-pgtable-arm.c +11 -2

index 4095af2..debdf04 100644
@@ -658,8 +658,12 @@ static struct io_pgtable *
 arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg *cfg, void *cookie)
 {
 	u64 reg;
-	struct arm_lpae_io_pgtable *data = arm_lpae_alloc_pgtable(cfg);
+	struct arm_lpae_io_pgtable *data;
 
+	if (cfg->quirks & ~IO_PGTABLE_QUIRK_ARM_NS)
+		return NULL;
+
+	data = arm_lpae_alloc_pgtable(cfg);
 	if (!data)
 		return NULL;

@@ -742,8 +746,13 @@ static struct io_pgtable *
 arm_64_lpae_alloc_pgtable_s2(struct io_pgtable_cfg *cfg, void *cookie)
 {
 	u64 reg, sl;
-	struct arm_lpae_io_pgtable *data = arm_lpae_alloc_pgtable(cfg);
+	struct arm_lpae_io_pgtable *data;
 
+	/* The NS quirk doesn't apply at stage 2 */
+	if (cfg->quirks)
+		return NULL;
+
+	data = arm_lpae_alloc_pgtable(cfg);
 	if (!data)
 		return NULL;

drivers/iommu/io-pgtable.h +18 -4

index 4faee7d..d4f5027 100644
@@ -47,10 +47,24 @@ struct iommu_gather_ops {
  *                 page table walker.
  */
 struct io_pgtable_cfg {
-	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0) /* Set NS bit in PTEs */
-	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1) /* No AP/XN bits */
-	#define IO_PGTABLE_QUIRK_TLBI_ON_MAP	BIT(2) /* TLB Inv. on map */
-	int				quirks;
+	/*
+	 * IO_PGTABLE_QUIRK_ARM_NS: (ARM formats) Set NS and NSTABLE bits in
+	 *	stage 1 PTEs, for hardware which insists on validating them
+	 *	even in	non-secure state where they should normally be ignored.
+	 *
+	 * IO_PGTABLE_QUIRK_NO_PERMS: Ignore the IOMMU_READ, IOMMU_WRITE and
+	 *	IOMMU_NOEXEC flags and map everything with full access, for
+	 *	hardware which does not implement the permissions of a given
+	 *	format, and/or requires some format-specific default value.
+	 *
+	 * IO_PGTABLE_QUIRK_TLBI_ON_MAP: If the format forbids caching invalid
+	 *	(unmapped) entries but the hardware might do so anyway, perform
+	 *	TLB maintenance when mapping as well as when unmapping.
+	 */
+	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0)
+	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1)
+	#define IO_PGTABLE_QUIRK_TLBI_ON_MAP	BIT(2)
+	unsigned long			quirks;
 	unsigned long			pgsize_bitmap;
 	unsigned int			ias;
 	unsigned int			oas;

Signature · 3 lines

Regards,

Laurent Pinchart

Robin Murphy

author reviewer

2016-02-12 17:25 UTC #21 lore raw

PATCH v2 1/1 iommu/io-pgtable: Rationalise quirk handling

Quoted 28 lines · Laurent Pinchart

Hi Robin,
Thank you for the patch.
Quoted 17 lines · On Friday 12 February 2016 17:09:46 Robin Murphy wrote:
As the number of io-pgtable implementations grows beyond 1, it's time to rationalise the quirks mechanism before things have a chance to start getting really ugly and out-of-hand.
To that end:
- Indicate exactly which quirks each format can/does support.
- Fail creating a table if a caller wants unsupported quirks.
- Properly document where each quirk applies and why.
Signed-off-by Robin Murphy <[email protected]>
---
Just to allay any confusion, I intend this as a followup to the short-descriptor[1] and Mediatek IOMMU[2] patches, rather than a crucial addition, so it can either get picked up along with those or wait 'til later.
Quoted 5 lines
v2:
- Use unsigned long for the bitmap.
- Do format-specific checks inline.
- Enough of a difference that I don't feel comfortable putting Laurent's reviewed-by from v1 on it...

Quoted 3 lines

Here it is again.
Reviewed-by Laurent Pinchart <[email protected]>

Thanks!

Robin.

Will Deacon

2016-02-12 17:27 UTC #22 lore raw

PATCH v2 1/1 iommu/io-pgtable: Rationalise quirk handling

In reply to Robin Murphy

Quoted 9 lines · Robin Murphy

As the number of io-pgtable implementations grows beyond 1, it's time to rationalise the quirks mechanism before things have a chance to start getting really ugly and out-of-hand.
To that end:
- Indicate exactly which quirks each format can/does support.
- Fail creating a table if a caller wants unsupported quirks.
- Properly document where each quirk applies and why.

Quoted 7 lines

Signed-off-by Robin Murphy <[email protected]>
---
Just to allay any confusion, I intend this as a followup to the short-descriptor[1] and Mediatek IOMMU[2] patches, rather than a crucial addition, so it can either get picked up along with those or wait 'til later.

I'd like to see it included in whichever tree takes the short-descriptor code so, in case that's not my tree:

Acked-by Will Deacon <[email protected]>

Will