Re: [PATCH 3/3] mm: Add p{g/4}d_leaf() in asm-generic/pgtable-nop{4/u}d.h

[Peter Xu <peterx@redhat.com>]

On Tue, Jul 09, 2024 at 09:48:24PM +0200, Christophe Leroy wrote:
> 
> 
> Le 04/07/2024 à 16:48, Peter Xu a écrit :
> > On Thu, Jul 04, 2024 at 08:30:05AM +0200, Christophe Leroy wrote:
> > > Commit 2c8a81dc0cc5 ("riscv/mm: fix two page table check related
> > > issues") added pud_leaf() in include/asm-generic/pgtable-nopmd.h
> > > 
> > > Do the same for p4d_leaf() and pgd_leaf() to avoid getting them
> > > erroneously defined by architectures that do not implement the
> > > related page level.
> > > 
> > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> > > ---
> > >   include/asm-generic/pgtable-nop4d.h | 1 +
> > >   include/asm-generic/pgtable-nopud.h | 1 +
> > >   include/linux/pgtable.h             | 6 +++---
> > >   3 files changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/include/asm-generic/pgtable-nop4d.h b/include/asm-generic/pgtable-nop4d.h
> > > index 03b7dae47dd4..75c96bbc5a96 100644
> > > --- a/include/asm-generic/pgtable-nop4d.h
> > > +++ b/include/asm-generic/pgtable-nop4d.h
> > > @@ -21,6 +21,7 @@ typedef struct { pgd_t pgd; } p4d_t;
> > >   static inline int pgd_none(pgd_t pgd)		{ return 0; }
> > >   static inline int pgd_bad(pgd_t pgd)		{ return 0; }
> > >   static inline int pgd_present(pgd_t pgd)	{ return 1; }
> > > +static inline int pgd_leaf(pgd_t pgd)		{ return 0; }
> > >   static inline void pgd_clear(pgd_t *pgd)	{ }
> > >   #define p4d_ERROR(p4d)				(pgd_ERROR((p4d).pgd))
> > > diff --git a/include/asm-generic/pgtable-nopud.h b/include/asm-generic/pgtable-nopud.h
> > > index eb70c6d7ceff..14aeb8ef2d8a 100644
> > > --- a/include/asm-generic/pgtable-nopud.h
> > > +++ b/include/asm-generic/pgtable-nopud.h
> > > @@ -28,6 +28,7 @@ typedef struct { p4d_t p4d; } pud_t;
> > >   static inline int p4d_none(p4d_t p4d)		{ return 0; }
> > >   static inline int p4d_bad(p4d_t p4d)		{ return 0; }
> > >   static inline int p4d_present(p4d_t p4d)	{ return 1; }
> > > +static inline int p4d_leaf(p4d_t p4d)		{ return 0; }
> > >   static inline void p4d_clear(p4d_t *p4d)	{ }
> > >   #define pud_ERROR(pud)				(p4d_ERROR((pud).p4d))
> > > diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> > > index 2a6a3cccfc36..b27e66f542d6 100644
> > > --- a/include/linux/pgtable.h
> > > +++ b/include/linux/pgtable.h
> > > @@ -1882,13 +1882,13 @@ typedef unsigned int pgtbl_mod_mask;
> > >    * - It should cover all kinds of huge mappings (e.g., pXd_trans_huge(),
> > >    *   pXd_devmap(), or hugetlb mappings).
> > >    */
> > > -#ifndef pgd_leaf
> > > +#if !defined(__PAGETABLE_P4D_FOLDED) && !defined(pgd_leaf)
> > >   #define pgd_leaf(x)	false
> > >   #endif
> > > -#ifndef p4d_leaf
> > > +#if !defined(__PAGETABLE_PUD_FOLDED) && !defined(p4d_leaf)
> > >   #define p4d_leaf(x)	false
> > >   #endif
> > > -#ifndef pud_leaf
> > > +#if !defined(__PAGETABLE_PMD_FOLDED) && !defined(pud_leaf)
> > >   #define pud_leaf(x)	false
> > >   #endif
> > >   #ifndef pmd_leaf
> > 
> > Is it possible to do it the other way round, so that we can still rely on
> > "ifdef pxx_leaf" to decide whether to provide a fallback, and define them
> > properly when needed?
> 
> What do you mean by the "other way round" ? Did I do a mistake ? I can't see
> it.
> 
> The purpose here is:
> - If the architecture has the said level and implements pXd_leaf(), that's
> fine
> - If the architecture has the said level and doesn't implement pXd_leaf(),
> that's also fine, a fallback is provided.
> - If the architecture doesn't have the said level but implements pXd_leaf(),
> it will conflict with the definition in include/asm-generic/pgtable-nopXd.h
> and the build will fail.
> 
> The purpose is to make sure architectures don't implement pXd_leaf() at the
> wrong level, for instance:
> - an architecture without PMDs shall not implement anything else than
> pmd_leaf()
> - an architecture without P4Ds shall not implement pgd_leaf().

What I meant is it'll be nice to keep the pattern of using:

#ifndef XXX
#define XXX ...
#endif

Rather than:

#if defined(YYY) && !defined(XXX)
#define XXX ...
#endif

The reason is it is not as obvious as previous one, and we can still miss
some defines depending on whether YYY was there.

The current patch also didn't follow the rule where "if pxx_leaf is
defined, we should define the macro" rule.  Then we introduce yet another
rule for defining these.

In short, what I thought as "the other way round" is as simple as something
like:

===8<===
diff --git a/include/asm-generic/pgtable-nopmd.h b/include/asm-generic/pgtable-nopmd.h
index 8ffd64e7a24c..cce31c1f9159 100644
--- a/include/asm-generic/pgtable-nopmd.h
+++ b/include/asm-generic/pgtable-nopmd.h
@@ -31,8 +31,8 @@ static inline int pud_none(pud_t pud)         { return 0; }
 static inline int pud_bad(pud_t pud)           { return 0; }
 static inline int pud_present(pud_t pud)       { return 1; }
 static inline int pud_user(pud_t pud)          { return 0; }
-static inline int pud_leaf(pud_t pud)          { return 0; }
 static inline void pud_clear(pud_t *pud)       { }
+#define pud_leaf(pud)                          false
 #define pmd_ERROR(pmd)                         (pud_ERROR((pmd).pud))
 
 #define pud_populate(mm, pmd, pte)             do { } while (0)
===8<===

When used as a macro we don't need to touch linux/pgtable.h.  Would that
look nicer?

Thanks,

-- 
Peter Xu