diff mbox series

[2/4] reset: Add RESET_TYPE_WAKEUP

Message ID 20240806160756.182524-3-jmarcin@redhat.com
State New
Headers show
Series virtio-mem: Implement support for suspend+wake-up with plugged memory | expand

Commit Message

Juraj Marcin Aug. 6, 2024, 4:07 p.m. UTC
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>
---
 docs/devel/reset.rst    | 7 +++++++
 hw/i386/pc.c            | 2 +-
 include/hw/resettable.h | 2 ++
 3 files changed, 10 insertions(+), 1 deletion(-)

Comments

David Hildenbrand Aug. 6, 2024, 4:41 p.m. UTC | #1
On 06.08.24 18:07, Juraj Marcin 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>
> ---
>   docs/devel/reset.rst    | 7 +++++++
>   hw/i386/pc.c            | 2 +-
>   include/hw/resettable.h | 2 ++
>   3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst
> index 9746a4e8a0..30c9a0cc2b 100644
> --- a/docs/devel/reset.rst
> +++ b/docs/devel/reset.rst
> @@ -44,6 +44,13 @@ 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 used when the machine is woken up from a suspended state (deep
> +  sleep, suspend-to-ram). Devices that must not be reset to their initial state
> +  after wake-up (for example virtio-mem) can use this state to differentiate
> +  cold start from wake-up can use this state to differentiate cold start from
> +  wake-up.
> +
>   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.

^ That sounds like using this for x86 below should just work.

Reviewed-by: David Hildenbrand <david@redhat.com>
Peter Maydell Aug. 8, 2024, 12:18 p.m. UTC | #2
On Tue, 6 Aug 2024 at 17:08, 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>
> ---
>  docs/devel/reset.rst    | 7 +++++++
>  hw/i386/pc.c            | 2 +-
>  include/hw/resettable.h | 2 ++
>  3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst
> index 9746a4e8a0..30c9a0cc2b 100644
> --- a/docs/devel/reset.rst
> +++ b/docs/devel/reset.rst
> @@ -44,6 +44,13 @@ 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 used when the machine is woken up from a suspended state (deep
> +  sleep, suspend-to-ram). Devices that must not be reset to their initial state
> +  after wake-up (for example virtio-mem) can use this state to differentiate
> +  cold start from wake-up can use this state to differentiate cold start from
> +  wake-up.

I feel like this needs more clarity about what this is, since
as a reset type it's a general behaviour, not a machine
specific one. What exactly is "wakeup" and when does it happen?
How does it differ from what you might call a "warm" reset,
where the user pressed the front-panel reset button?
Why is virtio-mem in particular interesting here?

thanks
-- PMM
Juraj Marcin Aug. 8, 2024, 3:28 p.m. UTC | #3
On Thu, Aug 8, 2024 at 2:18 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 6 Aug 2024 at 17:08, 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>
> > ---
> >  docs/devel/reset.rst    | 7 +++++++
> >  hw/i386/pc.c            | 2 +-
> >  include/hw/resettable.h | 2 ++
> >  3 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst
> > index 9746a4e8a0..30c9a0cc2b 100644
> > --- a/docs/devel/reset.rst
> > +++ b/docs/devel/reset.rst
> > @@ -44,6 +44,13 @@ 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 used when the machine is woken up from a suspended state (deep
> > +  sleep, suspend-to-ram). Devices that must not be reset to their initial state
> > +  after wake-up (for example virtio-mem) can use this state to differentiate
> > +  cold start from wake-up can use this state to differentiate cold start from
> > +  wake-up.
>
> I feel like this needs more clarity about what this is, since
> as a reset type it's a general behaviour, not a machine
> specific one. What exactly is "wakeup" and when does it happen?
> How does it differ from what you might call a "warm" reset,
> where the user pressed the front-panel reset button?
> Why is virtio-mem in particular interesting here?

Thank you for the feedback!

I have rewritten the paragraph:

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 type needs to reset in its ``MachineClass::wakeup()`` method,
this reset type should be used so that 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.

Is it clearer? Thank you!

>
> thanks
> -- PMM
>

--

Juraj Marcin
David Hildenbrand Aug. 8, 2024, 3:31 p.m. UTC | #4
On 08.08.24 17:28, Juraj Marcin wrote:
> On Thu, Aug 8, 2024 at 2:18 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Tue, 6 Aug 2024 at 17:08, 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>
>>> ---
>>>   docs/devel/reset.rst    | 7 +++++++
>>>   hw/i386/pc.c            | 2 +-
>>>   include/hw/resettable.h | 2 ++
>>>   3 files changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst
>>> index 9746a4e8a0..30c9a0cc2b 100644
>>> --- a/docs/devel/reset.rst
>>> +++ b/docs/devel/reset.rst
>>> @@ -44,6 +44,13 @@ 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 used when the machine is woken up from a suspended state (deep
>>> +  sleep, suspend-to-ram). Devices that must not be reset to their initial state
>>> +  after wake-up (for example virtio-mem) can use this state to differentiate
>>> +  cold start from wake-up can use this state to differentiate cold start from
>>> +  wake-up.
>>
>> I feel like this needs more clarity about what this is, since
>> as a reset type it's a general behaviour, not a machine
>> specific one. What exactly is "wakeup" and when does it happen?
>> How does it differ from what you might call a "warm" reset,
>> where the user pressed the front-panel reset button?
>> Why is virtio-mem in particular interesting here?
> 
> Thank you for the feedback!
> 
> I have rewritten the paragraph:
> 
> 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 type needs to reset in its ``MachineClass::wakeup()`` method,
> this reset type should be used so that 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.
> 
> Is it clearer? Thank you!

Conceptually, if we want to avoid the "WAKEUP" terminology here, maybe 
we should consider talking about a WARM reset -- in contrast to a COLD one?

During a WARM reset, memory content is supposed to stay untouched, which 
is what we effectively want to achieve with virtio-mem.

Peter, what would be your preference?
Peter Maydell Aug. 8, 2024, 3:56 p.m. UTC | #5
On Thu, 8 Aug 2024 at 16:31, David Hildenbrand <david@redhat.com> wrote:
>
> On 08.08.24 17:28, Juraj Marcin wrote:
> > On Thu, Aug 8, 2024 at 2:18 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >>
> >> On Tue, 6 Aug 2024 at 17:08, Juraj Marcin <jmarcin@redhat.com> wrote:
> >>> +``RESET_TYPE_WAKEUP``
> >>> +  This type is used when the machine is woken up from a suspended state (deep
> >>> +  sleep, suspend-to-ram). Devices that must not be reset to their initial state
> >>> +  after wake-up (for example virtio-mem) can use this state to differentiate
> >>> +  cold start from wake-up can use this state to differentiate cold start from
> >>> +  wake-up.
> >>
> >> I feel like this needs more clarity about what this is, since
> >> as a reset type it's a general behaviour, not a machine
> >> specific one. What exactly is "wakeup" and when does it happen?
> >> How does it differ from what you might call a "warm" reset,
> >> where the user pressed the front-panel reset button?
> >> Why is virtio-mem in particular interesting here?
> >
> > Thank you for the feedback!
> >
> > I have rewritten the paragraph:
> >
> > 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 type needs to reset in its ``MachineClass::wakeup()`` method,
> > this reset type should be used so that 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.
> >
> > Is it clearer? Thank you!
>
> Conceptually, if we want to avoid the "WAKEUP" terminology here, maybe
> we should consider talking about a WARM reset -- in contrast to a COLD one?
>
> During a WARM reset, memory content is supposed to stay untouched, which
> is what we effectively want to achieve with virtio-mem.

Right, I guess that's my question -- is "WAKEUP" ever any
different from "WARM" reset? I think the latter is a more
common general concept.

On the other hand it looks like we already have the
concept in the runstate state machine of
RUN_STATE_SUSPENDED versus RUN_STATE_RUNNING, and so if we
define "WAKEUP" as "goes from SUSPENDED to RUNNING" that's
quite a clearly defined condition. Whereas WARM reset would
be a much wider range of things and ought to include for
instance "guest triggers a reset by writing to port 92"
and all the other SHUTDOWN_CAUSE_GUEST_RESET cases.
(Side note: do we document all these runstates and transitions
anywhere?)

For virtio-mem, on a guest-triggered reset, should it
(a) definitely not unplug all the hotplugged memory
(b) definitely unplug all the hotplugged memory
(c) we don't care?

If (a) then that seems to push towards calling all these
cases of a "warm" reset; if (b) then that would be a
reason to make "warm" and "wakeup" different.

