[PATCH v3 0/2] qemu: support shmem device migration
Shmem device support property role with 'master'(master=on) or 'peer'(master=off, default mode), which controls to copy the shared memory on migration to the destination host or not. see https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/system/ivshmem.rst;hb=H... The 1st patch add attribute 'role', it based on Martin's old patch https://www.redhat.com/archives/libvir-list/2016-September/msg00536.html The 2nd patch remove shmem name check in migraiton, which allows user change shared memory path on destination host. v3 -> v2: 1. Correct description of shmem ���role��� attribute. If there is any mistake in the description, please show me. Sorry for my poor English :) 2. Remove the inappropriate suggestion in migration log. 3. allow shmem name change in migration, instead add a new 'mem-path' attribute. v2 -> v1: Fix ivshmem testcases failure. Wang Xin (2): qemu: add support for shmem-{plain, doorbell} role conf: allow shmem name change in migration docs/formatdomain.html.in | 13 ++++++- docs/schemas/domaincommon.rng | 8 ++++ src/conf/domain_conf.c | 37 +++++++++++++++++-- src/conf/domain_conf.h | 10 +++++ src/libvirt_private.syms | 2 + src/qemu/qemu_command.c | 17 ++++++++- src/qemu/qemu_migration.c | 20 ++++++++-- .../qemuhotplug-ivshmem-plain-detach.xml | 2 +- .../qemuhotplug-ivshmem-plain.xml | 2 +- ...qemuhotplug-base-live+ivshmem-doorbell.xml | 2 +- .../qemuhotplug-base-live+ivshmem-plain.xml | 2 +- .../shmem-plain-doorbell.args | 6 ++- .../qemuxml2argvdata/shmem-plain-doorbell.xml | 4 +- .../shmem-plain-doorbell.xml | 4 +- tests/qemuxml2xmltest.c | 3 +- 15 files changed, 110 insertions(+), 22 deletions(-) -- 2.23.0
Role(master or peer) controls how the domain behaves on migration. For more details about migration with ivshmem, see https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/system/ivshmem.rst;hb=H... It's a optional attribute in libvirt, and qemu will choose default role for ivshmem device if the user is not specified. With device property 'role', the value can be 'master' or 'peer'. - 'master' (means 'master=on' in qemu), the guest will copy the shared memory on migration to the destination host. - 'peer' (means 'master=off' in qemu), the migration is disabled. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> Signed-off-by: Yang Hang <yanghang44@huawei.com> Signed-off-by: Wang Xin <wangxinxin.wang@huawei.com> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index f3a639b972..1a7308cb00 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -9141,7 +9141,7 @@ qemu-kvm -net nic,model=? /dev/null <pre> ... <devices> - <shmem name='my_shmem0'> + <shmem name='my_shmem0' role='peer'> <model type='ivshmem-plain'/> <size unit='M'>4</size> </shmem> @@ -9162,6 +9162,17 @@ qemu-kvm -net nic,model=? /dev/null <code>name</code> to identify the shared memory. This attribute cannot be directory specific to <code>.</code> or <code>..</code> as well as it cannot involve path separator <code>/</code>. + The optional <code>role</code> (<span class="since">since 6.6.0</span>) + attribute specifies the shared memory is migratable or not. The value can + be either "master" or "peer", the former will mean that upon migration, + the data in the shared memory is migrated with the domain. There should + be only one "master" per shared memory object. Migration with "peer" role + is disabled. If migration of such domain is required, the shmem device + needs to be unplugged before migration and plugged in at the destination + upon successful migration. If the role not specified, the hypervisor + default is used. This attribute is currently available only for + <code>model</code> type <code>ivshmem-plain</code> and + <code>ivshmem-doorbell</code>. </dd> <dt><code>model</code></dt> <dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index a810f569c6..09d4ad3e96 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4417,6 +4417,14 @@ <param name="pattern">[^/]*</param> </data> </attribute> + <optional> + <attribute name="role"> + <choice> + <value>master</value> + <value>peer</value> + </choice> + </attribute> + </optional> <interleave> <optional> <element name="model"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8d328819af..bede53d5aa 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1325,6 +1325,13 @@ VIR_ENUM_IMPL(virDomainShmemModel, "ivshmem-doorbell", ); +VIR_ENUM_IMPL(virDomainShmemRole, + VIR_DOMAIN_SHMEM_ROLE_LAST, + "default", + "master", + "peer", +); + VIR_ENUM_IMPL(virDomainLaunchSecurity, VIR_DOMAIN_LAUNCH_SECURITY_LAST, "", @@ -15354,6 +15361,19 @@ virDomainShmemDefParseXML(virDomainXMLOptionPtr xmlopt, goto cleanup; } + if (def->model != VIR_DOMAIN_SHMEM_MODEL_IVSHMEM) { + tmp = virXMLPropString(node, "role"); + if (tmp) { + if ((def->role = virDomainShmemRoleTypeFromString(tmp)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Unknown shmem role type '%s'"), tmp); + goto cleanup; + } + + VIR_FREE(tmp); + } + } + if (virParseScaledValue("./size[1]", NULL, ctxt, &def->size, 1, ULLONG_MAX, false) < 0) goto cleanup; @@ -18573,6 +18593,9 @@ virDomainShmemDefEquals(virDomainShmemDefPtr src, if (src->model != dst->model) return false; + if (src->role != dst->role) + return false; + if (src->server.enabled != dst->server.enabled) return false; @@ -23752,6 +23775,15 @@ virDomainShmemDefCheckABIStability(virDomainShmemDefPtr src, return false; } + if (src->role != dst->role) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target shared memory role '%s' does not match " + "source role '%s'"), + virDomainShmemRoleTypeToString(dst->role), + virDomainShmemRoleTypeToString(src->role)); + return false; + } + if (src->model != dst->model) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target shared memory model '%s' does not match " @@ -27402,8 +27434,12 @@ virDomainShmemDefFormat(virBufferPtr buf, virDomainShmemDefPtr def, unsigned int flags) { - virBufferEscapeString(buf, "<shmem name='%s'>\n", def->name); + virBufferEscapeString(buf, "<shmem name='%s'", def->name); + if (def->role) + virBufferEscapeString(buf, " role='%s'", + virDomainShmemRoleTypeToString(def->role)); + virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); virBufferAsprintf(buf, "<model type='%s'/>\n", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 241149af24..855c144ddb 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1771,10 +1771,19 @@ typedef enum { VIR_DOMAIN_SHMEM_MODEL_LAST } virDomainShmemModel; +typedef enum { + VIR_DOMAIN_SHMEM_ROLE_DEFAULT, + VIR_DOMAIN_SHMEM_ROLE_MASTER, + VIR_DOMAIN_SHMEM_ROLE_PEER, + + VIR_DOMAIN_SHMEM_ROLE_LAST +} virDomainShmemRole; + struct _virDomainShmemDef { char *name; unsigned long long size; int model; /* enum virDomainShmemModel */ + int role; /* enum virDomainShmemRole */ struct { bool enabled; virDomainChrSourceDef chr; @@ -3624,6 +3633,7 @@ VIR_ENUM_DECL(virDomainMemoryAllocation); VIR_ENUM_DECL(virDomainIOMMUModel); VIR_ENUM_DECL(virDomainVsockModel); VIR_ENUM_DECL(virDomainShmemModel); +VIR_ENUM_DECL(virDomainShmemRole); VIR_ENUM_DECL(virDomainLaunchSecurity); /* from libvirt.h */ VIR_ENUM_DECL(virDomainState); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 73b72c9e10..3f28943575 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -597,6 +597,8 @@ virDomainShmemDefInsert; virDomainShmemDefRemove; virDomainShmemModelTypeFromString; virDomainShmemModelTypeToString; +virDomainShmemRoleTypeFromString; +virDomainShmemRoleTypeToString; virDomainShutdownReasonTypeFromString; virDomainShutdownReasonTypeToString; virDomainShutoffReasonTypeFromString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 839c93bfb4..0655d8359d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8539,11 +8539,24 @@ qemuBuildShmemDevStr(virDomainDefPtr def, virBufferAdd(&buf, virDomainShmemModelTypeToString(shmem->model), -1); virBufferAsprintf(&buf, ",id=%s", shmem->info.alias); - if (shmem->server.enabled) + if (shmem->server.enabled) { virBufferAsprintf(&buf, ",chardev=char%s", shmem->info.alias); - else + } else { virBufferAsprintf(&buf, ",memdev=shmmem-%s", shmem->info.alias); + switch ((virDomainShmemRole) shmem->role) { + case VIR_DOMAIN_SHMEM_ROLE_MASTER: + virBufferAddLit(&buf, ",master=on"); + break; + case VIR_DOMAIN_SHMEM_ROLE_PEER: + virBufferAddLit(&buf, ",master=off"); + break; + case VIR_DOMAIN_SHMEM_ROLE_DEFAULT: + case VIR_DOMAIN_SHMEM_ROLE_LAST: + break; + } + } + if (shmem->msi.vectors) virBufferAsprintf(&buf, ",vectors=%u", shmem->msi.vectors); if (shmem->msi.ioeventfd) { diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 2c7bf349c3..e588b74c2f 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1261,10 +1261,22 @@ qemuMigrationSrcIsAllowed(virQEMUDriverPtr driver, } } - if (vm->def->nshmems) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("migration with shmem device is not supported")); - return false; + for (i = 0; i < vm->def->nshmems; i++) { + virDomainShmemDefPtr shmem = vm->def->shmems[i]; + + if (shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("migration with legacy shmem device is not supported")); + return false; + } + if (shmem->role == VIR_DOMAIN_SHMEM_ROLE_PEER) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("shmem device '%s' with role='%s', " + "cannot be migrated"), + shmem->name, + virDomainShmemRoleTypeToString(shmem->role)); + return false; + } } for (i = 0; i < vm->def->nnets; i++) { diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain-detach.xml b/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain-detach.xml index 68f592fb21..338017aa28 100644 --- a/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain-detach.xml +++ b/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain-detach.xml @@ -1,4 +1,4 @@ -<shmem name='shmem0'> +<shmem name='shmem0' role='peer'> <model type='ivshmem-plain'/> <size unit='M'>4</size> <alias name='shmem0'/> diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain.xml b/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain.xml index 6bd96ff167..780e49de23 100644 --- a/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain.xml +++ b/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain.xml @@ -1,3 +1,3 @@ -<shmem name='shmem0'> +<shmem name='shmem0' role='peer'> <model type='ivshmem-plain'/> </shmem> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell.xml index a6930bfa69..8013264989 100644 --- a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell.xml +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell.xml @@ -45,7 +45,7 @@ <alias name='input1'/> </input> <memballoon model='none'/> - <shmem name='shmem0'> + <shmem name='shmem0' role='peer'> <model type='ivshmem-plain'/> <size unit='M'>4</size> <alias name='shmem0'/> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-plain.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-plain.xml index 757b6b0980..0490310760 100644 --- a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-plain.xml +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-plain.xml @@ -45,7 +45,7 @@ <alias name='input1'/> </input> <memballoon model='none'/> - <shmem name='shmem0'> + <shmem name='shmem0' role='peer'> <model type='ivshmem-plain'/> <size unit='M'>4</size> <alias name='shmem0'/> diff --git a/tests/qemuxml2argvdata/shmem-plain-doorbell.args b/tests/qemuxml2argvdata/shmem-plain-doorbell.args index 6ed86b7448..93d089c27a 100644 --- a/tests/qemuxml2argvdata/shmem-plain-doorbell.args +++ b/tests/qemuxml2argvdata/shmem-plain-doorbell.args @@ -30,10 +30,12 @@ size=4194304,share=yes \ -device ivshmem-plain,id=shmem0,memdev=shmmem-shmem0,bus=pci.0,addr=0x3 \ -object memory-backend-file,id=shmmem-shmem1,mem-path=/dev/shm/shmem1,\ size=134217728,share=yes \ --device ivshmem-plain,id=shmem1,memdev=shmmem-shmem1,bus=pci.0,addr=0x5 \ +-device ivshmem-plain,id=shmem1,memdev=shmmem-shmem1,master=off,bus=pci.0,\ +addr=0x5 \ -object memory-backend-file,id=shmmem-shmem2,mem-path=/dev/shm/shmem2,\ size=268435456,share=yes \ --device ivshmem-plain,id=shmem2,memdev=shmmem-shmem2,bus=pci.0,addr=0x4 \ +-device ivshmem-plain,id=shmem2,memdev=shmmem-shmem2,master=on,bus=pci.0,\ +addr=0x4 \ -device ivshmem-doorbell,id=shmem3,chardev=charshmem3,ioeventfd=on,bus=pci.0,\ addr=0x6 \ -chardev socket,id=charshmem3,path=/var/lib/libvirt/shmem-shmem3-sock \ diff --git a/tests/qemuxml2argvdata/shmem-plain-doorbell.xml b/tests/qemuxml2argvdata/shmem-plain-doorbell.xml index e248d637a5..7c76e0fbba 100644 --- a/tests/qemuxml2argvdata/shmem-plain-doorbell.xml +++ b/tests/qemuxml2argvdata/shmem-plain-doorbell.xml @@ -22,11 +22,11 @@ <shmem name='shmem0'> <model type='ivshmem-plain'/> </shmem> - <shmem name='shmem1'> + <shmem name='shmem1' role='peer'> <model type='ivshmem-plain'/> <size unit='M'>128</size> </shmem> - <shmem name='shmem2'> + <shmem name='shmem2' role='master'> <model type='ivshmem-plain'/> <size unit='M'>256</size> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> diff --git a/tests/qemuxml2xmloutdata/shmem-plain-doorbell.xml b/tests/qemuxml2xmloutdata/shmem-plain-doorbell.xml index c5dd0f4598..64c0a7d753 100644 --- a/tests/qemuxml2xmloutdata/shmem-plain-doorbell.xml +++ b/tests/qemuxml2xmloutdata/shmem-plain-doorbell.xml @@ -26,12 +26,12 @@ <size unit='M'>4</size> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> </shmem> - <shmem name='shmem1'> + <shmem name='shmem1' role='peer'> <model type='ivshmem-plain'/> <size unit='M'>128</size> <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> </shmem> - <shmem name='shmem2'> + <shmem name='shmem2' role='master'> <model type='ivshmem-plain'/> <size unit='M'>256</size> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index c17e3303b0..6cb2718767 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1135,7 +1135,8 @@ mymain(void) DO_TEST("tap-vhost", NONE); DO_TEST("tap-vhost-incorrect", NONE); DO_TEST("shmem", NONE); - DO_TEST("shmem-plain-doorbell", NONE); + DO_TEST("shmem-plain-doorbell", + QEMU_CAPS_DEVICE_IVSHMEM_PLAIN, QEMU_CAPS_DEVICE_IVSHMEM_DOORBELL); DO_TEST("smbios", NONE); DO_TEST("smbios-multiple-type2", NONE); DO_TEST("smbios-type-fwcfg", NONE); -- 2.23.0
On Fri, Jul 24, 2020 at 11:34:11AM +0800, Wang Xin wrote:
Role(master or peer) controls how the domain behaves on migration. For more details about migration with ivshmem, see https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/system/ivshmem.rst;hb=H...
It's a optional attribute in libvirt, and qemu will choose default role for ivshmem device if the user is not specified.
With device property 'role', the value can be 'master' or 'peer'. - 'master' (means 'master=on' in qemu), the guest will copy the shared memory on migration to the destination host. - 'peer' (means 'master=off' in qemu), the migration is disabled.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> Signed-off-by: Yang Hang <yanghang44@huawei.com> Signed-off-by: Wang Xin <wangxinxin.wang@huawei.com>
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 2c7bf349c3..e588b74c2f 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1261,10 +1261,22 @@ qemuMigrationSrcIsAllowed(virQEMUDriverPtr driver, } }
- if (vm->def->nshmems) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("migration with shmem device is not supported")); - return false; + for (i = 0; i < vm->def->nshmems; i++) { + virDomainShmemDefPtr shmem = vm->def->shmems[i]; + + if (shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("migration with legacy shmem device is not supported")); + return false; + } + if (shmem->role == VIR_DOMAIN_SHMEM_ROLE_PEER) {
This allows migration with the default role which you leave to be chosen by the hypervisor. In that case we need to change this to: if (shmem->role != VIR_DOMAIN_SHMEM_ROLE_MASTER) {
+ virReportError(VIR_ERR_OPERATION_INVALID, + _("shmem device '%s' with role='%s', " + "cannot be migrated"),
and this accordingly. But that's fine, I can change that before pushing. With that change: Reviewed-by: Martin Kletzander <mkletzan@redhat.com> but I'll wait after the release since this is a feature.
On Fri, Jul 24, 2020 at 11:34:11AM +0800, Wang Xin wrote:
Role(master or peer) controls how the domain behaves on migration. For more details about migration with ivshmem, see https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/system/ivshmem.rst;hb=H...
It's a optional attribute in libvirt, and qemu will choose default role for ivshmem device if the user is not specified.
With device property 'role', the value can be 'master' or 'peer'. - 'master' (means 'master=on' in qemu), the guest will copy the shared memory on migration to the destination host. - 'peer' (means 'master=off' in qemu), the migration is disabled.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> Signed-off-by: Yang Hang <yanghang44@huawei.com> Signed-off-by: Wang Xin <wangxinxin.wang@huawei.com>
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 2c7bf349c3..e588b74c2f 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1261,10 +1261,22 @@ qemuMigrationSrcIsAllowed(virQEMUDriverPtr driver, } }
- if (vm->def->nshmems) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("migration with shmem device is not supported")); - return false; + for (i = 0; i < vm->def->nshmems; i++) { + virDomainShmemDefPtr shmem = vm->def->shmems[i]; + + if (shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("migration with legacy shmem device is not supported")); + return false; + } + if (shmem->role == VIR_DOMAIN_SHMEM_ROLE_PEER) {
This allows migration with the default role which you leave to be chosen by the hypervisor. In that case we need to change this to:
if (shmem->role != VIR_DOMAIN_SHMEM_ROLE_MASTER) {
In fact, the hypervisor will also check weather the ivshmem device is migratable(by using migration blocker) or not. If 'role' is not specified in libvirt, the default role may 'master' or 'peer', we don't kown it, so, I think it's better to check it in hypervisor.
+ virReportError(VIR_ERR_OPERATION_INVALID, + _("shmem device '%s' with role='%s', " + "cannot be migrated"),
and this accordingly.
But that's fine, I can change that before pushing. With that change:
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
but I'll wait after the release since this is a feature.
On Mon, Aug 03, 2020 at 09:26:19AM +0000, Wangxin (Alexander) wrote:
On Fri, Jul 24, 2020 at 11:34:11AM +0800, Wang Xin wrote:
Role(master or peer) controls how the domain behaves on migration. For more details about migration with ivshmem, see https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/system/ivshmem.rst;hb=H...
It's a optional attribute in libvirt, and qemu will choose default role for ivshmem device if the user is not specified.
With device property 'role', the value can be 'master' or 'peer'. - 'master' (means 'master=on' in qemu), the guest will copy the shared memory on migration to the destination host. - 'peer' (means 'master=off' in qemu), the migration is disabled.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> Signed-off-by: Yang Hang <yanghang44@huawei.com> Signed-off-by: Wang Xin <wangxinxin.wang@huawei.com>
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 2c7bf349c3..e588b74c2f 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1261,10 +1261,22 @@ qemuMigrationSrcIsAllowed(virQEMUDriverPtr driver, } }
- if (vm->def->nshmems) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("migration with shmem device is not supported")); - return false; + for (i = 0; i < vm->def->nshmems; i++) { + virDomainShmemDefPtr shmem = vm->def->shmems[i]; + + if (shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("migration with legacy shmem device is not supported")); + return false; + } + if (shmem->role == VIR_DOMAIN_SHMEM_ROLE_PEER) {
This allows migration with the default role which you leave to be chosen by the hypervisor. In that case we need to change this to:
if (shmem->role != VIR_DOMAIN_SHMEM_ROLE_MASTER) {
In fact, the hypervisor will also check weather the ivshmem device is migratable(by using migration blocker) or not. If 'role' is not specified in libvirt, the default role may 'master' or 'peer', we don't kown it, so, I think it's better to check it in hypervisor.
We strive to prevent any errors in QEMU "just in case". Users could start doubting our backwards compatibility if their `default` role shmem stopped migrating in the future, for example. And by being strict we avoid any possible future issues like this.
+ virReportError(VIR_ERR_OPERATION_INVALID, + _("shmem device '%s' with role='%s', " + "cannot be migrated"),
and this accordingly.
But that's fine, I can change that before pushing. With that change:
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
but I'll wait after the release since this is a feature.
On Mon, Aug 03, 2020 at 09:26:19AM +0000, Wangxin (Alexander) wrote:
On Fri, Jul 24, 2020 at 11:34:11AM +0800, Wang Xin wrote:
Role(master or peer) controls how the domain behaves on migration. For more details about migration with ivshmem, see https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/system/ivshmem.rst;hb=H...
It's a optional attribute in libvirt, and qemu will choose default role for ivshmem device if the user is not specified.
With device property 'role', the value can be 'master' or 'peer'. - 'master' (means 'master=on' in qemu), the guest will copy the shared memory on migration to the destination host. - 'peer' (means 'master=off' in qemu), the migration is disabled.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> Signed-off-by: Yang Hang <yanghang44@huawei.com> Signed-off-by: Wang Xin <wangxinxin.wang@huawei.com>
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 2c7bf349c3..e588b74c2f 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1261,10 +1261,22 @@ qemuMigrationSrcIsAllowed(virQEMUDriverPtr driver, } }
- if (vm->def->nshmems) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("migration with shmem device is not supported")); - return false; + for (i = 0; i < vm->def->nshmems; i++) { + virDomainShmemDefPtr shmem = vm->def->shmems[i]; + + if (shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("migration with legacy shmem device is not supported")); + return false; + } + if (shmem->role == VIR_DOMAIN_SHMEM_ROLE_PEER) {
This allows migration with the default role which you leave to be chosen by the hypervisor. In that case we need to change this to:
if (shmem->role != VIR_DOMAIN_SHMEM_ROLE_MASTER) {
In fact, the hypervisor will also check weather the ivshmem device is migratable(by using migration blocker) or not. If 'role' is not specified in libvirt, the default role may 'master' or 'peer', we don't kown it, so, I think it's better to check it in hypervisor.
We strive to prevent any errors in QEMU "just in case". Users could start doubting our backwards compatibility if their `default` role shmem stopped migrating in the future, for example. And by being strict we avoid any possible future issues like this.
Get it, thanks for explaining. :)
+ virReportError(VIR_ERR_OPERATION_INVALID, + _("shmem device '%s' with role='%s', " + "cannot be migrated"),
and this accordingly.
But that's fine, I can change that before pushing. With that change:
Sure.
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
but I'll wait after the release since this is a feature.
Thanks for your review! Best regards, Alexander.
The shmem 'name' specifies the shared memory path in '/dev/shm/', however, we may need to change it to avoid filename conflict when VM migrate to other host. This patch remove shmem name consistency check. Signed-off-by: Wang Xin <wangxinxin.wang@huawei.com> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bede53d5aa..59cc61ea49 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23768,13 +23768,6 @@ static bool virDomainShmemDefCheckABIStability(virDomainShmemDefPtr src, virDomainShmemDefPtr dst) { - if (STRNEQ_NULLABLE(src->name, dst->name)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target shared memory name '%s' does not match source " - "'%s'"), dst->name, src->name); - return false; - } - if (src->role != dst->role) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target shared memory role '%s' does not match " -- 2.23.0
On Fri, Jul 24, 2020 at 11:34:12AM +0800, Wang Xin wrote:
The shmem 'name' specifies the shared memory path in '/dev/shm/', however, we may need to change it to avoid filename conflict when VM migrate to other host. This patch remove shmem name consistency check.
There is one extra thing you need to consider when allowing this. And that is "is that value part of the migration stream"? Even though this applies only to QEMU (in which case this could be handled there only, but the whole feature is QEMU-only, so...) we need to check that before allowing it. The easiest check is to actually just migrate such machine. I tried it and works so it's fine. Reviewed-by: Martin Kletzander <mkletzan@redhat.com> Same as with the first one I'll push it after the release. Thanks for the patches.
Signed-off-by: Wang Xin <wangxinxin.wang@huawei.com>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bede53d5aa..59cc61ea49 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23768,13 +23768,6 @@ static bool virDomainShmemDefCheckABIStability(virDomainShmemDefPtr src, virDomainShmemDefPtr dst) { - if (STRNEQ_NULLABLE(src->name, dst->name)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target shared memory name '%s' does not match source " - "'%s'"), dst->name, src->name); - return false; - } - if (src->role != dst->role) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target shared memory role '%s' does not match " -- 2.23.0
Ping.
Shmem device support property role with 'master'(master=on) or 'peer'(master=off, default mode), which controls to copy the shared memory on migration to the destination host or not. see https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/system/ivshmem.rst;hb=H...
The 1st patch add attribute 'role', it based on Martin's old patch https://www.redhat.com/archives/libvir-list/2016-September/msg00536.html
The 2nd patch remove shmem name check in migraiton, which allows user change shared memory path on destination host.
v3 -> v2: 1. Correct description of shmem ��ole�?attribute. If there is any mistake in the description, please show me. Sorry for my poor English :) 2. Remove the inappropriate suggestion in migration log. 3. allow shmem name change in migration, instead add a new 'mem-path' attribute.
v2 -> v1: Fix ivshmem testcases failure.
Wang Xin (2): qemu: add support for shmem-{plain, doorbell} role conf: allow shmem name change in migration
docs/formatdomain.html.in | 13 ++++++- docs/schemas/domaincommon.rng | 8 ++++ src/conf/domain_conf.c | 37 +++++++++++++++++-- src/conf/domain_conf.h | 10 +++++ src/libvirt_private.syms | 2 + src/qemu/qemu_command.c | 17 ++++++++- src/qemu/qemu_migration.c | 20 ++++++++-- .../qemuhotplug-ivshmem-plain-detach.xml | 2 +- .../qemuhotplug-ivshmem-plain.xml | 2 +- ...qemuhotplug-base-live+ivshmem-doorbell.xml | 2 +- .../qemuhotplug-base-live+ivshmem-plain.xml | 2 +- .../shmem-plain-doorbell.args | 6 ++- .../qemuxml2argvdata/shmem-plain-doorbell.xml | 4 +- .../shmem-plain-doorbell.xml | 4 +- tests/qemuxml2xmltest.c | 3 +- 15 files changed, 110 insertions(+), 22 deletions(-)
-- 2.23.0
On Fri, Jul 24, 2020 at 11:34:10AM +0800, Wang Xin wrote:
Shmem device support property role with 'master'(master=on) or 'peer'(master=off, default mode), which controls to copy the shared memory on migration to the destination host or not. see https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/system/ivshmem.rst;hb=H...
The 1st patch add attribute 'role', it based on Martin's old patch https://www.redhat.com/archives/libvir-list/2016-September/msg00536.html
The 2nd patch remove shmem name check in migraiton, which allows user change shared memory path on destination host.
v3 -> v2: 1. Correct description of shmem ‘role’ attribute. If there is any mistake in the description, please show me. Sorry for my poor English :) 2. Remove the inappropriate suggestion in migration log. 3. allow shmem name change in migration, instead add a new 'mem-path' attribute.
v2 -> v1: Fix ivshmem testcases failure.
Wang Xin (2): qemu: add support for shmem-{plain, doorbell} role conf: allow shmem name change in migration
So as you read in the reviews I'm going to push this after the release. But could you, please, send an update to the news after the release (or now, but mark it as for 6.7.0) so that it is also properly visible for the users? Thanks a lot.
docs/formatdomain.html.in | 13 ++++++- docs/schemas/domaincommon.rng | 8 ++++ src/conf/domain_conf.c | 37 +++++++++++++++++-- src/conf/domain_conf.h | 10 +++++ src/libvirt_private.syms | 2 + src/qemu/qemu_command.c | 17 ++++++++- src/qemu/qemu_migration.c | 20 ++++++++-- .../qemuhotplug-ivshmem-plain-detach.xml | 2 +- .../qemuhotplug-ivshmem-plain.xml | 2 +- ...qemuhotplug-base-live+ivshmem-doorbell.xml | 2 +- .../qemuhotplug-base-live+ivshmem-plain.xml | 2 +- .../shmem-plain-doorbell.args | 6 ++- .../qemuxml2argvdata/shmem-plain-doorbell.xml | 4 +- .../shmem-plain-doorbell.xml | 4 +- tests/qemuxml2xmltest.c | 3 +- 15 files changed, 110 insertions(+), 22 deletions(-)
-- 2.23.0
On Fri, Jul 24, 2020 at 11:34:10AM +0800, Wang Xin wrote:
Shmem device support property role with 'master'(master=on) or 'peer'(master=off, default mode), which controls to copy the shared memory on migration to the destination host or not. see https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/system/ivshmem.rst ;hb=HEAD
The 1st patch add attribute 'role', it based on Martin's old patch https://www.redhat.com/archives/libvir-list/2016-September/msg00536.htm l
The 2nd patch remove shmem name check in migraiton, which allows user change shared memory path on destination host.
v3 -> v2: 1. Correct description of shmem ‘role’ attribute. If there is any mistake in the description, please show me. Sorry for my poor English :) 2. Remove the inappropriate suggestion in migration log. 3. allow shmem name change in migration, instead add a new 'mem-path' attribute.
v2 -> v1: Fix ivshmem testcases failure.
Wang Xin (2): qemu: add support for shmem-{plain, doorbell} role conf: allow shmem name change in migration
So as you read in the reviews I'm going to push this after the release. But could you, please, send an update to the news after the release (or now, but mark it as for 6.7.0) so that it is also properly visible for the users?
Thanks a lot.
I'll send it after the v6.7.0 release. My pleasure.
docs/formatdomain.html.in | 13 ++++++- docs/schemas/domaincommon.rng | 8 ++++ src/conf/domain_conf.c | 37 +++++++++++++++++-- src/conf/domain_conf.h | 10 +++++ src/libvirt_private.syms | 2 + src/qemu/qemu_command.c | 17 ++++++++- src/qemu/qemu_migration.c | 20 ++++++++-- .../qemuhotplug-ivshmem-plain-detach.xml | 2 +- .../qemuhotplug-ivshmem-plain.xml | 2 +- ...qemuhotplug-base-live+ivshmem-doorbell.xml | 2 +- .../qemuhotplug-base-live+ivshmem-plain.xml | 2 +- .../shmem-plain-doorbell.args | 6 ++- .../qemuxml2argvdata/shmem-plain-doorbell.xml | 4 +- .../shmem-plain-doorbell.xml | 4 +- tests/qemuxml2xmltest.c | 3 +- 15 files changed, 110 insertions(+), 22 deletions(-)
-- 2.23.0
participants (3)
-
Martin Kletzander -
Wang Xin -
Wangxin (Alexander)