[PATCH 0/4] virtio-mem: Implement support for suspend+wake-up with plugged memory

[PATCH 0/4] virtio-mem: Implement support for suspend+wake-up with plugged memory

[Juraj Marcin]

Currently, the virtio-mem device would unplug all the memory with any
reset request, including when the machine wakes up from a suspended
state (deep sleep). This would lead to a loss of the contents of the
guest memory and therefore is disabled by the virtio-mem Linux Kernel
driver unless the VIRTIO_MEM_F_PERSISTENT_SUSPEND virtio feature is
exposed. [1]

To make deep sleep with virtio-mem possible, we need to differentiate
cold start reset from wake-up reset. The first patch updates
qemu_system_reset() and MachineClass children to accept ResetType
instead of ShutdownCause, which then could be passed down the device
tree. The second patch then introduces the new reset type for the
wake-up event and updates the i386 wake-up method (only architecture
using the explicit wake-up method).

The third patch replaces LegacyReset with the Resettable interface in
virtio-mem, so the memory device can access the reset type in the hold
phase. The last patch of the series implements the final support in the
hold phase of the virtio-mem reset callback and exposes
VIRTIO_MEM_F_PERSISTENT_SUSPEND to the kernel.

[1]: https://lore.kernel.org/all/20240318120645.105664-1-david@redhat.com/

Juraj Marcin (4):
  reset: Use ResetType for qemu_devices_reset() and
    MachineClass->reset()
  reset: Add RESET_TYPE_WAKEUP
  virtio-mem: Implement Resettable interface instead of using
    LegacyReset
  virtio-mem: Add support for suspend+wake-up with plugged memory

 docs/devel/reset.rst           |  7 +++++
 hw/arm/aspeed.c                |  4 +--
 hw/arm/mps2-tz.c               |  4 +--
 hw/core/reset.c                |  7 ++---
 hw/hppa/machine.c              |  4 +--
 hw/i386/microvm.c              |  4 +--
 hw/i386/pc.c                   |  6 ++---
 hw/ppc/pegasos2.c              |  4 +--
 hw/ppc/pnv.c                   |  4 +--
 hw/ppc/spapr.c                 |  6 ++---
 hw/s390x/s390-virtio-ccw.c     |  4 +--
 hw/virtio/virtio-mem.c         | 49 ++++++++++++++++++++++++----------
 hw/virtio/virtio-qmp.c         |  3 +++
 include/hw/boards.h            |  3 ++-
 include/hw/resettable.h        |  2 ++
 include/hw/virtio/virtio-mem.h |  4 +++
 include/sysemu/reset.h         |  5 ++--
 system/runstate.c              | 13 +++++++--
 18 files changed, 89 insertions(+), 44 deletions(-)

-- 
2.45.2

[PATCH 1/4] reset: Use ResetType for qemu_devices_reset() and MachineClass->reset()

[Juraj Marcin]

Currently, both qemu_devices_reset() and MachineClass->reset() use
ShutdownCause for the reason of the reset. However, the Resettable
interface uses ResetState, so ShutdownCause needs to be translated to
ResetType somewhere. Translating it qemu_devices_reset() makes adding
new reset types harder, as they cannot always be matched to a single
ShutdownCause here, and devices may need to check the ResetType to
determine what to reset and if to reset at all.

This patch moves this translation up in the call stack to
qemu_system_reset() and updates all MachineClass children to use the
ResetType instead.

Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
---
 hw/arm/aspeed.c            |  4 ++--
 hw/arm/mps2-tz.c           |  4 ++--
 hw/core/reset.c            |  7 ++-----
 hw/hppa/machine.c          |  4 ++--
 hw/i386/microvm.c          |  4 ++--
 hw/i386/pc.c               |  6 +++---
 hw/ppc/pegasos2.c          |  4 ++--
 hw/ppc/pnv.c               |  4 ++--
 hw/ppc/spapr.c             |  6 +++---
 hw/s390x/s390-virtio-ccw.c |  4 ++--
 include/hw/boards.h        |  3 ++-
 include/sysemu/reset.h     |  5 +++--
 system/runstate.c          | 13 +++++++++++--
 13 files changed, 38 insertions(+), 30 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index fd5603f7aa..cbca7685da 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -1529,12 +1529,12 @@ static void aspeed_machine_bletchley_class_init(ObjectClass *oc, void *data)
     aspeed_machine_class_init_cpus_defaults(mc);
 }
 
-static void fby35_reset(MachineState *state, ShutdownCause reason)
+static void fby35_reset(MachineState *state, ResetType type)
 {
     AspeedMachineState *bmc = ASPEED_MACHINE(state);
     AspeedGPIOState *gpio = &bmc->soc->gpio;
 
-    qemu_devices_reset(reason);
+    qemu_devices_reset(type);
 
     /* Board ID: 7 (Class-1, 4 slots) */
     object_property_set_bool(OBJECT(gpio), "gpioV4", true, &error_fatal);
diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index aec57c0d68..8edf57a66d 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -1254,7 +1254,7 @@ static void mps2_set_remap(Object *obj, const char *value, Error **errp)
     }
 }
 