thanks
-- PMM
David Hildenbrand Aug. 8, 2024, 4:04 p.m. UTC | #6
On 08.08.24 17:56, Peter Maydell wrote:
> On Thu, 8 Aug 2024 at 16:31, David Hildenbrand <david@redhat.com> wrote:
>>
>> On 08.08.24 17:28, Juraj Marcin wrote:
>>> On Thu, Aug 8, 2024 at 2:18 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>
>>>> On Tue, 6 Aug 2024 at 17:08, Juraj Marcin <jmarcin@redhat.com> wrote:
>>>>> +``RESET_TYPE_WAKEUP``
>>>>> +  This type is used when the machine is woken up from a suspended state (deep
>>>>> +  sleep, suspend-to-ram). Devices that must not be reset to their initial state
>>>>> +  after wake-up (for example virtio-mem) can use this state to differentiate
>>>>> +  cold start from wake-up can use this state to differentiate cold start from
>>>>> +  wake-up.
>>>>
>>>> I feel like this needs more clarity about what this is, since
>>>> as a reset type it's a general behaviour, not a machine
>>>> specific one. What exactly is "wakeup" and when does it happen?
>>>> How does it differ from what you might call a "warm" reset,
>>>> where the user pressed the front-panel reset button?
>>>> Why is virtio-mem in particular interesting here?
>>>
>>> Thank you for the feedback!
>>>
>>> I have rewritten the paragraph:
>>>
>>> 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 type needs to reset in its ``MachineClass::wakeup()`` method,
>>> this reset type should be used so that 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.
>>>
>>> Is it clearer? Thank you!
>>
>> Conceptually, if we want to avoid the "WAKEUP" terminology here, maybe
>> we should consider talking about a WARM reset -- in contrast to a COLD one?
>>
>> During a WARM reset, memory content is supposed to stay untouched, which
>> is what we effectively want to achieve with virtio-mem.
> 
> Right, I guess that's my question -- is "WAKEUP" ever any
> different from "WARM" reset? I think the latter is a more
> common general concept.
> 
> On the other hand it looks like we already have the
> concept in the runstate state machine of
> RUN_STATE_SUSPENDED versus RUN_STATE_RUNNING, and so if we
> define "WAKEUP" as "goes from SUSPENDED to RUNNING" that's
> quite a clearly defined condition.

Right.

> Whereas WARM reset would
> be a much wider range of things and ought to include for
> instance "guest triggers a reset by writing to port 92"
> and all the other SHUTDOWN_CAUSE_GUEST_RESET cases.
> (Side note: do we document all these runstates and transitions
> anywhere?)
> 
> For virtio-mem, on a guest-triggered reset, should it
> (a) definitely not unplug all the hotplugged memory
> (b) definitely unplug all the hotplugged memory
> (c) we don't care?

During ordinary system resets (COLD) where RAM content is not guaranteed 
to survive:

Effectively (b)

During special kexec-style resets (e.g., on s390x there is a difference) 
where RAM content must survive:

Effectively (a)


On implementing architectures (x86, arm64), kexec-style resets are 
really like warm resets. For example, when we trigger kexec->kdump we
must not suddenly lose the RAM content.

In that sense, at least virtio-mem wants to treat WARM and WAKEUP resets
alike. But I agree that simply because virtio-mem want sot treat them 
alike doesn't mean that we should represent in QEMU using a single reset 
type.

WARM reboots like s390x supports are rather stuff for the future (once 
s390x actually supports virtio-mem and could end up triggering it).

> 
> If (a) then that seems to push towards calling all these
> cases of a "warm" reset; if (b) then that would be a
> reason to make "warm" and "wakeup" different.

Likely different then.
Peter Maydell Aug. 8, 2024, 4:17 p.m. UTC | #7
On Thu, 8 Aug 2024 at 17:04, David Hildenbrand <david@redhat.com> wrote:
>
> On 08.08.24 17:56, Peter Maydell wrote:
> > Right, I guess that's my question -- is "WAKEUP" ever any
> > different from "WARM" reset? I think the latter is a more
> > common general concept.
> >
> > On the other hand it looks like we already have the
> > concept in the runstate state machine of
> > RUN_STATE_SUSPENDED versus RUN_STATE_RUNNING, and so if we
> > define "WAKEUP" as "goes from SUSPENDED to RUNNING" that's
> > quite a clearly defined condition.
>
> Right.
>
> > Whereas WARM reset would
> > be a much wider range of things and ought to include for
> > instance "guest triggers a reset by writing to port 92"
> > and all the other SHUTDOWN_CAUSE_GUEST_RESET cases.
> > (Side note: do we document all these runstates and transitions
> > anywhere?)
> >
> > For virtio-mem, on a guest-triggered reset, should it
> > (a) definitely not unplug all the hotplugged memory
> > (b) definitely unplug all the hotplugged memory
> > (c) we don't care?
>
> During ordinary system resets (COLD) where RAM content is not guaranteed
> to survive:

