On Mon, 3 Dec 2012 15:27:57 -0200
Eduardo Habkost <ehabk...@redhat.com> 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 <ehabk...@redhat.com>
> ---
> target-i386/cpu.c | 64
> ++++++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 44 insertions(+), 20 deletions(-)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index c6c2ca0..89fd700 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1208,13 +1208,31 @@ 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, ",");
> + for (def = x86_defs; def; def = def->next)
> + if (name && !strcmp(name, def->name))
> + break;
> + if (kvm_enabled() && name && strcmp(name, "host") == 0) {
> + kvm_cpu_fill_host(x86_cpu_def);
> + } else if (!def) {
> + goto error;
Could you use "return -1;" here, there is no point to use goto in case of
single error exit point.
> + } else {
> + memcpy(x86_cpu_def, def, sizeof(*def));
> + }
> + return 0;
> +error:
> + return -1;
and would be nice to remove above 2 lines, it will make patch shorter.
> +}
> +
> +/* 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;
> @@ -1227,22 +1245,11 @@ static int cpu_x86_find_by_name(x86_def_t
> *x86_cpu_def, const char *cpu_model)
> 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;
> - if (kvm_enabled() && name && strcmp(name, "host") == 0) {
> - kvm_cpu_fill_host(x86_cpu_def);
> - } else if (!def) {
> - goto error;
> - } else {
> - memcpy(x86_cpu_def, def, sizeof(*def));
> - }
> -
> 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;
> @@ -1376,11 +1383,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;
> }
>
> @@ -1490,11 +1495,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;
> @@ -1553,7 +1572,12 @@ int cpu_x86_register(X86CPU *cpu, const char
> *cpu_model)
> error_free(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
>
>
Provided requested changes are applied,
Reviewed-By: Igor Mammedov <imamm...@redhat.com>
--
Regards,
Igor
