Messages in this thread |  | | Subject | Re: [PATCH v2 1/5] powerpc/mm: Enhance 'slice' for supporting PPC32 | | From | Christophe LEROY <> | | Date | Fri, 19 Jan 2018 10:45:35 +0100 |
| |
Le 19/01/2018 à 10:13, Aneesh Kumar K.V a écrit : > > > On 01/19/2018 02:37 PM, Christophe LEROY wrote: >> >> >> Le 19/01/2018 à 10:02, Aneesh Kumar K.V a écrit : >>> >>> >>> On 01/19/2018 02:14 PM, Christophe LEROY wrote: >>>> >>>> >>>> Le 19/01/2018 à 09:24, Aneesh Kumar K.V a écrit : >>>>> Christophe Leroy <christophe.leroy@c-s.fr> writes: >>>>> >>>>>> In preparation for the following patch which will fix an issue on >>>>>> the 8xx by re-using the 'slices', this patch enhances the >>>>>> 'slices' implementation to support 32 bits CPUs. >>>>>> >>>>>> On PPC32, the address space is limited to 4Gbytes, hence only the low >>>>>> slices will be used. As of today, the code uses >>>>>> SLICE_LOW_TOP (0x100000000ul) and compares it with addr to determine >>>>>> if addr refers to low or high space. >>>>>> On PPC32, such a (addr < SLICE_LOW_TOP) test is always false because >>>>>> 0x100000000ul degrades to 0. Therefore, the patch modifies >>>>>> SLICE_LOW_TOP to (0xfffffffful) and modifies the tests to >>>>>> (addr <= SLICE_LOW_TOP) which will then always be true on PPC32 >>>>>> as addr has type 'unsigned long' while not modifying the PPC64 >>>>>> behaviour. >>>>>> >>>>>> This patch moves "slices" functions prototypes from page64.h to >>>>>> page.h >>>>>> >>>>>> The high slices use bitmaps. As bitmap functions are not prepared to >>>>>> handling bitmaps of size 0, the bitmap_xxx() calls are wrapped into >>>>>> slice_bitmap_xxx() macros which will take care of the 0 nbits case. >>>>>> >>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> >>>>>> --- >>>>>> v2: First patch of v1 serie split in two parts ; added >>>>>> slice_bitmap_xxx() macros. >>>>>> >>>>>> arch/powerpc/include/asm/page.h | 14 +++++++++ >>>>>> arch/powerpc/include/asm/page_32.h | 19 ++++++++++++ >>>>>> arch/powerpc/include/asm/page_64.h | 21 ++----------- >>>>>> arch/powerpc/mm/hash_utils_64.c | 2 +- >>>>>> arch/powerpc/mm/mmu_context_nohash.c | 7 +++++ >>>>>> arch/powerpc/mm/slice.c | 60 >>>>>> ++++++++++++++++++++++++------------ >>>>>> 6 files changed, 83 insertions(+), 40 deletions(-) >>>>>> >>>>>> diff --git a/arch/powerpc/include/asm/page.h >>>>>> b/arch/powerpc/include/asm/page.h >>>>>> index 8da5d4c1cab2..d0384f9db9eb 100644 >>>>>> --- a/arch/powerpc/include/asm/page.h >>>>>> +++ b/arch/powerpc/include/asm/page.h >>>>>> @@ -342,6 +342,20 @@ typedef struct page *pgtable_t; >>>>>> #endif >>>>>> #endif >>>>>> +#ifdef CONFIG_PPC_MM_SLICES >>>>>> +struct mm_struct; >>>>>> + >>>>>> +unsigned long slice_get_unmapped_area(unsigned long addr, >>>>>> unsigned long len, >>>>>> + unsigned long flags, unsigned int psize, >>>>>> + int topdown); >>>>>> + >>>>>> +unsigned int get_slice_psize(struct mm_struct *mm, unsigned long >>>>>> addr); >>>>>> + >>>>>> +void slice_set_user_psize(struct mm_struct *mm, unsigned int psize); >>>>>> +void slice_set_range_psize(struct mm_struct *mm, unsigned long >>>>>> start, >>>>>> + unsigned long len, unsigned int psize); >>>>>> +#endif >>>>>> + >>>>> >>>>> Should we do a slice.h ? the way we have other files? and then do >>>> >>>> Yes we could add a slice.h instead of using page.h for that, good idea. >>>> >>>>> >>>>> arch/powerpc/include/asm/book3s/64/slice.h that will carry >>>>> #define slice_bitmap_zero(dst, nbits) \ >>>>> do { if (nbits) bitmap_zero(dst, nbits); } while (0) >>>>> #define slice_bitmap_set(dst, pos, nbits) \ >>>>> do { if (nbits) bitmap_set(dst, pos, nbits); } while (0) >>>>> #define slice_bitmap_copy(dst, src, nbits) \ >>>>> do { if (nbits) bitmap_copy(dst, src, nbits); } while (0) >>>>> #define slice_bitmap_and(dst, src1, src2, nbits) \ >>>>> ({ (nbits) ? bitmap_and(dst, src1, src2, nbits) : 0; }) >>>>> #define slice_bitmap_or(dst, src1, src2, nbits) \ >>>>> do { if (nbits) bitmap_or(dst, src1, src2, nbits); } while (0) >>>>> #define slice_bitmap_andnot(dst, src1, src2, nbits) \ >>>>> ({ (nbits) ? bitmap_andnot(dst, src1, src2, nbits) : 0; }) >>>>> #define slice_bitmap_equal(src1, src2, nbits) \ >>>>> ({ (nbits) ? bitmap_equal(src1, src2, nbits) : 1; }) >>>>> #define slice_bitmap_empty(src, nbits) \ >>>>> ({ (nbits) ? bitmap_empty(src, nbits) : 1; }) >>>>> >>>>> This without that if(nbits) check and a proper static inline so >>>>> that we >>>>> can do type checking. >>>> >>>> Is it really worth duplicating that just for eliminating the 'if >>>> (nbits)' in one case ? >>>> >>>> Only in book3s/64 we will be able to eliminate that, for nohash/32 >>>> we need to keep the test due to the difference between low and high >>>> slices. >>> >>> the other advantage is we move the SLICE_LOW_SHIFT to the right >>> location. IMHO mm subystem is really complex with these really >>> overloaded headers. If we can keep it seperate we should with >>> minimal code duplication? >> >> For the constants I fully agree with your proposal and I will do it. I >> was only questionning the benefit of moving the slice_bitmap_xxxx() >> stuff, taking into account that the 'if (nbits)' test is already >> eliminated by the compiler. >> > > That is compiler dependent as you are finding with the other patch where > if (0) didn't get compiled out
I don't think so. When I had the missing prototype, the compilation goes ok, including the final link. Which means at the end the code is not included since radix_enabled() evaluates to 0.
Many many parts of the kernel are based on this assumption.
Christophe
> > -aneesh
|  |