| Message ID | 20240813153922.311788-3-jmarcin@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-mem: Implement support for suspend+wake-up with plugged memory | expand |
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
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 >
On 14.08.24 14:32, Juraj Marcin wrote: > 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... 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?
On Tue, 20 Aug 2024 at 12:40, David Hildenbrand <david@redhat.com> wrote: > > On 14.08.24 14:32, Juraj Marcin wrote: > > 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... > > 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 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
On 20.08.24 13:56, Peter Maydell wrote: > On Tue, 20 Aug 2024 at 12:40, David Hildenbrand <david@redhat.com> wrote: >> >> On 14.08.24 14:32, Juraj Marcin wrote: >>> 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... >> >> 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 Sorry, I meant "WAKEUP" ... :) > 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 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. Okay, so we should make it clear that a WAKEUP reset may be used on machines (especially x86) during a wakeup after suspending, when most of the devices are supposed to just do the same as a COLD reset, and only some of them (especially memory-layout/content related) need to preserve their state. The primary reason of the WAKEUP event is for these devices to handle these wakeups properly. Then, we should document that some machines might not perform any reset during a wakeup, which will result in no resets to be triggered at all.
Hi Peter, On Tue, Aug 20, 2024 at 1:56 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 20 Aug 2024 at 12:40, David Hildenbrand <david@redhat.com> wrote: > > > > On 14.08.24 14:32, Juraj Marcin wrote: > > > 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... > > > > 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!
> 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. Sounds good to me. I'd probably generalize the virtio-mem bit to: "For example, RAM content must not be lost during wake-up, and memory devices like virtio-mem that provide additional RAM must not reset such state during wake-ups, but might do so during cold resets." @Peter, WDYT?
On Thu, 29 Aug 2024 at 16:48, David Hildenbrand <david@redhat.com> wrote: > > > 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. > > Sounds good to me. > > I'd probably generalize the virtio-mem bit to: > > "For example, RAM content must not be lost during wake-up, and memory > devices like virtio-mem that provide additional RAM must not reset such > state during wake-ups, but might do so during cold resets." > > > @Peter, WDYT? Yep, seems good to me: I think it's clear about when this reset type happens and what you can/can't expect from it. Minor grammar nit: should be "from the ``MachineClass::wakeup()`` method". -- PMM
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(); } diff --git a/include/hw/resettable.h b/include/hw/resettable.h index 7e249deb8b..edb1f1361b 100644 --- a/include/hw/resettable.h +++ b/include/hw/resettable.h @@ -29,6 +29,7 @@ typedef struct ResettableState ResettableState; * Types of reset. * * + Cold: reset resulting from a power cycle of the object. + * + Wakeup: reset resulting from a wake-up from a suspended state. * * TODO: Support has to be added to handle more types. In particular, * ResettableState structure needs to be expanded. @@ -36,6 +37,7 @@ typedef struct ResettableState ResettableState; typedef enum ResetType { RESET_TYPE_COLD, RESET_TYPE_SNAPSHOT_LOAD, + RESET_TYPE_WAKEUP, } ResetType; /*