[libvirt] [PATCH] qemu: Get the dev from activePciHostdevs list before reattachment
BZ# https://bugzilla.redhat.com/show_bug.cgi?id=736214 The problem is caused by the original info of domain's PCI dev is maintained by qemu_driver->activePciHostdevs list, (E.g. dev->reprobe, which stands for whether need to reprobe driver for the dev when do reattachment). The fields (dev->reprobe, dev->unbind_from_stub, and dev->remove_slot) are initialized properly when preparing the PCI device for managed attachment. However, when do reattachment, it construct a complete new "pciDevice" without honoring the original dev info, and thus the dev won't get the original driver or can get other problem. This patch is to fix the problem by get the dev from list driver->activePciHostdevs if it's in the list, though it's unlikely the dev to be reattached is not in the list, as any PCI dev is added to the list when do preparation (qemuPrepareHostdevPCIDevices), the patch doesn't completely desert the old design so the device can be reattached even if it's not in list driver->activePciHostdevs. -- v1 patch to fix this problem is: http://www.redhat.com/archives/libvir-list/2011-September/msg01101.html But the patch title is complete different, so I don't send this and a v2. --- src/qemu/qemu_hostdev.c | 23 +++++++++++++++++++++-- 1 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 6f77717..73cadc5 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -262,6 +262,7 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, int nhostdevs) { pciDeviceList *pcidevs; + pciDeviceList *pci_list = NULL; int i; if (!(pcidevs = qemuGetPciHostDeviceList(hostdevs, nhostdevs))) { @@ -272,12 +273,23 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, return; } + if (!(pci_list = pciDeviceListNew())) + return; + /* Again 3 loops; mark all devices as inactive before reset * them and reset all the devices before re-attach */ - for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); - pciDeviceListDel(driver->activePciHostdevs, dev); + pciDevice *pci = NULL; + pci = pciDeviceListSteal(driver->activePciHostdevs, dev); + + if (pci) { + pciDeviceListDel(pcidevs, dev); + if (pciDeviceListAdd(pci_list, pci) < 0) { + pciFreeDevice(pci); + goto cleanup; + } + } } for (i = 0; i < pciDeviceListCount(pcidevs); i++) { @@ -290,11 +302,18 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, } } + for (i = 0; i < pciDeviceListCount(pci_list); i++) { + pciDevice *dev = pciDeviceListGet(pci_list, i); + qemuReattachPciDevice(dev, driver); + } + for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); qemuReattachPciDevice(dev, driver); } +cleanup: + pciDeviceListFree(pci_list); pciDeviceListFree(pcidevs); } -- 1.7.6
On 10/11/2011 01:59 AM, Osier Yang wrote:
BZ# https://bugzilla.redhat.com/show_bug.cgi?id=736214
The problem is caused by the original info of domain's PCI dev is maintained by qemu_driver->activePciHostdevs list, (E.g. dev->reprobe, which stands for whether need to reprobe driver for the dev when do reattachment). The fields (dev->reprobe, dev->unbind_from_stub, and dev->remove_slot) are initialized properly when preparing the PCI device for managed attachment. However, when do reattachment, it construct a complete new "pciDevice" without honoring the original dev info, and thus the dev won't get the original driver or can get other problem.
This patch is to fix the problem by get the dev from list driver->activePciHostdevs if it's in the list, though it's unlikely the dev to be reattached is not in the list, as any PCI dev is added to the list when do preparation (qemuPrepareHostdevPCIDevices), the patch doesn't completely desert the old design so the device can be reattached even if it's not in list driver->activePciHostdevs.
@@ -272,12 +273,23 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, return; }
+ if (!(pci_list = pciDeviceListNew())) + return;
Memory leak. You must goto cleanup, so that pcidevs is not lost.
+ /* Again 3 loops; mark all devices as inactive before reset * them and reset all the devices before re-attach */
Comment says 3 loops, but you added a fourth one (although conceptually, the last two loops are really a combination of one visit to the complete set of devices, divided into two subsets according to whether we are reusing active information).
- for (i = 0; i< pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); - pciDeviceListDel(driver->activePciHostdevs, dev); + pciDevice *pci = NULL; + pci = pciDeviceListSteal(driver->activePciHostdevs, dev); + + if (pci) { + pciDeviceListDel(pcidevs, dev); + if (pciDeviceListAdd(pci_list, pci)< 0) { + pciFreeDevice(pci); + goto cleanup; + } + } }
I'm wondering if all this transfer work between two lists is really necessary, or if there is a more efficient way to just copy the information from driver->activePciHostdevs back into pcidevs, without having to create a second pci_list. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年10月13日 01:31, Eric Blake 写道:
On 10/11/2011 01:59 AM, Osier Yang wrote:
BZ# https://bugzilla.redhat.com/show_bug.cgi?id=736214
The problem is caused by the original info of domain's PCI dev is maintained by qemu_driver->activePciHostdevs list, (E.g. dev->reprobe, which stands for whether need to reprobe driver for the dev when do reattachment). The fields (dev->reprobe, dev->unbind_from_stub, and dev->remove_slot) are initialized properly when preparing the PCI device for managed attachment. However, when do reattachment, it construct a complete new "pciDevice" without honoring the original dev info, and thus the dev won't get the original driver or can get other problem.
This patch is to fix the problem by get the dev from list driver->activePciHostdevs if it's in the list, though it's unlikely the dev to be reattached is not in the list, as any PCI dev is added to the list when do preparation (qemuPrepareHostdevPCIDevices), the patch doesn't completely desert the old design so the device can be reattached even if it's not in list driver->activePciHostdevs.
@@ -272,12 +273,23 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, return; }
+ if (!(pci_list = pciDeviceListNew())) + return;
Memory leak. You must goto cleanup, so that pcidevs is not lost.
+ /* Again 3 loops; mark all devices as inactive before reset * them and reset all the devices before re-attach */
Comment says 3 loops, but you added a fourth one (although conceptually, the last two loops are really a combination of one visit to the complete set of devices, divided into two subsets according to whether we are reusing active information).
- for (i = 0; i< pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); - pciDeviceListDel(driver->activePciHostdevs, dev); + pciDevice *pci = NULL; + pci = pciDeviceListSteal(driver->activePciHostdevs, dev); + + if (pci) { + pciDeviceListDel(pcidevs, dev); + if (pciDeviceListAdd(pci_list, pci)< 0) { + pciFreeDevice(pci); + goto cleanup; + } + } }
I'm wondering if all this transfer work between two lists is really necessary, or if there is a more efficient way to just copy the information from driver->activePciHostdevs back into pcidevs, without having to create a second pci_list.
I was wondering this too, definitely it will be more efficient to just use one list, but you known, I'm afraid it could break something, though it's unlikely currently, as qemuDomainReAttachHostdevDevices is only used by qemuDomainAttachHostPciDevice (when attaching a PCI), and qemuProcessStop, (when destroying a domain, or fails on starting the domain). This means the all the devices which will be reattached by qemuDomainReAttachHostdevDevices are already initialized as an element of driver->activePciHostdevs during preparation (qemuDomainReAttachHostdevDevices). Osier
于 2011年10月13日 09:05, Osier Yang 写道:
于 2011年10月13日 01:31, Eric Blake 写道:
On 10/11/2011 01:59 AM, Osier Yang wrote:
BZ# https://bugzilla.redhat.com/show_bug.cgi?id=736214
The problem is caused by the original info of domain's PCI dev is maintained by qemu_driver->activePciHostdevs list, (E.g. dev->reprobe, which stands for whether need to reprobe driver for the dev when do reattachment). The fields (dev->reprobe, dev->unbind_from_stub, and dev->remove_slot) are initialized properly when preparing the PCI device for managed attachment. However, when do reattachment, it construct a complete new "pciDevice" without honoring the original dev info, and thus the dev won't get the original driver or can get other problem.
This patch is to fix the problem by get the dev from list driver->activePciHostdevs if it's in the list, though it's unlikely the dev to be reattached is not in the list, as any PCI dev is added to the list when do preparation (qemuPrepareHostdevPCIDevices), the patch doesn't completely desert the old design so the device can be reattached even if it's not in list driver->activePciHostdevs.
@@ -272,12 +273,23 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, return; }
+ if (!(pci_list = pciDeviceListNew())) + return;
Memory leak. You must goto cleanup, so that pcidevs is not lost.
+ /* Again 3 loops; mark all devices as inactive before reset * them and reset all the devices before re-attach */
Comment says 3 loops, but you added a fourth one (although conceptually, the last two loops are really a combination of one visit to the complete set of devices, divided into two subsets according to whether we are reusing active information).
- for (i = 0; i< pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); - pciDeviceListDel(driver->activePciHostdevs, dev); + pciDevice *pci = NULL; + pci = pciDeviceListSteal(driver->activePciHostdevs, dev); + + if (pci) { + pciDeviceListDel(pcidevs, dev); + if (pciDeviceListAdd(pci_list, pci)< 0) { + pciFreeDevice(pci); + goto cleanup; + } + } }
I'm wondering if all this transfer work between two lists is really necessary, or if there is a more efficient way to just copy the information from driver->activePciHostdevs back into pcidevs, without having to create a second pci_list.
I was wondering this too, definitely it will be more efficient to just use one list, but you known, I'm afraid it could break something, though it's unlikely currently, as qemuDomainReAttachHostdevDevices is only used by qemuDomainAttachHostPciDevice (when attaching a PCI), and qemuProcessStop, (when destroying a domain, or fails on starting the domain). This means the all the devices which will be reattached by qemuDomainReAttachHostdevDevices are already initialized as an element of driver->activePciHostdevs during preparation (qemuDomainReAttachHostdevDevices).
Ok, I made two different versions, one keeps using 2 list, while another uses just one. Let's choose. :-) Osier
BZ# https://bugzilla.redhat.com/show_bug.cgi?id=736214 The problem is caused by the original info of domain's PCI dev is maintained by qemu_driver->activePciHostdevs list, (E.g. dev->reprobe, which stands for whether need to reprobe driver for the dev when do reattachment). The fields (dev->reprobe, dev->unbind_from_stub, and dev->remove_slot) are initialized properly when preparing the PCI device for managed attachment. However, when do reattachment, it construct a complete new "pciDevice" without honoring the original dev info, and thus the dev won't get the original driver or can get other problem. This patch is to fix the problem by get the devs from list driver->activePciHostdevs. Tested with following 3 scenarios: * the PCI was bound to some driver not pci-stub before attaching result: the device will be bound to the original driver * the PCI was bound to pci-stub before attaching result: no driver reprobing, and still bound to pci-stub * The PCI was not bound to any driver result: no driver reproing, and still not bound to any driver. --- src/qemu/qemu_hostdev.c | 51 ++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 48 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 6f77717..d8531ca 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -69,6 +69,49 @@ qemuGetPciHostDeviceList(virDomainHostdevDefPtr *hostdevs, int nhostdevs) return list; } +static pciDeviceList * +qemuGetActivePciHostDeviceList(struct qemud_driver *driver, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs) +{ + pciDeviceList *list; + int i; + + if (!(list = pciDeviceListNew())) + return NULL; + + for (i = 0 ; i < nhostdevs ; i++) { + virDomainHostdevDefPtr hostdev = hostdevs[i]; + pciDevice *dev; + pciDevice *activeDev; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + continue; + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) + continue; + + dev = pciGetDevice(hostdev->source.subsys.u.pci.domain, + hostdev->source.subsys.u.pci.bus, + hostdev->source.subsys.u.pci.slot, + hostdev->source.subsys.u.pci.function); + if (!dev) { + pciDeviceListFree(list); + return NULL; + } + + if ((activeDev = pciDeviceListFind(driver->activePciHostdevs, dev))) { + if (pciDeviceListAdd(list, activeDev) < 0) { + pciFreeDevice(dev); + pciDeviceListFree(list); + return NULL; + } + } + + pciFreeDevice(dev); + } + + return list; +} int qemuUpdateActivePciHostdevs(struct qemud_driver *driver, virDomainDefPtr def) @@ -264,7 +307,9 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, pciDeviceList *pcidevs; int i; - if (!(pcidevs = qemuGetPciHostDeviceList(hostdevs, nhostdevs))) { + if (!(pcidevs = qemuGetActivePciHostDeviceList(driver, + hostdevs, + nhostdevs))) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to allocate pciDeviceList: %s"), err ? err->message : _("unknown error")); @@ -274,10 +319,10 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, /* Again 3 loops; mark all devices as inactive before reset * them and reset all the devices before re-attach */ - for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); - pciDeviceListDel(driver->activePciHostdevs, dev); + /* pciDeviceListFree() will take care of freeing the dev. */ + pciDeviceListSteal(driver->activePciHostdevs, dev); } for (i = 0; i < pciDeviceListCount(pcidevs); i++) { -- 1.7.6
On 10/13/2011 02:30 AM, Osier Yang wrote:
BZ# https://bugzilla.redhat.com/show_bug.cgi?id=736214
The problem is caused by the original info of domain's PCI dev is maintained by qemu_driver->activePciHostdevs list, (E.g. dev->reprobe, which stands for whether need to reprobe driver for the dev when do reattachment). The fields (dev->reprobe, dev->unbind_from_stub, and dev->remove_slot) are initialized properly when preparing the PCI device for managed attachment. However, when do reattachment, it construct a complete new "pciDevice" without honoring the original dev info, and thus the dev won't get the original driver or can get other problem.
This patch is to fix the problem by get the devs from list driver->activePciHostdevs.
Tested with following 3 scenarios: * the PCI was bound to some driver not pci-stub before attaching
result: the device will be bound to the original driver
* the PCI was bound to pci-stub before attaching
result: no driver reprobing, and still bound to pci-stub
* The PCI was not bound to any driver
result: no driver reproing, and still not bound to any driver.
s/reproing/reprobing/ I like v3 better, even though it has the longer diffstat, because it divides the work into smaller self-contained functions rather than making an already long function longer. However, I got a merge conflict when applying on top of your other pci hostdev fixup patch; but I think I resolved that correctly. ACK and pushed, after I reproduced the issue and tested that the patch fixes it. It didn't help matters that virt-manager has a bug. It manually calls the equivalent of 'nodedev-detach' prior to hot-plugging any hostdev device, _even though it requests managed=1_. That means the device is already bound to pci-stub before the hotplug attempt is started; and on failure to hotplug, libvirt _faithfully_ restores the the device back to its original state (still pci-stub), but virt-manager does not recognize the error condition and does not call nodedev-reattach. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年10月15日 05:47, Eric Blake 写道:
On 10/13/2011 02:30 AM, Osier Yang wrote:
BZ# https://bugzilla.redhat.com/show_bug.cgi?id=736214
The problem is caused by the original info of domain's PCI dev is maintained by qemu_driver->activePciHostdevs list, (E.g. dev->reprobe, which stands for whether need to reprobe driver for the dev when do reattachment). The fields (dev->reprobe, dev->unbind_from_stub, and dev->remove_slot) are initialized properly when preparing the PCI device for managed attachment. However, when do reattachment, it construct a complete new "pciDevice" without honoring the original dev info, and thus the dev won't get the original driver or can get other problem.
This patch is to fix the problem by get the devs from list driver->activePciHostdevs.
Tested with following 3 scenarios: * the PCI was bound to some driver not pci-stub before attaching
result: the device will be bound to the original driver
* the PCI was bound to pci-stub before attaching
result: no driver reprobing, and still bound to pci-stub
* The PCI was not bound to any driver
result: no driver reproing, and still not bound to any driver.
s/reproing/reprobing/
I like v3 better, even though it has the longer diffstat, because it divides the work into smaller self-contained functions rather than making an already long function longer.
However, I got a merge conflict when applying on top of your other pci hostdev fixup patch; but I think I resolved that correctly.
ACK and pushed, after I reproduced the issue and tested that the patch fixes it.
Thanks.
It didn't help matters that virt-manager has a bug. It manually calls the equivalent of 'nodedev-detach' prior to hot-plugging any hostdev device, _even though it requests managed=1_. That means the device is already bound to pci-stub before the hotplug attempt is started; and on failure to hotplug, libvirt _faithfully_ restores the the device back to its original state (still pci-stub), but virt-manager does not recognize the error condition and does not call nodedev-reattach.
Probally we need to improve the documents for "managed" host device, user completely doesn't known what will happen if the device was bound to driver not pci-stub or not bound to any driver from the existed documents. " When <code>managed</code> is "yes" for a PCI device, it is detached from the host before being passed on to the guest." I could imaging one day some way will file bug for it. And also "managed" is used for USB device too, however, we don't do any reattachment for USB device actually. Do we need documents to tell the truth? though we might add the support soon. Osier
于 2011年10月15日 05:47, Eric Blake 写道:
On 10/13/2011 02:30 AM, Osier Yang wrote:
BZ# https://bugzilla.redhat.com/show_bug.cgi?id=736214
The problem is caused by the original info of domain's PCI dev is maintained by qemu_driver->activePciHostdevs list, (E.g. dev->reprobe, which stands for whether need to reprobe driver for the dev when do reattachment). The fields (dev->reprobe, dev->unbind_from_stub, and dev->remove_slot) are initialized properly when preparing the PCI device for managed attachment. However, when do reattachment, it construct a complete new "pciDevice" without honoring the original dev info, and thus the dev won't get the original driver or can get other problem.
This patch is to fix the problem by get the devs from list driver->activePciHostdevs.
Tested with following 3 scenarios: * the PCI was bound to some driver not pci-stub before attaching
result: the device will be bound to the original driver
* the PCI was bound to pci-stub before attaching
result: no driver reprobing, and still bound to pci-stub
* The PCI was not bound to any driver
result: no driver reproing, and still not bound to any driver.
s/reproing/reprobing/
I like v3 better, even though it has the longer diffstat, because it divides the work into smaller self-contained functions rather than making an already long function longer.
However, I got a merge conflict when applying on top of your other pci hostdev fixup patch; but I think I resolved that correctly.
ACK and pushed, after I reproduced the issue and tested that the patch fixes it.
It didn't help matters that virt-manager has a bug. It manually calls the equivalent of 'nodedev-detach' prior to hot-plugging any hostdev device, _even though it requests managed=1_. That means the device is already bound to pci-stub before the hotplug attempt is started; and on failure to hotplug, libvirt _faithfully_ restores the the device back to its original state (still pci-stub), but virt-manager does not recognize the error condition and does not call nodedev-reattach.
Also actually commit 2ce90ea296 is not a proper fix, it breaks following two 2 scenarios. * the PCI was bound to pci-stub before attaching result: no driver reprobing, and still bound to pci-stub * The PCI was not bound to any driver result: no driver reproing, and still not bound to any driver. Need to make a patch. Osier
BZ# https://bugzilla.redhat.com/show_bug.cgi?id=736214 The problem is caused by the original info of domain's PCI dev is maintained by qemu_driver->activePciHostdevs list, (E.g. dev->reprobe, which stands for whether need to reprobe driver for the dev when do reattachment). The fields (dev->reprobe, dev->unbind_from_stub, and dev->remove_slot) are initialized properly when preparing the PCI device for managed attachment. However, when do reattachment, it construct a complete new "pciDevice" without honoring the original dev info, and thus the dev won't get the original driver or can get other problem. This patch is to fix the problem by get the dev from list driver->activePciHostdevs if it's in the list, though it's unlikely the dev to be reattached is not in the list, as any PCI dev is added to the list when do preparation (qemuPrepareHostdevPCIDevices), the patch doesn't completely desert the old design so the device can be reattached even if it's not in list driver->activePciHostdevs. Tested with following 3 scenarios: * the PCI was bound to some driver not pci-stub before attaching result: the device will be bound to the original driver * the PCI was bound to pci-stub before attaching result: no driver reprobing, and still bound to pci-stub * The PCI was not bound to any driver result: no driver reproing, and still not bound to any driver. v1 ~ v2: * Fix memory leak. * Add comments to explain why there are too loops to reattach devs. --- src/qemu/qemu_hostdev.c | 23 +++++++++++++++++++++-- 1 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 6f77717..4c6555d 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -262,6 +262,7 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, int nhostdevs) { pciDeviceList *pcidevs; + pciDeviceList *activePciDevs = NULL; int i; if (!(pcidevs = qemuGetPciHostDeviceList(hostdevs, nhostdevs))) { @@ -272,12 +273,23 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, return; } + if (!(activePciDevs = pciDeviceListNew())) + goto cleanup; + /* Again 4 loops; mark all devices as inactive before reset * them and reset all the devices before re-attach */ - for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); - pciDeviceListDel(driver->activePciHostdevs, dev); + pciDevice *activeDev = NULL; + activeDev = pciDeviceListSteal(driver->activePciHostdevs, dev); + + if (activeDev) { + pciDeviceListDel(pcidevs, dev); + if (pciDeviceListAdd(activePciDevs, activeDev) < 0) { + pciFreeDevice(activeDev); + goto cleanup; + } + } } for (i = 0; i < pciDeviceListCount(pcidevs); i++) { @@ -290,11 +302,18 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, } } /* Reattach active devs, honoring the original dev info * from driver->activePciHostdevs. */ + for (i = 0; i < pciDeviceListCount(activePciDevs); i++) { + pciDevice *dev = pciDeviceListGet(activePciDevs, i); + qemuReattachPciDevice(dev, driver); + } + /* Reattach inactive devs, though this is unlikely to happen, * as all the domain PCI devices are added to list * driver->activePciHostdevs and marked as active when do * preparation (qemuPrepareHostdevPCIDevices). */ for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); qemuReattachPciDevice(dev, driver); } +cleanup: + pciDeviceListFree(activePciDevs); pciDeviceListFree(pcidevs); } -- 1.7.6
participants (2)
-
Eric Blake -
Osier Yang