[PATCH v2 1/2] qemu: add support for shmem-{plain, doorbell} role
Wangxin (Alexander)
wangxinxin.wang at huawei.com
Tue Jul 21 12:03:42 UTC 2020
> 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 at redhat.com>
>
>
> 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 at huawei.com>
> >Signed-off-by: Wang Xin <wangxinxin.wang at huawei.com>
> >
> >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.
More information about the libvir-list
mailing list