linux-kernel - Re: [PATCH v4 1/2] x86, lib: Add WBNOINVD helper functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAGdbjm+JVbszOKRmCtHWWFYpAs5495f9+ZuAxc8ZOkqVLPEScw@mail.gmail.com>
Date: Wed, 22 Jan 2025 17:17:03 -0800
From: Kevin Loughlin <kevinloughlin@...gle.com>
To: Dave Hansen <dave.hansen@...el.com>
Cc: Tom Lendacky <thomas.lendacky@....com>, 
	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>, linux-kernel@...r.kernel.org, 
	tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, 
	dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com, seanjc@...gle.com, 
	pbonzini@...hat.com, kai.huang@...el.com, ubizjak@...il.com, jgross@...e.com, 
	kvm@...r.kernel.org, pgonda@...gle.com, sidtelang@...gle.com, 
	mizhang@...gle.com, rientjes@...gle.com, manalinandan@...gle.com, 
	szy0127@...u.edu.cn
Subject: Re: [PATCH v4 1/2] x86, lib: Add WBNOINVD helper functions

On Wed, Jan 22, 2025 at 4:58 PM Kevin Loughlin <kevinloughlin@...gle.com> wrote:
>
> On Wed, Jan 22, 2025 at 4:33 PM Dave Hansen <dave.hansen@...el.com> 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?