On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote:

There is no need to open code the PCI address string format when we have a function that does exactly that.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_audit.c | 6 +----- src/util/virpci.c | 6 ++---- 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 8bc6633af4..5fa65a8078 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -361,11 +361,7 @@ virDomainAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev, case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS: switch ((virDomainHostdevSubsysType) hostdev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: - address = g_strdup_printf(VIR_PCI_DEVICE_ADDRESS_FMT, - pcisrc->addr.domain, - pcisrc->addr.bus, - pcisrc->addr.slot, - pcisrc->addr.function); + address = virPCIDeviceAddressAsString(&pcisrc->addr); break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: address = g_strdup_printf("%.3d.%.3d", usbsrc->bus, usbsrc->device); diff --git a/src/util/virpci.c b/src/util/virpci.c index 50fd5ef7ea..5f50f92abb 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1471,8 +1471,7 @@ virPCIDeviceNew(unsigned int domain, dev->address.slot = slot; dev->address.function = function;

- dev->name = g_strdup_printf(VIR_PCI_DEVICE_ADDRESS_FMT, domain, bus, slot, - function); + dev->name = virPCIDeviceAddressAsString(&dev->address);

dev->path = g_strdup_printf(PCI_SYSFS "devices/%s/config", dev->name);

@@ -1998,8 +1997,7 @@ virPCIDeviceAddressGetIOMMUGroupNum(virPCIDeviceAddressPtr addr) g_autofree char *groupNumStr = NULL; unsigned int groupNum;

- devName = g_strdup_printf(VIR_PCI_DEVICE_ADDRESS_FMT, addr->domain, addr->bus, - addr->slot, addr->function); + devName = virPCIDeviceAddressAsString(addr);

devPath = virPCIFile(devName, "iommu_group");

Reviewed-by: Laine Stump <laine@redhat.com>

On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote:

libxlNodeDeviceGetPCIInfo() and qemuNodeDeviceGetPCIInfo() are equal. Let's move the logic to a new virDomainDriverNodeDeviceGetPCIInfo() info to be used by libxl_driver.c and qemu_driver.c.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/hypervisor/domain_driver.c | 33 +++++++++++++++++++++++++++++ src/hypervisor/domain_driver.h | 7 +++++++ src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 38 ++++------------------------------ src/qemu/qemu_driver.c | 37 +++------------------------------ 5 files changed, 48 insertions(+), 68 deletions(-)

diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index 8dc5870a61..68dbf10ac6 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -21,6 +21,7 @@ #include <config.h>

#include "domain_driver.h" +#include "node_device_conf.h"

Since you #include node_device_conf.h from domain_driver.h, you don't need to include it here. With that fixed, Reviewed-by: Laine Stump <laine@redhat.com>

#include "viralloc.h" #include "virstring.h" #include "vircrypto.h" @@ -336,3 +337,35 @@ virDomainDriverSetupPersistentDefBlkioParams(virDomainDefPtr persistentDef,

return ret; } + + +int +virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def, + unsigned *domain, + unsigned *bus, + unsigned *slot, + unsigned *function) +{ + virNodeDevCapsDefPtr cap; + + cap = def->caps; + while (cap) { + if (cap->data.type == VIR_NODE_DEV_CAP_PCI_DEV) { + *domain = cap->data.pci_dev.domain; + *bus = cap->data.pci_dev.bus; + *slot = cap->data.pci_dev.slot; + *function = cap->data.pci_dev.function; + break; + } + + cap = cap->next; + } + + if (!cap) { + virReportError(VIR_ERR_INVALID_ARG, + _("device %s is not a PCI device"), def->name); + return -1; + } + + return 0; +} diff --git a/src/hypervisor/domain_driver.h b/src/hypervisor/domain_driver.h index b66ae2d421..2bb053d559 100644 --- a/src/hypervisor/domain_driver.h +++ b/src/hypervisor/domain_driver.h @@ -21,6 +21,7 @@ #pragma once

#include "domain_conf.h" +#include "node_device_conf.h"

char * virDomainDriverGenerateRootHash(const char *drivername, @@ -45,3 +46,9 @@ int virDomainDriverParseBlkioDeviceStr(char *blkioDeviceStr, const char *type, int virDomainDriverSetupPersistentDefBlkioParams(virDomainDefPtr persistentDef, virTypedParameterPtr params, int nparams); + +int virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def, + unsigned *domain, + unsigned *bus, + unsigned *slot, + unsigned *function); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c325040b60..55284577b0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1445,6 +1445,7 @@ virDomainCgroupSetupMemtune; virDomainDriverGenerateMachineName; virDomainDriverGenerateRootHash; virDomainDriverMergeBlkioDevice; +virDomainDriverNodeDeviceGetPCIInfo; virDomainDriverParseBlkioDeviceStr; virDomainDriverSetupPersistentDefBlkioParams;

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 5bd3614e21..0821d39c9b 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -56,6 +56,7 @@ #include "cpu/cpu.h" #include "virutil.h" #include "domain_validate.h" +#include "domain_driver.h"

#define VIR_FROM_THIS VIR_FROM_LIBXL

@@ -5773,37 +5774,6 @@ libxlConnectSupportsFeature(virConnectPtr conn, int feature) } }

-static int -libxlNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def, - unsigned *domain, - unsigned *bus, - unsigned *slot, - unsigned *function) -{ - virNodeDevCapsDefPtr cap; - - cap = def->caps; - while (cap) { - if (cap->data.type == VIR_NODE_DEV_CAP_PCI_DEV) { - *domain = cap->data.pci_dev.domain; - *bus = cap->data.pci_dev.bus; - *slot = cap->data.pci_dev.slot; - *function = cap->data.pci_dev.function; - break; - } - - cap = cap->next; - } - - if (!cap) { - virReportError(VIR_ERR_INVALID_ARG, - _("device %s is not a PCI device"), def->name); - return -1; - } - - return 0; -} - static int libxlNodeDeviceDetachFlags(virNodeDevicePtr dev, const char *driverName, @@ -5845,7 +5815,7 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev, if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0) goto cleanup;

- if (libxlNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) goto cleanup;

pci = virPCIDeviceNew(domain, bus, slot, function); @@ -5916,7 +5886,7 @@ libxlNodeDeviceReAttach(virNodeDevicePtr dev) if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0) goto cleanup;

- if (libxlNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) goto cleanup;

pci = virPCIDeviceNew(domain, bus, slot, function); @@ -5974,7 +5944,7 @@ libxlNodeDeviceReset(virNodeDevicePtr dev) if (virNodeDeviceResetEnsureACL(dev->conn, def) < 0) goto cleanup;

- if (libxlNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) goto cleanup;

pci = virPCIDeviceNew(domain, bus, slot, function); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 027617deef..0a732a241d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11955,37 +11955,6 @@ qemuDomainMigrateConfirm3Params(virDomainPtr domain, }

-static int -qemuNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def, - unsigned *domain, - unsigned *bus, - unsigned *slot, - unsigned *function) -{ - virNodeDevCapsDefPtr cap; - - cap = def->caps; - while (cap) { - if (cap->data.type == VIR_NODE_DEV_CAP_PCI_DEV) { - *domain = cap->data.pci_dev.domain; - *bus = cap->data.pci_dev.bus; - *slot = cap->data.pci_dev.slot; - *function = cap->data.pci_dev.function; - break; - } - - cap = cap->next; - } - - if (!cap) { - virReportError(VIR_ERR_INVALID_ARG, - _("device %s is not a PCI device"), def->name); - return -1; - } - - return 0; -} - static int qemuNodeDeviceDetachFlags(virNodeDevicePtr dev, const char *driverName, @@ -12028,7 +11997,7 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev, if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0) goto cleanup;