-static void mps2_machine_reset(MachineState *machine, ShutdownCause reason)
+static void mps2_machine_reset(MachineState *machine, ResetType type)
 {
     MPS2TZMachineState *mms = MPS2TZ_MACHINE(machine);
 
@@ -1264,7 +1264,7 @@ static void mps2_machine_reset(MachineState *machine, ShutdownCause reason)
      * reset see the correct mapping.
      */
     remap_memory(mms, mms->remap);
-    qemu_devices_reset(reason);
+    qemu_devices_reset(type);
 }
 
 static void mps2tz_class_init(ObjectClass *oc, void *data)
diff --git a/hw/core/reset.c b/hw/core/reset.c
index 58dfc8db3d..60c9c66d81 100644
--- a/hw/core/reset.c
+++ b/hw/core/reset.c
@@ -25,8 +25,8 @@
 
 #include "qemu/osdep.h"
 #include "sysemu/reset.h"
-#include "hw/resettable.h"
 #include "hw/core/resetcontainer.h"
+#include "hw/resettable.h"
 
 /*
  * Return a pointer to the singleton container that holds all the Resettable
@@ -170,11 +170,8 @@ void qemu_unregister_resettable(Object *obj)
     resettable_container_remove(get_root_reset_container(), obj);
 }
 
-void qemu_devices_reset(ShutdownCause reason)
+void qemu_devices_reset(ResetType type)
 {
-    ResetType type = (reason == SHUTDOWN_CAUSE_SNAPSHOT_LOAD) ?
-        RESET_TYPE_SNAPSHOT_LOAD : RESET_TYPE_COLD;
-
     /* Reset the simulation */
     resettable_reset(OBJECT(get_root_reset_container()), type);
 }
diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index 5d0a8739de..8259fe2e38 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -642,12 +642,12 @@ static void machine_HP_C3700_init(MachineState *machine)
     machine_HP_common_init_tail(machine, pci_bus, translate);
 }
 
-static void hppa_machine_reset(MachineState *ms, ShutdownCause reason)
+static void hppa_machine_reset(MachineState *ms, ResetType type)
 {
     unsigned int smp_cpus = ms->smp.cpus;
     int i;
 
-    qemu_devices_reset(reason);
+    qemu_devices_reset(type);
 
     /* Start all CPUs at the firmware entry point.
      *  Monarch CPU will initialize firmware, secondary CPUs
diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index 40edcee7af..8ae4dff7f2 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -462,7 +462,7 @@ static void microvm_machine_state_init(MachineState *machine)
     microvm_devices_init(mms);
 }
 
-static void microvm_machine_reset(MachineState *machine, ShutdownCause reason)
+static void microvm_machine_reset(MachineState *machine, ResetType type)
 {
     MicrovmMachineState *mms = MICROVM_MACHINE(machine);
     CPUState *cs;
@@ -475,7 +475,7 @@ static void microvm_machine_reset(MachineState *machine, ShutdownCause reason)
         mms->kernel_cmdline_fixed = true;
     }
 
-    qemu_devices_reset(reason);
+    qemu_devices_reset(type);
 
     CPU_FOREACH(cs) {
         cpu = X86_CPU(cs);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c74931d577..ccb9731c91 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1696,12 +1696,12 @@ static void pc_machine_initfn(Object *obj)
     qemu_add_machine_init_done_notifier(&pcms->machine_done);
 }
 
-static void pc_machine_reset(MachineState *machine, ShutdownCause reason)
+static void pc_machine_reset(MachineState *machine, ResetType type)
 {
     CPUState *cs;
     X86CPU *cpu;
 
-    qemu_devices_reset(reason);
+    qemu_devices_reset(type);
 
     /* Reset APIC after devices have been reset to cancel
      * any changes that qemu_devices_reset() might have done.
@@ -1716,7 +1716,7 @@ static void pc_machine_reset(MachineState *machine, ShutdownCause reason)
 static void pc_machine_wakeup(MachineState *machine)
 {
     cpu_synchronize_all_states();
-    pc_machine_reset(machine, SHUTDOWN_CAUSE_NONE);
+    pc_machine_reset(machine, RESET_TYPE_COLD);
     cpu_synchronize_all_post_reset();
 }
 
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 9b0a6b70ab..8ff4a00c34 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -291,14 +291,14 @@ static void pegasos2_superio_write(uint8_t addr, uint8_t val)
     cpu_physical_memory_write(PCI1_IO_BASE + 0x3f1, &val, 1);
 }
 
-static void pegasos2_machine_reset(MachineState *machine, ShutdownCause reason)
+static void pegasos2_machine_reset(MachineState *machine, ResetType type)
 {
     Pegasos2MachineState *pm = PEGASOS2_MACHINE(machine);
     void *fdt;
     uint64_t d[2];
     int sz;
 
-    qemu_devices_reset(reason);
+    qemu_devices_reset(type);
     if (!pm->vof) {
         return; /* Firmware should set up machine so nothing to do */
     }
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 3526852685..988fd55d88 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -709,13 +709,13 @@ static void pnv_powerdown_notify(Notifier *n, void *opaque)
     }
 }
 
