powerpc: Free up RPAGE_RSV bits

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.

Reviewed-by Aneesh Kumar K.V <[email protected]>

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 0c4e470..8909039 100644
@@ -48,6 +48,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 +6 -0

index 9732837..6652669 100644
@@ -74,6 +74,31 @@ static inline unsigned long __rpte_to_hidx(real_pte_t rpte, unsigned long index)
        return (pte_val(rpte.pte) >> H_PAGE_F_GIX_SHIFT) & 0xf;
 }
 
+/*
+ * 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,

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

index 9732837..6652669 100644
@@ -74,6 +74,31 @@ static inline unsigned long __rpte_to_hidx(real_pte_t rpte, unsigned long index)
 	return (pte_val(rpte.pte) >> H_PAGE_F_GIX_SHIFT) & 0xf;
 }
 
+/*
+ * Commit the hash slot and return pte bits that needs to be modified.

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.

Reviewed-by Aneesh Kumar K.V <[email protected]>

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 f884520..060c059 100644
@@ -166,6 +166,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 +0 -0

index 67ec2e9..e68f053 100644

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

+ */
+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);

Quoted 35 lines · Ram Pai

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.

Reviewed-by Aneesh Kumar K.V <[email protected]>

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 f884520..060c059 100644
@@ -166,6 +166,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 +0 -0

index 67ec2e9..e68f053 100644
@@ -1591,6 +1591,24 @@ static inline void tm_flush_hash_page(int local)
 }
 #endif
 

arch/powerpc/mm/hash_utils_64.c static inline void tm_flush_hash_page(int local) · line 1596

Quoted 3 lines

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

+/*
+ * return the global hash slot, corresponding to the given
+ * pte, which contains the hpte.

Does this work with native/guest page tables? I guess both.

The comment sounds trivial, could you please elaborate more. Looking at the code, it seems like given a real pte, we use the hash value and hidx to figure out the slot value in the global slot information. This uses information in the software page tables. Is that correct?

Do we have to consider validity and
present state here or is that guaranteed?

Quoted 9 lines

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

+ */
+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);

arch/powerpc/mm/hash_utils_64.c static inline void tm_flush_hash_page(int local) · line 1614

Quoted 10 lines

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

+       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 !
  */

Balbir Singh.

Rearrange 64K PTE bits to free up bits 3, 4, 5 and 6, in the 4K backed HPTE pages.These bits continue to be used for 64K backed HPTE pages in this patch, but will be freed up in the next patch. The bit numbers are big-endian as defined in the ISA3.0

The patch does the following change to the 4k htpe backed 64K PTE's format.

H_PAGE_BUSY moves from bit 3 to bit 9 (B bit in the figure
below)
V0 which occupied bit 4 is not used anymore.
V1 which occupied bit 5 is not used anymore.
V2 which occupied bit 6 is not used anymore.
V3 which occupied bit 7 is not used anymore.

Before the patch, the 4k 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|B|V0|V1|V2|V3|x| | |x|x|................|x|x|x|x| <- primary pte '_'_'_'_'__'__'__'__'_'_'_'_'_'________________'_'_'_'_' |S|G|I|X|S |G |I |X |S|G|I|X|..................|S|G|I|X| <- secondary pte '_'_'_'_'__'__'__'__'_'_'_'_'__________________'_'_'_'_'

After the patch, the 4k 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| | | | | |x|B| |x|x|................|.|.|.|.| <- primary pte '_'_'_'_'__'__'__'__'_'_'_'_'_'________________'_'_'_'_' |S|G|I|X|S |G |I |X |S|G|I|X|..................|S|G|I|X| <- secondary pte '_'_'_'_'__'__'__'__'_'_'_'_'__________________'_'_'_'_'

the four bits S,G,I,X (one quadruplet per 4k HPTE) that cache the hash-bucket slot value, is initialized to
1,1,1,1 indicating -- an invalid slot. If a HPTE gets
cached in a 1111 slot(i.e 7th slot of secondary hash bucket), it is released immediately. In other words,
even though 1111 is a valid slot value in the hash
bucket, we consider it invalid and release the slot and the HPTE. This gives us the opportunity to determine the validity of S,G,I,X bits based on its contents and not on any of the bits V0,V1,V2 or V3 in the primary PTE

When   we  release  a    HPTE    cached in the 1111 slot
we also    release  a  legitimate   slot  in the primary

hash bucket and unmap its corresponding HPTE. This
is to ensure that we do get a HPTE cached in a slot
of the primary hash bucket, the next time we retry.

Though treating 1111 slot as invalid, reduces the number of available slots in the hash bucket and may
have an effect on the performance, the probabilty of
hitting a 1111 slot is extermely low.

Compared to the current scheme, the above scheme
reduces the number of false hash table updates
significantly and has the added advantage of releasing four valuable PTE bits for other purpose.

NOTE:even though bits 3, 4, 5, 6, 7 are not used when the 64K PTE is backed by 4k HPTE, they continue to be used if the PTE gets backed by 64k HPTE. The next patch will decouple that aswell, and truely release the bits.

This idea was jointly developed by Paul Mackerras, Aneesh, Michael Ellermen and myself.

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

