From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47598) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQgQU-0000ls-Qu for qemu-devel@nongnu.org; Tue, 23 Oct 2012 11:29:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TQgQO-0006DZ-E1 for qemu-devel@nongnu.org; Tue, 23 Oct 2012 11:29:54 -0400 Received: from hub021-nj-6.exch021.serverdata.net ([206.225.164.222]:52344) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQgQO-0006D4-9b for qemu-devel@nongnu.org; Tue, 23 Oct 2012 11:29:48 -0400 Message-ID: <5086B7EA.4000500@CloudSwitch.Com> Date: Tue, 23 Oct 2012 11:29:46 -0400 From: Don Slutz MIME-Version: 1.0 References: <1350918203-25198-1-git-send-email-imammedo@redhat.com> <1350918203-25198-33-git-send-email-imammedo@redhat.com> In-Reply-To: <1350918203-25198-33-git-send-email-imammedo@redhat.com> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 32/37] target-i386: use static properties for setting cpuid features List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: aliguori@us.ibm.com, ehabkost@redhat.com, jan.kiszka@siemens.com, mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org, blauwirbel@gmail.com, stefanha@redhat.com, pbonzini@redhat.com, afaerber@suse.de On 10/22/12 11:03, Igor Mammedov wrote: > - def->kvm_features was used only for setting hadcoded defaults, > with defaults set by static props, we do not need it anymore, > ignore it in cpudef_2_x86_cpu(). > - take in account CPUID_EXT_HYPERVISOR feature set by static > property default value; > > Signed-off-by: Igor Mammedov > --- > * fix double free of features in cpu_x86_find_by_name() > --- > target-i386/cpu.c | 271 +++++++----------------------------------------------- > 1 file changed, 31 insertions(+), 240 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index c804965..59809f8 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -87,31 +87,6 @@ static const char *ext3_feature_name[] = { > NULL, NULL, NULL, NULL, > }; > > -static const char *kvm_feature_name[] = { > - "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock", "kvm_asyncpf", NULL, "kvm_pv_eoi", NULL, > - NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, > - NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, > - NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, > -}; > - > -static const char *svm_feature_name[] = { > - "npt", "lbrv", "svm_lock", "nrip_save", > - "tsc_scale", "vmcb_clean", "flushbyasid", "decodeassists", > - NULL, NULL, "pause_filter", NULL, > - "pfthreshold", NULL, NULL, NULL, > - NULL, NULL, NULL, NULL, > - NULL, NULL, NULL, NULL, > - NULL, NULL, NULL, NULL, > - NULL, NULL, NULL, NULL, > -}; > - > -static const char *cpuid_7_0_ebx_feature_name[] = { > - NULL, NULL, NULL, NULL, NULL, NULL, NULL, "smep", > - NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, > - NULL, NULL, NULL, NULL, "smap", NULL, NULL, NULL, > - NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, > -}; > - > #if defined(CONFIG_KVM) > static void x86_cpu_get_kvmclock(Object *obj, Visitor *v, void *opaque, > const char *name, Error **errp) > @@ -752,88 +727,6 @@ void host_cpuid(uint32_t function, uint32_t count, > #endif > } > > -#define iswhite(c) ((c) && ((c) <= ' ' || '~' < (c))) > - > -/* general substring compare of *[s1..e1) and *[s2..e2). sx is start of > - * a substring. ex if !NULL points to the first char after a substring, > - * otherwise the string is assumed to sized by a terminating nul. > - * Return lexical ordering of *s1:*s2. > - */ > -static int sstrcmp(const char *s1, const char *e1, const char *s2, > - const char *e2) > -{ > - for (;;) { > - if (!*s1 || !*s2 || *s1 != *s2) > - return (*s1 - *s2); > - ++s1, ++s2; > - if (s1 == e1 && s2 == e2) > - return (0); > - else if (s1 == e1) > - return (*s2); > - else if (s2 == e2) > - return (*s1); > - } > -} > - > -/* compare *[s..e) to *altstr. *altstr may be a simple string or multiple > - * '|' delimited (possibly empty) strings in which case search for a match > - * within the alternatives proceeds left to right. Return 0 for success, > - * non-zero otherwise. > - */ > -static int altcmp(const char *s, const char *e, const char *altstr) > -{ > - const char *p, *q; > - > - for (q = p = altstr; ; ) { > - while (*p && *p != '|') > - ++p; > - if ((q == p && !*s) || (q != p && !sstrcmp(s, e, q, p))) > - return (0); > - if (!*p) > - return (1); > - else > - q = ++p; > - } > -} > - > -/* search featureset for flag *[s..e), if found set corresponding bit in > - * *pval and return true, otherwise return false > - */ > -static bool lookup_feature(uint32_t *pval, const char *s, const char *e, > - const char **featureset) > -{ > - uint32_t mask; > - const char **ppc; > - bool found = false; > - > - for (mask = 1, ppc = featureset; mask; mask <<= 1, ++ppc) { > - if (*ppc && !altcmp(s, e, *ppc)) { > - *pval |= mask; > - found = true; > - } > - } > - 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, > - uint32_t *cpuid_7_0_ebx_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) && > - !lookup_feature(cpuid_7_0_ebx_features, flagname, NULL, > - cpuid_7_0_ebx_feature_name)) > - fprintf(stderr, "CPU feature %s not found\n", flagname); > -} > - > typedef struct x86_def_t { > struct x86_def_t *next; > const char *name; > @@ -1432,10 +1325,10 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp) > object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000, > "tsc-frequency", errp); > env->cpuid_features = def->features; > - env->cpuid_ext_features = def->ext_features; > + env->cpuid_ext_features = (CPUID_EXT_HYPERVISOR & 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_features = def->cpuid_7_0_ebx_features; > @@ -1532,25 +1425,32 @@ out: > return; > } > > +/* Set features on X86CPU object based on a QDict */ > +static void cpu_x86_set_props(X86CPU *cpu, QDict *features, Error **errp) > +{ > + const QDictEntry *ent; > + > + 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)) { > + return; > + } > + } > +} > + > static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def, > const char *cpu_model, Error **errp) > { > x86_def_t *def; > - CPUX86State *env = &cpu->env; > + QDict *features = NULL; > + char *name = NULL; > > - char *s = g_strdup(cpu_model); > - char *featurestr, *name = strtok(s, ","); > - /* Features to be added*/ > - uint32_t plus_features = 0, plus_ext_features = env->cpuid_ext_features; > - uint32_t plus_ext2_features = 0, plus_ext3_features = 0; > - uint32_t plus_kvm_features = env->cpuid_kvm_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; > + 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)) > @@ -1563,127 +1463,18 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def, > memcpy(x86_cpu_def, def, sizeof(*def)); > } > > - featurestr = strtok(NULL, ","); > - > - 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, &plus_7_0_ebx_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, &minus_7_0_ebx_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; > - } > - if (numvalue < 0x80000000) { > - numvalue += 0x80000000; > - } > - 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")) { This makes model_id= as a feature stop working. It needs to be added to this patch, or maybe #10 where "tsc_freq" was handled. > - 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; > - } > - featurestr = strtok(NULL, ","); > + cpu_x86_set_props(cpu, features, errp); > + if (error_is_set(errp)) { > + goto error; > } > - 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->cpuid_7_0_ebx_features |= plus_7_0_ebx_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; > - x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features; > - g_free(s); > + > + g_free(name); > + QDECREF(features); > return 0; > > error: > - g_free(s); > + g_free(name); > + QDECREF(features); > if (!error_is_set(errp)) { > error_set(errp, QERR_INVALID_PARAMETER_COMBINATION); > } -Don Slutz