[Qemu-devel] [PATCH v9 0/6] migration/ppc: migrating DRC, ccs_list and pending_events

[Qemu-devel] [PATCH v9 0/6] migration/ppc: migrating DRC, ccs_list and pending_events

[Daniel Henrique Barboza]

v9:
- patch 1 (*new*): added a qtail in sPAPRMachineState called pending_dimm_unplugs
that stores the DIMM LMB state during the unplug process.
- patch 2 (*new*): merged v8-patch1 and v8-patch2: removing detach_cb and
detach_cb_opaque.
- patch 3:
    * removed dk->vmsd entry. We're using vmstate_register instead
    * added 'awaiting_allocation' flag in the DRC migration
- patch 4 (*new*): migrating spapr->pending_dimm_unplugs qtailq to allow
for an ongoing PCDIMM unplug to continue after a migration.

v8:
- new patch added: 'removing spapr_drc_detach_cb opaques'. This new patch removes
the need for the detach_cb_opaques inside the removal callback functions. See
the commit message of the patch for more info.

v7:
- removed the first patch. DRC registration is now done by vmstate_register
in patch 2.
- added a new patch that changes spapr_dr_connector_new to receive as argument
the detach_cb.
- removed the callback logic of patch 2 since there is no need to restore the
detach_cb on post-load due to the detach_cb on spapr_dr_connector_new change.
- added word separators in the VMSD names of patch 3 and 4.

v6: - Rebased with QEMU master after 6+ months.
    - Simplified the logic in patch 1.
    - Reworked patch 2: added CPU DRC migration, removed a function pointer from DRC
class and minor improvements.
    - Added clarifications from the previous v5 discussions in the commit messages.

v5: - Rebased to David's ppc-for-2.8.

v4: - Introduce a way to set customized instance_id in SaveStateEntry. Use it
      to set instance_id for DRC using its unique index to address David 
      Gibson's concern.
    - Rename VMS_CSTM to VMS_LINKED based on Paolo Bonzini's suggestions.
    - Clean up qjson stuff in put_qtailq. 
    - Add trace for put_qtailq and get_qtailq based on David Gilbert's 
      suggestion.

    - Based on David's ppc-for-2.7. 

v3: - Simplify overall design followng discussion with Paolo. No longer need
      metadata to migrate QTAILQ.
    - Extend VMStateInfo instead of adding similar fields to VMStateField.
    - Clean up macros in qemu/queue.h.
(link: https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg05695.html)

v2: - Introduce a general approach to migrate QTAILQ in qemu/queue.h.
    - Migrate signalled field in the DRC state.
    - Put the newly added migrating fields in subsections so that backward 
      migration is not broken.  
    - Set detach_cb field right after migration so that a migrated hot-unplug
      event could finish its course.
(link: https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg04188.html)

v1: - Inital version.
(link: https://lists.nongnu.org/archive/html/qemu-devel/2016-04/msg02601.html)


To make guest devices (PCI, CPU and memory) hotplug work together 
with guest migration, spapr drc state needs be transmitted in
migration. This patch defines the VMStateDescription struct for
spapr drc state to enable it.

To fix the potential racing between hotplug events on guest and 
guest migration, ccs_list and pending_events of spapr state need be 
transmitted in migration. This patch set also takes care of it.



Daniel Henrique Barboza (4):
  hw/ppc/spapr.c: adding pending_dimm_unplugs to sPAPRMachineState
  hw/ppc: removing drc->detach_cb and drc->detach_cb_opaque
  hw/ppc: migrating the DRC state of hotplugged devices
  hw/ppc/spapr.c: migrate pending_dimm_unplugs of spapr state

Jianjun Duan (2):
  migration: spapr: migrate ccs_list in spapr state
  migration: spapr: migrate pending_events of spapr state

 hw/ppc/spapr.c              | 158 +++++++++++++++++++++++++++++++++++++++++---
 hw/ppc/spapr_drc.c          |  97 ++++++++++++++++++++++-----
 hw/ppc/spapr_events.c       |  24 ++++---
 hw/ppc/spapr_pci.c          |   5 +-
 include/hw/pci-host/spapr.h |   3 +
 include/hw/ppc/spapr.h      |  24 ++++++-
 include/hw/ppc/spapr_drc.h  |   8 +--
 7 files changed, 273 insertions(+), 46 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v9 1/6] hw/ppc/spapr.c: adding pending_dimm_unplugs to sPAPRMachineState

[Daniel Henrique Barboza]

The LMB DRC release callback, spapr_lmb_release(), uses an opaque
parameter, a sPAPRDIMMState struct that stores the current LMBs that
are allocated to a DIMM (nr_lmbs). After each call to this callback,
the nr_lmbs is decremented by one and, when it reaches zero, the callback
proceeds with the qdev calls to hot unplug the LMB.

Using drc->detach_cb_opaque is problematic because it can't be migrated in
the future DRC migration work. This patch makes the following changes to
eliminate the usage of this opaque callback inside spapr_lmb_release:

- sPAPRDIMMState was moved from spapr.c and added to spapr.h. A new
attribute called 'addr' was added to it. This is used as an unique
identifier to associate a sPAPRDIMMState to a PCDIMM element.

- sPAPRMachineState now hosts a new QTAILQ called 'pending_dimm_unplugs'.
This queue of sPAPRDIMMState elements will store the DIMM state of DIMMs
that are currently going under an unplug process.

- spapr_lmb_release() will now retrieve the nr_lmbs value by getting the
correspondent sPAPRDIMMState. A helper function called spapr_dimm_get_address
was created to fetch the address of a PCDIMM device inside spapr_lmb_release.
When nr_lmbs reaches zero and the callback proceeds with the qdev hot unplug
calls, the sPAPRDIMMState struct is removed from spapr->pending_dimm_unplugs.

After these changes, the opaque argument for spapr_lmb_release is now
unused and is passed as NULL inside spapr_del_lmbs. This and the other
opaque arguments can now be safely removed from the code.

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c         | 54 ++++++++++++++++++++++++++++++++++++++++++++------
 include/hw/ppc/spapr.h | 17 ++++++++++++++++
 2 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 80d12d0..346c827 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2043,6 +2043,7 @@ static void ppc_spapr_init(MachineState *machine)
     msi_nonbroken = true;
 
     QLIST_INIT(&spapr->phbs);
+    QTAILQ_INIT(&spapr->pending_dimm_unplugs);
 
     /* Allocate RMA if necessary */
     rma_alloc_size = kvmppc_alloc_rma(&rma);
@@ -2596,20 +2597,32 @@ out:
     error_propagate(errp, local_err);
 }
 
-typedef struct sPAPRDIMMState {
-    uint32_t nr_lmbs;
-} sPAPRDIMMState;
+static uint64_t spapr_dimm_get_address(PCDIMMDevice *dimm)
+{
+    Error *local_err = NULL;
+    uint64_t addr;
+    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP,
+                                   &local_err);
+    if (local_err) {
+        error_propagate(&error_abort, local_err);
+        return 0;
+    }
+    return addr;
+}
 
 static void spapr_lmb_release(DeviceState *dev, void *opaque)
 {
-    sPAPRDIMMState *ds = (sPAPRDIMMState *)opaque;
     HotplugHandler *hotplug_ctrl;
 
+    uint64_t addr = spapr_dimm_get_address(PC_DIMM(dev));
+    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+    sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, addr);
+
     if (--ds->nr_lmbs) {
         return;
     }
 
-    g_free(ds);
+    spapr_pending_dimm_unplugs_remove(spapr, ds);
 
     /*
      * Now that all the LMBs have been removed by the guest, call the
@@ -2626,17 +2639,20 @@ static void spapr_del_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
     sPAPRDRConnectorClass *drck;
     uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
     int i;
+    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     sPAPRDIMMState *ds = g_malloc0(sizeof(sPAPRDIMMState));
     uint64_t addr = addr_start;
 
     ds->nr_lmbs = nr_lmbs;
+    ds->addr = addr_start;
+    spapr_pending_dimm_unplugs_add(spapr, ds);
     for (i = 0; i < nr_lmbs; i++) {
         drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
                 addr / SPAPR_MEMORY_BLOCK_SIZE);
         g_assert(drc);
 
         drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-        drck->detach(drc, dev, spapr_lmb_release, ds, errp);
+        drck->detach(drc, dev, spapr_lmb_release, NULL, errp);
         addr += SPAPR_MEMORY_BLOCK_SIZE;
     }
 
@@ -3515,3 +3531,29 @@ static void spapr_machine_register_types(void)
 }
 
 type_init(spapr_machine_register_types)
+
+sPAPRDIMMState *spapr_pending_dimm_unplugs_find(sPAPRMachineState *spapr,
+                                                uint64_t addr)
+{
+    sPAPRDIMMState *dimm_state = NULL;
+    QTAILQ_FOREACH(dimm_state, &spapr->pending_dimm_unplugs, next) {
+        if (dimm_state->addr == addr) {
+            break;
+        }
+    }
+    return dimm_state;
+}
+
+void spapr_pending_dimm_unplugs_add(sPAPRMachineState *spapr,
+                                   sPAPRDIMMState *dimm_state)
+{
+    g_assert(!spapr_pending_dimm_unplugs_find(spapr, dimm_state->addr));
+    QTAILQ_INSERT_HEAD(&spapr->pending_dimm_unplugs, dimm_state, next);
+}
+
+void spapr_pending_dimm_unplugs_remove(sPAPRMachineState *spapr,
+                                      sPAPRDIMMState *dimm_state)
+{
+    QTAILQ_REMOVE(&spapr->pending_dimm_unplugs, dimm_state, next);
+    g_free(dimm_state);
+}
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 5802f88..3e2b5ab 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -32,6 +32,7 @@ struct sPAPRRTCState {
     int64_t ns_offset;
 };
 
+typedef struct sPAPRDIMMState sPAPRDIMMState;
 typedef struct sPAPRMachineClass sPAPRMachineClass;
 
 #define TYPE_SPAPR_MACHINE      "spapr-machine"
@@ -104,6 +105,9 @@ struct sPAPRMachineState {
     /* RTAS state */
     QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list;
 
+    /* pending DIMM unplug queue */
+    QTAILQ_HEAD(, sPAPRDIMMState) pending_dimm_unplugs;
+
     /*< public >*/
     char *kvm_type;
     MemoryHotplugState hotplug_memory;
@@ -646,6 +650,19 @@ struct sPAPRConfigureConnectorState {
 
 void spapr_ccs_reset_hook(void *opaque);
 
+struct sPAPRDIMMState {
+    uint64_t addr;
+    uint32_t nr_lmbs;
+    QTAILQ_ENTRY(sPAPRDIMMState) next;
+};
+
+sPAPRDIMMState *spapr_pending_dimm_unplugs_find(sPAPRMachineState *spapr,
+                                               uint64_t addr);
+void spapr_pending_dimm_unplugs_add(sPAPRMachineState *spapr,
+                                   sPAPRDIMMState *dimm_state);
+void spapr_pending_dimm_unplugs_remove(sPAPRMachineState *spapr,
+                                      sPAPRDIMMState *dimm_state);
+
 void spapr_rtc_read(sPAPRRTCState *rtc, struct tm *tm, uint32_t *ns);
 int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t legacy_offset);
 
-- 
2.9.3

[Qemu-devel] [PATCH v9 2/6] hw/ppc: removing drc->detach_cb and drc->detach_cb_opaque

[Daniel Henrique Barboza]

The pointer drc->detach_cb is being used as a way of informing
the detach() function inside spapr_drc.c which cb to execute. This
information can also be retrieved simply by checking drc->type and
choosing the right callback based on it. In this context, detach_cb
is redundant information that must be managed.

After the previous spapr_lmb_release change, no detach_cb_opaques
are being used by any of the three callbacks functions. This is
yet another information that is now unused and, on top of that, can't
be migrated either.

This patch makes the following changes:

- removal of detach_cb_opaque. the 'opaque' argument was removed from
the callbacks and from the detach() function of sPAPRConnectorClass. The
attribute detach_cb_opaque of sPAPRConnector was removed.

- removal of detach_cb from the detach() call. The function pointer
detach_cb of sPAPRConnector was removed. detach() now uses a
switch(drc->type) to execute the apropriate callback. To achieve this,
spapr_core_release, spapr_lmb_release and spapr_phb_remove_pci_device_cb
callbacks were made public to be visible inside detach().

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c              | 10 ++++++----
 hw/ppc/spapr_drc.c          | 36 ++++++++++++++++++++----------------
 hw/ppc/spapr_pci.c          |  5 +++--
 include/hw/pci-host/spapr.h |  3 +++
 include/hw/ppc/spapr.h      |  4 ++++
 include/hw/ppc/spapr_drc.h  |  8 +-------
 6 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 346c827..e190eb9 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2610,7 +2610,8 @@ static uint64_t spapr_dimm_get_address(PCDIMMDevice *dimm)
     return addr;
 }
 
-static void spapr_lmb_release(DeviceState *dev, void *opaque)
+/* Callback to be called during DRC release. */
+void spapr_lmb_release(DeviceState *dev)
 {
     HotplugHandler *hotplug_ctrl;
 
@@ -2652,7 +2653,7 @@ static void spapr_del_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
         g_assert(drc);
 
         drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-        drck->detach(drc, dev, spapr_lmb_release, NULL, errp);
+        drck->detach(drc, dev, errp);
         addr += SPAPR_MEMORY_BLOCK_SIZE;
     }
 
@@ -2728,7 +2729,8 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
     object_unparent(OBJECT(dev));
 }
 
-static void spapr_core_release(DeviceState *dev, void *opaque)
+/* Callback to be called during DRC release. */
+void spapr_core_release(DeviceState *dev)
 {
     HotplugHandler *hotplug_ctrl;
 
@@ -2761,7 +2763,7 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
     g_assert(drc);
 
     drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-    drck->detach(drc, dev, spapr_core_release, NULL, &local_err);
+    drck->detach(drc, dev, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index a1cdc87..1c72160 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -20,6 +20,7 @@
 #include "qapi/visitor.h"
 #include "qemu/error-report.h"
 #include "hw/ppc/spapr.h" /* for RTAS return codes */
+#include "hw/pci-host/spapr.h" /* spapr_phb_remove_pci_device_cb callback */
 #include "trace.h"
 
 #define DRC_CONTAINER_PATH "/dr-connector"
@@ -99,8 +100,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
         if (drc->awaiting_release) {
             if (drc->configured) {
                 trace_spapr_drc_set_isolation_state_finalizing(get_index(drc));
-                drck->detach(drc, DEVICE(drc->dev), drc->detach_cb,
-                             drc->detach_cb_opaque, NULL);
+                drck->detach(drc, DEVICE(drc->dev), NULL);
             } else {
                 trace_spapr_drc_set_isolation_state_deferring(get_index(drc));
             }
@@ -153,8 +153,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
         if (drc->awaiting_release &&
             drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
             trace_spapr_drc_set_allocation_state_finalizing(get_index(drc));
-            drck->detach(drc, DEVICE(drc->dev), drc->detach_cb,
-                         drc->detach_cb_opaque, NULL);
+            drck->detach(drc, DEVICE(drc->dev), NULL);
         } else if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) {
             drc->awaiting_allocation = false;
         }
@@ -404,15 +403,10 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
                              NULL, 0, NULL);
 }
 
-static void detach(sPAPRDRConnector *drc, DeviceState *d,
-                   spapr_drc_detach_cb *detach_cb,
-                   void *detach_cb_opaque, Error **errp)
+static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
 {
     trace_spapr_drc_detach(get_index(drc));
 
-    drc->detach_cb = detach_cb;
-    drc->detach_cb_opaque = detach_cb_opaque;
-
     /* if we've signalled device presence to the guest, or if the guest
      * has gone ahead and configured the device (via manually-executed
      * device add via drmgr in guest, namely), we need to wait
@@ -456,8 +450,21 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d,
 
     drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
 
-    if (drc->detach_cb) {
-        drc->detach_cb(drc->dev, drc->detach_cb_opaque);
+    /* Calling release callbacks based on drc->type. */
+    switch (drc->type) {
+    case SPAPR_DR_CONNECTOR_TYPE_CPU:
+        spapr_core_release(drc->dev);
+        break;
+    case SPAPR_DR_CONNECTOR_TYPE_PHB:
+    case SPAPR_DR_CONNECTOR_TYPE_VIO:
+    case SPAPR_DR_CONNECTOR_TYPE_PCI:
+        spapr_phb_remove_pci_device_cb(drc->dev);
+        break;
+    case SPAPR_DR_CONNECTOR_TYPE_LMB:
+        spapr_lmb_release(drc->dev);
+        break;
+    default:
+        ;
     }
 
     drc->awaiting_release = false;
@@ -467,8 +474,6 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d,
     drc->fdt_start_offset = 0;
     object_property_del(OBJECT(drc), "device", NULL);
     drc->dev = NULL;
-    drc->detach_cb = NULL;
-    drc->detach_cb_opaque = NULL;
 }
 
 static bool release_pending(sPAPRDRConnector *drc)
