qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 00/13] Multi-phase reset mechanism
@ 2019-10-18 15:06 Damien Hedde
  2019-10-18 15:06 ` [PATCH v5 01/13] add device_legacy_reset function to prepare for reset api change Damien Hedde
                   ` (16 more replies)
  0 siblings, 17 replies; 57+ messages in thread
From: Damien Hedde @ 2019-10-18 15:06 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, berrange, ehabkost, cohuck,
	mark.burton, qemu-s390x, edgari, qemu-arm, pbonzini, philmd,
	david

Hi all,

The purpose of this series is to split the current reset procedure
into multiple phases. This will help to solve some ordering
difficulties we have during reset. Previous version can be found here:
https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg04359.html

This series adds resettable interface and transitions base Device and
Bus classes (sysbus subclasses are ok too). It provides new reset
functions but does not switch anymore the old functions
(device_reset() and qdev/qbus_reset_all()) to resettable interface.
These functions keep the exact same behavior as before.

The series also transition the main reset handlers registration which
has no impact until devices and buses are transitioned.

I think this version is way better regarding the transition from the
legacy to the resettable interface than the previous one.
After this series, the plan is then to transition devices, buses and
legacy reset call sites. Devices and buses have to be transitioned
from mother class to daughter classes order but until the final
(daughter) class is transitioned, old monolitic reset behavior will
be kept for this class.

Changes v4 -> v5:
  + various improvement in the resettable interface (regarding
    transition, robustness and several reset types)
  + better handling of transition from legacy reset to resettable
  + device hotplug and parent bus 'hot' change support
  + improved doc with examples and converted to rst format

Regarding the Resettable interface changes and how to handle more
reset types, please read patch 3 message.

The series is organized as follows:
Patch 1 is unmodified. Patch 2 adds some utility trace events.
Patches 3 to 8 adds resettable api in devices and buses. Patch 9 adds
some documentation. Patches 10 and 11 transition the call sites of
qemu_register_reset(qdev/qbus_reset_all_fn, ...).

Apart from patch 7 about hotplug which is really a rfc. I think other
patches are in pretty good shape.
Patch 3 and 4 are quite big but I don't think it make much sense to
split them. I could give it a try if you think it will ease reviews.
Note that depending on what name we choose for device/bus reset
functions (see patch 8), we may finally don't need patch 1.

I've also added patches 12 and 13 which handle the raspi sd card
reparenting. I'm not sure they fit well in this series but at some
point in this development I thought they had to be before patch 9
(finally it is not the case). Since I had to develop some specific
resettable support just for this case , I kept them as an example
of what transition a device is. Note that patch 13 handle the only
reset parent change (tricky) case I found (apart from hotplug).

Thanks for your feedback,
Damien

Damien Hedde (13):
  add device_legacy_reset function to prepare for reset api change
  hw/core/qdev: add trace events to help with resettable transition
  hw/core: create Resettable QOM interface
  hw/core: add Resettable support to BusClass and DeviceClass
  hw/core/resettable: add support for changing parent
  hw/core/qdev: handle parent bus change regarding resettable
  hw/core/qdev: update hotplug reset regarding resettable
  hw/core: deprecate old reset functions and introduce new ones
  docs/devel/reset.txt: add doc about Resettable interface
  vl: replace deprecated qbus_reset_all registration
  hw/s390x/ipl: replace deprecated qdev_reset_all registration
  hw/gpio/bcm2835_gpio: Isolate sdbus reparenting
  hw/gpio/bcm2835_gpio: Update to resettable

 Makefile.objs            |   1 +
 docs/devel/reset.rst     | 282 ++++++++++++++++++++++++++++++++++++++
 hw/audio/intel-hda.c     |   2 +-
 hw/core/Makefile.objs    |   1 +
 hw/core/bus.c            | 102 ++++++++++++++
 hw/core/qdev.c           | 156 +++++++++++++++++++--
 hw/core/resettable.c     | 289 +++++++++++++++++++++++++++++++++++++++
 hw/core/trace-events     |  27 ++++
 hw/gpio/bcm2835_gpio.c   |  31 +++--
 hw/hyperv/hyperv.c       |   2 +-
 hw/i386/pc.c             |   2 +-
 hw/ide/microdrive.c      |   8 +-
 hw/intc/spapr_xive.c     |   2 +-
 hw/ppc/pnv_psi.c         |   2 +-
 hw/ppc/spapr_pci.c       |   2 +-
 hw/ppc/spapr_vio.c       |   2 +-
 hw/s390x/ipl.c           |  10 +-
 hw/s390x/s390-pci-inst.c |   2 +-
 hw/scsi/vmw_pvscsi.c     |   2 +-
 hw/sd/omap_mmc.c         |   2 +-
 hw/sd/pl181.c            |   2 +-
 include/hw/qdev-core.h   |  58 +++++++-
 include/hw/resettable.h  | 224 ++++++++++++++++++++++++++++++
 tests/Makefile.include   |   1 +
 vl.c                     |  10 +-
 25 files changed, 1185 insertions(+), 37 deletions(-)
 create mode 100644 docs/devel/reset.rst
 create mode 100644 hw/core/resettable.c
 create mode 100644 include/hw/resettable.h

-- 
2.23.0



^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH v5 01/13] add device_legacy_reset function to prepare for reset api change
  2019-10-18 15:06 [PATCH v5 00/13] Multi-phase reset mechanism Damien Hedde
@ 2019-10-18 15:06 ` Damien Hedde
  2019-10-19 17:35   ` Richard Henderson
  2019-12-03 10:55   ` Cornelia Huck
  2019-10-18 15:06 ` [PATCH v5 02/13] hw/core/qdev: add trace events to help with resettable transition Damien Hedde
                   ` (15 subsequent siblings)
  16 siblings, 2 replies; 57+ messages in thread
From: Damien Hedde @ 2019-10-18 15:06 UTC (permalink / raw)
  To: qemu-devel
  Cc: Fam Zheng, peter.maydell, Collin Walling, Dmitry Fleytman,
	Michael S. Tsirkin, Gerd Hoffmann, David Hildenbrand,
	Halil Pasic, Christian Borntraeger, david, philmd, ehabkost,
	qemu-s390x, qemu-arm, Cédric Le Goater, John Snow,
	Richard Henderson, Damien Hedde, berrange, cohuck, mark.burton,
	edgari, pbonzini

Provide a temporary device_legacy_reset function doing what
device_reset does to prepare for the transition with Resettable
API.

All occurrence of device_reset in the code tree are also replaced
by device_legacy_reset.

The new resettable API has different prototype and semantics
(resetting child buses as well as the specified device). Subsequent
commits will make the changeover for each call site individually; once
that is complete device_legacy_reset() will be removed.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Acked-by: David Gibson <david@gibson.dropbear.id.au>
---
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: John Snow <jsnow@redhat.com>
Cc: "Cédric Le Goater" <clg@kaod.org>
Cc: Collin Walling <walling@linux.ibm.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Halil Pasic <pasic@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Dmitry Fleytman <dmitry.fleytman@gmail.com>
Cc: Fam Zheng <fam@euphon.net>
---
 hw/audio/intel-hda.c     | 2 +-
 hw/core/qdev.c           | 6 +++---
 hw/hyperv/hyperv.c       | 2 +-
 hw/i386/pc.c             | 2 +-
 hw/ide/microdrive.c      | 8 ++++----
 hw/intc/spapr_xive.c     | 2 +-
 hw/ppc/pnv_psi.c         | 2 +-
 hw/ppc/spapr_pci.c       | 2 +-
 hw/ppc/spapr_vio.c       | 2 +-
 hw/s390x/s390-pci-inst.c | 2 +-
 hw/scsi/vmw_pvscsi.c     | 2 +-
 hw/sd/omap_mmc.c         | 2 +-
 hw/sd/pl181.c            | 2 +-
 include/hw/qdev-core.h   | 4 ++--
 14 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 6ecd383540..27b71c57cf 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -1087,7 +1087,7 @@ static void intel_hda_reset(DeviceState *dev)
     QTAILQ_FOREACH(kid, &d->codecs.qbus.children, sibling) {
         DeviceState *qdev = kid->child;
         cdev = HDA_CODEC_DEVICE(qdev);
-        device_reset(DEVICE(cdev));
+        device_legacy_reset(DEVICE(cdev));
         d->state_sts |= (1 << cdev->cad);
     }
     intel_hda_update_irq(d);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cbad6c1d55..60be2f2fef 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -274,7 +274,7 @@ HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
 
 static int qdev_reset_one(DeviceState *dev, void *opaque)
 {
-    device_reset(dev);
+    device_legacy_reset(dev);
 
     return 0;
 }
@@ -882,7 +882,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
             }
         }
         if (dev->hotplugged) {
-            device_reset(dev);
+            device_legacy_reset(dev);
         }
         dev->pending_deleted_event = false;
 
@@ -1104,7 +1104,7 @@ void device_class_set_parent_unrealize(DeviceClass *dc,
     dc->unrealize = dev_unrealize;
 }
 
-void device_reset(DeviceState *dev)
+void device_legacy_reset(DeviceState *dev)
 {
     DeviceClass *klass = DEVICE_GET_CLASS(dev);
 
diff --git a/hw/hyperv/hyperv.c b/hw/hyperv/hyperv.c
index 6ebf31c310..cd9db3cb5c 100644
--- a/hw/hyperv/hyperv.c
+++ b/hw/hyperv/hyperv.c
@@ -140,7 +140,7 @@ void hyperv_synic_reset(CPUState *cs)
     SynICState *synic = get_synic(cs);
 
     if (synic) {
-        device_reset(DEVICE(synic));
+        device_legacy_reset(DEVICE(synic));
     }
 }
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 4b1904237e..50e64078e1 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2668,7 +2668,7 @@ static void pc_machine_reset(MachineState *machine)
         cpu = X86_CPU(cs);
 
         if (cpu->apic_state) {
-            device_reset(cpu->apic_state);
+            device_legacy_reset(cpu->apic_state);
         }
     }
 }
diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c
index b0272ea14b..6b30e36ed8 100644
--- a/hw/ide/microdrive.c
+++ b/hw/ide/microdrive.c
@@ -173,7 +173,7 @@ static void md_attr_write(PCMCIACardState *card, uint32_t at, uint8_t value)
     case 0x00:	/* Configuration Option Register */
         s->opt = value & 0xcf;
         if (value & OPT_SRESET) {
-            device_reset(DEVICE(s));
+            device_legacy_reset(DEVICE(s));
         }
         md_interrupt_update(s);
         break;
@@ -316,7 +316,7 @@ static void md_common_write(PCMCIACardState *card, uint32_t at, uint16_t value)
     case 0xe:	/* Device Control */
         s->ctrl = value;
         if (value & CTRL_SRST) {
-            device_reset(DEVICE(s));
+            device_legacy_reset(DEVICE(s));
         }
         md_interrupt_update(s);
         break;
@@ -541,7 +541,7 @@ static int dscm1xxxx_attach(PCMCIACardState *card)
     md->attr_base = pcc->cis[0x74] | (pcc->cis[0x76] << 8);
     md->io_base = 0x0;
 
-    device_reset(DEVICE(md));
+    device_legacy_reset(DEVICE(md));
     md_interrupt_update(md);
 
     return 0;
@@ -551,7 +551,7 @@ static int dscm1xxxx_detach(PCMCIACardState *card)
 {
     MicroDriveState *md = MICRODRIVE(card);
 
-    device_reset(DEVICE(md));
+    device_legacy_reset(DEVICE(md));
     return 0;
 }
 
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index 04879abf2e..170e87c7aa 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -1510,7 +1510,7 @@ static target_ulong h_int_reset(PowerPCCPU *cpu,
         return H_PARAMETER;
     }
 
-    device_reset(DEVICE(xive));
+    device_legacy_reset(DEVICE(xive));
 
     if (kvm_irqchip_in_kernel()) {
         Error *local_err = NULL;
diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
index a997f16bb4..9d65784a52 100644
--- a/hw/ppc/pnv_psi.c
+++ b/hw/ppc/pnv_psi.c
@@ -705,7 +705,7 @@ static void pnv_psi_p9_mmio_write(void *opaque, hwaddr addr,
         break;
     case PSIHB9_INTERRUPT_CONTROL:
         if (val & PSIHB9_IRQ_RESET) {
-            device_reset(DEVICE(&psi9->source));
+            device_legacy_reset(DEVICE(&psi9->source));
         }
         psi->regs[reg] = val;
         break;
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 01ff41d4c4..c1fe60b218 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -2014,7 +2014,7 @@ static int spapr_phb_children_reset(Object *child, void *opaque)
     DeviceState *dev = (DeviceState *) object_dynamic_cast(child, TYPE_DEVICE);
 
     if (dev) {
-        device_reset(dev);
+        device_legacy_reset(dev);
     }
 
     return 0;
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index 554de9930d..f14944e900 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -304,7 +304,7 @@ int spapr_vio_send_crq(SpaprVioDevice *dev, uint8_t *crq)
 static void spapr_vio_quiesce_one(SpaprVioDevice *dev)
 {
     if (dev->tcet) {
-        device_reset(DEVICE(dev->tcet));
+        device_legacy_reset(DEVICE(dev->tcet));
     }
     free_crq(dev);
 }
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 92c7e45df5..2f7a7d7bd1 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -243,7 +243,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
                 stw_p(&ressetpci->hdr.rsp, CLP_RC_SETPCIFN_FHOP);
                 goto out;
             }
-            device_reset(DEVICE(pbdev));
+            device_legacy_reset(DEVICE(pbdev));
             pbdev->fh &= ~FH_MASK_ENABLE;
             pbdev->state = ZPCI_FS_DISABLED;
             stl_p(&ressetpci->fh, pbdev->fh);
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index 452a3b63b2..7baab1532f 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -838,7 +838,7 @@ pvscsi_on_cmd_reset_device(PVSCSIState *s)
 
     if (sdev != NULL) {
         s->resetting++;
-        device_reset(&sdev->qdev);
+        device_legacy_reset(&sdev->qdev);
         s->resetting--;
         return PVSCSI_COMMAND_PROCESSING_SUCCEEDED;
     }
diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index c6e516b611..4088a8a80b 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -318,7 +318,7 @@ void omap_mmc_reset(struct omap_mmc_s *host)
      * into any bus, and we must reset it manually. When omap_mmc is
      * QOMified this must move into the QOM reset function.
      */
-    device_reset(DEVICE(host->card));
+    device_legacy_reset(DEVICE(host->card));
 }
 
 static uint64_t omap_mmc_read(void *opaque, hwaddr offset,
diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index 8033fe455d..2b3776a6a0 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -482,7 +482,7 @@ static void pl181_reset(DeviceState *d)
     /* Since we're still using the legacy SD API the card is not plugged
      * into any bus, and we must reset it manually.
      */
-    device_reset(DEVICE(s->card));
+    device_legacy_reset(DEVICE(s->card));
 }
 
 static void pl181_init(Object *obj)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index aa123f88cb..01963984f8 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -407,11 +407,11 @@ char *qdev_get_own_fw_dev_path_from_handler(BusState *bus, DeviceState *dev);
 void qdev_machine_init(void);
 
 /**
- * @device_reset
+ * device_legacy_reset:
  *
  * Reset a single device (by calling the reset method).
  */
-void device_reset(DeviceState *dev);
+void device_legacy_reset(DeviceState *dev);
 
 void device_class_set_parent_reset(DeviceClass *dc,
                                    DeviceReset dev_reset,
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH v5 02/13] hw/core/qdev: add trace events to help with resettable transition
  2019-10-18 15:06 [PATCH v5 00/13] Multi-phase reset mechanism Damien Hedde
  2019-10-18 15:06 ` [PATCH v5 01/13] add device_legacy_reset function to prepare for reset api change Damien Hedde
@ 2019-10-18 15:06 ` Damien Hedde
  2019-10-19 17:44   ` Richard Henderson
                     ` (2 more replies)
  2019-10-18 15:06 ` [PATCH v5 03/13] hw/core: create Resettable QOM interface Damien Hedde
                   ` (14 subsequent siblings)
  16 siblings, 3 replies; 57+ messages in thread
From: Damien Hedde @ 2019-10-18 15:06 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, berrange, ehabkost, cohuck,
	mark.burton, qemu-s390x, edgari, qemu-arm, pbonzini, philmd,
	david

Adds trace events to reset procedure and when updating the parent
bus of a device.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 hw/core/qdev.c       | 27 ++++++++++++++++++++++++---
 hw/core/trace-events |  9 +++++++++
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 60be2f2fef..f230063189 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -38,6 +38,7 @@
 #include "hw/boards.h"
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
+#include "trace.h"
 
 bool qdev_hotplug = false;
 static bool qdev_hot_added = false;
@@ -98,7 +99,9 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
     bool replugging = dev->parent_bus != NULL;
 
     if (replugging) {
-        /* Keep a reference to the device while it's not plugged into
+        trace_qdev_update_parent_bus(dev, dev->parent_bus, bus);
+        /*
+         * Keep a reference to the device while it's not plugged into
          * any bus, to avoid it potentially evaporating when it is
          * dereffed in bus_remove_child().
          */
@@ -272,6 +275,18 @@ HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
     return hotplug_ctrl;
 }
 
+static int qdev_prereset(DeviceState *dev, void *opaque)
+{
+    trace_qdev_reset_tree(dev, object_get_typename(OBJECT(dev)));
+    return 0;
+}
+
+static int qbus_prereset(BusState *bus, void *opaque)
+{
+    trace_qbus_reset_tree(bus, object_get_typename(OBJECT(bus)));
+    return 0;
+}
+
 static int qdev_reset_one(DeviceState *dev, void *opaque)
 {
     device_legacy_reset(dev);
@@ -282,6 +297,7 @@ static int qdev_reset_one(DeviceState *dev, void *opaque)
 static int qbus_reset_one(BusState *bus, void *opaque)
 {
     BusClass *bc = BUS_GET_CLASS(bus);
+    trace_qbus_reset(bus, object_get_typename(OBJECT(bus)));
     if (bc->reset) {
         bc->reset(bus);
     }
@@ -290,7 +306,9 @@ static int qbus_reset_one(BusState *bus, void *opaque)
 
 void qdev_reset_all(DeviceState *dev)
 {
-    qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL);
+    trace_qdev_reset_all(dev, object_get_typename(OBJECT(dev)));
+    qdev_walk_children(dev, qdev_prereset, qbus_prereset,
+                       qdev_reset_one, qbus_reset_one, NULL);
 }
 
 void qdev_reset_all_fn(void *opaque)
@@ -300,7 +318,9 @@ void qdev_reset_all_fn(void *opaque)
 
 void qbus_reset_all(BusState *bus)
 {
-    qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL);
+    trace_qbus_reset_all(bus, object_get_typename(OBJECT(bus)));
+    qbus_walk_children(bus, qdev_prereset, qbus_prereset,
+                       qdev_reset_one, qbus_reset_one, NULL);
 }
 
 void qbus_reset_all_fn(void *opaque)
@@ -1108,6 +1128,7 @@ void device_legacy_reset(DeviceState *dev)
 {
     DeviceClass *klass = DEVICE_GET_CLASS(dev);
 
+    trace_qdev_reset(dev, object_get_typename(OBJECT(dev)));
     if (klass->reset) {
         klass->reset(dev);
     }
diff --git a/hw/core/trace-events b/hw/core/trace-events
index fe47a9c8cb..1a669be0ea 100644
--- a/hw/core/trace-events
+++ b/hw/core/trace-events
@@ -1,2 +1,11 @@
 # loader.c
 loader_write_rom(const char *name, uint64_t gpa, uint64_t size, bool isrom) "%s: @0x%"PRIx64" size=0x%"PRIx64" ROM=%d"
+
+# qdev.c
+qdev_reset(void *obj, const char *objtype) "obj=%p(%s)"
+qdev_reset_all(void *obj, const char *objtype) "obj=%p(%s)"
+qdev_reset_tree(void *obj, const char *objtype) "obj=%p(%s)"
+qbus_reset(void *obj, const char *objtype) "obj=%p(%s)"
+qbus_reset_all(void *obj, const char *objtype) "obj=%p(%s)"
+qbus_reset_tree(void *obj, const char *objtype) "obj=%p(%s)"
+qdev_update_parent_bus(void *obj, void *oldp, void *newp) "obj=%p old_parent=%p new_parent=%p"
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH v5 03/13] hw/core: create Resettable QOM interface
  2019-10-18 15:06 [PATCH v5 00/13] Multi-phase reset mechanism Damien Hedde
  2019-10-18 15:06 ` [PATCH v5 01/13] add device_legacy_reset function to prepare for reset api change Damien Hedde
  2019-10-18 15:06 ` [PATCH v5 02/13] hw/core/qdev: add trace events to help with resettable transition Damien Hedde
@ 2019-10-18 15:06 ` Damien Hedde
  2019-11-29 18:32   ` Peter Maydell
  2019-12-03 11:16   ` Cornelia Huck
  2019-10-18 15:06 ` [PATCH v5 04/13] hw/core: add Resettable support to BusClass and DeviceClass Damien Hedde
                   ` (13 subsequent siblings)
  16 siblings, 2 replies; 57+ messages in thread
From: Damien Hedde @ 2019-10-18 15:06 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, berrange, ehabkost, cohuck,
	mark.burton, qemu-s390x, edgari, qemu-arm, pbonzini, philmd,
	david

This commit defines an interface allowing multi-phase reset. This aims
to solve a problem of the actual single-phase reset (built in
DeviceClass and BusClass): reset behavior is dependent on the order
in which reset handlers are called. In particular doing external
side-effect (like setting an qemu_irq) is problematic because receiving
object may not be reset yet.

The Resettable interface divides the reset in 3 well defined phases.
To reset an object tree, all 1st phases are executed then all 2nd then
all 3rd. See the comments in include/hw/resettable.h for a more complete
description. The interface defines 3 phases to let the future
possibility of holding an object into reset for some time.

The qdev/qbus reset in DeviceClass and BusClass will be modified in
following commits to use this interface. A mechanism is provided
to allow executing a transitional reset handler in place of the 2nd
phase which is executed in children-then-parent order inside a tree.
This will allow to transition devices and buses smoothly while
keeping the exact current qdev/qbus reset behavior for now.

Documentation will be added in a following commit.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---

In this patch only a single reset type is supported, but the interface
allows for more to be defined.

I had some thought about problems which may arise when having multiple
reset types:

- reset type propagation. Right now we propagate the same reset type
  to the children. I don't think it will work that with multiple
  types.
  For example, if we add pci_bus_reset type: a pci device will
  implement the reset type but not its children (they may have
  nothing to do with pci).
  This can be solved by changing the child_foreach method rules.
  We should say that child_foreach may change the type it
  propagates to its children (on a children by children basis).
  For example, the pci device may just propagate cold reset type
  to its children.
  For this we need to pass the type as parameter to child_foreach()
  method.

- are all children concerned ? For a given reset type, some child
  may not need to be reset. As above we can handle that with
  child_foreach: an resettable object can propagate the reset only
  to a partial set of its child.
  For this we need to know the type when we release the reset,
  that's why I added it to resettable_release_reset() even if it
  is unused right now.
  I've also added an opaque parameter to child_foreach. I think
  we will need that to handle the change of parent because we
  will need to test if a child is concerned by a reset type: the
  opaque will allow to use a test callback and get some result.

- several reset types at the same time. I don't another solution
  than saying we execute *enter* and *hold* phase for every reset
  type. *exit* will still be executed once for all at the end.
  It will be up for each object to cope with it if it handle
  multiple reset types. For *enter* is trivial, calling it twice
  in a row is no problem given that it should only reset internal
  state. For *hold* there may be some complication.

- Obviously we will need to at least an interface class field to hold
  the supported reset types by the class. Also the reset state will
  need some modification.
---
 Makefile.objs           |   1 +
 hw/core/Makefile.objs   |   1 +
 hw/core/resettable.c    | 230 ++++++++++++++++++++++++++++++++++++++++
 hw/core/trace-events    |  17 +++
 include/hw/resettable.h | 199 ++++++++++++++++++++++++++++++++++
 5 files changed, 448 insertions(+)
 create mode 100644 hw/core/resettable.c
 create mode 100644 include/hw/resettable.h

diff --git a/Makefile.objs b/Makefile.objs
index abcbd89654..9c3fc37e29 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -191,6 +191,7 @@ trace-events-subdirs += migration
 trace-events-subdirs += net
 trace-events-subdirs += ui
 endif
+trace-events-subdirs += hw/core
 trace-events-subdirs += hw/display
 trace-events-subdirs += qapi
 trace-events-subdirs += qom
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index fd0550d1d9..18f5ddebce 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -1,6 +1,7 @@
 # core qdev-related obj files, also used by *-user:
 common-obj-y += qdev.o qdev-properties.o
 common-obj-y += bus.o reset.o
+common-obj-y += resettable.o
 common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
 common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
 # irq.o needed for qdev GPIO handling:
diff --git a/hw/core/resettable.c b/hw/core/resettable.c
new file mode 100644
index 0000000000..c5e11cff4f
--- /dev/null
+++ b/hw/core/resettable.c
@@ -0,0 +1,230 @@
+/*
+ * Resettable interface.
+ *
+ * Copyright (c) 2019 GreenSocs SAS
+ *
+ * Authors:
+ *   Damien Hedde
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "hw/resettable.h"
+#include "trace.h"
+
+/**
+ * resettable_phase_enter/hold/exit:
+ * Function executing a phase recursively in a resettable object and its
+ * children.
+ */
+static void resettable_phase_enter(Object *obj, void *opaque, ResetType type);
+static void resettable_phase_hold(Object *obj, void *opaque, ResetType type);
+static void resettable_phase_exit(Object *obj, void *opaque, ResetType type);
+
+/**
+ * enter_phase_in_progress:
+ * Flag telling whether we are currently in an enter phase where side
+ * effects are forbidden. This flag allows us to catch if reset is called
+ * again during during this phase.
+ */
+static bool enter_phase_in_progress;
+
+void resettable_reset(Object *obj, ResetType type)
+{
+    trace_resettable_reset(obj, type);
+    resettable_assert_reset(obj, type);
+    resettable_release_reset(obj, type);
+}
+
+void resettable_assert_reset(Object *obj, ResetType type)
+{
+    assert(!enter_phase_in_progress);
+    /* TODO: change that assert when adding support for other reset types */
+    assert(type == RESET_TYPE_COLD);
+    trace_resettable_reset_assert_begin(obj, type);
+    enter_phase_in_progress = true;
+    resettable_phase_enter(obj, NULL, type);
+    enter_phase_in_progress = false;
+    resettable_phase_hold(obj, NULL, type);
+    trace_resettable_reset_assert_end(obj);
+}
+
+void resettable_release_reset(Object *obj, ResetType type)
+{
+    assert(!enter_phase_in_progress);
+    /* TODO: change that assert when adding support for other reset types */
+    assert(type == RESET_TYPE_COLD);
+    trace_resettable_reset_release_begin(obj, type);
+    resettable_phase_exit(obj, NULL, type);
+    trace_resettable_reset_release_end(obj);
+}
+
+bool resettable_is_in_reset(Object *obj)
+{
+    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
+    ResettableState *s = rc->get_state(obj);
+
+    return (s->count > 0);
+}
+
+/**
+ * resettable_child_foreach:
+ * helper to avoid checking the existence of the method.
+ */
+static void resettable_child_foreach(ResettableClass *rc, Object *obj,
+                                     ResettableChildCallback cb,
+                                     void *opaque, ResetType type)
+{
+    if (rc->child_foreach) {
+        rc->child_foreach(obj, cb, opaque, type);
+    }
+}
+
+/**
+ * resettable_get_tr_func:
+ * helper to fetch transitional reset callback if any.
+ */
+static ResettableTrFunction resettable_get_tr_func(ResettableClass *rc,
+                                                   Object *obj)
+{
+    ResettableTrFunction tr_func = NULL;
+    if (rc->get_transitional_function) {
+        tr_func = rc->get_transitional_function(obj);
+    }
+    return tr_func;
+}
+
+static void resettable_phase_enter(Object *obj, void *opaque, ResetType type)
+{
+    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
+    ResettableState *s = rc->get_state(obj);
+    const char *obj_typename = object_get_typename(obj);
+    bool action_needed = false;
+
+    /* exit phase has to finish properly before entering back in reset */
+    assert(!s->exit_phase_in_progress);
+
+    trace_resettable_phase_enter_begin(obj, obj_typename, s->count, type);
+
+    /* Only take action if we really enter reset for the 1st time. */
+    /*
+     * TODO: if adding more ResetType support, some additional checks
+     * are probably needed here.
+     */
+    if (s->count++ == 0) {
+        action_needed = true;
+    }
+    /*
+     * We limit the count to an arbitrary "big" value. The value is big
+     * enough not to be triggered nominally.
+     * The assert will stop an infinite loop if there is a cycle in the
+     * reset tree. The loop goes through resettable_foreach_child below
+     * which at some point will call us again.
+     */
+    assert(s->count <= 50);
+
+    /*
+     * handle the children even if action_needed is at false so that
+     * children counts are incremented too
+     */
+    resettable_child_foreach(rc, obj, resettable_phase_enter, NULL, type);
+
+    /* execute enter phase for the object if needed */
+    if (action_needed) {
+        trace_resettable_phase_enter_exec(obj, obj_typename, type,
+                                          !!rc->phases.enter);
+        if (rc->phases.enter && !resettable_get_tr_func(rc, obj)) {
+            rc->phases.enter(obj, type);
+        }
+        s->hold_phase_pending = true;
+    }
+    trace_resettable_phase_enter_end(obj, obj_typename, s->count);
+}
+
+static void resettable_phase_hold(Object *obj, void *opaque, ResetType type)
+{
+    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
+    ResettableState *s = rc->get_state(obj);
+    const char *obj_typename = object_get_typename(obj);
+
+    /* exit phase has to finish properly before entering back in reset */
+    assert(!s->exit_phase_in_progress);
+
+    trace_resettable_phase_hold_begin(obj, obj_typename, s->count, type);
+
+    /* handle children first */
+    resettable_child_foreach(rc, obj, resettable_phase_hold, NULL, type);
+
+    /* exec hold phase */
+    if (s->hold_phase_pending) {
+        s->hold_phase_pending = false;
+        ResettableTrFunction tr_func = resettable_get_tr_func(rc, obj);
+        trace_resettable_phase_hold_exec(obj, obj_typename, !!rc->phases.hold);
+        if (tr_func) {
+            trace_resettable_transitional_function(obj, obj_typename);
+            tr_func(obj);
+        } else if (rc->phases.hold) {
+            rc->phases.hold(obj);
+        }
+    }
+    trace_resettable_phase_hold_end(obj, obj_typename, s->count);
+}
+
+static void resettable_phase_exit(Object *obj, void *opaque, ResetType type)
+{
+    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
+    ResettableState *s = rc->get_state(obj);
+    const char *obj_typename = object_get_typename(obj);
+
+    assert(!s->exit_phase_in_progress);
+    trace_resettable_phase_exit_begin(obj, obj_typename, s->count, type);
+
+    /* exit_phase_in_progress ensure this phase is 'atomic' */
+    s->exit_phase_in_progress = true;
+    resettable_child_foreach(rc, obj, resettable_phase_exit, NULL, type);
+
+    assert(s->count > 0);
+    if (s->count == 1) {
+        trace_resettable_phase_exit_exec(obj, obj_typename, !!rc->phases.exit);
+        if (rc->phases.exit && !resettable_get_tr_func(rc, obj)) {
+            rc->phases.exit(obj);
+        }
+        s->count = 0;
+    }
+    s->exit_phase_in_progress = false;
+    trace_resettable_phase_exit_end(obj, obj_typename, s->count);
+}
+
+void resettable_class_set_parent_phases(ResettableClass *rc,
+                                        ResettableEnterPhase enter,
+                                        ResettableHoldPhase hold,
+                                        ResettableExitPhase exit,
+                                        ResettablePhases *parent_phases)
+{
+    *parent_phases = rc->phases;
+    if (enter) {
+        rc->phases.enter = enter;
+    }
+    if (hold) {
+        rc->phases.hold = hold;
+    }
+    if (exit) {
+        rc->phases.exit = exit;
+    }
+}
+
+static const TypeInfo resettable_interface_info = {
+    .name       = TYPE_RESETTABLE_INTERFACE,
+    .parent     = TYPE_INTERFACE,
+    .class_size = sizeof(ResettableClass),
+};
+
+static void reset_register_types(void)
+{
+    type_register_static(&resettable_interface_info);
+}
+
+type_init(reset_register_types)
diff --git a/hw/core/trace-events b/hw/core/trace-events
index 1a669be0ea..66081d0fb4 100644
--- a/hw/core/trace-events
+++ b/hw/core/trace-events
@@ -9,3 +9,20 @@ qbus_reset(void *obj, const char *objtype) "obj=%p(%s)"
 qbus_reset_all(void *obj, const char *objtype) "obj=%p(%s)"
 qbus_reset_tree(void *obj, const char *objtype) "obj=%p(%s)"
 qdev_update_parent_bus(void *obj, void *oldp, void *newp) "obj=%p old_parent=%p new_parent=%p"
