[Qemu-devel] [PATCH qom-cpu for-1.5 0/4] target-i386: X86CPU compatibility properties

[Qemu-devel] [PATCH qom-cpu for-1.5 0/4] target-i386: X86CPU compatibility properties

[Andreas Färber]

Hello,

It's easier adapting the infrastructure to our needs than working around it:
X86CPU already has QOM properties today. What's lacking is model subclasses,
and with the one X86CPU type its global properties are overwritten by models.
But we already know the designated naming scheme for the models!

So let's simply prepare compat_props for CPU models and make sure they are
already picked up today.

This works just fine for changing the 486 CPUID model value and avoids to
redo the PC part once we have X86CPU subclasses.
Tested using: ./QMP/qom-get /machine/icc-bridge/icc/child[0].model

For changing n270 CPUID flags we'll still need to resort to Eduardo's proposed
helper functions for now.

Regards,
Andreas

Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Anthony Liguori <anthony@codemonkey.ws>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>

Andreas Färber (4):
  qdev: Let qdev_prop_parse() pass through Error
  qdev: Introduce qdev_prop_set_custom_globals()
  target-i386: Emulate X86CPU subclasses for global properties
  target-i386: Change CPUID model of 486 to 8

 hw/core/qdev-properties.c    | 50 ++++++++++++++++++++++++++------------------
 hw/core/qdev.c               |  7 ++++++-
 include/hw/i386/pc.h         |  4 ++++
 include/hw/qdev-properties.h |  7 +++++--
 qdev-monitor.c               |  6 +++++-
 target-i386/cpu.c            | 11 +++++++++-
 6 files changed, 60 insertions(+), 25 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH qom-cpu for-1.5 1/4] qdev: Let qdev_prop_parse() pass through Error

[Andreas Färber]

Move error reporting to callers.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/core/qdev-properties.c    | 25 +++++++++++--------------
 hw/core/qdev.c               |  7 ++++++-
 include/hw/qdev-properties.h |  5 +++--
 qdev-monitor.c               |  6 +++++-
 4 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index ca1739e..716ba19 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -986,25 +986,18 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
     }
 }
 
-int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
+void qdev_prop_parse(DeviceState *dev, const char *name, const char *value,
+                     Error **errp)
 {
     char *legacy_name;
-    Error *err = NULL;
 
     legacy_name = g_strdup_printf("legacy-%s", name);
     if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) {
-        object_property_parse(OBJECT(dev), value, legacy_name, &err);
+        object_property_parse(OBJECT(dev), value, legacy_name, errp);
     } else {
-        object_property_parse(OBJECT(dev), value, name, &err);
+        object_property_parse(OBJECT(dev), value, name, errp);
     }
     g_free(legacy_name);
-
-    if (err) {
-        qerror_report_err(err);
-        error_free(err);
-        return -1;
-    }
-    return 0;
 }
 
 void qdev_prop_set_bit(DeviceState *dev, const char *name, bool value)
@@ -1106,18 +1099,22 @@ void qdev_prop_register_global_list(GlobalProperty *props)
     }
 }
 
-void qdev_prop_set_globals(DeviceState *dev)
+void qdev_prop_set_globals(DeviceState *dev, Error **errp)
 {
     ObjectClass *class = object_get_class(OBJECT(dev));
 
     do {
         GlobalProperty *prop;
         QTAILQ_FOREACH(prop, &global_props, next) {
+            Error *err = NULL;
+
             if (strcmp(object_class_get_name(class), prop->driver) != 0) {
                 continue;
             }
-            if (qdev_prop_parse(dev, prop->property, prop->value) != 0) {
-                exit(1);
+            qdev_prop_parse(dev, prop->property, prop->value, &err);
+            if (err != NULL) {
+                error_propagate(errp, err);
+                return;
             }
         }
         class = object_class_get_parent(class);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index ab1d8f5..7167c90 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -752,7 +752,12 @@ static void device_initfn(Object *obj)
         }
         class = object_class_get_parent(class);
     } while (class != object_class_by_name(TYPE_DEVICE));
-    qdev_prop_set_globals(dev);
+    qdev_prop_set_globals(dev, &err);
+    if (err != NULL) {
+        qerror_report_err(err);
+        error_free(err);
+        exit(1);
+    }
 
     object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
                              (Object **)&dev->parent_bus, &err);
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 25dd1bb..38469d4 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -148,7 +148,8 @@ extern PropertyInfo qdev_prop_arraylen;
 
 /* Set properties between creation and init.  */
 void *qdev_get_prop_ptr(DeviceState *dev, Property *prop);
