Re: [PATCH 8/8] qemu: Add support for hot/cold-(un)plug of shmem devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

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@xxxxxxxxxx>
---
 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

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]