On Fri, May 17, 2024 at 09:00:09PM +0200, Christophe Leroy wrote:
> In order to allow leaf PMD entries, switch the PGD to 64 bits entries.
>
> Signed-off-by: Christophe Leroy <[email protected]>
I do not quite understand this change.
Are not powerE500 and power85xx two different things?
You are changing making it 64 for PPC_E500_64bits, but you are updating head_85xx.
Are they sharing this code?
Also, we would benefit from a slightly bigger changelog, explaining why
do we need this change in some more detail.
> diff --git a/arch/powerpc/include/asm/pgtable-types.h b/arch/powerpc/include/asm/pgtable-types.h
> index 082c85cc09b1..db965d98e0ae 100644
> --- a/arch/powerpc/include/asm/pgtable-types.h
> +++ b/arch/powerpc/include/asm/pgtable-types.h
> @@ -49,7 +49,11 @@ static inline unsigned long pud_val(pud_t x)
> #endif /* CONFIG_PPC64 */
>
> /* PGD level */
> +#if defined(CONFIG_PPC_E500) && defined(CONFIG_PTE_64BIT)
> +typedef struct { unsigned long long pgd; } pgd_t;
> +#else
> typedef struct { unsigned long pgd; } pgd_t;
> +#endif
> #define __pgd(x) ((pgd_t) { (x) })
> static inline unsigned long pgd_val(pgd_t x)
> {
> diff --git a/arch/powerpc/kernel/head_85xx.S b/arch/powerpc/kernel/head_85xx.S
> index 39724ff5ae1f..a305244afc9f 100644
> --- a/arch/powerpc/kernel/head_85xx.S
> +++ b/arch/powerpc/kernel/head_85xx.S
> @@ -307,8 +307,9 @@ set_ivor:
> #ifdef CONFIG_PTE_64BIT
> #ifdef CONFIG_HUGETLB_PAGE
> #define FIND_PTE \
> - rlwinm r12, r10, 13, 19, 29; /* Compute pgdir/pmd offset */ \
> - lwzx r11, r12, r11; /* Get pgd/pmd entry */ \
> + rlwinm r12, r10, 14, 18, 28; /* Compute pgdir/pmd offset */ \
> + add r12, r11, r12;
You add the offset to pgdir?
> + lwz r11, 4(r12); /* Get pgd/pmd entry */ \
What is i offset 4?
--
Oscar Salvador
SUSE Labs
Le 25/05/2024 à 06:54, Oscar Salvador a écrit :
> On Fri, May 17, 2024 at 09:00:09PM +0200, Christophe Leroy wrote:
>> In order to allow leaf PMD entries, switch the PGD to 64 bits entries.
>>
>> Signed-off-by: Christophe Leroy <[email protected]>
>
> I do not quite understand this change.
> Are not powerE500 and power85xx two different things?
Yes they are two different things, but one contains the other
e500 is the processor-core which is included inside the MPC85xx micro
controller.
But CONFIG_PPC_E500 is a bit more than e500 core, it also includes e5500
and e6500 which are evolutions of e500.
mpc85xx is 32 bits
e5500 and e6500 are 64 bits
> You are changing making it 64 for PPC_E500_64bits, but you are updating head_85xx.
> Are they sharing this code?
Not exactly. mpc85xx can be built with 32 bits PTE or 64 bits PTE, based
on CONFIG_PTE_64BIT
When CONFIG_PTE_64BIT is selected it uses the same PTE layout on 32-bits
and 64-bits. But on 32-bits the PGD is still 32-bits, so it is not
possible to use leaf entries at PGD level hence the change.
When CONFIG_PTE_64BIT is not selected, huge pages are not supported.
>
> Also, we would benefit from a slightly bigger changelog, explaining why
> do we need this change in some more detail.
Yes I can write this is because PTEs are 64-bits allthought I thought it
was obvious.
>
>
>> diff --git a/arch/powerpc/include/asm/pgtable-types.h b/arch/powerpc/include/asm/pgtable-types.h
>> index 082c85cc09b1..db965d98e0ae 100644
>> --- a/arch/powerpc/include/asm/pgtable-types.h
>> +++ b/arch/powerpc/include/asm/pgtable-types.h
>> @@ -49,7 +49,11 @@ static inline unsigned long pud_val(pud_t x)
>> #endif /* CONFIG_PPC64 */
>>
>> /* PGD level */
>> +#if defined(CONFIG_PPC_E500) && defined(CONFIG_PTE_64BIT)
>> +typedef struct { unsigned long long pgd; } pgd_t;
>> +#else
>> typedef struct { unsigned long pgd; } pgd_t;
>> +#endif
>> #define __pgd(x) ((pgd_t) { (x) })
>> static inline unsigned long pgd_val(pgd_t x)
>> {
>> diff --git a/arch/powerpc/kernel/head_85xx.S b/arch/powerpc/kernel/head_85xx.S
>> index 39724ff5ae1f..a305244afc9f 100644
>> --- a/arch/powerpc/kernel/head_85xx.S
>> +++ b/arch/powerpc/kernel/head_85xx.S
>> @@ -307,8 +307,9 @@ set_ivor:
>> #ifdef CONFIG_PTE_64BIT
>> #ifdef CONFIG_HUGETLB_PAGE
>> #define FIND_PTE \
>> - rlwinm r12, r10, 13, 19, 29; /* Compute pgdir/pmd offset */ \
>> - lwzx r11, r12, r11; /* Get pgd/pmd entry */ \
>> + rlwinm r12, r10, 14, 18, 28; /* Compute pgdir/pmd offset */ \
>> + add r12, r11, r12;
>
> You add the offset to pgdir?
Yes because later r12 points to the PTE so when it is a leaf PGD entry
we need r12 to point to that entry.
>
>> + lwz r11, 4(r12); /* Get pgd/pmd entry */ \
>
> What is i offset 4?
It is big endian, the entry is now 64 bits but the real content of the
entry is still 32 bits so it is in the lower word.
>
>