-int qdev_prop_parse(DeviceState *dev, const char *name, const char *value);
+void qdev_prop_parse(DeviceState *dev, const char *name, const char *value,
+                     Error **errp);
 void qdev_prop_set_bit(DeviceState *dev, const char *name, bool value);
 void qdev_prop_set_uint8(DeviceState *dev, const char *name, uint8_t value);
 void qdev_prop_set_uint16(DeviceState *dev, const char *name, uint16_t value);
@@ -167,7 +168,7 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
 
 void qdev_prop_register_global(GlobalProperty *prop);
 void qdev_prop_register_global_list(GlobalProperty *props);
-void qdev_prop_set_globals(DeviceState *dev);
+void qdev_prop_set_globals(DeviceState *dev, Error **errp);
 void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
                                     Property *prop, const char *value);
 
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 2cb5600..e54dbc2 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -105,13 +105,17 @@ static void qdev_print_devinfo(ObjectClass *klass, void *opaque)
 static int set_property(const char *name, const char *value, void *opaque)
 {
     DeviceState *dev = opaque;
+    Error *err = NULL;
 
     if (strcmp(name, "driver") == 0)
         return 0;
     if (strcmp(name, "bus") == 0)
         return 0;
 
-    if (qdev_prop_parse(dev, name, value) == -1) {
+    qdev_prop_parse(dev, name, value, &err);
+    if (err != NULL) {
+        qerror_report_err(err);
+        error_free(err);
         return -1;
     }
     return 0;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH qom-cpu for-1.5 1/4] qdev: Let qdev_prop_parse() pass through Error

[Eduardo Habkost]

On Wed, May 01, 2013 at 06:07:17PM +0200, Andreas Färber wrote:
> Move error reporting to callers.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>

Keeps existing behavior, and makes sense even if the rest of the series
doesn't get included.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


> ---
>  hw/core/qdev-properties.c    | 25 +++++++++++--------------
>  hw/core/qdev.c               |  7 ++++++-
>  include/hw/qdev-properties.h |  5 +++--
>  qdev-monitor.c               |  6 +++++-
>  4 files changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index ca1739e..716ba19 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -986,25 +986,18 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
>      }
>  }
>  
> -int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
> +void qdev_prop_parse(DeviceState *dev, const char *name, const char *value,
> +                     Error **errp)
>  {
>      char *legacy_name;
> -    Error *err = NULL;
>  
>      legacy_name = g_strdup_printf("legacy-%s", name);
>      if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) {
> -        object_property_parse(OBJECT(dev), value, legacy_name, &err);
> +        object_property_parse(OBJECT(dev), value, legacy_name, errp);
>      } else {
> -        object_property_parse(OBJECT(dev), value, name, &err);
> +        object_property_parse(OBJECT(dev), value, name, errp);
>      }
>      g_free(legacy_name);
> -
> -    if (err) {
> -        qerror_report_err(err);
> -        error_free(err);
> -        return -1;
> -    }
> -    return 0;
>  }
>  
>  void qdev_prop_set_bit(DeviceState *dev, const char *name, bool value)
> @@ -1106,18 +1099,22 @@ void qdev_prop_register_global_list(GlobalProperty *props)
>      }
>  }
>  
> -void qdev_prop_set_globals(DeviceState *dev)
> +void qdev_prop_set_globals(DeviceState *dev, Error **errp)
>  {
>      ObjectClass *class = object_get_class(OBJECT(dev));
>  
>      do {
>          GlobalProperty *prop;
>          QTAILQ_FOREACH(prop, &global_props, next) {
> +            Error *err = NULL;
> +
>              if (strcmp(object_class_get_name(class), prop->driver) != 0) {
>                  continue;
>              }
> -            if (qdev_prop_parse(dev, prop->property, prop->value) != 0) {
> -                exit(1);
> +            qdev_prop_parse(dev, prop->property, prop->value, &err);
> +            if (err != NULL) {
> +                error_propagate(errp, err);
> +                return;
>              }
>          }
>          class = object_class_get_parent(class);
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index ab1d8f5..7167c90 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -752,7 +752,12 @@ static void device_initfn(Object *obj)
>          }
>          class = object_class_get_parent(class);
>      } while (class != object_class_by_name(TYPE_DEVICE));
> -    qdev_prop_set_globals(dev);
> +    qdev_prop_set_globals(dev, &err);
> +    if (err != NULL) {
> +        qerror_report_err(err);
> +        error_free(err);
> +        exit(1);
> +    }
>  
>      object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
>                               (Object **)&dev->parent_bus, &err);
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 25dd1bb..38469d4 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -148,7 +148,8 @@ extern PropertyInfo qdev_prop_arraylen;
>  
>  /* Set properties between creation and init.  */
>  void *qdev_get_prop_ptr(DeviceState *dev, Property *prop);
> -int qdev_prop_parse(DeviceState *dev, const char *name, const char *value);
> +void qdev_prop_parse(DeviceState *dev, const char *name, const char *value,
> +                     Error **errp);
>  void qdev_prop_set_bit(DeviceState *dev, const char *name, bool value);
>  void qdev_prop_set_uint8(DeviceState *dev, const char *name, uint8_t value);
>  void qdev_prop_set_uint16(DeviceState *dev, const char *name, uint16_t value);
> @@ -167,7 +168,7 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
>  
>  void qdev_prop_register_global(GlobalProperty *prop);
>  void qdev_prop_register_global_list(GlobalProperty *props);
> -void qdev_prop_set_globals(DeviceState *dev);
> +void qdev_prop_set_globals(DeviceState *dev, Error **errp);
>  void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
>                                      Property *prop, const char *value);
>  
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 2cb5600..e54dbc2 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -105,13 +105,17 @@ static void qdev_print_devinfo(ObjectClass *klass, void *opaque)
>  static int set_property(const char *name, const char *value, void *opaque)
>  {
>      DeviceState *dev = opaque;
> +    Error *err = NULL;
>  
>      if (strcmp(name, "driver") == 0)
>          return 0;
>      if (strcmp(name, "bus") == 0)
>          return 0;
>  
> -    if (qdev_prop_parse(dev, name, value) == -1) {
> +    qdev_prop_parse(dev, name, value, &err);
> +    if (err != NULL) {
> +        qerror_report_err(err);
> +        error_free(err);
>          return -1;
>      }
>      return 0;
> -- 
> 1.8.1.4
> 

-- 
Eduardo

[Qemu-devel] [PATCH qom-cpu for-1.5 2/4] qdev: Introduce qdev_prop_set_custom_globals()

[Andreas Färber]

Reuse it in qdev_prop_set_globals().

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/core/qdev-properties.c    | 35 ++++++++++++++++++++++++-----------
 include/hw/qdev-properties.h |  2 ++
 2 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 716ba19..68d1bff 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1099,23 +1099,36 @@ void qdev_prop_register_global_list(GlobalProperty *props)
     }
 }
 