+
+# resettable.c
+resettable_reset(void *obj, int cold) "obj=%p cold=%d"
+resettable_reset_assert_begin(void *obj, int cold) "obj=%p cold=%d"
+resettable_reset_assert_end(void *obj) "obj=%p"
+resettable_reset_release_begin(void *obj, int cold) "obj=%p cold=%d"
+resettable_reset_release_end(void *obj) "obj=%p"
+resettable_phase_enter_begin(void *obj, const char *objtype, uint32_t count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
+resettable_phase_enter_exec(void *obj, const char *objtype, int type, int has_method) "obj=%p(%s) type=%d method=%d"
+resettable_phase_enter_end(void *obj, const char *objtype, uint32_t count) "obj=%p(%s) count=%" PRIu32
+resettable_phase_hold_begin(void *obj, const char *objtype, uint32_t count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
+resettable_phase_hold_exec(void *obj, const char *objtype, int has_method) "obj=%p(%s) method=%d"
+resettable_phase_hold_end(void *obj, const char *objtype, uint32_t count) "obj=%p(%s) count=%" PRIu32
+resettable_phase_exit_begin(void *obj, const char *objtype, uint32_t count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
+resettable_phase_exit_exec(void *obj, const char *objtype, int has_method) "obj=%p(%s) method=%d"
+resettable_phase_exit_end(void *obj, const char *objtype, uint32_t count) "obj=%p(%s) count=%" PRIu32
+resettable_transitional_function(void *obj, const char *objtype) "obj=%p(%s)"
diff --git a/include/hw/resettable.h b/include/hw/resettable.h
new file mode 100644
index 0000000000..6b24e1633b
--- /dev/null
+++ b/include/hw/resettable.h
@@ -0,0 +1,199 @@
+#ifndef HW_RESETTABLE_H
+#define HW_RESETTABLE_H
+
+#include "qom/object.h"
+
+#define TYPE_RESETTABLE_INTERFACE "resettable"
+
+#define RESETTABLE_CLASS(class) \
+    OBJECT_CLASS_CHECK(ResettableClass, (class), TYPE_RESETTABLE_INTERFACE)
+
+#define RESETTABLE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE_INTERFACE)
+
+typedef struct ResettableState ResettableState;
+
+/**
+ * ResetType:
+ * Types of reset.
+ *
+ * + Cold: reset resulting from a power cycle of the object.
+ *
+ * TODO: Support has to be added to handle more types. In particular,
+ * ResettableState structure needs to be expanded.
+ */
+typedef enum ResetType {
+    RESET_TYPE_COLD,
+} ResetType;
+
+/*
+ * ResettableClass:
+ * Interface for resettable objects.
+ *
+ * See docs/devel/reset.rst for more detailed information about
+ * how QEMU models reset.
+ *
+ * All objects which can be reset must implement this interface;
+ * it is usually provided by a base class such as DeviceClass or BusClass.
+ * Every Resettable object must maintain some state tracking the
+ * progress of a reset operation by providing a ResettableState structure.
+ * The functions defined in this module take care of updating the
+ * state of the reset.
+ * The base class implementation of the interface provides this
+ * state and implements the associated method: get_state.
+ *
+ * Concrete object implementations (typically specific devices
+ * such as a UART model) should provide the functions
+ * for the phases.enter, phases.hold and phases.exit methods, which
+ * they can set in their class init function, either directly or
+ * by calling resettable_class_set_parent_phases().
+ * The phase methods are guaranteed to only only ever be called once
+ * for any reset event, in the order 'enter', 'hold', 'exit'.
+ * An object will always move quickly from 'enter' to 'hold'
+ * but might remain in 'hold' for an arbitrary period of time
+ * before eventually reset is deasserted and the 'exit' phase is called.
+ * Object implementations should be prepared for functions handling
+ * inbound connections from other devices (such as qemu_irq handler
+ * functions) to be called at any point during reset after their
+ * 'enter' method has been called.
+ *
+ * Users of a resettable object should not call these methods
+ * directly, but instead use the function resettable_reset().
+ *
+ * @phases.enter: This phase is called when the object enters reset. It
+ * should reset local state of the object, but it must not do anything that
+ * has a side-effect on other objects, such as raising or lowering a qemu_irq
+ * line or reading or writing guest memory. It takes the reset's type as
+ * argument.
+ *
+ * @phases.hold: This phase is called for entry into reset, once every object
+ * in the system which is being reset has had its @phases.enter method called.
+ * At this point devices can do actions that affect other objects.
+ *
+ * @phases.exit: This phase is called when the object leaves the reset state.
+ * Actions affecting other objects are permitted.
+ *
+ * @get_state: Mandatory method which must return a pointer to a
+ * ResettableState.
+ *
+ * @get_transitional_function: transitional method to handle Resettable objects
+ * not yet fully moved to this interface. It will be removed as soon as is not
+ * needed anymore. This method is optional and may return a pointer to a
+ * function to be used instead of the phases. In case the method exists and
+ * returns a non-NULL function pointer; it is executed as a replacement of the
+ * 'hold' phase method taking the object as argument. The two other phase
+ * methods are not executed.
+ *
+ * @child_foreach: Executes a given callback on every Resettable child. Child
+ * in this context means a child in the qbus tree, so the children of a qbus
+ * are the devices on it, and the children of a device are all the buses it
+ * owns. This is not the same as the QOM object hierarchy. The function takes
+ * additional opaque and ResetType arguments which must be passed unmodified to
+ * the callback.
+ */
+typedef void (*ResettableEnterPhase)(Object *obj, ResetType type);
+typedef void (*ResettableHoldPhase)(Object *obj);
+typedef void (*ResettableExitPhase)(Object *obj);
+typedef ResettableState * (*ResettableGetState)(Object *obj);
+typedef void (*ResettableTrFunction)(Object *obj);
+typedef ResettableTrFunction (*ResettableGetTrFunction)(Object *obj);
+typedef void (*ResettableChildCallback)(Object *, void *opaque,
+                                        ResetType type);
+typedef void (*ResettableChildForeach)(Object *obj,
+                                       ResettableChildCallback cb,
+                                       void *opaque, ResetType type);
+typedef struct ResettableClass {
+    InterfaceClass parent_class;
+
+    /* Phase methods */
+    struct ResettablePhases {
+        ResettableEnterPhase enter;
+        ResettableHoldPhase hold;
+        ResettableExitPhase exit;
+    } phases;
+
+    /* State access method */
+    ResettableGetState get_state;
+
+    /* Transitional method for legacy reset compatibility */
+    ResettableGetTrFunction get_transitional_function;
+
+    /* Hiearchy handling method */
+    ResettableChildForeach child_foreach;
+} ResettableClass;
+typedef struct ResettablePhases ResettablePhases;
+
+/**
+ * ResettableState:
+ * Structure holding reset related state. The fields should not be accessed
+ * directly, the definition is here to allow further inclusion into other
+ * objects.
+ *
+ * @count: Number of reset level the object is into. It is incremented when
+ * the reset operation starts and decremented when it finishes.
+ * @hold_phase_pending: flag which indicates that we need to invoke the 'hold'
+ * phase handler for this object.
+ * @exit_phase_in_progress: flag telling if currently in exit phase
+ */
+struct ResettableState {
+    uint32_t count;
+    bool hold_phase_pending;
+    bool exit_phase_in_progress;
+};
+
+/**
+ * resettable_reset:
+ * Trigger a reset on a object @obj of type @type. @obj must implement
+ * Resettable interface.
+ *
+ * Calling this function is equivalent to calling @resettable_assert_reset()
+ * then @resettable_release_reset().
+ */
+void resettable_reset(Object *obj, ResetType type);
+
+/**
+ * resettable_assert_reset:
+ * Put an object @obj into reset. @obj must implement Resettable interface.
+ *
+ * @resettable_release_reset() must eventually be called after this call.
+ * There must be one call to @resettable_release_reset() per call of
+ * @resettable_assert_reset(), with the same type argument.
+ *
+ * NOTE: Until support for migration is added, the @resettable_release_reset()
+ * must not be delayed. It have to occur just after @resettable_assert_reset()
+ * so that migration cannot be triggered in between. Prefer using
+ * @resettable_reset() for now.
+ */
+void resettable_assert_reset(Object *obj, ResetType type);
+
+/**
+ * resettable_release_reset:
+ * Release the object @obj from reset. @obj must implement Resettable interface.
+ *
+ * See @resettable_assert_reset() description for details.
+ */
+void resettable_release_reset(Object *obj, ResetType type);
+
+/**
+ * resettable_is_in_reset:
+ * Return true if @obj is under reset.
+ *
+ * @obj must implement Resettable interface.
+ */
+bool resettable_is_in_reset(Object *obj);
+
+/**
+ * resettable_class_set_parent_phases:
+ *
+ * Save @rc current reset phases into @parent_phases and override @rc phases
+ * by the given new methods (@enter, @hold and @exit).
+ * Each phase is overridden only if the new one is not NULL allowing to
+ * override a subset of phases.
+ */
+void resettable_class_set_parent_phases(ResettableClass *rc,
+                                        ResettableEnterPhase enter,
+                                        ResettableHoldPhase hold,
+                                        ResettableExitPhase exit,
+                                        ResettablePhases *parent_phases);
+
+#endif
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH v5 04/13] hw/core: add Resettable support to BusClass and DeviceClass
  2019-10-18 15:06 [PATCH v5 00/13] Multi-phase reset mechanism Damien Hedde
                   ` (2 preceding siblings ...)
  2019-10-18 15:06 ` [PATCH v5 03/13] hw/core: create Resettable QOM interface Damien Hedde
@ 2019-10-18 15:06 ` Damien Hedde
  2019-10-19 18:49   ` Richard Henderson
  2019-11-29 18:36   ` Peter Maydell
  2019-10-18 15:06 ` [PATCH v5 05/13] hw/core/resettable: add support for changing parent Damien Hedde
                   ` (12 subsequent siblings)
  16 siblings, 2 replies; 57+ messages in thread
From: Damien Hedde @ 2019-10-18 15:06 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, berrange, ehabkost, cohuck,
	mark.burton, qemu-s390x, edgari, qemu-arm, pbonzini, philmd,
	david

This commit adds support of Resettable interface to buses and devices:
+ ResettableState structure is added in the Bus/Device state
+ Resettable methods are implemented.
+ device/bus_is_in_reset function defined

This commit allows to transition the objects to the new
multi-phase interface without changing the reset behavior at all.
Object single reset method can be split into the 3 different phases
but the 3 phases are still executed in a row for a given object.
From the qdev/qbus reset api point of view, nothing is changed.
qdev_reset_all() and qbus_reset_all() are not modified as well as
device_legacy_reset().

Transition of an object must be done from mother class to daughter
classes. Care has been taken to allow the transition of a mother class
without requiring the daughter classes to be transitioned at the same
time. Note that SysBus and SysBusDevice class do not need any transition
because they do not override the legacy reset method.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 hw/core/bus.c          | 97 ++++++++++++++++++++++++++++++++++++++++++
 hw/core/qdev.c         | 97 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/qdev-core.h | 27 ++++++++++++
 tests/Makefile.include |  1 +
 4 files changed, 222 insertions(+)

diff --git a/hw/core/bus.c b/hw/core/bus.c
index 7f3d2a3dbd..7c05e80db8 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -68,6 +68,28 @@ int qbus_walk_children(BusState *bus,
     return 0;
 }
 
+bool bus_is_in_reset(BusState *bus)
+{
+    return resettable_is_in_reset(OBJECT(bus));
+}
+
+static ResettableState *bus_get_reset_state(Object *obj)
+{
+    BusState *bus = BUS(obj);
+    return &bus->reset;
+}
+
+static void bus_reset_child_foreach(Object *obj, ResettableChildCallback cb,
+                                    void *opaque, ResetType type)
+{
+    BusState *bus = BUS(obj);
+    BusChild *kid;
+
+    QTAILQ_FOREACH(kid, &bus->children, sibling) {
+        cb(OBJECT(kid->child), opaque, type);
+    }
+}
+
 static void qbus_realize(BusState *bus, DeviceState *parent, const char *name)
 {
     const char *typename = object_get_typename(OBJECT(bus));
@@ -199,12 +221,83 @@ static char *default_bus_get_fw_dev_path(DeviceState *dev)
     return g_strdup(object_get_typename(OBJECT(dev)));
 }
 
+/**
+ * bus_phases_reset:
+ * Transition reset method for buses to allow moving
+ * smoothly from legacy reset method to multi-phases
+ */
+static void bus_phases_reset(BusState *bus)
+{
+    ResettableClass *rc = RESETTABLE_GET_CLASS(bus);
+
+    if (rc->phases.enter) {
+        rc->phases.enter(OBJECT(bus), RESET_TYPE_COLD);
+    }
+    if (rc->phases.hold) {
+        rc->phases.hold(OBJECT(bus));
+    }
+    if (rc->phases.exit) {
+        rc->phases.exit(OBJECT(bus));
+    }
+}
+
+static void bus_transitional_reset(Object *obj)
+{
+    BusClass *bc = BUS_GET_CLASS(obj);
+
+    /*
+     * This will call either @bus_phases_reset (for multi-phases transitioned
+     * buses) or a bus's specific method for not-yet transitioned buses.
+     * In both case, it does not reset children.
+     */
+    if (bc->reset) {
+        bc->reset(BUS(obj));
+    }
+}
+
+/**
+ * bus_get_transitional_reset:
+ * check if the bus's class is ready for multi-phase
+ */
+static ResettableTrFunction bus_get_transitional_reset(Object *obj)
+{
+    BusClass *dc = BUS_GET_CLASS(obj);
+    if (dc->reset != bus_phases_reset) {
+        /*
+         * dc->reset has been overridden by a subclass,
+         * the bus is not ready for multi phase yet.
+         */
+        return bus_transitional_reset;
+    }
+    return NULL;
+}
+
 static void bus_class_init(ObjectClass *class, void *data)
 {
     BusClass *bc = BUS_CLASS(class);
+    ResettableClass *rc = RESETTABLE_CLASS(class);
 
     class->unparent = bus_unparent;
     bc->get_fw_dev_path = default_bus_get_fw_dev_path;
+
+    rc->get_state = bus_get_reset_state;
+    rc->child_foreach = bus_reset_child_foreach;
+
+    /*
+     * @bus_phases_reset is put as the default reset method below, allowing
+     * to do the multi-phase transition from base classes to leaf classes. It
+     * allows a legacy-reset Bus class to extend a multi-phases-reset
+     * Bus class for the following reason:
+     * + If a base class B has been moved to multi-phase, then it does not
+     *   override this default reset method and may have defined phase methods.
+     * + A daughter class C (extending class B) which uses
+     *   bus_class_set_parent_reset() (or similar means) to override the
+     *   reset method will still work as expected. @bus_phases_reset function
+     *   will be registered as the parent reset method and effectively call
+     *   parent reset phases.
+     */
+    bc->reset = bus_phases_reset;
+    rc->get_transitional_function = bus_get_transitional_reset;
 }
 
 static void qbus_finalize(Object *obj)
@@ -223,6 +316,10 @@ static const TypeInfo bus_info = {
     .instance_init = qbus_initfn,
     .instance_finalize = qbus_finalize,
     .class_init = bus_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_RESETTABLE_INTERFACE },
+        { }
+    },
 };
 
 static void bus_register_types(void)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index f230063189..81784c73fb 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -329,6 +329,28 @@ void qbus_reset_all_fn(void *opaque)
     qbus_reset_all(bus);
 }
 
+bool device_is_in_reset(DeviceState *dev)
+{
+    return resettable_is_in_reset(OBJECT(dev));
+}
+
+static ResettableState *device_get_reset_state(Object *obj)
+{
+    DeviceState *dev = DEVICE(obj);
+    return &dev->reset;
+}
+
+static void device_reset_child_foreach(Object *obj, ResettableChildCallback cb,
+                                       void *opaque, ResetType type)
+{
+    DeviceState *dev = DEVICE(obj);
+    BusState *bus;
+
+    QLIST_FOREACH(bus, &dev->child_bus, sibling) {
+        cb(OBJECT(bus), opaque, type);
+    }
+}
+
 /* can be used as ->unplug() callback for the simple cases */
 void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
                                   DeviceState *dev, Error **errp)
@@ -1084,9 +1106,61 @@ static void device_unparent(Object *obj)
     }
 }
 
+/**
+ * device_phases_reset:
+ * Transition reset method for devices to allow moving
+ * smoothly from legacy reset method to multi-phases
+ */
+static void device_phases_reset(DeviceState *dev)
+{
+    ResettableClass *rc = RESETTABLE_GET_CLASS(dev);
+
+    if (rc->phases.enter) {
+        rc->phases.enter(OBJECT(dev), RESET_TYPE_COLD);
+    }
+    if (rc->phases.hold) {
+        rc->phases.hold(OBJECT(dev));
+    }
+    if (rc->phases.exit) {
+        rc->phases.exit(OBJECT(dev));
+    }
+}
+
+static void device_transitional_reset(Object *obj)
+{
+    DeviceClass *dc = DEVICE_GET_CLASS(obj);
+
+    /*
+     * This will call either @device_phases_reset (for multi-phases transitioned
+     * devices) or a device's specific method for not-yet transitioned devices.
+     * In both case, it does not reset children.
+     */
+    if (dc->reset) {
+        dc->reset(DEVICE(obj));
+    }
+}
+
+/**
+ * device_get_transitional_reset:
+ * check if the device's class is ready for multi-phase
+ */
+static ResettableTrFunction device_get_transitional_reset(Object *obj)
+{
+    DeviceClass *dc = DEVICE_GET_CLASS(obj);
+    if (dc->reset != device_phases_reset) {
+        /*
+         * dc->reset has been overridden by a subclass,
+         * the device is not ready for multi phase yet.
+         */
+        return device_transitional_reset;
+    }
+    return NULL;
+}
+
 static void device_class_init(ObjectClass *class, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(class);
+    ResettableClass *rc = RESETTABLE_CLASS(class);
 
     class->unparent = device_unparent;
 
@@ -1098,6 +1172,25 @@ static void device_class_init(ObjectClass *class, void *data)
      */
     dc->hotpluggable = true;
     dc->user_creatable = true;
+
+    rc->get_state = device_get_reset_state;
+    rc->child_foreach = device_reset_child_foreach;
+
+    /*
+     * @device_phases_reset is put as the default reset method below, allowing
+     * to do the multi-phase transition from base classes to leaf classes. It
+     * allows a legacy-reset Device class to extend a multi-phases-reset
+     * Device class for the following reason:
+     * + If a base class B has been moved to multi-phase, then it does not
+     *   override this default reset method and may have defined phase methods.
+     * + A daughter class C (extending class B) which uses
+     *   device_class_set_parent_reset() (or similar means) to override the
+     *   reset method will still work as expected. @device_phases_reset function
+     *   will be registered as the parent reset method and effectively call
+     *   parent reset phases.
+     */
+    dc->reset = device_phases_reset;
+    rc->get_transitional_function = device_get_transitional_reset;
 }
 
 void device_class_set_parent_reset(DeviceClass *dc,
@@ -1156,6 +1249,10 @@ static const TypeInfo device_type_info = {
     .class_init = device_class_init,
     .abstract = true,
     .class_size = sizeof(DeviceClass),
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_RESETTABLE_INTERFACE },
+        { }
+    },
 };
 
 static void qdev_register_types(void)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 01963984f8..2e3346600e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -5,6 +5,7 @@
 #include "qemu/bitmap.h"
 #include "qom/object.h"
 #include "hw/hotplug.h"
+#include "hw/resettable.h"
 
 enum {
     DEV_NVECTORS_UNSPECIFIED = -1,
@@ -104,6 +105,11 @@ typedef struct DeviceClass {
     bool hotpluggable;
 
     /* callbacks */
+    /*
+     * Reset method here is deprecated and replaced by methods in the
+     * resettable class interface to implement a multi-phase reset.
+     * TODO: remove once every reset callback is unused
+     */
     DeviceReset reset;
     DeviceRealize realize;
     DeviceUnrealize unrealize;
@@ -128,6 +134,7 @@ struct NamedGPIOList {
 /**
  * DeviceState:
  * @realized: Indicates whether the device has been fully constructed.
+ * @reset: ResettableState for the device; handled by Resettable interface.
  *
  * This structure should not be accessed directly.  We declare it here
  * so that it can be embedded in individual device state structures.
@@ -149,6 +156,7 @@ struct DeviceState {
     int num_child_bus;
     int instance_id_alias;
     int alias_required_for_version;
+    ResettableState reset;
 };
 
 struct DeviceListener {
@@ -195,6 +203,7 @@ typedef struct BusChild {
 /**
  * BusState:
  * @hotplug_handler: link to a hotplug handler associated with bus.
+ * @reset: ResettableState for the bus; handled by Resettable interface.
  */
 struct BusState {
     Object obj;
@@ -206,6 +215,7 @@ struct BusState {
     int num_children;
     QTAILQ_HEAD(, BusChild) children;
     QLIST_ENTRY(BusState) sibling;
+    ResettableState reset;
 };
 
 /**
@@ -392,6 +402,18 @@ void qdev_reset_all_fn(void *opaque);
 void qbus_reset_all(BusState *bus);
 void qbus_reset_all_fn(void *opaque);
 
+/**
+ * device_is_in_reset:
+ * Return true if the device @dev is currently being reset.
+ */
+bool device_is_in_reset(DeviceState *dev);
+
+/**
+ * bus_is_in_reset:
+ * Return true if the bus @bus is currently being reset.
+ */
+bool bus_is_in_reset(BusState *bus);
+
 /* This should go away once we get rid of the NULL bus hack */
 BusState *sysbus_get_default(void);
 
@@ -413,6 +435,11 @@ void qdev_machine_init(void);
  */
 void device_legacy_reset(DeviceState *dev);
 
+/**
+ * device_class_set_parent_reset:
+ * TODO: remove the function when DeviceClass's reset method
+ * is not used anymore.
+ */
 void device_class_set_parent_reset(DeviceClass *dc,
                                    DeviceReset dev_reset,
                                    DeviceReset *parent_reset);
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3543451ed3..618ad55401 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -562,6 +562,7 @@ tests/fp/%:
 tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \
 	hw/core/qdev.o hw/core/qdev-properties.o hw/core/hotplug.o\
 	hw/core/bus.o \
+	hw/core/resettable.o \
 	hw/core/irq.o \
 	hw/core/fw-path-provider.o \
 	hw/core/reset.o \
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH v5 05/13] hw/core/resettable: add support for changing parent
  2019-10-18 15:06 [PATCH v5 00/13] Multi-phase reset mechanism Damien Hedde
                   ` (3 preceding siblings ...)
  2019-10-18 15:06 ` [PATCH v5 04/13] hw/core: add Resettable support to BusClass and DeviceClass Damien Hedde
@ 2019-10-18 15:06 ` Damien Hedde
  2019-11-29 18:38   ` Peter Maydell
  2019-10-18 15:06 ` [PATCH v5 06/13] hw/core/qdev: handle parent bus change regarding resettable Damien Hedde
                   ` (11 subsequent siblings)
  16 siblings, 1 reply; 57+ messages in thread
From: Damien Hedde @ 2019-10-18 15:06 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, berrange, ehabkost, cohuck,
	mark.burton, qemu-s390x, edgari, qemu-arm, pbonzini, philmd,
	david

Add a function resettable_change_parent() to do the required
plumbing when changing the parent a of Resettable object.

We need to make sure that the reset state of the object remains
coherent with the reset state of the new parent.

We make the 2 following hypothesis:
+ when an object is put in a parent under reset, the object goes in
reset.
+ when an object is removed from a parent under reset, the object
leaves reset.

The added function avoid any glitch if both old and new parent are
already in reset.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 hw/core/resettable.c    | 54 +++++++++++++++++++++++++++++++++++++++++
 hw/core/trace-events    |  1 +
 include/hw/resettable.h | 16 ++++++++++++
 3 files changed, 71 insertions(+)

diff --git a/hw/core/resettable.c b/hw/core/resettable.c
index c5e11cff4f..60d4285fcc 100644
--- a/hw/core/resettable.c
+++ b/hw/core/resettable.c
@@ -32,6 +32,14 @@ static void resettable_phase_exit(Object *obj, void *opaque, ResetType type);
  */
 static bool enter_phase_in_progress;
 
+/**
+ * exit_phase_in_progress:
+ * Flag telling whether we are currently in an enter phase where side
+ * effects are forbidden. This flag allows us to catch if
+ * resettable_change_parent() is called during exit phase.
+ */
+static unsigned exit_phase_in_progress;
+
 void resettable_reset(Object *obj, ResetType type)
 {
     trace_resettable_reset(obj, type);
@@ -58,7 +66,9 @@ void resettable_release_reset(Object *obj, ResetType type)
     /* TODO: change that assert when adding support for other reset types */
     assert(type == RESET_TYPE_COLD);
     trace_resettable_reset_release_begin(obj, type);
+    exit_phase_in_progress += 1;
     resettable_phase_exit(obj, NULL, type);
+    exit_phase_in_progress -= 1;
     trace_resettable_reset_release_end(obj);
 }
 
@@ -198,6 +208,50 @@ static void resettable_phase_exit(Object *obj, void *opaque, ResetType type)
     trace_resettable_phase_exit_end(obj, obj_typename, s->count);
 }
 
+/*
+ * resettable_get_count:
+ * Get the count of the Resettable object @obj. Return 0 if @obj is NULL.
+ */
+static uint32_t resettable_get_count(Object *obj)
+{
+    if (obj) {
+        ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
+        return rc->get_state(obj)->count;
+    }
+    return 0;
+}
+
+void resettable_change_parent(Object *obj, Object *newp, Object *oldp)
+{
+    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
+    ResettableState *s = rc->get_state(obj);
+    uint32_t newp_count = resettable_get_count(newp);
+    uint32_t oldp_count = resettable_get_count(oldp);
+
+    assert(!enter_phase_in_progress && !exit_phase_in_progress);
+    trace_resettable_change_parent(obj, oldp, oldp_count, newp, newp_count);
+
+    /*
+     * At most one of the two 'for' loop will be executed below
+     * in order to cope with the diff between the two count.
+     */
+    /* if newp is more reset than oldp */
+    for (uint32_t i = oldp_count; i < newp_count; i++) {
+        resettable_assert_reset(obj, RESET_TYPE_COLD);
+    }
+    /*
+     * if obj is leaving a bus under reset, we need to ensure
+     * hold phase is not pending.
+     */
+    if (oldp_count && s->hold_phase_pending) {
+        resettable_phase_hold(obj, NULL, RESET_TYPE_COLD);
+    }
+    /* if oldp is more reset than newp */
+    for (uint32_t i = newp_count; i < oldp_count; i++) {
+        resettable_release_reset(obj, RESET_TYPE_COLD);
+    }
+}
+
 void resettable_class_set_parent_phases(ResettableClass *rc,
                                         ResettableEnterPhase enter,
                                         ResettableHoldPhase hold,
diff --git a/hw/core/trace-events b/hw/core/trace-events
index 66081d0fb4..6d03ef7ff9 100644
--- a/hw/core/trace-events
+++ b/hw/core/trace-events
@@ -16,6 +16,7 @@ resettable_reset_assert_begin(void *obj, int cold) "obj=%p cold=%d"
 resettable_reset_assert_end(void *obj) "obj=%p"
 resettable_reset_release_begin(void *obj, int cold) "obj=%p cold=%d"
 resettable_reset_release_end(void *obj) "obj=%p"
+resettable_change_parent(void *obj, void *o, uint32_t oc, void *n, uint32_t nc) "obj=%p from=%p(%" PRIu32 ") to=%p(%" PRIu32 ")"
 resettable_phase_enter_begin(void *obj, const char *objtype, uint32_t count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
 resettable_phase_enter_exec(void *obj, const char *objtype, int type, int has_method) "obj=%p(%s) type=%d method=%d"
 resettable_phase_enter_end(void *obj, const char *objtype, uint32_t count) "obj=%p(%s) count=%" PRIu32
diff --git a/include/hw/resettable.h b/include/hw/resettable.h
index 6b24e1633b..f6d379669f 100644
--- a/include/hw/resettable.h
+++ b/include/hw/resettable.h
@@ -182,6 +182,22 @@ void resettable_release_reset(Object *obj, ResetType type);
  */
 bool resettable_is_in_reset(Object *obj);
 
+/**
+ * resettable_change_parent:
+ * Indicate that the parent of Ressettable @obj change from @oldp to @newp.
+ * All 3 objects must implements resettable interface. @oldp or @newp may be
+ * NULL.
+ *
+ * This function will adapt the reset state of @obj so that it is coherent
+ * with the reset state of @newp. It may trigger @resettable_assert_reset()
+ * or @resettable_release_reset(). It will do such things only if the reset
+ * state of @newp and @oldp are different.
+ *
+ * When using this function during reset, it must only be called during
+ * an hold phase method. Calling this during enter or exit phase is an error.
+ */
+void resettable_change_parent(Object *obj, Object *newp, Object *oldp);
+
 /**
  * resettable_class_set_parent_phases:
  *
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH v5 06/13] hw/core/qdev: handle parent bus change regarding resettable
  2019-10-18 15:06 [PATCH v5 00/13] Multi-phase reset mechanism Damien Hedde
                   ` (4 preceding siblings ...)
  2019-10-18 15:06 ` [PATCH v5 05/13] hw/core/resettable: add support for changing parent Damien Hedde
@ 2019-10-18 15:06 ` Damien Hedde
  2019-11-29 18:41   ` Peter Maydell
  2019-10-18 15:06 ` [PATCH v5 07/13] hw/core/qdev: update hotplug reset " Damien Hedde
                   ` (10 subsequent siblings)
  16 siblings, 1 reply; 57+ messages in thread
From: Damien Hedde @ 2019-10-18 15:06 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, berrange, ehabkost, cohuck,
	mark.burton, qemu-s390x, edgari, qemu-arm, pbonzini, philmd,
	david

In qdev_set_parent_bus(), when changing the parent bus of a
realized device, if the source and destination buses are not in the
same reset state, some adaptation are required. This patch adds
needed call to resettable_change_parent() to make sure a device reset
state stays coherent with its parent bus.

The addition is a no-op if:
1. the device being parented is not realized.
2. the device is realized, but both buses are not under reset.

Case 2 means that as long as qdev_set_parent_bus() is called
during the machine realization procedure (which is before the
machine reset so nothing is in reset), it is a no op.

There are 49 call sites of qdev_set_parent_bus(). All but one fall
into the no-op case:
+ 28 calls related to virtio (in hw/{s390x,display,virtio}/
  {vhost,virtio}-xxx.c) to set a _vdev_/_vgpu_ composing device
  parent bus just before realizing the _vdev_/_vgpu_.
+ hw/qdev.c: when creating a device in qdev_try_create()
+ hw/sysbus.c: when initializing a device in the sysbus
+ hw/display/virtio-gpu-pci.c: before realizing VirtIOGPUPCIBase/vgpu
+ hw/display/virtio-vga.c: before realizing VirtIOVGABase/vgpu
+ hw/i386/amd_iommu.c: before realizing AMDVIState/pci
+ hw/misc/auxbus.c: when creating an AUXBus
+ hw/misc/auxbus.c: when creating an AUXBus child
+ hw/misc/macio/macio.c: when initializing a MACIOState child
+ hw/misc/macio/macio.c: before realizing NewWorldMacIOState/pmu
+ hw/misc/macio/macio.c: before realizing NewWorldMacIOState/cuda
+ hw/pci-host/designware.c: before realizing DesignwarePCIEHost/root
+ hw/pci-host/gpex.c: before realizing GPEXHost/root
+ hw/pci-host/prep.c: when initializaing PREPPCIState/pci_dev
+ hw/pci-host/q35.c: before realizing Q35PCIHost/mch
+ hw/pci-host/versatile.c: when initializing PCIVPBState/pci_dev
+ hw/pci-host/xilinx-pcie.c: before realizing XilinxPCIEHost/root
+ hw/s390x/event-facility.c: when creating SCLPEventFacility/
                             TYPE_SCLP_QUIESCE
+ hw/s390x/event-facility.c: ditto with SCLPEventFacility/
                             TYPE_SCLP_CPU_HOTPLUG
+ hw/s390x/sclp.c: Not trivial because it is called on a SLCPDevice
  just after realizing it. Ok because at this point the destination
  bus (sysbus) is not in reset; the realize step is before the
  machine reset.
+ hw/sd/core.c: Not OK. Used in sdbus_reparent_card(). See below.
+ hw/ssi/ssi.c: Used to put spi slave on spi bus and connect the cs
  line in ssi_auto_connect_slave(). Ok because this function is only
  used in realize step in hw/ssi/aspeed_smc.ci, hw/ssi/imx_spi.c,
  hw/ssi/mss-spi.c, hw/ssi/xilinx_spi.c and hw/ssi/xilinx_spips.c.
+ hw/xen/xen-legacy-backend.c: when creating a XenLegacyDevice device
+ qdev-monitor.c: in device hotplug creation procedure before realize

Note that this commit alone will have no effect, right now there is no
use of resettable API to reset anything. So a bus will never be tagged
as in-reset by this same API.

The one place where side-effect will occurs is in hw/sd/core.c in
sdbus_reparent_card(). This function is only used in the raspi machines,
including during the sysbus reset procedure. This case will be fixed by
a following commit before globally enabling resettable API for sysbus
reset.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---

Exhaustive list of the 28 virtio caller to qdev_set_parent_bus():
+ hw/display/virtio-gpu-pci.c     -> VirtIOGPUPCIBase/vgpu realize
+ hw/display_virtio-vga.c         -> VirtIOVGABase/vgpu realize
+ hw/s390x/vhost-vsock-ccw.c      -> VHostVSockCCWState/vdev realize
+ hw/s390x/virtio-ccw-9p.c        -> V9fsCCWState/vdev realize
+ hw/s390x/virtio-ccw-balloon.c   -> VirtIOBalloonCcw/vdev realize
+ hw/s390x/virtio-ccw-blk.c       -> VirtIOBlkCcw/vdev realize
+ hw/s390x/virtio-ccw-crypto.c    -> VirtIOCryptoCcw/vdev realize
+ hw/s390x/virtio-ccw-gpu.c       -> VirtIOGPUCcw/vdev realize
+ hw/s390x/virtio-ccw-input.c     -> VirtIOInputCcw/vdev realize
+ hw/s390x/virtio-ccw-net.c       -> VirtIONetCcw/vdev realize
+ hw/s390x/virtio-ccw-rng.c       -> VirtIORNGCcw/vdev realize
+ hw/s390x/virtio-ccw-scsi.c      -> VirtIOSCSICcw/vdev realize
+ hw/s390x/virtio-ccw-scsi.c      -> VHostSCSICcw/vdev realize
+ hw/s390x/virtio-ccw-serial.c    -> VirtioSerialCcw/vdev realize
+ hw/virtio/vhost-scsi-pci.c      -> VHostSCSIPCI/vdev realize
+ hw/virtio/vhost-user-blk-pci.c  -> VHostUserBlkPCI/vdev realize
+ hw/virtio/vhost-user-scsi-pci.c -> VHostUserSCSIPCI/vdev realize
+ hw/virtio/vhost-vsock-pci.c     -> VHostVSockPCI/vdev realize
+ hw/virtio/virtio-9p-pci.c       -> V9fsPCIState/vdev realize
+ hw/virtio/virtio-balloon-pci.c  -> VirtIOBalloonPCI/vdev realize
+ hw/virtio/virtio-blk-pci.c      -> VirtIOBlkPCI/vdev realize
+ hw/virtio/virtio-crypto-pci.c   -> VirtIOCryptoPCI/vdev realize
+ hw/virtio/virtio-input-pci.c    -> VirtIOInputPCI/vdev realize
+ hw/virtio/virtio-net-pci.c      -> VirtIONetPCI/vdev realize
+ hw/virtio/virtio-pmem-pci.c     -> VirtIOPMEMPCI/vdev realize
+ hw/virtio/virtio-rng-pci.c      -> VirtIORngPCI/vdev realize
+ hw/virtio/virtio-scsi-pci.c     -> VirtIOSCSIPCI/vdev realize
+ hw/virtio/virtio-serial-pci.c   -> VirtIOSerialPCI/vdev realize
---
 hw/core/qdev.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 81784c73fb..3933f62d0c 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -96,23 +96,29 @@ static void bus_add_child(BusState *bus, DeviceState *child)
 
 void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
 {
-    bool replugging = dev->parent_bus != NULL;
+    BusState *old_parent_bus = dev->parent_bus;
 
-    if (replugging) {
+    if (old_parent_bus) {
         trace_qdev_update_parent_bus(dev, dev->parent_bus, bus);
         /*
          * Keep a reference to the device while it's not plugged into
          * any bus, to avoid it potentially evaporating when it is
          * dereffed in bus_remove_child().
+         * Also keep the ref of the parent bus until the end, so that
+         * we can safely call resettable_change_parent() below.
          */
         object_ref(OBJECT(dev));
         bus_remove_child(dev->parent_bus, dev);
-        object_unref(OBJECT(dev->parent_bus));
     }
     dev->parent_bus = bus;
     object_ref(OBJECT(bus));
     bus_add_child(bus, dev);
-    if (replugging) {
+    if (dev->realized) {
+        resettable_change_parent(OBJECT(dev), OBJECT(bus),
+                                 OBJECT(old_parent_bus));
+    }
+    if (old_parent_bus) {
+        object_unref(OBJECT(old_parent_bus));
         object_unref(OBJECT(dev));
     }
 }
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH v5 07/13] hw/core/qdev: update hotplug reset regarding resettable
  2019-10-18 15:06 [PATCH v5 00/13] Multi-phase reset mechanism Damien Hedde
                   ` (5 preceding siblings ...)
  2019-10-18 15:06 ` [PATCH v5 06/13] hw/core/qdev: handle parent bus change regarding resettable Damien Hedde
@ 2019-10-18 15:06 ` Damien Hedde
  2019-11-08 15:14   ` Damien Hedde
  2019-10-18 15:06 ` [PATCH v5 08/13] hw/core: deprecate old reset functions and introduce new ones Damien Hedde
                   ` (9 subsequent siblings)
  16 siblings, 1 reply; 57+ messages in thread
From: Damien Hedde @ 2019-10-18 15:06 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, berrange, ehabkost, cohuck,
	mark.burton, qemu-s390x, edgari, qemu-arm, pbonzini, philmd,
	david

This commit make use of the resettable API to reset the device being
hotplugged during when it is realized. Also it make sure it is put in
a reset state coherent with the parent it is plugged into.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---

I'm not sure I've done everything that's required here since I do not
understand everything that's behind the hotplug and realize/unrealize.
I'm a bit lost there...

One of the remaining question is: do we need to do things related to
unrealize ?
It seems, a device can be realized, unrealized, and re-realized ? But
is that true also for a hotplugged device ?

Also resettable API is called there, so children if any are reset too.
This was not the case before, this a probably not a big deal, as long
as all children are realized too at this point. I'm not sure we have a
guarantee on this; the recursive realize is not done in the base bus
class so it will go only down to first buses level if it is not
propagated by bus subclasses. Do hotplug devices can have more than
single level bus subtree (ie: more than some child buses with no
devices on them) ?
---
 hw/core/qdev.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 3933f62d0c..c5d107ea4e 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -930,7 +930,14 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
             }
         }
         if (dev->hotplugged) {
-            device_legacy_reset(dev);
+            /*
+             * Reset the device, as well as its subtree which should be
+             * realized too
+             */
+            resettable_assert_reset(OBJECT(dev), RESET_TYPE_COLD);
+            resettable_change_parent(OBJECT(dev), OBJECT(dev->parent_bus),
+                                     NULL);
+            resettable_release_reset(OBJECT(dev), RESET_TYPE_COLD);
         }
         dev->pending_deleted_event = false;
 
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH v5 08/13] hw/core: deprecate old reset functions and introduce new ones
  2019-10-18 15:06 [PATCH v5 00/13] Multi-phase reset mechanism Damien Hedde
                   ` (6 preceding siblings ...)
  2019-10-18 15:06 ` [PATCH v5 07/13] hw/core/qdev: update hotplug reset " Damien Hedde
@ 2019-10-18 15:06 ` Damien Hedde
  2019-10-31 23:35   ` Philippe Mathieu-Daudé
  2019-11-29 18:42   ` Peter Maydell
  2019-10-18 15:06 ` [PATCH v5 09/13] docs/devel/reset.txt: add doc about Resettable interface Damien Hedde
                   ` (8 subsequent siblings)
  16 siblings, 2 replies; 57+ messages in thread
From: Damien Hedde @ 2019-10-18 15:06 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, berrange, ehabkost, cohuck,
	mark.burton, qemu-s390x, edgari, qemu-arm, pbonzini, philmd,
	david

Deprecate device_legacy_reset(), qdev_reset_all() and
qbus_reset_all() to be replaced by new functions
device_cold_reset() and bus_cold_reset() which uses resettable API.

Also introduce resettable_cold_reset_fn() which may be used as a
replacement for qdev_reset_all_fn and qbus_reset_all_fn().

Following patches will be needed to look at legacy reset call sites
and switch to resettable api. The legacy functions will be removed
when unused.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---

I've removed the general helpers
+ device_reset(DeviceState *dev, ResetType type)
+ bus_reset(BusState *dev, ResetType type)
because with several reset types, all devices and buses will not
implement all of them. I think it is preferable to define a
type-specific helper every time it is needed for base classes
implementing the reset type (eg a device_pci_reset(PciDev*) for the
pci reset type if add that).

So device_reset()/bus_reset() symbol names are not taken anymore. I
don't have a strong opinion of what names should have the
device_cold_reset() and bus_cold_reset() function added in this
patch. It could be device/bus_hard_reset() (the current
qbus_reset_all() comment mention we do a "hard" reset) or simply
device/bus_reset() or anything else.
What do you think ? Any better idea ? It should be consistent with
the reset type name but we can change it also if cold is not what we
want.

Note that if we don't settle for device/bus_reset(). We can drop
the first patch that renames device_reset() to device_legacy_reset()
---
 hw/core/bus.c           |  5 +++++
 hw/core/qdev.c          |  5 +++++
 hw/core/resettable.c    |  5 +++++
 include/hw/qdev-core.h  | 27 +++++++++++++++++++++++++++
 include/hw/resettable.h |  9 +++++++++
 5 files changed, 51 insertions(+)

diff --git a/hw/core/bus.c b/hw/core/bus.c
index 7c05e80db8..5f9e261bb2 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -68,6 +68,11 @@ int qbus_walk_children(BusState *bus,
     return 0;
 }
 
+void bus_cold_reset(BusState *bus)
+{
+    resettable_reset(OBJECT(bus), RESET_TYPE_COLD);
+}
+
 bool bus_is_in_reset(BusState *bus)
 {
     return resettable_is_in_reset(OBJECT(bus));
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index c5d107ea4e..3e6600ce76 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -335,6 +335,11 @@ void qbus_reset_all_fn(void *opaque)
     qbus_reset_all(bus);
 }
 
+void device_cold_reset(DeviceState *dev)
+{
+    resettable_reset(OBJECT(dev), RESET_TYPE_COLD);
+}
+
 bool device_is_in_reset(DeviceState *dev)
 {
     return resettable_is_in_reset(OBJECT(dev));
diff --git a/hw/core/resettable.c b/hw/core/resettable.c
index 60d4285fcc..3d3200bdbc 100644
--- a/hw/core/resettable.c
+++ b/hw/core/resettable.c
@@ -252,6 +252,11 @@ void resettable_change_parent(Object *obj, Object *newp, Object *oldp)
     }
 }
 
+void resettable_cold_reset_fn(void *opaque)
+{
+    resettable_reset((Object *) opaque, RESET_TYPE_COLD);
+}
+
 void resettable_class_set_parent_phases(ResettableClass *rc,
                                         ResettableEnterPhase enter,
                                         ResettableHoldPhase hold,
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 2e3346600e..1e88cb2f6a 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -386,6 +386,13 @@ int qdev_walk_children(DeviceState *dev,
                        qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
                        void *opaque);
 
+/**
+ * @qdev_reset_all:
+ * Reset @dev. See @qbus_reset_all() for more details.
+ *
+ * Note: This function is deprecated and will be removed when it becomes unused.
+ * Please use device_cold_reset() now.
+ */
 void qdev_reset_all(DeviceState *dev);
 void qdev_reset_all_fn(void *opaque);
 
@@ -398,10 +405,28 @@ void qdev_reset_all_fn(void *opaque);
  * hard reset means that qbus_reset_all will reset all state of the device.
  * For PCI devices, for example, this will include the base address registers
  * or configuration space.
+ *
+ * Note: This function is deprecated and will be removed when it becomes unused.
+ * Please use bus_cold_reset() now.
  */
 void qbus_reset_all(BusState *bus);
 void qbus_reset_all_fn(void *opaque);
 
+/**
+ * device_cold_reset:
+ * Reset device @dev and perform a recursive processing using the resettable
+ * interface. It triggers a RESET_TYPE_COLD.
+ */
+void device_cold_reset(DeviceState *dev);
+
+/**
+ * bus_cold_reset:
+ *
+ * Reset bus @bus and perform a recursive processing using the resettable
+ * interface. It triggers a RESET_TYPE_COLD.
+ */
+void bus_cold_reset(BusState *bus);
+
 /**
  * device_is_in_reset:
  * Return true if the device @dev is currently being reset.
@@ -432,6 +457,8 @@ void qdev_machine_init(void);
  * device_legacy_reset:
  *
  * Reset a single device (by calling the reset method).
+ * Note: This function is deprecated and will be removed when it becomes unused.
+ * Please use device_cold_reset() now.
  */
 void device_legacy_reset(DeviceState *dev);
 
diff --git a/include/hw/resettable.h b/include/hw/resettable.h
index f6d379669f..6a9e17344e 100644
--- a/include/hw/resettable.h
+++ b/include/hw/resettable.h
@@ -198,6 +198,15 @@ bool resettable_is_in_reset(Object *obj);
  */
 void resettable_change_parent(Object *obj, Object *newp, Object *oldp);
 
+/**
+ * resettable_cold_reset_fn:
+ * Helper to call resettable_reset((Object *) opaque, RESET_TYPE_COLD).
+ *
+ * This function is typically useful to register a reset handler with
+ * qemu_register_reset.
+ */
+void resettable_cold_reset_fn(void *opaque);
+
 /**
  * resettable_class_set_parent_phases:
  *
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH v5 09/13] docs/devel/reset.txt: add doc about Resettable interface
  2019-10-18 15:06 [PATCH v5 00/13] Multi-phase reset mechanism Damien Hedde
                   ` (7 preceding siblings ...)
  2019-10-18 15:06 ` [PATCH v5 08/13] hw/core: deprecate old reset functions and introduce new ones Damien Hedde
@ 2019-10-18 15:06 ` Damien Hedde
  2019-11-29 19:00   ` Peter Maydell
  2019-10-18 15:06 ` [PATCH v5 10/13] vl: replace deprecated qbus_reset_all registration Damien Hedde
                   ` (7 subsequent siblings)
  16 siblings, 1 reply; 57+ messages in thread
From: Damien Hedde @ 2019-10-18 15:06 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, berrange, ehabkost, cohuck,
	mark.burton, qemu-s390x, edgari, qemu-arm, pbonzini, philmd,
	david

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---

Should I add an entry into index.rst ?
---
 docs/devel/reset.rst | 282 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 282 insertions(+)
 create mode 100644 docs/devel/reset.rst

diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst
new file mode 100644
index 0000000000..a562720f55
--- /dev/null
+++ b/docs/devel/reset.rst
@@ -0,0 +1,282 @@
+
+=======================================
+Reset in QEMU: the Resettable interface
+=======================================
+
+The reset of qemu objects is handled using the resettable interface declared
+in ``include/hw/resettable.h``.
+
+This interface allows to group objects (on a tree basis) and to reset the
+whole group 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.
+
+
+Triggering reset
+----------------
+
+This section documents the APIs which "users" of a resettable object should use
+to control it. All resettable control functions must be called while holding
+the iothread lock.
+
+You can apply a reset to an object using ``resettable_assert_reset()``. You need
+to call ``resettable_release_reset()`` to release the object from reset. To
+instantly reset an object, without keeping it in reset state, just call
+``resettable_reset()``. These functions function takes two parameters: a pointer
+to the object to reset and a reset type.
+
+Several types of reset will be supported. For now only cold reset is defined,
+others may be added later. The Resettable interface handles reset types with an
+enum:
+
+``RESET_TYPE_COLD``
+  Cold reset is supported by every resettable object. In QEMU, it means we reset
+  to the initial state corresponding to the start of QEMU; this might differs
+  from what is a read hardware cold reset. It differs from other reset (like
+  warm or bus resets) which may keep certain parts untouched.
+
+Calling ``resettable_reset()`` is equivalent to calling
+``resettable_assert_reset()`` then ``resettable_release_reset()``. It is
+possible to interleave multiple calls to these three functions. There may
+be several reset sources/controllers of a given object. The interface handles
+everything and the different reset controllers do not need to know anything
+about each others. The object will leave reset state only when each other
+controllers end their reset operation. This point is handled internally by
+maintaining a count of reset; it is crucial to call
+``resettable_release_reset()`` one time and only one time per
+``resettable_assert_reset()`` call.
+
+For now migration of a device or bus in reset is not supported. Care must be
+taken not to delayed ``resettable_release_reset()`` from its
+``resettable_assert_reset()`` counterpart.
+
+Note that, since resettable is an interface, the API takes simple Object as
+parameter. Still, it is a programming error to call a resettable function on a
+non-resettable object and it will trigger a run time assert error. Since most
+call to resettable interface are done through base class functions, such an
+error is not likely to happen.
+
+For Devices and Buses, the following helper functions exists:
+
+- ``device_cold_reset()``
+- ``bus_cold_reset()``
+
+These are simple wrappers around resettable_reset() function; they only cast the
+Device or Bus into an Object and pass the cold reset type. When possible
+prefer to use these functions instead ``resettable_reset()``.
+
+Device and bus functions co-exist because there can be semantic differences
+between resetting a bus and resetting the controller bridge which owns it.
+For example, considering a SCSI controller. Resetting the controller puts all
+its registers back to what reset state was as well as reset everything on the
+SCSI bus. Whereas resetting just the SCSI bus only resets everything that's on
+it but not the controller.
+
+
+Multi-phase mechanism
+---------------------
+
+This section documents the internals of the resettable interface.
+
+The resettable interface uses a multi-phase system to relieve objects and
+machines from reset ordering problems. To address this, the reset operation
+of an object is split into three well defined phases.
+
+When resetting a several objects (for example the whole machine at simulation
+startup), all first phases of all objects are executed, then all second phases
+and then all third phases.
+
+The three phases are:
+
+1. The **enter** phase is executed when the object enters reset. It reset only
+   local state of the object; it must not do anything that has a side-effect
+   on other objects, such as raising or lowering a qemu_irq line or reading or
+   writing guest memory.
+
+2. The **hold** phase is executed for entry into reset, once every object in the
+   group which is being reset has had its *enter* phase executed. At this point
+   devices can do actions that affect other objects.
+
+3. The **exit** phase is executed when the object leaves the reset state.
+   Actions affecting other objects are permitted.
+
+As said in previous section, the interface maintains a count of reset. This
+count is used to ensure phases are executed only when required. *enter* and
+*hold* phases are executed only when asserting reset for the first time
+(if an object is already in reset state when calling
+``resettable_assert_reset()`` or ``resettable_reset()``, they are not
+executed).
+The *exit* phase is executed only when the last reset operation ends. Therefore
+the object has not to care how many reset controllers it has and how many of
+them have started a reset.
+
+
+Handling reset in a resettable object
+-----------------------------------------
+
+This section documents the APIs that an implementation of a resettable object
+must provide and what functions it has access to. It is intended for people
+who want to implement or convert a class which has the resettable interface;
+for example when specializing an existing device or bus.
+
+Methods to implement
+....................
+
+Three methods should be defined or left empty. Each method corresponds to a
+phase of the reset; they are name ``phases.enter()``, ``phases.hold()`` and
+``phases.exit()``. They all take the object as parameter. The *enter* method
+also take the reset type as second parameter.
+
+When extending an existing class, these methods may need to be extended too.
+The ``resettable_class_set_parent_phases()`` class function may be used to
+backup mother class methods.
+
+Here follows an example to implement reset for a Device which set an IO while
+in reset.
+
+::
+
+    static void mydev_reset_enter(Object *obj, ResetType type)
+    {
+        MyDevClass *myclass = MYDEV_GET_CLASS(obj);
+        MyDevState *mydev = MYDEV(obj);
+        /* initialize local state only */
+        mydev->var = 0;
+        /* call mother class enter phase */
+        if (myclass->parent_phases.enter) {
+            myclass->parent_phases.enter(obj, type);
+        }
+    }
+
+    static void mydev_reset_hold(Object *obj)
+    {
+        MyDevClass *myclass = MYDEV_GET_CLASS(obj);
+        MyDevState *mydev = MYDEV(obj);
+        /* set an IO */
+        qemu_set_irq(mydev->irq, 1);
+        /* call mother class hold phase */
+        if (myclass->parent_phases.hold) {
+            myclass->parent_phases.hold(obj);
+        }
+    }
+
+    static void mydev_reset_exit(Object *obj)
+    {
+        MyDevClass *myclass = MYDEV_GET_CLASS(obj);
+        MyDevState *mydev = MYDEV(obj);
+        /* clear an IO */
+        qemu_set_irq(mydev->irq, 0);
+        /* call mother class exit phase */
+        if (myclass->parent_phases.exit) {
+            myclass->parent_phases.exit(obj);
+        }
+    }
+
+    typedef struct MyDevClass {
+        MyMotherClass mother_class;
+        /* to store eventual parent reset methods */
+        ResettablePhases parent_phases;
+    } MyDevClass;
+
+    static void mydev_class_init(ObjectClass *class, void *data)
+    {
+        MyDevClass *myclass = MYDEV_CLASS(class);
+        ResettableClass *rc = RESETTABLE_CLASS(class);
+        resettable_class_set_parent_reset_phases(rc,
+                                                 mydev_reset_enter,
+                                                 mydev_reset_hold,
+                                                 mydev_reset_exit,
+                                                 &myclass->parent_phases);
+    }
+
+In the above example, we override the three phases. It is possible to override
+only part of them by passing NULL instead of a function pointer to
+``resettable_class_set_parent_reset_phases()``. For example, the following will
+only override the *enter* phase and leave *hold* and *exit* untouched::
+
+    resettable_class_set_parent_reset_phases(rc, mydev_reset_enter,
+                                             NULL, NULL,
+                                             &myclass->parent_phases);
+
+
+Polling the reset state
+.......................
+
+Resettable interface provide the ``resettable_is_in_reset()`` function.
+This function tells if the object parameter is currently under reset.
+
+An object is under reset from the beginning of the *init* phase to the end of
+*exit* phase. During all three phases, the function will return that the object
+is in reset.
+
+This function may be used if behavior has to be adapted when receiving external
+interaction while in reset state. Note that until migration of the reset state
+is supported, an object should not be left in reset. So apart from being
+currently executing one of a reset phase, the only cases when this
+function should return true is if an external interaction is made during *hold*
+or *exit* phase of another object in the same reset group.
+
+Helpers ``device_is_in_reset()`` and ``bus_is_in_reset()`` are also provided
+for devices and buses and should be preferred.
+
+
+Base class handling of reset
+----------------------------
+
+This section documents parts of the reset mechanism that you only need to know
+about if you are extending it to work with a new base class other than
+DeviceClass or BusClass, or maintaining the existing code in those classes. Most
+people can ignore it.
+
+Methods to implement
+....................
+
+There are two other methods that need to exist in a class implementing the
+interface: ``get_state()`` and ``child_foreach()``.
+
+``get_state()`` is simple. *resettable* is an interface and, as a consequence,
+does not have any class state structure. But in order to factorize the code, we
+need one. This method must return a pointer to ``ResettableState`` structure.
+The structure must be allocated by the base class; preferably it should be
+located inside the object instance structure.
+
+``child_foreach()`` is more complex. It should execute the given callback on
+every reset child of the given resettable object. All children must be
+resettable too. Additional parameters (a reset type and an opaque pointer) must
+be passed to the callback too.
+
+In ``DeviceClass`` and ``BusClass`` the ``ResettableState`` is located
+``DeviceState`` and ``BusState`` structure. ``child_foreach()`` is implemented
+to follow the bus hierarchy; for a bus, it calls the function on every child
+device; for a device, it calls the function on every bus child. When we reset
+the main system bus, we reset the whole machine bus tree.
+
+Changing a resettable parent
+............................
+
+One thing so should be taken care of by the base class is handling reset
+hierarchy changes.
+
+The reset hierarchy is supposed to be static and built during machine creation.
+But there are actually some exceptions. To cope with this, the resettable API
+provides ``resettable_change_parent()``. This function allows to set, update or
+remove the parent of a resettable object after machine creation is done. As
+parameters, it takes the object being moved, the old parent if any and the new
+parent if any.
+
+This function can be used at anytime when not in a reset operation. During
+a reset operation it must be used only in *hold* phase. Using it in *enter* or
+*exit* phase is an error.
+Also it should not be used during machine creation, although it is harmless to
+do so: the function is a no-op as long as old and new parent are NULL or not
+in reset.
+
+There is currently 2 cases where this function is used:
+
+1. *device hotplug*; it means a new device is introduced on a live bus.
+
+2. *hot bus change*; it means an existing live device is added, moved or
+   removed in the bus hiearchy. At the moment, it occurs only in the raspi
+   machines for chaning the sdbus used by sd card.
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH v5 10/13] vl: replace deprecated qbus_reset_all registration
  2019-10-18 15:06 [PATCH v5 00/13] Multi-phase reset mechanism Damien Hedde
                   ` (8 preceding siblings ...)
  2019-10-18 15:06 ` [PATCH v5 09/13] docs/devel/reset.txt: add doc about Resettable interface Damien Hedde
