From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54117) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c7gux-0007YO-S9 for qemu-devel@nongnu.org; Fri, 18 Nov 2016 06:01:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c7gur-0006tT-IO for qemu-devel@nongnu.org; Fri, 18 Nov 2016 06:01:15 -0500 Date: Fri, 18 Nov 2016 17:04:33 +1100 From: David Gibson Message-ID: <20161118060433.GE31640@umbus.fritz.box> References: <1479433227-29238-1-git-send-email-mdroth@linux.vnet.ibm.com> <1479433227-29238-3-git-send-email-mdroth@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="6Vw0j8UKbyX0bfpA" Content-Disposition: inline In-Reply-To: <1479433227-29238-3-git-send-email-mdroth@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH for-2.8 2/3] migration: spapr_drc: defined VMStateDescription struct List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, duanj@linux.vnet.ibm.com, bharata@linux.vnet.ibm.com, dgilbert@redhat.com, quintela@redhat.com, amit.shah@redhat.com --6Vw0j8UKbyX0bfpA Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Nov 17, 2016 at 07:40:26PM -0600, Michael Roth wrote: > From: Jianjun Duan >=20 > 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. >=20 > 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 > * add migration for awaiting_allocation state > Signed-off-by: Michael Roth > --- > hw/ppc/spapr_drc.c | 70 ++++++++++++++++++++++++++++++++++++++++= ++++++ > hw/ppc/spapr_pci.c | 22 +++++++++++++++ > include/hw/ppc/spapr_drc.h | 9 ++++++ > 3 files changed, 101 insertions(+) >=20 > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index a0c44ee..1ec6551 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c > @@ -632,6 +632,72 @@ 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; > + 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(awaiting_allocation, 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); > @@ -640,6 +706,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; > @@ -653,6 +721,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 f9661b7..661f7d8 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -1638,11 +1638,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_detach_= cb, > + &bus_no); > + } This reconstruction of the detach_cbs is a bit clunky. As noted elsewhere, I'm not going to delay the patches for that, of course. IIRC, this callback is always either NULL, or the detach callback that belongs to the particular DRC type. So for the sake of migration, I think it would make sense to change the representation of the DRC to instead of including a callback directly, simply have a boolean flag for whether the callback is active at the moment. When we need to call the callback, we check if it is active, and if it is look up the right function for the DRC type (we could use subclasses of DRC to do this). > =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 --6Vw0j8UKbyX0bfpA Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYLpnvAAoJEGw4ysog2bOSZTsQALauBOdYH9fIw06xSVr95o5w 4BTCuswgHnjJbeopgOzrMAlO0edVdfxK5yzx4AkPeKwaiupTi8A6lV1ulvfMVviR 65qfEe99g5APj+XDCuCHSiPK1OpWou/cr6HY+Ey7ixJM5tAf3QC0pG0u9HaxgS6k BXoinfRK/V4UQT/DZr4bDC+vOJ0exeezf6m0kmtzBAyrZsOli6sCafeQkIznBP1b eeNoMIAbDhKHCKLAXkaJ1/lB5dVRIAxcKxgxOTgf1+U2G9FgrI01IHi6EerPztzl fTBYPpXEiPo2ZiWMm6PpY6V8soWsysK5/3RYJ4Sl3DOm+/XfuvbiBQCF9M3CZuZ0 as7UCS3RR7eJZyF9exMEcz21gqiYhnyf1e71VRjg4HTm9cFhRlEQ5I1h5GdlywGa ut3d1pYSC/KUk2NjJAbMJ0/cSRDPTsTsEeA4ENXo0mWEXGLGEG6/pkD0hhj7Koix BeRimwLTiVSJ+zEJiEDt2eDek5jPtDv3zWtJ2kJU1Rgm/ZKS6N4uJqn3/DkJfp5C 3197LkSpFJBysIEOc1TGlDkC3nVSGPQQYiQ4vd4DH75Pt5PrElfIqMovCMk22pmB XaN2EfONcda3/hjKaUIPRcIz+1Cfz4I0T7gJgZhEvQpWJOtM7I/Co4bdDMMgSyI6 syC7xnwTSIuyU+ayTtt3 =aNwD -----END PGP SIGNATURE----- --6Vw0j8UKbyX0bfpA--