[PATCH 0 of 4] Add commands to automatically prep devices for pass-through

[PATCH 0 of 4] Add commands to automatically prep devices for pass-through

[George Dunlap]

The current method for passing through devices requires users to
either modify cryptic Linux boot parameters and reboot, or do a lot of
manual reads and writes into sysfs nodes.

This set of patches introduces commands to make this easier.  It expands
on the concept of "assignable" (from the list_assignable_devices command).

The new xl commands are:

pci_assignable_add: Make a device assignable to guests.  This involves
unbinding the device from its old driver, creating a slot for it in
pciback (if necessary), and binding it to pciback.

pci_assignable_list: List devices assignable to guests.  Just renamed
from pci_list_assignable.

pci_assignable_remove: Make the device no longer assignable to guests. 
This involves unbinding the device from pciback and removing the slot.  It 
optionally involves rebinding the device to the driver from which we stole
it.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

[PATCH 1 of 4] libxl: Make a helper function write a BDF to a sysfs path

[George Dunlap]

This functionality will be used several times in subsequent patches.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r d5268710a5ca -r 0772f1d07d1c tools/libxl/libxl_pci.c
--- a/tools/libxl/libxl_pci.c	Fri May 04 10:46:23 2012 +0100
+++ b/tools/libxl/libxl_pci.c	Tue May 08 17:18:31 2012 +0100
@@ -327,6 +327,36 @@ static int is_pcidev_in_array(libxl_devi
     return 0;
 }
 
+/* Write the standard BDF into the sysfs path given by sysfs_path. */
+static int sysfs_write_bdf(libxl__gc *gc, const char * sysfs_path,
+                           libxl_device_pci *pcidev)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    int rc, fd;
+    char *buf;
+
+    fd = open(sysfs_path, O_WRONLY);
+    if (fd < 0) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn't open %s",
+                         sysfs_path);
+        return ERROR_FAIL;
+    }
+        
+    buf = libxl__sprintf(gc, PCI_BDF, pcidev->domain, pcidev->bus,
+                         pcidev->dev, pcidev->func);
+    rc = write(fd, buf, strlen(buf));
+    /* Annoying to have two if's, but we need the errno */
+    if (rc < 0)
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                         "write to %s returned %d", sysfs_path, rc);
+    close(fd);
+
+    if (rc < 0)
+        return ERROR_FAIL;
+
+    return 0;
+}
+
 libxl_device_pci *libxl_device_pci_list_assignable(libxl_ctx *ctx, int *num)
 {
     GC_INIT(ctx);
@@ -571,27 +601,12 @@ static int do_pci_add(libxl__gc *gc, uin
 
         /* Don't restrict writes to the PCI config space from this VM */
         if (pcidev->permissive) {
-            int fd;
-            char *buf;
-            
-            sysfs_path = libxl__sprintf(gc, SYSFS_PCIBACK_DRIVER"/permissive");
-            fd = open(sysfs_path, O_WRONLY);
-            if (fd < 0) {
-                LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn't open %s",
-                                 sysfs_path);
+            if ( sysfs_write_bdf(gc, SYSFS_PCIBACK_DRIVER"/permissive",
+                                 pcidev) < 0 ) {
+                LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                           "Setting permissive for device");
                 return ERROR_FAIL;
             }
- 
-            buf = libxl__sprintf(gc, PCI_BDF, pcidev->domain, pcidev->bus,
-                                 pcidev->dev, pcidev->func);
-            rc = write(fd, buf, strlen(buf));
-            /* Annoying to have two if's, but we need the errno */
-            if (rc < 0)
-                LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
-                                 "write to %s returned %d", sysfs_path, rc);
-            close(fd);
-            if (rc < 0)
-                return ERROR_FAIL;
         }
         break;
     }

[PATCH 2 of 4] libxl: Rename pci_list_assignable to pci_assignable_list

[George Dunlap]

...to prepare for a consistent "pci_assignable_*" naming scheme.

Also move the man page entry into the PCI PASS-THROUGH section, rather
than the XEN HOST section.

No functional changes.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r 0772f1d07d1c -r 5b5070d487d9 docs/man/xl.pod.1
--- a/docs/man/xl.pod.1	Tue May 08 17:18:31 2012 +0100
+++ b/docs/man/xl.pod.1	Wed May 09 11:21:19 2012 +0100
@@ -687,13 +687,6 @@ explanatory.
 
 Prints the current uptime of the domains running.
 
-=item B<pci-list-assignable-devices>
-
-List all the assignable PCI devices.
-These are devices in the system which are configured to be
-available for passthrough and are bound to a suitable PCI
-backend driver in domain 0 rather than a real driver.
-
 =back
 
 =head1 SCHEDULER SUBCOMMANDS
@@ -1026,6 +1019,13 @@ List virtual network interfaces for a do
 
 =over 4
 
+=item B<pci-assignable-list>
+
+List all the assignable PCI devices.
+These are devices in the system which are configured to be
+available for passthrough and are bound to a suitable PCI
+backend driver in domain 0 rather than a real driver.
+
 =item B<pci-attach> I<domain-id> I<BDF>
 
 Hot-plug a new pass-through pci device to the specified domain.
diff -r 0772f1d07d1c -r 5b5070d487d9 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Tue May 08 17:18:31 2012 +0100
+++ b/tools/libxl/libxl.h	Wed May 09 11:21:19 2012 +0100
@@ -662,7 +662,7 @@ libxl_device_pci *libxl_device_pci_list(
  * could be assigned to a domain (i.e. are bound to the backend
  * driver) but are not currently.
  */
-libxl_device_pci *libxl_device_pci_list_assignable(libxl_ctx *ctx, int *num);
+libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num);
 
 /* CPUID handling */
 int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str);
diff -r 0772f1d07d1c -r 5b5070d487d9 tools/libxl/libxl_pci.c
--- a/tools/libxl/libxl_pci.c	Tue May 08 17:18:31 2012 +0100
+++ b/tools/libxl/libxl_pci.c	Wed May 09 11:21:19 2012 +0100
@@ -357,7 +357,7 @@ static int sysfs_write_bdf(libxl__gc *gc
     return 0;
 }
 
-libxl_device_pci *libxl_device_pci_list_assignable(libxl_ctx *ctx, int *num)
+libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num)
 {
     GC_INIT(ctx);
     libxl_device_pci *pcidevs = NULL, *new, *assigned;
@@ -684,7 +684,7 @@ static int libxl_pcidev_assignable(libxl
     libxl_device_pci *pcidevs;
     int num, i;
 
-    pcidevs = libxl_device_pci_list_assignable(ctx, &num);
+    pcidevs = libxl_device_pci_assignable_list(ctx, &num);
     for (i = 0; i < num; i++) {
         if (pcidevs[i].domain == pcidev->domain &&
             pcidevs[i].bus == pcidev->bus &&
diff -r 0772f1d07d1c -r 5b5070d487d9 tools/libxl/xl.h
--- a/tools/libxl/xl.h	Tue May 08 17:18:31 2012 +0100
+++ b/tools/libxl/xl.h	Wed May 09 11:21:19 2012 +0100
@@ -34,9 +34,9 @@ int main_cd_insert(int argc, char **argv
 int main_console(int argc, char **argv);
 int main_vncviewer(int argc, char **argv);
 int main_pcilist(int argc, char **argv);
-int main_pcilist_assignable(int argc, char **argv);
 int main_pcidetach(int argc, char **argv);
 int main_pciattach(int argc, char **argv);
+int main_pciassignable_list(int argc, char **argv);
 int main_restore(int argc, char **argv);
 int main_migrate_receive(int argc, char **argv);
 int main_save(int argc, char **argv);
diff -r 0772f1d07d1c -r 5b5070d487d9 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Tue May 08 17:18:31 2012 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Wed May 09 11:21:19 2012 +0100
@@ -2223,34 +2223,6 @@ int main_vncviewer(int argc, char **argv
     return 0;
 }
 
-static void pcilist_assignable(void)
-{
-    libxl_device_pci *pcidevs;
-    int num, i;
-
-    pcidevs = libxl_device_pci_list_assignable(ctx, &num);
-
-    if ( pcidevs == NULL )
-        return;
-    for (i = 0; i < num; i++) {
-        printf("%04x:%02x:%02x.%01x\n",
-               pcidevs[i].domain, pcidevs[i].bus, pcidevs[i].dev, pcidevs[i].func);
-        libxl_device_pci_dispose(&pcidevs[i]);
-    }
-    free(pcidevs);
-}
-
-int main_pcilist_assignable(int argc, char **argv)
-{
-    int opt;
-
-    if ((opt = def_getopt(argc, argv, "", "pci-list-assignable-devices", 0)) != -1)
-        return opt;
-
-    pcilist_assignable();
-    return 0;
-}
-
 static void pcilist(const char *dom)
 {
     libxl_device_pci *pcidevs;
@@ -2368,6 +2340,34 @@ int main_pciattach(int argc, char **argv
     return 0;
 }
 
+static void pciassignable_list(void)
+{
+    libxl_device_pci *pcidevs;
+    int num, i;
+
+    pcidevs = libxl_device_pci_assignable_list(ctx, &num);
+
+    if ( pcidevs == NULL )
+        return;
+    for (i = 0; i < num; i++) {
+        printf("%04x:%02x:%02x.%01x\n",
+               pcidevs[i].domain, pcidevs[i].bus, pcidevs[i].dev, pcidevs[i].func);
+        libxl_device_pci_dispose(&pcidevs[i]);
+    }
+    free(pcidevs);
+}
+
+int main_pciassignable_list(int argc, char **argv)
+{
+    int opt;
+
+    if ((opt = def_getopt(argc, argv, "", "pci-assignable-list", 0)) != -1)
+        return opt;
+
+    pciassignable_list();
+    return 0;
+}
+
 static void pause_domain(const char *p)
 {
     find_domain(p);
diff -r 0772f1d07d1c -r 5b5070d487d9 tools/libxl/xl_cmdtable.c
--- a/tools/libxl/xl_cmdtable.c	Tue May 08 17:18:31 2012 +0100
+++ b/tools/libxl/xl_cmdtable.c	Wed May 09 11:21:19 2012 +0100
@@ -86,8 +86,8 @@ struct cmd_spec cmd_table[] = {
       "List pass-through pci devices for a domain",
       "<Domain>",
     },
-    { "pci-list-assignable-devices",
-      &main_pcilist_assignable, 0,
+    { "pci-assignable-list",
+      &main_pciassignable_list, 0,
       "List all the assignable pci devices",
       "",
     },
diff -r 0772f1d07d1c -r 5b5070d487d9 tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c	Tue May 08 17:18:31 2012 +0100
+++ b/tools/python/xen/lowlevel/xl/xl.c	Wed May 09 11:21:19 2012 +0100
@@ -566,13 +566,13 @@ static PyObject *pyxl_pci_parse(XlObject
     return (PyObject *)pci;
 }
 
-static PyObject *pyxl_pci_list_assignable(XlObject *self, PyObject *args)
+static PyObject *pyxl_pci_assignable_list(XlObject *self, PyObject *args)
 {
     libxl_device_pci *dev;
     PyObject *list;
     int nr_dev, i;
 
-    dev = libxl_device_pci_list_assignable(self->ctx, &nr_dev);
+    dev = libxl_device_pci_assignable_list(self->ctx, &nr_dev);
     if ( dev == NULL ) {
         PyErr_SetString(xl_error_obj, "Cannot list assignable devices");
         return NULL;
@@ -662,8 +662,8 @@ static PyMethodDef pyxl_methods[] = {
          "Parse pass-through PCI device spec (BDF)"},
     {"device_pci_list", (PyCFunction)pyxl_pci_list, METH_VARARGS,
         "List PCI devices assigned to a domain"},
-    {"device_pci_list_assignable",
-        (PyCFunction)pyxl_pci_list_assignable, METH_NOARGS,
+    {"device_pci_assignable_list",
+        (PyCFunction)pyxl_pci_assignable_list, METH_NOARGS,
         "List assignable PCI devices"},
     { NULL, NULL, 0, NULL }
 };

[PATCH 3 of 4] libxl: Introduce pci_assignable_add and pci_assignable_remove

[George Dunlap]

Introduce libxl helper functions to prepare devices to be passed through to
guests.  This is meant to replace of all the manual sysfs commands which
are currently required.

pci_assignable_add accepts a BDF for a device and will:
* Unbind a device from its current driver, if any
* If "rebind" is set, it will store the path of the driver from which we
  unplugged it in /libxl/pciback/$BDF/driver_path
* If necessary, create a slot for it in pciback
* Bind the device to pciback

At this point it will show up in pci_assignable_list, and is ready to be passed
through to a guest.

pci_assignable_remove accepts a BDF for a device and will:
* Unbind the device from pciback
* Remove the slot from pciback
* If "rebind" is set, and /libx/pciback/$BDF/driver_path exists, it will
  attempt to rebind the device to its original driver.

NB that "$BDF" in this case uses dashes instead of : and ., because : and . are
illegal characters in xenstore paths.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r 5b5070d487d9 -r 17a5b562d1e7 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Wed May 09 11:21:19 2012 +0100
+++ b/tools/libxl/libxl.h	Wed May 09 11:21:28 2012 +0100
@@ -658,10 +658,25 @@ int libxl_device_pci_destroy(libxl_ctx *
 libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid, int *num);
 
 /*
- * Similar to libxl_device_pci_list but returns all devices which
- * could be assigned to a domain (i.e. are bound to the backend
- * driver) but are not currently.
+ * Functions related to making devices assignable -- that is, bound to
+ * the pciback driver, ready to be given to a guest via
+ * libxl_pci_device_add.
+ *
+ * - ..._add() will unbind the device from its current driver (if
+ * already bound) and re-bind it to pciback; at that point it will be
+ * ready to be assigned to a VM.  If rebind is set, it will store the
+ * path to the old driver in xenstore so that it can be handed back to
+ * dom0 on restore.
+ *
+ * - ..._remove() will unbind the device from pciback, and if
+ * rebind is non-zero, attempt to assign it back to the driver
+ * from whence it came.
+ *
+ * - ..._list() will return a list of the PCI devices available to be
+ * assigned.
  */
+int libxl_device_pci_assignable_add(libxl_ctx *ctx, libxl_device_pci *pcidev, int rebind);
+int libxl_device_pci_assignable_remove(libxl_ctx *ctx, libxl_device_pci *pcidev, int rebind);
 libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num);
 
 /* CPUID handling */
diff -r 5b5070d487d9 -r 17a5b562d1e7 tools/libxl/libxl_pci.c
--- a/tools/libxl/libxl_pci.c	Wed May 09 11:21:19 2012 +0100
+++ b/tools/libxl/libxl_pci.c	Wed May 09 11:21:28 2012 +0100
@@ -21,6 +21,7 @@
 #define PCI_BDF                "%04x:%02x:%02x.%01x"
 #define PCI_BDF_SHORT          "%02x:%02x.%01x"
 #define PCI_BDF_VDEVFN         "%04x:%02x:%02x.%01x@%02x"
+#define PCI_BDF_XSPATH         "%04x-%02x-%02x-%01x"
 
 static unsigned int pcidev_encode_bdf(libxl_device_pci *pcidev)
 {
@@ -408,6 +409,347 @@ out:
     return pcidevs;
 }
 
+/* Unbind device from its current driver, if any.  If driver_path is non-NULL,
+ * store the path to the original driver in it. */
+static int sysfs_dev_unbind(libxl__gc *gc, libxl_device_pci *pcidev, char **driver_path)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    char * spath, *_dp, **dp;
+    struct stat st;
+
+    if ( driver_path )
+        dp = driver_path;
+    else
+        dp = &_dp;
+
+    spath = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/driver",
+                           pcidev->domain,
+                           pcidev->bus,
+                           pcidev->dev,
+                           pcidev->func);
+    if ( !lstat(spath, &st) ) {
+        /* Find the canonical path to the driver. */
+        *dp = libxl__zalloc(gc, PATH_MAX);
+        *dp = realpath(spath, *dp);
+        if ( !*dp ) {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "realpath() failed");
+            return -1;
+        }
+
+        LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Driver re-plug path: %s",
+                   *dp);
+        
+        /* Unbind from the old driver */
+        spath = libxl__sprintf(gc, "%s/unbind", *dp);
+        if ( sysfs_write_bdf(gc, spath, pcidev) < 0 ) {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Couldn't unbind device");
+            return -1;
+        }
+    }
+    return 0;
+}
+
+/* Scan through /sys/.../pciback/slots looking for pcidev's BDF */
+static int pciback_dev_has_slot(libxl__gc *gc, libxl_device_pci *pcidev)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    FILE *f;
+    int rc = 0;
+    unsigned bus, dev, func;
+    
+    f = fopen(SYSFS_PCIBACK_DRIVER"/slots", "r");
+
+    if (f == NULL) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn't open %s",
+                         SYSFS_PCIBACK_DRIVER"/slots");
+        return ERROR_FAIL;
+    }
+
+    while(fscanf(f, "0000:%x:%x.%x\n", &bus, &dev, &func)==3) {
+        if(bus == pcidev->bus
+           && dev == pcidev->dev
+           && func == pcidev->func) {
+            rc = 1;
+            goto out;
+        }
+    }
+out:
+    fclose(f);
+    return rc;
+}
+
+static int pciback_dev_is_assigned(libxl__gc *gc, libxl_device_pci *pcidev)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    char * spath;
+    int rc;
+    struct stat st;
+
+    spath = libxl__sprintf(gc, SYSFS_PCIBACK_DRIVER"/"PCI_BDF,
+                           pcidev->domain, pcidev->bus,
+                           pcidev->dev, pcidev->func);
+    rc = lstat(spath, &st);
+
+    if( rc == 0 )
+        return 1;
+    if ( rc < 0 && errno == ENOENT )
+        return 0;
+    LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Accessing %s", spath);
+    return -1;
+}
+
+static int pciback_dev_assign(libxl__gc *gc, libxl_device_pci *pcidev)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    int rc;
+
+    if ( (rc=pciback_dev_has_slot(gc, pcidev)) < 0 ) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                   "Error checking for pciback slot");
+        return ERROR_FAIL;
+    } else if (rc == 0) {
+        if ( sysfs_write_bdf(gc, SYSFS_PCIBACK_DRIVER"/new_slot",
+                             pcidev) < 0 ) {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                       "Couldn't bind device to pciback!");
+            return ERROR_FAIL;
+        }
+    }
+    
+    if ( sysfs_write_bdf(gc, SYSFS_PCIBACK_DRIVER"/bind", pcidev) < 0 ) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Couldn't bind device to pciback!");
+        return ERROR_FAIL;
+    }
+    return 0;
+}
+
+static int pciback_dev_unassign(libxl__gc *gc, libxl_device_pci *pcidev)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+
+    /* Remove from pciback */
+    if ( sysfs_dev_unbind(gc, pcidev, NULL) < 0 ) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Couldn't unbind device!");
+        return ERROR_FAIL;
+    }
+
+    /* Remove slot if necessary */
+    if ( pciback_dev_has_slot(gc, pcidev) > 0 ) {
+        if ( sysfs_write_bdf(gc, SYSFS_PCIBACK_DRIVER"/remove_slot",
+                             pcidev) < 0 ) {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                       "Couldn't remove pciback slot");
+            return ERROR_FAIL;
+        }
+    }
+    return 0;
+}
+
+/* FIXME */
+#define PCIBACK_INFO_PATH "/libxl/pciback"
+
+static void pci_assignable_driver_path_write(libxl__gc *gc,
+                                            libxl_device_pci *pcidev,
+                                            char *driver_path)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    char *path;
+    xs_transaction_t t = 0;
+    struct xs_permissions perm[1];
+
+    perm[0].id = 0;
+    perm[0].perms = XS_PERM_NONE;
+
+retry:
+    t = xs_transaction_start(ctx->xsh);
+
+    path = libxl__sprintf(gc, PCIBACK_INFO_PATH"/"PCI_BDF_XSPATH"/driver_path",
+                          pcidev->domain,
+                          pcidev->bus,
+                          pcidev->dev,
+                          pcidev->func);
+    xs_rm(ctx->xsh, t, path);
+    if ( libxl__xs_write(gc, XBT_NULL, path, "%s", driver_path) < 0 ) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                         "Write of %s to node %s failed",
+                         driver_path, path);
+    }
+
+    if (!xs_transaction_end(ctx->xsh, t, 0)) {
+        if (errno == EAGAIN) {
+            t = 0;
+            goto retry;
+        }
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                         "xenstore transaction commit failed; device "
+                         " will not be rebound to original driver.");
+    }
+}
+
+static char * pci_assignable_driver_path_read(libxl__gc *gc,
+                                              libxl_device_pci *pcidev)
+{
+    return libxl__xs_read(gc, XBT_NULL, 
+                          libxl__sprintf(gc,
+                           PCIBACK_INFO_PATH "/" PCI_BDF_XSPATH "/driver_path",
+                           pcidev->domain,
+                           pcidev->bus,
+                           pcidev->dev,
+                           pcidev->func));
+}
+
+static void pci_assignable_driver_path_remove(libxl__gc *gc,
+                                              libxl_device_pci *pcidev)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    char * path;
+    xs_transaction_t t;
+
+    /* Remove the xenstore entry */
+retry:
+    t = xs_transaction_start(ctx->xsh);
+    path = libxl__sprintf(gc, PCIBACK_INFO_PATH "/" PCI_BDF_XSPATH,
+                          pcidev->domain,
+                          pcidev->bus,
+                          pcidev->dev,
+                          pcidev->func);
+    xs_rm(ctx->xsh, t, path);
+    if (!xs_transaction_end(ctx->xsh, t, 0)) {
+        if (errno == EAGAIN)
+            goto retry;
+        else
+            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                             "Failed to remove xenstore entry");
+    }
+}
+
+static int libxl__device_pci_assignable_add(libxl__gc *gc,
+                                            libxl_device_pci *pcidev,
+                                            int rebind)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    unsigned dom, bus, dev, func;
+    char *spath, *driver_path = NULL;
+    struct stat st;
+
+    /* Local copy for convenience */
+    dom = pcidev->domain;
+    bus = pcidev->bus;
+    dev = pcidev->dev;
+    func = pcidev->func;
+
+    /* See if the device exists */
+    spath = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF, dom, bus, dev, func);
+    if ( lstat(spath, &st) ) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn't lstat %s", spath);
+        return ERROR_FAIL;
+    }
+
+    /* Check to see if it's already assigned to pciback */
+    if ( pciback_dev_is_assigned(gc, pcidev) ) {
+        LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, PCI_BDF" already assigned to pciback",
+                   dom, bus, dev, func);
+        return 0;
+    }
+
+    /* Check to see if there's already a driver that we need to unbind from */
+    if ( sysfs_dev_unbind(gc, pcidev, &driver_path ) ) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Couldn't unbind "PCI_BDF" from driver",
+                   dom, bus, dev, func);
+        return ERROR_FAIL;
+    }
+
+    /* Store driver_path for rebinding to dom0 */
+    if ( rebind ) {
+        if ( driver_path ) {
+            pci_assignable_driver_path_write(gc, pcidev, driver_path);
+        } else {
+            LIBXL__LOG(ctx, LIBXL__LOG_WARNING,
+                       PCI_BDF" not bound to a driver, will not be rebound.",
+                       dom, bus, dev, func);
+        }
+    }
+
+    if ( pciback_dev_assign(gc, pcidev) ) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Couldn't bind device to pciback!");
+        return ERROR_FAIL;
+    }
+
+    return 0;
+}
+
+static int libxl__device_pci_assignable_remove(libxl__gc *gc,
+                                               libxl_device_pci *pcidev,
+                                               int rebind)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    int rc;
+    char *driver_path;
+
+    /* Unbind from pciback */
+    if ( (rc=pciback_dev_is_assigned(gc, pcidev)) < 0 ) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Checking if pciback was assigned");
+        return ERROR_FAIL;
+    } else if ( rc ) {
+        pciback_dev_unassign(gc, pcidev);
+    } else {
+        LIBXL__LOG(ctx, LIBXL__LOG_WARNING,
+                   "Not bound to pciback");
+    }
+
+    /* Rebind if necessary */
+    driver_path = pci_assignable_driver_path_read(gc, pcidev);
+
+    if ( driver_path ) {
+        if ( rebind ) {
+            LIBXL__LOG(ctx, LIBXL__LOG_INFO, "Rebinding to driver at %s",
+                       driver_path);
+
+            if ( sysfs_write_bdf(gc,
+                                 libxl__sprintf(gc, "%s/bind", driver_path),
+                                 pcidev) < 0 ) {
+                LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                           "Couldn't bind device to %s", driver_path);
+                return -1;
+            }
+        }
+
+        pci_assignable_driver_path_remove(gc, pcidev);
+    } else {
+        if ( rebind ) {
+            LIBXL__LOG(ctx, LIBXL__LOG_WARNING,
+                       "Couldn't find path for original driver; not rebinding");
+        }
+    }
+
+    return 0;
+}
+
+int libxl_device_pci_assignable_add(libxl_ctx *ctx, libxl_device_pci *pcidev,
+                                    int rebind)
+{
+    GC_INIT(ctx);
+    int rc;
+
+    rc = libxl__device_pci_assignable_add(gc, pcidev, rebind);
+
+    GC_FREE;
+    return rc;
+}
+
+
+int libxl_device_pci_assignable_remove(libxl_ctx *ctx, libxl_device_pci *pcidev,
+                                       int rebind)
+{
+    GC_INIT(ctx);
+    int rc;
+
+    rc = libxl__device_pci_assignable_remove(gc, pcidev, rebind);
+
+    GC_FREE;
+    return rc;
+}
+
 /*
  * This function checks that all functions of a device are bound to pciback
  * driver. It also initialises a bit-mask of which function numbers are present

[PATCH 4 of 4] xl: Add pci_assignable_add and remove commands

[George Dunlap]

pci-assignable-add will always store the driver rebind path, but
pci-assignable-remove will only actually rebind if asked to do so.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r 17a5b562d1e7 -r a1c357853735 docs/man/xl.pod.1
--- a/docs/man/xl.pod.1	Wed May 09 11:21:28 2012 +0100
+++ b/docs/man/xl.pod.1	Wed May 09 11:24:01 2012 +0100
@@ -1026,6 +1026,26 @@ These are devices in the system which ar
 available for passthrough and are bound to a suitable PCI
 backend driver in domain 0 rather than a real driver.
 
+=item B<pci-assignable-add> I<BDF>
+
+Make the device at PCI Bus/Device/Function BDF assignable to guests.  This
+will bind the device to the pciback driver.  If it is already bound to a
+driver, it will first be unbound, and the original driver stored so that it
+can be re-bound to the same driver later if desired.  
+
+CAUTION: This will make the device unusable by Domain 0 until it is
+returned with pci-assignable-remove.  Care should therefore be taken
+not to do this on a device critical to domain 0's operation, such as
+storage controllers, network interfaces, or GPUs that are currently
+being used.
+
+=item B<pci-assignable-remove> [I<-r>] I<BDF>
+
+Make the device at PCI Bus/Device/Function BDF assignable to guests.  This
+will at least unbind the device from pciback.  If the -r option is specified,
+it will also attempt to re-bind the device to its original driver, making it
+usable by Domain 0 again.
+
 =item B<pci-attach> I<domain-id> I<BDF>
 
 Hot-plug a new pass-through pci device to the specified domain.
diff -r 17a5b562d1e7 -r a1c357853735 tools/libxl/xl.h
--- a/tools/libxl/xl.h	Wed May 09 11:21:28 2012 +0100
+++ b/tools/libxl/xl.h	Wed May 09 11:24:01 2012 +0100
@@ -36,6 +36,8 @@ int main_vncviewer(int argc, char **argv
 int main_pcilist(int argc, char **argv);
 int main_pcidetach(int argc, char **argv);
 int main_pciattach(int argc, char **argv);
+int main_pciassignable_add(int argc, char **argv);
+int main_pciassignable_remove(int argc, char **argv);
 int main_pciassignable_list(int argc, char **argv);
 int main_restore(int argc, char **argv);
 int main_migrate_receive(int argc, char **argv);
diff -r 17a5b562d1e7 -r a1c357853735 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Wed May 09 11:21:28 2012 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Wed May 09 11:24:01 2012 +0100
@@ -2368,6 +2368,82 @@ int main_pciassignable_list(int argc, ch
     return 0;
 }
 
+static void pciassignable_add(const char *bdf, int rebind)
+{
+    libxl_device_pci pcidev;
+    XLU_Config *config;
+
+    memset(&pcidev, 0x00, sizeof(pcidev));
+
+    config = xlu_cfg_init(stderr, "command line");
+    if (!config) { perror("xlu_cfg_inig"); exit(-1); }
+
+    if (xlu_pci_parse_bdf(config, &pcidev, bdf)) {
+        fprintf(stderr, "pci-assignable-add: malformed BDF specification \"%s\"\n", bdf);
+        exit(2);
+    }
+    libxl_device_pci_assignable_add(ctx, &pcidev, rebind);
+    libxl_device_pci_dispose(&pcidev);
+}
+
+int main_pciassignable_add(int argc, char **argv)
+{
+    int opt;
+    const char *bdf = NULL;
+
+    while ((opt = def_getopt(argc, argv, "", "pci-assignable-add", 1)) != -1) {
+        switch (opt) {
+        case 0: case 2:
+            return opt;
+        }
+    }
+
+    bdf = argv[optind];
+
+    pciassignable_add(bdf, 1);
+    return 0;
+}
+
+static void pciassignable_remove(const char *bdf, int rebind)
+{
+    libxl_device_pci pcidev;
+    XLU_Config *config;
+
+    memset(&pcidev, 0x00, sizeof(pcidev));
+
+    config = xlu_cfg_init(stderr, "command line");
+    if (!config) { perror("xlu_cfg_inig"); exit(-1); }
+
+    if (xlu_pci_parse_bdf(config, &pcidev, bdf)) {
+        fprintf(stderr, "pci-assignable-remove: malformed BDF specification \"%s\"\n", bdf);
+        exit(2);
+    }
+    libxl_device_pci_assignable_remove(ctx, &pcidev, rebind);
+    libxl_device_pci_dispose(&pcidev);
+}
+
+int main_pciassignable_remove(int argc, char **argv)
+{
+    int opt;
+    const char *bdf = NULL;
+    int rebind = 0;
+
+    while ((opt = def_getopt(argc, argv, "r", "pci-assignable-remove", 1)) != -1) {
+        switch (opt) {
+        case 0: case 2:
+            return opt;
+        case 'r':
+            rebind=1;
+            break;
+        }
+    }
+
+    bdf = argv[optind];
+
+    pciassignable_remove(bdf, rebind);
+    return 0;
+}
+
 static void pause_domain(const char *p)
 {
     find_domain(p);
diff -r 17a5b562d1e7 -r a1c357853735 tools/libxl/xl_cmdtable.c
--- a/tools/libxl/xl_cmdtable.c	Wed May 09 11:21:28 2012 +0100
+++ b/tools/libxl/xl_cmdtable.c	Wed May 09 11:24:01 2012 +0100
@@ -86,6 +86,20 @@ struct cmd_spec cmd_table[] = {
       "List pass-through pci devices for a domain",
       "<Domain>",
     },
+    { "pci-assignable-add",
+      &main_pciassignable_add, 0,
+      "Make a device assignable for pci-passthru",
+      "<BDF>",
+      "-h                      Print this help.\n"
+    },
+    { "pci-assignable-remove",
+      &main_pciassignable_remove, 0,
+      "Remove a device from being assignable",
+      "[options] <BDF>",
+      "-h                      Print this help.\n"
+      "-r                      Attempt to re-assign the device to the\n"
+      "                        original driver"
+    },
     { "pci-assignable-list",
       &main_pciassignable_list, 0,
       "List all the assignable pci devices",

Re: [PATCH 0 of 4] Add commands to automatically prep devices for pass-through

[Ian Campbell]

On Wed, 2012-05-09 at 11:28 +0100, George Dunlap wrote:
> The current method for passing through devices requires users to
> either modify cryptic Linux boot parameters and reboot, or do a lot of
> manual reads and writes into sysfs nodes.
> 
> This set of patches introduces commands to make this easier.

Is this intended for 4.2 or an RFC for 4.3?

>   It expands
> on the concept of "assignable" (from the list_assignable_devices command).
> 
> The new xl commands are:
> 
> pci_assignable_add: Make a device assignable to guests.  This involves
> unbinding the device from its old driver, creating a slot for it in
> pciback (if necessary), and binding it to pciback.
> 
> pci_assignable_list: List devices assignable to guests.  Just renamed
> from pci_list_assignable.
> 
> pci_assignable_remove: Make the device no longer assignable to guests. 
> This involves unbinding the device from pciback and removing the slot.  It 
> optionally involves rebinding the device to the driver from which we stole
> it.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 0 of 4] Add commands to automatically prep devices for pass-through

[David Vrabel]

On 09/05/12 11:28, George Dunlap wrote:
> The current method for passing through devices requires users to
> either modify cryptic Linux boot parameters and reboot, or do a lot of
> manual reads and writes into sysfs nodes.
> 
> This set of patches introduces commands to make this easier.  It expands
> on the concept of "assignable" (from the list_assignable_devices command).
> 
> The new xl commands are:
> 
> pci_assignable_add: Make a device assignable to guests.  This involves
> unbinding the device from its old driver, creating a slot for it in
> pciback (if necessary), and binding it to pciback.
> 
> pci_assignable_list: List devices assignable to guests.  Just renamed
> from pci_list_assignable.

You use underscores here (and elsewhere in the commit message) but the
code uses dashes.  Suggest updating the commit messages with the format
for the commands.

David

Re: [PATCH 0 of 4] Add commands to automatically prep devices for pass-through

[George Dunlap]

On 09/05/12 11:49, Ian Campbell wrote:
> On Wed, 2012-05-09 at 11:28 +0100, George Dunlap wrote:
>> The current method for passing through devices requires users to
>> either modify cryptic Linux boot parameters and reboot, or do a lot of
>> manual reads and writes into sysfs nodes.
>>
>> This set of patches introduces commands to make this easier.
> Is this intended for 4.2 or an RFC for 4.3?
Well, I would really like it to go in to 4.2 -- it's been on my to-do 
list for a month at least, and it will make doing pci pass-through, and 
thus driver domains, a lot more straightforward.

  -George


>
>>    It expands
>> on the concept of "assignable" (from the list_assignable_devices command).
>>
>> The new xl commands are:
>>
>> pci_assignable_add: Make a device assignable to guests.  This involves
>> unbinding the device from its old driver, creating a slot for it in
>> pciback (if necessary), and binding it to pciback.
>>
>> pci_assignable_list: List devices assignable to guests.  Just renamed
>> from pci_list_assignable.
>>
>> pci_assignable_remove: Make the device no longer assignable to guests.
>> This involves unbinding the device from pciback and removing the slot.  It
>> optionally involves rebinding the device to the driver from which we stole
>> it.
>>
>> Signed-off-by: George Dunlap<george.dunlap@eu.citrix.com>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>

Re: [PATCH 0 of 4] Add commands to automatically prep devices for pass-through

[George Dunlap]

On 09/05/12 11:56, David Vrabel wrote:
> pci_assignable_add: Make a device assignable to guests.  This involves
> unbinding the device from its old driver, creating a slot for it in
> pciback (if necessary), and binding it to pciback.
>
> pci_assignable_list: List devices assignable to guests.  Just renamed
> from pci_list_assignable.
> You use underscores here (and elsewhere in the commit message) but the
> code uses dashes.  Suggest updating the commit messages with the format
> for the commands.
I used underscores in the patch to libxl, because the libxl functions 
have undescores, and dashes in the xl patch, because the xl commands use 
dashes.  :-)

  -George

Re: [PATCH 0 of 4] Add commands to automatically prep devices for pass-through

[Ian Campbell]

On Wed, 2012-05-09 at 12:03 +0100, George Dunlap wrote:
> On 09/05/12 11:49, Ian Campbell wrote:
> > On Wed, 2012-05-09 at 11:28 +0100, George Dunlap wrote:
> >> The current method for passing through devices requires users to
> >> either modify cryptic Linux boot parameters and reboot, or do a lot of
> >> manual reads and writes into sysfs nodes.
> >>
> >> This set of patches introduces commands to make this easier.
> > Is this intended for 4.2 or an RFC for 4.3?
> Well, I would really like it to go in to 4.2 -- it's been on my to-do 
> list for a month at least, and it will make doing pci pass-through, and 
> thus driver domains, a lot more straightforward.

Right, however it is strictly speaking a new feature which is not
mentioned on the TODO list and has not previously been posted (AFAIK,
please correct me if not) and we are currently supposed to be in feature
freeze (and have been for several weeks, if not a month).

IIRC this functionality was mooted when the pci permissive patch was
being done as something which would be a 4.3 feature.

We need to decide if we want to make an exception for this new feature
or not. Although I'm sure this feature is very nice and handy, we've
lived without it for years and people seem to be able to use the
existing scheme.

Ian.

> 
>   -George
> 
> 
> >
> >>    It expands
> >> on the concept of "assignable" (from the list_assignable_devices command).
> >>
> >> The new xl commands are:
> >>
> >> pci_assignable_add: Make a device assignable to guests.  This involves
> >> unbinding the device from its old driver, creating a slot for it in
> >> pciback (if necessary), and binding it to pciback.
> >>
> >> pci_assignable_list: List devices assignable to guests.  Just renamed
> >> from pci_list_assignable.
> >>
> >> pci_assignable_remove: Make the device no longer assignable to guests.
> >> This involves unbinding the device from pciback and removing the slot.  It
> >> optionally involves rebinding the device to the driver from which we stole
> >> it.
> >>
> >> Signed-off-by: George Dunlap<george.dunlap@eu.citrix.com>
> >>
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@lists.xen.org
> >> http://lists.xen.org/xen-devel
> >
> 

Re: [PATCH 0 of 4] Add commands to automatically prep devices for pass-through

[George Dunlap]

On 09/05/12 12:59, Ian Campbell wrote:
> Right, however it is strictly speaking a new feature which is not
> mentioned on the TODO list and has not previously been posted (AFAIK,
> please correct me if not) and we are currently supposed to be in feature
> freeze (and have been for several weeks, if not a month).
>
> IIRC this functionality was mooted when the pci permissive patch was
> being done as something which would be a 4.3 feature.
> We need to decide if we want to make an exception for this new feature
> or not. Although I'm sure this feature is very nice and handy, we've
> lived without it for years and people seem to be able to use the
> existing scheme.
My recollection was that I did "moot" the functionailty basically a day 
or two after the official feature freeze; my impression from those 
discussions was that we wouldn't add the feature to the list, but that 
it was reasonable to ask for an exception at such time as I actually had 
the patches.  (Quite possible that my understanding is wrong there.)  
Unfortunately due to other priorities, I didn't manage to actually start 
working on them until the end of last week.

Maybe part of the issue is how they're being presented.  My original 
plan was to add options to libxl_pci_{add,remove} do the rebinding, 
which would have looked less like a new feature and more like an 
improvement.  This version actually introduces new functions, so it 
looks much more like a "new feature", even though the functionality is 
the same, and arguably having a separate step is less of a risk of 
someone tripping over something.

Of course everyone thinks their pet feature is incredibly important. 
:-)  But we are planning on making a public push on some of the security 
features of Xen this summer, which will hopefully mean a lot of people 
investigate the idea of using pci pass-through functionality for network 
driver domains.  The problem with saying "people seem to be able to use 
the existing scheme" is that you only see those who have gone through it 
and succeeded; you don't see how many took at look at the instructions 
and said, "That sounds too complicated/dangerous for me."  It would be a 
shame if we tooted Xen's horn about security, got an extra several 
thousand people to look into it, and then had half of them go away 
because of something simple like this.  I think that's my main concern.

We could of course make the HOWTOs easier to follow even without 
including this functionality; including Anthony's (very useful) 
rebinding script would certainly be a lot better than having everyone 
manually doing the sysfs stuff.  But not nearly as good as having the 
commands in-tree.

If we decide not to take the new functions, can I propose that we at 
least take the one that renames "pci-device-list-assignable", so we 
won't have to rename it / deal with compatibility issues when these are 
implemented for 4.3?

  -George

Re: [PATCH 0 of 4] Add commands to automatically prep devices for pass-through

[George Dunlap]

On Wed, May 9, 2012 at 2:45 PM, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> On 09/05/12 12:59, Ian Campbell wrote:
>>
>> Right, however it is strictly speaking a new feature which is not
>> mentioned on the TODO list and has not previously been posted (AFAIK,
>> please correct me if not) and we are currently supposed to be in feature
>> freeze (and have been for several weeks, if not a month).
>>
>> IIRC this functionality was mooted when the pci permissive patch was
>> being done as something which would be a 4.3 feature.
>> We need to decide if we want to make an exception for this new feature
>> or not. Although I'm sure this feature is very nice and handy, we've
>> lived without it for years and people seem to be able to use the
>> existing scheme.

And, I realize that at some point all of the deadlines are going to be
arbitrary; but I have always felt this is important enough to get an
exception.  I consider having to muck about with sysfs to be basically
a UI bug that really needs fixing.  I have a lot of other things that
I would like to get done for the 4.2 release; but I thought this was
important enough to get priority (above the PoD patch series, for
instance).  NB I'm not saying that you should accept it because I
worked on it; I only bring it up to demonstrate how important I think
the feature is.

 -George

Re: [PATCH 0 of 4] Add commands to automatically prep devices for pass-through

[Ian Campbell]

On Thu, 2012-05-10 at 11:17 +0100, George Dunlap wrote:
> On Wed, May 9, 2012 at 2:45 PM, George Dunlap
> <george.dunlap@eu.citrix.com> wrote:
> > On 09/05/12 12:59, Ian Campbell wrote:
> >>
> >> Right, however it is strictly speaking a new feature which is not
> >> mentioned on the TODO list and has not previously been posted (AFAIK,
> >> please correct me if not) and we are currently supposed to be in feature
> >> freeze (and have been for several weeks, if not a month).
> >>
> >> IIRC this functionality was mooted when the pci permissive patch was
> >> being done as something which would be a 4.3 feature.
> >> We need to decide if we want to make an exception for this new feature
> >> or not. Although I'm sure this feature is very nice and handy, we've
> >> lived without it for years and people seem to be able to use the
> >> existing scheme.
> 
> And, I realize that at some point all of the deadlines are going to be
> arbitrary; but I have always felt this is important enough to get an
> exception.  I consider having to muck about with sysfs to be basically
> a UI bug that really needs fixing.  I have a lot of other things that
> I would like to get done for the 4.2 release; but I thought this was
> important enough to get priority (above the PoD patch series, for
> instance).  NB I'm not saying that you should accept it because I
> worked on it; I only bring it up to demonstrate how important I think
> the feature is.

OK, given that the code is basically self contained and shouldn't effect
anything unless a user "opts-in" to using it I think you've convinced
me. Lets take this (I'll review the actual patches shortly).

BTW, IMHO it is preferable for actual deployments to use the kernel
command line options to hide devices rather than either this feature or
sysfs.

Fully hiding the device from dom0 drivers generally seems like it is
always better. That way the first driver to try and touch the hardware
is the one inside the domU. This avoids issues with dom0 drivers setting
stuff up but not tearing it down in a way that domU can cope with and
makes the use of hardware which doesn't support FLR more reliable etc.

Ian.

Re: [PATCH 1 of 4] libxl: Make a helper function write a BDF to a sysfs path

[Ian Campbell]

On Wed, 2012-05-09 at 11:28 +0100, George Dunlap wrote:
> This functionality will be used several times in subsequent patches.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> diff -r d5268710a5ca -r 0772f1d07d1c tools/libxl/libxl_pci.c
> --- a/tools/libxl/libxl_pci.c	Fri May 04 10:46:23 2012 +0100
> +++ b/tools/libxl/libxl_pci.c	Tue May 08 17:18:31 2012 +0100
> @@ -327,6 +327,36 @@ static int is_pcidev_in_array(libxl_devi
>      return 0;
>  }
>  
> +/* Write the standard BDF into the sysfs path given by sysfs_path. */
> +static int sysfs_write_bdf(libxl__gc *gc, const char * sysfs_path,
> +                           libxl_device_pci *pcidev)
> +{
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    int rc, fd;
> +    char *buf;
> +
> +    fd = open(sysfs_path, O_WRONLY);
> +    if (fd < 0) {
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn't open %s",
> +                         sysfs_path);
> +        return ERROR_FAIL;
> +    }
> +        
> +    buf = libxl__sprintf(gc, PCI_BDF, pcidev->domain, pcidev->bus,
> +                         pcidev->dev, pcidev->func);
> +    rc = write(fd, buf, strlen(buf));
> +    /* Annoying to have two if's, but we need the errno */
> +    if (rc < 0)
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> +                         "write to %s returned %d", sysfs_path, rc);
> +    close(fd);
> +
> +    if (rc < 0)
> +        return ERROR_FAIL;
> +
> +    return 0;
> +}
> +
>  libxl_device_pci *libxl_device_pci_list_assignable(libxl_ctx *ctx, int *num)
>  {
>      GC_INIT(ctx);
> @@ -571,27 +601,12 @@ static int do_pci_add(libxl__gc *gc, uin
>  
>          /* Don't restrict writes to the PCI config space from this VM */
>          if (pcidev->permissive) {
> -            int fd;
> -            char *buf;
> -            
> -            sysfs_path = libxl__sprintf(gc, SYSFS_PCIBACK_DRIVER"/permissive");
> -            fd = open(sysfs_path, O_WRONLY);
> -            if (fd < 0) {
> -                LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn't open %s",
> -                                 sysfs_path);
> +            if ( sysfs_write_bdf(gc, SYSFS_PCIBACK_DRIVER"/permissive",
> +                                 pcidev) < 0 ) {
> +                LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> +                           "Setting permissive for device");
>                  return ERROR_FAIL;
>              }
> - 
> -            buf = libxl__sprintf(gc, PCI_BDF, pcidev->domain, pcidev->bus,
> -                                 pcidev->dev, pcidev->func);
> -            rc = write(fd, buf, strlen(buf));
> -            /* Annoying to have two if's, but we need the errno */
> -            if (rc < 0)
> -                LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> -                                 "write to %s returned %d", sysfs_path, rc);
> -            close(fd);
> -            if (rc < 0)
> -                return ERROR_FAIL;
>          }
>          break;
>      }
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 2 of 4] libxl: Rename pci_list_assignable to pci_assignable_list

[Ian Campbell]

On Wed, 2012-05-09 at 11:28 +0100, George Dunlap wrote:
> ...to prepare for a consistent "pci_assignable_*" naming scheme.
> 
> Also move the man page entry into the PCI PASS-THROUGH section, rather
> than the XEN HOST section.
> 
> No functional changes.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

(moving the definition of pcilist_assignable et al seems superfluous,
but I suppose it serves some purpose later in the series and the
function is small enough to review the changes + code motion by eye)

> 
> diff -r 0772f1d07d1c -r 5b5070d487d9 docs/man/xl.pod.1
> --- a/docs/man/xl.pod.1	Tue May 08 17:18:31 2012 +0100
> +++ b/docs/man/xl.pod.1	Wed May 09 11:21:19 2012 +0100
> @@ -687,13 +687,6 @@ explanatory.
>  
>  Prints the current uptime of the domains running.
>  
> -=item B<pci-list-assignable-devices>
> -
> -List all the assignable PCI devices.
> -These are devices in the system which are configured to be
> -available for passthrough and are bound to a suitable PCI
> -backend driver in domain 0 rather than a real driver.
> -
>  =back
>  
>  =head1 SCHEDULER SUBCOMMANDS
> @@ -1026,6 +1019,13 @@ List virtual network interfaces for a do
>  
>  =over 4
>  
> +=item B<pci-assignable-list>
> +
> +List all the assignable PCI devices.
> +These are devices in the system which are configured to be
> +available for passthrough and are bound to a suitable PCI
> +backend driver in domain 0 rather than a real driver.
> +
>  =item B<pci-attach> I<domain-id> I<BDF>
>  
>  Hot-plug a new pass-through pci device to the specified domain.
> diff -r 0772f1d07d1c -r 5b5070d487d9 tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h	Tue May 08 17:18:31 2012 +0100
> +++ b/tools/libxl/libxl.h	Wed May 09 11:21:19 2012 +0100
> @@ -662,7 +662,7 @@ libxl_device_pci *libxl_device_pci_list(
>   * could be assigned to a domain (i.e. are bound to the backend
>   * driver) but are not currently.
>   */
> -libxl_device_pci *libxl_device_pci_list_assignable(libxl_ctx *ctx, int *num);
> +libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num);
>  
>  /* CPUID handling */
>  int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str);
> diff -r 0772f1d07d1c -r 5b5070d487d9 tools/libxl/libxl_pci.c
> --- a/tools/libxl/libxl_pci.c	Tue May 08 17:18:31 2012 +0100
> +++ b/tools/libxl/libxl_pci.c	Wed May 09 11:21:19 2012 +0100
> @@ -357,7 +357,7 @@ static int sysfs_write_bdf(libxl__gc *gc
>      return 0;
>  }
>  
> -libxl_device_pci *libxl_device_pci_list_assignable(libxl_ctx *ctx, int *num)
> +libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num)
>  {
>      GC_INIT(ctx);
>      libxl_device_pci *pcidevs = NULL, *new, *assigned;
> @@ -684,7 +684,7 @@ static int libxl_pcidev_assignable(libxl
>      libxl_device_pci *pcidevs;
>      int num, i;
>  
> -    pcidevs = libxl_device_pci_list_assignable(ctx, &num);
> +    pcidevs = libxl_device_pci_assignable_list(ctx, &num);
>      for (i = 0; i < num; i++) {
>          if (pcidevs[i].domain == pcidev->domain &&
>              pcidevs[i].bus == pcidev->bus &&
> diff -r 0772f1d07d1c -r 5b5070d487d9 tools/libxl/xl.h
> --- a/tools/libxl/xl.h	Tue May 08 17:18:31 2012 +0100
> +++ b/tools/libxl/xl.h	Wed May 09 11:21:19 2012 +0100
> @@ -34,9 +34,9 @@ int main_cd_insert(int argc, char **argv
>  int main_console(int argc, char **argv);
>  int main_vncviewer(int argc, char **argv);
>  int main_pcilist(int argc, char **argv);
> -int main_pcilist_assignable(int argc, char **argv);
>  int main_pcidetach(int argc, char **argv);
>  int main_pciattach(int argc, char **argv);
> +int main_pciassignable_list(int argc, char **argv);
>  int main_restore(int argc, char **argv);
>  int main_migrate_receive(int argc, char **argv);
>  int main_save(int argc, char **argv);
> diff -r 0772f1d07d1c -r 5b5070d487d9 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c	Tue May 08 17:18:31 2012 +0100
> +++ b/tools/libxl/xl_cmdimpl.c	Wed May 09 11:21:19 2012 +0100
> @@ -2223,34 +2223,6 @@ int main_vncviewer(int argc, char **argv
>      return 0;
>  }
>  
> -static void pcilist_assignable(void)
> -{
> -    libxl_device_pci *pcidevs;
> -    int num, i;
> -
> -    pcidevs = libxl_device_pci_list_assignable(ctx, &num);
> -
> -    if ( pcidevs == NULL )
> -        return;
> -    for (i = 0; i < num; i++) {
> -        printf("%04x:%02x:%02x.%01x\n",
> -               pcidevs[i].domain, pcidevs[i].bus, pcidevs[i].dev, pcidevs[i].func);
> -        libxl_device_pci_dispose(&pcidevs[i]);
> -    }
> -    free(pcidevs);
> -}
> -
> -int main_pcilist_assignable(int argc, char **argv)
> -{
> -    int opt;
> -
> -    if ((opt = def_getopt(argc, argv, "", "pci-list-assignable-devices", 0)) != -1)
> -        return opt;
> -
> -    pcilist_assignable();
> -    return 0;
> -}
> -
>  static void pcilist(const char *dom)
>  {
>      libxl_device_pci *pcidevs;
> @@ -2368,6 +2340,34 @@ int main_pciattach(int argc, char **argv
>      return 0;
>  }
>  
> +static void pciassignable_list(void)
> +{
> +    libxl_device_pci *pcidevs;
> +    int num, i;
> +
> +    pcidevs = libxl_device_pci_assignable_list(ctx, &num);
> +
> +    if ( pcidevs == NULL )
> +        return;
> +    for (i = 0; i < num; i++) {
> +        printf("%04x:%02x:%02x.%01x\n",
> +               pcidevs[i].domain, pcidevs[i].bus, pcidevs[i].dev, pcidevs[i].func);
> +        libxl_device_pci_dispose(&pcidevs[i]);
> +    }
> +    free(pcidevs);
> +}
> +
> +int main_pciassignable_list(int argc, char **argv)
> +{
> +    int opt;
> +
> +    if ((opt = def_getopt(argc, argv, "", "pci-assignable-list", 0)) != -1)
> +        return opt;
> +
> +    pciassignable_list();
> +    return 0;
> +}
> +
>  static void pause_domain(const char *p)
>  {
>      find_domain(p);
> diff -r 0772f1d07d1c -r 5b5070d487d9 tools/libxl/xl_cmdtable.c
> --- a/tools/libxl/xl_cmdtable.c	Tue May 08 17:18:31 2012 +0100
> +++ b/tools/libxl/xl_cmdtable.c	Wed May 09 11:21:19 2012 +0100
> @@ -86,8 +86,8 @@ struct cmd_spec cmd_table[] = {
>        "List pass-through pci devices for a domain",
>        "<Domain>",
>      },
> -    { "pci-list-assignable-devices",
> -      &main_pcilist_assignable, 0,
> +    { "pci-assignable-list",
> +      &main_pciassignable_list, 0,
>        "List all the assignable pci devices",
>        "",
>      },
> diff -r 0772f1d07d1c -r 5b5070d487d9 tools/python/xen/lowlevel/xl/xl.c
> --- a/tools/python/xen/lowlevel/xl/xl.c	Tue May 08 17:18:31 2012 +0100
> +++ b/tools/python/xen/lowlevel/xl/xl.c	Wed May 09 11:21:19 2012 +0100
> @@ -566,13 +566,13 @@ static PyObject *pyxl_pci_parse(XlObject
>      return (PyObject *)pci;
>  }
>  
> -static PyObject *pyxl_pci_list_assignable(XlObject *self, PyObject *args)
> +static PyObject *pyxl_pci_assignable_list(XlObject *self, PyObject *args)
>  {
>      libxl_device_pci *dev;
>      PyObject *list;
>      int nr_dev, i;
>  
> -    dev = libxl_device_pci_list_assignable(self->ctx, &nr_dev);
> +    dev = libxl_device_pci_assignable_list(self->ctx, &nr_dev);
>      if ( dev == NULL ) {
>          PyErr_SetString(xl_error_obj, "Cannot list assignable devices");
>          return NULL;
> @@ -662,8 +662,8 @@ static PyMethodDef pyxl_methods[] = {
>           "Parse pass-through PCI device spec (BDF)"},
>      {"device_pci_list", (PyCFunction)pyxl_pci_list, METH_VARARGS,
>          "List PCI devices assigned to a domain"},
> -    {"device_pci_list_assignable",
> -        (PyCFunction)pyxl_pci_list_assignable, METH_NOARGS,
> +    {"device_pci_assignable_list",
> +        (PyCFunction)pyxl_pci_assignable_list, METH_NOARGS,
>          "List assignable PCI devices"},
>      { NULL, NULL, 0, NULL }
>  };
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 2 of 4] libxl: Rename pci_list_assignable to pci_assignable_list

[George Dunlap]

On 10/05/12 11:43, Ian Campbell wrote:
> Acked-by: Ian Campbell<ian.campbell@citrix.com>
>
> (moving the definition of pcilist_assignable et al seems superfluous,
> but I suppose it serves some purpose later in the series and the
> function is small enough to review the changes + code motion by eye)
Ah, right -- I didn't think about that.  I'll try to avoid motion + 
changes in the future.  You're right, the code motion is because we're 
changing the function from being mainly a "pcilist" thing (thus grouped 
with the other pci-list command) to mainly being an "assignable" thing 
(and thus grouped with other "assignable" commands).

  -George

Re: [PATCH 3 of 4] libxl: Introduce pci_assignable_add and pci_assignable_remove

[Ian Campbell]

On Wed, 2012-05-09 at 11:28 +0100, George Dunlap wrote:
> Introduce libxl helper functions to prepare devices to be passed through to
> guests.  This is meant to replace of all the manual sysfs commands which
> are currently required.
> 
> pci_assignable_add accepts a BDF for a device and will:
> * Unbind a device from its current driver, if any
> * If "rebind" is set, it will store the path of the driver from which we
>   unplugged it in /libxl/pciback/$BDF/driver_path

Since you don't know whether the user is going to pass -r to
assignable_remove why not always store this?

> * If necessary, create a slot for it in pciback

I must confess I'm a bit fuzzy on the relationship between slots and
bindings, where does the "if necessary" come into it?

I was wondering while reading the patch if unconditionally adding a
removing the slot might simplify a bunch of stuff, but I suspect I just
don't appreciate some particular aspect of how pciback works...

> * Bind the device to pciback
> 
> At this point it will show up in pci_assignable_list, and is ready to be passed
> through to a guest.
> 
> pci_assignable_remove accepts a BDF for a device and will:
> * Unbind the device from pciback
> * Remove the slot from pciback
> * If "rebind" is set, and /libx/pciback/$BDF/driver_path exists, it will
>   attempt to rebind the device to its original driver.
> 
> NB that "$BDF" in this case uses dashes instead of : and ., because : and . are
> illegal characters in xenstore paths.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> diff -r 5b5070d487d9 -r 17a5b562d1e7 tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h       Wed May 09 11:21:19 2012 +0100
> +++ b/tools/libxl/libxl.h       Wed May 09 11:21:28 2012 +0100
> @@ -658,10 +658,25 @@ int libxl_device_pci_destroy(libxl_ctx *
>  libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid, int *num);
> 
>  /*
> - * Similar to libxl_device_pci_list but returns all devices which
> - * could be assigned to a domain (i.e. are bound to the backend
> - * driver) but are not currently.
> + * Functions related to making devices assignable -- that is, bound to
> + * the pciback driver, ready to be given to a guest via
> + * libxl_pci_device_add.
> + *
> + * - ..._add() will unbind the device from its current driver (if
> + * already bound) and re-bind it to pciback; at that point it will be
> + * ready to be assigned to a VM.  If rebind is set, it will store the
> + * path to the old driver in xenstore so that it can be handed back to
> + * dom0 on restore.
> + *
> + * - ..._remove() will unbind the device from pciback, and if
> + * rebind is non-zero, attempt to assign it back to the driver
> + * from whence it came.
> + *
> + * - ..._list() will return a list of the PCI devices available to be
> + * assigned.
>   */
> +int libxl_device_pci_assignable_add(libxl_ctx *ctx, libxl_device_pci *pcidev, int rebind);
> +int libxl_device_pci_assignable_remove(libxl_ctx *ctx, libxl_device_pci *pcidev, int rebind);
>  libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num);
> 
>  /* CPUID handling */
> diff -r 5b5070d487d9 -r 17a5b562d1e7 tools/libxl/libxl_pci.c
> --- a/tools/libxl/libxl_pci.c   Wed May 09 11:21:19 2012 +0100
> +++ b/tools/libxl/libxl_pci.c   Wed May 09 11:21:28 2012 +0100
> @@ -21,6 +21,7 @@
>  #define PCI_BDF                "%04x:%02x:%02x.%01x"
>  #define PCI_BDF_SHORT          "%02x:%02x.%01x"
>  #define PCI_BDF_VDEVFN         "%04x:%02x:%02x.%01x@%02x"
> +#define PCI_BDF_XSPATH         "%04x-%02x-%02x-%01x"
> 
>  static unsigned int pcidev_encode_bdf(libxl_device_pci *pcidev)
>  {
> @@ -408,6 +409,347 @@ out:
>      return pcidevs;
>  }
> 
> +/* Unbind device from its current driver, if any.  If driver_path is non-NULL,
> + * store the path to the original driver in it. */
> +static int sysfs_dev_unbind(libxl__gc *gc, libxl_device_pci *pcidev, char **driver_path)
> +{
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    char * spath, *_dp, **dp;
> +    struct stat st;
> +
> +    if ( driver_path )
> +        dp = driver_path;
> +    else
> +        dp = &_dp;
> +
> +    spath = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/driver",
> +                           pcidev->domain,
> +                           pcidev->bus,
> +                           pcidev->dev,
> +                           pcidev->func);
> +    if ( !lstat(spath, &st) ) {
> +        /* Find the canonical path to the driver. */
> +        *dp = libxl__zalloc(gc, PATH_MAX);

Should we be actually using fpathconf / sysconf here?

> +        *dp = realpath(spath, *dp);
> +        if ( !*dp ) {
> +            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "realpath() failed");

Since errno is meaningful you want LIBXL__LOGERRNO here. Or the short
form LOGE().

Where you have pointer output params (like driver_path here) in general
I think it is preferable to do everything with a local (single
indirection) pointer and only update the output parameter on success.
This means you avoid leaking/exposing a partial result on error but also
means you can drop a lot of "*" from around the function.

Maybe the gc makes this moot, but the "if ( driver_path )" stuff at the
top of the fn took me several seconds to work out also ;-).

> +            return -1;
> +        }
> +
> +        LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Driver re-plug path: %s",
> +                   *dp);
> +
> +        /* Unbind from the old driver */
> +        spath = libxl__sprintf(gc, "%s/unbind", *dp);
> +        if ( sysfs_write_bdf(gc, spath, pcidev) < 0 ) {
> +            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Couldn't unbind device");

Not sure what errno is like here, worth printing it. Looking back at
patch #1 I suspect sysfs_write_bdf should preserve errno over the call
to close, so that suitable reporting can happen in the caller.

> +            return -1;
> +        }
> +    }
> +    return 0;
> +}
> +
> +/* Scan through /sys/.../pciback/slots looking for pcidev's BDF */
> +static int pciback_dev_has_slot(libxl__gc *gc, libxl_device_pci *pcidev)

Is the concept of "having a slot" distinct from being bound to pciback?

> +{
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    FILE *f;
> +    int rc = 0;
> +    unsigned bus, dev, func;
> +
> +    f = fopen(SYSFS_PCIBACK_DRIVER"/slots", "r");
> +
> +    if (f == NULL) {
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn't open %s",
> +                         SYSFS_PCIBACK_DRIVER"/slots");
> +        return ERROR_FAIL;
> +    }
> +
> +    while(fscanf(f, "0000:%x:%x.%x\n", &bus, &dev, &func)==3) {

Jan recently added support for PCI domains, does that have any impact on
the hardcoded 0000 here? I suppose that would require PCI domains
support in the dom0 kernel -- but that seems likely to already be there
(or be imminent)

I think the 0000 should be %x into domain compared with pcidev->domain.

> +        if(bus == pcidev->bus
> +           && dev == pcidev->dev
> +           && func == pcidev->func) {
> +            rc = 1;
> +            goto out;
> +        }
> +    }
> +out:
> +    fclose(f);
> +    return rc;
> +}
> +
> +static int pciback_dev_is_assigned(libxl__gc *gc, libxl_device_pci *pcidev)
> +{
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    char * spath;
> +    int rc;
> +    struct stat st;
> +
> +    spath = libxl__sprintf(gc, SYSFS_PCIBACK_DRIVER"/"PCI_BDF,
> +                           pcidev->domain, pcidev->bus,
> +                           pcidev->dev, pcidev->func);
> +    rc = lstat(spath, &st);
> +
> +    if( rc == 0 )
> +        return 1;
> +    if ( rc < 0 && errno == ENOENT )
> +        return 0;
> +    LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Accessing %s", spath);
> +    return -1;
> +}
> +
> +static int pciback_dev_assign(libxl__gc *gc, libxl_device_pci *pcidev)
> +{
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    int rc;
> +
> +    if ( (rc=pciback_dev_has_slot(gc, pcidev)) < 0 ) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> +                   "Error checking for pciback slot");

Log errno?

Also pciback_dev_has_slot itself always logs on error, you probably
don't need both.

> +        return ERROR_FAIL;
> +    } else if (rc == 0) {
> +        if ( sysfs_write_bdf(gc, SYSFS_PCIBACK_DRIVER"/new_slot",
> +                             pcidev) < 0 ) {
> +            LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> +                       "Couldn't bind device to pciback!");

ERRNO here too.

> +            return ERROR_FAIL;
> +        }
> +    }
> +
> +    if ( sysfs_write_bdf(gc, SYSFS_PCIBACK_DRIVER"/bind", pcidev) < 0 ) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Couldn't bind device to pciback!");

ERRNO here too. Or since there are loads of these perhaps sysfs_write_bf
should log on failure, either just the fact of the failed write to a
path (which implies the  underlying failure was to bind/unbind) or you
could add a "const char *what" param to use in logging.

> +        return ERROR_FAIL;
> +    }
> +    return 0;
> +}
> +
> +static int pciback_dev_unassign(libxl__gc *gc, libxl_device_pci *pcidev)
> +{
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +
> +    /* Remove from pciback */
> +    if ( sysfs_dev_unbind(gc, pcidev, NULL) < 0 ) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Couldn't unbind device!");
> +        return ERROR_FAIL;
> +    }
> +
> +    /* Remove slot if necessary */
> +    if ( pciback_dev_has_slot(gc, pcidev) > 0 ) {
> +        if ( sysfs_write_bdf(gc, SYSFS_PCIBACK_DRIVER"/remove_slot",
> +                             pcidev) < 0 ) {
> +            LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> +                       "Couldn't remove pciback slot");

ERRNO

> +            return ERROR_FAIL;
> +        }
> +    }
> +    return 0;
> +}
> +
> +/* FIXME */

Leftover?

> +#define PCIBACK_INFO_PATH "/libxl/pciback"
> +
> +static void pci_assignable_driver_path_write(libxl__gc *gc,
> +                                            libxl_device_pci *pcidev,
> +                                            char *driver_path)
> +{
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    char *path;
> +    xs_transaction_t t = 0;
> +    struct xs_permissions perm[1];
> +
> +    perm[0].id = 0;
> +    perm[0].perms = XS_PERM_NONE;
> +
> +retry:
> +    t = xs_transaction_start(ctx->xsh);
> +
> +    path = libxl__sprintf(gc, PCIBACK_INFO_PATH"/"PCI_BDF_XSPATH"/driver_path",
> +                          pcidev->domain,
> +                          pcidev->bus,
> +                          pcidev->dev,
> +                          pcidev->func);
> +    xs_rm(ctx->xsh, t, path);

Why do you need to rm first? Won't the write just replace whatever it is
(and that means the need for a transaction goes away too)

In any case you should create path outside the retry loop instead of
needlessly recreating it each time around.

> +    if ( libxl__xs_write(gc, XBT_NULL, path, "%s", driver_path) < 0 ) {
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> +                         "Write of %s to node %s failed",
> +                         driver_path, path);
> +    }
> +
> +    if (!xs_transaction_end(ctx->xsh, t, 0)) {
> +        if (errno == EAGAIN) {
> +            t = 0;
> +            goto retry;
> +        }
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> +                         "xenstore transaction commit failed; device "
> +                         " will not be rebound to original driver.");
> +    }
> +}
> +
> +static char * pci_assignable_driver_path_read(libxl__gc *gc,
> +                                              libxl_device_pci *pcidev)
> +{
> +    return libxl__xs_read(gc, XBT_NULL,
> +                          libxl__sprintf(gc,
> +                           PCIBACK_INFO_PATH "/" PCI_BDF_XSPATH "/driver_path",
> +                           pcidev->domain,
> +                           pcidev->bus,
> +                           pcidev->dev,
> +                           pcidev->func));
> +}
> +
> +static void pci_assignable_driver_path_remove(libxl__gc *gc,
> +                                              libxl_device_pci *pcidev)
> +{
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    char * path;
> +    xs_transaction_t t;
> +
> +    /* Remove the xenstore entry */
> +retry:
> +    t = xs_transaction_start(ctx->xsh);
> +    path = libxl__sprintf(gc, PCIBACK_INFO_PATH "/" PCI_BDF_XSPATH,
> +                          pcidev->domain,
> +                          pcidev->bus,
> +                          pcidev->dev,
> +                          pcidev->func);
> +    xs_rm(ctx->xsh, t, path);

You don't need a transaction for a single operation. (and if you did
then "path = ..." could have been hoisted out)

> +    if (!xs_transaction_end(ctx->xsh, t, 0)) {
> +        if (errno == EAGAIN)
> +            goto retry;
> +        else
> +            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> +                             "Failed to remove xenstore entry");
> +    }
> +}
> +
> +static int libxl__device_pci_assignable_add(libxl__gc *gc,
> +                                            libxl_device_pci *pcidev,
> +                                            int rebind)
> +{
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    unsigned dom, bus, dev, func;
> +    char *spath, *driver_path = NULL;
> +    struct stat st;
> +
> +    /* Local copy for convenience */
> +    dom = pcidev->domain;
> +    bus = pcidev->bus;
> +    dev = pcidev->dev;
> +    func = pcidev->func;
> +
> +    /* See if the device exists */
> +    spath = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF, dom, bus, dev, func);
> +    if ( lstat(spath, &st) ) {
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn't lstat %s", spath);
> +        return ERROR_FAIL;
> +    }
> +
> +    /* Check to see if it's already assigned to pciback */
> +    if ( pciback_dev_is_assigned(gc, pcidev) ) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, PCI_BDF" already assigned to pciback",
> +                   dom, bus, dev, func);
> +        return 0;
> +    }
> +
> +    /* Check to see if there's already a driver that we need to unbind from */
> +    if ( sysfs_dev_unbind(gc, pcidev, &driver_path ) ) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Couldn't unbind "PCI_BDF" from driver",
> +                   dom, bus, dev, func);
> +        return ERROR_FAIL;
> +    }
> +
> +    /* Store driver_path for rebinding to dom0 */
> +    if ( rebind ) {
> +        if ( driver_path ) {
> +            pci_assignable_driver_path_write(gc, pcidev, driver_path);
> +        } else {
> +            LIBXL__LOG(ctx, LIBXL__LOG_WARNING,
> +                       PCI_BDF" not bound to a driver, will not be rebound.",
> +                       dom, bus, dev, func);
> +        }
> +    }
> +
> +    if ( pciback_dev_assign(gc, pcidev) ) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Couldn't bind device to pciback!");
> +        return ERROR_FAIL;
> +    }
> +
> +    return 0;
> +}
> +
> +static int libxl__device_pci_assignable_remove(libxl__gc *gc,
> +                                               libxl_device_pci *pcidev,
> +                                               int rebind)
> +{
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    int rc;
> +    char *driver_path;
> +
> +    /* Unbind from pciback */
> +    if ( (rc=pciback_dev_is_assigned(gc, pcidev)) < 0 ) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Checking if pciback was assigned");
> +        return ERROR_FAIL;
> +    } else if ( rc ) {
> +        pciback_dev_unassign(gc, pcidev);
> +    } else {
> +        LIBXL__LOG(ctx, LIBXL__LOG_WARNING,
> +                   "Not bound to pciback");
> +    }
> +
> +    /* Rebind if necessary */
> +    driver_path = pci_assignable_driver_path_read(gc, pcidev);
> +
> +    if ( driver_path ) {
> +        if ( rebind ) {
> +            LIBXL__LOG(ctx, LIBXL__LOG_INFO, "Rebinding to driver at %s",
> +                       driver_path);
> +
> +            if ( sysfs_write_bdf(gc,
> +                                 libxl__sprintf(gc, "%s/bind", driver_path),
> +                                 pcidev) < 0 ) {
> +                LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> +                           "Couldn't bind device to %s", driver_path);
> +                return -1;
> +            }
> +        }
> +
> +        pci_assignable_driver_path_remove(gc, pcidev);
> +    } else {
> +        if ( rebind ) {
> +            LIBXL__LOG(ctx, LIBXL__LOG_WARNING,
> +                       "Couldn't find path for original driver; not rebinding");
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +int libxl_device_pci_assignable_add(libxl_ctx *ctx, libxl_device_pci *pcidev,
> +                                    int rebind)
> +{
> +    GC_INIT(ctx);
> +    int rc;
> +
> +    rc = libxl__device_pci_assignable_add(gc, pcidev, rebind);
> +
> +    GC_FREE;
> +    return rc;
> +}

Are there internal callers of libxl__device_pci_assignable_add/remove?
If not then there's no reason to split into internal/external functions.
Doesn't matter much I guess.

> +
> +
> +int libxl_device_pci_assignable_remove(libxl_ctx *ctx, libxl_device_pci *pcidev,
> +                                       int rebind)
> +{
> +    GC_INIT(ctx);
> +    int rc;
> +
> +    rc = libxl__device_pci_assignable_remove(gc, pcidev, rebind);
> +
> +    GC_FREE;
> +    return rc;
> +}
> +
>  /*
>   * This function checks that all functions of a device are bound to pciback
>   * driver. It also initialises a bit-mask of which function numbers are present
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 4 of 4] xl: Add pci_assignable_add and remove commands

[Ian Campbell]

On Wed, 2012-05-09 at 11:28 +0100, George Dunlap wrote:
> pci-assignable-add will always store the driver rebind path, but
> pci-assignable-remove will only actually rebind if asked to do so.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> diff -r 17a5b562d1e7 -r a1c357853735 docs/man/xl.pod.1
> --- a/docs/man/xl.pod.1	Wed May 09 11:21:28 2012 +0100
> +++ b/docs/man/xl.pod.1	Wed May 09 11:24:01 2012 +0100
> @@ -1026,6 +1026,26 @@ These are devices in the system which ar
>  available for passthrough and are bound to a suitable PCI
>  backend driver in domain 0 rather than a real driver.
>  
> +=item B<pci-assignable-add> I<BDF>
> +
> +Make the device at PCI Bus/Device/Function BDF assignable to guests.  This
> +will bind the device to the pciback driver.  If it is already bound to a
> +driver, it will first be unbound, and the original driver stored so that it
> +can be re-bound to the same driver later if desired.  
> +
> +CAUTION: This will make the device unusable by Domain 0 until it is
> +returned with pci-assignable-remove.  Care should therefore be taken
> +not to do this on a device critical to domain 0's operation, such as
> +storage controllers, network interfaces, or GPUs that are currently
> +being used.
> +
> +=item B<pci-assignable-remove> [I<-r>] I<BDF>
> +
> +Make the device at PCI Bus/Device/Function BDF assignable to guests.  This
> +will at least unbind the device from pciback.  If the -r option is specified,
> +it will also attempt to re-bind the device to its original driver, making it
> +usable by Domain 0 again.
> +
>  =item B<pci-attach> I<domain-id> I<BDF>
>  
>  Hot-plug a new pass-through pci device to the specified domain.
> diff -r 17a5b562d1e7 -r a1c357853735 tools/libxl/xl.h
> --- a/tools/libxl/xl.h	Wed May 09 11:21:28 2012 +0100
> +++ b/tools/libxl/xl.h	Wed May 09 11:24:01 2012 +0100
> @@ -36,6 +36,8 @@ int main_vncviewer(int argc, char **argv
>  int main_pcilist(int argc, char **argv);
>  int main_pcidetach(int argc, char **argv);
>  int main_pciattach(int argc, char **argv);
> +int main_pciassignable_add(int argc, char **argv);
> +int main_pciassignable_remove(int argc, char **argv);
>  int main_pciassignable_list(int argc, char **argv);
>  int main_restore(int argc, char **argv);
>  int main_migrate_receive(int argc, char **argv);
> diff -r 17a5b562d1e7 -r a1c357853735 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c	Wed May 09 11:21:28 2012 +0100
> +++ b/tools/libxl/xl_cmdimpl.c	Wed May 09 11:24:01 2012 +0100
> @@ -2368,6 +2368,82 @@ int main_pciassignable_list(int argc, ch
>      return 0;
>  }
>  
> +static void pciassignable_add(const char *bdf, int rebind)
> +{
> +    libxl_device_pci pcidev;
> +    XLU_Config *config;
> +
> +    memset(&pcidev, 0x00, sizeof(pcidev));

libxl_device_pci_init please.

> +
> +    config = xlu_cfg_init(stderr, "command line");
> +    if (!config) { perror("xlu_cfg_inig"); exit(-1); }
> +
> +    if (xlu_pci_parse_bdf(config, &pcidev, bdf)) {
> +        fprintf(stderr, "pci-assignable-add: malformed BDF specification \"%s\"\n", bdf);
> +        exit(2);
> +    }
> +    libxl_device_pci_assignable_add(ctx, &pcidev, rebind);
> +    libxl_device_pci_dispose(&pcidev);
> +}
> +
> +int main_pciassignable_add(int argc, char **argv)
> +{
> +    int opt;
> +    const char *bdf = NULL;
> +
> +    while ((opt = def_getopt(argc, argv, "", "pci-assignable-add", 1)) != -1) {
> +        switch (opt) {
> +        case 0: case 2:
> +            return opt;
> +        }
> +    }
> +
> +    bdf = argv[optind];
> +
> +    pciassignable_add(bdf, 1);
> +    return 0;
> +}
> +
> +static void pciassignable_remove(const char *bdf, int rebind)
> +{
> +    libxl_device_pci pcidev;
> +    XLU_Config *config;
> +
> +    memset(&pcidev, 0x00, sizeof(pcidev));

libxl_device_pci_init also.

You are also missing the xlu_cfg_destroy both here and in the add fn.

(it's a bit annoying that an XLU_COnfig is needed for these simple
helper functions, I suppose it's for logging, but maybe they could log
to stderr if cfg==NULL. Anyway, lets not fix that here)

> +
> +    config = xlu_cfg_init(stderr, "command line");
> +    if (!config) { perror("xlu_cfg_inig"); exit(-1); }
> +
> +    if (xlu_pci_parse_bdf(config, &pcidev, bdf)) {
> +        fprintf(stderr, "pci-assignable-remove: malformed BDF specification \"%s\"\n", bdf);
> +        exit(2);
> +    }
> +    libxl_device_pci_assignable_remove(ctx, &pcidev, rebind);
> +    libxl_device_pci_dispose(&pcidev);
> +}
> +
> +int main_pciassignable_remove(int argc, char **argv)
> +{
> +    int opt;
> +    const char *bdf = NULL;
> +    int rebind = 0;
> +
> +    while ((opt = def_getopt(argc, argv, "r", "pci-assignable-remove", 1)) != -1) {
> +        switch (opt) {
> +        case 0: case 2:
> +            return opt;
> +        case 'r':
> +            rebind=1;
> +            break;
> +        }
> +    }
> +
> +    bdf = argv[optind];
> +
> +    pciassignable_remove(bdf, rebind);
> +    return 0;
> +}
> +
>  static void pause_domain(const char *p)
>  {
>      find_domain(p);
> diff -r 17a5b562d1e7 -r a1c357853735 tools/libxl/xl_cmdtable.c
> --- a/tools/libxl/xl_cmdtable.c	Wed May 09 11:21:28 2012 +0100
> +++ b/tools/libxl/xl_cmdtable.c	Wed May 09 11:24:01 2012 +0100
> @@ -86,6 +86,20 @@ struct cmd_spec cmd_table[] = {
>        "List pass-through pci devices for a domain",
>        "<Domain>",
>      },
> +    { "pci-assignable-add",
> +      &main_pciassignable_add, 0,
> +      "Make a device assignable for pci-passthru",
> +      "<BDF>",
> +      "-h                      Print this help.\n"
> +    },
> +    { "pci-assignable-remove",
> +      &main_pciassignable_remove, 0,
> +      "Remove a device from being assignable",
> +      "[options] <BDF>",
> +      "-h                      Print this help.\n"
> +      "-r                      Attempt to re-assign the device to the\n"
> +      "                        original driver"
> +    },
>      { "pci-assignable-list",
>        &main_pciassignable_list, 0,
>        "List all the assignable pci devices",
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 0 of 4] Add commands to automatically prep devices for pass-through

[Sander Eikelenboom]

Hello Ian,

Thursday, May 10, 2012, 12:38:40 PM, you wrote:

> On Thu, 2012-05-10 at 11:17 +0100, George Dunlap wrote:
>> On Wed, May 9, 2012 at 2:45 PM, George Dunlap
>> <george.dunlap@eu.citrix.com> wrote:
>> > On 09/05/12 12:59, Ian Campbell wrote:
>> >>
>> >> Right, however it is strictly speaking a new feature which is not
>> >> mentioned on the TODO list and has not previously been posted (AFAIK,
>> >> please correct me if not) and we are currently supposed to be in feature
>> >> freeze (and have been for several weeks, if not a month).
>> >>
>> >> IIRC this functionality was mooted when the pci permissive patch was
>> >> being done as something which would be a 4.3 feature.
>> >> We need to decide if we want to make an exception for this new feature
>> >> or not. Although I'm sure this feature is very nice and handy, we've
>> >> lived without it for years and people seem to be able to use the
>> >> existing scheme.
>> 
>> And, I realize that at some point all of the deadlines are going to be
>> arbitrary; but I have always felt this is important enough to get an
>> exception.  I consider having to muck about with sysfs to be basically
>> a UI bug that really needs fixing.  I have a lot of other things that
>> I would like to get done for the 4.2 release; but I thought this was
>> important enough to get priority (above the PoD patch series, for
>> instance).  NB I'm not saying that you should accept it because I
>> worked on it; I only bring it up to demonstrate how important I think
>> the feature is.

> OK, given that the code is basically self contained and shouldn't effect
> anything unless a user "opts-in" to using it I think you've convinced
> me. Lets take this (I'll review the actual patches shortly).

> BTW, IMHO it is preferable for actual deployments to use the kernel
> command line options to hide devices rather than either this feature or
> sysfs.

> Fully hiding the device from dom0 drivers generally seems like it is
> always better. That way the first driver to try and touch the hardware
> is the one inside the domU. This avoids issues with dom0 drivers setting
> stuff up but not tearing it down in a way that domU can cope with and
> makes the use of hardware which doesn't support FLR more reliable etc.

Haven't checked it (haven't got the time right now) but:
Is using wildcards in the BDF's on the commandline supported already (like the ones supported in the config files for domains)

I tend to have quite long commandlines to hide a pci-e card with 8 functions (needed to specify 8 BDF's seperatly) for pci passthrough, would be nice if one could just specify 09:00.* for example.


> Ian.







-- 
Best regards,
 Sander                            mailto:linux@eikelenboom.it

Re: [PATCH 0 of 4] Add commands to automatically prep devices for pass-through

[Ian Campbell]

On Thu, 2012-05-10 at 15:12 +0100, Sander Eikelenboom wrote:
> Hello Ian,
> 
> Thursday, May 10, 2012, 12:38:40 PM, you wrote:
> 
> > On Thu, 2012-05-10 at 11:17 +0100, George Dunlap wrote:
> >> On Wed, May 9, 2012 at 2:45 PM, George Dunlap
> >> <george.dunlap@eu.citrix.com> wrote:
> >> > On 09/05/12 12:59, Ian Campbell wrote:
> >> >>
> >> >> Right, however it is strictly speaking a new feature which is not
> >> >> mentioned on the TODO list and has not previously been posted (AFAIK,
> >> >> please correct me if not) and we are currently supposed to be in feature
> >> >> freeze (and have been for several weeks, if not a month).
> >> >>
> >> >> IIRC this functionality was mooted when the pci permissive patch was
> >> >> being done as something which would be a 4.3 feature.
> >> >> We need to decide if we want to make an exception for this new feature
> >> >> or not. Although I'm sure this feature is very nice and handy, we've
> >> >> lived without it for years and people seem to be able to use the
> >> >> existing scheme.
> >> 
> >> And, I realize that at some point all of the deadlines are going to be
> >> arbitrary; but I have always felt this is important enough to get an
> >> exception.  I consider having to muck about with sysfs to be basically
> >> a UI bug that really needs fixing.  I have a lot of other things that
> >> I would like to get done for the 4.2 release; but I thought this was
> >> important enough to get priority (above the PoD patch series, for
> >> instance).  NB I'm not saying that you should accept it because I
> >> worked on it; I only bring it up to demonstrate how important I think
> >> the feature is.
> 
> > OK, given that the code is basically self contained and shouldn't effect
> > anything unless a user "opts-in" to using it I think you've convinced
> > me. Lets take this (I'll review the actual patches shortly).
> 
> > BTW, IMHO it is preferable for actual deployments to use the kernel
> > command line options to hide devices rather than either this feature or
> > sysfs.
> 
> > Fully hiding the device from dom0 drivers generally seems like it is
> > always better. That way the first driver to try and touch the hardware
> > is the one inside the domU. This avoids issues with dom0 drivers setting
> > stuff up but not tearing it down in a way that domU can cope with and
> > makes the use of hardware which doesn't support FLR more reliable etc.
> 
> Haven't checked it (haven't got the time right now) but:
> Is using wildcards in the BDF's on the commandline supported already
> (like the ones supported in the config files for domains)

Based on a quick scan of the code it doesn't appear so, I don't maintain
PCI backthough so there might be something in the pipeline.

> I tend to have quite long commandlines to hide a pci-e card with 8
> functions (needed to specify 8 BDF's seperatly) for pci passthrough,
> would be nice if one could just specify 09:00.* for example.

It sounds useful to me.

Re: [PATCH 3 of 4] libxl: Introduce pci_assignable_add and pci_assignable_remove

[George Dunlap]

On 10/05/12 12:19, Ian Campbell wrote:
> On Wed, 2012-05-09 at 11:28 +0100, George Dunlap wrote:
>> Introduce libxl helper functions to prepare devices to be passed through to
>> guests.  This is meant to replace of all the manual sysfs commands which
>> are currently required.
>>
>> pci_assignable_add accepts a BDF for a device and will:
>> * Unbind a device from its current driver, if any
>> * If "rebind" is set, it will store the path of the driver from which we
>>    unplugged it in /libxl/pciback/$BDF/driver_path
> Since you don't know whether the user is going to pass -r to
> assignable_remove why not always store this?
The xl command does in fact do this (i.e., always passes '1' here).  I 
considered just taking this option out, as you suggest,  but I thought 
it might be useful for the libxl implementation to have more flexibility.
>> * If necessary, create a slot for it in pciback
> I must confess I'm a bit fuzzy on the relationship between slots and
> bindings, where does the "if necessary" come into it?
>
> I was wondering while reading the patch if unconditionally adding a
> removing the slot might simplify a bunch of stuff, but I suspect I just
> don't appreciate some particular aspect of how pciback works...
I have no idea what the "slot" thing is for.  What I've determined by 
experimentation is:
* Before "echo $BDF > .../pciback/bind" will work, $BDF must be listed 
in pciback/slots
* The way to get $BDF listed in pciback/slots is "echo $BDF > 
pciback/new_slot"
* The above command is not idempotent; if you do it twice, you'll get 
two entries of $BDF in pciback/slots

I wasn't sure if having two slots would be a problem or not; so I did 
the conservative thing, and check for the existence of $BDF in 
pciback/slots first, only creating a new slot if there is not already an 
existing slot.

So "if necessary" means, "if the device doesn't already have a slot".

>> +    spath = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/driver",
>> +                           pcidev->domain,
>> +                           pcidev->bus,
>> +                           pcidev->dev,
>> +                           pcidev->func);
>> +    if ( !lstat(spath,&st) ) {
>> +        /* Find the canonical path to the driver. */
>> +        *dp = libxl__zalloc(gc, PATH_MAX);
> Should we be actually using fpathconf / sysconf here?
I don't really follow.  What exactly is it you're proposing?
>> +        *dp = realpath(spath, *dp);
>> +        if ( !*dp ) {
>> +            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "realpath() failed");
> Since errno is meaningful you want LIBXL__LOGERRNO here. Or the short
> form LOGE().
Done.
>
> Where you have pointer output params (like driver_path here) in general
> I think it is preferable to do everything with a local (single
> indirection) pointer and only update the output parameter on success.
> This means you avoid leaking/exposing a partial result on error but also
> means you can drop a lot of "*" from around the function.
>
> Maybe the gc makes this moot, but the "if ( driver_path )" stuff at the
> top of the fn took me several seconds to work out also ;-).
Yeah, that's a lot simpler, and easier to read.  Done.
>> +            return -1;
>> +        }
>> +
>> +        LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Driver re-plug path: %s",
>> +                   *dp);
>> +
>> +        /* Unbind from the old driver */
>> +        spath = libxl__sprintf(gc, "%s/unbind", *dp);
>> +        if ( sysfs_write_bdf(gc, spath, pcidev)<  0 ) {
>> +            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Couldn't unbind device");
> Not sure what errno is like here, worth printing it. Looking back at
> patch #1 I suspect sysfs_write_bdf should preserve errno over the call
> to close, so that suitable reporting can happen in the caller.
Done.
>> +/* Scan through /sys/.../pciback/slots looking for pcidev's BDF */
>> +static int pciback_dev_has_slot(libxl__gc *gc, libxl_device_pci *pcidev)
> Is the concept of "having a slot" distinct from being bound to pciback?
Technically, yes.  You can't be bound without a slot; but you can have a 
slot without being bound.  I don't know exactly what the "slot" 
functionality is for, and why it's a separate step, but that's the way 
it is at the moment.
>> +{
>> +    libxl_ctx *ctx = libxl__gc_owner(gc);
>> +    FILE *f;
>> +    int rc = 0;
>> +    unsigned bus, dev, func;
>> +
>> +    f = fopen(SYSFS_PCIBACK_DRIVER"/slots", "r");
>> +
>> +    if (f == NULL) {
>> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn't open %s",
>> +                         SYSFS_PCIBACK_DRIVER"/slots");
>> +        return ERROR_FAIL;
>> +    }
>> +
>> +    while(fscanf(f, "0000:%x:%x.%x\n",&bus,&dev,&func)==3) {
> Jan recently added support for PCI domains, does that have any impact on
> the hardcoded 0000 here? I suppose that would require PCI domains
> support in the dom0 kernel -- but that seems likely to already be there
> (or be imminent)
>
> I think the 0000 should be %x into domain compared with pcidev->domain.
Ah, right -- I don't think I knew anything about the whole PCI domains 
thing. Done.
>
>> +    if ( (rc=pciback_dev_has_slot(gc, pcidev))<  0 ) {
>> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
>> +                   "Error checking for pciback slot");
> Log errno?
>
> Also pciback_dev_has_slot itself always logs on error, you probably
> don't need both.
This way you get a sort of callback path; but I could take it out if you 
want.
>
>> +        return ERROR_FAIL;
>> +    } else if (rc == 0) {
>> +        if ( sysfs_write_bdf(gc, SYSFS_PCIBACK_DRIVER"/new_slot",
>> +                             pcidev)<  0 ) {
>> +            LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
>> +                       "Couldn't bind device to pciback!");
> ERRNO here too.
ack
>
>> +            return ERROR_FAIL;
>> +        }
>> +    }
>> +
>> +    if ( sysfs_write_bdf(gc, SYSFS_PCIBACK_DRIVER"/bind", pcidev)<  0 ) {
>> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Couldn't bind device to pciback!");
> ERRNO here too. Or since there are loads of these perhaps sysfs_write_bf
> should log on failure, either just the fact of the failed write to a
> path (which implies the  underlying failure was to bind/unbind) or you
> could add a "const char *what" param to use in logging.
Just doing ERRNO for all the callers makes more sense to me.
>> +    /* Remove slot if necessary */
>> +    if ( pciback_dev_has_slot(gc, pcidev)>  0 ) {
>> +        if ( sysfs_write_bdf(gc, SYSFS_PCIBACK_DRIVER"/remove_slot",
>> +                             pcidev)<  0 ) {
>> +            LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
>> +                       "Couldn't remove pciback slot");
> ERRNO
ack
>
>> +            return ERROR_FAIL;
>> +        }
>> +    }
>> +    return 0;
>> +}
>> +
>> +/* FIXME */
> Leftover?
Yeah, noticed this just after I sent it. :-)
>> +retry:
>> +    t = xs_transaction_start(ctx->xsh);
>> +
>> +    path = libxl__sprintf(gc, PCIBACK_INFO_PATH"/"PCI_BDF_XSPATH"/driver_path",
>> +                          pcidev->domain,
>> +                          pcidev->bus,
>> +                          pcidev->dev,
>> +                          pcidev->func);
>> +    xs_rm(ctx->xsh, t, path);
> Why do you need to rm first? Won't the write just replace whatever it is
> (and that means the need for a transaction goes away too)
>
> In any case you should create path outside the retry loop instead of
> needlessly recreating it each time around.
TBH, I just looked at another bit of code that did xs transactions and 
tried to follow suit.  Since I only need one operation, I'll take out 
the transaction stuff.
>> +    xs_rm(ctx->xsh, t, path);
> You don't need a transaction for a single operation. (and if you did
> then "path = ..." could have been hoisted out)
Very well.
>
>> +int libxl_device_pci_assignable_add(libxl_ctx *ctx, libxl_device_pci *pcidev,
>> +                                    int rebind)
>> +{
>> +    GC_INIT(ctx);
>> +    int rc;
>> +
>> +    rc = libxl__device_pci_assignable_add(gc, pcidev, rebind);
>> +
>> +    GC_FREE;
>> +    return rc;
>> +}
> Are there internal callers of libxl__device_pci_assignable_add/remove?
> If not then there's no reason to split into internal/external functions.
> Doesn't matter much I guess.
Not yet, but I don't think it hurts to have that flexibility.

Thanks for the detailed review.

  -George

Re: [PATCH 3 of 4] libxl: Introduce pci_assignable_add and pci_assignable_remove

[Ian Campbell]

On Thu, 2012-05-10 at 15:55 +0100, George Dunlap wrote:
> On 10/05/12 12:19, Ian Campbell wrote:
> > On Wed, 2012-05-09 at 11:28 +0100, George Dunlap wrote:
> >> Introduce libxl helper functions to prepare devices to be passed through to
> >> guests.  This is meant to replace of all the manual sysfs commands which
> >> are currently required.
> >>
> >> pci_assignable_add accepts a BDF for a device and will:
> >> * Unbind a device from its current driver, if any
> >> * If "rebind" is set, it will store the path of the driver from which we
> >>    unplugged it in /libxl/pciback/$BDF/driver_path
> > Since you don't know whether the user is going to pass -r to
> > assignable_remove why not always store this?
> The xl command does in fact do this (i.e., always passes '1' here).  I 
> considered just taking this option out, as you suggest,  but I thought 
> it might be useful for the libxl implementation to have more flexibility.

Oh, I see, I lost track of this being a libxl patch. That seems fine.

> >> * If necessary, create a slot for it in pciback
> > I must confess I'm a bit fuzzy on the relationship between slots and
> > bindings, where does the "if necessary" come into it?
> >
> > I was wondering while reading the patch if unconditionally adding a
> > removing the slot might simplify a bunch of stuff, but I suspect I just
> > don't appreciate some particular aspect of how pciback works...
> I have no idea what the "slot" thing is for.  What I've determined by 
> experimentation is:
> * Before "echo $BDF > .../pciback/bind" will work, $BDF must be listed 
> in pciback/slots
> * The way to get $BDF listed in pciback/slots is "echo $BDF > 
> pciback/new_slot"
> * The above command is not idempotent; if you do it twice, you'll get 
> two entries of $BDF in pciback/slots
> 
> I wasn't sure if having two slots would be a problem or not; so I did 
> the conservative thing, and check for the existence of $BDF in 
> pciback/slots first, only creating a new slot if there is not already an 
> existing slot.
> 
> So "if necessary" means, "if the device doesn't already have a slot".

OK, so it looks like the stuff to check all this is in fact necessary.

> 
> >> +    spath = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/driver",
> >> +                           pcidev->domain,
> >> +                           pcidev->bus,
> >> +                           pcidev->dev,
> >> +                           pcidev->func);
> >> +    if ( !lstat(spath,&st) ) {
> >> +        /* Find the canonical path to the driver. */
> >> +        *dp = libxl__zalloc(gc, PATH_MAX);
> > Should we be actually using fpathconf / sysconf here?
> I don't really follow.  What exactly is it you're proposing?

PATH_MAX isn't really a constant these days, you can get the dynamic
value for a particular filesystem from fpathconf. I honestly don't know
how much of a concern this really is, especially given we are always
necessarily talking to sysfs.

> >> +    if ( (rc=pciback_dev_has_slot(gc, pcidev))<  0 ) {
> >> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> >> +                   "Error checking for pciback slot");
> > Log errno?
> >
> > Also pciback_dev_has_slot itself always logs on error, you probably
> > don't need both.
> This way you get a sort of callback path; but I could take it out if you 
> want.

I don't feel especially strongly, up to you (/knocks ball back over
net ;-) )

> >
> >> +            return ERROR_FAIL;
> >> +        }
> >> +    }
> >> +    return 0;
> >> +}
> >> +
> >> +/* FIXME */
> > Leftover?
> Yeah, noticed this just after I sent it. :-)

That's always the way...

> >> +int libxl_device_pci_assignable_add(libxl_ctx *ctx, libxl_device_pci *pcidev,
> >> +                                    int rebind)
> >> +{
> >> +    GC_INIT(ctx);
> >> +    int rc;
> >> +
> >> +    rc = libxl__device_pci_assignable_add(gc, pcidev, rebind);
> >> +
> >> +    GC_FREE;
> >> +    return rc;
> >> +}
> > Are there internal callers of libxl__device_pci_assignable_add/remove?
> > If not then there's no reason to split into internal/external functions.
> > Doesn't matter much I guess.
> Not yet, but I don't think it hurts to have that flexibility.

Sure.

> Thanks for the detailed review.

No problem. BTW, rather than writing done/ack dozens of times I normally
assume agreement if someone just trims the whole section.

Ian.

Re: [PATCH 0 of 4] Add commands to automatically prep devices for pass-through

[Konrad Rzeszutek Wilk]

> > > Fully hiding the device from dom0 drivers generally seems like it is
> > > always better. That way the first driver to try and touch the hardware
> > > is the one inside the domU. This avoids issues with dom0 drivers setting
> > > stuff up but not tearing it down in a way that domU can cope with and
> > > makes the use of hardware which doesn't support FLR more reliable etc.
> > 
> > Haven't checked it (haven't got the time right now) but:
> > Is using wildcards in the BDF's on the commandline supported already
> > (like the ones supported in the config files for domains)
> 
> Based on a quick scan of the code it doesn't appear so, I don't maintain
> PCI backthough so there might be something in the pipeline.
> 
> > I tend to have quite long commandlines to hide a pci-e card with 8
> > functions (needed to specify 8 BDF's seperatly) for pci passthrough,
> > would be nice if one could just specify 09:00.* for example.
> 
> It sounds useful to me.

Patches are most welcome :-)

Re: [PATCH 3 of 4] libxl: Introduce pci_assignable_add and pci_assignable_remove

[George Dunlap]

On 10/05/12 16:04, Ian Campbell wrote:
>>>> +    spath = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/driver",
>>>> +                           pcidev->domain,
>>>> +                           pcidev->bus,
>>>> +                           pcidev->dev,
>>>> +                           pcidev->func);
>>>> +    if ( !lstat(spath,&st) ) {
>>>> +        /* Find the canonical path to the driver. */
>>>> +        *dp = libxl__zalloc(gc, PATH_MAX);
>>> Should we be actually using fpathconf / sysconf here?
>> I don't really follow.  What exactly is it you're proposing?
> PATH_MAX isn't really a constant these days, you can get the dynamic
> value for a particular filesystem from fpathconf. I honestly don't know
> how much of a concern this really is, especially given we are always
> necessarily talking to sysfs.
Ah right -- I didn't get that you were referring to PATH_MAX.  The 
"realpath" manpage specifies:   "The resulting path‐name is stored as a 
null-terminated string, up to a maximum of PATH_MAX bytes, in the buffer 
pointed to by resolved_path."  That's why I used PATH_MAX in the 
allocation.  I would hope that if the manpage says PATH_MAX, it means 
PATH_MAX, and not "some other thing which you can get by running this 
complicated command I haven't mentioned". :-)

OK -- I've also added a comment explaining why I'm doing what I'm doing 
with slots, which I'll include when I re-post the patch.

  -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3 of 4] libxl: Introduce pci_assignable_add and pci_assignable_remove

[Ian Campbell]

On Thu, 2012-05-10 at 17:29 +0100, George Dunlap wrote:
> On 10/05/12 16:04, Ian Campbell wrote:
> >>>> +    spath = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/driver",
> >>>> +                           pcidev->domain,
> >>>> +                           pcidev->bus,
> >>>> +                           pcidev->dev,
> >>>> +                           pcidev->func);
> >>>> +    if ( !lstat(spath,&st) ) {
> >>>> +        /* Find the canonical path to the driver. */
> >>>> +        *dp = libxl__zalloc(gc, PATH_MAX);
> >>> Should we be actually using fpathconf / sysconf here?
> >> I don't really follow.  What exactly is it you're proposing?
> > PATH_MAX isn't really a constant these days, you can get the dynamic
> > value for a particular filesystem from fpathconf. I honestly don't know
> > how much of a concern this really is, especially given we are always
> > necessarily talking to sysfs.
> Ah right -- I didn't get that you were referring to PATH_MAX.  The 
> "realpath" manpage specifies:   "The resulting path‐name is stored as a 
> null-terminated string, up to a maximum of PATH_MAX bytes, in the buffer 
> pointed to by resolved_path."  That's why I used PATH_MAX in the 
> allocation.  I would hope that if the manpage says PATH_MAX, it means 
> PATH_MAX, and not "some other thing which you can get by running this 
> complicated command I haven't mentioned". :-)

Yes, that's fair ;-)

I think the issue might be that some systems declare PATH_MAX to be
enormous (to cover all bases) and sysconf lets you use something more
realistic/relevant to the actual situation.

Looks like on Linux it is 4096, which I guess is ok.

> OK -- I've also added a comment explaining why I'm doing what I'm doing 
> with slots, which I'll include when I re-post the patch.

Ta!

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 4 of 4] xl: Add pci_assignable_add and remove commands

[George Dunlap]

>> +
>> +static void pciassignable_remove(const char *bdf, int rebind)
>> +{
>> +    libxl_device_pci pcidev;
>> +    XLU_Config *config;
>> +
>> +    memset(&pcidev, 0x00, sizeof(pcidev));
> libxl_device_pci_init also.
>
> You are also missing the xlu_cfg_destroy both here and in the add fn.
>
> (it's a bit annoying that an XLU_COnfig is needed for these simple
> helper functions, I suppose it's for logging, but maybe they could log
> to stderr if cfg==NULL. Anyway, lets not fix that here)
Hmm -- I just copied and pasted from pciattach() and friends.  I'll fix 
those up while I'm at it.

  -George

Re: [PATCH 4 of 4] xl: Add pci_assignable_add and remove commands

[Ian Campbell]

On Fri, 2012-05-11 at 12:13 +0100, George Dunlap wrote:
> >> +
> >> +static void pciassignable_remove(const char *bdf, int rebind)
> >> +{
> >> +    libxl_device_pci pcidev;
> >> +    XLU_Config *config;
> >> +
> >> +    memset(&pcidev, 0x00, sizeof(pcidev));
> > libxl_device_pci_init also.
> >
> > You are also missing the xlu_cfg_destroy both here and in the add fn.
> >
> > (it's a bit annoying that an XLU_COnfig is needed for these simple
> > helper functions, I suppose it's for logging, but maybe they could log
> > to stderr if cfg==NULL. Anyway, lets not fix that here)
> Hmm -- I just copied and pasted from pciattach() and friends.  I'll fix 
> those up while I'm at it.

Thanks, looked like my grep missed them somehow when I added the init
fns. The reset of the memset's in xl_cmdimpl.c look ok except for:

    memset(cpumap.map, 0, cpumap.size);

in main_cpupoolnumasplit which smells a bit fishy -- I'll investigate
that one...

Ian.

Re: [PATCH 4 of 4] xl: Add pci_assignable_add and remove commands

[George Dunlap]

On 11/05/12 12:19, Ian Campbell wrote:
> On Fri, 2012-05-11 at 12:13 +0100, George Dunlap wrote:
>>>> +
>>>> +static void pciassignable_remove(const char *bdf, int rebind)
>>>> +{
>>>> +    libxl_device_pci pcidev;
>>>> +    XLU_Config *config;
>>>> +
>>>> +    memset(&pcidev, 0x00, sizeof(pcidev));
>>> libxl_device_pci_init also.
>>>
>>> You are also missing the xlu_cfg_destroy both here and in the add fn.
>>>
>>> (it's a bit annoying that an XLU_COnfig is needed for these simple
>>> helper functions, I suppose it's for logging, but maybe they could log
>>> to stderr if cfg==NULL. Anyway, lets not fix that here)
>> Hmm -- I just copied and pasted from pciattach() and friends.  I'll fix
>> those up while I'm at it.
> Thanks, looked like my grep missed them somehow when I added the init
> fns. The reset of the memset's in xl_cmdimpl.c look ok except for:
>
>      memset(cpumap.map, 0, cpumap.size);
>
> in main_cpupoolnumasplit which smells a bit fishy -- I'll investigate
> that one...
OK -- I found another place xl_cfg_destroy() wasn't being called.  I 
think I'll send those as a separate clean-up series; but the new 
functions added in the series will have the fixes you suggested.

  -George

Re: [PATCH 4 of 4] xl: Add pci_assignable_add and remove commands

[Ian Campbell]

On Fri, 2012-05-11 at 13:50 +0100, George Dunlap wrote:
> On 11/05/12 12:19, Ian Campbell wrote:
> > On Fri, 2012-05-11 at 12:13 +0100, George Dunlap wrote:
> >>>> +
> >>>> +static void pciassignable_remove(const char *bdf, int rebind)
> >>>> +{
> >>>> +    libxl_device_pci pcidev;
> >>>> +    XLU_Config *config;
> >>>> +
> >>>> +    memset(&pcidev, 0x00, sizeof(pcidev));
> >>> libxl_device_pci_init also.
> >>>
> >>> You are also missing the xlu_cfg_destroy both here and in the add fn.
> >>>
> >>> (it's a bit annoying that an XLU_COnfig is needed for these simple
> >>> helper functions, I suppose it's for logging, but maybe they could log
> >>> to stderr if cfg==NULL. Anyway, lets not fix that here)
> >> Hmm -- I just copied and pasted from pciattach() and friends.  I'll fix
> >> those up while I'm at it.
> > Thanks, looked like my grep missed them somehow when I added the init
> > fns. The reset of the memset's in xl_cmdimpl.c look ok except for:
> >
> >      memset(cpumap.map, 0, cpumap.size);
> >
> > in main_cpupoolnumasplit which smells a bit fishy -- I'll investigate
> > that one...
> OK -- I found another place xl_cfg_destroy() wasn't being called.  I 
> think I'll send those as a separate clean-up series; but the new 
> functions added in the series will have the fixes you suggested.

THanks!