On Tue, 4 Dec 2012 16:58:19 -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>
> ---
> 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
