[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMj1kXFsvWYMWy4_YCETsp2W0HQWe4RKcwXdB-fWKnoVqjzoqQ@mail.gmail.com>
Date: Tue, 6 May 2025 18:58:05 +0200
From: Ard Biesheuvel <ardb@...nel.org>
To: Ingo Molnar <mingo@...nel.org>
Cc: Ard Biesheuvel <ardb+git@...gle.com>, linux-kernel@...r.kernel.org, x86@...nel.org,
Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [RFC PATCH 3/3] x86/boot: Use alternatives based selector for
5-level paging constants
On Tue, 6 May 2025 at 18:50, Ingo Molnar <mingo@...nel.org> wrote:
>
>
> * Ard Biesheuvel <ardb+git@...gle.com> wrote:
>
> > +static inline int __attribute_const__ choose_l5_enabled(int yes, int no)
> > +{
> > + int ret = no;
> > +
> > + asm_inline(ALTERNATIVE_TERNARY("jmp 6f; 8:", %c[feat], "movl %[yes], %[ret]", "")
> > + " .pushsection .altinstr_aux,\"ax\" \n"
> > + "6: pushfq \n"
> > + " testb $1, %a[l5en] \n"
> > + " jz 7f \n"
> > + " movl %[yes], %[ret] \n"
> > + "7: popfq \n"
> > + " jmp 8b \n"
> > + " .popsection \n"
> > + : [ret] "+rm" (ret)
> > + : [feat] "i" (X86_FEATURE_LA57),
> > + [yes] "i" (yes),
> > + [l5en] "i" (&__pgtable_l5_enabled));
> > +
> > + return ret;
>
> So why not create a new synthethic cpufeature flag,
> X86_FEATURE_LA57_ENABLED or so, which could then be queried via the
> regular facilities? This ternary logic is not really needed, because
> the hardware isn't ternary. :)
>
The logic is not ternary, and ALTERNATIVE_TERNARY() is a misnomer. The
first branch of the alternative is the preliminary version that gets
used before alternatives have been patched.
This is needed, because otherwise, code that runs really early may
observe the wrong value. This is the whole reason the
USE_EARLY_PGTABLE_L5 exists to begin with.
> With that we could do with only a single, obvious line of ALTERNATIVE()
> assembly:
>
> #define ALTERNATIVES_CONST_U32(__val1, __val2, __feature) \
> ({ \
> u32 __val; \
> \
> asm_inline (ALTERNATIVE("movl $" #__val1 ", %0", "movl $" __val2 ", %0", __feature) :"=g" (__val)); \
> \
> __val; \
> })
>
> ...
>
> #define MAX_PHYSMEM_BITS ALTERNATIVE_CONST_U32(46, 52, X86_FEATURE_LA57_ENABLED)
>
This will produce 46 during early boot, and may therefore result in
the logic in __startup_64() to behave incorrectly. (Not sure whether
this macro in particular is implicated in that, but in general,
anything that relies on binary CPU feature logic in this context is
potentially broken.
I do agree that having a separate CPU feature X86_FEATURE_LA57_ENABLED
would be preferable: it is kind of strange that we have to pretend the
CPU does not implement 5-level paging in order to use only 4 levels,
and I suppose there may be cases where we want the wider physical
address space even when using only 4 levels of virtual addressing.
Powered by blists - more mailing lists