Re: x86/idle: Remove barriers for X86_BUG_CLFLUSH_MONITOR

[Borislav Petkov]

On Wed, Apr 02, 2025 at 10:10:17AM +0100, Andrew Cooper wrote:
> Commit 7e98b7192046 ("x86, idle: Use static_cpu_has() for CLFLUSH
> workaround, add barriers") adds barriers, justified with:
> 
>   ... and add memory barriers around it since the documentation is explicit
>   that CLFLUSH is only ordered with respect to MFENCE.
> 
> The SDM currently states:
> 
>   Executions of the CLFLUSH instruction are ordered with respect to each
>   other and with respect to writes, locked read-modify-write instructions,
>   and fence instructions[1].
> 
> With footnote 1 reading:
> 
>   Earlier versions of this manual specified that executions of the CLFLUSH
>   instruction were ordered only by the MFENCE instruction.  All processors
>   implementing the CLFLUSH instruction also order it relative to the other
>   operations enumerated above.
> 
> i.e. The SDM was incorrect at the time, and barriers should not have been
> inserted.  Double checking the original AAI65 errata (not available from
> intel.com any more) shows no mention of barriers either.
> 
> Additionally, drop the static_cpu_has_bug() and use a plain alternative.
> The workaround is a single instruction, with identical address setup to the
> MONITOR instruction.
> 
> Link: https://web.archive.org/web/20090219054841/http://download.intel.com/design/xeon/specupdt/32033601.pdf
> Fixes: 7e98b7192046 ("x86, idle: Use static_cpu_has() for CLFLUSH workaround, add barriers")
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> CC: Ingo Molnar <mingo@xxxxxxxxxx>
> CC: Borislav Petkov <bp@xxxxxxxxx>
> CC: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> CC: x86@xxxxxxxxxx
> CC: "H. Peter Anvin" <hpa@xxxxxxxxx>
> CC: linux-kernel@xxxxxxxxxxxxxxx
> 
> diff --git a/arch/x86/include/asm/mwait.h b/arch/x86/include/asm/mwait.h
> index ce857ef54cf1..dff9e7d854ed 100644
> --- a/arch/x86/include/asm/mwait.h
> +++ b/arch/x86/include/asm/mwait.h
> @@ -116,13 +116,11 @@ static __always_inline void __sti_mwait(unsigned long eax, unsigned long ecx)
>  static __always_inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx)
>  {
>  	if (static_cpu_has_bug(X86_BUG_MONITOR) || !current_set_polling_and_test()) {
> -		if (static_cpu_has_bug(X86_BUG_CLFLUSH_MONITOR)) {
> -			mb();
> -			clflush((void *)&current_thread_info()->flags);
> -			mb();
> -		}
> +		const void *addr = &current_thread_info()->flags;
>  
> -		__monitor((void *)&current_thread_info()->flags, 0, 0);
> +		alternative_input("", "clflush (%[addr])", X86_BUG_CLFLUSH_MONITOR,
> +				  [addr] "a" (addr));
> +		__monitor(addr, 0, 0);
>  
>  		if (!need_resched()) {
>  			if (ecx & 1) {

LGTM.

Acked-by: Borislav Petkov (AMD) <bp@xxxxxxxxx>

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette