Re: [PATCH RFC v2 13/20] x86: mm: carve out the generic compile-time folded pgtable case in effective_prot()

> On 7/22/26 23:00, Yeoreum Yun wrote:
> > On Wed, Jul 22, 2026 at 01:20:39PM -0700, Dave Hansen wrote:
> >> On 7/22/26 10:37, Yeoreum Yun wrote:
> >>> However, mm_pXd_folded() requires to mm for other architecture like
> >>> s390. might save the mm instead of first_level and calls the
> >>> ptdump_pt_level_first() with static inline version would make the
> >>> constant comparison. So it requires to save *mm* structure in here.
> >>
> >> I'm having a really hard time parsing that.
> >>
> >> I think you're trying to say that the effective_prot_p*() functions need
> >> to know the first level but they don't (today) need the mm_struct. If
> >> they don't get the (calculated) first_level passed in, they need the
> >> mm_struct instead.
> >>
> >> I think you're arguing 'pg_state' needs a ->first_level or a ->mm.
> >> Having a pg_state->mm doesn't seem bad to me at all.
> >>
> >> But, it's also a little bit silly. It would not be rocket science to
> >> pass an mm_struct down to the effective_prot_p*() functions. It makes a
> >> whole lot of sense to me for a page table walking function to need
> >> metadata from the mm_struct to walk correctly.
> > 
> > Yes. I mean to add pg_state->mm like:
> > 
> > diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
> > index 2afa7a23340e..aaf00f0c6624 100644
> > --- a/arch/x86/mm/dump_pagetables.c
> > +++ b/arch/x86/mm/dump_pagetables.c
> > @@ -38,6 +38,7 @@ struct pg_state {
> >         bool check_wx;
> >         unsigned long wx_pages;
> >         struct seq_file *seq;
> > +       struct mm_struct *mm;
> >  };
> > 
> >  struct addr_marker {
> > @@ -254,7 +255,7 @@ static void effective_prot(struct ptdump_state *pt_st, int level, u64 val)
> >         pgprotval_t prot = val & PTE_FLAGS_MASK;
> >         pgprotval_t effective;
> > 
> > -       if (level > 0) {
> > +       if (level > pgtable_first_level(st->mm)) {
> >                 pgprotval_t higher_prot = st->prot_levels[level - 1];
> > 
> >                 effective = (higher_prot & prot & (_PAGE_USER | _PAGE_RW)) |
> > @@ -452,7 +453,8 @@ bool ptdump_walk_pgd_level_core(struct seq_file *m,
> >                 .level = -1,
> >                 .to_dmesg       = dmesg,
> >                 .check_wx       = checkwx,
> > -               .seq            = m
> > +               .seq            = m,
> > +               .mm             = mm,
> >         };
> > 
> >         ptdump_walk_pgd(&st.ptdump, mm, pgd);
> > diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> > index 8c093c119e5a..6e7d0580db99 100644
> > --- a/include/linux/pgtable.h
> > +++ b/include/linux/pgtable.h
> > @@ -2490,4 +2490,15 @@ pgprot_t vm_get_page_prot(vm_flags_t vm_flags)                           \
> >  }                                                                      \
> >  EXPORT_SYMBOL(vm_get_page_prot);
> > 
> > +static inline int pgtable_first_level(struct mm_struct *mm)
> > +{
> > +       if (mm_pmd_folded(mm))
> > +               return 3;
> > +       if (mm_pud_folded(mm))
> > +               return 2;
> > +       if (mm_p4d_folded(mm))
> > +               return 1;
> > +       return 0;
> > +}
> 
> In that case the function should probably be called
> 
> "mm_first_pgtable_level"
> 
> But now it gets confusing, because we have
> 
> enum pgtable_level {
> 	PGTABLE_LEVEL_PTE = 0,
> 	PGTABLE_LEVEL_PMD,
> 	PGTABLE_LEVEL_PUD,
> 	PGTABLE_LEVEL_P4D,
> 	PGTABLE_LEVEL_PGD,
> };
> 
> 
> But maybe we can make sense of it and do
> 
> /*
>  * The enum values correspond to the numerical page table level,
>  * starting with the highest level being level 0.
>  */
> enum pgtable_level {
> 	PGTABLE_LEVEL_PGD = 0,
> 	PGTABLE_LEVEL_P4D,
> 	PGTABLE_LEVEL_PUD,
> 	PGTABLE_LEVEL_PMD,
> 	PGTABLE_LEVEL_PTE,
> };
> 
> static inline enum pgtable_level mm_first_pgtable_level(struct mm_struct *mm)
> {
> 	if (mm_pmd_folded(mm))
> 		return PGTABLE_LEVEL_PMD;
> 	if (mm_pud_folded(mm))
> 		return PGTABLE_LEVEL_PUD;
> 	if (mm_p4d_folded(mm))
> 		return PGTABLE_LEVEL_P4D;
> 	return PGTABLE_LEVEL_PGD;
> }
> 
> 
> We could even teach effective_prot() and friends to consume enum pgtable_level
> now and have it all be a bit cleaner?

Yes. That would be good for me unless others comment.


-- 
Sincerely,
Yeoreum Yun

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv