Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1533551 > unrolled thread

[PATCH 0/4] CPUID-less CPU fixes and improvements

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] CPUID-less CPU fixes and improvements Andy Lutomirski <luto@kernel.org> - 2016-11-30 21:40 +0100
    [PATCH 4/4] x86/asm: Change sync_core() to use MOV to CR2 to serialize Andy Lutomirski <luto@kernel.org> - 2016-11-30 21:40 +0100
      Re: [PATCH 4/4] x86/asm: Change sync_core() to use MOV to CR2 to  serialize Borislav Petkov <bp@alien8.de> - 2016-12-01 10:30 +0100
        Re: [PATCH 4/4] x86/asm: Change sync_core() to use MOV to CR2 to serialize Andy Lutomirski <luto@amacapital.net> - 2016-12-01 18:10 +0100
          Re: [PATCH 4/4] x86/asm: Change sync_core() to use MOV to CR2 to  serialize Andrew Cooper <andrew.cooper3@citrix.com> - 2016-12-01 18:50 +0100
        Re: [PATCH 4/4] x86/asm: Change sync_core() to use MOV to CR2 to  serialize Ingo Molnar <mingo@kernel.org> - 2016-12-02 08:40 +0100
    [PATCH 3/4] x86/microcode/intel: Replace sync_core() with cpuid_eax(1) Andy Lutomirski <luto@kernel.org> - 2016-11-30 21:40 +0100
      Re: [PATCH 3/4] x86/microcode/intel: Replace sync_core() with  cpuid_eax(1) Borislav Petkov <bp@alien8.de> - 2016-12-01 10:20 +0100
    [PATCH v2 6/6] x86/asm: Change sync_core() to use MOV to CR2 to serialize Andy Lutomirski <luto@kernel.org> - 2016-12-02 01:40 +0100
    [PATCH v2 4/6] x86/paravirt: Make sync_core() be a paravirt op Andy Lutomirski <luto@kernel.org> - 2016-12-02 01:40 +0100
    [PATCH v2 3/6] x86/microcode/intel: Replace sync_core() with cpuid_eax(1) Andy Lutomirski <luto@kernel.org> - 2016-12-02 01:40 +0100
    [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation Andy Lutomirski <luto@kernel.org> - 2016-12-02 01:40 +0100
      Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core()  implementation Andrew Cooper <andrew.cooper3@citrix.com> - 2016-12-02 12:50 +0100
        Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation Andy Lutomirski <luto@amacapital.net> - 2016-12-02 18:10 +0100
          Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core()  implementation Andrew Cooper <andrew.cooper3@citrix.com> - 2016-12-02 18:20 +0100
            Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core()  implementation Andrew Cooper <andrew.cooper3@citrix.com> - 2016-12-02 18:30 +0100
            Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation Andy Lutomirski <luto@amacapital.net> - 2016-12-02 18:30 +0100
        Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core()  implementation Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-12-02 19:50 +0100
          Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation Andy Lutomirski <luto@amacapital.net> - 2016-12-02 20:40 +0100
        Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core()  implementation Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-12-02 21:10 +0100
      Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation Linus Torvalds <torvalds@linux-foundation.org> - 2016-12-02 18:40 +0100
        Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation Andy Lutomirski <luto@kernel.org> - 2016-12-02 18:40 +0100
          Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core()  implementation Borislav Petkov <bp@kernel.org> - 2016-12-02 19:20 +0100
            Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation Linus Torvalds <torvalds@linux-foundation.org> - 2016-12-02 19:30 +0100
              Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core()  implementation Borislav Petkov <bp@kernel.org> - 2016-12-02 20:00 +0100
                Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation Linus Torvalds <torvalds@linux-foundation.org> - 2016-12-02 20:10 +0100
                  Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation Linus Torvalds <torvalds@linux-foundation.org> - 2016-12-02 20:30 +0100
                    Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core()  implementation Borislav Petkov <bp@alien8.de> - 2016-12-02 20:30 +0100
                      [PATCH] x86/alternatives: Do not use sync_core() to serialize I$ Borislav Petkov <bp@alien8.de> - 2016-12-03 16:10 +0100
                        Re: [PATCH] x86/alternatives: Do not use sync_core() to serialize I$ Andy Lutomirski <luto@amacapital.net> - 2016-12-03 18:20 +0100
                    Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation Linus Torvalds <torvalds@linux-foundation.org> - 2016-12-02 20:40 +0100
                      Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation Andy Lutomirski <luto@amacapital.net> - 2016-12-02 21:50 +0100
                        Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation Linus Torvalds <torvalds@linux-foundation.org> - 2016-12-02 22:20 +0100
                          Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation Andy Lutomirski <luto@amacapital.net> - 2016-12-03 00:00 +0100
                            Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation Linus Torvalds <torvalds@linux-foundation.org> - 2016-12-03 00:20 +0100
                    Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation Andy Lutomirski <luto@amacapital.net> - 2016-12-02 20:40 +0100
                  Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation Andy Lutomirski <luto@amacapital.net> - 2016-12-02 20:30 +0100
                    Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core()  implementation Borislav Petkov <bp@alien8.de> - 2016-12-02 20:40 +0100
                  Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core()  implementation Borislav Petkov <bp@alien8.de> - 2016-12-02 20:30 +0100
          Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation Linus Torvalds <torvalds@linux-foundation.org> - 2016-12-02 19:20 +0100
          Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core()  implementation Borislav Petkov <bp@alien8.de> - 2016-12-03 13:50 +0100
    [PATCH v2 2/6] Revert "x86/boot: Fail the boot if !M486 and CPUID is missing" Andy Lutomirski <luto@kernel.org> - 2016-12-02 01:40 +0100
    Re: [PATCH 0/4] CPUID-less CPU fixes and improvements Andy Lutomirski <luto@amacapital.net> - 2016-12-02 01:40 +0100
      Re: [PATCH 0/4] CPUID-less CPU fixes and improvements Ingo Molnar <mingo@kernel.org> - 2016-12-02 11:20 +0100
    [PATCH v2 0/6] CPUID-less CPU/sync_core fixes and improvements Andy Lutomirski <luto@kernel.org> - 2016-12-02 01:40 +0100
    [PATCH v2 1/6] x86/asm/32: Make sync_core() handle missing CPUID on all 32-bit kernels Andy Lutomirski <luto@kernel.org> - 2016-12-02 01:40 +0100

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1535097 — Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation

On Thu, Dec 1, 2016 at 4:35 PM, Andy Lutomirski <luto@kernel.org> wrote:
>
> On my laptop, CPUID(eax=1, ecx=0) is ~83ns and IRET-to-self is
> ~110ns.  But Xen PV will trap CPUID if possible, so IRET-to-self
> should end up being a nice speedup.

So if we care deeply about the performance of this, we should really
ask ourselves how much we need this...

There are *very* few places where we really need to do a full
serializing instruction, and I'd worry that we really don't need it in
many of the places we do this.

The only real case I'm aware of is modifying code that is modified
through a different linear address than it's executed.

Is there anything else where we _really_ need this sync-core thing?
Sure, the microcode loader looks fine, but that doesn't look
particularly performance-critical either.

So I'd like to know which sync_core is actually so
performance-critical that w e care about it, and then I'd like to
understand why it's needed at all, because I suspect a number of them
has been added with the model of "sprinkle random things around and
hope".

Looking at ftrace, for example, which is one of the users, does it
actually _really_ need sync_core() at all? It seems to use the kerrnel
virtual address, and then the instruction stream will be coherent.

Adding Peter Anvin to the participants list, because iirc he was the
one who really talked to hardwre engineers about the synchronization
issues with serializing kernel code.

                Linus

[toc] | [prev] | [next] | [standalone]

#1535100 — Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation

On Fri, Dec 2, 2016 at 9:32 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Dec 1, 2016 at 4:35 PM, Andy Lutomirski <luto@kernel.org> wrote:
>>
>> On my laptop, CPUID(eax=1, ecx=0) is ~83ns and IRET-to-self is
>> ~110ns.  But Xen PV will trap CPUID if possible, so IRET-to-self
>> should end up being a nice speedup.
>
> So if we care deeply about the performance of this, we should really
> ask ourselves how much we need this...
>
> There are *very* few places where we really need to do a full
> serializing instruction, and I'd worry that we really don't need it in
> many of the places we do this.
>
> The only real case I'm aware of is modifying code that is modified
> through a different linear address than it's executed.

TBH, I didn't start down this path for performance.  I did it because
I wanted to kill off a CPUID that was breaking on old CPUs that don't
have CPUID.  So I propose MOV-to-CR2 followed by an unconditional
jump.  My goal here is to make the #*!& thing work reliably and not be
ludicrously slow.  Borislav and I mulled over using an alternative to
use CPUID if and only if we have CPUID, but that doesn't work because
we call sync_core() before we're done applying alternatives.

>
> Is there anything else where we _really_ need this sync-core thing?
> Sure, the microcode loader looks fine, but that doesn't look
> particularly performance-critical either.
>
> So I'd like to know which sync_core is actually so
> performance-critical that w e care about it, and then I'd like to
> understand why it's needed at all, because I suspect a number of them
> has been added with the model of "sprinkle random things around and
> hope".

apply_alternatives, unfortunately.  It's performance-critical because
it's intensely stupid and does sync_core() for every single patch.
Fixing that would be nice, too.

> Adding Peter Anvin to the participants list, because iirc he was the
> one who really talked to hardwre engineers about the synchronization
> issues with serializing kernel code.
>
>                 Linus

[toc] | [prev] | [next] | [standalone]

#1535112 — Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation

On Fri, Dec 02, 2016 at 09:38:38AM -0800, Andy Lutomirski wrote:
> apply_alternatives, unfortunately.  It's performance-critical because
> it's intensely stupid and does sync_core() for every single patch.
> Fixing that would be nice, too.

So I did experiment at the time to batch that sync_core() and interrupts
disabling in text_poke_early() but that amounted to almost nothing.
That's why I didn't bother chasing this further. I can try to dig out
that old thread...

/me goes and searches...

ah, here's something:

SNB:
* before:
[    0.011707] SMP alternatives: Starting apply_alternatives...
[    0.011784] SMP alternatives: done.
[    0.011806] Freeing SMP alternatives: 20k freed

* after:
[    0.011699] SMP alternatives: Starting apply_alternatives...
[    0.011721] SMP alternatives: done.
[    0.011743] Freeing SMP alternatives: 20k freed

-> 63 microseconds speedup


* kvm guest:
* before:
[    0.017005] SMP alternatives: Starting apply_alternatives...
[    0.019024] SMP alternatives: done.
[    0.020119] Freeing SMP alternatives: 20k freed

* after:
[    0.015008] SMP alternatives: Starting apply_alternatives...
[    0.016029] SMP alternatives: done.
[    0.017118] Freeing SMP alternatives: 20k freed

-> ~3 milliseconds speedup

---

I tried something simple like this:

---
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 1850592f4700..f4d5689ea503 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -253,10 +253,13 @@ void __init_or_module apply_alternatives(struct alt_instr *start,
                                         struct alt_instr *end)
 {
        struct alt_instr *a;
+       unsigned long flags;
        u8 *instr, *replacement;
        u8 insnbuf[MAX_PATCH_LEN];

        DPRINTK("%s: alt table %p -> %p\n", __func__, start, end);
+
+       local_irq_save(flags);
        /*
         * The scan order should be from start to end. A later scanned
         * alternative code can overwrite a previous scanned alternative code.
@@ -284,8 +287,10 @@ void __init_or_module apply_alternatives(struct alt_instr *start,
                add_nops(insnbuf + a->replacementlen,
                         a->instrlen - a->replacementlen);

-               text_poke_early(instr, insnbuf, a->instrlen);
+               memcpy(instr, insnbuf, a->instrlen);
        }
+       sync_core();
+       local_irq_restore(flags);
 }

 #ifdef CONFIG_SMP
---

I could try and redo the measurements again but I doubt it'll be any
different. Unless I haven't made a mistake somewhere...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1535115 — Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation

On Fri, Dec 2, 2016 at 10:03 AM, Borislav Petkov <bp@kernel.org> wrote:
>
> SNB:
> * before:
> * after:

I suspect it's entirely invisible on raw hardware. But quite possibly
more noticeable in a VM that takes slow faults for every case.

But yes, even there is' probably not *that* noticeable.

I'd prefer to get rid of them just because I don't like voodoo
programming. If there is no actual reason for "sync_core()", we
shouldn't have one there. Even if it were entirely free and the
compiler optimized it away because the compiler was so smart that it
would see that it's pointless, it's misleading and wrong on a source
level.

That is, of course, assuming that there  is no really subtle reason
why that stupid sync_core() is there.

      Linus

[toc] | [prev] | [next] | [standalone]

#1535124 — Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation

On Fri, Dec 02, 2016 at 10:27:29AM -0800, Linus Torvalds wrote:
> That is, of course, assuming that there  is no really subtle reason
> why that stupid sync_core() is there.

Right, we can try to do something like invalidate_icache() or so in
there with the JMP so that the BSP refetches modified code and see where
it gets us.

The good thing is, the early patching paths run before SMP is
up but from looking at load_module(), for example, which does
post_relocation()->module_finalize()->apply_alternatives(), this can
happen late.

Now there I'd like to avoid other cores walking into that address being
patched. Or are we "safe" there in the sense that load_module() happens
on one CPU only sequentially? (I haven't looked at that code to see
what's going on there, actually).

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1535128 — Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation

On Fri, Dec 2, 2016 at 10:50 AM, Borislav Petkov <bp@kernel.org> wrote:
>
> Right, we can try to do something like invalidate_icache() or so in
> there with the JMP so that the BSP refetches modified code and see where
> it gets us.

I'd really rather rjust mark it noinline with a comment. That way the
return from the function acts as the control flow change.

> The good thing is, the early patching paths run before SMP is
> up but from looking at load_module(), for example, which does
> post_relocation()->module_finalize()->apply_alternatives(), this can
> happen late.
>
> Now there I'd like to avoid other cores walking into that address being
> patched. Or are we "safe" there in the sense that load_module() happens
> on one CPU only sequentially? (I haven't looked at that code to see
> what's going on there, actually).

'sync_core()' doesn't help for other CPU's anyway, you need to do the
cross-call IPI. So worrying about other CPU's is *not* a valid reason
to keep a "sync_core()" call.

Seriously, the only reason I can see for "sync_core()" really is:

 - some deep non-serialized MSR  access or similar (ie things like
firmware loading etc really might want it, and a mchine check might
want it)

 - writing to one virtual address, and then executing from another. We
do this for (a) user mode (of course) and (b) text_poke(), but
text_poke() is a whole different dance.

but I may have forgotten some other case.

The issues with modifying code while another CPU may be just about to
access it is a separate issue. And as noted, "sync_core()" is not
sufficient for that, you have to do a whole careful dance with
single-byte debug instruction writes and then a final cross-call.

See the whole "text_poke_bp()" and "text_poke()" for *that* whole
dance. That's a much more complex thing from the normal
apply_alternatives().

             Linus

[toc] | [prev] | [next] | [standalone]

#1535145 — Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation

On Fri, Dec 2, 2016 at 11:20 AM, Borislav Petkov <bp@alien8.de> wrote:
>
> Something like below?

The optimize-nops thing needs it too, I think.

Again, this will never matter in practice (even if somebody has a i486
s till, the prefetch window size is like 16 bytes or something), but
from a documentation standpoint it's good.

              Linus

[toc] | [prev] | [next] | [standalone]

#1535147 — Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation

On Fri, Dec 02, 2016 at 11:24:09AM -0800, Linus Torvalds wrote:
> The optimize-nops thing needs it too, I think.

Ah, it is called only from apply_alternatives() but sure, it is safer
this way. Lemme do that and run it through the boxes to see whether
anything catches fire.

> Again, this will never matter in practice (even if somebody has a i486
> s till, the prefetch window size is like 16 bytes or something), but
> from a documentation standpoint it's good.

Right.

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1535433 — [PATCH] x86/alternatives: Do not use sync_core() to serialize I$

On Fri, Dec 02, 2016 at 08:28:44PM +0100, Borislav Petkov wrote:
> Ah, it is called only from apply_alternatives() but sure, it is safer
> this way. Lemme do that and run it through the boxes to see whether
> anything catches fire.

Looks good, ran it on a bunch of machines, two of them huge AMD and
Intel, 8-socket monsters.

Here is a version with a commit message:

---
From: Borislav Petkov <bp@suse.de>
Date: Fri, 2 Dec 2016 23:18:51 +0100
Subject: [PATCH] x86/alternatives: Do not use sync_core() to serialize I$

We use sync_core() in the alternatives code to stop speculative
execution of prefetched instructions because we are potentially changing
them and don't want to execute stale bytes.

What it does on most machines is call CPUID which is a serializing
instruction. And that's expensive.

However, the instruction cache is serialized when we're on the local CPU
and are changing the data through the same virtual address. So then, we
don't need the serializing CPUID but a simple control flow change. Last
being accomplished with a CALL/RET which the noinline causes.

So let's try it.

Signed-off-by: Borislav Petkov <bp@suse.de>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
---
 arch/x86/kernel/alternative.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 5cb272a7a5a3..c5b8f760473c 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -337,7 +337,11 @@ recompute_jump(struct alt_instr *a, u8 *orig_insn, u8 *repl_insn, u8 *insnbuf)
 		n_dspl, (unsigned long)orig_insn + n_dspl + repl_len);
 }
 
-static void __init_or_module optimize_nops(struct alt_instr *a, u8 *instr)
+/*
+ * "noinline" to cause control flow change and thus invalidate I$ and
+ * cause refetch after modification.
+ */
+static void __init_or_module noinline optimize_nops(struct alt_instr *a, u8 *instr)
 {
 	unsigned long flags;
 
@@ -346,7 +350,6 @@ static void __init_or_module optimize_nops(struct alt_instr *a, u8 *instr)
 
 	local_irq_save(flags);
 	add_nops(instr + (a->instrlen - a->padlen), a->padlen);
-	sync_core();
 	local_irq_restore(flags);
 
 	DUMP_BYTES(instr, a->instrlen, "%p: [%d:%d) optimized NOPs: ",
@@ -359,9 +362,12 @@ static void __init_or_module optimize_nops(struct alt_instr *a, u8 *instr)
  * This implies that asymmetric systems where APs have less capabilities than
  * the boot processor are not handled. Tough. Make sure you disable such
  * features by hand.
+ *
+ * Marked "noinline" to cause control flow change and thus insn cache
+ * to refetch changed I$ lines.
  */
-void __init_or_module apply_alternatives(struct alt_instr *start,
-					 struct alt_instr *end)
+void __init_or_module noinline apply_alternatives(struct alt_instr *start,
+						  struct alt_instr *end)
 {
 	struct alt_instr *a;
 	u8 *instr, *replacement;
@@ -667,7 +673,6 @@ void *__init_or_module text_poke_early(void *addr, const void *opcode,
 	unsigned long flags;
 	local_irq_save(flags);
 	memcpy(addr, opcode, len);
-	sync_core();
 	local_irq_restore(flags);
 	/* Could also do a CLFLUSH here to speed up CPU recovery; but
 	   that causes hangs on some VIA CPUs. */
-- 
2.11.0

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1535466 — Re: [PATCH] x86/alternatives: Do not use sync_core() to serialize I$

On Sat, Dec 3, 2016 at 7:02 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Fri, Dec 02, 2016 at 08:28:44PM +0100, Borislav Petkov wrote:
>> Ah, it is called only from apply_alternatives() but sure, it is safer
>> this way. Lemme do that and run it through the boxes to see whether
>> anything catches fire.
>
> Looks good, ran it on a bunch of machines, two of them huge AMD and
> Intel, 8-socket monsters.
>
> Here is a version with a commit message:
>
> ---
> From: Borislav Petkov <bp@suse.de>
> Date: Fri, 2 Dec 2016 23:18:51 +0100
> Subject: [PATCH] x86/alternatives: Do not use sync_core() to serialize I$
>
> We use sync_core() in the alternatives code to stop speculative
> execution of prefetched instructions because we are potentially changing
> them and don't want to execute stale bytes.
>
> What it does on most machines is call CPUID which is a serializing
> instruction. And that's expensive.
>
> However, the instruction cache is serialized when we're on the local CPU
> and are changing the data through the same virtual address. So then, we
> don't need the serializing CPUID but a simple control flow change. Last
> being accomplished with a CALL/RET which the noinline causes.
>
> So let's try it.

I like it.

Reviewed-by: Andy Lutomirski <luto@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1535155 — Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation

On Fri, Dec 2, 2016 at 11:30 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> How's this?

Looks ok. I do think that

> I suppose it could be an unconditional IRET-to-self, but that's a good
> deal slower and not a whole lot simpler.  Although if we start doing
> it right, performance won't really matter here.

Considering you already got the iret-to-self wrong in the first
version, I really like the "one single unconditional version" so that
everybody tests that _one_ thing and there isn't anything subtle going
on.

Hmm?

And yes, if it turns out that performance matters, we almost certainly
are doing something really wrong, and we shouldn't be using that
sync_core() thing in that place anyway.

                 Linus

[toc] | [prev] | [next] | [standalone]

#1535211 — Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation

On Fri, Dec 2, 2016 at 11:35 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Dec 2, 2016 at 11:30 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> How's this?
>
> Looks ok. I do think that
>
>> I suppose it could be an unconditional IRET-to-self, but that's a good
>> deal slower and not a whole lot simpler.  Although if we start doing
>> it right, performance won't really matter here.
>
> Considering you already got the iret-to-self wrong in the first
> version, I really like the "one single unconditional version" so that
> everybody tests that _one_ thing and there isn't anything subtle going
> on.
>
> Hmm?

Okay, sold.  It makes the patchset much much shorter, too.

>
> And yes, if it turns out that performance matters, we almost certainly
> are doing something really wrong, and we shouldn't be using that
> sync_core() thing in that place anyway.

To play devil's advocate (and definitely out of scope for this
particular patchset), is user code permitted to do:

1. Map a page RX at one address and RW somewhere else (for nice ASLR).
2. Write to the RW mapping.
3. CPUID or IRET-to-self.
4. Jump to the RX mapping.

Because, if so, we should maybe serialize whenever we migrate a
process to a different CPU.  (We *definitely* need to flush the store
buffer when migrating, because the x86 architecture makes some memory
ordering promises that get broken if a store from a thread stays in
the store buffer of a different CPU when the thread gets migrated.)
And if we're going to start serializing when migrating a thread, then
we actually care about performance, in which case we should optimize
the crap out of this thing, which probably means using MFENCE on AMD
CPUs (AMD promises that MFENCE flushes the pipeline.  Intel seems to
be confused as to exactly what effect MFENCE has, or at least I'm
confused as to what Intel thinks MFENCE does.)  And we should make
sure that we only do the extra flush when we don't switch mms.

--Andy

[toc] | [prev] | [next] | [standalone]

#1535220 — Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation

On Fri, Dec 2, 2016 at 12:41 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> Because, if so, we should maybe serialize whenever we migrate a
> process to a different CPU.

The intel docs are bad on this issue.

Technically what we do could fall under the "cross-modifying code"
case, where one CPU does the write, and then we run it on another CPU.

And no, we do *not* do a serializing instruction before returning to
user space. Sure, we might do an iret (which is serializing), but we
equally well might be doing a systret (which is not).

Honestly, I think Intel should clean up their documentation.

> (We *definitely* need to flush the store  buffer when migrating,

There is no such thing as flushing the store buffer.

But we do end up doing a memory barrier which gives you the required
semantics. That's not a problem. Those operations are fast. The
serializing instructions are not.

               Linus

[toc] | [prev] | [next] | [standalone]

#1535261 — Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation

On Fri, Dec 2, 2016 at 1:10 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Dec 2, 2016 at 12:41 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> Because, if so, we should maybe serialize whenever we migrate a
>> process to a different CPU.
>
> The intel docs are bad on this issue.
>
> Technically what we do could fall under the "cross-modifying code"
> case, where one CPU does the write, and then we run it on another CPU.
>
> And no, we do *not* do a serializing instruction before returning to
> user space. Sure, we might do an iret (which is serializing), but we
> equally well might be doing a systret (which is not).
>
> Honestly, I think Intel should clean up their documentation.
>

I'm not sure I follow.  If a user program gets migrated, it might end
up doing cross-modification when it expects self-modification.  If
that trips the program up, is that a user bug or a kernel bug?

Admittedly, I'd be very surprised if this happened in practice.
Migration is *slow*, caches tend to get blown away, lots of code gets
executed, etc.  Presumably any prefetched / trace cached / decoded /
i-cached user code is long gone when we migrate.

[toc] | [prev] | [next] | [standalone]

#1535267 — Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation

On Fri, Dec 2, 2016 at 2:55 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> Honestly, I think Intel should clean up their documentation.
>
> I'm not sure I follow.  If a user program gets migrated, it might end
> up doing cross-modification when it expects self-modification.  If
> that trips the program up, is that a user bug or a kernel bug?

Well, the user may not see it as a cross-modification.

Example: user compiles a program, and writes out the new binary. That
write goes to the page cache.

The user then immediately executes that program.

It's technically a "cross modification", because the build that wrote
the page cache ran on one CPU, and then it gets loaded on another.

Not, page faulting the binary does bring in a known serializing
instruction: iret.

But let's theorize that we have your "optimistic sysret" return path
because sometimes it can happen. So the "iret" isn't exactly
fundamental.

But we know we will write %cr2, which is a serializing instruction.

But that's not fundamental either, because we <i>could</i> just have a
program just load the object file into its own address space using the
dynamic linker. And if you don't unmap anything, there won't be any
TLB flushes.

Now, that is safe <i>too</I>, but by then we're not even relying on
simply the fact that the code couldn't even have been in any virtual
caches in the running environment, so it _must_ have come from the
physically indexed data cache. So no actual serializing instruction
even _needed_.

So there is no room for any cache I$ coherency issue at any point, but
note how we got to the point where we're now basically depending on
some fairly fundamental logic that is not in the Intel documentation?

THAT is what I don't like. I don't doubt for a moment that what we're
doing is entirely coherent, and we're fine. But the intel memory
ordering documentation simply doesn't cover this situation at all. The
"real" memory ordering documentation only covers the normal data
cache. And then they handwave the "self-modifying code" situation with
incomplete examples and just bullshit "you need a serializing
instruction", which clearly isn't actually the case, and is also
something that we very much don't even do.

It would be better if here was actual documentation, and we had some
nice clear "yeah, we don't need any stinking serializing instructions,
because we're already doing X".

              Linus

[toc] | [prev] | [next] | [standalone]

#1535157 — Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation

On Fri, Dec 2, 2016 at 11:24 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Dec 2, 2016 at 11:20 AM, Borislav Petkov <bp@alien8.de> wrote:
>>
>> Something like below?
>
> The optimize-nops thing needs it too, I think.
>
> Again, this will never matter in practice (even if somebody has a i486
> s till, the prefetch window size is like 16 bytes or something), but
> from a documentation standpoint it's good.

How's this?

/*
 * This function forces the icache and prefetched instruction stream to
 * catch up with reality in two very specific cases:
 *
 *  a) Text was modified using one virtual address and is about to be executed
 *     from the same physical page at a different virtual address.
 *
 *  b) Text was modified on a different CPU, may subsequently be
 *     executed on this CPU, and you want to make sure the new version
 *     gets executed.  This generally means you're calling this in a IPI.
 *
 * If you're calling this for a different reason, you're probably doing
 * it wrong.
 */
static inline void native_sync_core(void) { ... }

The body will do a MOV-to-CR2 followed by jmp 1f; 1:.  This sequence
should be guaranteed to flush the pipeline on any real CPU.  On Xen it
will do IRET-to-self.

I suppose it could be an unconditional IRET-to-self, but that's a good
deal slower and not a whole lot simpler.  Although if we start doing
it right, performance won't really matter here.

--Andy

[toc] | [prev] | [next] | [standalone]

#1535148 — Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation

On Fri, Dec 2, 2016 at 11:03 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Dec 2, 2016 at 10:50 AM, Borislav Petkov <bp@kernel.org> wrote:
>>
>> Right, we can try to do something like invalidate_icache() or so in
>> there with the JMP so that the BSP refetches modified code and see where
>> it gets us.
>
> I'd really rather rjust mark it noinline with a comment. That way the
> return from the function acts as the control flow change.
>
>> The good thing is, the early patching paths run before SMP is
>> up but from looking at load_module(), for example, which does
>> post_relocation()->module_finalize()->apply_alternatives(), this can
>> happen late.
>>
>> Now there I'd like to avoid other cores walking into that address being
>> patched. Or are we "safe" there in the sense that load_module() happens
>> on one CPU only sequentially? (I haven't looked at that code to see
>> what's going on there, actually).
>
> 'sync_core()' doesn't help for other CPU's anyway, you need to do the
> cross-call IPI. So worrying about other CPU's is *not* a valid reason
> to keep a "sync_core()" call.
>
> Seriously, the only reason I can see for "sync_core()" really is:
>
>  - some deep non-serialized MSR  access or similar (ie things like
> firmware loading etc really might want it, and a mchine check might
> want it)

Not even firmware loading wants it.  Firmware loading needs
specifically cpuid(eax=1).  It has nothing to do with serializing
anything -- it's just a CPU bug that was turned into "architecture".
I think it really is just cross-address or cross-core modification,
and I'll add a comment to that effect.

--Andy

[toc] | [prev] | [next] | [standalone]

#1535158 — Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation

On Fri, Dec 02, 2016 at 11:23:09AM -0800, Andy Lutomirski wrote:
> Not even firmware loading wants it.  Firmware loading needs

Microcode...

> specifically cpuid(eax=1).  It has nothing to do with serializing

... but yes, of course. NOT sync_core() but CPUID(1).

Thanks!

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1535149 — Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation

On Fri, Dec 02, 2016 at 11:03:50AM -0800, Linus Torvalds wrote:
> I'd really rather rjust mark it noinline with a comment. That way the
> return from the function acts as the control flow change.

Something like below?

It boots in a guest but that doesn't mean anything.

> 'sync_core()' doesn't help for other CPU's anyway, you need to do the
> cross-call IPI. So worrying about other CPU's is *not* a valid reason
> to keep a "sync_core()" call.

Yeah, no, I'm not crazy about it either - I was just sanity-checking all
call sites of apply_alternatives(). But as you say, we would've gotten
much bigger problems if other CPUs would walk in there on us.

> Seriously, the only reason I can see for "sync_core()" really is:
> 
>  - some deep non-serialized MSR  access or similar (ie things like
> firmware loading etc really might want it, and a mchine check might
> want it)

Yah, we do it in the #MC handler - apparently we need it there - and
in the microcode loader to tickle out the version of the microcode
currently applied into the MSR.

> The issues with modifying code while another CPU may be just about to
> access it is a separate issue. And as noted, "sync_core()" is not
> sufficient for that, you have to do a whole careful dance with
> single-byte debug instruction writes and then a final cross-call.
> 
> See the whole "text_poke_bp()" and "text_poke()" for *that* whole
> dance. That's a much more complex thing from the normal
> apply_alternatives().

Yeah.

---
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 5cb272a7a5a3..b1d0c35e6dcb 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -346,7 +346,6 @@ static void __init_or_module optimize_nops(struct alt_instr *a, u8 *instr)
 
 	local_irq_save(flags);
 	add_nops(instr + (a->instrlen - a->padlen), a->padlen);
-	sync_core();
 	local_irq_restore(flags);
 
 	DUMP_BYTES(instr, a->instrlen, "%p: [%d:%d) optimized NOPs: ",
@@ -359,9 +358,12 @@ static void __init_or_module optimize_nops(struct alt_instr *a, u8 *instr)
  * This implies that asymmetric systems where APs have less capabilities than
  * the boot processor are not handled. Tough. Make sure you disable such
  * features by hand.
+ *
+ * Marked "noinline" to cause control flow change and thus insn cache
+ * to refetch changed I$ lines.
  */
-void __init_or_module apply_alternatives(struct alt_instr *start,
-					 struct alt_instr *end)
+void __init_or_module noinline apply_alternatives(struct alt_instr *start,
+						  struct alt_instr *end)
 {
 	struct alt_instr *a;
 	u8 *instr, *replacement;
@@ -667,7 +669,6 @@ void *__init_or_module text_poke_early(void *addr, const void *opcode,
 	unsigned long flags;
 	local_irq_save(flags);
 	memcpy(addr, opcode, len);
-	sync_core();
 	local_irq_restore(flags);
 	/* Could also do a CLFLUSH here to speed up CPU recovery; but
 	   that causes hangs on some VIA CPUs. */

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1535113 — Re: [PATCH v2 5/6] x86/xen: Add a Xen-specific sync_core() implementation

On Fri, Dec 2, 2016 at 9:38 AM, Andy Lutomirski <luto@kernel.org> wrote:
>
> apply_alternatives, unfortunately.  It's performance-critical because
> it's intensely stupid and does sync_core() for every single patch.
> Fixing that would be nice, too.

So looking at text_poke_early(), that's very much a case that really
shouldn't need any "sync_core()" at all as far as I can tell.

Only the current CPU is running, and for local CPU I$ coherence all
you need is a jump instruction, and even that is only on really old
CPU's. From the PPro onwards (maybe even Pentium?) the I$ is entirely
serialized as long as you change the data using the same linear
address.

So at most, that function could mark itsel f"noinline" just to
guarantee that it will cause a control flow change before returning.
The sync_core() seems entirely bogus.

Same goes for optimize_nops() too.

              Linus

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web