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: <3dd183fa-df95-487e-a2e9-73579fa160be@intel.com>
Date: Wed, 22 Jan 2025 16:33:04 -0800
From: Dave Hansen <dave.hansen@...el.com>
To: Kevin Loughlin <kevinloughlin@...gle.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 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.