Re: i686 quirk for AMD Geode

[Martin Schleier]

On Fri, 6 Nov 2009 15:59:16 +0100 Matteo Croce wrote:
> On Sat, Oct 3, 2009 at 8:21 AM, Ingo Molnar <mingo@...> wrote:
> >
> > * Matteo Croce <technoboy85@...> 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@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/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@...>
> + * Matteo Croce <technoboy85@...>
> + */
> +
> +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@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Alan Cox]

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

Firstly please learn to trim your email
Secondly Ingo knows how to operate checkpatch and trivial style bits like
that are irrelevant to meaningful discussion about code


Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Matteo Croce-3]

On Fri, Nov 6, 2009 at 4:59 PM, Alan Cox <alan@...> wrote:
>> Looks good? checkpatch.pl has a very different opinion.
>
> Firstly please learn to trim your email
> Secondly Ingo knows how to operate checkpatch and trivial style bits like
> that are irrelevant to meaningful discussion about code
>
>
> Alan
>

I fixed the style issues with checkpatch.pl, here is it.

The AMD Geode LX has an x86 id of 5 (i586) tought it's technically an i686:

root@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/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 17:36:40.684361766 +0100
@@ -0,0 +1,106 @@
+/*
+ *  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@...>
+ * Matteo Croce <technoboy85@...>
+ */
+
+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);
+}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Martin Schleier]

On Fri, 6 Nov 2009 15:59:37 Alan Cox wrote:
> > Looks good? checkpatch.pl has a very different opinion.
>
> Firstly please learn to trim your email
No, this email contained comments regarding the code.

If it wasn't riddled with 19 errors (not bad for only 133 lines),
I would have bothered to remove these irrelevant lines.

So this advises goes right out of the window.

> Secondly Ingo knows how to operate checkpatch and trivial style bits
> like that are irrelevant to meaningful discussion about code.

Oh, I'm deeply sorry Sir Cox,
I was unaware of the fact that Ingo is just one of your checkpatch minions!?

There's a document, you should have heard of before,
Documentation/SubmittingPatches and it states: "

4) Style check your changes.

Check your patch for basic style violations, details of which can be
found in Documentation/CodingStyle.  Failure to do so simply wastes
the reviewers time and will get your patch rejected, probably
without even being read.

At a minimum you should check your patches with the patch style
checker prior to submission (scripts/checkpatch.pl).  You should
be able to justify all violations that remain in your patch."

Style checks are indeed part of the job of submitting a patch.

It's supposed to make life easier for the Maintainers, so
they only need to add the SOB-line. Instead of wasting their
precious time with really trivia checkpatch.pl fixes.

If you don't like these guidelines,
I'm sure you can call for one of your other
minions e.g. linus to change that for your majesty.

---


Anyway, Matteo Croce reacted quickly and posted a followup.
Well done!
--
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@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Alan Cox]

> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Martin Schleier]

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.
--
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@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Martin Schleier]

On Fri, 6 Nov 2009 21:02:59 Alan Cox wrote:
> ** Plonk ***

*** Plonk ***

here, Sir, I have that posting fixed that for you.

--
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@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Krzysztof Halasa]

"Martin Schleier" <drahemmaps@...> 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@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Martin Schleier]

Sat, 07 Nov 2009 00:05:12 Krzystof Halasa

> "Martin Schleier" <drahemmaps@...> 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@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Krzysztof Halasa]

"Martin Schleier" <drahemmaps@...> 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@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Matteo Croce-3]

On Sat, Nov 7, 2009 at 1:05 AM, Martin Schleier <drahemmaps@...> wrote:

> Sat, 07 Nov 2009 00:05:12 Krzystof Halasa
>> "Martin Schleier" <drahemmaps@...> 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@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/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@...>
+ * Matteo Croce <technoboy85@...>
+ */
+
+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@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Martin Schleier]

On Sat, 07 Nov 2009 11:37:46 Krzysztof Halasa

> "Martin Schleier" <drahemmaps@...> 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.

