Re: [Xen-devel] [PATCH 3/4] libxc: stop migration in case of p2m list structural changes

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 11/12/15 11:31, Juergen Gross wrote:
> With support of the virtual mapped linear p2m list for migration it is
> now possible to detect structural changes of the p2m list which before
> would either lead to a crashing or otherwise wrong behaving domU.
>
> A guest supporting the linear p2m list will increment the
> p2m_generation counter located in the shared info page before and after
> each modification of a mapping related to the p2m list. A change of
> that counter can be detected by the tools and reacted upon.
>
> As such a change should occur only very rarely once the domU is up the
> most simple reaction is to cancel migration in such an event.
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
>  tools/libxc/xc_sr_common.h       | 11 ++++++++++
>  tools/libxc/xc_sr_save.c         |  4 ++++
>  tools/libxc/xc_sr_save_x86_hvm.c |  7 +++++++
>  tools/libxc/xc_sr_save_x86_pv.c  | 44 
> ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 66 insertions(+)
>
> diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h
> index 9aecde2..bfb9602 100644
> --- a/tools/libxc/xc_sr_common.h
> +++ b/tools/libxc/xc_sr_common.h
> @@ -83,6 +83,14 @@ struct xc_sr_save_ops
>      int (*end_of_checkpoint)(struct xc_sr_context *ctx);
>  
>      /**
> +     * Check whether new iteration can be started.  This is called before 
> each
> +     * iteration to check whether all criteria for the migration are still
> +     * met.  If that's not the case either migration is cancelled via a bad 
> rc
> +     * or the situation is handled, e.g. by sending appropriate records.
> +     */
> +    int (*check_iteration)(struct xc_sr_context *ctx);
> +

This is slightly ambiguous, especially with the not-so-different
differences between live migration and remus checkpoints.

I would be tempted to name it check_vm_state() and document simply that
it is called periodically, to allow for fixup (or abort) for guest state
which may have changed while the VM was running.

On the remus side, it needs to be called between start_of_checkpoint()
and send_memory_***() in save(), as the guest gets to run between the
checkpoints.

> +    /**
>       * Clean up the local environment.  Will be called exactly once, either
>       * after a successful save, or upon encountering an error.
>       */
> @@ -280,6 +288,9 @@ struct xc_sr_context
>              /* Read-only mapping of guests shared info page */
>              shared_info_any_t *shinfo;
>  
> +            /* p2m generation count for verifying validity of local p2m. */
> +            uint64_t p2m_generation;
> +
>              union
>              {
>                  struct
> diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
> index cefcef5..c235706 100644
> --- a/tools/libxc/xc_sr_save.c
> +++ b/tools/libxc/xc_sr_save.c
> @@ -370,6 +370,10 @@ static int send_dirty_pages(struct xc_sr_context *ctx,
>      DECLARE_HYPERCALL_BUFFER_SHADOW(unsigned long, dirty_bitmap,
>                                      &ctx->save.dirty_bitmap_hbuf);
>  
> +    rc = ctx->save.ops.check_iteration(ctx);
> +    if ( rc )
> +        return rc;
> +

As there is now a call at each start of checkpoint, this call
essentially becomes back-to-back.  I would suggest having it after the
batch, rather than ahead.

>      for ( p = 0, written = 0; p < ctx->save.p2m_size; ++p )
>      {
>          if ( !test_bit(p, dirty_bitmap) )
> diff --git a/tools/libxc/xc_sr_save_x86_hvm.c 
> b/tools/libxc/xc_sr_save_x86_hvm.c
> index f3d6cee..aa24f90 100644
> --- a/tools/libxc/xc_sr_save_x86_hvm.c
> +++ b/tools/libxc/xc_sr_save_x86_hvm.c
> @@ -175,6 +175,12 @@ static int x86_hvm_start_of_checkpoint(struct 
> xc_sr_context *ctx)
>      return 0;
>  }
>  
> +static int x86_hvm_check_iteration(struct xc_sr_context *ctx)
> +{
> +    /* no-op */
> +    return 0;
> +}
> +
>  static int x86_hvm_end_of_checkpoint(struct xc_sr_context *ctx)
>  {
>      int rc;
> @@ -221,6 +227,7 @@ struct xc_sr_save_ops save_ops_x86_hvm =
>      .start_of_stream     = x86_hvm_start_of_stream,
>      .start_of_checkpoint = x86_hvm_start_of_checkpoint,
>      .end_of_checkpoint   = x86_hvm_end_of_checkpoint,
> +    .check_iteration     = x86_hvm_check_iteration,
>      .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 0237378..3a58d0d 100644
> --- a/tools/libxc/xc_sr_save_x86_pv.c
> +++ b/tools/libxc/xc_sr_save_x86_pv.c
> @@ -268,6 +268,39 @@ err:
>  }
>  
>  /*
> + * Get p2m_generation count.
> + * Returns an error if the generation count has changed since the last call.
> + */
> +static int get_p2m_generation(struct xc_sr_context *ctx)
> +{
> +    uint64_t p2m_generation;
> +    int rc;
> +
> +    p2m_generation = GET_FIELD(ctx->x86_pv.shinfo, arch.p2m_generation,
> +                               ctx->x86_pv.width);
> +
> +    rc = (p2m_generation == ctx->x86_pv.p2m_generation) ? 0 : -1;
> +    ctx->x86_pv.p2m_generation = p2m_generation;
> +
> +    return rc;
> +}
> +
> +static int x86_pv_check_iteration_p2m_list(struct xc_sr_context *ctx)
> +{
> +    xc_interface *xch = ctx->xch;
> +    int rc;
> +
> +    if ( !ctx->save.live )
> +        return 0;
> +
> +    rc = get_p2m_generation(ctx);
> +    if ( rc )
> +        ERROR("p2m generation count changed. Migration aborted.");
> +
> +    return rc;
> +}
> +
> +/*
>   * Map the guest p2m frames specified via a cr3 value, a virtual address, and
>   * the maximum pfn.
>   */
> @@ -281,6 +314,9 @@ static int map_p2m_list(struct xc_sr_context *ctx, 
> uint64_t p2m_cr3)
>      unsigned fpp, n_pages, level, shift, idx_start, idx_end, idx, saved_idx;
>      int rc = -1;
>  
> +    /* Before each iteration check for local p2m list still valid. */
> +    ctx->save.ops.check_iteration = x86_pv_check_iteration_p2m_list;
> +

This is admittedly the first, but definitely not the only eventual thing
needed for check iteration.  To avoid clobbering one check with another
in the future, it would be cleaner to have a single
x86_pv_check_iteration() which performs the get_p2m_generation() check
iff linear p2m is in use.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel