diff mbox series

[RFC,v2,10/20] powerpc/mm: Fix __find_linux_pte() on 32 bits with PMD leaf entries

Message ID 41bf05806501c0091a7d52e118b187416d24a76f.1715971869.git.christophe.leroy@csgroup.eu (mailing list archive)
State New
Headers show
Series Reimplement huge pages without hugepd on powerpc (8xx, e500, book3s/64) | expand

Commit Message

Christophe Leroy May 17, 2024, 7 p.m. UTC
Building on 32 bits with pmd_leaf() not returning always false leads
to the following error:

  CC      arch/powerpc/mm/pgtable.o
arch/powerpc/mm/pgtable.c: In function '__find_linux_pte':
arch/powerpc/mm/pgtable.c:506:1: error: function may return address of local variable [-Werror=return-local-addr]
  506 | }
      | ^
arch/powerpc/mm/pgtable.c:394:15: note: declared here
  394 |         pud_t pud, *pudp;
      |               ^~~
arch/powerpc/mm/pgtable.c:394:15: note: declared here

This is due to pmd_offset() being a no-op in that case.

So rework it for powerpc/32 so that pXd_offset() are used on real
pointers and not on on-stack copies.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/mm/pgtable.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Oscar Salvador May 25, 2024, 4:12 a.m. UTC | #1
On Fri, May 17, 2024 at 09:00:04PM +0200, Christophe Leroy wrote:
> Building on 32 bits with pmd_leaf() not returning always false leads
> to the following error:

I am curious though.
pmd_leaf is only defined in include/linux/pgtable.h for 32bits, and is hardcoded
to false.
I do not see where we change it in previous patches, so is this artificial?

> 
>   CC      arch/powerpc/mm/pgtable.o
> arch/powerpc/mm/pgtable.c: In function '__find_linux_pte':
> arch/powerpc/mm/pgtable.c:506:1: error: function may return address of local variable [-Werror=return-local-addr]
>   506 | }
>       | ^
> arch/powerpc/mm/pgtable.c:394:15: note: declared here
>   394 |         pud_t pud, *pudp;
>       |               ^~~
> arch/powerpc/mm/pgtable.c:394:15: note: declared here
> 
> This is due to pmd_offset() being a no-op in that case.

This is because 32bits powerpc include pgtable-nopmd.h?

