[libvirt] [PATCH 2/2] virpci: support driver_override sysfs interface
Jim Fehlig
jfehlig at suse.com
Tue Aug 2 03:45:42 UTC 2016
On 07/29/2016 11:43 AM, Laine Stump wrote:
> 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...
Yeah, I was trying to shoehorn this in the existing code, which we have found to
be unwise.
>
>
>> /* 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:
I was certain it was needed in my last round of testing, but while working on V2
I've found it is unnecessary.
>
>
> 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?).
Good suggestion. I've incorporated it into V2
https://www.redhat.com/archives/libvir-list/2016-August/msg00087.html
Regards,
Jim
More information about the libvir-list
mailing list