Re: [PATCH 1/3] powerpc/32: Fix hugepage allocation on 8xx at hint address

[Aneesh Kumar K.V]

On 01/16/2018 10:01 PM, Christophe LEROY wrote:
> > > diff --git a/arch/powerpc/include/asm/page_64.h 
> > > b/arch/powerpc/include/asm/page_64.h
> > > index 56234c6fcd61..a7baef5bbe5f 100644
> > > --- a/arch/powerpc/include/asm/page_64.h
> > > +++ b/arch/powerpc/include/asm/page_64.h
> > > @@ -91,30 +91,13 @@ extern u64 ppc64_pft_size;
> > > Â #define SLICE_LOW_SHIFTÂÂÂÂÂÂÂ 28
> > > Â #define SLICE_HIGH_SHIFTÂÂÂ 40
> > > -#define SLICE_LOW_TOPÂÂÂÂÂÂÂ (0x100000000ul)
> > > -#define SLICE_NUM_LOWÂÂÂÂÂÂÂ (SLICE_LOW_TOP >> SLICE_LOW_SHIFT)
> > > +#define SLICE_LOW_TOPÂÂÂÂÂÂÂ (0xfffffffful)
> > > +#define SLICE_NUM_LOWÂÂÂÂÂÂÂ ((SLICE_LOW_TOP >> SLICE_LOW_SHIFT) + 1)
> > > Â #define SLICE_NUM_HIGHÂÂÂÂÂÂÂ (H_PGTABLE_RANGE >> SLICE_HIGH_SHIFT)
> >
> >
> > Why are you changing this? is this a bug fix?
>
> That's because 0x100000000ul is out of range of unsigned long on PPC32.

Ok that detail was important. I missed that.

> > > Â #define GET_LOW_SLICE_INDEX(addr)ÂÂÂ ((addr) >> SLICE_LOW_SHIFT)
> > > Â #define GET_HIGH_SLICE_INDEX(addr)ÂÂÂ ((addr) >> SLICE_HIGH_SHIFT)
> > > -#ifndef __ASSEMBLY__
> > > -struct mm_struct;
> > > -
> > > -extern unsigned long slice_get_unmapped_area(unsigned long addr,
> > > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long len,
> > > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long flags,
> > > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned int psize,
> > > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ int topdown);
> > > -
> > > -extern unsigned int get_slice_psize(struct mm_struct *mm,
> > > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long addr);
> > > -
> > > -extern void slice_set_user_psize(struct mm_struct *mm, unsigned int 
> > > psize);
> > > -extern void slice_set_range_psize(struct mm_struct *mm, unsigned 
> > > long start,
> > > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long len, unsigned int psize);
> > > -
> > > -#endif /* __ASSEMBLY__ */
> > > Â #else
> > > Â #define slice_init()
> > > Â #ifdef CONFIG_PPC_BOOK3S_64
> > > diff --git a/arch/powerpc/kernel/setup-common.c 
> > > b/arch/powerpc/kernel/setup-common.c
> > > index 9d213542a48b..a285e1067713 100644
> > > --- a/arch/powerpc/kernel/setup-common.c
> > > +++ b/arch/powerpc/kernel/setup-common.c
> > > @@ -928,7 +928,7 @@ void __init setup_arch(char **cmdline_p)
> > > ÂÂÂÂÂ if (!radix_enabled())
> > > ÂÂÂÂÂÂÂÂÂ init_mm.context.slb_addr_limit = DEFAULT_MAP_WINDOW_USER64;
> > > Â #else
> > > -#errorÂÂÂ "context.addr_limit not initialized."
> > > +ÂÂÂ init_mm.context.slb_addr_limit = DEFAULT_MAP_WINDOW;
> > > Â #endif
> >
> >
> > May be put this within #ifdef 8XX and retain the error?
>
> Is this error really worth it ?
> I wanted to avoid spreading too many #ifdef PPC_8xx, but ok I can do that.
>
> > > Â #endif
> > > diff --git a/arch/powerpc/mm/8xx_mmu.c b/arch/powerpc/mm/8xx_mmu.c
> > > index f29212e40f40..0be77709446c 100644
> > > --- a/arch/powerpc/mm/8xx_mmu.c
> > > +++ b/arch/powerpc/mm/8xx_mmu.c
> > > @@ -192,7 +192,7 @@ void set_context(unsigned long id, pgd_t *pgd)
> > > ÂÂÂÂÂ mtspr(SPRN_M_TW, __pa(pgd) - offset);
> > > ÂÂÂÂÂ /* Update context */
> > > -ÂÂÂ mtspr(SPRN_M_CASID, id);
> > > +ÂÂÂ mtspr(SPRN_M_CASID, id - 1);
> > > ÂÂÂÂÂ /* sync */
> > > ÂÂÂÂÂ mb();
> > > Â }
> > > diff --git a/arch/powerpc/mm/hash_utils_64.c 
> > > b/arch/powerpc/mm/hash_utils_64.c
> > > index 655a5a9a183d..3266b3326088 100644
> > > --- a/arch/powerpc/mm/hash_utils_64.c
> > > +++ b/arch/powerpc/mm/hash_utils_64.c
> > > @@ -1101,7 +1101,7 @@ static unsigned int get_paca_psize(unsigned 
> > > long addr)
> > > ÂÂÂÂÂ unsigned char *hpsizes;
> > > ÂÂÂÂÂ unsigned long index, mask_index;
> > > -ÂÂÂ if (addr < SLICE_LOW_TOP) {
> > > +ÂÂÂ if (addr <= SLICE_LOW_TOP) {
> >
> > If this is part of bug fix, please do it as part of seperate patch 
> > with details
>
> As explained above, in order to allow comparison to work on PPC32, 
> SLICE_LOW_TOP has to be 0xffffffff instead of 0x100000000
>
> How should I split in separate patches ? Something like ?
> 1/ Slice support for PPC32 > 2/ Activate slice for 8xx

Yes something like that. Will you  be able to avoid that
 if (SLICE_NUM_HIGH) from the code? That makes the code ugly. Right now 
i don't have definite suggestion on what we could do though.


-aneesh