[libvirt] [PATCH 0/2]: Small fixes for PCI device assignment
This series fixes a couple of bugs found while testing libvirt PCI device assignment.
In the current libvirt PCI code, there is no checking whether a PCI device is in use by a guest when doing node device detach or reattach. This causes problems when a device is assigned to a guest, and the administrator starts issuing nodedevice commands. Make it so that we check the list of active devices when trying to detach/reattach, and only allow the operation if the device is not assigned to a guest. Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/qemu/qemu_driver.c | 20 +++++++++++--------- src/util/pci.c | 16 ++++++++++++++-- src/util/pci.h | 4 ++-- src/xen/xen_driver.c | 4 ++-- 4 files changed, 29 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fc5585e..66e9f52 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2858,7 +2858,7 @@ qemuPrepareHostPCIDevices(struct qemud_driver *driver, goto cleanup; if (pciDeviceGetManaged(dev) && - pciDettachDevice(dev) < 0) + pciDettachDevice(dev, driver->activePciHostdevs) < 0) goto cleanup; } @@ -2938,7 +2938,7 @@ qemuPrepareHostDevices(struct qemud_driver *driver, static void -qemudReattachManagedDevice(pciDevice *dev) +qemudReattachManagedDevice(pciDevice *dev, struct qemud_driver *driver) { int retries = 100; @@ -2948,7 +2948,7 @@ qemudReattachManagedDevice(pciDevice *dev) usleep(100*1000); retries--; } - if (pciReAttachDevice(dev) < 0) { + if (pciReAttachDevice(dev, driver->activePciHostdevs) < 0) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to re-attach PCI device: %s"), err ? err->message : _("unknown error")); @@ -2995,7 +2995,7 @@ qemuDomainReAttachHostDevices(struct qemud_driver *driver, for (i = 0; i < pciDeviceListCount(pcidevs); i++) { pciDevice *dev = pciDeviceListGet(pcidevs, i); - qemudReattachManagedDevice(dev); + qemudReattachManagedDevice(dev, driver); } pciDeviceListFree(pcidevs); @@ -7727,7 +7727,7 @@ static int qemudDomainAttachHostPciDevice(struct qemud_driver *driver, return -1; if (!pciDeviceIsAssignable(pci, !driver->relaxedACS) || - (hostdev->managed && pciDettachDevice(pci) < 0) || + (hostdev->managed && pciDettachDevice(pci, driver->activePciHostdevs) < 0) || pciResetDevice(pci, driver->activePciHostdevs) < 0) { pciFreeDevice(pci); return -1; @@ -7815,7 +7815,7 @@ error: if (pciResetDevice(pci, driver->activePciHostdevs) < 0) VIR_WARN0("Unable to reset PCI device after assign failure"); else if (hostdev->managed && - pciReAttachDevice(pci) < 0) + pciReAttachDevice(pci, driver->activePciHostdevs) < 0) VIR_WARN0("Unable to re-attach PCI device after assign failure"); pciFreeDevice(pci); @@ -8726,7 +8726,7 @@ static int qemudDomainDetachHostPciDevice(struct qemud_driver *driver, pciDeviceListDel(driver->activePciHostdevs, pci); if (pciResetDevice(pci, driver->activePciHostdevs) < 0) ret = -1; - qemudReattachManagedDevice(pci); + qemudReattachManagedDevice(pci, driver); pciFreeDevice(pci); } @@ -11179,6 +11179,7 @@ out: static int qemudNodeDeviceDettach (virNodeDevicePtr dev) { + struct qemud_driver *driver = dev->conn->privateData; pciDevice *pci; unsigned domain, bus, slot, function; int ret = -1; @@ -11190,7 +11191,7 @@ qemudNodeDeviceDettach (virNodeDevicePtr dev) if (!pci) return -1; - if (pciDettachDevice(pci) < 0) + if (pciDettachDevice(pci, driver->activePciHostdevs) < 0) goto out; ret = 0; @@ -11202,6 +11203,7 @@ out: static int qemudNodeDeviceReAttach (virNodeDevicePtr dev) { + struct qemud_driver *driver = dev->conn->privateData; pciDevice *pci; unsigned domain, bus, slot, function; int ret = -1; @@ -11213,7 +11215,7 @@ qemudNodeDeviceReAttach (virNodeDevicePtr dev) if (!pci) return -1; - if (pciReAttachDevice(pci) < 0) + if (pciReAttachDevice(pci, driver->activePciHostdevs) < 0) goto out; ret = 0; diff --git a/src/util/pci.c b/src/util/pci.c index 0c6d308..fe128db 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -815,7 +815,7 @@ pciBindDeviceToStub(pciDevice *dev, const char *driver) } int -pciDettachDevice(pciDevice *dev) +pciDettachDevice(pciDevice *dev, pciDeviceList *activeDevs) { const char *driver = pciFindStubDriver(); if (!driver) { @@ -824,6 +824,12 @@ pciDettachDevice(pciDevice *dev) return -1; } + if (activeDevs && pciDeviceListFind(activeDevs, dev)) { + pciReportError(VIR_ERR_INTERNAL_ERROR, + _("Not detaching active device %s"), dev->name); + return -1; + } + return pciBindDeviceToStub(dev, driver); } @@ -876,7 +882,7 @@ pciUnBindDeviceFromStub(pciDevice *dev, const char *driver) } int -pciReAttachDevice(pciDevice *dev) +pciReAttachDevice(pciDevice *dev, pciDeviceList *activeDevs) { const char *driver = pciFindStubDriver(); if (!driver) { @@ -885,6 +891,12 @@ pciReAttachDevice(pciDevice *dev) return -1; } + if (activeDevs && pciDeviceListFind(activeDevs, dev)) { + pciReportError(VIR_ERR_INTERNAL_ERROR, + _("Not reattaching active device %s"), dev->name); + return -1; + } + return pciUnBindDeviceFromStub(dev, driver); } diff --git a/src/util/pci.h b/src/util/pci.h index fdef18c..7fe3d1f 100644 --- a/src/util/pci.h +++ b/src/util/pci.h @@ -32,8 +32,8 @@ pciDevice *pciGetDevice (unsigned domain, unsigned slot, unsigned function); void pciFreeDevice (pciDevice *dev); -int pciDettachDevice (pciDevice *dev); -int pciReAttachDevice (pciDevice *dev); +int pciDettachDevice (pciDevice *dev, pciDeviceList *activeDevs); +int pciReAttachDevice (pciDevice *dev, pciDeviceList *activeDevs); int pciResetDevice (pciDevice *dev, pciDeviceList *activeDevs); void pciDeviceSetManaged(pciDevice *dev, diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 91f0acd..5f8efa7 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1838,7 +1838,7 @@ xenUnifiedNodeDeviceDettach (virNodeDevicePtr dev) if (!pci) return -1; - if (pciDettachDevice(pci) < 0) + if (pciDettachDevice(pci, NULL) < 0) goto out; ret = 0; @@ -1861,7 +1861,7 @@ xenUnifiedNodeDeviceReAttach (virNodeDevicePtr dev) if (!pci) return -1; - if (pciReAttachDevice(pci) < 0) + if (pciReAttachDevice(pci, NULL) < 0) goto out; ret = 0; -- 1.6.6.1
On Tue, Jun 15, 2010 at 08:03:11AM -0400, Chris Lalancette wrote:
In the current libvirt PCI code, there is no checking whether a PCI device is in use by a guest when doing node device detach or reattach. This causes problems when a device is assigned to a guest, and the administrator starts issuing nodedevice commands. Make it so that we check the list of active devices when trying to detach/reattach, and only allow the operation if the device is not assigned to a guest.
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/qemu/qemu_driver.c | 20 +++++++++++--------- src/util/pci.c | 16 ++++++++++++++-- src/util/pci.h | 4 ++-- src/xen/xen_driver.c | 4 ++-- 4 files changed, 29 insertions(+), 15 deletions(-)
@@ -11179,6 +11179,7 @@ out: static int qemudNodeDeviceDettach (virNodeDevicePtr dev) { + struct qemud_driver *driver = dev->conn->privateData; pciDevice *pci; unsigned domain, bus, slot, function; int ret = -1; @@ -11190,7 +11191,7 @@ qemudNodeDeviceDettach (virNodeDevicePtr dev) if (!pci) return -1;
- if (pciDettachDevice(pci) < 0) + if (pciDettachDevice(pci, driver->activePciHostdevs) < 0) goto out;
ret = 0; @@ -11202,6 +11203,7 @@ out: static int qemudNodeDeviceReAttach (virNodeDevicePtr dev) { + struct qemud_driver *driver = dev->conn->privateData; pciDevice *pci; unsigned domain, bus, slot, function; int ret = -1; @@ -11213,7 +11215,7 @@ qemudNodeDeviceReAttach (virNodeDevicePtr dev) if (!pci) return -1;
- if (pciReAttachDevice(pci) < 0) + if (pciReAttachDevice(pci, driver->activePciHostdevs) < 0) goto out;
ret = 0;
You're accessing 'driver' here without first locking it Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 06/15/10 - 01:13:00PM, Daniel P. Berrange wrote:
@@ -11179,6 +11179,7 @@ out: static int qemudNodeDeviceDettach (virNodeDevicePtr dev) { + struct qemud_driver *driver = dev->conn->privateData; pciDevice *pci; unsigned domain, bus, slot, function; int ret = -1; @@ -11190,7 +11191,7 @@ qemudNodeDeviceDettach (virNodeDevicePtr dev) if (!pci) return -1;
- if (pciDettachDevice(pci) < 0) + if (pciDettachDevice(pci, driver->activePciHostdevs) < 0) goto out;
ret = 0; @@ -11202,6 +11203,7 @@ out: static int qemudNodeDeviceReAttach (virNodeDevicePtr dev) { + struct qemud_driver *driver = dev->conn->privateData; pciDevice *pci; unsigned domain, bus, slot, function; int ret = -1; @@ -11213,7 +11215,7 @@ qemudNodeDeviceReAttach (virNodeDevicePtr dev) if (!pci) return -1;
- if (pciReAttachDevice(pci) < 0) + if (pciReAttachDevice(pci, driver->activePciHostdevs) < 0) goto out;
ret = 0;
You're accessing 'driver' here without first locking it
D'oh, of course. Thanks for that. Updated patch below.
On 06/15/2010 06:35 AM, Chris Lalancette wrote:
On 06/15/10 - 01:13:00PM, Daniel P. Berrange wrote:
You're accessing 'driver' here without first locking it
D'oh, of course. Thanks for that. Updated patch below.
From 56b1d0463790b52e23975968c54d4f9f9a3c5fbd Mon Sep 17 00:00:00 2001 From: Chris Lalancette <clalance@redhat.com> Date: Mon, 14 Jun 2010 17:12:35 -0400 Subject: [PATCH v2 1/2] Check for active PCI devices when doing nodedevice operations.
In the current libvirt PCI code, there is no checking whether a PCI device is in use by a guest when doing node device detach or reattach. This causes problems when a device is assigned to a guest, and the administrator starts issuing nodedevice commands. Make it so that we check the list of active devices when trying to detach/reattach, and only allow the operation if the device is not assigned to a guest.
v2: Add locking to qemudNodeDeviceDettach and qemudNodeDeviceReAttach
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- - pciDettachDevice(dev) < 0) + pciDettachDevice(dev, driver->activePciHostdevs) < 0)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 06/22/10 - 03:33:43PM, Eric Blake wrote:
On 06/15/2010 06:35 AM, Chris Lalancette wrote:
On 06/15/10 - 01:13:00PM, Daniel P. Berrange wrote:
You're accessing 'driver' here without first locking it
D'oh, of course. Thanks for that. Updated patch below.
From 56b1d0463790b52e23975968c54d4f9f9a3c5fbd Mon Sep 17 00:00:00 2001 From: Chris Lalancette <clalance@redhat.com> Date: Mon, 14 Jun 2010 17:12:35 -0400 Subject: [PATCH v2 1/2] Check for active PCI devices when doing nodedevice operations.
In the current libvirt PCI code, there is no checking whether a PCI device is in use by a guest when doing node device detach or reattach. This causes problems when a device is assigned to a guest, and the administrator starts issuing nodedevice commands. Make it so that we check the list of active devices when trying to detach/reattach, and only allow the operation if the device is not assigned to a guest.
v2: Add locking to qemudNodeDeviceDettach and qemudNodeDeviceReAttach
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- - pciDettachDevice(dev) < 0) + pciDettachDevice(dev, driver->activePciHostdevs) < 0)
ACK.
Thanks, I pushed this now. -- Chris Lalancette
Make sure to *not* call qemuDomainPCIAddressReleaseAddr if QEMUD_CMD_FLAG_DEVICE is *not* set (for older qemu). This prevents a crash when trying to do device detachment from a qemu guest. Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/qemu/qemu_driver.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 66e9f52..93072a4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8522,9 +8522,9 @@ static int qemudDomainDetachPciControllerDevice(struct qemud_driver *driver, vm->def->ncontrollers = 0; } - if (qemuDomainPCIAddressReleaseAddr(priv->pciaddrs, &detach->info) < 0) { + if ((qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && + qemuDomainPCIAddressReleaseAddr(priv->pciaddrs, &detach->info) < 0) VIR_WARN0("Unable to release PCI address on controller"); - } virDomainControllerDefFree(detach); @@ -8730,9 +8730,9 @@ static int qemudDomainDetachHostPciDevice(struct qemud_driver *driver, pciFreeDevice(pci); } - if (qemuDomainPCIAddressReleaseAddr(priv->pciaddrs, &detach->info) < 0) { + if ((qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && + qemuDomainPCIAddressReleaseAddr(priv->pciaddrs, &detach->info) < 0) VIR_WARN0("Unable to release PCI address on controller"); - } if (vm->def->nhostdevs > 1) { memmove(vm->def->hostdevs + i, -- 1.6.6.1
On 06/15/2010 06:03 AM, Chris Lalancette wrote:
Make sure to *not* call qemuDomainPCIAddressReleaseAddr if QEMUD_CMD_FLAG_DEVICE is *not* set (for older qemu). This prevents a crash when trying to do device detachment from a qemu guest.
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/qemu/qemu_driver.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 66e9f52..93072a4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8522,9 +8522,9 @@ static int qemudDomainDetachPciControllerDevice(struct qemud_driver *driver, vm->def->ncontrollers = 0; }
- if (qemuDomainPCIAddressReleaseAddr(priv->pciaddrs, &detach->info) < 0) { + if ((qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && + qemuDomainPCIAddressReleaseAddr(priv->pciaddrs, &detach->info) < 0) VIR_WARN0("Unable to release PCI address on controller"); - }
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 06/22/10 - 03:38:19PM, Eric Blake wrote:
On 06/15/2010 06:03 AM, Chris Lalancette wrote:
Make sure to *not* call qemuDomainPCIAddressReleaseAddr if QEMUD_CMD_FLAG_DEVICE is *not* set (for older qemu). This prevents a crash when trying to do device detachment from a qemu guest.
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/qemu/qemu_driver.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 66e9f52..93072a4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8522,9 +8522,9 @@ static int qemudDomainDetachPciControllerDevice(struct qemud_driver *driver, vm->def->ncontrollers = 0; }
- if (qemuDomainPCIAddressReleaseAddr(priv->pciaddrs, &detach->info) < 0) { + if ((qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && + qemuDomainPCIAddressReleaseAddr(priv->pciaddrs, &detach->info) < 0) VIR_WARN0("Unable to release PCI address on controller"); - }
ACK.
Thanks, I pushed this now. -- Chris Lalancette
participants (3)
-
Chris Lalancette -
Daniel P. Berrange -
Eric Blake