* [PATCH v5 01/26] powerpc/book3s64/pkeys: Fixup bit numbering
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 02/26] powerpc/book3s64/pkeys: pkeys are supported only on hash on book3s Aneesh Kumar K.V
` (24 subsequent siblings)
25 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
This number the pkey bit such that it is easy to follow. PKEY_BIT0 is
the lower order bit. This makes further changes easy to follow.
No functional change in this patch other than linux page table for
hash translation now maps pkeys differently.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/book3s/64/hash-4k.h | 9 +++----
arch/powerpc/include/asm/book3s/64/hash-64k.h | 8 +++----
arch/powerpc/include/asm/book3s/64/mmu-hash.h | 8 +++----
arch/powerpc/include/asm/pkeys.h | 24 +++++++++----------
4 files changed, 25 insertions(+), 24 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/hash-4k.h b/arch/powerpc/include/asm/book3s/64/hash-4k.h
index 3f9ae3585ab9..f889d56bf8cf 100644
--- a/arch/powerpc/include/asm/book3s/64/hash-4k.h
+++ b/arch/powerpc/include/asm/book3s/64/hash-4k.h
@@ -57,11 +57,12 @@
#define H_PMD_FRAG_NR (PAGE_SIZE >> H_PMD_FRAG_SIZE_SHIFT)
/* memory key bits, only 8 keys supported */
-#define H_PTE_PKEY_BIT0 0
-#define H_PTE_PKEY_BIT1 0
+#define H_PTE_PKEY_BIT4 0
+#define H_PTE_PKEY_BIT3 0
#define H_PTE_PKEY_BIT2 _RPAGE_RSV3
-#define H_PTE_PKEY_BIT3 _RPAGE_RSV4
-#define H_PTE_PKEY_BIT4 _RPAGE_RSV5
+#define H_PTE_PKEY_BIT1 _RPAGE_RSV4
+#define H_PTE_PKEY_BIT0 _RPAGE_RSV5
+
/*
* On all 4K setups, remap_4k_pfn() equates to remap_pfn_range()
diff --git a/arch/powerpc/include/asm/book3s/64/hash-64k.h b/arch/powerpc/include/asm/book3s/64/hash-64k.h
index 0729c034e56f..0a15fd14cf72 100644
--- a/arch/powerpc/include/asm/book3s/64/hash-64k.h
+++ b/arch/powerpc/include/asm/book3s/64/hash-64k.h
@@ -36,11 +36,11 @@
#define H_PAGE_HASHPTE _RPAGE_RPN43 /* PTE has associated HPTE */
/* memory key bits. */
-#define H_PTE_PKEY_BIT0 _RPAGE_RSV1
-#define H_PTE_PKEY_BIT1 _RPAGE_RSV2
+#define H_PTE_PKEY_BIT4 _RPAGE_RSV1
+#define H_PTE_PKEY_BIT3 _RPAGE_RSV2
#define H_PTE_PKEY_BIT2 _RPAGE_RSV3
-#define H_PTE_PKEY_BIT3 _RPAGE_RSV4
-#define H_PTE_PKEY_BIT4 _RPAGE_RSV5
+#define H_PTE_PKEY_BIT1 _RPAGE_RSV4
+#define H_PTE_PKEY_BIT0 _RPAGE_RSV5
/*
* We need to differentiate between explicit huge page and THP huge
diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
index 3fa1b962dc27..58fcc959f9d5 100644
--- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h
+++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
@@ -86,8 +86,8 @@
#define HPTE_R_PP0 ASM_CONST(0x8000000000000000)
#define HPTE_R_TS ASM_CONST(0x4000000000000000)
#define HPTE_R_KEY_HI ASM_CONST(0x3000000000000000)
-#define HPTE_R_KEY_BIT0 ASM_CONST(0x2000000000000000)
-#define HPTE_R_KEY_BIT1 ASM_CONST(0x1000000000000000)
+#define HPTE_R_KEY_BIT4 ASM_CONST(0x2000000000000000)
+#define HPTE_R_KEY_BIT3 ASM_CONST(0x1000000000000000)
#define HPTE_R_RPN_SHIFT 12
#define HPTE_R_RPN ASM_CONST(0x0ffffffffffff000)
#define HPTE_R_RPN_3_0 ASM_CONST(0x01fffffffffff000)
@@ -103,8 +103,8 @@
#define HPTE_R_R ASM_CONST(0x0000000000000100)
#define HPTE_R_KEY_LO ASM_CONST(0x0000000000000e00)
#define HPTE_R_KEY_BIT2 ASM_CONST(0x0000000000000800)
-#define HPTE_R_KEY_BIT3 ASM_CONST(0x0000000000000400)
-#define HPTE_R_KEY_BIT4 ASM_CONST(0x0000000000000200)
+#define HPTE_R_KEY_BIT1 ASM_CONST(0x0000000000000400)
+#define HPTE_R_KEY_BIT0 ASM_CONST(0x0000000000000200)
#define HPTE_R_KEY (HPTE_R_KEY_LO | HPTE_R_KEY_HI)
#define HPTE_V_1TB_SEG ASM_CONST(0x4000000000000000)
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 20ebf153c871..f8f4d0793789 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -35,11 +35,11 @@ static inline u64 vmflag_to_pte_pkey_bits(u64 vm_flags)
if (static_branch_likely(&pkey_disabled))
return 0x0UL;
- return (((vm_flags & VM_PKEY_BIT0) ? H_PTE_PKEY_BIT4 : 0x0UL) |
- ((vm_flags & VM_PKEY_BIT1) ? H_PTE_PKEY_BIT3 : 0x0UL) |
+ return (((vm_flags & VM_PKEY_BIT0) ? H_PTE_PKEY_BIT0 : 0x0UL) |
+ ((vm_flags & VM_PKEY_BIT1) ? H_PTE_PKEY_BIT1 : 0x0UL) |
((vm_flags & VM_PKEY_BIT2) ? H_PTE_PKEY_BIT2 : 0x0UL) |
- ((vm_flags & VM_PKEY_BIT3) ? H_PTE_PKEY_BIT1 : 0x0UL) |
- ((vm_flags & VM_PKEY_BIT4) ? H_PTE_PKEY_BIT0 : 0x0UL));
+ ((vm_flags & VM_PKEY_BIT3) ? H_PTE_PKEY_BIT3 : 0x0UL) |
+ ((vm_flags & VM_PKEY_BIT4) ? H_PTE_PKEY_BIT4 : 0x0UL));
}
static inline int vma_pkey(struct vm_area_struct *vma)
@@ -53,20 +53,20 @@ static inline int vma_pkey(struct vm_area_struct *vma)
static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
{
- return (((pteflags & H_PTE_PKEY_BIT0) ? HPTE_R_KEY_BIT0 : 0x0UL) |
- ((pteflags & H_PTE_PKEY_BIT1) ? HPTE_R_KEY_BIT1 : 0x0UL) |
- ((pteflags & H_PTE_PKEY_BIT2) ? HPTE_R_KEY_BIT2 : 0x0UL) |
+ return (((pteflags & H_PTE_PKEY_BIT4) ? HPTE_R_KEY_BIT4 : 0x0UL) |
((pteflags & H_PTE_PKEY_BIT3) ? HPTE_R_KEY_BIT3 : 0x0UL) |
- ((pteflags & H_PTE_PKEY_BIT4) ? HPTE_R_KEY_BIT4 : 0x0UL));
+ ((pteflags & H_PTE_PKEY_BIT2) ? HPTE_R_KEY_BIT2 : 0x0UL) |
+ ((pteflags & H_PTE_PKEY_BIT1) ? HPTE_R_KEY_BIT1 : 0x0UL) |
+ ((pteflags & H_PTE_PKEY_BIT0) ? HPTE_R_KEY_BIT0 : 0x0UL));
}
static inline u16 pte_to_pkey_bits(u64 pteflags)
{
- return (((pteflags & H_PTE_PKEY_BIT0) ? 0x10 : 0x0UL) |
- ((pteflags & H_PTE_PKEY_BIT1) ? 0x8 : 0x0UL) |
+ return (((pteflags & H_PTE_PKEY_BIT4) ? 0x10 : 0x0UL) |
+ ((pteflags & H_PTE_PKEY_BIT3) ? 0x8 : 0x0UL) |
((pteflags & H_PTE_PKEY_BIT2) ? 0x4 : 0x0UL) |
- ((pteflags & H_PTE_PKEY_BIT3) ? 0x2 : 0x0UL) |
- ((pteflags & H_PTE_PKEY_BIT4) ? 0x1 : 0x0UL));
+ ((pteflags & H_PTE_PKEY_BIT1) ? 0x2 : 0x0UL) |
+ ((pteflags & H_PTE_PKEY_BIT0) ? 0x1 : 0x0UL));
}
#define pkey_alloc_mask(pkey) (0x1 << pkey)
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v5 02/26] powerpc/book3s64/pkeys: pkeys are supported only on hash on book3s.
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 01/26] powerpc/book3s64/pkeys: Fixup bit numbering Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 03/26] powerpc/book3s64/pkeys: Move pkey related bits in the linux page table Aneesh Kumar K.V
` (23 subsequent siblings)
25 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
Move them to hash specific file and add BUG() for radix path.
---
.../powerpc/include/asm/book3s/64/hash-pkey.h | 32 ++++++++++++++++
arch/powerpc/include/asm/book3s/64/pkeys.h | 25 +++++++++++++
arch/powerpc/include/asm/pkeys.h | 37 ++++---------------
3 files changed, 64 insertions(+), 30 deletions(-)
create mode 100644 arch/powerpc/include/asm/book3s/64/hash-pkey.h
create mode 100644 arch/powerpc/include/asm/book3s/64/pkeys.h
diff --git a/arch/powerpc/include/asm/book3s/64/hash-pkey.h b/arch/powerpc/include/asm/book3s/64/hash-pkey.h
new file mode 100644
index 000000000000..795010897e5d
--- /dev/null
+++ b/arch/powerpc/include/asm/book3s/64/hash-pkey.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_POWERPC_BOOK3S_64_HASH_PKEY_H
+#define _ASM_POWERPC_BOOK3S_64_HASH_PKEY_H
+
+static inline u64 hash__vmflag_to_pte_pkey_bits(u64 vm_flags)
+{
+ return (((vm_flags & VM_PKEY_BIT0) ? H_PTE_PKEY_BIT0 : 0x0UL) |
+ ((vm_flags & VM_PKEY_BIT1) ? H_PTE_PKEY_BIT1 : 0x0UL) |
+ ((vm_flags & VM_PKEY_BIT2) ? H_PTE_PKEY_BIT2 : 0x0UL) |
+ ((vm_flags & VM_PKEY_BIT3) ? H_PTE_PKEY_BIT3 : 0x0UL) |
+ ((vm_flags & VM_PKEY_BIT4) ? H_PTE_PKEY_BIT4 : 0x0UL));
+}
+
+static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
+{
+ return (((pteflags & H_PTE_PKEY_BIT4) ? HPTE_R_KEY_BIT4 : 0x0UL) |
+ ((pteflags & H_PTE_PKEY_BIT3) ? HPTE_R_KEY_BIT3 : 0x0UL) |
+ ((pteflags & H_PTE_PKEY_BIT2) ? HPTE_R_KEY_BIT2 : 0x0UL) |
+ ((pteflags & H_PTE_PKEY_BIT1) ? HPTE_R_KEY_BIT1 : 0x0UL) |
+ ((pteflags & H_PTE_PKEY_BIT0) ? HPTE_R_KEY_BIT0 : 0x0UL));
+}
+
+static inline u16 hash__pte_to_pkey_bits(u64 pteflags)
+{
+ return (((pteflags & H_PTE_PKEY_BIT4) ? 0x10 : 0x0UL) |
+ ((pteflags & H_PTE_PKEY_BIT3) ? 0x8 : 0x0UL) |
+ ((pteflags & H_PTE_PKEY_BIT2) ? 0x4 : 0x0UL) |
+ ((pteflags & H_PTE_PKEY_BIT1) ? 0x2 : 0x0UL) |
+ ((pteflags & H_PTE_PKEY_BIT0) ? 0x1 : 0x0UL));
+}
+
+#endif
diff --git a/arch/powerpc/include/asm/book3s/64/pkeys.h b/arch/powerpc/include/asm/book3s/64/pkeys.h
new file mode 100644
index 000000000000..8174662a9173
--- /dev/null
+++ b/arch/powerpc/include/asm/book3s/64/pkeys.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#ifndef _ASM_POWERPC_BOOK3S_64_PKEYS_H
+#define _ASM_POWERPC_BOOK3S_64_PKEYS_H
+
+#include <asm/book3s/64/hash-pkey.h>
+
+static inline u64 vmflag_to_pte_pkey_bits(u64 vm_flags)
+{
+ if (static_branch_likely(&pkey_disabled))
+ return 0x0UL;
+
+ if (radix_enabled())
+ BUG();
+ return hash__vmflag_to_pte_pkey_bits(vm_flags);
+}
+
+static inline u16 pte_to_pkey_bits(u64 pteflags)
+{
+ if (radix_enabled())
+ BUG();
+ return hash__pte_to_pkey_bits(pteflags);
+}
+
+#endif /*_ASM_POWERPC_KEYS_H */
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index f8f4d0793789..5dd0a79d1809 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -25,23 +25,18 @@ extern u32 reserved_allocation_mask; /* bits set for reserved keys */
PKEY_DISABLE_WRITE | \
PKEY_DISABLE_EXECUTE)
+#ifdef CONFIG_PPC_BOOK3S_64
+#include <asm/book3s/64/pkeys.h>
+#else
+#error "Not supported"
+#endif
+
+
static inline u64 pkey_to_vmflag_bits(u16 pkey)
{
return (((u64)pkey << VM_PKEY_SHIFT) & ARCH_VM_PKEY_FLAGS);
}
-static inline u64 vmflag_to_pte_pkey_bits(u64 vm_flags)
-{
- if (static_branch_likely(&pkey_disabled))
- return 0x0UL;
-
- return (((vm_flags & VM_PKEY_BIT0) ? H_PTE_PKEY_BIT0 : 0x0UL) |
- ((vm_flags & VM_PKEY_BIT1) ? H_PTE_PKEY_BIT1 : 0x0UL) |
- ((vm_flags & VM_PKEY_BIT2) ? H_PTE_PKEY_BIT2 : 0x0UL) |
- ((vm_flags & VM_PKEY_BIT3) ? H_PTE_PKEY_BIT3 : 0x0UL) |
- ((vm_flags & VM_PKEY_BIT4) ? H_PTE_PKEY_BIT4 : 0x0UL));
-}
-
static inline int vma_pkey(struct vm_area_struct *vma)
{
if (static_branch_likely(&pkey_disabled))
@@ -51,24 +46,6 @@ static inline int vma_pkey(struct vm_area_struct *vma)
#define arch_max_pkey() pkeys_total
-static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
-{
- return (((pteflags & H_PTE_PKEY_BIT4) ? HPTE_R_KEY_BIT4 : 0x0UL) |
- ((pteflags & H_PTE_PKEY_BIT3) ? HPTE_R_KEY_BIT3 : 0x0UL) |
- ((pteflags & H_PTE_PKEY_BIT2) ? HPTE_R_KEY_BIT2 : 0x0UL) |
- ((pteflags & H_PTE_PKEY_BIT1) ? HPTE_R_KEY_BIT1 : 0x0UL) |
- ((pteflags & H_PTE_PKEY_BIT0) ? HPTE_R_KEY_BIT0 : 0x0UL));
-}
-
-static inline u16 pte_to_pkey_bits(u64 pteflags)
-{
- return (((pteflags & H_PTE_PKEY_BIT4) ? 0x10 : 0x0UL) |
- ((pteflags & H_PTE_PKEY_BIT3) ? 0x8 : 0x0UL) |
- ((pteflags & H_PTE_PKEY_BIT2) ? 0x4 : 0x0UL) |
- ((pteflags & H_PTE_PKEY_BIT1) ? 0x2 : 0x0UL) |
- ((pteflags & H_PTE_PKEY_BIT0) ? 0x1 : 0x0UL));
-}
-
#define pkey_alloc_mask(pkey) (0x1 << pkey)
#define mm_pkey_allocation_map(mm) (mm->context.pkey_allocation_map)
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v5 03/26] powerpc/book3s64/pkeys: Move pkey related bits in the linux page table
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 01/26] powerpc/book3s64/pkeys: Fixup bit numbering Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 02/26] powerpc/book3s64/pkeys: pkeys are supported only on hash on book3s Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 04/26] powerpc/book3s64/pkeys: Explain key 1 reservation details Aneesh Kumar K.V
` (22 subsequent siblings)
25 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
To keep things simple, all the pkey related bits are kept together
in linux page table for 64K config with hash translation. With hash-4k
kernel requires 4 bits to store slots details. This is done by overloading
some of the RPN bits for storing the slot details. Due to this PKEY_BIT0 on
the 4K config is used for storing hash slot details.
64K before
|....|RSV1| RSV2| RSV3 | RSV4 | RPN44| RPN43 |.... | RSV5|
|....| P4 | P3 | P2 | P1 | Busy | HASHPTE |.... | P0 |
after
|....|RSV1| RSV2| RSV3 | RSV4 | RPN44 | RPN43 |.... | RSV5 |
|....| P4 | P3 | P2 | P1 | P0 | HASHPTE |.... | Busy |
4k before
|....| RSV1 | RSV2 | RSV3 | RSV4 | RPN44| RPN43.... | RSV5|
|....| Busy | HASHPTE | P2 | P1 | F_SEC| F_GIX.... | P0 |
after
|....| RSV1 | RSV2| RSV3 | RSV4 | Free | RPN43.... | RSV5 |
|....| HASHPTE | P2 | P1 | P0 | F_SEC| F_GIX.... | BUSY |
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/book3s/64/hash-4k.h | 16 ++++++++--------
arch/powerpc/include/asm/book3s/64/hash-64k.h | 12 ++++++------
arch/powerpc/include/asm/book3s/64/pgtable.h | 17 ++++++++---------
3 files changed, 22 insertions(+), 23 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/hash-4k.h b/arch/powerpc/include/asm/book3s/64/hash-4k.h
index f889d56bf8cf..082b98808701 100644
--- a/arch/powerpc/include/asm/book3s/64/hash-4k.h
+++ b/arch/powerpc/include/asm/book3s/64/hash-4k.h
@@ -34,11 +34,11 @@
#define H_PUD_TABLE_SIZE (sizeof(pud_t) << H_PUD_INDEX_SIZE)
#define H_PGD_TABLE_SIZE (sizeof(pgd_t) << H_PGD_INDEX_SIZE)
-#define H_PAGE_F_GIX_SHIFT 53
-#define H_PAGE_F_SECOND _RPAGE_RPN44 /* HPTE is in 2ndary HPTEG */
-#define H_PAGE_F_GIX (_RPAGE_RPN43 | _RPAGE_RPN42 | _RPAGE_RPN41)
-#define H_PAGE_BUSY _RPAGE_RSV1 /* software: PTE & hash are busy */
-#define H_PAGE_HASHPTE _RPAGE_RSV2 /* software: PTE & hash are busy */
+#define H_PAGE_F_GIX_SHIFT _PAGE_PA_MAX
+#define H_PAGE_F_SECOND _RPAGE_PKEY_BIT0 /* HPTE is in 2ndary HPTEG */
+#define H_PAGE_F_GIX (_RPAGE_RPN43 | _RPAGE_RPN42 | _RPAGE_RPN41)
+#define H_PAGE_BUSY _RPAGE_RSV1
+#define H_PAGE_HASHPTE _RPAGE_PKEY_BIT4
/* PTE flags to conserve for HPTE identification */
#define _PAGE_HPTEFLAGS (H_PAGE_BUSY | H_PAGE_HASHPTE | \
@@ -59,9 +59,9 @@
/* memory key bits, only 8 keys supported */
#define H_PTE_PKEY_BIT4 0
#define H_PTE_PKEY_BIT3 0
-#define H_PTE_PKEY_BIT2 _RPAGE_RSV3
-#define H_PTE_PKEY_BIT1 _RPAGE_RSV4
-#define H_PTE_PKEY_BIT0 _RPAGE_RSV5
+#define H_PTE_PKEY_BIT2 _RPAGE_PKEY_BIT3
+#define H_PTE_PKEY_BIT1 _RPAGE_PKEY_BIT2
+#define H_PTE_PKEY_BIT0 _RPAGE_PKEY_BIT1
/*
diff --git a/arch/powerpc/include/asm/book3s/64/hash-64k.h b/arch/powerpc/include/asm/book3s/64/hash-64k.h
index 0a15fd14cf72..f20de1149ebe 100644
--- a/arch/powerpc/include/asm/book3s/64/hash-64k.h
+++ b/arch/powerpc/include/asm/book3s/64/hash-64k.h
@@ -32,15 +32,15 @@
*/
#define H_PAGE_COMBO _RPAGE_RPN0 /* this is a combo 4k page */
#define H_PAGE_4K_PFN _RPAGE_RPN1 /* PFN is for a single 4k page */
-#define H_PAGE_BUSY _RPAGE_RPN44 /* software: PTE & hash are busy */
+#define H_PAGE_BUSY _RPAGE_RSV1 /* software: PTE & hash are busy */
#define H_PAGE_HASHPTE _RPAGE_RPN43 /* PTE has associated HPTE */
/* memory key bits. */
-#define H_PTE_PKEY_BIT4 _RPAGE_RSV1
-#define H_PTE_PKEY_BIT3 _RPAGE_RSV2
-#define H_PTE_PKEY_BIT2 _RPAGE_RSV3
-#define H_PTE_PKEY_BIT1 _RPAGE_RSV4
-#define H_PTE_PKEY_BIT0 _RPAGE_RSV5
+#define H_PTE_PKEY_BIT4 _RPAGE_PKEY_BIT4
+#define H_PTE_PKEY_BIT3 _RPAGE_PKEY_BIT3
+#define H_PTE_PKEY_BIT2 _RPAGE_PKEY_BIT2
+#define H_PTE_PKEY_BIT1 _RPAGE_PKEY_BIT1
+#define H_PTE_PKEY_BIT0 _RPAGE_PKEY_BIT0
/*
* We need to differentiate between explicit huge page and THP huge
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index f17442c3a092..b7c0ba977d6a 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -32,11 +32,13 @@
#define _RPAGE_SW1 0x00800
#define _RPAGE_SW2 0x00400
#define _RPAGE_SW3 0x00200
-#define _RPAGE_RSV1 0x1000000000000000UL
-#define _RPAGE_RSV2 0x0800000000000000UL
-#define _RPAGE_RSV3 0x0400000000000000UL
-#define _RPAGE_RSV4 0x0200000000000000UL
-#define _RPAGE_RSV5 0x00040UL
+#define _RPAGE_RSV1 0x00040UL
+
+#define _RPAGE_PKEY_BIT4 0x1000000000000000UL
+#define _RPAGE_PKEY_BIT3 0x0800000000000000UL
+#define _RPAGE_PKEY_BIT2 0x0400000000000000UL
+#define _RPAGE_PKEY_BIT1 0x0200000000000000UL
+#define _RPAGE_PKEY_BIT0 0x0100000000000000UL
#define _PAGE_PTE 0x4000000000000000UL /* distinguishes PTEs from pointers */
#define _PAGE_PRESENT 0x8000000000000000UL /* pte contains a translation */
@@ -58,13 +60,12 @@
*/
#define _RPAGE_RPN0 0x01000
#define _RPAGE_RPN1 0x02000
-#define _RPAGE_RPN44 0x0100000000000000UL
#define _RPAGE_RPN43 0x0080000000000000UL
#define _RPAGE_RPN42 0x0040000000000000UL
#define _RPAGE_RPN41 0x0020000000000000UL
/* Max physical address bit as per radix table */
-#define _RPAGE_PA_MAX 57
+#define _RPAGE_PA_MAX 56
/*
* Max physical address bit we will use for now.
@@ -125,8 +126,6 @@
_PAGE_ACCESSED | _PAGE_SPECIAL | _PAGE_PTE | \
_PAGE_SOFT_DIRTY | _PAGE_DEVMAP)
-#define H_PTE_PKEY (H_PTE_PKEY_BIT0 | H_PTE_PKEY_BIT1 | H_PTE_PKEY_BIT2 | \
- H_PTE_PKEY_BIT3 | H_PTE_PKEY_BIT4)
/*
* We define 2 sets of base prot bits, one for basic pages (ie,
* cacheable kernel and user pages) and one for non cacheable
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v5 04/26] powerpc/book3s64/pkeys: Explain key 1 reservation details
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (2 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 03/26] powerpc/book3s64/pkeys: Move pkey related bits in the linux page table Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 05/26] powerpc/book3s64/pkeys: Simplify the key initialization Aneesh Kumar K.V
` (21 subsequent siblings)
25 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
This explains the details w.r.t key 1.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/mm/book3s64/pkeys.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index 1199fc2bfaec..d60e6bfa3e03 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c
@@ -124,7 +124,10 @@ static int pkey_initialize(void)
#else
os_reserved = 0;
#endif
- /* Bits are in LE format. */
+ /*
+ * key 1 is recommended not to be used. PowerISA(3.0) page 1015,
+ * programming note.
+ */
reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key);
/* register mask is in BE format */
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v5 05/26] powerpc/book3s64/pkeys: Simplify the key initialization
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (3 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 04/26] powerpc/book3s64/pkeys: Explain key 1 reservation details Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 06/26] powerpc/book3s64/pkeys: Prevent key 1 modification from userspace Aneesh Kumar K.V
` (20 subsequent siblings)
25 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
Add documentation explaining the execute_only_key. The reservation and initialization mask
details are also explained in this patch.
No functional change in this patch.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/mm/book3s64/pkeys.c | 186 ++++++++++++++++++-------------
1 file changed, 107 insertions(+), 79 deletions(-)
diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index d60e6bfa3e03..3db0b3cfc322 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c
@@ -15,48 +15,71 @@
DEFINE_STATIC_KEY_TRUE(pkey_disabled);
int pkeys_total; /* Total pkeys as per device tree */
u32 initial_allocation_mask; /* Bits set for the initially allocated keys */
-u32 reserved_allocation_mask; /* Bits set for reserved keys */
+/*
+ * Keys marked in the reservation list cannot be allocated by userspace
+ */
+u32 reserved_allocation_mask;
static bool pkey_execute_disable_supported;
-static bool pkeys_devtree_defined; /* property exported by device tree */
-static u64 pkey_amr_mask; /* Bits in AMR not to be touched */
-static u64 pkey_iamr_mask; /* Bits in AMR not to be touched */
-static u64 pkey_uamor_mask; /* Bits in UMOR not to be touched */
+static u64 default_amr;
+static u64 default_iamr;
+/* Allow all keys to be modified by default */
+static u64 default_uamor = ~0x0UL;
+/*
+ * Key used to implement PROT_EXEC mmap. Denies READ/WRITE
+ * We pick key 2 because 0 is special key and 1 is reserved as per ISA.
+ */
static int execute_only_key = 2;
+
#define AMR_BITS_PER_PKEY 2
#define AMR_RD_BIT 0x1UL
#define AMR_WR_BIT 0x2UL
#define IAMR_EX_BIT 0x1UL
-#define PKEY_REG_BITS (sizeof(u64)*8)
+#define PKEY_REG_BITS (sizeof(u64) * 8)
#define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))
-static void scan_pkey_feature(void)
+static int scan_pkey_feature(void)
{
u32 vals[2];
+ int pkeys_total = 0;
struct device_node *cpu;
+ /*
+ * Pkey is not supported with Radix translation.
+ */
+ if (radix_enabled())
+ return 0;
+
cpu = of_find_node_by_type(NULL, "cpu");
if (!cpu)
- return;
+ return 0;
if (of_property_read_u32_array(cpu,
- "ibm,processor-storage-keys", vals, 2))
- return;
+ "ibm,processor-storage-keys", vals, 2) == 0) {
+ /*
+ * Since any pkey can be used for data or execute, we will
+ * just treat all keys as equal and track them as one entity.
+ */
+ pkeys_total = vals[0];
+ /* Should we check for IAMR support FIXME!! */
+ } else {
+ /*
+ * Let's assume 32 pkeys on P8 bare metal, if its not defined by device
+ * tree. We make this exception since skiboot forgot to expose this
+ * property on power8.
+ */
+ if (!firmware_has_feature(FW_FEATURE_LPAR) &&
+ cpu_has_feature(CPU_FTRS_POWER8))
+ pkeys_total = 32;
+ }
/*
- * Since any pkey can be used for data or execute, we will just treat
- * all keys as equal and track them as one entity.
+ * Adjust the upper limit, based on the number of bits supported by
+ * arch-neutral code.
*/
- pkeys_total = vals[0];
- pkeys_devtree_defined = true;
-}
-
-static inline bool pkey_mmu_enabled(void)
-{
- if (firmware_has_feature(FW_FEATURE_LPAR))
- return pkeys_total;
- else
- return cpu_has_feature(CPU_FTR_PKEY);
+ pkeys_total = min_t(int, pkeys_total,
+ ((ARCH_VM_PKEY_FLAGS >> VM_PKEY_SHIFT) + 1));
+ return pkeys_total;
}
static int pkey_initialize(void)
@@ -80,31 +103,13 @@ static int pkey_initialize(void)
!= (sizeof(u64) * BITS_PER_BYTE));
/* scan the device tree for pkey feature */
- scan_pkey_feature();
-
- /*
- * Let's assume 32 pkeys on P8 bare metal, if its not defined by device
- * tree. We make this exception since skiboot forgot to expose this
- * property on power8.
- */
- if (!pkeys_devtree_defined && !firmware_has_feature(FW_FEATURE_LPAR) &&
- cpu_has_feature(CPU_FTRS_POWER8))
- pkeys_total = 32;
-
- /*
- * Adjust the upper limit, based on the number of bits supported by
- * arch-neutral code.
- */
- pkeys_total = min_t(int, pkeys_total,
- ((ARCH_VM_PKEY_FLAGS >> VM_PKEY_SHIFT)+1));
-
- if (!pkey_mmu_enabled() || radix_enabled() || !pkeys_total)
- static_branch_enable(&pkey_disabled);
- else
+ pkeys_total = scan_pkey_feature();
+ if (pkeys_total)
static_branch_disable(&pkey_disabled);
-
- if (static_branch_likely(&pkey_disabled))
+ else {
+ static_branch_enable(&pkey_disabled);
return 0;
+ }
/*
* The device tree cannot be relied to indicate support for
@@ -118,48 +123,71 @@ static int pkey_initialize(void)
#ifdef CONFIG_PPC_4K_PAGES
/*
* The OS can manage only 8 pkeys due to its inability to represent them
- * in the Linux 4K PTE.
+ * in the Linux 4K PTE. Mark all other keys reserved.
*/
os_reserved = pkeys_total - 8;
#else
os_reserved = 0;
#endif
- /*
- * key 1 is recommended not to be used. PowerISA(3.0) page 1015,
- * programming note.
- */
- reserved_allocation_mask = (0x1 << 1) | (0x1 << execute_only_key);
-
- /* register mask is in BE format */
- pkey_amr_mask = ~0x0ul;
- pkey_amr_mask &= ~(0x3ul << pkeyshift(0));
-
- pkey_iamr_mask = ~0x0ul;
- pkey_iamr_mask &= ~(0x3ul << pkeyshift(0));
- pkey_iamr_mask &= ~(0x3ul << pkeyshift(execute_only_key));
-
- pkey_uamor_mask = ~0x0ul;
- pkey_uamor_mask &= ~(0x3ul << pkeyshift(0));
- pkey_uamor_mask &= ~(0x3ul << pkeyshift(execute_only_key));
-
- /* mark the rest of the keys as reserved and hence unavailable */
- for (i = (pkeys_total - os_reserved); i < pkeys_total; i++) {
- reserved_allocation_mask |= (0x1 << i);
- pkey_uamor_mask &= ~(0x3ul << pkeyshift(i));
- }
- initial_allocation_mask = reserved_allocation_mask | (0x1 << 0);
if (unlikely((pkeys_total - os_reserved) <= execute_only_key)) {
/*
* Insufficient number of keys to support
* execute only key. Mark it unavailable.
- * Any AMR, UAMOR, IAMR bit set for
- * this key is irrelevant since this key
- * can never be allocated.
*/
execute_only_key = -1;
+ } else {
+ /*
+ * Mark the execute_only_pkey as not available for
+ * user allocation via pkey_alloc.
+ */
+ reserved_allocation_mask |= (0x1 << execute_only_key);
+
+ /*
+ * Deny READ/WRITE for execute_only_key.
+ * Allow execute in IAMR.
+ */
+ default_amr |= (0x3ul << pkeyshift(execute_only_key));
+ default_iamr &= ~(0x3ul << pkeyshift(execute_only_key));
+
+ /*
+ * Clear the uamor bits for this key.
+ */
+ default_uamor &= ~(0x3ul << pkeyshift(execute_only_key));
}
+ /*
+ * Allow access for only key 0. And prevent any other modification.
+ */
+ default_amr &= ~(0x3ul << pkeyshift(0));
+ default_iamr &= ~(0x3ul << pkeyshift(0));
+ default_uamor &= ~(0x3ul << pkeyshift(0));
+ /*
+ * key 0 is special in that we want to consider it an allocated
+ * key which is preallocated. We don't allow changing AMR bits
+ * w.r.t key 0. But one can pkey_free(key0)
+ */
+ initial_allocation_mask |= (0x1 << 0);
+
+ /*
+ * key 1 is recommended not to be used. PowerISA(3.0) page 1015,
+ * programming note.
+ */
+ reserved_allocation_mask |= (0x1 << 1);
+
+ /*
+ * Prevent the usage of OS reserved the keys. Update UAMOR
+ * for those keys.
+ */
+ for (i = (pkeys_total - os_reserved); i < pkeys_total; i++) {
+ reserved_allocation_mask |= (0x1 << i);
+ default_uamor &= ~(0x3ul << pkeyshift(i));
+ }
+ /*
+ * Prevent the allocation of reserved keys too.
+ */
+ initial_allocation_mask |= reserved_allocation_mask;
+
return 0;
}
@@ -301,13 +329,13 @@ void thread_pkey_regs_init(struct thread_struct *thread)
if (static_branch_likely(&pkey_disabled))
return;
- thread->amr = pkey_amr_mask;
- thread->iamr = pkey_iamr_mask;
- thread->uamor = pkey_uamor_mask;
+ thread->amr = default_amr;
+ thread->iamr = default_iamr;
+ thread->uamor = default_uamor;
- write_uamor(pkey_uamor_mask);
- write_amr(pkey_amr_mask);
- write_iamr(pkey_iamr_mask);
+ write_amr(default_amr);
+ write_iamr(default_iamr);
+ write_uamor(default_uamor);
}
int __execute_only_pkey(struct mm_struct *mm)
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v5 06/26] powerpc/book3s64/pkeys: Prevent key 1 modification from userspace.
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (4 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 05/26] powerpc/book3s64/pkeys: Simplify the key initialization Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 07/26] powerpc/book3s64/pkeys: kill cpu feature key CPU_FTR_PKEY Aneesh Kumar K.V
` (19 subsequent siblings)
25 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
Key 1 is marked reserved by ISA. Setup uamor to prevent userspace modification
of the same.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/mm/book3s64/pkeys.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index 3db0b3cfc322..9e68a08799ee 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c
@@ -174,6 +174,7 @@ static int pkey_initialize(void)
* programming note.
*/
reserved_allocation_mask |= (0x1 << 1);
+ default_uamor &= ~(0x3ul << pkeyshift(1));
/*
* Prevent the usage of OS reserved the keys. Update UAMOR
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v5 07/26] powerpc/book3s64/pkeys: kill cpu feature key CPU_FTR_PKEY
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (5 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 06/26] powerpc/book3s64/pkeys: Prevent key 1 modification from userspace Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 08/26] powerpc/book3s64/pkeys: Convert execute key support to static key Aneesh Kumar K.V
` (18 subsequent siblings)
25 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
We don't use CPU_FTR_PKEY anymore. Remove the feature bit and mark it
free.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/cputable.h | 13 ++++++-------
arch/powerpc/kernel/dt_cpu_ftrs.c | 6 ------
2 files changed, 6 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
index bac2252c839e..dd0a2e77a695 100644
--- a/arch/powerpc/include/asm/cputable.h
+++ b/arch/powerpc/include/asm/cputable.h
@@ -198,7 +198,7 @@ static inline void cpu_feature_keys_init(void) { }
#define CPU_FTR_STCX_CHECKS_ADDRESS LONG_ASM_CONST(0x0000000080000000)
#define CPU_FTR_POPCNTB LONG_ASM_CONST(0x0000000100000000)
#define CPU_FTR_POPCNTD LONG_ASM_CONST(0x0000000200000000)
-#define CPU_FTR_PKEY LONG_ASM_CONST(0x0000000400000000)
+/* LONG_ASM_CONST(0x0000000400000000) Free */
#define CPU_FTR_VMX_COPY LONG_ASM_CONST(0x0000000800000000)
#define CPU_FTR_TM LONG_ASM_CONST(0x0000001000000000)
#define CPU_FTR_CFAR LONG_ASM_CONST(0x0000002000000000)
@@ -438,7 +438,7 @@ static inline void cpu_feature_keys_init(void) { }
CPU_FTR_DSCR | CPU_FTR_SAO | CPU_FTR_ASYM_SMT | \
CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
CPU_FTR_CFAR | CPU_FTR_HVMODE | \
- CPU_FTR_VMX_COPY | CPU_FTR_HAS_PPR | CPU_FTR_DABRX | CPU_FTR_PKEY)
+ CPU_FTR_VMX_COPY | CPU_FTR_HAS_PPR | CPU_FTR_DABRX )
#define CPU_FTRS_POWER8 (CPU_FTR_LWSYNC | \
CPU_FTR_PPCAS_ARCH_V2 | CPU_FTR_CTRL | CPU_FTR_ARCH_206 |\
CPU_FTR_MMCRA | CPU_FTR_SMT | \
@@ -448,7 +448,7 @@ static inline void cpu_feature_keys_init(void) { }
CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY | \
CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_DAWR | \
- CPU_FTR_ARCH_207S | CPU_FTR_TM_COMP | CPU_FTR_PKEY)
+ CPU_FTR_ARCH_207S | CPU_FTR_TM_COMP )
#define CPU_FTRS_POWER8E (CPU_FTRS_POWER8 | CPU_FTR_PMAO_BUG)
#define CPU_FTRS_POWER9 (CPU_FTR_LWSYNC | \
CPU_FTR_PPCAS_ARCH_V2 | CPU_FTR_CTRL | CPU_FTR_ARCH_206 |\
@@ -459,8 +459,8 @@ static inline void cpu_feature_keys_init(void) { }
CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY | \
CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_ARCH_207S | \
- CPU_FTR_TM_COMP | CPU_FTR_ARCH_300 | CPU_FTR_PKEY | \
- CPU_FTR_P9_TLBIE_STQ_BUG | CPU_FTR_P9_TLBIE_ERAT_BUG | CPU_FTR_P9_TIDR)
+ CPU_FTR_TM_COMP | CPU_FTR_ARCH_300 | CPU_FTR_P9_TLBIE_STQ_BUG | \
+ CPU_FTR_P9_TLBIE_ERAT_BUG | CPU_FTR_P9_TIDR)
#define CPU_FTRS_POWER9_DD2_0 (CPU_FTRS_POWER9 | CPU_FTR_P9_RADIX_PREFETCH_BUG)
#define CPU_FTRS_POWER9_DD2_1 (CPU_FTRS_POWER9 | \
CPU_FTR_P9_RADIX_PREFETCH_BUG | \
@@ -477,8 +477,7 @@ static inline void cpu_feature_keys_init(void) { }
CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY | \
CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_ARCH_207S | \
- CPU_FTR_TM_COMP | CPU_FTR_ARCH_300 | CPU_FTR_PKEY | \
- CPU_FTR_ARCH_31)
+ CPU_FTR_TM_COMP | CPU_FTR_ARCH_300 | CPU_FTR_ARCH_31)
#define CPU_FTRS_CELL (CPU_FTR_LWSYNC | \
CPU_FTR_PPCAS_ARCH_V2 | CPU_FTR_CTRL | \
CPU_FTR_ALTIVEC_COMP | CPU_FTR_MMCRA | CPU_FTR_SMT | \
diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 3a409517c031..0acec481d4d1 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -776,12 +776,6 @@ static __init void cpufeatures_cpu_quirks(void)
}
update_tlbie_feature_flag(version);
- /*
- * PKEY was not in the initial base or feature node
- * specification, but it should become optional in the next
- * cpu feature version sequence.
- */
- cur_cpu_spec->cpu_features |= CPU_FTR_PKEY;
}
static void __init cpufeatures_setup_finished(void)
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v5 08/26] powerpc/book3s64/pkeys: Convert execute key support to static key
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (6 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 07/26] powerpc/book3s64/pkeys: kill cpu feature key CPU_FTR_PKEY Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-07-06 7:19 ` Michael Ellerman
2020-06-19 13:58 ` [PATCH v5 09/26] powerpc/book3s64/pkeys: Simplify pkey disable branch Aneesh Kumar K.V
` (17 subsequent siblings)
25 siblings, 1 reply; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
Convert the bool to a static key like pkey_disabled.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/mm/book3s64/pkeys.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index 9e68a08799ee..7d400d5a4076 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c
@@ -13,13 +13,13 @@
#include <linux/of_device.h>
DEFINE_STATIC_KEY_TRUE(pkey_disabled);
+DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);
int pkeys_total; /* Total pkeys as per device tree */
u32 initial_allocation_mask; /* Bits set for the initially allocated keys */
/*
* Keys marked in the reservation list cannot be allocated by userspace
*/
u32 reserved_allocation_mask;
-static bool pkey_execute_disable_supported;
static u64 default_amr;
static u64 default_iamr;
/* Allow all keys to be modified by default */
@@ -116,9 +116,7 @@ static int pkey_initialize(void)
* execute_disable support. Instead we use a PVR check.
*/
if (pvr_version_is(PVR_POWER7) || pvr_version_is(PVR_POWER7p))
- pkey_execute_disable_supported = false;
- else
- pkey_execute_disable_supported = true;
+ static_branch_enable(&execute_pkey_disabled);
#ifdef CONFIG_PPC_4K_PAGES
/*
@@ -214,7 +212,7 @@ static inline void write_amr(u64 value)
static inline u64 read_iamr(void)
{
- if (!likely(pkey_execute_disable_supported))
+ if (static_branch_unlikely(&execute_pkey_disabled))
return 0x0UL;
return mfspr(SPRN_IAMR);
@@ -222,7 +220,7 @@ static inline u64 read_iamr(void)
static inline void write_iamr(u64 value)
{
- if (!likely(pkey_execute_disable_supported))
+ if (static_branch_unlikely(&execute_pkey_disabled))
return;
mtspr(SPRN_IAMR, value);
@@ -282,7 +280,7 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
return -EINVAL;
if (init_val & PKEY_DISABLE_EXECUTE) {
- if (!pkey_execute_disable_supported)
+ if (static_branch_unlikely(&execute_pkey_disabled))
return -EINVAL;
new_iamr_bits |= IAMR_EX_BIT;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v5 08/26] powerpc/book3s64/pkeys: Convert execute key support to static key
2020-06-19 13:58 ` [PATCH v5 08/26] powerpc/book3s64/pkeys: Convert execute key support to static key Aneesh Kumar K.V
@ 2020-07-06 7:19 ` Michael Ellerman
2020-07-06 8:47 ` Aneesh Kumar K.V
0 siblings, 1 reply; 49+ messages in thread
From: Michael Ellerman @ 2020-07-06 7:19 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev; +Cc: Aneesh Kumar K.V, linuxram, bauerman
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> Convert the bool to a static key like pkey_disabled.
I'm not convinced this is worth the added complexity of a static key.
It's not used in any performance critical paths, except for context
switch, but that's already guarded by:
if (old_thread->iamr != new_thread->iamr)
Which should always be false on machines which don't have the execute
key enabled.
cheers
> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
> index 9e68a08799ee..7d400d5a4076 100644
> --- a/arch/powerpc/mm/book3s64/pkeys.c
> +++ b/arch/powerpc/mm/book3s64/pkeys.c
> @@ -13,13 +13,13 @@
> #include <linux/of_device.h>
>
> DEFINE_STATIC_KEY_TRUE(pkey_disabled);
> +DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);
> int pkeys_total; /* Total pkeys as per device tree */
> u32 initial_allocation_mask; /* Bits set for the initially allocated keys */
> /*
> * Keys marked in the reservation list cannot be allocated by userspace
> */
> u32 reserved_allocation_mask;
> -static bool pkey_execute_disable_supported;
> static u64 default_amr;
> static u64 default_iamr;
> /* Allow all keys to be modified by default */
> @@ -116,9 +116,7 @@ static int pkey_initialize(void)
> * execute_disable support. Instead we use a PVR check.
> */
> if (pvr_version_is(PVR_POWER7) || pvr_version_is(PVR_POWER7p))
> - pkey_execute_disable_supported = false;
> - else
> - pkey_execute_disable_supported = true;
> + static_branch_enable(&execute_pkey_disabled);
>
> #ifdef CONFIG_PPC_4K_PAGES
> /*
> @@ -214,7 +212,7 @@ static inline void write_amr(u64 value)
>
> static inline u64 read_iamr(void)
> {
> - if (!likely(pkey_execute_disable_supported))
> + if (static_branch_unlikely(&execute_pkey_disabled))
> return 0x0UL;
>
> return mfspr(SPRN_IAMR);
> @@ -222,7 +220,7 @@ static inline u64 read_iamr(void)
>
> static inline void write_iamr(u64 value)
> {
> - if (!likely(pkey_execute_disable_supported))
> + if (static_branch_unlikely(&execute_pkey_disabled))
> return;
>
> mtspr(SPRN_IAMR, value);
> @@ -282,7 +280,7 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> return -EINVAL;
>
> if (init_val & PKEY_DISABLE_EXECUTE) {
> - if (!pkey_execute_disable_supported)
> + if (static_branch_unlikely(&execute_pkey_disabled))
> return -EINVAL;
> new_iamr_bits |= IAMR_EX_BIT;
> }
> --
> 2.26.2
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v5 08/26] powerpc/book3s64/pkeys: Convert execute key support to static key
2020-07-06 7:19 ` Michael Ellerman
@ 2020-07-06 8:47 ` Aneesh Kumar K.V
0 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-07-06 8:47 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: linuxram, bauerman
On 7/6/20 12:49 PM, Michael Ellerman wrote:
> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>> Convert the bool to a static key like pkey_disabled.
>
> I'm not convinced this is worth the added complexity of a static key.
>
> It's not used in any performance critical paths, except for context
> switch, but that's already guarded by:
>
> if (old_thread->iamr != new_thread->iamr)
>
> Which should always be false on machines which don't have the execute
> key enabled.
>
Ok will drop the patch.
-aneesh
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v5 09/26] powerpc/book3s64/pkeys: Simplify pkey disable branch
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (7 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 08/26] powerpc/book3s64/pkeys: Convert execute key support to static key Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 10/26] powerpc/book3s64/pkeys: Convert pkey_total to max_pkey Aneesh Kumar K.V
` (16 subsequent siblings)
25 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
Make the default value FALSE (pkey enabled) and set to TRUE when we
find the total number of keys supported to be zero.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/pkeys.h | 2 +-
arch/powerpc/mm/book3s64/pkeys.c | 7 +++----
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 5dd0a79d1809..75d2a2c19c04 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -11,7 +11,7 @@
#include <linux/jump_label.h>
#include <asm/firmware.h>
-DECLARE_STATIC_KEY_TRUE(pkey_disabled);
+DECLARE_STATIC_KEY_FALSE(pkey_disabled);
extern int pkeys_total; /* total pkeys as per device tree */
extern u32 initial_allocation_mask; /* bits set for the initially allocated keys */
extern u32 reserved_allocation_mask; /* bits set for reserved keys */
diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index 7d400d5a4076..87d882a9aaf2 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c
@@ -12,7 +12,7 @@
#include <linux/pkeys.h>
#include <linux/of_device.h>
-DEFINE_STATIC_KEY_TRUE(pkey_disabled);
+DEFINE_STATIC_KEY_FALSE(pkey_disabled);
DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);
int pkeys_total; /* Total pkeys as per device tree */
u32 initial_allocation_mask; /* Bits set for the initially allocated keys */
@@ -104,9 +104,8 @@ static int pkey_initialize(void)
/* scan the device tree for pkey feature */
pkeys_total = scan_pkey_feature();
- if (pkeys_total)
- static_branch_disable(&pkey_disabled);
- else {
+ if (!pkeys_total) {
+ /* No support for pkey. Mark it disabled */
static_branch_enable(&pkey_disabled);
return 0;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v5 10/26] powerpc/book3s64/pkeys: Convert pkey_total to max_pkey
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (8 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 09/26] powerpc/book3s64/pkeys: Simplify pkey disable branch Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-07-06 7:04 ` Michael Ellerman
2020-06-19 13:58 ` [PATCH v5 11/26] powerpc/book3s64/pkeys: Make initial_allocation_mask static Aneesh Kumar K.V
` (15 subsequent siblings)
25 siblings, 1 reply; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
max_pkey now represents max key value that userspace can allocate.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/pkeys.h | 7 +++++--
arch/powerpc/mm/book3s64/pkeys.c | 14 +++++++-------
2 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 75d2a2c19c04..652bad7334f3 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -12,7 +12,7 @@
#include <asm/firmware.h>
DECLARE_STATIC_KEY_FALSE(pkey_disabled);
-extern int pkeys_total; /* total pkeys as per device tree */
+extern int max_pkey;
extern u32 initial_allocation_mask; /* bits set for the initially allocated keys */
extern u32 reserved_allocation_mask; /* bits set for reserved keys */
@@ -44,7 +44,10 @@ static inline int vma_pkey(struct vm_area_struct *vma)
return (vma->vm_flags & ARCH_VM_PKEY_FLAGS) >> VM_PKEY_SHIFT;
}
-#define arch_max_pkey() pkeys_total
+static inline int arch_max_pkey(void)
+{
+ return max_pkey;
+}
#define pkey_alloc_mask(pkey) (0x1 << pkey)
diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index 87d882a9aaf2..a4d7287082a8 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c
@@ -14,7 +14,7 @@
DEFINE_STATIC_KEY_FALSE(pkey_disabled);
DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);
-int pkeys_total; /* Total pkeys as per device tree */
+int max_pkey; /* Maximum key value supported */
u32 initial_allocation_mask; /* Bits set for the initially allocated keys */
/*
* Keys marked in the reservation list cannot be allocated by userspace
@@ -84,7 +84,7 @@ static int scan_pkey_feature(void)
static int pkey_initialize(void)
{
- int os_reserved, i;
+ int pkeys_total, i;
/*
* We define PKEY_DISABLE_EXECUTE in addition to the arch-neutral
@@ -122,12 +122,12 @@ static int pkey_initialize(void)
* The OS can manage only 8 pkeys due to its inability to represent them
* in the Linux 4K PTE. Mark all other keys reserved.
*/
- os_reserved = pkeys_total - 8;
+ max_pkey = min(8, pkeys_total);
#else
- os_reserved = 0;
+ max_pkey = pkeys_total;
#endif
- if (unlikely((pkeys_total - os_reserved) <= execute_only_key)) {
+ if (unlikely(max_pkey <= execute_only_key)) {
/*
* Insufficient number of keys to support
* execute only key. Mark it unavailable.
@@ -174,10 +174,10 @@ static int pkey_initialize(void)
default_uamor &= ~(0x3ul << pkeyshift(1));
/*
- * Prevent the usage of OS reserved the keys. Update UAMOR
+ * Prevent the usage of OS reserved keys. Update UAMOR
* for those keys.
*/
- for (i = (pkeys_total - os_reserved); i < pkeys_total; i++) {
+ for (i = max_pkey; i < pkeys_total; i++) {
reserved_allocation_mask |= (0x1 << i);
default_uamor &= ~(0x3ul << pkeyshift(i));
}
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v5 10/26] powerpc/book3s64/pkeys: Convert pkey_total to max_pkey
2020-06-19 13:58 ` [PATCH v5 10/26] powerpc/book3s64/pkeys: Convert pkey_total to max_pkey Aneesh Kumar K.V
@ 2020-07-06 7:04 ` Michael Ellerman
2020-07-06 7:20 ` Aneesh Kumar K.V
0 siblings, 1 reply; 49+ messages in thread
From: Michael Ellerman @ 2020-07-06 7:04 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev; +Cc: Aneesh Kumar K.V, linuxram, bauerman
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> max_pkey now represents max key value that userspace can allocate.
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
> arch/powerpc/include/asm/pkeys.h | 7 +++++--
> arch/powerpc/mm/book3s64/pkeys.c | 14 +++++++-------
> 2 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> index 75d2a2c19c04..652bad7334f3 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -12,7 +12,7 @@
> #include <asm/firmware.h>
>
> DECLARE_STATIC_KEY_FALSE(pkey_disabled);
> -extern int pkeys_total; /* total pkeys as per device tree */
> +extern int max_pkey;
> extern u32 initial_allocation_mask; /* bits set for the initially allocated keys */
> extern u32 reserved_allocation_mask; /* bits set for reserved keys */
>
> @@ -44,7 +44,10 @@ static inline int vma_pkey(struct vm_area_struct *vma)
> return (vma->vm_flags & ARCH_VM_PKEY_FLAGS) >> VM_PKEY_SHIFT;
> }
>
> -#define arch_max_pkey() pkeys_total
> +static inline int arch_max_pkey(void)
> +{
> + return max_pkey;
> +}
If pkeys_total = 32 then max_pkey = 31.
So we can't just substitute one for the other. ie. arch_max_pkey() must
have been wrong, or it is wrong now.
> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
> index 87d882a9aaf2..a4d7287082a8 100644
> --- a/arch/powerpc/mm/book3s64/pkeys.c
> +++ b/arch/powerpc/mm/book3s64/pkeys.c
> @@ -14,7 +14,7 @@
>
> DEFINE_STATIC_KEY_FALSE(pkey_disabled);
> DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);
> -int pkeys_total; /* Total pkeys as per device tree */
> +int max_pkey; /* Maximum key value supported */
> u32 initial_allocation_mask; /* Bits set for the initially allocated keys */
> /*
> * Keys marked in the reservation list cannot be allocated by userspace
> @@ -84,7 +84,7 @@ static int scan_pkey_feature(void)
>
> static int pkey_initialize(void)
> {
> - int os_reserved, i;
> + int pkeys_total, i;
>
> /*
> * We define PKEY_DISABLE_EXECUTE in addition to the arch-neutral
> @@ -122,12 +122,12 @@ static int pkey_initialize(void)
> * The OS can manage only 8 pkeys due to its inability to represent them
> * in the Linux 4K PTE. Mark all other keys reserved.
> */
> - os_reserved = pkeys_total - 8;
> + max_pkey = min(8, pkeys_total);
Shouldn't it be 7 ?
> #else
> - os_reserved = 0;
> + max_pkey = pkeys_total;
> #endif
>
> - if (unlikely((pkeys_total - os_reserved) <= execute_only_key)) {
> + if (unlikely(max_pkey <= execute_only_key)) {
Isn't that an off-by-one now?
This is one-off boot time code, there's no need to clutter it with
unlikely.
> /*
> * Insufficient number of keys to support
> * execute only key. Mark it unavailable.
> @@ -174,10 +174,10 @@ static int pkey_initialize(void)
> default_uamor &= ~(0x3ul << pkeyshift(1));
>
> /*
> - * Prevent the usage of OS reserved the keys. Update UAMOR
> + * Prevent the usage of OS reserved keys. Update UAMOR
> * for those keys.
> */
> - for (i = (pkeys_total - os_reserved); i < pkeys_total; i++) {
> + for (i = max_pkey; i < pkeys_total; i++) {
Another off-by-one? Shouldn't we start from max_pkey + 1 ?
> reserved_allocation_mask |= (0x1 << i);
> default_uamor &= ~(0x3ul << pkeyshift(i));
> }
cheers
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v5 10/26] powerpc/book3s64/pkeys: Convert pkey_total to max_pkey
2020-07-06 7:04 ` Michael Ellerman
@ 2020-07-06 7:20 ` Aneesh Kumar K.V
2020-07-07 1:26 ` Michael Ellerman
0 siblings, 1 reply; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-07-06 7:20 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: linuxram, bauerman
On 7/6/20 12:34 PM, Michael Ellerman wrote:
> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>> max_pkey now represents max key value that userspace can allocate.
>>
I guess commit message is confusing.
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>> arch/powerpc/include/asm/pkeys.h | 7 +++++--
>> arch/powerpc/mm/book3s64/pkeys.c | 14 +++++++-------
>> 2 files changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
>> index 75d2a2c19c04..652bad7334f3 100644
>> --- a/arch/powerpc/include/asm/pkeys.h
>> +++ b/arch/powerpc/include/asm/pkeys.h
>> @@ -12,7 +12,7 @@
>> #include <asm/firmware.h>
>>
>> DECLARE_STATIC_KEY_FALSE(pkey_disabled);
>> -extern int pkeys_total; /* total pkeys as per device tree */
>> +extern int max_pkey;
>> extern u32 initial_allocation_mask; /* bits set for the initially allocated keys */
>> extern u32 reserved_allocation_mask; /* bits set for reserved keys */
>>
>> @@ -44,7 +44,10 @@ static inline int vma_pkey(struct vm_area_struct *vma)
>> return (vma->vm_flags & ARCH_VM_PKEY_FLAGS) >> VM_PKEY_SHIFT;
>> }
>>
>> -#define arch_max_pkey() pkeys_total
>> +static inline int arch_max_pkey(void)
>> +{
>> + return max_pkey;
>> +}
>
> If pkeys_total = 32 then max_pkey = 31.
we have
#ifdef CONFIG_PPC_4K_PAGES
/*
* The OS can manage only 8 pkeys due to its inability to represent them
* in the Linux 4K PTE. Mark all other keys reserved.
*/
max_pkey = min(8, pkeys_total);
#else
max_pkey = pkeys_total;
#endif
so it is 32.
>
> So we can't just substitute one for the other. ie. arch_max_pkey() must
> have been wrong, or it is wrong now.
>
>> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
>> index 87d882a9aaf2..a4d7287082a8 100644
>> --- a/arch/powerpc/mm/book3s64/pkeys.c
>> +++ b/arch/powerpc/mm/book3s64/pkeys.c
>> @@ -14,7 +14,7 @@
>>
>> DEFINE_STATIC_KEY_FALSE(pkey_disabled);
>> DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);
>> -int pkeys_total; /* Total pkeys as per device tree */
>> +int max_pkey; /* Maximum key value supported */
>> u32 initial_allocation_mask; /* Bits set for the initially allocated keys */
>> /*
>> * Keys marked in the reservation list cannot be allocated by userspace
>> @@ -84,7 +84,7 @@ static int scan_pkey_feature(void)
>>
>> static int pkey_initialize(void)
>> {
>> - int os_reserved, i;
>> + int pkeys_total, i;
>>
>> /*
>> * We define PKEY_DISABLE_EXECUTE in addition to the arch-neutral
>> @@ -122,12 +122,12 @@ static int pkey_initialize(void)
>> * The OS can manage only 8 pkeys due to its inability to represent them
>> * in the Linux 4K PTE. Mark all other keys reserved.
>> */
>> - os_reserved = pkeys_total - 8;
>> + max_pkey = min(8, pkeys_total);
>
> Shouldn't it be 7 ?
>
>> #else
>> - os_reserved = 0;
>> + max_pkey = pkeys_total;
>> #endif
>>
>> - if (unlikely((pkeys_total - os_reserved) <= execute_only_key)) {
>> + if (unlikely(max_pkey <= execute_only_key)) {
>
> Isn't that an off-by-one now?
>
> This is one-off boot time code, there's no need to clutter it with
> unlikely.
>
>> /*
>> * Insufficient number of keys to support
>> * execute only key. Mark it unavailable.
>> @@ -174,10 +174,10 @@ static int pkey_initialize(void)
>> default_uamor &= ~(0x3ul << pkeyshift(1));
>>
>> /*
>> - * Prevent the usage of OS reserved the keys. Update UAMOR
>> + * Prevent the usage of OS reserved keys. Update UAMOR
>> * for those keys.
>> */
>> - for (i = (pkeys_total - os_reserved); i < pkeys_total; i++) {
>> + for (i = max_pkey; i < pkeys_total; i++) {
>
> Another off-by-one? Shouldn't we start from max_pkey + 1 ?
>
>> reserved_allocation_mask |= (0x1 << i);
>> default_uamor &= ~(0x3ul << pkeyshift(i));
>> }
>
It is the commit message. It is max pkey such that userspace can
allocate 0 - maxp_pkey -1.
-aneesh
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v5 10/26] powerpc/book3s64/pkeys: Convert pkey_total to max_pkey
2020-07-06 7:20 ` Aneesh Kumar K.V
@ 2020-07-07 1:26 ` Michael Ellerman
0 siblings, 0 replies; 49+ messages in thread
From: Michael Ellerman @ 2020-07-07 1:26 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev; +Cc: linuxram, bauerman
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> On 7/6/20 12:34 PM, Michael Ellerman wrote:
>> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>>> max_pkey now represents max key value that userspace can allocate.
>>>
>
> I guess commit message is confusing.
And the name.
If it's called max_pkey then it should be the maximum allowed pkey
value.
cheers
>>> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
>>> index 75d2a2c19c04..652bad7334f3 100644
>>> --- a/arch/powerpc/include/asm/pkeys.h
>>> +++ b/arch/powerpc/include/asm/pkeys.h
>>> @@ -12,7 +12,7 @@
>>> #include <asm/firmware.h>
>>>
>>> DECLARE_STATIC_KEY_FALSE(pkey_disabled);
>>> -extern int pkeys_total; /* total pkeys as per device tree */
>>> +extern int max_pkey;
>>> extern u32 initial_allocation_mask; /* bits set for the initially allocated keys */
>>> extern u32 reserved_allocation_mask; /* bits set for reserved keys */
>>>
>>> @@ -44,7 +44,10 @@ static inline int vma_pkey(struct vm_area_struct *vma)
>>> return (vma->vm_flags & ARCH_VM_PKEY_FLAGS) >> VM_PKEY_SHIFT;
>>> }
>>>
>>> -#define arch_max_pkey() pkeys_total
>>> +static inline int arch_max_pkey(void)
>>> +{
>>> + return max_pkey;
>>> +}
>>
>> If pkeys_total = 32 then max_pkey = 31.
>
> we have
>
> #ifdef CONFIG_PPC_4K_PAGES
> /*
> * The OS can manage only 8 pkeys due to its inability to represent them
> * in the Linux 4K PTE. Mark all other keys reserved.
> */
> max_pkey = min(8, pkeys_total);
> #else
> max_pkey = pkeys_total;
> #endif
>
> so it is 32.
>
>>
>> So we can't just substitute one for the other. ie. arch_max_pkey() must
>> have been wrong, or it is wrong now.
>>
>>> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
>>> index 87d882a9aaf2..a4d7287082a8 100644
>>> --- a/arch/powerpc/mm/book3s64/pkeys.c
>>> +++ b/arch/powerpc/mm/book3s64/pkeys.c
>>> @@ -14,7 +14,7 @@
>>>
>>> DEFINE_STATIC_KEY_FALSE(pkey_disabled);
>>> DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);
>>> -int pkeys_total; /* Total pkeys as per device tree */
>>> +int max_pkey; /* Maximum key value supported */
>>> u32 initial_allocation_mask; /* Bits set for the initially allocated keys */
>>> /*
>>> * Keys marked in the reservation list cannot be allocated by userspace
>>> @@ -84,7 +84,7 @@ static int scan_pkey_feature(void)
>>>
>>> static int pkey_initialize(void)
>>> {
>>> - int os_reserved, i;
>>> + int pkeys_total, i;
>>>
>>> /*
>>> * We define PKEY_DISABLE_EXECUTE in addition to the arch-neutral
>>> @@ -122,12 +122,12 @@ static int pkey_initialize(void)
>>> * The OS can manage only 8 pkeys due to its inability to represent them
>>> * in the Linux 4K PTE. Mark all other keys reserved.
>>> */
>>> - os_reserved = pkeys_total - 8;
>>> + max_pkey = min(8, pkeys_total);
>>
>> Shouldn't it be 7 ?
>>
>>> #else
>>> - os_reserved = 0;
>>> + max_pkey = pkeys_total;
>>> #endif
>>>
>>> - if (unlikely((pkeys_total - os_reserved) <= execute_only_key)) {
>>> + if (unlikely(max_pkey <= execute_only_key)) {
>>
>> Isn't that an off-by-one now?
>>
>> This is one-off boot time code, there's no need to clutter it with
>> unlikely.
>>
>>> /*
>>> * Insufficient number of keys to support
>>> * execute only key. Mark it unavailable.
>>> @@ -174,10 +174,10 @@ static int pkey_initialize(void)
>>> default_uamor &= ~(0x3ul << pkeyshift(1));
>>>
>>> /*
>>> - * Prevent the usage of OS reserved the keys. Update UAMOR
>>> + * Prevent the usage of OS reserved keys. Update UAMOR
>>> * for those keys.
>>> */
>>> - for (i = (pkeys_total - os_reserved); i < pkeys_total; i++) {
>>> + for (i = max_pkey; i < pkeys_total; i++) {
>>
>> Another off-by-one? Shouldn't we start from max_pkey + 1 ?
>>
>>> reserved_allocation_mask |= (0x1 << i);
>>> default_uamor &= ~(0x3ul << pkeyshift(i));
>>> }
>>
>
> It is the commit message. It is max pkey such that userspace can
> allocate 0 - maxp_pkey -1.
>
> -aneesh
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v5 11/26] powerpc/book3s64/pkeys: Make initial_allocation_mask static
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (9 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 10/26] powerpc/book3s64/pkeys: Convert pkey_total to max_pkey Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-07-06 7:04 ` Michael Ellerman
2020-06-19 13:58 ` [PATCH v5 12/26] powerpc/book3s64/pkeys: Mark all the pkeys above max pkey as reserved Aneesh Kumar K.V
` (14 subsequent siblings)
25 siblings, 1 reply; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
initial_allocation_mask is not used outside this file.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/pkeys.h | 1 -
arch/powerpc/mm/book3s64/pkeys.c | 2 +-
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 652bad7334f3..47c81d41ea9a 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -13,7 +13,6 @@
DECLARE_STATIC_KEY_FALSE(pkey_disabled);
extern int max_pkey;
-extern u32 initial_allocation_mask; /* bits set for the initially allocated keys */
extern u32 reserved_allocation_mask; /* bits set for reserved keys */
#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index a4d7287082a8..73b5ef1490c8 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c
@@ -15,11 +15,11 @@
DEFINE_STATIC_KEY_FALSE(pkey_disabled);
DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);
int max_pkey; /* Maximum key value supported */
-u32 initial_allocation_mask; /* Bits set for the initially allocated keys */
/*
* Keys marked in the reservation list cannot be allocated by userspace
*/
u32 reserved_allocation_mask;
+static u32 initial_allocation_mask; /* Bits set for the initially allocated keys */
static u64 default_amr;
static u64 default_iamr;
/* Allow all keys to be modified by default */
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v5 11/26] powerpc/book3s64/pkeys: Make initial_allocation_mask static
2020-06-19 13:58 ` [PATCH v5 11/26] powerpc/book3s64/pkeys: Make initial_allocation_mask static Aneesh Kumar K.V
@ 2020-07-06 7:04 ` Michael Ellerman
2020-07-06 8:48 ` Aneesh Kumar K.V
0 siblings, 1 reply; 49+ messages in thread
From: Michael Ellerman @ 2020-07-06 7:04 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev; +Cc: Aneesh Kumar K.V, linuxram, bauerman
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> initial_allocation_mask is not used outside this file.
And never modified after init, so make it __ro_after_init as well?
cheers
> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> index 652bad7334f3..47c81d41ea9a 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -13,7 +13,6 @@
>
> DECLARE_STATIC_KEY_FALSE(pkey_disabled);
> extern int max_pkey;
> -extern u32 initial_allocation_mask; /* bits set for the initially allocated keys */
> extern u32 reserved_allocation_mask; /* bits set for reserved keys */
>
> #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
> index a4d7287082a8..73b5ef1490c8 100644
> --- a/arch/powerpc/mm/book3s64/pkeys.c
> +++ b/arch/powerpc/mm/book3s64/pkeys.c
> @@ -15,11 +15,11 @@
> DEFINE_STATIC_KEY_FALSE(pkey_disabled);
> DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);
> int max_pkey; /* Maximum key value supported */
> -u32 initial_allocation_mask; /* Bits set for the initially allocated keys */
> /*
> * Keys marked in the reservation list cannot be allocated by userspace
> */
> u32 reserved_allocation_mask;
> +static u32 initial_allocation_mask; /* Bits set for the initially allocated keys */
> static u64 default_amr;
> static u64 default_iamr;
> /* Allow all keys to be modified by default */
> --
> 2.26.2
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v5 11/26] powerpc/book3s64/pkeys: Make initial_allocation_mask static
2020-07-06 7:04 ` Michael Ellerman
@ 2020-07-06 8:48 ` Aneesh Kumar K.V
0 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-07-06 8:48 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: linuxram, bauerman
On 7/6/20 12:34 PM, Michael Ellerman wrote:
> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>> initial_allocation_mask is not used outside this file.
>
> And never modified after init, so make it __ro_after_init as well?
>
ok, will update reserved_allocation_maask too.
> cheers
>
>> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
>> index 652bad7334f3..47c81d41ea9a 100644
>> --- a/arch/powerpc/include/asm/pkeys.h
>> +++ b/arch/powerpc/include/asm/pkeys.h
>> @@ -13,7 +13,6 @@
>>
>> DECLARE_STATIC_KEY_FALSE(pkey_disabled);
>> extern int max_pkey;
>> -extern u32 initial_allocation_mask; /* bits set for the initially allocated keys */
>> extern u32 reserved_allocation_mask; /* bits set for reserved keys */
>>
>> #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
>> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
>> index a4d7287082a8..73b5ef1490c8 100644
>> --- a/arch/powerpc/mm/book3s64/pkeys.c
>> +++ b/arch/powerpc/mm/book3s64/pkeys.c
>> @@ -15,11 +15,11 @@
>> DEFINE_STATIC_KEY_FALSE(pkey_disabled);
>> DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);
>> int max_pkey; /* Maximum key value supported */
>> -u32 initial_allocation_mask; /* Bits set for the initially allocated keys */
>> /*
>> * Keys marked in the reservation list cannot be allocated by userspace
>> */
>> u32 reserved_allocation_mask;
>> +static u32 initial_allocation_mask; /* Bits set for the initially allocated keys */
>> static u64 default_amr;
>> static u64 default_iamr;
>> /* Allow all keys to be modified by default */
>> --
>> 2.26.2
-aneesh
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v5 12/26] powerpc/book3s64/pkeys: Mark all the pkeys above max pkey as reserved
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (10 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 11/26] powerpc/book3s64/pkeys: Make initial_allocation_mask static Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 13/26] powerpc/book3s64/pkeys: Enable MMU_FTR_PKEY Aneesh Kumar K.V
` (13 subsequent siblings)
25 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
The hypervisor can return less than max allowed pkey (for ex: 31) instead
of 32. We should mark all the pkeys above max allowed as reserved so
that we avoid the allocation of the wrong pkey(for ex: key 31 in the above
case) by userspace.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/mm/book3s64/pkeys.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index 73b5ef1490c8..0ff59acdbb84 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c
@@ -175,9 +175,10 @@ static int pkey_initialize(void)
/*
* Prevent the usage of OS reserved keys. Update UAMOR
- * for those keys.
+ * for those keys. Also mark the rest of the bits in the
+ * 32 bit mask as reserved.
*/
- for (i = max_pkey; i < pkeys_total; i++) {
+ for (i = max_pkey; i < 32 ; i++) {
reserved_allocation_mask |= (0x1 << i);
default_uamor &= ~(0x3ul << pkeyshift(i));
}
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v5 13/26] powerpc/book3s64/pkeys: Enable MMU_FTR_PKEY
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (11 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 12/26] powerpc/book3s64/pkeys: Mark all the pkeys above max pkey as reserved Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-07-06 13:10 ` Michael Ellerman
2020-06-19 13:58 ` [PATCH v5 14/26] powerpc/book3s64/kuep: Add MMU_FTR_KUEP Aneesh Kumar K.V
` (12 subsequent siblings)
25 siblings, 1 reply; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
Parse storage keys related device tree entry in early_init_devtree
and enable MMU feature MMU_FTR_PKEY if pkeys are supported.
MMU feature is used instead of CPU feature because this enables us
to group MMU_FTR_KUAP and MMU_FTR_PKEY in asm feature fixup code.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/book3s/64/mmu.h | 6 +++
arch/powerpc/include/asm/mmu.h | 6 +++
arch/powerpc/kernel/prom.c | 5 +++
arch/powerpc/mm/book3s64/pkeys.c | 54 ++++++++++++++----------
4 files changed, 48 insertions(+), 23 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/mmu.h b/arch/powerpc/include/asm/book3s/64/mmu.h
index 5393a535240c..3371ea05b7d3 100644
--- a/arch/powerpc/include/asm/book3s/64/mmu.h
+++ b/arch/powerpc/include/asm/book3s/64/mmu.h
@@ -209,6 +209,12 @@ extern int mmu_io_psize;
void mmu_early_init_devtree(void);
void hash__early_init_devtree(void);
void radix__early_init_devtree(void);
+#ifdef CONFIG_PPC_MEM_KEYS
+void pkey_early_init_devtree(void);
+#else
+static inline void pkey_early_init_devtree(void) {}
+#endif
+
extern void hash__early_init_mmu(void);
extern void radix__early_init_mmu(void);
static inline void __init early_init_mmu(void)
diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
index f4ac25d4df05..72966d3d8f64 100644
--- a/arch/powerpc/include/asm/mmu.h
+++ b/arch/powerpc/include/asm/mmu.h
@@ -23,6 +23,7 @@
/* Radix page table supported and enabled */
#define MMU_FTR_TYPE_RADIX ASM_CONST(0x00000040)
+#define MMU_FTR_PKEY ASM_CONST(0x00000080)
/*
* Individual features below.
@@ -177,6 +178,9 @@ enum {
MMU_FTR_RADIX_KUAP |
#endif /* CONFIG_PPC_KUAP */
#endif /* CONFIG_PPC_RADIX_MMU */
+#ifdef CONFIG_PPC_MEM_KEYS
+ MMU_FTR_PKEY |
+#endif
0,
};
@@ -356,6 +360,8 @@ extern void setup_initial_memory_limit(phys_addr_t first_memblock_base,
phys_addr_t first_memblock_size);
static inline void mmu_early_init_devtree(void) { }
+static inline void pkey_early_init_devtree(void) {}
+
extern void *abatron_pteptrs[2];
#endif /* __ASSEMBLY__ */
#endif
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 6a3bac357e24..6d70797352d8 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -815,6 +815,11 @@ void __init early_init_devtree(void *params)
/* Now try to figure out if we are running on LPAR and so on */
pseries_probe_fw_features();
+ /*
+ * Initialize pkey features and default AMR/IAMR values
+ */
+ pkey_early_init_devtree();
+
#ifdef CONFIG_PPC_PS3
/* Identify PS3 firmware */
if (of_flat_dt_is_compatible(of_get_flat_dt_root(), "sony,ps3"))
diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index 0ff59acdbb84..bbba9c601e14 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c
@@ -10,7 +10,8 @@
#include <asm/mmu.h>
#include <asm/setup.h>
#include <linux/pkeys.h>
-#include <linux/of_device.h>
+#include <linux/of_fdt.h>
+
DEFINE_STATIC_KEY_FALSE(pkey_disabled);
DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);
@@ -38,38 +39,45 @@ static int execute_only_key = 2;
#define PKEY_REG_BITS (sizeof(u64) * 8)
#define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))
+static int __init dt_scan_storage_keys(unsigned long node,
+ const char *uname, int depth,
+ void *data)
+{
+ const char *type = of_get_flat_dt_prop(node, "device_type", NULL);
+ const __be32 *prop;
+ int pkeys_total;
+
+ /* We are scanning "cpu" nodes only */
+ if (type == NULL || strcmp(type, "cpu") != 0)
+ return 0;
+
+ prop = of_get_flat_dt_prop(node, "ibm,processor-storage-keys", NULL);
+ if (!prop)
+ return 0;
+ pkeys_total = be32_to_cpu(prop[0]);
+ return pkeys_total;
+}
+
static int scan_pkey_feature(void)
{
- u32 vals[2];
- int pkeys_total = 0;
- struct device_node *cpu;
+ int pkeys_total;
/*
* Pkey is not supported with Radix translation.
*/
- if (radix_enabled())
+ if (early_radix_enabled())
return 0;
- cpu = of_find_node_by_type(NULL, "cpu");
- if (!cpu)
- return 0;
+ pkeys_total = of_scan_flat_dt(dt_scan_storage_keys, NULL);
+ if (pkeys_total == 0) {
- if (of_property_read_u32_array(cpu,
- "ibm,processor-storage-keys", vals, 2) == 0) {
- /*
- * Since any pkey can be used for data or execute, we will
- * just treat all keys as equal and track them as one entity.
- */
- pkeys_total = vals[0];
- /* Should we check for IAMR support FIXME!! */
- } else {
/*
* Let's assume 32 pkeys on P8 bare metal, if its not defined by device
* tree. We make this exception since skiboot forgot to expose this
* property on power8.
*/
if (!firmware_has_feature(FW_FEATURE_LPAR) &&
- cpu_has_feature(CPU_FTRS_POWER8))
+ early_cpu_has_feature(CPU_FTRS_POWER8))
pkeys_total = 32;
}
@@ -82,7 +90,7 @@ static int scan_pkey_feature(void)
return pkeys_total;
}
-static int pkey_initialize(void)
+void __init pkey_early_init_devtree(void)
{
int pkeys_total, i;
@@ -107,9 +115,11 @@ static int pkey_initialize(void)
if (!pkeys_total) {
/* No support for pkey. Mark it disabled */
static_branch_enable(&pkey_disabled);
- return 0;
+ return;
}
+ cur_cpu_spec->mmu_features |= MMU_FTR_PKEY;
+
/*
* The device tree cannot be relied to indicate support for
* execute_disable support. Instead we use a PVR check.
@@ -187,11 +197,9 @@ static int pkey_initialize(void)
*/
initial_allocation_mask |= reserved_allocation_mask;
- return 0;
+ return;
}
-arch_initcall(pkey_initialize);
-
void pkey_mm_init(struct mm_struct *mm)
{
if (static_branch_likely(&pkey_disabled))
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v5 13/26] powerpc/book3s64/pkeys: Enable MMU_FTR_PKEY
2020-06-19 13:58 ` [PATCH v5 13/26] powerpc/book3s64/pkeys: Enable MMU_FTR_PKEY Aneesh Kumar K.V
@ 2020-07-06 13:10 ` Michael Ellerman
2020-07-06 14:09 ` Aneesh Kumar K.V
0 siblings, 1 reply; 49+ messages in thread
From: Michael Ellerman @ 2020-07-06 13:10 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev; +Cc: Aneesh Kumar K.V, linuxram, bauerman
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> Parse storage keys related device tree entry in early_init_devtree
> and enable MMU feature MMU_FTR_PKEY if pkeys are supported.
>
> MMU feature is used instead of CPU feature because this enables us
> to group MMU_FTR_KUAP and MMU_FTR_PKEY in asm feature fixup code.
Subject should probably be "Add MMU_FTR_PKEY".
> diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
> index f4ac25d4df05..72966d3d8f64 100644
> --- a/arch/powerpc/include/asm/mmu.h
> +++ b/arch/powerpc/include/asm/mmu.h
> @@ -23,6 +23,7 @@
>
> /* Radix page table supported and enabled */
> #define MMU_FTR_TYPE_RADIX ASM_CONST(0x00000040)
> +#define MMU_FTR_PKEY ASM_CONST(0x00000080)
It's not a type, so it should be with the individual feature bits below:
> /*
> * Individual features below.
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index 6a3bac357e24..6d70797352d8 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -815,6 +815,11 @@ void __init early_init_devtree(void *params)
> /* Now try to figure out if we are running on LPAR and so on */
> pseries_probe_fw_features();
>
> + /*
> + * Initialize pkey features and default AMR/IAMR values
> + */
> + pkey_early_init_devtree();
Not your fault, but the fact that we're having to do more and more
initialisation this early, based on the flat device tree, makes me
wonder if we need to rethink how we're doing the CPU/MMU feature setup.
> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
> index 0ff59acdbb84..bbba9c601e14 100644
> --- a/arch/powerpc/mm/book3s64/pkeys.c
> +++ b/arch/powerpc/mm/book3s64/pkeys.c
> @@ -38,38 +39,45 @@ static int execute_only_key = 2;
> #define PKEY_REG_BITS (sizeof(u64) * 8)
> #define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))
>
> +static int __init dt_scan_storage_keys(unsigned long node,
> + const char *uname, int depth,
> + void *data)
> +{
> + const char *type = of_get_flat_dt_prop(node, "device_type", NULL);
> + const __be32 *prop;
> + int pkeys_total;
> +
> + /* We are scanning "cpu" nodes only */
> + if (type == NULL || strcmp(type, "cpu") != 0)
> + return 0;
> +
> + prop = of_get_flat_dt_prop(node, "ibm,processor-storage-keys", NULL);
> + if (!prop)
> + return 0;
> + pkeys_total = be32_to_cpu(prop[0]);
> + return pkeys_total;
That's not really the way the return value is meant to be used for these
scanning functions.
The usual return values are 0 to keep scanning and !0 means we've found
the node we're looking for and we should stop scanning.
Doing it this way means if you find 0 pkeys it will keep scanning.
Instead you should pass &pkeys_total as the data pointer and set that.
> +}
> +
> static int scan_pkey_feature(void)
> {
> - u32 vals[2];
> - int pkeys_total = 0;
> - struct device_node *cpu;
> + int pkeys_total;
>
> /*
> * Pkey is not supported with Radix translation.
> */
> - if (radix_enabled())
> + if (early_radix_enabled())
> return 0;
>
> - cpu = of_find_node_by_type(NULL, "cpu");
> - if (!cpu)
> - return 0;
> + pkeys_total = of_scan_flat_dt(dt_scan_storage_keys, NULL);
> + if (pkeys_total == 0) {
>
> - if (of_property_read_u32_array(cpu,
> - "ibm,processor-storage-keys", vals, 2) == 0) {
> - /*
> - * Since any pkey can be used for data or execute, we will
> - * just treat all keys as equal and track them as one entity.
> - */
> - pkeys_total = vals[0];
> - /* Should we check for IAMR support FIXME!! */
???
> - } else {
This loses the ability to differentiate between no storage-keys property
at all vs a property that specifies zero keys, which I don't think is a
good change.
> /*
> * Let's assume 32 pkeys on P8 bare metal, if its not defined by device
> * tree. We make this exception since skiboot forgot to expose this
> * property on power8.
> */
> if (!firmware_has_feature(FW_FEATURE_LPAR) &&
> - cpu_has_feature(CPU_FTRS_POWER8))
> + early_cpu_has_feature(CPU_FTRS_POWER8))
> pkeys_total = 32;
That's not how cpu_has_feature() works, we'll need to fix that.
cheers
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v5 13/26] powerpc/book3s64/pkeys: Enable MMU_FTR_PKEY
2020-07-06 13:10 ` Michael Ellerman
@ 2020-07-06 14:09 ` Aneesh Kumar K.V
2020-07-06 17:17 ` Aneesh Kumar K.V
0 siblings, 1 reply; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-07-06 14:09 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: linuxram, bauerman
On 7/6/20 6:40 PM, Michael Ellerman wrote:
> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>> Parse storage keys related device tree entry in early_init_devtree
>> and enable MMU feature MMU_FTR_PKEY if pkeys are supported.
>>
>> MMU feature is used instead of CPU feature because this enables us
>> to group MMU_FTR_KUAP and MMU_FTR_PKEY in asm feature fixup code.
>
> Subject should probably be "Add MMU_FTR_PKEY".
>
>> diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
>> index f4ac25d4df05..72966d3d8f64 100644
>> --- a/arch/powerpc/include/asm/mmu.h
>> +++ b/arch/powerpc/include/asm/mmu.h
>> @@ -23,6 +23,7 @@
>>
>> /* Radix page table supported and enabled */
>> #define MMU_FTR_TYPE_RADIX ASM_CONST(0x00000040)
>> +#define MMU_FTR_PKEY ASM_CONST(0x00000080)
>
> It's not a type, so it should be with the individual feature bits below:
>
We don't have free bit in the other group. For now i will move this to
modified arch/powerpc/include/asm/mmu.h
@@ -23,12 +23,15 @@
/* Radix page table supported and enabled */
#define MMU_FTR_TYPE_RADIX ASM_CONST(0x00000040)
-#define MMU_FTR_PKEY ASM_CONST(0x00000080)
/*
* Individual features below.
*/
+/*
+ * Support for memory protection keys.
+ */
+#define MMU_FTR_PKEY ASM_CONST(0x00001000)
/*
* Support for 68 bit VA space. We added that from ISA 2.05
*/
>> /*
>> * Individual features below.
>
>> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>> index 6a3bac357e24..6d70797352d8 100644
>> --- a/arch/powerpc/kernel/prom.c
>> +++ b/arch/powerpc/kernel/prom.c
>> @@ -815,6 +815,11 @@ void __init early_init_devtree(void *params)
>> /* Now try to figure out if we are running on LPAR and so on */
>> pseries_probe_fw_features();
>>
>> + /*
>> + * Initialize pkey features and default AMR/IAMR values
>> + */
>> + pkey_early_init_devtree();
>
> Not your fault, but the fact that we're having to do more and more
> initialisation this early, based on the flat device tree, makes me
> wonder if we need to rethink how we're doing the CPU/MMU feature setup.
>
>> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
>> index 0ff59acdbb84..bbba9c601e14 100644
>> --- a/arch/powerpc/mm/book3s64/pkeys.c
>> +++ b/arch/powerpc/mm/book3s64/pkeys.c
>> @@ -38,38 +39,45 @@ static int execute_only_key = 2;
>> #define PKEY_REG_BITS (sizeof(u64) * 8)
>> #define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))
>>
>> +static int __init dt_scan_storage_keys(unsigned long node,
>> + const char *uname, int depth,
>> + void *data)
>> +{
>> + const char *type = of_get_flat_dt_prop(node, "device_type", NULL);
>> + const __be32 *prop;
>> + int pkeys_total;
>> +
>> + /* We are scanning "cpu" nodes only */
>> + if (type == NULL || strcmp(type, "cpu") != 0)
>> + return 0;
>> +
>> + prop = of_get_flat_dt_prop(node, "ibm,processor-storage-keys", NULL);
>> + if (!prop)
>> + return 0;
>> + pkeys_total = be32_to_cpu(prop[0]);
>> + return pkeys_total;
>
> That's not really the way the return value is meant to be used for these
> scanning functions.
>
> The usual return values are 0 to keep scanning and !0 means we've found
> the node we're looking for and we should stop scanning.
>
> Doing it this way means if you find 0 pkeys it will keep scanning.
>
> Instead you should pass &pkeys_total as the data pointer and set that.
>
>> +}
>> +
done
modified arch/powerpc/mm/book3s64/pkeys.c
@@ -52,7 +52,7 @@ static int __init dt_scan_storage_keys(unsigned long node,
{
const char *type = of_get_flat_dt_prop(node, "device_type", NULL);
const __be32 *prop;
- int pkeys_total;
+ int *pkeys_total = (int *) data;
/* We are scanning "cpu" nodes only */
if (type == NULL || strcmp(type, "cpu") != 0)
@@ -61,12 +61,13 @@ static int __init dt_scan_storage_keys(unsigned long
node,
prop = of_get_flat_dt_prop(node, "ibm,processor-storage-keys", NULL);
if (!prop)
return 0;
- pkeys_total = be32_to_cpu(prop[0]);
- return pkeys_total;
+ *pkeys_total = be32_to_cpu(prop[0]);
+ return 1;
}
static int scan_pkey_feature(void)
{
+ int ret;
int pkeys_total;
/*
@@ -75,8 +76,8 @@ static int scan_pkey_feature(void)
if (early_radix_enabled())
return 0;
- pkeys_total = of_scan_flat_dt(dt_scan_storage_keys, NULL);
- if (pkeys_total == 0) {
+ ret = of_scan_flat_dt(dt_scan_storage_keys, &pkeys_total);
+ if (ret == 0) {
/*
* Let's assume 32 pkeys on P8/P9 bare metal, if its not defined by
device
@@ -87,7 +88,6 @@ static int scan_pkey_feature(void)
(early_cpu_has_feature(CPU_FTR_ARCH_207S) ||
early_cpu_has_feature(CPU_FTR_ARCH_300)))
pkeys_total = 32;
-
}
/*
>> static int scan_pkey_feature(void)
>> {
>> - u32 vals[2];
>> - int pkeys_total = 0;
>> - struct device_node *cpu;
>> + int pkeys_total;
>>
>> /*
>> * Pkey is not supported with Radix translation.
>> */
>> - if (radix_enabled())
>> + if (early_radix_enabled())
>> return 0;
>>
>> - cpu = of_find_node_by_type(NULL, "cpu");
>> - if (!cpu)
>> - return 0;
>> + pkeys_total = of_scan_flat_dt(dt_scan_storage_keys, NULL);
>> + if (pkeys_total == 0) {
>>
>> - if (of_property_read_u32_array(cpu,
>> - "ibm,processor-storage-keys", vals, 2) == 0) {
>> - /*
>> - * Since any pkey can be used for data or execute, we will
>> - * just treat all keys as equal and track them as one entity.
>> - */
>> - pkeys_total = vals[0];
>> - /* Should we check for IAMR support FIXME!! */
>
> ???
The device tree allows us to have different count for both AMR and IAMR.
The current code skip that. I guess i added a comment in earlier patch
to check that whether we need to handle different AMR and IAMR counts.
The same comment get dropped here.
>
>> - } else {
>
> This loses the ability to differentiate between no storage-keys property
> at all vs a property that specifies zero keys, which I don't think is a
> good change.
>
>> /*
>> * Let's assume 32 pkeys on P8 bare metal, if its not defined by device
>> * tree. We make this exception since skiboot forgot to expose this
>> * property on power8.
>> */
>> if (!firmware_has_feature(FW_FEATURE_LPAR) &&
>> - cpu_has_feature(CPU_FTRS_POWER8))
>> + early_cpu_has_feature(CPU_FTRS_POWER8))
>> pkeys_total = 32;
>
> That's not how cpu_has_feature() works, we'll need to fix that.
>
> cheers
>
I did a separate patch to handle that which switch the above to
/*
* Let's assume 32 pkeys on P8/P9 bare metal, if its not defined by device
* tree. We make this exception since skiboot forgot to expose this
* property on power8/9.
*/
if (!firmware_has_feature(FW_FEATURE_LPAR) &&
(early_cpu_has_feature(CPU_FTR_ARCH_207S) ||
early_cpu_has_feature(CPU_FTR_ARCH_300)))
pkeys_total = 32;
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v5 13/26] powerpc/book3s64/pkeys: Enable MMU_FTR_PKEY
2020-07-06 14:09 ` Aneesh Kumar K.V
@ 2020-07-06 17:17 ` Aneesh Kumar K.V
2020-07-07 1:02 ` Michael Ellerman
0 siblings, 1 reply; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-07-06 17:17 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: linuxram, bauerman
>>
>>> /*
>>> * Let's assume 32 pkeys on P8 bare metal, if its not
>>> defined by device
>>> * tree. We make this exception since skiboot forgot to
>>> expose this
>>> * property on power8.
>>> */
>>> if (!firmware_has_feature(FW_FEATURE_LPAR) &&
>>> - cpu_has_feature(CPU_FTRS_POWER8))
>>> + early_cpu_has_feature(CPU_FTRS_POWER8))
>>> pkeys_total = 32;
>>
>> That's not how cpu_has_feature() works, we'll need to fix that.
>>
>> cheers
>>
>
> I did a separate patch to handle that which switch the above to
>
> /*
> * Let's assume 32 pkeys on P8/P9 bare metal, if its not
> defined by device
> * tree. We make this exception since skiboot forgot to expose
> this
> * property on power8/9.
> */
> if (!firmware_has_feature(FW_FEATURE_LPAR) &&
> (early_cpu_has_feature(CPU_FTR_ARCH_207S) ||
> early_cpu_has_feature(CPU_FTR_ARCH_300)))
> pkeys_total = 32;
>
We should do a PVR check here i guess.
ret = of_scan_flat_dt(dt_scan_storage_keys, &pkeys_total);
if (ret == 0) {
/*
* Let's assume 32 pkeys on P8/P9 bare metal, if its not defined by device
* tree. We make this exception since skiboot forgot to expose this
* property on power8/9.
*/
if (!firmware_has_feature(FW_FEATURE_LPAR) &&
(pvr_version_is(PVR_POWER8) || pvr_version_is(PVR_POWER9)))
pkeys_total = 32;
}
-aneesh
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v5 13/26] powerpc/book3s64/pkeys: Enable MMU_FTR_PKEY
2020-07-06 17:17 ` Aneesh Kumar K.V
@ 2020-07-07 1:02 ` Michael Ellerman
0 siblings, 0 replies; 49+ messages in thread
From: Michael Ellerman @ 2020-07-07 1:02 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev; +Cc: linuxram, bauerman
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>>>
>>>> /*
>>>> * Let's assume 32 pkeys on P8 bare metal, if its not
>>>> defined by device
>>>> * tree. We make this exception since skiboot forgot to
>>>> expose this
>>>> * property on power8.
>>>> */
>>>> if (!firmware_has_feature(FW_FEATURE_LPAR) &&
>>>> - cpu_has_feature(CPU_FTRS_POWER8))
>>>> + early_cpu_has_feature(CPU_FTRS_POWER8))
>>>> pkeys_total = 32;
>>>
>>> That's not how cpu_has_feature() works, we'll need to fix that.
>>>
>>> cheers
>>>
>>
>> I did a separate patch to handle that which switch the above to
>>
>> /*
>> * Let's assume 32 pkeys on P8/P9 bare metal, if its not
>> defined by device
>> * tree. We make this exception since skiboot forgot to expose
>> this
>> * property on power8/9.
>> */
>> if (!firmware_has_feature(FW_FEATURE_LPAR) &&
>> (early_cpu_has_feature(CPU_FTR_ARCH_207S) ||
>> early_cpu_has_feature(CPU_FTR_ARCH_300)))
>> pkeys_total = 32;
>>
>
> We should do a PVR check here i guess.
Yes, the ARCH features don't work because P10 will have both of those
enabled.
> ret = of_scan_flat_dt(dt_scan_storage_keys, &pkeys_total);
> if (ret == 0) {
>
> /*
> * Let's assume 32 pkeys on P8/P9 bare metal, if its not defined by device
> * tree. We make this exception since skiboot forgot to expose this
> * property on power8/9.
Well, it does expose it on Power9 after v6.6, but most P9 systems have
an older firmware than that.
And also the kernel has been enabling that on Power9 because of the
CPU_FTRS_POWER8 bug, so this is not actually a behaviour change.
> */
> if (!firmware_has_feature(FW_FEATURE_LPAR) &&
> (pvr_version_is(PVR_POWER8) || pvr_version_is(PVR_POWER9)))
> pkeys_total = 32;
> }
You need PVR_POWER8E and PVR_POWER8NVL as well.
cheers
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v5 14/26] powerpc/book3s64/kuep: Add MMU_FTR_KUEP
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (12 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 13/26] powerpc/book3s64/pkeys: Enable MMU_FTR_PKEY Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 15/26] powerpc/book3s64/pkeys: Use execute_pkey_disable static key Aneesh Kumar K.V
` (11 subsequent siblings)
25 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
This will be used to enable/disable Kernel Userspace Execution
Prevention (KUEP).
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/mmu.h | 5 +++++
arch/powerpc/mm/book3s64/radix_pgtable.c | 4 +++-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
index 72966d3d8f64..94435f85e3bc 100644
--- a/arch/powerpc/include/asm/mmu.h
+++ b/arch/powerpc/include/asm/mmu.h
@@ -24,6 +24,7 @@
/* Radix page table supported and enabled */
#define MMU_FTR_TYPE_RADIX ASM_CONST(0x00000040)
#define MMU_FTR_PKEY ASM_CONST(0x00000080)
+#define MMU_FTR_KUEP ASM_CONST(0x00000100)
/*
* Individual features below.
@@ -181,6 +182,10 @@ enum {
#ifdef CONFIG_PPC_MEM_KEYS
MMU_FTR_PKEY |
#endif
+#ifdef CONFIG_PPC_KUEP
+ MMU_FTR_KUEP |
+#endif /* CONFIG_PPC_KUAP */
+
0,
};
diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
index 8acb96de0e48..04fd749c6339 100644
--- a/arch/powerpc/mm/book3s64/radix_pgtable.c
+++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
@@ -520,8 +520,10 @@ void setup_kuep(bool disabled)
if (disabled || !early_radix_enabled())
return;
- if (smp_processor_id() == boot_cpuid)
+ if (smp_processor_id() == boot_cpuid) {
pr_info("Activating Kernel Userspace Execution Prevention\n");
+ cur_cpu_spec->mmu_features |= MMU_FTR_KUEP;
+ }
/*
* Radix always uses key0 of the IAMR to determine if an access is
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v5 15/26] powerpc/book3s64/pkeys: Use execute_pkey_disable static key
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (13 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 14/26] powerpc/book3s64/kuep: Add MMU_FTR_KUEP Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-07-06 7:20 ` Michael Ellerman
2020-06-19 13:58 ` [PATCH v5 16/26] powerpc/book3s64/pkeys: Use MMU_FTR_PKEY instead of pkey_disabled " Aneesh Kumar K.V
` (10 subsequent siblings)
25 siblings, 1 reply; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
Use execute_pkey_disabled static key to check for execute key support instead
of pkey_disabled.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/pkeys.h | 10 +---------
arch/powerpc/mm/book3s64/pkeys.c | 5 ++++-
2 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 47c81d41ea9a..09fbaa409ac4 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -126,15 +126,7 @@ static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
* Try to dedicate one of the protection keys to be used as an
* execute-only protection key.
*/
-extern int __execute_only_pkey(struct mm_struct *mm);
-static inline int execute_only_pkey(struct mm_struct *mm)
-{
- if (static_branch_likely(&pkey_disabled))
- return -1;
-
- return __execute_only_pkey(mm);
-}
-
+extern int execute_only_pkey(struct mm_struct *mm);
extern int __arch_override_mprotect_pkey(struct vm_area_struct *vma,
int prot, int pkey);
static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,
diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index bbba9c601e14..fed4f159011b 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c
@@ -345,8 +345,11 @@ void thread_pkey_regs_init(struct thread_struct *thread)
write_uamor(default_uamor);
}
-int __execute_only_pkey(struct mm_struct *mm)
+int execute_only_pkey(struct mm_struct *mm)
{
+ if (static_branch_likely(&execute_pkey_disabled))
+ return -1;
+
return mm->context.execute_only_pkey;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v5 15/26] powerpc/book3s64/pkeys: Use execute_pkey_disable static key
2020-06-19 13:58 ` [PATCH v5 15/26] powerpc/book3s64/pkeys: Use execute_pkey_disable static key Aneesh Kumar K.V
@ 2020-07-06 7:20 ` Michael Ellerman
2020-07-06 8:49 ` Aneesh Kumar K.V
0 siblings, 1 reply; 49+ messages in thread
From: Michael Ellerman @ 2020-07-06 7:20 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev; +Cc: Aneesh Kumar K.V, linuxram, bauerman
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> Use execute_pkey_disabled static key to check for execute key support instead
> of pkey_disabled.
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
> arch/powerpc/include/asm/pkeys.h | 10 +---------
> arch/powerpc/mm/book3s64/pkeys.c | 5 ++++-
> 2 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> index 47c81d41ea9a..09fbaa409ac4 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -126,15 +126,7 @@ static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
> * Try to dedicate one of the protection keys to be used as an
> * execute-only protection key.
> */
> -extern int __execute_only_pkey(struct mm_struct *mm);
> -static inline int execute_only_pkey(struct mm_struct *mm)
> -{
> - if (static_branch_likely(&pkey_disabled))
> - return -1;
> -
> - return __execute_only_pkey(mm);
> -}
> -
> +extern int execute_only_pkey(struct mm_struct *mm);
> extern int __arch_override_mprotect_pkey(struct vm_area_struct *vma,
> int prot, int pkey);
> static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,
> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
> index bbba9c601e14..fed4f159011b 100644
> --- a/arch/powerpc/mm/book3s64/pkeys.c
> +++ b/arch/powerpc/mm/book3s64/pkeys.c
> @@ -345,8 +345,11 @@ void thread_pkey_regs_init(struct thread_struct *thread)
> write_uamor(default_uamor);
> }
>
> -int __execute_only_pkey(struct mm_struct *mm)
> +int execute_only_pkey(struct mm_struct *mm)
> {
> + if (static_branch_likely(&execute_pkey_disabled))
> + return -1;
> +
> return mm->context.execute_only_pkey;
> }
That adds the overhead of a function call, but then uses a static_key to
avoid an easy to predict branch, which seems like a bad tradeoff. And
it's not a performance critical path AFAICS.
Anyway this seems unnecessary:
pkey_early_init_devtree()
{
...
if (unlikely(max_pkey <= execute_only_key)) {
/*
* Insufficient number of keys to support
* execute only key. Mark it unavailable.
*/
execute_only_key = -1;
void pkey_mm_init(struct mm_struct *mm)
{
...
mm->context.execute_only_pkey = execute_only_key;
}
ie. Can't it just be:
static inline int execute_only_pkey(struct mm_struct *mm)
{
return mm->context.execute_only_pkey;
}
cheers
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v5 15/26] powerpc/book3s64/pkeys: Use execute_pkey_disable static key
2020-07-06 7:20 ` Michael Ellerman
@ 2020-07-06 8:49 ` Aneesh Kumar K.V
0 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-07-06 8:49 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: linuxram, bauerman
On 7/6/20 12:50 PM, Michael Ellerman wrote:
> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>> Use execute_pkey_disabled static key to check for execute key support instead
>> of pkey_disabled.
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>> arch/powerpc/include/asm/pkeys.h | 10 +---------
>> arch/powerpc/mm/book3s64/pkeys.c | 5 ++++-
>> 2 files changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
>> index 47c81d41ea9a..09fbaa409ac4 100644
>> --- a/arch/powerpc/include/asm/pkeys.h
>> +++ b/arch/powerpc/include/asm/pkeys.h
>> @@ -126,15 +126,7 @@ static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
>> * Try to dedicate one of the protection keys to be used as an
>> * execute-only protection key.
>> */
>> -extern int __execute_only_pkey(struct mm_struct *mm);
>> -static inline int execute_only_pkey(struct mm_struct *mm)
>> -{
>> - if (static_branch_likely(&pkey_disabled))
>> - return -1;
>> -
>> - return __execute_only_pkey(mm);
>> -}
>> -
>> +extern int execute_only_pkey(struct mm_struct *mm);
>> extern int __arch_override_mprotect_pkey(struct vm_area_struct *vma,
>> int prot, int pkey);
>> static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,
>> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
>> index bbba9c601e14..fed4f159011b 100644
>> --- a/arch/powerpc/mm/book3s64/pkeys.c
>> +++ b/arch/powerpc/mm/book3s64/pkeys.c
>> @@ -345,8 +345,11 @@ void thread_pkey_regs_init(struct thread_struct *thread)
>> write_uamor(default_uamor);
>> }
>>
>> -int __execute_only_pkey(struct mm_struct *mm)
>> +int execute_only_pkey(struct mm_struct *mm)
>> {
>> + if (static_branch_likely(&execute_pkey_disabled))
>> + return -1;
>> +
>> return mm->context.execute_only_pkey;
>> }
>
> That adds the overhead of a function call, but then uses a static_key to
> avoid an easy to predict branch, which seems like a bad tradeoff. And
> it's not a performance critical path AFAICS.
>
> Anyway this seems unnecessary:
>
> pkey_early_init_devtree()
> {
> ...
> if (unlikely(max_pkey <= execute_only_key)) {
> /*
> * Insufficient number of keys to support
> * execute only key. Mark it unavailable.
> */
> execute_only_key = -1;
>
> void pkey_mm_init(struct mm_struct *mm)
> {
> ...
> mm->context.execute_only_pkey = execute_only_key;
> }
>
>
> ie. Can't it just be:
>
> static inline int execute_only_pkey(struct mm_struct *mm)
> {
> return mm->context.execute_only_pkey;
> }
>
ok updated with
modified arch/powerpc/mm/book3s64/pkeys.c
@@ -151,7 +151,7 @@ void __init pkey_early_init_devtree(void)
max_pkey = pkeys_total;
#endif
- if (unlikely(max_pkey <= execute_only_key)) {
+ if (unlikely(max_pkey <= execute_only_key) ||
!pkey_execute_disable_supported) {
/*
* Insufficient number of keys to support
* execute only key. Mark it unavailable.
@@ -368,9 +368,6 @@ int __arch_set_user_pkey_access(struct task_struct
*tsk, int pkey,
int execute_only_pkey(struct mm_struct *mm)
{
- if (static_branch_likely(&execute_pkey_disabled))
- return -1;
-
return mm->context.execute_only_pkey;
}
-aneesh
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v5 16/26] powerpc/book3s64/pkeys: Use MMU_FTR_PKEY instead of pkey_disabled static key
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (14 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 15/26] powerpc/book3s64/pkeys: Use execute_pkey_disable static key Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 17/26] powerpc/book3s64/keys: Print information during boot Aneesh Kumar K.V
` (9 subsequent siblings)
25 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
Instead of pkey_disabled static key use mmu feature MMU_FTR_PKEY.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/book3s/64/pkeys.h | 2 +-
arch/powerpc/include/asm/pkeys.h | 14 ++++++--------
arch/powerpc/mm/book3s64/pkeys.c | 16 +++++++---------
3 files changed, 14 insertions(+), 18 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/pkeys.h b/arch/powerpc/include/asm/book3s/64/pkeys.h
index 8174662a9173..5b178139f3c0 100644
--- a/arch/powerpc/include/asm/book3s/64/pkeys.h
+++ b/arch/powerpc/include/asm/book3s/64/pkeys.h
@@ -7,7 +7,7 @@
static inline u64 vmflag_to_pte_pkey_bits(u64 vm_flags)
{
- if (static_branch_likely(&pkey_disabled))
+ if (!mmu_has_feature(MMU_FTR_PKEY))
return 0x0UL;
if (radix_enabled())
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 09fbaa409ac4..b1d448c53209 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -11,7 +11,6 @@
#include <linux/jump_label.h>
#include <asm/firmware.h>
-DECLARE_STATIC_KEY_FALSE(pkey_disabled);
extern int max_pkey;
extern u32 reserved_allocation_mask; /* bits set for reserved keys */
@@ -38,7 +37,7 @@ static inline u64 pkey_to_vmflag_bits(u16 pkey)
static inline int vma_pkey(struct vm_area_struct *vma)
{
- if (static_branch_likely(&pkey_disabled))
+ if (!mmu_has_feature(MMU_FTR_PKEY))
return 0;
return (vma->vm_flags & ARCH_VM_PKEY_FLAGS) >> VM_PKEY_SHIFT;
}
@@ -93,9 +92,8 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
u32 all_pkeys_mask = (u32)(~(0x0));
int ret;
- if (static_branch_likely(&pkey_disabled))
+ if (!mmu_has_feature(MMU_FTR_PKEY))
return -1;
-
/*
* Are we out of pkeys? We must handle this specially because ffz()
* behavior is undefined if there are no zeros.
@@ -111,7 +109,7 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
{
- if (static_branch_likely(&pkey_disabled))
+ if (!mmu_has_feature(MMU_FTR_PKEY))
return -1;
if (!mm_pkey_is_allocated(mm, pkey))
@@ -132,7 +130,7 @@ extern int __arch_override_mprotect_pkey(struct vm_area_struct *vma,
static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,
int prot, int pkey)
{
- if (static_branch_likely(&pkey_disabled))
+ if (!mmu_has_feature(MMU_FTR_PKEY))
return 0;
/*
@@ -150,7 +148,7 @@ extern int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
unsigned long init_val)
{
- if (static_branch_likely(&pkey_disabled))
+ if (!mmu_has_feature(MMU_FTR_PKEY))
return -EINVAL;
/*
@@ -167,7 +165,7 @@ static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
static inline bool arch_pkeys_enabled(void)
{
- return !static_branch_likely(&pkey_disabled);
+ return mmu_has_feature(MMU_FTR_PKEY);
}
extern void pkey_mm_init(struct mm_struct *mm);
diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index fed4f159011b..810118123e70 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c
@@ -13,7 +13,6 @@
#include <linux/of_fdt.h>
-DEFINE_STATIC_KEY_FALSE(pkey_disabled);
DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);
int max_pkey; /* Maximum key value supported */
/*
@@ -114,7 +113,6 @@ void __init pkey_early_init_devtree(void)
pkeys_total = scan_pkey_feature();
if (!pkeys_total) {
/* No support for pkey. Mark it disabled */
- static_branch_enable(&pkey_disabled);
return;
}
@@ -202,7 +200,7 @@ void __init pkey_early_init_devtree(void)
void pkey_mm_init(struct mm_struct *mm)
{
- if (static_branch_likely(&pkey_disabled))
+ if (!mmu_has_feature(MMU_FTR_PKEY))
return;
mm_pkey_allocation_map(mm) = initial_allocation_mask;
mm->context.execute_only_pkey = execute_only_key;
@@ -306,7 +304,7 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
void thread_pkey_regs_save(struct thread_struct *thread)
{
- if (static_branch_likely(&pkey_disabled))
+ if (!mmu_has_feature(MMU_FTR_PKEY))
return;
/*
@@ -320,7 +318,7 @@ void thread_pkey_regs_save(struct thread_struct *thread)
void thread_pkey_regs_restore(struct thread_struct *new_thread,
struct thread_struct *old_thread)
{
- if (static_branch_likely(&pkey_disabled))
+ if (!mmu_has_feature(MMU_FTR_PKEY))
return;
if (old_thread->amr != new_thread->amr)
@@ -333,7 +331,7 @@ void thread_pkey_regs_restore(struct thread_struct *new_thread,
void thread_pkey_regs_init(struct thread_struct *thread)
{
- if (static_branch_likely(&pkey_disabled))
+ if (!mmu_has_feature(MMU_FTR_PKEY))
return;
thread->amr = default_amr;
@@ -408,7 +406,7 @@ static bool pkey_access_permitted(int pkey, bool write, bool execute)
bool arch_pte_access_permitted(u64 pte, bool write, bool execute)
{
- if (static_branch_likely(&pkey_disabled))
+ if (!mmu_has_feature(MMU_FTR_PKEY))
return true;
return pkey_access_permitted(pte_to_pkey_bits(pte), write, execute);
@@ -425,7 +423,7 @@ bool arch_pte_access_permitted(u64 pte, bool write, bool execute)
bool arch_vma_access_permitted(struct vm_area_struct *vma, bool write,
bool execute, bool foreign)
{
- if (static_branch_likely(&pkey_disabled))
+ if (!mmu_has_feature(MMU_FTR_PKEY))
return true;
/*
* Do not enforce our key-permissions on a foreign vma.
@@ -438,7 +436,7 @@ bool arch_vma_access_permitted(struct vm_area_struct *vma, bool write,
void arch_dup_pkeys(struct mm_struct *oldmm, struct mm_struct *mm)
{
- if (static_branch_likely(&pkey_disabled))
+ if (!mmu_has_feature(MMU_FTR_PKEY))
return;
/* Duplicate the oldmm pkey state in mm: */
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v5 17/26] powerpc/book3s64/keys: Print information during boot.
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (15 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 16/26] powerpc/book3s64/pkeys: Use MMU_FTR_PKEY instead of pkey_disabled " Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-07-06 7:52 ` Michael Ellerman
2020-06-19 13:58 ` [PATCH v5 18/26] powerpc/book3s64/keys/kuap: Reset AMR/IAMR values on kexec Aneesh Kumar K.V
` (8 subsequent siblings)
25 siblings, 1 reply; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/mm/book3s64/pkeys.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index 810118123e70..0d72c0246052 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c
@@ -195,6 +195,7 @@ void __init pkey_early_init_devtree(void)
*/
initial_allocation_mask |= reserved_allocation_mask;
+ pr_info("Enabling Memory keys with max key count %d", max_pkey);
return;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v5 17/26] powerpc/book3s64/keys: Print information during boot.
2020-06-19 13:58 ` [PATCH v5 17/26] powerpc/book3s64/keys: Print information during boot Aneesh Kumar K.V
@ 2020-07-06 7:52 ` Michael Ellerman
0 siblings, 0 replies; 49+ messages in thread
From: Michael Ellerman @ 2020-07-06 7:52 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev; +Cc: Aneesh Kumar K.V, linuxram, bauerman
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
> arch/powerpc/mm/book3s64/pkeys.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
> index 810118123e70..0d72c0246052 100644
> --- a/arch/powerpc/mm/book3s64/pkeys.c
> +++ b/arch/powerpc/mm/book3s64/pkeys.c
> @@ -195,6 +195,7 @@ void __init pkey_early_init_devtree(void)
> */
> initial_allocation_mask |= reserved_allocation_mask;
>
> + pr_info("Enabling Memory keys with max key count %d", max_pkey);
^
missing newline
> return;
> }
The syscall is called pkey_mprotect() and the manpage is "pkeys", so I
think it would make sense to use "pkey" in the message.
cheers
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v5 18/26] powerpc/book3s64/keys/kuap: Reset AMR/IAMR values on kexec
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (16 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 17/26] powerpc/book3s64/keys: Print information during boot Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-07-06 12:29 ` Michael Ellerman
2020-06-19 13:58 ` [PATCH v5 19/26] powerpc/book3s64/kuap: Move KUAP related function outside radix Aneesh Kumar K.V
` (7 subsequent siblings)
25 siblings, 1 reply; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
As we kexec across kernels that use AMR/IAMR for different purposes
we need to ensure that new kernels get kexec'd with a reset value
of AMR/IAMR. For ex: the new kernel can use key 0 for kernel mapping and the old
AMR value prevents access to key 0.
This patch also removes reset if IAMR and AMOR in kexec_sequence. Reset of AMOR
is not needed and the IAMR reset is partial (it doesn't do the reset
on secondary cpus) and is redundant with this patch.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
.../powerpc/include/asm/book3s/64/kup-radix.h | 20 +++++++++++++++++++
arch/powerpc/include/asm/kup.h | 14 +++++++++++++
arch/powerpc/kernel/misc_64.S | 14 -------------
arch/powerpc/kexec/core_64.c | 3 +++
arch/powerpc/mm/book3s64/pgtable.c | 3 +++
5 files changed, 40 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
index 3ee1ec60be84..c57063c35833 100644
--- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
+++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
@@ -180,6 +180,26 @@ static inline unsigned long kuap_get_and_check_amr(void)
}
#endif /* CONFIG_PPC_KUAP */
+#define reset_kuap reset_kuap
+static inline void reset_kuap(void)
+{
+ if (mmu_has_feature(MMU_FTR_RADIX_KUAP)) {
+ mtspr(SPRN_AMR, 0);
+ /* Do we need isync()? We are going via a kexec reset */
+ isync();
+ }
+}
+
+#define reset_kuep reset_kuep
+static inline void reset_kuep(void)
+{
+ if (mmu_has_feature(MMU_FTR_KUEP)) {
+ mtspr(SPRN_IAMR, 0);
+ /* Do we need isync()? We are going via a kexec reset */
+ isync();
+ }
+}
+
#endif /* __ASSEMBLY__ */
#endif /* _ASM_POWERPC_BOOK3S_64_KUP_RADIX_H */
diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
index c745ee41ad66..4dc23a706910 100644
--- a/arch/powerpc/include/asm/kup.h
+++ b/arch/powerpc/include/asm/kup.h
@@ -113,6 +113,20 @@ static inline void prevent_current_write_to_user(void)
prevent_user_access(NULL, NULL, ~0UL, KUAP_CURRENT_WRITE);
}
+#ifndef reset_kuap
+#define reset_kuap reset_kuap
+static inline void reset_kuap(void)
+{
+}
+#endif
+
+#ifndef reset_kuep
+#define reset_kuep reset_kuep
+static inline void reset_kuep(void)
+{
+}
+#endif
+
#endif /* !__ASSEMBLY__ */
#endif /* _ASM_POWERPC_KUAP_H_ */
diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
index 1864605eca29..7bb46ad98207 100644
--- a/arch/powerpc/kernel/misc_64.S
+++ b/arch/powerpc/kernel/misc_64.S
@@ -413,20 +413,6 @@ _GLOBAL(kexec_sequence)
li r0,0
std r0,16(r1)
-BEGIN_FTR_SECTION
- /*
- * This is the best time to turn AMR/IAMR off.
- * key 0 is used in radix for supervisor<->user
- * protection, but on hash key 0 is reserved
- * ideally we want to enter with a clean state.
- * NOTE, we rely on r0 being 0 from above.
- */
- mtspr SPRN_IAMR,r0
-BEGIN_FTR_SECTION_NESTED(42)
- mtspr SPRN_AMOR,r0
-END_FTR_SECTION_NESTED_IFSET(CPU_FTR_HVMODE, 42)
-END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
-
/* save regs for local vars on new stack.
* yes, we won't go back, but ...
*/
diff --git a/arch/powerpc/kexec/core_64.c b/arch/powerpc/kexec/core_64.c
index b4184092172a..a124715f33ea 100644
--- a/arch/powerpc/kexec/core_64.c
+++ b/arch/powerpc/kexec/core_64.c
@@ -152,6 +152,9 @@ static void kexec_smp_down(void *arg)
if (ppc_md.kexec_cpu_down)
ppc_md.kexec_cpu_down(0, 1);
+ reset_kuap();
+ reset_kuep();
+
kexec_smp_wait();
/* NOTREACHED */
}
diff --git a/arch/powerpc/mm/book3s64/pgtable.c b/arch/powerpc/mm/book3s64/pgtable.c
index c58ad1049909..9673f4b74c9a 100644
--- a/arch/powerpc/mm/book3s64/pgtable.c
+++ b/arch/powerpc/mm/book3s64/pgtable.c
@@ -165,6 +165,9 @@ void mmu_cleanup_all(void)
radix__mmu_cleanup_all();
else if (mmu_hash_ops.hpte_clear_all)
mmu_hash_ops.hpte_clear_all();
+
+ reset_kuap();
+ reset_kuep();
}
#ifdef CONFIG_MEMORY_HOTPLUG
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v5 18/26] powerpc/book3s64/keys/kuap: Reset AMR/IAMR values on kexec
2020-06-19 13:58 ` [PATCH v5 18/26] powerpc/book3s64/keys/kuap: Reset AMR/IAMR values on kexec Aneesh Kumar K.V
@ 2020-07-06 12:29 ` Michael Ellerman
2020-07-06 14:39 ` Aneesh Kumar K.V
0 siblings, 1 reply; 49+ messages in thread
From: Michael Ellerman @ 2020-07-06 12:29 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev; +Cc: Aneesh Kumar K.V, linuxram, bauerman
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> As we kexec across kernels that use AMR/IAMR for different purposes
> we need to ensure that new kernels get kexec'd with a reset value
> of AMR/IAMR. For ex: the new kernel can use key 0 for kernel mapping and the old
> AMR value prevents access to key 0.
>
> This patch also removes reset if IAMR and AMOR in kexec_sequence. Reset of AMOR
> is not needed and the IAMR reset is partial (it doesn't do the reset
> on secondary cpus) and is redundant with this patch.
I like the idea of cleaning this stuff up.
But I think tying it into kup/kuep/kuap and the MMU features and ifdefs
and so on is overly complicated.
We should just have eg:
void reset_sprs(void)
{
if (early_cpu_has_feature(CPU_FTR_ARCH_206)) {
mtspr(SPRN_AMR, 0);
mtspr(SPRN_UAMOR, 0);
}
if (early_cpu_has_feature(CPU_FTR_ARCH_207S)) {
mtspr(SPRN_IAMR, 0);
}
}
And call that from the kexec paths.
cheers
> diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
> index 3ee1ec60be84..c57063c35833 100644
> --- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
> +++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
> @@ -180,6 +180,26 @@ static inline unsigned long kuap_get_and_check_amr(void)
> }
> #endif /* CONFIG_PPC_KUAP */
>
> +#define reset_kuap reset_kuap
> +static inline void reset_kuap(void)
> +{
> + if (mmu_has_feature(MMU_FTR_RADIX_KUAP)) {
> + mtspr(SPRN_AMR, 0);
> + /* Do we need isync()? We are going via a kexec reset */
> + isync();
> + }
> +}
> +
> +#define reset_kuep reset_kuep
> +static inline void reset_kuep(void)
> +{
> + if (mmu_has_feature(MMU_FTR_KUEP)) {
> + mtspr(SPRN_IAMR, 0);
> + /* Do we need isync()? We are going via a kexec reset */
> + isync();
> + }
> +}
> +
> #endif /* __ASSEMBLY__ */
>
> #endif /* _ASM_POWERPC_BOOK3S_64_KUP_RADIX_H */
> diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
> index c745ee41ad66..4dc23a706910 100644
> --- a/arch/powerpc/include/asm/kup.h
> +++ b/arch/powerpc/include/asm/kup.h
> @@ -113,6 +113,20 @@ static inline void prevent_current_write_to_user(void)
> prevent_user_access(NULL, NULL, ~0UL, KUAP_CURRENT_WRITE);
> }
>
> +#ifndef reset_kuap
> +#define reset_kuap reset_kuap
> +static inline void reset_kuap(void)
> +{
> +}
> +#endif
> +
> +#ifndef reset_kuep
> +#define reset_kuep reset_kuep
> +static inline void reset_kuep(void)
> +{
> +}
> +#endif
> +
> #endif /* !__ASSEMBLY__ */
>
> #endif /* _ASM_POWERPC_KUAP_H_ */
> diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
> index 1864605eca29..7bb46ad98207 100644
> --- a/arch/powerpc/kernel/misc_64.S
> +++ b/arch/powerpc/kernel/misc_64.S
> @@ -413,20 +413,6 @@ _GLOBAL(kexec_sequence)
> li r0,0
> std r0,16(r1)
>
> -BEGIN_FTR_SECTION
> - /*
> - * This is the best time to turn AMR/IAMR off.
> - * key 0 is used in radix for supervisor<->user
> - * protection, but on hash key 0 is reserved
> - * ideally we want to enter with a clean state.
> - * NOTE, we rely on r0 being 0 from above.
> - */
> - mtspr SPRN_IAMR,r0
> -BEGIN_FTR_SECTION_NESTED(42)
> - mtspr SPRN_AMOR,r0
> -END_FTR_SECTION_NESTED_IFSET(CPU_FTR_HVMODE, 42)
> -END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
> -
> /* save regs for local vars on new stack.
> * yes, we won't go back, but ...
> */
> diff --git a/arch/powerpc/kexec/core_64.c b/arch/powerpc/kexec/core_64.c
> index b4184092172a..a124715f33ea 100644
> --- a/arch/powerpc/kexec/core_64.c
> +++ b/arch/powerpc/kexec/core_64.c
> @@ -152,6 +152,9 @@ static void kexec_smp_down(void *arg)
> if (ppc_md.kexec_cpu_down)
> ppc_md.kexec_cpu_down(0, 1);
>
> + reset_kuap();
> + reset_kuep();
> +
> kexec_smp_wait();
> /* NOTREACHED */
> }
> diff --git a/arch/powerpc/mm/book3s64/pgtable.c b/arch/powerpc/mm/book3s64/pgtable.c
> index c58ad1049909..9673f4b74c9a 100644
> --- a/arch/powerpc/mm/book3s64/pgtable.c
> +++ b/arch/powerpc/mm/book3s64/pgtable.c
> @@ -165,6 +165,9 @@ void mmu_cleanup_all(void)
> radix__mmu_cleanup_all();
> else if (mmu_hash_ops.hpte_clear_all)
> mmu_hash_ops.hpte_clear_all();
> +
> + reset_kuap();
> + reset_kuep();
> }
>
> #ifdef CONFIG_MEMORY_HOTPLUG
> --
> 2.26.2
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v5 18/26] powerpc/book3s64/keys/kuap: Reset AMR/IAMR values on kexec
2020-07-06 12:29 ` Michael Ellerman
@ 2020-07-06 14:39 ` Aneesh Kumar K.V
2020-07-07 1:07 ` Michael Ellerman
0 siblings, 1 reply; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-07-06 14:39 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: linuxram, bauerman
On 7/6/20 5:59 PM, Michael Ellerman wrote:
> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>> As we kexec across kernels that use AMR/IAMR for different purposes
>> we need to ensure that new kernels get kexec'd with a reset value
>> of AMR/IAMR. For ex: the new kernel can use key 0 for kernel mapping and the old
>> AMR value prevents access to key 0.
>>
>> This patch also removes reset if IAMR and AMOR in kexec_sequence. Reset of AMOR
>> is not needed and the IAMR reset is partial (it doesn't do the reset
>> on secondary cpus) and is redundant with this patch.
>
> I like the idea of cleaning this stuff up.
>
> But I think tying it into kup/kuep/kuap and the MMU features and ifdefs
> and so on is overly complicated.
>
> We should just have eg:
>
> void reset_sprs(void)
> {
> if (early_cpu_has_feature(CPU_FTR_ARCH_206)) {
> mtspr(SPRN_AMR, 0);
> mtspr(SPRN_UAMOR, 0);
> }
>
> if (early_cpu_has_feature(CPU_FTR_ARCH_207S)) {
> mtspr(SPRN_IAMR, 0);
> }
> }
>
> And call that from the kexec paths.
>
Will fix. Should that be early_cpu_has_feature()? cpu_has_feature()
should work there right?
-aneesh
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v5 18/26] powerpc/book3s64/keys/kuap: Reset AMR/IAMR values on kexec
2020-07-06 14:39 ` Aneesh Kumar K.V
@ 2020-07-07 1:07 ` Michael Ellerman
0 siblings, 0 replies; 49+ messages in thread
From: Michael Ellerman @ 2020-07-07 1:07 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev; +Cc: linuxram, bauerman
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> On 7/6/20 5:59 PM, Michael Ellerman wrote:
>> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>>> As we kexec across kernels that use AMR/IAMR for different purposes
>>> we need to ensure that new kernels get kexec'd with a reset value
>>> of AMR/IAMR. For ex: the new kernel can use key 0 for kernel mapping and the old
>>> AMR value prevents access to key 0.
>>>
>>> This patch also removes reset if IAMR and AMOR in kexec_sequence. Reset of AMOR
>>> is not needed and the IAMR reset is partial (it doesn't do the reset
>>> on secondary cpus) and is redundant with this patch.
>>
>> I like the idea of cleaning this stuff up.
>>
>> But I think tying it into kup/kuep/kuap and the MMU features and ifdefs
>> and so on is overly complicated.
>>
>> We should just have eg:
>>
>> void reset_sprs(void)
>> {
>> if (early_cpu_has_feature(CPU_FTR_ARCH_206)) {
>> mtspr(SPRN_AMR, 0);
>> mtspr(SPRN_UAMOR, 0);
>> }
>>
>> if (early_cpu_has_feature(CPU_FTR_ARCH_207S)) {
>> mtspr(SPRN_IAMR, 0);
>> }
>> }
>>
>> And call that from the kexec paths.
>
> Will fix. Should that be early_cpu_has_feature()? cpu_has_feature()
> should work there right?
Yeah I guess. I was thinking if we crashed before code patching was
done, but if that happens we can't kdump anyway. So I'm probably being
over paranoid.
cheers
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v5 19/26] powerpc/book3s64/kuap: Move KUAP related function outside radix
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (17 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 18/26] powerpc/book3s64/keys/kuap: Reset AMR/IAMR values on kexec Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-07-06 12:41 ` Michael Ellerman
2020-06-19 13:58 ` [PATCH v5 20/26] powerpc/book3s64/kuep: Move KUEP " Aneesh Kumar K.V
` (6 subsequent siblings)
25 siblings, 1 reply; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
The next set of patches adds support for kuap with hash translation.
In preparation for that rename/move kuap related functions to
non radix names.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
.../asm/book3s/64/{kup-radix.h => kup.h} | 6 +++---
arch/powerpc/include/asm/kup.h | 2 +-
arch/powerpc/kernel/syscall_64.c | 2 +-
arch/powerpc/mm/book3s64/pkeys.c | 19 +++++++++++++++++++
arch/powerpc/mm/book3s64/radix_pgtable.c | 18 ------------------
5 files changed, 24 insertions(+), 23 deletions(-)
rename arch/powerpc/include/asm/book3s/64/{kup-radix.h => kup.h} (97%)
diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup.h
similarity index 97%
rename from arch/powerpc/include/asm/book3s/64/kup-radix.h
rename to arch/powerpc/include/asm/book3s/64/kup.h
index c57063c35833..54e237c093da 100644
--- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
+++ b/arch/powerpc/include/asm/book3s/64/kup.h
@@ -1,6 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef _ASM_POWERPC_BOOK3S_64_KUP_RADIX_H
-#define _ASM_POWERPC_BOOK3S_64_KUP_RADIX_H
+#ifndef _ASM_POWERPC_BOOK3S_64_KUP_H
+#define _ASM_POWERPC_BOOK3S_64_KUP_H
#include <linux/const.h>
#include <asm/reg.h>
@@ -202,4 +202,4 @@ static inline void reset_kuep(void)
#endif /* __ASSEMBLY__ */
-#endif /* _ASM_POWERPC_BOOK3S_64_KUP_RADIX_H */
+#endif /* _ASM_POWERPC_BOOK3S_64_KUP_H */
diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
index 4dc23a706910..593707e112cc 100644
--- a/arch/powerpc/include/asm/kup.h
+++ b/arch/powerpc/include/asm/kup.h
@@ -15,7 +15,7 @@
#define KUAP_CURRENT (KUAP_CURRENT_READ | KUAP_CURRENT_WRITE)
#ifdef CONFIG_PPC64
-#include <asm/book3s/64/kup-radix.h>
+#include <asm/book3s/64/kup.h>
#endif
#ifdef CONFIG_PPC_8xx
#include <asm/nohash/32/kup-8xx.h>
diff --git a/arch/powerpc/kernel/syscall_64.c b/arch/powerpc/kernel/syscall_64.c
index 79edba3ab312..7e560a01afa4 100644
--- a/arch/powerpc/kernel/syscall_64.c
+++ b/arch/powerpc/kernel/syscall_64.c
@@ -2,7 +2,7 @@
#include <linux/err.h>
#include <asm/asm-prototypes.h>
-#include <asm/book3s/64/kup-radix.h>
+#include <asm/book3s/64/kup.h>
#include <asm/cputime.h>
#include <asm/hw_irq.h>
#include <asm/kprobes.h>
diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index 0d72c0246052..e93b65a0e6e7 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c
@@ -12,6 +12,7 @@
#include <linux/pkeys.h>
#include <linux/of_fdt.h>
+#include <asm/smp.h>
DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);
int max_pkey; /* Maximum key value supported */
@@ -199,6 +200,24 @@ void __init pkey_early_init_devtree(void)
return;
}
+#ifdef CONFIG_PPC_KUAP
+void __init setup_kuap(bool disabled)
+{
+ if (disabled || !early_radix_enabled())
+ return;
+
+ if (smp_processor_id() == boot_cpuid) {
+ pr_info("Activating Kernel Userspace Access Prevention\n");
+ cur_cpu_spec->mmu_features |= MMU_FTR_RADIX_KUAP;
+ }
+
+ /* Make sure userspace can't change the AMR */
+ mtspr(SPRN_UAMOR, 0);
+ mtspr(SPRN_AMR, AMR_KUAP_BLOCKED);
+ isync();
+}
+#endif
+
void pkey_mm_init(struct mm_struct *mm)
{
if (!mmu_has_feature(MMU_FTR_PKEY))
diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
index 04fd749c6339..3959b7d4ad3c 100644
--- a/arch/powerpc/mm/book3s64/radix_pgtable.c
+++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
@@ -534,24 +534,6 @@ void setup_kuep(bool disabled)
}
#endif
-#ifdef CONFIG_PPC_KUAP
-void setup_kuap(bool disabled)
-{
- if (disabled || !early_radix_enabled())
- return;
-
- if (smp_processor_id() == boot_cpuid) {
- pr_info("Activating Kernel Userspace Access Prevention\n");
- cur_cpu_spec->mmu_features |= MMU_FTR_RADIX_KUAP;
- }
-
- /* Make sure userspace can't change the AMR */
- mtspr(SPRN_UAMOR, 0);
- mtspr(SPRN_AMR, AMR_KUAP_BLOCKED);
- isync();
-}
-#endif
-
void __init radix__early_init_mmu(void)
{
unsigned long lpcr;
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v5 19/26] powerpc/book3s64/kuap: Move KUAP related function outside radix
2020-06-19 13:58 ` [PATCH v5 19/26] powerpc/book3s64/kuap: Move KUAP related function outside radix Aneesh Kumar K.V
@ 2020-07-06 12:41 ` Michael Ellerman
2020-07-06 14:41 ` Aneesh Kumar K.V
0 siblings, 1 reply; 49+ messages in thread
From: Michael Ellerman @ 2020-07-06 12:41 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev; +Cc: Aneesh Kumar K.V, linuxram, bauerman
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> The next set of patches adds support for kuap with hash translation.
That's no longer true of this series.
> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
> index 0d72c0246052..e93b65a0e6e7 100644
> --- a/arch/powerpc/mm/book3s64/pkeys.c
> +++ b/arch/powerpc/mm/book3s64/pkeys.c
> @@ -199,6 +200,24 @@ void __init pkey_early_init_devtree(void)
> return;
> }
>
> +#ifdef CONFIG_PPC_KUAP
> +void __init setup_kuap(bool disabled)
> +{
> + if (disabled || !early_radix_enabled())
> + return;
> +
> + if (smp_processor_id() == boot_cpuid) {
> + pr_info("Activating Kernel Userspace Access Prevention\n");
> + cur_cpu_spec->mmu_features |= MMU_FTR_RADIX_KUAP;
> + }
> +
> + /* Make sure userspace can't change the AMR */
> + mtspr(SPRN_UAMOR, 0);
> + mtspr(SPRN_AMR, AMR_KUAP_BLOCKED);
> + isync();
> +}
> +#endif
This makes this code depend on CONFIG_PPC_MEM_KEYS=y, which it didn't
used to.
That risks breaking people's existing .configs, if they have
PPC_MEM_KEYS=n they will now lose KUAP.
And I'm not convinced the two features should be tied together, at least
at the user-visible Kconfig level.
cheers
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v5 19/26] powerpc/book3s64/kuap: Move KUAP related function outside radix
2020-07-06 12:41 ` Michael Ellerman
@ 2020-07-06 14:41 ` Aneesh Kumar K.V
2020-07-07 1:22 ` Michael Ellerman
0 siblings, 1 reply; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-07-06 14:41 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: linuxram, bauerman
On 7/6/20 6:11 PM, Michael Ellerman wrote:
> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>> The next set of patches adds support for kuap with hash translation.
>
> That's no longer true of this series.
>
>> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
>> index 0d72c0246052..e93b65a0e6e7 100644
>> --- a/arch/powerpc/mm/book3s64/pkeys.c
>> +++ b/arch/powerpc/mm/book3s64/pkeys.c
>> @@ -199,6 +200,24 @@ void __init pkey_early_init_devtree(void)
>> return;
>> }
>>
>> +#ifdef CONFIG_PPC_KUAP
>> +void __init setup_kuap(bool disabled)
>> +{
>> + if (disabled || !early_radix_enabled())
>> + return;
>> +
>> + if (smp_processor_id() == boot_cpuid) {
>> + pr_info("Activating Kernel Userspace Access Prevention\n");
>> + cur_cpu_spec->mmu_features |= MMU_FTR_RADIX_KUAP;
>> + }
>> +
>> + /* Make sure userspace can't change the AMR */
>> + mtspr(SPRN_UAMOR, 0);
>> + mtspr(SPRN_AMR, AMR_KUAP_BLOCKED);
>> + isync();
>> +}
>> +#endif
>
> This makes this code depend on CONFIG_PPC_MEM_KEYS=y, which it didn't
> used to.
>
> That risks breaking people's existing .configs, if they have
> PPC_MEM_KEYS=n they will now lose KUAP.
>
> And I'm not convinced the two features should be tied together, at least
> at the user-visible Kconfig level.
>
That simplifies the addition of hash kuap a lot. Especially in the
exception entry and return paths. I did try to consider them as
independent options. But then the feature fixup in asm code gets
unnecessarily complicated. Also the UAMOR handling also get complicated.
-aneesh
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v5 19/26] powerpc/book3s64/kuap: Move KUAP related function outside radix
2020-07-06 14:41 ` Aneesh Kumar K.V
@ 2020-07-07 1:22 ` Michael Ellerman
0 siblings, 0 replies; 49+ messages in thread
From: Michael Ellerman @ 2020-07-07 1:22 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev; +Cc: linuxram, bauerman
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> On 7/6/20 6:11 PM, Michael Ellerman wrote:
>> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>>> The next set of patches adds support for kuap with hash translation.
>>
>> That's no longer true of this series.
>>
>>> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
>>> index 0d72c0246052..e93b65a0e6e7 100644
>>> --- a/arch/powerpc/mm/book3s64/pkeys.c
>>> +++ b/arch/powerpc/mm/book3s64/pkeys.c
>>> @@ -199,6 +200,24 @@ void __init pkey_early_init_devtree(void)
>>> return;
>>> }
>>>
>>> +#ifdef CONFIG_PPC_KUAP
>>> +void __init setup_kuap(bool disabled)
>>> +{
>>> + if (disabled || !early_radix_enabled())
>>> + return;
>>> +
>>> + if (smp_processor_id() == boot_cpuid) {
>>> + pr_info("Activating Kernel Userspace Access Prevention\n");
>>> + cur_cpu_spec->mmu_features |= MMU_FTR_RADIX_KUAP;
>>> + }
>>> +
>>> + /* Make sure userspace can't change the AMR */
>>> + mtspr(SPRN_UAMOR, 0);
>>> + mtspr(SPRN_AMR, AMR_KUAP_BLOCKED);
>>> + isync();
>>> +}
>>> +#endif
>>
>> This makes this code depend on CONFIG_PPC_MEM_KEYS=y, which it didn't
>> used to.
>>
>> That risks breaking people's existing .configs, if they have
>> PPC_MEM_KEYS=n they will now lose KUAP.
>>
>> And I'm not convinced the two features should be tied together, at least
>> at the user-visible Kconfig level.
>
> That simplifies the addition of hash kuap a lot. Especially in the
> exception entry and return paths. I did try to consider them as
> independent options. But then the feature fixup in asm code gets
> unnecessarily complicated. Also the UAMOR handling also get complicated.
Yep. I'm OK if most of the code is enabled for either/both options, but
I think the user-visible options should not depend on each other.
So something like:
config PPC_PKEY
def_bool y
depends on PPC_MEM_KEYS || PPC_KUAP
And then the low-level code is guarded by PPC_PKEY.
Or we just say that making MEM_KEYS configurable is not worth the
added complexity and turn it on always.
cheers
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v5 20/26] powerpc/book3s64/kuep: Move KUEP related function outside radix
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (18 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 19/26] powerpc/book3s64/kuap: Move KUAP related function outside radix Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 21/26] powerpc/book3s64/kuap: Rename MMU_FTR_RADIX_KUAP to MMU_FTR_KUAP Aneesh Kumar K.V
` (5 subsequent siblings)
25 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
The next set of patches adds support for kuep with hash translation.
In preparation for that rename/move kuap related functions to
non radix names.
Also set MMU_FTR_KUEP and add the missing isync().
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/book3s/64/kup.h | 1 +
arch/powerpc/mm/book3s64/pkeys.c | 21 +++++++++++++++++++++
arch/powerpc/mm/book3s64/radix_pgtable.c | 20 --------------------
3 files changed, 22 insertions(+), 20 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/kup.h b/arch/powerpc/include/asm/book3s/64/kup.h
index 54e237c093da..c4cf9b1caa23 100644
--- a/arch/powerpc/include/asm/book3s/64/kup.h
+++ b/arch/powerpc/include/asm/book3s/64/kup.h
@@ -7,6 +7,7 @@
#define AMR_KUAP_BLOCK_READ UL(0x4000000000000000)
#define AMR_KUAP_BLOCK_WRITE UL(0x8000000000000000)
+#define AMR_KUEP_BLOCKED (1UL << 62)
#define AMR_KUAP_BLOCKED (AMR_KUAP_BLOCK_READ | AMR_KUAP_BLOCK_WRITE)
#define AMR_KUAP_SHIFT 62
diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index e93b65a0e6e7..8ec677a91f80 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c
@@ -200,6 +200,27 @@ void __init pkey_early_init_devtree(void)
return;
}
+#ifdef CONFIG_PPC_KUEP
+void __init setup_kuep(bool disabled)
+{
+ if (disabled || !early_radix_enabled())
+ return;
+
+ if (smp_processor_id() == boot_cpuid) {
+ pr_info("Activating Kernel Userspace Execution Prevention\n");
+ cur_cpu_spec->mmu_features |= MMU_FTR_KUEP;
+ }
+
+ /*
+ * Radix always uses key0 of the IAMR to determine if an access is
+ * allowed. We set bit 0 (IBM bit 1) of key0, to prevent instruction
+ * fetch.
+ */
+ mtspr(SPRN_IAMR, AMR_KUEP_BLOCKED);
+ isync();
+}
+#endif
+
#ifdef CONFIG_PPC_KUAP
void __init setup_kuap(bool disabled)
{
diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
index 3959b7d4ad3c..2f641e1d7e82 100644
--- a/arch/powerpc/mm/book3s64/radix_pgtable.c
+++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
@@ -514,26 +514,6 @@ static void radix_init_amor(void)
mtspr(SPRN_AMOR, (3ul << 62));
}
-#ifdef CONFIG_PPC_KUEP
-void setup_kuep(bool disabled)
-{
- if (disabled || !early_radix_enabled())
- return;
-
- if (smp_processor_id() == boot_cpuid) {
- pr_info("Activating Kernel Userspace Execution Prevention\n");
- cur_cpu_spec->mmu_features |= MMU_FTR_KUEP;
- }
-
- /*
- * Radix always uses key0 of the IAMR to determine if an access is
- * allowed. We set bit 0 (IBM bit 1) of key0, to prevent instruction
- * fetch.
- */
- mtspr(SPRN_IAMR, (1ul << 62));
-}
-#endif
-
void __init radix__early_init_mmu(void)
{
unsigned long lpcr;
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v5 21/26] powerpc/book3s64/kuap: Rename MMU_FTR_RADIX_KUAP to MMU_FTR_KUAP
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (19 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 20/26] powerpc/book3s64/kuep: Move KUEP " Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 22/26] powerpc/book3s64/kuap/kuep: Make KUAP and KUEP a subfeature of PPC_MEM_KEYS Aneesh Kumar K.V
` (4 subsequent siblings)
25 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
The next set of patches adds support for kuap with hash translation.
In preparation for that rename/move kuap related functions to
non radix names.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/book3s/64/kup.h | 20 ++++++++++----------
arch/powerpc/include/asm/mmu.h | 6 +++---
arch/powerpc/mm/book3s64/pkeys.c | 2 +-
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/kup.h b/arch/powerpc/include/asm/book3s/64/kup.h
index c4cf9b1caa23..3cecd964a63f 100644
--- a/arch/powerpc/include/asm/book3s/64/kup.h
+++ b/arch/powerpc/include/asm/book3s/64/kup.h
@@ -24,7 +24,7 @@
mtspr SPRN_AMR, \gpr2
/* No isync required, see kuap_restore_amr() */
998:
- END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_RADIX_KUAP, 67)
+ END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_KUAP, 67)
#endif
.endm
@@ -36,7 +36,7 @@
sldi \gpr2, \gpr2, AMR_KUAP_SHIFT
999: tdne \gpr1, \gpr2
EMIT_BUG_ENTRY 999b, __FILE__, __LINE__, (BUGFLAG_WARNING | BUGFLAG_ONCE)
- END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_RADIX_KUAP, 67)
+ END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_KUAP, 67)
#endif
.endm
@@ -56,7 +56,7 @@
mtspr SPRN_AMR, \gpr2
isync
99:
- END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_RADIX_KUAP, 67)
+ END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_KUAP, 67)
#endif
.endm
@@ -69,7 +69,7 @@
static inline void kuap_restore_amr(struct pt_regs *regs, unsigned long amr)
{
- if (mmu_has_feature(MMU_FTR_RADIX_KUAP) && unlikely(regs->kuap != amr)) {
+ if (mmu_has_feature(MMU_FTR_KUAP) && unlikely(regs->kuap != amr)) {
isync();
mtspr(SPRN_AMR, regs->kuap);
/*
@@ -82,7 +82,7 @@ static inline void kuap_restore_amr(struct pt_regs *regs, unsigned long amr)
static inline unsigned long kuap_get_and_check_amr(void)
{
- if (mmu_has_feature(MMU_FTR_RADIX_KUAP)) {
+ if (mmu_has_feature(MMU_FTR_KUAP)) {
unsigned long amr = mfspr(SPRN_AMR);
if (IS_ENABLED(CONFIG_PPC_KUAP_DEBUG)) /* kuap_check_amr() */
WARN_ON_ONCE(amr != AMR_KUAP_BLOCKED);
@@ -93,7 +93,7 @@ static inline unsigned long kuap_get_and_check_amr(void)
static inline void kuap_check_amr(void)
{
- if (IS_ENABLED(CONFIG_PPC_KUAP_DEBUG) && mmu_has_feature(MMU_FTR_RADIX_KUAP))
+ if (IS_ENABLED(CONFIG_PPC_KUAP_DEBUG) && mmu_has_feature(MMU_FTR_KUAP))
WARN_ON_ONCE(mfspr(SPRN_AMR) != AMR_KUAP_BLOCKED);
}
@@ -104,7 +104,7 @@ static inline void kuap_check_amr(void)
static inline unsigned long get_kuap(void)
{
- if (!early_mmu_has_feature(MMU_FTR_RADIX_KUAP))
+ if (!early_mmu_has_feature(MMU_FTR_KUAP))
return 0;
return mfspr(SPRN_AMR);
@@ -112,7 +112,7 @@ static inline unsigned long get_kuap(void)
static inline void set_kuap(unsigned long value)
{
- if (!early_mmu_has_feature(MMU_FTR_RADIX_KUAP))
+ if (!early_mmu_has_feature(MMU_FTR_KUAP))
return;
/*
@@ -162,7 +162,7 @@ static inline void restore_user_access(unsigned long flags)
static inline bool
bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
{
- return WARN(mmu_has_feature(MMU_FTR_RADIX_KUAP) &&
+ return WARN(mmu_has_feature(MMU_FTR_KUAP) &&
(regs->kuap & (is_write ? AMR_KUAP_BLOCK_WRITE : AMR_KUAP_BLOCK_READ)),
"Bug: %s fault blocked by AMR!", is_write ? "Write" : "Read");
}
@@ -184,7 +184,7 @@ static inline unsigned long kuap_get_and_check_amr(void)
#define reset_kuap reset_kuap
static inline void reset_kuap(void)
{
- if (mmu_has_feature(MMU_FTR_RADIX_KUAP)) {
+ if (mmu_has_feature(MMU_FTR_KUAP)) {
mtspr(SPRN_AMR, 0);
/* Do we need isync()? We are going via a kexec reset */
isync();
diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
index 94435f85e3bc..14d7e6803453 100644
--- a/arch/powerpc/include/asm/mmu.h
+++ b/arch/powerpc/include/asm/mmu.h
@@ -112,7 +112,7 @@
/*
* Supports KUAP (key 0 controlling userspace addresses) on radix
*/
-#define MMU_FTR_RADIX_KUAP ASM_CONST(0x80000000)
+#define MMU_FTR_KUAP ASM_CONST(0x80000000)
/* MMU feature bit sets for various CPUs */
#define MMU_FTRS_DEFAULT_HPTE_ARCH_V2 \
@@ -175,10 +175,10 @@ enum {
#endif
#ifdef CONFIG_PPC_RADIX_MMU
MMU_FTR_TYPE_RADIX |
+#endif /* CONFIG_PPC_RADIX_MMU */
#ifdef CONFIG_PPC_KUAP
- MMU_FTR_RADIX_KUAP |
+ MMU_FTR_KUAP |
#endif /* CONFIG_PPC_KUAP */
-#endif /* CONFIG_PPC_RADIX_MMU */
#ifdef CONFIG_PPC_MEM_KEYS
MMU_FTR_PKEY |
#endif
diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index 8ec677a91f80..aeecc8b8e11c 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c
@@ -229,7 +229,7 @@ void __init setup_kuap(bool disabled)
if (smp_processor_id() == boot_cpuid) {
pr_info("Activating Kernel Userspace Access Prevention\n");
- cur_cpu_spec->mmu_features |= MMU_FTR_RADIX_KUAP;
+ cur_cpu_spec->mmu_features |= MMU_FTR_KUAP;
}
/* Make sure userspace can't change the AMR */
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v5 22/26] powerpc/book3s64/kuap/kuep: Make KUAP and KUEP a subfeature of PPC_MEM_KEYS
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (20 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 21/26] powerpc/book3s64/kuap: Rename MMU_FTR_RADIX_KUAP to MMU_FTR_KUAP Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 23/26] powerpc/book3s64/kuap: Move UAMOR setup to key init function Aneesh Kumar K.V
` (3 subsequent siblings)
25 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
The next set of patches adds support for kuap with hash translation.
Hence make KUAP a BOOK3S_64 feature. Also make it a subfeature of
PPC_MEM_KEYS. Hash translation is going to use pkeys to support
KUAP/KUEP. Adding this dependency reduces the code complexity and
enables us to move some of the initialization code to pkeys.c
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/book3s/64/kup.h | 33 ++++++++++++++----------
arch/powerpc/include/asm/ptrace.h | 2 +-
arch/powerpc/kernel/asm-offsets.c | 2 +-
arch/powerpc/platforms/Kconfig.cputype | 4 +--
4 files changed, 23 insertions(+), 18 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/kup.h b/arch/powerpc/include/asm/book3s/64/kup.h
index 3cecd964a63f..3a0e138d2735 100644
--- a/arch/powerpc/include/asm/book3s/64/kup.h
+++ b/arch/powerpc/include/asm/book3s/64/kup.h
@@ -62,7 +62,7 @@
#else /* !__ASSEMBLY__ */
-#ifdef CONFIG_PPC_KUAP
+#ifdef CONFIG_PPC_MEM_KEYS
#include <asm/mmu.h>
#include <asm/ptrace.h>
@@ -97,6 +97,24 @@ static inline void kuap_check_amr(void)
WARN_ON_ONCE(mfspr(SPRN_AMR) != AMR_KUAP_BLOCKED);
}
+#else /* CONFIG_PPC_MEM_KEYS */
+
+static inline void kuap_restore_amr(struct pt_regs *regs, unsigned long amr)
+{
+}
+
+static inline void kuap_check_amr(void)
+{
+}
+
+static inline unsigned long kuap_get_and_check_amr(void)
+{
+ return 0;
+}
+#endif /* CONFIG_PPC_MEM_KEYS */
+
+
+#ifdef CONFIG_PPC_KUAP
/*
* We support individually allowing read or write, but we don't support nesting
* because that would require an expensive read/modify write of the AMR.
@@ -166,19 +184,6 @@ bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
(regs->kuap & (is_write ? AMR_KUAP_BLOCK_WRITE : AMR_KUAP_BLOCK_READ)),
"Bug: %s fault blocked by AMR!", is_write ? "Write" : "Read");
}
-#else /* CONFIG_PPC_KUAP */
-static inline void kuap_restore_amr(struct pt_regs *regs, unsigned long amr)
-{
-}
-
-static inline void kuap_check_amr(void)
-{
-}
-
-static inline unsigned long kuap_get_and_check_amr(void)
-{
- return 0;
-}
#endif /* CONFIG_PPC_KUAP */
#define reset_kuap reset_kuap
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index ac3970fff0d5..1a6cadf63d14 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -53,7 +53,7 @@ struct pt_regs
#ifdef CONFIG_PPC64
unsigned long ppr;
#endif
-#ifdef CONFIG_PPC_KUAP
+#ifdef CONFIG_PPC_HAVE_KUAP
unsigned long kuap;
#endif
};
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index 9b9cde07e396..1694c4f531b9 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -354,7 +354,7 @@ int main(void)
STACK_PT_REGS_OFFSET(_PPR, ppr);
#endif /* CONFIG_PPC64 */
-#ifdef CONFIG_PPC_KUAP
+#ifdef CONFIG_PPC_HAVE_KUAP
STACK_PT_REGS_OFFSET(STACK_REGS_KUAP, kuap);
#endif
diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
index d349603fb889..053c46aecf80 100644
--- a/arch/powerpc/platforms/Kconfig.cputype
+++ b/arch/powerpc/platforms/Kconfig.cputype
@@ -99,6 +99,8 @@ config PPC_BOOK3S_64
select ARCH_SUPPORTS_NUMA_BALANCING
select IRQ_WORK
select PPC_MM_SLICES
+ select PPC_HAVE_KUAP if PPC_MEM_KEYS
+ select PPC_HAVE_KUEP if PPC_MEM_KEYS
config PPC_BOOK3E_64
bool "Embedded processors"
@@ -350,8 +352,6 @@ config PPC_RADIX_MMU
bool "Radix MMU Support"
depends on PPC_BOOK3S_64
select ARCH_HAS_GIGANTIC_PAGE
- select PPC_HAVE_KUEP
- select PPC_HAVE_KUAP
default y
help
Enable support for the Power ISA 3.0 Radix style MMU. Currently this
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v5 23/26] powerpc/book3s64/kuap: Move UAMOR setup to key init function
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (21 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 22/26] powerpc/book3s64/kuap/kuep: Make KUAP and KUEP a subfeature of PPC_MEM_KEYS Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-07-07 6:05 ` Michael Ellerman
2020-06-19 13:58 ` [PATCH v5 24/26] powerpc/selftest/ptrave-pkey: Rename variables to make it easier to follow code Aneesh Kumar K.V
` (2 subsequent siblings)
25 siblings, 1 reply; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
UAMOR values are not application-specific. The kernel initializes
its value based on different reserved keys. Remove the thread-specific
UAMOR value and don't switch the UAMOR on context switch.
Move UAMOR initialization to key initialization code. Now that
KUAP/KUEP feature depends on PPC_MEM_KEYS, we can start to consolidate
all register initialization to keys init.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/book3s/64/kup.h | 2 ++
arch/powerpc/include/asm/processor.h | 1 -
arch/powerpc/kernel/ptrace/ptrace-view.c | 17 ++++++++----
arch/powerpc/kernel/smp.c | 5 ++++
arch/powerpc/mm/book3s64/pkeys.c | 35 ++++++++++++++----------
5 files changed, 39 insertions(+), 21 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/kup.h b/arch/powerpc/include/asm/book3s/64/kup.h
index 3a0e138d2735..942594745dfa 100644
--- a/arch/powerpc/include/asm/book3s/64/kup.h
+++ b/arch/powerpc/include/asm/book3s/64/kup.h
@@ -67,6 +67,8 @@
#include <asm/mmu.h>
#include <asm/ptrace.h>
+extern u64 default_uamor;
+
static inline void kuap_restore_amr(struct pt_regs *regs, unsigned long amr)
{
if (mmu_has_feature(MMU_FTR_KUAP) && unlikely(regs->kuap != amr)) {
diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index 52a67835057a..6ac12168f1fe 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -237,7 +237,6 @@ struct thread_struct {
#ifdef CONFIG_PPC_MEM_KEYS
unsigned long amr;
unsigned long iamr;
- unsigned long uamor;
#endif
#ifdef CONFIG_KVM_BOOK3S_32_HANDLER
void* kvm_shadow_vcpu; /* KVM internal data */
diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c b/arch/powerpc/kernel/ptrace/ptrace-view.c
index caeb5822a8f4..689711eb018a 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-view.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
@@ -488,14 +488,22 @@ static int pkey_active(struct task_struct *target, const struct user_regset *reg
static int pkey_get(struct task_struct *target, const struct user_regset *regset,
unsigned int pos, unsigned int count, void *kbuf, void __user *ubuf)
{
+ int ret;
+
BUILD_BUG_ON(TSO(amr) + sizeof(unsigned long) != TSO(iamr));
- BUILD_BUG_ON(TSO(iamr) + sizeof(unsigned long) != TSO(uamor));
if (!arch_pkeys_enabled())
return -ENODEV;
- return user_regset_copyout(&pos, &count, &kbuf, &ubuf, &target->thread.amr,
- 0, ELF_NPKEY * sizeof(unsigned long));
+ ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &target->thread.amr,
+ 0, 2 * sizeof(unsigned long));
+ if (ret)
+ goto err_out;
+
+ ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &default_uamor,
+ 2 * sizeof(unsigned long), 3 * sizeof(unsigned long));
+err_out:
+ return ret;
}
static int pkey_set(struct task_struct *target, const struct user_regset *regset,
@@ -518,8 +526,7 @@ static int pkey_set(struct task_struct *target, const struct user_regset *regset
return ret;
/* UAMOR determines which bits of the AMR can be set from userspace. */
- target->thread.amr = (new_amr & target->thread.uamor) |
- (target->thread.amr & ~target->thread.uamor);
+ target->thread.amr = (new_amr & default_uamor) | (target->thread.amr & ~default_uamor);
return 0;
}
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index c820c95162ff..eec40082599f 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -59,6 +59,7 @@
#include <asm/asm-prototypes.h>
#include <asm/cpu_has_feature.h>
#include <asm/ftrace.h>
+#include <asm/kup.h>
#ifdef DEBUG
#include <asm/udbg.h>
@@ -1256,6 +1257,10 @@ void start_secondary(void *unused)
mmgrab(&init_mm);
current->active_mm = &init_mm;
+#ifdef CONFIG_PPC_MEM_KEYS
+ mtspr(SPRN_UAMOR, default_uamor);
+#endif
+
smp_store_cpu_info(cpu);
set_dec(tb_ticks_per_jiffy);
preempt_disable();
diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index aeecc8b8e11c..3f3593f85358 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c
@@ -24,7 +24,7 @@ static u32 initial_allocation_mask; /* Bits set for the initially allocated k
static u64 default_amr;
static u64 default_iamr;
/* Allow all keys to be modified by default */
-static u64 default_uamor = ~0x0UL;
+u64 default_uamor = ~0x0UL;
/*
* Key used to implement PROT_EXEC mmap. Denies READ/WRITE
* We pick key 2 because 0 is special key and 1 is reserved as per ISA.
@@ -113,8 +113,16 @@ void __init pkey_early_init_devtree(void)
/* scan the device tree for pkey feature */
pkeys_total = scan_pkey_feature();
if (!pkeys_total) {
- /* No support for pkey. Mark it disabled */
- return;
+ /*
+ * No key support but on radix we can use key 0
+ * to implement kuap.
+ */
+ if (early_radix_enabled())
+ /*
+ * Make sure userspace can't change the AMR
+ */
+ default_uamor = 0;
+ goto err_out;
}
cur_cpu_spec->mmu_features |= MMU_FTR_PKEY;
@@ -197,6 +205,12 @@ void __init pkey_early_init_devtree(void)
initial_allocation_mask |= reserved_allocation_mask;
pr_info("Enabling Memory keys with max key count %d", max_pkey);
+err_out:
+ /*
+ * Setup uamor on boot cpu
+ */
+ mtspr(SPRN_UAMOR, default_uamor);
+
return;
}
@@ -232,8 +246,9 @@ void __init setup_kuap(bool disabled)
cur_cpu_spec->mmu_features |= MMU_FTR_KUAP;
}
- /* Make sure userspace can't change the AMR */
- mtspr(SPRN_UAMOR, 0);
+ /*
+ * Set the default kernel AMR values on all cpus.
+ */
mtspr(SPRN_AMR, AMR_KUAP_BLOCKED);
isync();
}
@@ -278,11 +293,6 @@ static inline u64 read_uamor(void)
return mfspr(SPRN_UAMOR);
}
-static inline void write_uamor(u64 value)
-{
- mtspr(SPRN_UAMOR, value);
-}
-
static bool is_pkey_enabled(int pkey)
{
u64 uamor = read_uamor();
@@ -353,7 +363,6 @@ void thread_pkey_regs_save(struct thread_struct *thread)
*/
thread->amr = read_amr();
thread->iamr = read_iamr();
- thread->uamor = read_uamor();
}
void thread_pkey_regs_restore(struct thread_struct *new_thread,
@@ -366,8 +375,6 @@ void thread_pkey_regs_restore(struct thread_struct *new_thread,
write_amr(new_thread->amr);
if (old_thread->iamr != new_thread->iamr)
write_iamr(new_thread->iamr);
- if (old_thread->uamor != new_thread->uamor)
- write_uamor(new_thread->uamor);
}
void thread_pkey_regs_init(struct thread_struct *thread)
@@ -377,11 +384,9 @@ void thread_pkey_regs_init(struct thread_struct *thread)
thread->amr = default_amr;
thread->iamr = default_iamr;
- thread->uamor = default_uamor;
write_amr(default_amr);
write_iamr(default_iamr);
- write_uamor(default_uamor);
}
int execute_only_pkey(struct mm_struct *mm)
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v5 23/26] powerpc/book3s64/kuap: Move UAMOR setup to key init function
2020-06-19 13:58 ` [PATCH v5 23/26] powerpc/book3s64/kuap: Move UAMOR setup to key init function Aneesh Kumar K.V
@ 2020-07-07 6:05 ` Michael Ellerman
2020-07-07 11:25 ` Aneesh Kumar K.V
0 siblings, 1 reply; 49+ messages in thread
From: Michael Ellerman @ 2020-07-07 6:05 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev; +Cc: Aneesh Kumar K.V, linuxram, bauerman
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> UAMOR values are not application-specific.
It used to be, that's worth mentioning.
> The kernel initializes its value based on different reserved keys.
> Remove the thread-specific UAMOR value and don't switch the UAMOR on
> context switch.
>
> Move UAMOR initialization to key initialization code. Now that
> KUAP/KUEP feature depends on PPC_MEM_KEYS, we can start to consolidate
> all register initialization to keys init.
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
> arch/powerpc/include/asm/book3s/64/kup.h | 2 ++
> arch/powerpc/include/asm/processor.h | 1 -
> arch/powerpc/kernel/ptrace/ptrace-view.c | 17 ++++++++----
> arch/powerpc/kernel/smp.c | 5 ++++
> arch/powerpc/mm/book3s64/pkeys.c | 35 ++++++++++++++----------
> 5 files changed, 39 insertions(+), 21 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/64/kup.h b/arch/powerpc/include/asm/book3s/64/kup.h
> index 3a0e138d2735..942594745dfa 100644
> --- a/arch/powerpc/include/asm/book3s/64/kup.h
> +++ b/arch/powerpc/include/asm/book3s/64/kup.h
> @@ -67,6 +67,8 @@
> #include <asm/mmu.h>
> #include <asm/ptrace.h>
>
> +extern u64 default_uamor;
> +
> static inline void kuap_restore_amr(struct pt_regs *regs, unsigned long amr)
> {
> if (mmu_has_feature(MMU_FTR_KUAP) && unlikely(regs->kuap != amr)) {
> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
> index 52a67835057a..6ac12168f1fe 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -237,7 +237,6 @@ struct thread_struct {
> #ifdef CONFIG_PPC_MEM_KEYS
> unsigned long amr;
> unsigned long iamr;
> - unsigned long uamor;
> #endif
> #ifdef CONFIG_KVM_BOOK3S_32_HANDLER
> void* kvm_shadow_vcpu; /* KVM internal data */
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c b/arch/powerpc/kernel/ptrace/ptrace-view.c
> index caeb5822a8f4..689711eb018a 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-view.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
> @@ -488,14 +488,22 @@ static int pkey_active(struct task_struct *target, const struct user_regset *reg
> static int pkey_get(struct task_struct *target, const struct user_regset *regset,
> unsigned int pos, unsigned int count, void *kbuf, void __user *ubuf)
> {
> + int ret;
> +
> BUILD_BUG_ON(TSO(amr) + sizeof(unsigned long) != TSO(iamr));
> - BUILD_BUG_ON(TSO(iamr) + sizeof(unsigned long) != TSO(uamor));
>
> if (!arch_pkeys_enabled())
> return -ENODEV;
>
> - return user_regset_copyout(&pos, &count, &kbuf, &ubuf, &target->thread.amr,
> - 0, ELF_NPKEY * sizeof(unsigned long));
> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &target->thread.amr,
> + 0, 2 * sizeof(unsigned long));
> + if (ret)
> + goto err_out;
Why not just return?
> +
> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &default_uamor,
> + 2 * sizeof(unsigned long), 3 * sizeof(unsigned long));
> +err_out:
> + return ret;
> }
>
> static int pkey_set(struct task_struct *target, const struct user_regset *regset,
> @@ -518,8 +526,7 @@ static int pkey_set(struct task_struct *target, const struct user_regset *regset
> return ret;
>
> /* UAMOR determines which bits of the AMR can be set from userspace. */
> - target->thread.amr = (new_amr & target->thread.uamor) |
> - (target->thread.amr & ~target->thread.uamor);
> + target->thread.amr = (new_amr & default_uamor) | (target->thread.amr & ~default_uamor);
That comment could explain better why we are bothering to mask with ~default_uamor.
> return 0;
> }
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index c820c95162ff..eec40082599f 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -59,6 +59,7 @@
> #include <asm/asm-prototypes.h>
> #include <asm/cpu_has_feature.h>
> #include <asm/ftrace.h>
> +#include <asm/kup.h>
>
> #ifdef DEBUG
> #include <asm/udbg.h>
> @@ -1256,6 +1257,10 @@ void start_secondary(void *unused)
> mmgrab(&init_mm);
> current->active_mm = &init_mm;
>
> +#ifdef CONFIG_PPC_MEM_KEYS
> + mtspr(SPRN_UAMOR, default_uamor);
> +#endif
That's 1) not very pretty and 2) risks blowing up on other CPUs.
It should at least go in early_init_mmu_secondary().
> smp_store_cpu_info(cpu);
> set_dec(tb_ticks_per_jiffy);
> preempt_disable();
> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
> index aeecc8b8e11c..3f3593f85358 100644
> --- a/arch/powerpc/mm/book3s64/pkeys.c
> +++ b/arch/powerpc/mm/book3s64/pkeys.c
> @@ -24,7 +24,7 @@ static u32 initial_allocation_mask; /* Bits set for the initially allocated k
> static u64 default_amr;
> static u64 default_iamr;
> /* Allow all keys to be modified by default */
> -static u64 default_uamor = ~0x0UL;
> +u64 default_uamor = ~0x0UL;
__ro_after_init?
> /*
> * Key used to implement PROT_EXEC mmap. Denies READ/WRITE
> * We pick key 2 because 0 is special key and 1 is reserved as per ISA.
> @@ -113,8 +113,16 @@ void __init pkey_early_init_devtree(void)
> /* scan the device tree for pkey feature */
> pkeys_total = scan_pkey_feature();
> if (!pkeys_total) {
> - /* No support for pkey. Mark it disabled */
> - return;
> + /*
> + * No key support but on radix we can use key 0
> + * to implement kuap.
> + */
> + if (early_radix_enabled())
> + /*
> + * Make sure userspace can't change the AMR
> + */
> + default_uamor = 0;
> + goto err_out;
Would be cleaner if you inverted that. ie. initialise to 0 and then set
to ~0x0UL when you detect pkeys.
> }
>
> cur_cpu_spec->mmu_features |= MMU_FTR_PKEY;
> @@ -197,6 +205,12 @@ void __init pkey_early_init_devtree(void)
> initial_allocation_mask |= reserved_allocation_mask;
>
> pr_info("Enabling Memory keys with max key count %d", max_pkey);
> +err_out:
It's not "err" out if the OK path goes via here. That's just "out".
> + /*
> + * Setup uamor on boot cpu
> + */
> + mtspr(SPRN_UAMOR, default_uamor);
> +
> return;
> }
>
> @@ -232,8 +246,9 @@ void __init setup_kuap(bool disabled)
> cur_cpu_spec->mmu_features |= MMU_FTR_KUAP;
> }
>
> - /* Make sure userspace can't change the AMR */
> - mtspr(SPRN_UAMOR, 0);
Why not just leave it there. It's extra insurance and it's good
documentation.
> + /*
> + * Set the default kernel AMR values on all cpus.
> + */
> mtspr(SPRN_AMR, AMR_KUAP_BLOCKED);
> isync();
> }
> @@ -278,11 +293,6 @@ static inline u64 read_uamor(void)
> return mfspr(SPRN_UAMOR);
> }
>
> -static inline void write_uamor(u64 value)
> -{
> - mtspr(SPRN_UAMOR, value);
> -}
> -
> static bool is_pkey_enabled(int pkey)
> {
> u64 uamor = read_uamor();
> @@ -353,7 +363,6 @@ void thread_pkey_regs_save(struct thread_struct *thread)
> */
> thread->amr = read_amr();
> thread->iamr = read_iamr();
> - thread->uamor = read_uamor();
> }
>
> void thread_pkey_regs_restore(struct thread_struct *new_thread,
> @@ -366,8 +375,6 @@ void thread_pkey_regs_restore(struct thread_struct *new_thread,
> write_amr(new_thread->amr);
> if (old_thread->iamr != new_thread->iamr)
> write_iamr(new_thread->iamr);
> - if (old_thread->uamor != new_thread->uamor)
> - write_uamor(new_thread->uamor);
> }
>
> void thread_pkey_regs_init(struct thread_struct *thread)
> @@ -377,11 +384,9 @@ void thread_pkey_regs_init(struct thread_struct *thread)
>
> thread->amr = default_amr;
> thread->iamr = default_iamr;
> - thread->uamor = default_uamor;
>
> write_amr(default_amr);
> write_iamr(default_iamr);
> - write_uamor(default_uamor);
> }
>
> int execute_only_pkey(struct mm_struct *mm)
cheers
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v5 23/26] powerpc/book3s64/kuap: Move UAMOR setup to key init function
2020-07-07 6:05 ` Michael Ellerman
@ 2020-07-07 11:25 ` Aneesh Kumar K.V
0 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-07-07 11:25 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: linuxram, bauerman
Michael Ellerman <mpe@ellerman.id.au> writes:
> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>
.....
>> @@ -232,8 +246,9 @@ void __init setup_kuap(bool disabled)
>> cur_cpu_spec->mmu_features |= MMU_FTR_KUAP;
>> }
>>
>> - /* Make sure userspace can't change the AMR */
>> - mtspr(SPRN_UAMOR, 0);
>
> Why not just leave it there. It's extra insurance and it's good
> documentation.
We can't se the value to 0, because with hash kuap it is derived
from what other keys are used for. Are you suggesting to keep it as
if (radix_enabled())
mtspr(SPRN_UAMOR, 0);
That would confuse w.r.t what happens with hash.
I can add a comment there explaining details?
>
>> + /*
>> + * Set the default kernel AMR values on all cpus.
>> + */
>> mtspr(SPRN_AMR, AMR_KUAP_BLOCKED);
>> isync();
>> }
>> @@ -278,11 +293,6 @@ static inline u64 read_uamor(void)
>> return mfspr(SPRN_UAMOR);
>> }
>>
>> -static inline void write_uamor(u64 value)
>> -{
>> - mtspr(SPRN_UAMOR, value);
>> -}
>> -
>> static bool is_pkey_enabled(int pkey)
>> {
>> u64 uamor = read_uamor();
>> @@ -353,7 +363,6 @@ void thread_pkey_regs_save(struct thread_struct *thread)
>> */
>> thread->amr = read_amr();
>> thread->iamr = read_iamr();
>> - thread->uamor = read_uamor();
>> }
>>
>> void thread_pkey_regs_restore(struct thread_struct *new_thread,
>> @@ -366,8 +375,6 @@ void thread_pkey_regs_restore(struct thread_struct *new_thread,
>> write_amr(new_thread->amr);
>> if (old_thread->iamr != new_thread->iamr)
>> write_iamr(new_thread->iamr);
>> - if (old_thread->uamor != new_thread->uamor)
>> - write_uamor(new_thread->uamor);
>> }
>>
>> void thread_pkey_regs_init(struct thread_struct *thread)
>> @@ -377,11 +384,9 @@ void thread_pkey_regs_init(struct thread_struct *thread)
>>
>> thread->amr = default_amr;
>> thread->iamr = default_iamr;
>> - thread->uamor = default_uamor;
>>
>> write_amr(default_amr);
>> write_iamr(default_iamr);
>> - write_uamor(default_uamor);
>> }
>>
>> int execute_only_pkey(struct mm_struct *mm)
>
> cheers
-aneesh
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v5 24/26] powerpc/selftest/ptrave-pkey: Rename variables to make it easier to follow code
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (22 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 23/26] powerpc/book3s64/kuap: Move UAMOR setup to key init function Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 25/26] powerpc/selftest/ptrace-pkey: Update the test to mark an invalid pkey correctly Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 26/26] powerpc/selftest/ptrace-pkey: IAMR and uamor cannot be updated by ptrace Aneesh Kumar K.V
25 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
Rename variable to indicate that they are invalid values which we will use to
test ptrace update of pkeys.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
.../selftests/powerpc/ptrace/ptrace-pkey.c | 26 +++++++++----------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c b/tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c
index bdbbbe8431e0..f9216c7a1829 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c
@@ -44,7 +44,7 @@ struct shared_info {
unsigned long amr2;
/* AMR value that ptrace should refuse to write to the child. */
- unsigned long amr3;
+ unsigned long invalid_amr;
/* IAMR value the parent expects to read from the child. */
unsigned long expected_iamr;
@@ -57,8 +57,8 @@ struct shared_info {
* (even though they're valid ones) because userspace doesn't have
* access to those registers.
*/
- unsigned long new_iamr;
- unsigned long new_uamor;
+ unsigned long invalid_iamr;
+ unsigned long invalid_uamor;
};
static int sys_pkey_alloc(unsigned long flags, unsigned long init_access_rights)
@@ -100,7 +100,7 @@ static int child(struct shared_info *info)
info->amr1 |= 3ul << pkeyshift(pkey1);
info->amr2 |= 3ul << pkeyshift(pkey2);
- info->amr3 |= info->amr2 | 3ul << pkeyshift(pkey3);
+ info->invalid_amr |= info->amr2 | 3ul << pkeyshift(pkey3);
if (disable_execute)
info->expected_iamr |= 1ul << pkeyshift(pkey1);
@@ -111,8 +111,8 @@ static int child(struct shared_info *info)
info->expected_uamor |= 3ul << pkeyshift(pkey1) |
3ul << pkeyshift(pkey2);
- info->new_iamr |= 1ul << pkeyshift(pkey1) | 1ul << pkeyshift(pkey2);
- info->new_uamor |= 3ul << pkeyshift(pkey1);
+ info->invalid_iamr |= 1ul << pkeyshift(pkey1) | 1ul << pkeyshift(pkey2);
+ info->invalid_uamor |= 3ul << pkeyshift(pkey1);
/*
* We won't use pkey3. We just want a plausible but invalid key to test
@@ -196,9 +196,9 @@ static int parent(struct shared_info *info, pid_t pid)
PARENT_SKIP_IF_UNSUPPORTED(ret, &info->child_sync);
PARENT_FAIL_IF(ret, &info->child_sync);
- info->amr1 = info->amr2 = info->amr3 = regs[0];
- info->expected_iamr = info->new_iamr = regs[1];
- info->expected_uamor = info->new_uamor = regs[2];
+ info->amr1 = info->amr2 = info->invalid_amr = regs[0];
+ info->expected_iamr = info->invalid_iamr = regs[1];
+ info->expected_uamor = info->invalid_uamor = regs[2];
/* Wake up child so that it can set itself up. */
ret = prod_child(&info->child_sync);
@@ -234,10 +234,10 @@ static int parent(struct shared_info *info, pid_t pid)
return ret;
/* Write invalid AMR value in child. */
- ret = ptrace_write_regs(pid, NT_PPC_PKEY, &info->amr3, 1);
+ ret = ptrace_write_regs(pid, NT_PPC_PKEY, &info->invalid_amr, 1);
PARENT_FAIL_IF(ret, &info->child_sync);
- printf("%-30s AMR: %016lx\n", ptrace_write_running, info->amr3);
+ printf("%-30s AMR: %016lx\n", ptrace_write_running, info->invalid_amr);
/* Wake up child so that it can verify it didn't change. */
ret = prod_child(&info->child_sync);
@@ -249,7 +249,7 @@ static int parent(struct shared_info *info, pid_t pid)
/* Try to write to IAMR. */
regs[0] = info->amr1;
- regs[1] = info->new_iamr;
+ regs[1] = info->invalid_iamr;
ret = ptrace_write_regs(pid, NT_PPC_PKEY, regs, 2);
PARENT_FAIL_IF(!ret, &info->child_sync);
@@ -257,7 +257,7 @@ static int parent(struct shared_info *info, pid_t pid)
ptrace_write_running, regs[0], regs[1]);
/* Try to write to IAMR and UAMOR. */
- regs[2] = info->new_uamor;
+ regs[2] = info->invalid_uamor;
ret = ptrace_write_regs(pid, NT_PPC_PKEY, regs, 3);
PARENT_FAIL_IF(!ret, &info->child_sync);
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v5 25/26] powerpc/selftest/ptrace-pkey: Update the test to mark an invalid pkey correctly
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (23 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 24/26] powerpc/selftest/ptrave-pkey: Rename variables to make it easier to follow code Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 26/26] powerpc/selftest/ptrace-pkey: IAMR and uamor cannot be updated by ptrace Aneesh Kumar K.V
25 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
.../selftests/powerpc/ptrace/ptrace-pkey.c | 30 ++++++++-----------
1 file changed, 12 insertions(+), 18 deletions(-)
diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c b/tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c
index f9216c7a1829..bc33d748d95b 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c
@@ -66,11 +66,6 @@ static int sys_pkey_alloc(unsigned long flags, unsigned long init_access_rights)
return syscall(__NR_pkey_alloc, flags, init_access_rights);
}
-static int sys_pkey_free(int pkey)
-{
- return syscall(__NR_pkey_free, pkey);
-}
-
static int child(struct shared_info *info)
{
unsigned long reg;
@@ -100,7 +95,11 @@ static int child(struct shared_info *info)
info->amr1 |= 3ul << pkeyshift(pkey1);
info->amr2 |= 3ul << pkeyshift(pkey2);
- info->invalid_amr |= info->amr2 | 3ul << pkeyshift(pkey3);
+ /*
+ * invalid amr value where we try to force write
+ * things which are deined by a uamor setting.
+ */
+ info->invalid_amr = info->amr2 | (~0x0UL & ~info->expected_uamor);
if (disable_execute)
info->expected_iamr |= 1ul << pkeyshift(pkey1);
@@ -111,17 +110,12 @@ static int child(struct shared_info *info)
info->expected_uamor |= 3ul << pkeyshift(pkey1) |
3ul << pkeyshift(pkey2);
- info->invalid_iamr |= 1ul << pkeyshift(pkey1) | 1ul << pkeyshift(pkey2);
- info->invalid_uamor |= 3ul << pkeyshift(pkey1);
-
/*
- * We won't use pkey3. We just want a plausible but invalid key to test
- * whether ptrace will let us write to AMR bits we are not supposed to.
- *
- * This also tests whether the kernel restores the UAMOR permissions
- * after a key is freed.
+ * Create an IAMR value different from expected value.
+ * Kernel will reject an IAMR and UAMOR change.
*/
- sys_pkey_free(pkey3);
+ info->invalid_iamr = info->expected_iamr | (1ul << pkeyshift(pkey1) | 1ul << pkeyshift(pkey2));
+ info->invalid_uamor = info->expected_uamor & ~(0x3ul << pkeyshift(pkey1));
printf("%-30s AMR: %016lx pkey1: %d pkey2: %d pkey3: %d\n",
user_write, info->amr1, pkey1, pkey2, pkey3);
@@ -196,9 +190,9 @@ static int parent(struct shared_info *info, pid_t pid)
PARENT_SKIP_IF_UNSUPPORTED(ret, &info->child_sync);
PARENT_FAIL_IF(ret, &info->child_sync);
- info->amr1 = info->amr2 = info->invalid_amr = regs[0];
- info->expected_iamr = info->invalid_iamr = regs[1];
- info->expected_uamor = info->invalid_uamor = regs[2];
+ info->amr1 = info->amr2 = regs[0];
+ info->expected_iamr = regs[1];
+ info->expected_uamor = regs[2];
/* Wake up child so that it can set itself up. */
ret = prod_child(&info->child_sync);
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v5 26/26] powerpc/selftest/ptrace-pkey: IAMR and uamor cannot be updated by ptrace
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
` (24 preceding siblings ...)
2020-06-19 13:58 ` [PATCH v5 25/26] powerpc/selftest/ptrace-pkey: Update the test to mark an invalid pkey correctly Aneesh Kumar K.V
@ 2020-06-19 13:58 ` Aneesh Kumar K.V
25 siblings, 0 replies; 49+ messages in thread
From: Aneesh Kumar K.V @ 2020-06-19 13:58 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V, linuxram, bauerman
Both IAMR and uamor are privileged and cannot be updated by userspace. Hence
we also don't allow ptrace interface to update them. Don't update them in the
test. Also expected_iamr is only changed if we can allocate a DISABLE_EXECUTE
pkey.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c b/tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c
index bc33d748d95b..5c3c8222de46 100644
--- a/tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c
+++ b/tools/testing/selftests/powerpc/ptrace/ptrace-pkey.c
@@ -101,15 +101,12 @@ static int child(struct shared_info *info)
*/
info->invalid_amr = info->amr2 | (~0x0UL & ~info->expected_uamor);
+ /*
+ * if PKEY_DISABLE_EXECUTE succeeded we should update the expected_iamr
+ */
if (disable_execute)
info->expected_iamr |= 1ul << pkeyshift(pkey1);
- else
- info->expected_iamr &= ~(1ul << pkeyshift(pkey1));
-
- info->expected_iamr &= ~(1ul << pkeyshift(pkey2) | 1ul << pkeyshift(pkey3));
- info->expected_uamor |= 3ul << pkeyshift(pkey1) |
- 3ul << pkeyshift(pkey2);
/*
* Create an IAMR value different from expected value.
* Kernel will reject an IAMR and UAMOR change.
--
2.26.2
^ permalink raw reply related [flat|nested] 49+ messages in thread