[libvirt] [PATCH 1/2] virpci: simplify virPCIDeviceBindToStub

[Jim Fehlig]

On 07/28/2016 06:01 PM, Laine Stump wrote:
> On 07/11/2016 02:23 PM, Jim Fehlig wrote:
>> Early in virPCIDeviceBindToStub, there is a check to see if the
>> stub is already bound to the device, returning success with no
>> further actions if that is the case.
>>
>> The same condition is unnecessarily checked later in the function.
>
> Looking at the original code, the condition (whether the device is bound to
> the stub driver) is checked after writing the PCI ID of the device to the stub
> driver's "new_id" node. If you look here:
>
> https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-bus-pci
>
> It says that when you write a PCI ID to a driver's "new_id", if there are any
> devices with the given PCI ID currently not bound to any driver, they will be
> immediately bound to the given driver. So I don't think the check is
> unnecessary - if the device wasn't bound to any driver to begin with (e.g. if
> the host driver for the device was blacklisted), it will be bound to the stub
> driver *immediately* after writing the PCI ID to new_id (ie before getting to
> the 2nd check).
>
> So the condition isn't unnecessarily checked. It really can happen that we
> weren't bound to the stub in the first case, but were by the time we get to
> the 2nd.
>
> On the other hand, this points out how utterly atrocious the new_id interface
> is - when I tested this by blacklisting the igbvf driver (so all the VFs of my
> 82576 card would have no driver bound to them), then started a domain that
> used a single VF, *all* of the VFs were suddenly bound to vfio-pci !!!
>
> I also made a bunch of comments below before I noticed this part that I've put
> at the top, but in the end I think that especially since this whole
> bind/unbind/new_id interface is a dying thing, it would be better to leave it
> untouched (to avoid unexpected regressions) unless it's going to make a
> significant difference in how the driver_override stuff is added in.

Yes, I agree after reading your above observations. I've dropped this patch in
V2 and with the exception of renaming functions, left the exiting code
untouched. I think that approach will also make it easier to drop the new_id
code once support for older kernels is no longer desired.

Regards,
Jim