- if (qemuNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) goto cleanup;

pci = virPCIDeviceNew(domain, bus, slot, function); @@ -12109,7 +12078,7 @@ qemuNodeDeviceReAttach(virNodeDevicePtr dev) if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0) goto cleanup;

- if (qemuNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) goto cleanup;

pci = virPCIDeviceNew(domain, bus, slot, function); @@ -12163,7 +12132,7 @@ qemuNodeDeviceReset(virNodeDevicePtr dev) if (virNodeDeviceResetEnsureACL(dev->conn, def) < 0) goto cleanup;

- if (qemuNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) goto cleanup;

pci = virPCIDeviceNew(domain, bus, slot, function);

On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote:

Instead of receiving 4 uints in order and write domain/bus/slot/function, receive a virPCIDeviceAddressPtr instead and write into it.

This change will allow us to simplify the API for virPCIDeviceNew() in the next patch.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/hypervisor/domain_driver.c | 13 +++++-------- src/hypervisor/domain_driver.h | 5 +---- src/libxl/libxl_driver.c | 18 +++++++++--------- src/qemu/qemu_driver.c | 18 +++++++++--------- 4 files changed, 24 insertions(+), 30 deletions(-)

diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index 68dbf10ac6..cdb53bcf3e 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -341,20 +341,17 @@ virDomainDriverSetupPersistentDefBlkioParams(virDomainDefPtr persistentDef,

int virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def, - unsigned *domain, - unsigned *bus, - unsigned *slot, - unsigned *function) + virPCIDeviceAddressPtr devAddr) { virNodeDevCapsDefPtr cap;

cap = def->caps; while (cap) { if (cap->data.type == VIR_NODE_DEV_CAP_PCI_DEV) { - *domain = cap->data.pci_dev.domain; - *bus = cap->data.pci_dev.bus; - *slot = cap->data.pci_dev.slot; - *function = cap->data.pci_dev.function; + devAddr->domain = cap->data.pci_dev.domain; + devAddr->bus = cap->data.pci_dev.bus; + devAddr->slot = cap->data.pci_dev.slot; + devAddr->function = cap->data.pci_dev.function;

Looking at this, my first thought was "surely cap->data.pci_dev should have a single virPCIDeviceAddress rather than the individual members! Then those 4 lines could just be "devAddr = cap->data.pci_dev.address". Except that somewhere along the way someone decided that the virZPCIDeviceAddress should be a subelement of a virPCIDeviceAddress, so there is extra stuff in there that's unwanted in this case (can't place the blame entirely there of course :-) - even before that, there was the extra "multi" setting, which is really an attribute of a PCI slot, not an attribute of a PCI address.) At any rate, what you've done is much better than what was there before, and simplifications/consolidations like this will often lead to identification of other simplifications/consolidations that can be made later. (Oh, and we mustn't forget that passing a single virPCIDeviceAddressPtr instead of 4 int* makes it much less likely that someone will typo the items in the wrong order) Reviewed-by: Laine Stump <laine@redhat.com> I'm really liking these so far! :-)

break; }

diff --git a/src/hypervisor/domain_driver.h b/src/hypervisor/domain_driver.h index 2bb053d559..86b92d0284 100644 --- a/src/hypervisor/domain_driver.h +++ b/src/hypervisor/domain_driver.h @@ -48,7 +48,4 @@ int virDomainDriverSetupPersistentDefBlkioParams(virDomainDefPtr persistentDef, int nparams);

int virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def, - unsigned *domain, - unsigned *bus, - unsigned *slot, - unsigned *function); + virPCIDeviceAddressPtr devAddr); diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 0821d39c9b..360d553a22 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5780,7 +5780,7 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev, unsigned int flags) { virPCIDevicePtr pci = NULL; - unsigned domain = 0, bus = 0, slot = 0, function = 0; + virPCIDeviceAddress devAddr; int ret = -1; virNodeDeviceDefPtr def = NULL; char *xml = NULL; @@ -5815,10 +5815,10 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev, if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0) goto cleanup;

- if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup;

- pci = virPCIDeviceNew(domain, bus, slot, function); + pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); if (!pci) goto cleanup;

@@ -5853,7 +5853,7 @@ static int libxlNodeDeviceReAttach(virNodeDevicePtr dev) { virPCIDevicePtr pci = NULL; - unsigned domain = 0, bus = 0, slot = 0, function = 0; + virPCIDeviceAddress devAddr; int ret = -1; virNodeDeviceDefPtr def = NULL; char *xml = NULL; @@ -5886,10 +5886,10 @@ libxlNodeDeviceReAttach(virNodeDevicePtr dev) if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0) goto cleanup;

- if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup;

- pci = virPCIDeviceNew(domain, bus, slot, function); + pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); if (!pci) goto cleanup;

@@ -5911,7 +5911,7 @@ static int libxlNodeDeviceReset(virNodeDevicePtr dev) { virPCIDevicePtr pci = NULL; - unsigned domain = 0, bus = 0, slot = 0, function = 0; + virPCIDeviceAddress devAddr; int ret = -1; virNodeDeviceDefPtr def = NULL; char *xml = NULL; @@ -5944,10 +5944,10 @@ libxlNodeDeviceReset(virNodeDevicePtr dev) if (virNodeDeviceResetEnsureACL(dev->conn, def) < 0) goto cleanup;

- if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup;

- pci = virPCIDeviceNew(domain, bus, slot, function); + pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); if (!pci) goto cleanup;

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0a732a241d..28781cc34b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11962,7 +11962,7 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev, { virQEMUDriverPtr driver = dev->conn->privateData; virPCIDevicePtr pci = NULL; - unsigned domain = 0, bus = 0, slot = 0, function = 0; + virPCIDeviceAddress devAddr; int ret = -1; virNodeDeviceDefPtr def = NULL; g_autofree char *xml = NULL; @@ -11997,10 +11997,10 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev, if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0) goto cleanup;

- if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup;

- pci = virPCIDeviceNew(domain, bus, slot, function); + pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); if (!pci) goto cleanup;

@@ -12046,7 +12046,7 @@ qemuNodeDeviceReAttach(virNodeDevicePtr dev) { virQEMUDriverPtr driver = dev->conn->privateData; virPCIDevicePtr pci = NULL; - unsigned domain = 0, bus = 0, slot = 0, function = 0; + virPCIDeviceAddress devAddr; int ret = -1; virNodeDeviceDefPtr def = NULL; g_autofree char *xml = NULL; @@ -12078,10 +12078,10 @@ qemuNodeDeviceReAttach(virNodeDevicePtr dev) if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0) goto cleanup;

- if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup;

- pci = virPCIDeviceNew(domain, bus, slot, function); + pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); if (!pci) goto cleanup;

@@ -12100,7 +12100,7 @@ qemuNodeDeviceReset(virNodeDevicePtr dev) { virQEMUDriverPtr driver = dev->conn->privateData; virPCIDevicePtr pci; - unsigned domain = 0, bus = 0, slot = 0, function = 0; + virPCIDeviceAddress devAddr; int ret = -1; virNodeDeviceDefPtr def = NULL; g_autofree char *xml = NULL; @@ -12132,10 +12132,10 @@ qemuNodeDeviceReset(virNodeDevicePtr dev) if (virNodeDeviceResetEnsureACL(dev->conn, def) < 0) goto cleanup;

- if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup;

- pci = virPCIDeviceNew(domain, bus, slot, function); + pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); if (!pci) goto cleanup;

On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote:

The current virPCIDeviceNew() signature, receiving 4 uints in sequence (domain, bus, slot, function), is not neat.

We already have a way to represent a PCI address in virPCIDeviceAddress that is used in the code. Aside from the test files, most of virPCIDeviceNew() callers have access to a virPCIDeviceAddress reference, but then we need to retrieve the 4 required uints (addr.domain, addr.bus, addr.slot, addr.function) to satisfy virPCIDeviceNew(). The result is that we have extra verbosity/boilerplate to retrieve an information that is already available in virPCIDeviceAddress.

A better way is presented by virNVMEDeviceNew(), where the caller just supplies a virPCIDeviceAddress pointer and the function handles the details internally.

This patch changes virPCIDeviceNew() to receive a virPCIDeviceAddress pointer instead of 4 uints.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/hypervisor/virhostdev.c | 3 +-- src/libxl/libxl_driver.c | 6 ++--- src/node_device/node_device_udev.c | 11 +++++--- src/qemu/qemu_domain_address.c | 5 +--- src/qemu/qemu_driver.c | 6 ++--- src/security/security_apparmor.c | 3 +-- src/security/security_dac.c | 6 ++--- src/security/security_selinux.c | 6 ++--- src/security/virt-aa-helper.c | 6 +---- src/util/virnetdev.c | 3 +-- src/util/virnvme.c | 5 +--- src/util/virpci.c | 40 +++++++++--------------------- src/util/virpci.h | 5 +--- tests/virhostdevtest.c | 3 ++- tests/virpcitest.c | 35 +++++++++++++++++++------- 15 files changed, 64 insertions(+), 79 deletions(-)

diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c index be32a26164..bd35397f2c 100644 --- a/src/hypervisor/virhostdev.c +++ b/src/hypervisor/virhostdev.c @@ -235,8 +235,7 @@ virHostdevGetPCIHostDevice(const virDomainHostdevDef *hostdev, hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) return 0;

- actual = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, - pcisrc->addr.slot, pcisrc->addr.function); + actual = virPCIDeviceNew(&pcisrc->addr);

if (!actual) return -1; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 360d553a22..3eaf106006 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5818,7 +5818,7 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev, if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup;

- pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); + pci = virPCIDeviceNew(&devAddr); if (!pci) goto cleanup;

@@ -5889,7 +5889,7 @@ libxlNodeDeviceReAttach(virNodeDevicePtr dev) if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup;

- pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); + pci = virPCIDeviceNew(&devAddr); if (!pci) goto cleanup;

@@ -5947,7 +5947,7 @@ libxlNodeDeviceReset(virNodeDevicePtr dev) if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup;

- pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); + pci = virPCIDeviceNew(&devAddr); if (!pci) goto cleanup;

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 55a2731681..fceb135aa5 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -367,6 +367,7 @@ udevProcessPCI(struct udev_device *device, virNodeDevCapPCIDevPtr pci_dev = &def->caps->data.pci_dev; virPCIEDeviceInfoPtr pci_express = NULL; virPCIDevicePtr pciDev = NULL; + virPCIDeviceAddress devAddr; int ret = -1; char *p; bool privileged; @@ -416,10 +417,12 @@ udevProcessPCI(struct udev_device *device, if (virNodeDeviceGetPCIDynamicCaps(def->sysfs_path, pci_dev) < 0) goto cleanup;

- if (!(pciDev = virPCIDeviceNew(pci_dev->domain, - pci_dev->bus, - pci_dev->slot, - pci_dev->function))) + devAddr.domain = pci_dev->domain; + devAddr.bus = pci_dev->bus; + devAddr.slot = pci_dev->slot; + devAddr.function = pci_dev->function;

Now see - there we are again! If virNodeDevCapPCIDev could just have a virPCIDeviceAddress, then this could have been a 4->1 conversion instead of 4->4. Ah well, tomorrow is another day!

+ + if (!(pciDev = virPCIDeviceNew(&devAddr))) goto cleanup;

/* We need to be root to read PCI device configs */ diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index f0ba318cc8..ae4ad6677c 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -857,10 +857,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, return 0; }

- if (!(pciDev = virPCIDeviceNew(hostAddr->domain, - hostAddr->bus, - hostAddr->slot, - hostAddr->function))) { + if (!(pciDev = virPCIDeviceNew(hostAddr))) { /* libvirt should be able to perform all the * operations in virPCIDeviceNew() even if it's * running unprivileged, so if this fails, the device diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 28781cc34b..7581e3c8cb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12000,7 +12000,7 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev, if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup;

- pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); + pci = virPCIDeviceNew(&devAddr); if (!pci) goto cleanup;

@@ -12081,7 +12081,7 @@ qemuNodeDeviceReAttach(virNodeDevicePtr dev) if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup;

- pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); + pci = virPCIDeviceNew(&devAddr); if (!pci) goto cleanup;

@@ -12135,7 +12135,7 @@ qemuNodeDeviceReset(virNodeDevicePtr dev) if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup;

- pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); + pci = virPCIDeviceNew(&devAddr); if (!pci) goto cleanup;

diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 1b035cce2f..b564866678 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -876,8 +876,7 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr,

case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { virPCIDevicePtr pci = - virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, - pcisrc->addr.slot, pcisrc->addr.function); + virPCIDeviceNew(&pcisrc->addr);

if (!pci) goto done; diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4f4a0a069e..b5e56feaa8 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1268,8 +1268,7 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,

case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { virPCIDevicePtr pci = - virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, - pcisrc->addr.slot, pcisrc->addr.function); + virPCIDeviceNew(&pcisrc->addr);

if (!pci) return -1; @@ -1437,8 +1436,7 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr mgr,

case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { virPCIDevicePtr pci = - virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, - pcisrc->addr.slot, pcisrc->addr.function); + virPCIDeviceNew(&pcisrc->addr);

if (!pci) return -1; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 2fc6ef2616..76edaed027 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2103,8 +2103,7 @@ virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr,

case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { virPCIDevicePtr pci = - virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, - pcisrc->addr.slot, pcisrc->addr.function); + virPCIDeviceNew(&pcisrc->addr);

if (!pci) return -1; @@ -2343,8 +2342,7 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr,

case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { virPCIDevicePtr pci = - virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, - pcisrc->addr.slot, pcisrc->addr.function); + virPCIDeviceNew(&pcisrc->addr);

if (!pci) return -1; diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5a6f4a5f7d..5efdb61e59 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1104,11 +1104,7 @@ get_files(vahControl * ctl) }

case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { - virPCIDevicePtr pci = virPCIDeviceNew( - dev->source.subsys.u.pci.addr.domain, - dev->source.subsys.u.pci.addr.bus, - dev->source.subsys.u.pci.addr.slot, - dev->source.subsys.u.pci.addr.function); + virPCIDevicePtr pci = virPCIDeviceNew(&dev->source.subsys.u.pci.addr);

See! Like this!! Anyway, this is giving me some thoughts about solving it, but in the meantime this patch is good on its own: Reviewed-by: Laine Stump <laine@redhat.com>

virDomainHostdevSubsysPCIBackendType backend = dev->source.subsys.u.pci.backend; if (backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO || diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index a73e5f72f1..acb3ec960c 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1141,8 +1141,7 @@ virNetDevGetPCIDevice(const char *devName) if (!vfPCIAddr) return NULL;

- return virPCIDeviceNew(vfPCIAddr->domain, vfPCIAddr->bus, - vfPCIAddr->slot, vfPCIAddr->function); + return virPCIDeviceNew(vfPCIAddr); } # endif

diff --git a/src/util/virnvme.c b/src/util/virnvme.c index b8179aa431..66b73cd1d1 100644 --- a/src/util/virnvme.c +++ b/src/util/virnvme.c @@ -290,10 +290,7 @@ virNVMeDeviceCreatePCIDevice(const virNVMeDevice *nvme) { g_autoptr(virPCIDevice) pci = NULL;

- if (!(pci = virPCIDeviceNew(nvme->address.domain, - nvme->address.bus, - nvme->address.slot, - nvme->address.function))) + if (!(pci = virPCIDeviceNew(&nvme->address))) return NULL;

/* NVMe devices must be bound to vfio */ diff --git a/src/util/virpci.c b/src/util/virpci.c index 5f50f92abb..5a91553b5f 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -475,24 +475,24 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate, return -1;

while ((ret = virDirRead(dir, &entry, PCI_SYSFS "devices")) > 0) { - unsigned int domain, bus, slot, function; g_autoptr(virPCIDevice) check = NULL; + virPCIDeviceAddress devAddr; char *tmp;

/* expected format: <domain>:<bus>:<slot>.<function> */ if (/* domain */ - virStrToLong_ui(entry->d_name, &tmp, 16, &domain) < 0 || *tmp != ':' || + virStrToLong_ui(entry->d_name, &tmp, 16, &devAddr.domain) < 0 || *tmp != ':' || /* bus */ - virStrToLong_ui(tmp + 1, &tmp, 16, &bus) < 0 || *tmp != ':' || + virStrToLong_ui(tmp + 1, &tmp, 16, &devAddr.bus) < 0 || *tmp != ':' || /* slot */ - virStrToLong_ui(tmp + 1, &tmp, 16, &slot) < 0 || *tmp != '.' || + virStrToLong_ui(tmp + 1, &tmp, 16, &devAddr.slot) < 0 || *tmp != '.' || /* function */ - virStrToLong_ui(tmp + 1, NULL, 16, &function) < 0) { + virStrToLong_ui(tmp + 1, NULL, 16, &devAddr.function) < 0) { VIR_WARN("Unusual entry in " PCI_SYSFS "devices: %s", entry->d_name);

Isn't there a utility function that does this?

continue; }

- check = virPCIDeviceNew(domain, bus, slot, function); + check = virPCIDeviceNew(&devAddr); if (!check) { ret = -1; break; @@ -767,10 +767,7 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void *data) */ if (dev->address.bus > secondary && dev->address.bus <= subordinate) { if (*best == NULL) { - *best = virPCIDeviceNew(check->address.domain, - check->address.bus, - check->address.slot, - check->address.function); + *best = virPCIDeviceNew(&check->address); if (*best == NULL) { ret = -1; goto cleanup; @@ -790,10 +787,7 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void *data)

if (secondary > best_secondary) { virPCIDeviceFree(*best); - *best = virPCIDeviceNew(check->address.domain, - check->address.bus, - check->address.slot, - check->address.function); + *best = virPCIDeviceNew(&check->address); if (*best == NULL) { ret = -1; goto cleanup; @@ -1455,10 +1449,7 @@ virPCIDeviceAddressAsString(const virPCIDeviceAddress *addr) }

virPCIDevicePtr -virPCIDeviceNew(unsigned int domain, - unsigned int bus, - unsigned int slot, - unsigned int function) +virPCIDeviceNew(const virPCIDeviceAddress *address) { g_autoptr(virPCIDevice) dev = NULL; g_autofree char *vendor = NULL; @@ -1466,10 +1457,7 @@ virPCIDeviceNew(unsigned int domain,

dev = g_new0(virPCIDevice, 1);

- dev->address.domain = domain; - dev->address.bus = bus; - dev->address.slot = slot; - dev->address.function = function; + virPCIDeviceAddressCopy(&dev->address, address);

dev->name = virPCIDeviceAddressAsString(&dev->address);

@@ -1896,8 +1884,7 @@ virPCIDeviceGetIOMMUGroupAddOne(virPCIDeviceAddressPtr newDevAddr, void *opaque) virPCIDeviceListPtr groupList = opaque; g_autoptr(virPCIDevice) newDev = NULL;

- if (!(newDev = virPCIDeviceNew(newDevAddr->domain, newDevAddr->bus, - newDevAddr->slot, newDevAddr->function))) + if (!(newDev = virPCIDeviceNew(newDevAddr))) return -1;

if (virPCIDeviceListAdd(groupList, newDev) < 0) @@ -2028,10 +2015,7 @@ virPCIDeviceAddressGetIOMMUGroupDev(const virPCIDeviceAddress *devAddr) { g_autoptr(virPCIDevice) pci = NULL;

- if (!(pci = virPCIDeviceNew(devAddr->domain, - devAddr->bus, - devAddr->slot, - devAddr->function))) + if (!(pci = virPCIDeviceNew(devAddr))) return NULL;

return virPCIDeviceGetIOMMUGroupDev(pci); diff --git a/src/util/virpci.h b/src/util/virpci.h index 43828b0a8a..d4451848c1 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -113,10 +113,7 @@ struct _virPCIEDeviceInfo { virPCIELink *link_sta; /* Actually negotiated capabilities */ };

-virPCIDevicePtr virPCIDeviceNew(unsigned int domain, - unsigned int bus, - unsigned int slot, - unsigned int function); +virPCIDevicePtr virPCIDeviceNew(const virPCIDeviceAddress *address); virPCIDevicePtr virPCIDeviceCopy(virPCIDevicePtr dev); void virPCIDeviceFree(virPCIDevicePtr dev); const char *virPCIDeviceGetName(virPCIDevicePtr dev); diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index 385db0849a..40c14a5281 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -138,7 +138,8 @@ myInit(void) }

for (i = 0; i < nhostdevs; i++) { - if (!(dev[i] = virPCIDeviceNew(0, 0, i + 1, 0))) + virDomainHostdevSubsys subsys = hostdevs[i]->source.subsys; + if (!(dev[i] = virPCIDeviceNew(&subsys.u.pci.addr))) goto cleanup;

virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_VFIO); diff --git a/tests/virpcitest.c b/tests/virpcitest.c index 6f064a3f85..6a4bd5518d 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -60,8 +60,9 @@ testVirPCIDeviceNew(const void *opaque G_GNUC_UNUSED) int ret = -1; virPCIDevicePtr dev; const char *devName; + virPCIDeviceAddress devAddr = {.domain = 0, .bus = 0, .slot = 0, .function = 0};

- if (!(dev = virPCIDeviceNew(0, 0, 0, 0))) + if (!(dev = virPCIDeviceNew(&devAddr))) goto cleanup;

devName = virPCIDeviceGetName(dev); @@ -103,7 +104,9 @@ testVirPCIDeviceDetach(const void *opaque G_GNUC_UNUSED) CHECK_LIST_COUNT(inactiveDevs, 0);

for (i = 0; i < nDev; i++) { - if (!(dev[i] = virPCIDeviceNew(0, 0, i + 1, 0))) + virPCIDeviceAddress devAddr = {.domain = 0, .bus = 0, + .slot = i + 1, .function = 0}; + if (!(dev[i] = virPCIDeviceNew(&devAddr))) goto cleanup;

virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_VFIO); @@ -144,7 +147,9 @@ testVirPCIDeviceReset(const void *opaque G_GNUC_UNUSED) CHECK_LIST_COUNT(inactiveDevs, 0);

for (i = 0; i < nDev; i++) { - if (!(dev[i] = virPCIDeviceNew(0, 0, i + 1, 0))) + virPCIDeviceAddress devAddr = {.domain = 0, .bus = 0, + .slot = i + 1, .function = 0}; + if (!(dev[i] = virPCIDeviceNew(&devAddr))) goto cleanup;

virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_VFIO); @@ -176,7 +181,9 @@ testVirPCIDeviceReattach(const void *opaque G_GNUC_UNUSED) goto cleanup;

for (i = 0; i < nDev; i++) { - if (!(dev[i] = virPCIDeviceNew(0, 0, i + 1, 0))) + virPCIDeviceAddress devAddr = {.domain = 0, .bus = 0, + .slot = i + 1, .function = 0}; + if (!(dev[i] = virPCIDeviceNew(&devAddr))) goto cleanup;

if (virPCIDeviceListAdd(inactiveDevs, dev[i]) < 0) { @@ -222,8 +229,10 @@ testVirPCIDeviceIsAssignable(const void *opaque) const struct testPCIDevData *data = opaque; int ret = -1; virPCIDevicePtr dev; + virPCIDeviceAddress devAddr = {.domain = data->domain, .bus = data->bus, + .slot = data->slot, .function = data->function};

- if (!(dev = virPCIDeviceNew(data->domain, data->bus, data->slot, data->function))) + if (!(dev = virPCIDeviceNew(&devAddr))) return -1;

if (virPCIDeviceIsAssignable(dev, true)) @@ -239,8 +248,10 @@ testVirPCIDeviceDetachSingle(const void *opaque) const struct testPCIDevData *data = opaque; int ret = -1; virPCIDevicePtr dev; + virPCIDeviceAddress devAddr = {.domain = data->domain, .bus = data->bus, + .slot = data->slot, .function = data->function};

- dev = virPCIDeviceNew(data->domain, data->bus, data->slot, data->function); + dev = virPCIDeviceNew(&devAddr); if (!dev) goto cleanup;

@@ -261,8 +272,10 @@ testVirPCIDeviceReattachSingle(const void *opaque) const struct testPCIDevData *data = opaque; int ret = -1; virPCIDevicePtr dev; + virPCIDeviceAddress devAddr = {.domain = data->domain, .bus = data->bus, + .slot = data->slot, .function = data->function};

- dev = virPCIDeviceNew(data->domain, data->bus, data->slot, data->function); + dev = virPCIDeviceNew(&devAddr); if (!dev) goto cleanup;

@@ -285,8 +298,10 @@ testVirPCIDeviceCheckDriverTest(const void *opaque) const struct testPCIDevData *data = opaque; int ret = -1; virPCIDevicePtr dev; + virPCIDeviceAddress devAddr = {.domain = data->domain, .bus = data->bus, + .slot = data->slot, .function = data->function};

- dev = virPCIDeviceNew(data->domain, data->bus, data->slot, data->function); + dev = virPCIDeviceNew(&devAddr); if (!dev) goto cleanup;

@@ -305,8 +320,10 @@ testVirPCIDeviceUnbind(const void *opaque) const struct testPCIDevData *data = opaque; int ret = -1; virPCIDevicePtr dev; + virPCIDeviceAddress devAddr = {.domain = data->domain, .bus = data->bus, + .slot = data->slot, .function = data->function};

- dev = virPCIDeviceNew(data->domain, data->bus, data->slot, data->function); + dev = virPCIDeviceNew(&devAddr); if (!dev) goto cleanup;

Sorry for the delay in answering. I usually get the replies to my patches in my inbox because I usually get my email CC'ed in the reply. On 1/20/21 10:33 PM, Laine Stump wrote:

On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote:

...

Gitlab issue #72 [1] reports that removing SR-IOVs VFs before removing the devices from the running domains can have strange consequences.

Resolution: "Don't do that!!" :-)

Seriously, though, there are any number of things that someone with root access to the host could do that would completely confuse libvirt and break everything. For example, assigning a PF to a guest after having assigned one or more of that PF's VFs to guests.

But I guess there's no harm in trying to mitigate the damage. As long as it doesn't complicate the code to the extent that it becomes more confusing, difficult to maintain, and liable to other regressions.

This was the idea with this series. I'm trying my best to not interfere with working code/logic by adding these mitigations.

...

  QEMU might be able to hotunplug the device inside the guest, but Libvirt will not be aware of that,

If qemu knows enough to unplug the device from the guest, then I'm presuming that it must also be sending a DEVICE_DELETED event up to libvirt. Of course libvirt isn't expecting this, and so probably throws it away, right?

---- long story warning ---- We have 2 scenarios I'm trying to deal with in this work: a SR-IOV disappearing from the host, while a running domain was using one of its functions, in a condition where: 1) the VF was coldplugged in the domain (i.e. it was in the domain XML before starting the domain) 2) the VF was hotplugged in the domain In scenario (2), QEMU will throw a DEVICE_DEL event and we'll capture it accordingly via processDeviceDeleteEvent ->qemuDomainRemoveDevice -> qemuDomainRemoveHostDevice. The problem is,by the time we reach qemuDomainRemoveHostDevice, the VF is already gone from the host and we didn't foresee this as a possibility. All of this is pinned on how we interpret virPCIDeviceNew(). My initial idea for this work was to change virPCIDeviceNew() to allow the object to be created, regardless of whether the device actually exists in the host, and then we would have a 'present' flag to determine if the device is present or not. I got terrified by the idea of changing the semantics of virPCIDeviceNew(), which will return -1 if the device isn't present, for all Libvirt devices that uses the object, just to handle a niche case. I decided then to do the 'device is present' checks you'll see around in this series. As far as scenario (1) goes, I don't fully understand why it happens TBH. For QEMU there is no difference - QEMU will throw a DEVICE_DEL event for a VF that disappears from the host, even if the VF was coldplugged. When launching the VM with Libvirt, QEMU internals aren't aware of kernel intent to remove the device. Looking in the vfio driver in the kernel I learned that this intent is expressed by an eventfd call of vfio_pci_request. My somewhat educated guess is that Libvirt is wrapping up the QEMU process in a way that QEMU never sees this eventfd. The result, as you can see in the bug, is that unless the admin kills the terminal where "echo 0 > num_vfs" was issue, 'echo' will lock for as long as the domain using the VF keeps running. And when the domain is shut down the VF is removed from the process, and the host, and then we have a similar problem in our shutdown path, via qemuProcessStop.

...

  and then the guest is now inconsistent with the domain definition.

There's also the possibility of the VFs removal not succeeding while the domain is running but then, as soon as the domain is shutdown, all the VFs are removed. Libvirt can't handle the removal of the PCI devices while trying to reattach the hostdevs, and the Libvirt daemon can be left in an inconsistent state (see [2]).

This patch starts to address the issue related in Gitlab #72, most notably the issue described in [2]. When shutting down a domain with SR-IOV hostdevs that got missing, virHostdevReAttachPCIDevices() is failing the whole process and failing to reattach all the PCI devices, including the ones that aren't related to the VFs that went missing. Let's make it more resilient with host changes by changing virHostdevGetPCIHostDevice() to return an exclusive error code '-2' for this case. virHostdevGetPCIHostDeviceList() can then tell when virHostdevGetPCIHostDevice() failed to find the PCI device of a hostdev and continue to make the list of PCI devices.

virHostdevReAttachPCIDevices() will now be able to proceed reattaching all other valid PCI devices, at least. The 'ghost hostdevs' will be handled later on.

[1] https://gitlab.com/libvirt/libvirt/-/issues/72 [2] https://gitlab.com/libvirt/libvirt/-/issues/72#note_459032148

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> ---   src/hypervisor/virhostdev.c | 8 ++++++--   1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c index bd35397f2c..dbba36193b 100644 --- a/src/hypervisor/virhostdev.c +++ b/src/hypervisor/virhostdev.c @@ -220,7 +220,8 @@ virHostdevManagerGetDefault(void)    * is returned.    *    * Returns: 0 on success (@pci might be NULL though), - *         -1 otherwise (with error reported). + *         -1 otherwise (with error reported), + *         -2 PCI device not found. @pci will be NULL    */   static int   virHostdevGetPCIHostDevice(const virDomainHostdevDef *hostdev, @@ -235,6 +236,9 @@ virHostdevGetPCIHostDevice(const virDomainHostdevDef *hostdev,           hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)           return 0; +    if (!virPCIDeviceExists(&pcisrc->addr)) +        return -2;

What happens if the device disappeared and then came back before you got here? (i.e. by setting sriov_numvfs = 0, and then sriov_numvfs = 128 (or whatever)?

I'm not sure what would happen. I never though that this could trigger a TOCTOU situation. I **think** that this wouldn't be too nefarious - we would remove the device from the domain definition and the device would be there in the host. Would need to try to simulate this to be sure though. Thanks, DHB

It's too late in the day for my to process this too much, so I'm going to come back to it tomorrow. I will continue on to see if there is other "simple" stuff in subsequent patches though - even if we can't get all of this pushed right away, there's a significant amount that can be pushed.

...

+       actual = virPCIDeviceNew(&pcisrc->addr);       if (!actual) @@ -270,7 +274,7 @@ virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr *hostdevs, int nhostdevs)           virDomainHostdevDefPtr hostdev = hostdevs[i];           g_autoptr(virPCIDevice) pci = NULL; -        if (virHostdevGetPCIHostDevice(hostdev, &pci) < 0) +        if (virHostdevGetPCIHostDevice(hostdev, &pci) == -1)               return NULL;           if (!pci)

On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote:

Add virObjectUnref an autoptr cleanup func for virNodeDevice, then remove all unref and free calls from qemuNodeDeviceReAttach().

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/datatypes.h | 2 ++ src/qemu/qemu_driver.c | 32 ++++++++++++-------------------- 2 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/src/datatypes.h b/src/datatypes.h index ade3779e43..7a88aba0df 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -707,6 +707,8 @@ struct _virNodeDevice { char *parentName; /* parent device name */ };

+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNodeDevice, virObjectUnref);

It may seem like overkill, but I think this ^^^ should be added in a separate patch. That way if some other patch that uses g_autoptr(virNodeDevice) needs to be backported to a downstream release that doesn't want to take the rest of this patch's refactoring of qemuNodeDeviceReAttach(), they can do it. Reviewed-by: Laine Stump <laine@redhat.com> but split the above line into a separate patch (which you can also put my R-b on)

+ /** * _virSecret: * diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7581e3c8cb..f9aa93fa3e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12045,17 +12045,16 @@ static int qemuNodeDeviceReAttach(virNodeDevicePtr dev) { virQEMUDriverPtr driver = dev->conn->privateData; - virPCIDevicePtr pci = NULL; + g_autoptr(virPCIDevice) pci = NULL; virPCIDeviceAddress devAddr; - int ret = -1; - virNodeDeviceDefPtr def = NULL; + g_autoptr(virNodeDeviceDef) def = NULL; g_autofree char *xml = NULL; virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; - virConnectPtr nodeconn = NULL; - virNodeDevicePtr nodedev = NULL; + g_autoptr(virConnect) nodeconn = NULL; + g_autoptr(virNodeDevice) nodedev = NULL;

if (!(nodeconn = virGetConnectNodeDev())) - goto cleanup; + return -1;

/* 'dev' is associated with the QEMU virConnectPtr, * so for split daemons, we need to get a copy that @@ -12063,36 +12062,29 @@ qemuNodeDeviceReAttach(virNodeDevicePtr dev) */ if (!(nodedev = virNodeDeviceLookupByName( nodeconn, virNodeDeviceGetName(dev)))) - goto cleanup; + return -1;

xml = virNodeDeviceGetXMLDesc(nodedev, 0); if (!xml) - goto cleanup; + return -1;

def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); if (!def) - goto cleanup; + return -1;

/* ACL check must happen against original 'dev', * not the new 'nodedev' we acquired */ if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0) - goto cleanup; + return -1;

if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) - goto cleanup; + return -1;

pci = virPCIDeviceNew(&devAddr); if (!pci) - goto cleanup; - - ret = virHostdevPCINodeDeviceReAttach(hostdev_mgr, pci); + return -1;

- virPCIDeviceFree(pci); - cleanup: - virNodeDeviceDefFree(def); - virObjectUnref(nodedev); - virObjectUnref(nodeconn); - return ret; + return virHostdevPCINodeDeviceReAttach(hostdev_mgr, pci); }

static int

On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote:

Use g_auto* wherever we can and remove the 'cleanup' label.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

This patch is also obsoleted by 23cdab6a3de0f6336505adcb446f77a6e0628e6b

--- src/libxl/libxl_driver.c | 37 ++++++++++++++----------------------- 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 3eaf106006..fbb67e9ed6 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5852,18 +5852,17 @@ libxlNodeDeviceDettach(virNodeDevicePtr dev) static int libxlNodeDeviceReAttach(virNodeDevicePtr dev) { - virPCIDevicePtr pci = NULL; + g_autoptr(virPCIDevice) pci = NULL; virPCIDeviceAddress devAddr; - int ret = -1; - virNodeDeviceDefPtr def = NULL; - char *xml = NULL; + g_autoptr(virNodeDeviceDef) def = NULL; + g_autofree char *xml = NULL; libxlDriverPrivatePtr driver = dev->conn->privateData; virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; - virConnectPtr nodeconn = NULL; - virNodeDevicePtr nodedev = NULL; + g_autoptr(virConnect) nodeconn = NULL; + g_autoptr(virNodeDevice) nodedev = NULL;

if (!(nodeconn = virGetConnectNodeDev())) - goto cleanup; + return -1;

/* 'dev' is associated with the QEMU virConnectPtr, * so for split daemons, we need to get a copy that @@ -5871,40 +5870,32 @@ libxlNodeDeviceReAttach(virNodeDevicePtr dev) */ if (!(nodedev = virNodeDeviceLookupByName( nodeconn, virNodeDeviceGetName(dev)))) - goto cleanup; + return -1;

xml = virNodeDeviceGetXMLDesc(nodedev, 0); if (!xml) - goto cleanup; + return -1;

def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); if (!def) - goto cleanup; + return -1;

/* ACL check must happen against original 'dev', * not the new 'nodedev' we acquired */ if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0) - goto cleanup; + return -1;

if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) - goto cleanup; + return -1;

pci = virPCIDeviceNew(&devAddr); if (!pci) - goto cleanup; + return -1;

if (virHostdevPCINodeDeviceReAttach(hostdev_mgr, pci) < 0) - goto cleanup; - - ret = 0; + return -1;

- cleanup: - virPCIDeviceFree(pci); - virNodeDeviceDefFree(def); - virObjectUnref(nodedev); - virObjectUnref(nodeconn); - VIR_FREE(xml); - return ret; + return 0; }

static int

On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote:

There is no need to bother with cgroup tearing down for absent PCI devices, given that their entries in the sysfs are already gone.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_cgroup.c | 10 ++++++++++ 1 file changed, 10 insertions(+)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index f7146a71c9..050df21d87 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -467,6 +467,16 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm, if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) return 0;

+ /* Skip tearing down Cgroup for hostdevs that represents absent + * PCI devices, e.g. SR-IOV virtual functions that were removed from + * the host while the domain was still running. */ + if (virHostdevIsPCIDevice(dev)) { + const virDomainHostdevSubsysPCI *pcisrc = &dev->source.subsys.u.pci; + + if (!virPCIDeviceExists(&pcisrc->addr)) + return 0;

I would have skipped creating the temprorary variable, since it's only used once, but.. eh. Potato, potahtoe. Reviewed-by: Laine Stump <laine@redhat.com>

+ } + if (qemuDomainGetHostdevPath(dev, &path, NULL) < 0) return -1;

On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote:

We're going to need a way to remove a PCI Device from a list without having a valid virPCIDevicePtr, because the device is missing from the host. This means that virPCIDevicesListDel() must operate with a PCI Device address instead.

Turns out that virPCIDevicesListDel() and its related functions only use the virPCIDeviceAddressPtr of the virPCIDevicePtr, so this change is simple to do and will not cause hassle in all other callers. Let's start adapting virPCIDeviceListFindIndex() and crawl our way up to virPCIDevicesListDel().

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: Laine Stump <laine@redhat.com> (also Declared-should-have-been-done-this-way-in-the-first-place-by: Laine Stump <laine@redhat.com> :-))

--- src/util/virpci.c | 15 ++++++++------- src/util/virpci.h | 2 +- 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/src/util/virpci.c b/src/util/virpci.c index 7143380348..1554acffb6 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1745,7 +1745,7 @@ virPCIDevicePtr virPCIDeviceListSteal(virPCIDeviceListPtr list, virPCIDevicePtr dev) { - return virPCIDeviceListStealIndex(list, virPCIDeviceListFindIndex(list, dev)); + return virPCIDeviceListStealIndex(list, virPCIDeviceListFindIndex(list, &dev->address)); }

void @@ -1756,16 +1756,17 @@ virPCIDeviceListDel(virPCIDeviceListPtr list, }

int -virPCIDeviceListFindIndex(virPCIDeviceListPtr list, virPCIDevicePtr dev) +virPCIDeviceListFindIndex(virPCIDeviceListPtr list, + virPCIDeviceAddressPtr devAddr) { size_t i;

for (i = 0; i < list->count; i++) { virPCIDevicePtr other = list->devs[i]; - if (other->address.domain == dev->address.domain && - other->address.bus == dev->address.bus && - other->address.slot == dev->address.slot && - other->address.function == dev->address.function) + if (other->address.domain == devAddr->domain && + other->address.bus == devAddr->bus && + other->address.slot == devAddr->slot && + other->address.function == devAddr->function) return i; } return -1; @@ -1798,7 +1799,7 @@ virPCIDeviceListFind(virPCIDeviceListPtr list, virPCIDevicePtr dev) { int idx;

- if ((idx = virPCIDeviceListFindIndex(list, dev)) >= 0) + if ((idx = virPCIDeviceListFindIndex(list, &dev->address)) >= 0) return list->devs[idx]; else return NULL; diff --git a/src/util/virpci.h b/src/util/virpci.h index a9c597a428..8c6776da21 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -175,7 +175,7 @@ virPCIDeviceListFindByIDs(virPCIDeviceListPtr list, unsigned int slot, unsigned int function); int virPCIDeviceListFindIndex(virPCIDeviceListPtr list, - virPCIDevicePtr dev); + virPCIDeviceAddressPtr devAddr);

/* * Callback that will be invoked once for each file

On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote:

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/hypervisor/virhostdev.c | 12 ++++++++---- src/util/virpci.c | 16 ++++++++-------- src/util/virpci.h | 2 +- 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c index 402d7be42d..3bfb04c674 100644 --- a/src/hypervisor/virhostdev.c +++ b/src/hypervisor/virhostdev.c @@ -657,7 +657,8 @@ virHostdevReattachAllPCIDevices(virHostdevManagerPtr mgr,

/* We need to look up the actual device because that's what * virPCIDeviceReattach() expects as its argument */ - if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) + if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, + virPCIDeviceGetAddress(pci)))) continue;