+void qdev_prop_set_custom_globals(DeviceState *dev, const char *driver,
+                                  Error **errp)
+{
+    GlobalProperty *prop;
+
+    QTAILQ_FOREACH(prop, &global_props, next) {
+        Error *err = NULL;
+
+        if (strcmp(driver, prop->driver) != 0) {
+            continue;
+        }
+        qdev_prop_parse(dev, prop->property, prop->value, &err);
+        if (err != NULL) {
+            error_propagate(errp, err);
+            return;
+        }
+    }
+}
+
 void qdev_prop_set_globals(DeviceState *dev, Error **errp)
 {
     ObjectClass *class = object_get_class(OBJECT(dev));
 
     do {
-        GlobalProperty *prop;
-        QTAILQ_FOREACH(prop, &global_props, next) {
-            Error *err = NULL;
+        Error *err = NULL;
 
-            if (strcmp(object_class_get_name(class), prop->driver) != 0) {
-                continue;
-            }
-            qdev_prop_parse(dev, prop->property, prop->value, &err);
-            if (err != NULL) {
-                error_propagate(errp, err);
-                return;
-            }
+        qdev_prop_set_custom_globals(dev, object_class_get_name(class), &err);
+        if (err != NULL) {
+            error_propagate(errp, err);
+            return;
         }
         class = object_class_get_parent(class);
     } while (class);
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 38469d4..833300c 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -169,6 +169,8 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
 void qdev_prop_register_global(GlobalProperty *prop);
 void qdev_prop_register_global_list(GlobalProperty *props);
 void qdev_prop_set_globals(DeviceState *dev, Error **errp);
+void qdev_prop_set_custom_globals(DeviceState *dev, const char *driver,
+                                  Error **errp);
 void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
                                     Property *prop, const char *value);
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH qom-cpu for-1.5 2/4] qdev: Introduce qdev_prop_set_custom_globals()

