[Qemu-devel] [PATCH RFC 0/3] qom: Generalize qdev init to "realize"

[Qemu-devel] [PATCH RFC 0/3] qom: Generalize qdev init to "realize"

[Andreas Färber]

Hello Anthony,

Here's a mini series introducing ObjectClass::realize(Object *) and
forwarding it to existing DeviceClass::init(DeviceState *).

I've also added a convenience wrapper for setting the "realized" property
and doing error checking and exit, tested with the SH7750 SoC.

Regards,
Andreas

Cc: Anthony Liguori <anthony@codemonkey.ws>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Wanpeng Li <liwp@linux.vnet.ibm.com>

Andreas Färber (3):
  qom: Add "realized" property to Object
  qom: Introduce object_realize()
  qdev: Hook up DeviceClass::init to ObjectClass::realize

 hw/qdev.c             |   13 +++++++++++++
 include/qemu/object.h |   11 +++++++++++
 qom/object.c          |   42 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 0 deletions(-)

-- 
1.7.7

[Qemu-devel] [PATCH RFC 1/3] qom: Add "realized" property to Object

[Andreas Färber]

The Object::realized property can only be set once and, on setting it,
invokes the ObjectClass::realize callback.

Signed-off-by: Andreas Färber <afaerber@suse.de>
Cc: Anthony Liguori <anthony@codemonkey.ws>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/object.h |    2 ++
 qom/object.c          |   31 +++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/include/qemu/object.h b/include/qemu/object.h
