Re: [PATCH] x86/boot: Fail the boot if !M486 and CPUID is missing

[Andy Lutomirski]

On Wed, Nov 30, 2016 at 5:18 AM, One Thousand Gnomes
 wrote:
>> Rather than trying to work around these issues, just have the kernel
>> fail loudly if it's running on a CPUID-less 486, doesn't have CPUID,
>> and doesn't have CONFIG_M486 set.
>
> NAK
>
> This still breaks the Geode at the very least and I think the ELAN and
> some of the other older socket 7 devices. These have their own config CPU
> type (and in some cases *need* it selected) but do not have CPUID.
>
> Given the cores without CPUID are often post 486 in other respects this
> seems a bit odd. In fact I can't help thinking that the problem is that
> sync_core tests CONFIG_M486 whereas we should have and test
>
> HAVE_CPUID
>
> and set this by processor type (M586/M486/GEODEGX1/GEODE_LX1/Cyrix plus I
> think ELAN not having it)
>
> I'd been wondering why Geode wasn't working on modern kernels, now I
> think I know 8)
>

Ick.  Am I understanding correctly that this isn't a regression per se
since the affected machines were already broken?  If so, let's fix it
for real rather than just reverting this patch.

--Andy

Re: [PATCH] x86/boot: Fail the boot if !M486 and CPUID is missing

[Andy Lutomirski]

On Wed, Nov 30, 2016 at 5:18 AM, One Thousand Gnomes
 wrote:
>> Rather than trying to work around these issues, just have the kernel
>> fail loudly if it's running on a CPUID-less 486, doesn't have CPUID,
>> and doesn't have CONFIG_M486 set.
>
> NAK
>
> This still breaks the Geode at the very least and I think the ELAN and
> some of the other older socket 7 devices. These have their own config CPU
> type (and in some cases *need* it selected) but do not have CPUID.
>
> Given the cores without CPUID are often post 486 in other respects this
> seems a bit odd. In fact I can't help thinking that the problem is that
> sync_core tests CONFIG_M486 whereas we should have and test
>
> HAVE_CPUID
>
> and set this by processor type (M586/M486/GEODEGX1/GEODE_LX1/Cyrix plus I
> think ELAN not having it)
>
> I'd been wondering why Geode wasn't working on modern kernels, now I
> think I know 8)
>

Ick.  Am I understanding correctly that this isn't a regression per se
since the affected machines were already broken?  If so, let's fix it
for real rather than just reverting this patch.

--Andy

Re: [PATCH] x86/boot: Fail the boot if !M486 and CPUID is missing

[One Thousand Gnomes]

> Rather than trying to work around these issues, just have the kernel
> fail loudly if it's running on a CPUID-less 486, doesn't have CPUID,
> and doesn't have CONFIG_M486 set.

NAK

This still breaks the Geode at the very least and I think the ELAN and
some of the other older socket 7 devices. These have their own config CPU
type (and in some cases *need* it selected) but do not have CPUID.

Given the cores without CPUID are often post 486 in other respects this
seems a bit odd. In fact I can't help thinking that the problem is that
sync_core tests CONFIG_M486 whereas we should have and test

HAVE_CPUID

and set this by processor type (M586/M486/GEODEGX1/GEODE_LX1/Cyrix plus I
think ELAN not having it)

I'd been wondering why Geode wasn't working on modern kernels, now I
think I know 8)

Alan

Re: [PATCH] x86/boot: Fail the boot if !M486 and CPUID is missing

[One Thousand Gnomes]

> Rather than trying to work around these issues, just have the kernel
> fail loudly if it's running on a CPUID-less 486, doesn't have CPUID,
> and doesn't have CONFIG_M486 set.

NAK

This still breaks the Geode at the very least and I think the ELAN and
some of the other older socket 7 devices. These have their own config CPU
type (and in some cases *need* it selected) but do not have CPUID.

Given the cores without CPUID are often post 486 in other respects this
seems a bit odd. In fact I can't help thinking that the problem is that
sync_core tests CONFIG_M486 whereas we should have and test

HAVE_CPUID

