Re: [RFC PATCH v1 20/26] kvm: vmi: intercept live migration

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

* Adalbert Lazăr (alazar@bitdefender.com) wrote:
> From: Marian Rotariu <marian.c.rotariu@gmail.com>
> 
> It is possible that the introspection tool has made some changes inside
> the introspected VM which can make the guest crash if the introspection
> connection is suddenly closed.
> 
> When the live migration starts, for now, the introspection tool is
> signaled to remove its hooks from the introspected VM.
> 
> CC: Juan Quintela <quintela@redhat.com>
> CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Signed-off-by: Marian Rotariu <marian.c.rotariu@gmail.com>
> Signed-off-by: Adalbert Lazăr <alazar@bitdefender.com>

OK, so this isn't too intrusive to the migration code; and other than
renaming 'start_live_migration_thread' to
'start_outgoing_migration_thread' I think I'd be OK with this,

but it might depend what your overall aim is.

For example, you might be better intercepting each migration_state
change in your notifier, that's much finer grain than just the start of
migration.

The other thing I worry about is that there doesn't seem to be much
guard against odd orderings of things - for example, what happens
if the introspection client was to issue the  INTERCEPT_MIGRATE command
twice while a migration was already running?  Or before an actual
incoming channel connetion had happened?

Dave

> ---
>  accel/kvm/vmi.c                | 31 +++++++++++++++++++++++++++----
>  include/sysemu/vmi-intercept.h |  1 +
>  migration/migration.c          | 18 +++++++++++++++---
>  migration/migration.h          |  2 ++
>  4 files changed, 45 insertions(+), 7 deletions(-)
> 
> diff --git a/accel/kvm/vmi.c b/accel/kvm/vmi.c
> index 90906478b4..ea7191e48d 100644
> --- a/accel/kvm/vmi.c
> +++ b/accel/kvm/vmi.c
> @@ -21,6 +21,8 @@
>  #include "chardev/char.h"
>  #include "chardev/char-fe.h"
>  #include "migration/vmstate.h"
> +#include "migration/migration.h"
> +#include "migration/misc.h"
>  
>  #include "sysemu/vmi-intercept.h"
>  #include "sysemu/vmi-handshake.h"
> @@ -58,6 +60,7 @@ typedef struct VMIntrospection {
>      int64_t vm_start_time;
>  
>      Notifier machine_ready;
> +    Notifier migration_state_change;
>      bool created_from_command_line;
>  
>      bool kvmi_hooked;
> @@ -74,9 +77,11 @@ static const char *action_string[] = {
>      "suspend",
>      "resume",
>      "force-reset",
> +    "migrate",
>  };
>  
>  static bool suspend_pending;
> +static bool migrate_pending;
>  
>  #define TYPE_VM_INTROSPECTION "introspection"
>  
> @@ -88,6 +93,15 @@ static bool suspend_pending;
>  static Error *vm_introspection_init(VMIntrospection *i);
>  static void vm_introspection_reset(void *opaque);
>  
> +static void migration_state_notifier(Notifier *notifier, void *data)
> +{
> +    MigrationState *s = data;
> +
> +    if (migration_has_failed(s)) {
> +        migrate_pending = false;
> +    }
> +}
> +
>  static void machine_ready(Notifier *notifier, void *data)
>  {
>      VMIntrospection *i = container_of(notifier, VMIntrospection, machine_ready);
> @@ -144,6 +158,9 @@ static void complete(UserCreatable *uc, Error **errp)
>  
>      ic->uniq = i;
>  
> +    i->migration_state_change.notify = migration_state_notifier;
> +    add_migration_state_change_notifier(&i->migration_state_change);
> +
>      qemu_register_reset(vm_introspection_reset, i);
>  }
>  
> @@ -478,6 +495,9 @@ static void continue_with_the_intercepted_action(VMIntrospection *i)
>      case VMI_INTERCEPT_SUSPEND:
>          vm_stop(RUN_STATE_PAUSED);
>          break;
> +    case VMI_INTERCEPT_MIGRATE:
> +        start_live_migration_thread(migrate_get_current());
> +        break;
>      default:
>          error_report("VMI: %s: unexpected action %d",
>                       __func__, i->intercepted_action);
> @@ -571,9 +591,9 @@ static void chr_event_open(VMIntrospection *i)
>  {
>      Error *local_err = NULL;
>  
> -    if (suspend_pending) {
> -        info_report("VMI: %s: too soon (suspend=%d)",
> -                    __func__, suspend_pending);
> +    if (suspend_pending || migrate_pending) {
> +        info_report("VMI: %s: too soon (suspend=%d, migrate=%d)",
> +                    __func__, suspend_pending, migrate_pending);
>          maybe_disable_socket_reconnect(i);
>          qemu_chr_fe_disconnect(&i->sock);
>          return;
> @@ -608,7 +628,7 @@ static void chr_event_close(VMIntrospection *i)
>      cancel_unhook_timer(i);
>      cancel_handshake_timer(i);
>  
> -    if (suspend_pending) {
> +    if (suspend_pending || migrate_pending) {
>          maybe_disable_socket_reconnect(i);
>  
>          if (i->intercepted_action != VMI_INTERCEPT_NONE) {
> @@ -680,6 +700,9 @@ static bool record_intercept_action(VMI_intercept_command action)
>          break;
>      case VMI_INTERCEPT_FORCE_RESET:
>          break;
> +    case VMI_INTERCEPT_MIGRATE:
> +        migrate_pending = true;
> +        break;
>      default:
>          return false;
>      }
> diff --git a/include/sysemu/vmi-intercept.h b/include/sysemu/vmi-intercept.h
> index ef591b49e7..b4a9a3faa7 100644
> --- a/include/sysemu/vmi-intercept.h
> +++ b/include/sysemu/vmi-intercept.h
> @@ -15,6 +15,7 @@ typedef enum {
>      VMI_INTERCEPT_SUSPEND,
>      VMI_INTERCEPT_RESUME,
>      VMI_INTERCEPT_FORCE_RESET,
> +    VMI_INTERCEPT_MIGRATE,
>  } VMI_intercept_command;
>  
>  bool vm_introspection_intercept(VMI_intercept_command ic, Error **errp);
> diff --git a/migration/migration.c b/migration/migration.c
> index 187ac0410c..222037d739 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -55,6 +55,8 @@
>  #include "qemu/queue.h"
>  #include "multifd.h"
>  
> +#include "sysemu/vmi-intercept.h"
> +
>  #define MAX_THROTTLE  (32 << 20)      /* Migration transfer speed throttling */
>  
>  /* Amount of time to allocate to each "chunk" of bandwidth-throttled
> @@ -3471,6 +3473,13 @@ static void *migration_thread(void *opaque)
>      return NULL;
>  }
>  
> +void start_live_migration_thread(MigrationState *s)
> +{
> +    qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
> +                    QEMU_THREAD_JOINABLE);
> +    s->migration_thread_running = true;
> +}
> +
>  void migrate_fd_connect(MigrationState *s, Error *error_in)
>  {
>      Error *local_err = NULL;
> @@ -3534,9 +3543,12 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>          migrate_fd_cleanup(s);
>          return;
>      }
> -    qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
> -                       QEMU_THREAD_JOINABLE);
> -    s->migration_thread_running = true;
> +
> +    if (vm_introspection_intercept(VMI_INTERCEPT_MIGRATE, &error_in)) {
> +        return;
> +    }
> +
> +    start_live_migration_thread(s);
>  }
>  
>  void migration_global_dump(Monitor *mon)
> diff --git a/migration/migration.h b/migration/migration.h
> index 507284e563..eb5668e1f2 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -263,6 +263,8 @@ struct MigrationState
>      uint8_t clear_bitmap_shift;
>  };
>  
> +void start_live_migration_thread(MigrationState *s);
> +
>  void migrate_set_state(int *state, int old_state, int new_state);
>  
>  void migration_fd_process_incoming(QEMUFile *f, Error **errp);
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK