[Qemu-devel] [PATCH 0/2] qdev: make reset propagation overrideable by subclasses

[Qemu-devel] [PATCH 0/2] qdev: make reset propagation overrideable by subclasses

[Anthony Liguori]

This series allows subclasses to override how reset propagates to child
devices and/or busses.

This will allow a controller to perform specific actions either before or
after reset occurs for a specific device.

Since DeviceState::reset should model cold reset, I don't believe that any
controller would really need to override propagation order but I think its
useful to demonstrate how a bus specific reset function should be implemented.

This applies on top of Paolo's virtio-scsi reset fix patch series.

[Qemu-devel] [PATCH 1/2] qdev: make reset propagation overrideable by subclasses

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev-core.h | 16 +++++++++++++++-
 hw/qdev.c      | 11 ++++++++++-
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/hw/qdev-core.h b/hw/qdev-core.h
index 853bd08..f40fd15 100644
--- a/hw/qdev-core.h
+++ b/hw/qdev-core.h
@@ -36,9 +36,23 @@ typedef struct DeviceClass {
     Property *props;
     int no_user;
 
-    /* callbacks */
+    /**
+     * reset:
+     *
+     * Cold reset of a device. This function must be implemented by a device's
+     * that never have children busses.
+     */
     void (*reset)(DeviceState *dev);
 
+    /**
+     * reset_all:
+     *
+     * Cold reset of a device with children.  The default implementation of this
+     * method will invoke DeviceClass::reset and then recursively call
+     * qbus_reset_all() on each child in an arbitrary order.
+     */
+    void (*reset_all)(DeviceState *dev);
+
     /* device state */
     const struct VMStateDescription *vmsd;
 
diff --git a/hw/qdev.c b/hw/qdev.c
index 1b68d02..e02b5be 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -223,11 +223,17 @@ static int qbus_reset_one(BusState *bus, void *opaque)
     return 0;
 }
 
-void qdev_reset_all(DeviceState *dev)
+static void qdev_reset_children(DeviceState *dev)
 {
     qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL);
 }
 
