> On Fri, Jul 17, 2020 at 09:04:50PM +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=HEAD > > > >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@xxxxxxxxxx> > > > I do not remember signing off this patch. Is this a re-spin of some old series? It's about 4 years ago, https://www.redhat.com/archives/libvir-list/2016-September/msg00536.html And as you mentioned in msg(see link), you may had a plan to do it, but I didn't find the patch. https://www.redhat.com/archives/libvir-list/2016-August/msg00591.html So, I do some tests and re-send your first patch. > > I see with this v2 you fixed the `make check`. We tend to use the cover letters > for that, it's nicely recognisable what are the changes there. However make > syntax-check still fails after this patch, even though that's just a minor > whitespace change that can be done automatically. Not only make syntax-check > outputs the diff for you (which you can apply), but it also suggests the script > we have there that does all of that for you. Sorry for missing the check, I will fix it in V3, and list the changes in cover letter. > > >Signed-off-by: Yang Hang <yanghang44@xxxxxxxxxx> > >Signed-off-by: Wang Xin <wangxinxin.wang@xxxxxxxxxx> > > > >diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > >index f5ee97de81..d4d90ad44d 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> > >@@ -9159,9 +9159,15 @@ qemu-kvm -net nic,model=? /dev/null > > <dt><code>shmem</code></dt> > > <dd> > > The <code>shmem</code> element has one mandatory attribute, > >- <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>. > >+ <code>name</code> to identify the shared memory. Optional attribute > >+ <code>role</code> can be set to values "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. This attribute cannot be directory > >+ specific to <code>.</code> or <code>..</code> as well as it cannot involve path > >+ separator <code>/</code>. > > You wrote your text in the middle of the explanation of another attribute. Now > it looks like the optional attribute "role" can be set to values "master" or > "peer", cannot be a directory specific to "." or ".." and it cannot use a path > separator "/". It's a bit confusing, even though it is still true =) > > You should also describe what leaving out that attribute does. We usually leave > it to the hypervisor to decide although this might be enough of a difference > that we might fill in the default ourselves. But I'll leave the decision up to > you. Good suggestion, accept. > > > </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 7ecd2818b9..6e67c7ebe0 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, > > "", > >@@ -15357,6 +15364,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; > >@@ -18579,6 +18599,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; > > > >@@ -23758,6 +23781,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 " > >@@ -27408,8 +27440,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 78e64344f6..5ae85c7ae7 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, > >+ _("domain's shmem device '%s' has role='%s', " > >+ "try unplugging it first"), > > I, for one, like similar useful suggestions here, but in this case it is true > that it is nothing special compared to other devices that cannot be migrated. > So this one probably should not be suggesting it, having it in the documentation > is enough. And it's still missing the reason for failure, the fact that the > migration is not supported with such device/role combination. I will make the error msg more clear. > > The rest looks fine to me, so you can keep the S-o-b with the appropriate > changes. Thanks.