[libvirt] [PATCH 2/2] virpci: support driver_override sysfs interface
Laine Stump
laine at laine.org
Fri Jul 29 17:43:25 UTC 2016
On 07/11/2016 02:23 PM, Jim Fehlig wrote:
> Currently, libvirt uses the new_id PCI sysfs interface to bind a PCI
> stub driver to a PCI device. The new_id interface is known to be
> buggy and racey, hence a more deterministic interface was introduced
> in the 3.12 kernel - driver_override. For more details see
>
> https://www.redhat.com/archives/libvir-list/2016-June/msg02124.html
>
> This patch changes the stub binding/unbinding code to use the
> driver_override interface if present. If not present, the new_id
> interface will be used to provide compatibility with older kernels
> lacking driver_override.
>
> Signed-off-by: Jim Fehlig <jfehlig at suse.com>
> ---
> src/util/virpci.c | 199 +++++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 152 insertions(+), 47 deletions(-)
>
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 127b3b6..3c2fc9f 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -1158,6 +1158,19 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
>
> VIR_DEBUG("Reprobing for PCI device %s", dev->name);
>
> + /* Remove driver_override if it exists */
> + VIR_FREE(path);
> + if (!(path = virPCIFile(dev->name, "driver_override")))
> + goto cleanup;
> +
> + if (virFileExists(path) && virFileWriteStr(path, "\n", 0) < 0) {
> + virReportSystemError(errno,
> + _("Failed to remove stub driver from "
> + "driver_override interface of PCI device '%s'"),
> + dev->name);
> + goto cleanup;
> + }
> +
For unbinding the stub, you're just adding in the "clearing" of
driver_override, without eliminating any of the other stuff that's done
for the older deprecated method. If the driver_override file exists, you
shouldn't do *any* of the old operations, just do what is described in
the commit log message of the patch that added driver_override to the
kernel:
https://www.redhat.com/archives/libvir-list/2014-May/msg00670.html
I'll review the rest of it based on what ends up in the code rather than
the diffs, since the diffs will be messed up when you get rid of patch
1/2...
> /* Trigger a re-probe of the device is not in the stub's dynamic
> * ID table. If the stub is available, but 'remove_id' isn't
> * available, then re-probing would just cause the device to be
> @@ -1193,49 +1206,13 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
>
>
> static int
> -virPCIDeviceBindToStub(virPCIDevicePtr dev)
> +virPCIDeviceBindToStubWithNewid(virPCIDevicePtr dev,
> + const char *stubDriverName)
> {
> - int result = -1;
> - char *stubDriverPath = NULL;
> - char *driverLink = NULL;
> - char *path = NULL; /* reused for different purposes */
> - const char *stubDriverName = NULL;
> + int ret = -1;
> + char *path = NULL;
> virErrorPtr err = NULL;
>
> - /* Check the device is configured to use one of the known stub drivers */
> - if (dev->stubDriver == VIR_PCI_STUB_DRIVER_NONE) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("No stub driver configured for PCI device %s"),
> - dev->name);
> - return -1;
> - } else if (!(stubDriverName = virPCIStubDriverTypeToString(dev->stubDriver))) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Unknown stub driver configured for PCI device %s"),
> - dev->name);
> - return -1;
> - }
> -
> - if (!(stubDriverPath = virPCIDriverDir(stubDriverName)) ||
> - !(driverLink = virPCIFile(dev->name, "driver")))
> - goto cleanup;
> -
> - if (virFileExists(driverLink)) {
> - if (virFileLinkPointsTo(driverLink, stubDriverPath)) {
> - /* The device is already bound to the correct driver */
> - VIR_DEBUG("Device %s is already bound to %s",
> - dev->name, stubDriverName);
> - dev->unbind_from_stub = true;
> - dev->remove_slot = true;
> - result = 0;
> - goto cleanup;
> - }
> - /*
> - * If the device is bound to a driver that is not the stub, we'll
> - * need to reprobe later
> - */
> - dev->reprobe = true;
> - }
> -
> /* Add the PCI device ID to the stub's dynamic ID table;
> * this is needed to allow us to bind the device to the stub.
> * Note: if the device is not currently bound to any driver,
> @@ -1283,7 +1260,7 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev)
> }
> dev->unbind_from_stub = true;
>
> - result = 0;
> + ret = 0;
>
> remove_id:
> err = virSaveLastError();
> @@ -1299,7 +1276,7 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev)
> "cannot be probed again.", dev->id, stubDriverName);
> }
> dev->reprobe = false;
> - result = -1;
> + ret = -1;
> goto cleanup;
> }
>
> @@ -1314,11 +1291,143 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev)
> "cannot be probed again.", dev->id, stubDriverName);
> }
> dev->reprobe = false;
> - result = -1;
> + ret = -1;
> goto cleanup;
> }
>
> cleanup:
> + VIR_FREE(path);
> +
> + if (err)
> + virSetError(err);
> + virFreeError(err);
> +
> + return ret;
> +}
> +
> +
> +static int
> +virPCIDeviceBindToStubWithOverride(virPCIDevicePtr dev,
> + const char *stubDriverName)
> +{
> + int ret = -1;
> + char *path = NULL;
> +
> + /*
> + * Add stub to the device's driver_override, falling back to
> + * adding the device ID to the stub's dynamic ID table.
> + */
> + if (!(path = virPCIFile(dev->name, "driver_override")))
> + return -1;
> +
> + if (virFileWriteStr(path, stubDriverName, 0) < 0) {
> + virReportSystemError(errno,
> + _("Failed to add stub driver '%s' to "
> + "driver_override interface of PCI device '%s'"),
> + stubDriverName, dev->name);
> + goto cleanup;
> + }
> +
> + if (virPCIDeviceUnbind(dev) < 0)
> + goto cleanup;
> +
> + /* Xen's pciback.ko wants you to use new_slot first */
> + VIR_FREE(path);
> + if (!(path = virPCIDriverFile(stubDriverName, "new_slot")))
> + goto cleanup;
What is this "new_slot" stuff all about? Are you certain that it's still
needed when using driver_override, or is it just some lore from the old
method that isn't needed anymore? (I'm unable to test this because I
don't have a host with Xen). The only things that should be needed are:
1) write the stub driver name to driver_override
2) write the device's PCI address to driver/unbind
3) write the device's PCI address to /sys/bus/pci/drivers_probe.
(As a matter of fact, it's exactly the same operation as when binding
back to the host driver, just the string written to driver_override
changes, so there should probably be a common routine that takes the
driver name as an argument - maybe just add driverName to this function
and rename it?).
> +
> + if (virFileExists(path) && virFileWriteStr(path, dev->name, 0) < 0) {
> + virReportSystemError(errno,
> + _("Failed to add slot for "
> + "PCI device '%s' to %s"),
> + dev->name, stubDriverName);
> + goto cleanup;
> + }
> + dev->remove_slot = true;
> +
> + if (virFileWriteStr(PCI_SYSFS "drivers_probe", dev->name, 0) < 0) {
> + virReportSystemError(errno,
> + _("Failed to trigger a re-probe for PCI device '%s'"),
> + dev->name);
> + goto cleanup;
> + }
> +
> + /*
> + * Device is now bound to the stub. Set reprobe so it will be re-bound
> + * when unbinding from the stub.
> + */
> + dev->reprobe = true;
> + dev->unbind_from_stub = true;
> +
> + ret = 0;
> +
> + cleanup:
> + VIR_FREE(path);
> + return ret;
> +}
> +
> +
> +static int
> +virPCIDeviceBindToStub(virPCIDevicePtr dev)
> +{
> + int result = -1;
> + char *stubDriverPath = NULL;
> + char *driverLink = NULL;
> + char *path = NULL; /* reused for different purposes */
> + const char *stubDriverName = NULL;
> +
> + /* Check the device is configured to use one of the known stub drivers */
> + if (dev->stubDriver == VIR_PCI_STUB_DRIVER_NONE) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("No stub driver configured for PCI device %s"),
> + dev->name);
> + return -1;
> + } else if (!(stubDriverName = virPCIStubDriverTypeToString(dev->stubDriver))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unknown stub driver configured for PCI device %s"),
> + dev->name);
> + return -1;
> + }
> +
> + if (!(stubDriverPath = virPCIDriverDir(stubDriverName)) ||
> + !(driverLink = virPCIFile(dev->name, "driver")))
> + goto cleanup;
> +
> + if (virFileExists(driverLink)) {
> + if (virFileLinkPointsTo(driverLink, stubDriverPath)) {
> + /* The device is already bound to the correct driver */
> + VIR_DEBUG("Device %s is already bound to %s",
> + dev->name, stubDriverName);
> + dev->unbind_from_stub = true;
> + dev->remove_slot = true;
As I mentioned in the review of patch 1, the above two lines are
unnecessary.
> + result = 0;
> + goto cleanup;
> + }
> + /*
> + * If the device is bound to a driver that is not the stub, we'll
> + * need to reprobe later
> + */
> + dev->reprobe = true;
> + }
> +
> + /*
> + * Add stub to the device's driver_override, falling back to
> + * adding the device ID to the stub's dynamic ID table.
> + */
> + if (!(path = virPCIFile(dev->name, "driver_override")))
> + goto cleanup;
> +
> + if (virFileExists(path)) {
> + if (virPCIDeviceBindToStubWithOverride(dev, stubDriverName) < 0)
> + goto cleanup;
> + } else {
> + if (virPCIDeviceBindToStubWithNewid(dev, stubDriverName) < 0)
> + goto cleanup;
> + }
Yep, this is the way it should be handled for UnbindFromStub as well.
> +
> + result = 0;
> +
> + cleanup:
> VIR_FREE(stubDriverPath);
> VIR_FREE(driverLink);
> VIR_FREE(path);
> @@ -1326,10 +1435,6 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev)
> if (result < 0)
> virPCIDeviceUnbindFromStub(dev);
>
> - if (err)
> - virSetError(err);
> - virFreeError(err);
> -
> return result;
> }
>
More information about the libvir-list
mailing list