-static void pnv_reset(MachineState *machine, ShutdownCause reason)
+static void pnv_reset(MachineState *machine, ResetType type)
 {
     PnvMachineState *pnv = PNV_MACHINE(machine);
     IPMIBmc *bmc;
     void *fdt;
 
-    qemu_devices_reset(reason);
+    qemu_devices_reset(type);
 
     /*
      * The machine should provide by default an internal BMC simulator.
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 370d7c35d3..95dbb91ba0 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1725,7 +1725,7 @@ void spapr_check_mmu_mode(bool guest_radix)
     }
 }
 
-static void spapr_machine_reset(MachineState *machine, ShutdownCause reason)
+static void spapr_machine_reset(MachineState *machine, ResetType type)
 {
     SpaprMachineState *spapr = SPAPR_MACHINE(machine);
     PowerPCCPU *first_ppc_cpu;
@@ -1733,7 +1733,7 @@ static void spapr_machine_reset(MachineState *machine, ShutdownCause reason)
     void *fdt;
     int rc;
 
-    if (reason != SHUTDOWN_CAUSE_SNAPSHOT_LOAD) {
+    if (type != RESET_TYPE_SNAPSHOT_LOAD) {
         /*
          * Record-replay snapshot load must not consume random, this was
          * already replayed from initial machine reset.
@@ -1762,7 +1762,7 @@ static void spapr_machine_reset(MachineState *machine, ShutdownCause reason)
         spapr_setup_hpt(spapr);
     }
 
-    qemu_devices_reset(reason);
+    qemu_devices_reset(type);
 
     spapr_ovec_cleanup(spapr->ov5_cas);
     spapr->ov5_cas = spapr_ovec_new();
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index c483ff8064..3471abb58b 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -440,7 +440,7 @@ static void s390_pv_prepare_reset(S390CcwMachineState *ms)
     s390_pv_prep_reset();
 }
 
-static void s390_machine_reset(MachineState *machine, ShutdownCause reason)
+static void s390_machine_reset(MachineState *machine, ResetType type)
 {
     S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
     enum s390_reset reset_type;
@@ -472,7 +472,7 @@ static void s390_machine_reset(MachineState *machine, ShutdownCause reason)
          * Device reset includes CPU clear resets so this has to be
          * done AFTER the unprotect call above.
          */
-        qemu_devices_reset(reason);
+        qemu_devices_reset(type);
         s390_crypto_reset();
 
         /* configure and start the ipl CPU only */
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 48ff6d8b93..3e8a6986cd 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -10,6 +10,7 @@
 #include "qemu/module.h"
 #include "qom/object.h"
 #include "hw/core/cpu.h"
+#include "hw/resettable.h"
 
 #define TYPE_MACHINE_SUFFIX "-machine"
 
@@ -253,7 +254,7 @@ struct MachineClass {
     const char *deprecation_reason;
 
     void (*init)(MachineState *state);
-    void (*reset)(MachineState *state, ShutdownCause reason);
+    void (*reset)(MachineState *state, ResetType type);
     void (*wakeup)(MachineState *state);
     int (*kvm_type)(MachineState *machine, const char *arg);
 
diff --git a/include/sysemu/reset.h b/include/sysemu/reset.h
index ae436044a9..0e297c0e02 100644
--- a/include/sysemu/reset.h
+++ b/include/sysemu/reset.h
@@ -27,6 +27,7 @@
 #ifndef QEMU_SYSEMU_RESET_H
 #define QEMU_SYSEMU_RESET_H
 
+#include "hw/resettable.h"
 #include "qapi/qapi-events-run-state.h"
 
 typedef void QEMUResetHandler(void *opaque);
@@ -110,7 +111,7 @@ void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
 
 /**
  * qemu_devices_reset: Perform a complete system reset
- * @reason: reason for the reset
+ * @reason: type of the reset
  *
  * This function performs the low-level work needed to do a complete reset
  * of the system (calling all the callbacks registered with
@@ -121,6 +122,6 @@ void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
  * If you want to trigger a system reset from, for instance, a device
  * model, don't use this function. Use qemu_system_reset_request().
  */
-void qemu_devices_reset(ShutdownCause reason);
+void qemu_devices_reset(ResetType type);
 
 #endif
diff --git a/system/runstate.c b/system/runstate.c
index c833316f6d..eae959655e 100644
--- a/system/runstate.c
+++ b/system/runstate.c
@@ -32,6 +32,7 @@
 #include "exec/cpu-common.h"
 #include "gdbstub/syscalls.h"
 #include "hw/boards.h"
+#include "hw/resettable.h"
 #include "migration/misc.h"
 #include "migration/postcopy-ram.h"
 #include "monitor/monitor.h"
@@ -482,15 +483,23 @@ static int qemu_debug_requested(void)
 void qemu_system_reset(ShutdownCause reason)
 {
     MachineClass *mc;
+    ResetType type;
 
     mc = current_machine ? MACHINE_GET_CLASS(current_machine) : NULL;
 
     cpu_synchronize_all_states();
 
+    switch (reason) {
+    case SHUTDOWN_CAUSE_SNAPSHOT_LOAD:
+        type = RESET_TYPE_SNAPSHOT_LOAD;
+        break;
+    default:
+        type = RESET_TYPE_COLD;
+    }
     if (mc && mc->reset) {
-        mc->reset(current_machine, reason);
+        mc->reset(current_machine, type);
     } else {
-        qemu_devices_reset(reason);
+        qemu_devices_reset(type);
     }
     switch (reason) {
     case SHUTDOWN_CAUSE_NONE:
-- 
2.45.2

Re: [PATCH 1/4] reset: Use ResetType for qemu_devices_reset() and MachineClass->reset()

[David Hildenbrand]

On 06.08.24 18:07, Juraj Marcin wrote:
> Currently, both qemu_devices_reset() and MachineClass->reset() use
> ShutdownCause for the reason of the reset. However, the Resettable
> interface uses ResetState, so ShutdownCause needs to be translated to
> ResetType somewhere. Translating it qemu_devices_reset() makes adding
> new reset types harder, as they cannot always be matched to a single
> ShutdownCause here, and devices may need to check the ResetType to
> determine what to reset and if to reset at all.
> 
> This patch moves this translation up in the call stack to
> qemu_system_reset() and updates all MachineClass children to use the
> ResetType instead.
> 
> Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
> ---

[...]

>   
>   static void mps2tz_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/core/reset.c b/hw/core/reset.c
> index 58dfc8db3d..60c9c66d81 100644
> --- a/hw/core/reset.c
> +++ b/hw/core/reset.c
> @@ -25,8 +25,8 @@
>   
>   #include "qemu/osdep.h"
>   #include "sysemu/reset.h"
> -#include "hw/resettable.h"
>   #include "hw/core/resetcontainer.h"
> +#include "hw/resettable.h"

Curious, is that change really required?

Apart from that LGTM!

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/4] reset: Use ResetType for qemu_devices_reset() and MachineClass->reset()

[Juraj Marcin]

On Tue, Aug 6, 2024 at 6:40 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 06.08.24 18:07, Juraj Marcin wrote:
> > Currently, both qemu_devices_reset() and MachineClass->reset() use
> > ShutdownCause for the reason of the reset. However, the Resettable
> > interface uses ResetState, so ShutdownCause needs to be translated to
> > ResetType somewhere. Translating it qemu_devices_reset() makes adding
> > new reset types harder, as they cannot always be matched to a single
> > ShutdownCause here, and devices may need to check the ResetType to
> > determine what to reset and if to reset at all.
> >
> > This patch moves this translation up in the call stack to
> > qemu_system_reset() and updates all MachineClass children to use the
> > ResetType instead.
> >
> > Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
> > ---
>
> [...]
>
> >
> >   static void mps2tz_class_init(ObjectClass *oc, void *data)
> > diff --git a/hw/core/reset.c b/hw/core/reset.c
> > index 58dfc8db3d..60c9c66d81 100644
> > --- a/hw/core/reset.c
> > +++ b/hw/core/reset.c
> > @@ -25,8 +25,8 @@
> >
> >   #include "qemu/osdep.h"
> >   #include "sysemu/reset.h"
> > -#include "hw/resettable.h"
> >   #include "hw/core/resetcontainer.h"
> > +#include "hw/resettable.h"
>
> Curious, is that change really required?

You are right, it is not required. I will fix it when some more
feedback comes in. Thank you!

>
> Apart from that LGTM!
>
> Reviewed-by: David Hildenbrand <david@redhat.com>
>
> --
> Cheers,
>
> David / dhildenb
>

-- 
Juraj Marcin

Re: [PATCH 1/4] reset: Use ResetType for qemu_devices_reset() and MachineClass->reset()

[Peter Maydell]

On Tue, 6 Aug 2024 at 17:08, Juraj Marcin <jmarcin@redhat.com> wrote:
>
> Currently, both qemu_devices_reset() and MachineClass->reset() use
> ShutdownCause for the reason of the reset. However, the Resettable
> interface uses ResetState, so ShutdownCause needs to be translated to
> ResetType somewhere. Translating it qemu_devices_reset() makes adding
> new reset types harder, as they cannot always be matched to a single
> ShutdownCause here, and devices may need to check the ResetType to
> determine what to reset and if to reset at all.
>
> This patch moves this translation up in the call stack to
> qemu_system_reset() and updates all MachineClass children to use the
> ResetType instead.

I have a work-in-progress refactoring which makes
TYPE_MACHINE be a subclass of TYPE_DEVICE, with the aim of
converting all the MachineClass::reset functions into
proper Resettable reset functions. If we did that then all
these reset functions would take a ResetType anyway.
But I might not get round to having another go at that
refactoring for a while, and this is a good cleanup in the
meantime.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 2/4] reset: Add RESET_TYPE_WAKEUP

[Juraj Marcin]

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.
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;
 
 /*
-- 
2.45.2

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

[David Hildenbrand]

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>

-- 
Cheers,

David / dhildenb

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

[Peter Maydell]

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

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

[Juraj Marcin]

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

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

[David Hildenbrand]

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?

-- 
Cheers,

David / dhildenb

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

[Peter Maydell]

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

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

[David Hildenbrand]

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.

-- 
Cheers,

David / dhildenb

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

[Peter Maydell]

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

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

[David Hildenbrand]

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!

-- 
Cheers,

David / dhildenb

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

[Peter Maydell]

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

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

[David Hildenbrand]

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 :)

-- 
Cheers,

David / dhildenb

[PATCH 3/4] virtio-mem: Implement Resettable interface instead of using LegacyReset

[Juraj Marcin]

LegacyReset does not pass ResetType to the reset callback method, which
the new Resettable interface uses. Due to this, virtio-mem cannot use
the new RESET_TYPE_WAKEUP to skip reset during wake-up from a suspended
state.

This patch adds the Resettable interface to the VirtioMemClass interface
list, implements the necessary methods and replaces
qemu_[un]register_reset() calls with qemu_[un]register_resettable().

Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
---
 hw/virtio/virtio-mem.c         | 39 ++++++++++++++++++++++------------
 include/hw/virtio/virtio-mem.h |  4 ++++
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index ef64bf1b4a..4f2fd7dc2e 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -895,18 +895,6 @@ static int virtio_mem_validate_features(VirtIODevice *vdev)
     return 0;
 }
 
-static void virtio_mem_system_reset(void *opaque)
-{
-    VirtIOMEM *vmem = VIRTIO_MEM(opaque);
-
-    /*
-     * During usual resets, we will unplug all memory and shrink the usable
-     * region size. This is, however, not possible in all scenarios. Then,
-     * the guest has to deal with this manually (VIRTIO_MEM_REQ_UNPLUG_ALL).
-     */
-    virtio_mem_unplug_all(vmem);
-}
-
 static void virtio_mem_prepare_mr(VirtIOMEM *vmem)
 {
     const uint64_t region_size = memory_region_size(&vmem->memdev->mr);
@@ -1123,7 +1111,7 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp)
         vmstate_register_any(VMSTATE_IF(vmem),
                              &vmstate_virtio_mem_device_early, vmem);
     }
