thr3ads.net - Xen devel - [PATCH 0 of 4] Add commands to automatically prep devices for pass-through [May 2012]

If this information is useful, please help other people find it:
Share via:

George Dunlap

2012-May-09 10:28 UTC

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

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>

George Dunlap

2012-May-09 10:28 UTC

head link

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

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;
     }

George Dunlap

2012-May-09 10:28 UTC

head link

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

...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 }
 };

George Dunlap

2012-May-09 10:28 UTC

head link

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

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

George Dunlap

2012-May-09 10:28 UTC

head link

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

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",

Ian Campbell

2012-May-09 10:49 UTC

head link

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

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

David Vrabel

2012-May-09 10:56 UTC

head link

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

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

George Dunlap

2012-May-09 11:03 UTC

head link

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

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
>

George Dunlap

2012-May-09 11:11 UTC

head link

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

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

Ian Campbell

2012-May-09 11:59 UTC

head link

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

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
> >
>

George Dunlap

2012-May-09 13:45 UTC

head link

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

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

George Dunlap

2012-May-10 10:17 UTC

head link

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

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

Ian Campbell

2012-May-10 10:38 UTC

head link

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

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.

Ian Campbell

2012-May-10 10:40 UTC

head link

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

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

Ian Campbell

2012-May-10 10:43 UTC

head link

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

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

George Dunlap

2012-May-10 10:54 UTC

head link

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

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

Ian Campbell

2012-May-10 11:19 UTC

head link

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

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

Ian Campbell

2012-May-10 11:31 UTC

head link

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

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

Sander Eikelenboom

2012-May-10 14:12 UTC

head link

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

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

Ian Campbell

2012-May-10 14:16 UTC

head link

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

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.

George Dunlap

2012-May-10 14:55 UTC

head link

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

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

Ian Campbell

2012-May-10 15:04 UTC

head link

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

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.

Konrad Rzeszutek Wilk

2012-May-10 16:15 UTC

head link

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

> > > 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 :-)

George Dunlap

2012-May-10 16:29 UTC

head link

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

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

Ian Campbell

2012-May-10 16:45 UTC

head link

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

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

George Dunlap

2012-May-11 11:13 UTC

head link

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

>> +
>> +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

Ian Campbell

2012-May-11 11:19 UTC

head link

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

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.

George Dunlap

2012-May-11 12:50 UTC

head link

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

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

Ian Campbell

2012-May-11 12:58 UTC

head link

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

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!

Xen devel - May 2012 - [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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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