and set this by processor type (M586/M486/GEODEGX1/GEODE_LX1/Cyrix plus I
think ELAN not having it)

I'd been wondering why Geode wasn't working on modern kernels, now I
think I know 8)

Alan

Re: [PATCH] x86/boot: Fail the boot if !M486 and CPUID is missing

[Borislav Petkov]

On Sun, Nov 20, 2016 at 05:34:43PM -0200, Henrique de Moraes Holschuh wrote:
> On Sun, 20 Nov 2016, Borislav Petkov wrote:
> > We will have set (or not) the X86_FEATURE_CPUID bit at
> > early_identify_cpu() time. Looking at the code, we do call sync_core()
> > pretty early. :-\
> 
> Hmm, watch out for the early microcode update driver for Intel
> processors should something get changed in the implementation, or in the
> behavior of sync_core().

That's exactly what I had in mind when I wrote the above sentence...

> That driver absolutely needs to issue a cpuid (with EAX = 1) before each
> rdmsr(MSR_IA32_UCODE_REV).  And it uses sync_core() calls to do it.  A
> CR2 access just won't do in this extremely specific case.
> 
> This kind of pitfall is why I wanted to replace all use of sync_core()
> in arch/x86/kernel/cpu/microcode/intel.c with an explicit use of an
> inconditional cpuid(eax = 1)...
> 
> (note: this protocol to read MSR_IA32_UCODE_REV was made an
> architectural requirement a while ago -- it was once considered an
> erratum workaround.  It is documented in the "Intel 64 and IA‐32
> Architectures Software Developer's Manual", Volume 3A: System
> Programming Guide, Part 1, section 9.11).

Well, I'm looking at this:

"CPUID returns a value in a model specific register in addition to its
usual register return values. The semantics of CPUID cause it to deposit
an update ID value in the 64-bit model-specific register at address 08BH
(IA32_BIOS_SIGN_ID)."

And I think yes, we should do an explicit CPUID(1) regardless of
what happens to sync_core(). Feel free to send a patch against
tip:x86/microcode.

-- 
Regards/Gruss,
Boris.

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

Re: [PATCH] x86/boot: Fail the boot if !M486 and CPUID is missing

[Borislav Petkov]

On Sun, Nov 20, 2016 at 05:34:43PM -0200, Henrique de Moraes Holschuh wrote:
> On Sun, 20 Nov 2016, Borislav Petkov wrote:
> > We will have set (or not) the X86_FEATURE_CPUID bit at
> > early_identify_cpu() time. Looking at the code, we do call sync_core()
> > pretty early. :-\
> 
> Hmm, watch out for the early microcode update driver for Intel
> processors should something get changed in the implementation, or in the
> behavior of sync_core().

That's exactly what I had in mind when I wrote the above sentence...

> That driver absolutely needs to issue a cpuid (with EAX = 1) before each
> rdmsr(MSR_IA32_UCODE_REV).  And it uses sync_core() calls to do it.  A
> CR2 access just won't do in this extremely specific case.
> 
> This kind of pitfall is why I wanted to replace all use of sync_core()
> in arch/x86/kernel/cpu/microcode/intel.c with an explicit use of an
> inconditional cpuid(eax = 1)...
> 
> (note: this protocol to read MSR_IA32_UCODE_REV was made an
> architectural requirement a while ago -- it was once considered an
> erratum workaround.  It is documented in the "Intel 64 and IA‐32
> Architectures Software Developer's Manual", Volume 3A: System
> Programming Guide, Part 1, section 9.11).

Well, I'm looking at this:

"CPUID returns a value in a model specific register in addition to its
usual register return values. The semantics of CPUID cause it to deposit
an update ID value in the 64-bit model-specific register at address 08BH
(IA32_BIOS_SIGN_ID)."

And I think yes, we should do an explicit CPUID(1) regardless of
what happens to sync_core(). Feel free to send a patch against
tip:x86/microcode.

-- 
Regards/Gruss,
Boris.

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

Re: [PATCH] x86/boot: Fail the boot if !M486 and CPUID is missing

[Henrique de Moraes Holschuh]

On Sun, 20 Nov 2016, Borislav Petkov wrote:
> We will have set (or not) the X86_FEATURE_CPUID bit at
> early_identify_cpu() time. Looking at the code, we do call sync_core()
> pretty early. :-\

Hmm, watch out for the early microcode update driver for Intel
processors should something get changed in the implementation, or in the
behavior of sync_core().

That driver absolutely needs to issue a cpuid (with EAX = 1) before each
rdmsr(MSR_IA32_UCODE_REV).  And it uses sync_core() calls to do it.  A
CR2 access just won't do in this extremely specific case.

This kind of pitfall is why I wanted to replace all use of sync_core()
in arch/x86/kernel/cpu/microcode/intel.c with an explicit use of an
inconditional cpuid(eax = 1)...

(note: this protocol to read MSR_IA32_UCODE_REV was made an
architectural requirement a while ago -- it was once considered an
erratum workaround.  It is documented in the "Intel 64 and IA‐32
Architectures Software Developer's Manual", Volume 3A: System
Programming Guide, Part 1, section 9.11).

-- 
  Henrique Holschuh

Re: [PATCH] x86/boot: Fail the boot if !M486 and CPUID is missing

[Henrique de Moraes Holschuh]

On Sun, 20 Nov 2016, Borislav Petkov wrote:
> We will have set (or not) the X86_FEATURE_CPUID bit at
> early_identify_cpu() time. Looking at the code, we do call sync_core()
> pretty early. :-\

Hmm, watch out for the early microcode update driver for Intel
processors should something get changed in the implementation, or in the
behavior of sync_core().

That driver absolutely needs to issue a cpuid (with EAX = 1) before each
rdmsr(MSR_IA32_UCODE_REV).  And it uses sync_core() calls to do it.  A
CR2 access just won't do in this extremely specific case.

This kind of pitfall is why I wanted to replace all use of sync_core()
in arch/x86/kernel/cpu/microcode/intel.c with an explicit use of an
inconditional cpuid(eax = 1)...

(note: this protocol to read MSR_IA32_UCODE_REV was made an
architectural requirement a while ago -- it was once considered an
erratum workaround.  It is documented in the "Intel 64 and IA‐32
Architectures Software Developer's Manual", Volume 3A: System
Programming Guide, Part 1, section 9.11).

-- 
  Henrique Holschuh

Re: [PATCH] x86/boot: Fail the boot if !M486 and CPUID is missing

[Borislav Petkov]

On Sun, Nov 20, 2016 at 08:22:25AM -0800, Andy Lutomirski wrote:
> This makes me nervous: don't some CPUs actually need the cpuid
> instruction when patching alternatives?

Nope, we use boot_cpu_has() in apply_alternatives() if that is what you
mean.

> And with this approach, we won't have the cpuid instruction there
> until after patching.

We will have set (or not) the X86_FEATURE_CPUID bit at
early_identify_cpu() time. Looking at the code, we do call sync_core()
pretty early. :-\

Hmm, that #ifdef CONFIG_M486 is there for a reason.

> Why not change this function entirely:
> 
> write_cr2(0);
> 
> CR2 should be available on all 32-bit CPUs.  It clobbers fewer
> registers.

Yap, just one. And not only that - it clobbers a register which gcc
doesn't have to reload at all.

> More usefully, CPUID causes an exit when running under
> most hypervisors, and that's quite slow.  The only case I can think of
> where CPUID should be faster than MOV to CR2 is on Xen PV before Ivy
> Bridge, and I'm not sure I care about performance there.
> 
> (On Xen PV, it will do a hypercall instead, but the hypercall should
> be good enough to serialize, too.)

A nop hypercall or whatever...

But yeah, pending a nod from hw people, this one sounds nice too. You
can do it basically on every CPU which supports paging. And that should
be all we support in Linux anyway.

> Or we could do it dynamically:
> 
> bt $X86_FEATURE_CPUID, CPU_FLAGS(boot_cpu_data) # or whatever -- I
> think we need to add an asm offset
> jnc 1f # here's our jump
> cpuid
> 1:

We could... we did move the X86_FEATURE* things to a separate header so
that they can be used in asm too.