"COLD" isn't an "ordinary system reset", though -- COLD
reset is more like "I powered the computer off and then
turned it on again". A "WARM" reset is like "I pressed
the front panel reset button, or the watchdog device
fired and reset the system". We don't currently really
model front-panel or watchdog reset properly, we assume
that it's close enough to have it be the same as
power-off-and-on reset (and there are some kludges in
various places where it's not quite right).

> Effectively (b)
>
> During special kexec-style resets (e.g., on s390x there is a difference)
> where RAM content must survive:
>
> Effectively (a)
>
>
> On implementing architectures (x86, arm64), kexec-style resets are
> really like warm resets. For example, when we trigger kexec->kdump we
> must not suddenly lose the RAM content.

(There's an awkward conflict here with our rom blob
system, which currently does a "copy any guest images
back into RAM" on reset. Should we do that on warm
reset? For some usecases you want those original
images back again, but for "guest did a kexec" you
almost certainly don't...)

> In that sense, at least virtio-mem wants to treat WARM and WAKEUP resets
> alike. But I agree that simply because virtio-mem want sot treat them
> alike doesn't mean that we should represent in QEMU using a single reset
> type.

On the other hand, if virtio-mem does want to treat them
the same we could start with only implementing WARM and
not add WAKEUP until we have a use-case for it.

By the way, I assume this patchseries is 9.2 material,
not trying to fix a bug for this release ?

thanks
-- PMM
David Hildenbrand Aug. 8, 2024, 4:29 p.m. UTC | #8
On 08.08.24 18:17, Peter Maydell wrote:
> On Thu, 8 Aug 2024 at 17:04, David Hildenbrand <david@redhat.com> wrote:
>>
>> On 08.08.24 17:56, Peter Maydell wrote:
>>> Right, I guess that's my question -- is "WAKEUP" ever any
>>> different from "WARM" reset? I think the latter is a more
>>> common general concept.
>>>
>>> On the other hand it looks like we already have the
>>> concept in the runstate state machine of
>>> RUN_STATE_SUSPENDED versus RUN_STATE_RUNNING, and so if we
>>> define "WAKEUP" as "goes from SUSPENDED to RUNNING" that's
>>> quite a clearly defined condition.
>>
>> Right.
>>
>>> Whereas WARM reset would
>>> be a much wider range of things and ought to include for
>>> instance "guest triggers a reset by writing to port 92"
>>> and all the other SHUTDOWN_CAUSE_GUEST_RESET cases.
>>> (Side note: do we document all these runstates and transitions
>>> anywhere?)
>>>
>>> For virtio-mem, on a guest-triggered reset, should it
>>> (a) definitely not unplug all the hotplugged memory
>>> (b) definitely unplug all the hotplugged memory
>>> (c) we don't care?
>>
>> During ordinary system resets (COLD) where RAM content is not guaranteed
>> to survive:
> 
> "COLD" isn't an "ordinary system reset", though -- COLD
> reset is more like "I powered the computer off and then
> turned it on again". A "WARM" reset is like "I pressed
> the front panel reset button, or the watchdog device
> fired and reset the system". We don't currently really
> model front-panel or watchdog reset properly, we assume
> that it's close enough to have it be the same as
> power-off-and-on reset (and there are some kludges in
> various places where it's not quite right).

Agreed, there are many flavors of resets, even different flavors of warm 
ones I'm afraid.

To summarize, if a VM does an ordinary reset ("shutdown -r") we want to 
unplug all hotplugged memory. Same with most external resets we 
currently implement. In all other caes, we likely want to keep the 
memory hotplugged and RAM content alive.

I think we did something similar with DIMMs/ACPI devices with unplug 
requests in that past, but I'm not able to locate that code easily.

> 
>> Effectively (b)
>>
>> During special kexec-style resets (e.g., on s390x there is a difference)
>> where RAM content must survive:
>>
>> Effectively (a)
>>
>>
>> On implementing architectures (x86, arm64), kexec-style resets are
>> really like warm resets. For example, when we trigger kexec->kdump we
>> must not suddenly lose the RAM content.
> 
> (There's an awkward conflict here with our rom blob
> system, which currently does a "copy any guest images
> back into RAM" on reset. Should we do that on warm
> reset? For some usecases you want those original
> images back again, but for "guest did a kexec" you
> almost certainly don't...)

Right, maybe on some warm resets where the ROM blobs are required and 
might have gotten corrupted you would actually want to restore them, 
maybe? It's difficult. At least for "guest did a kexec", I *think* x86 
just doesn't trigger any special reset in QEMU. I know that s390x 
triggers a special subsystem reset (s390x has various types of resets ...).

> 
>> In that sense, at least virtio-mem wants to treat WARM and WAKEUP resets
>> alike. But I agree that simply because virtio-mem want sot treat them
>> alike doesn't mean that we should represent in QEMU using a single reset
>> type.
> 
> On the other hand, if virtio-mem does want to treat them
> the same we could start with only implementing WARM and
> not add WAKEUP until we have a use-case for it.

Makes sense.

> 
> By the way, I assume this patchseries is 9.2 material,
> not trying to fix a bug for this release ?

Yes, it can wait. Thanks for the guidance!
Peter Maydell Aug. 8, 2024, 5:30 p.m. UTC | #9
On Thu, 8 Aug 2024 at 17:29, David Hildenbrand <david@redhat.com> wrote:
>
> On 08.08.24 18:17, Peter Maydell wrote:
> > On Thu, 8 Aug 2024 at 17:04, David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 08.08.24 17:56, Peter Maydell wrote:
> >>> Right, I guess that's my question -- is "WAKEUP" ever any
> >>> different from "WARM" reset? I think the latter is a more
> >>> common general concept.
> >>>
> >>> On the other hand it looks like we already have the
> >>> concept in the runstate state machine of
> >>> RUN_STATE_SUSPENDED versus RUN_STATE_RUNNING, and so if we
> >>> define "WAKEUP" as "goes from SUSPENDED to RUNNING" that's
> >>> quite a clearly defined condition.
> >>
> >> Right.
> >>
> >>> Whereas WARM reset would
> >>> be a much wider range of things and ought to include for
> >>> instance "guest triggers a reset by writing to port 92"
> >>> and all the other SHUTDOWN_CAUSE_GUEST_RESET cases.
> >>> (Side note: do we document all these runstates and transitions
> >>> anywhere?)
> >>>
> >>> For virtio-mem, on a guest-triggered reset, should it
> >>> (a) definitely not unplug all the hotplugged memory
> >>> (b) definitely unplug all the hotplugged memory
> >>> (c) we don't care?
> >>
> >> During ordinary system resets (COLD) where RAM content is not guaranteed
> >> to survive:
> >
> > "COLD" isn't an "ordinary system reset", though -- COLD
> > reset is more like "I powered the computer off and then
> > turned it on again". A "WARM" reset is like "I pressed
> > the front panel reset button, or the watchdog device
> > fired and reset the system". We don't currently really
> > model front-panel or watchdog reset properly, we assume
> > that it's close enough to have it be the same as
> > power-off-and-on reset (and there are some kludges in
> > various places where it's not quite right).
>
> Agreed, there are many flavors of resets, even different flavors of warm
> ones I'm afraid.
>
> To summarize, if a VM does an ordinary reset ("shutdown -r") we want to
> unplug all hotplugged memory. Same with most external resets we
> currently implement. In all other caes, we likely want to keep the
> memory hotplugged and RAM content alive.

OK, if we want 'shutdown -r' to unplug hotplugged memory
then that sounds like we do want a WAKEUP reset type.
(Though I'm surprised that we want that -- my expectation
would have been that the hotplugged memory should stay,
because if you do real hardware hotplug of RAM then it
doesn't all pop back out of the sockets when you press
the reset button on the front panel :-))

thanks
-- PMM
David Hildenbrand Aug. 8, 2024, 6:07 p.m. UTC | #10
On 08.08.24 19:30, Peter Maydell wrote:
> On Thu, 8 Aug 2024 at 17:29, David Hildenbrand <david@redhat.com> wrote:
>>
>> On 08.08.24 18:17, Peter Maydell wrote:
>>> On Thu, 8 Aug 2024 at 17:04, David Hildenbrand <david@redhat.com> wrote:
>>>>
>>>> On 08.08.24 17:56, Peter Maydell wrote:
>>>>> Right, I guess that's my question -- is "WAKEUP" ever any
>>>>> different from "WARM" reset? I think the latter is a more
>>>>> common general concept.
>>>>>
>>>>> On the other hand it looks like we already have the
>>>>> concept in the runstate state machine of
>>>>> RUN_STATE_SUSPENDED versus RUN_STATE_RUNNING, and so if we
>>>>> define "WAKEUP" as "goes from SUSPENDED to RUNNING" that's
>>>>> quite a clearly defined condition.
>>>>
>>>> Right.
>>>>
>>>>> Whereas WARM reset would
>>>>> be a much wider range of things and ought to include for
>>>>> instance "guest triggers a reset by writing to port 92"
>>>>> and all the other SHUTDOWN_CAUSE_GUEST_RESET cases.
>>>>> (Side note: do we document all these runstates and transitions
>>>>> anywhere?)
>>>>>
>>>>> For virtio-mem, on a guest-triggered reset, should it
>>>>> (a) definitely not unplug all the hotplugged memory
>>>>> (b) definitely unplug all the hotplugged memory
>>>>> (c) we don't care?
>>>>
>>>> During ordinary system resets (COLD) where RAM content is not guaranteed
>>>> to survive:
>>>
>>> "COLD" isn't an "ordinary system reset", though -- COLD
>>> reset is more like "I powered the computer off and then
>>> turned it on again". A "WARM" reset is like "I pressed
>>> the front panel reset button, or the watchdog device
>>> fired and reset the system". We don't currently really
>>> model front-panel or watchdog reset properly, we assume
>>> that it's close enough to have it be the same as
>>> power-off-and-on reset (and there are some kludges in
>>> various places where it's not quite right).
>>
>> Agreed, there are many flavors of resets, even different flavors of warm
>> ones I'm afraid.
>>
>> To summarize, if a VM does an ordinary reset ("shutdown -r") we want to
>> unplug all hotplugged memory. Same with most external resets we
>> currently implement. In all other caes, we likely want to keep the
>> memory hotplugged and RAM content alive.
> 
> OK, if we want 'shutdown -r' to unplug hotplugged memory
> then that sounds like we do want a WAKEUP reset type.
> (Though I'm surprised that we want that -- my expectation
> would have been that the hotplugged memory should stay,
> because if you do real hardware hotplug of RAM then it
> doesn't all pop back out of the sockets when you press
> the reset button on the front panel :-))

:)

The main difference to ordinary RAM is that (virtio-mem) device memory 
is always detected by a guest OS driver, it's never part of the 
bios/firmware-provided memory map. And not all OSes support virtio-mem.

So one reason we added that handling in the beginning was if you would 
reboot from OS X to Y, whereby X suppors virtio-mem (and we hotplugged 
memory) and Y does not, then you would not leave that 
unusable-but-memory-consuming memory around for Y (possibly leaving 
customers paying for dynamic memory capacities angry).

Another reason was to force the fresh OS to effectively defragment 
hotplugged device memory (so we have less mixture of plugged and 
unplugged memory blocks that any previous OS might have left behind 
after unplugging memory).

Yet another reason was that one could have pending unplug requests 
(e.g., unplug 1 GiB, but only part of that request was fulfilled by the 
old OS), and this way we can easily enforce that once we get the chance to.

So to summarize, the idea was to "start from scratch" in all cases where 
it is safe. For wakeup, it apparently wasn't safe :)
diff mbox series

Patch

diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst
index 9746a4e8a0..30c9a0cc2b 100644
--- a/docs/devel/reset.rst
+++ b/docs/devel/reset.rst
@@ -44,6 +44,13 @@  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 used when the machine is woken up from a suspended state (deep
+  sleep, suspend-to-ram). Devices that must not be reset to their initial state
+  after wake-up (for example virtio-mem) can use this state to differentiate
+  cold start from wake-up can use this state to differentiate cold start from
+  wake-up.
+
 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;
 
 /*