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

[PATCH v2 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/

v2:
- Removed unnecessary include directives changes
- Updated RESET_TYPE_WAKEUP documentation
- Removed unnecessary interface from VirtIOMEMClass type info

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

 docs/devel/reset.rst           |  8 ++++++
 hw/arm/aspeed.c                |  4 +--
 hw/arm/mps2-tz.c               |  4 +--
 hw/core/reset.c                |  5 +---
 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         | 48 ++++++++++++++++++++++++----------
 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, 88 insertions(+), 43 deletions(-)

-- 
2.46.0

[PATCH v2 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>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/aspeed.c            |  4 ++--
 hw/arm/mps2-tz.c           |  4 ++--
 hw/core/reset.c            |  5 +----
 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, 37 insertions(+), 29 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..14a2639fbf 100644
--- a/hw/core/reset.c
+++ b/hw/core/reset.c
@@ -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.46.0

[PATCH v2 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>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 docs/devel/reset.rst    | 8 ++++++++
 hw/i386/pc.c            | 2 +-
 include/hw/resettable.h | 2 ++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst
index 9746a4e8a0..a7c9467313 100644
--- a/docs/devel/reset.rst
+++ b/docs/devel/reset.rst
@@ -44,6 +44,14 @@ The Resettable interface handles reset types with an enum ``ResetType``:
   value on each cold reset, such as RNG seed information, and which they
   must not reinitialize on a snapshot-load reset.
 
+``RESET_TYPE_WAKEUP``
+  This type is called for a reset when the system is being woken-up from a
+  suspended state using the ``qemu_system_wakeup()`` function. If the machine
+  needs to reset its devices in its ``MachineClass::wakeup()`` method, this
+  reset type should be used, so devices can differentiate system wake-up from
+  other reset types. For example, a virtio-mem device must not unplug its
+  memory during wake-up as that would clear the guest RAM.
+
 Devices which implement reset methods must treat any unknown ``ResetType``
 as equivalent to ``RESET_TYPE_COLD``; this will reduce the amount of
 existing code we need to change if we add more types in future.
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index ccb9731c91..49efd0a997 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1716,7 +1716,7 @@ static void pc_machine_reset(MachineState *machine, ResetType type)
 static void pc_machine_wakeup(MachineState *machine)
 {
     cpu_synchronize_all_states();
-    pc_machine_reset(machine, RESET_TYPE_COLD);
+    pc_machine_reset(machine, RESET_TYPE_WAKEUP);
     cpu_synchronize_all_post_reset();
 }
 
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.46.0

[PATCH v2 3/4] virtio-mem: Use new Resettable framework instead of LegacyReset

[Juraj Marcin]

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

This patch adds overrides Resettable interface methods in VirtIOMEMClass
to use the new Resettable framework 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>
---
 hw/virtio/virtio-mem.c         | 38 +++++++++++++++++++++-------------
 include/hw/virtio/virtio-mem.h |  4 ++++
 2 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index ef64bf1b4a..aeee381eb2 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 = {
diff --git a/include/hw/virtio/virtio-mem.h b/include/hw/virtio/virtio-mem.h
index 5f5b02b8f9..a1af144c28 100644
--- a/include/hw/virtio/virtio-mem.h
+++ b/include/hw/virtio/virtio-mem.h
@@ -14,6 +14,7 @@
 #define HW_VIRTIO_MEM_H
 
 #include "standard-headers/linux/virtio_mem.h"
+#include "hw/resettable.h"
 #include "hw/virtio/virtio.h"
 #include "qapi/qapi-types-misc.h"
 #include "sysemu/hostmem.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.46.0

[PATCH v2 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>
Reviewed-by: David Hildenbrand <david@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 aeee381eb2..ce228da369 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.46.0

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

[Peter Maydell]

On Tue, 13 Aug 2024 at 16:39, Juraj Marcin <jmarcin@redhat.com> wrote:
>
> Some devices need to distinguish cold start reset from waking up from a
> suspended state. This patch adds new value to the enum, and updates the
> i386 wakeup method to use this new reset type.
>
> Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
>  docs/devel/reset.rst    | 8 ++++++++
>  hw/i386/pc.c            | 2 +-
>  include/hw/resettable.h | 2 ++
>  3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst
> index 9746a4e8a0..a7c9467313 100644
> --- a/docs/devel/reset.rst
> +++ b/docs/devel/reset.rst
> @@ -44,6 +44,14 @@ The Resettable interface handles reset types with an enum ``ResetType``:
>    value on each cold reset, such as RNG seed information, and which they
>    must not reinitialize on a snapshot-load reset.
>
> +``RESET_TYPE_WAKEUP``
> +  This type is called for a reset when the system is being woken-up from a
> +  suspended state using the ``qemu_system_wakeup()`` function. If the machine
> +  needs to reset its devices in its ``MachineClass::wakeup()`` method, this
> +  reset type should be used, so devices can differentiate system wake-up from
> +  other reset types. For example, a virtio-mem device must not unplug its
> +  memory during wake-up as that would clear the guest RAM.
> +
>  Devices which implement reset methods must treat any unknown ``ResetType``
>  as equivalent to ``RESET_TYPE_COLD``; this will reduce the amount of
>  existing code we need to change if we add more types in future.
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index ccb9731c91..49efd0a997 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1716,7 +1716,7 @@ static void pc_machine_reset(MachineState *machine, ResetType type)
>  static void pc_machine_wakeup(MachineState *machine)
>  {
>      cpu_synchronize_all_states();
> -    pc_machine_reset(machine, RESET_TYPE_COLD);
> +    pc_machine_reset(machine, RESET_TYPE_WAKEUP);
>      cpu_synchronize_all_post_reset();
>  }

I'm happy (following discussion in the previous thread)
that 'wakeup' is the right reset event to be using here.
But looking at the existing code for qemu_system_wakeup()
something seems odd here. qemu_system_wakeup() calls
the MachineClass::wakeup method if it's set, and does
nothing if it's not. The PC implementation of that calls
pc_machine_reset(), which does a qemu_devices_reset(),
which does a complete three-phase reset of the system.
But if the machine doesn't implement wakeup then we
never reset the system at all.

Shouldn't qemu_system_wakeup() do a qemu_devices_reset()
if there's no MachineClass::wakeup, in a similar way to
how qemu_system_reset() does a qemu_devices_reset()
if there's no MachineClass::reset method ? Having the
wakeup event be "sometimes this will do a RESET_TYPE_WAKEUP
but sometimes it won't" doesn't seem right to me...

thanks
-- PMM

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

[Juraj Marcin]

On Tue, Aug 13, 2024 at 6:37 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 13 Aug 2024 at 16:39, Juraj Marcin <jmarcin@redhat.com> wrote:
> >
> > Some devices need to distinguish cold start reset from waking up from a
> > suspended state. This patch adds new value to the enum, and updates the
> > i386 wakeup method to use this new reset type.
> >
> > Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
> > Reviewed-by: David Hildenbrand <david@redhat.com>
> > ---
> >  docs/devel/reset.rst    | 8 ++++++++
> >  hw/i386/pc.c            | 2 +-
> >  include/hw/resettable.h | 2 ++
> >  3 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst
> > index 9746a4e8a0..a7c9467313 100644
> > --- a/docs/devel/reset.rst
> > +++ b/docs/devel/reset.rst
> > @@ -44,6 +44,14 @@ The Resettable interface handles reset types with an enum ``ResetType``:
> >    value on each cold reset, such as RNG seed information, and which they
> >    must not reinitialize on a snapshot-load reset.
> >
> > +``RESET_TYPE_WAKEUP``
> > +  This type is called for a reset when the system is being woken-up from a
> > +  suspended state using the ``qemu_system_wakeup()`` function. If the machine
> > +  needs to reset its devices in its ``MachineClass::wakeup()`` method, this
> > +  reset type should be used, so devices can differentiate system wake-up from
> > +  other reset types. For example, a virtio-mem device must not unplug its
> > +  memory during wake-up as that would clear the guest RAM.
> > +
> >  Devices which implement reset methods must treat any unknown ``ResetType``
> >  as equivalent to ``RESET_TYPE_COLD``; this will reduce the amount of
> >  existing code we need to change if we add more types in future.
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index ccb9731c91..49efd0a997 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1716,7 +1716,7 @@ static void pc_machine_reset(MachineState *machine, ResetType type)
> >  static void pc_machine_wakeup(MachineState *machine)
> >  {
> >      cpu_synchronize_all_states();
> > -    pc_machine_reset(machine, RESET_TYPE_COLD);
> > +    pc_machine_reset(machine, RESET_TYPE_WAKEUP);
> >      cpu_synchronize_all_post_reset();
> >  }
>
> I'm happy (following discussion in the previous thread)
> that 'wakeup' is the right reset event to be using here.
> But looking at the existing code for qemu_system_wakeup()
> something seems odd here. qemu_system_wakeup() calls
> the MachineClass::wakeup method if it's set, and does
> nothing if it's not. The PC implementation of that calls
> pc_machine_reset(), which does a qemu_devices_reset(),
> which does a complete three-phase reset of the system.
> But if the machine doesn't implement wakeup then we
> never reset the system at all.
>
> Shouldn't qemu_system_wakeup() do a qemu_devices_reset()
> if there's no MachineClass::wakeup, in a similar way to
> how qemu_system_reset() does a qemu_devices_reset()
> if there's no MachineClass::reset method ? Having the
> wakeup event be "sometimes this will do a RESET_TYPE_WAKEUP
> but sometimes it won't" doesn't seem right to me...

From my understanding that I have gathered from the code (but please,
someone correct me if I am wrong), this is machine specific. Some
machine types might not support suspend+wake-up at all. The support
has to be explicitly advertised through qemu_register_wakeup_support()
(for example, aarch64 with a generic virt machine type does not
advertise support). Even if the machine type advertises
suspend+wake-up support, it might not need to do anything machine
specific. This is the case of pSeries PowerPC machine (sPAPR) that
advertises support, but does not implement MachineClass::wakeup()
method as nothing needs to change in the machine state. [1]

So, if a restart during wake-up happens, it can be differentiated with
the wake-up reset type, and if the machine type does not need to reset
its devices during wake-up, there is no reset that needs to be
differentiated.

[1]: https://gitlab.com/qemu-project/qemu/-/blob/a733f37aef3b7d1d33bfe2716af88cdfd67ba64e/hw/ppc/spapr.c?#L3155

>
> thanks
> -- PMM
>

-- 
Juraj Marcin

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

[David Hildenbrand]

On 14.08.24 14:32, Juraj Marcin wrote:
> On Tue, Aug 13, 2024 at 6:37 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Tue, 13 Aug 2024 at 16:39, Juraj Marcin <jmarcin@redhat.com> wrote:
>>>
>>> Some devices need to distinguish cold start reset from waking up from a
>>> suspended state. This patch adds new value to the enum, and updates the
>>> i386 wakeup method to use this new reset type.
>>>
>>> Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>   docs/devel/reset.rst    | 8 ++++++++
>>>   hw/i386/pc.c            | 2 +-
>>>   include/hw/resettable.h | 2 ++
>>>   3 files changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst
>>> index 9746a4e8a0..a7c9467313 100644
>>> --- a/docs/devel/reset.rst
>>> +++ b/docs/devel/reset.rst
>>> @@ -44,6 +44,14 @@ The Resettable interface handles reset types with an enum ``ResetType``:
>>>     value on each cold reset, such as RNG seed information, and which they
>>>     must not reinitialize on a snapshot-load reset.
>>>
>>> +``RESET_TYPE_WAKEUP``
>>> +  This type is called for a reset when the system is being woken-up from a
>>> +  suspended state using the ``qemu_system_wakeup()`` function. If the machine
>>> +  needs to reset its devices in its ``MachineClass::wakeup()`` method, this
>>> +  reset type should be used, so devices can differentiate system wake-up from
>>> +  other reset types. For example, a virtio-mem device must not unplug its
>>> +  memory during wake-up as that would clear the guest RAM.
>>> +
>>>   Devices which implement reset methods must treat any unknown ``ResetType``
>>>   as equivalent to ``RESET_TYPE_COLD``; this will reduce the amount of
>>>   existing code we need to change if we add more types in future.
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index ccb9731c91..49efd0a997 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -1716,7 +1716,7 @@ static void pc_machine_reset(MachineState *machine, ResetType type)
>>>   static void pc_machine_wakeup(MachineState *machine)
>>>   {
>>>       cpu_synchronize_all_states();
>>> -    pc_machine_reset(machine, RESET_TYPE_COLD);
>>> +    pc_machine_reset(machine, RESET_TYPE_WAKEUP);
>>>       cpu_synchronize_all_post_reset();
>>>   }
>>
>> I'm happy (following discussion in the previous thread)
>> that 'wakeup' is the right reset event to be using here.
>> But looking at the existing code for qemu_system_wakeup()
>> something seems odd here. qemu_system_wakeup() calls
>> the MachineClass::wakeup method if it's set, and does
>> nothing if it's not. The PC implementation of that calls
>> pc_machine_reset(), which does a qemu_devices_reset(),
>> which does a complete three-phase reset of the system.
>> But if the machine doesn't implement wakeup then we
>> never reset the system at all.
>>
>> Shouldn't qemu_system_wakeup() do a qemu_devices_reset()
>> if there's no MachineClass::wakeup, in a similar way to
>> how qemu_system_reset() does a qemu_devices_reset()
>> if there's no MachineClass::reset method ? Having the
>> wakeup event be "sometimes this will do a RESET_TYPE_WAKEUP
>> but sometimes it won't" doesn't seem right to me...

One thing one could consider would probably be to send a WARM reset to 
all devices. The main issue here is that other devices will default to a 
COLD device then, and that's precisely what the other machines that 
implement suspend+resume do not want. And ...

> 
>  From my understanding that I have gathered from the code (but please,
> someone correct me if I am wrong), this is machine specific. Some
> machine types might not support suspend+wake-up at all. The support
> has to be explicitly advertised through qemu_register_wakeup_support()
> (for example, aarch64 with a generic virt machine type does not
> advertise support). Even if the machine type advertises
> suspend+wake-up support, it might not need to do anything machine
> specific. This is the case of pSeries PowerPC machine (sPAPR) that
> advertises support, but does not implement MachineClass::wakeup()
> method as nothing needs to change in the machine state. [1]
> 
> So, if a restart during wake-up happens, it can be differentiated with
> the wake-up reset type, and if the machine type does not need to reset
> its devices during wake-up, there is no reset that needs to be
> differentiated.

... if the machine does not do any resets during suspend+wakeup, this 
implies that there is not even a warm reset.

I guess we should make that clearer in the documentation: it's up to a 
machine implementation whether it wants to trigger a WARM reset during 
suspend+wakeup. If not, not resets will be performed at all.

@Peter, does that sound reasonable?

-- 
Cheers,

David / dhildenb

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

[Peter Maydell]

On Tue, 20 Aug 2024 at 12:40, David Hildenbrand <david@redhat.com> wrote:
>
> On 14.08.24 14:32, Juraj Marcin wrote:
> > On Tue, Aug 13, 2024 at 6:37 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >>
> >> On Tue, 13 Aug 2024 at 16:39, Juraj Marcin <jmarcin@redhat.com> wrote:
> >>>
> >>> Some devices need to distinguish cold start reset from waking up from a
> >>> suspended state. This patch adds new value to the enum, and updates the
> >>> i386 wakeup method to use this new reset type.
> >>>
> >>> Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
> >>> Reviewed-by: David Hildenbrand <david@redhat.com>
> >>> ---
> >>>   docs/devel/reset.rst    | 8 ++++++++
> >>>   hw/i386/pc.c            | 2 +-
> >>>   include/hw/resettable.h | 2 ++
> >>>   3 files changed, 11 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst
> >>> index 9746a4e8a0..a7c9467313 100644
> >>> --- a/docs/devel/reset.rst
> >>> +++ b/docs/devel/reset.rst
> >>> @@ -44,6 +44,14 @@ The Resettable interface handles reset types with an enum ``ResetType``:
> >>>     value on each cold reset, such as RNG seed information, and which they
> >>>     must not reinitialize on a snapshot-load reset.
> >>>
> >>> +``RESET_TYPE_WAKEUP``
> >>> +  This type is called for a reset when the system is being woken-up from a
> >>> +  suspended state using the ``qemu_system_wakeup()`` function. If the machine
> >>> +  needs to reset its devices in its ``MachineClass::wakeup()`` method, this
> >>> +  reset type should be used, so devices can differentiate system wake-up from
> >>> +  other reset types. For example, a virtio-mem device must not unplug its
> >>> +  memory during wake-up as that would clear the guest RAM.
> >>> +
> >>>   Devices which implement reset methods must treat any unknown ``ResetType``
> >>>   as equivalent to ``RESET_TYPE_COLD``; this will reduce the amount of
> >>>   existing code we need to change if we add more types in future.
> >>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >>> index ccb9731c91..49efd0a997 100644
> >>> --- a/hw/i386/pc.c
> >>> +++ b/hw/i386/pc.c
> >>> @@ -1716,7 +1716,7 @@ static void pc_machine_reset(MachineState *machine, ResetType type)
> >>>   static void pc_machine_wakeup(MachineState *machine)
> >>>   {
> >>>       cpu_synchronize_all_states();
> >>> -    pc_machine_reset(machine, RESET_TYPE_COLD);
> >>> +    pc_machine_reset(machine, RESET_TYPE_WAKEUP);
> >>>       cpu_synchronize_all_post_reset();
> >>>   }
> >>
> >> I'm happy (following discussion in the previous thread)
> >> that 'wakeup' is the right reset event to be using here.
> >> But looking at the existing code for qemu_system_wakeup()
> >> something seems odd here. qemu_system_wakeup() calls
> >> the MachineClass::wakeup method if it's set, and does
> >> nothing if it's not. The PC implementation of that calls
> >> pc_machine_reset(), which does a qemu_devices_reset(),
> >> which does a complete three-phase reset of the system.
> >> But if the machine doesn't implement wakeup then we
> >> never reset the system at all.
> >>
> >> Shouldn't qemu_system_wakeup() do a qemu_devices_reset()
> >> if there's no MachineClass::wakeup, in a similar way to
> >> how qemu_system_reset() does a qemu_devices_reset()
> >> if there's no MachineClass::reset method ? Having the
> >> wakeup event be "sometimes this will do a RESET_TYPE_WAKEUP
> >> but sometimes it won't" doesn't seem right to me...
>
> One thing one could consider would probably be to send a WARM reset to
> all devices. The main issue here is that other devices will default to a
> COLD device then, and that's precisely what the other machines that
> implement suspend+resume do not want. And ...
>
> >
> >  From my understanding that I have gathered from the code (but please,
> > someone correct me if I am wrong), this is machine specific. Some
> > machine types might not support suspend+wake-up at all. The support
> > has to be explicitly advertised through qemu_register_wakeup_support()
> > (for example, aarch64 with a generic virt machine type does not
> > advertise support). Even if the machine type advertises
> > suspend+wake-up support, it might not need to do anything machine
> > specific. This is the case of pSeries PowerPC machine (sPAPR) that
> > advertises support, but does not implement MachineClass::wakeup()
> > method as nothing needs to change in the machine state. [1]
> >
> > So, if a restart during wake-up happens, it can be differentiated with
> > the wake-up reset type, and if the machine type does not need to reset
> > its devices during wake-up, there is no reset that needs to be
> > differentiated.
>
> ... if the machine does not do any resets during suspend+wakeup, this
> implies that there is not even a warm reset.
>
> I guess we should make that clearer in the documentation: it's up to a
> machine implementation whether it wants to trigger a WARM reset during
> suspend+wakeup. If not, not resets will be performed at all.
>
> @Peter, does that sound reasonable?

Well, so far we've established that we need a "WAKEUP" reset
type, but not that we need a "WARM" reset type. The latter
would be something we'd need to trigger for quite a lot of
reset-causes where we currently default to COLD reset, so
I don't think we should do that until we need it.

If suspend-and-wakeup really is supposed to be a reset event
on some machines but not on others, that sounds unhelpfully
nonstandard, but then hardware designers rarely make choices
to make our lives easier :-) And yes, we should make sure
that's clear in the documentation.

I think with adding new reset events it's going to be
important that we're clear about what they are (and in
particular what the triggering events that cause them
are) so that we have a solid guide for what it means.
The thing in particular I'm hoping to avoid here is that
we vaguely define, for example, a "warm" reset type and then
different parts of the system interpret "warm" in different
ways.

thanks
-- PMM

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

[David Hildenbrand]

On 20.08.24 13:56, Peter Maydell wrote:
> On Tue, 20 Aug 2024 at 12:40, David Hildenbrand <david@redhat.com> wrote:
>>
>> On 14.08.24 14:32, Juraj Marcin wrote:
>>> On Tue, Aug 13, 2024 at 6:37 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>
>>>> On Tue, 13 Aug 2024 at 16:39, Juraj Marcin <jmarcin@redhat.com> wrote:
>>>>>
>>>>> Some devices need to distinguish cold start reset from waking up from a
>>>>> suspended state. This patch adds new value to the enum, and updates the
>>>>> i386 wakeup method to use this new reset type.
>>>>>
>>>>> Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
>>>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>>>> ---
>>>>>    docs/devel/reset.rst    | 8 ++++++++
>>>>>    hw/i386/pc.c            | 2 +-
>>>>>    include/hw/resettable.h | 2 ++
>>>>>    3 files changed, 11 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst
>>>>> index 9746a4e8a0..a7c9467313 100644
>>>>> --- a/docs/devel/reset.rst
>>>>> +++ b/docs/devel/reset.rst
>>>>> @@ -44,6 +44,14 @@ The Resettable interface handles reset types with an enum ``ResetType``:
>>>>>      value on each cold reset, such as RNG seed information, and which they
>>>>>      must not reinitialize on a snapshot-load reset.
>>>>>
>>>>> +``RESET_TYPE_WAKEUP``
>>>>> +  This type is called for a reset when the system is being woken-up from a
>>>>> +  suspended state using the ``qemu_system_wakeup()`` function. If the machine
>>>>> +  needs to reset its devices in its ``MachineClass::wakeup()`` method, this
>>>>> +  reset type should be used, so devices can differentiate system wake-up from
>>>>> +  other reset types. For example, a virtio-mem device must not unplug its
>>>>> +  memory during wake-up as that would clear the guest RAM.
>>>>> +
>>>>>    Devices which implement reset methods must treat any unknown ``ResetType``
>>>>>    as equivalent to ``RESET_TYPE_COLD``; this will reduce the amount of
>>>>>    existing code we need to change if we add more types in future.
>>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>>> index ccb9731c91..49efd0a997 100644
>>>>> --- a/hw/i386/pc.c
>>>>> +++ b/hw/i386/pc.c
>>>>> @@ -1716,7 +1716,7 @@ static void pc_machine_reset(MachineState *machine, ResetType type)
>>>>>    static void pc_machine_wakeup(MachineState *machine)
>>>>>    {
>>>>>        cpu_synchronize_all_states();
>>>>> -    pc_machine_reset(machine, RESET_TYPE_COLD);
>>>>> +    pc_machine_reset(machine, RESET_TYPE_WAKEUP);
>>>>>        cpu_synchronize_all_post_reset();
>>>>>    }
>>>>
>>>> I'm happy (following discussion in the previous thread)
>>>> that 'wakeup' is the right reset event to be using here.
>>>> But looking at the existing code for qemu_system_wakeup()
>>>> something seems odd here. qemu_system_wakeup() calls
>>>> the MachineClass::wakeup method if it's set, and does
>>>> nothing if it's not. The PC implementation of that calls
>>>> pc_machine_reset(), which does a qemu_devices_reset(),
>>>> which does a complete three-phase reset of the system.
>>>> But if the machine doesn't implement wakeup then we
>>>> never reset the system at all.
>>>>
>>>> Shouldn't qemu_system_wakeup() do a qemu_devices_reset()
>>>> if there's no MachineClass::wakeup, in a similar way to
>>>> how qemu_system_reset() does a qemu_devices_reset()
>>>> if there's no MachineClass::reset method ? Having the
>>>> wakeup event be "sometimes this will do a RESET_TYPE_WAKEUP
>>>> but sometimes it won't" doesn't seem right to me...
>>
>> One thing one could consider would probably be to send a WARM reset to
>> all devices. The main issue here is that other devices will default to a
>> COLD device then, and that's precisely what the other machines that
>> implement suspend+resume do not want. And ...
>>
>>>
>>>   From my understanding that I have gathered from the code (but please,
>>> someone correct me if I am wrong), this is machine specific. Some
>>> machine types might not support suspend+wake-up at all. The support
>>> has to be explicitly advertised through qemu_register_wakeup_support()
>>> (for example, aarch64 with a generic virt machine type does not
>>> advertise support). Even if the machine type advertises
>>> suspend+wake-up support, it might not need to do anything machine
>>> specific. This is the case of pSeries PowerPC machine (sPAPR) that
>>> advertises support, but does not implement MachineClass::wakeup()
>>> method as nothing needs to change in the machine state. [1]
>>>
>>> So, if a restart during wake-up happens, it can be differentiated with
>>> the wake-up reset type, and if the machine type does not need to reset
>>> its devices during wake-up, there is no reset that needs to be
>>> differentiated.
>>
>> ... if the machine does not do any resets during suspend+wakeup, this
>> implies that there is not even a warm reset.
>>
>> I guess we should make that clearer in the documentation: it's up to a
>> machine implementation whether it wants to trigger a WARM reset during
>> suspend+wakeup. If not, not resets will be performed at all.
>>
>> @Peter, does that sound reasonable?
> 
> Well, so far we've established that we need a "WAKEUP" reset
> type, but not that we need a "WARM" reset type. The latter

Sorry, I meant "WAKEUP" ... :)

> would be something we'd need to trigger for quite a lot of
> reset-causes where we currently default to COLD reset, so
> I don't think we should do that until we need it.
> 
> If suspend-and-wakeup really is supposed to be a reset event
> on some machines but not on others, that sounds unhelpfully
> nonstandard, but then hardware designers rarely make choices
> to make our lives easier :-) And yes, we should make sure
> that's clear in the documentation.
> 
> I think with adding new reset events it's going to be
> important that we're clear about what they are (and in
> particular what the triggering events that cause them
> are) so that we have a solid guide for what it means.
> The thing in particular I'm hoping to avoid here is that
> we vaguely define, for example, a "warm" reset type and then
> different parts of the system interpret "warm" in different
> ways.

