Re: [PATCH 1/1] mm/pgtable/debug: Add test validating architecture page table helpers

From: Christophe Leroy
Date: Tue Sep 10 2019 - 00:45:42 EST

Next message: Sandro Volery: "Re: [PATCH] Staging: gasket: Use temporaries to reduce line length."
Previous message: Mike Travis: "[PATCH 1/8] x86/platform/uv: Save OEM_ID from ACPI MADT probe"
In reply to: Anshuman Khandual: "Re: [PATCH 1/1] mm/pgtable/debug: Add test validating architecture page table helpers"
Next in thread: Anshuman Khandual: "Re: [PATCH 1/1] mm/pgtable/debug: Add test validating architecture page table helpers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 09/10/2019 03:56 AM, Anshuman Khandual wrote:

On 09/09/2019 08:43 PM, Kirill A. Shutemov wrote:

On Mon, Sep 09, 2019 at 11:56:50AM +0530, Anshuman Khandual wrote:

On 09/07/2019 12:33 AM, Gerald Schaefer wrote:

On Fri, 6 Sep 2019 11:58:59 +0530
Anshuman Khandual <anshuman.khandual@xxxxxxx> wrote:

On 09/05/2019 10:36 PM, Gerald Schaefer wrote:

On Thu, 5 Sep 2019 14:48:14 +0530
Anshuman Khandual <anshuman.khandual@xxxxxxx> wrote:

[...]

+
+#if !defined(__PAGETABLE_PMD_FOLDED) && !defined(__ARCH_HAS_4LEVEL_HACK)
+static void pud_clear_tests(pud_t *pudp)
+{
+ memset(pudp, RANDOM_NZVALUE, sizeof(pud_t));
+ pud_clear(pudp);
+ WARN_ON(!pud_none(READ_ONCE(*pudp)));
+}

For pgd/p4d/pud_clear(), we only clear if the page table level is present
and not folded. The memset() here overwrites the table type bits, so
pud_clear() will not clear anything on s390 and the pud_none() check will
fail.
Would it be possible to OR a (larger) random value into the table, so that
the lower 12 bits would be preserved?

So the suggestion is instead of doing memset() on entry with RANDOM_NZVALUE,
it should OR a large random value preserving lower 12 bits. Hmm, this should
still do the trick for other platforms, they just need non zero value. So on
s390, the lower 12 bits on the page table entry already has valid value while
entering this function which would make sure that pud_clear() really does
clear the entry ?

Yes, in theory the table entry on s390 would have the type set in the last
4 bits, so preserving those would be enough. If it does not conflict with
others, I would still suggest preserving all 12 bits since those would contain
arch-specific flags in general, just to be sure. For s390, the pte/pmd tests
would also work with the memset, but for consistency I think the same logic
should be used in all pxd_clear_tests.

Makes sense but..

There is a small challenge with this. Modifying individual bits on a given
page table entry from generic code like this test case is bit tricky. That
is because there are not enough helpers to create entries with an absolute
value. This would have been easier if all the platforms provided functions
like __pxx() which is not the case now. Otherwise something like this should
have worked.

pud_t pud = READ_ONCE(*pudp);
pud = __pud(pud_val(pud) | RANDOM_VALUE (keeping lower 12 bits 0))
WRITE_ONCE(*pudp, pud);

But __pud() will fail to build in many platforms.

Hmm, I simply used this on my system to make pud_clear_tests() work, not
sure if it works on all archs:

pud_val(*pudp) |= RANDOM_NZVALUE;

Which compiles on arm64 but then fails on x86 because of the way pmd_val()
has been defined there.

Use instead

*pudp = __pud(pud_val(*pudp) | RANDOM_NZVALUE);

Agreed.

As I had mentioned before this would have been really the cleanest approach.

It *should* be more portable.

Not really, because not all the platforms have __pxx() definitions right now.
Going with these will clearly cause build failures on affected platforms. Lets
examine __pud() for instance. It is defined only on these platforms.

arch/arm64/include/asm/pgtable-types.h: #define __pud(x) ((pud_t) { (x) } )
arch/mips/include/asm/pgtable-64.h: #define __pud(x) ((pud_t) { (x) })
arch/powerpc/include/asm/pgtable-be-types.h: #define __pud(x) ((pud_t) { cpu_to_be64(x) })
arch/powerpc/include/asm/pgtable-types.h: #define __pud(x) ((pud_t) { (x) })
arch/s390/include/asm/page.h: #define __pud(x) ((pud_t) { (x) } )
arch/sparc/include/asm/page_64.h: #define __pud(x) ((pud_t) { (x) } )
arch/sparc/include/asm/page_64.h: #define __pud(x) (x)
arch/x86/include/asm/pgtable.h: #define __pud(x) native_make_pud(x)

You missed:
arch/x86/include/asm/paravirt.h:static inline pud_t __pud(pudval_t val)
include/asm-generic/pgtable-nop4d-hack.h:#define __pud(x) ((pud_t) { __pgd(x) })
include/asm-generic/pgtable-nopud.h:#define __pud(x) ((pud_t) { __p4d(x) })

Similarly for __pmd()

arch/alpha/include/asm/page.h: #define __pmd(x) ((pmd_t) { (x) } )
arch/arm/include/asm/page-nommu.h: #define __pmd(x) (x)
arch/arm/include/asm/pgtable-2level-types.h: #define __pmd(x) ((pmd_t) { (x) } )
arch/arm/include/asm/pgtable-2level-types.h: #define __pmd(x) (x)
arch/arm/include/asm/pgtable-3level-types.h: #define __pmd(x) ((pmd_t) { (x) } )
arch/arm/include/asm/pgtable-3level-types.h: #define __pmd(x) (x)
arch/arm64/include/asm/pgtable-types.h: #define __pmd(x) ((pmd_t) { (x) } )
arch/m68k/include/asm/page.h: #define __pmd(x) ((pmd_t) { { (x) }, })
arch/mips/include/asm/pgtable-64.h: #define __pmd(x) ((pmd_t) { (x) } )
arch/nds32/include/asm/page.h: #define __pmd(x) (x)
arch/parisc/include/asm/page.h: #define __pmd(x) ((pmd_t) { (x) } )
arch/parisc/include/asm/page.h: #define __pmd(x) (x)
arch/powerpc/include/asm/pgtable-be-types.h: #define __pmd(x) ((pmd_t) { cpu_to_be64(x) })
arch/powerpc/include/asm/pgtable-types.h: #define __pmd(x) ((pmd_t) { (x) })
arch/riscv/include/asm/pgtable-64.h: #define __pmd(x) ((pmd_t) { (x) })
arch/s390/include/asm/page.h: #define __pmd(x) ((pmd_t) { (x) } )
arch/sh/include/asm/pgtable-3level.h: #define __pmd(x) ((pmd_t) { (x) } )
arch/sparc/include/asm/page_32.h: #define __pmd(x) ((pmd_t) { { (x) }, })
arch/sparc/include/asm/page_32.h: #define __pmd(x) ((pmd_t) { { (x) }, })
arch/sparc/include/asm/page_64.h: #define __pmd(x) ((pmd_t) { (x) } )
arch/sparc/include/asm/page_64.h: #define __pmd(x) (x)
arch/um/include/asm/page.h: #define __pmd(x) ((pmd_t) { (x) } )
arch/um/include/asm/page.h: #define __pmd(x) ((pmd_t) { (x) } )
arch/x86/include/asm/pgtable.h: #define __pmd(x) native_make_pmd(x)

You missed:
arch/x86/include/asm/paravirt.h:static inline pmd_t __pmd(pmdval_t val)
include/asm-generic/page.h:#define __pmd(x) ((pmd_t) { (x) } )
include/asm-generic/pgtable-nopmd.h:#define __pmd(x) ((pmd_t) { __pud(x) } )

Similarly for __pgd()

arch/alpha/include/asm/page.h: #define __pgd(x) ((pgd_t) { (x) } )
arch/alpha/include/asm/page.h: #define __pgd(x) (x)
arch/arc/include/asm/page.h: #define __pgd(x) ((pgd_t) { (x) })
arch/arc/include/asm/page.h: #define __pgd(x) (x)
arch/arm/include/asm/pgtable-3level-types.h: #define __pgd(x) ((pgd_t) { (x) } )
arch/arm/include/asm/pgtable-3level-types.h: #define __pgd(x) (x)
arch/arm64/include/asm/pgtable-types.h: #define __pgd(x) ((pgd_t) { (x) } )
arch/csky/include/asm/page.h: #define __pgd(x) ((pgd_t) { (x) })
arch/hexagon/include/asm/page.h: #define __pgd(x) ((pgd_t) { (x) })
arch/m68k/include/asm/page.h: #define __pgd(x) ((pgd_t) { (x) } )
arch/mips/include/asm/page.h: #define __pgd(x) ((pgd_t) { (x) } )
arch/nds32/include/asm/page.h: #define __pgd(x) (x)
arch/nios2/include/asm/page.h: #define __pgd(x) ((pgd_t) { (x) })
arch/openrisc/include/asm/page.h: #define __pgd(x) ((pgd_t) { (x) })
arch/parisc/include/asm/page.h: #define __pgd(x) ((pgd_t) { (x) } )
arch/parisc/include/asm/page.h: #define __pgd(x) (x)
arch/powerpc/include/asm/pgtable-be-types.h: #define __pgd(x) ((pgd_t) { cpu_to_be64(x) })
arch/powerpc/include/asm/pgtable-types.h: #define __pgd(x) ((pgd_t) { (x) })
arch/riscv/include/asm/page.h: #define __pgd(x) ((pgd_t) { (x) })
arch/s390/include/asm/page.h: #define __pgd(x) ((pgd_t) { (x) } )
arch/sh/include/asm/page.h: #define __pgd(x) ((pgd_t) { (x) } )
arch/sparc/include/asm/page_32.h: #define __pgd(x) ((pgd_t) { (x) } )
arch/sparc/include/asm/page_32.h: #define __pgd(x) (x)
arch/sparc/include/asm/page_64.h: #define __pgd(x) ((pgd_t) { (x) } )
arch/sparc/include/asm/page_64.h: #define __pgd(x) (x)
arch/um/include/asm/page.h: #define __pgd(x) ((pgd_t) { (x) } )
arch/unicore32/include/asm/page.h: #define __pgd(x) ((pgd_t) { (x) })
arch/unicore32/include/asm/page.h: #define __pgd(x) (x)
arch/x86/include/asm/pgtable.h: #define __pgd(x) native_make_pgd(x)
arch/xtensa/include/asm/page.h: #define __pgd(x) ((pgd_t) { (x) } )

You missed:
arch/x86/include/asm/paravirt.h:static inline pgd_t __pgd(pgdval_t val)
include/asm-generic/page.h:#define __pgd(x) ((pgd_t) { (x) } )

Similarly for __p4d()

arch/s390/include/asm/page.h: #define __p4d(x) ((p4d_t) { (x) } )
arch/x86/include/asm/pgtable.h: #define __p4d(x) native_make_p4d(x)

You missed:
arch/x86/include/asm/paravirt.h:static inline p4d_t __p4d(p4dval_t val)
include/asm-generic/5level-fixup.h:#define __p4d(x) __pgd(x)
include/asm-generic/pgtable-nop4d.h:#define __p4d(x) ((p4d_t) { __pgd(x) })

The search pattern here has been "#define __pxx(". Unless I am missing something,
I dont see how we can use these without risking build failures.

I guess you missed that arches not defining them fall back on the definitions in include/asm-generic

Christophe

Next message: Sandro Volery: "Re: [PATCH] Staging: gasket: Use temporaries to reduce line length."
Previous message: Mike Travis: "[PATCH 1/8] x86/platform/uv: Save OEM_ID from ACPI MADT probe"
In reply to: Anshuman Khandual: "Re: [PATCH 1/1] mm/pgtable/debug: Add test validating architecture page table helpers"
Next in thread: Anshuman Khandual: "Re: [PATCH 1/1] mm/pgtable/debug: Add test validating architecture page table helpers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]