PTE fixes for ARM LPAE

PATCH New Steve Capper 3 participants 8 messages Jul 3, 2014 Arm (32-bit)

Share & tools

Steve Capper

author

2014-07-03 12:06 UTC #0 lore raw

v6 cover PTE fixes for ARM LPAE

This series fixes a couple of problems that I encountered on ARM with LPAE.
1) Some pte accessors can have their results cancelled out by a downcast. This is addressed by the first patch.

2) It is impossible to distinguish between clean writeable ptes and read only ptes. This is addressed by the second patch.

Notable changes from V5->V6:
o) Renamed PMD_SECT_RDONLY to PMD_SECT_AP2 to make the distinction between software and hardware bits clearer.
o) pmd_trans_huge tidied up to use !pmd_table(.)

Notable changes in this series from V4->V5:
o) Rather than switch to L_PTE_WRITE semantics, we move L_PTE_RDONLY to a software bit. This allows for a much smaller patch set that does not affect the 2-level code.

Notable changes in this series from V3->V4:
o) Rebased to take into account the BE LPAE fix in 3.16-rc1.
o) THP logic added as this was missing.

This series has been tested with libhugetlbfs unit tests, ltp mm tests and a custom THP test to test what happens when PROT_NONE THPs are faulted. I have tested on an Arndale board with LPAE and classic MMU (with classic MMU, only ltp mm tests were run). Testing was performed against 3.16-rc3.

Any comments/feedback would be appreciated.

Cheers,
--
Steve

Steve Capper (2):
arm: mm: Introduce {pte,pmd}_isset and {pte,pmd}_isclear
arm: mm: Modify pte_write and pmd_write logic for LPAE

 arch/arm/include/asm/pgtable-3level-hwdef.h |  3 +-
 arch/arm/include/asm/pgtable-3level.h       | 49 ++++++++++++++++++-----------
 arch/arm/include/asm/pgtable.h              | 18 ++++++-----
 arch/arm/mm/dump.c                          |  4 +--
 arch/arm/mm/proc-v7-3level.S                |  9 ++++--
 5 files changed, 52 insertions(+), 31 deletions(-)

Signature · 1 line

1.9.3

Steve Capper

author

2014-07-03 12:06 UTC #1 lore raw

PATCH v6 1/2 arm: mm: Introduce {pte, pmd}_isset and {pte, pmd}_isclear

Long descriptors on ARM are 64 bits, and some pte functions such as pte_dirty return a bitwise-and of a flag with the pte value. If the flag to be tested resides in the upper 32 bits of the pte, then we run into the danger of the result being dropped if downcast.

For example:

	gather_stats(page, md, pte_dirty(*pte), 1);

where pte_dirty(*pte) is downcast to an int.

This patch introduces a new macro pte_isset which performs the bitwise and, then performs a double logical invert (where needed) to ensure predictable downcasting. The logical inverse pte_isclear is also introduced.

Equivalent pmd functions for Transparent HugePages have also been added.

Signed-off-by Steve Capper <[email protected]>

---
 arch/arm/include/asm/pgtable-3level.h | 12 ++++++++----
 arch/arm/include/asm/pgtable.h        | 18 +++++++++++-------
 2 files changed, 19 insertions(+), 11 deletions(-)

arch/arm/include/asm/pgtable-3level.h +8 -4

index 85c60ad..34f371c 100644
@@ -207,17 +207,21 @@ static inline pmd_t *pmd_offset(pud_t *pud, unsigned long addr)
 #define pte_huge(pte)		(pte_val(pte) && !(pte_val(pte) & PTE_TABLE_BIT))
 #define pte_mkhuge(pte)		(__pte(pte_val(pte) & ~PTE_TABLE_BIT))
 
-#define pmd_young(pmd)		(pmd_val(pmd) & PMD_SECT_AF)
+#define pmd_isset(pmd, val)	((u32)(val) == (val) ? pmd_val(pmd) & (val) \
+						: !!(pmd_val(pmd) & (val)))
+#define pmd_isclear(pmd, val)	(!(pmd_val(pmd) & (val)))
+
+#define pmd_young(pmd)		(pmd_isset((pmd), PMD_SECT_AF))
 
 #define __HAVE_ARCH_PMD_WRITE
