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

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

[Andrew Cooper]

See individual patches for details.

Andrew Cooper (3):
  [RFC] x86/hvm: Permit HVM_PARAM_IDENT_PT to be set more than once
  tools/libxc: Properly quote macro parameters
  libxc/migrationv2: Split {start,end}_of_stream() to make checkpoint
    variants

 tools/libxc/include/xenctrl.h    |    8 +++----
 tools/libxc/xc_sr_common.h       |   25 +++++++++++++++------
 tools/libxc/xc_sr_save.c         |    8 +++++++
 tools/libxc/xc_sr_save_x86_hvm.c |   46 ++++++++++++++++++++++++--------------
 tools/libxc/xc_sr_save_x86_pv.c  |   46 +++++++++++++++++++++++++-------------
 xen/arch/x86/hvm/hvm.c           |   12 +++++-----
 6 files changed, 95 insertions(+), 50 deletions(-)

-- 
1.7.10.4

[PATCH 1/3] [RFC] x86/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>
---
 xen/arch/x86/hvm/hvm.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 8140a27..5824e43 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5730,12 +5730,13 @@ 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) )
+        d->arch.hvm_domain.params[a.index] = a.value;
+        /*
+         * Only actually required for VT-x lacking unrestricted_guest
+         * capabilities.  Short circuit the pause if possible.
+         */
+        if ( !paging_mode_hap(d) || !cpu_has_vmx )
             break;
 
         /*
@@ -5749,7 +5750,6 @@ static int hvmop_set_param(
 
         rc = 0;
         domain_pause(d);
-        d->arch.hvm_domain.params[a.index] = a.value;
         for_each_vcpu ( d, v )
             paging_update_cr3(v);
         domain_unpause(d);
-- 
1.7.10.4

[PATCH 2/3] tools/libxc: Properly quote macro parameters

[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/xenctrl.h |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 6994c51..fc880e1 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -270,7 +270,7 @@ struct xc_hypercall_buffer {
  * transparently converted to the hypercall buffer as necessary.
  */
 #define DECLARE_HYPERCALL_BUFFER(_type, _name)                 \
