Re: AMD Geode NOPL emulation for kernel 2.6.36-rc2

Previous thread: [PATCHSET 2.6.36-rc2] block, dm: finish REQ_FLUSH/FUA conversion by Tejun Heo on Friday, August 27, 2010 - 10:10 am. (6 messages)

Next thread: [patch 2/3] x86, intr-remap: remove IRTE setup duplicate code by Suresh Siddha on Friday, August 27, 2010 - 11:09 am. (2 messages)
From: Matteo Croce
Date: Friday, August 27, 2010 - 11:07 am

Hi,
I have received many mails asking to refresh the patch so I decided to
post them on the public mailing list
In this version such feature is configurable via a kernel config option

Signed-off-by: Matteo Croce <matteo@openwrt.org>

--- a/arch/x86/kernel/Makefile	2010-08-27 00:27:50.101261000 +0200
+++ b/arch/x86/kernel/Makefile	2010-08-27 00:31:11.391261002 +0200
@@ -88,6 +88,8 @@
 obj-$(CONFIG_APB_TIMER)		+= apb_timer.o

 obj-$(CONFIG_K8_NB)		+= k8.o
+obj-$(CONFIG_GEODE_NOPL)	+= nopl_emu.o
+obj-$(CONFIG_GEODE_NOPL)	+= nopl_emu.o
 obj-$(CONFIG_DEBUG_RODATA_TEST)	+= test_rodata.o
 obj-$(CONFIG_DEBUG_NX_TEST)	+= test_nx.o

--- a/arch/x86/kernel/cpu/amd.c	2010-08-27 00:27:50.161261000 +0200
+++ b/arch/x86/kernel/cpu/amd.c	2010-08-27 00:34:13.371261000 +0200
@@ -137,11 +137,15 @@
 		return;
 	}

+#ifdef CONFIG_GEODE_NOPL
 	if (c->x86_model == 10) {
-		/* AMD Geode LX is model 10 */
-		/* placeholder for any needed mods */
+		/* Geode only lacks the NOPL instruction to be i686,
+		   but we can promote it to a i686 class cpu
+		   and emulate NOPLs in the exception handler*/
+		boot_cpu_data.x86 = 6;
 		return;
 	}
+#endif
 }

 static void __cpuinit amd_k7_smp_check(struct cpuinfo_x86 *c)
--- a/arch/x86/kernel/entry_32.S	2010-08-27 00:27:50.051261000 +0200
+++ b/arch/x86/kernel/entry_32.S	2010-08-27 00:57:35.531261000 +0200
@@ -978,7 +978,11 @@
 	RING0_INT_FRAME
 	pushl $0
 	CFI_ADJUST_CFA_OFFSET 4
+#ifdef CONFIG_GEODE_NOPL
+	pushl $do_nopl_emu
+#else
 	pushl $do_invalid_op
+#endif
 	CFI_ADJUST_CFA_OFFSET 4
 	jmp error_code
 	CFI_ENDPROC
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ b/arch/x86/kernel/nopl_emu.c	2010-08-27 00:27:57.881261002 +0200
@@ -0,0 +1,110 @@
+/*
+ *  linux/arch/x86/kernel/nopl_emu.c
+ *
+ *  Copyright (C) 2002  Willy Tarreau
+ *  Copyright (C) 2010  Matteo Croce
+ */
+
+#include <asm/processor.h>
+#include <asm/traps.h>
+
+/* This code can be used to allow the AMD Geode to hopefully correctly execute
+ * some code ...
From: H. Peter Anvin
Date: Friday, August 27, 2010 - 11:48 am

You're doing user-space references without get_user().

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

--

From: Matteo Croce
Date: Friday, August 27, 2010 - 1:15 pm

Here with get_user.
I CC Natale which is my beta tester, he have some Alix boards running 24/24
with my patch

--- a/arch/x86/kernel/Makefile	2010-08-27 19:42:01.795858001 +0200
+++ b/arch/x86/kernel/Makefile	2010-08-27 19:42:12.525858001 +0200
@@ -88,6 +88,8 @@
 obj-$(CONFIG_APB_TIMER)		+= apb_timer.o

 obj-$(CONFIG_K8_NB)		+= k8.o
+obj-$(CONFIG_GEODE_NOPL)	+= nopl_emu.o
+obj-$(CONFIG_GEODE_NOPL)	+= nopl_emu.o
 obj-$(CONFIG_DEBUG_RODATA_TEST)	+= test_rodata.o
 obj-$(CONFIG_DEBUG_NX_TEST)	+= test_nx.o

--- a/arch/x86/kernel/cpu/amd.c	2010-08-27 19:42:01.855858001 +0200
+++ b/arch/x86/kernel/cpu/amd.c	2010-08-27 19:42:12.535858001 +0200
@@ -137,11 +137,15 @@
 		return;
 	}

+#ifdef CONFIG_GEODE_NOPL
 	if (c->x86_model == 10) {
-		/* AMD Geode LX is model 10 */
-		/* placeholder for any needed mods */
+		/* Geode only lacks the NOPL instruction to be i686,
+		   but we can promote it to a i686 class cpu
+		   and emulate NOPLs in the exception handler*/
+		boot_cpu_data.x86 = 6;
 		return;
 	}