+void qdev_reset_all(DeviceState *dev)
+{
+    DeviceClass *dc = DEVICE_GET_CLASS(dev);
+    dc->reset_all(dev);
+}
+
 void qbus_reset_all(BusState *bus)
 {
     qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
@@ -714,7 +720,10 @@ static void device_unparent(Object *obj)
 
 static void device_class_init(ObjectClass *class, void *data)
 {
+    DeviceClass *dc = DEVICE_CLASS(class);
+
     class->unparent = device_unparent;
+    dc->reset_all = qdev_reset_children;
 }
 
 void device_reset(DeviceState *dev)
-- 
1.8.0

[Qemu-devel] [PATCH 2/2] qbus: make bus reset overrideable by subclasses

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev-core.h | 15 +++++++++++++++
 hw/qdev.c      | 16 +++++++++++++++-
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/hw/qdev-core.h b/hw/qdev-core.h
index f40fd15..453a061 100644
--- a/hw/qdev-core.h
+++ b/hw/qdev-core.h
@@ -100,7 +100,22 @@ struct BusClass {
      * bindings can be found at http://playground.sun.com/1275/bindings/.
      */
     char *(*get_fw_dev_path)(DeviceState *dev);
+
+    /**
+     * reset:
+     *
+     * Cold reset of a bus. This only resets the controller state of the bus.
+     */
     int (*reset)(BusState *bus);
+
+    /**
+     * reset_all:
+     *
+     * Cold reset of a bus with children.  The default implementation of this
+     * method will invoke BusState::reset and then recursively call
+     * qdev_reset_all() on each child in an arbitrary order.
+     */
+    void (*reset_all)(BusState *bus);
 };
 
 typedef struct BusChild {
diff --git a/hw/qdev.c b/hw/qdev.c
index e02b5be..db19b14 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -234,11 +234,17 @@ void qdev_reset_all(DeviceState *dev)
     dc->reset_all(dev);
 }
 
-void qbus_reset_all(BusState *bus)
+static void qbus_reset_children(BusState *bus)
 {
     qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
 }
 
+void qbus_reset_all(BusState *bus)
+{
+    BusClass *bc = BUS_GET_CLASS(bus);
+    bc->reset_all(bus);
+}
+
 void qbus_reset_all_fn(void *opaque)
 {
     BusState *bus = opaque;
@@ -758,6 +764,13 @@ static const TypeInfo device_type_info = {
     .class_size = sizeof(DeviceClass),
 };
 
+static void qbus_class_initfn(ObjectClass *klass, void *data)
+{
+    BusClass *bc = BUS_CLASS(klass);
+
+    bc->reset_all = qbus_reset_children;
+}
+
 static void qbus_initfn(Object *obj)
 {
     BusState *bus = BUS(obj);
@@ -789,6 +802,7 @@ static const TypeInfo bus_info = {
     .parent = TYPE_OBJECT,
     .instance_size = sizeof(BusState),
     .abstract = true,
+    .class_init = qbus_class_initfn,
     .class_size = sizeof(BusClass),
     .instance_init = qbus_initfn,
     .instance_finalize = qbus_finalize,
-- 
1.8.0

Re: [Qemu-devel] [PATCH 2/2] qbus: make bus reset overrideable by subclasses

[Paolo Bonzini]

Il 11/01/2013 15:17, Anthony Liguori ha scritto:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/qdev-core.h | 15 +++++++++++++++
>  hw/qdev.c      | 16 +++++++++++++++-
>  2 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/qdev-core.h b/hw/qdev-core.h
> index f40fd15..453a061 100644
> --- a/hw/qdev-core.h
> +++ b/hw/qdev-core.h
> @@ -100,7 +100,22 @@ struct BusClass {
>       * bindings can be found at http://playground.sun.com/1275/bindings/.
>       */
>      char *(*get_fw_dev_path)(DeviceState *dev);
> +
> +    /**
> +     * reset:
> +     *
> +     * Cold reset of a bus. This only resets the controller state of the bus.
> +     */
>      int (*reset)(BusState *bus);
> +
> +    /**
> +     * reset_all:
> +     *
> +     * Cold reset of a bus with children.  The default implementation of this
> +     * method will invoke BusState::reset and then recursively call
> +     * qdev_reset_all() on each child in an arbitrary order.
> +     */
> +    void (*reset_all)(BusState *bus);
>  };
>  
>  typedef struct BusChild {
> diff --git a/hw/qdev.c b/hw/qdev.c
> index e02b5be..db19b14 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -234,11 +234,17 @@ void qdev_reset_all(DeviceState *dev)
>      dc->reset_all(dev);
>  }
>  
> -void qbus_reset_all(BusState *bus)
> +static void qbus_reset_children(BusState *bus)
>  {
>      qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
>  }
>  
> +void qbus_reset_all(BusState *bus)
> +{
> +    BusClass *bc = BUS_GET_CLASS(bus);
> +    bc->reset_all(bus);
> +}
> +
>  void qbus_reset_all_fn(void *opaque)
>  {
>      BusState *bus = opaque;
> @@ -758,6 +764,13 @@ static const TypeInfo device_type_info = {
>      .class_size = sizeof(DeviceClass),
>  };
>  
> +static void qbus_class_initfn(ObjectClass *klass, void *data)
> +{
> +    BusClass *bc = BUS_CLASS(klass);
> +
> +    bc->reset_all = qbus_reset_children;
> +}
> +
>  static void qbus_initfn(Object *obj)
>  {
>      BusState *bus = BUS(obj);
> @@ -789,6 +802,7 @@ static const TypeInfo bus_info = {
>      .parent = TYPE_OBJECT,
>      .instance_size = sizeof(BusState),
>      .abstract = true,
> +    .class_init = qbus_class_initfn,
>      .class_size = sizeof(BusClass),
>      .instance_init = qbus_initfn,
>      .instance_finalize = qbus_finalize,
> 

Once you do this, the return value of bus->reset should become void, and
classes that used to return 1 should instead override reset_all.  Can
you do that too?

Paolo

Re: [Qemu-devel] [PATCH 2/2] qbus: make bus reset overrideable by subclasses

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 11/01/2013 15:17, Anthony Liguori ha scritto:
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>>  hw/qdev-core.h | 15 +++++++++++++++
>>  hw/qdev.c      | 16 +++++++++++++++-
>>  2 files changed, 30 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/qdev-core.h b/hw/qdev-core.h
>> index f40fd15..453a061 100644
>> --- a/hw/qdev-core.h
>> +++ b/hw/qdev-core.h
>> @@ -100,7 +100,22 @@ struct BusClass {
>>       * bindings can be found at http://playground.sun.com/1275/bindings/.
>>       */
>>      char *(*get_fw_dev_path)(DeviceState *dev);
>> +
>> +    /**
>> +     * reset:
>> +     *
>> +     * Cold reset of a bus. This only resets the controller state of the bus.
>> +     */
>>      int (*reset)(BusState *bus);
>> +
>> +    /**
>> +     * reset_all:
>> +     *
>> +     * Cold reset of a bus with children.  The default implementation of this
>> +     * method will invoke BusState::reset and then recursively call
>> +     * qdev_reset_all() on each child in an arbitrary order.
>> +     */
>> +    void (*reset_all)(BusState *bus);
>>  };
>>  
>>  typedef struct BusChild {
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index e02b5be..db19b14 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -234,11 +234,17 @@ void qdev_reset_all(DeviceState *dev)
>>      dc->reset_all(dev);
>>  }
>>  
>> -void qbus_reset_all(BusState *bus)
>> +static void qbus_reset_children(BusState *bus)
>>  {
>>      qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
>>  }
>>  
>> +void qbus_reset_all(BusState *bus)
>> +{
>> +    BusClass *bc = BUS_GET_CLASS(bus);
>> +    bc->reset_all(bus);
>> +}
>> +
>>  void qbus_reset_all_fn(void *opaque)
>>  {
>>      BusState *bus = opaque;
>> @@ -758,6 +764,13 @@ static const TypeInfo device_type_info = {
>>      .class_size = sizeof(DeviceClass),
>>  };
>>  
>> +static void qbus_class_initfn(ObjectClass *klass, void *data)
>> +{
>> +    BusClass *bc = BUS_CLASS(klass);
>> +
>> +    bc->reset_all = qbus_reset_children;
>> +}
>> +
>>  static void qbus_initfn(Object *obj)
>>  {
>>      BusState *bus = BUS(obj);
>> @@ -789,6 +802,7 @@ static const TypeInfo bus_info = {
>>      .parent = TYPE_OBJECT,
>>      .instance_size = sizeof(BusState),
>>      .abstract = true,
>> +    .class_init = qbus_class_initfn,
>>      .class_size = sizeof(BusClass),
>>      .instance_init = qbus_initfn,
>>      .instance_finalize = qbus_finalize,
>> 
>
> Once you do this, the return value of bus->reset should become void, and
> classes that used to return 1 should instead override reset_all.  Can
> you do that too?

Sure.

Regards,

Anthony Liguori

>
> Paolo

Re: [Qemu-devel] [PATCH 1/2] qdev: make reset propagation overrideable by subclasses

[Andreas Färber]

Am 11.01.2013 15:17, schrieb Anthony Liguori:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/qdev-core.h | 16 +++++++++++++++-
>  hw/qdev.c      | 11 ++++++++++-
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/qdev-core.h b/hw/qdev-core.h
> index 853bd08..f40fd15 100644
> --- a/hw/qdev-core.h
> +++ b/hw/qdev-core.h
> @@ -36,9 +36,23 @@ typedef struct DeviceClass {
>      Property *props;
>      int no_user;
>  
> -    /* callbacks */
> +    /**
> +     * reset:
> +     *
> +     * Cold reset of a device. This function must be implemented by a device's
> +     * that never have children busses.
> +     */
>      void (*reset)(DeviceState *dev);
>  
> +    /**
> +     * reset_all:
> +     *
> +     * Cold reset of a device with children.  The default implementation of this
> +     * method will invoke DeviceClass::reset and then recursively call
> +     * qbus_reset_all() on each child in an arbitrary order.
> +     */
> +    void (*reset_all)(DeviceState *dev);

This documentation is bogus, it should go into DeviceClass as @reset and
@reset_all since these are struct fields, not standalone functions.

This also happens to conflict with the realize/unrealize callbacks being
added, v3 adding DeviceClass documentation on Eduardo's request. Can you
please apply mine first?

No objections to adding such a second reset callback. Maybe add a
DeviceReset typedef to facilitate overriding these in subclasses?

Regards,
Andreas

> +
>      /* device state */
>      const struct VMStateDescription *vmsd;
>  
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 1b68d02..e02b5be 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -223,11 +223,17 @@ static int qbus_reset_one(BusState *bus, void *opaque)
>      return 0;
>  }
>  
> -void qdev_reset_all(DeviceState *dev)
> +static void qdev_reset_children(DeviceState *dev)
>  {
>      qdev_walk_children(dev, qdev_reset_one, qbus_reset_one, NULL);
>  }
>  
> +void qdev_reset_all(DeviceState *dev)
> +{
> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
> +    dc->reset_all(dev);
> +}
> +
>  void qbus_reset_all(BusState *bus)
>  {
>      qbus_walk_children(bus, qdev_reset_one, qbus_reset_one, NULL);
> @@ -714,7 +720,10 @@ static void device_unparent(Object *obj)
>  
>  static void device_class_init(ObjectClass *class, void *data)
>  {
> +    DeviceClass *dc = DEVICE_CLASS(class);
> +
>      class->unparent = device_unparent;
> +    dc->reset_all = qdev_reset_children;
>  }
>  
>  void device_reset(DeviceState *dev)

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