Re: [Qemu-devel] [PATCH COLO-Frame v5 11/29] COLO VMstate: Load VM state into qsb before restore it

[Dr. David Alan Gilbert]

* zhanghailiang (address@hidden) wrote:
> We should cache the device state before restore it,
> besides, we should call qemu_system_reset() before load VM state,
> which can ensure the data is intact.

I think the description could be better; to me the important
point is not that it's a 'cache', but the important point is that you
don't destroy the state of the secondary until you are sure that you can
read the whole state from the primary, just in case the primary fails
in the middle of sending the state.

However, other than that:

Reviewed-by: Dr. David Alan Gilbert <address@hidden>

(I suspect you'll need updates as the qemu migration code updates)

Dave

> Note: If we discard qemu_system_reset(), there will be some odd error,
> For exmple, qemu in slave side crashes and reports:
> 
> KVM: entry failed, hardware error 0x7
> EAX=00000000 EBX=0000e000 ECX=00009578 EDX=0000434f
> ESI=0000fc10 EDI=0000434f EBP=00000000 ESP=00001fca
> EIP=00009594 EFL=00010246 [---Z-P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> ES =0040 00000400 0000ffff 00009300
> CS =f000 000f0000 0000ffff 00009b00
> SS =434f 000434f0 0000ffff 00009300
> DS =434f 000434f0 0000ffff 00009300
> FS =0000 00000000 0000ffff 00009300
> GS =0000 00000000 0000ffff 00009300
> LDT=0000 00000000 0000ffff 00008200
> TR =0000 00000000 0000ffff 00008b00
> GDT=     0002dcc8 00000047
> IDT=     00000000 0000ffff
> CR0=00000010 CR2=ffffffff CR3=00000000 CR4=00000000
> DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 
> DR3=0000000000000000
> DR6=00000000ffff0ff0 DR7=0000000000000400
> EFER=0000000000000000
> Code=c0 74 0f 66 b9 78 95 00 00 66 31 d2 66 31 c0 e9 47 e0 fb 90 <f3> 90 fa 
> fc 66 c3 66 53 66 89 c3 66 e8 9d e8 ff ff 66 01 c3 66 89 d8 66 e8 40 e9 ff ff 
> 66
> ERROR: invalid runstate transition: 'internal-error' -> 'colo'
> 
> The reason is, some of the device state will be ignored when saving device 
> state to slave,
> if the corresponding data is in its initial value, such as 0.
> But the device state in slave maybe in initialized value, after a loop of 
> checkpoint,
> there will be inconsistent for the value of device state.
> This will happen when the PVM reboot or SVM run ahead of PVM in the startup 
> process.
> 
> Signed-off-by: zhanghailiang <address@hidden>
> Signed-off-by: Yang Hongyang <address@hidden>
> Signed-off-by: Gonglei <address@hidden>
> ---
>  migration/colo.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 50 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c
> index 39cd698..0f61786 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -309,8 +309,10 @@ void *colo_process_incoming_checkpoints(void *opaque)
>      struct colo_incoming *colo_in = opaque;
>      QEMUFile *f = colo_in->file;
>      int fd = qemu_get_fd(f);
> -    QEMUFile *ctl = NULL;
> +    QEMUFile *ctl = NULL, *fb = NULL;
>      int ret;
> +    uint64_t total_size;
> +
>      colo = qemu_coroutine_self();
>      assert(colo != NULL);
>  
> @@ -325,10 +327,17 @@ void *colo_process_incoming_checkpoints(void *opaque)
>          goto out;
>      }
>  
> +    colo_buffer = qsb_create(NULL, COLO_BUFFER_BASE_SIZE);
> +    if (colo_buffer == NULL) {
> +        error_report("Failed to allocate colo buffer!");
> +        goto out;
> +    }
> +
>      ret = colo_ctl_put(ctl, COLO_CHECPOINT_READY);
>      if (ret < 0) {
>          goto out;
>      }
> +
>      qemu_mutex_lock_iothread();
>      /* in COLO mode, slave is runing, so start the vm */
>      vm_start();
> @@ -364,7 +373,18 @@ void *colo_process_incoming_checkpoints(void *opaque)
>          }
>          trace_colo_receive_message("COLO_CHECKPOINT_SEND");
>  
> -        /*TODO Load VM state */
> +        /* read the VM state total size first */
> +        ret = colo_ctl_get_value(f, &total_size);
> +        if (ret < 0) {
> +            goto out;
> +        }
> +
> +        /* read vm device state into colo buffer */
> +        ret = qsb_fill_buffer(colo_buffer, f, total_size);
> +        if (ret != total_size) {
> +            error_report("can't get all migration data");
> +            goto out;
> +        }
>  
>          ret = colo_ctl_put(ctl, COLO_CHECKPOINT_RECEIVED);
>          if (ret < 0) {
> @@ -372,6 +392,22 @@ void *colo_process_incoming_checkpoints(void *opaque)
>          }
>          trace_colo_receive_message("COLO_CHECKPOINT_RECEIVED");
>  
> +        /* open colo buffer for read */
> +        fb = qemu_bufopen("r", colo_buffer);
> +        if (!fb) {
> +            error_report("can't open colo buffer for read");
> +            goto out;
> +        }
> +
> +        qemu_mutex_lock_iothread();
> +        qemu_system_reset(VMRESET_SILENT);
> +        if (qemu_loadvm_state(fb) < 0) {
> +            error_report("COLO: loadvm failed");
> +            qemu_mutex_unlock_iothread();
> +            goto out;
> +        }
> +        qemu_mutex_unlock_iothread();
> +
>          /* TODO: flush vm state */
>  
>          ret = colo_ctl_put(ctl, COLO_CHECKPOINT_LOADED);
> @@ -384,14 +420,25 @@ void *colo_process_incoming_checkpoints(void *opaque)
>          vm_start();
>          qemu_mutex_unlock_iothread();
>          trace_colo_vm_state_change("stop", "start");
> -}
> +
> +        qemu_fclose(fb);
> +        fb = NULL;
> +    }
>  
>  out:
>      colo = NULL;
> +
> +    if (fb) {
> +        qemu_fclose(fb);
> +    }
> +
>      release_ram_cache();
>      if (ctl) {
>          qemu_fclose(ctl);
>      }
> +
> +    qsb_free(colo_buffer);
> +
>      loadvm_exit_colo();
>  
>      return NULL;
> -- 
> 1.7.12.4
> 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK