Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1668160 > unrolled thread
| Started by | Ram Pai <linuxram@us.ibm.com> |
|---|---|
| First post | 2017-06-17 06:00 +0200 |
| Last post | 2017-06-21 02:20 +0200 |
| Articles | 20 on this page of 50 — 6 participants |
Back to article view | Back to linux.kernel
[RFC v2 00/12] powerpc: Memory Protection Keys Ram Pai <linuxram@us.ibm.com> - 2017-06-17 06:00 +0200
[RFC v2 09/12] powerpc: Deliver SEGV signal on pkey violation. Ram Pai <linuxram@us.ibm.com> - 2017-06-17 06:00 +0200
Re: [RFC v2 09/12] powerpc: Deliver SEGV signal on pkey violation. Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-06-20 09:00 +0200
Re: [RFC v2 09/12] powerpc: Deliver SEGV signal on pkey violation. Ram Pai <linuxram@us.ibm.com> - 2017-06-21 02:00 +0200
Re: [RFC v2 09/12] powerpc: Deliver SEGV signal on pkey violation. Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-06-21 05:20 +0200
Re: [RFC v2 09/12] powerpc: Deliver SEGV signal on pkey violation. Ram Pai <linuxram@us.ibm.com> - 2017-06-21 08:20 +0200
[RFC v2 07/12] powerpc: Macro the mask used for checking DSI exception Ram Pai <linuxram@us.ibm.com> - 2017-06-17 06:00 +0200
Re: [RFC v2 07/12] powerpc: Macro the mask used for checking DSI exception Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-06-20 10:20 +0200
Re: [RFC v2 07/12] powerpc: Macro the mask used for checking DSI exception Ram Pai <linuxram@us.ibm.com> - 2017-06-21 01:30 +0200
Re: [RFC v2 07/12] powerpc: Macro the mask used for checking DSI exception "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2017-06-21 09:30 +0200
Re: [RFC v2 07/12] powerpc: Macro the mask used for checking DSI exception Ram Pai <linuxram@us.ibm.com> - 2017-06-21 11:20 +0200
[RFC v2 04/12] powerpc: store and restore the pkey state across context switches. Ram Pai <linuxram@us.ibm.com> - 2017-06-17 06:00 +0200
[RFC v2 05/12] powerpc: Implementation for sys_mprotect_pkey() system call. Ram Pai <linuxram@us.ibm.com> - 2017-06-17 06:00 +0200
Re: [RFC v2 05/12] powerpc: Implementation for sys_mprotect_pkey() system call. "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2017-06-21 09:20 +0200
[RFC v2 11/12]Documentation: Documentation updates. Ram Pai <linuxram@us.ibm.com> - 2017-06-17 06:00 +0200
Re: [RFC v2 11/12]Documentation: Documentation updates. Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-06-20 08:20 +0200
Re: [RFC v2 11/12]Documentation: Documentation updates. Ram Pai <linuxram@us.ibm.com> - 2017-06-21 02:10 +0200
[RFC v2 01/12] powerpc: Free up four 64K PTE bits in 4K backed hpte pages. Ram Pai <linuxram@us.ibm.com> - 2017-06-17 06:00 +0200
Re: [RFC v2 01/12] powerpc: Free up four 64K PTE bits in 4K backed hpte pages. Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-06-20 12:30 +0200
Re: [RFC v2 01/12] powerpc: Free up four 64K PTE bits in 4K backed hpte pages. Ram Pai <linuxram@us.ibm.com> - 2017-06-21 01:30 +0200
Re: [RFC v2 01/12] powerpc: Free up four 64K PTE bits in 4K backed hpte pages. Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-06-21 07:40 +0200
Re: [RFC v2 01/12] powerpc: Free up four 64K PTE bits in 4K backed hpte pages. Ram Pai <linuxram@us.ibm.com> - 2017-06-21 08:40 +0200
Re: [RFC v2 01/12] powerpc: Free up four 64K PTE bits in 4K backed hpte pages. "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2017-06-21 08:50 +0200
Re: [RFC v2 01/12] powerpc: Free up four 64K PTE bits in 4K backed hpte pages. Ram Pai <linuxram@us.ibm.com> - 2017-06-21 11:40 +0200
Re: [RFC v2 01/12] powerpc: Free up four 64K PTE bits in 4K backed hpte pages. Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-06-22 11:10 +0200
Re: [RFC v2 01/12] powerpc: Free up four 64K PTE bits in 4K backed hpte pages. Ram Pai <linuxram@us.ibm.com> - 2017-06-22 18:30 +0200
[RFC v2 10/12] powerpc: Read AMR only if pkey-violation caused the exception. Ram Pai <linuxram@us.ibm.com> - 2017-06-17 06:00 +0200
Re: [RFC v2 10/12] powerpc: Read AMR only if pkey-violation caused the exception. Michael Ellerman <mpe@ellerman.id.au> - 2017-06-19 13:10 +0200
Re: [RFC v2 10/12] powerpc: Read AMR only if pkey-violation caused the exception. Ram Pai <linuxram@us.ibm.com> - 2017-06-19 20:00 +0200
Re: [RFC v2 10/12] powerpc: Read AMR only if pkey-violation caused the exception. Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-06-20 08:50 +0200
Re: [RFC v2 10/12] powerpc: Read AMR only if pkey-violation caused the exception. Ram Pai <linuxram@us.ibm.com> - 2017-06-21 02:00 +0200
Re: [RFC v2 10/12] powerpc: Read AMR only if pkey-violation caused the exception. Ram Pai <linuxram@us.ibm.com> - 2017-06-21 02:00 +0200
[RFC v2 03/12] powerpc: Implement sys_pkey_alloc and sys_pkey_free system call. Ram Pai <linuxram@us.ibm.com> - 2017-06-17 06:00 +0200
Re: [RFC v2 03/12] powerpc: Implement sys_pkey_alloc and sys_pkey_free system call. Michael Ellerman <mpe@ellerman.id.au> - 2017-06-19 14:20 +0200
Re: [RFC v2 03/12] powerpc: Implement sys_pkey_alloc and sys_pkey_free system call. Ram Pai <linuxram@us.ibm.com> - 2017-06-21 00:50 +0200
[RFC v2 06/12] powerpc: Program HPTE key protection bits. Ram Pai <linuxram@us.ibm.com> - 2017-06-17 06:00 +0200
Re: [RFC v2 06/12] powerpc: Program HPTE key protection bits. Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-06-20 10:30 +0200
Re: [RFC v2 06/12] powerpc: Program HPTE key protection bits. Ram Pai <linuxram@us.ibm.com> - 2017-06-21 01:30 +0200
[RFC v2 02/12] powerpc: Free up four 64K PTE bits in 64K backed hpte pages. Ram Pai <linuxram@us.ibm.com> - 2017-06-17 06:00 +0200
Re: [RFC v2 02/12] powerpc: Free up four 64K PTE bits in 64K backed hpte pages. Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-06-20 13:00 +0200
Re: [RFC v2 02/12] powerpc: Free up four 64K PTE bits in 64K backed hpte pages. Ram Pai <linuxram@us.ibm.com> - 2017-06-21 01:30 +0200
Re: [RFC v2 02/12] powerpc: Free up four 64K PTE bits in 64K backed hpte pages. "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2017-06-21 09:00 +0200
Re: [RFC v2 02/12] powerpc: Free up four 64K PTE bits in 64K backed hpte pages. Ram Pai <linuxram@us.ibm.com> - 2017-06-21 22:20 +0200
Re: [RFC v2 02/12] powerpc: Free up four 64K PTE bits in 64K backed hpte pages. "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2017-06-21 09:00 +0200
Re: [RFC v2 12/12]selftest: Updated protection key selftest Michael Ellerman <mpe@ellerman.id.au> - 2017-06-19 13:10 +0200
Re: [RFC v2 00/12] powerpc: Memory Protection Keys Balbir Singh <bsingharora@gmail.com> - 2017-06-20 07:20 +0200
Re: [RFC v2 00/12] powerpc: Memory Protection Keys Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-06-20 08:10 +0200
Re: [RFC v2 00/12] powerpc: Memory Protection Keys Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2017-06-20 12:00 +0200
Re: [RFC v2 12/12]selftest: Updated protection key selftest Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-06-20 08:30 +0200
Re: [RFC v2 12/12]selftest: Updated protection key selftest Ram Pai <linuxram@us.ibm.com> - 2017-06-21 02:20 +0200
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
| From | Anshuman Khandual <khandual@linux.vnet.ibm.com> |
|---|---|
| Date | 2017-06-21 07:40 +0200 |
| Subject | Re: [RFC v2 01/12] powerpc: Free up four 64K PTE bits in 4K backed hpte pages. |
| Message-ID | <tUGwV-1WC-11@gated-at.bofh.it> |
| In reply to | #1671231 |
On 06/21/2017 04:53 AM, Ram Pai wrote:
> On Tue, Jun 20, 2017 at 03:50:25PM +0530, Anshuman Khandual wrote:
>> On 06/17/2017 09:22 AM, Ram Pai wrote:
>>> 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 counting 3, 4, 5 and 6 are in BE format I believe, I was
>> initially trying to see that from right to left as we normally
>> do in the kernel and was getting confused. So basically these
>> bits (which are only applicable for 64K mapping IIUC) are going
>> to be freed up from the PTE format.
>>
>> #define _RPAGE_RSV1 0x1000000000000000UL
>> #define _RPAGE_RSV2 0x0800000000000000UL
>> #define _RPAGE_RSV3 0x0400000000000000UL
>> #define _RPAGE_RSV4 0x0200000000000000UL
>>
>> As you have mentioned before this feature is available for 64K
>> page size only and not for 4K mappings. So I assume we support
>> both the combinations.
>>
>> * 64K mapping on 64K
>> * 64K mapping on 4K
>
> yes.
>
>>
>> These are the current users of the above bits
>>
>> #define H_PAGE_BUSY _RPAGE_RSV1 /* software: PTE & hash are busy */
>> #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 */
>>
>>>
>>> The patch does the following change to the 64K PTE format
>>>
>>> H_PAGE_BUSY moves from bit 3 to bit 9
>>
>> and what is in there on bit 9 now ? This ?
>>
>> #define _RPAGE_SW2 0x00400
>>
>> which is used as
>>
>> #define _PAGE_SPECIAL _RPAGE_SW2 /* software: special page */
>>
>> which will not be required any more ?
>
> i think you are reading bit 9 from right to left. the bit 9 i refer to
> is from left to right. Using the same numbering convention the ISA3.0 uses.
Right, my bad. Then it would be this one.
'#define _RPAGE_RPN42 0x0040000000000000UL'
> I know it is confusing, will make a mention in the comment of this
> patch, to read it the big-endian way.
Right.
>
> BTW: Bit 9 is not used currently. so using it in this patch. But this is
> a temporary move. the H_PAGE_BUSY will move to bit 7 in the next patch.
>
> Had to keep at bit 9, because bit 7 is not yet entirely freed up. it is
> used by 64K PTE backed by 64k htpe.
Got it.
>
>>
>>> H_PAGE_F_SECOND which occupied bit 4 moves to the second part
>>> of the pte.
>>> H_PAGE_F_GIX which occupied bit 5, 6 and 7 also moves to the
>>> second part of the pte.
>>>
>>> the four bits((H_PAGE_F_SECOND|H_PAGE_F_GIX) that represent a slot
>>> is initialized to 0xF indicating an invalid slot. If a hpte
>>> gets cached in a 0xF slot(i.e 7th slot of secondary), it is
>>> released immediately. In other words, even though 0xF is a
>>
>> Release immediately means we attempt again for a new hash slot ?
>
> yes.
>
>>
>>> valid slot we discard and consider it as an invalid
>>> slot;i.e hpte_soft_invalid(). This gives us an opportunity to not
>>> depend on a bit in the primary PTE in order to determine the
>>> validity of a slot.
>>
>> So we have to see the slot number in the second half for each PTE to
>> figure out if it has got a valid slot in the hash page table.
>
> yes.
>
>>
>>>
>>> When we release a hpte in the 0xF slot we also release a
>>> legitimate primary slot and unmap that entry. This is to
>>> ensure that we do get a legimate non-0xF slot the next time we
>>> retry for a slot.
>>
>> Okay.
>>
>>>
>>> Though treating 0xF slot as invalid reduces the number of available
>>> slots and may have an effect on the performance, the probabilty
>>> of hitting a 0xF is extermely low.
>>
>> Why you say that ? I thought every slot number has the same probability
>> of hit from the hash function.
>
> Every hash bucket has the same probability. But every slot within the
> hash bucket is filled in sequentially. so it takes 15 hptes to hash to
> the same bucket before we get to the 15th slot in the secondary.
Okay, would the last one be 16th instead ?
>
>>
>>>
>>> Compared to the current scheme, the above described scheme reduces
>>> the number of false hash table updates significantly and has the
>>
>> How it reduces false hash table updates ?
>
> earlier, we had 1 bit allocated in the first-part-of-the 64K-PTE
> for four consecutive 4K hptes. If any one 4k hpte got hashed-in,
> the bit got set. Which means anytime it faulted on the remaining
> three 4k hpte, we saw the bit already set and tried to erroneously
> update that hpte. So we had a 75% update error rate. Funcationally
> not bad, but bad from a performance point of view.
I am bit out of sync regarding these PTE bits, after Aneesh's radix
changes went in :) Will look into this bit closer.
>
> With the current scheme, we decide if a 4k slot is valid by looking
> at its value rather than depending on a bit in the main-pte. So
> there is no chance of getting mislead. And hence no chance of trying
> to update a invalid hpte. Should improve performance and at the same
> time give us four valuable PTE bits.
I am not sure why you say 'invalid hpte'. IIUC
* We will require 16 '64K on 4K' mappings to actually cover 64K on 64K
* A single (64K on 4K)'s TLB can cover 64K on 64K as long as the TLB is
present and not flushed. That gets us performance. Once flushed, a new
HPTE entry covering new (64K on 4K) is inserted. As long as the PFN
for the 4K is different HPTE will be different and it cannot collide
with any existing ones and create problems (ERAT error ?)
As you are pointing out, I am not sure whether the existing design had
more probability for an invalid HPTE insert. Will look into this in
detail.
>
>
>>
>>> added advantage of releasing four valuable PTE bits for other
>>> purpose.
>>>
>>> This idea was jointly developed by Paul Mackerras, Aneesh, Michael
>>> Ellermen and myself.
>>>
>>> 4K PTE format remain unchanged currently.
>>>
>>> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>>> ---
>>> arch/powerpc/include/asm/book3s/64/hash-4k.h | 20 +++++++
>>> arch/powerpc/include/asm/book3s/64/hash-64k.h | 32 +++++++----
>>> arch/powerpc/include/asm/book3s/64/hash.h | 15 +++--
>>> arch/powerpc/include/asm/book3s/64/mmu-hash.h | 5 ++
>>> arch/powerpc/mm/dump_linuxpagetables.c | 3 +-
>>> arch/powerpc/mm/hash64_4k.c | 14 ++---
>>> arch/powerpc/mm/hash64_64k.c | 81 ++++++++++++---------------
>>> arch/powerpc/mm/hash_utils_64.c | 30 +++++++---
>>> 8 files changed, 122 insertions(+), 78 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/book3s/64/hash-4k.h b/arch/powerpc/include/asm/book3s/64/hash-4k.h
>>> index b4b5e6b..5ef1d81 100644
>>> --- a/arch/powerpc/include/asm/book3s/64/hash-4k.h
>>> +++ b/arch/powerpc/include/asm/book3s/64/hash-4k.h
>>> @@ -16,6 +16,18 @@
>>> #define H_PUD_TABLE_SIZE (sizeof(pud_t) << H_PUD_INDEX_SIZE)
>>> #define H_PGD_TABLE_SIZE (sizeof(pgd_t) << H_PGD_INDEX_SIZE)
>>>
>>> +
>>> +/*
>>> + * Only supported by 4k linux page size
>>> + */
>>> +#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_F_GIX_SHIFT 56
>>> +
>>> +#define H_PAGE_BUSY _RPAGE_RSV1 /* software: PTE & hash are busy */
>>> +#define H_PAGE_HASHPTE _RPAGE_RPN43 /* PTE has associated HPTE */
>>> +
>>> +
>>
>> So we moved the common 64K definitions here.
>
> yes.
>>
>>
>>> /* PTE flags to conserve for HPTE identification */
>>> #define _PAGE_HPTEFLAGS (H_PAGE_BUSY | H_PAGE_HASHPTE | \
>>> H_PAGE_F_SECOND | H_PAGE_F_GIX)
>>> @@ -48,6 +60,14 @@ static inline int hash__hugepd_ok(hugepd_t hpd)
>>> }
>>> #endif
>>>
>>> +static inline unsigned long set_hidx_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);
>>> +}
>>
>> Why we are passing the first 3 arguments of the function if we never
>> use it inside. Is the caller expected to take care of it ?
>
> trying to keep the same prototype for the 4K-pte and 64K-pte cases.
> Otherwise the caller has to wonder which parameter scheme to use.
>
>>
>>> +
>>> +
>>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>>
>>> static inline char *get_hpte_slot_array(pmd_t *pmdp)
>>> diff --git a/arch/powerpc/include/asm/book3s/64/hash-64k.h b/arch/powerpc/include/asm/book3s/64/hash-64k.h
>>> index 9732837..0eb3c89 100644
>>> --- a/arch/powerpc/include/asm/book3s/64/hash-64k.h
>>> +++ b/arch/powerpc/include/asm/book3s/64/hash-64k.h
>>> @@ -10,23 +10,25 @@
>>> * 64k aligned address free up few of the lower bits of RPN for us
>>> * We steal that here. For more deatils look at pte_pfn/pfn_pte()
>>> */
>>> -#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_COMBO _RPAGE_RPN0 /* this is a combo 4k page */
>>> +#define H_PAGE_4K_PFN _RPAGE_RPN1 /* PFN is for a single 4k page */
>>
>> Its the same thing, changes nothing.
>
> it fixes some space/tab problem.
>
>>
>>> +#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_F_GIX_SHIFT 56
>>> +
>>> +
>>> +#define H_PAGE_BUSY _RPAGE_RPN42 /* software: PTE & hash are busy */
>>> +#define H_PAGE_HASHPTE _RPAGE_RPN43 /* PTE has associated HPTE */
>>
>> H_PAGE_BUSY seems to be differently defined here.
>
> Yes. it is using two different bits depending on 4K hpte v/s 64k hpte
> case. But in the next patch all will be same and consistent.
>
>>
>>> +
>>> /*
>>> * We need to differentiate between explicit huge page and THP huge
>>> * page, since THP huge page also need to track real subpage details
>>> */
>>> #define H_PAGE_THP_HUGE H_PAGE_4K_PFN
>>>
>>> -/*
>>> - * Used to track subpage group valid if H_PAGE_COMBO is set
>>> - * This overloads H_PAGE_F_GIX and H_PAGE_F_SECOND
>>> - */
>>> -#define H_PAGE_COMBO_VALID (H_PAGE_F_GIX | H_PAGE_F_SECOND)
>>
>> H_PAGE_COMBO_VALID is not defined alternately ?
>
> it is not needed anymore.
>
>>
>>> -
>>> /* 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)
>>> +
>>
>> Slot information has moved to the second half, hence _PAGE_HPTEFLAGS
>> need not carry that.
>
> yes.
>
>>
>>> /*
>>> * we support 16 fragments per PTE page of 64K size.
>>> */
>>> @@ -74,6 +76,16 @@ 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;
>>> }
>>>
>>> +static inline unsigned long set_hidx_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));
>>> + return 0x0UL;
>>> +}
>>
>> New method to insert the slot information in the second half.
>
> yes. well it basically trying to reduce code redundancy. Too many places
> using exactly the same code to accomplish the same thing. Makes sense to
> bring it all in one place.
Right.
>
>>
>>> +
>>> #define __rpte_to_pte(r) ((r).pte)
>>> extern bool __rpte_sub_valid(real_pte_t rpte, unsigned long index);
>>> /*
>>> diff --git a/arch/powerpc/include/asm/book3s/64/hash.h b/arch/powerpc/include/asm/book3s/64/hash.h
>>> index 4e957b0..e7cf03a 100644
>>> --- a/arch/powerpc/include/asm/book3s/64/hash.h
>>> +++ b/arch/powerpc/include/asm/book3s/64/hash.h
>>> @@ -8,11 +8,8 @@
>>> *
>>> */
>>> #define H_PTE_NONE_MASK _PAGE_HPTEFLAGS
>>> -#define H_PAGE_F_GIX_SHIFT 56
>>> -#define H_PAGE_BUSY _RPAGE_RSV1 /* software: PTE & hash are busy */
>>> -#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 */
>>
>> Removing the common definitions.
>>
>>> +
>>> +#define INIT_HIDX (~0x0UL)
>>>
>>> #ifdef CONFIG_PPC_64K_PAGES
>>> #include <asm/book3s/64/hash-64k.h>
>>> @@ -160,6 +157,14 @@ static inline int hash__pte_none(pte_t pte)
>>> return (pte_val(pte) & ~H_PTE_NONE_MASK) == 0;
>>> }
>>>
>>> +static inline bool hpte_soft_invalid(unsigned long slot)
>>> +{
>>> + return ((slot & 0xfUL) == 0xfUL);
>>> +}
>>> +
>>> +unsigned long get_hidx_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
>>> diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
>>> index 6981a52..cfb8169 100644
>>> --- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h
>>> +++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
>>> @@ -435,6 +435,11 @@ extern int __hash_page_4K(unsigned long ea, unsigned long access,
>>> extern int __hash_page_64K(unsigned long ea, unsigned long access,
>>> unsigned long vsid, pte_t *ptep, unsigned long trap,
>>> unsigned long flags, int ssize);
>>> +extern unsigned long set_hidx_slot(pte_t *ptep, real_pte_t rpte,
>>> + unsigned int subpg_index, unsigned long slot);
>>> +extern unsigned long get_hidx_slot(unsigned long vpn, unsigned long shift,
>>> + int ssize, real_pte_t rpte, unsigned int subpg_index);
>>
>> I wonder what purpose set_hidx_slot() defined previously, served.
>>
>>> +
>>> struct mm_struct;
>>> unsigned int hash_page_do_lazy_icache(unsigned int pp, pte_t pte, int trap);
>>> extern int hash_page_mm(struct mm_struct *mm, unsigned long ea,
>>> diff --git a/arch/powerpc/mm/dump_linuxpagetables.c b/arch/powerpc/mm/dump_linuxpagetables.c
>>> index 44fe483..b832ed3 100644
>>> --- a/arch/powerpc/mm/dump_linuxpagetables.c
>>> +++ b/arch/powerpc/mm/dump_linuxpagetables.c
>>> @@ -213,7 +213,7 @@ struct flag_info {
>>> .val = H_PAGE_4K_PFN,
>>> .set = "4K_pfn",
>>> }, {
>>> -#endif
>>> +#else
>>> .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 */
>>
>> Are we adding H_PAGE_F_GIX as an element for 4K mapping ?
>
> I think there is mistake here.
> In the next patch when these bits are divorsed from
> 64K ptes entirely, we will not need the above code for 64K ptes.
> But good catch. Will fix the error in this patch.
>
>>
>>> #endif
>>> .mask = _PAGE_SPECIAL,
>>> .val = _PAGE_SPECIAL,
>>> diff --git a/arch/powerpc/mm/hash64_4k.c b/arch/powerpc/mm/hash64_4k.c
>>> index 6fa450c..c673829 100644
>>> --- a/arch/powerpc/mm/hash64_4k.c
>>> +++ b/arch/powerpc/mm/hash64_4k.c
>>> @@ -20,6 +20,7 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
>>> pte_t *ptep, unsigned long trap, unsigned long flags,
>>> int ssize, int subpg_prot)
>>> {
>>> + real_pte_t rpte;
>>> unsigned long hpte_group;
>>> unsigned long rflags, pa;
>>> unsigned long old_pte, new_pte;
>>> @@ -54,6 +55,7 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
>>> * need to add in 0x1 if it's a read-only user page
>>> */
>>> 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))
>>> @@ -64,13 +66,10 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
>>> /*
>>> * 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;
>>> + unsigned long gslot = get_hidx_gslot(vpn, shift,
>>> + ssize, rpte, 0);
>>
>> I am wondering why there is a 'g' before the slot in all these
>> functions.
>
> Right. even i was confused initially. :)
>
> hash table slots are originized as one big table. 8 consecutive entires
> in that table form a bucket. the term slot is used to refer to the
> slot within the bucket. the term gslot is used to refer to an entry
> in the table. roughly speaking slot 2 in bucket 2, will be gslot 2*8+2=18.
Global slot as it can point any where on that two dimensional table ?
>
>>
>> Its already too much of changes in a single patch. Being a single
>> logical change it needs to be inside a single change but then we
>> need much more description in the commit message for some one to
>> understand what all changed and how.
>
> I have further broken down this patch, one to introduce get_hidx_gslot()
> one to introduce set_hidx_slot() . Hopefully that will reduce the size
> of the patch to graspable level. let me know,
I did some experiments with the first two patches.
* First of all the first patch does not compile without this.
--- a/arch/powerpc/mm/hash_utils_64.c
+++ b/arch/powerpc/mm/hash_utils_64.c
@@ -1612,7 +1612,7 @@ unsigned long get_hidx_gslot(unsigned long vpn, unsigned long shift,
void flush_hash_page(unsigned long vpn, real_pte_t pte, int psize, int ssize,
unsigned long flags)
{
- unsigned long hash, index, shift, hidx, gslot;
+ unsigned long index, shift, gslot;
int local = flags & HPTE_LOCAL_UPDATE;
DBG_LOW("flush_hash_page(vpn=%016lx)\n", vpn);
* Though it boots the kernel, system is kind of unresponsive while attempting
to compile a kernel. Though I did not dig further on this, seems like the
first patch is not self sufficient yet.
* With both first and second patch, the kernel boots fine and compiles a kernel.
We need to sort out issues in the first two patches before looking into
the rest of the patch series.
[toc] | [prev] | [next] | [standalone]
| From | Ram Pai <linuxram@us.ibm.com> |
|---|---|
| Date | 2017-06-21 08:40 +0200 |
| Subject | Re: [RFC v2 01/12] powerpc: Free up four 64K PTE bits in 4K backed hpte pages. |
| Message-ID | <tUHt0-2vO-35@gated-at.bofh.it> |
| In reply to | #1671370 |
On Wed, Jun 21, 2017 at 11:05:33AM +0530, Anshuman Khandual wrote:
> On 06/21/2017 04:53 AM, Ram Pai wrote:
> > On Tue, Jun 20, 2017 at 03:50:25PM +0530, Anshuman Khandual wrote:
> >> On 06/17/2017 09:22 AM, Ram Pai wrote:
> >>> 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 counting 3, 4, 5 and 6 are in BE format I believe, I was
> >> initially trying to see that from right to left as we normally
> >> do in the kernel and was getting confused. So basically these
> >> bits (which are only applicable for 64K mapping IIUC) are going
> >> to be freed up from the PTE format.
> >>
> >> #define _RPAGE_RSV1 0x1000000000000000UL
> >> #define _RPAGE_RSV2 0x0800000000000000UL
> >> #define _RPAGE_RSV3 0x0400000000000000UL
> >> #define _RPAGE_RSV4 0x0200000000000000UL
> >>
> >> As you have mentioned before this feature is available for 64K
> >> page size only and not for 4K mappings. So I assume we support
> >> both the combinations.
> >>
> >> * 64K mapping on 64K
> >> * 64K mapping on 4K
> >
> > yes.
> >
> >>
> >> These are the current users of the above bits
> >>
> >> #define H_PAGE_BUSY _RPAGE_RSV1 /* software: PTE & hash are busy */
> >> #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 */
> >>
> >>>
> >>> The patch does the following change to the 64K PTE format
> >>>
> >>> H_PAGE_BUSY moves from bit 3 to bit 9
> >>
> >> and what is in there on bit 9 now ? This ?
> >>
> >> #define _RPAGE_SW2 0x00400
> >>
> >> which is used as
> >>
> >> #define _PAGE_SPECIAL _RPAGE_SW2 /* software: special page */
> >>
> >> which will not be required any more ?
> >
> > i think you are reading bit 9 from right to left. the bit 9 i refer to
> > is from left to right. Using the same numbering convention the ISA3.0 uses.
>
> Right, my bad. Then it would be this one.
>
> '#define _RPAGE_RPN42 0x0040000000000000UL'
>
> > I know it is confusing, will make a mention in the comment of this
> > patch, to read it the big-endian way.
>
> Right.
>
> >
> > BTW: Bit 9 is not used currently. so using it in this patch. But this is
> > a temporary move. the H_PAGE_BUSY will move to bit 7 in the next patch.
> >
> > Had to keep at bit 9, because bit 7 is not yet entirely freed up. it is
> > used by 64K PTE backed by 64k htpe.
>
> Got it.
>
> >
> >>
> >>> H_PAGE_F_SECOND which occupied bit 4 moves to the second part
> >>> of the pte.
> >>> H_PAGE_F_GIX which occupied bit 5, 6 and 7 also moves to the
> >>> second part of the pte.
> >>>
> >>> the four bits((H_PAGE_F_SECOND|H_PAGE_F_GIX) that represent a slot
> >>> is initialized to 0xF indicating an invalid slot. If a hpte
> >>> gets cached in a 0xF slot(i.e 7th slot of secondary), it is
> >>> released immediately. In other words, even though 0xF is a
> >>
> >> Release immediately means we attempt again for a new hash slot ?
> >
> > yes.
> >
> >>
> >>> valid slot we discard and consider it as an invalid
> >>> slot;i.e hpte_soft_invalid(). This gives us an opportunity to not
> >>> depend on a bit in the primary PTE in order to determine the
> >>> validity of a slot.
> >>
> >> So we have to see the slot number in the second half for each PTE to
> >> figure out if it has got a valid slot in the hash page table.
> >
> > yes.
> >
> >>
> >>>
> >>> When we release a hpte in the 0xF slot we also release a
> >>> legitimate primary slot and unmap that entry. This is to
> >>> ensure that we do get a legimate non-0xF slot the next time we
> >>> retry for a slot.
> >>
> >> Okay.
> >>
> >>>
> >>> Though treating 0xF slot as invalid reduces the number of available
> >>> slots and may have an effect on the performance, the probabilty
> >>> of hitting a 0xF is extermely low.
> >>
> >> Why you say that ? I thought every slot number has the same probability
> >> of hit from the hash function.
> >
> > Every hash bucket has the same probability. But every slot within the
> > hash bucket is filled in sequentially. so it takes 15 hptes to hash to
> > the same bucket before we get to the 15th slot in the secondary.
>
> Okay, would the last one be 16th instead ?
>
> >
> >>
> >>>
> >>> Compared to the current scheme, the above described scheme reduces
> >>> the number of false hash table updates significantly and has the
> >>
> >> How it reduces false hash table updates ?
> >
> > earlier, we had 1 bit allocated in the first-part-of-the 64K-PTE
> > for four consecutive 4K hptes. If any one 4k hpte got hashed-in,
> > the bit got set. Which means anytime it faulted on the remaining
> > three 4k hpte, we saw the bit already set and tried to erroneously
> > update that hpte. So we had a 75% update error rate. Funcationally
> > not bad, but bad from a performance point of view.
>
> I am bit out of sync regarding these PTE bits, after Aneesh's radix
> changes went in :) Will look into this bit closer.
>
> >
> > With the current scheme, we decide if a 4k slot is valid by looking
> > at its value rather than depending on a bit in the main-pte. So
> > there is no chance of getting mislead. And hence no chance of trying
> > to update a invalid hpte. Should improve performance and at the same
> > time give us four valuable PTE bits.
>
> I am not sure why you say 'invalid hpte'. IIUC
I mean to say a entry which does not yet have a mapped hpte.
>
> * We will require 16 '64K on 4K' mappings to actually cover 64K on 64K
>
> * A single (64K on 4K)'s TLB can cover 64K on 64K as long as the TLB is
> present and not flushed. That gets us performance. Once flushed, a new
> HPTE entry covering new (64K on 4K) is inserted. As long as the PFN
> for the 4K is different HPTE will be different and it cannot collide
> with any existing ones and create problems (ERAT error ?)
>
> As you are pointing out, I am not sure whether the existing design had
> more probability for an invalid HPTE insert. Will look into this in
> detail.
>
> >
> >
> >>
> >>> added advantage of releasing four valuable PTE bits for other
> >>> purpose.
> >>>
> >>> This idea was jointly developed by Paul Mackerras, Aneesh, Michael
> >>> Ellermen and myself.
> >>>
> >>> 4K PTE format remain unchanged currently.
> >>>
> >>> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> >>> ---
> >>> arch/powerpc/include/asm/book3s/64/hash-4k.h | 20 +++++++
> >>> arch/powerpc/include/asm/book3s/64/hash-64k.h | 32 +++++++----
> >>> arch/powerpc/include/asm/book3s/64/hash.h | 15 +++--
> >>> arch/powerpc/include/asm/book3s/64/mmu-hash.h | 5 ++
> >>> arch/powerpc/mm/dump_linuxpagetables.c | 3 +-
> >>> arch/powerpc/mm/hash64_4k.c | 14 ++---
> >>> arch/powerpc/mm/hash64_64k.c | 81 ++++++++++++---------------
> >>> arch/powerpc/mm/hash_utils_64.c | 30 +++++++---
> >>> 8 files changed, 122 insertions(+), 78 deletions(-)
> >>>
> >>> diff --git a/arch/powerpc/include/asm/book3s/64/hash-4k.h b/arch/powerpc/include/asm/book3s/64/hash-4k.h
> >>> index b4b5e6b..5ef1d81 100644
> >>> --- a/arch/powerpc/include/asm/book3s/64/hash-4k.h
> >>> +++ b/arch/powerpc/include/asm/book3s/64/hash-4k.h
> >>> @@ -16,6 +16,18 @@
> >>> #define H_PUD_TABLE_SIZE (sizeof(pud_t) << H_PUD_INDEX_SIZE)
> >>> #define H_PGD_TABLE_SIZE (sizeof(pgd_t) << H_PGD_INDEX_SIZE)
> >>>
> >>> +
> >>> +/*
> >>> + * Only supported by 4k linux page size
> >>> + */
> >>> +#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_F_GIX_SHIFT 56
> >>> +
> >>> +#define H_PAGE_BUSY _RPAGE_RSV1 /* software: PTE & hash are busy */
> >>> +#define H_PAGE_HASHPTE _RPAGE_RPN43 /* PTE has associated HPTE */
> >>> +
> >>> +
> >>
> >> So we moved the common 64K definitions here.
> >
> > yes.
> >>
> >>
> >>> /* PTE flags to conserve for HPTE identification */
> >>> #define _PAGE_HPTEFLAGS (H_PAGE_BUSY | H_PAGE_HASHPTE | \
> >>> H_PAGE_F_SECOND | H_PAGE_F_GIX)
> >>> @@ -48,6 +60,14 @@ static inline int hash__hugepd_ok(hugepd_t hpd)
> >>> }
> >>> #endif
> >>>
> >>> +static inline unsigned long set_hidx_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);
> >>> +}
> >>
> >> Why we are passing the first 3 arguments of the function if we never
> >> use it inside. Is the caller expected to take care of it ?
> >
> > trying to keep the same prototype for the 4K-pte and 64K-pte cases.
> > Otherwise the caller has to wonder which parameter scheme to use.
> >
> >>
> >>> +
> >>> +
> >>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >>>
> >>> static inline char *get_hpte_slot_array(pmd_t *pmdp)
> >>> diff --git a/arch/powerpc/include/asm/book3s/64/hash-64k.h b/arch/powerpc/include/asm/book3s/64/hash-64k.h
> >>> index 9732837..0eb3c89 100644
> >>> --- a/arch/powerpc/include/asm/book3s/64/hash-64k.h
> >>> +++ b/arch/powerpc/include/asm/book3s/64/hash-64k.h
> >>> @@ -10,23 +10,25 @@
> >>> * 64k aligned address free up few of the lower bits of RPN for us
> >>> * We steal that here. For more deatils look at pte_pfn/pfn_pte()
> >>> */
> >>> -#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_COMBO _RPAGE_RPN0 /* this is a combo 4k page */
> >>> +#define H_PAGE_4K_PFN _RPAGE_RPN1 /* PFN is for a single 4k page */
> >>
> >> Its the same thing, changes nothing.
> >
> > it fixes some space/tab problem.
> >
> >>
> >>> +#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_F_GIX_SHIFT 56
> >>> +
> >>> +
> >>> +#define H_PAGE_BUSY _RPAGE_RPN42 /* software: PTE & hash are busy */
> >>> +#define H_PAGE_HASHPTE _RPAGE_RPN43 /* PTE has associated HPTE */
> >>
> >> H_PAGE_BUSY seems to be differently defined here.
> >
> > Yes. it is using two different bits depending on 4K hpte v/s 64k hpte
> > case. But in the next patch all will be same and consistent.
> >
> >>
> >>> +
> >>> /*
> >>> * We need to differentiate between explicit huge page and THP huge
> >>> * page, since THP huge page also need to track real subpage details
> >>> */
> >>> #define H_PAGE_THP_HUGE H_PAGE_4K_PFN
> >>>
> >>> -/*
> >>> - * Used to track subpage group valid if H_PAGE_COMBO is set
> >>> - * This overloads H_PAGE_F_GIX and H_PAGE_F_SECOND
> >>> - */
> >>> -#define H_PAGE_COMBO_VALID (H_PAGE_F_GIX | H_PAGE_F_SECOND)
> >>
> >> H_PAGE_COMBO_VALID is not defined alternately ?
> >
> > it is not needed anymore.
> >
> >>
> >>> -
> >>> /* 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)
> >>> +
> >>
> >> Slot information has moved to the second half, hence _PAGE_HPTEFLAGS
> >> need not carry that.
> >
> > yes.
> >
> >>
> >>> /*
> >>> * we support 16 fragments per PTE page of 64K size.
> >>> */
> >>> @@ -74,6 +76,16 @@ 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;
> >>> }
> >>>
> >>> +static inline unsigned long set_hidx_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));
> >>> + return 0x0UL;
> >>> +}
> >>
> >> New method to insert the slot information in the second half.
> >
> > yes. well it basically trying to reduce code redundancy. Too many places
> > using exactly the same code to accomplish the same thing. Makes sense to
> > bring it all in one place.
>
> Right.
>
> >
> >>
> >>> +
> >>> #define __rpte_to_pte(r) ((r).pte)
> >>> extern bool __rpte_sub_valid(real_pte_t rpte, unsigned long index);
> >>> /*
> >>> diff --git a/arch/powerpc/include/asm/book3s/64/hash.h b/arch/powerpc/include/asm/book3s/64/hash.h
> >>> index 4e957b0..e7cf03a 100644
> >>> --- a/arch/powerpc/include/asm/book3s/64/hash.h
> >>> +++ b/arch/powerpc/include/asm/book3s/64/hash.h
> >>> @@ -8,11 +8,8 @@
> >>> *
> >>> */
> >>> #define H_PTE_NONE_MASK _PAGE_HPTEFLAGS
> >>> -#define H_PAGE_F_GIX_SHIFT 56
> >>> -#define H_PAGE_BUSY _RPAGE_RSV1 /* software: PTE & hash are busy */
> >>> -#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 */
> >>
> >> Removing the common definitions.
> >>
> >>> +
> >>> +#define INIT_HIDX (~0x0UL)
> >>>
> >>> #ifdef CONFIG_PPC_64K_PAGES
> >>> #include <asm/book3s/64/hash-64k.h>
> >>> @@ -160,6 +157,14 @@ static inline int hash__pte_none(pte_t pte)
> >>> return (pte_val(pte) & ~H_PTE_NONE_MASK) == 0;
> >>> }
> >>>
> >>> +static inline bool hpte_soft_invalid(unsigned long slot)
> >>> +{
> >>> + return ((slot & 0xfUL) == 0xfUL);
> >>> +}
> >>> +
> >>> +unsigned long get_hidx_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
> >>> diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> >>> index 6981a52..cfb8169 100644
> >>> --- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> >>> +++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> >>> @@ -435,6 +435,11 @@ extern int __hash_page_4K(unsigned long ea, unsigned long access,
> >>> extern int __hash_page_64K(unsigned long ea, unsigned long access,
> >>> unsigned long vsid, pte_t *ptep, unsigned long trap,
> >>> unsigned long flags, int ssize);
> >>> +extern unsigned long set_hidx_slot(pte_t *ptep, real_pte_t rpte,
> >>> + unsigned int subpg_index, unsigned long slot);
> >>> +extern unsigned long get_hidx_slot(unsigned long vpn, unsigned long shift,
> >>> + int ssize, real_pte_t rpte, unsigned int subpg_index);
> >>
> >> I wonder what purpose set_hidx_slot() defined previously, served.
> >>
> >>> +
> >>> struct mm_struct;
> >>> unsigned int hash_page_do_lazy_icache(unsigned int pp, pte_t pte, int trap);
> >>> extern int hash_page_mm(struct mm_struct *mm, unsigned long ea,
> >>> diff --git a/arch/powerpc/mm/dump_linuxpagetables.c b/arch/powerpc/mm/dump_linuxpagetables.c
> >>> index 44fe483..b832ed3 100644
> >>> --- a/arch/powerpc/mm/dump_linuxpagetables.c
> >>> +++ b/arch/powerpc/mm/dump_linuxpagetables.c
> >>> @@ -213,7 +213,7 @@ struct flag_info {
> >>> .val = H_PAGE_4K_PFN,
> >>> .set = "4K_pfn",
> >>> }, {
> >>> -#endif
> >>> +#else
> >>> .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 */
> >>
> >> Are we adding H_PAGE_F_GIX as an element for 4K mapping ?
> >
> > I think there is mistake here.
> > In the next patch when these bits are divorsed from
> > 64K ptes entirely, we will not need the above code for 64K ptes.
> > But good catch. Will fix the error in this patch.
> >
> >>
> >>> #endif
> >>> .mask = _PAGE_SPECIAL,
> >>> .val = _PAGE_SPECIAL,
> >>> diff --git a/arch/powerpc/mm/hash64_4k.c b/arch/powerpc/mm/hash64_4k.c
> >>> index 6fa450c..c673829 100644
> >>> --- a/arch/powerpc/mm/hash64_4k.c
> >>> +++ b/arch/powerpc/mm/hash64_4k.c
> >>> @@ -20,6 +20,7 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> >>> pte_t *ptep, unsigned long trap, unsigned long flags,
> >>> int ssize, int subpg_prot)
> >>> {
> >>> + real_pte_t rpte;
> >>> unsigned long hpte_group;
> >>> unsigned long rflags, pa;
> >>> unsigned long old_pte, new_pte;
> >>> @@ -54,6 +55,7 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> >>> * need to add in 0x1 if it's a read-only user page
> >>> */
> >>> 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))
> >>> @@ -64,13 +66,10 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> >>> /*
> >>> * 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;
> >>> + unsigned long gslot = get_hidx_gslot(vpn, shift,
> >>> + ssize, rpte, 0);
> >>
> >> I am wondering why there is a 'g' before the slot in all these
> >> functions.
> >
> > Right. even i was confused initially. :)
> >
> > hash table slots are originized as one big table. 8 consecutive entires
> > in that table form a bucket. the term slot is used to refer to the
> > slot within the bucket. the term gslot is used to refer to an entry
> > in the table. roughly speaking slot 2 in bucket 2, will be gslot 2*8+2=18.
>
> Global slot as it can point any where on that two dimensional table ?
>
> >
> >>
> >> Its already too much of changes in a single patch. Being a single
> >> logical change it needs to be inside a single change but then we
> >> need much more description in the commit message for some one to
> >> understand what all changed and how.
> >
> > I have further broken down this patch, one to introduce get_hidx_gslot()
> > one to introduce set_hidx_slot() . Hopefully that will reduce the size
> > of the patch to graspable level. let me know,
>
> I did some experiments with the first two patches.
>
> * First of all the first patch does not compile without this.
its a warning that a variable is defined but not used. I have fixed it
in my new patch series; about to be launched soon.
>
> --- a/arch/powerpc/mm/hash_utils_64.c
> +++ b/arch/powerpc/mm/hash_utils_64.c
> @@ -1612,7 +1612,7 @@ unsigned long get_hidx_gslot(unsigned long vpn, unsigned long shift,
> void flush_hash_page(unsigned long vpn, real_pte_t pte, int psize, int ssize,
> unsigned long flags)
> {
> - unsigned long hash, index, shift, hidx, gslot;
> + unsigned long index, shift, gslot;
> int local = flags & HPTE_LOCAL_UPDATE;
>
> DBG_LOW("flush_hash_page(vpn=%016lx)\n", vpn);
>
> * Though it boots the kernel, system is kind of unresponsive while attempting
> to compile a kernel. Though I did not dig further on this, seems like the
> first patch is not self sufficient yet.
I wouldn't have broken the the patch into two, because there is too much
coupling between the two. But Aneesh wanted it that way. And it makes
sense to break it from a review point of view.
>
> * With both first and second patch, the kernel boots fine and compiles a kernel.
Yes. that meets my expectation.
>
> We need to sort out issues in the first two patches before looking into
> the rest of the patch series.
I am not aware of any issues in the first two patches though. Do you see
any?
RP
--
Ram Pai
[toc] | [prev] | [next] | [standalone]
| From | "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> |
|---|---|
| Date | 2017-06-21 08:50 +0200 |
| Subject | Re: [RFC v2 01/12] powerpc: Free up four 64K PTE bits in 4K backed hpte pages. |
| Message-ID | <tUHCF-2z2-1@gated-at.bofh.it> |
| In reply to | #1668166 |
Ram Pai <linuxram@us.ibm.com> writes:
> 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 patch does the following change to the 64K PTE format
>
> H_PAGE_BUSY moves from bit 3 to bit 9
> H_PAGE_F_SECOND which occupied bit 4 moves to the second part
> of the pte.
> H_PAGE_F_GIX which occupied bit 5, 6 and 7 also moves to the
> second part of the pte.
>
> the four bits((H_PAGE_F_SECOND|H_PAGE_F_GIX) that represent a slot
> is initialized to 0xF indicating an invalid slot. If a hpte
> gets cached in a 0xF slot(i.e 7th slot of secondary), it is
> released immediately. In other words, even though 0xF is a
> valid slot we discard and consider it as an invalid
> slot;i.e hpte_soft_invalid(). This gives us an opportunity to not
> depend on a bit in the primary PTE in order to determine the
> validity of a slot.
>
> When we release a hpte in the 0xF slot we also release a
> legitimate primary slot and unmap that entry. This is to
> ensure that we do get a legimate non-0xF slot the next time we
> retry for a slot.
>
> Though treating 0xF slot as invalid reduces the number of available
> slots and may have an effect on the performance, the probabilty
> of hitting a 0xF is extermely low.
>
> Compared to the current scheme, the above described scheme reduces
> the number of false hash table updates significantly and has the
> added advantage of releasing four valuable PTE bits for other
> purpose.
>
> This idea was jointly developed by Paul Mackerras, Aneesh, Michael
> Ellermen and myself.
>
> 4K PTE format remain unchanged currently.
>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
> arch/powerpc/include/asm/book3s/64/hash-4k.h | 20 +++++++
> arch/powerpc/include/asm/book3s/64/hash-64k.h | 32 +++++++----
> arch/powerpc/include/asm/book3s/64/hash.h | 15 +++--
> arch/powerpc/include/asm/book3s/64/mmu-hash.h | 5 ++
> arch/powerpc/mm/dump_linuxpagetables.c | 3 +-
> arch/powerpc/mm/hash64_4k.c | 14 ++---
> arch/powerpc/mm/hash64_64k.c | 81 ++++++++++++---------------
> arch/powerpc/mm/hash_utils_64.c | 30 +++++++---
> 8 files changed, 122 insertions(+), 78 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/64/hash-4k.h b/arch/powerpc/include/asm/book3s/64/hash-4k.h
> index b4b5e6b..5ef1d81 100644
> --- a/arch/powerpc/include/asm/book3s/64/hash-4k.h
> +++ b/arch/powerpc/include/asm/book3s/64/hash-4k.h
> @@ -16,6 +16,18 @@
> #define H_PUD_TABLE_SIZE (sizeof(pud_t) << H_PUD_INDEX_SIZE)
> #define H_PGD_TABLE_SIZE (sizeof(pgd_t) << H_PGD_INDEX_SIZE)
>
> +
> +/*
> + * Only supported by 4k linux page size
> + */
> +#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_F_GIX_SHIFT 56
> +
> +#define H_PAGE_BUSY _RPAGE_RSV1 /* software: PTE & hash are busy */
> +#define H_PAGE_HASHPTE _RPAGE_RPN43 /* PTE has associated HPTE */
> +
> +
> /* PTE flags to conserve for HPTE identification */
> #define _PAGE_HPTEFLAGS (H_PAGE_BUSY | H_PAGE_HASHPTE | \
> H_PAGE_F_SECOND | H_PAGE_F_GIX)
> @@ -48,6 +60,14 @@ static inline int hash__hugepd_ok(hugepd_t hpd)
> }
> #endif
>
> +static inline unsigned long set_hidx_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)
> diff --git a/arch/powerpc/include/asm/book3s/64/hash-64k.h b/arch/powerpc/include/asm/book3s/64/hash-64k.h
> index 9732837..0eb3c89 100644
> --- a/arch/powerpc/include/asm/book3s/64/hash-64k.h
> +++ b/arch/powerpc/include/asm/book3s/64/hash-64k.h
> @@ -10,23 +10,25 @@
> * 64k aligned address free up few of the lower bits of RPN for us
> * We steal that here. For more deatils look at pte_pfn/pfn_pte()
> */
> -#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_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_F_SECOND _RPAGE_RSV2 /* HPTE is in 2ndary HPTEG */
> +#define H_PAGE_F_GIX (_RPAGE_RSV3 | _RPAGE_RSV4 | _RPAGE_RPN44)
> +#define H_PAGE_F_GIX_SHIFT 56
> +
> +
> +#define H_PAGE_BUSY _RPAGE_RPN42 /* software: PTE & hash are busy */
> +#define H_PAGE_HASHPTE _RPAGE_RPN43 /* PTE has associated HPTE */
> +
> /*
> * We need to differentiate between explicit huge page and THP huge
> * page, since THP huge page also need to track real subpage details
> */
> #define H_PAGE_THP_HUGE H_PAGE_4K_PFN
>
> -/*
> - * Used to track subpage group valid if H_PAGE_COMBO is set
> - * This overloads H_PAGE_F_GIX and H_PAGE_F_SECOND
> - */
> -#define H_PAGE_COMBO_VALID (H_PAGE_F_GIX | H_PAGE_F_SECOND)
> -
> /* 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)
Why in this patch ? This is related to 64K pte
> +
> /*
> * we support 16 fragments per PTE page of 64K size.
> */
> @@ -74,6 +76,16 @@ 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;
> }
>
> +static inline unsigned long set_hidx_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));
> + return 0x0UL;
> +}
> +
> #define __rpte_to_pte(r) ((r).pte)
> extern bool __rpte_sub_valid(real_pte_t rpte, unsigned long index);
> /*
> diff --git a/arch/powerpc/include/asm/book3s/64/hash.h b/arch/powerpc/include/asm/book3s/64/hash.h
> index 4e957b0..e7cf03a 100644
> --- a/arch/powerpc/include/asm/book3s/64/hash.h
> +++ b/arch/powerpc/include/asm/book3s/64/hash.h
> @@ -8,11 +8,8 @@
> *
> */
> #define H_PTE_NONE_MASK _PAGE_HPTEFLAGS
> -#define H_PAGE_F_GIX_SHIFT 56
> -#define H_PAGE_BUSY _RPAGE_RSV1 /* software: PTE & hash are busy */
> -#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 */
> +
> +#define INIT_HIDX (~0x0UL)
>
> #ifdef CONFIG_PPC_64K_PAGES
> #include <asm/book3s/64/hash-64k.h>
> @@ -160,6 +157,14 @@ static inline int hash__pte_none(pte_t pte)
> return (pte_val(pte) & ~H_PTE_NONE_MASK) == 0;
> }
>
> +static inline bool hpte_soft_invalid(unsigned long slot)
> +{
> + return ((slot & 0xfUL) == 0xfUL);
> +}
> +
> +unsigned long get_hidx_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
> diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> index 6981a52..cfb8169 100644
> --- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> +++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> @@ -435,6 +435,11 @@ extern int __hash_page_4K(unsigned long ea, unsigned long access,
> extern int __hash_page_64K(unsigned long ea, unsigned long access,
> unsigned long vsid, pte_t *ptep, unsigned long trap,
> unsigned long flags, int ssize);
> +extern unsigned long set_hidx_slot(pte_t *ptep, real_pte_t rpte,
> + unsigned int subpg_index, unsigned long slot);
> +extern unsigned long get_hidx_slot(unsigned long vpn, unsigned long shift,
> + int ssize, real_pte_t rpte, unsigned int subpg_index);
> +
> struct mm_struct;
> unsigned int hash_page_do_lazy_icache(unsigned int pp, pte_t pte, int trap);
> extern int hash_page_mm(struct mm_struct *mm, unsigned long ea,
> diff --git a/arch/powerpc/mm/dump_linuxpagetables.c b/arch/powerpc/mm/dump_linuxpagetables.c
> index 44fe483..b832ed3 100644
> --- a/arch/powerpc/mm/dump_linuxpagetables.c
> +++ b/arch/powerpc/mm/dump_linuxpagetables.c
> @@ -213,7 +213,7 @@ struct flag_info {
> .val = H_PAGE_4K_PFN,
> .set = "4K_pfn",
> }, {
> -#endif
> +#else
> .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,
> diff --git a/arch/powerpc/mm/hash64_4k.c b/arch/powerpc/mm/hash64_4k.c
> index 6fa450c..c673829 100644
> --- a/arch/powerpc/mm/hash64_4k.c
> +++ b/arch/powerpc/mm/hash64_4k.c
> @@ -20,6 +20,7 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> pte_t *ptep, unsigned long trap, unsigned long flags,
> int ssize, int subpg_prot)
> {
> + real_pte_t rpte;
> unsigned long hpte_group;
> unsigned long rflags, pa;
> unsigned long old_pte, new_pte;
> @@ -54,6 +55,7 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> * need to add in 0x1 if it's a read-only user page
> */
> 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))
> @@ -64,13 +66,10 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> /*
> * 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;
> + unsigned long gslot = get_hidx_gslot(vpn, shift,
> + ssize, rpte, 0);
>
> - if (mmu_hash_ops.hpte_updatepp(slot, rflags, vpn, MMU_PAGE_4K,
> + if (mmu_hash_ops.hpte_updatepp(gslot, rflags, vpn, MMU_PAGE_4K,
> MMU_PAGE_4K, ssize, flags) == -1)
> old_pte &= ~_PAGE_HPTEFLAGS;
> }
> @@ -118,8 +117,7 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> 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 |= set_hidx_slot(ptep, rpte, 0, slot);
> }
> *ptep = __pte(new_pte & ~H_PAGE_BUSY);
> return 0;
None of the above changes are needed. We are not changing anything w.r.t
4k linux page table yet. So we can drop this.
> diff --git a/arch/powerpc/mm/hash64_64k.c b/arch/powerpc/mm/hash64_64k.c
> index 1a68cb1..3702a3c 100644
> --- a/arch/powerpc/mm/hash64_64k.c
> +++ b/arch/powerpc/mm/hash64_64k.c
> @@ -15,34 +15,13 @@
> #include <linux/mm.h>
> #include <asm/machdep.h>
> #include <asm/mmu.h>
> +
> /*
> * index from 0 - 15
> */
> bool __rpte_sub_valid(real_pte_t rpte, unsigned long index)
> {
> - unsigned long g_idx;
> - unsigned long ptev = pte_val(rpte.pte);
> -
> - g_idx = (ptev & H_PAGE_COMBO_VALID) >> H_PAGE_F_GIX_SHIFT;
> - index = index >> 2;
> - if (g_idx & (0x1 << index))
> - return true;
> - else
> - return false;
> -}
> -/*
> - * index from 0 - 15
> - */
> -static unsigned long mark_subptegroup_valid(unsigned long ptev, unsigned long index)
> -{
> - unsigned long g_idx;
> -
> - if (!(ptev & H_PAGE_COMBO))
> - return ptev;
> - index = index >> 2;
> - g_idx = 0x1 << index;
> -
> - return ptev | (g_idx << H_PAGE_F_GIX_SHIFT);
> + return !(hpte_soft_invalid(rpte.hidx >> (index << 2)));
> }
>
> int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> @@ -50,10 +29,9 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> int ssize, int subpg_prot)
> {
> real_pte_t rpte;
> - unsigned long *hidxp;
> unsigned long hpte_group;
> unsigned int subpg_index;
> - unsigned long rflags, pa, hidx;
> + unsigned long rflags, pa;
> unsigned long old_pte, new_pte, subpg_pte;
> unsigned long vpn, hash, slot;
> unsigned long shift = mmu_psize_defs[MMU_PAGE_4K].shift;
> @@ -116,28 +94,23 @@ 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;
> }
> /*
> * Check for sub page valid and update
> */
> if (__rpte_sub_valid(rpte, subpg_index)) {
> - int ret;
>
> - 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;
> + unsigned long gslot = get_hidx_gslot(vpn, shift,
> + ssize, rpte, subpg_index);
Converting that to helper is also not needed in this patch. Leave it as
it is. It is much easier to review.
>
> - ret = mmu_hash_ops.hpte_updatepp(slot, rflags, vpn,
> + int ret = mmu_hash_ops.hpte_updatepp(gslot, 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
> * we try an insertion.
> */
> if (ret == -1)
> @@ -148,6 +121,15 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> }
>
> htab_insert_hpte:
> +
> + /*
> + * initialize all hidx entries to a invalid value,
> + * the first time the PTE is about to allocate
> + * a 4K hpte
> + */
> + if (!(old_pte & H_PAGE_COMBO))
> + rpte.hidx = INIT_HIDX;
> +
> /*
> * handle H_PAGE_4K_PFN case
> */
> @@ -177,10 +159,20 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> rflags, HPTE_V_SECONDARY,
> MMU_PAGE_4K, MMU_PAGE_4K,
> ssize);
> - if (slot == -1) {
> - if (mftb() & 0x1)
> +
> + if (unlikely(hpte_soft_invalid(slot))) {
Should we name that hpte_slot_invalid() ? ie. s/soft/slot/ ?
> + slot = slot & _PTEIDX_GROUP_IX;
> + mmu_hash_ops.hpte_invalidate(hpte_group+slot, vpn,
> + MMU_PAGE_4K, MMU_PAGE_4K,
> + ssize, flags);
What is the last arg flags here ? I guess we need to pass 0 there ?
We can't do a local = 1 invalidate, because we don't know whether
anybody did really access this address in between and has got the entry
in TLB.
> + }
> +
> + if (unlikely(slot == -1 || hpte_soft_invalid(slot))) {
> +
Can you add a comment around explaining invalid slot always result in
removing from primary ? Also do we want to store that invalid slot
details in a variable ? instead of doing that conditional again and
again ? This is hotpath.
> + if (hpte_soft_invalid(slot) || (mftb() & 0x1))
> hpte_group = ((hash & htab_hash_mask) *
> HPTES_PER_GROUP) & ~0x7UL;
> +
> mmu_hash_ops.hpte_remove(hpte_group);
> /*
> * FIXME!! Should be try the group from which we removed ?
> @@ -204,11 +196,9 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> * Since we have H_PAGE_BUSY set on ptep, we can be sure
> * nobody is undating hidx.
> */
> - hidxp = (unsigned long *)(ptep + PTRS_PER_PTE);
> - rpte.hidx &= ~(0xfUL << (subpg_index << 2));
> - *hidxp = rpte.hidx | (slot << (subpg_index << 2));
> - new_pte = mark_subptegroup_valid(new_pte, subpg_index);
> - new_pte |= H_PAGE_HASHPTE;
> + new_pte |= set_hidx_slot(ptep, rpte, subpg_index, slot);
> + new_pte |= H_PAGE_HASHPTE;
> +
> /*
> * check __real_pte for details on matching smp_rmb()
> */
> @@ -322,9 +312,10 @@ 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);
> + (H_PAGE_F_SECOND | H_PAGE_F_GIX);
> + new_pte = (new_pte & ~_PAGE_HPTEFLAGS) | H_PAGE_HASHPTE;
What is this change ? i guess we want this in second patch ?
> }
> *ptep = __pte(new_pte & ~H_PAGE_BUSY);
> return 0;
> diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
> index f2095ce..c0f4b46 100644
> --- a/arch/powerpc/mm/hash_utils_64.c
> +++ b/arch/powerpc/mm/hash_utils_64.c
> @@ -975,8 +975,9 @@ void __init hash__early_init_devtree(void)
>
> void __init hash__early_init_mmu(void)
> {
> +#ifndef CONFIG_PPC_64K_PAGES
> /*
> - * We have code in __hash_page_64K() and elsewhere, which assumes it can
> + * We have code in __hash_page_4K() and elsewhere, which assumes it can
> * do the following:
> * new_pte |= (slot << H_PAGE_F_GIX_SHIFT) & (H_PAGE_F_SECOND | H_PAGE_F_GIX);
> *
> @@ -987,6 +988,7 @@ void __init hash__early_init_mmu(void)
> * with a BUILD_BUG_ON().
> */
> BUILD_BUG_ON(H_PAGE_F_SECOND != (1ul << (H_PAGE_F_GIX_SHIFT + 3)));
> +#endif /* CONFIG_PPC_64K_PAGES */
>
> htab_init_page_sizes();
>
> @@ -1589,29 +1591,39 @@ static inline void tm_flush_hash_page(int local)
> }
> #endif
>
> +unsigned long get_hidx_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;
> +}
We don't need this helper for this patch series ?
> +
> +
> /* WARNING: This is called from hash_low_64.S, if you change this prototype,
> * do not forget to update the assembly call site !
> */
> void flush_hash_page(unsigned long vpn, real_pte_t pte, int psize, int ssize,
> unsigned long flags)
> {
> - unsigned long hash, index, shift, hidx, slot;
> + unsigned long hash, index, shift, hidx, gslot;
> int local = flags & HPTE_LOCAL_UPDATE;
>
> DBG_LOW("flush_hash_page(vpn=%016lx)\n", vpn);
> pte_iterate_hashed_subpages(pte, psize, vpn, index, shift) {
> - hash = hpt_hash(vpn, shift, ssize);
> - hidx = __rpte_to_hidx(pte, index);
> - if (hidx & _PTEIDX_SECONDARY)
> - hash = ~hash;
> - slot = (hash & htab_hash_mask) * HPTES_PER_GROUP;
> - slot += hidx & _PTEIDX_GROUP_IX;
> + gslot = get_hidx_gslot(vpn, shift, ssize, pte, index);
> DBG_LOW(" sub %ld: hash=%lx, hidx=%lx\n", index, slot, hidx);
> /*
> * We use same base page size and actual psize, because we don't
> * use these functions for hugepage
> */
> - mmu_hash_ops.hpte_invalidate(slot, vpn, psize, psize,
> + mmu_hash_ops.hpte_invalidate(gslot, vpn, psize, psize,
> ssize, local);
> } pte_iterate_hashed_end();
>
And if we avoid adding that helper, changes like this can be avoided in
the patch.
-aneesh
[toc] | [prev] | [next] | [standalone]
| From | Ram Pai <linuxram@us.ibm.com> |
|---|---|
| Date | 2017-06-21 11:40 +0200 |
| Subject | Re: [RFC v2 01/12] powerpc: Free up four 64K PTE bits in 4K backed hpte pages. |
| Message-ID | <tUKhd-4ly-35@gated-at.bofh.it> |
| In reply to | #1671401 |
On Wed, Jun 21, 2017 at 12:11:32PM +0530, Aneesh Kumar K.V wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
>
> > 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 patch does the following change to the 64K PTE format
> >
> > H_PAGE_BUSY moves from bit 3 to bit 9
> > H_PAGE_F_SECOND which occupied bit 4 moves to the second part
> > of the pte.
> > H_PAGE_F_GIX which occupied bit 5, 6 and 7 also moves to the
> > second part of the pte.
> >
> > the four bits((H_PAGE_F_SECOND|H_PAGE_F_GIX) that represent a slot
> > is initialized to 0xF indicating an invalid slot. If a hpte
> > gets cached in a 0xF slot(i.e 7th slot of secondary), it is
> > released immediately. In other words, even though 0xF is a
> > valid slot we discard and consider it as an invalid
> > slot;i.e hpte_soft_invalid(). This gives us an opportunity to not
> > depend on a bit in the primary PTE in order to determine the
> > validity of a slot.
> >
> > When we release a hpte in the 0xF slot we also release a
> > legitimate primary slot and unmap that entry. This is to
> > ensure that we do get a legimate non-0xF slot the next time we
> > retry for a slot.
> >
> > Though treating 0xF slot as invalid reduces the number of available
> > slots and may have an effect on the performance, the probabilty
> > of hitting a 0xF is extermely low.
> >
> > Compared to the current scheme, the above described scheme reduces
> > the number of false hash table updates significantly and has the
> > added advantage of releasing four valuable PTE bits for other
> > purpose.
> >
> > This idea was jointly developed by Paul Mackerras, Aneesh, Michael
> > Ellermen and myself.
> >
> > 4K PTE format remain unchanged currently.
> >
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > ---
> > arch/powerpc/include/asm/book3s/64/hash-4k.h | 20 +++++++
> > arch/powerpc/include/asm/book3s/64/hash-64k.h | 32 +++++++----
> > arch/powerpc/include/asm/book3s/64/hash.h | 15 +++--
> > arch/powerpc/include/asm/book3s/64/mmu-hash.h | 5 ++
> > arch/powerpc/mm/dump_linuxpagetables.c | 3 +-
> > arch/powerpc/mm/hash64_4k.c | 14 ++---
> > arch/powerpc/mm/hash64_64k.c | 81 ++++++++++++---------------
> > arch/powerpc/mm/hash_utils_64.c | 30 +++++++---
> > 8 files changed, 122 insertions(+), 78 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/book3s/64/hash-4k.h b/arch/powerpc/include/asm/book3s/64/hash-4k.h
> > index b4b5e6b..5ef1d81 100644
> > --- a/arch/powerpc/include/asm/book3s/64/hash-4k.h
> > +++ b/arch/powerpc/include/asm/book3s/64/hash-4k.h
> > @@ -16,6 +16,18 @@
> > #define H_PUD_TABLE_SIZE (sizeof(pud_t) << H_PUD_INDEX_SIZE)
> > #define H_PGD_TABLE_SIZE (sizeof(pgd_t) << H_PGD_INDEX_SIZE)
> >
> > +
> > +/*
> > + * Only supported by 4k linux page size
> > + */
> > +#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_F_GIX_SHIFT 56
> > +
> > +#define H_PAGE_BUSY _RPAGE_RSV1 /* software: PTE & hash are busy */
> > +#define H_PAGE_HASHPTE _RPAGE_RPN43 /* PTE has associated HPTE */
> > +
> > +
> > /* PTE flags to conserve for HPTE identification */
> > #define _PAGE_HPTEFLAGS (H_PAGE_BUSY | H_PAGE_HASHPTE | \
> > H_PAGE_F_SECOND | H_PAGE_F_GIX)
> > @@ -48,6 +60,14 @@ static inline int hash__hugepd_ok(hugepd_t hpd)
> > }
> > #endif
> >
> > +static inline unsigned long set_hidx_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)
> > diff --git a/arch/powerpc/include/asm/book3s/64/hash-64k.h b/arch/powerpc/include/asm/book3s/64/hash-64k.h
> > index 9732837..0eb3c89 100644
> > --- a/arch/powerpc/include/asm/book3s/64/hash-64k.h
> > +++ b/arch/powerpc/include/asm/book3s/64/hash-64k.h
> > @@ -10,23 +10,25 @@
> > * 64k aligned address free up few of the lower bits of RPN for us
> > * We steal that here. For more deatils look at pte_pfn/pfn_pte()
> > */
> > -#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_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_F_SECOND _RPAGE_RSV2 /* HPTE is in 2ndary HPTEG */
> > +#define H_PAGE_F_GIX (_RPAGE_RSV3 | _RPAGE_RSV4 | _RPAGE_RPN44)
> > +#define H_PAGE_F_GIX_SHIFT 56
> > +
> > +
> > +#define H_PAGE_BUSY _RPAGE_RPN42 /* software: PTE & hash are busy */
> > +#define H_PAGE_HASHPTE _RPAGE_RPN43 /* PTE has associated HPTE */
> > +
> > /*
> > * We need to differentiate between explicit huge page and THP huge
> > * page, since THP huge page also need to track real subpage details
> > */
> > #define H_PAGE_THP_HUGE H_PAGE_4K_PFN
> >
> > -/*
> > - * Used to track subpage group valid if H_PAGE_COMBO is set
> > - * This overloads H_PAGE_F_GIX and H_PAGE_F_SECOND
> > - */
> > -#define H_PAGE_COMBO_VALID (H_PAGE_F_GIX | H_PAGE_F_SECOND)
> > -
> > /* 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)
>
> Why in this patch ? This is related to 64K pte
>
Yes its in the wrong patch. Have fixed it in my new series.
>
> > +
> > /*
> > * we support 16 fragments per PTE page of 64K size.
> > */
> > @@ -74,6 +76,16 @@ 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;
> > }
> >
> > +static inline unsigned long set_hidx_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));
> > + return 0x0UL;
> > +}
> > +
> > #define __rpte_to_pte(r) ((r).pte)
> > extern bool __rpte_sub_valid(real_pte_t rpte, unsigned long index);
> > /*
> > diff --git a/arch/powerpc/include/asm/book3s/64/hash.h b/arch/powerpc/include/asm/book3s/64/hash.h
> > index 4e957b0..e7cf03a 100644
> > --- a/arch/powerpc/include/asm/book3s/64/hash.h
> > +++ b/arch/powerpc/include/asm/book3s/64/hash.h
> > @@ -8,11 +8,8 @@
> > *
> > */
> > #define H_PTE_NONE_MASK _PAGE_HPTEFLAGS
> > -#define H_PAGE_F_GIX_SHIFT 56
> > -#define H_PAGE_BUSY _RPAGE_RSV1 /* software: PTE & hash are busy */
> > -#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 */
> > +
> > +#define INIT_HIDX (~0x0UL)
> >
> > #ifdef CONFIG_PPC_64K_PAGES
> > #include <asm/book3s/64/hash-64k.h>
> > @@ -160,6 +157,14 @@ static inline int hash__pte_none(pte_t pte)
> > return (pte_val(pte) & ~H_PTE_NONE_MASK) == 0;
> > }
> >
> > +static inline bool hpte_soft_invalid(unsigned long slot)
> > +{
> > + return ((slot & 0xfUL) == 0xfUL);
> > +}
> > +
> > +unsigned long get_hidx_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
> > diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> > index 6981a52..cfb8169 100644
> > --- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> > +++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> > @@ -435,6 +435,11 @@ extern int __hash_page_4K(unsigned long ea, unsigned long access,
> > extern int __hash_page_64K(unsigned long ea, unsigned long access,
> > unsigned long vsid, pte_t *ptep, unsigned long trap,
> > unsigned long flags, int ssize);
> > +extern unsigned long set_hidx_slot(pte_t *ptep, real_pte_t rpte,
> > + unsigned int subpg_index, unsigned long slot);
> > +extern unsigned long get_hidx_slot(unsigned long vpn, unsigned long shift,
> > + int ssize, real_pte_t rpte, unsigned int subpg_index);
> > +
> > struct mm_struct;
> > unsigned int hash_page_do_lazy_icache(unsigned int pp, pte_t pte, int trap);
> > extern int hash_page_mm(struct mm_struct *mm, unsigned long ea,
> > diff --git a/arch/powerpc/mm/dump_linuxpagetables.c b/arch/powerpc/mm/dump_linuxpagetables.c
> > index 44fe483..b832ed3 100644
> > --- a/arch/powerpc/mm/dump_linuxpagetables.c
> > +++ b/arch/powerpc/mm/dump_linuxpagetables.c
> > @@ -213,7 +213,7 @@ struct flag_info {
> > .val = H_PAGE_4K_PFN,
> > .set = "4K_pfn",
> > }, {
> > -#endif
> > +#else
> > .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,
> > diff --git a/arch/powerpc/mm/hash64_4k.c b/arch/powerpc/mm/hash64_4k.c
> > index 6fa450c..c673829 100644
> > --- a/arch/powerpc/mm/hash64_4k.c
> > +++ b/arch/powerpc/mm/hash64_4k.c
> > @@ -20,6 +20,7 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> > pte_t *ptep, unsigned long trap, unsigned long flags,
> > int ssize, int subpg_prot)
> > {
> > + real_pte_t rpte;
> > unsigned long hpte_group;
> > unsigned long rflags, pa;
> > unsigned long old_pte, new_pte;
> > @@ -54,6 +55,7 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> > * need to add in 0x1 if it's a read-only user page
> > */
> > 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))
> > @@ -64,13 +66,10 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> > /*
> > * 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;
> > + unsigned long gslot = get_hidx_gslot(vpn, shift,
> > + ssize, rpte, 0);
> >
> > - if (mmu_hash_ops.hpte_updatepp(slot, rflags, vpn, MMU_PAGE_4K,
> > + if (mmu_hash_ops.hpte_updatepp(gslot, rflags, vpn, MMU_PAGE_4K,
> > MMU_PAGE_4K, ssize, flags) == -1)
> > old_pte &= ~_PAGE_HPTEFLAGS;
> > }
> > @@ -118,8 +117,7 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> > 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 |= set_hidx_slot(ptep, rpte, 0, slot);
> > }
> > *ptep = __pte(new_pte & ~H_PAGE_BUSY);
> > return 0;
>
>
>
> None of the above changes are needed. We are not changing anything w.r.t
> 4k linux page table yet. So we can drop this.
>
I have put these changes in a separate patch. If needed it can be pulled
in. But its not mandatory. Would be nice to have though, since it
reduces a bunch of lines.
>
> > diff --git a/arch/powerpc/mm/hash64_64k.c b/arch/powerpc/mm/hash64_64k.c
> > index 1a68cb1..3702a3c 100644
> > --- a/arch/powerpc/mm/hash64_64k.c
> > +++ b/arch/powerpc/mm/hash64_64k.c
> > @@ -15,34 +15,13 @@
> > #include <linux/mm.h>
> > #include <asm/machdep.h>
> > #include <asm/mmu.h>
> > +
> > /*
> > * index from 0 - 15
> > */
> > bool __rpte_sub_valid(real_pte_t rpte, unsigned long index)
> > {
> > - unsigned long g_idx;
> > - unsigned long ptev = pte_val(rpte.pte);
> > -
> > - g_idx = (ptev & H_PAGE_COMBO_VALID) >> H_PAGE_F_GIX_SHIFT;
> > - index = index >> 2;
> > - if (g_idx & (0x1 << index))
> > - return true;
> > - else
> > - return false;
> > -}
> > -/*
> > - * index from 0 - 15
> > - */
> > -static unsigned long mark_subptegroup_valid(unsigned long ptev, unsigned long index)
> > -{
> > - unsigned long g_idx;
> > -
> > - if (!(ptev & H_PAGE_COMBO))
> > - return ptev;
> > - index = index >> 2;
> > - g_idx = 0x1 << index;
> > -
> > - return ptev | (g_idx << H_PAGE_F_GIX_SHIFT);
> > + return !(hpte_soft_invalid(rpte.hidx >> (index << 2)));
> > }
> >
> > int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> > @@ -50,10 +29,9 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> > int ssize, int subpg_prot)
> > {
> > real_pte_t rpte;
> > - unsigned long *hidxp;
> > unsigned long hpte_group;
> > unsigned int subpg_index;
> > - unsigned long rflags, pa, hidx;
> > + unsigned long rflags, pa;
> > unsigned long old_pte, new_pte, subpg_pte;
> > unsigned long vpn, hash, slot;
> > unsigned long shift = mmu_psize_defs[MMU_PAGE_4K].shift;
> > @@ -116,28 +94,23 @@ 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;
> > }
> > /*
> > * Check for sub page valid and update
> > */
> > if (__rpte_sub_valid(rpte, subpg_index)) {
> > - int ret;
> >
> > - 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;
> > + unsigned long gslot = get_hidx_gslot(vpn, shift,
> > + ssize, rpte, subpg_index);
>
>
> Converting that to helper is also not needed in this patch. Leave it as
> it is. It is much easier to review.
>
ok. But dont want to reduce a bunch of lines?
>
> >
> > - ret = mmu_hash_ops.hpte_updatepp(slot, rflags, vpn,
> > + int ret = mmu_hash_ops.hpte_updatepp(gslot, 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
> > * we try an insertion.
> > */
> > if (ret == -1)
> > @@ -148,6 +121,15 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> > }
> >
> > htab_insert_hpte:
> > +
> > + /*
> > + * initialize all hidx entries to a invalid value,
> > + * the first time the PTE is about to allocate
> > + * a 4K hpte
> > + */
> > + if (!(old_pte & H_PAGE_COMBO))
> > + rpte.hidx = INIT_HIDX;
> > +
> > /*
> > * handle H_PAGE_4K_PFN case
> > */
> > @@ -177,10 +159,20 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> > rflags, HPTE_V_SECONDARY,
> > MMU_PAGE_4K, MMU_PAGE_4K,
> > ssize);
> > - if (slot == -1) {
> > - if (mftb() & 0x1)
> > +
> > + if (unlikely(hpte_soft_invalid(slot))) {
>
> Should we name that hpte_slot_invalid() ? ie. s/soft/slot/ ?
I intentionally used the word soft, since for the hardware it is a
valid slot. The *soft*ware is considering it invalid. Hence the word
*soft*.
>
>
> > + slot = slot & _PTEIDX_GROUP_IX;
> > + mmu_hash_ops.hpte_invalidate(hpte_group+slot, vpn,
> > + MMU_PAGE_4K, MMU_PAGE_4K,
> > + ssize, flags);
>
> What is the last arg flags here ? I guess we need to pass 0 there ?
> We can't do a local = 1 invalidate, because we don't know whether
> anybody did really access this address in between and has got the entry
> in TLB.
ok. I think you are right. it should be 0.
>
>
> > + }
> > +
> > + if (unlikely(slot == -1 || hpte_soft_invalid(slot))) {
> > +
>
> Can you add a comment around explaining invalid slot always result in
> removing from primary ? Also do we want to store that invalid slot
> details in a variable ? instead of doing that conditional again and
> again ? This is hotpath.
>
will do.
> > + if (hpte_soft_invalid(slot) || (mftb() & 0x1))
> > hpte_group = ((hash & htab_hash_mask) *
> > HPTES_PER_GROUP) & ~0x7UL;
> > +
> > mmu_hash_ops.hpte_remove(hpte_group);
> > /*
> > * FIXME!! Should be try the group from which we removed ?
> > @@ -204,11 +196,9 @@ int __hash_page_4K(unsigned long ea, unsigned long access, unsigned long vsid,
> > * Since we have H_PAGE_BUSY set on ptep, we can be sure
> > * nobody is undating hidx.
> > */
> > - hidxp = (unsigned long *)(ptep + PTRS_PER_PTE);
> > - rpte.hidx &= ~(0xfUL << (subpg_index << 2));
> > - *hidxp = rpte.hidx | (slot << (subpg_index << 2));
> > - new_pte = mark_subptegroup_valid(new_pte, subpg_index);
> > - new_pte |= H_PAGE_HASHPTE;
> > + new_pte |= set_hidx_slot(ptep, rpte, subpg_index, slot);
> > + new_pte |= H_PAGE_HASHPTE;
> > +
> > /*
> > * check __real_pte for details on matching smp_rmb()
> > */
> > @@ -322,9 +312,10 @@ 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);
> > + (H_PAGE_F_SECOND | H_PAGE_F_GIX);
> > + new_pte = (new_pte & ~_PAGE_HPTEFLAGS) | H_PAGE_HASHPTE;
>
> What is this change ? i guess we want this in second patch ?
yes. have moved it to the second patch.
>
>
> > }
> > *ptep = __pte(new_pte & ~H_PAGE_BUSY);
> > return 0;
> > diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
> > index f2095ce..c0f4b46 100644
> > --- a/arch/powerpc/mm/hash_utils_64.c
> > +++ b/arch/powerpc/mm/hash_utils_64.c
> > @@ -975,8 +975,9 @@ void __init hash__early_init_devtree(void)
> >
> > void __init hash__early_init_mmu(void)
> > {
> > +#ifndef CONFIG_PPC_64K_PAGES
> > /*
> > - * We have code in __hash_page_64K() and elsewhere, which assumes it can
> > + * We have code in __hash_page_4K() and elsewhere, which assumes it can
> > * do the following:
> > * new_pte |= (slot << H_PAGE_F_GIX_SHIFT) & (H_PAGE_F_SECOND | H_PAGE_F_GIX);
> > *
> > @@ -987,6 +988,7 @@ void __init hash__early_init_mmu(void)
> > * with a BUILD_BUG_ON().
> > */
> > BUILD_BUG_ON(H_PAGE_F_SECOND != (1ul << (H_PAGE_F_GIX_SHIFT + 3)));
> > +#endif /* CONFIG_PPC_64K_PAGES */
> >
> > htab_init_page_sizes();
> >
> > @@ -1589,29 +1591,39 @@ static inline void tm_flush_hash_page(int local)
> > }
> > #endif
> >
> > +unsigned long get_hidx_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;
> > +}
>
>
> We don't need this helper for this patch series ?
>
the helpers will now be moved into independent patches. Can be applied
if needed.
> > +
> > +
> > /* WARNING: This is called from hash_low_64.S, if you change this prototype,
> > * do not forget to update the assembly call site !
> > */
> > void flush_hash_page(unsigned long vpn, real_pte_t pte, int psize, int ssize,
> > unsigned long flags)
> > {
> > - unsigned long hash, index, shift, hidx, slot;
> > + unsigned long hash, index, shift, hidx, gslot;
> > int local = flags & HPTE_LOCAL_UPDATE;
> >
> > DBG_LOW("flush_hash_page(vpn=%016lx)\n", vpn);
> > pte_iterate_hashed_subpages(pte, psize, vpn, index, shift) {
> > - hash = hpt_hash(vpn, shift, ssize);
> > - hidx = __rpte_to_hidx(pte, index);
> > - if (hidx & _PTEIDX_SECONDARY)
> > - hash = ~hash;
> > - slot = (hash & htab_hash_mask) * HPTES_PER_GROUP;
> > - slot += hidx & _PTEIDX_GROUP_IX;
> > + gslot = get_hidx_gslot(vpn, shift, ssize, pte, index);
> > DBG_LOW(" sub %ld: hash=%lx, hidx=%lx\n", index, slot, hidx);
> > /*
> > * We use same base page size and actual psize, because we don't
> > * use these functions for hugepage
> > */
> > - mmu_hash_ops.hpte_invalidate(slot, vpn, psize, psize,
> > + mmu_hash_ops.hpte_invalidate(gslot, vpn, psize, psize,
> > ssize, local);
> > } pte_iterate_hashed_end();
> >
> And if we avoid adding that helper, changes like this can be avoided in
> the patch.
>
>
> -aneesh
[toc] | [prev] | [next] | [standalone]
| From | Anshuman Khandual <khandual@linux.vnet.ibm.com> |
|---|---|
| Date | 2017-06-22 11:10 +0200 |
| Subject | Re: [RFC v2 01/12] powerpc: Free up four 64K PTE bits in 4K backed hpte pages. |
| Message-ID | <tV6hI-2Cj-23@gated-at.bofh.it> |
| In reply to | #1668166 |
On 06/17/2017 09:22 AM, Ram Pai wrote: > 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 patch does the following change to the 64K PTE format > > H_PAGE_BUSY moves from bit 3 to bit 9 > H_PAGE_F_SECOND which occupied bit 4 moves to the second part > of the pte. > H_PAGE_F_GIX which occupied bit 5, 6 and 7 also moves to the > second part of the pte. > > the four bits((H_PAGE_F_SECOND|H_PAGE_F_GIX) that represent a slot > is initialized to 0xF indicating an invalid slot. If a hpte > gets cached in a 0xF slot(i.e 7th slot of secondary), it is > released immediately. In other words, even though 0xF is a > valid slot we discard and consider it as an invalid > slot;i.e hpte_soft_invalid(). This gives us an opportunity to not > depend on a bit in the primary PTE in order to determine the > validity of a slot. > > When we release a hpte in the 0xF slot we also release a > legitimate primary slot and unmap that entry. This is to > ensure that we do get a legimate non-0xF slot the next time we > retry for a slot. > > Though treating 0xF slot as invalid reduces the number of available > slots and may have an effect on the performance, the probabilty > of hitting a 0xF is extermely low. > > Compared to the current scheme, the above described scheme reduces > the number of false hash table updates significantly and has the > added advantage of releasing four valuable PTE bits for other > purpose. > > This idea was jointly developed by Paul Mackerras, Aneesh, Michael > Ellermen and myself. > > 4K PTE format remain unchanged currently. Scanned through the PTE format again for hash 64K and 4K. It seems to me that there might be 5 free bits already present on the PTE format. I might have seriously mistaken something here :) Please correct me if that is not the case. _RPAGE_RPN* I think is applicable only for hash page table format and will not be available for radix later. +#define _PAGE_FREE_1 0x0000000000000040UL /* Not used */ +#define _RPAGE_SW0 0x2000000000000000UL /* Not used */ +#define _RPAGE_SW1 0x0000000000000800UL /* Not used */ +#define _RPAGE_RPN42 0x0040000000000000UL /* Not used */ +#define _RPAGE_RPN41 0x0020000000000000UL /* Not used */
[toc] | [prev] | [next] | [standalone]
| From | Ram Pai <linuxram@us.ibm.com> |
|---|---|
| Date | 2017-06-22 18:30 +0200 |
| Subject | Re: [RFC v2 01/12] powerpc: Free up four 64K PTE bits in 4K backed hpte pages. |
| Message-ID | <tVd9w-7bb-21@gated-at.bofh.it> |
| In reply to | #1672447 |
On Thu, Jun 22, 2017 at 02:37:27PM +0530, Anshuman Khandual wrote: > On 06/17/2017 09:22 AM, Ram Pai wrote: > > 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 patch does the following change to the 64K PTE format > > > > H_PAGE_BUSY moves from bit 3 to bit 9 > > H_PAGE_F_SECOND which occupied bit 4 moves to the second part > > of the pte. > > H_PAGE_F_GIX which occupied bit 5, 6 and 7 also moves to the > > second part of the pte. > > > > the four bits((H_PAGE_F_SECOND|H_PAGE_F_GIX) that represent a slot > > is initialized to 0xF indicating an invalid slot. If a hpte > > gets cached in a 0xF slot(i.e 7th slot of secondary), it is > > released immediately. In other words, even though 0xF is a > > valid slot we discard and consider it as an invalid > > slot;i.e hpte_soft_invalid(). This gives us an opportunity to not > > depend on a bit in the primary PTE in order to determine the > > validity of a slot. > > > > When we release a hpte in the 0xF slot we also release a > > legitimate primary slot and unmap that entry. This is to > > ensure that we do get a legimate non-0xF slot the next time we > > retry for a slot. > > > > Though treating 0xF slot as invalid reduces the number of available > > slots and may have an effect on the performance, the probabilty > > of hitting a 0xF is extermely low. > > > > Compared to the current scheme, the above described scheme reduces > > the number of false hash table updates significantly and has the > > added advantage of releasing four valuable PTE bits for other > > purpose. > > > > This idea was jointly developed by Paul Mackerras, Aneesh, Michael > > Ellermen and myself. > > > > 4K PTE format remain unchanged currently. > > Scanned through the PTE format again for hash 64K and 4K. It seems > to me that there might be 5 free bits already present on the PTE > format. I might have seriously mistaken something here :) Please > correct me if that is not the case. _RPAGE_RPN* I think is applicable > only for hash page table format and will not be available for radix > later. > > +#define _PAGE_FREE_1 0x0000000000000040UL /* Not used */ > +#define _RPAGE_SW0 0x2000000000000000UL /* Not used */ > +#define _RPAGE_SW1 0x0000000000000800UL /* Not used */ > +#define _RPAGE_RPN42 0x0040000000000000UL /* Not used */ > +#define _RPAGE_RPN41 0x0020000000000000UL /* Not used */ > The bits are chosen to future proof for radix implementation. _RPAGE_SW* will eat into what is available for software in the future, and these key-bits will certainly be something that the radix hardware will read, in the future. The _RPAGE_RPN* bits cannot be relied on for radix. But finally the bits that we chose (H_PAGE_F_SECOND|H_PAGE_F_GIX) had the best potential for giving us the highest number of free bits with relatively less effort. RP
[toc] | [prev] | [next] | [standalone]
| From | Ram Pai <linuxram@us.ibm.com> |
|---|---|
| Date | 2017-06-17 06:00 +0200 |
| Subject | [RFC v2 10/12] powerpc: Read AMR only if pkey-violation caused the exception. |
| Message-ID | <tTd3Y-1kp-17@gated-at.bofh.it> |
| In reply to | #1668160 |
Signed-off-by: Ram Pai <linuxram@us.ibm.com> --- arch/powerpc/kernel/exceptions-64s.S | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S index 8db9ef8..a4de1b4 100644 --- a/arch/powerpc/kernel/exceptions-64s.S +++ b/arch/powerpc/kernel/exceptions-64s.S @@ -493,13 +493,15 @@ EXC_COMMON_BEGIN(data_access_common) ld r12,_MSR(r1) ld r3,PACA_EXGEN+EX_DAR(r13) lwz r4,PACA_EXGEN+EX_DSISR(r13) + std r3,_DAR(r1) + std r4,_DSISR(r1) #ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS + andis. r0,r4,DSISR_KEYFAULT@h /* save AMR only if its a key fault */ + beq+ 1f mfspr r5,SPRN_AMR std r5,PACA_AMR(r13) #endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */ - li r5,0x300 - std r3,_DAR(r1) - std r4,_DSISR(r1) +1: li r5,0x300 BEGIN_MMU_FTR_SECTION b do_hash_page /* Try to handle as hpte fault */ MMU_FTR_SECTION_ELSE @@ -565,13 +567,15 @@ EXC_COMMON_BEGIN(instruction_access_common) ld r12,_MSR(r1) ld r3,_NIP(r1) andis. r4,r12,0x5820 + std r3,_DAR(r1) + std r4,_DSISR(r1) #ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS + andis. r0,r4,DSISR_KEYFAULT@h /* save AMR only if its a key fault */ + beq+ 1f mfspr r5,SPRN_AMR std r5,PACA_AMR(r13) #endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */ - li r5,0x400 - std r3,_DAR(r1) - std r4,_DSISR(r1) +1: li r5,0x400 BEGIN_MMU_FTR_SECTION b do_hash_page /* Try to handle as hpte fault */ MMU_FTR_SECTION_ELSE -- 1.8.3.1
[toc] | [prev] | [next] | [standalone]
| From | Michael Ellerman <mpe@ellerman.id.au> |
|---|---|
| Date | 2017-06-19 13:10 +0200 |
| Subject | Re: [RFC v2 10/12] powerpc: Read AMR only if pkey-violation caused the exception. |
| Message-ID | <tU2Jc-1RM-21@gated-at.bofh.it> |
| In reply to | #1668167 |
Ram Pai <linuxram@us.ibm.com> writes: > Signed-off-by: Ram Pai <linuxram@us.ibm.com> > --- > arch/powerpc/kernel/exceptions-64s.S | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S > index 8db9ef8..a4de1b4 100644 > --- a/arch/powerpc/kernel/exceptions-64s.S > +++ b/arch/powerpc/kernel/exceptions-64s.S > @@ -493,13 +493,15 @@ EXC_COMMON_BEGIN(data_access_common) > ld r12,_MSR(r1) > ld r3,PACA_EXGEN+EX_DAR(r13) > lwz r4,PACA_EXGEN+EX_DSISR(r13) > + std r3,_DAR(r1) > + std r4,_DSISR(r1) > #ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS > + andis. r0,r4,DSISR_KEYFAULT@h /* save AMR only if its a key fault */ > + beq+ 1f This seems to be incremental on top of one of your other patches. But I don't see why, can you please just squash this into whatever patch adds this code in the first place. cheers
[toc] | [prev] | [next] | [standalone]
| From | Ram Pai <linuxram@us.ibm.com> |
|---|---|
| Date | 2017-06-19 20:00 +0200 |
| Subject | Re: [RFC v2 10/12] powerpc: Read AMR only if pkey-violation caused the exception. |
| Message-ID | <tU97Y-5OJ-9@gated-at.bofh.it> |
| In reply to | #1669010 |
On Mon, Jun 19, 2017 at 09:06:13PM +1000, Michael Ellerman wrote: > Ram Pai <linuxram@us.ibm.com> writes: > > > Signed-off-by: Ram Pai <linuxram@us.ibm.com> > > --- > > arch/powerpc/kernel/exceptions-64s.S | 16 ++++++++++------ > > 1 file changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S > > index 8db9ef8..a4de1b4 100644 > > --- a/arch/powerpc/kernel/exceptions-64s.S > > +++ b/arch/powerpc/kernel/exceptions-64s.S > > @@ -493,13 +493,15 @@ EXC_COMMON_BEGIN(data_access_common) > > ld r12,_MSR(r1) > > ld r3,PACA_EXGEN+EX_DAR(r13) > > lwz r4,PACA_EXGEN+EX_DSISR(r13) > > + std r3,_DAR(r1) > > + std r4,_DSISR(r1) > > #ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS > > + andis. r0,r4,DSISR_KEYFAULT@h /* save AMR only if its a key fault */ > > + beq+ 1f > > This seems to be incremental on top of one of your other patches. > > But I don't see why, can you please just squash this into whatever patch > adds this code in the first place. It was an optimization added later. But yes it can be squashed into an earlier patch. RP
[toc] | [prev] | [next] | [standalone]
| From | Anshuman Khandual <khandual@linux.vnet.ibm.com> |
|---|---|
| Date | 2017-06-20 08:50 +0200 |
| Subject | Re: [RFC v2 10/12] powerpc: Read AMR only if pkey-violation caused the exception. |
| Message-ID | <tUl98-55H-1@gated-at.bofh.it> |
| In reply to | #1669556 |
On 06/19/2017 11:29 PM, Ram Pai wrote: > On Mon, Jun 19, 2017 at 09:06:13PM +1000, Michael Ellerman wrote: >> Ram Pai <linuxram@us.ibm.com> writes: >> >>> Signed-off-by: Ram Pai <linuxram@us.ibm.com> >>> --- >>> arch/powerpc/kernel/exceptions-64s.S | 16 ++++++++++------ >>> 1 file changed, 10 insertions(+), 6 deletions(-) >>> >>> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S >>> index 8db9ef8..a4de1b4 100644 >>> --- a/arch/powerpc/kernel/exceptions-64s.S >>> +++ b/arch/powerpc/kernel/exceptions-64s.S >>> @@ -493,13 +493,15 @@ EXC_COMMON_BEGIN(data_access_common) >>> ld r12,_MSR(r1) >>> ld r3,PACA_EXGEN+EX_DAR(r13) >>> lwz r4,PACA_EXGEN+EX_DSISR(r13) >>> + std r3,_DAR(r1) >>> + std r4,_DSISR(r1) >>> #ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS >>> + andis. r0,r4,DSISR_KEYFAULT@h /* save AMR only if its a key fault */ >>> + beq+ 1f >> >> This seems to be incremental on top of one of your other patches. >> >> But I don't see why, can you please just squash this into whatever patch >> adds this code in the first place. > > It was an optimization added later. But yes it can be squashed into an > earlier patch. Could you please explain what is the optimization this achieves ?
[toc] | [prev] | [next] | [standalone]
| From | Ram Pai <linuxram@us.ibm.com> |
|---|---|
| Date | 2017-06-21 02:00 +0200 |
| Subject | Re: [RFC v2 10/12] powerpc: Read AMR only if pkey-violation caused the exception. |
| Message-ID | <tUBdU-6Oo-13@gated-at.bofh.it> |
| In reply to | #1670336 |
On Tue, Jun 20, 2017 at 12:16:40PM +0530, Anshuman Khandual wrote: > On 06/19/2017 11:29 PM, Ram Pai wrote: > > On Mon, Jun 19, 2017 at 09:06:13PM +1000, Michael Ellerman wrote: > >> Ram Pai <linuxram@us.ibm.com> writes: > >> > >>> Signed-off-by: Ram Pai <linuxram@us.ibm.com> > >>> --- > >>> arch/powerpc/kernel/exceptions-64s.S | 16 ++++++++++------ > >>> 1 file changed, 10 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S > >>> index 8db9ef8..a4de1b4 100644 > >>> --- a/arch/powerpc/kernel/exceptions-64s.S > >>> +++ b/arch/powerpc/kernel/exceptions-64s.S > >>> @@ -493,13 +493,15 @@ EXC_COMMON_BEGIN(data_access_common) > >>> ld r12,_MSR(r1) > >>> ld r3,PACA_EXGEN+EX_DAR(r13) > >>> lwz r4,PACA_EXGEN+EX_DSISR(r13) > >>> + std r3,_DAR(r1) > >>> + std r4,_DSISR(r1) > >>> #ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS > >>> + andis. r0,r4,DSISR_KEYFAULT@h /* save AMR only if its a key fault */ > >>> + beq+ 1f > >> > >> This seems to be incremental on top of one of your other patches. > >> > >> But I don't see why, can you please just squash this into whatever patch > >> adds this code in the first place. > > > > It was an optimization added later. But yes it can be squashed into an > > earlier patch. > > Could you please explain what is the optimization this achieves ? Don't want to read the AMR if it is not a key protection fault. This is a hot-hot-path. A few cycles saved can accumulate into signficant savings overall. RP
[toc] | [prev] | [next] | [standalone]
| From | Ram Pai <linuxram@us.ibm.com> |
|---|---|
| Date | 2017-06-21 02:00 +0200 |
| Subject | Re: [RFC v2 10/12] powerpc: Read AMR only if pkey-violation caused the exception. |
| Message-ID | <tUBdU-6Oo-9@gated-at.bofh.it> |
| In reply to | #1669010 |
On Mon, Jun 19, 2017 at 09:06:13PM +1000, Michael Ellerman wrote: > Ram Pai <linuxram@us.ibm.com> writes: > > > Signed-off-by: Ram Pai <linuxram@us.ibm.com> > > --- > > arch/powerpc/kernel/exceptions-64s.S | 16 ++++++++++------ > > 1 file changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S > > index 8db9ef8..a4de1b4 100644 > > --- a/arch/powerpc/kernel/exceptions-64s.S > > +++ b/arch/powerpc/kernel/exceptions-64s.S > > @@ -493,13 +493,15 @@ EXC_COMMON_BEGIN(data_access_common) > > ld r12,_MSR(r1) > > ld r3,PACA_EXGEN+EX_DAR(r13) > > lwz r4,PACA_EXGEN+EX_DSISR(r13) > > + std r3,_DAR(r1) > > + std r4,_DSISR(r1) > > #ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS > > + andis. r0,r4,DSISR_KEYFAULT@h /* save AMR only if its a key fault */ > > + beq+ 1f > > This seems to be incremental on top of one of your other patches. > > But I don't see why, can you please just squash this into whatever patch > adds this code in the first place. Yes. squash it is. next version of my patch will have it squashed. RP
[toc] | [prev] | [next] | [standalone]
| From | Ram Pai <linuxram@us.ibm.com> |
|---|---|
| Date | 2017-06-17 06:00 +0200 |
| Subject | [RFC v2 03/12] powerpc: Implement sys_pkey_alloc and sys_pkey_free system call. |
| Message-ID | <tTd3Y-1kp-19@gated-at.bofh.it> |
| In reply to | #1668160 |
Sys_pkey_alloc() allocates and returns available pkey
Sys_pkey_free() frees up the pkey.
Total 32 keys are supported on powerpc. However pkey 0,1 and 31
are reserved. So effectively we have 29 pkeys.
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
arch/powerpc/Kconfig | 15 ++++
arch/powerpc/include/asm/book3s/64/mmu.h | 10 +++
arch/powerpc/include/asm/book3s/64/pgtable.h | 62 ++++++++++++++
arch/powerpc/include/asm/pkeys.h | 124 +++++++++++++++++++++++++++
arch/powerpc/include/asm/systbl.h | 2 +
arch/powerpc/include/asm/unistd.h | 4 +-
arch/powerpc/include/uapi/asm/unistd.h | 2 +
arch/powerpc/mm/Makefile | 1 +
arch/powerpc/mm/mmu_context_book3s64.c | 5 ++
arch/powerpc/mm/pkeys.c | 88 +++++++++++++++++++
include/linux/mm.h | 31 ++++---
include/uapi/asm-generic/mman-common.h | 2 +-
12 files changed, 331 insertions(+), 15 deletions(-)
create mode 100644 arch/powerpc/include/asm/pkeys.h
create mode 100644 arch/powerpc/mm/pkeys.c
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index f7c8f99..b6960617 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -871,6 +871,21 @@ 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/powerpc/protection-keys.txt
+
+ If unsure, say y.
endmenu
config ISA_DMA_API
diff --git a/arch/powerpc/include/asm/book3s/64/mmu.h b/arch/powerpc/include/asm/book3s/64/mmu.h
index 77529a3..0c0a2a8 100644
--- a/arch/powerpc/include/asm/book3s/64/mmu.h
+++ b/arch/powerpc/include/asm/book3s/64/mmu.h
@@ -108,6 +108,16 @@ struct patb_entry {
#ifdef CONFIG_SPAPR_TCE_IOMMU
struct list_head iommu_group_mem_list;
#endif
+
+#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
+ /*
+ * Each bit represents one protection key.
+ * bit set -> key allocated
+ * bit unset -> key available for allocation
+ */
+ u32 pkey_allocation_map;
+ s16 execute_only_pkey; /* key holding execute-only protection */
+#endif
} mm_context_t;
/*
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 85bc987..87e9a89 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -428,6 +428,68 @@ static inline void huge_ptep_set_wrprotect(struct mm_struct *mm,
pte_update(mm, addr, ptep, 0, _PAGE_PRIVILEGED, 1);
}
+
+#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
+
+#include <asm/reg.h>
+static inline u64 read_amr(void)
+{
+ return mfspr(SPRN_AMR);
+}
+static inline void write_amr(u64 value)
+{
+ mtspr(SPRN_AMR, value);
+}
+static inline u64 read_iamr(void)
+{
+ return mfspr(SPRN_IAMR);
+}
+static inline void write_iamr(u64 value)
+{
+ mtspr(SPRN_IAMR, value);
+}
+static inline u64 read_uamor(void)
+{
+ return mfspr(SPRN_UAMOR);
+}
+static inline void write_uamor(u64 value)
+{
+ mtspr(SPRN_UAMOR, value);
+}
+
+#else /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
+
+static inline u64 read_amr(void)
+{
+ WARN(1, "%s called with MEMORY PROTECTION KEYS disabled\n", __func__);
+ return -1;
+}
+static inline void write_amr(u64 value)
+{
+ WARN(1, "%s called with MEMORY PROTECTION KEYS disabled\n", __func__);
+}
+static inline u64 read_uamor(void)
+{
+ WARN(1, "%s called with MEMORY PROTECTION KEYS disabled\n", __func__);
+ return -1;
+}
+static inline void write_uamor(u64 value)
+{
+ WARN(1, "%s called with MEMORY PROTECTION KEYS disabled\n", __func__);
+}
+static inline u64 read_iamr(void)
+{
+ WARN(1, "%s called with MEMORY PROTECTION KEYS disabled\n", __func__);
+ return -1;
+}
+static inline void write_iamr(u64 value)
+{
+ WARN(1, "%s called with MEMORY PROTECTION KEYS disabled\n", __func__);
+}
+
+#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
+
+
#define __HAVE_ARCH_PTEP_GET_AND_CLEAR
static inline pte_t ptep_get_and_clear(struct mm_struct *mm,
unsigned long addr, pte_t *ptep)
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
new file mode 100644
index 0000000..7bc8746
--- /dev/null
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -0,0 +1,124 @@
+#ifndef _ASM_PPC64_PKEYS_H
+#define _ASM_PPC64_PKEYS_H
+
+
+#define arch_max_pkey() 32
+
+#define AMR_AD_BIT 0x1UL
+#define AMR_WD_BIT 0x2UL
+#define IAMR_EX_BIT 0x1UL
+#define AMR_BITS_PER_PKEY 2
+#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | \
+ VM_PKEY_BIT1 | \
+ VM_PKEY_BIT2 | \
+ 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
+
+#define pkeybit_mask(pkey) (0x1 << (arch_max_pkey() - pkey - 1))
+
+#define mm_pkey_allocation_map(mm) (mm->context.pkey_allocation_map)
+
+#define mm_set_pkey_allocated(mm, pkey) { \
+ mm_pkey_allocation_map(mm) |= pkeybit_mask(pkey); \
+}
+
+#define mm_set_pkey_free(mm, pkey) { \
+ mm_pkey_allocation_map(mm) &= ~pkeybit_mask(pkey); \
+}
+
+#define mm_set_pkey_is_allocated(mm, pkey) \
+ (mm_pkey_allocation_map(mm) & pkeybit_mask(pkey))
+
+#define mm_set_pkey_is_reserved(mm, pkey) (PKEY_INITIAL_ALLOCAION & \
+ pkeybit_mask(pkey))
+
+static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
+{
+ /* a reserved key is never considered as 'explicitly allocated' */
+ return (!mm_set_pkey_is_reserved(mm, pkey) &&
+ mm_set_pkey_is_allocated(mm, pkey));
+}
+
+/*
+ * Returns a positive, 5-bit key on success, or -1 on failure.
+ */
+static inline int mm_pkey_alloc(struct mm_struct *mm)
+{
+ /*
+ * Note: this is the one and only place we make sure
+ * that the pkey is valid as far as the hardware is
+ * concerned. The rest of the kernel trusts that
+ * only good, valid pkeys come out of here.
+ */
+ u32 all_pkeys_mask = (u32)(~(0x0));
+ int ret;
+
+ /*
+ * Are we out of pkeys? We must handle this specially
+ * because ffz() behavior is undefined if there are no
+ * zeros.
+ */
+ if (mm_pkey_allocation_map(mm) == all_pkeys_mask)
+ return -1;
+
+ ret = arch_max_pkey() -
+ ffz((u32)mm_pkey_allocation_map(mm))
+ - 1;
+ mm_set_pkey_allocated(mm, ret);
+ return ret;
+}
+
+static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
+{
+ if (!mm_pkey_is_allocated(mm, pkey))
+ return -EINVAL;
+
+ mm_set_pkey_free(mm, pkey);
+
+ return 0;
+}
+
+/*
+ * Try to dedicate one of the protection keys to be used as an
+ * execute-only protection key.
+ */
+extern int __execute_only_pkey(struct mm_struct *mm);
+static inline int execute_only_pkey(struct mm_struct *mm)
+{
+ return __execute_only_pkey(mm);
+}
+
+extern int __arch_override_mprotect_pkey(struct vm_area_struct *vma,
+ int prot, int pkey);
+static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,
+ int prot, int pkey)
+{
+ return __arch_override_mprotect_pkey(vma, prot, pkey);
+}
+
+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 __arch_set_user_pkey_access(tsk, pkey, init_val);
+}
+
+static inline pkey_mm_init(struct mm_struct *mm)
+{
+ mm_pkey_allocation_map(mm) = PKEY_INITIAL_ALLOCAION;
+ /* -1 means unallocated or invalid */
+ mm->context.execute_only_pkey = -1;
+}
+
+#endif /*_ASM_PPC64_PKEYS_H */
diff --git a/arch/powerpc/include/asm/systbl.h b/arch/powerpc/include/asm/systbl.h
index 1c94708..22dd776 100644
--- a/arch/powerpc/include/asm/systbl.h
+++ b/arch/powerpc/include/asm/systbl.h
@@ -388,3 +388,5 @@
COMPAT_SYS_SPU(pwritev2)
SYSCALL(kexec_file_load)
SYSCALL(statx)
+SYSCALL(pkey_alloc)
+SYSCALL(pkey_free)
diff --git a/arch/powerpc/include/asm/unistd.h b/arch/powerpc/include/asm/unistd.h
index 9ba11db..e0273bc 100644
--- a/arch/powerpc/include/asm/unistd.h
+++ b/arch/powerpc/include/asm/unistd.h
@@ -12,13 +12,11 @@
#include <uapi/asm/unistd.h>
-#define NR_syscalls 384
+#define NR_syscalls 386
#define __NR__exit __NR_exit
#define __IGNORE_pkey_mprotect
-#define __IGNORE_pkey_alloc
-#define __IGNORE_pkey_free
#ifndef __ASSEMBLY__
diff --git a/arch/powerpc/include/uapi/asm/unistd.h b/arch/powerpc/include/uapi/asm/unistd.h
index b85f142..7993a07 100644
--- a/arch/powerpc/include/uapi/asm/unistd.h
+++ b/arch/powerpc/include/uapi/asm/unistd.h
@@ -394,5 +394,7 @@
#define __NR_pwritev2 381
#define __NR_kexec_file_load 382
#define __NR_statx 383
+#define __NR_pkey_alloc 384
+#define __NR_pkey_free 385
#endif /* _UAPI_ASM_POWERPC_UNISTD_H_ */
diff --git a/arch/powerpc/mm/Makefile b/arch/powerpc/mm/Makefile
index 7414034..8cc2ff1 100644
--- a/arch/powerpc/mm/Makefile
+++ b/arch/powerpc/mm/Makefile
@@ -45,3 +45,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
diff --git a/arch/powerpc/mm/mmu_context_book3s64.c b/arch/powerpc/mm/mmu_context_book3s64.c
index c6dca2a..2da9931 100644
--- a/arch/powerpc/mm/mmu_context_book3s64.c
+++ b/arch/powerpc/mm/mmu_context_book3s64.c
@@ -16,6 +16,7 @@
#include <linux/string.h>
#include <linux/types.h>
#include <linux/mm.h>
+#include <linux/pkeys.h>
#include <linux/spinlock.h>
#include <linux/idr.h>
#include <linux/export.h>
@@ -120,6 +121,10 @@ static int hash__init_new_context(struct mm_struct *mm)
subpage_prot_init_new_context(mm);
+#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
+ pkey_mm_init(mm);
+#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
+
return index;
}
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
new file mode 100644
index 0000000..b97366e
--- /dev/null
+++ b/arch/powerpc/mm/pkeys.c
@@ -0,0 +1,88 @@
+/*
+ * 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_* */
+#include <uapi/asm-generic/mman-common.h>
+
+
+/*
+ * set the access right in AMR IAMR and UAMOR register
+ * for @pkey to that specified in @init_val.
+ */
+int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
+ unsigned long init_val)
+{
+ u64 old_amr, old_uamor, old_iamr;
+ int pkey_shift = (arch_max_pkey()-pkey-1) * AMR_BITS_PER_PKEY;
+ u64 new_amr_bits = 0x0ul;
+ u64 new_iamr_bits = 0x0ul;
+ u64 new_uamor_bits = 0x3ul;
+
+ /* Set the bits we need in AMR: */
+ if (init_val & PKEY_DISABLE_ACCESS)
+ new_amr_bits |= AMR_AD_BIT;
+ if (init_val & PKEY_DISABLE_WRITE)
+ new_amr_bits |= AMR_WD_BIT;
+
+ /*
+ * 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;
+
+ /* Shift the bits in to the correct place in AMR for pkey: */
+ new_amr_bits <<= pkey_shift;
+ new_iamr_bits <<= pkey_shift;
+ new_uamor_bits <<= pkey_shift;
+
+ /* Get old AMR and mask off any old bits in place: */
+ old_amr = read_amr();
+ old_amr &= ~((u64)(AMR_AD_BIT|AMR_WD_BIT) << pkey_shift);
+
+ old_iamr = read_iamr();
+ old_iamr &= ~(0x3ul << pkey_shift);
+
+ old_uamor = read_uamor();
+ old_uamor &= ~(0x3ul << pkey_shift);
+
+ /* Write old part along with new part: */
+ write_amr(old_amr | new_amr_bits);
+ write_iamr(old_iamr | new_iamr_bits);
+ write_uamor(old_uamor | new_uamor_bits);
+
+ return 0;
+}
+
+int __execute_only_pkey(struct mm_struct *mm)
+{
+ return -1;
+}
+
+/*
+ * This should only be called for *plain* mprotect calls.
+ */
+int __arch_override_mprotect_pkey(struct vm_area_struct *vma, int prot,
+ int pkey)
+{
+ /*
+ * Is this an mprotect_pkey() call? If so, never
+ * override the value that came from the user.
+ */
+ if (pkey != -1)
+ return pkey;
+
+ return 0;
+}
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 7cb17c6..34ddac7 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -204,26 +204,35 @@ extern int overcommit_kbytes_handler(struct ctl_table *, int, void __user *,
#define VM_MERGEABLE 0x80000000 /* KSM may merge identical pages */
#ifdef CONFIG_ARCH_USES_HIGH_VMA_FLAGS
-#define VM_HIGH_ARCH_BIT_0 32 /* bit only usable on 64-bit architectures */
-#define VM_HIGH_ARCH_BIT_1 33 /* bit only usable on 64-bit architectures */
-#define VM_HIGH_ARCH_BIT_2 34 /* bit only usable on 64-bit architectures */
-#define VM_HIGH_ARCH_BIT_3 35 /* bit only usable on 64-bit architectures */
+#define VM_HIGH_ARCH_BIT_0 32 /* bit only usable on 64-bit arch */
+#define VM_HIGH_ARCH_BIT_1 33 /* bit only usable on 64-bit arch */
+#define VM_HIGH_ARCH_BIT_2 34 /* bit only usable on 64-bit arch */
+#define VM_HIGH_ARCH_BIT_3 35 /* bit only usable on 64-bit arch */
+#define VM_HIGH_ARCH_BIT_4 36 /* bit only usable on 64-bit arch */
#define VM_HIGH_ARCH_0 BIT(VM_HIGH_ARCH_BIT_0)
#define VM_HIGH_ARCH_1 BIT(VM_HIGH_ARCH_BIT_1)
#define VM_HIGH_ARCH_2 BIT(VM_HIGH_ARCH_BIT_2)
#define VM_HIGH_ARCH_3 BIT(VM_HIGH_ARCH_BIT_3)
+#define VM_HIGH_ARCH_4 BIT(VM_HIGH_ARCH_BIT_4)
#endif /* CONFIG_ARCH_USES_HIGH_VMA_FLAGS */
#if defined(CONFIG_X86)
# define VM_PAT VM_ARCH_1 /* PAT reserves whole VMA at once (x86) */
-#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
+#if defined(CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS) \
+ || defined(CONFIG_PPC64_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 5-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 /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
#elif defined(CONFIG_PPC)
+#define VM_PKEY_BIT0 VM_HIGH_ARCH_0 /* A protection key is a 5-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
+#define VM_PKEY_BIT4 VM_HIGH_ARCH_4 /* intel does not use this bit */
+ /* but reserved for future expansion */
# define VM_SAO VM_ARCH_1 /* Strong Access Ordering (powerpc) */
#elif defined(CONFIG_PARISC)
# define VM_GROWSUP VM_ARCH_1
diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
index 8c27db0..b13ecc6 100644
--- a/include/uapi/asm-generic/mman-common.h
+++ b/include/uapi/asm-generic/mman-common.h
@@ -76,5 +76,5 @@
#define PKEY_DISABLE_WRITE 0x2
#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\
PKEY_DISABLE_WRITE)
-
+#define PKEY_DISABLE_EXECUTE 0x4
#endif /* __ASM_GENERIC_MMAN_COMMON_H */
--
1.8.3.1
[toc] | [prev] | [next] | [standalone]
| From | Michael Ellerman <mpe@ellerman.id.au> |
|---|---|
| Date | 2017-06-19 14:20 +0200 |
| Subject | Re: [RFC v2 03/12] powerpc: Implement sys_pkey_alloc and sys_pkey_free system call. |
| Message-ID | <tU3OW-2xn-15@gated-at.bofh.it> |
| In reply to | #1668168 |
Hi Ram,
Ram Pai <linuxram@us.ibm.com> writes:
> Sys_pkey_alloc() allocates and returns available pkey
> Sys_pkey_free() frees up the pkey.
>
> Total 32 keys are supported on powerpc. However pkey 0,1 and 31
> are reserved. So effectively we have 29 pkeys.
>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
> include/linux/mm.h | 31 ++++---
> include/uapi/asm-generic/mman-common.h | 2 +-
Those changes need to be split out and acked by mm folks.
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 7cb17c6..34ddac7 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -204,26 +204,35 @@ extern int overcommit_kbytes_handler(struct ctl_table *, int, void __user *,
> #define VM_MERGEABLE 0x80000000 /* KSM may merge identical pages */
>
> #ifdef CONFIG_ARCH_USES_HIGH_VMA_FLAGS
> -#define VM_HIGH_ARCH_BIT_0 32 /* bit only usable on 64-bit architectures */
> -#define VM_HIGH_ARCH_BIT_1 33 /* bit only usable on 64-bit architectures */
> -#define VM_HIGH_ARCH_BIT_2 34 /* bit only usable on 64-bit architectures */
> -#define VM_HIGH_ARCH_BIT_3 35 /* bit only usable on 64-bit architectures */
> +#define VM_HIGH_ARCH_BIT_0 32 /* bit only usable on 64-bit arch */
> +#define VM_HIGH_ARCH_BIT_1 33 /* bit only usable on 64-bit arch */
> +#define VM_HIGH_ARCH_BIT_2 34 /* bit only usable on 64-bit arch */
> +#define VM_HIGH_ARCH_BIT_3 35 /* bit only usable on 64-bit arch */
Please don't change the comments, it makes the diff harder to read.
You're actually just adding this AFAICS:
> +#define VM_HIGH_ARCH_BIT_4 36 /* bit only usable on 64-bit arch */
> #define VM_HIGH_ARCH_0 BIT(VM_HIGH_ARCH_BIT_0)
> #define VM_HIGH_ARCH_1 BIT(VM_HIGH_ARCH_BIT_1)
> #define VM_HIGH_ARCH_2 BIT(VM_HIGH_ARCH_BIT_2)
> #define VM_HIGH_ARCH_3 BIT(VM_HIGH_ARCH_BIT_3)
> +#define VM_HIGH_ARCH_4 BIT(VM_HIGH_ARCH_BIT_4)
> #endif /* CONFIG_ARCH_USES_HIGH_VMA_FLAGS */
>
> #if defined(CONFIG_X86)
^
> # define VM_PAT VM_ARCH_1 /* PAT reserves whole VMA at once (x86) */
> -#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
> +#if defined(CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS) \
> + || defined(CONFIG_PPC64_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 5-bit value */
^ 4?
> +#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 /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
That appears to be inside an #if defined(CONFIG_X86) ?
> #elif defined(CONFIG_PPC)
^
Should be CONFIG_PPC64_MEMORY_PROTECTION_KEYS no?
> +#define VM_PKEY_BIT0 VM_HIGH_ARCH_0 /* A protection key is a 5-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
> +#define VM_PKEY_BIT4 VM_HIGH_ARCH_4 /* intel does not use this bit */
> + /* but reserved for future expansion */
But this hunk is for PPC ?
Is it OK for the other arches & generic code to add another VM_PKEY_BIT4 ?
Do you need to update show_smap_vma_flags() ?
> # define VM_SAO VM_ARCH_1 /* Strong Access Ordering (powerpc) */
> #elif defined(CONFIG_PARISC)
> # define VM_GROWSUP VM_ARCH_1
> diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
> index 8c27db0..b13ecc6 100644
> --- a/include/uapi/asm-generic/mman-common.h
> +++ b/include/uapi/asm-generic/mman-common.h
> @@ -76,5 +76,5 @@
> #define PKEY_DISABLE_WRITE 0x2
> #define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\
> PKEY_DISABLE_WRITE)
> -
> +#define PKEY_DISABLE_EXECUTE 0x4
How you can set that if it's not in PKEY_ACCESS_MASK?
See:
SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
{
int pkey;
int ret;
/* No flags supported yet. */
if (flags)
return -EINVAL;
/* check for unsupported init values */
if (init_val & ~PKEY_ACCESS_MASK)
return -EINVAL;
cheers
[toc] | [prev] | [next] | [standalone]
| From | Ram Pai <linuxram@us.ibm.com> |
|---|---|
| Date | 2017-06-21 00:50 +0200 |
| Subject | Re: [RFC v2 03/12] powerpc: Implement sys_pkey_alloc and sys_pkey_free system call. |
| Message-ID | <tUA89-69h-5@gated-at.bofh.it> |
| In reply to | #1669026 |
On Mon, Jun 19, 2017 at 10:18:01PM +1000, Michael Ellerman wrote:
> Hi Ram,
>
> Ram Pai <linuxram@us.ibm.com> writes:
> > Sys_pkey_alloc() allocates and returns available pkey
> > Sys_pkey_free() frees up the pkey.
> >
> > Total 32 keys are supported on powerpc. However pkey 0,1 and 31
> > are reserved. So effectively we have 29 pkeys.
> >
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > ---
> > include/linux/mm.h | 31 ++++---
> > include/uapi/asm-generic/mman-common.h | 2 +-
>
> Those changes need to be split out and acked by mm folks.
>
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 7cb17c6..34ddac7 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -204,26 +204,35 @@ extern int overcommit_kbytes_handler(struct ctl_table *, int, void __user *,
> > #define VM_MERGEABLE 0x80000000 /* KSM may merge identical pages */
> >
> > #ifdef CONFIG_ARCH_USES_HIGH_VMA_FLAGS
> > -#define VM_HIGH_ARCH_BIT_0 32 /* bit only usable on 64-bit architectures */
> > -#define VM_HIGH_ARCH_BIT_1 33 /* bit only usable on 64-bit architectures */
> > -#define VM_HIGH_ARCH_BIT_2 34 /* bit only usable on 64-bit architectures */
> > -#define VM_HIGH_ARCH_BIT_3 35 /* bit only usable on 64-bit architectures */
> > +#define VM_HIGH_ARCH_BIT_0 32 /* bit only usable on 64-bit arch */
> > +#define VM_HIGH_ARCH_BIT_1 33 /* bit only usable on 64-bit arch */
> > +#define VM_HIGH_ARCH_BIT_2 34 /* bit only usable on 64-bit arch */
> > +#define VM_HIGH_ARCH_BIT_3 35 /* bit only usable on 64-bit arch */
>
> Please don't change the comments, it makes the diff harder to read.
The lines were surpassing 80 columns. tried to compress the comments
without loosing meaning. will restore.
>
> You're actually just adding this AFAICS:
>
> > +#define VM_HIGH_ARCH_BIT_4 36 /* bit only usable on 64-bit arch */
>
> > #define VM_HIGH_ARCH_0 BIT(VM_HIGH_ARCH_BIT_0)
> > #define VM_HIGH_ARCH_1 BIT(VM_HIGH_ARCH_BIT_1)
> > #define VM_HIGH_ARCH_2 BIT(VM_HIGH_ARCH_BIT_2)
> > #define VM_HIGH_ARCH_3 BIT(VM_HIGH_ARCH_BIT_3)
> > +#define VM_HIGH_ARCH_4 BIT(VM_HIGH_ARCH_BIT_4)
> > #endif /* CONFIG_ARCH_USES_HIGH_VMA_FLAGS */
> >
> > #if defined(CONFIG_X86)
> ^
> > # define VM_PAT VM_ARCH_1 /* PAT reserves whole VMA at once (x86) */
> > -#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
> > +#if defined(CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS) \
> > + || defined(CONFIG_PPC64_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 5-bit value */
> ^ 4?
> > +#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 /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
>
> That appears to be inside an #if defined(CONFIG_X86) ?
>
> > #elif defined(CONFIG_PPC)
> ^
> Should be CONFIG_PPC64_MEMORY_PROTECTION_KEYS no?
Its a little garbled. Will fix it.
>
> > +#define VM_PKEY_BIT0 VM_HIGH_ARCH_0 /* A protection key is a 5-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
> > +#define VM_PKEY_BIT4 VM_HIGH_ARCH_4 /* intel does not use this bit */
> > + /* but reserved for future expansion */
>
> But this hunk is for PPC ?
>
> Is it OK for the other arches & generic code to add another VM_PKEY_BIT4 ?
No. it has to be PPC specific.
>
> Do you need to update show_smap_vma_flags() ?
>
> > # define VM_SAO VM_ARCH_1 /* Strong Access Ordering (powerpc) */
> > #elif defined(CONFIG_PARISC)
> > # define VM_GROWSUP VM_ARCH_1
>
> > diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
> > index 8c27db0..b13ecc6 100644
> > --- a/include/uapi/asm-generic/mman-common.h
> > +++ b/include/uapi/asm-generic/mman-common.h
> > @@ -76,5 +76,5 @@
> > #define PKEY_DISABLE_WRITE 0x2
> > #define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\
> > PKEY_DISABLE_WRITE)
> > -
> > +#define PKEY_DISABLE_EXECUTE 0x4
>
> How you can set that if it's not in PKEY_ACCESS_MASK?
I was wondering how to handle this. x86 does not support this flag.
However powerpc has the ability to enable/disable execute permission
on a key. It cannot be done from userspace, but can be done through
the sys_mprotect_pkey() sys call. Initially I was thinking of not
enabling it in powerpc aswell, but than i think we should be not block
the hardware feature from being used. I will make
PKEY_DISABLE_EXECUTE as part of the PKEY_ACCESS_MASK, and have powerpc
handle it. Also will x86 patch that return error if the flag is
provided.
makes sense?
Thanks for your comments,
RP
>
> See:
>
> SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
> {
> int pkey;
> int ret;
>
> /* No flags supported yet. */
> if (flags)
> return -EINVAL;
> /* check for unsupported init values */
> if (init_val & ~PKEY_ACCESS_MASK)
> return -EINVAL;
>
>
> cheers
--
Ram Pai
[toc] | [prev] | [next] | [standalone]
| From | Ram Pai <linuxram@us.ibm.com> |
|---|---|
| Date | 2017-06-17 06:00 +0200 |
| Subject | [RFC v2 06/12] powerpc: Program HPTE key protection bits. |
| Message-ID | <tTd3Y-1kp-23@gated-at.bofh.it> |
| In reply to | #1668160 |
Map the PTE protection key bits to the HPTE key protection bits, while creatiing HPTE entries. Signed-off-by: Ram Pai <linuxram@us.ibm.com> --- arch/powerpc/include/asm/book3s/64/mmu-hash.h | 5 +++++ arch/powerpc/include/asm/pkeys.h | 7 +++++++ arch/powerpc/mm/hash_utils_64.c | 5 +++++ 3 files changed, 17 insertions(+) diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h index cfb8169..3d7872c 100644 --- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h +++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h @@ -90,6 +90,8 @@ #define HPTE_R_PP0 ASM_CONST(0x8000000000000000) #define HPTE_R_TS ASM_CONST(0x4000000000000000) #define HPTE_R_KEY_HI ASM_CONST(0x3000000000000000) +#define HPTE_R_KEY_BIT0 ASM_CONST(0x2000000000000000) +#define HPTE_R_KEY_BIT1 ASM_CONST(0x1000000000000000) #define HPTE_R_RPN_SHIFT 12 #define HPTE_R_RPN ASM_CONST(0x0ffffffffffff000) #define HPTE_R_RPN_3_0 ASM_CONST(0x01fffffffffff000) @@ -104,6 +106,9 @@ #define HPTE_R_C ASM_CONST(0x0000000000000080) #define HPTE_R_R ASM_CONST(0x0000000000000100) #define HPTE_R_KEY_LO ASM_CONST(0x0000000000000e00) +#define HPTE_R_KEY_BIT2 ASM_CONST(0x0000000000000800) +#define HPTE_R_KEY_BIT3 ASM_CONST(0x0000000000000400) +#define HPTE_R_KEY_BIT4 ASM_CONST(0x0000000000000200) #define HPTE_V_1TB_SEG ASM_CONST(0x4000000000000000) #define HPTE_V_VRMA_MASK ASM_CONST(0x4001ffffff000000) diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h index 0f3dca8..9b6820d 100644 --- a/arch/powerpc/include/asm/pkeys.h +++ b/arch/powerpc/include/asm/pkeys.h @@ -27,6 +27,13 @@ ((vm_flags & VM_PKEY_BIT3) ? H_PAGE_PKEY_BIT1 : 0x0UL) | \ ((vm_flags & VM_PKEY_BIT4) ? H_PAGE_PKEY_BIT0 : 0x0UL)) +#define calc_pte_to_hpte_pkey_bits(pteflags) \ + (((pteflags & H_PAGE_PKEY_BIT0) ? HPTE_R_KEY_BIT0 : 0x0UL) | \ + ((pteflags & H_PAGE_PKEY_BIT1) ? HPTE_R_KEY_BIT1 : 0x0UL) | \ + ((pteflags & H_PAGE_PKEY_BIT2) ? HPTE_R_KEY_BIT2 : 0x0UL) | \ + ((pteflags & H_PAGE_PKEY_BIT3) ? HPTE_R_KEY_BIT3 : 0x0UL) | \ + ((pteflags & H_PAGE_PKEY_BIT4) ? HPTE_R_KEY_BIT4 : 0x0UL)) + /* * Bits are in BE format. * NOTE: key 31, 1, 0 are not used. diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c index c0f4b46..7d974cd 100644 --- a/arch/powerpc/mm/hash_utils_64.c +++ b/arch/powerpc/mm/hash_utils_64.c @@ -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> @@ -230,6 +231,10 @@ unsigned long htab_convert_pte_flags(unsigned long pteflags) */ rflags |= HPTE_R_M; +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS + rflags |= calc_pte_to_hpte_pkey_bits(pteflags); +#endif + return rflags; } -- 1.8.3.1
[toc] | [prev] | [next] | [standalone]
| From | Anshuman Khandual <khandual@linux.vnet.ibm.com> |
|---|---|
| Date | 2017-06-20 10:30 +0200 |
| Subject | Re: [RFC v2 06/12] powerpc: Program HPTE key protection bits. |
| Message-ID | <tUmHV-6aA-33@gated-at.bofh.it> |
| In reply to | #1668169 |
On 06/17/2017 09:22 AM, Ram Pai wrote: > Map the PTE protection key bits to the HPTE key protection bits, > while creatiing HPTE entries. > > Signed-off-by: Ram Pai <linuxram@us.ibm.com> > --- > arch/powerpc/include/asm/book3s/64/mmu-hash.h | 5 +++++ > arch/powerpc/include/asm/pkeys.h | 7 +++++++ > arch/powerpc/mm/hash_utils_64.c | 5 +++++ > 3 files changed, 17 insertions(+) > > diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h > index cfb8169..3d7872c 100644 > --- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h > +++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h > @@ -90,6 +90,8 @@ > #define HPTE_R_PP0 ASM_CONST(0x8000000000000000) > #define HPTE_R_TS ASM_CONST(0x4000000000000000) > #define HPTE_R_KEY_HI ASM_CONST(0x3000000000000000) > +#define HPTE_R_KEY_BIT0 ASM_CONST(0x2000000000000000) > +#define HPTE_R_KEY_BIT1 ASM_CONST(0x1000000000000000) > #define HPTE_R_RPN_SHIFT 12 > #define HPTE_R_RPN ASM_CONST(0x0ffffffffffff000) > #define HPTE_R_RPN_3_0 ASM_CONST(0x01fffffffffff000) > @@ -104,6 +106,9 @@ > #define HPTE_R_C ASM_CONST(0x0000000000000080) > #define HPTE_R_R ASM_CONST(0x0000000000000100) > #define HPTE_R_KEY_LO ASM_CONST(0x0000000000000e00) > +#define HPTE_R_KEY_BIT2 ASM_CONST(0x0000000000000800) > +#define HPTE_R_KEY_BIT3 ASM_CONST(0x0000000000000400) > +#define HPTE_R_KEY_BIT4 ASM_CONST(0x0000000000000200) > Should we indicate/document how these 5 bits are not contiguous in the HPTE format for any given real page ? > #define HPTE_V_1TB_SEG ASM_CONST(0x4000000000000000) > #define HPTE_V_VRMA_MASK ASM_CONST(0x4001ffffff000000) > diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h > index 0f3dca8..9b6820d 100644 > --- a/arch/powerpc/include/asm/pkeys.h > +++ b/arch/powerpc/include/asm/pkeys.h > @@ -27,6 +27,13 @@ > ((vm_flags & VM_PKEY_BIT3) ? H_PAGE_PKEY_BIT1 : 0x0UL) | \ > ((vm_flags & VM_PKEY_BIT4) ? H_PAGE_PKEY_BIT0 : 0x0UL)) > > +#define calc_pte_to_hpte_pkey_bits(pteflags) \ > + (((pteflags & H_PAGE_PKEY_BIT0) ? HPTE_R_KEY_BIT0 : 0x0UL) | \ > + ((pteflags & H_PAGE_PKEY_BIT1) ? HPTE_R_KEY_BIT1 : 0x0UL) | \ > + ((pteflags & H_PAGE_PKEY_BIT2) ? HPTE_R_KEY_BIT2 : 0x0UL) | \ > + ((pteflags & H_PAGE_PKEY_BIT3) ? HPTE_R_KEY_BIT3 : 0x0UL) | \ > + ((pteflags & H_PAGE_PKEY_BIT4) ? HPTE_R_KEY_BIT4 : 0x0UL)) > + We can drop calc_ in here. pte_to_hpte_pkey_bits should be sufficient.
[toc] | [prev] | [next] | [standalone]
| From | Ram Pai <linuxram@us.ibm.com> |
|---|---|
| Date | 2017-06-21 01:30 +0200 |
| Subject | Re: [RFC v2 06/12] powerpc: Program HPTE key protection bits. |
| Message-ID | <tUAKS-6CT-15@gated-at.bofh.it> |
| In reply to | #1670419 |
On Tue, Jun 20, 2017 at 01:51:45PM +0530, Anshuman Khandual wrote: > On 06/17/2017 09:22 AM, Ram Pai wrote: > > Map the PTE protection key bits to the HPTE key protection bits, > > while creatiing HPTE entries. > > > > Signed-off-by: Ram Pai <linuxram@us.ibm.com> > > --- > > arch/powerpc/include/asm/book3s/64/mmu-hash.h | 5 +++++ > > arch/powerpc/include/asm/pkeys.h | 7 +++++++ > > arch/powerpc/mm/hash_utils_64.c | 5 +++++ > > 3 files changed, 17 insertions(+) > > > > diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h > > index cfb8169..3d7872c 100644 > > --- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h > > +++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h > > @@ -90,6 +90,8 @@ > > #define HPTE_R_PP0 ASM_CONST(0x8000000000000000) > > #define HPTE_R_TS ASM_CONST(0x4000000000000000) > > #define HPTE_R_KEY_HI ASM_CONST(0x3000000000000000) > > +#define HPTE_R_KEY_BIT0 ASM_CONST(0x2000000000000000) > > +#define HPTE_R_KEY_BIT1 ASM_CONST(0x1000000000000000) > > #define HPTE_R_RPN_SHIFT 12 > > #define HPTE_R_RPN ASM_CONST(0x0ffffffffffff000) > > #define HPTE_R_RPN_3_0 ASM_CONST(0x01fffffffffff000) > > @@ -104,6 +106,9 @@ > > #define HPTE_R_C ASM_CONST(0x0000000000000080) > > #define HPTE_R_R ASM_CONST(0x0000000000000100) > > #define HPTE_R_KEY_LO ASM_CONST(0x0000000000000e00) > > +#define HPTE_R_KEY_BIT2 ASM_CONST(0x0000000000000800) > > +#define HPTE_R_KEY_BIT3 ASM_CONST(0x0000000000000400) > > +#define HPTE_R_KEY_BIT4 ASM_CONST(0x0000000000000200) > > > > Should we indicate/document how these 5 bits are not contiguous > in the HPTE format for any given real page ? I can, but its all well documented in the ISA. Infact all the bits and the macros are one to one translation from the ISA. > > > #define HPTE_V_1TB_SEG ASM_CONST(0x4000000000000000) > > #define HPTE_V_VRMA_MASK ASM_CONST(0x4001ffffff000000) > > diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h > > index 0f3dca8..9b6820d 100644 > > --- a/arch/powerpc/include/asm/pkeys.h > > +++ b/arch/powerpc/include/asm/pkeys.h > > @@ -27,6 +27,13 @@ > > ((vm_flags & VM_PKEY_BIT3) ? H_PAGE_PKEY_BIT1 : 0x0UL) | \ > > ((vm_flags & VM_PKEY_BIT4) ? H_PAGE_PKEY_BIT0 : 0x0UL)) > > > > +#define calc_pte_to_hpte_pkey_bits(pteflags) \ > > + (((pteflags & H_PAGE_PKEY_BIT0) ? HPTE_R_KEY_BIT0 : 0x0UL) | \ > > + ((pteflags & H_PAGE_PKEY_BIT1) ? HPTE_R_KEY_BIT1 : 0x0UL) | \ > > + ((pteflags & H_PAGE_PKEY_BIT2) ? HPTE_R_KEY_BIT2 : 0x0UL) | \ > > + ((pteflags & H_PAGE_PKEY_BIT3) ? HPTE_R_KEY_BIT3 : 0x0UL) | \ > > + ((pteflags & H_PAGE_PKEY_BIT4) ? HPTE_R_KEY_BIT4 : 0x0UL)) > > + > > We can drop calc_ in here. pte_to_hpte_pkey_bits should be > sufficient. ok. will do. thanks for your comments, RP
[toc] | [prev] | [next] | [standalone]
| From | Ram Pai <linuxram@us.ibm.com> |
|---|---|
| Date | 2017-06-17 06:00 +0200 |
| Subject | [RFC v2 02/12] powerpc: Free up four 64K PTE bits in 64K backed hpte pages. |
| Message-ID | <tTd3Y-1kp-25@gated-at.bofh.it> |
| In reply to | #1668160 |
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.
This patch does the following change to 64K PTE that is
backed by 64K hpte.
H_PAGE_F_SECOND which occupied bit 4 moves to the second part
of the pte.
H_PAGE_F_GIX which occupied bit 5, 6 and 7 also moves to the
second part of the pte.
since bit 7 is now freed up, we move H_PAGE_BUSY from bit 9
to bit 7. Trying to minimize gaps so that contiguous bits
can be allocated if needed in the future.
The second part of the PTE will hold
(H_PAGE_F_SECOND|H_PAGE_F_GIX) at bit 60,61,62,63.
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
arch/powerpc/include/asm/book3s/64/hash-64k.h | 26 ++++++++------------------
arch/powerpc/mm/hash64_64k.c | 16 +++++++---------
arch/powerpc/mm/hugetlbpage-hash64.c | 16 ++++++----------
3 files changed, 21 insertions(+), 37 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/hash-64k.h b/arch/powerpc/include/asm/book3s/64/hash-64k.h
index 0eb3c89..2fa5c60 100644
--- a/arch/powerpc/include/asm/book3s/64/hash-64k.h
+++ b/arch/powerpc/include/asm/book3s/64/hash-64k.h
@@ -12,12 +12,8 @@
*/
#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_F_SECOND _RPAGE_RSV2 /* HPTE is in 2ndary HPTEG */
-#define H_PAGE_F_GIX (_RPAGE_RSV3 | _RPAGE_RSV4 | _RPAGE_RPN44)
-#define H_PAGE_F_GIX_SHIFT 56
-
-#define H_PAGE_BUSY _RPAGE_RPN42 /* software: PTE & hash are busy */
+#define H_PAGE_BUSY _RPAGE_RPN44 /* software: PTE & hash are busy */
#define H_PAGE_HASHPTE _RPAGE_RPN43 /* PTE has associated HPTE */
/*
@@ -56,24 +52,18 @@ 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;
- }
+ /*
+ * The store side ordering is done in __hash_page_4K
+ */
+ 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);
}
static inline unsigned long set_hidx_slot(pte_t *ptep, real_pte_t rpte,
diff --git a/arch/powerpc/mm/hash64_64k.c b/arch/powerpc/mm/hash64_64k.c
index 3702a3c..1c25ec2 100644
--- a/arch/powerpc/mm/hash64_64k.c
+++ b/arch/powerpc/mm/hash64_64k.c
@@ -211,6 +211,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;
@@ -247,6 +248,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))
@@ -254,16 +256,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,
+ gslot = get_hidx_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;
@@ -313,8 +312,7 @@ int __hash_page_64K(unsigned long ea, unsigned long access,
return -1;
}
- new_pte |= (slot << H_PAGE_F_GIX_SHIFT) &
- (H_PAGE_F_SECOND | H_PAGE_F_GIX);
+ set_hidx_slot(ptep, rpte, 0, slot);
new_pte = (new_pte & ~_PAGE_HPTEFLAGS) | H_PAGE_HASHPTE;
}
*ptep = __pte(new_pte & ~H_PAGE_BUSY);
diff --git a/arch/powerpc/mm/hugetlbpage-hash64.c b/arch/powerpc/mm/hugetlbpage-hash64.c
index a84bb44..239ca86 100644
--- a/arch/powerpc/mm/hugetlbpage-hash64.c
+++ b/arch/powerpc/mm/hugetlbpage-hash64.c
@@ -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,15 +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;
+ 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,
+ gslot = get_hidx_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;
}
@@ -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 |= set_hidx_slot(ptep, rpte, 0, slot);
}
/*
--
1.8.3.1
[toc] | [prev] | [next] | [standalone]
| From | Anshuman Khandual <khandual@linux.vnet.ibm.com> |
|---|---|
| Date | 2017-06-20 13:00 +0200 |
| Subject | Re: [RFC v2 02/12] powerpc: Free up four 64K PTE bits in 64K backed hpte pages. |
| Message-ID | <tUp33-7yf-1@gated-at.bofh.it> |
| In reply to | #1668170 |
On 06/17/2017 09:22 AM, Ram Pai 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. > > This patch does the following change to 64K PTE that is > backed by 64K hpte. > > H_PAGE_F_SECOND which occupied bit 4 moves to the second part > of the pte. > H_PAGE_F_GIX which occupied bit 5, 6 and 7 also moves to the > second part of the pte. > > since bit 7 is now freed up, we move H_PAGE_BUSY from bit 9 > to bit 7. Trying to minimize gaps so that contiguous bits > can be allocated if needed in the future. > > The second part of the PTE will hold > (H_PAGE_F_SECOND|H_PAGE_F_GIX) at bit 60,61,62,63. I still dont understand how we freed up the 5th bit which is used in the 5th patch. Was that bit never used for any thing on 64K page size (64K and 4K mappings) ? +#define _RPAGE_RSV5 0x00040UL +#define H_PAGE_PKEY_BIT0 _RPAGE_RSV1 +#define H_PAGE_PKEY_BIT1 _RPAGE_RSV2 +#define H_PAGE_PKEY_BIT2 _RPAGE_RSV3 +#define H_PAGE_PKEY_BIT3 _RPAGE_RSV4 +#define H_PAGE_PKEY_BIT4 _RPAGE_RSV5
[toc] | [prev] | [next] | [standalone]
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
Back to top | Article view | linux.kernel
csiph-web