@ 2019-10-18 15:06 ` Damien Hedde
  2019-11-29 19:01   ` Peter Maydell
  2019-10-18 15:06 ` [PATCH v5 11/13] hw/s390x/ipl: replace deprecated qdev_reset_all registration Damien Hedde
                   ` (6 subsequent siblings)
  16 siblings, 1 reply; 57+ messages in thread
From: Damien Hedde @ 2019-10-18 15:06 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, berrange, ehabkost, cohuck,
	mark.burton, qemu-s390x, edgari, qemu-arm, pbonzini, philmd,
	david

Replace deprecated qbus_reset_all by resettable_cold_reset_fn for
the sysbus reset registration.

Apart for the raspi machines, this does not impact the behavior
because:
+ at this point resettable just call the old reset methods of devices
  and buses in the same order than qdev/qbus.
+ resettable handlers registered with qemu_register_reset are
  serialized; there is no interleaving.
+ eventual explicit calls to legacy reset API (device_reset or
  qdev/qbus_reset) inside this reset handler will not be masked out
  by resettable mechanism; they do not go through resettable api.

For the raspi machines, during the sysbus reset the sd-card is not
reset twice anymore but only once. This is consequence of switching
both sysbus reset and changing parent to resettable; it detects the
second reset is not needed. This has no impact on the state after
reset; the sd-card reset method only reset local state and query
information from the block backend.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---

