Re: [Qemu-devel] [PATCH 2/5] Migration: Defined VMStateDescription struct for spapr_drc

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

On 04/19/2016 09:32 PM, David Gibson wrote:
> On Fri, Apr 15, 2016 at 01:33:02PM -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 by guest actions or device add/remove operation
>> are migrated.
>>
>> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
> Urgh.  It would be really nice if we could avoid this and instead
> calculate these states from other information.  I hate migrating
> what's essentially transitory state if we can possibly avoid it - is
> there any way to defer or retry hotplug operations to make this
> unnececessary?
>
> Even if we have to migrate some state here, I'm a bit dubious about
> whether directly migrating the PAPR indicator states is the best way.
> It does have the advantage of having a spec, on the other hand the
> PAPR indicators are really weird and hard to understand the meaning
> of.
I don't think we can avoid this. I would think migrating the machine 
state is
actually a clean approach, as you said it does have PAPR spec.
>> ---
>>   hw/ppc/spapr_drc.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>> index 3173940..5f7a25f 100644
>> --- a/hw/ppc/spapr_drc.c
>> +++ b/hw/ppc/spapr_drc.c
>> @@ -610,6 +610,20 @@ static void spapr_dr_connector_instance_init(Object *obj)
>>                           NULL, NULL, NULL, NULL);
>>   }
>>   
>> +static const VMStateDescription vmstate_spapr_drc = {
>> +    .name = "spapr_drc",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .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_END_OF_LIST()
>> +    }
>> +};
>> +
>>   static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
>>   {
>>       DeviceClass *dk = DEVICE_CLASS(k);
>> @@ -618,6 +632,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;