powerpc: Memory Protection Keys

.....

arch/powerpc/mm/hash64_64k.c int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid, · line 110

Quoted 11 lines

arch/powerpc/mm/hash64_64k.c +2 -2

 	/*

@@ -116,8 +104,8 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
 		 * On hash insert failure we use old pte value and we don't
 		 * want slot information there if we have a insert failure.
 		 */
-		old_pte &= ~(H_PAGE_HASHPTE | H_PAGE_F_GIX | H_PAGE_F_SECOND);
-		new_pte &= ~(H_PAGE_HASHPTE | H_PAGE_F_GIX | H_PAGE_F_SECOND);
+		old_pte &= ~(H_PAGE_HASHPTE);
+		new_pte &= ~(H_PAGE_HASHPTE);
 		goto htab_insert_hpte;
 	}

With the current path order and above hunk we will breaks the bisect I guess. With the above, when we convert a 64k hpte to 4khpte, since this is the first patch, we
should clear that H_PAGE_F_GIX and H_PAGE_F_SECOND. We still use them
for 64k. I guess you should move this hunk to second patch.

Rearrange 64K PTE bits to free up bits 3, 4, 5 and 6 in the 64K backed HPTE pages. This along with the earlier patch will entirely free up the four bits from 64K PTE. The bit numbers are big-endian as defined in the ISA3.0

This patch does the following change to 64K PTE backed by 64K HPTE.

H_PAGE_F_SECOND (S) which occupied bit 4 moves to the
second part of the pte to bit 60.
H_PAGE_F_GIX (G,I,X) which occupied bit 5, 6 and 7 also

	moves  to  the   second part of the pte to bit 61,
       	62, 63, 64 respectively

since bit 7 is now freed up, we move H_PAGE_BUSY (B) from bit 9 to bit 7.

The second part of the PTE will hold (H_PAGE_F_SECOND|H_PAGE_F_GIX) at bit 60,61,62,63.
NOTE: None of the bits in the secondary PTE were not used by 64k-HPTE backed PTE.

Before the patch, the 64K HPTE backed 64k PTE format was as follows

 0 1 2 3 4  5  6  7  8 9 10...........................63
 : : : : :  :  :  :  : : :                            :
 v v v v v  v  v  v  v v v                            v

,-,-,-,-,--,--,--,--,-,-,-,-,-,------------------,-,-,-, |x|x|x| |S |G |I |X |x|B| |x|x|................|x|x|x|x| <- primary pte '_'_'_'_'__'__'__'__'_'_'_'_'_'________________'_'_'_'_'
| | | | | | | | | | | | |..................| | | | | <- secondary pte '_'_'_'_'__'__'__'__'_'_'_'_'__________________'_'_'_'_'

After the patch, the 64k HPTE backed 64k PTE format is as follows

 0 1 2 3 4  5  6  7  8 9 10...........................63
 : : : : :  :  :  :  : : :                            :
 v v v v v  v  v  v  v v v                            v

,-,-,-,-,--,--,--,--,-,-,-,-,-,------------------,-,-,-, |x|x|x| | | | |B |x| | |x|x|................|.|.|.|.| <- primary pte '_'_'_'_'__'__'__'__'_'_'_'_'_'________________'_'_'_'_'
| | | | | | | | | | | | |..................|S|G|I|X| <- secondary pte '_'_'_'_'__'__'__'__'_'_'_'_'__________________'_'_'_'_'

The above PTE changes is applicable to hugetlbpages aswell.

The patch does the following code changes:

a) moves the H_PAGE_F_SECOND and H_PAGE_F_GIX to 4k PTE
header since it is no more needed b the 64k PTEs.
b) abstracts out __real_pte() and __rpte_to_hidx() so the caller need not know the bit location of the slot.
c) moves the slot bits the secondary pte.

Signed-off-by Ram Pai <[email protected]>

---

Introduce pte_set_hash_slot().It sets the (H_PAGE_F_SECOND|H_PAGE_F_GIX)
bits at the appropriate location in the PTE of 4K PTE. For
64K PTE, it sets the bits in the second part of the PTE. Though the implementation for the former just needs the slot parameter, it does take some additional parameters to keep the prototype consistent.

This function will be handy as we work towards re-arranging the
bits in the later patches.

Signed-off-by Ram Pai <[email protected]>

---
 arch/powerpc/include/asm/book3s/64/hash-4k.h  |   15 +++++++++++++++
 arch/powerpc/include/asm/book3s/64/hash-64k.h |   25 +++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 0 deletions(-)

arch/powerpc/include/asm/book3s/64/hash-4k.h +15 -0

index d2cf949..dc153c6 100644
@@ -53,6 +53,21 @@ static inline int hash__hugepd_ok(hugepd_t hpd)
 }
 #endif
 
+/*
+ * 4k pte format is  different  from  64k  pte  format.  Saving  the
+ * hash_slot is just a matter of returning the pte bits that need to
+ * be modified. On 64k pte, things are a  little  more  involved and
+ * hence  needs   many   more  parameters  to  accomplish  the  same.
+ * However we  want  to abstract this out from the caller by keeping
+ * the prototype consistent across the two formats.
+ */
+static inline unsigned long pte_set_hash_slot(pte_t *ptep, real_pte_t rpte,
+			unsigned int subpg_index, unsigned long slot)
+{
+	return (slot << H_PAGE_F_GIX_SHIFT) &
+		(H_PAGE_F_SECOND | H_PAGE_F_GIX);
+}
+
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 
 static inline char *get_hpte_slot_array(pmd_t *pmdp)

arch/powerpc/include/asm/book3s/64/hash-64k.h +25 -0

index c281f18..89ef5a9 100644
@@ -67,6 +67,31 @@ static inline unsigned long __rpte_to_hidx(real_pte_t rpte, unsigned long index)
 	return ((rpte.hidx >> (index<<2)) & 0xfUL);
 }
 
