Re: [RFC PATCH v2 06/20] powerpc/8xx: Fix size given to set_huge_pte_at()
+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 ?
Re: [RFC PATCH v2 06/20] powerpc/8xx: Fix size given to set_huge_pte_at()
Le 20/05/2024 à 19:42, Oscar Salvador a écrit : > On Mon, May 20, 2024 at 04:31:39PM +, 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
Re: [RFC PATCH v2 06/20] powerpc/8xx: Fix size given to set_huge_pte_at()
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
Re: [RFC PATCH v2 06/20] powerpc/8xx: Fix size given to set_huge_pte_at()
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. -- Oscar Salvador SUSE Labs
Re: [RFC PATCH v2 06/20] powerpc/8xx: Fix size given to set_huge_pte_at()
Christophe Leroy 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
>>
>> Reviewed-by: Oscar Salvador
>>
>> 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
Re: [RFC PATCH v2 06/20] powerpc/8xx: Fix size given to set_huge_pte_at()
On Mon, May 20, 2024 at 04:31:39PM +, 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. -- Oscar Salvador SUSE Labs
Re: [RFC PATCH v2 06/20] powerpc/8xx: Fix size given to set_huge_pte_at()
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
>
> Reviewed-by: Oscar Salvador
>
> 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
>>
>
Re: [RFC PATCH v2 06/20] powerpc/8xx: Fix size given to set_huge_pte_at()
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
Reviewed-by: Oscar Salvador
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
>
--
Oscar Salvador
SUSE Labs
[RFC PATCH v2 06/20] powerpc/8xx: Fix size given to set_huge_pte_at()
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
---
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
