On Tue, Aug 13, 2024 at 6:37 PM Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> 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...
>From my understanding that I have gathered from the code (but please,
someone correct me if I am wrong), this is machine specific. Some
machine types might not support suspend+wake-up at all. The support
has to be explicitly advertised through qemu_register_wakeup_support()
(for example, aarch64 with a generic virt machine type does not
advertise support). Even if the machine type advertises
suspend+wake-up support, it might not need to do anything machine
specific. This is the case of pSeries PowerPC machine (sPAPR) that
advertises support, but does not implement MachineClass::wakeup()
method as nothing needs to change in the machine state. [1]
So, if a restart during wake-up happens, it can be differentiated with
the wake-up reset type, and if the machine type does not need to reset
its devices during wake-up, there is no reset that needs to be
differentiated.
[1]:
https://gitlab.com/qemu-project/qemu/-/blob/a733f37aef3b7d1d33bfe2716af88cdfd67ba64e/hw/ppc/spapr.c?#L3155
>
> thanks
> -- PMM
>
--
Juraj Marcin
