diff mbox series

[08/10] hw/core/reset: Implement qemu_register_reset via qemu_register_resettable

Message ID 20240220160622.114437-9-peter.maydell@linaro.org
State Superseded
Headers show
Series reset: Make whole system three-phase-reset aware | expand

Commit Message

Peter Maydell Feb. 20, 2024, 4:06 p.m. UTC 
Reimplement qemu_register_reset() via qemu_register_resettable().

We define a new LegacyReset object which implements Resettable and
defines its reset hold phase method to call a QEMUResetHandler
function.  When qemu_register_reset() is called, we create a new
LegacyReset object and add it to the simulation_reset
ResettableContainer.  When qemu_unregister_reset() is called, we find
the LegacyReset object in the container and remove it.

This implementation of qemu_unregister_reset() means we'll end up
scanning the ResetContainer's list of child objects twice, once
to find the LegacyReset object, and once in g_ptr_array_remove().
In theory we could avoid this by having the ResettableContainer
interface include a resettable_container_remove_with_equal_func()
that took a callback method so that we could use
g_ptr_array_find_with_equal_func() and g_ptr_array_remove_index().
But we don't expect qemu_unregister_reset() to be called frequently
or in hot paths, and we expect the simulation_reset container to
usually not have many children.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
The way that a legacy reset function needs to check the ShutdownCause
and this doesn't line up with the ResetType is a bit awkward; this
is an area we should come back and clean up, but I didn't want to
tackle that in this patchset.
---
 include/sysemu/reset.h |   7 ++-
 hw/core/reset.c        | 137 +++++++++++++++++++++++++++++++----------
 2 files changed, 110 insertions(+), 34 deletions(-)

Comments

Richard Henderson Feb. 20, 2024, 8:06 p.m. UTC | #1 
On 2/20/24 06:06, Peter Maydell wrote:
> Reimplement qemu_register_reset() via qemu_register_resettable().
> 
> We define a new LegacyReset object which implements Resettable and
> defines its reset hold phase method to call a QEMUResetHandler
> function.  When qemu_register_reset() is called, we create a new
> LegacyReset object and add it to the simulation_reset
> ResettableContainer.  When qemu_unregister_reset() is called, we find
> the LegacyReset object in the container and remove it.
> 
> This implementation of qemu_unregister_reset() means we'll end up
> scanning the ResetContainer's list of child objects twice, once
> to find the LegacyReset object, and once in g_ptr_array_remove().
> In theory we could avoid this by having the ResettableContainer
> interface include a resettable_container_remove_with_equal_func()
> that took a callback method so that we could use
> g_ptr_array_find_with_equal_func() and g_ptr_array_remove_index().
> But we don't expect qemu_unregister_reset() to be called frequently
> or in hot paths, and we expect the simulation_reset container to
> usually not have many children.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> The way that a legacy reset function needs to check the ShutdownCause
> and this doesn't line up with the ResetType is a bit awkward; this
> is an area we should come back and clean up, but I didn't want to
> tackle that in this patchset.
> ---
>   include/sysemu/reset.h |   7 ++-
>   hw/core/reset.c        | 137 +++++++++++++++++++++++++++++++----------
>   2 files changed, 110 insertions(+), 34 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~
Zhao Liu Feb. 27, 2024, 6:18 a.m. UTC | #2 
On Tue, Feb 20, 2024 at 04:06:20PM +0000, Peter Maydell wrote:
> Date: Tue, 20 Feb 2024 16:06:20 +0000
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: [PATCH 08/10] hw/core/reset: Implement qemu_register_reset via
>  qemu_register_resettable
> X-Mailer: git-send-email 2.34.1
> 
> Reimplement qemu_register_reset() via qemu_register_resettable().
> 
> We define a new LegacyReset object which implements Resettable and
> defines its reset hold phase method to call a QEMUResetHandler
> function.  When qemu_register_reset() is called, we create a new
> LegacyReset object and add it to the simulation_reset
> ResettableContainer.  When qemu_unregister_reset() is called, we find
> the LegacyReset object in the container and remove it.
> 
> This implementation of qemu_unregister_reset() means we'll end up
> scanning the ResetContainer's list of child objects twice, once
> to find the LegacyReset object, and once in g_ptr_array_remove().
> In theory we could avoid this by having the ResettableContainer
> interface include a resettable_container_remove_with_equal_func()
> that took a callback method so that we could use
> g_ptr_array_find_with_equal_func() and g_ptr_array_remove_index().
> But we don't expect qemu_unregister_reset() to be called frequently
> or in hot paths, and we expect the simulation_reset container to
> usually not have many children.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> The way that a legacy reset function needs to check the ShutdownCause
> and this doesn't line up with the ResetType is a bit awkward; this
> is an area we should come back and clean up, but I didn't want to
> tackle that in this patchset.
> ---
>  include/sysemu/reset.h |   7 ++-
>  hw/core/reset.c        | 137 +++++++++++++++++++++++++++++++----------
>  2 files changed, 110 insertions(+), 34 deletions(-)
>

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
diff mbox series