The raspi reset change can be observed by using the following command
(reset will occurs, then do Ctrl-C to end qemu; no firmware is
given here).
qemu-system-aarch64 -M raspi3 \
    -trace resettable_phase_hold_exec \
    -trace qdev_update_parent_bus \
    -trace resettable_change_parent \
    -trace qdev_reset -trace qbus_reset

Before the patch, the qdev/qbus_reset traces show when reset method are
called. After the patch, the resettable_phase_hold_exec show when reset
method are called.

The traced reset order of the raspi3 is listed below. I've added empty
lines and the tree structure.

 +->bcm2835-peripherals reset
 |
 |       +->sd-card reset
 |   +->sd-bus reset
 +->bcm2835_gpio reset
 |      -> dev_update_parent_bus (move the sd-card on the sdhci-bus)
 |      -> resettable_change_parent
 |
 +->bcm2835-dma reset
 |
 |   +->bcm2835-sdhost-bus reset
 +->bcm2835-sdhost reset
 |
 |       +->sd-card (reset ONLY BEFORE BEFORE THE PATCH)
 |   +->sdhci-bus reset
 +->generic-sdhci reset
 |
 +->bcm2835-rng reset
 +->bcm2835-property reset
 +->bcm2835-fb reset
 +->bcm2835-mbox reset
 +->bcm2835-aux reset
 +->pl011 reset
 +->bcm2835-ic reset
 +->bcm2836-control reset
System reset

In both case, the sd-card is reset (being on bcm2835_gpio/sd-bus) then moved
to generic-sdhci/sdhci-bus by the bcm2835_gpio reset method.

Before the patch, it is then reset again being part of generic-sdhci/sdhci-bus.
After the patch, it considered again for reset but its reset method is not
called because it is already flagged as reset.
---
 vl.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index 4489cfb2bb..186f93e42f 100644
--- a/vl.c
+++ b/vl.c
@@ -4402,7 +4402,15 @@ int main(int argc, char **argv, char **envp)
 
     /* TODO: once all bus devices are qdevified, this should be done
      * when bus is created by qdev.c */
-    qemu_register_reset(qbus_reset_all_fn, sysbus_get_default());
+    /*
+     * 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
+     * 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_run_machine_init_done_notifiers();
 
     if (rom_check_and_register_reset() != 0) {
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH v5 11/13] hw/s390x/ipl: replace deprecated qdev_reset_all registration
  2019-10-18 15:06 [PATCH v5 00/13] Multi-phase reset mechanism Damien Hedde
                   ` (9 preceding siblings ...)
  2019-10-18 15:06 ` [PATCH v5 10/13] vl: replace deprecated qbus_reset_all registration Damien Hedde
@ 2019-10-18 15:06 ` Damien Hedde
  2019-10-31 23:38   ` Philippe Mathieu-Daudé
                     ` (2 more replies)
  2019-10-18 15:06 ` [PATCH v5 12/13] hw/gpio/bcm2835_gpio: Isolate sdbus reparenting Damien Hedde
                   ` (5 subsequent siblings)
  16 siblings, 3 replies; 57+ messages in thread
From: Damien Hedde @ 2019-10-18 15:06 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, Thomas Huth, berrange, ehabkost,
	cohuck, mark.burton, qemu-s390x, Christian Borntraeger, edgari,
	qemu-arm, pbonzini, philmd, david

Replace deprecated qdev_reset_all by resettable_cold_reset_fn for
the ipl registration in the main reset handlers.

This does not impact the behavior for the following reasons:
+ at this point resettable just call the old reset methods of devices
  and buses in the same order than qdev/qbus.
+ resettable handlers registered with qemu_register_reset are
  serialized; there is no interleaving.
+ eventual explicit calls to legacy reset API (device_reset or
  qdev/qbus_reset) inside this reset handler will not be masked out
  by resettable mechanism; they do not go through resettable api.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: qemu-s390x@nongnu.org
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Thomas Huth <thuth@redhat.com>
---
 hw/s390x/ipl.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index ca544d64c5..2689f7a017 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -237,7 +237,15 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
      */
     ipl->compat_start_addr = ipl->start_addr;
     ipl->compat_bios_start_addr = ipl->bios_start_addr;
-    qemu_register_reset(qdev_reset_all_fn, dev);
+    /*
+     * 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_reset(resettable_cold_reset_fn, dev);
 error:
     error_propagate(errp, err);
 }
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH v5 12/13] hw/gpio/bcm2835_gpio: Isolate sdbus reparenting
  2019-10-18 15:06 [PATCH v5 00/13] Multi-phase reset mechanism Damien Hedde
                   ` (10 preceding siblings ...)
  2019-10-18 15:06 ` [PATCH v5 11/13] hw/s390x/ipl: replace deprecated qdev_reset_all registration Damien Hedde
@ 2019-10-18 15:06 ` Damien Hedde
  2019-11-29 19:05   ` Peter Maydell
  2019-10-18 15:06 ` [PATCH v5 13/13] hw/gpio/bcm2835_gpio: Update to resettable Damien Hedde
                   ` (4 subsequent siblings)
  16 siblings, 1 reply; 57+ messages in thread
From: Damien Hedde @ 2019-10-18 15:06 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, berrange, ehabkost, cohuck,
	mark.burton, qemu-s390x, Andrew Baumann, edgari, qemu-arm,
	pbonzini, philmd, david

Split gpfsel_set() in 2 so that the sdbus reparenting is done
in a dedicated function.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Andrew Baumann <Andrew.Baumann@microsoft.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: qemu-arm@nongnu.org
---
 hw/gpio/bcm2835_gpio.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/gpio/bcm2835_gpio.c b/hw/gpio/bcm2835_gpio.c
index 91ce3d10cc..81fe07132f 100644
--- a/hw/gpio/bcm2835_gpio.c
+++ b/hw/gpio/bcm2835_gpio.c
@@ -75,7 +75,10 @@ static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
             s->fsel[index] = fsel;
         }
     }
+}
 
+static void gpfsel_update_sdbus(BCM2835GpioState *s)
+{
     /* SD controller selection (48-53) */
     if (s->sd_fsel != 0
             && (s->fsel[48] == 0) /* SD_CLK_R */
@@ -86,6 +89,7 @@ static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
             && (s->fsel[53] == 0) /* SD_DATA3_R */
             ) {
         /* SDHCI controller selected */
+        sdbus_reparent_card(&s->sdbus, s->sdbus_sdhci);
         sdbus_reparent_card(s->sdbus_sdhost, s->sdbus_sdhci);
         s->sd_fsel = 0;
     } else if (s->sd_fsel != 4
@@ -97,6 +101,7 @@ static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
             && (s->fsel[53] == 4) /* SD_DATA3_R */
             ) {
         /* SDHost controller selected */
+        sdbus_reparent_card(&s->sdbus, s->sdbus_sdhost);
         sdbus_reparent_card(s->sdbus_sdhci, s->sdbus_sdhost);
         s->sd_fsel = 4;
     }
@@ -210,6 +215,7 @@ static void bcm2835_gpio_write(void *opaque, hwaddr offset,
     case GPFSEL4:
     case GPFSEL5:
         gpfsel_set(s, offset / 4, value);
+        gpfsel_update_sdbus(s);
         break;
     case GPSET0:
         gpset(s, value, 0, 32, &s->lev0);
@@ -265,10 +271,12 @@ static void bcm2835_gpio_reset(DeviceState *dev)
         gpfsel_set(s, i, 0);
     }
 
-    s->sd_fsel = 0;
-
-    /* SDHCI is selected by default */
-    sdbus_reparent_card(&s->sdbus, s->sdbus_sdhci);
+    /*
+     * Setup the right sdbus (put 1 in sd_fsel to force reparenting
+     * the sd). It will be SDHCI because of the gpfsel_set() above.
+     */
+    s->sd_fsel = 1;
+    gpfsel_update_sdbus(s);
 
     s->lev0 = 0;
     s->lev1 = 0;
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH v5 13/13] hw/gpio/bcm2835_gpio: Update to resettable
  2019-10-18 15:06 [PATCH v5 00/13] Multi-phase reset mechanism Damien Hedde
                   ` (11 preceding siblings ...)
  2019-10-18 15:06 ` [PATCH v5 12/13] hw/gpio/bcm2835_gpio: Isolate sdbus reparenting Damien Hedde
@ 2019-10-18 15:06 ` Damien Hedde
  2019-11-29 19:02   ` Peter Maydell
  2019-10-19  9:01 ` [PATCH v5 00/13] Multi-phase reset mechanism no-reply
                   ` (3 subsequent siblings)
  16 siblings, 1 reply; 57+ messages in thread
From: Damien Hedde @ 2019-10-18 15:06 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, berrange, ehabkost, cohuck,
	mark.burton, qemu-s390x, Andrew Baumann, edgari, qemu-arm,
	pbonzini, philmd, david

Transition the bcm2835_gpio device class to Resettable.
The sdbus reparenting is delayed in hold phase to respect
resettable side-effect rules.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Andrew Baumann <Andrew.Baumann@microsoft.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: qemu-arm@nongnu.org
---
 hw/gpio/bcm2835_gpio.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/gpio/bcm2835_gpio.c b/hw/gpio/bcm2835_gpio.c
index 81fe07132f..24e39eac5e 100644
--- a/hw/gpio/bcm2835_gpio.c
+++ b/hw/gpio/bcm2835_gpio.c
@@ -262,24 +262,29 @@ err_out:
             __func__, offset);
 }
 
-static void bcm2835_gpio_reset(DeviceState *dev)
+static void bcm2835_gpio_reset_enter(Object *obj, ResetType type)
 {
-    BCM2835GpioState *s = BCM2835_GPIO(dev);
+    BCM2835GpioState *s = BCM2835_GPIO(obj);
 
     int i;
     for (i = 0; i < 6; i++) {
         gpfsel_set(s, i, 0);
     }
 
+    s->lev0 = 0;
+    s->lev1 = 0;
+}
+
+static void bcm2835_gpio_reset_hold(Object *obj)
+{
+    BCM2835GpioState *s = BCM2835_GPIO(obj);
     /*
      * Setup the right sdbus (put 1 in sd_fsel to force reparenting
-     * the sd). It will be SDHCI because of the gpfsel_set() above.
+     * the sd). It will be SDHCI because of the gpfsel_set() done
+     * in bcm2835_gpio_reset_enter().
      */
     s->sd_fsel = 1;
     gpfsel_update_sdbus(s);
-
-    s->lev0 = 0;
-    s->lev1 = 0;
 }
 
 static const MemoryRegionOps bcm2835_gpio_ops = {
@@ -342,10 +347,12 @@ static void bcm2835_gpio_realize(DeviceState *dev, Error **errp)
 static void bcm2835_gpio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
 
     dc->vmsd = &vmstate_bcm2835_gpio;
     dc->realize = &bcm2835_gpio_realize;
-    dc->reset = &bcm2835_gpio_reset;
+    rc->phases.enter = &bcm2835_gpio_reset_enter;
+    rc->phases.hold  = &bcm2835_gpio_reset_hold;
 }
 
 static const TypeInfo bcm2835_gpio_info = {
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 00/13] Multi-phase reset mechanism
  2019-10-18 15:06 [PATCH v5 00/13] Multi-phase reset mechanism Damien Hedde
                   ` (12 preceding siblings ...)
  2019-10-18 15:06 ` [PATCH v5 13/13] hw/gpio/bcm2835_gpio: Update to resettable Damien Hedde
@ 2019-10-19  9:01 ` no-reply
  2019-10-29 15:53 ` Damien Hedde
                   ` (2 subsequent siblings)
  16 siblings, 0 replies; 57+ messages in thread
From: no-reply @ 2019-10-19  9:01 UTC (permalink / raw)
  To: damien.hedde
  Cc: damien.hedde, peter.maydell, berrange, ehabkost, cohuck,
	mark.burton, qemu-devel, qemu-s390x, qemu-arm, pbonzini, philmd,
	edgari, david

Patchew URL: https://patchew.org/QEMU/20191018150630.31099-1-damien.hedde@greensocs.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      hw/virtio/trace.o
  CC      hw/watchdog/trace.o

Warning, treated as error:
/tmp/qemu-test/src/docs/devel/reset.rst:document isn't included in any toctree
  CC      hw/xen/trace.o
  CC      hw/gpio/trace.o
---
  CC      stubs/bdrv-next-monitor-owned.o
  CC      stubs/blk-commit-all.o
  CC      stubs/blockdev-close-all-bdrv-states.o
make: *** [Makefile:994: docs/devel/index.html] Error 2
make: *** Waiting for unfinished jobs....
  CC      stubs/clock-warp.o
Traceback (most recent call last):
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=d026138e19654e3c80d1650d68316bd0', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-if7vrvxd/src/docker-src.2019-10-19-04.59.52.11111:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=d026138e19654e3c80d1650d68316bd0
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-if7vrvxd/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    1m44.422s
user    0m7.664s


The full log is available at
http://patchew.org/logs/20191018150630.31099-1-damien.hedde@greensocs.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 01/13] add device_legacy_reset function to prepare for reset api change
  2019-10-18 15:06 ` [PATCH v5 01/13] add device_legacy_reset function to prepare for reset api change Damien Hedde
@ 2019-10-19 17:35   ` Richard Henderson
  2019-12-03 10:55   ` Cornelia Huck
  1 sibling, 0 replies; 57+ messages in thread
From: Richard Henderson @ 2019-10-19 17:35 UTC (permalink / raw)
  To: Damien Hedde, qemu-devel
  Cc: Fam Zheng, peter.maydell, Collin Walling, Dmitry Fleytman,
	Michael S. Tsirkin, Gerd Hoffmann, David Hildenbrand,
	Halil Pasic, Christian Borntraeger, david, philmd, ehabkost,
	qemu-s390x, qemu-arm, Cédric Le Goater, John Snow,
	Richard Henderson, berrange, cohuck, mark.burton, edgari,
	pbonzini

On 10/18/19 8:06 AM, Damien Hedde wrote:
> Provide a temporary device_legacy_reset function doing what
> device_reset does to prepare for the transition with Resettable
> API.
> 
> All occurrence of device_reset in the code tree are also replaced
> by device_legacy_reset.
> 
> The new resettable API has different prototype and semantics
> (resetting child buses as well as the specified device). Subsequent
> commits will make the changeover for each call site individually; once
> that is complete device_legacy_reset() will be removed.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Acked-by: David Gibson <david@gibson.dropbear.id.au>
> ---

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


r~


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 02/13] hw/core/qdev: add trace events to help with resettable transition
  2019-10-18 15:06 ` [PATCH v5 02/13] hw/core/qdev: add trace events to help with resettable transition Damien Hedde
@ 2019-10-19 17:44   ` Richard Henderson
  2019-10-31 23:23   ` Philippe Mathieu-Daudé
  2019-12-03 10:57   ` Cornelia Huck
  2 siblings, 0 replies; 57+ messages in thread
From: Richard Henderson @ 2019-10-19 17:44 UTC (permalink / raw)
  To: Damien Hedde, qemu-devel
  Cc: peter.maydell, berrange, ehabkost, cohuck, mark.burton, edgari,
	qemu-s390x, qemu-arm, pbonzini, philmd, david

On 10/18/19 8:06 AM, Damien Hedde wrote:
> Adds trace events to reset procedure and when updating the parent
> bus of a device.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>  hw/core/qdev.c       | 27 ++++++++++++++++++++++++---
>  hw/core/trace-events |  9 +++++++++
>  2 files changed, 33 insertions(+), 3 deletions(-)

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


r~



^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 04/13] hw/core: add Resettable support to BusClass and DeviceClass
  2019-10-18 15:06 ` [PATCH v5 04/13] hw/core: add Resettable support to BusClass and DeviceClass Damien Hedde
@ 2019-10-19 18:49   ` Richard Henderson
  2019-11-29 18:36   ` Peter Maydell
  1 sibling, 0 replies; 57+ messages in thread
From: Richard Henderson @ 2019-10-19 18:49 UTC (permalink / raw)
  To: Damien Hedde, qemu-devel
  Cc: peter.maydell, berrange, ehabkost, cohuck, mark.burton, edgari,
	qemu-s390x, qemu-arm, pbonzini, philmd, david

On 10/18/19 8:06 AM, Damien Hedde wrote:
> This commit adds support of Resettable interface to buses and devices:
> + ResettableState structure is added in the Bus/Device state
> + Resettable methods are implemented.
> + device/bus_is_in_reset function defined
> 
> This commit allows to transition the objects to the new
> multi-phase interface without changing the reset behavior at all.
> Object single reset method can be split into the 3 different phases
> but the 3 phases are still executed in a row for a given object.
> From the qdev/qbus reset api point of view, nothing is changed.
> qdev_reset_all() and qbus_reset_all() are not modified as well as
> device_legacy_reset().
> 
> Transition of an object must be done from mother class to daughter
> classes. Care has been taken to allow the transition of a mother class
> without requiring the daughter classes to be transitioned at the same
> time. Note that SysBus and SysBusDevice class do not need any transition
> because they do not override the legacy reset method.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>  hw/core/bus.c          | 97 ++++++++++++++++++++++++++++++++++++++++++
>  hw/core/qdev.c         | 97 ++++++++++++++++++++++++++++++++++++++++++
>  include/hw/qdev-core.h | 27 ++++++++++++
>  tests/Makefile.include |  1 +
>  4 files changed, 222 insertions(+)

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


r~



^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 00/13] Multi-phase reset mechanism
  2019-10-18 15:06 [PATCH v5 00/13] Multi-phase reset mechanism Damien Hedde
                   ` (13 preceding siblings ...)
  2019-10-19  9:01 ` [PATCH v5 00/13] Multi-phase reset mechanism no-reply
@ 2019-10-29 15:53 ` Damien Hedde
  2019-11-08 15:26   ` Damien Hedde
  2019-11-29 19:07 ` Peter Maydell
  2019-12-03 11:44 ` Cornelia Huck
  16 siblings, 1 reply; 57+ messages in thread
From: Damien Hedde @ 2019-10-29 15:53 UTC (permalink / raw)
  To: qemu-devel
  Cc: peter.maydell, berrange, ehabkost, cohuck, mark.burton,
	qemu-s390x, edgari, qemu-arm, pbonzini, philmd, david

Hi,

Does anyone has comment about the interface / patch 3 ?
Should I try to split it ?

Thanks,
Damien

On 10/18/19 5:06 PM, Damien Hedde wrote:
> Hi all,
> 
> The purpose of this series is to split the current reset procedure
> into multiple phases. This will help to solve some ordering
> difficulties we have during reset. Previous version can be found here:
> https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg04359.html
> 
> This series adds resettable interface and transitions base Device and
> Bus classes (sysbus subclasses are ok too). It provides new reset
> functions but does not switch anymore the old functions
> (device_reset() and qdev/qbus_reset_all()) to resettable interface.
> These functions keep the exact same behavior as before.
> 
> The series also transition the main reset handlers registration which
> has no impact until devices and buses are transitioned.
> 
> I think this version is way better regarding the transition from the
> legacy to the resettable interface than the previous one.
> After this series, the plan is then to transition devices, buses and
> legacy reset call sites. Devices and buses have to be transitioned
> from mother class to daughter classes order but until the final
> (daughter) class is transitioned, old monolitic reset behavior will
> be kept for this class.
> 
> Changes v4 -> v5:
>   + various improvement in the resettable interface (regarding
>     transition, robustness and several reset types)
>   + better handling of transition from legacy reset to resettable
>   + device hotplug and parent bus 'hot' change support
>   + improved doc with examples and converted to rst format
> 
> Regarding the Resettable interface changes and how to handle more
> reset types, please read patch 3 message.
> 
> The series is organized as follows:
> Patch 1 is unmodified. Patch 2 adds some utility trace events.
> Patches 3 to 8 adds resettable api in devices and buses. Patch 9 adds
> some documentation. Patches 10 and 11 transition the call sites of
> qemu_register_reset(qdev/qbus_reset_all_fn, ...).
> 
> Apart from patch 7 about hotplug which is really a rfc. I think other
> patches are in pretty good shape.
> Patch 3 and 4 are quite big but I don't think it make much sense to
> split them. I could give it a try if you think it will ease reviews.
> Note that depending on what name we choose for device/bus reset
> functions (see patch 8), we may finally don't need patch 1.
> 
> I've also added patches 12 and 13 which handle the raspi sd card
> reparenting. I'm not sure they fit well in this series but at some
> point in this development I thought they had to be before patch 9
> (finally it is not the case). Since I had to develop some specific
> resettable support just for this case , I kept them as an example
> of what transition a device is. Note that patch 13 handle the only
> reset parent change (tricky) case I found (apart from hotplug).
> 
> Thanks for your feedback,
> Damien
> 
> Damien Hedde (13):
>   add device_legacy_reset function to prepare for reset api change
>   hw/core/qdev: add trace events to help with resettable transition
>   hw/core: create Resettable QOM interface
>   hw/core: add Resettable support to BusClass and DeviceClass
>   hw/core/resettable: add support for changing parent
>   hw/core/qdev: handle parent bus change regarding resettable
>   hw/core/qdev: update hotplug reset regarding resettable
>   hw/core: deprecate old reset functions and introduce new ones
>   docs/devel/reset.txt: add doc about Resettable interface
>   vl: replace deprecated qbus_reset_all registration
>   hw/s390x/ipl: replace deprecated qdev_reset_all registration
>   hw/gpio/bcm2835_gpio: Isolate sdbus reparenting
>   hw/gpio/bcm2835_gpio: Update to resettable
> 
>  Makefile.objs            |   1 +
>  docs/devel/reset.rst     | 282 ++++++++++++++++++++++++++++++++++++++
>  hw/audio/intel-hda.c     |   2 +-
>  hw/core/Makefile.objs    |   1 +
>  hw/core/bus.c            | 102 ++++++++++++++
>  hw/core/qdev.c           | 156 +++++++++++++++++++--
>  hw/core/resettable.c     | 289 +++++++++++++++++++++++++++++++++++++++
>  hw/core/trace-events     |  27 ++++
>  hw/gpio/bcm2835_gpio.c   |  31 +++--
>  hw/hyperv/hyperv.c       |   2 +-
>  hw/i386/pc.c             |   2 +-
>  hw/ide/microdrive.c      |   8 +-
>  hw/intc/spapr_xive.c     |   2 +-
>  hw/ppc/pnv_psi.c         |   2 +-
>  hw/ppc/spapr_pci.c       |   2 +-
>  hw/ppc/spapr_vio.c       |   2 +-
>  hw/s390x/ipl.c           |  10 +-
>  hw/s390x/s390-pci-inst.c |   2 +-
>  hw/scsi/vmw_pvscsi.c     |   2 +-
>  hw/sd/omap_mmc.c         |   2 +-
>  hw/sd/pl181.c            |   2 +-
>  include/hw/qdev-core.h   |  58 +++++++-
>  include/hw/resettable.h  | 224 ++++++++++++++++++++++++++++++
>  tests/Makefile.include   |   1 +
>  vl.c                     |  10 +-
>  25 files changed, 1185 insertions(+), 37 deletions(-)
>  create mode 100644 docs/devel/reset.rst
>  create mode 100644 hw/core/resettable.c
>  create mode 100644 include/hw/resettable.h
> 


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 02/13] hw/core/qdev: add trace events to help with resettable transition
  2019-10-18 15:06 ` [PATCH v5 02/13] hw/core/qdev: add trace events to help with resettable transition Damien Hedde
  2019-10-19 17:44   ` Richard Henderson
@ 2019-10-31 23:23   ` Philippe Mathieu-Daudé
  2019-11-04 12:16     ` Damien Hedde
  2019-12-03 10:57   ` Cornelia Huck
  2 siblings, 1 reply; 57+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-31 23:23 UTC (permalink / raw)
  To: Damien Hedde, qemu-devel
  Cc: peter.maydell, berrange, ehabkost, cohuck, mark.burton,
	qemu-s390x, edgari, qemu-arm, pbonzini, david

On 10/18/19 5:06 PM, Damien Hedde wrote:
> Adds trace events to reset procedure and when updating the parent
> bus of a device.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>   hw/core/qdev.c       | 27 ++++++++++++++++++++++++---
>   hw/core/trace-events |  9 +++++++++
>   2 files changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 60be2f2fef..f230063189 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -38,6 +38,7 @@
>   #include "hw/boards.h"
>   #include "hw/sysbus.h"
>   #include "migration/vmstate.h"
> +#include "trace.h"
>   
>   bool qdev_hotplug = false;
>   static bool qdev_hot_added = false;
> @@ -98,7 +99,9 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
>       bool replugging = dev->parent_bus != NULL;
>   
>       if (replugging) {
> -        /* Keep a reference to the device while it's not plugged into
> +        trace_qdev_update_parent_bus(dev, dev->parent_bus, bus);

Nitpicking, if you respin, can you add object_get_typename(OBJECT(dev))) 
and object_get_typename(OBJECT(bus)))?

With/without it:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +        /*
> +         * Keep a reference to the device while it's not plugged into
>            * any bus, to avoid it potentially evaporating when it is
>            * dereffed in bus_remove_child().
>            */
> @@ -272,6 +275,18 @@ HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
>       return hotplug_ctrl;
>   }
>   
> +static int qdev_prereset(DeviceState *dev, void *opaque)
> +{
> +    trace_qdev_reset_tree(dev, object_get_typename(OBJECT(dev)));
> +    return 0;
> +}
> +
> +static int qbus_prereset(BusState *bus, void *opaque)
> +{
> +    trace_qbus_reset_tree(bus, object_get_typename(OBJECT(bus)));
> +    return 0;
> +}
> +
>   static int qdev_reset_one(DeviceState *dev, void *opaque)
>   {
>       device_legacy_reset(dev);
> @@ -282,6 +297,7 @@ static int qdev_reset_one(DeviceState *dev, void *opaque)
>   static int qbus_reset_one(BusState *bus, void *opaque)
>   {
>       BusClass *bc = BUS_GET_CLASS(bus);
> +    trace_qbus_reset(bus, object_get_typename(OBJECT(bus)));
>       if (bc->reset) {
>           bc->reset(bus);
>       }
> @@ -290,7 +306,9 @@ static int qbus_reset_one(BusState *bus, void *opaque)
>   
>   void qdev_reset_all(DeviceState *dev)
>   {
> -    qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL);
> +    trace_qdev_reset_all(dev, object_get_typename(OBJECT(dev)));
> +    qdev_walk_children(dev, qdev_prereset, qbus_prereset,
> +                       qdev_reset_one, qbus_reset_one, NULL);
>   }
>   
>   void qdev_reset_all_fn(void *opaque)
> @@ -300,7 +318,9 @@ void qdev_reset_all_fn(void *opaque)
>   
>   void qbus_reset_all(BusState *bus)
>   {
> -    qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL);
> +    trace_qbus_reset_all(bus, object_get_typename(OBJECT(bus)));
> +    qbus_walk_children(bus, qdev_prereset, qbus_prereset,
> +                       qdev_reset_one, qbus_reset_one, NULL);
>   }
>   
>   void qbus_reset_all_fn(void *opaque)
> @@ -1108,6 +1128,7 @@ void device_legacy_reset(DeviceState *dev)
>   {
>       DeviceClass *klass = DEVICE_GET_CLASS(dev);
>   
> +    trace_qdev_reset(dev, object_get_typename(OBJECT(dev)));
>       if (klass->reset) {
>           klass->reset(dev);
>       }
> diff --git a/hw/core/trace-events b/hw/core/trace-events
> index fe47a9c8cb..1a669be0ea 100644
> --- a/hw/core/trace-events
> +++ b/hw/core/trace-events
> @@ -1,2 +1,11 @@
>   # loader.c
>   loader_write_rom(const char *name, uint64_t gpa, uint64_t size, bool isrom) "%s: @0x%"PRIx64" size=0x%"PRIx64" ROM=%d"
> +
> +# qdev.c
> +qdev_reset(void *obj, const char *objtype) "obj=%p(%s)"
> +qdev_reset_all(void *obj, const char *objtype) "obj=%p(%s)"
> +qdev_reset_tree(void *obj, const char *objtype) "obj=%p(%s)"
> +qbus_reset(void *obj, const char *objtype) "obj=%p(%s)"
> +qbus_reset_all(void *obj, const char *objtype) "obj=%p(%s)"
> +qbus_reset_tree(void *obj, const char *objtype) "obj=%p(%s)"
> +qdev_update_parent_bus(void *obj, void *oldp, void *newp) "obj=%p old_parent=%p new_parent=%p"
> 


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 08/13] hw/core: deprecate old reset functions and introduce new ones
  2019-10-18 15:06 ` [PATCH v5 08/13] hw/core: deprecate old reset functions and introduce new ones Damien Hedde
@ 2019-10-31 23:35   ` Philippe Mathieu-Daudé
  2019-11-04 12:01     ` Damien Hedde
  2019-11-29 18:42   ` Peter Maydell
  1 sibling, 1 reply; 57+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-31 23:35 UTC (permalink / raw)
  To: Damien Hedde, qemu-devel
  Cc: peter.maydell, berrange, ehabkost, cohuck, mark.burton,
	qemu-s390x, edgari, qemu-arm, pbonzini, david

