| Message ID | 04f4e737608ea0b177b88057db138fbf0d6ab138.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:00PM +0200, Christophe Leroy wrote: > set_huge_pte_at() expects the real page size, not the psize which is "expects the size of the huge page" sounds bettter? > the index of the page definition in table mmu_psize_defs[] > > Fixes: 935d4f0c6dc8 ("mm: hugetlb: add huge page size param to set_huge_pte_at()") > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> Reviewed-by: Oscar Salvador <osalvador@suse.de> AFAICS, this fixup is not related to the series, right? (yes, you will the parameter later) I would have it at the very beginning of the series. > --- > arch/powerpc/mm/nohash/8xx.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/mm/nohash/8xx.c b/arch/powerpc/mm/nohash/8xx.c > index 43d4842bb1c7..d93433e26ded 100644 > --- a/arch/powerpc/mm/nohash/8xx.c > +++ b/arch/powerpc/mm/nohash/8xx.c > @@ -94,7 +94,8 @@ static int __ref __early_map_kernel_hugepage(unsigned long va, phys_addr_t pa, > return -EINVAL; > > set_huge_pte_at(&init_mm, va, ptep, > - pte_mkhuge(pfn_pte(pa >> PAGE_SHIFT, prot)), psize); > + pte_mkhuge(pfn_pte(pa >> PAGE_SHIFT, prot)), > + 1UL << mmu_psize_to_shift(psize)); > > return 0; > } > -- > 2.44.0 >
Hi Oscar, hi Michael, Le 20/05/2024 à 11:14, Oscar Salvador a écrit : > On Fri, May 17, 2024 at 09:00:00PM +0200, Christophe Leroy wrote: >> set_huge_pte_at() expects the real page size, not the psize which is > > "expects the size of the huge page" sounds bettter? Parameter 'pzize' already provides the size of the hugepage, but not in the way set_huge_pte_at() expects it. psize has one of the values defined by MMU_PAGE_XXX macros defined in arch/powerpc/include/asm/mmu.h while set_huge_pte_at() expects the size as a value. > >> the index of the page definition in table mmu_psize_defs[] >> >> Fixes: 935d4f0c6dc8 ("mm: hugetlb: add huge page size param to set_huge_pte_at()") >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > > Reviewed-by: Oscar Salvador <osalvador@suse.de> > > AFAICS, this fixup is not related to the series, right? (yes, you will > the parameter later) > I would have it at the very beginning of the series. You are right, I should have submitted it separately. Michael can you take it as a fix for 6.10 ? > > >> --- >> arch/powerpc/mm/nohash/8xx.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/mm/nohash/8xx.c b/arch/powerpc/mm/nohash/8xx.c >> index 43d4842bb1c7..d93433e26ded 100644 >> --- a/arch/powerpc/mm/nohash/8xx.c >> +++ b/arch/powerpc/mm/nohash/8xx.c >> @@ -94,7 +94,8 @@ static int __ref __early_map_kernel_hugepage(unsigned long va, phys_addr_t pa, >> return -EINVAL; >> >> set_huge_pte_at(&init_mm, va, ptep, >> - pte_mkhuge(pfn_pte(pa >> PAGE_SHIFT, prot)), psize); >> + pte_mkhuge(pfn_pte(pa >> PAGE_SHIFT, prot)), >> + 1UL << mmu_psize_to_shift(psize)); >> >> return 0; >> } >> -- >> 2.44.0 >> >
On Mon, May 20, 2024 at 04:31:39PM +0000, Christophe Leroy wrote: > Hi Oscar, hi Michael, > > Le 20/05/2024 à 11:14, Oscar Salvador a écrit : > > On Fri, May 17, 2024 at 09:00:00PM +0200, Christophe Leroy wrote: > >> set_huge_pte_at() expects the real page size, not the psize which is > > > > "expects the size of the huge page" sounds bettter? > > Parameter 'pzize' already provides the size of the hugepage, but not in > the way set_huge_pte_at() expects it. > > psize has one of the values defined by MMU_PAGE_XXX macros defined in > arch/powerpc/include/asm/mmu.h while set_huge_pte_at() expects the size > as a value. Yes, psize is an index, which is not a size by itself but used to get mmu_psize_def.shift to see the actual size, I guess. This is why I thought that being explicit about "expects the size of the huge page" was better. But no strong feelings here.
Christophe Leroy <christophe.leroy@csgroup.eu> writes: > Hi Oscar, hi Michael, > > Le 20/05/2024 à 11:14, Oscar Salvador a écrit : >> On Fri, May 17, 2024 at 09:00:00PM +0200, Christophe Leroy wrote: >>> set_huge_pte_at() expects the real page size, not the psize which is >> >> "expects the size of the huge page" sounds bettter? > > Parameter 'pzize' already provides the size of the hugepage, but not in > the way set_huge_pte_at() expects it. > > psize has one of the values defined by MMU_PAGE_XXX macros defined in > arch/powerpc/include/asm/mmu.h while set_huge_pte_at() expects the size > as a value. > >> >>> the index of the page definition in table mmu_psize_defs[] >>> >>> Fixes: 935d4f0c6dc8 ("mm: hugetlb: add huge page size param to set_huge_pte_at()") >>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> >> Reviewed-by: Oscar Salvador <osalvador@suse.de> >> >> AFAICS, this fixup is not related to the series, right? (yes, you will >> the parameter later) >> I would have it at the very beginning of the series. > > You are right, I should have submitted it separately. > > Michael can you take it as a fix for 6.10 ? Yeah I can. Does it actually cause a bug at runtime (I assume so)? cheers
On Tue, May 21, 2024 at 10:48:21AM +1000, Michael Ellerman wrote:
> Yeah I can. Does it actually cause a bug at runtime (I assume so)?
No, currently set_huge_pte_at() from 8xx ignores the 'sz' parameter.
But it will be used after this series.
Le 21/05/2024 à 11:26, Oscar Salvador a écrit : > On Tue, May 21, 2024 at 10:48:21AM +1000, Michael Ellerman wrote: >> Yeah I can. Does it actually cause a bug at runtime (I assume so)? > > No, currently set_huge_pte_at() from 8xx ignores the 'sz' parameter. > But it will be used after this series. > Ah yes, I mixed things up with something else in my mind. So this patch doesn't qualify as a fix and doesn't need to be handled separately from the series and doesn't really need to go on top of the series either, I think it is better to keep it grouped with other 8xx changes. Christophe
Le 20/05/2024 à 19:42, Oscar Salvador a écrit : > On Mon, May 20, 2024 at 04:31:39PM +0000, Christophe Leroy wrote: >> Hi Oscar, hi Michael, >> >> Le 20/05/2024 à 11:14, Oscar Salvador a écrit : >>> On Fri, May 17, 2024 at 09:00:00PM +0200, Christophe Leroy wrote: >>>> set_huge_pte_at() expects the real page size, not the psize which is >>> >>> "expects the size of the huge page" sounds bettter? >> >> Parameter 'pzize' already provides the size of the hugepage, but not in >> the way set_huge_pte_at() expects it. >> >> psize has one of the values defined by MMU_PAGE_XXX macros defined in >> arch/powerpc/include/asm/mmu.h while set_huge_pte_at() expects the size >> as a value. > > Yes, psize is an index, which is not a size by itself but used to get > mmu_psize_def.shift to see the actual size, I guess. > This is why I thought that being explicit about "expects the size of the > huge page" was better. > > But no strong feelings here. > Thanks, I'll try a rephrase. Christophe
+Peter Z. who added that commit. Le 22/05/2024 à 10:32, Christophe Leroy a écrit : > > > Le 21/05/2024 à 11:26, Oscar Salvador a écrit : >> On Tue, May 21, 2024 at 10:48:21AM +1000, Michael Ellerman wrote: >>> Yeah I can. Does it actually cause a bug at runtime (I assume so)? >> >> No, currently set_huge_pte_at() from 8xx ignores the 'sz' parameter. >> But it will be used after this series. >> > > Ah yes, I mixed things up with something else in my mind. > > So this patch doesn't qualify as a fix and doesn't need to be handled > separately from the series and doesn't really need to go on top of the > series either, I think it is better to keep it grouped with other 8xx > changes. > I remember now, what I had in mind was commit c5eecbb58f65 ("powerpc/8xx: Implement pXX_leaf_size() support") That commit is buggy, because pgd_leaf() will always return false on 8xx. First of all pgd_leaf() could only return true on a target with P4Ds. Without P4Ds it should just return 0 like pgd_none(), pgd_bad(), ... as defined in include/asm-generic/pgtable-nop4d.h So it is pmd_leaf_size() that could eventually return something for 8xx. But as 8xx is using hugepd, at the best case it will return crap, worst case the read will go in the weed. To be correct we should had support of hugepd in perf_get_pgtable_size() but that's not trivial and this series is aiming at removing hugepd completely so there is no point in fixing stuff here, except maybe for stable ?
diff --git a/arch/powerpc/mm/nohash/8xx.c b/arch/powerpc/mm/nohash/8xx.c index 43d4842bb1c7..d93433e26ded 100644 --- a/arch/powerpc/mm/nohash/8xx.c +++ b/arch/powerpc/mm/nohash/8xx.c @@ -94,7 +94,8 @@ static int __ref __early_map_kernel_hugepage(unsigned long va, phys_addr_t pa, return -EINVAL; set_huge_pte_at(&init_mm, va, ptep, - pte_mkhuge(pfn_pte(pa >> PAGE_SHIFT, prot)), psize); + pte_mkhuge(pfn_pte(pa >> PAGE_SHIFT, prot)), + 1UL << mmu_psize_to_shift(psize)); return 0; }
set_huge_pte_at() expects the real page size, not the psize which is the index of the page definition in table mmu_psize_defs[] Fixes: 935d4f0c6dc8 ("mm: hugetlb: add huge page size param to set_huge_pte_at()") Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- arch/powerpc/mm/nohash/8xx.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)