Re: [RFT PATCH v2 03/23] x86/boot: Drop global variables keeping track of LA57 state

Ard Biesheuvel
2025-05-05T22:30:15Z

On Mon, 5 May 2025 at 23:25, Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Ingo Molnar <mingo@kernel.org> wrote:
>
> > Anyway, with these limitations in mind, we can see that the top 5
> > usecases cover about 80% of all uses:
> >
> > - MAX_PHYSMEM_BITS: (inlined 179 times)
> >
> > arch/x86/include/asm/sparsemem.h:# define MAX_PHYSMEM_BITS (pgtable_l5_enabled() ? 52 : 46)
> >
> > This could be implemented via a precomputed, constant percpu value
> > (per_cpu__x86_MAX_PHYSMEM_BITS) of 52 vs. 46, eliminating not just
> > the CR4 access, but also a branch, at the cost of a percpu memory
> > access. (Which should still be a win on all microarchitectures IMO.)
> >
> > Alternatively, since this value is a semi-constant of 52 vs. 46, we
> > could also, I suspect, ALTERNATIVES-patch MAX_PHYSMEM_BITS in as an
> > immediate constant value? Any reason this shouldn't work:
> >
> > static inline unsigned int __MAX_PHYSMEM_BITS(void)
> > {
> > unsigned int bits;
> >
> > asm_inline (ALTERNATIVE("movl $46, %0", "movl $52, %0", X86_FEATURE_LA57) :"=g" (bits));
> >
> > return bits;
> > }
> > #define MAX_PHYSMEM_BITS __MAX_PHYSMEM_BITS()
> >
> > ... or something like that? This would result in the best code
> > generation IMO, by far. (It would even make use of the
> > zero-extension property of a 32-bit MOVL, further compressing the
> > opcode to only 5 bytes or so.)
> >
> > We'd even create a secondary helper macro for this, something like:
> >
> > #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)
> >
> > (Or so. Totally untested.)
>
> BTW., I keep comparing it to the CR4 access, which is a bit unfair,
> since that's only one out of the 3 variants of ALTERNATIVE_TERNARY():
>
> static __always_inline __pure bool pgtable_l5_enabled(void)
> {
> unsigned long r;
> bool ret;
>
> if (!IS_ENABLED(CONFIG_X86_5LEVEL))
> return false;
>
> asm(ALTERNATIVE_TERNARY(
> "movq %%cr4, %[reg] \n\t btl %[la57], %k[reg]" CC_SET(c),
> %P[feat], "stc", "clc")
> : [reg] "=&r" (r), CC_OUT(c) (ret)
> : [feat] "i" (X86_FEATURE_LA57),
> [la57] "i" (X86_CR4_LA57_BIT)
> : "cc");
>
> return ret;
> }
>
> The STC and CLC variants will probably be the more common outcomes on
> modern CPUs.
>

The CR4 access is only used before alternatives patching; after that,
either the STC or the CLC is patched in.

> But I still think the ALTERNATIVE_CONST_U32() approach I outline above
> generates superior code for the binary-values cases, which covers 3 out
> of the top 5 uses of pgtable_l5_enabled().
>
> For non-constant branching uses of pgtable_l5_enabled() I suspect the
> STC/CLC approach above is pretty good, although the 'cc' constraint
> will clobber all flags I suspect, while ALTERNATIVE_CONST_U32()
> doesn't? Ie. with ALTERNATIVE_CONST_U32() we just load the resulting
> constant into a register, with no additional branches and with flags
> undisturbed.
>
> Also, is STC/CLC always just as fast as the testing of an immediate (or
> a static branch), on CPUs we care about?
>

The reasoning behind using the C flag rather than asm goto is
precisely the fact that in many cases, pgtable_l5_enabled() is not
used for control flow but for picking between constants, and this
approach permits the compiler [in theory] to resolve it without a
branch. But I didn't inspect the resulting codegen, and just patching
in a MOV immediate is obviously more efficient than that.

I could just use asm goto here, and implement something similar to
cpu_feature_enabled(), but in a way that removes the need for
USE_EARLY_PGTABLE_L5. Then, we could get rid of the
__pgtable_l5_enabled variable in a separate patch, and base it on a
test of CR4.LA57 in the early init path.

Then, any optimizations related to constants defined in terms of
(pgtable_l5_enabled() ? foo : bar) could be layered on top of that.
Having runtime constants is the obvious choice, although I'm skeptical
whether it's worth the hassle. I also haven't yet looked into what it
would entail to share these runtime constants with the startup code.