| Message ID | 20170424220828.1472-2-danielhb@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Quoting Daniel Henrique Barboza (2017-04-24 17:08:25) > From: Jianjun Duan <duanj@linux.vnet.ibm.com> > > In QOM (QEMU Object Model) migrated objects are identified with instance_id > which is calculated automatically using their path in the QOM composition > tree. For some objects, this path could change from source to target in > migration. To migrate such objects, we need to make sure the instance_id does > not change from source to target. We add a hook in DeviceClass to do customized > instance_id calculation in such cases. When I tried to pluck a subset of these patches for another series it was noticed that we don't actually need this patch anymore: https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg05475.html hw/ppc/spapr_iommu.c already implements an approach for registering DRCs that would work for our case as well since DRCs are bus-less and do not sit on a BusClass that implements bc->get_dev_path, so using vmstate_register(DEVICE(drc), drck->get_index(drc), ...) will work in the same way this patch expects it to. > > As a result, in these cases compat will not be set in the concerned > SaveStateEntry. This will prevent the inconsistent idstr to be sent over in > migration. We could have set alias_id in a similar way. But that will be > overloading the purpose of alias_id. > > The first application will be setting instance_id for pseries DRC objects using > its unique index. Doing this makes the instance_id of DRC to be consistent > across migration and supports flexible management of DRC objects in migration. > > Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com> > Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> > --- > include/hw/qdev-core.h | 6 ++++++ > migration/savevm.c | 6 ++++++ > 2 files changed, 12 insertions(+) > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index 4bf86b0..9b3914c 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -127,6 +127,12 @@ typedef struct DeviceClass { > qdev_initfn init; /* TODO remove, once users are converted to realize */ > qdev_event exit; /* TODO remove, once users are converted to unrealize */ > const char *bus_type; > + > + /* When this field is set, qemu will use it to get an unique instance_id > + * instead of calculating an auto idstr and instance_id for the relevant > + * SaveStateEntry > + */ > + int (*dev_get_instance_id)(DeviceState *dev); > } DeviceClass; > > typedef struct NamedGPIOList NamedGPIOList; > diff --git a/migration/savevm.c b/migration/savevm.c > index 03ae1bd..5d8135f 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -606,6 +606,9 @@ int register_savevm_live(DeviceState *dev, > calculate_compat_instance_id(idstr) : instance_id; > instance_id = -1; > } > + if (DEVICE_GET_CLASS(dev)->dev_get_instance_id) { > + instance_id = DEVICE_GET_CLASS(dev)->dev_get_instance_id(dev); > + } > } > pstrcat(se->idstr, sizeof(se->idstr), idstr); > > @@ -696,6 +699,9 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id, > calculate_compat_instance_id(vmsd->name) : instance_id; > instance_id = -1; > } > + if (DEVICE_GET_CLASS(dev)->dev_get_instance_id) { > + instance_id = DEVICE_GET_CLASS(dev)->dev_get_instance_id(dev); > + } > } > pstrcat(se->idstr, sizeof(se->idstr), vmsd->name); > > -- > 2.9.3 > >
On 04/25/2017 07:26 PM, Michael Roth wrote: > Quoting Daniel Henrique Barboza (2017-04-24 17:08:25) >> From: Jianjun Duan <duanj@linux.vnet.ibm.com> >> >> In QOM (QEMU Object Model) migrated objects are identified with instance_id >> which is calculated automatically using their path in the QOM composition >> tree. For some objects, this path could change from source to target in >> migration. To migrate such objects, we need to make sure the instance_id does >> not change from source to target. We add a hook in DeviceClass to do customized >> instance_id calculation in such cases. > When I tried to pluck a subset of these patches for another series it > was noticed that we don't actually need this patch anymore: > > https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg05475.html > > hw/ppc/spapr_iommu.c already implements an approach for registering DRCs > that would work for our case as well since DRCs are bus-less and do not sit > on a BusClass that implements bc->get_dev_path, so using > vmstate_register(DEVICE(drc), drck->get_index(drc), ...) will work in > the same way this patch expects it to. Good to know. I'll see if I can get rid of this whole patch and use this approach instead. > >> As a result, in these cases compat will not be set in the concerned >> SaveStateEntry. This will prevent the inconsistent idstr to be sent over in >> migration. We could have set alias_id in a similar way. But that will be >> overloading the purpose of alias_id. >> >> The first application will be setting instance_id for pseries DRC objects using >> its unique index. Doing this makes the instance_id of DRC to be consistent >> across migration and supports flexible management of DRC objects in migration. >> >> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com> >> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> >> --- >> include/hw/qdev-core.h | 6 ++++++ >> migration/savevm.c | 6 ++++++ >> 2 files changed, 12 insertions(+) >> >> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >> index 4bf86b0..9b3914c 100644 >> --- a/include/hw/qdev-core.h >> +++ b/include/hw/qdev-core.h >> @@ -127,6 +127,12 @@ typedef struct DeviceClass { >> qdev_initfn init; /* TODO remove, once users are converted to realize */ >> qdev_event exit; /* TODO remove, once users are converted to unrealize */ >> const char *bus_type; >> + >> + /* When this field is set, qemu will use it to get an unique instance_id >> + * instead of calculating an auto idstr and instance_id for the relevant >> + * SaveStateEntry >> + */ >> + int (*dev_get_instance_id)(DeviceState *dev); >> } DeviceClass; >> >> typedef struct NamedGPIOList NamedGPIOList; >> diff --git a/migration/savevm.c b/migration/savevm.c >> index 03ae1bd..5d8135f 100644 >> --- a/migration/savevm.c >> +++ b/migration/savevm.c >> @@ -606,6 +606,9 @@ int register_savevm_live(DeviceState *dev, >> calculate_compat_instance_id(idstr) : instance_id; >> instance_id = -1; >> } >> + if (DEVICE_GET_CLASS(dev)->dev_get_instance_id) { >> + instance_id = DEVICE_GET_CLASS(dev)->dev_get_instance_id(dev); >> + } >> } >> pstrcat(se->idstr, sizeof(se->idstr), idstr); >> >> @@ -696,6 +699,9 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id, >> calculate_compat_instance_id(vmsd->name) : instance_id; >> instance_id = -1; >> } >> + if (DEVICE_GET_CLASS(dev)->dev_get_instance_id) { >> + instance_id = DEVICE_GET_CLASS(dev)->dev_get_instance_id(dev); >> + } >> } >> pstrcat(se->idstr, sizeof(se->idstr), vmsd->name); >> >> -- >> 2.9.3 >> >> >
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 4bf86b0..9b3914c 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -127,6 +127,12 @@ typedef struct DeviceClass { qdev_initfn init; /* TODO remove, once users are converted to realize */ qdev_event exit; /* TODO remove, once users are converted to unrealize */ const char *bus_type; + + /* When this field is set, qemu will use it to get an unique instance_id + * instead of calculating an auto idstr and instance_id for the relevant + * SaveStateEntry + */ + int (*dev_get_instance_id)(DeviceState *dev); } DeviceClass; typedef struct NamedGPIOList NamedGPIOList; diff --git a/migration/savevm.c b/migration/savevm.c index 03ae1bd..5d8135f 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -606,6 +606,9 @@ int register_savevm_live(DeviceState *dev, calculate_compat_instance_id(idstr) : instance_id; instance_id = -1; } + if (DEVICE_GET_CLASS(dev)->dev_get_instance_id) { + instance_id = DEVICE_GET_CLASS(dev)->dev_get_instance_id(dev); + } } pstrcat(se->idstr, sizeof(se->idstr), idstr); @@ -696,6 +699,9 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id, calculate_compat_instance_id(vmsd->name) : instance_id; instance_id = -1; } + if (DEVICE_GET_CLASS(dev)->dev_get_instance_id) { + instance_id = DEVICE_GET_CLASS(dev)->dev_get_instance_id(dev); + } } pstrcat(se->idstr, sizeof(se->idstr), vmsd->name);