diff mbox

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

Message ID alpine.DEB.2.02.1409102005450.8137@kaball.uk.xensource.com
State New
Headers show

Commit Message

Stefano Stabellini Sept. 10, 2014, 7:15 p.m. UTC
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:




> 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@lists.xen.org
> >> http://lists.xen.org/xen-devel
> >>
> > .
> > 
>

Comments

Wen Congyang Sept. 11, 2014, 5:03 a.m. UTC | #1
On 09/11/2014 03:15 AM, Stefano Stabellini wrote:
> 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, I think it works for me.

Wen Congyang

>  
> 
> 
> 
>> 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@lists.xen.org
>>>> http://lists.xen.org/xen-devel
>>>>
>>> .
>>>
>>
> .
>
diff mbox

Patch

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));