diff mbox series

[RFC,v2,06/20] powerpc/8xx: Fix size given to set_huge_pte_at()

Message ID 04f4e737608ea0b177b88057db138fbf0d6ab138.1715971869.git.christophe.leroy@csgroup.eu (mailing list archive)
State Superseded
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
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(-)

Comments

Oscar Salvador May 20, 2024, 9:14 a.m. UTC | #1
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
>
Christophe Leroy May 20, 2024, 4:31 p.m. UTC | #2
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
>>
>
Oscar Salvador May 20, 2024, 5:42 p.m. UTC | #3
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.
Michael Ellerman May 21, 2024, 12:48 a.m. UTC | #4
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
Oscar Salvador May 21, 2024, 9:26 a.m. UTC | #5
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.
Christophe Leroy May 22, 2024, 8:32 a.m. UTC | #6
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
Christophe Leroy May 22, 2024, 8:45 a.m. UTC | #7
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
Christophe Leroy May 22, 2024, 12:18 p.m. UTC | #8
+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 mbox series

Patch

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;
 }