+#endif
 }

 static void __cpuinit amd_k7_smp_check(struct cpuinfo_x86 *c)
--- a/arch/x86/kernel/entry_32.S	2010-08-27 19:42:01.735858001 +0200
+++ b/arch/x86/kernel/entry_32.S	2010-08-27 19:42:12.535858001 +0200
@@ -978,7 +978,11 @@
 	RING0_INT_FRAME
 	pushl $0
 	CFI_ADJUST_CFA_OFFSET 4
+#ifdef CONFIG_GEODE_NOPL
+	pushl $do_nopl_emu
+#else
 	pushl $do_invalid_op
+#endif
 	CFI_ADJUST_CFA_OFFSET 4
 	jmp error_code
 	CFI_ENDPROC
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ b/arch/x86/kernel/nopl_emu.c	2010-08-27 22:09:58.005858001 +0200
@@ -0,0 +1,124 @@
+/*
+ *  linux/arch/x86/kernel/nopl_emu.c
+ *
+ *  Copyright (C) 2002  Willy Tarreau
+ *  Copyright (C) 2010  Matteo Croce
+ */
+
+#include <asm/processor.h>
+#include <asm/traps.h>
+#include <asm/uaccess.h>
+
+/* This code can be used to allow the AMD Geode to hopefully correctly execute
+ * some code which was originally compiled for an i686, by emulating NOPL,
+ * the only missing i686 ...
From: Thomas Backlund
Date: Friday, August 27, 2010 - 1:49 pm

Same line added twice...

--
Thomas
--

From: Matteo Croce
Date: Friday, August 27, 2010 - 2:32 pm

can I ignore the return value when I expect val to be non zero?
the doc says: "On error, the variable @x is set to zero."




-- 
Matteo Croce
OpenWrt developer
  _______                     ________        __
 |       |.-----.-----.-----.|  |  |  |.----.|  |_
 |   -   ||  _  |  -__|     ||  |  |  ||   _||   _|
 |_______||   __|_____|__|__||________||__|  |____|
          |__| W I R E L E S S   F R E E D O M
 KAMIKAZE (bleeding edge) ------------------
  * 10 oz Vodka       Shake well with ice and strain
  * 10 oz Triple sec  mixture into 10 shot glasses.
  * 10 oz lime juice  Salute!
 ---------------------------------------------------
--

From: Matteo Croce
Date: Friday, August 27, 2010 - 3:16 pm

Here is with proper checks

--- a/arch/x86/kernel/Makefile	2010-08-27 19:42:01.795858001 +0200
+++ b/arch/x86/kernel/Makefile	2010-08-27 19:42:12.525858001 +0200
@@ -88,6 +88,8 @@
 obj-$(CONFIG_APB_TIMER)		+= apb_timer.o

 obj-$(CONFIG_K8_NB)		+= k8.o
+obj-$(CONFIG_GEODE_NOPL)	+= nopl_emu.o
+obj-$(CONFIG_GEODE_NOPL)	+= nopl_emu.o
 obj-$(CONFIG_DEBUG_RODATA_TEST)	+= test_rodata.o
 obj-$(CONFIG_DEBUG_NX_TEST)	+= test_nx.o

--- a/arch/x86/kernel/cpu/amd.c	2010-08-27 19:42:01.855858001 +0200
+++ b/arch/x86/kernel/cpu/amd.c	2010-08-27 19:42:12.535858001 +0200
@@ -137,11 +137,15 @@
 		return;
 	}

+#ifdef CONFIG_GEODE_NOPL
 	if (c->x86_model == 10) {
-		/* AMD Geode LX is model 10 */
-		/* placeholder for any needed mods */
+		/* Geode only lacks the NOPL instruction to be i686,
+		   but we can promote it to a i686 class cpu
+		   and emulate NOPLs in the exception handler*/
+		boot_cpu_data.x86 = 6;
 		return;
 	}
+#endif
 }

 static void __cpuinit amd_k7_smp_check(struct cpuinfo_x86 *c)
--- a/arch/x86/kernel/entry_32.S	2010-08-27 19:42:01.735858001 +0200
+++ b/arch/x86/kernel/entry_32.S	2010-08-27 19:42:12.535858001 +0200
@@ -978,7 +978,11 @@
 	RING0_INT_FRAME
 	pushl $0
 	CFI_ADJUST_CFA_OFFSET 4
+#ifdef CONFIG_GEODE_NOPL
+	pushl $do_nopl_emu
+#else
 	pushl $do_invalid_op
+#endif
 	CFI_ADJUST_CFA_OFFSET 4
 	jmp error_code
 	CFI_ENDPROC
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ b/arch/x86/kernel/nopl_emu.c	2010-08-28 00:11:52.627085002 +0200
@@ -0,0 +1,128 @@
+/*
+ *  linux/arch/x86/kernel/nopl_emu.c
+ *
+ *  Copyright (C) 2002  Willy Tarreau
+ *  Copyright (C) 2010  Matteo Croce
+ */
+
+#include <asm/processor.h>
+#include <asm/traps.h>
+#include <asm/uaccess.h>
+
+/* This code can be used to allow the AMD Geode to hopefully correctly execute
+ * some code which was originally compiled for an i686, by emulating NOPL,
+ * the only missing i686 instruction in the CPU
+ *
+ * Willy Tarreau <willy@meta-x.org>
+ * Matteo Croce ...
From: Matteo Croce
Date: Friday, August 27, 2010 - 3:19 pm

Sorry still left the duplicated line

--- a/arch/x86/kernel/Makefile	2010-08-27 19:42:01.795858001 +0200
+++ b/arch/x86/kernel/Makefile	2010-08-28 00:16:53.607507000 +0200
@@ -88,6 +88,7 @@
 obj-$(CONFIG_APB_TIMER)		+= apb_timer.o

 obj-$(CONFIG_K8_NB)		+= k8.o
+obj-$(CONFIG_GEODE_NOPL)	+= nopl_emu.o
 obj-$(CONFIG_DEBUG_RODATA_TEST)	+= test_rodata.o
 obj-$(CONFIG_DEBUG_NX_TEST)	+= test_nx.o

--- a/arch/x86/kernel/cpu/amd.c	2010-08-27 19:42:01.855858001 +0200
+++ b/arch/x86/kernel/cpu/amd.c	2010-08-27 19:42:12.535858001 +0200
@@ -137,11 +137,15 @@
 		return;
 	}

+#ifdef CONFIG_GEODE_NOPL
 	if (c->x86_model == 10) {
-		/* AMD Geode LX is model 10 */
-		/* placeholder for any needed mods */
+		/* Geode only lacks the NOPL instruction to be i686,
+		   but we can promote it to a i686 class cpu
+		   and emulate NOPLs in the exception handler*/
+		boot_cpu_data.x86 = 6;
 		return;
 	}
+#endif
 }

 static void __cpuinit amd_k7_smp_check(struct cpuinfo_x86 *c)