Okay, so we should make it clear that a WAKEUP reset may be used on 
machines (especially x86) during a wakeup after suspending, when most of 
the devices are supposed to just do the same as a COLD reset, and only 
some of them (especially memory-layout/content related) need to preserve 
their state. The primary reason of the WAKEUP event is for these devices 
to handle these wakeups properly.

Then, we should document that some machines might not perform any reset 
during a wakeup, which will result in no resets to be triggered at all.

-- 
Cheers,

David / dhildenb

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

[Juraj Marcin]

Hi Peter,

On Tue, Aug 20, 2024 at 1:56 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 20 Aug 2024 at 12:40, David Hildenbrand <david@redhat.com> wrote:
> >
> > On 14.08.24 14:32, Juraj Marcin wrote:
> > > On Tue, Aug 13, 2024 at 6:37 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > >>
> > >> On Tue, 13 Aug 2024 at 16:39, Juraj Marcin <jmarcin@redhat.com> wrote:
> > >>>
> > >>> Some devices need to distinguish cold start reset from waking up from a
> > >>> suspended state. This patch adds new value to the enum, and updates the
> > >>> i386 wakeup method to use this new reset type.
> > >>>
> > >>> Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
> > >>> Reviewed-by: David Hildenbrand <david@redhat.com>
> > >>> ---
> > >>>   docs/devel/reset.rst    | 8 ++++++++
> > >>>   hw/i386/pc.c            | 2 +-
> > >>>   include/hw/resettable.h | 2 ++
> > >>>   3 files changed, 11 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst
> > >>> index 9746a4e8a0..a7c9467313 100644
> > >>> --- a/docs/devel/reset.rst
> > >>> +++ b/docs/devel/reset.rst
> > >>> @@ -44,6 +44,14 @@ The Resettable interface handles reset types with an enum ``ResetType``:
> > >>>     value on each cold reset, such as RNG seed information, and which they
> > >>>     must not reinitialize on a snapshot-load reset.
> > >>>
> > >>> +``RESET_TYPE_WAKEUP``
> > >>> +  This type is called for a reset when the system is being woken-up from a
> > >>> +  suspended state using the ``qemu_system_wakeup()`` function. If the machine
> > >>> +  needs to reset its devices in its ``MachineClass::wakeup()`` method, this
> > >>> +  reset type should be used, so devices can differentiate system wake-up from
> > >>> +  other reset types. For example, a virtio-mem device must not unplug its
> > >>> +  memory during wake-up as that would clear the guest RAM.
> > >>> +
> > >>>   Devices which implement reset methods must treat any unknown ``ResetType``
> > >>>   as equivalent to ``RESET_TYPE_COLD``; this will reduce the amount of
> > >>>   existing code we need to change if we add more types in future.
> > >>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > >>> index ccb9731c91..49efd0a997 100644
> > >>> --- a/hw/i386/pc.c
> > >>> +++ b/hw/i386/pc.c
> > >>> @@ -1716,7 +1716,7 @@ static void pc_machine_reset(MachineState *machine, ResetType type)
> > >>>   static void pc_machine_wakeup(MachineState *machine)
> > >>>   {
> > >>>       cpu_synchronize_all_states();
> > >>> -    pc_machine_reset(machine, RESET_TYPE_COLD);
> > >>> +    pc_machine_reset(machine, RESET_TYPE_WAKEUP);
> > >>>       cpu_synchronize_all_post_reset();
> > >>>   }
> > >>
> > >> I'm happy (following discussion in the previous thread)
> > >> that 'wakeup' is the right reset event to be using here.
> > >> But looking at the existing code for qemu_system_wakeup()
> > >> something seems odd here. qemu_system_wakeup() calls
> > >> the MachineClass::wakeup method if it's set, and does
> > >> nothing if it's not. The PC implementation of that calls
> > >> pc_machine_reset(), which does a qemu_devices_reset(),
> > >> which does a complete three-phase reset of the system.
> > >> But if the machine doesn't implement wakeup then we
> > >> never reset the system at all.
> > >>
> > >> Shouldn't qemu_system_wakeup() do a qemu_devices_reset()
> > >> if there's no MachineClass::wakeup, in a similar way to
> > >> how qemu_system_reset() does a qemu_devices_reset()
> > >> if there's no MachineClass::reset method ? Having the
> > >> wakeup event be "sometimes this will do a RESET_TYPE_WAKEUP
> > >> but sometimes it won't" doesn't seem right to me...
> >
> > One thing one could consider would probably be to send a WARM reset to
> > all devices. The main issue here is that other devices will default to a
> > COLD device then, and that's precisely what the other machines that
> > implement suspend+resume do not want. And ...
> >
> > >
> > >  From my understanding that I have gathered from the code (but please,
> > > someone correct me if I am wrong), this is machine specific. Some
> > > machine types might not support suspend+wake-up at all. The support
> > > has to be explicitly advertised through qemu_register_wakeup_support()
> > > (for example, aarch64 with a generic virt machine type does not
> > > advertise support). Even if the machine type advertises
> > > suspend+wake-up support, it might not need to do anything machine
> > > specific. This is the case of pSeries PowerPC machine (sPAPR) that
> > > advertises support, but does not implement MachineClass::wakeup()
> > > method as nothing needs to change in the machine state. [1]
> > >
> > > So, if a restart during wake-up happens, it can be differentiated with
> > > the wake-up reset type, and if the machine type does not need to reset
> > > its devices during wake-up, there is no reset that needs to be
> > > differentiated.
> >
> > ... if the machine does not do any resets during suspend+wakeup, this
> > implies that there is not even a warm reset.
> >
> > I guess we should make that clearer in the documentation: it's up to a
> > machine implementation whether it wants to trigger a WARM reset during
> > suspend+wakeup. If not, not resets will be performed at all.
> >
> > @Peter, does that sound reasonable?
>
> Well, so far we've established that we need a "WAKEUP" reset
> type, but not that we need a "WARM" reset type. The latter
> would be something we'd need to trigger for quite a lot of
> reset-causes where we currently default to COLD reset, so
> I don't think we should do that until we need it.
>
> If suspend-and-wakeup really is supposed to be a reset event
> on some machines but not on others, that sounds unhelpfully
> nonstandard, but then hardware designers rarely make choices
> to make our lives easier :-) And yes, we should make sure
> that's clear in the documentation.

