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

[Wen Congyang <wency@xxxxxxxxxxxxxx>]

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


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