[Qemu-devel] [PATCH v5 0/1] Introduce "xen-load-devices-state"

[Qemu-devel] [PATCH v5 0/1] Introduce "xen-load-devices-state"

[Changlong Xie]

Changelog
v5:
1. Introduce qio channel since 8925839f
v4:
1. Rebased to the lastest code
v3:
1. Addressed on David's commets, fix a bug
v2:
1. Rebased to the lastest code
2. Addressed on Eric's comments, fixed coding style


Wen Congyang (1):
  Introduce "xen-load-devices-state"

 migration/savevm.c | 37 +++++++++++++++++++++++++++++++++++++
 qapi-schema.json   | 14 ++++++++++++++
 qmp-commands.hx    | 27 +++++++++++++++++++++++++++
 3 files changed, 78 insertions(+)

-- 
1.9.3

[Qemu-devel] [PATCH v5 1/1] Introduce "xen-load-devices-state"

[Changlong Xie]

From: Wen Congyang <wency@cn.fujitsu.com>

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.

We only have hmp commands savevm/loadvm, and qmp commands
xen-save-devices-state.

We use this new command for COLO:
1. suspend both primary 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' state in any time.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
---
 migration/savevm.c | 37 +++++++++++++++++++++++++++++++++++++
 qapi-schema.json   | 14 ++++++++++++++
 qmp-commands.hx    | 27 +++++++++++++++++++++++++++
 3 files changed, 78 insertions(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index 6c21231..e144b8f 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -31,6 +31,7 @@
 #include "hw/boards.h"
 #include "hw/hw.h"
 #include "hw/qdev.h"
+#include "hw/xen/xen.h"
 #include "net/net.h"
 #include "monitor/monitor.h"
 #include "sysemu/sysemu.h"
@@ -1754,6 +1755,12 @@ qemu_loadvm_section_start_full(QEMUFile *f, MigrationIncomingState *mis)
         return -EINVAL;
     }
 
+    /* Validate if it is a device's state */
+    if (xen_enabled() && se->is_ram) {
+        error_report("loadvm: %s RAM loading not allowed on Xen", idstr);
+        return -EINVAL;
+    }
+
     /* Add entry */
     le = g_malloc0(sizeof(*le));
 
@@ -2064,6 +2071,36 @@ void qmp_xen_save_devices_state(const char *filename, Error **errp)
     }
 }
 
+void qmp_xen_load_devices_state(const char *filename, Error **errp)
+{
+    QEMUFile *f;
+    QIOChannelFile *ioc;
+    int ret;
+
+    /* Guest must be paused before loading the device state; the RAM state
+     * will already have been loaded by xc
+     */
+    if (runstate_is_running()) {
+        error_setg(errp, "Cannot update device state while vm is running");
+        return;
+    }
+    vm_stop(RUN_STATE_RESTORE_VM);
+
+    ioc = qio_channel_file_new_path(filename, O_WRONLY | O_CREAT, 0660, errp);
+    if (!ioc) {
+        return;
+    }
+    f = qemu_fopen_channel_output(QIO_CHANNEL(ioc));
+
+    migration_incoming_state_new(f);
+    ret = qemu_loadvm_state(f);
+    qemu_fclose(f);
+    if (ret < 0) {
+        error_setg(errp, QERR_IO_ERROR);
+    }
+    migration_incoming_state_destroy();
+}
+
 int load_vmstate(const char *name)
 {
     BlockDriverState *bs, *bs_vm_state;
diff --git a/qapi-schema.json b/qapi-schema.json
index 8483bdf..48c3a6f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4201,6 +4201,20 @@
   'data': [ 'none', 'record', 'play' ] }
 
 ##
