[libvirt] [PATCH v1 03/31] virpcimock: Move actions checking one level up

[Michal Privoznik]

On 7/16/19 2:03 PM, Peter Krempa wrote:
> On Thu, Jul 11, 2019 at 17:53:50 +0200, Michal Privoznik wrote:
>> The pci_driver_bind() and pci_driver_unbind() functions are
>> "internal implementation", meaning other parts of the code should
>> be able to call them and get the job done. Checking for actions
>> (PCI_ACTION_BIND and PCI_ACTION_UNBIND) should be done in
>> handlers (pci_driver_handle_bind() and
>> pci_driver_handle_unbind()). Surprisingly, the other two actions
>> (PCI_ACTION_NEW_ID and PCI_ACTION_REMOVE_ID) are checked already
>> at this level.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>   tests/virpcimock.c | 16 ++++++++--------
>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/tests/virpcimock.c b/tests/virpcimock.c
>> index beb5e1490d..6865f992dc 100644
>> --- a/tests/virpcimock.c
>> +++ b/tests/virpcimock.c
>> @@ -551,8 +551,8 @@ pci_driver_bind(struct pciDriver *driver,
>>       int ret = -1;
>>       char *devpath = NULL, *driverpath = NULL;
>>   
>> -    if (dev->driver || PCI_ACTION_BIND & driver->fail) {
>> -        /* Device already bound or failing driver requested */
>> +    if (dev->driver) {
>> +        /* Device already bound */
>>           errno = ENODEV;
>>           return ret;
>>       }
> 
> So this function ...
> 
>> @@ -669,8 +669,8 @@ pci_driver_handle_bind(const char *path)
>>       struct pciDevice *dev = pci_device_find_by_content(path);
>>       struct pciDriver *driver = pci_driver_find_by_path(path);
>>   
>> -    if (!driver || !dev) {
>> -        /* This should never happen (TM) */
>> +    if (!driver || !dev || PCI_ACTION_BIND & driver->fail) {
>> +        /* No driver, no device or failing driver requested */
>>           errno = ENODEV;
>>           goto cleanup;
>>       }
> 
> ... is called here, which you fix, but also in
> 
> pci_device_autobind and pci_driver_handle_new_id which are not fixed by
> this commit.
> 
> I don't quite understand deeply what this is supposed to do, so I don't
> know what's supposed to happen in that case, but this seems suspicious
> to me. Please try explaining/justifying why the two other call paths are
> not changed.
> 
> Also I did not bother checking the unbind code for the same problem.
> 

This whole PCI_ACTION_BIND mess exists because with RHEL-7 kernel it's not possible to bind a PCI device directly to vfio-pci driver. I mean, with RHEL-7 kernel the following steps fail:

01:00.0 Non-Volatile memory controller: Device 1cc1:8201 (rev 03)
        Subsystem: Device 1cc1:8201
        Kernel driver in use: nvme

# echo "1cc1 8201" > /sys/bus/pci/drivers/vfio-pci/new_id
# echo "0000:01:00.0" > /sys/bus/pci/devices/0000\:01\:00.0/driver/unbind
# echo "0000:01:00.0" > /sys/bus/pci/drivers/vfio-pci/bind

01:00.0 Non-Volatile memory controller: Device 1cc1:8201 (rev 03)
        Subsystem: Device 1cc1:8201
        Kernel driver in use: vfio-pci

But on anything else (e.g. my vanilla kernel) this is allowed. Anyway, this is irrelevant because even on RHEL-7 we are using 'driver_override' (which is way simpler to use and doesn't create a window where an unbinded PCI device can be claimed by a different PCI driver).

How does this concern pcimock? Well, there are only so many "entry" points to the mock. These are functions which have "handle" in their name and are called from within pci_driver_handle_change(). Every other function is just a helper. For instance, pci_device_autobind() is called from pci_device_new_from_stub() which just creates a PCI device during the mock initialization. Or, it's called on a write to "drivers_probe" which again succeeds (on both RHEL-7 and vanilla kernels). And for pci_driver_handle_new_id it's the same story.

If you want, I can add those checks there so that this patch looks complete. But honestly, it doesn't matter because this code path will not be used once 'driver_override' is implemented (patch 05/31).

Michal