Re: [PATCH 06/10] hw/core: Add ResetContainer which holds objects implementing Resettable

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 21/2/24 17:09, Peter Maydell wrote:
> On Wed, 21 Feb 2024 at 15:34, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 20/2/24 17:06, Peter Maydell wrote:
>>> Implement a ResetContainer.  This is a subclass of Object, and it
>>> implements the Resettable interface.  The container holds a list of
>>> arbitrary other objects which implement Resettable, and when the
>>> container is reset, all the objects it contains are also reset.
>>>
>>> This will allow us to have a 3-phase-reset equivalent of the old
>>> qemu_register_reset() API: we will have a single "simulation reset"
>>> top level ResetContainer, and objects in it are the equivalent of the
>>> old QEMUResetHandler functions.
>>>
>>> The qemu_register_reset() API manages its list of callbacks using a
>>> QTAILQ, but here we use a GPtrArray for our list of Resettable
>>> children: we expect the "remove" operation (which will need to do an
>>> iteration through the list) to be fairly uncommon, and we get simpler
>>> code with fewer memory allocations.
>>>
>>> Since there is currently no listed owner in MAINTAINERS for the
>>> existing reset-related source files, create a new section for
>>> them, and add these new files there also.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>>    MAINTAINERS                      | 10 +++++
>>>    include/hw/core/resetcontainer.h | 48 ++++++++++++++++++++
>>>    hw/core/resetcontainer.c         | 76 ++++++++++++++++++++++++++++++++
>>>    hw/core/meson.build              |  1 +
>>>    4 files changed, 135 insertions(+)
>>>    create mode 100644 include/hw/core/resetcontainer.h
>>>    create mode 100644 hw/core/resetcontainer.c
>>
>>
>>> +static void resettable_container_child_foreach(Object *obj,
>>> +                                               ResettableChildCallback cb,
>>> +                                               void *opaque, ResetType type)
>>> +{
>>> +    ResettableContainer *rc = RESETTABLE_CONTAINER(obj);
>>> +    unsigned int len = rc->children->len;
>>> +
>>> +    for (unsigned int i = 0; i < len; i++) {
>>
>> Worth a pair of trace events around the callback call.
> 
> Do you think so? What would be the interest in them?
> (The way the resettable handling works this foreach loop
> gets called several times for any particular reset event,
> as well as getting called if anybody calls qemu_unregister_reset():
> so "something is iterating the resettable container children"
> can be for multiple reasons.)

I remember Damien added a bunch resettable* events and I've been
using them to test his series, but also later while refactoring
some devices or QDevifying others, in particular when devices
contain buses.

$ git grep trace_ hw/core/reset*
hw/core/resettable.c:44:    trace_resettable_reset(obj, type);
hw/core/resettable.c:53:    trace_resettable_reset_assert_begin(obj, type);
hw/core/resettable.c:62:    trace_resettable_reset_assert_end(obj);
hw/core/resettable.c:69:    trace_resettable_reset_release_begin(obj, type);
hw/core/resettable.c:76:    trace_resettable_reset_release_end(obj);
hw/core/resettable.c:124:    trace_resettable_phase_enter_begin(obj, 
obj_typename, s->count, type);
hw/core/resettable.c:151:        trace_resettable_phase_enter_exec(obj, 
obj_typename, type,
hw/core/resettable.c:158:    trace_resettable_phase_enter_end(obj, 
obj_typename, s->count);
hw/core/resettable.c:170:    trace_resettable_phase_hold_begin(obj, 
obj_typename, s->count, type);
hw/core/resettable.c:179:        trace_resettable_phase_hold_exec(obj, 
obj_typename, !!rc->phases.hold);
hw/core/resettable.c:181: 
trace_resettable_transitional_function(obj, obj_typename);
hw/core/resettable.c:187:    trace_resettable_phase_hold_end(obj, 
obj_typename, s->count);
hw/core/resettable.c:197:    trace_resettable_phase_exit_begin(obj, 
obj_typename, s->count, type);
hw/core/resettable.c:205:        trace_resettable_phase_exit_exec(obj, 
obj_typename, !!rc->phases.exit);
hw/core/resettable.c:211:    trace_resettable_phase_exit_end(obj, 
obj_typename, s->count);
hw/core/resettable.c:243:    trace_resettable_change_parent(obj, oldp, 
oldp_count, newp, newp_count);

Anyway, can be added later if useful for debugging. Certainly not a
blocker :)