[libvirt] [PATCH v4 05/10] Split reprobe action from the virPCIUnbindFromStub into a new function

[Alex Williamson]

On Fri, 2015-11-20 at 11:33 -0500, Laine Stump wrote:
> On 11/14/2015 03:36 AM, Shivaprasad G Bhat wrote:
> > The reprobe needs to be called multiple times for vfio devices for each
> > device in the iommu group in future patch. So split the reprobe into a
> > new function. No functional change.
> >
> > Signed-off-by: Shivaprasad G Bhat <sbhat at linux.vnet.ibm.com>
> > ---
> >   src/util/virpci.c |   47 +++++++++++++++++++++++++++++++----------------
> >   1 file changed, 31 insertions(+), 16 deletions(-)
> >
> > diff --git a/src/util/virpci.c b/src/util/virpci.c
> > index 89e69e2..febf100 100644
> > --- a/src/util/virpci.c
> > +++ b/src/util/virpci.c
> > @@ -1099,6 +1099,36 @@ virPCIIsKnownStub(char *driver)
> >   }
> >   
> >   static int
> > +virPCIDeviceReprobeHostDriver(virPCIDevicePtr dev,
> > +                              const char *driver,
> > +                              const char *drvdir)
> > +{
> > +    char *path = NULL;
> > +    int ret = -1;
> > +
> > +    /* Trigger a re-probe of the device is not in the stub's dynamic
> 
> As long as you're moving the code, s/of/if/
> > +     * ID table. If the stub is available, but 'remove_id' isn't
> > +     * available, then re-probing would just cause the device to be
> > +     * re-bound to the stub.
> > +     */
> > +    if (driver && !(path = virPCIDriverFile(driver, "remove_id")))
> > +        goto cleanup;
> > +
> > +    if (!driver || !virFileExists(drvdir) || virFileExists(path)) {
> > +        if (virFileWriteStr(PCI_SYSFS "drivers_probe", dev->name, 0) < 0) {
> > +            virReportSystemError(errno,
> > +                                 _("Failed to trigger a re-probe for PCI device '%s'"),
> > +                                 dev->name);
> > +            goto cleanup;
> > +        }
> > +    }
> > +    ret = 0;
> > + cleanup:
> > +    VIR_FREE(path);
> > +    return ret;
> > +}
> > +
> > +static int
> >   virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
> >   {
> >       int result = -1;
> > @@ -1150,24 +1180,9 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
> >           goto cleanup;
> >       }
> >   
> > -    /* 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
> > -     * re-bound to the stub.
> > -     */
> > -    VIR_FREE(path);
> > -    if (driver && !(path = virPCIDriverFile(driver, "remove_id")))
> > +    if (virPCIDeviceReprobeHostDriver(dev, driver, drvdir) < 0)
> >           goto cleanup;
> >   
> > -    if (!driver || !virFileExists(drvdir) || virFileExists(path)) {
> > -        if (virFileWriteStr(PCI_SYSFS "drivers_probe", dev->name, 0) < 0) {
> > -            virReportSystemError(errno,
> > -                                 _("Failed to trigger a re-probe for PCI device '%s'"),
> > -                                 dev->name);
> > -            goto cleanup;
> > -        }
> > -    }
> > -
> >       result = 0;
> >   
> >    cleanup:
> >
> 
> Seems safe, but is this really what we want to do? I haven't 
> read/understood the remaining patches yet, but this makes it sound like 
> what is going to happen is that all of the devices will be unbound from 
> vfio-pci immediately, so they are "in limbo", and will then be reprobed 
> once all devices are unused (and therefore unbound from vfio-pci).
> 
> I think that may be a bit dangerous. Instead, we should leave the 
> devices bound to vfio-pci until all of them are unused, and at that 
> time, we should unbind them all from vfio-pci, then reprobe them all. 
> (again, I may have misunderstood the direction, if so ignore this).
> 
> If I am misunderstanding, and unbinding from vfio-pci will also be 
> delayed until all devices are unused, then ACK.

Why don't we start making use of the driver_override feature that we've
had in the kernel instead of continuing to hack on the racy
add_id/remove_id stuff?  We've already solved the problem in the kernel.

Want to bind a device to vfio-pci:

echo vfio-pci > /sys/bus/pci/devices/<dev>/driver_override
if [ -e /sys/bus/pci/devices/<dev>/driver ]; then
  echo <dev> > /sys/bus/pci/devices/<dev>/driver/unbind
fi
echo <dev> > /sys/bus/pci/drivers_probe

To rebind, replace vfio-pci in the first echo with null and repeat the
rest.  The affects are limited to a single device, we're not going to
have surprise unbound devices show up bound to the driver, and we don't
race anyone manipulating other devices.  Thanks,

Alex