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

[li guang]

在 2013-01-18五的 14:40 +0100，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.
> 

No, "_vendor_words2str" is OK, though I still prefer "_vendor_str"
stubbornly :)

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

that's true.

Thanks!

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

-- 
regards!
li guang