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

[Jianjun Duan]

On 04/21/2016 09:25 PM, David Gibson wrote:
> On Thu, Apr 21, 2016 at 10:03:56AM -0700, Jianjun Duan wrote:
>>
>>
>> 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 <address@hidden>
>>> 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.
> 
> Hm, ok.  The next question, though, is what's the absolute minimum of
> state we can transfer.  The indicator states are described by PAPR,
> but the configured and awaiting_release states are internal.  Not
> thinking carefully about the data model of what we migrate is a recipe
> for future migration problems.
> 
>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. DRC is the
mechanism used to implement hotplugging, its state need be transferred.
The minimum state we need to transfer should cover all the pieces
changed by hotplugging.
>>>> ---
>>>>  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;
>>
>