From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54730) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XRwX3-0005bs-DD for qemu-devel@nongnu.org; Thu, 11 Sep 2014 01:03:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XRwWy-0002jc-DQ for qemu-devel@nongnu.org; Thu, 11 Sep 2014 01:02:57 -0400 Received: from [59.151.112.132] (port=35324 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XRwWx-0002gv-Gt for qemu-devel@nongnu.org; Thu, 11 Sep 2014 01:02:52 -0400 Message-ID: <54112D10.9060207@cn.fujitsu.com> Date: Thu, 11 Sep 2014 13:03:12 +0800 From: Wen Congyang MIME-Version: 1.0 References: <1409909158-19243-1-git-send-email-wency@cn.fujitsu.com> <1409909158-19243-24-git-send-email-wency@cn.fujitsu.com> <540E6A44.8090507@cn.fujitsu.com> In-Reply-To: Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Xen-devel] [RFC Patch v3 23/22] Introduce "xen-load-devices-state" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: Ian Campbell , Ian Jackson , Jiang Yunhong , Dong Eddie , qemu-devel@nongnu.org, xen devel , Paolo Bonzini , Yang Hongyang , Lai Jiangshan 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 >>>> >>> . >>> >> > . >