I have rewritten the documentation section to make it more explicit
that the reset might not happen. I would appreciate feedback if some
part still needs some care or if it is clear now.

  If the machine supports waking up from a suspended state and needs to
  reset its devices during wake-up (from ``MachineClass::wakeup()``
  method), this reset type should be used for such a request. Devices
  can utilize this reset type to differentiate the reset requested
  during machine wake-up from other reset requests. For example, a
  virtio-mem device must not unplug its memory blocks during wake-up as
  the contents of the guest RAM would get lost. However, this reset type
  should not be used for wake-up detection, as not every machine type
  issues a device reset request during wake-up.

>
> I think with adding new reset events it's going to be
> important that we're clear about what they are (and in
> particular what the triggering events that cause them
> are) so that we have a solid guide for what it means.
> The thing in particular I'm hoping to avoid here is that
> we vaguely define, for example, a "warm" reset type and then
> different parts of the system interpret "warm" in different
> ways.
>
> thanks
> -- PMM
>

Thank you!

-- 

Juraj Marcin

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

[David Hildenbrand]

> I have rewritten the documentation section to make it more explicit
> that the reset might not happen. I would appreciate feedback if some
> part still needs some care or if it is clear now.
> 
>    If the machine supports waking up from a suspended state and needs to
>    reset its devices during wake-up (from ``MachineClass::wakeup()``
>    method), this reset type should be used for such a request. Devices
>    can utilize this reset type to differentiate the reset requested
>    during machine wake-up from other reset requests. For example, a
>    virtio-mem device must not unplug its memory blocks during wake-up as
>    the contents of the guest RAM would get lost. However, this reset type
>    should not be used for wake-up detection, as not every machine type
>    issues a device reset request during wake-up.

