Date:	Wed, 8 Sep 2010 11:15:15 +0200
From:	"Hans-Peter Jansen" <hpj@...la.net>
To:	linux-kernel@...r.kernel.org
Cc:	Nick Lowe <nick.lowe@...il.com>
Subject: Re: AMD Geode NOPL emulation for kernel 2.6.36-rc2

On Tuesday 07 September 2010, 17:57:17 Nick Lowe wrote:
> In my opinion, this patch shouldn't be seen as a solution in the
> slightest. To me, it's a ugly hack and a kludge at best!
>
> Abstractly a NOP is meant to be exactly as it says on the tin, a No
> Operation.
>
> It's meant to do nothing at all - for a predefined number of clock
> cycles. A NOP is commonly used for timing purposes, this completely
> breaks that contract surely?
>
> NOPL is not standard i686, it was undocumented and has just been de facto
> supported since the Pentium Pro.
>
> The solution is surely to ensure that when something is compiled for
> generic i686, NOPL is nowhere to be seen... Any compiler that does
> otherwise is broken.

>From a user POV, this patch done right is the better solution, because you 
cannot ensure, that all code _generally_ comply to your *policy*. This is 
not a kernel only issue! Keep processes crashing is worse IMHO.

With properly done, I mean:
 - runtime check, if iopl opcode exists
 - emulate, if not

Hence, don't bind this to nor label it after a certain cpu model.

Remember: we do not live in a perfect world.

Pete

> For example, until recently, the basic semantics for choosing the NOP
> sequence were completely wrong in binutils. This has, finally, been
> fixed!
>
> http://sourceware.org/ml/binutils-cvs/2010-08/msg00057.html
> http://gcc.gnu.org/ml/gcc/2010-08/msg00194.html
>
> On 27 Aug 2010, at 19:10, Matteo Croce wrote:
> > 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@...nwrt.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 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.co>
> > + */
> > +
> > +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;
> > +	}
> > +	return 0;
> > +}
> > +
> > +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)
> > +{
> > +	u8 *ip = (u8 *)instruction_pointer(regs);
> > +	int res = do_start(ip);
> > +
> > +	if (res) {
> > +		int i = 0;
> > +		do {
> > +			ip += res;
> > +			i++;
> > +			res = do_start(ip);
> > +		} while(res);
> > +		printk(KERN_DEBUG "geode_nopl: emulated %d instructions\n", i);
> > +		regs->ip = (typeof(regs->ip))ip;
> > +	} else
> > +		do_invalid_op(regs, error_code);
> > +}
> >
> >
> > --
> > 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!
> > Â ---------------------------------------------------
> > --
> > 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/
>
> --
> 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/


--
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