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

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(a)linux.vnet.ibm.com&gt; > --- > 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.