On 10/18/19 5:06 PM, Damien Hedde wrote:
> Deprecate device_legacy_reset(), qdev_reset_all() and
> qbus_reset_all() to be replaced by new functions
> device_cold_reset() and bus_cold_reset() which uses resettable API.
> 
> Also introduce resettable_cold_reset_fn() which may be used as a
> replacement for qdev_reset_all_fn and qbus_reset_all_fn().
> 
> Following patches will be needed to look at legacy reset call sites
> and switch to resettable api. The legacy functions will be removed
> when unused.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> 
> I've removed the general helpers
> + device_reset(DeviceState *dev, ResetType type)
> + bus_reset(BusState *dev, ResetType type)
> because with several reset types, all devices and buses will not
> implement all of them. I think it is preferable to define a
> type-specific helper every time it is needed for base classes
> implementing the reset type (eg a device_pci_reset(PciDev*) for the
> pci reset type if add that).

Agreed.

> So device_reset()/bus_reset() symbol names are not taken anymore. I
> don't have a strong opinion of what names should have the
> device_cold_reset() and bus_cold_reset() function added in this
> patch. It could be device/bus_hard_reset() (the current
> qbus_reset_all() comment mention we do a "hard" reset) or simply
> device/bus_reset() or anything else.
> What do you think ? Any better idea ? It should be consistent with
> the reset type name but we can change it also if cold is not what we
> want.

Cold/hot are self-illustrative so seems fine. Hard/soft are used in 
electronic but less in software I'd say.

> Note that if we don't settle for device/bus_reset(). We can drop
> the first patch that renames device_reset() to device_legacy_reset()
> ---
>   hw/core/bus.c           |  5 +++++
>   hw/core/qdev.c          |  5 +++++
>   hw/core/resettable.c    |  5 +++++
>   include/hw/qdev-core.h  | 27 +++++++++++++++++++++++++++
>   include/hw/resettable.h |  9 +++++++++
>   5 files changed, 51 insertions(+)
> 
> diff --git a/hw/core/bus.c b/hw/core/bus.c
> index 7c05e80db8..5f9e261bb2 100644
> --- a/hw/core/bus.c
> +++ b/hw/core/bus.c
> @@ -68,6 +68,11 @@ int qbus_walk_children(BusState *bus,
>       return 0;
>   }
>   
> +void bus_cold_reset(BusState *bus)
> +{
> +    resettable_reset(OBJECT(bus), RESET_TYPE_COLD);
> +}
> +
>   bool bus_is_in_reset(BusState *bus)
>   {
>       return resettable_is_in_reset(OBJECT(bus));
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index c5d107ea4e..3e6600ce76 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -335,6 +335,11 @@ void qbus_reset_all_fn(void *opaque)
>       qbus_reset_all(bus);
>   }
>   
> +void device_cold_reset(DeviceState *dev)
> +{
> +    resettable_reset(OBJECT(dev), RESET_TYPE_COLD);
> +}
> +
>   bool device_is_in_reset(DeviceState *dev)
>   {
>       return resettable_is_in_reset(OBJECT(dev));
> diff --git a/hw/core/resettable.c b/hw/core/resettable.c
> index 60d4285fcc..3d3200bdbc 100644
> --- a/hw/core/resettable.c
> +++ b/hw/core/resettable.c
> @@ -252,6 +252,11 @@ void resettable_change_parent(Object *obj, Object *newp, Object *oldp)
>       }
>   }
>   
> +void resettable_cold_reset_fn(void *opaque)
> +{
> +    resettable_reset((Object *) opaque, RESET_TYPE_COLD);

Why not take a Object* argument?

> +}
> +
>   void resettable_class_set_parent_phases(ResettableClass *rc,
>                                           ResettableEnterPhase enter,
>                                           ResettableHoldPhase hold,
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 2e3346600e..1e88cb2f6a 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -386,6 +386,13 @@ int qdev_walk_children(DeviceState *dev,
>                          qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
>                          void *opaque);
>   
> +/**
> + * @qdev_reset_all:
> + * Reset @dev. See @qbus_reset_all() for more details.
> + *
> + * Note: This function is deprecated and will be removed when it becomes unused.
> + * Please use device_cold_reset() now.
> + */
>   void qdev_reset_all(DeviceState *dev);
>   void qdev_reset_all_fn(void *opaque);
>   
> @@ -398,10 +405,28 @@ void qdev_reset_all_fn(void *opaque);
>    * hard reset means that qbus_reset_all will reset all state of the device.
>    * For PCI devices, for example, this will include the base address registers
>    * or configuration space.
> + *
> + * Note: This function is deprecated and will be removed when it becomes unused.
> + * Please use bus_cold_reset() now.
>    */
>   void qbus_reset_all(BusState *bus);
>   void qbus_reset_all_fn(void *opaque);
>   
> +/**
> + * device_cold_reset:
> + * Reset device @dev and perform a recursive processing using the resettable
> + * interface. It triggers a RESET_TYPE_COLD.
> + */
> +void device_cold_reset(DeviceState *dev);
> +
> +/**
> + * bus_cold_reset:
> + *
> + * Reset bus @bus and perform a recursive processing using the resettable
> + * interface. It triggers a RESET_TYPE_COLD.
> + */
> +void bus_cold_reset(BusState *bus);
> +
>   /**
>    * device_is_in_reset:
>    * Return true if the device @dev is currently being reset.
> @@ -432,6 +457,8 @@ void qdev_machine_init(void);
>    * device_legacy_reset:
>    *
>    * Reset a single device (by calling the reset method).
> + * Note: This function is deprecated and will be removed when it becomes unused.
> + * Please use device_cold_reset() now.
>    */
>   void device_legacy_reset(DeviceState *dev);
>   
> diff --git a/include/hw/resettable.h b/include/hw/resettable.h
> index f6d379669f..6a9e17344e 100644
> --- a/include/hw/resettable.h
> +++ b/include/hw/resettable.h
> @@ -198,6 +198,15 @@ bool resettable_is_in_reset(Object *obj);
>    */
>   void resettable_change_parent(Object *obj, Object *newp, Object *oldp);
>   
> +/**
> + * resettable_cold_reset_fn:
> + * Helper to call resettable_reset((Object *) opaque, RESET_TYPE_COLD).
> + *
> + * This function is typically useful to register a reset handler with
> + * qemu_register_reset.
> + */
> +void resettable_cold_reset_fn(void *opaque);
> +
>   /**
>    * resettable_class_set_parent_phases:
>    *
> 


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 11/13] hw/s390x/ipl: replace deprecated qdev_reset_all registration
  2019-10-18 15:06 ` [PATCH v5 11/13] hw/s390x/ipl: replace deprecated qdev_reset_all registration Damien Hedde
@ 2019-10-31 23:38   ` Philippe Mathieu-Daudé
  2019-11-29 19:02   ` Peter Maydell
  2019-12-03 11:41   ` Cornelia Huck
  2 siblings, 0 replies; 57+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-31 23:38 UTC (permalink / raw)
  To: Damien Hedde, qemu-devel
  Cc: peter.maydell, Thomas Huth, berrange, ehabkost, cohuck,
	mark.burton, qemu-s390x, Christian Borntraeger, edgari, qemu-arm,
	pbonzini, david

On 10/18/19 5:06 PM, Damien Hedde wrote:
> Replace deprecated qdev_reset_all by resettable_cold_reset_fn for
> the ipl registration in the main reset handlers.
> 
> This does not impact the behavior for the following reasons:
> + at this point resettable just call the old reset methods of devices
>    and buses in the same order than qdev/qbus.
> + resettable handlers registered with qemu_register_reset are
>    serialized; there is no interleaving.
> + eventual explicit calls to legacy reset API (device_reset or
>    qdev/qbus_reset) inside this reset handler will not be masked out
>    by resettable mechanism; they do not go through resettable api.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: qemu-s390x@nongnu.org
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Thomas Huth <thuth@redhat.com>
> ---
>   hw/s390x/ipl.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index ca544d64c5..2689f7a017 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -237,7 +237,15 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>        */
>       ipl->compat_start_addr = ipl->start_addr;
>       ipl->compat_bios_start_addr = ipl->bios_start_addr;
> -    qemu_register_reset(qdev_reset_all_fn, dev);
> +    /*
> +     * 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_reset(resettable_cold_reset_fn, dev);

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>   error:
>       error_propagate(errp, err);
>   }
> 


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 08/13] hw/core: deprecate old reset functions and introduce new ones
  2019-10-31 23:35   ` Philippe Mathieu-Daudé
@ 2019-11-04 12:01     ` Damien Hedde
  2019-11-04 15:42       ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 57+ messages in thread
From: Damien Hedde @ 2019-11-04 12:01 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: peter.maydell, berrange, ehabkost, cohuck, mark.burton,
	qemu-s390x, edgari, qemu-arm, pbonzini, david



On 11/1/19 12:35 AM, Philippe Mathieu-Daudé wrote:
> On 10/18/19 5:06 PM, Damien Hedde wrote:
>> Deprecate device_legacy_reset(), qdev_reset_all() and
>> qbus_reset_all() to be replaced by new functions
>> device_cold_reset() and bus_cold_reset() which uses resettable API.
>>
>> Also introduce resettable_cold_reset_fn() which may be used as a
>> replacement for qdev_reset_all_fn and qbus_reset_all_fn().
>>
>> Following patches will be needed to look at legacy reset call sites
>> and switch to resettable api. The legacy functions will be removed
>> when unused.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>> [...]>>   +void resettable_cold_reset_fn(void *opaque)
>> +{
>> +    resettable_reset((Object *) opaque, RESET_TYPE_COLD);
> 
> Why not take a Object* argument?

This function is used to register a reset callback with
qemu_register_reset() (path 10 and 11), so we need void* to match the
prototype.




^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 02/13] hw/core/qdev: add trace events to help with resettable transition
  2019-10-31 23:23   ` Philippe Mathieu-Daudé
@ 2019-11-04 12:16     ` Damien Hedde
  2019-11-04 14:33       ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 57+ messages in thread
From: Damien Hedde @ 2019-11-04 12:16 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: peter.maydell, berrange, ehabkost, cohuck, mark.burton,
	qemu-s390x, edgari, qemu-arm, pbonzini, david


On 11/1/19 12:23 AM, Philippe Mathieu-Daudé wrote:
> On 10/18/19 5:06 PM, Damien Hedde wrote:
>> Adds trace events to reset procedure and when updating the parent
>> bus of a device.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>>   hw/core/qdev.c       | 27 ++++++++++++++++++++++++---
>>   hw/core/trace-events |  9 +++++++++
>>   2 files changed, 33 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 60be2f2fef..f230063189 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -38,6 +38,7 @@
>>   #include "hw/boards.h"
>>   #include "hw/sysbus.h"
>>   #include "migration/vmstate.h"
>> +#include "trace.h"
>>     bool qdev_hotplug = false;
>>   static bool qdev_hot_added = false;
>> @@ -98,7 +99,9 @@ void qdev_set_parent_bus(DeviceState *dev, BusState
>> *bus)
>>       bool replugging = dev->parent_bus != NULL;
>>         if (replugging) {
>> -        /* Keep a reference to the device while it's not plugged into
>> +        trace_qdev_update_parent_bus(dev, dev->parent_bus, bus);
> 
> Nitpicking, if you respin, can you add object_get_typename(OBJECT(dev)))
> and object_get_typename(OBJECT(bus)))?

sure. I was wondering if having some kind of qom object support to trace
would be feasible. Because it's a bit tedious to add this each time. And
IMO it would be more useful to have the path, but we can't reasonably
compute it as a trace_..() arguments.

> 
> With/without it:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
>> +        /*
>> +         * Keep a reference to the device while it's not plugged into
>>            * any bus, to avoid it potentially evaporating when it is
>>            * dereffed in bus_remove_child().
>>            */
>> @@ -272,6 +275,18 @@ HotplugHandler
>> *qdev_get_hotplug_handler(DeviceState *dev)
>>       return hotplug_ctrl;
>>   }
>>   +static int qdev_prereset(DeviceState *dev, void *opaque)
>> +{
>> +    trace_qdev_reset_tree(dev, object_get_typename(OBJECT(dev)));
>> +    return 0;
>> +}
>> +
>> +static int qbus_prereset(BusState *bus, void *opaque)
>> +{
>> +    trace_qbus_reset_tree(bus, object_get_typename(OBJECT(bus)));
>> +    return 0;
>> +}
>> +
>>   static int qdev_reset_one(DeviceState *dev, void *opaque)
>>   {
>>       device_legacy_reset(dev);
>> @@ -282,6 +297,7 @@ static int qdev_reset_one(DeviceState *dev, void
>> *opaque)
>>   static int qbus_reset_one(BusState *bus, void *opaque)
>>   {
>>       BusClass *bc = BUS_GET_CLASS(bus);
>> +    trace_qbus_reset(bus, object_get_typename(OBJECT(bus)));
>>       if (bc->reset) {
>>           bc->reset(bus);
>>       }
>> @@ -290,7 +306,9 @@ static int qbus_reset_one(BusState *bus, void
>> *opaque)
>>     void qdev_reset_all(DeviceState *dev)
>>   {
>> -    qdev_walk_children(dev, NULL, NULL, qdev_reset_one,
>> qbus_reset_one, NULL);
>> +    trace_qdev_reset_all(dev, object_get_typename(OBJECT(dev)));
>> +    qdev_walk_children(dev, qdev_prereset, qbus_prereset,
>> +                       qdev_reset_one, qbus_reset_one, NULL);
>>   }
>>     void qdev_reset_all_fn(void *opaque)
>> @@ -300,7 +318,9 @@ void qdev_reset_all_fn(void *opaque)
>>     void qbus_reset_all(BusState *bus)
>>   {
>> -    qbus_walk_children(bus, NULL, NULL, qdev_reset_one,
>> qbus_reset_one, NULL);
>> +    trace_qbus_reset_all(bus, object_get_typename(OBJECT(bus)));
>> +    qbus_walk_children(bus, qdev_prereset, qbus_prereset,
>> +                       qdev_reset_one, qbus_reset_one, NULL);
>>   }
>>     void qbus_reset_all_fn(void *opaque)
>> @@ -1108,6 +1128,7 @@ void device_legacy_reset(DeviceState *dev)
>>   {
>>       DeviceClass *klass = DEVICE_GET_CLASS(dev);
>>   +    trace_qdev_reset(dev, object_get_typename(OBJECT(dev)));
>>       if (klass->reset) {
>>           klass->reset(dev);
>>       }
>> diff --git a/hw/core/trace-events b/hw/core/trace-events
>> index fe47a9c8cb..1a669be0ea 100644
>> --- a/hw/core/trace-events
>> +++ b/hw/core/trace-events
>> @@ -1,2 +1,11 @@
>>   # loader.c
>>   loader_write_rom(const char *name, uint64_t gpa, uint64_t size, bool
>> isrom) "%s: @0x%"PRIx64" size=0x%"PRIx64" ROM=%d"
>> +
>> +# qdev.c
>> +qdev_reset(void *obj, const char *objtype) "obj=%p(%s)"
>> +qdev_reset_all(void *obj, const char *objtype) "obj=%p(%s)"
>> +qdev_reset_tree(void *obj, const char *objtype) "obj=%p(%s)"
>> +qbus_reset(void *obj, const char *objtype) "obj=%p(%s)"
>> +qbus_reset_all(void *obj, const char *objtype) "obj=%p(%s)"
>> +qbus_reset_tree(void *obj, const char *objtype) "obj=%p(%s)"
>> +qdev_update_parent_bus(void *obj, void *oldp, void *newp) "obj=%p
>> old_parent=%p new_parent=%p"
>>


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 02/13] hw/core/qdev: add trace events to help with resettable transition
  2019-11-04 12:16     ` Damien Hedde
@ 2019-11-04 14:33       ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 57+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-11-04 14:33 UTC (permalink / raw)
  To: Damien Hedde, qemu-devel
  Cc: peter.maydell, berrange, ehabkost, cohuck, mark.burton,
	qemu-s390x, edgari, qemu-arm, pbonzini, david

On 11/4/19 1:16 PM, Damien Hedde wrote:
> On 11/1/19 12:23 AM, Philippe Mathieu-Daudé wrote:
>> On 10/18/19 5:06 PM, Damien Hedde wrote:
>>> Adds trace events to reset procedure and when updating the parent
>>> bus of a device.
>>>
>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>> ---
>>>    hw/core/qdev.c       | 27 ++++++++++++++++++++++++---
>>>    hw/core/trace-events |  9 +++++++++
>>>    2 files changed, 33 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>> index 60be2f2fef..f230063189 100644
>>> --- a/hw/core/qdev.c
>>> +++ b/hw/core/qdev.c
>>> @@ -38,6 +38,7 @@
>>>    #include "hw/boards.h"
>>>    #include "hw/sysbus.h"
>>>    #include "migration/vmstate.h"
>>> +#include "trace.h"
>>>      bool qdev_hotplug = false;
>>>    static bool qdev_hot_added = false;
>>> @@ -98,7 +99,9 @@ void qdev_set_parent_bus(DeviceState *dev, BusState
>>> *bus)
>>>        bool replugging = dev->parent_bus != NULL;
>>>          if (replugging) {
>>> -        /* Keep a reference to the device while it's not plugged into
>>> +        trace_qdev_update_parent_bus(dev, dev->parent_bus, bus);
>>
>> Nitpicking, if you respin, can you add object_get_typename(OBJECT(dev)))
>> and object_get_typename(OBJECT(bus)))?
> 
> sure. I was wondering if having some kind of qom object support to trace
> would be feasible. Because it's a bit tedious to add this each time. And
> IMO it would be more useful to have the path, but we can't reasonably
> compute it as a trace_..() arguments.

Meanwhile you can use:

   if (trace_event_get_state_backends(TRACE_QDEV_UPDATE_PARENT_BUS)) {
       ...

>>
>> With/without it:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>>> +        /*
>>> +         * Keep a reference to the device while it's not plugged into
>>>             * any bus, to avoid it potentially evaporating when it is
>>>             * dereffed in bus_remove_child().
>>>             */
>>> @@ -272,6 +275,18 @@ HotplugHandler
>>> *qdev_get_hotplug_handler(DeviceState *dev)
>>>        return hotplug_ctrl;
>>>    }
>>>    +static int qdev_prereset(DeviceState *dev, void *opaque)
>>> +{
>>> +    trace_qdev_reset_tree(dev, object_get_typename(OBJECT(dev)));
>>> +    return 0;
>>> +}
>>> +
>>> +static int qbus_prereset(BusState *bus, void *opaque)
>>> +{
>>> +    trace_qbus_reset_tree(bus, object_get_typename(OBJECT(bus)));
>>> +    return 0;
>>> +}
>>> +
>>>    static int qdev_reset_one(DeviceState *dev, void *opaque)
>>>    {
>>>        device_legacy_reset(dev);
>>> @@ -282,6 +297,7 @@ static int qdev_reset_one(DeviceState *dev, void
>>> *opaque)
>>>    static int qbus_reset_one(BusState *bus, void *opaque)
>>>    {
>>>        BusClass *bc = BUS_GET_CLASS(bus);
>>> +    trace_qbus_reset(bus, object_get_typename(OBJECT(bus)));
>>>        if (bc->reset) {
>>>            bc->reset(bus);
>>>        }
>>> @@ -290,7 +306,9 @@ static int qbus_reset_one(BusState *bus, void
>>> *opaque)
>>>      void qdev_reset_all(DeviceState *dev)
>>>    {
>>> -    qdev_walk_children(dev, NULL, NULL, qdev_reset_one,
>>> qbus_reset_one, NULL);
>>> +    trace_qdev_reset_all(dev, object_get_typename(OBJECT(dev)));
>>> +    qdev_walk_children(dev, qdev_prereset, qbus_prereset,
>>> +                       qdev_reset_one, qbus_reset_one, NULL);
>>>    }
>>>      void qdev_reset_all_fn(void *opaque)
>>> @@ -300,7 +318,9 @@ void qdev_reset_all_fn(void *opaque)
>>>      void qbus_reset_all(BusState *bus)
>>>    {
>>> -    qbus_walk_children(bus, NULL, NULL, qdev_reset_one,
>>> qbus_reset_one, NULL);
>>> +    trace_qbus_reset_all(bus, object_get_typename(OBJECT(bus)));
>>> +    qbus_walk_children(bus, qdev_prereset, qbus_prereset,
>>> +                       qdev_reset_one, qbus_reset_one, NULL);
>>>    }
>>>      void qbus_reset_all_fn(void *opaque)
>>> @@ -1108,6 +1128,7 @@ void device_legacy_reset(DeviceState *dev)
>>>    {
>>>        DeviceClass *klass = DEVICE_GET_CLASS(dev);
>>>    +    trace_qdev_reset(dev, object_get_typename(OBJECT(dev)));
>>>        if (klass->reset) {
>>>            klass->reset(dev);
>>>        }
>>> diff --git a/hw/core/trace-events b/hw/core/trace-events
>>> index fe47a9c8cb..1a669be0ea 100644
>>> --- a/hw/core/trace-events
>>> +++ b/hw/core/trace-events
>>> @@ -1,2 +1,11 @@
>>>    # loader.c
>>>    loader_write_rom(const char *name, uint64_t gpa, uint64_t size, bool
>>> isrom) "%s: @0x%"PRIx64" size=0x%"PRIx64" ROM=%d"
>>> +
>>> +# qdev.c
>>> +qdev_reset(void *obj, const char *objtype) "obj=%p(%s)"
>>> +qdev_reset_all(void *obj, const char *objtype) "obj=%p(%s)"
>>> +qdev_reset_tree(void *obj, const char *objtype) "obj=%p(%s)"
>>> +qbus_reset(void *obj, const char *objtype) "obj=%p(%s)"
>>> +qbus_reset_all(void *obj, const char *objtype) "obj=%p(%s)"
>>> +qbus_reset_tree(void *obj, const char *objtype) "obj=%p(%s)"
>>> +qdev_update_parent_bus(void *obj, void *oldp, void *newp) "obj=%p
>>> old_parent=%p new_parent=%p"
>>>


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 08/13] hw/core: deprecate old reset functions and introduce new ones
  2019-11-04 12:01     ` Damien Hedde
@ 2019-11-04 15:42       ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 57+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-11-04 15:42 UTC (permalink / raw)
  To: Damien Hedde, qemu-devel
  Cc: peter.maydell, berrange, ehabkost, cohuck, mark.burton,
	qemu-s390x, edgari, qemu-arm, pbonzini, david

On 11/4/19 1:01 PM, Damien Hedde wrote:
> On 11/1/19 12:35 AM, Philippe Mathieu-Daudé wrote:
>> On 10/18/19 5:06 PM, Damien Hedde wrote:
>>> Deprecate device_legacy_reset(), qdev_reset_all() and
>>> qbus_reset_all() to be replaced by new functions
>>> device_cold_reset() and bus_cold_reset() which uses resettable API.
>>>
>>> Also introduce resettable_cold_reset_fn() which may be used as a
>>> replacement for qdev_reset_all_fn and qbus_reset_all_fn().
>>>
>>> Following patches will be needed to look at legacy reset call sites
>>> and switch to resettable api. The legacy functions will be removed
>>> when unused.
>>>
>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>> ---
>>> [...]>>   +void resettable_cold_reset_fn(void *opaque)
>>> +{
>>> +    resettable_reset((Object *) opaque, RESET_TYPE_COLD);
>>
>> Why not take a Object* argument?
> 
> This function is used to register a reset callback with
> qemu_register_reset() (path 10 and 11), so we need void* to match the
> prototype.

Alright.
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 07/13] hw/core/qdev: update hotplug reset regarding resettable
  2019-10-18 15:06 ` [PATCH v5 07/13] hw/core/qdev: update hotplug reset " Damien Hedde
@ 2019-11-08 15:14   ` Damien Hedde
  0 siblings, 0 replies; 57+ messages in thread
From: Damien Hedde @ 2019-11-08 15:14 UTC (permalink / raw)
  To: qemu-devel
  Cc: peter.maydell, berrange, ehabkost, cohuck, mark.burton,
	qemu-s390x, edgari, qemu-arm, pbonzini, philmd, david


On 10/18/19 5:06 PM, Damien Hedde wrote:
> This commit make use of the resettable API to reset the device being
> hotplugged during when it is realized. Also it make sure it is put in
> a reset state coherent with the parent it is plugged into.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> 
> I'm not sure I've done everything that's required here since I do not
> understand everything that's behind the hotplug and realize/unrealize.
> I'm a bit lost there...
> 
> One of the remaining question is: do we need to do things related to
> unrealize ?
> It seems, a device can be realized, unrealized, and re-realized ? But
> is that true also for a hotplugged device ?>
> Also resettable API is called there, so children if any are reset too.
> This was not the case before, this a probably not a big deal, as long
> as all children are realized too at this point. I'm not sure we have a
> guarantee on this; the recursive realize is not done in the base bus
> class so it will go only down to first buses level if it is not
> propagated by bus subclasses. Do hotplug devices can have more than
> single level bus subtree (ie: more than some child buses with no
> devices on them) ?
> ---
>  hw/core/qdev.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 3933f62d0c..c5d107ea4e 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -930,7 +930,14 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>              }
>          }
>          if (dev->hotplugged) {

Just thinking that if it is possible to have un-realize then realize
cycle, we probably need some code here (or even before realizing
children) to ensure the reset state is zeroed before doing the following.

> -            device_legacy_reset(dev);
> +            /*
> +             * Reset the device, as well as its subtree which should be
> +             * realized too
> +             */
> +            resettable_assert_reset(OBJECT(dev), RESET_TYPE_COLD);
> +            resettable_change_parent(OBJECT(dev), OBJECT(dev->parent_bus),
> +                                     NULL);
> +            resettable_release_reset(OBJECT(dev), RESET_TYPE_COLD);
>          }
>          dev->pending_deleted_event = false;
>  
>

--
Damien



^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 00/13] Multi-phase reset mechanism
  2019-10-29 15:53 ` Damien Hedde
@ 2019-11-08 15:26   ` Damien Hedde
  2019-11-08 15:28     ` Peter Maydell
  0 siblings, 1 reply; 57+ messages in thread
From: Damien Hedde @ 2019-11-08 15:26 UTC (permalink / raw)
  To: qemu-devel
  Cc: peter.maydell, berrange, ehabkost, cohuck, mark.burton,
	qemu-s390x, edgari, qemu-arm, pbonzini, philmd, david


On 10/29/19 4:53 PM, Damien Hedde wrote:
> Hi,
> 
> Does anyone has comment about the interface / patch 3 ?
> Should I try to split it ?

ping

> 
> Thanks,
> Damien
> 

Thanks,
Damien


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 00/13] Multi-phase reset mechanism
  2019-11-08 15:26   ` Damien Hedde
@ 2019-11-08 15:28     ` Peter Maydell
  2019-11-08 15:58       ` Damien Hedde
  0 siblings, 1 reply; 57+ messages in thread
From: Peter Maydell @ 2019-11-08 15:28 UTC (permalink / raw)
  To: Damien Hedde
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Edgar Iglesias, qemu-arm,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

On Fri, 8 Nov 2019 at 15:26, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
>
> On 10/29/19 4:53 PM, Damien Hedde wrote:
> > Hi,
> >
> > Does anyone has comment about the interface / patch 3 ?
> > Should I try to split it ?
>
> ping

Hi; this patchset is still in my to-review queue, but we've
just gone into softfreeze for 4.2 so I'm a bit short on time
to look at anything that's not for this release.

I do definitely want to get this patchset in early in the
5.0 release cycle though.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 00/13] Multi-phase reset mechanism
  2019-11-08 15:28     ` Peter Maydell
@ 2019-11-08 15:58       ` Damien Hedde
  0 siblings, 0 replies; 57+ messages in thread
From: Damien Hedde @ 2019-11-08 15:58 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Edgar Iglesias, qemu-arm,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson


On 11/8/19 4:28 PM, Peter Maydell wrote:
> On Fri, 8 Nov 2019 at 15:26, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>>
>> On 10/29/19 4:53 PM, Damien Hedde wrote:
>>> Hi,
>>>
>>> Does anyone has comment about the interface / patch 3 ?
>>> Should I try to split it ?
>>
>> ping
> 
> Hi; this patchset is still in my to-review queue, but we've
> just gone into softfreeze for 4.2 so I'm a bit short on time
> to look at anything that's not for this release.
> 
> I do definitely want to get this patchset in early in the
> 5.0 release cycle though.

Hi,
I understand,
Then I'll maybe try to advance more on the multiple reset type handling
and a do v6.

