[PATCH] x86: Don't use NOPL on 32-bit cpu's because not all systems support it.

[PATCH] x86: Don't use NOPL on 32-bit cpu's because not all systems support it.

[David Sanders]

From: David Sanders <linux@sandersweb.net>

Currently in alternative.c NOPLs are introduced based on the synthetic
cpu feature X86_FEATURE_NOPL.  However, some systems (like Virtual PC 2007)
appear to support it but then will not boot 50% of the time because of
the NOPLs (when paravirtualization support is built into kernel).
This patch standardizes the treatment of NOPL to be like
include/asm-x86/nops.h which only uses NOPLs on 64-bit processors.

Applies to 2.6.27-rc6.

Signed-off-by: David Sanders <linux@sandersweb.net>
---
 arch/x86/kernel/alternative.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 65a0c1b..dceb843 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -160,8 +160,12 @@ const unsigned char *const *find_nop_table(void)
 		return k8_nops;
 	else if (boot_cpu_has(X86_FEATURE_K7))
 		return k7_nops;
-	else if (boot_cpu_has(X86_FEATURE_NOPL))
-		return p6_nops;
+	/*
+	 * Don't use NOPL in 32-bit mode because some Emulators
+	 * do not support it.
+	 */
+	/* else if (boot_cpu_has(X86_FEATURE_NOPL))
+		return p6_nops; */
 	else
 		return intel_nops;
 }
--
1.6.0.2.229.g1293c

Re: [PATCH] x86: Don't use NOPL on 32-bit cpu's because not all systems support it.

[H. Peter Anvin]

