Re: [libvirt] [PATCH V2] virpci: support driver_override sysfs interface
On 08/31/2016 01:42 PM, Jim Fehlig wrote: > On 08/30/2016 11:59 PM, Laine Stump wrote: >> 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=782a985d7af26db39e86070d28f987cad21313c0 > Yep, good point. Nice to have a description of the interface and examples of > its > use. I'll add it to the commit messages. > >> >> >> 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 :-/) > Thanks! I'll wait until 2.2.0 is out before pushing this. Pushed now that the release is out. Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V2] virpci: support driver_override sysfs interface
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=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
---
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/:03:00.0/driver_override
+ * echo :03:00.0 > /sys/bus/pci/devices/:03:00.0/driver/unbind
+ * echo :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
Re: [libvirt] [PATCH V2] virpci: support driver_override sysfs interface
Hi Laine,
Did you have a chance to look at V2 of this patch? As discussed in V1, I left
the existing code untouched and added new functions for the driver_override
interface. Thanks!
Regards,
Jim
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
>
> 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
> ---
>
> 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/:03:00.0/driver_override
> + * echo :03:00.0 > /sys/bus/pci/devices/:03:00.0/driver/unbind
> + * echo :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
[libvirt] [PATCH V2] virpci: support driver_override sysfs interface
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 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
---
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/:03:00.0/driver_override
+ * echo :03:00.0 > /sys/bus/pci/devices/:03:00.0/driver/unbind
+ * echo :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);
+