Thanks,
Damien


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 03/13] hw/core: create Resettable QOM interface
  2019-10-18 15:06 ` [PATCH v5 03/13] hw/core: create Resettable QOM interface Damien Hedde
@ 2019-11-29 18:32   ` Peter Maydell
  2019-12-02 11:07     ` Damien Hedde
  2019-12-03 11:16   ` Cornelia Huck
  1 sibling, 1 reply; 57+ messages in thread
From: Peter Maydell @ 2019-11-29 18:32 UTC (permalink / raw)
  To: Damien Hedde
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Edgar Iglesias, qemu-arm,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> This commit defines an interface allowing multi-phase reset. This aims
> to solve a problem of the actual single-phase reset (built in
> DeviceClass and BusClass): reset behavior is dependent on the order
> in which reset handlers are called. In particular doing external
> side-effect (like setting an qemu_irq) is problematic because receiving
> object may not be reset yet.
>
> The Resettable interface divides the reset in 3 well defined phases.
> To reset an object tree, all 1st phases are executed then all 2nd then
> all 3rd. See the comments in include/hw/resettable.h for a more complete
> description. The interface defines 3 phases to let the future
> possibility of holding an object into reset for some time.
>
> The qdev/qbus reset in DeviceClass and BusClass will be modified in
> following commits to use this interface. A mechanism is provided
> to allow executing a transitional reset handler in place of the 2nd
> phase which is executed in children-then-parent order inside a tree.
> This will allow to transition devices and buses smoothly while
> keeping the exact current qdev/qbus reset behavior for now.
>
> Documentation will be added in a following commit.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>
> In this patch only a single reset type is supported, but the interface
> allows for more to be defined.
>
> I had some thought about problems which may arise when having multiple
> reset types:

[snip]

Yeah, these all seem right. We clearly need to think a bit
more before we add multiple reset types. Let's get this basic
just-cold-reset in for now and come back to the rest later.


Almost all of my comments below are just grammar/typo fixes
for comments. The only substantives are:
 * globals
 * copyright/licensing comment needed in the .h file
and they're pretty minor items.

> +/**
> + * enter_phase_in_progress:
> + * Flag telling whether we are currently in an enter phase where side
> + * effects are forbidden. This flag allows us to catch if reset is called
> + * again during during this phase.
> + */
> +static bool enter_phase_in_progress;

This looks weird -- I don't think a global for this works,
because you might have several distinct subtrees of
devices, and be doing reset on them both at once.
I think that we only use this for an assert, though -- is
that right? If so, we could just drop this.

> +void resettable_assert_reset(Object *obj, ResetType type)
> +{
> +    assert(!enter_phase_in_progress);
> +    /* TODO: change that assert when adding support for other reset types */

I'm not sure which assert this is referring to -- the one above
the comment, or the one below ?

> +    assert(type == RESET_TYPE_COLD);
> +    trace_resettable_reset_assert_begin(obj, type);
> +    enter_phase_in_progress = true;
> +    resettable_phase_enter(obj, NULL, type);
> +    enter_phase_in_progress = false;
> +    resettable_phase_hold(obj, NULL, type);
> +    trace_resettable_reset_assert_end(obj);
> +}
> +
> +void resettable_release_reset(Object *obj, ResetType type)
> +{
> +    assert(!enter_phase_in_progress);
> +    /* TODO: change that assert when adding support for other reset types */

Ditto.

> +    assert(type == RESET_TYPE_COLD);
> +    trace_resettable_reset_release_begin(obj, type);
> +    resettable_phase_exit(obj, NULL, type);
> +    trace_resettable_reset_release_end(obj);
> +}
> +

> +static void resettable_phase_enter(Object *obj, void *opaque, ResetType type)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResettableState *s = rc->get_state(obj);
> +    const char *obj_typename = object_get_typename(obj);
> +    bool action_needed = false;
> +
> +    /* exit phase has to finish properly before entering back in reset */
> +    assert(!s->exit_phase_in_progress);
> +
> +    trace_resettable_phase_enter_begin(obj, obj_typename, s->count, type);
> +
> +    /* Only take action if we really enter reset for the 1st time. */
> +    /*
> +     * TODO: if adding more ResetType support, some additional checks
> +     * are probably needed here.
> +     */
> +    if (s->count++ == 0) {
> +        action_needed = true;
> +    }
> +    /*
> +     * We limit the count to an arbitrary "big" value. The value is big
> +     * enough not to be triggered nominally.

"normally"

> +     * The assert will stop an infinite loop if there is a cycle in the
> +     * reset tree. The loop goes through resettable_foreach_child below
> +     * which at some point will call us again.
> +     */
> +    assert(s->count <= 50);
> +
> +    /*
> +     * handle the children even if action_needed is at false so that
> +     * children counts are incremented too

"child counts"

> +     */
> +    resettable_child_foreach(rc, obj, resettable_phase_enter, NULL, type);
> +
> +    /* execute enter phase for the object if needed */
> +    if (action_needed) {
> +        trace_resettable_phase_enter_exec(obj, obj_typename, type,
> +                                          !!rc->phases.enter);
> +        if (rc->phases.enter && !resettable_get_tr_func(rc, obj)) {
> +            rc->phases.enter(obj, type);
> +        }
> +        s->hold_phase_pending = true;
> +    }
> +    trace_resettable_phase_enter_end(obj, obj_typename, s->count);
> +}
> +

> +static void resettable_phase_exit(Object *obj, void *opaque, ResetType type)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResettableState *s = rc->get_state(obj);
> +    const char *obj_typename = object_get_typename(obj);
> +
> +    assert(!s->exit_phase_in_progress);
> +    trace_resettable_phase_exit_begin(obj, obj_typename, s->count, type);
> +
> +    /* exit_phase_in_progress ensure this phase is 'atomic' */

"ensures"

> +    s->exit_phase_in_progress = true;
> +    resettable_child_foreach(rc, obj, resettable_phase_exit, NULL, type);
> +
> +    assert(s->count > 0);
> +    if (s->count == 1) {
> +        trace_resettable_phase_exit_exec(obj, obj_typename, !!rc->phases.exit);
> +        if (rc->phases.exit && !resettable_get_tr_func(rc, obj)) {
> +            rc->phases.exit(obj);
> +        }
> +        s->count = 0;
> +    }
> +    s->exit_phase_in_progress = false;
> +    trace_resettable_phase_exit_end(obj, obj_typename, s->count);
> +}


> --- /dev/null
> +++ b/include/hw/resettable.h
> @@ -0,0 +1,199 @@
> +#ifndef HW_RESETTABLE_H
> +#define HW_RESETTABLE_H
> +

All new files, including even small header files, should have
the usual copyright-and-license comment at the top. (Can you
check also whether this needs adding for any other new files the
patchset creates, please?)

> +#include "qom/object.h"
> +
> +#define TYPE_RESETTABLE_INTERFACE "resettable"
> +
> +#define RESETTABLE_CLASS(class) \
> +    OBJECT_CLASS_CHECK(ResettableClass, (class), TYPE_RESETTABLE_INTERFACE)
> +
> +#define RESETTABLE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE_INTERFACE)
> +
> +typedef struct ResettableState ResettableState;
> +
> +/**
> + * ResetType:
> + * Types of reset.
> + *
> + * + Cold: reset resulting from a power cycle of the object.
> + *
> + * TODO: Support has to be added to handle more types. In particular,
> + * ResettableState structure needs to be expanded.
> + */
> +typedef enum ResetType {
> +    RESET_TYPE_COLD,
> +} ResetType;

> + * @get_transitional_function: transitional method to handle Resettable objects
> + * not yet fully moved to this interface. It will be removed as soon as is not

"as soon as it is"

> + * needed anymore. This method is optional and may return a pointer to a
> + * function to be used instead of the phases. In case the method exists and

"If the method exists"

> + * returns a non-NULL function pointer; it is executed as a replacement of the

"pointer then that function is"

> + * 'hold' phase method taking the object as argument. The two other phase
> + * methods are not executed.
> + *
> + * @child_foreach: Executes a given callback on every Resettable child. Child
> + * in this context means a child in the qbus tree, so the children of a qbus
> + * are the devices on it, and the children of a device are all the buses it
> + * owns. This is not the same as the QOM object hierarchy. The function takes
> + * additional opaque and ResetType arguments which must be passed unmodified to
> + * the callback.
> + */

> +    /* Transitional method for legacy reset compatibility */
> +    ResettableGetTrFunction get_transitional_function;
> +
> +    /* Hiearchy handling method */

"Hierarchy"

> +    ResettableChildForeach child_foreach;
> +} ResettableClass;
> +typedef struct ResettablePhases ResettablePhases;
> +
> +/**
> + * ResettableState:
> + * Structure holding reset related state. The fields should not be accessed
> + * directly, the definition is here to allow further inclusion into other

"directly; the definition"


> + * objects.
> + *
> + * @count: Number of reset level the object is into. It is incremented when
> + * the reset operation starts and decremented when it finishes.
> + * @hold_phase_pending: flag which indicates that we need to invoke the 'hold'
> + * phase handler for this object.
> + * @exit_phase_in_progress: flag telling if currently in exit phase

"@exit_phase_in_progress: true if we are currently in the exit phase"

> + */
> +struct ResettableState {
> +    uint32_t count;
> +    bool hold_phase_pending;
> +    bool exit_phase_in_progress;
> +};
> +
> +/**
> + * resettable_reset:
> + * Trigger a reset on a object @obj of type @type. @obj must implement

"an object"

> + * Resettable interface.
> + *
> + * Calling this function is equivalent to calling @resettable_assert_reset()
> + * then @resettable_release_reset().
> + */
> +void resettable_reset(Object *obj, ResetType type);
> +
> +/**
> + * resettable_assert_reset:
> + * Put an object @obj into reset. @obj must implement Resettable interface.
> + *
> + * @resettable_release_reset() must eventually be called after this call.
> + * There must be one call to @resettable_release_reset() per call of
> + * @resettable_assert_reset(), with the same type argument.
> + *
> + * NOTE: Until support for migration is added, the @resettable_release_reset()
> + * must not be delayed. It have to occur just after @resettable_assert_reset()

"It must occur"

> + * so that migration cannot be triggered in between. Prefer using
> + * @resettable_reset() for now.
> + */
> +void resettable_assert_reset(Object *obj, ResetType type);
> +
> +/**
> + * resettable_release_reset:
> + * Release the object @obj from reset. @obj must implement Resettable interface.
> + *
> + * See @resettable_assert_reset() description for details.
> + */
> +void resettable_release_reset(Object *obj, ResetType type);
> +
> +/**
> + * resettable_is_in_reset:
> + * Return true if @obj is under reset.
> + *
> + * @obj must implement Resettable interface.
> + */
> +bool resettable_is_in_reset(Object *obj);
> +
> +/**
> + * resettable_class_set_parent_phases:
> + *
> + * Save @rc current reset phases into @parent_phases and override @rc phases
> + * by the given new methods (@enter, @hold and @exit).
> + * Each phase is overridden only if the new one is not NULL allowing to
> + * override a subset of phases.
> + */
> +void resettable_class_set_parent_phases(ResettableClass *rc,
> +                                        ResettableEnterPhase enter,
> +                                        ResettableHoldPhase hold,
> +                                        ResettableExitPhase exit,
> +                                        ResettablePhases *parent_phases);
> +
> +#endif

thanks
-- PMM


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 04/13] hw/core: add Resettable support to BusClass and DeviceClass
  2019-10-18 15:06 ` [PATCH v5 04/13] hw/core: add Resettable support to BusClass and DeviceClass Damien Hedde
  2019-10-19 18:49   ` Richard Henderson
@ 2019-11-29 18:36   ` Peter Maydell
  2019-12-02 11:38     ` Damien Hedde
  1 sibling, 1 reply; 57+ messages in thread
From: Peter Maydell @ 2019-11-29 18:36 UTC (permalink / raw)
  To: Damien Hedde
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Edgar Iglesias, qemu-arm,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> This commit adds support of Resettable interface to buses and devices:
> + ResettableState structure is added in the Bus/Device state
> + Resettable methods are implemented.
> + device/bus_is_in_reset function defined
>
> This commit allows to transition the objects to the new
> multi-phase interface without changing the reset behavior at all.
> Object single reset method can be split into the 3 different phases
> but the 3 phases are still executed in a row for a given object.
> From the qdev/qbus reset api point of view, nothing is changed.
> qdev_reset_all() and qbus_reset_all() are not modified as well as
> device_legacy_reset().
>
> Transition of an object must be done from mother class to daughter
> classes.

The standard terminology here is "parent class" and "child class".

I notice you've used mother/daughter in a bunch of other comments
in various patches -- could you change that to the more usual
terms, please?

> Care has been taken to allow the transition of a mother class
> without requiring the daughter classes to be transitioned at the same
> time. Note that SysBus and SysBusDevice class do not need any transition
> because they do not override the legacy reset method.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>

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

thanks
-- PMM


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 05/13] hw/core/resettable: add support for changing parent
  2019-10-18 15:06 ` [PATCH v5 05/13] hw/core/resettable: add support for changing parent Damien Hedde
@ 2019-11-29 18:38   ` Peter Maydell
  2019-12-02 11:43     ` Damien Hedde
  0 siblings, 1 reply; 57+ messages in thread
From: Peter Maydell @ 2019-11-29 18:38 UTC (permalink / raw)
  To: Damien Hedde
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Edgar Iglesias, qemu-arm,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Add a function resettable_change_parent() to do the required
> plumbing when changing the parent a of Resettable object.
>
> We need to make sure that the reset state of the object remains
> coherent with the reset state of the new parent.
>
> We make the 2 following hypothesis:
> + when an object is put in a parent under reset, the object goes in
> reset.
> + when an object is removed from a parent under reset, the object
> leaves reset.
>
> The added function avoid any glitch if both old and new parent are
> already in reset.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>  hw/core/resettable.c    | 54 +++++++++++++++++++++++++++++++++++++++++
>  hw/core/trace-events    |  1 +
>  include/hw/resettable.h | 16 ++++++++++++
>  3 files changed, 71 insertions(+)
>
> diff --git a/hw/core/resettable.c b/hw/core/resettable.c
> index c5e11cff4f..60d4285fcc 100644
> --- a/hw/core/resettable.c
> +++ b/hw/core/resettable.c
> @@ -32,6 +32,14 @@ static void resettable_phase_exit(Object *obj, void *opaque, ResetType type);
>   */
>  static bool enter_phase_in_progress;
>
> +/**
> + * exit_phase_in_progress:
> + * Flag telling whether we are currently in an enter phase where side
> + * effects are forbidden. This flag allows us to catch if
> + * resettable_change_parent() is called during exit phase.
> + */
> +static unsigned exit_phase_in_progress;

This is another global that I don't think we should have.
Is it also just for asserts ?


> +
>  void resettable_reset(Object *obj, ResetType type)
>  {
>      trace_resettable_reset(obj, type);
> @@ -58,7 +66,9 @@ void resettable_release_reset(Object *obj, ResetType type)
>      /* TODO: change that assert when adding support for other reset types */
>      assert(type == RESET_TYPE_COLD);
>      trace_resettable_reset_release_begin(obj, type);
> +    exit_phase_in_progress += 1;
>      resettable_phase_exit(obj, NULL, type);
> +    exit_phase_in_progress -= 1;
>      trace_resettable_reset_release_end(obj);
>  }
>
> @@ -198,6 +208,50 @@ static void resettable_phase_exit(Object *obj, void *opaque, ResetType type)
>      trace_resettable_phase_exit_end(obj, obj_typename, s->count);
>  }
>
> +/*
> + * resettable_get_count:
> + * Get the count of the Resettable object @obj. Return 0 if @obj is NULL.
> + */
> +static uint32_t resettable_get_count(Object *obj)
> +{
> +    if (obj) {
> +        ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +        return rc->get_state(obj)->count;
> +    }
> +    return 0;
> +}
> +
> +void resettable_change_parent(Object *obj, Object *newp, Object *oldp)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResettableState *s = rc->get_state(obj);
> +    uint32_t newp_count = resettable_get_count(newp);
> +    uint32_t oldp_count = resettable_get_count(oldp);
> +
> +    assert(!enter_phase_in_progress && !exit_phase_in_progress);
> +    trace_resettable_change_parent(obj, oldp, oldp_count, newp, newp_count);
> +
> +    /*
> +     * At most one of the two 'for' loop will be executed below

"loops"

> +     * in order to cope with the diff between the two count.

"difference". "counts".

> +     */
> +    /* if newp is more reset than oldp */
> +    for (uint32_t i = oldp_count; i < newp_count; i++) {
> +        resettable_assert_reset(obj, RESET_TYPE_COLD);
> +    }
> +    /*
> +     * if obj is leaving a bus under reset, we need to ensure
> +     * hold phase is not pending.
> +     */
> +    if (oldp_count && s->hold_phase_pending) {
> +        resettable_phase_hold(obj, NULL, RESET_TYPE_COLD);
> +    }
> +    /* if oldp is more reset than newp */
> +    for (uint32_t i = newp_count; i < oldp_count; i++) {
> +        resettable_release_reset(obj, RESET_TYPE_COLD);
> +    }
> +}
> +
>  void resettable_class_set_parent_phases(ResettableClass *rc,
>                                          ResettableEnterPhase enter,
>                                          ResettableHoldPhase hold,
> diff --git a/hw/core/trace-events b/hw/core/trace-events
> index 66081d0fb4..6d03ef7ff9 100644
> --- a/hw/core/trace-events
> +++ b/hw/core/trace-events
> @@ -16,6 +16,7 @@ resettable_reset_assert_begin(void *obj, int cold) "obj=%p cold=%d"
>  resettable_reset_assert_end(void *obj) "obj=%p"
>  resettable_reset_release_begin(void *obj, int cold) "obj=%p cold=%d"
>  resettable_reset_release_end(void *obj) "obj=%p"
> +resettable_change_parent(void *obj, void *o, uint32_t oc, void *n, uint32_t nc) "obj=%p from=%p(%" PRIu32 ") to=%p(%" PRIu32 ")"
>  resettable_phase_enter_begin(void *obj, const char *objtype, uint32_t count, int type) "obj=%p(%s) count=%" PRIu32 " type=%d"
>  resettable_phase_enter_exec(void *obj, const char *objtype, int type, int has_method) "obj=%p(%s) type=%d method=%d"
>  resettable_phase_enter_end(void *obj, const char *objtype, uint32_t count) "obj=%p(%s) count=%" PRIu32
> diff --git a/include/hw/resettable.h b/include/hw/resettable.h
> index 6b24e1633b..f6d379669f 100644
> --- a/include/hw/resettable.h
> +++ b/include/hw/resettable.h
> @@ -182,6 +182,22 @@ void resettable_release_reset(Object *obj, ResetType type);
>   */
>  bool resettable_is_in_reset(Object *obj);
>
> +/**
> + * resettable_change_parent:
> + * Indicate that the parent of Ressettable @obj change from @oldp to @newp.

"is changing from"

> + * All 3 objects must implements resettable interface. @oldp or @newp may be

"implement"

> + * NULL.
> + *
> + * This function will adapt the reset state of @obj so that it is coherent
> + * with the reset state of @newp. It may trigger @resettable_assert_reset()
> + * or @resettable_release_reset(). It will do such things only if the reset
> + * state of @newp and @oldp are different.
> + *
> + * When using this function during reset, it must only be called during
> + * an hold phase method. Calling this during enter or exit phase is an error.

"a hold phase"

> + */
> +void resettable_change_parent(Object *obj, Object *newp, Object *oldp);
> +

thanks
-- PMM


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 06/13] hw/core/qdev: handle parent bus change regarding resettable
  2019-10-18 15:06 ` [PATCH v5 06/13] hw/core/qdev: handle parent bus change regarding resettable Damien Hedde
@ 2019-11-29 18:41   ` Peter Maydell
  2019-12-03 11:37     ` Cornelia Huck
  0 siblings, 1 reply; 57+ messages in thread
From: Peter Maydell @ 2019-11-29 18:41 UTC (permalink / raw)
  To: Damien Hedde
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Edgar Iglesias, qemu-arm,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> In qdev_set_parent_bus(), when changing the parent bus of a
> realized device, if the source and destination buses are not in the
> same reset state, some adaptation are required. This patch adds

"adaptations"

> needed call to resettable_change_parent() to make sure a device reset
> state stays coherent with its parent bus.
>
> The addition is a no-op if:
> 1. the device being parented is not realized.
> 2. the device is realized, but both buses are not under reset.
>
> Case 2 means that as long as qdev_set_parent_bus() is called
> during the machine realization procedure (which is before the
> machine reset so nothing is in reset), it is a no op.
>
> There are 49 call sites of qdev_set_parent_bus(). All but one fall
> into the no-op case:
> + 28 calls related to virtio (in hw/{s390x,display,virtio}/
>   {vhost,virtio}-xxx.c) to set a _vdev_/_vgpu_ composing device
>   parent bus just before realizing the _vdev_/_vgpu_.
> + hw/qdev.c: when creating a device in qdev_try_create()
> + hw/sysbus.c: when initializing a device in the sysbus
> + hw/display/virtio-gpu-pci.c: before realizing VirtIOGPUPCIBase/vgpu
> + hw/display/virtio-vga.c: before realizing VirtIOVGABase/vgpu
> + hw/i386/amd_iommu.c: before realizing AMDVIState/pci
> + hw/misc/auxbus.c: when creating an AUXBus
> + hw/misc/auxbus.c: when creating an AUXBus child
> + hw/misc/macio/macio.c: when initializing a MACIOState child
> + hw/misc/macio/macio.c: before realizing NewWorldMacIOState/pmu
> + hw/misc/macio/macio.c: before realizing NewWorldMacIOState/cuda
> + hw/pci-host/designware.c: before realizing DesignwarePCIEHost/root
> + hw/pci-host/gpex.c: before realizing GPEXHost/root
> + hw/pci-host/prep.c: when initializaing PREPPCIState/pci_dev
> + hw/pci-host/q35.c: before realizing Q35PCIHost/mch
> + hw/pci-host/versatile.c: when initializing PCIVPBState/pci_dev
> + hw/pci-host/xilinx-pcie.c: before realizing XilinxPCIEHost/root
> + hw/s390x/event-facility.c: when creating SCLPEventFacility/
>                              TYPE_SCLP_QUIESCE
> + hw/s390x/event-facility.c: ditto with SCLPEventFacility/
>                              TYPE_SCLP_CPU_HOTPLUG
> + hw/s390x/sclp.c: Not trivial because it is called on a SLCPDevice
>   just after realizing it. Ok because at this point the destination
>   bus (sysbus) is not in reset; the realize step is before the
>   machine reset.
> + hw/sd/core.c: Not OK. Used in sdbus_reparent_card(). See below.
> + hw/ssi/ssi.c: Used to put spi slave on spi bus and connect the cs
>   line in ssi_auto_connect_slave(). Ok because this function is only
>   used in realize step in hw/ssi/aspeed_smc.ci, hw/ssi/imx_spi.c,
>   hw/ssi/mss-spi.c, hw/ssi/xilinx_spi.c and hw/ssi/xilinx_spips.c.
> + hw/xen/xen-legacy-backend.c: when creating a XenLegacyDevice device
> + qdev-monitor.c: in device hotplug creation procedure before realize

This is a really useful analysis to have in the commit message;
thanks!

(Side note: I wonder if the sclp.c case could be reordered so
it realizes the device after parenting it? Anyway, not something
to worry about now.)

> Note that this commit alone will have no effect, right now there is no
> use of resettable API to reset anything. So a bus will never be tagged
> as in-reset by this same API.
>
> The one place where side-effect will occurs is in hw/sd/core.c in
> sdbus_reparent_card(). This function is only used in the raspi machines,
> including during the sysbus reset procedure. This case will be fixed by
> a following commit before globally enabling resettable API for sysbus
> reset.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>

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

thanks
-- PMM


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 08/13] hw/core: deprecate old reset functions and introduce new ones
  2019-10-18 15:06 ` [PATCH v5 08/13] hw/core: deprecate old reset functions and introduce new ones Damien Hedde
  2019-10-31 23:35   ` Philippe Mathieu-Daudé
@ 2019-11-29 18:42   ` Peter Maydell
  1 sibling, 0 replies; 57+ messages in thread
From: Peter Maydell @ 2019-11-29 18:42 UTC (permalink / raw)
  To: Damien Hedde
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Edgar Iglesias, qemu-arm,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Deprecate device_legacy_reset(), qdev_reset_all() and
> qbus_reset_all() to be replaced by new functions
> device_cold_reset() and bus_cold_reset() which uses resettable API.
>
> Also introduce resettable_cold_reset_fn() which may be used as a
> replacement for qdev_reset_all_fn and qbus_reset_all_fn().
>
> Following patches will be needed to look at legacy reset call sites
> and switch to resettable api. The legacy functions will be removed
> when unused.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>
> I've removed the general helpers
> + device_reset(DeviceState *dev, ResetType type)
> + bus_reset(BusState *dev, ResetType type)
> because with several reset types, all devices and buses will not
> implement all of them. I think it is preferable to define a
> type-specific helper every time it is needed for base classes
> implementing the reset type (eg a device_pci_reset(PciDev*) for the
> pci reset type if add that).
>
> So device_reset()/bus_reset() symbol names are not taken anymore. I
> don't have a strong opinion of what names should have the
> device_cold_reset() and bus_cold_reset() function added in this
> patch. It could be device/bus_hard_reset() (the current
> qbus_reset_all() comment mention we do a "hard" reset) or simply
> device/bus_reset() or anything else.
> What do you think ? Any better idea ? It should be consistent with
> the reset type name but we can change it also if cold is not what we
> want.
>
> Note that if we don't settle for device/bus_reset(). We can drop
> the first patch that renames device_reset() to device_legacy_reset()

I think we're good the way you have things in this patchset.

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

thanks
-- PMM


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 09/13] docs/devel/reset.txt: add doc about Resettable interface
  2019-10-18 15:06 ` [PATCH v5 09/13] docs/devel/reset.txt: add doc about Resettable interface Damien Hedde
@ 2019-11-29 19:00   ` Peter Maydell
  2019-12-06 15:40     ` Damien Hedde
  2019-12-06 16:21     ` Damien Hedde
  0 siblings, 2 replies; 57+ messages in thread
From: Peter Maydell @ 2019-11-29 19:00 UTC (permalink / raw)
  To: Damien Hedde
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Edgar Iglesias, qemu-arm,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>

Subject line still says ".txt".

> ---
>
> Should I add an entry into index.rst ?

Yes, please.

I have one substantive comment on this patch: in the
MyDev example of implementing reset, you make the enter/hold/exit
functions do the reset of the subclass and then call the
myclass->parent_phases method. This feels to me like it's the
wrong way round and the parent class should reset first. Does
it make any difference to do it this way round?


Everything below here is just typo/grammar fixes, so
if you correct those then
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>


> ---
>  docs/devel/reset.rst | 282 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 282 insertions(+)
>  create mode 100644 docs/devel/reset.rst
>
> diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst
> new file mode 100644
> index 0000000000..a562720f55
> --- /dev/null
> +++ b/docs/devel/reset.rst
> @@ -0,0 +1,282 @@
> +
> +=======================================
> +Reset in QEMU: the Resettable interface
> +=======================================
> +
> +The reset of qemu objects is handled using the resettable interface declared
> +in ``include/hw/resettable.h``.
> +
> +This interface allows to group objects (on a tree basis) and to reset the

"allows objects to be grouped"; "so that the whole group can be
reset consistently".

> +whole group 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.
> +
> +
> +Triggering reset
> +----------------
> +
> +This section documents the APIs which "users" of a resettable object should use
> +to control it. All resettable control functions must be called while holding
> +the iothread lock.
> +
> +You can apply a reset to an object using ``resettable_assert_reset()``. You need
> +to call ``resettable_release_reset()`` to release the object from reset. To
> +instantly reset an object, without keeping it in reset state, just call
> +``resettable_reset()``. These functions function takes two parameters: a pointer

"These functions take two parameters"

> +to the object to reset and a reset type.
> +
> +Several types of reset will be supported. For now only cold reset is defined,

"defined;"

> +others may be added later. The Resettable interface handles reset types with an
> +enum:
> +
> +``RESET_TYPE_COLD``
> +  Cold reset is supported by every resettable object. In QEMU, it means we reset
> +  to the initial state corresponding to the start of QEMU; this might differs

"differ"

> +  from what is a read hardware cold reset. It differs from other reset (like

"real"; "other resets"

> +  warm or bus resets) which may keep certain parts untouched.
> +
> +Calling ``resettable_reset()`` is equivalent to calling
> +``resettable_assert_reset()`` then ``resettable_release_reset()``. It is
> +possible to interleave multiple calls to these three functions. There may
> +be several reset sources/controllers of a given object. The interface handles
> +everything and the different reset controllers do not need to know anything
> +about each others. The object will leave reset state only when each other

"each other". "when all controllers end"

> +controllers end their reset operation. This point is handled internally by
> +maintaining a count of reset; it is crucial to call

"of in-progress resets"

> +``resettable_release_reset()`` one time and only one time per
> +``resettable_assert_reset()`` call.
> +
> +For now migration of a device or bus in reset is not supported. Care must be
> +taken not to delayed ``resettable_release_reset()`` from its

"delay"; "after its"

> +``resettable_assert_reset()`` counterpart.

"in a way that would allow migration to happen between the two".

> +
> +Note that, since resettable is an interface, the API takes simple Object as

"a simple"

> +parameter. Still, it is a programming error to call a resettable function on a
> +non-resettable object and it will trigger a run time assert error. Since most
> +call to resettable interface are done through base class functions, such an

"calls"

> +error is not likely to happen.
> +
> +For Devices and Buses, the following helper functions exists:

"exist"

> +
> +- ``device_cold_reset()``
> +- ``bus_cold_reset()``
> +
> +These are simple wrappers around resettable_reset() function; they only cast the
> +Device or Bus into an Object and pass the cold reset type. When possible
> +prefer to use these functions instead ``resettable_reset()``.

"instead of"


> +
> +Device and bus functions co-exist because there can be semantic differences
> +between resetting a bus and resetting the controller bridge which owns it.
> +For example, considering a SCSI controller. Resetting the controller puts all

"consider"

> +its registers back to what reset state was as well as reset everything on the

"to their reset state, as well as resetting everything"

> +SCSI bus. Whereas resetting just the SCSI bus only resets everything that's on

"bus, whereas"

> +it but not the controller.
> +
> +
> +Multi-phase mechanism
> +---------------------
> +
> +This section documents the internals of the resettable interface.
> +
> +The resettable interface uses a multi-phase system to relieve objects and
> +machines from reset ordering problems. To address this, the reset operation
> +of an object is split into three well defined phases.
> +
> +When resetting a several objects (for example the whole machine at simulation

"resetting several"

> +startup), all first phases of all objects are executed, then all second phases
> +and then all third phases.
> +
> +The three phases are:
> +
> +1. The **enter** phase is executed when the object enters reset. It reset only

"It resets"

> +   local state of the object; it must not do anything that has a side-effect
> +   on other objects, such as raising or lowering a qemu_irq line or reading or
> +   writing guest memory.
> +
> +2. The **hold** phase is executed for entry into reset, once every object in the
> +   group which is being reset has had its *enter* phase executed. At this point
> +   devices can do actions that affect other objects.
> +
> +3. The **exit** phase is executed when the object leaves the reset state.
> +   Actions affecting other objects are permitted.
> +
> +As said in previous section, the interface maintains a count of reset. This
> +count is used to ensure phases are executed only when required. *enter* and
> +*hold* phases are executed only when asserting reset for the first time
> +(if an object is already in reset state when calling
> +``resettable_assert_reset()`` or ``resettable_reset()``, they are not
> +executed).
> +The *exit* phase is executed only when the last reset operation ends. Therefore
> +the object has not to care how many reset controllers it has and how many of

"does not need to care how many"

> +them have started a reset.
> +
> +
> +Handling reset in a resettable object
> +-----------------------------------------

There seem to be too many '-' in this line.

> +
> +This section documents the APIs that an implementation of a resettable object
> +must provide and what functions it has access to. It is intended for people
> +who want to implement or convert a class which has the resettable interface;
> +for example when specializing an existing device or bus.
> +
> +Methods to implement
> +....................
> +
> +Three methods should be defined or left empty. Each method corresponds to a
> +phase of the reset; they are name ``phases.enter()``, ``phases.hold()`` and
> +``phases.exit()``. They all take the object as parameter. The *enter* method
> +also take the reset type as second parameter.
> +
> +When extending an existing class, these methods may need to be extended too.
> +The ``resettable_class_set_parent_phases()`` class function may be used to
> +backup mother class methods.

"parent class" (the terminology oddity is particularly noticeable here
because the function name uses the usual names).

> +
> +Here follows an example to implement reset for a Device which set an IO while

"sets"

> +in reset.
> +
> +::
> +
> +    static void mydev_reset_enter(Object *obj, ResetType type)
> +    {
> +        MyDevClass *myclass = MYDEV_GET_CLASS(obj);
> +        MyDevState *mydev = MYDEV(obj);
> +        /* initialize local state only */
> +        mydev->var = 0;
> +        /* call mother class enter phase */
> +        if (myclass->parent_phases.enter) {
> +            myclass->parent_phases.enter(obj, type);
> +        }
> +    }
> +
> +    static void mydev_reset_hold(Object *obj)
> +    {
> +        MyDevClass *myclass = MYDEV_GET_CLASS(obj);
> +        MyDevState *mydev = MYDEV(obj);
> +        /* set an IO */
> +        qemu_set_irq(mydev->irq, 1);
> +        /* call mother class hold phase */
> +        if (myclass->parent_phases.hold) {
> +            myclass->parent_phases.hold(obj);
> +        }
> +    }
> +
> +    static void mydev_reset_exit(Object *obj)
> +    {
> +        MyDevClass *myclass = MYDEV_GET_CLASS(obj);
> +        MyDevState *mydev = MYDEV(obj);
> +        /* clear an IO */
> +        qemu_set_irq(mydev->irq, 0);
> +        /* call mother class exit phase */
> +        if (myclass->parent_phases.exit) {
> +            myclass->parent_phases.exit(obj);
> +        }
> +    }
> +
> +    typedef struct MyDevClass {
> +        MyMotherClass mother_class;
> +        /* to store eventual parent reset methods */
> +        ResettablePhases parent_phases;
> +    } MyDevClass;
> +
> +    static void mydev_class_init(ObjectClass *class, void *data)
> +    {
> +        MyDevClass *myclass = MYDEV_CLASS(class);
> +        ResettableClass *rc = RESETTABLE_CLASS(class);
> +        resettable_class_set_parent_reset_phases(rc,
> +                                                 mydev_reset_enter,
> +                                                 mydev_reset_hold,
> +                                                 mydev_reset_exit,
> +                                                 &myclass->parent_phases);
> +    }
> +
> +In the above example, we override the three phases. It is possible to override

"all three phases".

> +only part of them by passing NULL instead of a function pointer to

"only some of them"

> +``resettable_class_set_parent_reset_phases()``. For example, the following will
> +only override the *enter* phase and leave *hold* and *exit* untouched::
> +
> +    resettable_class_set_parent_reset_phases(rc, mydev_reset_enter,
> +                                             NULL, NULL,
> +                                             &myclass->parent_phases);

"This is equivalent to providing a trivial implementation of the
hold and exit phases which does nothing but call the parent class's
implementation of the phase."


> +
> +
> +Polling the reset state
> +.......................
> +
> +Resettable interface provide the ``resettable_is_in_reset()`` function.

"The Resettable interface provides"

> +This function tells if the object parameter is currently under reset.

"This function returns true if"

> +
> +An object is under reset from the beginning of the *init* phase to the end of

"end of the"

> +*exit* phase. During all three phases, the function will return that the object
> +is in reset.
> +
> +This function may be used if behavior has to be adapted when receiving external
> +interaction while in reset state. +Note that until migration of the reset state
> +is supported, an object should not be left in reset. So apart from being
> +currently executing one of a reset phase, the only cases when this
> +function should return true is if an external interaction is made during *hold*
> +or *exit* phase of another object in the same reset group.

This paragraph feels a bit unclear to me but I'm not sure how to improve it.

> +
> +Helpers ``device_is_in_reset()`` and ``bus_is_in_reset()`` are also provided
> +for devices and buses and should be preferred.
> +
> +
> +Base class handling of reset
> +----------------------------
> +
> +This section documents parts of the reset mechanism that you only need to know
> +about if you are extending it to work with a new base class other than
> +DeviceClass or BusClass, or maintaining the existing code in those classes. Most
> +people can ignore it.
> +
> +Methods to implement
> +....................
> +
> +There are two other methods that need to exist in a class implementing the
> +interface: ``get_state()`` and ``child_foreach()``.
> +
> +``get_state()`` is simple. *resettable* is an interface and, as a consequence,
> +does not have any class state structure. But in order to factorize the code, we
> +need one. This method must return a pointer to ``ResettableState`` structure.
> +The structure must be allocated by the base class; preferably it should be
> +located inside the object instance structure.
> +
> +``child_foreach()`` is more complex. It should execute the given callback on
> +every reset child of the given resettable object. All children must be
> +resettable too. Additional parameters (a reset type and an opaque pointer) must
> +be passed to the callback too.
> +
> +In ``DeviceClass`` and ``BusClass`` the ``ResettableState`` is located
> +``DeviceState`` and ``BusState`` structure. ``child_foreach()`` is implemented
> +to follow the bus hierarchy; for a bus, it calls the function on every child
> +device; for a device, it calls the function on every bus child. When we reset
> +the main system bus, we reset the whole machine bus tree.
> +
> +Changing a resettable parent
> +............................
> +
> +One thing so should be taken care of by the base class is handling reset

"One thing which"

> +hierarchy changes.
> +
> +The reset hierarchy is supposed to be static and built during machine creation.
> +But there are actually some exceptions. To cope with this, the resettable API
> +provides ``resettable_change_parent()``. This function allows to set, update or
> +remove the parent of a resettable object after machine creation is done. As
> +parameters, it takes the object being moved, the old parent if any and the new
> +parent if any.
> +
> +This function can be used at anytime when not in a reset operation. During

"any time"

> +a reset operation it must be used only in *hold* phase. Using it in *enter* or
> +*exit* phase is an error.
> +Also it should not be used during machine creation, although it is harmless to
> +do so: the function is a no-op as long as old and new parent are NULL or not
> +in reset.
> +
> +There is currently 2 cases where this function is used:
> +
> +1. *device hotplug*; it means a new device is introduced on a live bus.
> +
> +2. *hot bus change*; it means an existing live device is added, moved or
> +   removed in the bus hiearchy. At the moment, it occurs only in the raspi
> +   machines for chaning the sdbus used by sd card.

"changing"

> --
> 2.23.0
>

thanks
-- PMM


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 10/13] vl: replace deprecated qbus_reset_all registration
  2019-10-18 15:06 ` [PATCH v5 10/13] vl: replace deprecated qbus_reset_all registration Damien Hedde
@ 2019-11-29 19:01   ` Peter Maydell
  0 siblings, 0 replies; 57+ messages in thread
From: Peter Maydell @ 2019-11-29 19:01 UTC (permalink / raw)
  To: Damien Hedde
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Edgar Iglesias, qemu-arm,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Replace deprecated qbus_reset_all by resettable_cold_reset_fn for
> the sysbus reset registration.
>
> Apart for the raspi machines, this does not impact the behavior
> because:
> + at this point resettable just call the old reset methods of devices

"calls"

>   and buses in the same order than qdev/qbus.

"same order as"

> + resettable handlers registered with qemu_register_reset are
>   serialized; there is no interleaving.
> + eventual explicit calls to legacy reset API (device_reset or
>   qdev/qbus_reset) inside this reset handler will not be masked out
>   by resettable mechanism; they do not go through resettable api.
>
> For the raspi machines, during the sysbus reset the sd-card is not
> reset twice anymore but only once. This is consequence of switching

"a consequence"

> both sysbus reset and changing parent to resettable; it detects the
> second reset is not needed. This has no impact on the state after
> reset; the sd-card reset method only reset local state and query
> information from the block backend.

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

thanks
-- PMM


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 11/13] hw/s390x/ipl: replace deprecated qdev_reset_all registration
  2019-10-18 15:06 ` [PATCH v5 11/13] hw/s390x/ipl: replace deprecated qdev_reset_all registration Damien Hedde
  2019-10-31 23:38   ` Philippe Mathieu-Daudé
@ 2019-11-29 19:02   ` Peter Maydell
  2019-12-03 11:41   ` Cornelia Huck
  2 siblings, 0 replies; 57+ messages in thread
From: Peter Maydell @ 2019-11-29 19:02 UTC (permalink / raw)
  To: Damien Hedde
  Cc: Thomas Huth, Daniel P. Berrange, Eduardo Habkost, qemu-s390x,
	Cornelia Huck, Mark Burton, QEMU Developers,
	Christian Borntraeger, Edgar Iglesias, qemu-arm, Paolo Bonzini,
	Philippe Mathieu-Daudé,
	David Gibson

On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Replace deprecated qdev_reset_all by resettable_cold_reset_fn for
> the ipl registration in the main reset handlers.
>
> This does not impact the behavior for the following reasons:
> + at this point resettable just call the old reset methods of devices
>   and buses in the same order than qdev/qbus.
> + resettable handlers registered with qemu_register_reset are
>   serialized; there is no interleaving.
> + eventual explicit calls to legacy reset API (device_reset or
>   qdev/qbus_reset) inside this reset handler will not be masked out
>   by resettable mechanism; they do not go through resettable api.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---

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

thanks
-- PMM


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 13/13] hw/gpio/bcm2835_gpio: Update to resettable
  2019-10-18 15:06 ` [PATCH v5 13/13] hw/gpio/bcm2835_gpio: Update to resettable Damien Hedde
@ 2019-11-29 19:02   ` Peter Maydell
  0 siblings, 0 replies; 57+ messages in thread
From: Peter Maydell @ 2019-11-29 19:02 UTC (permalink / raw)
  To: Damien Hedde
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Andrew Baumann, Edgar Iglesias,
	qemu-arm, Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Transition the bcm2835_gpio device class to Resettable.
> The sdbus reparenting is delayed in hold phase to respect
> resettable side-effect rules.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>

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

thanks
-- PMM


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 12/13] hw/gpio/bcm2835_gpio: Isolate sdbus reparenting
  2019-10-18 15:06 ` [PATCH v5 12/13] hw/gpio/bcm2835_gpio: Isolate sdbus reparenting Damien Hedde
@ 2019-11-29 19:05   ` Peter Maydell
  2019-12-02 12:27     ` Damien Hedde
  0 siblings, 1 reply; 57+ messages in thread
From: Peter Maydell @ 2019-11-29 19:05 UTC (permalink / raw)
  To: Damien Hedde
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Andrew Baumann, Edgar Iglesias,
	qemu-arm, Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Split gpfsel_set() in 2 so that the sdbus reparenting is done
> in a dedicated function.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Andrew Baumann <Andrew.Baumann@microsoft.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: qemu-arm@nongnu.org
> ---
>  hw/gpio/bcm2835_gpio.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/hw/gpio/bcm2835_gpio.c b/hw/gpio/bcm2835_gpio.c
> index 91ce3d10cc..81fe07132f 100644
> --- a/hw/gpio/bcm2835_gpio.c
> +++ b/hw/gpio/bcm2835_gpio.c
> @@ -75,7 +75,10 @@ static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
>              s->fsel[index] = fsel;
>          }
>      }
> +}
>
> +static void gpfsel_update_sdbus(BCM2835GpioState *s)
> +{
>      /* SD controller selection (48-53) */
>      if (s->sd_fsel != 0
>              && (s->fsel[48] == 0) /* SD_CLK_R */
> @@ -86,6 +89,7 @@ static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
>              && (s->fsel[53] == 0) /* SD_DATA3_R */
>              ) {
>          /* SDHCI controller selected */
> +        sdbus_reparent_card(&s->sdbus, s->sdbus_sdhci);
>          sdbus_reparent_card(s->sdbus_sdhost, s->sdbus_sdhci);
>          s->sd_fsel = 0;>      } else if (s->sd_fsel != 4
> @@ -97,6 +101,7 @@ static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
>              && (s->fsel[53] == 4) /* SD_DATA3_R */
>              ) {
>          /* SDHost controller selected */
> +        sdbus_reparent_card(&s->sdbus, s->sdbus_sdhost);
>          sdbus_reparent_card(s->sdbus_sdhci, s->sdbus_sdhost);

The commit message says it's just splitting the function in two,
but these two hunks are adding extra calls to sdbus_reparent_card().
Why do we need to call it twice ?

>          s->sd_fsel = 4;
>      }

thanks
-- PMM


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 00/13] Multi-phase reset mechanism
  2019-10-18 15:06 [PATCH v5 00/13] Multi-phase reset mechanism Damien Hedde
                   ` (14 preceding siblings ...)
  2019-10-29 15:53 ` Damien Hedde
@ 2019-11-29 19:07 ` Peter Maydell
  2019-12-03 11:44 ` Cornelia Huck
  16 siblings, 0 replies; 57+ messages in thread
From: Peter Maydell @ 2019-11-29 19:07 UTC (permalink / raw)
  To: Damien Hedde
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Edgar Iglesias, qemu-arm,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Hi all,
>
> The purpose of this series is to split the current reset procedure
> into multiple phases. This will help to solve some ordering
> difficulties we have during reset. Previous version can be found here:
> https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg04359.html
>
> This series adds resettable interface and transitions base Device and
> Bus classes (sysbus subclasses are ok too). It provides new reset
> functions but does not switch anymore the old functions
> (device_reset() and qdev/qbus_reset_all()) to resettable interface.
> These functions keep the exact same behavior as before.
>
> The series also transition the main reset handlers registration which
> has no impact until devices and buses are transitioned.
>
> I think this version is way better regarding the transition from the
> legacy to the resettable interface than the previous one.
> After this series, the plan is then to transition devices, buses and
> legacy reset call sites. Devices and buses have to be transitioned
> from mother class to daughter classes order but until the final
> (daughter) class is transitioned, old monolitic reset behavior will
> be kept for this class.

Hi; I finally got back to reviewing this patchset, and I've
left comments on various patches (minor stuff only). I didn't
bother to comment on a few patches that I was happy with and
somebody else had already reviewed.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 03/13] hw/core: create Resettable QOM interface
  2019-11-29 18:32   ` Peter Maydell
@ 2019-12-02 11:07     ` Damien Hedde
  2019-12-02 11:14       ` Peter Maydell
  0 siblings, 1 reply; 57+ messages in thread
From: Damien Hedde @ 2019-12-02 11:07 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Edgar Iglesias, qemu-arm,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson


On 11/29/19 7:32 PM, Peter Maydell wrote:
> On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> This commit defines an interface allowing multi-phase reset. This aims
>> to solve a problem of the actual single-phase reset (built in
>> DeviceClass and BusClass): reset behavior is dependent on the order
>> in which reset handlers are called. In particular doing external
>> side-effect (like setting an qemu_irq) is problematic because receiving
>> object may not be reset yet.
>>
>> The Resettable interface divides the reset in 3 well defined phases.
>> To reset an object tree, all 1st phases are executed then all 2nd then
>> all 3rd. See the comments in include/hw/resettable.h for a more complete
>> description. The interface defines 3 phases to let the future
>> possibility of holding an object into reset for some time.
>>
>> The qdev/qbus reset in DeviceClass and BusClass will be modified in
>> following commits to use this interface. A mechanism is provided
>> to allow executing a transitional reset handler in place of the 2nd
>> phase which is executed in children-then-parent order inside a tree.
>> This will allow to transition devices and buses smoothly while
>> keeping the exact current qdev/qbus reset behavior for now.
>>
>> Documentation will be added in a following commit.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>>
>> In this patch only a single reset type is supported, but the interface
>> allows for more to be defined.
>>
>> I had some thought about problems which may arise when having multiple
>> reset types:
> 
> [snip]
> 
> Yeah, these all seem right. We clearly need to think a bit
> more before we add multiple reset types. Let's get this basic
> just-cold-reset in for now and come back to the rest later.
> 
> 
> Almost all of my comments below are just grammar/typo fixes
> for comments. The only substantives are:
>  * globals
>  * copyright/licensing comment needed in the .h file
> and they're pretty minor items.
> 
>> +/**
>> + * enter_phase_in_progress:
>> + * Flag telling whether we are currently in an enter phase where side
>> + * effects are forbidden. This flag allows us to catch if reset is called
>> + * again during during this phase.
>> + */
>> +static bool enter_phase_in_progress;
> 
> This looks weird -- I don't think a global for this works,
> because you might have several distinct subtrees of
> devices, and be doing reset on them both at once.
> I think that we only use this for an assert, though -- is
> that right? If so, we could just drop this.

We say that we need to own the iothread mutex for any reset, so global
should be ok. Thought, I just checked, it's only mentioned in the
documentation not in the header file. I should probably add a comment
there too along with the link to the documentation file.

If we want to drop the iothread mutex constraint. I think we need to
carefully check there is no hidden problem. In particular in hold and
exit phases we allow to have external effects like setting gpios and we
have no way to control what it provokes.

You're right it is just for assert: to avoid any miss-use of the api
which could lead to being in bad reset state. So we can indeed drop it.

> 
>> +void resettable_assert_reset(Object *obj, ResetType type)
>> +{
>> +    assert(!enter_phase_in_progress);
>> +    /* TODO: change that assert when adding support for other reset types */
> 
> I'm not sure which assert this is referring to -- the one above
> the comment, or the one below ?

It refers to the assert(type == RESET_TYPE_COLD).
I added theses because we cannot just add items in the enum to have
working multiple reset types.
A comment like this will be more clear:
/*
 * TODO: Additional reset types need support in phases handling
 * functions (resettable_phase_enter/hold/exit()) before allowing more
 * enum entries. Remove the following assert when it is done.
 */

> 
>> +    assert(type == RESET_TYPE_COLD);
>> +    trace_resettable_reset_assert_begin(obj, type);
>> +    enter_phase_in_progress = true;
>> +    resettable_phase_enter(obj, NULL, type);
>> +    enter_phase_in_progress = false;
>> +    resettable_phase_hold(obj, NULL, type);
>> +    trace_resettable_reset_assert_end(obj);
>> +}
>> +
>> +void resettable_release_reset(Object *obj, ResetType type)
>> +{
>> +    assert(!enter_phase_in_progress);
>> +    /* TODO: change that assert when adding support for other reset types */
> 
> Ditto.
> 
> 
> 
>> --- /dev/null
>> +++ b/include/hw/resettable.h
>> @@ -0,0 +1,199 @@
>> +#ifndef HW_RESETTABLE_H
>> +#define HW_RESETTABLE_H
>> +
> 
> All new files, including even small header files, should have
> the usual copyright-and-license comment at the top. (Can you
> check also whether this needs adding for any other new files the
> patchset creates, please?)

I'll do that and fix all the typos

Thanks for the review,
--
Damien


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 03/13] hw/core: create Resettable QOM interface
  2019-12-02 11:07     ` Damien Hedde
@ 2019-12-02 11:14       ` Peter Maydell
  0 siblings, 0 replies; 57+ messages in thread
From: Peter Maydell @ 2019-12-02 11:14 UTC (permalink / raw)
  To: Damien Hedde
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Edgar Iglesias, qemu-arm,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

On Mon, 2 Dec 2019 at 11:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
>
> On 11/29/19 7:32 PM, Peter Maydell wrote:
> > On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
> >> +/**
> >> + * enter_phase_in_progress:
> >> + * Flag telling whether we are currently in an enter phase where side
> >> + * effects are forbidden. This flag allows us to catch if reset is called
> >> + * again during during this phase.
> >> + */
> >> +static bool enter_phase_in_progress;
> >
> > This looks weird -- I don't think a global for this works,
> > because you might have several distinct subtrees of
> > devices, and be doing reset on them both at once.
> > I think that we only use this for an assert, though -- is
> > that right? If so, we could just drop this.
>
> We say that we need to own the iothread mutex for any reset, so global
> should be ok. Thought, I just checked, it's only mentioned in the
> documentation not in the header file. I should probably add a comment
> there too along with the link to the documentation file.

Ah, right, I hadn't considered that the mutex is effectively
restricting to only a single reset happening at once. If
you want to keep the asserts you can, if you add a comment
noting that these globals are (a) only for asserts and (b)
OK because we rely on the iothread mutex to ensure that only
one reset operation can be in progress at once.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 04/13] hw/core: add Resettable support to BusClass and DeviceClass
  2019-11-29 18:36   ` Peter Maydell
@ 2019-12-02 11:38     ` Damien Hedde
  0 siblings, 0 replies; 57+ messages in thread
From: Damien Hedde @ 2019-12-02 11:38 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Edgar Iglesias, qemu-arm,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson



On 11/29/19 7:36 PM, Peter Maydell wrote:
> On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> This commit adds support of Resettable interface to buses and devices:
>> + ResettableState structure is added in the Bus/Device state
>> + Resettable methods are implemented.
>> + device/bus_is_in_reset function defined
>>
>> This commit allows to transition the objects to the new
>> multi-phase interface without changing the reset behavior at all.
>> Object single reset method can be split into the 3 different phases
>> but the 3 phases are still executed in a row for a given object.
>> From the qdev/qbus reset api point of view, nothing is changed.
>> qdev_reset_all() and qbus_reset_all() are not modified as well as
>> device_legacy_reset().
>>
>> Transition of an object must be done from mother class to daughter
>> classes.
> 
> The standard terminology here is "parent class" and "child class".
> 
> I notice you've used mother/daughter in a bunch of other comments
> in various patches -- could you change that to the more usual
> terms, please?
Yes. Sorry for that.

> 
>> Care has been taken to allow the transition of a mother class
>> without requiring the daughter classes to be transitioned at the same
>> time. Note that SysBus and SysBusDevice class do not need any transition
>> because they do not override the legacy reset method.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> 
> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> thanks
> -- PMM
> 

--
Damien


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 05/13] hw/core/resettable: add support for changing parent
  2019-11-29 18:38   ` Peter Maydell
@ 2019-12-02 11:43     ` Damien Hedde
  0 siblings, 0 replies; 57+ messages in thread
From: Damien Hedde @ 2019-12-02 11:43 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Edgar Iglesias, qemu-arm,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson



On 11/29/19 7:38 PM, Peter Maydell wrote:
> On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> Add a function resettable_change_parent() to do the required
>> plumbing when changing the parent a of Resettable object.
>>
>> We need to make sure that the reset state of the object remains
>> coherent with the reset state of the new parent.
>>
>> We make the 2 following hypothesis:
>> + when an object is put in a parent under reset, the object goes in
>> reset.
>> + when an object is removed from a parent under reset, the object
>> leaves reset.
>>
>> The added function avoid any glitch if both old and new parent are
>> already in reset.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>>  hw/core/resettable.c    | 54 +++++++++++++++++++++++++++++++++++++++++
>>  hw/core/trace-events    |  1 +
>>  include/hw/resettable.h | 16 ++++++++++++
>>  3 files changed, 71 insertions(+)
>>
>> diff --git a/hw/core/resettable.c b/hw/core/resettable.c
>> index c5e11cff4f..60d4285fcc 100644
>> --- a/hw/core/resettable.c
>> +++ b/hw/core/resettable.c
>> @@ -32,6 +32,14 @@ static void resettable_phase_exit(Object *obj, void *opaque, ResetType type);
>>   */
>>  static bool enter_phase_in_progress;
>>
>> +/**
>> + * exit_phase_in_progress:
>> + * Flag telling whether we are currently in an enter phase where side
>> + * effects are forbidden. This flag allows us to catch if
>> + * resettable_change_parent() is called during exit phase.
>> + */
>> +static unsigned exit_phase_in_progress;
> 
> This is another global that I don't think we should have.
> Is it also just for asserts ?

Yes. It's only to ensure we don't miss-use the api.

--
Damien




^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 12/13] hw/gpio/bcm2835_gpio: Isolate sdbus reparenting
  2019-11-29 19:05   ` Peter Maydell
@ 2019-12-02 12:27     ` Damien Hedde
  2019-12-02 12:33       ` Peter Maydell
  0 siblings, 1 reply; 57+ messages in thread
From: Damien Hedde @ 2019-12-02 12:27 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Andrew Baumann, Edgar Iglesias,
	qemu-arm, Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson



On 11/29/19 8:05 PM, Peter Maydell wrote:
> On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> Split gpfsel_set() in 2 so that the sdbus reparenting is done
>> in a dedicated function.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Andrew Baumann <Andrew.Baumann@microsoft.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Cc: qemu-arm@nongnu.org
>> ---
>>  hw/gpio/bcm2835_gpio.c | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/gpio/bcm2835_gpio.c b/hw/gpio/bcm2835_gpio.c
>> index 91ce3d10cc..81fe07132f 100644
>> --- a/hw/gpio/bcm2835_gpio.c
>> +++ b/hw/gpio/bcm2835_gpio.c
>> @@ -75,7 +75,10 @@ static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
>>              s->fsel[index] = fsel;
>>          }
>>      }
>> +}
>>
>> +static void gpfsel_update_sdbus(BCM2835GpioState *s)
>> +{
>>      /* SD controller selection (48-53) */
>>      if (s->sd_fsel != 0
>>              && (s->fsel[48] == 0) /* SD_CLK_R */
>> @@ -86,6 +89,7 @@ static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
>>              && (s->fsel[53] == 0) /* SD_DATA3_R */
>>              ) {
>>          /* SDHCI controller selected */
>> +        sdbus_reparent_card(&s->sdbus, s->sdbus_sdhci);
>>          sdbus_reparent_card(s->sdbus_sdhost, s->sdbus_sdhci);
>>          s->sd_fsel = 0;>      } else if (s->sd_fsel != 4
>> @@ -97,6 +101,7 @@ static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
>>              && (s->fsel[53] == 4) /* SD_DATA3_R */
>>              ) {
>>          /* SDHost controller selected */
>> +        sdbus_reparent_card(&s->sdbus, s->sdbus_sdhost);
>>          sdbus_reparent_card(s->sdbus_sdhci, s->sdbus_sdhost);
> 
> The commit message says it's just splitting the function in two,
> but these two hunks are adding extra calls to sdbus_reparent_card().
> Why do we need to call it twice ?

You're right. I forgot to update the commit message. The patch also
refactor a little the reset procedure and move the call to
sdbus_reparent_card(&s->sdbus, s->sdbus_sdhci)
which was in there to this part of the code.

raspi machines create the sd in &s->sdbus. So there is need for a first
reparenting from this bus.

With this addition "gpfsel_update_sdbus" always do the expected effect
of putting the sd card onto the right bus.

sdbus_reparent_card(src,dst) only do something if the _src_ bus has a
card. So only one of the 2 sdbus_reparent_card will have an effect. If
the card is already onto the _dst_, both calls will be nop-op.

What about rewording the commit message like this ?
| hw/gpio/bcm2835_gpio: Refactor sdbus reparenting
|
| Split gpfsel_set() in 2 so that the sdbus reparenting is done in a
| dedicated function gpfsel_update_sdbus() and update call sites.
| Also make gpfsel_update_sdbus() handle the case where the sdcard is in
| BCM2835GpioState.sdbus (initial sd card holding bus at machine
| creation).
| Refactor the reset procedure in consequence.
|
| This patch is a preparation step for the migration to multi-phases
| reset which will be done in a following commit

Thanks,
--
Damien


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 12/13] hw/gpio/bcm2835_gpio: Isolate sdbus reparenting
  2019-12-02 12:27     ` Damien Hedde
@ 2019-12-02 12:33       ` Peter Maydell
  2019-12-02 13:05         ` Damien Hedde
  0 siblings, 1 reply; 57+ messages in thread
From: Peter Maydell @ 2019-12-02 12:33 UTC (permalink / raw)
  To: Damien Hedde
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Andrew Baumann, Edgar Iglesias,
	qemu-arm, Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

On Mon, 2 Dec 2019 at 12:27, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
>
>
> On 11/29/19 8:05 PM, Peter Maydell wrote:
> > On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
> >> @@ -97,6 +101,7 @@ static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
> >>              && (s->fsel[53] == 4) /* SD_DATA3_R */
> >>              ) {
> >>          /* SDHost controller selected */
> >> +        sdbus_reparent_card(&s->sdbus, s->sdbus_sdhost);
> >>          sdbus_reparent_card(s->sdbus_sdhci, s->sdbus_sdhost);
> >
> > The commit message says it's just splitting the function in two,
> > but these two hunks are adding extra calls to sdbus_reparent_card().
> > Why do we need to call it twice ?
>
> You're right. I forgot to update the commit message. The patch also
> refactor a little the reset procedure and move the call to
> sdbus_reparent_card(&s->sdbus, s->sdbus_sdhci)
> which was in there to this part of the code.
>
> raspi machines create the sd in &s->sdbus. So there is need for a first
> reparenting from this bus.
>
> With this addition "gpfsel_update_sdbus" always do the expected effect
> of putting the sd card onto the right bus.
>
> sdbus_reparent_card(src,dst) only do something if the _src_ bus has a
> card. So only one of the 2 sdbus_reparent_card will have an effect. If
> the card is already onto the _dst_, both calls will be nop-op

The intention of sdbus_reparent_card() is that it moves
something from the 'src' bus to the 'dst' bus. So one
call is supposed to do the whole job of the move. If
it doesn't, then that's a bug.

I thought the raspi machines had an sd card that could
either be connected to one of the controllers, or the
other. Why would the sd card ever be somewhere else
than on one of those two buses? If the machine creation
puts the sdcard somewhere wrong then we should probably
just fix that.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 12/13] hw/gpio/bcm2835_gpio: Isolate sdbus reparenting
  2019-12-02 12:33       ` Peter Maydell
@ 2019-12-02 13:05         ` Damien Hedde
  2019-12-02 13:10           ` Peter Maydell
  0 siblings, 1 reply; 57+ messages in thread
From: Damien Hedde @ 2019-12-02 13:05 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Andrew Baumann, Edgar Iglesias,
	qemu-arm, Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson



On 12/2/19 1:33 PM, Peter Maydell wrote:
> On Mon, 2 Dec 2019 at 12:27, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>>
>>
>> On 11/29/19 8:05 PM, Peter Maydell wrote:
>>> On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>>> @@ -97,6 +101,7 @@ static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
>>>>              && (s->fsel[53] == 4) /* SD_DATA3_R */
>>>>              ) {
>>>>          /* SDHost controller selected */
>>>> +        sdbus_reparent_card(&s->sdbus, s->sdbus_sdhost);
>>>>          sdbus_reparent_card(s->sdbus_sdhci, s->sdbus_sdhost);
>>>
>>> The commit message says it's just splitting the function in two,
>>> but these two hunks are adding extra calls to sdbus_reparent_card().
>>> Why do we need to call it twice ?
>>
>> You're right. I forgot to update the commit message. The patch also
>> refactor a little the reset procedure and move the call to
>> sdbus_reparent_card(&s->sdbus, s->sdbus_sdhci)
>> which was in there to this part of the code.
>>
>> raspi machines create the sd in &s->sdbus. So there is need for a first
>> reparenting from this bus.
>>
>> With this addition "gpfsel_update_sdbus" always do the expected effect
>> of putting the sd card onto the right bus.
>>
>> sdbus_reparent_card(src,dst) only do something if the _src_ bus has a
>> card. So only one of the 2 sdbus_reparent_card will have an effect. If
>> the card is already onto the _dst_, both calls will be nop-op
> 
> The intention of sdbus_reparent_card() is that it moves
> something from the 'src' bus to the 'dst' bus. So one
> call is supposed to do the whole job of the move. If
> it doesn't, then that's a bug.
> 
> I thought the raspi machines had an sd card that could
> either be connected to one of the controllers, or the
> other. Why would the sd card ever be somewhere else
> than on one of those two buses? If the machine creation
> puts the sdcard somewhere wrong then we should probably
> just fix that.
> 

I don't know why it has been implemented like this but right now the
raspi_init() does the following during machine creation:
| bus = qdev_get_child_bus(DEVICE(&s->soc), "sd-bus");
| [...]
| carddev = qdev_create(bus, TYPE_SD_CARD);
which put the sd in the BCM2835GpioState.sdbus .

Then the reset procedure of the BCM2835Gpio move the sd card
to one of the two usable controllers and the sd card can never go back
to the initial BCM2835GpioState.sdbus.
As far as I understand, it is theorically possible to have the sd card
on no controller at all (it's maybe the reason for the .sdbus "useless"
bus) (for example if the BCM2835Gpio is badly configured) but this is
not implemented in qemu.

Anyway I can add some plumbing to only call sdbus_reparent_card() when
really needed by:
+ not duplicating the sdbus_reparent_card() in gpfsel_update_sdbus()
+ adding needed test in reset() method to only do the initial
  sdbus_reparent_card() if needed (first time we call reset).

--
Damien



^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 12/13] hw/gpio/bcm2835_gpio: Isolate sdbus reparenting
  2019-12-02 13:05         ` Damien Hedde
@ 2019-12-02 13:10           ` Peter Maydell
  0 siblings, 0 replies; 57+ messages in thread
From: Peter Maydell @ 2019-12-02 13:10 UTC (permalink / raw)
  To: Damien Hedde
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Andrew Baumann, Edgar Iglesias,
	qemu-arm, Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

On Mon, 2 Dec 2019 at 13:05, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> I don't know why it has been implemented like this but right now the
> raspi_init() does the following during machine creation:
> | bus = qdev_get_child_bus(DEVICE(&s->soc), "sd-bus");
> | [...]
> | carddev = qdev_create(bus, TYPE_SD_CARD);
> which put the sd in the BCM2835GpioState.sdbus .
>
> Then the reset procedure of the BCM2835Gpio move the sd card
> to one of the two usable controllers and the sd card can never go back
> to the initial BCM2835GpioState.sdbus.

This seems like it's just an oversight. The code in raspi_init()
which creates the SD card was added in 2016, a year before
the gpio device was added, so when it was written there was
only ever one place the SD card could be, I think. We should
fix it so it puts the card in the right place to start with.

> As far as I understand, it is theorically possible to have the sd card
> on no controller at all (it's maybe the reason for the .sdbus "useless"
> bus) (for example if the BCM2835Gpio is badly configured) but this is
> not implemented in qemu.
>
> Anyway I can add some plumbing to only call sdbus_reparent_card() when
> really needed by:
> + not duplicating the sdbus_reparent_card() in gpfsel_update_sdbus()
> + adding needed test in reset() method to only do the initial
>   sdbus_reparent_card() if needed (first time we call reset).

I don't think you need the latter if we make the machine model
put the card in the right place to start with.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 01/13] add device_legacy_reset function to prepare for reset api change
  2019-10-18 15:06 ` [PATCH v5 01/13] add device_legacy_reset function to prepare for reset api change Damien Hedde
  2019-10-19 17:35   ` Richard Henderson
@ 2019-12-03 10:55   ` Cornelia Huck
  1 sibling, 0 replies; 57+ messages in thread
From: Cornelia Huck @ 2019-12-03 10:55 UTC (permalink / raw)
  To: Damien Hedde
  Cc: Fam Zheng, peter.maydell, Collin Walling, Dmitry Fleytman,
	Michael S. Tsirkin, qemu-devel, Gerd Hoffmann, David Hildenbrand,
	Halil Pasic, Christian Borntraeger, david, philmd, ehabkost,
	qemu-s390x, qemu-arm, Cédric Le Goater, John Snow,
	Richard Henderson, berrange, mark.burton, edgari, pbonzini

On Fri, 18 Oct 2019 17:06:18 +0200
Damien Hedde <damien.hedde@greensocs.com> wrote:

> Provide a temporary device_legacy_reset function doing what
> device_reset does to prepare for the transition with Resettable
> API.
> 
> All occurrence of device_reset in the code tree are also replaced
> by device_legacy_reset.
> 
> The new resettable API has different prototype and semantics
> (resetting child buses as well as the specified device). Subsequent
> commits will make the changeover for each call site individually; once
> that is complete device_legacy_reset() will be removed.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Acked-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Daniel P. Berrangé" <berrange@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: John Snow <jsnow@redhat.com>
> Cc: "Cédric Le Goater" <clg@kaod.org>
> Cc: Collin Walling <walling@linux.ibm.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Halil Pasic <pasic@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Dmitry Fleytman <dmitry.fleytman@gmail.com>
> Cc: Fam Zheng <fam@euphon.net>
> ---
>  hw/audio/intel-hda.c     | 2 +-
>  hw/core/qdev.c           | 6 +++---
>  hw/hyperv/hyperv.c       | 2 +-
>  hw/i386/pc.c             | 2 +-
>  hw/ide/microdrive.c      | 8 ++++----
>  hw/intc/spapr_xive.c     | 2 +-
>  hw/ppc/pnv_psi.c         | 2 +-
>  hw/ppc/spapr_pci.c       | 2 +-
>  hw/ppc/spapr_vio.c       | 2 +-
>  hw/s390x/s390-pci-inst.c | 2 +-
>  hw/scsi/vmw_pvscsi.c     | 2 +-
>  hw/sd/omap_mmc.c         | 2 +-
>  hw/sd/pl181.c            | 2 +-
>  include/hw/qdev-core.h   | 4 ++--
>  14 files changed, 20 insertions(+), 20 deletions(-)

Acked-by: Cornelia Huck <cohuck@redhat.com>



^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 02/13] hw/core/qdev: add trace events to help with resettable transition
  2019-10-18 15:06 ` [PATCH v5 02/13] hw/core/qdev: add trace events to help with resettable transition Damien Hedde
  2019-10-19 17:44   ` Richard Henderson
  2019-10-31 23:23   ` Philippe Mathieu-Daudé
@ 2019-12-03 10:57   ` Cornelia Huck
  2 siblings, 0 replies; 57+ messages in thread
From: Cornelia Huck @ 2019-12-03 10:57 UTC (permalink / raw)
  To: Damien Hedde
  Cc: peter.maydell, berrange, ehabkost, qemu-s390x, mark.burton,
	qemu-devel, edgari, qemu-arm, pbonzini, philmd, david

On Fri, 18 Oct 2019 17:06:19 +0200
Damien Hedde <damien.hedde@greensocs.com> wrote:

> Adds trace events to reset procedure and when updating the parent
> bus of a device.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>  hw/core/qdev.c       | 27 ++++++++++++++++++++++++---
>  hw/core/trace-events |  9 +++++++++
>  2 files changed, 33 insertions(+), 3 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>



^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 03/13] hw/core: create Resettable QOM interface
  2019-10-18 15:06 ` [PATCH v5 03/13] hw/core: create Resettable QOM interface Damien Hedde
  2019-11-29 18:32   ` Peter Maydell
@ 2019-12-03 11:16   ` Cornelia Huck
  1 sibling, 0 replies; 57+ messages in thread
From: Cornelia Huck @ 2019-12-03 11:16 UTC (permalink / raw)
  To: Damien Hedde
  Cc: peter.maydell, berrange, ehabkost, qemu-s390x, mark.burton,
	qemu-devel, edgari, qemu-arm, pbonzini, philmd, david

On Fri, 18 Oct 2019 17:06:20 +0200
Damien Hedde <damien.hedde@greensocs.com> wrote:

> This commit defines an interface allowing multi-phase reset. This aims
> to solve a problem of the actual single-phase reset (built in
> DeviceClass and BusClass): reset behavior is dependent on the order
> in which reset handlers are called. In particular doing external
> side-effect (like setting an qemu_irq) is problematic because receiving
> object may not be reset yet.
> 
> The Resettable interface divides the reset in 3 well defined phases.
> To reset an object tree, all 1st phases are executed then all 2nd then
> all 3rd. See the comments in include/hw/resettable.h for a more complete
> description. The interface defines 3 phases to let the future
> possibility of holding an object into reset for some time.
> 
> The qdev/qbus reset in DeviceClass and BusClass will be modified in
> following commits to use this interface. A mechanism is provided
> to allow executing a transitional reset handler in place of the 2nd
> phase which is executed in children-then-parent order inside a tree.
> This will allow to transition devices and buses smoothly while
> keeping the exact current qdev/qbus reset behavior for now.
> 
> Documentation will be added in a following commit.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> 
> In this patch only a single reset type is supported, but the interface
> allows for more to be defined.
> 
> I had some thought about problems which may arise when having multiple
> reset types:
> 
> - reset type propagation. Right now we propagate the same reset type
>   to the children. I don't think it will work that with multiple
>   types.
>   For example, if we add pci_bus_reset type: a pci device will
>   implement the reset type but not its children (they may have
>   nothing to do with pci).
>   This can be solved by changing the child_foreach method rules.
>   We should say that child_foreach may change the type it
>   propagates to its children (on a children by children basis).
>   For example, the pci device may just propagate cold reset type
>   to its children.
>   For this we need to pass the type as parameter to child_foreach()
>   method.
> 
> - are all children concerned ? For a given reset type, some child
>   may not need to be reset. As above we can handle that with
>   child_foreach: an resettable object can propagate the reset only
>   to a partial set of its child.
>   For this we need to know the type when we release the reset,
>   that's why I added it to resettable_release_reset() even if it
>   is unused right now.
>   I've also added an opaque parameter to child_foreach. I think
>   we will need that to handle the change of parent because we
>   will need to test if a child is concerned by a reset type: the
>   opaque will allow to use a test callback and get some result.

What about an optional ->filter() callback? That would be invoked if
existing prior to calling the child_foreach callback and could be used
to exclude children from the reset for this round for all callbacks. Or
have it modify the reset type (like in your pci reset -> cold reset
example above), and completely skip it if the reset type has been
modified to a 'no reset' type?

> 
> - several reset types at the same time. I don't another solution
>   than saying we execute *enter* and *hold* phase for every reset
>   type. *exit* will still be executed once for all at the end.
>   It will be up for each object to cope with it if it handle
>   multiple reset types. For *enter* is trivial, calling it twice
>   in a row is no problem given that it should only reset internal
>   state. For *hold* there may be some complication.
> 
> - Obviously we will need to at least an interface class field to hold
>   the supported reset types by the class. Also the reset state will
>   need some modification.
> ---
>  Makefile.objs           |   1 +
>  hw/core/Makefile.objs   |   1 +
>  hw/core/resettable.c    | 230 ++++++++++++++++++++++++++++++++++++++++
>  hw/core/trace-events    |  17 +++
>  include/hw/resettable.h | 199 ++++++++++++++++++++++++++++++++++
>  5 files changed, 448 insertions(+)
>  create mode 100644 hw/core/resettable.c
>  create mode 100644 include/hw/resettable.h



^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 06/13] hw/core/qdev: handle parent bus change regarding resettable
  2019-11-29 18:41   ` Peter Maydell
@ 2019-12-03 11:37     ` Cornelia Huck
  0 siblings, 0 replies; 57+ messages in thread
From: Cornelia Huck @ 2019-12-03 11:37 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Damien Hedde, Daniel P. Berrange, Eduardo Habkost, qemu-s390x,
	Mark Burton, QEMU Developers, Edgar Iglesias, qemu-arm,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

On Fri, 29 Nov 2019 18:41:26 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
> >
> > In qdev_set_parent_bus(), when changing the parent bus of a
> > realized device, if the source and destination buses are not in the
> > same reset state, some adaptation are required. This patch adds  
> 
> "adaptations"
> 
> > needed call to resettable_change_parent() to make sure a device reset
> > state stays coherent with its parent bus.
> >
> > The addition is a no-op if:
> > 1. the device being parented is not realized.
> > 2. the device is realized, but both buses are not under reset.
> >
> > Case 2 means that as long as qdev_set_parent_bus() is called
> > during the machine realization procedure (which is before the
> > machine reset so nothing is in reset), it is a no op.
> >
> > There are 49 call sites of qdev_set_parent_bus(). All but one fall
> > into the no-op case:
> > + 28 calls related to virtio (in hw/{s390x,display,virtio}/
> >   {vhost,virtio}-xxx.c) to set a _vdev_/_vgpu_ composing device
> >   parent bus just before realizing the _vdev_/_vgpu_.
> > + hw/qdev.c: when creating a device in qdev_try_create()
> > + hw/sysbus.c: when initializing a device in the sysbus
> > + hw/display/virtio-gpu-pci.c: before realizing VirtIOGPUPCIBase/vgpu
> > + hw/display/virtio-vga.c: before realizing VirtIOVGABase/vgpu
> > + hw/i386/amd_iommu.c: before realizing AMDVIState/pci
> > + hw/misc/auxbus.c: when creating an AUXBus
> > + hw/misc/auxbus.c: when creating an AUXBus child
> > + hw/misc/macio/macio.c: when initializing a MACIOState child
> > + hw/misc/macio/macio.c: before realizing NewWorldMacIOState/pmu
> > + hw/misc/macio/macio.c: before realizing NewWorldMacIOState/cuda
> > + hw/pci-host/designware.c: before realizing DesignwarePCIEHost/root
> > + hw/pci-host/gpex.c: before realizing GPEXHost/root
> > + hw/pci-host/prep.c: when initializaing PREPPCIState/pci_dev
> > + hw/pci-host/q35.c: before realizing Q35PCIHost/mch
> > + hw/pci-host/versatile.c: when initializing PCIVPBState/pci_dev
> > + hw/pci-host/xilinx-pcie.c: before realizing XilinxPCIEHost/root
> > + hw/s390x/event-facility.c: when creating SCLPEventFacility/
> >                              TYPE_SCLP_QUIESCE
> > + hw/s390x/event-facility.c: ditto with SCLPEventFacility/
> >                              TYPE_SCLP_CPU_HOTPLUG
> > + hw/s390x/sclp.c: Not trivial because it is called on a SLCPDevice
> >   just after realizing it. Ok because at this point the destination
> >   bus (sysbus) is not in reset; the realize step is before the
> >   machine reset.
> > + hw/sd/core.c: Not OK. Used in sdbus_reparent_card(). See below.
> > + hw/ssi/ssi.c: Used to put spi slave on spi bus and connect the cs
> >   line in ssi_auto_connect_slave(). Ok because this function is only
> >   used in realize step in hw/ssi/aspeed_smc.ci, hw/ssi/imx_spi.c,
> >   hw/ssi/mss-spi.c, hw/ssi/xilinx_spi.c and hw/ssi/xilinx_spips.c.
> > + hw/xen/xen-legacy-backend.c: when creating a XenLegacyDevice device
> > + qdev-monitor.c: in device hotplug creation procedure before realize  
> 
> This is a really useful analysis to have in the commit message;
> thanks!
> 
> (Side note: I wonder if the sclp.c case could be reordered so
> it realizes the device after parenting it? Anyway, not something
> to worry about now.)

As far as I can see, that should work. This code is a bit weird anyway;
the problem is that we need the sysbus somewhere in there... I'm
wondering if that can be handled in a different way. But agreed, that
is something we can revisit later.

> 
> > Note that this commit alone will have no effect, right now there is no
> > use of resettable API to reset anything. So a bus will never be tagged
> > as in-reset by this same API.
> >
> > The one place where side-effect will occurs is in hw/sd/core.c in
> > sdbus_reparent_card(). This function is only used in the raspi machines,
> > including during the sysbus reset procedure. This case will be fixed by
> > a following commit before globally enabling resettable API for sysbus
> > reset.
> >
> > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>  
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> thanks
> -- PMM
> 



^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 11/13] hw/s390x/ipl: replace deprecated qdev_reset_all registration
  2019-10-18 15:06 ` [PATCH v5 11/13] hw/s390x/ipl: replace deprecated qdev_reset_all registration Damien Hedde
  2019-10-31 23:38   ` Philippe Mathieu-Daudé
  2019-11-29 19:02   ` Peter Maydell
@ 2019-12-03 11:41   ` Cornelia Huck
  2 siblings, 0 replies; 57+ messages in thread
From: Cornelia Huck @ 2019-12-03 11:41 UTC (permalink / raw)
  To: Damien Hedde
  Cc: peter.maydell, Thomas Huth, berrange, ehabkost, qemu-s390x,
	mark.burton, qemu-devel, Christian Borntraeger, edgari, qemu-arm,
	pbonzini, philmd, david

On Fri, 18 Oct 2019 17:06:28 +0200
Damien Hedde <damien.hedde@greensocs.com> wrote:

> Replace deprecated qdev_reset_all by resettable_cold_reset_fn for
> the ipl registration in the main reset handlers.
> 
> This does not impact the behavior for the following reasons:
> + at this point resettable just call the old reset methods of devices
>   and buses in the same order than qdev/qbus.
> + resettable handlers registered with qemu_register_reset are
>   serialized; there is no interleaving.
> + eventual explicit calls to legacy reset API (device_reset or
>   qdev/qbus_reset) inside this reset handler will not be masked out
>   by resettable mechanism; they do not go through resettable api.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: qemu-s390x@nongnu.org
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Thomas Huth <thuth@redhat.com>
> ---
>  hw/s390x/ipl.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>



^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 00/13] Multi-phase reset mechanism
  2019-10-18 15:06 [PATCH v5 00/13] Multi-phase reset mechanism Damien Hedde
                   ` (15 preceding siblings ...)
  2019-11-29 19:07 ` Peter Maydell
@ 2019-12-03 11:44 ` Cornelia Huck
  16 siblings, 0 replies; 57+ messages in thread
From: Cornelia Huck @ 2019-12-03 11:44 UTC (permalink / raw)
  To: Damien Hedde
  Cc: peter.maydell, berrange, ehabkost, qemu-s390x, mark.burton,
	qemu-devel, edgari, qemu-arm, pbonzini, philmd, david

On Fri, 18 Oct 2019 17:06:17 +0200
Damien Hedde <damien.hedde@greensocs.com> wrote:

> Hi all,
> 
> The purpose of this series is to split the current reset procedure
> into multiple phases. This will help to solve some ordering
> difficulties we have during reset. Previous version can be found here:
> https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg04359.html
> 
> This series adds resettable interface and transitions base Device and
> Bus classes (sysbus subclasses are ok too). It provides new reset
> functions but does not switch anymore the old functions
> (device_reset() and qdev/qbus_reset_all()) to resettable interface.
> These functions keep the exact same behavior as before.
> 
> The series also transition the main reset handlers registration which
> has no impact until devices and buses are transitioned.
> 
> I think this version is way better regarding the transition from the
> legacy to the resettable interface than the previous one.
> After this series, the plan is then to transition devices, buses and
> legacy reset call sites. Devices and buses have to be transitioned
> from mother class to daughter classes order but until the final
> (daughter) class is transitioned, old monolitic reset behavior will
> be kept for this class.

I have looked over this patchset a bit (with an eye to the s390 stuff).
Seems sane, although I currently don't have the resources to review
more in detail.



^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 09/13] docs/devel/reset.txt: add doc about Resettable interface
  2019-11-29 19:00   ` Peter Maydell
@ 2019-12-06 15:40     ` Damien Hedde
  2019-12-06 16:21     ` Damien Hedde
  1 sibling, 0 replies; 57+ messages in thread
From: Damien Hedde @ 2019-12-06 15:40 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Edgar Iglesias, qemu-arm,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson



On 11/29/19 8:00 PM, Peter Maydell wrote:
> On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> 
> Subject line still says ".txt".
> 
>> ---
>>
>> Should I add an entry into index.rst ?
> 
> Yes, please.
> 
> I have one substantive comment on this patch: in the
> MyDev example of implementing reset, you make the enter/hold/exit
> functions do the reset of the subclass and then call the
> myclass->parent_phases method. This feels to me like it's the
> wrong way round and the parent class should reset first. Does
> it make any difference to do it this way round?
> 

Not really, I'll update them to what you say.

--
Damien


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v5 09/13] docs/devel/reset.txt: add doc about Resettable interface
  2019-11-29 19:00   ` Peter Maydell
  2019-12-06 15:40     ` Damien Hedde
@ 2019-12-06 16:21     ` Damien Hedde
  1 sibling, 0 replies; 57+ messages in thread
From: Damien Hedde @ 2019-12-06 16:21 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Daniel P. Berrange, Eduardo Habkost, qemu-s390x, Cornelia Huck,
	Mark Burton, QEMU Developers, Edgar Iglesias, qemu-arm,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson



On 11/29/19 8:00 PM, Peter Maydell wrote:
> On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> 
> Subject line still says ".txt".
> 
>> ---
>>
>> +Polling the reset state
>> +.......................
>> +
>> +Resettable interface provides the ``resettable_is_in_reset()`` function.
>> +This function returns true if the object parameter is currently under reset.
>> +
>> +An object is under reset from the beginning of the *init* phase to the end of
>> +the *exit* phase. During all three phases, the function will return that the
>> +object is in reset.
>> +
>> +This function may be used if behavior has to be adapted when receiving external
>> +interaction while in reset state. Note that until migration of the reset state
>> +is supported, an object should not be left in reset. So apart from being
>> +currently executing one of a reset phase, the only cases when this
>> +function should return true is if an external interaction is made during *hold*
>> +or *exit* phase of another object in the same reset group.
> 
> This paragraph feels a bit unclear to me but I'm not sure how to improve it.
> 

If I add an example like this is it better ?

This function may be used if the object behavior has to be adapted
while in reset state. For example if a device has an irq input,
it will probably need to ignore it while in reset; then it can for
example check the reset state at the beginning of the irq callback.

Note that until migration of the reset state is supported, an object
should not be left in reset. So apart from being currently executing
one of the reset phases, the only cases when this function will return
true is if an external interaction (like changing an io) is made during
*hold* or *exit* phase of another object in the same reset group.

--
Damien



^ permalink raw reply	[flat|nested] 57+ messages in thread

end of thread, other threads:[~2019-12-06 18:12 UTC | newest]

Thread overview: 57+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-18 15:06 [PATCH v5 00/13] Multi-phase reset mechanism Damien Hedde
2019-10-18 15:06 ` [PATCH v5 01/13] add device_legacy_reset function to prepare for reset api change Damien Hedde
2019-10-19 17:35   ` Richard Henderson
2019-12-03 10:55   ` Cornelia Huck
2019-10-18 15:06 ` [PATCH v5 02/13] hw/core/qdev: add trace events to help with resettable transition Damien Hedde
2019-10-19 17:44   ` Richard Henderson
2019-10-31 23:23   ` Philippe Mathieu-Daudé
2019-11-04 12:16     ` Damien Hedde
2019-11-04 14:33       ` Philippe Mathieu-Daudé
2019-12-03 10:57   ` Cornelia Huck
2019-10-18 15:06 ` [PATCH v5 03/13] hw/core: create Resettable QOM interface Damien Hedde
2019-11-29 18:32   ` Peter Maydell
2019-12-02 11:07     ` Damien Hedde
2019-12-02 11:14       ` Peter Maydell
2019-12-03 11:16   ` Cornelia Huck
2019-10-18 15:06 ` [PATCH v5 04/13] hw/core: add Resettable support to BusClass and DeviceClass Damien Hedde
2019-10-19 18:49   ` Richard Henderson
2019-11-29 18:36   ` Peter Maydell
2019-12-02 11:38     ` Damien Hedde
2019-10-18 15:06 ` [PATCH v5 05/13] hw/core/resettable: add support for changing parent Damien Hedde
2019-11-29 18:38   ` Peter Maydell
2019-12-02 11:43     ` Damien Hedde
2019-10-18 15:06 ` [PATCH v5 06/13] hw/core/qdev: handle parent bus change regarding resettable Damien Hedde
2019-11-29 18:41   ` Peter Maydell
2019-12-03 11:37     ` Cornelia Huck
2019-10-18 15:06 ` [PATCH v5 07/13] hw/core/qdev: update hotplug reset " Damien Hedde
2019-11-08 15:14   ` Damien Hedde
2019-10-18 15:06 ` [PATCH v5 08/13] hw/core: deprecate old reset functions and introduce new ones Damien Hedde
2019-10-31 23:35   ` Philippe Mathieu-Daudé
2019-11-04 12:01     ` Damien Hedde
2019-11-04 15:42       ` Philippe Mathieu-Daudé
2019-11-29 18:42   ` Peter Maydell
2019-10-18 15:06 ` [PATCH v5 09/13] docs/devel/reset.txt: add doc about Resettable interface Damien Hedde
2019-11-29 19:00   ` Peter Maydell
2019-12-06 15:40     ` Damien Hedde
2019-12-06 16:21     ` Damien Hedde
2019-10-18 15:06 ` [PATCH v5 10/13] vl: replace deprecated qbus_reset_all registration Damien Hedde
2019-11-29 19:01   ` Peter Maydell
2019-10-18 15:06 ` [PATCH v5 11/13] hw/s390x/ipl: replace deprecated qdev_reset_all registration Damien Hedde
2019-10-31 23:38   ` Philippe Mathieu-Daudé
2019-11-29 19:02   ` Peter Maydell
2019-12-03 11:41   ` Cornelia Huck
2019-10-18 15:06 ` [PATCH v5 12/13] hw/gpio/bcm2835_gpio: Isolate sdbus reparenting Damien Hedde
2019-11-29 19:05   ` Peter Maydell
2019-12-02 12:27     ` Damien Hedde
2019-12-02 12:33       ` Peter Maydell
2019-12-02 13:05         ` Damien Hedde
2019-12-02 13:10           ` Peter Maydell
2019-10-18 15:06 ` [PATCH v5 13/13] hw/gpio/bcm2835_gpio: Update to resettable Damien Hedde
2019-11-29 19:02   ` Peter Maydell
2019-10-19  9:01 ` [PATCH v5 00/13] Multi-phase reset mechanism no-reply
2019-10-29 15:53 ` Damien Hedde
2019-11-08 15:26   ` Damien Hedde
2019-11-08 15:28     ` Peter Maydell
2019-11-08 15:58       ` Damien Hedde
2019-11-29 19:07 ` Peter Maydell
2019-12-03 11:44 ` Cornelia Huck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).