Re: [PATCH RFC 4/5] cpu: Allow cpu_synchronize_all_post_init() to take an errp

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

* Peter Xu (peterx@redhat.com) wrote:
> Allow cpu_synchronize_all_post_init() to fail with an errp when it's set.
> Modify both precopy and postcopy to try to detect such error.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/core/machine.c     |  2 +-
>  include/sysemu/cpus.h |  2 +-
>  migration/savevm.c    | 20 +++++++++++++++++---
>  softmmu/cpus.c        |  2 +-
>  4 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index c53548d0b1..b5daad82f8 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1447,7 +1447,7 @@ void qemu_remove_machine_init_done_notifier(Notifier *notify)
>  
>  void qdev_machine_creation_done(void)
>  {
> -    cpu_synchronize_all_post_init();
> +    cpu_synchronize_all_post_init(NULL);
>  
>      if (current_machine->boot_config.has_once) {
>          qemu_boot_set(current_machine->boot_config.once, &error_fatal);
> diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
> index b5c87d48b3..a51ee46441 100644
> --- a/include/sysemu/cpus.h
> +++ b/include/sysemu/cpus.h
> @@ -45,7 +45,7 @@ bool cpus_are_resettable(void);
>  
>  void cpu_synchronize_all_states(void);
>  void cpu_synchronize_all_post_reset(void);
> -void cpu_synchronize_all_post_init(void);
> +void cpu_synchronize_all_post_init(Error **errp);
>  void cpu_synchronize_all_pre_loadvm(void);
>  
>  #ifndef CONFIG_USER_ONLY
> diff --git a/migration/savevm.c b/migration/savevm.c
> index d9076897b8..1175ddefd4 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2005,7 +2005,17 @@ static void loadvm_postcopy_handle_run_bh(void *opaque)
>      /* TODO we should move all of this lot into postcopy_ram.c or a shared code
>       * in migration.c
>       */
> -    cpu_synchronize_all_post_init();
> +    cpu_synchronize_all_post_init(&local_err);
> +    if (local_err) {
> +        /*
> +         * TODO: a better way to do this is to tell the src that we cannot
> +         * run the VM here so hopefully we can keep the VM running on src
> +         * and immediately halt the switch-over.  But that needs work.

Yes, I think it is possible; unlike some of the later errors in the same
function, in this case we know no disks/network/etc have been touched,
so we should be able to recover.
I wonder if we can move the postcopy_state_set(POSTCOPY_INCOMING_RUNNING)
out of loadvm_postcopy_handle_run to after this point.

We've already got the return path, so we should be able to signal the
failure unless we're very unlucky.

Dave

> +         */
> +        error_report_err(local_err);
> +        local_err = NULL;
> +        autostart = false;
> +    }
>  
>      trace_loadvm_postcopy_handle_run_bh("after cpu sync");
>  
> @@ -2772,7 +2782,11 @@ int qemu_loadvm_state(QEMUFile *f)
>      }
>  
>      qemu_loadvm_state_cleanup();
> -    cpu_synchronize_all_post_init();
> +    cpu_synchronize_all_post_init(&local_err);
> +    if (local_err) {
> +        error_report_err(local_err);
> +        return -EINVAL;
> +    }
>  
>      return ret;
>  }
> @@ -2789,7 +2803,7 @@ int qemu_load_device_state(QEMUFile *f)
>          return ret;
>      }
>  
> -    cpu_synchronize_all_post_init();
> +    cpu_synchronize_all_post_init(NULL);
>      return 0;
>  }
>  
> diff --git a/softmmu/cpus.c b/softmmu/cpus.c
> index 464c06201c..59c70fd496 100644
> --- a/softmmu/cpus.c
> +++ b/softmmu/cpus.c
> @@ -146,7 +146,7 @@ void cpu_synchronize_all_post_reset(void)
>      }
>  }
>  
> -void cpu_synchronize_all_post_init(void)
> +void cpu_synchronize_all_post_init(Error **errp)
>  {
>      CPUState *cpu;
>  
> -- 
> 2.32.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK