[PATCH 00/10] reset: Make whole system three-phase-reset aware

[PATCH 00/10] reset: Make whole system three-phase-reset aware

[Peter Maydell]

This patchset is an incremental improvement to our reset handling that
tries to roll out the "three-phase-reset" design we have for devices
to a wider scope.

At the moment devices and buses have a three-phase reset system, with
separate 'enter', 'hold' and 'exit' phases. When the qbus tree is
reset, first all the devices on it have their 'enter' method called,
then they all have 'enter' called, and finally 'exit'. The idea is
that we can use this, among other things, as a way to resolve annoying
"this bit of reset work needs to happen before this other bit of reset
work" ordering issues.

However, there is still a "legacy" reset option, where you register a
callback function with qemu_register_reset(). These functions know
nothing about the three-phase system, and "reset the qbus" is just one
of the things set up to happen at reset via qemu_register_reset().
That means that a qemu_register_reset() function might happen before
all the enter/hold/exit phases of device reset, or it might happen after
all of them.

This patchset provides a new way to register a three-phase-aware reset
in this list of "reset the whole system" actions, and reimplements
qemu_register_reset() in terms of that new mechanism. This means that
qemu_register_reset() functions are now all called in the 'hold' phase
of system reset. (This is an ordering change, so in theory it could
introduce new bugs if we are accidentally depending on the current
ordering; but we have very few enter and exit phase methods at the
moment so I don't expect much trouble, if any.)

The first three patches remove the only two places in the codebase
that rely on "a reset callback can unregister itself within the
callback"; this is awkward to continue to support in the new
implementation, and an unusual thing to do given that reset is in
principle supposed to be something you can do as many times as you
like, not something that behaves differently the first time through.

Patch 4 is an improvement to the QOM macros that's been on list as an
RFC already.
Patches 5 and 6 are the "new mechanism": qemu_register_resettable()
takes any object that implements the Resettable interface. System
reset is then doing 3-phase reset on all of these, so everything
gets its 'enter' phase called, then everything gets 'hold', then
everything gets 'exit'.

Patch 7 reimplements the qemu_register_reset() API to be
"qemu_register_resettable(), and the callback function is called
in the 'hold' phase".

Patch 8 makes the first real use of the new API: instead of
doing the qbus reset via qemu_register_reset(), we pass the
root of the qbus to qemu_register_resettable(). (This is where
the ordering change I mention above happens, as device enter and
exit method calls will now happen before and after all the
qemu_register_reset() function callbacks, rather than in the
middle of them.)

Finally, patch 9 updates the developer docs to describe how a
complete system reset currently works.

This series doesn't actually do a great deal as it stands, but I
think it's a necessary foundation for some cleanups:
 * the vfio reset ordering problem discussed on list a while back
   should now hopefully be solvable by having the vfio code use
   qemu_register_resettable() so it can arrange to do the "needs to
   happen last" stuff in an exit phase method
 * there are some other missing pieces here, but eventually I think
   it should be possible to get rid of the workarounds for
   dependencies between ROM image loading and CPUs that want to
   read an initial PC/SP on reset (eg arm, m68k)
I also think it's a good idea to get it into the tree so that we
have a chance to see if there are any unexpected regressions
before we start putting in bugfixes etc that depend on it :-)

After this, I think the next thing I'm going to look at is whether
we can move the MachineState class from inheriting from TYPE_OBJECT
to TYPE_DEVICE. This would allow us to have machine-level reset
(and would bring "machine as a container of devices" into line
with "SoC object as container of devices").

thanks
-- PMM

Peter Maydell (10):
  hw/i386: Store pointers to IDE buses in PCMachineState
  hw/i386/pc: Do pc_cmos_init_late() from pc_machine_done()
  system/bootdevice: Don't unregister reset handler in
    restore_boot_order()
  include/qom/object.h: New OBJECT_DEFINE_SIMPLE_TYPE{,_WITH_INTERFACES}
    macros
  hw/core: Add documentation and license comments to reset.h
  hw/core: Add ResetContainer which holds objects implementing
    Resettable
  hw/core/reset: Add qemu_{register,unregister}_resettable()
  hw/core/reset: Implement qemu_register_reset via
    qemu_register_resettable
  hw/core/machine: Use qemu_register_resettable for sysbus reset
  docs/devel/reset: Update to discuss system reset

 MAINTAINERS                      |  10 ++
 docs/devel/qom.rst               |  34 ++++++-
 docs/devel/reset.rst             |  44 +++++++-
 include/hw/core/resetcontainer.h |  48 +++++++++
 include/hw/i386/pc.h             |   4 +-
 include/qom/object.h             | 114 ++++++++++++++++-----
 include/sysemu/reset.h           | 113 +++++++++++++++++++++
 hw/core/machine.c                |   7 +-
 hw/core/reset.c                  | 166 +++++++++++++++++++++++++------
 hw/core/resetcontainer.c         |  76 ++++++++++++++
 hw/i386/pc.c                     |  40 +++-----
 hw/i386/pc_piix.c                |  16 ++-
 hw/i386/pc_q35.c                 |   9 +-
 system/bootdevice.c              |  25 +++--
 hw/core/meson.build              |   1 +
 15 files changed, 587 insertions(+), 120 deletions(-)
 create mode 100644 include/hw/core/resetcontainer.h
 create mode 100644 hw/core/resetcontainer.c

-- 
2.34.1

[PATCH 01/10] hw/i386: Store pointers to IDE buses in PCMachineState

[Peter Maydell]

Add the two IDE bus BusState pointers to the set we keep in PCMachineState.
This allows us to avoid passing them to pc_cmos_init(), and also will
allow a refactoring of how we call pc_cmos_init_late().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/i386/pc.h |  4 +++-
 hw/i386/pc.c         |  5 ++---
 hw/i386/pc_piix.c    | 16 +++++++---------
 hw/i386/pc_q35.c     |  9 ++++-----
 4 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index ec0e5efcb28..8f8ac894b10 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -17,6 +17,8 @@
 
 #define HPET_INTCAP "hpet-intcap"
 
+#define MAX_IDE_BUS 2
+
 /**
  * PCMachineState:
  * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling
@@ -37,6 +39,7 @@ typedef struct PCMachineState {
     PFlashCFI01 *flash[2];
     ISADevice *pcspk;
     DeviceState *iommu;
+    BusState *idebus[MAX_IDE_BUS];
 
     /* Configuration options: */
     uint64_t max_ram_below_4g;
@@ -182,7 +185,6 @@ void pc_basic_device_init(struct PCMachineState *pcms,
                           bool create_fdctrl,
                           uint32_t hpet_irqs);
 void pc_cmos_init(PCMachineState *pcms,
-                  BusState *ide0, BusState *ide1,
                   ISADevice *s);
 void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus);
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 196827531a5..8b0f54e284c 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -574,7 +574,6 @@ static void pc_cmos_init_late(void *opaque)
 }
 
 void pc_cmos_init(PCMachineState *pcms,
-                  BusState *idebus0, BusState *idebus1,
                   ISADevice *rtc)
 {
     int val;
@@ -634,8 +633,8 @@ void pc_cmos_init(PCMachineState *pcms,
 
     /* hard drives and FDC */
     arg.rtc_state = s;
-    arg.idebus[0] = idebus0;
-    arg.idebus[1] = idebus1;
+    arg.idebus[0] = pcms->idebus[0];
+    arg.idebus[1] = pcms->idebus[1];
     qemu_register_reset(pc_cmos_init_late, &arg);
 }
 
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 999b7b806ca..8df88a6ccd1 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -68,7 +68,6 @@
 #include "kvm/kvm-cpu.h"
 #include "target/i386/cpu.h"
 
-#define MAX_IDE_BUS 2
 #define XEN_IOAPIC_NUM_PIRQS 128ULL
 
 #ifdef CONFIG_IDE_ISA
@@ -114,7 +113,6 @@ static void pc_init1(MachineState *machine,
     Object *piix4_pm = NULL;
     qemu_irq smi_irq;
     GSIState *gsi_state;
-    BusState *idebus[MAX_IDE_BUS];
     ISADevice *rtc_state;
     MemoryRegion *ram_memory;
     MemoryRegion *pci_memory = NULL;
@@ -299,8 +297,8 @@ static void pc_init1(MachineState *machine,
         piix4_pm = object_resolve_path_component(OBJECT(pci_dev), "pm");
         dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "ide"));
         pci_ide_create_devs(PCI_DEVICE(dev));
-        idebus[0] = qdev_get_child_bus(dev, "ide.0");
-        idebus[1] = qdev_get_child_bus(dev, "ide.1");
+        pcms->idebus[0] = qdev_get_child_bus(dev, "ide.0");
+        pcms->idebus[1] = qdev_get_child_bus(dev, "ide.1");
     } else {
         isa_bus = isa_bus_new(NULL, system_memory, system_io,
                               &error_abort);
@@ -312,8 +310,8 @@ static void pc_init1(MachineState *machine,
 
         i8257_dma_init(OBJECT(machine), isa_bus, 0);
         pcms->hpet_enabled = false;
-        idebus[0] = NULL;
-        idebus[1] = NULL;
+        pcms->idebus[0] = NULL;
+        pcms->idebus[1] = NULL;
     }
 
     if (x86ms->pic == ON_OFF_AUTO_ON || x86ms->pic == ON_OFF_AUTO_AUTO) {
@@ -342,7 +340,7 @@ static void pc_init1(MachineState *machine,
     pc_nic_init(pcmc, isa_bus, pci_bus);
 
     if (pcmc->pci_enabled) {
-        pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
+        pc_cmos_init(pcms, rtc_state);
     }
 #ifdef CONFIG_IDE_ISA
     else {
@@ -361,9 +359,9 @@ static void pc_init1(MachineState *machine,
              * second one.
              */
             busname[4] = '0' + i;
-            idebus[i] = qdev_get_child_bus(DEVICE(dev), busname);
+            pcms->idebus[i] = qdev_get_child_bus(DEVICE(dev), busname);
         }
-        pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
+        pc_cmos_init(pcms, rtc_state);
     }
 #endif
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index d346fa3b1d6..71402c36eb2 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -126,7 +126,6 @@ static void pc_q35_init(MachineState *machine)
     PCIBus *host_bus;
     PCIDevice *lpc;
     DeviceState *lpc_dev;
-    BusState *idebus[MAX_SATA_PORTS];
     ISADevice *rtc_state;
     MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *system_io = get_system_io();
@@ -300,13 +299,13 @@ static void pc_q35_init(MachineState *machine)
                                                          ICH9_SATA1_FUNC),
                                                "ich9-ahci");
         ich9 = ICH9_AHCI(pdev);
-        idebus[0] = qdev_get_child_bus(DEVICE(pdev), "ide.0");
-        idebus[1] = qdev_get_child_bus(DEVICE(pdev), "ide.1");
+        pcms->idebus[0] = qdev_get_child_bus(DEVICE(pdev), "ide.0");
+        pcms->idebus[1] = qdev_get_child_bus(DEVICE(pdev), "ide.1");
         g_assert(MAX_SATA_PORTS == ich9->ahci.ports);
         ide_drive_get(hd, ich9->ahci.ports);
         ahci_ide_create_devs(&ich9->ahci, hd);
     } else {
-        idebus[0] = idebus[1] = NULL;
+        pcms->idebus[0] = pcms->idebus[1] = NULL;
     }
 
     if (machine_usb(machine)) {
@@ -327,7 +326,7 @@ static void pc_q35_init(MachineState *machine)
         smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
     }
 
-    pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
+    pc_cmos_init(pcms, rtc_state);
 
     /* the rest devices to which pci devfn is automatically assigned */
     pc_vga_init(isa_bus, host_bus);
-- 
2.34.1

[PATCH 02/10] hw/i386/pc: Do pc_cmos_init_late() from pc_machine_done()

[Peter Maydell]

In the i386 PC machine, we want to run the pc_cmos_init_late()
function only once the IDE and floppy drive devices have been set up.
We currently do this using qemu_register_reset(), and then have the
function call qemu_unregister_reset() on itself, so it runs exactly
once.

This was an expedient way to do it back in 2010 when we first added
this (in commit c0897e0cb94e8), but now we have a more obvious point
to do "machine initialization that has to happen after generic device
init": the machine-init-done hook.

Do the pc_cmos_init_late() work from our existing PC machine init
done hook function, so we can drop the use of qemu_register_reset()
and qemu_unregister_reset().

Because the pointers to the devices we need (the IDE buses and the
RTC) are now all in the machine state, we don't need the
pc_cmos_init_late_arg struct and can just pass the PCMachineState
pointer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/i386/pc.c | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 8b0f54e284c..4c3cfe9fc35 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -465,11 +465,6 @@ static void pc_cmos_init_floppy(MC146818RtcState *rtc_state, ISADevice *floppy)
     mc146818rtc_set_cmos_data(rtc_state, REG_EQUIPMENT_BYTE, val);
 }
 
-typedef struct pc_cmos_init_late_arg {
-    MC146818RtcState *rtc_state;
-    BusState *idebus[2];
-} pc_cmos_init_late_arg;
-
 typedef struct check_fdc_state {
     ISADevice *floppy;
     bool multiple;
@@ -530,23 +525,25 @@ static ISADevice *pc_find_fdc0(void)
     return state.floppy;
 }
 
