Re: [PATCH] mm/ptdump: Drop GENERIC_PTDUMP

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 12/17/24 13:17, Christophe Leroy wrote:
> 
> 
> Le 17/12/2024 à 04:48, Anshuman Khandual a écrit :
>> GENERIC_PTDUMP does not guard any code but instead just used for platform's
>> subscription into core ptdump defined under PTDUMP_CORE, which is selected.
>> Instead use PTDUMP_CORE for platform subscription and drop GENERIC_PTDUMP.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Jonathan Corbet <corbet@lwn.net>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: Nicholas Piggin <npiggin@gmail.com>
>> Cc: Paul Walmsley <paul.walmsley@sifive.com>
>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
>> Cc: Heiko Carstens <hca@linux.ibm.com>
>> Cc: Vasily Gorbik <gor@linux.ibm.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-doc@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: kvmarm@lists.linux.dev
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Cc: linux-riscv@lists.infradead.org
>> Cc: linux-s390@vger.kernel.org
>> Cc: linux-mm@kvack.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>> This patch applies on v6.13-rc3 and has been tested on arm64, although it
>> also clears build tests on impacted platforms.
>>
>>   Documentation/arch/arm64/ptdump.rst       | 1 -
>>   arch/arm64/Kconfig                        | 2 +-
>>   arch/arm64/kvm/Kconfig                    | 3 +--
>>   arch/powerpc/Kconfig                      | 2 +-
>>   arch/powerpc/configs/mpc885_ads_defconfig | 1 -
>>   arch/riscv/Kconfig                        | 2 +-
>>   arch/s390/Kconfig                         | 2 +-
>>   arch/x86/Kconfig                          | 2 +-
>>   arch/x86/Kconfig.debug                    | 2 +-
>>   kernel/configs/debug.config               | 1 -
>>   mm/Kconfig.debug                          | 8 ++------
>>   11 files changed, 9 insertions(+), 17 deletions(-)
>>
> 
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index a0ce777f9706..c716f8df10de 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -203,7 +203,7 @@ config PPC
>>       select GENERIC_IRQ_SHOW
>>       select GENERIC_IRQ_SHOW_LEVEL
>>       select GENERIC_PCI_IOMAP        if PCI
>> -    select GENERIC_PTDUMP
>> +    select PTDUMP_CORE
> 
> Please keep alphabetical order.

Sure, will do that.

> 
>>       select GENERIC_SMP_IDLE_THREAD
>>       select GENERIC_TIME_VSYSCALL
>>       select GENERIC_VDSO_TIME_NS
> 
>> diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
>> index 41a58536531d..b206e5a11f96 100644
>> --- a/mm/Kconfig.debug
>> +++ b/mm/Kconfig.debug
>> @@ -187,7 +187,7 @@ config DEBUG_WX
>>       bool "Warn on W+X mappings at boot"
>>       depends on ARCH_HAS_DEBUG_WX
>>       depends on MMU
>> -    select PTDUMP_CORE
>> +    depends on PTDUMP_CORE
> 
> This change is unclear to me. It works because all arch selecting ARCH_HAS_DEBUG_WX also select GENERIC_PTDUMP. For riscv it's even more tricking, Riscv only selects GENERIC_PTDUMP when MMU is set, so it works because here it also depends on MMU.

Right, all the archs selecting ARCH_HAS_DEBUG_WX also select PTDUMP_CORE.
DEBUG_WX dependency on PTDUMP_CORE will always be met, when the platform
selects ARCH_HAS_DEBUG_WX as expected.

git grep "select PTDUMP_CORE" arch/
arch/arm64/Kconfig:     select PTDUMP_CORE
arch/powerpc/Kconfig:   select PTDUMP_CORE
arch/riscv/Kconfig:     select PTDUMP_CORE if MMU
arch/s390/Kconfig:      select PTDUMP_CORE
arch/x86/Kconfig:       select PTDUMP_CORE

git grep ARCH_HAS_DEBUG_WX arch/
arch/arm64/Kconfig:     select ARCH_HAS_DEBUG_WX
arch/powerpc/Kconfig:   select ARCH_HAS_DEBUG_WX                if STRICT_KERNEL_RWX
arch/riscv/Kconfig:     select ARCH_HAS_DEBUG_WX
arch/s390/Kconfig:      select ARCH_HAS_DEBUG_WX
arch/x86/Kconfig:       select ARCH_HAS_DEBUG_WX

On riscv platform, does not PTDUMP_CORE implicitly imply existence of MMU ?
Could there be a page table dumper without a MMU.

> 
> But what happens if in the future an architecture selects ARCH_HAS_DEBUG_WX without selecting PTDUMP_CORE ?

Should DEBUG_WX just select PTDUMP_CORE as it exists now. A platform
will be able to get PTDUMP_CORE via DEBUG_WX if ARCH_HAS_DEBUG_WX is
subscribed into. Would that be better ?

> 
> 
>>       help
>>         Generate a warning if any W+X mappings are found at boot.
>>   @@ -212,9 +212,6 @@ config DEBUG_WX
>>           If in doubt, say "Y".
>>   -config GENERIC_PTDUMP
>> -    bool
>> -
>>   config PTDUMP_CORE
>>       bool
>>   @@ -222,8 +219,7 @@ config PTDUMP_DEBUGFS
>>       bool "Export kernel pagetable layout to userspace via debugfs"
>>       depends on DEBUG_KERNEL
>>       depends on DEBUG_FS
>> -    depends on GENERIC_PTDUMP
>> -    select PTDUMP_CORE
>> +    depends on PTDUMP_CORE
>>       help
>>         Say Y here if you want to show the kernel pagetable layout in a
>>         debugfs file. This information is only useful for kernel developers
>