+/*
+ * Commit the hash slot and return pte bits that needs to be modified.
+ * The caller is expected to modify the pte bits accordingly and
+ * commit the pte to memory.
+ */
+static inline unsigned long pte_set_hash_slot(pte_t *ptep, real_pte_t rpte,
+		unsigned int subpg_index, unsigned long slot)
+{
+	unsigned long *hidxp = (unsigned long *)(ptep + PTRS_PER_PTE);
+
+	rpte.hidx &= ~(0xfUL << (subpg_index << 2));
+	*hidxp = rpte.hidx  | (slot << (subpg_index << 2));
+	/*
+	 * Commit the hidx bits to memory before returning.
+	 * Anyone reading  pte  must  ensure hidx bits are
+	 * read  only  after  reading the pte by using the
+	 * read-side  barrier  smp_rmb(). __real_pte() can
+	 * help ensure that.
+	 */
+	smp_wmb();
+
+	/* no pte bits to be modified, return 0x0UL */
+	return 0x0UL;
+}
+
 #define __rpte_to_pte(r)	((r).pte)
 extern bool __rpte_sub_valid(real_pte_t rpte, unsigned long index);
 /*

Signature · 1 line

1.7.1

Introduce pte_get_hash_gslot()() which returns the slot number of the HPTE in the global hash table.

This function will come in handy as we work towards re-arranging the PTE bits in the later patches.

Signed-off-by Ram Pai <[email protected]>

---
 arch/powerpc/include/asm/book3s/64/hash.h |    3 +++
 arch/powerpc/mm/hash_utils_64.c           |   18 ++++++++++++++++++
 2 files changed, 21 insertions(+), 0 deletions(-)

arch/powerpc/include/asm/book3s/64/hash.h +3 -0

index d27f885..277158c 100644
@@ -156,6 +156,9 @@ static inline int hash__pte_none(pte_t pte)
 	return (pte_val(pte) & ~H_PTE_NONE_MASK) == 0;
 }
 
+unsigned long pte_get_hash_gslot(unsigned long vpn, unsigned long shift,
+		int ssize, real_pte_t rpte, unsigned int subpg_index);
+
 /* This low level function performs the actual PTE insertion
  * Setting the PTE depends on the MMU type and other factors. It's
  * an horrible mess that I'm not going to try to clean up now but

arch/powerpc/mm/hash_utils_64.c +18 -0

index 1b494d0..d3604da 100644
@@ -1591,6 +1591,24 @@ static inline void tm_flush_hash_page(int local)
 }
 #endif
 
+/*
+ * return the global hash slot, corresponding to the given
+ * pte, which contains the hpte.
+ */
+unsigned long pte_get_hash_gslot(unsigned long vpn, unsigned long shift,
+		int ssize, real_pte_t rpte, unsigned int subpg_index)
+{
+	unsigned long hash, slot, hidx;
+
+	hash = hpt_hash(vpn, shift, ssize);
+	hidx = __rpte_to_hidx(rpte, subpg_index);
+	if (hidx & _PTEIDX_SECONDARY)
+		hash = ~hash;
+	slot = (hash & htab_hash_mask) * HPTES_PER_GROUP;
+	slot += hidx & _PTEIDX_GROUP_IX;
+	return slot;
+}
+
 /* WARNING: This is called from hash_low_64.S, if you change this prototype,
  *          do not forget to update the assembly call site !
  */

Signature · 1 line

1.7.1

The H_PAGE_F_SECOND,H_PAGE_F_GIX are not in the 64K main-PTE. capture these changes in the dump pte report.

Signed-off-by Ram Pai <[email protected]>

---
 arch/powerpc/mm/dump_linuxpagetables.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

arch/powerpc/mm/dump_linuxpagetables.c +2 -1

