On Sat, 26 Jan 2013 16:26:07 +0100
Andreas Färber <afaer...@suse.de> wrote:
> Am 21.01.2013 15:06, schrieb Igor Mammedov:
> > 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:
> > * use object_property_parse() property setter so it would be mechanical
> > change to switch to global properties later.
> > * And after all 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>
> > ---
> > v2:
> > - removed temporary list of properties.
> > Suggested-By: Eduardo Habkost <ehabk...@redhat.com>
> > - check if error is set before cpu_x86_parse_featurestr() is called
> > to avoid assert in error_setg().
> > ---
> > target-i386/cpu.c | 118
> > +++++++++++++++++++++++------------------------------
> > 1 files changed, 51 insertions(+), 67 deletions(-)
> >
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 768ca69..640cc8f 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1299,7 +1299,7 @@ static int cpu_x86_find_by_name(x86_def_t
> > *x86_cpu_def, const char *name)
> >
> > /* 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 */
> > @@ -1307,6 +1307,7 @@ 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;
> >
> > featurestr = features ? strtok(features, ",") : NULL;
> >
> > @@ -1319,77 +1320,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;
> > + object_property_parse(OBJECT(cpu), val, featurestr, errp);
> > } 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;
> > + object_property_parse(OBJECT(cpu), val, featurestr, errp);
> > } 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 ;
> > + object_property_parse(OBJECT(cpu), val, featurestr, errp);
> > } 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;
> > + object_property_parse(OBJECT(cpu), val, featurestr, errp);
> > } 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 removed in future
> > versions\n");
> > numvalue += 0x80000000;
> > }
> > - x86_cpu_def->xlevel = numvalue;
> > + snprintf(num, sizeof(num), "%" PRIu32, numvalue);
> > + object_property_parse(OBJECT(cpu), num, featurestr, errp);
> > } else if (!strcmp(featurestr, "vendor")) {
> > - pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor),
> > val);
> > + object_property_parse(OBJECT(cpu), val, featurestr, errp);
> > } else if (!strcmp(featurestr, "model_id")) {
> > - pstrcpy(x86_cpu_def->model_id,
> > sizeof(x86_cpu_def->model_id),
> > - val);
> > + object_property_parse(OBJECT(cpu), val, "model-id", errp);
> > } 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);
> > + object_property_parse(OBJECT(cpu), num, "tsc-frequency",
> > errp);
> > } 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;
> > @@ -1400,31 +1381,34 @@ 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;
> > + }
> > + if (error_is_set(errp)) {
>
> Note that such error_is_set() is dangerous in that it relies on errp
> being non-NULL for expected behavior. If we want to call this outside of
> cpu_x86_register(), please follow-up with an Error *local_err and
> error_propagate().
Sure, I'll follow up with patch.
>
> > + goto out;
>
> Any reason not to just do "return;" here and above?
we might need cleanup out path, when converting flags features to properties
in case qdict is used as a temporary var to maintain '-foo' behavior.
> Andreas
>
> > }
> > 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];
> >
> > -error:
> > - return -1;
> > +out:
> > + return;
> > }
> >
> > /* generate a composite string into buf of all cpuid names in featureset
> > @@ -1556,10 +1540,6 @@ int cpu_x86_register(X86CPU *cpu, const char
> > *cpu_model)
> > }
> > 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;
> > - }
> > object_property_set_str(OBJECT(cpu), def->vendor, "vendor", &error);
> > object_property_set_int(OBJECT(cpu), def->level, "level", &error);
> > object_property_set_int(OBJECT(cpu), def->family, "family", &error);
> > @@ -1579,7 +1559,11 @@ int cpu_x86_register(X86CPU *cpu, const char
> > *cpu_model)
> > "tsc-frequency", &error);
> >
> > object_property_set_str(OBJECT(cpu), def->model_id, "model-id",
> > &error);
> > + if (error) {
> > + goto out;
> > + }
> >
> > + cpu_x86_parse_featurestr(cpu, features, &error);
> > out:
> > g_strfreev(model_pieces);
> > if (error) {
> >
>
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
--
Regards,
Igor
