Re: [PATCH v5 02/13] hw/core/qdev: add trace events to help with resettable transition

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 11/4/19 1:16 PM, Damien Hedde wrote:
> On 11/1/19 12:23 AM, Philippe Mathieu-Daudé wrote:
>> On 10/18/19 5:06 PM, Damien Hedde wrote:
>>> Adds trace events to reset procedure and when updating the parent
>>> bus of a device.
>>>
>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>> ---
>>>    hw/core/qdev.c       | 27 ++++++++++++++++++++++++---
>>>    hw/core/trace-events |  9 +++++++++
>>>    2 files changed, 33 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>> index 60be2f2fef..f230063189 100644
>>> --- a/hw/core/qdev.c
>>> +++ b/hw/core/qdev.c
>>> @@ -38,6 +38,7 @@
>>>    #include "hw/boards.h"
>>>    #include "hw/sysbus.h"
>>>    #include "migration/vmstate.h"
>>> +#include "trace.h"
>>>      bool qdev_hotplug = false;
>>>    static bool qdev_hot_added = false;
>>> @@ -98,7 +99,9 @@ void qdev_set_parent_bus(DeviceState *dev, BusState
>>> *bus)
>>>        bool replugging = dev->parent_bus != NULL;
>>>          if (replugging) {
>>> -        /* Keep a reference to the device while it's not plugged into
>>> +        trace_qdev_update_parent_bus(dev, dev->parent_bus, bus);
>>
>> Nitpicking, if you respin, can you add object_get_typename(OBJECT(dev)))
>> and object_get_typename(OBJECT(bus)))?
> 
> sure. I was wondering if having some kind of qom object support to trace
> would be feasible. Because it's a bit tedious to add this each time. And
> IMO it would be more useful to have the path, but we can't reasonably
> compute it as a trace_..() arguments.

Meanwhile you can use:

   if (trace_event_get_state_backends(TRACE_QDEV_UPDATE_PARENT_BUS)) {
       ...

>>
>> With/without it:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>>> +        /*
>>> +         * Keep a reference to the device while it's not plugged into
>>>             * any bus, to avoid it potentially evaporating when it is
>>>             * dereffed in bus_remove_child().
>>>             */
>>> @@ -272,6 +275,18 @@ HotplugHandler
>>> *qdev_get_hotplug_handler(DeviceState *dev)
>>>        return hotplug_ctrl;
>>>    }
>>>    +static int qdev_prereset(DeviceState *dev, void *opaque)
>>> +{
>>> +    trace_qdev_reset_tree(dev, object_get_typename(OBJECT(dev)));
>>> +    return 0;
>>> +}
>>> +
>>> +static int qbus_prereset(BusState *bus, void *opaque)
>>> +{
>>> +    trace_qbus_reset_tree(bus, object_get_typename(OBJECT(bus)));
>>> +    return 0;
>>> +}
>>> +
>>>    static int qdev_reset_one(DeviceState *dev, void *opaque)
>>>    {
>>>        device_legacy_reset(dev);
>>> @@ -282,6 +297,7 @@ static int qdev_reset_one(DeviceState *dev, void
>>> *opaque)
>>>    static int qbus_reset_one(BusState *bus, void *opaque)
>>>    {
>>>        BusClass *bc = BUS_GET_CLASS(bus);
>>> +    trace_qbus_reset(bus, object_get_typename(OBJECT(bus)));
>>>        if (bc->reset) {
>>>            bc->reset(bus);
>>>        }
>>> @@ -290,7 +306,9 @@ static int qbus_reset_one(BusState *bus, void
>>> *opaque)
>>>      void qdev_reset_all(DeviceState *dev)
>>>    {
>>> -    qdev_walk_children(dev, NULL, NULL, qdev_reset_one,
>>> qbus_reset_one, NULL);
>>> +    trace_qdev_reset_all(dev, object_get_typename(OBJECT(dev)));
>>> +    qdev_walk_children(dev, qdev_prereset, qbus_prereset,
>>> +                       qdev_reset_one, qbus_reset_one, NULL);
>>>    }
>>>      void qdev_reset_all_fn(void *opaque)
>>> @@ -300,7 +318,9 @@ void qdev_reset_all_fn(void *opaque)
>>>      void qbus_reset_all(BusState *bus)
>>>    {
>>> -    qbus_walk_children(bus, NULL, NULL, qdev_reset_one,
>>> qbus_reset_one, NULL);
>>> +    trace_qbus_reset_all(bus, object_get_typename(OBJECT(bus)));
>>> +    qbus_walk_children(bus, qdev_prereset, qbus_prereset,
>>> +                       qdev_reset_one, qbus_reset_one, NULL);
>>>    }
>>>      void qbus_reset_all_fn(void *opaque)
>>> @@ -1108,6 +1128,7 @@ void device_legacy_reset(DeviceState *dev)
>>>    {
>>>        DeviceClass *klass = DEVICE_GET_CLASS(dev);
>>>    +    trace_qdev_reset(dev, object_get_typename(OBJECT(dev)));
>>>        if (klass->reset) {
>>>            klass->reset(dev);
>>>        }
>>> diff --git a/hw/core/trace-events b/hw/core/trace-events
>>> index fe47a9c8cb..1a669be0ea 100644
>>> --- a/hw/core/trace-events
>>> +++ b/hw/core/trace-events
>>> @@ -1,2 +1,11 @@
>>>    # loader.c
>>>    loader_write_rom(const char *name, uint64_t gpa, uint64_t size, bool
>>> isrom) "%s: @0x%"PRIx64" size=0x%"PRIx64" ROM=%d"
>>> +
>>> +# qdev.c
>>> +qdev_reset(void *obj, const char *objtype) "obj=%p(%s)"
>>> +qdev_reset_all(void *obj, const char *objtype) "obj=%p(%s)"
>>> +qdev_reset_tree(void *obj, const char *objtype) "obj=%p(%s)"
>>> +qbus_reset(void *obj, const char *objtype) "obj=%p(%s)"
>>> +qbus_reset_all(void *obj, const char *objtype) "obj=%p(%s)"
>>> +qbus_reset_tree(void *obj, const char *objtype) "obj=%p(%s)"
>>> +qdev_update_parent_bus(void *obj, void *oldp, void *newp) "obj=%p
>>> old_parent=%p new_parent=%p"
>>>