index 44fe483..5627edd 100644
@@ -213,7 +213,7 @@ struct flag_info {
 		.val	= H_PAGE_4K_PFN,
 		.set	= "4K_pfn",
 	}, {
-#endif
+#else /* CONFIG_PPC_64K_PAGES */
 		.mask	= H_PAGE_F_GIX,
 		.val	= H_PAGE_F_GIX,
 		.set	= "f_gix",

@@ -224,6 +224,7 @@ struct flag_info {
 		.val	= H_PAGE_F_SECOND,
 		.set	= "f_second",
 	}, {
+#endif /* CONFIG_PPC_64K_PAGES */
 #endif
 		.mask	= _PAGE_SPECIAL,
 		.val	= _PAGE_SPECIAL,

Signature · 1 line

1.7.1

replace redundant code in __hash_page_64K() with helper functions pte_get_hash_gslot() and pte_set_hash_slot()

Signed-off-by Ram Pai <[email protected]>

---
 arch/powerpc/mm/hash64_64k.c |   24 ++++--------------------
 1 files changed, 4 insertions(+), 20 deletions(-)

arch/powerpc/mm/hash64_64k.c +4 -20

index 0012618..645f621 100644
@@ -244,7 +244,6 @@ int __hash_page_64K(unsigned long ea, unsigned long access,
 		    unsigned long flags, int ssize)
 {
 	real_pte_t rpte;
-	unsigned long *hidxp;
 	unsigned long hpte_group;
 	unsigned long rflags, pa;
 	unsigned long old_pte, new_pte;

@@ -289,18 +288,12 @@ int __hash_page_64K(unsigned long ea, unsigned long access,
 
 	vpn  = hpt_vpn(ea, vsid, ssize);
 	if (unlikely(old_pte & H_PAGE_HASHPTE)) {
-		unsigned long hash, slot, hidx;
-
-		hash = hpt_hash(vpn, shift, ssize);
-		hidx = __rpte_to_hidx(rpte, 0);
-		if (hidx & _PTEIDX_SECONDARY)
-			hash = ~hash;
-		slot = (hash & htab_hash_mask) * HPTES_PER_GROUP;
-		slot += hidx & _PTEIDX_GROUP_IX;
+		unsigned long gslot;
 		/*
 		 * There MIGHT be an HPTE for this pte
 		 */
-		if (mmu_hash_ops.hpte_updatepp(slot, rflags, vpn, MMU_PAGE_64K,
+		gslot = pte_get_hash_gslot(vpn, shift, ssize, rpte, 0);
+		if (mmu_hash_ops.hpte_updatepp(gslot, rflags, vpn, MMU_PAGE_64K,
 					       MMU_PAGE_64K, ssize,
 					       flags) == -1)
 			old_pte &= ~_PAGE_HPTEFLAGS;

@@ -350,17 +343,8 @@ int __hash_page_64K(unsigned long ea, unsigned long access,
 			return -1;
 		}
 
-		/*
-		 * Insert slot number & secondary bit in PTE second half.
-		 */
-		hidxp = (unsigned long *)(ptep + PTRS_PER_PTE);
-		rpte.hidx &= ~(0xfUL);
-		*hidxp = rpte.hidx  | (slot & 0xfUL);
-		/*
-		 * check __real_pte for details on matching smp_rmb()
-		 */
-		smp_wmb();
 		new_pte = (new_pte & ~_PAGE_HPTEFLAGS) | H_PAGE_HASHPTE;
+		new_pte |= pte_set_hash_slot(ptep, rpte, 0, slot);
 	}
 	*ptep = __pte(new_pte & ~H_PAGE_BUSY);
 	return 0;

Signature · 1 line

1.7.1

replace redundant code in __hash_page_huge() with helper functions pte_get_hash_gslot() and pte_set_hash_slot()

Signed-off-by Ram Pai <[email protected]>

---
 arch/powerpc/mm/hugetlbpage-hash64.c |   24 ++++--------------------
 1 files changed, 4 insertions(+), 20 deletions(-)

arch/powerpc/mm/hugetlbpage-hash64.c +4 -20

index 6f7aee3..e6dcd50 100644
@@ -23,7 +23,6 @@ int __hash_page_huge(unsigned long ea, unsigned long access, unsigned long vsid,
 		     int ssize, unsigned int shift, unsigned int mmu_psize)
 {
 	real_pte_t rpte;
-	unsigned long *hidxp;
 	unsigned long vpn;
 	unsigned long old_pte, new_pte;
 	unsigned long rflags, pa, sz;

@@ -74,16 +73,10 @@ int __hash_page_huge(unsigned long ea, unsigned long access, unsigned long vsid,
 	/* Check if pte already has an hpte (case 2) */
 	if (unlikely(old_pte & H_PAGE_HASHPTE)) {
 		/* There MIGHT be an HPTE for this pte */
-		unsigned long hash, slot, hidx;
+		unsigned long gslot;
 
-		hash = hpt_hash(vpn, shift, ssize);
-		hidx = __rpte_to_hidx(rpte, 0);
-		if (hidx & _PTEIDX_SECONDARY)
-			hash = ~hash;
-		slot = (hash & htab_hash_mask) * HPTES_PER_GROUP;
-		slot += hidx & _PTEIDX_GROUP_IX;
-
-		if (mmu_hash_ops.hpte_updatepp(slot, rflags, vpn, mmu_psize,
+		gslot = pte_get_hash_gslot(vpn, shift, ssize, rpte, 0);
+		if (mmu_hash_ops.hpte_updatepp(gslot, rflags, vpn, mmu_psize,
 					       mmu_psize, ssize, flags) == -1)
 			old_pte &= ~_PAGE_HPTEFLAGS;
 	}

@@ -110,16 +103,7 @@ int __hash_page_huge(unsigned long ea, unsigned long access, unsigned long vsid,
 			return -1;
 		}
 
-		/*
-		 * Insert slot number & secondary bit in PTE second half.
-		 */
-		hidxp = (unsigned long *)(ptep + PTRS_PER_PTE);
-		rpte.hidx &= ~(0xfUL);
-		*hidxp = rpte.hidx  | (slot & 0xfUL);
-		/*
-		 * check __real_pte for details on matching smp_rmb()
-		 */
-		smp_wmb();
+		new_pte |= pte_set_hash_slot(ptep, rpte, 0, slot);
 	}
 
 	/*

Signature · 1 line

1.7.1

basic setup to initialize the pkey system. Only 64K kernel in HPT mode, enables the pkey system.

Signed-off-by Ram Pai <[email protected]>

---
 arch/powerpc/Kconfig                   |   16 ++++++++++
 arch/powerpc/include/asm/mmu_context.h |    5 +++
 arch/powerpc/include/asm/pkeys.h       |   51 ++++++++++++++++++++++++++++++++
 arch/powerpc/kernel/setup_64.c         |    4 ++
 arch/powerpc/mm/Makefile               |    1 +
 arch/powerpc/mm/hash_utils_64.c        |    1 +
 arch/powerpc/mm/pkeys.c                |   18 +++++++++++
 7 files changed, 96 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/include/asm/pkeys.h
 create mode 100644 arch/powerpc/mm/pkeys.c

arch/powerpc/Kconfig +7 -0

index bf4391d..5c60fd6 100644
@@ -855,6 +855,22 @@ config SECCOMP
 
 	  If unsure, say Y. Only embedded should say N here.
 
+config PPC64_MEMORY_PROTECTION_KEYS
+	prompt "PowerPC Memory Protection Keys"
+	def_bool y
+	# Note: only available in 64-bit mode
+	depends on PPC64 && PPC_64K_PAGES
+	select ARCH_USES_HIGH_VMA_FLAGS
+	select ARCH_HAS_PKEYS

Quoted 38 lines · Ram Pai

basic setup to initialize the pkey system. Only 64K kernel in HPT mode, enables the pkey system.

Signed-off-by Ram Pai <[email protected]>

---
 arch/powerpc/Kconfig                   |   16 ++++++++++
 arch/powerpc/include/asm/mmu_context.h |    5 +++
 arch/powerpc/include/asm/pkeys.h       |   51 ++++++++++++++++++++++++++++++++
 arch/powerpc/kernel/setup_64.c         |    4 ++
 arch/powerpc/mm/Makefile               |    1 +
 arch/powerpc/mm/hash_utils_64.c        |    1 +
 arch/powerpc/mm/pkeys.c                |   18 +++++++++++
 7 files changed, 96 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/include/asm/pkeys.h
 create mode 100644 arch/powerpc/mm/pkeys.c

arch/powerpc/Kconfig +14 -0

index bf4391d..5c60fd6 100644
@@ -855,6 +855,22 @@ config SECCOMP
 
 	  If unsure, say Y. Only embedded should say N here.
 
+config PPC64_MEMORY_PROTECTION_KEYS
+	prompt "PowerPC Memory Protection Keys"
+	def_bool y
+	# Note: only available in 64-bit mode
+	depends on PPC64 && PPC_64K_PAGES
+	select ARCH_USES_HIGH_VMA_FLAGS
+	select ARCH_HAS_PKEYS
+	---help---
+	  Memory Protection Keys provides a mechanism for enforcing
+	  page-based protections, but without requiring modification of the
+	  page tables when an application changes protection domains.
+
+	  For details, see Documentation/vm/protection-keys.txt
+

arch/powerpc/Kconfig config SECCOMP · line 874

Quoted 3 lines

arch/powerpc/Kconfig +2 -0

+	  If unsure, say y.
+
 endmenu

Shouldn't this come as the last patch ? Or can we enable this config by this patch ? If so what does it do ? Did you test boot each of this patch to make sure we don't break git bisect ?

arch/powerpc/include/asm/pkeys.h +2 -1

index 203d7de..09b268e 100644
@@ -2,21 +2,87 @@
 #define _ASM_PPC64_PKEYS_H
 
 extern bool pkey_inited;
-#define ARCH_VM_PKEY_FLAGS 0
+#define arch_max_pkey()  32
+#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \

Hello Ram,

I'm still going through the patches and haven't formed a full picture of the feature in my mind yet, so my comments today won't be particularly insightful...

But hopefully the comments that I currently have will be helpful anyway.

Quoted 7 lines · Ram Pai

arch/powerpc/include/asm/pkeys.h +0 -0

index 203d7de..09b268e 100644
@@ -2,21 +2,87 @@
 #define _ASM_PPC64_PKEYS_H
 

Quoted 4 lines

arch/powerpc/include/asm/pkeys.h +2 -1

 extern bool pkey_inited;
-#define ARCH_VM_PKEY_FLAGS 0
+#define arch_max_pkey()  32
+#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \

arch/powerpc/include/asm/pkeys.h line 16

Quoted 10 lines

arch/powerpc/include/asm/pkeys.h +10 -0

+				VM_PKEY_BIT3 | VM_PKEY_BIT4)
+/*
+ * Bits are in BE format.
+ * NOTE: key 31, 1, 0 are not used.
+ * key 0 is used by default. It give read/write/execute permission.
+ * key 31 is reserved by the hypervisor.
+ * key 1 is recommended to be not used.
+ * PowerISA(3.0) page 1015, programming note.
+ */
+#define PKEY_INITIAL_ALLOCAION  0xc0000001

There's a typo in the macro name, should be "ALLOCATION".

Signature · 3 lines

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

arch/powerpc/include/asm/pkeys.h +7 -0

+
+	if (!mm_pkey_is_allocated(mm, pkey))
+		return -EINVAL;
+
+	mm_set_pkey_free(mm, pkey);
+
+	return 0;
 }

Introduce helper functions that can initialize the bits in the AMR, IAMR and UMOR register; the bits that correspond to the given pkey.

Signed-off-by Ram Pai <[email protected]>

arch/powerpc/mm/pkeys.c +7 -0

@@ -16,3 +16,47 @@
 #include <linux/pkeys.h>                /* PKEY_*                       */
 
 bool pkey_inited;
+#define pkeyshift(pkey) ((arch_max_pkey()-pkey-1) * AMR_BITS_PER_PKEY)
+
+static inline void init_amr(int pkey, u8 init_bits)
+{
+	u64 new_amr_bits = (((u64)init_bits & 0x3UL) << pkeyshift(pkey));
+	u64 old_amr = read_amr() & ~((u64)(0x3ul) << pkeyshift(pkey));
+

Quoted 4 lines · Ram Pai

Introduce helper functions that can initialize the bits in the AMR, IAMR and UMOR register; the bits that correspond to the given pkey.

Signed-off-by Ram Pai <[email protected]>

s/UMOR/UAMOR/ here and in the subject as well.

Quoted 18 lines

arch/powerpc/mm/pkeys.c +11 -0

@@ -16,3 +16,47 @@
 #include <linux/pkeys.h>                /* PKEY_*                       */
 
 bool pkey_inited;
+#define pkeyshift(pkey) ((arch_max_pkey()-pkey-1) * AMR_BITS_PER_PKEY)
+
+static inline void init_amr(int pkey, u8 init_bits)
+{
+	u64 new_amr_bits = (((u64)init_bits & 0x3UL) << pkeyshift(pkey));
+	u64 old_amr = read_amr() & ~((u64)(0x3ul) << pkeyshift(pkey));
+
+	write_amr(old_amr | new_amr_bits);
+}
+
+static inline void init_iamr(int pkey, u8 init_bits)

arch/powerpc/mm/pkeys.c line 35

Quoted 6 lines

arch/powerpc/mm/pkeys.c +6 -0

+{
+	u64 new_iamr_bits = (((u64)init_bits & 0x3UL) << pkeyshift(pkey));
+	u64 old_iamr = read_iamr() & ~((u64)(0x3ul) << pkeyshift(pkey));
+
+	write_amr(old_iamr | new_iamr_bits);
+}

init_iamr should call write_iamr, not write_amr.

arch/powerpc/include/asm/pkeys.h +0 -0

@@ -113,10 +117,14 @@ static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,
 	return 0;
 }
 

Quoted 4 lines · Ram Pai

arch/powerpc/include/asm/pkeys.h +0 -0

@@ -113,10 +117,14 @@ static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,
 	return 0;
 }
 

arch/powerpc/include/asm/pkeys.h static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma, · line 128

Quoted 10 lines

arch/powerpc/include/asm/pkeys.h +5 -1

+extern int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
+		unsigned long init_val);
 static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 		unsigned long init_val)
 {
-	return 0;
+	if (!pkey_inited)
+		return -1;
+	return __arch_set_user_pkey_access(tsk, pkey, init_val);
 }

If non-zero, the return value of this function will be passed to userspace by the pkey_alloc syscall. Shouldn't it be returning an errno macro such as -EPERM?

Also, why are there both arch_set_user_pkey_access and __arch_set_user_pkey_access? Is it a speed optimization so that the early return is inlined into the caller? Ditto for execute_only_pkey and __arch_override_mprotect_pkey.

arch/powerpc/include/asm/pkeys.h +2 -0

@@ -2,6 +2,18 @@
 #define _ASM_PPC64_PKEYS_H
 
 extern bool pkey_inited;
+/* override any generic PKEY Permission defines */
+#undef  PKEY_DISABLE_ACCESS

arch/powerpc/mm/pkeys.c +2 -0

index 98d0391..b9ad98d 100644
@@ -73,6 +73,7 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 		unsigned long init_val)
 {
 	u64 new_amr_bits = 0x0ul;
+	u64 new_iamr_bits = 0x0ul;
 
 	if (!is_pkey_enabled(pkey))
 		return -1;

@@ -85,5 +86,14 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 
 	init_amr(pkey, new_amr_bits);
 
+	/*

arch/powerpc/include/asm/pkeys.h +0 -11

index e31f5ee8e81f..67e6a3a343ae 100644
@@ -4,17 +4,6 @@
 #include <asm/firmware.h>
 
 extern bool pkey_inited;
-/* override any generic PKEY Permission defines */
-#undef  PKEY_DISABLE_ACCESS
-#define PKEY_DISABLE_ACCESS    0x1
-#undef  PKEY_DISABLE_WRITE
-#define PKEY_DISABLE_WRITE     0x2
-#undef  PKEY_DISABLE_EXECUTE
-#define PKEY_DISABLE_EXECUTE   0x4
-#undef  PKEY_ACCESS_MASK
-#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |\
-				PKEY_DISABLE_WRITE  |\
-				PKEY_DISABLE_EXECUTE)
 
 #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
 				VM_PKEY_BIT3 | VM_PKEY_BIT4)

arch/powerpc/include/uapi/asm/mman.h +2 -0

index ab45cc2f3101..dee43feb7c53 100644
@@ -45,4 +45,6 @@
 #define MAP_HUGE_1GB	(30 << MAP_HUGE_SHIFT)	/* 1GB   HugeTLB Page */
 #define MAP_HUGE_16GB	(34 << MAP_HUGE_SHIFT)	/* 16GB  HugeTLB Page */
 
+#define PKEY_DISABLE_EXECUTE   0x4
+
 #endif /* _UAPI_ASM_POWERPC_MMAN_H */

arch/powerpc/mm/pkeys.c +1 -1

index 72eb9a1bde79..777f8f8dff47 100644
@@ -12,7 +12,7 @@
  * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
  * more details.
  */
-#include <uapi/asm-generic/mman-common.h>
+#include <asm/mman.h>
 #include <linux/pkeys.h>                /* PKEY_*                       */
 
 bool pkey_inited;

include/uapi/asm-generic/mman-common.h +2 -0

index 8c27db0c5c08..93e3841d9ada 100644
@@ -74,7 +74,15 @@
 
 #define PKEY_DISABLE_ACCESS	0x1
 #define PKEY_DISABLE_WRITE	0x2
+
+/* The arch-specific code may define PKEY_DISABLE_EXECUTE */

Quoted 8 lines · Ram Pai

arch/powerpc/include/asm/pkeys.h +2 -0

@@ -2,6 +2,18 @@
 #define _ASM_PPC64_PKEYS_H
 
 extern bool pkey_inited;
+/* override any generic PKEY Permission defines */
+#undef  PKEY_DISABLE_ACCESS

arch/powerpc/include/asm/pkeys.h line 16

Quoted 10 lines

arch/powerpc/include/asm/pkeys.h +10 -0

+#define PKEY_DISABLE_ACCESS    0x1
+#undef  PKEY_DISABLE_WRITE
+#define PKEY_DISABLE_WRITE     0x2
+#undef  PKEY_DISABLE_EXECUTE
+#define PKEY_DISABLE_EXECUTE   0x4
+#undef  PKEY_ACCESS_MASK
+#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |\
+				PKEY_DISABLE_WRITE  |\
+				PKEY_DISABLE_EXECUTE)
+

Is it ok to #undef macros from another header? Especially since said header is in uapi (include/uapi/asm-generic/mman-common.h).

Also, it's unnecessary to undef the _ACCESS and _WRITE macros since they are identical to the original definition. And since these macros are originally defined in an uapi header, the powerpc-specific ones should be in an uapi header as well, if I understand it correctly.

An alternative solution is to define only PKEY_DISABLE_EXECUTE in arch/powerpc/include/uapi/asm/mman.h and then test for its existence to properly define PKEY_ACCESS_MASK in include/uapi/asm-generic/mman-common.h. What do you think of the code below?

arch/powerpc/include/asm/pkeys.h +0 -11

index e31f5ee8e81f..67e6a3a343ae 100644
@@ -4,17 +4,6 @@
 #include <asm/firmware.h>
 
 extern bool pkey_inited;
-/* override any generic PKEY Permission defines */
-#undef  PKEY_DISABLE_ACCESS
-#define PKEY_DISABLE_ACCESS    0x1
-#undef  PKEY_DISABLE_WRITE
-#define PKEY_DISABLE_WRITE     0x2
-#undef  PKEY_DISABLE_EXECUTE
-#define PKEY_DISABLE_EXECUTE   0x4
-#undef  PKEY_ACCESS_MASK
-#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |\
-				PKEY_DISABLE_WRITE  |\
-				PKEY_DISABLE_EXECUTE)
 
 #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
 				VM_PKEY_BIT3 | VM_PKEY_BIT4)

