[libvirt] [PATCH 8/8] qemu: Add support for hot/cold-(un)plug of shmem devices
Martin Kletzander
mkletzan at redhat.com
Fri Oct 14 14:02:16 UTC 2016
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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161014/347cb396/attachment-0001.sig>
More information about the libvir-list
mailing list