Re: [Xen-devel] [RFC Patch v3 23/22] Introduce "xen-load-devices-state"

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 9 Sep 2014, Wen Congyang wrote:
> At 09/06/2014 05:57 AM, Stefano Stabellini Write:
> > On Fri, 5 Sep 2014, Wen Congyang wrote:
> >> introduce a "xen-load-devices-state" QAPI command that can be used to load
> >> the state of all devices, but not the RAM or the block devices of the
> >> VM.
> > 
> > Hello Wen,
> > please CC qemu-devel too for QEMU patches.
> > 
> > Could you please explain why do you need this command?
> > 
> > Is the main issue that there are no QMP commands today to load the state
> > of a VM? It looks like that for vm restore we are using the -incoming
> > command line option, but there is no alternative over QMP.
> 
> We only have hmp commands savevm/loadvm, and qmp commands 
> xen-save-devices-state.
> 
> We use this new command for COLO:
> 1. suspend both primay vm and secondary vm
> 2. sync the state
> 3. resume both primary vm and secondary vm
> 
> In such case, we need to update all devices's state in any time.

OK. Please state this in the commit message.


> > [...]
> > 
> > 
> >> diff --git a/savevm.c b/savevm.c
> >> index 22123be..c6aa502 100644
> >> --- a/savevm.c
> >> +++ b/savevm.c
> >> @@ -863,6 +863,105 @@ out:
> >>      return ret;
> >>  }
> >>  
> >> +static int qemu_load_devices_state(QEMUFile *f)
> >> +{
> >> +    uint8_t section_type;
> >> +    unsigned int v;
> >> +    int ret;
> >> +
> >> +    if (qemu_savevm_state_blocked(NULL)) {
> >> +        return -EINVAL;
> >> +    }
> >> +
> >> +    v = qemu_get_be32(f);
> >> +    if (v != QEMU_VM_FILE_MAGIC) {
> >> +        return -EINVAL;
> >> +    }
> >> +
> >> +    v = qemu_get_be32(f);
> >> +    if (v == QEMU_VM_FILE_VERSION_COMPAT) {
> >> +        fprintf(stderr, "SaveVM v2 format is obsolete and don't work 
> >> anymore\n");
> >> +        return -ENOTSUP;
> >> +    }
> >> +    if (v != QEMU_VM_FILE_VERSION) {
> >> +        return -ENOTSUP;
> >> +    }
> >> +
> >> +    while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) {
> >> +        uint32_t instance_id, version_id, section_id;
> >> +        SaveStateEntry *se;
> >> +        char idstr[257];
> >> +        int len;
> >> +
> >> +        switch (section_type) {
> >> +        case QEMU_VM_SECTION_FULL:
> >> +            /* Read section start */
> >> +            section_id = qemu_get_be32(f);
> >> +            len = qemu_get_byte(f);
> >> +            qemu_get_buffer(f, (uint8_t *)idstr, len);
> >> +            idstr[len] = 0;
> >> +            instance_id = qemu_get_be32(f);
> >> +            version_id = qemu_get_be32(f);
> >> +
> >> +            /* Find savevm section */
> >> +            se = find_se(idstr, instance_id);
> >> +            if (se == NULL) {
> >> +                fprintf(stderr, "Unknown savevm section or instance '%s' 
> >> %d\n",
> >> +                        idstr, instance_id);
> >> +                ret = -EINVAL;
> >> +                goto out;
> >> +            }
> >> +
> >> +            /* Validate version */
> >> +            if (version_id > se->version_id) {
> >> +                fprintf(stderr, "loadvm: unsupported version %d for '%s' 
> >> v%d\n",
> >> +                        version_id, idstr, se->version_id);
> >> +                ret = -EINVAL;
> >> +                goto out;
> >> +            }
> >> +
> >> +            /* Validate if it is a device's state */
> >> +            if (se->is_ram) {
> >> +                fprintf(stderr, "loadvm: %s is not devices state\n", 
> >> idstr);
> >> +                ret = -EINVAL;
> >> +                goto out;
> >> +            }
> >> +
> >> +            ret = vmstate_load(f, se, version_id);
> >> +            if (ret < 0) {
> >> +                fprintf(stderr, "qemu: warning: error while loading state 
> >> for instance 0x%x of device '%s'\n",
> >> +                        instance_id, idstr);
> >> +                goto out;
> >> +            }
> >> +            break;
> >> +        case QEMU_VM_SECTION_START:
> >> +        case QEMU_VM_SECTION_PART:
> >> +        case QEMU_VM_SECTION_END:
> >> +            /*
> >> +             * The file is saved by the command xen-save-devices-state,
> >> +             * So it should not contain section start/part/end.
> >> +             */
> >> +        default:
> >> +            fprintf(stderr, "Unknown savevm section type %d\n", 
> >> section_type);
> >> +            ret = -EINVAL;
> >> +            goto out;
> >> +        }
> >> +    }
> >> +
> >> +    cpu_synchronize_all_post_init();
> >> +
> >> +    ret = 0;
> >> +
> >> +out:
> >> +    if (ret == 0) {
> >> +        if (qemu_file_get_error(f)) {
> >> +            ret = -EIO;
> >> +        }
> >> +    }
> >> +
> >> +    return ret;
> >> +}
> > 
> > Assuming that the state file only contains device states, why don't you
> > just call qemu_loadvm_state to implement the command?
> 
> Do you mean there is no need to check the file? If the file contains
> some memory, it will cause unexpected problem.