--- a/arch/x86/kernel/entry_32.S	2010-08-27 19:42:01.735858001 +0200
+++ b/arch/x86/kernel/entry_32.S	2010-08-27 19:42:12.535858001 +0200
@@ -978,7 +978,11 @@
 	RING0_INT_FRAME
 	pushl $0
 	CFI_ADJUST_CFA_OFFSET 4
+#ifdef CONFIG_GEODE_NOPL
+	pushl $do_nopl_emu
+#else
 	pushl $do_invalid_op
+#endif
 	CFI_ADJUST_CFA_OFFSET 4
 	jmp error_code
 	CFI_ENDPROC
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ b/arch/x86/kernel/nopl_emu.c	2010-08-28 00:11:52.627085002 +0200
@@ -0,0 +1,128 @@
+/*
+ *  linux/arch/x86/kernel/nopl_emu.c
+ *
+ *  Copyright (C) 2002  Willy Tarreau
+ *  Copyright (C) 2010  Matteo Croce
+ */
+
+#include <asm/processor.h>
+#include <asm/traps.h>
+#include <asm/uaccess.h>
+
+/* This code can be used to allow the AMD Geode to hopefully correctly execute
+ * some code which was originally compiled for an i686, by emulating NOPL,
+ * the only missing i686 instruction in the CPU
+ *
+ * Willy Tarreau <willy@meta-x.org>
+ * Matteo Croce <technoboy85@gmail.com>
+ */
+
+static ...
From: H. Peter Anvin
Date: Friday, August 27, 2010 - 4:07 pm

No.  You need to deliver a page fault to the application in this case.

The *real* test for this kind of crap is correct page fault behavior,
and so forth.

Also, at the very least you need to check for:

- CS == USER_CS
- IP in the proper range for user space

Your patch in its current form is one big security hole.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

--

From: Avi Kivity
Date: Sunday, August 29, 2010 - 5:52 am

Is that mandated by the ABI?  Is it illegal for an application to load a 
segment with a nonzero base into the LDT and use it for CS?

What about vm86?

-- 
error compiling committee.c: too many arguments to function

--

From: Matteo Croce
Date: Sunday, August 29, 2010 - 6:39 am

If the parsing fails due get_user returning error I call
`do_invalid_op(regs, error_code);`
which is the default handler, which does the page fault.
to check the CS I do `regs->cs != __USER_CS` but how to check the IP value?
convert_ip_to_linear() and then check something?

-- 
Matteo Croce
OpenWrt developer
  _______                     ________        __
 |       |.-----.-----.-----.|  |  |  |.----.|  |_
 |   -   ||  _  |  -__|     ||  |  |  ||   _||   _|
 |_______||   __|_____|__|__||________||__|  |____|
          |__| W I R E L E S S   F R E E D O M
 KAMIKAZE (bleeding edge) ------------------
  * 10 oz Vodka       Shake well with ice and strain
  * 10 oz Triple sec  mixture into 10 shot glasses.
  * 10 oz lime juice  Salute!
 ---------------------------------------------------
--

From: H. Peter Anvin
Date: Wednesday, September 8, 2010 - 1:59 pm

No, it doesn't.  It does an SIGILL, not a SIGSEGV.  An application which
does its own VM management depends on the difference.

Also, you only test for specific forms of NOPL, whereas the right thing

get_user() will check for the validity of a linear address, and yes,
convert_ip_to_linear() should give you the linear address to check for.
 However, you also have to check for the CPU mode, since the byte
sequences mean different things in 16-, 32- and 64-bit mode.

All of this is why I'm extremely reluctant to allow in an ad hoc hack
like this one ... there just are way too many pitfalls, any of which can
turn into a security hole.

	-hpa
--

From: H. Peter Anvin
Date: Friday, August 27, 2010 - 1:54 pm

You actually have to check the get_user return, or use exception brackets....


-- 
Sent from my mobile phone.  Please pardon any lack of formatting.
--

Previous thread: [PATCHSET 2.6.36-rc2] block, dm: finish REQ_FLUSH/FUA conversion by Tejun Heo on Friday, August 27, 2010 - 10:10 am. (6 messages)

Next thread: [patch 2/3] x86, intr-remap: remove IRTE setup duplicate code by Suresh Siddha on Friday, August 27, 2010 - 11:09 am. (2 messages)