From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D9F29C35240 for ; Thu, 30 Jan 2020 13:04:34 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 8DF582173E for ; Thu, 30 Jan 2020 13:04:34 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8DF582173E Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 2A54C6B034B; Thu, 30 Jan 2020 08:04:34 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 233386B034C; Thu, 30 Jan 2020 08:04:34 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 082C76B034D; Thu, 30 Jan 2020 08:04:34 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0042.hostedemail.com [216.40.44.42]) by kanga.kvack.org (Postfix) with ESMTP id DFAD06B034B for ; Thu, 30 Jan 2020 08:04:33 -0500 (EST) Received: from smtpin03.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 979AF2C8B for ; Thu, 30 Jan 2020 13:04:33 +0000 (UTC) X-FDA: 76434319626.03.aunt55_44c70226a8455 X-HE-Tag: aunt55_44c70226a8455 X-Filterd-Recvd-Size: 13316 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf49.hostedemail.com (Postfix) with ESMTP for ; Thu, 30 Jan 2020 13:04:32 +0000 (UTC) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 726D7328; Thu, 30 Jan 2020 05:04:31 -0800 (PST) Received: from [192.168.0.129] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 818EF3F68E; Thu, 30 Jan 2020 05:04:18 -0800 (PST) From: Anshuman Khandual Subject: Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers To: Christophe Leroy , linux-mm@kvack.org Cc: Andrew Morton , Vlastimil Babka , Greg Kroah-Hartman , Thomas Gleixner , Mike Rapoport , Jason Gunthorpe , Dan Williams , Peter Zijlstra , Michal Hocko , Mark Rutland , Mark Brown , Steven Price , Ard Biesheuvel , Masahiro Yamada , Kees Cook , Tetsuo Handa , Matthew Wilcox , Sri Krishna chowdary , Dave Hansen , Russell King - ARM Linux , Michael Ellerman , Paul Mackerras , Martin Schwidefsky , Heiko Carstens , "David S. Miller" , Vineet Gupta , James Hogan , Paul Burton , Ralf Baechle , "Kirill A . Shutemov" , Gerald Schaefer , Ingo Molnar , linux-snps-arc@lists.infradead.org, linux-mips@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-ia64@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-s390@vger.kernel.org, linux-sh@vger.kernel.org, sparclinux@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org References: <1580174873-18117-1-git-send-email-anshuman.khandual@arm.com> <68ed6488-aa25-ab41-8da6-f0ddeb15d52b@c-s.fr> Message-ID: <49754f74-53a7-0e4a-bb16-53617f8c902c@arm.com> Date: Thu, 30 Jan 2020 18:34:14 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <68ed6488-aa25-ab41-8da6-f0ddeb15d52b@c-s.fr> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 01/28/2020 10:35 PM, Christophe Leroy wrote: >=20 >=20 > Le 28/01/2020 =C3=A0 02:27, Anshuman Khandual a =C3=A9crit=C2=A0: >> This adds tests which will validate architecture page table helpers an= d >> other accessors in their compliance with expected generic MM semantics= . >> This will help various architectures in validating changes to existing >> page table helpers or addition of new ones. >> >> This test covers basic page table entry transformations including but = not >> limited to old, young, dirty, clean, write, write protect etc at vario= us >> level along with populating intermediate entries with next page table = page >> and validating them. >> >> Test page table pages are allocated from system memory with required s= ize >> and alignments. The mapped pfns at page table levels are derived from = a >> real pfn representing a valid kernel text symbol. This test gets calle= d >> right after page_alloc_init_late(). >> >> This gets build and run when CONFIG_DEBUG_VM_PGTABLE is selected along= with >> CONFIG_VM_DEBUG. Architectures willing to subscribe this test also nee= d to >> select CONFIG_ARCH_HAS_DEBUG_VM_PGTABLE which for now is limited to x8= 6 and >> arm64. Going forward, other architectures too can enable this after fi= xing >> build or runtime problems (if any) with their page table helpers. >> >> Folks interested in making sure that a given platform's page table hel= pers >> conform to expected generic MM semantics should enable the above confi= g >> which will just trigger this test during boot. Any non conformity here= will >> be reported as an warning which would need to be fixed. This test will= help >> catch any changes to the agreed upon semantics expected from generic M= M and >> enable platforms to accommodate it thereafter. >> >=20 > [...] >=20 >> >> Tested-by: Christophe Leroy =C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 #PPC32 >=20 > Also tested on PPC64 (under QEMU): book3s/64 64k pages, book3s/64 4k pa= ges and book3e/64 Hmm but earlier Michael Ellerman had reported some problems while running these tests on PPC64, a soft lock up in hash__pte_update() and a kernel BUG (radix MMU). Are those problems gone away now ? Details in this thread - https://patchwork.kernel.org/patch/11214603/ >=20 >> Reviewed-by: Ingo Molnar >> Suggested-by: Catalin Marinas >> Signed-off-by: Andrew Morton >> Signed-off-by: Christophe Leroy >> Signed-off-by: Anshuman Khandual >> --- >=20 > [...] >=20 >> >> diff --git a/Documentation/features/debug/debug-vm-pgtable/arch-suppor= t.txt b/Documentation/features/debug/debug-vm-pgtable/arch-support.txt >> new file mode 100644 >> index 000000000000..f3f8111edbe3 >> --- /dev/null >> +++ b/Documentation/features/debug/debug-vm-pgtable/arch-support.txt >> @@ -0,0 +1,35 @@ >> +# >> +# Feature name:=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= debug-vm-pgtable >> +#=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Kconfig:=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 ARCH_HAS_DEBUG_VM_PGTABLE >> +#=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 description:=C2=A0=C2= =A0 arch supports pgtable tests for semantics compliance >> +# >> +=C2=A0=C2=A0=C2=A0 ----------------------- >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 = arch |status| >> +=C2=A0=C2=A0=C2=A0 ----------------------- >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 alpha: | TOD= O | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 = arc: |=C2=A0 ok=C2=A0 | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 = arm: | TODO | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 arm64: |=C2=A0= ok=C2=A0 | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 = c6x: | TODO | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 csky: = | TODO | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 h8300: | TOD= O | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0 hexagon: | TODO | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ia64: = | TODO | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 m68k: = | TODO | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0 microblaze: | TODO | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mips: = | TODO | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 nds32: | TOD= O | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 nios2: | TOD= O | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0 openrisc: | TODO | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 parisc: | TODO | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0 powerpc/32: |=C2=A0 ok=C2=A0 | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0 powerpc/64: | TODO | >=20 > You can change the two above lines by >=20 > =C2=A0=C2=A0=C2=A0=C2=A0powerpc: ok > >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 riscv: | TOD= O | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 s390: = | TODO | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 sh: | TODO | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sparc: | TOD= O | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 um: | TODO | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0 unicore32: | TODO | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 = x86: |=C2=A0 ok=C2=A0 | >> +=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 xtensa: | TODO | >> +=C2=A0=C2=A0=C2=A0 ----------------------- >=20 >> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig >> index 1ec34e16ed65..253dcab0bebc 100644 >> --- a/arch/powerpc/Kconfig >> +++ b/arch/powerpc/Kconfig >> @@ -120,6 +120,7 @@ config PPC >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 # >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 select ARCH_32BIT_OFF_T if PPC32 >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 select ARCH_HAS_DEBUG_VIRTUAL >> +=C2=A0=C2=A0=C2=A0 select ARCH_HAS_DEBUG_VM_PGTABLE if PPC32 >=20 > Remove the 'if PPC32' as we now know it also work on PPC64. But in case there is a subset of PPC64 which still does not work (problem reported earlier) with the test, will have to adjust the config accordingly. >=20 >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 select ARCH_HAS_DEVMEM_IS_ALLOWED >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 select ARCH_HAS_ELF_RANDOMIZE >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 select ARCH_HAS_FORTIFY_SOURCE >=20 >> diff --git a/arch/x86/include/asm/pgtable_64.h b/arch/x86/include/asm/= pgtable_64.h >> index 0b6c4042942a..fb0e76d254b3 100644 >> --- a/arch/x86/include/asm/pgtable_64.h >> +++ b/arch/x86/include/asm/pgtable_64.h >> @@ -53,6 +53,12 @@ static inline void sync_initial_page_table(void) { = } >> =C2=A0 =C2=A0 struct mm_struct; >> =C2=A0 +#define mm_p4d_folded mm_p4d_folded >> +static inline bool mm_p4d_folded(struct mm_struct *mm) >> +{ >> +=C2=A0=C2=A0=C2=A0 return !pgtable_l5_enabled(); >> +} >> + >=20 > For me this should be part of another patch, it is not directly linked = to the tests. We did discuss about this earlier and Kirril mentioned its not worth a separate patch. https://lore.kernel.org/linux-arm-kernel/20190913091305.rkds4f3fqv3yjhjy@= box/ >=20 >> =C2=A0 void set_pte_vaddr_p4d(p4d_t *p4d_page, unsigned long vaddr, pt= e_t new_pte); >> =C2=A0 void set_pte_vaddr_pud(pud_t *pud_page, unsigned long vaddr, pt= e_t new_pte); >> =C2=A0 diff --git a/include/asm-generic/pgtable.h b/include/asm-generi= c/pgtable.h >> index 798ea36a0549..e0b04787e789 100644 >> --- a/include/asm-generic/pgtable.h >> +++ b/include/asm-generic/pgtable.h >> @@ -1208,6 +1208,12 @@ static inline bool arch_has_pfn_modify_check(vo= id) >> =C2=A0 # define PAGE_KERNEL_EXEC PAGE_KERNEL >> =C2=A0 #endif >> =C2=A0 +#ifdef CONFIG_DEBUG_VM_PGTABLE >=20 > Not sure it is a good idea to put that in include/asm-generic/pgtable.h Logically that is the right place, as it is related to page table but not something platform related. >=20 > By doing this you are forcing a rebuild of almost all files, whereas on= ly init/main.o and mm/debug_vm_pgtable.o should be rebuilt when activatin= g this config option. I agreed but whats the alternative ? We could move these into init/main.c to make things simpler but will that be a right place, given its related to generic page table. >=20 >> +extern void debug_vm_pgtable(void); >=20 > Please don't use the 'extern' keyword, it is useless and not to be used= for functions declaration. Really ? But, there are tons of examples doing the same thing both in generic and platform code as well. >=20 >> +#else >> +static inline void debug_vm_pgtable(void) { } >> +#endif >> + >> =C2=A0 #endif /* !__ASSEMBLY__ */ >> =C2=A0 =C2=A0 #ifndef io_remap_pfn_range >> diff --git a/init/main.c b/init/main.c >> index da1bc0b60a7d..5e59e6ac0780 100644 >> --- a/init/main.c >> +++ b/init/main.c >> @@ -1197,6 +1197,7 @@ static noinline void __init kernel_init_freeable= (void) >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sched_init_smp(); >> =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 page_alloc_init_late(); >> +=C2=A0=C2=A0=C2=A0 debug_vm_pgtable(); >=20 > Wouldn't it be better to call debug_vm_pgtable() in kernel_init() betwe= en the call to async_synchronise_full() and ftrace_free_init_mem() ? IIRC, proposed location is the earliest we could call debug_vm_pgtable(). Is there any particular benefit or reason to move it into kernel_init() ? >=20 >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* Initialize page ext after all struct= pages are initialized. */ >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 page_ext_init(); >> =C2=A0 diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug >> index 5ffe144c9794..7cceae923c05 100644 >> --- a/lib/Kconfig.debug >> +++ b/lib/Kconfig.debug >> @@ -653,6 +653,12 @@ config SCHED_STACK_END_CHECK >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data corruption or a sporad= ic crash at a later stage once the region >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 is examined. The runtime ov= erhead introduced is minimal. >> =C2=A0 +config ARCH_HAS_DEBUG_VM_PGTABLE >> +=C2=A0=C2=A0=C2=A0 bool >> +=C2=A0=C2=A0=C2=A0 help >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 An architecture should select this whe= n it can successfully >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 build and run DEBUG_VM_PGTABLE. >> + >> =C2=A0 config DEBUG_VM >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 bool "Debug VM" >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 depends on DEBUG_KERNEL >> @@ -688,6 +694,22 @@ config DEBUG_VM_PGFLAGS >> =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 If unsure, say N. >> =C2=A0 +config DEBUG_VM_PGTABLE >> +=C2=A0=C2=A0=C2=A0 bool "Debug arch page table for semantics complian= ce" >> +=C2=A0=C2=A0=C2=A0 depends on MMU >> +=C2=A0=C2=A0=C2=A0 depends on DEBUG_VM >=20 > Does it really need to depend on DEBUG_VM ? No. It seemed better to package this test along with DEBUG_VM (although I dont remember the conversation around it) and hence this dependency. > I think we could make it standalone and 'default y if DEBUG_VM' instead= . Which will yield the same result like before but in a different way. But yes, this test could go about either way but unless there is a good enoug= h reason why change the current one. >=20 >> +=C2=A0=C2=A0=C2=A0 depends on ARCH_HAS_DEBUG_VM_PGTABLE >> +=C2=A0=C2=A0=C2=A0 default y >> +=C2=A0=C2=A0=C2=A0 help >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 This option provides a debug method wh= ich can be used to test >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 architecture page table helper functio= ns on various platforms in >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 verifying if they comply with expected= generic MM semantics. This >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 will help architecture code in making = sure that any changes or >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 new additions of these helpers still c= onform to expected >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 semantics of the generic MM. >> + >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 If unsure, say N. >> + >=20 > Does it make sense to make it 'default y' and say 'If unsure, say N' ? No it does. Not when it defaults 'y' unconditionally. Will drop the last sentence "If unsure, say N". Nice catch, thank you. >=20 >> =C2=A0 config ARCH_HAS_DEBUG_VIRTUAL >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 bool >> =C2=A0=20 >=20 > Christophe >=20