Re: [PATCH 1/1] x86/mm/pae: Align up pteval_t, pmdval_t and pudval_t to avoid split locks

[Brian Gerst]

On Wed, Apr 3, 2024 at 4:00 AM Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
>
> * Dave Hansen <dave.hansen@xxxxxxxxx> wrote:
>
> > On 4/2/24 10:23, Javier Pello wrote:
> > > On Mon, 1 Apr 2024 10:56:14 -0700 Dave Hansen wrote:
> > >> First of all, how is it that you're running a PAE kernel on new,
> > >> 64-bit hardware?  That's rather odd.
> > >
> > > I got this motherboard and cpu fairly recently to replace old
> > > hardware, and I just plugged my old hard disk and went along with
> > > it, because I did not feel like bootstrapping a 64-bit system.
> >
> > Fair enough.  I can totally understand wanting the convenience.  But
> > you're leaving _so_ much performance on the floor that split locks are
> > the least of your problems.
> >
> > >> The case that you're hitting is actually an on-stack pmd_t.  The
> > >> fun part is that it's not shared and doesn't even _need_ atomics.
> > >> I think it's just using pmd_populate() because it's convenient.
> > >
> > > I see. So just annotating the variable on the stack with
> > > __aligned(8) should do it? But the code is under mm/, so it should
> > > be arch-agnostic, right? What would the correct fix be, then? I take
> > > from your message that using atomics through pmd_populate() here is
> > > not needed, but what accessors should be used instead? I am not
> > > familiar at all with this part of the kernel.
> >
> > I don't think there's a better accessor.
> >
> > >> I'd honestly much rather just disable split lock support in 32-bit
> > >> builds than mess with this stuff.  You really shouldn't be running
> > >> 32-but kernels on this hardware.
> > >
> > > Why? Is it unsupported?
> >
> > Yes, it's effectively unsupported.  We're not adding new hardware
> > features to 32-bit.  The fact that split lock detection got enabled
> > was an accident.
>
> We do accept well-tested fixes and minor enablement patches though,
> within reason - but indeed this page table entry alignment quirk added
> for the sake of a split-lock debugging false positive doesn't seem to
> be worth it.

What would happen if you ran a 32-bit VM on such hardware?  If the
split lock detection on the guest were disabled, would the host get
the fault instead?

> > It's not a technical reason.  It's a practical one: I don't want to
> > spend time reviewing the fixes and dealing with the fallout and
> > regressions that the fixes might cause.
>
> Yeah, so it's an indirect technical argument: fixes *with tradeoffs*
> like this one have a future maintenance & robustness cost. Fixes
> without tradeoffs are fine of course.

What tradeoffs are there with this patch?  This would not affect the
page tables, since those are already properly aligned.  Forcing
alignment of stack variables is only a problem if it tickles a
compiler bug.

Brian Gerst