+# @xen-load-devices-state:
+#
+# Load the state of all devices from file. The RAM and the block devices
+# of the VM are not loaded by this command.
+#
+# @filename: the file to load the state of the devices from as binary
+# data. See xen-save-devices-state.txt for a description of the binary
+# format.
+#
+# Since: 2.7
+##
+{ 'command': 'xen-load-devices-state', 'data': {'filename': 'str'} }
+
+##
 # @GICCapability:
 #
 # The struct describes capability for a specific GIC (Generic
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 28801a2..780e7f2 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -587,6 +587,33 @@ Example:
 EQMP
 
     {
+        .name       = "xen-load-devices-state",
+        .args_type  = "filename:F",
+        .mhandler.cmd_new = qmp_marshal_xen_load_devices_state,
+    },
+
+SQMP
+xen-load-devices-state
+----------------------
+
+Load the state of all devices from file. The RAM and the block devices
+of the VM are not loaded by this command.
+
+Arguments:
+
+- "filename": the file to load the state of the devices from as binary
+data. See xen-save-devices-state.txt for a description of the binary
+format.
+
+Example:
+
+-> { "execute": "xen-load-devices-state",
+     "arguments": { "filename": "/tmp/resume" } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "xen-set-global-dirty-log",
         .args_type  = "enable:b",
         .mhandler.cmd_new = qmp_marshal_xen_set_global_dirty_log,
-- 
1.9.3

Re: [Qemu-devel] [PATCH v5 1/1] Introduce "xen-load-devices-state"

[Anthony PERARD]

On Thu, Jun 02, 2016 at 06:36:46PM +0800, Changlong Xie wrote:
> +void qmp_xen_load_devices_state(const char *filename, Error **errp)
> +{
> +    QEMUFile *f;
> +    QIOChannelFile *ioc;
> +    int ret;
> +
> +    /* Guest must be paused before loading the device state; the RAM state
> +     * will already have been loaded by xc
> +     */
> +    if (runstate_is_running()) {
> +        error_setg(errp, "Cannot update device state while vm is running");
> +        return;
> +    }
> +    vm_stop(RUN_STATE_RESTORE_VM);
> +
> +    ioc = qio_channel_file_new_path(filename, O_WRONLY | O_CREAT, 0660, errp);

This does not look right, it looks like it's going to open the file
to write to it. You probably want O_RDONLY, also I don't think the
O_CREAT flag is needed. (and without O_WRONLY, mode can be 0 instead of
0660.)

> +    if (!ioc) {
> +        return;
> +    }
> +    f = qemu_fopen_channel_output(QIO_CHANNEL(ioc));

I'm not sure, but I guess here you want qemu_fopen_channel_input here.

Thanks,

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH v5 1/1] Introduce "xen-load-devices-state"

[Changlong Xie]

On 06/02/2016 11:14 PM, Anthony PERARD wrote:
> On Thu, Jun 02, 2016 at 06:36:46PM +0800, Changlong Xie wrote:
>> +void qmp_xen_load_devices_state(const char *filename, Error **errp)
>> +{
>> +    QEMUFile *f;
>> +    QIOChannelFile *ioc;
>> +    int ret;
>> +
>> +    /* Guest must be paused before loading the device state; the RAM state
>> +     * will already have been loaded by xc
>> +     */
>> +    if (runstate_is_running()) {
>> +        error_setg(errp, "Cannot update device state while vm is running");
>> +        return;
>> +    }
>> +    vm_stop(RUN_STATE_RESTORE_VM);
>> +
>> +    ioc = qio_channel_file_new_path(filename, O_WRONLY | O_CREAT, 0660, errp);
>
> This does not look right, it looks like it's going to open the file
> to write to it. You probably want O_RDONLY, also I don't think the
> O_CREAT flag is needed. (and without O_WRONLY, mode can be 0 instead of
> 0660.)
>

Yes, as you said. We should use 0_RDONLY for open(2), so mode should be 0.

Thanks
	-Xie

>> +    if (!ioc) {
>> +        return;
>> +    }
>> +    f = qemu_fopen_channel_output(QIO_CHANNEL(ioc));
>
> I'm not sure, but I guess here you want qemu_fopen_channel_input here.
>
> Thanks,
>

Re: [Qemu-devel] [PATCH v5 1/1] Introduce "xen-load-devices-state"

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 974 bytes --]

On 06/02/2016 07:26 PM, Changlong Xie wrote:

>>> +
>>> +    ioc = qio_channel_file_new_path(filename, O_WRONLY | O_CREAT,
>>> 0660, errp);
>>
>> This does not look right, it looks like it's going to open the file
>> to write to it. You probably want O_RDONLY, also I don't think the
>> O_CREAT flag is needed. (and without O_WRONLY, mode can be 0 instead of
>> 0660.)
>>
> 
> Yes, as you said. We should use 0_RDONLY for open(2), so mode should be 0.

Huh?  mode doesn't affect the current fd, but DOES affect the next
person to open the file.  If you are truly creating the file, then a
mode of 0 means you won't be able to reopen it without chmod.  And if
you are doing O_RDONLY | O_CREAT, all you will be able to create is an
empty file, which is a pretty boring read.  So drop the O_CREAT, and
then you don't need a mode argument at all.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v5 1/1] Introduce "xen-load-devices-state"

[Changlong Xie]

On 06/03/2016 09:45 AM, Eric Blake wrote:
> On 06/02/2016 07:26 PM, Changlong Xie wrote:
>
>>>> +
>>>> +    ioc = qio_channel_file_new_path(filename, O_WRONLY | O_CREAT,
>>>> 0660, errp);
>>>
>>> This does not look right, it looks like it's going to open the file
>>> to write to it. You probably want O_RDONLY, also I don't think the
>>> O_CREAT flag is needed. (and without O_WRONLY, mode can be 0 instead of
>>> 0660.)
>>>
>>
>> Yes, as you said. We should use 0_RDONLY for open(2), so mode should be 0.
>
> Huh?  mode doesn't affect the current fd, but DOES affect the next
> person to open the file.  If you are truly creating the file, then a
> mode of 0 means you won't be able to reopen it without chmod.  And if
> you are doing O_RDONLY | O_CREAT, all you will be able to create is an
> empty file, which is a pretty boring read.  So drop the O_CREAT, and
> then you don't need a mode argument at all.
>

Yes, i just mean qio_channel_file_new_path(filename, O_RDONLY, 0, errp) 
here. Maybe my poor english make you confused :(

Thanks
	-Xie

Re: [Qemu-devel] [PATCH v5 1/1] Introduce "xen-load-devices-state"

[Changlong Xie]

On 06/03/2016 10:13 AM, Changlong Xie wrote:
> On 06/03/2016 09:45 AM, Eric Blake wrote:
>> On 06/02/2016 07:26 PM, Changlong Xie wrote:
>>
>>>>> +
>>>>> +    ioc = qio_channel_file_new_path(filename, O_WRONLY | O_CREAT,
>>>>> 0660, errp);
>>>>
>>>> This does not look right, it looks like it's going to open the file
>>>> to write to it. You probably want O_RDONLY, also I don't think the
>>>> O_CREAT flag is needed. (and without O_WRONLY, mode can be 0 instead of
>>>> 0660.)
>>>>
>>>
>>> Yes, as you said. We should use 0_RDONLY for open(2), so mode should
>>> be 0.
>>
>> Huh?  mode doesn't affect the current fd, but DOES affect the next
>> person to open the file.  If you are truly creating the file, then a
>> mode of 0 means you won't be able to reopen it without chmod.  And if
>> you are doing O_RDONLY | O_CREAT, all you will be able to create is an
>> empty file, which is a pretty boring read.  So drop the O_CREAT, and
>> then you don't need a mode argument at all.
>>
>
> Yes, i just mean qio_channel_file_new_path(filename, O_RDONLY, 0, errp)

I just notice that, qemu specifies flag 'O_BINARY' to allow system to 
differentiate between a text file and a binary file( I guess so?). For 
backward compatibility, refer to function test_io_channel_file(), i will 
use

qio_channel_file_new_path(filename, O_RDONLY | O_BINARY, 0, errp)

here.

> here. Maybe my poor english make you confused :(
>
> Thanks
>      -Xie
>
>
>
>

Re: [Qemu-devel] [PATCH v5 1/1] Introduce "xen-load-devices-state"

[Changlong Xie]

On 06/02/2016 11:14 PM, Anthony PERARD wrote:
> On Thu, Jun 02, 2016 at 06:36:46PM +0800, Changlong Xie wrote:
>> +void qmp_xen_load_devices_state(const char *filename, Error **errp)
>> +{
>> +    QEMUFile *f;
>> +    QIOChannelFile *ioc;
>> +    int ret;
>> +
>> +    /* Guest must be paused before loading the device state; the RAM state
>> +     * will already have been loaded by xc
>> +     */
>> +    if (runstate_is_running()) {
>> +        error_setg(errp, "Cannot update device state while vm is running");
>> +        return;
>> +    }
>> +    vm_stop(RUN_STATE_RESTORE_VM);
>> +
>> +    ioc = qio_channel_file_new_path(filename, O_WRONLY | O_CREAT, 0660, errp);
>
> This does not look right, it looks like it's going to open the file
> to write to it. You probably want O_RDONLY, also I don't think the
> O_CREAT flag is needed. (and without O_WRONLY, mode can be 0 instead of
> 0660.)
>
>> +    if (!ioc) {
>> +        return;
>> +    }
>> +    f = qemu_fopen_channel_output(QIO_CHANNEL(ioc));
>
> I'm not sure, but I guess here you want qemu_fopen_channel_input here.

After go over io channel mechanism, i think you are right here.

>
> Thanks,
>