Patchwork [1/9] target-i386: cpu: convert existing dynamic properties into static properties

login
register
mail settings
Submitter Eduardo Habkost
Date Feb. 18, 2013, 9:27 p.m.
Message ID <20130218212716.GT8494@otherpad.lan.raisama.net>
Download mbox | patch
Permalink /patch/221499/
State New
Headers show

Comments

Eduardo Habkost - Feb. 18, 2013, 9:27 p.m.
On Mon, Feb 18, 2013 at 09:36:28PM +0100, Andreas Färber wrote:
> Am 18.02.2013 21:30, schrieb Eduardo Habkost:
> > On Mon, Feb 11, 2013 at 05:35:03PM +0100, Igor Mammedov wrote:
> >> Following properties are converted:
> >>     * vendor
> >>     * xlevel
> >>         * custom setter/getter replaced by qdev's DEFINE_PROP_UINT32
> >>     * level
> >>         * custom setter/getter replaced by qdev's DEFINE_PROP_UINT32
> >>     * tsc-frequency
> >>     * stepping
> >>     * model
> >>     * family
> >>     * model-id
> >>         * check "if (model_id == NULL)" looks unnecessary now, since all
> >>         builtin model-ids are not NULL and user shouldn't be able to set
> >>         it NULL (cpumodel string parsing code takes care of it, if feature
> >>         is specified as "model-id=" on command line, its parsing will
> >>         result in an empty string as value).
> >>         * use g_malloc0() instead of g_malloc() in x86_cpuid_get_model_id()
> >>
> >> Common changes to all properties:
> >>     * properties code are moved to the top of file, before properties array
> >>       definition
> >>     * s/error_set/error_setg/;s/QERR*/with similar message/
> > 
> > Why?
> 
> Been having a similar question (sorry for not replying yet):
> 
> Can't we leave my new-style getters and setters in place and invoke them
> through some glue code from whatever old-style machinery qdev static
> properties still use?

I don't understand your question. What do you mean by "my new-style
getters and setters"?

Igor's patch keeps the existing getters/setters almost the same (I
manually run "diff" to check that, see the end of this message), except
for the error_set() calls, the small differences I pointed out in my
message, and extra visit_type_str() calls required for the string
getter/setters (that have a different signature from
object_property_add_str() getter/setters).


> 
> BTW I'm not yet clear on how we should proceed with subclasses and KVM.
> Proposing we proceed with your properties refactoring after all, then
> maybe it becomes more clear where the problems are.

I believe Igor will send a new version soon. I completely agree with
having KVM-specific and TCG-specific classes, because they _are_
different CPU models. The same reasons we have[1] to make "-cpu
SandyBridge" and "-cpu Haswell" different classes, apply to making
"-disable-kvm -cpu SandyBridge" and "-enable-kvm -cpu SandyBridge"
different classes as well.

About the default for the "vendor" property: right now it is not an
issue because Igor's patches are not setting any externally-visible
default value for the "vendor" property (so this is just an
implementation detail), and now I am happy with either of the solutions
we have been discussing (as my questions about QOM design have been
answered). In either case, when we actually get to set a introspectable
default for the property, I hope to have heard additional feedback from
other people (especially from the libvirt folks).


[1] The reasons I remember are:

1) Allowing CPU model definition probing, by simply querying the default
   values of properties for each class;
   * Rationale for separating TCG/KVM: the resulting CPU features are
     different when running KVM and when running TCG, so they are in
     practice different CPU models. For example: differences between
     "-enable-kvm -cpu SandyBridge" and "-disable-kvm -cpu SandyBridge"
     are huge when compared to the small differences between
     "-enable-kvm -cpu SandyBridge" and "-enable-kvm -cpu Haswell".

2) Allowing machine-type compatibility to be specified easily using
   global properties on the machine-type compat_props field.
   * Rationale for separating TCG/KVM: we may want to set a
     compatibility property only for TCG or only for KVM. For example,
     the n270 MOVBE fix we are going to include QEMU in 1.5 will need to
     disable MOVBE on pc-1.4, but only on the TCG CPU model, because KVM
     already had MOVBE working on pc-1.4.

Patch

--- /tmp/oldsetters	2013-02-18 17:12:53.781477861 -0300
+++ /tmp/newsetters	2013-02-18 17:12:59.069477982 -0300
@@ -26,8 +26,9 @@ 
         return;
     }
     if (value < min || value > max) {
-        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
-                  name ? name : "null", value, min, max);
+        error_setg(errp, "Property %s.%s doesn't take value %" PRId64 " (min"
+                  "imum: %" PRId64 ", maximum: %" PRId64,
+                  object_get_typename(obj), name, value, min, max);
         return;
     }
 
@@ -39,6 +40,14 @@ 
     }
 }
 
+PropertyInfo qdev_prop_family = {
+    .name  = "uint32",
+    .get   = x86_cpuid_version_get_family,
+    .set   = x86_cpuid_version_set_family,
+};
+#define DEFINE_PROP_FAMILY(_n)                                                 \
+    DEFINE_PROP(_n, X86CPU, env.cpuid_version, qdev_prop_family, uint32_t)
+
 static void x86_cpuid_version_get_model(Object *obj, Visitor *v, void *opaque,
                                         const char *name, Error **errp)
 {
@@ -65,8 +74,9 @@ 
         return;
     }
     if (value < min || value > max) {
-        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
-                  name ? name : "null", value, min, max);
+        error_setg(errp, "Property %s.%s doesn't take value %" PRId64 " (min"
+                  "imum: %" PRId64 ", maximum: %" PRId64,
+                  object_get_typename(obj), name, value, min, max);
         return;
     }
 
@@ -74,6 +84,14 @@ 
     env->cpuid_version |= ((value & 0xf) << 4) | ((value >> 4) << 16);
 }
 
+PropertyInfo qdev_prop_model = {
+    .name  = "uint32",
+    .get   = x86_cpuid_version_get_model,
+    .set   = x86_cpuid_version_set_model,
+};
+#define DEFINE_PROP_MODEL(_n)                                                  \
+    DEFINE_PROP(_n, X86CPU, env.cpuid_version, qdev_prop_model, uint32_t)
+
 static void x86_cpuid_version_get_stepping(Object *obj, Visitor *v,
                                            void *opaque, const char *name,
                                            Error **errp)
@@ -101,8 +119,9 @@ 
         return;
     }
     if (value < min || value > max) {
-        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
-                  name ? name : "null", value, min, max);
+        error_setg(errp, "Property %s.%s doesn't take value %" PRId64 " (min"
+                  "imum: %" PRId64 ", maximum: %" PRId64,
+                  object_get_typename(obj), name, value, min, max);
         return;
     }
 
@@ -110,39 +129,16 @@ 
     env->cpuid_version |= value & 0xf;
 }
 
-static void x86_cpuid_get_level(Object *obj, Visitor *v, void *opaque,
-                                const char *name, Error **errp)
-{
-    X86CPU *cpu = X86_CPU(obj);
-
-    visit_type_uint32(v, &cpu->env.cpuid_level, name, errp);
-}
+PropertyInfo qdev_prop_stepping = {
+    .name  = "uint32",
+    .get   = x86_cpuid_version_get_stepping,
+    .set   = x86_cpuid_version_set_stepping,
+};
+#define DEFINE_PROP_STEPPING(_n)                                               \
+    DEFINE_PROP(_n, X86CPU, env.cpuid_version, qdev_prop_stepping, uint32_t)
 
-static void x86_cpuid_set_level(Object *obj, Visitor *v, void *opaque,
-                                const char *name, Error **errp)
-{
-    X86CPU *cpu = X86_CPU(obj);
-
-    visit_type_uint32(v, &cpu->env.cpuid_level, name, errp);
-}
-
-static void x86_cpuid_get_xlevel(Object *obj, Visitor *v, void *opaque,
-                                 const char *name, Error **errp)
-{
-    X86CPU *cpu = X86_CPU(obj);
-
-    visit_type_uint32(v, &cpu->env.cpuid_xlevel, name, errp);
-}
-
-static void x86_cpuid_set_xlevel(Object *obj, Visitor *v, void *opaque,
-                                 const char *name, Error **errp)
-{
-    X86CPU *cpu = X86_CPU(obj);
-
-    visit_type_uint32(v, &cpu->env.cpuid_xlevel, name, errp);
-}
-
-static char *x86_cpuid_get_vendor(Object *obj, Error **errp)
+static void x86_cpuid_get_vendor(Object *obj, Visitor *v, void *opaque,
+                       const char *name, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
@@ -151,19 +147,27 @@ 
     value = (char *)g_malloc(CPUID_VENDOR_SZ + 1);
     x86_cpu_vendor_words2str(value, env->cpuid_vendor1, env->cpuid_vendor2,
                              env->cpuid_vendor3);
-    return value;
+    visit_type_str(v, &value, name, errp);
+    g_free(value);
 }
 
-static void x86_cpuid_set_vendor(Object *obj, const char *value,
-                                 Error **errp)
+static void x86_cpuid_set_vendor(Object *obj, Visitor *v, void *opaque,
+                       const char *name, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
+    char *value;
     int i;
 
+    visit_type_str(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+
     if (strlen(value) != CPUID_VENDOR_SZ) {
-        error_set(errp, QERR_PROPERTY_VALUE_BAD, "",
-                  "vendor", value);
+        error_setg(errp, "Property '%s.%s' doesn't take value '%s'",
+                   object_get_typename(obj), name, value);
+        g_free(value);
         return;
     }
 
@@ -171,47 +175,73 @@ 
     env->cpuid_vendor2 = 0;
     env->cpuid_vendor3 = 0;
     for (i = 0; i < 4; i++) {
-        env->cpuid_vendor1 |= ((uint8_t)value[i    ]) << (8 * i);
+        env->cpuid_vendor1 |= ((uint8_t)value[i])     << (8 * i);
         env->cpuid_vendor2 |= ((uint8_t)value[i + 4]) << (8 * i);
         env->cpuid_vendor3 |= ((uint8_t)value[i + 8]) << (8 * i);
     }
+    g_free(value);
+}
+
+PropertyInfo qdev_prop_vendor = {
+    .name  = "string",
+    .get   = x86_cpuid_get_vendor,
+    .set   = x86_cpuid_set_vendor,
+};
+#define DEFINE_PROP_VENDOR(_n) {                                               \
+    .name = _n,                                                                \
+    .info  = &qdev_prop_vendor                                                 \
 }
 
-static char *x86_cpuid_get_model_id(Object *obj, Error **errp)
+static void x86_cpuid_get_model_id(Object *obj, Visitor *v, void *opaque,
+                                   const char *name, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
     char *value;
     int i;
 
-    value = g_malloc(48 + 1);
+    value = g_malloc0(48 + 1);
     for (i = 0; i < 48; i++) {
         value[i] = env->cpuid_model[i >> 2] >> (8 * (i & 3));
     }
-    value[48] = '\0';
-    return value;
+    visit_type_str(v, &value, name, errp);
+    g_free(value);
 }
 
-static void x86_cpuid_set_model_id(Object *obj, const char *model_id,
-                                   Error **errp)
+static void x86_cpuid_set_model_id(Object *obj, Visitor *v, void *opaque,
+                                   const char *name, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
     int c, len, i;
+    char *value;
 
-    if (model_id == NULL) {
-        model_id = "";
+    visit_type_str(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
     }
-    len = strlen(model_id);
+
+    len = strlen(value);
     memset(env->cpuid_model, 0, 48);
     for (i = 0; i < 48; i++) {
         if (i >= len) {
             c = '\0';
         } else {
-            c = (uint8_t)model_id[i];
+            c = (uint8_t)value[i];
         }
         env->cpuid_model[i >> 2] |= c << (8 * (i & 3));
     }
+    g_free(value);
+}
+
+PropertyInfo qdev_prop_model_id = {
+    .name  = "string",
+    .get   = x86_cpuid_get_model_id,
+    .set   = x86_cpuid_set_model_id,
+};
+#define DEFINE_PROP_MODEL_ID(_n) {                                             \
+    .name  = _n,                                                               \
+    .info  = &qdev_prop_model_id                                               \
 }
 
 static void x86_cpuid_get_tsc_freq(Object *obj, Visitor *v, void *opaque,
@@ -237,11 +267,20 @@ 
         return;
     }
     if (value < min || value > max) {
-        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
-                  name ? name : "null", value, min, max);
+        error_setg(errp, "Property %s.%s doesn't take value %" PRId64 " (min"
+                  "imum: %" PRId64 ", maximum: %" PRId64,
+                  object_get_typename(obj), name, value, min, max);
         return;
     }
 
     cpu->env.tsc_khz = value / 1000;
 }
 
+PropertyInfo qdev_prop_tsc_freq = {
+    .name  = "int64",
+    .get   = x86_cpuid_get_tsc_freq,
+    .set   = x86_cpuid_set_tsc_freq,
+};
+#define DEFINE_PROP_TSC_FREQ(_n)                                               \
+    DEFINE_PROP(_n, X86CPU, env.tsc_khz, qdev_prop_tsc_freq, int32_t)
+