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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGdbjm+pBXysSJjt6GaJHFQB8S5857Yk3LVziXGOf7QH5SzyeQ@mail.gmail.com>
Date: Wed, 22 Jan 2025 16:58:00 -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: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).