Re: [PATCH v7 04/11] hw/core: add Resettable support to BusClass and DeviceClass

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 1/15/20 1:36 PM, Damien Hedde wrote:
> This commit adds support of Resettable interface to buses and devices:
> + ResettableState structure is added in the Bus/Device state
> + Resettable methods are implemented.
> + device/bus_is_in_reset function defined
> 
> This commit allows to transition the objects to the new
> multi-phase interface without changing the reset behavior at all.
> Object single reset method can be split into the 3 different phases
> but the 3 phases are still executed in a row for a given object.
>  From the qdev/qbus reset api point of view, nothing is changed.
> qdev_reset_all() and qbus_reset_all() are not modified as well as
> device_legacy_reset().
> 
> Transition of an object must be done from parent class to child class.
> Care has been taken to allow the transition of a parent class
> without requiring the child classes to be transitioned at the same
> time. Note that SysBus and SysBusDevice class do not need any transition
> because they do not override the legacy reset method.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   include/hw/qdev-core.h | 27 ++++++++++++
>   hw/core/bus.c          | 97 ++++++++++++++++++++++++++++++++++++++++++
>   hw/core/qdev.c         | 93 ++++++++++++++++++++++++++++++++++++++++
>   tests/Makefile.include |  1 +
>   4 files changed, 218 insertions(+)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index b8341b0fb0..1b4b420617 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -5,6 +5,7 @@
>   #include "qemu/bitmap.h"
>   #include "qom/object.h"
>   #include "hw/hotplug.h"
> +#include "hw/resettable.h"
>   
>   enum {
>       DEV_NVECTORS_UNSPECIFIED = -1,
> @@ -117,6 +118,11 @@ typedef struct DeviceClass {
>       bool hotpluggable;
>   
>       /* callbacks */
> +    /*
> +     * Reset method here is deprecated and replaced by methods in the
> +     * resettable class interface to implement a multi-phase reset.
> +     * TODO: remove once every reset callback is unused
> +     */
>       DeviceReset reset;
>       DeviceRealize realize;
>       DeviceUnrealize unrealize;
> @@ -141,6 +147,7 @@ struct NamedGPIOList {
>   /**
>    * DeviceState:
>    * @realized: Indicates whether the device has been fully constructed.
> + * @reset: ResettableState for the device; handled by Resettable interface.
>    *
>    * This structure should not be accessed directly.  We declare it here
>    * so that it can be embedded in individual device state structures.
> @@ -163,6 +170,7 @@ struct DeviceState {
>       int num_child_bus;
>       int instance_id_alias;
>       int alias_required_for_version;
> +    ResettableState reset;
>   };
>   
>   struct DeviceListener {
> @@ -215,6 +223,7 @@ typedef struct BusChild {
>   /**
>    * BusState:
>    * @hotplug_handler: link to a hotplug handler associated with bus.
> + * @reset: ResettableState for the bus; handled by Resettable interface.
>    */
>   struct BusState {
>       Object obj;
> @@ -226,6 +235,7 @@ struct BusState {
>       int num_children;
>       QTAILQ_HEAD(, BusChild) children;
>       QLIST_ENTRY(BusState) sibling;
> +    ResettableState reset;
>   };
>   
>   /**
> @@ -412,6 +422,18 @@ void qdev_reset_all_fn(void *opaque);
>   void qbus_reset_all(BusState *bus);
>   void qbus_reset_all_fn(void *opaque);
>   
> +/**
> + * device_is_in_reset:
> + * Return true if the device @dev is currently being reset.
> + */
> +bool device_is_in_reset(DeviceState *dev);
> +
> +/**
> + * bus_is_in_reset:
> + * Return true if the bus @bus is currently being reset.
> + */
> +bool bus_is_in_reset(BusState *bus);
> +
>   /* This should go away once we get rid of the NULL bus hack */
>   BusState *sysbus_get_default(void);
>   
> @@ -433,6 +455,11 @@ void qdev_machine_init(void);
>    */
>   void device_legacy_reset(DeviceState *dev);
>   
> +/**
> + * device_class_set_parent_reset:
> + * TODO: remove the function when DeviceClass's reset method
> + * is not used anymore.
> + */
>   void device_class_set_parent_reset(DeviceClass *dc,
>                                      DeviceReset dev_reset,
>                                      DeviceReset *parent_reset);
> diff --git a/hw/core/bus.c b/hw/core/bus.c
> index 7f3d2a3dbd..2698f715bd 100644
> --- a/hw/core/bus.c
> +++ b/hw/core/bus.c
> @@ -68,6 +68,28 @@ int qbus_walk_children(BusState *bus,
>       return 0;
>   }
>   
> +bool bus_is_in_reset(BusState *bus)
> +{
> +    return resettable_is_in_reset(OBJECT(bus));
> +}
> +
> +static ResettableState *bus_get_reset_state(Object *obj)
> +{
> +    BusState *bus = BUS(obj);
> +    return &bus->reset;
> +}
> +
> +static void bus_reset_child_foreach(Object *obj, ResettableChildCallback cb,
> +                                    void *opaque, ResetType type)
> +{
> +    BusState *bus = BUS(obj);
> +    BusChild *kid;
> +
> +    QTAILQ_FOREACH(kid, &bus->children, sibling) {
> +        cb(OBJECT(kid->child), opaque, type);
> +    }
> +}
> +
>   static void qbus_realize(BusState *bus, DeviceState *parent, const char *name)
>   {
>       const char *typename = object_get_typename(OBJECT(bus));
> @@ -199,12 +221,83 @@ static char *default_bus_get_fw_dev_path(DeviceState *dev)
>       return g_strdup(object_get_typename(OBJECT(dev)));
>   }
>   
> +/**
> + * bus_phases_reset:
> + * Transition reset method for buses to allow moving
> + * smoothly from legacy reset method to multi-phases
> + */
> +static void bus_phases_reset(BusState *bus)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(bus);
> +
> +    if (rc->phases.enter) {
> +        rc->phases.enter(OBJECT(bus), RESET_TYPE_COLD);
> +    }
> +    if (rc->phases.hold) {
> +        rc->phases.hold(OBJECT(bus));
> +    }
> +    if (rc->phases.exit) {
> +        rc->phases.exit(OBJECT(bus));
> +    }
> +}
> +
> +static void bus_transitional_reset(Object *obj)
> +{
> +    BusClass *bc = BUS_GET_CLASS(obj);
> +
> +    /*
> +     * This will call either @bus_phases_reset (for multi-phases transitioned
> +     * buses) or a bus's specific method for not-yet transitioned buses.
> +     * In both case, it does not reset children.
> +     */
> +    if (bc->reset) {
> +        bc->reset(BUS(obj));
> +    }
> +}
> +
> +/**
> + * bus_get_transitional_reset:
> + * check if the bus's class is ready for multi-phase
> + */
> +static ResettableTrFunction bus_get_transitional_reset(Object *obj)
> +{
> +    BusClass *dc = BUS_GET_CLASS(obj);
> +    if (dc->reset != bus_phases_reset) {
> +        /*
> +         * dc->reset has been overridden by a subclass,
> +         * the bus is not ready for multi phase yet.
> +         */
> +        return bus_transitional_reset;
> +    }
> +    return NULL;
> +}
> +
>   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->get_state = bus_get_reset_state;
> +    rc->child_foreach = bus_reset_child_foreach;
> +
> +    /*
> +     * @bus_phases_reset is put as the default reset method below, allowing
> +     * to do the multi-phase transition from base classes to leaf classes. It
> +     * allows a legacy-reset Bus class to extend a multi-phases-reset
> +     * Bus class for the following reason:
> +     * + If a base class B has been moved to multi-phase, then it does not
> +     *   override this default reset method and may have defined phase methods.
> +     * + A child class C (extending class B) which uses
> +     *   bus_class_set_parent_reset() (or similar means) to override the
> +     *   reset method will still work as expected. @bus_phases_reset function
> +     *   will be registered as the parent reset method and effectively call
> +     *   parent reset phases.
> +     */
> +    bc->reset = bus_phases_reset;
> +    rc->get_transitional_function = bus_get_transitional_reset;
>   }
>   
>   static void qbus_finalize(Object *obj)
> @@ -223,6 +316,10 @@ static const TypeInfo bus_info = {
>       .instance_init = qbus_initfn,
>       .instance_finalize = qbus_finalize,
>       .class_init = bus_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_RESETTABLE_INTERFACE },
> +        { }
> +    },
>   };
>   
>   static void bus_register_types(void)
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 5cb03136b5..d431a020fc 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -355,6 +355,28 @@ void qbus_reset_all_fn(void *opaque)
>       qbus_reset_all(bus);
>   }
>   
> +bool device_is_in_reset(DeviceState *dev)
> +{
> +    return resettable_is_in_reset(OBJECT(dev));
> +}
> +
> +static ResettableState *device_get_reset_state(Object *obj)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    return &dev->reset;
> +}
> +
> +static void device_reset_child_foreach(Object *obj, ResettableChildCallback cb,
> +                                       void *opaque, ResetType type)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    BusState *bus;
> +
> +    QLIST_FOREACH(bus, &dev->child_bus, sibling) {
> +        cb(OBJECT(bus), opaque, type);
> +    }
> +}
> +
>   /* can be used as ->unplug() callback for the simple cases */
>   void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
>                                     DeviceState *dev, Error **errp)
> @@ -1107,10 +1129,62 @@ device_vmstate_if_get_id(VMStateIf *obj)
>       return qdev_get_dev_path(dev);
>   }
>   
> +/**
> + * device_phases_reset:
> + * Transition reset method for devices to allow moving
> + * smoothly from legacy reset method to multi-phases
> + */
> +static void device_phases_reset(DeviceState *dev)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(dev);
> +
> +    if (rc->phases.enter) {
> +        rc->phases.enter(OBJECT(dev), RESET_TYPE_COLD);
> +    }
> +    if (rc->phases.hold) {
> +        rc->phases.hold(OBJECT(dev));
> +    }
> +    if (rc->phases.exit) {
> +        rc->phases.exit(OBJECT(dev));
> +    }
> +}
> +
> +static void device_transitional_reset(Object *obj)
> +{
> +    DeviceClass *dc = DEVICE_GET_CLASS(obj);
> +
> +    /*
> +     * This will call either @device_phases_reset (for multi-phases transitioned
> +     * devices) or a device's specific method for not-yet transitioned devices.
> +     * In both case, it does not reset children.
> +     */
> +    if (dc->reset) {
> +        dc->reset(DEVICE(obj));
> +    }
> +}
> +
> +/**
> + * device_get_transitional_reset:
> + * check if the device's class is ready for multi-phase
> + */
> +static ResettableTrFunction device_get_transitional_reset(Object *obj)
> +{
> +    DeviceClass *dc = DEVICE_GET_CLASS(obj);
> +    if (dc->reset != device_phases_reset) {
> +        /*
> +         * dc->reset has been overridden by a subclass,
> +         * the device is not ready for multi phase yet.
> +         */
> +        return device_transitional_reset;
> +    }
> +    return NULL;
> +}
> +
>   static void device_class_init(ObjectClass *class, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(class);
>       VMStateIfClass *vc = VMSTATE_IF_CLASS(class);
> +    ResettableClass *rc = RESETTABLE_CLASS(class);
>   
>       class->unparent = device_unparent;
>   
> @@ -1123,6 +1197,24 @@ static void device_class_init(ObjectClass *class, void *data)
>       dc->hotpluggable = true;
>       dc->user_creatable = true;
>       vc->get_id = device_vmstate_if_get_id;
> +    rc->get_state = device_get_reset_state;
> +    rc->child_foreach = device_reset_child_foreach;
> +
> +    /*
> +     * @device_phases_reset is put as the default reset method below, allowing
> +     * to do the multi-phase transition from base classes to leaf classes. It
> +     * allows a legacy-reset Device class to extend a multi-phases-reset
> +     * Device class for the following reason:
> +     * + If a base class B has been moved to multi-phase, then it does not
> +     *   override this default reset method and may have defined phase methods.
> +     * + A child class C (extending class B) which uses
> +     *   device_class_set_parent_reset() (or similar means) to override the
> +     *   reset method will still work as expected. @device_phases_reset function
> +     *   will be registered as the parent reset method and effectively call
> +     *   parent reset phases.
> +     */
> +    dc->reset = device_phases_reset;
> +    rc->get_transitional_function = device_get_transitional_reset;
>   }
>   
>   void device_class_set_parent_reset(DeviceClass *dc,
> @@ -1183,6 +1275,7 @@ static const TypeInfo device_type_info = {
>       .class_size = sizeof(DeviceClass),
>       .interfaces = (InterfaceInfo[]) {
>           { TYPE_VMSTATE_IF },
> +        { TYPE_RESETTABLE_INTERFACE },
>           { }
>       }
>   };
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 1ae14a8b15..8ad07621b0 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -427,6 +427,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/bus.o \
> +	hw/core/resettable.o \
>   	hw/core/irq.o \
>   	hw/core/fw-path-provider.o \
>   	hw/core/reset.o \
>