[libvirt] [PATCH V2] virpci: support driver_override sysfs interface

[Laine Stump]

On 08/01/2016 11:36 PM, Jim Fehlig wrote:
> 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

That message details the change to the kernel that caused the regression 
for Xen, but not the driver_override interface. Maybe you could add a 
link to the kernel commit that adds driver_override:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pci/pci-driver.c?h=v3.12&id=782a985d7af26db39e86070d28f987cad21313c0


Everything else looks good, and passes my tests for vfio device 
assignment (including when the host driver has been blacklisted).

ACK. (Sorry I forgot about this earlier in the month :-/)

>
> This patch adds support for the driver_override interface by
>
> - adding new virPCIDevice{BindTo,UnbindFrom}StubWithOverride functions
>    that use the driver_override interface
> - renames the existing virPCIDevice{BindTo,UnbindFrom}Stub functions
>    to virPCIDevice{BindTo,UnbindFrom}StubWithNewid to perserve existing
>    behavior on new_id interface
> - changes virPCIDevice{BindTo,UnbindFrom}Stub function to call one of
>    the above depending on availability of driver_override
>
> The patch includes a bit of duplicate code, but allows for easily
> dropping the new_id code once support for older kernels is no
> longer desired.
>
> Signed-off-by: Jim Fehlig <jfehlig at suse.com>
> ---
>
> V1:
>
> https://www.redhat.com/archives/libvir-list/2016-July/msg00370.html
>
> Changes since V1:
> - drop patch1
> - change patch2 to preserve the existing new_id code and add new
>    functions to implement the driver_override interface
>
>   src/util/virpci.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 149 insertions(+), 2 deletions(-)
>
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 132948d..6c8174a 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -1089,8 +1089,54 @@ virPCIDeviceUnbind(virPCIDevicePtr dev)
>       return ret;
>   }
>   
> +/*
> + * Bind a PCI device to a driver using driver_override sysfs interface.
> + * E.g.
> + *
> + *  echo driver-name > /sys/bus/pci/devices/0000:03:00.0/driver_override
> + *  echo 0000:03:00.0 > /sys/bus/pci/devices/0000:03:00.0/driver/unbind
> + *  echo 0000:03:00.0 > /sys/bus/pci/drivers_probe
> + *
> + * An empty driverName will cause the device to be bound to its
> + * preferred driver.
> + */
>   static int
> -virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
> +virPCIDeviceBindWithDriverOverride(virPCIDevicePtr dev,
> +                                   const char *driverName)
> +{
> +    int ret = -1;
> +    char *path;
> +
> +    if (!(path = virPCIFile(dev->name, "driver_override")))
> +        return -1;
> +
> +    if (virFileWriteStr(path, driverName, 0) < 0) {
> +        virReportSystemError(errno,
> +                             _("Failed to add driver '%s' to driver_override "
> +                               " interface of PCI device '%s'"),
> +                             driverName, dev->name);
> +        goto cleanup;
> +    }
> +
> +    if (virPCIDeviceUnbind(dev) < 0)
> +        goto cleanup;
> +
> +    if (virFileWriteStr(PCI_SYSFS "drivers_probe", dev->name, 0) < 0) {
> +        virReportSystemError(errno,
> +                             _("Failed to trigger a probe for PCI device '%s'"),
> +                             dev->name);
> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    VIR_FREE(path);
> +    return ret;
> +}
> +
> +static int
> +virPCIDeviceUnbindFromStubWithNewid(virPCIDevicePtr dev)
>   {
>       int result = -1;
>       char *drvdir = NULL;
> @@ -1191,9 +1237,41 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
>       return result;
>   }
>   
> +static int
> +virPCIDeviceUnbindFromStubWithOverride(virPCIDevicePtr dev)
> +{
> +    if (!dev->unbind_from_stub) {
> +        VIR_DEBUG("Unbind from stub skipped for PCI device %s", dev->name);
> +        return 0;
> +    }
> +
> +    return virPCIDeviceBindWithDriverOverride(dev, "\n");
> +}
> +
> +static int
> +virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
> +{
> +    int ret;
> +    char *path;
> +
> +    /*
> +     * Prefer using the device's driver_override interface, falling back
> +     * to the unpleasant new_id interface.
> +     */
> +    if (!(path = virPCIFile(dev->name, "driver_override")))
> +        return -1;
> +
> +    if (virFileExists(path))
> +        ret = virPCIDeviceUnbindFromStubWithOverride(dev);
> +    else
> +        ret = virPCIDeviceUnbindFromStubWithNewid(dev);
> +
> +    VIR_FREE(path);
> +    return ret;
> +}
>   
>   static int
> -virPCIDeviceBindToStub(virPCIDevicePtr dev)
> +virPCIDeviceBindToStubWithNewid(virPCIDevicePtr dev)
>   {
>       int result = -1;
>       bool reprobe = false;
> @@ -1345,6 +1423,75 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev)
>       return result;
>   }
>   
> +static int
> +virPCIDeviceBindToStubWithOverride(virPCIDevicePtr dev)
> +{
> +    int ret = -1;
> +    const char *stubDriverName;
> +    char *stubDriverPath = NULL;
> +    char *driverLink = 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);
> +            ret = 0;
> +            goto cleanup;
> +        }
> +    }
> +
> +    if (virPCIDeviceBindWithDriverOverride(dev, stubDriverName) < 0)
> +        goto cleanup;
> +
> +    dev->unbind_from_stub = true;
> +    ret = 0;
> +
> + cleanup:
> +    VIR_FREE(stubDriverPath);
> +    VIR_FREE(driverLink);
> +    return ret;
> +}
> +
> +static int
> +virPCIDeviceBindToStub(virPCIDevicePtr dev)
> +{
> +    int ret;
> +    char *path;
> +
> +    /*
> +     * Prefer using the device's driver_override interface, falling back
> +     * to the unpleasant new_id interface.
> +     */
> +    if (!(path = virPCIFile(dev->name, "driver_override")))
> +        return -1;
> +
> +    if (virFileExists(path))
> +        ret = virPCIDeviceBindToStubWithOverride(dev);
> +    else
> +        ret = virPCIDeviceBindToStubWithNewid(dev);
> +
> +    VIR_FREE(path);
> +    return ret;
> +}
> +
>   /* virPCIDeviceDetach:
>    *
>    * Detach this device from the host driver, attach it to the stub