[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <AANLkTi=U40mzeMh-UJJy=ggVDry2qDWW-MYYyBhS_Kex@mail.gmail.com>
Date: Wed, 8 Sep 2010 12:34:12 +0100
From: Nick Lowe <nick.lowe@...il.com>
To: Hans-Peter Jansen <hpj@...la.net>, linux-kernel@...r.kernel.org
Subject: Re: AMD Geode NOPL emulation for kernel 2.6.36-rc2
Hi,
If a process crashes because of NOPL usage, you get the feedback loop to fix it.
The beauty of open source is that we can recompile to correct the bad
assumption; the latest version of binutils (GAS) finally corrects the
bad semantic that generic i686 includes NOPL.
http://sourceware.org/ml/binutils-cvs/2010-08/msg00057.html
http://gcc.gnu.org/ml/gcc/2010-08/msg00194.html
That fixes the vast vast majority of cases, so I really don't see the
need for this.
I've posted a RFC on "Promoting Crusoe and Geode Processors to i686
Status" if you have any thoughts on that?
Cheers,
Nick
On Wed, Sep 8, 2010 at 10:15 AM, Hans-Peter Jansen <hpj@...la.net> wrote:
> 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