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 <imamm...@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
>
--
Eduardo
