Re: [PATCH v9 8/8] tools/libxc: add DOMAIN_CONTEXT records to the migration stream...

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/09/2020 14:10, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@xxxxxxxxxx>
>
> ... and bump the version.
>
> This patch implements version 4 of the migration stream by adding the code
> necessary to save and restore DOMAIN_CONTEXT records, and removing the code
> to save the SHARED_INFO and TSC_INFO records (as these are deprecated in
> version 4).
>
> Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>

This really needs to be at least 3 patches.

First to adjust tools/python/scripts/verify-stream-v2 to understand the
new changes in the stream.

My testing tends to include running the script over the result of `xl
save`, and using a script in place of libxl-save-helper which tee's the
stream through the script, which lets you test in-line migrate.  (I
wonder if it would be better to add a pass-through mode to the script
and give libxl a way of running it in the same way as it currently runs
covert-legacy-stream.)

Next, a patch updating the receive side only to understand the new
changes in the stream.  In particular, this makes it far easier to
confirm that backwards compatibility is maintained.

Finally, a patch updating the sending side, if applicable.  (I've got an
alternative suggestion to avoid burning a load of major version numbers,
but will follow up on a different patch with that).

> ---
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Wei Liu <wl@xxxxxxx>
>
> v7:
>  - New in v7
> ---
>  tools/libs/guest/xg_sr_common.h       |  3 ++
>  tools/libs/guest/xg_sr_common_x86.c   | 20 -----------
>  tools/libs/guest/xg_sr_common_x86.h   |  6 ----
>  tools/libs/guest/xg_sr_restore.c      | 45 +++++++++++++++++++++--
>  tools/libs/guest/xg_sr_save.c         | 52 ++++++++++++++++++++++++++-
>  tools/libs/guest/xg_sr_save_x86_hvm.c |  5 ---
>  tools/libs/guest/xg_sr_save_x86_pv.c  | 22 ------------
>  7 files changed, 97 insertions(+), 56 deletions(-)
>
> diff --git a/tools/libs/guest/xg_sr_common.h b/tools/libs/guest/xg_sr_common.h
> index 13fcc47420..d440281cc1 100644
> --- a/tools/libs/guest/xg_sr_common.h
> +++ b/tools/libs/guest/xg_sr_common.h
> @@ -298,6 +298,9 @@ struct xc_sr_context
>  
>              /* Sender has invoked verify mode on the stream. */
>              bool verify;
> +
> +            /* Domain context blob. */
> +            struct xc_sr_blob context;

We already have

ctx->x86.hvm.restore.context

and are now gaining

ctx->restore.context

This is concerning close to being ambiguous.  How about dom_context ?

Also, you leak the memory allocation.  Free it in xg_sr_restore.c:cleanup().

> diff --git a/tools/libs/guest/xg_sr_restore.c 
> b/tools/libs/guest/xg_sr_restore.c
> index b57a787519..453a383ba4 100644
> --- a/tools/libs/guest/xg_sr_restore.c
> +++ b/tools/libs/guest/xg_sr_restore.c
> @@ -529,6 +529,20 @@ static int send_checkpoint_dirty_pfn_list(struct 
> xc_sr_context *ctx)
>      return rc;
>  }
>  
> +static int stream_complete(struct xc_sr_context *ctx)
> +{
> +    xc_interface *xch = ctx->xch;
> +    int rc;
> +
> +    rc = xc_domain_setcontext(xch, ctx->domid,
> +                              ctx->restore.context.ptr,
> +                              ctx->restore.context.size);
> +    if ( rc < 0 )
> +        PERROR("Unable to restore domain context");
> +
> +    return rc;
> +}

Please put this in the PV and HVM stream_complete() hooks.

This is somewhat of a layering violation and enforcing an order which
might not be appropriate in all cases.

~Andrew