On Mon, 13 Aug 2012 17:48:24 -0300
Eduardo Habkost <ehabk...@redhat.com> wrote:
> On Fri, Aug 10, 2012 at 01:22:34PM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> > ---
> > target-i386/cpu.c | 167
> > +++++++-----------------------------------------------
> > 1 file changed, 20 insertions(+), 147 deletions(-)
> >
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 7555b08..0174c4d 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -206,22 +206,6 @@ static bool lookup_feature(uint32_t *pval, const char
> > *s, const char *e,
> > return found;
> > }
> >
> > -static void add_flagname_to_bitmaps(const char *flagname, uint32_t
> > *features,
> > - uint32_t *ext_features,
> > - uint32_t *ext2_features,
> > - uint32_t *ext3_features,
> > - uint32_t *kvm_features,
> > - uint32_t *svm_features)
> > -{
> > - if (!lookup_feature(features, flagname, NULL, feature_name) &&
> > - !lookup_feature(ext_features, flagname, NULL, ext_feature_name) &&
> > - !lookup_feature(ext2_features, flagname, NULL, ext2_feature_name)
> > &&
> > - !lookup_feature(ext3_features, flagname, NULL, ext3_feature_name)
> > &&
> > - !lookup_feature(kvm_features, flagname, NULL, kvm_feature_name) &&
> > - !lookup_feature(svm_features, flagname, NULL, svm_feature_name))
> > - fprintf(stderr, "CPU feature %s not found\n", flagname);
> > -}
> > -
> > typedef struct x86_def_t {
> > struct x86_def_t *next;
> > const char *name;
> > @@ -1104,7 +1088,6 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t
> > *def, Error **errp)
> > env->cpuid_ext_features = def->ext_features;
> > env->cpuid_ext2_features = def->ext2_features;
> > env->cpuid_ext3_features = def->ext3_features;
> > - env->cpuid_kvm_features = def->kvm_features;
> > env->cpuid_svm_features = def->svm_features;
> > env->cpuid_ext4_features = def->ext4_features;
> > env->cpuid_7_0_ebx = def->cpuid_7_0_ebx_features;
> > @@ -1168,17 +1151,14 @@ static int cpu_x86_find_by_name(X86CPU *cpu,
> > x86_def_t *x86_cpu_def,
> > {
> > 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 = 0, plus_svm_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 numvalue;
> > + QDict *features;
> > + const QDictEntry *ent;
> > + char *name;
> > +
> > + compat_normalize_cpu_model(cpu_model, &name, &features, errp);
> > + if (error_is_set(errp)) {
> > + goto error;
> > + }
> >
> > for (def = x86_defs; def; def = def->next)
> > if (name && !strcmp(name, def->name))
> > @@ -1191,133 +1171,28 @@ static int cpu_x86_find_by_name(X86CPU *cpu,
> > x86_def_t *x86_cpu_def,
> > memcpy(x86_cpu_def, def, sizeof(*def));
> > }
> >
> > - plus_kvm_features = ~0; /* not supported bits will be filtered out
> > later */
> > -
> > - add_flagname_to_bitmaps("hypervisor", &plus_features,
> > - &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
> > - &plus_kvm_features, &plus_svm_features);
> > + cpudef_2_x86_cpu(cpu, def, errp);
>
> It looks like you have to use 'x86_cpu_def' instead of 'def', here, as
> 'def' is the variable for the x86_defs name-lookup loop, only,
> 'x86_cpu_def' is the actual CPU definition.
yep, I'll fix it.
>
>
> >
> > - featurestr = strtok(NULL, ",");
> > + /* not supported bits will be filtered out later */
> > + env->cpuid_kvm_features = ~0;
> >
> > - while (featurestr) {
> > - char *val;
> > - if (featurestr[0] == '+') {
> > - add_flagname_to_bitmaps(featurestr + 1, &plus_features,
> > - &plus_ext_features, &plus_ext2_features,
> > - &plus_ext3_features, &plus_kvm_features,
> > - &plus_svm_features);
> > - } else if (featurestr[0] == '-') {
> > - add_flagname_to_bitmaps(featurestr + 1, &minus_features,
> > - &minus_ext_features, &minus_ext2_features,
> > - &minus_ext3_features, &minus_kvm_features,
> > - &minus_svm_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;
> > - } 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;
> > - } 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 ;
> > - } 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;
> > - } else if (!strcmp(featurestr, "xlevel")) {
> > - char *err;
> > - numvalue = strtoul(val, &err, 0);
> > - if (!*val || *err) {
> > - fprintf(stderr, "bad numerical value %s\n", val);
> > - goto error;
> > - }
> > - x86_cpu_def->xlevel = numvalue;
> > - } else if (!strcmp(featurestr, "vendor")) {
> > - pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor),
> > val);
> > - x86_cpu_def->vendor_override = 1;
> > - } else if (!strcmp(featurestr, "model_id")) {
> > - pstrcpy(x86_cpu_def->model_id,
> > sizeof(x86_cpu_def->model_id),
> > - val);
> > - } else if (!strcmp(featurestr, "tsc_freq")) {
> > - int64_t tsc_freq;
> > - char *err;
> > -
> > - 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;
> > - }
> > - x86_cpu_def->tsc_khz = tsc_freq / 1000;
> > - } 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;
> > - }
> > - hyperv_set_spinlock_retries(numvalue);
> > - } else {
> > - fprintf(stderr, "unrecognized feature %s\n", featurestr);
> > - goto error;
> > - }
> > - } else if (!strcmp(featurestr, "check")) {
> > - check_cpuid = 1;
> > - } else if (!strcmp(featurestr, "enforce")) {
> > - check_cpuid = enforce_cpuid = 1;
> > - } else if (!strcmp(featurestr, "hv_relaxed")) {
> > - hyperv_enable_relaxed_timing(true);
> > - } 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;
> > - }
> > + object_property_set_bool(OBJECT(cpu), true, "hypervisor", errp);
> >
> > + for (ent = qdict_first(features); ent; ent = qdict_next(features,
> > ent)) {
> > + const QString *qval = qobject_to_qstring(qdict_entry_value(ent));
> > + object_property_parse(OBJECT(cpu), qstring_get_str(qval),
> > + qdict_entry_key(ent), errp);
> > if (error_is_set(errp)) {
> > goto error;
> > }
> > -
> > - featurestr = strtok(NULL, ",");
> > }
> > - x86_cpu_def->features |= plus_features;
> > - x86_cpu_def->ext_features |= plus_ext_features;
> > - x86_cpu_def->ext2_features |= plus_ext2_features;
> > - x86_cpu_def->ext3_features |= plus_ext3_features;
> > - x86_cpu_def->kvm_features |= plus_kvm_features;
> > - x86_cpu_def->svm_features |= plus_svm_features;
> > - x86_cpu_def->features &= ~minus_features;
> > - x86_cpu_def->ext_features &= ~minus_ext_features;
> > - x86_cpu_def->ext2_features &= ~minus_ext2_features;
> > - x86_cpu_def->ext3_features &= ~minus_ext3_features;
> > - x86_cpu_def->kvm_features &= ~minus_kvm_features;
> > - x86_cpu_def->svm_features &= ~minus_svm_features;
> > - g_free(s);
> > + QDECREF(features);
> > +
> > + g_free(name);
> > return 0;
> >
> > error:
> > - g_free(s);
> > + g_free(name);
> > if (!error_is_set(errp)) {
> > error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> > }
> > @@ -1427,8 +1302,6 @@ int cpu_x86_register(X86CPU *cpu, const char
> > *cpu_model)
> > if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0)
> > goto out;
> >
> > - cpudef_2_x86_cpu(cpu, def, &error);
> > -
>
> You could move the 'def' and 'def1' variables inside
> cpu_x86_find_by_name(), now, as they are now only used inside that
> function.
it's better stay here, I'll make another patch that will leave only def lookup
in cpu_x86_find_by_name() and move the rest in cpu_x86_register().
with subclasses cpu_x86_find_by_name() should go away and cpu_x86_register()
could be disbanded into pieces as it was shown in [NOT RFC] patch I've posted
recently.
and if you do not object I'd take your "create cpu_x86_set_props() function"
patch from
https://github.com/ehabkost/qemu-hacks/commit/4d734b316625d0b689031460328ce6e531be5b90
>
> > out:
> > if (error_is_set(&error)) {
> > fprintf(stderr, "%s\n", error_get_pretty(error));
> > --
> > 1.7.11.2
> >
> >
>
> --
> Eduardo
--
Regards,
Igor