-    qemu_register_reset(virtio_mem_system_reset, vmem);
+    qemu_register_resettable(OBJECT(vmem));
 
     /*
      * Set ourselves as RamDiscardManager before the plug handler maps the
@@ -1143,7 +1131,7 @@ static void virtio_mem_device_unrealize(DeviceState *dev)
      * found via an address space anymore. Unset ourselves.
      */
     memory_region_set_ram_discard_manager(&vmem->memdev->mr, NULL);
-    qemu_unregister_reset(virtio_mem_system_reset, vmem);
+    qemu_unregister_resettable(OBJECT(vmem));
     if (vmem->early_migration) {
         vmstate_unregister(VMSTATE_IF(vmem), &vmstate_virtio_mem_device_early,
                            vmem);
@@ -1843,12 +1831,31 @@ static void virtio_mem_unplug_request_check(VirtIOMEM *vmem, Error **errp)
     }
 }
 
+static ResettableState *virtio_mem_get_reset_state(Object *obj)
+{
+    VirtIOMEM *vmem = VIRTIO_MEM(obj);
+    return &vmem->reset_state;
+}
+
+static void virtio_mem_system_reset_hold(Object *obj, ResetType type)
+{
+    VirtIOMEM *vmem = VIRTIO_MEM(obj);
+
+    /*
+     * During usual resets, we will unplug all memory and shrink the usable
+     * region size. This is, however, not possible in all scenarios. Then,
+     * the guest has to deal with this manually (VIRTIO_MEM_REQ_UNPLUG_ALL).
+     */
+    virtio_mem_unplug_all(vmem);
+}
+
 static void virtio_mem_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
     VirtIOMEMClass *vmc = VIRTIO_MEM_CLASS(klass);
     RamDiscardManagerClass *rdmc = RAM_DISCARD_MANAGER_CLASS(klass);
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
 
     device_class_set_props(dc, virtio_mem_properties);
     dc->vmsd = &vmstate_virtio_mem;
