[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [Qemu-devel] [QEMU RFC PATCH v3 1/6] Migration: Defined V
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [Qemu-devel] [QEMU RFC PATCH v3 1/6] Migration: Defined VMStateDescription struct for spapr_drc |
Date: |
Fri, 3 Jun 2016 11:48:14 +1000 |
User-agent: |
Mutt/1.6.1 (2016-04-27) |
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 <address@hidden>
> >
> > 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