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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4703 bytes --]

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 <damien.hedde@greensocs.com>
> >> ---
> >>  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.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]