Patchwork [07/23] target-i386: convert cpuid features into properties

login
register
mail settings
Submitter Igor Mammedov
Date Oct. 2, 2012, 3:36 p.m.
Message ID <1349192235-31895-8-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/188571/
State New
Headers show

Comments

Igor Mammedov - Oct. 2, 2012, 3:36 p.m.
add property accessors for cpuid feature bits defined by
*_feature_name arrays.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * replaced mask/ffs tricks by plain 'for (bit = 0; bit < 32; bit++)'
    as suggested by Eduardo Habkost
v3:
  * check if property exists before adding it
  * rebased on top of  "i386: cpu: remove duplicate feature names"
          http://www.mail-archive.com/qemu-devel@nongnu.org/msg129458.html
    place ext2_feature_name for AMD case into setter, so that not to
    clutter x86_cpu_realize() with property specific code.
  * fix for convert cpuid features
v4:
  * Mingw doesn't have strtok_r(), use g_strsplit() instead.
v5:
  * rebase on top of "x86: Implement SMEP and SMAP"
---
 target-i386/cpu.c |  111 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 111 insertions(+), 0 deletions(-)
Eduardo Habkost - Oct. 2, 2012, 8:38 p.m.
(Now replying on the right thread, to keep the discussion in the right
place. I don't know how I ended up replying to a pre-historic version of
the patch, sorry.)

On Tue, Oct 02, 2012 at 05:36:59PM +0200, Igor Mammedov wrote:
[...]
> @@ -1938,6 +2043,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);

Stupid question about qdev:

- qdev_prop_set_globals() is called from device_initfn()
- device_initfn() is called before the child class instance_init()
  function (x86_cpu_initfn())
- So, qdev_prop_set_globals() gets called before the CPU class
  properties are registered.

So this would defeat the whole point of all the work we're doing, that
is to allow compatibility bits to be set as machine-type global
properties. But I don't know what's the right solution here.

Should the qdev_prop_set_globals() call be moved to qdev_init() instead?
Should the CPU properties be registered somewhere else?
Eduardo Habkost - Oct. 3, 2012, 3:03 p.m.
On Tue, Oct 02, 2012 at 05:38:45PM -0300, Eduardo Habkost wrote:
> (Now replying on the right thread, to keep the discussion in the right
> place. I don't know how I ended up replying to a pre-historic version of
> the patch, sorry.)
> 
> On Tue, Oct 02, 2012 at 05:36:59PM +0200, Igor Mammedov wrote:
> [...]
> > @@ -1938,6 +2043,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);
> 
> Stupid question about qdev:
> 
> - qdev_prop_set_globals() is called from device_initfn()
> - device_initfn() is called before the child class instance_init()
>   function (x86_cpu_initfn())
> - So, qdev_prop_set_globals() gets called before the CPU class
>   properties are registered.
> 
> So this would defeat the whole point of all the work we're doing, that
> is to allow compatibility bits to be set as machine-type global
> properties. But I don't know what's the right solution here.
> 
> Should the qdev_prop_set_globals() call be moved to qdev_init() instead?
> Should the CPU properties be registered somewhere else?

Replying to myself: it wouldn't make sense to call it on qdev_init() as
it would overwrite properties set between the calls to object_new() and
qdev_init().

But I still don't see what would be the proper way to solve this. I
don't see any mechanism that would allow us to ensure the
object_property_add() calls are made before qdev_prop_set_globals() is
called.
Paolo Bonzini - Oct. 3, 2012, 3:20 p.m.
Il 03/10/2012 17:03, Eduardo Habkost ha scritto:
> On Tue, Oct 02, 2012 at 05:38:45PM -0300, Eduardo Habkost wrote:
>> (Now replying on the right thread, to keep the discussion in the right
>> place. I don't know how I ended up replying to a pre-historic version of
>> the patch, sorry.)
>>
>> On Tue, Oct 02, 2012 at 05:36:59PM +0200, Igor Mammedov wrote:
>> [...]
>>> @@ -1938,6 +2043,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);
>>
>> Stupid question about qdev:
>>
>> - qdev_prop_set_globals() is called from device_initfn()
>> - device_initfn() is called before the child class instance_init()
>>   function (x86_cpu_initfn())
>> - So, qdev_prop_set_globals() gets called before the CPU class
>>   properties are registered.
>>
>> So this would defeat the whole point of all the work we're doing, that
>> is to allow compatibility bits to be set as machine-type global
>> properties. But I don't know what's the right solution here.
>>
>> Should the qdev_prop_set_globals() call be moved to qdev_init() instead?
>> Should the CPU properties be registered somewhere else?

Properties should be registered (for all objects, not just CPUs) in the
instance_init function.  This is device_initfn.

I would add an instance_postinit function that is called at the end of
object_initialize_with_type, that is after instance_init, and in the
opposite order (i.e. from the leaf to the root).

Paolo

> Replying to myself: it wouldn't make sense to call it on qdev_init() as
> it would overwrite properties set between the calls to object_new() and
> qdev_init().
> 
> But I still don't see what would be the proper way to solve this. I
> don't see any mechanism that would allow us to ensure the
> object_property_add() calls are made before qdev_prop_set_globals() is
> called.
>

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index a58b30d..5604f13 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -844,6 +844,111 @@  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;
+    }
+
+    if (value) {
+        *dst_features |= mask;
+    } else {
+        *dst_features &= ~mask;
+    }
+
+    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
+     * CPUID[1].EDX.
+     */
+    if (dst_features == &env->cpuid_features &&
+            env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
+            env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
+            env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
+        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
+        env->cpuid_ext2_features |= *dst_features & CPUID_EXT2_AMD_ALIASES;
+    }
+}
+
+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;
+            int i;
+
+            feature_name = g_strsplit(featureset[bit], "|", 0);
+            for (i = 0; feature_name[i]; ++i) {
+                if (!object_property_find(obj, feature_name[i], NULL)) {
+                    object_property_add(obj, feature_name[i], "bool",
+                                    x86_cpuid_get_feature,
+                                    x86_cpuid_set_feature, NULL, NULL, NULL);
+                }
+            }
+            g_strfreev(feature_name);
+        }
+    }
+}
+
 static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
                                          const char *name, Error **errp)
 {
@@ -1938,6 +2043,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;