4:38 a.m.
https://bugzilla.redhat.com/show_bug.cgi?id=877095
Patch 1/4 fixes a possible double free on error path and the other patches
deal with memory and file descriptor leaks in PCI device attachment code.
Jiri Denemark (4):
qemu: Don't free PCI device if adding it to activePciHostdevs fails
util: Slightly refactor PCI list functions
qemu: Fix memory (and FD) leak on PCI device detach
qemu: Do not keep PCI device config file open
src/libvirt_private.syms | 3 +++
src/qemu/qemu_hostdev.c | 22 ++++++++--------
src/util/pci.c | 66 ++++++++++++++++++++++++++++--------------------
src/util/pci.h | 5 ++++
4 files changed, 58 insertions(+), 38 deletions(-)
--
1.8.0
4:38 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: Don't free PCI device if adding it to activePciHostdevs fails
The device is still referenced from pcidevs and freeing it would leave
an invalid pointer there.
---
src/qemu/qemu_hostdev.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index ab0f173..b79319e 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -491,10 +491,8 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver,
/* Loop 5: Now mark all the devices as active */
for (i = 0; i < pciDeviceListCount(pcidevs); i++) {
pciDevice *dev = pciDeviceListGet(pcidevs, i);
- if (pciDeviceListAdd(driver->activePciHostdevs, dev) < 0) {
- pciFreeDevice(dev);
+ if (pciDeviceListAdd(driver->activePciHostdevs, dev) < 0)
goto inactivedevs;
- }
}
/* Loop 6: Now remove the devices from inactive list. */
--
1.8.0
7:59 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: Don't free PCI device if adding it to activePciHostdevs fails
On 2012年12月04日 18:38, Jiri Denemark wrote:
The device is still referenced from pcidevs and freeing it would
leave
an invalid pointer there.
---
src/qemu/qemu_hostdev.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index ab0f173..b79319e 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -491,10 +491,8 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver,
/* Loop 5: Now mark all the devices as active */
for (i = 0; i< pciDeviceListCount(pcidevs); i++) {
pciDevice *dev = pciDeviceListGet(pcidevs, i);
- if (pciDeviceListAdd(driver->activePciHostdevs, dev)< 0) {
- pciFreeDevice(dev);
+ if (pciDeviceListAdd(driver->activePciHostdevs, dev)< 0)
goto inactivedevs;
- }
}
/* Loop 6: Now remove the devices from inactive list. */
ACK
4:38 a.m.
New subject: [libvirt] [PATCH 2/4] util: Slightly refactor PCI list functions
In order to be able to steal PCI device by its index in the list.
---
src/libvirt_private.syms | 2 ++
src/util/pci.c | 60 +++++++++++++++++++++++++++++-------------------
src/util/pci.h | 4 ++++
3 files changed, 42 insertions(+), 24 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 41e2629..625490f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1000,10 +1000,12 @@ pciDeviceListAdd;
pciDeviceListCount;
pciDeviceListDel;
pciDeviceListFind;
+pciDeviceListFindIndex;
pciDeviceListFree;
pciDeviceListGet;
pciDeviceListNew;
pciDeviceListSteal;
+pciDeviceListStealIndex;
pciDeviceNetName;
pciDeviceReAttachInit;
pciDeviceSetManaged;
diff --git a/src/util/pci.c b/src/util/pci.c
index 191f99d..3ebf6f7 100644
--- a/src/util/pci.c
+++ b/src/util/pci.c
@@ -1554,35 +1554,36 @@ pciDeviceListCount(pciDeviceList *list)
}
pciDevice *
-pciDeviceListSteal(pciDeviceList *list,
- pciDevice *dev)
+pciDeviceListStealIndex(pciDeviceList *list,
+ int idx)
{
- pciDevice *ret = NULL;
- int i;
-
- for (i = 0; i < list->count; i++) {
- if (list->devs[i]->domain != dev->domain ||
- list->devs[i]->bus != dev->bus ||
- list->devs[i]->slot != dev->slot ||
- list->devs[i]->function != dev->function)
- continue;
+ pciDevice *ret;
- ret = list->devs[i];
+ if (idx < 0)
+ return NULL;
- if (i != --list->count)
- memmove(&list->devs[i],
- &list->devs[i+1],
- sizeof(*list->devs) * (list->count-i));
+ ret = list->devs[idx];
- if (VIR_REALLOC_N(list->devs, list->count) < 0) {
- ; /* not fatal */
- }
+ if (idx != --list->count) {
+ memmove(&list->devs[idx],
+ &list->devs[idx + 1],
+ sizeof(*list->devs) * (list->count - idx));
+ }
- break;
+ if (VIR_REALLOC_N(list->devs, list->count) < 0) {
+ ; /* not fatal */
}
+
return ret;
}
+pciDevice *
+pciDeviceListSteal(pciDeviceList *list,
+ pciDevice *dev)
+{
+ return pciDeviceListStealIndex(list, pciDeviceListFindIndex(list, dev));
+}
+
void
pciDeviceListDel(pciDeviceList *list,
pciDevice *dev)
@@ -1592,8 +1593,8 @@ pciDeviceListDel(pciDeviceList *list,
pciFreeDevice(ret);
}
-pciDevice *
-pciDeviceListFind(pciDeviceList *list, pciDevice *dev)
+int
+pciDeviceListFindIndex(pciDeviceList *list, pciDevice *dev)
{
int i;
@@ -1602,8 +1603,19 @@ pciDeviceListFind(pciDeviceList *list, pciDevice *dev)
list->devs[i]->bus == dev->bus &&
list->devs[i]->slot == dev->slot &&
list->devs[i]->function == dev->function)
- return list->devs[i];
- return NULL;
+ return i;
+ return -1;
+}
+
+pciDevice *
+pciDeviceListFind(pciDeviceList *list, pciDevice *dev)
+{
+ int i;
+
+ if ((i = pciDeviceListFindIndex(list, dev)) >= 0)
+ return list->devs[i];
+ else
+ return NULL;
}
diff --git a/src/util/pci.h b/src/util/pci.h
index d3cc85d..814c24e 100644
--- a/src/util/pci.h
+++ b/src/util/pci.h
@@ -75,10 +75,14 @@ pciDevice * pciDeviceListGet (pciDeviceList *list,
int pciDeviceListCount (pciDeviceList *list);
pciDevice * pciDeviceListSteal (pciDeviceList *list,
pciDevice *dev);
+pciDevice * pciDeviceListStealIndex(pciDeviceList *list,
+ int idx);
void pciDeviceListDel (pciDeviceList *list,
pciDevice *dev);
pciDevice * pciDeviceListFind (pciDeviceList *list,
pciDevice *dev);
+int pciDeviceListFindIndex(pciDeviceList *list,
+ pciDevice *dev);
/*
* Callback that will be invoked once for each file
--
1.8.0
8:08 a.m.
New subject: [libvirt] [PATCH 2/4] util: Slightly refactor PCI list functions
On 2012年12月04日 18:38, Jiri Denemark wrote:
In order to be able to steal PCI device by its index in the list.
---
src/libvirt_private.syms | 2 ++
src/util/pci.c | 60 +++++++++++++++++++++++++++++-------------------
src/util/pci.h | 4 ++++
3 files changed, 42 insertions(+), 24 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 41e2629..625490f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1000,10 +1000,12 @@ pciDeviceListAdd;
pciDeviceListCount;
pciDeviceListDel;
pciDeviceListFind;
+pciDeviceListFindIndex;
pciDeviceListFree;
pciDeviceListGet;
pciDeviceListNew;
pciDeviceListSteal;
+pciDeviceListStealIndex;
pciDeviceNetName;
pciDeviceReAttachInit;
pciDeviceSetManaged;
diff --git a/src/util/pci.c b/src/util/pci.c
index 191f99d..3ebf6f7 100644
--- a/src/util/pci.c
+++ b/src/util/pci.c
@@ -1554,35 +1554,36 @@ pciDeviceListCount(pciDeviceList *list)
}
pciDevice *
-pciDeviceListSteal(pciDeviceList *list,
- pciDevice *dev)
+pciDeviceListStealIndex(pciDeviceList *list,
+ int idx)
{
- pciDevice *ret = NULL;
- int i;
-
- for (i = 0; i< list->count; i++) {
- if (list->devs[i]->domain != dev->domain ||
- list->devs[i]->bus != dev->bus ||
- list->devs[i]->slot != dev->slot ||
- list->devs[i]->function != dev->function)
- continue;
+ pciDevice *ret;
- ret = list->devs[i];
+ if (idx< 0)
+ return NULL;
Per the function is not static, it's better to check the upper range
of the list index too.
Assuming all the new helpers are for later patches use, ACK with
the index checking fixed.
Osier
8:32 a.m.
New subject: [libvirt] [PATCH 2/4] util: Slightly refactor PCI list functions
On 2012年12月04日 18:38, Jiri Denemark wrote:
In order to be able to steal PCI device by its index in the list.
---
src/libvirt_private.syms | 2 ++
src/util/pci.c | 60 +++++++++++++++++++++++++++++-------------------
src/util/pci.h | 4 ++++
3 files changed, 42 insertions(+), 24 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 41e2629..625490f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1000,10 +1000,12 @@ pciDeviceListAdd;
pciDeviceListCount;
pciDeviceListDel;
pciDeviceListFind;
+pciDeviceListFindIndex;
pciDeviceListFree;
pciDeviceListGet;
pciDeviceListNew;
pciDeviceListSteal;
After looking through all the 4 patches, I see there is no use
of pciDeviceListFindIndex. Personally I'd like change it to
static.
Osier
12:13 p.m.
New subject: [libvirt] [PATCH 2/4] util: Slightly refactor PCI list functions
On Tue, Dec 04, 2012 at 22:32:35 +0800, Osier Yang wrote:
On 2012年12月04日 18:38, Jiri Denemark wrote:
> In order to be able to steal PCI device by its index in the list.
> ---
> src/libvirt_private.syms | 2 ++
> src/util/pci.c | 60 +++++++++++++++++++++++++++++-------------------
> src/util/pci.h | 4 ++++
> 3 files changed, 42 insertions(+), 24 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 41e2629..625490f 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1000,10 +1000,12 @@ pciDeviceListAdd;
> pciDeviceListCount;
> pciDeviceListDel;
> pciDeviceListFind;
> +pciDeviceListFindIndex;
> pciDeviceListFree;
> pciDeviceListGet;
> pciDeviceListNew;
> pciDeviceListSteal;
After looking through all the 4 patches, I see there is no use
of pciDeviceListFindIndex. Personally I'd like change it to
static.
Right, it's not used outside pci.c, however it might be potentially useful and
the result is definitely easier to review than a patch that would also move
pciDeviceListFindIndex code several functions up so that it can be made static
:-) However, I agree with adding an additional check in
pciDeviceListStealIndex.
Jirka
4:38 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: Fix memory (and FD) leak on PCI device detach
Unmanaged PCI devices were only leaked if pciDeviceListAdd failed but
managed devices were always leaked. And leaking PCI device is likely to
leave PCI config file descriptor open. This patch fixes
qemuReattachPciDevice to either free the PCI device or add it to the
inactivePciHostdevs list.
---
src/qemu/qemu_hostdev.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index b79319e..a748b8b 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -546,10 +546,8 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver,
}
/* Loop 9: Now steal all the devices from pcidevs */
- while (pciDeviceListCount(pcidevs) > 0) {
- pciDevice *dev = pciDeviceListGet(pcidevs, 0);
- pciDeviceListSteal(pcidevs, dev);
- }
+ while (pciDeviceListCount(pcidevs) > 0)
+ pciDeviceListStealIndex(pcidevs, 0);
ret = 0;
goto cleanup;
@@ -818,7 +816,8 @@ void qemuReattachPciDevice(pciDevice *dev, virQEMUDriverPtr driver)
* successfully, it must have been inactive.
*/
if (!pciDeviceGetManaged(dev)) {
- pciDeviceListAdd(driver->inactivePciHostdevs, dev);
+ if (pciDeviceListAdd(driver->inactivePciHostdevs, dev) < 0)
+ pciFreeDevice(dev);
return;
}
@@ -835,6 +834,7 @@ void qemuReattachPciDevice(pciDevice *dev, virQEMUDriverPtr driver)
err ? err->message : _("unknown error"));
virResetError(err);
}
+ pciFreeDevice(dev);
}
@@ -905,8 +905,8 @@ void qemuDomainReAttachHostdevDevices(virQEMUDriverPtr driver,
}
}
- for (i = 0; i < pciDeviceListCount(pcidevs); i++) {
- pciDevice *dev = pciDeviceListGet(pcidevs, i);
+ while (pciDeviceListCount(pcidevs) > 0) {
+ pciDevice *dev = pciDeviceListStealIndex(pcidevs, 0);
qemuReattachPciDevice(dev, driver);
}
--
1.8.0
8:29 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: Fix memory (and FD) leak on PCI device detach
On 2012年12月04日 18:38, Jiri Denemark wrote:
Unmanaged PCI devices were only leaked if pciDeviceListAdd failed
but
managed devices were always leaked. And leaking PCI device is likely to
leave PCI config file descriptor open. This patch fixes
qemuReattachPciDevice to either free the PCI device or add it to the
inactivePciHostdevs list.
---
src/qemu/qemu_hostdev.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index b79319e..a748b8b 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -546,10 +546,8 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver,
}
/* Loop 9: Now steal all the devices from pcidevs */
- while (pciDeviceListCount(pcidevs)> 0) {
- pciDevice *dev = pciDeviceListGet(pcidevs, 0);
- pciDeviceListSteal(pcidevs, dev);
- }
+ while (pciDeviceListCount(pcidevs)> 0)
+ pciDeviceListStealIndex(pcidevs, 0);
ret = 0;
goto cleanup;
@@ -818,7 +816,8 @@ void qemuReattachPciDevice(pciDevice *dev, virQEMUDriverPtr driver)
* successfully, it must have been inactive.
*/
if (!pciDeviceGetManaged(dev)) {
- pciDeviceListAdd(driver->inactivePciHostdevs, dev);
+ if (pciDeviceListAdd(driver->inactivePciHostdevs, dev)< 0)
+ pciFreeDevice(dev);
return;
}
@@ -835,6 +834,7 @@ void qemuReattachPciDevice(pciDevice *dev, virQEMUDriverPtr driver)
err ? err->message : _("unknown error"));
virResetError(err);
}
+ pciFreeDevice(dev);
}
This produces the similar problem 1/4 tries to fix. pciDeviceListFree
right after will free the whole list. Except this, the using of
pciDeviceListStealIndex is nice.
Osier
11:48 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: Fix memory (and FD) leak on PCI device detach
On Tue, Dec 04, 2012 at 22:29:23 +0800, Osier Yang wrote:
On 2012年12月04日 18:38, Jiri Denemark wrote:
> Unmanaged PCI devices were only leaked if pciDeviceListAdd failed but
> managed devices were always leaked. And leaking PCI device is likely to
> leave PCI config file descriptor open. This patch fixes
> qemuReattachPciDevice to either free the PCI device or add it to the
> inactivePciHostdevs list.
> ---
> src/qemu/qemu_hostdev.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
> index b79319e..a748b8b 100644
> --- a/src/qemu/qemu_hostdev.c
> +++ b/src/qemu/qemu_hostdev.c
> @@ -546,10 +546,8 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver,
> }
>
> /* Loop 9: Now steal all the devices from pcidevs */
> - while (pciDeviceListCount(pcidevs)> 0) {
> - pciDevice *dev = pciDeviceListGet(pcidevs, 0);
> - pciDeviceListSteal(pcidevs, dev);
> - }
> + while (pciDeviceListCount(pcidevs)> 0)
> + pciDeviceListStealIndex(pcidevs, 0);
>
> ret = 0;
> goto cleanup;
> @@ -818,7 +816,8 @@ void qemuReattachPciDevice(pciDevice *dev, virQEMUDriverPtr
driver)
> * successfully, it must have been inactive.
> */
> if (!pciDeviceGetManaged(dev)) {
> - pciDeviceListAdd(driver->inactivePciHostdevs, dev);
> + if (pciDeviceListAdd(driver->inactivePciHostdevs, dev)< 0)
> + pciFreeDevice(dev);
> return;
> }
>
> @@ -835,6 +834,7 @@ void qemuReattachPciDevice(pciDevice *dev, virQEMUDriverPtr
driver)
> err ? err->message : _("unknown error"));
> virResetError(err);
> }
> + pciFreeDevice(dev);
> }
This produces the similar problem 1/4 tries to fix. pciDeviceListFree
right after will free the whole list. Except this, the using of
pciDeviceListStealIndex is nice.
I don't think it does. There are only two places where qemuReattachPciDevice
is called. The first one is the hunk you removed from this patch:
@@ -905,8 +905,8 @@ void qemuDomainReAttachHostdevDevices(virQEMUDriverPtr driver,
}
}
- for (i = 0; i < pciDeviceListCount(pcidevs); i++) {
- pciDevice *dev = pciDeviceListGet(pcidevs, i);
+ while (pciDeviceListCount(pcidevs) > 0) {
+ pciDevice *dev = pciDeviceListStealIndex(pcidevs, 0);
qemuReattachPciDevice(dev, driver);
}
and its purpose is to fix the caller to call qemuReattachPciDevice with a
device that is not referenced from pcidevs list (hence, it changes
pciDeviceListGet into pciDeviceListStealIndex). And the second one is in
qemu_hotplug.c:
activePci = pciDeviceListSteal(driver->activePciHostdevs, pci);
if (activePci &&
pciResetDevice(activePci, driver->activePciHostdevs,
driver->inactivePciHostdevs) == 0) {
qemuReattachPciDevice(activePci, driver);
} else {
/* reset of the device failed, treat it as if it was returned */
pciFreeDevice(activePci);
ret = -1;
}
That is, it has already been relying on qemuReattachPciDevice to properly free
activePci if needed.
Jirka
11:55 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: Fix memory (and FD) leak on PCI device detach
On 2012年12月05日 01:48, Jiri Denemark wrote:
On Tue, Dec 04, 2012 at 22:29:23 +0800, Osier Yang wrote:
> On 2012年12月04日 18:38, Jiri Denemark wrote:
>> Unmanaged PCI devices were only leaked if pciDeviceListAdd failed but
>> managed devices were always leaked. And leaking PCI device is likely to
>> leave PCI config file descriptor open. This patch fixes
>> qemuReattachPciDevice to either free the PCI device or add it to the
>> inactivePciHostdevs list.
>> ---
>> src/qemu/qemu_hostdev.c | 14 +++++++-------
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
>> index b79319e..a748b8b 100644
>> --- a/src/qemu/qemu_hostdev.c
>> +++ b/src/qemu/qemu_hostdev.c
>> @@ -546,10 +546,8 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver,
>> }
>>
>> /* Loop 9: Now steal all the devices from pcidevs */
>> - while (pciDeviceListCount(pcidevs)> 0) {
>> - pciDevice *dev = pciDeviceListGet(pcidevs, 0);
>> - pciDeviceListSteal(pcidevs, dev);
>> - }
>> + while (pciDeviceListCount(pcidevs)> 0)
>> + pciDeviceListStealIndex(pcidevs, 0);
>>
>> ret = 0;
>> goto cleanup;
>> @@ -818,7 +816,8 @@ void qemuReattachPciDevice(pciDevice *dev, virQEMUDriverPtr
driver)
>> * successfully, it must have been inactive.
>> */
>> if (!pciDeviceGetManaged(dev)) {
>> - pciDeviceListAdd(driver->inactivePciHostdevs, dev);
>> + if (pciDeviceListAdd(driver->inactivePciHostdevs, dev)< 0)
>> + pciFreeDevice(dev);
>> return;
>> }
>>
>> @@ -835,6 +834,7 @@ void qemuReattachPciDevice(pciDevice *dev, virQEMUDriverPtr
driver)
>> err ? err->message : _("unknown error"));
>> virResetError(err);
>> }
>> + pciFreeDevice(dev);
>> }
>
> This produces the similar problem 1/4 tries to fix. pciDeviceListFree
> right after will free the whole list. Except this, the using of
> pciDeviceListStealIndex is nice.
I don't think it does. There are only two places where qemuReattachPciDevice
is called. The first one is the hunk you removed from this patch:
@@ -905,8 +905,8 @@ void qemuDomainReAttachHostdevDevices(virQEMUDriverPtr driver,
}
}
- for (i = 0; i< pciDeviceListCount(pcidevs); i++) {
- pciDevice *dev = pciDeviceListGet(pcidevs, i);
+ while (pciDeviceListCount(pcidevs)> 0) {
+ pciDevice *dev = pciDeviceListStealIndex(pcidevs, 0);
qemuReattachPciDevice(dev, driver);
}
and its purpose is to fix the caller to call qemuReattachPciDevice with a
device that is not referenced from pcidevs list (hence, it changes
pciDeviceListGet into pciDeviceListStealIndex
Hum, I ignored this pciDeviceListStealIndex changing when reviewing. ACK
then.
Osier
4:38 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: Do not keep PCI device config file open
We only need to access the PCI device config file when
attaching/detaching the device to a domain. Keeping it open all the time
the device is attached to a domain is useless.
---
src/libvirt_private.syms | 1 +
src/qemu/qemu_hostdev.c | 6 ++++--
src/util/pci.c | 6 +++---
src/util/pci.h | 1 +
4 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 625490f..672a99a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -986,6 +986,7 @@ virNWFilterVarValueGetSimple;
# pci.h
pciConfigAddressToSysfsFile;
pciDettachDevice;
+pciDeviceClose;
pciDeviceFileIterate;
pciDeviceGetManaged;
pciDeviceGetName;
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index a748b8b..ef4722e 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -545,9 +545,11 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver,
pciFreeDevice(dev);
}
- /* Loop 9: Now steal all the devices from pcidevs */
+ /* Loop 9: Now steal all the devices from pcidevs and close
+ * their config files
+ */
while (pciDeviceListCount(pcidevs) > 0)
- pciDeviceListStealIndex(pcidevs, 0);
+ pciDeviceClose(pciDeviceListStealIndex(pcidevs, 0));
ret = 0;
goto cleanup;
diff --git a/src/util/pci.c b/src/util/pci.c
index 3ebf6f7..e32f2e0 100644
--- a/src/util/pci.c
+++ b/src/util/pci.c
@@ -185,8 +185,8 @@ pciOpenConfig(pciDevice *dev)
return 0;
}
-static void
-pciCloseConfig(pciDevice *dev)
+void
+pciDeviceClose(pciDevice *dev)
{
if (!dev)
return;
@@ -1407,7 +1407,7 @@ pciFreeDevice(pciDevice *dev)
if (!dev)
return;
VIR_DEBUG("%s %s: freeing", dev->id, dev->name);
- pciCloseConfig(dev);
+ pciDeviceClose(dev);
VIR_FREE(dev->path);
VIR_FREE(dev);
}
diff --git a/src/util/pci.h b/src/util/pci.h
index 814c24e..91ebaff 100644
--- a/src/util/pci.h
+++ b/src/util/pci.h
@@ -65,6 +65,7 @@ unsigned pciDeviceGetReprobe(pciDevice *dev);
void pciDeviceSetReprobe(pciDevice *dev,
unsigned reprobe);
void pciDeviceReAttachInit(pciDevice *dev);
+void pciDeviceClose(pciDevice *dev);
pciDeviceList *pciDeviceListNew (void);
void pciDeviceListFree (pciDeviceList *list);
--
1.8.0
8:45 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: Do not keep PCI device config file open
On 2012年12月04日 18:38, Jiri Denemark wrote:
We only need to access the PCI device config file when
attaching/detaching the device to a domain. Keeping it open all the time
the device is attached to a domain is useless.
---
src/libvirt_private.syms | 1 +
src/qemu/qemu_hostdev.c | 6 ++++--
src/util/pci.c | 6 +++---
src/util/pci.h | 1 +
4 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 625490f..672a99a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -986,6 +986,7 @@ virNWFilterVarValueGetSimple;
# pci.h
pciConfigAddressToSysfsFile;
pciDettachDevice;
+pciDeviceClose;
pciDeviceFileIterate;
pciDeviceGetManaged;
pciDeviceGetName;
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index a748b8b..ef4722e 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -545,9 +545,11 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver,
pciFreeDevice(dev);
}
- /* Loop 9: Now steal all the devices from pcidevs */
+ /* Loop 9: Now steal all the devices from pcidevs and close
+ * their config files
+ */
while (pciDeviceListCount(pcidevs)> 0)
- pciDeviceListStealIndex(pcidevs, 0);
+ pciDeviceClose(pciDeviceListStealIndex(pcidevs, 0));
ret = 0;
goto cleanup;
diff --git a/src/util/pci.c b/src/util/pci.c
index 3ebf6f7..e32f2e0 100644
--- a/src/util/pci.c
+++ b/src/util/pci.c
@@ -185,8 +185,8 @@ pciOpenConfig(pciDevice *dev)
return 0;
}
-static void
-pciCloseConfig(pciDevice *dev)
+void
+pciDeviceClose(pciDevice *dev)
{
if (!dev)
return;
@@ -1407,7 +1407,7 @@ pciFreeDevice(pciDevice *dev)
if (!dev)
return;
VIR_DEBUG("%s %s: freeing", dev->id, dev->name);
- pciCloseConfig(dev);
+ pciDeviceClose(dev);
VIR_FREE(dev->path);
VIR_FREE(dev);
}
diff --git a/src/util/pci.h b/src/util/pci.h
index 814c24e..91ebaff 100644
--- a/src/util/pci.h
+++ b/src/util/pci.h
@@ -65,6 +65,7 @@ unsigned pciDeviceGetReprobe(pciDevice *dev);
void pciDeviceSetReprobe(pciDevice *dev,
unsigned reprobe);
void pciDeviceReAttachInit(pciDevice *dev);
+void pciDeviceClose(pciDevice *dev);
pciDeviceList *pciDeviceListNew (void);
void pciDeviceListFree (pciDeviceList *list);
ACK
11:55 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: Do not keep PCI device config file open
On Tue, Dec 04, 2012 at 11:38:22AM +0100, Jiri Denemark wrote:
We only need to access the PCI device config file when
attaching/detaching the device to a domain. Keeping it open all the time
the device is attached to a domain is useless.
IMHO this is really just papering over a really terrible API
design in the pciDevicePtr code. IMHO the core issue here is
that two of our APIs pciDeviceReset() and pciDeviceIsAssignable()
will open an FD todo their work and then not close it afterwards.
As a caller of pciDeviceReset or pciDeviceIsAssignable I would
really not expect that a file descriptor is left open. As such
I don't think requiring the caller to use pciDeviceClose is the
right approach. Those methods should be made leak-proof by
having them close the FD themselves. To re-inforce this, I'd
actually remove 'int fd' from the struct entirely, and have it
be method-local, passed around as needed.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
12:24 p.m.
New subject: [libvirt] [PATCH 4/4] qemu: Do not keep PCI device config file open
On Tue, Dec 04, 2012 at 17:55:33 +0000, Daniel P. Berrange wrote:
On Tue, Dec 04, 2012 at 11:38:22AM +0100, Jiri Denemark wrote:
> We only need to access the PCI device config file when
> attaching/detaching the device to a domain. Keeping it open all the time
> the device is attached to a domain is useless.
IMHO this is really just papering over a really terrible API
design in the pciDevicePtr code. IMHO the core issue here is
that two of our APIs pciDeviceReset() and pciDeviceIsAssignable()
will open an FD todo their work and then not close it afterwards.
As a caller of pciDeviceReset or pciDeviceIsAssignable I would
really not expect that a file descriptor is left open. As such
I don't think requiring the caller to use pciDeviceClose is the
right approach. Those methods should be made leak-proof by
having them close the FD themselves. To re-inforce this, I'd
actually remove 'int fd' from the struct entirely, and have it
be method-local, passed around as needed.
Hmm, right, although having this terrible design allowed us to catch the
memory leak since fd leak is more visible :-) It really seems there's no need
to keep the fd open across several APIs. I'll rework this patch.
Jirka
4:23 p.m.
New subject: [libvirt] [PATCH v2 4/4] util: Do not keep PCI device config file open
Directly open and close PCI config file in the APIs that need it rather
than keeping the file open for the whole life of PCI device structure.
---
src/util/pci.c | 265 +++++++++++++++++++++++++++++++++------------------------
1 file changed, 156 insertions(+), 109 deletions(-)
diff --git a/src/util/pci.c b/src/util/pci.c
index e410245..8bded78 100644
--- a/src/util/pci.c
+++ b/src/util/pci.c
@@ -58,9 +58,7 @@ struct _pciDevice {
char id[PCI_ID_LEN]; /* product vendor */
char *path;
const char *used_by; /* The domain which uses the device */
- int fd;
- unsigned initted;
unsigned pcie_cap_pos;
unsigned pci_pm_cap_pos;
unsigned has_flr : 1;
@@ -166,44 +164,51 @@ struct _pciDeviceList {
PCI_EXT_CAP_ACS_UF)
static int
-pciOpenConfig(pciDevice *dev)
+pciConfigOpen(pciDevice *dev, bool fatal)
{
int fd;
- if (dev->fd > 0)
- return 0;
-
fd = open(dev->path, O_RDWR);
+
if (fd < 0) {
- char ebuf[1024];
- VIR_WARN("Failed to open config space file '%s': %s",
- dev->path, virStrerror(errno, ebuf, sizeof(ebuf)));
+ if (fatal) {
+ virReportSystemError(errno,
+ _("Failed to open config space file
'%s'"),
+ dev->path);
+ } else {
+ char ebuf[1024];
+ VIR_WARN("Failed to open config space file '%s': %s",
+ dev->path, virStrerror(errno, ebuf, sizeof(ebuf)));
+ }
return -1;
}
+
VIR_DEBUG("%s %s: opened %s", dev->id, dev->name, dev->path);
- dev->fd = fd;
- return 0;
+ return fd;
}
static void
-pciCloseConfig(pciDevice *dev)
+pciConfigClose(pciDevice *dev, int cfgfd)
{
- if (!dev)
- return;
-
- VIR_FORCE_CLOSE(dev->fd);
+ if (VIR_CLOSE(cfgfd) < 0) {
+ char ebuf[1024];
+ VIR_WARN("Failed to close config space file '%s': %s",
+ dev->path, virStrerror(errno, ebuf, sizeof(ebuf)));
+ }
}
+
static int
-pciRead(pciDevice *dev, unsigned pos, uint8_t *buf, unsigned buflen)
+pciRead(pciDevice *dev,
+ int cfgfd,
+ unsigned pos,
+ uint8_t *buf,
+ unsigned buflen)
{
memset(buf, 0, buflen);
- if (pciOpenConfig(dev) < 0)
- return -1;
-
- if (lseek(dev->fd, pos, SEEK_SET) != pos ||
- saferead(dev->fd, buf, buflen) != buflen) {
+ if (lseek(cfgfd, pos, SEEK_SET) != pos ||
+ saferead(cfgfd, buf, buflen) != buflen) {
char ebuf[1024];
VIR_WARN("Failed to read from '%s' : %s", dev->path,
virStrerror(errno, ebuf, sizeof(ebuf)));
@@ -213,37 +218,38 @@ pciRead(pciDevice *dev, unsigned pos, uint8_t *buf, unsigned
buflen)
}
static uint8_t
-pciRead8(pciDevice *dev, unsigned pos)
+pciRead8(pciDevice *dev, int cfgfd, unsigned pos)
{
uint8_t buf;
- pciRead(dev, pos, &buf, sizeof(buf));
+ pciRead(dev, cfgfd, pos, &buf, sizeof(buf));
return buf;
}
static uint16_t
-pciRead16(pciDevice *dev, unsigned pos)
+pciRead16(pciDevice *dev, int cfgfd, unsigned pos)
{
uint8_t buf[2];
- pciRead(dev, pos, &buf[0], sizeof(buf));
+ pciRead(dev, cfgfd, pos, &buf[0], sizeof(buf));
return (buf[0] << 0) | (buf[1] << 8);
}
static uint32_t
-pciRead32(pciDevice *dev, unsigned pos)
+pciRead32(pciDevice *dev, int cfgfd, unsigned pos)
{
uint8_t buf[4];
- pciRead(dev, pos, &buf[0], sizeof(buf));
+ pciRead(dev, cfgfd, pos, &buf[0], sizeof(buf));
return (buf[0] << 0) | (buf[1] << 8) | (buf[2] << 16) | (buf[3]
<< 24);
}
static int
-pciWrite(pciDevice *dev, unsigned pos, uint8_t *buf, unsigned buflen)
-{
- if (pciOpenConfig(dev) < 0)
- return -1;
-
- if (lseek(dev->fd, pos, SEEK_SET) != pos ||
- safewrite(dev->fd, buf, buflen) != buflen) {
+pciWrite(pciDevice *dev,
+ int cfgfd,
+ unsigned pos,
+ uint8_t *buf,
+ unsigned buflen)
+{
+ if (lseek(cfgfd, pos, SEEK_SET) != pos ||
+ safewrite(cfgfd, buf, buflen) != buflen) {
char ebuf[1024];
VIR_WARN("Failed to write to '%s' : %s", dev->path,
virStrerror(errno, ebuf, sizeof(ebuf)));
@@ -253,17 +259,17 @@ pciWrite(pciDevice *dev, unsigned pos, uint8_t *buf, unsigned
buflen)
}
static void
-pciWrite16(pciDevice *dev, unsigned pos, uint16_t val)
+pciWrite16(pciDevice *dev, int cfgfd, unsigned pos, uint16_t val)
{
uint8_t buf[2] = { (val >> 0), (val >> 8) };
- pciWrite(dev, pos, &buf[0], sizeof(buf));
+ pciWrite(dev, cfgfd, pos, &buf[0], sizeof(buf));
}
static void
-pciWrite32(pciDevice *dev, unsigned pos, uint32_t val)
+pciWrite32(pciDevice *dev, int cfgfd, unsigned pos, uint32_t val)
{
uint8_t buf[4] = { (val >> 0), (val >> 8), (val >> 16), (val
>> 14) };
- pciWrite(dev, pos, &buf[0], sizeof(buf));
+ pciWrite(dev, cfgfd, pos, &buf[0], sizeof(buf));
}
typedef int (*pciIterPredicate)(pciDevice *, pciDevice *, void *);
@@ -343,16 +349,16 @@ pciIterDevices(pciIterPredicate predicate,
}
static uint8_t
-pciFindCapabilityOffset(pciDevice *dev, unsigned capability)
+pciFindCapabilityOffset(pciDevice *dev, int cfgfd, unsigned capability)
{
uint16_t status;
uint8_t pos;
- status = pciRead16(dev, PCI_STATUS);
+ status = pciRead16(dev, cfgfd, PCI_STATUS);
if (!(status & PCI_STATUS_CAP_LIST))
return 0;
- pos = pciRead8(dev, PCI_CAPABILITY_LIST);
+ pos = pciRead8(dev, cfgfd, PCI_CAPABILITY_LIST);
/* Zero indicates last capability, capabilities can't
* be in the config space header and 0xff is returned
@@ -362,14 +368,14 @@ pciFindCapabilityOffset(pciDevice *dev, unsigned capability)
* capabilities here.
*/
while (pos >= PCI_CONF_HEADER_LEN && pos != 0xff) {
- uint8_t capid = pciRead8(dev, pos);
+ uint8_t capid = pciRead8(dev, cfgfd, pos);
if (capid == capability) {
VIR_DEBUG("%s %s: found cap 0x%.2x at 0x%.2x",
dev->id, dev->name, capability, pos);
return pos;
}
- pos = pciRead8(dev, pos + 1);
+ pos = pciRead8(dev, cfgfd, pos + 1);
}
VIR_DEBUG("%s %s: failed to find cap 0x%.2x", dev->id, dev->name,
capability);
@@ -378,7 +384,9 @@ pciFindCapabilityOffset(pciDevice *dev, unsigned capability)
}
static unsigned int
-pciFindExtendedCapabilityOffset(pciDevice *dev, unsigned capability)
+pciFindExtendedCapabilityOffset(pciDevice *dev,
+ int cfgfd,
+ unsigned capability)
{
int ttl;
unsigned int pos;
@@ -389,7 +397,7 @@ pciFindExtendedCapabilityOffset(pciDevice *dev, unsigned capability)
pos = PCI_EXT_CAP_BASE;
while (ttl > 0 && pos >= PCI_EXT_CAP_BASE) {
- header = pciRead32(dev, pos);
+ header = pciRead32(dev, cfgfd, pos);
if ((header & PCI_EXT_CAP_ID_MASK) == capability)
return pos;
@@ -405,7 +413,7 @@ pciFindExtendedCapabilityOffset(pciDevice *dev, unsigned capability)
* not have FLR, 1 if it does, and -1 on error
*/
static int
-pciDetectFunctionLevelReset(pciDevice *dev)
+pciDetectFunctionLevelReset(pciDevice *dev, int cfgfd)
{
uint32_t caps;
uint8_t pos;
@@ -419,7 +427,7 @@ pciDetectFunctionLevelReset(pciDevice *dev)
* on SR-IOV NICs at the moment.
*/
if (dev->pcie_cap_pos) {
- caps = pciRead32(dev, dev->pcie_cap_pos + PCI_EXP_DEVCAP);
+ caps = pciRead32(dev, cfgfd, dev->pcie_cap_pos + PCI_EXP_DEVCAP);
if (caps & PCI_EXP_DEVCAP_FLR) {
VIR_DEBUG("%s %s: detected PCIe FLR capability", dev->id,
dev->name);
return 1;
@@ -430,9 +438,9 @@ pciDetectFunctionLevelReset(pciDevice *dev)
* the same thing, except for conventional PCI
* devices. This is not common yet.
*/
- pos = pciFindCapabilityOffset(dev, PCI_CAP_ID_AF);
+ pos = pciFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF);
if (pos) {
- caps = pciRead16(dev, pos + PCI_AF_CAP);
+ caps = pciRead16(dev, cfgfd, pos + PCI_AF_CAP);
if (caps & PCI_AF_CAP_FLR) {
VIR_DEBUG("%s %s: detected PCI FLR capability", dev->id,
dev->name);
return 1;
@@ -468,13 +476,13 @@ pciDetectFunctionLevelReset(pciDevice *dev)
* internal reset, not just a soft reset.
*/
static unsigned
-pciDetectPowerManagementReset(pciDevice *dev)
+pciDetectPowerManagementReset(pciDevice *dev, int cfgfd)
{
if (dev->pci_pm_cap_pos) {
uint32_t ctl;
/* require the NO_SOFT_RESET bit is clear */
- ctl = pciRead32(dev, dev->pci_pm_cap_pos + PCI_PM_CTRL);
+ ctl = pciRead32(dev, cfgfd, dev->pci_pm_cap_pos + PCI_PM_CTRL);
if (!(ctl & PCI_PM_CTRL_NO_SOFT_RESET)) {
VIR_DEBUG("%s %s: detected PM reset capability", dev->id,
dev->name);
return 1;
@@ -524,30 +532,37 @@ pciIsParent(pciDevice *dev, pciDevice *check, void *data)
uint16_t device_class;
uint8_t header_type, secondary, subordinate;
pciDevice **best = data;
+ int ret = 0;
+ int fd;
if (dev->domain != check->domain)
return 0;
+ if ((fd = pciConfigOpen(check, false)) < 0)
+ return 0;
+
/* Is it a bridge? */
- device_class = pciRead16(check, PCI_CLASS_DEVICE);
+ device_class = pciRead16(check, fd, PCI_CLASS_DEVICE);
if (device_class != PCI_CLASS_BRIDGE_PCI)
- return 0;
+ goto cleanup;
/* Is it a plane? */
- header_type = pciRead8(check, PCI_HEADER_TYPE);
+ header_type = pciRead8(check, fd, PCI_HEADER_TYPE);
if ((header_type & PCI_HEADER_TYPE_MASK) != PCI_HEADER_TYPE_BRIDGE)
- return 0;
+ goto cleanup;
- secondary = pciRead8(check, PCI_SECONDARY_BUS);
- subordinate = pciRead8(check, PCI_SUBORDINATE_BUS);
+ secondary = pciRead8(check, fd, PCI_SECONDARY_BUS);
+ subordinate = pciRead8(check, fd, PCI_SUBORDINATE_BUS);
VIR_DEBUG("%s %s: found parent device %s", dev->id, dev->name,
check->name);
/* if the secondary bus exactly equals the device's bus, then we found
* the direct parent. No further work is necessary
*/
- if (dev->bus == secondary)
- return 1;
+ if (dev->bus == secondary) {
+ ret = 1;
+ goto cleanup;
+ }
/* otherwise, SRIOV allows VFs to be on different busses then their PFs.
* In this case, what we need to do is look for the "best" match; i.e.
@@ -557,25 +572,38 @@ pciIsParent(pciDevice *dev, pciDevice *check, void *data)
if (*best == NULL) {
*best = pciGetDevice(check->domain, check->bus, check->slot,
check->function);
- if (*best == NULL)
- return -1;
- }
- else {
+ if (*best == NULL) {
+ ret = -1;
+ goto cleanup;
+ }
+ } else {
/* OK, we had already recorded a previous "best" match for the
* parent. See if the current device is more restrictive than the
* best, and if so, make it the new best
*/
- if (secondary > pciRead8(*best, PCI_SECONDARY_BUS)) {
+ int bestfd;
+ uint8_t best_secondary;
+
+ if ((bestfd = pciConfigOpen(*best, false)) < 0)
+ goto cleanup;
+ best_secondary = pciRead8(*best, bestfd, PCI_SECONDARY_BUS);
+ pciConfigClose(*best, bestfd);
+
+ if (secondary > best_secondary) {
pciFreeDevice(*best);
*best = pciGetDevice(check->domain, check->bus, check->slot,
check->function);
- if (*best == NULL)
- return -1;
+ if (*best == NULL) {
+ ret = -1;
+ goto cleanup;
+ }
}
}
}
- return 0;
+cleanup:
+ pciConfigClose(check, fd);
+ return ret;
}
static int
@@ -598,12 +626,14 @@ pciGetParentDevice(pciDevice *dev, pciDevice **parent)
*/
static int
pciTrySecondaryBusReset(pciDevice *dev,
+ int cfgfd,
pciDeviceList *inactiveDevs)
{
pciDevice *parent, *conflict;
uint8_t config_space[PCI_CONF_LEN];
uint16_t ctl;
int ret = -1;
+ int parentfd;
/* Refuse to do a secondary bus reset if there are other
* devices/functions behind the bus are used by the host
@@ -625,6 +655,8 @@ pciTrySecondaryBusReset(pciDevice *dev,
dev->name);
return -1;
}
+ if ((parentfd = pciConfigOpen(parent, true)) < 0)
+ goto out;
VIR_DEBUG("%s %s: doing a secondary bus reset", dev->id, dev->name);
@@ -632,7 +664,7 @@ pciTrySecondaryBusReset(pciDevice *dev,
* for the supplied device since we refuse to do a reset if there
* are multiple devices/functions
*/
- if (pciRead(dev, 0, config_space, PCI_CONF_LEN) < 0) {
+ if (pciRead(dev, cfgfd, 0, config_space, PCI_CONF_LEN) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to read PCI config space for %s"),
dev->name);
@@ -642,24 +674,27 @@ pciTrySecondaryBusReset(pciDevice *dev,
/* Read the control register, set the reset flag, wait 200ms,
* unset the reset flag and wait 200ms.
*/
- ctl = pciRead16(dev, PCI_BRIDGE_CONTROL);
+ ctl = pciRead16(dev, cfgfd, PCI_BRIDGE_CONTROL);
- pciWrite16(parent, PCI_BRIDGE_CONTROL, ctl | PCI_BRIDGE_CTL_RESET);
+ pciWrite16(parent, parentfd, PCI_BRIDGE_CONTROL,
+ ctl | PCI_BRIDGE_CTL_RESET);
usleep(200 * 1000); /* sleep 200ms */
- pciWrite16(parent, PCI_BRIDGE_CONTROL, ctl);
+ pciWrite16(parent, parentfd, PCI_BRIDGE_CONTROL, ctl);
usleep(200 * 1000); /* sleep 200ms */
- if (pciWrite(dev, 0, config_space, PCI_CONF_LEN) < 0) {
+ if (pciWrite(dev, cfgfd, 0, config_space, PCI_CONF_LEN) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to restore PCI config space for %s"),
dev->name);
goto out;
}
ret = 0;
+
out:
+ pciConfigClose(parent, parentfd);
pciFreeDevice(parent);
return ret;
}
@@ -669,7 +704,7 @@ out:
* above we require the device supports a full internal reset.
*/
static int
-pciTryPowerManagementReset(pciDevice *dev)
+pciTryPowerManagementReset(pciDevice *dev, int cfgfd)
{
uint8_t config_space[PCI_CONF_LEN];
uint32_t ctl;
@@ -678,7 +713,7 @@ pciTryPowerManagementReset(pciDevice *dev)
return -1;
/* Save and restore the device's config space. */
- if (pciRead(dev, 0, &config_space[0], PCI_CONF_LEN) < 0) {
+ if (pciRead(dev, cfgfd, 0, &config_space[0], PCI_CONF_LEN) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to read PCI config space for %s"),
dev->name);
@@ -687,18 +722,20 @@ pciTryPowerManagementReset(pciDevice *dev)
VIR_DEBUG("%s %s: doing a power management reset", dev->id,
dev->name);
- ctl = pciRead32(dev, dev->pci_pm_cap_pos + PCI_PM_CTRL);
+ ctl = pciRead32(dev, cfgfd, dev->pci_pm_cap_pos + PCI_PM_CTRL);
ctl &= ~PCI_PM_CTRL_STATE_MASK;
- pciWrite32(dev, dev->pci_pm_cap_pos + PCI_PM_CTRL, ctl|PCI_PM_CTRL_STATE_D3hot);
+ pciWrite32(dev, cfgfd, dev->pci_pm_cap_pos + PCI_PM_CTRL,
+ ctl | PCI_PM_CTRL_STATE_D3hot);
usleep(10 * 1000); /* sleep 10ms */
- pciWrite32(dev, dev->pci_pm_cap_pos + PCI_PM_CTRL, ctl|PCI_PM_CTRL_STATE_D0);
+ pciWrite32(dev, cfgfd, dev->pci_pm_cap_pos + PCI_PM_CTRL,
+ ctl | PCI_PM_CTRL_STATE_D0);
usleep(10 * 1000); /* sleep 10ms */
- if (pciWrite(dev, 0, &config_space[0], PCI_CONF_LEN) < 0) {
+ if (pciWrite(dev, cfgfd, 0, &config_space[0], PCI_CONF_LEN) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to restore PCI config space for %s"),
dev->name);
@@ -709,25 +746,18 @@ pciTryPowerManagementReset(pciDevice *dev)
}
static int
-pciInitDevice(pciDevice *dev)
+pciInitDevice(pciDevice *dev, int cfgfd)
{
int flr;
- if (pciOpenConfig(dev) < 0) {
- virReportSystemError(errno,
- _("Failed to open config space file
'%s'"),
- dev->path);
- return -1;
- }
-
- dev->pcie_cap_pos = pciFindCapabilityOffset(dev, PCI_CAP_ID_EXP);
- dev->pci_pm_cap_pos = pciFindCapabilityOffset(dev, PCI_CAP_ID_PM);
- flr = pciDetectFunctionLevelReset(dev);
+ dev->pcie_cap_pos = pciFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_EXP);
+ dev->pci_pm_cap_pos = pciFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_PM);
+ flr = pciDetectFunctionLevelReset(dev, cfgfd);
if (flr < 0)
return flr;
dev->has_flr = flr;
- dev->has_pm_reset = pciDetectPowerManagementReset(dev);
- dev->initted = 1;
+ dev->has_pm_reset = pciDetectPowerManagementReset(dev, cfgfd);
+
return 0;
}
@@ -737,6 +767,7 @@ pciResetDevice(pciDevice *dev,
pciDeviceList *inactiveDevs)
{
int ret = -1;
+ int fd;
if (activeDevs && pciDeviceListFind(activeDevs, dev)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -744,25 +775,30 @@ pciResetDevice(pciDevice *dev,
return -1;
}
- if (!dev->initted && pciInitDevice(dev) < 0)
+ if ((fd = pciConfigOpen(dev, true)) < 0)
return -1;
+ if (pciInitDevice(dev, fd) < 0)
+ goto cleanup;
+
/* KVM will perform FLR when starting and stopping
* a guest, so there is no need for us to do it here.
*/
- if (dev->has_flr)
- return 0;
+ if (dev->has_flr) {
+ ret = 0;
+ goto cleanup;
+ }
/* If the device supports PCI power management reset,
* that's the next best thing because it only resets
* the function, not the whole device.
*/
if (dev->has_pm_reset)
- ret = pciTryPowerManagementReset(dev);
+ ret = pciTryPowerManagementReset(dev, fd);
/* Bus reset is not an option with the root bus */
if (ret < 0 && dev->bus != 0)
- ret = pciTrySecondaryBusReset(dev, inactiveDevs);
+ ret = pciTrySecondaryBusReset(dev, fd, inactiveDevs);
if (ret < 0) {
virErrorPtr err = virGetLastError();
@@ -772,6 +808,8 @@ pciResetDevice(pciDevice *dev,
err ? err->message : _("no FLR, PM reset or bus reset
available"));
}
+cleanup:
+ pciConfigClose(dev, fd);
return ret;
}
@@ -1342,7 +1380,6 @@ pciGetDevice(unsigned domain,
return NULL;
}
- dev->fd = -1;
dev->domain = domain;
dev->bus = bus;
dev->slot = slot;
@@ -1407,7 +1444,6 @@ pciFreeDevice(pciDevice *dev)
if (!dev)
return;
VIR_DEBUG("%s %s: freeing", dev->id, dev->name);
- pciCloseConfig(dev);
VIR_FREE(dev->path);
VIR_FREE(dev);
}
@@ -1677,32 +1713,43 @@ pciDeviceDownstreamLacksACS(pciDevice *dev)
uint16_t flags;
uint16_t ctrl;
unsigned int pos;
+ int fd;
+ int ret = 0;
- if (!dev->initted && pciInitDevice(dev) < 0)
+ if ((fd = pciConfigOpen(dev, true)) < 0)
return -1;
+ if (pciInitDevice(dev, fd) < 0) {
+ ret = -1;
+ goto cleanup;
+ }
+
pos = dev->pcie_cap_pos;
- if (!pos || pciRead16(dev, PCI_CLASS_DEVICE) != PCI_CLASS_BRIDGE_PCI)
- return 0;
+ if (!pos || pciRead16(dev, fd, PCI_CLASS_DEVICE) != PCI_CLASS_BRIDGE_PCI)
+ goto cleanup;
- flags = pciRead16(dev, pos + PCI_EXP_FLAGS);
+ flags = pciRead16(dev, fd, pos + PCI_EXP_FLAGS);
if (((flags & PCI_EXP_FLAGS_TYPE) >> 4) != PCI_EXP_TYPE_DOWNSTREAM)
- return 0;
+ goto cleanup;
- pos = pciFindExtendedCapabilityOffset(dev, PCI_EXT_CAP_ID_ACS);
+ pos = pciFindExtendedCapabilityOffset(dev, fd, PCI_EXT_CAP_ID_ACS);
if (!pos) {
VIR_DEBUG("%s %s: downstream port lacks ACS", dev->id,
dev->name);
- return 1;
+ ret = 1;
+ goto cleanup;
}
- ctrl = pciRead16(dev, pos + PCI_EXT_ACS_CTRL);
+ ctrl = pciRead16(dev, fd, pos + PCI_EXT_ACS_CTRL);
if ((ctrl & PCI_EXT_CAP_ACS_ENABLED) != PCI_EXT_CAP_ACS_ENABLED) {
VIR_DEBUG("%s %s: downstream port has ACS disabled",
dev->id, dev->name);
- return 1;
+ ret = 1;
+ goto cleanup;
}
- return 0;
+cleanup:
+ pciConfigClose(dev, fd);
+ return ret;
}
static int
--
1.8.0
8:42 p.m.
New subject: [libvirt] [PATCH v2 4/4] util: Do not keep PCI device config file open
在 2012-12-04二的 23:23 +0100，Jiri Denemark写道：
Directly open and close PCI config file in the APIs that need it
rather
than keeping the file open for the whole life of PCI device structure.
---
src/util/pci.c | 265 +++++++++++++++++++++++++++++++++------------------------
1 file changed, 156 insertions(+), 109 deletions(-)
diff --git a/src/util/pci.c b/src/util/pci.c
index e410245..8bded78 100644
[snip]
/* otherwise, SRIOV allows VFs to be on different busses then
their PFs.
* In this case, what we need to do is look for the "best" match; i.e.
@@ -557,25 +572,38 @@ pciIsParent(pciDevice *dev, pciDevice *check, void *data)
if (*best == NULL) {
*best = pciGetDevice(check->domain, check->bus, check->slot,
check->function);
- if (*best == NULL)
- return -1;
- }
- else {
-- ** --
+ if (*best == NULL) {
+ ret = -1;
+ goto cleanup;
+ }
--first--
+ } else {
/* OK, we had already recorded a previous "best" match for the
* parent. See if the current device is more restrictive than the
* best, and if so, make it the new best
*/
- if (secondary > pciRead8(*best, PCI_SECONDARY_BUS)) {
+ int bestfd;
+ uint8_t best_secondary;
+
+ if ((bestfd = pciConfigOpen(*best, false)) < 0)
+ goto cleanup;
+ best_secondary = pciRead8(*best, bestfd, PCI_SECONDARY_BUS);
+ pciConfigClose(*best, bestfd);
+
+ if (secondary > best_secondary) {
pciFreeDevice(*best);
*best = pciGetDevice(check->domain, check->bus, check->slot,
check->function);
- if (*best == NULL)
- return -1;
-- ** --
+ if (*best == NULL) {
+ ret = -1;
+ goto cleanup;
+ }
--second--
}
}
logically, the 2 'if (*best == NULL) {'
can be combined and plcaced here.
}
- return 0;
+cleanup:
+ pciConfigClose(check, fd);
+ return ret;
}
static int
@@ -598,12 +626,14 @@ pciGetParentDevice(pciDevice *dev, pciDevice **parent)
*/
[snip]
@@ -669,7 +704,7 @@ out:
* above we require the device supports a full internal reset.
*/
static int
-pciTryPowerManagementReset(pciDevice *dev)
+pciTryPowerManagementReset(pciDevice *dev, int cfgfd)
{
uint8_t config_space[PCI_CONF_LEN];
uint32_t ctl;
@@ -678,7 +713,7 @@ pciTryPowerManagementReset(pciDevice *dev)
return -1;
/* Save and restore the device's config space. */
- if (pciRead(dev, 0, &config_space[0], PCI_CONF_LEN) < 0) {
+ if (pciRead(dev, cfgfd, 0, &config_space[0], PCI_CONF_LEN) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to read PCI config space for %s"),
dev->name);
@@ -687,18 +722,20 @@ pciTryPowerManagementReset(pciDevice *dev)
VIR_DEBUG("%s %s: doing a power management reset", dev->id,
dev->name);
- ctl = pciRead32(dev, dev->pci_pm_cap_pos + PCI_PM_CTRL);
+ ctl = pciRead32(dev, cfgfd, dev->pci_pm_cap_pos + PCI_PM_CTRL);
ctl &= ~PCI_PM_CTRL_STATE_MASK;
- pciWrite32(dev, dev->pci_pm_cap_pos + PCI_PM_CTRL, ctl|PCI_PM_CTRL_STATE_D3hot);
seems more than 80 characters
+ pciWrite32(dev, cfgfd, dev->pci_pm_cap_pos + PCI_PM_CTRL,
+ ctl | PCI_PM_CTRL_STATE_D3hot);
usleep(10 * 1000); /* sleep 10ms */
- pciWrite32(dev, dev->pci_pm_cap_pos + PCI_PM_CTRL, ctl|PCI_PM_CTRL_STATE_D0);
+ pciWrite32(dev, cfgfd, dev->pci_pm_cap_pos + PCI_PM_CTRL,
+ ctl | PCI_PM_CTRL_STATE_D0);
usleep(10 * 1000); /* sleep 10ms */
- if (pciWrite(dev, 0, &config_space[0], PCI_CONF_LEN) < 0) {
+ if (pciWrite(dev, cfgfd, 0, &config_space[0], PCI_CONF_LEN) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to restore PCI config space for %s"),
dev->name);
[snip]
--
regards!
li guang
5 a.m.
New subject: [libvirt] [PATCH v2 4/4] util: Do not keep PCI device config file open
On Wed, Dec 05, 2012 at 10:42:14 +0800, li guang wrote:
在 2012-12-04二的 23:23 +0100，Jiri Denemark写道：
...
> @@ -557,25 +572,38 @@ pciIsParent(pciDevice *dev, pciDevice
*check, void *data)
> if (*best == NULL) {
> *best = pciGetDevice(check->domain, check->bus, check->slot,
> check->function);
> - if (*best == NULL)
> - return -1;
> - }
> - else {
-- ** --
> + if (*best == NULL) {
> + ret = -1;
> + goto cleanup;
> + }
--first--
> + } else {
> /* OK, we had already recorded a previous "best" match for
the
> * parent. See if the current device is more restrictive than the
> * best, and if so, make it the new best
> */
> - if (secondary > pciRead8(*best, PCI_SECONDARY_BUS)) {
> + int bestfd;
> + uint8_t best_secondary;
> +
> + if ((bestfd = pciConfigOpen(*best, false)) < 0)
> + goto cleanup;
> + best_secondary = pciRead8(*best, bestfd, PCI_SECONDARY_BUS);
> + pciConfigClose(*best, bestfd);
> +
> + if (secondary > best_secondary) {
> pciFreeDevice(*best);
> *best = pciGetDevice(check->domain, check->bus,
check->slot,
> check->function);
> - if (*best == NULL)
> - return -1;
-- ** --
> + if (*best == NULL) {
> + ret = -1;
> + goto cleanup;
> + }
--second--
> }
> }
logically, the 2 'if (*best == NULL) {'
can be combined and plcaced here.
Yes, they can be. But the purpose of this patch was to change the way PCI
config files are used. Combining that with other changes would make the patch
(which is already pretty big) harder to review.
...
> @@ -687,18 +722,20 @@ pciTryPowerManagementReset(pciDevice
*dev)
>
> VIR_DEBUG("%s %s: doing a power management reset", dev->id,
dev->name);
>
> - ctl = pciRead32(dev, dev->pci_pm_cap_pos + PCI_PM_CTRL);
> + ctl = pciRead32(dev, cfgfd, dev->pci_pm_cap_pos + PCI_PM_CTRL);
> ctl &= ~PCI_PM_CTRL_STATE_MASK;
>
> - pciWrite32(dev, dev->pci_pm_cap_pos + PCI_PM_CTRL,
ctl|PCI_PM_CTRL_STATE_D3hot);
seems more than 80 characters
Well, the line is being replaced with the following to lines by this patch:
> + pciWrite32(dev, cfgfd, dev->pci_pm_cap_pos +
PCI_PM_CTRL,
> + ctl | PCI_PM_CTRL_STATE_D3hot);
Jirka
5:34 a.m.
New subject: [libvirt] [PATCH v2 4/4] util: Do not keep PCI device config file open
On 12/04/12 23:23, Jiri Denemark wrote:
Directly open and close PCI config file in the APIs that need it
rather
than keeping the file open for the whole life of PCI device structure.
---
src/util/pci.c | 265 +++++++++++++++++++++++++++++++++------------------------
1 file changed, 156 insertions(+), 109 deletions(-)
diff --git a/src/util/pci.c b/src/util/pci.c
index e410245..8bded78 100644
--- a/src/util/pci.c
+++ b/src/util/pci.c
static void
-pciWrite32(pciDevice *dev, unsigned pos, uint32_t val)
+pciWrite32(pciDevice *dev, int cfgfd, unsigned pos, uint32_t val)
{
uint8_t buf[4] = { (val >> 0), (val >> 8), (val >> 16), (val
>> 14) };
EW! this is a serious bug! Moving the value by 14 bits. I'll post a
patch for this as it's not relevant for this patch.
- pciWrite(dev, pos, &buf[0], sizeof(buf));
+ pciWrite(dev, cfgfd, pos, &buf[0], sizeof(buf));
}
typedef int (*pciIterPredicate)(pciDevice *, pciDevice *, void *);
@@ -343,16 +349,16 @@ pciIterDevices(pciIterPredicate predicate,
}
@@ -687,18 +722,20 @@ pciTryPowerManagementReset(pciDevice *dev)
VIR_DEBUG("%s %s: doing a power management reset", dev->id,
dev->name);
- ctl = pciRead32(dev, dev->pci_pm_cap_pos + PCI_PM_CTRL);
+ ctl = pciRead32(dev, cfgfd, dev->pci_pm_cap_pos + PCI_PM_CTRL);
ctl &= ~PCI_PM_CTRL_STATE_MASK;
- pciWrite32(dev, dev->pci_pm_cap_pos + PCI_PM_CTRL, ctl|PCI_PM_CTRL_STATE_D3hot);
+ pciWrite32(dev, cfgfd, dev->pci_pm_cap_pos + PCI_PM_CTRL,
+ ctl | PCI_PM_CTRL_STATE_D3hot);
And the issue pointed up would hit us right here.
usleep(10 * 1000); /* sleep 10ms */
- pciWrite32(dev, dev->pci_pm_cap_pos + PCI_PM_CTRL, ctl|PCI_PM_CTRL_STATE_D0);
+ pciWrite32(dev, cfgfd, dev->pci_pm_cap_pos + PCI_PM_CTRL,
+ ctl | PCI_PM_CTRL_STATE_D0);
usleep(10 * 1000); /* sleep 10ms */
- if (pciWrite(dev, 0, &config_space[0], PCI_CONF_LEN) < 0) {
+ if (pciWrite(dev, cfgfd, 0, &config_space[0], PCI_CONF_LEN) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to restore PCI config space for %s"),
dev->name);
ACK.
Peter
7:02 a.m.
On Tue, Dec 04, 2012 at 11:38:18 +0100, Jiri Denemark wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=877095
Patch 1/4 fixes a possible double free on error path and the other patches
deal with memory and file descriptor leaks in PCI device attachment code.
Jiri Denemark (4):
qemu: Don't free PCI device if adding it to activePciHostdevs fails
util: Slightly refactor PCI list functions
qemu: Fix memory (and FD) leak on PCI device detach
qemu: Do not keep PCI device config file open
I fixed the issue in 2/4 found by Osier and pushed. Thanks.
Jirka
11 a.m.
On 12/04/2012 05:38 AM, Jiri Denemark wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=877095
Patch 1/4 fixes a possible double free on error path and the other patches
deal with memory and file descriptor leaks in PCI device attachment code.
Jiri Denemark (4):
qemu: Don't free PCI device if adding it to activePciHostdevs fails
util: Slightly refactor PCI list functions
qemu: Fix memory (and FD) leak on PCI device detach
qemu: Do not keep PCI device config file open
src/libvirt_private.syms | 3 +++
src/qemu/qemu_hostdev.c | 22 ++++++++--------
src/util/pci.c | 66 ++++++++++++++++++++++++++++--------------------
src/util/pci.h | 5 ++++
4 files changed, 58 insertions(+), 38 deletions(-)
I haven't looked at the individual patches, but just a comment of
something to look out for if you're going to be doing any changes to the
API as Dan suggested:
The network driver can have pools of physical netdevs that it allocates
to guests either for use with macvtap or pci passthrough. Currently
these pools of netdevs are self-contained and ignorant of the world
around them, but it would be very nice if libvirt could have a unified
idea of what host devices are in use so that, for example, a device
could be listed in multiple netdev pools, but would then not be blindly
assigned to a guest if it was already in use via a different network's
pool (or via standard hostdev assignment). Likewise, if someone tried to
do a hostdev assignment of a network device that was already in use by a
guest for a macvtap network interface, that would be caught and prevented.
I'm not suggesting that any of that work is done here, but just to keep
that in mind when making any changes.
On top of all this is the idea that we've tossed around about giving the
networking functions a semi-public API and perhaps even their own daemon
so that they can be easily called from an unprivileged libvirtd - if we
really do this by putting the network functions in a separate daemon,
that daemon would need access back to the centralized pci functions
(which maybe argues for keeping the networking functions in libvirtd
itself, but giving them public APIs and teaching unprivileged libvirtd
to call privileged libvirtd when it needs those functions. But now I'm
seriously hijacking this thread, so I'll shut up :-)
4371
days inactive
4373
days old
20 comments
7 participants
participants (7)
-
Daniel P. Berrange -
jd >> Jiri Denemark -
Jiri Denemark
-
Laine Stump -
li guang -
Osier Yang -
Peter Krempa