On 6/20/22 12:11, Christophe Leroy wrote:
>
>
> Le 20/06/2022 à 07:16, Anshuman Khandual a écrit :
>>
>>
>> On 6/16/22 11:05, Christophe Leroy wrote:
>>>
>>> Le 16/06/2022 à 06:09, Anshuman Khandual a écrit :
>>>> Restrict generic protection_map[] array visibility only for platforms which
>>>> do not enable ARCH_HAS_VM_GET_PAGE_PROT. For other platforms that do define
>>>> their own vm_get_page_prot() enabling ARCH_HAS_VM_GET_PAGE_PROT, could have
>>>> their private static protection_map[] still implementing an array look up.
>>>> These private protection_map[] array could do without __PXXX/__SXXX macros,
>>>> making them redundant and dropping them off as well.
>>>>
>>>> But platforms which do not define their custom vm_get_page_prot() enabling
>>>> ARCH_HAS_VM_GET_PAGE_PROT, will still have to provide __PXXX/__SXXX macros.
>>>>
>>>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>>>> Cc: linux-mm@xxxxxxxxx
>>>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>>>> Acked-by: Christoph Hellwig <hch@xxxxxx>
>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>>>> ---
>>>> arch/arm64/include/asm/pgtable-prot.h | 18 ------------------
>>>> arch/arm64/mm/mmap.c | 21 +++++++++++++++++++++
>>>> arch/powerpc/include/asm/pgtable.h | 2 ++
>>>> arch/powerpc/mm/book3s64/pgtable.c | 20 ++++++++++++++++++++
>>>> arch/sparc/include/asm/pgtable_64.h | 19 -------------------
>>>> arch/sparc/mm/init_64.c | 3 +++
>>>> arch/x86/include/asm/pgtable_types.h | 19 -------------------
>>>> arch/x86/mm/pgprot.c | 19 +++++++++++++++++++
>>>> include/linux/mm.h | 2 ++
>>>> mm/mmap.c | 2 +-
>>>> 10 files changed, 68 insertions(+), 57 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h
>>>> index d564d0ecd4cd..8ed2a80c896e 100644
>>>> --- a/arch/powerpc/include/asm/pgtable.h
>>>> +++ b/arch/powerpc/include/asm/pgtable.h
>>>> @@ -21,6 +21,7 @@ struct mm_struct;
>>>> #endif /* !CONFIG_PPC_BOOK3S */
>>>>
>>>> /* Note due to the way vm flags are laid out, the bits are XWR */
>>>> +#ifndef CONFIG_ARCH_HAS_VM_GET_PAGE_PROT
>>> This ifdef if not necessary for now, it doesn't matter if __P000 etc
>>> still exist thought not used.
>>>
>>>> #define __P000 PAGE_NONE
>>>> #define __P001 PAGE_READONLY
>>>> #define __P010 PAGE_COPY
>>>> @@ -38,6 +39,7 @@ struct mm_struct;
>>>> #define __S101 PAGE_READONLY_X
>>>> #define __S110 PAGE_SHARED_X
>>>> #define __S111 PAGE_SHARED_X
>>>> +#endif
>>>>
>>>> #ifndef __ASSEMBLY__
>>>>
>>>> diff --git a/arch/powerpc/mm/book3s64/pgtable.c b/arch/powerpc/mm/book3s64/pgtable.c
>>>> index 7b9966402b25..d3b019b95c1d 100644
>>>> --- a/arch/powerpc/mm/book3s64/pgtable.c
>>>> +++ b/arch/powerpc/mm/book3s64/pgtable.c
>>>> @@ -551,6 +551,26 @@ unsigned long memremap_compat_align(void)
>>>> EXPORT_SYMBOL_GPL(memremap_compat_align);
>>>> #endif
>>>>
>>>> +/* Note due to the way vm flags are laid out, the bits are XWR */
>>>> +static const pgprot_t protection_map[16] = {
>>>> + [VM_NONE] = PAGE_NONE,
>>>> + [VM_READ] = PAGE_READONLY,
>>>> + [VM_WRITE] = PAGE_COPY,
>>>> + [VM_WRITE | VM_READ] = PAGE_COPY,
>>>> + [VM_EXEC] = PAGE_READONLY_X,
>>>> + [VM_EXEC | VM_READ] = PAGE_READONLY_X,
>>>> + [VM_EXEC | VM_WRITE] = PAGE_COPY_X,
>>>> + [VM_EXEC | VM_WRITE | VM_READ] = PAGE_COPY_X,
>>>> + [VM_SHARED] = PAGE_NONE,
>>>> + [VM_SHARED | VM_READ] = PAGE_READONLY,
>>>> + [VM_SHARED | VM_WRITE] = PAGE_SHARED,
>>>> + [VM_SHARED | VM_WRITE | VM_READ] = PAGE_SHARED,
>>>> + [VM_SHARED | VM_EXEC] = PAGE_READONLY_X,
>>>> + [VM_SHARED | VM_EXEC | VM_READ] = PAGE_READONLY_X,
>>>> + [VM_SHARED | VM_EXEC | VM_WRITE] = PAGE_SHARED_X,
>>>> + [VM_SHARED | VM_EXEC | VM_WRITE | VM_READ] = PAGE_SHARED_X
>>>> +};
>>>> +
>>> There is not much point is first additing that here and then move it
>>> elsewhere in the second patch.
>>>
>>> I think with my suggestion to use #ifdef __P000 as a guard, the powerpc
>>> changes could go in a single patch.
>>>
>>>> pgprot_t vm_get_page_prot(unsigned long vm_flags)
>>>> {
>>>> unsigned long prot = pgprot_val(protection_map[vm_flags &
>>>> diff --git a/mm/mmap.c b/mm/mmap.c
>>>> index 61e6135c54ef..e66920414945 100644
>>>> --- a/mm/mmap.c
>>>> +++ b/mm/mmap.c
>>>> @@ -101,6 +101,7 @@ static void unmap_region(struct mm_struct *mm,
>>>> * w: (no) no
>>>> * x: (yes) yes
>>>> */
>>>> +#ifndef CONFIG_ARCH_HAS_VM_GET_PAGE_PROT
>>> You should use #ifdef __P000 instead, that way you could migrate
>>> architectures one by one.
>>
>> If vm_get_page_prot() gets moved into all platforms, wondering what would be
>> the preferred method to organize this patch series ?
>>
>> 1. Move protection_map[] inside platforms with ARCH_HAS_VM_PAGE_PROT (current patch 1)
>> 2. Convert remaining platforms to use ARCH_HAS_VM_PAGE_PROT one after the other
>> 3. Drop ARCH_HAS_VM_PAGE_PROT completely
>>
>> Using "#ifdef __P000" for wrapping protection_map[] will leave two different #ifdefs
>> in flight i.e __P000, ARCH_HAS_VM_PAGE_PROT in the generic mmap code, until both gets
>> dropped eventually. But using "#ifdef __P000" does enable splitting the first patch
>> into multiple changes for each individual platforms.
>
> From previous discussions and based on Christoph's suggestion, I guess
> we now aim at getting vm_get_page_prot() moved into all platforms
> together with protection_map[]. Therefore the use of #ifdef __P000 could
> be very temporary at the begining of the series:
> 1. Guard generic protection_map[] with #ifdef ___P000
> 2. Move protection_map[] into architecture and drop __Pxxx/__Sxxx for arm64
> 3. Same for sparc
> 4. Same for x86
> 5. Convert entire powerpc to ARCH_HAS_VM_PAGE_PROT and move
> protection_map[] into architecture and drop __Pxxx/__Sxxx
> 6. Replace #ifdef __P000 by #ifdef CONFIG_ARCH_HAS_VM_PAGE_PROT
> 7. Convert all remaining platforms to CONFIG_ARCH_HAS_VM_PAGE_PROT one
> by one (but keep a protection_map[] table, don't use switch/case)
> 8. Drop ARCH_HAS_VM_PAGE_PROT completely.
>
> Eventually you can squash step 6 into step 8.
Keeping individual platform changes in a separate patch will make
the series cleaner, and also much easier to review. But the flow
explained above sounds good to me. I will work on these changes.
