diff mbox series

[03/10] system/bootdevice: Don't unregister reset handler in restore_boot_order()

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

Commit Message

Peter Maydell Feb. 20, 2024, 4:06 p.m. UTC 
Currently the qemu_register_reset() API permits the reset handler functions
registered with it to remove themselves from within the callback function.
This is fine with our current implementation, but is a bit odd, because
generally reset is supposed to be idempotent, and doesn't fit well in a
three-phase-reset world where a resettable object will get multiple
callbacks as the system is reset.

We now have only one user of qemu_register_reset() which makes use of
the ability to unregister itself within the callback:
restore_boot_order().  We want to change our implementation of
qemu_register_reset() to something where it would be awkward to
maintain the "can self-unregister" feature.  Rather than making that
reimplementation complicated, change restore_boot_order() so that it
doesn't unregister itself but instead returns doing nothing for any
calls after it has done the "restore the boot order" work.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
It would be nicer not to use reset at all, especially since I'm not
a fan of conflating "system is reset" with "system boots", but I
didn't have a good idea for how to do that.
---
 system/bootdevice.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

Comments

Richard Henderson Feb. 20, 2024, 7:35 p.m. UTC | #1 
On 2/20/24 06:06, Peter Maydell wrote:
> Currently the qemu_register_reset() API permits the reset handler functions
> registered with it to remove themselves from within the callback function.
> This is fine with our current implementation, but is a bit odd, because
> generally reset is supposed to be idempotent, and doesn't fit well in a
> three-phase-reset world where a resettable object will get multiple
> callbacks as the system is reset.
> 
> We now have only one user of qemu_register_reset() which makes use of
> the ability to unregister itself within the callback:
> restore_boot_order().  We want to change our implementation of
> qemu_register_reset() to something where it would be awkward to
> maintain the "can self-unregister" feature.  Rather than making that
> reimplementation complicated, change restore_boot_order() so that it
> doesn't unregister itself but instead returns doing nothing for any
> calls after it has done the "restore the boot order" work.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> It would be nicer not to use reset at all, especially since I'm not
> a fan of conflating "system is reset" with "system boots", but I
> didn't have a good idea for how to do that.
> ---
>   system/bootdevice.c | 25 ++++++++++++++-----------
>   1 file changed, 14 insertions(+), 11 deletions(-)

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

r~
Zhao Liu Feb. 26, 2024, 2:16 p.m. UTC | #2 
On Tue, Feb 20, 2024 at 04:06:15PM +0000, Peter Maydell wrote:
> Date: Tue, 20 Feb 2024 16:06:15 +0000
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: [PATCH 03/10] system/bootdevice: Don't unregister reset handler in
>  restore_boot_order()
> X-Mailer: git-send-email 2.34.1
> 
> Currently the qemu_register_reset() API permits the reset handler functions
> registered with it to remove themselves from within the callback function.
> This is fine with our current implementation, but is a bit odd, because
> generally reset is supposed to be idempotent, and doesn't fit well in a
> three-phase-reset world where a resettable object will get multiple
> callbacks as the system is reset.
> 
> We now have only one user of qemu_register_reset() which makes use of
> the ability to unregister itself within the callback:
> restore_boot_order().  We want to change our implementation of
> qemu_register_reset() to something where it would be awkward to
> maintain the "can self-unregister" feature.  Rather than making that
> reimplementation complicated, change restore_boot_order() so that it
> doesn't unregister itself but instead returns doing nothing for any
> calls after it has done the "restore the boot order" work.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> It would be nicer not to use reset at all, especially since I'm not
> a fan of conflating "system is reset" with "system boots", but I
> didn't have a good idea for how to do that.
> ---
>  system/bootdevice.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)

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

Patch

diff --git a/system/bootdevice.c b/system/bootdevice.c
index 2106f1026ff..2c55c9bd90c 100644
--- a/system/bootdevice.c
+++ b/system/bootdevice.c
@@ -101,20 +101,23 @@  void validate_bootdevices(const char *devices, Error **errp)
 void restore_boot_order(void *opaque)
 {
     char *normal_boot_order = opaque;
-    static int first = 1;
+    static int bootcount = 0;
 
-    /* Restore boot order and remove ourselves after the first boot */
-    if (first) {
-        first = 0;
+    switch (bootcount++) {
+    case 0:
+        /* First boot: use the one-time config */
+        return;
+    case 1:
+        /* Second boot: restore normal boot order */
+        if (boot_set_handler) {
+            qemu_boot_set(normal_boot_order, &error_abort);
+        }
+        g_free(normal_boot_order);
+        return;
+    default:
+        /* Subsequent boots: keep using normal boot order */
         return;
     }
-
-    if (boot_set_handler) {
-        qemu_boot_set(normal_boot_order, &error_abort);
-    }
-
-    qemu_unregister_reset(restore_boot_order, normal_boot_order);
-    g_free(normal_boot_order);
 }
 
 void check_boot_index(int32_t bootindex, Error **errp)