Re: [PATCH 00/10] reset: Make whole system three-phase-reset aware

[Michael S. Tsirkin]

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