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
>
>
> 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
> >Signed-off-by: Wang Xin
> >
> >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
> >
> > ...
> > 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
> > shmem
> >
> > The shmem element has one mandatory attribute,
> >- name to identify the shared memory. This attribute cannot
> >- be directory specific to . or .. as well as
> >- it cannot involve path separator /.
> >+ name to identify the shared memory. Optional attribute
> >+ role 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 . or .. as well as it cannot
> >involve path
> >+ separator /.
>
> 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.
>
> >
> > model
> >
> >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 @@
> > [^/]*
> >
> >
> >+
> >+
> >+
> >+master
> >+peer
> >+
> >+
> >+
> >
> >
> >
> >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,
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
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
Signed-off-by: Wang Xin
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
...
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
shmem
The shmem element has one mandatory attribute,
- name to identify the shared memory. This attribute cannot
- be directory specific to . or .. as well as
- it cannot involve path separator /.
+ name to identify the shared memory. Optional attribute
+ role 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 . or .. as well as it cannot
involve path
+ separator /.
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.
model
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 @@
[^/]*
+
+
+
+master
+peer
+
+
+
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,
>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,