arch/powerpc/include/uapi/asm/mman.h +2 -0

index ab45cc2f3101..dee43feb7c53 100644
@@ -45,4 +45,6 @@
 #define MAP_HUGE_1GB	(30 << MAP_HUGE_SHIFT)	/* 1GB   HugeTLB Page */
 #define MAP_HUGE_16GB	(34 << MAP_HUGE_SHIFT)	/* 16GB  HugeTLB Page */
 
+#define PKEY_DISABLE_EXECUTE   0x4
+
 #endif /* _UAPI_ASM_POWERPC_MMAN_H */

arch/powerpc/mm/pkeys.c +1 -1

index 72eb9a1bde79..777f8f8dff47 100644
@@ -12,7 +12,7 @@
  * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
  * more details.
  */
-#include <uapi/asm-generic/mman-common.h>
+#include <asm/mman.h>
 #include <linux/pkeys.h>                /* PKEY_*                       */
 
 bool pkey_inited;

include/uapi/asm-generic/mman-common.h +2 -0

index 8c27db0c5c08..93e3841d9ada 100644
@@ -74,7 +74,15 @@
 
 #define PKEY_DISABLE_ACCESS	0x1
 #define PKEY_DISABLE_WRITE	0x2
+
+/* The arch-specific code may define PKEY_DISABLE_EXECUTE */

