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

-- 
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

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