[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Xen-devel] [RFC Patch v3 23/22] Introduce "xen-load-de
From: |
Stefano Stabellini |
Subject: |
Re: [Qemu-devel] [Xen-devel] [RFC Patch v3 23/22] Introduce "xen-load-devices-state" |
Date: |
Wed, 10 Sep 2014 20:15:18 +0100 |
User-agent: |
Alpine 2.02 (DEB 1266 2009-07-14) |
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
> >> address@hidden
> >> http://lists.xen.org/xen-devel
> >>
> > .
> >
>