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 <[email protected]>
> ---
> 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
--
Oscar Salvador
SUSE Labs
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 <[email protected]>
>> ---
>> 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
>
>
>