| Message ID | 20240220160622.114437-10-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reset: Make whole system three-phase-reset aware | expand |
On 20/2/24 17:06, Peter Maydell wrote: > Move the reset of the sysbus (and thus all devices and buses anywhere > on the qbus tree) from qemu_register_reset() to qemu_register_resettable(). > > This is a behaviour change: because qemu_register_resettable() is > aware of three-phase reset, this now means that: > * 'enter' phase reset methods of devices and buses are called > before any legacy reset callbacks registered with qemu_register_reset() > * 'exit' phase reset methods of devices and buses are called > after any legacy qemu_register_reset() callbacks > > Put another way, a qemu_register_reset() callback is now correctly > ordered in the 'hold' phase along with any other 'hold' phase methods. > > The motivation for doing this is that we will now be able to resolve > some reset-ordering issues using the three-phase mechanism, because > the 'exit' phase is always after the 'hold' phase, even when the > 'hold' phase function was registered with qemu_register_reset(). > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > I believe that given we don't make much use of enter/exit phases > currently that this is unlikely to cause unexpected regressions due > to an accidental reset-order dependency that is no longer satisfied, > but it's always possible... > --- > hw/core/machine.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index fb5afdcae4c..9ac5d5389a6 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -1577,14 +1577,13 @@ void qdev_machine_creation_done(void) > /* TODO: once all bus devices are qdevified, this should be done > * when bus is created by qdev.c */ > /* > - * TODO: If we had a main 'reset container' that the whole system > - * lived in, we could reset that using the multi-phase reset > - * APIs. For the moment, we just reset the sysbus, which will cause > + * This is where we arrange for the sysbus to be reset when the > + * whole simulation is reset. In turn, resetting the sysbus will cause > * all devices hanging off it (and all their child buses, recursively) > * to be reset. Note that this will *not* reset any Device objects > * which are not attached to some part of the qbus tree! > */ > - qemu_register_reset(resettable_cold_reset_fn, sysbus_get_default()); Interestingly after this patch TYPE_S390_IPL is the last device using resettable_cold_reset_fn(). Per commit cd45c506c8e: /* * Because this Device is not on any bus in the qbus tree (it is * not a sysbus device and it's not on some other bus like a PCI * bus) it will not be automatically reset by the 'reset the * sysbus' hook registered by vl.c like most devices. So we must * manually register a reset hook for it. * TODO: there should be a better way to do this. */ > + qemu_register_resettable(OBJECT(sysbus_get_default())); > > notifier_list_notify(&machine_init_done_notifiers, NULL); >
On Tue, 20 Feb 2024 at 19:06, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 20/2/24 17:06, Peter Maydell wrote: > > Move the reset of the sysbus (and thus all devices and buses anywhere > > on the qbus tree) from qemu_register_reset() to qemu_register_resettable(). > > > > This is a behaviour change: because qemu_register_resettable() is > > aware of three-phase reset, this now means that: > > * 'enter' phase reset methods of devices and buses are called > > before any legacy reset callbacks registered with qemu_register_reset() > > * 'exit' phase reset methods of devices and buses are called > > after any legacy qemu_register_reset() callbacks > > > > Put another way, a qemu_register_reset() callback is now correctly > > ordered in the 'hold' phase along with any other 'hold' phase methods. > > > > The motivation for doing this is that we will now be able to resolve > > some reset-ordering issues using the three-phase mechanism, because > > the 'exit' phase is always after the 'hold' phase, even when the > > 'hold' phase function was registered with qemu_register_reset(). > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > I believe that given we don't make much use of enter/exit phases > > currently that this is unlikely to cause unexpected regressions due > > to an accidental reset-order dependency that is no longer satisfied, > > but it's always possible... > > --- > > hw/core/machine.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > index fb5afdcae4c..9ac5d5389a6 100644 > > --- a/hw/core/machine.c > > +++ b/hw/core/machine.c > > @@ -1577,14 +1577,13 @@ void qdev_machine_creation_done(void) > > /* TODO: once all bus devices are qdevified, this should be done > > * when bus is created by qdev.c */ > > /* > > - * TODO: If we had a main 'reset container' that the whole system > > - * lived in, we could reset that using the multi-phase reset > > - * APIs. For the moment, we just reset the sysbus, which will cause > > + * This is where we arrange for the sysbus to be reset when the > > + * whole simulation is reset. In turn, resetting the sysbus will cause > > * all devices hanging off it (and all their child buses, recursively) > > * to be reset. Note that this will *not* reset any Device objects > > * which are not attached to some part of the qbus tree! > > */ > > - qemu_register_reset(resettable_cold_reset_fn, sysbus_get_default()); > > Interestingly after this patch TYPE_S390_IPL is the last device > using resettable_cold_reset_fn(). Per commit cd45c506c8e: > > /* > * Because this Device is not on any bus in the qbus tree (it is > * not a sysbus device and it's not on some other bus like a PCI > * bus) it will not be automatically reset by the 'reset the > * sysbus' hook registered by vl.c like most devices. So we must > * manually register a reset hook for it. > * TODO: there should be a better way to do this. > */ Mmm, we could now have that s390 code call qemu_register_resettable(OBJECT(dev)). Though the "better way" remark still applies, because ideally we shouldn't be doing reset only via the qbus tree. thanks -- PMM
On 2/20/24 06:06, Peter Maydell wrote: > Move the reset of the sysbus (and thus all devices and buses anywhere > on the qbus tree) from qemu_register_reset() to qemu_register_resettable(). > > This is a behaviour change: because qemu_register_resettable() is > aware of three-phase reset, this now means that: > * 'enter' phase reset methods of devices and buses are called > before any legacy reset callbacks registered with qemu_register_reset() > * 'exit' phase reset methods of devices and buses are called > after any legacy qemu_register_reset() callbacks > > Put another way, a qemu_register_reset() callback is now correctly > ordered in the 'hold' phase along with any other 'hold' phase methods. > > The motivation for doing this is that we will now be able to resolve > some reset-ordering issues using the three-phase mechanism, because > the 'exit' phase is always after the 'hold' phase, even when the > 'hold' phase function was registered with qemu_register_reset(). > > Signed-off-by: Peter Maydell<peter.maydell@linaro.org> > --- > I believe that given we don't make much use of enter/exit phases > currently that this is unlikely to cause unexpected regressions due > to an accidental reset-order dependency that is no longer satisfied, > but it's always possible... > --- > hw/core/machine.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 20/2/24 17:06, Peter Maydell wrote: > Move the reset of the sysbus (and thus all devices and buses anywhere > on the qbus tree) from qemu_register_reset() to qemu_register_resettable(). > > This is a behaviour change: because qemu_register_resettable() is > aware of three-phase reset, this now means that: > * 'enter' phase reset methods of devices and buses are called > before any legacy reset callbacks registered with qemu_register_reset() > * 'exit' phase reset methods of devices and buses are called > after any legacy qemu_register_reset() callbacks > > Put another way, a qemu_register_reset() callback is now correctly > ordered in the 'hold' phase along with any other 'hold' phase methods. > > The motivation for doing this is that we will now be able to resolve > some reset-ordering issues using the three-phase mechanism, because > the 'exit' phase is always after the 'hold' phase, even when the > 'hold' phase function was registered with qemu_register_reset(). > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > I believe that given we don't make much use of enter/exit phases > currently that this is unlikely to cause unexpected regressions due > to an accidental reset-order dependency that is no longer satisfied, > but it's always possible... > --- > hw/core/machine.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index fb5afdcae4c..9ac5d5389a6 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -1577,14 +1577,13 @@ void qdev_machine_creation_done(void) > /* TODO: once all bus devices are qdevified, this should be done > * when bus is created by qdev.c */ > /* > - * TODO: If we had a main 'reset container' that the whole system > - * lived in, we could reset that using the multi-phase reset > - * APIs. For the moment, we just reset the sysbus, which will cause > + * This is where we arrange for the sysbus to be reset when the > + * whole simulation is reset. In turn, resetting the sysbus will cause > * all devices hanging off it (and all their child buses, recursively) > * to be reset. Note that this will *not* reset any Device objects > * which are not attached to some part of the qbus tree! > */ > - qemu_register_reset(resettable_cold_reset_fn, sysbus_get_default()); > + qemu_register_resettable(OBJECT(sysbus_get_default())); Correct, so: Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> But I'd rather move that call to main_system_bus_create() in hw/core/sysbus.c, as it doesn't seem to be related to the machine creation phases anymore. Maybe clearer to do in a separate patch although. > notifier_list_notify(&machine_init_done_notifiers, NULL); >
On Tue, Feb 20, 2024 at 04:06:21PM +0000, Peter Maydell wrote: > Date: Tue, 20 Feb 2024 16:06:21 +0000 > From: Peter Maydell <peter.maydell@linaro.org> > Subject: [PATCH 09/10] hw/core/machine: Use qemu_register_resettable for > sysbus reset > X-Mailer: git-send-email 2.34.1 > > Move the reset of the sysbus (and thus all devices and buses anywhere > on the qbus tree) from qemu_register_reset() to qemu_register_resettable(). > > This is a behaviour change: because qemu_register_resettable() is > aware of three-phase reset, this now means that: > * 'enter' phase reset methods of devices and buses are called > before any legacy reset callbacks registered with qemu_register_reset() > * 'exit' phase reset methods of devices and buses are called > after any legacy qemu_register_reset() callbacks > > Put another way, a qemu_register_reset() callback is now correctly > ordered in the 'hold' phase along with any other 'hold' phase methods. > > The motivation for doing this is that we will now be able to resolve > some reset-ordering issues using the three-phase mechanism, because > the 'exit' phase is always after the 'hold' phase, even when the > 'hold' phase function was registered with qemu_register_reset(). > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > I believe that given we don't make much use of enter/exit phases > currently that this is unlikely to cause unexpected regressions due > to an accidental reset-order dependency that is no longer satisfied, > but it's always possible... > --- > hw/core/machine.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
diff --git a/hw/core/machine.c b/hw/core/machine.c index fb5afdcae4c..9ac5d5389a6 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -1577,14 +1577,13 @@ void qdev_machine_creation_done(void) /* TODO: once all bus devices are qdevified, this should be done * when bus is created by qdev.c */ /* - * TODO: If we had a main 'reset container' that the whole system - * lived in, we could reset that using the multi-phase reset - * APIs. For the moment, we just reset the sysbus, which will cause + * This is where we arrange for the sysbus to be reset when the + * whole simulation is reset. In turn, resetting the sysbus will cause * all devices hanging off it (and all their child buses, recursively) * to be reset. Note that this will *not* reset any Device objects * which are not attached to some part of the qbus tree! */ - qemu_register_reset(resettable_cold_reset_fn, sysbus_get_default()); + qemu_register_resettable(OBJECT(sysbus_get_default())); notifier_list_notify(&machine_init_done_notifiers, NULL);
Move the reset of the sysbus (and thus all devices and buses anywhere on the qbus tree) from qemu_register_reset() to qemu_register_resettable(). This is a behaviour change: because qemu_register_resettable() is aware of three-phase reset, this now means that: * 'enter' phase reset methods of devices and buses are called before any legacy reset callbacks registered with qemu_register_reset() * 'exit' phase reset methods of devices and buses are called after any legacy qemu_register_reset() callbacks Put another way, a qemu_register_reset() callback is now correctly ordered in the 'hold' phase along with any other 'hold' phase methods. The motivation for doing this is that we will now be able to resolve some reset-ordering issues using the three-phase mechanism, because the 'exit' phase is always after the 'hold' phase, even when the 'hold' phase function was registered with qemu_register_reset(). Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- I believe that given we don't make much use of enter/exit phases currently that this is unlikely to cause unexpected regressions due to an accidental reset-order dependency that is no longer satisfied, but it's always possible... --- hw/core/machine.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)