Re: [Qemu-ppc] [PATCH for-2.8 1/3] migration: alternative way to set instance_id in SaveStateEntry

[Michael Roth]

Quoting David Gibson (2016-11-22 00:15:10)
> On Thu, Nov 17, 2016 at 07:40:25PM -0600, Michael Roth wrote:
> > From: Jianjun Duan <address@hidden>
> > 
> > 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 <address@hidden>
> > * 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 <address@hidden>
> 
> 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.

Sorry for the breakage. I will make sure to run make check on ppc64
beforehand next time as well. Though in this case we might've gotten
lucky that it happened to trigger at all...

> 
> 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
> 
> 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.

It turns out all these strange connections might just be red herrings.
I think the problem is here in spapr_pci_post_load:

    unsigned int bus_no = 0;

    /* Set detach_cb for the drc unconditionally after migration */
    if (bus) {
        pci_for_each_device(bus, pci_bus_num(bus), spapr_pci_set_detach_cb,
                            &bus_no);
    }

I think that was a copy/paste artifact from one of the other callsites in
spapr_pci.c. &bus_no is an opaque that spapr_pci_set_detach_cb ends up trying
to use as sPAPRMachineState, which was leading to a NULL pointer dereference
when trying to access the DRC corresponding the the PCIDevice. The search
query is something like:

    return spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PCI,
                                    (phb->index << 16) |
                                    (busnr << 8) |
                                    devfn);

My guess is that in some cases phb->index just happened to be 0, which would
in most cases lead to a successful query. That might explain the different
results on different architectures.

Based on the some discussion elsehwere is turns out we don't need this
detach cb stuff at all for the unplug issue we're trying to fix here, so
I will strip it out for the next version.

> 
> I do have one vague theory..

I think that needs to be dealt with as well, more comments below.

> 
> > ---
> >  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.

I don't think this is the cause (verified the loading orders didn't
change before/after these patches for the scenario in question), but
that's a good catch:

there was previously se->compat->idstr/instance_id handling for
cases where a we register a VMSD for a Device and provide an explicit
instance_id. This registers an additional check so that we can map
the se->compat values from an older QEMU to the current ID. This
unfortunately doesn't work in the opposite direction so didn't suite
our purposes for controlling identifiers in the outgoing stream, but
by accidentally bypassing it completely there's now a chance old->new
configurations might break for certain devices, so this needs to be
re-thought a bit...

> 
> >          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