write_cr2(0) doesn't sound so bad either. Except what happens if
someone decides to sync_core() before the first line of do_page_fault()
executes... I know, it is unlikely but we do unlikely things :)

But yeah, the write_cr2() sounds better if one considers the lower
register pressure. Which is nice.

-- 
Regards/Gruss,
Boris.

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

Re: [PATCH] x86/boot: Fail the boot if !M486 and CPUID is missing

[Borislav Petkov]

On Sun, Nov 20, 2016 at 08:22:25AM -0800, Andy Lutomirski wrote:
> This makes me nervous: don't some CPUs actually need the cpuid
> instruction when patching alternatives?

Nope, we use boot_cpu_has() in apply_alternatives() if that is what you
mean.

> And with this approach, we won't have the cpuid instruction there
> until after patching.

We will have set (or not) the X86_FEATURE_CPUID bit at
early_identify_cpu() time. Looking at the code, we do call sync_core()
pretty early. :-\

Hmm, that #ifdef CONFIG_M486 is there for a reason.

> Why not change this function entirely:
> 
> write_cr2(0);
> 
> CR2 should be available on all 32-bit CPUs.  It clobbers fewer
> registers.

Yap, just one. And not only that - it clobbers a register which gcc
doesn't have to reload at all.

> More usefully, CPUID causes an exit when running under
> most hypervisors, and that's quite slow.  The only case I can think of
> where CPUID should be faster than MOV to CR2 is on Xen PV before Ivy
> Bridge, and I'm not sure I care about performance there.
> 
> (On Xen PV, it will do a hypercall instead, but the hypercall should
> be good enough to serialize, too.)

A nop hypercall or whatever...

But yeah, pending a nod from hw people, this one sounds nice too. You
can do it basically on every CPU which supports paging. And that should
be all we support in Linux anyway.

> Or we could do it dynamically:
> 
> bt $X86_FEATURE_CPUID, CPU_FLAGS(boot_cpu_data) # or whatever -- I
> think we need to add an asm offset
> jnc 1f # here's our jump
> cpuid
> 1:

We could... we did move the X86_FEATURE* things to a separate header so
that they can be used in asm too.

write_cr2(0) doesn't sound so bad either. Except what happens if
someone decides to sync_core() before the first line of do_page_fault()
executes... I know, it is unlikely but we do unlikely things :)

But yeah, the write_cr2() sounds better if one considers the lower
register pressure. Which is nice.

-- 
Regards/Gruss,
Boris.

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

Re: [PATCH] x86/boot: Fail the boot if !M486 and CPUID is missing

[Andy Lutomirski]

On Nov 20, 2016 3:19 AM, "Borislav Petkov"  wrote:
>
> On Sat, Nov 19, 2016 at 03:37:30PM -0800, Andy Lutomirski wrote:
> > Linux will have all kinds of sporadic problems on systems that don't
> > have the CPUID instruction unless CONFIG_M486=y.  In particular,
> > sync_core() will explode.
>
> Btw, I think we should do something like this, in addition:
>
> ---
> diff --git a/arch/x86/include/asm/cpufeatures.h 
> b/arch/x86/include/asm/cpufeatures.h
> index 50425dd7e113..ee9de769941f 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -105,6 +105,7 @@
>  #define X86_FEATURE_AMD_DCM ( 3*32+27) /* multi-node processor */
>  #define X86_FEATURE_APERFMPERF ( 3*32+28) /* APERFMPERF */
>  #define X86_FEATURE_NONSTOP_TSC_S3 ( 3*32+30) /* TSC doesn't stop in S3 
> state */
> +#define X86_FEATURE_CPUID  ( 3*32+31) /* CPU has CPUID instruction 
> itself */
>
>  /* Intel-defined CPU features, CPUID level 0x0001 (ecx), word 4 */
>  #define X86_FEATURE_XMM3   ( 4*32+ 0) /* "pni" SSE-3 */
> diff --git a/arch/x86/include/asm/processor.h 
> b/arch/x86/include/asm/processor.h
> index 1f6a92903b09..63aa4842c0ae 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -602,34 +602,21 @@ static __always_inline void cpu_relax(void)
> rep_nop();
>  }
>
> -/* Stop speculative execution and prefetching of modified code. */
> +/*
> + * Stop speculative execution and prefetching of modified code. CPUID is a
> + * barrier to speculative execution. Prefetched instructions are 
> automatically
> + * invalidated when modified.
> + */
>  static inline void sync_core(void)
>  {
> int tmp;
>
> -#ifdef CONFIG_M486
> -   /*
> -* 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.
> -*/
> -   asm volatile("cpuid"
> -: "=a" (tmp)
> -: "0" (1)
> -: "ebx", "ecx", "edx", "memory");
> -#endif
> +   /* Do a CPUID if available, otherwise do a forward jump. */
> +   alternative_io("jmp 1f\n\t1:", "cpuid",
> +   X86_FEATURE_CPUID,
> +   "=a" (tmp),
> +   "0" (1)
> +   : "ebx", "ecx", "edx", "memory");
>  }

