3:10 p.m.
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
3:10 p.m.
New subject: [libvirt] [PATCH v1 1/3] virhostdev: introduce virHostdevResetAllPCIDevices
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(a)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
6:52 a.m.
New subject: [libvirt] [PATCH v1 1/3] virhostdev: introduce virHostdevResetAllPCIDevices
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(a)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
3:10 p.m.
New subject: [libvirt] [PATCH v1 2/3] virhostdev: introduce virHostdevReattachAllPCIDevices
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(a)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
6:52 a.m.
New subject: [libvirt] [PATCH v1 2/3] virhostdev: introduce virHostdevReattachAllPCIDevices
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(a)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
7:26 a.m.
New subject: [libvirt] [PATCH v1 2/3] virhostdev: introduce virHostdevReattachAllPCIDevices
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(a)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
3:10 p.m.
New subject: [libvirt] [PATCH v1 3/3] virhostdev: remove virHostdevReattachPCIDevice
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(a)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
6:52 a.m.
New subject: [libvirt] [PATCH v1 3/3] virhostdev: remove virHostdevReattachPCIDevice
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(a)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
7:36 a.m.
New subject: [libvirt] [PATCH v1 3/3] virhostdev: remove virHostdevReattachPCIDevice
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(a)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
1954
days inactive
1955
days old
8 comments
2 participants
participants (2)
-
Daniel Henrique Barboza -
Michal Privoznik