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 1 of 3  [1] 2 3  Next page →

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

*** PATCHES 1 and 2 MAY BE 4.9 MATERIAL ***

Alan Cox pointed out that the 486 isn't the only supported CPU that
doesn't have CPUID.  Let's clean up the mess.

Patch 1 is intended to be an easy fix: it makes sync_core() work
without CPUID on all 32-bit kernels.  It should be quite safe.  This
will have a negligible performance cost during boot on kernels built
for newer CPUs.  With this in place, patch 2 reverts the buggy 486
check I added.

Patches 3 and 4 are meant to improve the situation.  Patch 3 cleans
up the Intel microcode loader and patch 4 (which depends on patch 3)
stops using CPUID in sync_core() altogether.

Andy Lutomirski (4):
  x86/asm/32: Make sync_core() handle missing CPUID on all 32-bit
    kernels
  Revert "x86/boot: Fail the boot if !M486 and CPUID is missing"
  x86/microcode/intel: Replace sync_core() with cpuid_eax(1)
  x86/asm: Change sync_core() to use MOV to CR2 to serialize

 arch/x86/boot/cpu.c                   |  6 ------
 arch/x86/include/asm/processor.h      | 31 ++++++++-----------------------
 arch/x86/kernel/cpu/microcode/intel.c |  6 +++---
 3 files changed, 11 insertions(+), 32 deletions(-)

-- 
2.9.3

[toc] | [next] | [standalone]

#1533552 — [PATCH 4/4] x86/asm: Change sync_core() to use MOV to CR2 to serialize

Aside from being excessively slow, CPUID is problematic: Linux runs
on a handful of CPUs that don't have CPUID.  MOV to CR2 is always
available, so use it instead.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/include/asm/processor.h | 31 ++++++++-----------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 64fbc937d586..0388f3d85700 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -593,31 +593,16 @@ static __always_inline void cpu_relax(void)
 /* Stop speculative execution and prefetching of modified code. */
 static inline void sync_core(void)
 {
-	int tmp;
-
-#ifdef CONFIG_X86_32
-	/*
-	 * Do a CPUID if available, otherwise do a jump.  The jump
-	 * can conveniently enough be the jump around CPUID.
-	 */
-	asm volatile("cmpl %2,%1\n\t"
-		     "jl 1f\n\t"
-		     "cpuid\n"
-		     "1:"
-		     : "=a" (tmp)
-		     : "rm" (boot_cpu_data.cpuid_level), "ri" (0), "0" (1)
-		     : "ebx", "ecx", "edx", "memory");
-#else
 	/*
-	 * CPUID is a barrier to speculative execution.
-	 * Prefetched instructions are automatically
-	 * invalidated when modified.
+	 * MOV to CR2 is architecturally defined as a serializing
+	 * instruction.  It's nice because it works on all CPUs, it
+	 * doesn't clobber registers, and (unlike CPUID) it won't force
+	 * a VM exit.
+	 *
+	 * 0xbf172b23 is random poison just in case something ends up
+	 * caring about this value.
 	 */
-	asm volatile("cpuid"
-		     : "=a" (tmp)
-		     : "0" (1)
-		     : "ebx", "ecx", "edx", "memory");
-#endif
+	write_cr2(0xbf172b23);
 }
 
 extern void select_idle_routine(const struct cpuinfo_x86 *c);
-- 
2.9.3

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

#1533923 — Re: [PATCH 4/4] x86/asm: Change sync_core() to use MOV to CR2 to serialize

On Wed, Nov 30, 2016 at 12:34:55PM -0800, Andy Lutomirski wrote:
> Aside from being excessively slow, CPUID is problematic: Linux runs
> on a handful of CPUs that don't have CPUID.  MOV to CR2 is always
> available, so use it instead.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/include/asm/processor.h | 31 ++++++++-----------------------
>  1 file changed, 8 insertions(+), 23 deletions(-)

Looks nice.

I'm wondering if we should leave this one in tip for an additional cycle
to have it tested on more hw. I know, it is architectural and so on but
who knows what every implementation actually does...

-- 
Regards/Gruss,
    Boris.

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

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

#1534295 — Re: [PATCH 4/4] x86/asm: Change sync_core() to use MOV to CR2 to serialize

On Thu, Dec 1, 2016 at 1:22 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Nov 30, 2016 at 12:34:55PM -0800, Andy Lutomirski wrote:
>> Aside from being excessively slow, CPUID is problematic: Linux runs
>> on a handful of CPUs that don't have CPUID.  MOV to CR2 is always
>> available, so use it instead.
>>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>  arch/x86/include/asm/processor.h | 31 ++++++++-----------------------
>>  1 file changed, 8 insertions(+), 23 deletions(-)
>
> Looks nice.
>
> I'm wondering if we should leave this one in tip for an additional cycle
> to have it tested on more hw. I know, it is architectural and so on but
> who knows what every implementation actually does...

I want the Xen opinion as well.

Xen folks, can Linux use write_cr2 to serialize the CPU core on Xen PV
or do we need something a bit heavier weight like native_write_cr2?

--Andy

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

#1534337 — Re: [PATCH 4/4] x86/asm: Change sync_core() to use MOV to CR2 to serialize

On 01/12/16 17:08, Andy Lutomirski wrote:
> On Thu, Dec 1, 2016 at 1:22 AM, Borislav Petkov <bp@alien8.de> wrote:
>> On Wed, Nov 30, 2016 at 12:34:55PM -0800, Andy Lutomirski wrote:
>>> Aside from being excessively slow, CPUID is problematic: Linux runs
>>> on a handful of CPUs that don't have CPUID.  MOV to CR2 is always
>>> available, so use it instead.
>>>
>>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>>> ---
>>>  arch/x86/include/asm/processor.h | 31 ++++++++-----------------------
>>>  1 file changed, 8 insertions(+), 23 deletions(-)
>> Looks nice.
>>
>> I'm wondering if we should leave this one in tip for an additional cycle
>> to have it tested on more hw. I know, it is architectural and so on but
>> who knows what every implementation actually does...
> I want the Xen opinion as well.
>
> Xen folks, can Linux use write_cr2 to serialize the CPU core on Xen PV
> or do we need something a bit heavier weight like native_write_cr2?

To sum up our conversation on IRC.

xen_write_cr2() is not serialising; it is just a write into a shared page.

native_write_cr2() would trap and be emulated.  This will incur #GP[0]
due to cpl, although not necessarily an iret on the way back out of Xen.

Something like an iret-to-self would be far quicker, and avoid trapping
into the hypervisor.

~Andrew

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

#1534717 — Re: [PATCH 4/4] x86/asm: Change sync_core() to use MOV to CR2 to serialize

* Borislav Petkov <bp@alien8.de> wrote:

> On Wed, Nov 30, 2016 at 12:34:55PM -0800, Andy Lutomirski wrote:
> > Aside from being excessively slow, CPUID is problematic: Linux runs
> > on a handful of CPUs that don't have CPUID.  MOV to CR2 is always
> > available, so use it instead.
> > 
> > Signed-off-by: Andy Lutomirski <luto@kernel.org>
> > ---
> >  arch/x86/include/asm/processor.h | 31 ++++++++-----------------------
> >  1 file changed, 8 insertions(+), 23 deletions(-)
> 
> Looks nice.
> 
> I'm wondering if we should leave this one in tip for an additional cycle
> to have it tested on more hw. I know, it is architectural and so on but
> who knows what every implementation actually does...

I think -tip and "upstream of the day" mostly gets tested on relatively recent x86 
hardware - proven by the fact that these regressions are many months old.

The reason v4.9 got extra testing is the announced Long Term Support (LTS) aspect: 
more, older, weirder hardware is being tested because it's going to be a very 
popular base kernel.

So the best option would be to get these fixes into -tip, make sure it's sane all 
around and works on hardware that gets tested on bleeding edge kernels, then push 
it upstream sooner rather than later and also have Cc:stable tags on the obvious 
fixes, and handle any eventual fallout as it happens.

That's the best we can do I think.

Thanks,

	Ingo

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

#1533554 — [PATCH 3/4] x86/microcode/intel: Replace sync_core() with cpuid_eax(1)

The Intel microcode driver is using sync_core() to mean "do CPUID
with EAX=1".  I want to rework sync_core(), but first the Intel
microcode driver needs to stop depending on its current behavior.

Reported-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/kernel/cpu/microcode/intel.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index cdc0deab00c9..2542d036a30e 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -385,7 +385,7 @@ static int collect_cpu_info_early(struct ucode_cpu_info *uci)
 	native_wrmsrl(MSR_IA32_UCODE_REV, 0);
 
 	/* As documented in the SDM: Do a CPUID 1 here */
-	sync_core();
+	cpuid_eax(1);
 
 	/* get the current revision from MSR 0x8B */
 	native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
@@ -627,7 +627,7 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
 	native_wrmsrl(MSR_IA32_UCODE_REV, 0);
 
 	/* As documented in the SDM: Do a CPUID 1 here */
-	sync_core();
+	cpuid_eax(1);
 
 	/* get the current revision from MSR 0x8B */
 	native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