index 1a68cb1..c6c5559 100644
@@ -126,18 +113,13 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
 	if (__rpte_sub_valid(rpte, subpg_index)) {
 		int ret;
 

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

 	/*
 	 * handle H_PAGE_4K_PFN case
 	 */

@@ -172,15 +163,41 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
 	 * Primary is full, try the secondary
 	 */
 	if (unlikely(slot == -1)) {
+		bool soft_invalid;
+
 		hpte_group = ((~hash & htab_hash_mask) * HPTES_PER_GROUP) & ~0x7UL;
 		slot = mmu_hash_ops.hpte_insert(hpte_group, vpn, pa,
 						rflags, HPTE_V_SECONDARY,
 						MMU_PAGE_4K, MMU_PAGE_4K,
 						ssize);
-		if (slot == -1) {
-			if (mftb() & 0x1)
+
+		soft_invalid = hpte_soft_invalid(slot);
+		if (unlikely(soft_invalid)) {
 
 
+			/*

Quoted 8 lines · Ram Pai

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

index 1a68cb1..c6c5559 100644
@@ -126,18 +113,13 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
 	if (__rpte_sub_valid(rpte, subpg_index)) {
 		int ret;
 

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

Quoted 10 lines

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

-		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;
+		gslot = pte_get_hash_gslot(vpn, shift, ssize, rpte,
+					subpg_index);
+		ret = mmu_hash_ops.hpte_updatepp(gslot, rflags, vpn,
+			MMU_PAGE_4K, MMU_PAGE_4K, ssize, flags);

This was formatted correctly before:

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

Quoted 6 lines

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

-		ret = mmu_hash_ops.hpte_updatepp(slot, rflags, vpn,
-						 MMU_PAGE_4K, MMU_PAGE_4K,
-						 ssize, flags);
 		/*
-		 *if we failed because typically the HPTE wasn't really here
+		 * if we failed because typically the HPTE wasn't really here

If you're fixing it up please make it "If ...".

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

Quoted 13 lines

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

 		 * we try an insertion.
 		 */
 		if (ret == -1)

@@ -148,6 +130,15 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
 	}
 
 htab_insert_hpte:
+
+	/*
+	 * initialize all hidx entries to invalid value,
+	 * the first time the PTE is about to allocate
+	 * a 4K hpte
+	 */

Should be:

	/*
	 * Initialize all hidx entries to invalid value, the first time
         * the PTE is about to allocate a 4K HPTE.
	 */

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

Quoted 3 lines

arch/powerpc/mm/hash64_64k.c +3 -0

+	if (!(old_pte & H_PAGE_COMBO))
+		rpte.hidx = ~0x0UL;
+

Paul had the idea that if we biased the slot number by 1, we could make the "invalid" value be == 0.

That would avoid needing to that above, and also mean the value is correctly invalid from the get-go, which would be good IMO.

I think now that you've added the slot accessors it would be pretty easy to do.

Quoted 8 lines · Ram Pai

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

index 1a68cb1..c6c5559 100644
@@ -126,18 +113,13 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
 	if (__rpte_sub_valid(rpte, subpg_index)) {
 		int ret;
 

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

Quoted 10 lines

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

-		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;
+		gslot = pte_get_hash_gslot(vpn, shift, ssize, rpte,
+					subpg_index);
+		ret = mmu_hash_ops.hpte_updatepp(gslot, rflags, vpn,
+			MMU_PAGE_4K, MMU_PAGE_4K, ssize, flags);

This was formatted correctly before:

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

Quoted 6 lines

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

-		ret = mmu_hash_ops.hpte_updatepp(slot, rflags, vpn,
-						 MMU_PAGE_4K, MMU_PAGE_4K,
-						 ssize, flags);
 		/*
-		 *if we failed because typically the HPTE wasn't really here
+		 * if we failed because typically the HPTE wasn't really here

If you're fixing it up please make it "If ...".

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

Quoted 13 lines

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

 		 * we try an insertion.
 		 */
 		if (ret == -1)

@@ -148,6 +130,15 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
 	}
 
 htab_insert_hpte:
+
+	/*
+	 * initialize all hidx entries to invalid value,
+	 * the first time the PTE is about to allocate
+	 * a 4K hpte
+	 */

Should be:

	/*
	 * Initialize all hidx entries to invalid value, the first time
         * the PTE is about to allocate a 4K HPTE.
	 */

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

Quoted 3 lines

arch/powerpc/mm/hash64_64k.c +3 -0

+	if (!(old_pte & H_PAGE_COMBO))
+		rpte.hidx = ~0x0UL;
+

Paul had the idea that if we biased the slot number by 1, we could make the "invalid" value be == 0.

That would avoid needing to that above, and also mean the value is correctly invalid from the get-go, which would be good IMO.

I think now that you've added the slot accessors it would be pretty easy to do.

Quoted 62 lines · Michael Ellerman

Quoted 8 lines · Ram Pai

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

index 1a68cb1..c6c5559 100644
@@ -126,18 +113,13 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
 	if (__rpte_sub_valid(rpte, subpg_index)) {
 		int ret;
 

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

Quoted 10 lines

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

-		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;
+		gslot = pte_get_hash_gslot(vpn, shift, ssize, rpte,
+					subpg_index);
+		ret = mmu_hash_ops.hpte_updatepp(gslot, rflags, vpn,
+			MMU_PAGE_4K, MMU_PAGE_4K, ssize, flags);

This was formatted correctly before:

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

Quoted 6 lines

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

-		ret = mmu_hash_ops.hpte_updatepp(slot, rflags, vpn,
-						 MMU_PAGE_4K, MMU_PAGE_4K,
-						 ssize, flags);
 		/*
-		 *if we failed because typically the HPTE wasn't really here
+		 * if we failed because typically the HPTE wasn't really here

If you're fixing it up please make it "If ...".

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

Quoted 13 lines

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

 		 * we try an insertion.
 		 */
 		if (ret == -1)

@@ -148,6 +130,15 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
 	}
 
 htab_insert_hpte:
+
+	/*
+	 * initialize all hidx entries to invalid value,
+	 * the first time the PTE is about to allocate
+	 * a 4K hpte
+	 */

Should be:

	/*
	 * Initialize all hidx entries to invalid value, the first time
         * the PTE is about to allocate a 4K HPTE.
	 */

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

Quoted 3 lines

arch/powerpc/mm/hash64_64k.c +3 -0

+	if (!(old_pte & H_PAGE_COMBO))
+		rpte.hidx = ~0x0UL;
+

Paul had the idea that if we biased the slot number by 1, we could make the "invalid" value be == 0.

That would avoid needing to that above, and also mean the value is correctly invalid from the get-go, which would be good IMO.

Quoted 2 lines

I think now that you've added the slot accessors it would be pretty easy to do.

That would be imply, we loose one slot in primary group, which means we will do extra work in some case because our primary now has only 7 slots. And in case of pseries, the hypervisor will always return the least available slot, which implie we will do extra hcalls in case of an hpte insert to an empty group?

Quoted 2 lines · Aneesh Kumar K.V

Quoted 1 line · Michael Ellerman

Ram Pai <[email protected]> writes:

Quoted 16 lines

Quoted 16 lines

Should be:

	/*
	 * Initialize all hidx entries to invalid value, the first time
         * the PTE is about to allocate a 4K HPTE.
	 */

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

Quoted 3 lines

arch/powerpc/mm/hash64_64k.c +3 -0

+	if (!(old_pte & H_PAGE_COMBO))
+		rpte.hidx = ~0x0UL;
+

Paul had the idea that if we biased the slot number by 1, we could make the "invalid" value be == 0.

That would avoid needing to that above, and also mean the value is correctly invalid from the get-go, which would be good IMO.

Quoted 8 lines

Quoted 2 lines

I think now that you've added the slot accessors it would be pretty easy to do.

That would be imply, we loose one slot in primary group, which means we will do extra work in some case because our primary now has only 7 slots. And in case of pseries, the hypervisor will always return the least available slot, which implie we will do extra hcalls in case of an hpte insert to an empty group?

No. that is not the idea. The idea is that slot 'F' in the seconday will continue to be a invalid slot, but will be represented as offset-by-one in the PTE. In other words, 0 will be repesented as 1,
1 as 2.... and n as (n+1)%32

The idea seems feasible. It has the advantage -- where 0 in the PTE means invalid slot. But it can be confusing to the casual code- reader. Will need to put in a big-huge comment to explain that.

Quoted 3 lines · Ram Pai

Quoted 2 lines · Aneesh Kumar K.V

Quoted 1 line · Michael Ellerman

Ram Pai <[email protected]> writes:

...

Quoted 25 lines

Quoted 16 lines

Quoted 16 lines

Should be:

	/*
	 * Initialize all hidx entries to invalid value, the first time
         * the PTE is about to allocate a 4K HPTE.
	 */

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

Quoted 3 lines

arch/powerpc/mm/hash64_64k.c +3 -0

+	if (!(old_pte & H_PAGE_COMBO))
+		rpte.hidx = ~0x0UL;
+

Paul had the idea that if we biased the slot number by 1, we could make the "invalid" value be == 0.

That would avoid needing to that above, and also mean the value is correctly invalid from the get-go, which would be good IMO.

Quoted 8 lines

Quoted 2 lines

I think now that you've added the slot accessors it would be pretty easy to do.

That would be imply, we loose one slot in primary group, which means we will do extra work in some case because our primary now has only 7 slots. And in case of pseries, the hypervisor will always return the least available slot, which implie we will do extra hcalls in case of an hpte insert to an empty group?

Quoted 4 lines

No. that is not the idea. The idea is that slot 'F' in the seconday will continue to be a invalid slot, but will be represented as offset-by-one in the PTE. In other words, 0 will be repesented as 1,
1 as 2.... and n as (n+1)%32

Right.

Quoted 3 lines

The idea seems feasible. It has the advantage -- where 0 in the PTE means invalid slot. But it can be confusing to the casual code- reader. Will need to put in a big-huge comment to explain that.

This code is already confusing to *any* reader, so I don't think it's a worry. :)

cheers

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 to the secondary pte.

Reviewed-by Aneesh Kumar K.V <[email protected]>

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

---
 arch/powerpc/include/asm/book3s/64/hash-4k.h  |    3 ++
 arch/powerpc/include/asm/book3s/64/hash-64k.h |   29 +++++++++++-------------
 arch/powerpc/include/asm/book3s/64/hash.h     |    3 --
 arch/powerpc/mm/hash64_64k.c                  |   23 ++++++++-----------
 arch/powerpc/mm/hugetlbpage-hash64.c          |   18 ++++++---------
 5 files changed, 33 insertions(+), 43 deletions(-)

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

index e66bfeb..dc153c6 100644
@@ -16,6 +16,9 @@
 #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	56
+#define H_PAGE_F_SECOND	_RPAGE_RSV2	/* HPTE is in 2ndary HPTEG */
+#define H_PAGE_F_GIX	(_RPAGE_RSV3 | _RPAGE_RSV4 | _RPAGE_RPN44)
 #define H_PAGE_BUSY	_RPAGE_RSV1     /* software: PTE & hash are busy */
 
 /* PTE flags to conserve for HPTE identification */

arch/powerpc/include/asm/book3s/64/hash-64k.h +13 -16

index e038f1c..89ef5a9 100644
@@ -12,7 +12,7 @@
  */
 #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_RPN42     /* software: PTE & hash are busy */
+#define H_PAGE_BUSY	_RPAGE_RPN44     /* software: PTE & hash are busy */
 
 /*
  * We need to differentiate between explicit huge page and THP huge

@@ -21,8 +21,7 @@
 #define H_PAGE_THP_HUGE  H_PAGE_4K_PFN
 
 /* PTE flags to conserve for HPTE identification */
-#define _PAGE_HPTEFLAGS (H_PAGE_BUSY | H_PAGE_F_SECOND | \
-			 H_PAGE_F_GIX | H_PAGE_HASHPTE | H_PAGE_COMBO)
+#define _PAGE_HPTEFLAGS (H_PAGE_BUSY | H_PAGE_HASHPTE | H_PAGE_COMBO)
 /*
  * we support 16 fragments per PTE page of 64K size.
  */

@@ -50,24 +49,22 @@ static inline real_pte_t __real_pte(pte_t pte, pte_t *ptep)
 	unsigned long *hidxp;
 
 	rpte.pte = pte;
-	rpte.hidx = 0;
-	if (pte_val(pte) & H_PAGE_COMBO) {
-		/*
-		 * Make sure we order the hidx load against the H_PAGE_COMBO
-		 * check. The store side ordering is done in __hash_page_4K
-		 */
-		smp_rmb();
-		hidxp = (unsigned long *)(ptep + PTRS_PER_PTE);
-		rpte.hidx = *hidxp;
-	}
+	/*
+	 * Ensure that we do not read the hidx before we read
+	 * the pte. Because the writer side is  expected
+	 * to finish writing the hidx first followed by the pte,
+	 * by using smp_wmb().
+	 * pte_set_hash_slot() ensures that.
+	 */
+	smp_rmb();
+	hidxp = (unsigned long *)(ptep + PTRS_PER_PTE);
+	rpte.hidx = *hidxp;
 	return rpte;
 }
 
 static inline unsigned long __rpte_to_hidx(real_pte_t rpte, unsigned long index)
 {
-	if ((pte_val(rpte.pte) & H_PAGE_COMBO))
-		return (rpte.hidx >> (index<<2)) & 0xf;
-	return (pte_val(rpte.pte) >> H_PAGE_F_GIX_SHIFT) & 0xf;
+	return ((rpte.hidx >> (index<<2)) & 0xfUL);
 }
 
 /*

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

index 8ce4112..46f3a23 100644
@@ -8,9 +8,6 @@
  *
  */
 #define H_PTE_NONE_MASK		_PAGE_HPTEFLAGS
-#define H_PAGE_F_GIX_SHIFT	56
-#define H_PAGE_F_SECOND		_RPAGE_RSV2	/* HPTE is in 2ndary HPTEG */
-#define H_PAGE_F_GIX		(_RPAGE_RSV3 | _RPAGE_RSV4 | _RPAGE_RPN44)
 #define H_PAGE_HASHPTE		_RPAGE_RPN43	/* PTE has associated HPTE */
 
 #ifdef CONFIG_PPC_64K_PAGES

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

index c6c5559..9c63844 100644

arch/powerpc/mm/hash64_64k.c +8 -11

 		goto htab_insert_hpte;
 	}
 	/*

@@ -227,6 +227,7 @@ int __hash_page_64K(unsigned long ea, unsigned long access,
 		    unsigned long vsid, pte_t *ptep, unsigned long trap,
 		    unsigned long flags, int ssize)
 {
+	real_pte_t rpte;
 	unsigned long hpte_group;
 	unsigned long rflags, pa;
 	unsigned long old_pte, new_pte;

@@ -263,6 +264,7 @@ int __hash_page_64K(unsigned long ea, unsigned long access,
 	} while (!pte_xchg(ptep, __pte(old_pte), __pte(new_pte)));
 
 	rflags = htab_convert_pte_flags(new_pte);
+	rpte = __real_pte(__pte(old_pte), ptep);
 
 	if (cpu_has_feature(CPU_FTR_NOEXECUTE) &&
 	    !cpu_has_feature(CPU_FTR_COHERENT_ICACHE))

@@ -270,18 +272,13 @@ 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 gslot;
 		/*
 		 * There MIGHT be an HPTE for this pte
 		 */
-		hash = hpt_hash(vpn, shift, ssize);
-		if (old_pte & H_PAGE_F_SECOND)
-			hash = ~hash;
-		slot = (hash & htab_hash_mask) * HPTES_PER_GROUP;
-		slot += (old_pte & H_PAGE_F_GIX) >> H_PAGE_F_GIX_SHIFT;
-
-		if (mmu_hash_ops.hpte_updatepp(slot, rflags, vpn, MMU_PAGE_64K,
-					       MMU_PAGE_64K, ssize,
-					       flags) == -1)
+		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;
 	}
 

@@ -328,9 +325,9 @@ int __hash_page_64K(unsigned long ea, unsigned long access,
 					   MMU_PAGE_64K, MMU_PAGE_64K, old_pte);
 			return -1;
 		}
+
 		new_pte = (new_pte & ~_PAGE_HPTEFLAGS) | H_PAGE_HASHPTE;
-		new_pte |= (slot << H_PAGE_F_GIX_SHIFT) &
-			(H_PAGE_F_SECOND | H_PAGE_F_GIX);
+		new_pte |= pte_set_hash_slot(ptep, rpte, 0, slot);
 	}
 	*ptep = __pte(new_pte & ~H_PAGE_BUSY);
 	return 0;

arch/powerpc/mm/hugetlbpage-hash64.c +6 -9

