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

[Laine Stump <laine@xxxxxxxxx>]

On 11/20/2015 12:51 PM, Alex Williamson wrote:
> 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@xxxxxxxxxxxxxxxxxx>
> > > ---
> > >    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.

How far back was this available? I'm guessing it is safe in any kernel 
that also supports vfio? What about earlier than that?

(okay, I think I just answered my own question by booting a RHEL6 guest 
(kernel 2.6.32) and seeing that it doesn't have driver_override. So we 
have to keep the existing code that uses add_id/remove_id, but can add 
use of driver_override in the cases that it is available. This 
can/should be done orthogonally to this current patch series).


> 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

If multiple devices will be rebound to their host drivers, is there an 
advantage to doing all of the driver_override writing for all devices 
first, then doing the drivers_probe once at the end? Since libvirt has 
historically dealt with a single device at a time, the two were always 
done in immediate succession, but now we're talking about binding 
multiple devices at a time.

> 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

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