linux-kernel - Re: [PATCH v6 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: <Z75t3d1EXQpmim9m@google.com>
Date: Tue, 25 Feb 2025 17:26:53 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Kevin Loughlin <kevinloughlin@...gle.com>
Cc: 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, 
	pbonzini@...hat.com, kirill.shutemov@...ux.intel.com, kai.huang@...el.com, 
	ubizjak@...il.com, jgross@...e.com, kvm@...r.kernel.org, 
	thomas.lendacky@....com, pgonda@...gle.com, sidtelang@...gle.com, 
	mizhang@...gle.com, rientjes@...gle.com, manalinandan@...gle.com, 
	szy0127@...u.edu.cn
Subject: Re: [PATCH v6 1/2] x86, lib: Add WBNOINVD helper functions

On Sat, Feb 01, 2025, Kevin Loughlin wrote:
> +static inline int wbnoinvd_on_all_cpus(void)
> +{
> +	wbnoinvd();
> +	return 0;

Returning anything is silly.  I'll prepend a patch (I'm going to send a combined
version of this and the targeted flushing series) to remove the return value
from wbinvd_on_all_cpus(), which I'm guessing is the source of the silliness.

>  static inline struct cpumask *cpu_llc_shared_mask(int cpu)
>  {
>  	return (struct cpumask *)cpumask_of(0);
> diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
> index 03e7c2d49559..86a903742139 100644
> --- a/arch/x86/include/asm/special_insns.h
> +++ b/arch/x86/include/asm/special_insns.h
> @@ -117,7 +117,24 @@ static inline void wrpkru(u32 pkru)
>  
>  static __always_inline void wbinvd(void)
>  {
> -	asm volatile("wbinvd": : :"memory");
> +	asm volatile("wbinvd" : : : "memory");
> +}
> +
> +/* Instruction encoding provided for binutils backwards compatibility. */
> +#define WBNOINVD ".byte 0xf3,0x0f,0x09"

Argh.  This causes problems for KVM, because KVM's newfangled CPUID macros heavily
use token pasting with X86_FEATURE_xxx, and so KVM's usage of:

	F(WBNOINVD)

causes explosions.  Somewhat amusingly, this is the second time today I ran into
this problem, as WRMSRNS as the safe issue.

Dave (and others),

Any thoughts on the best way forward?  I hacked around a similar collision in
commit 8d862c270bf1 ("KVM: x86: #undef SPEC_CTRL_SSBD in cpuid.c to avoid macro
collisions"), but (a) that scares me less because KVM should never use the
SPEC_CTRL_SSBD macro, and (b) I really, really don't want that to be the long-term
solution.  The only reason I committed the hack was because it was the only
blocking issue for a massive rework, and I couldn't get traction on renaming
the MSR macro.

For WBNOINVD, WRMSRNS, and any other instructions that come along, what about this?
Quite ugly, but it's at least descriptive.  And more importantly, the chances of
unwanted behavior are quite low.

/* Instruction encoding provided for binutils backwards compatibility. */
#define ASM_WBNOINVD ".byte 0xf3,0x0f,0x09"

/*
 * Cheaper version of wbinvd(). Call when caches need to be written back but
 * not invalidated.
 */
static __always_inline void wbnoinvd(void)
{
	/*
	 * If WBNOINVD is unavailable, fall back to the compatible but
	 * more destructive WBINVD (which still writes the caches back
	 * but also invalidates them).
	 */
	alternative("wbinvd", ASM_WBNOINVD, X86_FEATURE_WBNOINVD);
}