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: 5758 bytes --]

On Wed, Aug 07, 2019 at 09:55:13AM +0200, Damien Hedde wrote:
> 
> 
> 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 <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.
> > 
> 
> 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 ?

Maybe, yeah.

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



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