[libvirt] [PATCH 0/4] Introduce support for mediated devices hot plug
Libvirt shouldn't forbid the operation as unsupported. In fact, from VFIO point of view, mdevs have supported hot plug since the beginning. Then it's up to the 3rd party vendor driver whether it can cope with this feature reliably or not. Erik Skultety (4): qemu: hotplug: Provide a string of a subsystem type instead of an int qemu: hotplug: Introduce hot plug support for mediated devices qemu: hotplug: Introduce hot unplug for mediated devices news: Update release news with mediated devices hot {plug,unplug} docs/news.xml | 14 +++++ src/qemu/qemu_hotplug.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 149 insertions(+), 1 deletion(-) -- 2.14.3
If one tries to detach a non-existent device, the error they get is: "Unexpected hostdev type <int>". Let's use ToString conversion, since the XML parser would have complained already if the type to be unplugged was unknown to libvirt. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_hotplug.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 49af4d4ff..6ec401e21 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5077,7 +5077,8 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, break; default: virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected hostdev type %d"), subsys->type); + _("unexpected hostdev type '%s'"), + virDomainHostdevSubsysTypeToString(subsys->type)); break; } return -1; -- 2.14.3
On Tue, Mar 27, 2018 at 10:57:13 +0200, Erik Skultety wrote:
If one tries to detach a non-existent device, the error they get is: "Unexpected hostdev type <int>". Let's use ToString conversion, since the XML parser would have complained already if the type to be unplugged was unknown to libvirt.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_hotplug.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 49af4d4ff..6ec401e21 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5077,7 +5077,8 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, break; default: virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected hostdev type %d"), subsys->type); + _("unexpected hostdev type '%s'"), + virDomainHostdevSubsysTypeToString(subsys->type));
If the type is out of range of the enum this will return NULL. So you need to use NULLSTR() ACK with that tweak
On Tue, Mar 27, 2018 at 11:17:51AM +0200, Peter Krempa wrote:
On Tue, Mar 27, 2018 at 10:57:13 +0200, Erik Skultety wrote:
If one tries to detach a non-existent device, the error they get is: "Unexpected hostdev type <int>". Let's use ToString conversion, since the XML parser would have complained already if the type to be unplugged was unknown to libvirt.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_hotplug.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 49af4d4ff..6ec401e21 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5077,7 +5077,8 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, break; default: virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected hostdev type %d"), subsys->type); + _("unexpected hostdev type '%s'"), + virDomainHostdevSubsysTypeToString(subsys->type));
If the type is out of range of the enum this will return NULL. So you need to use NULLSTR()
ACK with that tweak
Actually any default: or _LAST: case should use virReportEnumRangeError(). The virDomainHostdevSubsysTypeToString() methods should only be used in explicitly matched enums constants. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Mar 27, 2018 at 10:24:54AM +0100, Daniel P. Berrangé wrote:
On Tue, Mar 27, 2018 at 11:17:51AM +0200, Peter Krempa wrote:
On Tue, Mar 27, 2018 at 10:57:13 +0200, Erik Skultety wrote:
If one tries to detach a non-existent device, the error they get is: "Unexpected hostdev type <int>". Let's use ToString conversion, since the XML parser would have complained already if the type to be unplugged was unknown to libvirt.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_hotplug.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 49af4d4ff..6ec401e21 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5077,7 +5077,8 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, break; default: virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected hostdev type %d"), subsys->type); + _("unexpected hostdev type '%s'"), + virDomainHostdevSubsysTypeToString(subsys->type));
If the type is out of range of the enum this will return NULL. So you need to use NULLSTR()
ACK with that tweak
Actually any default: or _LAST: case should use virReportEnumRangeError(). The virDomainHostdevSubsysTypeToString() methods should only be used in explicitly matched enums constants.
I understand having this kind of safe guard, but we have a specific code path here where the XML parser has already taken care of non-existent types and we're left with only types which do not support or do not care about hotplug, therefore an error like "Unexpected value <number> blah" doesn't IMHO make sense and doesn't tell you anything, we should reformulate the whole 'unexpected' part of the error, we expected it just fine, it's just there's no such device in the domain and even if it was, we don't know whether we can do hotplug on it, that's what you can read from the code as you follow the detach procedure. Erik
On Tue, Mar 27, 2018 at 03:42:08PM +0200, Erik Skultety wrote:
On Tue, Mar 27, 2018 at 10:24:54AM +0100, Daniel P. Berrangé wrote:
On Tue, Mar 27, 2018 at 11:17:51AM +0200, Peter Krempa wrote:
On Tue, Mar 27, 2018 at 10:57:13 +0200, Erik Skultety wrote:
If one tries to detach a non-existent device, the error they get is: "Unexpected hostdev type <int>". Let's use ToString conversion, since the XML parser would have complained already if the type to be unplugged was unknown to libvirt.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_hotplug.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 49af4d4ff..6ec401e21 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5077,7 +5077,8 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, break; default: virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected hostdev type %d"), subsys->type); + _("unexpected hostdev type '%s'"), + virDomainHostdevSubsysTypeToString(subsys->type));
If the type is out of range of the enum this will return NULL. So you need to use NULLSTR()
ACK with that tweak
Actually any default: or _LAST: case should use virReportEnumRangeError(). The virDomainHostdevSubsysTypeToString() methods should only be used in explicitly matched enums constants.
I understand having this kind of safe guard, but we have a specific code path here where the XML parser has already taken care of non-existent types and we're left with only types which do not support or do not care about hotplug, therefore an error like "Unexpected value <number> blah" doesn't IMHO make sense and doesn't tell you anything, we should reformulate the whole 'unexpected' part of the error, we expected it just fine, it's just there's no such device in the domain and even if it was, we don't know whether we can do hotplug on it, that's what you can read from the code as you follow the detach procedure.
The point is to be robust against mistakes in the XML parser, or code that runs after it. We've had cases in the code where we parsed the wrong enum into a field, so we would have ended up with unexpected values. Or something could mistakenly overwrite the type value after parsing but before calling this function. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Mar 27, 2018 at 02:46:49PM +0100, Daniel P. Berrangé wrote:
On Tue, Mar 27, 2018 at 03:42:08PM +0200, Erik Skultety wrote:
On Tue, Mar 27, 2018 at 10:24:54AM +0100, Daniel P. Berrangé wrote:
On Tue, Mar 27, 2018 at 11:17:51AM +0200, Peter Krempa wrote:
On Tue, Mar 27, 2018 at 10:57:13 +0200, Erik Skultety wrote:
If one tries to detach a non-existent device, the error they get is: "Unexpected hostdev type <int>". Let's use ToString conversion, since the XML parser would have complained already if the type to be unplugged was unknown to libvirt.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_hotplug.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 49af4d4ff..6ec401e21 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5077,7 +5077,8 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, break; default: virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected hostdev type %d"), subsys->type); + _("unexpected hostdev type '%s'"), + virDomainHostdevSubsysTypeToString(subsys->type));
If the type is out of range of the enum this will return NULL. So you need to use NULLSTR()
ACK with that tweak
Actually any default: or _LAST: case should use virReportEnumRangeError(). The virDomainHostdevSubsysTypeToString() methods should only be used in explicitly matched enums constants.
I understand having this kind of safe guard, but we have a specific code path here where the XML parser has already taken care of non-existent types and we're left with only types which do not support or do not care about hotplug, therefore an error like "Unexpected value <number> blah" doesn't IMHO make sense and doesn't tell you anything, we should reformulate the whole 'unexpected' part of the error, we expected it just fine, it's just there's no such device in the domain and even if it was, we don't know whether we can do hotplug on it, that's what you can read from the code as you follow the detach procedure.
The point is to be robust against mistakes in the XML parser, or code that runs after it. We've had cases in the code where we parsed the wrong enum into a field, so we would have ended up with unexpected values. Or something could mistakenly overwrite the type value after parsing but before calling this function.
Fair enough, I'm going to drop this patch for the time being and create a bite-sized task to add virReportEnumRangeError to all the places where it's missing. Erik
Mediated devices support hot-{plug,unplug} since their introduction in kernel 4.10, however this feature has been missing in libvirt since commit ec783d7c introduced a hostdev type for mdevs. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_hotplug.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6ec401e21..4abc7393b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2567,6 +2567,88 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver, } +static int +qemuDomainAttachMediatedDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr hostdev) +{ + int ret = -1; + char *devstr = NULL; + bool added = false; + bool teardowncgroup = false; + bool teardownlabel = false; + bool teardowndevice = false; + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_HOSTDEV, + { .hostdev = hostdev } }; + + if (qemuDomainEnsurePCIAddress(vm, &dev, driver) < 0) + return -1; + + if (qemuHostdevPrepareMediatedDevices(driver, + vm->def->name, + &hostdev, + 1) < 0) + goto cleanup; + added = true; + + if (qemuDomainNamespaceSetupHostdev(vm, hostdev) < 0) + goto cleanup; + teardowndevice = true; + + if (qemuSetupHostdevCgroup(vm, hostdev) < 0) + goto cleanup; + teardowncgroup = true; + + if (qemuSecuritySetHostdevLabel(driver, vm, hostdev) < 0) + goto cleanup; + teardownlabel = true; + + if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1) < 0) + goto cleanup; + + if (!(devstr = qemuBuildHostdevMediatedDevStr(vm->def, hostdev, + priv->qemuCaps))) + goto cleanup; + + if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs+1) < 0) + goto cleanup; + + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorAddDevice(priv->mon, devstr); + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + ret = -1; + goto cleanup; + } + + virDomainAuditHostdev(vm, hostdev, "attach", ret == 0); + if (ret < 0) + goto cleanup; + + VIR_APPEND_ELEMENT_INPLACE(vm->def->hostdevs, vm->def->nhostdevs, hostdev); + ret = 0; + cleanup: + if (ret < 0) { + if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0) + VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail"); + if (teardownlabel && + qemuSecurityRestoreHostdevLabel(driver, vm, hostdev) < 0) + VIR_WARN("Unable to restore host device labelling on hotplug fail"); + if (teardowndevice && + qemuDomainNamespaceTeardownHostdev(vm, hostdev) < 0) + VIR_WARN("Unable to remove host device from /dev"); + if (added) + qemuHostdevReAttachMediatedDevices(driver, + vm->def->name, + vm->def->hostdevs, + vm->def->nhostdevs); + qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL); + } + VIR_FREE(devstr); + return ret; +} + + int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -2602,6 +2684,10 @@ qemuDomainAttachHostDevice(virQEMUDriverPtr driver, if (qemuDomainAttachSCSIVHostDevice(driver, vm, hostdev) < 0) goto error; break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: + if (qemuDomainAttachMediatedDevice(driver, vm, hostdev) < 0) + goto error; + break; default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, -- 2.14.3
On Tue, Mar 27, 2018 at 10:57:14 +0200, Erik Skultety wrote:
Mediated devices support hot-{plug,unplug} since their introduction in kernel 4.10, however this feature has been missing in libvirt since commit ec783d7c introduced a hostdev type for mdevs.
I think the feature was missing since d77e1a9642fe1efe9aa5f737a640354.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_hotplug.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6ec401e21..4abc7393b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2567,6 +2567,88 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver, }
+static int +qemuDomainAttachMediatedDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr hostdev) +{ + int ret = -1; + char *devstr = NULL; + bool added = false; + bool teardowncgroup = false; + bool teardownlabel = false; + bool teardowndevice = false; + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_HOSTDEV, + { .hostdev = hostdev } }; + + if (qemuDomainEnsurePCIAddress(vm, &dev, driver) < 0) + return -1; + + if (qemuHostdevPrepareMediatedDevices(driver, + vm->def->name, + &hostdev, + 1) < 0) + goto cleanup; + added = true; + + if (qemuDomainNamespaceSetupHostdev(vm, hostdev) < 0) + goto cleanup; + teardowndevice = true; + + if (qemuSetupHostdevCgroup(vm, hostdev) < 0) + goto cleanup; + teardowncgroup = true; + + if (qemuSecuritySetHostdevLabel(driver, vm, hostdev) < 0) + goto cleanup; + teardownlabel = true; + + if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1) < 0) + goto cleanup; + + if (!(devstr = qemuBuildHostdevMediatedDevStr(vm->def, hostdev, + priv->qemuCaps))) + goto cleanup;
Usually we'd call this prior to setting up everything, bug given that this does not do any validation it's okay.
+ + if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs+1) < 0)
Spaces around + sign.
+ goto cleanup; + + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorAddDevice(priv->mon, devstr); + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + ret = -1; + goto cleanup; + } + + virDomainAuditHostdev(vm, hostdev, "attach", ret == 0); + if (ret < 0) + goto cleanup; + + VIR_APPEND_ELEMENT_INPLACE(vm->def->hostdevs, vm->def->nhostdevs, hostdev); + ret = 0; + cleanup: + if (ret < 0) { + if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0) + VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail"); + if (teardownlabel && + qemuSecurityRestoreHostdevLabel(driver, vm, hostdev) < 0) + VIR_WARN("Unable to restore host device labelling on hotplug fail"); + if (teardowndevice && + qemuDomainNamespaceTeardownHostdev(vm, hostdev) < 0) + VIR_WARN("Unable to remove host device from /dev"); + if (added) + qemuHostdevReAttachMediatedDevices(driver, + vm->def->name, + vm->def->hostdevs, + vm->def->nhostdevs);
All of them?!?
+ qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL); + } + VIR_FREE(devstr); + return ret; +} + + int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -2602,6 +2684,10 @@ qemuDomainAttachHostDevice(virQEMUDriverPtr driver, if (qemuDomainAttachSCSIVHostDevice(driver, vm, hostdev) < 0) goto error; break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: + if (qemuDomainAttachMediatedDevice(driver, vm, hostdev) < 0) + goto error; + break;
default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, -- 2.14.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Mediated devices support hot-{plug,unplug} since their introduction in kernel 4.10, however this feature has been missing in libvirt since commit ec783d7c introduced a hostdev type for mdevs. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_hotplug.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4abc7393b..ff77b47bc 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4030,6 +4030,17 @@ qemuDomainRemoveSCSIVHostDevice(virQEMUDriverPtr driver, qemuHostdevReAttachSCSIVHostDevices(driver, vm->def->name, &hostdev, 1); } + +static void +qemuDomainRemoveMediatedDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr hostdev) +{ + qemuHostdevReAttachMediatedDevices(driver, vm->def->name, &hostdev, 1); + qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL); +} + + static int qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -4132,6 +4143,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, qemuDomainRemoveSCSIVHostDevice(driver, vm, hostdev); break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: + qemuDomainRemoveMediatedDevice(driver, vm, hostdev); break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: break; @@ -5059,6 +5071,32 @@ qemuDomainDetachSCSIVHostDevice(virQEMUDriverPtr driver, return ret; } + +static int +qemuDomainDetachMediatedDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr detach) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (!detach->info->alias) { + virReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("device cannot be detached without a device alias")); + return -1; + } + + qemuDomainMarkDeviceForRemoval(vm, detach->info); + + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorDelDevice(priv->mon, detach->info->alias); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; + + return ret; +} + + static int qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -5082,6 +5120,9 @@ qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: ret = qemuDomainDetachSCSIVHostDevice(driver, vm, detach); break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: + ret = qemuDomainDetachMediatedDevice(driver, vm, detach); + break; default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("hot unplug is not supported for hostdev subsys type '%s'"), @@ -5111,6 +5152,7 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, virDomainHostdevSubsysUSBPtr usbsrc = &subsys->u.usb; virDomainHostdevSubsysPCIPtr pcisrc = &subsys->u.pci; virDomainHostdevSubsysSCSIPtr scsisrc = &subsys->u.scsi; + virDomainHostdevSubsysMediatedDevPtr mdevsrc = &subsys->u.mdev; virDomainHostdevDefPtr detach = NULL; int idx; @@ -5159,6 +5201,11 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, } break; } + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: + virReportError(VIR_ERR_DEVICE_MISSING, + _("mediated device '%s' not found"), + mdevsrc->uuidstr); + break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: break; default: -- 2.14.3
On Tue, Mar 27, 2018 at 10:57:15 +0200, Erik Skultety wrote:
Mediated devices support hot-{plug,unplug} since their introduction in kernel 4.10, however this feature has been missing in libvirt since commit ec783d7c introduced a hostdev type for mdevs.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_hotplug.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4abc7393b..ff77b47bc 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4030,6 +4030,17 @@ qemuDomainRemoveSCSIVHostDevice(virQEMUDriverPtr driver, qemuHostdevReAttachSCSIVHostDevices(driver, vm->def->name, &hostdev, 1); }
+ +static void +qemuDomainRemoveMediatedDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr hostdev) +{ + qemuHostdevReAttachMediatedDevices(driver, vm->def->name, &hostdev, 1); + qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL);
Looks like you are missing teardown of the cgroups, and namespace membership here.
+} + + static int qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -4132,6 +4143,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, qemuDomainRemoveSCSIVHostDevice(driver, vm, hostdev); break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: + qemuDomainRemoveMediatedDevice(driver, vm, hostdev); break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: break; @@ -5059,6 +5071,32 @@ qemuDomainDetachSCSIVHostDevice(virQEMUDriverPtr driver, return ret; }
+ +static int +qemuDomainDetachMediatedDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr detach) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (!detach->info->alias) { + virReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("device cannot be detached without a device alias")); + return -1; + } + + qemuDomainMarkDeviceForRemoval(vm, detach->info); + + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorDelDevice(priv->mon, detach->info->alias); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;
You need to wait for removal here and delete it inplace if the call returns soon enough.. Also call to qemuDomainResetDeviceRemoval is missing.
+ + return ret; +} + + static int qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -5082,6 +5120,9 @@ qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: ret = qemuDomainDetachSCSIVHostDevice(driver, vm, detach); break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: + ret = qemuDomainDetachMediatedDevice(driver, vm, detach); + break; default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("hot unplug is not supported for hostdev subsys type '%s'"), @@ -5111,6 +5152,7 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, virDomainHostdevSubsysUSBPtr usbsrc = &subsys->u.usb; virDomainHostdevSubsysPCIPtr pcisrc = &subsys->u.pci; virDomainHostdevSubsysSCSIPtr scsisrc = &subsys->u.scsi; + virDomainHostdevSubsysMediatedDevPtr mdevsrc = &subsys->u.mdev; virDomainHostdevDefPtr detach = NULL; int idx;
@@ -5159,6 +5201,11 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, } break; } + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: + virReportError(VIR_ERR_DEVICE_MISSING, + _("mediated device '%s' not found"), + mdevsrc->uuidstr); + break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: break; default: -- 2.14.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Mar 27, 2018 at 11:41:45AM +0200, Peter Krempa wrote:
On Tue, Mar 27, 2018 at 10:57:15 +0200, Erik Skultety wrote:
Mediated devices support hot-{plug,unplug} since their introduction in kernel 4.10, however this feature has been missing in libvirt since commit ec783d7c introduced a hostdev type for mdevs.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_hotplug.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4abc7393b..ff77b47bc 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4030,6 +4030,17 @@ qemuDomainRemoveSCSIVHostDevice(virQEMUDriverPtr driver, qemuHostdevReAttachSCSIVHostDevices(driver, vm->def->name, &hostdev, 1); }
+ +static void +qemuDomainRemoveMediatedDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr hostdev) +{ + qemuHostdevReAttachMediatedDevices(driver, vm->def->name, &hostdev, 1); + qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL);
Looks like you are missing teardown of the cgroups, and namespace membership here.
I'm not, this is handled from qemuDomainRemoveHostDevice which is called from qemuDomainDetachThisHostDevice right after qemuDomainDetachMediatedDevice was called. ...
+static int +qemuDomainDetachMediatedDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr detach) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (!detach->info->alias) { + virReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("device cannot be detached without a device alias")); + return -1; + } + + qemuDomainMarkDeviceForRemoval(vm, detach->info); + + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorDelDevice(priv->mon, detach->info->alias); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;
You need to wait for removal here and delete it inplace if the call returns soon enough.. Also call to qemuDomainResetDeviceRemoval is missing.
All of this is already done as part of qemuDomainDetachThisHostDevice, point 1 being done right at the end of the function mentioned above (qemuDomainWaitForDeviceRemoval), point 2 being satisfied by the last call in the same function. Erik
On Wed, Mar 28, 2018 at 11:47:47 +0200, Erik Skultety wrote:
On Tue, Mar 27, 2018 at 11:41:45AM +0200, Peter Krempa wrote:
On Tue, Mar 27, 2018 at 10:57:15 +0200, Erik Skultety wrote:
Mediated devices support hot-{plug,unplug} since their introduction in kernel 4.10, however this feature has been missing in libvirt since commit ec783d7c introduced a hostdev type for mdevs.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_hotplug.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4abc7393b..ff77b47bc 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4030,6 +4030,17 @@ qemuDomainRemoveSCSIVHostDevice(virQEMUDriverPtr driver, qemuHostdevReAttachSCSIVHostDevices(driver, vm->def->name, &hostdev, 1); }
+ +static void +qemuDomainRemoveMediatedDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr hostdev) +{ + qemuHostdevReAttachMediatedDevices(driver, vm->def->name, &hostdev, 1); + qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL);
Looks like you are missing teardown of the cgroups, and namespace membership here.
I'm not, this is handled from qemuDomainRemoveHostDevice which is called from qemuDomainDetachThisHostDevice right after qemuDomainDetachMediatedDevice was called.
Okay, I did not notice that this was registered under the hostdev removal function rather than the device removal function.
+static int +qemuDomainDetachMediatedDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr detach) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (!detach->info->alias) { + virReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("device cannot be detached without a device alias")); + return -1; + } + + qemuDomainMarkDeviceForRemoval(vm, detach->info); + + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorDelDevice(priv->mon, detach->info->alias); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1;
You need to wait for removal here and delete it inplace if the call returns soon enough.. Also call to qemuDomainResetDeviceRemoval is missing.
All of this is already done as part of qemuDomainDetachThisHostDevice, point 1 being done right at the end of the function mentioned above (qemuDomainWaitForDeviceRemoval), point 2 being satisfied by the last call in the same function.
Fair enough, both points come from the fact that I've assumed that these are generic device deletion functions as we have for other devices and not hostdev-specific ones, called from the hostdev removal function. The confusion originates from the fact that the hostdev-specific workers use the same prefix as other device deletion fucntions rather than having something like "hostdev" in the name. ACK to this patch if you fix the very long line as suggested.
On Tue, Mar 27, 2018 at 10:57:15 +0200, Erik Skultety wrote:
Mediated devices support hot-{plug,unplug} since their introduction in kernel 4.10, however this feature has been missing in libvirt since commit ec783d7c introduced a hostdev type for mdevs.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_hotplug.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4abc7393b..ff77b47bc 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c
[...]
@@ -5059,6 +5071,32 @@ qemuDomainDetachSCSIVHostDevice(virQEMUDriverPtr driver, return ret; }
+ +static int +qemuDomainDetachMediatedDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr detach) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (!detach->info->alias) { + virReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("device cannot be detached without a device alias"));
This line is very long and can be optimized by moving the formatting string on the previous line.
+ return -1; + }
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/news.xml | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 108889574..6075cc9ca 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -92,6 +92,20 @@ previously started domains to 256, or restart them. </description> </change> + <change> + <summary> + qemu: Support hot plug and hot unplug with mediated devices + </summary> + <description> + Libvirt now allows mediated devices to be hot plugged and hot + unplugged from a guest rather than reporting an error that this isn't + supported. In fact, this has been supported with mediated devices + since their first appearance in 4.10 kernel. Be ware though that this + still doesn't guarantee any functionality with respect to the guest + driver which might not cope with a device being hot plugged or + unplugged. + </description> + </change> </section> <section title="Bug fixes"> </section> -- 2.14.3
On Tue, Mar 27, 2018 at 10:57:16 +0200, Erik Skultety wrote:
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- docs/news.xml | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/docs/news.xml b/docs/news.xml index 108889574..6075cc9ca 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -92,6 +92,20 @@ previously started domains to 256, or restart them. </description> </change> + <change> + <summary> + qemu: Support hot plug and hot unplug with mediated devices
s/with/of/
+ </summary> + <description> + Libvirt now allows mediated devices to be hot plugged and hot + unplugged from a guest rather than reporting an error that this isn't
The end of the sentence is kind of obvious.
+ supported. In fact, this has been supported with mediated devices
I'd say 'In fact, kernel supported this ...' so that it's obvious which component you are speaking of.
+ since their first appearance in 4.10 kernel. Be ware though that this + still doesn't guarantee any functionality with respect to the guest + driver which might not cope with a device being hot plugged or + unplugged.
As with any hotplug. The drivers in the guest can ignore the device, or disallow unplug if it is not supported. If you want to point out that drivers currently don't support it in some cases, please be more direct.
participants (3)
-
Daniel P. Berrangé -
Erik Skultety -
Peter Krempa