Re: [PATCH v2 2/4] reset: Add RESET_TYPE_WAKEUP

[Peter Maydell]

On Tue, 13 Aug 2024 at 16:39, Juraj Marcin <jmarcin@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 <jmarcin@redhat.com>
> Reviewed-by: David Hildenbrand <david@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