@@ -927,7 +927,7 @@ static int apply_microcode_intel(int cpu)
 	wrmsrl(MSR_IA32_UCODE_REV, 0);
 
 	/* As documented in the SDM: Do a CPUID 1 here */
-	sync_core();
+	cpuid_eax(1);
 
 	/* get the current revision from MSR 0x8B */
 	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
-- 
2.9.3

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

#1533921 — Re: [PATCH 3/4] x86/microcode/intel: Replace sync_core() with cpuid_eax(1)

On Wed, Nov 30, 2016 at 12:34:54PM -0800, Andy Lutomirski wrote:
> The Intel microcode driver is using sync_core() to mean "do CPUID
> with EAX=1".  I want to rework sync_core(), but first the Intel
> microcode driver needs to stop depending on its current behavior.
> 
> Reported-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/kernel/cpu/microcode/intel.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Acked-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

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

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

#1534589 — [PATCH v2 6/6] x86/asm: Change sync_core() to use MOV to CR2 to serialize

Aside from being excessively slow, CPUID is problematic: Linux runs
on a handful of CPUs that don't have CPUID.  MOV to CR2 is always
available, so use it instead.

On my laptop, CPUID(eax=1, ecx=0) is ~83ns and MOV-to-CR2 is ~42ns,
so this should be a nice speedup.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/include/asm/processor.h | 31 ++++++++-----------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index c4402053c663..6727ed1c0ca0 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -594,31 +594,16 @@ static __always_inline void cpu_relax(void)
 /* Stop speculative execution and prefetching of modified code. */
 static inline void native_sync_core(void)
 {
-	int tmp;
-
-#ifdef CONFIG_X86_32
-	/*
-	 * Do a CPUID if available, otherwise do a jump.  The jump
-	 * can conveniently enough be the jump around CPUID.
-	 */
-	asm volatile("cmpl %2,%1\n\t"
-		     "jl 1f\n\t"
-		     "cpuid\n"
-		     "1:"
-		     : "=a" (tmp)
-		     : "rm" (boot_cpu_data.cpuid_level), "ri" (0), "0" (1)
-		     : "ebx", "ecx", "edx", "memory");
-#else
 	/*
-	 * CPUID is a barrier to speculative execution.
-	 * Prefetched instructions are automatically
-	 * invalidated when modified.
+	 * MOV to CR2 is architecturally defined as a serializing
+	 * instruction.  It's nice because it works on all CPUs, it
+	 * doesn't clobber registers, and (unlike CPUID) it won't force
+	 * a VM exit.
+	 *
+	 * 0xbf172b23 is random poison just in case something ends up
+	 * caring about this value.
 	 */
-	asm volatile("cpuid"
-		     : "=a" (tmp)
-		     : "0" (1)
-		     : "ebx", "ecx", "edx", "memory");
-#endif
+	native_write_cr2(0xbf172b23);
 }
 
 extern void select_idle_routine(const struct cpuinfo_x86 *c);
-- 
2.9.3

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

#1534590 — [PATCH v2 4/6] x86/paravirt: Make sync_core() be a paravirt op

I want to change sync_core() to use MOV to CR2, but that won't work
the way we want on Xen PV, and the easiest fix is to make
sync_core() be a paravirt op.  Make it so.

A real paravirt guru could probably microoptimize this.  I doubt it
matters much, though, as sync_core() is mostly used during boot.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/include/asm/paravirt.h       | 5 +++++
 arch/x86/include/asm/paravirt_types.h | 2 ++
 arch/x86/include/asm/processor.h      | 3 ++-
 arch/x86/kernel/paravirt.c            | 2 ++
 4 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index ce932812f142..7e76b72aa698 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -28,6 +28,11 @@ static inline void __cpuid(unsigned int *eax, unsigned int *ebx,
 	PVOP_VCALL4(pv_cpu_ops.cpuid, eax, ebx, ecx, edx);
 }
 
