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

[Damien Hedde]

On 8/6/19 2:35 AM, David Gibson wrote:
> On Wed, Jul 31, 2019 at 11:09:05AM +0200, Damien Hedde wrote:
>>
>>
>> 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.
> 
> Right.  I can't immediately see a better way to handle this, but it
> still seems like a mild code smell.
> 

Only definitive solution to this would be to make DeviceClass and
BusClass inherit from a common Resettable object.

Would it be better if I use a common struct (eg: ResettableState)
holding the different fields ?
Then I can have a single implementation of the method and do for example:
static uint32_t bus_decrement_reset_count(Object *obj)
{
     return decrement_reset_count(&(BUS(obj))->reset_state);
}
static uint32_t device_decrement_reset_count(Object *obj)
{
     return decrement_reset_count(&(DEV(dev))->reset_state);
}

I can also merge the 3 count related method into one if it helps.

--
Damien