Skip to contentSkip to conversation

Clean up vendor identification

PATCH In review Alexander Graf 5 participants 11 messages Aug 11, 2008

lore
Share & tools
lore

Alexander Graf

author
#0 lore raw

PATCH 1/3 Clean up vendor identification

Right now CPU vendor identification contains a lot of magic numbers. The patch cleans them up to defines, so we can identify the CPU later on without copying magic numbers.

Signed-off-by Alexander Graf <[email protected]>
qemu/target-i386/cpu.h +8 -0
index 7e95900..3c84dc9 100644
@@ -339,6 +341,14 @@
 #define CPUID_EXT3_IBS     (1 << 10)
 #define CPUID_EXT3_SKINIT  (1 << 12)
 
+#define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
+#define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
+#define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
+
+#define CPUID_VENDOR_AMD_1   0x68747541 /* "Auth" */
+#define CPUID_VENDOR_AMD_2   0x69746e65 /* "enti" */ 
+#define CPUID_VENDOR_AMD_3   0x444d4163 /* "cAMD" */
+
 #define EXCP00_DIVZ	0
 #define EXCP01_SSTP	1
 #define EXCP02_NMI	2
qemu/target-i386/helper.c +6 -6
index 73ce7da..0a02a90 100644
@@ -154,9 +154,9 @@ static x86_def_t x86_defs[] = {
     {
         .name = "qemu64",
         .level = 2,
-        .vendor1 = 0x68747541, /* "Auth" */
-        .vendor2 = 0x69746e65, /* "enti" */
-        .vendor3 = 0x444d4163, /* "cAMD" */
+        .vendor1 = CPUID_VENDOR_AMD_1,
+        .vendor2 = CPUID_VENDOR_AMD_2,
+        .vendor3 = CPUID_VENDOR_AMD_3,
         .family = 6,
         .model = 2,
         .stepping = 3,

@@ -355,9 +372,9 @@ static int cpu_x86_register (CPUX86State *env, const char *cpu_model)
         env->cpuid_vendor2 = def->vendor2;
         env->cpuid_vendor3 = def->vendor3;
     } else {
-        env->cpuid_vendor1 = 0x756e6547; /* "Genu" */
-        env->cpuid_vendor2 = 0x49656e69; /* "ineI" */
-        env->cpuid_vendor3 = 0x6c65746e; /* "ntel" */
+        env->cpuid_vendor1 = CPUID_VENDOR_INTEL_1;
+        env->cpuid_vendor2 = CPUID_VENDOR_INTEL_2;
+        env->cpuid_vendor3 = CPUID_VENDOR_INTEL_3;
     }
     env->cpuid_level = def->level;
     env->cpuid_version = (def->family << 8) | (def->model << 4) | def->stepping;
#1 lore raw
Quoted 14 lines · Alexander Graf

Right now CPU vendor identification contains a lot of magic numbers. The
patch cleans them up to defines, so we can identify the CPU later on without copying magic numbers.

Signed-off-by Alexander Graf <[email protected]>
qemu/target-i386/cpu.h +0 -0
index 7e95900..3c84dc9 100644

qemu/target-i386/cpu.h line 350

Quoted 10 lines
qemu/target-i386/cpu.h +7 -0
@@ -339,6 +341,14 @@
 #define CPUID_EXT3_IBS     (1 << 10)
 #define CPUID_EXT3_SKINIT  (1 << 12)
 
+#define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
+#define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
+#define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
+
+#define CPUID_VENDOR_AMD_1   0x68747541 /* "Auth" */
+#define CPUID_VENDOR_AMD_2   0x69746e65 /* "enti" */ 
+#define CPUID_VENDOR_AMD_3   0x444d4163 /* "cAMD" */

if you add "-Wno-multichar" in C flags, you can use:

#define CPUID_VENDOR_INTEL_1 'Genu'
...

Regards,
Laurent

Signature · 3 lines
----------------- [email protected]  ------------------
  "La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry
author
#2 lore raw
Quoted 28 lines · Laurent Vivier
Quoted 14 lines · Alexander Graf

Right now CPU vendor identification contains a lot of magic numbers. The
patch cleans them up to defines, so we can identify the CPU later on without copying magic numbers.

Signed-off-by Alexander Graf <[email protected]>
qemu/target-i386/cpu.h +0 -0
index 7e95900..3c84dc9 100644

qemu/target-i386/cpu.h line 350

Quoted 10 lines
qemu/target-i386/cpu.h +7 -0
@@ -339,6 +341,14 @@
 #define CPUID_EXT3_IBS     (1 << 10)
 #define CPUID_EXT3_SKINIT  (1 << 12)
 
+#define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
+#define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
+#define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
+
+#define CPUID_VENDOR_AMD_1   0x68747541 /* "Auth" */
+#define CPUID_VENDOR_AMD_2   0x69746e65 /* "enti" */
+#define CPUID_VENDOR_AMD_3   0x444d4163 /* "cAMD" */

if you add "-Wno-multichar" in C flags, you can use:

Quoted 2 lines

#define CPUID_VENDOR_INTEL_1 'Genu'
...

Is that a real improvement? It won't make the code faster and the readability doesn't improve that much either.

Alex

Quoted 11 lines

Regards,
Laurent

Signature · 3 lines
----------------- [email protected]  ------------------
 "La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry
#3 lore raw
Quoted 32 lines · Alexander Graf
Quoted 29 lines · Laurent Vivier
Quoted 15 lines · Alexander Graf

Right now CPU vendor identification contains a lot of magic numbers. The
patch cleans them up to defines, so we can identify the CPU later on without copying magic numbers.

Signed-off-by Alexander Graf <[email protected]>
qemu/target-i386/cpu.h +0 -0
index 7e95900..3c84dc9 100644

qemu/target-i386/cpu.h line 350

Quoted 10 lines
qemu/target-i386/cpu.h +7 -0
@@ -339,6 +341,14 @@
 #define CPUID_EXT3_IBS     (1 << 10)
 #define CPUID_EXT3_SKINIT  (1 << 12)
 
+#define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
+#define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
+#define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
+
+#define CPUID_VENDOR_AMD_1   0x68747541 /* "Auth" */
+#define CPUID_VENDOR_AMD_2   0x69746e65 /* "enti" */
+#define CPUID_VENDOR_AMD_3   0x444d4163 /* "cAMD" */

if you add "-Wno-multichar" in C flags, you can use:

Quoted 5 lines
Quoted 2 lines

#define CPUID_VENDOR_INTEL_1 'Genu'
...

Is that a real improvement? It won't make the code faster and the readability doesn't improve that much either.

I'd take it as a security improvement. One can easily commit a typo when editting magic numbers, either decimal or hexa.

Signature · 2 lines
If you think things can't get worse it's probably only
because you lack sufficient imagination.
#6 lore raw
In reply to Alexander Graf
Quoted 30 lines · Alexander Graf
Quoted 28 lines · Laurent Vivier
Quoted 14 lines · Alexander Graf

Right now CPU vendor identification contains a lot of magic numbers. The
patch cleans them up to defines, so we can identify the CPU later on without copying magic numbers.

Signed-off-by Alexander Graf <[email protected]>
qemu/target-i386/cpu.h +0 -0
index 7e95900..3c84dc9 100644

qemu/target-i386/cpu.h line 350

Quoted 10 lines
qemu/target-i386/cpu.h +7 -0
@@ -339,6 +341,14 @@
 #define CPUID_EXT3_IBS     (1 << 10)
 #define CPUID_EXT3_SKINIT  (1 << 12)
 
+#define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
+#define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
+#define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
+
+#define CPUID_VENDOR_AMD_1   0x68747541 /* "Auth" */
+#define CPUID_VENDOR_AMD_2   0x69746e65 /* "enti" */
+#define CPUID_VENDOR_AMD_3   0x444d4163 /* "cAMD" */

if you add "-Wno-multichar" in C flags, you can use:

Quoted 5 lines
Quoted 2 lines

#define CPUID_VENDOR_INTEL_1 'Genu'
...

Is that a real improvement? It won't make the code faster and the readability doesn't improve that much either.

The code is not faster, it is not more readable,
but when you read this, are you sure 0x756e6547 is equal to "Genu" ?

Personally, I'm not.

But I agree using 'Genu' can have an impact on portability.

Regards,
Laurent

Signature · 3 lines
----------------- [email protected]  ------------------
  "La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry
#9 lore raw
In reply to Laurent Vivier
Quoted 5 lines
Quoted 1 line

+#define CPUID_VENDOR_AMD_3 0x444d4163 /* "cAMD" */

if you add "-Wno-multichar" in C flags, you can use:

#define CPUID_VENDOR_INTEL_1 'Genu'

I recommend against doing this. The meaning of multi-character constants is implementation defined, and the gcc definition has changed at least once.

Paul

#10 lore raw
In reply to Laurent Vivier

In message: <1218455786.3871.2.camel@frecb07144>
Laurent Vivier <[email protected]> writes:
: Le lundi 11 août 2008 à 12:37 +0200, Alexander Graf a écrit :
: > Right now CPU vendor identification contains a lot of magic numbers. : > The
: > patch cleans them up to defines, so we can identify the CPU later on : > without copying magic numbers.
: >
: > Signed-off-by: Alexander Graf <[email protected]>
: >
: >
: >
: > diff --git a/qemu/target-i386/cpu.h b/qemu/target-i386/cpu.h : > index 7e95900..3c84dc9 100644
: > --- a/qemu/target-i386/cpu.h
: > +++ b/qemu/target-i386/cpu.h
: > @@ -339,6 +341,14 @@

: >  #define CPUID_EXT3_IBS     (1 << 10)
: >  #define CPUID_EXT3_SKINIT  (1 << 12)

: >
: > +#define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */ : > +#define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */ : > +#define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */ : > +

: > +#define CPUID_VENDOR_AMD_1   0x68747541 /* "Auth" */
: > +#define CPUID_VENDOR_AMD_2   0x69746e65 /* "enti" */ 
: > +#define CPUID_VENDOR_AMD_3   0x444d4163 /* "cAMD" */

:
: if you add "-Wno-multichar" in C flags, you can use:
:
: #define CPUID_VENDOR_INTEL_1 'Genu'
: ...

Except that suffers from endian issues, no?

Warner

#7 lore raw
In reply to Laurent Vivier

In message: <1218459039.3871.18.camel@frecb07144>
Laurent Vivier <[email protected]> writes:
: Le lundi 11 août 2008 à 14:32 +0200, Alexander Graf a écrit :
: > On Aug 11, 2008, at 1:56 PM, Laurent Vivier wrote:
: >
: > > Le lundi 11 août 2008 à 12:37 +0200, Alexander Graf a écrit :
: > >> Right now CPU vendor identification contains a lot of magic numbers. : > >> The
: > >> patch cleans them up to defines, so we can identify the CPU later on : > >> without copying magic numbers.
: > >>
: > >> Signed-off-by: Alexander Graf <[email protected]>
: > >>
: > >>
: > >>
: > >> diff --git a/qemu/target-i386/cpu.h b/qemu/target-i386/cpu.h : > >> index 7e95900..3c84dc9 100644
: > >> --- a/qemu/target-i386/cpu.h
: > >> +++ b/qemu/target-i386/cpu.h
: > >> @@ -339,6 +341,14 @@
: > >> #define CPUID_EXT3_IBS (1 << 10)
: > >> #define CPUID_EXT3_SKINIT (1 << 12)
: > >>
: > >> +#define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
: > >> +#define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
: > >> +#define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
: > >> +

: > >> +#define CPUID_VENDOR_AMD_1   0x68747541 /* "Auth" */
: > >> +#define CPUID_VENDOR_AMD_2   0x69746e65 /* "enti" */
: > >> +#define CPUID_VENDOR_AMD_3   0x444d4163 /* "cAMD" */

: > >
: > > if you add "-Wno-multichar" in C flags, you can use:
: > >
: > > #define CPUID_VENDOR_INTEL_1 'Genu'
: > > ...
: >
: > Is that a real improvement? It won't make the code faster and the : > readability doesn't improve that much either.
:
: The code is not faster, it is not more readable,
: but when you read this, are you sure 0x756e6547 is equal to "Genu" ? :
: Personally, I'm not.

man ascii

and you'll be sure... Besides, you know you have an upper case letter, followed by 3 lower case ones just by glancing at it...

Warner

#8 lore raw
Quoted 44 lines · M. Warner Losh

In message: <1218459039.3871.18.camel@frecb07144>
Laurent Vivier <[email protected]> writes:
: Le lundi 11 août 2008 à 14:32 +0200, Alexander Graf a écrit :
: > On Aug 11, 2008, at 1:56 PM, Laurent Vivier wrote:
: >
: > > Le lundi 11 août 2008 à 12:37 +0200, Alexander Graf a écrit :
: > >> Right now CPU vendor identification contains a lot of magic numbers. : > >> The
: > >> patch cleans them up to defines, so we can identify the CPU later on : > >> without copying magic numbers.
: > >>
: > >> Signed-off-by: Alexander Graf <[email protected]>
: > >>
: > >>
: > >>
: > >> diff --git a/qemu/target-i386/cpu.h b/qemu/target-i386/cpu.h : > >> index 7e95900..3c84dc9 100644
: > >> --- a/qemu/target-i386/cpu.h
: > >> +++ b/qemu/target-i386/cpu.h
: > >> @@ -339,6 +341,14 @@
: > >> #define CPUID_EXT3_IBS (1 << 10)
: > >> #define CPUID_EXT3_SKINIT (1 << 12)
: > >>
: > >> +#define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
: > >> +#define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
: > >> +#define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
: > >> +

: > >> +#define CPUID_VENDOR_AMD_1   0x68747541 /* "Auth" */
: > >> +#define CPUID_VENDOR_AMD_2   0x69746e65 /* "enti" */
: > >> +#define CPUID_VENDOR_AMD_3   0x444d4163 /* "cAMD" */

: > >
: > > if you add "-Wno-multichar" in C flags, you can use:
: > >
: > > #define CPUID_VENDOR_INTEL_1 'Genu'
: > > ...
: >
: > Is that a real improvement? It won't make the code faster and the : > readability doesn't improve that much either.
:
: The code is not faster, it is not more readable,
: but when you read this, are you sure 0x756e6547 is equal to "Genu" ? :
: Personally, I'm not.

Quoted 5 lines

man ascii

and you'll be sure... Besides, you know you have an upper case letter, followed by 3 lower case ones just by glancing at it...

Well, this what I want to avoid.
But the comment from Paul is the good one.

Laurent

Signature · 3 lines
----------------- [email protected]  ------------------
  "La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry
#4 lore raw
In reply to Carlos A. M. dos Santos
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

#define CPUID_VENDOR_INTEL_1 'Genu'
...

Is that a real improvement? It won't make the code faster and the readability doesn't improve that much either.

I'd take it as a security improvement. One can easily commit a typo when editting magic numbers, either decimal or hexa.

I don't believe this argument at all. A bug is a bug, it's got nothing to do with "security". If anything the hex values are more reliable because you don't have to guess how the string is converted to an integer - this is proved by the fact that the above suggestion is incorrect[1]. The hex values are given in the cpu reference docs, so there shouldn't be any doubt whether they are correct.

[1] As someone else mentioned (though not very clearly) gcc interprets 'Genu' as a big-endian value, whereas the Intel docs define "Genu" as being little-endian.

Paul

#5 lore raw
Quoted 16 lines · Paul Brook
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

#define CPUID_VENDOR_INTEL_1 'Genu'
...

Is that a real improvement? It won't make the code faster and the readability doesn't improve that much either.

I'd take it as a security improvement. One can easily commit a typo when editting magic numbers, either decimal or hexa.

I don't believe this argument at all. A bug is a bug, it's got nothing to do with "security". If anything the hex values are more reliable because you don't have to guess how the string is converted to an integer - this is proved by the fact that the above suggestion is incorrect[1]. The hex values are given in the cpu reference docs, so there shouldn't be any doubt whether they are correct.

Quoted 3 lines

[1] As someone else mentioned (though not very clearly) gcc interprets 'Genu' as a big-endian value, whereas the Intel docs define "Genu" as being little-endian.

Right, this kills my argument. Let's go forward and stop wasting time discussing the color of the bikeshed. :-)

Signature · 2 lines
If you think things can't get worse it's probably only
because you lack sufficient imagination.