Re: [Qemu-devel] [QEMU RFC PATCH v3 1/6] Migration: Defined VMStateDescription struct for spapr_drc

[Jianjun Duan <duanj@linux.vnet.ibm.com>]

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 <duanj@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.
> 
> 
>> ---
>>  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