From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59373) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cCDHG-0008Cc-7h for qemu-devel@nongnu.org; Wed, 30 Nov 2016 17:23:00 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cCDHC-0001HA-Aa for qemu-devel@nongnu.org; Wed, 30 Nov 2016 17:22:58 -0500 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:36536 helo=mx0a-001b2d01.pphosted.com) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cCDHC-0001Ge-5Y for qemu-devel@nongnu.org; Wed, 30 Nov 2016 17:22:54 -0500 Received: from pps.filterd (m0098419.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id uAUMIqAa013498 for ; Wed, 30 Nov 2016 17:22:53 -0500 Received: from e36.co.us.ibm.com (e36.co.us.ibm.com [32.97.110.154]) by mx0b-001b2d01.pphosted.com with ESMTP id 2727e8g65g-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Wed, 30 Nov 2016 17:22:53 -0500 Received: from localhost by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 30 Nov 2016 15:22:52 -0700 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <20161122225844.3756.74867@loki> References: <1479433227-29238-1-git-send-email-mdroth@linux.vnet.ibm.com> <1479433227-29238-2-git-send-email-mdroth@linux.vnet.ibm.com> <20161122061510.GE28479@umbus.fritz.box> <20161122225844.3756.74867@loki> Date: Wed, 30 Nov 2016 16:22:42 -0600 Message-Id: <20161130222242.1725.43900@loki> Subject: Re: [Qemu-devel] [PATCH for-2.8 1/3] migration: alternative way to set instance_id in SaveStateEntry List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: duanj@linux.vnet.ibm.com, quintela@redhat.com, qemu-devel@nongnu.org, dgilbert@redhat.com, qemu-ppc@nongnu.org, bharata@linux.vnet.ibm.com, amit.shah@redhat.com Quoting Michael Roth (2016-11-22 16:58:44) > 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 > > > = > > > 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 =3D=3D -1 and thus falling back to the previous > > > logic of using the QOM path. > > > = > > > Signed-off-by: Jianjun Duan > > > * 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 > > = > > 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 =3D 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_c= b, > &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 tr= ying > to use as sPAPRMachineState, which was leading to a NULL pointer derefere= nce > 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 wo= uld > 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, boo= l value, Error **errp) > > > } > > > = > > > if (qdev_get_vmsd(dev)) { > > > - vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(de= v), dev, > > > + int instance_id =3D DEVICE_GET_CLASS(dev)->dev_get_insta= nce_id > > > + ? DEVICE_GET_CLASS(dev)->dev_get_instance_id(dev) > > > + : -1; > > > + > > > + vmstate_register_with_alias_id(dev, instance_id, qdev_ge= t_vmsd(dev), dev, > > > dev->instance_id_alias, > > > dev->alias_required_for_v= ersion); > > > } > > > 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 re= alize */ > > > qdev_event exit; /* TODO remove, once users are converted to unr= ealize */ > > > const char *bus_type; > > > + > > > + /* When this field is set, instead of using the device's QOM pat= h, > > > + * SaveStateEntry's for devices will be identified using a combi= nation > > > + * 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 identifie= r 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 =3D 1; > > > } > > > = > > > - if (dev) { > > > + if (dev && instance_id =3D=3D -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 =3D=3D -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... It turns out spapr_iommu already had a fairly straightforward solution. Rather than relying on qdev to register it's vmstate, it does it manually via: vmstate_register(DEVICE(tcet), tcet->liobn, &vmstate_spapr_tce_table, tcet); This avoids the qom-path-based code due to the fact that qdev_get_dev_path() is NULL for bus-less Devices, so as a fallback the VMSD gets identified via "spapr_iommu"/liobn. So using this approach, we don't actually need patch 1 at all. But it turns out, for this particular memory unplug issue, we don't actually need DRC migration. The real issue is that the default DRC state for coldplugged LMBs *should* correspond to the post-hotplug state. Otherwise, unplug fails for cold-plugged LMBs *even if you don't migrate beforehand*. The fix for that is much more straight-forward, and the same approach we currently use for coldplugged CPUs. Will send a patch shortly. > = > > = > > > char *id =3D 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 *d= ev, int instance_id, > > > se->vmsd =3D vmsd; > > > se->alias_id =3D alias_id; > > > = > > > - if (dev) { > > > + if (dev && instance_id =3D=3D -1) { > > > char *id =3D qdev_get_dev_path(dev); > > > if (id) { > > > pstrcpy(se->idstr, sizeof(se->idstr), id); > > = > > -- = > > David Gibson | I'll have my music baroque, and my co= de > > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _ot= her_ > > | _way_ _around_! > > http://www.ozlabs.org/~dgibson > = >=20