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

[PATCH v3 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-v3

~Andrew

[PATCH v3 1/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 v3 2/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 record is
identified as once per stream.  Currently the X86_PV_P2M_FRAMES record is as
well, but this is because of an implementation bug and can move back to being
on an as-needed basis when fixed.

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>

---
v3: Adjust commit message
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

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

[George Dunlap]

On Tue, May 12, 2015 at 10:35 AM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> 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;
>  }

Nit: It looks like this boils down to:

if ( rc != 0 ) return rc; else return 0;

Why not just return rc and be done with it?

(Just skimming through, no other comments ATM)

 -George

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

[Andrew Cooper]

On 13/05/15 11:09, George Dunlap wrote:
> On Tue, May 12, 2015 at 10:35 AM, Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>> 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;
>>  }
> Nit: It looks like this boils down to:
>
> if ( rc != 0 ) return rc; else return 0;
>
> Why not just return rc and be done with it?
>
> (Just skimming through, no other comments ATM)

Cleaner when adding new hunks in, which other patches do.

~Andrew

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

[George Dunlap]

On Wed, May 13, 2015 at 11:23 AM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 13/05/15 11:09, George Dunlap wrote:
>> On Tue, May 12, 2015 at 10:35 AM, Andrew Cooper
>> <andrew.cooper3@citrix.com> wrote:
>>> 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;
>>>  }
>> Nit: It looks like this boils down to:
>>
>> if ( rc != 0 ) return rc; else return 0;
>>
>> Why not just return rc and be done with it?
>>
>> (Just skimming through, no other comments ATM)
>
> Cleaner when adding new hunks in, which other patches do.

Sounds reasonable. :-)

 -George

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

[Andrew Cooper]

Checkpointed streams need to signal the end of a consistent view of VM state,
and the start of the libxl data.

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>

---

After discusions with David, we have come to the conclusion that a CHECKPOINT
record alone is sufficient for the remus case.  There is no functional need to
explicitly identify the stream as being of arbitrary length.
---
 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 520240f..97edd7d 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
@@ -227,7 +225,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_
@@ -578,6 +578,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 d116ca6..6d0f8fd 100644
--- a/tools/libxc/xc_sr_stream_format.h
+++ b/tools/libxc/xc_sr_stream_format.h
@@ -74,6 +74,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 v3 3/6] libxc/migration: Specification update for CHECKPOINT records

[Yang Hongyang]

On 05/12/2015 05:35 PM, Andrew Cooper wrote:
> Checkpointed streams need to signal the end of a consistent view of VM state,
> and the start of the libxl data.

Ah, now I know what's CHECKPOINT record mean, the name is OK for me then.

>
> 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>
>
> ---
>
> After discusions with David, we have come to the conclusion that a CHECKPOINT
> record alone is sufficient for the remus case.  There is no functional need to
> explicitly identify the stream as being of arbitrary length.
> ---
>   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 520240f..97edd7d 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
> @@ -227,7 +225,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_
> @@ -578,6 +578,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 d116ca6..6d0f8fd 100644
> --- a/tools/libxc/xc_sr_stream_format.h
> +++ b/tools/libxc/xc_sr_stream_format.h
> @@ -74,6 +74,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
>
>

-- 
Thanks,
Yang.

Re: [PATCH v3 3/6] libxc/migration: Specification update for CHECKPOINT records

[David Vrabel]

On 12/05/15 10:35, Andrew Cooper wrote:
> Checkpointed streams need to signal the end of a consistent view of VM state,
> and the start of the libxl data.
[...]
> --- a/docs/specs/libxc-migration-stream.pandoc
> +++ b/docs/specs/libxc-migration-stream.pandoc
[...]
> @@ -578,6 +578,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

...that all the preceding records 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.

This paragraph is stale.

I think you should expand a bit more on this record.  Perhaps:

  If the stream is embedded in a higher level toolstack stream, the
  CHECKPOINT record marks the end of the libxc portion of the stream
  and the stream is handed back to the higher level for further
  processing.

  The higher level stream may then hand the stream back to libxc to
  process another set of records for the next consistent VM state
  snapshot.  This next set of records may be terminated by another
  CHECKPOINT record or an END record.

David

Re: [PATCH v3 3/6] libxc/migration: Specification update for CHECKPOINT records

[Yang Hongyang]

Hi Andrew, David,

   Would you mind if I fix the comments, and combine this series with mine
prepare patches for Remus, and then send it out together?