[Eduardo Habkost]

On Wed, May 01, 2013 at 06:07:18PM +0200, Andreas Färber wrote:
> Reuse it in qdev_prop_set_globals().
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>

Very useful to allow us to implement sane bugfixes while the QOM
conversion is still work in progress, and much more flexible than having
to write special-purpose compat functions for code that isn't completely
QOMified yet.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

I assume that although this works with "-global" on the command-line, we
will try to use the (currently) fake class names from patch 3/4 only for
the internal machine-type compat_props tables, and discourage users from
using them in the command-line.


> ---
>  hw/core/qdev-properties.c    | 35 ++++++++++++++++++++++++-----------
>  include/hw/qdev-properties.h |  2 ++
>  2 files changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 716ba19..68d1bff 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1099,23 +1099,36 @@ void qdev_prop_register_global_list(GlobalProperty *props)
>      }
>  }
>  
> +void qdev_prop_set_custom_globals(DeviceState *dev, const char *driver,
> +                                  Error **errp)
> +{
> +    GlobalProperty *prop;
> +
> +    QTAILQ_FOREACH(prop, &global_props, next) {
> +        Error *err = NULL;
> +
> +        if (strcmp(driver, prop->driver) != 0) {
> +            continue;
> +        }
> +        qdev_prop_parse(dev, prop->property, prop->value, &err);
> +        if (err != NULL) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +    }
> +}
> +
>  void qdev_prop_set_globals(DeviceState *dev, Error **errp)
>  {
>      ObjectClass *class = object_get_class(OBJECT(dev));
>  
>      do {
> -        GlobalProperty *prop;
> -        QTAILQ_FOREACH(prop, &global_props, next) {
> -            Error *err = NULL;
> +        Error *err = NULL;
>  
> -            if (strcmp(object_class_get_name(class), prop->driver) != 0) {
> -                continue;
> -            }
> -            qdev_prop_parse(dev, prop->property, prop->value, &err);
> -            if (err != NULL) {
> -                error_propagate(errp, err);
> -                return;
> -            }
> +        qdev_prop_set_custom_globals(dev, object_class_get_name(class), &err);
> +        if (err != NULL) {
> +            error_propagate(errp, err);
> +            return;
>          }
>          class = object_class_get_parent(class);
>      } while (class);
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 38469d4..833300c 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -169,6 +169,8 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
>  void qdev_prop_register_global(GlobalProperty *prop);
>  void qdev_prop_register_global_list(GlobalProperty *props);
>  void qdev_prop_set_globals(DeviceState *dev, Error **errp);
> +void qdev_prop_set_custom_globals(DeviceState *dev, const char *driver,
> +                                  Error **errp);
>  void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
>                                      Property *prop, const char *value);
>  
> -- 
> 1.8.1.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH qom-cpu for-1.5 2/4] qdev: Introduce qdev_prop_set_custom_globals()