-static void pc_cmos_init_late(void *opaque)
+static void pc_cmos_init_late(PCMachineState *pcms)
 {
-    pc_cmos_init_late_arg *arg = opaque;
-    MC146818RtcState *s = arg->rtc_state;
+    X86MachineState *x86ms = X86_MACHINE(pcms);
+    MC146818RtcState *s = MC146818_RTC(x86ms->rtc);
     int16_t cylinders;
     int8_t heads, sectors;
     int val;
     int i, trans;
 
     val = 0;
-    if (arg->idebus[0] && ide_get_geometry(arg->idebus[0], 0,
-                                           &cylinders, &heads, &sectors) >= 0) {
+    if (pcms->idebus[0] &&
+        ide_get_geometry(pcms->idebus[0], 0,
+                         &cylinders, &heads, &sectors) >= 0) {
         cmos_init_hd(s, 0x19, 0x1b, cylinders, heads, sectors);
         val |= 0xf0;
     }
-    if (arg->idebus[0] && ide_get_geometry(arg->idebus[0], 1,
-                                           &cylinders, &heads, &sectors) >= 0) {
+    if (pcms->idebus[0] &&
+        ide_get_geometry(pcms->idebus[0], 1,
+                         &cylinders, &heads, &sectors) >= 0) {
         cmos_init_hd(s, 0x1a, 0x24, cylinders, heads, sectors);
         val |= 0x0f;
     }
@@ -558,10 +555,11 @@ static void pc_cmos_init_late(void *opaque)
            geometry.  It is always such that: 1 <= sects <= 63, 1
            <= heads <= 16, 1 <= cylinders <= 16383. The BIOS
            geometry can be different if a translation is done. */
-        if (arg->idebus[i / 2] &&
-            ide_get_geometry(arg->idebus[i / 2], i % 2,
+        BusState *idebus = pcms->idebus[i / 2];
+        if (idebus &&
+            ide_get_geometry(idebus, i % 2,
                              &cylinders, &heads, &sectors) >= 0) {
-            trans = ide_get_bios_chs_trans(arg->idebus[i / 2], i % 2) - 1;
+            trans = ide_get_bios_chs_trans(idebus, i % 2) - 1;
             assert((trans & ~3) == 0);
             val |= trans << (i * 2);
         }
@@ -569,15 +567,12 @@ static void pc_cmos_init_late(void *opaque)
     mc146818rtc_set_cmos_data(s, 0x39, val);
 
     pc_cmos_init_floppy(s, pc_find_fdc0());
-
-    qemu_unregister_reset(pc_cmos_init_late, opaque);
 }
 
 void pc_cmos_init(PCMachineState *pcms,
                   ISADevice *rtc)
 {
     int val;
-    static pc_cmos_init_late_arg arg;
     X86MachineState *x86ms = X86_MACHINE(pcms);
     MC146818RtcState *s = MC146818_RTC(rtc);
 
@@ -631,11 +626,7 @@ void pc_cmos_init(PCMachineState *pcms,
     val |= 0x04; /* PS/2 mouse installed */
     mc146818rtc_set_cmos_data(s, REG_EQUIPMENT_BYTE, val);
 
-    /* hard drives and FDC */
-    arg.rtc_state = s;
-    arg.idebus[0] = pcms->idebus[0];
-    arg.idebus[1] = pcms->idebus[1];
-    qemu_register_reset(pc_cmos_init_late, &arg);
+    /* hard drives and FDC are handled by pc_cmos_init_late() */
 }
 
 static void handle_a20_line_change(void *opaque, int irq, int level)
@@ -703,6 +694,8 @@ void pc_machine_done(Notifier *notifier, void *data)
         /* update FW_CFG_NB_CPUS to account for -device added CPUs */
         fw_cfg_modify_i16(x86ms->fw_cfg, FW_CFG_NB_CPUS, x86ms->boot_cpus);
     }
+
+    pc_cmos_init_late(pcms);
 }
 
 void pc_guest_info_init(PCMachineState *pcms)
-- 
2.34.1

[PATCH 03/10] system/bootdevice: Don't unregister reset handler in restore_boot_order()

[Peter Maydell]

Currently the qemu_register_reset() API permits the reset handler functions
registered with it to remove themselves from within the callback function.
This is fine with our current implementation, but is a bit odd, because
generally reset is supposed to be idempotent, and doesn't fit well in a
three-phase-reset world where a resettable object will get multiple
callbacks as the system is reset.

We now have only one user of qemu_register_reset() which makes use of
the ability to unregister itself within the callback:
restore_boot_order().  We want to change our implementation of
qemu_register_reset() to something where it would be awkward to
maintain the "can self-unregister" feature.  Rather than making that
reimplementation complicated, change restore_boot_order() so that it
doesn't unregister itself but instead returns doing nothing for any
calls after it has done the "restore the boot order" work.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
It would be nicer not to use reset at all, especially since I'm not
a fan of conflating "system is reset" with "system boots", but I
didn't have a good idea for how to do that.
---
 system/bootdevice.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/system/bootdevice.c b/system/bootdevice.c
index 2106f1026ff..2c55c9bd90c 100644
--- a/system/bootdevice.c
+++ b/system/bootdevice.c
@@ -101,20 +101,23 @@ void validate_bootdevices(const char *devices, Error **errp)
 void restore_boot_order(void *opaque)
 {
     char *normal_boot_order = opaque;
-    static int first = 1;
+    static int bootcount = 0;
 
-    /* Restore boot order and remove ourselves after the first boot */
-    if (first) {
-        first = 0;
+    switch (bootcount++) {
+    case 0:
+        /* First boot: use the one-time config */
+        return;
+    case 1:
+        /* Second boot: restore normal boot order */
+        if (boot_set_handler) {
+            qemu_boot_set(normal_boot_order, &error_abort);
+        }
+        g_free(normal_boot_order);
+        return;
+    default:
+        /* Subsequent boots: keep using normal boot order */
         return;
     }
-
-    if (boot_set_handler) {
-        qemu_boot_set(normal_boot_order, &error_abort);
-    }
-
-    qemu_unregister_reset(restore_boot_order, normal_boot_order);
-    g_free(normal_boot_order);
 }
 
 void check_boot_index(int32_t bootindex, Error **errp)
-- 
2.34.1

[PATCH 04/10] include/qom/object.h: New OBJECT_DEFINE_SIMPLE_TYPE{, _WITH_INTERFACES} macros

[Peter Maydell]

We have an OBJECT_DEFINE_TYPE_EXTENDED macro, plus several variations
on it, which emits the boilerplate for the TypeInfo and ensures it is
registered with the type system.  However, all the existing macros
insist that the type being defined has its own FooClass struct, so
they aren't useful for the common case of a simple leaf class which
doesn't have any new methods or any other need for its own class
struct (that is, for the kind of type that OBJECT_DECLARE_SIMPLE_TYPE
declares).

Pull the actual implementation of OBJECT_DEFINE_TYPE_EXTENDED out
into a new DO_OBJECT_DEFINE_TYPE_EXTENDED which parameterizes the
value we use for the class_size field.  This lets us add a new
OBJECT_DEFINE_SIMPLE_TYPE which does the same job as the various
existing OBJECT_DEFINE_*_TYPE_* family macros for this kind of simple
type, and the variant OBJECT_DEFINE_SIMPLE_TYPE_WITH_INTERFACES for
when the type will implement some interfaces.

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 docs/devel/qom.rst   |  34 +++++++++++--
 include/qom/object.h | 114 +++++++++++++++++++++++++++++++++----------
 2 files changed, 117 insertions(+), 31 deletions(-)

diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst
index 9918fac7f21..0889ca949c1 100644
--- a/docs/devel/qom.rst
+++ b/docs/devel/qom.rst
@@ -348,12 +348,14 @@ used. This does the same as OBJECT_DECLARE_SIMPLE_TYPE(), but without
 the 'struct MyDeviceClass' definition.
 
 To implement the type, the OBJECT_DEFINE macro family is available.
-In the simple case the OBJECT_DEFINE_TYPE macro is suitable:
+For the simplest case of a leaf class which doesn't need any of its
+own virtual functions (i.e. which was declared with OBJECT_DECLARE_SIMPLE_TYPE)
+the OBJECT_DEFINE_SIMPLE_TYPE macro is suitable:
 
 .. code-block:: c
    :caption: Defining a simple type
 
-   OBJECT_DEFINE_TYPE(MyDevice, my_device, MY_DEVICE, DEVICE)
+   OBJECT_DEFINE_SIMPLE_TYPE(MyDevice, my_device, MY_DEVICE, DEVICE)
 
 This is equivalent to the following:
 
@@ -370,7 +372,6 @@ This is equivalent to the following:
        .instance_size = sizeof(MyDevice),
        .instance_init = my_device_init,
        .instance_finalize = my_device_finalize,
-       .class_size = sizeof(MyDeviceClass),
        .class_init = my_device_class_init,
    };
 
@@ -385,13 +386,36 @@ This is sufficient to get the type registered with the type
 system, and the three standard methods now need to be implemented
 along with any other logic required for the type.
 
+If the class needs its own virtual methods, or has some other
+per-class state it needs to store in its own class struct,
+then you can use the OBJECT_DEFINE_TYPE macro. This does the
+same thing as OBJECT_DEFINE_SIMPLE_TYPE, but it also sets the
+class_size of the type to the size of the class struct.
+
+.. code-block:: c
+   :caption: Defining a type which needs a class struct
+
+   OBJECT_DEFINE_TYPE(MyDevice, my_device, MY_DEVICE, DEVICE)
+
 If the type needs to implement one or more interfaces, then the
-OBJECT_DEFINE_TYPE_WITH_INTERFACES() macro can be used instead.
-This accepts an array of interface type names.
+OBJECT_DEFINE_SIMPLE_TYPE_WITH_INTERFACES() and
+OBJECT_DEFINE_TYPE_WITH_INTERFACES() macros can be used instead.
+These accept an array of interface type names. The difference between
+them is that the former is for simple leaf classes that don't need
+a class struct, and the latter is for when you will be defining
+a class struct.
 
 .. code-block:: c
    :caption: Defining a simple type implementing interfaces
 
+   OBJECT_DEFINE_SIMPLE_TYPE_WITH_INTERFACES(MyDevice, my_device,
+                                             MY_DEVICE, DEVICE,
+                                             { TYPE_USER_CREATABLE },
+                                             { NULL })
+
+.. code-block:: c
+   :caption: Defining a type implementing interfaces
+
    OBJECT_DEFINE_TYPE_WITH_INTERFACES(MyDevice, my_device,
                                       MY_DEVICE, DEVICE,
                                       { TYPE_USER_CREATABLE },
diff --git a/include/qom/object.h b/include/qom/object.h
index afccd24ca7a..f52ab216cdd 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -258,6 +258,51 @@ struct Object
     DECLARE_INSTANCE_CHECKER(InstanceType, MODULE_OBJ_NAME, TYPE_##MODULE_OBJ_NAME)
 
 
+/**
+ * DO_OBJECT_DEFINE_TYPE_EXTENDED:
+ * @ModuleObjName: the object name with initial caps
+ * @module_obj_name: the object name in lowercase with underscore separators
+ * @MODULE_OBJ_NAME: the object name in uppercase with underscore separators
+ * @PARENT_MODULE_OBJ_NAME: the parent object name in uppercase with underscore
+ *                          separators
+ * @ABSTRACT: boolean flag to indicate whether the object can be instantiated
+ * @CLASS_SIZE: size of the type's class
+ * @...: list of initializers for "InterfaceInfo" to declare implemented interfaces
+ *
+ * This is the base macro used to implement all the OBJECT_DEFINE_*
+ * macros. It should never be used directly in a source file.
+ */
+#define DO_OBJECT_DEFINE_TYPE_EXTENDED(ModuleObjName, module_obj_name, \
+                                       MODULE_OBJ_NAME, \
+                                       PARENT_MODULE_OBJ_NAME, \
+                                       ABSTRACT, CLASS_SIZE, ...) \
+    static void \
+    module_obj_name##_finalize(Object *obj); \
+    static void \
+    module_obj_name##_class_init(ObjectClass *oc, void *data); \
+    static void \
+    module_obj_name##_init(Object *obj); \
+    \
+    static const TypeInfo module_obj_name##_info = { \
+        .parent = TYPE_##PARENT_MODULE_OBJ_NAME, \
+        .name = TYPE_##MODULE_OBJ_NAME, \
+        .instance_size = sizeof(ModuleObjName), \
+        .instance_align = __alignof__(ModuleObjName), \
+        .instance_init = module_obj_name##_init, \
+        .instance_finalize = module_obj_name##_finalize, \
+        .class_size = CLASS_SIZE, \
+        .class_init = module_obj_name##_class_init, \
+        .abstract = ABSTRACT, \
+        .interfaces = (InterfaceInfo[]) { __VA_ARGS__ } , \
+    }; \
+    \
+    static void \
+    module_obj_name##_register_types(void) \
+    { \
+        type_register_static(&module_obj_name##_info); \
+    } \
+    type_init(module_obj_name##_register_types);
+
 /**
  * OBJECT_DEFINE_TYPE_EXTENDED:
  * @ModuleObjName: the object name with initial caps
@@ -284,32 +329,10 @@ struct Object
 #define OBJECT_DEFINE_TYPE_EXTENDED(ModuleObjName, module_obj_name, \
                                     MODULE_OBJ_NAME, PARENT_MODULE_OBJ_NAME, \
                                     ABSTRACT, ...) \
-    static void \
-    module_obj_name##_finalize(Object *obj); \
-    static void \
-    module_obj_name##_class_init(ObjectClass *oc, void *data); \
-    static void \
-    module_obj_name##_init(Object *obj); \
-    \
-    static const TypeInfo module_obj_name##_info = { \
-        .parent = TYPE_##PARENT_MODULE_OBJ_NAME, \
-        .name = TYPE_##MODULE_OBJ_NAME, \
-        .instance_size = sizeof(ModuleObjName), \
-        .instance_align = __alignof__(ModuleObjName), \
-        .instance_init = module_obj_name##_init, \
-        .instance_finalize = module_obj_name##_finalize, \
-        .class_size = sizeof(ModuleObjName##Class), \
-        .class_init = module_obj_name##_class_init, \
-        .abstract = ABSTRACT, \
-        .interfaces = (InterfaceInfo[]) { __VA_ARGS__ } , \
-    }; \
-    \
-    static void \
-    module_obj_name##_register_types(void) \
-    { \
-        type_register_static(&module_obj_name##_info); \
-    } \
-    type_init(module_obj_name##_register_types);
+    DO_OBJECT_DEFINE_TYPE_EXTENDED(ModuleObjName, module_obj_name, \
+                                   MODULE_OBJ_NAME, PARENT_MODULE_OBJ_NAME, \
+                                   ABSTRACT, sizeof(ModuleObjName##Class), \
+                                   __VA_ARGS__)
 
 /**
  * OBJECT_DEFINE_TYPE:
@@ -368,6 +391,45 @@ struct Object
                                 MODULE_OBJ_NAME, PARENT_MODULE_OBJ_NAME, \
                                 true, { NULL })
 
+/**
+ * OBJECT_DEFINE_SIMPLE_TYPE_WITH_INTERFACES:
+ * @ModuleObjName: the object name with initial caps
+ * @module_obj_name: the object name in lowercase with underscore separators
+ * @MODULE_OBJ_NAME: the object name in uppercase with underscore separators
+ * @PARENT_MODULE_OBJ_NAME: the parent object name in uppercase with underscore
+ *                          separators
+ *
+ * This is a variant of OBJECT_DEFINE_TYPE_EXTENDED, which is suitable for
+ * the case of a non-abstract type, with interfaces, and with no requirement
+ * for a class struct.
+ */
+#define OBJECT_DEFINE_SIMPLE_TYPE_WITH_INTERFACES(ModuleObjName, \
+                                                  module_obj_name, \
+                                                  MODULE_OBJ_NAME, \
+                                                  PARENT_MODULE_OBJ_NAME, ...) \
+    DO_OBJECT_DEFINE_TYPE_EXTENDED(ModuleObjName, module_obj_name, \
+                                   MODULE_OBJ_NAME, PARENT_MODULE_OBJ_NAME, \
+                                   false, 0, __VA_ARGS__)
+
+/**
+ * OBJECT_DEFINE_SIMPLE_TYPE:
+ * @ModuleObjName: the object name with initial caps
+ * @module_obj_name: the object name in lowercase with underscore separators
+ * @MODULE_OBJ_NAME: the object name in uppercase with underscore separators
+ * @PARENT_MODULE_OBJ_NAME: the parent object name in uppercase with underscore
+ *                          separators
+ *
+ * This is a variant of OBJECT_DEFINE_TYPE_EXTENDED, which is suitable for
+ * the common case of a non-abstract type, without any interfaces, and with
+ * no requirement for a class struct. If you declared your type with
+ * OBJECT_DECLARE_SIMPLE_TYPE then this is probably the right choice for
+ * defining it.
+ */
+#define OBJECT_DEFINE_SIMPLE_TYPE(ModuleObjName, module_obj_name, \
+                                  MODULE_OBJ_NAME, PARENT_MODULE_OBJ_NAME) \
+    OBJECT_DEFINE_SIMPLE_TYPE_WITH_INTERFACES(ModuleObjName, module_obj_name, \
+        MODULE_OBJ_NAME, PARENT_MODULE_OBJ_NAME, { NULL })
+
 /**
  * struct TypeInfo:
  * @name: The name of the type.
-- 
2.34.1

[PATCH 05/10] hw/core: Add documentation and license comments to reset.h

[Peter Maydell]

Add the usual boilerplate license/copyright comment to reset.h (using
the text from reset.c), and document the existing functions.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/sysemu/reset.h | 79 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/include/sysemu/reset.h b/include/sysemu/reset.h
index 609e4d50c26..6aa11846a69 100644
--- a/include/sysemu/reset.h
+++ b/include/sysemu/reset.h
@@ -1,3 +1,29 @@
+/*
+ *  Reset handlers.
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ * Copyright (c) 2016 Red Hat, Inc.
+ * Copyright (c) 2024 Linaro, Ltd.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
 #ifndef QEMU_SYSEMU_RESET_H
 #define QEMU_SYSEMU_RESET_H
 
@@ -5,9 +31,62 @@
 
 typedef void QEMUResetHandler(void *opaque);
 
+/**
+ * qemu_register_reset: Register a callback for system reset
+ * @func: function to call
+ * @opaque: opaque data to pass to @func
+ *
+ * Register @func on the list of functions which are called when the
+ * entire system is reset. The functions are called in the order in
+ * which they are registered.
+ *
+ * In general this function should not be used in new code where possible;
+ * for instance device model reset is better accomplished using the
+ * methods on DeviceState.
+ *
+ * It is not permitted to register or unregister reset functions from
+ * within the @func callback.
+ *
+ * We assume that the caller holds the BQL.
+ */
 void qemu_register_reset(QEMUResetHandler *func, void *opaque);
+
+/**
+ * qemu_register_reset_nosnapshotload: Register a callback for system reset
+ * @func: function to call
+ * @opaque: opaque data to pass to @func
+ *
+ * This is the same as qemu_register_reset(), except that @func is
+ * not called if the reason that the system is being reset is to
+ * put it into a clean state prior to loading a snapshot (i.e. for
+ * SHUTDOWN_CAUSE_SNAPSHOT_LOAD).
+ */
 void qemu_register_reset_nosnapshotload(QEMUResetHandler *func, void *opaque);
+
+/**
+ * qemu_unregister_reset: Unregister a system reset callback
+ * @func: function registered with qemu_register_reset()
+ * @opaque: the same opaque data that was passed to qemu_register_reset()
+ *
+ * Undo the effects of a qemu_register_reset(). The @func and @opaque
+ * must both match the arguments originally used with qemu_register_reset().
+ *
+ * We assume that the caller holds the BQL.
+ */
 void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
+
+/**
+ * qemu_devices_reset: Perform a complete system reset
+ * @reason: reason for the reset
+ *
+ * This function performs the low-level work needed to do a complete reset
+ * of the system (calling all the callbacks registered with
+ * qemu_register_reset()). It should only be called by the code in a
+ * MachineClass reset method.
+ *
+ * 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);
 
 #endif
-- 
2.34.1

[PATCH 06/10] hw/core: Add ResetContainer which holds objects implementing Resettable

[Peter Maydell]

Implement a ResetContainer.  This is a subclass of Object, and it
implements the Resettable interface.  The container holds a list of
arbitrary other objects which implement Resettable, and when the
container is reset, all the objects it contains are also reset.

This will allow us to have a 3-phase-reset equivalent of the old
qemu_register_reset() API: we will have a single "simulation reset"
top level ResetContainer, and objects in it are the equivalent of the
old QEMUResetHandler functions.

The qemu_register_reset() API manages its list of callbacks using a
QTAILQ, but here we use a GPtrArray for our list of Resettable
children: we expect the "remove" operation (which will need to do an
iteration through the list) to be fairly uncommon, and we get simpler
code with fewer memory allocations.

Since there is currently no listed owner in MAINTAINERS for the
existing reset-related source files, create a new section for
them, and add these new files there also.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 MAINTAINERS                      | 10 +++++
 include/hw/core/resetcontainer.h | 48 ++++++++++++++++++++
 hw/core/resetcontainer.c         | 76 ++++++++++++++++++++++++++++++++
 hw/core/meson.build              |  1 +
 4 files changed, 135 insertions(+)
 create mode 100644 include/hw/core/resetcontainer.h
 create mode 100644 hw/core/resetcontainer.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7d61fb93194..df8a526905b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3667,6 +3667,16 @@ F: hw/core/clock-vmstate.c
 F: hw/core/qdev-clock.c
 F: docs/devel/clocks.rst
 
+Reset framework
+M: Peter Maydell <peter.maydell@linaro.org>
+S: Maintained
+F: include/hw/resettable.h
+F: include/hw/core/resetcontainer.h
+F: include/sysemu/reset.h
+F: hw/core/reset.c
+F: hw/core/resettable.c
+F: hw/core/resetcontainer.c
+
 Usermode Emulation
 ------------------
 Overall usermode emulation
diff --git a/include/hw/core/resetcontainer.h b/include/hw/core/resetcontainer.h
new file mode 100644
index 00000000000..23db0c7a880
--- /dev/null
+++ b/include/hw/core/resetcontainer.h
@@ -0,0 +1,48 @@
+/*
+ * Reset container
+ *
+ * Copyright (c) 2024 Linaro, Ltd
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef HW_RESETCONTAINER_H
+#define HW_RESETCONTAINER_H
+
+/*
+ * The "reset container" is an object which implements the Resettable
+ * interface. It contains a list of arbitrary other objects which also
+ * implement Resettable. Resetting the reset container resets all the
+ * objects in it.
+ */
+
+#include "qom/object.h"
+
+#define TYPE_RESETTABLE_CONTAINER "resettable-container"
+OBJECT_DECLARE_TYPE(ResettableContainer, ResettableContainerClass, RESETTABLE_CONTAINER)
+
+/**
+ * resettable_container_add: Add a resettable object to the container
+ * @rc: container
+ * @obj: object to add to the container
+ *
+ * Add @obj to the ResettableContainer @rc. @obj must implement the
+ * Resettable interface.
+ *
+ * When @rc is reset, it will reset every object that has been added
+ * to it, in the order they were added.
+ */
+void resettable_container_add(ResettableContainer *rc, Object *obj);
+
+/**
+ * resettable_container_remove: Remove an object from the container
+ * @rc: container
+ * @obj: object to remove from the container
+ *
+ * Remove @obj from the ResettableContainer @rc. @obj must have been
+ * previously added to this container.
+ */
+void resettable_container_remove(ResettableContainer *rc, Object *obj);
+
+#endif
diff --git a/hw/core/resetcontainer.c b/hw/core/resetcontainer.c
new file mode 100644
index 00000000000..cd627f16f0e
--- /dev/null
+++ b/hw/core/resetcontainer.c
@@ -0,0 +1,76 @@
+/*
+ * Reset container
+ *
+ * Copyright (c) 2024 Linaro, Ltd
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+/*
+ * The "reset container" is an object which implements the Resettable
+ * interface. It contains a list of arbitrary other objects which also
+ * implement Resettable. Resetting the reset container resets all the
+ * objects in it.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/resettable.h"
+#include "hw/core/resetcontainer.h"
+
+struct ResettableContainer {
+    Object parent;
+    ResettableState reset_state;
+    GPtrArray *children;
+};
+
+OBJECT_DEFINE_SIMPLE_TYPE_WITH_INTERFACES(ResettableContainer, resettable_container, RESETTABLE_CONTAINER, OBJECT, { TYPE_RESETTABLE_INTERFACE }, { })
+
+void resettable_container_add(ResettableContainer *rc, Object *obj)
+{
+    g_ptr_array_add(rc->children, obj);
+}
+
+void resettable_container_remove(ResettableContainer *rc, Object *obj)
+{
+    g_ptr_array_remove(rc->children, obj);
+}
+
+static ResettableState *resettable_container_get_state(Object *obj)
+{
+    ResettableContainer *rc = RESETTABLE_CONTAINER(obj);
+    return &rc->reset_state;
+}
+
+static void resettable_container_child_foreach(Object *obj,
+                                               ResettableChildCallback cb,
+                                               void *opaque, ResetType type)
+{
+    ResettableContainer *rc = RESETTABLE_CONTAINER(obj);
+    unsigned int len = rc->children->len;
+
+    for (unsigned int i = 0; i < len; i++) {
+        cb(g_ptr_array_index(rc->children, i), opaque, type);
+        /* Detect callbacks trying to unregister themselves */
+        assert(len == rc->children->len);
+    }
+}
+
+static void resettable_container_init(Object *obj)
+{
+    ResettableContainer *rc = RESETTABLE_CONTAINER(obj);
+
+    rc->children = g_ptr_array_new();
+}
+
+static void resettable_container_finalize(Object *obj)
+{
+}
+
+static void resettable_container_class_init(ObjectClass *klass, void *data)
+{
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
+
+    rc->get_state = resettable_container_get_state;
+    rc->child_foreach = resettable_container_child_foreach;
+}
diff --git a/hw/core/meson.build b/hw/core/meson.build
index 67dad04de55..e26f2e088c3 100644
--- a/hw/core/meson.build
+++ b/hw/core/meson.build
@@ -4,6 +4,7 @@ hwcore_ss.add(files(
   'qdev-properties.c',
   'qdev.c',
   'reset.c',
+  'resetcontainer.c',
   'resettable.c',
   'vmstate-if.c',
   # irq.c needed for qdev GPIO handling:
-- 
2.34.1

[PATCH 07/10] hw/core/reset: Add qemu_{register, unregister}_resettable()

[Peter Maydell]

Implement new functions qemu_register_resettable() and
qemu_unregister_resettable().  These are intended to be
three-phase-reset aware equivalents of the old qemu_register_reset()
and qemu_unregister_reset().  Instead of passing in a function
pointer and opaque, you register any QOM object that implements the
Resettable interface.

The implementation is simple: we have a single global instance of a
ResettableContainer, which we reset in qemu_devices_reset(), and
the Resettable objects passed to qemu_register_resettable() are
added to it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/sysemu/reset.h | 37 ++++++++++++++++++++++++++++++++++---
 hw/core/reset.c        | 31 +++++++++++++++++++++++++++++--
 2 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/include/sysemu/reset.h b/include/sysemu/reset.h
index 6aa11846a69..a3de48cb9cf 100644
--- a/include/sysemu/reset.h
+++ b/include/sysemu/reset.h
@@ -31,6 +31,36 @@
 
 typedef void QEMUResetHandler(void *opaque);
 
+/**
+ * qemu_register_resettable: Register an object to be reset
+ * @obj: object to be reset: it must implement the Resettable interface
+ *
+ * Register @obj on the list of objects which will be reset when the
+ * simulation is reset. These objects will be reset in the order
+ * they were added, using the three-phase Resettable protocol,
+ * so first all objects go through the enter phase, then all objects
+ * go through the hold phase, and then finally all go through the
+ * exit phase.
+ *
+ * It is not permitted to register or unregister reset functions or
+ * resettable objects from within any of the reset phase methods of @obj.
+ *
+ * We assume that the caller holds the BQL.
+ */
+void qemu_register_resettable(Object *obj);
+
+/**
+ * qemu_unregister_resettable: Unregister an object to be reset
+ * @obj: object to unregister
+ *
+ * Remove @obj from the list of objects which are reset when the
+ * simulation is reset. It must have been previously added to
+ * the list via qemu_register_resettable().
+ *
+ * We assume that the caller holds the BQL.
+ */
+void qemu_unregister_resettable(Object *obj);
+
 /**
  * qemu_register_reset: Register a callback for system reset
  * @func: function to call
@@ -44,8 +74,8 @@ typedef void QEMUResetHandler(void *opaque);
  * for instance device model reset is better accomplished using the
  * methods on DeviceState.
  *
- * It is not permitted to register or unregister reset functions from
- * within the @func callback.
+ * It is not permitted to register or unregister reset functions or
+ * resettable objects from within the @func callback.
  *
  * We assume that the caller holds the BQL.
  */
@@ -81,7 +111,8 @@ void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
  *
  * This function performs the low-level work needed to do a complete reset
  * of the system (calling all the callbacks registered with
- * qemu_register_reset()). It should only be called by the code in a
+ * qemu_register_reset() and resetting all the Resettable objects registered
+ * with qemu_register_resettable()). It should only be called by the code in a
  * MachineClass reset method.
  *
  * If you want to trigger a system reset from, for instance, a device
diff --git a/hw/core/reset.c b/hw/core/reset.c
index d3263b613e6..a9b30e705fe 100644
--- a/hw/core/reset.c
+++ b/hw/core/reset.c
@@ -26,8 +26,23 @@
 #include "qemu/osdep.h"
 #include "qemu/queue.h"
 #include "sysemu/reset.h"
+#include "hw/resettable.h"
+#include "hw/core/resetcontainer.h"
 
-/* reset/shutdown handler */
+/*
+ * Return a pointer to the singleton container that holds all the Resettable
+ * items that will be reset when qemu_devices_reset() is called.
+ */
+static ResettableContainer *get_root_reset_container(void)
+{
+    static ResettableContainer *root_reset_container;
+
+    if (!root_reset_container) {
+        root_reset_container =
+            RESETTABLE_CONTAINER(object_new(TYPE_RESETTABLE_CONTAINER));
+    }
+    return root_reset_container;
+}
 
 typedef struct QEMUResetEntry {
     QTAILQ_ENTRY(QEMUResetEntry) entry;
@@ -71,6 +86,16 @@ void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
     }
 }
 
+void qemu_register_resettable(Object *obj)
+{
+    resettable_container_add(get_root_reset_container(), obj);
+}
+
+void qemu_unregister_resettable(Object *obj)
+{
+    resettable_container_remove(get_root_reset_container(), obj);
+}
+
 void qemu_devices_reset(ShutdownCause reason)
 {
     QEMUResetEntry *re, *nre;
@@ -83,5 +108,7 @@ void qemu_devices_reset(ShutdownCause reason)
         }
         re->func(re->opaque);
     }
