Re: [Qemu-devel] [PATCH v3 04/33] make Device and Bus Resettable

[David Gibson]

On Mon, Jul 29, 2019 at 04:56:25PM +0200, Damien Hedde wrote:
> This add Resettable interface implementation for both Bus and Device.
> 
> *resetting* counter and *reset_is_cold* flag are added in DeviceState
> and BusState.
> 
> Compatibility with existing code base is ensured.
> The legacy bus or device reset method is called in the new exit phase
> and the other 2 phases are let empty. Using the exit phase guarantee that
> legacy resets are called in the "post" order (ie: children then parent)
> in hierarchical reset. That is the same order as legacy qdev_reset_all
> or qbus_reset_all were using.
> 
> New *device_reset* and *bus_reset* function are proposed with an
> additional boolean argument telling whether the reset is cold or warm.
> Helper functions *device_reset_[warm|cold]* and *bus_reset_[warm|cold]*
> are defined also as helpers.
> 
> Also add a [device|bus]_is_resetting and [device|bus]_is_reset_cold
> functions telling respectively whether the object is currently under reset and
> if the current reset is cold or not.
> 
> Signed-off-by: Damien Hedde <address@hidden>
> ---
>  hw/core/bus.c          | 85 ++++++++++++++++++++++++++++++++++++++++++
>  hw/core/qdev.c         | 82 ++++++++++++++++++++++++++++++++++++++++
>  include/hw/qdev-core.h | 84 ++++++++++++++++++++++++++++++++++++++---
>  tests/Makefile.include |  1 +
>  4 files changed, 247 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/core/bus.c b/hw/core/bus.c
> index 17bc1edcde..08a97addb6 100644
> --- a/hw/core/bus.c
> +++ b/hw/core/bus.c
> @@ -22,6 +22,7 @@
>  #include "qemu/module.h"
>  #include "hw/qdev.h"
>  #include "qapi/error.h"
> +#include "hw/resettable.h"
>  
>  void qbus_set_hotplug_handler(BusState *bus, Object *handler, Error **errp)
>  {
> @@ -68,6 +69,75 @@ int qbus_walk_children(BusState *bus,
>      return 0;
>  }
>  
> +void bus_reset(BusState *bus, bool cold)
> +{
> +    resettable_reset(OBJECT(bus), cold);
> +}
> +
> +bool bus_is_resetting(BusState *bus)
> +{
> +    return (bus->resetting != 0);
> +}
> +
> +bool bus_is_reset_cold(BusState *bus)
> +{
> +    return bus->reset_is_cold;
> +}
> +
> +static uint32_t bus_get_reset_count(Object *obj)
> +{
> +    BusState *bus = BUS(obj);
> +    return bus->resetting;
> +}
> +
> +static uint32_t bus_increment_reset_count(Object *obj)
> +{
> +    BusState *bus = BUS(obj);
> +    return ++bus->resetting;
> +}
> +
> +static uint32_t bus_decrement_reset_count(Object *obj)
> +{
> +    BusState *bus = BUS(obj);
> +    return --bus->resetting;
> +}
> +
> +static bool bus_set_reset_cold(Object *obj, bool cold)
> +{
> +    BusState *bus = BUS(obj);
> +    bool old = bus->reset_is_cold;
> +    bus->reset_is_cold = cold;
> +    return old;
> +}
> +
> +static bool bus_set_hold_needed(Object *obj, bool hold_needed)
> +{
> +    BusState *bus = BUS(obj);
> +    bool old = bus->reset_hold_needed;
> +    bus->reset_hold_needed = hold_needed;
> +    return old;
> +}
> +
> +static void bus_foreach_reset_child(Object *obj, void (*func)(Object *))
> +{
> +    BusState *bus = BUS(obj);
> +    BusChild *kid;
> +
> +    QTAILQ_FOREACH(kid, &bus->children, sibling) {
> +        func(OBJECT(kid->child));
> +    }
> +}

IIUC, every resettable class would need more or less identical
implementations of the above.  That seems like an awful lot of
boilerplate.

> +
> +static void bus_obj_legacy_reset(Object *obj)
> +{
> +    BusState *bus = BUS(obj);
> +    BusClass *bc = BUS_GET_CLASS(obj);
> +
> +    if (bc->reset) {
> +        bc->reset(bus);
> +    }
> +}
> +
>  static void qbus_realize(BusState *bus, DeviceState *parent, const char 
> *name)
>  {
>      const char *typename = object_get_typename(OBJECT(bus));
> @@ -192,6 +262,8 @@ static void qbus_initfn(Object *obj)
>                               NULL);
>      object_property_add_bool(obj, "realized",
>                               bus_get_realized, bus_set_realized, NULL);
> +
> +    bus->resetting = 0;
>  }
>  
>  static char *default_bus_get_fw_dev_path(DeviceState *dev)
> @@ -202,9 +274,18 @@ static char *default_bus_get_fw_dev_path(DeviceState 
> *dev)
>  static void bus_class_init(ObjectClass *class, void *data)
>  {
>      BusClass *bc = BUS_CLASS(class);
> +    ResettableClass *rc = RESETTABLE_CLASS(class);
>  
>      class->unparent = bus_unparent;
>      bc->get_fw_dev_path = default_bus_get_fw_dev_path;
> +
> +    rc->phases.exit = bus_obj_legacy_reset;
> +    rc->get_count = bus_get_reset_count;
> +    rc->increment_count = bus_increment_reset_count;
> +    rc->decrement_count = bus_decrement_reset_count;
> +    rc->foreach_child = bus_foreach_reset_child;
> +    rc->set_cold = bus_set_reset_cold;
> +    rc->set_hold_needed = bus_set_hold_needed;
>  }
>  
>  static void qbus_finalize(Object *obj)
> @@ -223,6 +304,10 @@ static const TypeInfo bus_info = {
>      .instance_init = qbus_initfn,
>      .instance_finalize = qbus_finalize,
>      .class_init = bus_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_RESETTABLE },
> +        { }
> +    },
>  };
>  
>  static void bus_register_types(void)
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 043e058396..559ced070d 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -254,6 +254,64 @@ HotplugHandler *qdev_get_hotplug_handler(DeviceState 
> *dev)
>      return hotplug_ctrl;
>  }
>  
> +void device_reset(DeviceState *dev, bool cold)
> +{
> +    resettable_reset(OBJECT(dev), cold);
> +}
> +
> +bool device_is_resetting(DeviceState *dev)
> +{
> +    return (dev->resetting != 0);
> +}
> +
> +bool device_is_reset_cold(DeviceState *dev)
> +{
> +    return dev->reset_is_cold;
> +}
> +
> +static uint32_t device_get_reset_count(Object *obj)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    return dev->resetting;
> +}
> +
> +static uint32_t device_increment_reset_count(Object *obj)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    return ++dev->resetting;
> +}
> +
> +static uint32_t device_decrement_reset_count(Object *obj)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    return --dev->resetting;
> +}
> +
> +static bool device_set_reset_cold(Object *obj, bool cold)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    bool old = dev->reset_is_cold;
> +    dev->reset_is_cold = cold;
> +    return old;
> +}
> +
> +static bool device_set_hold_needed(Object *obj, bool hold_needed)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    bool old = dev->reset_hold_needed;
> +    dev->reset_hold_needed = hold_needed;
> +    return old;
> +}
> +
> +static void device_foreach_reset_child(Object *obj, void (*func)(Object *))
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    BusState *bus;
> +    QLIST_FOREACH(bus, &dev->child_bus, sibling) {
> +        func(OBJECT(bus));
> +    }
> +}
> +
>  static int qdev_reset_one(DeviceState *dev, void *opaque)
>  {
>      device_legacy_reset(dev);
> @@ -954,6 +1012,7 @@ static void device_initfn(Object *obj)
>  
>      dev->instance_id_alias = -1;
>      dev->realized = false;
> +    dev->resetting = 0;
>  
>      object_property_add_bool(obj, "realized",
>                               device_get_realized, device_set_realized, NULL);
> @@ -1046,9 +1105,20 @@ static void device_unparent(Object *obj)
>      }
>  }
>  
> +static void device_obj_legacy_reset(Object *obj)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
> +
> +    if (dc->reset) {
> +        dc->reset(dev);
> +    }
> +}
> +
>  static void device_class_init(ObjectClass *class, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(class);
> +    ResettableClass *rc = RESETTABLE_CLASS(class);
>  
>      class->unparent = device_unparent;
>  
> @@ -1060,6 +1130,14 @@ static void device_class_init(ObjectClass *class, void 
> *data)
>       */
>      dc->hotpluggable = true;
>      dc->user_creatable = true;
> +
> +    rc->phases.exit = device_obj_legacy_reset;
> +    rc->get_count = device_get_reset_count;
> +    rc->increment_count = device_increment_reset_count;
> +    rc->decrement_count = device_decrement_reset_count;
> +    rc->foreach_child = device_foreach_reset_child;
> +    rc->set_cold = device_set_reset_cold;
> +    rc->set_hold_needed = device_set_hold_needed;
>  }
>  
>  void device_class_set_parent_reset(DeviceClass *dc,
> @@ -1117,6 +1195,10 @@ static const TypeInfo device_type_info = {
>      .class_init = device_class_init,
>      .abstract = true,
>      .class_size = sizeof(DeviceClass),
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_RESETTABLE },
> +        { }
> +    },
>  };
>  
>  static void qdev_register_types(void)
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 690ce72433..eeb75611c8 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -7,6 +7,7 @@
>  #include "hw/irq.h"
>  #include "hw/hotplug.h"
>  #include "sysemu/sysemu.h"
> +#include "hw/resettable.h"
>  
>  enum {
>      DEV_NVECTORS_UNSPECIFIED = -1,
> @@ -132,6 +133,10 @@ struct NamedGPIOList {
>  /**
>   * DeviceState:
>   * @realized: Indicates whether the device has been fully constructed.
> + * @resetting: Indicates whether the device is under reset. Also
> + * used to count how many times reset has been initiated on the device.
> + * @reset_is_cold: If the device is under reset, indicates whether it is cold
> + * or warm.
>   *
>   * This structure should not be accessed directly.  We declare it here
>   * so that it can be embedded in individual device state structures.
> @@ -153,6 +158,9 @@ struct DeviceState {
>      int num_child_bus;
>      int instance_id_alias;
>      int alias_required_for_version;
> +    uint32_t resetting;
> +    bool reset_is_cold;
> +    bool reset_hold_needed;
>  };
>  
>  struct DeviceListener {
> @@ -199,6 +207,10 @@ typedef struct BusChild {
>  /**
>   * BusState:
>   * @hotplug_handler: link to a hotplug handler associated with bus.
> + * @resetting: Indicates whether the bus is under reset. Also
> + * used to count how many times reset has been initiated on the bus.
> + * @reset_is_cold: If the bus is under reset, indicates whether it is cold
> + * or warm.
>   */
>  struct BusState {
>      Object obj;
> @@ -210,6 +222,9 @@ struct BusState {
>      int num_children;
>      QTAILQ_HEAD(, BusChild) children;
>      QLIST_ENTRY(BusState) sibling;
> +    uint32_t resetting;
> +    bool reset_is_cold;
> +    bool reset_hold_needed;
>  };
>  
>  /**
> @@ -376,6 +391,70 @@ int qdev_walk_children(DeviceState *dev,
>                         qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
>                         void *opaque);
>  
> +/**
> + * device_reset:
> + * Resets the device @dev, @cold tell whether to do a cold or warm reset.
> + * Uses the ressetable interface.
> + * Base behavior is to reset the device and its qdev/qbus subtree.
> + */
> +void device_reset(DeviceState *dev, bool cold);
> +
> +static inline void device_reset_warm(DeviceState *dev)
> +{
> +    device_reset(dev, false);
> +}
> +
> +static inline void device_reset_cold(DeviceState *dev)
> +{
> +    device_reset(dev, true);
> +}
> +
> +/**
> + * bus_reset:
> + * Resets the bus @bus, @cold tell whether to do a cold or warm reset.
> + * Uses the ressetable interface.
> + * Base behavior is to reset the bus and its qdev/qbus subtree.
> + */
> +void bus_reset(BusState *bus, bool cold);
> +
> +static inline void bus_reset_warm(BusState *bus)
> +{
> +    bus_reset(bus, false);
> +}
> +
> +static inline void bus_reset_cold(BusState *bus)
> +{
> +    bus_reset(bus, true);
> +}
> +
> +/**
> + * device_is_resetting:
> + * Tell whether the device @dev is currently under reset.
> + */
> +bool device_is_resetting(DeviceState *dev);
> +
> +/**
> + * device_is_reset_cold:
> + * Tell whether the device @dev is currently under reset cold or warm reset.
> + *
> + * Note: only valid when device_is_resetting returns true.
> + */
> +bool device_is_reset_cold(DeviceState *dev);
> +
> +/**
> + * bus_is_resetting:
> + * Tell whether the bus @bus is currently under reset.
> + */
> +bool bus_is_resetting(BusState *bus);
> +
> +/**
> + * bus_is_reset_cold:
> + * Tell whether the bus @bus is currently under reset cold or warm reset.
> + *
> + * Note: only valid when bus_is_resetting returns true.
> + */
> +bool bus_is_reset_cold(BusState *bus);
> +
>  void qdev_reset_all(DeviceState *dev);
>  void qdev_reset_all_fn(void *opaque);
>  
> @@ -413,11 +492,6 @@ void qdev_machine_init(void);
>   */
>  void device_legacy_reset(DeviceState *dev);
>  
> -static inline void device_reset(DeviceState *dev)
> -{
> -    device_legacy_reset(dev);
> -}
> -
>  void device_class_set_parent_reset(DeviceClass *dc,
>                                     DeviceReset dev_reset,
>                                     DeviceReset *parent_reset);
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index fd7fdb8658..1c0a5345b9 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -561,6 +561,7 @@ tests/fp/%:
>  
>  tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \
>       hw/core/qdev.o hw/core/qdev-properties.o hw/core/hotplug.o\
> +     hw/core/resettable.o \
>       hw/core/bus.o \
>       hw/core/irq.o \
>       hw/core/fw-path-provider.o \

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature