Make Xen VT-d PCI attach/detach work

The Xen code for making HVM VT-d PCI passthrough attach and detach
wasn't working properly:

1)  In xenDaemonAttachDevice(), we were always trying to reconfigure
a PCI passthrough device, even the first time we added it.  This was
because the code in virDomainXMLDevID() was not checking xenstore for
the existence of the device, and always returning 0 (meaning that
the device already existed).

2)  In xenDaemonDetachDevice(), we were trying to use "device_destroy"
to detach a PCI device.  While you would think that is the right
method to call, it's actually wrong for PCI devices.  In particular,
in upstream Xen (and soon in RHEL-5 Xen), device_configure is actually
used to destroy a PCI device.

To fix the attach
problem I add a lookup into xenstore to see if the device we are
trying to attach already exists.  To fix the detach problem I change
it so that for PCI detach (only), we use device_configure with the
appropriate sxpr to do the detachment.

* src/xen/xend_internal.c: don't use device_destroy for PCI devices
  and fix the other issues.
* src/xen/xs_internal.c src/xen/xs_internal.h: add
  xenStoreDomainGetPCIID()

diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
index d0142d1e520d409577b6773eb3c72bf1039fecf8..827aac4b769b7ab006cbacc7fc4798d2b34d48d0 100644 (file)
--- a/src/xen/xend_internal.c
+++ b/src/xen/xend_internal.c
@@ -88,7 +88,8 @@ xenDaemonFormatSxprNet(virConnectPtr conn ATTRIBUTE_UNUSED,
 static int
 xenDaemonFormatSxprOnePCI(virConnectPtr conn,
                           virDomainHostdevDefPtr def,
-                          virBufferPtr buf);
+                          virBufferPtr buf,
+                          int detach);
 
 static int
 virDomainXMLDevID(virDomainPtr domain,
@@ -4165,7 +4166,7 @@ xenDaemonAttachDevice(virDomainPtr domain, const char *xml)
             dev->data.hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
             if (xenDaemonFormatSxprOnePCI(domain->conn,
                                           dev->data.hostdev,
-                                          &buf) < 0)
+                                          &buf, 0) < 0)
                 goto cleanup;
         } else {
             virXendError(domain->conn, VIR_ERR_NO_SUPPORT, "%s",
@@ -4217,6 +4218,8 @@ xenDaemonDetachDevice(virDomainPtr domain, const char *xml)
     virDomainDeviceDefPtr dev = NULL;
     virDomainDefPtr def = NULL;
     int ret = -1;
+    char *xendev = NULL;
+    virBuffer buf = VIR_BUFFER_INITIALIZER;
 
     if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL)) {
         virXendError((domain ? domain->conn : NULL), VIR_ERR_INVALID_ARG,
@@ -4247,8 +4250,28 @@ xenDaemonDetachDevice(virDomainPtr domain, const char *xml)
     if (virDomainXMLDevID(domain, dev, class, ref, sizeof(ref)))
         goto cleanup;
 
-    ret = xend_op(domain->conn, domain->name, "op", "device_destroy",
-                  "type", class, "dev", ref, "force", "0", "rm_cfg", "1", NULL);
+    if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) {
+        if (dev->data.hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
+            dev->data.hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
+            if (xenDaemonFormatSxprOnePCI(domain->conn,
+                                          dev->data.hostdev,
+                                          &buf, 1) < 0)
+                goto cleanup;
+        } else {
+            virXendError(domain->conn, VIR_ERR_NO_SUPPORT, "%s",
+                         _("unsupported device type"));
+            goto cleanup;
+        }
+        xendev = virBufferContentAndReset(&buf);
+        ret = xend_op(domain->conn, domain->name, "op", "device_configure",
+                      "config", xendev, "dev", ref, NULL);
+        VIR_FREE(xendev);
+    }
+    else {
+        ret = xend_op(domain->conn, domain->name, "op", "device_destroy",
+                      "type", class, "dev", ref, "force", "0", "rm_cfg", "1",
+                      NULL);
+    }
 
 cleanup:
     virDomainDefFree(def);
@@ -5567,7 +5590,8 @@ xenDaemonFormatSxprPCI(virDomainHostdevDefPtr def,
 static int
 xenDaemonFormatSxprOnePCI(virConnectPtr conn,
                           virDomainHostdevDefPtr def,
-                          virBufferPtr buf)
+                          virBufferPtr buf,
+                          int detach)
 {
     if (def->managed) {
         virXendError(conn, VIR_ERR_NO_SUPPORT, "%s",
@@ -5577,6 +5601,10 @@ xenDaemonFormatSxprOnePCI(virConnectPtr conn,
 
     virBufferAddLit(buf, "(pci ");
     xenDaemonFormatSxprPCI(def, buf);
+    if (detach)
+        virBufferAddLit(buf, "(state 'Closing')");
+    else
+        virBufferAddLit(buf, "(state 'Initialising')");
     virBufferAddLit(buf, ")");
 
     return 0;
@@ -5608,9 +5636,8 @@ xenDaemonFormatSxprAllPCI(virConnectPtr conn,
      *    )
      * )
      *
-     * Normally there is one (device ...) block per device, but in
-     * wierd world of Xen PCI, once (device ...) covers multiple
-     * devices.
+     * Normally there is one (device ...) block per device, but in the
+     * weird world of Xen PCI, one (device ...) covers multiple devices.
      */
 
     virBufferAddLit(buf, "(device (pci ");
@@ -5949,6 +5976,8 @@ error:
  *  - if disk, copy in ref the target name from description
  *  - if network, get MAC address from description, scan XenStore and
  *    copy in ref the corresponding vif number.
+ *  - if pci, get BDF from description, scan XenStore and
+ *    copy in ref the corresponding dev number.
  *
  * Returns 0 in case of success, -1 in case of failure.
  */
@@ -6006,6 +6035,31 @@ virDomainXMLDevID(virDomainPtr domain,
     } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV &&
                dev->data.hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
                dev->data.hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
+        char *bdf;
+        virDomainHostdevDefPtr def = dev->data.hostdev;
+
+        if (virAsprintf(&bdf, "%04x:%02x:%02x.%0x",
+                        def->source.subsys.u.pci.domain,
+                        def->source.subsys.u.pci.bus,
+                        def->source.subsys.u.pci.slot,
+                        def->source.subsys.u.pci.function) < 0) {
+            virReportOOMError(NULL);
+            return -1;
+        }
+
+        strcpy(class, "pci");
+
+        xenUnifiedLock(priv);
+        xref = xenStoreDomainGetPCIID(domain->conn, domain->id, bdf);
+        xenUnifiedUnlock(priv);
+        VIR_FREE(bdf);
+        if (xref == NULL)
+            return -1;
+
+        tmp = virStrcpy(ref, xref, ref_len);
+        VIR_FREE(xref);
+        if (tmp == NULL)
+            return -1;
     } else {
         virXendError(NULL, VIR_ERR_NO_SUPPORT,
                      "%s", _("hotplug of device type not supported"));

diff --git a/src/xen/xs_internal.c b/src/xen/xs_internal.c
index 10b1f8e614554cb3600d53c670a03d9e22c8838a..8a64d4e77b12593531b2ecada5e44c360583b67e 100644 (file)
--- a/src/xen/xs_internal.c
+++ b/src/xen/xs_internal.c
@@ -1055,6 +1055,61 @@ xenStoreDomainGetDiskID(virConnectPtr conn, int id, const char *dev) {
     return (NULL);
 }
 
+/*
+ * xenStoreDomainGetPCIID:
+ * @conn: pointer to the connection.
+ * @id: the domain id
+ * @bdf: the PCI BDF
+ *
+ * Get the reference (i.e. the string number) for the device on that domain
+ * which uses the given PCI address
+ *
+ * The caller must hold the lock on the privateData
+ * associated with the 'conn' parameter.
+ *
+ * Returns the new string or NULL in case of error, the string must be
+ *         freed by the caller.
+ */
+char *
+xenStoreDomainGetPCIID(virConnectPtr conn, int id, const char *bdf)
+{
+    char dir[80], path[128], **list = NULL, *val = NULL;
+    unsigned int len, i, num;
+    char *ret = NULL;
+    xenUnifiedPrivatePtr priv;
+
+    if (id < 0)
+        return(NULL);
+
+    priv = (xenUnifiedPrivatePtr) conn->privateData;
+    if (priv->xshandle == NULL)
+        return (NULL);
+    if (bdf == NULL)
+        return (NULL);
+
+    snprintf(dir, sizeof(dir), "/local/domain/0/backend/pci/%d", id);
+    list = xs_directory(priv->xshandle, 0, dir, &num);
+    if (list == NULL)
+        return(NULL);
+    for (i = 0; i < num; i++) {
+        snprintf(path, sizeof(path), "%s/%s/%s", dir, list[i], "dev-0");
+        if ((val = xs_read(priv->xshandle, 0, path, &len)) == NULL)
+            break;
+
+        bool match = STREQ(val, bdf);
+
+        VIR_FREE(val);
+
+        if (match) {
+            ret = strdup(list[i]);
+            break;
+        }
+    }
+
+    VIR_FREE(list);
+    return(ret);
+}
+
 /*
  * The caller must hold the lock on the privateData
  * associated with the 'conn' parameter.

diff --git a/src/xen/xs_internal.h b/src/xen/xs_internal.h
index 29e680fb07bb46ac2c3612aaa18394b019423001..6e0f40dc1d23a4a18728afa420abf5a970b14cfe 100644 (file)
--- a/src/xen/xs_internal.h
+++ b/src/xen/xs_internal.h
@@ -50,6 +50,9 @@ char *                xenStoreDomainGetNetworkID(virConnectPtr conn,
 char *         xenStoreDomainGetDiskID(virConnectPtr conn,
                                          int id,
                                          const char *dev);
+char *         xenStoreDomainGetPCIID(virConnectPtr conn,
+                                   int domid,
+                                   const char *bdf);
 char *          xenStoreDomainGetName(virConnectPtr conn,
                                       int id);
 int             xenStoreDomainGetUUID(virConnectPtr conn,