From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33149) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b8eCH-0007Bx-Ud for qemu-devel@nongnu.org; Thu, 02 Jun 2016 21:46:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b8eCF-0004ZU-1E for qemu-devel@nongnu.org; Thu, 02 Jun 2016 21:46:48 -0400 Date: Fri, 3 Jun 2016 11:48:14 +1000 From: David Gibson Message-ID: <20160603014814.GF1087@voom.fritz.box> References: <1464717764-9040-1-git-send-email-duanj@linux.vnet.ibm.com> <1464717764-9040-2-git-send-email-duanj@linux.vnet.ibm.com> <20160602040703.GA22347@voom.fritz.box> <5750CF3D.6060308@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="/aVve/J9H4Wl5yVO" Content-Disposition: inline In-Reply-To: <5750CF3D.6060308@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [QEMU RFC PATCH v3 1/6] Migration: Defined VMStateDescription struct for spapr_drc 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 --/aVve/J9H4Wl5yVO Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jun 02, 2016 at 05:28:45PM -0700, Jianjun Duan wrote: >=20 >=20 > On 06/01/2016 09:07 PM, David Gibson wrote: > > On Tue, May 31, 2016 at 11:02:39AM -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. > >> > >> 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. > >> > >> 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. > >> > >> 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. > >> > >> Signed-off-by: Jianjun Duan > >=20 > > So, the thing which concerns me about this patch is that it exposes > > the DRCs as real objects in the migration stream, which will make it > > difficult to ever remove them in the future. > >=20 > > Particularly for LMBs having full QOM objects for every individual DRC > > is kind of clunky, and I'd already considered replacing them instead > > with a QOM interface implementing a whole array of DRCs as part of the > > "owning" object (machine or PHB). > >=20 > > The internal structure of the VMSD looks ok to me, but the > > complication is the automatic "addressing" of it as a separate > > object. If we could manually take that over, allowing us to direct > > the same VMSD data at elements in a simpler DRC array later, that > > would be preferable. >=20 > We can address this concern by setting the unique instance_id for each > drc. The instance_id can be used to match the drcs from source and > target. Ah, yes, that sounds like it should work. And we already have a DRC id value we can use for the purpose. > >=20 > >=20 > >> --- > >> hw/ppc/spapr_drc.c | 61 +++++++++++++++++++++++++++++++++++++= +++++++++ > >> hw/ppc/spapr_pci.c | 22 +++++++++++++++++ > >> include/hw/ppc/spapr_drc.h | 9 +++++++ > >> 3 files changed, 92 insertions(+) > >> > >> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > >> index 94c875d..1fb5e23 100644 > >> --- a/hw/ppc/spapr_drc.c > >> +++ b/hw/ppc/spapr_drc.c > >> @@ -617,6 +617,65 @@ static void spapr_dr_connector_instance_init(Obje= ct *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 st= ate */ > >> + if (value !=3D SPAPR_DR_ENTITY_SENSE_PRESENT) { > >> + return false; > >> + } > >> + /* > >> + * If there is dev plugged in, we need to migrate the DRC state w= hen > >> + * 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_STAT= E_UNISOLATED) && > >> + (drc->allocation_state =3D=3D SPAPR_DR_ALLOCATION_STAT= E_USABLE) && > >> + drc->configured && drc->signalled && !drc->awaiting_re= lease); > >> + break; > >> + /* for LMB type */ > >> + case SPAPR_DR_CONNECTOR_TYPE_LMB: > >> + rc =3D !((drc->isolation_state =3D=3D SPAPR_DR_ISOLATION_STAT= E_ISOLATED) && > >> + (drc->allocation_state =3D=3D SPAPR_DR_ALLOCATION_STAT= E_UNUSABLE) && > >> + drc->configured && drc->signalled && !drc->awaiting_re= lease); > >> + break; > >> + 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; > >> +} > >> + > >> +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); > >> @@ -625,6 +684,7 @@ static void spapr_dr_connector_class_init(ObjectCl= ass *k, void *data) > >> dk->reset =3D reset; > >> dk->realize =3D realize; > >> dk->unrealize =3D unrealize; > >> + dk->vmsd =3D &vmstate_spapr_drc; > >> drck->set_isolation_state =3D set_isolation_state; > >> drck->set_indicator_state =3D set_indicator_state; > >> drck->set_allocation_state =3D set_allocation_state; > >> @@ -638,6 +698,7 @@ static void spapr_dr_connector_class_init(ObjectCl= ass *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 856aec7..c68da08 100644 > >> --- a/hw/ppc/spapr_pci.c > >> +++ b/hw/ppc/spapr_pci.c > >> @@ -1562,11 +1562,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); > >> +} > >> + > >> 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_deta= ch_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 fa21ba0..a68433b 100644 > >> --- a/include/hw/ppc/spapr_drc.h > >> +++ b/include/hw/ppc/spapr_drc.h > >> @@ -190,6 +190,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 > Thanks, > Jianjun >=20 --=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 --/aVve/J9H4Wl5yVO Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXUOHeAAoJEGw4ysog2bOSw3YP/0q8bxidEFjitfxQQltnye62 g9zUnOz4S5fDgFGU5wKBV+eajt7MCXhQTsdMKhRYfTNziWOaC510u4hu8r20FCFo vWI8K0pRHZBLo+hKDeoobLLUZw2SCYWht0ZkC61GIjhvKYFHgGiGBpyg8ZfHPXYW SFjYVaV3b4Jtdu/E7AnGylwIK1v9y5ryi6wv1JSa1mWKm28hGyrE8U9DNjuLWGuP 6XbUnMEr0XO3kRHjpaYNfSdND4n8qx7+QD1jTwj4SpPAI59WexoUWkPV9uiFMqbW tUzWJDwxJSqQOYUljM8FKm59z8FroUMBFyBHgrH9zYm+rzYf40YBJAYhCKpPfA8/ uslFWC8Z+D02Z5C9e0mhb5Of9whJAFAdqGFuM31sON5QlTnBIcnZlami+YaMTYWU UZtSz1kgd5d7n1oBcd6xC+DLrOmwpaAJIDpwArGBxir0BRjd+3H8KRziM5HWDkNt Exv+RBJf90VfV+Fq0jLpd9JbJSkrsflDR8EUkOp55dYpVHbplOt3ocpLB9JRD7sT IHqhaShucquzsPg/0GK4LyYhBmQ5lGHK4SRI2kCr816Vm2lRfkBQIC5WItA9G8z3 Wsl3ojnDDBpGUi1W+6GVM3w4CYAyN9Z1N8ECvC9Keyo+vgpLGyZ4U3XLziH+xH+y 7BLSAWLNg4zYudOE3nlS =O9/y -----END PGP SIGNATURE----- --/aVve/J9H4Wl5yVO--