From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:33602) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TwCBg-0002w6-Fr for qemu-devel@nongnu.org; Fri, 18 Jan 2013 08:40:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TwCBe-0004Nd-Nw for qemu-devel@nongnu.org; Fri, 18 Jan 2013 08:40:52 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59259) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TwCBe-0004NR-FF for qemu-devel@nongnu.org; Fri, 18 Jan 2013 08:40:50 -0500 Date: Fri, 18 Jan 2013 14:40:42 +0100 From: Igor Mammedov Message-ID: <20130118144042.479acd50@nial.usersys.redhat.com> In-Reply-To: <1358493156.15036.22.camel@liguang.fnst.cn.fujitsu.com> References: <1358435794-8406-1-git-send-email-imammedo@redhat.com> <1358435794-8406-3-git-send-email-imammedo@redhat.com> <1358493156.15036.22.camel@liguang.fnst.cn.fujitsu.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/5] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: li guang Cc: qemu-devel@nongnu.org, afaerber@suse.de, ehabkost@redhat.com On Fri, 18 Jan 2013 15:12:36 +0800 li guang wrote: > =E5=9C=A8 2013-01-17=E5=9B=9B=E7=9A=84 16:16 +0100=EF=BC=8CIgor Mammedov= =E5=86=99=E9=81=93=EF=BC=9A >=20 > >=20 > > 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 > > =20 > > +static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1, > > + uint32_t vendor2, uint32_t vendor= 3) >=20 > 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. >=20 > > +{ > > + int i; > > + for (i =3D 0; i < 4; i++) { > > + dst[i] =3D vendor1 >> (8 * i); > > + dst[i + 4] =3D vendor2 >> (8 * i); > > + dst[i + 8] =3D vendor3 >> (8 * i); > > + } > > + dst[CPUID_VENDOR_SZ] =3D '\0'; > > +} > > + >=20 > > --- 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" > > =20 >=20 > 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 initializ= ing 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? >=20 > > #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" > > =20 > > -#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" > > =20 > > #define CPUID_MWAIT_IBE (1 << 1) /* Interrupts can exit capability= */ > > #define CPUID_MWAIT_EMX (1 << 0) /* enumeration supported */ >=20