Re: LPA2 on non-LPA2 hardware broken with 16K pages

[Will Deacon <will@kernel.org>]

Hey Ard,

On Fri, Jul 19, 2024 at 11:02:29AM -0700, Ard Biesheuvel wrote:
> Thanks for the cc, and thanks to Lina for the excellent diagnosis -
> this is really helpful.
> 
> > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> > index f8efbc128446..3afe624a39e1 100644
> > --- a/arch/arm64/include/asm/pgtable.h
> > +++ b/arch/arm64/include/asm/pgtable.h
> > @@ -1065,6 +1065,13 @@ static inline bool pgtable_l5_enabled(void) { return false; }
> >
> >  #define p4d_offset_kimg(dir,addr)      ((p4d_t *)dir)
> >
> > +static inline
> > +p4d_t *p4d_offset_lockless(pgd_t *pgdp, pgd_t pgd, unsigned long addr)
> 
> This is in the wrong place, I think - we already define this for the
> 5-level case (around line 1760).

Hmm, I'm a bit confused. In my tree, we have one definition at line 1012,
which is for the 5-level case (i.e. guarded by
'#if CONFIG_PGTABLE_LEVELS > 4'). I'm adding a new one at line 1065,
which puts it in the '#else' block and means we use an override instead
of the problematic generic version when we're folding.

> We'll need to introduce another version for the 4-level case, so
> perhaps, to reduce the risk of confusion, we might define it as
> 
> static inline
> p4d_t *p4d_offset_lockless_folded(pgd_t *pgdp, pgd_t pgd, unsigned long addr)
> {
> ...
> }
> #ifdef __PAGETABLE_P4D_FOLDED
> #define p4d_offset_lockless p4d_offset_lockless_folded
> #endif

Renaming will definitely make this easier on the eye, so I'll do that.
I don't think I need the 'ifdef' though.

> > +{
> 
> We might add
> 
> if (pgtable_l4_enabled())
>     pgdp = &pgd;
> 
> here to preserve the existing 'lockless' behavior when PUDs are not
> folded.

The code still needs to be 'lockless' for the 5-level case, so I don't
think this is necessary. Yes, we'll load the same entry multiple times,
but it should be fine because they're in the context of a different
(albeit folded) level.

> > +       return p4d_offset(pgdp, addr);
> > +}
> > +#define p4d_offset_lockless p4d_offset_lockless
> > +
> >  #endif  /* CONFIG_PGTABLE_LEVELS > 4 */
> >
> 
> I suggest we also add something like the below so we can catch these
> issues more easily
> 
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -874,9 +874,26 @@ static inline phys_addr_t p4d_page_paddr(p4d_t p4d)
> 
>  static inline pud_t *p4d_to_folded_pud(p4d_t *p4dp, unsigned long addr)
>  {
> +       /*
> +        * The transformation below does not work correctly for descriptors
> +        * copied to the stack.
> +        */
> +       VM_WARN_ON((u64)p4dp >= VMALLOC_START && !__is_kernel((u64)p4dp));

Hmm, this is a bit coarse. Does it work properly with the fixmap?

Will