if (virPCIDeviceGetManaged(actual)) { @@ -777,7 +778,8 @@ virHostdevPreparePCIDevicesImpl(virHostdevManagerPtr mgr,

/* Unmanaged devices should already have been marked as * inactive: if that's the case, we can simply move on */ - if (virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)) { + if (virPCIDeviceListFind(mgr->inactivePCIHostdevs, + virPCIDeviceGetAddress(pci))) {

Wondered at first why you were using a access function to get the pointer rather than just referencing it directly. I'd forgotten that virPCIDevice is one of those rare "private structures" in libvirt - defined only in a single .c file. Anyway, Reviewed-by: Laine Stump <laine@redhat.com>

VIR_DEBUG("Not detaching unmanaged PCI device %s", virPCIDeviceGetName(pci)); continue; @@ -860,7 +862,8 @@ virHostdevPreparePCIDevicesImpl(virHostdevManagerPtr mgr, * there because 'pci' only contain address information and will * be released at the end of the function */ pci = virPCIDeviceListGet(pcidevs, i); - actual = virPCIDeviceListFind(mgr->activePCIHostdevs, pci); + actual = virPCIDeviceListFind(mgr->activePCIHostdevs, + virPCIDeviceGetAddress(pci));

VIR_DEBUG("Setting driver and domain information for PCI device %s", virPCIDeviceGetName(pci)); @@ -992,7 +995,8 @@ virHostdevReAttachPCIDevicesImpl(virHostdevManagerPtr mgr, * information such as by which domain and driver it is used. As a * side effect, by looking it up we can also tell whether it was * really active in the first place */ - actual = virPCIDeviceListFind(mgr->activePCIHostdevs, pci); + actual = virPCIDeviceListFind(mgr->activePCIHostdevs, + virPCIDeviceGetAddress(pci)); if (actual) { const char *actual_drvname; const char *actual_domname; diff --git a/src/util/virpci.c b/src/util/virpci.c index 1554acffb6..9544275c31 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -705,7 +705,7 @@ virPCIDeviceSharesBusWithActive(virPCIDevicePtr dev, virPCIDevicePtr check, void return 0;

/* same bus, but inactive, i.e. about to be assigned to guest */ - if (inactiveDevs && virPCIDeviceListFind(inactiveDevs, check)) + if (inactiveDevs && virPCIDeviceListFind(inactiveDevs, &check->address)) return 0;

return 1; @@ -1022,7 +1022,7 @@ virPCIDeviceReset(virPCIDevicePtr dev, return -1; }

- if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) { + if (activeDevs && virPCIDeviceListFind(activeDevs, &dev->address)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Not resetting active device %s"), dev->name); return -1; @@ -1294,7 +1294,7 @@ virPCIDeviceDetach(virPCIDevicePtr dev, if (virPCIProbeStubDriver(dev->stubDriver) < 0) return -1;

- if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) { + if (activeDevs && virPCIDeviceListFind(activeDevs, &dev->address)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Not detaching active device %s"), dev->name); return -1; @@ -1306,7 +1306,7 @@ virPCIDeviceDetach(virPCIDevicePtr dev, /* Add *a copy of* the dev into list inactiveDevs, if * it's not already there. */ - if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev)) { + if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, &dev->address)) { VIR_DEBUG("Adding PCI device %s to inactive list", dev->name); if (virPCIDeviceListAddCopy(inactiveDevs, dev) < 0) return -1; @@ -1324,7 +1324,7 @@ virPCIDeviceReattach(virPCIDevicePtr dev, virPCIDeviceListPtr activeDevs, virPCIDeviceListPtr inactiveDevs) { - if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) { + if (activeDevs && virPCIDeviceListFind(activeDevs, &dev->address)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Not reattaching active device %s"), dev->name); return -1; @@ -1684,7 +1684,7 @@ int virPCIDeviceListAdd(virPCIDeviceListPtr list, virPCIDevicePtr dev) { - if (virPCIDeviceListFind(list, dev)) { + if (virPCIDeviceListFind(list, &dev->address)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Device %s is already in use"), dev->name); return -1; @@ -1795,11 +1795,11 @@ virPCIDeviceListFindByIDs(virPCIDeviceListPtr list,

virPCIDevicePtr -virPCIDeviceListFind(virPCIDeviceListPtr list, virPCIDevicePtr dev) +virPCIDeviceListFind(virPCIDeviceListPtr list, virPCIDeviceAddressPtr devAddr) { int idx;

- if ((idx = virPCIDeviceListFindIndex(list, &dev->address)) >= 0) + if ((idx = virPCIDeviceListFindIndex(list, devAddr)) >= 0) return list->devs[idx]; else return NULL; diff --git a/src/util/virpci.h b/src/util/virpci.h index 8c6776da21..628a293972 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -167,7 +167,7 @@ virPCIDevicePtr virPCIDeviceListStealIndex(virPCIDeviceListPtr list, void virPCIDeviceListDel(virPCIDeviceListPtr list, virPCIDevicePtr dev); virPCIDevicePtr virPCIDeviceListFind(virPCIDeviceListPtr list, - virPCIDevicePtr dev); + virPCIDeviceAddressPtr devAddr); virPCIDevicePtr virPCIDeviceListFindByIDs(virPCIDeviceListPtr list, unsigned int domain,

On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote:

virHostdevReAttachPCIDevices() is called when we want to re-attach a list of hostdevs back to the host, either on the shutdown path or via a 'virsh detach-device' call. This function always count on the existence of the device in the host to work, but this can lead to problems. For example, a SR-IOV device can be removed via an admin "echo 0 > /sys/bus/pci/devices/<addr>/sriov_numvfs", making the kernel fire up and eventfd_signal() to the process, asking for the process to release the device. The result might vary depending on the device driver and OS/arch, but two possible outcomes are:

1) the hypervisor driver will detach the device from the VM, issuing a delete event to Libvirt. This can be observed in QEMU;

2) the 'echo 0 > ...' will hang waiting for the device to be unplugged. This means that the VM process failed/refused to release the hostdev back to the host, and the hostdev will be detached during VM shutdown.

Today we don't behave well for both cases. We'll fail to remove the PCI device reference from mgr->activePCIHostdevs and mgr->inactivePCIHostdevs because we rely on the existence of the PCI device conf file in the sysfs. Attempting to re-utilize the same device (assuming it is now present back in the host) can result in an error like this:

$ ./run tools/virsh start vm1-sriov --console error: Failed to start domain vm1-sriov error: Requested operation is not valid: PCI device 0000:01:00.2 is in use by driver QEMU, domain vm1-sriov

For (1), a VM destroy/start cycle is needed to re-use the VF in the guest. For (2), the effect is more nefarious, requiring a Libvirtd daemon restart to use the VF again in any guest.

We can make it a bit better by checking, during virHostdevReAttachPCIDevices(), if there is any missing PCI device that will be left behind in activePCIHostdevs and inactivePCIHostdevs lists. Remove any missing device found from both lists, unconditionally, matching the current state of the host. This change affects the code path in (1) (processDeviceDeletedEvent into qemuDomainRemoveDevice, all the way back to qemuHostdevReAttachPCIDevices) and also in (b) (qemuProcessStop into qemuHostdevReAttachDomainDevices).

NB: Although this patch enables the possibility of 'outside Libvirt' SR-IOV hotunplug of PCI devices, if the hypervisor and the PCI driver copes with it, our goal is to mitigate what it is still considered an user oopsie. For all''

This is one of those odd cases where you use "a" before a word starting with a vowel rather than "an". It's that way because the *sound* starting the next word is "you" rather than "ooh".

supported purposes, the admin must remove the SR-IOV VFs from all running domains before removing the VFs from the host.

Resolves: https://gitlab.com/libvirt/libvirt/-/issues/72 Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/hypervisor/virhostdev.c | 38 +++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)

diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c index c708791eec..ed43733e71 100644 --- a/src/hypervisor/virhostdev.c +++ b/src/hypervisor/virhostdev.c @@ -1077,6 +1077,40 @@ virHostdevReAttachPCIDevicesImpl(virHostdevManagerPtr mgr, }

+static void +virHostdevDeleteMissingPCIDevices(virHostdevManagerPtr mgr, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs) +{ + size_t i; + + if (nhostdevs == 0) + return; + + virObjectLock(mgr->activePCIHostdevs); + virObjectLock(mgr->inactivePCIHostdevs); + + for (i = 0; i < nhostdevs; i++) { + virDomainHostdevDef *hostdev = hostdevs[i]; + virDomainHostdevSubsysPCI *pcisrc = &hostdev->source.subsys.u.pci; + g_autoptr(virPCIDevice) pci = NULL; + + if (virHostdevGetPCIHostDevice(hostdev, &pci) != -2) + continue; + + /* The PCI device from 'hostdev' does not exist in the host + * anymore. Delete it from both active and inactive lists to + * reflect the current host state. + */ + virPCIDeviceListDel(mgr->activePCIHostdevs, &pcisrc->addr); + virPCIDeviceListDel(mgr->inactivePCIHostdevs, &pcisrc->addr); + } + + virObjectUnlock(mgr->activePCIHostdevs); + virObjectUnlock(mgr->inactivePCIHostdevs);

It makes me nervous that you're unlocking these lists in the same order that you lock them, since that could theoretically lead to a deadlock. I haven't even thought beyond this single function to figure out if it's even a possibility in this case, but still would feel better if you unlocked in the opposite order that you lock. With that fixed: Reviewed-by: Laine Stump <laine@redhat.com>

+} + + /* @oldStateDir: * For upgrade purpose: see virHostdevRestoreNetConfig */ @@ -1102,6 +1136,10 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,

virHostdevReAttachPCIDevicesImpl(mgr, drv_name, dom_name, pcidevs, hostdevs, nhostdevs, oldStateDir); + + /* Handle the case where PCI devices from the host went missing + * during the domain lifetime */ + virHostdevDeleteMissingPCIDevices(mgr, hostdevs, nhostdevs); }