Quoted 19 lines

arch/powerpc/mm/pkeys.c +2 -0

index 98d0391..b9ad98d 100644
@@ -73,6 +73,7 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 		unsigned long init_val)
 {
 	u64 new_amr_bits = 0x0ul;
+	u64 new_iamr_bits = 0x0ul;
 
 	if (!is_pkey_enabled(pkey))
 		return -1;

@@ -85,5 +86,14 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 
 	init_amr(pkey, new_amr_bits);
 
+	/*

arch/powerpc/mm/pkeys.c int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey, · line 99

Quoted 10 lines

arch/powerpc/mm/pkeys.c +8 -0

+	 * By default execute is disabled.
+	 * To enable execute, PKEY_ENABLE_EXECUTE
+	 * needs to be specified.
+	 */
+	if ((init_val & PKEY_DISABLE_EXECUTE))
+		new_iamr_bits |= IAMR_EX_BIT;
+
+	init_iamr(pkey, new_iamr_bits);
 	return 0;
 }

The comment seems to be from an earlier version which has the logic inverted, and there is no PKEY_ENABLE_EXECUTE. Should the comment be updated to the following?

By default execute is enabled.
To disable execute, PKEY_DISABLE_EXECUTE needs to be specified.

Signature · 3 lines

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Quoted 25 lines · Thiago Jung Bauermann