-    _type *_name = NULL;                                       \
+    _type *(_name) = NULL;                                     \
     xc_hypercall_buffer_t XC__HYPERCALL_BUFFER_NAME(_name) = { \
         .hbuf = NULL,                                          \
         .param_shadow = NULL,                                  \
@@ -288,10 +288,10 @@ struct xc_hypercall_buffer {
  * required.
  */
 #define DECLARE_HYPERCALL_BUFFER_SHADOW(_type, _name, _hbuf)   \
-    _type *_name = _hbuf->hbuf;                                \
+    _type *(_name) = (_hbuf)->hbuf;                            \
     xc_hypercall_buffer_t XC__HYPERCALL_BUFFER_NAME(_name) = { \
         .hbuf = (void *)-1,                                    \
-        .param_shadow = _hbuf,                                 \
+        .param_shadow = (_hbuf),                               \
         HYPERCALL_BUFFER_INIT_NO_BOUNCE                        \
     }
 
@@ -302,7 +302,7 @@ struct xc_hypercall_buffer {
 #define DECLARE_HYPERCALL_BUFFER_ARGUMENT(_name)               \
     xc_hypercall_buffer_t XC__HYPERCALL_BUFFER_NAME(_name) = { \
         .hbuf = (void *)-1,                                    \
-        .param_shadow = _name,                                 \
+        .param_shadow = (_name),                               \
         HYPERCALL_BUFFER_INIT_NO_BOUNCE                        \
     }
 
-- 
1.7.10.4

[PATCH 3/3] libxc/migrationv2: Split {start, end}_of_stream() to make checkpoint variants

[Andrew Cooper]

This is in preparation for supporting checkpointed streams in migration v2.
 - For PV guests, the VCPU context is moved to end_of_checkpoint().
 - For HVM guests, the HVM context and params are moved to end_of_checkpoint().

For regular migration, this results in a reordering of the tail records, but
no semantic change.

In addition fix a couple of stylistic issues and adjust some comments.

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>

---
This patch has not tested yet, but is just some code shuffling and should have
no observable effect from the guests point of view.
---
 tools/libxc/xc_sr_common.h       |   25 +++++++++++++++------
 tools/libxc/xc_sr_save.c         |    8 +++++++
 tools/libxc/xc_sr_save_x86_hvm.c |   46 ++++++++++++++++++++++++--------------
 tools/libxc/xc_sr_save_x86_pv.c  |   46 +++++++++++++++++++++++++-------------
 4 files changed, 85 insertions(+), 40 deletions(-)

diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h
index ef42412..56169ea 100644
--- a/tools/libxc/xc_sr_common.h
+++ b/tools/libxc/xc_sr_common.h
@@ -61,17 +61,28 @@ 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 (*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);
+
+    /**
+     * Send records which need to be at the end of the stream.  This is called
+     * once, before the END record is written.
      */
     int (*end_of_stream)(struct xc_sr_context *ctx);
 
diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
index 5d9c267..49c65e9 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,6 +682,10 @@ static int save(struct xc_sr_context *ctx, uint16_t guest_type)
         goto err;
     }
 
+    rc = ctx->save.ops.end_of_checkpoint(ctx);
+    if ( rc )
+        goto err;
+
     xc_report_progress_single(xch, "End of stream");
 
     rc = ctx->save.ops.end_of_stream(ctx);
diff --git a/tools/libxc/xc_sr_save_x86_hvm.c b/tools/libxc/xc_sr_save_x86_hvm.c
index 8baa104..4ad1264 100644
--- a/tools/libxc/xc_sr_save_x86_hvm.c
+++ b/tools/libxc/xc_sr_save_x86_hvm.c
@@ -191,32 +191,42 @@ 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;
 
-    /* Write the TSC record. */
-    rc = write_tsc_info(ctx);
+    rc = write_hvm_context(ctx);
     if ( rc )
         return rc;
 
-#ifdef XG_LIBXL_HVM_COMPAT
-    rc = write_toolstack(ctx);
+    rc = write_hvm_params(ctx);
     if ( rc )
         return rc;
-#endif
 
-    /* Write the HVM_CONTEXT record. */
-    rc = write_hvm_context(ctx);
+    return 0;
+}
+
+static int x86_hvm_end_of_stream(struct xc_sr_context *ctx)
+{
+    int rc;
+
+    rc = write_tsc_info(ctx);
     if ( rc )
         return rc;
 
-    /* Write HVM_PARAMS record contains applicable HVM params. */
-    rc = write_hvm_params(ctx);
+#ifdef XG_LIBXL_HVM_COMPAT
+    rc = write_toolstack(ctx);
     if ( rc )
         return rc;
+#endif
 
-    return rc;
+    return 0;
 }
 
 static int x86_hvm_cleanup(struct xc_sr_context *ctx)
@@ -237,12 +247,14 @@ 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,
+    .end_of_stream       = x86_hvm_end_of_stream,
+    .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..df54ef3 100644
--- a/tools/libxc/xc_sr_save_x86_pv.c
+++ b/tools/libxc/xc_sr_save_x86_pv.c
@@ -805,9 +805,6 @@ static int x86_pv_setup(struct xc_sr_context *ctx)
     return 0;
 }
 
