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

[Juergen Gross <jgross@suse.com>]

On 11/12/15 16:20, Andrew Cooper wrote:
> 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@suse.com>
>> ---
>>  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.

Yes, this is better.

> 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.

Okay.

> 
>> +    /**
>>       * 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.

Okay.

> 
>>      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.

Agreed.


Juergen