* [Qemu-devel] [PATCH 00/10] migration: s390x css migration
@ 2017-05-05 17:34 Halil Pasic
2017-05-05 17:34 ` [Qemu-devel] [PATCH 01/10] s390x: add helper get_machine_class Halil Pasic
` (9 more replies)
0 siblings, 10 replies; 42+ messages in thread
From: Halil Pasic @ 2017-05-05 17:34 UTC (permalink / raw)
To: Cornelia Huck
Cc: Michael S. Tsirkin, Dr. David Alan Gilbert, qemu-devel, Halil Pasic
This patch set has a dual purpose: introduce proper migration for the
channel subsystem (css) and convert the migration of ccw devices to
vmstate. The state of the css was only partially migrated by the
virtio-ccw devices (was the only device type until recent), which was
good enough for a while.
Since the new vmstate infrastructure is used also for handling the
migration stream for legacy machines some bits are a bit convoluted:
that is, if you see something convoluted there is a good chance the
reason why is compatibility.
Halil Pasic (10):
s390x: add helper get_machine_class
s390x: add css_migration_enabled to machine class
s390x/css: add vmstate entities for css
s390x/css: add vmstate macro for CcwDevice
virtio-ccw: add vmstate entities for VirtioCcwDevice
virtio-ccw: use vmstate way for config migration
s390x/css: remove unused subch_dev_(load|save)
s390x/css: add ORB to SubchDev
s390x/css: turn on channel subsystem migration
s390x/css: use SubchDev.orb
hw/intc/s390_flic.c | 48 ++++
hw/s390x/ccw-device.c | 11 +
hw/s390x/ccw-device.h | 4 +
hw/s390x/css.c | 478 +++++++++++++++++++++++++------------
hw/s390x/s390-virtio-ccw.c | 58 +++--
hw/s390x/virtio-ccw.c | 164 +++++++------
include/hw/s390x/css.h | 17 +-
include/hw/s390x/s390-virtio-ccw.h | 7 +
include/hw/s390x/s390_flic.h | 5 +
9 files changed, 543 insertions(+), 249 deletions(-)
--
2.10.2
^ permalink raw reply [flat|nested] 42+ messages in thread
* [Qemu-devel] [PATCH 01/10] s390x: add helper get_machine_class
2017-05-05 17:34 [Qemu-devel] [PATCH 00/10] migration: s390x css migration Halil Pasic
@ 2017-05-05 17:34 ` Halil Pasic
2017-05-05 17:34 ` [Qemu-devel] [PATCH 02/10] s390x: add css_migration_enabled to machine class Halil Pasic
` (8 subsequent siblings)
9 siblings, 0 replies; 42+ messages in thread
From: Halil Pasic @ 2017-05-05 17:34 UTC (permalink / raw)
To: Cornelia Huck
Cc: Michael S. Tsirkin, Dr. David Alan Gilbert, qemu-devel, Halil Pasic
We will need the machine class at machine initialization time, so the
usual way via qdev won't do. Let's cache the machine class and also use
the default values of the base machine for capability discovery.
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
hw/s390x/s390-virtio-ccw.c | 46 +++++++++++++++++++++++-----------------------
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 04bd0eb..7a053a4 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -242,36 +242,35 @@ static inline void machine_set_dea_key_wrap(Object *obj, bool value,
ms->dea_key_wrap = value;
}
-bool ri_allowed(void)
-{
- if (kvm_enabled()) {
- MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
- if (object_class_dynamic_cast(OBJECT_CLASS(mc),
- TYPE_S390_CCW_MACHINE)) {
- S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
+static S390CcwMachineClass *current_mc;
- return s390mc->ri_allowed;
- }
+static S390CcwMachineClass *get_machine_class(void)
+{
+ if (unlikely(!current_mc)) {
/*
- * Make sure the "none" machine can have ri, otherwise it won't * be
- * unlocked in KVM and therefore the host CPU model might be wrong.
- */
- return true;
+ * No s390 ccw machine was instantiated, we are likely to
+ * be called for the 'none' machine. The properties will
+ * have their after-initialization values.
+ */
+ current_mc = S390_MACHINE_CLASS(
+ object_class_by_name(TYPE_S390_CCW_MACHINE));
}
- return 0;
+ return current_mc;
}
-bool cpu_model_allowed(void)
+bool ri_allowed(void)
{
- MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
- if (object_class_dynamic_cast(OBJECT_CLASS(mc),
- TYPE_S390_CCW_MACHINE)) {
- S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
-
- return s390mc->cpu_model_allowed;
+ if (!kvm_enabled()) {
+ return false;
}
- /* allow CPU model qmp queries with the "none" machine */
- return true;
+ /* for "none" machine this results in true */
+ return get_machine_class()->ri_allowed;
+}
+
+bool cpu_model_allowed(void)
+{
+ /* for "none" machine this results in true */
+ return get_machine_class()->cpu_model_allowed;
}
static inline void s390_machine_initfn(Object *obj)
@@ -323,6 +322,7 @@ static const TypeInfo ccw_machine_info = {
static void ccw_machine_##suffix##_instance_init(Object *obj) \
{ \
MachineState *machine = MACHINE(obj); \
+ current_mc = S390_MACHINE_CLASS(MACHINE_GET_CLASS(machine)); \
ccw_machine_##suffix##_instance_options(machine); \
} \
static const TypeInfo ccw_machine_##suffix##_info = { \
--
2.10.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [Qemu-devel] [PATCH 02/10] s390x: add css_migration_enabled to machine class
2017-05-05 17:34 [Qemu-devel] [PATCH 00/10] migration: s390x css migration Halil Pasic
2017-05-05 17:34 ` [Qemu-devel] [PATCH 01/10] s390x: add helper get_machine_class Halil Pasic
@ 2017-05-05 17:34 ` Halil Pasic
2017-05-05 17:35 ` [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css Halil Pasic
` (7 subsequent siblings)
9 siblings, 0 replies; 42+ messages in thread
From: Halil Pasic @ 2017-05-05 17:34 UTC (permalink / raw)
To: Cornelia Huck
Cc: Michael S. Tsirkin, Dr. David Alan Gilbert, qemu-devel, Halil Pasic
Currently the migration of the channel subsystem (css) is only partial
and is done by the virtio ccw proxies -- the only css devices existing
at the moment. With the current efforts towards emulated and passthrough
devices we need to improve on the migration of the css. This however
will necessarily break the migration compatibility. So let us introduce
a switch (at the machine class) and put it in off state for now. We will
turn the switch on for future machines once all preparations are met.
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
hw/s390x/s390-virtio-ccw.c | 13 +++++++++++++
include/hw/s390x/s390-virtio-ccw.h | 7 +++++++
2 files changed, 20 insertions(+)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 7a053a4..698e8fc 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -196,6 +196,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
s390mc->ri_allowed = true;
s390mc->cpu_model_allowed = true;
+ s390mc->css_migration_enabled = false; /* TODO: set to true */
mc->init = ccw_init;
mc->reset = s390_machine_reset;
mc->hot_add_cpu = s390_hot_add_cpu;
@@ -307,6 +308,11 @@ static const TypeInfo ccw_machine_info = {
},
};
+bool css_migration_enabled(void)
+{
+ return get_machine_class()->css_migration_enabled;
+}
+
#define DEFINE_CCW_MACHINE(suffix, verstr, latest) \
static void ccw_machine_##suffix##_class_init(ObjectClass *oc, \
void *data) \
@@ -408,6 +414,10 @@ static const TypeInfo ccw_machine_info = {
static void ccw_machine_2_10_instance_options(MachineState *machine)
{
+ /*
+ * TODO Once preparations are done register vmstate for the css if
+ * css_migration_enabled().
+ */
}
static void ccw_machine_2_10_class_options(MachineClass *mc)
@@ -422,8 +432,11 @@ static void ccw_machine_2_9_instance_options(MachineState *machine)
static void ccw_machine_2_9_class_options(MachineClass *mc)
{
+ S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
+
ccw_machine_2_10_class_options(mc);
SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_9);
+ s390mc->css_migration_enabled = false;
}
DEFINE_CCW_MACHINE(2_9, "2.9", false);
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 6ecae00..b043550 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -37,6 +37,7 @@ typedef struct S390CcwMachineClass {
/*< public >*/
bool ri_allowed;
bool cpu_model_allowed;
+ bool css_migration_enabled;
} S390CcwMachineClass;
/* runtime-instrumentation allowed by the machine */
@@ -44,4 +45,10 @@ bool ri_allowed(void);
/* cpu model allowed by the machine */
bool cpu_model_allowed(void);
+/**
+ * Returns true if (vmstate based) migration of the channel subsystem
+ * is enabled, false if it is disabled.
+ */
+bool css_migration_enabled(void);
+
#endif
--
2.10.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css
2017-05-05 17:34 [Qemu-devel] [PATCH 00/10] migration: s390x css migration Halil Pasic
2017-05-05 17:34 ` [Qemu-devel] [PATCH 01/10] s390x: add helper get_machine_class Halil Pasic
2017-05-05 17:34 ` [Qemu-devel] [PATCH 02/10] s390x: add css_migration_enabled to machine class Halil Pasic
@ 2017-05-05 17:35 ` Halil Pasic
2017-05-08 16:45 ` Dr. David Alan Gilbert
2017-05-05 17:35 ` [Qemu-devel] [PATCH 04/10] s390x/css: add vmstate macro for CcwDevice Halil Pasic
` (6 subsequent siblings)
9 siblings, 1 reply; 42+ messages in thread
From: Halil Pasic @ 2017-05-05 17:35 UTC (permalink / raw)
To: Cornelia Huck
Cc: Michael S. Tsirkin, Dr. David Alan Gilbert, qemu-devel, Halil Pasic
As a preparation for switching to a vmstate based migration let us
introduce vmstate entities (e.g. VMStateDescription) for the css entities
to be migrated. Alongside some comments explaining or indicating the not
migration of certain members are introduced too.
No changes in behavior, we just added some dead code -- which should
rise to life soon.
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
hw/s390x/css.c | 276 +++++++++++++++++++++++++++++++++++++++++++++++++
include/hw/s390x/css.h | 10 +-
2 files changed, 285 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index c03bb20..2bda7d0 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -20,29 +20,231 @@
#include "hw/s390x/css.h"
#include "trace.h"
#include "hw/s390x/s390_flic.h"
+#include "hw/s390x/s390-virtio-ccw.h"
typedef struct CrwContainer {
CRW crw;
QTAILQ_ENTRY(CrwContainer) sibling;
} CrwContainer;
+static const VMStateDescription vmstate_crw = {
+ .name = "s390_crw",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT16(flags, CRW),
+ VMSTATE_UINT16(rsid, CRW),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
+static const VMStateDescription vmstate_crw_container = {
+ .name = "s390_crw_container",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_STRUCT(crw, CrwContainer, 0, vmstate_crw, CRW),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
typedef struct ChpInfo {
uint8_t in_use;
uint8_t type;
uint8_t is_virtual;
} ChpInfo;
+static const VMStateDescription vmstate_chp_info = {
+ .name = "s390_chp_info",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT8(in_use, ChpInfo),
+ VMSTATE_UINT8(type, ChpInfo),
+ VMSTATE_UINT8(is_virtual, ChpInfo),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
typedef struct SubchSet {
SubchDev *sch[MAX_SCHID + 1];
unsigned long schids_used[BITS_TO_LONGS(MAX_SCHID + 1)];
unsigned long devnos_used[BITS_TO_LONGS(MAX_SCHID + 1)];
} SubchSet;
+static const VMStateDescription vmstate_scsw = {
+ .name = "s390_scsw",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT16(flags, SCSW),
+ VMSTATE_UINT16(ctrl, SCSW),
+ VMSTATE_UINT32(cpa, SCSW),
+ VMSTATE_UINT8(dstat, SCSW),
+ VMSTATE_UINT8(cstat, SCSW),
+ VMSTATE_UINT16(count, SCSW),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static const VMStateDescription vmstate_pmcw = {
+ .name = "s390_pmcw",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(intparm, PMCW),
+ VMSTATE_UINT16(flags, PMCW),
+ VMSTATE_UINT16(devno, PMCW),
+ VMSTATE_UINT8(lpm, PMCW),
+ VMSTATE_UINT8(pnom, PMCW),
+ VMSTATE_UINT8(lpum, PMCW),
+ VMSTATE_UINT8(pim, PMCW),
+ VMSTATE_UINT16(mbi, PMCW),
+ VMSTATE_UINT8(pom, PMCW),
+ VMSTATE_UINT8(pam, PMCW),
+ VMSTATE_UINT8_ARRAY(chpid, PMCW, 8),
+ VMSTATE_UINT32(chars, PMCW),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static const VMStateDescription vmstate_schib = {
+ .name = "s390_schib",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_STRUCT(pmcw, SCHIB, 0, vmstate_pmcw, PMCW),
+ VMSTATE_STRUCT(scsw, SCHIB, 0, vmstate_scsw, SCSW),
+ VMSTATE_UINT64(mba, SCHIB),
+ VMSTATE_UINT8_ARRAY(mda, SCHIB, 4),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+
+static const VMStateDescription vmstate_ccw1 = {
+ .name = "s390_ccw1",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT8(cmd_code, CCW1),
+ VMSTATE_UINT8(flags, CCW1),
+ VMSTATE_UINT16(count, CCW1),
+ VMSTATE_UINT32(cda, CCW1),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static const VMStateDescription vmstate_ciw = {
+ .name = "s390_ciw",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT8(type, CIW),
+ VMSTATE_UINT8(command, CIW),
+ VMSTATE_UINT16(count, CIW),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static const VMStateDescription vmstate_sense_id = {
+ .name = "s390_sense_id",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT8(reserved, SenseId),
+ VMSTATE_UINT16(cu_type, SenseId),
+ VMSTATE_UINT8(cu_model, SenseId),
+ VMSTATE_UINT16(dev_type, SenseId),
+ VMSTATE_UINT8(dev_model, SenseId),
+ VMSTATE_UINT8(unused, SenseId),
+ VMSTATE_STRUCT_ARRAY(ciw, SenseId, MAX_CIWS, 0, vmstate_ciw, CIW),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static int subch_dev_post_load(void *opaque, int version_id);
+static void subch_dev_pre_save(void *opaque);
+
+const VMStateDescription vmstate_subch_dev = {
+ .name = "s390_subch_dev",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .post_load = subch_dev_post_load,
+ .pre_save = subch_dev_pre_save,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT8_EQUAL(cssid, SubchDev),
+ VMSTATE_UINT8_EQUAL(ssid, SubchDev),
+ VMSTATE_UINT16(migrated_schid, SubchDev),
+ VMSTATE_UINT16_EQUAL(devno, SubchDev),
+ VMSTATE_BOOL(thinint_active, SubchDev),
+ VMSTATE_STRUCT(curr_status, SubchDev, 0, vmstate_schib, SCHIB),
+ VMSTATE_UINT8_ARRAY(sense_data, SubchDev, 32),
+ VMSTATE_UINT64(channel_prog, SubchDev),
+ VMSTATE_STRUCT(last_cmd, SubchDev, 0, vmstate_ccw1, CCW1),
+ VMSTATE_BOOL(last_cmd_valid, SubchDev),
+ VMSTATE_STRUCT(id, SubchDev, 0, vmstate_sense_id, SenseId),
+ VMSTATE_BOOL(ccw_fmt_1, SubchDev),
+ VMSTATE_UINT8(ccw_no_data_cnt, SubchDev),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static int css_get_ind_addr(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
+{
+ int32_t len;
+ IndAddr **ind_addr = pv;
+
+ len = qemu_get_be32(f);
+ if (len != 0) {
+ *ind_addr = get_indicator(qemu_get_be64(f), len);
+ } else {
+ qemu_get_be64(f);
+ *ind_addr = NULL;
+ }
+ return 0;
+}
+
+static int css_put_ind_addr(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field, QJSON *vmdesc)
+{
+ IndAddr *ind_addr = *(IndAddr **) pv;
+
+ if (ind_addr != NULL) {
+ qemu_put_be32(f, ind_addr->len);
+ qemu_put_be64(f, ind_addr->addr);
+ } else {
+ qemu_put_be32(f, 0);
+ qemu_put_be64(f, 0UL);
+ }
+ return 0;
+}
+
+const VMStateInfo vmstate_info_ind_addr = {
+ .name = "s390_ind_addr",
+ .get = css_get_ind_addr,
+ .put = css_put_ind_addr
+};
+
typedef struct CssImage {
SubchSet *sch_set[MAX_SSID + 1];
ChpInfo chpids[MAX_CHPID + 1];
} CssImage;
+static const VMStateDescription vmstate_css_img = {
+ .name = "s390_css_img",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ /* Subchannel sets have no relevant state. */
+ VMSTATE_STRUCT_ARRAY(chpids, CssImage, MAX_CHPID + 1, 0,
+ vmstate_chp_info, ChpInfo),
+ VMSTATE_END_OF_LIST()
+ }
+
+};
+
typedef struct IoAdapter {
uint32_t id;
uint8_t type;
@@ -60,10 +262,34 @@ typedef struct ChannelSubSys {
uint64_t chnmon_area;
CssImage *css[MAX_CSSID + 1];
uint8_t default_cssid;
+ /* don't migrate */
IoAdapter *io_adapters[CSS_IO_ADAPTER_TYPE_NUMS][MAX_ISC + 1];
+ /* don't migrate */
QTAILQ_HEAD(, IndAddr) indicator_addresses;
} ChannelSubSys;
+static const VMStateDescription vmstate_css = {
+ .name = "s390_css",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_QTAILQ_V(pending_crws, ChannelSubSys, 1, vmstate_crw_container,
+ CrwContainer, sibling),
+ VMSTATE_BOOL(sei_pending, ChannelSubSys),
+ VMSTATE_BOOL(do_crw_mchk, ChannelSubSys),
+ VMSTATE_BOOL(crws_lost, ChannelSubSys),
+ /* These were kind of migrated by virtio */
+ VMSTATE_UINT8(max_cssid, ChannelSubSys),
+ VMSTATE_UINT8(max_ssid, ChannelSubSys),
+ VMSTATE_BOOL(chnmon_active, ChannelSubSys),
+ VMSTATE_UINT64(chnmon_area, ChannelSubSys),
+ VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(css, ChannelSubSys, MAX_CSSID + 1,
+ 0, vmstate_css_img, CssImage),
+ VMSTATE_UINT8(default_cssid, ChannelSubSys),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static ChannelSubSys channel_subsys = {
.pending_crws = QTAILQ_HEAD_INITIALIZER(channel_subsys.pending_crws),
.do_crw_mchk = true,
@@ -75,6 +301,56 @@ static ChannelSubSys channel_subsys = {
QTAILQ_HEAD_INITIALIZER(channel_subsys.indicator_addresses),
};
+static void subch_dev_pre_save(void *opaque)
+{
+ SubchDev *s = opaque;
+
+ /* Prepare remote_schid for save */
+ s->migrated_schid = s->schid;
+}
+
+static int subch_dev_post_load(void *opaque, int version_id)
+{
+
+ SubchDev *s = opaque;
+
+ /* Re-assign the subchannel to remote_schid if necessary */
+ if (s->migrated_schid != s->schid) {
+ if (css_find_subch(true, s->cssid, s->ssid, s->schid) == s) {
+ /*
+ * Cleanup the slot before moving to s->migrated_schid provided
+ * it still belongs to us, i.e. it was not changed by previous
+ * invocation of this function.
+ */
+ css_subch_assign(s->cssid, s->ssid, s->schid, s->devno, NULL);
+ }
+ /* It's OK to re-assign without a prior de-assign. */
+ s->schid = s->migrated_schid;
+ css_subch_assign(s->cssid, s->ssid, s->schid, s->devno, s);
+ }
+
+ if (css_migration_enabled()) {
+ /* No compat voodoo to do ;) */
+ return 0;
+ }
+ /*
+ * Hack alert. If we don't migrate the channel subsystem status
+ * we still need to find out if the guest enabled mss/mcss-e.
+ * If the subchannel is enabled, it certainly was able to access it,
+ * so adjust the max_ssid/max_cssid values for relevant ssid/cssid
+ * values. This is not watertight, but better than nothing.
+ */
+ if (s->curr_status.pmcw.flags & PMCW_FLAGS_MASK_ENA) {
+ if (s->ssid) {
+ channel_subsys.max_ssid = MAX_SSID;
+ }
+ if (s->cssid != channel_subsys.default_cssid) {
+ channel_subsys.max_cssid = MAX_CSSID;
+ }
+ }
+ return 0;
+}
+
IndAddr *get_indicator(hwaddr ind_addr, int len)
{
IndAddr *indicator;
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index f1f0d7f..6a451b2 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -87,6 +87,7 @@ struct SubchDev {
bool ccw_fmt_1;
bool thinint_active;
uint8_t ccw_no_data_cnt;
+ uint16_t migrated_schid; /* used for missmatch detection */
/* transport-provided data: */
int (*ccw_cb) (SubchDev *, CCW1);
void (*disable_cb)(SubchDev *);
@@ -94,14 +95,21 @@ struct SubchDev {
void *driver_data;
};
+extern const VMStateDescription vmstate_subch_dev;
+
typedef struct IndAddr {
hwaddr addr;
uint64_t map;
unsigned long refcnt;
- int len;
+ int32_t len;
QTAILQ_ENTRY(IndAddr) sibling;
} IndAddr;
+extern const VMStateInfo vmstate_info_ind_addr;
+
+#define VMSTATE_PTR_TO_IND_ADDR(_f, _s) \
+ VMSTATE_SINGLE(_f, _s, 1 , vmstate_info_ind_addr, IndAddr*)
+
IndAddr *get_indicator(hwaddr ind_addr, int len);
void release_indicator(AdapterInfo *adapter, IndAddr *indicator);
int map_indicator(AdapterInfo *adapter, IndAddr *indicator);
--
2.10.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [Qemu-devel] [PATCH 04/10] s390x/css: add vmstate macro for CcwDevice
2017-05-05 17:34 [Qemu-devel] [PATCH 00/10] migration: s390x css migration Halil Pasic
` (2 preceding siblings ...)
2017-05-05 17:35 ` [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css Halil Pasic
@ 2017-05-05 17:35 ` Halil Pasic
2017-05-05 17:35 ` [Qemu-devel] [PATCH 05/10] virtio-ccw: add vmstate entities for VirtioCcwDevice Halil Pasic
` (5 subsequent siblings)
9 siblings, 0 replies; 42+ messages in thread
From: Halil Pasic @ 2017-05-05 17:35 UTC (permalink / raw)
To: Cornelia Huck
Cc: Michael S. Tsirkin, Dr. David Alan Gilbert, qemu-devel, Halil Pasic
As a preparation for switching to a vmstate based migration let us
introduce a vmstate macro for CcwDevice.
No changes in behavior, we just added some dead code -- which should
rise to life soon.
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
---
hw/s390x/ccw-device.c | 10 ++++++++++
hw/s390x/ccw-device.h | 4 ++++
2 files changed, 14 insertions(+)
diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
index fb8d640..f9bfa15 100644
--- a/hw/s390x/ccw-device.c
+++ b/hw/s390x/ccw-device.c
@@ -50,6 +50,16 @@ static void ccw_device_class_init(ObjectClass *klass, void *data)
dc->props = ccw_device_properties;
}
+const VMStateDescription vmstate_ccw_dev = {
+ .name = "s390_ccw_dev",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_STRUCT_POINTER(sch, CcwDevice, vmstate_subch_dev, SubchDev),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static const TypeInfo ccw_device_info = {
.name = TYPE_CCW_DEVICE,
.parent = TYPE_DEVICE,
diff --git a/hw/s390x/ccw-device.h b/hw/s390x/ccw-device.h
index 89c8e5d..4e6af28 100644
--- a/hw/s390x/ccw-device.h
+++ b/hw/s390x/ccw-device.h
@@ -27,6 +27,10 @@ typedef struct CcwDevice {
CssDevId subch_id;
} CcwDevice;
+extern const VMStateDescription vmstate_ccw_dev;
+#define VMSTATE_CCW_DEVICE(_field, _state) \
+ VMSTATE_STRUCT(_field, _state, 1, vmstate_ccw_dev, CcwDevice)
+
typedef struct CCWDeviceClass {
DeviceClass parent_class;
void (*unplug)(HotplugHandler *, DeviceState *, Error **);
--
2.10.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [Qemu-devel] [PATCH 05/10] virtio-ccw: add vmstate entities for VirtioCcwDevice
2017-05-05 17:34 [Qemu-devel] [PATCH 00/10] migration: s390x css migration Halil Pasic
` (3 preceding siblings ...)
2017-05-05 17:35 ` [Qemu-devel] [PATCH 04/10] s390x/css: add vmstate macro for CcwDevice Halil Pasic
@ 2017-05-05 17:35 ` Halil Pasic
2017-05-05 17:35 ` [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration Halil Pasic
` (4 subsequent siblings)
9 siblings, 0 replies; 42+ messages in thread
From: Halil Pasic @ 2017-05-05 17:35 UTC (permalink / raw)
To: Cornelia Huck
Cc: Michael S. Tsirkin, Dr. David Alan Gilbert, qemu-devel, Halil Pasic
As a preparation for a transition to a vmstate based migration let us
introduce vmstate entities for VirtioCcwDevice and its members. This
patch does not take care of config_vector which is common VirtIO state
but not migrated by common VirtiIO code.
No changes in behavior, we just added some dead code -- which should
rise to life soon.
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
---
Note: In this patch series the config_vector issue is handled by the
patch "virtio-ccw: use vmstate way for config migration". Further
details there.
---
hw/intc/s390_flic.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
hw/s390x/virtio-ccw.c | 38 +++++++++++++++++++++++++++++++++++
include/hw/s390x/s390_flic.h | 5 +++++
3 files changed, 91 insertions(+)
diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
index 711c114..b889e74 100644
--- a/hw/intc/s390_flic.c
+++ b/hw/intc/s390_flic.c
@@ -18,6 +18,7 @@
#include "trace.h"
#include "hw/qdev.h"
#include "qapi/error.h"
+#include "hw/s390x/s390-virtio-ccw.h"
S390FLICState *s390_get_flic(void)
{
@@ -137,3 +138,50 @@ static void qemu_s390_flic_register_types(void)
}
type_init(qemu_s390_flic_register_types)
+
+static bool adapter_info_so_needed(void *opaque)
+{
+ return css_migration_enabled();
+}
+
+const VMStateDescription vmstate_adapter_info_so = {
+ .name = "s390_adapter_info/summary_offset",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = adapter_info_so_needed,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(summary_offset, AdapterInfo),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+const VMStateDescription vmstate_adapter_info = {
+ .name = "s390_adapter_info",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT64(ind_offset, AdapterInfo),
+ /*
+ * We do not have to migrate neither the id nor the addresses.
+ * The id is set by css_register_io_adapter and the addresses
+ * are set based on the IndAddr objects after those get mapped.
+ */
+ VMSTATE_END_OF_LIST()
+ },
+ .subsections = (const VMStateDescription * []) {
+ &vmstate_adapter_info_so,
+ NULL
+ }
+};
+
+const VMStateDescription vmstate_adapter_routes = {
+
+ .name = "s390_adapter_routes",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_STRUCT(adapter, AdapterRoutes, 1, vmstate_adapter_info, \
+ AdapterInfo),
+ VMSTATE_END_OF_LIST()
+ }
+};
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index e7167e3..c2badfe 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -34,9 +34,47 @@
#include "virtio-ccw.h"
#include "trace.h"
#include "hw/s390x/css-bridge.h"
+#include "hw/s390x/s390-virtio-ccw.h"
#define NR_CLASSIC_INDICATOR_BITS 64
+static int virtio_ccw_dev_post_load(void *opaque, int version_id)
+{
+ VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(opaque);
+ CcwDevice *ccw_dev = CCW_DEVICE(dev);
+ CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev);
+
+ ccw_dev->sch->driver_data = dev;
+ if (CCW_DEVICE(dev)->sch->thinint_active) {
+ dev->routes.adapter.adapter_id = css_get_adapter_id(
+ CSS_IO_ADAPTER_VIRTIO,
+ dev->thinint_isc);
+ }
+ /* Re-fill subch_id after loading the subchannel states.*/
+ if (ck->refill_ids) {
+ ck->refill_ids(ccw_dev);
+ }
+ return 0;
+}
+
+const VMStateDescription vmstate_virtio_ccw_dev = {
+ .name = "s390_virtio_ccw_dev",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .post_load = virtio_ccw_dev_post_load,
+ .fields = (VMStateField[]) {
+ VMSTATE_CCW_DEVICE(parent_obj, VirtioCcwDevice),
+ VMSTATE_PTR_TO_IND_ADDR(indicators, VirtioCcwDevice),
+ VMSTATE_PTR_TO_IND_ADDR(indicators2, VirtioCcwDevice),
+ VMSTATE_PTR_TO_IND_ADDR(summary_indicator, VirtioCcwDevice),
+ VMSTATE_STRUCT(routes, VirtioCcwDevice, 1, vmstate_adapter_routes, \
+ AdapterRoutes),
+ VMSTATE_UINT8(thinint_isc, VirtioCcwDevice),
+ VMSTATE_INT32(revision, VirtioCcwDevice),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static void virtio_ccw_bus_new(VirtioBusState *bus, size_t bus_size,
VirtioCcwDevice *dev);
diff --git a/include/hw/s390x/s390_flic.h b/include/hw/s390x/s390_flic.h
index f9e6890..caa6fc6 100644
--- a/include/hw/s390x/s390_flic.h
+++ b/include/hw/s390x/s390_flic.h
@@ -31,6 +31,11 @@ typedef struct AdapterRoutes {
int gsi[ADAPTER_ROUTES_MAX_GSI];
} AdapterRoutes;
+extern const VMStateDescription vmstate_adapter_routes;
+
+#define VMSTATE_ADAPTER_ROUTES(_f, _s) \
+ VMSTATE_STRUCT(_f, _s, 1, vmstate_adapter_routes, AdapterRoutes)
+
#define TYPE_S390_FLIC_COMMON "s390-flic"
#define S390_FLIC_COMMON(obj) \
OBJECT_CHECK(S390FLICState, (obj), TYPE_S390_FLIC_COMMON)
--
2.10.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration
2017-05-05 17:34 [Qemu-devel] [PATCH 00/10] migration: s390x css migration Halil Pasic
` (4 preceding siblings ...)
2017-05-05 17:35 ` [Qemu-devel] [PATCH 05/10] virtio-ccw: add vmstate entities for VirtioCcwDevice Halil Pasic
@ 2017-05-05 17:35 ` Halil Pasic
2017-05-08 16:55 ` Dr. David Alan Gilbert
2017-05-08 17:36 ` Dr. David Alan Gilbert
2017-05-05 17:35 ` [Qemu-devel] [PATCH 07/10] s390x/css: remove unused subch_dev_(load|save) Halil Pasic
` (3 subsequent siblings)
9 siblings, 2 replies; 42+ messages in thread
From: Halil Pasic @ 2017-05-05 17:35 UTC (permalink / raw)
To: Cornelia Huck
Cc: Michael S. Tsirkin, Dr. David Alan Gilbert, qemu-devel, Halil Pasic
Let us use the freshly introduced vmstate migration helpers instead of
saving/loading the config manually.
To achieve this we need to hack the config_vector which is a common
VirtIO state in the middle of the VirtioCcwDevice state representation.
This somewhat ugly but we have no choice because the stream format needs
to be preserved.
Still no changes in behavior, but the dead code we added previously is
finally awakening to life.
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
---
hw/s390x/virtio-ccw.c | 116 +++++++++++++++++++-------------------------------
1 file changed, 44 insertions(+), 72 deletions(-)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index c2badfe..8ab655c 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -57,6 +57,32 @@ static int virtio_ccw_dev_post_load(void *opaque, int version_id)
return 0;
}
+static int get_config_vector(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
+{
+ VirtioCcwDevice *dev = pv;
+ VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
+
+ qemu_get_be16s(f, &vdev->config_vector);
+ return 0;
+}
+
+static int put_config_vector(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field, QJSON *vmdesc)
+{
+ VirtioCcwDevice *dev = pv;
+ VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
+
+ qemu_put_be16(f, vdev->config_vector);
+ return 0;
+}
+
+const VMStateInfo vmstate_info_config_vector = {
+ .name = "config_vector",
+ .get = get_config_vector,
+ .put = put_config_vector,
+};
+
const VMStateDescription vmstate_virtio_ccw_dev = {
.name = "s390_virtio_ccw_dev",
.version_id = 1,
@@ -67,6 +93,14 @@ const VMStateDescription vmstate_virtio_ccw_dev = {
VMSTATE_PTR_TO_IND_ADDR(indicators, VirtioCcwDevice),
VMSTATE_PTR_TO_IND_ADDR(indicators2, VirtioCcwDevice),
VMSTATE_PTR_TO_IND_ADDR(summary_indicator, VirtioCcwDevice),
+ {
+ /*
+ * Ugly hack because VirtIODevice does not migrate itself.
+ * This also makes legacy via vmstate_save_state possible.
+ */
+ .name = "virtio/config_vector",
+ .info = &vmstate_info_config_vector,
+ },
VMSTATE_STRUCT(routes, VirtioCcwDevice, 1, vmstate_adapter_routes, \
AdapterRoutes),
VMSTATE_UINT8(thinint_isc, VirtioCcwDevice),
@@ -1272,85 +1306,23 @@ static int virtio_ccw_load_queue(DeviceState *d, int n, QEMUFile *f)
static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f)
{
VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
- CcwDevice *ccw_dev = CCW_DEVICE(d);
- SubchDev *s = ccw_dev->sch;
- VirtIODevice *vdev = virtio_ccw_get_vdev(s);
- subch_device_save(s, f);
- if (dev->indicators != NULL) {
- qemu_put_be32(f, dev->indicators->len);
- qemu_put_be64(f, dev->indicators->addr);
- } else {
- qemu_put_be32(f, 0);
- qemu_put_be64(f, 0UL);
- }
- if (dev->indicators2 != NULL) {
- qemu_put_be32(f, dev->indicators2->len);
- qemu_put_be64(f, dev->indicators2->addr);
- } else {
- qemu_put_be32(f, 0);
- qemu_put_be64(f, 0UL);
- }
- if (dev->summary_indicator != NULL) {
- qemu_put_be32(f, dev->summary_indicator->len);
- qemu_put_be64(f, dev->summary_indicator->addr);
- } else {
- qemu_put_be32(f, 0);
- qemu_put_be64(f, 0UL);
- }
- qemu_put_be16(f, vdev->config_vector);
- qemu_put_be64(f, dev->routes.adapter.ind_offset);
- qemu_put_byte(f, dev->thinint_isc);
- qemu_put_be32(f, dev->revision);
+ /*
+ * We save in legacy mode. The components take care of their own
+ * compat. representation (based on css_migration_enabled).
+ */
+ vmstate_save_state(f, &vmstate_virtio_ccw_dev, dev, NULL);
}
static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
{
VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
- CcwDevice *ccw_dev = CCW_DEVICE(d);
- CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev);
- SubchDev *s = ccw_dev->sch;
- VirtIODevice *vdev = virtio_ccw_get_vdev(s);
- int len;
-
- s->driver_data = dev;
- subch_device_load(s, f);
- /* Re-fill subch_id after loading the subchannel states.*/
- if (ck->refill_ids) {
- ck->refill_ids(ccw_dev);
- }
- len = qemu_get_be32(f);
- if (len != 0) {
- dev->indicators = get_indicator(qemu_get_be64(f), len);
- } else {
- qemu_get_be64(f);
- dev->indicators = NULL;
- }
- len = qemu_get_be32(f);
- if (len != 0) {
- dev->indicators2 = get_indicator(qemu_get_be64(f), len);
- } else {
- qemu_get_be64(f);
- dev->indicators2 = NULL;
- }
- len = qemu_get_be32(f);
- if (len != 0) {
- dev->summary_indicator = get_indicator(qemu_get_be64(f), len);
- } else {
- qemu_get_be64(f);
- dev->summary_indicator = NULL;
- }
- qemu_get_be16s(f, &vdev->config_vector);
- dev->routes.adapter.ind_offset = qemu_get_be64(f);
- dev->thinint_isc = qemu_get_byte(f);
- dev->revision = qemu_get_be32(f);
- if (s->thinint_active) {
- dev->routes.adapter.adapter_id = css_get_adapter_id(
- CSS_IO_ADAPTER_VIRTIO,
- dev->thinint_isc);
- }
- return 0;
+ /*
+ * We load in legacy mode. The components take take care to read
+ * only stuff which is actually there (based on css_migration_enabled).
+ */
+ return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1);
}
static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
--
2.10.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [Qemu-devel] [PATCH 07/10] s390x/css: remove unused subch_dev_(load|save)
2017-05-05 17:34 [Qemu-devel] [PATCH 00/10] migration: s390x css migration Halil Pasic
` (5 preceding siblings ...)
2017-05-05 17:35 ` [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration Halil Pasic
@ 2017-05-05 17:35 ` Halil Pasic
2017-05-05 17:35 ` [Qemu-devel] [PATCH 08/10] s390x/css: add ORB to SubchDev Halil Pasic
` (2 subsequent siblings)
9 siblings, 0 replies; 42+ messages in thread
From: Halil Pasic @ 2017-05-05 17:35 UTC (permalink / raw)
To: Cornelia Huck
Cc: Michael S. Tsirkin, Dr. David Alan Gilbert, qemu-devel, Halil Pasic
Since the functions subch_dev_load and subch_dev save are not used
any more let's remove them.
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
---
hw/s390x/css.c | 143 -------------------------------------------------
include/hw/s390x/css.h | 2 -
2 files changed, 145 deletions(-)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 2bda7d0..94338b2 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1914,149 +1914,6 @@ int css_enable_mss(void)
return 0;
}
-void subch_device_save(SubchDev *s, QEMUFile *f)
-{
- int i;
-
- qemu_put_byte(f, s->cssid);
- qemu_put_byte(f, s->ssid);
- qemu_put_be16(f, s->schid);
- qemu_put_be16(f, s->devno);
- qemu_put_byte(f, s->thinint_active);
- /* SCHIB */
- /* PMCW */
- qemu_put_be32(f, s->curr_status.pmcw.intparm);
- qemu_put_be16(f, s->curr_status.pmcw.flags);
- qemu_put_be16(f, s->curr_status.pmcw.devno);
- qemu_put_byte(f, s->curr_status.pmcw.lpm);
- qemu_put_byte(f, s->curr_status.pmcw.pnom);
- qemu_put_byte(f, s->curr_status.pmcw.lpum);
- qemu_put_byte(f, s->curr_status.pmcw.pim);
- qemu_put_be16(f, s->curr_status.pmcw.mbi);
- qemu_put_byte(f, s->curr_status.pmcw.pom);
- qemu_put_byte(f, s->curr_status.pmcw.pam);
- qemu_put_buffer(f, s->curr_status.pmcw.chpid, 8);
- qemu_put_be32(f, s->curr_status.pmcw.chars);
- /* SCSW */
- qemu_put_be16(f, s->curr_status.scsw.flags);
- qemu_put_be16(f, s->curr_status.scsw.ctrl);
- qemu_put_be32(f, s->curr_status.scsw.cpa);
- qemu_put_byte(f, s->curr_status.scsw.dstat);
- qemu_put_byte(f, s->curr_status.scsw.cstat);
- qemu_put_be16(f, s->curr_status.scsw.count);
- qemu_put_be64(f, s->curr_status.mba);
- qemu_put_buffer(f, s->curr_status.mda, 4);
- /* end SCHIB */
- qemu_put_buffer(f, s->sense_data, 32);
- qemu_put_be64(f, s->channel_prog);
- /* last cmd */
- qemu_put_byte(f, s->last_cmd.cmd_code);
- qemu_put_byte(f, s->last_cmd.flags);
- qemu_put_be16(f, s->last_cmd.count);
- qemu_put_be32(f, s->last_cmd.cda);
- qemu_put_byte(f, s->last_cmd_valid);
- qemu_put_byte(f, s->id.reserved);
- qemu_put_be16(f, s->id.cu_type);
- qemu_put_byte(f, s->id.cu_model);
- qemu_put_be16(f, s->id.dev_type);
- qemu_put_byte(f, s->id.dev_model);
- qemu_put_byte(f, s->id.unused);
- for (i = 0; i < ARRAY_SIZE(s->id.ciw); i++) {
- qemu_put_byte(f, s->id.ciw[i].type);
- qemu_put_byte(f, s->id.ciw[i].command);
- qemu_put_be16(f, s->id.ciw[i].count);
- }
- qemu_put_byte(f, s->ccw_fmt_1);
- qemu_put_byte(f, s->ccw_no_data_cnt);
-}
-
-int subch_device_load(SubchDev *s, QEMUFile *f)
-{
- SubchDev *old_s;
- uint16_t old_schid = s->schid;
- int i;
-
- s->cssid = qemu_get_byte(f);
- s->ssid = qemu_get_byte(f);
- s->schid = qemu_get_be16(f);
- s->devno = qemu_get_be16(f);
- /* Re-assign subch. */
- if (old_schid != s->schid) {
- old_s = channel_subsys.css[s->cssid]->sch_set[s->ssid]->sch[old_schid];
- /*
- * (old_s != s) means that some other device has its correct
- * subchannel already assigned (in load).
- */
- if (old_s == s) {
- css_subch_assign(s->cssid, s->ssid, old_schid, s->devno, NULL);
- }
- /* It's OK to re-assign without a prior de-assign. */
- css_subch_assign(s->cssid, s->ssid, s->schid, s->devno, s);
- }
- s->thinint_active = qemu_get_byte(f);
- /* SCHIB */
- /* PMCW */
- s->curr_status.pmcw.intparm = qemu_get_be32(f);
- s->curr_status.pmcw.flags = qemu_get_be16(f);
- s->curr_status.pmcw.devno = qemu_get_be16(f);
- s->curr_status.pmcw.lpm = qemu_get_byte(f);
- s->curr_status.pmcw.pnom = qemu_get_byte(f);
- s->curr_status.pmcw.lpum = qemu_get_byte(f);
- s->curr_status.pmcw.pim = qemu_get_byte(f);
- s->curr_status.pmcw.mbi = qemu_get_be16(f);
- s->curr_status.pmcw.pom = qemu_get_byte(f);
- s->curr_status.pmcw.pam = qemu_get_byte(f);
- qemu_get_buffer(f, s->curr_status.pmcw.chpid, 8);
- s->curr_status.pmcw.chars = qemu_get_be32(f);
- /* SCSW */
- s->curr_status.scsw.flags = qemu_get_be16(f);
- s->curr_status.scsw.ctrl = qemu_get_be16(f);
- s->curr_status.scsw.cpa = qemu_get_be32(f);
- s->curr_status.scsw.dstat = qemu_get_byte(f);
- s->curr_status.scsw.cstat = qemu_get_byte(f);
- s->curr_status.scsw.count = qemu_get_be16(f);
- s->curr_status.mba = qemu_get_be64(f);
- qemu_get_buffer(f, s->curr_status.mda, 4);
- /* end SCHIB */
- qemu_get_buffer(f, s->sense_data, 32);
- s->channel_prog = qemu_get_be64(f);
- /* last cmd */
- s->last_cmd.cmd_code = qemu_get_byte(f);
- s->last_cmd.flags = qemu_get_byte(f);
- s->last_cmd.count = qemu_get_be16(f);
- s->last_cmd.cda = qemu_get_be32(f);
- s->last_cmd_valid = qemu_get_byte(f);
- s->id.reserved = qemu_get_byte(f);
- s->id.cu_type = qemu_get_be16(f);
- s->id.cu_model = qemu_get_byte(f);
- s->id.dev_type = qemu_get_be16(f);
- s->id.dev_model = qemu_get_byte(f);
- s->id.unused = qemu_get_byte(f);
- for (i = 0; i < ARRAY_SIZE(s->id.ciw); i++) {
- s->id.ciw[i].type = qemu_get_byte(f);
- s->id.ciw[i].command = qemu_get_byte(f);
- s->id.ciw[i].count = qemu_get_be16(f);
- }
- s->ccw_fmt_1 = qemu_get_byte(f);
- s->ccw_no_data_cnt = qemu_get_byte(f);
- /*
- * Hack alert. We don't migrate the channel subsystem status (no
- * device!), but we need to find out if the guest enabled mss/mcss-e.
- * If the subchannel is enabled, it certainly was able to access it,
- * so adjust the max_ssid/max_cssid values for relevant ssid/cssid
- * values. This is not watertight, but better than nothing.
- */
- if (s->curr_status.pmcw.flags & PMCW_FLAGS_MASK_ENA) {
- if (s->ssid) {
- channel_subsys.max_ssid = MAX_SSID;
- }
- if (s->cssid != channel_subsys.default_cssid) {
- channel_subsys.max_cssid = MAX_CSSID;
- }
- }
- return 0;
-}
-
void css_reset_sch(SubchDev *sch)
{
PMCW *p = &sch->curr_status.pmcw;
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 6a451b2..e67ac5c 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -116,8 +116,6 @@ int map_indicator(AdapterInfo *adapter, IndAddr *indicator);
typedef SubchDev *(*css_subch_cb_func)(uint8_t m, uint8_t cssid, uint8_t ssid,
uint16_t schid);
-void subch_device_save(SubchDev *s, QEMUFile *f);
-int subch_device_load(SubchDev *s, QEMUFile *f);
int css_create_css_image(uint8_t cssid, bool default_image);
bool css_devno_used(uint8_t cssid, uint8_t ssid, uint16_t devno);
void css_subch_assign(uint8_t cssid, uint8_t ssid, uint16_t schid,
--
2.10.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [Qemu-devel] [PATCH 08/10] s390x/css: add ORB to SubchDev
2017-05-05 17:34 [Qemu-devel] [PATCH 00/10] migration: s390x css migration Halil Pasic
` (6 preceding siblings ...)
2017-05-05 17:35 ` [Qemu-devel] [PATCH 07/10] s390x/css: remove unused subch_dev_(load|save) Halil Pasic
@ 2017-05-05 17:35 ` Halil Pasic
2017-05-05 17:35 ` [Qemu-devel] [PATCH 09/10] s390x/css: turn on channel subsystem migration Halil Pasic
2017-05-05 17:35 ` [Qemu-devel] [PATCH 10/10] s390x/css: use SubchDev.orb Halil Pasic
9 siblings, 0 replies; 42+ messages in thread
From: Halil Pasic @ 2017-05-05 17:35 UTC (permalink / raw)
To: Cornelia Huck
Cc: Michael S. Tsirkin, Dr. David Alan Gilbert, qemu-devel, Halil Pasic
Since we are going to need a migration compatibility breaking change to
activate ChannelSubSys migration let us use the opportunity to introduce
ORB to the SubchDev before that (otherwise we would need separate
handling e.g. a compat property).
The ORB will be useful for implementing IDA, or async handling of
subchannel work.
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
---
hw/s390x/css.c | 35 +++++++++++++++++++++++++++++++++++
include/hw/s390x/css.h | 1 +
2 files changed, 36 insertions(+)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 94338b2..d9a0fb9 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -163,6 +163,36 @@ static const VMStateDescription vmstate_sense_id = {
}
};
+static const VMStateDescription vmstate_orb = {
+ .name = "s390_orb",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(intparm, ORB),
+ VMSTATE_UINT16(ctrl0, ORB),
+ VMSTATE_UINT8(lpm, ORB),
+ VMSTATE_UINT8(ctrl1, ORB),
+ VMSTATE_UINT32(cpa, ORB),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static bool vmstate_schdev_orb_needed(void *opaque)
+{
+ return css_migration_enabled();
+}
+
+static const VMStateDescription vmstate_schdev_orb = {
+ .name = "s390_subch_dev/orb",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = vmstate_schdev_orb_needed,
+ .fields = (VMStateField[]) {
+ VMSTATE_STRUCT(orb, SubchDev, 1, vmstate_orb, ORB),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static int subch_dev_post_load(void *opaque, int version_id);
static void subch_dev_pre_save(void *opaque);
@@ -187,6 +217,10 @@ const VMStateDescription vmstate_subch_dev = {
VMSTATE_BOOL(ccw_fmt_1, SubchDev),
VMSTATE_UINT8(ccw_no_data_cnt, SubchDev),
VMSTATE_END_OF_LIST()
+ },
+ .subsections = (const VMStateDescription * []) {
+ &vmstate_schdev_orb,
+ NULL
}
};
@@ -1234,6 +1268,7 @@ int css_do_ssch(SubchDev *sch, ORB *orb)
if (channel_subsys.chnmon_active) {
css_update_chnmon(sch);
}
+ sch->orb = *orb;
sch->channel_prog = orb->cpa;
/* Trigger the start function. */
s->ctrl |= (SCSW_FCTL_START_FUNC | SCSW_ACTL_START_PEND);
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index e67ac5c..dbe093e 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -88,6 +88,7 @@ struct SubchDev {
bool thinint_active;
uint8_t ccw_no_data_cnt;
uint16_t migrated_schid; /* used for missmatch detection */
+ ORB orb;
/* transport-provided data: */
int (*ccw_cb) (SubchDev *, CCW1);
void (*disable_cb)(SubchDev *);
--
2.10.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [Qemu-devel] [PATCH 09/10] s390x/css: turn on channel subsystem migration
2017-05-05 17:34 [Qemu-devel] [PATCH 00/10] migration: s390x css migration Halil Pasic
` (7 preceding siblings ...)
2017-05-05 17:35 ` [Qemu-devel] [PATCH 08/10] s390x/css: add ORB to SubchDev Halil Pasic
@ 2017-05-05 17:35 ` Halil Pasic
2017-05-08 17:27 ` Dr. David Alan Gilbert
2017-05-05 17:35 ` [Qemu-devel] [PATCH 10/10] s390x/css: use SubchDev.orb Halil Pasic
9 siblings, 1 reply; 42+ messages in thread
From: Halil Pasic @ 2017-05-05 17:35 UTC (permalink / raw)
To: Cornelia Huck
Cc: Michael S. Tsirkin, Dr. David Alan Gilbert, qemu-devel, Halil Pasic
Turn on migration for the channel subsystem and the new scheme for
migrating virtio-ccw proxy devices (instead of letting the transport
independent child device migrate it's proxy, use the usual
DeviceClass.vmsd mechanism) for future machine versions.
The vmstate based migration of the channel subsystem is not migration
stream compatible with the method for handling migration of legacy
machines.
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
---
hw/s390x/ccw-device.c | 1 +
hw/s390x/css.c | 5 +++++
hw/s390x/s390-virtio-ccw.c | 9 ++++-----
hw/s390x/virtio-ccw.c | 14 ++++++++++++++
include/hw/s390x/css.h | 4 ++++
5 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
index f9bfa15..3b5df03 100644
--- a/hw/s390x/ccw-device.c
+++ b/hw/s390x/ccw-device.c
@@ -48,6 +48,7 @@ static void ccw_device_class_init(ObjectClass *klass, void *data)
k->realize = ccw_device_realize;
k->refill_ids = ccw_device_refill_ids;
dc->props = ccw_device_properties;
+ dc->vmsd = &vmstate_ccw_dev;
}
const VMStateDescription vmstate_ccw_dev = {
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index d9a0fb9..b58832a 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -385,6 +385,11 @@ static int subch_dev_post_load(void *opaque, int version_id)
return 0;
}
+void css_register_vmstate(void)
+{
+ vmstate_register(NULL, 0, &vmstate_css, &channel_subsys);
+}
+
IndAddr *get_indicator(hwaddr ind_addr, int len)
{
IndAddr *indicator;
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 698e8fc..5307f59 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -196,7 +196,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
s390mc->ri_allowed = true;
s390mc->cpu_model_allowed = true;
- s390mc->css_migration_enabled = false; /* TODO: set to true */
+ s390mc->css_migration_enabled = true;
mc->init = ccw_init;
mc->reset = s390_machine_reset;
mc->hot_add_cpu = s390_hot_add_cpu;
@@ -414,10 +414,9 @@ bool css_migration_enabled(void)
static void ccw_machine_2_10_instance_options(MachineState *machine)
{
- /*
- * TODO Once preparations are done register vmstate for the css if
- * css_migration_enabled().
- */
+ if (css_migration_enabled()) {
+ css_register_vmstate();
+ }
}
static void ccw_machine_2_10_class_options(MachineClass *mc)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 8ab655c..c611b6f 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1307,6 +1307,10 @@ static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f)
{
VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
+ if (css_migration_enabled()) {
+ /* we migrate via DeviceClass.vmsd */
+ return;
+ }
/*
* We save in legacy mode. The components take care of their own
* compat. representation (based on css_migration_enabled).
@@ -1318,6 +1322,10 @@ static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
{
VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
+ if (css_migration_enabled()) {
+ /* we migrate via DeviceClass.vmsd */
+ return 0;
+ }
/*
* We load in legacy mode. The components take take care to read
* only stuff which is actually there (based on css_migration_enabled).
@@ -1365,6 +1373,11 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
sch->id.cu_model = virtio_bus_get_vdev_id(&dev->bus);
+ /* Avoid generating unknown section for legacy migration target. */
+ if (!css_migration_enabled()) {
+ DEVICE_GET_CLASS(ccw_dev)->vmsd = NULL;
+ }
+
css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
d->hotplugged, 1);
}
@@ -1657,6 +1670,7 @@ static void virtio_ccw_device_class_init(ObjectClass *klass, void *data)
dc->realize = virtio_ccw_busdev_realize;
dc->exit = virtio_ccw_busdev_exit;
dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
+ dc->vmsd = &vmstate_virtio_ccw_dev;
}
static const TypeInfo virtio_ccw_device_info = {
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index dbe093e..be5eb81 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -207,4 +207,8 @@ extern PropertyInfo css_devid_ro_propinfo;
* is responsible for unregistering and freeing it.
*/
SubchDev *css_create_virtual_sch(CssDevId bus_id, Error **errp);
+
+/** Turn on css migration */
+void css_register_vmstate(void);
+
#endif
--
2.10.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [Qemu-devel] [PATCH 10/10] s390x/css: use SubchDev.orb
2017-05-05 17:34 [Qemu-devel] [PATCH 00/10] migration: s390x css migration Halil Pasic
` (8 preceding siblings ...)
2017-05-05 17:35 ` [Qemu-devel] [PATCH 09/10] s390x/css: turn on channel subsystem migration Halil Pasic
@ 2017-05-05 17:35 ` Halil Pasic
9 siblings, 0 replies; 42+ messages in thread
From: Halil Pasic @ 2017-05-05 17:35 UTC (permalink / raw)
To: Cornelia Huck
Cc: Michael S. Tsirkin, Dr. David Alan Gilbert, qemu-devel, Halil Pasic
Instead of passing around a pointer to ORB let us simplify some
functions signatures by using the previously introduced ORB saved at the
subchannel (SubchDev).
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
---
hw/s390x/css.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index b58832a..694365b 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -838,7 +838,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
return ret;
}
-static void sch_handle_start_func(SubchDev *sch, ORB *orb)
+static void sch_handle_start_func(SubchDev *sch)
{
PMCW *p = &sch->curr_status.pmcw;
@@ -852,10 +852,10 @@ static void sch_handle_start_func(SubchDev *sch, ORB *orb)
if (!(s->ctrl & SCSW_ACTL_SUSP)) {
/* Start Function triggered via ssch, i.e. we have an ORB */
+ ORB *orb = &sch->orb;
s->cstat = 0;
s->dstat = 0;
/* Look at the orb and try to execute the channel program. */
- assert(orb != NULL); /* resume does not pass an orb */
p->intparm = orb->intparm;
if (!(orb->lpm & path)) {
/* Generate a deferred cc 3 condition. */
@@ -869,8 +869,7 @@ static void sch_handle_start_func(SubchDev *sch, ORB *orb)
sch->ccw_no_data_cnt = 0;
suspend_allowed = !!(orb->ctrl0 & ORB_CTRL0_MASK_SPND);
} else {
- /* Start Function resumed via rsch, i.e. we don't have an
- * ORB */
+ /* Start Function resumed via rsch */
s->ctrl &= ~(SCSW_ACTL_SUSP | SCSW_ACTL_RESUME_PEND);
/* The channel program had been suspended before. */
suspend_allowed = true;
@@ -943,7 +942,7 @@ static void sch_handle_start_func(SubchDev *sch, ORB *orb)
* read/writes) asynchronous later on if we start supporting more than
* our current very simple devices.
*/
-static void do_subchannel_work(SubchDev *sch, ORB *orb)
+static void do_subchannel_work(SubchDev *sch)
{
SCSW *s = &sch->curr_status.scsw;
@@ -954,7 +953,7 @@ static void do_subchannel_work(SubchDev *sch, ORB *orb)
sch_handle_halt_func(sch);
} else if (s->ctrl & SCSW_FCTL_START_FUNC) {
/* Triggered by both ssch and rsch. */
- sch_handle_start_func(sch, orb);
+ sch_handle_start_func(sch);
} else {
/* Cannot happen. */
return;
@@ -1163,7 +1162,7 @@ int css_do_csch(SubchDev *sch)
s->ctrl &= ~(SCSW_CTRL_MASK_FCTL | SCSW_CTRL_MASK_ACTL);
s->ctrl |= SCSW_FCTL_CLEAR_FUNC | SCSW_ACTL_CLEAR_PEND;
- do_subchannel_work(sch, NULL);
+ do_subchannel_work(sch);
ret = 0;
out:
@@ -1204,7 +1203,7 @@ int css_do_hsch(SubchDev *sch)
}
s->ctrl |= SCSW_ACTL_HALT_PEND;
- do_subchannel_work(sch, NULL);
+ do_subchannel_work(sch);
ret = 0;
out:
@@ -1279,7 +1278,7 @@ int css_do_ssch(SubchDev *sch, ORB *orb)
s->ctrl |= (SCSW_FCTL_START_FUNC | SCSW_ACTL_START_PEND);
s->flags &= ~SCSW_FLAGS_MASK_PNO;
- do_subchannel_work(sch, orb);
+ do_subchannel_work(sch);
ret = 0;
out:
@@ -1559,7 +1558,7 @@ int css_do_rsch(SubchDev *sch)
}
s->ctrl |= SCSW_ACTL_RESUME_PEND;
- do_subchannel_work(sch, NULL);
+ do_subchannel_work(sch);
ret = 0;
out:
--
2.10.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css
2017-05-05 17:35 ` [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css Halil Pasic
@ 2017-05-08 16:45 ` Dr. David Alan Gilbert
2017-05-09 12:00 ` Halil Pasic
2017-05-09 12:20 ` Halil Pasic
0 siblings, 2 replies; 42+ messages in thread
From: Dr. David Alan Gilbert @ 2017-05-08 16:45 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, Michael S. Tsirkin, qemu-devel
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> As a preparation for switching to a vmstate based migration let us
> introduce vmstate entities (e.g. VMStateDescription) for the css entities
> to be migrated. Alongside some comments explaining or indicating the not
> migration of certain members are introduced too.
>
> No changes in behavior, we just added some dead code -- which should
> rise to life soon.
>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> hw/s390x/css.c | 276 +++++++++++++++++++++++++++++++++++++++++++++++++
> include/hw/s390x/css.h | 10 +-
> 2 files changed, 285 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index c03bb20..2bda7d0 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -20,29 +20,231 @@
> #include "hw/s390x/css.h"
> #include "trace.h"
> #include "hw/s390x/s390_flic.h"
> +#include "hw/s390x/s390-virtio-ccw.h"
>
> typedef struct CrwContainer {
> CRW crw;
> QTAILQ_ENTRY(CrwContainer) sibling;
> } CrwContainer;
>
> +static const VMStateDescription vmstate_crw = {
> + .name = "s390_crw",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT16(flags, CRW),
> + VMSTATE_UINT16(rsid, CRW),
> + VMSTATE_END_OF_LIST()
> + },
> +};
> +
> +static const VMStateDescription vmstate_crw_container = {
> + .name = "s390_crw_container",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_STRUCT(crw, CrwContainer, 0, vmstate_crw, CRW),
> + VMSTATE_END_OF_LIST()
> + },
> +};
> +
> typedef struct ChpInfo {
> uint8_t in_use;
> uint8_t type;
> uint8_t is_virtual;
> } ChpInfo;
>
> +static const VMStateDescription vmstate_chp_info = {
> + .name = "s390_chp_info",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT8(in_use, ChpInfo),
> + VMSTATE_UINT8(type, ChpInfo),
> + VMSTATE_UINT8(is_virtual, ChpInfo),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> typedef struct SubchSet {
> SubchDev *sch[MAX_SCHID + 1];
> unsigned long schids_used[BITS_TO_LONGS(MAX_SCHID + 1)];
> unsigned long devnos_used[BITS_TO_LONGS(MAX_SCHID + 1)];
> } SubchSet;
>
> +static const VMStateDescription vmstate_scsw = {
> + .name = "s390_scsw",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT16(flags, SCSW),
> + VMSTATE_UINT16(ctrl, SCSW),
> + VMSTATE_UINT32(cpa, SCSW),
> + VMSTATE_UINT8(dstat, SCSW),
> + VMSTATE_UINT8(cstat, SCSW),
> + VMSTATE_UINT16(count, SCSW),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static const VMStateDescription vmstate_pmcw = {
> + .name = "s390_pmcw",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32(intparm, PMCW),
> + VMSTATE_UINT16(flags, PMCW),
> + VMSTATE_UINT16(devno, PMCW),
> + VMSTATE_UINT8(lpm, PMCW),
> + VMSTATE_UINT8(pnom, PMCW),
> + VMSTATE_UINT8(lpum, PMCW),
> + VMSTATE_UINT8(pim, PMCW),
> + VMSTATE_UINT16(mbi, PMCW),
> + VMSTATE_UINT8(pom, PMCW),
> + VMSTATE_UINT8(pam, PMCW),
> + VMSTATE_UINT8_ARRAY(chpid, PMCW, 8),
> + VMSTATE_UINT32(chars, PMCW),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static const VMStateDescription vmstate_schib = {
> + .name = "s390_schib",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_STRUCT(pmcw, SCHIB, 0, vmstate_pmcw, PMCW),
> + VMSTATE_STRUCT(scsw, SCHIB, 0, vmstate_scsw, SCSW),
> + VMSTATE_UINT64(mba, SCHIB),
> + VMSTATE_UINT8_ARRAY(mda, SCHIB, 4),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +
> +static const VMStateDescription vmstate_ccw1 = {
> + .name = "s390_ccw1",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT8(cmd_code, CCW1),
> + VMSTATE_UINT8(flags, CCW1),
> + VMSTATE_UINT16(count, CCW1),
> + VMSTATE_UINT32(cda, CCW1),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static const VMStateDescription vmstate_ciw = {
> + .name = "s390_ciw",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT8(type, CIW),
> + VMSTATE_UINT8(command, CIW),
> + VMSTATE_UINT16(count, CIW),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static const VMStateDescription vmstate_sense_id = {
> + .name = "s390_sense_id",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT8(reserved, SenseId),
> + VMSTATE_UINT16(cu_type, SenseId),
> + VMSTATE_UINT8(cu_model, SenseId),
> + VMSTATE_UINT16(dev_type, SenseId),
> + VMSTATE_UINT8(dev_model, SenseId),
> + VMSTATE_UINT8(unused, SenseId),
> + VMSTATE_STRUCT_ARRAY(ciw, SenseId, MAX_CIWS, 0, vmstate_ciw, CIW),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static int subch_dev_post_load(void *opaque, int version_id);
> +static void subch_dev_pre_save(void *opaque);
> +
> +const VMStateDescription vmstate_subch_dev = {
> + .name = "s390_subch_dev",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .post_load = subch_dev_post_load,
> + .pre_save = subch_dev_pre_save,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT8_EQUAL(cssid, SubchDev),
> + VMSTATE_UINT8_EQUAL(ssid, SubchDev),
> + VMSTATE_UINT16(migrated_schid, SubchDev),
> + VMSTATE_UINT16_EQUAL(devno, SubchDev),
> + VMSTATE_BOOL(thinint_active, SubchDev),
> + VMSTATE_STRUCT(curr_status, SubchDev, 0, vmstate_schib, SCHIB),
> + VMSTATE_UINT8_ARRAY(sense_data, SubchDev, 32),
> + VMSTATE_UINT64(channel_prog, SubchDev),
> + VMSTATE_STRUCT(last_cmd, SubchDev, 0, vmstate_ccw1, CCW1),
> + VMSTATE_BOOL(last_cmd_valid, SubchDev),
> + VMSTATE_STRUCT(id, SubchDev, 0, vmstate_sense_id, SenseId),
> + VMSTATE_BOOL(ccw_fmt_1, SubchDev),
> + VMSTATE_UINT8(ccw_no_data_cnt, SubchDev),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static int css_get_ind_addr(QEMUFile *f, void *pv, size_t size,
> + VMStateField *field)
> +{
> + int32_t len;
> + IndAddr **ind_addr = pv;
> +
> + len = qemu_get_be32(f);
> + if (len != 0) {
> + *ind_addr = get_indicator(qemu_get_be64(f), len);
> + } else {
> + qemu_get_be64(f);
> + *ind_addr = NULL;
> + }
> + return 0;
> +}
> +
> +static int css_put_ind_addr(QEMUFile *f, void *pv, size_t size,
> + VMStateField *field, QJSON *vmdesc)
> +{
> + IndAddr *ind_addr = *(IndAddr **) pv;
> +
> + if (ind_addr != NULL) {
> + qemu_put_be32(f, ind_addr->len);
> + qemu_put_be64(f, ind_addr->addr);
> + } else {
> + qemu_put_be32(f, 0);
> + qemu_put_be64(f, 0UL);
> + }
> + return 0;
> +}
> +
> +const VMStateInfo vmstate_info_ind_addr = {
> + .name = "s390_ind_addr",
> + .get = css_get_ind_addr,
> + .put = css_put_ind_addr
> +};
You should be able to avoid this .get/.put by using VMSTATE_WITH_TMP,
declare a temporary struct something like:
struct tmp_ind_addr {
IndAddr *parent;
uint32_t len;
uint64_t addr;
}
and then your .get/.put routines turn into pre_save/post_load
routines to just setup the len/addr.
> +
> typedef struct CssImage {
> SubchSet *sch_set[MAX_SSID + 1];
> ChpInfo chpids[MAX_CHPID + 1];
> } CssImage;
>
> +static const VMStateDescription vmstate_css_img = {
> + .name = "s390_css_img",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + /* Subchannel sets have no relevant state. */
> + VMSTATE_STRUCT_ARRAY(chpids, CssImage, MAX_CHPID + 1, 0,
> + vmstate_chp_info, ChpInfo),
> + VMSTATE_END_OF_LIST()
> + }
> +
> +};
> +
> typedef struct IoAdapter {
> uint32_t id;
> uint8_t type;
> @@ -60,10 +262,34 @@ typedef struct ChannelSubSys {
> uint64_t chnmon_area;
> CssImage *css[MAX_CSSID + 1];
> uint8_t default_cssid;
> + /* don't migrate */
> IoAdapter *io_adapters[CSS_IO_ADAPTER_TYPE_NUMS][MAX_ISC + 1];
> + /* don't migrate */
You don't say *why*
Dave
> QTAILQ_HEAD(, IndAddr) indicator_addresses;
> } ChannelSubSys;
>
> +static const VMStateDescription vmstate_css = {
> + .name = "s390_css",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_QTAILQ_V(pending_crws, ChannelSubSys, 1, vmstate_crw_container,
> + CrwContainer, sibling),
> + VMSTATE_BOOL(sei_pending, ChannelSubSys),
> + VMSTATE_BOOL(do_crw_mchk, ChannelSubSys),
> + VMSTATE_BOOL(crws_lost, ChannelSubSys),
> + /* These were kind of migrated by virtio */
> + VMSTATE_UINT8(max_cssid, ChannelSubSys),
> + VMSTATE_UINT8(max_ssid, ChannelSubSys),
> + VMSTATE_BOOL(chnmon_active, ChannelSubSys),
> + VMSTATE_UINT64(chnmon_area, ChannelSubSys),
> + VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(css, ChannelSubSys, MAX_CSSID + 1,
> + 0, vmstate_css_img, CssImage),
> + VMSTATE_UINT8(default_cssid, ChannelSubSys),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> static ChannelSubSys channel_subsys = {
> .pending_crws = QTAILQ_HEAD_INITIALIZER(channel_subsys.pending_crws),
> .do_crw_mchk = true,
> @@ -75,6 +301,56 @@ static ChannelSubSys channel_subsys = {
> QTAILQ_HEAD_INITIALIZER(channel_subsys.indicator_addresses),
> };
>
> +static void subch_dev_pre_save(void *opaque)
> +{
> + SubchDev *s = opaque;
> +
> + /* Prepare remote_schid for save */
> + s->migrated_schid = s->schid;
> +}
> +
> +static int subch_dev_post_load(void *opaque, int version_id)
> +{
> +
> + SubchDev *s = opaque;
> +
> + /* Re-assign the subchannel to remote_schid if necessary */
> + if (s->migrated_schid != s->schid) {
> + if (css_find_subch(true, s->cssid, s->ssid, s->schid) == s) {
> + /*
> + * Cleanup the slot before moving to s->migrated_schid provided
> + * it still belongs to us, i.e. it was not changed by previous
> + * invocation of this function.
> + */
> + css_subch_assign(s->cssid, s->ssid, s->schid, s->devno, NULL);
> + }
> + /* It's OK to re-assign without a prior de-assign. */
> + s->schid = s->migrated_schid;
> + css_subch_assign(s->cssid, s->ssid, s->schid, s->devno, s);
> + }
> +
> + if (css_migration_enabled()) {
> + /* No compat voodoo to do ;) */
> + return 0;
> + }
> + /*
> + * Hack alert. If we don't migrate the channel subsystem status
> + * we still need to find out if the guest enabled mss/mcss-e.
> + * If the subchannel is enabled, it certainly was able to access it,
> + * so adjust the max_ssid/max_cssid values for relevant ssid/cssid
> + * values. This is not watertight, but better than nothing.
> + */
> + if (s->curr_status.pmcw.flags & PMCW_FLAGS_MASK_ENA) {
> + if (s->ssid) {
> + channel_subsys.max_ssid = MAX_SSID;
> + }
> + if (s->cssid != channel_subsys.default_cssid) {
> + channel_subsys.max_cssid = MAX_CSSID;
> + }
> + }
> + return 0;
> +}
> +
> IndAddr *get_indicator(hwaddr ind_addr, int len)
> {
> IndAddr *indicator;
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index f1f0d7f..6a451b2 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -87,6 +87,7 @@ struct SubchDev {
> bool ccw_fmt_1;
> bool thinint_active;
> uint8_t ccw_no_data_cnt;
> + uint16_t migrated_schid; /* used for missmatch detection */
> /* transport-provided data: */
> int (*ccw_cb) (SubchDev *, CCW1);
> void (*disable_cb)(SubchDev *);
> @@ -94,14 +95,21 @@ struct SubchDev {
> void *driver_data;
> };
>
> +extern const VMStateDescription vmstate_subch_dev;
> +
> typedef struct IndAddr {
> hwaddr addr;
> uint64_t map;
> unsigned long refcnt;
> - int len;
> + int32_t len;
> QTAILQ_ENTRY(IndAddr) sibling;
> } IndAddr;
>
> +extern const VMStateInfo vmstate_info_ind_addr;
> +
> +#define VMSTATE_PTR_TO_IND_ADDR(_f, _s) \
> + VMSTATE_SINGLE(_f, _s, 1 , vmstate_info_ind_addr, IndAddr*)
> +
> IndAddr *get_indicator(hwaddr ind_addr, int len);
> void release_indicator(AdapterInfo *adapter, IndAddr *indicator);
> int map_indicator(AdapterInfo *adapter, IndAddr *indicator);
> --
> 2.10.2
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration
2017-05-05 17:35 ` [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration Halil Pasic
@ 2017-05-08 16:55 ` Dr. David Alan Gilbert
2017-05-09 17:05 ` Halil Pasic
2017-05-08 17:36 ` Dr. David Alan Gilbert
1 sibling, 1 reply; 42+ messages in thread
From: Dr. David Alan Gilbert @ 2017-05-08 16:55 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, Michael S. Tsirkin, qemu-devel
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> Let us use the freshly introduced vmstate migration helpers instead of
> saving/loading the config manually.
>
> To achieve this we need to hack the config_vector which is a common
> VirtIO state in the middle of the VirtioCcwDevice state representation.
> This somewhat ugly but we have no choice because the stream format needs
> to be preserved.
>
> Still no changes in behavior, but the dead code we added previously is
> finally awakening to life.
>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> ---
> hw/s390x/virtio-ccw.c | 116 +++++++++++++++++++-------------------------------
> 1 file changed, 44 insertions(+), 72 deletions(-)
>
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index c2badfe..8ab655c 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -57,6 +57,32 @@ static int virtio_ccw_dev_post_load(void *opaque, int version_id)
> return 0;
> }
>
> +static int get_config_vector(QEMUFile *f, void *pv, size_t size,
> + VMStateField *field)
> +{
> + VirtioCcwDevice *dev = pv;
> + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
> +
> + qemu_get_be16s(f, &vdev->config_vector);
> + return 0;
> +}
> +
> +static int put_config_vector(QEMUFile *f, void *pv, size_t size,
> + VMStateField *field, QJSON *vmdesc)
> +{
> + VirtioCcwDevice *dev = pv;
> + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
> +
> + qemu_put_be16(f, vdev->config_vector);
> + return 0;
> +}
Again that should be doable using WITH_TMP.
(I do wonder if we need a macro for cases where it's just a casting
operation to another type).
Dave
> +const VMStateInfo vmstate_info_config_vector = {
> + .name = "config_vector",
> + .get = get_config_vector,
> + .put = put_config_vector,
> +};
> +
> const VMStateDescription vmstate_virtio_ccw_dev = {
> .name = "s390_virtio_ccw_dev",
> .version_id = 1,
> @@ -67,6 +93,14 @@ const VMStateDescription vmstate_virtio_ccw_dev = {
> VMSTATE_PTR_TO_IND_ADDR(indicators, VirtioCcwDevice),
> VMSTATE_PTR_TO_IND_ADDR(indicators2, VirtioCcwDevice),
> VMSTATE_PTR_TO_IND_ADDR(summary_indicator, VirtioCcwDevice),
> + {
> + /*
> + * Ugly hack because VirtIODevice does not migrate itself.
> + * This also makes legacy via vmstate_save_state possible.
> + */
> + .name = "virtio/config_vector",
> + .info = &vmstate_info_config_vector,
> + },
> VMSTATE_STRUCT(routes, VirtioCcwDevice, 1, vmstate_adapter_routes, \
> AdapterRoutes),
> VMSTATE_UINT8(thinint_isc, VirtioCcwDevice),
> @@ -1272,85 +1306,23 @@ static int virtio_ccw_load_queue(DeviceState *d, int n, QEMUFile *f)
> static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f)
> {
> VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> - CcwDevice *ccw_dev = CCW_DEVICE(d);
> - SubchDev *s = ccw_dev->sch;
> - VirtIODevice *vdev = virtio_ccw_get_vdev(s);
>
> - subch_device_save(s, f);
> - if (dev->indicators != NULL) {
> - qemu_put_be32(f, dev->indicators->len);
> - qemu_put_be64(f, dev->indicators->addr);
> - } else {
> - qemu_put_be32(f, 0);
> - qemu_put_be64(f, 0UL);
> - }
> - if (dev->indicators2 != NULL) {
> - qemu_put_be32(f, dev->indicators2->len);
> - qemu_put_be64(f, dev->indicators2->addr);
> - } else {
> - qemu_put_be32(f, 0);
> - qemu_put_be64(f, 0UL);
> - }
> - if (dev->summary_indicator != NULL) {
> - qemu_put_be32(f, dev->summary_indicator->len);
> - qemu_put_be64(f, dev->summary_indicator->addr);
> - } else {
> - qemu_put_be32(f, 0);
> - qemu_put_be64(f, 0UL);
> - }
> - qemu_put_be16(f, vdev->config_vector);
> - qemu_put_be64(f, dev->routes.adapter.ind_offset);
> - qemu_put_byte(f, dev->thinint_isc);
> - qemu_put_be32(f, dev->revision);
> + /*
> + * We save in legacy mode. The components take care of their own
> + * compat. representation (based on css_migration_enabled).
> + */
> + vmstate_save_state(f, &vmstate_virtio_ccw_dev, dev, NULL);
> }
>
> static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
> {
> VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> - CcwDevice *ccw_dev = CCW_DEVICE(d);
> - CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev);
> - SubchDev *s = ccw_dev->sch;
> - VirtIODevice *vdev = virtio_ccw_get_vdev(s);
> - int len;
> -
> - s->driver_data = dev;
> - subch_device_load(s, f);
> - /* Re-fill subch_id after loading the subchannel states.*/
> - if (ck->refill_ids) {
> - ck->refill_ids(ccw_dev);
> - }
> - len = qemu_get_be32(f);
> - if (len != 0) {
> - dev->indicators = get_indicator(qemu_get_be64(f), len);
> - } else {
> - qemu_get_be64(f);
> - dev->indicators = NULL;
> - }
> - len = qemu_get_be32(f);
> - if (len != 0) {
> - dev->indicators2 = get_indicator(qemu_get_be64(f), len);
> - } else {
> - qemu_get_be64(f);
> - dev->indicators2 = NULL;
> - }
> - len = qemu_get_be32(f);
> - if (len != 0) {
> - dev->summary_indicator = get_indicator(qemu_get_be64(f), len);
> - } else {
> - qemu_get_be64(f);
> - dev->summary_indicator = NULL;
> - }
> - qemu_get_be16s(f, &vdev->config_vector);
> - dev->routes.adapter.ind_offset = qemu_get_be64(f);
> - dev->thinint_isc = qemu_get_byte(f);
> - dev->revision = qemu_get_be32(f);
> - if (s->thinint_active) {
> - dev->routes.adapter.adapter_id = css_get_adapter_id(
> - CSS_IO_ADAPTER_VIRTIO,
> - dev->thinint_isc);
> - }
>
> - return 0;
> + /*
> + * We load in legacy mode. The components take take care to read
> + * only stuff which is actually there (based on css_migration_enabled).
> + */
> + return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1);
> }
>
> static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
> --
> 2.10.2
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 09/10] s390x/css: turn on channel subsystem migration
2017-05-05 17:35 ` [Qemu-devel] [PATCH 09/10] s390x/css: turn on channel subsystem migration Halil Pasic
@ 2017-05-08 17:27 ` Dr. David Alan Gilbert
2017-05-08 18:03 ` Halil Pasic
0 siblings, 1 reply; 42+ messages in thread
From: Dr. David Alan Gilbert @ 2017-05-08 17:27 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, Michael S. Tsirkin, qemu-devel
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> Turn on migration for the channel subsystem and the new scheme for
> migrating virtio-ccw proxy devices (instead of letting the transport
> independent child device migrate it's proxy, use the usual
> DeviceClass.vmsd mechanism) for future machine versions.
>
> The vmstate based migration of the channel subsystem is not migration
> stream compatible with the method for handling migration of legacy
> machines.
>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
> ---
> hw/s390x/ccw-device.c | 1 +
> hw/s390x/css.c | 5 +++++
> hw/s390x/s390-virtio-ccw.c | 9 ++++-----
> hw/s390x/virtio-ccw.c | 14 ++++++++++++++
> include/hw/s390x/css.h | 4 ++++
> 5 files changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/hw/s390x/ccw-device.c b/hw/s390x/ccw-device.c
> index f9bfa15..3b5df03 100644
> --- a/hw/s390x/ccw-device.c
> +++ b/hw/s390x/ccw-device.c
> @@ -48,6 +48,7 @@ static void ccw_device_class_init(ObjectClass *klass, void *data)
> k->realize = ccw_device_realize;
> k->refill_ids = ccw_device_refill_ids;
> dc->props = ccw_device_properties;
> + dc->vmsd = &vmstate_ccw_dev;
> }
>
> const VMStateDescription vmstate_ccw_dev = {
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index d9a0fb9..b58832a 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -385,6 +385,11 @@ static int subch_dev_post_load(void *opaque, int version_id)
> return 0;
> }
>
> +void css_register_vmstate(void)
> +{
> + vmstate_register(NULL, 0, &vmstate_css, &channel_subsys);
> +}
> +
Why isn't that attached to a device vmsd?
> IndAddr *get_indicator(hwaddr ind_addr, int len)
> {
> IndAddr *indicator;
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 698e8fc..5307f59 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -196,7 +196,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
>
> s390mc->ri_allowed = true;
> s390mc->cpu_model_allowed = true;
> - s390mc->css_migration_enabled = false; /* TODO: set to true */
> + s390mc->css_migration_enabled = true;
> mc->init = ccw_init;
> mc->reset = s390_machine_reset;
> mc->hot_add_cpu = s390_hot_add_cpu;
> @@ -414,10 +414,9 @@ bool css_migration_enabled(void)
>
> static void ccw_machine_2_10_instance_options(MachineState *machine)
> {
> - /*
> - * TODO Once preparations are done register vmstate for the css if
> - * css_migration_enabled().
> - */
> + if (css_migration_enabled()) {
> + css_register_vmstate();
> + }
> }
>
> static void ccw_machine_2_10_class_options(MachineClass *mc)
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 8ab655c..c611b6f 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -1307,6 +1307,10 @@ static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f)
> {
> VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
>
> + if (css_migration_enabled()) {
> + /* we migrate via DeviceClass.vmsd */
> + return;
> + }
> /*
> * We save in legacy mode. The components take care of their own
> * compat. representation (based on css_migration_enabled).
> @@ -1318,6 +1322,10 @@ static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
> {
> VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
>
> + if (css_migration_enabled()) {
> + /* we migrate via DeviceClass.vmsd */
> + return 0;
> + }
> /*
> * We load in legacy mode. The components take take care to read
> * only stuff which is actually there (based on css_migration_enabled).
> @@ -1365,6 +1373,11 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
> sch->id.cu_model = virtio_bus_get_vdev_id(&dev->bus);
>
>
> + /* Avoid generating unknown section for legacy migration target. */
> + if (!css_migration_enabled()) {
> + DEVICE_GET_CLASS(ccw_dev)->vmsd = NULL;
> + }
> +
That's a very odd thing to do; can't you use a .needed at the
top level of the vmstate_virtio_ccw_dev to avoid having to
set it to NULL like this?
> css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
> d->hotplugged, 1);
> }
> @@ -1657,6 +1670,7 @@ static void virtio_ccw_device_class_init(ObjectClass *klass, void *data)
> dc->realize = virtio_ccw_busdev_realize;
> dc->exit = virtio_ccw_busdev_exit;
> dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
> + dc->vmsd = &vmstate_virtio_ccw_dev;
> }
>
> static const TypeInfo virtio_ccw_device_info = {
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index dbe093e..be5eb81 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -207,4 +207,8 @@ extern PropertyInfo css_devid_ro_propinfo;
> * is responsible for unregistering and freeing it.
> */
> SubchDev *css_create_virtual_sch(CssDevId bus_id, Error **errp);
> +
> +/** Turn on css migration */
> +void css_register_vmstate(void);
> +
> #endif
> --
> 2.10.2
Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration
2017-05-05 17:35 ` [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration Halil Pasic
2017-05-08 16:55 ` Dr. David Alan Gilbert
@ 2017-05-08 17:36 ` Dr. David Alan Gilbert
2017-05-08 17:53 ` Halil Pasic
1 sibling, 1 reply; 42+ messages in thread
From: Dr. David Alan Gilbert @ 2017-05-08 17:36 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, Michael S. Tsirkin, qemu-devel
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> Let us use the freshly introduced vmstate migration helpers instead of
> saving/loading the config manually.
>
> To achieve this we need to hack the config_vector which is a common
> VirtIO state in the middle of the VirtioCcwDevice state representation.
> This somewhat ugly but we have no choice because the stream format needs
> to be preserved.
>
> Still no changes in behavior, but the dead code we added previously is
> finally awakening to life.
>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> ---
> hw/s390x/virtio-ccw.c | 116 +++++++++++++++++++-------------------------------
> 1 file changed, 44 insertions(+), 72 deletions(-)
>
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index c2badfe..8ab655c 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -57,6 +57,32 @@ static int virtio_ccw_dev_post_load(void *opaque, int version_id)
> return 0;
> }
>
> +static int get_config_vector(QEMUFile *f, void *pv, size_t size,
> + VMStateField *field)
> +{
> + VirtioCcwDevice *dev = pv;
> + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
> +
> + qemu_get_be16s(f, &vdev->config_vector);
> + return 0;
> +}
> +
> +static int put_config_vector(QEMUFile *f, void *pv, size_t size,
> + VMStateField *field, QJSON *vmdesc)
> +{
> + VirtioCcwDevice *dev = pv;
> + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
> +
> + qemu_put_be16(f, vdev->config_vector);
> + return 0;
> +}
> +
> +const VMStateInfo vmstate_info_config_vector = {
> + .name = "config_vector",
> + .get = get_config_vector,
> + .put = put_config_vector,
> +};
> +
> const VMStateDescription vmstate_virtio_ccw_dev = {
> .name = "s390_virtio_ccw_dev",
> .version_id = 1,
> @@ -67,6 +93,14 @@ const VMStateDescription vmstate_virtio_ccw_dev = {
> VMSTATE_PTR_TO_IND_ADDR(indicators, VirtioCcwDevice),
> VMSTATE_PTR_TO_IND_ADDR(indicators2, VirtioCcwDevice),
> VMSTATE_PTR_TO_IND_ADDR(summary_indicator, VirtioCcwDevice),
> + {
> + /*
> + * Ugly hack because VirtIODevice does not migrate itself.
> + * This also makes legacy via vmstate_save_state possible.
> + */
> + .name = "virtio/config_vector",
> + .info = &vmstate_info_config_vector,
> + },
I'm a bit confused - isn't just this bit the bit going
through the vdev->load_config hook?
Dave
> VMSTATE_STRUCT(routes, VirtioCcwDevice, 1, vmstate_adapter_routes, \
> AdapterRoutes),
> VMSTATE_UINT8(thinint_isc, VirtioCcwDevice),
> @@ -1272,85 +1306,23 @@ static int virtio_ccw_load_queue(DeviceState *d, int n, QEMUFile *f)
> static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f)
> {
> VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> - CcwDevice *ccw_dev = CCW_DEVICE(d);
> - SubchDev *s = ccw_dev->sch;
> - VirtIODevice *vdev = virtio_ccw_get_vdev(s);
>
> - subch_device_save(s, f);
> - if (dev->indicators != NULL) {
> - qemu_put_be32(f, dev->indicators->len);
> - qemu_put_be64(f, dev->indicators->addr);
> - } else {
> - qemu_put_be32(f, 0);
> - qemu_put_be64(f, 0UL);
> - }
> - if (dev->indicators2 != NULL) {
> - qemu_put_be32(f, dev->indicators2->len);
> - qemu_put_be64(f, dev->indicators2->addr);
> - } else {
> - qemu_put_be32(f, 0);
> - qemu_put_be64(f, 0UL);
> - }
> - if (dev->summary_indicator != NULL) {
> - qemu_put_be32(f, dev->summary_indicator->len);
> - qemu_put_be64(f, dev->summary_indicator->addr);
> - } else {
> - qemu_put_be32(f, 0);
> - qemu_put_be64(f, 0UL);
> - }
> - qemu_put_be16(f, vdev->config_vector);
> - qemu_put_be64(f, dev->routes.adapter.ind_offset);
> - qemu_put_byte(f, dev->thinint_isc);
> - qemu_put_be32(f, dev->revision);
> + /*
> + * We save in legacy mode. The components take care of their own
> + * compat. representation (based on css_migration_enabled).
> + */
> + vmstate_save_state(f, &vmstate_virtio_ccw_dev, dev, NULL);
> }
>
> static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
> {
> VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> - CcwDevice *ccw_dev = CCW_DEVICE(d);
> - CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev);
> - SubchDev *s = ccw_dev->sch;
> - VirtIODevice *vdev = virtio_ccw_get_vdev(s);
> - int len;
> -
> - s->driver_data = dev;
> - subch_device_load(s, f);
> - /* Re-fill subch_id after loading the subchannel states.*/
> - if (ck->refill_ids) {
> - ck->refill_ids(ccw_dev);
> - }
> - len = qemu_get_be32(f);
> - if (len != 0) {
> - dev->indicators = get_indicator(qemu_get_be64(f), len);
> - } else {
> - qemu_get_be64(f);
> - dev->indicators = NULL;
> - }
> - len = qemu_get_be32(f);
> - if (len != 0) {
> - dev->indicators2 = get_indicator(qemu_get_be64(f), len);
> - } else {
> - qemu_get_be64(f);
> - dev->indicators2 = NULL;
> - }
> - len = qemu_get_be32(f);
> - if (len != 0) {
> - dev->summary_indicator = get_indicator(qemu_get_be64(f), len);
> - } else {
> - qemu_get_be64(f);
> - dev->summary_indicator = NULL;
> - }
> - qemu_get_be16s(f, &vdev->config_vector);
> - dev->routes.adapter.ind_offset = qemu_get_be64(f);
> - dev->thinint_isc = qemu_get_byte(f);
> - dev->revision = qemu_get_be32(f);
> - if (s->thinint_active) {
> - dev->routes.adapter.adapter_id = css_get_adapter_id(
> - CSS_IO_ADAPTER_VIRTIO,
> - dev->thinint_isc);
> - }
>
> - return 0;
> + /*
> + * We load in legacy mode. The components take take care to read
> + * only stuff which is actually there (based on css_migration_enabled).
> + */
> + return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1);
> }
>
> static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
> --
> 2.10.2
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration
2017-05-08 17:36 ` Dr. David Alan Gilbert
@ 2017-05-08 17:53 ` Halil Pasic
2017-05-08 17:59 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 42+ messages in thread
From: Halil Pasic @ 2017-05-08 17:53 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Cornelia Huck, Michael S. Tsirkin, qemu-devel
On 05/08/2017 07:36 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>> Let us use the freshly introduced vmstate migration helpers instead of
>> saving/loading the config manually.
>>
>> To achieve this we need to hack the config_vector which is a common
>> VirtIO state in the middle of the VirtioCcwDevice state representation.
>> This somewhat ugly but we have no choice because the stream format needs
>> to be preserved.
>>
>> Still no changes in behavior, but the dead code we added previously is
>> finally awakening to life.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>> ---
>> hw/s390x/virtio-ccw.c | 116 +++++++++++++++++++-------------------------------
>> 1 file changed, 44 insertions(+), 72 deletions(-)
>>
>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>> index c2badfe..8ab655c 100644
>> --- a/hw/s390x/virtio-ccw.c
>> +++ b/hw/s390x/virtio-ccw.c
>> @@ -57,6 +57,32 @@ static int virtio_ccw_dev_post_load(void *opaque, int version_id)
>> return 0;
>> }
>>
>> +static int get_config_vector(QEMUFile *f, void *pv, size_t size,
>> + VMStateField *field)
>> +{
>> + VirtioCcwDevice *dev = pv;
>> + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
>> +
>> + qemu_get_be16s(f, &vdev->config_vector);
>> + return 0;
>> +}
>> +
>> +static int put_config_vector(QEMUFile *f, void *pv, size_t size,
>> + VMStateField *field, QJSON *vmdesc)
>> +{
>> + VirtioCcwDevice *dev = pv;
>> + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
>> +
>> + qemu_put_be16(f, vdev->config_vector);
>> + return 0;
>> +}
>> +
>> +const VMStateInfo vmstate_info_config_vector = {
>> + .name = "config_vector",
>> + .get = get_config_vector,
>> + .put = put_config_vector,
>> +};
>> +
>> const VMStateDescription vmstate_virtio_ccw_dev = {
>> .name = "s390_virtio_ccw_dev",
>> .version_id = 1,
>> @@ -67,6 +93,14 @@ const VMStateDescription vmstate_virtio_ccw_dev = {
>> VMSTATE_PTR_TO_IND_ADDR(indicators, VirtioCcwDevice),
>> VMSTATE_PTR_TO_IND_ADDR(indicators2, VirtioCcwDevice),
>> VMSTATE_PTR_TO_IND_ADDR(summary_indicator, VirtioCcwDevice),
>> + {
>> + /*
>> + * Ugly hack because VirtIODevice does not migrate itself.
>> + * This also makes legacy via vmstate_save_state possible.
>> + */
>> + .name = "virtio/config_vector",
>> + .info = &vmstate_info_config_vector,
>> + },
>
> I'm a bit confused - isn't just this bit the bit going
> through the vdev->load_config hook?
>
Exactly! If you examine the part underscored with ============== in
virtio_ccw_(load|save)_config (that is what is behind vdev->load_config)
you should see that we use vmstate_virtio_ccw_dev (that is this
VMStateDescription to implement these function.
Actually virtio_ccw_(load|save)_config won't do anything after patch 9
and for new machines since the VirtIOCcwDevice is going to migrate
itself via DeviceClass.vmsd like other "normal" devices, and we fall
back to the VirtIO specific callbacks only in "legacy mode".
I hope this helps!
Halil
>
>> VMSTATE_STRUCT(routes, VirtioCcwDevice, 1, vmstate_adapter_routes, \
>> AdapterRoutes),
>> VMSTATE_UINT8(thinint_isc, VirtioCcwDevice),
>> @@ -1272,85 +1306,23 @@ static int virtio_ccw_load_queue(DeviceState *d, int n, QEMUFile *f)
>> static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f)
>> {
>> VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
>> - CcwDevice *ccw_dev = CCW_DEVICE(d);
>> - SubchDev *s = ccw_dev->sch;
>> - VirtIODevice *vdev = virtio_ccw_get_vdev(s);
>>
>> - subch_device_save(s, f);
>> - if (dev->indicators != NULL) {
>> - qemu_put_be32(f, dev->indicators->len);
>> - qemu_put_be64(f, dev->indicators->addr);
>> - } else {
>> - qemu_put_be32(f, 0);
>> - qemu_put_be64(f, 0UL);
>> - }
>> - if (dev->indicators2 != NULL) {
>> - qemu_put_be32(f, dev->indicators2->len);
>> - qemu_put_be64(f, dev->indicators2->addr);
>> - } else {
>> - qemu_put_be32(f, 0);
>> - qemu_put_be64(f, 0UL);
>> - }
>> - if (dev->summary_indicator != NULL) {
>> - qemu_put_be32(f, dev->summary_indicator->len);
>> - qemu_put_be64(f, dev->summary_indicator->addr);
>> - } else {
>> - qemu_put_be32(f, 0);
>> - qemu_put_be64(f, 0UL);
>> - }
>> - qemu_put_be16(f, vdev->config_vector);
>> - qemu_put_be64(f, dev->routes.adapter.ind_offset);
>> - qemu_put_byte(f, dev->thinint_isc);
>> - qemu_put_be32(f, dev->revision);
>> + /*
>> + * We save in legacy mode. The components take care of their own
>> + * compat. representation (based on css_migration_enabled).
>> + */
>> + vmstate_save_state(f, &vmstate_virtio_ccw_dev, dev, NULL);
====================================================================
>> }
>>
>> static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
>> {
>> VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
>> - CcwDevice *ccw_dev = CCW_DEVICE(d);
>> - CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev);
>> - SubchDev *s = ccw_dev->sch;
>> - VirtIODevice *vdev = virtio_ccw_get_vdev(s);
>> - int len;
>> -
>> - s->driver_data = dev;
>> - subch_device_load(s, f);
>> - /* Re-fill subch_id after loading the subchannel states.*/
>> - if (ck->refill_ids) {
>> - ck->refill_ids(ccw_dev);
>> - }
>> - len = qemu_get_be32(f);
>> - if (len != 0) {
>> - dev->indicators = get_indicator(qemu_get_be64(f), len);
>> - } else {
>> - qemu_get_be64(f);
>> - dev->indicators = NULL;
>> - }
>> - len = qemu_get_be32(f);
>> - if (len != 0) {
>> - dev->indicators2 = get_indicator(qemu_get_be64(f), len);
>> - } else {
>> - qemu_get_be64(f);
>> - dev->indicators2 = NULL;
>> - }
>> - len = qemu_get_be32(f);
>> - if (len != 0) {
>> - dev->summary_indicator = get_indicator(qemu_get_be64(f), len);
>> - } else {
>> - qemu_get_be64(f);
>> - dev->summary_indicator = NULL;
>> - }
>> - qemu_get_be16s(f, &vdev->config_vector);
>> - dev->routes.adapter.ind_offset = qemu_get_be64(f);
>> - dev->thinint_isc = qemu_get_byte(f);
>> - dev->revision = qemu_get_be32(f);
>> - if (s->thinint_active) {
>> - dev->routes.adapter.adapter_id = css_get_adapter_id(
>> - CSS_IO_ADAPTER_VIRTIO,
>> - dev->thinint_isc);
>> - }
>>
>> - return 0;
>> + /*
>> + * We load in legacy mode. The components take take care to read
>> + * only stuff which is actually there (based on css_migration_enabled).
>> + */
>> + return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1);
========================================================================
>> }
>>
>> static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
>> --
>> 2.10.2
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration
2017-05-08 17:53 ` Halil Pasic
@ 2017-05-08 17:59 ` Dr. David Alan Gilbert
2017-05-08 18:27 ` Halil Pasic
0 siblings, 1 reply; 42+ messages in thread
From: Dr. David Alan Gilbert @ 2017-05-08 17:59 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, Michael S. Tsirkin, qemu-devel
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>
>
> On 05/08/2017 07:36 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> >> Let us use the freshly introduced vmstate migration helpers instead of
> >> saving/loading the config manually.
> >>
> >> To achieve this we need to hack the config_vector which is a common
> >> VirtIO state in the middle of the VirtioCcwDevice state representation.
> >> This somewhat ugly but we have no choice because the stream format needs
> >> to be preserved.
> >>
> >> Still no changes in behavior, but the dead code we added previously is
> >> finally awakening to life.
> >>
> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >> ---
> >> ---
> >> hw/s390x/virtio-ccw.c | 116 +++++++++++++++++++-------------------------------
> >> 1 file changed, 44 insertions(+), 72 deletions(-)
> >>
> >> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> >> index c2badfe..8ab655c 100644
> >> --- a/hw/s390x/virtio-ccw.c
> >> +++ b/hw/s390x/virtio-ccw.c
> >> @@ -57,6 +57,32 @@ static int virtio_ccw_dev_post_load(void *opaque, int version_id)
> >> return 0;
> >> }
> >>
> >> +static int get_config_vector(QEMUFile *f, void *pv, size_t size,
> >> + VMStateField *field)
> >> +{
> >> + VirtioCcwDevice *dev = pv;
> >> + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
> >> +
> >> + qemu_get_be16s(f, &vdev->config_vector);
> >> + return 0;
> >> +}
> >> +
> >> +static int put_config_vector(QEMUFile *f, void *pv, size_t size,
> >> + VMStateField *field, QJSON *vmdesc)
> >> +{
> >> + VirtioCcwDevice *dev = pv;
> >> + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
> >> +
> >> + qemu_put_be16(f, vdev->config_vector);
> >> + return 0;
> >> +}
> >> +
> >> +const VMStateInfo vmstate_info_config_vector = {
> >> + .name = "config_vector",
> >> + .get = get_config_vector,
> >> + .put = put_config_vector,
> >> +};
> >> +
> >> const VMStateDescription vmstate_virtio_ccw_dev = {
> >> .name = "s390_virtio_ccw_dev",
> >> .version_id = 1,
> >> @@ -67,6 +93,14 @@ const VMStateDescription vmstate_virtio_ccw_dev = {
> >> VMSTATE_PTR_TO_IND_ADDR(indicators, VirtioCcwDevice),
> >> VMSTATE_PTR_TO_IND_ADDR(indicators2, VirtioCcwDevice),
> >> VMSTATE_PTR_TO_IND_ADDR(summary_indicator, VirtioCcwDevice),
> >> + {
> >> + /*
> >> + * Ugly hack because VirtIODevice does not migrate itself.
> >> + * This also makes legacy via vmstate_save_state possible.
> >> + */
> >> + .name = "virtio/config_vector",
> >> + .info = &vmstate_info_config_vector,
> >> + },
> >
> > I'm a bit confused - isn't just this bit the bit going
> > through the vdev->load_config hook?
> >
>
> Exactly! If you examine the part underscored with ============== in
> virtio_ccw_(load|save)_config (that is what is behind vdev->load_config)
> you should see that we use vmstate_virtio_ccw_dev (that is this
> VMStateDescription to implement these function.
>
> Actually virtio_ccw_(load|save)_config won't do anything after patch 9
> and for new machines since the VirtIOCcwDevice is going to migrate
> itself via DeviceClass.vmsd like other "normal" devices, and we fall
> back to the VirtIO specific callbacks only in "legacy mode".
>
> I hope this helps!
Hmm, still confused!
Why are you changing a Virtio device not to use the same migration
oddities as all the other virtio devices?
I was assuming we'd have to change the virtio core code to
solve that for all virtio devices.
Dave
> Halil
>
>
> >
> >> VMSTATE_STRUCT(routes, VirtioCcwDevice, 1, vmstate_adapter_routes, \
> >> AdapterRoutes),
> >> VMSTATE_UINT8(thinint_isc, VirtioCcwDevice),
> >> @@ -1272,85 +1306,23 @@ static int virtio_ccw_load_queue(DeviceState *d, int n, QEMUFile *f)
> >> static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f)
> >> {
> >> VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> >> - CcwDevice *ccw_dev = CCW_DEVICE(d);
> >> - SubchDev *s = ccw_dev->sch;
> >> - VirtIODevice *vdev = virtio_ccw_get_vdev(s);
> >>
> >> - subch_device_save(s, f);
> >> - if (dev->indicators != NULL) {
> >> - qemu_put_be32(f, dev->indicators->len);
> >> - qemu_put_be64(f, dev->indicators->addr);
> >> - } else {
> >> - qemu_put_be32(f, 0);
> >> - qemu_put_be64(f, 0UL);
> >> - }
> >> - if (dev->indicators2 != NULL) {
> >> - qemu_put_be32(f, dev->indicators2->len);
> >> - qemu_put_be64(f, dev->indicators2->addr);
> >> - } else {
> >> - qemu_put_be32(f, 0);
> >> - qemu_put_be64(f, 0UL);
> >> - }
> >> - if (dev->summary_indicator != NULL) {
> >> - qemu_put_be32(f, dev->summary_indicator->len);
> >> - qemu_put_be64(f, dev->summary_indicator->addr);
> >> - } else {
> >> - qemu_put_be32(f, 0);
> >> - qemu_put_be64(f, 0UL);
> >> - }
> >> - qemu_put_be16(f, vdev->config_vector);
> >> - qemu_put_be64(f, dev->routes.adapter.ind_offset);
> >> - qemu_put_byte(f, dev->thinint_isc);
> >> - qemu_put_be32(f, dev->revision);
> >> + /*
> >> + * We save in legacy mode. The components take care of their own
> >> + * compat. representation (based on css_migration_enabled).
> >> + */
> >> + vmstate_save_state(f, &vmstate_virtio_ccw_dev, dev, NULL);
> ====================================================================
>
> >> }
> >>
> >> static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
> >> {
> >> VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> >> - CcwDevice *ccw_dev = CCW_DEVICE(d);
> >> - CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev);
> >> - SubchDev *s = ccw_dev->sch;
> >> - VirtIODevice *vdev = virtio_ccw_get_vdev(s);
> >> - int len;
> >> -
> >> - s->driver_data = dev;
> >> - subch_device_load(s, f);
> >> - /* Re-fill subch_id after loading the subchannel states.*/
> >> - if (ck->refill_ids) {
> >> - ck->refill_ids(ccw_dev);
> >> - }
> >> - len = qemu_get_be32(f);
> >> - if (len != 0) {
> >> - dev->indicators = get_indicator(qemu_get_be64(f), len);
> >> - } else {
> >> - qemu_get_be64(f);
> >> - dev->indicators = NULL;
> >> - }
> >> - len = qemu_get_be32(f);
> >> - if (len != 0) {
> >> - dev->indicators2 = get_indicator(qemu_get_be64(f), len);
> >> - } else {
> >> - qemu_get_be64(f);
> >> - dev->indicators2 = NULL;
> >> - }
> >> - len = qemu_get_be32(f);
> >> - if (len != 0) {
> >> - dev->summary_indicator = get_indicator(qemu_get_be64(f), len);
> >> - } else {
> >> - qemu_get_be64(f);
> >> - dev->summary_indicator = NULL;
> >> - }
> >> - qemu_get_be16s(f, &vdev->config_vector);
> >> - dev->routes.adapter.ind_offset = qemu_get_be64(f);
> >> - dev->thinint_isc = qemu_get_byte(f);
> >> - dev->revision = qemu_get_be32(f);
> >> - if (s->thinint_active) {
> >> - dev->routes.adapter.adapter_id = css_get_adapter_id(
> >> - CSS_IO_ADAPTER_VIRTIO,
> >> - dev->thinint_isc);
> >> - }
> >>
> >> - return 0;
> >> + /*
> >> + * We load in legacy mode. The components take take care to read
> >> + * only stuff which is actually there (based on css_migration_enabled).
> >> + */
> >> + return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1);
> ========================================================================
>
> >> }
> >>
> >> static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
> >> --
> >> 2.10.2
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 09/10] s390x/css: turn on channel subsystem migration
2017-05-08 17:27 ` Dr. David Alan Gilbert
@ 2017-05-08 18:03 ` Halil Pasic
2017-05-08 18:37 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 42+ messages in thread
From: Halil Pasic @ 2017-05-08 18:03 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Cornelia Huck, qemu-devel, Michael S. Tsirkin
On 05/08/2017 07:27 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>> Turn on migration for the channel subsystem and the new scheme for
>> migrating virtio-ccw proxy devices (instead of letting the transport
>> independent child device migrate it's proxy, use the usual
>> DeviceClass.vmsd mechanism) for future machine versions.
[..]
>> +void css_register_vmstate(void)
>> +{
>> + vmstate_register(NULL, 0, &vmstate_css, &channel_subsys);
>> +}
>> +
>
> Why isn't that attached to a device vmsd?
Because there is no device. The channel subsystem is not modeled
as a QEMU device but it does have state which needs to be
migrated.
[..]
>> @@ -1365,6 +1373,11 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
>> sch->id.cu_model = virtio_bus_get_vdev_id(&dev->bus);
>>
>>
>> + /* Avoid generating unknown section for legacy migration target. */
>> + if (!css_migration_enabled()) {
>> + DEVICE_GET_CLASS(ccw_dev)->vmsd = NULL;
>> + }
>> +
>
> That's a very odd thing to do; can't you use a .needed at the
> top level of the vmstate_virtio_ccw_dev to avoid having to
> set it to NULL like this?
>
I agree it's odd. As far as I remember I can't use .needed but
I will double check.
Many thanks for your review!
Halil
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration
2017-05-08 17:59 ` Dr. David Alan Gilbert
@ 2017-05-08 18:27 ` Halil Pasic
2017-05-08 18:42 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 42+ messages in thread
From: Halil Pasic @ 2017-05-08 18:27 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Cornelia Huck, Michael S. Tsirkin, qemu-devel
On 05/08/2017 07:59 PM, Dr. David Alan Gilbert wrote:
>>>> const VMStateDescription vmstate_virtio_ccw_dev = {
>>>> .name = "s390_virtio_ccw_dev",
>>>> .version_id = 1,
>>>> @@ -67,6 +93,14 @@ const VMStateDescription vmstate_virtio_ccw_dev = {
>>>> VMSTATE_PTR_TO_IND_ADDR(indicators, VirtioCcwDevice),
>>>> VMSTATE_PTR_TO_IND_ADDR(indicators2, VirtioCcwDevice),
>>>> VMSTATE_PTR_TO_IND_ADDR(summary_indicator, VirtioCcwDevice),
>>>> + {
>>>> + /*
>>>> + * Ugly hack because VirtIODevice does not migrate itself.
>>>> + * This also makes legacy via vmstate_save_state possible.
>>>> + */
>>>> + .name = "virtio/config_vector",
>>>> + .info = &vmstate_info_config_vector,
>>>> + },
>>> I'm a bit confused - isn't just this bit the bit going
>>> through the vdev->load_config hook?
>>>
>> Exactly! If you examine the part underscored with ============== in
>> virtio_ccw_(load|save)_config (that is what is behind vdev->load_config)
>> you should see that we use vmstate_virtio_ccw_dev (that is this
>> VMStateDescription to implement these function.
>>
>> Actually virtio_ccw_(load|save)_config won't do anything after patch 9
>> and for new machines since the VirtIOCcwDevice is going to migrate
>> itself via DeviceClass.vmsd like other "normal" devices, and we fall
>> back to the VirtIO specific callbacks only in "legacy mode".
>>
>> I hope this helps!
> Hmm, still confused!
> Why are you changing a Virtio device not to use the same migration
> oddities as all the other virtio devices?
>
> I was assuming we'd have to change the virtio core code to
> solve that for all virtio devices.
>
You can ask difficult questions ;). First I'm not aware of any
ongoing effort to solve this for all virtio devices by changing
the core, and probably breaking compatibility for all devices
(in a sense I break migration compatibility for all virtio-ccw
devices). To be honest, I have considered that move unlikely,
but the possibility is one of my reasons for seeking an upstream
discussion and having You and Michael on cc.
Why not use virtio oddities? Because they are oddities. I have
figured, it's a good idea to separate the migration of the
proxy form the rest: we have two QEMU Device objects and it
should be good practice, that these are migrating themselves via
DeviceClass.vmsd. That's what I get with this patch set,
for new machine versions (since we can not fix the past), and
with the notable difference of config_vector, because it is
defined as a common infrastructure (struct VirtIODevice) but
ain't migrated as a common virtio infrastructure.
Would you suggest to rather keep the oddities? Should I expect
to see a generic solution to the problem sometime soon?
Halil
> Dave
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 09/10] s390x/css: turn on channel subsystem migration
2017-05-08 18:03 ` Halil Pasic
@ 2017-05-08 18:37 ` Dr. David Alan Gilbert
2017-05-09 17:27 ` Halil Pasic
0 siblings, 1 reply; 42+ messages in thread
From: Dr. David Alan Gilbert @ 2017-05-08 18:37 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, qemu-devel, Michael S. Tsirkin
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>
>
> On 05/08/2017 07:27 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> >> Turn on migration for the channel subsystem and the new scheme for
> >> migrating virtio-ccw proxy devices (instead of letting the transport
> >> independent child device migrate it's proxy, use the usual
> >> DeviceClass.vmsd mechanism) for future machine versions.
>
> [..]
>
> >> +void css_register_vmstate(void)
> >> +{
> >> + vmstate_register(NULL, 0, &vmstate_css, &channel_subsys);
> >> +}
> >> +
> >
> > Why isn't that attached to a device vmsd?
>
> Because there is no device. The channel subsystem is not modeled
> as a QEMU device but it does have state which needs to be
> migrated.
Ah OK.
> [..]
>
> >> @@ -1365,6 +1373,11 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
> >> sch->id.cu_model = virtio_bus_get_vdev_id(&dev->bus);
> >>
> >>
> >> + /* Avoid generating unknown section for legacy migration target. */
> >> + if (!css_migration_enabled()) {
> >> + DEVICE_GET_CLASS(ccw_dev)->vmsd = NULL;
> >> + }
> >> +
> >
> > That's a very odd thing to do; can't you use a .needed at the
> > top level of the vmstate_virtio_ccw_dev to avoid having to
> > set it to NULL like this?
> >
>
> I agree it's odd. As far as I remember I can't use .needed but
> I will double check.
You can have top level .needed's - both vmstate_globalstate in
migration.c and colo's colo_state use them.
Dave
> Many thanks for your review!
>
> Halil
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration
2017-05-08 18:27 ` Halil Pasic
@ 2017-05-08 18:42 ` Dr. David Alan Gilbert
2017-05-10 11:52 ` Halil Pasic
0 siblings, 1 reply; 42+ messages in thread
From: Dr. David Alan Gilbert @ 2017-05-08 18:42 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, Michael S. Tsirkin, qemu-devel
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>
>
> On 05/08/2017 07:59 PM, Dr. David Alan Gilbert wrote:
> >>>> const VMStateDescription vmstate_virtio_ccw_dev = {
> >>>> .name = "s390_virtio_ccw_dev",
> >>>> .version_id = 1,
> >>>> @@ -67,6 +93,14 @@ const VMStateDescription vmstate_virtio_ccw_dev = {
> >>>> VMSTATE_PTR_TO_IND_ADDR(indicators, VirtioCcwDevice),
> >>>> VMSTATE_PTR_TO_IND_ADDR(indicators2, VirtioCcwDevice),
> >>>> VMSTATE_PTR_TO_IND_ADDR(summary_indicator, VirtioCcwDevice),
> >>>> + {
> >>>> + /*
> >>>> + * Ugly hack because VirtIODevice does not migrate itself.
> >>>> + * This also makes legacy via vmstate_save_state possible.
> >>>> + */
> >>>> + .name = "virtio/config_vector",
> >>>> + .info = &vmstate_info_config_vector,
> >>>> + },
> >>> I'm a bit confused - isn't just this bit the bit going
> >>> through the vdev->load_config hook?
> >>>
> >> Exactly! If you examine the part underscored with ============== in
> >> virtio_ccw_(load|save)_config (that is what is behind vdev->load_config)
> >> you should see that we use vmstate_virtio_ccw_dev (that is this
> >> VMStateDescription to implement these function.
> >>
> >> Actually virtio_ccw_(load|save)_config won't do anything after patch 9
> >> and for new machines since the VirtIOCcwDevice is going to migrate
> >> itself via DeviceClass.vmsd like other "normal" devices, and we fall
> >> back to the VirtIO specific callbacks only in "legacy mode".
> >>
> >> I hope this helps!
> > Hmm, still confused!
> > Why are you changing a Virtio device not to use the same migration
> > oddities as all the other virtio devices?
> >
> > I was assuming we'd have to change the virtio core code to
> > solve that for all virtio devices.
> >
>
> You can ask difficult questions ;). First I'm not aware of any
> ongoing effort to solve this for all virtio devices by changing
> the core, and probably breaking compatibility for all devices
> (in a sense I break migration compatibility for all virtio-ccw
> devices). To be honest, I have considered that move unlikely,
> but the possibility is one of my reasons for seeking an upstream
> discussion and having You and Michael on cc.
Well I have been trying to improve it, but that code is particularly
nasty; and I don't want to break compatibility. I had some ideas,
for example I was thinking of changing the vdev->load_config to
a VMState* and then calling a vmstate_load_state(f, vdc->config,...
from virtio_load - but there's some challenging casting and hackery
there.
>
> Why not use virtio oddities? Because they are oddities. I have
> figured, it's a good idea to separate the migration of the
> proxy form the rest: we have two QEMU Device objects and it
> should be good practice, that these are migrating themselves via
> DeviceClass.vmsd. That's what I get with this patch set,
> for new machine versions (since we can not fix the past), and
> with the notable difference of config_vector, because it is
> defined as a common infrastructure (struct VirtIODevice) but
> ain't migrated as a common virtio infrastructure.
Have you got a bit of a description of your classes/structure - it's
a little hard to get my head around.
> Would you suggest to rather keep the oddities? Should I expect
> to see a generic solution to the problem sometime soon?
Not soon I fear; virtio_load is just too hairy.
Dave
> Halil
>
> > Dave
> >
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css
2017-05-08 16:45 ` Dr. David Alan Gilbert
@ 2017-05-09 12:00 ` Halil Pasic
2017-05-15 18:01 ` Dr. David Alan Gilbert
2017-05-09 12:20 ` Halil Pasic
1 sibling, 1 reply; 42+ messages in thread
From: Halil Pasic @ 2017-05-09 12:00 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Cornelia Huck, Michael S. Tsirkin, qemu-devel
On 05/08/2017 06:45 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>> As a preparation for switching to a vmstate based migration let us
>> introduce vmstate entities (e.g. VMStateDescription) for the css entities
>> to be migrated. Alongside some comments explaining or indicating the not
>> migration of certain members are introduced too.
>>
>> No changes in behavior, we just added some dead code -- which should
>> rise to life soon.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>> hw/s390x/css.c | 276 +++++++++++++++++++++++++++++++++++++++++++++++++
>> include/hw/s390x/css.h | 10 +-
>> 2 files changed, 285 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index c03bb20..2bda7d0 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -20,29 +20,231 @@
>> #include "hw/s390x/css.h"
>> #include "trace.h"
>> #include "hw/s390x/s390_flic.h"
>> +#include "hw/s390x/s390-virtio-ccw.h"
>>
[..]
>> +static int css_get_ind_addr(QEMUFile *f, void *pv, size_t size,
>> + VMStateField *field)
>> +{
>> + int32_t len;
>> + IndAddr **ind_addr = pv;
>> +
>> + len = qemu_get_be32(f);
>> + if (len != 0) {
>> + *ind_addr = get_indicator(qemu_get_be64(f), len);
>> + } else {
>> + qemu_get_be64(f);
>> + *ind_addr = NULL;
>> + }
>> + return 0;
>> +}
>> +
>> +static int css_put_ind_addr(QEMUFile *f, void *pv, size_t size,
>> + VMStateField *field, QJSON *vmdesc)
>> +{
>> + IndAddr *ind_addr = *(IndAddr **) pv;
>> +
>> + if (ind_addr != NULL) {
>> + qemu_put_be32(f, ind_addr->len);
>> + qemu_put_be64(f, ind_addr->addr);
>> + } else {
>> + qemu_put_be32(f, 0);
>> + qemu_put_be64(f, 0UL);
>> + }
>> + return 0;
>> +}
>> +
>> +const VMStateInfo vmstate_info_ind_addr = {
>> + .name = "s390_ind_addr",
>> + .get = css_get_ind_addr,
>> + .put = css_put_ind_addr
>> +};
>
> You should be able to avoid this .get/.put by using VMSTATE_WITH_TMP,
> declare a temporary struct something like:
> struct tmp_ind_addr {
> IndAddr *parent;
> uint32_t len;
> uint64_t addr;
> }
>
> and then your .get/.put routines turn into pre_save/post_load
> routines to just setup the len/addr.
>
I don't think this is going to work -- unfortunately! You can see below,
how this IndAddr* migration stuff is supposed to be used:
the client code just uses the VMSTATE_PTR_TO_IND_ADDR macro as a
field when describing state which needs and IndAddr* migrated.
The problem is, we do not know in what state will this field
be embedded, the pre_save/post_load called by put_tmp/get_tmp
is however copying the pointer to this state into the parent.
So instead of having a pointer to IndAddr* in those functions
and updating it accordingly, I would have to find the IndAddr*
in some arbitrary state (in our case VirtioCcwDevice) first,
and I lack information for that.
If it's hard to follow I can give you the patch I was debugging
to come to this conclusion. (By the way I ended up with 10
lines of code more than in this version, and although I think
it looks nicer, it's simpler only if one knows how WITH_TMP
works. My plan was to ask you which version do you like more
and go with that before I realized it ain't gonna work.)
>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
>> index f1f0d7f..6a451b2 100644
>> --- a/include/hw/s390x/css.h
[..]
>>
>> +extern const VMStateInfo vmstate_info_ind_addr;
>> +
>> +#define VMSTATE_PTR_TO_IND_ADDR(_f, _s) \
>> + VMSTATE_SINGLE(_f, _s, 1 , vmstate_info_ind_addr, IndAddr*)
>> +
>> IndAddr *get_indicator(hwaddr ind_addr, int len);
>> void release_indicator(AdapterInfo *adapter, IndAddr *indicator);
>> int map_indicator(AdapterInfo *adapter, IndAddr *indicator);
>> --
>> 2.10.2
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
Cheers,
Halil
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css
2017-05-08 16:45 ` Dr. David Alan Gilbert
2017-05-09 12:00 ` Halil Pasic
@ 2017-05-09 12:20 ` Halil Pasic
1 sibling, 0 replies; 42+ messages in thread
From: Halil Pasic @ 2017-05-09 12:20 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Cornelia Huck, qemu-devel, Michael S. Tsirkin
On 05/08/2017 06:45 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>> As a preparation for switching to a vmstate based migration let us
>> introduce vmstate entities (e.g. VMStateDescription) for the css entities
>> to be migrated. Alongside some comments explaining or indicating the not
>> migration of certain members are introduced too.
>>
>> No changes in behavior, we just added some dead code -- which should
>> rise to life soon.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>> hw/s390x/css.c | 276 +++++++++++++++++++++++++++++++++++++++++++++++++
>> include/hw/s390x/css.h | 10 +-
[..]
>> typedef struct IoAdapter {
>> uint32_t id;
>> uint8_t type;
>> @@ -60,10 +262,34 @@ typedef struct ChannelSubSys {
>> uint64_t chnmon_area;
>> CssImage *css[MAX_CSSID + 1];
>> uint8_t default_cssid;
>> + /* don't migrate */
/* populated at bus init time, not subject to migration */
>> IoAdapter *io_adapters[CSS_IO_ADAPTER_TYPE_NUMS][MAX_ISC + 1];
>> + /* don't migrate */
>
> You don't say *why*
>
Because its obvious if you stare at the code for a month or so.
Joke aside let me try again (above and below).
> Dave
>
/* migrated by the owning device when get_indicator is called */
>> QTAILQ_HEAD(, IndAddr) indicator_addresses;
That is by vmstate_info_ind_addr.get == css_get_ind_addr
I know it is convoluted but I don't think it is possible to simplify
the interactions with reasonable effort and within this patch set.
If things can be simplified, this needs to happen before or after the
vmstate conversion IMHO. I would like to keep the mantra no behavior changes
until patch 9 because otherwise it will get so complicated that I won't
feel comfortable myself.
Cheers,
Halil
>> } ChannelSubSys;
>>
>>
[..]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration
2017-05-08 16:55 ` Dr. David Alan Gilbert
@ 2017-05-09 17:05 ` Halil Pasic
2017-05-10 10:31 ` Dr. David Alan Gilbert
2017-05-10 10:38 ` Cornelia Huck
0 siblings, 2 replies; 42+ messages in thread
From: Halil Pasic @ 2017-05-09 17:05 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Cornelia Huck, qemu-devel, Michael S. Tsirkin
On 05/08/2017 06:55 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>> Let us use the freshly introduced vmstate migration helpers instead of
>> saving/loading the config manually.
>>
>> To achieve this we need to hack the config_vector which is a common
>> VirtIO state in the middle of the VirtioCcwDevice state representation.
>> This somewhat ugly but we have no choice because the stream format needs
>> to be preserved.
>>
>> Still no changes in behavior, but the dead code we added previously is
>> finally awakening to life.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>> ---
>> hw/s390x/virtio-ccw.c | 116 +++++++++++++++++++-------------------------------
>> 1 file changed, 44 insertions(+), 72 deletions(-)
>>
>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>> index c2badfe..8ab655c 100644
>> --- a/hw/s390x/virtio-ccw.c
>> +++ b/hw/s390x/virtio-ccw.c
>> @@ -57,6 +57,32 @@ static int virtio_ccw_dev_post_load(void *opaque, int version_id)
>> return 0;
>> }
>>
>> +static int get_config_vector(QEMUFile *f, void *pv, size_t size,
>> + VMStateField *field)
>> +{
>> + VirtioCcwDevice *dev = pv;
>> + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
>> +
>> + qemu_get_be16s(f, &vdev->config_vector);
>> + return 0;
>> +}
>> +
>> +static int put_config_vector(QEMUFile *f, void *pv, size_t size,
>> + VMStateField *field, QJSON *vmdesc)
>> +{
>> + VirtioCcwDevice *dev = pv;
>> + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
>> +
>> + qemu_put_be16(f, vdev->config_vector);
>> + return 0;
>> +}
> Again that should be doable using WITH_TMP.
> (I do wonder if we need a macro for cases where it's just a casting
> operation to another type).
>
Yeah this one can be done with WITH_TMP. Below is the patch on top of
this patch set. It's a bit more verbose (+6 lines) but it looks a bit
nicer and probably also safer in (terms of symmetric read and write).
If you think its the way to go I will squash it into this patch for
the next version.
-----------------------------8<-----------------------------------------
>From e92135590ab95cc565b37913de77a9ed17012933 Mon Sep 17 00:00:00 2001
From: Halil Pasic <pasic@linux.vnet.ibm.com>
Date: Tue, 9 May 2017 16:01:50 +0200
Subject: [PATCH 1/2] virtio-ccw: replace info with VMSTATE_WITH_TMP
Convert s VMSatateInfo based solution manipulating the migration stream
directly to VMSTATE_WITH_TMP witch keeps IO and transformation logic
separate.
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
hw/s390x/virtio-ccw.c | 40 +++++++++++++++++++++++-----------------
1 file changed, 23 insertions(+), 17 deletions(-)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index c611b6f..6ebc78a 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -57,30 +57,38 @@ static int virtio_ccw_dev_post_load(void *opaque, int version_id)
return 0;
}
-static int get_config_vector(QEMUFile *f, void *pv, size_t size,
- VMStateField *field)
+typedef struct VirtioCcwDeviceTmp {
+ VirtioCcwDevice *parent;
+ uint16_t config_vector;
+} VirtioCcwDeviceTmp;
+
+static void virtio_ccw_dev_tmp_pre_save(void *opaque)
{
- VirtioCcwDevice *dev = pv;
+ VirtioCcwDeviceTmp *tmp = opaque;
+ VirtioCcwDevice *dev = tmp->parent;
VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
- qemu_get_be16s(f, &vdev->config_vector);
- return 0;
+ tmp->config_vector = vdev->config_vector;
}
-static int put_config_vector(QEMUFile *f, void *pv, size_t size,
- VMStateField *field, QJSON *vmdesc)
+static int virtio_ccw_dev_tmp_post_load(void *opaque, int version_id)
{
- VirtioCcwDevice *dev = pv;
+ VirtioCcwDeviceTmp *tmp = opaque;
+ VirtioCcwDevice *dev = tmp->parent;
VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
- qemu_put_be16(f, vdev->config_vector);
+ vdev->config_vector = tmp->config_vector;
return 0;
}
-const VMStateInfo vmstate_info_config_vector = {
- .name = "config_vector",
- .get = get_config_vector,
- .put = put_config_vector,
+const VMStateDescription vmstate_virtio_ccw_dev_tmp = {
+ .name = "s390_virtio_ccw_dev_tmp",
+ .pre_save = virtio_ccw_dev_tmp_pre_save,
+ .post_load = virtio_ccw_dev_tmp_post_load,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT16(config_vector, VirtioCcwDeviceTmp),
+ VMSTATE_END_OF_LIST()
+ }
};
const VMStateDescription vmstate_virtio_ccw_dev = {
@@ -93,14 +101,12 @@ const VMStateDescription vmstate_virtio_ccw_dev = {
VMSTATE_PTR_TO_IND_ADDR(indicators, VirtioCcwDevice),
VMSTATE_PTR_TO_IND_ADDR(indicators2, VirtioCcwDevice),
VMSTATE_PTR_TO_IND_ADDR(summary_indicator, VirtioCcwDevice),
- {
/*
* Ugly hack because VirtIODevice does not migrate itself.
* This also makes legacy via vmstate_save_state possible.
*/
- .name = "virtio/config_vector",
- .info = &vmstate_info_config_vector,
- },
+ VMSTATE_WITH_TMP(VirtioCcwDevice, VirtioCcwDeviceTmp,
+ vmstate_virtio_ccw_dev_tmp),
VMSTATE_STRUCT(routes, VirtioCcwDevice, 1, vmstate_adapter_routes, \
AdapterRoutes),
VMSTATE_UINT8(thinint_isc, VirtioCcwDevice),
--
2.10.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 09/10] s390x/css: turn on channel subsystem migration
2017-05-08 18:37 ` Dr. David Alan Gilbert
@ 2017-05-09 17:27 ` Halil Pasic
0 siblings, 0 replies; 42+ messages in thread
From: Halil Pasic @ 2017-05-09 17:27 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Cornelia Huck, qemu-devel, Michael S. Tsirkin
On 05/08/2017 08:37 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>>
>>
>> On 05/08/2017 07:27 PM, Dr. David Alan Gilbert wrote:
>>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>>>> Turn on migration for the channel subsystem and the new scheme for
>>>> migrating virtio-ccw proxy devices (instead of letting the transport
>>>> independent child device migrate it's proxy, use the usual
>>>> DeviceClass.vmsd mechanism) for future machine versions.
>>
>> [..]
>>
>>>> @@ -1365,6 +1373,11 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
>>>> sch->id.cu_model = virtio_bus_get_vdev_id(&dev->bus);
>>>>
>>>>
>>>> + /* Avoid generating unknown section for legacy migration target. */
>>>> + if (!css_migration_enabled()) {
>>>> + DEVICE_GET_CLASS(ccw_dev)->vmsd = NULL;
>>>> + }
>>>> +
>>>
>>> That's a very odd thing to do; can't you use a .needed at the
>>> top level of the vmstate_virtio_ccw_dev to avoid having to
>>> set it to NULL like this?
>>>
>>
>> I agree it's odd. As far as I remember I can't use .needed but
>> I will double check.
>
> You can have top level .needed's - both vmstate_globalstate in
> migration.c and colo's colo_state use them.
>
Works like charm. I'm really happy to get rid of this ugly hunk. Thanks
a lot!
I was probably confused by the fact that I want to use the same vmsd with
vmstate_save_state when the needed is false. That works, but I have probably
blindly assumed (back then) it does not. Of course it does make sense to
ignore .needed in that function, because for a vmsd coming from a
recursive call while processing a filed then the non-presence of a field
should be indicated by field_exists.
I wonder if adding a comment at the definition site would be helpful.
Something like:
struct VMStateDescription {
....
void (*pre_save)(void *opaque);
+ /* Controls the existence of sections and subsections, but not fields. */
bool (*needed)(void *opaque);
VMStateField *fields;
const VMStateDescription **subsections;
};
Halil
> Dave
>
>> Many thanks for your review!
>>
>> Halil
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration
2017-05-09 17:05 ` Halil Pasic
@ 2017-05-10 10:31 ` Dr. David Alan Gilbert
2017-05-10 10:38 ` Cornelia Huck
1 sibling, 0 replies; 42+ messages in thread
From: Dr. David Alan Gilbert @ 2017-05-10 10:31 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, qemu-devel, Michael S. Tsirkin
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>
>
> On 05/08/2017 06:55 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> >> Let us use the freshly introduced vmstate migration helpers instead of
> >> saving/loading the config manually.
> >>
> >> To achieve this we need to hack the config_vector which is a common
> >> VirtIO state in the middle of the VirtioCcwDevice state representation.
> >> This somewhat ugly but we have no choice because the stream format needs
> >> to be preserved.
> >>
> >> Still no changes in behavior, but the dead code we added previously is
> >> finally awakening to life.
> >>
> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >> ---
> >> ---
> >> hw/s390x/virtio-ccw.c | 116 +++++++++++++++++++-------------------------------
> >> 1 file changed, 44 insertions(+), 72 deletions(-)
> >>
> >> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> >> index c2badfe..8ab655c 100644
> >> --- a/hw/s390x/virtio-ccw.c
> >> +++ b/hw/s390x/virtio-ccw.c
> >> @@ -57,6 +57,32 @@ static int virtio_ccw_dev_post_load(void *opaque, int version_id)
> >> return 0;
> >> }
> >>
> >> +static int get_config_vector(QEMUFile *f, void *pv, size_t size,
> >> + VMStateField *field)
> >> +{
> >> + VirtioCcwDevice *dev = pv;
> >> + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
> >> +
> >> + qemu_get_be16s(f, &vdev->config_vector);
> >> + return 0;
> >> +}
> >> +
> >> +static int put_config_vector(QEMUFile *f, void *pv, size_t size,
> >> + VMStateField *field, QJSON *vmdesc)
> >> +{
> >> + VirtioCcwDevice *dev = pv;
> >> + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
> >> +
> >> + qemu_put_be16(f, vdev->config_vector);
> >> + return 0;
> >> +}
> > Again that should be doable using WITH_TMP.
> > (I do wonder if we need a macro for cases where it's just a casting
> > operation to another type).
> >
>
> Yeah this one can be done with WITH_TMP. Below is the patch on top of
> this patch set. It's a bit more verbose (+6 lines) but it looks a bit
> nicer and probably also safer in (terms of symmetric read and write).
>
> If you think its the way to go I will squash it into this patch for
> the next version.
Yes, I prefer that to using qemu_put/qemu_get - I'm trying to avoid
all new uses of those except where we can't avoid them.
(There's still the separate question of whether reworking a virtio
device migration to be unlike the other virtio devices is right,
but that's separate to whether we should avoid qemu_get/put)
Dave
> -----------------------------8<-----------------------------------------
>
>
> From e92135590ab95cc565b37913de77a9ed17012933 Mon Sep 17 00:00:00 2001
> From: Halil Pasic <pasic@linux.vnet.ibm.com>
> Date: Tue, 9 May 2017 16:01:50 +0200
> Subject: [PATCH 1/2] virtio-ccw: replace info with VMSTATE_WITH_TMP
>
> Convert s VMSatateInfo based solution manipulating the migration stream
> directly to VMSTATE_WITH_TMP witch keeps IO and transformation logic
> separate.
>
>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> hw/s390x/virtio-ccw.c | 40 +++++++++++++++++++++++-----------------
> 1 file changed, 23 insertions(+), 17 deletions(-)
>
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index c611b6f..6ebc78a 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -57,30 +57,38 @@ static int virtio_ccw_dev_post_load(void *opaque, int version_id)
> return 0;
> }
>
> -static int get_config_vector(QEMUFile *f, void *pv, size_t size,
> - VMStateField *field)
> +typedef struct VirtioCcwDeviceTmp {
> + VirtioCcwDevice *parent;
> + uint16_t config_vector;
> +} VirtioCcwDeviceTmp;
> +
> +static void virtio_ccw_dev_tmp_pre_save(void *opaque)
> {
> - VirtioCcwDevice *dev = pv;
> + VirtioCcwDeviceTmp *tmp = opaque;
> + VirtioCcwDevice *dev = tmp->parent;
> VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
>
> - qemu_get_be16s(f, &vdev->config_vector);
> - return 0;
> + tmp->config_vector = vdev->config_vector;
> }
>
> -static int put_config_vector(QEMUFile *f, void *pv, size_t size,
> - VMStateField *field, QJSON *vmdesc)
> +static int virtio_ccw_dev_tmp_post_load(void *opaque, int version_id)
> {
> - VirtioCcwDevice *dev = pv;
> + VirtioCcwDeviceTmp *tmp = opaque;
> + VirtioCcwDevice *dev = tmp->parent;
> VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
>
> - qemu_put_be16(f, vdev->config_vector);
> + vdev->config_vector = tmp->config_vector;
> return 0;
> }
>
> -const VMStateInfo vmstate_info_config_vector = {
> - .name = "config_vector",
> - .get = get_config_vector,
> - .put = put_config_vector,
> +const VMStateDescription vmstate_virtio_ccw_dev_tmp = {
> + .name = "s390_virtio_ccw_dev_tmp",
> + .pre_save = virtio_ccw_dev_tmp_pre_save,
> + .post_load = virtio_ccw_dev_tmp_post_load,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT16(config_vector, VirtioCcwDeviceTmp),
> + VMSTATE_END_OF_LIST()
> + }
> };
>
> const VMStateDescription vmstate_virtio_ccw_dev = {
> @@ -93,14 +101,12 @@ const VMStateDescription vmstate_virtio_ccw_dev = {
> VMSTATE_PTR_TO_IND_ADDR(indicators, VirtioCcwDevice),
> VMSTATE_PTR_TO_IND_ADDR(indicators2, VirtioCcwDevice),
> VMSTATE_PTR_TO_IND_ADDR(summary_indicator, VirtioCcwDevice),
> - {
> /*
> * Ugly hack because VirtIODevice does not migrate itself.
> * This also makes legacy via vmstate_save_state possible.
> */
> - .name = "virtio/config_vector",
> - .info = &vmstate_info_config_vector,
> - },
> + VMSTATE_WITH_TMP(VirtioCcwDevice, VirtioCcwDeviceTmp,
> + vmstate_virtio_ccw_dev_tmp),
> VMSTATE_STRUCT(routes, VirtioCcwDevice, 1, vmstate_adapter_routes, \
> AdapterRoutes),
> VMSTATE_UINT8(thinint_isc, VirtioCcwDevice),
> --
> 2.10.2
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration
2017-05-09 17:05 ` Halil Pasic
2017-05-10 10:31 ` Dr. David Alan Gilbert
@ 2017-05-10 10:38 ` Cornelia Huck
1 sibling, 0 replies; 42+ messages in thread
From: Cornelia Huck @ 2017-05-10 10:38 UTC (permalink / raw)
To: Halil Pasic; +Cc: Dr. David Alan Gilbert, qemu-devel, Michael S. Tsirkin
On Tue, 9 May 2017 19:05:28 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> From e92135590ab95cc565b37913de77a9ed17012933 Mon Sep 17 00:00:00 2001
> From: Halil Pasic <pasic@linux.vnet.ibm.com>
> Date: Tue, 9 May 2017 16:01:50 +0200
> Subject: [PATCH 1/2] virtio-ccw: replace info with VMSTATE_WITH_TMP
>
> Convert s VMSatateInfo based solution manipulating the migration stream
> directly to VMSTATE_WITH_TMP witch keeps IO and transformation logic
> separate.
>
>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> hw/s390x/virtio-ccw.c | 40 +++++++++++++++++++++++-----------------
> 1 file changed, 23 insertions(+), 17 deletions(-)
FWIW: Looks reasonable to me.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration
2017-05-08 18:42 ` Dr. David Alan Gilbert
@ 2017-05-10 11:52 ` Halil Pasic
2017-05-15 19:07 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 42+ messages in thread
From: Halil Pasic @ 2017-05-10 11:52 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Cornelia Huck, Michael S. Tsirkin, qemu-devel
On 05/08/2017 08:42 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>>
>>
>> On 05/08/2017 07:59 PM, Dr. David Alan Gilbert wrote:
>>>>>> const VMStateDescription vmstate_virtio_ccw_dev = {
>>>>>> .name = "s390_virtio_ccw_dev",
>>>>>> .version_id = 1,
>>>>>> @@ -67,6 +93,14 @@ const VMStateDescription vmstate_virtio_ccw_dev = {
>>>>>> VMSTATE_PTR_TO_IND_ADDR(indicators, VirtioCcwDevice),
>>>>>> VMSTATE_PTR_TO_IND_ADDR(indicators2, VirtioCcwDevice),
>>>>>> VMSTATE_PTR_TO_IND_ADDR(summary_indicator, VirtioCcwDevice),
>>>>>> + {
>>>>>> + /*
>>>>>> + * Ugly hack because VirtIODevice does not migrate itself.
>>>>>> + * This also makes legacy via vmstate_save_state possible.
>>>>>> + */
>>>>>> + .name = "virtio/config_vector",
>>>>>> + .info = &vmstate_info_config_vector,
>>>>>> + },
>>>>> I'm a bit confused - isn't just this bit the bit going
>>>>> through the vdev->load_config hook?
>>>>>
>>>> Exactly! If you examine the part underscored with ============== in
>>>> virtio_ccw_(load|save)_config (that is what is behind vdev->load_config)
>>>> you should see that we use vmstate_virtio_ccw_dev (that is this
>>>> VMStateDescription to implement these function.
>>>>
>>>> Actually virtio_ccw_(load|save)_config won't do anything after patch 9
>>>> and for new machines since the VirtIOCcwDevice is going to migrate
>>>> itself via DeviceClass.vmsd like other "normal" devices, and we fall
>>>> back to the VirtIO specific callbacks only in "legacy mode".
>>>>
>>>> I hope this helps!
>>> Hmm, still confused!
>>> Why are you changing a Virtio device not to use the same migration
>>> oddities as all the other virtio devices?
>>>
>>> I was assuming we'd have to change the virtio core code to
>>> solve that for all virtio devices.
>>>
>>
>> You can ask difficult questions ;). First I'm not aware of any
>> ongoing effort to solve this for all virtio devices by changing
>> the core, and probably breaking compatibility for all devices
>> (in a sense I break migration compatibility for all virtio-ccw
>> devices). To be honest, I have considered that move unlikely,
>> but the possibility is one of my reasons for seeking an upstream
>> discussion and having You and Michael on cc.
>
> Well I have been trying to improve it, but that code is particularly
> nasty; and I don't want to break compatibility. I had some ideas,
> for example I was thinking of changing the vdev->load_config to
> a VMState* and then calling a vmstate_load_state(f, vdc->config,...
> from virtio_load - but there's some challenging casting and hackery
> there.
>
>>
>> Why not use virtio oddities? Because they are oddities. I have
>> figured, it's a good idea to separate the migration of the
>> proxy form the rest: we have two QEMU Device objects and it
>> should be good practice, that these are migrating themselves via
>> DeviceClass.vmsd. That's what I get with this patch set,
>> for new machine versions (since we can not fix the past), and
>> with the notable difference of config_vector, because it is
>> defined as a common infrastructure (struct VirtIODevice) but
>> ain't migrated as a common virtio infrastructure.
>
> Have you got a bit of a description of your classes/structure - it's
> a little hard to get my head around.
>
Unfortunately I do not have any extra description besides the comments
and the commit messages. What exactly do you mean by 'my
classes/structure'? I would like to provide some helpful developer
documentation on how migration works for s390x. There were voices on the
internal mailing list too requesting something like that, but I find it
hard, because for me, the most challenging part was understanding how
qemu migration works in general and the virtio oddities come next.
Fore example, I still don't understand why is is (virtio) load_config
called like that, when what it mainly does is loading state of the proxy
which is basically the reification of the device side of the virtio spec
calls the transport within QOM. (I say mainly, because of this
config_vector which resides in core but is migrated by via a callback for
some strange reason I do not understand).
Could tell me to which (specific) questions should I provide an answer?
It would make my job much easier.
About the general approach. First step was to provide VMStateDescription
for the entities which have migration relevant state but no
VMStateDescription (patches 3, 4 and 5). This is done so that
lots of qemu_put/qem_get calls can be replaced with few
vmstate_save_state/vmstate_save_state calls (patch 6 and 7) on one hand,
and that state not migrated yet but needed is also included, if the
compat. switch (property) added in patch 2 is on. Then in patch 8, I add
ORB which is a state we wanted to add for some time now, but we needed
vmstate to add it without breaking migration. So we waited.
>> Would you suggest to rather keep the oddities? Should I expect
>> to see a generic solution to the problem sometime soon?
>
> Not soon I fear; virtio_load is just too hairy.
Of course it ain't a problem for me to omit assigning an vmsd to
VirtioCcwDeviceClass, but since I have to introduce a new section anyway
(for the css stuff) it ain't hurting me to put the state of
VirtioCcwDevice, that is the virtio proxy, into a separate section.
I can't think of any decisive benefit in favor of having separate
sections for the proxy (transport) providing a virtio bus and the generic
virtio device sitting on that bus, but I think it should be slightly more
robust. One of the reasons I included this change is to make thinking
about the question easier by clearing the questions: is it viable and
complex/ugly is it to implement.
What is your preference, keep the migration of the two devices lumped
together in one section, or rather go with two?
Halil
>
> Dave
>
>> Halil
>>
>>> Dave
>>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css
2017-05-09 12:00 ` Halil Pasic
@ 2017-05-15 18:01 ` Dr. David Alan Gilbert
2017-05-18 14:15 ` Halil Pasic
0 siblings, 1 reply; 42+ messages in thread
From: Dr. David Alan Gilbert @ 2017-05-15 18:01 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, Michael S. Tsirkin, qemu-devel
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>
>
> On 05/08/2017 06:45 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> >> As a preparation for switching to a vmstate based migration let us
> >> introduce vmstate entities (e.g. VMStateDescription) for the css entities
> >> to be migrated. Alongside some comments explaining or indicating the not
> >> migration of certain members are introduced too.
> >>
> >> No changes in behavior, we just added some dead code -- which should
> >> rise to life soon.
> >>
> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >> ---
> >> hw/s390x/css.c | 276 +++++++++++++++++++++++++++++++++++++++++++++++++
> >> include/hw/s390x/css.h | 10 +-
> >> 2 files changed, 285 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >> index c03bb20..2bda7d0 100644
> >> --- a/hw/s390x/css.c
> >> +++ b/hw/s390x/css.c
> >> @@ -20,29 +20,231 @@
> >> #include "hw/s390x/css.h"
> >> #include "trace.h"
> >> #include "hw/s390x/s390_flic.h"
> >> +#include "hw/s390x/s390-virtio-ccw.h"
> >>
>
> [..]
>
> >> +static int css_get_ind_addr(QEMUFile *f, void *pv, size_t size,
> >> + VMStateField *field)
> >> +{
> >> + int32_t len;
> >> + IndAddr **ind_addr = pv;
> >> +
> >> + len = qemu_get_be32(f);
> >> + if (len != 0) {
> >> + *ind_addr = get_indicator(qemu_get_be64(f), len);
> >> + } else {
> >> + qemu_get_be64(f);
> >> + *ind_addr = NULL;
> >> + }
> >> + return 0;
> >> +}
> >> +
> >> +static int css_put_ind_addr(QEMUFile *f, void *pv, size_t size,
> >> + VMStateField *field, QJSON *vmdesc)
> >> +{
> >> + IndAddr *ind_addr = *(IndAddr **) pv;
> >> +
> >> + if (ind_addr != NULL) {
> >> + qemu_put_be32(f, ind_addr->len);
> >> + qemu_put_be64(f, ind_addr->addr);
> >> + } else {
> >> + qemu_put_be32(f, 0);
> >> + qemu_put_be64(f, 0UL);
> >> + }
> >> + return 0;
> >> +}
> >> +
> >> +const VMStateInfo vmstate_info_ind_addr = {
> >> + .name = "s390_ind_addr",
> >> + .get = css_get_ind_addr,
> >> + .put = css_put_ind_addr
> >> +};
> >
> > You should be able to avoid this .get/.put by using VMSTATE_WITH_TMP,
> > declare a temporary struct something like:
> > struct tmp_ind_addr {
> > IndAddr *parent;
> > uint32_t len;
> > uint64_t addr;
> > }
> >
> > and then your .get/.put routines turn into pre_save/post_load
> > routines to just setup the len/addr.
> >
>
> I don't think this is going to work -- unfortunately! You can see below,
> how this IndAddr* migration stuff is supposed to be used:
> the client code just uses the VMSTATE_PTR_TO_IND_ADDR macro as a
> field when describing state which needs and IndAddr* migrated.
>
> The problem is, we do not know in what state will this field
> be embedded, the pre_save/post_load called by put_tmp/get_tmp
> is however copying the pointer to this state into the parent.
> So instead of having a pointer to IndAddr* in those functions
> and updating it accordingly, I would have to find the IndAddr*
> in some arbitrary state (in our case VirtioCcwDevice) first,
> and I lack information for that.
>
> If it's hard to follow I can give you the patch I was debugging
> to come to this conclusion. (By the way I ended up with 10
> lines of code more than in this version, and although I think
> it looks nicer, it's simpler only if one knows how WITH_TMP
> works. My plan was to ask you which version do you like more
> and go with that before I realized it ain't gonna work.)
>
Yes, I see - I've got some similar other cases; the challenge
is it's a custom allocator - 'get_indicator' - and it's used
as fields in a few places. Hmm.
Dave
> >> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> >> index f1f0d7f..6a451b2 100644
> >> --- a/include/hw/s390x/css.h
>
> [..]
>
> >>
> >> +extern const VMStateInfo vmstate_info_ind_addr;
> >> +
> >> +#define VMSTATE_PTR_TO_IND_ADDR(_f, _s) \
> >> + VMSTATE_SINGLE(_f, _s, 1 , vmstate_info_ind_addr, IndAddr*)
> >> +
> >> IndAddr *get_indicator(hwaddr ind_addr, int len);
> >> void release_indicator(AdapterInfo *adapter, IndAddr *indicator);
> >> int map_indicator(AdapterInfo *adapter, IndAddr *indicator);
> >> --
> >> 2.10.2
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
>
>
> Cheers,
> Halil
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration
2017-05-10 11:52 ` Halil Pasic
@ 2017-05-15 19:07 ` Dr. David Alan Gilbert
2017-05-16 22:05 ` Halil Pasic
0 siblings, 1 reply; 42+ messages in thread
From: Dr. David Alan Gilbert @ 2017-05-15 19:07 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, Michael S. Tsirkin, qemu-devel
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>
>
> On 05/08/2017 08:42 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> >>
> >>
> >> On 05/08/2017 07:59 PM, Dr. David Alan Gilbert wrote:
> >>>>>> const VMStateDescription vmstate_virtio_ccw_dev = {
> >>>>>> .name = "s390_virtio_ccw_dev",
> >>>>>> .version_id = 1,
> >>>>>> @@ -67,6 +93,14 @@ const VMStateDescription vmstate_virtio_ccw_dev = {
> >>>>>> VMSTATE_PTR_TO_IND_ADDR(indicators, VirtioCcwDevice),
> >>>>>> VMSTATE_PTR_TO_IND_ADDR(indicators2, VirtioCcwDevice),
> >>>>>> VMSTATE_PTR_TO_IND_ADDR(summary_indicator, VirtioCcwDevice),
> >>>>>> + {
> >>>>>> + /*
> >>>>>> + * Ugly hack because VirtIODevice does not migrate itself.
> >>>>>> + * This also makes legacy via vmstate_save_state possible.
> >>>>>> + */
> >>>>>> + .name = "virtio/config_vector",
> >>>>>> + .info = &vmstate_info_config_vector,
> >>>>>> + },
> >>>>> I'm a bit confused - isn't just this bit the bit going
> >>>>> through the vdev->load_config hook?
> >>>>>
> >>>> Exactly! If you examine the part underscored with ============== in
> >>>> virtio_ccw_(load|save)_config (that is what is behind vdev->load_config)
> >>>> you should see that we use vmstate_virtio_ccw_dev (that is this
> >>>> VMStateDescription to implement these function.
> >>>>
> >>>> Actually virtio_ccw_(load|save)_config won't do anything after patch 9
> >>>> and for new machines since the VirtIOCcwDevice is going to migrate
> >>>> itself via DeviceClass.vmsd like other "normal" devices, and we fall
> >>>> back to the VirtIO specific callbacks only in "legacy mode".
> >>>>
> >>>> I hope this helps!
> >>> Hmm, still confused!
> >>> Why are you changing a Virtio device not to use the same migration
> >>> oddities as all the other virtio devices?
> >>>
> >>> I was assuming we'd have to change the virtio core code to
> >>> solve that for all virtio devices.
> >>>
> >>
> >> You can ask difficult questions ;). First I'm not aware of any
> >> ongoing effort to solve this for all virtio devices by changing
> >> the core, and probably breaking compatibility for all devices
> >> (in a sense I break migration compatibility for all virtio-ccw
> >> devices). To be honest, I have considered that move unlikely,
> >> but the possibility is one of my reasons for seeking an upstream
> >> discussion and having You and Michael on cc.
> >
> > Well I have been trying to improve it, but that code is particularly
> > nasty; and I don't want to break compatibility. I had some ideas,
> > for example I was thinking of changing the vdev->load_config to
> > a VMState* and then calling a vmstate_load_state(f, vdc->config,...
> > from virtio_load - but there's some challenging casting and hackery
> > there.
> >
> >>
> >> Why not use virtio oddities? Because they are oddities. I have
> >> figured, it's a good idea to separate the migration of the
> >> proxy form the rest: we have two QEMU Device objects and it
> >> should be good practice, that these are migrating themselves via
> >> DeviceClass.vmsd. That's what I get with this patch set,
> >> for new machine versions (since we can not fix the past), and
> >> with the notable difference of config_vector, because it is
> >> defined as a common infrastructure (struct VirtIODevice) but
> >> ain't migrated as a common virtio infrastructure.
> >
> > Have you got a bit of a description of your classes/structure - it's
> > a little hard to get my head around.
> >
>
> Unfortunately I do not have any extra description besides the comments
> and the commit messages. What exactly do you mean by 'my
> classes/structure'? I would like to provide some helpful developer
> documentation on how migration works for s390x. There were voices on the
> internal mailing list too requesting something like that, but I find it
> hard, because for me, the most challenging part was understanding how
> qemu migration works in general and the virtio oddities come next.
Yes, there are only about 2 people who have the overlap of understanding
migration AND s390 IO.
> Fore example, I still don't understand why is is (virtio) load_config
> called like that, when what it mainly does is loading state of the proxy
> which is basically the reification of the device side of the virtio spec
> calls the transport within QOM. (I say mainly, because of this
> config_vector which resides in core but is migrated by via a callback for
> some strange reason I do not understand).
I think the idea is that virtio_load is trying to act as a generic
save/load with a number of virtual components that are specialised for:
a) The device (e.g. rng, serial, gpu, net, blk)
b) The transport (PCI, MMIO, CCW etc)
c) The virtio queue content
d) But has a load of core stuff (features, the virtio ring management)
(a) & (b) are very much virtual-function like that doesn't fit that
well with the migration macro structure.
The split between (a) & (c) isn't necessary clean - gpu does it a
different way.
And the order of a/b/c/d is very random (aka wrong).
> Could tell me to which (specific) questions should I provide an answer?
> It would make my job much easier.
>
> About the general approach. First step was to provide VMStateDescription
> for the entities which have migration relevant state but no
> VMStateDescription (patches 3, 4 and 5). This is done so that
> lots of qemu_put/qem_get calls can be replaced with few
> vmstate_save_state/vmstate_save_state calls (patch 6 and 7) on one hand,
> and that state not migrated yet but needed is also included, if the
> compat. switch (property) added in patch 2 is on. Then in patch 8, I add
> ORB which is a state we wanted to add for some time now, but we needed
> vmstate to add it without breaking migration. So we waited.
I'm most interested at this point in understanding which bits aren't
changing behaviour - if we've got stuff that's just converting qemu_get
to vmstate then lets go for it, no problem; easy to check.
I'm just trying to make sure I understand the bit where you're
converting from being a virtio device.
> >> Would you suggest to rather keep the oddities? Should I expect
> >> to see a generic solution to the problem sometime soon?
> >
> > Not soon I fear; virtio_load is just too hairy.
>
> Of course it ain't a problem for me to omit assigning an vmsd to
> VirtioCcwDeviceClass, but since I have to introduce a new section anyway
> (for the css stuff) it ain't hurting me to put the state of
> VirtioCcwDevice, that is the virtio proxy, into a separate section.
>
> I can't think of any decisive benefit in favor of having separate
> sections for the proxy (transport) providing a virtio bus and the generic
> virtio device sitting on that bus, but I think it should be slightly more
> robust. One of the reasons I included this change is to make thinking
> about the question easier by clearing the questions: is it viable and
> complex/ugly is it to implement.
>
> What is your preference, keep the migration of the two devices lumped
> together in one section, or rather go with two?
I'm not sure!
But my main worries with you changing it are:
a) What happens when something changes in virtio and they need to add
some new virtio field somewhere - if you do it different will it
make it harder.
b) If you have a virtio device which does it differently, is it going
to make cleaning up virtio_load/save even harder?
Dave
> Halil
>
> >
> > Dave
> >
> >> Halil
> >>
> >>> Dave
> >>>
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration
2017-05-15 19:07 ` Dr. David Alan Gilbert
@ 2017-05-16 22:05 ` Halil Pasic
2017-05-19 17:28 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 42+ messages in thread
From: Halil Pasic @ 2017-05-16 22:05 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Cornelia Huck, qemu-devel, Michael S. Tsirkin
On 05/15/2017 09:07 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>>
>>
>> On 05/08/2017 08:42 PM, Dr. David Alan Gilbert wrote:
>>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>>>>
>>>>
>>>> On 05/08/2017 07:59 PM, Dr. David Alan Gilbert wrote:
[..]
>>>>
>>>> Why not use virtio oddities? Because they are oddities. I have
>>>> figured, it's a good idea to separate the migration of the
>>>> proxy form the rest: we have two QEMU Device objects and it
>>>> should be good practice, that these are migrating themselves via
>>>> DeviceClass.vmsd. That's what I get with this patch set,
>>>> for new machine versions (since we can not fix the past), and
>>>> with the notable difference of config_vector, because it is
>>>> defined as a common infrastructure (struct VirtIODevice) but
>>>> ain't migrated as a common virtio infrastructure.
>>>
>>> Have you got a bit of a description of your classes/structure - it's
>>> a little hard to get my head around.
>>>
>>
>> Unfortunately I do not have any extra description besides the comments
>> and the commit messages. What exactly do you mean by 'my
>> classes/structure'? I would like to provide some helpful developer
>> documentation on how migration works for s390x. There were voices on the
>> internal mailing list too requesting something like that, but I find it
>> hard, because for me, the most challenging part was understanding how
>> qemu migration works in general and the virtio oddities come next.
>
> Yes, there are only about 2 people who have the overlap of understanding
> migration AND s390 IO.
>
>> Fore example, I still don't understand why is is (virtio) load_config
>> called like that, when what it mainly does is loading state of the proxy
>> which is basically the reification of the device side of the virtio spec
>> calls the transport within QOM. (I say mainly, because of this
>> config_vector which resides in core but is migrated by via a callback for
>> some strange reason I do not understand).
>
> I think the idea is that virtio_load is trying to act as a generic
> save/load with a number of virtual components that are specialised for:
> a) The device (e.g. rng, serial, gpu, net, blk)
> b) The transport (PCI, MMIO, CCW etc)
> c) The virtio queue content
> d) But has a load of core stuff (features, the virtio ring management)
>
> (a) & (b) are very much virtual-function like that doesn't fit that
> well with the migration macro structure.
>
> The split between (a) & (c) isn't necessary clean - gpu does it a
> different way.
> And the order of a/b/c/d is very random (aka wrong).
>
I mostly agree with your analysis. Honestly I have forgot abut this
load_queue callback (I think its c)), but it's a strange one too. What it
does is handling the vector of the queue which is again common
infrastructure in a sense that it reside within VirtIODevice, but it may
need some proxy specific handling.
In my understanding the virtio migration and the migration subsystem
(lets call it vmstate) are a misfit in the following aspect. Most
importantly it separation of concerns. In my understanding, for vmstate,
each device is supposed to load/save itself, and loading state and doing
stuff with the state we have loaded are separate concerns. I'm not sure
whats the vmstate place for code which is supposed to run as a part of
the migration logic, but requires cooperation of devices (e.g. notify in
virtio_load which basically generates an interrupt).
>> Could tell me to which (specific) questions should I provide an answer?
>> It would make my job much easier.
>>
>> About the general approach. First step was to provide VMStateDescription
>> for the entities which have migration relevant state but no
>> VMStateDescription (patches 3, 4 and 5). This is done so that
>> lots of qemu_put/qem_get calls can be replaced with few
>> vmstate_save_state/vmstate_save_state calls (patch 6 and 7) on one hand,
>> and that state not migrated yet but needed is also included, if the
>> compat. switch (property) added in patch 2 is on. Then in patch 8, I add
>> ORB which is a state we wanted to add for some time now, but we needed
>> vmstate to add it without breaking migration. So we waited.
>
> I'm most interested at this point in understanding which bits aren't
> changing behaviour - if we've got stuff that's just converting qemu_get
> to vmstate then lets go for it, no problem; easy to check.
The commit messages should be helpful. Up to patch 8 all I do is
converting qemu_get to vmstate as you said.
> I'm just trying to make sure I understand the bit where you're
> converting from being a virtio device.
>
By converting from being a virtio device you mean factoring out the
transport stuff into a separate section? That's happening in patch
9. Let me try to explain that patch.
The think of that patch is the following:
* Prior to it css_migration_enabled() always returned false. After
patch 9 it returns true for new machine versions and false for old
ones. That ensures there is still no change of behavior except
for the conversion to vmstate for old machines.
It's hunk where the change happens:
@@ -196,7 +196,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
s390mc->ri_allowed = true;
s390mc->cpu_model_allowed = true;
- s390mc->css_migration_enabled = false; /* TODO: set to true */
+ s390mc->css_migration_enabled = true;
* If css_migration_enabled() we turn virtio_load_config into noop because we
use the DeviceClass.vmsd to migrate the stuff that was migrated
in virtio_load_config (new section).
* We need to to make sure if !css_migration_enabled() we do not have
a new section (this is what we rewrote with top level .needed).
* ccw_machine_2_10_instance_options makes sure all future machine versions
are calling css_register_vmstate() if css_migration_enabled() so
we have a new subsection for the common css stuff (not device specific).
* css_migration_enabled() returning true has a side effect that some
subsections are become needed. That's effectively switching between
compat. representation and complete representation. That includes
the ORB which was added in patch 8 so that it's subsection can utilize
this very same kill switch instead of needing a new property. Otherwise
the ORB has no connection with the objective of this patch set whatsoever.
>>>> Would you suggest to rather keep the oddities? Should I expect
>>>> to see a generic solution to the problem sometime soon?
>>>
>>> Not soon I fear; virtio_load is just too hairy.
>>
>> Of course it ain't a problem for me to omit assigning an vmsd to
>> VirtioCcwDeviceClass, but since I have to introduce a new section anyway
>> (for the css stuff) it ain't hurting me to put the state of
>> VirtioCcwDevice, that is the virtio proxy, into a separate section.
>>
>> I can't think of any decisive benefit in favor of having separate
>> sections for the proxy (transport) providing a virtio bus and the generic
>> virtio device sitting on that bus, but I think it should be slightly more
>> robust. One of the reasons I included this change is to make thinking
>> about the question easier by clearing the questions: is it viable and
>> complex/ugly is it to implement.
>>
>> What is your preference, keep the migration of the two devices lumped
>> together in one section, or rather go with two?
>
> I'm not sure!
> But my main worries with you changing it are:
> a) What happens when something changes in virtio and they need to add
> some new virtio field somewhere - if you do it different will it
> make it harder.
> b) If you have a virtio device which does it differently, is it going
> to make cleaning up virtio_load/save even harder?
>
> Dave
My biggest concerns are actually -- let's assign them letters too:
c) Possible dependencies between the states of the proxy and the device:
If virtio_load should need some transport state, or the other way around,
if the proxy device should need some device state in post_load for
example we have a problem (AFAIK) because the order in which the states
of the two qdev devices are loaded not any more under the control of
virtio_load.
d) It would be an oddball among the oddballs. I have no idea how would I
change the documentation to reflect that properly.
I think c) is quite nasty. For instance we have
virtio_notify_vector(vdev, VIRTIO_NO_VECTOR) from virtio_load is called
which needs to happen after the proxy is loaded. My code works, because
the order of load is the same as the order of load, which is the same
as the order of device_set_realized calls which is proxy first
and then device (e.g. virtio-net). But it's fragile, because of some
recent changes with priorities. So should virtio get (high) prio assigned
to it this would break pretty bad!
I wonder if thinking into the direction of splitting the stuff makes
sense any more.
If we agree on not splitting up the virtio proxy and the rest into two
sections (one per device), I would like to fix that and do a re-spin.
Your opinion?
I would also like give you my opinion on your concerns, keep on reading
if you think it's still relevant:
AFAIU a) is a human problem, and I'm not that concerned about it: if it
is a generic virtio field, it should be placed into the core, and is
completely unaffected by this change (except if something breaks it may
be easier to figure out because different sections). If it is a transport
specific thing, it's likely to be done by the s390x folks anyway. So I
think @Connie should make the call on that one.
About b), I guess it depends. If things go towards separate sections for
separate qdev devices (that is, for the transport (proxy) and for the
generic specific virtio device (e.g. rng, serial, gpu, net, blk) which
inherits for VirtIODevice which is a part of what we call the virtio
core it may even simplify things. If we are going to keep the state of
the two devices in one section, then it would remain an exception, and
exceptions are ugly.
To sum it up although I'm currently leaning towards abandoning my idea
of two sections for two devices, I'm not comfortable with making the
call myself. I'm hoping for some maintainer guidance (s390x, virtio
and migration).
Many thanks!
Halil
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css
2017-05-15 18:01 ` Dr. David Alan Gilbert
@ 2017-05-18 14:15 ` Halil Pasic
2017-05-19 14:55 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 42+ messages in thread
From: Halil Pasic @ 2017-05-18 14:15 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Cornelia Huck, qemu-devel, Michael S. Tsirkin
On 05/15/2017 08:01 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>>
>>
>> On 05/08/2017 06:45 PM, Dr. David Alan Gilbert wrote:
>>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>>>> As a preparation for switching to a vmstate based migration let us
>>>> introduce vmstate entities (e.g. VMStateDescription) for the css entities
>>>> to be migrated. Alongside some comments explaining or indicating the not
>>>> migration of certain members are introduced too.
>>>>
>>>> No changes in behavior, we just added some dead code -- which should
>>>> rise to life soon.
>>>>
>>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>> ---
>>>> hw/s390x/css.c | 276 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>> include/hw/s390x/css.h | 10 +-
>>>> 2 files changed, 285 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>>> index c03bb20..2bda7d0 100644
>>>> --- a/hw/s390x/css.c
>>>> +++ b/hw/s390x/css.c
>>>> @@ -20,29 +20,231 @@
>>>> #include "hw/s390x/css.h"
>>>> #include "trace.h"
>>>> #include "hw/s390x/s390_flic.h"
>>>> +#include "hw/s390x/s390-virtio-ccw.h"
>>>>
>>
>> [..]
>>
>>>> +static int css_get_ind_addr(QEMUFile *f, void *pv, size_t size,
>>>> + VMStateField *field)
>>>> +{
>>>> + int32_t len;
>>>> + IndAddr **ind_addr = pv;
>>>> +
>>>> + len = qemu_get_be32(f);
>>>> + if (len != 0) {
>>>> + *ind_addr = get_indicator(qemu_get_be64(f), len);
>>>> + } else {
>>>> + qemu_get_be64(f);
>>>> + *ind_addr = NULL;
>>>> + }
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int css_put_ind_addr(QEMUFile *f, void *pv, size_t size,
>>>> + VMStateField *field, QJSON *vmdesc)
>>>> +{
>>>> + IndAddr *ind_addr = *(IndAddr **) pv;
>>>> +
>>>> + if (ind_addr != NULL) {
>>>> + qemu_put_be32(f, ind_addr->len);
>>>> + qemu_put_be64(f, ind_addr->addr);
>>>> + } else {
>>>> + qemu_put_be32(f, 0);
>>>> + qemu_put_be64(f, 0UL);
>>>> + }
>>>> + return 0;
>>>> +}
>>>> +
>>>> +const VMStateInfo vmstate_info_ind_addr = {
>>>> + .name = "s390_ind_addr",
>>>> + .get = css_get_ind_addr,
>>>> + .put = css_put_ind_addr
>>>> +};
>>>
>>> You should be able to avoid this .get/.put by using VMSTATE_WITH_TMP,
>>> declare a temporary struct something like:
>>> struct tmp_ind_addr {
>>> IndAddr *parent;
>>> uint32_t len;
>>> uint64_t addr;
>>> }
>>>
>>> and then your .get/.put routines turn into pre_save/post_load
>>> routines to just setup the len/addr.
>>>
>>
>> I don't think this is going to work -- unfortunately! You can see below,
>> how this IndAddr* migration stuff is supposed to be used:
>> the client code just uses the VMSTATE_PTR_TO_IND_ADDR macro as a
>> field when describing state which needs and IndAddr* migrated.
>>
>> The problem is, we do not know in what state will this field
>> be embedded, the pre_save/post_load called by put_tmp/get_tmp
>> is however copying the pointer to this state into the parent.
>> So instead of having a pointer to IndAddr* in those functions
>> and updating it accordingly, I would have to find the IndAddr*
>> in some arbitrary state (in our case VirtioCcwDevice) first,
>> and I lack information for that.
>>
>> If it's hard to follow I can give you the patch I was debugging
>> to come to this conclusion. (By the way I ended up with 10
>> lines of code more than in this version, and although I think
>> it looks nicer, it's simpler only if one knows how WITH_TMP
>> works. My plan was to ask you which version do you like more
>> and go with that before I realized it ain't gonna work.)
>>
>
> Yes, I see - I've got some similar other cases; the challenge
> is it's a custom allocator - 'get_indicator' - and it's used
> as fields in a few places. Hmm.
>
>
The problem can be worked around by wrapping the WITH_TMP into a another
vmsd and using VMSTATE_STRUCT for describing the field in question. It's
quite some boilerplate (+16 lines). Should I post the patch here?
We could also consider making WITH_TMP act as a normal field.
Working on the whole state looks like a bit like a corner case:
we have some stuff adjacent in the migration stream, and we have
to map it on multiple fields (and vice-versa). Getting the whole
state with a pointer to a certain field could work via container_of.
Btw, I would rather call it get_indicator a factory method or even a
constructor than an allocator, but I think we understand each-other
anyway.
Regards,
Halil
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css
2017-05-18 14:15 ` Halil Pasic
@ 2017-05-19 14:55 ` Dr. David Alan Gilbert
2017-05-19 15:08 ` Halil Pasic
` (2 more replies)
0 siblings, 3 replies; 42+ messages in thread
From: Dr. David Alan Gilbert @ 2017-05-19 14:55 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, qemu-devel, Michael S. Tsirkin
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>
>
> On 05/15/2017 08:01 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> >>
> >>
> >> On 05/08/2017 06:45 PM, Dr. David Alan Gilbert wrote:
> >>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> >>>> As a preparation for switching to a vmstate based migration let us
> >>>> introduce vmstate entities (e.g. VMStateDescription) for the css entities
> >>>> to be migrated. Alongside some comments explaining or indicating the not
> >>>> migration of certain members are introduced too.
> >>>>
> >>>> No changes in behavior, we just added some dead code -- which should
> >>>> rise to life soon.
> >>>>
> >>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >>>> ---
> >>>> hw/s390x/css.c | 276 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>>> include/hw/s390x/css.h | 10 +-
> >>>> 2 files changed, 285 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >>>> index c03bb20..2bda7d0 100644
> >>>> --- a/hw/s390x/css.c
> >>>> +++ b/hw/s390x/css.c
> >>>> @@ -20,29 +20,231 @@
> >>>> #include "hw/s390x/css.h"
> >>>> #include "trace.h"
> >>>> #include "hw/s390x/s390_flic.h"
> >>>> +#include "hw/s390x/s390-virtio-ccw.h"
> >>>>
> >>
> >> [..]
> >>
> >>>> +static int css_get_ind_addr(QEMUFile *f, void *pv, size_t size,
> >>>> + VMStateField *field)
> >>>> +{
> >>>> + int32_t len;
> >>>> + IndAddr **ind_addr = pv;
> >>>> +
> >>>> + len = qemu_get_be32(f);
> >>>> + if (len != 0) {
> >>>> + *ind_addr = get_indicator(qemu_get_be64(f), len);
> >>>> + } else {
> >>>> + qemu_get_be64(f);
> >>>> + *ind_addr = NULL;
> >>>> + }
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +static int css_put_ind_addr(QEMUFile *f, void *pv, size_t size,
> >>>> + VMStateField *field, QJSON *vmdesc)
> >>>> +{
> >>>> + IndAddr *ind_addr = *(IndAddr **) pv;
> >>>> +
> >>>> + if (ind_addr != NULL) {
> >>>> + qemu_put_be32(f, ind_addr->len);
> >>>> + qemu_put_be64(f, ind_addr->addr);
> >>>> + } else {
> >>>> + qemu_put_be32(f, 0);
> >>>> + qemu_put_be64(f, 0UL);
> >>>> + }
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +const VMStateInfo vmstate_info_ind_addr = {
> >>>> + .name = "s390_ind_addr",
> >>>> + .get = css_get_ind_addr,
> >>>> + .put = css_put_ind_addr
> >>>> +};
> >>>
> >>> You should be able to avoid this .get/.put by using VMSTATE_WITH_TMP,
> >>> declare a temporary struct something like:
> >>> struct tmp_ind_addr {
> >>> IndAddr *parent;
> >>> uint32_t len;
> >>> uint64_t addr;
> >>> }
> >>>
> >>> and then your .get/.put routines turn into pre_save/post_load
> >>> routines to just setup the len/addr.
> >>>
> >>
> >> I don't think this is going to work -- unfortunately! You can see below,
> >> how this IndAddr* migration stuff is supposed to be used:
> >> the client code just uses the VMSTATE_PTR_TO_IND_ADDR macro as a
> >> field when describing state which needs and IndAddr* migrated.
> >>
> >> The problem is, we do not know in what state will this field
> >> be embedded, the pre_save/post_load called by put_tmp/get_tmp
> >> is however copying the pointer to this state into the parent.
> >> So instead of having a pointer to IndAddr* in those functions
> >> and updating it accordingly, I would have to find the IndAddr*
> >> in some arbitrary state (in our case VirtioCcwDevice) first,
> >> and I lack information for that.
> >>
> >> If it's hard to follow I can give you the patch I was debugging
> >> to come to this conclusion. (By the way I ended up with 10
> >> lines of code more than in this version, and although I think
> >> it looks nicer, it's simpler only if one knows how WITH_TMP
> >> works. My plan was to ask you which version do you like more
> >> and go with that before I realized it ain't gonna work.)
> >>
> >
> > Yes, I see - I've got some similar other cases; the challenge
> > is it's a custom allocator - 'get_indicator' - and it's used
> > as fields in a few places. Hmm.
> >
> >
>
> The problem can be worked around by wrapping the WITH_TMP into a another
> vmsd and using VMSTATE_STRUCT for describing the field in question. It's
> quite some boilerplate (+16 lines). Should I post the patch here?
Yes please.
> We could also consider making WITH_TMP act as a normal field.
> Working on the whole state looks like a bit like a corner case:
> we have some stuff adjacent in the migration stream, and we have
> to map it on multiple fields (and vice-versa). Getting the whole
> state with a pointer to a certain field could work via container_of.
You do need to know which field you're working on to be able to safely
use container_of, so I'm not sure how it would work for you in this
case.
The other thought I'd had was that perhaps we could change the temporary
structure in VMSTATE_WITH_TMP to:
struct foo {
struct whatever **parent;
so now you could write to *parent in cases like these.
> Btw, I would rather call it get_indicator a factory method or even a
> constructor than an allocator, but I think we understand each-other
> anyway.
Yes; I'm not too worried about the actual name as long as it's short
and obvious.
I'd thought of 'allocator' since in most cases it's used where the
load-time code allocates memory for the object being loaded.
A constructor is normally something I think of as happening after
allocation; and a factory, hmm maybe. However, if it does the right
thing I wouldn't object to any of those names.
Dave
P.S. I'm out for about a week.
> Regards,
> Halil
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css
2017-05-19 14:55 ` Dr. David Alan Gilbert
@ 2017-05-19 15:08 ` Halil Pasic
2017-05-19 16:00 ` Halil Pasic
2017-05-19 16:33 ` Halil Pasic
2 siblings, 0 replies; 42+ messages in thread
From: Halil Pasic @ 2017-05-19 15:08 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Cornelia Huck, qemu-devel, Michael S. Tsirkin
On 05/19/2017 04:55 PM, Dr. David Alan Gilbert wrote:
> Dave
> P.S. I'm out for about a week.
Thanks for the info! Could you say something about our 'two
devices two sections vs two devices one section' dilemma
form PATCH 06/10 before leaving? I do not want to be pushy,
but I'm also eager to make progress :).
Have a good whatever it is next week!
Halil
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css
2017-05-19 14:55 ` Dr. David Alan Gilbert
2017-05-19 15:08 ` Halil Pasic
@ 2017-05-19 16:00 ` Halil Pasic
2017-05-19 17:43 ` Dr. David Alan Gilbert
2017-05-19 16:33 ` Halil Pasic
2 siblings, 1 reply; 42+ messages in thread
From: Halil Pasic @ 2017-05-19 16:00 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Cornelia Huck, qemu-devel, Michael S. Tsirkin
On 05/19/2017 04:55 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>>
>>
>> On 05/15/2017 08:01 PM, Dr. David Alan Gilbert wrote:
>>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>>>>
>>>>
>>>> On 05/08/2017 06:45 PM, Dr. David Alan Gilbert wrote:
>>>>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>>>>>> As a preparation for switching to a vmstate based migration let us
>>>>>> introduce vmstate entities (e.g. VMStateDescription) for the css entities
>>>>>> to be migrated. Alongside some comments explaining or indicating the not
>>>>>> migration of certain members are introduced too.
>>>>>>
>>>>>> No changes in behavior, we just added some dead code -- which should
>>>>>> rise to life soon.
>>>>>>
>>>>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>>>> ---
>>>>>> hw/s390x/css.c | 276 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>> include/hw/s390x/css.h | 10 +-
>>>>>> 2 files changed, 285 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>>>>> index c03bb20..2bda7d0 100644
>>>>>> --- a/hw/s390x/css.c
>>>>>> +++ b/hw/s390x/css.c
>>>>>> @@ -20,29 +20,231 @@
>>>>>> #include "hw/s390x/css.h"
>>>>>> #include "trace.h"
>>>>>> #include "hw/s390x/s390_flic.h"
>>>>>> +#include "hw/s390x/s390-virtio-ccw.h"
>>>>>>
>>>>
>>>> [..]
>>>>
>>>>>> +static int css_get_ind_addr(QEMUFile *f, void *pv, size_t size,
>>>>>> + VMStateField *field)
>>>>>> +{
>>>>>> + int32_t len;
>>>>>> + IndAddr **ind_addr = pv;
>>>>>> +
>>>>>> + len = qemu_get_be32(f);
>>>>>> + if (len != 0) {
>>>>>> + *ind_addr = get_indicator(qemu_get_be64(f), len);
>>>>>> + } else {
>>>>>> + qemu_get_be64(f);
>>>>>> + *ind_addr = NULL;
>>>>>> + }
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int css_put_ind_addr(QEMUFile *f, void *pv, size_t size,
>>>>>> + VMStateField *field, QJSON *vmdesc)
>>>>>> +{
>>>>>> + IndAddr *ind_addr = *(IndAddr **) pv;
>>>>>> +
>>>>>> + if (ind_addr != NULL) {
>>>>>> + qemu_put_be32(f, ind_addr->len);
>>>>>> + qemu_put_be64(f, ind_addr->addr);
>>>>>> + } else {
>>>>>> + qemu_put_be32(f, 0);
>>>>>> + qemu_put_be64(f, 0UL);
>>>>>> + }
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +const VMStateInfo vmstate_info_ind_addr = {
>>>>>> + .name = "s390_ind_addr",
>>>>>> + .get = css_get_ind_addr,
>>>>>> + .put = css_put_ind_addr
>>>>>> +};
>>>>>
>>>>> You should be able to avoid this .get/.put by using VMSTATE_WITH_TMP,
>>>>> declare a temporary struct something like:
>>>>> struct tmp_ind_addr {
>>>>> IndAddr *parent;
>>>>> uint32_t len;
>>>>> uint64_t addr;
>>>>> }
>>>>>
>>>>> and then your .get/.put routines turn into pre_save/post_load
>>>>> routines to just setup the len/addr.
>>>>>
>>>>
>>>> I don't think this is going to work -- unfortunately! You can see below,
>>>> how this IndAddr* migration stuff is supposed to be used:
>>>> the client code just uses the VMSTATE_PTR_TO_IND_ADDR macro as a
>>>> field when describing state which needs and IndAddr* migrated.
>>>>
>>>> The problem is, we do not know in what state will this field
>>>> be embedded, the pre_save/post_load called by put_tmp/get_tmp
>>>> is however copying the pointer to this state into the parent.
>>>> So instead of having a pointer to IndAddr* in those functions
>>>> and updating it accordingly, I would have to find the IndAddr*
>>>> in some arbitrary state (in our case VirtioCcwDevice) first,
>>>> and I lack information for that.
>>>>
>>>> If it's hard to follow I can give you the patch I was debugging
>>>> to come to this conclusion. (By the way I ended up with 10
>>>> lines of code more than in this version, and although I think
>>>> it looks nicer, it's simpler only if one knows how WITH_TMP
>>>> works. My plan was to ask you which version do you like more
>>>> and go with that before I realized it ain't gonna work.)
>>>>
>>>
>>> Yes, I see - I've got some similar other cases; the challenge
>>> is it's a custom allocator - 'get_indicator' - and it's used
>>> as fields in a few places. Hmm.
>>>
>>>
>>
>> The problem can be worked around by wrapping the WITH_TMP into a another
>> vmsd and using VMSTATE_STRUCT for describing the field in question. It's
>> quite some boilerplate (+16 lines). Should I post the patch here?
>
> Yes please.
>
--------------------------------8<------------------------------------------
>From a0b6c725b114745c8434f683133995c4e33d936e Mon Sep 17 00:00:00 2001
From: Halil Pasic <pasic@linux.vnet.ibm.com>
Date: Tue, 9 May 2017 12:06:42 +0200
Subject: [PATCH 1/1] s390x/css: replace info with VMSTATE_WITH_TMP
Convert s VMSatateInfo based solution manipulating the migration stream
directly to VMSTATE_WITH_TMP witch keeps IO and transformation logic
separate.
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
hw/s390x/css.c | 56 ++++++++++++++++++++++++++++++++------------------
include/hw/s390x/css.h | 4 ++--
2 files changed, 38 insertions(+), 22 deletions(-)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 694365b395..71d1b95e3f 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -224,41 +224,57 @@ const VMStateDescription vmstate_subch_dev = {
}
};
-static int css_get_ind_addr(QEMUFile *f, void *pv, size_t size,
- VMStateField *field)
-{
+typedef struct IndAddrPtrTmp {
+ IndAddr **parent;
+ uint64_t addr;
int32_t len;
- IndAddr **ind_addr = pv;
+} IndAddrPtrTmp;
- len = qemu_get_be32(f);
- if (len != 0) {
- *ind_addr = get_indicator(qemu_get_be64(f), len);
+static int post_load_ind_addr(void *opaque, int version_id)
+{
+ IndAddrPtrTmp *ptmp = opaque;
+ IndAddr **ind_addr = ptmp->parent;
+
+ if (ptmp->len != 0) {
+ *ind_addr = get_indicator(ptmp->addr, ptmp->len);
} else {
- qemu_get_be64(f);
*ind_addr = NULL;
}
return 0;
}
-static int css_put_ind_addr(QEMUFile *f, void *pv, size_t size,
- VMStateField *field, QJSON *vmdesc)
+static void pre_save_ind_addr(void *opaque)
{
- IndAddr *ind_addr = *(IndAddr **) pv;
+ IndAddrPtrTmp *ptmp = opaque;
+ IndAddr *ind_addr = *(ptmp->parent);
if (ind_addr != NULL) {
- qemu_put_be32(f, ind_addr->len);
- qemu_put_be64(f, ind_addr->addr);
+ ptmp->len = ind_addr->len;
+ ptmp->addr = ind_addr->addr;
} else {
- qemu_put_be32(f, 0);
- qemu_put_be64(f, 0UL);
+ ptmp->len = 0;
+ ptmp->addr = 0L;
}
- return 0;
}
-const VMStateInfo vmstate_info_ind_addr = {
- .name = "s390_ind_addr",
- .get = css_get_ind_addr,
- .put = css_put_ind_addr
+const VMStateDescription vmstate_ind_addr_tmp = {
+ .name = "s390_ind_addr_tmp",
+ .pre_save = pre_save_ind_addr,
+ .post_load = post_load_ind_addr,
+
+ .fields = (VMStateField[]) {
+ VMSTATE_INT32(len, IndAddrPtrTmp),
+ VMSTATE_UINT64(addr, IndAddrPtrTmp),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+const VMStateDescription vmstate_ind_addr = {
+ .name = "s390_ind_addr_tmp",
+ .fields = (VMStateField[]) {
+ VMSTATE_WITH_TMP(IndAddr*, IndAddrPtrTmp, vmstate_ind_addr_tmp),
+ VMSTATE_END_OF_LIST()
+ }
};
typedef struct CssImage {
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index be5eb81ece..efe7cafef0 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -106,10 +106,10 @@ typedef struct IndAddr {
QTAILQ_ENTRY(IndAddr) sibling;
} IndAddr;
-extern const VMStateInfo vmstate_info_ind_addr;
+extern const VMStateDescription vmstate_ind_addr;
#define VMSTATE_PTR_TO_IND_ADDR(_f, _s) \
- VMSTATE_SINGLE(_f, _s, 1 , vmstate_info_ind_addr, IndAddr*)
+ VMSTATE_STRUCT(_f, _s, 1, vmstate_ind_addr, IndAddr*)
IndAddr *get_indicator(hwaddr ind_addr, int len);
void release_indicator(AdapterInfo *adapter, IndAddr *indicator);
--
2.11.2
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css
2017-05-19 14:55 ` Dr. David Alan Gilbert
2017-05-19 15:08 ` Halil Pasic
2017-05-19 16:00 ` Halil Pasic
@ 2017-05-19 16:33 ` Halil Pasic
2017-05-19 17:47 ` Dr. David Alan Gilbert
2 siblings, 1 reply; 42+ messages in thread
From: Halil Pasic @ 2017-05-19 16:33 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Cornelia Huck, qemu-devel, Michael S. Tsirkin
On 05/19/2017 04:55 PM, Dr. David Alan Gilbert wrote:
>> We could also consider making WITH_TMP act as a normal field.
>> Working on the whole state looks like a bit like a corner case:
>> we have some stuff adjacent in the migration stream, and we have
>> to map it on multiple fields (and vice-versa). Getting the whole
>> state with a pointer to a certain field could work via container_of.
> You do need to know which field you're working on to be able to safely
> use container_of, so I'm not sure how it would work for you in this
> case.
Well, if you have to write to just one field you are good because you
already have a pointer to that field (.offset was added).
If you need to write to multiple fields in post_load then you just pick
one of the fields you are going to write to (probably the first) and use
container_of to gain access to the whole state. The logic is specific to
the bunch of the fields you are going to touch anyway.
In fact any member of the state struct will do it's only important that
you use the same when creating the VMStateField and when trying to get a
pointer to the parent in pre_save and post_load.
I haven't tried, so I'm not 100% sure, but if you like I can try, and send
you a patch if it's viable.
I think the key to a good solution is really what is intended and typical
usage, and what is corner case. My patch in the other reply shows that we
can do without changing the ways of VMSTATE_WITH_TMP. I think we can make
what I'm trying to do here a bit prettier at the expense of making what
you are doing in virtio-net a bit uglier, but whether it's a good idea to
do so, I cant tell.
>
> The other thought I'd had was that perhaps we could change the temporary
> structure in VMSTATE_WITH_TMP to:
>
> struct foo {
> struct whatever **parent;
>
> so now you could write to *parent in cases like these.
>
Sorry, I do not get your idea. If you have some WIP patch in this
direction I would be happy to provide some feedback.
>> Btw, I would rather call it get_indicator a factory method or even a
>> constructor than an allocator, but I think we understand each-other
>> anyway.
> Yes; I'm not too worried about the actual name as long as it's short
> and obvious.
>
> I'd thought of 'allocator' since in most cases it's used where the
> load-time code allocates memory for the object being loaded.
> A constructor is normally something I think of as happening after
> allocation; and a factory, hmm maybe. However, if it does the right
> thing I wouldn't object to any of those names.
>
I think we are on the same page.
Cheers,
Halil
> Dave
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration
2017-05-16 22:05 ` Halil Pasic
@ 2017-05-19 17:28 ` Dr. David Alan Gilbert
2017-05-19 18:02 ` Halil Pasic
0 siblings, 1 reply; 42+ messages in thread
From: Dr. David Alan Gilbert @ 2017-05-19 17:28 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, qemu-devel, Michael S. Tsirkin, dhildenb
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>
>
> On 05/15/2017 09:07 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> >>
> >>
> >> On 05/08/2017 08:42 PM, Dr. David Alan Gilbert wrote:
> >>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> >>>>
> >>>>
> >>>> On 05/08/2017 07:59 PM, Dr. David Alan Gilbert wrote:
> [..]
> >>>>
> >>>> Why not use virtio oddities? Because they are oddities. I have
> >>>> figured, it's a good idea to separate the migration of the
> >>>> proxy form the rest: we have two QEMU Device objects and it
> >>>> should be good practice, that these are migrating themselves via
> >>>> DeviceClass.vmsd. That's what I get with this patch set,
> >>>> for new machine versions (since we can not fix the past), and
> >>>> with the notable difference of config_vector, because it is
> >>>> defined as a common infrastructure (struct VirtIODevice) but
> >>>> ain't migrated as a common virtio infrastructure.
> >>>
> >>> Have you got a bit of a description of your classes/structure - it's
> >>> a little hard to get my head around.
> >>>
> >>
> >> Unfortunately I do not have any extra description besides the comments
> >> and the commit messages. What exactly do you mean by 'my
> >> classes/structure'? I would like to provide some helpful developer
> >> documentation on how migration works for s390x. There were voices on the
> >> internal mailing list too requesting something like that, but I find it
> >> hard, because for me, the most challenging part was understanding how
> >> qemu migration works in general and the virtio oddities come next.
> >
> > Yes, there are only about 2 people who have the overlap of understanding
> > migration AND s390 IO.
> >
> >> Fore example, I still don't understand why is is (virtio) load_config
> >> called like that, when what it mainly does is loading state of the proxy
> >> which is basically the reification of the device side of the virtio spec
> >> calls the transport within QOM. (I say mainly, because of this
> >> config_vector which resides in core but is migrated by via a callback for
> >> some strange reason I do not understand).
> >
> > I think the idea is that virtio_load is trying to act as a generic
> > save/load with a number of virtual components that are specialised for:
> > a) The device (e.g. rng, serial, gpu, net, blk)
> > b) The transport (PCI, MMIO, CCW etc)
> > c) The virtio queue content
> > d) But has a load of core stuff (features, the virtio ring management)
> >
> > (a) & (b) are very much virtual-function like that doesn't fit that
> > well with the migration macro structure.
> >
> > The split between (a) & (c) isn't necessary clean - gpu does it a
> > different way.
> > And the order of a/b/c/d is very random (aka wrong).
> >
>
> I mostly agree with your analysis. Honestly I have forgot abut this
> load_queue callback (I think its c)), but it's a strange one too. What it
> does is handling the vector of the queue which is again common
> infrastructure in a sense that it reside within VirtIODevice, but it may
> need some proxy specific handling.
>
> In my understanding the virtio migration and the migration subsystem
> (lets call it vmstate) are a misfit in the following aspect. Most
> importantly it separation of concerns. In my understanding, for vmstate,
> each device is supposed to load/save itself, and loading state and doing
> stuff with the state we have loaded are separate concerns. I'm not sure
> whats the vmstate place for code which is supposed to run as a part of
> the migration logic, but requires cooperation of devices (e.g. notify in
> virtio_load which basically generates an interrupt).
>
>
> >> Could tell me to which (specific) questions should I provide an answer?
> >> It would make my job much easier.
> >>
> >> About the general approach. First step was to provide VMStateDescription
> >> for the entities which have migration relevant state but no
> >> VMStateDescription (patches 3, 4 and 5). This is done so that
> >> lots of qemu_put/qem_get calls can be replaced with few
> >> vmstate_save_state/vmstate_save_state calls (patch 6 and 7) on one hand,
> >> and that state not migrated yet but needed is also included, if the
> >> compat. switch (property) added in patch 2 is on. Then in patch 8, I add
> >> ORB which is a state we wanted to add for some time now, but we needed
> >> vmstate to add it without breaking migration. So we waited.
> >
> > I'm most interested at this point in understanding which bits aren't
> > changing behaviour - if we've got stuff that's just converting qemu_get
> > to vmstate then lets go for it, no problem; easy to check.
>
> The commit messages should be helpful. Up to patch 8 all I do is
> converting qemu_get to vmstate as you said.
>
> > I'm just trying to make sure I understand the bit where you're
> > converting from being a virtio device.
> >
>
> By converting from being a virtio device you mean factoring out the
> transport stuff into a separate section? That's happening in patch
> 9. Let me try to explain that patch.
>
> The think of that patch is the following:
> * Prior to it css_migration_enabled() always returned false. After
> patch 9 it returns true for new machine versions and false for old
> ones. That ensures there is still no change of behavior except
> for the conversion to vmstate for old machines.
>
> It's hunk where the change happens:
> @@ -196,7 +196,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
>
> s390mc->ri_allowed = true;
> s390mc->cpu_model_allowed = true;
> - s390mc->css_migration_enabled = false; /* TODO: set to true */
> + s390mc->css_migration_enabled = true;
>
> * If css_migration_enabled() we turn virtio_load_config into noop because we
> use the DeviceClass.vmsd to migrate the stuff that was migrated
> in virtio_load_config (new section).
> * We need to to make sure if !css_migration_enabled() we do not have
> a new section (this is what we rewrote with top level .needed).
> * ccw_machine_2_10_instance_options makes sure all future machine versions
> are calling css_register_vmstate() if css_migration_enabled() so
> we have a new subsection for the common css stuff (not device specific).
> * css_migration_enabled() returning true has a side effect that some
> subsections are become needed. That's effectively switching between
> compat. representation and complete representation. That includes
> the ORB which was added in patch 8 so that it's subsection can utilize
> this very same kill switch instead of needing a new property. Otherwise
> the ORB has no connection with the objective of this patch set whatsoever.
>
>
> >>>> Would you suggest to rather keep the oddities? Should I expect
> >>>> to see a generic solution to the problem sometime soon?
> >>>
> >>> Not soon I fear; virtio_load is just too hairy.
> >>
> >> Of course it ain't a problem for me to omit assigning an vmsd to
> >> VirtioCcwDeviceClass, but since I have to introduce a new section anyway
> >> (for the css stuff) it ain't hurting me to put the state of
> >> VirtioCcwDevice, that is the virtio proxy, into a separate section.
> >>
> >> I can't think of any decisive benefit in favor of having separate
> >> sections for the proxy (transport) providing a virtio bus and the generic
> >> virtio device sitting on that bus, but I think it should be slightly more
> >> robust. One of the reasons I included this change is to make thinking
> >> about the question easier by clearing the questions: is it viable and
> >> complex/ugly is it to implement.
> >>
> >> What is your preference, keep the migration of the two devices lumped
> >> together in one section, or rather go with two?
> >
> > I'm not sure!
> > But my main worries with you changing it are:
> > a) What happens when something changes in virtio and they need to add
> > some new virtio field somewhere - if you do it different will it
> > make it harder.
> > b) If you have a virtio device which does it differently, is it going
> > to make cleaning up virtio_load/save even harder?
> >
> > Dave
>
> My biggest concerns are actually -- let's assign them letters too:
> c) Possible dependencies between the states of the proxy and the device:
> If virtio_load should need some transport state, or the other way around,
> if the proxy device should need some device state in post_load for
> example we have a problem (AFAIK) because the order in which the states
> of the two qdev devices are loaded not any more under the control of
> virtio_load.
> d) It would be an oddball among the oddballs. I have no idea how would I
> change the documentation to reflect that properly.
>
> I think c) is quite nasty. For instance we have
> virtio_notify_vector(vdev, VIRTIO_NO_VECTOR) from virtio_load is called
> which needs to happen after the proxy is loaded. My code works, because
> the order of load is the same as the order of load, which is the same
> as the order of device_set_realized calls which is proxy first
> and then device (e.g. virtio-net). But it's fragile, because of some
> recent changes with priorities. So should virtio get (high) prio assigned
> to it this would break pretty bad!
>
> I wonder if thinking into the direction of splitting the stuff makes
> sense any more.
>
> If we agree on not splitting up the virtio proxy and the rest into two
> sections (one per device), I would like to fix that and do a re-spin.
> Your opinion?
>
> I would also like give you my opinion on your concerns, keep on reading
> if you think it's still relevant:
>
> AFAIU a) is a human problem, and I'm not that concerned about it: if it
> is a generic virtio field, it should be placed into the core, and is
> completely unaffected by this change (except if something breaks it may
> be easier to figure out because different sections). If it is a transport
> specific thing, it's likely to be done by the s390x folks anyway. So I
> think @Connie should make the call on that one.
>
> About b), I guess it depends. If things go towards separate sections for
> separate qdev devices (that is, for the transport (proxy) and for the
> generic specific virtio device (e.g. rng, serial, gpu, net, blk) which
> inherits for VirtIODevice which is a part of what we call the virtio
> core it may even simplify things. If we are going to keep the state of
> the two devices in one section, then it would remain an exception, and
> exceptions are ugly.
>
> To sum it up although I'm currently leaning towards abandoning my idea
> of two sections for two devices, I'm not comfortable with making the
> call myself. I'm hoping for some maintainer guidance (s390x, virtio
> and migration).
<A couple of hours of reading, the CCW and PCI code, chatting with
dhildenb etc>
OK, so I think:
a) First split the series into two separate series; one that
VMStatifies the existing stuff without breaking compatibility;
and one that adds the new stuff. Lets get the first of those
going in while we think about the second.
a.1) I'd do this with patches that convert one chunk into
vmstate and remove the corresponding C code at the same time.
b) While the way PCI devices are done is weird, I think it'll
be a lot simpler if you can stick to a structure that's similar
to them while diverging. It's hard enough for those of us
who don't do Virtio every day to get our minds around virtio-pci
without it being different for virtio-ccw/css.
c) To do (b) I suggest:
c.1) you *don't* add a vmsd to your virtio_ccw_device
c.2) but *do* add a VMSTATE_CCW_DEVICE to the start of any
non-virtio devices you migrate (like each of the PCI devices
have)
c.3) You can add extra state for CSS to the ->save_extra_state
handle on virtio devices or to the config
d) vmstatifying the config is OK as well.
I should say I'm no virtio expert, so if any of that's truly
mad say so.
Dave
> Many thanks!
>
> Halil
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css
2017-05-19 16:00 ` Halil Pasic
@ 2017-05-19 17:43 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 42+ messages in thread
From: Dr. David Alan Gilbert @ 2017-05-19 17:43 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, qemu-devel, Michael S. Tsirkin
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>
>
> On 05/19/2017 04:55 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> >>
> >>
> >> On 05/15/2017 08:01 PM, Dr. David Alan Gilbert wrote:
> >>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> >>>>
> >>>>
> >>>> On 05/08/2017 06:45 PM, Dr. David Alan Gilbert wrote:
> >>>>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> >>>>>> As a preparation for switching to a vmstate based migration let us
> >>>>>> introduce vmstate entities (e.g. VMStateDescription) for the css entities
> >>>>>> to be migrated. Alongside some comments explaining or indicating the not
> >>>>>> migration of certain members are introduced too.
> >>>>>>
> >>>>>> No changes in behavior, we just added some dead code -- which should
> >>>>>> rise to life soon.
> >>>>>>
> >>>>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >>>>>> ---
> >>>>>> hw/s390x/css.c | 276 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>> include/hw/s390x/css.h | 10 +-
> >>>>>> 2 files changed, 285 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >>>>>> index c03bb20..2bda7d0 100644
> >>>>>> --- a/hw/s390x/css.c
> >>>>>> +++ b/hw/s390x/css.c
> >>>>>> @@ -20,29 +20,231 @@
> >>>>>> #include "hw/s390x/css.h"
> >>>>>> #include "trace.h"
> >>>>>> #include "hw/s390x/s390_flic.h"
> >>>>>> +#include "hw/s390x/s390-virtio-ccw.h"
> >>>>>>
> >>>>
> >>>> [..]
> >>>>
> >>>>>> +static int css_get_ind_addr(QEMUFile *f, void *pv, size_t size,
> >>>>>> + VMStateField *field)
> >>>>>> +{
> >>>>>> + int32_t len;
> >>>>>> + IndAddr **ind_addr = pv;
> >>>>>> +
> >>>>>> + len = qemu_get_be32(f);
> >>>>>> + if (len != 0) {
> >>>>>> + *ind_addr = get_indicator(qemu_get_be64(f), len);
> >>>>>> + } else {
> >>>>>> + qemu_get_be64(f);
> >>>>>> + *ind_addr = NULL;
> >>>>>> + }
> >>>>>> + return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int css_put_ind_addr(QEMUFile *f, void *pv, size_t size,
> >>>>>> + VMStateField *field, QJSON *vmdesc)
> >>>>>> +{
> >>>>>> + IndAddr *ind_addr = *(IndAddr **) pv;
> >>>>>> +
> >>>>>> + if (ind_addr != NULL) {
> >>>>>> + qemu_put_be32(f, ind_addr->len);
> >>>>>> + qemu_put_be64(f, ind_addr->addr);
> >>>>>> + } else {
> >>>>>> + qemu_put_be32(f, 0);
> >>>>>> + qemu_put_be64(f, 0UL);
> >>>>>> + }
> >>>>>> + return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> +const VMStateInfo vmstate_info_ind_addr = {
> >>>>>> + .name = "s390_ind_addr",
> >>>>>> + .get = css_get_ind_addr,
> >>>>>> + .put = css_put_ind_addr
> >>>>>> +};
> >>>>>
> >>>>> You should be able to avoid this .get/.put by using VMSTATE_WITH_TMP,
> >>>>> declare a temporary struct something like:
> >>>>> struct tmp_ind_addr {
> >>>>> IndAddr *parent;
> >>>>> uint32_t len;
> >>>>> uint64_t addr;
> >>>>> }
> >>>>>
> >>>>> and then your .get/.put routines turn into pre_save/post_load
> >>>>> routines to just setup the len/addr.
> >>>>>
> >>>>
> >>>> I don't think this is going to work -- unfortunately! You can see below,
> >>>> how this IndAddr* migration stuff is supposed to be used:
> >>>> the client code just uses the VMSTATE_PTR_TO_IND_ADDR macro as a
> >>>> field when describing state which needs and IndAddr* migrated.
> >>>>
> >>>> The problem is, we do not know in what state will this field
> >>>> be embedded, the pre_save/post_load called by put_tmp/get_tmp
> >>>> is however copying the pointer to this state into the parent.
> >>>> So instead of having a pointer to IndAddr* in those functions
> >>>> and updating it accordingly, I would have to find the IndAddr*
> >>>> in some arbitrary state (in our case VirtioCcwDevice) first,
> >>>> and I lack information for that.
> >>>>
> >>>> If it's hard to follow I can give you the patch I was debugging
> >>>> to come to this conclusion. (By the way I ended up with 10
> >>>> lines of code more than in this version, and although I think
> >>>> it looks nicer, it's simpler only if one knows how WITH_TMP
> >>>> works. My plan was to ask you which version do you like more
> >>>> and go with that before I realized it ain't gonna work.)
> >>>>
> >>>
> >>> Yes, I see - I've got some similar other cases; the challenge
> >>> is it's a custom allocator - 'get_indicator' - and it's used
> >>> as fields in a few places. Hmm.
> >>>
> >>>
> >>
> >> The problem can be worked around by wrapping the WITH_TMP into a another
> >> vmsd and using VMSTATE_STRUCT for describing the field in question. It's
> >> quite some boilerplate (+16 lines). Should I post the patch here?
> >
> > Yes please.
> >
> --------------------------------8<------------------------------------------
>
> From a0b6c725b114745c8434f683133995c4e33d936e Mon Sep 17 00:00:00 2001
> From: Halil Pasic <pasic@linux.vnet.ibm.com>
> Date: Tue, 9 May 2017 12:06:42 +0200
> Subject: [PATCH 1/1] s390x/css: replace info with VMSTATE_WITH_TMP
>
> Convert s VMSatateInfo based solution manipulating the migration stream
> directly to VMSTATE_WITH_TMP witch keeps IO and transformation logic
> separate.
>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> hw/s390x/css.c | 56 ++++++++++++++++++++++++++++++++------------------
> include/hw/s390x/css.h | 4 ++--
> 2 files changed, 38 insertions(+), 22 deletions(-)
>
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 694365b395..71d1b95e3f 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -224,41 +224,57 @@ const VMStateDescription vmstate_subch_dev = {
> }
> };
>
> -static int css_get_ind_addr(QEMUFile *f, void *pv, size_t size,
> - VMStateField *field)
> -{
> +typedef struct IndAddrPtrTmp {
> + IndAddr **parent;
> + uint64_t addr;
> int32_t len;
> - IndAddr **ind_addr = pv;
> +} IndAddrPtrTmp;
>
> - len = qemu_get_be32(f);
> - if (len != 0) {
> - *ind_addr = get_indicator(qemu_get_be64(f), len);
> +static int post_load_ind_addr(void *opaque, int version_id)
> +{
> + IndAddrPtrTmp *ptmp = opaque;
> + IndAddr **ind_addr = ptmp->parent;
> +
> + if (ptmp->len != 0) {
> + *ind_addr = get_indicator(ptmp->addr, ptmp->len);
> } else {
> - qemu_get_be64(f);
> *ind_addr = NULL;
> }
> return 0;
> }
>
> -static int css_put_ind_addr(QEMUFile *f, void *pv, size_t size,
> - VMStateField *field, QJSON *vmdesc)
> +static void pre_save_ind_addr(void *opaque)
> {
> - IndAddr *ind_addr = *(IndAddr **) pv;
> + IndAddrPtrTmp *ptmp = opaque;
> + IndAddr *ind_addr = *(ptmp->parent);
>
> if (ind_addr != NULL) {
> - qemu_put_be32(f, ind_addr->len);
> - qemu_put_be64(f, ind_addr->addr);
> + ptmp->len = ind_addr->len;
> + ptmp->addr = ind_addr->addr;
> } else {
> - qemu_put_be32(f, 0);
> - qemu_put_be64(f, 0UL);
> + ptmp->len = 0;
> + ptmp->addr = 0L;
> }
> - return 0;
> }
>
> -const VMStateInfo vmstate_info_ind_addr = {
> - .name = "s390_ind_addr",
> - .get = css_get_ind_addr,
> - .put = css_put_ind_addr
> +const VMStateDescription vmstate_ind_addr_tmp = {
> + .name = "s390_ind_addr_tmp",
> + .pre_save = pre_save_ind_addr,
> + .post_load = post_load_ind_addr,
> +
> + .fields = (VMStateField[]) {
> + VMSTATE_INT32(len, IndAddrPtrTmp),
> + VMSTATE_UINT64(addr, IndAddrPtrTmp),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +const VMStateDescription vmstate_ind_addr = {
> + .name = "s390_ind_addr_tmp",
> + .fields = (VMStateField[]) {
> + VMSTATE_WITH_TMP(IndAddr*, IndAddrPtrTmp, vmstate_ind_addr_tmp),
> + VMSTATE_END_OF_LIST()
> + }
> };
>
> typedef struct CssImage {
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index be5eb81ece..efe7cafef0 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -106,10 +106,10 @@ typedef struct IndAddr {
> QTAILQ_ENTRY(IndAddr) sibling;
> } IndAddr;
>
> -extern const VMStateInfo vmstate_info_ind_addr;
> +extern const VMStateDescription vmstate_ind_addr;
>
> #define VMSTATE_PTR_TO_IND_ADDR(_f, _s) \
> - VMSTATE_SINGLE(_f, _s, 1 , vmstate_info_ind_addr, IndAddr*)
> + VMSTATE_STRUCT(_f, _s, 1, vmstate_ind_addr, IndAddr*)
>
> IndAddr *get_indicator(hwaddr ind_addr, int len);
> void release_indicator(AdapterInfo *adapter, IndAddr *indicator);
Oh if that works that's great; I'd thought we'd need to change
something in VMSTATE_WITH_TMP to be able to get it to be an IndAddr **parent;
there is a bit much boiler-plate but I've not got any easy
ideas for removing that for now.
Dave
> --
> 2.11.2
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css
2017-05-19 16:33 ` Halil Pasic
@ 2017-05-19 17:47 ` Dr. David Alan Gilbert
2017-05-19 18:04 ` Halil Pasic
0 siblings, 1 reply; 42+ messages in thread
From: Dr. David Alan Gilbert @ 2017-05-19 17:47 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, qemu-devel, Michael S. Tsirkin
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>
>
> On 05/19/2017 04:55 PM, Dr. David Alan Gilbert wrote:
> >> We could also consider making WITH_TMP act as a normal field.
> >> Working on the whole state looks like a bit like a corner case:
> >> we have some stuff adjacent in the migration stream, and we have
> >> to map it on multiple fields (and vice-versa). Getting the whole
> >> state with a pointer to a certain field could work via container_of.
> > You do need to know which field you're working on to be able to safely
> > use container_of, so I'm not sure how it would work for you in this
> > case.
>
>
> Well, if you have to write to just one field you are good because you
> already have a pointer to that field (.offset was added).
>
> If you need to write to multiple fields in post_load then you just pick
> one of the fields you are going to write to (probably the first) and use
> container_of to gain access to the whole state. The logic is specific to
> the bunch of the fields you are going to touch anyway.
>
> In fact any member of the state struct will do it's only important that
> you use the same when creating the VMStateField and when trying to get a
> pointer to the parent in pre_save and post_load.
>
> I haven't tried, so I'm not 100% sure, but if you like I can try, and send
> you a patch if it's viable.
>
> I think the key to a good solution is really what is intended and typical
> usage, and what is corner case. My patch in the other reply shows that we
> can do without changing the ways of VMSTATE_WITH_TMP. I think we can make
> what I'm trying to do here a bit prettier at the expense of making what
> you are doing in virtio-net a bit uglier, but whether it's a good idea to
> do so, I cant tell.
Lets go with what you put in the other patch (I replied to it); I hadn't
realised that was possible (hence my comment below).
Once we have a bunch of different uses of VMSTATE_WITH_TMP in the code
base, I'll step back and see how to tidy them up.
Dave
> >
> > The other thought I'd had was that perhaps we could change the temporary
> > structure in VMSTATE_WITH_TMP to:
> >
> > struct foo {
> > struct whatever **parent;
> >
> > so now you could write to *parent in cases like these.
> >
>
> Sorry, I do not get your idea. If you have some WIP patch in this
> direction I would be happy to provide some feedback.
>
>
> >> Btw, I would rather call it get_indicator a factory method or even a
> >> constructor than an allocator, but I think we understand each-other
> >> anyway.
> > Yes; I'm not too worried about the actual name as long as it's short
> > and obvious.
> >
> > I'd thought of 'allocator' since in most cases it's used where the
> > load-time code allocates memory for the object being loaded.
> > A constructor is normally something I think of as happening after
> > allocation; and a factory, hmm maybe. However, if it does the right
> > thing I wouldn't object to any of those names.
> >
>
> I think we are on the same page.
>
> Cheers,
> Halil
>
> > Dave
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration
2017-05-19 17:28 ` Dr. David Alan Gilbert
@ 2017-05-19 18:02 ` Halil Pasic
2017-05-19 18:38 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 42+ messages in thread
From: Halil Pasic @ 2017-05-19 18:02 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Cornelia Huck, qemu-devel, Michael S. Tsirkin, dhildenb
On 05/19/2017 07:28 PM, Dr. David Alan Gilbert wrote:
>> To sum it up although I'm currently leaning towards abandoning my idea
>> of two sections for two devices, I'm not comfortable with making the
>> call myself. I'm hoping for some maintainer guidance (s390x, virtio
>> and migration).
> <A couple of hours of reading, the CCW and PCI code, chatting with
> dhildenb etc>
>
> OK, so I think:
> a) First split the series into two separate series; one that
> VMStatifies the existing stuff without breaking compatibility;
> and one that adds the new stuff. Lets get the first of those
> going in while we think about the second.
>
> a.1) I'd do this with patches that convert one chunk into
> vmstate and remove the corresponding C code at the same time.
>
> b) While the way PCI devices are done is weird, I think it'll
> be a lot simpler if you can stick to a structure that's similar
> to them while diverging. It's hard enough for those of us
> who don't do Virtio every day to get our minds around virtio-pci
> without it being different for virtio-ccw/css.
>
> c) To do (b) I suggest:
> c.1) you *don't* add a vmsd to your virtio_ccw_device
>
> c.2) but *do* add a VMSTATE_CCW_DEVICE to the start of any
> non-virtio devices you migrate (like each of the PCI devices
> have)
>
> c.3) You can add extra state for CSS to the ->save_extra_state
> handle on virtio devices or to the config
>
> d) vmstatifying the config is OK as well.
>
> I should say I'm no virtio expert, so if any of that's truly
> mad say so.
>
> Dave
>
Agreed (a,b,c,d). Reorganizing my patch set according to a) is
going to require some effort, but it should not be too bad.
About c.2): I don't think we have any migratable non virtio devices
yet, but I'll keep it in mind.
I do not understand what you mean by c.3) and extra_sate. Maybe
it will settle with time. My idea of extending VirtioCcwDevice
is just adding subsections to it's VMStateDescription. The
call vmstate_save_state(f, &vmstate_virtio_ccw_dev, dev, NULL)
in save_config should take care of compatibility. Maybe some
staring at virtio-pci is going to help, but right now I can't tell
what's the extra_state for, and how/why is it 'extra'.
Thanks for your patience!
Regards,
Halil
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css
2017-05-19 17:47 ` Dr. David Alan Gilbert
@ 2017-05-19 18:04 ` Halil Pasic
0 siblings, 0 replies; 42+ messages in thread
From: Halil Pasic @ 2017-05-19 18:04 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Cornelia Huck, qemu-devel, Michael S. Tsirkin
On 05/19/2017 07:47 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>>
>>
>> On 05/19/2017 04:55 PM, Dr. David Alan Gilbert wrote:
>>>> We could also consider making WITH_TMP act as a normal field.
>>>> Working on the whole state looks like a bit like a corner case:
>>>> we have some stuff adjacent in the migration stream, and we have
>>>> to map it on multiple fields (and vice-versa). Getting the whole
>>>> state with a pointer to a certain field could work via container_of.
>>> You do need to know which field you're working on to be able to safely
>>> use container_of, so I'm not sure how it would work for you in this
>>> case.
>>
>>
>> Well, if you have to write to just one field you are good because you
>> already have a pointer to that field (.offset was added).
>>
>> If you need to write to multiple fields in post_load then you just pick
>> one of the fields you are going to write to (probably the first) and use
>> container_of to gain access to the whole state. The logic is specific to
>> the bunch of the fields you are going to touch anyway.
>>
>> In fact any member of the state struct will do it's only important that
>> you use the same when creating the VMStateField and when trying to get a
>> pointer to the parent in pre_save and post_load.
>>
>> I haven't tried, so I'm not 100% sure, but if you like I can try, and send
>> you a patch if it's viable.
>>
>> I think the key to a good solution is really what is intended and typical
>> usage, and what is corner case. My patch in the other reply shows that we
>> can do without changing the ways of VMSTATE_WITH_TMP. I think we can make
>> what I'm trying to do here a bit prettier at the expense of making what
>> you are doing in virtio-net a bit uglier, but whether it's a good idea to
>> do so, I cant tell.
>
> Lets go with what you put in the other patch (I replied to it); I hadn't
> realised that was possible (hence my comment below).
> Once we have a bunch of different uses of VMSTATE_WITH_TMP in the code
> base, I'll step back and see how to tidy them up.
>
> Dave
>
Sounds very reasonable. Let's do it like that!
Halil
>>>
>>> The other thought I'd had was that perhaps we could change the temporary
>>> structure in VMSTATE_WITH_TMP to:
>>>
>>> struct foo {
>>> struct whatever **parent;
>>>
>>> so now you could write to *parent in cases like these.
>>>
>>
>> Sorry, I do not get your idea. If you have some WIP patch in this
>> direction I would be happy to provide some feedback.
>>
>>
>>>> Btw, I would rather call it get_indicator a factory method or even a
>>>> constructor than an allocator, but I think we understand each-other
>>>> anyway.
>>> Yes; I'm not too worried about the actual name as long as it's short
>>> and obvious.
>>>
>>> I'd thought of 'allocator' since in most cases it's used where the
>>> load-time code allocates memory for the object being loaded.
>>> A constructor is normally something I think of as happening after
>>> allocation; and a factory, hmm maybe. However, if it does the right
>>> thing I wouldn't object to any of those names.
>>>
>>
>> I think we are on the same page.
>>
>> Cheers,
>> Halil
>>
>>> Dave
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration
2017-05-19 18:02 ` Halil Pasic
@ 2017-05-19 18:38 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 42+ messages in thread
From: Dr. David Alan Gilbert @ 2017-05-19 18:38 UTC (permalink / raw)
To: Halil Pasic; +Cc: Cornelia Huck, qemu-devel, Michael S. Tsirkin, dhildenb
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>
>
> On 05/19/2017 07:28 PM, Dr. David Alan Gilbert wrote:
> >> To sum it up although I'm currently leaning towards abandoning my idea
> >> of two sections for two devices, I'm not comfortable with making the
> >> call myself. I'm hoping for some maintainer guidance (s390x, virtio
> >> and migration).
> > <A couple of hours of reading, the CCW and PCI code, chatting with
> > dhildenb etc>
> >
> > OK, so I think:
> > a) First split the series into two separate series; one that
> > VMStatifies the existing stuff without breaking compatibility;
> > and one that adds the new stuff. Lets get the first of those
> > going in while we think about the second.
> >
> > a.1) I'd do this with patches that convert one chunk into
> > vmstate and remove the corresponding C code at the same time.
> >
> > b) While the way PCI devices are done is weird, I think it'll
> > be a lot simpler if you can stick to a structure that's similar
> > to them while diverging. It's hard enough for those of us
> > who don't do Virtio every day to get our minds around virtio-pci
> > without it being different for virtio-ccw/css.
> >
> > c) To do (b) I suggest:
> > c.1) you *don't* add a vmsd to your virtio_ccw_device
> >
> > c.2) but *do* add a VMSTATE_CCW_DEVICE to the start of any
> > non-virtio devices you migrate (like each of the PCI devices
> > have)
> >
> > c.3) You can add extra state for CSS to the ->save_extra_state
> > handle on virtio devices or to the config
> >
> > d) vmstatifying the config is OK as well.
> >
> > I should say I'm no virtio expert, so if any of that's truly
> > mad say so.
> >
> > Dave
> >
>
> Agreed (a,b,c,d). Reorganizing my patch set according to a) is
> going to require some effort, but it should not be too bad.
> About c.2): I don't think we have any migratable non virtio devices
> yet, but I'll keep it in mind.
>
> I do not understand what you mean by c.3) and extra_sate. Maybe
> it will settle with time. My idea of extending VirtioCcwDevice
> is just adding subsections to it's VMStateDescription. The
> call vmstate_save_state(f, &vmstate_virtio_ccw_dev, dev, NULL)
> in save_config should take care of compatibility. Maybe some
> staring at virtio-pci is going to help, but right now I can't tell
> what's the extra_state for, and how/why is it 'extra'.
Yes adding extra subsections into the 'config' is probably fine;
but there's also another hook that Jason added, see a6df8adf3,
it's an existing subsection at the end of the virtio state
that can be linked for transport specific data.
Dave
> Thanks for your patience!
>
> Regards,
> Halil
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2017-05-19 18:38 UTC | newest]
Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-05 17:34 [Qemu-devel] [PATCH 00/10] migration: s390x css migration Halil Pasic
2017-05-05 17:34 ` [Qemu-devel] [PATCH 01/10] s390x: add helper get_machine_class Halil Pasic
2017-05-05 17:34 ` [Qemu-devel] [PATCH 02/10] s390x: add css_migration_enabled to machine class Halil Pasic
2017-05-05 17:35 ` [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css Halil Pasic
2017-05-08 16:45 ` Dr. David Alan Gilbert
2017-05-09 12:00 ` Halil Pasic
2017-05-15 18:01 ` Dr. David Alan Gilbert
2017-05-18 14:15 ` Halil Pasic
2017-05-19 14:55 ` Dr. David Alan Gilbert
2017-05-19 15:08 ` Halil Pasic
2017-05-19 16:00 ` Halil Pasic
2017-05-19 17:43 ` Dr. David Alan Gilbert
2017-05-19 16:33 ` Halil Pasic
2017-05-19 17:47 ` Dr. David Alan Gilbert
2017-05-19 18:04 ` Halil Pasic
2017-05-09 12:20 ` Halil Pasic
2017-05-05 17:35 ` [Qemu-devel] [PATCH 04/10] s390x/css: add vmstate macro for CcwDevice Halil Pasic
2017-05-05 17:35 ` [Qemu-devel] [PATCH 05/10] virtio-ccw: add vmstate entities for VirtioCcwDevice Halil Pasic
2017-05-05 17:35 ` [Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration Halil Pasic
2017-05-08 16:55 ` Dr. David Alan Gilbert
2017-05-09 17:05 ` Halil Pasic
2017-05-10 10:31 ` Dr. David Alan Gilbert
2017-05-10 10:38 ` Cornelia Huck
2017-05-08 17:36 ` Dr. David Alan Gilbert
2017-05-08 17:53 ` Halil Pasic
2017-05-08 17:59 ` Dr. David Alan Gilbert
2017-05-08 18:27 ` Halil Pasic
2017-05-08 18:42 ` Dr. David Alan Gilbert
2017-05-10 11:52 ` Halil Pasic
2017-05-15 19:07 ` Dr. David Alan Gilbert
2017-05-16 22:05 ` Halil Pasic
2017-05-19 17:28 ` Dr. David Alan Gilbert
2017-05-19 18:02 ` Halil Pasic
2017-05-19 18:38 ` Dr. David Alan Gilbert
2017-05-05 17:35 ` [Qemu-devel] [PATCH 07/10] s390x/css: remove unused subch_dev_(load|save) Halil Pasic
2017-05-05 17:35 ` [Qemu-devel] [PATCH 08/10] s390x/css: add ORB to SubchDev Halil Pasic
2017-05-05 17:35 ` [Qemu-devel] [PATCH 09/10] s390x/css: turn on channel subsystem migration Halil Pasic
2017-05-08 17:27 ` Dr. David Alan Gilbert
2017-05-08 18:03 ` Halil Pasic
2017-05-08 18:37 ` Dr. David Alan Gilbert
2017-05-09 17:27 ` Halil Pasic
2017-05-05 17:35 ` [Qemu-devel] [PATCH 10/10] s390x/css: use SubchDev.orb Halil Pasic
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.