Re: [PATCH 2/2] virpci: support driver_override sysfs interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxx>
>> ---
>>   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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]