RE: [PATCH v2 1/2] qemu: add support for shmem-{plain, doorbell} role

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(a)redhat.com&gt; I do not remember signing off this patch. Is this a re-spin of some old series?

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.

>Signed-off-by: Yang Hang <yanghang44(a)huawei.com&gt; >Signed-off-by: Wang Xin <wangxinxin.wang(a)huawei.com&gt; > >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> > ... > &lt;devices&gt; >- &lt;shmem name='my_shmem0'&gt; >+ &lt;shmem name='my_shmem0' role='peer'&gt; > &lt;model type='ivshmem-plain'/&gt; > &lt;size unit='M'&gt;4&lt;/size&gt; > &lt;/shmem&gt; >@@ -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.

> </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.

The rest looks fine to me, so you can keep the S-o-b with the appropriate changes.