Message ID | 1357870231-26762-5-git-send-email-imammedo@redhat.com |
---|---|
State | New |
Headers | show |
Am 11.01.2013 03:28, schrieb Eduardo Habkost: > On Fri, Jan 11, 2013 at 03:10:18AM +0100, Igor Mammedov wrote: >> Make for() cycle reusable for the next patch >> >> Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > Very minor style comment below. > >> --- >> v4: >> - rename x86cpu_vendor_words2str() to x86_cpu_vendor_words2str() >> Suggested-By: Andreas Färber <afaerber@suse.de> >> v3: >> -replace e[bcd]x arguments naming with vendor[123] >> -fix/swap vendor2 and vendor3 order >> Spotted-By: Eduardo Habkost <ehabkost@redhat.com> >> v2: >> -place x86cpu_vendor_words2str() a bit earlier, before feature >> arrays to avoid compile error when vendor property is converted >> static property. >> --- >> target-i386/cpu.c | 21 ++++++++++++++------- >> 1 files changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c >> index c4ff761..64cc88f 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) Same nit here. >> +{ >> + 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'; >> +} >> + >> /* feature flags taken from "Intel Processor Identification and the CPUID >> * Instruction" and AMD's "CPUID Specification". In cases of disagreement >> * between feature naming conventions, aliases may be added. >> @@ -1213,15 +1225,10 @@ static char *x86_cpuid_get_vendor(Object *obj, Error **errp) >> X86CPU *cpu = X86_CPU(obj); >> CPUX86State *env = &cpu->env; >> char *value; >> - int i; >> >> value = (char *)g_malloc(CPUID_VENDOR_SZ + 1); >> - for (i = 0; i < 4; i++) { >> - value[i ] = env->cpuid_vendor1 >> (8 * i); >> - value[i + 4] = env->cpuid_vendor2 >> (8 * i); >> - value[i + 8] = env->cpuid_vendor3 >> (8 * i); >> - } >> - value[CPUID_VENDOR_SZ] = '\0'; >> + x86_cpu_vendor_words2str(value, env->cpuid_vendor1, env->cpuid_vendor2, >> + env->cpuid_vendor3); > > I would add one extra space, so the argument in the second line are > aligned with the first argument from the previous line. I can edit both in myself, but I won't manage to go through the whole patchset(s) til tomorrow. 1-4 look fine so far. Andreas > >> return value; >> } >> >> -- >> 1.7.1 >> >
On Mon, 14 Jan 2013 18:14:27 +0100 Andreas Färber <afaerber@suse.de> wrote: > Am 11.01.2013 03:28, schrieb Eduardo Habkost: > > On Fri, Jan 11, 2013 at 03:10:18AM +0100, Igor Mammedov wrote: > >> Make for() cycle reusable for the next patch > >> > >> Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > > > Very minor style comment below. > > > >> --- > >> v4: > >> - rename x86cpu_vendor_words2str() to x86_cpu_vendor_words2str() > >> Suggested-By: Andreas Färber <afaerber@suse.de> > >> v3: > >> -replace e[bcd]x arguments naming with vendor[123] > >> -fix/swap vendor2 and vendor3 order > >> Spotted-By: Eduardo Habkost <ehabkost@redhat.com> > >> v2: > >> -place x86cpu_vendor_words2str() a bit earlier, before feature > >> arrays to avoid compile error when vendor property is converted > >> static property. > >> --- > >> target-i386/cpu.c | 21 ++++++++++++++------- > >> 1 files changed, 14 insertions(+), 7 deletions(-) > >> > >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c > >> index c4ff761..64cc88f 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) > > Same nit here. I'll resubmit fixed patch. Thanks! > > >> +{ > >> + 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'; > >> +} > >> + > >> /* feature flags taken from "Intel Processor Identification and the CPUID > >> * Instruction" and AMD's "CPUID Specification". In cases of disagreement > >> * between feature naming conventions, aliases may be added. > >> @@ -1213,15 +1225,10 @@ static char *x86_cpuid_get_vendor(Object *obj, Error **errp) > >> X86CPU *cpu = X86_CPU(obj); > >> CPUX86State *env = &cpu->env; > >> char *value; > >> - int i; > >> > >> value = (char *)g_malloc(CPUID_VENDOR_SZ + 1); > >> - for (i = 0; i < 4; i++) { > >> - value[i ] = env->cpuid_vendor1 >> (8 * i); > >> - value[i + 4] = env->cpuid_vendor2 >> (8 * i); > >> - value[i + 8] = env->cpuid_vendor3 >> (8 * i); > >> - } > >> - value[CPUID_VENDOR_SZ] = '\0'; > >> + x86_cpu_vendor_words2str(value, env->cpuid_vendor1, env->cpuid_vendor2, > >> + env->cpuid_vendor3); > > > > I would add one extra space, so the argument in the second line are > > aligned with the first argument from the previous line. it was fixed in "[PATCH 04/17 v2] target-i386: add x86_cpu_vendor_words2str()" follow-up to original patch. > > I can edit both in myself, but I won't manage to go through the whole > patchset(s) til tomorrow. 1-4 look fine so far. No need to edit. I'll re-submit fixed version in a moment. > > Andreas > > > > >> return value; > >> } > >> > >> -- > >> 1.7.1 > >> > > > > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg >
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index c4ff761..64cc88f 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) +{ + 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'; +} + /* feature flags taken from "Intel Processor Identification and the CPUID * Instruction" and AMD's "CPUID Specification". In cases of disagreement * between feature naming conventions, aliases may be added. @@ -1213,15 +1225,10 @@ static char *x86_cpuid_get_vendor(Object *obj, Error **errp) X86CPU *cpu = X86_CPU(obj); CPUX86State *env = &cpu->env; char *value; - int i; value = (char *)g_malloc(CPUID_VENDOR_SZ + 1); - for (i = 0; i < 4; i++) { - value[i ] = env->cpuid_vendor1 >> (8 * i); - value[i + 4] = env->cpuid_vendor2 >> (8 * i); - value[i + 8] = env->cpuid_vendor3 >> (8 * i); - } - value[CPUID_VENDOR_SZ] = '\0'; + x86_cpu_vendor_words2str(value, env->cpuid_vendor1, env->cpuid_vendor2, + env->cpuid_vendor3); return value; }
Make for() cycle reusable for the next patch Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- v4: - rename x86cpu_vendor_words2str() to x86_cpu_vendor_words2str() Suggested-By: Andreas Färber <afaerber@suse.de> v3: -replace e[bcd]x arguments naming with vendor[123] -fix/swap vendor2 and vendor3 order Spotted-By: Eduardo Habkost <ehabkost@redhat.com> v2: -place x86cpu_vendor_words2str() a bit earlier, before feature arrays to avoid compile error when vendor property is converted static property. --- target-i386/cpu.c | 21 ++++++++++++++------- 1 files changed, 14 insertions(+), 7 deletions(-)