index a84bb44..d52d667 100644
@@ -22,6 +22,7 @@ int __hash_page_huge(unsigned long ea, unsigned long access, unsigned long vsid,
 		     pte_t *ptep, unsigned long trap, unsigned long flags,
 		     int ssize, unsigned int shift, unsigned int mmu_psize)
 {
+	real_pte_t rpte;
 	unsigned long vpn;
 	unsigned long old_pte, new_pte;
 	unsigned long rflags, pa, sz;

@@ -61,6 +62,7 @@ int __hash_page_huge(unsigned long ea, unsigned long access, unsigned long vsid,
 	} while(!pte_xchg(ptep, __pte(old_pte), __pte(new_pte)));
 
 	rflags = htab_convert_pte_flags(new_pte);
+	rpte = __real_pte(__pte(old_pte), ptep);
 
 	sz = ((1UL) << shift);
 	if (!cpu_has_feature(CPU_FTR_COHERENT_ICACHE))

@@ -71,16 +73,11 @@ 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;
+		unsigned long gslot;
 
-		hash = hpt_hash(vpn, shift, ssize);
-		if (old_pte & H_PAGE_F_SECOND)
-			hash = ~hash;
-		slot = (hash & htab_hash_mask) * HPTES_PER_GROUP;
-		slot += (old_pte & H_PAGE_F_GIX) >> H_PAGE_F_GIX_SHIFT;
-
-		if (mmu_hash_ops.hpte_updatepp(slot, rflags, vpn, mmu_psize,
-					       mmu_psize, ssize, flags) == -1)
+		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;
 	}
 

Quoted 154 lines · On Fri, 8 Sep 2017 15:44:44 -0700 Ram Pai <[email protected]> wrote:

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 to the secondary pte.

Reviewed-by Aneesh Kumar K.V <[email protected]>

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

---
 arch/powerpc/include/asm/book3s/64/hash-4k.h  |    3 ++
 arch/powerpc/include/asm/book3s/64/hash-64k.h |   29 +++++++++++-------------
 arch/powerpc/include/asm/book3s/64/hash.h     |    3 --
 arch/powerpc/mm/hash64_64k.c                  |   23 ++++++++-----------
 arch/powerpc/mm/hugetlbpage-hash64.c          |   18 ++++++---------
 5 files changed, 33 insertions(+), 43 deletions(-)

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

index e66bfeb..dc153c6 100644
@@ -16,6 +16,9 @@
 #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	56
+#define H_PAGE_F_SECOND	_RPAGE_RSV2	/* HPTE is in 2ndary HPTEG */
+#define H_PAGE_F_GIX	(_RPAGE_RSV3 | _RPAGE_RSV4 | _RPAGE_RPN44)
 #define H_PAGE_BUSY	_RPAGE_RSV1     /* software: PTE & hash are busy */
 
 /* PTE flags to conserve for HPTE identification */

arch/powerpc/include/asm/book3s/64/hash-64k.h +13 -16

index e038f1c..89ef5a9 100644
@@ -12,7 +12,7 @@
  */
 #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_RPN42     /* software: PTE & hash are busy */
+#define H_PAGE_BUSY	_RPAGE_RPN44     /* software: PTE & hash are busy */
 
 /*
  * We need to differentiate between explicit huge page and THP huge

@@ -21,8 +21,7 @@
 #define H_PAGE_THP_HUGE  H_PAGE_4K_PFN
 
 /* PTE flags to conserve for HPTE identification */
-#define _PAGE_HPTEFLAGS (H_PAGE_BUSY | H_PAGE_F_SECOND | \
-			 H_PAGE_F_GIX | H_PAGE_HASHPTE | H_PAGE_COMBO)
+#define _PAGE_HPTEFLAGS (H_PAGE_BUSY | H_PAGE_HASHPTE | H_PAGE_COMBO)
 /*
  * we support 16 fragments per PTE page of 64K size.
  */

@@ -50,24 +49,22 @@ static inline real_pte_t __real_pte(pte_t pte, pte_t *ptep)
 	unsigned long *hidxp;
 
 	rpte.pte = pte;
-	rpte.hidx = 0;
-	if (pte_val(pte) & H_PAGE_COMBO) {
-		/*
-		 * Make sure we order the hidx load against the H_PAGE_COMBO
-		 * check. The store side ordering is done in __hash_page_4K
-		 */
-		smp_rmb();
-		hidxp = (unsigned long *)(ptep + PTRS_PER_PTE);
-		rpte.hidx = *hidxp;
-	}
+	/*
+	 * Ensure that we do not read the hidx before we read
+	 * the pte. Because the writer side is  expected
+	 * to finish writing the hidx first followed by the pte,
+	 * by using smp_wmb().
+	 * pte_set_hash_slot() ensures that.
+	 */
+	smp_rmb();
+	hidxp = (unsigned long *)(ptep + PTRS_PER_PTE);
+	rpte.hidx = *hidxp;
 	return rpte;
 }
 
 static inline unsigned long __rpte_to_hidx(real_pte_t rpte, unsigned long index)
 {
-	if ((pte_val(rpte.pte) & H_PAGE_COMBO))
-		return (rpte.hidx >> (index<<2)) & 0xf;
-	return (pte_val(rpte.pte) >> H_PAGE_F_GIX_SHIFT) & 0xf;
+	return ((rpte.hidx >> (index<<2)) & 0xfUL);
 }
 
 /*

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

index 8ce4112..46f3a23 100644
@@ -8,9 +8,6 @@
  *
  */
 #define H_PTE_NONE_MASK		_PAGE_HPTEFLAGS
-#define H_PAGE_F_GIX_SHIFT	56
-#define H_PAGE_F_SECOND		_RPAGE_RSV2	/* HPTE is in 2ndary HPTEG */
-#define H_PAGE_F_GIX		(_RPAGE_RSV3 | _RPAGE_RSV4 | _RPAGE_RPN44)
 #define H_PAGE_HASHPTE		_RPAGE_RPN43	/* PTE has associated HPTE */
 

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

Quoted 13 lines

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

 #ifdef CONFIG_PPC_64K_PAGES

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

index c6c5559..9c63844 100644
@@ -103,8 +103,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;

Shouldn't we set old/new_pte.slot = invalid? via rpte.hidx

Quoted 97 lines

arch/powerpc/mm/hash64_64k.c +8 -11

 		goto htab_insert_hpte;
 	}
 	/*

@@ -227,6 +227,7 @@ int __hash_page_64K(unsigned long ea, unsigned long access,
 		    unsigned long vsid, pte_t *ptep, unsigned long trap,
 		    unsigned long flags, int ssize)
 {
+	real_pte_t rpte;
 	unsigned long hpte_group;
 	unsigned long rflags, pa;
 	unsigned long old_pte, new_pte;

@@ -263,6 +264,7 @@ int __hash_page_64K(unsigned long ea, unsigned long access,
 	} while (!pte_xchg(ptep, __pte(old_pte), __pte(new_pte)));
 
 	rflags = htab_convert_pte_flags(new_pte);
+	rpte = __real_pte(__pte(old_pte), ptep);
 
 	if (cpu_has_feature(CPU_FTR_NOEXECUTE) &&
 	    !cpu_has_feature(CPU_FTR_COHERENT_ICACHE))

@@ -270,18 +272,13 @@ 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 gslot;
 		/*
 		 * There MIGHT be an HPTE for this pte
 		 */
-		hash = hpt_hash(vpn, shift, ssize);
-		if (old_pte & H_PAGE_F_SECOND)
-			hash = ~hash;
-		slot = (hash & htab_hash_mask) * HPTES_PER_GROUP;
-		slot += (old_pte & H_PAGE_F_GIX) >> H_PAGE_F_GIX_SHIFT;
-
-		if (mmu_hash_ops.hpte_updatepp(slot, rflags, vpn, MMU_PAGE_64K,
-					       MMU_PAGE_64K, ssize,
-					       flags) == -1)
+		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;
 	}
 

@@ -328,9 +325,9 @@ int __hash_page_64K(unsigned long ea, unsigned long access,
 					   MMU_PAGE_64K, MMU_PAGE_64K, old_pte);
 			return -1;
 		}
+
 		new_pte = (new_pte & ~_PAGE_HPTEFLAGS) | H_PAGE_HASHPTE;
