Hi Peter,
On Tue, Aug 20, 2024 at 1:56 PM Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> On Tue, 20 Aug 2024 at 12:40, David Hildenbrand <da...@redhat.com> wrote:
> >
> > On 14.08.24 14:32, Juraj Marcin wrote:
> > > 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...
> >
> > One thing one could consider would probably be to send a WARM reset to
> > all devices. The main issue here is that other devices will default to a
> > COLD device then, and that's precisely what the other machines that
> > implement suspend+resume do not want. And ...
> >
> > >
> > > 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.
> >
> > ... if the machine does not do any resets during suspend+wakeup, this
> > implies that there is not even a warm reset.
> >
> > I guess we should make that clearer in the documentation: it's up to a
> > machine implementation whether it wants to trigger a WARM reset during
> > suspend+wakeup. If not, not resets will be performed at all.
> >
> > @Peter, does that sound reasonable?
>
> Well, so far we've established that we need a "WAKEUP" reset
> type, but not that we need a "WARM" reset type. The latter
> would be something we'd need to trigger for quite a lot of
> reset-causes where we currently default to COLD reset, so
> I don't think we should do that until we need it.
>
> If suspend-and-wakeup really is supposed to be a reset event
> on some machines but not on others, that sounds unhelpfully
> nonstandard, but then hardware designers rarely make choices
> to make our lives easier :-) And yes, we should make sure
> that's clear in the documentation.
I have rewritten the documentation section to make it more explicit
that the reset might not happen. I would appreciate feedback if some
part still needs some care or if it is clear now.
If the machine supports waking up from a suspended state and needs to
reset its devices during wake-up (from ``MachineClass::wakeup()``
method), this reset type should be used for such a request. Devices
can utilize this reset type to differentiate the reset requested
during machine wake-up from other reset requests. For example, a
virtio-mem device must not unplug its memory blocks during wake-up as
the contents of the guest RAM would get lost. However, this reset type
should not be used for wake-up detection, as not every machine type
issues a device reset request during wake-up.
>
> I think with adding new reset events it's going to be
> important that we're clear about what they are (and in
> particular what the triggering events that cause them
> are) so that we have a solid guide for what it means.
> The thing in particular I'm hoping to avoid here is that
> we vaguely define, for example, a "warm" reset type and then
> different parts of the system interpret "warm" in different
> ways.
>
> thanks
> -- PMM
>
Thank you!
--
Juraj Marcin
