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

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

[Stefano Stabellini]

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.

[...]


> 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?



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

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

[Wen Congyang]

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.

> 
> [...]
> 
> 
>> 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.

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
>>
> .
> 

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

[Stefano Stabellini]

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

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

[Wen Congyang]

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
>>>>
>>> .
>>>
>>
> .
>