[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 07/21] target-i386: convert cpuid features into
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH 07/21] target-i386: convert cpuid features into properties |
Date: |
Mon, 20 Aug 2012 09:34:12 -0300 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Mon, Aug 20, 2012 at 02:16:26PM +0200, Igor Mammedov wrote:
> On Thu, 16 Aug 2012 15:10:50 -0300
> Eduardo Habkost <address@hidden> wrote:
>
> > On Wed, Aug 15, 2012 at 06:13:27PM +0200, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <address@hidden>
> > > --
> > > 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.
Sure, keeping the behavior while changing the code to use properties
makes perfect sense. The only problem is that the current behavior is
incorrect and makes the property handling more complicated, so it makes
sense to fix it first.
>
> >
> > > +
> > > + 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
--
Eduardo
- [Qemu-devel] [PATCH 03/21] target-i386: if x86_cpu_realize() failed report error and do cleanup, (continued)
- [Qemu-devel] [PATCH 03/21] target-i386: if x86_cpu_realize() failed report error and do cleanup, Igor Mammedov, 2012/08/15
- [Qemu-devel] [PATCH 06/21] target-i386: xlevel should be more than 0x80000000, move fixup into setter, Igor Mammedov, 2012/08/15
- [Qemu-devel] [PATCH 04/21] target-i386: filter out not TCG features if running without kvm at realize time, Igor Mammedov, 2012/08/15
- [Qemu-devel] [PATCH 08/21] target-i386: convert 'hv_spinlocks' feature into property, Igor Mammedov, 2012/08/15
- [Qemu-devel] [PATCH 07/21] target-i386: convert cpuid features into properties, Igor Mammedov, 2012/08/15
- [Qemu-devel] [PATCH 10/21] target-i386: convert 'hv_vapic' feature into property, Igor Mammedov, 2012/08/15
- [Qemu-devel] [PATCH 11/21] target-i386: convert 'check' and 'enforce' features into properties, Igor Mammedov, 2012/08/15
- [Qemu-devel] [PATCH 02/21] target-i386: cpu_x86_register(): report error from property setter, Igor Mammedov, 2012/08/15
- [Qemu-devel] [PATCH 14/21] target-i386: introduce vendor-override property, Igor Mammedov, 2012/08/15
- [Qemu-devel] [PATCH 15/21] target-i386: use define for cpuid vendor string size, Igor Mammedov, 2012/08/15
- [Qemu-devel] [PATCH 13/21] target-i386: use visit_type_hz to parse tsc_freq property value, Igor Mammedov, 2012/08/15
- [Qemu-devel] [PATCH 05/21] target-i386: move out CPU features initialization in separate func, Igor Mammedov, 2012/08/15
- [Qemu-devel] [PATCH 16/21] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t, Igor Mammedov, 2012/08/15
- [Qemu-devel] [PATCH 19/21] target-i386: move init of "hypervisor" feature into CPU initializer from cpudef, Igor Mammedov, 2012/08/15
- [Qemu-devel] [PATCH 20/21] target-i386: move default init of cpuid_kvm_features bitmap into CPU initializer from cpudef, Igor Mammedov, 2012/08/15