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

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

On 1/16/20 9:53 AM, Damien Hedde wrote:
> 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
>>>
> 
>>
>> 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.
> 
> You can use $sort to remove duplicates in make.

Ah $(sort ...) thanks, I was too tired to remember it =)

>>
>> 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
>> ---
>>
> 
> I'll remove the duplicate entry.

I prepared a patch to move hw/core, if qemu-trivial merges it first, you 
shouldn't need to respin your series.