-		new_pte |= (slot << H_PAGE_F_GIX_SHIFT) &
-			(H_PAGE_F_SECOND | H_PAGE_F_GIX);
+		new_pte |= pte_set_hash_slot(ptep, rpte, 0, slot);
 	}
 	*ptep = __pte(new_pte & ~H_PAGE_BUSY);
 	return 0;

arch/powerpc/mm/hugetlbpage-hash64.c +6 -9

index a84bb44..d52d667 100644
@@ -22,6 +22,7 @@ int __hash_page_huge(unsigned long ea, unsigned long access, unsigned long vsid,
 		     pte_t *ptep, unsigned long trap, unsigned long flags,
 		     int ssize, unsigned int shift, unsigned int mmu_psize)
 {
+	real_pte_t rpte;
 	unsigned long vpn;
 	unsigned long old_pte, new_pte;
 	unsigned long rflags, pa, sz;

@@ -61,6 +62,7 @@ int __hash_page_huge(unsigned long ea, unsigned long access, unsigned long vsid,
 	} while(!pte_xchg(ptep, __pte(old_pte), __pte(new_pte)));
 
 	rflags = htab_convert_pte_flags(new_pte);
+	rpte = __real_pte(__pte(old_pte), ptep);
 
 	sz = ((1UL) << shift);
 	if (!cpu_has_feature(CPU_FTR_COHERENT_ICACHE))

@@ -71,16 +73,11 @@ 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;
+		unsigned long gslot;
 
-		hash = hpt_hash(vpn, shift, ssize);
-		if (old_pte & H_PAGE_F_SECOND)
-			hash = ~hash;
-		slot = (hash & htab_hash_mask) * HPTES_PER_GROUP;
-		slot += (old_pte & H_PAGE_F_GIX) >> H_PAGE_F_GIX_SHIFT;
-
-		if (mmu_hash_ops.hpte_updatepp(slot, rflags, vpn, mmu_psize,
-					       mmu_psize, ssize, flags) == -1)
+		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;
 	}
 

arch/powerpc/mm/hugetlbpage-hash64.c int __hash_page_huge(unsigned long ea, unsigned long access, unsigned long vsid, · line 109

Quoted 9 lines

arch/powerpc/mm/hugetlbpage-hash64.c +1 -2

@@ -106,8 +103,7 @@ int __hash_page_huge(unsigned long ea, unsigned long access, unsigned long vsid,
 			return -1;
 		}
 
-		new_pte |= (slot << H_PAGE_F_GIX_SHIFT) &
-			(H_PAGE_F_SECOND | H_PAGE_F_GIX);
+		new_pte |= pte_set_hash_slot(ptep, rpte, 0, slot);
 	}
 
 	/*

Balbir

Quoted 157 lines · Balbir Singh

Quoted 154 lines · On Fri, 8 Sep 2017 15:44:44 -0700 Ram Pai <[email protected]> wrote:

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 to the secondary pte.

Reviewed-by Aneesh Kumar K.V <[email protected]>

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

---
 arch/powerpc/include/asm/book3s/64/hash-4k.h  |    3 ++
 arch/powerpc/include/asm/book3s/64/hash-64k.h |   29 +++++++++++-------------
 arch/powerpc/include/asm/book3s/64/hash.h     |    3 --
 arch/powerpc/mm/hash64_64k.c                  |   23 ++++++++-----------
 arch/powerpc/mm/hugetlbpage-hash64.c          |   18 ++++++---------
 5 files changed, 33 insertions(+), 43 deletions(-)

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

index e66bfeb..dc153c6 100644
@@ -16,6 +16,9 @@
 #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	56
+#define H_PAGE_F_SECOND	_RPAGE_RSV2	/* HPTE is in 2ndary HPTEG */
+#define H_PAGE_F_GIX	(_RPAGE_RSV3 | _RPAGE_RSV4 | _RPAGE_RPN44)
 #define H_PAGE_BUSY	_RPAGE_RSV1     /* software: PTE & hash are busy */
 
 /* PTE flags to conserve for HPTE identification */

arch/powerpc/include/asm/book3s/64/hash-64k.h +13 -16

index e038f1c..89ef5a9 100644
@@ -12,7 +12,7 @@
  */
 #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_RPN42     /* software: PTE & hash are busy */
+#define H_PAGE_BUSY	_RPAGE_RPN44     /* software: PTE & hash are busy */
 
 /*
  * We need to differentiate between explicit huge page and THP huge

@@ -21,8 +21,7 @@
 #define H_PAGE_THP_HUGE  H_PAGE_4K_PFN
 
 /* PTE flags to conserve for HPTE identification */
-#define _PAGE_HPTEFLAGS (H_PAGE_BUSY | H_PAGE_F_SECOND | \
-			 H_PAGE_F_GIX | H_PAGE_HASHPTE | H_PAGE_COMBO)
+#define _PAGE_HPTEFLAGS (H_PAGE_BUSY | H_PAGE_HASHPTE | H_PAGE_COMBO)
 /*
  * we support 16 fragments per PTE page of 64K size.
  */

@@ -50,24 +49,22 @@ static inline real_pte_t __real_pte(pte_t pte, pte_t *ptep)
 	unsigned long *hidxp;
 
 	rpte.pte = pte;
-	rpte.hidx = 0;
-	if (pte_val(pte) & H_PAGE_COMBO) {
-		/*
-		 * Make sure we order the hidx load against the H_PAGE_COMBO
-		 * check. The store side ordering is done in __hash_page_4K
-		 */
-		smp_rmb();
-		hidxp = (unsigned long *)(ptep + PTRS_PER_PTE);
-		rpte.hidx = *hidxp;
-	}
+	/*
+	 * Ensure that we do not read the hidx before we read
+	 * the pte. Because the writer side is  expected
+	 * to finish writing the hidx first followed by the pte,
+	 * by using smp_wmb().
+	 * pte_set_hash_slot() ensures that.
+	 */
+	smp_rmb();
+	hidxp = (unsigned long *)(ptep + PTRS_PER_PTE);
+	rpte.hidx = *hidxp;
 	return rpte;
 }
 
 static inline unsigned long __rpte_to_hidx(real_pte_t rpte, unsigned long index)
 {
-	if ((pte_val(rpte.pte) & H_PAGE_COMBO))
-		return (rpte.hidx >> (index<<2)) & 0xf;
-	return (pte_val(rpte.pte) >> H_PAGE_F_GIX_SHIFT) & 0xf;
+	return ((rpte.hidx >> (index<<2)) & 0xfUL);
 }
 
 /*

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

index 8ce4112..46f3a23 100644
@@ -8,9 +8,6 @@
  *
  */
 #define H_PTE_NONE_MASK		_PAGE_HPTEFLAGS
-#define H_PAGE_F_GIX_SHIFT	56
-#define H_PAGE_F_SECOND		_RPAGE_RSV2	/* HPTE is in 2ndary HPTEG */
-#define H_PAGE_F_GIX		(_RPAGE_RSV3 | _RPAGE_RSV4 | _RPAGE_RPN44)
 #define H_PAGE_HASHPTE		_RPAGE_RPN43	/* PTE has associated HPTE */
 

Quoted 15 lines

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

Quoted 13 lines

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

 #ifdef CONFIG_PPC_64K_PAGES

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

index c6c5559..9c63844 100644
@@ -103,8 +103,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;

Shouldn't we set old/new_pte.slot = invalid? via rpte.hidx

by resetting the H_PAGE_HASHPTE flag, we are invalidating slot information. Would that not be sufficient?

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.

Quoted 4 lines · Ram Pai

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.

Have you measured the performance impact of this ? The second part of the PTE being in a different cache line there could be one...

