From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40456) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T3QuO-0001ya-J6 for qemu-devel@nongnu.org; Mon, 20 Aug 2012 08:16:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T3QuN-0004D2-4L for qemu-devel@nongnu.org; Mon, 20 Aug 2012 08:16:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56680) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T3QuM-0004Cp-RU for qemu-devel@nongnu.org; Mon, 20 Aug 2012 08:16:39 -0400 Date: Mon, 20 Aug 2012 14:16:26 +0200 From: Igor Mammedov Message-ID: <20120820141626.183ee13a@thinkpad.mammed.net> In-Reply-To: <20120816181050.GC17695@otherpad.lan.raisama.net> References: <1345047221-26898-1-git-send-email-imammedo@redhat.com> <1345047221-26898-8-git-send-email-imammedo@redhat.com> <20120816181050.GC17695@otherpad.lan.raisama.net> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 07/21] target-i386: convert cpuid features into properties List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: aliguori@us.ibm.com, stefanha@linux.vnet.ibm.com, gleb@redhat.com, vijaymohan.pandarathil@hp.com, jan.kiszka@siemens.com, mtosatti@redhat.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com, blauwirbel@gmail.com, avi@redhat.com, pbonzini@redhat.com, akong@redhat.com, lersek@redhat.com, afaerber@suse.de On Thu, 16 Aug 2012 15:10:50 -0300 Eduardo Habkost wrote: > On Wed, Aug 15, 2012 at 06:13:27PM +0200, Igor Mammedov wrote: > > Signed-off-by: Igor Mammedov > > -- > > v2: > > * replaced mask/ffs tricks by plain 'for (bit = 0; bit < 32; bit++)' > > as suggested by Eduardo Habkost > > --- > > target-i386/cpu.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 101 insertions(+) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 37ba5ef..440e724 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -606,6 +606,101 @@ static int check_features_against_host(x86_def_t *guest_def) > > return rv; > > } > > > > +static bool is_feature_set(const char *name, const uint32_t featbitmap, > > + const char **featureset) > > +{ > > + uint32_t bit; > > + > > + for (bit = 0; bit < 32; ++bit) { > > + if (featureset[bit] && !altcmp(name, NULL, featureset[bit])) { > > + if (featbitmap & (1 << bit)) { > > + return true; > > + } > > + } > > + } > > + return false; > > +} > > + > > +static void x86_cpuid_get_feature(Object *obj, Visitor *v, void *opaque, > > + const char *name, Error **errp) > > +{ > > + X86CPU *cpu = X86_CPU(obj); > > + CPUX86State *env = &cpu->env; > > + bool value = true; > > + > > + if (!is_feature_set(name, env->cpuid_features, feature_name) && > > + !is_feature_set(name, env->cpuid_ext_features, ext_feature_name) && > > + !is_feature_set(name, env->cpuid_ext2_features, ext2_feature_name) && > > + !is_feature_set(name, env->cpuid_ext3_features, ext3_feature_name) && > > + !is_feature_set(name, env->cpuid_kvm_features, kvm_feature_name) && > > + !is_feature_set(name, env->cpuid_svm_features, svm_feature_name)) { > > + value = false; > > + } > > + > > + visit_type_bool(v, &value, name, errp); > > +} > > + > > +static void x86_cpuid_set_feature(Object *obj, Visitor *v, void *opaque, > > + const char *name, Error **errp) > > +{ > > + X86CPU *cpu = X86_CPU(obj); > > + CPUX86State *env = &cpu->env; > > + uint32_t mask = 0; > > + uint32_t *dst_features; > > + bool value; > > + > > + visit_type_bool(v, &value, name, errp); > > + if (error_is_set(errp)) { > > + return; > > + } > > + > > + if (lookup_feature(&mask, name, NULL, feature_name)) { > > + dst_features = &env->cpuid_features; > > + } else if (lookup_feature(&mask, name, NULL, ext_feature_name)) { > > + dst_features = &env->cpuid_ext_features; > > + } else if (lookup_feature(&mask, name, NULL, ext2_feature_name)) { > > + dst_features = &env->cpuid_ext2_features; > > + } else if (lookup_feature(&mask, name, NULL, ext3_feature_name)) { > > + dst_features = &env->cpuid_ext3_features; > > + } else if (lookup_feature(&mask, name, NULL, kvm_feature_name)) { > > + dst_features = &env->cpuid_kvm_features; > > + } else if (lookup_feature(&mask, name, NULL, svm_feature_name)) { > > + dst_features = &env->cpuid_svm_features; > > + } else { > > + error_set(errp, QERR_PROPERTY_NOT_FOUND, "", name); > > + return; > > + } > > Some feature names are duplicated on feature_names and > ext_feature_names. On AMD CPU models, we have to set both, on Intel > models we need to set the bits only on cpuid_features. > > Maybe it's better to: > > 1) eliminate the duplication and set the names only on feature_name > array; > 2) At the end of CPU initialization, set the features on > cpuid_ext2_features as copies of the corresponding cpuid_features > bits if CPU vendor == AMD (or, maybe, if some boolean > "ext2_features_aliases" flag is set, to make it not > vendor-dependent). So far I was trying to keep original behavior and keep scope of this series only on moving CPU features into properties. I'll rebase this series on top of what you proposed in a week when I'm back from vacation. > > > + > > + if (value) { > > + *dst_features |= mask; > > + } else { > > + *dst_features &= ~mask; > > + } > > +} > > + > > +static void x86_register_cpuid_properties(Object *obj, const char **featureset) > > +{ > > + uint32_t bit; > > + > > + for (bit = 0; bit < 32; ++bit) { > > + if (featureset[bit]) { > > + char *feature_name, *save_ptr; > > + char buf[32]; > > + if (strlen(featureset[bit]) > sizeof(buf) - 1) { > > + abort(); > > + } > > + pstrcpy(buf, sizeof(buf), featureset[bit]); > > + feature_name = strtok_r(buf, "|", &save_ptr); > > + while (feature_name) { > > + object_property_add(obj, feature_name, "bool", > > + x86_cpuid_get_feature, > > + x86_cpuid_set_feature, NULL, NULL, NULL); > > + feature_name = strtok_r(NULL, "|", &save_ptr); > > What happens when object_property_add() is called twice with the same > feature name? Just extra item in list (pure waste). Thanks for pointing out. I've fixed it (not yet pushed) by checking if property already exists before adding it. > > > + } > > + } > > + } > > +} > > + > > static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque, > > const char *name, Error **errp) > > { > > @@ -1824,6 +1919,12 @@ static void x86_cpu_initfn(Object *obj) > > object_property_add(obj, "tsc-frequency", "int", > > x86_cpuid_get_tsc_freq, > > x86_cpuid_set_tsc_freq, NULL, NULL, NULL); > > + x86_register_cpuid_properties(obj, feature_name); > > + x86_register_cpuid_properties(obj, ext_feature_name); > > + x86_register_cpuid_properties(obj, ext2_feature_name); > > + x86_register_cpuid_properties(obj, ext3_feature_name); > > + x86_register_cpuid_properties(obj, kvm_feature_name); > > + x86_register_cpuid_properties(obj, svm_feature_name); > > > > env->cpuid_apic_id = env->cpu_index; > > > > -- > > 1.7.11.2 > > > > > > -- > Eduardo > -- Regards, Igor