[libvirt] [PATCH V2] xen: check if device is assigned to guest before reattaching
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=664059 This is the version2 patch for BZ#664059. Reattaching pci device back to host without destroying guest or detaching device from guest would cause host to crash. This patch adds a check before doing device reattach. If the device is being assigned to guest, libvirt refuses to reattach device to host. The patch only works for Xen, for it just checks xenstore to get pci device information. Signed-off-by: Yufang Zhang <yuzhang@redhat.com> --- src/xen/xen_driver.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 67 insertions(+), 0 deletions(-) diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 4c11b11..318bb6a 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1891,11 +1891,70 @@ out: } static int +xenUnifiedNodeDeviceAssignedDomainId (virNodeDevicePtr dev) +{ + int numdomains; + int ret = -1, i; + int *ids = NULL; + char *bdf = NULL; + char *xref = NULL; + unsigned int domain, bus, slot, function; + virConnectPtr conn = dev->conn; + xenUnifiedPrivatePtr priv = conn->privateData; + + /* Get active domains */ + numdomains = xenUnifiedNumOfDomains(conn); + if (numdomains < 0) { + return ret; + } + if (numdomains > 0){ + if (VIR_ALLOC_N(ids, numdomains) < 0){ + virReportOOMError(); + goto out; + } + if ((numdomains = xenUnifiedListDomains(conn, &ids[0], numdomains)) < 0){ + goto out; + } + } + + /* Get pci bdf */ + if (xenUnifiedNodeDeviceGetPciInfo(dev, &domain, &bus, &slot, &function) < 0) + goto out; + + if (virAsprintf(&bdf, "%04x:%02x:%02x.%0x", + domain, bus, slot, function) < 0) { + virReportOOMError(); + goto out; + } + + /* Check if bdf is assigned to one of active domains */ + for (i = 0; i < numdomains; i++ ){ + xenUnifiedLock(priv); + xref = xenStoreDomainGetPCIID(conn, ids[i], bdf); + xenUnifiedUnlock(priv); + if (xref == NULL) + continue; + else { + ret = ids[i]; + break; + } + } + + VIR_FREE(xref); + VIR_FREE(bdf); +out: + VIR_FREE(ids); + + return ret; +} + +static int xenUnifiedNodeDeviceReAttach (virNodeDevicePtr dev) { pciDevice *pci; unsigned domain, bus, slot, function; int ret = -1; + int domid; if (xenUnifiedNodeDeviceGetPciInfo(dev, &domain, &bus, &slot, &function) < 0) return -1; @@ -1904,6 +1963,14 @@ xenUnifiedNodeDeviceReAttach (virNodeDevicePtr dev) if (!pci) return -1; + /* Check if device is assigned to an active guest */ + if ((domid = xenUnifiedNodeDeviceAssignedDomainId(dev)) >= 0){ + xenUnifiedError(VIR_ERR_INTERNAL_ERROR, + _("Device %s has been assigned to guest %d"), + dev->name, domid); + goto out; + } + if (pciReAttachDevice(pci, NULL) < 0) goto out; -- 1.7.3.4
On 01/07/2011 12:15 AM, Yufang Zhang wrote:
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=664059
This is the version2 patch for BZ#664059. Reattaching pci device back to host without destroying guest or detaching device from guest would cause host to crash. This patch adds a check before doing device reattach. If the device is being assigned to guest, libvirt refuses to reattach device to host. The patch only works for Xen, for it just checks xenstore to get pci device information.
Signed-off-by: Yufang Zhang <yuzhang@redhat.com> --- src/xen/xen_driver.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 67 insertions(+), 0 deletions(-)
Wow, just noticed that this hasn't had any review - does it still apply to the latest upstream?
diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 4c11b11..318bb6a 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1891,11 +1891,70 @@ out: }
static int +xenUnifiedNodeDeviceAssignedDomainId (virNodeDevicePtr dev)
+{ + int numdomains; + int ret = -1, i; + int *ids = NULL; + char *bdf = NULL; + char *xref = NULL; + unsigned int domain, bus, slot, function; + virConnectPtr conn = dev->conn; + xenUnifiedPrivatePtr priv = conn->privateData; + + /* Get active domains */ + numdomains = xenUnifiedNumOfDomains(conn); + if (numdomains < 0) { + return ret; + } + if (numdomains > 0){ + if (VIR_ALLOC_N(ids, numdomains) < 0){
Formatting nit - space before {, as in: if (VIR_ALLOC_N(ids, numdomains) < 0) {
+ virReportOOMError(); + goto out; + } + if ((numdomains = xenUnifiedListDomains(conn, &ids[0], numdomains)) < 0){ + goto out; + } + } + + /* Get pci bdf */ + if (xenUnifiedNodeDeviceGetPciInfo(dev, &domain, &bus, &slot, &function) < 0) + goto out; + + if (virAsprintf(&bdf, "%04x:%02x:%02x.%0x", + domain, bus, slot, function) < 0) { + virReportOOMError(); + goto out; + } + + /* Check if bdf is assigned to one of active domains */ + for (i = 0; i < numdomains; i++ ){
Again, a formatting nit: for (i = 0; i < numdomains; i++) {
+ xenUnifiedLock(priv); + xref = xenStoreDomainGetPCIID(conn, ids[i], bdf); + xenUnifiedUnlock(priv);
This locks and unlocks around every iteration of the loop. Would it be better to hold the lock for the entire loop duration, so that the action is atomic (no other thread can interrupt things), or is it better to drop the lock each iteration? I'm not as familiar with the xen architecture to give a good answer to this off the top of my head.
+ if (xref == NULL) + continue; + else {
Formatting - if one branch of an if-else has {}, then both need it: if (xref == NULL) { continue; } else {
+ ret = ids[i]; + break; + } + } + + VIR_FREE(xref); + VIR_FREE(bdf); +out: + VIR_FREE(ids); + + return ret; +} + +static int xenUnifiedNodeDeviceReAttach (virNodeDevicePtr dev) { pciDevice *pci; unsigned domain, bus, slot, function; int ret = -1; + int domid;
if (xenUnifiedNodeDeviceGetPciInfo(dev, &domain, &bus, &slot, &function) < 0) return -1; @@ -1904,6 +1963,14 @@ xenUnifiedNodeDeviceReAttach (virNodeDevicePtr dev) if (!pci) return -1;
+ /* Check if device is assigned to an active guest */ + if ((domid = xenUnifiedNodeDeviceAssignedDomainId(dev)) >= 0){ + xenUnifiedError(VIR_ERR_INTERNAL_ERROR, + _("Device %s has been assigned to guest %d"), + dev->name, domid); + goto out; + } + if (pciReAttachDevice(pci, NULL) < 0) goto out;
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 04/26/2011 10:26 PM, Eric Blake wrote:
On 01/07/2011 12:15 AM, Yufang Zhang wrote:
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=664059
This is the version2 patch for BZ#664059. Reattaching pci device back to host without destroying guest or detaching device from guest would cause host to crash. This patch adds a check before doing device reattach. If the device is being assigned to guest, libvirt refuses to reattach device to host. The patch only works for Xen, for it just checks xenstore to get pci device information.
Signed-off-by: Yufang Zhang<yuzhang@redhat.com> --- src/xen/xen_driver.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 67 insertions(+), 0 deletions(-) Wow, just noticed that this hasn't had any review - does it still apply to the latest upstream?
The v3 patch has been sent which is based on latest upstream.
diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 4c11b11..318bb6a 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1891,11 +1891,70 @@ out: }
static int +xenUnifiedNodeDeviceAssignedDomainId (virNodeDevicePtr dev) +{ + int numdomains; + int ret = -1, i; + int *ids = NULL; + char *bdf = NULL; + char *xref = NULL; + unsigned int domain, bus, slot, function; + virConnectPtr conn = dev->conn; + xenUnifiedPrivatePtr priv = conn->privateData; + + /* Get active domains */ + numdomains = xenUnifiedNumOfDomains(conn); + if (numdomains< 0) { + return ret; + } + if (numdomains> 0){ + if (VIR_ALLOC_N(ids, numdomains)< 0){ Formatting nit - space before {, as in:
if (VIR_ALLOC_N(ids, numdomains)< 0) {
+ virReportOOMError(); + goto out; + } + if ((numdomains = xenUnifiedListDomains(conn,&ids[0], numdomains))< 0){ + goto out; + } + } + + /* Get pci bdf */ + if (xenUnifiedNodeDeviceGetPciInfo(dev,&domain,&bus,&slot,&function)< 0) + goto out; + + if (virAsprintf(&bdf, "%04x:%02x:%02x.%0x", + domain, bus, slot, function)< 0) { + virReportOOMError(); + goto out; + } + + /* Check if bdf is assigned to one of active domains */ + for (i = 0; i< numdomains; i++ ){ Again, a formatting nit:
for (i = 0; i< numdomains; i++) {
+ xenUnifiedLock(priv); + xref = xenStoreDomainGetPCIID(conn, ids[i], bdf); + xenUnifiedUnlock(priv); This locks and unlocks around every iteration of the loop. Would it be better to hold the lock for the entire loop duration, so that the action is atomic (no other thread can interrupt things), or is it better to drop the lock each iteration? I'm not as familiar with the xen architecture to give a good answer to this off the top of my head.
I think moving the lock-unlock pair out of loop should work.
+ if (xref == NULL) + continue; + else { Formatting - if one branch of an if-else has {}, then both need it:
if (xref == NULL) { continue; } else {
+ ret = ids[i]; + break; + } + } + + VIR_FREE(xref); + VIR_FREE(bdf); +out: + VIR_FREE(ids); + + return ret; +} + +static int xenUnifiedNodeDeviceReAttach (virNodeDevicePtr dev) { pciDevice *pci; unsigned domain, bus, slot, function; int ret = -1; + int domid;
if (xenUnifiedNodeDeviceGetPciInfo(dev,&domain,&bus,&slot,&function)< 0) return -1; @@ -1904,6 +1963,14 @@ xenUnifiedNodeDeviceReAttach (virNodeDevicePtr dev) if (!pci) return -1;
+ /* Check if device is assigned to an active guest */ + if ((domid = xenUnifiedNodeDeviceAssignedDomainId(dev))>= 0){ + xenUnifiedError(VIR_ERR_INTERNAL_ERROR, + _("Device %s has been assigned to guest %d"), + dev->name, domid); + goto out; + } + if (pciReAttachDevice(pci, NULL)< 0) goto out;
participants (2)
-
Eric Blake -
Yufang Zhang