[Andreas Färber]

Am 01.05.2013 18:07, schrieb Andreas Färber:
> Reuse it in qdev_prop_set_globals().
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  hw/core/qdev-properties.c    | 35 ++++++++++++++++++++++++-----------
>  include/hw/qdev-properties.h |  2 ++
>  2 files changed, 26 insertions(+), 11 deletions(-)

Igor suggested to avoid "custom", so renaming to
qdev_prop_set_globals_for_type():

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 68d1bff..3a324fb 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1099,15 +1099,15 @@ void
qdev_prop_register_global_list(GlobalProperty *props)
     }
 }

-void qdev_prop_set_custom_globals(DeviceState *dev, const char *driver,
-                                  Error **errp)
+void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
+                                    Error **errp)
 {
     GlobalProperty *prop;

     QTAILQ_FOREACH(prop, &global_props, next) {
         Error *err = NULL;

-        if (strcmp(driver, prop->driver) != 0) {
+        if (strcmp(typename, prop->driver) != 0) {
             continue;
         }
         qdev_prop_parse(dev, prop->property, prop->value, &err);
@@ -1125,7 +1125,8 @@ void qdev_prop_set_globals(DeviceState *dev, Error
**errp)
     do {
         Error *err = NULL;

-        qdev_prop_set_custom_globals(dev, object_class_get_name(class),
&err);
+        qdev_prop_set_globals_for_type(dev, object_class_get_name(class),
+                                       &err);
         if (err != NULL) {
             error_propagate(errp, err);
             return;
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 833300c..39448b7 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -169,8 +169,8 @@ void qdev_prop_set_ptr(DeviceState *dev, const char
*name, void *value);
 void qdev_prop_register_global(GlobalProperty *prop);
 void qdev_prop_register_global_list(GlobalProperty *props);
 void qdev_prop_set_globals(DeviceState *dev, Error **errp);
-void qdev_prop_set_custom_globals(DeviceState *dev, const char *driver,
-                                  Error **errp);
+void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
+                                    Error **errp);
 void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState
*dev,
                                     Property *prop, const char *value);


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH qom-cpu for-1.5 3/4] target-i386: Emulate X86CPU subclasses for global properties

[Andreas Färber]

After initializing the object from its x86_def_t and before setting any
additional -cpu arguments, set any global properties for the designated
subclass <name>-{i386,x86_64}-cpu.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 target-i386/cpu.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index bba41fe..8a9563b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1626,6 +1626,7 @@ X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
     CPUX86State *env;
     gchar **model_pieces;
     char *name, *features;
+    char *typename;
     Error *error = NULL;
 
     model_pieces = g_strsplit(cpu_model, ",", 2);
@@ -1653,6 +1654,14 @@ X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
         goto out;
     }
 
+    /* Emulate per-model subclasses for global properties */
+    typename = g_strdup_printf("%s-" TYPE_X86_CPU, name);
+    qdev_prop_set_custom_globals(DEVICE(cpu), typename, &error);
+    g_free(typename);
+    if (error) {
+        goto out;
+    }
+
     cpu_x86_parse_featurestr(cpu, features, &error);
     if (error) {
         goto out;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH qom-cpu for-1.5 3/4] target-i386: Emulate X86CPU subclasses for global properties

[Eduardo Habkost]

On Wed, May 01, 2013 at 06:07:19PM +0200, Andreas Färber wrote:
> After initializing the object from its x86_def_t and before setting any
> additional -cpu arguments, set any global properties for the designated
> subclass <name>-{i386,x86_64}-cpu.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  target-i386/cpu.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index bba41fe..8a9563b 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1626,6 +1626,7 @@ X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
>      CPUX86State *env;
>      gchar **model_pieces;
>      char *name, *features;
> +    char *typename;
>      Error *error = NULL;
>  
>      model_pieces = g_strsplit(cpu_model, ",", 2);
> @@ -1653,6 +1654,14 @@ X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
>          goto out;
>      }
>  
> +    /* Emulate per-model subclasses for global properties */
> +    typename = g_strdup_printf("%s-" TYPE_X86_CPU, name);
> +    qdev_prop_set_custom_globals(DEVICE(cpu), typename, &error);
> +    g_free(typename);
> +    if (error) {
> +        goto out;
> +    }

