[PATCH 0/3] Fix hotplug of unassigned hostdevs
Honestly, I don't understand the reasoning behind <address type='unassigned'/> (esp. when the device is not accessible to the guest anyway). We offer virNodeDeviceDettach() and virNodeDeviceReAttach(). But apparently there are some issues with hotplug/hotunplug of hostdevs with such type of address. Michal Prívozník (3): domain_validate: Refuse VIR_DOMAIN_DEVICE_ADDRESS_TYPE_UNASSIGNED qemuDomainAttachHostPCIDevice: Handle hostevs with unassigned type of address qemuDomainDetachDeviceLive: Handle hostevs with unassigned type of address src/conf/domain_validate.c | 45 ++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.c | 20 ++++++++++++++--- 2 files changed, 62 insertions(+), 3 deletions(-) -- 2.34.1
We document that <address type='unassigned'/> can be used only for <hostdev/>-s. However, corresponding validation rule is missing. Let's put the rule into hypervisor agnostic part of validation process so that all drivers can benefit. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_validate.c | 45 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index e9baf1d41a..d5e74db243 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -2257,10 +2257,55 @@ virDomainGraphicsDefValidate(const virDomainDef *def, return 0; } +static int +virDomainDeviceInfoValidate(const virDomainDeviceDef *dev) +{ + virDomainDeviceInfo *info; + + if (!(info = virDomainDeviceGetInfo(dev))) + return 0; + + switch ((virDomainDeviceAddressType) info->type) { + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM: + /* No validation for these address types yet */ + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_UNASSIGNED: + if (dev->type != VIR_DOMAIN_DEVICE_HOSTDEV) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("address of type '%s' is supported only for hostdevs"), + virDomainDeviceAddressTypeToString(info->type)); + return -1; + } + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST: + default: + virReportEnumRangeError(virDomainDeviceAddressType, info->type); + return -1; + } + + return 0; +} + static int virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev, const virDomainDef *def) { + if (virDomainDeviceInfoValidate(dev) < 0) + return -1; + switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: return virDomainDiskDefValidate(def, dev->data.disk); -- 2.34.1
On a Tuesday in 2022, Michal Privoznik wrote:
We document that <address type='unassigned'/> can be used only for <hostdev/>-s. However, corresponding validation rule is missing. Let's put the rule into hypervisor agnostic part of validation process so that all drivers can benefit.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_validate.c | 45 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
A <hostdev/> can have <address type='unassigned'/> which means libvirt manages the device detach from/reattach to the host but the device is never exposed to the guest. This means that we have to take a shortcut during hotplug, similar to the one we are taking when constructing the command line (see qemuBuildHostdevCommandLine()). Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2040548 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f36de2385a..b998b51f5a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1668,6 +1668,12 @@ qemuDomainAttachHostPCIDevice(virQEMUDriver *driver, qemuDomainFillDeviceIsolationGroup(vm->def, &dev); } + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_UNASSIGNED) { + /* Unassigned devices are not exposed to QEMU. Our job is done here. */ + ret = 0; + goto done; + } + if (qemuDomainEnsurePCIAddress(vm, &dev) < 0) goto error; releaseaddr = true; @@ -1692,6 +1698,7 @@ qemuDomainAttachHostPCIDevice(virQEMUDriver *driver, exit_monitor: qemuDomainObjExitMonitor(driver, vm); + done: virDomainAuditHostdev(vm, hostdev, "attach", ret == 0); if (ret < 0) goto error; -- 2.34.1
On a Tuesday in 2022, Michal Privoznik wrote:
A <hostdev/> can have <address type='unassigned'/> which means libvirt manages the device detach from/reattach to the host but the device is never exposed to the guest. This means that we have to take a shortcut during hotplug, similar to the one we are taking when constructing the command line (see qemuBuildHostdevCommandLine()).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2040548 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 7 +++++++ 1 file changed, 7 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
A <hostdev/> can have <address type='unassigned'/> which means libvirt manages the device detach from/reattach to the host but the device is never exposed to the guest. This means that we have to take a shortcut during hotunplug (e.g. never ask QEMU on the monitor to detach the device, or never wait for DEVICE_DELETED event). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b998b51f5a..bde5f683d8 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -6043,6 +6043,7 @@ qemuDomainDetachDeviceLive(virDomainObj *vm, { virDomainDeviceDef detach = { .type = match->type }; virDomainDeviceInfo *info = NULL; + bool unassigned = false; int ret = -1; switch ((virDomainDeviceType)match->type) { @@ -6181,6 +6182,8 @@ qemuDomainDetachDeviceLive(virDomainObj *vm, return -1; } + unassigned = info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_UNASSIGNED; + if (qemuIsMultiFunctionDevice(vm->def, info)) { virReportError(VIR_ERR_OPERATION_FAILED, _("cannot hot unplug %s device with multifunction PCI guest address: " @@ -6225,8 +6228,10 @@ qemuDomainDetachDeviceLive(virDomainObj *vm, * Issue the qemu monitor command to delete the device (based on * its alias), and optionally wait a short time in case the * DEVICE_DELETED event arrives from qemu right away. + * Unassigned devices are not exposed to QEMU, so mimic + * !async case. */ - if (!async) + if (!async || unassigned) qemuDomainMarkDeviceForRemoval(vm, info); if (qemuDomainDeleteDevice(vm, info->alias) < 0) { @@ -6235,11 +6240,13 @@ qemuDomainDetachDeviceLive(virDomainObj *vm, goto cleanup; } - if (async) { + if (async && !unassigned) { ret = 0; } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + if (unassigned || + (ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) { ret = qemuDomainRemoveDevice(driver, vm, &detach); + } } cleanup: -- 2.34.1
On a Tuesday in 2022, Michal Privoznik wrote:
A <hostdev/> can have <address type='unassigned'/> which means libvirt manages the device detach from/reattach to the host but the device is never exposed to the guest. This means that we have to take a shortcut during hotunplug (e.g. never ask QEMU on the monitor to detach the device, or never wait for DEVICE_DELETED event).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b998b51f5a..bde5f683d8 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -6043,6 +6043,7 @@ qemuDomainDetachDeviceLive(virDomainObj *vm, { virDomainDeviceDef detach = { .type = match->type }; virDomainDeviceInfo *info = NULL; + bool unassigned = false; int ret = -1;
switch ((virDomainDeviceType)match->type) { @@ -6181,6 +6182,8 @@ qemuDomainDetachDeviceLive(virDomainObj *vm, return -1; }
+ unassigned = info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_UNASSIGNED; + if (qemuIsMultiFunctionDevice(vm->def, info)) { virReportError(VIR_ERR_OPERATION_FAILED, _("cannot hot unplug %s device with multifunction PCI guest address: " @@ -6225,8 +6228,10 @@ qemuDomainDetachDeviceLive(virDomainObj *vm, * Issue the qemu monitor command to delete the device (based on * its alias), and optionally wait a short time in case the * DEVICE_DELETED event arrives from qemu right away. + * Unassigned devices are not exposed to QEMU, so mimic + * !async case. */ - if (!async) + if (!async || unassigned) qemuDomainMarkDeviceForRemoval(vm, info);
This does not seem right - we won't be waiting for an event from QEMU so we don't need to mark the alias for unassigned devices.
if (qemuDomainDeleteDevice(vm, info->alias) < 0) {
qemuDomainDeleteDevice is the one that calls 'device_del' on the monitor, it should not be called for unassigned devices.
@@ -6235,11 +6240,13 @@ qemuDomainDetachDeviceLive(virDomainObj *vm, goto cleanup; }
- if (async) { + if (async && !unassigned) { ret = 0; } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + if (unassigned || + (ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) { ret = qemuDomainRemoveDevice(driver, vm, &detach);
RemoveDevice is the only interesting function. Instead of prefixing each line of code with if (unassigned) or if (!unassigned) depending on whether it makes sense for unassigned hostdevs, it would be more readable to call qemuDomainRemoveDevice earlier and return early. Jano
+ } }
cleanup: -- 2.34.1
participants (2)
-
Ján Tomko -
Michal Privoznik