-}
 
+    /* Reset the simulation */
+    resettable_reset(OBJECT(get_root_reset_container()), RESET_TYPE_COLD);
+}
-- 
2.34.1

[PATCH 08/10] hw/core/reset: Implement qemu_register_reset via qemu_register_resettable

[Peter Maydell]

Reimplement qemu_register_reset() via qemu_register_resettable().

We define a new LegacyReset object which implements Resettable and
defines its reset hold phase method to call a QEMUResetHandler
function.  When qemu_register_reset() is called, we create a new
LegacyReset object and add it to the simulation_reset
ResettableContainer.  When qemu_unregister_reset() is called, we find
the LegacyReset object in the container and remove it.

This implementation of qemu_unregister_reset() means we'll end up
scanning the ResetContainer's list of child objects twice, once
to find the LegacyReset object, and once in g_ptr_array_remove().
In theory we could avoid this by having the ResettableContainer
interface include a resettable_container_remove_with_equal_func()
that took a callback method so that we could use
g_ptr_array_find_with_equal_func() and g_ptr_array_remove_index().
But we don't expect qemu_unregister_reset() to be called frequently
or in hot paths, and we expect the simulation_reset container to
usually not have many children.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
The way that a legacy reset function needs to check the ShutdownCause
and this doesn't line up with the ResetType is a bit awkward; this
is an area we should come back and clean up, but I didn't want to
tackle that in this patchset.
---
 include/sysemu/reset.h |   7 ++-
 hw/core/reset.c        | 137 +++++++++++++++++++++++++++++++----------
 2 files changed, 110 insertions(+), 34 deletions(-)

diff --git a/include/sysemu/reset.h b/include/sysemu/reset.h
index a3de48cb9cf..ada4527e67e 100644
--- a/include/sysemu/reset.h
+++ b/include/sysemu/reset.h
@@ -67,8 +67,11 @@ void qemu_unregister_resettable(Object *obj);
  * @opaque: opaque data to pass to @func
  *
  * Register @func on the list of functions which are called when the
- * entire system is reset. The functions are called in the order in
- * which they are registered.
+ * entire system is reset. Functions registered with this API and
+ * Resettable objects registered with qemu_register_resettable() are
+ * handled together, in the order in which they were registered.
+ * Functions registered with this API are called in the 'hold' phase
+ * of the 3-phase reset.
  *
  * In general this function should not be used in new code where possible;
  * for instance device model reset is better accomplished using the
diff --git a/hw/core/reset.c b/hw/core/reset.c
index a9b30e705fe..d50da7e3041 100644
--- a/hw/core/reset.c
+++ b/hw/core/reset.c
@@ -24,7 +24,6 @@
  */
 
 #include "qemu/osdep.h"
-#include "qemu/queue.h"
 #include "sysemu/reset.h"
 #include "hw/resettable.h"
 #include "hw/core/resetcontainer.h"
@@ -44,45 +43,128 @@ static ResettableContainer *get_root_reset_container(void)
     return root_reset_container;
 }
 
-typedef struct QEMUResetEntry {
-    QTAILQ_ENTRY(QEMUResetEntry) entry;
+/*
+ * Reason why the currently in-progress qemu_devices_reset() was called.
+ * If we made at least SHUTDOWN_CAUSE_SNAPSHOT_LOAD have a corresponding
+ * ResetType we could perhaps avoid the need for this global.
+ */
+static ShutdownCause device_reset_reason;
+
+/*
+ * This is an Object which implements Resettable simply to call the
+ * callback function in the hold phase.
+ */
+#define TYPE_LEGACY_RESET "legacy-reset"
+OBJECT_DECLARE_SIMPLE_TYPE(LegacyReset, LEGACY_RESET)
+
+struct LegacyReset {
+    Object parent;
+    ResettableState reset_state;
     QEMUResetHandler *func;
     void *opaque;
     bool skip_on_snapshot_load;
-} QEMUResetEntry;
+};
 
