Re: [Qemu-devel] [QEMU PATCH v5 5/6] migration: spapr: migrate ccs_list in spapr state

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3843 bytes --]

On Mon, Oct 03, 2016 at 11:24:56AM -0700, Jianjun Duan wrote:
> ccs_list in spapr state maintains the device tree related
> information on the rtas side for hotplugged devices. In racing
> situations between hotplug events and migration operation, a rtas
> hotplug event could be migrated from the source guest to target
> guest, or the source guest could have not yet finished fetching
> the device tree when migration is started, the target will try
> to finish fetching the device tree. By migrating ccs_list, the
> target can fetch the device tree properly.
> 
> ccs_list is put in a subsection in the spapr state VMSD to make
> sure migration across different versions is not broken.
> 
> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>

I'm still not entirely convinced we need to migrate the ccs_list.
What would happen if we did this:

   * Keep a flag which indicates whether the guest is in the middle of
     the configure_connector process.
       - I'm not sure if that would need to be a new bit of state, or
         if we could deduce it from the value of the isolation and
	 allocation states
       - If it's new state, we'd need to migrate it, obviously not if
         we can derive it from other state flags

   * On the destination during post_load, if there was an in-progress
     configure_connector on the source, we set another "stale
     configure" flag

   * When a configure_connector call is attempted on the destination
     with the stale configure flag set, return an error

The question is, if we choose the right error, can we get the guest to
either restart the configure from scratch, or fail gracefully, so the
operator can restart the hotplug

> ---
>  hw/ppc/spapr.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 63b6a0d..1847d35 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1255,6 +1255,36 @@ static bool version_before_3(void *opaque, int version_id)
>      return version_id < 3;
>  }
>  
> +static bool spapr_ccs_list_needed(void *opaque)
> +{
> +    sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> +    return !QTAILQ_EMPTY(&spapr->ccs_list);
> +}
> +
> +static const VMStateDescription vmstate_spapr_ccs = {
> +    .name = "spaprconfigureconnectorstate",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(drc_index, sPAPRConfigureConnectorState),
> +        VMSTATE_INT32(fdt_offset, sPAPRConfigureConnectorState),
> +        VMSTATE_INT32(fdt_depth, sPAPRConfigureConnectorState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static const VMStateDescription vmstate_spapr_ccs_list = {
> +    .name = "spaprccslist",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = spapr_ccs_list_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_QTAILQ_V(ccs_list, sPAPRMachineState, 1,
> +                         vmstate_spapr_ccs, sPAPRConfigureConnectorState, next),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>  static const VMStateDescription vmstate_spapr = {
>      .name = "spapr",
>      .version_id = 3,
> @@ -1270,6 +1300,10 @@ static const VMStateDescription vmstate_spapr = {
>          VMSTATE_PPC_TIMEBASE_V(tb, sPAPRMachineState, 2),
>          VMSTATE_END_OF_LIST()
>      },
> +    .subsections = (const VMStateDescription*[]) {
> +        &vmstate_spapr_ccs_list,
> +        NULL
> +    }
>  };
>  
>  static int htab_save_setup(QEMUFile *f, void *opaque)

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]