[PATCH v2 0/6] Misc patches to aid migration v2 Remus support

[PATCH v2 0/6] Misc patches to aid migration v2 Remus support

[Andrew Cooper]

See individual patches for details.

Git tree available at:
 git://xenbits.xen.org/people/andrewcoop/xen.git remus-migv2-v2

~Andrew

[PATCH v2 1/6] xen/hvm: Permit HVM_PARAM_IDENT_PT to be set more than once

[Andrew Cooper]

There is no conceptual problem with setting this parameter more than once.
Checkpointed migration streams will typically set it once per checkpoint to
the same value.

The parameter is only actually needed on early-generation VT-x which lacked
the unrestricted guest capability, although it could plausibly be used on
newer VT-x with unusual execution control settings.  Short circuit the
expensive operations on non VT-x hardware.

The parameter itself must always be latched to avoid issues if the VM
eventually migrates to a host which needs to use the pagetable.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Yang Hongyang <yanghy@cn.fujitsu.com>

---
v2: Don't expose new value prematurely to VT-x
---
 xen/arch/x86/hvm/hvm.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 8140a27..85dfa52 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5730,13 +5730,15 @@ static int hvmop_set_param(
             rc = -EINVAL;
         break;
     case HVM_PARAM_IDENT_PT:
-        rc = -EINVAL;
-        if ( d->arch.hvm_domain.params[a.index] != 0 )
-            break;
-
-        rc = 0;
-        if ( !paging_mode_hap(d) )
+        /*
+         * Only actually required for VT-x lacking unrestricted_guest
+         * capabilities.  Short circuit the pause if possible.
+         */
+        if ( !paging_mode_hap(d) || !cpu_has_vmx )
+        {
+            d->arch.hvm_domain.params[a.index] = a.value;
             break;
+        }
 
         /*
          * Update GUEST_CR3 in each VMCS to point at identity map.
-- 
1.7.10.4

[PATCH v2 2/6] libxc/restore: Bail if unknown options are found

[Andrew Cooper]

When restoring a domain, check for unknown options in Image Header.  Nothing
good will come from attempting to continue.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: David Vrabel <david.vrabel@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Yang Hongyang <yanghy@cn.fujitsu.com>

---
David: The wording of the spec disallows even adding new options without
bumping the protocol version.  Do we want to relax the restriction slightly?
---
 docs/specs/libxc-migration-stream.pandoc |    5 +++--
 tools/libxc/xc_sr_restore.c              |    6 ++++++
 tools/libxc/xc_sr_stream_format.h        |    2 ++
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/docs/specs/libxc-migration-stream.pandoc b/docs/specs/libxc-migration-stream.pandoc
index 520240f..fa501e7 100644
--- a/docs/specs/libxc-migration-stream.pandoc
+++ b/docs/specs/libxc-migration-stream.pandoc
@@ -131,11 +131,12 @@ version     0x00000002.  The version of this specification.
 
 options     bit 0: Endianness.  0 = little-endian, 1 = big-endian.
 
-            bit 1-15: Reserved.
+            bit 1-15: Reserved. (Must be zero)
 --------------------------------------------------------------------
 
 The endianness shall be 0 (little-endian) for images generated on an
-i386, x86_64, or arm host.
+i386, x86_64, or arm host.  The receiving side should confirm that no
+unexpected options have been specified.
 
 \clearpage
 
diff --git a/tools/libxc/xc_sr_restore.c b/tools/libxc/xc_sr_restore.c
index 0bf4bae..7d65a29 100644
--- a/tools/libxc/xc_sr_restore.c
+++ b/tools/libxc/xc_sr_restore.c
@@ -37,6 +37,12 @@ static int read_headers(struct xc_sr_context *ctx)
               ihdr.version, IHDR_VERSION);
         return -1;
     }
+    else if ( ihdr.options & IHDR_OPT_RSVD_MASK )
+    {
+        ERROR("Unknown options in Image Header: 0x%04x",
+              ihdr.options & IHDR_OPT_RSVD_MASK);
+        return -1;
+    }
     else if ( ihdr.options & IHDR_OPT_BIG_ENDIAN )
     {
         ERROR("Unable to handle big endian streams");
diff --git a/tools/libxc/xc_sr_stream_format.h b/tools/libxc/xc_sr_stream_format.h
index d116ca6..9d8c128 100644
--- a/tools/libxc/xc_sr_stream_format.h
+++ b/tools/libxc/xc_sr_stream_format.h
@@ -29,6 +29,8 @@ struct xc_sr_ihdr
 #define IHDR_OPT_LITTLE_ENDIAN (0 << _IHDR_OPT_ENDIAN)
 #define IHDR_OPT_BIG_ENDIAN    (1 << _IHDR_OPT_ENDIAN)
 
+#define IHDR_OPT_RSVD_MASK     (~(IHDR_OPT_BIG_ENDIAN))
+
 /*
  * Domain Header
  */
-- 
1.7.10.4

[PATCH v2 3/6] libxc/migration: Be rather stricter with illformed callers

[Andrew Cooper]

The migration code itself should be able to validly assume all mandatory
callbacks are set up.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Yang Hongyang <yanghy@cn.fujitsu.com>
---
 tools/libxc/xc_sr_save.c         |    4 ++++
 tools/libxc/xc_sr_save_x86_hvm.c |    7 -------
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
index 5d9c267..83f0591 100644
--- a/tools/libxc/xc_sr_save.c
+++ b/tools/libxc/xc_sr_save.c
@@ -738,6 +738,10 @@ int xc_domain_save2(xc_interface *xch, int io_fd, uint32_t dom,
     ctx.save.max_iterations = 5;
     ctx.save.dirty_threshold = 50;
 
+    /* Sanity checks for callbacks. */
+    if ( hvm )
+        assert(callbacks->switch_qemu_logdirty);
+
     IPRINTF("In experimental %s", __func__);
     DPRINTF("fd %d, dom %u, max_iters %u, max_factor %u, flags %u, hvm %d",
             io_fd, dom, max_iters, max_factor, flags, hvm);
diff --git a/tools/libxc/xc_sr_save_x86_hvm.c b/tools/libxc/xc_sr_save_x86_hvm.c
index 8baa104..58efdb9 100644
--- a/tools/libxc/xc_sr_save_x86_hvm.c
+++ b/tools/libxc/xc_sr_save_x86_hvm.c
@@ -166,13 +166,6 @@ static int x86_hvm_setup(struct xc_sr_context *ctx)
 {
     xc_interface *xch = ctx->xch;
 
-    if ( !ctx->save.callbacks->switch_qemu_logdirty )
-    {
-        ERROR("No switch_qemu_logdirty callback provided");
-        errno = EINVAL;
-        return -1;
-    }
-
     if ( ctx->save.callbacks->switch_qemu_logdirty(
              ctx->domid, 1, ctx->save.callbacks->data) )
     {
-- 
1.7.10.4

[PATCH v2 4/6] libxc/save: Adjust stream-position callbacks for checkpointed streams

[Andrew Cooper]

There are some records which should only be sent once in the stream, and not
repeated for each checkpoint.  {start,end}_of_stream() become per-checkpoint,
and a new start_of_stream() is introduced.

There is no resulting change record order, but the X86_PV_INFO and
X86_PV_P2M_FRAMES records are positively identified as once per stream, rather
than once per checkpoint.

In addition, a few minor adjustments of comments and layout.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Yang Hongyang <yanghy@cn.fujitsu.com>

---
v2: Drop end_of_stream().  In hindsight it was silly.
---
 tools/libxc/xc_sr_common.h       |   21 +++++++++++++--------
 tools/libxc/xc_sr_save.c         |   10 +++++++---
 tools/libxc/xc_sr_save_x86_hvm.c |   23 +++++++++++++++--------
 tools/libxc/xc_sr_save_x86_pv.c  |   29 +++++++++++++++++++----------
 4 files changed, 54 insertions(+), 29 deletions(-)

diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h
index ef42412..c4fe92c 100644
--- a/tools/libxc/xc_sr_common.h
+++ b/tools/libxc/xc_sr_common.h
@@ -61,19 +61,24 @@ struct xc_sr_save_ops
     int (*setup)(struct xc_sr_context *ctx);
 
     /**
-     * Write records which need to be at the start of the stream.  This is
-     * called after the Image and Domain headers are written.  (Any records
-     * which need to be ahead of the memory.)
+     * Send records which need to be at the start of the stream.  This is
+     * called once, after the Image and Domain headers are written.
      */
     int (*start_of_stream)(struct xc_sr_context *ctx);
 
     /**
-     * Write records which need to be at the end of the stream, following the
-     * complete memory contents.  The caller shall handle writing the END
-     * record into the stream.  (Any records which need to be after the memory
-     * is complete.)
+     * Send records which need to be at the start of a checkpoint.  This is
+     * called once, or once per checkpoint in a checkpointed stream, and is
+     * ahead of memory data.
      */
-    int (*end_of_stream)(struct xc_sr_context *ctx);
+    int (*start_of_checkpoint)(struct xc_sr_context *ctx);
+
+    /**
+     * Send records which need to be at the end of the checkpoint.  This is
+     * called once, or once per checkpoint in a checkpointed stream, and is
+     * after the memory data.
+     */
+    int (*end_of_checkpoint)(struct xc_sr_context *ctx);
 
     /**
      * Clean up the local environment.  Will be called exactly once, either
diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
index 83f0591..66fcd3e 100644
--- a/tools/libxc/xc_sr_save.c
+++ b/tools/libxc/xc_sr_save.c
@@ -662,6 +662,10 @@ static int save(struct xc_sr_context *ctx, uint16_t guest_type)
     if ( rc )
         goto err;
 
+    rc = ctx->save.ops.start_of_checkpoint(ctx);
+    if ( rc )
+        goto err;
+
     if ( ctx->save.live )
         rc = send_domain_memory_live(ctx);
     else
@@ -678,12 +682,12 @@ static int save(struct xc_sr_context *ctx, uint16_t guest_type)
         goto err;
     }
 
-    xc_report_progress_single(xch, "End of stream");
-
-    rc = ctx->save.ops.end_of_stream(ctx);
+    rc = ctx->save.ops.end_of_checkpoint(ctx);
     if ( rc )
         goto err;
 
+    xc_report_progress_single(xch, "End of stream");
+
     rc = write_end_record(ctx);
     if ( rc )
         goto err;
diff --git a/tools/libxc/xc_sr_save_x86_hvm.c b/tools/libxc/xc_sr_save_x86_hvm.c
index 58efdb9..f4604db 100644
--- a/tools/libxc/xc_sr_save_x86_hvm.c
+++ b/tools/libxc/xc_sr_save_x86_hvm.c
@@ -184,7 +184,13 @@ static int x86_hvm_start_of_stream(struct xc_sr_context *ctx)
     return 0;
 }
 
-static int x86_hvm_end_of_stream(struct xc_sr_context *ctx)
+static int x86_hvm_start_of_checkpoint(struct xc_sr_context *ctx)
+{
+    /* no-op */
+    return 0;
+}
+
+static int x86_hvm_end_of_checkpoint(struct xc_sr_context *ctx)
 {
     int rc;
 
@@ -209,7 +215,7 @@ static int x86_hvm_end_of_stream(struct xc_sr_context *ctx)
     if ( rc )
         return rc;
 
-    return rc;
+    return 0;
 }
 
 static int x86_hvm_cleanup(struct xc_sr_context *ctx)
@@ -230,12 +236,13 @@ static int x86_hvm_cleanup(struct xc_sr_context *ctx)
 
 struct xc_sr_save_ops save_ops_x86_hvm =
 {
-    .pfn_to_gfn      = x86_hvm_pfn_to_gfn,
-    .normalise_page  = x86_hvm_normalise_page,
-    .setup           = x86_hvm_setup,
-    .start_of_stream = x86_hvm_start_of_stream,
-    .end_of_stream   = x86_hvm_end_of_stream,
-    .cleanup         = x86_hvm_cleanup,
+    .pfn_to_gfn          = x86_hvm_pfn_to_gfn,
+    .normalise_page      = x86_hvm_normalise_page,
+    .setup               = x86_hvm_setup,
+    .start_of_stream     = x86_hvm_start_of_stream,
+    .start_of_checkpoint = x86_hvm_start_of_checkpoint,
+    .end_of_checkpoint   = x86_hvm_end_of_checkpoint,
+    .cleanup             = x86_hvm_cleanup,
 };
 
 /*
diff --git a/tools/libxc/xc_sr_save_x86_pv.c b/tools/libxc/xc_sr_save_x86_pv.c
index a668221..f63f40b 100644
--- a/tools/libxc/xc_sr_save_x86_pv.c
+++ b/tools/libxc/xc_sr_save_x86_pv.c
@@ -816,6 +816,12 @@ static int x86_pv_start_of_stream(struct xc_sr_context *ctx)
     if ( rc )
         return rc;
 
+    /*
+     * Ideally should be able to change during migration.  Currently
+     * corruption will occur if the contents or location of the P2M changes
+     * during the live migration loop.  If one is very lucky, the breakage
+     * will not be subtle.
+     */
     rc = write_x86_pv_p2m_frames(ctx);
     if ( rc )
         return rc;
@@ -823,10 +829,12 @@ static int x86_pv_start_of_stream(struct xc_sr_context *ctx)
     return 0;
 }
 
-/*
- * save_ops function.  Writes tail records information into the stream.
- */
-static int x86_pv_end_of_stream(struct xc_sr_context *ctx)
+static int x86_pv_start_of_checkpoint(struct xc_sr_context *ctx)
+{
+    return 0;
+}
+
+static int x86_pv_end_of_checkpoint(struct xc_sr_context *ctx)
 {
     int rc;
 
@@ -866,12 +874,13 @@ static int x86_pv_cleanup(struct xc_sr_context *ctx)
 
 struct xc_sr_save_ops save_ops_x86_pv =
 {
-    .pfn_to_gfn      = x86_pv_pfn_to_gfn,
-    .normalise_page  = x86_pv_normalise_page,
-    .setup           = x86_pv_setup,
-    .start_of_stream = x86_pv_start_of_stream,
-    .end_of_stream   = x86_pv_end_of_stream,
-    .cleanup         = x86_pv_cleanup,
+    .pfn_to_gfn          = x86_pv_pfn_to_gfn,
+    .normalise_page      = x86_pv_normalise_page,
+    .setup               = x86_pv_setup,
+    .start_of_stream     = x86_pv_start_of_stream,
+    .start_of_checkpoint = x86_pv_start_of_checkpoint,
+    .end_of_checkpoint   = x86_pv_end_of_checkpoint,
+    .cleanup             = x86_pv_cleanup,
 };
 
 /*
-- 
1.7.10.4

[PATCH v2 5/6] tools/migration: Specification update for 'checkpointed' flag

[Andrew Cooper]

From: Yang Hongyang <yanghy@cn.fujitsu.com>

This allows different types of streams to be distinguished from the Image
Header alone, at the start of day.

In addition, update the save and restore code to:
 * Collect checkpointed-ness from their callers
 * Use the new stream flag
 * Sanity check their environment

Based on an eariler patch from Yang Hongyang

Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: David Vrabel <david.vrabel@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 docs/specs/libxc-migration-stream.pandoc |    5 ++++-
 tools/libxc/include/xenguest.h           |    1 +
 tools/libxc/xc_sr_common.h               |    6 ++++++
 tools/libxc/xc_sr_restore.c              |    9 +++++++++
 tools/libxc/xc_sr_save.c                 |    8 +++++++-
 tools/libxc/xc_sr_stream_format.h        |    6 +++++-
 tools/libxl/libxl_dom.c                  |    1 +
 7 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/docs/specs/libxc-migration-stream.pandoc b/docs/specs/libxc-migration-stream.pandoc
index fa501e7..031a1d5 100644
--- a/docs/specs/libxc-migration-stream.pandoc
+++ b/docs/specs/libxc-migration-stream.pandoc
@@ -131,7 +131,10 @@ version     0x00000002.  The version of this specification.
 
 options     bit 0: Endianness.  0 = little-endian, 1 = big-endian.
 
-            bit 1-15: Reserved. (Must be zero)
+            bit 1: Checkpointed.  0 = plain stream with single VM,
+                                  1 = stream with VM checkpoints.
+
+            bit 2-15: Reserved. (Must be zero)
 --------------------------------------------------------------------
 
 The endianness shall be 0 (little-endian) for images generated on an
diff --git a/tools/libxc/include/xenguest.h b/tools/libxc/include/xenguest.h
index 8e39075..7581263 100644
--- a/tools/libxc/include/xenguest.h
+++ b/tools/libxc/include/xenguest.h
@@ -30,6 +30,7 @@
 #define XCFLAGS_HVM       (1 << 2)
 #define XCFLAGS_STDVGA    (1 << 3)
 #define XCFLAGS_CHECKPOINT_COMPRESS    (1 << 4)
+#define XCFLAGS_CHECKPOINTED    (1 << 5)
 
 #define X86_64_B_SIZE   64 
 #define X86_32_B_SIZE   32
diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h
index c4fe92c..a16fe22 100644
--- a/tools/libxc/xc_sr_common.h
+++ b/tools/libxc/xc_sr_common.h
@@ -174,6 +174,9 @@ struct xc_sr_context
             /* Live migrate vs non live suspend. */
             bool live;
 
+            /* Plain VM, or checkpoints over time. */
+            bool checkpointed;
+
             /* Further debugging information in the stream. */
             bool debug;
 
@@ -197,6 +200,9 @@ struct xc_sr_context
             /* From Image Header. */
             uint32_t format_version;
 
+            /* Plain VM, or checkpoints over time. */
+            bool checkpointed;
+
             /* From Domain Header. */
             uint32_t guest_type;
             uint32_t guest_page_size;
diff --git a/tools/libxc/xc_sr_restore.c b/tools/libxc/xc_sr_restore.c
index 7d65a29..b815db9 100644
--- a/tools/libxc/xc_sr_restore.c
+++ b/tools/libxc/xc_sr_restore.c
@@ -48,6 +48,14 @@ static int read_headers(struct xc_sr_context *ctx)
         ERROR("Unable to handle big endian streams");
         return -1;
     }
+    else if ( (ihdr.options & IHDR_OPT_CHECKPOINTED) !=
+              ctx->restore.checkpointed )
+    {
+        ERROR("Caller (%c) and stream (%c) disagree over checkpointed-ness",
+              (ihdr.options & IHDR_OPT_CHECKPOINTED) ? 'y' : 'n',
+              ctx->restore.checkpointed ? 'y' : 'n');
+        return -1;
+    }
 
     ctx->restore.format_version = ihdr.version;
 
@@ -589,6 +597,7 @@ int xc_domain_restore2(xc_interface *xch, int io_fd, uint32_t dom,
     ctx.restore.console_domid = console_domid;
     ctx.restore.xenstore_evtchn = store_evtchn;
     ctx.restore.xenstore_domid = store_domid;
+    ctx.restore.checkpointed = checkpointed_stream;
     ctx.restore.callbacks = callbacks;
 
     IPRINTF("In experimental %s", __func__);
diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
index 66fcd3e..2730000 100644
--- a/tools/libxc/xc_sr_save.c
+++ b/tools/libxc/xc_sr_save.c
@@ -15,7 +15,10 @@ static int write_headers(struct xc_sr_context *ctx, uint16_t guest_type)
             .marker  = IHDR_MARKER,
             .id      = htonl(IHDR_ID),
             .version = htonl(IHDR_VERSION),
-            .options = htons(IHDR_OPT_LITTLE_ENDIAN),
+            .options = htons(
+                IHDR_OPT_LITTLE_ENDIAN |
+                (ctx->save.checkpointed ? IHDR_OPT_CHECKPOINTED : 0)
+                ),
         };
     struct xc_sr_dhdr dhdr =
         {
@@ -732,6 +735,7 @@ int xc_domain_save2(xc_interface *xch, int io_fd, uint32_t dom,
     ctx.save.callbacks = callbacks;
     ctx.save.live  = !!(flags & XCFLAGS_LIVE);
     ctx.save.debug = !!(flags & XCFLAGS_DEBUG);
+    ctx.save.checkpointed = !!(flags & XCFLAGS_CHECKPOINTED);
 
     /*
      * TODO: Find some time to better tweak the live migration algorithm.
@@ -745,6 +749,8 @@ int xc_domain_save2(xc_interface *xch, int io_fd, uint32_t dom,
     /* Sanity checks for callbacks. */
     if ( hvm )
         assert(callbacks->switch_qemu_logdirty);
+    if ( ctx.save.checkpointed )
+        assert(callbacks->checkpoint && callbacks->postcopy);
 
     IPRINTF("In experimental %s", __func__);
     DPRINTF("fd %d, dom %u, max_iters %u, max_factor %u, flags %u, hvm %d",
diff --git a/tools/libxc/xc_sr_stream_format.h b/tools/libxc/xc_sr_stream_format.h
index 9d8c128..c35dcbd 100644
--- a/tools/libxc/xc_sr_stream_format.h
+++ b/tools/libxc/xc_sr_stream_format.h
@@ -29,7 +29,11 @@ struct xc_sr_ihdr
 #define IHDR_OPT_LITTLE_ENDIAN (0 << _IHDR_OPT_ENDIAN)
 #define IHDR_OPT_BIG_ENDIAN    (1 << _IHDR_OPT_ENDIAN)
 
-#define IHDR_OPT_RSVD_MASK     (~(IHDR_OPT_BIG_ENDIAN))
+#define _IHDR_OPT_CHECKPOINTED 1
+#define IHDR_OPT_CHECKPOINTED  (1 << _IHDR_OPT_CHECKPOINTED)
+
+#define IHDR_OPT_RSVD_MASK     (~(IHDR_OPT_BIG_ENDIAN |     \
+                                  IHDR_OPT_CHECKPOINTED))
 
 /*
  * Domain Header
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index f408646..a0c9850 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -2003,6 +2003,7 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
 
     if (r_info != NULL) {
         dss->interval = r_info->interval;
+        dss->xcflags |= XCFLAGS_CHECKPOINTED;
         if (libxl_defbool_val(r_info->compression))
             dss->xcflags |= XCFLAGS_CHECKPOINT_COMPRESS;
     }
-- 
1.7.10.4

[PATCH v2 6/6] libxc/migration: Specification update for CHECKPOINT records

[Andrew Cooper]

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: David Vrabel <david.vrabel@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Yang Hongyang <yanghy@cn.fujitsu.com>
---
 docs/specs/libxc-migration-stream.pandoc |   23 ++++++++++++++++++++---
 tools/libxc/xc_sr_common.c               |    1 +
 tools/libxc/xc_sr_stream_format.h        |    1 +
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/docs/specs/libxc-migration-stream.pandoc b/docs/specs/libxc-migration-stream.pandoc
index 031a1d5..a1fcc2c 100644
--- a/docs/specs/libxc-migration-stream.pandoc
+++ b/docs/specs/libxc-migration-stream.pandoc
@@ -37,8 +37,6 @@ Not Yet Included
 The following features are not yet fully specified and will be
 included in a future draft.
 
-* Remus
-
 * Page data compression.
 
 * ARM
@@ -231,7 +229,9 @@ type         0x00000000: END
 
              0x0000000D: VERIFY
 
-             0x0000000E - 0x7FFFFFFF: Reserved for future _mandatory_
+             0x0000000E: CHECKPOINT
+
+             0x0000000F - 0x7FFFFFFF: Reserved for future _mandatory_
              records.
 
              0x80000000 - 0xFFFFFFFF: Reserved for future _optional_
@@ -582,6 +582,23 @@ The verify record contains no fields; its body_length is 0.
 
 \clearpage
 
+CHECKPOINT
+----------
+
+A checkpoint record indicates that the records thusfar in the stream represent
+a consistent view of VM state.
+
+     0     1     2     3     4     5     6     7 octet
+    +-------------------------------------------------+
+
+The checkpoint record contains no fields; its body_length is 0
+
+A stream containing checkpoint records must have indicated itself as a
+checkpointed stream in the Image Header.  Conversely, a stream not identified
+as checkpointed must not contain checkpoint records.
+
+\clearpage
+
 Layout
 ======
 
diff --git a/tools/libxc/xc_sr_common.c b/tools/libxc/xc_sr_common.c
index 59e0c5d..945cfa6 100644
--- a/tools/libxc/xc_sr_common.c
+++ b/tools/libxc/xc_sr_common.c
@@ -34,6 +34,7 @@ const char *dhdr_type_to_str(uint32_t type)
     [REC_TYPE_TOOLSTACK]            = "Toolstack",
     [REC_TYPE_X86_PV_VCPU_MSRS]     = "x86 PV vcpu msrs",
     [REC_TYPE_VERIFY]               = "Verify",
+    [REC_TYPE_CHECKPOINT]           = "Checkpoint",
 };
 
 const char *rec_type_to_str(uint32_t type)
diff --git a/tools/libxc/xc_sr_stream_format.h b/tools/libxc/xc_sr_stream_format.h
index c35dcbd..b7c3974 100644
--- a/tools/libxc/xc_sr_stream_format.h
+++ b/tools/libxc/xc_sr_stream_format.h
@@ -80,6 +80,7 @@ struct xc_sr_rhdr
 #define REC_TYPE_TOOLSTACK            0x0000000bU
 #define REC_TYPE_X86_PV_VCPU_MSRS     0x0000000cU
 #define REC_TYPE_VERIFY               0x0000000dU
+#define REC_TYPE_CHECKPOINT           0x0000000eU
 
 #define REC_TYPE_OPTIONAL             0x80000000U
 
-- 
1.7.10.4

Re: [PATCH v2 1/6] xen/hvm: Permit HVM_PARAM_IDENT_PT to be set more than once

[Hongyang Yang]

I've reviewed all of the series, looks good to me in general, I will rebase my
patches on top of this series. Would you like me to take this series and send
it together with mine?

On 05/09/2015 05:14 AM, Andrew Cooper wrote:
> There is no conceptual problem with setting this parameter more than once.
> Checkpointed migration streams will typically set it once per checkpoint to
> the same value.
>
> The parameter is only actually needed on early-generation VT-x which lacked
> the unrestricted guest capability, although it could plausibly be used on
> newer VT-x with unusual execution control settings.  Short circuit the
> expensive operations on non VT-x hardware.
>
> The parameter itself must always be latched to avoid issues if the VM
> eventually migrates to a host which needs to use the pagetable.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Yang Hongyang <yanghy@cn.fujitsu.com>
>
> ---
> v2: Don't expose new value prematurely to VT-x
> ---
>   xen/arch/x86/hvm/hvm.c |   14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 8140a27..85dfa52 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5730,13 +5730,15 @@ static int hvmop_set_param(
>               rc = -EINVAL;
>           break;
>       case HVM_PARAM_IDENT_PT:
> -        rc = -EINVAL;
> -        if ( d->arch.hvm_domain.params[a.index] != 0 )
> -            break;
> -
> -        rc = 0;
> -        if ( !paging_mode_hap(d) )
> +        /*
> +         * Only actually required for VT-x lacking unrestricted_guest
> +         * capabilities.  Short circuit the pause if possible.
> +         */
> +        if ( !paging_mode_hap(d) || !cpu_has_vmx )
> +        {
> +            d->arch.hvm_domain.params[a.index] = a.value;
>               break;
> +        }
>
>           /*
>            * Update GUEST_CR3 in each VMCS to point at identity map.
>

-- 
Thanks,
Yang.

Re: [PATCH v2 1/6] xen/hvm: Permit HVM_PARAM_IDENT_PT to be set more than once

[Andrew Cooper]

On 11/05/2015 08:51, Hongyang Yang wrote:
> I've reviewed all of the series, looks good to me in general, I will
> rebase my
> patches on top of this series. Would you like me to take this series
> and send
> it together with mine?

Yes please - for ease of review it would be far easier to maintain one
series rather than two.

(I do have some feedback from your other comments so far, which you
probably want to consider before sending a new version of the series)

~Andrew

Re: [PATCH v2 1/6] xen/hvm: Permit HVM_PARAM_IDENT_PT to be set more than once

[Hongyang Yang]

On 05/11/2015 04:00 PM, Andrew Cooper wrote:
> On 11/05/2015 08:51, Hongyang Yang wrote:
>> I've reviewed all of the series, looks good to me in general, I will
>> rebase my
>> patches on top of this series. Would you like me to take this series
>> and send
>> it together with mine?
>
> Yes please - for ease of review it would be far easier to maintain one
> series rather than two.
>
> (I do have some feedback from your other comments so far, which you
> probably want to consider before sending a new version of the series)

Ok

>
> ~Andrew
>

-- 
Thanks,
Yang.

Re: [PATCH v2 2/6] libxc/restore: Bail if unknown options are found

[David Vrabel]

On 08/05/15 22:14, Andrew Cooper wrote:
> When restoring a domain, check for unknown options in Image Header.  Nothing
> good will come from attempting to continue.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: David Vrabel <david.vrabel@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Yang Hongyang <yanghy@cn.fujitsu.com>
> 
> ---
> David: The wording of the spec disallows even adding new options without
> bumping the protocol version.  Do we want to relax the restriction slightly?
> ---
>  docs/specs/libxc-migration-stream.pandoc |    5 +++--
>  tools/libxc/xc_sr_restore.c              |    6 ++++++
>  tools/libxc/xc_sr_stream_format.h        |    2 ++
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/specs/libxc-migration-stream.pandoc b/docs/specs/libxc-migration-stream.pandoc
> index 520240f..fa501e7 100644
> --- a/docs/specs/libxc-migration-stream.pandoc
> +++ b/docs/specs/libxc-migration-stream.pandoc
> @@ -131,11 +131,12 @@ version     0x00000002.  The version of this specification.
>  
>  options     bit 0: Endianness.  0 = little-endian, 1 = big-endian.
>  
> -            bit 1-15: Reserved.
> +            bit 1-15: Reserved. (Must be zero)
>  --------------------------------------------------------------------
>  
>  The endianness shall be 0 (little-endian) for images generated on an
> -i386, x86_64, or arm host.
> +i386, x86_64, or arm host.  The receiving side should confirm that no
> +unexpected options have been specified.

The image header provides information about the format of the image.  I
suppose one could make an argument that an checkpointed stream is an
infinite stream vs finite, but I think a CHECKPOINT_ENABLE record might
be better?

David

Re: [PATCH v2 2/6] libxc/restore: Bail if unknown options are found

[David Vrabel]

On 08/05/15 22:14, Andrew Cooper wrote:
> When restoring a domain, check for unknown options in Image Header.  Nothing
> good will come from attempting to continue.

Section 2.3 (Fields) explicitly states that "Padding and reserved fields
are set to zero on save and must be ignored during restore".

I don't think there is a compelling reason to change this behaviour for
this particular field.

David

Re: [PATCH v2 2/6] libxc/restore: Bail if unknown options are found

[David Vrabel]

On 11/05/15 10:23, David Vrabel wrote:
> 
> The image header provides information about the format of the image.  I
> suppose one could make an argument that an checkpointed stream is an
> infinite stream vs finite, but I think a CHECKPOINT_ENABLE record might
> be better?

Sorry, this was supposed to be a reply to patch #5 which added the
"checkpointed" bit to the image header.

David

Re: [PATCH v2 3/6] libxc/migration: Be rather stricter with illformed callers

[Ian Campbell]

On Fri, 2015-05-08 at 22:14 +0100, Andrew Cooper wrote:
> The migration code itself should be able to validly assume all mandatory
> callbacks are set up.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Yang Hongyang <yanghy@cn.fujitsu.com>
> ---
>  tools/libxc/xc_sr_save.c         |    4 ++++
>  tools/libxc/xc_sr_save_x86_hvm.c |    7 -------
>  2 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
> index 5d9c267..83f0591 100644
> --- a/tools/libxc/xc_sr_save.c
> +++ b/tools/libxc/xc_sr_save.c
> @@ -738,6 +738,10 @@ int xc_domain_save2(xc_interface *xch, int io_fd, uint32_t dom,
>      ctx.save.max_iterations = 5;
>      ctx.save.dirty_threshold = 50;
>  
> +    /* Sanity checks for callbacks. */
> +    if ( hvm )
> +        assert(callbacks->switch_qemu_logdirty);

assert(!hvm || callbacks->switch_qemu_logdirty)

?

> +
>      IPRINTF("In experimental %s", __func__);
>      DPRINTF("fd %d, dom %u, max_iters %u, max_factor %u, flags %u, hvm %d",
>              io_fd, dom, max_iters, max_factor, flags, hvm);
> diff --git a/tools/libxc/xc_sr_save_x86_hvm.c b/tools/libxc/xc_sr_save_x86_hvm.c
> index 8baa104..58efdb9 100644
> --- a/tools/libxc/xc_sr_save_x86_hvm.c
> +++ b/tools/libxc/xc_sr_save_x86_hvm.c
> @@ -166,13 +166,6 @@ static int x86_hvm_setup(struct xc_sr_context *ctx)
>  {
>      xc_interface *xch = ctx->xch;
>  
> -    if ( !ctx->save.callbacks->switch_qemu_logdirty )
> -    {
> -        ERROR("No switch_qemu_logdirty callback provided");
> -        errno = EINVAL;
> -        return -1;
> -    }
> -
>      if ( ctx->save.callbacks->switch_qemu_logdirty(
>               ctx->domid, 1, ctx->save.callbacks->data) )
>      {

Re: [PATCH v2 4/6] libxc/save: Adjust stream-position callbacks for checkpointed streams

[Ian Campbell]

On Fri, 2015-05-08 at 22:14 +0100, Andrew Cooper wrote:
> There are some records which should only be sent once in the stream, and not
> repeated for each checkpoint.  {start,end}_of_stream() become per-checkpoint,
> and a new start_of_stream() is introduced.
> 
> There is no resulting change record order, but the X86_PV_INFO and
> X86_PV_P2M_FRAMES records are positively identified as once per stream, rather
> than once per checkpoint.

In the case of the latter judging from the comments this is currently an
implementation detail and in principal we want to be able to resend the
p2m frames as well?

[...]
> diff --git a/tools/libxc/xc_sr_save_x86_pv.c b/tools/libxc/xc_sr_save_x86_pv.c
> index a668221..f63f40b 100644
> --- a/tools/libxc/xc_sr_save_x86_pv.c
> +++ b/tools/libxc/xc_sr_save_x86_pv.c
> @@ -816,6 +816,12 @@ static int x86_pv_start_of_stream(struct xc_sr_context *ctx)
>      if ( rc )
>          return rc;
>  
> +    /*
> +     * Ideally should be able to change during migration.  Currently
> +     * corruption will occur if the contents or location of the P2M changes
> +     * during the live migration loop.  If one is very lucky, the breakage
> +     * will not be subtle.
> +     */
>      rc = write_x86_pv_p2m_frames(ctx);
>      if ( rc )
>          return rc;

This is the comment I'm basing the above on.

Ian.

Re: [PATCH v2 3/6] libxc/migration: Be rather stricter with illformed callers

[Andrew Cooper]

On 11/05/15 12:33, Ian Campbell wrote:
> On Fri, 2015-05-08 at 22:14 +0100, Andrew Cooper wrote:
>> The migration code itself should be able to validly assume all mandatory
>> callbacks are set up.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Yang Hongyang <yanghy@cn.fujitsu.com>
>> ---
>>  tools/libxc/xc_sr_save.c         |    4 ++++
>>  tools/libxc/xc_sr_save_x86_hvm.c |    7 -------
>>  2 files changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
>> index 5d9c267..83f0591 100644
>> --- a/tools/libxc/xc_sr_save.c
>> +++ b/tools/libxc/xc_sr_save.c
>> @@ -738,6 +738,10 @@ int xc_domain_save2(xc_interface *xch, int io_fd, uint32_t dom,
>>      ctx.save.max_iterations = 5;
>>      ctx.save.dirty_threshold = 50;
>>  
>> +    /* Sanity checks for callbacks. */
>> +    if ( hvm )
>> +        assert(callbacks->switch_qemu_logdirty);
> assert(!hvm || callbacks->switch_qemu_logdirty)
>
> ?

I would argue that the former is far easier to read.

The intention for this, and a subsequent patch, is "if (some option)
assert(mandatory callbacks for option)"

~Andrew

>
>> +
>>      IPRINTF("In experimental %s", __func__);
>>      DPRINTF("fd %d, dom %u, max_iters %u, max_factor %u, flags %u, hvm %d",
>>              io_fd, dom, max_iters, max_factor, flags, hvm);
>> diff --git a/tools/libxc/xc_sr_save_x86_hvm.c b/tools/libxc/xc_sr_save_x86_hvm.c
>> index 8baa104..58efdb9 100644
>> --- a/tools/libxc/xc_sr_save_x86_hvm.c
>> +++ b/tools/libxc/xc_sr_save_x86_hvm.c
>> @@ -166,13 +166,6 @@ static int x86_hvm_setup(struct xc_sr_context *ctx)
>>  {
>>      xc_interface *xch = ctx->xch;
>>  
>> -    if ( !ctx->save.callbacks->switch_qemu_logdirty )
>> -    {
>> -        ERROR("No switch_qemu_logdirty callback provided");
>> -        errno = EINVAL;
>> -        return -1;
>> -    }
>> -
>>      if ( ctx->save.callbacks->switch_qemu_logdirty(
>>               ctx->domid, 1, ctx->save.callbacks->data) )
>>      {
>

Re: [PATCH v2 4/6] libxc/save: Adjust stream-position callbacks for checkpointed streams

[Andrew Cooper]

On 11/05/15 12:36, Ian Campbell wrote:
> On Fri, 2015-05-08 at 22:14 +0100, Andrew Cooper wrote:
>> There are some records which should only be sent once in the stream, and not
>> repeated for each checkpoint.  {start,end}_of_stream() become per-checkpoint,
>> and a new start_of_stream() is introduced.
>>
>> There is no resulting change record order, but the X86_PV_INFO and
>> X86_PV_P2M_FRAMES records are positively identified as once per stream, rather
>> than once per checkpoint.
> In the case of the latter judging from the comments this is currently an
> implementation detail and in principal we want to be able to resend the
> p2m frames as well?

Correct, and the spec regarding X86_PV_P2M_FRAMES was explicitly
intended to allow partial updates in the future.

It is currently an implementation bug (or many) that the P2M can't
change during the live part of migration.

~Andrew

Re: [PATCH v2 3/6] libxc/migration: Be rather stricter with illformed callers

[Ian Campbell]

On Mon, 2015-05-11 at 12:47 +0100, Andrew Cooper wrote:
> On 11/05/15 12:33, Ian Campbell wrote:
> > On Fri, 2015-05-08 at 22:14 +0100, Andrew Cooper wrote:
> >> The migration code itself should be able to validly assume all mandatory
> >> callbacks are set up.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> CC: Ian Campbell <Ian.Campbell@citrix.com>
> >> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> >> CC: Wei Liu <wei.liu2@citrix.com>
> >> CC: Yang Hongyang <yanghy@cn.fujitsu.com>
> >> ---
> >>  tools/libxc/xc_sr_save.c         |    4 ++++
> >>  tools/libxc/xc_sr_save_x86_hvm.c |    7 -------
> >>  2 files changed, 4 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
> >> index 5d9c267..83f0591 100644
> >> --- a/tools/libxc/xc_sr_save.c
> >> +++ b/tools/libxc/xc_sr_save.c
> >> @@ -738,6 +738,10 @@ int xc_domain_save2(xc_interface *xch, int io_fd, uint32_t dom,
> >>      ctx.save.max_iterations = 5;
> >>      ctx.save.dirty_threshold = 50;
> >>  
> >> +    /* Sanity checks for callbacks. */
> >> +    if ( hvm )
> >> +        assert(callbacks->switch_qemu_logdirty);
> > assert(!hvm || callbacks->switch_qemu_logdirty)
> >
> > ?
> 
> I would argue that the former is far easier to read.

I read this as "assert that either the guest is not an  hvm guest or the
callback is provided", but fair enough.

> The intention for this, and a subsequent patch, is "if (some option)
> assert(mandatory callbacks for option)"
> 
> ~Andrew
> 
> >
> >> +
> >>      IPRINTF("In experimental %s", __func__);
> >>      DPRINTF("fd %d, dom %u, max_iters %u, max_factor %u, flags %u, hvm %d",
> >>              io_fd, dom, max_iters, max_factor, flags, hvm);
> >> diff --git a/tools/libxc/xc_sr_save_x86_hvm.c b/tools/libxc/xc_sr_save_x86_hvm.c
> >> index 8baa104..58efdb9 100644
> >> --- a/tools/libxc/xc_sr_save_x86_hvm.c
> >> +++ b/tools/libxc/xc_sr_save_x86_hvm.c
> >> @@ -166,13 +166,6 @@ static int x86_hvm_setup(struct xc_sr_context *ctx)
> >>  {
> >>      xc_interface *xch = ctx->xch;
> >>  
> >> -    if ( !ctx->save.callbacks->switch_qemu_logdirty )
> >> -    {
> >> -        ERROR("No switch_qemu_logdirty callback provided");
> >> -        errno = EINVAL;
> >> -        return -1;
> >> -    }
> >> -
> >>      if ( ctx->save.callbacks->switch_qemu_logdirty(
> >>               ctx->domid, 1, ctx->save.callbacks->data) )
> >>      {
> >
> 

Re: [PATCH v2 4/6] libxc/save: Adjust stream-position callbacks for checkpointed streams

[Ian Campbell]

On Mon, 2015-05-11 at 12:56 +0100, Andrew Cooper wrote:
> On 11/05/15 12:36, Ian Campbell wrote:
> > On Fri, 2015-05-08 at 22:14 +0100, Andrew Cooper wrote:
> >> There are some records which should only be sent once in the stream, and not
> >> repeated for each checkpoint.  {start,end}_of_stream() become per-checkpoint,
> >> and a new start_of_stream() is introduced.
> >>
> >> There is no resulting change record order, but the X86_PV_INFO and
> >> X86_PV_P2M_FRAMES records are positively identified as once per stream, rather
> >> than once per checkpoint.
> > In the case of the latter judging from the comments this is currently an
> > implementation detail and in principal we want to be able to resend the
> > p2m frames as well?
> 
> Correct, and the spec regarding X86_PV_P2M_FRAMES was explicitly
> intended to allow partial updates in the future.
> 
> It is currently an implementation bug (or many) that the P2M can't
> change during the live part of migration.

In which case I'd suggest avoiding saying "positively identified as once
per stream" since that gives the impression that this is per the spec
rather than just the current implementation.

Ian.

Re: [PATCH v2 4/6] libxc/save: Adjust stream-position callbacks for checkpointed streams

[Andrew Cooper]

On 11/05/15 13:05, Ian Campbell wrote:
> On Mon, 2015-05-11 at 12:56 +0100, Andrew Cooper wrote:
>> On 11/05/15 12:36, Ian Campbell wrote:
>>> On Fri, 2015-05-08 at 22:14 +0100, Andrew Cooper wrote:
>>>> There are some records which should only be sent once in the stream, and not
>>>> repeated for each checkpoint.  {start,end}_of_stream() become per-checkpoint,
>>>> and a new start_of_stream() is introduced.
>>>>
>>>> There is no resulting change record order, but the X86_PV_INFO and
>>>> X86_PV_P2M_FRAMES records are positively identified as once per stream, rather
>>>> than once per checkpoint.
>>> In the case of the latter judging from the comments this is currently an
>>> implementation detail and in principal we want to be able to resend the
>>> p2m frames as well?
>> Correct, and the spec regarding X86_PV_P2M_FRAMES was explicitly
>> intended to allow partial updates in the future.
>>
>> It is currently an implementation bug (or many) that the P2M can't
>> change during the live part of migration.
> In which case I'd suggest avoiding saying "positively identified as once
> per stream" since that gives the impression that this is per the spec
> rather than just the current implementation.

Ok - I will adjust the comments.

~Andrew