Re: [qemu-s390x] [PATCH v3 04/33] make Device and Bus Resettable

[Damien Hedde]

On 7/31/19 7:56 AM, David Gibson wrote:
> 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.

Do you mean the get/increment_count/decrement_count, set_cold/hold part ?
True, but it's limited to the base classes.
Since Resettable is an interface, we have no state there to store what
we need. Only alternative is to have some kind of single
get_resettable_state method returning a pointer to the state (allowing
us to keep the functions in the interface code).
Beyond Device and Bus, which are done here, there is probably not so
many class candidates for the Resettable interface.

Damien