[-- Attachment #1: Type: text/plain, Size: 1092 bytes --]

David Sanders wrote:
> From: David Sanders <linux@sandersweb.net>
> 
> Currently in alternative.c NOPLs are introduced based on the synthetic
> cpu feature X86_FEATURE_NOPL.  However, some systems (like Virtual PC 2007)
> appear to support it but then will not boot 50% of the time because of
> the NOPLs (when paravirtualization support is built into kernel).
> This patch standardizes the treatment of NOPL to be like
> include/asm-x86/nops.h which only uses NOPLs on 64-bit processors.
> 

Oh good grief.

VPC is apparently so broken that these instructions work *some* of the 
time, which may include the first time, but not later.  That is an 
impressive level of cockup.

Given that, we should either just rip this code out, or detect VPC 
(how?).  The latter option can be done post-.27, of course; if so, we 
should just force the bit off for now rather than doing it in the 
alternatives code.  The only reason for doing the latter at all would be 
to mitigate the overhead of paravirt_ops and other dynamic patch sites.

It would also be the least impact for .27, I believe.

	-hpa

[-- Attachment #2: 0001-x86-completely-disable-NOPL-on-32-bits.patch --]
[-- Type: text/x-patch, Size: 2064 bytes --]

>From 62e240f4f79d0473c91b12cbc1254f85bffee14b Mon Sep 17 00:00:00 2001
From: H. Peter Anvin <hpa@zytor.com>
Date: Tue, 16 Sep 2008 09:29:40 -0700
Subject: [PATCH] x86: completely disable NOPL on 32 bits

Completely disable NOPL on 32 bits.  It turns out that Microsoft
Virtual PC is so broken it can't even reliably *fail* in the presence
of NOPL.

This leaves the infrastructure in place but disables it
unconditionally.

Signed-off-by: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/kernel/cpu/common.c |   24 ++++--------------------
 1 files changed, 4 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 8aab851..4e456bd 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -344,31 +344,15 @@ static void __init early_cpu_detect(void)
 
 /*
  * The NOPL instruction is supposed to exist on all CPUs with
- * family >= 6, unfortunately, that's not true in practice because
+ * family >= 6; unfortunately, that's not true in practice because
  * of early VIA chips and (more importantly) broken virtualizers that
- * are not easy to detect.  Hence, probe for it based on first
- * principles.
+ * are not easy to detect.  In the latter case it doesn't even *fail*
+ * reliably, so probing for it doesn't even work.  Disable it completely
+ * unless we can find a reliable way to detect all the broken cases.
  */
 static void __cpuinit detect_nopl(struct cpuinfo_x86 *c)
 {
-	const u32 nopl_signature = 0x888c53b1; /* Random number */
-	u32 has_nopl = nopl_signature;
-
 	clear_cpu_cap(c, X86_FEATURE_NOPL);
-	if (c->x86 >= 6) {
-		asm volatile("\n"
-			     "1:      .byte 0x0f,0x1f,0xc0\n" /* nopl %eax */
-			     "2:\n"
-			     "        .section .fixup,\"ax\"\n"
-			     "3:      xor %0,%0\n"
-			     "        jmp 2b\n"
-			     "        .previous\n"
-			     _ASM_EXTABLE(1b,3b)
-			     : "+a" (has_nopl));
-
-		if (has_nopl == nopl_signature)
-			set_cpu_cap(c, X86_FEATURE_NOPL);
-	}
 }
 
 static void __cpuinit generic_identify(struct cpuinfo_x86 *c)
-- 
1.6.0.1

Re: [PATCH] x86: Don't use NOPL on 32-bit cpu's because not all systems support it.

[Alan Cox]

> Given that, we should either just rip this code out, or detect VPC 
> (how?).  The latter option can be done post-.27, of course; if so, we 

Make the user pass a "vpcworkarounds" boot option...

Re: [PATCH] x86: Don't use NOPL on 32-bit cpu's because not all systems support it.

[H. Peter Anvin]

Alan Cox wrote:
>> Given that, we should either just rip this code out, or detect VPC 
>> (how?).  The latter option can be done post-.27, of course; if so, we 
> 
> Make the user pass a "vpcworkarounds" boot option...

<joke>
Kernel Tainted: Microsoft
</joke>

	-hpa

Re: [PATCH] x86: Don't use NOPL on 32-bit cpu's because not all systems support it.

[David Sanders]

On Tuesday 16 September 2008 12:32, H. Peter Anvin wrote:
> Oh good grief.
>
> VPC is apparently so broken that these instructions work *some* of the
> time, which may include the first time, but not later.  That is an
> impressive level of cockup.
>
> Given that, we should either just rip this code out, or detect VPC
> (how?).  The latter option can be done post-.27, of course; if so, we
> should just force the bit off for now rather than doing it in the
> alternatives code.  The only reason for doing the latter at all would be
> to mitigate the overhead of paravirt_ops and other dynamic patch sites.
>
> It would also be the least impact for .27, I believe.

I think you still want the nopl enabled for 64-bits.

This is how I detect virtual pc.  I based it on a google search.  Microsoft 
itself provides no info:

#include <stdio.h>
#include <signal.h>
#include <stdlib.h>

void except(int e)
{
	printf("Not in Virtual PC\n");
	exit(1);
}

int main()
{
	signal(SIGILL, except);
	asm("\n"
		"mov $0x01, %eax\n" /* function number */
		".byte 0x0f, 0x3f, 0x07, 0x0b\n" /* call VPC */
	   );
	printf("Inside Virtual PC\n");
	return 0;
}

Re: [PATCH] x86: Don't use NOPL on 32-bit cpu's because not all systems support it.

[David Sanders]

On Tuesday 16 September 2008 13:00, Alan Cox wrote:
> > Given that, we should either just rip this code out, or detect VPC
> > (how?).  The latter option can be done post-.27, of course; if so, we
>
> Make the user pass a "vpcworkarounds" boot option...
Actually, you can pass noreplace-paravirt, but that is non-obvious to the 
regular user.

Re: [PATCH] x86: Don't use NOPL on 32-bit cpu's because not all systems support it.

[H. Peter Anvin]

> 
> I think you still want the nopl enabled for 64-bits.
> 

Yes.  Accordingly, I didn't change 64 bits.

> int main()
> {
> 	signal(SIGILL, except);
> 	asm("\n"
> 		"mov $0x01, %eax\n" /* function number */
> 		".byte 0x0f, 0x3f, 0x07, 0x0b\n" /* call VPC */
> 	   );
> 	printf("Inside Virtual PC\n");
> 	return 0;
> }

Well, isn't that "interesting".  0F 3F is in the range expected 
(although not documented) to be used for 3-byte opcode prefixes in the 
future.  It's also the Centaur ALTINST instruction (which I believe have 
to be enabled via an MSR, so in that way it's not a bad choice.)

It's definitely less broken than the official way to detect VMWare, but 
still pretty daft.  (CPUID, MSR or PCI IDs make a lot more sense.)

The "official" way to detect Virtual PC is apparently to look for 
"Microsoft Corportation" as the DMI motherboard manufacturer.  This is 
downright dangerously stupid, since Microsoft likes to make false starts 
in the hardware business on a regular basis, and doesn't work with old 
Connectix VPC anyway.  However, there is supposedly "Virtual Machine" as 
the model, which can be used to disambiguate.

	-hpa