-#define pmd_write(pmd)		(!(pmd_val(pmd) & PMD_SECT_RDONLY))
+#define pmd_write(pmd)		(pmd_isclear((pmd), PMD_SECT_RDONLY))
 
 #define pmd_hugewillfault(pmd)	(!pmd_young(pmd) || !pmd_write(pmd))
 #define pmd_thp_or_huge(pmd)	(pmd_huge(pmd) || pmd_trans_huge(pmd))
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
-#define pmd_trans_huge(pmd)	(pmd_val(pmd) && !(pmd_val(pmd) & PMD_TABLE_BIT))
-#define pmd_trans_splitting(pmd) (pmd_val(pmd) & PMD_SECT_SPLITTING)
+#define pmd_trans_huge(pmd)	(pmd_val(pmd) && !pmd_table(pmd))
+#define pmd_trans_splitting(pmd) (pmd_isset((pmd), PMD_SECT_SPLITTING))
 #endif
 
 #define PMD_BIT_FUNC(fn,op) \

arch/arm/include/asm/pgtable.h +11 -7

index 5478e5d..01baef0 100644
@@ -214,18 +214,22 @@ static inline pte_t *pmd_page_vaddr(pmd_t pmd)
 
 #define pte_clear(mm,addr,ptep)	set_pte_ext(ptep, __pte(0), 0)
 
+#define pte_isset(pte, val)	((u32)(val) == (val) ? pte_val(pte) & (val) \
+						: !!(pte_val(pte) & (val)))
+#define pte_isclear(pte, val)	(!(pte_val(pte) & (val)))
+
 #define pte_none(pte)		(!pte_val(pte))
-#define pte_present(pte)	(pte_val(pte) & L_PTE_PRESENT)
-#define pte_valid(pte)		(pte_val(pte) & L_PTE_VALID)
+#define pte_present(pte)	(pte_isset((pte), L_PTE_PRESENT))
+#define pte_valid(pte)		(pte_isset((pte), L_PTE_VALID))
 #define pte_accessible(mm, pte)	(mm_tlb_flush_pending(mm) ? pte_present(pte) : pte_valid(pte))
-#define pte_write(pte)		(!(pte_val(pte) & L_PTE_RDONLY))
-#define pte_dirty(pte)		(pte_val(pte) & L_PTE_DIRTY)
-#define pte_young(pte)		(pte_val(pte) & L_PTE_YOUNG)
-#define pte_exec(pte)		(!(pte_val(pte) & L_PTE_XN))
+#define pte_write(pte)		(pte_isclear((pte), L_PTE_RDONLY))
+#define pte_dirty(pte)		(pte_isset((pte), L_PTE_DIRTY))
+#define pte_young(pte)		(pte_isset((pte), L_PTE_YOUNG))
+#define pte_exec(pte)		(pte_isclear((pte), L_PTE_XN))
 #define pte_special(pte)	(0)
 
 #define pte_valid_user(pte)	\
-	(pte_valid(pte) && (pte_val(pte) & L_PTE_USER) && pte_young(pte))
+	(pte_valid(pte) && pte_isset((pte), L_PTE_USER) && pte_young(pte))
 
 #if __LINUX_ARM_ARCH__ < 6
 static inline void __sync_icache_dcache(pte_t pteval)

Signature · 1 line

1.9.3

Will Deacon

2014-07-03 16:01 UTC #2 lore raw

PATCH v6 1/2 arm: mm: Introduce {pte,pmd}_isset and {pte,pmd}_isclear

Quoted 14 lines · Steve Capper

Long descriptors on ARM are 64 bits, and some pte functions such as pte_dirty return a bitwise-and of a flag with the pte value. If the flag to be tested resides in the upper 32 bits of the pte, then we run into the danger of the result being dropped if downcast.
For example:
	gather_stats(page, md, pte_dirty(*pte), 1);
where pte_dirty(*pte) is downcast to an int.
This patch introduces a new macro pte_isset which performs the bitwise and, then performs a double logical invert (where needed) to ensure predictable downcasting. The logical inverse pte_isclear is also introduced.

Quoted 4 lines

Equivalent pmd functions for Transparent HugePages have also been added.
Signed-off-by Steve Capper <[email protected]>

Reviewed-by Will Deacon <[email protected]>

Will

Steve Capper

author

2014-07-03 12:06 UTC #3 lore raw

PATCH v6 2/2 arm: mm: Modify pte_write and pmd_write logic for LPAE

In reply to Steve Capper

For LPAE, we have the following means for encoding writable or dirty ptes:

                              L_PTE_DIRTY       L_PTE_RDONLY
    !pte_dirty && !pte_write        0               1
    !pte_dirty && pte_write         0               1
    pte_dirty && !pte_write         1               1
    pte_dirty && pte_write          1               0

So we can't distinguish between writeable clean ptes and read only ptes. This can cause problems with ptes being incorrectly flagged as read only when they are writeable but not dirty.

