* Yury Kotov (yury-ko...@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.
>
> Signed-off-by: Yury Kotov <yury-ko...@yandex-team.ru>
Yes, I think that's OK.
(We might want to find a way to move existing capabilities into the list
of validated capabilities on new enough machine types; e.g. say
postcopy-ram but only check it on new machine types so we don't break
compatibility)
Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
> ---
> migration/savevm.c | 137 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 137 insertions(+)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 322660438d..a721cf5868 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;
> + MigrationCapability *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(MigrationCapability, 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,40 @@ 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++) {
> + MigrationCapability capability = state->capabilities[i];
> + 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,9 +437,53 @@ static int configuration_post_load(void *opaque, int
> version_id)
> return -EINVAL;
> }
>
> + if (!configuration_validate_capabilities(state)) {
> + return -EINVAL;
> + }
> +
> return 0;
> }
>
> +static int get_capability(QEMUFile *f, void *pv, size_t size,
> + const VMStateField *field)
> +{
> + MigrationCapability *capability = pv;
> + char capability_str[UINT8_MAX + 1];
> + uint8_t len;
> + int i;
> +
> + len = qemu_get_byte(f);
> + qemu_get_buffer(f, (uint8_t *)capability_str, len);
> + capability_str[len] = '\0';
> + for (i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
> + if (!strcmp(MigrationCapability_str(i), capability_str)) {
> + *capability = i;
> + return 0;
> + }
> + }
> + error_report("Received unknown capability %s", capability_str);
> + return -EINVAL;
> +}
> +
> +static int put_capability(QEMUFile *f, void *pv, size_t size,
> + const VMStateField *field, QJSON *vmdesc)
> +{
> + MigrationCapability *capability = pv;
> + const char *capability_str = MigrationCapability_str(*capability);
> + size_t len = strlen(capability_str);
> + assert(len <= UINT8_MAX);
> +
> + qemu_put_byte(f, len);
> + qemu_put_buffer(f, (uint8_t *)capability_str, len);
> + return 0;
> +}
> +
> +static const VMStateInfo vmstate_info_capability = {
> + .name = "capability",
> + .get = get_capability,
> + .put = put_capability,
> +};
> +
> /* The target-page-bits subsection is present only if the
> * target page size is not the same as the default (ie the
> * minimum page size for a variable-page-size guest CPU).
> @@ -380,6 +497,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 +513,20 @@ 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_capability,
> + MigrationCapability),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> static const VMStateDescription vmstate_configuration = {
> .name = "configuration",
> .version_id = 1,
> @@ -404,6 +540,7 @@ static const VMStateDescription vmstate_configuration = {
> },
> .subsections = (const VMStateDescription*[]) {
> &vmstate_target_page_bits,
> + &vmstate_capabilites,
> NULL
> }
> };
> --
> 2.20.1
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
