On Tue, 13 Aug 2024 at 16:39, Juraj Marcin <jmar...@redhat.com> wrote:
>
> Some devices need to distinguish cold start reset from waking up from a
> suspended state. This patch adds new value to the enum, and updates the
> i386 wakeup method to use this new reset type.
>
> Signed-off-by: Juraj Marcin <jmar...@redhat.com>
> Reviewed-by: David Hildenbrand <da...@redhat.com>
> ---
> docs/devel/reset.rst | 8 ++++++++
> hw/i386/pc.c | 2 +-
> include/hw/resettable.h | 2 ++
> 3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst
> index 9746a4e8a0..a7c9467313 100644
> --- a/docs/devel/reset.rst
> +++ b/docs/devel/reset.rst
> @@ -44,6 +44,14 @@ The Resettable interface handles reset types with an enum
> ``ResetType``:
> value on each cold reset, such as RNG seed information, and which they
> must not reinitialize on a snapshot-load reset.
>
> +``RESET_TYPE_WAKEUP``
> + This type is called for a reset when the system is being woken-up from a
> + suspended state using the ``qemu_system_wakeup()`` function. If the machine
> + needs to reset its devices in its ``MachineClass::wakeup()`` method, this
> + reset type should be used, so devices can differentiate system wake-up from
> + other reset types. For example, a virtio-mem device must not unplug its
> + memory during wake-up as that would clear the guest RAM.
> +
> Devices which implement reset methods must treat any unknown ``ResetType``
> as equivalent to ``RESET_TYPE_COLD``; this will reduce the amount of
> existing code we need to change if we add more types in future.
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index ccb9731c91..49efd0a997 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1716,7 +1716,7 @@ static void pc_machine_reset(MachineState *machine,
> ResetType type)
> static void pc_machine_wakeup(MachineState *machine)
> {
> cpu_synchronize_all_states();
> - pc_machine_reset(machine, RESET_TYPE_COLD);
> + pc_machine_reset(machine, RESET_TYPE_WAKEUP);
> cpu_synchronize_all_post_reset();
> }
I'm happy (following discussion in the previous thread)
that 'wakeup' is the right reset event to be using here.
But looking at the existing code for qemu_system_wakeup()
something seems odd here. qemu_system_wakeup() calls
the MachineClass::wakeup method if it's set, and does
nothing if it's not. The PC implementation of that calls
pc_machine_reset(), which does a qemu_devices_reset(),
which does a complete three-phase reset of the system.
But if the machine doesn't implement wakeup then we
never reset the system at all.
Shouldn't qemu_system_wakeup() do a qemu_devices_reset()
if there's no MachineClass::wakeup, in a similar way to
how qemu_system_reset() does a qemu_devices_reset()
if there's no MachineClass::reset method ? Having the
wakeup event be "sometimes this will do a RESET_TYPE_WAKEUP
but sometimes it won't" doesn't seem right to me...
thanks
-- PMM
