Message-ID: <20091106154911.29400@gmx.net>
Date:	Fri, 06 Nov 2009 16:49:11 +0100
From:	"Martin Schleier" <drahemmaps@....net>
To:	Matteo Croce <technoboy85@...il.com>, mingo@...e.hu
Cc:	linux-kernel@...r.kernel.org, hpa@...or.com
Subject: Re: i686 quirk for AMD Geode

On Fri, 6 Nov 2009 15:59:16 +0100 Matteo Croce wrote:
> On Sat, Oct 3, 2009 at 8:21 AM, Ingo Molnar <mingo@...e.hu> wrote:
> >
> > * Matteo Croce <technoboy85@...il.com> wrote:
[snip]
> > Looks good, but your signoff line is missing.
> >
> >        Ingo
> >
> 
> The AMD Geode LX has an x86 id of 5 (i586) tought it's technically an
> i686:
> 
> root@...x:~# egrep '^(cpu family|model name|flags)' /proc/cpuinfo
> cpu family      : 5
> model name      : Geode(TM) Integrated Processor by AMD PCS
> flags           : fpu de pse tsc msr cx8 sep pge cmov clflush mmx
> mmxext 3dnowext 3dnow
> 
> indeed it has MMX, MMXEXT and CMOV, just lacks the long NOP instruction
> (NOPL).
> This patch adds a quirck to promote the Geode to an i686 and emulates
> the NOPL in the do_invalid_op trap, so the userspace never notices.
> Emulating the NOPL has minimum performance loss, emulating a NOPL
> takes 0.5 usecs
> and they are rarely used in x86
> 
> Signed-off-by: Matteo Croce <technoboy85@...il.com>
> 
> --- a/arch/x86/kernel/Makefile	2009-11-06 15:06:52.246223989 +0100
> +++ b/arch/x86/kernel/Makefile	2009-11-06 15:07:04.294054613 +0100
> @@ -89,7 +89,7 @@
>  obj-$(CONFIG_HPET_TIMER) 	+= hpet.o
> 
>  obj-$(CONFIG_K8_NB)		+= k8.o
> -obj-$(CONFIG_MGEODE_LX)		+= geode_32.o mfgpt_32.o
> +obj-$(CONFIG_MGEODE_LX)		+= geode_32.o mfgpt_32.o 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	2009-11-06 15:06:52.254223805 +0100
> +++ b/arch/x86/kernel/cpu/amd.c	2009-11-06 15:07:04.294054613 +0100
> @@ -138,8 +138,10 @@
>  	}
> 
>  	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 emulate it in the exception handler
> +		   and promote it to a class 6 cpu */
> +		boot_cpu_data.x86 = 6;
>  		return;
>  	}
>  }
> --- a/arch/x86/kernel/entry_32.S	2009-11-06 15:06:52.258224172 +0100
> +++ b/arch/x86/kernel/entry_32.S	2009-11-06 15:07:04.306230613 +0100
> @@ -901,7 +901,11 @@
>  	RING0_INT_FRAME
>  	pushl $0
>  	CFI_ADJUST_CFA_OFFSET 4
> +#ifdef CONFIG_MGEODE_LX
> +	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	2009-11-06 15:07:33.537723795 +0100
> @@ -0,0 +1,102 @@
> +/*
> + *  linux/arch/x86/kernel/nopl_emu.c
> + *
> + *  Copyright (C) 2002  Willy Tarreau
> + *  Copyright (C) 2009  Matteo Croce
> + */
> +
> +#include <linux/sched.h>
> +#include <linux/linkage.h>
> +#include <linux/preempt.h>
> +#include <linux/types.h>
> +
> +void do_invalid_op(struct pt_regs *regs, long error_code);
> +
> +/* 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@...a-x.org>
> + * Matteo Croce <technoboy85@...il.com>
> + */
> +
> +static inline int do_1f(u8 *ip)
> +{
> +	int length = 3;
> +	switch(*ip) {
> +		case 0x84:if(!ip[5])
> +				length++;
> +			else
> +				return 0;
> +		case 0x80:if(!ip[4] && !ip[3])
> +				length += 2;
> +			else
> +				return 0;
> +		case 0x44:if(!ip[2])
> +				length++;
> +			else
> +				return 0;
> +		case 0x40:if(!ip[1])
> +				length++;
> +			else
> +				return 0;
> +		case 0x00:return length;
> +		default: return 0;
> +	}
> +	return length;
> +}
> +
> +static inline int do_0f(u8 *ip)
> +{
> +	if(*ip == 0x1f)
> +		return do_1f(ip + 1);
> +	return 0;
> +}
> +
> +static inline int do_66(u8 *ip)
> +{
> +	if(*ip == 0x90)
> +		return 2;
> +	if(*ip == 0x0f) {
> +		int res = do_0f(ip + 1);
> +		if(res)
> +			return res + 1;
> +		else
> +			return 0;
> +	}
> +	return 0;
> +}
> +
> +static inline int do_start(u8 *ip)
> +{
> +	if(*ip == 0x0f)
> +		return do_0f(ip + 1);
> +	if(*ip == 0x66)
> +		return do_66(ip + 1);
> +	return 0;
> +}
> +
> +/* [do_nopl_emu] is called by exception 6 after an invalid opcode has
> been
> + * encountered. It will try to emulate it by doing nothing,
> + * and will send a SIGILL or SIGSEGV to the process if not possible.
> + * the NOPL can have variable length opcodes:
> +
> +bytes number	opcode
> +	2	66 90
> +	3	0f 1f 00
> +	4	0f 1f 40 00
> +	5	0f 1f 44 00 00
> +	6	66 0f 1f 44 00 00
> +	7	0f 1f 80 00 00 00 00
> +	8	0f 1f 84 00 00 00 00 00
> +	9	66 0f 1f 84 00 00 00 00 00
> +*/
> +void do_nopl_emu(struct pt_regs *regs, long error_code)
> +{
> +	int res = do_start((u8*)regs->ip);
> +
> +	if(res)
> +		regs->ip += res;
> +	else
> +		do_invalid_op(regs, error_code);
> +}
> --