-static QTAILQ_HEAD(, QEMUResetEntry) reset_handlers =
-    QTAILQ_HEAD_INITIALIZER(reset_handlers);
+OBJECT_DEFINE_SIMPLE_TYPE_WITH_INTERFACES(LegacyReset, legacy_reset, LEGACY_RESET, OBJECT, { TYPE_RESETTABLE_INTERFACE }, { })
+
+static ResettableState *legacy_reset_get_state(Object *obj)
+{
+    LegacyReset *lr = LEGACY_RESET(obj);
+    return &lr->reset_state;
+}
+
+static void legacy_reset_hold(Object *obj)
+{
+    LegacyReset *lr = LEGACY_RESET(obj);
+
+    if (device_reset_reason == SHUTDOWN_CAUSE_SNAPSHOT_LOAD &&
+        lr->skip_on_snapshot_load) {
+        return;
+    }
+    lr->func(lr->opaque);
+}
+
+static void legacy_reset_init(Object *obj)
+{
+}
+
+static void legacy_reset_finalize(Object *obj)
+{
+}
+
+static void legacy_reset_class_init(ObjectClass *klass, void *data)
+{
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
+
+    rc->get_state = legacy_reset_get_state;
+    rc->phases.hold = legacy_reset_hold;
+}
 
 void qemu_register_reset(QEMUResetHandler *func, void *opaque)
 {
-    QEMUResetEntry *re = g_new0(QEMUResetEntry, 1);
+    Object *obj = object_new(TYPE_LEGACY_RESET);
+    LegacyReset *lr = LEGACY_RESET(obj);
 
-    re->func = func;
-    re->opaque = opaque;
-    QTAILQ_INSERT_TAIL(&reset_handlers, re, entry);
+    lr->func = func;
+    lr->opaque = opaque;
+    qemu_register_resettable(obj);
 }
 
 void qemu_register_reset_nosnapshotload(QEMUResetHandler *func, void *opaque)
 {
-    QEMUResetEntry *re = g_new0(QEMUResetEntry, 1);
+    Object *obj = object_new(TYPE_LEGACY_RESET);
+    LegacyReset *lr = LEGACY_RESET(obj);
 
-    re->func = func;
-    re->opaque = opaque;
-    re->skip_on_snapshot_load = true;
-    QTAILQ_INSERT_TAIL(&reset_handlers, re, entry);
+    lr->func = func;
+    lr->opaque = opaque;
+    lr->skip_on_snapshot_load = true;
+    qemu_register_resettable(obj);
+}
+
+typedef struct FindLegacyInfo {
+    QEMUResetHandler *func;
+    void *opaque;
+    LegacyReset *lr;
+} FindLegacyInfo;
+
+static void find_legacy_reset_cb(Object *obj, void *opaque, ResetType type)
+{
+    LegacyReset *lr;
+    FindLegacyInfo *fli = opaque;
+
+    /* Not everything in the ResettableContainer will be a LegacyReset */
+    lr = LEGACY_RESET(object_dynamic_cast(obj, TYPE_LEGACY_RESET));
+    if (lr && lr->func == fli->func && lr->opaque == fli->opaque) {
+        fli->lr = lr;
+    }
+}
+
+static LegacyReset *find_legacy_reset(QEMUResetHandler *func, void *opaque)
+{
+    /*
+     * Find the LegacyReset with the specified func and opaque,
+     * by getting the ResettableContainer to call our callback for
+     * every item in it.
+     */
+    ResettableContainer *rootcon = get_root_reset_container();
+    ResettableClass *rc = RESETTABLE_GET_CLASS(rootcon);
+    FindLegacyInfo fli;
+
+    fli.func = func;
+    fli.opaque = opaque;
+    fli.lr = NULL;
+    rc->child_foreach(OBJECT(rootcon), find_legacy_reset_cb,
+                      &fli, RESET_TYPE_COLD);
+    return fli.lr;
 }
 
 void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
 {
-    QEMUResetEntry *re;
+    Object *obj = OBJECT(find_legacy_reset(func, opaque));
 
-    QTAILQ_FOREACH(re, &reset_handlers, entry) {
-        if (re->func == func && re->opaque == opaque) {
-            QTAILQ_REMOVE(&reset_handlers, re, entry);
-            g_free(re);
-            return;
-        }
+    if (obj) {
+        qemu_unregister_resettable(obj);
+        object_unref(obj);
     }
 }
 
@@ -98,16 +180,7 @@ void qemu_unregister_resettable(Object *obj)
 
 void qemu_devices_reset(ShutdownCause reason)
 {
-    QEMUResetEntry *re, *nre;
-
-    /* reset all devices */
-    QTAILQ_FOREACH_SAFE(re, &reset_handlers, entry, nre) {
-        if (reason == SHUTDOWN_CAUSE_SNAPSHOT_LOAD &&
-            re->skip_on_snapshot_load) {
-            continue;
-        }
-        re->func(re->opaque);
-    }
+    device_reset_reason = reason;
 
     /* Reset the simulation */
     resettable_reset(OBJECT(get_root_reset_container()), RESET_TYPE_COLD);
-- 
2.34.1

[PATCH 09/10] hw/core/machine: Use qemu_register_resettable for sysbus reset

[Peter Maydell]

Move the reset of the sysbus (and thus all devices and buses anywhere
on the qbus tree) from qemu_register_reset() to qemu_register_resettable().

This is a behaviour change: because qemu_register_resettable() is
aware of three-phase reset, this now means that:
 * 'enter' phase reset methods of devices and buses are called
   before any legacy reset callbacks registered with qemu_register_reset()
 * 'exit' phase reset methods of devices and buses are called
   after any legacy qemu_register_reset() callbacks

Put another way, a qemu_register_reset() callback is now correctly
ordered in the 'hold' phase along with any other 'hold' phase methods.

The motivation for doing this is that we will now be able to resolve
some reset-ordering issues using the three-phase mechanism, because
the 'exit' phase is always after the 'hold' phase, even when the
'hold' phase function was registered with qemu_register_reset().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I believe that given we don't make much use of enter/exit phases
currently that this is unlikely to cause unexpected regressions due
to an accidental reset-order dependency that is no longer satisfied,
but it's always possible...
---
 hw/core/machine.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index fb5afdcae4c..9ac5d5389a6 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1577,14 +1577,13 @@ void qdev_machine_creation_done(void)
     /* TODO: once all bus devices are qdevified, this should be done
      * when bus is created by qdev.c */
     /*
-     * TODO: If we had a main 'reset container' that the whole system
-     * lived in, we could reset that using the multi-phase reset
-     * APIs. For the moment, we just reset the sysbus, which will cause
+     * This is where we arrange for the sysbus to be reset when the
+     * whole simulation is reset. In turn, resetting the sysbus will cause
      * all devices hanging off it (and all their child buses, recursively)
      * to be reset. Note that this will *not* reset any Device objects
      * which are not attached to some part of the qbus tree!
      */
-    qemu_register_reset(resettable_cold_reset_fn, sysbus_get_default());
+    qemu_register_resettable(OBJECT(sysbus_get_default()));
 
     notifier_list_notify(&machine_init_done_notifiers, NULL);
 
-- 
2.34.1

[PATCH 10/10] docs/devel/reset: Update to discuss system reset

[Peter Maydell]

Now that system reset uses a three-phase-reset, update the reset
documentation to include a section describing how this works.
Include documentation of the current major beartrap in reset, which
is that only devices on the qbus tree will get automatically reset.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This merely documents the current situation, and says nothing
about what we might like to do with it in future...
---
 docs/devel/reset.rst | 44 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst
index d4e79718bac..2ea85e7779b 100644
--- a/docs/devel/reset.rst
+++ b/docs/devel/reset.rst
@@ -11,8 +11,8 @@ whole group can be reset consistently. Each individual member object does not
 have to care about others; in particular, problems of order (which object is
 reset first) are addressed.
 
-As of now DeviceClass and BusClass implement this interface.
-
+The main object types which implement this interface are DeviceClass
+and BusClass.
 
 Triggering reset
 ----------------
@@ -288,3 +288,43 @@ There is currently 2 cases where this function is used:
 2. *hot bus change*; it means an existing live device is added, moved or
    removed in the bus hierarchy. At the moment, it occurs only in the raspi
    machines for changing the sdbus used by sd card.
+
+Reset of the complete system
+----------------------------
+
+Reset of the complete system is a little complicated. The typical
+flow is:
+
+1. Code which wishes to reset the entire system does so by calling
+   ``qemu_system_reset_request()``. This schedules a reset, but the
+   reset will happen asynchronously after the function returns.
+   That makes this safe to call from, for example, device models.
+
+2. The function which is called to make the reset happen is
+   ``qemu_system_reset()``. Generally only core system code should
+   call this directly.
+
+3. ``qemu_system_reset()`` calls the ``MachineClass::reset`` method of
+   the current machine, if it has one. That method must call
+   ``qemu_devices_reset()``. If the machine has no reset method,
+   ``qemu_system_reset()`` calls ``qemu_devices_reset()`` directly.
+
+4. ``qemu_devices_reset()`` performs a reset of the system, using
+   the three-phase mechanism listed above. It resets all objects
+   that were registered with it using ``qemu_register_resettable()``.
+   It also calls all the functions registered with it using
+   ``qemu_register_reset()``. Those functions are called during the
+   "hold" phase of this reset.
+
+5. The most important object that this reset resets is the
+   'sysbus' bus. The sysbus bus is the root of the qbus tree. This
+   means that all devices on the sysbus are reset, and all their
+   child buses, and all the devices on those child buses.
+
+6. Devices which are not on the qbus tree are *not* automatically
+   reset! (The most obvious example of this is CPU objects, but
+   anything that directly inherits from ``TYPE_OBJECT`` or ``TYPE_DEVICE``
+   rather than from ``TYPE_SYS_BUS_DEVICE`` or some other plugs-into-a-bus
+   type will be in this category.) You need to therefore arrange for these
+   to be reset in some other way (e.g. using ``qemu_register_resettable()``
+   or ``qemu_register_reset()``).
-- 
2.34.1

Re: [PATCH 09/10] hw/core/machine: Use qemu_register_resettable for sysbus reset

[Philippe Mathieu-Daudé]

On 20/2/24 17:06, Peter Maydell wrote:
> Move the reset of the sysbus (and thus all devices and buses anywhere
> on the qbus tree) from qemu_register_reset() to qemu_register_resettable().
> 
> This is a behaviour change: because qemu_register_resettable() is
> aware of three-phase reset, this now means that:
>   * 'enter' phase reset methods of devices and buses are called
>     before any legacy reset callbacks registered with qemu_register_reset()
>   * 'exit' phase reset methods of devices and buses are called
>     after any legacy qemu_register_reset() callbacks
> 
> Put another way, a qemu_register_reset() callback is now correctly
> ordered in the 'hold' phase along with any other 'hold' phase methods.
> 
> The motivation for doing this is that we will now be able to resolve
> some reset-ordering issues using the three-phase mechanism, because
> the 'exit' phase is always after the 'hold' phase, even when the
> 'hold' phase function was registered with qemu_register_reset().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I believe that given we don't make much use of enter/exit phases
> currently that this is unlikely to cause unexpected regressions due
> to an accidental reset-order dependency that is no longer satisfied,
> but it's always possible...
> ---
>   hw/core/machine.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index fb5afdcae4c..9ac5d5389a6 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1577,14 +1577,13 @@ void qdev_machine_creation_done(void)
>       /* TODO: once all bus devices are qdevified, this should be done
>        * when bus is created by qdev.c */
>       /*
> -     * TODO: If we had a main 'reset container' that the whole system
> -     * lived in, we could reset that using the multi-phase reset
> -     * APIs. For the moment, we just reset the sysbus, which will cause
> +     * This is where we arrange for the sysbus to be reset when the
> +     * whole simulation is reset. In turn, resetting the sysbus will cause
>        * all devices hanging off it (and all their child buses, recursively)
>        * to be reset. Note that this will *not* reset any Device objects
>        * which are not attached to some part of the qbus tree!
>        */
> -    qemu_register_reset(resettable_cold_reset_fn, sysbus_get_default());

Interestingly after this patch TYPE_S390_IPL is the last device
using resettable_cold_reset_fn(). Per commit cd45c506c8e:

     /*
      * Because this Device is not on any bus in the qbus tree (it is
      * not a sysbus device and it's not on some other bus like a PCI
      * bus) it will not be automatically reset by the 'reset the
      * sysbus' hook registered by vl.c like most devices. So we must
      * manually register a reset hook for it.
      * TODO: there should be a better way to do this.
      */

> +    qemu_register_resettable(OBJECT(sysbus_get_default()));
>   
>       notifier_list_notify(&machine_init_done_notifiers, NULL);
>   

Re: [PATCH 09/10] hw/core/machine: Use qemu_register_resettable for sysbus reset

[Peter Maydell]

On Tue, 20 Feb 2024 at 19:06, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 20/2/24 17:06, Peter Maydell wrote:
> > Move the reset of the sysbus (and thus all devices and buses anywhere
> > on the qbus tree) from qemu_register_reset() to qemu_register_resettable().
> >
> > This is a behaviour change: because qemu_register_resettable() is
> > aware of three-phase reset, this now means that:
> >   * 'enter' phase reset methods of devices and buses are called
> >     before any legacy reset callbacks registered with qemu_register_reset()
> >   * 'exit' phase reset methods of devices and buses are called
> >     after any legacy qemu_register_reset() callbacks
> >
> > Put another way, a qemu_register_reset() callback is now correctly
> > ordered in the 'hold' phase along with any other 'hold' phase methods.
> >
> > The motivation for doing this is that we will now be able to resolve
> > some reset-ordering issues using the three-phase mechanism, because
> > the 'exit' phase is always after the 'hold' phase, even when the
> > 'hold' phase function was registered with qemu_register_reset().
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > I believe that given we don't make much use of enter/exit phases
> > currently that this is unlikely to cause unexpected regressions due
> > to an accidental reset-order dependency that is no longer satisfied,
> > but it's always possible...
> > ---
> >   hw/core/machine.c | 7 +++----
> >   1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index fb5afdcae4c..9ac5d5389a6 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -1577,14 +1577,13 @@ void qdev_machine_creation_done(void)
> >       /* TODO: once all bus devices are qdevified, this should be done
> >        * when bus is created by qdev.c */
> >       /*
> > -     * TODO: If we had a main 'reset container' that the whole system
> > -     * lived in, we could reset that using the multi-phase reset
> > -     * APIs. For the moment, we just reset the sysbus, which will cause
> > +     * This is where we arrange for the sysbus to be reset when the
> > +     * whole simulation is reset. In turn, resetting the sysbus will cause
> >        * all devices hanging off it (and all their child buses, recursively)
> >        * to be reset. Note that this will *not* reset any Device objects
> >        * which are not attached to some part of the qbus tree!
> >        */
> > -    qemu_register_reset(resettable_cold_reset_fn, sysbus_get_default());
>
> Interestingly after this patch TYPE_S390_IPL is the last device
> using resettable_cold_reset_fn(). Per commit cd45c506c8e:
>
>      /*
>       * Because this Device is not on any bus in the qbus tree (it is
>       * not a sysbus device and it's not on some other bus like a PCI
>       * bus) it will not be automatically reset by the 'reset the
>       * sysbus' hook registered by vl.c like most devices. So we must
>       * manually register a reset hook for it.
>       * TODO: there should be a better way to do this.
>       */

Mmm, we could now have that s390 code call
qemu_register_resettable(OBJECT(dev)).
Though the "better way" remark still applies, because ideally we shouldn't
be doing reset only via the qbus tree.

thanks
-- PMM

Re: [PATCH 01/10] hw/i386: Store pointers to IDE buses in PCMachineState

[Richard Henderson]

On 2/20/24 06:06, Peter Maydell wrote:
> Add the two IDE bus BusState pointers to the set we keep in PCMachineState.
> This allows us to avoid passing them to pc_cmos_init(), and also will
> allow a refactoring of how we call pc_cmos_init_late().
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/hw/i386/pc.h |  4 +++-
>   hw/i386/pc.c         |  5 ++---
>   hw/i386/pc_piix.c    | 16 +++++++---------
>   hw/i386/pc_q35.c     |  9 ++++-----
>   4 files changed, 16 insertions(+), 18 deletions(-)

Acked-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 02/10] hw/i386/pc: Do pc_cmos_init_late() from pc_machine_done()

[Richard Henderson]

On 2/20/24 06:06, Peter Maydell wrote:
> -static void pc_cmos_init_late(void *opaque)
> +static void pc_cmos_init_late(PCMachineState *pcms)
>   {
> -    pc_cmos_init_late_arg *arg = opaque;
> -    MC146818RtcState *s = arg->rtc_state;
> +    X86MachineState *x86ms = X86_MACHINE(pcms);

We've already done the X86_MACHINE resolution in pc_machine_done -- why not just pass it in?

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 03/10] system/bootdevice: Don't unregister reset handler in restore_boot_order()

[Richard Henderson]

On 2/20/24 06:06, Peter Maydell wrote:
> Currently the qemu_register_reset() API permits the reset handler functions
> registered with it to remove themselves from within the callback function.
> This is fine with our current implementation, but is a bit odd, because
> generally reset is supposed to be idempotent, and doesn't fit well in a
> three-phase-reset world where a resettable object will get multiple
> callbacks as the system is reset.
> 
> We now have only one user of qemu_register_reset() which makes use of
> the ability to unregister itself within the callback:
> restore_boot_order().  We want to change our implementation of
> qemu_register_reset() to something where it would be awkward to
> maintain the "can self-unregister" feature.  Rather than making that
> reimplementation complicated, change restore_boot_order() so that it
> doesn't unregister itself but instead returns doing nothing for any
> calls after it has done the "restore the boot order" work.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> It would be nicer not to use reset at all, especially since I'm not
> a fan of conflating "system is reset" with "system boots", but I
> didn't have a good idea for how to do that.
> ---
>   system/bootdevice.c | 25 ++++++++++++++-----------
>   1 file changed, 14 insertions(+), 11 deletions(-)

Acked-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 04/10] include/qom/object.h: New OBJECT_DEFINE_SIMPLE_TYPE{, _WITH_INTERFACES} macros

[Richard Henderson]

On 2/20/24 06:06, Peter Maydell wrote:
> We have an OBJECT_DEFINE_TYPE_EXTENDED macro, plus several variations
> on it, which emits the boilerplate for the TypeInfo and ensures it is
> registered with the type system.  However, all the existing macros
> insist that the type being defined has its own FooClass struct, so
> they aren't useful for the common case of a simple leaf class which
> doesn't have any new methods or any other need for its own class
> struct (that is, for the kind of type that OBJECT_DECLARE_SIMPLE_TYPE
> declares).
> 
> Pull the actual implementation of OBJECT_DEFINE_TYPE_EXTENDED out
> into a new DO_OBJECT_DEFINE_TYPE_EXTENDED which parameterizes the
> value we use for the class_size field.  This lets us add a new
> OBJECT_DEFINE_SIMPLE_TYPE which does the same job as the various
> existing OBJECT_DEFINE_*_TYPE_* family macros for this kind of simple
> type, and the variant OBJECT_DEFINE_SIMPLE_TYPE_WITH_INTERFACES for
> when the type will implement some interfaces.
> 
> Reviewed-by: Daniel P. Berrangé<berrange@redhat.com>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   docs/devel/qom.rst   |  34 +++++++++++--
>   include/qom/object.h | 114 +++++++++++++++++++++++++++++++++----------
>   2 files changed, 117 insertions(+), 31 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 05/10] hw/core: Add documentation and license comments to reset.h