@@ -1875,6 +1882,9 @@ static void virtio_mem_class_init(ObjectClass *klass, void *data)
     rdmc->replay_discarded = virtio_mem_rdm_replay_discarded;
     rdmc->register_listener = virtio_mem_rdm_register_listener;
     rdmc->unregister_listener = virtio_mem_rdm_unregister_listener;
+
+    rc->get_state = virtio_mem_get_reset_state;
+    rc->phases.hold = virtio_mem_system_reset_hold;
 }
 
 static const TypeInfo virtio_mem_info = {
@@ -1887,6 +1897,7 @@ static const TypeInfo virtio_mem_info = {
     .class_size = sizeof(VirtIOMEMClass),
     .interfaces = (InterfaceInfo[]) {
         { TYPE_RAM_DISCARD_MANAGER },
+        { TYPE_RESETTABLE_INTERFACE },
         { }
     },
 };
diff --git a/include/hw/virtio/virtio-mem.h b/include/hw/virtio/virtio-mem.h
index 5f5b02b8f9..79f197216b 100644
--- a/include/hw/virtio/virtio-mem.h
+++ b/include/hw/virtio/virtio-mem.h
@@ -13,6 +13,7 @@
 #ifndef HW_VIRTIO_MEM_H
 #define HW_VIRTIO_MEM_H
 
+#include "hw/resettable.h"
 #include "standard-headers/linux/virtio_mem.h"
 #include "hw/virtio/virtio.h"
 #include "qapi/qapi-types-misc.h"
@@ -115,6 +116,9 @@ struct VirtIOMEM {
 
     /* listeners to notify on plug/unplug activity. */
     QLIST_HEAD(, RamDiscardListener) rdl_list;
+
+    /* State of the resettable container */
+    ResettableState reset_state;
 };
 
 struct VirtIOMEMClass {
-- 
2.45.2

Re: [PATCH 3/4] virtio-mem: Implement Resettable interface instead of using LegacyReset

[David Hildenbrand]

On 06.08.24 18:07, Juraj Marcin wrote:
> LegacyReset does not pass ResetType to the reset callback method, which
> the new Resettable interface uses. Due to this, virtio-mem cannot use
> the new RESET_TYPE_WAKEUP to skip reset during wake-up from a suspended
> state.
> 
> This patch adds the Resettable interface to the VirtioMemClass interface
> list, implements the necessary methods and replaces
> qemu_[un]register_reset() calls with qemu_[un]register_resettable().
> 
> Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 3/4] virtio-mem: Implement Resettable interface instead of using LegacyReset

[Peter Maydell]

On Tue, 6 Aug 2024 at 17:08, Juraj Marcin <jmarcin@redhat.com> wrote:
>
> LegacyReset does not pass ResetType to the reset callback method, which
> the new Resettable interface uses. Due to this, virtio-mem cannot use
> the new RESET_TYPE_WAKEUP to skip reset during wake-up from a suspended
> state.
>
> This patch adds the Resettable interface to the VirtioMemClass interface
> list, implements the necessary methods and replaces
> qemu_[un]register_reset() calls with qemu_[un]register_resettable().

> @@ -1887,6 +1897,7 @@ static const TypeInfo virtio_mem_info = {
>      .class_size = sizeof(VirtIOMEMClass),
>      .interfaces = (InterfaceInfo[]) {
>          { TYPE_RAM_DISCARD_MANAGER },
> +        { TYPE_RESETTABLE_INTERFACE },
>          { }
>      },
>  };

TYPE_VIRTIO_MEM is-a TYPE_VIRTIO_DEVICE, which is-a TYPE_DEVICE,
which implements the TYPE_RESETTABLE_INTERFACE. In other words,
as a device this is already Resettable. Re-implementing the
interface doesn't seem like the right thing here (it probably
breaks the general reset implementation in the base class).
Maybe what you want to do here is implement the Resettable
methods that you already have?

thanks
-- PMM

Re: [PATCH 3/4] virtio-mem: Implement Resettable interface instead of using LegacyReset

[David Hildenbrand]

On 08.08.24 14:25, Peter Maydell wrote:
> On Tue, 6 Aug 2024 at 17:08, Juraj Marcin <jmarcin@redhat.com> wrote:
>>
>> LegacyReset does not pass ResetType to the reset callback method, which
>> the new Resettable interface uses. Due to this, virtio-mem cannot use
>> the new RESET_TYPE_WAKEUP to skip reset during wake-up from a suspended
>> state.
>>
>> This patch adds the Resettable interface to the VirtioMemClass interface
>> list, implements the necessary methods and replaces
>> qemu_[un]register_reset() calls with qemu_[un]register_resettable().
> 
>> @@ -1887,6 +1897,7 @@ static const TypeInfo virtio_mem_info = {
>>       .class_size = sizeof(VirtIOMEMClass),
>>       .interfaces = (InterfaceInfo[]) {
>>           { TYPE_RAM_DISCARD_MANAGER },
>> +        { TYPE_RESETTABLE_INTERFACE },
>>           { }
>>       },
>>   };
> 
> TYPE_VIRTIO_MEM is-a TYPE_VIRTIO_DEVICE, which is-a TYPE_DEVICE,
> which implements the TYPE_RESETTABLE_INTERFACE. In other words,
> as a device this is already Resettable. Re-implementing the
> interface doesn't seem like the right thing here (it probably
> breaks the general reset implementation in the base class).
> Maybe what you want to do here is implement the Resettable
> methods that you already have?

TYPE_DEVICE indeed is TYPE_RESETTABLE_INTERFACE.

And there, we implement a single "dc->reset", within which we 
unconditionally use "RESET_TYPE_COLD".

Looks like more plumbing might be required to get the actual reset type
to the device that way, unless I am missing the easy way out.

-- 
Cheers,

David / dhildenb

Re: [PATCH 3/4] virtio-mem: Implement Resettable interface instead of using LegacyReset

[Peter Maydell]

On Thu, 8 Aug 2024 at 13:37, David Hildenbrand <david@redhat.com> wrote:
>
> On 08.08.24 14:25, Peter Maydell wrote:
> > On Tue, 6 Aug 2024 at 17:08, Juraj Marcin <jmarcin@redhat.com> wrote:
> >>
> >> LegacyReset does not pass ResetType to the reset callback method, which
> >> the new Resettable interface uses. Due to this, virtio-mem cannot use
> >> the new RESET_TYPE_WAKEUP to skip reset during wake-up from a suspended
> >> state.
> >>
> >> This patch adds the Resettable interface to the VirtioMemClass interface
> >> list, implements the necessary methods and replaces
> >> qemu_[un]register_reset() calls with qemu_[un]register_resettable().
> >
> >> @@ -1887,6 +1897,7 @@ static const TypeInfo virtio_mem_info = {
> >>       .class_size = sizeof(VirtIOMEMClass),
> >>       .interfaces = (InterfaceInfo[]) {
> >>           { TYPE_RAM_DISCARD_MANAGER },
> >> +        { TYPE_RESETTABLE_INTERFACE },
> >>           { }
> >>       },
> >>   };
> >
> > TYPE_VIRTIO_MEM is-a TYPE_VIRTIO_DEVICE, which is-a TYPE_DEVICE,
> > which implements the TYPE_RESETTABLE_INTERFACE. In other words,
> > as a device this is already Resettable. Re-implementing the
> > interface doesn't seem like the right thing here (it probably
> > breaks the general reset implementation in the base class).
> > Maybe what you want to do here is implement the Resettable
> > methods that you already have?
>
> TYPE_DEVICE indeed is TYPE_RESETTABLE_INTERFACE.
>
> And there, we implement a single "dc->reset", within which we
> unconditionally use "RESET_TYPE_COLD".

That's the glue that implements compatibility with the legacy
DeviceClass::reset method.

There's two kinds of glue here:

(1) When a device is reset via a method that is three-phase-reset
aware (including full system reset), if the device (i.e. some
subclass of TYPE_DEVICE) implements the Resettable methods, then
they get used. If the device doesn't implement those methods,
then the base class logic will arrange to call the legacy
DeviceClass::reset method of the subclass. This is what
device_transitional_reset() is doing.

(2) When a three-phase-reset device is reset via a method that is not
three-phase aware, the glue in the other direction is the
default DeviceState::reset method which is device_phases_reset(),
which does a RESET_TYPE_COLD reset for each phase in turn.
Here we have to pick a RESET_TYPE because the old legacy
reset API had no concept of reset types at all.
The set of cases where this can happen is now very restricted
because I've been gradually trying to convert places that can
trigger a reset to be three-phase aware. I think the only
remaining case is "parent class is 3-phase but it has a subclass
that is not 3-phase aware and tries to chain to the parent
class reset using device_class_set_parent_reset()", and the
only remaining cases of that are s390 CPU and s390 virtio-ccw.

For TYPE_VIRTIO_MEM neither of these should matter.

Other places where RESET_TYPE_COLD gets used:
 * device_cold_reset() is a function to say "cold reset this
   device", and so it always uses RESET_TYPE_COLD. The assumption
   is that the caller knows they wanted a cold reset; they can
   use resettable_reset(OBJECT(x), RESET_TYPE_FOO) if they want to
   trigger some other kind of reset on a specific device
 * similarly bus_cold_reset() for bus resets
 * when a hot-plug device is first realized, it gets a
   RESET_TYPE_COLD (makes sense to me, this is like "power on")

I think these should all not be relevant to the WAKEUP
usecase here.

> Looks like more plumbing might be required to get the actual reset type
> to the device that way, unless I am missing the easy way out.

I think the plumbing should all be in place already.

thanks
-- PMM

Re: [PATCH 3/4] virtio-mem: Implement Resettable interface instead of using LegacyReset

[Juraj Marcin]

On Thu, Aug 8, 2024 at 5:47 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 8 Aug 2024 at 13:37, David Hildenbrand <david@redhat.com> wrote:
> >
> > On 08.08.24 14:25, Peter Maydell wrote:
> > > On Tue, 6 Aug 2024 at 17:08, Juraj Marcin <jmarcin@redhat.com> wrote:
> > >>
> > >> LegacyReset does not pass ResetType to the reset callback method, which
> > >> the new Resettable interface uses. Due to this, virtio-mem cannot use
> > >> the new RESET_TYPE_WAKEUP to skip reset during wake-up from a suspended
> > >> state.
> > >>
> > >> This patch adds the Resettable interface to the VirtioMemClass interface
> > >> list, implements the necessary methods and replaces
> > >> qemu_[un]register_reset() calls with qemu_[un]register_resettable().
> > >
> > >> @@ -1887,6 +1897,7 @@ static const TypeInfo virtio_mem_info = {
> > >>       .class_size = sizeof(VirtIOMEMClass),
> > >>       .interfaces = (InterfaceInfo[]) {
> > >>           { TYPE_RAM_DISCARD_MANAGER },
> > >> +        { TYPE_RESETTABLE_INTERFACE },
> > >>           { }
> > >>       },
> > >>   };
> > >
> > > TYPE_VIRTIO_MEM is-a TYPE_VIRTIO_DEVICE, which is-a TYPE_DEVICE,
> > > which implements the TYPE_RESETTABLE_INTERFACE. In other words,
> > > as a device this is already Resettable. Re-implementing the
> > > interface doesn't seem like the right thing here (it probably
> > > breaks the general reset implementation in the base class).
> > > Maybe what you want to do here is implement the Resettable
> > > methods that you already have?
> >
> > TYPE_DEVICE indeed is TYPE_RESETTABLE_INTERFACE.
> >
> > And there, we implement a single "dc->reset", within which we
> > unconditionally use "RESET_TYPE_COLD".
>
> That's the glue that implements compatibility with the legacy
> DeviceClass::reset method.
>
> There's two kinds of glue here:
>
> (1) When a device is reset via a method that is three-phase-reset
> aware (including full system reset), if the device (i.e. some
> subclass of TYPE_DEVICE) implements the Resettable methods, then
> they get used. If the device doesn't implement those methods,
> then the base class logic will arrange to call the legacy
> DeviceClass::reset method of the subclass. This is what
> device_transitional_reset() is doing.
>
> (2) When a three-phase-reset device is reset via a method that is not
> three-phase aware, the glue in the other direction is the
> default DeviceState::reset method which is device_phases_reset(),
> which does a RESET_TYPE_COLD reset for each phase in turn.
> Here we have to pick a RESET_TYPE because the old legacy
> reset API had no concept of reset types at all.
> The set of cases where this can happen is now very restricted
> because I've been gradually trying to convert places that can
> trigger a reset to be three-phase aware. I think the only
> remaining case is "parent class is 3-phase but it has a subclass
> that is not 3-phase aware and tries to chain to the parent
> class reset using device_class_set_parent_reset()", and the
> only remaining cases of that are s390 CPU and s390 virtio-ccw.
>
> For TYPE_VIRTIO_MEM neither of these should matter.
>
> Other places where RESET_TYPE_COLD gets used:
>  * device_cold_reset() is a function to say "cold reset this
>    device", and so it always uses RESET_TYPE_COLD. The assumption
>    is that the caller knows they wanted a cold reset; they can
>    use resettable_reset(OBJECT(x), RESET_TYPE_FOO) if they want to
>    trigger some other kind of reset on a specific device
>  * similarly bus_cold_reset() for bus resets
>  * when a hot-plug device is first realized, it gets a
>    RESET_TYPE_COLD (makes sense to me, this is like "power on")
>
> I think these should all not be relevant to the WAKEUP
> usecase here.
>
> > Looks like more plumbing might be required to get the actual reset type
> > to the device that way, unless I am missing the easy way out.
>
> I think the plumbing should all be in place already.