@@ -498,8 +503,7 @@ static void reset(DeviceState *d)
          * force removal if we are
          */
         if (drc->awaiting_release) {
-            drck->detach(drc, DEVICE(drc->dev), drc->detach_cb,
-                         drc->detach_cb_opaque, NULL);
+            drck->detach(drc, DEVICE(drc->dev), NULL);
         }
 
         /* non-PCI devices may be awaiting a transition to UNUSABLE */
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index e7567e2..5775db8 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1369,7 +1369,8 @@ out:
     }
 }
 
-static void spapr_phb_remove_pci_device_cb(DeviceState *dev, void *opaque)
+/* Callback to be called during DRC release. */
+void spapr_phb_remove_pci_device_cb(DeviceState *dev)
 {
     /* some version guests do not wait for completion of a device
      * cleanup (generally done asynchronously by the kernel) before
@@ -1392,7 +1393,7 @@ static void spapr_phb_remove_pci_device(sPAPRDRConnector *drc,
 {
     sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
 
-    drck->detach(drc, DEVICE(pdev), spapr_phb_remove_pci_device_cb, phb, errp);
+    drck->detach(drc, DEVICE(pdev), errp);
 }
 
 static sPAPRDRConnector *spapr_phb_get_pci_func_drc(sPAPRPHBState *phb,
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 1c2e970..38470b2 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -123,6 +123,9 @@ sPAPRPHBState *spapr_pci_find_phb(sPAPRMachineState *spapr, uint64_t buid);
 PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid,
                               uint32_t config_addr);
 
+/* PCI release callback. */
+void spapr_phb_remove_pci_device_cb(DeviceState *dev);
+
 /* VFIO EEH hooks */
 #ifdef CONFIG_LINUX
 bool spapr_phb_eeh_available(sPAPRPHBState *sphb);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 3e2b5ab..adc9fdb 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -640,6 +640,10 @@ void spapr_hotplug_req_remove_by_count_indexed(sPAPRDRConnectorType drc_type,
 void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
                                     sPAPRMachineState *spapr);
 
+/* CPU and LMB DRC release callbacks. */
+void spapr_core_release(DeviceState *dev);
+void spapr_lmb_release(DeviceState *dev);
+
 /* rtas-configure-connector state */
 struct sPAPRConfigureConnectorState {
     uint32_t drc_index;
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 5524247..813b9ff 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -130,8 +130,6 @@ typedef enum {
     SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE = -9003,
 } sPAPRDRCCResponse;
 
-typedef void (spapr_drc_detach_cb)(DeviceState *d, void *opaque);
-
 typedef struct sPAPRDRConnector {
     /*< private >*/
     DeviceState parent;
@@ -158,8 +156,6 @@ typedef struct sPAPRDRConnector {
 
     /* device pointer, via link property */
     DeviceState *dev;
-    spapr_drc_detach_cb *detach_cb;
-    void *detach_cb_opaque;
 } sPAPRDRConnector;
 
 typedef struct sPAPRDRConnectorClass {
@@ -188,9 +184,7 @@ typedef struct sPAPRDRConnectorClass {
     /* QEMU interfaces for managing hotplug operations */
     void (*attach)(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
                    int fdt_start_offset, bool coldplug, Error **errp);
-    void (*detach)(sPAPRDRConnector *drc, DeviceState *d,
-                   spapr_drc_detach_cb *detach_cb,
-                   void *detach_cb_opaque, Error **errp);
+    void (*detach)(sPAPRDRConnector *drc, DeviceState *d, Error **errp);
     bool (*release_pending)(sPAPRDRConnector *drc);
     void (*set_signalled)(sPAPRDRConnector *drc);
 } sPAPRDRConnectorClass;
-- 
2.9.3

[Qemu-devel] [PATCH v9 3/6] hw/ppc: migrating the DRC state of hotplugged devices

[Daniel Henrique Barboza]

In pseries, a firmware abstraction called Dynamic Reconfiguration
Connector (DRC) is used to assign a particular dynamic resource
to the guest and provide an interface to manage configuration/removal
of the resource associated with it. In other words, DRC is the
'plugged state' of a device.

Before this patch, DRC wasn't being migrated. This causes
post-migration problems due to DRC state mismatch between source and
target. The DRC state of a device X in the source might
change, while in the target the DRC state of X is still fresh. When
migrating the guest, X will not have the same hotplugged state as it
did in the source. This means that we can't hot unplug X in the
target after migration is completed because its DRC state is not consistent.
https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1677552 is one
bug that is caused by this DRC state mismatch between source and
target.

To migrate the DRC state, we defined the VMStateDescription struct for
spapr_drc to enable the transmission of spapr_drc state in migration.
Not all the elements in the DRC state are migrated - only those
that can be modified by guest actions or device add/remove
operations:

- 'isolation_state', 'allocation_state' and 'indicator_state'
are involved in the DR state transition diagram from
PAPR+ 2.7, 13.4;

- 'configured', 'signalled', 'awaiting_release' and 'awaiting_allocation'
are needed in attaching and detaching devices;

- 'indicator_state' provides users with hardware state information.

These are the DRC elements that are migrated.

In this patch the DRC state is migrated for PCI, LMB and CPU
connector types. At this moment there is no support to migrate
DRC for the PHB (PCI Host Bridge) type.

In the 'realize' function the DRC is registered using vmstate_register,
similar to what hw/ppc/spapr_iommu.c does in 'spapr_tce_table_realize'.
This approach works because  DRCs are bus-less and do not sit
on a BusClass that implements bc->get_dev_path, so as a fallback the
VMSD gets identified via "spapr_drc"/get_index(drc).

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
---
 hw/ppc/spapr_drc.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 1c72160..926b945 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -519,6 +519,65 @@ static void reset(DeviceState *d)
     }
 }
 
+static bool spapr_drc_needed(void *opaque)
+{
+    sPAPRDRConnector *drc = (sPAPRDRConnector *)opaque;
+    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+    bool rc = false;
+    sPAPRDREntitySense value;
+    drck->entity_sense(drc, &value);
+    /* If no dev is plugged in there is no need to migrate the DRC state */
+    if (value != SPAPR_DR_ENTITY_SENSE_PRESENT) {
+        return false;
+    }
+
+    /*
+     * If there is dev plugged in, we need to migrate the DRC state when
+     * it is different from cold-plugged state
+     */
+    switch (drc->type) {
+
+    case SPAPR_DR_CONNECTOR_TYPE_PCI:
+        rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) &&
+               (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) &&
+               drc->configured && drc->signalled && !drc->awaiting_release);
+        break;
+
+    case SPAPR_DR_CONNECTOR_TYPE_LMB:
+        rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
+               (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) &&
+               drc->configured && drc->signalled && !drc->awaiting_release);
+        break;
+
+    case SPAPR_DR_CONNECTOR_TYPE_CPU:
+        rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
+               (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) &&
+                drc->configured && drc->signalled && !drc->awaiting_release);
+        break;
+
+    default:
+        ;
+    }
+    return rc;
+}
+
+static const VMStateDescription vmstate_spapr_drc = {
+    .name = "spapr_drc",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = spapr_drc_needed,
+    .fields  = (VMStateField []) {
+        VMSTATE_UINT32(isolation_state, sPAPRDRConnector),
+        VMSTATE_UINT32(allocation_state, sPAPRDRConnector),
+        VMSTATE_UINT32(indicator_state, sPAPRDRConnector),
+        VMSTATE_BOOL(configured, sPAPRDRConnector),
+        VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
+        VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
+        VMSTATE_BOOL(signalled, sPAPRDRConnector),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void realize(DeviceState *d, Error **errp)
 {
     sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
@@ -547,6 +606,8 @@ static void realize(DeviceState *d, Error **errp)
         object_unref(OBJECT(drc));
     }
     g_free(child_name);
+    vmstate_register(DEVICE(drc), drck->get_index(drc), &vmstate_spapr_drc,
+                     drc);
     trace_spapr_drc_realize_complete(drck->get_index(drc));
 }
 
-- 
2.9.3

[Qemu-devel] [PATCH v9 4/6] hw/ppc/spapr.c: migrate pending_dimm_unplugs of spapr state

[Daniel Henrique Barboza]

To allow for a DIMM unplug event to resume its work if a migration
occurs in the middle of it, this patch migrates the non-empty
pending_dimm_unplugs QTAILQ that stores the DIMM information
that the spapr_lmb_release() callback uses.

It was considered an apprach where the DIMM states would be restored
on the post-_load after a migration. The problem is that there is
no way of knowing, from the sPAPRMachineState, if a given DIMM is going
through an unplug process and the callback needs the updated DIMM State.

We could migrate a flag indicating that there is an unplug event going
on for a certain DIMM, fetching this information from the start of the
spapr_del_lmbs call. But this would also require a scan on post_load to
figure out how many nr_lmbs are left. At this point we can just
migrate the nr_lmbs information as well, given that it is being calculated
at spapr_del_lmbs already, and spare a scanning/discovery in the
post-load. All that we need is inside the sPAPRDIMMState structure
that is added to the pending_dimm_unplugs queue at the start of the
spapr_del_lmbs, so it's convenient to just migrated this queue it if it's
not empty.

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e190eb9..30f0b7b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1437,6 +1437,36 @@ static bool version_before_3(void *opaque, int version_id)
     return version_id < 3;
 }
 
+static bool spapr_pending_dimm_unplugs_needed(void *opaque)
+{
+    sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
+    return !QTAILQ_EMPTY(&spapr->pending_dimm_unplugs);
+}
+
+static const VMStateDescription vmstate_spapr_dimmstate = {
+    .name = "spapr_dimm_state",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(addr, sPAPRDIMMState),
+        VMSTATE_UINT32(nr_lmbs, sPAPRDIMMState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static const VMStateDescription vmstate_spapr_pending_dimm_unplugs = {
+    .name = "spapr_pending_dimm_unplugs",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = spapr_pending_dimm_unplugs_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_QTAILQ_V(pending_dimm_unplugs, sPAPRMachineState, 1,
+                         vmstate_spapr_dimmstate, sPAPRDIMMState,
+                         next),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static bool spapr_ov5_cas_needed(void *opaque)
 {
     sPAPRMachineState *spapr = opaque;
@@ -1535,6 +1565,7 @@ static const VMStateDescription vmstate_spapr = {
     .subsections = (const VMStateDescription*[]) {
         &vmstate_spapr_ov5_cas,
         &vmstate_spapr_patb_entry,
+        &vmstate_spapr_pending_dimm_unplugs,
         NULL
     }
 };
-- 
2.9.3

[Qemu-devel] [PATCH v9 5/6] migration: spapr: migrate ccs_list in spapr state

[Daniel Henrique Barboza]

From: Jianjun Duan <duanj@linux.vnet.ibm.com>

ccs_list in spapr state maintains the device tree related
information on the rtas side for hotplugged devices. In racing
situations between hotplug events and migration operation, a rtas
hotplug event could be migrated from the source guest to target
guest, or the source guest could have not yet finished fetching
the device tree when migration is started, the target will try
to finish fetching the device tree. By migrating ccs_list, the
target can fetch the device tree properly.

In theory there would be other alternatives besides migrating the
css_list to fix this. For example, we could add a flag that indicates
whether a device is in the middle of the configure_connector during the
migration process, in the post_load we can detect if this flag
is active and then return an error informing the guest to restart the
hotplug process. However, the DRC state can still be modified outside of
hotplug. Using:

   drmgr -c pci -s <drc_index> -r
   drmgr -c pci -s <drc_index> -a

it is possible to return a device to firmware and then later take it
back and reconfigure it. This is not a common case but it's not prohibited,
and performing a migration between these 2 operations would fail because
the default coldplug state on target assumes a configured state in
the source*. Migrating ccs_list is one solution that cover this
case as well.

ccs_list is put in a subsection in the spapr state VMSD to make
sure migration across different versions is not broken.

* see http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg01763.html
for more information on this discussion.

Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 30f0b7b..bc56249 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1467,6 +1467,37 @@ static const VMStateDescription vmstate_spapr_pending_dimm_unplugs = {
     },
 };
 
+static bool spapr_ccs_list_needed(void *opaque)
+{
+    sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
+    return !QTAILQ_EMPTY(&spapr->ccs_list);
+}
+
+static const VMStateDescription vmstate_spapr_ccs = {
+    .name = "spapr_configure_connector_state",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(drc_index, sPAPRConfigureConnectorState),
+        VMSTATE_INT32(fdt_offset, sPAPRConfigureConnectorState),
+        VMSTATE_INT32(fdt_depth, sPAPRConfigureConnectorState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static const VMStateDescription vmstate_spapr_ccs_list = {
+    .name = "spapr_ccs_list",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = spapr_ccs_list_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_QTAILQ_V(ccs_list, sPAPRMachineState, 1,
+                         vmstate_spapr_ccs, sPAPRConfigureConnectorState,
+                         next),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static bool spapr_ov5_cas_needed(void *opaque)
 {
     sPAPRMachineState *spapr = opaque;
@@ -1566,6 +1597,7 @@ static const VMStateDescription vmstate_spapr = {
         &vmstate_spapr_ov5_cas,
         &vmstate_spapr_patb_entry,
         &vmstate_spapr_pending_dimm_unplugs,
+        &vmstate_spapr_ccs_list,
         NULL
     }
 };
-- 
2.9.3

[Qemu-devel] [PATCH v9 6/6] migration: spapr: migrate pending_events of spapr state

[Daniel Henrique Barboza]

From: Jianjun Duan <duanj@linux.vnet.ibm.com>

In racing situations between hotplug events and migration operation,
a rtas hotplug event could have not yet be delivered to the source
guest when migration is started. In this case the pending_events of
spapr state need be transmitted to the target so that the hotplug
event can be finished on the target.

All the different fields of the events are encoded as defined by
PAPR. We can migrate them as uint8_t binary stream without any
concerns about data padding or endianess.

pending_events is put in a subsection in the spapr state VMSD to make
sure migration across different versions is not broken.

Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c         | 33 +++++++++++++++++++++++++++++++++
 hw/ppc/spapr_events.c  | 24 +++++++++++++-----------
 include/hw/ppc/spapr.h |  3 ++-
 3 files changed, 48 insertions(+), 12 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index bc56249..e924fd4 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1498,6 +1498,38 @@ static const VMStateDescription vmstate_spapr_ccs_list = {
     },
 };
 
+static bool spapr_pending_events_needed(void *opaque)
+{
+    sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
+    return !QTAILQ_EMPTY(&spapr->pending_events);
+}
+
+static const VMStateDescription vmstate_spapr_event_entry = {
+    .name = "spapr_event_log_entry",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT32(log_type, sPAPREventLogEntry),
+        VMSTATE_BOOL(exception, sPAPREventLogEntry),
+        VMSTATE_UINT32(data_size, sPAPREventLogEntry),
+        VMSTATE_VARRAY_UINT32_ALLOC(data, sPAPREventLogEntry, data_size,
+                                    0, vmstate_info_uint8, uint8_t),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static const VMStateDescription vmstate_spapr_pending_events = {
+    .name = "spapr_pending_events",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = spapr_pending_events_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_QTAILQ_V(pending_events, sPAPRMachineState, 1,
+                         vmstate_spapr_event_entry, sPAPREventLogEntry, next),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static bool spapr_ov5_cas_needed(void *opaque)
 {
     sPAPRMachineState *spapr = opaque;
@@ -1598,6 +1630,7 @@ static const VMStateDescription vmstate_spapr = {
         &vmstate_spapr_patb_entry,
         &vmstate_spapr_pending_dimm_unplugs,
         &vmstate_spapr_ccs_list,
+        &vmstate_spapr_pending_events,
         NULL
     }
 };
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index f0b28d8..70c7cfc 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -342,7 +342,8 @@ static int rtas_event_log_to_irq(sPAPRMachineState *spapr, int log_type)
     return source->irq;
 }
 
-static void rtas_event_log_queue(int log_type, void *data, bool exception)
+static void rtas_event_log_queue(int log_type, void *data, bool exception,
+                                 int data_size)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     sPAPREventLogEntry *entry = g_new(sPAPREventLogEntry, 1);
@@ -351,6 +352,7 @@ static void rtas_event_log_queue(int log_type, void *data, bool exception)
     entry->log_type = log_type;
     entry->exception = exception;
     entry->data = data;
+    entry->data_size = data_size;
     QTAILQ_INSERT_TAIL(&spapr->pending_events, entry, next);
 }
 
@@ -445,6 +447,7 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
     struct rtas_event_log_v6_mainb *mainb;
     struct rtas_event_log_v6_epow *epow;
     struct epow_log_full *new_epow;
+    uint32_t data_size;
 
     new_epow = g_malloc0(sizeof(*new_epow));
     hdr = &new_epow->hdr;
@@ -453,14 +456,13 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
     mainb = &new_epow->mainb;
     epow = &new_epow->epow;
 
+    data_size = sizeof(*new_epow);
     hdr->summary = cpu_to_be32(RTAS_LOG_VERSION_6
                                | RTAS_LOG_SEVERITY_EVENT
                                | RTAS_LOG_DISPOSITION_NOT_RECOVERED
                                | RTAS_LOG_OPTIONAL_PART_PRESENT
                                | RTAS_LOG_TYPE_EPOW);
-    hdr->extended_length = cpu_to_be32(sizeof(*new_epow)
-                                       - sizeof(new_epow->hdr));
-
+    hdr->extended_length = cpu_to_be32(data_size - sizeof(new_epow->hdr));
     spapr_init_v6hdr(v6hdr);
     spapr_init_maina(maina, 3 /* Main-A, Main-B and EPOW */);
 
@@ -479,7 +481,7 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
     epow->event_modifier = RTAS_LOG_V6_EPOW_MODIFIER_NORMAL;
     epow->extended_modifier = RTAS_LOG_V6_EPOW_XMODIFIER_PARTITION_SPECIFIC;
 
-    rtas_event_log_queue(RTAS_LOG_TYPE_EPOW, new_epow, true);
+    rtas_event_log_queue(RTAS_LOG_TYPE_EPOW, new_epow, true, data_size);
 
     qemu_irq_pulse(xics_get_qirq(XICS_FABRIC(spapr),
                                  rtas_event_log_to_irq(spapr,
@@ -504,6 +506,7 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
     struct rtas_event_log_v6_maina *maina;
     struct rtas_event_log_v6_mainb *mainb;
     struct rtas_event_log_v6_hp *hp;
+    uint32_t data_size;
 
     new_hp = g_malloc0(sizeof(struct hp_log_full));
     hdr = &new_hp->hdr;
@@ -512,14 +515,14 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
     mainb = &new_hp->mainb;
     hp = &new_hp->hp;
 
+    data_size = sizeof(*new_hp);
     hdr->summary = cpu_to_be32(RTAS_LOG_VERSION_6
                                | RTAS_LOG_SEVERITY_EVENT
                                | RTAS_LOG_DISPOSITION_NOT_RECOVERED
                                | RTAS_LOG_OPTIONAL_PART_PRESENT
                                | RTAS_LOG_INITIATOR_HOTPLUG
                                | RTAS_LOG_TYPE_HOTPLUG);
-    hdr->extended_length = cpu_to_be32(sizeof(*new_hp)
-                                       - sizeof(new_hp->hdr));
+    hdr->extended_length = cpu_to_be32(data_size - sizeof(new_hp->hdr));
 
     spapr_init_v6hdr(v6hdr);
     spapr_init_maina(maina, 3 /* Main-A, Main-B, HP */);
@@ -572,7 +575,7 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
             cpu_to_be32(drc_id->count_indexed.index);
     }
 
-    rtas_event_log_queue(RTAS_LOG_TYPE_HOTPLUG, new_hp, true);
+    rtas_event_log_queue(RTAS_LOG_TYPE_HOTPLUG, new_hp, true, data_size);
 
     qemu_irq_pulse(xics_get_qirq(XICS_FABRIC(spapr),
                                  rtas_event_log_to_irq(spapr,
@@ -671,8 +674,7 @@ static void check_exception(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     if (!event) {
         goto out_no_events;
     }
-
-    hdr = event->data;
+    hdr = (struct rtas_error_log *)event->data;
     event_len = be32_to_cpu(hdr->extended_length) + sizeof(*hdr);
 
     if (event_len < len) {
@@ -728,7 +730,7 @@ static void event_scan(PowerPCCPU *cpu, sPAPRMachineState *spapr,
         goto out_no_events;
     }
 
-    hdr = event->data;
+    hdr = (struct rtas_error_log *)event->data;
     event_len = be32_to_cpu(hdr->extended_length) + sizeof(*hdr);
 
     if (event_len < len) {
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index adc9fdb..ddac014 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -603,7 +603,8 @@ sPAPRTCETable *spapr_tce_find_by_liobn(target_ulong liobn);
 struct sPAPREventLogEntry {
     int log_type;
     bool exception;
-    void *data;
+    uint8_t *data;
+    uint32_t data_size;
     QTAILQ_ENTRY(sPAPREventLogEntry) next;
 };
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH v9 0/6] migration/ppc: migrating DRC, ccs_list and pending_events

[Laurent Vivier]

Hi Daniel,

I'd like to test your patch series: is there a way to generate easily
the problem your series wants to fix?

Thanks,
Laurent

On 05/05/2017 22:47, Daniel Henrique Barboza wrote:
> v9:
> - patch 1 (*new*): added a qtail in sPAPRMachineState called pending_dimm_unplugs
> that stores the DIMM LMB state during the unplug process.
> - patch 2 (*new*): merged v8-patch1 and v8-patch2: removing detach_cb and
> detach_cb_opaque.
> - patch 3:
>     * removed dk->vmsd entry. We're using vmstate_register instead
>     * added 'awaiting_allocation' flag in the DRC migration
> - patch 4 (*new*): migrating spapr->pending_dimm_unplugs qtailq to allow
> for an ongoing PCDIMM unplug to continue after a migration.
> 
> v8:
> - new patch added: 'removing spapr_drc_detach_cb opaques'. This new patch removes
> the need for the detach_cb_opaques inside the removal callback functions. See
> the commit message of the patch for more info.
> 
> v7:
> - removed the first patch. DRC registration is now done by vmstate_register
> in patch 2.
> - added a new patch that changes spapr_dr_connector_new to receive as argument
> the detach_cb.
> - removed the callback logic of patch 2 since there is no need to restore the
> detach_cb on post-load due to the detach_cb on spapr_dr_connector_new change.
> - added word separators in the VMSD names of patch 3 and 4.
> 
> v6: - Rebased with QEMU master after 6+ months.
>     - Simplified the logic in patch 1.
>     - Reworked patch 2: added CPU DRC migration, removed a function pointer from DRC
> class and minor improvements.
>     - Added clarifications from the previous v5 discussions in the commit messages.
> 
> v5: - Rebased to David's ppc-for-2.8.
> 
> v4: - Introduce a way to set customized instance_id in SaveStateEntry. Use it
>       to set instance_id for DRC using its unique index to address David 
>       Gibson's concern.
>     - Rename VMS_CSTM to VMS_LINKED based on Paolo Bonzini's suggestions.
>     - Clean up qjson stuff in put_qtailq. 
>     - Add trace for put_qtailq and get_qtailq based on David Gilbert's 
>       suggestion.
> 
>     - Based on David's ppc-for-2.7. 
> 
> v3: - Simplify overall design followng discussion with Paolo. No longer need
>       metadata to migrate QTAILQ.
>     - Extend VMStateInfo instead of adding similar fields to VMStateField.
>     - Clean up macros in qemu/queue.h.
> (link: https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg05695.html)
> 
> v2: - Introduce a general approach to migrate QTAILQ in qemu/queue.h.
>     - Migrate signalled field in the DRC state.
>     - Put the newly added migrating fields in subsections so that backward 
>       migration is not broken.  
>     - Set detach_cb field right after migration so that a migrated hot-unplug
>       event could finish its course.
> (link: https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg04188.html)
> 
> v1: - Inital version.
> (link: https://lists.nongnu.org/archive/html/qemu-devel/2016-04/msg02601.html)
> 
> 
> To make guest devices (PCI, CPU and memory) hotplug work together 
> with guest migration, spapr drc state needs be transmitted in
> migration. This patch defines the VMStateDescription struct for
> spapr drc state to enable it.
> 
> To fix the potential racing between hotplug events on guest and 
> guest migration, ccs_list and pending_events of spapr state need be 
> transmitted in migration. This patch set also takes care of it.
> 
> 
> 
> Daniel Henrique Barboza (4):
>   hw/ppc/spapr.c: adding pending_dimm_unplugs to sPAPRMachineState
>   hw/ppc: removing drc->detach_cb and drc->detach_cb_opaque
>   hw/ppc: migrating the DRC state of hotplugged devices
>   hw/ppc/spapr.c: migrate pending_dimm_unplugs of spapr state
> 
> Jianjun Duan (2):
>   migration: spapr: migrate ccs_list in spapr state
>   migration: spapr: migrate pending_events of spapr state
> 
>  hw/ppc/spapr.c              | 158 +++++++++++++++++++++++++++++++++++++++++---
>  hw/ppc/spapr_drc.c          |  97 ++++++++++++++++++++++-----
>  hw/ppc/spapr_events.c       |  24 ++++---
>  hw/ppc/spapr_pci.c          |   5 +-
>  include/hw/pci-host/spapr.h |   3 +
>  include/hw/ppc/spapr.h      |  24 ++++++-
>  include/hw/ppc/spapr_drc.h  |   8 +--
>  7 files changed, 273 insertions(+), 46 deletions(-)
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH v9 0/6] migration/ppc: migrating DRC, ccs_list and pending_events

[Daniel Henrique Barboza]

Hi Laurent,

To easily reproduce the error scenario:

1 - start QEMU in both source and target host with the same devices 
(target qemu
with the extra --incoming to accept the migration)

2 - in both QEMU instances add one or more cpus with device add:

device_add host-spapr-cpu-core,id=core1,core-id=1

3 - execute the migration

4 - after the migration is completed try to detach the cpu(s) you added 
in the target QEMU:

device_del core1

There will be no errors thrown in the QEMU console but you'll notice in the
VM that the cpus will still be there. dmesg will show something like this:

[  +0.482307] cgroup: new mount options do not match the existing 
superblock, will be ignored
[Apr19 14:47] random: crng init done
[Apr19 14:51] pseries-hotplug-cpu: CPU with drc index 10000008 already 
exists
[  +0.020319] cpu 1 (hwid 8) Ready to die...
[  +0.020550] pseries-hotplug-cpu: Failed to release drc (10000008) for 
CPU <NULL>, rc: -1
[Apr19 14:56] cpu 1 (hwid 8) Ready to die...
[  +0.014549] pseries-hotplug-cpu: Failed to release drc (10000008) for 
CPU <NULL>, rc: -1


The bug was originally reproduced using virsh:

# virsh setvcpus avocado-vt-vm1-migration 64 --live

# virsh -c 'qemu:///system' migrate --live --domain 
avocado-vt-vm1-migration --desturi qemu+ssh://<target_ip>/system 
--timeout 60

# virsh -c 'qemu+ssh://<target_ip>/system' setvcpus 
avocado-vt-vm1-migration 8 --live
error: operation failed: vcpu unplug request timed out


This happens because virsh is hot plugging the CPUs in both source and 
target
QEMUs and then migrating the VM, instead of starting the target QEMU 
with the
extra CPUs. When trying to hot unplug the CPUs after the migration, it 
fails because
these hot plugged CPUs at the target didn't pass the DRC state changes that
happened in the source.

This patch series aims to solve this scenario by migrating the relevant 
DRC states
to allow for proper device removal after a migration.


You can see more info in this launchpad bug:

https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1677552


Thanks,


Daniel


On 05/11/2017 03:38 PM, Laurent Vivier wrote:
> Hi Daniel,
>
> I'd like to test your patch series: is there a way to generate easily
> the problem your series wants to fix?
>
> Thanks,
> Laurent
>
> On 05/05/2017 22:47, Daniel Henrique Barboza wrote:
>> v9:
>> - patch 1 (*new*): added a qtail in sPAPRMachineState called pending_dimm_unplugs
>> that stores the DIMM LMB state during the unplug process.
>> - patch 2 (*new*): merged v8-patch1 and v8-patch2: removing detach_cb and
>> detach_cb_opaque.
>> - patch 3:
>>      * removed dk->vmsd entry. We're using vmstate_register instead
>>      * added 'awaiting_allocation' flag in the DRC migration
>> - patch 4 (*new*): migrating spapr->pending_dimm_unplugs qtailq to allow
>> for an ongoing PCDIMM unplug to continue after a migration.
>>
>> v8:
>> - new patch added: 'removing spapr_drc_detach_cb opaques'. This new patch removes
>> the need for the detach_cb_opaques inside the removal callback functions. See
>> the commit message of the patch for more info.
>>
>> v7:
>> - removed the first patch. DRC registration is now done by vmstate_register
>> in patch 2.
>> - added a new patch that changes spapr_dr_connector_new to receive as argument
>> the detach_cb.
>> - removed the callback logic of patch 2 since there is no need to restore the
>> detach_cb on post-load due to the detach_cb on spapr_dr_connector_new change.
>> - added word separators in the VMSD names of patch 3 and 4.
>>
>> v6: - Rebased with QEMU master after 6+ months.
>>      - Simplified the logic in patch 1.
>>      - Reworked patch 2: added CPU DRC migration, removed a function pointer from DRC
>> class and minor improvements.
>>      - Added clarifications from the previous v5 discussions in the commit messages.
>>
>> v5: - Rebased to David's ppc-for-2.8.
>>
>> v4: - Introduce a way to set customized instance_id in SaveStateEntry. Use it
>>        to set instance_id for DRC using its unique index to address David
>>        Gibson's concern.
>>      - Rename VMS_CSTM to VMS_LINKED based on Paolo Bonzini's suggestions.
>>      - Clean up qjson stuff in put_qtailq.
>>      - Add trace for put_qtailq and get_qtailq based on David Gilbert's
>>        suggestion.
>>
>>      - Based on David's ppc-for-2.7.
>>
>> v3: - Simplify overall design followng discussion with Paolo. No longer need
>>        metadata to migrate QTAILQ.
>>      - Extend VMStateInfo instead of adding similar fields to VMStateField.
>>      - Clean up macros in qemu/queue.h.
>> (link: https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg05695.html)
>>
>> v2: - Introduce a general approach to migrate QTAILQ in qemu/queue.h.
>>      - Migrate signalled field in the DRC state.
>>      - Put the newly added migrating fields in subsections so that backward
>>        migration is not broken.
>>      - Set detach_cb field right after migration so that a migrated hot-unplug
>>        event could finish its course.
>> (link: https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg04188.html)
>>
>> v1: - Inital version.
>> (link: https://lists.nongnu.org/archive/html/qemu-devel/2016-04/msg02601.html)
>>
>>
>> To make guest devices (PCI, CPU and memory) hotplug work together
>> with guest migration, spapr drc state needs be transmitted in
>> migration. This patch defines the VMStateDescription struct for
>> spapr drc state to enable it.
>>
>> To fix the potential racing between hotplug events on guest and
>> guest migration, ccs_list and pending_events of spapr state need be
>> transmitted in migration. This patch set also takes care of it.
>>
>>
>>
>> Daniel Henrique Barboza (4):
>>    hw/ppc/spapr.c: adding pending_dimm_unplugs to sPAPRMachineState
>>    hw/ppc: removing drc->detach_cb and drc->detach_cb_opaque
>>    hw/ppc: migrating the DRC state of hotplugged devices
>>    hw/ppc/spapr.c: migrate pending_dimm_unplugs of spapr state
>>
>> Jianjun Duan (2):
>>    migration: spapr: migrate ccs_list in spapr state
>>    migration: spapr: migrate pending_events of spapr state
>>
>>   hw/ppc/spapr.c              | 158 +++++++++++++++++++++++++++++++++++++++++---
>>   hw/ppc/spapr_drc.c          |  97 ++++++++++++++++++++++-----
>>   hw/ppc/spapr_events.c       |  24 ++++---
>>   hw/ppc/spapr_pci.c          |   5 +-
>>   include/hw/pci-host/spapr.h |   3 +
>>   include/hw/ppc/spapr.h      |  24 ++++++-
>>   include/hw/ppc/spapr_drc.h  |   8 +--
>>   7 files changed, 273 insertions(+), 46 deletions(-)
>>
>
>

Re: [Qemu-devel] [PATCH v9 1/6] hw/ppc/spapr.c: adding pending_dimm_unplugs to sPAPRMachineState

[David Gibson]

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

On Fri, May 05, 2017 at 05:47:41PM -0300, Daniel Henrique Barboza wrote:
> The LMB DRC release callback, spapr_lmb_release(), uses an opaque
> parameter, a sPAPRDIMMState struct that stores the current LMBs that
> are allocated to a DIMM (nr_lmbs). After each call to this callback,
> the nr_lmbs is decremented by one and, when it reaches zero, the callback
> proceeds with the qdev calls to hot unplug the LMB.
> 
> Using drc->detach_cb_opaque is problematic because it can't be migrated in
> the future DRC migration work. This patch makes the following changes to
> eliminate the usage of this opaque callback inside spapr_lmb_release:
> 
> - sPAPRDIMMState was moved from spapr.c and added to spapr.h. A new
> attribute called 'addr' was added to it. This is used as an unique
> identifier to associate a sPAPRDIMMState to a PCDIMM element.
> 
> - sPAPRMachineState now hosts a new QTAILQ called 'pending_dimm_unplugs'.
> This queue of sPAPRDIMMState elements will store the DIMM state of DIMMs
> that are currently going under an unplug process.
> 
> - spapr_lmb_release() will now retrieve the nr_lmbs value by getting the
> correspondent sPAPRDIMMState. A helper function called spapr_dimm_get_address
> was created to fetch the address of a PCDIMM device inside spapr_lmb_release.
> When nr_lmbs reaches zero and the callback proceeds with the qdev hot unplug
> calls, the sPAPRDIMMState struct is removed from spapr->pending_dimm_unplugs.
> 
> After these changes, the opaque argument for spapr_lmb_release is now
> unused and is passed as NULL inside spapr_del_lmbs. This and the other
> opaque arguments can now be safely removed from the code.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>

Urgh.  Moving this into the machine is really ugly.  Unfortunately, I
can't quickly see a better way to accomplish what you need.  So I
guess this approach is ok, with the hope that we can find a better way
in future.

There are a few more superficial problems to address, though.

> ---
>  hw/ppc/spapr.c         | 54 ++++++++++++++++++++++++++++++++++++++++++++------
>  include/hw/ppc/spapr.h | 17 ++++++++++++++++
>  2 files changed, 65 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 80d12d0..346c827 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2043,6 +2043,7 @@ static void ppc_spapr_init(MachineState *machine)
>      msi_nonbroken = true;
>  
>      QLIST_INIT(&spapr->phbs);
> +    QTAILQ_INIT(&spapr->pending_dimm_unplugs);
>  
>      /* Allocate RMA if necessary */
>      rma_alloc_size = kvmppc_alloc_rma(&rma);
> @@ -2596,20 +2597,32 @@ out:
>      error_propagate(errp, local_err);
>  }
>  
> -typedef struct sPAPRDIMMState {
> -    uint32_t nr_lmbs;
> -} sPAPRDIMMState;
> +static uint64_t spapr_dimm_get_address(PCDIMMDevice *dimm)
> +{
> +    Error *local_err = NULL;
> +    uint64_t addr;
> +    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> +                                   &local_err);
> +    if (local_err) {
> +        error_propagate(&error_abort, local_err);
> +        return 0;
> +    }
> +    return addr;
> +}
>  
>  static void spapr_lmb_release(DeviceState *dev, void *opaque)
>  {
> -    sPAPRDIMMState *ds = (sPAPRDIMMState *)opaque;
>      HotplugHandler *hotplug_ctrl;
>  

No need for this blank line in the middle of declarations.

> +    uint64_t addr = spapr_dimm_get_address(PC_DIMM(dev));
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +    sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, addr);
> +
>      if (--ds->nr_lmbs) {
>          return;
>      }
>  
> -    g_free(ds);
> +    spapr_pending_dimm_unplugs_remove(spapr, ds);
>  
>      /*
>       * Now that all the LMBs have been removed by the guest, call the
> @@ -2626,17 +2639,20 @@ static void spapr_del_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>      sPAPRDRConnectorClass *drck;
>      uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
>      int i;
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      sPAPRDIMMState *ds = g_malloc0(sizeof(sPAPRDIMMState));
>      uint64_t addr = addr_start;
>  
>      ds->nr_lmbs = nr_lmbs;
> +    ds->addr = addr_start;
> +    spapr_pending_dimm_unplugs_add(spapr, ds);
>      for (i = 0; i < nr_lmbs; i++) {
>          drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
>                  addr / SPAPR_MEMORY_BLOCK_SIZE);
>          g_assert(drc);
>  
>          drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -        drck->detach(drc, dev, spapr_lmb_release, ds, errp);
> +        drck->detach(drc, dev, spapr_lmb_release, NULL, errp);
>          addr += SPAPR_MEMORY_BLOCK_SIZE;
>      }
>  
> @@ -3515,3 +3531,29 @@ static void spapr_machine_register_types(void)
>  }
>  
>  type_init(spapr_machine_register_types)
> +
> +sPAPRDIMMState *spapr_pending_dimm_unplugs_find(sPAPRMachineState *spapr,
> +                                                uint64_t addr)
> +{
> +    sPAPRDIMMState *dimm_state = NULL;
> +    QTAILQ_FOREACH(dimm_state, &spapr->pending_dimm_unplugs, next) {
> +        if (dimm_state->addr == addr) {
> +            break;
> +        }
> +    }
> +    return dimm_state;
> +}
> +
> +void spapr_pending_dimm_unplugs_add(sPAPRMachineState *spapr,
> +                                   sPAPRDIMMState *dimm_state)
> +{
> +    g_assert(!spapr_pending_dimm_unplugs_find(spapr, dimm_state->addr));
> +    QTAILQ_INSERT_HEAD(&spapr->pending_dimm_unplugs, dimm_state, next);
> +}
> +
> +void spapr_pending_dimm_unplugs_remove(sPAPRMachineState *spapr,
> +                                      sPAPRDIMMState *dimm_state)
> +{
> +    QTAILQ_REMOVE(&spapr->pending_dimm_unplugs, dimm_state, next);
> +    g_free(dimm_state);
> +}
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 5802f88..3e2b5ab 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -32,6 +32,7 @@ struct sPAPRRTCState {
>      int64_t ns_offset;
>  };
>  
> +typedef struct sPAPRDIMMState sPAPRDIMMState;
>  typedef struct sPAPRMachineClass sPAPRMachineClass;
>  
>  #define TYPE_SPAPR_MACHINE      "spapr-machine"
> @@ -104,6 +105,9 @@ struct sPAPRMachineState {
>      /* RTAS state */
>      QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list;
>  
> +    /* pending DIMM unplug queue */
> +    QTAILQ_HEAD(, sPAPRDIMMState) pending_dimm_unplugs;
> +
>      /*< public >*/
>      char *kvm_type;
>      MemoryHotplugState hotplug_memory;
> @@ -646,6 +650,19 @@ struct sPAPRConfigureConnectorState {
>  
>  void spapr_ccs_reset_hook(void *opaque);
>  
> +struct sPAPRDIMMState {
> +    uint64_t addr;
> +    uint32_t nr_lmbs;
> +    QTAILQ_ENTRY(sPAPRDIMMState) next;
> +};
> +
> +sPAPRDIMMState *spapr_pending_dimm_unplugs_find(sPAPRMachineState *spapr,
> +                                               uint64_t addr);
> +void spapr_pending_dimm_unplugs_add(sPAPRMachineState *spapr,
> +                                   sPAPRDIMMState *dimm_state);
> +void spapr_pending_dimm_unplugs_remove(sPAPRMachineState *spapr,
> +                                      sPAPRDIMMState *dimm_state);
> +

AFAICT all these new functions are only used in spapr.c, even in the
rest of the series.  So they should be static, and not in the header
file.  Likewise the structure definition.

>  void spapr_rtc_read(sPAPRRTCState *rtc, struct tm *tm, uint32_t *ns);
>  int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t legacy_offset);
>  

-- 
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: 819 bytes --]

Re: [Qemu-devel] [PATCH v9 2/6] hw/ppc: removing drc->detach_cb and drc->detach_cb_opaque

[David Gibson]

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

On Fri, May 05, 2017 at 05:47:42PM -0300, Daniel Henrique Barboza wrote:
> The pointer drc->detach_cb is being used as a way of informing
> the detach() function inside spapr_drc.c which cb to execute. This
> information can also be retrieved simply by checking drc->type and
> choosing the right callback based on it. In this context, detach_cb
> is redundant information that must be managed.
> 
> After the previous spapr_lmb_release change, no detach_cb_opaques
> are being used by any of the three callbacks functions. This is
> yet another information that is now unused and, on top of that, can't
> be migrated either.
> 
> This patch makes the following changes:
> 
> - removal of detach_cb_opaque. the 'opaque' argument was removed from
> the callbacks and from the detach() function of sPAPRConnectorClass. The
> attribute detach_cb_opaque of sPAPRConnector was removed.
> 
> - removal of detach_cb from the detach() call. The function pointer
> detach_cb of sPAPRConnector was removed. detach() now uses a
> switch(drc->type) to execute the apropriate callback. To achieve this,
> spapr_core_release, spapr_lmb_release and spapr_phb_remove_pci_device_cb
> callbacks were made public to be visible inside detach().
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c              | 10 ++++++----
>  hw/ppc/spapr_drc.c          | 36 ++++++++++++++++++++----------------
>  hw/ppc/spapr_pci.c          |  5 +++--
>  include/hw/pci-host/spapr.h |  3 +++
>  include/hw/ppc/spapr.h      |  4 ++++
>  include/hw/ppc/spapr_drc.h  |  8 +-------
>  6 files changed, 37 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 346c827..e190eb9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2610,7 +2610,8 @@ static uint64_t spapr_dimm_get_address(PCDIMMDevice *dimm)
>      return addr;
>  }
>  
> -static void spapr_lmb_release(DeviceState *dev, void *opaque)
> +/* Callback to be called during DRC release. */
> +void spapr_lmb_release(DeviceState *dev)
>  {
>      HotplugHandler *hotplug_ctrl;
>  
> @@ -2652,7 +2653,7 @@ static void spapr_del_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>          g_assert(drc);
>  
>          drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -        drck->detach(drc, dev, spapr_lmb_release, NULL, errp);
> +        drck->detach(drc, dev, errp);
>          addr += SPAPR_MEMORY_BLOCK_SIZE;
>      }
>  
> @@ -2728,7 +2729,8 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      object_unparent(OBJECT(dev));
>  }
>  
> -static void spapr_core_release(DeviceState *dev, void *opaque)
> +/* Callback to be called during DRC release. */
> +void spapr_core_release(DeviceState *dev)
>  {
>      HotplugHandler *hotplug_ctrl;
>  
> @@ -2761,7 +2763,7 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
>      g_assert(drc);
>  
>      drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -    drck->detach(drc, dev, spapr_core_release, NULL, &local_err);
> +    drck->detach(drc, dev, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index a1cdc87..1c72160 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -20,6 +20,7 @@
>  #include "qapi/visitor.h"
>  #include "qemu/error-report.h"
>  #include "hw/ppc/spapr.h" /* for RTAS return codes */
> +#include "hw/pci-host/spapr.h" /* spapr_phb_remove_pci_device_cb callback */
>  #include "trace.h"
>  
>  #define DRC_CONTAINER_PATH "/dr-connector"
> @@ -99,8 +100,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
>          if (drc->awaiting_release) {
>              if (drc->configured) {
>                  trace_spapr_drc_set_isolation_state_finalizing(get_index(drc));
> -                drck->detach(drc, DEVICE(drc->dev), drc->detach_cb,
> -                             drc->detach_cb_opaque, NULL);
> +                drck->detach(drc, DEVICE(drc->dev), NULL);
>              } else {
>                  trace_spapr_drc_set_isolation_state_deferring(get_index(drc));
>              }
> @@ -153,8 +153,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
>          if (drc->awaiting_release &&
>              drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
>              trace_spapr_drc_set_allocation_state_finalizing(get_index(drc));
> -            drck->detach(drc, DEVICE(drc->dev), drc->detach_cb,
> -                         drc->detach_cb_opaque, NULL);
> +            drck->detach(drc, DEVICE(drc->dev), NULL);
>          } else if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) {
>              drc->awaiting_allocation = false;
>          }
> @@ -404,15 +403,10 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>                               NULL, 0, NULL);
>  }
>  
> -static void detach(sPAPRDRConnector *drc, DeviceState *d,
> -                   spapr_drc_detach_cb *detach_cb,
> -                   void *detach_cb_opaque, Error **errp)
> +static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
>  {
>      trace_spapr_drc_detach(get_index(drc));
>  
> -    drc->detach_cb = detach_cb;
> -    drc->detach_cb_opaque = detach_cb_opaque;
> -
>      /* if we've signalled device presence to the guest, or if the guest
>       * has gone ahead and configured the device (via manually-executed
>       * device add via drmgr in guest, namely), we need to wait
> @@ -456,8 +450,21 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d,
>  
>      drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
>  
> -    if (drc->detach_cb) {
> -        drc->detach_cb(drc->dev, drc->detach_cb_opaque);
> +    /* Calling release callbacks based on drc->type. */
> +    switch (drc->type) {
> +    case SPAPR_DR_CONNECTOR_TYPE_CPU:
> +        spapr_core_release(drc->dev);
> +        break;
> +    case SPAPR_DR_CONNECTOR_TYPE_PHB:
> +    case SPAPR_DR_CONNECTOR_TYPE_VIO:
> +    case SPAPR_DR_CONNECTOR_TYPE_PCI:

This isn't correct.  PHB and VIO DRCs haven't been implemented yet,
and when they are, I don't believe the PCI callback will be suitable.
PHB and VIO should go to the default case for now.

> +        spapr_phb_remove_pci_device_cb(drc->dev);
> +        break;
> +    case SPAPR_DR_CONNECTOR_TYPE_LMB:
> +        spapr_lmb_release(drc->dev);
> +        break;
> +    default:
> +        ;

.. and the default case should probably assert().  If we get here it
means something else has built a bad DRC.

>      }
>  
>      drc->awaiting_release = false;
> @@ -467,8 +474,6 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d,
>      drc->fdt_start_offset = 0;
>      object_property_del(OBJECT(drc), "device", NULL);
>      drc->dev = NULL;
> -    drc->detach_cb = NULL;
> -    drc->detach_cb_opaque = NULL;
>  }
>  
>  static bool release_pending(sPAPRDRConnector *drc)
> @@ -498,8 +503,7 @@ static void reset(DeviceState *d)
>           * force removal if we are
>           */
>          if (drc->awaiting_release) {
> -            drck->detach(drc, DEVICE(drc->dev), drc->detach_cb,
> -                         drc->detach_cb_opaque, NULL);
> +            drck->detach(drc, DEVICE(drc->dev), NULL);
>          }
>  
>          /* non-PCI devices may be awaiting a transition to UNUSABLE */
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index e7567e2..5775db8 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1369,7 +1369,8 @@ out:
>      }
>  }
>  
> -static void spapr_phb_remove_pci_device_cb(DeviceState *dev, void *opaque)
> +/* Callback to be called during DRC release. */
> +void spapr_phb_remove_pci_device_cb(DeviceState *dev)
>  {
>      /* some version guests do not wait for completion of a device
>       * cleanup (generally done asynchronously by the kernel) before
> @@ -1392,7 +1393,7 @@ static void spapr_phb_remove_pci_device(sPAPRDRConnector *drc,
>  {
>      sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>  
> -    drck->detach(drc, DEVICE(pdev), spapr_phb_remove_pci_device_cb, phb, errp);
> +    drck->detach(drc, DEVICE(pdev), errp);
>  }
>  
>  static sPAPRDRConnector *spapr_phb_get_pci_func_drc(sPAPRPHBState *phb,
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 1c2e970..38470b2 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -123,6 +123,9 @@ sPAPRPHBState *spapr_pci_find_phb(sPAPRMachineState *spapr, uint64_t buid);
>  PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid,
>                                uint32_t config_addr);
>  
> +/* PCI release callback. */
> +void spapr_phb_remove_pci_device_cb(DeviceState *dev);
> +
>  /* VFIO EEH hooks */
>  #ifdef CONFIG_LINUX
>  bool spapr_phb_eeh_available(sPAPRPHBState *sphb);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 3e2b5ab..adc9fdb 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -640,6 +640,10 @@ void spapr_hotplug_req_remove_by_count_indexed(sPAPRDRConnectorType drc_type,
>  void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
>                                      sPAPRMachineState *spapr);
>  
> +/* CPU and LMB DRC release callbacks. */
> +void spapr_core_release(DeviceState *dev);
> +void spapr_lmb_release(DeviceState *dev);
> +
>  /* rtas-configure-connector state */
>  struct sPAPRConfigureConnectorState {
>      uint32_t drc_index;
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 5524247..813b9ff 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -130,8 +130,6 @@ typedef enum {
>      SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE = -9003,
>  } sPAPRDRCCResponse;
>  
> -typedef void (spapr_drc_detach_cb)(DeviceState *d, void *opaque);
> -
>  typedef struct sPAPRDRConnector {
>      /*< private >*/
>      DeviceState parent;
> @@ -158,8 +156,6 @@ typedef struct sPAPRDRConnector {
>  
>      /* device pointer, via link property */
>      DeviceState *dev;
> -    spapr_drc_detach_cb *detach_cb;
> -    void *detach_cb_opaque;
>  } sPAPRDRConnector;
>  
>  typedef struct sPAPRDRConnectorClass {
> @@ -188,9 +184,7 @@ typedef struct sPAPRDRConnectorClass {
>      /* QEMU interfaces for managing hotplug operations */
>      void (*attach)(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>                     int fdt_start_offset, bool coldplug, Error **errp);
> -    void (*detach)(sPAPRDRConnector *drc, DeviceState *d,
> -                   spapr_drc_detach_cb *detach_cb,
> -                   void *detach_cb_opaque, Error **errp);
> +    void (*detach)(sPAPRDRConnector *drc, DeviceState *d, Error **errp);
>      bool (*release_pending)(sPAPRDRConnector *drc);
>      void (*set_signalled)(sPAPRDRConnector *drc);
>  } sPAPRDRConnectorClass;

-- 
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: 819 bytes --]

Re: [Qemu-devel] [PATCH v9 3/6] hw/ppc: migrating the DRC state of hotplugged devices

[David Gibson]

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

On Fri, May 05, 2017 at 05:47:43PM -0300, Daniel Henrique Barboza wrote:
> In pseries, a firmware abstraction called Dynamic Reconfiguration
> Connector (DRC) is used to assign a particular dynamic resource
> to the guest and provide an interface to manage configuration/removal
> of the resource associated with it. In other words, DRC is the
> 'plugged state' of a device.
> 
> Before this patch, DRC wasn't being migrated. This causes
> post-migration problems due to DRC state mismatch between source and
> target. The DRC state of a device X in the source might
> change, while in the target the DRC state of X is still fresh. When
> migrating the guest, X will not have the same hotplugged state as it
> did in the source. This means that we can't hot unplug X in the
> target after migration is completed because its DRC state is not consistent.
> https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1677552 is one
> bug that is caused by this DRC state mismatch between source and
> target.
> 
> To migrate the DRC state, we defined the VMStateDescription struct for
> spapr_drc to enable the transmission of spapr_drc state in migration.
> Not all the elements in the DRC state are migrated - only those
> that can be modified by guest actions or device add/remove
> operations:
> 
> - 'isolation_state', 'allocation_state' and 'indicator_state'
> are involved in the DR state transition diagram from
> PAPR+ 2.7, 13.4;
> 
> - 'configured', 'signalled', 'awaiting_release' and 'awaiting_allocation'
> are needed in attaching and detaching devices;
> 
> - 'indicator_state' provides users with hardware state information.
> 
> These are the DRC elements that are migrated.
> 
> In this patch the DRC state is migrated for PCI, LMB and CPU
> connector types. At this moment there is no support to migrate
> DRC for the PHB (PCI Host Bridge) type.
> 
> In the 'realize' function the DRC is registered using vmstate_register,
> similar to what hw/ppc/spapr_iommu.c does in 'spapr_tce_table_realize'.
> This approach works because  DRCs are bus-less and do not sit
> on a BusClass that implements bc->get_dev_path, so as a fallback the
> VMSD gets identified via "spapr_drc"/get_index(drc).
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_drc.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 1c72160..926b945 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -519,6 +519,65 @@ static void reset(DeviceState *d)
>      }
>  }
>  
> +static bool spapr_drc_needed(void *opaque)
> +{
> +    sPAPRDRConnector *drc = (sPAPRDRConnector *)opaque;
> +    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +    bool rc = false;
> +    sPAPRDREntitySense value;

Blank line after the declarations, please.

> +    drck->entity_sense(drc, &value);
> +    /* If no dev is plugged in there is no need to migrate the DRC state */
> +    if (value != SPAPR_DR_ENTITY_SENSE_PRESENT) {
> +        return false;
> +    }
> +
> +    /*
> +     * If there is dev plugged in, we need to migrate the DRC state when
> +     * it is different from cold-plugged state
> +     */
> +    switch (drc->type) {
> +

No blank line here please.

> +    case SPAPR_DR_CONNECTOR_TYPE_PCI:
> +        rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) &&
> +               (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) &&
> +               drc->configured && drc->signalled && !drc->awaiting_release);

You don't do any more manipulation of the rc value, so you might as
well just 'return' directly here.


> +        break;
> +
> +    case SPAPR_DR_CONNECTOR_TYPE_LMB:
> +        rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
> +               (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) &&
> +               drc->configured && drc->signalled && !drc->awaiting_release);
> +        break;
> +
> +    case SPAPR_DR_CONNECTOR_TYPE_CPU:
> +        rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
> +               (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) &&
> +                drc->configured && drc->signalled && !drc->awaiting_release);
> +        break;
> +
> +    default:
> +        ;

This should probably assert().

> +    }
> +    return rc;
> +}
> +
> +static const VMStateDescription vmstate_spapr_drc = {
> +    .name = "spapr_drc",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = spapr_drc_needed,
> +    .fields  = (VMStateField []) {
> +        VMSTATE_UINT32(isolation_state, sPAPRDRConnector),
> +        VMSTATE_UINT32(allocation_state, sPAPRDRConnector),
> +        VMSTATE_UINT32(indicator_state, sPAPRDRConnector),
> +        VMSTATE_BOOL(configured, sPAPRDRConnector),
> +        VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
> +        VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
> +        VMSTATE_BOOL(signalled, sPAPRDRConnector),

Hrm.  I'm happy translation the 3 state integers, since their meaning
is in the PAPR spec.  The others are qemu local information, so it's
not as clear that they'll have a stable meaning.  Are you absolutely
sure you need all of them?

> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static void realize(DeviceState *d, Error **errp)
>  {
>      sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
> @@ -547,6 +606,8 @@ static void realize(DeviceState *d, Error **errp)
>          object_unref(OBJECT(drc));
>      }
>      g_free(child_name);
> +    vmstate_register(DEVICE(drc), drck->get_index(drc), &vmstate_spapr_drc,
> +                     drc);
>      trace_spapr_drc_realize_complete(drck->get_index(drc));
>  }
>  

-- 
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: 819 bytes --]

Re: [Qemu-devel] [PATCH v9 4/6] hw/ppc/spapr.c: migrate pending_dimm_unplugs of spapr state

[David Gibson]

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

On Fri, May 05, 2017 at 05:47:44PM -0300, Daniel Henrique Barboza wrote:
> To allow for a DIMM unplug event to resume its work if a migration
> occurs in the middle of it, this patch migrates the non-empty
> pending_dimm_unplugs QTAILQ that stores the DIMM information
> that the spapr_lmb_release() callback uses.
> 
> It was considered an apprach where the DIMM states would be restored
> on the post-_load after a migration. The problem is that there is
> no way of knowing, from the sPAPRMachineState, if a given DIMM is going
> through an unplug process and the callback needs the updated DIMM State.
> 
> We could migrate a flag indicating that there is an unplug event going
> on for a certain DIMM, fetching this information from the start of the
> spapr_del_lmbs call. But this would also require a scan on post_load to
> figure out how many nr_lmbs are left. At this point we can just
> migrate the nr_lmbs information as well, given that it is being calculated
> at spapr_del_lmbs already, and spare a scanning/discovery in the
> post-load. All that we need is inside the sPAPRDIMMState structure
> that is added to the pending_dimm_unplugs queue at the start of the
> spapr_del_lmbs, so it's convenient to just migrated this queue it if it's
> not empty.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>

NACK.

As I believe I suggested previously, you can reconstruct this state on
the receiving side by doing a full scan of the DIMM and LMB DRC states.

> ---
>  hw/ppc/spapr.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e190eb9..30f0b7b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1437,6 +1437,36 @@ static bool version_before_3(void *opaque, int version_id)
>      return version_id < 3;
>  }
>  
> +static bool spapr_pending_dimm_unplugs_needed(void *opaque)
> +{
> +    sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> +    return !QTAILQ_EMPTY(&spapr->pending_dimm_unplugs);
> +}
> +
> +static const VMStateDescription vmstate_spapr_dimmstate = {
> +    .name = "spapr_dimm_state",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(addr, sPAPRDIMMState),
> +        VMSTATE_UINT32(nr_lmbs, sPAPRDIMMState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static const VMStateDescription vmstate_spapr_pending_dimm_unplugs = {
> +    .name = "spapr_pending_dimm_unplugs",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = spapr_pending_dimm_unplugs_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_QTAILQ_V(pending_dimm_unplugs, sPAPRMachineState, 1,
> +                         vmstate_spapr_dimmstate, sPAPRDIMMState,
> +                         next),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>  static bool spapr_ov5_cas_needed(void *opaque)
>  {
>      sPAPRMachineState *spapr = opaque;
> @@ -1535,6 +1565,7 @@ static const VMStateDescription vmstate_spapr = {
>      .subsections = (const VMStateDescription*[]) {
>          &vmstate_spapr_ov5_cas,
>          &vmstate_spapr_patb_entry,
> +        &vmstate_spapr_pending_dimm_unplugs,
>          NULL
>      }
>  };

-- 
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: 819 bytes --]

Re: [Qemu-devel] [PATCH v9 6/6] migration: spapr: migrate pending_events of spapr state

[David Gibson]

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

On Fri, May 05, 2017 at 05:47:46PM -0300, Daniel Henrique Barboza wrote:
> From: Jianjun Duan <duanj@linux.vnet.ibm.com>
> 
> In racing situations between hotplug events and migration operation,
> a rtas hotplug event could have not yet be delivered to the source
> guest when migration is started. In this case the pending_events of
> spapr state need be transmitted to the target so that the hotplug
> event can be finished on the target.
> 
> All the different fields of the events are encoded as defined by
> PAPR. We can migrate them as uint8_t binary stream without any
> concerns about data padding or endianess.
> 
> pending_events is put in a subsection in the spapr state VMSD to make
> sure migration across different versions is not broken.
> 
> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>

This seems like it's probably a good idea, even independent of the
hotplug migration stuff.  I suspect there are other races where we
could lose a shutdown event or similar if there's a migration.

> ---
>  hw/ppc/spapr.c         | 33 +++++++++++++++++++++++++++++++++
>  hw/ppc/spapr_events.c  | 24 +++++++++++++-----------
>  include/hw/ppc/spapr.h |  3 ++-
>  3 files changed, 48 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bc56249..e924fd4 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1498,6 +1498,38 @@ static const VMStateDescription vmstate_spapr_ccs_list = {
>      },
>  };
>  
> +static bool spapr_pending_events_needed(void *opaque)
> +{
> +    sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> +    return !QTAILQ_EMPTY(&spapr->pending_events);
> +}
> +
> +static const VMStateDescription vmstate_spapr_event_entry = {
> +    .name = "spapr_event_log_entry",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_INT32(log_type, sPAPREventLogEntry),

This requires changing the actual type to int32_t in the structure.

> +        VMSTATE_BOOL(exception, sPAPREventLogEntry),

So, at the moment, AFAICT every event is marked as exception == true,
so this doesn't actually tell us anything.   If that becomes not the
case in future, can the exception flag be derived from the log_type or
information in the even buffer?

> +        VMSTATE_UINT32(data_size, sPAPREventLogEntry),
> +        VMSTATE_VARRAY_UINT32_ALLOC(data, sPAPREventLogEntry, data_size,
> +                                    0, vmstate_info_uint8, uint8_t),

So, data_size duplicates information that's in the event header, which
is a bit sad.  I suppose I'm ok with that, since setting up the VARRAY
thing is going to be pretty awkward otherwise.

> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static const VMStateDescription vmstate_spapr_pending_events = {
> +    .name = "spapr_pending_events",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = spapr_pending_events_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_QTAILQ_V(pending_events, sPAPRMachineState, 1,
> +                         vmstate_spapr_event_entry, sPAPREventLogEntry, next),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>  static bool spapr_ov5_cas_needed(void *opaque)
>  {
>      sPAPRMachineState *spapr = opaque;
> @@ -1598,6 +1630,7 @@ static const VMStateDescription vmstate_spapr = {
>          &vmstate_spapr_patb_entry,
>          &vmstate_spapr_pending_dimm_unplugs,
>          &vmstate_spapr_ccs_list,
> +        &vmstate_spapr_pending_events,
>          NULL
>      }
>  };
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index f0b28d8..70c7cfc 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -342,7 +342,8 @@ static int rtas_event_log_to_irq(sPAPRMachineState *spapr, int log_type)
>      return source->irq;
>  }
>  
> -static void rtas_event_log_queue(int log_type, void *data, bool exception)
> +static void rtas_event_log_queue(int log_type, void *data, bool exception,
> +                                 int data_size)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      sPAPREventLogEntry *entry = g_new(sPAPREventLogEntry, 1);
> @@ -351,6 +352,7 @@ static void rtas_event_log_queue(int log_type, void *data, bool exception)
>      entry->log_type = log_type;
>      entry->exception = exception;
>      entry->data = data;
> +    entry->data_size = data_size;

I think it would make more sense to derive data_size from the buffer
header contents here, rather than in all the callers.

>      QTAILQ_INSERT_TAIL(&spapr->pending_events, entry, next);
>  }
>  
> @@ -445,6 +447,7 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
>      struct rtas_event_log_v6_mainb *mainb;
>      struct rtas_event_log_v6_epow *epow;
>      struct epow_log_full *new_epow;
> +    uint32_t data_size;
>  
>      new_epow = g_malloc0(sizeof(*new_epow));
>      hdr = &new_epow->hdr;
> @@ -453,14 +456,13 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
>      mainb = &new_epow->mainb;
>      epow = &new_epow->epow;
>  
> +    data_size = sizeof(*new_epow);
>      hdr->summary = cpu_to_be32(RTAS_LOG_VERSION_6
>                                 | RTAS_LOG_SEVERITY_EVENT
>                                 | RTAS_LOG_DISPOSITION_NOT_RECOVERED
>                                 | RTAS_LOG_OPTIONAL_PART_PRESENT
>                                 | RTAS_LOG_TYPE_EPOW);
> -    hdr->extended_length = cpu_to_be32(sizeof(*new_epow)
> -                                       - sizeof(new_epow->hdr));
> -
> +    hdr->extended_length = cpu_to_be32(data_size - sizeof(new_epow->hdr));
>      spapr_init_v6hdr(v6hdr);
>      spapr_init_maina(maina, 3 /* Main-A, Main-B and EPOW */);
>  
> @@ -479,7 +481,7 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
>      epow->event_modifier = RTAS_LOG_V6_EPOW_MODIFIER_NORMAL;
>      epow->extended_modifier = RTAS_LOG_V6_EPOW_XMODIFIER_PARTITION_SPECIFIC;
>  
> -    rtas_event_log_queue(RTAS_LOG_TYPE_EPOW, new_epow, true);
> +    rtas_event_log_queue(RTAS_LOG_TYPE_EPOW, new_epow, true, data_size);
>  
>      qemu_irq_pulse(xics_get_qirq(XICS_FABRIC(spapr),
>                                   rtas_event_log_to_irq(spapr,
> @@ -504,6 +506,7 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
>      struct rtas_event_log_v6_maina *maina;
>      struct rtas_event_log_v6_mainb *mainb;
>      struct rtas_event_log_v6_hp *hp;
> +    uint32_t data_size;
>  
>      new_hp = g_malloc0(sizeof(struct hp_log_full));
>      hdr = &new_hp->hdr;
> @@ -512,14 +515,14 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
>      mainb = &new_hp->mainb;
>      hp = &new_hp->hp;
>  
> +    data_size = sizeof(*new_hp);
>      hdr->summary = cpu_to_be32(RTAS_LOG_VERSION_6
>                                 | RTAS_LOG_SEVERITY_EVENT
>                                 | RTAS_LOG_DISPOSITION_NOT_RECOVERED
>                                 | RTAS_LOG_OPTIONAL_PART_PRESENT
>                                 | RTAS_LOG_INITIATOR_HOTPLUG
>                                 | RTAS_LOG_TYPE_HOTPLUG);
> -    hdr->extended_length = cpu_to_be32(sizeof(*new_hp)
> -                                       - sizeof(new_hp->hdr));
> +    hdr->extended_length = cpu_to_be32(data_size - sizeof(new_hp->hdr));
>  
>      spapr_init_v6hdr(v6hdr);
>      spapr_init_maina(maina, 3 /* Main-A, Main-B, HP */);
> @@ -572,7 +575,7 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
>              cpu_to_be32(drc_id->count_indexed.index);
>      }
>  
> -    rtas_event_log_queue(RTAS_LOG_TYPE_HOTPLUG, new_hp, true);
> +    rtas_event_log_queue(RTAS_LOG_TYPE_HOTPLUG, new_hp, true, data_size);
>  
>      qemu_irq_pulse(xics_get_qirq(XICS_FABRIC(spapr),
>                                   rtas_event_log_to_irq(spapr,
> @@ -671,8 +674,7 @@ static void check_exception(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      if (!event) {
>          goto out_no_events;
>      }
> -
> -    hdr = event->data;
> +    hdr = (struct rtas_error_log *)event->data;
>      event_len = be32_to_cpu(hdr->extended_length) + sizeof(*hdr);
>  
>      if (event_len < len) {
> @@ -728,7 +730,7 @@ static void event_scan(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>          goto out_no_events;
>      }
>  
> -    hdr = event->data;
> +    hdr = (struct rtas_error_log *)event->data;
>      event_len = be32_to_cpu(hdr->extended_length) + sizeof(*hdr);
>  
>      if (event_len < len) {
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index adc9fdb..ddac014 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -603,7 +603,8 @@ sPAPRTCETable *spapr_tce_find_by_liobn(target_ulong liobn);
>  struct sPAPREventLogEntry {
>      int log_type;
>      bool exception;
> -    void *data;
> +    uint8_t *data;

I think you can avoid this type change (and the casts it requires) by
using VBUFFER instead of VARRAY.

> +    uint32_t data_size;
>      QTAILQ_ENTRY(sPAPREventLogEntry) next;
>  };
>  

-- 
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: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH v9 4/6] hw/ppc/spapr.c: migrate pending_dimm_unplugs of spapr state

[Daniel Henrique Barboza]

On 05/12/2017 03:12 AM, David Gibson wrote:
> On Fri, May 05, 2017 at 05:47:44PM -0300, Daniel Henrique Barboza wrote:
>> To allow for a DIMM unplug event to resume its work if a migration
>> occurs in the middle of it, this patch migrates the non-empty
>> pending_dimm_unplugs QTAILQ that stores the DIMM information
>> that the spapr_lmb_release() callback uses.
>>
>> It was considered an apprach where the DIMM states would be restored
>> on the post-_load after a migration. The problem is that there is
>> no way of knowing, from the sPAPRMachineState, if a given DIMM is going
>> through an unplug process and the callback needs the updated DIMM State.
>>
>> We could migrate a flag indicating that there is an unplug event going
>> on for a certain DIMM, fetching this information from the start of the
>> spapr_del_lmbs call. But this would also require a scan on post_load to
>> figure out how many nr_lmbs are left. At this point we can just
>> migrate the nr_lmbs information as well, given that it is being calculated
>> at spapr_del_lmbs already, and spare a scanning/discovery in the
>> post-load. All that we need is inside the sPAPRDIMMState structure
>> that is added to the pending_dimm_unplugs queue at the start of the
>> spapr_del_lmbs, so it's convenient to just migrated this queue it if it's
>> not empty.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> NACK.
>
> As I believe I suggested previously, you can reconstruct this state on
> the receiving side by doing a full scan of the DIMM and LMB DRC states.

Just had an idea that I think it's in the line of what you're 
suggesting. Given
that the information we need is only created in the spapr_del_lmbs
(as per patch 1), we can use the absence of this information in the
release callback as a sort of a flag, an indication that a migration got
in the way and we need to reconstruct the nr_lmbs states again, using
the same scanning function I've used in v8.

The flow would be like this (considering the changes in the
previous 3 patches so far):

------------

/* Callback to be called during DRC release. */
void spapr_lmb_release(DeviceState *dev)
{
      HotplugHandler *hotplug_ctrl;

      uint64_t addr = spapr_dimm_get_address(PC_DIMM(dev));
      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
      sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, addr);

     // no DIMM state found in spapr - re-create it to find out how may 
LMBs are left
     if (ds == NULL) {
         uint32 nr_lmbs  = ***call_scanning_LMB_DRCs_function(dev)***
         // recreate the sPAPRDIMMState element and add it back to spapr
     }

     ( resume callback as usual )

-----------

Is this approach be adequate? Another alternative would be to use another
way of detecting if an LMB unplug is happening and, if positive, do the same
process in the post_load(). In this case I'll need to take a look in the 
code and
see how we can detect an ongoing unplug besides what I've said above.


Thanks,


Daniel

>
>> ---
>>   hw/ppc/spapr.c | 31 +++++++++++++++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index e190eb9..30f0b7b 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1437,6 +1437,36 @@ static bool version_before_3(void *opaque, int version_id)
>>       return version_id < 3;
>>   }
>>   
>> +static bool spapr_pending_dimm_unplugs_needed(void *opaque)
>> +{
>> +    sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
>> +    return !QTAILQ_EMPTY(&spapr->pending_dimm_unplugs);
>> +}
>> +
>> +static const VMStateDescription vmstate_spapr_dimmstate = {
>> +    .name = "spapr_dimm_state",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT64(addr, sPAPRDIMMState),
>> +        VMSTATE_UINT32(nr_lmbs, sPAPRDIMMState),
>> +        VMSTATE_END_OF_LIST()
>> +    },
>> +};
>> +
>> +static const VMStateDescription vmstate_spapr_pending_dimm_unplugs = {
>> +    .name = "spapr_pending_dimm_unplugs",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .needed = spapr_pending_dimm_unplugs_needed,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_QTAILQ_V(pending_dimm_unplugs, sPAPRMachineState, 1,
>> +                         vmstate_spapr_dimmstate, sPAPRDIMMState,
>> +                         next),
>> +        VMSTATE_END_OF_LIST()
>> +    },
>> +};
>> +
>>   static bool spapr_ov5_cas_needed(void *opaque)
>>   {
>>       sPAPRMachineState *spapr = opaque;
>> @@ -1535,6 +1565,7 @@ static const VMStateDescription vmstate_spapr = {
>>       .subsections = (const VMStateDescription*[]) {
>>           &vmstate_spapr_ov5_cas,
>>           &vmstate_spapr_patb_entry,
>> +        &vmstate_spapr_pending_dimm_unplugs,
>>           NULL
>>       }
>>   };

Re: [Qemu-devel] [Qemu-ppc] [PATCH v9 6/6] migration: spapr: migrate pending_events of spapr state

[Daniel Henrique Barboza]

On 05/12/2017 03:28 AM, David Gibson wrote:
> On Fri, May 05, 2017 at 05:47:46PM -0300, Daniel Henrique Barboza wrote:
>> From: Jianjun Duan <duanj@linux.vnet.ibm.com>
>>
>> In racing situations between hotplug events and migration operation,
>> a rtas hotplug event could have not yet be delivered to the source
>> guest when migration is started. In this case the pending_events of
>> spapr state need be transmitted to the target so that the hotplug
>> event can be finished on the target.
>>
>> All the different fields of the events are encoded as defined by
>> PAPR. We can migrate them as uint8_t binary stream without any
>> concerns about data padding or endianess.
>>
>> pending_events is put in a subsection in the spapr state VMSD to make
>> sure migration across different versions is not broken.
>>
>> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> This seems like it's probably a good idea, even independent of the
> hotplug migration stuff.  I suspect there are other races where we
> could lose a shutdown event or similar if there's a migration.
Perhaps we can detach this patch (and the ccs_list one) from this
series and evaluate them separately?


Daniel

>
>> ---
>>   hw/ppc/spapr.c         | 33 +++++++++++++++++++++++++++++++++
>>   hw/ppc/spapr_events.c  | 24 +++++++++++++-----------
>>   include/hw/ppc/spapr.h |  3 ++-
>>   3 files changed, 48 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index bc56249..e924fd4 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1498,6 +1498,38 @@ static const VMStateDescription vmstate_spapr_ccs_list = {
>>       },
>>   };
>>   
>> +static bool spapr_pending_events_needed(void *opaque)
>> +{
>> +    sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
>> +    return !QTAILQ_EMPTY(&spapr->pending_events);
>> +}
>> +
>> +static const VMStateDescription vmstate_spapr_event_entry = {
>> +    .name = "spapr_event_log_entry",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_INT32(log_type, sPAPREventLogEntry),
> This requires changing the actual type to int32_t in the structure.
>
>> +        VMSTATE_BOOL(exception, sPAPREventLogEntry),
> So, at the moment, AFAICT every event is marked as exception == true,
> so this doesn't actually tell us anything.   If that becomes not the
> case in future, can the exception flag be derived from the log_type or
> information in the even buffer?
>
>> +        VMSTATE_UINT32(data_size, sPAPREventLogEntry),
>> +        VMSTATE_VARRAY_UINT32_ALLOC(data, sPAPREventLogEntry, data_size,
>> +                                    0, vmstate_info_uint8, uint8_t),
> So, data_size duplicates information that's in the event header, which
> is a bit sad.  I suppose I'm ok with that, since setting up the VARRAY
> thing is going to be pretty awkward otherwise.
>
>> +        VMSTATE_END_OF_LIST()
>> +    },
>> +};
>> +
>> +static const VMStateDescription vmstate_spapr_pending_events = {
>> +    .name = "spapr_pending_events",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .needed = spapr_pending_events_needed,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_QTAILQ_V(pending_events, sPAPRMachineState, 1,
>> +                         vmstate_spapr_event_entry, sPAPREventLogEntry, next),
>> +        VMSTATE_END_OF_LIST()
>> +    },
>> +};
>> +
>>   static bool spapr_ov5_cas_needed(void *opaque)
>>   {
>>       sPAPRMachineState *spapr = opaque;
>> @@ -1598,6 +1630,7 @@ static const VMStateDescription vmstate_spapr = {
>>           &vmstate_spapr_patb_entry,
>>           &vmstate_spapr_pending_dimm_unplugs,
>>           &vmstate_spapr_ccs_list,
>> +        &vmstate_spapr_pending_events,
>>           NULL
>>       }
>>   };
>> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
>> index f0b28d8..70c7cfc 100644
>> --- a/hw/ppc/spapr_events.c
>> +++ b/hw/ppc/spapr_events.c
>> @@ -342,7 +342,8 @@ static int rtas_event_log_to_irq(sPAPRMachineState *spapr, int log_type)
>>       return source->irq;
>>   }
>>   
>> -static void rtas_event_log_queue(int log_type, void *data, bool exception)
>> +static void rtas_event_log_queue(int log_type, void *data, bool exception,
>> +                                 int data_size)
>>   {
>>       sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>>       sPAPREventLogEntry *entry = g_new(sPAPREventLogEntry, 1);
>> @@ -351,6 +352,7 @@ static void rtas_event_log_queue(int log_type, void *data, bool exception)
>>       entry->log_type = log_type;
>>       entry->exception = exception;
>>       entry->data = data;
>> +    entry->data_size = data_size;
> I think it would make more sense to derive data_size from the buffer
> header contents here, rather than in all the callers.
>
>>       QTAILQ_INSERT_TAIL(&spapr->pending_events, entry, next);
>>   }
>>   
>> @@ -445,6 +447,7 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
>>       struct rtas_event_log_v6_mainb *mainb;
>>       struct rtas_event_log_v6_epow *epow;
>>       struct epow_log_full *new_epow;
>> +    uint32_t data_size;
>>   
>>       new_epow = g_malloc0(sizeof(*new_epow));
>>       hdr = &new_epow->hdr;
>> @@ -453,14 +456,13 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
>>       mainb = &new_epow->mainb;
>>       epow = &new_epow->epow;
>>   
>> +    data_size = sizeof(*new_epow);
>>       hdr->summary = cpu_to_be32(RTAS_LOG_VERSION_6
>>                                  | RTAS_LOG_SEVERITY_EVENT
>>                                  | RTAS_LOG_DISPOSITION_NOT_RECOVERED
>>                                  | RTAS_LOG_OPTIONAL_PART_PRESENT
>>                                  | RTAS_LOG_TYPE_EPOW);
>> -    hdr->extended_length = cpu_to_be32(sizeof(*new_epow)
>> -                                       - sizeof(new_epow->hdr));
>> -
>> +    hdr->extended_length = cpu_to_be32(data_size - sizeof(new_epow->hdr));
>>       spapr_init_v6hdr(v6hdr);
>>       spapr_init_maina(maina, 3 /* Main-A, Main-B and EPOW */);
>>   
>> @@ -479,7 +481,7 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
>>       epow->event_modifier = RTAS_LOG_V6_EPOW_MODIFIER_NORMAL;
>>       epow->extended_modifier = RTAS_LOG_V6_EPOW_XMODIFIER_PARTITION_SPECIFIC;
>>   
>> -    rtas_event_log_queue(RTAS_LOG_TYPE_EPOW, new_epow, true);
>> +    rtas_event_log_queue(RTAS_LOG_TYPE_EPOW, new_epow, true, data_size);
>>   
>>       qemu_irq_pulse(xics_get_qirq(XICS_FABRIC(spapr),
>>                                    rtas_event_log_to_irq(spapr,
>> @@ -504,6 +506,7 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
>>       struct rtas_event_log_v6_maina *maina;
>>       struct rtas_event_log_v6_mainb *mainb;
>>       struct rtas_event_log_v6_hp *hp;
>> +    uint32_t data_size;
>>   
>>       new_hp = g_malloc0(sizeof(struct hp_log_full));
>>       hdr = &new_hp->hdr;
>> @@ -512,14 +515,14 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
>>       mainb = &new_hp->mainb;
>>       hp = &new_hp->hp;
>>   
>> +    data_size = sizeof(*new_hp);
>>       hdr->summary = cpu_to_be32(RTAS_LOG_VERSION_6
>>                                  | RTAS_LOG_SEVERITY_EVENT
>>                                  | RTAS_LOG_DISPOSITION_NOT_RECOVERED
>>                                  | RTAS_LOG_OPTIONAL_PART_PRESENT
>>                                  | RTAS_LOG_INITIATOR_HOTPLUG
>>                                  | RTAS_LOG_TYPE_HOTPLUG);
>> -    hdr->extended_length = cpu_to_be32(sizeof(*new_hp)
>> -                                       - sizeof(new_hp->hdr));
>> +    hdr->extended_length = cpu_to_be32(data_size - sizeof(new_hp->hdr));
>>   
>>       spapr_init_v6hdr(v6hdr);
>>       spapr_init_maina(maina, 3 /* Main-A, Main-B, HP */);
>> @@ -572,7 +575,7 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
>>               cpu_to_be32(drc_id->count_indexed.index);
>>       }
>>   
>> -    rtas_event_log_queue(RTAS_LOG_TYPE_HOTPLUG, new_hp, true);
>> +    rtas_event_log_queue(RTAS_LOG_TYPE_HOTPLUG, new_hp, true, data_size);
>>   
>>       qemu_irq_pulse(xics_get_qirq(XICS_FABRIC(spapr),
>>                                    rtas_event_log_to_irq(spapr,
>> @@ -671,8 +674,7 @@ static void check_exception(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>>       if (!event) {
>>           goto out_no_events;
>>       }
>> -
>> -    hdr = event->data;
>> +    hdr = (struct rtas_error_log *)event->data;
>>       event_len = be32_to_cpu(hdr->extended_length) + sizeof(*hdr);
>>   
>>       if (event_len < len) {
>> @@ -728,7 +730,7 @@ static void event_scan(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>>           goto out_no_events;
>>       }
>>   
>> -    hdr = event->data;
>> +    hdr = (struct rtas_error_log *)event->data;
>>       event_len = be32_to_cpu(hdr->extended_length) + sizeof(*hdr);
>>   
>>       if (event_len < len) {
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index adc9fdb..ddac014 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -603,7 +603,8 @@ sPAPRTCETable *spapr_tce_find_by_liobn(target_ulong liobn);
>>   struct sPAPREventLogEntry {
>>       int log_type;
>>       bool exception;
>> -    void *data;
>> +    uint8_t *data;
> I think you can avoid this type change (and the casts it requires) by
> using VBUFFER instead of VARRAY.
>
>> +    uint32_t data_size;
>>       QTAILQ_ENTRY(sPAPREventLogEntry) next;
>>   };
>>   

Re: [Qemu-devel] [Qemu-ppc] [PATCH v9 6/6] migration: spapr: migrate pending_events of spapr state

[David Gibson]

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

On Fri, May 12, 2017 at 05:02:44PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 05/12/2017 03:28 AM, David Gibson wrote:
> > On Fri, May 05, 2017 at 05:47:46PM -0300, Daniel Henrique Barboza wrote:
> > > From: Jianjun Duan <duanj@linux.vnet.ibm.com>
> > > 
> > > In racing situations between hotplug events and migration operation,
> > > a rtas hotplug event could have not yet be delivered to the source
> > > guest when migration is started. In this case the pending_events of
> > > spapr state need be transmitted to the target so that the hotplug
> > > event can be finished on the target.
> > > 
> > > All the different fields of the events are encoded as defined by
> > > PAPR. We can migrate them as uint8_t binary stream without any
> > > concerns about data padding or endianess.
> > > 
> > > pending_events is put in a subsection in the spapr state VMSD to make
> > > sure migration across different versions is not broken.
> > > 
> > > Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
> > > Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> > This seems like it's probably a good idea, even independent of the
> > hotplug migration stuff.  I suspect there are other races where we
> > could lose a shutdown event or similar if there's a migration.
> Perhaps we can detach this patch (and the ccs_list one) from this
> series and evaluate them separately?

Fine by me for this patch.  Not so much with the ccs one, because
migrating the ccs stuff really doesn't make sense without properly
migrating the rest of the drc state.

-- 
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: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH v9 4/6] hw/ppc/spapr.c: migrate pending_dimm_unplugs of spapr state

[Michael Roth]

Quoting Daniel Henrique Barboza (2017-05-12 14:54:57)
> 
> 
> On 05/12/2017 03:12 AM, David Gibson wrote:
> > On Fri, May 05, 2017 at 05:47:44PM -0300, Daniel Henrique Barboza wrote:
> >> To allow for a DIMM unplug event to resume its work if a migration
> >> occurs in the middle of it, this patch migrates the non-empty
> >> pending_dimm_unplugs QTAILQ that stores the DIMM information
> >> that the spapr_lmb_release() callback uses.
> >>
> >> It was considered an apprach where the DIMM states would be restored
> >> on the post-_load after a migration. The problem is that there is
> >> no way of knowing, from the sPAPRMachineState, if a given DIMM is going
> >> through an unplug process and the callback needs the updated DIMM State.
> >>
> >> We could migrate a flag indicating that there is an unplug event going
> >> on for a certain DIMM, fetching this information from the start of the
> >> spapr_del_lmbs call. But this would also require a scan on post_load to
> >> figure out how many nr_lmbs are left. At this point we can just
> >> migrate the nr_lmbs information as well, given that it is being calculated
> >> at spapr_del_lmbs already, and spare a scanning/discovery in the
> >> post-load. All that we need is inside the sPAPRDIMMState structure
> >> that is added to the pending_dimm_unplugs queue at the start of the
> >> spapr_del_lmbs, so it's convenient to just migrated this queue it if it's
> >> not empty.
> >>
> >> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> > NACK.
> >
> > As I believe I suggested previously, you can reconstruct this state on
> > the receiving side by doing a full scan of the DIMM and LMB DRC states.
> 
> Just had an idea that I think it's in the line of what you're 
> suggesting. Given
> that the information we need is only created in the spapr_del_lmbs
> (as per patch 1), we can use the absence of this information in the
> release callback as a sort of a flag, an indication that a migration got
> in the way and we need to reconstruct the nr_lmbs states again, using
> the same scanning function I've used in v8.
> 
> The flow would be like this (considering the changes in the
> previous 3 patches so far):
> 
> ------------
> 
> /* Callback to be called during DRC release. */
> void spapr_lmb_release(DeviceState *dev)
> {
>       HotplugHandler *hotplug_ctrl;
> 
>       uint64_t addr = spapr_dimm_get_address(PC_DIMM(dev));
>       sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>       sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, addr);
> 
>      // no DIMM state found in spapr - re-create it to find out how may 
> LMBs are left
>      if (ds == NULL) {
>          uint32 nr_lmbs  = ***call_scanning_LMB_DRCs_function(dev)***
>          // recreate the sPAPRDIMMState element and add it back to spapr
>      }
> 
>      ( resume callback as usual )
> 
> -----------
> 
> Is this approach be adequate? Another alternative would be to use another

Seems reasonable to me.

> way of detecting if an LMB unplug is happening and, if positive, do the same
> process in the post_load(). In this case I'll need to take a look in the 
> code and
> see how we can detect an ongoing unplug besides what I've said above.

You could scan DRCs/LMBs for each DIMM and generate an sPAPRDIMMState for any
case where DRCs/LMBs are not all present. But doing it the way you've
proposed above lets us re-generate them as needed and avoid the exhaustive
scan. I'd be sure to document it in the comments as a migration-specific hook
though since you lose that understanding by moving it out of postload and
future refactorings might forget why we have a hook that regenerates it
even though spapr_del_lmbs always sets it initially.

> 
> 
> Thanks,
> 
> 
> Daniel
> 
> >
> >> ---
> >>   hw/ppc/spapr.c | 31 +++++++++++++++++++++++++++++++
> >>   1 file changed, 31 insertions(+)
> >>
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index e190eb9..30f0b7b 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -1437,6 +1437,36 @@ static bool version_before_3(void *opaque, int version_id)
> >>       return version_id < 3;
> >>   }
> >>   
> >> +static bool spapr_pending_dimm_unplugs_needed(void *opaque)
> >> +{
> >> +    sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> >> +    return !QTAILQ_EMPTY(&spapr->pending_dimm_unplugs);
> >> +}
> >> +
> >> +static const VMStateDescription vmstate_spapr_dimmstate = {
> >> +    .name = "spapr_dimm_state",
> >> +    .version_id = 1,
> >> +    .minimum_version_id = 1,
> >> +    .fields = (VMStateField[]) {
> >> +        VMSTATE_UINT64(addr, sPAPRDIMMState),
> >> +        VMSTATE_UINT32(nr_lmbs, sPAPRDIMMState),
> >> +        VMSTATE_END_OF_LIST()
> >> +    },
> >> +};
> >> +
> >> +static const VMStateDescription vmstate_spapr_pending_dimm_unplugs = {
> >> +    .name = "spapr_pending_dimm_unplugs",
> >> +    .version_id = 1,
> >> +    .minimum_version_id = 1,
> >> +    .needed = spapr_pending_dimm_unplugs_needed,
> >> +    .fields = (VMStateField[]) {
> >> +        VMSTATE_QTAILQ_V(pending_dimm_unplugs, sPAPRMachineState, 1,
> >> +                         vmstate_spapr_dimmstate, sPAPRDIMMState,
> >> +                         next),
> >> +        VMSTATE_END_OF_LIST()
> >> +    },
> >> +};
> >> +
> >>   static bool spapr_ov5_cas_needed(void *opaque)
> >>   {
> >>       sPAPRMachineState *spapr = opaque;
> >> @@ -1535,6 +1565,7 @@ static const VMStateDescription vmstate_spapr = {
> >>       .subsections = (const VMStateDescription*[]) {
> >>           &vmstate_spapr_ov5_cas,
> >>           &vmstate_spapr_patb_entry,
> >> +        &vmstate_spapr_pending_dimm_unplugs,
> >>           NULL
> >>       }
> >>   };
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH v9 1/6] hw/ppc/spapr.c: adding pending_dimm_unplugs to sPAPRMachineState

[Daniel Henrique Barboza]

On 05/12/2017 03:04 AM, David Gibson wrote:
> On Fri, May 05, 2017 at 05:47:41PM -0300, Daniel Henrique Barboza wrote:
>> The LMB DRC release callback, spapr_lmb_release(), uses an opaque
>> parameter, a sPAPRDIMMState struct that stores the current LMBs that
>> are allocated to a DIMM (nr_lmbs). After each call to this callback,
>> the nr_lmbs is decremented by one and, when it reaches zero, the callback
>> proceeds with the qdev calls to hot unplug the LMB.
>>
>> Using drc->detach_cb_opaque is problematic because it can't be migrated in
>> the future DRC migration work. This patch makes the following changes to
>> eliminate the usage of this opaque callback inside spapr_lmb_release:
>>
>> - sPAPRDIMMState was moved from spapr.c and added to spapr.h. A new
>> attribute called 'addr' was added to it. This is used as an unique
>> identifier to associate a sPAPRDIMMState to a PCDIMM element.
>>
>> - sPAPRMachineState now hosts a new QTAILQ called 'pending_dimm_unplugs'.
>> This queue of sPAPRDIMMState elements will store the DIMM state of DIMMs
>> that are currently going under an unplug process.
>>
>> - spapr_lmb_release() will now retrieve the nr_lmbs value by getting the
>> correspondent sPAPRDIMMState. A helper function called spapr_dimm_get_address
>> was created to fetch the address of a PCDIMM device inside spapr_lmb_release.
>> When nr_lmbs reaches zero and the callback proceeds with the qdev hot unplug
>> calls, the sPAPRDIMMState struct is removed from spapr->pending_dimm_unplugs.
>>
>> After these changes, the opaque argument for spapr_lmb_release is now
>> unused and is passed as NULL inside spapr_del_lmbs. This and the other
>> opaque arguments can now be safely removed from the code.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> Urgh.  Moving this into the machine is really ugly.  Unfortunately, I
> can't quickly see a better way to accomplish what you need.  So I
> guess this approach is ok, with the hope that we can find a better way
> in future.

Agreed.

>
> There are a few more superficial problems to address, though.
>
>> ---
>>   hw/ppc/spapr.c         | 54 ++++++++++++++++++++++++++++++++++++++++++++------
>>   include/hw/ppc/spapr.h | 17 ++++++++++++++++
>>   2 files changed, 65 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 80d12d0..346c827 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -2043,6 +2043,7 @@ static void ppc_spapr_init(MachineState *machine)
>>       msi_nonbroken = true;
>>   
>>       QLIST_INIT(&spapr->phbs);
>> +    QTAILQ_INIT(&spapr->pending_dimm_unplugs);
>>   
>>       /* Allocate RMA if necessary */
>>       rma_alloc_size = kvmppc_alloc_rma(&rma);
>> @@ -2596,20 +2597,32 @@ out:
>>       error_propagate(errp, local_err);
>>   }
>>   
>> -typedef struct sPAPRDIMMState {
>> -    uint32_t nr_lmbs;
>> -} sPAPRDIMMState;
>> +static uint64_t spapr_dimm_get_address(PCDIMMDevice *dimm)
>> +{
>> +    Error *local_err = NULL;
>> +    uint64_t addr;
>> +    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP,
>> +                                   &local_err);
>> +    if (local_err) {
>> +        error_propagate(&error_abort, local_err);
>> +        return 0;
>> +    }
>> +    return addr;
>> +}
>>   
>>   static void spapr_lmb_release(DeviceState *dev, void *opaque)
>>   {
>> -    sPAPRDIMMState *ds = (sPAPRDIMMState *)opaque;
>>       HotplugHandler *hotplug_ctrl;
>>   
> No need for this blank line in the middle of declarations.
>
>> +    uint64_t addr = spapr_dimm_get_address(PC_DIMM(dev));
>> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>> +    sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, addr);
>> +
>>       if (--ds->nr_lmbs) {
>>           return;
>>       }
>>   
>> -    g_free(ds);
>> +    spapr_pending_dimm_unplugs_remove(spapr, ds);
>>   
>>       /*
>>        * Now that all the LMBs have been removed by the guest, call the
>> @@ -2626,17 +2639,20 @@ static void spapr_del_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>>       sPAPRDRConnectorClass *drck;
>>       uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
>>       int i;
>> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>>       sPAPRDIMMState *ds = g_malloc0(sizeof(sPAPRDIMMState));
>>       uint64_t addr = addr_start;
>>   
>>       ds->nr_lmbs = nr_lmbs;
>> +    ds->addr = addr_start;
>> +    spapr_pending_dimm_unplugs_add(spapr, ds);
>>       for (i = 0; i < nr_lmbs; i++) {
>>           drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
>>                   addr / SPAPR_MEMORY_BLOCK_SIZE);
>>           g_assert(drc);
>>   
>>           drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>> -        drck->detach(drc, dev, spapr_lmb_release, ds, errp);
>> +        drck->detach(drc, dev, spapr_lmb_release, NULL, errp);
>>           addr += SPAPR_MEMORY_BLOCK_SIZE;
>>       }
>>   
>> @@ -3515,3 +3531,29 @@ static void spapr_machine_register_types(void)
>>   }
>>   
>>   type_init(spapr_machine_register_types)
>> +
>> +sPAPRDIMMState *spapr_pending_dimm_unplugs_find(sPAPRMachineState *spapr,
>> +                                                uint64_t addr)
>> +{
>> +    sPAPRDIMMState *dimm_state = NULL;
>> +    QTAILQ_FOREACH(dimm_state, &spapr->pending_dimm_unplugs, next) {
>> +        if (dimm_state->addr == addr) {
>> +            break;
>> +        }
>> +    }
>> +    return dimm_state;
>> +}
>> +
>> +void spapr_pending_dimm_unplugs_add(sPAPRMachineState *spapr,
>> +                                   sPAPRDIMMState *dimm_state)
>> +{
>> +    g_assert(!spapr_pending_dimm_unplugs_find(spapr, dimm_state->addr));
>> +    QTAILQ_INSERT_HEAD(&spapr->pending_dimm_unplugs, dimm_state, next);
>> +}
>> +
>> +void spapr_pending_dimm_unplugs_remove(sPAPRMachineState *spapr,
>> +                                      sPAPRDIMMState *dimm_state)
>> +{
>> +    QTAILQ_REMOVE(&spapr->pending_dimm_unplugs, dimm_state, next);
>> +    g_free(dimm_state);
>> +}
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 5802f88..3e2b5ab 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -32,6 +32,7 @@ struct sPAPRRTCState {
>>       int64_t ns_offset;
>>   };
>>   
>> +typedef struct sPAPRDIMMState sPAPRDIMMState;
>>   typedef struct sPAPRMachineClass sPAPRMachineClass;
>>   
>>   #define TYPE_SPAPR_MACHINE      "spapr-machine"
>> @@ -104,6 +105,9 @@ struct sPAPRMachineState {
>>       /* RTAS state */
>>       QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list;
>>   
>> +    /* pending DIMM unplug queue */
>> +    QTAILQ_HEAD(, sPAPRDIMMState) pending_dimm_unplugs;
>> +
>>       /*< public >*/
>>       char *kvm_type;
>>       MemoryHotplugState hotplug_memory;
>> @@ -646,6 +650,19 @@ struct sPAPRConfigureConnectorState {
>>   
>>   void spapr_ccs_reset_hook(void *opaque);
>>   
>> +struct sPAPRDIMMState {
>> +    uint64_t addr;
>> +    uint32_t nr_lmbs;
>> +    QTAILQ_ENTRY(sPAPRDIMMState) next;
>> +};
>> +
>> +sPAPRDIMMState *spapr_pending_dimm_unplugs_find(sPAPRMachineState *spapr,
>> +                                               uint64_t addr);
>> +void spapr_pending_dimm_unplugs_add(sPAPRMachineState *spapr,
>> +                                   sPAPRDIMMState *dimm_state);
>> +void spapr_pending_dimm_unplugs_remove(sPAPRMachineState *spapr,
>> +                                      sPAPRDIMMState *dimm_state);
>> +
> AFAICT all these new functions are only used in spapr.c, even in the
> rest of the series.  So they should be static, and not in the header
> file.  Likewise the structure definition.
>
>>   void spapr_rtc_read(sPAPRRTCState *rtc, struct tm *tm, uint32_t *ns);
>>   int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t legacy_offset);
>>   

Re: [Qemu-devel] [Qemu-ppc] [PATCH v9 3/6] hw/ppc: migrating the DRC state of hotplugged devices

[Daniel Henrique Barboza]

On 05/12/2017 03:11 AM, David Gibson wrote:
> On Fri, May 05, 2017 at 05:47:43PM -0300, Daniel Henrique Barboza wrote:
>> In pseries, a firmware abstraction called Dynamic Reconfiguration
>> Connector (DRC) is used to assign a particular dynamic resource
>> to the guest and provide an interface to manage configuration/removal
>> of the resource associated with it. In other words, DRC is the
>> 'plugged state' of a device.
>>
>> Before this patch, DRC wasn't being migrated. This causes
>> post-migration problems due to DRC state mismatch between source and
>> target. The DRC state of a device X in the source might
>> change, while in the target the DRC state of X is still fresh. When
>> migrating the guest, X will not have the same hotplugged state as it
>> did in the source. This means that we can't hot unplug X in the
>> target after migration is completed because its DRC state is not consistent.
>> https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1677552 is one
>> bug that is caused by this DRC state mismatch between source and
>> target.
>>
>> To migrate the DRC state, we defined the VMStateDescription struct for
>> spapr_drc to enable the transmission of spapr_drc state in migration.
>> Not all the elements in the DRC state are migrated - only those
>> that can be modified by guest actions or device add/remove
>> operations:
>>
>> - 'isolation_state', 'allocation_state' and 'indicator_state'
>> are involved in the DR state transition diagram from
>> PAPR+ 2.7, 13.4;
>>
>> - 'configured', 'signalled', 'awaiting_release' and 'awaiting_allocation'
>> are needed in attaching and detaching devices;
>>
>> - 'indicator_state' provides users with hardware state information.
>>
>> These are the DRC elements that are migrated.
>>
>> In this patch the DRC state is migrated for PCI, LMB and CPU
>> connector types. At this moment there is no support to migrate
>> DRC for the PHB (PCI Host Bridge) type.
>>
>> In the 'realize' function the DRC is registered using vmstate_register,
>> similar to what hw/ppc/spapr_iommu.c does in 'spapr_tce_table_realize'.
>> This approach works because  DRCs are bus-less and do not sit
>> on a BusClass that implements bc->get_dev_path, so as a fallback the
>> VMSD gets identified via "spapr_drc"/get_index(drc).
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
>> ---
>>   hw/ppc/spapr_drc.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 61 insertions(+)
>>
>> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>> index 1c72160..926b945 100644
>> --- a/hw/ppc/spapr_drc.c
>> +++ b/hw/ppc/spapr_drc.c
>> @@ -519,6 +519,65 @@ static void reset(DeviceState *d)
>>       }
>>   }
>>   
>> +static bool spapr_drc_needed(void *opaque)
>> +{
>> +    sPAPRDRConnector *drc = (sPAPRDRConnector *)opaque;
>> +    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>> +    bool rc = false;
>> +    sPAPRDREntitySense value;
> Blank line after the declarations, please.
>
>> +    drck->entity_sense(drc, &value);
>> +    /* If no dev is plugged in there is no need to migrate the DRC state */
>> +    if (value != SPAPR_DR_ENTITY_SENSE_PRESENT) {
>> +        return false;
>> +    }
>> +
>> +    /*
>> +     * If there is dev plugged in, we need to migrate the DRC state when
>> +     * it is different from cold-plugged state
>> +     */
>> +    switch (drc->type) {
>> +
> No blank line here please.
>
>> +    case SPAPR_DR_CONNECTOR_TYPE_PCI:
>> +        rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) &&
>> +               (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) &&
>> +               drc->configured && drc->signalled && !drc->awaiting_release);
> You don't do any more manipulation of the rc value, so you might as
> well just 'return' directly here.
>
>
>> +        break;
>> +
>> +    case SPAPR_DR_CONNECTOR_TYPE_LMB:
>> +        rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
>> +               (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) &&
>> +               drc->configured && drc->signalled && !drc->awaiting_release);
>> +        break;
>> +
>> +    case SPAPR_DR_CONNECTOR_TYPE_CPU:
>> +        rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
>> +               (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) &&
>> +                drc->configured && drc->signalled && !drc->awaiting_release);
>> +        break;
>> +
>> +    default:
>> +        ;
> This should probably assert().
>
>> +    }
>> +    return rc;
>> +}
>> +
>> +static const VMStateDescription vmstate_spapr_drc = {
>> +    .name = "spapr_drc",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .needed = spapr_drc_needed,
>> +    .fields  = (VMStateField []) {
>> +        VMSTATE_UINT32(isolation_state, sPAPRDRConnector),
>> +        VMSTATE_UINT32(allocation_state, sPAPRDRConnector),
>> +        VMSTATE_UINT32(indicator_state, sPAPRDRConnector),
>> +        VMSTATE_BOOL(configured, sPAPRDRConnector),
>> +        VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
>> +        VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
>> +        VMSTATE_BOOL(signalled, sPAPRDRConnector),
> Hrm.  I'm happy translation the 3 state integers, since their meaning
> is in the PAPR spec.  The others are qemu local information, so it's
> not as clear that they'll have a stable meaning.  Are you absolutely
> sure you need all of them?

The first time I saw this code (originally from Jianjun) I've tried to 
simplify/cut
some of these values in the hope that they could be deduced later in the
post_load. Unfortunately I've reached the same conclusion as him: these qemu
state information isn't easily deduced in the post_load stage today and 
removing
any of them from migration will break the unplug process afterwards.

This doesn't mean that we can't simplify it in the future though. I have 
some ideas
of how we can simplify the DRC code (splitting the logic into LogicalDRC 
and PhysicalDRC
for example) that can help simplify all this DRC logic altogether. I'm 
also thinking about
adding unit tests for this DRC code too - it would help us to make a 
redesign without
worrying too much about unintended bugs.


Daniel

>
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>   static void realize(DeviceState *d, Error **errp)
>>   {
>>       sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
>> @@ -547,6 +606,8 @@ static void realize(DeviceState *d, Error **errp)
>>           object_unref(OBJECT(drc));
>>       }
>>       g_free(child_name);
>> +    vmstate_register(DEVICE(drc), drck->get_index(drc), &vmstate_spapr_drc,
>> +                     drc);
>>       trace_spapr_drc_realize_complete(drck->get_index(drc));
>>   }
>>   

Re: [Qemu-devel] [Qemu-ppc] [PATCH v9 4/6] hw/ppc/spapr.c: migrate pending_dimm_unplugs of spapr state

[David Gibson]

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

On Fri, May 12, 2017 at 04:54:57PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 05/12/2017 03:12 AM, David Gibson wrote:
> > On Fri, May 05, 2017 at 05:47:44PM -0300, Daniel Henrique Barboza wrote:
> > > To allow for a DIMM unplug event to resume its work if a migration
> > > occurs in the middle of it, this patch migrates the non-empty
> > > pending_dimm_unplugs QTAILQ that stores the DIMM information
> > > that the spapr_lmb_release() callback uses.
> > > 
> > > It was considered an apprach where the DIMM states would be restored
> > > on the post-_load after a migration. The problem is that there is
> > > no way of knowing, from the sPAPRMachineState, if a given DIMM is going
> > > through an unplug process and the callback needs the updated DIMM State.
> > > 
> > > We could migrate a flag indicating that there is an unplug event going
> > > on for a certain DIMM, fetching this information from the start of the
> > > spapr_del_lmbs call. But this would also require a scan on post_load to
> > > figure out how many nr_lmbs are left. At this point we can just
> > > migrate the nr_lmbs information as well, given that it is being calculated
> > > at spapr_del_lmbs already, and spare a scanning/discovery in the
> > > post-load. All that we need is inside the sPAPRDIMMState structure
> > > that is added to the pending_dimm_unplugs queue at the start of the
> > > spapr_del_lmbs, so it's convenient to just migrated this queue it if it's
> > > not empty.
> > > 
> > > Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> > NACK.
> > 
> > As I believe I suggested previously, you can reconstruct this state on
> > the receiving side by doing a full scan of the DIMM and LMB DRC states.
> 
> Just had an idea that I think it's in the line of what you're suggesting.
> Given
> that the information we need is only created in the spapr_del_lmbs
> (as per patch 1), we can use the absence of this information in the
> release callback as a sort of a flag, an indication that a migration got
> in the way and we need to reconstruct the nr_lmbs states again, using
> the same scanning function I've used in v8.
> 
> The flow would be like this (considering the changes in the
> previous 3 patches so far):
> 
> ------------
> 
> /* Callback to be called during DRC release. */
> void spapr_lmb_release(DeviceState *dev)
> {
>      HotplugHandler *hotplug_ctrl;
> 
>      uint64_t addr = spapr_dimm_get_address(PC_DIMM(dev));
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, addr);
> 
>     // no DIMM state found in spapr - re-create it to find out how may LMBs
> are left
>     if (ds == NULL) {
>         uint32 nr_lmbs  = ***call_scanning_LMB_DRCs_function(dev)***
>         // recreate the sPAPRDIMMState element and add it back to spapr
>     }
> 
>     ( resume callback as usual )
> 
> -----------

Yes, the above seems like a reasonable plan.

> Is this approach be adequate? Another alternative would be to use another
> way of detecting if an LMB unplug is happening and, if positive, do the same
> process in the post_load(). In this case I'll need to take a look in the
> code and
> see how we can detect an ongoing unplug besides what I've said above.

You could, but I think the lazy approach above is preferable.

> 
> Thanks,
> 
> 
> Daniel
> 
> > 
> > > ---
> > >   hw/ppc/spapr.c | 31 +++++++++++++++++++++++++++++++
> > >   1 file changed, 31 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index e190eb9..30f0b7b 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1437,6 +1437,36 @@ static bool version_before_3(void *opaque, int version_id)
> > >       return version_id < 3;
> > >   }
> > > +static bool spapr_pending_dimm_unplugs_needed(void *opaque)
> > > +{
> > > +    sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> > > +    return !QTAILQ_EMPTY(&spapr->pending_dimm_unplugs);
> > > +}
> > > +
> > > +static const VMStateDescription vmstate_spapr_dimmstate = {
> > > +    .name = "spapr_dimm_state",
> > > +    .version_id = 1,
> > > +    .minimum_version_id = 1,
> > > +    .fields = (VMStateField[]) {
> > > +        VMSTATE_UINT64(addr, sPAPRDIMMState),
> > > +        VMSTATE_UINT32(nr_lmbs, sPAPRDIMMState),
> > > +        VMSTATE_END_OF_LIST()
> > > +    },
> > > +};
> > > +
> > > +static const VMStateDescription vmstate_spapr_pending_dimm_unplugs = {
> > > +    .name = "spapr_pending_dimm_unplugs",
> > > +    .version_id = 1,
> > > +    .minimum_version_id = 1,
> > > +    .needed = spapr_pending_dimm_unplugs_needed,
> > > +    .fields = (VMStateField[]) {
> > > +        VMSTATE_QTAILQ_V(pending_dimm_unplugs, sPAPRMachineState, 1,
> > > +                         vmstate_spapr_dimmstate, sPAPRDIMMState,
> > > +                         next),
> > > +        VMSTATE_END_OF_LIST()
> > > +    },
> > > +};
> > > +
> > >   static bool spapr_ov5_cas_needed(void *opaque)
> > >   {
> > >       sPAPRMachineState *spapr = opaque;
> > > @@ -1535,6 +1565,7 @@ static const VMStateDescription vmstate_spapr = {
> > >       .subsections = (const VMStateDescription*[]) {
> > >           &vmstate_spapr_ov5_cas,
> > >           &vmstate_spapr_patb_entry,
> > > +        &vmstate_spapr_pending_dimm_unplugs,
> > >           NULL
> > >       }
> > >   };
> 

-- 
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: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH v9 3/6] hw/ppc: migrating the DRC state of hotplugged devices

[David Gibson]

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

On Tue, May 16, 2017 at 10:46:23AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 05/12/2017 03:11 AM, David Gibson wrote:
> > On Fri, May 05, 2017 at 05:47:43PM -0300, Daniel Henrique Barboza wrote:
> > > In pseries, a firmware abstraction called Dynamic Reconfiguration
> > > Connector (DRC) is used to assign a particular dynamic resource
> > > to the guest and provide an interface to manage configuration/removal
> > > of the resource associated with it. In other words, DRC is the
> > > 'plugged state' of a device.
> > > 
> > > Before this patch, DRC wasn't being migrated. This causes
> > > post-migration problems due to DRC state mismatch between source and
> > > target. The DRC state of a device X in the source might
> > > change, while in the target the DRC state of X is still fresh. When
> > > migrating the guest, X will not have the same hotplugged state as it
> > > did in the source. This means that we can't hot unplug X in the
> > > target after migration is completed because its DRC state is not consistent.
> > > https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1677552 is one
> > > bug that is caused by this DRC state mismatch between source and
> > > target.
> > > 
> > > To migrate the DRC state, we defined the VMStateDescription struct for
> > > spapr_drc to enable the transmission of spapr_drc state in migration.
> > > Not all the elements in the DRC state are migrated - only those
> > > that can be modified by guest actions or device add/remove
> > > operations:
> > > 
> > > - 'isolation_state', 'allocation_state' and 'indicator_state'
> > > are involved in the DR state transition diagram from
> > > PAPR+ 2.7, 13.4;
> > > 
> > > - 'configured', 'signalled', 'awaiting_release' and 'awaiting_allocation'
> > > are needed in attaching and detaching devices;
> > > 
> > > - 'indicator_state' provides users with hardware state information.
> > > 
> > > These are the DRC elements that are migrated.
> > > 
> > > In this patch the DRC state is migrated for PCI, LMB and CPU
> > > connector types. At this moment there is no support to migrate
> > > DRC for the PHB (PCI Host Bridge) type.
> > > 
> > > In the 'realize' function the DRC is registered using vmstate_register,
> > > similar to what hw/ppc/spapr_iommu.c does in 'spapr_tce_table_realize'.
> > > This approach works because  DRCs are bus-less and do not sit
> > > on a BusClass that implements bc->get_dev_path, so as a fallback the
> > > VMSD gets identified via "spapr_drc"/get_index(drc).
> > > 
> > > Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> > > ---
> > >   hw/ppc/spapr_drc.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 61 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > index 1c72160..926b945 100644
> > > --- a/hw/ppc/spapr_drc.c
> > > +++ b/hw/ppc/spapr_drc.c
> > > @@ -519,6 +519,65 @@ static void reset(DeviceState *d)
> > >       }
> > >   }
> > > +static bool spapr_drc_needed(void *opaque)
> > > +{
> > > +    sPAPRDRConnector *drc = (sPAPRDRConnector *)opaque;
> > > +    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > +    bool rc = false;
> > > +    sPAPRDREntitySense value;
> > Blank line after the declarations, please.
> > 
> > > +    drck->entity_sense(drc, &value);
> > > +    /* If no dev is plugged in there is no need to migrate the DRC state */
> > > +    if (value != SPAPR_DR_ENTITY_SENSE_PRESENT) {
> > > +        return false;
> > > +    }
> > > +
> > > +    /*
> > > +     * If there is dev plugged in, we need to migrate the DRC state when
> > > +     * it is different from cold-plugged state
> > > +     */
> > > +    switch (drc->type) {
> > > +
> > No blank line here please.
> > 
> > > +    case SPAPR_DR_CONNECTOR_TYPE_PCI:
> > > +        rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) &&
> > > +               (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) &&
> > > +               drc->configured && drc->signalled && !drc->awaiting_release);
> > You don't do any more manipulation of the rc value, so you might as
> > well just 'return' directly here.
> > 
> > 
> > > +        break;
> > > +
> > > +    case SPAPR_DR_CONNECTOR_TYPE_LMB:
> > > +        rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
> > > +               (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) &&
> > > +               drc->configured && drc->signalled && !drc->awaiting_release);
> > > +        break;
> > > +
> > > +    case SPAPR_DR_CONNECTOR_TYPE_CPU:
> > > +        rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
> > > +               (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) &&
> > > +                drc->configured && drc->signalled && !drc->awaiting_release);
> > > +        break;
> > > +
> > > +    default:
> > > +        ;
> > This should probably assert().
> > 
> > > +    }
> > > +    return rc;
> > > +}
> > > +
> > > +static const VMStateDescription vmstate_spapr_drc = {
> > > +    .name = "spapr_drc",
> > > +    .version_id = 1,
> > > +    .minimum_version_id = 1,
> > > +    .needed = spapr_drc_needed,
> > > +    .fields  = (VMStateField []) {
> > > +        VMSTATE_UINT32(isolation_state, sPAPRDRConnector),
> > > +        VMSTATE_UINT32(allocation_state, sPAPRDRConnector),
> > > +        VMSTATE_UINT32(indicator_state, sPAPRDRConnector),
> > > +        VMSTATE_BOOL(configured, sPAPRDRConnector),
> > > +        VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
> > > +        VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
> > > +        VMSTATE_BOOL(signalled, sPAPRDRConnector),
> > Hrm.  I'm happy translation the 3 state integers, since their meaning
> > is in the PAPR spec.  The others are qemu local information, so it's
> > not as clear that they'll have a stable meaning.  Are you absolutely
> > sure you need all of them?
> 
> The first time I saw this code (originally from Jianjun) I've tried to
> simplify/cut
> some of these values in the hope that they could be deduced later in the
> post_load. Unfortunately I've reached the same conclusion as him: these qemu
> state information isn't easily deduced in the post_load stage today and
> removing
> any of them from migration will break the unplug process afterwards.

Ok.

> This doesn't mean that we can't simplify it in the future though. I have
> some ideas
> of how we can simplify the DRC code (splitting the logic into LogicalDRC and
> PhysicalDRC
> for example) that can help simplify all this DRC logic altogether.

Ok.  I like that idea in principle, but bear in mind that refactoring
the data in the DRC is likely to mean a bunch of complicated
compatibility code to handle migrations.

> I'm also
> thinking about
> adding unit tests for this DRC code too - it would help us to make a
> redesign without
> worrying too much about unintended bugs.

That sounds great.
> 
> 
> Daniel
> 
> > 
> > > +        VMSTATE_END_OF_LIST()
> > > +    }
> > > +};
> > > +
> > >   static void realize(DeviceState *d, Error **errp)
> > >   {
> > >       sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
> > > @@ -547,6 +606,8 @@ static void realize(DeviceState *d, Error **errp)
> > >           object_unref(OBJECT(drc));
> > >       }
> > >       g_free(child_name);
> > > +    vmstate_register(DEVICE(drc), drck->get_index(drc), &vmstate_spapr_drc,
> > > +                     drc);
> > >       trace_spapr_drc_realize_complete(drck->get_index(drc));
> > >   }
> 

-- 
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: 819 bytes --]