[Richard Henderson]

On 2/20/24 06:06, Peter Maydell wrote:
> Add the usual boilerplate license/copyright comment to reset.h (using
> the text from reset.c), and document the existing functions.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/sysemu/reset.h | 79 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 79 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 06/10] hw/core: Add ResetContainer which holds objects implementing Resettable

[Richard Henderson]

On 2/20/24 06:06, Peter Maydell wrote:
> Implement a ResetContainer.  This is a subclass of Object, and it
> implements the Resettable interface.  The container holds a list of
> arbitrary other objects which implement Resettable, and when the
> container is reset, all the objects it contains are also reset.
> 
> This will allow us to have a 3-phase-reset equivalent of the old
> qemu_register_reset() API: we will have a single "simulation reset"
> top level ResetContainer, and objects in it are the equivalent of the
> old QEMUResetHandler functions.
> 
> The qemu_register_reset() API manages its list of callbacks using a
> QTAILQ, but here we use a GPtrArray for our list of Resettable
> children: we expect the "remove" operation (which will need to do an
> iteration through the list) to be fairly uncommon, and we get simpler
> code with fewer memory allocations.
> 
> Since there is currently no listed owner in MAINTAINERS for the
> existing reset-related source files, create a new section for
> them, and add these new files there also.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   MAINTAINERS                      | 10 +++++
>   include/hw/core/resetcontainer.h | 48 ++++++++++++++++++++
>   hw/core/resetcontainer.c         | 76 ++++++++++++++++++++++++++++++++
>   hw/core/meson.build              |  1 +
>   4 files changed, 135 insertions(+)
>   create mode 100644 include/hw/core/resetcontainer.h
>   create mode 100644 hw/core/resetcontainer.c

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 06/10] hw/core: Add ResetContainer which holds objects implementing Resettable

[Richard Henderson]

On 2/20/24 06:06, Peter Maydell wrote:
> +void resettable_container_add(ResettableContainer *rc, Object *obj)
> +{
> +    g_ptr_array_add(rc->children, obj);
> +}

Did you want to assert here that obj does in fact implement Resettable?


r~

Re: [PATCH 07/10] hw/core/reset: Add qemu_{register, unregister}_resettable()

[Richard Henderson]

On 2/20/24 06:06, Peter Maydell wrote:
> Implement new functions qemu_register_resettable() and
> qemu_unregister_resettable().  These are intended to be
> three-phase-reset aware equivalents of the old qemu_register_reset()
> and qemu_unregister_reset().  Instead of passing in a function
> pointer and opaque, you register any QOM object that implements the
> Resettable interface.
> 
> The implementation is simple: we have a single global instance of a
> ResettableContainer, which we reset in qemu_devices_reset(), and
> the Resettable objects passed to qemu_register_resettable() are
> added to it.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/sysemu/reset.h | 37 ++++++++++++++++++++++++++++++++++---
>   hw/core/reset.c        | 31 +++++++++++++++++++++++++++++--
>   2 files changed, 63 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 08/10] hw/core/reset: Implement qemu_register_reset via qemu_register_resettable

[Richard Henderson]

On 2/20/24 06:06, Peter Maydell wrote:
> Reimplement qemu_register_reset() via qemu_register_resettable().
> 
> We define a new LegacyReset object which implements Resettable and
> defines its reset hold phase method to call a QEMUResetHandler
> function.  When qemu_register_reset() is called, we create a new
> LegacyReset object and add it to the simulation_reset
> ResettableContainer.  When qemu_unregister_reset() is called, we find
> the LegacyReset object in the container and remove it.
> 
> This implementation of qemu_unregister_reset() means we'll end up
> scanning the ResetContainer's list of child objects twice, once
> to find the LegacyReset object, and once in g_ptr_array_remove().
> In theory we could avoid this by having the ResettableContainer
> interface include a resettable_container_remove_with_equal_func()
> that took a callback method so that we could use
> g_ptr_array_find_with_equal_func() and g_ptr_array_remove_index().
> But we don't expect qemu_unregister_reset() to be called frequently
> or in hot paths, and we expect the simulation_reset container to
> usually not have many children.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> The way that a legacy reset function needs to check the ShutdownCause
> and this doesn't line up with the ResetType is a bit awkward; this
> is an area we should come back and clean up, but I didn't want to
> tackle that in this patchset.
> ---
>   include/sysemu/reset.h |   7 ++-
>   hw/core/reset.c        | 137 +++++++++++++++++++++++++++++++----------
>   2 files changed, 110 insertions(+), 34 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 09/10] hw/core/machine: Use qemu_register_resettable for sysbus reset

[Richard Henderson]

On 2/20/24 06:06, Peter Maydell wrote:
> Move the reset of the sysbus (and thus all devices and buses anywhere
> on the qbus tree) from qemu_register_reset() to qemu_register_resettable().
> 
> This is a behaviour change: because qemu_register_resettable() is
> aware of three-phase reset, this now means that:
>   * 'enter' phase reset methods of devices and buses are called
>     before any legacy reset callbacks registered with qemu_register_reset()
>   * 'exit' phase reset methods of devices and buses are called
>     after any legacy qemu_register_reset() callbacks
> 
> Put another way, a qemu_register_reset() callback is now correctly
> ordered in the 'hold' phase along with any other 'hold' phase methods.
> 
> The motivation for doing this is that we will now be able to resolve
> some reset-ordering issues using the three-phase mechanism, because
> the 'exit' phase is always after the 'hold' phase, even when the
> 'hold' phase function was registered with qemu_register_reset().
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> I believe that given we don't make much use of enter/exit phases
> currently that this is unlikely to cause unexpected regressions due
> to an accidental reset-order dependency that is no longer satisfied,
> but it's always possible...
> ---
>   hw/core/machine.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 10/10] docs/devel/reset: Update to discuss system reset

[Richard Henderson]

On 2/20/24 06:06, Peter Maydell wrote:
> Now that system reset uses a three-phase-reset, update the reset
> documentation to include a section describing how this works.
> Include documentation of the current major beartrap in reset, which
> is that only devices on the qbus tree will get automatically reset.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> This merely documents the current situation, and says nothing
> about what we might like to do with it in future...
> ---
>   docs/devel/reset.rst | 44 ++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 42 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 02/10] hw/i386/pc: Do pc_cmos_init_late() from pc_machine_done()

[Peter Maydell]

