From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55054) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bsLyi-0001Li-AJ for qemu-devel@nongnu.org; Thu, 06 Oct 2016 23:37:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bsLyg-00083S-Fn for qemu-devel@nongnu.org; Thu, 06 Oct 2016 23:37:44 -0400 Date: Fri, 7 Oct 2016 14:12:56 +1100 From: David Gibson Message-ID: <20161007031256.GL18490@umbus.fritz.box> References: <1475519097-27611-1-git-send-email-duanj@linux.vnet.ibm.com> <1475519097-27611-3-git-send-email-duanj@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="sMZCuqyhuhd4ycTi" Content-Disposition: inline In-Reply-To: <1475519097-27611-3-git-send-email-duanj@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [QEMU PATCH v5 2/6] migration: spapr_drc: defined VMStateDescription struct List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jianjun Duan Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, dmitry@daynix.com, peter.maydell@linaro.org, kraxel@redhat.com, mst@redhat.com, pbonzini@redhat.com, veroniabahaa@gmail.com, quintela@redhat.com, amit.shah@redhat.com, mreitz@redhat.com, kwolf@redhat.com, rth@twiddle.net, aurelien@aurel32.net, leon.alrae@imgtec.com, blauwirbel@gmail.com, mark.cave-ayland@ilande.co.uk, mdroth@linux.vnet.ibm.com, dgilbert@redhat.com --sMZCuqyhuhd4ycTi Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Oct 03, 2016 at 11:24:53AM -0700, Jianjun Duan wrote: > To manage hotplug/unplug of dynamic resources such as PCI cards, > memory, and CPU on sPAPR guests, a firmware abstraction known as > a Dynamic Resource Connector (DRC) is used to assign a particular > dynamic resource to the guest, and provide an interface for the > guest to manage configuration/removal of the resource associated > with it. >=20 > To migrate the hotplugged resources in migration, the > associated DRC state need be migrated. To migrate the DRC state, > we defined the VMStateDescription struct for spapr_drc to enable > the transmission of spapr_drc state in migration. >=20 > Not all the elements in the DRC state are migrated. Only those > ones modifiable or needed by guest actions or device add/remove > operation are migrated. From the perspective of device > hotplugging, if we hotplug a device on the source, we need to > "coldplug" it on the target. The states across two hosts for the > same device are not the same. Ideally we want the states be same > after migration so that the device would function as hotplugged > on the target. For example we can unplug it. The minimum DRC > state we need to transfer should cover all the pieces changed by > hotplugging. Out of the elements of the DRC state, isolation_state, > allocation_sate, and configured are involved in the DR state > transition diagram from PAPR+ 2.7, 13.4. configured and signalled > are needed in attaching and detaching devices. indicator_state > provides users with hardware state information. These 6 elements > are migrated. Hmm.. are you saying that the DRC state of a coldplugged device (after we've fully booted) is different from the DRC state of a hotplugged device (after all the hotplug operations have fully completed)? If that's correct that sounds like a general bug in the DRC state management, not something only related to migration. Looking at the code, though, that doesn't really seem to be what it's doing. > detach_cb in the DRC state is a function pointer that cannot be > migrated. We set it right after DRC state is migrated so that > a migrated hot-unplug event could finish its work. >=20 > The instance_id is used to identify objects in migration. We set > instance_id of DRC using the unique index so that it is the same > across migration. >=20 > Signed-off-by: Jianjun Duan > --- > hw/ppc/spapr_drc.c | 69 ++++++++++++++++++++++++++++++++++++++++= ++++++ > hw/ppc/spapr_pci.c | 22 +++++++++++++++ > include/hw/ppc/spapr_drc.h | 9 ++++++ > 3 files changed, 100 insertions(+) >=20 > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index 6e54fd4..369ec02 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c > @@ -615,6 +615,71 @@ static void spapr_dr_connector_instance_init(Object = *obj) > NULL, NULL, NULL, NULL); > } > =20 > +static bool spapr_drc_needed(void *opaque) > +{ > + sPAPRDRConnector *drc =3D (sPAPRDRConnector *)opaque; > + sPAPRDRConnectorClass *drck =3D SPAPR_DR_CONNECTOR_GET_CLASS(drc); > + bool rc =3D false; > + sPAPRDREntitySense value; > + > + drck->entity_sense(drc, &value); > + /* If no dev is plugged in there is no need to migrate the DRC state= */ > + if (value !=3D SPAPR_DR_ENTITY_SENSE_PRESENT) { > + return false; > + } > + /* > + * If there is dev plugged in, we need to migrate the DRC state when > + * it is different from cold-plugged state > + */ > + switch(drc->type) { > + /* for PCI type */ > + case SPAPR_DR_CONNECTOR_TYPE_PCI: > + rc =3D !((drc->isolation_state =3D=3D SPAPR_DR_ISOLATION_STATE_U= NISOLATED) && > + (drc->allocation_state =3D=3D SPAPR_DR_ALLOCATION_STATE_U= SABLE) && > + drc->configured && drc->signalled && !drc->awaiting_relea= se); > + break; > + /* for LMB type */ > + case SPAPR_DR_CONNECTOR_TYPE_LMB: > + rc =3D !((drc->isolation_state =3D=3D SPAPR_DR_ISOLATION_STATE_I= SOLATED) && > + (drc->allocation_state =3D=3D SPAPR_DR_ALLOCATION_STATE_U= NUSABLE) && > + drc->configured && drc->signalled && !drc->awaiting_relea= se); > + break; What about CPU type?z > + default: > + ; > + } > + > + return rc; > +} > + > +/* detach_cb needs be set since it is not migrated */ > +static void postmigrate_set_detach_cb(sPAPRDRConnector *drc, > + spapr_drc_detach_cb *detach_cb) > +{ > + drc->detach_cb =3D detach_cb; > +} > + > +/* return the unique drc index as instance_id for qom interfaces*/ > +static int get_instance_id(DeviceState *dev) > +{ > + return (int)get_index(SPAPR_DR_CONNECTOR(OBJECT(dev))); > +} > + > +static const VMStateDescription vmstate_spapr_drc =3D { > + .name =3D "spapr_drc", > + .version_id =3D 1, > + .minimum_version_id =3D 1, > + .needed =3D spapr_drc_needed, > + .fields =3D (VMStateField []) { > + VMSTATE_UINT32(isolation_state, sPAPRDRConnector), > + VMSTATE_UINT32(allocation_state, sPAPRDRConnector), > + VMSTATE_UINT32(indicator_state, sPAPRDRConnector), > + VMSTATE_BOOL(configured, sPAPRDRConnector), > + VMSTATE_BOOL(awaiting_release, sPAPRDRConnector), > + VMSTATE_BOOL(signalled, sPAPRDRConnector), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static void spapr_dr_connector_class_init(ObjectClass *k, void *data) > { > DeviceClass *dk =3D DEVICE_CLASS(k); > @@ -623,6 +688,8 @@ static void spapr_dr_connector_class_init(ObjectClass= *k, void *data) > dk->reset =3D reset; > dk->realize =3D realize; > dk->unrealize =3D unrealize; > + dk->vmsd =3D &vmstate_spapr_drc; > + dk->dev_get_instance_id =3D get_instance_id; > drck->set_isolation_state =3D set_isolation_state; > drck->set_indicator_state =3D set_indicator_state; > drck->set_allocation_state =3D set_allocation_state; > @@ -636,6 +703,8 @@ static void spapr_dr_connector_class_init(ObjectClass= *k, void *data) > drck->detach =3D detach; > drck->release_pending =3D release_pending; > drck->set_signalled =3D set_signalled; > + drck->postmigrate_set_detach_cb =3D postmigrate_set_detach_cb; > + > /* > * Reason: it crashes FIXME find and document the real reason > */ > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 4f00865..080471c 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -1594,11 +1594,33 @@ static void spapr_pci_pre_save(void *opaque) > } > } > =20 > +/* > + * detach_cb in the DRC state is a function pointer that cannot be > + * migrated. We set it right after migration so that a migrated > + * hot-unplug event could finish its work. > + */ > +static void spapr_pci_set_detach_cb(PCIBus *bus, PCIDevice *pdev, > + void *opaque) > +{ > + sPAPRPHBState *sphb =3D opaque; > + sPAPRDRConnector *drc =3D spapr_phb_get_pci_drc(sphb, pdev); > + sPAPRDRConnectorClass *drck =3D SPAPR_DR_CONNECTOR_GET_CLASS(drc); > + drck->postmigrate_set_detach_cb(drc, spapr_phb_remove_pci_device_cb); Why is spapr_phb_remove_pci_device_cb the right callback rather than something else? This doesn't seem sensible. More to the point, you're not restoring detach_cb_opaque, which means the detach_cb callback won't work properly anyway. > +} > + > static int spapr_pci_post_load(void *opaque, int version_id) > { > sPAPRPHBState *sphb =3D opaque; > gpointer key, value; > int i; > + PCIBus *bus =3D PCI_HOST_BRIDGE(sphb)->bus; > + 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_= cb, > + &bus_no); > + } > =20 > for (i =3D 0; i < sphb->msi_devs_num; ++i) { > key =3D g_memdup(&sphb->msi_devs[i].key, > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h > index fa531d5..17589c8 100644 > --- a/include/hw/ppc/spapr_drc.h > +++ b/include/hw/ppc/spapr_drc.h > @@ -192,6 +192,15 @@ typedef struct sPAPRDRConnectorClass { > void *detach_cb_opaque, Error **errp); > bool (*release_pending)(sPAPRDRConnector *drc); > void (*set_signalled)(sPAPRDRConnector *drc); > + > + /* > + * QEMU interface for setting detach_cb after migration. > + * detach_cb in the DRC state is a function pointer that cannot be > + * migrated. We set it right after migration so that a migrated > + * hot-unplug event could finish its work. > + */ > + void (*postmigrate_set_detach_cb)(sPAPRDRConnector *drc, > + spapr_drc_detach_cb *detach_cb); > } sPAPRDRConnectorClass; > =20 > sPAPRDRConnector *spapr_dr_connector_new(Object *owner, --=20 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 --sMZCuqyhuhd4ycTi Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJX9xK1AAoJEGw4ysog2bOSwn4QAJa7RxINbNtzsvofiinuxfLL sxotx8ut3H8wcA9TetkHOVp147dyAl8XFjefYS6ix9F9WwRU8UhJIOvLPR2Ul0+Z oLOZLZqeceiQpqmyHGeXzCUM7ReqjqrsCF7ucpjn6nJyFYXiixSzYv2z3WtXFBc7 OZSB9cB4WHOODFkibDt6a47NBY/9GcXWACspwRUcZQDXSQw605Y81cSkEi3JOiNB E0dWK4hVhXKYD/DiP1JRgvl8F1NAS8pCzVwvR/R2EgL/H/m4H2Oy4XeiyRRrYfym J+59BG3v1ZcBoL22HU5Dr83tc7JJtcA1mIJ1OQCDJH4oDwbTRFaUhROu6EE1Bwnc leRwjbl/jTj7TfPgNFNhRoFdFFqMio6Y0e09b851ZCufGD9A0uMC9E8Va2tfhDDg BWEK5OWLjhZYrpoVH+DgYbweYM+O+lAIpYxvmKXutG0Ok2s+z8m7u7h5opPAKgh0 1lBupuvqxX7C8YfY7ZFxZiO/6drrS1pnJAPxIBBgXHM6c/dbd502djI8QTLNvo3L 4hvatS3VKyGVARH3p4gv+Rvvfxg8qK8g0SKWNsHHXpmwsVUJLiaPE9O9LVlTQoRi lduusM779E9TQdUboNtJ9pmS4AD0ODV1q4UHqisZ5TLYe7EC0FAW3K2H9b6l56Jk ajL24nwnNYwe226MAws6 =awyh -----END PGP SIGNATURE----- --sMZCuqyhuhd4ycTi--