diff mbox series

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

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

Commit Message

Juraj Marcin Aug. 6, 2024, 4:07 p.m. UTC 
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(-)

Comments

David Hildenbrand Aug. 6, 2024, 4:39 p.m. UTC | #1 
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>
Juraj Marcin Aug. 7, 2024, 10:03 a.m. UTC | #2 
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
>
Peter Maydell Aug. 8, 2024, 12:32 p.m. UTC | #3 
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
diff mbox series

Patch

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: