* David Gibson (da...@gibson.dropbear.id.au) wrote:
> On Thu, Nov 17, 2016 at 07:40:25PM -0600, Michael Roth wrote:
> > From: Jianjun Duan <du...@linux.vnet.ibm.com>
> >
> > Currently migrated Devices are identified with an idstr which is
> > calculated automatically using their path in the QOM composition
> > tree. In some cases these Devices may have a more reliable
> > identifier that may be preferable in situations where there's a
> > chance their path in the composition tree might change in the
> > future as a resulting of changes in how the device is modeled
> > in QEMU.
> >
> > One such Device is the "spapr-dr-connector" used to handle hotplug
> > for various resources on pseries machine types, where the PAPR
> > specification mandates that each DRC have a 32-bit globally unique
> > identifier associated with it. As such, this identifier is also ideal
> > as a reliable way to identify a particular DRC in the migration
> > stream, so we introduce support here for using a caller-side supplied
> > instance_id for Devices in preparation for that.
> >
> > register_savevm_live() and vmstate_register_with_alias_id() already
> > provide a means to let the caller supply an instance_id instead of
> > relying on the migration subsystem to generate one automatically,
> > but in cases where we're registering SaveVMHandlers or VMSDs
> > (respectively) that are associated with a Device, the instance_id is
> > ignored in favor of a string identifier based solely on the QOM path.
> >
> > This patch generalizes this so that an instance_id can also be
> > supplied by the caller for Devices. Since VMSD registration for
> > Devices is generally handled automatically by qdev, we also introduce
> > a DeviceClass->dev_get_instance_id() method that, when set, is called
> > by qdev to obtain the corresponding instance_id that should be used
> > for a particular Device. Otherwise we maintain the original behavior
> > of passing instance_id == -1 and thus falling back to the previous
> > logic of using the QOM path.
> >
> > Signed-off-by: Jianjun Duan <du...@linux.vnet.ibm.com>
> > * moved usage of DeviceClass->dev_get_instance_id() out of savevm.c
> > and into caller (qdev) instead
> > * clarified usage/intent in comments and commit msg
> > Signed-off-by: Michael Roth <mdr...@linux.vnet.ibm.com>
>
> I've had to remove this patch and 2/3 from ppc-for-2.8, because I
> discovered (as I was preparing a pull request) that it causes a weird
> breakage.
>
> Specifically, on some, but not all, setups it causes the postcopy-test
> to fail with the error:
>
> qemu-system-ppc64: RP: Received invalid message 0x0000 length 0x0000
> FAIL
Hmm; that's from the source side of the migration, it probably means the
destination
has died and that's hiding the real error somewhere.
Dave
> For me, it fails when running on a ppc64le or ppc64 host (RHEL7.3),
> and on 32-bit x86 (Fedora container) but not on x86_64 host (Fedora
> 24).
>
> That's a pretty baffling set of symptoms, and so far I haven't gotten
> far in figuring out how it could happen. But I really want to get the
> rest of the patches in ppc-for-2.8 pulled, so I've dropped these for
> now.
>
> I'll try to debug this once I've prepared the next pull request, but
> if you're able to work out what's going on for me, that would be
> extremely helpful.
>
> I do have one vague theory..
>
> > ---
> > hw/core/qdev.c | 6 +++++-
> > include/hw/qdev-core.h | 9 +++++++++
> > migration/savevm.c | 4 ++--
> > 3 files changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 5783442..c8c0c44 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -933,7 +933,11 @@ static void device_set_realized(Object *obj, bool
> > value, Error **errp)
> > }
> >
> > if (qdev_get_vmsd(dev)) {
> > - vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev),
> > dev,
> > + int instance_id = DEVICE_GET_CLASS(dev)->dev_get_instance_id
> > + ? DEVICE_GET_CLASS(dev)->dev_get_instance_id(dev)
> > + : -1;
> > +
> > + vmstate_register_with_alias_id(dev, instance_id,
> > qdev_get_vmsd(dev), dev,
> > dev->instance_id_alias,
> >
> > dev->alias_required_for_version);
> > }
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index 2c97347..8ba82af 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -139,6 +139,15 @@ 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, instead of using the device's QOM path,
> > + * SaveStateEntry's for devices will be identified using a combination
> > + * of the corresponding VMSD name and an instance_id returned by this
> > + * function. This should only be necessary for situations where the
> > + * QOM path is anticipated to change and a more stable identifier is
> > + * desired to identify a device in the migration stream.
> > + */
> > + int (*dev_get_instance_id)(DeviceState *dev);
> > } DeviceClass;
> >
> > typedef struct NamedGPIOList NamedGPIOList;
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 0363372..a95fff9 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -556,7 +556,7 @@ int register_savevm_live(DeviceState *dev,
> > se->is_ram = 1;
> > }
> >
> > - if (dev) {
> > + if (dev && instance_id == -1) {
>
> .. so, I'm not sure how this could lead to the observed symptoms, but
> I did note that adding this condition makes another if within the
> block redundant, because it also checks for instance_id == -1. That
> suggests that simply skipping the whole block in this case is probably
> not the right thing to do.
>
> > char *id = qdev_get_dev_path(dev);
> > if (id) {
> > pstrcpy(se->idstr, sizeof(se->idstr), id);
> > @@ -640,7 +640,7 @@ int vmstate_register_with_alias_id(DeviceState *dev,
> > int instance_id,
> > se->vmsd = vmsd;
> > se->alias_id = alias_id;
> >
> > - if (dev) {
> > + if (dev && instance_id == -1) {
> > char *id = qdev_get_dev_path(dev);
> > if (id) {
> > pstrcpy(se->idstr, sizeof(se->idstr), id);
>
> --
> David Gibson | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_
> _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
