Add commands to automatically prep devices for pass-through

This functionality will be used several times in subsequent patches.

Signed-off-by George Dunlap <[email protected]>

diff -r d5268710a5ca -r 0772f1d07d1c tools/libxl/libxl_pci.c

tools/libxl/libxl_pci.c +34 -19

@@ -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
[email protected]
http://lists.xen.org/xen-devel

...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 <[email protected]>

diff -r 0772f1d07d1c -r 5b5070d487d9 docs/man/xl.pod.1

docs/man/xl.pod.1 +7 -7

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

tools/libxl/libxl.h +1 -1

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

tools/libxl/libxl_pci.c +2 -2

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

tools/libxl/xl.h +1 -1

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

tools/libxl/xl_cmdimpl.c +28 -28

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

tools/libxl/xl_cmdtable.c +2 -2

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

tools/python/xen/lowlevel/xl/xl.c +4 -4

@@ -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
[email protected]
http://lists.xen.org/xen-devel

Acked-by Ian Campbell<[email protected]>

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

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 <[email protected]>

diff -r 5b5070d487d9 -r 17a5b562d1e7 tools/libxl/libxl.h

tools/libxl/libxl.h Wed May 09 11:21:28 2012 +0100 renamed from tools/libxl/libxl.h Wed May 09 11:21:19 2012 +0100 +18 -3

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

tools/libxl/libxl_pci.c Wed May 09 11:21:28 2012 +0100 renamed from tools/libxl/libxl_pci.c Wed May 09 11:21:19 2012 +0100 +12 -0

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

tools/libxl/libxl_pci.c Wed May 09 11:21:28 2012 +0100 renamed from tools/libxl/libxl_pci.c Wed May 09 11:21:19 2012 +0100 +5 -0

+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    FILE *f;
+    int rc = 0;
+    unsigned bus, dev, func;

tools/libxl/libxl_pci.c Wed May 09 11:21:28 2012 +0100 renamed from tools/libxl/libxl_pci.c Wed May 09 11:21:19 2012 +0100 +30 -0

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

tools/libxl/libxl_pci.c Wed May 09 11:21:28 2012 +0100 renamed from tools/libxl/libxl_pci.c Wed May 09 11:21:19 2012 +0100 +11 -0

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

tools/libxl/libxl_pci.c Wed May 09 11:21:28 2012 +0100 renamed from tools/libxl/libxl_pci.c Wed May 09 11:21:19 2012 +0100 +13 -0

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

tools/libxl/libxl_pci.c Wed May 09 11:21:28 2012 +0100 renamed from tools/libxl/libxl_pci.c Wed May 09 11:21:19 2012 +0100 +35 -0

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

tools/libxl/libxl_pci.c Wed May 09 11:21:28 2012 +0100 renamed from tools/libxl/libxl_pci.c Wed May 09 11:21:19 2012 +0100 +112 -0

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

tools/libxl/libxl_pci.c Wed May 09 11:21:28 2012 +0100 renamed from tools/libxl/libxl_pci.c Wed May 09 11:21:19 2012 +0100 +14 -0

+
+
+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
[email protected]
http://lists.xen.org/xen-devel

Quoted 8 lines · George Dunlap

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

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

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

Quoted 1 line

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

tools/libxl/libxl_pci.c out: · line 432

Quoted 8 lines

tools/libxl/libxl_pci.c +8 -0

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

tools/libxl/libxl_pci.c out: · line 435

Quoted 3 lines

tools/libxl/libxl_pci.c +3 -0

+        *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 ;-).

Quoted 5 lines

tools/libxl/libxl_pci.c +5 -0

+            return -1;
+        }
+
+        LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Driver re-plug path: %s",
+                   *dp);

tools/libxl/libxl_pci.c out: · line 445

Quoted 5 lines

tools/libxl/libxl_pci.c +5 -0