I really like this solution.

I find it ironic that we spent 2 or 3 releases struggling with the
imposed restriction that global-properties can't be used to set dynamic
QOM properties (only static properties), and now we are introducing a
mechanism to do exactly that.  :-)

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


> +
>      cpu_x86_parse_featurestr(cpu, features, &error);
>      if (error) {
>          goto out;
> -- 
> 1.8.1.4
> 

-- 
Eduardo

[Qemu-devel] [PATCH qom-cpu for-1.5 4/4] target-i386: Change CPUID model of 486 to 8

[Andreas Färber]

This changes the model number of 486 to 8 (DX4) which matches the
feature set presented, and actually has the CPUID instruction.

This adds a compatibility property, to keep model=0 on pc-*-1.4 and older.

Signed-off-by: H. Peter Anvin <hpa@zytor.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
[AF: Add compat_props entry]
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 include/hw/i386/pc.h | 4 ++++
 target-i386/cpu.c    | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 41869e5..417afe4 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -242,6 +242,10 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
             .driver   = "pc-sysfw",\
             .property = "rom_only",\
             .value    = stringify(0),\
+        },{\
+            .driver   = "486-" TYPE_X86_CPU,\
+            .property = "model",\
+            .value    = stringify(0),\
         }
 
 #endif
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 8a9563b..f7d4d9b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -550,7 +550,7 @@ static x86_def_t builtin_x86_defs[] = {
         .level = 1,
         .vendor = CPUID_VENDOR_INTEL,
         .family = 4,
-        .model = 0,
+        .model = 8,
         .stepping = 0,
         .features = I486_FEATURES,
         .xlevel = 0,
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH qom-cpu for-1.5 4/4] target-i386: Change CPUID model of 486 to 8

[Eduardo Habkost]

On Wed, May 01, 2013 at 06:07:20PM +0200, Andreas Färber wrote:
> This changes the model number of 486 to 8 (DX4) which matches the
> feature set presented, and actually has the CPUID instruction.
> 
> This adds a compatibility property, to keep model=0 on pc-*-1.4 and older.
> 
> Signed-off-by: H. Peter Anvin <hpa@zytor.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> [AF: Add compat_props entry]
> Signed-off-by: Andreas Färber <afaerber@suse.de>

Tested by running a simple CPUID-dump test kernel, CPUID data is
changing according to the machine-type as expected. Didn't test using a
Linux kernel.

Tested-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


> ---
>  include/hw/i386/pc.h | 4 ++++
>  target-i386/cpu.c    | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 41869e5..417afe4 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -242,6 +242,10 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
>              .driver   = "pc-sysfw",\
>              .property = "rom_only",\
>              .value    = stringify(0),\
> +        },{\
> +            .driver   = "486-" TYPE_X86_CPU,\
> +            .property = "model",\
> +            .value    = stringify(0),\
>          }
>  
>  #endif
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 8a9563b..f7d4d9b 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -550,7 +550,7 @@ static x86_def_t builtin_x86_defs[] = {
>          .level = 1,
>          .vendor = CPUID_VENDOR_INTEL,
>          .family = 4,
> -        .model = 0,
> +        .model = 8,
>          .stepping = 0,
>          .features = I486_FEATURES,
>          .xlevel = 0,
> -- 
> 1.8.1.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH qom-cpu for-1.5 0/4] target-i386: X86CPU compatibility properties

[Anthony Liguori]

Andreas Färber <afaerber@suse.de> writes:

> Hello,
>
> It's easier adapting the infrastructure to our needs than working around it:
> X86CPU already has QOM properties today. What's lacking is model subclasses,
> and with the one X86CPU type its global properties are overwritten by models.
> But we already know the designated naming scheme for the models!
>
> So let's simply prepare compat_props for CPU models and make sure they are
> already picked up today.
>
> This works just fine for changing the 486 CPUID model value and avoids to
> redo the PC part once we have X86CPU subclasses.
> Tested using: ./QMP/qom-get /machine/icc-bridge/icc/child[0].model

So, what's left to do with subclass modelling?

How long are we going to need to carry something like this?

It's a clever work around but I'm a bit concerned that it would grow
beyond cpu subclasses and that we'd be stuck with it forever.

Regards,

Anthony Liguori

>
> For changing n270 CPUID flags we'll still need to resort to Eduardo's proposed
> helper functions for now.
>
> Regards,
> Andreas
>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Anthony Liguori <anthony@codemonkey.ws>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
>
> Andreas Färber (4):
>   qdev: Let qdev_prop_parse() pass through Error
>   qdev: Introduce qdev_prop_set_custom_globals()
>   target-i386: Emulate X86CPU subclasses for global properties
>   target-i386: Change CPUID model of 486 to 8
>
>  hw/core/qdev-properties.c    | 50 ++++++++++++++++++++++++++------------------
>  hw/core/qdev.c               |  7 ++++++-
>  include/hw/i386/pc.h         |  4 ++++
>  include/hw/qdev-properties.h |  7 +++++--
>  qdev-monitor.c               |  6 +++++-
>  target-i386/cpu.c            | 11 +++++++++-
>  6 files changed, 60 insertions(+), 25 deletions(-)
>
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH qom-cpu for-1.5 0/4] target-i386: X86CPU compatibility properties

[Igor Mammedov]

On Fri, 03 May 2013 11:46:59 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> Andreas Färber <afaerber@suse.de> writes:
> 
> > Hello,
> >
> > It's easier adapting the infrastructure to our needs than working around it:
> > X86CPU already has QOM properties today. What's lacking is model subclasses,
> > and with the one X86CPU type its global properties are overwritten by models.
> > But we already know the designated naming scheme for the models!
> >
> > So let's simply prepare compat_props for CPU models and make sure they are
> > already picked up today.
> >
> > This works just fine for changing the 486 CPUID model value and avoids to
> > redo the PC part once we have X86CPU subclasses.
> > Tested using: ./QMP/qom-get /machine/icc-bridge/icc/child[0].model
> 
> So, what's left to do with subclass modelling?
I plan to resubmit static properties and then subclass modeling for 1.6.

> 
> How long are we going to need to carry something like this?
It will be removed with x86 CPU static properties and CPU sub-classes.

> 
> It's a clever work around but I'm a bit concerned that it would grow
> beyond cpu subclasses and that we'd be stuck with it forever.
> 
> Regards,
> 
> Anthony Liguori
> 
> >
> > For changing n270 CPUID flags we'll still need to resort to Eduardo's proposed
> > helper functions for now.
> >
> > Regards,
> > Andreas
> >
> > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > Cc: H. Peter Anvin <hpa@zytor.com>
> > Cc: Borislav Petkov <bp@suse.de>
> > Cc: Anthony Liguori <anthony@codemonkey.ws>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> >
> > Andreas Färber (4):
> >   qdev: Let qdev_prop_parse() pass through Error
> >   qdev: Introduce qdev_prop_set_custom_globals()
> >   target-i386: Emulate X86CPU subclasses for global properties
> >   target-i386: Change CPUID model of 486 to 8
> >
> >  hw/core/qdev-properties.c    | 50 ++++++++++++++++++++++++++------------------
> >  hw/core/qdev.c               |  7 ++++++-
> >  include/hw/i386/pc.h         |  4 ++++
> >  include/hw/qdev-properties.h |  7 +++++--
> >  qdev-monitor.c               |  6 +++++-
> >  target-i386/cpu.c            | 11 +++++++++-
> >  6 files changed, 60 insertions(+), 25 deletions(-)
> >
> > -- 
> > 1.8.1.4


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH qom-cpu for-1.5 0/4] target-i386: X86CPU compatibility properties

[Andreas Färber]

Am 03.05.2013 18:46, schrieb Anthony Liguori:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> Hello,
>>
>> It's easier adapting the infrastructure to our needs than working around it:
>> X86CPU already has QOM properties today. What's lacking is model subclasses,
>> and with the one X86CPU type its global properties are overwritten by models.
>> But we already know the designated naming scheme for the models!
>>
>> So let's simply prepare compat_props for CPU models and make sure they are
>> already picked up today.
>>
>> This works just fine for changing the 486 CPUID model value and avoids to
>> redo the PC part once we have X86CPU subclasses.
>> Tested using: ./QMP/qom-get /machine/icc-bridge/icc/child[0].model
> 
> So, what's left to do with subclass modelling?

Well, in the past there had been multiple approaches:

X86CPUInfo (me)
instance_init (proposed my PMM, no code)
class_init (me)
x86_def_t (me)
static properties (imammedo)

Either of the last three ran into issues/discussions for -cpu host.

Igor's proposal is touching on the same issue as this series:

qdev_set_global_properties() is run from device's instance_init, which
is run *before* derived classes add any QOM-style properties in theirs.
So for 1.6 I would rather like to amend our QOM infrastructure with an
.instance_post_init hook (prior art for qdev props: .class_base_init) or
otherwise allow to set global defaults for any device or QOM object than
converting perfectly fine QOM properties back into qdev static
properties, when it's all just for one single function call. :)

> How long are we going to need to carry something like this?

I've always been hoping to get subclasses done for the next release...
That said, it depends on whether we can find an agreeable solution for
the above when-do-we-set-globals problem. If we do need static
properties, Igor has a patch to that effect that looked okay.

> It's a clever work around but I'm a bit concerned that it would grow
> beyond cpu subclasses and that we'd be stuck with it forever.

I would've proposed to add a header comment indicating it's not to be
generally used, but experience shows such comments are rarely read.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH qom-cpu for-1.5 0/4] target-i386: X86CPU compatibility properties

[Andreas Färber]

Am 01.05.2013 18:07, schrieb Andreas Färber:
> Hello,
> 
> It's easier adapting the infrastructure to our needs than working around it:
> X86CPU already has QOM properties today. What's lacking is model subclasses,
> and with the one X86CPU type its global properties are overwritten by models.
> But we already know the designated naming scheme for the models!
> 
> So let's simply prepare compat_props for CPU models and make sure they are
> already picked up today.
> 
> This works just fine for changing the 486 CPUID model value and avoids to
> redo the PC part once we have X86CPU subclasses.
> Tested using: ./QMP/qom-get /machine/icc-bridge/icc/child[0].model
> 
> For changing n270 CPUID flags we'll still need to resort to Eduardo's proposed
> helper functions for now.
> 
> Regards,
> Andreas
> 
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Anthony Liguori <anthony@codemonkey.ws>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> 
> Andreas Färber (4):
>   qdev: Let qdev_prop_parse() pass through Error
>   qdev: Introduce qdev_prop_set_custom_globals()
>   target-i386: Emulate X86CPU subclasses for global properties
>   target-i386: Change CPUID model of 486 to 8

Hearing no veto from Anthony on IRC, I have applied these to qom-cpu
(with helper function renamed):
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

Andreas

> 
>  hw/core/qdev-properties.c    | 50 ++++++++++++++++++++++++++------------------
>  hw/core/qdev.c               |  7 ++++++-
>  include/hw/i386/pc.h         |  4 ++++
>  include/hw/qdev-properties.h |  7 +++++--
>  qdev-monitor.c               |  6 +++++-
>  target-i386/cpu.c            | 11 +++++++++-
>  6 files changed, 60 insertions(+), 25 deletions(-)
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg