* [Qemu-devel] [QEMU RFC PATCH v3 0/6]Migration: ensure hotplug and migration work together
@ 2016-05-31 18:02 Jianjun Duan
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 1/6] Migration: Defined VMStateDescription struct for spapr_drc Jianjun Duan
` (5 more replies)
0 siblings, 6 replies; 27+ messages in thread
From: Jianjun Duan @ 2016-05-31 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, duanj, dmitry, peter.maydell, kraxel, mst, david,
pbonzini, veroniabahaa, quintela, amit.shah, mreitz, kwolf, rth,
aurelien, leon.alrae, blauwirbel, mark.cave-ayland, mdroth
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.
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 device (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 also takes care of it.
Alexey Kardashevskiy (1):
vmstate: Define VARRAY with VMS_ALLOC
Jianjun Duan (5):
Migration: Defined VMStateDescription struct for spapr_drc
Migration: extend VMStateInfo
Migration: migrate QTAILQ
Migration: migrate ccs_list in spapr state
Migration: migrate pending_events of spapr state
hw/net/vmxnet3.c | 18 +++--
hw/nvram/eeprom93xx.c | 6 +-
hw/nvram/fw_cfg.c | 6 +-
hw/pci/msix.c | 6 +-
hw/pci/pci.c | 12 ++-
hw/pci/shpc.c | 5 +-
hw/ppc/spapr.c | 67 +++++++++++++++++
hw/ppc/spapr_drc.c | 61 ++++++++++++++++
hw/ppc/spapr_events.c | 22 +++---
hw/ppc/spapr_pci.c | 22 ++++++
hw/scsi/scsi-bus.c | 6 +-
hw/timer/twl92230.c | 6 +-
hw/usb/redirect.c | 18 +++--
hw/virtio/virtio-pci.c | 6 +-
hw/virtio/virtio.c | 6 +-
include/hw/ppc/spapr.h | 3 +-
include/hw/ppc/spapr_drc.h | 9 +++
include/migration/vmstate.h | 42 ++++++++++-
include/qemu/queue.h | 32 ++++++++
migration/savevm.c | 5 +-
migration/vmstate.c | 174 +++++++++++++++++++++++++++++++++++---------
target-alpha/machine.c | 5 +-
target-arm/machine.c | 12 ++-
target-i386/machine.c | 21 ++++--
target-mips/machine.c | 10 ++-
target-ppc/machine.c | 10 ++-
target-sparc/machine.c | 5 +-
27 files changed, 488 insertions(+), 107 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [QEMU RFC PATCH v3 1/6] Migration: Defined VMStateDescription struct for spapr_drc
2016-05-31 18:02 [Qemu-devel] [QEMU RFC PATCH v3 0/6]Migration: ensure hotplug and migration work together Jianjun Duan
@ 2016-05-31 18:02 ` Jianjun Duan
2016-06-02 4:07 ` David Gibson
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 2/6] vmstate: Define VARRAY with VMS_ALLOC Jianjun Duan
` (4 subsequent siblings)
5 siblings, 1 reply; 27+ messages in thread
From: Jianjun Duan @ 2016-05-31 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, duanj, dmitry, peter.maydell, kraxel, mst, david,
pbonzini, veroniabahaa, quintela, amit.shah, mreitz, kwolf, rth,
aurelien, leon.alrae, blauwirbel, mark.cave-ayland, mdroth
To manage hotplug/unplug of dynamic resources such as PCI cards,
memory, and CPU on sPAPR guests, a firmware abstraction known as
a Dynamic Resource Connector (DRC) is used to assign a particular
dynamic resource to the guest, and provide an interface for the
guest to manage configuration/removal of the resource associated
with it.
To migrate the hotplugged resources in migration, the
associated DRC state need be migrated. 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
ones modifiable or needed by guest actions or device add/remove
operation are migrated. From the perspective of device
hotplugging, if we hotplug a device on the source, we need to
"coldplug" it on the target. The states across two hosts for the
same device are not the same. Ideally we want the states be same
after migration so that the device would function as hotplugged
on the target. For example we can unplug it. The minimum DRC
state we need to transfer should cover all the pieces changed by
hotplugging. Out of the elements of the DRC state, isolation_state,
allocation_sate, and configured are involved in the DR state
transition diagram from PAPR+ 2.7, 13.4. configured and signalled
are needed in attaching and detaching devices. indicator_state
provides users with hardware state information. These 6 elements
are migrated.
detach_cb in the DRC state is a function pointer that cannot be
migrated. We set it right after DRC state is migrated so that
a migrated hot-unplug event could finish its work.
Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
---
hw/ppc/spapr_drc.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++
hw/ppc/spapr_pci.c | 22 +++++++++++++++++
include/hw/ppc/spapr_drc.h | 9 +++++++
3 files changed, 92 insertions(+)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 94c875d..1fb5e23 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -617,6 +617,65 @@ static void spapr_dr_connector_instance_init(Object *obj)
NULL, NULL, NULL, NULL);
}
+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) {
+ /* for PCI 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;
+ /* for LMB type */
+ 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;
+ default:
+ ;
+ }
+
+ return rc;
+}
+
+/* detach_cb needs be set since it is not migrated */
+static void postmigrate_set_detach_cb(sPAPRDRConnector *drc,
+ spapr_drc_detach_cb *detach_cb)
+{
+ drc->detach_cb = detach_cb;
+}
+
+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(signalled, sPAPRDRConnector),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
{
DeviceClass *dk = DEVICE_CLASS(k);
@@ -625,6 +684,7 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
dk->reset = reset;
dk->realize = realize;
dk->unrealize = unrealize;
+ dk->vmsd = &vmstate_spapr_drc;
drck->set_isolation_state = set_isolation_state;
drck->set_indicator_state = set_indicator_state;
drck->set_allocation_state = set_allocation_state;
@@ -638,6 +698,7 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
drck->detach = detach;
drck->release_pending = release_pending;
drck->set_signalled = set_signalled;
+ drck->postmigrate_set_detach_cb = postmigrate_set_detach_cb;
/*
* Reason: it crashes FIXME find and document the real reason
*/
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 856aec7..c68da08 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1562,11 +1562,33 @@ static void spapr_pci_pre_save(void *opaque)
}
}
+/*
+ * detach_cb in the DRC state is a function pointer that cannot be
+ * migrated. We set it right after migration so that a migrated
+ * hot-unplug event could finish its work.
+ */
+static void spapr_pci_set_detach_cb(PCIBus *bus, PCIDevice *pdev,
+ void *opaque)
+{
+ sPAPRPHBState *sphb = opaque;
+ sPAPRDRConnector *drc = spapr_phb_get_pci_drc(sphb, pdev);
+ sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+ drck->postmigrate_set_detach_cb(drc, spapr_phb_remove_pci_device_cb);
+}
+
static int spapr_pci_post_load(void *opaque, int version_id)
{
sPAPRPHBState *sphb = opaque;
gpointer key, value;
int i;
+ PCIBus *bus = PCI_HOST_BRIDGE(sphb)->bus;
+ unsigned int bus_no = 0;
+
+ /* Set detach_cb for the drc unconditionally after migration */
+ if (bus) {
+ pci_for_each_device(bus, pci_bus_num(bus), spapr_pci_set_detach_cb,
+ &bus_no);
+ }
for (i = 0; i < sphb->msi_devs_num; ++i) {
key = g_memdup(&sphb->msi_devs[i].key,
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index fa21ba0..a68433b 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -190,6 +190,15 @@ typedef struct sPAPRDRConnectorClass {
void *detach_cb_opaque, Error **errp);
bool (*release_pending)(sPAPRDRConnector *drc);
void (*set_signalled)(sPAPRDRConnector *drc);
+
+ /*
+ * QEMU interface for setting detach_cb after migration.
+ * detach_cb in the DRC state is a function pointer that cannot be
+ * migrated. We set it right after migration so that a migrated
+ * hot-unplug event could finish its work.
+ */
+ void (*postmigrate_set_detach_cb)(sPAPRDRConnector *drc,
+ spapr_drc_detach_cb *detach_cb);
} sPAPRDRConnectorClass;
sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
--
1.9.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [QEMU RFC PATCH v3 2/6] vmstate: Define VARRAY with VMS_ALLOC
2016-05-31 18:02 [Qemu-devel] [QEMU RFC PATCH v3 0/6]Migration: ensure hotplug and migration work together Jianjun Duan
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 1/6] Migration: Defined VMStateDescription struct for spapr_drc Jianjun Duan
@ 2016-05-31 18:02 ` Jianjun Duan
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 3/6] Migration: extend VMStateInfo Jianjun Duan
` (3 subsequent siblings)
5 siblings, 0 replies; 27+ messages in thread
From: Jianjun Duan @ 2016-05-31 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, duanj, dmitry, peter.maydell, kraxel, mst, david,
pbonzini, veroniabahaa, quintela, amit.shah, mreitz, kwolf, rth,
aurelien, leon.alrae, blauwirbel, mark.cave-ayland, mdroth
From: Alexey Kardashevskiy <aik@ozlabs.ru>
This allows dynamic allocation for migrating arrays.
Already existing VMSTATE_VARRAY_UINT32 requires an array to be
pre-allocated, however there are cases when the size is not known in
advance and there is no real need to enforce it.
This defines another variant of VMSTATE_VARRAY_UINT32 with WMS_ALLOC
flag which tells the receiving side to allocate memory for the array
before receiving the data.
The first user of it is a dynamic DMA window which existence and size
are totally dynamic.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
---
include/migration/vmstate.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 30ecc44..6c65811 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -386,6 +386,16 @@ extern const VMStateInfo vmstate_info_bitmap;
.offset = vmstate_offset_pointer(_state, _field, _type), \
}
+#define VMSTATE_VARRAY_UINT32_ALLOC(_field, _state, _field_num, _version, _info, _type) {\
+ .name = (stringify(_field)), \
+ .version_id = (_version), \
+ .num_offset = vmstate_offset_value(_state, _field_num, uint32_t),\
+ .info = &(_info), \
+ .size = sizeof(_type), \
+ .flags = VMS_VARRAY_UINT32|VMS_POINTER|VMS_ALLOC, \
+ .offset = vmstate_offset_pointer(_state, _field, _type), \
+}
+
#define VMSTATE_VARRAY_UINT16_UNSAFE(_field, _state, _field_num, _version, _info, _type) {\
.name = (stringify(_field)), \
.version_id = (_version), \
--
1.9.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [QEMU RFC PATCH v3 3/6] Migration: extend VMStateInfo
2016-05-31 18:02 [Qemu-devel] [QEMU RFC PATCH v3 0/6]Migration: ensure hotplug and migration work together Jianjun Duan
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 1/6] Migration: Defined VMStateDescription struct for spapr_drc Jianjun Duan
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 2/6] vmstate: Define VARRAY with VMS_ALLOC Jianjun Duan
@ 2016-05-31 18:02 ` Jianjun Duan
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ Jianjun Duan
` (2 subsequent siblings)
5 siblings, 0 replies; 27+ messages in thread
From: Jianjun Duan @ 2016-05-31 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, duanj, dmitry, peter.maydell, kraxel, mst, david,
pbonzini, veroniabahaa, quintela, amit.shah, mreitz, kwolf, rth,
aurelien, leon.alrae, blauwirbel, mark.cave-ayland, mdroth
Current migration code cannot handle some data structures such as
QTAILQ in qemu/queue.h. Here we extend the signatures of put/get
in VMStateInfo so that customized handling is supported.
Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
---
hw/net/vmxnet3.c | 18 ++++++---
hw/nvram/eeprom93xx.c | 6 ++-
hw/nvram/fw_cfg.c | 6 ++-
hw/pci/msix.c | 6 ++-
hw/pci/pci.c | 12 ++++--
hw/pci/shpc.c | 5 ++-
hw/scsi/scsi-bus.c | 6 ++-
hw/timer/twl92230.c | 6 ++-
hw/usb/redirect.c | 18 ++++++---
hw/virtio/virtio-pci.c | 6 ++-
hw/virtio/virtio.c | 6 ++-
include/migration/vmstate.h | 10 +++--
migration/savevm.c | 5 ++-
migration/vmstate.c | 95 ++++++++++++++++++++++++++++-----------------
target-alpha/machine.c | 5 ++-
target-arm/machine.c | 12 ++++--
target-i386/machine.c | 21 ++++++----
target-mips/machine.c | 10 +++--
target-ppc/machine.c | 10 +++--
target-sparc/machine.c | 5 ++-
20 files changed, 171 insertions(+), 97 deletions(-)
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 20f26b7..7ba77a4 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2439,7 +2439,8 @@ static void vmxnet3_put_tx_stats_to_file(QEMUFile *f,
qemu_put_be64(f, tx_stat->pktsTxDiscard);
}
-static int vmxnet3_get_txq_descr(QEMUFile *f, void *pv, size_t size)
+static int vmxnet3_get_txq_descr(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
Vmxnet3TxqDescr *r = pv;
@@ -2453,7 +2454,8 @@ static int vmxnet3_get_txq_descr(QEMUFile *f, void *pv, size_t size)
return 0;
}
-static void vmxnet3_put_txq_descr(QEMUFile *f, void *pv, size_t size)
+static void vmxnet3_put_txq_descr(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field, QJSON *vmdesc)
{
Vmxnet3TxqDescr *r = pv;
@@ -2500,7 +2502,8 @@ static void vmxnet3_put_rx_stats_to_file(QEMUFile *f,
qemu_put_be64(f, rx_stat->pktsRxError);
}
-static int vmxnet3_get_rxq_descr(QEMUFile *f, void *pv, size_t size)
+static int vmxnet3_get_rxq_descr(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
Vmxnet3RxqDescr *r = pv;
int i;
@@ -2518,7 +2521,8 @@ static int vmxnet3_get_rxq_descr(QEMUFile *f, void *pv, size_t size)
return 0;
}
-static void vmxnet3_put_rxq_descr(QEMUFile *f, void *pv, size_t size)
+static void vmxnet3_put_rxq_descr(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field, QJSON *vmdesc)
{
Vmxnet3RxqDescr *r = pv;
int i;
@@ -2562,7 +2566,8 @@ static const VMStateInfo rxq_descr_info = {
.put = vmxnet3_put_rxq_descr
};
-static int vmxnet3_get_int_state(QEMUFile *f, void *pv, size_t size)
+static int vmxnet3_get_int_state(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
Vmxnet3IntState *r = pv;
@@ -2573,7 +2578,8 @@ static int vmxnet3_get_int_state(QEMUFile *f, void *pv, size_t size)
return 0;
}
-static void vmxnet3_put_int_state(QEMUFile *f, void *pv, size_t size)
+static void vmxnet3_put_int_state(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field, QJSON *vmdesc)
{
Vmxnet3IntState *r = pv;
diff --git a/hw/nvram/eeprom93xx.c b/hw/nvram/eeprom93xx.c
index 2c16fc2..76d5f41 100644
--- a/hw/nvram/eeprom93xx.c
+++ b/hw/nvram/eeprom93xx.c
@@ -94,14 +94,16 @@ struct _eeprom_t {
This is a Big hack, but it is how the old state did it.
*/
-static int get_uint16_from_uint8(QEMUFile *f, void *pv, size_t size)
+static int get_uint16_from_uint8(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
uint16_t *v = pv;
*v = qemu_get_ubyte(f);
return 0;
}
-static void put_unused(QEMUFile *f, void *pv, size_t size)
+static void put_unused(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
fprintf(stderr, "uint16_from_uint8 is used only for backwards compatibility.\n");
fprintf(stderr, "Never should be used to write a new state.\n");
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index cdbdfb5..c294f9a 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -524,14 +524,16 @@ static void fw_cfg_reset(DeviceState *d)
Or we broke compatibility in the state, or we can't use struct tm
*/
-static int get_uint32_as_uint16(QEMUFile *f, void *pv, size_t size)
+static int get_uint32_as_uint16(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
uint32_t *v = pv;
*v = qemu_get_be16(f);
return 0;
}
-static void put_unused(QEMUFile *f, void *pv, size_t size)
+static void put_unused(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
fprintf(stderr, "uint32_as_uint16 is only used for backward compatibility.\n");
fprintf(stderr, "This functions shouldn't be called.\n");
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index b75f0e9..3838065 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -587,12 +587,14 @@ void msix_unset_vector_notifiers(PCIDevice *dev)
dev->msix_vector_poll_notifier = NULL;
}
-static void put_msix_state(QEMUFile *f, void *pv, size_t size)
+static void put_msix_state(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field, QJSON *vmdesc)
{
msix_save(pv, f);
}
-static int get_msix_state(QEMUFile *f, void *pv, size_t size)
+static int get_msix_state(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
msix_load(pv, f);
return 0;
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index bb605ef..75abae6 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -414,7 +414,8 @@ int pci_bus_numa_node(PCIBus *bus)
return PCI_BUS_GET_CLASS(bus)->numa_node(bus);
}
-static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
+static int get_pci_config_device(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
PCIDevice *s = container_of(pv, PCIDevice, config);
PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(s);
@@ -453,7 +454,8 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
}
/* just put buffer */
-static void put_pci_config_device(QEMUFile *f, void *pv, size_t size)
+static void put_pci_config_device(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field, QJSON *vmdesc)
{
const uint8_t **v = pv;
assert(size == pci_config_size(container_of(pv, PCIDevice, config)));
@@ -466,7 +468,8 @@ static VMStateInfo vmstate_info_pci_config = {
.put = put_pci_config_device,
};
-static int get_pci_irq_state(QEMUFile *f, void *pv, size_t size)
+static int get_pci_irq_state(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
PCIDevice *s = container_of(pv, PCIDevice, irq_state);
uint32_t irq_state[PCI_NUM_PINS];
@@ -487,7 +490,8 @@ static int get_pci_irq_state(QEMUFile *f, void *pv, size_t size)
return 0;
}
-static void put_pci_irq_state(QEMUFile *f, void *pv, size_t size)
+static void put_pci_irq_state(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field, QJSON *vmdesc)
{
int i;
PCIDevice *s = container_of(pv, PCIDevice, irq_state);
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 3dcd472..9f82aa6 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -695,13 +695,14 @@ void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
shpc_cap_update_dword(d);
}
-static void shpc_save(QEMUFile *f, void *pv, size_t size)
+static void shpc_save(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
PCIDevice *d = container_of(pv, PCIDevice, shpc);
qemu_put_buffer(f, d->shpc->config, SHPC_SIZEOF(d));
}
-static int shpc_load(QEMUFile *f, void *pv, size_t size)
+static int shpc_load(QEMUFile *f, void *pv, size_t size, VMStateField *field)
{
PCIDevice *d = container_of(pv, PCIDevice, shpc);
int ret = qemu_get_buffer(f, d->shpc->config, SHPC_SIZEOF(d));
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index ad6f398..ae2bfa2 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1906,7 +1906,8 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
/* SCSI request list. For simplicity, pv points to the whole device */
-static void put_scsi_requests(QEMUFile *f, void *pv, size_t size)
+static void put_scsi_requests(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field, QJSON *vmdesc)
{
SCSIDevice *s = pv;
SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, s->qdev.parent_bus);
@@ -1931,7 +1932,8 @@ static void put_scsi_requests(QEMUFile *f, void *pv, size_t size)
qemu_put_sbyte(f, 0);
}
-static int get_scsi_requests(QEMUFile *f, void *pv, size_t size)
+static int get_scsi_requests(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
SCSIDevice *s = pv;
SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, s->qdev.parent_bus);
diff --git a/hw/timer/twl92230.c b/hw/timer/twl92230.c
index 7ba4e9a..95eb7f3 100644
--- a/hw/timer/twl92230.c
+++ b/hw/timer/twl92230.c
@@ -747,14 +747,16 @@ static int menelaus_rx(I2CSlave *i2c)
Or we broke compatibility in the state, or we can't use struct tm
*/
-static int get_int32_as_uint16(QEMUFile *f, void *pv, size_t size)
+static int get_int32_as_uint16(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
int *v = pv;
*v = qemu_get_be16(f);
return 0;
}
-static void put_int32_as_uint16(QEMUFile *f, void *pv, size_t size)
+static void put_int32_as_uint16(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field, QJSON *vmdesc)
{
int *v = pv;
qemu_put_be16(f, *v);
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 8d80540..806af1b 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -2151,7 +2151,8 @@ static int usbredir_post_load(void *priv, int version_id)
}
/* For usbredirparser migration */
-static void usbredir_put_parser(QEMUFile *f, void *priv, size_t unused)
+static void usbredir_put_parser(QEMUFile *f, void *priv, size_t unused,
+ void *opaque, QJSON *vmdesc)
{
USBRedirDevice *dev = priv;
uint8_t *data;
@@ -2171,7 +2172,8 @@ static void usbredir_put_parser(QEMUFile *f, void *priv, size_t unused)
free(data);
}
-static int usbredir_get_parser(QEMUFile *f, void *priv, size_t unused)
+static int usbredir_get_parser(QEMUFile *f, void *priv, size_t unused,
+ void *opaque)
{
USBRedirDevice *dev = priv;
uint8_t *data;
@@ -2214,7 +2216,8 @@ static const VMStateInfo usbredir_parser_vmstate_info = {
/* For buffered packets (iso/irq) queue migration */
-static void usbredir_put_bufpq(QEMUFile *f, void *priv, size_t unused)
+static void usbredir_put_bufpq(QEMUFile *f, void *priv, size_t unused,
+ VMStateField *field, QJSON *vmdesc)
{
struct endp_data *endp = priv;
USBRedirDevice *dev = endp->dev;
@@ -2234,7 +2237,8 @@ static void usbredir_put_bufpq(QEMUFile *f, void *priv, size_t unused)
assert(i == endp->bufpq_size);
}
-static int usbredir_get_bufpq(QEMUFile *f, void *priv, size_t unused)
+static int usbredir_get_bufpq(QEMUFile *f, void *priv, size_t unused,
+ VMStateField *field)
{
struct endp_data *endp = priv;
USBRedirDevice *dev = endp->dev;
@@ -2337,7 +2341,8 @@ static const VMStateDescription usbredir_ep_vmstate = {
/* For PacketIdQueue migration */
-static void usbredir_put_packet_id_q(QEMUFile *f, void *priv, size_t unused)
+static void usbredir_put_packet_id_q(QEMUFile *f, void *priv, size_t unused,
+ VMStateField *field, QJSON *vmdesc)
{
struct PacketIdQueue *q = priv;
USBRedirDevice *dev = q->dev;
@@ -2353,7 +2358,8 @@ static void usbredir_put_packet_id_q(QEMUFile *f, void *priv, size_t unused)
assert(remain == 0);
}
-static int usbredir_get_packet_id_q(QEMUFile *f, void *priv, size_t unused)
+static int usbredir_get_packet_id_q(QEMUFile *f, void *priv, size_t unused,
+ VMStateField *field)
{
struct PacketIdQueue *q = priv;
USBRedirDevice *dev = q->dev;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index bfedbbf..79914c2 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -108,7 +108,8 @@ static bool virtio_pci_has_extra_state(DeviceState *d)
return proxy->flags & VIRTIO_PCI_FLAG_MIGRATE_EXTRA;
}
-static int get_virtio_pci_modern_state(QEMUFile *f, void *pv, size_t size)
+static int get_virtio_pci_modern_state(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
VirtIOPCIProxy *proxy = pv;
int i;
@@ -137,7 +138,8 @@ static void virtio_pci_save_modern_queue_state(VirtIOPCIQueue *vq,
qemu_put_be32(f, vq->used[1]);
}
-static void put_virtio_pci_modern_state(QEMUFile *f, void *pv, size_t size)
+static void put_virtio_pci_modern_state(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field, QJSON *vmdesc)
{
VirtIOPCIProxy *proxy = pv;
int i;
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 30ede3d..b5e0e54 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1314,7 +1314,8 @@ static const VMStateDescription vmstate_virtio_ringsize = {
}
};
-static int get_extra_state(QEMUFile *f, void *pv, size_t size)
+static int get_extra_state(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
VirtIODevice *vdev = pv;
BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
@@ -1327,7 +1328,8 @@ static int get_extra_state(QEMUFile *f, void *pv, size_t size)
}
}
-static void put_extra_state(QEMUFile *f, void *pv, size_t size)
+static void put_extra_state(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field, QJSON *vmdesc)
{
VirtIODevice *vdev = pv;
BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 6c65811..56a4171 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -80,11 +80,13 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque);
typedef struct VMStateInfo VMStateInfo;
typedef struct VMStateDescription VMStateDescription;
+typedef struct VMStateField VMStateField;
struct VMStateInfo {
const char *name;
- int (*get)(QEMUFile *f, void *pv, size_t size);
- void (*put)(QEMUFile *f, void *pv, size_t size);
+ int (*get)(QEMUFile *f, void *pv, size_t size, VMStateField *field);
+ void (*put)(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc);
};
enum VMStateFlags {
@@ -185,7 +187,7 @@ enum VMStateFlags {
VMS_MULTIPLY_ELEMENTS = 0x4000,
};
-typedef struct {
+struct VMStateField {
const char *name;
size_t offset;
size_t size;
@@ -198,7 +200,7 @@ typedef struct {
const VMStateDescription *vmsd;
int version_id;
bool (*field_exists)(void *opaque, int version_id);
-} VMStateField;
+};
struct VMStateDescription {
const char *name;
diff --git a/migration/savevm.c b/migration/savevm.c
index 6c21231..099dff0 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -219,14 +219,15 @@ void timer_get(QEMUFile *f, QEMUTimer *ts)
* Not in vmstate.c to not add qemu-timer.c as dependency to vmstate.c
*/
-static int get_timer(QEMUFile *f, void *pv, size_t size)
+static int get_timer(QEMUFile *f, void *pv, size_t size, VMStateField *field)
{
QEMUTimer *v = pv;
timer_get(f, v);
return 0;
}
-static void put_timer(QEMUFile *f, void *pv, size_t size)
+static void put_timer(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
QEMUTimer *v = pv;
timer_put(f, v);
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 46dc55e..644ba1f 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -121,7 +121,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
ret = vmstate_load_state(f, field->vmsd, addr,
field->vmsd->version_id);
} else {
- ret = field->info->get(f, addr, size);
+ ret = field->info->get(f, addr, size, NULL);
}
if (ret >= 0) {
@@ -327,7 +327,7 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
if (field->flags & VMS_STRUCT) {
vmstate_save_state(f, field->vmsd, addr, vmdesc_loop);
} else {
- field->info->put(f, addr, size);
+ field->info->put(f, addr, size, NULL, NULL);
}
written_bytes = qemu_ftell_fast(f) - old_offset;
@@ -460,14 +460,15 @@ static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
/* bool */
-static int get_bool(QEMUFile *f, void *pv, size_t size)
+static int get_bool(QEMUFile *f, void *pv, size_t size, VMStateField *field)
{
bool *v = pv;
*v = qemu_get_byte(f);
return 0;
}
-static void put_bool(QEMUFile *f, void *pv, size_t size)
+static void put_bool(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
bool *v = pv;
qemu_put_byte(f, *v);
@@ -481,14 +482,15 @@ const VMStateInfo vmstate_info_bool = {
/* 8 bit int */
-static int get_int8(QEMUFile *f, void *pv, size_t size)
+static int get_int8(QEMUFile *f, void *pv, size_t size, VMStateField *field)
{
int8_t *v = pv;
qemu_get_s8s(f, v);
return 0;
}
-static void put_int8(QEMUFile *f, void *pv, size_t size)
+static void put_int8(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
int8_t *v = pv;
qemu_put_s8s(f, v);
@@ -502,14 +504,15 @@ const VMStateInfo vmstate_info_int8 = {
/* 16 bit int */
-static int get_int16(QEMUFile *f, void *pv, size_t size)
+static int get_int16(QEMUFile *f, void *pv, size_t size, VMStateField *field)
{
int16_t *v = pv;
qemu_get_sbe16s(f, v);
return 0;
}
-static void put_int16(QEMUFile *f, void *pv, size_t size)
+static void put_int16(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
int16_t *v = pv;
qemu_put_sbe16s(f, v);
@@ -523,14 +526,15 @@ const VMStateInfo vmstate_info_int16 = {
/* 32 bit int */
-static int get_int32(QEMUFile *f, void *pv, size_t size)
+static int get_int32(QEMUFile *f, void *pv, size_t size, VMStateField *field)
{
int32_t *v = pv;
qemu_get_sbe32s(f, v);
return 0;
}
-static void put_int32(QEMUFile *f, void *pv, size_t size)
+static void put_int32(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
int32_t *v = pv;
qemu_put_sbe32s(f, v);
@@ -545,7 +549,8 @@ const VMStateInfo vmstate_info_int32 = {
/* 32 bit int. See that the received value is the same than the one
in the field */
-static int get_int32_equal(QEMUFile *f, void *pv, size_t size)
+static int get_int32_equal(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
int32_t *v = pv;
int32_t v2;
@@ -567,7 +572,7 @@ const VMStateInfo vmstate_info_int32_equal = {
* and less than or equal to the one in the field.
*/
-static int get_int32_le(QEMUFile *f, void *pv, size_t size)
+static int get_int32_le(QEMUFile *f, void *pv, size_t size, VMStateField *field)
{
int32_t *cur = pv;
int32_t loaded;
@@ -588,14 +593,15 @@ const VMStateInfo vmstate_info_int32_le = {
/* 64 bit int */
-static int get_int64(QEMUFile *f, void *pv, size_t size)
+static int get_int64(QEMUFile *f, void *pv, size_t size, VMStateField *field)
{
int64_t *v = pv;
qemu_get_sbe64s(f, v);
return 0;
}
-static void put_int64(QEMUFile *f, void *pv, size_t size)
+static void put_int64(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
int64_t *v = pv;
qemu_put_sbe64s(f, v);
@@ -609,14 +615,15 @@ const VMStateInfo vmstate_info_int64 = {
/* 8 bit unsigned int */
-static int get_uint8(QEMUFile *f, void *pv, size_t size)
+static int get_uint8(QEMUFile *f, void *pv, size_t size, VMStateField *field)
{
uint8_t *v = pv;
qemu_get_8s(f, v);
return 0;
}
-static void put_uint8(QEMUFile *f, void *pv, size_t size)
+static void put_uint8(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
uint8_t *v = pv;
qemu_put_8s(f, v);
@@ -630,14 +637,15 @@ const VMStateInfo vmstate_info_uint8 = {
/* 16 bit unsigned int */
-static int get_uint16(QEMUFile *f, void *pv, size_t size)
+static int get_uint16(QEMUFile *f, void *pv, size_t size, VMStateField *field)
{
uint16_t *v = pv;
qemu_get_be16s(f, v);
return 0;
}
-static void put_uint16(QEMUFile *f, void *pv, size_t size)
+static void put_uint16(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
uint16_t *v = pv;
qemu_put_be16s(f, v);
@@ -651,14 +659,15 @@ const VMStateInfo vmstate_info_uint16 = {
/* 32 bit unsigned int */
-static int get_uint32(QEMUFile *f, void *pv, size_t size)
+static int get_uint32(QEMUFile *f, void *pv, size_t size, VMStateField *field)
{
uint32_t *v = pv;
qemu_get_be32s(f, v);
return 0;
}
-static void put_uint32(QEMUFile *f, void *pv, size_t size)
+static void put_uint32(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
uint32_t *v = pv;
qemu_put_be32s(f, v);
@@ -673,7 +682,8 @@ const VMStateInfo vmstate_info_uint32 = {
/* 32 bit uint. See that the received value is the same than the one
in the field */
-static int get_uint32_equal(QEMUFile *f, void *pv, size_t size)
+static int get_uint32_equal(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
uint32_t *v = pv;
uint32_t v2;
@@ -693,14 +703,15 @@ const VMStateInfo vmstate_info_uint32_equal = {
/* 64 bit unsigned int */
-static int get_uint64(QEMUFile *f, void *pv, size_t size)
+static int get_uint64(QEMUFile *f, void *pv, size_t size, VMStateField *field)
{
uint64_t *v = pv;
qemu_get_be64s(f, v);
return 0;
}
-static void put_uint64(QEMUFile *f, void *pv, size_t size)
+static void put_uint64(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
uint64_t *v = pv;
qemu_put_be64s(f, v);
@@ -715,7 +726,8 @@ const VMStateInfo vmstate_info_uint64 = {
/* 64 bit unsigned int. See that the received value is the same than the one
in the field */
-static int get_uint64_equal(QEMUFile *f, void *pv, size_t size)
+static int get_uint64_equal(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
uint64_t *v = pv;
uint64_t v2;
@@ -736,7 +748,8 @@ const VMStateInfo vmstate_info_uint64_equal = {
/* 8 bit int. See that the received value is the same than the one
in the field */
-static int get_uint8_equal(QEMUFile *f, void *pv, size_t size)
+static int get_uint8_equal(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
uint8_t *v = pv;
uint8_t v2;
@@ -757,7 +770,8 @@ const VMStateInfo vmstate_info_uint8_equal = {
/* 16 bit unsigned int int. See that the received value is the same than the one
in the field */
-static int get_uint16_equal(QEMUFile *f, void *pv, size_t size)
+static int get_uint16_equal(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
uint16_t *v = pv;
uint16_t v2;
@@ -777,7 +791,8 @@ const VMStateInfo vmstate_info_uint16_equal = {
/* floating point */
-static int get_float64(QEMUFile *f, void *pv, size_t size)
+static int get_float64(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
float64 *v = pv;
@@ -785,7 +800,8 @@ static int get_float64(QEMUFile *f, void *pv, size_t size)
return 0;
}
-static void put_float64(QEMUFile *f, void *pv, size_t size)
+static void put_float64(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
uint64_t *v = pv;
@@ -800,7 +816,8 @@ const VMStateInfo vmstate_info_float64 = {
/* CPU_DoubleU type */
-static int get_cpudouble(QEMUFile *f, void *pv, size_t size)
+static int get_cpudouble(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
CPU_DoubleU *v = pv;
qemu_get_be32s(f, &v->l.upper);
@@ -808,7 +825,8 @@ static int get_cpudouble(QEMUFile *f, void *pv, size_t size)
return 0;
}
-static void put_cpudouble(QEMUFile *f, void *pv, size_t size)
+static void put_cpudouble(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field, QJSON *vmdesc)
{
CPU_DoubleU *v = pv;
qemu_put_be32s(f, &v->l.upper);
@@ -823,14 +841,16 @@ const VMStateInfo vmstate_info_cpudouble = {
/* uint8_t buffers */
-static int get_buffer(QEMUFile *f, void *pv, size_t size)
+static int get_buffer(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
uint8_t *v = pv;
qemu_get_buffer(f, v, size);
return 0;
}
-static void put_buffer(QEMUFile *f, void *pv, size_t size)
+static void put_buffer(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
uint8_t *v = pv;
qemu_put_buffer(f, v, size);
@@ -845,7 +865,8 @@ const VMStateInfo vmstate_info_buffer = {
/* unused buffers: space that was used for some fields that are
not useful anymore */
-static int get_unused_buffer(QEMUFile *f, void *pv, size_t size)
+static int get_unused_buffer(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
uint8_t buf[1024];
int block_len;
@@ -858,7 +879,8 @@ static int get_unused_buffer(QEMUFile *f, void *pv, size_t size)
return 0;
}
-static void put_unused_buffer(QEMUFile *f, void *pv, size_t size)
+static void put_unused_buffer(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field, QJSON *vmdesc)
{
static const uint8_t buf[1024];
int block_len;
@@ -883,7 +905,7 @@ const VMStateInfo vmstate_info_unused_buffer = {
*/
/* This is the number of 64 bit words sent over the wire */
#define BITS_TO_U64S(nr) DIV_ROUND_UP(nr, 64)
-static int get_bitmap(QEMUFile *f, void *pv, size_t size)
+static int get_bitmap(QEMUFile *f, void *pv, size_t size, VMStateField *field)
{
unsigned long *bmp = pv;
int i, idx = 0;
@@ -897,7 +919,8 @@ static int get_bitmap(QEMUFile *f, void *pv, size_t size)
return 0;
}
-static void put_bitmap(QEMUFile *f, void *pv, size_t size)
+static void put_bitmap(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
unsigned long *bmp = pv;
int i, idx = 0;
diff --git a/target-alpha/machine.c b/target-alpha/machine.c
index 710b783..48e3278 100644
--- a/target-alpha/machine.c
+++ b/target-alpha/machine.c
@@ -5,14 +5,15 @@
#include "hw/boards.h"
#include "migration/cpu.h"
-static int get_fpcr(QEMUFile *f, void *opaque, size_t size)
+static int get_fpcr(QEMUFile *f, void *opaque, size_t size, VMStateField *field)
{
CPUAlphaState *env = opaque;
cpu_alpha_store_fpcr(env, qemu_get_be64(f));
return 0;
}
-static void put_fpcr(QEMUFile *f, void *opaque, size_t size)
+static void put_fpcr(QEMUFile *f, void *opaque, size_t size,
+ VMStateField *field, QJSON *vmdesc)
{
CPUAlphaState *env = opaque;
qemu_put_be64(f, cpu_alpha_load_fpcr(env));
diff --git a/target-arm/machine.c b/target-arm/machine.c
index 2f45260..6815fd8 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -17,7 +17,8 @@ static bool vfp_needed(void *opaque)
return arm_feature(env, ARM_FEATURE_VFP);
}
-static int get_fpscr(QEMUFile *f, void *opaque, size_t size)
+static int get_fpscr(QEMUFile *f, void *opaque, size_t size,
+ VMStateField *field)
{
ARMCPU *cpu = opaque;
CPUARMState *env = &cpu->env;
@@ -27,7 +28,8 @@ static int get_fpscr(QEMUFile *f, void *opaque, size_t size)
return 0;
}
-static void put_fpscr(QEMUFile *f, void *opaque, size_t size)
+static void put_fpscr(QEMUFile *f, void *opaque, size_t size,
+ VMStateField *field, QJSON *vmdesc)
{
ARMCPU *cpu = opaque;
CPUARMState *env = &cpu->env;
@@ -163,7 +165,8 @@ static const VMStateDescription vmstate_pmsav7 = {
}
};
-static int get_cpsr(QEMUFile *f, void *opaque, size_t size)
+static int get_cpsr(QEMUFile *f, void *opaque, size_t size,
+ VMStateField *field)
{
ARMCPU *cpu = opaque;
CPUARMState *env = &cpu->env;
@@ -180,7 +183,8 @@ static int get_cpsr(QEMUFile *f, void *opaque, size_t size)
return 0;
}
-static void put_cpsr(QEMUFile *f, void *opaque, size_t size)
+static void put_cpsr(QEMUFile *f, void *opaque, size_t size,
+ VMStateField *field, QJSON *vmdesc)
{
ARMCPU *cpu = opaque;
CPUARMState *env = &cpu->env;
diff --git a/target-i386/machine.c b/target-i386/machine.c
index cb9adf2..e5279e7 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -139,7 +139,8 @@ static const VMStateDescription vmstate_mtrr_var = {
#define VMSTATE_MTRR_VARS(_field, _state, _n, _v) \
VMSTATE_STRUCT_ARRAY(_field, _state, _n, _v, vmstate_mtrr_var, MTRRVar)
-static void put_fpreg_error(QEMUFile *f, void *opaque, size_t size)
+static void put_fpreg_error(QEMUFile *f, void *opaque, size_t size,
+ VMStateField *field, QJSON *vmdesc)
{
fprintf(stderr, "call put_fpreg() with invalid arguments\n");
exit(0);
@@ -167,7 +168,8 @@ static void fp64_to_fp80(union x86_longdouble *p, uint64_t temp)
p->exp = e;
}
-static int get_fpreg(QEMUFile *f, void *opaque, size_t size)
+static int get_fpreg(QEMUFile *f, void *opaque, size_t size,
+ VMStateField *field)
{
FPReg *fp_reg = opaque;
uint64_t mant;
@@ -179,7 +181,8 @@ static int get_fpreg(QEMUFile *f, void *opaque, size_t size)
return 0;
}
-static void put_fpreg(QEMUFile *f, void *opaque, size_t size)
+static void put_fpreg(QEMUFile *f, void *opaque, size_t size,
+ VMStateField *field, QJSON *vmdesc)
{
FPReg *fp_reg = opaque;
uint64_t mant;
@@ -197,7 +200,8 @@ static const VMStateInfo vmstate_fpreg = {
.put = put_fpreg,
};
-static int get_fpreg_1_mmx(QEMUFile *f, void *opaque, size_t size)
+static int get_fpreg_1_mmx(QEMUFile *f, void *opaque, size_t size,
+ VMStateField *field)
{
union x86_longdouble *p = opaque;
uint64_t mant;
@@ -214,7 +218,8 @@ static const VMStateInfo vmstate_fpreg_1_mmx = {
.put = put_fpreg_error,
};
-static int get_fpreg_1_no_mmx(QEMUFile *f, void *opaque, size_t size)
+static int get_fpreg_1_no_mmx(QEMUFile *f, void *opaque, size_t size,
+ VMStateField *field)
{
union x86_longdouble *p = opaque;
uint64_t mant;
@@ -276,14 +281,16 @@ static bool less_than_7(void *opaque, int version_id)
return version_id < 7;
}
-static int get_uint64_as_uint32(QEMUFile *f, void *pv, size_t size)
+static int get_uint64_as_uint32(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
uint64_t *v = pv;
*v = qemu_get_be32(f);
return 0;
}
-static void put_uint64_as_uint32(QEMUFile *f, void *pv, size_t size)
+static void put_uint64_as_uint32(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field, QJSON *vmdesc)
{
uint64_t *v = pv;
qemu_put_be32(f, *v);
diff --git a/target-mips/machine.c b/target-mips/machine.c
index 7314cfe..ebc984e 100644
--- a/target-mips/machine.c
+++ b/target-mips/machine.c
@@ -20,7 +20,7 @@ static int cpu_post_load(void *opaque, int version_id)
/* FPU state */
-static int get_fpr(QEMUFile *f, void *pv, size_t size)
+static int get_fpr(QEMUFile *f, void *pv, size_t size, VMStateField *field)
{
int i;
fpr_t *v = pv;
@@ -31,7 +31,8 @@ static int get_fpr(QEMUFile *f, void *pv, size_t size)
return 0;
}
-static void put_fpr(QEMUFile *f, void *pv, size_t size)
+static void put_fpr(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
int i;
fpr_t *v = pv;
@@ -125,7 +126,7 @@ const VMStateDescription vmstate_mvp = {
/* TLB state */
-static int get_tlb(QEMUFile *f, void *pv, size_t size)
+static int get_tlb(QEMUFile *f, void *pv, size_t size, VMStateField *field)
{
r4k_tlb_t *v = pv;
uint16_t flags;
@@ -152,7 +153,8 @@ static int get_tlb(QEMUFile *f, void *pv, size_t size)
return 0;
}
-static void put_tlb(QEMUFile *f, void *pv, size_t size)
+static void put_tlb(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
r4k_tlb_t *v = pv;
diff --git a/target-ppc/machine.c b/target-ppc/machine.c
index f6c7256..0790f2e 100644
--- a/target-ppc/machine.c
+++ b/target-ppc/machine.c
@@ -103,7 +103,7 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
return 0;
}
-static int get_avr(QEMUFile *f, void *pv, size_t size)
+static int get_avr(QEMUFile *f, void *pv, size_t size, VMStateField *field)
{
ppc_avr_t *v = pv;
@@ -113,7 +113,8 @@ static int get_avr(QEMUFile *f, void *pv, size_t size)
return 0;
}
-static void put_avr(QEMUFile *f, void *pv, size_t size)
+static void put_avr(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
ppc_avr_t *v = pv;
@@ -321,7 +322,7 @@ static const VMStateDescription vmstate_sr = {
};
#ifdef TARGET_PPC64
-static int get_slbe(QEMUFile *f, void *pv, size_t size)
+static int get_slbe(QEMUFile *f, void *pv, size_t size, VMStateField *field)
{
ppc_slb_t *v = pv;
@@ -331,7 +332,8 @@ static int get_slbe(QEMUFile *f, void *pv, size_t size)
return 0;
}
-static void put_slbe(QEMUFile *f, void *pv, size_t size)
+static void put_slbe(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
ppc_slb_t *v = pv;
diff --git a/target-sparc/machine.c b/target-sparc/machine.c
index 59c92f7..3194e03 100644
--- a/target-sparc/machine.c
+++ b/target-sparc/machine.c
@@ -59,7 +59,7 @@ static const VMStateDescription vmstate_tlb_entry = {
};
#endif
-static int get_psr(QEMUFile *f, void *opaque, size_t size)
+static int get_psr(QEMUFile *f, void *opaque, size_t size, VMStateField *field)
{
SPARCCPU *cpu = opaque;
CPUSPARCState *env = &cpu->env;
@@ -72,7 +72,8 @@ static int get_psr(QEMUFile *f, void *opaque, size_t size)
return 0;
}
-static void put_psr(QEMUFile *f, void *opaque, size_t size)
+static void put_psr(QEMUFile *f, void *opaque, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
SPARCCPU *cpu = opaque;
CPUSPARCState *env = &cpu->env;
--
1.9.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
2016-05-31 18:02 [Qemu-devel] [QEMU RFC PATCH v3 0/6]Migration: ensure hotplug and migration work together Jianjun Duan
` (2 preceding siblings ...)
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 3/6] Migration: extend VMStateInfo Jianjun Duan
@ 2016-05-31 18:02 ` Jianjun Duan
2016-05-31 19:54 ` Paolo Bonzini
` (2 more replies)
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 5/6] Migration: migrate ccs_list in spapr state Jianjun Duan
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 6/6] Migration: migrate pending_events of " Jianjun Duan
5 siblings, 3 replies; 27+ messages in thread
From: Jianjun Duan @ 2016-05-31 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, duanj, dmitry, peter.maydell, kraxel, mst, david,
pbonzini, veroniabahaa, quintela, amit.shah, mreitz, kwolf, rth,
aurelien, leon.alrae, blauwirbel, mark.cave-ayland, mdroth
Currently we cannot directly transfer a QTAILQ instance because of the
limitation in the migration code. Here we introduce an approach to
transfer such structures. In our approach such a structure is tagged
with VMS_CSTM. We then modified vmstate_save_state and vmstate_load_state
so that when VMS_CSTM is encountered, put and get from VMStateInfo are
called respectively. This approach will be used to transfer pending_events
and ccs_list in spapr state.
We also create some macros in qemu/queue.h to access a QTAILQ using pointer
arithmetic. This ensures that we do not depend on the implementation
details about QTAILQ in the migration code.
Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
---
include/migration/vmstate.h | 22 +++++++++++++
include/qemu/queue.h | 32 ++++++++++++++++++
migration/vmstate.c | 79 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 133 insertions(+)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 56a4171..da4ef7f 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -185,6 +185,8 @@ enum VMStateFlags {
* to determine the number of entries in the array. Only valid in
* combination with one of VMS_VARRAY*. */
VMS_MULTIPLY_ELEMENTS = 0x4000,
+ /* For fields which need customized handling, such as QTAILQ in queue.h*/
+ VMS_CSTM = 0x8000,
};
struct VMStateField {
@@ -245,6 +247,7 @@ extern const VMStateInfo vmstate_info_timer;
extern const VMStateInfo vmstate_info_buffer;
extern const VMStateInfo vmstate_info_unused_buffer;
extern const VMStateInfo vmstate_info_bitmap;
+extern const VMStateInfo vmstate_info_qtailq;
#define type_check_2darray(t1,t2,n,m) ((t1(*)[n][m])0 - (t2*)0)
#define type_check_array(t1,t2,n) ((t1(*)[n])0 - (t2*)0)
@@ -656,6 +659,25 @@ extern const VMStateInfo vmstate_info_bitmap;
.offset = offsetof(_state, _field), \
}
+/* For QTAILQ that need customized handling
+ * _type: type of QTAILQ element
+ * _next: name of QTAILQ entry field in QTAILQ element
+ * _vmsd: VMSD for QTAILQ element
+ * size: size of QTAILQ element
+ * start: offset of QTAILQ entry in QTAILQ element
+ */
+#define VMSTATE_QTAILQ_V(_field, _state, _version, _vmsd, _type, _next) \
+{ \
+ .name = (stringify(_field)), \
+ .version_id = (_version), \
+ .vmsd = &(_vmsd), \
+ .size = sizeof(_type), \
+ .info = &vmstate_info_qtailq, \
+ .flags = VMS_CSTM, \
+ .offset = offsetof(_state, _field), \
+ .start = offsetof(_type, _next), \
+}
+
/* _f : field name
_f_n : num of elements field_name
_n : num of elements
diff --git a/include/qemu/queue.h b/include/qemu/queue.h
index f781aa2..003e368 100644
--- a/include/qemu/queue.h
+++ b/include/qemu/queue.h
@@ -437,3 +437,35 @@ struct { \
(*(((struct headname *)((elm)->field.tqe_prev))->tqh_last))
#endif /* !QEMU_SYS_QUEUE_H_ */
+
+/*
+ * Offsets of layout of a tail queue head.
+ */
+#define QTAILQ_FIRST_OFFSET 0
+#define QTAILQ_LAST_OFFSET (sizeof(void *))
+
+/*
+ * Offsets of layout of a tail queue element.
+ */
+#define QTAILQ_NEXT_OFFSET 0
+#define QTAILQ_PREV_OFFSET (sizeof(void *))
+
+/*
+ * Tail queue tranversal using pointer arithmetic.
+ */
+#define QTAILQ_RAW_FOREACH(elm, head, entry) \
+ for ((elm) = *((void **) ((char *) (head) + QTAILQ_FIRST_OFFSET)); \
+ (elm); \
+ (elm) = \
+ *((void **) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET)))
+/*
+ * Tail queue insertion using pointer arithmetic.
+ */
+#define QTAILQ_RAW_INSERT_TAIL(head, elm, entry) do { \
+ *((void **) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET)) = NULL; \
+ *((void **) ((char *) (elm) + (entry) + QTAILQ_PREV_OFFSET)) = \
+ *((void **) ((char *) (head) +QTAILQ_LAST_OFFSET)); \
+ **((void ***)((char *) (head) +QTAILQ_LAST_OFFSET)) = (elm); \
+ *((void **) ((char *) (head) + QTAILQ_LAST_OFFSET)) = \
+ (void *) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET); \
+} while (/*CONSTCOND*/0)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 644ba1f..ff56650 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -5,7 +5,9 @@
#include "migration/vmstate.h"
#include "qemu/bitops.h"
#include "qemu/error-report.h"
+#include "qemu/queue.h"
#include "trace.h"
+#include "migration/qjson.h"
static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, QJSON *vmdesc);
@@ -120,6 +122,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
if (field->flags & VMS_STRUCT) {
ret = vmstate_load_state(f, field->vmsd, addr,
field->vmsd->version_id);
+ } else if (field->flags & VMS_CSTM) {
+ ret = field->info->get(f, addr, size, field);
} else {
ret = field->info->get(f, addr, size, NULL);
@@ -192,6 +196,8 @@ static const char *vmfield_get_type_name(VMStateField *field)
if (field->flags & VMS_STRUCT) {
type = "struct";
+ } else if (field->flags & VMS_CSTM) {
+ type = "customized";
} else if (field->info->name) {
type = field->info->name;
}
@@ -326,6 +332,8 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
}
if (field->flags & VMS_STRUCT) {
vmstate_save_state(f, field->vmsd, addr, vmdesc_loop);
+ } else if (field->flags & VMS_CSTM) {
+ field->info->put(f, addr, size, field, vmdesc_loop);
} else {
field->info->put(f, addr, size, NULL, NULL);
}
@@ -938,3 +946,74 @@ const VMStateInfo vmstate_info_bitmap = {
.get = get_bitmap,
.put = put_bitmap,
};
+
+/*get for QTAILQ */
+static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
+ VMStateField *field)
+{
+ bool link;
+ int ret = 0;
+ const VMStateDescription *vmsd = field->vmsd;
+ size_t size = field->size;
+ size_t entry = field->start;
+ int version_id = field->version_id;
+ void *elm;
+
+ trace_vmstate_load_state(vmsd->name, version_id);
+ if (version_id > vmsd->version_id) {
+ trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL);
+ return -EINVAL;
+ }
+ if (version_id < vmsd->minimum_version_id) {
+ trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
+ return -EINVAL;
+ }
+
+ while (true) {
+ vmstate_info_bool.get(f, &link, sizeof(bool), NULL);
+ if (!link) {
+ break;
+ }
+
+ elm = g_malloc(size);
+ ret = vmstate_load_state(f, vmsd, elm, version_id);
+ if (ret) {
+ return ret;
+ }
+ QTAILQ_RAW_INSERT_TAIL(pv, elm, entry);
+ }
+
+ trace_vmstate_load_state_end(vmsd->name, "end", ret);
+ return ret;
+}
+
+/* put for QTAILQ */
+static void put_qtailq(QEMUFile *f, void *pv, size_t unused_size,
+ VMStateField *field, QJSON *vmdesc)
+{
+ bool link = true;
+ const VMStateDescription *vmsd = field->vmsd;
+ size_t entry = field->start;
+ void *elm;
+
+ if (vmdesc) {
+ json_prop_str(vmdesc, "vmsd_name", vmsd->name);
+ json_prop_int(vmdesc, "version", vmsd->version_id);
+ json_start_array(vmdesc, "fields");
+ }
+
+ QTAILQ_RAW_FOREACH(elm, pv, entry) {
+ vmstate_info_bool.put(f, &link, sizeof(bool), NULL, NULL);
+ vmstate_save_state(f, vmsd, elm, vmdesc);
+ }
+ link = false;
+ vmstate_info_bool.put(f, &link, sizeof(bool), NULL, NULL);
+ if (vmdesc) {
+ json_end_array(vmdesc);
+ }
+}
+const VMStateInfo vmstate_info_qtailq = {
+ .name = "qtailq",
+ .get = get_qtailq,
+ .put = put_qtailq,
+};
--
1.9.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [QEMU RFC PATCH v3 5/6] Migration: migrate ccs_list in spapr state
2016-05-31 18:02 [Qemu-devel] [QEMU RFC PATCH v3 0/6]Migration: ensure hotplug and migration work together Jianjun Duan
` (3 preceding siblings ...)
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ Jianjun Duan
@ 2016-05-31 18:02 ` Jianjun Duan
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 6/6] Migration: migrate pending_events of " Jianjun Duan
5 siblings, 0 replies; 27+ messages in thread
From: Jianjun Duan @ 2016-05-31 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, duanj, dmitry, peter.maydell, kraxel, mst, david,
pbonzini, veroniabahaa, quintela, amit.shah, mreitz, kwolf, rth,
aurelien, leon.alrae, blauwirbel, mark.cave-ayland, mdroth
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.
ccs_list 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>
---
hw/ppc/spapr.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 44e401a..f13584c 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1266,6 +1266,36 @@ static bool version_before_3(void *opaque, int version_id)
return version_id < 3;
}
+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 = "spaprconfigureconnectorstate",
+ .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 = "spaprccslist",
+ .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 const VMStateDescription vmstate_spapr = {
.name = "spapr",
.version_id = 3,
@@ -1281,6 +1311,10 @@ static const VMStateDescription vmstate_spapr = {
VMSTATE_PPC_TIMEBASE_V(tb, sPAPRMachineState, 2),
VMSTATE_END_OF_LIST()
},
+ .subsections = (const VMStateDescription*[]) {
+ &vmstate_spapr_ccs_list,
+ NULL
+ }
};
static int htab_save_setup(QEMUFile *f, void *opaque)
--
1.9.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [QEMU RFC PATCH v3 6/6] Migration: migrate pending_events of spapr state
2016-05-31 18:02 [Qemu-devel] [QEMU RFC PATCH v3 0/6]Migration: ensure hotplug and migration work together Jianjun Duan
` (4 preceding siblings ...)
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 5/6] Migration: migrate ccs_list in spapr state Jianjun Duan
@ 2016-05-31 18:02 ` Jianjun Duan
5 siblings, 0 replies; 27+ messages in thread
From: Jianjun Duan @ 2016-05-31 18:02 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-ppc, duanj, dmitry, peter.maydell, kraxel, mst, david,
pbonzini, veroniabahaa, quintela, amit.shah, mreitz, kwolf, rth,
aurelien, leon.alrae, blauwirbel, mark.cave-ayland, mdroth
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>
---
hw/ppc/spapr.c | 33 +++++++++++++++++++++++++++++++++
hw/ppc/spapr_events.c | 22 +++++++++++++---------
include/hw/ppc/spapr.h | 3 ++-
3 files changed, 48 insertions(+), 10 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f13584c..b38cf04 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1266,12 +1266,32 @@ static bool version_before_3(void *opaque, int version_id)
return version_id < 3;
}
+static bool spapr_pending_events_needed(void *opaque)
+{
+ sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
+ return !QTAILQ_EMPTY(&spapr->pending_events);
+}
+
static bool spapr_ccs_list_needed(void *opaque)
{
sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
return !QTAILQ_EMPTY(&spapr->ccs_list);
}
+static const VMStateDescription vmstate_spapr_event_entry = {
+ .name = "spapreventlogentry",
+ .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_ccs = {
.name = "spaprconfigureconnectorstate",
.version_id = 1,
@@ -1284,6 +1304,18 @@ static const VMStateDescription vmstate_spapr_ccs = {
},
};
+static const VMStateDescription vmstate_spapr_pending_events = {
+ .name = "spaprpendingevents",
+ .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 const VMStateDescription vmstate_spapr_ccs_list = {
.name = "spaprccslist",
.version_id = 1,
@@ -1312,6 +1344,7 @@ static const VMStateDescription vmstate_spapr = {
VMSTATE_END_OF_LIST()
},
.subsections = (const VMStateDescription*[]) {
+ &vmstate_spapr_pending_events,
&vmstate_spapr_ccs_list,
NULL
}
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 049fb1b..1680c08 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -239,7 +239,8 @@ void spapr_events_fdt_skel(void *fdt, uint32_t check_exception_irq)
_FDT((fdt_end_node(fdt)));
}
-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);
@@ -248,6 +249,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);
}
@@ -350,6 +352,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;
@@ -358,13 +361,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 */);
@@ -384,7 +387,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(spapr->icp, spapr->check_exception_irq));
}
@@ -407,6 +410,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;
@@ -415,14 +419,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 */);
@@ -463,7 +467,7 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
hp->drc.index = cpu_to_be32(drc);
}
- 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(spapr->icp, spapr->check_exception_irq));
}
@@ -530,7 +534,7 @@ static void check_exception(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) {
@@ -580,7 +584,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 815d5ee..5ded8b1 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -552,7 +552,8 @@ sPAPRTCETable *spapr_tce_find_by_liobn(target_ulong liobn);
struct sPAPREventLogEntry {
int log_type;
bool exception;
- void *data;
+ uint32_t data_size;
+ uint8_t *data;
QTAILQ_ENTRY(sPAPREventLogEntry) next;
};
--
1.9.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ Jianjun Duan
@ 2016-05-31 19:54 ` Paolo Bonzini
2016-05-31 21:53 ` Jianjun Duan
2016-06-02 3:58 ` David Gibson
2016-06-02 15:01 ` Sascha Silbe
2 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2016-05-31 19:54 UTC (permalink / raw)
To: Jianjun Duan
Cc: qemu-devel, qemu-ppc, dmitry, peter maydell, kraxel, mst, david,
veroniabahaa, quintela, amit shah, mreitz, kwolf, rth, aurelien,
leon alrae, blauwirbel, mark cave-ayland, mdroth
----- Original Message -----
> From: "Jianjun Duan" <duanj@linux.vnet.ibm.com>
> To: qemu-devel@nongnu.org
> Cc: qemu-ppc@nongnu.org, duanj@linux.vnet.ibm.com, dmitry@daynix.com, "peter maydell" <peter.maydell@linaro.org>,
> kraxel@redhat.com, mst@redhat.com, david@gibson.dropbear.id.au, pbonzini@redhat.com, veroniabahaa@gmail.com,
> quintela@redhat.com, "amit shah" <amit.shah@redhat.com>, mreitz@redhat.com, kwolf@redhat.com, rth@twiddle.net,
> aurelien@aurel32.net, "leon alrae" <leon.alrae@imgtec.com>, blauwirbel@gmail.com, "mark cave-ayland"
> <mark.cave-ayland@ilande.co.uk>, mdroth@linux.vnet.ibm.com
> Sent: Tuesday, May 31, 2016 8:02:42 PM
> Subject: [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
>
> Currently we cannot directly transfer a QTAILQ instance because of the
> limitation in the migration code. Here we introduce an approach to
> transfer such structures. In our approach such a structure is tagged
> with VMS_CSTM. We then modified vmstate_save_state and vmstate_load_state
> so that when VMS_CSTM is encountered, put and get from VMStateInfo are
> called respectively. This approach will be used to transfer pending_events
> and ccs_list in spapr state.
>
> We also create some macros in qemu/queue.h to access a QTAILQ using pointer
> arithmetic. This ensures that we do not depend on the implementation
> details about QTAILQ in the migration code.
>
> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
> ---
> include/migration/vmstate.h | 22 +++++++++++++
> include/qemu/queue.h | 32 ++++++++++++++++++
> migration/vmstate.c | 79
> +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 133 insertions(+)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 56a4171..da4ef7f 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -185,6 +185,8 @@ enum VMStateFlags {
> * to determine the number of entries in the array. Only valid in
> * combination with one of VMS_VARRAY*. */
> VMS_MULTIPLY_ELEMENTS = 0x4000,
> + /* For fields which need customized handling, such as QTAILQ in
> queue.h*/
> + VMS_CSTM = 0x8000,
Please call this VMS_LINKED. It can be adapted to other data
structures in qemu/queue.h if there is a need later on.
> };
>
> struct VMStateField {
> @@ -245,6 +247,7 @@ extern const VMStateInfo vmstate_info_timer;
> extern const VMStateInfo vmstate_info_buffer;
> extern const VMStateInfo vmstate_info_unused_buffer;
> extern const VMStateInfo vmstate_info_bitmap;
> +extern const VMStateInfo vmstate_info_qtailq;
>
> #define type_check_2darray(t1,t2,n,m) ((t1(*)[n][m])0 - (t2*)0)
> #define type_check_array(t1,t2,n) ((t1(*)[n])0 - (t2*)0)
> @@ -656,6 +659,25 @@ extern const VMStateInfo vmstate_info_bitmap;
> .offset = offsetof(_state, _field), \
> }
>
> +/* For QTAILQ that need customized handling
> + * _type: type of QTAILQ element
> + * _next: name of QTAILQ entry field in QTAILQ element
> + * _vmsd: VMSD for QTAILQ element
> + * size: size of QTAILQ element
> + * start: offset of QTAILQ entry in QTAILQ element
> + */
> +#define VMSTATE_QTAILQ_V(_field, _state, _version, _vmsd, _type, _next) \
> +{ \
> + .name = (stringify(_field)), \
> + .version_id = (_version), \
> + .vmsd = &(_vmsd), \
> + .size = sizeof(_type), \
> + .info = &vmstate_info_qtailq, \
> + .flags = VMS_CSTM, \
> + .offset = offsetof(_state, _field), \
> + .start = offsetof(_type, _next), \
> +}
> +
> /* _f : field name
> _f_n : num of elements field_name
> _n : num of elements
> diff --git a/include/qemu/queue.h b/include/qemu/queue.h
> index f781aa2..003e368 100644
> --- a/include/qemu/queue.h
> +++ b/include/qemu/queue.h
> @@ -437,3 +437,35 @@ struct {
> \
> (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last))
>
> #endif /* !QEMU_SYS_QUEUE_H_ */
> +
> +/*
> + * Offsets of layout of a tail queue head.
> + */
> +#define QTAILQ_FIRST_OFFSET 0
> +#define QTAILQ_LAST_OFFSET (sizeof(void *))
> +
> +/*
> + * Offsets of layout of a tail queue element.
> + */
> +#define QTAILQ_NEXT_OFFSET 0
> +#define QTAILQ_PREV_OFFSET (sizeof(void *))
> +
> +/*
> + * Tail queue tranversal using pointer arithmetic.
> + */
> +#define QTAILQ_RAW_FOREACH(elm, head, entry)
> \
> + for ((elm) = *((void **) ((char *) (head) + QTAILQ_FIRST_OFFSET));
> \
> + (elm);
> \
> + (elm) =
> \
> + *((void **) ((char *) (elm) + (entry) +
> QTAILQ_NEXT_OFFSET)))
> +/*
> + * Tail queue insertion using pointer arithmetic.
> + */
> +#define QTAILQ_RAW_INSERT_TAIL(head, elm, entry) do {
> \
> + *((void **) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET)) = NULL;
> \
> + *((void **) ((char *) (elm) + (entry) + QTAILQ_PREV_OFFSET)) =
> \
> + *((void **) ((char *) (head) +QTAILQ_LAST_OFFSET));
> \
> + **((void ***)((char *) (head) +QTAILQ_LAST_OFFSET)) = (elm);
> \
> + *((void **) ((char *) (head) + QTAILQ_LAST_OFFSET)) =
> \
> + (void *) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET);
> \
> +} while (/*CONSTCOND*/0)
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 644ba1f..ff56650 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -5,7 +5,9 @@
> #include "migration/vmstate.h"
> #include "qemu/bitops.h"
> #include "qemu/error-report.h"
> +#include "qemu/queue.h"
> #include "trace.h"
> +#include "migration/qjson.h"
>
> static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription
> *vmsd,
> void *opaque, QJSON *vmdesc);
> @@ -120,6 +122,8 @@ int vmstate_load_state(QEMUFile *f, const
> VMStateDescription *vmsd,
> if (field->flags & VMS_STRUCT) {
> ret = vmstate_load_state(f, field->vmsd, addr,
> field->vmsd->version_id);
> + } else if (field->flags & VMS_CSTM) {
> + ret = field->info->get(f, addr, size, field);
> } else {
> ret = field->info->get(f, addr, size, NULL);
>
> @@ -192,6 +196,8 @@ static const char *vmfield_get_type_name(VMStateField
> *field)
>
> if (field->flags & VMS_STRUCT) {
> type = "struct";
> + } else if (field->flags & VMS_CSTM) {
> + type = "customized";
> } else if (field->info->name) {
> type = field->info->name;
> }
> @@ -326,6 +332,8 @@ void vmstate_save_state(QEMUFile *f, const
> VMStateDescription *vmsd,
> }
> if (field->flags & VMS_STRUCT) {
> vmstate_save_state(f, field->vmsd, addr, vmdesc_loop);
> + } else if (field->flags & VMS_CSTM) {
> + field->info->put(f, addr, size, field, vmdesc_loop);
> } else {
> field->info->put(f, addr, size, NULL, NULL);
> }
> @@ -938,3 +946,74 @@ const VMStateInfo vmstate_info_bitmap = {
> .get = get_bitmap,
> .put = put_bitmap,
> };
> +
> +/*get for QTAILQ */
> +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
> + VMStateField *field)
> +{
> + bool link;
> + int ret = 0;
> + const VMStateDescription *vmsd = field->vmsd;
> + size_t size = field->size;
> + size_t entry = field->start;
> + int version_id = field->version_id;
> + void *elm;
> +
> + trace_vmstate_load_state(vmsd->name, version_id);
> + if (version_id > vmsd->version_id) {
> + trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL);
> + return -EINVAL;
> + }
> + if (version_id < vmsd->minimum_version_id) {
> + trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
> + return -EINVAL;
> + }
> +
> + while (true) {
> + vmstate_info_bool.get(f, &link, sizeof(bool), NULL);
You can use qemu_get_byte here, and likewise qemu_put_byte below in
put_qtailq.
> + if (!link) {
> + break;
> + }
> +
> + elm = g_malloc(size);
> + ret = vmstate_load_state(f, vmsd, elm, version_id);
> + if (ret) {
> + return ret;
> + }
> + QTAILQ_RAW_INSERT_TAIL(pv, elm, entry);
> + }
> +
> + trace_vmstate_load_state_end(vmsd->name, "end", ret);
> + return ret;
> +}
> +
> +/* put for QTAILQ */
> +static void put_qtailq(QEMUFile *f, void *pv, size_t unused_size,
> + VMStateField *field, QJSON *vmdesc)
> +{
> + bool link = true;
> + const VMStateDescription *vmsd = field->vmsd;
> + size_t entry = field->start;
> + void *elm;
> +
> + if (vmdesc) {
> + json_prop_str(vmdesc, "vmsd_name", vmsd->name);
> + json_prop_int(vmdesc, "version", vmsd->version_id);
> + json_start_array(vmdesc, "fields");
You need to store the fields exactly once here, even if there are
0 or >1 elements.
Otherwise looks great now!
Paolo
> + }
> +
> + QTAILQ_RAW_FOREACH(elm, pv, entry) {
> + vmstate_info_bool.put(f, &link, sizeof(bool), NULL, NULL);
> + vmstate_save_state(f, vmsd, elm, vmdesc);
> + }
> + link = false;
> + vmstate_info_bool.put(f, &link, sizeof(bool), NULL, NULL);
> + if (vmdesc) {
> + json_end_array(vmdesc);
> + }
> +}
> +const VMStateInfo vmstate_info_qtailq = {
> + .name = "qtailq",
> + .get = get_qtailq,
> + .put = put_qtailq,
> +};
> --
> 1.9.1
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
2016-05-31 19:54 ` Paolo Bonzini
@ 2016-05-31 21:53 ` Jianjun Duan
2016-06-01 15:29 ` Paolo Bonzini
0 siblings, 1 reply; 27+ messages in thread
From: Jianjun Duan @ 2016-05-31 21:53 UTC (permalink / raw)
To: Paolo Bonzini
Cc: qemu-devel, qemu-ppc, dmitry, peter maydell, kraxel, mst, david,
veroniabahaa, quintela, amit shah, mreitz, kwolf, rth, aurelien,
leon alrae, blauwirbel, mark cave-ayland, mdroth
On 05/31/2016 12:54 PM, Paolo Bonzini wrote:
>
>
> ----- Original Message -----
>> From: "Jianjun Duan" <duanj@linux.vnet.ibm.com>
>> To: qemu-devel@nongnu.org
>> Cc: qemu-ppc@nongnu.org, duanj@linux.vnet.ibm.com, dmitry@daynix.com, "peter maydell" <peter.maydell@linaro.org>,
>> kraxel@redhat.com, mst@redhat.com, david@gibson.dropbear.id.au, pbonzini@redhat.com, veroniabahaa@gmail.com,
>> quintela@redhat.com, "amit shah" <amit.shah@redhat.com>, mreitz@redhat.com, kwolf@redhat.com, rth@twiddle.net,
>> aurelien@aurel32.net, "leon alrae" <leon.alrae@imgtec.com>, blauwirbel@gmail.com, "mark cave-ayland"
>> <mark.cave-ayland@ilande.co.uk>, mdroth@linux.vnet.ibm.com
>> Sent: Tuesday, May 31, 2016 8:02:42 PM
>> Subject: [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
>>
>> Currently we cannot directly transfer a QTAILQ instance because of the
>> limitation in the migration code. Here we introduce an approach to
>> transfer such structures. In our approach such a structure is tagged
>> with VMS_CSTM. We then modified vmstate_save_state and vmstate_load_state
>> so that when VMS_CSTM is encountered, put and get from VMStateInfo are
>> called respectively. This approach will be used to transfer pending_events
>> and ccs_list in spapr state.
>>
>> We also create some macros in qemu/queue.h to access a QTAILQ using pointer
>> arithmetic. This ensures that we do not depend on the implementation
>> details about QTAILQ in the migration code.
>>
>> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
>> ---
>> include/migration/vmstate.h | 22 +++++++++++++
>> include/qemu/queue.h | 32 ++++++++++++++++++
>> migration/vmstate.c | 79
>> +++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 133 insertions(+)
>>
>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> index 56a4171..da4ef7f 100644
>> --- a/include/migration/vmstate.h
>> +++ b/include/migration/vmstate.h
>> @@ -185,6 +185,8 @@ enum VMStateFlags {
>> * to determine the number of entries in the array. Only valid in
>> * combination with one of VMS_VARRAY*. */
>> VMS_MULTIPLY_ELEMENTS = 0x4000,
>> + /* For fields which need customized handling, such as QTAILQ in
>> queue.h*/
>> + VMS_CSTM = 0x8000,
>
> Please call this VMS_LINKED. It can be adapted to other data
> structures in qemu/queue.h if there is a need later on.
>
>> };
>>
>> struct VMStateField {
>> @@ -245,6 +247,7 @@ extern const VMStateInfo vmstate_info_timer;
>> extern const VMStateInfo vmstate_info_buffer;
>> extern const VMStateInfo vmstate_info_unused_buffer;
>> extern const VMStateInfo vmstate_info_bitmap;
>> +extern const VMStateInfo vmstate_info_qtailq;
>>
>> #define type_check_2darray(t1,t2,n,m) ((t1(*)[n][m])0 - (t2*)0)
>> #define type_check_array(t1,t2,n) ((t1(*)[n])0 - (t2*)0)
>> @@ -656,6 +659,25 @@ extern const VMStateInfo vmstate_info_bitmap;
>> .offset = offsetof(_state, _field), \
>> }
>>
>> +/* For QTAILQ that need customized handling
>> + * _type: type of QTAILQ element
>> + * _next: name of QTAILQ entry field in QTAILQ element
>> + * _vmsd: VMSD for QTAILQ element
>> + * size: size of QTAILQ element
>> + * start: offset of QTAILQ entry in QTAILQ element
>> + */
>> +#define VMSTATE_QTAILQ_V(_field, _state, _version, _vmsd, _type, _next) \
>> +{ \
>> + .name = (stringify(_field)), \
>> + .version_id = (_version), \
>> + .vmsd = &(_vmsd), \
>> + .size = sizeof(_type), \
>> + .info = &vmstate_info_qtailq, \
>> + .flags = VMS_CSTM, \
>> + .offset = offsetof(_state, _field), \
>> + .start = offsetof(_type, _next), \
>> +}
>> +
>> /* _f : field name
>> _f_n : num of elements field_name
>> _n : num of elements
>> diff --git a/include/qemu/queue.h b/include/qemu/queue.h
>> index f781aa2..003e368 100644
>> --- a/include/qemu/queue.h
>> +++ b/include/qemu/queue.h
>> @@ -437,3 +437,35 @@ struct {
>> \
>> (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last))
>>
>> #endif /* !QEMU_SYS_QUEUE_H_ */
>> +
>> +/*
>> + * Offsets of layout of a tail queue head.
>> + */
>> +#define QTAILQ_FIRST_OFFSET 0
>> +#define QTAILQ_LAST_OFFSET (sizeof(void *))
>> +
>> +/*
>> + * Offsets of layout of a tail queue element.
>> + */
>> +#define QTAILQ_NEXT_OFFSET 0
>> +#define QTAILQ_PREV_OFFSET (sizeof(void *))
>> +
>> +/*
>> + * Tail queue tranversal using pointer arithmetic.
>> + */
>> +#define QTAILQ_RAW_FOREACH(elm, head, entry)
>> \
>> + for ((elm) = *((void **) ((char *) (head) + QTAILQ_FIRST_OFFSET));
>> \
>> + (elm);
>> \
>> + (elm) =
>> \
>> + *((void **) ((char *) (elm) + (entry) +
>> QTAILQ_NEXT_OFFSET)))
>> +/*
>> + * Tail queue insertion using pointer arithmetic.
>> + */
>> +#define QTAILQ_RAW_INSERT_TAIL(head, elm, entry) do {
>> \
>> + *((void **) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET)) = NULL;
>> \
>> + *((void **) ((char *) (elm) + (entry) + QTAILQ_PREV_OFFSET)) =
>> \
>> + *((void **) ((char *) (head) +QTAILQ_LAST_OFFSET));
>> \
>> + **((void ***)((char *) (head) +QTAILQ_LAST_OFFSET)) = (elm);
>> \
>> + *((void **) ((char *) (head) + QTAILQ_LAST_OFFSET)) =
>> \
>> + (void *) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET);
>> \
>> +} while (/*CONSTCOND*/0)
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index 644ba1f..ff56650 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -5,7 +5,9 @@
>> #include "migration/vmstate.h"
>> #include "qemu/bitops.h"
>> #include "qemu/error-report.h"
>> +#include "qemu/queue.h"
>> #include "trace.h"
>> +#include "migration/qjson.h"
>>
>> static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription
>> *vmsd,
>> void *opaque, QJSON *vmdesc);
>> @@ -120,6 +122,8 @@ int vmstate_load_state(QEMUFile *f, const
>> VMStateDescription *vmsd,
>> if (field->flags & VMS_STRUCT) {
>> ret = vmstate_load_state(f, field->vmsd, addr,
>> field->vmsd->version_id);
>> + } else if (field->flags & VMS_CSTM) {
>> + ret = field->info->get(f, addr, size, field);
>> } else {
>> ret = field->info->get(f, addr, size, NULL);
>>
>> @@ -192,6 +196,8 @@ static const char *vmfield_get_type_name(VMStateField
>> *field)
>>
>> if (field->flags & VMS_STRUCT) {
>> type = "struct";
>> + } else if (field->flags & VMS_CSTM) {
>> + type = "customized";
>> } else if (field->info->name) {
>> type = field->info->name;
>> }
>> @@ -326,6 +332,8 @@ void vmstate_save_state(QEMUFile *f, const
>> VMStateDescription *vmsd,
>> }
>> if (field->flags & VMS_STRUCT) {
>> vmstate_save_state(f, field->vmsd, addr, vmdesc_loop);
>> + } else if (field->flags & VMS_CSTM) {
>> + field->info->put(f, addr, size, field, vmdesc_loop);
>> } else {
>> field->info->put(f, addr, size, NULL, NULL);
>> }
>> @@ -938,3 +946,74 @@ const VMStateInfo vmstate_info_bitmap = {
>> .get = get_bitmap,
>> .put = put_bitmap,
>> };
>> +
>> +/*get for QTAILQ */
>> +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
>> + VMStateField *field)
>> +{
>> + bool link;
>> + int ret = 0;
>> + const VMStateDescription *vmsd = field->vmsd;
>> + size_t size = field->size;
>> + size_t entry = field->start;
>> + int version_id = field->version_id;
>> + void *elm;
>> +
>> + trace_vmstate_load_state(vmsd->name, version_id);
>> + if (version_id > vmsd->version_id) {
>> + trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL);
>> + return -EINVAL;
>> + }
>> + if (version_id < vmsd->minimum_version_id) {
>> + trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
>> + return -EINVAL;
>> + }
>> +
>> + while (true) {
>> + vmstate_info_bool.get(f, &link, sizeof(bool), NULL);
>
> You can use qemu_get_byte here, and likewise qemu_put_byte below in
> put_qtailq.
qemu_get/put is indeed better choice.
>
>> + if (!link) {
>> + break;
>> + }
>> +
>> + elm = g_malloc(size);
>> + ret = vmstate_load_state(f, vmsd, elm, version_id);
>> + if (ret) {
>> + return ret;
>> + }
>> + QTAILQ_RAW_INSERT_TAIL(pv, elm, entry);
>> + }
>> +
>> + trace_vmstate_load_state_end(vmsd->name, "end", ret);
>> + return ret;
>> +}
>> +
>> +/* put for QTAILQ */
>> +static void put_qtailq(QEMUFile *f, void *pv, size_t unused_size,
>> + VMStateField *field, QJSON *vmdesc)
>> +{
>> + bool link = true;
>> + const VMStateDescription *vmsd = field->vmsd;
>> + size_t entry = field->start;
>> + void *elm;
>> +
>> + if (vmdesc) {
>> + json_prop_str(vmdesc, "vmsd_name", vmsd->name);
>> + json_prop_int(vmdesc, "version", vmsd->version_id);
>> + json_start_array(vmdesc, "fields");
>
> You need to store the fields exactly once here, even if there are
> 0 or >1 elements.
>
Do you mean the json entries? I think it is already doing that.
> Otherwise looks great now!
>
> Paolo
>
>> + }
>> +
>> + QTAILQ_RAW_FOREACH(elm, pv, entry) {
>> + vmstate_info_bool.put(f, &link, sizeof(bool), NULL, NULL);
>> + vmstate_save_state(f, vmsd, elm, vmdesc);
>> + }
>> + link = false;
>> + vmstate_info_bool.put(f, &link, sizeof(bool), NULL, NULL);
>> + if (vmdesc) {
>> + json_end_array(vmdesc);
>> + }
>> +}
>> +const VMStateInfo vmstate_info_qtailq = {
>> + .name = "qtailq",
>> + .get = get_qtailq,
>> + .put = put_qtailq,
>> +};
>> --
>> 1.9.1
>>
>>
>
Thanks,
Jianjun
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
2016-05-31 21:53 ` Jianjun Duan
@ 2016-06-01 15:29 ` Paolo Bonzini
2016-06-01 17:06 ` Jianjun Duan
0 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2016-06-01 15:29 UTC (permalink / raw)
To: Jianjun Duan
Cc: qemu-devel, qemu-ppc, dmitry, peter maydell, kraxel, mst, david,
veroniabahaa, quintela, amit shah, mreitz, kwolf, rth, aurelien,
leon alrae, blauwirbel, mark cave-ayland, mdroth
On 31/05/2016 23:53, Jianjun Duan wrote:
>
>
> On 05/31/2016 12:54 PM, Paolo Bonzini wrote:
>>
>>
>> ----- Original Message -----
>>> From: "Jianjun Duan" <duanj@linux.vnet.ibm.com>
>>> To: qemu-devel@nongnu.org
>>> Cc: qemu-ppc@nongnu.org, duanj@linux.vnet.ibm.com, dmitry@daynix.com, "peter maydell" <peter.maydell@linaro.org>,
>>> kraxel@redhat.com, mst@redhat.com, david@gibson.dropbear.id.au, pbonzini@redhat.com, veroniabahaa@gmail.com,
>>> quintela@redhat.com, "amit shah" <amit.shah@redhat.com>, mreitz@redhat.com, kwolf@redhat.com, rth@twiddle.net,
>>> aurelien@aurel32.net, "leon alrae" <leon.alrae@imgtec.com>, blauwirbel@gmail.com, "mark cave-ayland"
>>> <mark.cave-ayland@ilande.co.uk>, mdroth@linux.vnet.ibm.com
>>> Sent: Tuesday, May 31, 2016 8:02:42 PM
>>> Subject: [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
>>>
>>> Currently we cannot directly transfer a QTAILQ instance because of the
>>> limitation in the migration code. Here we introduce an approach to
>>> transfer such structures. In our approach such a structure is tagged
>>> with VMS_CSTM. We then modified vmstate_save_state and vmstate_load_state
>>> so that when VMS_CSTM is encountered, put and get from VMStateInfo are
>>> called respectively. This approach will be used to transfer pending_events
>>> and ccs_list in spapr state.
>>>
>>> We also create some macros in qemu/queue.h to access a QTAILQ using pointer
>>> arithmetic. This ensures that we do not depend on the implementation
>>> details about QTAILQ in the migration code.
>>>
>>> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
>>> ---
>>> include/migration/vmstate.h | 22 +++++++++++++
>>> include/qemu/queue.h | 32 ++++++++++++++++++
>>> migration/vmstate.c | 79
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 133 insertions(+)
>>>
>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>>> index 56a4171..da4ef7f 100644
>>> --- a/include/migration/vmstate.h
>>> +++ b/include/migration/vmstate.h
>>> @@ -185,6 +185,8 @@ enum VMStateFlags {
>>> * to determine the number of entries in the array. Only valid in
>>> * combination with one of VMS_VARRAY*. */
>>> VMS_MULTIPLY_ELEMENTS = 0x4000,
>>> + /* For fields which need customized handling, such as QTAILQ in
>>> queue.h*/
>>> + VMS_CSTM = 0x8000,
>>
>> Please call this VMS_LINKED. It can be adapted to other data
>> structures in qemu/queue.h if there is a need later on.
>>
>>> };
>>>
>>> struct VMStateField {
>>> @@ -245,6 +247,7 @@ extern const VMStateInfo vmstate_info_timer;
>>> extern const VMStateInfo vmstate_info_buffer;
>>> extern const VMStateInfo vmstate_info_unused_buffer;
>>> extern const VMStateInfo vmstate_info_bitmap;
>>> +extern const VMStateInfo vmstate_info_qtailq;
>>>
>>> #define type_check_2darray(t1,t2,n,m) ((t1(*)[n][m])0 - (t2*)0)
>>> #define type_check_array(t1,t2,n) ((t1(*)[n])0 - (t2*)0)
>>> @@ -656,6 +659,25 @@ extern const VMStateInfo vmstate_info_bitmap;
>>> .offset = offsetof(_state, _field), \
>>> }
>>>
>>> +/* For QTAILQ that need customized handling
>>> + * _type: type of QTAILQ element
>>> + * _next: name of QTAILQ entry field in QTAILQ element
>>> + * _vmsd: VMSD for QTAILQ element
>>> + * size: size of QTAILQ element
>>> + * start: offset of QTAILQ entry in QTAILQ element
>>> + */
>>> +#define VMSTATE_QTAILQ_V(_field, _state, _version, _vmsd, _type, _next) \
>>> +{ \
>>> + .name = (stringify(_field)), \
>>> + .version_id = (_version), \
>>> + .vmsd = &(_vmsd), \
>>> + .size = sizeof(_type), \
>>> + .info = &vmstate_info_qtailq, \
>>> + .flags = VMS_CSTM, \
>>> + .offset = offsetof(_state, _field), \
>>> + .start = offsetof(_type, _next), \
>>> +}
>>> +
>>> /* _f : field name
>>> _f_n : num of elements field_name
>>> _n : num of elements
>>> diff --git a/include/qemu/queue.h b/include/qemu/queue.h
>>> index f781aa2..003e368 100644
>>> --- a/include/qemu/queue.h
>>> +++ b/include/qemu/queue.h
>>> @@ -437,3 +437,35 @@ struct {
>>> \
>>> (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last))
>>>
>>> #endif /* !QEMU_SYS_QUEUE_H_ */
>>> +
>>> +/*
>>> + * Offsets of layout of a tail queue head.
>>> + */
>>> +#define QTAILQ_FIRST_OFFSET 0
>>> +#define QTAILQ_LAST_OFFSET (sizeof(void *))
>>> +
>>> +/*
>>> + * Offsets of layout of a tail queue element.
>>> + */
>>> +#define QTAILQ_NEXT_OFFSET 0
>>> +#define QTAILQ_PREV_OFFSET (sizeof(void *))
>>> +
>>> +/*
>>> + * Tail queue tranversal using pointer arithmetic.
>>> + */
>>> +#define QTAILQ_RAW_FOREACH(elm, head, entry)
>>> \
>>> + for ((elm) = *((void **) ((char *) (head) + QTAILQ_FIRST_OFFSET));
>>> \
>>> + (elm);
>>> \
>>> + (elm) =
>>> \
>>> + *((void **) ((char *) (elm) + (entry) +
>>> QTAILQ_NEXT_OFFSET)))
>>> +/*
>>> + * Tail queue insertion using pointer arithmetic.
>>> + */
>>> +#define QTAILQ_RAW_INSERT_TAIL(head, elm, entry) do {
>>> \
>>> + *((void **) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET)) = NULL;
>>> \
>>> + *((void **) ((char *) (elm) + (entry) + QTAILQ_PREV_OFFSET)) =
>>> \
>>> + *((void **) ((char *) (head) +QTAILQ_LAST_OFFSET));
>>> \
>>> + **((void ***)((char *) (head) +QTAILQ_LAST_OFFSET)) = (elm);
>>> \
>>> + *((void **) ((char *) (head) + QTAILQ_LAST_OFFSET)) =
>>> \
>>> + (void *) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET);
>>> \
>>> +} while (/*CONSTCOND*/0)
>>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>>> index 644ba1f..ff56650 100644
>>> --- a/migration/vmstate.c
>>> +++ b/migration/vmstate.c
>>> @@ -5,7 +5,9 @@
>>> #include "migration/vmstate.h"
>>> #include "qemu/bitops.h"
>>> #include "qemu/error-report.h"
>>> +#include "qemu/queue.h"
>>> #include "trace.h"
>>> +#include "migration/qjson.h"
>>>
>>> static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription
>>> *vmsd,
>>> void *opaque, QJSON *vmdesc);
>>> @@ -120,6 +122,8 @@ int vmstate_load_state(QEMUFile *f, const
>>> VMStateDescription *vmsd,
>>> if (field->flags & VMS_STRUCT) {
>>> ret = vmstate_load_state(f, field->vmsd, addr,
>>> field->vmsd->version_id);
>>> + } else if (field->flags & VMS_CSTM) {
>>> + ret = field->info->get(f, addr, size, field);
>>> } else {
>>> ret = field->info->get(f, addr, size, NULL);
>>>
>>> @@ -192,6 +196,8 @@ static const char *vmfield_get_type_name(VMStateField
>>> *field)
>>>
>>> if (field->flags & VMS_STRUCT) {
>>> type = "struct";
>>> + } else if (field->flags & VMS_CSTM) {
>>> + type = "customized";
>>> } else if (field->info->name) {
>>> type = field->info->name;
>>> }
>>> @@ -326,6 +332,8 @@ void vmstate_save_state(QEMUFile *f, const
>>> VMStateDescription *vmsd,
>>> }
>>> if (field->flags & VMS_STRUCT) {
>>> vmstate_save_state(f, field->vmsd, addr, vmdesc_loop);
>>> + } else if (field->flags & VMS_CSTM) {
>>> + field->info->put(f, addr, size, field, vmdesc_loop);
>>> } else {
>>> field->info->put(f, addr, size, NULL, NULL);
>>> }
>>> @@ -938,3 +946,74 @@ const VMStateInfo vmstate_info_bitmap = {
>>> .get = get_bitmap,
>>> .put = put_bitmap,
>>> };
>>> +
>>> +/*get for QTAILQ */
>>> +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
>>> + VMStateField *field)
>>> +{
>>> + bool link;
>>> + int ret = 0;
>>> + const VMStateDescription *vmsd = field->vmsd;
>>> + size_t size = field->size;
>>> + size_t entry = field->start;
>>> + int version_id = field->version_id;
>>> + void *elm;
>>> +
>>> + trace_vmstate_load_state(vmsd->name, version_id);
>>> + if (version_id > vmsd->version_id) {
>>> + trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL);
>>> + return -EINVAL;
>>> + }
>>> + if (version_id < vmsd->minimum_version_id) {
>>> + trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + while (true) {
>>> + vmstate_info_bool.get(f, &link, sizeof(bool), NULL);
>>
>> You can use qemu_get_byte here, and likewise qemu_put_byte below in
>> put_qtailq.
>
> qemu_get/put is indeed better choice.
>>
>>> + if (!link) {
>>> + break;
>>> + }
>>> +
>>> + elm = g_malloc(size);
>>> + ret = vmstate_load_state(f, vmsd, elm, version_id);
>>> + if (ret) {
>>> + return ret;
>>> + }
>>> + QTAILQ_RAW_INSERT_TAIL(pv, elm, entry);
>>> + }
>>> +
>>> + trace_vmstate_load_state_end(vmsd->name, "end", ret);
>>> + return ret;
>>> +}
>>> +
>>> +/* put for QTAILQ */
>>> +static void put_qtailq(QEMUFile *f, void *pv, size_t unused_size,
>>> + VMStateField *field, QJSON *vmdesc)
>>> +{
>>> + bool link = true;
>>> + const VMStateDescription *vmsd = field->vmsd;
>>> + size_t entry = field->start;
>>> + void *elm;
>>> +
>>> + if (vmdesc) {
>>> + json_prop_str(vmdesc, "vmsd_name", vmsd->name);
>>> + json_prop_int(vmdesc, "version", vmsd->version_id);
>>> + json_start_array(vmdesc, "fields");
>>
>> You need to store the fields exactly once here, even if there are
>> 0 or >1 elements.
>>
> Do you mean the json entries? I think it is already doing that.
In the case of 0 entries we don't go through the loop, so the JSON
entries are definitely missing in that case. I'm not sure if QJSON
handles duplicates in the case of 2+ entries.
Thanks,
Paolo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
2016-06-01 15:29 ` Paolo Bonzini
@ 2016-06-01 17:06 ` Jianjun Duan
2016-06-01 18:32 ` Paolo Bonzini
0 siblings, 1 reply; 27+ messages in thread
From: Jianjun Duan @ 2016-06-01 17:06 UTC (permalink / raw)
To: Paolo Bonzini
Cc: qemu-devel, qemu-ppc, dmitry, peter maydell, kraxel, mst, david,
veroniabahaa, quintela, amit shah, mreitz, kwolf, rth, aurelien,
leon alrae, blauwirbel, mark cave-ayland, mdroth
On 06/01/2016 08:29 AM, Paolo Bonzini wrote:
>
>
> On 31/05/2016 23:53, Jianjun Duan wrote:
>>
>>
>> On 05/31/2016 12:54 PM, Paolo Bonzini wrote:
>>>
>>>
>>> ----- Original Message -----
>>>> From: "Jianjun Duan" <duanj@linux.vnet.ibm.com>
>>>> To: qemu-devel@nongnu.org
>>>> Cc: qemu-ppc@nongnu.org, duanj@linux.vnet.ibm.com, dmitry@daynix.com, "peter maydell" <peter.maydell@linaro.org>,
>>>> kraxel@redhat.com, mst@redhat.com, david@gibson.dropbear.id.au, pbonzini@redhat.com, veroniabahaa@gmail.com,
>>>> quintela@redhat.com, "amit shah" <amit.shah@redhat.com>, mreitz@redhat.com, kwolf@redhat.com, rth@twiddle.net,
>>>> aurelien@aurel32.net, "leon alrae" <leon.alrae@imgtec.com>, blauwirbel@gmail.com, "mark cave-ayland"
>>>> <mark.cave-ayland@ilande.co.uk>, mdroth@linux.vnet.ibm.com
>>>> Sent: Tuesday, May 31, 2016 8:02:42 PM
>>>> Subject: [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
>>>>
>>>> Currently we cannot directly transfer a QTAILQ instance because of the
>>>> limitation in the migration code. Here we introduce an approach to
>>>> transfer such structures. In our approach such a structure is tagged
>>>> with VMS_CSTM. We then modified vmstate_save_state and vmstate_load_state
>>>> so that when VMS_CSTM is encountered, put and get from VMStateInfo are
>>>> called respectively. This approach will be used to transfer pending_events
>>>> and ccs_list in spapr state.
>>>>
>>>> We also create some macros in qemu/queue.h to access a QTAILQ using pointer
>>>> arithmetic. This ensures that we do not depend on the implementation
>>>> details about QTAILQ in the migration code.
>>>>
>>>> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
>>>> ---
>>>> include/migration/vmstate.h | 22 +++++++++++++
>>>> include/qemu/queue.h | 32 ++++++++++++++++++
>>>> migration/vmstate.c | 79
>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 133 insertions(+)
>>>>
>>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>>>> index 56a4171..da4ef7f 100644
>>>> --- a/include/migration/vmstate.h
>>>> +++ b/include/migration/vmstate.h
>>>> @@ -185,6 +185,8 @@ enum VMStateFlags {
>>>> * to determine the number of entries in the array. Only valid in
>>>> * combination with one of VMS_VARRAY*. */
>>>> VMS_MULTIPLY_ELEMENTS = 0x4000,
>>>> + /* For fields which need customized handling, such as QTAILQ in
>>>> queue.h*/
>>>> + VMS_CSTM = 0x8000,
>>>
>>> Please call this VMS_LINKED. It can be adapted to other data
>>> structures in qemu/queue.h if there is a need later on.
>>>
>>>> };
>>>>
>>>> struct VMStateField {
>>>> @@ -245,6 +247,7 @@ extern const VMStateInfo vmstate_info_timer;
>>>> extern const VMStateInfo vmstate_info_buffer;
>>>> extern const VMStateInfo vmstate_info_unused_buffer;
>>>> extern const VMStateInfo vmstate_info_bitmap;
>>>> +extern const VMStateInfo vmstate_info_qtailq;
>>>>
>>>> #define type_check_2darray(t1,t2,n,m) ((t1(*)[n][m])0 - (t2*)0)
>>>> #define type_check_array(t1,t2,n) ((t1(*)[n])0 - (t2*)0)
>>>> @@ -656,6 +659,25 @@ extern const VMStateInfo vmstate_info_bitmap;
>>>> .offset = offsetof(_state, _field), \
>>>> }
>>>>
>>>> +/* For QTAILQ that need customized handling
>>>> + * _type: type of QTAILQ element
>>>> + * _next: name of QTAILQ entry field in QTAILQ element
>>>> + * _vmsd: VMSD for QTAILQ element
>>>> + * size: size of QTAILQ element
>>>> + * start: offset of QTAILQ entry in QTAILQ element
>>>> + */
>>>> +#define VMSTATE_QTAILQ_V(_field, _state, _version, _vmsd, _type, _next) \
>>>> +{ \
>>>> + .name = (stringify(_field)), \
>>>> + .version_id = (_version), \
>>>> + .vmsd = &(_vmsd), \
>>>> + .size = sizeof(_type), \
>>>> + .info = &vmstate_info_qtailq, \
>>>> + .flags = VMS_CSTM, \
>>>> + .offset = offsetof(_state, _field), \
>>>> + .start = offsetof(_type, _next), \
>>>> +}
>>>> +
>>>> /* _f : field name
>>>> _f_n : num of elements field_name
>>>> _n : num of elements
>>>> diff --git a/include/qemu/queue.h b/include/qemu/queue.h
>>>> index f781aa2..003e368 100644
>>>> --- a/include/qemu/queue.h
>>>> +++ b/include/qemu/queue.h
>>>> @@ -437,3 +437,35 @@ struct {
>>>> \
>>>> (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last))
>>>>
>>>> #endif /* !QEMU_SYS_QUEUE_H_ */
>>>> +
>>>> +/*
>>>> + * Offsets of layout of a tail queue head.
>>>> + */
>>>> +#define QTAILQ_FIRST_OFFSET 0
>>>> +#define QTAILQ_LAST_OFFSET (sizeof(void *))
>>>> +
>>>> +/*
>>>> + * Offsets of layout of a tail queue element.
>>>> + */
>>>> +#define QTAILQ_NEXT_OFFSET 0
>>>> +#define QTAILQ_PREV_OFFSET (sizeof(void *))
>>>> +
>>>> +/*
>>>> + * Tail queue tranversal using pointer arithmetic.
>>>> + */
>>>> +#define QTAILQ_RAW_FOREACH(elm, head, entry)
>>>> \
>>>> + for ((elm) = *((void **) ((char *) (head) + QTAILQ_FIRST_OFFSET));
>>>> \
>>>> + (elm);
>>>> \
>>>> + (elm) =
>>>> \
>>>> + *((void **) ((char *) (elm) + (entry) +
>>>> QTAILQ_NEXT_OFFSET)))
>>>> +/*
>>>> + * Tail queue insertion using pointer arithmetic.
>>>> + */
>>>> +#define QTAILQ_RAW_INSERT_TAIL(head, elm, entry) do {
>>>> \
>>>> + *((void **) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET)) = NULL;
>>>> \
>>>> + *((void **) ((char *) (elm) + (entry) + QTAILQ_PREV_OFFSET)) =
>>>> \
>>>> + *((void **) ((char *) (head) +QTAILQ_LAST_OFFSET));
>>>> \
>>>> + **((void ***)((char *) (head) +QTAILQ_LAST_OFFSET)) = (elm);
>>>> \
>>>> + *((void **) ((char *) (head) + QTAILQ_LAST_OFFSET)) =
>>>> \
>>>> + (void *) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET);
>>>> \
>>>> +} while (/*CONSTCOND*/0)
>>>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>>>> index 644ba1f..ff56650 100644
>>>> --- a/migration/vmstate.c
>>>> +++ b/migration/vmstate.c
>>>> @@ -5,7 +5,9 @@
>>>> #include "migration/vmstate.h"
>>>> #include "qemu/bitops.h"
>>>> #include "qemu/error-report.h"
>>>> +#include "qemu/queue.h"
>>>> #include "trace.h"
>>>> +#include "migration/qjson.h"
>>>>
>>>> static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription
>>>> *vmsd,
>>>> void *opaque, QJSON *vmdesc);
>>>> @@ -120,6 +122,8 @@ int vmstate_load_state(QEMUFile *f, const
>>>> VMStateDescription *vmsd,
>>>> if (field->flags & VMS_STRUCT) {
>>>> ret = vmstate_load_state(f, field->vmsd, addr,
>>>> field->vmsd->version_id);
>>>> + } else if (field->flags & VMS_CSTM) {
>>>> + ret = field->info->get(f, addr, size, field);
>>>> } else {
>>>> ret = field->info->get(f, addr, size, NULL);
>>>>
>>>> @@ -192,6 +196,8 @@ static const char *vmfield_get_type_name(VMStateField
>>>> *field)
>>>>
>>>> if (field->flags & VMS_STRUCT) {
>>>> type = "struct";
>>>> + } else if (field->flags & VMS_CSTM) {
>>>> + type = "customized";
>>>> } else if (field->info->name) {
>>>> type = field->info->name;
>>>> }
>>>> @@ -326,6 +332,8 @@ void vmstate_save_state(QEMUFile *f, const
>>>> VMStateDescription *vmsd,
>>>> }
>>>> if (field->flags & VMS_STRUCT) {
>>>> vmstate_save_state(f, field->vmsd, addr, vmdesc_loop);
>>>> + } else if (field->flags & VMS_CSTM) {
>>>> + field->info->put(f, addr, size, field, vmdesc_loop);
>>>> } else {
>>>> field->info->put(f, addr, size, NULL, NULL);
>>>> }
>>>> @@ -938,3 +946,74 @@ const VMStateInfo vmstate_info_bitmap = {
>>>> .get = get_bitmap,
>>>> .put = put_bitmap,
>>>> };
>>>> +
>>>> +/*get for QTAILQ */
>>>> +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
>>>> + VMStateField *field)
>>>> +{
>>>> + bool link;
>>>> + int ret = 0;
>>>> + const VMStateDescription *vmsd = field->vmsd;
>>>> + size_t size = field->size;
>>>> + size_t entry = field->start;
>>>> + int version_id = field->version_id;
>>>> + void *elm;
>>>> +
>>>> + trace_vmstate_load_state(vmsd->name, version_id);
>>>> + if (version_id > vmsd->version_id) {
>>>> + trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL);
>>>> + return -EINVAL;
>>>> + }
>>>> + if (version_id < vmsd->minimum_version_id) {
>>>> + trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + while (true) {
>>>> + vmstate_info_bool.get(f, &link, sizeof(bool), NULL);
>>>
>>> You can use qemu_get_byte here, and likewise qemu_put_byte below in
>>> put_qtailq.
>>
>> qemu_get/put is indeed better choice.
>>>
>>>> + if (!link) {
>>>> + break;
>>>> + }
>>>> +
>>>> + elm = g_malloc(size);
>>>> + ret = vmstate_load_state(f, vmsd, elm, version_id);
>>>> + if (ret) {
>>>> + return ret;
>>>> + }
>>>> + QTAILQ_RAW_INSERT_TAIL(pv, elm, entry);
>>>> + }
>>>> +
>>>> + trace_vmstate_load_state_end(vmsd->name, "end", ret);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +/* put for QTAILQ */
>>>> +static void put_qtailq(QEMUFile *f, void *pv, size_t unused_size,
>>>> + VMStateField *field, QJSON *vmdesc)
>>>> +{
>>>> + bool link = true;
>>>> + const VMStateDescription *vmsd = field->vmsd;
>>>> + size_t entry = field->start;
>>>> + void *elm;
>>>> +
>>>> + if (vmdesc) {
>>>> + json_prop_str(vmdesc, "vmsd_name", vmsd->name);
>>>> + json_prop_int(vmdesc, "version", vmsd->version_id);
>>>> + json_start_array(vmdesc, "fields");
>>>
>>> You need to store the fields exactly once here, even if there are
>>> 0 or >1 elements.
>>>
>> Do you mean the json entries? I think it is already doing that.
>
> In the case of 0 entries we don't go through the loop, so the JSON
> entries are definitely missing in that case. I'm not sure if QJSON
> handles duplicates in the case of 2+ entries.
The vmsd here is the vmsd for the queue elements. Not for the queue.
Maybe the stuff written here should be information about the qtailq
instead, but we don't have a vmsd for the queue as a whole.
As it stands, it will record the name, version of the element type. If
the queue is empty, it records nothing in the fields. otherwise it will
record each element and repeat.
In vmstate_save_state, the vmsd of the array element for a uncompressed
array is recorded every time an array element is saved. The number of
written bytes is also recorded. If the array has 0 elements, it is not
recorded.
If we want to make the behavior consistent, we should not do any json
stuff in put_qtatilq function.
If we want to record the vmsd of the queue element exactly once, I can
set the vmdesc to null after the first iteration. But we may need a
recursive function just to dump out the vmsd when the queue is empty,
and record that 0 bytes are written for each field.
I would say that let's remove the json stuff here to be consistent with
array behavior.
>
> Thanks,
>
> Paolo
>
Thanks,
Jianjun
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
2016-06-01 17:06 ` Jianjun Duan
@ 2016-06-01 18:32 ` Paolo Bonzini
0 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2016-06-01 18:32 UTC (permalink / raw)
To: Jianjun Duan
Cc: veroniabahaa, peter maydell, mdroth, mst, quintela,
mark cave-ayland, qemu-devel, mreitz, blauwirbel, amit shah,
qemu-ppc, kraxel, dmitry, kwolf, rth, leon alrae, aurelien,
david
On 01/06/2016 19:06, Jianjun Duan wrote:
> On 06/01/2016 08:29 AM, Paolo Bonzini wrote:
>> On 31/05/2016 23:53, Jianjun Duan wrote:
>>> On 05/31/2016 12:54 PM, Paolo Bonzini wrote:
>>>>> +/* put for QTAILQ */
>>>>> +static void put_qtailq(QEMUFile *f, void *pv, size_t unused_size,
>>>>> + VMStateField *field, QJSON *vmdesc)
>>>>> +{
>>>>> + bool link = true;
>>>>> + const VMStateDescription *vmsd = field->vmsd;
>>>>> + size_t entry = field->start;
>>>>> + void *elm;
>>>>> +
>>>>> + if (vmdesc) {
>>>>> + json_prop_str(vmdesc, "vmsd_name", vmsd->name);
>>>>> + json_prop_int(vmdesc, "version", vmsd->version_id);
>>>>> + json_start_array(vmdesc, "fields");
>>>>
>>>> You need to store the fields exactly once here, even if there are
>>>> 0 or >1 elements.
>>>>
>>> Do you mean the json entries? I think it is already doing that.
>>
>> In the case of 0 entries we don't go through the loop, so the JSON
>> entries are definitely missing in that case. I'm not sure if QJSON
>> handles duplicates in the case of 2+ entries.
>
> The vmsd here is the vmsd for the queue elements. Not for the queue.
> Maybe the stuff written here should be information about the qtailq
> instead, but we don't have a vmsd for the queue as a whole.
You're right, you could use vmsd_can_compress but it's not necessary to
do so. Your code is fine.
Paolo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ Jianjun Duan
2016-05-31 19:54 ` Paolo Bonzini
@ 2016-06-02 3:58 ` David Gibson
2016-06-02 15:01 ` Sascha Silbe
2 siblings, 0 replies; 27+ messages in thread
From: David Gibson @ 2016-06-02 3:58 UTC (permalink / raw)
To: Jianjun Duan
Cc: qemu-devel, qemu-ppc, dmitry, peter.maydell, kraxel, mst,
pbonzini, veroniabahaa, quintela, amit.shah, mreitz, kwolf, rth,
aurelien, leon.alrae, blauwirbel, mark.cave-ayland, mdroth,
dgilbert
[-- Attachment #1: Type: text/plain, Size: 9748 bytes --]
On Tue, May 31, 2016 at 11:02:42AM -0700, Jianjun Duan wrote:
> Currently we cannot directly transfer a QTAILQ instance because of the
> limitation in the migration code. Here we introduce an approach to
> transfer such structures. In our approach such a structure is tagged
> with VMS_CSTM. We then modified vmstate_save_state and vmstate_load_state
> so that when VMS_CSTM is encountered, put and get from VMStateInfo are
> called respectively. This approach will be used to transfer pending_events
> and ccs_list in spapr state.
>
> We also create some macros in qemu/queue.h to access a QTAILQ using pointer
> arithmetic. This ensures that we do not depend on the implementation
> details about QTAILQ in the migration code.
>
> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
I'm comfortable with 3&4 of 6, but I'd prefer to see them merged via
the migration tree.
Juan, Dave, are you ok to merge these?
> ---
> include/migration/vmstate.h | 22 +++++++++++++
> include/qemu/queue.h | 32 ++++++++++++++++++
> migration/vmstate.c | 79 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 133 insertions(+)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 56a4171..da4ef7f 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -185,6 +185,8 @@ enum VMStateFlags {
> * to determine the number of entries in the array. Only valid in
> * combination with one of VMS_VARRAY*. */
> VMS_MULTIPLY_ELEMENTS = 0x4000,
> + /* For fields which need customized handling, such as QTAILQ in queue.h*/
> + VMS_CSTM = 0x8000,
> };
>
> struct VMStateField {
> @@ -245,6 +247,7 @@ extern const VMStateInfo vmstate_info_timer;
> extern const VMStateInfo vmstate_info_buffer;
> extern const VMStateInfo vmstate_info_unused_buffer;
> extern const VMStateInfo vmstate_info_bitmap;
> +extern const VMStateInfo vmstate_info_qtailq;
>
> #define type_check_2darray(t1,t2,n,m) ((t1(*)[n][m])0 - (t2*)0)
> #define type_check_array(t1,t2,n) ((t1(*)[n])0 - (t2*)0)
> @@ -656,6 +659,25 @@ extern const VMStateInfo vmstate_info_bitmap;
> .offset = offsetof(_state, _field), \
> }
>
> +/* For QTAILQ that need customized handling
> + * _type: type of QTAILQ element
> + * _next: name of QTAILQ entry field in QTAILQ element
> + * _vmsd: VMSD for QTAILQ element
> + * size: size of QTAILQ element
> + * start: offset of QTAILQ entry in QTAILQ element
> + */
> +#define VMSTATE_QTAILQ_V(_field, _state, _version, _vmsd, _type, _next) \
> +{ \
> + .name = (stringify(_field)), \
> + .version_id = (_version), \
> + .vmsd = &(_vmsd), \
> + .size = sizeof(_type), \
> + .info = &vmstate_info_qtailq, \
> + .flags = VMS_CSTM, \
> + .offset = offsetof(_state, _field), \
> + .start = offsetof(_type, _next), \
> +}
> +
> /* _f : field name
> _f_n : num of elements field_name
> _n : num of elements
> diff --git a/include/qemu/queue.h b/include/qemu/queue.h
> index f781aa2..003e368 100644
> --- a/include/qemu/queue.h
> +++ b/include/qemu/queue.h
> @@ -437,3 +437,35 @@ struct { \
> (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last))
>
> #endif /* !QEMU_SYS_QUEUE_H_ */
> +
> +/*
> + * Offsets of layout of a tail queue head.
> + */
> +#define QTAILQ_FIRST_OFFSET 0
> +#define QTAILQ_LAST_OFFSET (sizeof(void *))
> +
> +/*
> + * Offsets of layout of a tail queue element.
> + */
> +#define QTAILQ_NEXT_OFFSET 0
> +#define QTAILQ_PREV_OFFSET (sizeof(void *))
> +
> +/*
> + * Tail queue tranversal using pointer arithmetic.
> + */
> +#define QTAILQ_RAW_FOREACH(elm, head, entry) \
> + for ((elm) = *((void **) ((char *) (head) + QTAILQ_FIRST_OFFSET)); \
> + (elm); \
> + (elm) = \
> + *((void **) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET)))
> +/*
> + * Tail queue insertion using pointer arithmetic.
> + */
> +#define QTAILQ_RAW_INSERT_TAIL(head, elm, entry) do { \
> + *((void **) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET)) = NULL; \
> + *((void **) ((char *) (elm) + (entry) + QTAILQ_PREV_OFFSET)) = \
> + *((void **) ((char *) (head) +QTAILQ_LAST_OFFSET)); \
> + **((void ***)((char *) (head) +QTAILQ_LAST_OFFSET)) = (elm); \
> + *((void **) ((char *) (head) + QTAILQ_LAST_OFFSET)) = \
> + (void *) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET); \
> +} while (/*CONSTCOND*/0)
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 644ba1f..ff56650 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -5,7 +5,9 @@
> #include "migration/vmstate.h"
> #include "qemu/bitops.h"
> #include "qemu/error-report.h"
> +#include "qemu/queue.h"
> #include "trace.h"
> +#include "migration/qjson.h"
>
> static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
> void *opaque, QJSON *vmdesc);
> @@ -120,6 +122,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> if (field->flags & VMS_STRUCT) {
> ret = vmstate_load_state(f, field->vmsd, addr,
> field->vmsd->version_id);
> + } else if (field->flags & VMS_CSTM) {
> + ret = field->info->get(f, addr, size, field);
> } else {
> ret = field->info->get(f, addr, size, NULL);
>
> @@ -192,6 +196,8 @@ static const char *vmfield_get_type_name(VMStateField *field)
>
> if (field->flags & VMS_STRUCT) {
> type = "struct";
> + } else if (field->flags & VMS_CSTM) {
> + type = "customized";
> } else if (field->info->name) {
> type = field->info->name;
> }
> @@ -326,6 +332,8 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> }
> if (field->flags & VMS_STRUCT) {
> vmstate_save_state(f, field->vmsd, addr, vmdesc_loop);
> + } else if (field->flags & VMS_CSTM) {
> + field->info->put(f, addr, size, field, vmdesc_loop);
> } else {
> field->info->put(f, addr, size, NULL, NULL);
> }
> @@ -938,3 +946,74 @@ const VMStateInfo vmstate_info_bitmap = {
> .get = get_bitmap,
> .put = put_bitmap,
> };
> +
> +/*get for QTAILQ */
> +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
> + VMStateField *field)
> +{
> + bool link;
> + int ret = 0;
> + const VMStateDescription *vmsd = field->vmsd;
> + size_t size = field->size;
> + size_t entry = field->start;
> + int version_id = field->version_id;
> + void *elm;
> +
> + trace_vmstate_load_state(vmsd->name, version_id);
> + if (version_id > vmsd->version_id) {
> + trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL);
> + return -EINVAL;
> + }
> + if (version_id < vmsd->minimum_version_id) {
> + trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
> + return -EINVAL;
> + }
> +
> + while (true) {
> + vmstate_info_bool.get(f, &link, sizeof(bool), NULL);
> + if (!link) {
> + break;
> + }
> +
> + elm = g_malloc(size);
> + ret = vmstate_load_state(f, vmsd, elm, version_id);
> + if (ret) {
> + return ret;
> + }
> + QTAILQ_RAW_INSERT_TAIL(pv, elm, entry);
> + }
> +
> + trace_vmstate_load_state_end(vmsd->name, "end", ret);
> + return ret;
> +}
> +
> +/* put for QTAILQ */
> +static void put_qtailq(QEMUFile *f, void *pv, size_t unused_size,
> + VMStateField *field, QJSON *vmdesc)
> +{
> + bool link = true;
> + const VMStateDescription *vmsd = field->vmsd;
> + size_t entry = field->start;
> + void *elm;
> +
> + if (vmdesc) {
> + json_prop_str(vmdesc, "vmsd_name", vmsd->name);
> + json_prop_int(vmdesc, "version", vmsd->version_id);
> + json_start_array(vmdesc, "fields");
> + }
> +
> + QTAILQ_RAW_FOREACH(elm, pv, entry) {
> + vmstate_info_bool.put(f, &link, sizeof(bool), NULL, NULL);
> + vmstate_save_state(f, vmsd, elm, vmdesc);
> + }
> + link = false;
> + vmstate_info_bool.put(f, &link, sizeof(bool), NULL, NULL);
> + if (vmdesc) {
> + json_end_array(vmdesc);
> + }
> +}
> +const VMStateInfo vmstate_info_qtailq = {
> + .name = "qtailq",
> + .get = get_qtailq,
> + .put = put_qtailq,
> +};
--
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 --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH v3 1/6] Migration: Defined VMStateDescription struct for spapr_drc
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 1/6] Migration: Defined VMStateDescription struct for spapr_drc Jianjun Duan
@ 2016-06-02 4:07 ` David Gibson
2016-06-03 0:28 ` Jianjun Duan
0 siblings, 1 reply; 27+ messages in thread
From: David Gibson @ 2016-06-02 4:07 UTC (permalink / raw)
To: Jianjun Duan
Cc: qemu-devel, qemu-ppc, dmitry, peter.maydell, kraxel, mst,
pbonzini, veroniabahaa, quintela, amit.shah, mreitz, kwolf, rth,
aurelien, leon.alrae, blauwirbel, mark.cave-ayland, mdroth
[-- Attachment #1: Type: text/plain, Size: 8827 bytes --]
On Tue, May 31, 2016 at 11:02:39AM -0700, Jianjun Duan wrote:
> To manage hotplug/unplug of dynamic resources such as PCI cards,
> memory, and CPU on sPAPR guests, a firmware abstraction known as
> a Dynamic Resource Connector (DRC) is used to assign a particular
> dynamic resource to the guest, and provide an interface for the
> guest to manage configuration/removal of the resource associated
> with it.
>
> To migrate the hotplugged resources in migration, the
> associated DRC state need be migrated. 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
> ones modifiable or needed by guest actions or device add/remove
> operation are migrated. From the perspective of device
> hotplugging, if we hotplug a device on the source, we need to
> "coldplug" it on the target. The states across two hosts for the
> same device are not the same. Ideally we want the states be same
> after migration so that the device would function as hotplugged
> on the target. For example we can unplug it. The minimum DRC
> state we need to transfer should cover all the pieces changed by
> hotplugging. Out of the elements of the DRC state, isolation_state,
> allocation_sate, and configured are involved in the DR state
> transition diagram from PAPR+ 2.7, 13.4. configured and signalled
> are needed in attaching and detaching devices. indicator_state
> provides users with hardware state information. These 6 elements
> are migrated.
>
> detach_cb in the DRC state is a function pointer that cannot be
> migrated. We set it right after DRC state is migrated so that
> a migrated hot-unplug event could finish its work.
>
> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
So, the thing which concerns me about this patch is that it exposes
the DRCs as real objects in the migration stream, which will make it
difficult to ever remove them in the future.
Particularly for LMBs having full QOM objects for every individual DRC
is kind of clunky, and I'd already considered replacing them instead
with a QOM interface implementing a whole array of DRCs as part of the
"owning" object (machine or PHB).
The internal structure of the VMSD looks ok to me, but the
complication is the automatic "addressing" of it as a separate
object. If we could manually take that over, allowing us to direct
the same VMSD data at elements in a simpler DRC array later, that
would be preferable.
> ---
> hw/ppc/spapr_drc.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++
> hw/ppc/spapr_pci.c | 22 +++++++++++++++++
> include/hw/ppc/spapr_drc.h | 9 +++++++
> 3 files changed, 92 insertions(+)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 94c875d..1fb5e23 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -617,6 +617,65 @@ static void spapr_dr_connector_instance_init(Object *obj)
> NULL, NULL, NULL, NULL);
> }
>
> +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) {
> + /* for PCI 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;
> + /* for LMB type */
> + 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;
> + default:
> + ;
> + }
> +
> + return rc;
> +}
> +
> +/* detach_cb needs be set since it is not migrated */
> +static void postmigrate_set_detach_cb(sPAPRDRConnector *drc,
> + spapr_drc_detach_cb *detach_cb)
> +{
> + drc->detach_cb = detach_cb;
> +}
> +
> +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(signalled, sPAPRDRConnector),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> {
> DeviceClass *dk = DEVICE_CLASS(k);
> @@ -625,6 +684,7 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> dk->reset = reset;
> dk->realize = realize;
> dk->unrealize = unrealize;
> + dk->vmsd = &vmstate_spapr_drc;
> drck->set_isolation_state = set_isolation_state;
> drck->set_indicator_state = set_indicator_state;
> drck->set_allocation_state = set_allocation_state;
> @@ -638,6 +698,7 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> drck->detach = detach;
> drck->release_pending = release_pending;
> drck->set_signalled = set_signalled;
> + drck->postmigrate_set_detach_cb = postmigrate_set_detach_cb;
> /*
> * Reason: it crashes FIXME find and document the real reason
> */
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 856aec7..c68da08 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1562,11 +1562,33 @@ static void spapr_pci_pre_save(void *opaque)
> }
> }
>
> +/*
> + * detach_cb in the DRC state is a function pointer that cannot be
> + * migrated. We set it right after migration so that a migrated
> + * hot-unplug event could finish its work.
> + */
> +static void spapr_pci_set_detach_cb(PCIBus *bus, PCIDevice *pdev,
> + void *opaque)
> +{
> + sPAPRPHBState *sphb = opaque;
> + sPAPRDRConnector *drc = spapr_phb_get_pci_drc(sphb, pdev);
> + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> + drck->postmigrate_set_detach_cb(drc, spapr_phb_remove_pci_device_cb);
> +}
> +
> static int spapr_pci_post_load(void *opaque, int version_id)
> {
> sPAPRPHBState *sphb = opaque;
> gpointer key, value;
> int i;
> + PCIBus *bus = PCI_HOST_BRIDGE(sphb)->bus;
> + unsigned int bus_no = 0;
> +
> + /* Set detach_cb for the drc unconditionally after migration */
> + if (bus) {
> + pci_for_each_device(bus, pci_bus_num(bus), spapr_pci_set_detach_cb,
> + &bus_no);
> + }
>
> for (i = 0; i < sphb->msi_devs_num; ++i) {
> key = g_memdup(&sphb->msi_devs[i].key,
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index fa21ba0..a68433b 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -190,6 +190,15 @@ typedef struct sPAPRDRConnectorClass {
> void *detach_cb_opaque, Error **errp);
> bool (*release_pending)(sPAPRDRConnector *drc);
> void (*set_signalled)(sPAPRDRConnector *drc);
> +
> + /*
> + * QEMU interface for setting detach_cb after migration.
> + * detach_cb in the DRC state is a function pointer that cannot be
> + * migrated. We set it right after migration so that a migrated
> + * hot-unplug event could finish its work.
> + */
> + void (*postmigrate_set_detach_cb)(sPAPRDRConnector *drc,
> + spapr_drc_detach_cb *detach_cb);
> } sPAPRDRConnectorClass;
>
> sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
--
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 --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ Jianjun Duan
2016-05-31 19:54 ` Paolo Bonzini
2016-06-02 3:58 ` David Gibson
@ 2016-06-02 15:01 ` Sascha Silbe
2016-06-02 16:11 ` Jianjun Duan
` (2 more replies)
2 siblings, 3 replies; 27+ messages in thread
From: Sascha Silbe @ 2016-06-02 15:01 UTC (permalink / raw)
To: Jianjun Duan, qemu-devel
Cc: veroniabahaa, peter.maydell, mst, quintela, mark.cave-ayland,
mdroth, mreitz, blauwirbel, amit.shah, qemu-ppc, kraxel, kwolf,
dmitry, pbonzini, rth, leon.alrae, aurelien, david
Dear Jianjun,
Jianjun Duan <duanj@linux.vnet.ibm.com> writes:
[include/migration/vmstate.h]
> @@ -185,6 +185,8 @@ enum VMStateFlags {
> * to determine the number of entries in the array. Only valid in
> * combination with one of VMS_VARRAY*. */
> VMS_MULTIPLY_ELEMENTS = 0x4000,
> + /* For fields which need customized handling, such as QTAILQ in queue.h*/
> + VMS_CSTM = 0x8000,
Can you describe (in the comment) how this customised handling is
performed, please? I.e. describe what exactly happens if the flag is
set, from the point of view of an API consumer.
Also, why do you need this flag at all? The only change I can see is
that you pass additional information to VMStateInfo.get() / .put(),
using NULL if it's not set. Why don't you just always pass the
additional information? If the additional information is not needed by
get() / put() the parameter will be unused anyway.
Sascha
--
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
2016-06-02 15:01 ` Sascha Silbe
@ 2016-06-02 16:11 ` Jianjun Duan
2016-06-07 14:43 ` Dr. David Alan Gilbert
2016-06-03 17:12 ` [Qemu-devel] " Jianjun Duan
2016-06-06 13:47 ` Paolo Bonzini
2 siblings, 1 reply; 27+ messages in thread
From: Jianjun Duan @ 2016-06-02 16:11 UTC (permalink / raw)
To: Sascha Silbe, qemu-devel
Cc: veroniabahaa, peter.maydell, mst, quintela, mark.cave-ayland,
mdroth, mreitz, blauwirbel, amit.shah, qemu-ppc, kraxel, kwolf,
dmitry, pbonzini, rth, leon.alrae, aurelien, david
Hi Sascha,
On 06/02/2016 08:01 AM, Sascha Silbe wrote:
> Dear Jianjun,
>
> Jianjun Duan <duanj@linux.vnet.ibm.com> writes:
>
> [include/migration/vmstate.h]
>> @@ -185,6 +185,8 @@ enum VMStateFlags {
>> * to determine the number of entries in the array. Only valid in
>> * combination with one of VMS_VARRAY*. */
>> VMS_MULTIPLY_ELEMENTS = 0x4000,
>> + /* For fields which need customized handling, such as QTAILQ in queue.h*/
>> + VMS_CSTM = 0x8000,
>
> Can you describe (in the comment) how this customised handling is
> performed, please? I.e. describe what exactly happens if the flag is
> set, from the point of view of an API consumer.
I will add more comments. When this flag is set VMStateInfo.get/put will
be used in vmstate_load/save_state instead of recursive call. And the
user should implement VMStateInfo.get/put to handle the concerned data
structure.
> Also, why do you need this flag at all? The only change I can see is
> that you pass additional information to VMStateInfo.get() / .put(),
> using NULL if it's not set. Why don't you just always pass the
> additional information? If the additional information is not needed by
> get() / put() the parameter will be unused anyway.
You can do it without creating this flag. Instead just to check if info
is set in the field. However I think it is more readable and more robust
to check this flag in vmstate_load/get_state.
> Sascha
>
Thanks,
Jianjun
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH v3 1/6] Migration: Defined VMStateDescription struct for spapr_drc
2016-06-02 4:07 ` David Gibson
@ 2016-06-03 0:28 ` Jianjun Duan
2016-06-03 1:48 ` David Gibson
0 siblings, 1 reply; 27+ messages in thread
From: Jianjun Duan @ 2016-06-03 0:28 UTC (permalink / raw)
To: David Gibson
Cc: qemu-devel, qemu-ppc, dmitry, peter.maydell, kraxel, mst,
pbonzini, veroniabahaa, quintela, amit.shah, mreitz, kwolf, rth,
aurelien, leon.alrae, blauwirbel, mark.cave-ayland, mdroth
On 06/01/2016 09:07 PM, David Gibson wrote:
> On Tue, May 31, 2016 at 11:02:39AM -0700, Jianjun Duan wrote:
>> To manage hotplug/unplug of dynamic resources such as PCI cards,
>> memory, and CPU on sPAPR guests, a firmware abstraction known as
>> a Dynamic Resource Connector (DRC) is used to assign a particular
>> dynamic resource to the guest, and provide an interface for the
>> guest to manage configuration/removal of the resource associated
>> with it.
>>
>> To migrate the hotplugged resources in migration, the
>> associated DRC state need be migrated. 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
>> ones modifiable or needed by guest actions or device add/remove
>> operation are migrated. From the perspective of device
>> hotplugging, if we hotplug a device on the source, we need to
>> "coldplug" it on the target. The states across two hosts for the
>> same device are not the same. Ideally we want the states be same
>> after migration so that the device would function as hotplugged
>> on the target. For example we can unplug it. The minimum DRC
>> state we need to transfer should cover all the pieces changed by
>> hotplugging. Out of the elements of the DRC state, isolation_state,
>> allocation_sate, and configured are involved in the DR state
>> transition diagram from PAPR+ 2.7, 13.4. configured and signalled
>> are needed in attaching and detaching devices. indicator_state
>> provides users with hardware state information. These 6 elements
>> are migrated.
>>
>> detach_cb in the DRC state is a function pointer that cannot be
>> migrated. We set it right after DRC state is migrated so that
>> a migrated hot-unplug event could finish its work.
>>
>> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
>
> So, the thing which concerns me about this patch is that it exposes
> the DRCs as real objects in the migration stream, which will make it
> difficult to ever remove them in the future.
>
> Particularly for LMBs having full QOM objects for every individual DRC
> is kind of clunky, and I'd already considered replacing them instead
> with a QOM interface implementing a whole array of DRCs as part of the
> "owning" object (machine or PHB).
>
> The internal structure of the VMSD looks ok to me, but the
> complication is the automatic "addressing" of it as a separate
> object. If we could manually take that over, allowing us to direct
> the same VMSD data at elements in a simpler DRC array later, that
> would be preferable.
We can address this concern by setting the unique instance_id for each
drc. The instance_id can be used to match the drcs from source and target.
>
>
>> ---
>> hw/ppc/spapr_drc.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++
>> hw/ppc/spapr_pci.c | 22 +++++++++++++++++
>> include/hw/ppc/spapr_drc.h | 9 +++++++
>> 3 files changed, 92 insertions(+)
>>
>> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>> index 94c875d..1fb5e23 100644
>> --- a/hw/ppc/spapr_drc.c
>> +++ b/hw/ppc/spapr_drc.c
>> @@ -617,6 +617,65 @@ static void spapr_dr_connector_instance_init(Object *obj)
>> NULL, NULL, NULL, NULL);
>> }
>>
>> +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) {
>> + /* for PCI 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;
>> + /* for LMB type */
>> + 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;
>> + default:
>> + ;
>> + }
>> +
>> + return rc;
>> +}
>> +
>> +/* detach_cb needs be set since it is not migrated */
>> +static void postmigrate_set_detach_cb(sPAPRDRConnector *drc,
>> + spapr_drc_detach_cb *detach_cb)
>> +{
>> + drc->detach_cb = detach_cb;
>> +}
>> +
>> +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(signalled, sPAPRDRConnector),
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
>> +
>> static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
>> {
>> DeviceClass *dk = DEVICE_CLASS(k);
>> @@ -625,6 +684,7 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
>> dk->reset = reset;
>> dk->realize = realize;
>> dk->unrealize = unrealize;
>> + dk->vmsd = &vmstate_spapr_drc;
>> drck->set_isolation_state = set_isolation_state;
>> drck->set_indicator_state = set_indicator_state;
>> drck->set_allocation_state = set_allocation_state;
>> @@ -638,6 +698,7 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
>> drck->detach = detach;
>> drck->release_pending = release_pending;
>> drck->set_signalled = set_signalled;
>> + drck->postmigrate_set_detach_cb = postmigrate_set_detach_cb;
>> /*
>> * Reason: it crashes FIXME find and document the real reason
>> */
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 856aec7..c68da08 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -1562,11 +1562,33 @@ static void spapr_pci_pre_save(void *opaque)
>> }
>> }
>>
>> +/*
>> + * detach_cb in the DRC state is a function pointer that cannot be
>> + * migrated. We set it right after migration so that a migrated
>> + * hot-unplug event could finish its work.
>> + */
>> +static void spapr_pci_set_detach_cb(PCIBus *bus, PCIDevice *pdev,
>> + void *opaque)
>> +{
>> + sPAPRPHBState *sphb = opaque;
>> + sPAPRDRConnector *drc = spapr_phb_get_pci_drc(sphb, pdev);
>> + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>> + drck->postmigrate_set_detach_cb(drc, spapr_phb_remove_pci_device_cb);
>> +}
>> +
>> static int spapr_pci_post_load(void *opaque, int version_id)
>> {
>> sPAPRPHBState *sphb = opaque;
>> gpointer key, value;
>> int i;
>> + PCIBus *bus = PCI_HOST_BRIDGE(sphb)->bus;
>> + unsigned int bus_no = 0;
>> +
>> + /* Set detach_cb for the drc unconditionally after migration */
>> + if (bus) {
>> + pci_for_each_device(bus, pci_bus_num(bus), spapr_pci_set_detach_cb,
>> + &bus_no);
>> + }
>>
>> for (i = 0; i < sphb->msi_devs_num; ++i) {
>> key = g_memdup(&sphb->msi_devs[i].key,
>> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
>> index fa21ba0..a68433b 100644
>> --- a/include/hw/ppc/spapr_drc.h
>> +++ b/include/hw/ppc/spapr_drc.h
>> @@ -190,6 +190,15 @@ typedef struct sPAPRDRConnectorClass {
>> void *detach_cb_opaque, Error **errp);
>> bool (*release_pending)(sPAPRDRConnector *drc);
>> void (*set_signalled)(sPAPRDRConnector *drc);
>> +
>> + /*
>> + * QEMU interface for setting detach_cb after migration.
>> + * detach_cb in the DRC state is a function pointer that cannot be
>> + * migrated. We set it right after migration so that a migrated
>> + * hot-unplug event could finish its work.
>> + */
>> + void (*postmigrate_set_detach_cb)(sPAPRDRConnector *drc,
>> + spapr_drc_detach_cb *detach_cb);
>> } sPAPRDRConnectorClass;
>>
>> sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
>
Thanks,
Jianjun
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH v3 1/6] Migration: Defined VMStateDescription struct for spapr_drc
2016-06-03 0:28 ` Jianjun Duan
@ 2016-06-03 1:48 ` David Gibson
0 siblings, 0 replies; 27+ messages in thread
From: David Gibson @ 2016-06-03 1:48 UTC (permalink / raw)
To: Jianjun Duan
Cc: qemu-devel, qemu-ppc, dmitry, peter.maydell, kraxel, mst,
pbonzini, veroniabahaa, quintela, amit.shah, mreitz, kwolf, rth,
aurelien, leon.alrae, blauwirbel, mark.cave-ayland, mdroth
[-- Attachment #1: Type: text/plain, Size: 9864 bytes --]
On Thu, Jun 02, 2016 at 05:28:45PM -0700, Jianjun Duan wrote:
>
>
> On 06/01/2016 09:07 PM, David Gibson wrote:
> > On Tue, May 31, 2016 at 11:02:39AM -0700, Jianjun Duan wrote:
> >> To manage hotplug/unplug of dynamic resources such as PCI cards,
> >> memory, and CPU on sPAPR guests, a firmware abstraction known as
> >> a Dynamic Resource Connector (DRC) is used to assign a particular
> >> dynamic resource to the guest, and provide an interface for the
> >> guest to manage configuration/removal of the resource associated
> >> with it.
> >>
> >> To migrate the hotplugged resources in migration, the
> >> associated DRC state need be migrated. 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
> >> ones modifiable or needed by guest actions or device add/remove
> >> operation are migrated. From the perspective of device
> >> hotplugging, if we hotplug a device on the source, we need to
> >> "coldplug" it on the target. The states across two hosts for the
> >> same device are not the same. Ideally we want the states be same
> >> after migration so that the device would function as hotplugged
> >> on the target. For example we can unplug it. The minimum DRC
> >> state we need to transfer should cover all the pieces changed by
> >> hotplugging. Out of the elements of the DRC state, isolation_state,
> >> allocation_sate, and configured are involved in the DR state
> >> transition diagram from PAPR+ 2.7, 13.4. configured and signalled
> >> are needed in attaching and detaching devices. indicator_state
> >> provides users with hardware state information. These 6 elements
> >> are migrated.
> >>
> >> detach_cb in the DRC state is a function pointer that cannot be
> >> migrated. We set it right after DRC state is migrated so that
> >> a migrated hot-unplug event could finish its work.
> >>
> >> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
> >
> > So, the thing which concerns me about this patch is that it exposes
> > the DRCs as real objects in the migration stream, which will make it
> > difficult to ever remove them in the future.
> >
> > Particularly for LMBs having full QOM objects for every individual DRC
> > is kind of clunky, and I'd already considered replacing them instead
> > with a QOM interface implementing a whole array of DRCs as part of the
> > "owning" object (machine or PHB).
> >
> > The internal structure of the VMSD looks ok to me, but the
> > complication is the automatic "addressing" of it as a separate
> > object. If we could manually take that over, allowing us to direct
> > the same VMSD data at elements in a simpler DRC array later, that
> > would be preferable.
>
> We can address this concern by setting the unique instance_id for each
> drc. The instance_id can be used to match the drcs from source and
> target.
Ah, yes, that sounds like it should work. And we already have a DRC
id value we can use for the purpose.
> >
> >
> >> ---
> >> hw/ppc/spapr_drc.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++
> >> hw/ppc/spapr_pci.c | 22 +++++++++++++++++
> >> include/hw/ppc/spapr_drc.h | 9 +++++++
> >> 3 files changed, 92 insertions(+)
> >>
> >> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> >> index 94c875d..1fb5e23 100644
> >> --- a/hw/ppc/spapr_drc.c
> >> +++ b/hw/ppc/spapr_drc.c
> >> @@ -617,6 +617,65 @@ static void spapr_dr_connector_instance_init(Object *obj)
> >> NULL, NULL, NULL, NULL);
> >> }
> >>
> >> +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) {
> >> + /* for PCI 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;
> >> + /* for LMB type */
> >> + 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;
> >> + default:
> >> + ;
> >> + }
> >> +
> >> + return rc;
> >> +}
> >> +
> >> +/* detach_cb needs be set since it is not migrated */
> >> +static void postmigrate_set_detach_cb(sPAPRDRConnector *drc,
> >> + spapr_drc_detach_cb *detach_cb)
> >> +{
> >> + drc->detach_cb = detach_cb;
> >> +}
> >> +
> >> +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(signalled, sPAPRDRConnector),
> >> + VMSTATE_END_OF_LIST()
> >> + }
> >> +};
> >> +
> >> static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> >> {
> >> DeviceClass *dk = DEVICE_CLASS(k);
> >> @@ -625,6 +684,7 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> >> dk->reset = reset;
> >> dk->realize = realize;
> >> dk->unrealize = unrealize;
> >> + dk->vmsd = &vmstate_spapr_drc;
> >> drck->set_isolation_state = set_isolation_state;
> >> drck->set_indicator_state = set_indicator_state;
> >> drck->set_allocation_state = set_allocation_state;
> >> @@ -638,6 +698,7 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> >> drck->detach = detach;
> >> drck->release_pending = release_pending;
> >> drck->set_signalled = set_signalled;
> >> + drck->postmigrate_set_detach_cb = postmigrate_set_detach_cb;
> >> /*
> >> * Reason: it crashes FIXME find and document the real reason
> >> */
> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> >> index 856aec7..c68da08 100644
> >> --- a/hw/ppc/spapr_pci.c
> >> +++ b/hw/ppc/spapr_pci.c
> >> @@ -1562,11 +1562,33 @@ static void spapr_pci_pre_save(void *opaque)
> >> }
> >> }
> >>
> >> +/*
> >> + * detach_cb in the DRC state is a function pointer that cannot be
> >> + * migrated. We set it right after migration so that a migrated
> >> + * hot-unplug event could finish its work.
> >> + */
> >> +static void spapr_pci_set_detach_cb(PCIBus *bus, PCIDevice *pdev,
> >> + void *opaque)
> >> +{
> >> + sPAPRPHBState *sphb = opaque;
> >> + sPAPRDRConnector *drc = spapr_phb_get_pci_drc(sphb, pdev);
> >> + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> >> + drck->postmigrate_set_detach_cb(drc, spapr_phb_remove_pci_device_cb);
> >> +}
> >> +
> >> static int spapr_pci_post_load(void *opaque, int version_id)
> >> {
> >> sPAPRPHBState *sphb = opaque;
> >> gpointer key, value;
> >> int i;
> >> + PCIBus *bus = PCI_HOST_BRIDGE(sphb)->bus;
> >> + unsigned int bus_no = 0;
> >> +
> >> + /* Set detach_cb for the drc unconditionally after migration */
> >> + if (bus) {
> >> + pci_for_each_device(bus, pci_bus_num(bus), spapr_pci_set_detach_cb,
> >> + &bus_no);
> >> + }
> >>
> >> for (i = 0; i < sphb->msi_devs_num; ++i) {
> >> key = g_memdup(&sphb->msi_devs[i].key,
> >> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> >> index fa21ba0..a68433b 100644
> >> --- a/include/hw/ppc/spapr_drc.h
> >> +++ b/include/hw/ppc/spapr_drc.h
> >> @@ -190,6 +190,15 @@ typedef struct sPAPRDRConnectorClass {
> >> void *detach_cb_opaque, Error **errp);
> >> bool (*release_pending)(sPAPRDRConnector *drc);
> >> void (*set_signalled)(sPAPRDRConnector *drc);
> >> +
> >> + /*
> >> + * QEMU interface for setting detach_cb after migration.
> >> + * detach_cb in the DRC state is a function pointer that cannot be
> >> + * migrated. We set it right after migration so that a migrated
> >> + * hot-unplug event could finish its work.
> >> + */
> >> + void (*postmigrate_set_detach_cb)(sPAPRDRConnector *drc,
> >> + spapr_drc_detach_cb *detach_cb);
> >> } sPAPRDRConnectorClass;
> >>
> >> sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> >
> Thanks,
> Jianjun
>
--
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 --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
2016-06-02 15:01 ` Sascha Silbe
2016-06-02 16:11 ` Jianjun Duan
@ 2016-06-03 17:12 ` Jianjun Duan
2016-06-06 13:47 ` Paolo Bonzini
2 siblings, 0 replies; 27+ messages in thread
From: Jianjun Duan @ 2016-06-03 17:12 UTC (permalink / raw)
To: Sascha Silbe, qemu-devel
Cc: veroniabahaa, peter.maydell, mst, quintela, mark.cave-ayland,
mdroth, mreitz, blauwirbel, amit.shah, qemu-ppc, kraxel, kwolf,
dmitry, pbonzini, rth, leon.alrae, aurelien, david
On 06/02/2016 08:01 AM, Sascha Silbe wrote:
> Dear Jianjun,
>
> Jianjun Duan <duanj@linux.vnet.ibm.com> writes:
>
> [include/migration/vmstate.h]
>> @@ -185,6 +185,8 @@ enum VMStateFlags {
>> * to determine the number of entries in the array. Only valid in
>> * combination with one of VMS_VARRAY*. */
>> VMS_MULTIPLY_ELEMENTS = 0x4000,
>> + /* For fields which need customized handling, such as QTAILQ in queue.h*/
>> + VMS_CSTM = 0x8000,
>
> Can you describe (in the comment) how this customised handling is
> performed, please? I.e. describe what exactly happens if the flag is
> set, from the point of view of an API consumer.
>
> Also, why do you need this flag at all? The only change I can see is
> that you pass additional information to VMStateInfo.get() / .put(),
> using NULL if it's not set. Why don't you just always pass the
> additional information? If the additional information is not needed by
> get() / put() the parameter will be unused anyway.
>
I now agree with you after some thought.
> Sascha
>
Thanks,
Jianjun
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
2016-06-02 15:01 ` Sascha Silbe
2016-06-02 16:11 ` Jianjun Duan
2016-06-03 17:12 ` [Qemu-devel] " Jianjun Duan
@ 2016-06-06 13:47 ` Paolo Bonzini
2 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2016-06-06 13:47 UTC (permalink / raw)
To: Sascha Silbe, Jianjun Duan, qemu-devel
Cc: veroniabahaa, peter.maydell, mst, quintela, mark.cave-ayland,
mdroth, mreitz, blauwirbel, amit.shah, qemu-ppc, kraxel, kwolf,
dmitry, rth, leon.alrae, aurelien, david
On 02/06/2016 17:01, Sascha Silbe wrote:
>> > + /* For fields which need customized handling, such as QTAILQ in queue.h*/
>> > + VMS_CSTM = 0x8000,
> Can you describe (in the comment) how this customised handling is
> performed, please? I.e. describe what exactly happens if the flag is
> set, from the point of view of an API consumer.
>
> Also, why do you need this flag at all? The only change I can see is
> that you pass additional information to VMStateInfo.get() / .put(),
> using NULL if it's not set. Why don't you just always pass the
> additional information? If the additional information is not needed by
> get() / put() the parameter will be unused anyway.
Having the flag seemed like a good idea to me, because it gives a
specific meaning to the offset fields in the VMStateField. On the other
hand, it's true that it's unused...
Paolo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
2016-06-02 16:11 ` Jianjun Duan
@ 2016-06-07 14:43 ` Dr. David Alan Gilbert
2016-06-07 16:31 ` Paolo Bonzini
2016-06-07 16:43 ` [Qemu-devel] [Qemu-ppc] " Jianjun Duan
0 siblings, 2 replies; 27+ messages in thread
From: Dr. David Alan Gilbert @ 2016-06-07 14:43 UTC (permalink / raw)
To: Jianjun Duan
Cc: Sascha Silbe, qemu-devel, veroniabahaa, peter.maydell, quintela,
mst, mark.cave-ayland, mdroth, mreitz, blauwirbel, dmitry,
qemu-ppc, kraxel, pbonzini, amit.shah, kwolf, david, leon.alrae,
aurelien, rth
* Jianjun Duan (duanj@linux.vnet.ibm.com) wrote:
> Hi Sascha,
>
> On 06/02/2016 08:01 AM, Sascha Silbe wrote:
> > Dear Jianjun,
> >
> > Jianjun Duan <duanj@linux.vnet.ibm.com> writes:
> >
> > [include/migration/vmstate.h]
> >> @@ -185,6 +185,8 @@ enum VMStateFlags {
> >> * to determine the number of entries in the array. Only valid in
> >> * combination with one of VMS_VARRAY*. */
> >> VMS_MULTIPLY_ELEMENTS = 0x4000,
> >> + /* For fields which need customized handling, such as QTAILQ in queue.h*/
> >> + VMS_CSTM = 0x8000,
> >
> > Can you describe (in the comment) how this customised handling is
> > performed, please? I.e. describe what exactly happens if the flag is
> > set, from the point of view of an API consumer.
>
> I will add more comments. When this flag is set VMStateInfo.get/put will
> be used in vmstate_load/save_state instead of recursive call. And the
> user should implement VMStateInfo.get/put to handle the concerned data
> structure.
> > Also, why do you need this flag at all? The only change I can see is
> > that you pass additional information to VMStateInfo.get() / .put(),
> > using NULL if it's not set. Why don't you just always pass the
> > additional information? If the additional information is not needed by
> > get() / put() the parameter will be unused anyway.
> You can do it without creating this flag. Instead just to check if info
> is set in the field. However I think it is more readable and more robust
> to check this flag in vmstate_load/get_state.
Apologies for not getting around to this sooner; I was on holiday when you first
sent it.
But:
a) Is there a reason to use the 'bool' between each element; can't you count the
length of the queue, send the length and then send the contents? In that case
it should look a lot more like an ARRAY case on the wire.
b) I think you should really try and split it into two parts:
1) A VMSTATE_ARRAY_CUSTOM (?) - so it's an array of elements but we've got a special
way of allocating/counting/reading the elements
2) A version of that which is for a QTAILQ.
It's important that whatever ends up on the migration stream doesn't reflect
that it happens to be implemented as a QTAILQ; so if you decide to change
it later the migration compatibility doesn't break.
c) Use new trace_ names for get_qtailq rather than misusing trace_vmstate_load_state*
d) Add a trace_ for put_qtailq as well - that way when it goes horribly wrong we can
turn the tracing on on both sides :-)
e) Is there anyway to make QTAILQ_RAW_INSERT_TAIL any more readable?
I don't think I understand why you can't use the standard QTAILQ macros.
f) You might need to fix up Amit's scripts/vmstate-static-checker.py
Dave
> > Sascha
> >
>
> Thanks,
> Jianjun
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
2016-06-07 14:43 ` Dr. David Alan Gilbert
@ 2016-06-07 16:31 ` Paolo Bonzini
2016-06-07 16:33 ` Michael S. Tsirkin
2016-06-07 16:34 ` Dr. David Alan Gilbert
2016-06-07 16:43 ` [Qemu-devel] [Qemu-ppc] " Jianjun Duan
1 sibling, 2 replies; 27+ messages in thread
From: Paolo Bonzini @ 2016-06-07 16:31 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Jianjun Duan
Cc: Sascha Silbe, qemu-devel, veroniabahaa, peter.maydell, quintela,
mst, mark.cave-ayland, mdroth, mreitz, blauwirbel, dmitry,
qemu-ppc, kraxel, amit.shah, kwolf, david, leon.alrae, aurelien,
rth
On 07/06/2016 16:43, Dr. David Alan Gilbert wrote:
> b) I think you should really try and split it into two parts:
> 1) A VMSTATE_ARRAY_CUSTOM (?) - so it's an array of elements but we've got a special
> way of allocating/counting/reading the elements
> 2) A version of that which is for a QTAILQ.
> It's important that whatever ends up on the migration stream doesn't reflect
> that it happens to be implemented as a QTAILQ; so if you decide to change
> it later the migration compatibility doesn't break.
(Just to clarify before Jianjun wakes up) a VMSTATE_ARRAY is just a
sequence of elements. The count, if needed as in a VARRAY, is stored
earlier and separately. Currently lists (including this QTAILQ) are
usually represented in the migration stream as a sequence of elements
preceded by "1" and terminated by a "0". Would you like to change it to
a count + sequence as well?
This would prevent using the new QTAILQ support for other existing lists
such as virtio-blk and scsi's request lists.
> c) Use new trace_ names for get_qtailq rather than misusing trace_vmstate_load_state*
> d) Add a trace_ for put_qtailq as well - that way when it goes horribly wrong we can
> turn the tracing on on both sides :-)
> e) Is there anyway to make QTAILQ_RAW_INSERT_TAIL any more readable?
> I don't think I understand why you can't use the standard QTAILQ macros.
Because they assume a particular type. The "raw" version need to work on
void*.
Thanks,
Paolo
> f) You might need to fix up Amit's scripts/vmstate-static-checker.py
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
2016-06-07 16:31 ` Paolo Bonzini
@ 2016-06-07 16:33 ` Michael S. Tsirkin
2016-06-07 16:34 ` Dr. David Alan Gilbert
1 sibling, 0 replies; 27+ messages in thread
From: Michael S. Tsirkin @ 2016-06-07 16:33 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Dr. David Alan Gilbert, Jianjun Duan, Sascha Silbe, qemu-devel,
veroniabahaa, peter.maydell, quintela, mark.cave-ayland, mdroth,
mreitz, blauwirbel, dmitry, qemu-ppc, kraxel, amit.shah, kwolf,
david, leon.alrae, aurelien, rth
On Tue, Jun 07, 2016 at 06:31:41PM +0200, Paolo Bonzini wrote:
>
>
> On 07/06/2016 16:43, Dr. David Alan Gilbert wrote:
> > b) I think you should really try and split it into two parts:
> > 1) A VMSTATE_ARRAY_CUSTOM (?) - so it's an array of elements but we've got a special
> > way of allocating/counting/reading the elements
> > 2) A version of that which is for a QTAILQ.
> > It's important that whatever ends up on the migration stream doesn't reflect
> > that it happens to be implemented as a QTAILQ; so if you decide to change
> > it later the migration compatibility doesn't break.
>
> (Just to clarify before Jianjun wakes up) a VMSTATE_ARRAY is just a
> sequence of elements. The count, if needed as in a VARRAY, is stored
> earlier and separately.
And if you migrate the count, you must validate that
it's not bigger than an actual array size before VARRAY.
> Currently lists (including this QTAILQ) are
> usually represented in the migration stream as a sequence of elements
> preceded by "1" and terminated by a "0". Would you like to change it to
> a count + sequence as well?
>
> This would prevent using the new QTAILQ support for other existing lists
> such as virtio-blk and scsi's request lists.
>
> > c) Use new trace_ names for get_qtailq rather than misusing trace_vmstate_load_state*
> > d) Add a trace_ for put_qtailq as well - that way when it goes horribly wrong we can
> > turn the tracing on on both sides :-)
> > e) Is there anyway to make QTAILQ_RAW_INSERT_TAIL any more readable?
> > I don't think I understand why you can't use the standard QTAILQ macros.
>
> Because they assume a particular type. The "raw" version need to work on
> void*.
>
> Thanks,
>
> Paolo
>
> > f) You might need to fix up Amit's scripts/vmstate-static-checker.py
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
2016-06-07 16:31 ` Paolo Bonzini
2016-06-07 16:33 ` Michael S. Tsirkin
@ 2016-06-07 16:34 ` Dr. David Alan Gilbert
2016-06-07 16:35 ` Paolo Bonzini
1 sibling, 1 reply; 27+ messages in thread
From: Dr. David Alan Gilbert @ 2016-06-07 16:34 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Jianjun Duan, Sascha Silbe, qemu-devel, veroniabahaa,
peter.maydell, quintela, mst, mark.cave-ayland, mdroth, mreitz,
blauwirbel, dmitry, qemu-ppc, kraxel, amit.shah, kwolf, david,
leon.alrae, aurelien, rth
* Paolo Bonzini (pbonzini@redhat.com) wrote:
>
>
> On 07/06/2016 16:43, Dr. David Alan Gilbert wrote:
> > b) I think you should really try and split it into two parts:
> > 1) A VMSTATE_ARRAY_CUSTOM (?) - so it's an array of elements but we've got a special
> > way of allocating/counting/reading the elements
> > 2) A version of that which is for a QTAILQ.
> > It's important that whatever ends up on the migration stream doesn't reflect
> > that it happens to be implemented as a QTAILQ; so if you decide to change
> > it later the migration compatibility doesn't break.
>
> (Just to clarify before Jianjun wakes up) a VMSTATE_ARRAY is just a
> sequence of elements. The count, if needed as in a VARRAY, is stored
> earlier and separately. Currently lists (including this QTAILQ) are
> usually represented in the migration stream as a sequence of elements
> preceded by "1" and terminated by a "0". Would you like to change it to
> a count + sequence as well?
>
> This would prevent using the new QTAILQ support for other existing lists
> such as virtio-blk and scsi's request lists.
That depends if it's something you're trying to keep migration compatibility
with; if you're trying to keep format compaibility then sure keep it as is.
If you're not trying to keep compatibility, then why can't virtio-blk or
scsi request lists also use a count rather than the markers?
> > c) Use new trace_ names for get_qtailq rather than misusing trace_vmstate_load_state*
> > d) Add a trace_ for put_qtailq as well - that way when it goes horribly wrong we can
> > turn the tracing on on both sides :-)
> > e) Is there anyway to make QTAILQ_RAW_INSERT_TAIL any more readable?
> > I don't think I understand why you can't use the standard QTAILQ macros.
>
> Because they assume a particular type. The "raw" version need to work on
> void*.
Ah OK.
Dave
>
> Thanks,
>
> Paolo
>
> > f) You might need to fix up Amit's scripts/vmstate-static-checker.py
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
2016-06-07 16:34 ` Dr. David Alan Gilbert
@ 2016-06-07 16:35 ` Paolo Bonzini
2016-06-07 16:46 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2016-06-07 16:35 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Jianjun Duan, Sascha Silbe, qemu-devel, veroniabahaa,
peter.maydell, quintela, mst, mark.cave-ayland, mdroth, mreitz,
blauwirbel, dmitry, qemu-ppc, kraxel, amit.shah, kwolf, david,
leon.alrae, aurelien, rth
On 07/06/2016 18:34, Dr. David Alan Gilbert wrote:
> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>>
>>
>> On 07/06/2016 16:43, Dr. David Alan Gilbert wrote:
>>> b) I think you should really try and split it into two parts:
>>> 1) A VMSTATE_ARRAY_CUSTOM (?) - so it's an array of elements but we've got a special
>>> way of allocating/counting/reading the elements
>>> 2) A version of that which is for a QTAILQ.
>>> It's important that whatever ends up on the migration stream doesn't reflect
>>> that it happens to be implemented as a QTAILQ; so if you decide to change
>>> it later the migration compatibility doesn't break.
>>
>> (Just to clarify before Jianjun wakes up) a VMSTATE_ARRAY is just a
>> sequence of elements. The count, if needed as in a VARRAY, is stored
>> earlier and separately. Currently lists (including this QTAILQ) are
>> usually represented in the migration stream as a sequence of elements
>> preceded by "1" and terminated by a "0". Would you like to change it to
>> a count + sequence as well?
>>
>> This would prevent using the new QTAILQ support for other existing lists
>> such as virtio-blk and scsi's request lists.
>
> That depends if it's something you're trying to keep migration compatibility
> with; if you're trying to keep format compaibility then sure keep it as is.
> If you're not trying to keep compatibility, then why can't virtio-blk or
> scsi request lists also use a count rather than the markers?
We're trying to keep compatibility, and I think it's among the last bits
that are resisting conversion to vmstate. Which explains my interest in
Jianjun's patches. :)
Paolo
>>> c) Use new trace_ names for get_qtailq rather than misusing trace_vmstate_load_state*
>>> d) Add a trace_ for put_qtailq as well - that way when it goes horribly wrong we can
>>> turn the tracing on on both sides :-)
>>> e) Is there anyway to make QTAILQ_RAW_INSERT_TAIL any more readable?
>>> I don't think I understand why you can't use the standard QTAILQ macros.
>>
>> Because they assume a particular type. The "raw" version need to work on
>> void*.
>
> Ah OK.
>
> Dave
>
>>
>> Thanks,
>>
>> Paolo
>>
>>> f) You might need to fix up Amit's scripts/vmstate-static-checker.py
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
2016-06-07 14:43 ` Dr. David Alan Gilbert
2016-06-07 16:31 ` Paolo Bonzini
@ 2016-06-07 16:43 ` Jianjun Duan
1 sibling, 0 replies; 27+ messages in thread
From: Jianjun Duan @ 2016-06-07 16:43 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Sascha Silbe, qemu-devel, veroniabahaa, peter.maydell, quintela,
mst, mark.cave-ayland, mdroth, mreitz, blauwirbel, dmitry,
qemu-ppc, kraxel, pbonzini, amit.shah, kwolf, david, leon.alrae,
aurelien, rth
On 06/07/2016 07:43 AM, Dr. David Alan Gilbert wrote:
> * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote:
>> Hi Sascha,
>>
>> On 06/02/2016 08:01 AM, Sascha Silbe wrote:
>>> Dear Jianjun,
>>>
>>> Jianjun Duan <duanj@linux.vnet.ibm.com> writes:
>>>
>>> [include/migration/vmstate.h]
>>>> @@ -185,6 +185,8 @@ enum VMStateFlags {
>>>> * to determine the number of entries in the array. Only valid in
>>>> * combination with one of VMS_VARRAY*. */
>>>> VMS_MULTIPLY_ELEMENTS = 0x4000,
>>>> + /* For fields which need customized handling, such as QTAILQ in queue.h*/
>>>> + VMS_CSTM = 0x8000,
>>>
>>> Can you describe (in the comment) how this customised handling is
>>> performed, please? I.e. describe what exactly happens if the flag is
>>> set, from the point of view of an API consumer.
>>
>> I will add more comments. When this flag is set VMStateInfo.get/put will
>> be used in vmstate_load/save_state instead of recursive call. And the
>> user should implement VMStateInfo.get/put to handle the concerned data
>> structure.
>>> Also, why do you need this flag at all? The only change I can see is
>>> that you pass additional information to VMStateInfo.get() / .put(),
>>> using NULL if it's not set. Why don't you just always pass the
>>> additional information? If the additional information is not needed by
>>> get() / put() the parameter will be unused anyway.
>> You can do it without creating this flag. Instead just to check if info
>> is set in the field. However I think it is more readable and more robust
>> to check this flag in vmstate_load/get_state.
>
> Apologies for not getting around to this sooner; I was on holiday when you first
> sent it.
>
> But:
> a) Is there a reason to use the 'bool' between each element; can't you count the
> length of the queue, send the length and then send the contents? In that case
> it should look a lot more like an ARRAY case on the wire.
> b) I think you should really try and split it into two parts:
> 1) A VMSTATE_ARRAY_CUSTOM (?) - so it's an array of elements but we've got a special
> way of allocating/counting/reading the elements
> 2) A version of that which is for a QTAILQ.
> It's important that whatever ends up on the migration stream doesn't reflect
> that it happens to be implemented as a QTAILQ; so if you decide to change
> it later the migration compatibility doesn't break.
There are certainly more than one way to do this. I have thought about
the way you suggested and decided not to do it that way. We either need
to track its size, which means new element in the structure containing
the queue, or go through the queue and count it. I don't want to add an
element unless it is necessary. Counting and dumping the queue will go
through the queue twice. Current solution only goes through the queue
once. Besides, the queue is recursive and current solution is a natural
one.
> c) Use new trace_ names for get_qtailq rather than misusing trace_vmstate_load_state*
> d) Add a trace_ for put_qtailq as well - that way when it goes horribly wrong we can
> turn the tracing on on both sides :-)
> e) Is there anyway to make QTAILQ_RAW_INSERT_TAIL any more readable?
> I don't think I understand why you can't use the standard QTAILQ macros.
I agree with this one. Need to make the trace more specific.
> f) You might need to fix up Amit's scripts/vmstate-static-checker.py
Will check it.
>
> Dave
>
>>> Sascha
>>>
>>
>> Thanks,
>> Jianjun
>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
Thanks,
Jianjun
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ
2016-06-07 16:35 ` Paolo Bonzini
@ 2016-06-07 16:46 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 27+ messages in thread
From: Dr. David Alan Gilbert @ 2016-06-07 16:46 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Jianjun Duan, Sascha Silbe, qemu-devel, veroniabahaa,
peter.maydell, quintela, mst, mark.cave-ayland, mdroth, mreitz,
blauwirbel, dmitry, qemu-ppc, kraxel, amit.shah, kwolf, david,
leon.alrae, aurelien, rth
* Paolo Bonzini (pbonzini@redhat.com) wrote:
>
>
> On 07/06/2016 18:34, Dr. David Alan Gilbert wrote:
> > * Paolo Bonzini (pbonzini@redhat.com) wrote:
> >>
> >>
> >> On 07/06/2016 16:43, Dr. David Alan Gilbert wrote:
> >>> b) I think you should really try and split it into two parts:
> >>> 1) A VMSTATE_ARRAY_CUSTOM (?) - so it's an array of elements but we've got a special
> >>> way of allocating/counting/reading the elements
> >>> 2) A version of that which is for a QTAILQ.
> >>> It's important that whatever ends up on the migration stream doesn't reflect
> >>> that it happens to be implemented as a QTAILQ; so if you decide to change
> >>> it later the migration compatibility doesn't break.
> >>
> >> (Just to clarify before Jianjun wakes up) a VMSTATE_ARRAY is just a
> >> sequence of elements. The count, if needed as in a VARRAY, is stored
> >> earlier and separately. Currently lists (including this QTAILQ) are
> >> usually represented in the migration stream as a sequence of elements
> >> preceded by "1" and terminated by a "0". Would you like to change it to
> >> a count + sequence as well?
> >>
> >> This would prevent using the new QTAILQ support for other existing lists
> >> such as virtio-blk and scsi's request lists.
> >
> > That depends if it's something you're trying to keep migration compatibility
> > with; if you're trying to keep format compaibility then sure keep it as is.
> > If you're not trying to keep compatibility, then why can't virtio-blk or
> > scsi request lists also use a count rather than the markers?
>
> We're trying to keep compatibility, and I think it's among the last bits
> that are resisting conversion to vmstate. Which explains my interest in
> Jianjun's patches. :)
OK, fine - if you're trying to keep format compatibility then I agree.
Although I'm not entirely sure that things like virtio-blk, scsi and everything else
are consistent in their structure.
Dave
>
> Paolo
>
> >>> c) Use new trace_ names for get_qtailq rather than misusing trace_vmstate_load_state*
> >>> d) Add a trace_ for put_qtailq as well - that way when it goes horribly wrong we can
> >>> turn the tracing on on both sides :-)
> >>> e) Is there anyway to make QTAILQ_RAW_INSERT_TAIL any more readable?
> >>> I don't think I understand why you can't use the standard QTAILQ macros.
> >>
> >> Because they assume a particular type. The "raw" version need to work on
> >> void*.
> >
> > Ah OK.
> >
> > Dave
> >
> >>
> >> Thanks,
> >>
> >> Paolo
> >>
> >>> f) You might need to fix up Amit's scripts/vmstate-static-checker.py
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2016-06-07 16:46 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-31 18:02 [Qemu-devel] [QEMU RFC PATCH v3 0/6]Migration: ensure hotplug and migration work together Jianjun Duan
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 1/6] Migration: Defined VMStateDescription struct for spapr_drc Jianjun Duan
2016-06-02 4:07 ` David Gibson
2016-06-03 0:28 ` Jianjun Duan
2016-06-03 1:48 ` David Gibson
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 2/6] vmstate: Define VARRAY with VMS_ALLOC Jianjun Duan
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 3/6] Migration: extend VMStateInfo Jianjun Duan
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ Jianjun Duan
2016-05-31 19:54 ` Paolo Bonzini
2016-05-31 21:53 ` Jianjun Duan
2016-06-01 15:29 ` Paolo Bonzini
2016-06-01 17:06 ` Jianjun Duan
2016-06-01 18:32 ` Paolo Bonzini
2016-06-02 3:58 ` David Gibson
2016-06-02 15:01 ` Sascha Silbe
2016-06-02 16:11 ` Jianjun Duan
2016-06-07 14:43 ` Dr. David Alan Gilbert
2016-06-07 16:31 ` Paolo Bonzini
2016-06-07 16:33 ` Michael S. Tsirkin
2016-06-07 16:34 ` Dr. David Alan Gilbert
2016-06-07 16:35 ` Paolo Bonzini
2016-06-07 16:46 ` Dr. David Alan Gilbert
2016-06-07 16:43 ` [Qemu-devel] [Qemu-ppc] " Jianjun Duan
2016-06-03 17:12 ` [Qemu-devel] " Jianjun Duan
2016-06-06 13:47 ` Paolo Bonzini
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 5/6] Migration: migrate ccs_list in spapr state Jianjun Duan
2016-05-31 18:02 ` [Qemu-devel] [QEMU RFC PATCH v3 6/6] Migration: migrate pending_events of " Jianjun Duan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.