I have gone through the code once more and I also think that. I think
that removing the interface from VirtIOMEM type info (as it already is
in the parent) and then overriding the Resettable methods in
virtio_mem_class_init() should be enough. This should also include
setting rc->get_transitional_function to NULL, so the
device_get_transitional_reset() does not interfere with the 3 phase
reset.

>
> thanks
> -- PMM
>

-- 

Juraj Marcin

[PATCH 4/4] virtio-mem: Add support for suspend+wake-up with plugged memory

[Juraj Marcin]

Before, the virtio-mem device would unplug all the memory with any reset
of the device, including during the wake-up of the guest from a
suspended state. Due to this, the virtio-mem driver in the Linux kernel
disallowed suspend-to-ram requests in the guest when the
VIRTIO_MEM_F_PERSISTENT_SUSPEND feature is not exposed by QEMU.

This patch adds the code to skip the reset on wake-up and exposes
theVIRTIO_MEM_F_PERSISTENT_SUSPEND feature to the guest kernel driver
when suspending is possible in QEMU (currently only x86).

Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
---
 hw/virtio/virtio-mem.c | 10 ++++++++++
 hw/virtio/virtio-qmp.c |  3 +++
 2 files changed, 13 insertions(+)

diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index 4f2fd7dc2e..d373eb0028 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -883,6 +883,9 @@ static uint64_t virtio_mem_get_features(VirtIODevice *vdev, uint64_t features,
     if (vmem->unplugged_inaccessible == ON_OFF_AUTO_ON) {
         virtio_add_feature(&features, VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE);
     }
+    if (qemu_wakeup_suspend_enabled()) {
+        virtio_add_feature(&features, VIRTIO_MEM_F_PERSISTENT_SUSPEND);
+    }
     return features;
 }
 
@@ -1841,6 +1844,13 @@ static void virtio_mem_system_reset_hold(Object *obj, ResetType type)
 {
     VirtIOMEM *vmem = VIRTIO_MEM(obj);
 
+    /*
+     * When waking up from standby/suspend-to-ram, do not unplug any memory.
+     */
+    if (type == RESET_TYPE_WAKEUP) {
+        return;
+    }
+
     /*
      * During usual resets, we will unplug all memory and shrink the usable
      * region size. This is, however, not possible in all scenarios. Then,
diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
index 1dd96ed20f..cccc6fe761 100644
--- a/hw/virtio/virtio-qmp.c
+++ b/hw/virtio/virtio-qmp.c
@@ -450,6 +450,9 @@ static const qmp_virtio_feature_map_t virtio_mem_feature_map[] = {
     FEATURE_ENTRY(VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, \
             "VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE: Unplugged memory cannot be "
             "accessed"),
+    FEATURE_ENTRY(VIRTIO_MEM_F_PERSISTENT_SUSPEND, \
+            "VIRTIO_MEM_F_PERSISTENT_SUSPND: Plugged memory will remain "
+            "plugged when suspending+resuming"),
     { -1, "" }
 };
 #endif
-- 
2.45.2

Re: [PATCH 4/4] virtio-mem: Add support for suspend+wake-up with plugged memory

[David Hildenbrand]

On 06.08.24 18:07, Juraj Marcin wrote:
> Before, the virtio-mem device would unplug all the memory with any reset
> of the device, including during the wake-up of the guest from a
> suspended state. Due to this, the virtio-mem driver in the Linux kernel
> disallowed suspend-to-ram requests in the guest when the
> VIRTIO_MEM_F_PERSISTENT_SUSPEND feature is not exposed by QEMU.
> 
> This patch adds the code to skip the reset on wake-up and exposes
> theVIRTIO_MEM_F_PERSISTENT_SUSPEND feature to the guest kernel driver
> when suspending is possible in QEMU (currently only x86).
> 
> Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
> ---

All looks good and straight-forward to me. Will try finding time to play 
with this this week!

Thanks!

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb