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

[Philippe Mathieu-Daudé]

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 <address@hidden>
> > > Reviewed-by: Richard Henderson <address@hidden>
> > > ---
> > >
> > > 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.