-/*
- * save_ops function.  Writes PV header records into the stream.
- */
 static int x86_pv_start_of_stream(struct xc_sr_context *ctx)
 {
     int rc;
@@ -816,6 +813,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,22 +826,31 @@ 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;
 
-    rc = write_tsc_info(ctx);
+    rc = write_all_vcpu_information(ctx);
     if ( rc )
         return rc;
 
-    rc = write_shared_info(ctx);
+    return 0;
+}
+
+static int x86_pv_end_of_stream(struct xc_sr_context *ctx)
+{
+    int rc;
+
+    rc = write_tsc_info(ctx);
     if ( rc )
         return rc;
 
-    rc = write_all_vcpu_information(ctx);
+    rc = write_shared_info(ctx);
     if ( rc )
         return rc;
 
@@ -866,12 +878,14 @@ 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,
+    .end_of_stream       = x86_pv_end_of_stream,
+    .cleanup             = x86_pv_cleanup,
 };
 
 /*
-- 
1.7.10.4

Re: [PATCH 2/3] tools/libxc: Properly quote macro parameters

[Andrew Cooper]

And of course, I meant s/quote/bracket/ in the subject.

~Andrew

Re: [PATCH 0/3] Misc patches to aid migration v2 Remus support

[Andrew Cooper]

On 08/05/15 13:54, Andrew Cooper wrote:
> See individual patches for details.

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

Yang: Please feel free to consume some of all of these patches as
appropriate into your series, rather than both of us attempting to
maintain different versions of the same series to support remus with
migration v2.

~Andrew

>
> Andrew Cooper (3):
>   [RFC] x86/hvm: Permit HVM_PARAM_IDENT_PT to be set more than once
>   tools/libxc: Properly quote macro parameters
>   libxc/migrationv2: Split {start,end}_of_stream() to make checkpoint
>     variants
>
>  tools/libxc/include/xenctrl.h    |    8 +++----
>  tools/libxc/xc_sr_common.h       |   25 +++++++++++++++------
>  tools/libxc/xc_sr_save.c         |    8 +++++++
>  tools/libxc/xc_sr_save_x86_hvm.c |   46 ++++++++++++++++++++++++--------------
>  tools/libxc/xc_sr_save_x86_pv.c  |   46 +++++++++++++++++++++++++-------------
>  xen/arch/x86/hvm/hvm.c           |   12 +++++-----
>  6 files changed, 95 insertions(+), 50 deletions(-)
>

Re: [PATCH 2/3] tools/libxc: Properly quote macro parameters

[Ian Campbell]

On Fri, 2015-05-08 at 13:54 +0100, Andrew Cooper wrote:
> 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/xenctrl.h |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 6994c51..fc880e1 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -270,7 +270,7 @@ struct xc_hypercall_buffer {
>   * transparently converted to the hypercall buffer as necessary.
>   */
>  #define DECLARE_HYPERCALL_BUFFER(_type, _name)                 \
> -    _type *_name = NULL;                                       \
> +    _type *(_name) = NULL;                                     \
>      xc_hypercall_buffer_t XC__HYPERCALL_BUFFER_NAME(_name) = { \
>          .hbuf = NULL,                                          \
>          .param_shadow = NULL,                                  \
> @@ -288,10 +288,10 @@ struct xc_hypercall_buffer {
>   * required.
>   */
>  #define DECLARE_HYPERCALL_BUFFER_SHADOW(_type, _name, _hbuf)   \
> -    _type *_name = _hbuf->hbuf;                                \
> +    _type *(_name) = (_hbuf)->hbuf;                            \

This seems to be the only one which matters in practice, but it's good
manners, so:

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH 3/3] libxc/migrationv2: Split {start, end}_of_stream() to make checkpoint variants

[Ian Campbell]

On Fri, 2015-05-08 at 13:54 +0100, Andrew Cooper wrote:
> This is in preparation for supporting checkpointed streams in migration v2.
>  - For PV guests, the VCPU context is moved to end_of_checkpoint().
>  - For HVM guests, the HVM context and params are moved to end_of_checkpoint().

[...]
> +    /**
> +     * 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);
> +
> +    /**
> +     * Send records which need to be at the end of the stream.  This is called
> +     * once, before the END record is written.
>       */
>      int (*end_of_stream)(struct xc_sr_context *ctx);
[...]
> +static int x86_hvm_end_of_stream(struct xc_sr_context *ctx)
> +{
> +    int rc;
> +
> +    rc = write_tsc_info(ctx);
>      if ( rc )
>          return rc;
>  
> -    /* Write HVM_PARAMS record contains applicable HVM params. */
> -    rc = write_hvm_params(ctx);
> +#ifdef XG_LIBXL_HVM_COMPAT
> +    rc = write_toolstack(ctx);

I'm not sure about this end_of_stream thing. In a check pointing for
fault tolerance scenario (Remus or COLO) then failover happens when the
sender has died for some reason, and therefore won't get the chance to
send any end of stream stuff.

IOW I think everything in end_of_stream actually needs to be in
end_of_checkpoint unless it is just for informational purposes in a
regular migration or something (which write_toolstack surely isn't)

Ian.

Re: [PATCH 3/3] libxc/migrationv2: Split {start, end}_of_stream() to make checkpoint variants

[Andrew Cooper]

On 08/05/15 14:30, Ian Campbell wrote:
> On Fri, 2015-05-08 at 13:54 +0100, Andrew Cooper wrote:
>> This is in preparation for supporting checkpointed streams in migration v2.
>>  - For PV guests, the VCPU context is moved to end_of_checkpoint().
>>  - For HVM guests, the HVM context and params are moved to end_of_checkpoint().
> [...]
>> +    /**
>> +     * 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);
>> +
>> +    /**
>> +     * Send records which need to be at the end of the stream.  This is called
>> +     * once, before the END record is written.
>>       */
>>      int (*end_of_stream)(struct xc_sr_context *ctx);
> [...]
>> +static int x86_hvm_end_of_stream(struct xc_sr_context *ctx)
>> +{
>> +    int rc;
>> +
>> +    rc = write_tsc_info(ctx);
>>      if ( rc )
>>          return rc;
>>  
>> -    /* Write HVM_PARAMS record contains applicable HVM params. */
>> -    rc = write_hvm_params(ctx);
>> +#ifdef XG_LIBXL_HVM_COMPAT
>> +    rc = write_toolstack(ctx);
> I'm not sure about this end_of_stream thing. In a check pointing for
> fault tolerance scenario (Remus or COLO) then failover happens when the
> sender has died for some reason, and therefore won't get the chance to
> send any end of stream stuff.

Does Remus currently function if the sending toolstack suddenly
disappears out of the mix?

Although on consideration, I do agree that end_of_stream() shouldn't
really be a thing.

>
> IOW I think everything in end_of_stream actually needs to be in
> end_of_checkpoint unless it is just for informational purposes in a
> regular migration or something (which write_toolstack surely isn't)

There are some side effects which I will have to work around, but I will
see what I can do.

~Andrew

Re: [PATCH 1/3] [RFC] x86/hvm: Permit HVM_PARAM_IDENT_PT to be set more than once

[Jan Beulich]

>>> On 08.05.15 at 14:54, <andrew.cooper3@citrix.com> 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.

Reads all plausible, except that I think the way you carry out this
last aspect needs adjustment:

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5730,12 +5730,13 @@ 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) )
> +        d->arch.hvm_domain.params[a.index] = a.value;
> +        /*
> +         * Only actually required for VT-x lacking unrestricted_guest
> +         * capabilities.  Short circuit the pause if possible.
> +         */
> +        if ( !paging_mode_hap(d) || !cpu_has_vmx )
>              break;

You should latch the new value inside this if()'s body and ...

> @@ -5749,7 +5750,6 @@ static int hvmop_set_param(
>  
>          rc = 0;
>          domain_pause(d);
> -        d->arch.hvm_domain.params[a.index] = a.value;
>          for_each_vcpu ( d, v )
>              paging_update_cr3(v);
>          domain_unpause(d);

... leave this one alone so the use site in VMX code won't see the
new value prematurely.

Jan

Re: [PATCH 1/3] [RFC] x86/hvm: Permit HVM_PARAM_IDENT_PT to be set more than once

[Andrew Cooper]

On 08/05/15 14:46, Jan Beulich wrote:
>>>> On 08.05.15 at 14:54, <andrew.cooper3@citrix.com> 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.
> Reads all plausible, except that I think the way you carry out this
> last aspect needs adjustment:
>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -5730,12 +5730,13 @@ 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) )
>> +        d->arch.hvm_domain.params[a.index] = a.value;
>> +        /*
>> +         * Only actually required for VT-x lacking unrestricted_guest
>> +         * capabilities.  Short circuit the pause if possible.
>> +         */
>> +        if ( !paging_mode_hap(d) || !cpu_has_vmx )
>>              break;
> You should latch the new value inside this if()'s body and ...
>
>> @@ -5749,7 +5750,6 @@ static int hvmop_set_param(
>>  
>>          rc = 0;
>>          domain_pause(d);
>> -        d->arch.hvm_domain.params[a.index] = a.value;
>>          for_each_vcpu ( d, v )
>>              paging_update_cr3(v);
>>          domain_unpause(d);
> ... leave this one alone so the use site in VMX code won't see the
> new value prematurely.

Good point - I will fix this up.

~Andrew

Re: [PATCH 3/3] libxc/migrationv2: Split {start, end}_of_stream() to make checkpoint variants

[Ian Campbell]

On Fri, 2015-05-08 at 14:37 +0100, Andrew Cooper wrote:
> Does Remus currently function if the sending toolstack suddenly
> disappears out of the mix?

I would assume so, that's its entire purpose...

Ian.

Re: [PATCH 3/3] libxc/migrationv2: Split {start, end}_of_stream() to make checkpoint variants

[Andrew Cooper]

On 08/05/15 14:50, Ian Campbell wrote:
> On Fri, 2015-05-08 at 14:37 +0100, Andrew Cooper wrote:
>> Does Remus currently function if the sending toolstack suddenly
>> disappears out of the mix?
> I would assume so, that's its entire purpose...

But the signal to resume the domain properly involves setting
last_checkpoint and wandering the recieve loop once more?

/me goes and checks the code more carefully.

~Andrew

Re: [PATCH 3/3] libxc/migrationv2: Split {start, end}_of_stream() to make checkpoint variants

[Ian Campbell]

On Fri, 2015-05-08 at 14:55 +0100, Andrew Cooper wrote:
> On 08/05/15 14:50, Ian Campbell wrote:
> > On Fri, 2015-05-08 at 14:37 +0100, Andrew Cooper wrote:
> >> Does Remus currently function if the sending toolstack suddenly
> >> disappears out of the mix?
> > I would assume so, that's its entire purpose...
> 
> But the signal to resume the domain properly involves setting
> last_checkpoint and wandering the recieve loop once more?
> 
> /me goes and checks the code more carefully.

I think there's supposed to be a read timeout in there somewhere, at
which point the last checkpoint you did manage to get is the one which
is used.

(The heartbeating and detection of this stuff bit of remus was always a
bit underwhelming)

Ian.

Re: [PATCH 2/3] tools/libxc: Properly quote macro parameters

[Ian Campbell]

On Fri, 2015-05-08 at 14:00 +0100, Andrew Cooper wrote:
> And of course, I meant s/quote/bracket/ in the subject.

I fixed that and applied.

Ian.

Re: [PATCH 3/3] libxc/migrationv2: Split {start, end}_of_stream() to make checkpoint variants

[Hongyang Yang]

On 05/08/2015 09:30 PM, Ian Campbell wrote:
> On Fri, 2015-05-08 at 13:54 +0100, Andrew Cooper wrote:
>> This is in preparation for supporting checkpointed streams in migration v2.
>>   - For PV guests, the VCPU context is moved to end_of_checkpoint().
>>   - For HVM guests, the HVM context and params are moved to end_of_checkpoint().
>
> [...]
>> +    /**
>> +     * 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);
>> +
>> +    /**
>> +     * Send records which need to be at the end of the stream.  This is called
>> +     * once, before the END record is written.
>>        */
>>       int (*end_of_stream)(struct xc_sr_context *ctx);
> [...]
>> +static int x86_hvm_end_of_stream(struct xc_sr_context *ctx)
>> +{
>> +    int rc;
>> +
>> +    rc = write_tsc_info(ctx);
>>       if ( rc )
>>           return rc;
>>
>> -    /* Write HVM_PARAMS record contains applicable HVM params. */
>> -    rc = write_hvm_params(ctx);
>> +#ifdef XG_LIBXL_HVM_COMPAT
>> +    rc = write_toolstack(ctx);
>
> I'm not sure about this end_of_stream thing. In a check pointing for
> fault tolerance scenario (Remus or COLO) then failover happens when the
> sender has died for some reason, and therefore won't get the chance to
> send any end of stream stuff.
>
> IOW I think everything in end_of_stream actually needs to be in
> end_of_checkpoint unless it is just for informational purposes in a
> regular migration or something (which write_toolstack surely isn't)

Yes, all records should be sent at every checkpoint, except those
only need to be sent once.

checkpoint:
You can see clearly from the patches a Remus migration explicit include
two stage, first stage is live migration, the second is Checkpointed
stream. The live migration is obvious, after the live migration, both
primary and secondary are in the same state, the primary will continue
to run until the next checkpoint, at checkpint, we sync the secondary
state with the primary, so that both side are in the same state, so
any record that could be changed while Guest is runing should be sent
at checkpoint.

failover:
The handling of Checkpointed stream on restore side is also include two stage,
first is buffer records, second is process records. This is because if master
died when sending records, the secondary state will be inconsistent. So we
have to make sure all records are received and then process the records.
If master died, the secondary can recover from the last checkpoint state.
Currently Remus failover relies on the migration channel. If the channel
break, we presume master is dead, so we will failover. The "goto err_buf" is
the failover path, with goto err_buf, we discard the current checkpoint
records because it is imperfect, then resume the guest with last checkpoint
state(the last processed records).

>
> Ian.
>
> .
>

-- 
Thanks,
Yang.

Re: [PATCH 3/3] libxc/migrationv2: Split {start, end}_of_stream() to make checkpoint variants

[Hongyang Yang]

On 05/08/2015 09:55 PM, Andrew Cooper wrote:
> On 08/05/15 14:50, Ian Campbell wrote:
>> On Fri, 2015-05-08 at 14:37 +0100, Andrew Cooper wrote:
>>> Does Remus currently function if the sending toolstack suddenly
>>> disappears out of the mix?
>> I would assume so, that's its entire purpose...
>
> But the signal to resume the domain properly involves setting
> last_checkpoint and wandering the recieve loop once more?

Please see my previous explain about Remus checkpoint and failover
Message-ID: <5550147B.1010304@cn.fujitsu.com>

If the sending toolstack suddenly disappears out of the mix, secondary
will recover from the last processed records which include the full
toolstack record.

>
> /me goes and checks the code more carefully.
>
> ~Andrew
> .
>

-- 
Thanks,
Yang.

Re: [PATCH 0/3] Misc patches to aid migration v2 Remus support

[Hongyang Yang]

On 05/08/2015 09:15 PM, Andrew Cooper wrote:
> On 08/05/15 13:54, Andrew Cooper wrote:
>> See individual patches for details.
>
> Git tree available at:
>   git://xenbits.xen.org/people/andrewcoop/xen.git remus-migv2-v1
>
> Yang: Please feel free to consume some of all of these patches as
> appropriate into your series, rather than both of us attempting to
> maintain different versions of the same series to support remus with
> migration v2.

Sure, thank you for the effort on Remus support :)

>
> ~Andrew
>
>>
>> Andrew Cooper (3):
>>    [RFC] x86/hvm: Permit HVM_PARAM_IDENT_PT to be set more than once
>>    tools/libxc: Properly quote macro parameters
>>    libxc/migrationv2: Split {start,end}_of_stream() to make checkpoint
>>      variants
>>
>>   tools/libxc/include/xenctrl.h    |    8 +++----
>>   tools/libxc/xc_sr_common.h       |   25 +++++++++++++++------
>>   tools/libxc/xc_sr_save.c         |    8 +++++++
>>   tools/libxc/xc_sr_save_x86_hvm.c |   46 ++++++++++++++++++++++++--------------
>>   tools/libxc/xc_sr_save_x86_pv.c  |   46 +++++++++++++++++++++++++-------------
>>   xen/arch/x86/hvm/hvm.c           |   12 +++++-----
>>   6 files changed, 95 insertions(+), 50 deletions(-)
>>
>
> .
>

-- 
Thanks,
Yang.

Re: [PATCH 3/3] libxc/migrationv2: Split {start, end}_of_stream() to make checkpoint variants

[Andrew Cooper]

On 11/05/15 03:31, Hongyang Yang wrote:
> On 05/08/2015 09:30 PM, Ian Campbell wrote:
>> On Fri, 2015-05-08 at 13:54 +0100, Andrew Cooper wrote:
>>> This is in preparation for supporting checkpointed streams in
>>> migration v2.
>>>   - For PV guests, the VCPU context is moved to end_of_checkpoint().
>>>   - For HVM guests, the HVM context and params are moved to
>>> end_of_checkpoint().
>>
>> [...]
>>> +    /**
>>> +     * 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);
>>> +
>>> +    /**
>>> +     * Send records which need to be at the end of the stream. 
>>> This is called
>>> +     * once, before the END record is written.
>>>        */
>>>       int (*end_of_stream)(struct xc_sr_context *ctx);
>> [...]
>>> +static int x86_hvm_end_of_stream(struct xc_sr_context *ctx)
>>> +{
>>> +    int rc;
>>> +
>>> +    rc = write_tsc_info(ctx);
>>>       if ( rc )
>>>           return rc;
>>>
>>> -    /* Write HVM_PARAMS record contains applicable HVM params. */
>>> -    rc = write_hvm_params(ctx);
>>> +#ifdef XG_LIBXL_HVM_COMPAT
>>> +    rc = write_toolstack(ctx);
>>
>> I'm not sure about this end_of_stream thing. In a check pointing for
>> fault tolerance scenario (Remus or COLO) then failover happens when the
>> sender has died for some reason, and therefore won't get the chance to
>> send any end of stream stuff.
>>
>> IOW I think everything in end_of_stream actually needs to be in
>> end_of_checkpoint unless it is just for informational purposes in a
>> regular migration or something (which write_toolstack surely isn't)
>
> Yes, all records should be sent at every checkpoint, except those
> only need to be sent once.
>
> checkpoint:
> You can see clearly from the patches a Remus migration explicit include
> two stage, first stage is live migration, the second is Checkpointed
> stream. The live migration is obvious, after the live migration, both
> primary and secondary are in the same state, the primary will continue
> to run until the next checkpoint, at checkpint, we sync the secondary
> state with the primary, so that both side are in the same state, so
> any record that could be changed while Guest is runing should be sent
> at checkpoint.
>
> failover:
> The handling of Checkpointed stream on restore side is also include
> two stage,
> first is buffer records, second is process records. This is because if
> master
> died when sending records, the secondary state will be inconsistent.
> So we
> have to make sure all records are received and then process the records.
> If master died, the secondary can recover from the last checkpoint state.
> Currently Remus failover relies on the migration channel. If the channel
> break, we presume master is dead, so we will failover. The "goto
> err_buf" is
> the failover path, with goto err_buf, we discard the current checkpoint
> records because it is imperfect, then resume the guest with last
> checkpoint
> state(the last processed records).

Thankyou for the clarification.

It occurs to me that, despite things like 'last_iter', it is actually
the first iteration which is actually special in Remus.

Is there a case where the primary decides to explicitly hand over to the
secondary?

~Andrew

Re: [PATCH 3/3] libxc/migrationv2: Split {start, end}_of_stream() to make checkpoint variants

[Hongyang Yang]

On 05/11/2015 05:02 PM, Andrew Cooper wrote:
> On 11/05/15 03:31, Hongyang Yang wrote:
>> On 05/08/2015 09:30 PM, Ian Campbell wrote:
>>> On Fri, 2015-05-08 at 13:54 +0100, Andrew Cooper wrote:
>>>> This is in preparation for supporting checkpointed streams in
>>>> migration v2.
>>>>    - For PV guests, the VCPU context is moved to end_of_checkpoint().
>>>>    - For HVM guests, the HVM context and params are moved to
>>>> end_of_checkpoint().
>>>
>>> [...]
>>>> +    /**
>>>> +     * 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);
>>>> +
>>>> +    /**
>>>> +     * Send records which need to be at the end of the stream.
>>>> This is called
>>>> +     * once, before the END record is written.
>>>>         */
>>>>        int (*end_of_stream)(struct xc_sr_context *ctx);
>>> [...]
>>>> +static int x86_hvm_end_of_stream(struct xc_sr_context *ctx)
>>>> +{
>>>> +    int rc;
>>>> +
>>>> +    rc = write_tsc_info(ctx);
>>>>        if ( rc )
>>>>            return rc;
>>>>
>>>> -    /* Write HVM_PARAMS record contains applicable HVM params. */
>>>> -    rc = write_hvm_params(ctx);
>>>> +#ifdef XG_LIBXL_HVM_COMPAT
>>>> +    rc = write_toolstack(ctx);
>>>
>>> I'm not sure about this end_of_stream thing. In a check pointing for
>>> fault tolerance scenario (Remus or COLO) then failover happens when the
>>> sender has died for some reason, and therefore won't get the chance to
>>> send any end of stream stuff.
>>>
>>> IOW I think everything in end_of_stream actually needs to be in
>>> end_of_checkpoint unless it is just for informational purposes in a
>>> regular migration or something (which write_toolstack surely isn't)
>>
>> Yes, all records should be sent at every checkpoint, except those
>> only need to be sent once.
>>
>> checkpoint:
>> You can see clearly from the patches a Remus migration explicit include
>> two stage, first stage is live migration, the second is Checkpointed
>> stream. The live migration is obvious, after the live migration, both
>> primary and secondary are in the same state, the primary will continue
>> to run until the next checkpoint, at checkpint, we sync the secondary
>> state with the primary, so that both side are in the same state, so
>> any record that could be changed while Guest is runing should be sent
>> at checkpoint.
>>
>> failover:
>> The handling of Checkpointed stream on restore side is also include
>> two stage,
>> first is buffer records, second is process records. This is because if
>> master
>> died when sending records, the secondary state will be inconsistent.
>> So we
>> have to make sure all records are received and then process the records.
>> If master died, the secondary can recover from the last checkpoint state.
>> Currently Remus failover relies on the migration channel. If the channel
>> break, we presume master is dead, so we will failover. The "goto
>> err_buf" is
>> the failover path, with goto err_buf, we discard the current checkpoint
>> records because it is imperfect, then resume the guest with last
>> checkpoint
>> state(the last processed records).
>
> Thankyou for the clarification.
>
> It occurs to me that, despite things like 'last_iter', it is actually
> the first iteration which is actually special in Remus.

yes, the 'last_iter' thing is actually suspend and send the dirty mem
pages to secondary.

>
> Is there a case where the primary decides to explicitly hand over to the
> secondary?

Currently there isn't, The secondary only starts on failover.

>
> ~Andrew
> .
>

-- 
Thanks,
Yang.