[libvirt] [PATCH 0/3] PCI device assignment bugfixes
These were all found as a result of researching Bug 1035490. Patch 1 fixes the originally reported problem, Patch 2 fixes the cause of incorrect error reporting in a later comment of that bug, and Patch 3 fixes a behavior that I encountered while searching for the cause. Laine Stump (3): qemu: properly set MaxMemLock when hotplugging with VFIO qemu: avoid duplicate security label restore on hostdev attach failure qemu: re-add hostdev interfaces to hostdev array on libvirtd restart src/conf/domain_conf.c | 9 ++++++--- src/qemu/qemu_hotplug.c | 22 +++++++++++----------- 2 files changed, 17 insertions(+), 14 deletions(-) -- 1.8.3.1
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1035490 virProcessSetMaxMemLock() (which is a wrapper over prlimit(3)) expects the memory size in bytes, but libvirt's domain definition (which was being used by qemuDomainAttachHostPciDevice()) stores all memory tuning parameters in KiB. This was being accounted for when setting MaxMemLock at domain startup time (so cold-plugged devices would work), but not for hotplug. This patch simplifies the few lines that call virProcessSetMemMaxLock(), and multiply the amount * 1024 so that we're locking the correct amount of memory. What remains a mystery to me is why hot-plug of a managed='no' device would succeed (at least on my system) while managed='yes' would fail. I guess in one case the memory was coincidentally already resident and in the other it wasn't. --- src/qemu/qemu_hotplug.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a375b6c..4a2c5ce 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1155,6 +1155,7 @@ qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver, bool teardowncgroup = false; bool teardownlabel = false; int backend = hostdev->source.subsys.u.pci.backend; + unsigned long long memKB; if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) return -1; @@ -1172,16 +1173,18 @@ qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver, goto error; } - /* VFIO requires all of the guest's memory to be locked resident. - * In this case, the guest's memory may already be locked, but it + /* VFIO requires all of the guest's memory to be locked + * resident (plus an additional 1GiB to cover IO space). During + * hotplug, the guest's memory may already be locked, but it * doesn't hurt to "change" the limit to the same value. + * NB: the domain's memory tuning parameters are stored as + * Kibibytes, but virProcessSetMaxMemLock expects the value in + * bytes. */ - if (vm->def->mem.hard_limit) - virProcessSetMaxMemLock(vm->pid, vm->def->mem.hard_limit); - else - virProcessSetMaxMemLock(vm->pid, - vm->def->mem.max_balloon + (1024 * 1024)); - + memKB = vm->def->mem.hard_limit + ? vm->def->mem.hard_limit + : vm->def->mem.max_balloon + (1024 * 1024); + virProcessSetMaxMemLock(vm->pid, memKB * 1024); break; case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT: -- 1.8.3.1
This eliminates the misleading error message that was being logged when a vfio hostdev hotplug failed: error: unable to set user and group to '107:107' on '/dev/vfio/22': No such file or directory as documented in: https://bugzilla.redhat.com/show_bug.cgi?id=1035490 Commit ee414b5d (pushed as a fix for Bug 1016511 and part of Bug 1025108) replaced the single call to virSecurityManagerSetHostdevLabel() in qemuDomainAttachHostDevice() with individual calls to that same function in each device-type-specific attach function (for PCI, USB, and SCSI). It also added a corresponding call to virSecurityManagerRestoreHostdevLabel() in the error handling of the device-type-specific functions, but forgot to remove the common call to that from qemuDomainAttachHostDevice() - this resulted in a duplicate call to virSecurityManagerRestoreHostdevLabel(), with the second occurrence being after (e.g.) a PCI device has already been re-attached to the host driver, thus destroying some of the device nodes / links that we then attempted to re-label (e.f. /dev/vfio/22) and generating an error log that obscured the original error. --- src/qemu/qemu_hotplug.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4a2c5ce..7a8caf1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1666,9 +1666,6 @@ int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, return 0; error: - if (virSecurityManagerRestoreHostdevLabel(driver->securityManager, - vm->def, hostdev, NULL) < 0) - VIR_WARN("Unable to restore host device labelling on hotplug fail"); return -1; } -- 1.8.3.1
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1045002 If a domain has an <interface type='hostdev'> or an <interface type='network'> where the network itself is a pool of hostdev devices, then libvirt will internally keep that device on both the interface list *and* the hostdev list for the domain. One of the places this comes in handy is when a new device is being added and libvirt wants to find a unique "alias" name for it - it just scans through the hostdev array and makes sure it picks a name that doesn't match the alias of any device in that array. However, when libvirtd was restarted, if there was an <interface type='network'> with the network being a hostdev pool, the device would not be added to the reconstructed internal hostdev array, so its alias would not be found during a scan of the hostdev array, thus attempts to add a new hostdev (or <interface type='hostdev'> or <interface type='network'>) would result in a message like this: internal error: unable to execute QEMU command 'device_add': Duplicate ID 'hostdev0' for device This patch simply fixes the existing code in the domain XML parser that fixes up the hostdev array in the case of <interface type='hostdev'> to do the same thing in the case of <interface type='network'> with a hostdev network. This bug has existed since the very first addition of hostdev networks to libvirt (0.10.0). --- src/conf/domain_conf.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0079234..583ea02 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12101,9 +12101,12 @@ virDomainDefParseXML(xmlDocPtr xml, def->nets[def->nnets++] = net; - /* <interface type='hostdev'> must also be in the hostdevs array */ - if (net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV && - virDomainHostdevInsert(def, &net->data.hostdev.def) < 0) { + /* <interface type='hostdev'> (and <interface type='net'> + * where the actual network type is already known to be + * hostdev) must also be in the hostdevs array. + */ + if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV && + virDomainHostdevInsert(def, virDomainNetGetActualHostdev(net)) < 0) { goto error; } } -- 1.8.3.1
On Thu, Dec 19, 2013 at 16:53:17 +0200, Laine Stump wrote:
These were all found as a result of researching Bug 1035490. Patch 1 fixes the originally reported problem, Patch 2 fixes the cause of incorrect error reporting in a later comment of that bug, and Patch 3 fixes a behavior that I encountered while searching for the cause.
Laine Stump (3): qemu: properly set MaxMemLock when hotplugging with VFIO qemu: avoid duplicate security label restore on hostdev attach failure qemu: re-add hostdev interfaces to hostdev array on libvirtd restart
src/conf/domain_conf.c | 9 ++++++--- src/qemu/qemu_hotplug.c | 22 +++++++++++----------- 2 files changed, 17 insertions(+), 14 deletions(-)
ACK series. Jirka
participants (2)
-
Jiri Denemark -
Laine Stump