[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [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.
>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>
---
 src/qemu/qemu_driver.c |   24 +++++++++++++++---------
 src/util/pci.c         |   16 ++++++++++++++--
 src/util/pci.h         |    4 ++--
 src/xen/xen_driver.c   |    4 ++--
 4 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fc5585e..769ee0f 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,11 +11191,13 @@ qemudNodeDeviceDettach (virNodeDevicePtr dev)
     if (!pci)
         return -1;
 
-    if (pciDettachDevice(pci) < 0)
+    qemuDriverLock(driver);
+    if (pciDettachDevice(pci, driver->activePciHostdevs) < 0)
         goto out;
 
     ret = 0;
 out:
+    qemuDriverUnlock(driver);
     pciFreeDevice(pci);
     return ret;
 }
@@ -11202,6 +11205,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,11 +11217,13 @@ qemudNodeDeviceReAttach (virNodeDevicePtr dev)
     if (!pci)
         return -1;
 
-    if (pciReAttachDevice(pci) < 0)
+    qemuDriverLock(driver);
+    if (pciReAttachDevice(pci, driver->activePciHostdevs) < 0)
         goto out;
 
     ret = 0;
 out:
+    qemuDriverUnlock(driver);
     pciFreeDevice(pci);
     return ret;
 }
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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]