QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Qemu-devel] [PATCH v4 00/10] Multi-phase reset mechanism
@ 2019-08-21 16:33 Damien Hedde
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 01/10] add device_legacy_reset function to prepare for reset api change Damien Hedde
                   ` (11 more replies)
  0 siblings, 12 replies; 20+ messages in thread
From: Damien Hedde @ 2019-08-21 16:33 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, edgar.iglesias, berrange, ehabkost,
	mark.burton, pbonzini, philmd, david

Hi all,

Here's the 4th version of the multi-phase reset proposal patches.
Previous version can be found here:
https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg06365.html

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. Please see patch 4 which adds documentation for more details.

Compared to previous version, I've reduced the scope of the series to
multi-phase basics. In particular, I've removed migration-related features
which can be added and discussed later when we've settled the api. I've also
not included the change to the new api for the ~20 impacted files because it
highly depends on whether we handle cold vs warm reset difference or not. I'll
handle them when we'll advance on this point.

I've isolated in patch 2 to 4, the multi-phase base mechanism handling only
cold reset as suggested by David.
Patches 5 and 6 do trivial modifications related to the registration of reset
handler in the main system reset.
Patch 7 handles the hotplug device reset case.
These first 7 patches form a multi-phase basics independent subset; the first 6
do not modify the behavior. I'd really like to move forward on them as we have
things depending on the basic multi-phase capability only (in particular the
clock-tree support).

Nevertheless I've kept in this series the addition of warm reset (in patches 8
to 10) so we can continue the discussion. Even if we don't introduce warm
reset, it allows discussion about multiple reset type support.
Maybe I should have put these 3 patches in their own (rfc ?) series, since
there was lot of questioning about the warm reset and talking about handling
other resets like bus specific ones.

I've tested that the actual reset order was not modified by these patches by
tracing calls to individual reset method after and before applying the series.


Changes v3 -> v4
general:
    + various comments and typos
patch 1: add device_legacy_reset function to prepare for reset api change
    + squash of 2 patches from v3 (asked by Peter and David)
patch 2: Create Resettable QOM interface
    + ResetType enum (only cold for now) (David and Philippe's remark)
    + ResetState to factorize most of the code (to address David's concern)
    + all phases order is now children-to-parent (Peter's remark)
    + assert/deassert removed to isolate migration related features
patch 3: add Resettable interface in Bus and Device classes
    + squash of 2 patches (make Device and Bus Resettable & switch to
      resettable api) since patch 2 has reduced their size.
    + adaptation to patch 2 changes (warm reset, state and methods)
    + isolate hotplug reset change into patch 7 (Peter's remark)
    + fix qdev/qbus_reset_not doing a cold reset (Peter's remark)
    + call helper device_reset_cold instead of device_reset (David's remark)
patch 4: docs/devel/reset.txt: create doc about Resettable interface
    + various improvements
    + in this patch, doc is reduced to cold multi-phase reset with no "in reset"
      state (other parts are kept for following commits)
patch 5: vl.c: replace deprecated qbus_reset_all registration
    + suggested comment improvement from Peter
patch 6: hw/s390x/ipl.c: replace deprecated qbus_reset_all registration
    + suggested comment improvement from Peter

Thanks for your feedback,
Damien

Damien Hedde (10):
  add device_legacy_reset function to prepare for reset api change
  hw/core: create Resettable QOM interface
  hw/core: add Resettable interface in Bus and Device classes
  docs/devel/reset.txt: create doc about Resettable interface
  vl.c: replace deprecated qbus_reset_all registration
  hw/s390x/ipl.c: replace deprecated qbus_reset_all registration
  hw/core/qdev: replace deprecated device_legacy_reset when hotplugging
    device
  hw/core/resettable: add support for warm reset
  hw/core/: add warm reset helpers for devices and buses
  docs/devel/reset.txt: add documentation about warm reset

 Makefile.objs            |   1 +
 docs/devel/reset.txt     | 284 +++++++++++++++++++++++++++++++++++++++
 hw/audio/intel-hda.c     |   2 +-
 hw/core/Makefile.objs    |   1 +
 hw/core/bus.c            |  64 +++++++++
 hw/core/qdev.c           |  86 +++++++++---
 hw/core/resettable.c     | 199 +++++++++++++++++++++++++++
 hw/core/trace-events     |  36 +++++
 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   | 100 +++++++++++++-
 include/hw/resettable.h  | 171 +++++++++++++++++++++++
 tests/Makefile.include   |   1 +
 vl.c                     |  10 +-
 24 files changed, 949 insertions(+), 44 deletions(-)
 create mode 100644 docs/devel/reset.txt
 create mode 100644 hw/core/resettable.c
 create mode 100644 hw/core/trace-events
 create mode 100644 include/hw/resettable.h

-- 
2.22.0



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

* [Qemu-devel] [PATCH v4 01/10] add device_legacy_reset function to prepare for reset api change
  2019-08-21 16:33 [Qemu-devel] [PATCH v4 00/10] Multi-phase reset mechanism Damien Hedde
@ 2019-08-21 16:33 ` Damien Hedde
  2019-08-24  9:50   ` David Gibson
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 02/10] hw/core: create Resettable QOM interface Damien Hedde
                   ` (10 subsequent siblings)
  11 siblings, 1 reply; 20+ messages in thread
From: Damien Hedde @ 2019-08-21 16:33 UTC (permalink / raw)
  To: qemu-devel
  Cc: Fam Zheng, peter.maydell, Collin Walling, Dmitry Fleytman,
	Michael S. Tsirkin, Gerd Hoffmann, edgar.iglesias, qemu-block,
	David Hildenbrand, Halil Pasic, Christian Borntraeger, david,
	philmd, ehabkost, qemu-s390x, qemu-arm, Cédric Le Goater,
	John Snow, Richard Henderson, Damien Hedde, berrange,
	Cornelia Huck, mark.burton, qemu-ppc, 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>
---
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>
Cc: qemu-block@nongnu.org
Cc: qemu-ppc@nongnu.org
Cc: qemu-s390x@nongnu.org
Cc: qemu-arm@nongnu.org
---
 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 60d66c2f39..a95d4fa87d 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -257,7 +257,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;
 }
@@ -865,7 +865,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;
 
@@ -1087,7 +1087,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 3ab4bcb3ca..f33a8de42f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2826,7 +2826,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 aad981cb78..a0f1787ee0 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -1514,7 +1514,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 88ba8e7b9b..bfdc30d4e1 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 bf31fd854c..f4c918be8b 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -2031,7 +2031,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 0803649658..ff7ee4d822 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -305,7 +305,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 00235148be..93cda37c27 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -242,7 +242,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 de70b7a19a..09e6dfd664 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -406,11 +406,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.22.0



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

* [Qemu-devel] [PATCH v4 02/10] hw/core: create Resettable QOM interface
  2019-08-21 16:33 [Qemu-devel] [PATCH v4 00/10] Multi-phase reset mechanism Damien Hedde
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 01/10] add device_legacy_reset function to prepare for reset api change Damien Hedde
@ 2019-08-21 16:33 ` Damien Hedde
  2019-09-11  8:06   ` David Gibson
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 03/10] hw/core: add Resettable interface in Bus and Device classes Damien Hedde
                   ` (9 subsequent siblings)
  11 siblings, 1 reply; 20+ messages in thread
From: Damien Hedde @ 2019-08-21 16:33 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, edgar.iglesias, berrange, ehabkost,
	mark.burton, 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. There is 3 phases to allow a device to be held in reset
state; the ability to control this state will be added in a following
commits.

The qdev/qbus reset in DeviceClass and BusClass will be modified in
following commits to use this interface.
No change of behavior is expected because the init phase execution order
follows the children-then-parent order inside a tree. Since this is the
actual order of qdev/qbus reset, we will be able to map current reset
handlers on init phase for example.

In this patch only cold reset is introduced, which is pretty much the
actual semantics of the current reset handlers. The interface can be
extended to support more reset types.

Documentation will be added in a following commit.

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

I kept the non-recursive count approach (a given object counts reset
initiated on it as well as reset initiated on its parents in reset
hierarchy). I implemented the other approach, it is possible but is more
complex (an object has to know its direct parent(s) and we need to scan
the reset hierarchy to know if we are in reset) so I prefer not
to introduce it here.
Moreover I think it has drawbacks if we want to handle complex reset
use cases with more reset type.
Anyway, as long as we don't migrate the reset-related state, there is
no problem to switch between approaches.
---
 Makefile.objs           |   1 +
 hw/core/Makefile.objs   |   1 +
 hw/core/resettable.c    | 186 ++++++++++++++++++++++++++++++++++++++++
 hw/core/trace-events    |  36 ++++++++
 include/hw/resettable.h | 159 ++++++++++++++++++++++++++++++++++
 5 files changed, 383 insertions(+)
 create mode 100644 hw/core/resettable.c
 create mode 100644 hw/core/trace-events
 create mode 100644 include/hw/resettable.h

diff --git a/Makefile.objs b/Makefile.objs
index 6a143dcd57..a723a47e14 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 b49f880a0c..69b408ad1c 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..b534c2c7a4
--- /dev/null
+++ b/hw/core/resettable.c
@@ -0,0 +1,186 @@
+/*
+ * 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"
+
+#define RESETTABLE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE_INTERFACE)
+
+static void resettable_foreach_child(ResettableClass *rc,
+                                     Object *obj,
+                                     void (*func)(Object *, ResetType type),
+                                     ResetType type)
+{
+    if (rc->foreach_child) {
+        rc->foreach_child(obj, func, type);
+    }
+}
+
+static void resettable_init_reset(Object *obj, ResetType type)
+{
+    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
+    ResetState *s = rc->get_state(obj);
+    bool action_needed = false;
+
+    /* 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;
+    }
+    s->count += 1;
+    /*
+     * 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);
+    trace_resettable_phase_init(obj, object_get_typename(obj), type,
+                                s->count, action_needed);
+
+    /*
+     * handle the children even if action_needed is at false so that
+     * children counts are incremented too
+     */
+    resettable_foreach_child(rc, obj, resettable_init_reset, type);
+
+    /* exec init phase */
+    if (action_needed) {
+        s->hold_phase_needed = true;
+        if (rc->phases.init) {
+            rc->phases.init(obj, type);
+        }
+    }
+    trace_resettable_phase_init_end(obj);
+}
+
+static void resettable_hold_reset_child(Object *obj, ResetType type)
+{
+    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
+    ResetState *s = rc->get_state(obj);
+
+    trace_resettable_phase_hold(obj, object_get_typename(obj));
+
+    /* handle children first */
+    resettable_foreach_child(rc, obj, resettable_hold_reset_child, 0);
+
+    /* exec hold phase */
+    if (s->hold_phase_needed) {
+        s->hold_phase_needed = false;
+        if (rc->phases.hold) {
+            rc->phases.hold(obj);
+        }
+    }
+    trace_resettable_phase_hold_end(obj, s->hold_phase_needed);
+}
+
+static void resettable_hold_reset(Object *obj)
+{
+    /* we don't care of 2nd argument here */
+    resettable_hold_reset_child(obj, 0);
+}
+
+static void resettable_exit_reset_child(Object *obj, ResetType type)
+{
+    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
+    ResetState *s = rc->get_state(obj);
+
+    trace_resettable_phase_exit(obj, object_get_typename(obj));
+
+    resettable_foreach_child(rc, obj, resettable_exit_reset_child, 0);
+
+    /*
+     * we could assert that count > 0 but there are some corner cases
+     * where we prefer to let it go as it is probably harmless.
+     * For example: if there is reset support addition between
+     * hosts when doing a migration. We may do such things as
+     * deassert a non-existing reset.
+     */
+    if (s->count > 0) {
+        s->count -= 1;
+    } else {
+        trace_resettable_count_underflow(obj);
+    }
+    if (s->count == 0) {
+        if (rc->phases.exit) {
+            rc->phases.exit(obj);
+        }
+    }
+    trace_resettable_phase_exit_end(obj, s->count);
+}
+
+static void resettable_exit_reset(Object *obj)
+{
+    /* we don't care of 2nd argument here */
+    resettable_exit_reset_child(obj, 0);
+}
+
+void resettable_reset(Object *obj, ResetType type)
+{
+    /* TODO: change that when adding support for other reset types */
+    assert(type == RESET_TYPE_COLD);
+    trace_resettable_reset(obj, type);
+    resettable_init_reset(obj, type);
+    resettable_hold_reset(obj);
+    resettable_exit_reset(obj);
+}
+
+void resettable_cold_reset_fn(void *opaque)
+{
+    resettable_reset((Object *) opaque, RESET_TYPE_COLD);
+}
+
+bool resettable_is_resetting(Object *obj)
+{
+    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
+    ResetState *s = rc->get_state(obj);
+
+    return (s->count > 0);
+}
+
+void resettable_class_set_parent_phases(ResettableClass *rc,
+                                        ResettableInitPhase init,
+                                        ResettableHoldPhase hold,
+                                        ResettableExitPhase exit,
+                                        ResettablePhases *parent_phases)
+{
+    *parent_phases = rc->phases;
+    if (init) {
+        rc->phases.init = init;
+    }
+    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
new file mode 100644
index 0000000000..ecf966c314
--- /dev/null
+++ b/hw/core/trace-events
@@ -0,0 +1,36 @@
+# See docs/devel/tracing.txt for syntax documentation.
+#
+# This file is processed by the tracetool script during the build.
+#
+# To add a new trace event:
+#
+# 1. Choose a name for the trace event.  Declare its arguments and format
+#    string.
+#
+# 2. Call the trace event from code using trace_##name, e.g. multiwrite_cb() ->
+#    trace_multiwrite_cb().  The source file must #include "trace.h".
+#
+# Format of a trace event:
+#
+# [disable] <name>(<type1> <arg1>[, <type2> <arg2>] ...) "<format-string>"
+#
+# Example: g_malloc(size_t size) "size %zu"
+#
+# The "disable" keyword will build without the trace event.
+#
+# The <name> must be a valid as a C function name.
+#
+# Types should be standard C types.  Use void * for pointers because the trace
+# system may not have the necessary headers included.
+#
+# The <format-string> should be a sprintf()-compatible format string.
+
+# resettable.c
+resettable_reset(void *obj, int cold) "obj=%p cold=%d"
+resettable_phase_init(void *obj, const char *type, int cold, uint32_t count, int needed) "obj=%p(%s) cold=%d count=%" PRIu32 " needed=%d"
+resettable_phase_init_end(void *obj) "obj=%p"
+resettable_phase_hold(void *obj, const char *type) "obj=%p(%s)"
+resettable_phase_hold_end(void *obj, int needed) "obj=%p needed=%d"
+resettable_phase_exit(void *obj, const char *type) "obj=%p(%s)"
+resettable_phase_exit_end(void *obj, uint32_t count) "obj=%p count=%" PRIu32
+resettable_count_underflow(void *obj) "obj=%p"
diff --git a/include/hw/resettable.h b/include/hw/resettable.h
new file mode 100644
index 0000000000..5808c3c187
--- /dev/null
+++ b/include/hw/resettable.h
@@ -0,0 +1,159 @@
+#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)
+
+typedef struct ResetState ResetState;
+
+/**
+ * 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,
+ * ResetState 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 ResetState 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.init, 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 'init', 'hold', 'exit'.
+ * An object will always move quickly from 'init' 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
+ * 'init' method has been called.
+ *
+ * Users of a resettable object should not call these methods
+ * directly, but instead use the function resettable_reset().
+ *
+ * @phases.init: 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.init 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 ResetState.
+ *
+ * @foreach_child: Executes a given function 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
+ * an additional ResetType argument which is passed to foreach_child.
+ */
+typedef void (*ResettableInitPhase)(Object *obj, ResetType type);
+typedef void (*ResettableHoldPhase)(Object *obj);
+typedef void (*ResettableExitPhase)(Object *obj);
+typedef ResetState * (*ResettableGetState)(Object *obj);
+typedef void (*ResettableForeachChild)(Object *obj,
+                                       void (*func)(Object *, ResetType type),
+                                       ResetType type);
+typedef struct ResettableClass {
+    InterfaceClass parent_class;
+
+    struct ResettablePhases {
+        ResettableInitPhase init;
+        ResettableHoldPhase hold;
+        ResettableExitPhase exit;
+    } phases;
+
+    ResettableGetState get_state;
+    ResettableForeachChild foreach_child;
+} ResettableClass;
+typedef struct ResettablePhases ResettablePhases;
+
+/**
+ * ResetState:
+ * 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_needed: flag which indicates that we need to invoke the 'hold'
+ * phase handler for this object.
+ */
+struct ResetState {
+    uint32_t count;
+    bool hold_phase_needed;
+};
+
+/**
+ * resettable_is_resetting:
+ * Return true if @obj is under reset.
+ *
+ * @obj must implement Resettable interface.
+ */
+bool resettable_is_resetting(Object *obj);
+
+/**
+ * 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_deassert_reset.
+ */
+void resettable_reset(Object *obj, ResetType type);
+
+/**
+ * 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:
+ *
+ * Save @rc current reset phases into @parent_phases and override @rc phases
+ * by the given new methods (@init, @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,
+                                        ResettableInitPhase init,
+                                        ResettableHoldPhase hold,
+                                        ResettableExitPhase exit,
+                                        ResettablePhases *parent_phases);
+
+#endif
-- 
2.22.0



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

* [Qemu-devel] [PATCH v4 03/10] hw/core: add Resettable interface in Bus and Device classes
  2019-08-21 16:33 [Qemu-devel] [PATCH v4 00/10] Multi-phase reset mechanism Damien Hedde
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 01/10] add device_legacy_reset function to prepare for reset api change Damien Hedde
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 02/10] hw/core: create Resettable QOM interface Damien Hedde
@ 2019-08-21 16:33 ` Damien Hedde
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 04/10] docs/devel/reset.txt: create doc about Resettable interface Damien Hedde
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 20+ messages in thread
From: Damien Hedde @ 2019-08-21 16:33 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, edgar.iglesias, berrange, ehabkost,
	mark.burton, pbonzini, philmd, david