Quoted 8 lines · Ram Pai

arch/powerpc/include/asm/pkeys.h +2 -0

@@ -2,6 +2,18 @@
 #define _ASM_PPC64_PKEYS_H
 
 extern bool pkey_inited;
+/* override any generic PKEY Permission defines */
+#undef  PKEY_DISABLE_ACCESS

arch/powerpc/include/asm/pkeys.h line 16

Quoted 10 lines

arch/powerpc/include/asm/pkeys.h +10 -0

+#define PKEY_DISABLE_ACCESS    0x1
+#undef  PKEY_DISABLE_WRITE
+#define PKEY_DISABLE_WRITE     0x2
+#undef  PKEY_DISABLE_EXECUTE
+#define PKEY_DISABLE_EXECUTE   0x4
+#undef  PKEY_ACCESS_MASK
+#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |\
+				PKEY_DISABLE_WRITE  |\
+				PKEY_DISABLE_EXECUTE)
+

Is it ok to #undef macros from another header? Especially since said header is in uapi (include/uapi/asm-generic/mman-common.h).

Quoted 4 lines

Also, it's unnecessary to undef the _ACCESS and _WRITE macros since they are identical to the original definition. And since these macros are originally defined in an uapi header, the powerpc-specific ones should be in an uapi header as well, if I understand it correctly.

The architectural neutral code allows the implementation to define the macros to its taste. powerpc headers due to legacy reason includes the include/uapi/asm-generic/mman-common.h header. That header includes the generic definitions of only PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE. Unfortunately we end up importing them. I dont want to depend on them. Any changes there could effect us. Example if the generic uapi header changed PKEY_DISABLE_ACCESS to 0x4, we will have a conflict with PKEY_DISABLE_EXECUTE. Hence I undef them and define the it my way.

Store and restore the AMR, IAMR and UMOR register state of the task before scheduling out and after scheduling in, respectively.

Signed-off-by Ram Pai <[email protected]>

arch/powerpc/kernel/process.c +5 -0

index 2ad725e..9429361 100644
@@ -1096,6 +1096,11 @@ static inline void save_sprs(struct thread_struct *t)
 		t->tar = mfspr(SPRN_TAR);
 	}
 #endif
+#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
+	t->amr = mfspr(SPRN_AMR);
+	t->iamr = mfspr(SPRN_IAMR);
+	t->uamor = mfspr(SPRN_UAMOR);
+#endif
 }
 
 static inline void restore_sprs(struct thread_struct *old_thread,

@@ -1131,6 +1136,14 @@ static inline void restore_sprs(struct thread_struct *old_thread,
 			mtspr(SPRN_TAR, new_thread->tar);
 	}

Quoted 4 lines · Ram Pai

Store and restore the AMR, IAMR and UMOR register state of the task before scheduling out and after scheduling in, respectively.

Signed-off-by Ram Pai <[email protected]>

s/UMOR/UAMOR/

Quoted 25 lines

arch/powerpc/kernel/process.c +10 -0

index 2ad725e..9429361 100644
@@ -1096,6 +1096,11 @@ static inline void save_sprs(struct thread_struct *t)
 		t->tar = mfspr(SPRN_TAR);
 	}
 #endif
+#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
+	t->amr = mfspr(SPRN_AMR);
+	t->iamr = mfspr(SPRN_IAMR);
+	t->uamor = mfspr(SPRN_UAMOR);
+#endif
 }
 
 static inline void restore_sprs(struct thread_struct *old_thread,

@@ -1131,6 +1136,14 @@ static inline void restore_sprs(struct thread_struct *old_thread,
 			mtspr(SPRN_TAR, new_thread->tar);
 	}
 #endif
+#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
+	if (old_thread->amr != new_thread->amr)
+		mtspr(SPRN_AMR, new_thread->amr);
+	if (old_thread->iamr != new_thread->iamr)
+		mtspr(SPRN_IAMR, new_thread->iamr);

arch/powerpc/kernel/process.c static inline void restore_sprs(struct thread_struct *old_thread, · line 1147

Quoted 4 lines

arch/powerpc/kernel/process.c +3 -0

+	if (old_thread->uamor != new_thread->uamor)
+		mtspr(SPRN_UAMOR, new_thread->uamor);
+#endif
 }

Shouldn't the saving and restoring of the SPRs be guarded by a check for whether memory protection keys are enabled? What happens when trying to access these registers on a CPU which doesn't have them?

Quoted 26 lines · Thiago Jung Bauermann

Quoted 25 lines · Ram Pai

arch/powerpc/kernel/process.c +10 -0

index 2ad725e..9429361 100644
@@ -1096,6 +1096,11 @@ static inline void save_sprs(struct thread_struct *t)
 		t->tar = mfspr(SPRN_TAR);
 	}
 #endif
+#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
+	t->amr = mfspr(SPRN_AMR);
+	t->iamr = mfspr(SPRN_IAMR);
+	t->uamor = mfspr(SPRN_UAMOR);
+#endif
 }
 
 static inline void restore_sprs(struct thread_struct *old_thread,

@@ -1131,6 +1136,14 @@ static inline void restore_sprs(struct thread_struct *old_thread,
 			mtspr(SPRN_TAR, new_thread->tar);
 	}
 #endif
+#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
+	if (old_thread->amr != new_thread->amr)
+		mtspr(SPRN_AMR, new_thread->amr);
+	if (old_thread->iamr != new_thread->iamr)
+		mtspr(SPRN_IAMR, new_thread->iamr);

Quoted 8 lines

arch/powerpc/kernel/process.c static inline void restore_sprs(struct thread_struct *old_thread, · line 1147

Quoted 4 lines

arch/powerpc/kernel/process.c +3 -0

+	if (old_thread->uamor != new_thread->uamor)
+		mtspr(SPRN_UAMOR, new_thread->uamor);
+#endif
 }

Shouldn't the saving and restoring of the SPRs be guarded by a check for whether memory protection keys are enabled? What happens when trying to access these registers on a CPU which doesn't have them?

Good point. need to guard it. However; i think, these registers have been available since power6.

arch/powerpc/mm/pkeys.c +47 -0

@@ -97,3 +97,60 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 	init_iamr(pkey, new_iamr_bits);
 	return 0;
 }
+
+static inline bool pkey_allows_readwrite(int pkey)
+{
+	int pkey_shift = pkeyshift(pkey);
+
+	if (!(read_uamor() & (0x3UL << pkey_shift)))
+		return true;
+
+	return !(read_amr() & ((AMR_RD_BIT|AMR_WR_BIT) << pkey_shift));
+}
+
+int __execute_only_pkey(struct mm_struct *mm)
+{
+	bool need_to_set_mm_pkey = false;
+	int execute_only_pkey = mm->context.execute_only_pkey;
+	int ret;
+
+	/* Do we need to assign a pkey for mm's execute-only maps? */
+	if (execute_only_pkey == -1) {
+		/* Go allocate one to use, which might fail */
+		execute_only_pkey = mm_pkey_alloc(mm);
+		if (execute_only_pkey < 0)
+			return -1;
+		need_to_set_mm_pkey = true;
+	}
+
+	/*
+	 * We do not want to go through the relatively costly
+	 * dance to set AMR if we do not need to.  Check it
+	 * first and assume that if the execute-only pkey is
+	 * readwrite-disabled than we do not have to set it
+	 * ourselves.
+	 */
+	if (!need_to_set_mm_pkey &&
+	    !pkey_allows_readwrite(execute_only_pkey))
+		return execute_only_pkey;
+
+	/*
+	 * Set up AMR so that it denies access for everything
+	 * other than execution.
+	 */
+	ret = __arch_set_user_pkey_access(current, execute_only_pkey,
+			(PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE));
+	/*
+	 * If the AMR-set operation failed somehow, just return
+	 * 0 and effectively disable execute-only support.
+	 */

Quoted 33 lines · Ram Pai

arch/powerpc/mm/pkeys.c +27 -0

@@ -97,3 +97,60 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 	init_iamr(pkey, new_iamr_bits);
 	return 0;
 }
+
+static inline bool pkey_allows_readwrite(int pkey)
+{
+	int pkey_shift = pkeyshift(pkey);
+
+	if (!(read_uamor() & (0x3UL << pkey_shift)))
+		return true;
+
+	return !(read_amr() & ((AMR_RD_BIT|AMR_WR_BIT) << pkey_shift));
+}
+
+int __execute_only_pkey(struct mm_struct *mm)
+{
+	bool need_to_set_mm_pkey = false;
+	int execute_only_pkey = mm->context.execute_only_pkey;
+	int ret;
+
+	/* Do we need to assign a pkey for mm's execute-only maps? */
+	if (execute_only_pkey == -1) {
+		/* Go allocate one to use, which might fail */
+		execute_only_pkey = mm_pkey_alloc(mm);
+		if (execute_only_pkey < 0)
+			return -1;
+		need_to_set_mm_pkey = true;
+	}
+
+	/*

arch/powerpc/mm/pkeys.c int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey, · line 134

Quoted 8 lines

arch/powerpc/mm/pkeys.c +8 -0

+	 * We do not want to go through the relatively costly
+	 * dance to set AMR if we do not need to.  Check it
+	 * first and assume that if the execute-only pkey is
+	 * readwrite-disabled than we do not have to set it
+	 * ourselves.
+	 */
+	if (!need_to_set_mm_pkey &&
+	    !pkey_allows_readwrite(execute_only_pkey))

arch/powerpc/mm/pkeys.c int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey, · line 142

Quoted 8 lines

arch/powerpc/mm/pkeys.c +8 -0

+		return execute_only_pkey;
+
+	/*
+	 * Set up AMR so that it denies access for everything
+	 * other than execution.
+	 */
+	ret = __arch_set_user_pkey_access(current, execute_only_pkey,
+			(PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE));

arch/powerpc/mm/pkeys.c int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey, · line 148

Quoted 6 lines

arch/powerpc/mm/pkeys.c +6 -0

+	/*
+	 * If the AMR-set operation failed somehow, just return
+	 * 0 and effectively disable execute-only support.
+	 */
+	if (ret) {
+		mm_set_pkey_free(mm, execute_only_pkey);

Quoted 7 lines

arch/powerpc/mm/pkeys.c +7 -0

+		return -1;
+	}
+
+	/* We got one, store it and use it from here on out */
+	if (need_to_set_mm_pkey)
+		mm->context.execute_only_pkey = execute_only_pkey;
+	return execute_only_pkey;

arch/powerpc/mm/pkeys.c int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey, · line 156

Quoted 1 line

arch/powerpc/mm/pkeys.c +1 -0

+}

If you follow the code flow in __execute_only_pkey, the AMR and UAMOR are read 3 times in total, and AMR is written twice. IAMR is read and written twice. Since they are SPRs and access to them is slow (or isn't it?), is it worth it to read them once in __execute_only_pkey and pass down their values to the callees, and then write them once at the end of the function?

Quoted 37 lines · Thiago Jung Bauermann

Quoted 35 lines · Ram Pai

arch/powerpc/mm/pkeys.c +29 -0

@@ -97,3 +97,60 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 	init_iamr(pkey, new_iamr_bits);
 	return 0;
 }