I would prefer to avoid duplicating qemu_loadvm_state just to add an
if (se->is_ram) check.
I would rather introduce a generic loadvm QMP command and rely on the
fact that we are saving only device states via xen-save-devices-state.

Given that loading memory in QEMU on Xen always leads to errors, maybe
we could still add a check in qemu_loadvm_state anyway. Something like:

diff --git a/savevm.c b/savevm.c
index e19ae0a..759eefa 100644
--- a/savevm.c
+++ b/savevm.c
@@ -938,6 +938,13 @@ int qemu_loadvm_state(QEMUFile *f)
                 goto out;
             }
 
+            /* Validate if it is a device's state */
+            if (xen_enabled() && se->is_ram) {
+                fprintf(stderr, "loadvm: %s RAM loading not allowed on Xen\n", 
idstr);
+                ret = -EINVAL;
+                goto out;
+            }
+
             /* Add entry */
             le = g_malloc0(sizeof(*le));
 



> Thanks
> Wen Congyang
> 
> > 
> > 
> > 
> >>  static BlockDriverState *find_vmstate_bs(void)
> >>  {
> >>      BlockDriverState *bs = NULL;
> >> @@ -1027,6 +1126,33 @@ void qmp_xen_save_devices_state(const char 
> >> *filename, Error **errp)
> >>      }
> >>  }
> >>  
> >> +void qmp_xen_load_devices_state(const char *filename, Error **errp)
> >> +{
> >> +    QEMUFile *f;
> >> +    int saved_vm_running;
> >> +    int ret;
> >> +
> >> +    saved_vm_running = runstate_is_running();
> >> +    vm_stop(RUN_STATE_RESTORE_VM);
> >> +
> >> +    f = qemu_fopen(filename, "rb");
> >> +    if (!f) {
> >> +        error_setg_file_open(errp, errno, filename);
> >> +        goto out;
> >> +    }
> >> +
> >> +    ret = qemu_load_devices_state(f);
> >> +    qemu_fclose(f);
> >> +    if (ret < 0) {
> >> +        error_set(errp, QERR_IO_ERROR);
> >> +    }
> >> +
> >> +out:
> >> +    if (saved_vm_running) {
> >> +        vm_start();
> >> +    }
> >> +}
> >> +
> >>  int load_vmstate(const char *name)
> >>  {
> >>      BlockDriverState *bs, *bs_vm_state;
> >> -- 
> >> 1.9.0
> >>
> >>
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@xxxxxxxxxxxxx
> >> http://lists.xen.org/xen-devel
> >>
> > .
> > 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel