[PATCH] Revert "mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge"

[Ard Biesheuvel]

On Mon, 19 Jul 2021 at 17:45, Christophe Leroy
<christophe.leroy at csgroup.eu> wrote:
>
> Ard Biesheuvel <ardb at kernel.org> a écrit :
>
> > (add some folks back to cc:)
> >
> > On Mon, 19 Jul 2021 at 15:58, Ard Biesheuvel <ardb at kernel.org> wrote:
> >>
> >> On Mon, 19 Jul 2021 at 12:49, Will Deacon <will at kernel.org> wrote:
> >> >
> >> > On Sat, Jul 17, 2021 at 06:31:08PM +0200, Christophe Leroy wrote:
> >> > > Jonathan Marek <jonathan at marek.ca> a écrit :
> >> > >
> >> > > > c742199a breaks arm64 for some configs because it stubs out
> >> functions which
> >> > > > should not have been stubbed out.
> >> > > >
> >> > > > With 4K pages and ARM64_VA_BITS_39=y, the kernel crashes
> >> early on unmapped
> >> > > > 1G pages in the linear map caused by pud_set_huge() in
> >> alloc_init_pud()
> >> > > > being stubbed out. Reverting c742199a fixes the crash.
> >> > >
> >> > > This patch is there for a reason. Reverting it will break some
> >> other config.
> >> >
> >> > Which config? Not reverting it breaks arm64.
> >> >
> >> > > The fix needs to allow it work with all platforms.
> >> >
> >> > Hmm, there was already a report that selftests were failing with this
> >> > change:
> >> >
> >> >
> >> https://lore.kernel.org/linux-arm-kernel/CAMuHMdXShORDox-xxaeUfDW3wx2PeggFSqhVSHVZNKCGK-y_vQ@mail.gmail.com/
> >> >
> >> > That was a week ago and doesn't seem to have progressed, so I'm
> >> inclined to
> >> > revert this if it's not resolved this week as we usually use -rc3
> >> as a base
> >> > for queuing patches for the next release.
> >> >
> >> > > I don't understand, why does arm64 needs these PUD helpers when
> >> it defines
> >> > > __PAGETABLE_PUD_FOLDED ?
> >>
> >> So if I am not mistaken, you modified arch/arm64 MMU code without
> >> understanding it and without cc'ing any of the arm64 maintainers,
> >> ignored reports that they were causing problems, and now you are
> >> objecting to them being reverted because it break 'some other config'?
> >> I don't think that is entirely reasonable, and the fact that the
> >> maintainers weren't even cc'ed on the patch is enough justification to
> >> simply revert it. IMHO.
>
> arm maintainers were in copy, see
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/73ec95f40cafbbb69bdfb43a7f53876fd845b0ce.1620990479.git.christophe.leroy@csgroup.eu/
>

I don't see Will or Catalin cc'ed on that patch, so no, the arm64
maintainers were not in copy. cc'ing a mailing list is not sufficient.

> I don't know what report you mention, I m only aware of one that I got
> after the start of my holidays and that I plan to handle first week of
> August when I'm back at work.
>

The one that Will just referred to:
https://lore.kernel.org/linux-arm-kernel/CAMuHMdXShORDox-xxaeUfDW3wx2PeggFSqhVSHVZNKCGK-y_vQ@mail.gmail.com/

> As I answered to Will, now that we are aware of an issue on Arm64 we
> need to work together and find a fix that works for all.
>

Sure. As long as arm64 gets fixed (and note that 39-bit VA/4k pages is
a very widely used config)


>
> >>
> >> >
> >> > Probably for the huge vmap code; see arch_vmap_pXd_supported(). That also
> >> > lines up with the failing selftests afaict.
> >> >
> >>
> >> The patch actually explains it: alloc_init_pud() in the swapper init
> >> code uses it to lay down 1 GB block mappings for the linear map. That
> >> code could quite easily be updated to work around this, but I guess it
> >> would be better to fix this more comprehensively.
> >>
> >> > For 4k pages with 3 levels of walk, we want to be able to use 1GB mappings
> >> > at level 1 (pgd), 2MB mappings at level 2 (pmd) as well as the usual 4k
> >> > page mappings at level 3 (pte).
> >> >
> >>
> >> Yes, we appear to use PUDs for 4k pages kernel regardless of whether
> >> they are folded into PGDs to refer to level 1/1GB block mappings.
>
> And that's probably the source of the misunderstanding, because a 3
> level architecture is supposed to only have pgd, pmd and pud.
>

Is this requirement documented somewhere? Having folded PUDs does not
necessarily mean that PUDs don't exist, but simply that PUDs and PGDs
are the same.


> Could the problematic arm pud_clear_huge and pud_set_huge be renamed
> pgd_clear_huge and pgd_set_huge ?
>

No, that would break 48-bit VA (4 level paging) configurations, as in
that case, PGDs and PUDs are different levels/sizes.