Re: [Qemu-devel] [PATCH 2/5] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t

[Igor Mammedov]

On Fri, 18 Jan 2013 15:12:36 +0800
li guang <address@hidden> wrote:

> 在 2013-01-17四的 16:16 +0100，Igor Mammedov写道：
> 
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index ce914da..ab80dbe 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -45,6 +45,18 @@
> >  #include "hw/apic_internal.h"
> >  #endif
> >  
> > +static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
> > +                                     uint32_t vendor2, uint32_t vendor3)
> 
> sorry, but I should say "_vendor_words2str" seems not so suitable,
> it's mostly not a convertor, but a compactor, so I suggest to use
> "_vendor_str" directly.
I think that "_vendor_words2str" describes more clearly what function does,
regardless whether it is conversion or compaction. "_vendor_str" seems more
ambiguous though. But if you insist, I can change to it.

BTW: it's not just copying, it copies from little endinan words to string.

> 
> > +{
> > +    int i;
> > +    for (i = 0; i < 4; i++) {
> > +        dst[i] = vendor1 >> (8 * i);
> > +        dst[i + 4] = vendor2 >> (8 * i);
> > +        dst[i + 8] = vendor3 >> (8 * i);
> > +    }
> > +    dst[CPUID_VENDOR_SZ] = '\0';
> > +}
> > +
> 
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -537,14 +537,14 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
> >  #define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
> >  #define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
> >  #define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
> > +#define CPUID_VENDOR_INTEL "GenuineIntel"
> >  
> 
> you said the reason you did not remove _VENDOR_INTEL_{1,2,3}
> is they're used somewhere, did you mean "target-i386/translate.c"
> for sysenter instruction?
> if it is, why can't we also remove them there?
That would imply conversion of CPUX86State to using string for cpuid_vendor
instead of currents words which would mean to do conversion every time cpuid
instruction is called in guest. I'd rather keep current cpuid_vendor{1,2,3}
in CPUX86State.

Purpose of this patch is to switch from direct field copying when initializing
CPU to using property setter.
If we ever decide to convert CPUX86State.cpuid_vendor{1,2,3} into string, it
could be done by a separate patch.

In addition, wouldn't strcmp() there be less effective performance wise,
versus just number comparison if we would convert
CPUX86State.cpuid_vendor{1,2,3} to string?

> 
> >  #define CPUID_VENDOR_AMD_1   0x68747541 /* "Auth" */
> >  #define CPUID_VENDOR_AMD_2   0x69746e65 /* "enti" */
> >  #define CPUID_VENDOR_AMD_3   0x444d4163 /* "cAMD" */
> > +#define CPUID_VENDOR_AMD   "AuthenticAMD"
> >  
> > -#define CPUID_VENDOR_VIA_1   0x746e6543 /* "Cent" */
> > -#define CPUID_VENDOR_VIA_2   0x48727561 /* "aurH" */
> > -#define CPUID_VENDOR_VIA_3   0x736c7561 /* "auls" */
> > +#define CPUID_VENDOR_VIA   "CentaurHauls"
> >  
> >  #define CPUID_MWAIT_IBE     (1 << 1) /* Interrupts can exit capability */
> >  #define CPUID_MWAIT_EMX     (1 << 0) /* enumeration supported */
>