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.he...@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
signature.asc
Description: PGP signature
