12:46 p.m.
This replaces the earlier single patch by the same name (which I somehow managed to send
without testing it :-()
This time it's split into two patches because some of the static
functions in virpci.c needed re-ordering for successful compilation,
so I made that movement a separate patch.
Laine Stump (2):
pci: reorder static functions
pci: make virPCIDeviceReset more autonomous
src/qemu/qemu_hostdev.c | 6 +-
src/qemu/qemu_hotplug.c | 5 +-
src/util/virpci.c | 204 +++++++++++++++++++++++++++---------------------
3 files changed, 118 insertions(+), 97 deletions(-)
--
1.7.11.7
12:46 p.m.
New subject: [libvirt] [PATCHv2 1/2] pci: reorder static functions
virPCIDeviceGetDriverPathAndName is a static function that will need
to be called by another function that occurs above it in the
file. This patch reorders the static functions so that a forward
declaration isn't needed.
---
src/util/virpci.c | 177 +++++++++++++++++++++++++++---------------------------
1 file changed, 90 insertions(+), 87 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 54f7715..fc76952 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -188,6 +188,96 @@ static int virPCIOnceInit(void)
VIR_ONCE_GLOBAL_INIT(virPCI)
+
+static int
+virPCIDriverDir(char **buffer, const char *driver)
+{
+ VIR_FREE(*buffer);
+
+ if (virAsprintf(buffer, PCI_SYSFS "drivers/%s", driver) < 0) {
+ virReportOOMError();
+ return -1;
+ }
+
+ return 0;
+}
+
+
+static int
+virPCIDriverFile(char **buffer, const char *driver, const char *file)
+{
+ VIR_FREE(*buffer);
+
+ if (virAsprintf(buffer, PCI_SYSFS "drivers/%s/%s", driver, file) < 0) {
+ virReportOOMError();
+ return -1;
+ }
+
+ return 0;
+}
+
+
+static int
+virPCIFile(char **buffer, const char *device, const char *file)
+{
+ VIR_FREE(*buffer);
+
+ if (virAsprintf(buffer, PCI_SYSFS "devices/%s/%s", device, file) < 0) {
+ virReportOOMError();
+ return -1;
+ }
+
+ return 0;
+}
+
+
+/* virPCIDeviceGetDriverPathAndName - put the path to the driver
+ * directory of the driver in use for this device in @path and the
+ * name of the driver in @name. Both could be NULL if it's not bound
+ * to any driver.
+ *
+ * Return 0 for success, -1 for error.
+ */
+static int
+virPCIDeviceGetDriverPathAndName(virPCIDevicePtr dev, char **path, char **name)
+{
+ int ret = -1;
+ char *drvlink = NULL;
+
+ *path = *name = NULL;
+ /* drvlink = "/sys/bus/pci/dddd:bb:ss.ff/driver" */
+ if (virPCIFile(&drvlink, dev->name, "driver") < 0)
+ goto cleanup;
+
+ if (virFileIsLink(drvlink) != 1) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid device %s driver file %s is not a symlink"),
+ dev->name, drvlink);
+ goto cleanup;
+ }
+ if (virFileResolveLink(drvlink, path) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to resolve device %s driver symlink %s"),
+ dev->name, drvlink);
+ goto cleanup;
+ }
+ /* path = "/sys/bus/pci/drivers/${drivername}" */
+
+ if (VIR_STRDUP(*name, last_component(*path)) < 0)
+ goto cleanup;
+ /* name = "${drivername}" */
+
+ ret = 0;
+cleanup:
+ VIR_FREE(drvlink);
+ if (ret < 0) {
+ VIR_FREE(*path);
+ VIR_FREE(*name);
+ }
+ return ret;
+}
+
+
static int
virPCIDeviceConfigOpen(virPCIDevicePtr dev, bool fatal)
{
@@ -842,93 +932,6 @@ cleanup:
static int
-virPCIDriverDir(char **buffer, const char *driver)
-{
- VIR_FREE(*buffer);
-
- if (virAsprintf(buffer, PCI_SYSFS "drivers/%s", driver) < 0) {
- virReportOOMError();
- return -1;
- }
-
- return 0;
-}
-
-static int
-virPCIDriverFile(char **buffer, const char *driver, const char *file)
-{
- VIR_FREE(*buffer);
-
- if (virAsprintf(buffer, PCI_SYSFS "drivers/%s/%s", driver, file) < 0) {
- virReportOOMError();
- return -1;
- }
-
- return 0;
-}
-
-static int
-virPCIFile(char **buffer, const char *device, const char *file)
-{
- VIR_FREE(*buffer);
-
- if (virAsprintf(buffer, PCI_SYSFS "devices/%s/%s", device, file) < 0) {
- virReportOOMError();
- return -1;
- }
-
- return 0;
-}
-
-
-/* virPCIDeviceGetDriverPathAndName - put the path to the driver
- * directory of the driver in use for this device in @path and the
- * name of the driver in @name. Both could be NULL if it's not bound
- * to any driver.
- *
- * Return 0 for success, -1 for error.
- */
-static int
-virPCIDeviceGetDriverPathAndName(virPCIDevicePtr dev, char **path, char **name)
-{
- int ret = -1;
- char *drvlink = NULL;
-
- *path = *name = NULL;
- /* drvlink = "/sys/bus/pci/dddd:bb:ss.ff/driver" */
- if (virPCIFile(&drvlink, dev->name, "driver") < 0)
- goto cleanup;
-
- if (virFileIsLink(drvlink) != 1) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Invalid device %s driver file %s is not a symlink"),
- dev->name, drvlink);
- goto cleanup;
- }
- if (virFileResolveLink(drvlink, path) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unable to resolve device %s driver symlink %s"),
- dev->name, drvlink);
- goto cleanup;
- }
- /* path = "/sys/bus/pci/drivers/${drivername}" */
-
- if (VIR_STRDUP(*name, last_component(*path)) < 0)
- goto cleanup;
- /* name = "${drivername}" */
-
- ret = 0;
-cleanup:
- VIR_FREE(drvlink);
- if (ret < 0) {
- VIR_FREE(*path);
- VIR_FREE(*name);
- }
- return ret;
-}
-
-
-static int
virPCIProbeStubDriver(const char *driver)
{
char *drvpath = NULL;
--
1.7.11.7
3:39 a.m.
New subject: [libvirt] [PATCHv2 1/2] pci: reorder static functions
On 07/01/2013 07:46 PM, Laine Stump wrote:
virPCIDeviceGetDriverPathAndName is a static function that will need
to be called by another function that occurs above it in the
file. This patch reorders the static functions so that a forward
declaration isn't needed.
---
src/util/virpci.c | 177 +++++++++++++++++++++++++++---------------------------
1 file changed, 90 insertions(+), 87 deletions(-)
Most of the time I was looking for those 3 lines added, but found out
those are blanks. Pure movement, ACK.
Martin
9:24 a.m.
New subject: [libvirt] [PATCHv2 1/2] pci: reorder static functions
On 07/15/2013 04:39 AM, Martin Kletzander wrote:
On 07/01/2013 07:46 PM, Laine Stump wrote:
> virPCIDeviceGetDriverPathAndName is a static function that will need
> to be called by another function that occurs above it in the
> file. This patch reorders the static functions so that a forward
> declaration isn't needed.
> ---
> src/util/virpci.c | 177 +++++++++++++++++++++++++++---------------------------
> 1 file changed, 90 insertions(+), 87 deletions(-)
>
Most of the time I was looking for those 3 lines added, but found out
those are blanks. Pure movement, ACK.
It would be really cool if git could be taught to show movement of
complete functions in some more compact manner (similar to what it does
when you rename a file). No idea exactly what that would look like though...
Thanks for the review. I'll push as soon as I rebase and re-compile.
12:46 p.m.
New subject: [libvirt] [PATCHv2 2/2] pci: make virPCIDeviceReset more autonomous
I recently patches the callers to virPCIDeviceReset() to not call it
if the current driver for a device was vfio-pci (since that driver
will always reset the device itself when appropriate. At the time, Dan
Berrange suggested that I could instead modify virPCIDeviceReset
to check the currently bound driver for the device, and decide
for itself whether or not to go ahead with the reset.
This patch removes the previously added checks, and replaces them with
a check down in virPCIDeviceReset(), as suggested.
The functional difference here is that previously we were deciding
based on either the hostdev configuration or the value of
stubDriverName in the virPCIDevice object, but now we are actually
comparing to the "driver" link in the device's sysfs entry
directly. In practice, both should be the same.
---
src/qemu/qemu_hostdev.c | 6 ++----
src/qemu/qemu_hotplug.c | 5 ++---
src/util/virpci.c | 27 ++++++++++++++++++++++++---
3 files changed, 28 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 404939e..3001d6b 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -544,8 +544,7 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver,
* can safely reset them */
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
- if (STREQ_NULLABLE(virPCIDeviceGetStubDriver(dev), "vfio-pci"))
- continue;
+
if (virPCIDeviceReset(dev, driver->activePciHostdevs,
driver->inactivePciHostdevs) < 0)
goto reattachdevs;
@@ -1116,8 +1115,7 @@ void qemuDomainReAttachHostdevDevices(virQEMUDriverPtr driver,
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
- if (STREQ_NULLABLE(virPCIDeviceGetStubDriver(dev), "vfio-pci"))
- continue;
+
if (virPCIDeviceReset(dev, driver->activePciHostdevs,
driver->inactivePciHostdevs) < 0) {
virErrorPtr err = virGetLastError();
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 46875ad..5edd36e 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2528,9 +2528,8 @@ qemuDomainDetachHostPciDevice(virQEMUDriverPtr driver,
if (pci) {
activePci = virPCIDeviceListSteal(driver->activePciHostdevs, pci);
if (activePci &&
- (subsys->u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO ||
- virPCIDeviceReset(activePci, driver->activePciHostdevs,
- driver->inactivePciHostdevs) == 0)) {
+ virPCIDeviceReset(activePci, driver->activePciHostdevs,
+ driver->inactivePciHostdevs) == 0) {
qemuReattachPciDevice(activePci, driver);
ret = 0;
} else {
diff --git a/src/util/virpci.c b/src/util/virpci.c
index fc76952..15de3f9 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -883,8 +883,10 @@ virPCIDeviceReset(virPCIDevicePtr dev,
virPCIDeviceList *activeDevs,
virPCIDeviceList *inactiveDevs)
{
+ char *drvPath = NULL;
+ char *drvName = NULL;
int ret = -1;
- int fd;
+ int fd = -1;
if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -892,8 +894,24 @@ virPCIDeviceReset(virPCIDevicePtr dev,
return -1;
}
+ /* If the device is currently bound to vfio-pci, ignore all
+ * requests to reset it, since the vfio-pci driver will always
+ * reset it whenever appropriate, so doing it ourselves would just
+ * be redundant.
+ */
+ if (virPCIDeviceGetDriverPathAndName(dev, &drvPath, &drvName) < 0)
+ goto cleanup;
+
+ if (STREQ_NULLABLE(drvName, "vfio-pci")) {
+ VIR_DEBUG("Device %s is bound to vfio-pci - skip reset",
+ dev->name);
+ ret = 0;
+ goto cleanup;
+ }
+ VIR_DEBUG("Resetting device %s", dev->name);
+
if ((fd = virPCIDeviceConfigOpen(dev, true)) < 0)
- return -1;
+ goto cleanup;
if (virPCIDeviceInit(dev, fd) < 0)
goto cleanup;
@@ -922,10 +940,13 @@ virPCIDeviceReset(virPCIDevicePtr dev,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to reset PCI device %s: %s"),
dev->name,
- err ? err->message : _("no FLR, PM reset or bus reset
available"));
+ err ? err->message :
+ _("no FLR, PM reset or bus reset available"));
}
cleanup:
+ VIR_FREE(drvPath);
+ VIR_FREE(drvName);
virPCIDeviceConfigClose(dev, fd);
return ret;
}
--
1.7.11.7
3:39 a.m.
New subject: [libvirt] [PATCHv2 2/2] pci: make virPCIDeviceReset more autonomous
On 07/01/2013 07:46 PM, Laine Stump wrote:
I recently patches the callers to virPCIDeviceReset() to not call it
if the current driver for a device was vfio-pci (since that driver
will always reset the device itself when appropriate. At the time, Dan
Berrange suggested that I could instead modify virPCIDeviceReset
to check the currently bound driver for the device, and decide
for itself whether or not to go ahead with the reset.
This patch removes the previously added checks, and replaces them with
a check down in virPCIDeviceReset(), as suggested.
The functional difference here is that previously we were deciding
based on either the hostdev configuration or the value of
stubDriverName in the virPCIDevice object, but now we are actually
comparing to the "driver" link in the device's sysfs entry
directly. In practice, both should be the same.
---
src/qemu/qemu_hostdev.c | 6 ++----
src/qemu/qemu_hotplug.c | 5 ++---
src/util/virpci.c | 27 ++++++++++++++++++++++++---
3 files changed, 28 insertions(+), 10 deletions(-)
This is better as it doesn't duplicate the code that much and makes us
sure that the reset won't be done redundantly.
ACK,
Martin
7:22 p.m.
Ping.
On 07/01/2013 01:46 PM, Laine Stump wrote:
This replaces the earlier single patch by the same name (which I
somehow managed to send without testing it :-()
This time it's split into two patches because some of the static
functions in virpci.c needed re-ordering for successful compilation,
so I made that movement a separate patch.
Laine Stump (2):
pci: reorder static functions
pci: make virPCIDeviceReset more autonomous
src/qemu/qemu_hostdev.c | 6 +-
src/qemu/qemu_hotplug.c | 5 +-
src/util/virpci.c | 204 +++++++++++++++++++++++++++---------------------
3 files changed, 118 insertions(+), 97 deletions(-)
4149
days inactive
4163
days old
6 comments
2 participants
participants (2)
-
Laine Stump -
Martin Kletzander