On Tue, 20 Feb 2024 at 19:31, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 2/20/24 06:06, Peter Maydell wrote:
> > -static void pc_cmos_init_late(void *opaque)
> > +static void pc_cmos_init_late(PCMachineState *pcms)
> >   {
> > -    pc_cmos_init_late_arg *arg = opaque;
> > -    MC146818RtcState *s = arg->rtc_state;
> > +    X86MachineState *x86ms = X86_MACHINE(pcms);
>
> We've already done the X86_MACHINE resolution in pc_machine_done -- why not just pass it in?

We want both the PCMachineState and X86MachineState and I think
our usual style is not to pass in two arguments that are the
same object under different pointer types.

-- PMM

Re: [PATCH 06/10] hw/core: Add ResetContainer which holds objects implementing Resettable

[Peter Maydell]

On Tue, 20 Feb 2024 at 19:46, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 2/20/24 06:06, Peter Maydell wrote:
> > +void resettable_container_add(ResettableContainer *rc, Object *obj)
> > +{
> > +    g_ptr_array_add(rc->children, obj);
> > +}
>
> Did you want to assert here that obj does in fact implement Resettable?

I guess that makes for a nicer detection of that class of bug,
so sure.

-- PMM

Re: [PATCH 00/10] reset: Make whole system three-phase-reset aware

[Michael S. Tsirkin]

On Tue, Feb 20, 2024 at 04:06:12PM +0000, Peter Maydell wrote:
> This patchset is an incremental improvement to our reset handling that
> tries to roll out the "three-phase-reset" design we have for devices
> to a wider scope.
> 
> At the moment devices and buses have a three-phase reset system, with
> separate 'enter', 'hold' and 'exit' phases. When the qbus tree is
> reset, first all the devices on it have their 'enter' method called,
> then they all have 'enter' called, and finally 'exit'. The idea is
> that we can use this, among other things, as a way to resolve annoying
> "this bit of reset work needs to happen before this other bit of reset
> work" ordering issues.
> 
> However, there is still a "legacy" reset option, where you register a
> callback function with qemu_register_reset(). These functions know
> nothing about the three-phase system, and "reset the qbus" is just one
> of the things set up to happen at reset via qemu_register_reset().
> That means that a qemu_register_reset() function might happen before
> all the enter/hold/exit phases of device reset, or it might happen after
> all of them.
> 
> This patchset provides a new way to register a three-phase-aware reset
> in this list of "reset the whole system" actions, and reimplements
> qemu_register_reset() in terms of that new mechanism. This means that
> qemu_register_reset() functions are now all called in the 'hold' phase
> of system reset. (This is an ordering change, so in theory it could
> introduce new bugs if we are accidentally depending on the current
> ordering; but we have very few enter and exit phase methods at the
> moment so I don't expect much trouble, if any.)
> 
> The first three patches remove the only two places in the codebase
> that rely on "a reset callback can unregister itself within the
> callback"; this is awkward to continue to support in the new
> implementation, and an unusual thing to do given that reset is in
> principle supposed to be something you can do as many times as you
> like, not something that behaves differently the first time through.
> 
> Patch 4 is an improvement to the QOM macros that's been on list as an
> RFC already.
> Patches 5 and 6 are the "new mechanism": qemu_register_resettable()
> takes any object that implements the Resettable interface. System
> reset is then doing 3-phase reset on all of these, so everything
> gets its 'enter' phase called, then everything gets 'hold', then
> everything gets 'exit'.
> 
> Patch 7 reimplements the qemu_register_reset() API to be
> "qemu_register_resettable(), and the callback function is called
> in the 'hold' phase".
> 
> Patch 8 makes the first real use of the new API: instead of
> doing the qbus reset via qemu_register_reset(), we pass the
> root of the qbus to qemu_register_resettable(). (This is where
> the ordering change I mention above happens, as device enter and
> exit method calls will now happen before and after all the
> qemu_register_reset() function callbacks, rather than in the
> middle of them.)
> 
> Finally, patch 9 updates the developer docs to describe how a
> complete system reset currently works.
> 
> This series doesn't actually do a great deal as it stands, but I
> think it's a necessary foundation for some cleanups:
>  * the vfio reset ordering problem discussed on list a while back
>    should now hopefully be solvable by having the vfio code use
>    qemu_register_resettable() so it can arrange to do the "needs to
>    happen last" stuff in an exit phase method
>  * there are some other missing pieces here, but eventually I think
>    it should be possible to get rid of the workarounds for
>    dependencies between ROM image loading and CPUs that want to
>    read an initial PC/SP on reset (eg arm, m68k)
> I also think it's a good idea to get it into the tree so that we
> have a chance to see if there are any unexpected regressions
> before we start putting in bugfixes etc that depend on it :-)
> 
> After this, I think the next thing I'm going to look at is whether
> we can move the MachineState class from inheriting from TYPE_OBJECT
> to TYPE_DEVICE. This would allow us to have machine-level reset
> (and would bring "machine as a container of devices" into line
> with "SoC object as container of devices").
> 
> thanks
> -- PMM


Like this overall and the pc cleanup is nice.

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>




> Peter Maydell (10):
>   hw/i386: Store pointers to IDE buses in PCMachineState
>   hw/i386/pc: Do pc_cmos_init_late() from pc_machine_done()
>   system/bootdevice: Don't unregister reset handler in
>     restore_boot_order()
>   include/qom/object.h: New OBJECT_DEFINE_SIMPLE_TYPE{,_WITH_INTERFACES}
>     macros
>   hw/core: Add documentation and license comments to reset.h
>   hw/core: Add ResetContainer which holds objects implementing
>     Resettable
>   hw/core/reset: Add qemu_{register,unregister}_resettable()
>   hw/core/reset: Implement qemu_register_reset via
>     qemu_register_resettable
>   hw/core/machine: Use qemu_register_resettable for sysbus reset
>   docs/devel/reset: Update to discuss system reset
> 
>  MAINTAINERS                      |  10 ++
>  docs/devel/qom.rst               |  34 ++++++-
>  docs/devel/reset.rst             |  44 +++++++-
>  include/hw/core/resetcontainer.h |  48 +++++++++
>  include/hw/i386/pc.h             |   4 +-
>  include/qom/object.h             | 114 ++++++++++++++++-----
>  include/sysemu/reset.h           | 113 +++++++++++++++++++++
>  hw/core/machine.c                |   7 +-
>  hw/core/reset.c                  | 166 +++++++++++++++++++++++++------
>  hw/core/resetcontainer.c         |  76 ++++++++++++++
>  hw/i386/pc.c                     |  40 +++-----
>  hw/i386/pc_piix.c                |  16 ++-
>  hw/i386/pc_q35.c                 |   9 +-
>  system/bootdevice.c              |  25 +++--
>  hw/core/meson.build              |   1 +
>  15 files changed, 587 insertions(+), 120 deletions(-)
>  create mode 100644 include/hw/core/resetcontainer.h
>  create mode 100644 hw/core/resetcontainer.c
> 
> -- 
> 2.34.1

Re: [PATCH 02/10] hw/i386/pc: Do pc_cmos_init_late() from pc_machine_done()

[Bernhard Beschow]

Am 20. Februar 2024 16:06:14 UTC schrieb Peter Maydell <peter.maydell@linaro.org>:
>In the i386 PC machine, we want to run the pc_cmos_init_late()
>function only once the IDE and floppy drive devices have been set up.
>We currently do this using qemu_register_reset(), and then have the
>function call qemu_unregister_reset() on itself, so it runs exactly
>once.
>
>This was an expedient way to do it back in 2010 when we first added
>this (in commit c0897e0cb94e8), but now we have a more obvious point
>to do "machine initialization that has to happen after generic device
>init": the machine-init-done hook.
>
>Do the pc_cmos_init_late() work from our existing PC machine init
>done hook function, so we can drop the use of qemu_register_reset()
>and qemu_unregister_reset().
>
>Because the pointers to the devices we need (the IDE buses and the
>RTC) are now all in the machine state, we don't need the
>pc_cmos_init_late_arg struct and can just pass the PCMachineState
>pointer.
>
>Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>---
> hw/i386/pc.c | 39 ++++++++++++++++-----------------------
> 1 file changed, 16 insertions(+), 23 deletions(-)
>
>diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>index 8b0f54e284c..4c3cfe9fc35 100644
>--- a/hw/i386/pc.c
>+++ b/hw/i386/pc.c
>@@ -465,11 +465,6 @@ static void pc_cmos_init_floppy(MC146818RtcState *rtc_state, ISADevice *floppy)
>     mc146818rtc_set_cmos_data(rtc_state, REG_EQUIPMENT_BYTE, val);
> }
> 
>-typedef struct pc_cmos_init_late_arg {
>-    MC146818RtcState *rtc_state;
>-    BusState *idebus[2];
>-} pc_cmos_init_late_arg;
>-
> typedef struct check_fdc_state {
>     ISADevice *floppy;
>     bool multiple;
>@@ -530,23 +525,25 @@ static ISADevice *pc_find_fdc0(void)
>     return state.floppy;
> }
> 
>-static void pc_cmos_init_late(void *opaque)
>+static void pc_cmos_init_late(PCMachineState *pcms)
> {
>-    pc_cmos_init_late_arg *arg = opaque;
>-    MC146818RtcState *s = arg->rtc_state;
>+    X86MachineState *x86ms = X86_MACHINE(pcms);
>+    MC146818RtcState *s = MC146818_RTC(x86ms->rtc);
>     int16_t cylinders;
>     int8_t heads, sectors;
>     int val;
>     int i, trans;
> 
>     val = 0;
>-    if (arg->idebus[0] && ide_get_geometry(arg->idebus[0], 0,
>-                                           &cylinders, &heads, &sectors) >= 0) {
>+    if (pcms->idebus[0] &&
>+        ide_get_geometry(pcms->idebus[0], 0,
>+                         &cylinders, &heads, &sectors) >= 0) {
>         cmos_init_hd(s, 0x19, 0x1b, cylinders, heads, sectors);
>         val |= 0xf0;
>     }
>-    if (arg->idebus[0] && ide_get_geometry(arg->idebus[0], 1,
>-                                           &cylinders, &heads, &sectors) >= 0) {
>+    if (pcms->idebus[0] &&
>+        ide_get_geometry(pcms->idebus[0], 1,
>+                         &cylinders, &heads, &sectors) >= 0) {
>         cmos_init_hd(s, 0x1a, 0x24, cylinders, heads, sectors);
>         val |= 0x0f;
>     }
>@@ -558,10 +555,11 @@ static void pc_cmos_init_late(void *opaque)
>            geometry.  It is always such that: 1 <= sects <= 63, 1
>            <= heads <= 16, 1 <= cylinders <= 16383. The BIOS
>            geometry can be different if a translation is done. */
>-        if (arg->idebus[i / 2] &&
>-            ide_get_geometry(arg->idebus[i / 2], i % 2,
>+        BusState *idebus = pcms->idebus[i / 2];
>+        if (idebus &&
>+            ide_get_geometry(idebus, i % 2,
>                              &cylinders, &heads, &sectors) >= 0) {
>-            trans = ide_get_bios_chs_trans(arg->idebus[i / 2], i % 2) - 1;
>+            trans = ide_get_bios_chs_trans(idebus, i % 2) - 1;
>             assert((trans & ~3) == 0);
>             val |= trans << (i * 2);
>         }
>@@ -569,15 +567,12 @@ static void pc_cmos_init_late(void *opaque)
>     mc146818rtc_set_cmos_data(s, 0x39, val);
> 
>     pc_cmos_init_floppy(s, pc_find_fdc0());
>-
>-    qemu_unregister_reset(pc_cmos_init_late, opaque);
> }
> 
> void pc_cmos_init(PCMachineState *pcms,
>                   ISADevice *rtc)
> {
>     int val;
>-    static pc_cmos_init_late_arg arg;
>     X86MachineState *x86ms = X86_MACHINE(pcms);
>     MC146818RtcState *s = MC146818_RTC(rtc);
> 
>@@ -631,11 +626,7 @@ void pc_cmos_init(PCMachineState *pcms,
>     val |= 0x04; /* PS/2 mouse installed */
>     mc146818rtc_set_cmos_data(s, REG_EQUIPMENT_BYTE, val);
> 
>-    /* hard drives and FDC */
>-    arg.rtc_state = s;
>-    arg.idebus[0] = pcms->idebus[0];
>-    arg.idebus[1] = pcms->idebus[1];
>-    qemu_register_reset(pc_cmos_init_late, &arg);
>+    /* hard drives and FDC are handled by pc_cmos_init_late() */
> }
> 
> static void handle_a20_line_change(void *opaque, int irq, int level)
>@@ -703,6 +694,8 @@ void pc_machine_done(Notifier *notifier, void *data)
>         /* update FW_CFG_NB_CPUS to account for -device added CPUs */
>         fw_cfg_modify_i16(x86ms->fw_cfg, FW_CFG_NB_CPUS, x86ms->boot_cpus);
>     }
>+
>+    pc_cmos_init_late(pcms);

Nice. With https://patchew.org/QEMU/20240208220349.4948-1-shentey@gmail.com/20240208220349.4948-9-shentey@gmail.com/ on top it might be possible to merge pc_cmos_init_late() and pc_cmos_init(), thus freeing pc_piix and pc_q35 entirely from having to deal with it.

Best regards,
Bernhard

> }
> 
> void pc_guest_info_init(PCMachineState *pcms)

Re: [PATCH 00/10] reset: Make whole system three-phase-reset aware

[Mark Cave-Ayland]

On 20/02/2024 16:06, Peter Maydell wrote:

> This patchset is an incremental improvement to our reset handling that
> tries to roll out the "three-phase-reset" design we have for devices
> to a wider scope.
> 
> At the moment devices and buses have a three-phase reset system, with
> separate 'enter', 'hold' and 'exit' phases. When the qbus tree is
> reset, first all the devices on it have their 'enter' method called,
> then they all have 'enter' called, and finally 'exit'. The idea is
> that we can use this, among other things, as a way to resolve annoying
> "this bit of reset work needs to happen before this other bit of reset
> work" ordering issues.
> 
> However, there is still a "legacy" reset option, where you register a
> callback function with qemu_register_reset(). These functions know
> nothing about the three-phase system, and "reset the qbus" is just one
> of the things set up to happen at reset via qemu_register_reset().
> That means that a qemu_register_reset() function might happen before
> all the enter/hold/exit phases of device reset, or it might happen after
> all of them.
> 
> This patchset provides a new way to register a three-phase-aware reset
> in this list of "reset the whole system" actions, and reimplements
> qemu_register_reset() in terms of that new mechanism. This means that
> qemu_register_reset() functions are now all called in the 'hold' phase
> of system reset. (This is an ordering change, so in theory it could
> introduce new bugs if we are accidentally depending on the current
> ordering; but we have very few enter and exit phase methods at the
> moment so I don't expect much trouble, if any.)
> 
> The first three patches remove the only two places in the codebase
> that rely on "a reset callback can unregister itself within the
> callback"; this is awkward to continue to support in the new
> implementation, and an unusual thing to do given that reset is in
> principle supposed to be something you can do as many times as you
> like, not something that behaves differently the first time through.
> 
> Patch 4 is an improvement to the QOM macros that's been on list as an
> RFC already.
> Patches 5 and 6 are the "new mechanism": qemu_register_resettable()
> takes any object that implements the Resettable interface. System
> reset is then doing 3-phase reset on all of these, so everything
> gets its 'enter' phase called, then everything gets 'hold', then
> everything gets 'exit'.
> 
> Patch 7 reimplements the qemu_register_reset() API to be
> "qemu_register_resettable(), and the callback function is called
> in the 'hold' phase".
> 
> Patch 8 makes the first real use of the new API: instead of
> doing the qbus reset via qemu_register_reset(), we pass the
> root of the qbus to qemu_register_resettable(). (This is where
> the ordering change I mention above happens, as device enter and
> exit method calls will now happen before and after all the
> qemu_register_reset() function callbacks, rather than in the
> middle of them.)
> 
> Finally, patch 9 updates the developer docs to describe how a
> complete system reset currently works.
> 
> This series doesn't actually do a great deal as it stands, but I
> think it's a necessary foundation for some cleanups:
>   * the vfio reset ordering problem discussed on list a while back
>     should now hopefully be solvable by having the vfio code use
>     qemu_register_resettable() so it can arrange to do the "needs to
>     happen last" stuff in an exit phase method
>   * there are some other missing pieces here, but eventually I think
>     it should be possible to get rid of the workarounds for
>     dependencies between ROM image loading and CPUs that want to
>     read an initial PC/SP on reset (eg arm, m68k)

Absolutely, this would definitely help with m68k :)

> I also think it's a good idea to get it into the tree so that we
> have a chance to see if there are any unexpected regressions
> before we start putting in bugfixes etc that depend on it :-)
> 
> After this, I think the next thing I'm going to look at is whether
> we can move the MachineState class from inheriting from TYPE_OBJECT
> to TYPE_DEVICE. This would allow us to have machine-level reset
> (and would bring "machine as a container of devices" into line
> with "SoC object as container of devices").

This would be really useful! In particular, moving MachineState to inherit from 
TYPE_DEVICE will allow setting MemoryRegion owners to the machine itself for 
arbitrary registers implemented by the board.

> thanks
> -- PMM
> 
> Peter Maydell (10):
>    hw/i386: Store pointers to IDE buses in PCMachineState
>    hw/i386/pc: Do pc_cmos_init_late() from pc_machine_done()
>    system/bootdevice: Don't unregister reset handler in
>      restore_boot_order()
>    include/qom/object.h: New OBJECT_DEFINE_SIMPLE_TYPE{,_WITH_INTERFACES}
>      macros
>    hw/core: Add documentation and license comments to reset.h
>    hw/core: Add ResetContainer which holds objects implementing
>      Resettable
>    hw/core/reset: Add qemu_{register,unregister}_resettable()
>    hw/core/reset: Implement qemu_register_reset via
>      qemu_register_resettable
>    hw/core/machine: Use qemu_register_resettable for sysbus reset
>    docs/devel/reset: Update to discuss system reset
> 
>   MAINTAINERS                      |  10 ++
>   docs/devel/qom.rst               |  34 ++++++-
>   docs/devel/reset.rst             |  44 +++++++-
>   include/hw/core/resetcontainer.h |  48 +++++++++
>   include/hw/i386/pc.h             |   4 +-
>   include/qom/object.h             | 114 ++++++++++++++++-----
>   include/sysemu/reset.h           | 113 +++++++++++++++++++++
>   hw/core/machine.c                |   7 +-
>   hw/core/reset.c                  | 166 +++++++++++++++++++++++++------
>   hw/core/resetcontainer.c         |  76 ++++++++++++++
>   hw/i386/pc.c                     |  40 +++-----
>   hw/i386/pc_piix.c                |  16 ++-
>   hw/i386/pc_q35.c                 |   9 +-
>   system/bootdevice.c              |  25 +++--
>   hw/core/meson.build              |   1 +
>   15 files changed, 587 insertions(+), 120 deletions(-)
>   create mode 100644 include/hw/core/resetcontainer.h
>   create mode 100644 hw/core/resetcontainer.c


ATB,

Mark.

Re: [PATCH 01/10] hw/i386: Store pointers to IDE buses in PCMachineState

[Philippe Mathieu-Daudé]

Hi Peter,

On 20/2/24 17:06, Peter Maydell wrote:
> Add the two IDE bus BusState pointers to the set we keep in PCMachineState.
> This allows us to avoid passing them to pc_cmos_init(), and also will
> allow a refactoring of how we call pc_cmos_init_late().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/i386/pc.h |  4 +++-
>   hw/i386/pc.c         |  5 ++---
>   hw/i386/pc_piix.c    | 16 +++++++---------
>   hw/i386/pc_q35.c     |  9 ++++-----
>   4 files changed, 16 insertions(+), 18 deletions(-)


> @@ -300,13 +299,13 @@ static void pc_q35_init(MachineState *machine)
>                                                            ICH9_SATA1_FUNC),
>                                                  "ich9-ahci");
>           ich9 = ICH9_AHCI(pdev);
> -        idebus[0] = qdev_get_child_bus(DEVICE(pdev), "ide.0");
> -        idebus[1] = qdev_get_child_bus(DEVICE(pdev), "ide.1");
> +        pcms->idebus[0] = qdev_get_child_bus(DEVICE(pdev), "ide.0");
> +        pcms->idebus[1] = qdev_get_child_bus(DEVICE(pdev), "ide.1");
>           g_assert(MAX_SATA_PORTS == ich9->ahci.ports);
>           ide_drive_get(hd, ich9->ahci.ports);
>           ahci_ide_create_devs(&ich9->ahci, hd);
>       } else {
> -        idebus[0] = idebus[1] = NULL;
> +        pcms->idebus[0] = pcms->idebus[1] = NULL;

Since PCMachineState is zero-initialized, this part is now
pointless.

Since my ICH9 series clashes with your patch, I'm inclined to
queue it in my hw-misc tree, with the following squashed:

-- >8 --

-    } else {
-        pcms->idebus[0] = pcms->idebus[1] = NULL;

---

>       }
>   
>       if (machine_usb(machine)) {
> @@ -327,7 +326,7 @@ static void pc_q35_init(MachineState *machine)
>           smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
>       }
>   
> -    pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
> +    pc_cmos_init(pcms, rtc_state);
>   
>       /* the rest devices to which pci devfn is automatically assigned */
>       pc_vga_init(isa_bus, host_bus);

Re: [PATCH 01/10] hw/i386: Store pointers to IDE buses in PCMachineState

[Philippe Mathieu-Daudé]

On 21/2/24 14:07, Philippe Mathieu-Daudé wrote:
> Hi Peter,
> 
> On 20/2/24 17:06, Peter Maydell wrote:
>> Add the two IDE bus BusState pointers to the set we keep in 
>> PCMachineState.
>> This allows us to avoid passing them to pc_cmos_init(), and also will
>> allow a refactoring of how we call pc_cmos_init_late().
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>   include/hw/i386/pc.h |  4 +++-
>>   hw/i386/pc.c         |  5 ++---
>>   hw/i386/pc_piix.c    | 16 +++++++---------
>>   hw/i386/pc_q35.c     |  9 ++++-----
>>   4 files changed, 16 insertions(+), 18 deletions(-)
> 
> 
>> @@ -300,13 +299,13 @@ static void pc_q35_init(MachineState *machine)
>>                                                            
>> ICH9_SATA1_FUNC),
>>                                                  "ich9-ahci");
>>           ich9 = ICH9_AHCI(pdev);
>> -        idebus[0] = qdev_get_child_bus(DEVICE(pdev), "ide.0");
>> -        idebus[1] = qdev_get_child_bus(DEVICE(pdev), "ide.1");
>> +        pcms->idebus[0] = qdev_get_child_bus(DEVICE(pdev), "ide.0");
>> +        pcms->idebus[1] = qdev_get_child_bus(DEVICE(pdev), "ide.1");
>>           g_assert(MAX_SATA_PORTS == ich9->ahci.ports);
>>           ide_drive_get(hd, ich9->ahci.ports);
>>           ahci_ide_create_devs(&ich9->ahci, hd);
>>       } else {
>> -        idebus[0] = idebus[1] = NULL;
>> +        pcms->idebus[0] = pcms->idebus[1] = NULL;
> 
> Since PCMachineState is zero-initialized, this part is now
> pointless.
> 
> Since my ICH9 series clashes with your patch, I'm inclined to
> queue it in my hw-misc tree, with the following squashed:
> 
> -- >8 --
> 
> -    } else {
> -        pcms->idebus[0] = pcms->idebus[1] = NULL;
> 
> ---

Sorry sent too fast. Squashing

-- >8 --
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 8df88a6ccd..77d1b03fdf 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -311,6 +311,4 @@ static void pc_init1(MachineState *machine,
          i8257_dma_init(OBJECT(machine), isa_bus, 0);
          pcms->hpet_enabled = false;
-        pcms->idebus[0] = NULL;
-        pcms->idebus[1] = NULL;
      }

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 71402c36eb..0e9bd27a6e 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -305,6 +305,4 @@ static void pc_q35_init(MachineState *machine)
          ide_drive_get(hd, ich9->ahci.ports);
          ahci_ide_create_devs(&ich9->ahci, hd);
-    } else {
-        pcms->idebus[0] = pcms->idebus[1] = NULL;
      }

---

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 02/10] hw/i386/pc: Do pc_cmos_init_late() from pc_machine_done()

[Philippe Mathieu-Daudé]

On 20/2/24 17:06, Peter Maydell wrote:
> In the i386 PC machine, we want to run the pc_cmos_init_late()
> function only once the IDE and floppy drive devices have been set up.
> We currently do this using qemu_register_reset(), and then have the
> function call qemu_unregister_reset() on itself, so it runs exactly
> once.
> 
> This was an expedient way to do it back in 2010 when we first added
> this (in commit c0897e0cb94e8), but now we have a more obvious point
> to do "machine initialization that has to happen after generic device
> init": the machine-init-done hook.
> 
> Do the pc_cmos_init_late() work from our existing PC machine init
> done hook function, so we can drop the use of qemu_register_reset()
> and qemu_unregister_reset().
> 
> Because the pointers to the devices we need (the IDE buses and the
> RTC) are now all in the machine state, we don't need the
> pc_cmos_init_late_arg struct and can just pass the PCMachineState
> pointer.

Even if we remove the IDEBus/RTC pointers from PCMachineState,
we can still QOM-resolve them from it in pc_cmos_init_late().

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/i386/pc.c | 39 ++++++++++++++++-----------------------
>   1 file changed, 16 insertions(+), 23 deletions(-)

Re: [PATCH 06/10] hw/core: Add ResetContainer which holds objects implementing Resettable

[Philippe Mathieu-Daudé]

On 20/2/24 17:06, Peter Maydell wrote:
> Implement a ResetContainer.  This is a subclass of Object, and it
> implements the Resettable interface.  The container holds a list of
> arbitrary other objects which implement Resettable, and when the
> container is reset, all the objects it contains are also reset.
> 
> This will allow us to have a 3-phase-reset equivalent of the old
> qemu_register_reset() API: we will have a single "simulation reset"
> top level ResetContainer, and objects in it are the equivalent of the
> old QEMUResetHandler functions.
> 
> The qemu_register_reset() API manages its list of callbacks using a
> QTAILQ, but here we use a GPtrArray for our list of Resettable
> children: we expect the "remove" operation (which will need to do an
> iteration through the list) to be fairly uncommon, and we get simpler
> code with fewer memory allocations.
> 
> Since there is currently no listed owner in MAINTAINERS for the
> existing reset-related source files, create a new section for
> them, and add these new files there also.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   MAINTAINERS                      | 10 +++++
>   include/hw/core/resetcontainer.h | 48 ++++++++++++++++++++
>   hw/core/resetcontainer.c         | 76 ++++++++++++++++++++++++++++++++
>   hw/core/meson.build              |  1 +
>   4 files changed, 135 insertions(+)
>   create mode 100644 include/hw/core/resetcontainer.h
>   create mode 100644 hw/core/resetcontainer.c


