[libvirt] [PATCH 00/20] 20 crazy IVSHMEM patches you won't believe compile
Let's see if the subject works if one is in need of reviews =) Yet another qemu device, right? We even have an existing device for that, right? That should be pretty straight-forward and easy, right? Well, let's see... I, at least, tried splitting the patches for you to be as easy to review as possible. Just in case you're trying out the hot-(un)plug on an upstream QEMU, make sure you do it on i440fx machine, not on q35 one, otherwise it will not work nicely (or rather at all). Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1347049 Martin Kletzander (20): qemuhotplugtest: Only read result_filename if used schema: Allow shmem to have alias conf: Allow copying of shmem defs conf: Add support for shmem model conf: Add support for shmem role qemu: Add support for shmem role qemu: Add newer shmem models qemu: Add capabilities for ivshmem-{plain,doorbell} qemu: Save various defaults for shmem qemu: Disable migration for shmem with peer role qemu: Make qemuBuildShmemDevStr static qemu: Rename qemuBuildShmemDevStr to qemuBuildShmemDevLegacyStr qemu: Move common checks outside qemuBuildShmemDevLegacyStr qemu: Reorder shmem params nicely qemu: Abstract shmem socket path preparation qemu: Rename qemuBuildShmemBackendStr to qemuBuildShmemBackendChrStr qemu: Support newer ivshmem device variants qemu: Add qemuAssignDeviceShmemAlias and use it conf: Add some shmem helpers for future use qemu: Add support for hot/cold-(un)plug of shmem devices docs/formatdomain.html.in | 10 +- docs/schemas/domaincommon.rng | 22 ++ src/conf/domain_conf.c | 154 ++++++++++++- src/conf/domain_conf.h | 30 +++ src/libvirt_private.syms | 9 + src/qemu/qemu_alias.c | 32 ++- src/qemu/qemu_alias.h | 4 + src/qemu/qemu_capabilities.c | 4 + src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_command.c | 245 ++++++++++++++++---- src/qemu/qemu_command.h | 14 +- src/qemu/qemu_domain.c | 46 ++++ src/qemu/qemu_domain.h | 3 + src/qemu/qemu_driver.c | 39 +++- src/qemu/qemu_hotplug.c | 247 ++++++++++++++++++++- src/qemu/qemu_hotplug.h | 6 + src/qemu/qemu_migration.c | 13 ++ .../caps_2.6.0-gicv2.aarch64.xml | 2 + .../caps_2.6.0-gicv3.aarch64.xml | 2 + tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml | 2 + tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 2 + tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 2 + tests/qemuhotplugtest.c | 23 +- .../qemuhotplug-ivshmem-doorbell-detach.xml | 7 + .../qemuhotplug-ivshmem-doorbell.xml | 4 + .../qemuhotplug-ivshmem-plain-detach.xml | 6 + .../qemuhotplug-ivshmem-plain.xml | 3 + ...muhotplug-base-live+ivshmem-doorbell-detach.xml | 1 + .../qemuhotplug-base-live+ivshmem-doorbell.xml | 65 ++++++ .../qemuhotplug-base-live+ivshmem-plain-detach.xml | 1 + .../qemuhotplug-base-live+ivshmem-plain.xml | 58 +++++ .../qemuxml2argv-shmem-plain-doorbell.args | 46 ++++ ...m.xml => qemuxml2argv-shmem-plain-doorbell.xml} | 15 +- tests/qemuxml2argvdata/qemuxml2argv-shmem.args | 16 +- tests/qemuxml2argvdata/qemuxml2argv-shmem.xml | 6 +- tests/qemuxml2argvtest.c | 3 + ...xml => qemuxml2xmlout-shmem-plain-doorbell.xml} | 24 +- tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml | 15 +- tests/qemuxml2xmltest.c | 2 + 39 files changed, 1092 insertions(+), 93 deletions(-) create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-doorbell-detach.xml create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-doorbell.xml create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain-detach.xml create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain.xml create mode 120000 tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell-detach.xml create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell.xml create mode 120000 tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-plain-detach.xml create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-plain.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args copy tests/qemuxml2argvdata/{qemuxml2argv-shmem.xml => qemuxml2argv-shmem-plain-doorbell.xml} (78%) copy tests/qemuxml2xmloutdata/{qemuxml2xmlout-shmem.xml => qemuxml2xmlout-shmem-plain-doorbell.xml} (78%) -- 2.10.0
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tests/qemuhotplugtest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 0a5f06834c14..6a9ed992f31d 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -240,7 +240,7 @@ testQemuHotplug(const void *data) virTestLoadFile(device_filename, &device_xml) < 0) goto cleanup; - if (test->action != UPDATE && + if (test->action == ATTACH && virTestLoadFile(result_filename, &result_xml) < 0) goto cleanup; -- 2.10.0
On Thu, Sep 15, 2016 at 18:14:26 +0200, Martin Kletzander wrote:
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tests/qemuhotplugtest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK
It already is used and tests will be automatically added in later patches. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/schemas/domaincommon.rng | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8aaa67e11115..751d9141deea 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3612,6 +3612,9 @@ </element> </optional> <optional> + <ref name="alias"/> + </optional> + <optional> <ref name="address"/> </optional> </interleave> -- 2.10.0
On Thu, Sep 15, 2016 at 18:14:27 +0200, Martin Kletzander wrote:
It already is used and tests will be automatically added in later patches.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/schemas/domaincommon.rng | 3 +++ 1 file changed, 3 insertions(+)
ACK
This way we'll be able to hotplug with both --live and --config in one API call. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0828041dd8e2..b4a3e92dd78a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -25019,11 +25019,14 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr src, case VIR_DOMAIN_DEVICE_MEMORY: rc = virDomainMemoryDefFormat(&buf, src->data.memory, flags); break; + case VIR_DOMAIN_DEVICE_SHMEM: + rc = virDomainShmemDefFormat(&buf, src->data.shmem, flags); + break; + case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: - case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_IOMMU: case VIR_DOMAIN_DEVICE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, -- 2.10.0
On Thu, Sep 15, 2016 at 18:14:28 +0200, Martin Kletzander wrote:
This way we'll be able to hotplug with both --live and --config in one API call.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
ACK
Just the default one now, new ones will be added in following commits. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/schemas/domaincommon.rng | 9 +++++ src/conf/domain_conf.c | 41 +++++++++++++++++------ src/conf/domain_conf.h | 8 +++++ src/libvirt_private.syms | 2 ++ src/qemu/qemu_command.c | 14 ++++++-- tests/qemuxml2argvdata/qemuxml2argv-shmem.xml | 2 ++ tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml | 8 +++++ 7 files changed, 70 insertions(+), 14 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 751d9141deea..3fb4f3999a01 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3586,6 +3586,15 @@ <attribute name="name"/> <interleave> <optional> + <element name="model"> + <attribute name="type"> + <choice> + <value>ivshmem</value> + </choice> + </attribute> + </element> + </optional> + <optional> <element name="size"> <ref name="scaledInteger"/> </element> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b4a3e92dd78a..97cb3de95529 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -843,6 +843,9 @@ VIR_ENUM_IMPL(virDomainBlockJob, VIR_DOMAIN_BLOCK_JOB_TYPE_LAST, VIR_ENUM_IMPL(virDomainMemoryModel, VIR_DOMAIN_MEMORY_MODEL_LAST, "", "dimm") +VIR_ENUM_IMPL(virDomainShmemModel, VIR_DOMAIN_SHMEM_MODEL_LAST, + "ivshmem") + static virClassPtr virDomainObjClass; static virClassPtr virDomainXMLOptionClass; static void virDomainObjDispose(void *obj); @@ -12198,6 +12201,20 @@ virDomainShmemDefParseXML(xmlNodePtr node, ctxt->node = node; + tmp = virXPathString("string(./model/@type)", ctxt); + if (tmp) { + /* If there's none, we will automatically have the first one + * (as default). Unfortunately this has to be done for + * compatibility reasons. */ + if ((def->model = virDomainShmemModelTypeFromString(tmp)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Unknown shmem model type '%s'"), tmp); + goto cleanup; + } + + VIR_FREE(tmp); + } + if (!(def->name = virXMLPropString(node, "name"))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("shmem element must contain 'name' attribute")); @@ -18687,6 +18704,15 @@ virDomainShmemDefCheckABIStability(virDomainShmemDefPtr src, return false; } + if (src->model != dst->model) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target shared memory model '%s' does not match " + "source model '%s'"), + virDomainShmemModelTypeToString(dst->model), + virDomainShmemModelTypeToString(src->model)); + return false; + } + if (src->size != dst->size) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target shared memory size '%llu' does not match " @@ -21764,20 +21790,13 @@ virDomainShmemDefFormat(virBufferPtr buf, virDomainShmemDefPtr def, unsigned int flags) { - virBufferEscapeString(buf, "<shmem name='%s'", def->name); - - if (!def->size && - !def->server.enabled && - !def->msi.enabled && - !virDomainDeviceInfoNeedsFormat(&def->info, flags)) { - virBufferAddLit(buf, "/>\n"); - return 0; - } else { - virBufferAddLit(buf, ">\n"); - } + virBufferEscapeString(buf, "<shmem name='%s'>\n", def->name); virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<model type='%s'/>\n", + virDomainShmemModelTypeToString(def->model)); + if (def->size) virBufferAsprintf(buf, "<size unit='M'>%llu</size>\n", def->size >> 20); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c14a39c439a6..5556b6d535f0 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1554,9 +1554,16 @@ struct _virDomainNVRAMDef { virDomainDeviceInfo info; }; +typedef enum { + VIR_DOMAIN_SHMEM_MODEL_IVSHMEM, + + VIR_DOMAIN_SHMEM_MODEL_LAST +} virDomainShmemModel; + struct _virDomainShmemDef { char *name; unsigned long long size; + int model; /* enum virDomainShmemModel */ struct { bool enabled; virDomainChrSourceDef chr; @@ -3044,6 +3051,7 @@ VIR_ENUM_DECL(virDomainTPMBackend) VIR_ENUM_DECL(virDomainMemoryModel) VIR_ENUM_DECL(virDomainMemoryBackingModel) VIR_ENUM_DECL(virDomainIOMMUModel) +VIR_ENUM_DECL(virDomainShmemModel) /* from libvirt.h */ VIR_ENUM_DECL(virDomainState) VIR_ENUM_DECL(virDomainNostateReason) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 80d5e86d0e34..409e0c0018e6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -454,6 +454,8 @@ virDomainSaveStatus; virDomainSaveXML; virDomainSeclabelTypeFromString; virDomainSeclabelTypeToString; +virDomainShmemModelTypeFromString; +virDomainShmemModelTypeToString; virDomainShutdownReasonTypeFromString; virDomainShutdownReasonTypeToString; virDomainShutoffReasonTypeFromString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3a61863b9abb..5eae0631a14f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8648,10 +8648,18 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager, { char *devstr = NULL; - if (!(devstr = qemuBuildShmemDevStr(def, shmem, qemuCaps))) + switch ((virDomainShmemModel)shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + if (!(devstr = qemuBuildShmemDevStr(def, shmem, qemuCaps))) + return -1; + virCommandAddArgList(cmd, "-device", devstr, NULL); + VIR_FREE(devstr); + break; + + /* coverity[dead_error_begin] */ + case VIR_DOMAIN_SHMEM_MODEL_LAST: return -1; - virCommandAddArgList(cmd, "-device", devstr, NULL); - VIR_FREE(devstr); + } if (shmem->server.enabled) { if (!(devstr = qemuBuildShmemBackendStr(logManager, cmd, cfg, def, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-shmem.xml b/tests/qemuxml2argvdata/qemuxml2argv-shmem.xml index 5bc49044894c..b56e9e187895 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-shmem.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-shmem.xml @@ -28,6 +28,7 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </shmem> <shmem name='shmem3'> + <model type='ivshmem'/> <size unit='M'>512</size> <server/> </shmem> @@ -41,6 +42,7 @@ <msi ioeventfd='off'/> </shmem> <shmem name='shmem6'> + <model type='ivshmem'/> <size unit='M'>4096</size> <server path='/tmp/shmem6-sock'/> <msi vectors='16'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml index 1197f361e3c4..5602913648bc 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml @@ -22,39 +22,47 @@ <input type='keyboard' bus='ps2'/> <memballoon model='none'/> <shmem name='shmem0'> + <model type='ivshmem'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> </shmem> <shmem name='shmem1'> + <model type='ivshmem'/> <size unit='M'>128</size> <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> </shmem> <shmem name='shmem2'> + <model type='ivshmem'/> <size unit='M'>256</size> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </shmem> <shmem name='shmem3'> + <model type='ivshmem'/> <size unit='M'>512</size> <server/> <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> </shmem> <shmem name='shmem4'> + <model type='ivshmem'/> <size unit='M'>1024</size> <server path='/tmp/shmem4-sock'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> </shmem> <shmem name='shmem5'> + <model type='ivshmem'/> <size unit='M'>2048</size> <server path='/tmp/shmem5-sock'/> <msi ioeventfd='off'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/> </shmem> <shmem name='shmem6'> + <model type='ivshmem'/> <size unit='M'>4096</size> <server path='/tmp/shmem6-sock'/> <msi vectors='16'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/> </shmem> <shmem name='shmem7'> + <model type='ivshmem'/> <size unit='M'>8192</size> <server path='/tmp/shmem7-sock'/> <msi vectors='32' ioeventfd='on'/> -- 2.10.0
On Thu, Sep 15, 2016 at 18:14:29 +0200, Martin Kletzander wrote:
Just the default one now, new ones will be added in following commits.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/schemas/domaincommon.rng | 9 +++++ src/conf/domain_conf.c | 41 +++++++++++++++++------ src/conf/domain_conf.h | 8 +++++ src/libvirt_private.syms | 2 ++ src/qemu/qemu_command.c | 14 ++++++-- tests/qemuxml2argvdata/qemuxml2argv-shmem.xml | 2 ++ tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml | 8 +++++ 7 files changed, 70 insertions(+), 14 deletions(-)
[...]
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3a61863b9abb..5eae0631a14f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8648,10 +8648,18 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager, { char *devstr = NULL;
- if (!(devstr = qemuBuildShmemDevStr(def, shmem, qemuCaps))) + switch ((virDomainShmemModel)shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + if (!(devstr = qemuBuildShmemDevStr(def, shmem, qemuCaps))) + return -1; + virCommandAddArgList(cmd, "-device", devstr, NULL); + VIR_FREE(devstr); + break; + + /* coverity[dead_error_begin] */
I'd don't really like stuff like this.
+ case VIR_DOMAIN_SHMEM_MODEL_LAST: return -1; - virCommandAddArgList(cmd, "-device", devstr, NULL); - VIR_FREE(devstr); + }
if (shmem->server.enabled) { if (!(devstr = qemuBuildShmemBackendStr(logManager, cmd, cfg, def,
ACK nevertheless. Peter
On Fri, Sep 16, 2016 at 09:12:23AM +0200, Peter Krempa wrote:
On Thu, Sep 15, 2016 at 18:14:29 +0200, Martin Kletzander wrote:
Just the default one now, new ones will be added in following commits.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/schemas/domaincommon.rng | 9 +++++ src/conf/domain_conf.c | 41 +++++++++++++++++------ src/conf/domain_conf.h | 8 +++++ src/libvirt_private.syms | 2 ++ src/qemu/qemu_command.c | 14 ++++++-- tests/qemuxml2argvdata/qemuxml2argv-shmem.xml | 2 ++ tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml | 8 +++++ 7 files changed, 70 insertions(+), 14 deletions(-)
[...]
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3a61863b9abb..5eae0631a14f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8648,10 +8648,18 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager, { char *devstr = NULL;
- if (!(devstr = qemuBuildShmemDevStr(def, shmem, qemuCaps))) + switch ((virDomainShmemModel)shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + if (!(devstr = qemuBuildShmemDevStr(def, shmem, qemuCaps))) + return -1; + virCommandAddArgList(cmd, "-device", devstr, NULL); + VIR_FREE(devstr); + break; + + /* coverity[dead_error_begin] */
I'd don't really like stuff like this.
Yeah, I know, me too, kinda, but look at it from this way. Would you rather like it here as a precaution or wasting another separate commit just for that one line with three paragraphs in the commit message? ;)
+ case VIR_DOMAIN_SHMEM_MODEL_LAST: return -1; - virCommandAddArgList(cmd, "-device", devstr, NULL); - VIR_FREE(devstr); + }
if (shmem->server.enabled) { if (!(devstr = qemuBuildShmemBackendStr(logManager, cmd, cfg, def,
ACK nevertheless.
Peter
On Fri, Sep 16, 2016 at 09:34:31 +0200, Martin Kletzander wrote:
On Fri, Sep 16, 2016 at 09:12:23AM +0200, Peter Krempa wrote:
On Thu, Sep 15, 2016 at 18:14:29 +0200, Martin Kletzander wrote:
Just the default one now, new ones will be added in following commits.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/schemas/domaincommon.rng | 9 +++++ src/conf/domain_conf.c | 41 +++++++++++++++++------ src/conf/domain_conf.h | 8 +++++ src/libvirt_private.syms | 2 ++ src/qemu/qemu_command.c | 14 ++++++-- tests/qemuxml2argvdata/qemuxml2argv-shmem.xml | 2 ++ tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml | 8 +++++ 7 files changed, 70 insertions(+), 14 deletions(-)
[...]
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3a61863b9abb..5eae0631a14f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8648,10 +8648,18 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager,
[...]
+ /* coverity[dead_error_begin] */
I'd don't really like stuff like this.
Yeah, I know, me too, kinda, but look at it from this way. Would you rather like it here as a precaution or wasting another separate commit just for that one line with three paragraphs in the commit message? ;)
Separate commit is better. It can be NACKed. Peter
Role controls how the domain behaves on migration. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/formatdomain.html.in | 10 +++++- docs/schemas/domaincommon.rng | 8 +++++ src/conf/domain_conf.c | 39 ++++++++++++++++++++++- src/conf/domain_conf.h | 11 +++++++ tests/qemuxml2argvdata/qemuxml2argv-shmem.xml | 4 +-- tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml | 4 +-- 6 files changed, 70 insertions(+), 6 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index f48a4d8b813f..f4d08959c787 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6747,7 +6747,7 @@ qemu-kvm -net nic,model=? /dev/null <pre> ... <devices> - <shmem name='my_shmem0'> + <shmem name='my_shmem0' role='peer'> <size unit='M'>4</size> </shmem> <shmem name='shmem_server'> @@ -6764,6 +6764,14 @@ qemu-kvm -net nic,model=? /dev/null <dd> The <code>shmem</code> element has one mandatory attribute, <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. </dd> <dt><code>size</code></dt> <dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 3fb4f3999a01..fd7d52d72515 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3584,6 +3584,14 @@ <define name="shmem"> <element name="shmem"> <attribute name="name"/> + <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 97cb3de95529..2ccc10515f30 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -846,6 +846,11 @@ VIR_ENUM_IMPL(virDomainMemoryModel, VIR_DOMAIN_MEMORY_MODEL_LAST, VIR_ENUM_IMPL(virDomainShmemModel, VIR_DOMAIN_SHMEM_MODEL_LAST, "ivshmem") +VIR_ENUM_IMPL(virDomainShmemRole, VIR_DOMAIN_SHMEM_ROLE_LAST, + "default", + "master", + "peer") + static virClassPtr virDomainObjClass; static virClassPtr virDomainXMLOptionClass; static void virDomainObjDispose(void *obj); @@ -12201,6 +12206,23 @@ virDomainShmemDefParseXML(xmlNodePtr node, ctxt->node = node; + if (!(def->name = virXMLPropString(node, "name"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("shmem element must contain 'name' attribute")); + goto cleanup; + } + + 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); + } + tmp = virXPathString("string(./model/@type)", ctxt); if (tmp) { /* If there's none, we will automatically have the first one @@ -18704,6 +18726,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 " @@ -21790,8 +21821,14 @@ 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 5556b6d535f0..bd674a565373 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1560,10 +1560,20 @@ 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; @@ -3052,6 +3062,7 @@ VIR_ENUM_DECL(virDomainMemoryModel) VIR_ENUM_DECL(virDomainMemoryBackingModel) VIR_ENUM_DECL(virDomainIOMMUModel) VIR_ENUM_DECL(virDomainShmemModel) +VIR_ENUM_DECL(virDomainShmemRole) /* from libvirt.h */ VIR_ENUM_DECL(virDomainState) VIR_ENUM_DECL(virDomainNostateReason) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-shmem.xml b/tests/qemuxml2argvdata/qemuxml2argv-shmem.xml index b56e9e187895..977947eb3355 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-shmem.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-shmem.xml @@ -20,10 +20,10 @@ <input type='keyboard' bus='ps2'/> <memballoon model='none'/> <shmem name='shmem0'/> - <shmem name='shmem1'> + <shmem name='shmem1' role='peer'> <size unit='M'>128</size> </shmem> - <shmem name='shmem2'> + <shmem name='shmem2' role='master'> <size unit='M'>256</size> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </shmem> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml index 5602913648bc..0a1579155170 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml @@ -25,12 +25,12 @@ <model type='ivshmem'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> </shmem> - <shmem name='shmem1'> + <shmem name='shmem1' role='peer'> <model type='ivshmem'/> <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'/> <size unit='M'>256</size> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> -- 2.10.0
On Thu, Sep 15, 2016 at 18:14:30 +0200, Martin Kletzander wrote:
Role controls how the domain behaves on migration.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/formatdomain.html.in | 10 +++++- docs/schemas/domaincommon.rng | 8 +++++ src/conf/domain_conf.c | 39 ++++++++++++++++++++++- src/conf/domain_conf.h | 11 +++++++ tests/qemuxml2argvdata/qemuxml2argv-shmem.xml | 4 +-- tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml | 4 +-- 6 files changed, 70 insertions(+), 6 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index f48a4d8b813f..f4d08959c787 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in
[...]
@@ -6764,6 +6764,14 @@ qemu-kvm -net nic,model=? /dev/null <dd> The <code>shmem</code> element has one mandatory attribute, <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.
I'm not in favor of adding the workaround suggestion. I'd change it for a more thoroguh explanation what this configuration does besides controlling migratability.
</dd> <dt><code>size</code></dt> <dd>
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 97cb3de95529..2ccc10515f30 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
[...]
@@ -12201,6 +12206,23 @@ virDomainShmemDefParseXML(xmlNodePtr node,
ctxt->node = node;
+ if (!(def->name = virXMLPropString(node, "name"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("shmem element must contain 'name' attribute")); + goto cleanup; + }
This code is already in the function a few lines after this hunk. You probably forgot to delete it.
+ + 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); + } + tmp = virXPathString("string(./model/@type)", ctxt); if (tmp) { /* If there's none, we will automatically have the first one @@ -18704,6 +18726,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; + }
Is this really guest ABI? Since it's not used in this patch I'll see later.
+ if (src->model != dst->model) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target shared memory model '%s' does not match " @@ -21790,8 +21821,14 @@ 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",
Peter
On Fri, Sep 16, 2016 at 09:20:16AM +0200, Peter Krempa wrote:
On Thu, Sep 15, 2016 at 18:14:30 +0200, Martin Kletzander wrote:
Role controls how the domain behaves on migration.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/formatdomain.html.in | 10 +++++- docs/schemas/domaincommon.rng | 8 +++++ src/conf/domain_conf.c | 39 ++++++++++++++++++++++- src/conf/domain_conf.h | 11 +++++++ tests/qemuxml2argvdata/qemuxml2argv-shmem.xml | 4 +-- tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml | 4 +-- 6 files changed, 70 insertions(+), 6 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index f48a4d8b813f..f4d08959c787 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in
[...]
@@ -6764,6 +6764,14 @@ qemu-kvm -net nic,model=? /dev/null <dd> The <code>shmem</code> element has one mandatory attribute, <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.
I'm not in favor of adding the workaround suggestion. I'd change it for a more thoroguh explanation what this configuration does besides controlling migratability.
Well, "technically", master controls the shm, where peer is just using it. When migrating, master will transfer that data in the stream. But as I understand it, peer would do the same due to the way how QEMU works and it memory is mapped. And since you cannot turn off copying that part of memory, qemu added a boolean 'master' that controls whether you can or cannot migrate. So it doesn't do anything else from a technical point of view, but it makes much more sense to have that separation from user's point of view as it feel closer how people think (I don't really know how to express what I mean any better, sorry).
</dd> <dt><code>size</code></dt> <dd>
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 97cb3de95529..2ccc10515f30 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
[...]
@@ -12201,6 +12206,23 @@ virDomainShmemDefParseXML(xmlNodePtr node,
ctxt->node = node;
+ if (!(def->name = virXMLPropString(node, "name"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("shmem element must contain 'name' attribute")); + goto cleanup; + }
This code is already in the function a few lines after this hunk. You probably forgot to delete it.
My bad, probably copy-paste or some other weird error.
+ + 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); + } + tmp = virXPathString("string(./model/@type)", ctxt); if (tmp) { /* If there's none, we will automatically have the first one @@ -18704,6 +18726,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; + }
Is this really guest ABI? Since it's not used in this patch I'll see later.
Well, it is not visible from the guest. Looking at it now (it's some time since I wrote this part) I think whatever is here was meant to be part of migration compatibility checks instead. I'll fix that up in v2, thanks for spotting that.
+ if (src->model != dst->model) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target shared memory model '%s' does not match " @@ -21790,8 +21821,14 @@ 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",
Peter
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/libvirt_private.syms | 2 ++ src/qemu/qemu_command.c | 4 ++++ tests/qemuxml2argvdata/qemuxml2argv-shmem.args | 4 ++-- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 409e0c0018e6..31020e6dc207 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -456,6 +456,8 @@ virDomainSeclabelTypeFromString; virDomainSeclabelTypeToString; virDomainShmemModelTypeFromString; virDomainShmemModelTypeToString; +virDomainShmemRoleTypeFromString; +virDomainShmemRoleTypeToString; virDomainShutdownReasonTypeFromString; virDomainShutdownReasonTypeToString; virDomainShutoffReasonTypeFromString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5eae0631a14f..60d662270cc8 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8583,6 +8583,10 @@ qemuBuildShmemDevStr(virDomainDefPtr def, if (!shmem->server.enabled) { virBufferAsprintf(&buf, ",shm=%s,id=%s", shmem->name, shmem->info.alias); + if (shmem->role) { + virBufferAsprintf(&buf, ",role=%s", + virDomainShmemRoleTypeToString(shmem->role)); + } } else { virBufferAsprintf(&buf, ",chardev=char%s,id=%s", shmem->info.alias, shmem->info.alias); if (shmem->msi.enabled) { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-shmem.args b/tests/qemuxml2argvdata/qemuxml2argv-shmem.args index 89caf499f8dd..dc69f4ab50fb 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-shmem.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-shmem.args @@ -18,8 +18,8 @@ QEMU_AUDIO_DRV=none \ -boot c \ -usb \ -device ivshmem,shm=shmem0,id=shmem0,bus=pci.0,addr=0x3 \ --device ivshmem,size=128m,shm=shmem1,id=shmem1,bus=pci.0,addr=0x5 \ --device ivshmem,size=256m,shm=shmem2,id=shmem2,bus=pci.0,addr=0x4 \ +-device ivshmem,size=128m,shm=shmem1,id=shmem1,role=peer,bus=pci.0,addr=0x5 \ +-device ivshmem,size=256m,shm=shmem2,id=shmem2,role=master,bus=pci.0,addr=0x4 \ -device ivshmem,size=512m,chardev=charshmem3,id=shmem3,bus=pci.0,addr=0x6 \ -chardev socket,id=charshmem3,path=/var/lib/libvirt/shmem-shmem3-sock \ -device ivshmem,size=1024m,chardev=charshmem4,id=shmem4,bus=pci.0,addr=0x7 \ -- 2.10.0
On Thu, Sep 15, 2016 at 18:14:31 +0200, Martin Kletzander wrote:
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/libvirt_private.syms | 2 ++ src/qemu/qemu_command.c | 4 ++++ tests/qemuxml2argvdata/qemuxml2argv-shmem.args | 4 ++-- 3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 409e0c0018e6..31020e6dc207 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -456,6 +456,8 @@ virDomainSeclabelTypeFromString; virDomainSeclabelTypeToString; virDomainShmemModelTypeFromString; virDomainShmemModelTypeToString; +virDomainShmemRoleTypeFromString; +virDomainShmemRoleTypeToString; virDomainShutdownReasonTypeFromString; virDomainShutdownReasonTypeToString; virDomainShutoffReasonTypeFromString;
This hunk probably belongs to previous patch.
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5eae0631a14f..60d662270cc8 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8583,6 +8583,10 @@ qemuBuildShmemDevStr(virDomainDefPtr def,
if (!shmem->server.enabled) { virBufferAsprintf(&buf, ",shm=%s,id=%s", shmem->name, shmem->info.alias); + if (shmem->role) { + virBufferAsprintf(&buf, ",role=%s", + virDomainShmemRoleTypeToString(shmem->role)); + }
Yup, previous patch needs more explanation.
} else { virBufferAsprintf(&buf, ",chardev=char%s,id=%s", shmem->info.alias, shmem->info.alias); if (shmem->msi.enabled) {
ACK
On Fri, Sep 16, 2016 at 09:22:39 +0200, Peter Krempa wrote:
On Thu, Sep 15, 2016 at 18:14:31 +0200, Martin Kletzander wrote:
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/libvirt_private.syms | 2 ++ src/qemu/qemu_command.c | 4 ++++ tests/qemuxml2argvdata/qemuxml2argv-shmem.args | 4 ++-- 3 files changed, 8 insertions(+), 2 deletions(-)
[...]
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5eae0631a14f..60d662270cc8 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8583,6 +8583,10 @@ qemuBuildShmemDevStr(virDomainDefPtr def,
if (!shmem->server.enabled) { virBufferAsprintf(&buf, ",shm=%s,id=%s", shmem->name, shmem->info.alias); + if (shmem->role) { + virBufferAsprintf(&buf, ",role=%s", + virDomainShmemRoleTypeToString(shmem->role)); + }
Yup, previous patch needs more explanation.
} else { virBufferAsprintf(&buf, ",chardev=char%s,id=%s", shmem->info.alias, shmem->info.alias); if (shmem->msi.enabled) {
ACK
I take back. I've read the commit you've pointed out a few patches later: * Property "role" replaced by "master". role=master becomes master=on, role=peer becomes master=off. Default is off instead of auto. I don't think it makes any sense to expose the old stuff since it does not work anyways and we never used it apparently.
The old ivshmem is deprecated in QEMU, so let's use the better ivshmem-{plain,doorbell} variants instead. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/schemas/domaincommon.rng | 2 ++ src/conf/domain_conf.c | 4 +++- src/conf/domain_conf.h | 2 ++ src/qemu/qemu_command.c | 7 +++++++ 4 files changed, 14 insertions(+), 1 deletion(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index fd7d52d72515..9827d6511781 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3598,6 +3598,8 @@ <attribute name="type"> <choice> <value>ivshmem</value> + <value>ivshmem-plain</value> + <value>ivshmem-doorbell</value> </choice> </attribute> </element> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2ccc10515f30..eeb8238c6b2f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -844,7 +844,9 @@ VIR_ENUM_IMPL(virDomainMemoryModel, VIR_DOMAIN_MEMORY_MODEL_LAST, "", "dimm") VIR_ENUM_IMPL(virDomainShmemModel, VIR_DOMAIN_SHMEM_MODEL_LAST, - "ivshmem") + "ivshmem", + "ivshmem-plain", + "ivshmem-doorbell") VIR_ENUM_IMPL(virDomainShmemRole, VIR_DOMAIN_SHMEM_ROLE_LAST, "default", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index bd674a565373..3013278458d8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1556,6 +1556,8 @@ struct _virDomainNVRAMDef { typedef enum { VIR_DOMAIN_SHMEM_MODEL_IVSHMEM, + VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN, + VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL, VIR_DOMAIN_SHMEM_MODEL_LAST } virDomainShmemModel; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 60d662270cc8..2b8e62dd4f30 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8660,6 +8660,13 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager, VIR_FREE(devstr); break; + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("%s device is not supported with this QEMU binary"), + virDomainShmemModelTypeToString(shmem->model)); + break; + /* coverity[dead_error_begin] */ case VIR_DOMAIN_SHMEM_MODEL_LAST: return -1; -- 2.10.0
On Thu, Sep 15, 2016 at 18:14:32 +0200, Martin Kletzander wrote:
The old ivshmem is deprecated in QEMU, so let's use the better ivshmem-{plain,doorbell} variants instead.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/schemas/domaincommon.rng | 2 ++ src/conf/domain_conf.c | 4 +++- src/conf/domain_conf.h | 2 ++ src/qemu/qemu_command.c | 7 +++++++ 4 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index fd7d52d72515..9827d6511781 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3598,6 +3598,8 @@ <attribute name="type"> <choice> <value>ivshmem</value> + <value>ivshmem-plain</value> + <value>ivshmem-doorbell</value>
I don't really like the qemu naming here but I don't have any better idea.
</choice> </attribute> </element>
Also note that using the new device models when migrating to older libvirt will break at the migration phase since older libvirt did not parse the model. Not sure whether it's worth worrying about though since this won't be used very much. Peter
On Fri, Sep 16, 2016 at 09:48:12 +0200, Peter Krempa wrote:
On Thu, Sep 15, 2016 at 18:14:32 +0200, Martin Kletzander wrote:
The old ivshmem is deprecated in QEMU, so let's use the better ivshmem-{plain,doorbell} variants instead.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/schemas/domaincommon.rng | 2 ++ src/conf/domain_conf.c | 4 +++- src/conf/domain_conf.h | 2 ++ src/qemu/qemu_command.c | 7 +++++++ 4 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index fd7d52d72515..9827d6511781 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3598,6 +3598,8 @@ <attribute name="type"> <choice> <value>ivshmem</value> + <value>ivshmem-plain</value> + <value>ivshmem-doorbell</value>
I don't really like the qemu naming here but I don't have any better idea.
</choice> </attribute> </element>
Also note that using the new device models when migrating to older libvirt will break at the migration phase since older libvirt did not parse the model. Not sure whether it's worth worrying about though since this won't be used very much.
I forgot to add: ACK
On Fri, Sep 16, 2016 at 09:48:12AM +0200, Peter Krempa wrote:
On Thu, Sep 15, 2016 at 18:14:32 +0200, Martin Kletzander wrote:
The old ivshmem is deprecated in QEMU, so let's use the better ivshmem-{plain,doorbell} variants instead.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/schemas/domaincommon.rng | 2 ++ src/conf/domain_conf.c | 4 +++- src/conf/domain_conf.h | 2 ++ src/qemu/qemu_command.c | 7 +++++++ 4 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index fd7d52d72515..9827d6511781 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3598,6 +3598,8 @@ <attribute name="type"> <choice> <value>ivshmem</value> + <value>ivshmem-plain</value> + <value>ivshmem-doorbell</value>
I don't really like the qemu naming here but I don't have any better idea.
I kinda wanted to do ivshmem-modern (and not split it between -plain and -doorbell), but "modern" is one of the worst naming options, the only worse one I can think of is ivshmem-new... So I left it this way.
</choice> </attribute> </element>
Also note that using the new device models when migrating to older libvirt will break at the migration phase since older libvirt did not parse the model. Not sure whether it's worth worrying about though since this won't be used very much.
Peter
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ src/qemu/qemu_capabilities.h | 2 ++ tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml | 2 ++ tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml | 2 ++ tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml | 2 ++ tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 2 ++ tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 2 ++ 7 files changed, 16 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index c71b4dcf44ca..a4beac351eb0 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -344,6 +344,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "query-hotpluggable-cpus", "virtio-net.rx_queue_size", /* 235 */ + "ivshmem-plain", + "ivshmem-doorbell", ); @@ -1568,6 +1570,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "pxb-pcie", QEMU_CAPS_DEVICE_PXB_PCIE }, { "tls-creds-x509", QEMU_CAPS_OBJECT_TLS_CREDS_X509 }, { "intel-iommu", QEMU_CAPS_DEVICE_INTEL_IOMMU }, + { "ivshmem-plain", QEMU_CAPS_DEVICE_IVSHMEM_PLAIN }, + { "ivshmem-doorbell", QEMU_CAPS_DEVICE_IVSHMEM_DOORBELL }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBalloon[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 26ac1fa6c364..88d1024536d9 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -378,6 +378,8 @@ typedef enum { /* 235 */ QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE, /* virtio-net-*.rx_queue_size */ + QEMU_CAPS_DEVICE_IVSHMEM_PLAIN, /* -device ivshmem-plain */ + QEMU_CAPS_DEVICE_IVSHMEM_DOORBELL, /* -device ivshmem-doorbell */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml index fd14665d5397..0644718ee081 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml @@ -159,6 +159,8 @@ <flag name='display'/> <flag name='smm'/> <flag name='virtio-pci-disable-legacy'/> + <flag name='ivshmem-plain'/> + <flag name='ivshmem-doorbell'/> <version>2005094</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml index eb708f8a983a..9526ae8b70b9 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml @@ -159,6 +159,8 @@ <flag name='display'/> <flag name='smm'/> <flag name='virtio-pci-disable-legacy'/> + <flag name='ivshmem-plain'/> + <flag name='ivshmem-doorbell'/> <version>2005094</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml index 482b3849b50f..f97d2b05ffa7 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml @@ -153,6 +153,8 @@ <flag name='display'/> <flag name='smm'/> <flag name='virtio-pci-disable-legacy'/> + <flag name='ivshmem-plain'/> + <flag name='ivshmem-doorbell'/> <version>2005094</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml index 60f1fcfe85d5..845a98bafe30 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml @@ -196,6 +196,8 @@ <flag name='intel-iommu'/> <flag name='smm'/> <flag name='virtio-pci-disable-legacy'/> + <flag name='ivshmem-plain'/> + <flag name='ivshmem-doorbell'/> <version>2006000</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml index 7a5404086bd1..1ed104680a04 100644 --- a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml @@ -194,6 +194,8 @@ <flag name='smm'/> <flag name='virtio-pci-disable-legacy'/> <flag name='query-hotpluggable-cpus'/> + <flag name='ivshmem-plain'/> + <flag name='ivshmem-doorbell'/> <version>2006091</version> <kvmVersion>0</kvmVersion> <package> (v2.7.0-rc1-52-g42e0d60)</package> -- 2.10.0
On Thu, Sep 15, 2016 at 18:14:33 +0200, Martin Kletzander wrote:
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ src/qemu/qemu_capabilities.h | 2 ++ tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml | 2 ++ tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml | 2 ++ tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml | 2 ++ tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 2 ++ tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 2 ++ 7 files changed, 16 insertions(+)
ACK
When we change the device used for the shared memory, it should not change the settings, so rather save them upfront then having problems later. The only thing we are not saving is the role for old ivshmem and that's because we were not specifying it, which means role=auto and that is really bad idea to be using (due to various things). However, we don't want to change the behaviour, so that's the reason for that. Details for the defaults of the newer implementation can be found in qemu's commit 5400c02b90bb: http://git.qemu.org/?p=qemu.git;a=commit;h=5400c02b90bb Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_domain.c | 33 +++++++++++++++++++++++ tests/qemuxml2argvdata/qemuxml2argv-shmem.args | 2 +- tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml | 3 ++- 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3f16dbee2e6a..1fdbdf68fc8b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2747,6 +2747,39 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, } } + if (dev->type == VIR_DOMAIN_DEVICE_SHMEM) { + if (!dev->data.shmem->server.enabled) { + /* The size is 4M if not specified */ + if (!dev->data.shmem->size) + dev->data.shmem->size = 4 << 20; + /* For old ivshmem we shouldn't change the role, the new + * ones are peer by default, mostly to safeguard that + * there should be no more than one master */ + if (!dev->data.shmem->role) { + if (dev->data.shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM) + dev->data.shmem->role = VIR_DOMAIN_SHMEM_ROLE_MASTER; + else + dev->data.shmem->role = VIR_DOMAIN_SHMEM_ROLE_PEER; + } + } else { + /* In QEMU, role doesn't make sense for server-side shmem */ + dev->data.shmem->role = VIR_DOMAIN_SHMEM_ROLE_DEFAULT; + + /* Defaults/Requirements for the newer device that we should save */ + if (dev->data.shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL) { + /* Size does not make much sense when claiming memory from + * the server and so the newer version doesn't support that */ + dev->data.shmem->size = 0; + + /* Also they can only exist with MSI and ioeventfd is + * enabled unless specifically disabled */ + dev->data.shmem->msi.enabled = true; + if (!dev->data.shmem->msi.ioeventfd) + dev->data.shmem->msi.ioeventfd = VIR_TRISTATE_SWITCH_ON; + } + } + } + ret = 0; cleanup: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-shmem.args b/tests/qemuxml2argvdata/qemuxml2argv-shmem.args index dc69f4ab50fb..d1624d5c00bd 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-shmem.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-shmem.args @@ -17,7 +17,7 @@ QEMU_AUDIO_DRV=none \ -no-acpi \ -boot c \ -usb \ --device ivshmem,shm=shmem0,id=shmem0,bus=pci.0,addr=0x3 \ +-device ivshmem,size=4m,shm=shmem0,id=shmem0,role=master,bus=pci.0,addr=0x3 \ -device ivshmem,size=128m,shm=shmem1,id=shmem1,role=peer,bus=pci.0,addr=0x5 \ -device ivshmem,size=256m,shm=shmem2,id=shmem2,role=master,bus=pci.0,addr=0x4 \ -device ivshmem,size=512m,chardev=charshmem3,id=shmem3,bus=pci.0,addr=0x6 \ diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml index 0a1579155170..d594806b9d64 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml @@ -21,8 +21,9 @@ <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> <memballoon model='none'/> - <shmem name='shmem0'> + <shmem name='shmem0' role='master'> <model type='ivshmem'/> + <size unit='M'>4</size> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> </shmem> <shmem name='shmem1' role='peer'> -- 2.10.0
On Thu, Sep 15, 2016 at 18:14:34 +0200, Martin Kletzander wrote:
When we change the device used for the shared memory, it should not change the settings, so rather save them upfront then having problems later.
The only thing we are not saving is the role for old ivshmem and that's because we were not specifying it, which means role=auto and that is really bad idea to be using (due to various things). However, we don't want to change the behaviour, so that's the reason for that.
Details for the defaults of the newer implementation can be found in qemu's commit 5400c02b90bb:
http://git.qemu.org/?p=qemu.git;a=commit;h=5400c02b90bb
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_domain.c | 33 +++++++++++++++++++++++ tests/qemuxml2argvdata/qemuxml2argv-shmem.args | 2 +- tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml | 3 ++- 3 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3f16dbee2e6a..1fdbdf68fc8b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2747,6 +2747,39 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, } }
+ if (dev->type == VIR_DOMAIN_DEVICE_SHMEM) { + if (!dev->data.shmem->server.enabled) { + /* The size is 4M if not specified */ + if (!dev->data.shmem->size) + dev->data.shmem->size = 4 << 20; + /* For old ivshmem we shouldn't change the role, the new + * ones are peer by default, mostly to safeguard that + * there should be no more than one master */ + if (!dev->data.shmem->role) { + if (dev->data.shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM) + dev->data.shmem->role = VIR_DOMAIN_SHMEM_ROLE_MASTER; + else + dev->data.shmem->role = VIR_DOMAIN_SHMEM_ROLE_PEER; + } + } else { + /* In QEMU, role doesn't make sense for server-side shmem */ + dev->data.shmem->role = VIR_DOMAIN_SHMEM_ROLE_DEFAULT;
Shouldn't this be VIR_DOMAIN_SHMEM_ROLE_PEER?
+ + /* Defaults/Requirements for the newer device that we should save */ + if (dev->data.shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL) { + /* Size does not make much sense when claiming memory from + * the server and so the newer version doesn't support that */ + dev->data.shmem->size = 0; + + /* Also they can only exist with MSI and ioeventfd is + * enabled unless specifically disabled */ + dev->data.shmem->msi.enabled = true; + if (!dev->data.shmem->msi.ioeventfd) + dev->data.shmem->msi.ioeventfd = VIR_TRISTATE_SWITCH_ON;
This does not tweak the number of ioeventfd vectors, which will probably remain 0.
+ } + } + }
The above code does not check the following two incompatible configs: 1) device is VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN and server is enabled 2) device is VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL and server is disabled
+ ret = 0;
Peter
On Fri, Sep 16, 2016 at 10:32:48AM +0200, Peter Krempa wrote:
On Thu, Sep 15, 2016 at 18:14:34 +0200, Martin Kletzander wrote:
When we change the device used for the shared memory, it should not change the settings, so rather save them upfront then having problems later.
The only thing we are not saving is the role for old ivshmem and that's because we were not specifying it, which means role=auto and that is really bad idea to be using (due to various things). However, we don't want to change the behaviour, so that's the reason for that.
Details for the defaults of the newer implementation can be found in qemu's commit 5400c02b90bb:
http://git.qemu.org/?p=qemu.git;a=commit;h=5400c02b90bb
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_domain.c | 33 +++++++++++++++++++++++ tests/qemuxml2argvdata/qemuxml2argv-shmem.args | 2 +- tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml | 3 ++- 3 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3f16dbee2e6a..1fdbdf68fc8b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2747,6 +2747,39 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, } }
+ if (dev->type == VIR_DOMAIN_DEVICE_SHMEM) { + if (!dev->data.shmem->server.enabled) { + /* The size is 4M if not specified */ + if (!dev->data.shmem->size) + dev->data.shmem->size = 4 << 20; + /* For old ivshmem we shouldn't change the role, the new + * ones are peer by default, mostly to safeguard that + * there should be no more than one master */ + if (!dev->data.shmem->role) { + if (dev->data.shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM) + dev->data.shmem->role = VIR_DOMAIN_SHMEM_ROLE_MASTER; + else + dev->data.shmem->role = VIR_DOMAIN_SHMEM_ROLE_PEER; + } + } else { + /* In QEMU, role doesn't make sense for server-side shmem */ + dev->data.shmem->role = VIR_DOMAIN_SHMEM_ROLE_DEFAULT;
Shouldn't this be VIR_DOMAIN_SHMEM_ROLE_PEER?
No, this should be DEFAULT, just the naming is weird. Doorbell has no roles (e.g. no 'master' parameter) and "DEFAULT" should actually be called "NONE".
+ + /* Defaults/Requirements for the newer device that we should save */ + if (dev->data.shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL) { + /* Size does not make much sense when claiming memory from + * the server and so the newer version doesn't support that */ + dev->data.shmem->size = 0; + + /* Also they can only exist with MSI and ioeventfd is + * enabled unless specifically disabled */ + dev->data.shmem->msi.enabled = true; + if (!dev->data.shmem->msi.ioeventfd) + dev->data.shmem->msi.ioeventfd = VIR_TRISTATE_SWITCH_ON;
This does not tweak the number of ioeventfd vectors, which will probably remain 0.
Default is 1, we can configure that if someone wants that in the future.
+ } + } + }
The above code does not check the following two incompatible configs: 1) device is VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN and server is enabled 2) device is VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL and server is disabled
I'll move those checks here as you suggested in some other patch.
+ ret = 0;
Peter
Such migration wouldn't work anyway with QEMU, we just haven't checked for it before. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_migration.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index e734816c4e12..d29b52b7aefa 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2346,6 +2346,19 @@ qemuMigrationIsAllowed(virQEMUDriverPtr driver, return false; } } + + for (i = 0; i < vm->def->nshmems; i++) { + virDomainShmemDefPtr shmem = vm->def->shmems[i]; + + if (shmem->role != VIR_DOMAIN_SHMEM_ROLE_MASTER) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("domain's shmem device '%s' has role='%s', " + "try unplugging it first"), + shmem->name, + virDomainShmemRoleTypeToString(shmem->role)); + return false; + } + } } return true; -- 2.10.0
On Thu, Sep 15, 2016 at 18:14:35 +0200, Martin Kletzander wrote:
Such migration wouldn't work anyway with QEMU, we just haven't checked for it before.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_migration.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index e734816c4e12..d29b52b7aefa 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2346,6 +2346,19 @@ qemuMigrationIsAllowed(virQEMUDriverPtr driver, return false; } } + + for (i = 0; i < vm->def->nshmems; i++) { + virDomainShmemDefPtr shmem = vm->def->shmems[i]; + + if (shmem->role != VIR_DOMAIN_SHMEM_ROLE_MASTER) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("domain's shmem device '%s' has role='%s', "
This includes "default" which may sound weird. You probably should convert "default" to something sane beforehand.
+ "try unplugging it first"),
I strongly disagree suggesting to the users to try hot(un)plug. Just state that migration is not supported with such config. Peter
On Fri, Sep 16, 2016 at 10:16:31AM +0200, Peter Krempa wrote:
On Thu, Sep 15, 2016 at 18:14:35 +0200, Martin Kletzander wrote:
Such migration wouldn't work anyway with QEMU, we just haven't checked for it before.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_migration.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index e734816c4e12..d29b52b7aefa 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2346,6 +2346,19 @@ qemuMigrationIsAllowed(virQEMUDriverPtr driver, return false; } } + + for (i = 0; i < vm->def->nshmems; i++) { + virDomainShmemDefPtr shmem = vm->def->shmems[i]; + + if (shmem->role != VIR_DOMAIN_SHMEM_ROLE_MASTER) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("domain's shmem device '%s' has role='%s', "
This includes "default" which may sound weird. You probably should convert "default" to something sane beforehand.
I can do a switch, I just wanted to forbid it for anything else than explicitly stated 'master' (in case there's more later). Default should not be here any more since postparse callback will make that into master/peer anyway.
+ "try unplugging it first"),
I strongly disagree suggesting to the users to try hot(un)plug. Just state that migration is not supported with such config.
OK, I'm fine with that.
Peter
It isn't used anywhere else. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_command.h | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2b8e62dd4f30..b99da23107f4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8546,7 +8546,7 @@ qemuBuildSmartcardCommandLine(virLogManagerPtr logManager, } -char * +static char * qemuBuildShmemDevStr(virDomainDefPtr def, virDomainShmemDefPtr shmem, virQEMUCapsPtr qemuCaps) diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 0c0b8f1a3853..2f2a6ff877e7 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -150,10 +150,6 @@ int qemuBuildRNGBackendProps(virDomainRNGDefPtr rng, const char **type, virJSONValuePtr *props); -char *qemuBuildShmemDevStr(virDomainDefPtr def, - virDomainShmemDefPtr shmem, - virQEMUCapsPtr qemuCaps); - int qemuOpenPCIConfig(virDomainHostdevDefPtr dev); /* Current, best practice */ -- 2.10.0
On Thu, Sep 15, 2016 at 18:14:36 +0200, Martin Kletzander wrote:
It isn't used anywhere else.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_command.h | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-)
ACK
This will make sense after adding support for newer device types. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b99da23107f4..8f9ad1963207 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8547,9 +8547,9 @@ qemuBuildSmartcardCommandLine(virLogManagerPtr logManager, static char * -qemuBuildShmemDevStr(virDomainDefPtr def, - virDomainShmemDefPtr shmem, - virQEMUCapsPtr qemuCaps) +qemuBuildShmemDevLegacyStr(virDomainDefPtr def, + virDomainShmemDefPtr shmem, + virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -8654,7 +8654,7 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager, switch ((virDomainShmemModel)shmem->model) { case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: - if (!(devstr = qemuBuildShmemDevStr(def, shmem, qemuCaps))) + if (!(devstr = qemuBuildShmemDevLegacyStr(def, shmem, qemuCaps))) return -1; virCommandAddArgList(cmd, "-device", devstr, NULL); VIR_FREE(devstr); -- 2.10.0
On Thu, Sep 15, 2016 at 18:14:37 +0200, Martin Kletzander wrote:
This will make sense after adding support for newer device types.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
ACK
Some checks will need to be performed for newer device types as well, so let's not duplicate them. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 54 +++++++++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8f9ad1963207..2a652d2747e8 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8557,29 +8557,12 @@ qemuBuildShmemDevLegacyStr(virDomainDefPtr def, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("ivshmem device is not supported " "with this QEMU binary")); - goto error; + return NULL; } virBufferAddLit(&buf, "ivshmem"); - if (shmem->size) { - /* - * Thanks to our parsing code, we have a guarantee that the - * size is power of two and is at least a mebibyte in size. - * But because it may change in the future, the checks are - * doubled in here. - */ - if (shmem->size & (shmem->size - 1)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("shmem size must be a power of two")); - goto error; - } - if (shmem->size < 1024 * 1024) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("shmem size must be at least 1 MiB (1024 KiB)")); - goto error; - } + if (shmem->size) virBufferAsprintf(&buf, ",size=%llum", shmem->size >> 20); - } if (!shmem->server.enabled) { virBufferAsprintf(&buf, ",shm=%s,id=%s", shmem->name, shmem->info.alias); @@ -8599,13 +8582,6 @@ qemuBuildShmemDevLegacyStr(virDomainDefPtr def, } } - if (shmem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("only 'pci' addresses are supported for the " - "shared memory device")); - goto error; - } - if (qemuBuildDeviceAddressStr(&buf, def, &shmem->info, qemuCaps) < 0) goto error; @@ -8652,6 +8628,32 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager, { char *devstr = NULL; + if (shmem->size) { + /* + * Thanks to our parsing code, we have a guarantee that the + * size is power of two and is at least a mebibyte in size. + * But because it may change in the future, the checks are + * doubled in here. + */ + if (shmem->size & (shmem->size - 1)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("shmem size must be a power of two")); + return -1; + } + if (shmem->size < 1024 * 1024) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("shmem size must be at least 1 MiB (1024 KiB)")); + return -1; + } + } + + if (shmem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only 'pci' addresses are supported for the " + "shared memory device")); + return -1; + } + switch ((virDomainShmemModel)shmem->model) { case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: if (!(devstr = qemuBuildShmemDevLegacyStr(def, shmem, qemuCaps))) -- 2.10.0
On Thu, Sep 15, 2016 at 18:14:38 +0200, Martin Kletzander wrote:
Some checks will need to be performed for newer device types as well, so let's not duplicate them.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 54 +++++++++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 26 deletions(-)
ACK
Always format id first so that we don't need to do that twice in different code paths. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 6 ++++-- tests/qemuxml2argvdata/qemuxml2argv-shmem.args | 16 ++++++++-------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2a652d2747e8..fdd656e241b3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8561,17 +8561,19 @@ qemuBuildShmemDevLegacyStr(virDomainDefPtr def, } virBufferAddLit(&buf, "ivshmem"); + virBufferAsprintf(&buf, ",id=%s", shmem->info.alias); + if (shmem->size) virBufferAsprintf(&buf, ",size=%llum", shmem->size >> 20); if (!shmem->server.enabled) { - virBufferAsprintf(&buf, ",shm=%s,id=%s", shmem->name, shmem->info.alias); + virBufferAsprintf(&buf, ",shm=%s", shmem->name); if (shmem->role) { virBufferAsprintf(&buf, ",role=%s", virDomainShmemRoleTypeToString(shmem->role)); } } else { - virBufferAsprintf(&buf, ",chardev=char%s,id=%s", shmem->info.alias, shmem->info.alias); + virBufferAsprintf(&buf, ",chardev=char%s", shmem->info.alias); if (shmem->msi.enabled) { virBufferAddLit(&buf, ",msi=on"); if (shmem->msi.vectors) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-shmem.args b/tests/qemuxml2argvdata/qemuxml2argv-shmem.args index d1624d5c00bd..cb0d53dd3c6a 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-shmem.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-shmem.args @@ -17,19 +17,19 @@ QEMU_AUDIO_DRV=none \ -no-acpi \ -boot c \ -usb \ --device ivshmem,size=4m,shm=shmem0,id=shmem0,role=master,bus=pci.0,addr=0x3 \ --device ivshmem,size=128m,shm=shmem1,id=shmem1,role=peer,bus=pci.0,addr=0x5 \ --device ivshmem,size=256m,shm=shmem2,id=shmem2,role=master,bus=pci.0,addr=0x4 \ --device ivshmem,size=512m,chardev=charshmem3,id=shmem3,bus=pci.0,addr=0x6 \ +-device ivshmem,id=shmem0,size=4m,shm=shmem0,role=master,bus=pci.0,addr=0x3 \ +-device ivshmem,id=shmem1,size=128m,shm=shmem1,role=peer,bus=pci.0,addr=0x5 \ +-device ivshmem,id=shmem2,size=256m,shm=shmem2,role=master,bus=pci.0,addr=0x4 \ +-device ivshmem,id=shmem3,size=512m,chardev=charshmem3,bus=pci.0,addr=0x6 \ -chardev socket,id=charshmem3,path=/var/lib/libvirt/shmem-shmem3-sock \ --device ivshmem,size=1024m,chardev=charshmem4,id=shmem4,bus=pci.0,addr=0x7 \ +-device ivshmem,id=shmem4,size=1024m,chardev=charshmem4,bus=pci.0,addr=0x7 \ -chardev socket,id=charshmem4,path=/tmp/shmem4-sock \ --device ivshmem,size=2048m,chardev=charshmem5,id=shmem5,msi=on,ioeventfd=off,\ +-device ivshmem,id=shmem5,size=2048m,chardev=charshmem5,msi=on,ioeventfd=off,\ bus=pci.0,addr=0x8 \ -chardev socket,id=charshmem5,path=/tmp/shmem5-sock \ --device ivshmem,size=4096m,chardev=charshmem6,id=shmem6,msi=on,vectors=16,\ +-device ivshmem,id=shmem6,size=4096m,chardev=charshmem6,msi=on,vectors=16,\ bus=pci.0,addr=0x9 \ -chardev socket,id=charshmem6,path=/tmp/shmem6-sock \ --device ivshmem,size=8192m,chardev=charshmem7,id=shmem7,msi=on,vectors=32,\ +-device ivshmem,id=shmem7,size=8192m,chardev=charshmem7,msi=on,vectors=32,\ ioeventfd=on,bus=pci.0,addr=0xa \ -chardev socket,id=charshmem7,path=/tmp/shmem7-sock -- 2.10.0
On Thu, Sep 15, 2016 at 18:14:39 +0200, Martin Kletzander wrote:
Always format id first so that we don't need to do that twice in different code paths.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 6 ++++-- tests/qemuxml2argvdata/qemuxml2argv-shmem.args | 16 ++++++++-------- 2 files changed, 12 insertions(+), 10 deletions(-)
ACK
Put it into qemuDomainPrepareShmemChardev() so it can be used later. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 5 +---- src/qemu/qemu_domain.c | 13 +++++++++++++ src/qemu/qemu_domain.h | 3 +++ 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index fdd656e241b3..aba7dc3db79b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8607,10 +8607,7 @@ qemuBuildShmemBackendStr(virLogManagerPtr logManager, { char *devstr = NULL; - if (!shmem->server.chr.data.nix.path && - virAsprintf(&shmem->server.chr.data.nix.path, - "/var/lib/libvirt/shmem-%s-sock", - shmem->name) < 0) + if (qemuDomainPrepareShmemChardev(shmem) < 0) return NULL; devstr = qemuBuildChrChardevStr(logManager, cmd, cfg, def, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1fdbdf68fc8b..940e216b7d1c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6089,6 +6089,19 @@ qemuDomainPrepareChannel(virDomainChrDefPtr channel, } +int +qemuDomainPrepareShmemChardev(virDomainShmemDefPtr shmem) +{ + if (!shmem->server.enabled || + shmem->server.chr.data.nix.path) + return 0; + + return virAsprintf(&shmem->server.chr.data.nix.path, + "/var/lib/libvirt/shmem-%s-sock", + shmem->name); +} + + /** * qemuDomainVcpuHotplugIsInOrder: * @def: domain definition diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index a1404d037825..c2fab3ba3521 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -722,6 +722,9 @@ int qemuDomainPrepareChannel(virDomainChrDefPtr chr, const char *domainChannelTargetDir) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int qemuDomainPrepareShmemChardev(virDomainShmemDefPtr shmem) + ATTRIBUTE_NONNULL(1); + bool qemuDomainVcpuHotplugIsInOrder(virDomainDefPtr def) ATTRIBUTE_NONNULL(1); -- 2.10.0
On Thu, Sep 15, 2016 at 18:14:40 +0200, Martin Kletzander wrote:
Put it into qemuDomainPrepareShmemChardev() so it can be used later.
This also adds a condition when the unix socket path is not generated when not required. You should mention this fix here.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 5 +---- src/qemu/qemu_domain.c | 13 +++++++++++++ src/qemu/qemu_domain.h | 3 +++ 3 files changed, 17 insertions(+), 4 deletions(-)
ACK
There will be more backends in the future so let's not complicate it. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index aba7dc3db79b..a28d503943d2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8598,12 +8598,12 @@ qemuBuildShmemDevLegacyStr(virDomainDefPtr def, } static char * -qemuBuildShmemBackendStr(virLogManagerPtr logManager, - virCommandPtr cmd, - virQEMUDriverConfigPtr cfg, - virDomainDefPtr def, - virDomainShmemDefPtr shmem, - virQEMUCapsPtr qemuCaps) +qemuBuildShmemBackendChrStr(virLogManagerPtr logManager, + virCommandPtr cmd, + virQEMUDriverConfigPtr cfg, + virDomainDefPtr def, + virDomainShmemDefPtr shmem, + virQEMUCapsPtr qemuCaps) { char *devstr = NULL; @@ -8674,8 +8674,8 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager, } if (shmem->server.enabled) { - if (!(devstr = qemuBuildShmemBackendStr(logManager, cmd, cfg, def, - shmem, qemuCaps))) + if (!(devstr = qemuBuildShmemBackendChrStr(logManager, cmd, cfg, def, + shmem, qemuCaps))) return -1; virCommandAddArgList(cmd, "-chardev", devstr, NULL); -- 2.10.0
On Thu, Sep 15, 2016 at 18:14:41 +0200, Martin Kletzander wrote:
There will be more backends in the future so let's not complicate it.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
ACK
QEMU added support for ivshmem-plain and ivshmem-doorbell. Those are reworked varians of legacy ivshmem that are compatible from the guest POV, but not from host's POV and have sane specification and handling. Details about the newer device type can be found in qemu's commit 5400c02b90bb: http://git.qemu.org/?p=qemu.git;a=commit;h=5400c02b90bb Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 137 ++++++++++++++++++++- src/qemu/qemu_command.h | 10 ++ .../qemuxml2argv-shmem-plain-doorbell.args | 46 +++++++ .../qemuxml2argv-shmem-plain-doorbell.xml | 63 ++++++++++ tests/qemuxml2argvtest.c | 3 + .../qemuxml2xmlout-shmem-plain-doorbell.xml | 70 +++++++++++ tests/qemuxml2xmltest.c | 2 + 7 files changed, 328 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem-plain-doorbell.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a28d503943d2..45e5b8e5ce27 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8597,6 +8597,82 @@ qemuBuildShmemDevLegacyStr(virDomainDefPtr def, return NULL; } +char * +qemuBuildShmemDevStr(virDomainDefPtr def, + virDomainShmemDefPtr shmem, + virQEMUCapsPtr qemuCaps) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN) { + if (shmem->server.enabled) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("ivshmem-plain is only supported without server " + "option, try using ivshmem-doorbell instead")); + return NULL; + } + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IVSHMEM_PLAIN)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("ivshmem-plain device is not supported " + "with this QEMU binary")); + return NULL; + } + } else { + if (!shmem->server.enabled) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("ivshmem-doorbell is only supported with server " + "option, try using ivshmem-doorbell instead")); + return NULL; + } + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IVSHMEM_PLAIN)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("ivshmem-doorbell device is not supported " + "with this QEMU binary")); + return NULL; + } + } + + if (shmem->server.enabled) { + virBufferAddLit(&buf, "ivshmem-doorbell"); + virBufferAsprintf(&buf, ",id=%s,chardev=char%s", + shmem->info.alias, shmem->info.alias); + if (shmem->msi.vectors) + virBufferAsprintf(&buf, ",vectors=%u", shmem->msi.vectors); + if (shmem->msi.ioeventfd) + virBufferAsprintf(&buf, ",ioeventfd=%s", + virTristateSwitchTypeToString(shmem->msi.ioeventfd)); + } else { + virBufferAddLit(&buf, "ivshmem-plain"); + virBufferAsprintf(&buf, ",id=%s,memdev=shmmem-%s", + shmem->info.alias, shmem->info.alias); + + virBufferAddLit(&buf, ",master="); + switch ((virDomainShmemRole) shmem->role) { + case VIR_DOMAIN_SHMEM_ROLE_MASTER: + virBufferAddLit(&buf, "on"); + break; + case VIR_DOMAIN_SHMEM_ROLE_PEER: + virBufferAddLit(&buf, "off"); + break; + case VIR_DOMAIN_SHMEM_ROLE_DEFAULT: + case VIR_DOMAIN_SHMEM_ROLE_LAST: + break; + } + } + + if (qemuBuildDeviceAddressStr(&buf, def, &shmem->info, qemuCaps) < 0) { + virBufferFreeAndReset(&buf); + return NULL; + } + + if (virBufferCheckError(&buf) < 0) + return NULL; + + return virBufferContentAndReset(&buf); +} + static char * qemuBuildShmemBackendChrStr(virLogManagerPtr logManager, virCommandPtr cmd, @@ -8617,6 +8693,51 @@ qemuBuildShmemBackendChrStr(virLogManagerPtr logManager, return devstr; } + +virJSONValuePtr +qemuBuildShmemBackendMemProps(virDomainShmemDefPtr shmem) +{ + char *mem_path = NULL; + virJSONValuePtr ret = NULL; + + if (virAsprintf(&mem_path, "/dev/shm/%s", shmem->name) < 0) + return NULL; + + virJSONValueObjectCreate(&ret, + "U:size", shmem->size, + "s:mem-path", mem_path, + NULL); + + VIR_FREE(mem_path); + + return ret; +} + + +static char * +qemuBuildShmemBackendMemStr(virDomainShmemDefPtr shmem) +{ + char *ret = NULL; + char *alias = NULL; + virJSONValuePtr props = qemuBuildShmemBackendMemProps(shmem); + + if (!props) + return NULL; + + if (virAsprintf(&alias, "shmmem-%s", shmem->info.alias) < 0) + goto cleanup; + + ret = virQEMUBuildObjectCommandlineFromJSON("memory-backend-file", + alias, + props); + cleanup: + VIR_FREE(alias); + virJSONValueFree(props); + + return ret; +} + + static int qemuBuildShmemCommandLine(virLogManagerPtr logManager, virCommandPtr cmd, @@ -8663,9 +8784,19 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager, case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("%s device is not supported with this QEMU binary"), - virDomainShmemModelTypeToString(shmem->model)); + if (!(devstr = qemuBuildShmemDevStr(def, shmem, qemuCaps))) + return -1; + + virCommandAddArgList(cmd, "-device", devstr, NULL); + VIR_FREE(devstr); + + if (!shmem->server.enabled) { + if (!(devstr = qemuBuildShmemBackendMemStr(shmem))) + return -1; + + virCommandAddArgList(cmd, "-object", devstr, NULL); + VIR_FREE(devstr); + } break; /* coverity[dead_error_begin] */ diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 2f2a6ff877e7..facc833bf886 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -186,4 +186,14 @@ bool qemuCheckCCWS390AddressSupport(const virDomainDef *def, virJSONValuePtr qemuBuildHotpluggableCPUProps(const virDomainVcpuDef *vcpu) ATTRIBUTE_NONNULL(1); +virJSONValuePtr qemuBuildShmemBackendMemProps(virDomainShmemDefPtr shmem) + ATTRIBUTE_NONNULL(1); + +char *qemuBuildShmemDevStr(virDomainDefPtr def, + virDomainShmemDefPtr shmem, + virQEMUCapsPtr qemuCaps) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + + + #endif /* __QEMU_COMMAND_H__*/ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args b/tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args new file mode 100644 index 000000000000..8d58b311a5c8 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args @@ -0,0 +1,46 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-device ivshmem-plain,id=shmem0,memdev=shmmem-shmem0,master=off,bus=pci.0,\ +addr=0x3 \ +-object memory-backend-file,id=shmmem-shmem0,size=4194304,\ +mem-path=/dev/shm/shmem0 \ +-device ivshmem-plain,id=shmem1,memdev=shmmem-shmem1,master=off,bus=pci.0,\ +addr=0x5 \ +-object memory-backend-file,id=shmmem-shmem1,size=134217728,\ +mem-path=/dev/shm/shmem1 \ +-device ivshmem-plain,id=shmem2,memdev=shmmem-shmem2,master=on,bus=pci.0,\ +addr=0x4 \ +-object memory-backend-file,id=shmmem-shmem2,size=268435456,\ +mem-path=/dev/shm/shmem2 \ +-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 \ +-device ivshmem-doorbell,id=shmem4,chardev=charshmem4,ioeventfd=on,bus=pci.0,\ +addr=0x7 \ +-chardev socket,id=charshmem4,path=/tmp/shmem4-sock \ +-device ivshmem-doorbell,id=shmem5,chardev=charshmem5,ioeventfd=off,bus=pci.0,\ +addr=0x8 \ +-chardev socket,id=charshmem5,path=/tmp/shmem5-sock \ +-device ivshmem-doorbell,id=shmem6,chardev=charshmem6,vectors=16,ioeventfd=on,\ +bus=pci.0,addr=0x9 \ +-chardev socket,id=charshmem6,path=/tmp/shmem6-sock \ +-device ivshmem-doorbell,id=shmem7,chardev=charshmem7,vectors=32,ioeventfd=on,\ +bus=pci.0,addr=0xa \ +-chardev socket,id=charshmem7,path=/tmp/shmem7-sock diff --git a/tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.xml b/tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.xml new file mode 100644 index 000000000000..23a7b27e87b1 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.xml @@ -0,0 +1,63 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + <shmem name='shmem0'> + <model type='ivshmem-plain'/> + </shmem> + <shmem name='shmem1' role='peer'> + <model type='ivshmem-plain'/> + <size unit='M'>128</size> + </shmem> + <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'/> + </shmem> + <shmem name='shmem3'> + <model type='ivshmem-doorbell'/> + <size unit='M'>512</size> + <server/> + </shmem> + <shmem name='shmem4'> + <model type='ivshmem-doorbell'/> + <size unit='M'>1024</size> + <server path='/tmp/shmem4-sock'/> + </shmem> + <shmem name='shmem5'> + <model type='ivshmem-doorbell'/> + <size unit='M'>2048</size> + <server path='/tmp/shmem5-sock'/> + <msi ioeventfd='off'/> + </shmem> + <shmem name='shmem6'> + <model type='ivshmem-doorbell'/> + <size unit='M'>4096</size> + <server path='/tmp/shmem6-sock'/> + <msi vectors='16'/> + </shmem> + <shmem name='shmem7'> + <model type='ivshmem-doorbell'/> + <size unit='M'>8192</size> + <server path='/tmp/shmem7-sock'/> + <msi vectors='32' ioeventfd='on'/> + </shmem> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index dbb0e4d56142..3f2c4ea152ba 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1987,6 +1987,9 @@ mymain(void) DO_TEST("fips-enabled", QEMU_CAPS_ENABLE_FIPS); DO_TEST("shmem", QEMU_CAPS_DEVICE_IVSHMEM); + DO_TEST("shmem-plain-doorbell", QEMU_CAPS_DEVICE_IVSHMEM, + QEMU_CAPS_DEVICE_IVSHMEM_PLAIN, + QEMU_CAPS_DEVICE_IVSHMEM_DOORBELL); DO_TEST_FAILURE("shmem", NONE); DO_TEST_FAILURE("shmem-invalid-size", QEMU_CAPS_DEVICE_IVSHMEM); diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem-plain-doorbell.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem-plain-doorbell.xml new file mode 100644 index 000000000000..0b3499335af5 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem-plain-doorbell.xml @@ -0,0 +1,70 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + <shmem name='shmem0' role='peer'> + <model type='ivshmem-plain'/> + <size unit='M'>4</size> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </shmem> + <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' role='master'> + <model type='ivshmem-plain'/> + <size unit='M'>256</size> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </shmem> + <shmem name='shmem3'> + <model type='ivshmem-doorbell'/> + <server/> + <msi ioeventfd='on'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </shmem> + <shmem name='shmem4'> + <model type='ivshmem-doorbell'/> + <server path='/tmp/shmem4-sock'/> + <msi ioeventfd='on'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> + </shmem> + <shmem name='shmem5'> + <model type='ivshmem-doorbell'/> + <server path='/tmp/shmem5-sock'/> + <msi ioeventfd='off'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/> + </shmem> + <shmem name='shmem6'> + <model type='ivshmem-doorbell'/> + <server path='/tmp/shmem6-sock'/> + <msi vectors='16' ioeventfd='on'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/> + </shmem> + <shmem name='shmem7'> + <model type='ivshmem-doorbell'/> + <server path='/tmp/shmem7-sock'/> + <msi vectors='32' ioeventfd='on'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/> + </shmem> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 14c2b0ccf2ce..ce32be47827b 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -812,6 +812,8 @@ mymain(void) DO_TEST("tap-vhost", NONE); DO_TEST("tap-vhost-incorrect", NONE); DO_TEST("shmem", 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); -- 2.10.0
On Thu, Sep 15, 2016 at 18:14:42 +0200, Martin Kletzander wrote:
QEMU added support for ivshmem-plain and ivshmem-doorbell. Those are reworked varians of legacy ivshmem that are compatible from the guest POV, but not from host's POV and have sane specification and handling.
Details about the newer device type can be found in qemu's commit 5400c02b90bb:
http://git.qemu.org/?p=qemu.git;a=commit;h=5400c02b90bb
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_command.c | 137 ++++++++++++++++++++- src/qemu/qemu_command.h | 10 ++ .../qemuxml2argv-shmem-plain-doorbell.args | 46 +++++++ .../qemuxml2argv-shmem-plain-doorbell.xml | 63 ++++++++++ tests/qemuxml2argvtest.c | 3 + .../qemuxml2xmlout-shmem-plain-doorbell.xml | 70 +++++++++++ tests/qemuxml2xmltest.c | 2 + 7 files changed, 328 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem-plain-doorbell.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a28d503943d2..45e5b8e5ce27 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8597,6 +8597,82 @@ qemuBuildShmemDevLegacyStr(virDomainDefPtr def, return NULL; }
+char * +qemuBuildShmemDevStr(virDomainDefPtr def, + virDomainShmemDefPtr shmem, + virQEMUCapsPtr qemuCaps) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN) {
This is weird and unflexible and not checked by the compiler. Either duplicate the switch from the caller function here or preferably split the generators for the doorbel and plain device into separate functions rather than separate branches.
+ if (shmem->server.enabled) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("ivshmem-plain is only supported without server " + "option, try using ivshmem-doorbell instead")); + return NULL; + }
Since you've added post parse handling for this you can move checks like this there so they don't clutter up the command line generator.
+ + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IVSHMEM_PLAIN)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("ivshmem-plain device is not supported " + "with this QEMU binary")); + return NULL; + } + } else { + if (!shmem->server.enabled) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("ivshmem-doorbell is only supported with server " + "option, try using ivshmem-doorbell instead")); + return NULL; + } + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IVSHMEM_PLAIN)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("ivshmem-doorbell device is not supported " + "with this QEMU binary")); + return NULL; + } + } + + if (shmem->server.enabled) {
This is true but needs to be compiled from implications of the above check. Decide based on the type and make sure that configuration is right for the given type rather than relying on other stuff.
+ virBufferAddLit(&buf, "ivshmem-doorbell"); + virBufferAsprintf(&buf, ",id=%s,chardev=char%s", + shmem->info.alias, shmem->info.alias); + if (shmem->msi.vectors) + virBufferAsprintf(&buf, ",vectors=%u", shmem->msi.vectors); + if (shmem->msi.ioeventfd) + virBufferAsprintf(&buf, ",ioeventfd=%s", + virTristateSwitchTypeToString(shmem->msi.ioeventfd)); + } else { + virBufferAddLit(&buf, "ivshmem-plain"); + virBufferAsprintf(&buf, ",id=%s,memdev=shmmem-%s", + shmem->info.alias, shmem->info.alias); + + virBufferAddLit(&buf, ",master="); + switch ((virDomainShmemRole) shmem->role) { + case VIR_DOMAIN_SHMEM_ROLE_MASTER: + virBufferAddLit(&buf, "on"); + break; + case VIR_DOMAIN_SHMEM_ROLE_PEER: + virBufferAddLit(&buf, "off"); + break; + case VIR_DOMAIN_SHMEM_ROLE_DEFAULT: + case VIR_DOMAIN_SHMEM_ROLE_LAST: + break; + } + }
Peter
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_alias.c | 32 +++++++++++++++++++++++++++++++- src/qemu/qemu_alias.h | 4 ++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 9094f3b137ef..cc83fec26862 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -353,6 +353,36 @@ qemuAssignDeviceMemoryAlias(virDomainDefPtr def, int +qemuAssignDeviceShmemAlias(virDomainDefPtr def, + virDomainShmemDefPtr shmem, + int idx) +{ + if (idx == -1) { + size_t i; + idx = 0; + for (i = 0; i < def->nshmems; i++) { + int thisidx; + + if ((thisidx = qemuDomainDeviceAliasIndex(&def->shmems[i]->info, + "shmem")) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to determine device index " + "for shmem device")); + return -1; + } + + if (thisidx >= idx) + idx = thisidx + 1; + } + } + + if (virAsprintf(&shmem->info.alias, "shmem%d", idx) < 0) + return -1; + return 0; +} + + +int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) { size_t i; @@ -421,7 +451,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) return -1; } for (i = 0; i < def->nshmems; i++) { - if (virAsprintf(&def->shmems[i]->info.alias, "shmem%zu", i) < 0) + if (qemuAssignDeviceShmemAlias(def, def->shmems[i], i) < 0) return -1; } for (i = 0; i < def->nsmartcards; i++) { diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index c366d28afc47..11d9fde2ccc2 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -60,6 +60,10 @@ int qemuAssignDeviceRNGAlias(virDomainDefPtr def, int qemuAssignDeviceMemoryAlias(virDomainDefPtr def, virDomainMemoryDefPtr mems); +int qemuAssignDeviceShmemAlias(virDomainDefPtr def, + virDomainShmemDefPtr shmem, + int idx); + int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps); int qemuDomainDeviceAliasIndex(const virDomainDeviceInfo *info, -- 2.10.0
On Thu, Sep 15, 2016 at 18:14:43 +0200, Martin Kletzander wrote:
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_alias.c | 32 +++++++++++++++++++++++++++++++- src/qemu/qemu_alias.h | 4 ++++ 2 files changed, 35 insertions(+), 1 deletion(-)
ACK
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 ++++++ src/libvirt_private.syms | 5 ++++ 3 files changed, 89 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index eeb8238c6b2f..11c7c5dd1f34 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14803,6 +14803,81 @@ virDomainRedirdevDefRemove(virDomainDefPtr def, size_t idx) } +int +virDomainShmemDefInsert(virDomainDefPtr def, + virDomainShmemDefPtr shmem) +{ + return VIR_APPEND_ELEMENT(def->shmems, def->nshmems, shmem); +} + + +bool +virDomainShmemDefEquals(virDomainShmemDefPtr src, + virDomainShmemDefPtr dst) +{ + if (STRNEQ_NULLABLE(src->name, dst->name)) + return false; + + if (src->size != dst->size) + return false; + + if (src->server.enabled != dst->server.enabled) + return false; + + if (src->server.enabled) { + if (STRNEQ_NULLABLE(src->server.chr.data.nix.path, + dst->server.chr.data.nix.path)) + return false; + } + + if (src->msi.enabled != dst->msi.enabled) + return false; + + if (src->msi.enabled) { + if (src->msi.vectors != dst->msi.vectors) + return false; + if (src->msi.ioeventfd != dst->msi.ioeventfd) + return false; + } + + if (src->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + !virDomainDeviceInfoAddressIsEqual(&src->info, &dst->info)) + return false; + + return true; +} + + +ssize_t +virDomainShmemDefFind(virDomainDefPtr def, + virDomainShmemDefPtr shmem) +{ + size_t i; + + for (i = 0; i < def->nshmems; i++) { + if (virDomainShmemDefEquals(def->shmems[i], shmem)) + break; + } + + if (i < def->nshmems) + return i; + + return -1; +} + + +virDomainShmemDefPtr +virDomainShmemDefRemove(virDomainDefPtr def, + size_t idx) +{ + virDomainShmemDefPtr ret = def->shmems[idx]; + + VIR_DELETE_ELEMENT(def->shmems, idx, def->nshmems); + + return ret; +} + + char * virDomainDefGetDefaultEmulator(virDomainDefPtr def, virCapsPtr caps) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3013278458d8..ec2cc09979cc 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2985,6 +2985,15 @@ int virDomainMemoryFindInactiveByDef(virDomainDefPtr def, virDomainMemoryDefPtr mem) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +int virDomainShmemDefInsert(virDomainDefPtr def, virDomainShmemDefPtr shmem) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +bool virDomainShmemDefEquals(virDomainShmemDefPtr src, virDomainShmemDefPtr dst) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +ssize_t virDomainShmemDefFind(virDomainDefPtr def, virDomainShmemDefPtr shmem) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +virDomainShmemDefPtr virDomainShmemDefRemove(virDomainDefPtr def, size_t idx) + ATTRIBUTE_NONNULL(1); + VIR_ENUM_DECL(virDomainTaint) VIR_ENUM_DECL(virDomainVirt) VIR_ENUM_DECL(virDomainBoot) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 31020e6dc207..e804d27e36b9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -454,6 +454,11 @@ virDomainSaveStatus; virDomainSaveXML; virDomainSeclabelTypeFromString; virDomainSeclabelTypeToString; +virDomainShmemDefEquals; +virDomainShmemDefFind; +virDomainShmemDefFree; +virDomainShmemDefInsert; +virDomainShmemDefRemove; virDomainShmemModelTypeFromString; virDomainShmemModelTypeToString; virDomainShmemRoleTypeFromString; -- 2.10.0
On Thu, Sep 15, 2016 at 18:14:44 +0200, Martin Kletzander wrote:
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 ++++++ src/libvirt_private.syms | 5 ++++ 3 files changed, 89 insertions(+)
ACK
This is needed in order to migrate with ivshmem role='peer' as that is not allowed to migrate. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 39 +++- src/qemu/qemu_hotplug.c | 247 ++++++++++++++++++++- src/qemu/qemu_hotplug.h | 6 + tests/qemuhotplugtest.c | 21 ++ .../qemuhotplug-ivshmem-doorbell-detach.xml | 7 + .../qemuhotplug-ivshmem-doorbell.xml | 4 + .../qemuhotplug-ivshmem-plain-detach.xml | 6 + .../qemuhotplug-ivshmem-plain.xml | 3 + ...muhotplug-base-live+ivshmem-doorbell-detach.xml | 1 + .../qemuhotplug-base-live+ivshmem-doorbell.xml | 65 ++++++ .../qemuhotplug-base-live+ivshmem-plain-detach.xml | 1 + .../qemuhotplug-base-live+ivshmem-plain.xml | 58 +++++ 12 files changed, 453 insertions(+), 5 deletions(-) create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-doorbell-detach.xml create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-doorbell.xml create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain-detach.xml create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain.xml create mode 120000 tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell-detach.xml create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell.xml create mode 120000 tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-plain-detach.xml create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-plain.xml diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e29180da4e75..0625df273f2f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7522,6 +7522,15 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, dev->data.memory = NULL; break; + case VIR_DOMAIN_DEVICE_SHMEM: + ret = qemuDomainAttachShmemDevice(driver, vm, + dev->data.shmem); + if (!ret) { + alias = dev->data.shmem->info.alias; + dev->data.shmem = NULL; + } + break; + case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: @@ -7533,7 +7542,6 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: - case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: case VIR_DOMAIN_DEVICE_IOMMU: @@ -7611,6 +7619,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_MEMORY: ret = qemuDomainDetachMemoryDevice(driver, vm, dev->data.memory); break; + case VIR_DOMAIN_DEVICE_SHMEM: + ret = qemuDomainDetachShmemDevice(driver, vm, dev); + break; case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: @@ -7622,7 +7633,6 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: - case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_REDIRDEV: case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_TPM: @@ -7769,6 +7779,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainControllerDefPtr controller; virDomainFSDefPtr fs; virDomainRedirdevDefPtr redirdev; + virDomainShmemDefPtr shmem; switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: @@ -7893,6 +7904,18 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, dev->data.redirdev = NULL; break; + case VIR_DOMAIN_DEVICE_SHMEM: + shmem = dev->data.shmem; + if (virDomainShmemDefFind(vmdef, shmem) >= 0) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("device is already in the domain configuration")); + return -1; + } + if (virDomainShmemDefInsert(vmdef, shmem) < 0) + return -1; + dev->data.shmem = NULL; + break; + case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: case VIR_DOMAIN_DEVICE_VIDEO: @@ -7902,7 +7925,6 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: - case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: @@ -8049,6 +8071,16 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainRedirdevDefFree(virDomainRedirdevDefRemove(vmdef, idx)); break; + case VIR_DOMAIN_DEVICE_SHMEM: + if ((idx = virDomainShmemDefFind(vmdef, dev->data.shmem)) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("matching shmem device was not found")); + return -1; + } + + virDomainShmemDefFree(virDomainShmemDefRemove(vmdef, idx)); + break; + case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: @@ -8059,7 +8091,6 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: - case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 72dd93bbe467..e70bf577139b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2242,6 +2242,141 @@ qemuDomainAttachHostDevice(virConnectPtr conn, return -1; } + +int +qemuDomainAttachShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr shmem) +{ + int ret = -1; + char *shmstr = NULL; + char *charAlias = NULL; + char *memAlias = NULL; + bool release_backing = false; + bool release_address = true; + bool supported = false; + virErrorPtr orig_err = NULL; + virJSONValuePtr props = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + + + switch (shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + supported = virQEMUCapsGet(priv->qemuCaps, + QEMU_CAPS_DEVICE_IVSHMEM_PLAIN); + break; + + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: + supported = virQEMUCapsGet(priv->qemuCaps, + QEMU_CAPS_DEVICE_IVSHMEM_DOORBELL); + break; + + default: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("live attach of shmem model '%s' is not supported"), + virDomainShmemModelTypeToString(shmem->model)); + return -1; + } + + if (!supported) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Device '%s' is not supported by this version of qemu"), + virDomainShmemModelTypeToString(shmem->model)); + return -1; + } + + if (qemuAssignDeviceShmemAlias(vm->def, shmem, -1) < 0) + return -1; + + if (qemuDomainPrepareShmemChardev(shmem) < 0) + return -1; + + if (VIR_REALLOC_N(vm->def->shmems, vm->def->nshmems + 1) < 0) + return -1; + + if ((shmem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE || + shmem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && + (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &shmem->info) < 0)) + return -1; + + if (!(shmstr = qemuBuildShmemDevStr(vm->def, shmem, priv->qemuCaps))) + goto cleanup; + + if (shmem->server.enabled) { + if (virAsprintf(&charAlias, "char%s", shmem->info.alias) < 0) + goto cleanup; + } else { + if (!(props = qemuBuildShmemBackendMemProps(shmem))) + goto cleanup; + + if (virAsprintf(&memAlias, "shmmem-%s", shmem->info.alias) < 0) + goto cleanup; + } + + if (virDomainShmemDefInsert(vm->def, shmem) < 0) + goto cleanup; + + qemuDomainObjEnterMonitor(driver, vm); + + if (shmem->server.enabled) { + if (qemuMonitorAttachCharDev(priv->mon, charAlias, + &shmem->server.chr) < 0) + goto audit; + } else { + if (qemuMonitorAddObject(priv->mon, "memory-backend-file", + memAlias, props) < 0) { + props = NULL; + goto audit; + } + props = NULL; + } + + release_backing = true; + + if (qemuMonitorAddDevice(priv->mon, shmstr) < 0) + goto audit; + + ret = 0; + release_address = false; + + audit: + virDomainAuditShmem(vm, shmem, "attach", ret == 0); + + orig_err = virSaveLastError(); + if (ret < 0 && release_backing) { + if (shmem->server.enabled) { + if (qemuMonitorDetachCharDev(priv->mon, charAlias) < 0) { + VIR_WARN("Unable to remove backing chardev '%s' after failed " + "qemuMonitorAddDevice", charAlias); + } + } else { + if (qemuMonitorDelObject(priv->mon, memAlias) < 0) { + VIR_WARN("Unable to remove backing memory '%s' after failed " + "qemuMonitorAddDevice", memAlias); + } + } + } + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; + + cleanup: + if (release_address) + qemuDomainReleaseDeviceAddress(vm, &shmem->info, NULL); + + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } + + VIR_FREE(memAlias); + VIR_FREE(charAlias); + VIR_FREE(shmstr); + + return ret; +} + + static int qemuDomainChangeNetBridge(virDomainObjPtr vm, virDomainNetDefPtr olddev, @@ -3486,6 +3621,61 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, } +static int +qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr shmem) +{ + int ret = -1; + ssize_t idx = -1; + char *charAlias = NULL; + char *memAlias = NULL; + virObjectEventPtr event = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + + + VIR_DEBUG("Removing shmem device %s from domain %p %s", + shmem->info.alias, vm, vm->def->name); + + if (shmem->server.enabled) { + if (virAsprintf(&charAlias, "char%s", shmem->info.alias) < 0) + goto cleanup; + } else { + if (virAsprintf(&memAlias, "shmmem-%s", shmem->info.alias) < 0) + goto cleanup; + } + + qemuDomainObjEnterMonitor(driver, vm); + + if (shmem->server.enabled) + ret = qemuMonitorDetachCharDev(priv->mon, charAlias); + else + ret = qemuMonitorDelObject(priv->mon, memAlias); + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + virDomainAuditShmem(vm, shmem, "detach", ret == 0); + + if (ret < 0) + goto cleanup; + + event = virDomainEventDeviceRemovedNewFromObj(vm, shmem->info.alias); + qemuDomainEventQueue(driver, event); + + if ((idx = virDomainShmemDefFind(vm->def, shmem)) >= 0) + virDomainShmemDefRemove(vm->def, idx); + qemuDomainReleaseDeviceAddress(vm, &shmem->info, NULL); + + ret = 0; + cleanup: + VIR_FREE(charAlias); + VIR_FREE(memAlias); + + return ret; +} + + int qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3517,6 +3707,10 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, ret = qemuDomainRemoveMemoryDevice(driver, vm, dev->data.memory); break; + case VIR_DOMAIN_DEVICE_SHMEM: + ret = qemuDomainRemoveShmemDevice(driver, vm, dev->data.shmem); + break; + case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: @@ -3530,7 +3724,6 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: - case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: case VIR_DOMAIN_DEVICE_IOMMU: @@ -4105,6 +4298,58 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, return qemuDomainDetachThisHostDevice(driver, vm, detach); } + +int +qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + int ret = -1; + virErrorPtr orig_err = NULL; + virDomainShmemDefPtr shmem = dev->data.shmem; + qemuDomainObjPrivatePtr priv = vm->privateData; + + + switch (shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: + break; + + default: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("live detach of shmem model '%s' is not supported"), + virDomainShmemModelTypeToString(shmem->model)); + return -1; + } + + qemuDomainMarkDeviceForRemoval(vm, &shmem->info); + qemuDomainObjEnterMonitor(driver, vm); + + ret = qemuMonitorDelDevice(priv->mon, shmem->info.alias); + + if (ret < 0) + orig_err = virSaveLastError(); + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; + + if (ret == 0) { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) { + qemuDomainReleaseDeviceAddress(vm, &shmem->info, NULL); + ret = qemuDomainRemoveDevice(driver, vm, dev); + } + } + qemuDomainResetDeviceRemoval(vm); + + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } + + return ret; +} + + int qemuDomainDetachNetDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index b048cf4688a4..12994e7cd468 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -50,6 +50,9 @@ int qemuDomainAttachHostDevice(virConnectPtr conn, virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev); +int qemuDomainAttachShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr shmem); int qemuDomainFindGraphicsIndex(virDomainDefPtr def, virDomainGraphicsDefPtr dev); int qemuDomainAttachMemory(virQEMUDriverPtr driver, @@ -86,6 +89,9 @@ int qemuDomainDetachNetDevice(virQEMUDriverPtr driver, int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev); +int qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev); int qemuDomainAttachLease(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainLeaseDefPtr lease); diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 6a9ed992f31d..dda3917a5034 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -74,6 +74,8 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt, virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_VIRTIO_SCSI); virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE); virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW); + virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_DEVICE_IVSHMEM_PLAIN); + virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_DEVICE_IVSHMEM_DOORBELL); if (event) virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT); @@ -119,6 +121,9 @@ testQemuHotplugAttach(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_CHR: ret = qemuDomainAttachChrDevice(&driver, vm, dev->data.chr); break; + case VIR_DOMAIN_DEVICE_SHMEM: + ret = qemuDomainAttachShmemDevice(&driver, vm, dev->data.shmem); + break; default: VIR_TEST_VERBOSE("device type '%s' cannot be attached\n", virDomainDeviceTypeToString(dev->type)); @@ -141,6 +146,9 @@ testQemuHotplugDetach(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_CHR: ret = qemuDomainDetachChrDevice(&driver, vm, dev->data.chr); break; + case VIR_DOMAIN_DEVICE_SHMEM: + ret = qemuDomainDetachShmemDevice(&driver, vm, dev); + break; default: VIR_TEST_VERBOSE("device type '%s' cannot be detached\n", virDomainDeviceTypeToString(dev->type)); @@ -560,6 +568,19 @@ mymain(void) "human-monitor-command", HMP("OK\\r\\n"), "device_add", QMP_OK); + DO_TEST_ATTACH("base-live", "ivshmem-plain", false, true, + "object-add", QMP_OK, + "device_add", QMP_OK); + DO_TEST_ATTACH("base-live", "ivshmem-doorbell", false, true, + "chardev-add", QMP_OK, + "device_add", QMP_OK); + DO_TEST_DETACH("base-live+ivshmem-plain", "ivshmem-doorbell-detach", false, true, + "device_del", QMP_OK, + "chardev-remove", QMP_OK); + DO_TEST_DETACH("base-live", "ivshmem-plain-detach", false, false, + "device_del", QMP_OK, + "object-del", QMP_OK); + qemuTestDriverFree(&driver); return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-doorbell-detach.xml b/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-doorbell-detach.xml new file mode 100644 index 000000000000..7c066964d745 --- /dev/null +++ b/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-doorbell-detach.xml @@ -0,0 +1,7 @@ +<shmem name='shmem1'> + <model type='ivshmem-doorbell'/> + <server path='/var/lib/libvirt/shmem-shmem1-sock'/> + <msi ioeventfd='on'/> + <alias name='shmem1'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> +</shmem> diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-doorbell.xml b/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-doorbell.xml new file mode 100644 index 000000000000..06cb0c978605 --- /dev/null +++ b/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-doorbell.xml @@ -0,0 +1,4 @@ +<shmem name='shmem1'> + <model type='ivshmem-doorbell'/> + <server/> +</shmem> diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain-detach.xml b/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain-detach.xml new file mode 100644 index 000000000000..338017aa2854 --- /dev/null +++ b/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain-detach.xml @@ -0,0 +1,6 @@ +<shmem name='shmem0' role='peer'> + <model type='ivshmem-plain'/> + <size unit='M'>4</size> + <alias name='shmem0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> +</shmem> diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain.xml b/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain.xml new file mode 100644 index 000000000000..6bd96ff16767 --- /dev/null +++ b/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain.xml @@ -0,0 +1,3 @@ +<shmem name='shmem0'> + <model type='ivshmem-plain'/> +</shmem> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell-detach.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell-detach.xml new file mode 120000 index 000000000000..021e5471d197 --- /dev/null +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell-detach.xml @@ -0,0 +1 @@ +qemuhotplug-base-live+ivshmem-plain.xml \ No newline at end of file diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell.xml new file mode 100644 index 000000000000..867192568168 --- /dev/null +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell.xml @@ -0,0 +1,65 @@ +<domain type='kvm' id='7'> + <name>hotplug</name> + <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid> + <memory unit='KiB'>4194304</memory> + <currentMemory unit='KiB'>4194304</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <controller type='usb' index='0'> + <alias name='usb'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <alias name='ide'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='scsi' index='0' model='virtio-scsi'> + <alias name='scsi0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'> + <alias name='pci'/> + </controller> + <controller type='virtio-serial' index='0'> + <alias name='virtio-serial0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'> + <alias name='input0'/> + </input> + <input type='keyboard' bus='ps2'> + <alias name='input1'/> + </input> + <memballoon model='none'> + <alias name='balloon0'/> + </memballoon> + <shmem name='shmem0' role='peer'> + <model type='ivshmem-plain'/> + <size unit='M'>4</size> + <alias name='shmem0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </shmem> + <shmem name='shmem1'> + <model type='ivshmem-doorbell'/> + <server path='/var/lib/libvirt/shmem-shmem1-sock'/> + <msi ioeventfd='on'/> + <alias name='shmem1'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </shmem> + </devices> + <seclabel type='none' model='none'/> +</domain> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-plain-detach.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-plain-detach.xml new file mode 120000 index 000000000000..48c1b1755a4e --- /dev/null +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-plain-detach.xml @@ -0,0 +1 @@ +qemuhotplug-base-live.xml \ No newline at end of file diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-plain.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-plain.xml new file mode 100644 index 000000000000..2873e7153e54 --- /dev/null +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-plain.xml @@ -0,0 +1,58 @@ +<domain type='kvm' id='7'> + <name>hotplug</name> + <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid> + <memory unit='KiB'>4194304</memory> + <currentMemory unit='KiB'>4194304</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/libexec/qemu-kvm</emulator> + <controller type='usb' index='0'> + <alias name='usb'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <alias name='ide'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='scsi' index='0' model='virtio-scsi'> + <alias name='scsi0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'> + <alias name='pci'/> + </controller> + <controller type='virtio-serial' index='0'> + <alias name='virtio-serial0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'> + <alias name='input0'/> + </input> + <input type='keyboard' bus='ps2'> + <alias name='input1'/> + </input> + <memballoon model='none'> + <alias name='balloon0'/> + </memballoon> + <shmem name='shmem0' role='peer'> + <model type='ivshmem-plain'/> + <size unit='M'>4</size> + <alias name='shmem0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </shmem> + </devices> + <seclabel type='none' model='none'/> +</domain> -- 2.10.0
On Thu, Sep 15, 2016 at 18:14:45 +0200, Martin Kletzander wrote:
This is needed in order to migrate with ivshmem role='peer' as that is not allowed to migrate.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 39 +++- src/qemu/qemu_hotplug.c | 247 ++++++++++++++++++++- src/qemu/qemu_hotplug.h | 6 + tests/qemuhotplugtest.c | 21 ++ .../qemuhotplug-ivshmem-doorbell-detach.xml | 7 + .../qemuhotplug-ivshmem-doorbell.xml | 4 + .../qemuhotplug-ivshmem-plain-detach.xml | 6 + .../qemuhotplug-ivshmem-plain.xml | 3 + ...muhotplug-base-live+ivshmem-doorbell-detach.xml | 1 + .../qemuhotplug-base-live+ivshmem-doorbell.xml | 65 ++++++ .../qemuhotplug-base-live+ivshmem-plain-detach.xml | 1 + .../qemuhotplug-base-live+ivshmem-plain.xml | 58 +++++ 12 files changed, 453 insertions(+), 5 deletions(-) create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-doorbell-detach.xml create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-doorbell.xml create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain-detach.xml create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain.xml create mode 120000 tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell-detach.xml create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell.xml create mode 120000 tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-plain-detach.xml create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-plain.xml
[...]
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 72dd93bbe467..e70bf577139b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2242,6 +2242,141 @@ qemuDomainAttachHostDevice(virConnectPtr conn, return -1; }
+ +int +qemuDomainAttachShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr shmem) +{ + int ret = -1; + char *shmstr = NULL; + char *charAlias = NULL; + char *memAlias = NULL; + bool release_backing = false; + bool release_address = true; + bool supported = false; + virErrorPtr orig_err = NULL; + virJSONValuePtr props = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; +
Too much whitespace.
+ + switch (shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + supported = virQEMUCapsGet(priv->qemuCaps, + QEMU_CAPS_DEVICE_IVSHMEM_PLAIN); + break; + + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: + supported = virQEMUCapsGet(priv->qemuCaps, + QEMU_CAPS_DEVICE_IVSHMEM_DOORBELL); + break; + + default:
We are trying to avoid 'default' cases as much as possible. Please use typecasted value in the switch and use all possible cases.
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("live attach of shmem model '%s' is not supported"), + virDomainShmemModelTypeToString(shmem->model)); + return -1; + } + + if (!supported) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Device '%s' is not supported by this version of qemu"), + virDomainShmemModelTypeToString(shmem->model)); + return -1; + } + + if (qemuAssignDeviceShmemAlias(vm->def, shmem, -1) < 0) + return -1; + + if (qemuDomainPrepareShmemChardev(shmem) < 0) + return -1; + + if (VIR_REALLOC_N(vm->def->shmems, vm->def->nshmems + 1) < 0) + return -1;
This isn't really necessary ...
+ + if ((shmem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE || + shmem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && + (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &shmem->info) < 0)) + return -1; + + if (!(shmstr = qemuBuildShmemDevStr(vm->def, shmem, priv->qemuCaps))) + goto cleanup; + + if (shmem->server.enabled) { + if (virAsprintf(&charAlias, "char%s", shmem->info.alias) < 0) + goto cleanup; + } else { + if (!(props = qemuBuildShmemBackendMemProps(shmem))) + goto cleanup; + + if (virAsprintf(&memAlias, "shmmem-%s", shmem->info.alias) < 0) + goto cleanup; + } + + if (virDomainShmemDefInsert(vm->def, shmem) < 0) + goto cleanup;
... since this uses VIR_APPEND_ELEMENT.
+ + qemuDomainObjEnterMonitor(driver, vm); + + if (shmem->server.enabled) { + if (qemuMonitorAttachCharDev(priv->mon, charAlias, + &shmem->server.chr) < 0) + goto audit; + } else { + if (qemuMonitorAddObject(priv->mon, "memory-backend-file", + memAlias, props) < 0) { + props = NULL; + goto audit; + } + props = NULL; + } + + release_backing = true; + + if (qemuMonitorAddDevice(priv->mon, shmstr) < 0) + goto audit; + + ret = 0; + release_address = false;
You didn't exit monitor here ...
+ + audit: + virDomainAuditShmem(vm, shmem, "attach", ret == 0);
Thus you don't own the mutex on @vm to access it.
+ orig_err = virSaveLastError(); + if (ret < 0 && release_backing) { + if (shmem->server.enabled) { + if (qemuMonitorDetachCharDev(priv->mon, charAlias) < 0) { + VIR_WARN("Unable to remove backing chardev '%s' after failed " + "qemuMonitorAddDevice", charAlias); + } + } else { + if (qemuMonitorDelObject(priv->mon, memAlias) < 0) { + VIR_WARN("Unable to remove backing memory '%s' after failed " + "qemuMonitorAddDevice", memAlias); + } + } + } + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; + + cleanup: + if (release_address) + qemuDomainReleaseDeviceAddress(vm, &shmem->info, NULL); + + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } + + VIR_FREE(memAlias); + VIR_FREE(charAlias); + VIR_FREE(shmstr); + + return ret; +} + + static int qemuDomainChangeNetBridge(virDomainObjPtr vm, virDomainNetDefPtr olddev, @@ -3486,6 +3621,61 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, }
+static int +qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr shmem) +{ + int ret = -1; + ssize_t idx = -1; + char *charAlias = NULL; + char *memAlias = NULL; + virObjectEventPtr event = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + +
Too much whitespace.
+ VIR_DEBUG("Removing shmem device %s from domain %p %s", + shmem->info.alias, vm, vm->def->name); + + if (shmem->server.enabled) { + if (virAsprintf(&charAlias, "char%s", shmem->info.alias) < 0) + goto cleanup; + } else { + if (virAsprintf(&memAlias, "shmmem-%s", shmem->info.alias) < 0) + goto cleanup; + } + + qemuDomainObjEnterMonitor(driver, vm); + + if (shmem->server.enabled) + ret = qemuMonitorDetachCharDev(priv->mon, charAlias); + else + ret = qemuMonitorDelObject(priv->mon, memAlias); + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + virDomainAuditShmem(vm, shmem, "detach", ret == 0); + + if (ret < 0) + goto cleanup; + + event = virDomainEventDeviceRemovedNewFromObj(vm, shmem->info.alias); + qemuDomainEventQueue(driver, event); + + if ((idx = virDomainShmemDefFind(vm->def, shmem)) >= 0) + virDomainShmemDefRemove(vm->def, idx); + qemuDomainReleaseDeviceAddress(vm, &shmem->info, NULL); + + ret = 0; + cleanup: + VIR_FREE(charAlias); + VIR_FREE(memAlias); + + return ret; +} + + int qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3517,6 +3707,10 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, ret = qemuDomainRemoveMemoryDevice(driver, vm, dev->data.memory); break;
+ case VIR_DOMAIN_DEVICE_SHMEM: + ret = qemuDomainRemoveShmemDevice(driver, vm, dev->data.shmem); + break; + case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: @@ -3530,7 +3724,6 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: - case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: case VIR_DOMAIN_DEVICE_IOMMU: @@ -4105,6 +4298,58 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, return qemuDomainDetachThisHostDevice(driver, vm, detach); }
+ +int +qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + int ret = -1; + virErrorPtr orig_err = NULL; + virDomainShmemDefPtr shmem = dev->data.shmem; + qemuDomainObjPrivatePtr priv = vm->privateData; + +
Too much whitespace.
+ switch (shmem->model) {
Non-typecasted switch.
+ case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: + break; + + default:
Thankfully we didn't support hotplug for the old device.
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("live detach of shmem model '%s' is not supported"), + virDomainShmemModelTypeToString(shmem->model)); + return -1; + } + + qemuDomainMarkDeviceForRemoval(vm, &shmem->info); + qemuDomainObjEnterMonitor(driver, vm); + + ret = qemuMonitorDelDevice(priv->mon, shmem->info.alias); + + if (ret < 0) + orig_err = virSaveLastError();
This is not necessary, only thing that overwrites the error is the exit from monitor which is a more critical error.
+ + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; + + if (ret == 0) { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) { + qemuDomainReleaseDeviceAddress(vm, &shmem->info, NULL); + ret = qemuDomainRemoveDevice(driver, vm, dev); + } + } + qemuDomainResetDeviceRemoval(vm); + + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } + + return ret; +} + + int qemuDomainDetachNetDevice(virQEMUDriverPtr driver, virDomainObjPtr vm,
On Thu, Sep 15, 2016 at 18:14:25 +0200, Martin Kletzander wrote:
Let's see if the subject works if one is in need of reviews =)
Yet another qemu device, right? We even have an existing device for that, right? That should be pretty straight-forward and easy, right? Well, let's see... I, at least, tried splitting the patches for you to be as easy to review as possible.
Just in case you're trying out the hot-(un)plug on an upstream QEMU, make sure you do it on i440fx machine, not on q35 one, otherwise it will not work nicely (or rather at all).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1347049
Few notes to add to the craziness: 1) According to the qemu documentation the old 'ivshmem' device is deprecated and should not be used and should be sensibly replaced with the new devices relevant to the configuration. This is not happening in this series so the above mentioned bugzilla is actually not resolved by this series. 2) If we are going to allow migration for the few certain configs that should work you'll need to add migration flags to support this. The need comes from the fact that previously we did not parse the model and thus you will either need to convert previously working configs to the old device or just to forbid the new definitions. 3) Adding support for the 'role' stuff for the legacy code will add more fun into making point 2 properly. Peter
On Fri, Sep 16, 2016 at 02:22:19PM +0200, Peter Krempa wrote:
On Thu, Sep 15, 2016 at 18:14:25 +0200, Martin Kletzander wrote:
Let's see if the subject works if one is in need of reviews =)
Yet another qemu device, right? We even have an existing device for that, right? That should be pretty straight-forward and easy, right? Well, let's see... I, at least, tried splitting the patches for you to be as easy to review as possible.
Just in case you're trying out the hot-(un)plug on an upstream QEMU, make sure you do it on i440fx machine, not on q35 one, otherwise it will not work nicely (or rather at all).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1347049
Few notes to add to the craziness:
1) According to the qemu documentation the old 'ivshmem' device is deprecated and should not be used and should be sensibly replaced with the new devices relevant to the configuration. This is not happening in this series so the above mentioned bugzilla is actually not resolved by this series.
It should be used, but it's not the same thing. The only thing ivshmem and ivshmem-(plain|doorbell) have in common is the Guest ABI.
2) If we are going to allow migration for the few certain configs that should work you'll need to add migration flags to support this. The need comes from the fact that previously we did not parse the model and thus you will either need to convert previously working configs to the old device or just to forbid the new definitions.
Sure, I'll add the flag and just forbid it.
3) Adding support for the 'role' stuff for the legacy code will add more fun into making point 2 properly.
Old will just not be able to migrate, ever. Nobody wants to support that.
Peter
On Fri, Sep 16, 2016 at 15:17:34 +0200, Martin Kletzander wrote:
On Fri, Sep 16, 2016 at 02:22:19PM +0200, Peter Krempa wrote:
On Thu, Sep 15, 2016 at 18:14:25 +0200, Martin Kletzander wrote:
Let's see if the subject works if one is in need of reviews =)
Yet another qemu device, right? We even have an existing device for that, right? That should be pretty straight-forward and easy, right? Well, let's see... I, at least, tried splitting the patches for you to be as easy to review as possible.
Just in case you're trying out the hot-(un)plug on an upstream QEMU, make sure you do it on i440fx machine, not on q35 one, otherwise it will not work nicely (or rather at all).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1347049
Few notes to add to the craziness:
1) According to the qemu documentation the old 'ivshmem' device is deprecated and should not be used and should be sensibly replaced with the new devices relevant to the configuration. This is not happening in this series so the above mentioned bugzilla is actually not resolved by this series.
It should be used, but it's not the same thing. The only thing ivshmem and ivshmem-(plain|doorbell) have in common is the Guest ABI.
Okay than that fact is not clearly stated in this series and nor in the qemu documentation. Please add docs that explain which device is necessary in which case and what are the limits.
2) If we are going to allow migration for the few certain configs that should work you'll need to add migration flags to support this. The need comes from the fact that previously we did not parse the model and thus you will either need to convert previously working configs to the old device or just to forbid the new definitions.
Sure, I'll add the flag and just forbid it.
3) Adding support for the 'role' stuff for the legacy code will add more fun into making point 2 properly.
Old will just not be able to migrate, ever. Nobody wants to support that.
If migration with the old device won't ever work shouldn't we just use the same config and internally deal with the differences rather than exposing the different device types? Peter
On Fri, Sep 16, 2016 at 04:20:55PM +0200, Peter Krempa wrote:
On Fri, Sep 16, 2016 at 15:17:34 +0200, Martin Kletzander wrote:
On Fri, Sep 16, 2016 at 02:22:19PM +0200, Peter Krempa wrote:
On Thu, Sep 15, 2016 at 18:14:25 +0200, Martin Kletzander wrote:
Let's see if the subject works if one is in need of reviews =)
Yet another qemu device, right? We even have an existing device for that, right? That should be pretty straight-forward and easy, right? Well, let's see... I, at least, tried splitting the patches for you to be as easy to review as possible.
Just in case you're trying out the hot-(un)plug on an upstream QEMU, make sure you do it on i440fx machine, not on q35 one, otherwise it will not work nicely (or rather at all).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1347049
Few notes to add to the craziness:
1) According to the qemu documentation the old 'ivshmem' device is deprecated and should not be used and should be sensibly replaced with the new devices relevant to the configuration. This is not happening in this series so the above mentioned bugzilla is actually not resolved by this series.
It should be used, but it's not the same thing. The only thing ivshmem and ivshmem-(plain|doorbell) have in common is the Guest ABI.
Okay than that fact is not clearly stated in this series and nor in the qemu documentation. Please add docs that explain which device is necessary in which case and what are the limits.
2) If we are going to allow migration for the few certain configs that should work you'll need to add migration flags to support this. The need comes from the fact that previously we did not parse the model and thus you will either need to convert previously working configs to the old device or just to forbid the new definitions.
Sure, I'll add the flag and just forbid it.
3) Adding support for the 'role' stuff for the legacy code will add more fun into making point 2 properly.
Old will just not be able to migrate, ever. Nobody wants to support that.
^^ this is bs, I don't know where I got that from, my mistake, sorry.
If migration with the old device won't ever work shouldn't we just use the same config and internally deal with the differences rather than exposing the different device types?
I forgot the old one is actually able to migrate currently. I wanted to have the models hidden and just keep exposing the same device, but I just remembered that the reason for me not to do that was due to the old device being migrated. But one idea comes to my mind. We could add a migration cookie flag that will be set if source is using the new ivshmem-*. Then we could effectively forbid migrating the old device. Newer libvirt on the source would do that automatically, for old libvirt on the source it would be checked for on destination. That could actually work. I still haven't thought of all the scenarios, I need to get some caffeine into my veins for that, but that could actually solve a bunch of problems. It would also mean getting back like three non-posted versions of this series :'( Martin
Peter
participants (2)
-
Martin Kletzander -
Peter Krempa