> So rework it for powerpc/32 so that pXd_offset() are used on real
> pointers and not on on-stack copies.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/mm/pgtable.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
> index 59f0d7706d2f..51ee508eeb5b 100644
> --- a/arch/powerpc/mm/pgtable.c
> +++ b/arch/powerpc/mm/pgtable.c
> @@ -390,8 +390,12 @@ pte_t *__find_linux_pte(pgd_t *pgdir, unsigned long ea,
>  			bool *is_thp, unsigned *hpage_shift)
>  {
>  	pgd_t *pgdp;
> -	p4d_t p4d, *p4dp;
> -	pud_t pud, *pudp;
> +	p4d_t *p4dp;
> +	pud_t *pudp;
> +#ifdef CONFIG_PPC64
> +	p4d_t p4d;
> +	pud_t pud;
> +#endif
>  	pmd_t pmd, *pmdp;
>  	pte_t *ret_pte;
>  	hugepd_t *hpdp = NULL;
> @@ -412,6 +416,7 @@ pte_t *__find_linux_pte(pgd_t *pgdir, unsigned long ea,
>  	 */
>  	pgdp = pgdir + pgd_index(ea);
>  	p4dp = p4d_offset(pgdp, ea);
> +#ifdef CONFIG_PPC64
>  	p4d  = READ_ONCE(*p4dp);
>  	pdshift = P4D_SHIFT;
>  
> @@ -452,6 +457,11 @@ pte_t *__find_linux_pte(pgd_t *pgdir, unsigned long ea,
>  
>  	pdshift = PMD_SHIFT;
>  	pmdp = pmd_offset(&pud, ea);
> +#else
> +	p4dp = p4d_offset(pgdp, ea);
> +	pudp = pud_offset(p4dp, ea);
> +	pmdp = pmd_offset(pudp, ea);

I would drop a comment on top explaining that these are no-op for 32bits,
otherwise it might not be obvious to people as why this distiction between 64 and
32bits.

Other than that looks good to me
Christophe Leroy May 25, 2024, 6:41 a.m. UTC | #2
Le 25/05/2024 à 06:12, Oscar Salvador a écrit :
> On Fri, May 17, 2024 at 09:00:04PM +0200, Christophe Leroy wrote:
>> Building on 32 bits with pmd_leaf() not returning always false leads
>> to the following error:
> 
> I am curious though.
> pmd_leaf is only defined in include/linux/pgtable.h for 32bits, and is hardcoded
> to false.
> I do not see where we change it in previous patches, so is this artificial?

Patch 17 brings pmd_leaf()

> 
>>
>>    CC      arch/powerpc/mm/pgtable.o
>> arch/powerpc/mm/pgtable.c: In function '__find_linux_pte':
>> arch/powerpc/mm/pgtable.c:506:1: error: function may return address of local variable [-Werror=return-local-addr]
>>    506 | }
>>        | ^
>> arch/powerpc/mm/pgtable.c:394:15: note: declared here
>>    394 |         pud_t pud, *pudp;
>>        |               ^~~
>> arch/powerpc/mm/pgtable.c:394:15: note: declared here
>>
>> This is due to pmd_offset() being a no-op in that case.
> 
> This is because 32bits powerpc include pgtable-nopmd.h?
> 
>> So rework it for powerpc/32 so that pXd_offset() are used on real
>> pointers and not on on-stack copies.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   arch/powerpc/mm/pgtable.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
>> index 59f0d7706d2f..51ee508eeb5b 100644
>> --- a/arch/powerpc/mm/pgtable.c
>> +++ b/arch/powerpc/mm/pgtable.c
>> @@ -390,8 +390,12 @@ pte_t *__find_linux_pte(pgd_t *pgdir, unsigned long ea,
>>   			bool *is_thp, unsigned *hpage_shift)
>>   {
>>   	pgd_t *pgdp;
>> -	p4d_t p4d, *p4dp;
>> -	pud_t pud, *pudp;
>> +	p4d_t *p4dp;
>> +	pud_t *pudp;
>> +#ifdef CONFIG_PPC64
>> +	p4d_t p4d;
>> +	pud_t pud;
>> +#endif
>>   	pmd_t pmd, *pmdp;
>>   	pte_t *ret_pte;
>>   	hugepd_t *hpdp = NULL;
>> @@ -412,6 +416,7 @@ pte_t *__find_linux_pte(pgd_t *pgdir, unsigned long ea,
>>   	 */
>>   	pgdp = pgdir + pgd_index(ea);
>>   	p4dp = p4d_offset(pgdp, ea);
>> +#ifdef CONFIG_PPC64
>>   	p4d  = READ_ONCE(*p4dp);
>>   	pdshift = P4D_SHIFT;
>>   
>> @@ -452,6 +457,11 @@ pte_t *__find_linux_pte(pgd_t *pgdir, unsigned long ea,
>>   
>>   	pdshift = PMD_SHIFT;
>>   	pmdp = pmd_offset(&pud, ea);
>> +#else
>> +	p4dp = p4d_offset(pgdp, ea);
>> +	pudp = pud_offset(p4dp, ea);
>> +	pmdp = pmd_offset(pudp, ea);
> 
> I would drop a comment on top explaining that these are no-op for 32bits,
> otherwise it might not be obvious to people as why this distiction between 64 and
> 32bits.

Ok

> 
> Other than that looks good to me
> 
>   
>
diff mbox series

Patch

diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
index 59f0d7706d2f..51ee508eeb5b 100644
--- a/arch/powerpc/mm/pgtable.c
+++ b/arch/powerpc/mm/pgtable.c
@@ -390,8 +390,12 @@  pte_t *__find_linux_pte(pgd_t *pgdir, unsigned long ea,
 			bool *is_thp, unsigned *hpage_shift)
 {
 	pgd_t *pgdp;
-	p4d_t p4d, *p4dp;
-	pud_t pud, *pudp;
+	p4d_t *p4dp;
+	pud_t *pudp;
+#ifdef CONFIG_PPC64
+	p4d_t p4d;
+	pud_t pud;
+#endif
 	pmd_t pmd, *pmdp;
 	pte_t *ret_pte;
 	hugepd_t *hpdp = NULL;
@@ -412,6 +416,7 @@  pte_t *__find_linux_pte(pgd_t *pgdir, unsigned long ea,
 	 */
 	pgdp = pgdir + pgd_index(ea);
 	p4dp = p4d_offset(pgdp, ea);
+#ifdef CONFIG_PPC64
 	p4d  = READ_ONCE(*p4dp);
 	pdshift = P4D_SHIFT;
 
@@ -452,6 +457,11 @@  pte_t *__find_linux_pte(pgd_t *pgdir, unsigned long ea,
 
 	pdshift = PMD_SHIFT;
 	pmdp = pmd_offset(&pud, ea);
+#else
+	p4dp = p4d_offset(pgdp, ea);
+	pudp = pud_offset(p4dp, ea);
+	pmdp = pmd_offset(pudp, ea);
+#endif
 	pmd  = READ_ONCE(*pmdp);
 
 	/*