Re: [RFC PATCH v2 15/20] powerpc/85xx: Switch to 64 bits PGD

[Christophe Leroy <christophe.leroy@csgroup.eu>]

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 <christophe.leroy@csgroup.eu>
> 
> 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.

> 
>