Looks good? checkpatch.pl has a very different opinion.

WARNING: externs should be avoided in .c files
#56: FILE: arch/x86/kernel/nopl_emu.c:13:
+void do_invalid_op(struct pt_regs *regs, long error_code);

ERROR: switch and case should be at the same indent
#69: FILE: arch/x86/kernel/nopl_emu.c:26:
+	switch(*ip) {
+		case 0x84:if(!ip[5])
[...]
+		case 0x80:if(!ip[4] && !ip[3])
[...]
+		case 0x44:if(!ip[2])
[...]
+		case 0x40:if(!ip[1])
[...]
+		case 0x00:return length;
+		default: return 0;

ERROR: space required before the open parenthesis '('
#69: FILE: arch/x86/kernel/nopl_emu.c:26:
+	switch(*ip) {

ERROR: space required before the open parenthesis '('
#70: FILE: arch/x86/kernel/nopl_emu.c:27:
+		case 0x84:if(!ip[5])

ERROR: trailing statements should be on next line
#70: FILE: arch/x86/kernel/nopl_emu.c:27:
+		case 0x84:if(!ip[5])

ERROR: space required before the open parenthesis '('
#74: FILE: arch/x86/kernel/nopl_emu.c:31:
+		case 0x80:if(!ip[4] && !ip[3])

ERROR: trailing statements should be on next line
#74: FILE: arch/x86/kernel/nopl_emu.c:31:
+		case 0x80:if(!ip[4] && !ip[3])

ERROR: space required before the open parenthesis '('
#78: FILE: arch/x86/kernel/nopl_emu.c:35:
+		case 0x44:if(!ip[2])

ERROR: trailing statements should be on next line
#78: FILE: arch/x86/kernel/nopl_emu.c:35:
+		case 0x44:if(!ip[2])

ERROR: space required before the open parenthesis '('
#82: FILE: arch/x86/kernel/nopl_emu.c:39:
+		case 0x40:if(!ip[1])

ERROR: trailing statements should be on next line
#82: FILE: arch/x86/kernel/nopl_emu.c:39:
+		case 0x40:if(!ip[1])

ERROR: space required before the open parenthesis '('
#94: FILE: arch/x86/kernel/nopl_emu.c:51:
+	if(*ip == 0x1f)

ERROR: space required before the open parenthesis '('
#101: FILE: arch/x86/kernel/nopl_emu.c:58:
+	if(*ip == 0x90)

ERROR: space required before the open parenthesis '('
#103: FILE: arch/x86/kernel/nopl_emu.c:60:
+	if(*ip == 0x0f) {

ERROR: space required before the open parenthesis '('
#105: FILE: arch/x86/kernel/nopl_emu.c:62:
+		if(res)

ERROR: space required before the open parenthesis '('
#115: FILE: arch/x86/kernel/nopl_emu.c:72:
+	if(*ip == 0x0f)

ERROR: space required before the open parenthesis '('
#117: FILE: arch/x86/kernel/nopl_emu.c:74:
+	if(*ip == 0x66)

ERROR: "(foo*)" should be "(foo *)"
#139: FILE: arch/x86/kernel/nopl_emu.c:96:
+	int res = do_start((u8*)regs->ip);

ERROR: space required before the open parenthesis '('
#141: FILE: arch/x86/kernel/nopl_emu.c:98:
+	if(res)

ERROR: Missing Signed-off-by: line(s)

total: 19 errors, 1 warnings, 133 lines checked

This patch has style problems, please review.
If any of these errors are false positives report them
to the maintainer, see CHECKPATCH in MAINTAINERS.

-- 
DSL-Preisknaller: DSL Komplettpakete von GMX schon für 
16,99 Euro mtl.!* Hier klicken: http://portal.gmx.net/de/go/dsl02
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists