diff mbox series

[08/12] libxc/restore: Support v3 streams, and cope with v2 compatibilty

Message ID 20191224151932.6304-9-andrew.cooper3@citrix.com (mailing list archive)
State Superseded
Headers show
Series Support CPUID/MSR data in migration streams | expand

Commit Message

Andrew Cooper Dec. 24, 2019, 3:19 p.m. UTC
Introduce a static_data_complete() hook which is called when a
STATIC_DATA_END record is found (v3) or inferred (v2).

Modify handle_page_data() and handle_x86_pv_p2m_frames() to infer the
position in v2 streams.

The implementation of x86_static_data_complete() needs to wait until
more plumbing is in place, to make a combined libxl/libxc change to
maintain (functional) bisectability.

No practical change to behaviour - this is all plumbing work.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 tools/libxc/xc_sr_common.h          | 19 +++++++++++++++
 tools/libxc/xc_sr_common_x86.c      |  7 ++++++
 tools/libxc/xc_sr_common_x86.h      |  5 ++++
 tools/libxc/xc_sr_restore.c         | 47 +++++++++++++++++++++++++++++++++++--
 tools/libxc/xc_sr_restore_x86_hvm.c |  1 +
 tools/libxc/xc_sr_restore_x86_pv.c  | 18 ++++++++++++++
 6 files changed, 95 insertions(+), 2 deletions(-)

Comments

Ian Jackson Jan. 14, 2020, 5:02 p.m. UTC | #1
Andrew Cooper writes ("[PATCH 08/12] libxc/restore: Support v3 streams, and cope with v2 compatibilty"):
> Introduce a static_data_complete() hook which is called when a
> STATIC_DATA_END record is found (v3) or inferred (v2).
> 
> Modify handle_page_data() and handle_x86_pv_p2m_frames() to infer the
> position in v2 streams.
> 
> The implementation of x86_static_data_complete() needs to wait until
> more plumbing is in place, to make a combined libxl/libxc change to
> maintain (functional) bisectability.
> 
> No practical change to behaviour - this is all plumbing work.

These parts are confusing to me:

> +    /*
> +     * This is a bit of a bodge, but it is less bad than duplicating
> +     * handle_page_data() between different architectures.
> +     */
> +#if defined(__i386__) || defined(__x86_64__)
> +    /* v2 compat.  Infer the position of STATIC_DATA_END. */
> +    if ( ctx->restore.format_version < 3 && !ctx->restore.seen_static_data_end )
> +    {
> +        rc = handle_static_data_end(ctx);
> +        if ( rc )
> +        {
> +            ERROR("Inferred STATIC_DATA_END record failed");
> +            goto err;
> +        }
> +        rc = -1;
> +    }
> +
> +    if ( !ctx->restore.seen_static_data_end )
> +    {
> +        ERROR("No STATIC_DATA_END seen");
> +        goto err;
> +    }
> +#endif

...

> +    /* v2 compat.  Infer the position of STATIC_DATA_END. */
> +    if ( ctx->restore.format_version < 3 && !ctx->restore.seen_static_data_end )
> +    {
> +        rc = handle_static_data_end(ctx);
> +        if ( rc )
> +        {
> +            ERROR("Inferred STATIC_DATA_END record failed");
> +            return rc;
> +        }
> +    }
> +
> +    if ( !ctx->restore.seen_static_data_end )
> +    {
> +        ERROR("No STATIC_DATA_END seen");
> +        return -1;
> +    }

Firstly, this code is remarkably similar.  Surely it should be
factored out into something like
  possible_implicit_static_data_end()
?

And secondly, I don't understand why the first part in
handle_page_data is arch-qualified.  Maybe this would make more sense
if I looked at the code in context rather than just the diff ?

Ian.
diff mbox series

Patch

diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h
index 0289c01e13..2e9a4bc587 100644
--- a/tools/libxc/xc_sr_common.h
+++ b/tools/libxc/xc_sr_common.h
@@ -153,6 +153,13 @@  struct xc_sr_restore_ops
     int (*process_record)(struct xc_sr_context *ctx, struct xc_sr_record *rec);
 
     /**
+     * Perform any actions required after the static data has arrived.  Called
+     * when the STATIC_DATA_COMPLETE record has been recieved (or inferred in
+     * v2-compatibility mode).
+     */
+    int (*static_data_complete)(struct xc_sr_context *ctx);
+
+    /**
      * Perform any actions required after the stream has been finished. Called
      * after the END record has been received.
      */
@@ -255,6 +262,12 @@  struct xc_sr_context
             /* Currently buffering records between a checkpoint */
             bool buffer_all_records;
 
+            /*
+             * Whether a STATIC_DATA_END record has been seen (or implied for
+             * v2 compatibility).
+             */
+            bool seen_static_data_end;
+
 /*
  * With Remus/COLO, we buffer the records sent by the primary at checkpoint,
  * in case the primary will fail, we can recover from the last
@@ -427,6 +440,12 @@  int read_record(struct xc_sr_context *ctx, int fd, struct xc_sr_record *rec);
 int populate_pfns(struct xc_sr_context *ctx, unsigned int count,
                   const xen_pfn_t *original_pfns, const uint32_t *types);
 
+/*
+ * Handle a STATIC_DATA_END record.  For a v2 compat stream, the position of
+ * this record is inferred from other records.
+ */
+int handle_static_data_end(struct xc_sr_context *ctx);
+
 #endif
 /*
  * Local variables:
diff --git a/tools/libxc/xc_sr_common_x86.c b/tools/libxc/xc_sr_common_x86.c
index 011684df97..083454d256 100644
--- a/tools/libxc/xc_sr_common_x86.c
+++ b/tools/libxc/xc_sr_common_x86.c
@@ -42,6 +42,13 @@  int handle_x86_tsc_info(struct xc_sr_context *ctx, struct xc_sr_record *rec)
     return 0;
 }
 
+int x86_static_data_complete(struct xc_sr_context *ctx)
+{
+    /* TODO - something useful. */
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxc/xc_sr_common_x86.h b/tools/libxc/xc_sr_common_x86.h
index ebc4355bd1..7c2d42efe8 100644
--- a/tools/libxc/xc_sr_common_x86.h
+++ b/tools/libxc/xc_sr_common_x86.h
@@ -14,6 +14,11 @@  int write_x86_tsc_info(struct xc_sr_context *ctx);
  */
 int handle_x86_tsc_info(struct xc_sr_context *ctx, struct xc_sr_record *rec);
 
+/*
+ * Perform common x86 actions required after the static data has arrived.
+ */
+int x86_static_data_complete(struct xc_sr_context *ctx);
+
 #endif
 /*
  * Local variables:
diff --git a/tools/libxc/xc_sr_restore.c b/tools/libxc/xc_sr_restore.c
index 0280e55d4b..d4bd60a31e 100644
--- a/tools/libxc/xc_sr_restore.c
+++ b/tools/libxc/xc_sr_restore.c
@@ -35,9 +35,9 @@  static int read_headers(struct xc_sr_context *ctx)
         return -1;
     }
 
-    if ( ihdr.version != 2 )
+    if ( ihdr.version < 2 || ihdr.version > 3 )
     {
-        ERROR("Invalid Version: Expected 2, Got %d",
+        ERROR("Invalid Version: Expected 2 <= ver <= 3, Got %d",
               ihdr.version);
         return -1;
     }
@@ -342,6 +342,30 @@  static int handle_page_data(struct xc_sr_context *ctx, struct xc_sr_record *rec)
     xen_pfn_t *pfns = NULL, pfn;
     uint32_t *types = NULL, type;
 
+    /*
+     * This is a bit of a bodge, but it is less bad than duplicating
+     * handle_page_data() between different architectures.
+     */
+#if defined(__i386__) || defined(__x86_64__)
+    /* v2 compat.  Infer the position of STATIC_DATA_END. */
+    if ( ctx->restore.format_version < 3 && !ctx->restore.seen_static_data_end )
+    {
+        rc = handle_static_data_end(ctx);
+        if ( rc )
+        {
+            ERROR("Inferred STATIC_DATA_END record failed");
+            goto err;
+        }
+        rc = -1;
+    }
+
+    if ( !ctx->restore.seen_static_data_end )
+    {
+        ERROR("No STATIC_DATA_END seen");
+        goto err;
+    }
+#endif
+
     if ( rec->length < sizeof(*pages) )
     {
         ERROR("PAGE_DATA record truncated: length %u, min %zu",
@@ -631,6 +655,21 @@  static int buffer_record(struct xc_sr_context *ctx, struct xc_sr_record *rec)
     return 0;
 }
 
+int handle_static_data_end(struct xc_sr_context *ctx)
+{
+    xc_interface *xch = ctx->xch;
+
+    if ( ctx->restore.seen_static_data_end )
+    {
+        ERROR("Multiple STATIC_DATA_END records found");
+        return -1;
+    }
+
+    ctx->restore.seen_static_data_end = true;
+
+    return ctx->restore.ops.static_data_complete(ctx);
+}
+
 static int process_record(struct xc_sr_context *ctx, struct xc_sr_record *rec)
 {
     xc_interface *xch = ctx->xch;
@@ -654,6 +693,10 @@  static int process_record(struct xc_sr_context *ctx, struct xc_sr_record *rec)
         rc = handle_checkpoint(ctx);
         break;
 
+    case REC_TYPE_STATIC_DATA_END:
+        rc = handle_static_data_end(ctx);
+        break;
+
     default:
         rc = ctx->restore.ops.process_record(ctx, rec);
         break;
diff --git a/tools/libxc/xc_sr_restore_x86_hvm.c b/tools/libxc/xc_sr_restore_x86_hvm.c
index 3f78248f32..94f47f2589 100644
--- a/tools/libxc/xc_sr_restore_x86_hvm.c
+++ b/tools/libxc/xc_sr_restore_x86_hvm.c
@@ -239,6 +239,7 @@  struct xc_sr_restore_ops restore_ops_x86_hvm =
     .localise_page   = x86_hvm_localise_page,
     .setup           = x86_hvm_setup,
     .process_record  = x86_hvm_process_record,
+    .static_data_complete = x86_static_data_complete,
     .stream_complete = x86_hvm_stream_complete,
     .cleanup         = x86_hvm_cleanup,
 };
diff --git a/tools/libxc/xc_sr_restore_x86_pv.c b/tools/libxc/xc_sr_restore_x86_pv.c
index 8f61a5e8b9..90b1e5427b 100644
--- a/tools/libxc/xc_sr_restore_x86_pv.c
+++ b/tools/libxc/xc_sr_restore_x86_pv.c
@@ -679,6 +679,23 @@  static int handle_x86_pv_p2m_frames(struct xc_sr_context *ctx,
     unsigned int start, end, x, fpp = PAGE_SIZE / ctx->x86.pv.width;
     int rc;
 
+    /* v2 compat.  Infer the position of STATIC_DATA_END. */
+    if ( ctx->restore.format_version < 3 && !ctx->restore.seen_static_data_end )
+    {
+        rc = handle_static_data_end(ctx);
+        if ( rc )
+        {
+            ERROR("Inferred STATIC_DATA_END record failed");
+            return rc;
+        }
+    }
+
+    if ( !ctx->restore.seen_static_data_end )
+    {
+        ERROR("No STATIC_DATA_END seen");
+        return -1;
+    }
+
     if ( !ctx->x86.pv.restore.seen_pv_info )
     {
         ERROR("Not yet received X86_PV_INFO record");
@@ -1168,6 +1185,7 @@  struct xc_sr_restore_ops restore_ops_x86_pv =
     .localise_page   = x86_pv_localise_page,
     .setup           = x86_pv_setup,
     .process_record  = x86_pv_process_record,
+    .static_data_complete = x86_static_data_complete,
     .stream_complete = x86_pv_stream_complete,
     .cleanup         = x86_pv_cleanup,
 };