Re: [PATCH v7 03/11] hw/core: create Resettable QOM interface

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

On 1/16/20 2:59 AM, Philippe Mathieu-Daudé wrote:
> On 1/15/20 1:36 PM, Damien Hedde wrote:
>> This commit defines an interface allowing multi-phase reset. This aims
>> to solve a problem of the actual single-phase reset (built in
>> DeviceClass and BusClass): reset behavior is dependent on the order
>> in which reset handlers are called. In particular doing external
>> side-effect (like setting an qemu_irq) is problematic because receiving
>> object may not be reset yet.
>>
>> The Resettable interface divides the reset in 3 well defined phases.
>> To reset an object tree, all 1st phases are executed then all 2nd then
>> all 3rd. See the comments in include/hw/resettable.h for a more complete
>> description. The interface defines 3 phases to let the future
>> possibility of holding an object into reset for some time.
>>
>> The qdev/qbus reset in DeviceClass and BusClass will be modified in
>> following commits to use this interface. A mechanism is provided
>> to allow executing a transitional reset handler in place of the 2nd
>> phase which is executed in children-then-parent order inside a tree.
>> This will allow to transition devices and buses smoothly while
>> keeping the exact current qdev/qbus reset behavior for now.
>>
>> Documentation will be added in a following commit.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>
>> v7 update: un-nest struct ResettablePhases
>> ---
>>   Makefile.objs           |   1 +
>>   include/hw/resettable.h | 211 +++++++++++++++++++++++++++++++++++
>>   hw/core/resettable.c    | 238 ++++++++++++++++++++++++++++++++++++++++
>>   hw/core/Makefile.objs   |   1 +
>>   hw/core/trace-events    |  17 +++
>>   5 files changed, 468 insertions(+)
>>   create mode 100644 include/hw/resettable.h
>>   create mode 100644 hw/core/resettable.c
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 7c1e50f9d6..9752d549b4 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -191,6 +191,7 @@ trace-events-subdirs += migration
>>   trace-events-subdirs += net
>>   trace-events-subdirs += ui
>>   endif
>> +trace-events-subdirs += hw/core
> 
> TL;DR Duplicating this line breaks using the LTTng Userspace Tracer 
> library (UST backend).
> 
> Probably because you started this series before commit 26b8e6dc42b got 
> merged, Jun 13 2019!
> 
> Indeed Oct 02 2018...
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg564153.html
> 
> The problem is you (correctly) sorted alphabetically while Alexey 
> appended at the end.
> 
>>   trace-events-subdirs += hw/display
>>   trace-events-subdirs += qapi
>>   trace-events-subdirs += qom
[...]
>> diff --git a/hw/core/trace-events b/hw/core/trace-events
>> index a375aa88a4..a2e43f1120 100644
>> --- a/hw/core/trace-events
>> +++ b/hw/core/trace-events
>> @@ -9,3 +9,20 @@ 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, const char *objtype, void *oldp, 
>> const char *oldptype, void *newp, const char *newptype) "obj=%p(%s) 
>> old_parent=%p(%s) new_parent=%p(%s)"
>> +
>> +# resettable.c
>> +resettable_reset(void *obj, int cold) "obj=%p cold=%d"
>> +resettable_reset_assert_begin(void *obj, int cold) "obj=%p cold=%d"
>> +resettable_reset_assert_end(void *obj) "obj=%p"
>> +resettable_reset_release_begin(void *obj, int cold) "obj=%p cold=%d"
>> +resettable_reset_release_end(void *obj) "obj=%p"
>> +resettable_phase_enter_begin(void *obj, const char *objtype, uint32_t 
>> count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
>> +resettable_phase_enter_exec(void *obj, const char *objtype, int type, 
>> int has_method) "obj=%p(%s) type=%d method=%d"
>> +resettable_phase_enter_end(void *obj, const char *objtype, uint32_t 
>> count) "obj=%p(%s) count=%" PRIu32
>> +resettable_phase_hold_begin(void *obj, const char *objtype, uint32_t 
>> count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
>> +resettable_phase_hold_exec(void *obj, const char *objtype, int 
>> has_method) "obj=%p(%s) method=%d"
>> +resettable_phase_hold_end(void *obj, const char *objtype, uint32_t 
>> count) "obj=%p(%s) count=%" PRIu32
>> +resettable_phase_exit_begin(void *obj, const char *objtype, uint32_t 
>> count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
>> +resettable_phase_exit_exec(void *obj, const char *objtype, int 
>> has_method) "obj=%p(%s) method=%d"
>> +resettable_phase_exit_end(void *obj, const char *objtype, uint32_t 
>> count) "obj=%p(%s) count=%" PRIu32
>> +resettable_transitional_function(void *obj, const char *objtype) 
>> "obj=%p(%s)"
> 
> Something here breaks ./configure --enable-trace-backends=ust:
> 
>    CC      trace-ust-all.o
> In file included from trace-ust-all.h:13,
>                   from trace-ust-all.c:13:
> trace-ust-all.h:35151:1: error: redefinition of 
> ‘__tracepoint_cb_qemu___loader_write_rom’
> 35151 | TRACEPOINT_EVENT(
>        | ^~~~~~~~~~~~~~~~
> trace-ust-all.h:31791:1: note: previous definition of 
> ‘__tracepoint_cb_qemu___loader_write_rom’ was here
> 31791 | TRACEPOINT_EVENT(
>        | ^~~~~~~~~~~~~~~~
> ...
> ./trace-ust-all.h:35388:1: error: redefinition of 
> ‘__tp_event_signature___qemu___resettable_transitional_function’
> 35388 | TRACEPOINT_EVENT(
>        | ^~~~~~~~~~~~~~~~
> ./trace-ust-all.h:32028:1: note: previous definition of 
> ‘__tp_event_signature___qemu___resettable_transitional_function’ was here
> 32028 | TRACEPOINT_EVENT(
>        | ^~~~~~~~~~~~~~~~
> In file included from /usr/include/lttng/tracepoint-event.h:58,
>                   from trace-ust-all.h:35401,
>                   from trace-ust-all.c:13:
> 
> Indeed:
> 
> 32028 TRACEPOINT_EVENT(
> 32029    qemu,
> 32030    resettable_transitional_function,
> 32031    TP_ARGS(void *, obj, const char *, objtype),
> 32032    TP_FIELDS(
> 32033        ctf_integer_hex(void *, obj, obj)
> 32034        ctf_string(objtype, objtype)
> 32035    )
> 32036 )
> 32037
> ...
> 35388 TRACEPOINT_EVENT(
> 35389    qemu,
> 35390    resettable_transitional_function,
> 35391    TP_ARGS(void *, obj, const char *, objtype),
> 35392    TP_FIELDS(
> 35393        ctf_integer_hex(void *, obj, obj)
> 35394        ctf_string(objtype, objtype)
> 35395    )
> 35396 )
> 35397
> 35398 #endif /* TRACE_ALL_GENERATED_UST_H */
> 
> Ah! I was going to say "no clue what could be wrong, so Cc'ing Stefan" 
> but got it:
> 
> $ git grep hw/core Makefile.objs
> Makefile.objs:194:trace-events-subdirs += hw/core
> Makefile.objs:207:trace-events-subdirs += hw/core
> 
> We might already have a 'uniq' makefile function to do:
> 
> trace-events-subdirs = $(call uniq $(trace-events-subdirs))
> 
> or maybe was it with $filter? I can't find it/remember, too tired.
> 
> So the fix is:
> 
> -- >8 --
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -191,7 +191,6 @@ trace-events-subdirs += migration
>   trace-events-subdirs += net
>   trace-events-subdirs += ui
>   endif
> -trace-events-subdirs += hw/core
>   trace-events-subdirs += hw/display
>   trace-events-subdirs += qapi
>   trace-events-subdirs += qom
> ---

Forgot to add, with trace-events-subdirs fixed:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>