+
+static inline bool pkey_allows_readwrite(int pkey)
+{
+	int pkey_shift = pkeyshift(pkey);
+
+	if (!(read_uamor() & (0x3UL << pkey_shift)))
+		return true;
+
+	return !(read_amr() & ((AMR_RD_BIT|AMR_WR_BIT) << pkey_shift));
+}
+
+int __execute_only_pkey(struct mm_struct *mm)
+{
+	bool need_to_set_mm_pkey = false;
+	int execute_only_pkey = mm->context.execute_only_pkey;
+	int ret;
+
+	/* Do we need to assign a pkey for mm's execute-only maps? */
+	if (execute_only_pkey == -1) {
+		/* Go allocate one to use, which might fail */
+		execute_only_pkey = mm_pkey_alloc(mm);
+		if (execute_only_pkey < 0)
+			return -1;
+		need_to_set_mm_pkey = true;
+	}
+
+	/*
+	 * We do not want to go through the relatively costly
+	 * dance to set AMR if we do not need to.  Check it

Quoted 6 lines

arch/powerpc/mm/pkeys.c int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey, · line 134

Quoted 6 lines

arch/powerpc/mm/pkeys.c +6 -0

+	 * first and assume that if the execute-only pkey is
+	 * readwrite-disabled than we do not have to set it
+	 * ourselves.
+	 */
+	if (!need_to_set_mm_pkey &&
+	    !pkey_allows_readwrite(execute_only_pkey))

		^^^^^
	Here uamor and amr is read once each.

Quoted 8 lines

arch/powerpc/mm/pkeys.c int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey, · line 142

Quoted 8 lines

arch/powerpc/mm/pkeys.c +8 -0

+		return execute_only_pkey;
+
+	/*
+	 * Set up AMR so that it denies access for everything
+	 * other than execution.
+	 */
+	ret = __arch_set_user_pkey_access(current, execute_only_pkey,
+			(PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE));

^^^^^^^
here amr and iamr are written once each if the the function returns successfully.

Quoted 6 lines

arch/powerpc/mm/pkeys.c int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey, · line 148

Quoted 6 lines

arch/powerpc/mm/pkeys.c +6 -0

+	/*
+	 * If the AMR-set operation failed somehow, just return
+	 * 0 and effectively disable execute-only support.
+	 */
+	if (ret) {
+		mm_set_pkey_free(mm, execute_only_pkey);

^^^
here only if __arch_set_user_pkey_access() fails amr and iamr and uamor will be written once each.

Quoted 7 lines

Quoted 7 lines

arch/powerpc/mm/pkeys.c +7 -0

+		return -1;
+	}
+
+	/* We got one, store it and use it from here on out */
+	if (need_to_set_mm_pkey)
+		mm->context.execute_only_pkey = execute_only_pkey;
+	return execute_only_pkey;

Quoted 8 lines

arch/powerpc/mm/pkeys.c int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey, · line 156

Quoted 1 line

arch/powerpc/mm/pkeys.c +1 -0

+}

If you follow the code flow in __execute_only_pkey, the AMR and UAMOR are read 3 times in total, and AMR is written twice. IAMR is read and written twice. Since they are SPRs and access to them is slow (or isn't it?), is it worth it to read them once in __execute_only_pkey and pass down their values to the callees, and then write them once at the end of the function?

If my calculations are right:
uamor may be read once and may be written once. amr may be read once and is written once. iamr is written once.
So not that bad, i think.

Ram Pai <[email protected]> writes:

arch/powerpc/mm/pkeys.c int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey, · line 156

Quoted 6 lines

arch/powerpc/mm/pkeys.c +6 -0

+
+	/* We got one, store it and use it from here on out */
+	if (need_to_set_mm_pkey)
+		mm->context.execute_only_pkey = execute_only_pkey;
+	return execute_only_pkey;
+}

If you follow the code flow in __execute_only_pkey, the AMR and UAMOR are read 3 times in total, and AMR is written twice. IAMR is read and written twice. Since they are SPRs and access to them is slow (or isn't it?),

Quoted 1 line · Thiago Jung Bauermann

Ram Pai <[email protected]> writes:

...

Quoted 11 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+
+	/* We got one, store it and use it from here on out */
+	if (need_to_set_mm_pkey)
+		mm->context.execute_only_pkey = execute_only_pkey;
+	return execute_only_pkey;
+}

If you follow the code flow in __execute_only_pkey, the AMR and UAMOR are read 3 times in total, and AMR is written twice. IAMR is read and written twice. Since they are SPRs and access to them is slow (or isn't it?),

SPRs read/writes are slow, but they're not *that* slow in comparison to a system call (which I think is where this code is being called?).

So we should try to avoid too many SPR read/writes, but at the same time we can accept more than the minimum if it makes the code much easier to follow.

Quoted 15 lines · Michael Ellerman

Quoted 1 line · Thiago Jung Bauermann

Ram Pai <[email protected]> writes:

...

Quoted 11 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+
+	/* We got one, store it and use it from here on out */
+	if (need_to_set_mm_pkey)
+		mm->context.execute_only_pkey = execute_only_pkey;
+	return execute_only_pkey;
+}

If you follow the code flow in __execute_only_pkey, the AMR and UAMOR are read 3 times in total, and AMR is written twice. IAMR is read and written twice. Since they are SPRs and access to them is slow (or isn't it?),

Quoted 2 lines

SPRs read/writes are slow, but they're not *that* slow in comparison to a system call (which I think is where this code is being called?).

Yes, this code runs on mprotect and mmap syscalls if the memory is requested to have execute but not read nor write permissions.

Quoted 3 lines

So we should try to avoid too many SPR read/writes, but at the same time we can accept more than the minimum if it makes the code much easier to follow.

Ok. Ram had asked me to suggest a way to optimize the SPR reads and writes and I came up with the patch below. Do you think it's worth it?

Quoted 36 lines · Michael Ellerman

Quoted 17 lines · Thiago Jung Bauermann

Quoted 15 lines · Michael Ellerman

Quoted 1 line · Thiago Jung Bauermann

Ram Pai <[email protected]> writes:

...

Quoted 11 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+
+	/* We got one, store it and use it from here on out */
+	if (need_to_set_mm_pkey)
+		mm->context.execute_only_pkey = execute_only_pkey;
+	return execute_only_pkey;
+}

If you follow the code flow in __execute_only_pkey, the AMR and UAMOR are read 3 times in total, and AMR is written twice. IAMR is read and written twice. Since they are SPRs and access to them is slow (or isn't it?),

Quoted 5 lines

Quoted 2 lines

SPRs read/writes are slow, but they're not *that* slow in comparison to a system call (which I think is where this code is being called?).

Yes, this code runs on mprotect and mmap syscalls if the memory is requested to have execute but not read nor write permissions.

Yep. That's not in the fast path for key usage, ie. the fast path is userspace changing the AMR itself, and the overhead of a syscall is already hundreds of cycles.

Quoted 6 lines

Quoted 3 lines

So we should try to avoid too many SPR read/writes, but at the same time we can accept more than the minimum if it makes the code much easier to follow.

Ok. Ram had asked me to suggest a way to optimize the SPR reads and writes and I came up with the patch below. Do you think it's worth it?

Quoted 8 lines

At a glance no I don't think it is. Sorry you spent that much time on it.

I think we can probably reduce the number of SPR accesses without needing to go to that level of complexity.

But don't throw the patch away, I may eat my words once I have the full series applied and am looking at it hard - at the moment I'm just reviewing the patches piecemeal as I get time.