+static inline void sync_core(void)
+{
+	PVOP_VCALL0(pv_cpu_ops.sync_core);
+}
+
 /*
  * These special macros can be used to get or set a debugging register
  */
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index 0f400c0e4979..e4d2cb2c0165 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -177,6 +177,8 @@ struct pv_cpu_ops {
 
 	void (*start_context_switch)(struct task_struct *prev);
 	void (*end_context_switch)(struct task_struct *next);
+
+	void (*sync_core)(void);
 };
 
 struct pv_irq_ops {
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 64fbc937d586..c4402053c663 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -507,6 +507,7 @@ static inline void load_sp0(struct tss_struct *tss,
 }
 
 #define set_iopl_mask native_set_iopl_mask
+#define sync_core native_sync_core
 #endif /* CONFIG_PARAVIRT */
 
 /* Free all resources held by a thread. */
@@ -591,7 +592,7 @@ static __always_inline void cpu_relax(void)
 #define cpu_relax_lowlatency() cpu_relax()
 
 /* Stop speculative execution and prefetching of modified code. */
-static inline void sync_core(void)
+static inline void native_sync_core(void)
 {
 	int tmp;
 
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index bbf3d5933eaa..4d6a20ecbc78 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -373,6 +373,8 @@ __visible struct pv_cpu_ops pv_cpu_ops = {
 
 	.start_context_switch = paravirt_nop,
 	.end_context_switch = paravirt_nop,
+
+	.sync_core = native_sync_core,
 };
 
 /* At this point, native_get/set_debugreg has real function entries */
-- 
2.9.3

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

#1534592 — [PATCH v2 3/6] x86/microcode/intel: Replace sync_core() with cpuid_eax(1)

The Intel microcode driver is using sync_core() to mean "do CPUID
with EAX=1".  I want to rework sync_core(), but first the Intel
microcode driver needs to stop depending on its current behavior.

Reported-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Acked-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/kernel/cpu/microcode/intel.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index cdc0deab00c9..2542d036a30e 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -385,7 +385,7 @@ static int collect_cpu_info_early(struct ucode_cpu_info *uci)
 	native_wrmsrl(MSR_IA32_UCODE_REV, 0);
 
 	/* As documented in the SDM: Do a CPUID 1 here */
-	sync_core();
+	cpuid_eax(1);
 
 	/* get the current revision from MSR 0x8B */
 	native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
@@ -627,7 +627,7 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
 	native_wrmsrl(MSR_IA32_UCODE_REV, 0);
 
 	/* As documented in the SDM: Do a CPUID 1 here */
-	sync_core();
+	cpuid_eax(1);
 
 	/* get the current revision from MSR 0x8B */
 	native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
@@ -927,7 +927,7 @@ static int apply_microcode_intel(int cpu)
 	wrmsrl(MSR_IA32_UCODE_REV, 0);
 
 	/* As documented in the SDM: Do a CPUID 1 here */
-	sync_core();
+	cpuid_eax(1);
 
 	/* get the current revision from MSR 0x8B */
 	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
-- 
2.9.3

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

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

On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't
guaranteed to serialize.  (Even CPUID isn't *really* guaranteed to
serialize on Xen PV, but, in practice, any trap it generates will
serialize.)

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.

Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/xen/enlighten.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index bdd855685403..1f765b41eee7 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -311,6 +311,39 @@ static __read_mostly unsigned int cpuid_leaf1_ecx_set_mask;
 static __read_mostly unsigned int cpuid_leaf5_ecx_val;
 static __read_mostly unsigned int cpuid_leaf5_edx_val;
 
+static void xen_sync_core(void)
+{
+	register void *__sp asm(_ASM_SP);
+
+#ifdef CONFIG_X86_32
+	asm volatile (
+		"pushl %%ss\n\t"
+		"pushl %%esp\n\t"
+		"addl $4, (%%esp)\n\t"
+		"pushfl\n\t"
+		"pushl %%cs\n\t"
+		"pushl $1f\n\t"
+		"iret\n\t"
+		"1:"
+		: "+r" (__sp) : : "cc");
+#else
+	unsigned long tmp;
+
+	asm volatile (
+		"movq %%ss, %0\n\t"
+		"pushq %0\n\t"
+		"pushq %%rsp\n\t"
+		"addq $8, (%%rsp)\n\t"
+		"pushfq\n\t"
+		"movq %%cs, %0\n\t"
+		"pushq %0\n\t"
+		"pushq $1f\n\t"
+		"iretq\n\t"
+		"1:"
+		: "=r" (tmp), "+r" (__sp) : : "cc");
+#endif
+}
+
 static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 		      unsigned int *cx, unsigned int *dx)
 {
@@ -1289,6 +1322,8 @@ static const struct pv_cpu_ops xen_cpu_ops __initconst = {
 
 	.start_context_switch = paravirt_start_context_switch,
 	.end_context_switch = xen_end_context_switch,
+
+	.sync_core = xen_sync_core,
 };
 
 static void xen_reboot(int reason)
-- 
2.9.3

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

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

On 02/12/16 00:35, Andy Lutomirski wrote:
> On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't
> guaranteed to serialize.  (Even CPUID isn't *really* guaranteed to
> serialize on Xen PV, but, in practice, any trap it generates will
> serialize.)

Well, Xen will enabled CPUID Faulting wherever it can, which is
realistically all IvyBridge hardware and newer.

All hypercalls are a privilege change to cpl0.  I'd hope this condition
is serialising, but I can't actually find any documentation proving or
disproving this.

>
> 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.
>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>

CC'ing xen-devel and the Xen maintainers in Linux.

As this is the only email from this series in my inbox, I will say this
here, but it should really be against patch 6.

A write to %cr2 is apparently (http://sandpile.org/x86/coherent.htm) not
serialising on the 486, but I don't have a manual to hand to check.

~Andrew

> ---
>  arch/x86/xen/enlighten.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index bdd855685403..1f765b41eee7 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -311,6 +311,39 @@ static __read_mostly unsigned int cpuid_leaf1_ecx_set_mask;
>  static __read_mostly unsigned int cpuid_leaf5_ecx_val;
>  static __read_mostly unsigned int cpuid_leaf5_edx_val;
>  
> +static void xen_sync_core(void)
> +{
> +	register void *__sp asm(_ASM_SP);
> +
> +#ifdef CONFIG_X86_32
> +	asm volatile (
> +		"pushl %%ss\n\t"
> +		"pushl %%esp\n\t"
> +		"addl $4, (%%esp)\n\t"
> +		"pushfl\n\t"
> +		"pushl %%cs\n\t"
> +		"pushl $1f\n\t"
> +		"iret\n\t"
> +		"1:"
> +		: "+r" (__sp) : : "cc");
> +#else
> +	unsigned long tmp;
> +
> +	asm volatile (
> +		"movq %%ss, %0\n\t"
> +		"pushq %0\n\t"
> +		"pushq %%rsp\n\t"
> +		"addq $8, (%%rsp)\n\t"
> +		"pushfq\n\t"
> +		"movq %%cs, %0\n\t"
> +		"pushq %0\n\t"
> +		"pushq $1f\n\t"
> +		"iretq\n\t"
> +		"1:"
> +		: "=r" (tmp), "+r" (__sp) : : "cc");
> +#endif
> +}
> +
>  static void xen_cpuid(unsigned int *ax, unsigned int *bx,
>  		      unsigned int *cx, unsigned int *dx)
>  {
> @@ -1289,6 +1322,8 @@ static const struct pv_cpu_ops xen_cpu_ops __initconst = {
>  
>  	.start_context_switch = paravirt_start_context_switch,
>  	.end_context_switch = xen_end_context_switch,
> +
> +	.sync_core = xen_sync_core,
>  };
>  
>  static void xen_reboot(int reason)

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

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

On Dec 2, 2016 3:44 AM, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:
>
> On 02/12/16 00:35, Andy Lutomirski wrote:
> > On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't
> > guaranteed to serialize.  (Even CPUID isn't *really* guaranteed to
> > serialize on Xen PV, but, in practice, any trap it generates will
> > serialize.)
>
> Well, Xen will enabled CPUID Faulting wherever it can, which is
> realistically all IvyBridge hardware and newer.
>
> All hypercalls are a privilege change to cpl0.  I'd hope this condition
> is serialising, but I can't actually find any documentation proving or
> disproving this.

I don't know for sure.  IRET is serializing, and if Xen returns using
IRET, we're fine.

>
> >
> > 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.
> >
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Signed-off-by: Andy Lutomirski <luto@kernel.org>
>
> CC'ing xen-devel and the Xen maintainers in Linux.
>
> As this is the only email from this series in my inbox, I will say this
> here, but it should really be against patch 6.
>
> A write to %cr2 is apparently (http://sandpile.org/x86/coherent.htm) not
> serialising on the 486, but I don't have a manual to hand to check.

I'll quote the (modern) SDM.  For self-modifying code "The use of one
of these options is not required for programs intended to run on the
Pentium or Intel486 processors,
but are recommended to ensure compatibility with the P6 and more
recent processor families.".  For cross-modifying code "The use of
this option is not required for programs intended to run on the
Intel486 processor, but is recommended
to ensure compatibility with the Pentium 4, Intel Xeon, P6 family, and
Pentium processors."  So I'm not sure there's a problem.

I can add an unconditional jump just to make sure.  It costs basically
nothing on modern CPUs.  (Also, CPUID also isn't serializing on 486
according to the table.)

--Andy

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

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

On 02/12/16 17:07, Andy Lutomirski wrote:
> On Dec 2, 2016 3:44 AM, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:
>> On 02/12/16 00:35, Andy Lutomirski wrote:
>>> On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't
>>> guaranteed to serialize.  (Even CPUID isn't *really* guaranteed to
>>> serialize on Xen PV, but, in practice, any trap it generates will
>>> serialize.)
>> Well, Xen will enabled CPUID Faulting wherever it can, which is
>> realistically all IvyBridge hardware and newer.
>>
>> All hypercalls are a privilege change to cpl0.  I'd hope this condition
>> is serialising, but I can't actually find any documentation proving or
>> disproving this.
> I don't know for sure.  IRET is serializing, and if Xen returns using
> IRET, we're fine.

All returns to a 64bit PV guest at defined points (hypercall return,
exception entry, etc) are from SYSRET, not IRET.

Talking of, I still have a patch to remove
PARAVIRT_ADJUST_EXCEPTION_FRAME which I need to complete and send upstream.

>
>>> 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.
>>>
>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> CC'ing xen-devel and the Xen maintainers in Linux.
>>
>> As this is the only email from this series in my inbox, I will say this
>> here, but it should really be against patch 6.
>>
>> A write to %cr2 is apparently (http://sandpile.org/x86/coherent.htm) not
>> serialising on the 486, but I don't have a manual to hand to check.
> I'll quote the (modern) SDM.  For self-modifying code "The use of one
> of these options is not required for programs intended to run on the
> Pentium or Intel486 processors,
> but are recommended to ensure compatibility with the P6 and more
> recent processor families.".  For cross-modifying code "The use of
> this option is not required for programs intended to run on the
> Intel486 processor, but is recommended
> to ensure compatibility with the Pentium 4, Intel Xeon, P6 family, and
> Pentium processors."  So I'm not sure there's a problem.

Fair enough.  (Assuming similar properties hold for the older processors
of other vendors.)

~Andrew

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

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

On 02/12/16 17:23, Andy Lutomirski wrote:
> On Fri, Dec 2, 2016 at 9:16 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 02/12/16 17:07, Andy Lutomirski wrote:
>>> On Dec 2, 2016 3:44 AM, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:
>>>> On 02/12/16 00:35, Andy Lutomirski wrote:
>>>>> On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't
>>>>> guaranteed to serialize.  (Even CPUID isn't *really* guaranteed to
>>>>> serialize on Xen PV, but, in practice, any trap it generates will
>>>>> serialize.)
>>>> Well, Xen will enabled CPUID Faulting wherever it can, which is
>>>> realistically all IvyBridge hardware and newer.
>>>>
>>>> All hypercalls are a privilege change to cpl0.  I'd hope this condition
>>>> is serialising, but I can't actually find any documentation proving or
>>>> disproving this.
>>> I don't know for sure.  IRET is serializing, and if Xen returns using
>>> IRET, we're fine.
>> All returns to a 64bit PV guest at defined points (hypercall return,
>> exception entry, etc) are from SYSRET, not IRET.
> But CPUID faulting isn't like this, right?  Unless Xen does
> opportunistic SYSRET.

Correct.  Xen doesn't do opportunistic SYSRET.

>
>> Talking of, I still have a patch to remove
>> PARAVIRT_ADJUST_EXCEPTION_FRAME which I need to complete and send upstream.
>>
>>>>> 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.
>>>>>
>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>>>> CC'ing xen-devel and the Xen maintainers in Linux.
>>>>
>>>> As this is the only email from this series in my inbox, I will say this
>>>> here, but it should really be against patch 6.
>>>>
>>>> A write to %cr2 is apparently (http://sandpile.org/x86/coherent.htm) not
>>>> serialising on the 486, but I don't have a manual to hand to check.
>>> I'll quote the (modern) SDM.  For self-modifying code "The use of one
>>> of these options is not required for programs intended to run on the
>>> Pentium or Intel486 processors,
>>> but are recommended to ensure compatibility with the P6 and more
>>> recent processor families.".  For cross-modifying code "The use of
>>> this option is not required for programs intended to run on the
>>> Intel486 processor, but is recommended
>>> to ensure compatibility with the Pentium 4, Intel Xeon, P6 family, and
>>> Pentium processors."  So I'm not sure there's a problem.
>> Fair enough.  (Assuming similar properties hold for the older processors
>> of other vendors.)
> No, you were right -- a different section of the SDM contradicts it:
>
> For Intel486 processors, a write to an instruction in the cache will
> modify it in both the cache and memory, but if
> the instruction was prefetched before the write, the old version of
> the instruction could be the one executed. To
> prevent the old instruction from being executed, flush the instruction
> prefetch unit by coding a jump instruction
> immediately after any write that modifies an instruction.

:(

Presumably this means patching has been subtly broken forever on the 486?

~Andrew

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

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

On Fri, Dec 2, 2016 at 9:16 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 02/12/16 17:07, Andy Lutomirski wrote:
>> On Dec 2, 2016 3:44 AM, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:
>>> On 02/12/16 00:35, Andy Lutomirski wrote:
>>>> On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't
>>>> guaranteed to serialize.  (Even CPUID isn't *really* guaranteed to
>>>> serialize on Xen PV, but, in practice, any trap it generates will
>>>> serialize.)
>>> Well, Xen will enabled CPUID Faulting wherever it can, which is
>>> realistically all IvyBridge hardware and newer.
>>>
>>> All hypercalls are a privilege change to cpl0.  I'd hope this condition
>>> is serialising, but I can't actually find any documentation proving or
>>> disproving this.
>> I don't know for sure.  IRET is serializing, and if Xen returns using
>> IRET, we're fine.
>
> All returns to a 64bit PV guest at defined points (hypercall return,
> exception entry, etc) are from SYSRET, not IRET.

But CPUID faulting isn't like this, right?  Unless Xen does
opportunistic SYSRET.

>
> Talking of, I still have a patch to remove
> PARAVIRT_ADJUST_EXCEPTION_FRAME which I need to complete and send upstream.
>
>>
>>>> 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.
>>>>
>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>>> CC'ing xen-devel and the Xen maintainers in Linux.
>>>
>>> As this is the only email from this series in my inbox, I will say this
>>> here, but it should really be against patch 6.
>>>
>>> A write to %cr2 is apparently (http://sandpile.org/x86/coherent.htm) not
>>> serialising on the 486, but I don't have a manual to hand to check.
>> I'll quote the (modern) SDM.  For self-modifying code "The use of one
>> of these options is not required for programs intended to run on the
>> Pentium or Intel486 processors,
>> but are recommended to ensure compatibility with the P6 and more
>> recent processor families.".  For cross-modifying code "The use of
>> this option is not required for programs intended to run on the
>> Intel486 processor, but is recommended
>> to ensure compatibility with the Pentium 4, Intel Xeon, P6 family, and
>> Pentium processors."  So I'm not sure there's a problem.
>
> Fair enough.  (Assuming similar properties hold for the older processors
> of other vendors.)

No, you were right -- a different section of the SDM contradicts it:

For Intel486 processors, a write to an instruction in the cache will
modify it in both the cache and memory, but if
the instruction was prefetched before the write, the old version of
the instruction could be the one executed. To
prevent the old instruction from being executed, flush the instruction
prefetch unit by coding a jump instruction
immediately after any write that modifies an instruction.

--Andy

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

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

On 12/02/2016 06:44 AM, Andrew Cooper wrote:
> On 02/12/16 00:35, Andy Lutomirski wrote:
>> On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't
>> guaranteed to serialize.  (Even CPUID isn't *really* guaranteed to
>> serialize on Xen PV, but, in practice, any trap it generates will
>> serialize.)
> Well, Xen will enabled CPUID Faulting wherever it can, which is
> realistically all IvyBridge hardware and newer.
>
> All hypercalls are a privilege change to cpl0.  I'd hope this condition
> is serialising, but I can't actually find any documentation proving or
> disproving this.
>
>> 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.
>>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> CC'ing xen-devel and the Xen maintainers in Linux.
>
> As this is the only email from this series in my inbox, I will say this
> here, but it should really be against patch 6.
>
> A write to %cr2 is apparently (http://sandpile.org/x86/coherent.htm) not
> serialising on the 486, but I don't have a manual to hand to check.
>
> ~Andrew
>
>> ---
>>  arch/x86/xen/enlighten.c | 35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>
>> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
>> index bdd855685403..1f765b41eee7 100644
>> --- a/arch/x86/xen/enlighten.c
>> +++ b/arch/x86/xen/enlighten.c
>> @@ -311,6 +311,39 @@ static __read_mostly unsigned int cpuid_leaf1_ecx_set_mask;
>>  static __read_mostly unsigned int cpuid_leaf5_ecx_val;
>>  static __read_mostly unsigned int cpuid_leaf5_edx_val;
>>  
>> +static void xen_sync_core(void)
>> +{
>> +	register void *__sp asm(_ASM_SP);
>> +
>> +#ifdef CONFIG_X86_32
>> +	asm volatile (
>> +		"pushl %%ss\n\t"
>> +		"pushl %%esp\n\t"
>> +		"addl $4, (%%esp)\n\t"
>> +		"pushfl\n\t"
>> +		"pushl %%cs\n\t"
>> +		"pushl $1f\n\t"
>> +		"iret\n\t"
>> +		"1:"
>> +		: "+r" (__sp) : : "cc");

This breaks 32-bit PV guests.

Why are we pushing %ss? We are not changing privilege levels so why not
just flags, cs and eip (which, incidentally, does work)?

-boris

>> +#else
>> +	unsigned long tmp;
>> +
>> +	asm volatile (
>> +		"movq %%ss, %0\n\t"
>> +		"pushq %0\n\t"
>> +		"pushq %%rsp\n\t"
>> +		"addq $8, (%%rsp)\n\t"
>> +		"pushfq\n\t"
>> +		"movq %%cs, %0\n\t"
>> +		"pushq %0\n\t"
>> +		"pushq $1f\n\t"
>> +		"iretq\n\t"
>> +		"1:"
>> +		: "=r" (tmp), "+r" (__sp) : : "cc");
>> +#endif
>> +}
>> +
>>  static void xen_cpuid(unsigned int *ax, unsigned int *bx,
>>  		      unsigned int *cx, unsigned int *dx)
>>  {
>> @@ -1289,6 +1322,8 @@ static const struct pv_cpu_ops xen_cpu_ops __initconst = {
>>  
>>  	.start_context_switch = paravirt_start_context_switch,
>>  	.end_context_switch = xen_end_context_switch,
>> +
>> +	.sync_core = xen_sync_core,
>>  };
>>  
>>  static void xen_reboot(int reason)

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

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

On Dec 2, 2016 10:48 AM, "Boris Ostrovsky" <boris.ostrovsky@oracle.com> wrote:
>
> On 12/02/2016 06:44 AM, Andrew Cooper wrote:
> > On 02/12/16 00:35, Andy Lutomirski wrote:
> >> On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't
> >> guaranteed to serialize.  (Even CPUID isn't *really* guaranteed to
> >> serialize on Xen PV, but, in practice, any trap it generates will
> >> serialize.)
> > Well, Xen will enabled CPUID Faulting wherever it can, which is
> > realistically all IvyBridge hardware and newer.
> >
> > All hypercalls are a privilege change to cpl0.  I'd hope this condition
> > is serialising, but I can't actually find any documentation proving or
> > disproving this.
> >
> >> 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.
> >>
> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> > CC'ing xen-devel and the Xen maintainers in Linux.
> >
> > As this is the only email from this series in my inbox, I will say this
> > here, but it should really be against patch 6.
> >
> > A write to %cr2 is apparently (http://sandpile.org/x86/coherent.htm) not
> > serialising on the 486, but I don't have a manual to hand to check.
> >
> > ~Andrew
> >
> >> ---
> >>  arch/x86/xen/enlighten.c | 35 +++++++++++++++++++++++++++++++++++
> >>  1 file changed, 35 insertions(+)
> >>
> >> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> >> index bdd855685403..1f765b41eee7 100644
> >> --- a/arch/x86/xen/enlighten.c
> >> +++ b/arch/x86/xen/enlighten.c
> >> @@ -311,6 +311,39 @@ static __read_mostly unsigned int cpuid_leaf1_ecx_set_mask;
> >>  static __read_mostly unsigned int cpuid_leaf5_ecx_val;
> >>  static __read_mostly unsigned int cpuid_leaf5_edx_val;
> >>
> >> +static void xen_sync_core(void)
> >> +{
> >> +    register void *__sp asm(_ASM_SP);
> >> +
> >> +#ifdef CONFIG_X86_32
> >> +    asm volatile (
> >> +            "pushl %%ss\n\t"
> >> +            "pushl %%esp\n\t"
> >> +            "addl $4, (%%esp)\n\t"
> >> +            "pushfl\n\t"
> >> +            "pushl %%cs\n\t"
> >> +            "pushl $1f\n\t"
> >> +            "iret\n\t"
> >> +            "1:"
> >> +            : "+r" (__sp) : : "cc");
>
> This breaks 32-bit PV guests.
>
> Why are we pushing %ss? We are not changing privilege levels so why not
> just flags, cs and eip (which, incidentally, does work)?
>

Doh!  I carefully tested 64-bit on Xen and 32-bit in user mode.

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

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

On 12/02/2016 06:44 AM, Andrew Cooper wrote:
> On 02/12/16 00:35, Andy Lutomirski wrote:
>> On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't
>> guaranteed to serialize.  (Even CPUID isn't *really* guaranteed to
>> serialize on Xen PV, but, in practice, any trap it generates will
>> serialize.)
> Well, Xen will enabled CPUID Faulting wherever it can, which is
> realistically all IvyBridge hardware and newer.
>
> All hypercalls are a privilege change to cpl0.  I'd hope this condition
> is serialising, but I can't actually find any documentation proving or
> disproving this.
>
>> 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.
>>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>

Executing CPUID in an HVM guest is quite expensive since it will cause a
VMEXIT. (And that should be true for any hypervisor, at least on Intel.
On AMD it's configurable)

-boris

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

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web