Implement Resettable interface in BusClass and DeviceClass; it adds the
ResetState structure into BusState and DeviceState and the required methods
in the classes.

Helpers device_cold_reset and bus_cold_reset are defined to call to
use the new interface. Also add a [device|bus]_is_resetting telling if the
device/bus is currently under reset.

Compatibility with existing code base is ensured; The legacy bus or device
reset method is called in the new init phase. Since this phase is called
for children before for parent in qbus hierarchy, the new reset behavior
will be the same as qdev/qbus_reset_all functions; qdev/qbus_reset_all
are now wrappers around device/bus_cold_reset functions.

Deprecate old reset apis (device_legacy_reset and qdev/qbus_reset_all).
These functions will be removed when transition to the new api is complete.

At this point no migration support is required since there is no way to
held an object into reset. This will be added later on.

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

I've kept the device/bus_cold_reset name; I think that even if we don't
introduce other reset types (like warm), it has to advantage to clarify
that current reset is "cold" (it put back what is the initial state when
qemu starts).
---
 hw/core/bus.c          | 53 +++++++++++++++++++++++++++++++
 hw/core/qdev.c         | 71 +++++++++++++++++++++++++++++++-----------
 include/hw/qdev-core.h | 62 +++++++++++++++++++++++++++++++++---
 tests/Makefile.include |  1 +
 4 files changed, 164 insertions(+), 23 deletions(-)

diff --git a/hw/core/bus.c b/hw/core/bus.c
index 7f3d2a3dbd..3be65ad041 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -22,6 +22,7 @@
 #include "qemu/ctype.h"
 #include "qemu/module.h"
 #include "qapi/error.h"
+#include "hw/resettable.h"
 
 void qbus_set_hotplug_handler(BusState *bus, Object *handler, Error **errp)
 {
@@ -68,6 +69,34 @@ int qbus_walk_children(BusState *bus,
     return 0;
 }
 
+void bus_cold_reset(BusState *bus)
+{
+    resettable_reset(OBJECT(bus), RESET_TYPE_COLD);
+}
+
+bool bus_is_resetting(BusState *bus)
+{
+    return resettable_is_resetting(OBJECT(bus));
+}
+
+static ResetState *bus_get_reset_state(Object *obj)
+{
+    BusState *bus = BUS(obj);
+    return &bus->reset;
+}
+
+static void bus_reset_foreach_child(Object *obj,
+                                    void (*func)(Object *, ResetType),
+                                    ResetType type)
+{
+    BusState *bus = BUS(obj);
+    BusChild *kid;
+
+    QTAILQ_FOREACH(kid, &bus->children, sibling) {
+        func(OBJECT(kid->child), type);
+    }
+}
+
 static void qbus_realize(BusState *bus, DeviceState *parent, const char *name)
 {
     const char *typename = object_get_typename(OBJECT(bus));
@@ -199,12 +228,32 @@ static char *default_bus_get_fw_dev_path(DeviceState *dev)
     return g_strdup(object_get_typename(OBJECT(dev)));
 }
 
+/**
+ * bus_legacy_reset_wrapper:
+ * Compatibility wrapper during the transition to multi-phase reset.
+ * This function should be deleted when all buses are converted.
+ */
+static void bus_legacy_reset_wrapper(Object *obj, ResetType type)
+{
+    BusState *bus = BUS(obj);
+    BusClass *bc = BUS_GET_CLASS(obj);
+
+    if (bc->reset) {
+        bc->reset(bus);
+    }
+}
+
 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->phases.init = bus_legacy_reset_wrapper;
+    rc->get_state = bus_get_reset_state;
+    rc->foreach_child = bus_reset_foreach_child;
 }
 
 static void qbus_finalize(Object *obj)
@@ -223,6 +272,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 a95d4fa87d..85c85822fd 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -255,25 +255,9 @@ HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
     return hotplug_ctrl;
 }
 
-static int qdev_reset_one(DeviceState *dev, void *opaque)
-{
-    device_legacy_reset(dev);
-
-    return 0;
-}
-
-static int qbus_reset_one(BusState *bus, void *opaque)
-{
-    BusClass *bc = BUS_GET_CLASS(bus);
-    if (bc->reset) {
-        bc->reset(bus);
-    }
-    return 0;
-}
-
 void qdev_reset_all(DeviceState *dev)
 {
-    qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one, NULL);
+    device_cold_reset(dev);
 }
 
 void qdev_reset_all_fn(void *opaque)
@@ -283,7 +267,7 @@ 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);
+    bus_cold_reset(bus);
 }
 
 void qbus_reset_all_fn(void *opaque)
@@ -292,6 +276,33 @@ 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_resetting(DeviceState *dev)
+{
+    return resettable_is_resetting(OBJECT(dev));
+}
+
+static ResetState *device_get_reset_state(Object *obj)
+{
+    DeviceState *dev = DEVICE(obj);
+    return &dev->reset;
+}
+
+static void device_reset_foreach_child(Object *obj,
+                                       void (*func)(Object *, ResetType),
+                                       ResetType type)
+{
+    DeviceState *dev = DEVICE(obj);
+    BusState *bus;
+    QLIST_FOREACH(bus, &dev->child_bus, sibling) {
+        func(OBJECT(bus), type);
+    }
+}
+
 /* can be used as ->unplug() callback for the simple cases */
 void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
                                   DeviceState *dev, Error **errp)
@@ -1047,9 +1058,25 @@ static void device_unparent(Object *obj)
     }
 }
 
+/**
+ * device_legacy_reset_wrapper:
+ * Compatibility wrapper during the transition to multi-phase reset.
+ * This function should be deleted when all devices are converted.
+ */
+static void device_legacy_reset_wrapper(Object *obj, ResetType type)
+{
+    DeviceState *dev = DEVICE(obj);
+    DeviceClass *dc = DEVICE_GET_CLASS(dev);
+
+    if (dc->reset) {
+        dc->reset(dev);
+    }
+}
+
 static void device_class_init(ObjectClass *class, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(class);
+    ResettableClass *rc = RESETTABLE_CLASS(class);
 
     class->unparent = device_unparent;
 
@@ -1061,6 +1088,10 @@ static void device_class_init(ObjectClass *class, void *data)
      */
     dc->hotpluggable = true;
     dc->user_creatable = true;
+
+    rc->phases.init = device_legacy_reset_wrapper;
+    rc->get_state = device_get_reset_state;
+    rc->foreach_child = device_reset_foreach_child;
 }
 
 void device_class_set_parent_reset(DeviceClass *dc,
@@ -1118,6 +1149,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 09e6dfd664..2976121fbc 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: ResetState 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;
+    ResetState reset;
 };
 
 struct DeviceListener {
@@ -195,6 +203,7 @@ typedef struct BusChild {
 /**
  * BusState:
  * @hotplug_handler: link to a hotplug handler associated with bus.
+ * @reset: ResetState 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;
+    ResetState reset;
 };
 
 /**
@@ -375,22 +385,55 @@ int qdev_walk_children(DeviceState *dev,
                        qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
                        void *opaque);
 
-void qdev_reset_all(DeviceState *dev);
-void qdev_reset_all_fn(void *opaque);
-
 /**
- * @qbus_reset_all:
- * @bus: Bus to be reset.
+ * qbus/qdev_reset_all:
+ * @bus (resp. @dev): Bus (resp. Device) to be reset.
  *
  * Reset @bus and perform a bus-level ("hard") reset of all devices connected
  * to it, including recursive processing of all buses below @bus itself.  A
  * 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.
+ *
+ * These functions are deprecated and are wrappers around device_cold_reset and
+ * bus_cold_reset. Please use these instead.
+ * TODO: remove them when all occurrences are removed
  */
+void qdev_reset_all(DeviceState *dev);
+void qdev_reset_all_fn(void *opaque);
 void qbus_reset_all(BusState *bus);
 void qbus_reset_all_fn(void *opaque);
 
+/**
+ * device_cold_reset:
+ * Trigger a cold reset of the device @dev.
+ *
+ * Use the Resettable interface (see hw/interface.h); it also reset the
+ * device's qdev/qbus subtree.
+ */
+void device_cold_reset(DeviceState *dev);
+
+/**
+ * bus_cold_reset:
+ * Trigger a cold reset of the bus @bus.
+ *
+ * Use the Resettable interface (see hw/interface.h); it also reset the
+ * bus's qdev/qbus subtree.
+ */
+void bus_cold_reset(BusState *bus);
+
+/**
+ * device_is_resetting:
+ * Return true if the device @dev is currently being reset.
+ */
+bool device_is_resetting(DeviceState *dev);
+
+/**
+ * bus_is_resetting:
+ * Return true if the bus @bus is currently being reset.
+ */
+bool bus_is_resetting(BusState *bus);
+
 /* This should go away once we get rid of the NULL bus hack */
 BusState *sysbus_get_default(void);
 
@@ -409,9 +452,18 @@ void qdev_machine_init(void);
  * device_legacy_reset:
  *
  * Reset a single device (by calling the reset method).
+ *
+ * This function is deprecated, please use Resettable api instead (eg:
+ * device_cold_reset())
+ * TODO: remove the function when all occurrences are removed.
  */
 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 6f02dfcc01..f0b4628cc6 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -561,6 +561,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/resettable.o \
 	hw/core/bus.o \
 	hw/core/irq.o \
 	hw/core/fw-path-provider.o \
-- 
2.22.0



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

* [Qemu-devel] [PATCH v4 04/10] docs/devel/reset.txt: create doc about Resettable interface
  2019-08-21 16:33 [Qemu-devel] [PATCH v4 00/10] Multi-phase reset mechanism Damien Hedde
                   ` (2 preceding siblings ...)
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 03/10] hw/core: add Resettable interface in Bus and Device classes Damien Hedde
@ 2019-08-21 16:33 ` Damien Hedde
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 05/10] vl.c: replace deprecated qbus_reset_all registration Damien Hedde
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 20+ messages in thread
From: Damien Hedde @ 2019-08-21 16:33 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, edgar.iglesias, berrange, ehabkost,
	mark.burton, pbonzini, philmd, david

It documents only the multi-phase mechanism with one reset possible type
(cold). Other features will be documented by further commits.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 docs/devel/reset.txt | 237 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 237 insertions(+)
 create mode 100644 docs/devel/reset.txt

diff --git a/docs/devel/reset.txt b/docs/devel/reset.txt
new file mode 100644
index 0000000000..77ff29b3d7
--- /dev/null
+++ b/docs/devel/reset.txt
@@ -0,0 +1,237 @@
+
+=====
+Reset
+=====
+
+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.
+
+
+Reset types
+-----------
+
+There are several kinds of reset. The most obvious one is the "cold" reset: a
+"cold" reset is the operation resulting of a power cycle (when we apply the
+power).
+
+By opposition, we call a "warm" reset, a reset operation not resulting of a
+power cycle; it can be triggered by a gpio or a software operation.
+
+Some buses also define specific kinds of reset.
+
+What does a reset is device-dependent. For most devices, cold or warm reset
+makes no difference. But there can be some; some configuration may be kept when
+applying a warm reset for example.
+
+The Resettable interface handles reset kinds with an enum. For now only cold
+reset is defined, others may be added later.
+```
+typedef enum ResetType {
+    RESET_TYPE_COLD,
+} ResetType;
+```
+
+In qemu, RESET_TYPE_COLD means we reset to the initial state corresponding to
+the start of qemu; this might differs from what is a read hardware cold reset.
+
+
+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 trigger a reset event on a resettable object with resettable_reset().
+The object will be instantly reset.
+
+```void resettable_reset(Object *obj, ResetType type);```
+The parameter "obj" is an object implementing the Resettable interface.
+The parameter "type" gives the type of reset you want to trigger.
+
+It is possible to interleave multiple calls to
+ - resettable_reset().
+
+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 by
+maintaining a count of reset.
+
+Note that 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:
+```
+void device_cold_reset(Device *dev);
+void bus_cold_reset(Bus *bus);
+```
+
+These are simple wrappers around resettable_reset() function; they only cast the
+Device or Bus into an Object and add the corresponding reset type.
+
+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.
+
+
+How it works: multi-phase reset
+-------------------------------
+
+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 3 well defined phases.
+
+When resetting a several objects (for example the whole machine at simulation
+startup), all 1st phases of all objects are executed, then all 2nd phases and
+then all 3rd phases.
+
+The 3 phases are:
+
+  1. INIT: This phase is executed 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.
+
+  2. HOLD: This phase is executed for entry into reset, once every object in the
+  system which is being reset has had its init phase executed. At this point
+  devices can do actions that affect other objects.
+
+  3. EXIT: This 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.
+init and hold phases are executed only when entering 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 new resettable object
+-----------------------------------------
+
+This section documents the APIs that an implementation of a resettable object
+must provide and what functions it has access to.
+
+There are three methods in the interface that must be implemented in an
+resettable object.
+The methods correspond to the three phases described in the previous section:
+```
+typedef void (*ResettableInitPhase)(Object *obj, ResetType type);
+typedef void (*ResettableHoldPhase)(Object *obj);
+typedef void (*ResettableExitPhase)(Object *obj);
+typedef struct ResettableClass {
+    InterfaceClass parent_class;
+
+    struct ResettablePhases {
+        ResettableInitPhase init;
+        ResettableHoldPhase hold;
+        ResettableExitPhase exit;
+    } phases;
+    [...]
+} ResettableClass;
+```
+
+All phases takes a pointer to the object as first argument. The init phase also
+takes the reset type.
+
+These methods should be updated when specializing an object. For this the
+helper function resettable_class_set_parent_phases() can be used to "backup"
+parent methods while changing the specialized ones:
+
+
+```
+void resettable_class_set_parent_reset_phases(ResettableClass *rc,
+                                              ResettableInitPhase init,
+                                              ResettableHoldPhase hold,
+                                              ResettableExitPhase exit,
+                                              ResettablePhases *parent_phases);
+```
+"rc" argument is the interface class structure; "init", "hold" and "exit" are
+the specialized phase methods for the object; and "parent_phases" is an
+allocated space (typically in the specialized object class) to backup the
+parent phases. This function only do the backup and update operation for phase
+arguments that are non-NULL; you can use it to specialize only the init method
+for example. When you specialize a method, it's on you to call or not the parent
+method inside the specialized one.
+
+If for some operation in the object, you need to know the reset state, there is
+a function to access that:
+```
+bool resettable_is_resetting(Object *obj);
+```
+
+resettable_is_resetting() tells if the resettable object is currently under
+reset.
+
+Helpers are defined for devices and buses that wrap resettable_is_resetting():
+```
+bool device_is_resetting(DeviceState *dev);
+bool bus_is_resetting(BusState *bus);
+```
+
+
+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.
+
+There are two other methods that need to exist in a class implementing the
+interface.
+
+```
+typedef struct ResetState {
+    uint32_t count;
+    bool hold_phase_needed;
+} ResetState;
+
+typedef ResetState *(*ResettableGetState)(Object *obj);
+typedef void (*ResettableForeachChild)(Object *obj,
+                                       void (*visitor)(Object *, ResetType),
+                                       ResetType type);
+typedef struct ResettableClass {
+    InterfaceClass parent_class;
+
+    [...]
+
+    ResettableGetState get_state;
+    ResettableForeachChild foreach_child;
+} ResettableClass;
+```
+
+get_state() must return a pointer to an allocated ResetState structure.
+This structure is used by the interface to store the information required
+to handle reset properly. This structure must not be modified by the object
+directly. The object must handle eventual allocation/deallocation of this
+structure during its creation and deletion. Typically it is located in the
+object state structure.
+
+The reset hierarchy is handled by means of the foreach_child() method. This
+method executes a given function on all reset children. An additional type
+argument is given to foreach_child() and must be passed to the function.
+
+In DeviceClass and BusClass the ResetState structure is located
+DeviceState/BusState structure. foreach_child() 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. So when we reset the main system bus,
+we reset the whole machine bus tree.
-- 
2.22.0



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

* [Qemu-devel] [PATCH v4 05/10] vl.c: replace deprecated qbus_reset_all registration
  2019-08-21 16:33 [Qemu-devel] [PATCH v4 00/10] Multi-phase reset mechanism Damien Hedde
                   ` (3 preceding siblings ...)
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 04/10] docs/devel/reset.txt: create doc about Resettable interface Damien Hedde
@ 2019-08-21 16:33 ` Damien Hedde
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 06/10] hw/s390x/ipl.c: " Damien Hedde
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 20+ messages in thread
From: Damien Hedde @ 2019-08-21 16:33 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, edgar.iglesias, berrange, ehabkost,
	mark.burton, pbonzini, philmd, david

Replace deprecated qbus_reset_all by resettable_cold_reset_fn for
the sysbus reset registration.

This does not impact the behavior: qbus_reset_all is already a wrapper
for the cold reset function.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 vl.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index edd5390110..8df686b333 100644
--- a/vl.c
+++ b/vl.c
@@ -4422,7 +4422,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.22.0



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

* [Qemu-devel] [PATCH v4 06/10] hw/s390x/ipl.c: replace deprecated qbus_reset_all registration
  2019-08-21 16:33 [Qemu-devel] [PATCH v4 00/10] Multi-phase reset mechanism Damien Hedde
                   ` (4 preceding siblings ...)
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 05/10] vl.c: replace deprecated qbus_reset_all registration Damien Hedde
@ 2019-08-21 16:33 ` " Damien Hedde
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 07/10] hw/core/qdev: replace deprecated device_legacy_reset when hotplugging device Damien Hedde
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 20+ messages in thread
From: Damien Hedde @ 2019-08-21 16:33 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, Thomas Huth, edgar.iglesias,
	berrange, ehabkost, Cornelia Huck, mark.burton,
	Christian Borntraeger, qemu-s390x, pbonzini, philmd, david

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

This does not impact the behavior: qbus_reset_all is already a wrapper
for the cold reset function.

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.22.0



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

* [Qemu-devel] [PATCH v4 07/10] hw/core/qdev: replace deprecated device_legacy_reset when hotplugging device
  2019-08-21 16:33 [Qemu-devel] [PATCH v4 00/10] Multi-phase reset mechanism Damien Hedde
                   ` (5 preceding siblings ...)
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 06/10] hw/s390x/ipl.c: " Damien Hedde
@ 2019-08-21 16:33 ` Damien Hedde
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 08/10] hw/core/resettable: add support for warm reset Damien Hedde
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 20+ messages in thread
From: Damien Hedde @ 2019-08-21 16:33 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, edgar.iglesias, berrange, ehabkost,
	mark.burton, pbonzini, philmd, david

Replace the hotplugged device_legacy_reset by device_cold_reset.
Since we're talking hotplug, cold reset seems the proper reset type.

This does change the behavior since the new function propagates
also the reset to eventual child buses. But it should be a bug if
resetting eventual child buses fail.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 hw/core/qdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 85c85822fd..1638bc9f3b 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -876,7 +876,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
             }
         }
         if (dev->hotplugged) {
-            device_legacy_reset(dev);
+            device_cold_reset(dev);
         }
         dev->pending_deleted_event = false;
 
-- 
2.22.0



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

* [Qemu-devel] [PATCH v4 08/10] hw/core/resettable: add support for warm reset
  2019-08-21 16:33 [Qemu-devel] [PATCH v4 00/10] Multi-phase reset mechanism Damien Hedde
                   ` (6 preceding siblings ...)
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 07/10] hw/core/qdev: replace deprecated device_legacy_reset when hotplugging device Damien Hedde
@ 2019-08-21 16:33 ` Damien Hedde
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 09/10] hw/core/: add warm reset helpers for devices and buses Damien Hedde
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 20+ messages in thread
From: Damien Hedde @ 2019-08-21 16:33 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, edgar.iglesias, berrange, ehabkost,
	mark.burton, pbonzini, philmd, david

Add the RESET_TYPE_WARM reset type.
Expand the actual implementation to support several types.

I used values which can be or'ed together. This way we can what reset
has been triggered.

Documentation is added in a following patch.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 hw/core/resettable.c    | 25 +++++++++++++++++++------
 include/hw/resettable.h | 22 +++++++++++++++++-----
 2 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/hw/core/resettable.c b/hw/core/resettable.c
index b534c2c7a4..80674292b3 100644
--- a/hw/core/resettable.c
+++ b/hw/core/resettable.c
@@ -34,12 +34,17 @@ static void resettable_init_reset(Object *obj, ResetType type)
     ResetState *s = rc->get_state(obj);
     bool action_needed = false;
 
-    /* Only take action if we really enter reset for the 1st time. */
+    /* ensure type is empty if no reset is in progress */
+    if (s->count == 0) {
+        s->type = 0;
+    }
+
     /*
-     * TODO: if adding more ResetType support, some additional checks
-     * are probably needed here.
+     * Only take action if:
+     * + we are not already in cold reset,
+     * + and we enter a new type of reset.
      */
-    if (s->count == 0) {
+    if ((s->type & RESET_TYPE_COLD) == 0 && (s->type & type) == 0) {
         action_needed = true;
     }
     s->count += 1;
@@ -62,6 +67,7 @@ static void resettable_init_reset(Object *obj, ResetType type)
 
     /* exec init phase */
     if (action_needed) {
+        s->type |= type;
         s->hold_phase_needed = true;
         if (rc->phases.init) {
             rc->phases.init(obj, type);
@@ -133,8 +139,7 @@ static void resettable_exit_reset(Object *obj)
 
 void resettable_reset(Object *obj, ResetType type)
 {
-    /* TODO: change that when adding support for other reset types */
-    assert(type == RESET_TYPE_COLD);
+    assert(type == RESET_TYPE_COLD || type == RESET_TYPE_WARM);
     trace_resettable_reset(obj, type);
     resettable_init_reset(obj, type);
     resettable_hold_reset(obj);
@@ -154,6 +159,14 @@ bool resettable_is_resetting(Object *obj)
     return (s->count > 0);
 }
 
+ResetType resettable_get_type(Object *obj)
+{
+    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
+    ResetState *s = rc->get_state(obj);
+
+    return s->type;
+}
+
 void resettable_class_set_parent_phases(ResettableClass *rc,
                                         ResettableInitPhase init,
                                         ResettableHoldPhase hold,
diff --git a/include/hw/resettable.h b/include/hw/resettable.h
index 5808c3c187..1e77cbd75b 100644
--- a/include/hw/resettable.h
+++ b/include/hw/resettable.h
@@ -12,15 +12,14 @@ typedef struct ResetState ResetState;
 
 /**
  * ResetType:
- * Types of reset.
+ * Types of reset, values can be OR'ed together.
  *
  * + Cold: reset resulting from a power cycle of the object.
- *
- * TODO: Support has to be added to handle more types. In particular,
- * ResetState structure needs to be expanded.
+ * + Warm: reset without power cycling.
  */
 typedef enum ResetType {
-    RESET_TYPE_COLD,
+    RESET_TYPE_COLD = 0x1,
+    RESET_TYPE_WARM = 0x2,
 } ResetType;
 
 /*
@@ -107,11 +106,13 @@ typedef struct ResettablePhases ResettablePhases;
  *
  * @count: Number of reset level the object is into. It is incremented when
  * the reset operation starts and decremented when it finishes.
+ * @type: Type of the in-progress reset. Valid only when count is non-zero.
  * @hold_phase_needed: flag which indicates that we need to invoke the 'hold'
  * phase handler for this object.
  */
 struct ResetState {
     uint32_t count;
+    ResetType type;
     bool hold_phase_needed;
 };
 
@@ -123,6 +124,17 @@ struct ResetState {
  */
 bool resettable_is_resetting(Object *obj);
 
+/**
+ * resettable_get_type:
+ * Return the current type of reset @obj is under.
+ *
+ * @obj must implement Resettable interface. Result is only valid if
+ * @resettable_is_resetting is true.
+ *
+ * Note: this return an OR'ed value of type if several reset were triggered
+ */
+ResetType resettable_get_type(Object *obj);
+
 /**
  * resettable_reset:
  * Trigger a reset on a object @obj of type @type. @obj must implement
-- 
2.22.0



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

* [Qemu-devel] [PATCH v4 09/10] hw/core/: add warm reset helpers for devices and buses
  2019-08-21 16:33 [Qemu-devel] [PATCH v4 00/10] Multi-phase reset mechanism Damien Hedde
                   ` (7 preceding siblings ...)
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 08/10] hw/core/resettable: add support for warm reset Damien Hedde
@ 2019-08-21 16:33 ` Damien Hedde
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 10/10] docs/devel/reset.txt: add documentation about warm reset Damien Hedde
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 20+ messages in thread
From: Damien Hedde @ 2019-08-21 16:33 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, edgar.iglesias, berrange, ehabkost,
	mark.burton, pbonzini, philmd, david

This add helpers to trigger warm reset and to retrieve the
reset current reset type.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 hw/core/bus.c          | 11 +++++++++++
 hw/core/qdev.c         | 11 +++++++++++
 include/hw/qdev-core.h | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+)

diff --git a/hw/core/bus.c b/hw/core/bus.c
index 3be65ad041..b245fd6937 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -74,11 +74,22 @@ void bus_cold_reset(BusState *bus)
     resettable_reset(OBJECT(bus), RESET_TYPE_COLD);
 }
 
+void bus_warm_reset(BusState *bus)
+{
+    resettable_reset(OBJECT(bus), RESET_TYPE_WARM);
+}
+
 bool bus_is_resetting(BusState *bus)
 {
     return resettable_is_resetting(OBJECT(bus));
 }
 
+bool bus_test_reset_type(BusState *bus, ResetType type)
+{
+    ResetType cur_type = resettable_get_type(OBJECT(bus));
+    return (cur_type & type);
+}
+
 static ResetState *bus_get_reset_state(Object *obj)
 {
     BusState *bus = BUS(obj);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 1638bc9f3b..9095f2b9c1 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -281,11 +281,22 @@ void device_cold_reset(DeviceState *dev)
     resettable_reset(OBJECT(dev), RESET_TYPE_COLD);
 }
 
+void device_warm_reset(DeviceState *dev)
+{
+    resettable_reset(OBJECT(dev), RESET_TYPE_WARM);
+}
+
 bool device_is_resetting(DeviceState *dev)
 {
     return resettable_is_resetting(OBJECT(dev));
 }
 
+bool device_test_reset_type(DeviceState *dev, ResetType type)
+{
+    ResetType cur_type = resettable_get_type(OBJECT(dev));
+    return (cur_type & type);
+}
+
 static ResetState *device_get_reset_state(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 2976121fbc..eb11f0f801 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -422,18 +422,52 @@ void device_cold_reset(DeviceState *dev);
  */
 void bus_cold_reset(BusState *bus);
 
+/**
+ * device_warm_reset:
+ * Trigger a warm reset of the device @dev.
+ *
+ * Use the Resettable interface (see hw/interface.h); it also reset the
+ * device's qdev/qbus subtree.
+ */
+void device_warm_reset(DeviceState *dev);
+
+/**
+ * bus_warm_reset:
+ * Trigger a warm reset of the bus @bus.
+ *
+ * Use the Resettable interface (see hw/interface.h); it also reset the
+ * bus's qdev/qbus subtree.
+ */
+void bus_warm_reset(BusState *bus);
+
 /**
  * device_is_resetting:
  * Return true if the device @dev is currently being reset.
  */
 bool device_is_resetting(DeviceState *dev);
 
+/**
+ * device_test_reset_type:
+ * Return true if the device @dev is currently under reset type
+ * @type.
+ * Only valid if device_is_resetting() is true
+ */
+bool device_test_reset_type(DeviceState *dev, ResetType type);
+
 /**
  * bus_is_resetting:
  * Return true if the bus @bus is currently being reset.
  */
 bool bus_is_resetting(BusState *bus);
 
+/**
+ * bus_test_reset_type:
+ * Return true if the bus @bus is currently under reset type
+ * @type.
+ * Only valid if device_is_resetting() is true
+ */
+bool bus_test_reset_type(BusState *bus, ResetType type);
+
 /* This should go away once we get rid of the NULL bus hack */
 BusState *sysbus_get_default(void);
 
-- 
2.22.0



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

* [Qemu-devel] [PATCH v4 10/10] docs/devel/reset.txt: add documentation about warm reset
  2019-08-21 16:33 [Qemu-devel] [PATCH v4 00/10] Multi-phase reset mechanism Damien Hedde
                   ` (8 preceding siblings ...)
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 09/10] hw/core/: add warm reset helpers for devices and buses Damien Hedde
@ 2019-08-21 16:33 ` Damien Hedde
  2019-08-21 17:09 ` [Qemu-devel] [PATCH v4 00/10] Multi-phase reset mechanism no-reply
  2019-09-10 10:33 ` Damien Hedde
  11 siblings, 0 replies; 20+ messages in thread
From: Damien Hedde @ 2019-08-21 16:33 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, peter.maydell, edgar.iglesias, berrange, ehabkost,
	mark.burton, pbonzini, philmd, david

Complete the documentation with the handling of several reset types
(cold and warm).

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 docs/devel/reset.txt | 55 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 4 deletions(-)

diff --git a/docs/devel/reset.txt b/docs/devel/reset.txt
index 77ff29b3d7..ed1a72566d 100644
--- a/docs/devel/reset.txt
+++ b/docs/devel/reset.txt
@@ -31,16 +31,21 @@ makes no difference. But there can be some; some configuration may be kept when
 applying a warm reset for example.
 
 The Resettable interface handles reset kinds with an enum. For now only cold
-reset is defined, others may be added later.
+and warm reset are defined, others may be added later.
 ```
 typedef enum ResetType {
     RESET_TYPE_COLD,
+    RESET_TYPE_WARM,
 } ResetType;
 ```
 
 In qemu, RESET_TYPE_COLD means we reset to the initial state corresponding to
 the start of qemu; this might differs from what is a read hardware cold reset.
 
+Although the interface can handle several kind of resets, these are not totally
+independant and disjoint. There are some constraints; these are explained below
+in the "multi-phase" section.
+
 
 Triggering reset
 ----------------
@@ -49,21 +54,41 @@ 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 trigger a reset event on a resettable object with resettable_reset().
-The object will be instantly reset.
+A resettable object can be put into its "in reset" state and held there
+indefinitely.
+
+You must call resettable_assert_reset() to put an object in reset. It will stay
+in this state until you eventually call resettable_deassert_reset(). Care must
+be taken to call resettable_deassert_reset() once and only once per call of
+resettable_assert_reset().
+
+```resettable_assert_reset(Object *obj, ResetType type);```
+The parameter "obj" is an object implementing the Resettable interface.
+The parameter "type" gives the type of reset you want to trigger.
+
+```resettable_deassert_reset(Object *obj);```
+The parameter "obj" is an object implementing the Resettable interface.
+
+If you want to just trigger a reset event but not leave the object in reset for
+any period of time, you can use resettable_reset(), which is a convenience
+function identical in behaviour to calling resettable_assert() and then
+immediately calling resettable_deassert().
 
 ```void resettable_reset(Object *obj, ResetType type);```
 The parameter "obj" is an object implementing the Resettable interface.
 The parameter "type" gives the type of reset you want to trigger.
 
 It is possible to interleave multiple calls to
+ - resettable_assert_reset(),
+ - resettable_deassert_reset(),
  - resettable_reset().
 
 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 by
-maintaining a count of reset.
+maintaining a count of reset; this is why resettable_deassert_reset() must be
+called once and only once per resettable_assert_reset().
 
 Note that 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
@@ -74,6 +99,8 @@ For Devices and Buses, the following helper functions exists:
 ```
 void device_cold_reset(Device *dev);
 void bus_cold_reset(Bus *bus);
+void device_warm_reset(Device *dev);
+void bus_warm_reset(Bus *bus);
 ```
 
 These are simple wrappers around resettable_reset() function; they only cast the
@@ -123,6 +150,25 @@ 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.
 
+An exception to that is when entering a new reset type AND if there was no
+previous cold reset; in that case, init and hold methods are executed again
+because the different reset type may reset more things than the previous one
+has done.
+
+For example if some controller has started a RESET_TYPE_WARM with
+resettable_assert_reset() on a device and another controller does a
+device_cold_reset() on the same object, then the init phase is executed with
+RESET_TYPE_COLD as an argument and then the hold phase.
+If the first reset was a cold reset, then the warm reset would have triggered
+nothing because the cold reset is "stronger".
+
+Note also that the exit phase will never be executed twice; it will only be
+executed when all reset operation are closed, independently of the number of
+reset types that were issued. This is a limitation of the interface, there is
+only a global count of reset (not a count per reset type). The consequence is
+that the different reset types must be close enough in behavior to not require
+different exit phases.
+
 
 Handling reset in a new resettable object
 -----------------------------------------
@@ -203,6 +249,7 @@ interface.
 typedef struct ResetState {
     uint32_t count;
     bool hold_phase_needed;
+    ResetType type;
 } ResetState;
 
 typedef ResetState *(*ResettableGetState)(Object *obj);
-- 
2.22.0



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

* Re: [Qemu-devel] [PATCH v4 00/10] Multi-phase reset mechanism
  2019-08-21 16:33 [Qemu-devel] [PATCH v4 00/10] Multi-phase reset mechanism Damien Hedde
                   ` (9 preceding siblings ...)
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 10/10] docs/devel/reset.txt: add documentation about warm reset Damien Hedde
@ 2019-08-21 17:09 ` no-reply
  2019-09-10 10:33 ` Damien Hedde
  11 siblings, 0 replies; 20+ messages in thread
From: no-reply @ 2019-08-21 17:09 UTC (permalink / raw)
  To: damien.hedde
  Cc: damien.hedde, peter.maydell, edgar.iglesias, berrange, ehabkost,
	mark.burton, qemu-devel, pbonzini, philmd, david

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



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH v4 00/10] Multi-phase reset mechanism
Message-id: 20190821163341.16309-1-damien.hedde@greensocs.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20190821163341.16309-1-damien.hedde@greensocs.com -> patchew/20190821163341.16309-1-damien.hedde@greensocs.com
Submodule 'capstone' (https://git.qemu.org/git/capstone.git) registered for path 'capstone'
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
Submodule 'roms/QemuMacDrivers' (https://git.qemu.org/git/QemuMacDrivers.git) registered for path 'roms/QemuMacDrivers'
Submodule 'roms/SLOF' (https://git.qemu.org/git/SLOF.git) registered for path 'roms/SLOF'
Submodule 'roms/edk2' (https://git.qemu.org/git/edk2.git) registered for path 'roms/edk2'
Submodule 'roms/ipxe' (https://git.qemu.org/git/ipxe.git) registered for path 'roms/ipxe'
Submodule 'roms/openbios' (https://git.qemu.org/git/openbios.git) registered for path 'roms/openbios'
Submodule 'roms/openhackware' (https://git.qemu.org/git/openhackware.git) registered for path 'roms/openhackware'
Submodule 'roms/opensbi' (https://git.qemu.org/git/opensbi.git) registered for path 'roms/opensbi'
Submodule 'roms/qemu-palcode' (https://git.qemu.org/git/qemu-palcode.git) registered for path 'roms/qemu-palcode'
Submodule 'roms/seabios' (https://git.qemu.org/git/seabios.git/) registered for path 'roms/seabios'
Submodule 'roms/seabios-hppa' (https://git.qemu.org/git/seabios-hppa.git) registered for path 'roms/seabios-hppa'
Submodule 'roms/sgabios' (https://git.qemu.org/git/sgabios.git) registered for path 'roms/sgabios'
Submodule 'roms/skiboot' (https://git.qemu.org/git/skiboot.git) registered for path 'roms/skiboot'
Submodule 'roms/u-boot' (https://git.qemu.org/git/u-boot.git) registered for path 'roms/u-boot'
Submodule 'roms/u-boot-sam460ex' (https://git.qemu.org/git/u-boot-sam460ex.git) registered for path 'roms/u-boot-sam460ex'
Submodule 'slirp' (https://git.qemu.org/git/libslirp.git) registered for path 'slirp'
Submodule 'tests/fp/berkeley-softfloat-3' (https://git.qemu.org/git/berkeley-softfloat-3.git) registered for path 'tests/fp/berkeley-softfloat-3'
Submodule 'tests/fp/berkeley-testfloat-3' (https://git.qemu.org/git/berkeley-testfloat-3.git) registered for path 'tests/fp/berkeley-testfloat-3'
Submodule 'ui/keycodemapdb' (https://git.qemu.org/git/keycodemapdb.git) registered for path 'ui/keycodemapdb'
Cloning into 'capstone'...
Submodule path 'capstone': checked out '22ead3e0bfdb87516656453336160e0a37b066bf'
Cloning into 'dtc'...
Submodule path 'dtc': checked out '88f18909db731a627456f26d779445f84e449536'
Cloning into 'roms/QemuMacDrivers'...
Submodule path 'roms/QemuMacDrivers': checked out '90c488d5f4a407342247b9ea869df1c2d9c8e266'
Cloning into 'roms/SLOF'...
Submodule path 'roms/SLOF': checked out '7bfe584e321946771692711ff83ad2b5850daca7'
Cloning into 'roms/edk2'...
Submodule path 'roms/edk2': checked out '20d2e5a125e34fc8501026613a71549b2a1a3e54'
Submodule 'SoftFloat' (https://github.com/ucb-bar/berkeley-softfloat-3.git) registered for path 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'
Submodule 'CryptoPkg/Library/OpensslLib/openssl' (https://github.com/openssl/openssl) registered for path 'CryptoPkg/Library/OpensslLib/openssl'
Cloning into 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'...
Submodule path 'roms/edk2/ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
Cloning into 'CryptoPkg/Library/OpensslLib/openssl'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl': checked out '50eaac9f3337667259de725451f201e784599687'
Submodule 'boringssl' (https://boringssl.googlesource.com/boringssl) registered for path 'boringssl'
Submodule 'krb5' (https://github.com/krb5/krb5) registered for path 'krb5'
Submodule 'pyca.cryptography' (https://github.com/pyca/cryptography.git) registered for path 'pyca-cryptography'
Cloning into 'boringssl'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/boringssl': checked out '2070f8ad9151dc8f3a73bffaa146b5e6937a583f'
Cloning into 'krb5'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/krb5': checked out 'b9ad6c49505c96a088326b62a52568e3484f2168'
Cloning into 'pyca-cryptography'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/pyca-cryptography': checked out '09403100de2f6f1cdd0d484dcb8e620f1c335c8f'
Cloning into 'roms/ipxe'...
Submodule path 'roms/ipxe': checked out 'de4565cbe76ea9f7913a01f331be3ee901bb6e17'
Cloning into 'roms/openbios'...
Submodule path 'roms/openbios': checked out 'c79e0ecb84f4f1ee3f73f521622e264edd1bf174'
Cloning into 'roms/openhackware'...
Submodule path 'roms/openhackware': checked out 'c559da7c8eec5e45ef1f67978827af6f0b9546f5'
Cloning into 'roms/opensbi'...
Submodule path 'roms/opensbi': checked out 'ce228ee0919deb9957192d723eecc8aaae2697c6'
Cloning into 'roms/qemu-palcode'...
Submodule path 'roms/qemu-palcode': checked out 'bf0e13698872450164fa7040da36a95d2d4b326f'
Cloning into 'roms/seabios'...
Submodule path 'roms/seabios': checked out 'a5cab58e9a3fb6e168aba919c5669bea406573b4'
Cloning into 'roms/seabios-hppa'...
Submodule path 'roms/seabios-hppa': checked out '0f4fe84658165e96ce35870fd19fc634e182e77b'
Cloning into 'roms/sgabios'...
Submodule path 'roms/sgabios': checked out 'cbaee52287e5f32373181cff50a00b6c4ac9015a'
Cloning into 'roms/skiboot'...
Submodule path 'roms/skiboot': checked out '261ca8e779e5138869a45f174caa49be6a274501'
Cloning into 'roms/u-boot'...
Submodule path 'roms/u-boot': checked out 'd3689267f92c5956e09cc7d1baa4700141662bff'
Cloning into 'roms/u-boot-sam460ex'...
Submodule path 'roms/u-boot-sam460ex': checked out '60b3916f33e617a815973c5a6df77055b2e3a588'
Cloning into 'slirp'...
Submodule path 'slirp': checked out '126c04acbabd7ad32c2b018fe10dfac2a3bc1210'
Cloning into 'tests/fp/berkeley-softfloat-3'...
Submodule path 'tests/fp/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
Cloning into 'tests/fp/berkeley-testfloat-3'...
Submodule path 'tests/fp/berkeley-testfloat-3': checked out '5a59dcec19327396a011a17fd924aed4fec416b3'
Cloning into 'ui/keycodemapdb'...
Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce'
Switched to a new branch 'test'
89a6fbd docs/devel/reset.txt: add documentation about warm reset
e23e78e hw/core/: add warm reset helpers for devices and buses
02fd837 hw/core/resettable: add support for warm reset
015bdfc hw/core/qdev: replace deprecated device_legacy_reset when hotplugging device
2467320 hw/s390x/ipl.c: replace deprecated qbus_reset_all registration
52a1958 vl.c: replace deprecated qbus_reset_all registration
335ff4c docs/devel/reset.txt: create doc about Resettable interface
9b58ee1 hw/core: add Resettable interface in Bus and Device classes
aa62637 hw/core: create Resettable QOM interface
6336094 add device_legacy_reset function to prepare for reset api change

=== OUTPUT BEGIN ===
1/10 Checking commit 6336094315df (add device_legacy_reset function to prepare for reset api change)
2/10 Checking commit aa62637a8f9b (hw/core: create Resettable QOM interface)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#62: 
new file mode 100644

total: 0 errors, 1 warnings, 395 lines checked

Patch 2/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/10 Checking commit 9b58ee15ffc4 (hw/core: add Resettable interface in Bus and Device classes)
4/10 Checking commit 335ff4c97c54 (docs/devel/reset.txt: create doc about Resettable interface)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#14: 
new file mode 100644

total: 0 errors, 1 warnings, 237 lines checked

Patch 4/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/10 Checking commit 52a19580dc1c (vl.c: replace deprecated qbus_reset_all registration)
6/10 Checking commit 246732015660 (hw/s390x/ipl.c: replace deprecated qbus_reset_all registration)
7/10 Checking commit 015bdfcc4bf7 (hw/core/qdev: replace deprecated device_legacy_reset when hotplugging device)
8/10 Checking commit 02fd837431e5 (hw/core/resettable: add support for warm reset)
9/10 Checking commit e23e78e680c6 (hw/core/: add warm reset helpers for devices and buses)
ERROR: return is not a function, parentheses are not required
#34: FILE: hw/core/bus.c:90:
+    return (cur_type & type);

ERROR: return is not a function, parentheses are not required
#61: FILE: hw/core/qdev.c:297:
+    return (cur_type & type);

total: 2 errors, 0 warnings, 95 lines checked

Patch 9/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

10/10 Checking commit 89a6fbdf06e7 (docs/devel/reset.txt: add documentation about warm reset)
=== OUTPUT END ===

Test command exited with code: 1


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

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

* Re: [Qemu-devel] [PATCH v4 01/10] add device_legacy_reset function to prepare for reset api change
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 01/10] add device_legacy_reset function to prepare for reset api change Damien Hedde
@ 2019-08-24  9:50   ` David Gibson
  0 siblings, 0 replies; 20+ messages in thread
From: David Gibson @ 2019-08-24  9:50 UTC (permalink / raw)
  To: Damien Hedde
  Cc: Fam Zheng, peter.maydell, Collin Walling, Dmitry Fleytman,
	Michael S. Tsirkin, qemu-devel, Gerd Hoffmann, edgar.iglesias,
	qemu-block, David Hildenbrand, Halil Pasic,
	Christian Borntraeger, philmd, ehabkost, qemu-s390x, qemu-arm,
	Cédric Le Goater, John Snow, Richard Henderson, berrange,
	Cornelia Huck, mark.burton, qemu-ppc, pbonzini

[-- Attachment #1: Type: text/plain, Size: 10728 bytes --]

On Wed, Aug 21, 2019 at 06:33:32PM +0200, 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>

ppc parts
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>
> Cc: qemu-block@nongnu.org
> Cc: qemu-ppc@nongnu.org
> Cc: qemu-s390x@nongnu.org
> Cc: qemu-arm@nongnu.org
> ---
>  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 60d66c2f39..a95d4fa87d 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -257,7 +257,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;
>  }
> @@ -865,7 +865,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;
>  
> @@ -1087,7 +1087,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 3ab4bcb3ca..f33a8de42f 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2826,7 +2826,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 aad981cb78..a0f1787ee0 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -1514,7 +1514,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 88ba8e7b9b..bfdc30d4e1 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 bf31fd854c..f4c918be8b 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -2031,7 +2031,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 0803649658..ff7ee4d822 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -305,7 +305,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 00235148be..93cda37c27 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -242,7 +242,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 de70b7a19a..09e6dfd664 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -406,11 +406,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,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

* Re: [Qemu-devel] [PATCH v4 00/10] Multi-phase reset mechanism
  2019-08-21 16:33 [Qemu-devel] [PATCH v4 00/10] Multi-phase reset mechanism Damien Hedde
                   ` (10 preceding siblings ...)
  2019-08-21 17:09 ` [Qemu-devel] [PATCH v4 00/10] Multi-phase reset mechanism no-reply
@ 2019-09-10 10:33 ` Damien Hedde
  11 siblings, 0 replies; 20+ messages in thread
From: Damien Hedde @ 2019-09-10 10:33 UTC (permalink / raw)
  To: qemu-devel
  Cc: edgar.iglesias, peter.maydell, berrange, ehabkost, mark.burton,
	pbonzini, philmd, david

ping !

Any comments ?

Thanks,
Damien

On 8/21/19 6:33 PM, Damien Hedde wrote:
> Hi all,
> 
> Here's the 4th version of the multi-phase reset proposal patches.
> Previous version can be found here:
> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg06365.html
> 
> 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. Please see patch 4 which adds documentation for more details.
> 
> Compared to previous version, I've reduced the scope of the series to
> multi-phase basics. In particular, I've removed migration-related features
> which can be added and discussed later when we've settled the api. I've also
> not included the change to the new api for the ~20 impacted files because it
> highly depends on whether we handle cold vs warm reset difference or not. I'll
> handle them when we'll advance on this point.
> 
> I've isolated in patch 2 to 4, the multi-phase base mechanism handling only
> cold reset as suggested by David.
> Patches 5 and 6 do trivial modifications related to the registration of reset
> handler in the main system reset.
> Patch 7 handles the hotplug device reset case.
> These first 7 patches form a multi-phase basics independent subset; the first 6
> do not modify the behavior. I'd really like to move forward on them as we have
> things depending on the basic multi-phase capability only (in particular the
> clock-tree support).
> 
> Nevertheless I've kept in this series the addition of warm reset (in patches 8
> to 10) so we can continue the discussion. Even if we don't introduce warm
> reset, it allows discussion about multiple reset type support.
> Maybe I should have put these 3 patches in their own (rfc ?) series, since
> there was lot of questioning about the warm reset and talking about handling
> other resets like bus specific ones.
> 
> I've tested that the actual reset order was not modified by these patches by
> tracing calls to individual reset method after and before applying the series.
> 
> 
> Changes v3 -> v4
> general:
>     + various comments and typos
> patch 1: add device_legacy_reset function to prepare for reset api change
>     + squash of 2 patches from v3 (asked by Peter and David)
> patch 2: Create Resettable QOM interface
>     + ResetType enum (only cold for now) (David and Philippe's remark)
>     + ResetState to factorize most of the code (to address David's concern)
>     + all phases order is now children-to-parent (Peter's remark)
>     + assert/deassert removed to isolate migration related features
> patch 3: add Resettable interface in Bus and Device classes
>     + squash of 2 patches (make Device and Bus Resettable & switch to
>       resettable api) since patch 2 has reduced their size.
>     + adaptation to patch 2 changes (warm reset, state and methods)
>     + isolate hotplug reset change into patch 7 (Peter's remark)
>     + fix qdev/qbus_reset_not doing a cold reset (Peter's remark)
>     + call helper device_reset_cold instead of device_reset (David's remark)
> patch 4: docs/devel/reset.txt: create doc about Resettable interface
>     + various improvements
>     + in this patch, doc is reduced to cold multi-phase reset with no "in reset"
>       state (other parts are kept for following commits)
> patch 5: vl.c: replace deprecated qbus_reset_all registration
>     + suggested comment improvement from Peter
> patch 6: hw/s390x/ipl.c: replace deprecated qbus_reset_all registration
>     + suggested comment improvement from Peter
> 
> Thanks for your feedback,
> Damien
> 
> Damien Hedde (10):
>   add device_legacy_reset function to prepare for reset api change
>   hw/core: create Resettable QOM interface
>   hw/core: add Resettable interface in Bus and Device classes
>   docs/devel/reset.txt: create doc about Resettable interface
>   vl.c: replace deprecated qbus_reset_all registration
>   hw/s390x/ipl.c: replace deprecated qbus_reset_all registration
>   hw/core/qdev: replace deprecated device_legacy_reset when hotplugging
>     device
>   hw/core/resettable: add support for warm reset
>   hw/core/: add warm reset helpers for devices and buses
>   docs/devel/reset.txt: add documentation about warm reset
> 
>  Makefile.objs            |   1 +
>  docs/devel/reset.txt     | 284 +++++++++++++++++++++++++++++++++++++++
>  hw/audio/intel-hda.c     |   2 +-
>  hw/core/Makefile.objs    |   1 +
>  hw/core/bus.c            |  64 +++++++++
>  hw/core/qdev.c           |  86 +++++++++---
>  hw/core/resettable.c     | 199 +++++++++++++++++++++++++++
>  hw/core/trace-events     |  36 +++++
>  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   | 100 +++++++++++++-
>  include/hw/resettable.h  | 171 +++++++++++++++++++++++
>  tests/Makefile.include   |   1 +
>  vl.c                     |  10 +-
>  24 files changed, 949 insertions(+), 44 deletions(-)
>  create mode 100644 docs/devel/reset.txt
>  create mode 100644 hw/core/resettable.c
>  create mode 100644 hw/core/trace-events
>  create mode 100644 include/hw/resettable.h
> 


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

* Re: [Qemu-devel] [PATCH v4 02/10] hw/core: create Resettable QOM interface
  2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 02/10] hw/core: create Resettable QOM interface Damien Hedde
@ 2019-09-11  8:06   ` David Gibson
  2019-09-11 14:56     ` Damien Hedde
  0 siblings, 1 reply; 20+ messages in thread
From: David Gibson @ 2019-09-11  8:06 UTC (permalink / raw)
  To: Damien Hedde
  Cc: edgar.iglesias, peter.maydell, berrange, ehabkost, mark.burton,
	qemu-devel, pbonzini, philmd

[-- Attachment #1: Type: text/plain, Size: 19429 bytes --]

On Wed, Aug 21, 2019 at 06:33:33PM +0200, Damien Hedde 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. There is 3 phases to allow a device to be held in reset
> state; the ability to control this state will be added in a following
> commits.
> 
> The qdev/qbus reset in DeviceClass and BusClass will be modified in
> following commits to use this interface.
> No change of behavior is expected because the init phase execution order
> follows the children-then-parent order inside a tree. Since this is the
> actual order of qdev/qbus reset, we will be able to map current reset
> handlers on init phase for example.
> 
> In this patch only cold reset is introduced, which is pretty much the
> actual semantics of the current reset handlers. The interface can be
> extended to support more reset types.
> 
> Documentation will be added in a following commit.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> 
> I kept the non-recursive count approach (a given object counts reset
> initiated on it as well as reset initiated on its parents in reset
> hierarchy). I implemented the other approach, it is possible but is more
> complex (an object has to know its direct parent(s) and we need to scan
> the reset hierarchy to know if we are in reset) so I prefer not
> to introduce it here.
> Moreover I think it has drawbacks if we want to handle complex reset
> use cases with more reset type.
> Anyway, as long as we don't migrate the reset-related state, there is
> no problem to switch between approaches.
> ---

So, I certainly prefer the more general "reset type" approach taken in
this version.  That said, I find it pretty hard to imagine what types
of reset other than cold will exist that have well enough defined
semantics to be meaningfully used from an external subsystem.

>  Makefile.objs           |   1 +
>  hw/core/Makefile.objs   |   1 +
>  hw/core/resettable.c    | 186 ++++++++++++++++++++++++++++++++++++++++
>  hw/core/trace-events    |  36 ++++++++
>  include/hw/resettable.h | 159 ++++++++++++++++++++++++++++++++++
>  5 files changed, 383 insertions(+)
>  create mode 100644 hw/core/resettable.c
>  create mode 100644 hw/core/trace-events
>  create mode 100644 include/hw/resettable.h
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 6a143dcd57..a723a47e14 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 b49f880a0c..69b408ad1c 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..b534c2c7a4
> --- /dev/null
> +++ b/hw/core/resettable.c
> @@ -0,0 +1,186 @@
> +/*
> + * 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"
> +
> +#define RESETTABLE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE_INTERFACE)
> +
> +static void resettable_foreach_child(ResettableClass *rc,
> +                                     Object *obj,
> +                                     void (*func)(Object *, ResetType type),
> +                                     ResetType type)
> +{
> +    if (rc->foreach_child) {
> +        rc->foreach_child(obj, func, type);
> +    }
> +}
> +
> +static void resettable_init_reset(Object *obj, ResetType type)

I wonder if "enter reset" would be better terminology so this doesn't
get confused with the initial, well, initialization of the device.

> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResetState *s = rc->get_state(obj);
> +    bool action_needed = false;
> +
> +    /* 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;
> +    }
> +    s->count += 1;
> +    /*
> +     * 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);
> +    trace_resettable_phase_init(obj, object_get_typename(obj), type,
> +                                s->count, action_needed);
> +
> +    /*
> +     * handle the children even if action_needed is at false so that
> +     * children counts are incremented too
> +     */
> +    resettable_foreach_child(rc, obj, resettable_init_reset, type);
> +
> +    /* exec init phase */
> +    if (action_needed) {
> +        s->hold_phase_needed = true;
> +        if (rc->phases.init) {
> +            rc->phases.init(obj, type);
> +        }
> +    }
> +    trace_resettable_phase_init_end(obj);
> +}
> +
> +static void resettable_hold_reset_child(Object *obj, ResetType type)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResetState *s = rc->get_state(obj);
> +
> +    trace_resettable_phase_hold(obj, object_get_typename(obj));
> +
> +    /* handle children first */
> +    resettable_foreach_child(rc, obj, resettable_hold_reset_child, 0);
> +
> +    /* exec hold phase */
> +    if (s->hold_phase_needed) {
> +        s->hold_phase_needed = false;
> +        if (rc->phases.hold) {
> +            rc->phases.hold(obj);

I was about to ask what purpose the hold phase has since it's always
executed right after the init phase, before realizing that it's
because it can happen after parent devices have completed their init
phase.

Point that out in a comment here might help to avoid confusion.

> +        }
> +    }
> +    trace_resettable_phase_hold_end(obj, s->hold_phase_needed);
> +}
> +
> +static void resettable_hold_reset(Object *obj)
> +{
> +    /* we don't care of 2nd argument here */
> +    resettable_hold_reset_child(obj, 0);
> +}
> +
> +static void resettable_exit_reset_child(Object *obj, ResetType type)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResetState *s = rc->get_state(obj);
> +
> +    trace_resettable_phase_exit(obj, object_get_typename(obj));
> +
> +    resettable_foreach_child(rc, obj, resettable_exit_reset_child, 0);
> +
> +    /*
> +     * we could assert that count > 0 but there are some corner cases
> +     * where we prefer to let it go as it is probably harmless.
> +     * For example: if there is reset support addition between
> +     * hosts when doing a migration. We may do such things as
> +     * deassert a non-existing reset.
> +     */
> +    if (s->count > 0) {
> +        s->count -= 1;
> +    } else {
> +        trace_resettable_count_underflow(obj);

Should this be an assert(), IIUC this could only come about from an
error within the qemu code, right?

> +    }
> +    if (s->count == 0) {
> +        if (rc->phases.exit) {
> +            rc->phases.exit(obj);

Hm.  It's not really clear to me whether child resets should go before
or after the parent.  It seems odd that it would be the same for both
entering and exiting reset, though.

> +        }
> +    }
> +    trace_resettable_phase_exit_end(obj, s->count);
> +}
> +
> +static void resettable_exit_reset(Object *obj)
> +{
> +    /* we don't care of 2nd argument here */
> +    resettable_exit_reset_child(obj, 0);
> +}
> +
> +void resettable_reset(Object *obj, ResetType type)
> +{
> +    /* TODO: change that when adding support for other reset types */
> +    assert(type == RESET_TYPE_COLD);
> +    trace_resettable_reset(obj, type);
> +    resettable_init_reset(obj, type);
> +    resettable_hold_reset(obj);
> +    resettable_exit_reset(obj);
> +}
> +
> +void resettable_cold_reset_fn(void *opaque)
> +{
> +    resettable_reset((Object *) opaque, RESET_TYPE_COLD);
> +}
> +
> +bool resettable_is_resetting(Object *obj)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResetState *s = rc->get_state(obj);
> +
> +    return (s->count > 0);
> +}
> +
> +void resettable_class_set_parent_phases(ResettableClass *rc,
> +                                        ResettableInitPhase init,
> +                                        ResettableHoldPhase hold,
> +                                        ResettableExitPhase exit,
> +                                        ResettablePhases *parent_phases)
> +{
> +    *parent_phases = rc->phases;
> +    if (init) {
> +        rc->phases.init = init;
> +    }
> +    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
> new file mode 100644
> index 0000000000..ecf966c314
> --- /dev/null
> +++ b/hw/core/trace-events
> @@ -0,0 +1,36 @@
> +# See docs/devel/tracing.txt for syntax documentation.
> +#
> +# This file is processed by the tracetool script during the build.
> +#
> +# To add a new trace event:
> +#
> +# 1. Choose a name for the trace event.  Declare its arguments and format
> +#    string.
> +#
> +# 2. Call the trace event from code using trace_##name, e.g. multiwrite_cb() ->
> +#    trace_multiwrite_cb().  The source file must #include "trace.h".
> +#
> +# Format of a trace event:
> +#
> +# [disable] <name>(<type1> <arg1>[, <type2> <arg2>] ...) "<format-string>"
> +#
> +# Example: g_malloc(size_t size) "size %zu"
> +#
> +# The "disable" keyword will build without the trace event.
> +#
> +# The <name> must be a valid as a C function name.
> +#
> +# Types should be standard C types.  Use void * for pointers because the trace
> +# system may not have the necessary headers included.
> +#
> +# The <format-string> should be a sprintf()-compatible format string.
> +
> +# resettable.c
> +resettable_reset(void *obj, int cold) "obj=%p cold=%d"
> +resettable_phase_init(void *obj, const char *type, int cold, uint32_t count, int needed) "obj=%p(%s) cold=%d count=%" PRIu32 " needed=%d"
> +resettable_phase_init_end(void *obj) "obj=%p"
> +resettable_phase_hold(void *obj, const char *type) "obj=%p(%s)"
> +resettable_phase_hold_end(void *obj, int needed) "obj=%p needed=%d"
> +resettable_phase_exit(void *obj, const char *type) "obj=%p(%s)"
> +resettable_phase_exit_end(void *obj, uint32_t count) "obj=%p count=%" PRIu32
> +resettable_count_underflow(void *obj) "obj=%p"
> diff --git a/include/hw/resettable.h b/include/hw/resettable.h
> new file mode 100644
> index 0000000000..5808c3c187
> --- /dev/null
> +++ b/include/hw/resettable.h
> @@ -0,0 +1,159 @@
> +#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)
> +
> +typedef struct ResetState ResetState;
> +
> +/**
> + * 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,
> + * ResetState 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 ResetState 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.init, 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 'init', 'hold', 'exit'.
> + * An object will always move quickly from 'init' 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
> + * 'init' method has been called.
> + *
> + * Users of a resettable object should not call these methods
> + * directly, but instead use the function resettable_reset().
> + *
> + * @phases.init: 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.init 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 ResetState.
> + *
> + * @foreach_child: Executes a given function 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
> + * an additional ResetType argument which is passed to foreach_child.
> + */
> +typedef void (*ResettableInitPhase)(Object *obj, ResetType type);
> +typedef void (*ResettableHoldPhase)(Object *obj);
> +typedef void (*ResettableExitPhase)(Object *obj);
> +typedef ResetState * (*ResettableGetState)(Object *obj);
> +typedef void (*ResettableForeachChild)(Object *obj,
> +                                       void (*func)(Object *, ResetType type),
> +                                       ResetType type);
> +typedef struct ResettableClass {
> +    InterfaceClass parent_class;
> +
> +    struct ResettablePhases {
> +        ResettableInitPhase init;
> +        ResettableHoldPhase hold;
> +        ResettableExitPhase exit;
> +    } phases;
> +
> +    ResettableGetState get_state;
> +    ResettableForeachChild foreach_child;
> +} ResettableClass;
> +typedef struct ResettablePhases ResettablePhases;
> +
> +/**
> + * ResetState:
> + * 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_needed: flag which indicates that we need to invoke the 'hold'
> + * phase handler for this object.
> + */
> +struct ResetState {
> +    uint32_t count;
> +    bool hold_phase_needed;
> +};
> +
> +/**
> + * resettable_is_resetting:
> + * Return true if @obj is under reset.
> + *
> + * @obj must implement Resettable interface.
> + */
> +bool resettable_is_resetting(Object *obj);
> +
> +/**
> + * 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_deassert_reset.
> + */
> +void resettable_reset(Object *obj, ResetType type);
> +
> +/**
> + * 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:
> + *
> + * Save @rc current reset phases into @parent_phases and override @rc phases
> + * by the given new methods (@init, @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,
> +                                        ResettableInitPhase init,
> +                                        ResettableHoldPhase hold,
> +                                        ResettableExitPhase exit,
> +                                        ResettablePhases *parent_phases);
> +
> +#endif

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

* Re: [Qemu-devel] [PATCH v4 02/10] hw/core: create Resettable QOM interface
  2019-09-11  8:06   ` David Gibson
@ 2019-09-11 14:56     ` Damien Hedde
  2019-09-18  9:11       ` David Gibson
  0 siblings, 1 reply; 20+ messages in thread
From: Damien Hedde @ 2019-09-11 14:56 UTC (permalink / raw)
  To: David Gibson
  Cc: edgar.iglesias, peter.maydell, berrange, ehabkost, mark.burton,
	qemu-devel, pbonzini, philmd



On 9/11/19 10:06 AM, David Gibson wrote:
> On Wed, Aug 21, 2019 at 06:33:33PM +0200, Damien Hedde 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. There is 3 phases to allow a device to be held in reset
>> state; the ability to control this state will be added in a following
>> commits.
>>
>> The qdev/qbus reset in DeviceClass and BusClass will be modified in
>> following commits to use this interface.
>> No change of behavior is expected because the init phase execution order
>> follows the children-then-parent order inside a tree. Since this is the
>> actual order of qdev/qbus reset, we will be able to map current reset
>> handlers on init phase for example.
>>
>> In this patch only cold reset is introduced, which is pretty much the
>> actual semantics of the current reset handlers. The interface can be
>> extended to support more reset types.
>>
>> Documentation will be added in a following commit.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>>
>> I kept the non-recursive count approach (a given object counts reset
>> initiated on it as well as reset initiated on its parents in reset
>> hierarchy). I implemented the other approach, it is possible but is more
>> complex (an object has to know its direct parent(s) and we need to scan
>> the reset hierarchy to know if we are in reset) so I prefer not
>> to introduce it here.
>> Moreover I think it has drawbacks if we want to handle complex reset
>> use cases with more reset type.
>> Anyway, as long as we don't migrate the reset-related state, there is
>> no problem to switch between approaches.
>> ---
> 
> So, I certainly prefer the more general "reset type" approach taken in
> this version.  That said, I find it pretty hard to imagine what types
> of reset other than cold will exist that have well enough defined
> semantics to be meaningfully used from an external subsystem.

Maybe I should completely remove the type then ?

> 
>>  Makefile.objs           |   1 +
>>  hw/core/Makefile.objs   |   1 +
>>  hw/core/resettable.c    | 186 ++++++++++++++++++++++++++++++++++++++++
>>  hw/core/trace-events    |  36 ++++++++
>>  include/hw/resettable.h | 159 ++++++++++++++++++++++++++++++++++
>>  5 files changed, 383 insertions(+)
>>  create mode 100644 hw/core/resettable.c
>>  create mode 100644 hw/core/trace-events
>>  create mode 100644 include/hw/resettable.h
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 6a143dcd57..a723a47e14 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 b49f880a0c..69b408ad1c 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..b534c2c7a4
>> --- /dev/null
>> +++ b/hw/core/resettable.c
>> @@ -0,0 +1,186 @@
>> +/*
>> + * 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"
>> +
>> +#define RESETTABLE_GET_CLASS(obj) \
>> +    OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE_INTERFACE)
>> +
>> +static void resettable_foreach_child(ResettableClass *rc,
>> +                                     Object *obj,
>> +                                     void (*func)(Object *, ResetType type),
>> +                                     ResetType type)
>> +{
>> +    if (rc->foreach_child) {
>> +        rc->foreach_child(obj, func, type);
>> +    }
>> +}
>> +
>> +static void resettable_init_reset(Object *obj, ResetType type)
> 
> I wonder if "enter reset" would be better terminology so this doesn't
> get confused with the initial, well, initialization of the device.

Do you mean for the function here or in general for the name of the phase ?

> 
>> +{
>> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
>> +    ResetState *s = rc->get_state(obj);
>> +    bool action_needed = false;
>> +
>> +    /* 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;
>> +    }
>> +    s->count += 1;
>> +    /*
>> +     * 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);
>> +    trace_resettable_phase_init(obj, object_get_typename(obj), type,
>> +                                s->count, action_needed);
>> +
>> +    /*
>> +     * handle the children even if action_needed is at false so that
>> +     * children counts are incremented too
>> +     */
>> +    resettable_foreach_child(rc, obj, resettable_init_reset, type);
>> +
>> +    /* exec init phase */
>> +    if (action_needed) {
>> +        s->hold_phase_needed = true;
>> +        if (rc->phases.init) {
>> +            rc->phases.init(obj, type);
>> +        }
>> +    }
>> +    trace_resettable_phase_init_end(obj);
>> +}
>> +
>> +static void resettable_hold_reset_child(Object *obj, ResetType type)
>> +{
>> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
>> +    ResetState *s = rc->get_state(obj);
>> +
>> +    trace_resettable_phase_hold(obj, object_get_typename(obj));
>> +
>> +    /* handle children first */
>> +    resettable_foreach_child(rc, obj, resettable_hold_reset_child, 0);
>> +
>> +    /* exec hold phase */
>> +    if (s->hold_phase_needed) {
>> +        s->hold_phase_needed = false;
>> +        if (rc->phases.hold) {
>> +            rc->phases.hold(obj);
> 
> I was about to ask what purpose the hold phase has since it's always
> executed right after the init phase, before realizing that it's
> because it can happen after parent devices have completed their init
> phase.
> 
> Point that out in a comment here might help to avoid confusion.

noted.

> 
>> +        }
>> +    }
>> +    trace_resettable_phase_hold_end(obj, s->hold_phase_needed);
>> +}
>> +
>> +static void resettable_hold_reset(Object *obj)
>> +{
>> +    /* we don't care of 2nd argument here */
>> +    resettable_hold_reset_child(obj, 0);
>> +}
>> +
>> +static void resettable_exit_reset_child(Object *obj, ResetType type)
>> +{
>> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
>> +    ResetState *s = rc->get_state(obj);
>> +
>> +    trace_resettable_phase_exit(obj, object_get_typename(obj));
>> +
>> +    resettable_foreach_child(rc, obj, resettable_exit_reset_child, 0);
>> +
>> +    /*
>> +     * we could assert that count > 0 but there are some corner cases
>> +     * where we prefer to let it go as it is probably harmless.
>> +     * For example: if there is reset support addition between
>> +     * hosts when doing a migration. We may do such things as
>> +     * deassert a non-existing reset.
>> +     */
>> +    if (s->count > 0) {
>> +        s->count -= 1;
>> +    } else {
>> +        trace_resettable_count_underflow(obj);
> 
> Should this be an assert(), IIUC this could only come about from an
> error within the qemu code, right?

Initially I was thinking that so I put an assert.

But while testing I found out that it is triggered by the raspi machine
during its reset because the qbus tree is modified during it.

So it depends if we consider this kind of action to be faulty. With no
migration support, the only way to trigger it is to modify the qdev
hierarchy during reset.

But it made me think about the migration cases (with "staying in reset
state"). There are some cases where migration between 2 different
versions can lead to problem with the counter (typically if you migrate
to a new version that have a new device on a bus held in reset).

> 
>> +    }
>> +    if (s->count == 0) {
>> +        if (rc->phases.exit) {
>> +            rc->phases.exit(obj);
> 
> Hm.  It's not really clear to me whether child resets should go before
> or after the parent.  It seems odd that it would be the same for both
> entering and exiting reset, though.

Since the whole point of the phases is to make the reset independent of
the order in which it is "scheduled", I think we could consider it as
"unspecified". If we find out that we need some hierarchical order for a
yet unknown reason, we can change it.

I've used the order children then parent because it is the actual qdev
reset order and we do not want to modify the actual order now.

> 
>> +        }
>> +    }
>> +    trace_resettable_phase_exit_end(obj, s->count);
>> +}
>> +
>> +static void resettable_exit_reset(Object *obj)
>> +{
>> +    /* we don't care of 2nd argument here */
>> +    resettable_exit_reset_child(obj, 0);
>> +}
>> +
>> +void resettable_reset(Object *obj, ResetType type)
>> +{
>> +    /* TODO: change that when adding support for other reset types */
>> +    assert(type == RESET_TYPE_COLD);
>> +    trace_resettable_reset(obj, type);
>> +    resettable_init_reset(obj, type);
>> +    resettable_hold_reset(obj);
>> +    resettable_exit_reset(obj);
>> +}
>> +
>> +void resettable_cold_reset_fn(void *opaque)
>> +{
>> +    resettable_reset((Object *) opaque, RESET_TYPE_COLD);
>> +}
>> +
>> +bool resettable_is_resetting(Object *obj)
>> +{
>> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
>> +    ResetState *s = rc->get_state(obj);
>> +
>> +    return (s->count > 0);
>> +}
>> +
>> +void resettable_class_set_parent_phases(ResettableClass *rc,
>> +                                        ResettableInitPhase init,
>> +                                        ResettableHoldPhase hold,
>> +                                        ResettableExitPhase exit,
>> +                                        ResettablePhases *parent_phases)
>> +{
>> +    *parent_phases = rc->phases;
>> +    if (init) {
>> +        rc->phases.init = init;
>> +    }
>> +    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
>> new file mode 100644
>> index 0000000000..ecf966c314
>> --- /dev/null
>> +++ b/hw/core/trace-events
>> @@ -0,0 +1,36 @@
>> +# See docs/devel/tracing.txt for syntax documentation.
>> +#
>> +# This file is processed by the tracetool script during the build.
>> +#
>> +# To add a new trace event:
>> +#
>> +# 1. Choose a name for the trace event.  Declare its arguments and format
>> +#    string.
>> +#
>> +# 2. Call the trace event from code using trace_##name, e.g. multiwrite_cb() ->
>> +#    trace_multiwrite_cb().  The source file must #include "trace.h".
>> +#
>> +# Format of a trace event:
>> +#
>> +# [disable] <name>(<type1> <arg1>[, <type2> <arg2>] ...) "<format-string>"
>> +#
>> +# Example: g_malloc(size_t size) "size %zu"
>> +#
>> +# The "disable" keyword will build without the trace event.
>> +#
>> +# The <name> must be a valid as a C function name.
>> +#
>> +# Types should be standard C types.  Use void * for pointers because the trace
>> +# system may not have the necessary headers included.
>> +#
>> +# The <format-string> should be a sprintf()-compatible format string.
>> +
>> +# resettable.c
>> +resettable_reset(void *obj, int cold) "obj=%p cold=%d"
>> +resettable_phase_init(void *obj, const char *type, int cold, uint32_t count, int needed) "obj=%p(%s) cold=%d count=%" PRIu32 " needed=%d"
>> +resettable_phase_init_end(void *obj) "obj=%p"
>> +resettable_phase_hold(void *obj, const char *type) "obj=%p(%s)"
>> +resettable_phase_hold_end(void *obj, int needed) "obj=%p needed=%d"
>> +resettable_phase_exit(void *obj, const char *type) "obj=%p(%s)"
>> +resettable_phase_exit_end(void *obj, uint32_t count) "obj=%p count=%" PRIu32
>> +resettable_count_underflow(void *obj) "obj=%p"
>> diff --git a/include/hw/resettable.h b/include/hw/resettable.h
>> new file mode 100644
>> index 0000000000..5808c3c187
>> --- /dev/null
>> +++ b/include/hw/resettable.h
>> @@ -0,0 +1,159 @@
>> +#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)
>> +
>> +typedef struct ResetState ResetState;
>> +
>> +/**
>> + * 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,
>> + * ResetState 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 ResetState 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.init, 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 'init', 'hold', 'exit'.
>> + * An object will always move quickly from 'init' 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
>> + * 'init' method has been called.
>> + *
>> + * Users of a resettable object should not call these methods
>> + * directly, but instead use the function resettable_reset().
>> + *
>> + * @phases.init: 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.init 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 ResetState.
>> + *
>> + * @foreach_child: Executes a given function 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
>> + * an additional ResetType argument which is passed to foreach_child.
>> + */
>> +typedef void (*ResettableInitPhase)(Object *obj, ResetType type);
>> +typedef void (*ResettableHoldPhase)(Object *obj);
>> +typedef void (*ResettableExitPhase)(Object *obj);
>> +typedef ResetState * (*ResettableGetState)(Object *obj);
>> +typedef void (*ResettableForeachChild)(Object *obj,
>> +                                       void (*func)(Object *, ResetType type),
>> +                                       ResetType type);
>> +typedef struct ResettableClass {
>> +    InterfaceClass parent_class;
>> +
>> +    struct ResettablePhases {
>> +        ResettableInitPhase init;
>> +        ResettableHoldPhase hold;
>> +        ResettableExitPhase exit;
>> +    } phases;
>> +
>> +    ResettableGetState get_state;
>> +    ResettableForeachChild foreach_child;
>> +} ResettableClass;
>> +typedef struct ResettablePhases ResettablePhases;
>> +
>> +/**
>> + * ResetState:
>> + * 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_needed: flag which indicates that we need to invoke the 'hold'
>> + * phase handler for this object.
>> + */
>> +struct ResetState {
>> +    uint32_t count;
>> +    bool hold_phase_needed;
>> +};
>> +
>> +/**
>> + * resettable_is_resetting:
>> + * Return true if @obj is under reset.
>> + *
>> + * @obj must implement Resettable interface.
>> + */
>> +bool resettable_is_resetting(Object *obj);
>> +
>> +/**
>> + * 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_deassert_reset.
>> + */
>> +void resettable_reset(Object *obj, ResetType type);
>> +
>> +/**
>> + * 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:
>> + *
>> + * Save @rc current reset phases into @parent_phases and override @rc phases
>> + * by the given new methods (@init, @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,
>> +                                        ResettableInitPhase init,
>> +                                        ResettableHoldPhase hold,
>> +                                        ResettableExitPhase exit,
>> +                                        ResettablePhases *parent_phases);
>> +
>> +#endif
> 


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

* Re: [Qemu-devel] [PATCH v4 02/10] hw/core: create Resettable QOM interface
  2019-09-11 14:56     ` Damien Hedde
@ 2019-09-18  9:11       ` David Gibson
  2019-09-24 11:21         ` Damien Hedde
  0 siblings, 1 reply; 20+ messages in thread
From: David Gibson @ 2019-09-18  9:11 UTC (permalink / raw)
  To: Damien Hedde
  Cc: edgar.iglesias, peter.maydell, berrange, ehabkost, mark.burton,
	qemu-devel, pbonzini, philmd

[-- Attachment #1: Type: text/plain, Size: 22819 bytes --]

On Wed, Sep 11, 2019 at 04:56:13PM +0200, Damien Hedde wrote:
> 
> 
> On 9/11/19 10:06 AM, David Gibson wrote:
> > On Wed, Aug 21, 2019 at 06:33:33PM +0200, Damien Hedde 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. There is 3 phases to allow a device to be held in reset
> >> state; the ability to control this state will be added in a following
> >> commits.
> >>
> >> The qdev/qbus reset in DeviceClass and BusClass will be modified in
> >> following commits to use this interface.
> >> No change of behavior is expected because the init phase execution order
> >> follows the children-then-parent order inside a tree. Since this is the
> >> actual order of qdev/qbus reset, we will be able to map current reset
> >> handlers on init phase for example.
> >>
> >> In this patch only cold reset is introduced, which is pretty much the
> >> actual semantics of the current reset handlers. The interface can be
> >> extended to support more reset types.
> >>
> >> Documentation will be added in a following commit.
> >>
> >> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> >> ---
> >>
> >> I kept the non-recursive count approach (a given object counts reset
> >> initiated on it as well as reset initiated on its parents in reset
> >> hierarchy). I implemented the other approach, it is possible but is more
> >> complex (an object has to know its direct parent(s) and we need to scan
> >> the reset hierarchy to know if we are in reset) so I prefer not
> >> to introduce it here.
> >> Moreover I think it has drawbacks if we want to handle complex reset
> >> use cases with more reset type.
> >> Anyway, as long as we don't migrate the reset-related state, there is
> >> no problem to switch between approaches.
> >> ---
> > 
> > So, I certainly prefer the more general "reset type" approach taken in
> > this version.  That said, I find it pretty hard to imagine what types
> > of reset other than cold will exist that have well enough defined
> > semantics to be meaningfully used from an external subsystem.
> 
> Maybe I should completely remove the type then ?

That makes sense to me.  I don't know if other possible users of the
mechanism have different opinions though.

> > 
> >>  Makefile.objs           |   1 +
> >>  hw/core/Makefile.objs   |   1 +
> >>  hw/core/resettable.c    | 186 ++++++++++++++++++++++++++++++++++++++++
> >>  hw/core/trace-events    |  36 ++++++++
> >>  include/hw/resettable.h | 159 ++++++++++++++++++++++++++++++++++
> >>  5 files changed, 383 insertions(+)
> >>  create mode 100644 hw/core/resettable.c
> >>  create mode 100644 hw/core/trace-events
> >>  create mode 100644 include/hw/resettable.h
> >>
> >> diff --git a/Makefile.objs b/Makefile.objs
> >> index 6a143dcd57..a723a47e14 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 b49f880a0c..69b408ad1c 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..b534c2c7a4
> >> --- /dev/null
> >> +++ b/hw/core/resettable.c
> >> @@ -0,0 +1,186 @@
> >> +/*
> >> + * 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"
> >> +
> >> +#define RESETTABLE_GET_CLASS(obj) \
> >> +    OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE_INTERFACE)
> >> +
> >> +static void resettable_foreach_child(ResettableClass *rc,
> >> +                                     Object *obj,
> >> +                                     void (*func)(Object *, ResetType type),
> >> +                                     ResetType type)
> >> +{
> >> +    if (rc->foreach_child) {
> >> +        rc->foreach_child(obj, func, type);
> >> +    }
> >> +}
> >> +
> >> +static void resettable_init_reset(Object *obj, ResetType type)
> > 
> > I wonder if "enter reset" would be better terminology so this doesn't
> > get confused with the initial, well, initialization of the device.
> 
> Do you mean for the function here or in general for the name of the phase ?

In general.

> >> +{
> >> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> >> +    ResetState *s = rc->get_state(obj);
> >> +    bool action_needed = false;
> >> +
> >> +    /* 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;
> >> +    }
> >> +    s->count += 1;
> >> +    /*
> >> +     * 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);
> >> +    trace_resettable_phase_init(obj, object_get_typename(obj), type,
> >> +                                s->count, action_needed);
> >> +
> >> +    /*
> >> +     * handle the children even if action_needed is at false so that
> >> +     * children counts are incremented too
> >> +     */
> >> +    resettable_foreach_child(rc, obj, resettable_init_reset, type);
> >> +
> >> +    /* exec init phase */
> >> +    if (action_needed) {
> >> +        s->hold_phase_needed = true;
> >> +        if (rc->phases.init) {
> >> +            rc->phases.init(obj, type);
> >> +        }
> >> +    }
> >> +    trace_resettable_phase_init_end(obj);
> >> +}
> >> +
> >> +static void resettable_hold_reset_child(Object *obj, ResetType type)
> >> +{
> >> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> >> +    ResetState *s = rc->get_state(obj);
> >> +
> >> +    trace_resettable_phase_hold(obj, object_get_typename(obj));
> >> +
> >> +    /* handle children first */
> >> +    resettable_foreach_child(rc, obj, resettable_hold_reset_child, 0);
> >> +
> >> +    /* exec hold phase */
> >> +    if (s->hold_phase_needed) {
> >> +        s->hold_phase_needed = false;
> >> +        if (rc->phases.hold) {
> >> +            rc->phases.hold(obj);
> > 
> > I was about to ask what purpose the hold phase has since it's always
> > executed right after the init phase, before realizing that it's
> > because it can happen after parent devices have completed their init
> > phase.
> > 
> > Point that out in a comment here might help to avoid confusion.
> 
> noted.
> 
> > 
> >> +        }
> >> +    }
> >> +    trace_resettable_phase_hold_end(obj, s->hold_phase_needed);
> >> +}
> >> +
> >> +static void resettable_hold_reset(Object *obj)
> >> +{
> >> +    /* we don't care of 2nd argument here */
> >> +    resettable_hold_reset_child(obj, 0);
> >> +}
> >> +
> >> +static void resettable_exit_reset_child(Object *obj, ResetType type)
> >> +{
> >> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> >> +    ResetState *s = rc->get_state(obj);
> >> +
> >> +    trace_resettable_phase_exit(obj, object_get_typename(obj));
> >> +
> >> +    resettable_foreach_child(rc, obj, resettable_exit_reset_child, 0);
> >> +
> >> +    /*
> >> +     * we could assert that count > 0 but there are some corner cases
> >> +     * where we prefer to let it go as it is probably harmless.
> >> +     * For example: if there is reset support addition between
> >> +     * hosts when doing a migration. We may do such things as
> >> +     * deassert a non-existing reset.
> >> +     */
> >> +    if (s->count > 0) {
> >> +        s->count -= 1;
> >> +    } else {
> >> +        trace_resettable_count_underflow(obj);
> > 
> > Should this be an assert(), IIUC this could only come about from an
> > error within the qemu code, right?
> 
> Initially I was thinking that so I put an assert.
> 
> But while testing I found out that it is triggered by the raspi machine
> during its reset because the qbus tree is modified during it.
> 
> So it depends if we consider this kind of action to be faulty. With no
> migration support, the only way to trigger it is to modify the qdev
> hierarchy during reset.

Hm, I see.  It feels like just ignoring underflow is ignoring the
error rather than really addressing it.  When we add a device to the
heirarchy, do we need to initialize its reset count based on its
parent's current count or something.

> But it made me think about the migration cases (with "staying in reset
> state"). There are some cases where migration between 2 different
> versions can lead to problem with the counter (typically if you migrate
> to a new version that have a new device on a bus held in reset).

Uh... migration is only permitted between identical machines.  I don't
see how a new device is possible.

> 
> > 
> >> +    }
> >> +    if (s->count == 0) {
> >> +        if (rc->phases.exit) {
> >> +            rc->phases.exit(obj);
> > 
> > Hm.  It's not really clear to me whether child resets should go before
> > or after the parent.  It seems odd that it would be the same for both
> > entering and exiting reset, though.
> 
> Since the whole point of the phases is to make the reset independent of
> the order in which it is "scheduled", I think we could consider it as
> "unspecified". If we find out that we need some hierarchical order for a
> yet unknown reason, we can change it.
> 
> I've used the order children then parent because it is the actual qdev
> reset order and we do not want to modify the actual order now.

Hm, ok.

> 
> > 
> >> +        }
> >> +    }
> >> +    trace_resettable_phase_exit_end(obj, s->count);
> >> +}
> >> +
> >> +static void resettable_exit_reset(Object *obj)
> >> +{
> >> +    /* we don't care of 2nd argument here */
> >> +    resettable_exit_reset_child(obj, 0);
> >> +}
> >> +
> >> +void resettable_reset(Object *obj, ResetType type)
> >> +{
> >> +    /* TODO: change that when adding support for other reset types */
> >> +    assert(type == RESET_TYPE_COLD);
> >> +    trace_resettable_reset(obj, type);
> >> +    resettable_init_reset(obj, type);
> >> +    resettable_hold_reset(obj);
> >> +    resettable_exit_reset(obj);
> >> +}
> >> +
> >> +void resettable_cold_reset_fn(void *opaque)
> >> +{
> >> +    resettable_reset((Object *) opaque, RESET_TYPE_COLD);
> >> +}
> >> +
> >> +bool resettable_is_resetting(Object *obj)
> >> +{
> >> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> >> +    ResetState *s = rc->get_state(obj);
> >> +
> >> +    return (s->count > 0);
> >> +}
> >> +
> >> +void resettable_class_set_parent_phases(ResettableClass *rc,
> >> +                                        ResettableInitPhase init,
> >> +                                        ResettableHoldPhase hold,
> >> +                                        ResettableExitPhase exit,
> >> +                                        ResettablePhases *parent_phases)
> >> +{
> >> +    *parent_phases = rc->phases;
> >> +    if (init) {
> >> +        rc->phases.init = init;
> >> +    }
> >> +    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
> >> new file mode 100644
> >> index 0000000000..ecf966c314
> >> --- /dev/null
> >> +++ b/hw/core/trace-events
> >> @@ -0,0 +1,36 @@
> >> +# See docs/devel/tracing.txt for syntax documentation.
> >> +#
> >> +# This file is processed by the tracetool script during the build.
> >> +#
> >> +# To add a new trace event:
> >> +#
> >> +# 1. Choose a name for the trace event.  Declare its arguments and format
> >> +#    string.
> >> +#
> >> +# 2. Call the trace event from code using trace_##name, e.g. multiwrite_cb() ->
> >> +#    trace_multiwrite_cb().  The source file must #include "trace.h".
> >> +#
> >> +# Format of a trace event:
> >> +#
> >> +# [disable] <name>(<type1> <arg1>[, <type2> <arg2>] ...) "<format-string>"
> >> +#
> >> +# Example: g_malloc(size_t size) "size %zu"
> >> +#
> >> +# The "disable" keyword will build without the trace event.
> >> +#
> >> +# The <name> must be a valid as a C function name.
> >> +#
> >> +# Types should be standard C types.  Use void * for pointers because the trace
> >> +# system may not have the necessary headers included.
> >> +#
> >> +# The <format-string> should be a sprintf()-compatible format string.
> >> +
> >> +# resettable.c
> >> +resettable_reset(void *obj, int cold) "obj=%p cold=%d"
> >> +resettable_phase_init(void *obj, const char *type, int cold, uint32_t count, int needed) "obj=%p(%s) cold=%d count=%" PRIu32 " needed=%d"
> >> +resettable_phase_init_end(void *obj) "obj=%p"
> >> +resettable_phase_hold(void *obj, const char *type) "obj=%p(%s)"
> >> +resettable_phase_hold_end(void *obj, int needed) "obj=%p needed=%d"
> >> +resettable_phase_exit(void *obj, const char *type) "obj=%p(%s)"
> >> +resettable_phase_exit_end(void *obj, uint32_t count) "obj=%p count=%" PRIu32
> >> +resettable_count_underflow(void *obj) "obj=%p"
> >> diff --git a/include/hw/resettable.h b/include/hw/resettable.h
> >> new file mode 100644
> >> index 0000000000..5808c3c187
> >> --- /dev/null
> >> +++ b/include/hw/resettable.h
> >> @@ -0,0 +1,159 @@
> >> +#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)
> >> +
> >> +typedef struct ResetState ResetState;
> >> +
> >> +/**
> >> + * 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,
> >> + * ResetState 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 ResetState 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.init, 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 'init', 'hold', 'exit'.
> >> + * An object will always move quickly from 'init' 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
> >> + * 'init' method has been called.
> >> + *
> >> + * Users of a resettable object should not call these methods
> >> + * directly, but instead use the function resettable_reset().
> >> + *
> >> + * @phases.init: 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.init 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 ResetState.
> >> + *
> >> + * @foreach_child: Executes a given function 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
> >> + * an additional ResetType argument which is passed to foreach_child.
> >> + */
> >> +typedef void (*ResettableInitPhase)(Object *obj, ResetType type);
> >> +typedef void (*ResettableHoldPhase)(Object *obj);
> >> +typedef void (*ResettableExitPhase)(Object *obj);
> >> +typedef ResetState * (*ResettableGetState)(Object *obj);
> >> +typedef void (*ResettableForeachChild)(Object *obj,
> >> +                                       void (*func)(Object *, ResetType type),
> >> +                                       ResetType type);
> >> +typedef struct ResettableClass {
> >> +    InterfaceClass parent_class;
> >> +
> >> +    struct ResettablePhases {
> >> +        ResettableInitPhase init;
> >> +        ResettableHoldPhase hold;
> >> +        ResettableExitPhase exit;
> >> +    } phases;
> >> +
> >> +    ResettableGetState get_state;
> >> +    ResettableForeachChild foreach_child;
> >> +} ResettableClass;
> >> +typedef struct ResettablePhases ResettablePhases;
> >> +
> >> +/**
> >> + * ResetState:
> >> + * 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_needed: flag which indicates that we need to invoke the 'hold'
> >> + * phase handler for this object.
> >> + */
> >> +struct ResetState {
> >> +    uint32_t count;
> >> +    bool hold_phase_needed;
> >> +};
> >> +
> >> +/**
> >> + * resettable_is_resetting:
> >> + * Return true if @obj is under reset.
> >> + *
> >> + * @obj must implement Resettable interface.
> >> + */
> >> +bool resettable_is_resetting(Object *obj);
> >> +
> >> +/**
> >> + * 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_deassert_reset.
> >> + */
> >> +void resettable_reset(Object *obj, ResetType type);
> >> +
> >> +/**
> >> + * 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:
> >> + *
> >> + * Save @rc current reset phases into @parent_phases and override @rc phases
> >> + * by the given new methods (@init, @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,
> >> +                                        ResettableInitPhase init,
> >> +                                        ResettableHoldPhase hold,
> >> +                                        ResettableExitPhase exit,
> >> +                                        ResettablePhases *parent_phases);
> >> +
> >> +#endif
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

* Re: [PATCH v4 02/10] hw/core: create Resettable QOM interface
  2019-09-18  9:11       ` David Gibson
@ 2019-09-24 11:21         ` Damien Hedde
  2019-09-27 13:07           ` Peter Maydell
  0 siblings, 1 reply; 20+ messages in thread
From: Damien Hedde @ 2019-09-24 11:21 UTC (permalink / raw)
  To: David Gibson
  Cc: edgar.iglesias, peter.maydell, berrange, ehabkost, mark.burton,
	qemu-devel, pbonzini, philmd

Hi All,

Do you think I should respin with the sugestions made by David so far ?

+ reset type removal
+ s/init/enter/ for the phases terminology
+ handling of parent changes during reset

On 9/18/19 11:11 AM, David Gibson wrote:
> On Wed, Sep 11, 2019 at 04:56:13PM +0200, Damien Hedde wrote:
>>
>>
>> On 9/11/19 10:06 AM, David Gibson wrote:
>>> On Wed, Aug 21, 2019 at 06:33:33PM +0200, Damien Hedde wrote:
>>>> This commit defines an interface allowing multi-phase reset. 
>>>
>>> So, I certainly prefer the more general "reset type" approach taken in
>>> this version.  That said, I find it pretty hard to imagine what types
>>> of reset other than cold will exist that have well enough defined
>>> semantics to be meaningfully used from an external subsystem.
>>
>> Maybe I should completely remove the type then ?
> 
> That makes sense to me.  I don't know if other possible users of the
> mechanism have different opinions though.
> 
>>>
>>>> +static void resettable_init_reset(Object *obj, ResetType type)
>>>
>>> I wonder if "enter reset" would be better terminology so this doesn't
>>> get confused with the initial, well, initialization of the device.
>>
>> Do you mean for the function here or in general for the name of the phase ?
> 
> In general.
> 
>>>> +    /*
>>>> +     * we could assert that count > 0 but there are some corner cases
>>>> +     * where we prefer to let it go as it is probably harmless.
>>>> +     * For example: if there is reset support addition between
>>>> +     * hosts when doing a migration. We may do such things as
>>>> +     * deassert a non-existing reset.
>>>> +     */
>>>> +    if (s->count > 0) {
>>>> +        s->count -= 1;
>>>> +    } else {
>>>> +        trace_resettable_count_underflow(obj);
>>>
>>> Should this be an assert(), IIUC this could only come about from an
>>> error within the qemu code, right?
>>
>> Initially I was thinking that so I put an assert.
>>
>> But while testing I found out that it is triggered by the raspi machine
>> during its reset because the qbus tree is modified during it.
>>
>> So it depends if we consider this kind of action to be faulty. With no
>> migration support, the only way to trigger it is to modify the qdev
>> hierarchy during reset.
> 
> Hm, I see.  It feels like just ignoring underflow is ignoring the
> error rather than really addressing it.  When we add a device to the
> heirarchy, do we need to initialize its reset count based on its
> parent's current count or something.
> 

I can add that.

Thanks,
--
Damien


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

* Re: [PATCH v4 02/10] hw/core: create Resettable QOM interface
  2019-09-24 11:21         ` Damien Hedde
@ 2019-09-27 13:07           ` Peter Maydell
  2019-10-10  9:18             ` Damien Hedde
  0 siblings, 1 reply; 20+ messages in thread
From: Peter Maydell @ 2019-09-27 13:07 UTC (permalink / raw)
  To: Damien Hedde
  Cc: Edgar Iglesias, Daniel P. Berrange, Eduardo Habkost, Mark Burton,
	QEMU Developers, Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

On Tue, 24 Sep 2019 at 12:21, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Hi All,
>
> Do you think I should respin with the sugestions made by David so far ?
>
> + reset type removal
> + s/init/enter/ for the phases terminology
> + handling of parent changes during reset

My takes:
 * I think we should keep the reset type. Among other things,
   we probably want a reset type for "PCI bus reset" and
   "SCSI bus reset", when we come to conversion of those
 * I don't have an opinion about the phase names
 * I think we should look at what we're doing for dynamic
   changes of the reset tree. This falls into two parts,
   both of which have come up in this thread:
    - hotplug, ie what state should a hotplugged device
      get set up to if it's plugged into a bus that's
      currently in reset
    - the modification of the qbus tree during reset,
      like the raspi sd card thing
   These feel related to me, so maybe handling the first
   gives a better answer to handling the second ?

thanks
-- PMM


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

* Re: [PATCH v4 02/10] hw/core: create Resettable QOM interface
  2019-09-27 13:07           ` Peter Maydell
@ 2019-10-10  9:18             ` Damien Hedde
  0 siblings, 0 replies; 20+ messages in thread
From: Damien Hedde @ 2019-10-10  9:18 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Edgar Iglesias, Daniel P. Berrange, Eduardo Habkost, Mark Burton,
	QEMU Developers, Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

On 9/27/19 3:07 PM, Peter Maydell wrote:
> On Tue, 24 Sep 2019 at 12:21, Damien Hedde <damien.hedde@greensocs.com> wrote:
> 
> My takes:
>  * I think we should keep the reset type. Among other things,
>    we probably want a reset type for "PCI bus reset" and
>    "SCSI bus reset", when we come to conversion of those
>  * I don't have an opinion about the phase names
>  * I think we should look at what we're doing for dynamic
>    changes of the reset tree. This falls into two parts,
>    both of which have come up in this thread:
>     - hotplug, ie what state should a hotplugged device
>       get set up to if it's plugged into a bus that's
>       currently in reset
>     - the modification of the qbus tree during reset,
>       like the raspi sd card thing
>    These feel related to me, so maybe handling the first
>    gives a better answer to handling the second ?
> 

Sorry for the delayed answer, I did not had much time to work on this
last week.

Regarding hotplug, right now hotplugged device are reset during the
'realize' step. So here is what I propose:
+ we always do the phase 1 and 2.
+ do the 3rd phase (to leave reset) if the device is not plugged in a
bus under reset.

For general case, like the raspi, it can be handled in set_parent_bus()
qdev function.

Damien


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

end of thread, back to index

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-21 16:33 [Qemu-devel] [PATCH v4 00/10] Multi-phase reset mechanism Damien Hedde
2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 01/10] add device_legacy_reset function to prepare for reset api change Damien Hedde
2019-08-24  9:50   ` David Gibson
2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 02/10] hw/core: create Resettable QOM interface Damien Hedde
2019-09-11  8:06   ` David Gibson
2019-09-11 14:56     ` Damien Hedde
2019-09-18  9:11       ` David Gibson
2019-09-24 11:21         ` Damien Hedde
2019-09-27 13:07           ` Peter Maydell
2019-10-10  9:18             ` Damien Hedde
2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 03/10] hw/core: add Resettable interface in Bus and Device classes Damien Hedde
2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 04/10] docs/devel/reset.txt: create doc about Resettable interface Damien Hedde
2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 05/10] vl.c: replace deprecated qbus_reset_all registration Damien Hedde
2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 06/10] hw/s390x/ipl.c: " Damien Hedde
2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 07/10] hw/core/qdev: replace deprecated device_legacy_reset when hotplugging device Damien Hedde
2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 08/10] hw/core/resettable: add support for warm reset Damien Hedde
2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 09/10] hw/core/: add warm reset helpers for devices and buses Damien Hedde
2019-08-21 16:33 ` [Qemu-devel] [PATCH v4 10/10] docs/devel/reset.txt: add documentation about warm reset Damien Hedde
2019-08-21 17:09 ` [Qemu-devel] [PATCH v4 00/10] Multi-phase reset mechanism no-reply
2019-09-10 10:33 ` Damien Hedde

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox