i686 quirk for AMD Geode
in [Kernel]
|
Prev: [PATCH 1/1] PM: Thaws refrigerated and to be exited kernel threads
Next: drm/ksm -> s2disk -> resume -> [drm:r100_ring_test] *ERROR* radeon: ring test failed (sracth(0x15E4)=0xCAFEDEAD)
From: Krzysztof Halasa on 6 Nov 2009 18:10 "Martin Schleier" <drahemmaps(a)gmx.net> writes: > e.g: > - correct use of some blackfin hi/lo macros. > - if certain data structures are declared as const > (struct seq_operations/file_operations) > - correct use of NR_CPUS is usually wrong > - complains about in_atomic() outside core kernel code > - warns about LINUX_VERSION_CODE, #if 0, > volatile or deprecated functions. > - informs about needless kfree/usb_free_urb checks > - etc... Did the patch in question contain such problems? -- Krzysztof Halasa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Martin Schleier on 6 Nov 2009 19:10 Sat, 07 Nov 2009 00:05:12 Krzystof Halasa > "Martin Schleier" <drahemmaps(a)gmx.net> writes: >On Fri, 6 Nov 2009 18:22:18 Alan Cox wrote: > > > If it wasn't riddled with 19 errors (not bad for only 133 lines), > > > I would have bothered to remove these irrelevant lines. > > > > Checkpatch is just formatting - its just an aide nothing more. > > It's not remotely useful to bother with them for stuff that is > > basically sanely formatted until such point as someone is actually > > sure the patch is worth going into the tree. > > > > the utility is called checkpatch and not checkstyle or > > checkformatting. > > And there's a good reason behind this decision, because it does > > more than just checking style. > > > > e.g: > > - correct use of some blackfin hi/lo macros. > > - if certain data structures are declared as const > > (struct seq_operations/file_operations) > > - correct use of NR_CPUS is usually wrong > > - complains about in_atomic() outside core kernel code > > - warns about LINUX_VERSION_CODE, #if 0, > > volatile or deprecated functions. > > - informs about needless kfree/usb_free_urb checks > > - etc... > > > > and I'm sure that future modifications will add more > >useful functionality _checks_ to many more _common pitfalls_ > >areas. > > Did the patch in question contain such problems? the last point: - etc... => "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);" ? (or do you think that this is a formatting issue?!) a grep will give you a header file where it is defined: "arch/x86/include/asm/traps.h" dotraplinkage void do_invalid_op(struct pt_regs *, long); anyway, in case we get more followers here. I put your question back in context of the original response. Because this discussion-branch was not about arguing about nopl emulation, since - apparently - nothing was/is wrong with the code itself. Instead, we ended up here because of: Fri, 6 Nov 2009 15:59:37 Alan Cox wrote: "Secondly Ingo knows how to operate checkpatch and trivial style bits like that are irrelevant to meaningful discussion about code." And this is clearly not the case. It is the job of a Submitter (as described in Documentations/SubmittingPatches section 4) to check and test his patches with tools like checkpatch or sparse before posting them. After all this patch is going into /arch/x86 and not /drivers/staging and this sort of "extern declaration" is prone to break one day when void do_invalid_op(struct pt_regs *, long); declaration is modified. -- GRATIS f�r alle GMX-Mitglieder: Die maxdome Movie-FLAT! Jetzt freischalten unter http://portal.gmx.net/de/go/maxdome01 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Alan Cox on 6 Nov 2009 19:50 On Fri, 6 Nov 2009 23:18:06 +0100 Matteo Croce <technoboy85(a)gmail.com> wrote: > On Fri, Nov 6, 2009 at 5:44 PM, H. Peter Anvin <hpa(a)zytor.com> wrote: > > On 11/06/2009 06:59 AM, Matteo Croce wrote: > >> indeed it has MMX, MMXEXT and CMOV, just lacks the long NOP instruction (NOPL). > > > > MMX and MMXEXT are hardly hallmarks of i686, which leaves only cmov. cmov is not i686 either. Its an optional extension that *should* only be used after you test its available. So gcc "i686" isn't quite "686". There are of course good reasons for that choice. > yes, I did some test like gzip, bzip2, lame etc and they give more or less > the same results of dhrystone How does that compare to i486. Certainly the old Nat Semi geode seemed to prefer to be fed i486 code to i586 (and wouldn't of course hack i686). You might also want to play around with -mtune= as well as arch= before assuming why i686 is a win (I still btw think the patch is a good idea, it simplifies life enormously for users with the CPU) Alan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Krzysztof Halasa on 7 Nov 2009 05:40 "Martin Schleier" <drahemmaps(a)gmx.net> writes: >> Did the patch in question contain such problems? > the last point: > - etc... => Yeah. > "WARNING: externs should be avoided in .c files Ironically, it's the only "WARNING" while the rest are "ERRORS". OTOH I personally believe all output from checkpatch should be labeled "WARNING"; it's not for checkpatch to decide. It's only a tool. > #56: FILE: arch/x86/kernel/nopl_emu.c:13: > +void do_invalid_op(struct pt_regs *regs, long error_code);" ? > (or do you think that this is a formatting issue?!) Actually, I think it wasn't any issue at all at this point, when it wasn't yet established if the patch makes sense at all. > It is the job of a Submitter > (as described in Documentations/SubmittingPatches section 4) > to check and test his patches with tools like checkpatch or sparse > before posting them. You apparently forgot what SubmittingPatches file is all about: "This text is a collection of suggestions which can greatly increase the chances of your change being accepted." You know, we don't have laws for everything here. And we're not androids specialized in producing C code. We are supposed to use some common sense first. > After all this patch is going into /arch/x86 and not /drivers/staging > and this sort of "extern declaration" is prone to break one day when > void do_invalid_op(struct pt_regs *, long); declaration is modified. That's true, though it's the same for "staging". -- Krzysztof Halasa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Matteo Croce on 7 Nov 2009 06:20
On Sat, Nov 7, 2009 at 1:05 AM, Martin Schleier <drahemmaps(a)gmx.net> wrote: > Sat, 07 Nov 2009 00:05:12 Krzystof Halasa >> "Martin Schleier" <drahemmaps(a)gmx.net> writes: >>On Fri, 6 Nov 2009 18:22:18 Alan Cox wrote: >> > > If it wasn't riddled with 19 errors (not bad for only 133 lines), >> > > I would have bothered to remove these irrelevant lines. >> > >> > Checkpatch is just formatting - its just an aide nothing more. >> > It's not remotely useful to bother with them for stuff that is >> > basically sanely formatted until such point as someone is actually >> > sure the patch is worth going into the tree. >> > >> > the utility is called checkpatch and not checkstyle or >> > checkformatting. >> > And there's a good reason behind this decision, because it does >> > more than just checking style. >> > >> > e.g: >> > - correct use of some blackfin hi/lo macros. >> > - if certain data structures are declared as const >> > � (struct seq_operations/file_operations) >> > - correct use of NR_CPUS is usually wrong >> > - complains about in_atomic() outside core kernel code >> > - warns about LINUX_VERSION_CODE, #if 0, >> > � volatile or deprecated functions. >> > - informs about needless kfree/usb_free_urb checks >> > - etc... >> > >> > and I'm sure that future modifications will add more >> >useful functionality _checks_ to many more _common pitfalls_ >> >areas. >> >> Did the patch in question contain such problems? > the last point: > �- etc... => > > "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);" ? > (or do you think that this is a formatting issue?!) > > a grep will give you a header file where it is defined: > "arch/x86/include/asm/traps.h" > dotraplinkage void do_invalid_op(struct pt_regs *, long); > > anyway, in case we get more followers here. I put your question back > in context of the original response. Because this discussion-branch was > not about arguing about nopl emulation, since - apparently - nothing > was/is wrong with the code itself. > > Instead, we ended up here because of: > > Fri, 6 Nov 2009 15:59:37 Alan Cox wrote: > "Secondly Ingo knows how to operate checkpatch and trivial style bits like > that are irrelevant to meaningful discussion about code." > > And this is clearly not the case. It is the job of a Submitter > (as described in Documentations/SubmittingPatches section 4) > to check and test his patches with tools like checkpatch or sparse > before posting them. > > After all this patch is going into /arch/x86 and not /drivers/staging > and this sort of "extern declaration" is prone to break one day when > void do_invalid_op(struct pt_regs *, long); declaration is modified. > -- > GRATIS f�r alle GMX-Mitglieder: Die maxdome Movie-FLAT! > Jetzt freischalten unter http://portal.gmx.net/de/go/maxdome01 > This one is perfect according to checkpatch.pl The AMD Geode LX has an x86 id of 5 (i586) tought it's technically an i686: root(a)alix:~# 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(a)gmail.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-07 11:59:17.667748571 +0100 @@ -0,0 +1,103 @@ +/* + * linux/arch/x86/kernel/nopl_emu.c + * + * Copyright (C) 2002 Willy Tarreau + * Copyright (C) 2009 Matteo Croce + */ + +#include <linux/linkage.h> +#include <asm/math_emu.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)meta-x.org> + * Matteo Croce <technoboy85(a)gmail.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; + } + 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) +{ + int res = do_start((u8 *)instruction_pointer(regs)); + + if (res) + regs->ip += res; + else + do_invalid_op(regs, error_code); +} -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |