| Message ID | 20240220160622.114437-1-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | reset: Make whole system three-phase-reset aware | expand |
On Tue, Feb 20, 2024 at 04:06:12PM +0000, Peter Maydell wrote: > This patchset is an incremental improvement to our reset handling that > tries to roll out the "three-phase-reset" design we have for devices > to a wider scope. > > At the moment devices and buses have a three-phase reset system, with > separate 'enter', 'hold' and 'exit' phases. When the qbus tree is > reset, first all the devices on it have their 'enter' method called, > then they all have 'enter' called, and finally 'exit'. The idea is > that we can use this, among other things, as a way to resolve annoying > "this bit of reset work needs to happen before this other bit of reset > work" ordering issues. > > However, there is still a "legacy" reset option, where you register a > callback function with qemu_register_reset(). These functions know > nothing about the three-phase system, and "reset the qbus" is just one > of the things set up to happen at reset via qemu_register_reset(). > That means that a qemu_register_reset() function might happen before > all the enter/hold/exit phases of device reset, or it might happen after > all of them. > > This patchset provides a new way to register a three-phase-aware reset > in this list of "reset the whole system" actions, and reimplements > qemu_register_reset() in terms of that new mechanism. This means that > qemu_register_reset() functions are now all called in the 'hold' phase > of system reset. (This is an ordering change, so in theory it could > introduce new bugs if we are accidentally depending on the current > ordering; but we have very few enter and exit phase methods at the > moment so I don't expect much trouble, if any.) > > The first three patches remove the only two places in the codebase > that rely on "a reset callback can unregister itself within the > callback"; this is awkward to continue to support in the new > implementation, and an unusual thing to do given that reset is in > principle supposed to be something you can do as many times as you > like, not something that behaves differently the first time through. > > Patch 4 is an improvement to the QOM macros that's been on list as an > RFC already. > Patches 5 and 6 are the "new mechanism": qemu_register_resettable() > takes any object that implements the Resettable interface. System > reset is then doing 3-phase reset on all of these, so everything > gets its 'enter' phase called, then everything gets 'hold', then > everything gets 'exit'. > > Patch 7 reimplements the qemu_register_reset() API to be > "qemu_register_resettable(), and the callback function is called > in the 'hold' phase". > > Patch 8 makes the first real use of the new API: instead of > doing the qbus reset via qemu_register_reset(), we pass the > root of the qbus to qemu_register_resettable(). (This is where > the ordering change I mention above happens, as device enter and > exit method calls will now happen before and after all the > qemu_register_reset() function callbacks, rather than in the > middle of them.) > > Finally, patch 9 updates the developer docs to describe how a > complete system reset currently works. > > This series doesn't actually do a great deal as it stands, but I > think it's a necessary foundation for some cleanups: > * the vfio reset ordering problem discussed on list a while back > should now hopefully be solvable by having the vfio code use > qemu_register_resettable() so it can arrange to do the "needs to > happen last" stuff in an exit phase method > * there are some other missing pieces here, but eventually I think > it should be possible to get rid of the workarounds for > dependencies between ROM image loading and CPUs that want to > read an initial PC/SP on reset (eg arm, m68k) > I also think it's a good idea to get it into the tree so that we > have a chance to see if there are any unexpected regressions > before we start putting in bugfixes etc that depend on it :-) > > After this, I think the next thing I'm going to look at is whether > we can move the MachineState class from inheriting from TYPE_OBJECT > to TYPE_DEVICE. This would allow us to have machine-level reset > (and would bring "machine as a container of devices" into line > with "SoC object as container of devices"). > > thanks > -- PMM Like this overall and the pc cleanup is nice. Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Peter Maydell (10): > hw/i386: Store pointers to IDE buses in PCMachineState > hw/i386/pc: Do pc_cmos_init_late() from pc_machine_done() > system/bootdevice: Don't unregister reset handler in > restore_boot_order() > include/qom/object.h: New OBJECT_DEFINE_SIMPLE_TYPE{,_WITH_INTERFACES} > macros > hw/core: Add documentation and license comments to reset.h > hw/core: Add ResetContainer which holds objects implementing > Resettable > hw/core/reset: Add qemu_{register,unregister}_resettable() > hw/core/reset: Implement qemu_register_reset via > qemu_register_resettable > hw/core/machine: Use qemu_register_resettable for sysbus reset > docs/devel/reset: Update to discuss system reset > > MAINTAINERS | 10 ++ > docs/devel/qom.rst | 34 ++++++- > docs/devel/reset.rst | 44 +++++++- > include/hw/core/resetcontainer.h | 48 +++++++++ > include/hw/i386/pc.h | 4 +- > include/qom/object.h | 114 ++++++++++++++++----- > include/sysemu/reset.h | 113 +++++++++++++++++++++ > hw/core/machine.c | 7 +- > hw/core/reset.c | 166 +++++++++++++++++++++++++------ > hw/core/resetcontainer.c | 76 ++++++++++++++ > hw/i386/pc.c | 40 +++----- > hw/i386/pc_piix.c | 16 ++- > hw/i386/pc_q35.c | 9 +- > system/bootdevice.c | 25 +++-- > hw/core/meson.build | 1 + > 15 files changed, 587 insertions(+), 120 deletions(-) > create mode 100644 include/hw/core/resetcontainer.h > create mode 100644 hw/core/resetcontainer.c > > -- > 2.34.1
On 20/02/2024 16:06, Peter Maydell wrote: > This patchset is an incremental improvement to our reset handling that > tries to roll out the "three-phase-reset" design we have for devices > to a wider scope. > > At the moment devices and buses have a three-phase reset system, with > separate 'enter', 'hold' and 'exit' phases. When the qbus tree is > reset, first all the devices on it have their 'enter' method called, > then they all have 'enter' called, and finally 'exit'. The idea is > that we can use this, among other things, as a way to resolve annoying > "this bit of reset work needs to happen before this other bit of reset > work" ordering issues. > > However, there is still a "legacy" reset option, where you register a > callback function with qemu_register_reset(). These functions know > nothing about the three-phase system, and "reset the qbus" is just one > of the things set up to happen at reset via qemu_register_reset(). > That means that a qemu_register_reset() function might happen before > all the enter/hold/exit phases of device reset, or it might happen after > all of them. > > This patchset provides a new way to register a three-phase-aware reset > in this list of "reset the whole system" actions, and reimplements > qemu_register_reset() in terms of that new mechanism. This means that > qemu_register_reset() functions are now all called in the 'hold' phase > of system reset. (This is an ordering change, so in theory it could > introduce new bugs if we are accidentally depending on the current > ordering; but we have very few enter and exit phase methods at the > moment so I don't expect much trouble, if any.) > > The first three patches remove the only two places in the codebase > that rely on "a reset callback can unregister itself within the > callback"; this is awkward to continue to support in the new > implementation, and an unusual thing to do given that reset is in > principle supposed to be something you can do as many times as you > like, not something that behaves differently the first time through. > > Patch 4 is an improvement to the QOM macros that's been on list as an > RFC already. > Patches 5 and 6 are the "new mechanism": qemu_register_resettable() > takes any object that implements the Resettable interface. System > reset is then doing 3-phase reset on all of these, so everything > gets its 'enter' phase called, then everything gets 'hold', then > everything gets 'exit'. > > Patch 7 reimplements the qemu_register_reset() API to be > "qemu_register_resettable(), and the callback function is called > in the 'hold' phase". > > Patch 8 makes the first real use of the new API: instead of > doing the qbus reset via qemu_register_reset(), we pass the > root of the qbus to qemu_register_resettable(). (This is where > the ordering change I mention above happens, as device enter and > exit method calls will now happen before and after all the > qemu_register_reset() function callbacks, rather than in the > middle of them.) > > Finally, patch 9 updates the developer docs to describe how a > complete system reset currently works. > > This series doesn't actually do a great deal as it stands, but I > think it's a necessary foundation for some cleanups: > * the vfio reset ordering problem discussed on list a while back > should now hopefully be solvable by having the vfio code use > qemu_register_resettable() so it can arrange to do the "needs to > happen last" stuff in an exit phase method > * there are some other missing pieces here, but eventually I think > it should be possible to get rid of the workarounds for > dependencies between ROM image loading and CPUs that want to > read an initial PC/SP on reset (eg arm, m68k) Absolutely, this would definitely help with m68k :) > I also think it's a good idea to get it into the tree so that we > have a chance to see if there are any unexpected regressions > before we start putting in bugfixes etc that depend on it :-) > > After this, I think the next thing I'm going to look at is whether > we can move the MachineState class from inheriting from TYPE_OBJECT > to TYPE_DEVICE. This would allow us to have machine-level reset > (and would bring "machine as a container of devices" into line > with "SoC object as container of devices"). This would be really useful! In particular, moving MachineState to inherit from TYPE_DEVICE will allow setting MemoryRegion owners to the machine itself for arbitrary registers implemented by the board. > thanks > -- PMM > > Peter Maydell (10): > hw/i386: Store pointers to IDE buses in PCMachineState > hw/i386/pc: Do pc_cmos_init_late() from pc_machine_done() > system/bootdevice: Don't unregister reset handler in > restore_boot_order() > include/qom/object.h: New OBJECT_DEFINE_SIMPLE_TYPE{,_WITH_INTERFACES} > macros > hw/core: Add documentation and license comments to reset.h > hw/core: Add ResetContainer which holds objects implementing > Resettable > hw/core/reset: Add qemu_{register,unregister}_resettable() > hw/core/reset: Implement qemu_register_reset via > qemu_register_resettable > hw/core/machine: Use qemu_register_resettable for sysbus reset > docs/devel/reset: Update to discuss system reset > > MAINTAINERS | 10 ++ > docs/devel/qom.rst | 34 ++++++- > docs/devel/reset.rst | 44 +++++++- > include/hw/core/resetcontainer.h | 48 +++++++++ > include/hw/i386/pc.h | 4 +- > include/qom/object.h | 114 ++++++++++++++++----- > include/sysemu/reset.h | 113 +++++++++++++++++++++ > hw/core/machine.c | 7 +- > hw/core/reset.c | 166 +++++++++++++++++++++++++------ > hw/core/resetcontainer.c | 76 ++++++++++++++ > hw/i386/pc.c | 40 +++----- > hw/i386/pc_piix.c | 16 ++- > hw/i386/pc_q35.c | 9 +- > system/bootdevice.c | 25 +++-- > hw/core/meson.build | 1 + > 15 files changed, 587 insertions(+), 120 deletions(-) > create mode 100644 include/hw/core/resetcontainer.h > create mode 100644 hw/core/resetcontainer.c ATB, Mark.
On Tue, 20 Feb 2024 at 16:06, Peter Maydell <peter.maydell@linaro.org> wrote: > > This patchset is an incremental improvement to our reset handling that > tries to roll out the "three-phase-reset" design we have for devices > to a wider scope. The first two patches here are already upstream; I'm going to take the rest via target-arm.next so we can get it into git well before softfreeze and have some time to shake out any unexpected consequences. thanks -- PMM
This patchset is an incremental improvement to our reset handling that tries to roll out the "three-phase-reset" design we have for devices to a wider scope. At the moment devices and buses have a three-phase reset system, with separate 'enter', 'hold' and 'exit' phases. When the qbus tree is reset, first all the devices on it have their 'enter' method called, then they all have 'enter' called, and finally 'exit'. The idea is that we can use this, among other things, as a way to resolve annoying "this bit of reset work needs to happen before this other bit of reset work" ordering issues. However, there is still a "legacy" reset option, where you register a callback function with qemu_register_reset(). These functions know nothing about the three-phase system, and "reset the qbus" is just one of the things set up to happen at reset via qemu_register_reset(). That means that a qemu_register_reset() function might happen before all the enter/hold/exit phases of device reset, or it might happen after all of them. This patchset provides a new way to register a three-phase-aware reset in this list of "reset the whole system" actions, and reimplements qemu_register_reset() in terms of that new mechanism. This means that qemu_register_reset() functions are now all called in the 'hold' phase of system reset. (This is an ordering change, so in theory it could introduce new bugs if we are accidentally depending on the current ordering; but we have very few enter and exit phase methods at the moment so I don't expect much trouble, if any.) The first three patches remove the only two places in the codebase that rely on "a reset callback can unregister itself within the callback"; this is awkward to continue to support in the new implementation, and an unusual thing to do given that reset is in principle supposed to be something you can do as many times as you like, not something that behaves differently the first time through. Patch 4 is an improvement to the QOM macros that's been on list as an RFC already. Patches 5 and 6 are the "new mechanism": qemu_register_resettable() takes any object that implements the Resettable interface. System reset is then doing 3-phase reset on all of these, so everything gets its 'enter' phase called, then everything gets 'hold', then everything gets 'exit'. Patch 7 reimplements the qemu_register_reset() API to be "qemu_register_resettable(), and the callback function is called in the 'hold' phase". Patch 8 makes the first real use of the new API: instead of doing the qbus reset via qemu_register_reset(), we pass the root of the qbus to qemu_register_resettable(). (This is where the ordering change I mention above happens, as device enter and exit method calls will now happen before and after all the qemu_register_reset() function callbacks, rather than in the middle of them.) Finally, patch 9 updates the developer docs to describe how a complete system reset currently works. This series doesn't actually do a great deal as it stands, but I think it's a necessary foundation for some cleanups: * the vfio reset ordering problem discussed on list a while back should now hopefully be solvable by having the vfio code use qemu_register_resettable() so it can arrange to do the "needs to happen last" stuff in an exit phase method * there are some other missing pieces here, but eventually I think it should be possible to get rid of the workarounds for dependencies between ROM image loading and CPUs that want to read an initial PC/SP on reset (eg arm, m68k) I also think it's a good idea to get it into the tree so that we have a chance to see if there are any unexpected regressions before we start putting in bugfixes etc that depend on it :-) After this, I think the next thing I'm going to look at is whether we can move the MachineState class from inheriting from TYPE_OBJECT to TYPE_DEVICE. This would allow us to have machine-level reset (and would bring "machine as a container of devices" into line with "SoC object as container of devices"). thanks -- PMM Peter Maydell (10): hw/i386: Store pointers to IDE buses in PCMachineState hw/i386/pc: Do pc_cmos_init_late() from pc_machine_done() system/bootdevice: Don't unregister reset handler in restore_boot_order() include/qom/object.h: New OBJECT_DEFINE_SIMPLE_TYPE{,_WITH_INTERFACES} macros hw/core: Add documentation and license comments to reset.h hw/core: Add ResetContainer which holds objects implementing Resettable hw/core/reset: Add qemu_{register,unregister}_resettable() hw/core/reset: Implement qemu_register_reset via qemu_register_resettable hw/core/machine: Use qemu_register_resettable for sysbus reset docs/devel/reset: Update to discuss system reset MAINTAINERS | 10 ++ docs/devel/qom.rst | 34 ++++++- docs/devel/reset.rst | 44 +++++++- include/hw/core/resetcontainer.h | 48 +++++++++ include/hw/i386/pc.h | 4 +- include/qom/object.h | 114 ++++++++++++++++----- include/sysemu/reset.h | 113 +++++++++++++++++++++ hw/core/machine.c | 7 +- hw/core/reset.c | 166 +++++++++++++++++++++++++------ hw/core/resetcontainer.c | 76 ++++++++++++++ hw/i386/pc.c | 40 +++----- hw/i386/pc_piix.c | 16 ++- hw/i386/pc_q35.c | 9 +- system/bootdevice.c | 25 +++-- hw/core/meson.build | 1 + 15 files changed, 587 insertions(+), 120 deletions(-) create mode 100644 include/hw/core/resetcontainer.h create mode 100644 hw/core/resetcontainer.c