On Thu, Jun 02, 2016 at 05:28:45PM -0700, Jianjun Duan wrote: > > > 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 <du...@linux.vnet.ibm.com> > > > > 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. > > > > 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). > > > > 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. > > 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. > > > > > >> --- > >> 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(Object > >> *obj) > >> NULL, NULL, NULL, NULL); > >> } > >> > >> +static bool spapr_drc_needed(void *opaque) > >> +{ > >> + sPAPRDRConnector *drc = (sPAPRDRConnector *)opaque; > >> + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); > >> + bool rc = 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 != 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 = !((drc->isolation_state == > >> SPAPR_DR_ISOLATION_STATE_UNISOLATED) && > >> + (drc->allocation_state == > >> SPAPR_DR_ALLOCATION_STATE_USABLE) && > >> + drc->configured && drc->signalled && > >> !drc->awaiting_release); > >> + break; > >> + /* for LMB type */ > >> + case SPAPR_DR_CONNECTOR_TYPE_LMB: > >> + rc = !((drc->isolation_state == > >> SPAPR_DR_ISOLATION_STATE_ISOLATED) && > >> + (drc->allocation_state == > >> SPAPR_DR_ALLOCATION_STATE_UNUSABLE) && > >> + drc->configured && drc->signalled && > >> !drc->awaiting_release); > >> + 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 = detach_cb; > >> +} > >> + > >> +static const VMStateDescription vmstate_spapr_drc = { > >> + .name = "spapr_drc", > >> + .version_id = 1, > >> + .minimum_version_id = 1, > >> + .needed = spapr_drc_needed, > >> + .fields = (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 = DEVICE_CLASS(k); > >> @@ -625,6 +684,7 @@ static void spapr_dr_connector_class_init(ObjectClass > >> *k, void *data) > >> dk->reset = reset; > >> dk->realize = realize; > >> dk->unrealize = unrealize; > >> + dk->vmsd = &vmstate_spapr_drc; > >> drck->set_isolation_state = set_isolation_state; > >> drck->set_indicator_state = set_indicator_state; > >> drck->set_allocation_state = set_allocation_state; > >> @@ -638,6 +698,7 @@ static void spapr_dr_connector_class_init(ObjectClass > >> *k, void *data) > >> drck->detach = detach; > >> drck->release_pending = release_pending; > >> drck->set_signalled = set_signalled; > >> + drck->postmigrate_set_detach_cb = 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) > >> } > >> } > >> > >> +/* > >> + * 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 = opaque; > >> + sPAPRDRConnector *drc = spapr_phb_get_pci_drc(sphb, pdev); > >> + sPAPRDRConnectorClass *drck = 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 = opaque; > >> gpointer key, value; > >> int i; > >> + PCIBus *bus = PCI_HOST_BRIDGE(sphb)->bus; > >> + 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); > >> + } > >> > >> for (i = 0; i < sphb->msi_devs_num; ++i) { > >> key = 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; > >> > >> sPAPRDRConnector *spapr_dr_connector_new(Object *owner, > > > Thanks, > Jianjun > -- 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
signature.asc
Description: PGP signature