index e8fc126..742b5b6 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -239,6 +239,7 @@ struct ObjectClass
 {
     /*< private >*/
     Type type;
+    int (*realize)(Object *obj);
 };
 
 /**
@@ -264,6 +265,7 @@ struct Object
     QTAILQ_HEAD(, ObjectProperty) properties;
     uint32_t ref;
     Object *parent;
+    bool realized;
 };
 
 /**
diff --git a/qom/object.c b/qom/object.c
index 9cd9506..ec143ad 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -273,6 +273,34 @@ static void object_init_with_type(Object *obj, TypeImpl *ti)
     }
 }
 
+static void object_get_realized(Object *obj, Visitor *v, void *opaque,
+                                const char *name, Error **errp)
+{
+    visit_type_bool(v, &obj->realized, name, errp);
+}
+
+static void object_set_realized(Object *obj, Visitor *v, void *opaque,
+                                const char *name, Error **errp)
+{
+    bool value;
+
+    if (obj->realized) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    }
+
+    visit_type_bool(v, &value, name, errp);
+    if (error_is_set(errp) || !value) {
+        return;
+    }
+
+    if (obj->class->realize != NULL && obj->class->realize(obj) != 0) {
+        error_set(errp, QERR_DEVICE_INIT_FAILED, object_get_typename(obj));
+        return;
+    }
+    obj->realized = true;
+}
+
 void object_initialize_with_type(void *data, TypeImpl *type)
 {
     Object *obj = data;
@@ -287,6 +315,9 @@ void object_initialize_with_type(void *data, TypeImpl *type)
     obj->class = type->class;
     QTAILQ_INIT(&obj->properties);
     object_init_with_type(obj, type);
+    object_property_add(obj, "realized", "boolean",
+                        object_get_realized, object_set_realized,
+                        NULL, NULL, NULL);
 }
 
 void object_initialize(void *data, const char *typename)
-- 
1.7.7

[Qemu-devel] [PATCH RFC 2/3] qom: Introduce object_realize()

[Andreas Färber]

Wrap setting of Object::realized property, error reporting and exit(1)
into a helper function.

Signed-off-by: Andreas Färber <afaerber@suse.de>
Cc: Anthony Liguori <anthony@codemonkey.ws>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/object.h |    9 +++++++++
 qom/object.c          |   11 +++++++++++
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/include/qemu/object.h b/include/qemu/object.h
index 742b5b6..dc8ae0f 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -467,6 +467,15 @@ void object_initialize_with_type(void *data, Type type);
 void object_initialize(void *obj, const char *typename);
 
 /**
+ * object_realize:
+ * @obj: The object to realize.
+ *
+ * This function will complete the initialization of an object based on
+ * properties set by setting the "realized" property to true.
+ */
+void object_realize(Object *obj);
+
+/**
  * object_finalize:
  * @obj: The object to finalize.
  *
diff --git a/qom/object.c b/qom/object.c
index ec143ad..33f6efc 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -327,6 +327,17 @@ void object_initialize(void *data, const char *typename)
     object_initialize_with_type(data, type);
 }
 
+void object_realize(Object *obj)
+{
+    Error *err = NULL;
+
+    object_property_set_bool(obj, true, "realized", &err);
+    if (error_is_set(&err)) {
+        qerror_report_err(err);
+        exit(1);
+    }
+}
+
 static void object_property_del_all(Object *obj)
 {
     while (!QTAILQ_EMPTY(&obj->properties)) {
-- 
1.7.7

[Qemu-devel] [PATCH RFC 3/3] qdev: Hook up DeviceClass::init to ObjectClass::realize

[Andreas Färber]

On realize, call the qdev init function.
If that returns an error, raise QERR_DEVICE_INIT_FAILED.

Signed-off-by: Andreas Färber <afaerber@suse.de>
Cc: Anthony Liguori <anthony@codemonkey.ws>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index ee21d90..c84b656 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -649,6 +649,13 @@ static void device_finalize(Object *obj)
     QTAILQ_REMOVE(&dev->parent_bus->children, dev, sibling);
 }
 
+static int device_realize(Object *obj)
+{
+    DeviceState *dev = DEVICE(obj);
+
+    return qdev_init(dev);
+}
+
 void device_reset(DeviceState *dev)
 {
     DeviceClass *klass = DEVICE_GET_CLASS(dev);
@@ -658,6 +665,11 @@ void device_reset(DeviceState *dev)
     }
 }
 
+static void device_class_init(ObjectClass *class, void *data)
+{
+    class->realize = device_realize;
+}
+
 static TypeInfo device_type_info = {
     .name = TYPE_DEVICE,
     .parent = TYPE_OBJECT,
@@ -666,6 +678,7 @@ static TypeInfo device_type_info = {
     .instance_finalize = device_finalize,
     .abstract = true,
     .class_size = sizeof(DeviceClass),
+    .class_init = device_class_init,
 };
 
 static void qdev_register_types(void)
-- 
1.7.7

Re: [Qemu-devel] [PATCH RFC 3/3] qdev: Hook up DeviceClass::init to ObjectClass::realize

[Andreas Färber]

Am 26.03.2012 15:46, schrieb Andreas Färber:
> On realize, call the qdev init function.

> If that returns an error, raise QERR_DEVICE_INIT_FAILED.

Sorry, that sentence is outdated - the error is set in qom/object.c
(patch 1/3) for non-zero return values. It is merely being passed
through here.

Andreas

> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> Cc: Anthony Liguori <anthony@codemonkey.ws>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/qdev.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index ee21d90..c84b656 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -649,6 +649,13 @@ static void device_finalize(Object *obj)
>      QTAILQ_REMOVE(&dev->parent_bus->children, dev, sibling);
>  }
>  
> +static int device_realize(Object *obj)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +
> +    return qdev_init(dev);
> +}
> +
>  void device_reset(DeviceState *dev)
>  {
>      DeviceClass *klass = DEVICE_GET_CLASS(dev);
[snip]

-- 
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 RFC 1/3] qom: Add "realized" property to Object

[Andreas Färber]

Am 26.03.2012 15:46, schrieb Andreas Färber:
> The Object::realized property can only be set once and, on setting it,
> invokes the ObjectClass::realize callback.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> Cc: Anthony Liguori <anthony@codemonkey.ws>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qemu/object.h |    2 ++
>  qom/object.c          |   31 +++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+), 0 deletions(-)
[snip]
> diff --git a/qom/object.c b/qom/object.c
> index 9cd9506..ec143ad 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -273,6 +273,34 @@ static void object_init_with_type(Object *obj, TypeImpl *ti)
>      }
>  }
>  
> +static void object_get_realized(Object *obj, Visitor *v, void *opaque,
> +                                const char *name, Error **errp)
> +{
> +    visit_type_bool(v, &obj->realized, name, errp);
> +}
> +
> +static void object_set_realized(Object *obj, Visitor *v, void *opaque,
> +                                const char *name, Error **errp)
> +{
> +    bool value;
> +
> +    if (obj->realized) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +
> +    visit_type_bool(v, &value, name, errp);
> +    if (error_is_set(errp) || !value) {
> +        return;
> +    }
> +
> +    if (obj->class->realize != NULL && obj->class->realize(obj) != 0) {
> +        error_set(errp, QERR_DEVICE_INIT_FAILED, object_get_typename(obj));

Will introduce a QERR_OBJECT_REALIZE_FAILED for v2 based on feedback.

/-F

> +        return;
> +    }
> +    obj->realized = true;
> +}
> +
>  void object_initialize_with_type(void *data, TypeImpl *type)
>  {
>      Object *obj = data;
[snip]

-- 
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 RFC 1/3] qom: Add "realized" property to Object

[Stefan Hajnoczi]

On Mon, Mar 26, 2012 at 2:46 PM, Andreas Färber <afaerber@suse.de> wrote:
> The Object::realized property can only be set once and, on setting it,
> invokes the ObjectClass::realize callback.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> Cc: Anthony Liguori <anthony@codemonkey.ws>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qemu/object.h |    2 ++
>  qom/object.c          |   31 +++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+), 0 deletions(-)
>
> diff --git a/include/qemu/object.h b/include/qemu/object.h
> index e8fc126..742b5b6 100644
> --- a/include/qemu/object.h
> +++ b/include/qemu/object.h
> @@ -239,6 +239,7 @@ struct ObjectClass
>  {
>     /*< private >*/
>     Type type;
> +    int (*realize)(Object *obj);
>  };
>
>  /**
> @@ -264,6 +265,7 @@ struct Object
>     QTAILQ_HEAD(, ObjectProperty) properties;
>     uint32_t ref;
>     Object *parent;
> +    bool realized;
>  };
>
>  /**
> diff --git a/qom/object.c b/qom/object.c
> index 9cd9506..ec143ad 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -273,6 +273,34 @@ static void object_init_with_type(Object *obj, TypeImpl *ti)
>     }
>  }
>
> +static void object_get_realized(Object *obj, Visitor *v, void *opaque,
> +                                const char *name, Error **errp)
> +{
> +    visit_type_bool(v, &obj->realized, name, errp);
> +}
> +
> +static void object_set_realized(Object *obj, Visitor *v, void *opaque,
> +                                const char *name, Error **errp)
> +{
> +    bool value;
> +
> +    if (obj->realized) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +
> +    visit_type_bool(v, &value, name, errp);
> +    if (error_is_set(errp) || !value) {
> +        return;
> +    }
> +
> +    if (obj->class->realize != NULL && obj->class->realize(obj) != 0) {
> +        error_set(errp, QERR_DEVICE_INIT_FAILED, object_get_typename(obj));

->realize() has no way of giving a detailed error.  I think we should
pass in errp.

Stefan

Re: [Qemu-devel] [PATCH RFC 0/3] qom: Generalize qdev init to "realize"

[Paolo Bonzini]

Il 26/03/2012 15:46, Andreas Färber ha scritto:
> Hello Anthony,
> 
> Here's a mini series introducing ObjectClass::realize(Object *) and
> forwarding it to existing DeviceClass::init(DeviceState *).

I think a major difference between realize and init should be that the
realize property also propagates down the whole composition tree (in
pre-order for setting to true, and post-order for setting to false).

An important, related point is whether the composition tree should be
created before or after realize/init.  Right now the few examples in the
tree do it after, but this sounds wrong to me and would block the above
auto-propagation.

Paolo

Re: [Qemu-devel] [PATCH RFC 0/3] qom: Generalize qdev init to "realize"

[Anthony Liguori]

On 03/27/2012 11:19 AM, Paolo Bonzini wrote:
> Il 26/03/2012 15:46, Andreas Färber ha scritto:
>> Hello Anthony,
>>
>> Here's a mini series introducing ObjectClass::realize(Object *) and
>> forwarding it to existing DeviceClass::init(DeviceState *).
>
> I think a major difference between realize and init should be that the
> realize property also propagates down the whole composition tree (in
> pre-order for setting to true, and post-order for setting to false).

Yes, I haven't reviewed this series yet, but my expectation would be that 
realize propagates and that the default implementation of DeviceClass::realize() 
would explicitly *not* propagate and instead just call ::init.

Likewise, unrealize() should do the same with reset().

Regards,

Anthony Liguori

>
> An important, related point is whether the composition tree should be
> created before or after realize/init.  Right now the few examples in the
> tree do it after, but this sounds wrong to me and would block the above
> auto-propagation.
>
> Paolo

Re: [Qemu-devel] [PATCH RFC 0/3] qom: Generalize qdev init to "realize"

[Peter Maydell]

On 27 March 2012 17:28, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 03/27/2012 11:19 AM, Paolo Bonzini wrote:
>> I think a major difference between realize and init should be that the
>> realize property also propagates down the whole composition tree (in
>> pre-order for setting to true, and post-order for setting to false).
>
>
> Yes, I haven't reviewed this series yet, but my expectation would be that
> realize propagates and that the default implementation of
> DeviceClass::realize() would explicitly *not* propagate and instead just
> call ::init.

So who calls realize for non-qdev QOM objects which are children of
qdev QOM objects?

I really don't like having the object lifecycle methods be different
for DeviceClass than for base objects if we can avoid it.

-- PMM

Re: [Qemu-devel] [PATCH RFC 0/3] qom: Generalize qdev init to "realize"

[Paolo Bonzini]

Il 27/03/2012 18:28, Anthony Liguori ha scritto:
> 
> Yes, I haven't reviewed this series yet, but my expectation would be
> that realize propagates and that the default implementation of
> DeviceClass::realize() would explicitly *not* propagate and instead just
> call ::init.
> 
> Likewise, unrealize() should do the same with reset().

Andreas, since this came up three times in three different threads
today, I sent my series to give all objects a canonical path.  With that
series and yours together, you should be able to experiment with pushing
qdev_init out of functions such as rtc_init, and instead just set
realize=true for the root (or the /machine container added in that series).

Paolo

Re: [Qemu-devel] [PATCH RFC 0/3] qom: Generalize qdev init to "realize"

[Paolo Bonzini]

Il 27/03/2012 18:33, Peter Maydell ha scritto:
>> > Yes, I haven't reviewed this series yet, but my expectation would be that
>> > realize propagates and that the default implementation of
>> > DeviceClass::realize() would explicitly *not* propagate and instead just
>> > call ::init.
> So who calls realize for non-qdev QOM objects which are children of
> qdev QOM objects?
> 
> I really don't like having the object lifecycle methods be different
> for DeviceClass than for base objects if we can avoid it.

The way I read it was that the "realize" property propagates to the
children and calls either the "realize" or "reset" method (in the
appropriate order).

DeviceClass::realize() would also do other things done currently by
qdev_init, such as register vmstate, so there would still be a
separation between DeviceClass::realize and DeviceClass::init (we do not
want yet another pass through the whole tree).

qdev_init would be a thin wrapper around object_realize that takes care
of freeing the object when init fails.

Is this correct?

Paolo

Re: [Qemu-devel] [PATCH RFC 0/3] qom: Generalize qdev init to "realize"

[Anthony Liguori]

On 03/27/2012 11:46 AM, Paolo Bonzini wrote:
> Il 27/03/2012 18:33, Peter Maydell ha scritto:
>>>> Yes, I haven't reviewed this series yet, but my expectation would be that
>>>> realize propagates and that the default implementation of
>>>> DeviceClass::realize() would explicitly *not* propagate and instead just
>>>> call ::init.
>> So who calls realize for non-qdev QOM objects which are children of
>> qdev QOM objects?
>>
>> I really don't like having the object lifecycle methods be different
>> for DeviceClass than for base objects if we can avoid it.
>
> The way I read it was that the "realize" property propagates to the
> children and calls either the "realize" or "reset" method (in the
> appropriate order).

Correct.

We would probably want to have two methods in Object:

/* realize yourself */
void realize(Error **errp);

/* realize your children */
void realize_children(Error **errp);

The actual realize property would call this->realize(errp); then 
this->realize_children(errp) modulo some cleverness to support transversal ordering.

That means usually, you can just implement realize() and rely on the standard 
child realize support.  But you could overload realize_children() if you needed to.

realize_children() would propagate to child<> properties.  All child<> 
properties trace their parentage back to the object_get_root() device.  So you 
can realize() the entire device/object model by setting the top level realize 
property.

>
> DeviceClass::realize() would also do other things done currently by
> qdev_init, such as register vmstate,

Correct.  I think DeviceClass::realize() should call DeviceClass::init() to 
start out with.  Then we can refactor incrementally to move stuff from ::init() 
to ::realize().  Some parts of :init() will get moved to the constructor.

> so there would still be a
> separation between DeviceClass::realize and DeviceClass::init (we do not
> want yet another pass through the whole tree).
>
> qdev_init would be a thin wrapper around object_realize that takes care
> of freeing the object when init fails.

Absolutely.

Regards,

Anthony Liguori

>
> Is this correct?
>
> Paolo