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

[Damien Hedde]

On 8/7/19 4:41 PM, Peter Maydell wrote:
> On Mon, 29 Jul 2019 at 15:58, Damien Hedde <address@hidden> 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
> 
> "left". "guarantees"
> 
>> 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.
> 
> This is true, but on the other hand the semantics of most device
> reset methods match "init", not "exit" -- they just set device
> internal fields to the correct reset state.

I changed from "init" to "exit" due to the change of the init phase call
order to parent-then-children.

This is a consequence of what I found about the raspi reset: it changes
the reset hierarchy during reset. The only way I saw to have a chance
allowing this kind of things cleanly is: do parent init first so that it
can setup its children before they are considered for reset.

I can put the legacy reset method to the hold phase which is part of the
"enter reset state". Otherwise I need to change back the order of init
phase.

My other concern with putting it in init phase is that some device do
things we forbid in it (like setting irq).


> 
>> 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.
> 
> I was expecting this patch to contain handling for migration
> of the new data fields. That's in a later patch, so the
> commit message here should say something like:
> 
> "This commit adds new fields to BusState and DeviceState, but
> does not set up migration handling for them; so the new functions
> can only be called after the subsequent commit which adds the
> migration support."
> 
>> 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));
>> +    }
>> +}
>> +
>> +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.
> 
> We should put a doc comment entry for @reset_hold_needed here too.
> 
>>   *
>>   * 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.
> 
> @reset_hold_needed missing.
> 
>>   */
>>  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.
> 
> "Resets the device @dev. @cold is true if this is a cold reset."
> 
> 
>> + * Uses the ressetable interface.
> 
> "Resettable".
> 
>> + * Base behavior is to reset the device and its qdev/qbus subtree.
> 
> What do you mean by "base behavior" here? When would this ever
> do anything else?
> 
>> + */
>> +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.
> 
> Same remarks as for device_reset above.
> 
>> + */
>> +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.
> 
> "Return true if the device @dev is currently being reset."
> 
>> + */
>> +bool device_is_resetting(DeviceState *dev);
>> +
>> +/**
>> + * device_is_reset_cold:
>> + * Tell whether the device @dev is currently under reset cold or warm reset.
> 
> "Return true if the reset currently in progress for @dev is
> a cold 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.
> 
> [similar rephrasings as above for 'tell whether']
> 
>> + */
>> +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 \
>> --
>> 2.22.0
> 
> thanks
> -- PMM
>