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

[David Gibson]

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.

> >>---
> >>  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;
> 

-- 
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