| Message ID | 1358435794-8406-5-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jan 17, 2013 at 04:16:33PM +0100, Igor Mammedov wrote: > Move custom features parsing after built-in cpu_model defaults are set > and set custom features directly on CPU instance. That allows to make > clear distinction between built-in cpu model defaults that eventually > should go into clas_init() and extra property setting which is done > after defaults are set on CPU instance. > > Impl. details: > - features that are already properties, are converted to normalized > (name, values) list. And after featurestr has been parsed, > properties from the list are applied directly to CPU instance. > * For now it provides uniform handling of properties with single > object_property_parse() property setter. > * And after current features/properties are converted into static > properties, it will take a trivial patch to switch to global properties. > Which will allow to: > * get CPU instance initialized with all parameters passed on -cpu ... > cmd. line from object_new() call. > * call cpu_model/featurestr parsing only once before CPUs are created > * open a road for removing CPUxxxState.cpu_model_str field, when other > CPUs are similarly converted to subclasses and static properties. > - re-factor error handling, to use Error instead of fprintf()s, since > it is anyway passed in for property setter. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > target-i386/cpu.c | 144 ++++++++++++++++++++++++++++------------------------- > 1 files changed, 77 insertions(+), 67 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 5cd7917..50e10b1 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1302,9 +1302,24 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name) > return 0; > } > > +typedef struct NameValuePair { > + char *name; > + char *value; > + QTAILQ_ENTRY(NameValuePair) next; > +} NameValuePair; > +typedef QTAILQ_HEAD(NVList, NameValuePair) NVList; > + > +static void x86_cpu_add_nv_pair(NVList *list, const char *name, > + const char *value) { > + NameValuePair *p = g_malloc0(sizeof(*p)); > + p->name = g_strdup(name); > + p->value = g_strdup(value); > + QTAILQ_INSERT_TAIL(list, p, next); > +} I am not sure I like this extra complexity. We don't need it if we simply set/register the properties directly. I have a different proposal: 1) By now, use: object_property_parse(OBJECT(cpu), P, V, errp) in the places you are using: x86_cpu_add_nv_pair(&props, P, V) below. 2) The day we move to global properties, we just need to mechanically replace the occurrences of: object_property_parse(OBJECT(cpu), P, V, errp) with: qdev_prop_register_global("X86CPU", P, V) What do you think? > + object_property_parse(OBJECT(cpu), p->value, p->name, errp); > + > /* Parse "+feature,-feature,feature=foo" CPU feature string > */ > -static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) > +static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp) > { > char *featurestr; /* Single 'key=value" string being parsed */ > /* Features to be added */ > @@ -1312,6 +1327,9 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) > /* Features to be removed */ > FeatureWordArray minus_features = { 0 }; > uint32_t numvalue; > + CPUX86State *env = &cpu->env; > + NVList props = QTAILQ_HEAD_INITIALIZER(props); > + NameValuePair *p, *tmp; > > featurestr = features ? strtok(features, ",") : NULL; > > @@ -1324,77 +1342,57 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) > } else if ((val = strchr(featurestr, '='))) { > *val = 0; val++; > if (!strcmp(featurestr, "family")) { > - char *err; > - numvalue = strtoul(val, &err, 0); > - if (!*val || *err || numvalue > 0xff + 0xf) { > - fprintf(stderr, "bad numerical value %s\n", val); > - goto error; > - } > - x86_cpu_def->family = numvalue; > + x86_cpu_add_nv_pair(&props, featurestr, val); > } else if (!strcmp(featurestr, "model")) { > - char *err; > - numvalue = strtoul(val, &err, 0); > - if (!*val || *err || numvalue > 0xff) { > - fprintf(stderr, "bad numerical value %s\n", val); > - goto error; > - } > - x86_cpu_def->model = numvalue; > + x86_cpu_add_nv_pair(&props, featurestr, val); > } else if (!strcmp(featurestr, "stepping")) { > - char *err; > - numvalue = strtoul(val, &err, 0); > - if (!*val || *err || numvalue > 0xf) { > - fprintf(stderr, "bad numerical value %s\n", val); > - goto error; > - } > - x86_cpu_def->stepping = numvalue ; > + x86_cpu_add_nv_pair(&props, featurestr, val); > } else if (!strcmp(featurestr, "level")) { > - char *err; > - numvalue = strtoul(val, &err, 0); > - if (!*val || *err) { > - fprintf(stderr, "bad numerical value %s\n", val); > - goto error; > - } > - x86_cpu_def->level = numvalue; > + x86_cpu_add_nv_pair(&props, featurestr, val); > } else if (!strcmp(featurestr, "xlevel")) { > char *err; > + char num[32]; > + > numvalue = strtoul(val, &err, 0); > if (!*val || *err) { > - fprintf(stderr, "bad numerical value %s\n", val); > - goto error; > + error_setg(errp, "bad numerical value %s\n", val); > + goto out; > } > if (numvalue < 0x80000000) { > fprintf(stderr, "xlevel value shall always be >= 0x80000000" > ", fixup will be deprecated in future versions\n"); > numvalue += 0x80000000; > } > - x86_cpu_def->xlevel = numvalue; > + snprintf(num, sizeof(num), "%" PRIu32, numvalue); > + x86_cpu_add_nv_pair(&props, featurestr, num); > } else if (!strcmp(featurestr, "vendor")) { > - pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val); > + x86_cpu_add_nv_pair(&props, featurestr, val); > } else if (!strcmp(featurestr, "model_id")) { > - pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id), > - val); > + x86_cpu_add_nv_pair(&props, "model-id", val); > } else if (!strcmp(featurestr, "tsc_freq")) { > int64_t tsc_freq; > char *err; > + char num[32]; > > tsc_freq = strtosz_suffix_unit(val, &err, > STRTOSZ_DEFSUFFIX_B, 1000); > if (tsc_freq < 0 || *err) { > - fprintf(stderr, "bad numerical value %s\n", val); > - goto error; > + error_setg(errp, "bad numerical value %s\n", val); > + goto out; > } > - x86_cpu_def->tsc_khz = tsc_freq / 1000; > + snprintf(num, sizeof(num), "%" PRId64, tsc_freq); > + x86_cpu_add_nv_pair(&props, "tsc-frequency", num); > } else if (!strcmp(featurestr, "hv_spinlocks")) { > char *err; > numvalue = strtoul(val, &err, 0); > if (!*val || *err) { > - fprintf(stderr, "bad numerical value %s\n", val); > - goto error; > + error_setg(errp, "bad numerical value %s\n", val); > + goto out; > } > hyperv_set_spinlock_retries(numvalue); > } else { > - fprintf(stderr, "unrecognized feature %s\n", featurestr); > - goto error; > + error_setg(errp, "unrecognized feature %s\n", featurestr); > + goto out; > } > } else if (!strcmp(featurestr, "check")) { > check_cpuid = 1; > @@ -1405,31 +1403,46 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) > } else if (!strcmp(featurestr, "hv_vapic")) { > hyperv_enable_vapic_recommended(true); > } else { > - fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr); > - goto error; > + error_setg(errp, "feature string `%s' not in format (+feature|" > + "-feature|feature=xyz)\n", featurestr); > + goto out; > } > featurestr = strtok(NULL, ","); > } > - x86_cpu_def->features |= plus_features[FEAT_1_EDX]; > - x86_cpu_def->ext_features |= plus_features[FEAT_1_ECX]; > - x86_cpu_def->ext2_features |= plus_features[FEAT_8000_0001_EDX]; > - x86_cpu_def->ext3_features |= plus_features[FEAT_8000_0001_ECX]; > - x86_cpu_def->ext4_features |= plus_features[FEAT_C000_0001_EDX]; > - x86_cpu_def->kvm_features |= plus_features[FEAT_KVM]; > - x86_cpu_def->svm_features |= plus_features[FEAT_SVM]; > - x86_cpu_def->cpuid_7_0_ebx_features |= plus_features[FEAT_7_0_EBX]; > - x86_cpu_def->features &= ~minus_features[FEAT_1_EDX]; > - x86_cpu_def->ext_features &= ~minus_features[FEAT_1_ECX]; > - x86_cpu_def->ext2_features &= ~minus_features[FEAT_8000_0001_EDX]; > - x86_cpu_def->ext3_features &= ~minus_features[FEAT_8000_0001_ECX]; > - x86_cpu_def->ext4_features &= ~minus_features[FEAT_C000_0001_EDX]; > - x86_cpu_def->kvm_features &= ~minus_features[FEAT_KVM]; > - x86_cpu_def->svm_features &= ~minus_features[FEAT_SVM]; > - x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX]; > - return 0; > + env->cpuid_features |= plus_features[FEAT_1_EDX]; > + env->cpuid_ext_features |= plus_features[FEAT_1_ECX]; > + env->cpuid_ext2_features |= plus_features[FEAT_8000_0001_EDX]; > + env->cpuid_ext3_features |= plus_features[FEAT_8000_0001_ECX]; > + env->cpuid_ext4_features |= plus_features[FEAT_C000_0001_EDX]; > + env->cpuid_kvm_features |= plus_features[FEAT_KVM]; > + env->cpuid_svm_features |= plus_features[FEAT_SVM]; > + env->cpuid_7_0_ebx_features |= plus_features[FEAT_7_0_EBX]; > + env->cpuid_features &= ~minus_features[FEAT_1_EDX]; > + env->cpuid_ext_features &= ~minus_features[FEAT_1_ECX]; > + env->cpuid_ext2_features &= ~minus_features[FEAT_8000_0001_EDX]; > + env->cpuid_ext3_features &= ~minus_features[FEAT_8000_0001_ECX]; > + env->cpuid_ext4_features &= ~minus_features[FEAT_C000_0001_EDX]; > + env->cpuid_kvm_features &= ~minus_features[FEAT_KVM]; > + env->cpuid_svm_features &= ~minus_features[FEAT_SVM]; > + env->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX]; > + > + /* Set features on X86CPU object based on a provided key,value list */ > + QTAILQ_FOREACH_SAFE(p, &props, next, tmp) { > + /* TODO: switch to using global properties after subclasses and > + * static properties are done */ > + object_property_parse(OBJECT(cpu), p->value, p->name, errp); > + if (error_is_set(errp)) { > + goto out; > + } > + } > > -error: > - return -1; > +out: > + QTAILQ_FOREACH_SAFE(p, &props, next, tmp) { > + QTAILQ_REMOVE(&props, p, next); > + g_free(p->value); > + g_free(p->name); > + g_free(p); > + } > } > > /* generate a composite string into buf of all cpuid names in featureset > @@ -1559,10 +1572,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) > def->kvm_features |= kvm_default_features; > def->ext_features |= CPUID_EXT_HYPERVISOR; > > - if (cpu_x86_parse_featurestr(def, features) < 0) { > - error_setg(&error, "Invalid cpu_model string format: %s", cpu_model); > - goto out; > - } > assert(def->vendor[0]); > object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error); > object_property_set_int(OBJECT(cpu), def->level, "level", &error); > @@ -1584,6 +1593,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) > > object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error); > > + cpu_x86_parse_featurestr(cpu, features, &error); > out: > g_strfreev(model_pieces); > if (error) { > -- > 1.7.1 >
On Thu, 17 Jan 2013 15:44:45 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Jan 17, 2013 at 04:16:33PM +0100, Igor Mammedov wrote: > > Move custom features parsing after built-in cpu_model defaults are set > > and set custom features directly on CPU instance. That allows to make > > clear distinction between built-in cpu model defaults that eventually > > should go into clas_init() and extra property setting which is done > > after defaults are set on CPU instance. > > > > Impl. details: > > - features that are already properties, are converted to normalized > > (name, values) list. And after featurestr has been parsed, > > properties from the list are applied directly to CPU instance. > > * For now it provides uniform handling of properties with single > > object_property_parse() property setter. > > * And after current features/properties are converted into static > > properties, it will take a trivial patch to switch to global > > properties. Which will allow to: > > * get CPU instance initialized with all parameters passed on -cpu ... > > cmd. line from object_new() call. > > * call cpu_model/featurestr parsing only once before CPUs are created > > * open a road for removing CPUxxxState.cpu_model_str field, when > > other CPUs are similarly converted to subclasses and static properties. > > - re-factor error handling, to use Error instead of fprintf()s, since > > it is anyway passed in for property setter. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > target-i386/cpu.c | 144 > > ++++++++++++++++++++++++++++------------------------- 1 files changed, 77 > > insertions(+), 67 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 5cd7917..50e10b1 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -1302,9 +1302,24 @@ static int cpu_x86_find_by_name(x86_def_t > > *x86_cpu_def, const char *name) return 0; > > } > > > > +typedef struct NameValuePair { > > + char *name; > > + char *value; > > + QTAILQ_ENTRY(NameValuePair) next; > > +} NameValuePair; > > +typedef QTAILQ_HEAD(NVList, NameValuePair) NVList; > > + > > +static void x86_cpu_add_nv_pair(NVList *list, const char *name, > > + const char *value) { > > + NameValuePair *p = g_malloc0(sizeof(*p)); > > + p->name = g_strdup(name); > > + p->value = g_strdup(value); > > + QTAILQ_INSERT_TAIL(list, p, next); > > +} > > I am not sure I like this extra complexity. We don't need it if we > simply set/register the properties directly. > > I have a different proposal: > > 1) By now, use: > object_property_parse(OBJECT(cpu), P, V, errp) > in the places you are using: > x86_cpu_add_nv_pair(&props, P, V) > below. > > 2) The day we move to global properties, we just need to mechanically > replace the occurrences of: > object_property_parse(OBJECT(cpu), P, V, errp) > with: > qdev_prop_register_global("X86CPU", P, V) > > What do you think? Agreed, changes, when switching to global properties, will be mechanical and not in many places, so it makes sense to drop list approach. I'll resubmit series. > > > > + object_property_parse(OBJECT(cpu), p->value, p->name, errp); > > + > > /* Parse "+feature,-feature,feature=foo" CPU feature string > > */ [...]
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 5cd7917..50e10b1 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1302,9 +1302,24 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name) return 0; } +typedef struct NameValuePair { + char *name; + char *value; + QTAILQ_ENTRY(NameValuePair) next; +} NameValuePair; +typedef QTAILQ_HEAD(NVList, NameValuePair) NVList; + +static void x86_cpu_add_nv_pair(NVList *list, const char *name, + const char *value) { + NameValuePair *p = g_malloc0(sizeof(*p)); + p->name = g_strdup(name); + p->value = g_strdup(value); + QTAILQ_INSERT_TAIL(list, p, next); +} + /* Parse "+feature,-feature,feature=foo" CPU feature string */ -static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) +static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp) { char *featurestr; /* Single 'key=value" string being parsed */ /* Features to be added */ @@ -1312,6 +1327,9 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) /* Features to be removed */ FeatureWordArray minus_features = { 0 }; uint32_t numvalue; + CPUX86State *env = &cpu->env; + NVList props = QTAILQ_HEAD_INITIALIZER(props); + NameValuePair *p, *tmp; featurestr = features ? strtok(features, ",") : NULL; @@ -1324,77 +1342,57 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) } else if ((val = strchr(featurestr, '='))) { *val = 0; val++; if (!strcmp(featurestr, "family")) { - char *err; - numvalue = strtoul(val, &err, 0); - if (!*val || *err || numvalue > 0xff + 0xf) { - fprintf(stderr, "bad numerical value %s\n", val); - goto error; - } - x86_cpu_def->family = numvalue; + x86_cpu_add_nv_pair(&props, featurestr, val); } else if (!strcmp(featurestr, "model")) { - char *err; - numvalue = strtoul(val, &err, 0); - if (!*val || *err || numvalue > 0xff) { - fprintf(stderr, "bad numerical value %s\n", val); - goto error; - } - x86_cpu_def->model = numvalue; + x86_cpu_add_nv_pair(&props, featurestr, val); } else if (!strcmp(featurestr, "stepping")) { - char *err; - numvalue = strtoul(val, &err, 0); - if (!*val || *err || numvalue > 0xf) { - fprintf(stderr, "bad numerical value %s\n", val); - goto error; - } - x86_cpu_def->stepping = numvalue ; + x86_cpu_add_nv_pair(&props, featurestr, val); } else if (!strcmp(featurestr, "level")) { - char *err; - numvalue = strtoul(val, &err, 0); - if (!*val || *err) { - fprintf(stderr, "bad numerical value %s\n", val); - goto error; - } - x86_cpu_def->level = numvalue; + x86_cpu_add_nv_pair(&props, featurestr, val); } else if (!strcmp(featurestr, "xlevel")) { char *err; + char num[32]; + numvalue = strtoul(val, &err, 0); if (!*val || *err) { - fprintf(stderr, "bad numerical value %s\n", val); - goto error; + error_setg(errp, "bad numerical value %s\n", val); + goto out; } if (numvalue < 0x80000000) { fprintf(stderr, "xlevel value shall always be >= 0x80000000" ", fixup will be deprecated in future versions\n"); numvalue += 0x80000000; } - x86_cpu_def->xlevel = numvalue; + snprintf(num, sizeof(num), "%" PRIu32, numvalue); + x86_cpu_add_nv_pair(&props, featurestr, num); } else if (!strcmp(featurestr, "vendor")) { - pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val); + x86_cpu_add_nv_pair(&props, featurestr, val); } else if (!strcmp(featurestr, "model_id")) { - pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id), - val); + x86_cpu_add_nv_pair(&props, "model-id", val); } else if (!strcmp(featurestr, "tsc_freq")) { int64_t tsc_freq; char *err; + char num[32]; tsc_freq = strtosz_suffix_unit(val, &err, STRTOSZ_DEFSUFFIX_B, 1000); if (tsc_freq < 0 || *err) { - fprintf(stderr, "bad numerical value %s\n", val); - goto error; + error_setg(errp, "bad numerical value %s\n", val); + goto out; } - x86_cpu_def->tsc_khz = tsc_freq / 1000; + snprintf(num, sizeof(num), "%" PRId64, tsc_freq); + x86_cpu_add_nv_pair(&props, "tsc-frequency", num); } else if (!strcmp(featurestr, "hv_spinlocks")) { char *err; numvalue = strtoul(val, &err, 0); if (!*val || *err) { - fprintf(stderr, "bad numerical value %s\n", val); - goto error; + error_setg(errp, "bad numerical value %s\n", val); + goto out; } hyperv_set_spinlock_retries(numvalue); } else { - fprintf(stderr, "unrecognized feature %s\n", featurestr); - goto error; + error_setg(errp, "unrecognized feature %s\n", featurestr); + goto out; } } else if (!strcmp(featurestr, "check")) { check_cpuid = 1; @@ -1405,31 +1403,46 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) } else if (!strcmp(featurestr, "hv_vapic")) { hyperv_enable_vapic_recommended(true); } else { - fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr); - goto error; + error_setg(errp, "feature string `%s' not in format (+feature|" + "-feature|feature=xyz)\n", featurestr); + goto out; } featurestr = strtok(NULL, ","); } - x86_cpu_def->features |= plus_features[FEAT_1_EDX]; - x86_cpu_def->ext_features |= plus_features[FEAT_1_ECX]; - x86_cpu_def->ext2_features |= plus_features[FEAT_8000_0001_EDX]; - x86_cpu_def->ext3_features |= plus_features[FEAT_8000_0001_ECX]; - x86_cpu_def->ext4_features |= plus_features[FEAT_C000_0001_EDX]; - x86_cpu_def->kvm_features |= plus_features[FEAT_KVM]; - x86_cpu_def->svm_features |= plus_features[FEAT_SVM]; - x86_cpu_def->cpuid_7_0_ebx_features |= plus_features[FEAT_7_0_EBX]; - x86_cpu_def->features &= ~minus_features[FEAT_1_EDX]; - x86_cpu_def->ext_features &= ~minus_features[FEAT_1_ECX]; - x86_cpu_def->ext2_features &= ~minus_features[FEAT_8000_0001_EDX]; - x86_cpu_def->ext3_features &= ~minus_features[FEAT_8000_0001_ECX]; - x86_cpu_def->ext4_features &= ~minus_features[FEAT_C000_0001_EDX]; - x86_cpu_def->kvm_features &= ~minus_features[FEAT_KVM]; - x86_cpu_def->svm_features &= ~minus_features[FEAT_SVM]; - x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX]; - return 0; + env->cpuid_features |= plus_features[FEAT_1_EDX]; + env->cpuid_ext_features |= plus_features[FEAT_1_ECX]; + env->cpuid_ext2_features |= plus_features[FEAT_8000_0001_EDX]; + env->cpuid_ext3_features |= plus_features[FEAT_8000_0001_ECX]; + env->cpuid_ext4_features |= plus_features[FEAT_C000_0001_EDX]; + env->cpuid_kvm_features |= plus_features[FEAT_KVM]; + env->cpuid_svm_features |= plus_features[FEAT_SVM]; + env->cpuid_7_0_ebx_features |= plus_features[FEAT_7_0_EBX]; + env->cpuid_features &= ~minus_features[FEAT_1_EDX]; + env->cpuid_ext_features &= ~minus_features[FEAT_1_ECX]; + env->cpuid_ext2_features &= ~minus_features[FEAT_8000_0001_EDX]; + env->cpuid_ext3_features &= ~minus_features[FEAT_8000_0001_ECX]; + env->cpuid_ext4_features &= ~minus_features[FEAT_C000_0001_EDX]; + env->cpuid_kvm_features &= ~minus_features[FEAT_KVM]; + env->cpuid_svm_features &= ~minus_features[FEAT_SVM]; + env->cpuid_7_0_ebx_features &= ~minus_features[FEAT_7_0_EBX]; + + /* Set features on X86CPU object based on a provided key,value list */ + QTAILQ_FOREACH_SAFE(p, &props, next, tmp) { + /* TODO: switch to using global properties after subclasses and + * static properties are done */ + object_property_parse(OBJECT(cpu), p->value, p->name, errp); + if (error_is_set(errp)) { + goto out; + } + } -error: - return -1; +out: + QTAILQ_FOREACH_SAFE(p, &props, next, tmp) { + QTAILQ_REMOVE(&props, p, next); + g_free(p->value); + g_free(p->name); + g_free(p); + } } /* generate a composite string into buf of all cpuid names in featureset @@ -1559,10 +1572,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) def->kvm_features |= kvm_default_features; def->ext_features |= CPUID_EXT_HYPERVISOR; - if (cpu_x86_parse_featurestr(def, features) < 0) { - error_setg(&error, "Invalid cpu_model string format: %s", cpu_model); - goto out; - } assert(def->vendor[0]); object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error); object_property_set_int(OBJECT(cpu), def->level, "level", &error); @@ -1584,6 +1593,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error); + cpu_x86_parse_featurestr(cpu, features, &error); out: g_strfreev(model_pieces); if (error) {
Move custom features parsing after built-in cpu_model defaults are set and set custom features directly on CPU instance. That allows to make clear distinction between built-in cpu model defaults that eventually should go into clas_init() and extra property setting which is done after defaults are set on CPU instance. Impl. details: - features that are already properties, are converted to normalized (name, values) list. And after featurestr has been parsed, properties from the list are applied directly to CPU instance. * For now it provides uniform handling of properties with single object_property_parse() property setter. * And after current features/properties are converted into static properties, it will take a trivial patch to switch to global properties. Which will allow to: * get CPU instance initialized with all parameters passed on -cpu ... cmd. line from object_new() call. * call cpu_model/featurestr parsing only once before CPUs are created * open a road for removing CPUxxxState.cpu_model_str field, when other CPUs are similarly converted to subclasses and static properties. - re-factor error handling, to use Error instead of fprintf()s, since it is anyway passed in for property setter. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- target-i386/cpu.c | 144 ++++++++++++++++++++++++++++------------------------- 1 files changed, 77 insertions(+), 67 deletions(-)