Ironically, I assumed that these matters are taken somewhat serious
and everything would be fine after the respin...

But instead, all everyone (except the submitter) is barking whenever checkpatch.pl is irrelevant - because apparently it only catches formatting errors -

(BTW: I think this message should be an ERROR, because it
 can really break Randy Dulap's massive kernel compile tests)

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

here's the quote from on which the comment was based:
| On Sat, Oct 3, 2009 at 8:21 AM, Ingo Molnar <mingo@...> wrote:
|
| Looks good, but your signoff line is missing.
|
|        Ingo

now tell me: What is the word he was using to say that the idea
needs _rethinking_ and that he's declined to merge the patch
in the foreseeable future because of these shortcomings?
I can't see them, but I would be delighted if you can
point them out to me.

The discussion whenever this feature make sense has
taken place _a bit_ earlier in the thread with a _positive_ result.
(if you look at the date: the thread started over a month ago:
 http://lkml.org/lkml/2009/10/2/464 )

so I'm not sure if everyone was aware of this,
since this might explain the _differences_.

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

Ahh common sense, so checking & testing your work
before submitting it not _common sense_ anymore?

Surely it's hard for anyone new to know about this before
hitting "send". But so far everyone has stumbled across this. :\

But back to the topic about laws.
What about: "12) Sign your work" in the same SubmittingPatches file?
Have you noticed that only SubmittingPatches talks about the signed-off-by?
And we all know that every patch has to have one.
So Clearly SubmittingPatches really contains LAWs for how to do things.

The only question is if "4) Style check your changes." is a
guideline or a _more_... And there's a paragraph in Documentation/SubmitChecklist:"

5: Check your patch for general style as detailed in
   Documentation/CodingStyle.  Check for trivial violations with the
   patch style checker prior to submission (scripts/checkpatch.pl).
   [BOLD] You should be able to justify all violations that remain in
   your patch. [BOLD]"

Andy Whitcroft <apw@...> clearly wrote that down.
(And there's no point in arguing about checkpatch.pl when you
 have to JUSTIFY ALL REMAINING VIOLATIONS, or more to the point:
 FIX THEM INSTEAD.)

And now my - head hurts - we need a lawyer to answer if this
IS or IS NOT a law before we can bang on with this.

And yes Documentation/SubmitChecklist also has the same _header_:  
"Here are some basic things that developers should do if they want to see their kernel patch submissions accepted more quickly."

I know about that and I agree: time is always an issue.
This cycle is already @ -rc6 (rc5) and given that the debating
started over a month ago it's really time to get cracking...
Thankfully v3 is already available, and even better: fixed :-).

> > 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".
AFAIK the only rule for staging is: it must compile (somehow).
but I'm sure we can ask greg here if there are uncertainties.


--
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@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Krzysztof Halasa]

"Martin Schleier" <drahemmaps@...> writes:

> And now my - head hurts - we need a lawyer to answer if this
> IS or IS NOT a law before we can bang on with this.

Good luck. I have no more questions.
--
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[H. Peter Anvin]

On 11/07/2009 03:11 AM, Matteo Croce wrote:

> --- /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@...>
> + * Matteo Croce <technoboy85@...>
> + */
> +

If we're doing to introduce a missed-instruction interpreter (which is
what this is) in the kernel, it needs to handle all the subtleties of
x86 execution correctly; in particular I believe it needs to check the
code segment limits, permissions, and mode.  Things it doesn't
understand it can SIGILL (or, if more appropriate, SIGSEGV) on, of course.

Personally I think the easiest is to verify that the code segment is
flat 32 bits or even more specifically CS == USER_CS, and SIGILL otherwise.

        -hpa

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Andres Salomon-8]

See comment below.  BTW, how does this affect performance on LXs?
Do you have any hard numbers for common tasks?

On Sat, 7 Nov 2009 12:11:55 +0100
Matteo Croce <technoboy85@...> wrote:
[...]

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

If you're going to update this, you also need to make sure that you're
not breaking things that check it.  For example,
arch/x86/include/asm/geode.h has an is_geode_lx check that expects
boot_cpu_data.x86 to be 5.  Please be sure to update all these places
when creating a patch like this.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Matteo Croce-3]

On Sun, Nov 8, 2009 at 5:05 PM, Andres Salomon <dilinger@...> wrote:

> See comment below.  BTW, how does this affect performance on LXs?
> Do you have any hard numbers for common tasks?
>
> On Sat, 7 Nov 2009 12:11:55 +0100
> Matteo Croce <technoboy85@...> wrote:
> [...]
>>
>> --- 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;
>>       }
>
> If you're going to update this, you also need to make sure that you're
> not breaking things that check it.  For example,
> arch/x86/include/asm/geode.h has an is_geode_lx check that expects
> boot_cpu_data.x86 to be 5.  Please be sure to update all these places
> when creating a patch like this.
>

True, but also remove the duplicate function is_geode in the NAND driver
and use the identical one defined in geode.h:

--- a/drivers/mtd/nand/cs553x_nand.c 2009-11-08 18:58:14.835043214 +0100
+++ b/drivers/mtd/nand/cs553x_nand.c 2009-11-08 19:00:07.914117831 +0100
@@ -30,6 +30,7 @@

 #include <asm/msr.h>
 #include <asm/io.h>
+#include <asm/geode.h>

 #define NR_CS553X_CONTROLLERS 4

@@ -260,23 +261,6 @@
  return err;
 }

-static int is_geode(void)
-{
- /* These are the CPUs which will have a CS553[56] companion chip */
- if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
-    boot_cpu_data.x86 == 5 &&
-    boot_cpu_data.x86_model == 10)
- return 1; /* Geode LX */
-
- if ((boot_cpu_data.x86_vendor == X86_VENDOR_NSC ||
-     boot_cpu_data.x86_vendor == X86_VENDOR_CYRIX) &&
-    boot_cpu_data.x86 == 5 &&
-    boot_cpu_data.x86_model == 5)
- return 1; /* Geode GX (née GX2) */
-
- return 0;
-}
-

 #ifdef CONFIG_MTD_PARTITIONS
 static const char *part_probes[] = { "cmdlinepart", NULL };
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Matteo Croce-3]

On Sun, Nov 8, 2009 at 5:05 PM, Andres Salomon <dilinger@...> wrote:

> See comment below.  BTW, how does this affect performance on LXs?
> Do you have any hard numbers for common tasks?
>
> On Sat, 7 Nov 2009 12:11:55 +0100
> Matteo Croce <technoboy85@...> wrote:
> [...]
>>
>> --- 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;
>>       }
>
> If you're going to update this, you also need to make sure that you're
> not breaking things that check it.  For example,
> arch/x86/include/asm/geode.h has an is_geode_lx check that expects
> boot_cpu_data.x86 to be 5.  Please be sure to update all these places
> when creating a patch like this.
>

Right, but what if is_geode_lx() is called befor the x86.id change takes effect?
Maybe something like this?

--- a/arch/x86/include/asm/geode.h 2009-11-08 19:13:43.531117343 +0100
+++ b/arch/x86/include/asm/geode.h      2009-11-08 19:19:42.130618023 +0100
@@ -177,7 +177,7 @@
 static inline int is_geode_lx(void)
 {
        return ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) &&
-               (boot_cpu_data.x86 == 5) &&
+               (boot_cpu_data.x86 == 5 || boot_cpu_data.x86 == 6) &&
                (boot_cpu_data.x86_model == 10));
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Andres Salomon-8]

On Sun, 8 Nov 2009 19:04:35 +0100
Matteo Croce <technoboy85@...> wrote:

[...]

>
> True, but also remove the duplicate function is_geode in the NAND
> driver and use the identical one defined in geode.h:
>
> --- a/drivers/mtd/nand/cs553x_nand.c 2009-11-08
> 18:58:14.835043214 +0100 +++ b/drivers/mtd/nand/cs553x_nand.c
> 2009-11-08 19:00:07.914117831 +0100 @@ -30,6 +30,7 @@
>
>  #include <asm/msr.h>
>  #include <asm/io.h>
> +#include <asm/geode.h>
>
>  #define NR_CS553X_CONTROLLERS 4
>
> @@ -260,23 +261,6 @@
>   return err;
>  }
>
> -static int is_geode(void)
> -{
> - /* These are the CPUs which will have a CS553[56] companion
> chip */
> - if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
> -    boot_cpu_data.x86 == 5 &&
> -    boot_cpu_data.x86_model == 10)
> - return 1; /* Geode LX */
> -
> - if ((boot_cpu_data.x86_vendor == X86_VENDOR_NSC ||
> -     boot_cpu_data.x86_vendor == X86_VENDOR_CYRIX) &&
> -    boot_cpu_data.x86 == 5 &&
> -    boot_cpu_data.x86_model == 5)
> - return 1; /* Geode GX (née GX2) */
> -
> - return 0;
> -}
> -
>
>  #ifdef CONFIG_MTD_PARTITIONS
>  static const char *part_probes[] = { "cmdlinepart", NULL };

I think the nand driver needs a bit more love than this.  The cs553x is
available for non-geode platforms, so a cs553x driver should not be
checking for the existence of a specific CPU.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Andres Salomon-8]

On Sun, 8 Nov 2009 19:22:47 +0100
Matteo Croce <technoboy85@...> wrote:

> On Sun, Nov 8, 2009 at 5:05 PM, Andres Salomon
> <dilinger@...> wrote:
> > See comment below.  BTW, how does this affect performance on LXs?
> > Do you have any hard numbers for common tasks?
> >
> > On Sat, 7 Nov 2009 12:11:55 +0100
> > Matteo Croce <technoboy85@...> wrote:
> > [...]
> >>
> >> --- 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;
> >>       }
> >
> > If you're going to update this, you also need to make sure that
> > you're not breaking things that check it.  For example,
> > arch/x86/include/asm/geode.h has an is_geode_lx check that expects
> > boot_cpu_data.x86 to be 5.  Please be sure to update all these
> > places when creating a patch like this.
> >
>
> Right, but what if is_geode_lx() is called befor the x86.id change
> takes effect? Maybe something like this?
>
> --- a/arch/x86/include/asm/geode.h 2009-11-08 19:13:43.531117343 +0100
> +++ b/arch/x86/include/asm/geode.h      2009-11-08 19:19:42.130618023
> +0100 @@ -177,7 +177,7 @@
>  static inline int is_geode_lx(void)
>  {
>         return ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) &&
> -               (boot_cpu_data.x86 == 5) &&
> +               (boot_cpu_data.x86 == 5 || boot_cpu_data.x86 == 6) &&
>                 (boot_cpu_data.x86_model == 10));
>  }

Yeah, that looks better.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Willy Tarreau-3]

On Sun, Nov 08, 2009 at 01:47:59PM -0500, Andres Salomon wrote:

> > Right, but what if is_geode_lx() is called befor the x86.id change
> > takes effect? Maybe something like this?
> >
> > --- a/arch/x86/include/asm/geode.h 2009-11-08 19:13:43.531117343 +0100
> > +++ b/arch/x86/include/asm/geode.h      2009-11-08 19:19:42.130618023
> > +0100 @@ -177,7 +177,7 @@
> >  static inline int is_geode_lx(void)
> >  {
> >         return ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) &&
> > -               (boot_cpu_data.x86 == 5) &&
> > +               (boot_cpu_data.x86 == 5 || boot_cpu_data.x86 == 6) &&
> >                 (boot_cpu_data.x86_model == 10));
> >  }
>
>
> Yeah, that looks better.

Wouldn't it be even better if we didn't touch boot_cpu_data.x86
in the first place ? We can provide the emulation to support
686 binaries without faking the CPU family/model, I think it
would be cleaner. Otherwise we would need to report "real" and
"emulated" families in /proc...

Willy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/