This makes me nervous: don't some CPUs actually need the cpuid
instruction when patching alternatives?  And with this approach, we
won't have the cpuid instruction there until after patching.

Why not change this function entirely:

write_cr2(0);

CR2 should be available on all 32-bit CPUs.  It clobbers fewer
registers.  More usefully, CPUID causes an exit when running under
most hypervisors, and that's quite slow.  The only case I can think of
where CPUID should be faster than MOV to CR2 is on Xen PV before Ivy
Bridge, and I'm not sure I care about performance there.

(On Xen PV, it will do a hypercall instead, but the hypercall should
be good enough to serialize, too.)

Or we could do it dynamically:

bt $X86_FEATURE_CPUID, CPU_FLAGS(boot_cpu_data) # or whatever -- I
think we need to add an asm offset
jnc 1f # here's our jump
cpuid
1:

It's not like CPUID is fast enough that avoiding a predictable branch
will help measurably.

--Andy

Re: [PATCH] x86/boot: Fail the boot if !M486 and CPUID is missing

[Andy Lutomirski]

On Nov 20, 2016 3:19 AM, "Borislav Petkov"  wrote:
>
> On Sat, Nov 19, 2016 at 03:37:30PM -0800, Andy Lutomirski wrote:
> > Linux will have all kinds of sporadic problems on systems that don't
> > have the CPUID instruction unless CONFIG_M486=y.  In particular,
> > sync_core() will explode.
>
> Btw, I think we should do something like this, in addition:
>
> ---
> diff --git a/arch/x86/include/asm/cpufeatures.h 
> b/arch/x86/include/asm/cpufeatures.h
> index 50425dd7e113..ee9de769941f 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -105,6 +105,7 @@
>  #define X86_FEATURE_AMD_DCM ( 3*32+27) /* multi-node processor */
>  #define X86_FEATURE_APERFMPERF ( 3*32+28) /* APERFMPERF */
>  #define X86_FEATURE_NONSTOP_TSC_S3 ( 3*32+30) /* TSC doesn't stop in S3 
> state */
> +#define X86_FEATURE_CPUID  ( 3*32+31) /* CPU has CPUID instruction 
> itself */
>
>  /* Intel-defined CPU features, CPUID level 0x0001 (ecx), word 4 */
>  #define X86_FEATURE_XMM3   ( 4*32+ 0) /* "pni" SSE-3 */
> diff --git a/arch/x86/include/asm/processor.h 
> b/arch/x86/include/asm/processor.h
> index 1f6a92903b09..63aa4842c0ae 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -602,34 +602,21 @@ static __always_inline void cpu_relax(void)
> rep_nop();
>  }
>
> -/* Stop speculative execution and prefetching of modified code. */
> +/*
> + * Stop speculative execution and prefetching of modified code. CPUID is a
> + * barrier to speculative execution. Prefetched instructions are 
> automatically
> + * invalidated when modified.
> + */
>  static inline void sync_core(void)
>  {
> int tmp;
>
> -#ifdef CONFIG_M486
> -   /*
> -* 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.
> -*/
> -   asm volatile("cpuid"
> -: "=a" (tmp)
> -: "0" (1)
> -: "ebx", "ecx", "edx", "memory");
> -#endif
> +   /* Do a CPUID if available, otherwise do a forward jump. */
> +   alternative_io("jmp 1f\n\t1:", "cpuid",
> +   X86_FEATURE_CPUID,
> +   "=a" (tmp),
> +   "0" (1)
> +   : "ebx", "ecx", "edx", "memory");
>  }

This makes me nervous: don't some CPUs actually need the cpuid
instruction when patching alternatives?  And with this approach, we
won't have the cpuid instruction there until after patching.

Why not change this function entirely:

write_cr2(0);

CR2 should be available on all 32-bit CPUs.  It clobbers fewer
registers.  More usefully, CPUID causes an exit when running under
most hypervisors, and that's quite slow.  The only case I can think of
where CPUID should be faster than MOV to CR2 is on Xen PV before Ivy
Bridge, and I'm not sure I care about performance there.

(On Xen PV, it will do a hypercall instead, but the hypercall should
be good enough to serialize, too.)

Or we could do it dynamically:

bt $X86_FEATURE_CPUID, CPU_FLAGS(boot_cpu_data) # or whatever -- I
think we need to add an asm offset
jnc 1f # here's our jump
cpuid
1:

It's not like CPUID is fast enough that avoiding a predictable branch
will help measurably.

--Andy

Re: [PATCH] x86/boot: Fail the boot if !M486 and CPUID is missing

[Borislav Petkov]

On Sat, Nov 19, 2016 at 03:37:30PM -0800, Andy Lutomirski wrote:
> Linux will have all kinds of sporadic problems on systems that don't
> have the CPUID instruction unless CONFIG_M486=y.  In particular,
> sync_core() will explode.

Btw, I think we should do something like this, in addition:

---
diff --git a/arch/x86/include/asm/cpufeatures.h 
b/arch/x86/include/asm/cpufeatures.h
index 50425dd7e113..ee9de769941f 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -105,6 +105,7 @@
 #define X86_FEATURE_AMD_DCM ( 3*32+27) /* multi-node processor */
 #define X86_FEATURE_APERFMPERF ( 3*32+28) /* APERFMPERF */
 #define X86_FEATURE_NONSTOP_TSC_S3 ( 3*32+30) /* TSC doesn't stop in S3 state 
*/
+#define X86_FEATURE_CPUID  ( 3*32+31) /* CPU has CPUID instruction itself 
*/
 
 /* Intel-defined CPU features, CPUID level 0x0001 (ecx), word 4 */
 #define X86_FEATURE_XMM3   ( 4*32+ 0) /* "pni" SSE-3 */
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 1f6a92903b09..63aa4842c0ae 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -602,34 +602,21 @@ static __always_inline void cpu_relax(void)
rep_nop();
 }
 
-/* Stop speculative execution and prefetching of modified code. */
+/*
+ * Stop speculative execution and prefetching of modified code. CPUID is a
+ * barrier to speculative execution. Prefetched instructions are automatically
+ * invalidated when modified.
+ */
 static inline void sync_core(void)
 {
int tmp;
 
-#ifdef CONFIG_M486
-   /*
-* 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.
-*/
-   asm volatile("cpuid"
-: "=a" (tmp)
-: "0" (1)
-: "ebx", "ecx", "edx", "memory");
-#endif
+   /* Do a CPUID if available, otherwise do a forward jump. */
+   alternative_io("jmp 1f\n\t1:", "cpuid",
+   X86_FEATURE_CPUID,
+   "=a" (tmp),
+   "0" (1)
+   : "ebx", "ecx", "edx", "memory");
 }
 
 extern void select_idle_routine(const struct cpuinfo_x86 *c);
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 90c007447507..8dcdcdeec569 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -800,14 +800,12 @@ static void __init early_identify_cpu(struct cpuinfo_x86 
*c)
memset(>x86_capability, 0, sizeof c->x86_capability);
c->extended_cpuid_level = 0;
 
-   if (!have_cpuid_p())
-   identify_cpu_without_cpuid(c);
-
/* cyrix could have cpuid enabled via c_identify()*/
if (have_cpuid_p()) {
cpu_detect(c);
get_cpu_vendor(c);
get_cpu_cap(c);
+   setup_force_cpu_cap(X86_FEATURE_CPUID);
 
if (this_cpu->c_early_init)
this_cpu->c_early_init(c);
@@ -817,6 +815,9 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 
if (this_cpu->c_bsp_init)
this_cpu->c_bsp_init(c);
+} else {
+   identify_cpu_without_cpuid(c);
+   setup_clear_cpu_cap(X86_FEATURE_CPUID);
}
 
setup_force_cpu_cap(X86_FEATURE_ALWAYS);

-- 
Regards/Gruss,
Boris.

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

Re: [PATCH] x86/boot: Fail the boot if !M486 and CPUID is missing

[Borislav Petkov]

On Sat, Nov 19, 2016 at 03:37:30PM -0800, Andy Lutomirski wrote:
> Linux will have all kinds of sporadic problems on systems that don't
> have the CPUID instruction unless CONFIG_M486=y.  In particular,
> sync_core() will explode.

Btw, I think we should do something like this, in addition:

---
diff --git a/arch/x86/include/asm/cpufeatures.h 
b/arch/x86/include/asm/cpufeatures.h
index 50425dd7e113..ee9de769941f 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -105,6 +105,7 @@
 #define X86_FEATURE_AMD_DCM ( 3*32+27) /* multi-node processor */
 #define X86_FEATURE_APERFMPERF ( 3*32+28) /* APERFMPERF */
 #define X86_FEATURE_NONSTOP_TSC_S3 ( 3*32+30) /* TSC doesn't stop in S3 state 
*/
+#define X86_FEATURE_CPUID  ( 3*32+31) /* CPU has CPUID instruction itself 
*/
 
 /* Intel-defined CPU features, CPUID level 0x0001 (ecx), word 4 */
 #define X86_FEATURE_XMM3   ( 4*32+ 0) /* "pni" SSE-3 */
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 1f6a92903b09..63aa4842c0ae 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -602,34 +602,21 @@ static __always_inline void cpu_relax(void)
rep_nop();
 }
 
-/* Stop speculative execution and prefetching of modified code. */
+/*
+ * Stop speculative execution and prefetching of modified code. CPUID is a
+ * barrier to speculative execution. Prefetched instructions are automatically
+ * invalidated when modified.
+ */
 static inline void sync_core(void)
 {
int tmp;
 
-#ifdef CONFIG_M486
-   /*
-* 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.
-*/
-   asm volatile("cpuid"
-: "=a" (tmp)
-: "0" (1)
-: "ebx", "ecx", "edx", "memory");
-#endif
+   /* Do a CPUID if available, otherwise do a forward jump. */
+   alternative_io("jmp 1f\n\t1:", "cpuid",
+   X86_FEATURE_CPUID,
+   "=a" (tmp),
+   "0" (1)
+   : "ebx", "ecx", "edx", "memory");
 }
 
 extern void select_idle_routine(const struct cpuinfo_x86 *c);
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 90c007447507..8dcdcdeec569 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -800,14 +800,12 @@ static void __init early_identify_cpu(struct cpuinfo_x86 
*c)
memset(>x86_capability, 0, sizeof c->x86_capability);
c->extended_cpuid_level = 0;
 
-   if (!have_cpuid_p())
-   identify_cpu_without_cpuid(c);
-
/* cyrix could have cpuid enabled via c_identify()*/
if (have_cpuid_p()) {
cpu_detect(c);
get_cpu_vendor(c);
get_cpu_cap(c);
+   setup_force_cpu_cap(X86_FEATURE_CPUID);
 
if (this_cpu->c_early_init)
this_cpu->c_early_init(c);
@@ -817,6 +815,9 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 
if (this_cpu->c_bsp_init)
this_cpu->c_bsp_init(c);
+} else {
+   identify_cpu_without_cpuid(c);
+   setup_clear_cpu_cap(X86_FEATURE_CPUID);
}
 
setup_force_cpu_cap(X86_FEATURE_ALWAYS);

-- 
Regards/Gruss,
Boris.

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