+
+        /* 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.

tools/libxl/libxl_pci.c out: · line 453

Quoted 2 lines

tools/libxl/libxl_pci.c +2 -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?

Quoted 5 lines

tools/libxl/libxl_pci.c +5 -0

+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    FILE *f;
+    int rc = 0;
+    unsigned bus, dev, func;

tools/libxl/libxl_pci.c out:

Quoted 10 lines

tools/libxl/libxl_pci.c +10 -0

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

tools/libxl/libxl_pci.c out: · line 508

Quoted 4 lines

tools/libxl/libxl_pci.c +3 -0

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

tools/libxl/libxl_pci.c out: · line 514

Quoted 7 lines

tools/libxl/libxl_pci.c +6 -0

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

tools/libxl/libxl_pci.c out: · line 520

Quoted 7 lines

tools/libxl/libxl_pci.c +6 -0

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

tools/libxl/libxl_pci.c out: · line 541

Quoted 6 lines

tools/libxl/libxl_pci.c +6 -0

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

tools/libxl/libxl_pci.c out: · line 548

Quoted 8 lines

tools/libxl/libxl_pci.c +7 -0

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

Leftover?

tools/libxl/libxl_pci.c out: · line 571

Quoted 9 lines

tools/libxl/libxl_pci.c +9 -0

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

Quoted 1 line

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

tools/libxl/libxl_pci.c out: · line 738

Quoted 12 lines

tools/libxl/libxl_pci.c +11 -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.

Quoted 9 lines · Ian Campbell

Quoted 8 lines · George Dunlap

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

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

Quoted 2 lines

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.

Quoted 7 lines

Quoted 1 line

* 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".

Quoted 10 lines

tools/libxl/libxl_pci.c out: · line 432

Quoted 9 lines

tools/libxl/libxl_pci.c +8 -0

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

Quoted 7 lines

tools/libxl/libxl_pci.c out: · line 508

Quoted 3 lines

tools/libxl/libxl_pci.c +3 -0

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

Quoted 9 lines

tools/libxl/libxl_pci.c out: · line 548

Quoted 8 lines

tools/libxl/libxl_pci.c +7 -0

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

Leftover?

Yeah, noticed this just after I sent it. :-)

Quoted 6 lines

Quoted 6 lines

tools/libxl/libxl_pci.c +6 -0

+int libxl_device_pci_assignable_add(libxl_ctx *ctx, libxl_device_pci *pcidev,
+                                    int rebind)
+{
+    GC_INIT(ctx);
+    int rc;
+

Quoted 8 lines

tools/libxl/libxl_pci.c out: · line 738

Quoted 5 lines

tools/libxl/libxl_pci.c +5 -0

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

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

tools/libxl/libxl_pci.c +8 -0

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

Quoted 2 lines

Quoted 1 line

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.

Quoted 8 lines · Ian Campbell

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

tools/libxl/libxl_pci.c +8 -0

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

Quoted 6 lines

Quoted 2 lines

Quoted 1 line

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.

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 <[email protected]>

diff -r 17a5b562d1e7 -r a1c357853735 docs/man/xl.pod.1

docs/man/xl.pod.1 +20 -0

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

tools/libxl/xl.h +2 -0

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

tools/libxl/xl_cmdimpl.c +26 -0

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

tools/libxl/xl_cmdimpl.c +34 -0

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

tools/libxl/xl_cmdtable.c +14 -0

@@ -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
[email protected]
http://lists.xen.org/xen-devel

tools/libxl/xl_cmdimpl.c int main_pciassignable_list(int argc, ch · line 2412

Quoted 8 lines

tools/libxl/xl_cmdimpl.c +7 -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)

Quoted 9 lines

tools/libxl/xl_cmdimpl.c int main_pciassignable_list(int argc, ch · line 2412

Quoted 7 lines

tools/libxl/xl_cmdimpl.c +7 -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.

Quoted 5 lines

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.

Quoted 9 lines · George Dunlap

Quoted 9 lines

tools/libxl/xl_cmdimpl.c int main_pciassignable_list(int argc, ch · line 2412

Quoted 7 lines

tools/libxl/xl_cmdimpl.c +7 -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.

Quoted 7 lines

Quoted 5 lines

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

Quoted 20 lines · Ian Campbell

Quoted 9 lines · George Dunlap

Quoted 9 lines

tools/libxl/xl_cmdimpl.c int main_pciassignable_list(int argc, ch · line 2412

Quoted 7 lines

tools/libxl/xl_cmdimpl.c +7 -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.

Quoted 7 lines

Quoted 5 lines

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:

Quoted 4 lines

     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.

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 <[email protected]>

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 5 lines · George Dunlap

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

This set of patches introduces commands to make this easier.

Is this intended for 4.2 or an RFC for 4.3?

Quoted 24 lines

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<[email protected]>

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Quoted 7 lines · Ian Campbell

Quoted 5 lines · George Dunlap

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

This set of patches introduces commands to make this easier.

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

Quoted 25 lines

Quoted 24 lines

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<[email protected]>

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

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.

Quoted 6 lines · Ian Campbell

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

Quoted 6 lines

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.

Quoted 7 lines · George Dunlap

Quoted 6 lines · Ian Campbell

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

Quoted 6 lines

Quoted 6 lines

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.

Quoted 16 lines · George Dunlap

Quoted 7 lines · George Dunlap

Quoted 6 lines · Ian Campbell

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

Quoted 6 lines

Quoted 6 lines

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.

Quoted 9 lines

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.

Hello Ian,

Quoted 35 lines · you

Quoted 16 lines · George Dunlap

Quoted 7 lines · George Dunlap

Quoted 6 lines · Ian Campbell

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

Quoted 6 lines

Quoted 6 lines

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.

Quoted 9 lines

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.

Quoted 5 lines

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.

Quoted 9 lines

Quoted 5 lines

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.

Quoted 3 lines

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.

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