Quoted 9 lines · Benjamin Herrenschmidt

Quoted 4 lines · Ram Pai

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.

Have you measured the performance impact of this ? The second part of the PTE being in a different cache line there could be one...

I am also looking at a patch series removing the slot tracking completely. With randomize address turned off and no swap in guest/host and making sure we touched most of guest ram, I don't find much impact in performance when we don't track the slot at all. I will post the patch series with numbers in a day or two. But my test was

while (5000) {
mmap(128M)
touch every page of 2048 pages
munmap()
}

I could also be the best case in my run because i might have always found the hash pte slot in the primary. In one measurement with swap on and address randmization enabled, i did find a 50% impact. But then i was not able to recreate that again. So could be something i did wrong in the test setup.

Ram,

Will you be able to get a test run with the above loop?

Quoted 4 lines · Ram Pai

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.

Have you measured the performance impact of this ? The second part of the PTE being in a different cache line there could be one...

Quoted 6 lines · Benjamin Herrenschmidt

Quoted 4 lines · Ram Pai

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.

Quoted 2 lines

Have you measured the performance impact of this ? The second part of the PTE being in a different cache line there could be one...

hmm..missed responding to this comment.

I did a preliminay measurement running mmap bench in the selftest. Ran it multiple times. almost always the numbers were either equal-to or better-than without the patch-series.

We need PTE bits 3 ,4, 5, 6 and 57 to support protection-keys, because these are the bits we want to consolidate on across all configuration to support protection keys.

Bit 3,4,5 and 6 are currently used on 4K-pte kernels. But bit 9 and 10 are available. Hence we use the two available bits and free up bit 5 and 6. We will still not be able to free up bit 3 and 4. In the absence of any other free bits, we will have to
stay satisfied with what we have :-(. This means we will not
be able to support 32 protection keys, but only 8. The bit numbers are big-endian as defined in the ISA3.0

Quoted 4 lines · On Fri, 8 Sep 2017 15:44:45 -0700 Ram Pai <[email protected]> wrote:

We need PTE bits 3 ,4, 5, 6 and 57 to support protection-keys, because these are the bits we want to consolidate on across all configuration to support protection keys.

Quoted 8 lines

Bit 3,4,5 and 6 are currently used on 4K-pte kernels. But bit 9 and 10 are available. Hence we use the two available bits and free up bit 5 and 6. We will still not be able to free up bit 3 and 4. In the absence of any other free bits, we will have to
stay satisfied with what we have :-(. This means we will not
be able to support 32 protection keys, but only 8. The bit numbers are big-endian as defined in the ISA3.0

Any chance for 4k PTE's we can do slot searching for the PTE? I guess thats add additional complexity

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

Reviewed-by Aneesh Kumar K.V <[email protected]>

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

---

Quoted 3 lines · On Fri, 8 Sep 2017 15:44:47 -0700 Ram Pai <[email protected]> wrote:

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

Quoted 3 lines

Reviewed-by Aneesh Kumar K.V <[email protected]>

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

---

So we lose slot and secondary information for 64K PTE's with this change?

Basic plumbing to initialize the pkey system.
Nothing is enabled yet. A later patch will enable it ones all the infrastructure is in place.

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       |   45 ++++++++++++++++++++++++++++++++
 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                |   33 +++++++++++++++++++++++
 7 files changed, 105 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/include/asm/pkeys.h
 create mode 100644 arch/powerpc/mm/pkeys.c

arch/powerpc/Kconfig +0 -0

index 9fc3c0b..a4cd210 100644

arch/powerpc/Kconfig +11 -0

+	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
+
+	  If unsure, say y.
+
 endmenu
 
 config ISA_DMA_API

arch/powerpc/include/asm/mmu_context.h +5 -0

index 3095925..7badf29 100644
@@ -141,5 +141,10 @@ static inline bool arch_vma_access_permitted(struct vm_area_struct *vma,
 	/* by default, allow everything */
 	return true;
 }
+
+#ifndef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
+#define pkey_initialize()
+#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
+
 #endif /* __KERNEL__ */
 #endif /* __ASM_POWERPC_MMU_CONTEXT_H */

arch/powerpc/include/asm/pkeys.h new file +45 -0

new file mode 100644
index 0000000..c02305a
@@ -0,0 +1,45 @@
+#ifndef _ASM_PPC64_PKEYS_H
+#define _ASM_PPC64_PKEYS_H
+
+extern bool pkey_inited;
+extern bool pkey_execute_disable_support;
+#define ARCH_VM_PKEY_FLAGS 0
+
+static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
+{
+	return (pkey == 0);
+}
+
+static inline int mm_pkey_alloc(struct mm_struct *mm)
+{
+	return -1;
+}
+
+static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
+{
+	return -EINVAL;
+}
+
+/*
+ * Try to dedicate one of the protection keys to be used as an
+ * execute-only protection key.
+ */
+static inline int execute_only_pkey(struct mm_struct *mm)
+{
+	return 0;
+}
+
+static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,
+		int prot, int pkey)
+{
+	return 0;
+}
+
+static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
+		unsigned long init_val)
+{
+	return 0;
+}
+
+extern void pkey_initialize(void);
+#endif /*_ASM_PPC64_PKEYS_H */

arch/powerpc/kernel/setup_64.c +4 -0

index b89c6aa..3b67014 100644
@@ -37,6 +37,7 @@
 #include <linux/memblock.h>
 #include <linux/memory.h>
 #include <linux/nmi.h>
+#include <linux/pkeys.h>
 
 #include <asm/io.h>
 #include <asm/kdump.h>

@@ -316,6 +317,9 @@ void __init early_setup(unsigned long dt_ptr)
 	/* Initialize the hash table or TLB handling */
 	early_init_mmu();
 
+	/* initialize the key subsystem */
+	pkey_initialize();
+
 	/*
 	 * At this point, we can let interrupts switch to virtual mode
 	 * (the MMU has been setup), so adjust the MSR in the PACA to

arch/powerpc/mm/Makefile +1 -0

index fb844d2..927620a 100644
@@ -43,3 +43,4 @@ obj-$(CONFIG_PPC_COPRO_BASE)	+= copro_fault.o
 obj-$(CONFIG_SPAPR_TCE_IOMMU)	+= mmu_context_iommu.o
 obj-$(CONFIG_PPC_PTDUMP)	+= dump_linuxpagetables.o
 obj-$(CONFIG_PPC_HTDUMP)	+= dump_hashpagetable.o
+obj-$(CONFIG_PPC64_MEMORY_PROTECTION_KEYS)	+= pkeys.o

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

index 0dff57b..67f62b5 100644
@@ -35,6 +35,7 @@
 #include <linux/memblock.h>
 #include <linux/context_tracking.h>
 #include <linux/libfdt.h>
+#include <linux/pkeys.h>
 
 #include <asm/debugfs.h>
 #include <asm/processor.h>

arch/powerpc/mm/pkeys.c new file +15 -0

new file mode 100644
index 0000000..418a05b
@@ -0,0 +1,33 @@
+/*
+ * PowerPC Memory Protection Keys management
+ * Copyright (c) 2015, Intel Corporation.
+ * Copyright (c) 2017, IBM Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+#include <linux/pkeys.h>                /* PKEY_*                       */

Quoted 25 lines · On Fri, 8 Sep 2017 15:44:49 -0700 Ram Pai <[email protected]> wrote:

Basic plumbing to initialize the pkey system.
Nothing is enabled yet. A later patch will enable it ones all the infrastructure is in place.

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       |   45 ++++++++++++++++++++++++++++++++
 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                |   33 +++++++++++++++++++++++
 7 files changed, 105 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/include/asm/pkeys.h
 create mode 100644 arch/powerpc/mm/pkeys.c

