On Wed, Jan 22, 2025 at 4:58 PM Kevin Loughlin <kevinloughlin@xxxxxxxxxx> wrote:
>
> On Wed, Jan 22, 2025 at 4:33 PM Dave Hansen <dave.hansen@xxxxxxxxx> wrote:
> >
> > On 1/22/25 16:06, Kevin Loughlin wrote:
> > >> BTW, I don't think you should be compelled to use alternative() as
> > >> opposed to a good old:
> > >>
> > >> if (cpu_feature_enabled(X86_FEATURE_WBNOINVD))
> > >> ...
> > > Agreed, though I'm leaving as alternative() for now (both because it
> > > results in fewer checks and because that's what is used in the rest of
> > > the file); please holler if you prefer otherwise. If so, my slight
> > > preference in that case would be to update the whole file
> > > stylistically in a separate commit.
> >
> > alternative() can make a _lot_ of sense. It's extremely compact in the
> > code it generates. It messes with compiler optimization, of course, just
> > like any assembly. But, overall, it's great.
> >
> > In this case, though, we don't care one bit about code generation or
> > performance. We're running the world's slowest instruction from an IPI.
> >
> > As for consistency, special_insns.h is gloriously inconsistent. But only
> > two instructions use alternatives, and they *need* the asm syntax
> > because they're passing registers and meaningful constraints in.
> >
> > The wbinvds don't get passed registers and their constraints are
> > trivial. This conditional:
> >
> > alternative_io(".byte 0x3e; clflush %0",
> > ".byte 0x66; clflush %0",
> > X86_FEATURE_CLFLUSHOPT,
> > "+m" (*(volatile char __force *)__p));
> >
> > could be written like this:
> >
> > if (cpu_feature_enabled(X86_FEATURE_CLFLUSHOPT))
> > asm volatile(".byte 0x3e; clflush %0",
> > "+m" (*(volatile char __force *)__p));
> > else
> > asm volatile(".byte 0x66; clflush %0",
> > "+m" (*(volatile char __force *)__p));
> >
> > But that's _actively_ ugly. alternative() syntax there makes sense.
> > Here, it's not ugly at all:
> >
> > if (cpu_feature_enabled(X86_FEATURE_WBNOINVD))
> > asm volatile(".byte 0xf3,0x0f,0x09\n\t": : :"memory");
> > else
> > wbinvd();
> >
> > and it's actually more readable with alternative() syntax.
> >
> > So, please just do what makes the code look most readable. Performance
> > and consistency aren't important. I see absolutely nothing wrong with:
> >
> > static __always_inline void raw_wbnoinvd(void)
> > {
> > asm volatile(".byte 0xf3,0x0f,0x09\n\t": : :"memory");
> > }
> >
> > void wbnoinvd(void)
> > {
> > if (cpu_feature_enabled(X86_FEATURE_WBNOINVD))
> > raw_wbnoinvd();
> > else
> > wbinvd();
> > }
> >
> > ... except the fact that cpu_feature_enabled() kinda sucks and needs
> > some work, but that's a whole other can of worms we can leave closed today.
>
> Thanks for the detailed explanation; you've convinced me. v6 coming up
> shortly (using native_wbnoinvd() instead of raw_wbnoinvd(), as you
> named the proposed wrapper in your reply to v5).
Actually, we may still want to use alternative() for the following reason:
Kirill noted in ad3fe525b950 ("x86/mm: Unify pgtable_l5_enabled usage
in early boot code") that cpu_feature_enabled() can't be used in early
boot code, which would mean using it would make the wbnoinvd()
implementation incompatible with early boot code if desired there. In
contrast, I believe alternative() will just fall back to WBINVD until
apply_alternatives() runs, at which point it will be replaced with
WBNOINVD if the processor supports it.
I'll still use the native_wbnoinvd() wrapper to clarify the encoding
as you suggested, but does this reasoning for keeping alternative()
make sense to you Dave? Or am I missing something?
