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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.