> +static void resettable_container_child_foreach(Object *obj,
> +                                               ResettableChildCallback cb,
> +                                               void *opaque, ResetType type)
> +{
> +    ResettableContainer *rc = RESETTABLE_CONTAINER(obj);
> +    unsigned int len = rc->children->len;
> +
> +    for (unsigned int i = 0; i < len; i++) {

Worth a pair of trace events around the callback call.

> +        cb(g_ptr_array_index(rc->children, i), opaque, type);

> +        /* Detect callbacks trying to unregister themselves */
> +        assert(len == rc->children->len);
> +    }
> +}

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 09/10] hw/core/machine: Use qemu_register_resettable for sysbus reset

[Philippe Mathieu-Daudé]

On 20/2/24 17:06, Peter Maydell wrote:
> Move the reset of the sysbus (and thus all devices and buses anywhere
> on the qbus tree) from qemu_register_reset() to qemu_register_resettable().
> 
> This is a behaviour change: because qemu_register_resettable() is
> aware of three-phase reset, this now means that:
>   * 'enter' phase reset methods of devices and buses are called
>     before any legacy reset callbacks registered with qemu_register_reset()
>   * 'exit' phase reset methods of devices and buses are called
>     after any legacy qemu_register_reset() callbacks
> 
> Put another way, a qemu_register_reset() callback is now correctly
> ordered in the 'hold' phase along with any other 'hold' phase methods.
> 
> The motivation for doing this is that we will now be able to resolve
> some reset-ordering issues using the three-phase mechanism, because
> the 'exit' phase is always after the 'hold' phase, even when the
> 'hold' phase function was registered with qemu_register_reset().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I believe that given we don't make much use of enter/exit phases
> currently that this is unlikely to cause unexpected regressions due
> to an accidental reset-order dependency that is no longer satisfied,
> but it's always possible...
> ---
>   hw/core/machine.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index fb5afdcae4c..9ac5d5389a6 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1577,14 +1577,13 @@ void qdev_machine_creation_done(void)
>       /* TODO: once all bus devices are qdevified, this should be done
>        * when bus is created by qdev.c */
>       /*
> -     * TODO: If we had a main 'reset container' that the whole system
> -     * lived in, we could reset that using the multi-phase reset
> -     * APIs. For the moment, we just reset the sysbus, which will cause
> +     * This is where we arrange for the sysbus to be reset when the
> +     * whole simulation is reset. In turn, resetting the sysbus will cause
>        * all devices hanging off it (and all their child buses, recursively)
>        * to be reset. Note that this will *not* reset any Device objects
>        * which are not attached to some part of the qbus tree!
>        */
> -    qemu_register_reset(resettable_cold_reset_fn, sysbus_get_default());
> +    qemu_register_resettable(OBJECT(sysbus_get_default()));

Correct, so:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

But I'd rather move that call to main_system_bus_create() in
hw/core/sysbus.c, as it doesn't seem to be related to the
machine creation phases anymore. Maybe clearer to do in a
separate patch although.

>       notifier_list_notify(&machine_init_done_notifiers, NULL);
>   

Re: [PATCH 06/10] hw/core: Add ResetContainer which holds objects implementing Resettable

[Peter Maydell]

On Wed, 21 Feb 2024 at 15:34, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 20/2/24 17:06, Peter Maydell wrote:
> > Implement a ResetContainer.  This is a subclass of Object, and it
> > implements the Resettable interface.  The container holds a list of
> > arbitrary other objects which implement Resettable, and when the
> > container is reset, all the objects it contains are also reset.
> >
> > This will allow us to have a 3-phase-reset equivalent of the old
> > qemu_register_reset() API: we will have a single "simulation reset"
> > top level ResetContainer, and objects in it are the equivalent of the
> > old QEMUResetHandler functions.
> >
> > The qemu_register_reset() API manages its list of callbacks using a
> > QTAILQ, but here we use a GPtrArray for our list of Resettable
> > children: we expect the "remove" operation (which will need to do an
> > iteration through the list) to be fairly uncommon, and we get simpler
> > code with fewer memory allocations.
> >
> > Since there is currently no listed owner in MAINTAINERS for the
> > existing reset-related source files, create a new section for
> > them, and add these new files there also.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   MAINTAINERS                      | 10 +++++
> >   include/hw/core/resetcontainer.h | 48 ++++++++++++++++++++
> >   hw/core/resetcontainer.c         | 76 ++++++++++++++++++++++++++++++++
> >   hw/core/meson.build              |  1 +
> >   4 files changed, 135 insertions(+)
> >   create mode 100644 include/hw/core/resetcontainer.h
> >   create mode 100644 hw/core/resetcontainer.c
>
>
> > +static void resettable_container_child_foreach(Object *obj,
> > +                                               ResettableChildCallback cb,
> > +                                               void *opaque, ResetType type)
> > +{
> > +    ResettableContainer *rc = RESETTABLE_CONTAINER(obj);
> > +    unsigned int len = rc->children->len;
> > +
> > +    for (unsigned int i = 0; i < len; i++) {
>
> Worth a pair of trace events around the callback call.

Do you think so? What would be the interest in them?
(The way the resettable handling works this foreach loop
gets called several times for any particular reset event,
as well as getting called if anybody calls qemu_unregister_reset():
so "something is iterating the resettable container children"
can be for multiple reasons.)

-- PMM

Re: [PATCH 06/10] hw/core: Add ResetContainer which holds objects implementing Resettable

[Philippe Mathieu-Daudé]

On 21/2/24 17:09, Peter Maydell wrote:
> On Wed, 21 Feb 2024 at 15:34, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 20/2/24 17:06, Peter Maydell wrote:
>>> Implement a ResetContainer.  This is a subclass of Object, and it
>>> implements the Resettable interface.  The container holds a list of
>>> arbitrary other objects which implement Resettable, and when the
>>> container is reset, all the objects it contains are also reset.
>>>
>>> This will allow us to have a 3-phase-reset equivalent of the old
>>> qemu_register_reset() API: we will have a single "simulation reset"
>>> top level ResetContainer, and objects in it are the equivalent of the
>>> old QEMUResetHandler functions.
>>>
>>> The qemu_register_reset() API manages its list of callbacks using a
>>> QTAILQ, but here we use a GPtrArray for our list of Resettable
>>> children: we expect the "remove" operation (which will need to do an
>>> iteration through the list) to be fairly uncommon, and we get simpler
>>> code with fewer memory allocations.
>>>
>>> Since there is currently no listed owner in MAINTAINERS for the
>>> existing reset-related source files, create a new section for
>>> them, and add these new files there also.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>>    MAINTAINERS                      | 10 +++++
>>>    include/hw/core/resetcontainer.h | 48 ++++++++++++++++++++
>>>    hw/core/resetcontainer.c         | 76 ++++++++++++++++++++++++++++++++
>>>    hw/core/meson.build              |  1 +
>>>    4 files changed, 135 insertions(+)
>>>    create mode 100644 include/hw/core/resetcontainer.h
>>>    create mode 100644 hw/core/resetcontainer.c
>>
>>
>>> +static void resettable_container_child_foreach(Object *obj,
>>> +                                               ResettableChildCallback cb,
>>> +                                               void *opaque, ResetType type)
>>> +{
>>> +    ResettableContainer *rc = RESETTABLE_CONTAINER(obj);
>>> +    unsigned int len = rc->children->len;
>>> +
>>> +    for (unsigned int i = 0; i < len; i++) {
>>
>> Worth a pair of trace events around the callback call.
> 
> Do you think so? What would be the interest in them?
> (The way the resettable handling works this foreach loop
> gets called several times for any particular reset event,
> as well as getting called if anybody calls qemu_unregister_reset():
> so "something is iterating the resettable container children"
> can be for multiple reasons.)

I remember Damien added a bunch resettable* events and I've been
using them to test his series, but also later while refactoring
some devices or QDevifying others, in particular when devices
contain buses.

$ git grep trace_ hw/core/reset*
hw/core/resettable.c:44:    trace_resettable_reset(obj, type);
hw/core/resettable.c:53:    trace_resettable_reset_assert_begin(obj, type);
hw/core/resettable.c:62:    trace_resettable_reset_assert_end(obj);
hw/core/resettable.c:69:    trace_resettable_reset_release_begin(obj, type);
hw/core/resettable.c:76:    trace_resettable_reset_release_end(obj);
hw/core/resettable.c:124:    trace_resettable_phase_enter_begin(obj, 
obj_typename, s->count, type);
hw/core/resettable.c:151:        trace_resettable_phase_enter_exec(obj, 
obj_typename, type,
hw/core/resettable.c:158:    trace_resettable_phase_enter_end(obj, 
obj_typename, s->count);
hw/core/resettable.c:170:    trace_resettable_phase_hold_begin(obj, 
obj_typename, s->count, type);
hw/core/resettable.c:179:        trace_resettable_phase_hold_exec(obj, 
obj_typename, !!rc->phases.hold);
hw/core/resettable.c:181: 
trace_resettable_transitional_function(obj, obj_typename);
hw/core/resettable.c:187:    trace_resettable_phase_hold_end(obj, 
obj_typename, s->count);
hw/core/resettable.c:197:    trace_resettable_phase_exit_begin(obj, 
obj_typename, s->count, type);
hw/core/resettable.c:205:        trace_resettable_phase_exit_exec(obj, 
obj_typename, !!rc->phases.exit);
hw/core/resettable.c:211:    trace_resettable_phase_exit_end(obj, 
obj_typename, s->count);
hw/core/resettable.c:243:    trace_resettable_change_parent(obj, oldp, 
oldp_count, newp, newp_count);

Anyway, can be added later if useful for debugging. Certainly not a
blocker :)

Re: [PATCH 01/10] hw/i386: Store pointers to IDE buses in PCMachineState

[Zhao Liu]

On Tue, Feb 20, 2024 at 04:06:13PM +0000, Peter Maydell wrote:
> Date: Tue, 20 Feb 2024 16:06:13 +0000
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: [PATCH 01/10] hw/i386: Store pointers to IDE buses in
>  PCMachineState
> X-Mailer: git-send-email 2.34.1
> 
> Add the two IDE bus BusState pointers to the set we keep in PCMachineState.
> This allows us to avoid passing them to pc_cmos_init(), and also will
> allow a refactoring of how we call pc_cmos_init_late().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/i386/pc.h |  4 +++-
>  hw/i386/pc.c         |  5 ++---
>  hw/i386/pc_piix.c    | 16 +++++++---------
>  hw/i386/pc_q35.c     |  9 ++++-----
>  4 files changed, 16 insertions(+), 18 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index ec0e5efcb28..8f8ac894b10 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -17,6 +17,8 @@
>  
>  #define HPET_INTCAP "hpet-intcap"
>  
> +#define MAX_IDE_BUS 2
> +
>  /**
>   * PCMachineState:
>   * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling
> @@ -37,6 +39,7 @@ typedef struct PCMachineState {
>      PFlashCFI01 *flash[2];
>      ISADevice *pcspk;
>      DeviceState *iommu;
> +    BusState *idebus[MAX_IDE_BUS];
>  
>      /* Configuration options: */
>      uint64_t max_ram_below_4g;
> @@ -182,7 +185,6 @@ void pc_basic_device_init(struct PCMachineState *pcms,
>                            bool create_fdctrl,
>                            uint32_t hpet_irqs);
>  void pc_cmos_init(PCMachineState *pcms,
> -                  BusState *ide0, BusState *ide1,
>                    ISADevice *s);
>  void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus);
>  
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 196827531a5..8b0f54e284c 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -574,7 +574,6 @@ static void pc_cmos_init_late(void *opaque)
>  }
>  
>  void pc_cmos_init(PCMachineState *pcms,
> -                  BusState *idebus0, BusState *idebus1,
>                    ISADevice *rtc)
>  {
>      int val;
> @@ -634,8 +633,8 @@ void pc_cmos_init(PCMachineState *pcms,
>  
>      /* hard drives and FDC */
>      arg.rtc_state = s;
> -    arg.idebus[0] = idebus0;
> -    arg.idebus[1] = idebus1;
> +    arg.idebus[0] = pcms->idebus[0];
> +    arg.idebus[1] = pcms->idebus[1];
>      qemu_register_reset(pc_cmos_init_late, &arg);
>  }
>  
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 999b7b806ca..8df88a6ccd1 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -68,7 +68,6 @@
>  #include "kvm/kvm-cpu.h"
>  #include "target/i386/cpu.h"
>  
> -#define MAX_IDE_BUS 2
>  #define XEN_IOAPIC_NUM_PIRQS 128ULL
>  
>  #ifdef CONFIG_IDE_ISA
> @@ -114,7 +113,6 @@ static void pc_init1(MachineState *machine,
>      Object *piix4_pm = NULL;
>      qemu_irq smi_irq;
>      GSIState *gsi_state;
> -    BusState *idebus[MAX_IDE_BUS];
>      ISADevice *rtc_state;
>      MemoryRegion *ram_memory;
>      MemoryRegion *pci_memory = NULL;
> @@ -299,8 +297,8 @@ static void pc_init1(MachineState *machine,
>          piix4_pm = object_resolve_path_component(OBJECT(pci_dev), "pm");
>          dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "ide"));
>          pci_ide_create_devs(PCI_DEVICE(dev));
> -        idebus[0] = qdev_get_child_bus(dev, "ide.0");
> -        idebus[1] = qdev_get_child_bus(dev, "ide.1");
> +        pcms->idebus[0] = qdev_get_child_bus(dev, "ide.0");
> +        pcms->idebus[1] = qdev_get_child_bus(dev, "ide.1");
>      } else {
>          isa_bus = isa_bus_new(NULL, system_memory, system_io,
>                                &error_abort);
> @@ -312,8 +310,8 @@ static void pc_init1(MachineState *machine,
>  
>          i8257_dma_init(OBJECT(machine), isa_bus, 0);
>          pcms->hpet_enabled = false;
> -        idebus[0] = NULL;
> -        idebus[1] = NULL;
> +        pcms->idebus[0] = NULL;
> +        pcms->idebus[1] = NULL;
>      }
>  
>      if (x86ms->pic == ON_OFF_AUTO_ON || x86ms->pic == ON_OFF_AUTO_AUTO) {
> @@ -342,7 +340,7 @@ static void pc_init1(MachineState *machine,
>      pc_nic_init(pcmc, isa_bus, pci_bus);
>  
>      if (pcmc->pci_enabled) {
> -        pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
> +        pc_cmos_init(pcms, rtc_state);
>      }
>  #ifdef CONFIG_IDE_ISA
>      else {
> @@ -361,9 +359,9 @@ static void pc_init1(MachineState *machine,
>               * second one.
>               */
>              busname[4] = '0' + i;
> -            idebus[i] = qdev_get_child_bus(DEVICE(dev), busname);
> +            pcms->idebus[i] = qdev_get_child_bus(DEVICE(dev), busname);
>          }
> -        pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
> +        pc_cmos_init(pcms, rtc_state);
>      }
>  #endif
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index d346fa3b1d6..71402c36eb2 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -126,7 +126,6 @@ static void pc_q35_init(MachineState *machine)
>      PCIBus *host_bus;
>      PCIDevice *lpc;
>      DeviceState *lpc_dev;
> -    BusState *idebus[MAX_SATA_PORTS];
>      ISADevice *rtc_state;
>      MemoryRegion *system_memory = get_system_memory();
>      MemoryRegion *system_io = get_system_io();
> @@ -300,13 +299,13 @@ static void pc_q35_init(MachineState *machine)
>                                                           ICH9_SATA1_FUNC),
>                                                 "ich9-ahci");
>          ich9 = ICH9_AHCI(pdev);
> -        idebus[0] = qdev_get_child_bus(DEVICE(pdev), "ide.0");
> -        idebus[1] = qdev_get_child_bus(DEVICE(pdev), "ide.1");
> +        pcms->idebus[0] = qdev_get_child_bus(DEVICE(pdev), "ide.0");
> +        pcms->idebus[1] = qdev_get_child_bus(DEVICE(pdev), "ide.1");
>          g_assert(MAX_SATA_PORTS == ich9->ahci.ports);
>          ide_drive_get(hd, ich9->ahci.ports);
>          ahci_ide_create_devs(&ich9->ahci, hd);
>      } else {
> -        idebus[0] = idebus[1] = NULL;
> +        pcms->idebus[0] = pcms->idebus[1] = NULL;
>      }
>  
>      if (machine_usb(machine)) {
> @@ -327,7 +326,7 @@ static void pc_q35_init(MachineState *machine)
>          smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
>      }
>  
> -    pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
> +    pc_cmos_init(pcms, rtc_state);
>  
>      /* the rest devices to which pci devfn is automatically assigned */
>      pc_vga_init(isa_bus, host_bus);
> -- 
> 2.34.1
> 
> 

Re: [PATCH 02/10] hw/i386/pc: Do pc_cmos_init_late() from pc_machine_done()

[Zhao Liu]

On Tue, Feb 20, 2024 at 04:06:14PM +0000, Peter Maydell wrote:
> Date: Tue, 20 Feb 2024 16:06:14 +0000
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: [PATCH 02/10] hw/i386/pc: Do pc_cmos_init_late() from
>  pc_machine_done()
> X-Mailer: git-send-email 2.34.1
> 
> In the i386 PC machine, we want to run the pc_cmos_init_late()
> function only once the IDE and floppy drive devices have been set up.
> We currently do this using qemu_register_reset(), and then have the
> function call qemu_unregister_reset() on itself, so it runs exactly
> once.
> 
> This was an expedient way to do it back in 2010 when we first added
> this (in commit c0897e0cb94e8), but now we have a more obvious point
> to do "machine initialization that has to happen after generic device
> init": the machine-init-done hook.
> 
> Do the pc_cmos_init_late() work from our existing PC machine init
> done hook function, so we can drop the use of qemu_register_reset()
> and qemu_unregister_reset().
> 
> Because the pointers to the devices we need (the IDE buses and the
> RTC) are now all in the machine state, we don't need the
> pc_cmos_init_late_arg struct and can just pass the PCMachineState
> pointer.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/i386/pc.c | 39 ++++++++++++++++-----------------------
>  1 file changed, 16 insertions(+), 23 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 03/10] system/bootdevice: Don't unregister reset handler in restore_boot_order()

[Zhao Liu]

On Tue, Feb 20, 2024 at 04:06:15PM +0000, Peter Maydell wrote:
> Date: Tue, 20 Feb 2024 16:06:15 +0000
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: [PATCH 03/10] system/bootdevice: Don't unregister reset handler in
>  restore_boot_order()
> X-Mailer: git-send-email 2.34.1
> 
> Currently the qemu_register_reset() API permits the reset handler functions
> registered with it to remove themselves from within the callback function.
> This is fine with our current implementation, but is a bit odd, because
> generally reset is supposed to be idempotent, and doesn't fit well in a
> three-phase-reset world where a resettable object will get multiple
> callbacks as the system is reset.
> 
> We now have only one user of qemu_register_reset() which makes use of
> the ability to unregister itself within the callback:
> restore_boot_order().  We want to change our implementation of
> qemu_register_reset() to something where it would be awkward to
> maintain the "can self-unregister" feature.  Rather than making that
> reimplementation complicated, change restore_boot_order() so that it
> doesn't unregister itself but instead returns doing nothing for any
> calls after it has done the "restore the boot order" work.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> It would be nicer not to use reset at all, especially since I'm not
> a fan of conflating "system is reset" with "system boots", but I
> didn't have a good idea for how to do that.
> ---
>  system/bootdevice.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 05/10] hw/core: Add documentation and license comments to reset.h

