[PATCH v2 0/3] Fix hotplug of unassigned hostdevs
v2 of: https://listman.redhat.com/archives/libvir-list/2022-January/msg01111.html Patches 1/3 and 2/3 were ACKed but I'm sending them for completeness. diff to v1: - Reworked patch 3/3 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 | 11 ++++++++++ 2 files changed, 56 insertions(+) -- 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> Reviewed-by: Ján Tomko <jtomko@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
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> Reviewed-by: Ján Tomko <jtomko@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
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 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b998b51f5a..31141cf7df 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -6219,6 +6219,10 @@ qemuDomainDetachDeviceLive(virDomainObj *vm, info->addr.pci.slot, info->addr.pci.function); return -1; } + } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_UNASSIGNED) { + /* Unassigned devices are not exposed to QEMU, so remove the device + * explicitly, just like if we received DEVICE_DELETED event.*/ + return qemuDomainRemoveDevice(driver, vm, &detach); } /* -- 2.34.1
s/hostevs/hostdevs/ in the commit summary On a Wednesday 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 | 4 ++++ 1 file changed, 4 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On 1/26/22 06:29, Michal Privoznik wrote:
v2 of:
https://listman.redhat.com/archives/libvir-list/2022-January/msg01111.html
Patches 1/3 and 2/3 were ACKed but I'm sending them for completeness.
Back when I've added this UNASSIGNED address type I was also pursuing a PCI multifunction hotplug/unplug feature in Libvirt and, apparently, I didn't mind testing hotplug/unplug of non-multifunction unassigned hostdevs like the bug mentioned in patch 1 did. Thanks for fixing this up. Daniel
diff to v1: - Reworked patch 3/3
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 | 11 ++++++++++ 2 files changed, 56 insertions(+)
On 1/27/22 14:57, Daniel Henrique Barboza wrote:
On 1/26/22 06:29, Michal Privoznik wrote:
v2 of:
https://listman.redhat.com/archives/libvir-list/2022-January/msg01111.html
Patches 1/3 and 2/3 were ACKed but I'm sending them for completeness.
Back when I've added this UNASSIGNED address type I was also pursuing a PCI multifunction hotplug/unplug feature in Libvirt and, apparently, I didn't mind testing hotplug/unplug of non-multifunction unassigned hostdevs like the bug mentioned in patch 1 did.
Thanks for fixing this up.
yeah, this is more of a workaround than a real fix, because IIUC unassigned PCI address makes sense for devices within one IOMMU group. I mean, you want to passthrough one specific PCI device but because it's in IOMMU group with another one, both have to be detached from the host. Well, and for that we would need the hotplug/hotunplug API understand that and attach/detach two or more devices at once. With my patches you can still hotplug/hotunplug just a single device, but hey, at least we don't error out! Michal
On 1/27/22 11:05, Michal Prívozník wrote:
On 1/27/22 14:57, Daniel Henrique Barboza wrote:
On 1/26/22 06:29, Michal Privoznik wrote:
v2 of:
https://listman.redhat.com/archives/libvir-list/2022-January/msg01111.html
Patches 1/3 and 2/3 were ACKed but I'm sending them for completeness.
Back when I've added this UNASSIGNED address type I was also pursuing a PCI multifunction hotplug/unplug feature in Libvirt and, apparently, I didn't mind testing hotplug/unplug of non-multifunction unassigned hostdevs like the bug mentioned in patch 1 did.
Thanks for fixing this up.
yeah, this is more of a workaround than a real fix, because IIUC unassigned PCI address makes sense for devices within one IOMMU group. I mean, you want to passthrough one specific PCI device but because it's in IOMMU group with another one, both have to be detached from the host.
Yes. The context where I added this UNASSIGNED address type was to allow a whole PCI multifunction card to be managed by Libvirt, which would detach all the functions from the IOMMU grupo of the host and, at the same time, filter which functions would be visible to the guest. I didn't foresee this usage back then.
Well, and for that we would need the hotplug/hotunplug API understand that and attach/detach two or more devices at once. With my patches you can still hotplug/hotunplug just a single device, but hey, at least we don't error out!
You could also just forbid hotplug of UNASSIGNED non-multifunction hostdevs. I can't think of an actual use of detaching a single hostdev from the host and doing nothing with it. If the idea is to refrain the host from using the hostdev the user can just nodedev-detach it. Matter of fact, I'd say that the 'UNASSIGNED' address type makes little sense for non-multifuction devices altogether. Thanks, Daniel
Michal
participants (4)
-
Daniel Henrique Barboza -
Ján Tomko -
Michal Privoznik -
Michal Prívozník