[libvirt] [PATCHv2 0/2] pci: make virPCIDeviceReset more autonomous
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
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
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
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.
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
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
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(-)
participants (2)
-
Laine Stump -
Martin Kletzander