[libvirt] [PATCH 0/8] IVSHMEM -- third time's the charm
In this version we bring back the model, but disable migration for now. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1347049 Martin Kletzander (8): conf: Fix virDomainShmemDefFind qemu: Disable migration with ivshmem conf, qemu: Add support for shmem model conf, qemu: Add newer shmem models qemu: Add capabilities for ivshmem-{plain,doorbell} qemu: Save various defaults for shmem qemu: Support newer ivshmem device variants qemu: Add support for hot/cold-(un)plug of shmem devices docs/schemas/domaincommon.rng | 11 + src/conf/domain_conf.c | 50 +++-- src/conf/domain_conf.h | 10 + src/libvirt_private.syms | 2 + src/qemu/qemu_capabilities.c | 4 + src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_command.c | 100 ++++++++- src/qemu/qemu_command.h | 10 + src/qemu/qemu_domain.c | 49 +++- src/qemu/qemu_driver.c | 39 +++- src/qemu/qemu_hotplug.c | 247 ++++++++++++++++++++- src/qemu/qemu_hotplug.h | 6 + src/qemu/qemu_migration.c | 6 + .../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 | 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 +++++ .../qemuxml2argv-shmem-plain-doorbell.args | 43 ++++ ...m.xml => qemuxml2argv-shmem-plain-doorbell.xml} | 11 +- tests/qemuxml2argvdata/qemuxml2argv-shmem.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-shmem.xml | 2 + tests/qemuxml2argvtest.c | 3 + tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml | 9 + 33 files changed, 760 insertions(+), 22 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} (82%) -- 2.10.0
Due to the switch of parameters in a call to virDomainShmemDefEquals() no device was found when looking for device with all the information except address. Also fix the indentation. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a06da46fabe4..dbf6eca57153 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14858,8 +14858,8 @@ virDomainShmemDefFind(virDomainDefPtr def, size_t i; for (i = 0; i < def->nshmems; i++) { - if (virDomainShmemDefEquals(def->shmems[i], shmem)) - break; + if (virDomainShmemDefEquals(shmem, def->shmems[i])) + break; } if (i < def->nshmems) -- 2.10.0
On 09/27/2016 08:24 AM, Martin Kletzander wrote:
Due to the switch of parameters in a call to virDomainShmemDefEquals() no device was found when looking for device with all the information except address. Also fix the indentation.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK - while you're here... instead of 'break;', just return i; and don't forget to remove the if (i < def->nshmems) return i; You can make it a separate patch and assume the ACK... John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a06da46fabe4..dbf6eca57153 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14858,8 +14858,8 @@ virDomainShmemDefFind(virDomainDefPtr def, size_t i;
for (i = 0; i < def->nshmems; i++) { - if (virDomainShmemDefEquals(def->shmems[i], shmem)) - break; + if (virDomainShmemDefEquals(shmem, def->shmems[i])) + break; }
if (i < def->nshmems)
It was never safe anyway and as such shouldn't have been enabled in the first place. Future patches will allow hot-(un)pluging of some ivshmem devices as a workaround. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_migration.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index e2ca3303efd3..0d7fec8360f3 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2346,6 +2346,12 @@ qemuMigrationIsAllowed(virQEMUDriverPtr driver, return false; } } + + if (vm->def->nshmems) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Migration with shmem device is not supported")); + return false; + } } return true; -- 2.10.0
On 09/27/2016 08:24 AM, Martin Kletzander wrote:
It was never safe anyway and as such shouldn't have been enabled in the first place. Future patches will allow hot-(un)pluging of some ivshmem devices as a workaround.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_migration.c | 6 ++++++ 1 file changed, 6 insertions(+)
Seems like a reasonable thing to disallow... Never quite sure what the "norm" is - start message w/ capital letter or not... I think others in the code go with lowercase... Your call. ACK, John
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index e2ca3303efd3..0d7fec8360f3 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2346,6 +2346,12 @@ qemuMigrationIsAllowed(virQEMUDriverPtr driver, return false; } } + + if (vm->def->nshmems) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Migration with shmem device is not supported")); + return false; + } }
return true;
On Fri, Oct 07, 2016 at 09:52:12AM -0400, John Ferlan wrote:
On 09/27/2016 08:24 AM, Martin Kletzander wrote:
It was never safe anyway and as such shouldn't have been enabled in the first place. Future patches will allow hot-(un)pluging of some ivshmem devices as a workaround.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_migration.c | 6 ++++++ 1 file changed, 6 insertions(+)
Seems like a reasonable thing to disallow...
Never quite sure what the "norm" is - start message w/ capital letter or not... I think others in the code go with lowercase... Your call.
All of them in qemuMigrationIsAllowed() start with lowercase, so I changed that, but I think rest of the code is uppercase and since it's a sentence after a colon, it should be so, but that's not what this series is about, so... =D
ACK,
John
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index e2ca3303efd3..0d7fec8360f3 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2346,6 +2346,12 @@ qemuMigrationIsAllowed(virQEMUDriverPtr driver, return false; } } + + if (vm->def->nshmems) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Migration with shmem device is not supported")); + return false; + } }
return true;
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 | 44 +++++++++++++++++------ src/conf/domain_conf.h | 8 +++++ src/libvirt_private.syms | 2 ++ src/qemu/qemu_command.c | 11 +++++- tests/qemuxml2argvdata/qemuxml2argv-shmem.xml | 2 ++ tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml | 8 +++++ 7 files changed, 72 insertions(+), 12 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 95c7882cb73d..43c8b6696ac7 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3587,6 +3587,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 dbf6eca57153..ed9a86af0f4e 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); @@ -12244,6 +12247,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")); @@ -14824,6 +14841,9 @@ virDomainShmemDefEquals(virDomainShmemDefPtr src, if (src->size != dst->size) return false; + if (src->model != dst->model) + return false; + if (src->server.enabled != dst->server.enabled) return false; @@ -18811,6 +18831,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 " @@ -21888,20 +21917,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 d2065cf3edea..4e74c5611b2e 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; @@ -3074,6 +3081,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 68d0ea973f27..a235a344d5e2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -470,6 +470,8 @@ virDomainShmemDefFind; virDomainShmemDefFree; virDomainShmemDefInsert; virDomainShmemDefRemove; +virDomainShmemModelTypeFromString; +virDomainShmemModelTypeToString; virDomainShutdownReasonTypeFromString; virDomainShutdownReasonTypeToString; virDomainShutoffReasonTypeFromString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f24a98bc76fd..4e4a1c9cafe9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8512,7 +8512,16 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager, return -1; } - if (!(devstr = qemuBuildShmemDevLegacyStr(def, shmem, qemuCaps))) + switch ((virDomainShmemModel)shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + devstr = qemuBuildShmemDevLegacyStr(def, shmem, qemuCaps); + break; + + case VIR_DOMAIN_SHMEM_MODEL_LAST: + break; + } + + if (!devstr) return -1; virCommandAddArgList(cmd, "-device", devstr, NULL); VIR_FREE(devstr); 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 09/27/2016 08:24 AM, 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 | 44 +++++++++++++++++------ src/conf/domain_conf.h | 8 +++++ src/libvirt_private.syms | 2 ++ src/qemu/qemu_command.c | 11 +++++- tests/qemuxml2argvdata/qemuxml2argv-shmem.xml | 2 ++ tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml | 8 +++++ 7 files changed, 72 insertions(+), 12 deletions(-)
docs/formatdomain.html.in ?? Just so I'm sure I understand ;-)... This is the existing 'model type' for 'shmem' being implemented as the "default" (e.g. 0 entry) because we know at some short time in the future we're going to be adding a new type (or two). Since the <model type='ivshmem'> is now going to be the default on output, we should explain in some way... and encourage choosing something else because "at some point in the future" ;-) we'll deprecate this one (whenever that dire time exists who knows). Making sure #2 - we don't have to care about save files, true? Since the default will be to now to ShmemDefFormat the <model> - a save file read on an older libvirt will have a failure, but that'd be true for any XML change I suppose. I understand it does matter that the model is printed just in case someone decided that's the model they wanted... If we really wanted overkill, some sort of bool could be created to keep track of whether model was read or not and then to not print out if not read in... So I don't forget, once you get to implementing 'plain' and 'doorbell', then perhaps some details from the v2 reply to 6/7 would be helpful to the less than informed user trying to figure out which to use ;-) ACK - in principle your call on the ShmemDefFormat bool or not... John
On Fri, Oct 07, 2016 at 10:53:48AM -0400, John Ferlan wrote:
On 09/27/2016 08:24 AM, 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 | 44 +++++++++++++++++------ src/conf/domain_conf.h | 8 +++++ src/libvirt_private.syms | 2 ++ src/qemu/qemu_command.c | 11 +++++- tests/qemuxml2argvdata/qemuxml2argv-shmem.xml | 2 ++ tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml | 8 +++++ 7 files changed, 72 insertions(+), 12 deletions(-)
docs/formatdomain.html.in ??
Just so I'm sure I understand ;-)... This is the existing 'model type' for 'shmem' being implemented as the "default" (e.g. 0 entry) because we know at some short time in the future we're going to be adding a new type (or two).
Since the <model type='ivshmem'> is now going to be the default on output, we should explain in some way... and encourage choosing something else because "at some point in the future" ;-) we'll deprecate this one (whenever that dire time exists who knows).
Making sure #2 - we don't have to care about save files, true? Since the default will be to now to ShmemDefFormat the <model> - a save file read on an older libvirt will have a failure, but that'd be true for any XML change I suppose.
IIRC ivshmem is non-migratable, so its impossible to have any existing save files. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On Fri, Oct 07, 2016 at 03:55:44PM +0100, Daniel P. Berrange wrote:
On Fri, Oct 07, 2016 at 10:53:48AM -0400, John Ferlan wrote:
On 09/27/2016 08:24 AM, 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 | 44 +++++++++++++++++------ src/conf/domain_conf.h | 8 +++++ src/libvirt_private.syms | 2 ++ src/qemu/qemu_command.c | 11 +++++- tests/qemuxml2argvdata/qemuxml2argv-shmem.xml | 2 ++ tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml | 8 +++++ 7 files changed, 72 insertions(+), 12 deletions(-)
docs/formatdomain.html.in ??
Just so I'm sure I understand ;-)... This is the existing 'model type' for 'shmem' being implemented as the "default" (e.g. 0 entry) because we know at some short time in the future we're going to be adding a new type (or two).
This is the 'ivshmem' being implemented as default if you don't pick any due to the fact that there are some older domains that already have <shmem/> without <model/> and we're sure they are using 'ivshmem'. So we don't want to change that. We could add a MODEL_DEFAULT and MODEL_IVSHMEM, but we would have to set it to _IVSHMEM in the PostParse anyway because _DEFAULT doesn't make sense here (in contrast to other places/options). That is because libvirt *has* to choose the model, there is no way to let the hypervisor (QEMU) choose.
Since the <model type='ivshmem'> is now going to be the default on output, we should explain in some way... and encourage choosing something else because "at some point in the future" ;-) we'll deprecate this one (whenever that dire time exists who knows).
Making sure #2 - we don't have to care about save files, true? Since the default will be to now to ShmemDefFormat the <model> - a save file read on an older libvirt will have a failure, but that'd be true for any XML change I suppose.
It won't, it would just *not* be parsed. We're not iterating over all children elements, we're just picking the ones we want. Have you ever edited the XML and lost the stuff you added because of a typo? Well that's exactly why. And also why we added RNG schema validation, I believe.
IIRC ivshmem is non-migratable, so its impossible to have any existing save files.
It's not that black-n-white, there are some grey areas. But we disabled the migration in previous commit due to the fact that it was never safe or that it would work for sure. Plus it doesn't make much sense, so IMNSHO we don't have to have a problem wrt migration.
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
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 43c8b6696ac7..5d591ccc6bdd 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3591,6 +3591,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 ed9a86af0f4e..a94266603811 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") static virClassPtr virDomainObjClass; static virClassPtr virDomainXMLOptionClass; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4e74c5611b2e..9f38cea831e6 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 4e4a1c9cafe9..aa8cff80e8b1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8517,6 +8517,13 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager, devstr = qemuBuildShmemDevLegacyStr(def, shmem, qemuCaps); 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; + case VIR_DOMAIN_SHMEM_MODEL_LAST: break; } -- 2.10.0
On 09/27/2016 08:24 AM, Martin Kletzander wrote:
The old ivshmem is deprecated in QEMU, so let's use the better ivshmem-{plain,doorbell} variants instead.
Perhaps explained a bit differently (my understanding ;-))... Older versions of qemu had one type (ivshmem - the current default), but newer versions of qemu will force selection of a specific type using "plain" or "doorbell" as the model type depending on which "features" are desired (??) [fill in the details] I guess the nagging question is - what happens if someone "chooses" to have <model type='ivshmem'> in this newer world? Does the deprecation from qemu kick in and the domain cannot start? We should note that somehow/somewhere.
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(-)
docs/formatdomain.html.in needs to change here too In particular what constitutes each setting - that is are there XML elements that would or wouldn't make sense for the type of shmem device (I'm thinking of Daniel's table here). BTW: When using your "Since" tags, it'll need to be clear which version of qemu no longer supports "type='ivshmem'" and forces someone to choose -plain or -doorbell. ACK w/ the doc change John
On Fri, Oct 07, 2016 at 11:09:54AM -0400, John Ferlan wrote:
On 09/27/2016 08:24 AM, Martin Kletzander wrote:
The old ivshmem is deprecated in QEMU, so let's use the better ivshmem-{plain,doorbell} variants instead.
Perhaps explained a bit differently (my understanding ;-))... Older versions of qemu had one type (ivshmem - the current default), but newer versions of qemu will force selection of a specific type using "plain" or "doorbell" as the model type depending on which "features" are desired (??) [fill in the details]
Old ivshmem could've been optionally used with or without "server" option (it would connect to a UNIX socket and talk to the server and so on. That got broken up to two separate new devices. One without server (-plain) and one with the server (-doorbell). As mentioned in one of the previous series, I could've add model named something like 'ivshmem-newer' and under the covers choose -plain or -doorbell, but 1) it would be really stupid and 2) the differentiation could change in the future and being specific is better than being bitten in the butt later on.
I guess the nagging question is - what happens if someone "chooses" to have <model type='ivshmem'> in this newer world? Does the deprecation from qemu kick in and the domain cannot start? We should note that somehow/somewhere.
They get a warning in the log saying "ivshmem is deprecated, please use ivshmem-plain or ivshmem-doorbell instead". And if QEMU stops supporting that, we'll know about it thanks to the capabilities and tell the user that the model is not available (like for _almost_ all other devices).
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(-)
docs/formatdomain.html.in needs to change here too
In particular what constitutes each setting - that is are there XML elements that would or wouldn't make sense for the type of shmem device (I'm thinking of Daniel's table here).
BTW: When using your "Since" tags, it'll need to be clear which version of qemu no longer supports "type='ivshmem'" and forces someone to choose -plain or -doorbell.
I'd rather remove the Since (at least for QEMU, maybe leave one for libvirt), the version doesn't mean poopsicle. I can see the nitpicky people saying: "look, I have this QEMU and it says it shouldn't be supported and it is, isn't this a bug?" because some distribution chose to backport one patch. I don't think the versions make sense, everyone can lookup when it was introduced. I believe it's not that hard.
ACK w/ the doc change
John
On 09/27/2016 08:24 AM, 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(-)
Doh... As I'm reading patch 7, I realized... This doesn't alter qemuxml2xmltest.c and it needs to as well as of course creating XML for each type (which can be stolen from 7/8) John
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 4d859c4dacac..b1d56cebd419 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", ); @@ -1587,6 +1589,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 ba0ef4859d42..f2c356b0cef3 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 98f37622bff0..cb6a55a9ec0c 100644 --- a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml @@ -197,6 +197,8 @@ <flag name='smm'/> <flag name='virtio-pci-disable-legacy'/> <flag name='query-hotpluggable-cpus'/> + <flag name='ivshmem-plain'/> + <flag name='ivshmem-doorbell'/> <version>2007000</version> <kvmVersion>0</kvmVersion> <package> (v2.7.0)</package> -- 2.10.0
On 09/27/2016 08:24 AM, 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(+)
re-ACK (this is ACK'd previously somewhere IIRC) John The question becomes who gets to push first and who gets to merge their capabilities changes ;-)
On Fri, Oct 07, 2016 at 11:11:53AM -0400, John Ferlan wrote:
On 09/27/2016 08:24 AM, 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(+)
re-ACK (this is ACK'd previously somewhere IIRC)
Well, yeah, this is just being reposted because pushing it doesn't make sense until it's used (we would end up with the flag and had to support it even if we didn't use it).
John
The question becomes who gets to push first and who gets to merge their capabilities changes ;-)
This is *always* the case. Of course.
We're keeping some things at default and that's not something we want to do intentionaly. Let's save some sensible defaults upfront then having problems later. The 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 | 49 ++++++++++++++++++++++- tests/qemuxml2argvdata/qemuxml2argv-shmem.args | 2 +- tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml | 1 + 3 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9b1a32ec3897..83b1f98d9a43 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2551,12 +2551,55 @@ qemuDomainChrDefDropDefaultPath(virDomainChrDefPtr chr, STRPREFIX(chr->source.data.nix.path, cfg->channelTargetDir)) { VIR_FREE(chr->source.data.nix.path); } - virObjectUnref(cfg); } static int +qemuDomainShmemDefPostParse(virDomainShmemDefPtr shm) +{ + if (!shm->size) + shm->size = 4 << 20; + + /* Nothing more to check/change for IVSHMEM */ + if (shm->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM) + return 0; + + if (!shm->server.enabled) { + if (shm->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("shmem model '%s' is supported " + "only with server option enabled"), + virDomainShmemModelTypeToString(shm->model)); + return -1; + } + + if (shm->msi.enabled) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("shmem model '%s' doesn't support " + "msi"), + virDomainShmemModelTypeToString(shm->model)); + } + } else { + if (shm->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("shmem model '%s' is supported " + "only with server option disabled"), + virDomainShmemModelTypeToString(shm->model)); + return -1; + } + + shm->size = 0; + shm->msi.enabled = true; + if (!shm->msi.ioeventfd) + shm->msi.ioeventfd = VIR_TRISTATE_SWITCH_ON; + } + + return 0; +} + + +static int qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, const virDomainDef *def, virCapsPtr caps, @@ -2760,6 +2803,10 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, } } + if (dev->type == VIR_DOMAIN_DEVICE_SHMEM && + qemuDomainShmemDefPostParse(dev->data.shmem) < 0) + goto cleanup; + ret = 0; cleanup: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-shmem.args b/tests/qemuxml2argvdata/qemuxml2argv-shmem.args index 99fac119b04c..bdf660a3c435 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,id=shmem0,shm=shmem0,bus=pci.0,addr=0x3 \ +-device ivshmem,id=shmem0,size=4m,shm=shmem0,bus=pci.0,addr=0x3 \ -device ivshmem,id=shmem1,size=128m,shm=shmem1,bus=pci.0,addr=0x5 \ -device ivshmem,id=shmem2,size=256m,shm=shmem2,bus=pci.0,addr=0x4 \ -device ivshmem,id=shmem3,size=512m,chardev=charshmem3,bus=pci.0,addr=0x6 \ diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml index 5602913648bc..04b463a27892 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml @@ -23,6 +23,7 @@ <memballoon model='none'/> <shmem name='shmem0'> <model type='ivshmem'/> + <size unit='M'>4</size> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> </shmem> <shmem name='shmem1'> -- 2.10.0
On 09/27/2016 08:24 AM, Martin Kletzander wrote:
We're keeping some things at default and that's not something we want to do intentionaly. Let's save some sensible defaults upfront then having
intentionally s/then having/in order to avoid having/
problems later. The 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 | 49 ++++++++++++++++++++++- tests/qemuxml2argvdata/qemuxml2argv-shmem.args | 2 +- tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml | 1 + 3 files changed, 50 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9b1a32ec3897..83b1f98d9a43 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2551,12 +2551,55 @@ qemuDomainChrDefDropDefaultPath(virDomainChrDefPtr chr, STRPREFIX(chr->source.data.nix.path, cfg->channelTargetDir)) { VIR_FREE(chr->source.data.nix.path); } -
Rogue editor change or OCD?
virObjectUnref(cfg); }
Although I'm sure the qemu commit referenced explains the defaults, maybe a bit of details here would help... To paraphrase from my quick read... The 'size' is only relevant for IVSHMEM, it's not used for plain and doorbell. The usage of plain or doorbell is based upon whether someone chooses <server> or <msi>. One cannot choose both in this new world. Selection of one or the other in the XML infers the type of shmem device.
static int +qemuDomainShmemDefPostParse(virDomainShmemDefPtr shm) +{ + if (!shm->size) + shm->size = 4 << 20;
I guess since we're not saving/migrating blindly setting won't cause strange failures for virDomainShmemDefFind/virDomainShmemDefEquals, but will it cause a problem for virDomainShmemDefCheckABIStability? There's a call to the ABI code from qemuDomainRevertToSnapshot which has comments for the transitions that would fall into this code being a migration, but without trying to page *that* back into my short term memory - well I figured I'd just note it and see what the consensus was! This also seem to have an effect on the default for -plain, but gets reset for -doorbell. [1] I think it'd be 'cleaner' if each model was part of a switch statement rather than the opposite world here where "server" assumes "-plain" and "msi" assumes "-doorbell". In the long run it'll be cleaner to read the code I think if we know what each has for a default...
+ + /* Nothing more to check/change for IVSHMEM */ + if (shm->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM) + return 0; + + if (!shm->server.enabled) { + if (shm->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("shmem model '%s' is supported " + "only with server option enabled"), + virDomainShmemModelTypeToString(shm->model)); + return -1; + } + + if (shm->msi.enabled) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("shmem model '%s' doesn't support " + "msi"), + virDomainShmemModelTypeToString(shm->model)); + } + } else { + if (shm->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("shmem model '%s' is supported " + "only with server option disabled"), + virDomainShmemModelTypeToString(shm->model)); + return -1; + } + + shm->size = 0;
[1] Is setting this to 0 the "correct" thing to do or should it be an error if a "-doorbell" shmem device uses the 'size' field? Blindly setting it to zero just doesn't seem right.
From the qemu checkin note...
Changes from ivshmem to ivshmem-plain: ... * Properties "shm" and "size" are gone. Use property "memdev" instead. ... Changes from ivshmem to ivshmem-doorbell: ... * Property "size" is gone. The new device can only map all the shared memory received from the server. ... There's an implication in the following patch too... I think it'd be good to post a v2 of this patch at least before an ACK. John
+ shm->msi.enabled = true; + if (!shm->msi.ioeventfd) + shm->msi.ioeventfd = VIR_TRISTATE_SWITCH_ON; + } + + return 0; +} + + +static int qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, const virDomainDef *def, virCapsPtr caps, @@ -2760,6 +2803,10 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, } }
+ if (dev->type == VIR_DOMAIN_DEVICE_SHMEM && + qemuDomainShmemDefPostParse(dev->data.shmem) < 0) + goto cleanup; + ret = 0;
cleanup: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-shmem.args b/tests/qemuxml2argvdata/qemuxml2argv-shmem.args index 99fac119b04c..bdf660a3c435 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,id=shmem0,shm=shmem0,bus=pci.0,addr=0x3 \ +-device ivshmem,id=shmem0,size=4m,shm=shmem0,bus=pci.0,addr=0x3 \ -device ivshmem,id=shmem1,size=128m,shm=shmem1,bus=pci.0,addr=0x5 \ -device ivshmem,id=shmem2,size=256m,shm=shmem2,bus=pci.0,addr=0x4 \ -device ivshmem,id=shmem3,size=512m,chardev=charshmem3,bus=pci.0,addr=0x6 \ diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml index 5602913648bc..04b463a27892 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem.xml @@ -23,6 +23,7 @@ <memballoon model='none'/> <shmem name='shmem0'> <model type='ivshmem'/> + <size unit='M'>4</size> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> </shmem> <shmem name='shmem1'>
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 | 88 +++++++++++++++++++++- src/qemu/qemu_command.h | 10 +++ .../qemuxml2argv-shmem-plain-doorbell.args | 43 +++++++++++ .../qemuxml2argv-shmem-plain-doorbell.xml | 63 ++++++++++++++++ tests/qemuxml2argvtest.c | 3 + 5 files changed, 204 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index aa8cff80e8b1..29f7130ef911 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8456,6 +8456,39 @@ qemuBuildShmemDevLegacyStr(virDomainDefPtr def, return NULL; } +char * +qemuBuildShmemDevStr(virDomainDefPtr def, + virDomainShmemDefPtr shmem, + virQEMUCapsPtr qemuCaps) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAsprintf(&buf, "%s", virDomainShmemModelTypeToString(shmem->model)); + virBufferAsprintf(&buf, ",id=%s", shmem->info.alias); + + if (shmem->server.enabled) + virBufferAsprintf(&buf, ",chardev=char%s", shmem->info.alias); + else + virBufferAsprintf(&buf, ",memdev=shmmem-%s", 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)); + } + + 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, @@ -8476,6 +8509,50 @@ 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, + "s:mem-path", mem_path, + "U:size", shmem->size, + 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, @@ -8518,10 +8595,15 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager, break; case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + if (!(devstr = qemuBuildShmemBackendMemStr(shmem))) + return -1; + + virCommandAddArgList(cmd, "-object", devstr, NULL); + VIR_FREE(devstr); + + /* fall-through */ case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("%s device is not supported with this QEMU binary"), - virDomainShmemModelTypeToString(shmem->model)); + devstr = qemuBuildShmemDevStr(def, shmem, qemuCaps); break; case VIR_DOMAIN_SHMEM_MODEL_LAST: 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..7abc7f8c4be5 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args @@ -0,0 +1,43 @@ +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 \ +-object memory-backend-file,id=shmmem-shmem0,mem-path=/dev/shm/shmem0,\ +size=4194304 \ +-device ivshmem-plain,id=shmem0,memdev=shmmem-shmem0,bus=pci.0,addr=0x3 \ +-object memory-backend-file,id=shmmem-shmem1,mem-path=/dev/shm/shmem1,\ +size=134217728 \ +-device ivshmem-plain,id=shmem1,memdev=shmmem-shmem1,bus=pci.0,addr=0x5 \ +-object memory-backend-file,id=shmmem-shmem2,mem-path=/dev/shm/shmem2,\ +size=268435456 \ +-device ivshmem-plain,id=shmem2,memdev=shmmem-shmem2,bus=pci.0,addr=0x4 \ +-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..0e10ce31fb1a --- /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'> + <model type='ivshmem-plain'/> + <size unit='M'>128</size> + </shmem> + <shmem name='shmem2'> + <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 0af71a1dbce5..03e3ca746f14 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2020,6 +2020,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); -- 2.10.0
On 09/27/2016 08:24 AM, 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.
It seems less "added support for" and more "forced libvirt to choose" between one or the other as of qemu v2.6.
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 | 88 +++++++++++++++++++++- src/qemu/qemu_command.h | 10 +++ .../qemuxml2argv-shmem-plain-doorbell.args | 43 +++++++++++ .../qemuxml2argv-shmem-plain-doorbell.xml | 63 ++++++++++++++++ tests/qemuxml2argvtest.c | 3 + 5 files changed, 204 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index aa8cff80e8b1..29f7130ef911 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8456,6 +8456,39 @@ qemuBuildShmemDevLegacyStr(virDomainDefPtr def, return NULL; }
+char * +qemuBuildShmemDevStr(virDomainDefPtr def, + virDomainShmemDefPtr shmem, + virQEMUCapsPtr qemuCaps) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAsprintf(&buf, "%s", virDomainShmemModelTypeToString(shmem->model)); + virBufferAsprintf(&buf, ",id=%s", shmem->info.alias); + + if (shmem->server.enabled) + virBufferAsprintf(&buf, ",chardev=char%s", shmem->info.alias); + else + virBufferAsprintf(&buf, ",memdev=shmmem-%s", 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)); + } + + if (qemuBuildDeviceAddressStr(&buf, def, &shmem->info, qemuCaps) < 0) { + virBufferFreeAndReset(&buf); + return NULL; + } + + if (virBufferCheckError(&buf) < 0)
Still would need to FreeAndReset - I'd be OK if it were an || to the previous if, although I know that causes agita for others. I suppose you could to the error: label path/option too.
+ return NULL; + + return virBufferContentAndReset(&buf); +} + static char * qemuBuildShmemBackendChrStr(virLogManagerPtr logManager, virCommandPtr cmd, @@ -8476,6 +8509,50 @@ 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, + "s:mem-path", mem_path, + "U:size", shmem->size, + NULL);
Hmm... perhaps not so much of an issue as previously thought... This will only be called for the -plain anyway and well shmem->size had better not be zero (since you're using "U:").. So still leaves me wondering if we should fail if a size is provided for -doorbell
+ + 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, @@ -8518,10 +8595,15 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager, break;
Should there be caps checks for : QEMU_CAPS_DEVICE_IVSHMEM_PLAIN QEMU_CAPS_DEVICE_IVSHMEM_DOORBELL You added the caps in the xml2argvtest, so I'd expect them... Obviously they wouldn't work on qemu 2.5 or earlier. As long as the memory leak is handled and there's an answer for the caps checks, this is ACK-able from my POV. John
case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + if (!(devstr = qemuBuildShmemBackendMemStr(shmem))) + return -1; + + virCommandAddArgList(cmd, "-object", devstr, NULL); + VIR_FREE(devstr); + + /* fall-through */ case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("%s device is not supported with this QEMU binary"), - virDomainShmemModelTypeToString(shmem->model)); + devstr = qemuBuildShmemDevStr(def, shmem, qemuCaps); break;
case VIR_DOMAIN_SHMEM_MODEL_LAST: 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..7abc7f8c4be5 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args @@ -0,0 +1,43 @@ +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 \ +-object memory-backend-file,id=shmmem-shmem0,mem-path=/dev/shm/shmem0,\ +size=4194304 \ +-device ivshmem-plain,id=shmem0,memdev=shmmem-shmem0,bus=pci.0,addr=0x3 \ +-object memory-backend-file,id=shmmem-shmem1,mem-path=/dev/shm/shmem1,\ +size=134217728 \ +-device ivshmem-plain,id=shmem1,memdev=shmmem-shmem1,bus=pci.0,addr=0x5 \ +-object memory-backend-file,id=shmmem-shmem2,mem-path=/dev/shm/shmem2,\ +size=268435456 \ +-device ivshmem-plain,id=shmem2,memdev=shmmem-shmem2,bus=pci.0,addr=0x4 \ +-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..0e10ce31fb1a --- /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'> + <model type='ivshmem-plain'/> + <size unit='M'>128</size> + </shmem> + <shmem name='shmem2'> + <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 0af71a1dbce5..03e3ca746f14 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2020,6 +2020,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);
On Fri, Oct 07, 2016 at 12:20:19PM -0400, John Ferlan wrote:
On 09/27/2016 08:24 AM, 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.
It seems less "added support for" and more "forced libvirt to choose" between one or the other as of qemu v2.6.
Well, not really, it's not removed from newer QEMU, it's just deprecated.
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 | 88 +++++++++++++++++++++- src/qemu/qemu_command.h | 10 +++ .../qemuxml2argv-shmem-plain-doorbell.args | 43 +++++++++++ .../qemuxml2argv-shmem-plain-doorbell.xml | 63 ++++++++++++++++ tests/qemuxml2argvtest.c | 3 + 5 files changed, 204 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index aa8cff80e8b1..29f7130ef911 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8456,6 +8456,39 @@ qemuBuildShmemDevLegacyStr(virDomainDefPtr def, return NULL; }
+char * +qemuBuildShmemDevStr(virDomainDefPtr def, + virDomainShmemDefPtr shmem, + virQEMUCapsPtr qemuCaps) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAsprintf(&buf, "%s", virDomainShmemModelTypeToString(shmem->model)); + virBufferAsprintf(&buf, ",id=%s", shmem->info.alias); + + if (shmem->server.enabled) + virBufferAsprintf(&buf, ",chardev=char%s", shmem->info.alias); + else + virBufferAsprintf(&buf, ",memdev=shmmem-%s", 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)); + } + + if (qemuBuildDeviceAddressStr(&buf, def, &shmem->info, qemuCaps) < 0) { + virBufferFreeAndReset(&buf); + return NULL; + } + + if (virBufferCheckError(&buf) < 0)
Still would need to FreeAndReset - I'd be OK if it were an || to the previous if, although I know that causes agita for others.
Well, not really. The content gets free()'d whenever the buf->error is set, so it is already cleared if there was an error.
I suppose you could to the error: label path/option too.
+ return NULL; + + return virBufferContentAndReset(&buf); +} + static char * qemuBuildShmemBackendChrStr(virLogManagerPtr logManager, virCommandPtr cmd, @@ -8476,6 +8509,50 @@ 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, + "s:mem-path", mem_path, + "U:size", shmem->size, + NULL);
Hmm... perhaps not so much of an issue as previously thought... This will only be called for the -plain anyway and well shmem->size had better not be zero (since you're using "U:")..
So still leaves me wondering if we should fail if a size is provided for -doorbell
OK, I added that.
+ + 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, @@ -8518,10 +8595,15 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager, break;
Should there be caps checks for :
QEMU_CAPS_DEVICE_IVSHMEM_PLAIN QEMU_CAPS_DEVICE_IVSHMEM_DOORBELL
Sure, I should've added that, good point.
You added the caps in the xml2argvtest, so I'd expect them... Obviously they wouldn't work on qemu 2.5 or earlier.
As long as the memory leak is handled and there's an answer for the caps checks, this is ACK-able from my POV.
I have to send v4 of a prerequisite patch, so I'll just repost it. Martin
[...]
+char * +qemuBuildShmemDevStr(virDomainDefPtr def, + virDomainShmemDefPtr shmem, + virQEMUCapsPtr qemuCaps) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAsprintf(&buf, "%s", virDomainShmemModelTypeToString(shmem->model)); + virBufferAsprintf(&buf, ",id=%s", shmem->info.alias); + + if (shmem->server.enabled) + virBufferAsprintf(&buf, ",chardev=char%s", shmem->info.alias); + else + virBufferAsprintf(&buf, ",memdev=shmmem-%s", 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)); + } + + if (qemuBuildDeviceAddressStr(&buf, def, &shmem->info, qemuCaps) < 0) { + virBufferFreeAndReset(&buf); + return NULL; + } + + if (virBufferCheckError(&buf) < 0)
Still would need to FreeAndReset - I'd be OK if it were an || to the previous if, although I know that causes agita for others.
Well, not really. The content gets free()'d whenever the buf->error is set, so it is already cleared if there was an error.
That's not how I read/see other callers of virBufferCheckError and I see no free in virBufferCheckErrorInternal John [...]
On Fri, Oct 14, 2016 at 10:19:58AM -0400, John Ferlan wrote:
[...]
+char * +qemuBuildShmemDevStr(virDomainDefPtr def, + virDomainShmemDefPtr shmem, + virQEMUCapsPtr qemuCaps) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAsprintf(&buf, "%s", virDomainShmemModelTypeToString(shmem->model)); + virBufferAsprintf(&buf, ",id=%s", shmem->info.alias); + + if (shmem->server.enabled) + virBufferAsprintf(&buf, ",chardev=char%s", shmem->info.alias); + else + virBufferAsprintf(&buf, ",memdev=shmmem-%s", 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)); + } + + if (qemuBuildDeviceAddressStr(&buf, def, &shmem->info, qemuCaps) < 0) { + virBufferFreeAndReset(&buf); + return NULL; + } + + if (virBufferCheckError(&buf) < 0)
Still would need to FreeAndReset - I'd be OK if it were an || to the previous if, although I know that causes agita for others.
Well, not really. The content gets free()'d whenever the buf->error is set, so it is already cleared if there was an error.
That's not how I read/see other callers of virBufferCheckError and I see no free in virBufferCheckErrorInternal
It's free()'d when it is *set*, see virBufferSetError (the only place in virbuffer.c that sets the ->error member).
John
[...]
This is needed in order to migrate a domain with shmem devices 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 7873a6814ee7..c78db761c112 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7528,6 +7528,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: @@ -7539,7 +7548,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: @@ -7617,6 +7625,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: @@ -7628,7 +7639,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: @@ -7775,6 +7785,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainControllerDefPtr controller; virDomainFSDefPtr fs; virDomainRedirdevDefPtr redirdev; + virDomainShmemDefPtr shmem; switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: @@ -7899,6 +7910,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: @@ -7908,7 +7931,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: @@ -8055,6 +8077,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: @@ -8065,7 +8097,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..03d75be5e3d7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2242,6 +2242,131 @@ 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; + virErrorPtr orig_err = NULL; + virJSONValuePtr props = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + + switch ((virDomainShmemModel)shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: + break; + + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("live attach of shmem model '%s' is not supported"), + virDomainShmemModelTypeToString(shmem->model)); + /* fall-through */ + case VIR_DOMAIN_SHMEM_MODEL_LAST: + 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; + } + + qemuDomainObjEnterMonitor(driver, vm); + + if (shmem->server.enabled) { + if (qemuMonitorAttachCharDev(priv->mon, charAlias, + &shmem->server.chr) < 0) + goto exit_monitor; + } else { + if (qemuMonitorAddObject(priv->mon, "memory-backend-file", + memAlias, props) < 0) { + props = NULL; + goto exit_monitor; + } + props = NULL; + } + + release_backing = true; + + if (qemuMonitorAddDevice(priv->mon, shmstr) < 0) + goto exit_monitor; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + release_address = false; + goto cleanup; + } + + /* Doing a copy here just so the pointer doesn't get nullified + * because we need it in the audit function */ + VIR_APPEND_ELEMENT_COPY_INPLACE(vm->def->shmems, vm->def->nshmems, shmem); + + ret = 0; + release_address = false; + + audit: + virDomainAuditShmem(vm, shmem, "attach", ret == 0); + + cleanup: + if (release_address) + qemuDomainReleaseDeviceAddress(vm, &shmem->info, NULL); + + virJSONValueFree(props); + VIR_FREE(memAlias); + VIR_FREE(charAlias); + VIR_FREE(shmstr); + + return ret; + + exit_monitor: + orig_err = virSaveLastError(); + if (release_backing) { + if (shmem->server.enabled) + ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); + else + ignore_value(qemuMonitorDelObject(priv->mon, memAlias)); + } + + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + release_address = false; + + goto audit; +} + + static int qemuDomainChangeNetBridge(virDomainObjPtr vm, virDomainNetDefPtr olddev, @@ -3486,6 +3611,61 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, } +static int +qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr shmem) +{ + int rc; + int ret = -1; + ssize_t idx = -1; + char *charAlias = NULL; + char *memAlias = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + virObjectEventPtr event = NULL; + + 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) + return -1; + } else { + if (virAsprintf(&memAlias, "shmmem-%s", shmem->info.alias) < 0) + return -1; + } + + qemuDomainObjEnterMonitor(driver, vm); + + if (shmem->server.enabled) + rc = qemuMonitorDetachCharDev(priv->mon, charAlias); + else + rc = qemuMonitorDelObject(priv->mon, memAlias); + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + virDomainAuditShmem(vm, shmem, "detach", rc == 0); + + if (rc < 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 +3697,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 +3714,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 +4288,68 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, return qemuDomainDetachThisHostDevice(driver, vm, detach); } + +int +qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + int ret = -1; + ssize_t idx = -1; + virErrorPtr orig_err = NULL; + virDomainShmemDefPtr shmem = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + + if ((idx = virDomainShmemDefFind(vm->def, dev->data.shmem) < 0)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("device not present in domain configuration")); + return -1; + } + + shmem = vm->def->shmems[idx]; + + switch ((virDomainShmemModel)shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: + break; + + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("live detach of shmem model '%s' is not supported"), + virDomainShmemModelTypeToString(shmem->model)); + /* fall-through */ + case VIR_DOMAIN_SHMEM_MODEL_LAST: + 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 43eb1cfd1fc3..d1357ebd7c57 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); @@ -120,6 +122,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)); @@ -142,6 +147,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)); @@ -561,6 +569,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..68f592fb2128 --- /dev/null +++ b/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain-detach.xml @@ -0,0 +1,6 @@ +<shmem name='shmem0'> + <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..8d09fee265fc --- /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'> + <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..ac3fa4f42047 --- /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'> + <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 09/27/2016 08:24 AM, Martin Kletzander wrote:
This is needed in order to migrate a domain with shmem devices as that is not allowed to migrate.
Sure, but how would anyone know since the migration fails... Not sure where could/should describe it, but perhaps something nice to be described somewhere...
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
Jeez someday I should try to learn how to use these hotplug tests ;-) [...]
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 72dd93bbe467..03d75be5e3d7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2242,6 +2242,131 @@ 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; + virErrorPtr orig_err = NULL; + virJSONValuePtr props = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + + switch ((virDomainShmemModel)shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL:
This is where I'd expect the capabilities checks to be
+ break; + + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("live attach of shmem model '%s' is not supported"), + virDomainShmemModelTypeToString(shmem->model)); + /* fall-through */ + case VIR_DOMAIN_SHMEM_MODEL_LAST: + 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; + } + + qemuDomainObjEnterMonitor(driver, vm); + + if (shmem->server.enabled) { + if (qemuMonitorAttachCharDev(priv->mon, charAlias, + &shmem->server.chr) < 0) + goto exit_monitor; + } else { + if (qemuMonitorAddObject(priv->mon, "memory-backend-file", + memAlias, props) < 0) {
Some day we should change the AddObject to be &props <sigh>
+ props = NULL; + goto exit_monitor; + } + props = NULL; + } + + release_backing = true; + + if (qemuMonitorAddDevice(priv->mon, shmstr) < 0) + goto exit_monitor; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + release_address = false; + goto cleanup; + } + + /* Doing a copy here just so the pointer doesn't get nullified + * because we need it in the audit function */ + VIR_APPEND_ELEMENT_COPY_INPLACE(vm->def->shmems, vm->def->nshmems, shmem); + + ret = 0; + release_address = false; + + audit: + virDomainAuditShmem(vm, shmem, "attach", ret == 0); + + cleanup: + if (release_address) + qemuDomainReleaseDeviceAddress(vm, &shmem->info, NULL); + + virJSONValueFree(props); + VIR_FREE(memAlias); + VIR_FREE(charAlias); + VIR_FREE(shmstr); + + return ret; + + exit_monitor: + orig_err = virSaveLastError(); + if (release_backing) { + if (shmem->server.enabled) + ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); + else + ignore_value(qemuMonitorDelObject(priv->mon, memAlias)); + } + + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + release_address = false; + + goto audit; +} + + static int qemuDomainChangeNetBridge(virDomainObjPtr vm, virDomainNetDefPtr olddev, @@ -3486,6 +3611,61 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, }
+static int +qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr shmem) +{ + int rc; + int ret = -1; + ssize_t idx = -1; + char *charAlias = NULL; + char *memAlias = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + virObjectEventPtr event = NULL; + + 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) + return -1; + } else { + if (virAsprintf(&memAlias, "shmmem-%s", shmem->info.alias) < 0) + return -1; + } + + qemuDomainObjEnterMonitor(driver, vm); + + if (shmem->server.enabled) + rc = qemuMonitorDetachCharDev(priv->mon, charAlias); + else + rc = qemuMonitorDelObject(priv->mon, memAlias); + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + virDomainAuditShmem(vm, shmem, "detach", rc == 0); + + if (rc < 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);
I think there be a virDomainShmemDefFree(shmem) here, right? The virDomainShmemDefRemove only removes shmem from the list
+ + ret = 0; + cleanup: + VIR_FREE(charAlias); + VIR_FREE(memAlias); + + return ret; +} + + int qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3517,6 +3697,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 +3714,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 +4288,68 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, return qemuDomainDetachThisHostDevice(driver, vm, detach); }
+ +int +qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + int ret = -1; + ssize_t idx = -1; + virErrorPtr orig_err = NULL; + virDomainShmemDefPtr shmem = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + + if ((idx = virDomainShmemDefFind(vm->def, dev->data.shmem) < 0)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("device not present in domain configuration")); + return -1; + } + + shmem = vm->def->shmems[idx]; + + switch ((virDomainShmemModel)shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: + break; + + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("live detach of shmem model '%s' is not supported"), + virDomainShmemModelTypeToString(shmem->model)); + /* fall-through */ + case VIR_DOMAIN_SHMEM_MODEL_LAST: + 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);
Why not a direct qemuDomainRemoveShmemDevice(driver, vm, shmem); It's the pattern other code uses - just concern over the difference - it does result in the same call eventually. ACK with the capabilities checks and of course being sure we've free'd shmem. John
+ } + } + 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 43eb1cfd1fc3..d1357ebd7c57 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);
@@ -120,6 +122,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)); @@ -142,6 +147,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)); @@ -561,6 +569,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..68f592fb2128 --- /dev/null +++ b/tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain-detach.xml @@ -0,0 +1,6 @@ +<shmem name='shmem0'> + <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..8d09fee265fc --- /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'> + <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..ac3fa4f42047 --- /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'> + <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>
On Fri, Oct 07, 2016 at 01:05:29PM -0400, John Ferlan wrote:
On 09/27/2016 08:24 AM, Martin Kletzander wrote:
This is needed in order to migrate a domain with shmem devices as that is not allowed to migrate.
Sure, but how would anyone know since the migration fails... Not sure where could/should describe it, but perhaps something nice to be described somewhere...
Because they'll get "migration with shmem device is not supported" message when they try it.
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
Jeez someday I should try to learn how to use these hotplug tests ;-)
I have some reworks of that in the works too, so if you'd like that you can finish the series ;)
[...]
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 72dd93bbe467..03d75be5e3d7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2242,6 +2242,131 @@ 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; + virErrorPtr orig_err = NULL; + virJSONValuePtr props = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + + switch ((virDomainShmemModel)shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL:
This is where I'd expect the capabilities checks to be
Now it's added in the qemuBuildShmemDevStr(), so it will work inherently. [...]
@@ -3486,6 +3611,61 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, }
+static int +qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr shmem) +{ + int rc; + int ret = -1; + ssize_t idx = -1; + char *charAlias = NULL; + char *memAlias = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + virObjectEventPtr event = NULL; + + 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) + return -1; + } else { + if (virAsprintf(&memAlias, "shmmem-%s", shmem->info.alias) < 0) + return -1; + } + + qemuDomainObjEnterMonitor(driver, vm); + + if (shmem->server.enabled) + rc = qemuMonitorDetachCharDev(priv->mon, charAlias); + else + rc = qemuMonitorDelObject(priv->mon, memAlias); + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + virDomainAuditShmem(vm, shmem, "detach", rc == 0); + + if (rc < 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);
I think there be a virDomainShmemDefFree(shmem) here, right? The virDomainShmemDefRemove only removes shmem from the list
Yes, thanks for noticing.
@@ -4105,6 +4288,68 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, return qemuDomainDetachThisHostDevice(driver, vm, detach); }
+ +int +qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + int ret = -1; + ssize_t idx = -1; + virErrorPtr orig_err = NULL; + virDomainShmemDefPtr shmem = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + + if ((idx = virDomainShmemDefFind(vm->def, dev->data.shmem) < 0)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("device not present in domain configuration")); + return -1; + } + + shmem = vm->def->shmems[idx]; + + switch ((virDomainShmemModel)shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: + break; + + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("live detach of shmem model '%s' is not supported"), + virDomainShmemModelTypeToString(shmem->model)); + /* fall-through */ + case VIR_DOMAIN_SHMEM_MODEL_LAST: + 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);
Why not a direct qemuDomainRemoveShmemDevice(driver, vm, shmem);
It's the pattern other code uses - just concern over the difference - it does result in the same call eventually.
What other code? It doesn't necessarily result in the same call every time. That's what qemuDomainWaitForDeviceRemoval() is for. We shouldn't remove it from the definition if QEMU didn't actually remove it.
ACK with the capabilities checks and of course being sure we've free'd shmem.
good to know, those things will be handled in the next version Martin
[...]
+int +qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + int ret = -1; + ssize_t idx = -1; + virErrorPtr orig_err = NULL; + virDomainShmemDefPtr shmem = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + + if ((idx = virDomainShmemDefFind(vm->def, dev->data.shmem) < 0)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("device not present in domain configuration")); + return -1; + } + + shmem = vm->def->shmems[idx]; + + switch ((virDomainShmemModel)shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: + break; + + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("live detach of shmem model '%s' is not supported"), + virDomainShmemModelTypeToString(shmem->model)); + /* fall-through */ + case VIR_DOMAIN_SHMEM_MODEL_LAST: + 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);
Why not a direct qemuDomainRemoveShmemDevice(driver, vm, shmem);
It's the pattern other code uses - just concern over the difference - it does result in the same call eventually.
What other code? It doesn't necessarily result in the same call every time. That's what qemuDomainWaitForDeviceRemoval() is for. We shouldn't remove it from the definition if QEMU didn't actually remove it.
Most callers to qemuDomainWaitForDeviceRemoval except qemuDomainDetachChrDevice which throws in release device address, but still calls its RemoveChrDevice directly rather than the generic RemoveDevice call. John
On Fri, Oct 14, 2016 at 10:25:26AM -0400, John Ferlan wrote:
[...]
+int +qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + int ret = -1; + ssize_t idx = -1; + virErrorPtr orig_err = NULL; + virDomainShmemDefPtr shmem = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + + if ((idx = virDomainShmemDefFind(vm->def, dev->data.shmem) < 0)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("device not present in domain configuration")); + return -1; + } + + shmem = vm->def->shmems[idx]; + + switch ((virDomainShmemModel)shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: + break; + + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("live detach of shmem model '%s' is not supported"), + virDomainShmemModelTypeToString(shmem->model)); + /* fall-through */ + case VIR_DOMAIN_SHMEM_MODEL_LAST: + 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);
Why not a direct qemuDomainRemoveShmemDevice(driver, vm, shmem);
It's the pattern other code uses - just concern over the difference - it does result in the same call eventually.
What other code? It doesn't necessarily result in the same call every time. That's what qemuDomainWaitForDeviceRemoval() is for. We shouldn't remove it from the definition if QEMU didn't actually remove it.
Most callers to qemuDomainWaitForDeviceRemoval except qemuDomainDetachChrDevice which throws in release device address, but still calls its RemoveChrDevice directly rather than the generic RemoveDevice call.
Oh, I misunderstood that, you meant qemuDomainRemoveShmemDevice() instead of qemuDomainRemoveDevice(). Yes, I can do that, I left the Device there because I was using the actual device for something in one of the versions before posting it and then haven't changed it back.
John
participants (3)
-
Daniel P. Berrange -
John Ferlan -
Martin Kletzander