| Message ID | 20240220160622.114437-7-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reset: Make whole system three-phase-reset aware | expand |
On 2/20/24 06: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 Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 2/20/24 06:06, Peter Maydell wrote: > +void resettable_container_add(ResettableContainer *rc, Object *obj) > +{ > + g_ptr_array_add(rc->children, obj); > +} Did you want to assert here that obj does in fact implement Resettable? r~
On Tue, 20 Feb 2024 at 19:46, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 2/20/24 06:06, Peter Maydell wrote: > > +void resettable_container_add(ResettableContainer *rc, Object *obj) > > +{ > > + g_ptr_array_add(rc->children, obj); > > +} > > Did you want to assert here that obj does in fact implement Resettable? I guess that makes for a nicer detection of that class of bug, so sure. -- PMM
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. > + cb(g_ptr_array_index(rc->children, i), opaque, type); > + /* Detect callbacks trying to unregister themselves */ > + assert(len == rc->children->len); > + } > +} Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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.) -- PMM
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 :)
On Tue, 20 Feb 2024 at 21:20, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 20 Feb 2024 at 19:46, Richard Henderson > <richard.henderson@linaro.org> wrote: > > > > On 2/20/24 06:06, Peter Maydell wrote: > > > +void resettable_container_add(ResettableContainer *rc, Object *obj) > > > +{ > > > + g_ptr_array_add(rc->children, obj); > > > +} > > > > Did you want to assert here that obj does in fact implement Resettable? > > I guess that makes for a nicer detection of that class of bug, > so sure. I'm going to do this with INTERFACE_CHECK(void, obj, TYPE_RESETTABLE_INTERFACE); If anybody thinks there's a better way of doing that let me know. -- PMM
On Tue, Feb 20, 2024 at 04:06:18PM +0000, Peter Maydell wrote: > Date: Tue, 20 Feb 2024 16:06:18 +0000 > From: Peter Maydell <peter.maydell@linaro.org> > Subject: [PATCH 06/10] hw/core: Add ResetContainer which holds objects > implementing Resettable > X-Mailer: git-send-email 2.34.1 > > 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 > Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
diff --git a/MAINTAINERS b/MAINTAINERS index 7d61fb93194..df8a526905b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3667,6 +3667,16 @@ F: hw/core/clock-vmstate.c F: hw/core/qdev-clock.c F: docs/devel/clocks.rst +Reset framework +M: Peter Maydell <peter.maydell@linaro.org> +S: Maintained +F: include/hw/resettable.h +F: include/hw/core/resetcontainer.h +F: include/sysemu/reset.h +F: hw/core/reset.c +F: hw/core/resettable.c +F: hw/core/resetcontainer.c + Usermode Emulation ------------------ Overall usermode emulation diff --git a/include/hw/core/resetcontainer.h b/include/hw/core/resetcontainer.h new file mode 100644 index 00000000000..23db0c7a880 --- /dev/null +++ b/include/hw/core/resetcontainer.h @@ -0,0 +1,48 @@ +/* + * Reset container + * + * Copyright (c) 2024 Linaro, Ltd + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +#ifndef HW_RESETCONTAINER_H +#define HW_RESETCONTAINER_H + +/* + * The "reset container" is an object which implements the Resettable + * interface. It contains a list of arbitrary other objects which also + * implement Resettable. Resetting the reset container resets all the + * objects in it. + */ + +#include "qom/object.h" + +#define TYPE_RESETTABLE_CONTAINER "resettable-container" +OBJECT_DECLARE_TYPE(ResettableContainer, ResettableContainerClass, RESETTABLE_CONTAINER) + +/** + * resettable_container_add: Add a resettable object to the container + * @rc: container + * @obj: object to add to the container + * + * Add @obj to the ResettableContainer @rc. @obj must implement the + * Resettable interface. + * + * When @rc is reset, it will reset every object that has been added + * to it, in the order they were added. + */ +void resettable_container_add(ResettableContainer *rc, Object *obj); + +/** + * resettable_container_remove: Remove an object from the container + * @rc: container + * @obj: object to remove from the container + * + * Remove @obj from the ResettableContainer @rc. @obj must have been + * previously added to this container. + */ +void resettable_container_remove(ResettableContainer *rc, Object *obj); + +#endif diff --git a/hw/core/resetcontainer.c b/hw/core/resetcontainer.c new file mode 100644 index 00000000000..cd627f16f0e --- /dev/null +++ b/hw/core/resetcontainer.c @@ -0,0 +1,76 @@ +/* + * Reset container + * + * Copyright (c) 2024 Linaro, Ltd + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +/* + * The "reset container" is an object which implements the Resettable + * interface. It contains a list of arbitrary other objects which also + * implement Resettable. Resetting the reset container resets all the + * objects in it. + */ + +#include "qemu/osdep.h" +#include "hw/resettable.h" +#include "hw/core/resetcontainer.h" + +struct ResettableContainer { + Object parent; + ResettableState reset_state; + GPtrArray *children; +}; + +OBJECT_DEFINE_SIMPLE_TYPE_WITH_INTERFACES(ResettableContainer, resettable_container, RESETTABLE_CONTAINER, OBJECT, { TYPE_RESETTABLE_INTERFACE }, { }) + +void resettable_container_add(ResettableContainer *rc, Object *obj) +{ + g_ptr_array_add(rc->children, obj); +} + +void resettable_container_remove(ResettableContainer *rc, Object *obj) +{ + g_ptr_array_remove(rc->children, obj); +} + +static ResettableState *resettable_container_get_state(Object *obj) +{ + ResettableContainer *rc = RESETTABLE_CONTAINER(obj); + return &rc->reset_state; +} + +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++) { + cb(g_ptr_array_index(rc->children, i), opaque, type); + /* Detect callbacks trying to unregister themselves */ + assert(len == rc->children->len); + } +} + +static void resettable_container_init(Object *obj) +{ + ResettableContainer *rc = RESETTABLE_CONTAINER(obj); + + rc->children = g_ptr_array_new(); +} + +static void resettable_container_finalize(Object *obj) +{ +} + +static void resettable_container_class_init(ObjectClass *klass, void *data) +{ + ResettableClass *rc = RESETTABLE_CLASS(klass); + + rc->get_state = resettable_container_get_state; + rc->child_foreach = resettable_container_child_foreach; +} diff --git a/hw/core/meson.build b/hw/core/meson.build index 67dad04de55..e26f2e088c3 100644 --- a/hw/core/meson.build +++ b/hw/core/meson.build @@ -4,6 +4,7 @@ hwcore_ss.add(files( 'qdev-properties.c', 'qdev.c', 'reset.c', + 'resetcontainer.c', 'resettable.c', 'vmstate-if.c', # irq.c needed for qdev GPIO handling:
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