This patch renumbers L_PTE_RDONLY from AP[2] to a software bit #58, and adds additional logic to set AP[2] whenever the pte is read only or not dirty. That way we can distinguish between clean writeable ptes and read only ptes.

HugeTLB pages will use this new logic automatically.

We need to add some logic to Transparent HugePages to ensure that they correctly interpret the revised pgprot permissions (L_PTE_RDONLY has moved and no longer matches PMD_SECT_AP2). In the process of revising THP, the names of the PMD software bits have been prefixed with L_ to make them easier to distinguish from their hardware bit counterparts.

Signed-off-by Steve Capper <[email protected]>

---
 arch/arm/include/asm/pgtable-3level-hwdef.h |  3 ++-
 arch/arm/include/asm/pgtable-3level.h       | 41 +++++++++++++++++------------
 arch/arm/mm/dump.c                          |  4 +--
 arch/arm/mm/proc-v7-3level.S                |  9 +++++--
 4 files changed, 35 insertions(+), 22 deletions(-)

arch/arm/include/asm/pgtable-3level-hwdef.h +2 -1

index 626989f..9fd61c7 100644
@@ -43,7 +43,7 @@
 #define PMD_SECT_BUFFERABLE	(_AT(pmdval_t, 1) << 2)
 #define PMD_SECT_CACHEABLE	(_AT(pmdval_t, 1) << 3)
 #define PMD_SECT_USER		(_AT(pmdval_t, 1) << 6)		/* AP[1] */
-#define PMD_SECT_RDONLY		(_AT(pmdval_t, 1) << 7)		/* AP[2] */
+#define PMD_SECT_AP2		(_AT(pmdval_t, 1) << 7)		/* read only */
 #define PMD_SECT_S		(_AT(pmdval_t, 3) << 8)
 #define PMD_SECT_AF		(_AT(pmdval_t, 1) << 10)
 #define PMD_SECT_nG		(_AT(pmdval_t, 1) << 11)

@@ -72,6 +72,7 @@
 #define PTE_TABLE_BIT		(_AT(pteval_t, 1) << 1)
 #define PTE_BUFFERABLE		(_AT(pteval_t, 1) << 2)		/* AttrIndx[0] */
 #define PTE_CACHEABLE		(_AT(pteval_t, 1) << 3)		/* AttrIndx[1] */
+#define PTE_AP2			(_AT(pteval_t, 1) << 7)		/* AP[2] */
 #define PTE_EXT_SHARED		(_AT(pteval_t, 3) << 8)		/* SH[1:0], inner shareable */
 #define PTE_EXT_AF		(_AT(pteval_t, 1) << 10)	/* Access Flag */
 #define PTE_EXT_NG		(_AT(pteval_t, 1) << 11)	/* nG */

arch/arm/include/asm/pgtable-3level.h +24 ~~-17~~

82 lines not on this page · raw message

arch/arm/mm/dump.c +2 -2

index c508f41..00a1cf9 100644
@@ -126,8 +126,8 @@ static const struct prot_bits section_bits[] = {
 		.val	= PMD_SECT_USER,
 		.set	= "USR",
 	}, {
-		.mask	= PMD_SECT_RDONLY,
-		.val	= PMD_SECT_RDONLY,
+		.mask	= PMD_SECT_AP2,
+		.val	= PMD_SECT_AP2,
 		.set	= "ro",
 		.clear	= "RW",
 #elif __LINUX_ARM_ARCH__ >= 6

arch/arm/mm/proc-v7-3level.S +7 -2

index 22e3ad6..eb81123 100644
@@ -86,8 +86,13 @@ ENTRY(cpu_v7_set_pte_ext)
 	tst	rh, #1 << (57 - 32)		@ L_PTE_NONE
 	bicne	rl, #L_PTE_VALID
 	bne	1f
-	tst	rh, #1 << (55 - 32)		@ L_PTE_DIRTY
-	orreq	rl, #L_PTE_RDONLY
+
+	eor	ip, rh, #1 << (55 - 32)	@ toggle L_PTE_DIRTY in temp reg to
+					@ test for !L_PTE_DIRTY || L_PTE_RDONLY
+	tst	ip, #1 << (55 - 32) | 1 << (58 - 32)
+	orrne	rl, #PTE_AP2
+	biceq	rl, #PTE_AP2
+
 1:	strd	r2, r3, [r0]
 	ALT_SMP(W(nop))
 	ALT_UP (mcr	p15, 0, r0, c7, c10, 1)		@ flush_pte

Signature · 1 line

1.9.3

Russell King - ARM Linux

2014-07-03 12:18 UTC #4 lore raw

PATCH v6 2/2 arm: mm: Modify pte_write and pmd_write logic for LPAE

arch/arm/mm/dump.c static const struct prot_bits section_bits[] = { · line 130

Quoted 12 lines · Steve Capper

arch/arm/mm/dump.c +2 -2

index c508f41..00a1cf9 100644
@@ -126,8 +126,8 @@ static const struct prot_bits section_bits[] = {
 		.val	= PMD_SECT_USER,
 		.set	= "USR",
 	}, {
-		.mask	= PMD_SECT_RDONLY,
-		.val	= PMD_SECT_RDONLY,
+		.mask	= PMD_SECT_AP2,
+		.val	= PMD_SECT_AP2,

I think we're almost there, except for this hunk, which I think can just be deleted.

We want to report what the PTEs/PMDs are requested to be, not what they physically are at the point where we dump them out. In other words, we don't want to know that they're physically read-only because the dirty bit isn't set - what we want to know is that they have permission to be written to if the dirty bit were to be set.

The reasoning is that we want the dump to reflect what is possible - for example, we want to know whether a page can be executed _and_ written to.

Consider the case where the dirty bit is implemented in hardware.

Signature · 2 lines

FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Steve Capper

author

2014-07-03 13:47 UTC #5 lore raw

PATCH v6 2/2 arm: mm: Modify pte_write and pmd_write logic for LPAE

Quoted 23 lines · Russell King - ARM Linux

arch/arm/mm/dump.c static const struct prot_bits section_bits[] = { · line 130
Quoted 12 lines · Steve Capper
arch/arm/mm/dump.c +2 -2
index c508f41..00a1cf9 100644
@@ -126,8 +126,8 @@ static const struct prot_bits section_bits[] = {
 		.val	= PMD_SECT_USER,
 		.set	= "USR",
 	}, {
-		.mask	= PMD_SECT_RDONLY,
-		.val	= PMD_SECT_RDONLY,
+		.mask	= PMD_SECT_AP2,
+		.val	= PMD_SECT_AP2,
I think we're almost there, except for this hunk, which I think can just be deleted.
We want to report what the PTEs/PMDs are requested to be, not what they physically are at the point where we dump them out. In other words, we don't want to know that they're physically read-only because the dirty bit isn't set - what we want to know is that they have permission to be written to if the dirty bit were to be set.

Quoted 5 lines

The reasoning is that we want the dump to reflect what is possible - for example, we want to know whether a page can be executed _and_ written to.
Consider the case where the dirty bit is implemented in hardware.

Thanks Russell, that makes more sense. I'll change the mask and val to:
L_PMD_SECT_RDONLY, to catch the software bit.

Cheers,

Signature · 1 line

Steve

Steve Capper

author

2014-07-03 15:44 UTC #6 lore raw

PATCH v7 1/1 arm: mm: Modify pte_write and pmd_write logic for LPAE

In reply to Steve Capper

For LPAE, we have the following means for encoding writable or dirty ptes:

                              L_PTE_DIRTY       L_PTE_RDONLY
    !pte_dirty && !pte_write        0               1
    !pte_dirty && pte_write         0               1
    pte_dirty && !pte_write         1               1
    pte_dirty && pte_write          1               0

So we can't distinguish between writeable clean ptes and read only ptes. This can cause problems with ptes being incorrectly flagged as read only when they are writeable but not dirty.

HugeTLB pages will use this new logic automatically.

Signed-off-by Steve Capper <[email protected]>

---
Just posting this patch, it should replace the last patch in the series:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/268977.html

Changed in v7:
Check for L_PMD_SECT_RDONLY in kernel pagetable layout walker rather than PMD_SECT_AP2.

---
 arch/arm/include/asm/pgtable-3level-hwdef.h |  3 ++-
 arch/arm/include/asm/pgtable-3level.h       | 41 +++++++++++++++++------------
 arch/arm/mm/dump.c                          |  4 +--
 arch/arm/mm/proc-v7-3level.S                |  9 +++++--
 4 files changed, 35 insertions(+), 22 deletions(-)

arch/arm/include/asm/pgtable-3level-hwdef.h +2 -1

index 626989f..9fd61c7 100644
@@ -43,7 +43,7 @@
 #define PMD_SECT_BUFFERABLE	(_AT(pmdval_t, 1) << 2)
 #define PMD_SECT_CACHEABLE	(_AT(pmdval_t, 1) << 3)
 #define PMD_SECT_USER		(_AT(pmdval_t, 1) << 6)		/* AP[1] */
-#define PMD_SECT_RDONLY		(_AT(pmdval_t, 1) << 7)		/* AP[2] */
+#define PMD_SECT_AP2		(_AT(pmdval_t, 1) << 7)		/* read only */
 #define PMD_SECT_S		(_AT(pmdval_t, 3) << 8)
 #define PMD_SECT_AF		(_AT(pmdval_t, 1) << 10)
 #define PMD_SECT_nG		(_AT(pmdval_t, 1) << 11)

@@ -72,6 +72,7 @@
 #define PTE_TABLE_BIT		(_AT(pteval_t, 1) << 1)
 #define PTE_BUFFERABLE		(_AT(pteval_t, 1) << 2)		/* AttrIndx[0] */
 #define PTE_CACHEABLE		(_AT(pteval_t, 1) << 3)		/* AttrIndx[1] */
+#define PTE_AP2			(_AT(pteval_t, 1) << 7)		/* AP[2] */
 #define PTE_EXT_SHARED		(_AT(pteval_t, 3) << 8)		/* SH[1:0], inner shareable */
 #define PTE_EXT_AF		(_AT(pteval_t, 1) << 10)	/* Access Flag */
 #define PTE_EXT_NG		(_AT(pteval_t, 1) << 11)	/* nG */

arch/arm/include/asm/pgtable-3level.h +24 ~~-17~~

82 lines not on this page · raw message

arch/arm/mm/dump.c +2 -2

index c508f41..5942493 100644
@@ -126,8 +126,8 @@ static const struct prot_bits section_bits[] = {
 		.val	= PMD_SECT_USER,
 		.set	= "USR",
 	}, {
-		.mask	= PMD_SECT_RDONLY,
-		.val	= PMD_SECT_RDONLY,
+		.mask	= L_PMD_SECT_RDONLY,
+		.val	= L_PMD_SECT_RDONLY,
 		.set	= "ro",
 		.clear	= "RW",
 #elif __LINUX_ARM_ARCH__ >= 6

arch/arm/mm/proc-v7-3level.S +7 -2

index 22e3ad6..eb81123 100644
@@ -86,8 +86,13 @@ ENTRY(cpu_v7_set_pte_ext)
 	tst	rh, #1 << (57 - 32)		@ L_PTE_NONE
 	bicne	rl, #L_PTE_VALID
 	bne	1f
-	tst	rh, #1 << (55 - 32)		@ L_PTE_DIRTY
-	orreq	rl, #L_PTE_RDONLY
+
+	eor	ip, rh, #1 << (55 - 32)	@ toggle L_PTE_DIRTY in temp reg to
+					@ test for !L_PTE_DIRTY || L_PTE_RDONLY
+	tst	ip, #1 << (55 - 32) | 1 << (58 - 32)
+	orrne	rl, #PTE_AP2
+	biceq	rl, #PTE_AP2
+
 1:	strd	r2, r3, [r0]
 	ALT_SMP(W(nop))
 	ALT_UP (mcr	p15, 0, r0, c7, c10, 1)		@ flush_pte

Signature · 1 line

1.9.3

Will Deacon

2014-07-03 16:02 UTC #7 lore raw

PATCH v7 1/1 arm: mm: Modify pte_write and pmd_write logic for LPAE

Quoted 19 lines · Steve Capper

For LPAE, we have the following means for encoding writable or dirty ptes:
                              L_PTE_DIRTY       L_PTE_RDONLY
    !pte_dirty && !pte_write        0               1
    !pte_dirty && pte_write         0               1
    pte_dirty && !pte_write         1               1
    pte_dirty && pte_write          1               0
So we can't distinguish between writeable clean ptes and read only ptes. This can cause problems with ptes being incorrectly flagged as read only when they are writeable but not dirty.
This patch renumbers L_PTE_RDONLY from AP[2] to a software bit #58, and adds additional logic to set AP[2] whenever the pte is read only or not dirty. That way we can distinguish between clean writeable ptes and read only ptes.
HugeTLB pages will use this new logic automatically.

Quoted 7 lines

We need to add some logic to Transparent HugePages to ensure that they correctly interpret the revised pgprot permissions (L_PTE_RDONLY has moved and no longer matches PMD_SECT_AP2). In the process of revising THP, the names of the PMD software bits have been prefixed with L_ to make them easier to distinguish from their hardware bit counterparts.
Signed-off-by Steve Capper <[email protected]>

Reviewed-by Will Deacon <[email protected]>

Will