Re: [libvirt] [PATCH v1 03/31] virpcimock: Move actions checking one level up
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(a)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