On 05/12/2015 05:54 PM, David Vrabel wrote:
> On 12/05/15 10:35, Andrew Cooper wrote:
>> Checkpointed streams need to signal the end of a consistent view of VM state,
>> and the start of the libxl data.
> [...]
>> --- a/docs/specs/libxc-migration-stream.pandoc
>> +++ b/docs/specs/libxc-migration-stream.pandoc
> [...]
>> @@ -578,6 +578,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
>
> ...that all the preceding records 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.
>
> This paragraph is stale.
>
> I think you should expand a bit more on this record.  Perhaps:
>
>    If the stream is embedded in a higher level toolstack stream, the
>    CHECKPOINT record marks the end of the libxc portion of the stream
>    and the stream is handed back to the higher level for further
>    processing.
>
>    The higher level stream may then hand the stream back to libxc to
>    process another set of records for the next consistent VM state
>    snapshot.  This next set of records may be terminated by another
>    CHECKPOINT record or an END record.
>
> David
> .
>

-- 
Thanks,
Yang.

Re: [PATCH v3 3/6] libxc/migration: Specification update for CHECKPOINT records

[Andrew Cooper]

On 12/05/15 11:49, Yang Hongyang wrote:
> Hi Andrew, David,
>
>   Would you mind if I fix the comments, and combine this series with mine
> prepare patches for Remus, and then send it out together?

Go for it.  I am happy with Davids corrections.

~Andrew

>
> On 05/12/2015 05:54 PM, David Vrabel wrote:
>> On 12/05/15 10:35, Andrew Cooper wrote:
>>> Checkpointed streams need to signal the end of a consistent view of
>>> VM state,
>>> and the start of the libxl data.
>> [...]
>>> --- a/docs/specs/libxc-migration-stream.pandoc
>>> +++ b/docs/specs/libxc-migration-stream.pandoc
>> [...]
>>> @@ -578,6 +578,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
>>
>> ...that all the preceding records 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.
>>
>> This paragraph is stale.
>>
>> I think you should expand a bit more on this record.  Perhaps:
>>
>>    If the stream is embedded in a higher level toolstack stream, the
>>    CHECKPOINT record marks the end of the libxc portion of the stream
>>    and the stream is handed back to the higher level for further
>>    processing.
>>
>>    The higher level stream may then hand the stream back to libxc to
>>    process another set of records for the next consistent VM state
>>    snapshot.  This next set of records may be terminated by another
>>    CHECKPOINT record or an END record.
>>
>> David
>> .
>>
>

[PATCH v3 4/6] tools/libxc: introduce process_record()

[Andrew Cooper]

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

Move record handle codes into a function process_record().
It will be used multiple times by Remus.
No functional change.

Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
Reviewed-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>
---
 tools/libxc/xc_sr_restore.c |   77 ++++++++++++++++++++++++-------------------
 1 file changed, 43 insertions(+), 34 deletions(-)

diff --git a/tools/libxc/xc_sr_restore.c b/tools/libxc/xc_sr_restore.c
index 0bf4bae..53bd674 100644
--- a/tools/libxc/xc_sr_restore.c
+++ b/tools/libxc/xc_sr_restore.c
@@ -468,6 +468,48 @@ static int handle_page_data(struct xc_sr_context *ctx, struct xc_sr_record *rec)
     return rc;
 }
 
+static int process_record(struct xc_sr_context *ctx, struct xc_sr_record *rec)
+{
+    xc_interface *xch = ctx->xch;
+    int rc = 0;
+
+    switch ( rec->type )
+    {
+    case REC_TYPE_END:
+        break;
+
+    case REC_TYPE_PAGE_DATA:
+        rc = handle_page_data(ctx, rec);
+        break;
+
+    case REC_TYPE_VERIFY:
+        DPRINTF("Verify mode enabled");
+        ctx->restore.verify = true;
+        break;
+
+    default:
+        rc = ctx->restore.ops.process_record(ctx, rec);
+        break;
+    }
+
+    free(rec->data);
+
+    if ( rc == RECORD_NOT_PROCESSED )
+    {
+        if ( rec->type & REC_TYPE_OPTIONAL )
+            DPRINTF("Ignoring optional record %#x (%s)",
+                    rec->type, rec_type_to_str(rec->type));
+        else
+        {
+            ERROR("Mandatory record %#x (%s) not handled",
+                  rec->type, rec_type_to_str(rec->type));
+            rc = -1;
+        }
+    }
+
+    return rc;
+}
+
 /*
  * Restore a domain.
  */
@@ -498,40 +540,7 @@ static int restore(struct xc_sr_context *ctx)
         if ( rc )
             goto err;
 
-        switch ( rec.type )
-        {
-        case REC_TYPE_END:
-            break;
-
-        case REC_TYPE_PAGE_DATA:
-            rc = handle_page_data(ctx, &rec);
-            break;
-
-        case REC_TYPE_VERIFY:
-            DPRINTF("Verify mode enabled");
-            ctx->restore.verify = true;
-            break;
-
-        default:
-            rc = ctx->restore.ops.process_record(ctx, &rec);
-            break;
-        }
-
-        free(rec.data);
-
-        if ( rc == RECORD_NOT_PROCESSED )
-        {
-            if ( rec.type & REC_TYPE_OPTIONAL )
-                DPRINTF("Ignoring optional record %#x (%s)",
-                        rec.type, rec_type_to_str(rec.type));
-            else
-            {
-                ERROR("Mandatory record %#x (%s) not handled",
-                      rec.type, rec_type_to_str(rec.type));
-                rc = -1;
-            }
-        }
-
+        rc = process_record(ctx, &rec);
         if ( rc )
             goto err;
 
-- 
1.7.10.4

[PATCH v3 5/6] libxc/migration: Pass checkpoint information into the save algorithm.

[Andrew Cooper]

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/include/xenguest.h |    1 +
 tools/libxc/xc_sr_common.h     |    3 +++
 tools/libxc/xc_sr_save.c       |    3 +++
 tools/libxl/libxl_dom.c        |    1 +
 4 files changed, 8 insertions(+)

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..c0f90d4 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;
 
diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
index 66fcd3e..caa727d 100644
--- a/tools/libxc/xc_sr_save.c
+++ b/tools/libxc/xc_sr_save.c
@@ -732,6 +732,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 +746,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/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 v3 6/6] [RFC] [INCOMPLETE] libxc/restore: Remus

[Andrew Cooper]

Some basic infrastructure.

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

---

I started attempting to explain this in text, but concluded that pseudocode
would be easier.

My design for the restore side handling was always based around a single "do {
} while ( not end )" loop, and specifically not to split into two phases.

If I have interpreted your restore patch correctly, then this should be
plausible way to avoid splitting into two phases.  Please feel free to modify
to suit.

If at all possible, I would like to see about simiarly not splitting the save
side into multiple phases, but that is proving far harder, and it is probably
worth waiting for another iteration with current improvements integrated.
---
 tools/libxc/xc_sr_common.h  |    6 ++++++
 tools/libxc/xc_sr_restore.c |   34 ++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h
index c0f90d4..116c89b 100644
--- a/tools/libxc/xc_sr_common.h
+++ b/tools/libxc/xc_sr_common.h
@@ -219,6 +219,12 @@ struct xc_sr_context
 
             /* Sender has invoked verify mode on the stream. */
             bool verify;
+
+            /* Expecting a checkpointed stream? */
+            bool checkpointed;
+
+            /* Currently buffering records between a checkpoints? */
+            bool buffer_all_records;
         } restore;
     };
 
diff --git a/tools/libxc/xc_sr_restore.c b/tools/libxc/xc_sr_restore.c
index 53bd674..7a124bd 100644
--- a/tools/libxc/xc_sr_restore.c
+++ b/tools/libxc/xc_sr_restore.c
@@ -468,11 +468,40 @@ static int handle_page_data(struct xc_sr_context *ctx, struct xc_sr_record *rec)
     return rc;
 }
 
+static int handle_checkpoint(struct xc_sr_context *ctx)
+{
+    xc_interface *xch = ctx->xch;
+    int rc = 0;
+
+    if ( !ctx->restore.checkpointed )
+    {
+        ERROR("Found checkpoint in non-checkpointed stream\n");
+        return -1;
+    }
+
+    if ( ctx->restore.buffer_all_records )
+    {
+        /* TODO drain buffer */
+    }
+    else
+        ctx->restore.buffer_all_records = 1;
+
+    return rc;
+}
+
 static int process_record(struct xc_sr_context *ctx, struct xc_sr_record *rec)
 {
     xc_interface *xch = ctx->xch;
     int rc = 0;
 
+    if ( ctx->restore.buffer_all_records &&
+         rec->type != REC_TYPE_END &&
+         rec->type != REC_TYPE_CHECKPOINT )
+    {
+        /* TODO: copy into buffer */
+        return 0;
+    }
+
     switch ( rec->type )
     {
     case REC_TYPE_END:
@@ -487,6 +516,10 @@ static int process_record(struct xc_sr_context *ctx, struct xc_sr_record *rec)
         ctx->restore.verify = true;
         break;
 
+    case REC_TYPE_CHECKPOINT:
+        rc = handle_checkpoint(ctx);
+        break;
+
     default:
         rc = ctx->restore.ops.process_record(ctx, rec);
         break;
@@ -592,6 +625,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__);
-- 
1.7.10.4

Re: [PATCH v3 6/6] [RFC] [INCOMPLETE] libxc/restore: Remus

[Yang Hongyang]

I will try to implement the restore side as you proposed. The save side
was a single do{}while loop already. I use ctx.save.live to differentiate
the two phases.

On 05/12/2015 05:35 PM, Andrew Cooper wrote:
> Some basic infrastructure.
>
> CC: Yang Hongyang <yanghy@cn.fujitsu.com>
>
> ---
>
> I started attempting to explain this in text, but concluded that pseudocode
> would be easier.
>
> My design for the restore side handling was always based around a single "do {
> } while ( not end )" loop, and specifically not to split into two phases.
>
> If I have interpreted your restore patch correctly, then this should be
> plausible way to avoid splitting into two phases.  Please feel free to modify
> to suit.
>
> If at all possible, I would like to see about simiarly not splitting the save
> side into multiple phases, but that is proving far harder, and it is probably
> worth waiting for another iteration with current improvements integrated.
> ---
>   tools/libxc/xc_sr_common.h  |    6 ++++++
>   tools/libxc/xc_sr_restore.c |   34 ++++++++++++++++++++++++++++++++++
>   2 files changed, 40 insertions(+)
>
> diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h
> index c0f90d4..116c89b 100644
> --- a/tools/libxc/xc_sr_common.h
> +++ b/tools/libxc/xc_sr_common.h
> @@ -219,6 +219,12 @@ struct xc_sr_context
>
>               /* Sender has invoked verify mode on the stream. */
>               bool verify;
> +
> +            /* Expecting a checkpointed stream? */
> +            bool checkpointed;
> +
> +            /* Currently buffering records between a checkpoints? */
> +            bool buffer_all_records;
>           } restore;
>       };
>
> diff --git a/tools/libxc/xc_sr_restore.c b/tools/libxc/xc_sr_restore.c
> index 53bd674..7a124bd 100644
> --- a/tools/libxc/xc_sr_restore.c
> +++ b/tools/libxc/xc_sr_restore.c
> @@ -468,11 +468,40 @@ static int handle_page_data(struct xc_sr_context *ctx, struct xc_sr_record *rec)
>       return rc;
>   }
>
> +static int handle_checkpoint(struct xc_sr_context *ctx)
> +{
> +    xc_interface *xch = ctx->xch;
> +    int rc = 0;
> +
> +    if ( !ctx->restore.checkpointed )
> +    {
> +        ERROR("Found checkpoint in non-checkpointed stream\n");
> +        return -1;
> +    }
> +
> +    if ( ctx->restore.buffer_all_records )
> +    {
> +        /* TODO drain buffer */
> +    }
> +    else
> +        ctx->restore.buffer_all_records = 1;
> +
> +    return rc;
> +}
> +
>   static int process_record(struct xc_sr_context *ctx, struct xc_sr_record *rec)
>   {
>       xc_interface *xch = ctx->xch;
>       int rc = 0;
>
> +    if ( ctx->restore.buffer_all_records &&
> +         rec->type != REC_TYPE_END &&
> +         rec->type != REC_TYPE_CHECKPOINT )
> +    {
> +        /* TODO: copy into buffer */
> +        return 0;
> +    }
> +
>       switch ( rec->type )
>       {
>       case REC_TYPE_END:
> @@ -487,6 +516,10 @@ static int process_record(struct xc_sr_context *ctx, struct xc_sr_record *rec)
>           ctx->restore.verify = true;
>           break;
>
> +    case REC_TYPE_CHECKPOINT:
> +        rc = handle_checkpoint(ctx);
> +        break;
> +
>       default:
>           rc = ctx->restore.ops.process_record(ctx, rec);
>           break;
> @@ -592,6 +625,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__);
>

-- 
Thanks,
Yang.