7:03 a.m.
This series fixes a couple of bugs found while testing libvirt PCI
device assignment.
7:03 a.m.
New subject: [libvirt] [PATCH 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.
Signed-off-by: Chris Lalancette <clalance(a)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
7:13 a.m.
New subject: [libvirt] [PATCH 1/2] Check for active PCI devices when doing nodedevice operations.
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(a)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 :|
7:35 a.m.
New subject: [libvirt] [PATCH 1/2] Check for active PCI devices when doing nodedevice operations.
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.
4:33 p.m.
New subject: [libvirt] [PATCH 1/2] Check for active PCI devices when doing nodedevice operations.
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(a)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(a)redhat.com>
---
- pciDettachDevice(dev) < 0)
+ pciDettachDevice(dev, driver->activePciHostdevs) < 0)
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:39 a.m.
New subject: [libvirt] [PATCH 1/2] Check for active PCI devices when doing nodedevice operations.
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(a)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(a)redhat.com>
> ---
> - pciDettachDevice(dev) < 0)
> + pciDettachDevice(dev, driver->activePciHostdevs) < 0)
ACK.
Thanks, I pushed this now.
--
Chris Lalancette
7:03 a.m.
New subject: [libvirt] [PATCH 2/2] Fix crash when detaching devices from qemu domains.
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(a)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
4:38 p.m.
New subject: [libvirt] [PATCH 2/2] Fix crash when detaching devices from qemu domains.
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(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:39 a.m.
New subject: [libvirt] [PATCH 2/2] Fix crash when detaching devices from qemu domains.
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(a)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
5261
days inactive
5275
days old
8 comments
3 participants
participants (3)
-
Chris Lalancette -
Daniel P. Berrange -
Eric Blake