| Message ID | 20240220160622.114437-4-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | reset: Make whole system three-phase-reset aware | expand |
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~
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 --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)
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(-)