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

[Juraj Marcin]

On Tue, Aug 13, 2024 at 6:37 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> 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...

>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