| 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 |
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
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 --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); /*
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(-)