Sounds good to me.

I'd probably generalize the virtio-mem bit to:

"For example, RAM content must not be lost during wake-up, and memory 
devices like virtio-mem that provide additional RAM must not reset such 
state during wake-ups, but might do so during cold resets."


@Peter, WDYT?

-- 
Cheers,

David / dhildenb

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

[Peter Maydell]

On Thu, 29 Aug 2024 at 16:48, David Hildenbrand <david@redhat.com> wrote:
>
> > I have rewritten the documentation section to make it more explicit
> > that the reset might not happen. I would appreciate feedback if some
> > part still needs some care or if it is clear now.
> >
> >    If the machine supports waking up from a suspended state and needs to
> >    reset its devices during wake-up (from ``MachineClass::wakeup()``
> >    method), this reset type should be used for such a request. Devices
> >    can utilize this reset type to differentiate the reset requested
> >    during machine wake-up from other reset requests. For example, a
> >    virtio-mem device must not unplug its memory blocks during wake-up as
> >    the contents of the guest RAM would get lost. However, this reset type
> >    should not be used for wake-up detection, as not every machine type
> >    issues a device reset request during wake-up.
>
> Sounds good to me.
>
> I'd probably generalize the virtio-mem bit to:
>
> "For example, RAM content must not be lost during wake-up, and memory
> devices like virtio-mem that provide additional RAM must not reset such
> state during wake-ups, but might do so during cold resets."
>
>
> @Peter, WDYT?

Yep, seems good to me: I think it's clear about when this
reset type happens and what you can/can't expect from it.

Minor grammar nit: should be
"from the ``MachineClass::wakeup()`` method".

-- PMM