Patch

diff --git a/include/sysemu/reset.h b/include/sysemu/reset.h
index a3de48cb9cf..ada4527e67e 100644
--- a/include/sysemu/reset.h
+++ b/include/sysemu/reset.h
@@ -67,8 +67,11 @@  void qemu_unregister_resettable(Object *obj);
  * @opaque: opaque data to pass to @func
  *
  * Register @func on the list of functions which are called when the
- * entire system is reset. The functions are called in the order in
- * which they are registered.
+ * entire system is reset. Functions registered with this API and
+ * Resettable objects registered with qemu_register_resettable() are
+ * handled together, in the order in which they were registered.
+ * Functions registered with this API are called in the 'hold' phase
+ * of the 3-phase reset.
  *
  * In general this function should not be used in new code where possible;
  * for instance device model reset is better accomplished using the
diff --git a/hw/core/reset.c b/hw/core/reset.c
index a9b30e705fe..d50da7e3041 100644
--- a/hw/core/reset.c
+++ b/hw/core/reset.c
@@ -24,7 +24,6 @@ 
  */
 
 #include "qemu/osdep.h"
-#include "qemu/queue.h"
 #include "sysemu/reset.h"
 #include "hw/resettable.h"
 #include "hw/core/resetcontainer.h"
@@ -44,45 +43,128 @@  static ResettableContainer *get_root_reset_container(void)
     return root_reset_container;
 }
 
-typedef struct QEMUResetEntry {
-    QTAILQ_ENTRY(QEMUResetEntry) entry;
+/*
+ * Reason why the currently in-progress qemu_devices_reset() was called.
+ * If we made at least SHUTDOWN_CAUSE_SNAPSHOT_LOAD have a corresponding
+ * ResetType we could perhaps avoid the need for this global.
+ */
+static ShutdownCause device_reset_reason;
+
+/*
+ * This is an Object which implements Resettable simply to call the
+ * callback function in the hold phase.
+ */
+#define TYPE_LEGACY_RESET "legacy-reset"
+OBJECT_DECLARE_SIMPLE_TYPE(LegacyReset, LEGACY_RESET)
+
+struct LegacyReset {
+    Object parent;
+    ResettableState reset_state;
     QEMUResetHandler *func;
     void *opaque;
     bool skip_on_snapshot_load;
-} QEMUResetEntry;
+};
 
-static QTAILQ_HEAD(, QEMUResetEntry) reset_handlers =
-    QTAILQ_HEAD_INITIALIZER(reset_handlers);
+OBJECT_DEFINE_SIMPLE_TYPE_WITH_INTERFACES(LegacyReset, legacy_reset, LEGACY_RESET, OBJECT, { TYPE_RESETTABLE_INTERFACE }, { })
+
+static ResettableState *legacy_reset_get_state(Object *obj)
+{
+    LegacyReset *lr = LEGACY_RESET(obj);
+    return &lr->reset_state;
+}
+
+static void legacy_reset_hold(Object *obj)
+{
+    LegacyReset *lr = LEGACY_RESET(obj);
+
+    if (device_reset_reason == SHUTDOWN_CAUSE_SNAPSHOT_LOAD &&
+        lr->skip_on_snapshot_load) {
+        return;
+    }
+    lr->func(lr->opaque);
+}
+
+static void legacy_reset_init(Object *obj)
+{
+}
+
+static void legacy_reset_finalize(Object *obj)
+{
+}
+
+static void legacy_reset_class_init(ObjectClass *klass, void *data)
+{
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
+
+    rc->get_state = legacy_reset_get_state;
+    rc->phases.hold = legacy_reset_hold;
+}
 
 void qemu_register_reset(QEMUResetHandler *func, void *opaque)
 {
-    QEMUResetEntry *re = g_new0(QEMUResetEntry, 1);
+    Object *obj = object_new(TYPE_LEGACY_RESET);
+    LegacyReset *lr = LEGACY_RESET(obj);
 
-    re->func = func;
-    re->opaque = opaque;
-    QTAILQ_INSERT_TAIL(&reset_handlers, re, entry);
+    lr->func = func;
+    lr->opaque = opaque;
+    qemu_register_resettable(obj);
 }
 
 void qemu_register_reset_nosnapshotload(QEMUResetHandler *func, void *opaque)
 {
-    QEMUResetEntry *re = g_new0(QEMUResetEntry, 1);
+    Object *obj = object_new(TYPE_LEGACY_RESET);
+    LegacyReset *lr = LEGACY_RESET(obj);
 
-    re->func = func;
-    re->opaque = opaque;
-    re->skip_on_snapshot_load = true;
-    QTAILQ_INSERT_TAIL(&reset_handlers, re, entry);
+    lr->func = func;
+    lr->opaque = opaque;
+    lr->skip_on_snapshot_load = true;
+    qemu_register_resettable(obj);
+}
+
+typedef struct FindLegacyInfo {
+    QEMUResetHandler *func;
+    void *opaque;
+    LegacyReset *lr;
+} FindLegacyInfo;
+
+static void find_legacy_reset_cb(Object *obj, void *opaque, ResetType type)
+{
+    LegacyReset *lr;
+    FindLegacyInfo *fli = opaque;
+
+    /* Not everything in the ResettableContainer will be a LegacyReset */
+    lr = LEGACY_RESET(object_dynamic_cast(obj, TYPE_LEGACY_RESET));
+    if (lr && lr->func == fli->func && lr->opaque == fli->opaque) {
+        fli->lr = lr;
+    }
+}
+
+static LegacyReset *find_legacy_reset(QEMUResetHandler *func, void *opaque)
+{
+    /*
+     * Find the LegacyReset with the specified func and opaque,
+     * by getting the ResettableContainer to call our callback for
+     * every item in it.
+     */
+    ResettableContainer *rootcon = get_root_reset_container();
+    ResettableClass *rc = RESETTABLE_GET_CLASS(rootcon);
+    FindLegacyInfo fli;
+
+    fli.func = func;
+    fli.opaque = opaque;
+    fli.lr = NULL;
+    rc->child_foreach(OBJECT(rootcon), find_legacy_reset_cb,
+                      &fli, RESET_TYPE_COLD);
+    return fli.lr;
 }
 
 void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
 {
-    QEMUResetEntry *re;
+    Object *obj = OBJECT(find_legacy_reset(func, opaque));
 
-    QTAILQ_FOREACH(re, &reset_handlers, entry) {
-        if (re->func == func && re->opaque == opaque) {
-            QTAILQ_REMOVE(&reset_handlers, re, entry);
-            g_free(re);
-            return;
-        }
+    if (obj) {
+        qemu_unregister_resettable(obj);
+        object_unref(obj);
     }
 }
 
@@ -98,16 +180,7 @@  void qemu_unregister_resettable(Object *obj)
 
 void qemu_devices_reset(ShutdownCause reason)
 {
-    QEMUResetEntry *re, *nre;
-
-    /* reset all devices */
-    QTAILQ_FOREACH_SAFE(re, &reset_handlers, entry, nre) {
-        if (reason == SHUTDOWN_CAUSE_SNAPSHOT_LOAD &&
-            re->skip_on_snapshot_load) {
-            continue;
-        }
-        re->func(re->opaque);
-    }
+    device_reset_reason = reason;
 
     /* Reset the simulation */
     resettable_reset(OBJECT(get_root_reset_container()), RESET_TYPE_COLD);