Re: [Qemu-devel] [PATCH v2 4/4] migration: Add capabilities validation

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Yury Kotov (yury-kotov@yandex-team.ru) wrote:
> Currently we don't check which capabilities set in the source QEMU.
> We just expect that the target QEMU has the same enabled capabilities.
> 
> Add explicit validation for capabilities to make sure that the target VM
> has them too. This is enabled for only new capabilities to keep compatibily.

I'd rather keep the capaiblities on the wire as strings, rather than
indexes; indexes are too delicate.

I guess we also have to think about what happens when new capabilities
are added; what happens when we migrate to an older qemu that doesn't
know about that capability?
What happens when we migrate to a newer capability which thinks you
forgot to send that capability across?

While we're on capabilities, I think you also need to check if the
skip-shared is enabled with postcopy; I don't think they'll work
together.

Dave


> Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
> ---
>  migration/savevm.c | 101 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 101 insertions(+)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 322660438d..9603a38bca 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -57,6 +57,7 @@
>  #include "sysemu/replay.h"
>  #include "qjson.h"
>  #include "migration/colo.h"
> +#include "qemu/bitmap.h"
>  
>  #ifndef ETH_P_RARP
>  #define ETH_P_RARP 0x8035
> @@ -316,6 +317,8 @@ typedef struct SaveState {
>      uint32_t len;
>      const char *name;
>      uint32_t target_page_bits;
> +    uint32_t caps_count;
> +    uint8_t *capabilities;
>  } SaveState;
>  
>  static SaveState savevm_state = {
> @@ -323,15 +326,51 @@ static SaveState savevm_state = {
>      .global_section_id = 0,
>  };
>  
> +static bool should_validate_capability(int capability)
> +{
> +    assert(capability >= 0 && capability < MIGRATION_CAPABILITY__MAX);
> +    /* Validate only new capabilities to keep compatibility. */
> +    switch (capability) {
> +    case MIGRATION_CAPABILITY_X_IGNORE_SHARED:
> +        return true;
> +    default:
> +        return false;
> +    }
> +}
> +
> +static uint32_t get_validatable_capabilities_count(void)
> +{
> +    MigrationState *s = migrate_get_current();
> +    uint32_t result = 0;
> +    int i;
> +    for (i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
> +        if (should_validate_capability(i) && s->enabled_capabilities[i]) {
> +            result++;
> +        }
> +    }
> +    return result;
> +}
> +
>  static int configuration_pre_save(void *opaque)
>  {
>      SaveState *state = opaque;
>      const char *current_name = MACHINE_GET_CLASS(current_machine)->name;
> +    MigrationState *s = migrate_get_current();
> +    int i, j;
>  
>      state->len = strlen(current_name);
>      state->name = current_name;
>      state->target_page_bits = qemu_target_page_bits();
>  
> +    state->caps_count = get_validatable_capabilities_count();
> +    state->capabilities = g_renew(uint8_t, state->capabilities,
> +                                  state->caps_count);
> +    for (i = j = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
> +        if (should_validate_capability(i) && s->enabled_capabilities[i]) {
> +            state->capabilities[j++] = i;
> +        }
> +    }
> +
>      return 0;
>  }
>  
> @@ -347,6 +386,45 @@ static int configuration_pre_load(void *opaque)
>      return 0;
>  }
>  
> +static bool configuration_validate_capabilities(SaveState *state)
> +{
> +    bool ret = true;
> +    MigrationState *s = migrate_get_current();
> +    unsigned long *source_caps_bm;
> +    int i;
> +
> +    source_caps_bm = bitmap_new(MIGRATION_CAPABILITY__MAX);
> +    for (i = 0; i < state->caps_count; i++) {
> +        int capability = state->capabilities[i];
> +        if (capability >= MIGRATION_CAPABILITY__MAX) {
> +            error_report("Received unknown capability %d", capability);
> +            ret = false;
> +        } else {
> +            set_bit(capability, source_caps_bm);
> +        }
> +    }
> +
> +    for (i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
> +        bool source_state, target_state;
> +        if (!should_validate_capability(i)) {
> +            continue;
> +        }
> +        source_state = test_bit(i, source_caps_bm);
> +        target_state = s->enabled_capabilities[i];
> +        if (source_state != target_state) {
> +            error_report("Capability %s is %s, but received capability is %s",
> +                         MigrationCapability_str(i),
> +                         target_state ? "on" : "off",
> +                         source_state ? "on" : "off");
> +            ret = false;
> +            /* Don't break here to report all failed capabilities */
> +        }
> +    }
> +
> +    g_free(source_caps_bm);
> +    return ret;
> +}
> +
>  static int configuration_post_load(void *opaque, int version_id)
>  {
>      SaveState *state = opaque;
> @@ -364,6 +442,10 @@ static int configuration_post_load(void *opaque, int version_id)
>          return -EINVAL;
>      }
>  
> +    if (!configuration_validate_capabilities(state)) {
> +        return -EINVAL;
> +    }
> +
>      return 0;
>  }
>  
> @@ -380,6 +462,11 @@ static bool vmstate_target_page_bits_needed(void *opaque)
>          > qemu_target_page_bits_min();
>  }
>  
> +static bool vmstate_capabilites_needed(void *opaque)
> +{
> +    return get_validatable_capabilities_count() > 0;
> +}
> +
>  static const VMStateDescription vmstate_target_page_bits = {
>      .name = "configuration/target-page-bits",
>      .version_id = 1,
> @@ -391,6 +478,19 @@ static const VMStateDescription vmstate_target_page_bits = {
>      }
>  };
>  
> +static const VMStateDescription vmstate_capabilites = {
> +    .name = "configuration/capabilities",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = vmstate_capabilites_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_V(caps_count, SaveState, 1),
> +        VMSTATE_VARRAY_UINT32_ALLOC(capabilities, SaveState, caps_count, 1,
> +                                    vmstate_info_uint8, uint8_t),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_configuration = {
>      .name = "configuration",
>      .version_id = 1,
> @@ -404,6 +504,7 @@ static const VMStateDescription vmstate_configuration = {
>      },
>      .subsections = (const VMStateDescription*[]) {
>          &vmstate_target_page_bits,
> +        &vmstate_capabilites,
>          NULL
>      }
>  };
> -- 
> 2.20.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK