[Qemu-devel] [PATCH 1/3] [x86] Clean up vendor identification

[Qemu-devel] [PATCH 1/3] [x86] Clean up vendor identification

[Alexander Graf]

[-- Attachment #1: Type: text/plain, Size: 225 bytes --]

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 <agraf@suse.de>




[-- Attachment #2: se01-clean-vendor.patch --]
[-- Type: text/x-patch, Size: 1873 bytes --]

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" */
+
 #define EXCP00_DIVZ	0
 #define EXCP01_SSTP	1
 #define EXCP02_NMI	2
diff --git a/qemu/target-i386/helper.c b/qemu/target-i386/helper.c
index 73ce7da..0a02a90 100644
--- a/qemu/target-i386/helper.c
+++ b/qemu/target-i386/helper.c
@@ -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;

Re: [Qemu-devel] [PATCH 1/3] [x86] Clean up vendor identification

[Laurent Vivier]

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 <agraf@suse.de>
> 
> 
> 
> 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'
...

Regards,
Laurent
-- 
----------------- Laurent.Vivier@bull.net  ------------------
  "La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry

Re: [Qemu-devel] [PATCH 1/3] [x86] Clean up vendor identification

[Alexander Graf]

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 <agraf@suse.de>
>>
>>
>>
>> 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.

Alex

>
>
> Regards,
> Laurent
> -- 
> ----------------- Laurent.Vivier@bull.net  ------------------
>  "La perfection est atteinte non quand il ne reste rien à
> ajouter mais quand il ne reste rien à enlever." Saint Exupéry
>
>
>

Re: [Qemu-devel] [PATCH 1/3] [x86] Clean up vendor identification

[Laurent Vivier]

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 <agraf@suse.de>
> >>
> >>
> >>
> >> 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.

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

Regards,
Laurent
-- 
----------------- Laurent.Vivier@bull.net  ------------------
  "La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry

Re: [Qemu-devel] [PATCH 1/3] [x86] Clean up vendor identification

[Carlos A. M. dos Santos]

On Mon, Aug 11, 2008 at 9:32 AM, Alexander Graf <agraf@suse.de> wrote:
>
> 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 <agraf@suse.de>
>>>
>>>
>>>
>>> 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.

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

-- 
If you think things can't get worse it's probably only
because you lack sufficient imagination.

Re: [Qemu-devel] [PATCH 1/3] [x86] Clean up vendor identification

[Paul Brook]

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

Re: [Qemu-devel] [PATCH 1/3] [x86] Clean up vendor identification

[M. Warner Losh]

In message: <1218455786.3871.2.camel@frecb07144>
            Laurent Vivier <Laurent.Vivier@bull.net> 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 <agraf@suse.de>
: > 
: > 
: > 
: > 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

Re: [Qemu-devel] [PATCH 1/3] [x86] Clean up vendor identification

[M. Warner Losh]

In message: <1218459039.3871.18.camel@frecb07144>
            Laurent Vivier <Laurent.Vivier@bull.net> 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 <agraf@suse.de>
: > >>
: > >>
: > >>
: > >> 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

Re: [Qemu-devel] [PATCH 1/3] [x86] Clean up vendor identification

[Laurent Vivier]

Le lundi 11 août 2008 à 07:37 -0600, M. Warner Losh a écrit :
> In message: <1218459039.3871.18.camel@frecb07144>
>             Laurent Vivier <Laurent.Vivier@bull.net> 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 <agraf@suse.de>
> : > >>
> : > >>
> : > >>
> : > >> 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...
> 

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

Laurent
-- 
----------------- Laurent.Vivier@bull.net  ------------------
  "La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry

Re: [Qemu-devel] [PATCH 1/3] [x86] Clean up vendor identification

[Paul Brook]

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

Re: [Qemu-devel] [PATCH 1/3] [x86] Clean up vendor identification

[Carlos A. M. dos Santos]

On Mon, Aug 11, 2008 at 11:51 AM, Paul Brook <paul@codesourcery.com> wrote:
>> >> #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.

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

-- 
If you think things can't get worse it's probably only
because you lack sufficient imagination.