Re: [PATCH v2] x86/idle: Remove MFENCEs for X86_BUG_CLFLUSH_MONITOR

[Uros Bizjak <ubizjak@gmail.com>]

On 2. 04. 25 19:24, 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.
> 
> This also triggered the same adjustment in commit
> f8e617f45829 ("sched/idle/x86: Optimize unnecessary mwait_idle() resched
> IPIs") during development, although it failed to get the static_cpu_has_bug()
> treatment.
> 
> X86_BUG_CLFLUSH_MONITOR (a.k.a the AAI65 errata) is specific to Intel CPUs,
> and 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.
> 
> Note: If this were a general codepath, the MFENCEs would be needed, because
>        AMD CPUs of the same vintage do sport otherwise-unordered CLFLUSHs.
> 
> Furthermore, use a plain alternative, rather than static_cpu_has_bug() and/or
> no optimisation.  The workaround is a single instruction.
> 
> Use an explicit %rax pointer rather than a general memory operand, because
> MONITOR takes the pointer implicitly in the same way.
> 
> 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")
> Fixes: f8e617f45829 ("sched/idle/x86: Optimize unnecessary mwait_idle() resched IPIs")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Borislav Petkov <bp@alien8.de>
> CC: Dave Hansen <dave.hansen@linux.intel.com>
> CC: x86@kernel.org
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: linux-kernel@vger.kernel.org
> 
> v2:
>   * Fix the same pattern in mwait_idle() too
>   * Expand on why we're not using a general memory operand.
> ---
>   arch/x86/include/asm/mwait.h | 11 +++++------
>   arch/x86/kernel/process.c    | 10 ++++------
>   2 files changed, 9 insertions(+), 12 deletions(-)

There is another instance of the same sequence in arch/x86/kernel/smpboot.c:

		/*
		 * The CLFLUSH is a workaround for erratum AAI65 for
		 * the Xeon 7400 series.  It's not clear it is actually
		 * needed, but it should be harmless in either case.
		 * The WBINVD is insufficient due to the spurious-wakeup
		 * case where we return around the loop.
		 */
		mb();
		clflush(md);
		mb();
		__monitor(md, 0, 0);
		mb();
		__mwait(eax_hint, 0);

Should this also be converted to the new sequence?

Uros.