arch/powerpc/Kconfig +0 -0

index 9fc3c0b..a4cd210 100644
@@ -864,6 +864,22 @@ config SECCOMP
 
 	  If unsure, say Y. Only embedded should say N here.
 

arch/powerpc/Kconfig config SECCOMP · line 871

Quoted 5 lines

arch/powerpc/Kconfig +5 -0

+config PPC64_MEMORY_PROTECTION_KEYS
+	prompt "PowerPC Memory Protection Keys"
+	def_bool y
+	# Note: only available in 64-bit mode
+	depends on PPC64

This is not sufficient right, you need PPC_BOOK3S_64 for compile time at-least?

Quoted 3 lines

arch/powerpc/Kconfig +2 -0

+	select ARCH_USES_HIGH_VMA_FLAGS
+

arch/powerpc/Kconfig

Quoted 6 lines

arch/powerpc/Kconfig +6 -0

+void __init pkey_initialize(void)
+{
+	/* disable the pkey system till everything
+	 * is in place. A patch further down the
+	 * line will enable it.
+	 */

Comment style is broken

arch/powerpc/include/asm/pkeys.h new file +13 -0

+}
+
+static inline int mm_pkey_alloc(struct mm_struct *mm)
+{
+	return -1;
+}
+
+static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
+{
+	return -EINVAL;
+}
+
+/*

Quoted 6 lines · Ram Pai

arch/powerpc/Kconfig +0 -0

index 9fc3c0b..a4cd210 100644
@@ -864,6 +864,22 @@ config SECCOMP
 

arch/powerpc/Kconfig config SECCOMP · line 867

Quoted 3 lines

arch/powerpc/Kconfig +1 -0

 	  If unsure, say Y. Only embedded should say N here.
 
+config PPC64_MEMORY_PROTECTION_KEYS

That's pretty wordy, can we make it CONFIG_PPC_MEM_KEYS ?

I think you're a sufficient vim wizard to search and replace all usages at once,

if not I can do it before I apply the series.

arch/powerpc/Kconfig config SECCOMP · line 870

Quoted 4 lines

arch/powerpc/Kconfig +3 -0

+	prompt "PowerPC Memory Protection Keys"
+	def_bool y
+	# Note: only available in 64-bit mode

We don't need the note, that's exactly what the next line says:

arch/powerpc/Kconfig config SECCOMP · line 871

Quoted 1 line

arch/powerpc/Kconfig +1 -0

+	depends on PPC64

But shouldn't it be BOOK3S_64 ?

I don't think it works on BookE does it?

arch/powerpc/Kconfig config SECCOMP · line 874

Quoted 3 lines

arch/powerpc/Kconfig +3 -0

+	select ARCH_USES_HIGH_VMA_FLAGS
+	select ARCH_HAS_PKEYS
+	---help---

I prefer just "help".

arch/powerpc/include/asm/mmu_context.h static inline bool arch_vma_access_permitted(struct vm_area_struct *vma,

Quoted 6 lines

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

index 3095925..7badf29 100644
@@ -141,5 +141,10 @@ static inline bool arch_vma_access_permitted(struct vm_area_struct *vma,

	/*

* Disable the pkey system till everything is in place. A patch
* further down the line will enable it.
*/

I'm going to stop commenting on every badly formatted comment :)

Quoted 8 lines · Michael Ellerman

Quoted 6 lines · Ram Pai

arch/powerpc/Kconfig +0 -0

index 9fc3c0b..a4cd210 100644
@@ -864,6 +864,22 @@ config SECCOMP
 

Quoted 8 lines

arch/powerpc/Kconfig config SECCOMP · line 867

Quoted 3 lines

arch/powerpc/Kconfig +1 -0

 	  If unsure, say Y. Only embedded should say N here.
 
+config PPC64_MEMORY_PROTECTION_KEYS

That's pretty wordy, can we make it CONFIG_PPC_MEM_KEYS ?

I think you're a sufficient vim wizard to search and replace all usages at once,

I take that as a compliment for now ;)

Quoted 1 line

if not I can do it before I apply the series.

Will change it...just that I was trying to keep it similar to what intel has X86_INTEL_MEMORY_PROTECTION_KEYS

powerpc needs an additional vma bit to support 32 keys. Till the additional vma bit lands in include/linux/mm.h we have to define it in powerpc specific header file. This is needed to get pkeys working on power.

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

---

Quoted 5 lines · On Fri, 8 Sep 2017 15:44:50 -0700 Ram Pai <[email protected]> wrote:

powerpc needs an additional vma bit to support 32 keys. Till the additional vma bit lands in include/linux/mm.h we have to define it in powerpc specific header file. This is needed to get pkeys working on power.

Quoted 2 lines

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

---

"This" being an arch specific hack for the additional bit?

Quoted 8 lines · Balbir Singh

Quoted 5 lines · On Fri, 8 Sep 2017 15:44:50 -0700 Ram Pai <[email protected]> wrote:

powerpc needs an additional vma bit to support 32 keys. Till the additional vma bit lands in include/linux/mm.h we have to define it in powerpc specific header file. This is needed to get pkeys working on power.

Quoted 4 lines

Quoted 2 lines

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

---

"This" being an arch specific hack for the additional bit?

Arch VMA bits ? really ? I'd rather we limit ourselves to 16 keys first then push for adding the extra bit to the generic code.

Quoted 5 lines · On Fri, 8 Sep 2017 15:44:50 -0700 Ram Pai <[email protected]> wrote:

powerpc needs an additional vma bit to support 32 keys. Till the additional vma bit lands in include/linux/mm.h we have to define it in powerpc specific header file. This is needed to get pkeys working on power.

Quoted 2 lines

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

---

"This" being an arch specific hack for the additional bit?

powerpc needs an additional vma bit to support 32 keys. Till the additional vma bit lands in include/linux/mm.h we have to define it in powerpc specific header file. This is needed to get pkeys working on power.

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

---
 arch/powerpc/include/asm/pkeys.h |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

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

index c02305a..44e01a2 100644
@@ -3,6 +3,24 @@
 
 extern bool pkey_inited;
 extern bool pkey_execute_disable_support;
+
+/*
+ * powerpc needs an additional vma bit to support 32 keys.
+ * Till the additional vma bit lands in include/linux/mm.h
+ * we have to carry the hunk below. This is  needed to get
+ * pkeys working on power. -- Ram
+ */
+#ifndef VM_HIGH_ARCH_BIT_4
+#define VM_HIGH_ARCH_BIT_4	36

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

 static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)

Signature · 1 line

1.7.1

Quoted 33 lines · Ram Pai

powerpc needs an additional vma bit to support 32 keys. Till the additional vma bit lands in include/linux/mm.h we have to define it in powerpc specific header file. This is needed to get pkeys working on power.

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

---
 arch/powerpc/include/asm/pkeys.h |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

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

index c02305a..44e01a2 100644
@@ -3,6 +3,24 @@
 
 extern bool pkey_inited;
 extern bool pkey_execute_disable_support;
+
+/*
+ * powerpc needs an additional vma bit to support 32 keys.
+ * Till the additional vma bit lands in include/linux/mm.h
+ * we have to carry the hunk below. This is  needed to get
+ * pkeys working on power. -- Ram
+ */
+#ifndef VM_HIGH_ARCH_BIT_4
+#define VM_HIGH_ARCH_BIT_4	36
+#define VM_HIGH_ARCH_4	BIT(VM_HIGH_ARCH_BIT_4)
+#define VM_PKEY_SHIFT VM_HIGH_ARCH_BIT_0
+#define VM_PKEY_BIT0	VM_HIGH_ARCH_0
+#define VM_PKEY_BIT1	VM_HIGH_ARCH_1
+#define VM_PKEY_BIT2	VM_HIGH_ARCH_2
+#define VM_PKEY_BIT3	VM_HIGH_ARCH_3

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

Quoted 4 lines

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

+#define VM_PKEY_BIT4	VM_HIGH_ARCH_4
+#endif
+
 #define ARCH_VM_PKEY_FLAGS 0

Do we want them in pkeys.h ? Even if they are arch specific for the existing ones we have them in include/linux/mm.h. IIUC, vmflags details are always in mm.h? This will be the first exception to that?

Quoted 35 lines · Aneesh Kumar K.V

Quoted 33 lines · Ram Pai

powerpc needs an additional vma bit to support 32 keys. Till the additional vma bit lands in include/linux/mm.h we have to define it in powerpc specific header file. This is needed to get pkeys working on power.

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

---
 arch/powerpc/include/asm/pkeys.h |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

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

index c02305a..44e01a2 100644
@@ -3,6 +3,24 @@
 
 extern bool pkey_inited;
 extern bool pkey_execute_disable_support;
+
+/*
+ * powerpc needs an additional vma bit to support 32 keys.
+ * Till the additional vma bit lands in include/linux/mm.h
+ * we have to carry the hunk below. This is  needed to get
+ * pkeys working on power. -- Ram
+ */
+#ifndef VM_HIGH_ARCH_BIT_4
+#define VM_HIGH_ARCH_BIT_4	36
+#define VM_HIGH_ARCH_4	BIT(VM_HIGH_ARCH_BIT_4)
+#define VM_PKEY_SHIFT VM_HIGH_ARCH_BIT_0
+#define VM_PKEY_BIT0	VM_HIGH_ARCH_0
+#define VM_PKEY_BIT1	VM_HIGH_ARCH_1
+#define VM_PKEY_BIT2	VM_HIGH_ARCH_2
+#define VM_PKEY_BIT3	VM_HIGH_ARCH_3

Quoted 8 lines

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

Quoted 4 lines

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

+#define VM_PKEY_BIT4	VM_HIGH_ARCH_4
+#endif
+
 #define ARCH_VM_PKEY_FLAGS 0

Do we want them in pkeys.h ? Even if they are arch specific for the existing ones we have them in include/linux/mm.h. IIUC, vmflags details are always in mm.h? This will be the first exception to that?

Also can we move that

#if defined (CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS)

# define VM_PKEY_SHIFT	VM_HIGH_ARCH_BIT_0
# define VM_PKEY_BIT0	VM_HIGH_ARCH_0	/* A protection key is a 4-bit value */
# define VM_PKEY_BIT1	VM_HIGH_ARCH_1
# define VM_PKEY_BIT2	VM_HIGH_ARCH_2
# define VM_PKEY_BIT3	VM_HIGH_ARCH_3
#endif

to

#if defined (CONFIG_ARCH_HAS_PKEYS)

# define VM_PKEY_SHIFT	VM_HIGH_ARCH_BIT_0
# define VM_PKEY_BIT0	VM_HIGH_ARCH_0	/* A protection key is a 4-bit value */
# define VM_PKEY_BIT1	VM_HIGH_ARCH_1
# define VM_PKEY_BIT2	VM_HIGH_ARCH_2
# define VM_PKEY_BIT3	VM_HIGH_ARCH_3
#endif

And then later update the generic to handle PKEY_BIT4?

Quoted 33 lines · Ram Pai

powerpc needs an additional vma bit to support 32 keys. Till the additional vma bit lands in include/linux/mm.h we have to define it in powerpc specific header file. This is needed to get pkeys working on power.

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

---
 arch/powerpc/include/asm/pkeys.h |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

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

index c02305a..44e01a2 100644
@@ -3,6 +3,24 @@
 
 extern bool pkey_inited;
 extern bool pkey_execute_disable_support;
+
+/*
+ * powerpc needs an additional vma bit to support 32 keys.
+ * Till the additional vma bit lands in include/linux/mm.h
+ * we have to carry the hunk below. This is  needed to get
+ * pkeys working on power. -- Ram
+ */
+#ifndef VM_HIGH_ARCH_BIT_4
+#define VM_HIGH_ARCH_BIT_4	36
+#define VM_HIGH_ARCH_4	BIT(VM_HIGH_ARCH_BIT_4)
+#define VM_PKEY_SHIFT VM_HIGH_ARCH_BIT_0
+#define VM_PKEY_BIT0	VM_HIGH_ARCH_0
+#define VM_PKEY_BIT1	VM_HIGH_ARCH_1
+#define VM_PKEY_BIT2	VM_HIGH_ARCH_2
+#define VM_PKEY_BIT3	VM_HIGH_ARCH_3

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

Quoted 4 lines

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

+#define VM_PKEY_BIT4	VM_HIGH_ARCH_4
+#endif
+
 #define ARCH_VM_PKEY_FLAGS 0

Do we want them in pkeys.h ? Even if they are arch specific for the existing ones we have them in include/linux/mm.h. IIUC, vmflags details are always in mm.h? This will be the first exception to that?

Total 32 keys are available on power7 and above. However pkey 0,1 are reserved. So effectively we have 30 pkeys.

On 4K kernels, we do not have 5 bits in the PTE to represent all the keys; we only have 3bits.Two of those keys are reserved; pkey 0 and pkey 1. So effectively we have 6 pkeys.

This patch keeps track of reserved keys, allocated keys and keys that are currently free.

Also it adds skeletal functions and macros, that the architecture-independent code expects to be available.

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

---
 arch/powerpc/include/asm/book3s/64/mmu.h |    9 ++++
 arch/powerpc/include/asm/mmu_context.h   |    1 +

arch/powerpc/include/asm/pkeys.h | 72 ++++++++++++++++++++++++=

 arch/powerpc/mm/mmu_context_book3s64.c   |    2 +
 arch/powerpc/mm/pkeys.c                  |   28 ++++++++++++
 5 files changed, 108 insertions(+), 4 deletions(-)

Quoted 14 lines · Ram Pai

Total 32 keys are available on power7 and above. However pkey 0,1 are reserved. So effectively we have 30 pkeys.

On 4K kernels, we do not have 5 bits in the PTE to represent all the keys; we only have 3bits.Two of those keys are reserved; pkey 0 and pkey 1. So effectively we have 6 pkeys.

This patch keeps track of reserved keys, allocated keys and keys that are currently free.

Also it adds skeletal functions and macros, that the architecture-independent code expects to be available.

Quoted 8 lines

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

---
 arch/powerpc/include/asm/book3s/64/mmu.h |    9 ++++
 arch/powerpc/include/asm/mmu_context.h   |    1 +
 arch/powerpc/include/asm/pkeys.h         |   72 ++++++++++++++++++++++++++++--
 arch/powerpc/mm/mmu_context_book3s64.c   |    2 +
 arch/powerpc/mm/pkeys.c                  |   28 ++++++++++++
 5 files changed, 108 insertions(+), 4 deletions(-)

This doesn't build for me, with pseries_le_defconfig. I assume it built for you. So something has changed upstream maybe?

In file included from ../include/linux/pkeys.h:8:0,

                 from ../mm/mprotect.c:26:
../mm/mprotect.c: In function ‘do_mprotect_pkey’:
../arch/powerpc/include/asm/pkeys.h:27:29: error: ‘VM_PKEY_BIT0’ undeclared (first use in this function)
 #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
                             ^