[Zhao Liu]

Hi Peter,

On Tue, Feb 20, 2024 at 04:06:17PM +0000, Peter Maydell wrote:
> Date: Tue, 20 Feb 2024 16:06:17 +0000
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: [PATCH 05/10] hw/core: Add documentation and license comments to
>  reset.h
> X-Mailer: git-send-email 2.34.1
> 
> Add the usual boilerplate license/copyright comment to reset.h (using
> the text from reset.c), and document the existing functions.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/sysemu/reset.h | 79 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/include/sysemu/reset.h b/include/sysemu/reset.h
> index 609e4d50c26..6aa11846a69 100644
> --- a/include/sysemu/reset.h
> +++ b/include/sysemu/reset.h
> @@ -1,3 +1,29 @@
> +/*
> + *  Reset handlers.
> + *
> + * Copyright (c) 2003-2008 Fabrice Bellard
> + * Copyright (c) 2016 Red Hat, Inc.
> + * Copyright (c) 2024 Linaro, Ltd.

An additional question, when there is a new (notable) contribution to a
file, then it's time to add the company's copyright. Right?

> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
>  #ifndef QEMU_SYSEMU_RESET_H
>  #define QEMU_SYSEMU_RESET_H
>  
> @@ -5,9 +31,62 @@
>  
>  typedef void QEMUResetHandler(void *opaque);
>  
> +/**
> + * qemu_register_reset: Register a callback for system reset
> + * @func: function to call
> + * @opaque: opaque data to pass to @func
> + *
> + * Register @func on the list of functions which are called when the
> + * entire system is reset. The functions are called in the order in
> + * which they are registered.
> + *
> + * In general this function should not be used in new code where possible;
> + * for instance device model reset is better accomplished using the

s/for instance device/for instance, device/

> + * methods on DeviceState.
> + *
> + * It is not permitted to register or unregister reset functions from
> + * within the @func callback.
> + *
> + * We assume that the caller holds the BQL.

HMM, what does BQL stand for?

Others LGTM.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 05/10] hw/core: Add documentation and license comments to reset.h

[Peter Maydell]

On Mon, 26 Feb 2024 at 14:13, Zhao Liu <zhao1.liu@intel.com> wrote:
>
> Hi Peter,
>
> On Tue, Feb 20, 2024 at 04:06:17PM +0000, Peter Maydell wrote:
> > Date: Tue, 20 Feb 2024 16:06:17 +0000
> > From: Peter Maydell <peter.maydell@linaro.org>
> > Subject: [PATCH 05/10] hw/core: Add documentation and license comments to
> >  reset.h
> > X-Mailer: git-send-email 2.34.1
> >
> > Add the usual boilerplate license/copyright comment to reset.h (using
> > the text from reset.c), and document the existing functions.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  include/sysemu/reset.h | 79 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 79 insertions(+)
> >
> > diff --git a/include/sysemu/reset.h b/include/sysemu/reset.h
> > index 609e4d50c26..6aa11846a69 100644
> > --- a/include/sysemu/reset.h
> > +++ b/include/sysemu/reset.h
> > @@ -1,3 +1,29 @@
> > +/*
> > + *  Reset handlers.
> > + *
> > + * Copyright (c) 2003-2008 Fabrice Bellard
> > + * Copyright (c) 2016 Red Hat, Inc.
> > + * Copyright (c) 2024 Linaro, Ltd.
>
> An additional question, when there is a new (notable) contribution to a
> file, then it's time to add the company's copyright. Right?

It's OK to do that; it's not obligatory. If you're contributing
on behalf of a company you should generally follow whatever
that company's policies are for copyright lines etc in open
source projects.

> > + * We assume that the caller holds the BQL.
>
> HMM, what does BQL stand for?

"Big QEMU Lock", the one you get via BQL_LOCK_GUARD().
BQL is the standard abbreviation -- we already use it
extensively both in comments in include/ and in docs/devel/.

-- PMM

Re: [PATCH 04/10] include/qom/object.h: New OBJECT_DEFINE_SIMPLE_TYPE{, _WITH_INTERFACES} macros

[Zhao Liu]

On Tue, Feb 20, 2024 at 04:06:16PM +0000, Peter Maydell wrote:
> Date: Tue, 20 Feb 2024 16:06:16 +0000
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: [PATCH 04/10] include/qom/object.h: New
>  OBJECT_DEFINE_SIMPLE_TYPE{, _WITH_INTERFACES} macros
> X-Mailer: git-send-email 2.34.1
> 
> We have an OBJECT_DEFINE_TYPE_EXTENDED macro, plus several variations
> on it, which emits the boilerplate for the TypeInfo and ensures it is
> registered with the type system.  However, all the existing macros
> insist that the type being defined has its own FooClass struct, so
> they aren't useful for the common case of a simple leaf class which
> doesn't have any new methods or any other need for its own class
> struct (that is, for the kind of type that OBJECT_DECLARE_SIMPLE_TYPE
> declares).
> 
> Pull the actual implementation of OBJECT_DEFINE_TYPE_EXTENDED out
> into a new DO_OBJECT_DEFINE_TYPE_EXTENDED which parameterizes the
> value we use for the class_size field.  This lets us add a new
> OBJECT_DEFINE_SIMPLE_TYPE which does the same job as the various
> existing OBJECT_DEFINE_*_TYPE_* family macros for this kind of simple
> type, and the variant OBJECT_DEFINE_SIMPLE_TYPE_WITH_INTERFACES for
> when the type will implement some interfaces.
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  docs/devel/qom.rst   |  34 +++++++++++--
>  include/qom/object.h | 114 +++++++++++++++++++++++++++++++++----------
>  2 files changed, 117 insertions(+), 31 deletions(-)
>

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 06/10] hw/core: Add ResetContainer which holds objects implementing Resettable

[Peter Maydell]

On Tue, 20 Feb 2024 at 21:20, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 20 Feb 2024 at 19:46, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> >
> > On 2/20/24 06:06, Peter Maydell wrote:
> > > +void resettable_container_add(ResettableContainer *rc, Object *obj)
> > > +{
> > > +    g_ptr_array_add(rc->children, obj);
> > > +}
> >
> > Did you want to assert here that obj does in fact implement Resettable?
>
> I guess that makes for a nicer detection of that class of bug,
> so sure.

I'm going to do this with

     INTERFACE_CHECK(void, obj, TYPE_RESETTABLE_INTERFACE);

If anybody thinks there's a better way of doing that let me know.

-- PMM

Re: [PATCH 00/10] reset: Make whole system three-phase-reset aware

[Peter Maydell]

On Tue, 20 Feb 2024 at 16:06, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchset is an incremental improvement to our reset handling that
> tries to roll out the "three-phase-reset" design we have for devices
> to a wider scope.

The first two patches here are already upstream; I'm going
to take the rest via target-arm.next so we can get it into
git well before softfreeze and have some time to shake out
any unexpected consequences.

thanks
-- PMM

Re: [PATCH 06/10] hw/core: Add ResetContainer which holds objects implementing Resettable

[Zhao Liu]

On Tue, Feb 20, 2024 at 04:06:18PM +0000, Peter Maydell wrote:
> Date: Tue, 20 Feb 2024 16:06:18 +0000
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: [PATCH 06/10] hw/core: Add ResetContainer which holds objects
>  implementing Resettable
> X-Mailer: git-send-email 2.34.1
> 
> Implement a ResetContainer.  This is a subclass of Object, and it
> implements the Resettable interface.  The container holds a list of
> arbitrary other objects which implement Resettable, and when the
> container is reset, all the objects it contains are also reset.
> 
> This will allow us to have a 3-phase-reset equivalent of the old
> qemu_register_reset() API: we will have a single "simulation reset"
> top level ResetContainer, and objects in it are the equivalent of the
> old QEMUResetHandler functions.
> 
> The qemu_register_reset() API manages its list of callbacks using a
> QTAILQ, but here we use a GPtrArray for our list of Resettable
> children: we expect the "remove" operation (which will need to do an
> iteration through the list) to be fairly uncommon, and we get simpler
> code with fewer memory allocations.
> 
> Since there is currently no listed owner in MAINTAINERS for the
> existing reset-related source files, create a new section for
> them, and add these new files there also.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  MAINTAINERS                      | 10 +++++
>  include/hw/core/resetcontainer.h | 48 ++++++++++++++++++++
>  hw/core/resetcontainer.c         | 76 ++++++++++++++++++++++++++++++++
>  hw/core/meson.build              |  1 +
>  4 files changed, 135 insertions(+)
>  create mode 100644 include/hw/core/resetcontainer.h
>  create mode 100644 hw/core/resetcontainer.c
>

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 07/10] hw/core/reset: Add qemu_{register, unregister}_resettable()

[Zhao Liu]

On Tue, Feb 20, 2024 at 04:06:19PM +0000, Peter Maydell wrote:
> Date: Tue, 20 Feb 2024 16:06:19 +0000
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: [PATCH 07/10] hw/core/reset: Add qemu_{register,
>  unregister}_resettable()
> X-Mailer: git-send-email 2.34.1
> 
> Implement new functions qemu_register_resettable() and
> qemu_unregister_resettable().  These are intended to be
> three-phase-reset aware equivalents of the old qemu_register_reset()
> and qemu_unregister_reset().  Instead of passing in a function
> pointer and opaque, you register any QOM object that implements the
> Resettable interface.
> 
> The implementation is simple: we have a single global instance of a
> ResettableContainer, which we reset in qemu_devices_reset(), and
> the Resettable objects passed to qemu_register_resettable() are
> added to it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/sysemu/reset.h | 37 ++++++++++++++++++++++++++++++++++---
>  hw/core/reset.c        | 31 +++++++++++++++++++++++++++++--
>  2 files changed, 63 insertions(+), 5 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com> 

Re: [PATCH 08/10] hw/core/reset: Implement qemu_register_reset via qemu_register_resettable

[Zhao Liu]

On Tue, Feb 20, 2024 at 04:06:20PM +0000, Peter Maydell wrote:
> Date: Tue, 20 Feb 2024 16:06:20 +0000
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: [PATCH 08/10] hw/core/reset: Implement qemu_register_reset via
>  qemu_register_resettable
> X-Mailer: git-send-email 2.34.1
> 
> Reimplement qemu_register_reset() via qemu_register_resettable().
> 
> We define a new LegacyReset object which implements Resettable and
> defines its reset hold phase method to call a QEMUResetHandler
> function.  When qemu_register_reset() is called, we create a new
> LegacyReset object and add it to the simulation_reset
> ResettableContainer.  When qemu_unregister_reset() is called, we find
> the LegacyReset object in the container and remove it.
> 
> This implementation of qemu_unregister_reset() means we'll end up
> scanning the ResetContainer's list of child objects twice, once
> to find the LegacyReset object, and once in g_ptr_array_remove().
> In theory we could avoid this by having the ResettableContainer
> interface include a resettable_container_remove_with_equal_func()
> that took a callback method so that we could use
> g_ptr_array_find_with_equal_func() and g_ptr_array_remove_index().
> But we don't expect qemu_unregister_reset() to be called frequently
> or in hot paths, and we expect the simulation_reset container to
> usually not have many children.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> The way that a legacy reset function needs to check the ShutdownCause
> and this doesn't line up with the ResetType is a bit awkward; this
> is an area we should come back and clean up, but I didn't want to
> tackle that in this patchset.
> ---
>  include/sysemu/reset.h |   7 ++-
>  hw/core/reset.c        | 137 +++++++++++++++++++++++++++++++----------
>  2 files changed, 110 insertions(+), 34 deletions(-)
>

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 09/10] hw/core/machine: Use qemu_register_resettable for sysbus reset

[Zhao Liu]

On Tue, Feb 20, 2024 at 04:06:21PM +0000, Peter Maydell wrote:
> Date: Tue, 20 Feb 2024 16:06:21 +0000
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: [PATCH 09/10] hw/core/machine: Use qemu_register_resettable for
>  sysbus reset
> X-Mailer: git-send-email 2.34.1
> 
> Move the reset of the sysbus (and thus all devices and buses anywhere
> on the qbus tree) from qemu_register_reset() to qemu_register_resettable().
> 
> This is a behaviour change: because qemu_register_resettable() is
> aware of three-phase reset, this now means that:
>  * 'enter' phase reset methods of devices and buses are called
>    before any legacy reset callbacks registered with qemu_register_reset()
>  * 'exit' phase reset methods of devices and buses are called
>    after any legacy qemu_register_reset() callbacks
> 
> Put another way, a qemu_register_reset() callback is now correctly
> ordered in the 'hold' phase along with any other 'hold' phase methods.
> 
> The motivation for doing this is that we will now be able to resolve
> some reset-ordering issues using the three-phase mechanism, because
> the 'exit' phase is always after the 'hold' phase, even when the
> 'hold' phase function was registered with qemu_register_reset().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I believe that given we don't make much use of enter/exit phases
> currently that this is unlikely to cause unexpected regressions due
> to an accidental reset-order dependency that is no longer satisfied,
> but it's always possible...
> ---
>  hw/core/machine.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 10/10] docs/devel/reset: Update to discuss system reset

[Zhao Liu]

On Tue, Feb 20, 2024 at 04:06:22PM +0000, Peter Maydell wrote:
> Date: Tue, 20 Feb 2024 16:06:22 +0000
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: [PATCH 10/10] docs/devel/reset: Update to discuss system reset
> X-Mailer: git-send-email 2.34.1
> 
> Now that system reset uses a three-phase-reset, update the reset
> documentation to include a section describing how this works.
> Include documentation of the current major beartrap in reset, which
> is that only devices on the qbus tree will get automatically reset.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This merely documents the current situation, and says nothing
> about what we might like to do with it in future...
> ---
>  docs/devel/reset.rst | 44 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 42 insertions(+), 2 deletions(-)
>

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>