From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56450) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tfxve-0007LN-Fs for qemu-devel@nongnu.org; Tue, 04 Dec 2012 14:13:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tfxvc-00013l-L2 for qemu-devel@nongnu.org; Tue, 04 Dec 2012 14:13:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:63773) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tfxvc-00013T-Dr for qemu-devel@nongnu.org; Tue, 04 Dec 2012 14:13:12 -0500 Date: Tue, 4 Dec 2012 20:13:06 +0100 From: Igor Mammedov Message-ID: <20121204201306.4f343588@thinkpad.mammed.net> In-Reply-To: <1354647503-30900-3-git-send-email-ehabkost@redhat.com> References: <1354647503-30900-1-git-send-email-ehabkost@redhat.com> <1354647503-30900-3-git-send-email-ehabkost@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/6] target-i386: cpu: separate feature string parsing from CPU model lookup List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: Don Slutz , qemu-devel@nongnu.org, Andreas =?UTF-8?B?RsOkcmJlcg==?= On Tue, 4 Dec 2012 16:58:19 -0200 Eduardo Habkost wrote: > Instead of using parsing the whole cpu_model string inside > cpu_x86_find_by_name(), first split it into the CPU model name and the > full feature string, then parse the feature string into pieces. > > When using CPU model classes, those two pieces of information will be > used at different moments (CPU model name will be used to find CPU > class, feature string will be used after CPU object was created), so > making the split in two steps will make it easier to refactor the code > later. > > This should also help on the CPU properties work, that will just need to > replace the cpu_x86_parse_featurestr() logic (and can keep the CPU model > lookup code as-is). > > Signed-off-by: Eduardo Habkost > --- > Changes v1 -> v2: > - Coding style changes > - Replace "goto error" with "return -1" > --- > target-i386/cpu.c | 67 +++++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 45 insertions(+), 22 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 7afe839..4090152 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1208,25 +1208,10 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque, > cpu->env.tsc_khz = value / 1000; > } > > -static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) > +static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name) > { > - unsigned int i; > x86_def_t *def; > > - char *s = g_strdup(cpu_model); > - char *featurestr, *name = strtok(s, ","); > - /* Features to be added*/ > - uint32_t plus_features = 0, plus_ext_features = 0; > - uint32_t plus_ext2_features = 0, plus_ext3_features = 0; > - uint32_t plus_kvm_features = kvm_default_features, plus_svm_features = 0; > - uint32_t plus_7_0_ebx_features = 0; > - /* Features to be removed */ > - uint32_t minus_features = 0, minus_ext_features = 0; > - uint32_t minus_ext2_features = 0, minus_ext3_features = 0; > - uint32_t minus_kvm_features = 0, minus_svm_features = 0; > - uint32_t minus_7_0_ebx_features = 0; > - uint32_t numvalue; > - > for (def = x86_defs; def; def = def->next) { > if (name && !strcmp(name, def->name)) { > break; > @@ -1235,16 +1220,37 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) > if (kvm_enabled() && name && strcmp(name, "host") == 0) { > kvm_cpu_fill_host(x86_cpu_def); > } else if (!def) { > - goto error; > + return -1; > } else { > memcpy(x86_cpu_def, def, sizeof(*def)); > } > > + return 0; > +} > + > +/* Parse "+feature,-feature,feature=foo" CPU feature string > + */ > +static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) > +{ > + unsigned int i; > + char *featurestr; /* Single 'key=value" string being parsed */ > + /* Features to be added*/ > + uint32_t plus_features = 0, plus_ext_features = 0; > + uint32_t plus_ext2_features = 0, plus_ext3_features = 0; > + uint32_t plus_kvm_features = kvm_default_features, plus_svm_features = 0; > + uint32_t plus_7_0_ebx_features = 0; > + /* Features to be removed */ > + uint32_t minus_features = 0, minus_ext_features = 0; > + uint32_t minus_ext2_features = 0, minus_ext3_features = 0; > + uint32_t minus_kvm_features = 0, minus_svm_features = 0; > + uint32_t minus_7_0_ebx_features = 0; > + uint32_t numvalue; > + > add_flagname_to_bitmaps("hypervisor", &plus_features, > &plus_ext_features, &plus_ext2_features, &plus_ext3_features, > &plus_kvm_features, &plus_svm_features, &plus_7_0_ebx_features); > > - featurestr = strtok(NULL, ","); > + featurestr = features ? strtok(features, ",") : NULL; > > while (featurestr) { > char *val; > @@ -1378,11 +1384,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) > if (x86_cpu_def->cpuid_7_0_ebx_features && x86_cpu_def->level < 7) { > x86_cpu_def->level = 7; > } > - g_free(s); > return 0; > > error: > - g_free(s); > return -1; > } > > @@ -1492,11 +1496,25 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) > CPUX86State *env = &cpu->env; > x86_def_t def1, *def = &def1; > Error *error = NULL; > + char *name, *features; > + gchar **model_pieces; > > memset(def, 0, sizeof(*def)); > > - if (cpu_x86_find_by_name(def, cpu_model) < 0) > - return -1; > + model_pieces = g_strsplit(cpu_model, ",", 2); > + if (!model_pieces[0]) { > + goto error; > + } > + name = model_pieces[0]; > + features = model_pieces[1]; > + > + if (cpu_x86_find_by_name(def, name) < 0) { > + goto error; > + } > + > + if (cpu_x86_parse_featurestr(def, features) < 0) { > + goto error; > + } > if (def->vendor1) { > env->cpuid_vendor1 = def->vendor1; > env->cpuid_vendor2 = def->vendor2; > @@ -1555,7 +1573,12 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) > error_free(error); model_pieces is leaked, s/return -1/goto error/ > return -1; > } > + > + g_strfreev(model_pieces); > return 0; > +error: > + g_strfreev(model_pieces); > + return -1; > } > > #if !defined(CONFIG_USER_ONLY) > -- > 1.7.11.7 > > -- Regards, Igor