[libvirt] [PATCH 0/3] reattach pci devices when qemuPrepareHostdevPCIDevices() failed
When I try to attach a pci device to guest OS, libvirtd resets the pci failed. But I can not use this pci device after attaching pci device failed. I must rollback the operation by hand. I think libvirtd should auto rollback the operation when it failed. Wen Congyang (3): remove devices from driver->activePciHostdevs when qemuPrepareHostdevPCIDevices() failed reattach pci device when pciBindDeviceToStub() failed reattach pci devices when qemuPrepareHostdevPCIDevices() failed src/qemu/qemu_hostdev.c | 33 ++++++++++++++--- src/util/pci.c | 90 +++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 107 insertions(+), 16 deletions(-)
We should not mark pci devices as active when qemuPrepareHostdevPCIDevices() failed. --- src/qemu/qemu_hostdev.c | 21 ++++++++++++++++++--- 1 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index f4b2108..30db0e2 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -112,7 +112,7 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, if (!(pcidevs = qemuGetPciHostDeviceList(hostdevs, nhostdevs))) return -1; - /* We have to use 3 loops here. *All* devices must + /* We have to use 4 loops here. *All* devices must * be detached before we reset any of them, because * in some cases you have to reset the whole PCI, * which impacts all devices on it. Also, all devices @@ -145,14 +145,29 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, /* Now mark all the devices as active */ for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); - pciDeviceListSteal(pcidevs, dev); if (pciDeviceListAdd(driver->activePciHostdevs, dev) < 0) { pciFreeDevice(dev); - goto cleanup; + goto inactivedevs; } } + /* Now steal all the devices from pcidevs */ + for (i = 0; i < pciDeviceListCount(pcidevs); i++) { + pciDevice *dev = pciDeviceListGet(pcidevs, i); + pciDeviceListSteal(pcidevs, dev); + } + ret = 0; + goto cleanup; + +inactivedevs: + /* Only steal all the devices from driver->activePciHostdevs. We will + * free them in pciDeviceListFree(). + */ + for (i = 0; i < pciDeviceListCount(pcidevs); i++) { + pciDevice *dev = pciDeviceListGet(pcidevs, i); + pciDeviceListSteal(driver->activePciHostdevs, dev); + } cleanup: pciDeviceListFree(pcidevs); -- 1.7.1
On 03/28/2011 01:01 AM, Wen Congyang wrote:
We should not mark pci devices as active when qemuPrepareHostdevPCIDevices() failed.
--- src/qemu/qemu_hostdev.c | 21 ++++++++++++++++++--- 1 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index f4b2108..30db0e2 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -112,7 +112,7 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, if (!(pcidevs = qemuGetPciHostDeviceList(hostdevs, nhostdevs))) return -1;
- /* We have to use 3 loops here. *All* devices must + /* We have to use 4 loops here. *All* devices must
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
We should bind pci device to original driver when pciBindDeviceToStub() failed. If the pci device is not bound to any driver before calling pciBindDeviceToStub(), we should only unbind it from pci-stub. If it is bound to pci-stub, we should not unbid it from pci-stub. --- src/util/pci.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 80 insertions(+), 10 deletions(-) diff --git a/src/util/pci.c b/src/util/pci.c index 8d2dbb0..e30f5cf 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -65,6 +65,11 @@ struct _pciDevice { unsigned has_flr : 1; unsigned has_pm_reset : 1; unsigned managed : 1; + + /* used by reattach function */ + unsigned unbind_from_stub : 1; + unsigned remove_slot : 1; + unsigned reprobe : 1; }; struct _pciDeviceList { @@ -834,11 +839,25 @@ recheck: } +static int pciUnBindDeviceFromStub(pciDevice *dev, const char *driver); static int pciBindDeviceToStub(pciDevice *dev, const char *driver) { char drvdir[PATH_MAX]; char path[PATH_MAX]; + int reprobe = 0; + int ret = 0; + + /* check whether the device is already bound to a driver */ + pciDriverDir(drvdir, sizeof(drvdir), driver); + pciDeviceFile(path, sizeof(path), dev->name, "driver"); + if (virFileExists(path)) { + if (virFileLinkPointsTo(path, drvdir)) { + /* The device is already bound to pci-stub */ + return 0; + } + reprobe = 1; + } /* Add the PCI device ID to the stub's dynamic ID table; * this is needed to allow us to bind the device to the stub. @@ -856,16 +875,31 @@ pciBindDeviceToStub(pciDevice *dev, const char *driver) return -1; } + /* check whether the device is bound to pci-stub when we write dev->id to + * new_id. + */ + pciDriverDir(drvdir, sizeof(drvdir), driver); + pciDeviceFile(path, sizeof(path), dev->name, "driver"); + if (virFileLinkPointsTo(path, drvdir)) { + dev->unbind_from_stub = 1; + dev->remove_slot = 1; + goto remove_id; + } + /* If the device is already bound to a driver, unbind it. * Note, this will have rather unpleasant side effects if this * PCI device happens to be IDE controller for the disk hosting * your root filesystem. */ pciDeviceFile(path, sizeof(path), dev->name, "driver/unbind"); - if (virFileExists(path) && virFileWriteStr(path, dev->name, 0) < 0) { - virReportSystemError(errno, - _("Failed to unbind PCI device '%s'"), dev->name); - return -1; + if (virFileExists(path)) { + if (virFileWriteStr(path, dev->name, 0) < 0) { + virReportSystemError(errno, + _("Failed to unbind PCI device '%s'"), + dev->name); + return -1; + } + dev->reprobe = reprobe; } /* If the device isn't already bound to pci-stub, try binding it now. @@ -879,18 +913,23 @@ pciBindDeviceToStub(pciDevice *dev, const char *driver) virReportSystemError(errno, _("Failed to add slot for PCI device '%s' to %s"), dev->name, driver); - return -1; + ret = -1; + goto remove_id; } + dev->remove_slot = 1; pciDriverFile(path, sizeof(path), driver, "bind"); if (virFileWriteStr(path, dev->name, 0) < 0) { virReportSystemError(errno, _("Failed to bind PCI device '%s' to %s"), dev->name, driver); - return -1; + ret = -1; + goto remove_id; } + dev->unbind_from_stub = 1; } +remove_id: /* If 'remove_id' exists, remove the device id from pci-stub's dynamic * ID table so that 'drivers_probe' works below. */ @@ -899,10 +938,21 @@ pciBindDeviceToStub(pciDevice *dev, const char *driver) virReportSystemError(errno, _("Failed to remove PCI ID '%s' from %s"), dev->id, driver); - return -1; + ret = -1; + + /* remove PCI ID from pci-stub failed, and we can not reprobe it */ + if (dev->reprobe) { + VIR_WARN("Faile to remove PCI ID '%s' from %s, and the device can" + "not be reprobed again.", dev->id, driver); + } + dev->reprobe = 0; } - return 0; + if (ret < 0) { + pciUnBindDeviceFromStub(dev, driver); + } + + return ret; } int @@ -930,6 +980,9 @@ pciUnBindDeviceFromStub(pciDevice *dev, const char *driver) char drvdir[PATH_MAX]; char path[PATH_MAX]; + if (!dev->unbind_from_stub) + goto remove_slot; + /* If the device is bound to stub, unbind it. */ pciDriverDir(drvdir, sizeof(drvdir), driver); @@ -938,21 +991,36 @@ pciUnBindDeviceFromStub(pciDevice *dev, const char *driver) pciDriverFile(path, sizeof(path), driver, "unbind"); if (virFileWriteStr(path, dev->name, 0) < 0) { virReportSystemError(errno, - _("Failed to bind PCI device '%s' to %s"), + _("Failed to unbind PCI device '%s' from %s"), dev->name, driver); + /* do not do it again */ + dev->unbind_from_stub = 0; + dev->remove_slot = 0; + dev->reprobe = 0; return -1; } } + dev->unbind_from_stub = 0; + +remove_slot: + if (!dev->remove_slot) + goto reprobe; /* Xen's pciback.ko wants you to use remove_slot on the specific device */ pciDriverFile(path, sizeof(path), driver, "remove_slot"); if (virFileExists(path) && virFileWriteStr(path, dev->name, 0) < 0) { virReportSystemError(errno, - _("Failed to remove slot for PCI device '%s' to %s"), + _("Failed to remove slot for PCI device '%s' from %s"), dev->name, driver); + dev->remove_slot = 0; + dev->reprobe = 0; return -1; } + dev->remove_slot = 0; +reprobe: + if (!dev->reprobe) + return 0; /* 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 @@ -965,9 +1033,11 @@ pciUnBindDeviceFromStub(pciDevice *dev, const char *driver) virReportSystemError(errno, _("Failed to trigger a re-probe for PCI device '%s'"), dev->name); + dev->reprobe = 0; return -1; } } + dev->reprobe = 0; return 0; } -- 1.7.1
On 03/28/2011 01:01 AM, Wen Congyang wrote:
We should bind pci device to original driver when pciBindDeviceToStub() failed. If the pci device is not bound to any driver before calling pciBindDeviceToStub(), we should only unbind it from pci-stub. If it is bound to pci-stub, we should not unbid it from pci-stub.
--- src/util/pci.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 80 insertions(+), 10 deletions(-)
diff --git a/src/util/pci.c b/src/util/pci.c index 8d2dbb0..e30f5cf 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -65,6 +65,11 @@ struct _pciDevice { unsigned has_flr : 1; unsigned has_pm_reset : 1; unsigned managed : 1; + + /* used by reattach function */ + unsigned unbind_from_stub : 1; + unsigned remove_slot : 1; + unsigned reprobe : 1; };
struct _pciDeviceList { @@ -834,11 +839,25 @@ recheck: }
+static int pciUnBindDeviceFromStub(pciDevice *dev, const char *driver);
I would have used Unbind rather than UnBind, but that's cosmetic. Is it possible to float that function up, instead of having to have a forward declaration? I tend to topologically sort my static functions when possible (again, cosmetic).
static int pciBindDeviceToStub(pciDevice *dev, const char *driver) { char drvdir[PATH_MAX]; char path[PATH_MAX]; + int reprobe = 0; + int ret = 0; + + /* check whether the device is already bound to a driver */ + pciDriverDir(drvdir, sizeof(drvdir), driver); + pciDeviceFile(path, sizeof(path), dev->name, "driver");
Ouch - conflict with Matthias's patches to avoid PATH_MAX. If that goes in first, your rebase might not be trivial, so it would be worth a v2. But overall, I think the idea of this patch is sane, and nothing obvious jumped out at me as needing fixing other than rebase issues. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
At 04/05/2011 06:53 AM, Eric Blake Write:
On 03/28/2011 01:01 AM, Wen Congyang wrote:
We should bind pci device to original driver when pciBindDeviceToStub() failed. If the pci device is not bound to any driver before calling pciBindDeviceToStub(), we should only unbind it from pci-stub. If it is bound to pci-stub, we should not unbid it from pci-stub.
--- src/util/pci.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 80 insertions(+), 10 deletions(-)
diff --git a/src/util/pci.c b/src/util/pci.c index 8d2dbb0..e30f5cf 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -65,6 +65,11 @@ struct _pciDevice { unsigned has_flr : 1; unsigned has_pm_reset : 1; unsigned managed : 1; + + /* used by reattach function */ + unsigned unbind_from_stub : 1; + unsigned remove_slot : 1; + unsigned reprobe : 1; };
struct _pciDeviceList { @@ -834,11 +839,25 @@ recheck: }
+static int pciUnBindDeviceFromStub(pciDevice *dev, const char *driver);
I would have used Unbind rather than UnBind, but that's cosmetic. Is it possible to float that function up, instead of having to have a forward declaration? I tend to topologically sort my static functions when possible (again, cosmetic).
Yes, 'Unbind' is better than UnBind. I will send a patch to rename it and float this function up.
static int pciBindDeviceToStub(pciDevice *dev, const char *driver) { char drvdir[PATH_MAX]; char path[PATH_MAX]; + int reprobe = 0; + int ret = 0; + + /* check whether the device is already bound to a driver */ + pciDriverDir(drvdir, sizeof(drvdir), driver); + pciDeviceFile(path, sizeof(path), dev->name, "driver");
Ouch - conflict with Matthias's patches to avoid PATH_MAX. If that goes in first, your rebase might not be trivial, so it would be worth a v2.
Yes, this patch conflicts with Matthias's patches. And his patches have been pushed. I will send v2.
But overall, I think the idea of this patch is sane, and nothing obvious jumped out at me as needing fixing other than rebase issues.
This patch do the following things: 1. rename the function as 'Unbind' is better than 'UnBind'. 2. pciUnbindDeviceFromStub() will be used in the function pciBindDeviceToStub() in next patch. Float it up, instead of having to have a forward declaration --- src/util/pci.c | 134 ++++++++++++++++++++++++++++---------------------------- 1 files changed, 67 insertions(+), 67 deletions(-) diff --git a/src/util/pci.c b/src/util/pci.c index a7b8caa..e69813c 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -867,6 +867,72 @@ recheck: return NULL; } +static int +pciUnbindDeviceFromStub(pciDevice *dev, const char *driver) +{ + int result = -1; + char *drvdir = NULL; + char *path = NULL; + + /* If the device is bound to stub, unbind it. + */ + if (pciDriverDir(&drvdir, driver) < 0 || + pciDeviceFile(&path, dev->name, "driver") < 0) { + goto cleanup; + } + + if (virFileExists(drvdir) && virFileLinkPointsTo(path, drvdir)) { + if (pciDriverFile(&path, driver, "unbind") < 0) { + goto cleanup; + } + + if (virFileWriteStr(path, dev->name, 0) < 0) { + virReportSystemError(errno, + _("Failed to bind PCI device '%s' to %s"), + dev->name, driver); + goto cleanup; + } + } + + /* Xen's pciback.ko wants you to use remove_slot on the specific device */ + if (pciDriverFile(&path, driver, "remove_slot") < 0) { + goto cleanup; + } + + if (virFileExists(path) && virFileWriteStr(path, dev->name, 0) < 0) { + virReportSystemError(errno, + _("Failed to remove slot for PCI device '%s' to %s"), + dev->name, driver); + 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. + */ + if (pciDriverFile(&path, driver, "remove_id") < 0) { + goto cleanup; + } + + if (!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: + VIR_FREE(drvdir); + VIR_FREE(path); + + return result; +} + static int pciBindDeviceToStub(pciDevice *dev, const char *driver) @@ -983,72 +1049,6 @@ pciDettachDevice(pciDevice *dev, pciDeviceList *activeDevs) return pciBindDeviceToStub(dev, driver); } -static int -pciUnBindDeviceFromStub(pciDevice *dev, const char *driver) -{ - int result = -1; - char *drvdir = NULL; - char *path = NULL; - - /* If the device is bound to stub, unbind it. - */ - if (pciDriverDir(&drvdir, driver) < 0 || - pciDeviceFile(&path, dev->name, "driver") < 0) { - goto cleanup; - } - - if (virFileExists(drvdir) && virFileLinkPointsTo(path, drvdir)) { - if (pciDriverFile(&path, driver, "unbind") < 0) { - goto cleanup; - } - - if (virFileWriteStr(path, dev->name, 0) < 0) { - virReportSystemError(errno, - _("Failed to bind PCI device '%s' to %s"), - dev->name, driver); - goto cleanup; - } - } - - /* Xen's pciback.ko wants you to use remove_slot on the specific device */ - if (pciDriverFile(&path, driver, "remove_slot") < 0) { - goto cleanup; - } - - if (virFileExists(path) && virFileWriteStr(path, dev->name, 0) < 0) { - virReportSystemError(errno, - _("Failed to remove slot for PCI device '%s' to %s"), - dev->name, driver); - 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. - */ - if (pciDriverFile(&path, driver, "remove_id") < 0) { - goto cleanup; - } - - if (!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: - VIR_FREE(drvdir); - VIR_FREE(path); - - return result; -} - int pciReAttachDevice(pciDevice *dev, pciDeviceList *activeDevs) { @@ -1065,7 +1065,7 @@ pciReAttachDevice(pciDevice *dev, pciDeviceList *activeDevs) return -1; } - return pciUnBindDeviceFromStub(dev, driver); + return pciUnbindDeviceFromStub(dev, driver); } /* Certain hypervisors (like qemu/kvm) map the PCI bar(s) on -- 1.7.1
On 04/06/2011 01:13 AM, Wen Congyang wrote:
This patch do the following things: 1. rename the function as 'Unbind' is better than 'UnBind'. 2. pciUnbindDeviceFromStub() will be used in the function pciBindDeviceToStub() in next patch. Float it up, instead of having to have a forward declaration
--- src/util/pci.c | 134 ++++++++++++++++++++++++++++---------------------------- 1 files changed, 67 insertions(+), 67 deletions(-)
ACK, and thanks for splitting the code motion into its own patch (much easier to review that way). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
We should bind pci device to original driver when pciBindDeviceToStub() failed. If the pci device is not bound to any driver before calling pciBindDeviceToStub(), we should only unbind it from pci-stub. If it is bound to pci-stub, we should not unbid it from pci-stub. --- src/util/pci.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 91 insertions(+), 12 deletions(-) diff --git a/src/util/pci.c b/src/util/pci.c index e69813c..ec9a0b9 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -65,6 +65,11 @@ struct _pciDevice { unsigned has_flr : 1; unsigned has_pm_reset : 1; unsigned managed : 1; + + /* used by reattach function */ + unsigned unbind_from_stub : 1; + unsigned remove_slot : 1; + unsigned reprobe : 1; }; struct _pciDeviceList { @@ -874,6 +879,9 @@ pciUnbindDeviceFromStub(pciDevice *dev, const char *driver) char *drvdir = NULL; char *path = NULL; + if (!dev->unbind_from_stub) + goto remove_slot; + /* If the device is bound to stub, unbind it. */ if (pciDriverDir(&drvdir, driver) < 0 || @@ -888,11 +896,16 @@ pciUnbindDeviceFromStub(pciDevice *dev, const char *driver) if (virFileWriteStr(path, dev->name, 0) < 0) { virReportSystemError(errno, - _("Failed to bind PCI device '%s' to %s"), + _("Failed to unbind PCI device '%s' from %s"), dev->name, driver); goto cleanup; } } + dev->unbind_from_stub = 0; + +remove_slot: + if (!dev->remove_slot) + goto reprobe; /* Xen's pciback.ko wants you to use remove_slot on the specific device */ if (pciDriverFile(&path, driver, "remove_slot") < 0) { @@ -901,10 +914,17 @@ pciUnbindDeviceFromStub(pciDevice *dev, const char *driver) if (virFileExists(path) && virFileWriteStr(path, dev->name, 0) < 0) { virReportSystemError(errno, - _("Failed to remove slot for PCI device '%s' to %s"), + _("Failed to remove slot for PCI device '%s' from %s"), dev->name, driver); goto cleanup; } + dev->remove_slot = 0; + +reprobe: + if (!dev->reprobe) { + result = 0; + 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 @@ -927,6 +947,11 @@ pciUnbindDeviceFromStub(pciDevice *dev, const char *driver) result = 0; cleanup: + /* do not do it again */ + dev->unbind_from_stub = 0; + dev->remove_slot = 0; + dev->reprobe = 0; + VIR_FREE(drvdir); VIR_FREE(path); @@ -940,6 +965,22 @@ pciBindDeviceToStub(pciDevice *dev, const char *driver) int result = -1; char *drvdir = NULL; char *path = NULL; + int reprobe = 0; + + /* check whether the device is already bound to a driver */ + if (pciDriverDir(&drvdir, driver) < 0 || + pciDeviceFile(&path, dev->name, "driver") < 0) { + goto cleanup; + } + + if (virFileExists(path)) { + if (virFileLinkPointsTo(path, drvdir)) { + /* The device is already bound to pci-stub */ + result = 0; + goto cleanup; + } + reprobe = 1; + } /* Add the PCI device ID to the stub's dynamic ID table; * this is needed to allow us to bind the device to the stub. @@ -950,7 +991,7 @@ pciBindDeviceToStub(pciDevice *dev, const char *driver) * bound by the stub. */ if (pciDriverFile(&path, driver, "new_id") < 0) { - return -1; + goto cleanup; } if (virFileWriteStr(path, dev->id, 0) < 0) { @@ -960,6 +1001,20 @@ pciBindDeviceToStub(pciDevice *dev, const char *driver) goto cleanup; } + /* check whether the device is bound to pci-stub when we write dev->id to + * new_id. + */ + if (pciDriverDir(&drvdir, driver) < 0 || + pciDeviceFile(&path, dev->name, "driver") < 0) { + goto remove_id; + } + + if (virFileLinkPointsTo(path, drvdir)) { + dev->unbind_from_stub = 1; + dev->remove_slot = 1; + goto remove_id; + } + /* If the device is already bound to a driver, unbind it. * Note, this will have rather unpleasant side effects if this * PCI device happens to be IDE controller for the disk hosting @@ -969,48 +1024,61 @@ pciBindDeviceToStub(pciDevice *dev, const char *driver) goto cleanup; } - if (virFileExists(path) && virFileWriteStr(path, dev->name, 0) < 0) { - virReportSystemError(errno, - _("Failed to unbind PCI device '%s'"), dev->name); - goto cleanup; + if (virFileExists(path)) { + if (virFileWriteStr(path, dev->name, 0) < 0) { + virReportSystemError(errno, + _("Failed to unbind PCI device '%s'"), + dev->name); + goto cleanup; + } + dev->reprobe = reprobe; } /* If the device isn't already bound to pci-stub, try binding it now. */ if (pciDriverDir(&drvdir, driver) < 0 || pciDeviceFile(&path, dev->name, "driver") < 0) { - goto cleanup; + goto remove_id; } if (!virFileLinkPointsTo(path, drvdir)) { /* Xen's pciback.ko wants you to use new_slot first */ if (pciDriverFile(&path, driver, "new_slot") < 0) { - goto cleanup; + goto remove_id; } if (virFileExists(path) && virFileWriteStr(path, dev->name, 0) < 0) { virReportSystemError(errno, _("Failed to add slot for PCI device '%s' to %s"), dev->name, driver); - goto cleanup; + goto remove_id; } + dev->remove_slot = 1; if (pciDriverFile(&path, driver, "bind") < 0) { - goto cleanup; + goto remove_id; } if (virFileWriteStr(path, dev->name, 0) < 0) { virReportSystemError(errno, _("Failed to bind PCI device '%s' to %s"), dev->name, driver); - goto cleanup; + goto remove_id; } + dev->unbind_from_stub = 1; } +remove_id: /* If 'remove_id' exists, remove the device id from pci-stub's dynamic * ID table so that 'drivers_probe' works below. */ if (pciDriverFile(&path, driver, "remove_id") < 0) { + /* We do not remove PCI ID from pci-stub, and we can not reprobe it */ + if (dev->reprobe) { + VIR_WARN("Not remove PCI ID '%s' from %s, and the device can" + "not be reprobed again.", dev->id, driver); + } + dev->reprobe = 0; goto cleanup; } @@ -1018,6 +1086,13 @@ pciBindDeviceToStub(pciDevice *dev, const char *driver) virReportSystemError(errno, _("Failed to remove PCI ID '%s' from %s"), dev->id, driver); + + /* remove PCI ID from pci-stub failed, and we can not reprobe it */ + if (dev->reprobe) { + VIR_WARN("Faile to remove PCI ID '%s' from %s, and the device can" + "not be reprobed again.", dev->id, driver); + } + dev->reprobe = 0; goto cleanup; } @@ -1027,6 +1102,10 @@ cleanup: VIR_FREE(drvdir); VIR_FREE(path); + if (result < 0) { + pciUnbindDeviceFromStub(dev, driver); + } + return result; } -- 1.7.1
On 04/06/2011 01:13 AM, Wen Congyang wrote:
We should bind pci device to original driver when pciBindDeviceToStub() failed. If the pci device is not bound to any driver before calling pciBindDeviceToStub(), we should only unbind it from pci-stub. If it is bound to pci-stub, we should not unbid it from pci-stub.
s/unbid/unbind/
+remove_id: /* If 'remove_id' exists, remove the device id from pci-stub's dynamic * ID table so that 'drivers_probe' works below. */ if (pciDriverFile(&path, driver, "remove_id") < 0) { + /* We do not remove PCI ID from pci-stub, and we can not reprobe it */ + if (dev->reprobe) { + VIR_WARN("Not remove PCI ID '%s' from %s, and the device can" + "not be reprobed again.", dev->id, driver);
s/Not remove/Could not remove/ s/can not/cannot/ s/reprobed again/probed again/
@@ -1018,6 +1086,13 @@ pciBindDeviceToStub(pciDevice *dev, const char *driver) virReportSystemError(errno, _("Failed to remove PCI ID '%s' from %s"), dev->id, driver); + + /* remove PCI ID from pci-stub failed, and we can not reprobe it */ + if (dev->reprobe) { + VIR_WARN("Faile to remove PCI ID '%s' from %s, and the device can" + "not be reprobed again.", dev->id, driver);
s/Faile /Failed / s/reprobed again/probed again/ ACK with those warning message grammar cleanups. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
At 04/07/2011 03:21 AM, Eric Blake Write:
On 04/06/2011 01:13 AM, Wen Congyang wrote:
We should bind pci device to original driver when pciBindDeviceToStub() failed. If the pci device is not bound to any driver before calling pciBindDeviceToStub(), we should only unbind it from pci-stub. If it is bound to pci-stub, we should not unbid it from pci-stub.
s/unbid/unbind/
+remove_id: /* If 'remove_id' exists, remove the device id from pci-stub's dynamic * ID table so that 'drivers_probe' works below. */ if (pciDriverFile(&path, driver, "remove_id") < 0) { + /* We do not remove PCI ID from pci-stub, and we can not reprobe it */ + if (dev->reprobe) { + VIR_WARN("Not remove PCI ID '%s' from %s, and the device can" + "not be reprobed again.", dev->id, driver);
s/Not remove/Could not remove/ s/can not/cannot/ s/reprobed again/probed again/
@@ -1018,6 +1086,13 @@ pciBindDeviceToStub(pciDevice *dev, const char *driver) virReportSystemError(errno, _("Failed to remove PCI ID '%s' from %s"), dev->id, driver); + + /* remove PCI ID from pci-stub failed, and we can not reprobe it */ + if (dev->reprobe) { + VIR_WARN("Faile to remove PCI ID '%s' from %s, and the device can" + "not be reprobed again.", dev->id, driver);
s/Faile /Failed / s/reprobed again/probed again/
ACK with those warning message grammar cleanups.
I fix these warning message grammar, and pushed these serial patches. Thanks for reviewing.
Reattach all pci devices that we detached when qemuPrepareHostdevPCIDevices() failed. --- src/qemu/qemu_hostdev.c | 12 +++++++++--- 1 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 30db0e2..7f5ad51 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -127,11 +127,11 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); if (!pciDeviceIsAssignable(dev, !driver->relaxedACS)) - goto cleanup; + goto reattachdevs; if (pciDeviceGetManaged(dev) && pciDettachDevice(dev, driver->activePciHostdevs) < 0) - goto cleanup; + goto reattachdevs; } /* Now that all the PCI hostdevs have be dettached, we can safely @@ -139,7 +139,7 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); if (pciResetDevice(dev, driver->activePciHostdevs, pcidevs) < 0) - goto cleanup; + goto reattachdevs; } /* Now mark all the devices as active */ @@ -169,6 +169,12 @@ inactivedevs: pciDeviceListSteal(driver->activePciHostdevs, dev); } +reattachdevs: + for (i = 0; i < pciDeviceListCount(pcidevs); i++) { + pciDevice *dev = pciDeviceListGet(pcidevs, i); + pciReAttachDevice(dev, driver->activePciHostdevs); + } + cleanup: pciDeviceListFree(pcidevs); return ret; -- 1.7.1
On 03/28/2011 01:01 AM, Wen Congyang wrote:
Reattach all pci devices that we detached when qemuPrepareHostdevPCIDevices() failed.
--- src/qemu/qemu_hostdev.c | 12 +++++++++--- 1 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 30db0e2..7f5ad51 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -127,11 +127,11 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); if (!pciDeviceIsAssignable(dev, !driver->relaxedACS)) - goto cleanup; + goto reattachdevs;
if (pciDeviceGetManaged(dev) && pciDettachDevice(dev, driver->activePciHostdevs) < 0) - goto cleanup; + goto reattachdevs;
Makes sense; ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Wen Congyang