[libvirt] [PATCH v1 0/3] misc virhostdevs cleanups
These are cleanups that I made together with an attempt to enable parcial PCI Multifunction assignment with managed=true. That work will be scrapped after discussions with Laine in [1], but these cleanups kind of make sense on their own, so here they are. [1] https://www.redhat.com/archives/libvir-list/2019-July/msg01175.html Daniel Henrique Barboza (3): virhostdev: introduce virHostdevResetAllPCIDevices virhostdev: introduce virHostdevReattachAllPCIDevices virhostdev: remove virHostdevReattachPCIDevice src/util/virhostdev.c | 151 +++++++++++++++++------------------------- src/util/virpci.c | 29 +++++++- 2 files changed, 86 insertions(+), 94 deletions(-) -- 2.21.0
This code that executes virPCIDeviceReset in all virPCIDevicePtr objects of a given virPCIDeviceListPtr list is replicated twice in the code. Putting it in a helper function helps with readability. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/util/virhostdev.c | 54 +++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index a3647a6cf4..7cb0beb545 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -613,6 +613,32 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev, } } +static int +virHostdevResetAllPCIDevices(virPCIDeviceListPtr pcidevs, + virHostdevManagerPtr mgr) +{ + int ret = 0; + size_t i; + + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + + /* We can avoid looking up the actual device here, because performing + * a PCI reset on a device doesn't require any information other than + * the address, which 'pci' already contains */ + VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci)); + if (virPCIDeviceReset(pci, mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) { + VIR_ERROR(_("Failed to reset PCI device: %s"), + virGetLastErrorMessage()); + virResetLastError(); + ret = -1; + } + } + + return ret; +} + int virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, const char *drv_name, @@ -765,17 +791,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, /* Step 3: Now that all the PCI hostdevs have been detached, we * can safely reset them */ - for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); - - /* We can avoid looking up the actual device here, because performing - * a PCI reset on a device doesn't require any information other than - * the address, which 'pci' already contains */ - VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci)); - if (virPCIDeviceReset(pci, mgr->activePCIHostdevs, - mgr->inactivePCIHostdevs) < 0) - goto reattachdevs; - } + if (virHostdevResetAllPCIDevices(pcidevs, mgr) < 0) + goto reattachdevs; /* Step 4: For SRIOV network devices, Now that we have detached the * the network device, set the new netdev config */ @@ -1046,20 +1063,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, } /* Step 4: perform a PCI Reset on all devices */ - for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); - - /* We can avoid looking up the actual device here, because performing - * a PCI reset on a device doesn't require any information other than - * the address, which 'pci' already contains */ - VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci)); - if (virPCIDeviceReset(pci, mgr->activePCIHostdevs, - mgr->inactivePCIHostdevs) < 0) { - VIR_ERROR(_("Failed to reset PCI device: %s"), - virGetLastErrorMessage()); - virResetLastError(); - } - } + virHostdevResetAllPCIDevices(pcidevs, mgr); /* Step 5: Reattach managed devices to their host drivers; unmanaged * devices don't need to be processed further */ -- 2.21.0
On 7/18/19 10:10 PM, Daniel Henrique Barboza wrote:
This code that executes virPCIDeviceReset in all virPCIDevicePtr objects of a given virPCIDeviceListPtr list is replicated twice in the code. Putting it in a helper function helps with readability.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/util/virhostdev.c | 54 +++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 25 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index a3647a6cf4..7cb0beb545 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -613,6 +613,32 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev, } }
+static int +virHostdevResetAllPCIDevices(virPCIDeviceListPtr pcidevs, + virHostdevManagerPtr mgr)
Small nit pick here, I prefer arguments appearing in their order of importance. That means, virHostdevManager is more important than the virPCIDeviceList, because the manager is built on the top of virpci module.
+{ + int ret = 0; + size_t i; + + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + + /* We can avoid looking up the actual device here, because performing + * a PCI reset on a device doesn't require any information other than + * the address, which 'pci' already contains */ + VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci)); + if (virPCIDeviceReset(pci, mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) { + VIR_ERROR(_("Failed to reset PCI device: %s"), + virGetLastErrorMessage()); + virResetLastError(); + ret = -1; + } + } + + return ret; +} +
Michal
There are two places in virhostdev that executes a re-attach operation in all pci devices of a virPCIDeviceListPtr array: virHostdevPreparePCIDevices and virHostdevReAttachPCIDevices. The difference is that the code inside virHostdevPreparePCIDevices uses virPCIDeviceReattach(), while inside virHostdevReAttachPCIDevices virHostdevReattachPCIDevice() is used. Both are called in the same manner inside a loop. To make the code easier to follow and to standardize it further, a new virHostdevReattachAllPCIDevices() helper function is created, which is now called in both places. virHostdevReattachPCIDevice() was chosen as re-attach function because it is an improved version of the raw virPCIDeviceReattach(), including a timer to wait for device cleanup in case of pci_stub_kvm. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/util/virhostdev.c | 114 +++++++++++++++++++----------------------- 1 file changed, 52 insertions(+), 62 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 7cb0beb545..53aacb59b4 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -613,6 +613,56 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev, } } +/* + * Pre-condition: inactivePCIHostdevs & activePCIHostdevs + * are locked + */ +static void +virHostdevReattachPCIDevice(virHostdevManagerPtr mgr, + virPCIDevicePtr actual) +{ + /* Wait for device cleanup if it is qemu/kvm */ + if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) { + int retries = 100; + while (virPCIDeviceWaitForCleanup(actual, "kvm_assigned_device") + && retries) { + usleep(100*1000); + retries--; + } + } + + VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(actual)); + if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) { + VIR_ERROR(_("Failed to re-attach PCI device: %s"), + virGetLastErrorMessage()); + virResetLastError(); + } +} + +static void +virHostdevReattachAllPCIDevices(virPCIDeviceListPtr pcidevs, + virHostdevManagerPtr mgr) +{ + size_t i; + + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual; + + /* We need to look up the actual device because that's what + * virHostdevReattachPCIDevice() expects as its argument */ + if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) + continue; + + if (virPCIDeviceGetManaged(actual)) + virHostdevReattachPCIDevice(mgr, actual); + else + VIR_DEBUG("Not reattaching unmanaged PCI device %s", + virPCIDeviceGetName(actual)); + } +} + static int virHostdevResetAllPCIDevices(virPCIDeviceListPtr pcidevs, virHostdevManagerPtr mgr) @@ -899,26 +949,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, } reattachdevs: - for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); - virPCIDevicePtr actual; - - /* We need to look up the actual device because that's what - * virPCIDeviceReattach() expects as its argument */ - if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) - continue; - - if (virPCIDeviceGetManaged(actual)) { - VIR_DEBUG("Reattaching managed PCI device %s", - virPCIDeviceGetName(pci)); - ignore_value(virPCIDeviceReattach(actual, - mgr->activePCIHostdevs, - mgr->inactivePCIHostdevs)); - } else { - VIR_DEBUG("Not reattaching unmanaged PCI device %s", - virPCIDeviceGetName(pci)); - } - } + virHostdevReattachAllPCIDevices(pcidevs, mgr); cleanup: virObjectUnlock(mgr->activePCIHostdevs); @@ -927,33 +958,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, return ret; } -/* - * Pre-condition: inactivePCIHostdevs & activePCIHostdevs - * are locked - */ -static void -virHostdevReattachPCIDevice(virHostdevManagerPtr mgr, - virPCIDevicePtr actual) -{ - /* Wait for device cleanup if it is qemu/kvm */ - if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) { - int retries = 100; - while (virPCIDeviceWaitForCleanup(actual, "kvm_assigned_device") - && retries) { - usleep(100*1000); - retries--; - } - } - - VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(actual)); - if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs, - mgr->inactivePCIHostdevs) < 0) { - VIR_ERROR(_("Failed to re-attach PCI device: %s"), - virGetLastErrorMessage()); - virResetLastError(); - } -} - /* @oldStateDir: * For upgrade purpose: see virHostdevRestoreNetConfig */ @@ -1067,21 +1071,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, /* Step 5: Reattach managed devices to their host drivers; unmanaged * devices don't need to be processed further */ - for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); - virPCIDevicePtr actual; - - /* We need to look up the actual device because that's what - * virHostdevReattachPCIDevice() expects as its argument */ - if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) - continue; - - if (virPCIDeviceGetManaged(actual)) - virHostdevReattachPCIDevice(mgr, actual); - else - VIR_DEBUG("Not reattaching unmanaged PCI device %s", - virPCIDeviceGetName(actual)); - } + virHostdevReattachAllPCIDevices(pcidevs, mgr); virObjectUnlock(mgr->activePCIHostdevs); virObjectUnlock(mgr->inactivePCIHostdevs); -- 2.21.0
On 7/18/19 10:10 PM, Daniel Henrique Barboza wrote:
There are two places in virhostdev that executes a re-attach operation in all pci devices of a virPCIDeviceListPtr array: virHostdevPreparePCIDevices and virHostdevReAttachPCIDevices. The difference is that the code inside virHostdevPreparePCIDevices uses virPCIDeviceReattach(), while inside virHostdevReAttachPCIDevices virHostdevReattachPCIDevice() is used. Both are called in the same manner inside a loop.
To make the code easier to follow and to standardize it further, a new virHostdevReattachAllPCIDevices() helper function is created, which is now called in both places. virHostdevReattachPCIDevice() was chosen as re-attach function because it is an improved version of the raw virPCIDeviceReattach(), including a timer to wait for device cleanup in case of pci_stub_kvm.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/util/virhostdev.c | 114 +++++++++++++++++++----------------------- 1 file changed, 52 insertions(+), 62 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 7cb0beb545..53aacb59b4 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -613,6 +613,56 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev, } }
+/* + * Pre-condition: inactivePCIHostdevs & activePCIHostdevs + * are locked + */ +static void +virHostdevReattachPCIDevice(virHostdevManagerPtr mgr, + virPCIDevicePtr actual) +{ + /* Wait for device cleanup if it is qemu/kvm */ + if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) { + int retries = 100; + while (virPCIDeviceWaitForCleanup(actual, "kvm_assigned_device") + && retries) { + usleep(100*1000); + retries--; + } + } + + VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(actual)); + if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) { + VIR_ERROR(_("Failed to re-attach PCI device: %s"), + virGetLastErrorMessage()); + virResetLastError(); + } +}
What good is there in introducing this new function if you're removing in the very next patch?
+ +static void +virHostdevReattachAllPCIDevices(virPCIDeviceListPtr pcidevs, + virHostdevManagerPtr mgr) +{ + size_t i; + + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual; + + /* We need to look up the actual device because that's what + * virHostdevReattachPCIDevice() expects as its argument */ + if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) + continue; + + if (virPCIDeviceGetManaged(actual)) + virHostdevReattachPCIDevice(mgr, actual);
Just move its internals here. I mean, even from the code you're moving we would be calling virPCIDeviceReattach().
+ else + VIR_DEBUG("Not reattaching unmanaged PCI device %s", + virPCIDeviceGetName(actual)); + } +} +
Michal
On 7/19/19 8:52 AM, Michal Privoznik wrote:
On 7/18/19 10:10 PM, Daniel Henrique Barboza wrote:
There are two places in virhostdev that executes a re-attach operation in all pci devices of a virPCIDeviceListPtr array: virHostdevPreparePCIDevices and virHostdevReAttachPCIDevices. The difference is that the code inside virHostdevPreparePCIDevices uses virPCIDeviceReattach(), while inside virHostdevReAttachPCIDevices virHostdevReattachPCIDevice() is used. Both are called in the same manner inside a loop.
To make the code easier to follow and to standardize it further, a new virHostdevReattachAllPCIDevices() helper function is created, which is now called in both places. virHostdevReattachPCIDevice() was chosen as re-attach function because it is an improved version of the raw virPCIDeviceReattach(), including a timer to wait for device cleanup in case of pci_stub_kvm.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/util/virhostdev.c | 114 +++++++++++++++++++----------------------- 1 file changed, 52 insertions(+), 62 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 7cb0beb545..53aacb59b4 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -613,6 +613,56 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev, } } +/* + * Pre-condition: inactivePCIHostdevs & activePCIHostdevs + * are locked + */ +static void +virHostdevReattachPCIDevice(virHostdevManagerPtr mgr, + virPCIDevicePtr actual) +{ + /* Wait for device cleanup if it is qemu/kvm */ + if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) { + int retries = 100; + while (virPCIDeviceWaitForCleanup(actual, "kvm_assigned_device") + && retries) { + usleep(100*1000); + retries--; + } + } + + VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(actual)); + if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) { + VIR_ERROR(_("Failed to re-attach PCI device: %s"), + virGetLastErrorMessage()); + virResetLastError(); + } +}
What good is there in introducing this new function if you're removing in the very next patch?
Yep, I'll probably merge this up with patch 3 in a re-spin.
+ +static void +virHostdevReattachAllPCIDevices(virPCIDeviceListPtr pcidevs, + virHostdevManagerPtr mgr) +{ + size_t i; + + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual; + + /* We need to look up the actual device because that's what + * virHostdevReattachPCIDevice() expects as its argument */ + if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) + continue; + + if (virPCIDeviceGetManaged(actual)) + virHostdevReattachPCIDevice(mgr, actual);
Just move its internals here. I mean, even from the code you're moving we would be calling virPCIDeviceReattach().
+ else + VIR_DEBUG("Not reattaching unmanaged PCI device %s", + virPCIDeviceGetName(actual)); + } +} +
Michal
We have 3 pieces of code that do slightly the same thing, but it varies depending on where it is called: - virPCIDeviceReattach(). This is where the actual re-attach work happens; - virHostdevReattachPCIDevice(). This is a static function from virhostdev.c that calls virPCIDeviceReattach() after waiting for device cleanup for the KVM PCI stub driver; - virHostdevPCINodeDeviceReAttach(). This function also calls virPCIDeviceReattach(), but setting some device properties beforehand. All these extra operations that are done before virPCIDeviceReattach() can be moved inside the function itself, allowing for the same re-attach behavior everywhere. This patch consolidates all these pre-conditions inside the body of virPCIDeviceReattach(). With that, virHostdevReattachPCIDevice() can be removed and the callers can use virPCIDeviceReattach() directly instead. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/util/virhostdev.c | 43 +++++++++---------------------------------- src/util/virpci.c | 29 ++++++++++++++++++++++++++--- 2 files changed, 35 insertions(+), 37 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 53aacb59b4..23be037a39 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -613,33 +613,6 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev, } } -/* - * Pre-condition: inactivePCIHostdevs & activePCIHostdevs - * are locked - */ -static void -virHostdevReattachPCIDevice(virHostdevManagerPtr mgr, - virPCIDevicePtr actual) -{ - /* Wait for device cleanup if it is qemu/kvm */ - if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) { - int retries = 100; - while (virPCIDeviceWaitForCleanup(actual, "kvm_assigned_device") - && retries) { - usleep(100*1000); - retries--; - } - } - - VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(actual)); - if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs, - mgr->inactivePCIHostdevs) < 0) { - VIR_ERROR(_("Failed to re-attach PCI device: %s"), - virGetLastErrorMessage()); - virResetLastError(); - } -} - static void virHostdevReattachAllPCIDevices(virPCIDeviceListPtr pcidevs, virHostdevManagerPtr mgr) @@ -655,11 +628,17 @@ virHostdevReattachAllPCIDevices(virPCIDeviceListPtr pcidevs, if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) continue; - if (virPCIDeviceGetManaged(actual)) - virHostdevReattachPCIDevice(mgr, actual); - else + if (virPCIDeviceGetManaged(actual)) { + if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) { + VIR_ERROR(_("Failed to re-attach PCI device: %s"), + virGetLastErrorMessage()); + virResetLastError(); + } + } else { VIR_DEBUG("Not reattaching unmanaged PCI device %s", virPCIDeviceGetName(actual)); + } } } @@ -2050,10 +2029,6 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr mgr, if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data)) goto cleanup; - virPCIDeviceSetUnbindFromStub(pci, true); - virPCIDeviceSetRemoveSlot(pci, true); - virPCIDeviceSetReprobe(pci, true); - if (virPCIDeviceReattach(pci, mgr->activePCIHostdevs, mgr->inactivePCIHostdevs) < 0) goto cleanup; diff --git a/src/util/virpci.c b/src/util/virpci.c index 75e8daadd5..4594643d3c 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1509,19 +1509,39 @@ virPCIDeviceDetach(virPCIDevicePtr dev, return 0; } +/* + * Pre-condition: inactivePCIHostdevs & activePCIHostdevs + * are locked + */ int virPCIDeviceReattach(virPCIDevicePtr dev, virPCIDeviceListPtr activeDevs, virPCIDeviceListPtr inactiveDevs) { + int ret = -1; + if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Not reattaching active device %s"), dev->name); - return -1; + goto exit; + } + + virPCIDeviceSetUnbindFromStub(dev, true); + virPCIDeviceSetRemoveSlot(dev, true); + virPCIDeviceSetReprobe(dev, true); + + /* Wait for device cleanup if it is qemu/kvm */ + if (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_KVM) { + int retries = 100; + while (virPCIDeviceWaitForCleanup(dev, "kvm_assigned_device") + && retries) { + usleep(100*1000); + retries--; + } } if (virPCIDeviceUnbindFromStub(dev) < 0) - return -1; + goto exit; /* Steal the dev from list inactiveDevs */ if (inactiveDevs) { @@ -1529,7 +1549,10 @@ virPCIDeviceReattach(virPCIDevicePtr dev, virPCIDeviceListDel(inactiveDevs, dev); } - return 0; + ret = 0; + + exit: + return ret; } /* Certain hypervisors (like qemu/kvm) map the PCI bar(s) on -- 2.21.0
On 7/18/19 10:10 PM, Daniel Henrique Barboza wrote:
We have 3 pieces of code that do slightly the same thing, but it varies depending on where it is called:
- virPCIDeviceReattach(). This is where the actual re-attach work happens;
- virHostdevReattachPCIDevice(). This is a static function from virhostdev.c that calls virPCIDeviceReattach() after waiting for device cleanup for the KVM PCI stub driver;
- virHostdevPCINodeDeviceReAttach(). This function also calls virPCIDeviceReattach(), but setting some device properties beforehand.
All these extra operations that are done before virPCIDeviceReattach() can be moved inside the function itself, allowing for the same re-attach behavior everywhere.
This patch consolidates all these pre-conditions inside the body of virPCIDeviceReattach(). With that, virHostdevReattachPCIDevice() can be removed and the callers can use virPCIDeviceReattach() directly instead.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/util/virhostdev.c | 43 +++++++++---------------------------------- src/util/virpci.c | 29 ++++++++++++++++++++++++++--- 2 files changed, 35 insertions(+), 37 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 53aacb59b4..23be037a39 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -613,33 +613,6 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev, } }
-/* - * Pre-condition: inactivePCIHostdevs & activePCIHostdevs - * are locked - */ -static void -virHostdevReattachPCIDevice(virHostdevManagerPtr mgr, - virPCIDevicePtr actual) -{ - /* Wait for device cleanup if it is qemu/kvm */ - if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) { - int retries = 100; - while (virPCIDeviceWaitForCleanup(actual, "kvm_assigned_device") - && retries) { - usleep(100*1000); - retries--; - } - } - - VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(actual)); - if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs, - mgr->inactivePCIHostdevs) < 0) { - VIR_ERROR(_("Failed to re-attach PCI device: %s"), - virGetLastErrorMessage()); - virResetLastError(); - } -} - static void virHostdevReattachAllPCIDevices(virPCIDeviceListPtr pcidevs, virHostdevManagerPtr mgr) @@ -655,11 +628,17 @@ virHostdevReattachAllPCIDevices(virPCIDeviceListPtr pcidevs, if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) continue;
- if (virPCIDeviceGetManaged(actual)) - virHostdevReattachPCIDevice(mgr, actual); - else + if (virPCIDeviceGetManaged(actual)) { + if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) { + VIR_ERROR(_("Failed to re-attach PCI device: %s"), + virGetLastErrorMessage()); + virResetLastError(); + } + } else { VIR_DEBUG("Not reattaching unmanaged PCI device %s", virPCIDeviceGetName(actual)); + } } }
@@ -2050,10 +2029,6 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr mgr, if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data)) goto cleanup;
- virPCIDeviceSetUnbindFromStub(pci, true); - virPCIDeviceSetRemoveSlot(pci, true); - virPCIDeviceSetReprobe(pci, true); -
The real change here, is that these lines ^^
if (virPCIDeviceReattach(pci, mgr->activePCIHostdevs, mgr->inactivePCIHostdevs) < 0) goto cleanup; diff --git a/src/util/virpci.c b/src/util/virpci.c index 75e8daadd5..4594643d3c 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1509,19 +1509,39 @@ virPCIDeviceDetach(virPCIDevicePtr dev, return 0; }
+/* + * Pre-condition: inactivePCIHostdevs & activePCIHostdevs + * are locked + */ int virPCIDeviceReattach(virPCIDevicePtr dev, virPCIDeviceListPtr activeDevs, virPCIDeviceListPtr inactiveDevs) { + int ret = -1; + if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Not reattaching active device %s"), dev->name); - return -1; + goto exit; + } + + virPCIDeviceSetUnbindFromStub(dev, true); + virPCIDeviceSetRemoveSlot(dev, true); + virPCIDeviceSetReprobe(dev, true);
are moved here ^^^
+ + /* Wait for device cleanup if it is qemu/kvm */ + if (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_KVM) { + int retries = 100; + while (virPCIDeviceWaitForCleanup(dev, "kvm_assigned_device") + && retries) { + usleep(100*1000); + retries--; + } }
and the wait is done here. I'm not against it, but maybe you can explain more why do you think this change is needed? Did you run into any troubles? Also, if we set these three attributes unconditionally, what good is there to keep those APIs around? Also, you forgot to update testVirPCIDeviceReattachSingle().
if (virPCIDeviceUnbindFromStub(dev) < 0) - return -1; + goto exit;
/* Steal the dev from list inactiveDevs */ if (inactiveDevs) { @@ -1529,7 +1549,10 @@ virPCIDeviceReattach(virPCIDevicePtr dev, virPCIDeviceListDel(inactiveDevs, dev); }
- return 0; + ret = 0; + + exit:
This is a useless label. Michal
On 7/19/19 8:52 AM, Michal Privoznik wrote:
On 7/18/19 10:10 PM, Daniel Henrique Barboza wrote:
We have 3 pieces of code that do slightly the same thing, but it varies depending on where it is called:
- virPCIDeviceReattach(). This is where the actual re-attach work happens;
- virHostdevReattachPCIDevice(). This is a static function from virhostdev.c that calls virPCIDeviceReattach() after waiting for device cleanup for the KVM PCI stub driver;
- virHostdevPCINodeDeviceReAttach(). This function also calls virPCIDeviceReattach(), but setting some device properties beforehand.
All these extra operations that are done before virPCIDeviceReattach() can be moved inside the function itself, allowing for the same re-attach behavior everywhere.
This patch consolidates all these pre-conditions inside the body of virPCIDeviceReattach(). With that, virHostdevReattachPCIDevice() can be removed and the callers can use virPCIDeviceReattach() directly instead.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/util/virhostdev.c | 43 +++++++++---------------------------------- src/util/virpci.c | 29 ++++++++++++++++++++++++++--- 2 files changed, 35 insertions(+), 37 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 53aacb59b4..23be037a39 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -613,33 +613,6 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev, } } -/* - * Pre-condition: inactivePCIHostdevs & activePCIHostdevs - * are locked - */ -static void -virHostdevReattachPCIDevice(virHostdevManagerPtr mgr, - virPCIDevicePtr actual) -{ - /* Wait for device cleanup if it is qemu/kvm */ - if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) { - int retries = 100; - while (virPCIDeviceWaitForCleanup(actual, "kvm_assigned_device") - && retries) { - usleep(100*1000); - retries--; - } - } - - VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(actual)); - if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs, - mgr->inactivePCIHostdevs) < 0) { - VIR_ERROR(_("Failed to re-attach PCI device: %s"), - virGetLastErrorMessage()); - virResetLastError(); - } -} - static void virHostdevReattachAllPCIDevices(virPCIDeviceListPtr pcidevs, virHostdevManagerPtr mgr) @@ -655,11 +628,17 @@ virHostdevReattachAllPCIDevices(virPCIDeviceListPtr pcidevs, if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) continue; - if (virPCIDeviceGetManaged(actual)) - virHostdevReattachPCIDevice(mgr, actual); - else + if (virPCIDeviceGetManaged(actual)) { + if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) { + VIR_ERROR(_("Failed to re-attach PCI device: %s"), + virGetLastErrorMessage()); + virResetLastError(); + } + } else { VIR_DEBUG("Not reattaching unmanaged PCI device %s", virPCIDeviceGetName(actual)); + } } } @@ -2050,10 +2029,6 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr mgr, if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data)) goto cleanup; - virPCIDeviceSetUnbindFromStub(pci, true); - virPCIDeviceSetRemoveSlot(pci, true); - virPCIDeviceSetReprobe(pci, true); -
The real change here, is that these lines ^^
if (virPCIDeviceReattach(pci, mgr->activePCIHostdevs, mgr->inactivePCIHostdevs) < 0) goto cleanup; diff --git a/src/util/virpci.c b/src/util/virpci.c index 75e8daadd5..4594643d3c 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1509,19 +1509,39 @@ virPCIDeviceDetach(virPCIDevicePtr dev, return 0; } +/* + * Pre-condition: inactivePCIHostdevs & activePCIHostdevs + * are locked + */ int virPCIDeviceReattach(virPCIDevicePtr dev, virPCIDeviceListPtr activeDevs, virPCIDeviceListPtr inactiveDevs) { + int ret = -1; + if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Not reattaching active device %s"), dev->name); - return -1; + goto exit; + } + + virPCIDeviceSetUnbindFromStub(dev, true); + virPCIDeviceSetRemoveSlot(dev, true); + virPCIDeviceSetReprobe(dev, true);
are moved here ^^^
+ + /* Wait for device cleanup if it is qemu/kvm */ + if (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_KVM) { + int retries = 100; + while (virPCIDeviceWaitForCleanup(dev, "kvm_assigned_device") + && retries) { + usleep(100*1000); + retries--; + } }
and the wait is done here. I'm not against it, but maybe you can explain more why do you think this change is needed? Did you run into any troubles? Also, if we set these three attributes unconditionally, what good is there to keep those APIs around? Also, you forgot to update testVirPCIDeviceReattachSingle().
My point for this change is to make it less confusing. Why do we set those 3 attributes in one place but neglect them in other instances? Basically we're doing a slight different process in 2-3 places without any particular reason. Perhaps a better change would be to remove those 3 setX(dev,true) lines from virHostdevPCINodeDeviceReAttach flat out (removing the APIs if they are being used just to set stuff to 'true' around the code), then introduce the wait code inside virPCIDeviceReattach(). And also update testVirPCIDeviceReattachSingle(), of course. I'll respin this up.
if (virPCIDeviceUnbindFromStub(dev) < 0) - return -1; + goto exit; /* Steal the dev from list inactiveDevs */ if (inactiveDevs) { @@ -1529,7 +1549,10 @@ virPCIDeviceReattach(virPCIDevicePtr dev, virPCIDeviceListDel(inactiveDevs, dev); } - return 0; + ret = 0; + + exit:
This is a useless label.
Michal
participants (2)
-
Daniel Henrique Barboza -
Michal Privoznik