On Sat, Jan 31, 2015 at 12:31:42AM +0100, Borislav Petkov wrote:
> asm volatile(ALTERNATIVE(ASM_NOP3, "clflush %[p]", X86_BUG_CLFLUSH_MONITOR)
> : [p] "+m" (*mwait_ptr));
>
> Totally untested though - it is supposed to show the idea only.
Yeah, here's a working diff, ontop of this patchset:
https://lkml.kernel.org/r/[email protected]
---
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 6d7022c683e3..771ebd6e8b77 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1432,7 +1432,11 @@ static inline void mwait_play_dead(void)
* case where we return around the loop.
*/
mb();
- clflush(mwait_ptr);
+
+ asm volatile(ALTERNATIVE("", "clflush %[p]",
+ X86_BUG_CLFLUSH_MONITOR)
+ : [p] "+m" (*(unsigned long *)mwait_ptr));
+
mb();
__monitor(mwait_ptr, 0, 0);
mb();
--
At build time you have:
--
...
ffffffff81038dcc: 65 48 8b 1c 25 88 ab mov %gs:0xab88,%rbx # movq %gs:kernel_stack,%rbx #, pfo_ret__
ffffffff81038dd3: 00 00
ffffffff81038dd5: 48 81 eb c8 3f 00 00 sub $0x3fc8,%rbx # subq $16328, %rbx #, mwait_ptr
ffffffff81038ddc: 0f 09 wbinvd
ffffffff81038dde: 45 31 e4 xor %r12d,%r12d
ffffffff81038de1: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
ffffffff81038de8: 0f ae f0 mfence
ffffffff81038deb: 90 nop
ffffffff81038dec: 90 nop
ffffffff81038ded: 90 nop
ffffffff81038dee: 0f ae f0 mfence
ffffffff81038df1: 48 89 d8 mov %rbx,%rax
...
--
which during runtime, on those affected machines only, gets patched to:
---
...
ffffffff81038dcc: mov %gs:0xab88,%rbx
ffffffff81038dd5: sub $0x3fc8,%rbx
ffffffff81038ddc: wbinvd
ffffffff81038dde: xor %r12d,%r12d
ffffffff81038de1: nopl 0x0(%rax)
ffffffff81038de8: mfence
ffffffff81038deb: clflush (%rbx) <---
ffffffff81038dee: mfence
ffffffff81038df1: mov %rbx,%rax
...
---
:-)
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
From: Borislav Petkov <[email protected]>
Subject: [PATCH] x86, smpboot: Call CLFLUSH only on X86_BUG_CLFLUSH_MONITOR-affected CPUs
Make the AAI65 erratum workaround for Xeon 7400 machines only instead of
punishing all CPUs doing idle with MWAIT with the CLFLUSH penalty.
Based on a patch originally by Scotty Bauer <[email protected]>.
Cc: Scotty Bauer <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/kernel/smpboot.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 6d7022c683e3..771ebd6e8b77 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1432,7 +1432,11 @@ static inline void mwait_play_dead(void)
* case where we return around the loop.
*/
mb();
- clflush(mwait_ptr);
+
+ asm volatile(ALTERNATIVE("", "clflush %[p]",
+ X86_BUG_CLFLUSH_MONITOR)
+ : [p] "+m" (*(unsigned long *)mwait_ptr));
+
mb();
__monitor(mwait_ptr, 0, 0);
mb();
--
2.2.0.33.gc18b867
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
For what its worth I tried it out and it works fine on my end.
Thanks for doing the hard work for me, Boris. Also, thanks for a pointer to the alternatives.
I think it may be worth doing a patch that is almost verbatim to this for mwait_idle_with_hints in arch/x86/include/asm/mwait.h to keep things consistent. I can work on that over the weekend.
--Scotty
On 02/06/2015 09:13 AM, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
> Subject: [PATCH] x86, smpboot: Call CLFLUSH only on X86_BUG_CLFLUSH_MONITOR-affected CPUs
>
> Make the AAI65 erratum workaround for Xeon 7400 machines only instead of
> punishing all CPUs doing idle with MWAIT with the CLFLUSH penalty.
>
> Based on a patch originally by Scotty Bauer <[email protected]>.
>
> Cc: Scotty Bauer <[email protected]>
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
> arch/x86/kernel/smpboot.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 6d7022c683e3..771ebd6e8b77 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1432,7 +1432,11 @@ static inline void mwait_play_dead(void)
> * case where we return around the loop.
> */
> mb();
> - clflush(mwait_ptr);
> +
> + asm volatile(ALTERNATIVE("", "clflush %[p]",
> + X86_BUG_CLFLUSH_MONITOR)
> + : [p] "+m" (*(unsigned long *)mwait_ptr));
> +
> mb();
> __monitor(mwait_ptr, 0, 0);
> mb();
Hi Scotty,
On Wed, Feb 11, 2015 at 11:39:32AM -0700, Scotty Bauer wrote:
> For what its worth I tried it out and it works fine on my end.
>
> Thanks for doing the hard work for me, Boris. Also, thanks for a pointer to the alternatives.
>
> I think it may be worth doing a patch that is almost verbatim to this for mwait_idle_with_hints in arch/x86/include/asm/mwait.h to keep things consistent. I can work on that over the weekend.
please do not top-post, thanks.
Right, in thinking about this more, your original version actually is,
IMO, still the right thing to do.
Why, you ask. Well, because even with the alternatives, we need to
alternate not only the CLFLUSH but the surrounding MFENCEs too. And
those are different instructions on 32- and 64-bit. And doing that with
the alternatives might become uglier/more cluttered in the end than your
version.
And so, in the end of the day, having an unconditional, two-byte JMP in
there for all machines which are *not* affected shouldn't hurt - we're
jumping with great probability to the same I$ cacheline so not even a
cache miss. And we're on our way to idle so one more JMP is a dont-care.
And the C-code is actually readable. :-)
My only suggestion would be to change your original patch to what is
done in mwait_idle_with_hints() and use static_cpu_has_bug().
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--