Re: [libvirt] [PATCH 8/8] qemu: Add support for hot/cold-(un)plug of shmem devices
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
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 8/8] qemu: Add support for hot/cold-(un)plug of shmem devices
[...]
>>> +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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 8/8] qemu: Add support for hot/cold-(un)plug of shmem devices
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
---
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 12
tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell-detach.xml
create mode 100644
tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell.xml
create mode 12
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 qemuDomainDetachThis
Re: [libvirt] [PATCH 8/8] qemu: Add support for hot/cold-(un)plug of shmem devices
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
> ---
> 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 12
> tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell-detach.xml
> create mode 100644
> tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell.xml
> create mode 12
> 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
> +props = NULL;
> +goto exit_monitor;
> +}
> +props = NULL;
> +}
> +
> +release_backing = true;
> +
> +if (qemuMonitorAddDevice
