[PATCH 0 of 2] [v2] Add per-device and global permissive config options for pci passthrough

[PATCH 0 of 2] [v2] Add per-device and global permissive config options for pci passthrough

[George Dunlap]

This patch series adds the "permissive" option for passed-through devices
which access config space out of the "known good" PCI config space.

v2:
- Added patch to move bdf parsing to libxlu
- Addressed some formatting comments

[PATCH 1 of 2] libxl: Move bdf parsing into libxlu

[George Dunlap]

# HG changeset patch
# User George Dunlap <george.dunlap@eu.citrix.com>
# Date 1333362574 -3600
# Node ID 5386937e6c5c9afaa8a3cd56d391dcc9e40d0596
# Parent  f744e82ea74075983de6d5b0ad0cf7ccacf999a2
libxl: Move bdf parsing into libxlu

Config parsing functions do not properly belong in libxl.  Move them into
libxlu so that others can use them or not as they see fit.

No functional changes.  One side-effect was making public a private libxl
utility function which just set the elements of a structure from the  function
arguments passed in.

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

diff -r f744e82ea740 -r 5386937e6c5c tools/libxl/Makefile
--- a/tools/libxl/Makefile	Wed Feb 29 16:30:34 2012 +0000
+++ b/tools/libxl/Makefile	Mon Apr 02 11:29:34 2012 +0100
@@ -57,7 +57,7 @@ LIBXL_OBJS += _libxl_types.o libxl_flask
 AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h
 AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c
 LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
-	libxlu_disk_l.o libxlu_disk.o
+	libxlu_disk_l.o libxlu_disk.o libxlu_pci.o
 $(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h
 
 CLIENTS = xl testidl
diff -r f744e82ea740 -r 5386937e6c5c tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Wed Feb 29 16:30:34 2012 +0000
+++ b/tools/libxl/libxl.h	Mon Apr 02 11:29:34 2012 +0100
@@ -573,13 +573,10 @@ int libxl_device_pci_add(libxl_ctx *ctx,
 int libxl_device_pci_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev);
 int libxl_device_pci_destroy(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev);
 libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid, int *num);
-
-/*
- * Parse a PCI BDF into a PCI device structure.
- */
-int libxl_device_pci_parse_bdf(libxl_ctx *ctx,
-                               libxl_device_pci *pcidev,
-                               const char *str);
+/* Just initialize the structure elements with the arguments provided. */
+int libxl_pci_dev_init(libxl_device_pci *pcidev, unsigned int domain,
+                       unsigned int bus, unsigned int dev,
+                       unsigned int func, unsigned int vdevfn);
 
 /*
  * Similar to libxl_device_pci_list but returns all devices which
diff -r f744e82ea740 -r 5386937e6c5c tools/libxl/libxl_pci.c
--- a/tools/libxl/libxl_pci.c	Wed Feb 29 16:30:34 2012 +0000
+++ b/tools/libxl/libxl_pci.c	Mon Apr 02 11:29:34 2012 +0100
@@ -34,7 +34,7 @@ static unsigned int pcidev_encode_bdf(li
     return value;
 }
 
-static int pcidev_init(libxl_device_pci *pcidev, unsigned int domain,
+int libxl_pci_dev_init(libxl_device_pci *pcidev, unsigned int domain,
                           unsigned int bus, unsigned int dev,
                           unsigned int func, unsigned int vdevfn)
 {
@@ -46,149 +46,6 @@ static int pcidev_init(libxl_device_pci 
     return 0;
 }
 
-static int hex_convert(const char *str, unsigned int *val, unsigned int mask)
-{
-    unsigned long ret;
-    char *end;
-
-    ret = strtoul(str, &end, 16);
-    if ( end == str || *end != '\0' )
-        return -1;
-    if ( ret & ~mask )
-        return -1;
-    *val = (unsigned int)ret & mask;
-    return 0;
-}
-
-#define STATE_DOMAIN    0
-#define STATE_BUS       1
-#define STATE_DEV       2
-#define STATE_FUNC      3
-#define STATE_VSLOT     4
-#define STATE_OPTIONS_K 6
-#define STATE_OPTIONS_V 7
-#define STATE_TERMINAL  8
-int libxl_device_pci_parse_bdf(libxl_ctx *ctx, libxl_device_pci *pcidev, const char *str)
-{
-    unsigned state = STATE_DOMAIN;
-    unsigned dom, bus, dev, func, vslot = 0;
-    char *buf2, *tok, *ptr, *end, *optkey = NULL;
-
-    if ( NULL == (buf2 = ptr = strdup(str)) )
-        return ERROR_NOMEM;
-
-    for(tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++) {
-        switch(state) {
-        case STATE_DOMAIN:
-            if ( *ptr == ':' ) {
-                state = STATE_BUS;
-                *ptr = '\0';
-                if ( hex_convert(tok, &dom, 0xffff) )
-                    goto parse_error;
-                tok = ptr + 1;
-            }
-            break;
-        case STATE_BUS:
-            if ( *ptr == ':' ) {
-                state = STATE_DEV;
-                *ptr = '\0';
-                if ( hex_convert(tok, &bus, 0xff) )
-                    goto parse_error;
-                tok = ptr + 1;
-            }else if ( *ptr == '.' ) {
-                state = STATE_FUNC;
-                *ptr = '\0';
-                if ( dom & ~0xff )
-                    goto parse_error;
-                bus = dom;
-                dom = 0;
-                if ( hex_convert(tok, &dev, 0xff) )
-                    goto parse_error;
-                tok = ptr + 1;
-            }
-            break;
-        case STATE_DEV:
-            if ( *ptr == '.' ) {
-                state = STATE_FUNC;
-                *ptr = '\0';
-                if ( hex_convert(tok, &dev, 0xff) )
-                    goto parse_error;
-                tok = ptr + 1;
-            }
-            break;
-        case STATE_FUNC:
-            if ( *ptr == '\0' || *ptr == '@' || *ptr == ',' ) {
-                switch( *ptr ) {
-                case '\0':
-                    state = STATE_TERMINAL;
-                    break;
-                case '@':
-                    state = STATE_VSLOT;
-                    break;
-                case ',':
-                    state = STATE_OPTIONS_K;
-                    break;
-                }
-                *ptr = '\0';
-                if ( !strcmp(tok, "*") ) {
-                    pcidev->vfunc_mask = LIBXL_PCI_FUNC_ALL;
-                }else{
-                    if ( hex_convert(tok, &func, 0x7) )
-                        goto parse_error;
-                    pcidev->vfunc_mask = (1 << 0);
-                }
-                tok = ptr + 1;
-            }
-            break;
-        case STATE_VSLOT:
-            if ( *ptr == '\0' || *ptr == ',' ) {
-                state = ( *ptr == ',' ) ? STATE_OPTIONS_K : STATE_TERMINAL;
-                *ptr = '\0';
-                if ( hex_convert(tok, &vslot, 0xff) )
-                    goto parse_error;
-                tok = ptr + 1;
-            }
-            break;
-        case STATE_OPTIONS_K:
-            if ( *ptr == '=' ) {
-                state = STATE_OPTIONS_V;
-                *ptr = '\0';
-                optkey = tok;
-                tok = ptr + 1;
-            }
-            break;
-        case STATE_OPTIONS_V:
-            if ( *ptr == ',' || *ptr == '\0' ) {
-                state = (*ptr == ',') ? STATE_OPTIONS_K : STATE_TERMINAL;
-                *ptr = '\0';
-                if ( !strcmp(optkey, "msitranslate") ) {
-                    pcidev->msitranslate = atoi(tok);
-                }else if ( !strcmp(optkey, "power_mgmt") ) {
-                    pcidev->power_mgmt = atoi(tok);
-                }else{
-                    LIBXL__LOG(ctx, LIBXL__LOG_WARNING,
-                           "Unknown PCI BDF option: %s", optkey);
-                }
-                tok = ptr + 1;
-            }
-        default:
-            break;
-        }
-    }
-
-    free(buf2);
-
-    if ( tok != ptr || state != STATE_TERMINAL )
-        goto parse_error;
-
-    pcidev_init(pcidev, dom, bus, dev, func, vslot << 3);
-
-    return 0;
-
-parse_error:
-    return ERROR_INVAL;
-}
-
 static void libxl_create_pci_backend_device(libxl__gc *gc, flexarray_t *back, int num, libxl_device_pci *pcidev)
 {
     flexarray_append(back, libxl__sprintf(gc, "key-%d", num));
@@ -436,7 +293,7 @@ static int get_all_assigned_devices(libx
                     *list = realloc(*list, sizeof(libxl_device_pci) * ((*num) + 1));
                     if (*list == NULL)
                         return ERROR_NOMEM;
-                    pcidev_init(*list + *num, dom, bus, dev, func, 0);
+                    libxl_pci_dev_init(*list + *num, dom, bus, dev, func, 0);
                     (*num)++;
                 }
             }
@@ -507,7 +364,7 @@ libxl_device_pci *libxl_device_pci_list_
         new = pcidevs + *num;
 
         memset(new, 0, sizeof(*new));
-        pcidev_init(new, dom, bus, dev, func, 0);
+        libxl_pci_dev_init(new, dom, bus, dev, func, 0);
         (*num)++;
     }
 
@@ -1086,7 +943,7 @@ static void libxl__device_pci_from_xs_be
     if (s)
         vdevfn = strtol(s, (char **) NULL, 16);
 
-    pcidev_init(pci, domain, bus, dev, func, vdevfn);
+    libxl_pci_dev_init(pci, domain, bus, dev, func, vdevfn);
 
     s = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/opts-%d", be_path, nr));
     if (s) {
diff -r f744e82ea740 -r 5386937e6c5c tools/libxl/libxlu_pci.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/libxl/libxlu_pci.c	Mon Apr 02 11:29:34 2012 +0100
@@ -0,0 +1,160 @@
+#include "libxl_osdeps.h" /* must come before any other headers */
+#include "libxlu_internal.h"
+#include "libxlu_disk_l.h"
+#include "libxlu_disk_i.h"
+#include "libxlu_cfg_i.h"
+
+
+#define XLU__PCI_ERR(_c, _x, _a...) \
+    if((_c) && (_c)->report) fprintf((_c)->report, _x, ##_a)
+
+static int hex_convert(const char *str, unsigned int *val, unsigned int mask)
+{
+    unsigned long ret;
+    char *end;
+
+    ret = strtoul(str, &end, 16);
+    if ( end == str || *end != '\0' )
+        return -1;
+    if ( ret & ~mask )
+        return -1;
+    *val = (unsigned int)ret & mask;
+    return 0;
+}
+
+#define STATE_DOMAIN    0
+#define STATE_BUS       1
+#define STATE_DEV       2
+#define STATE_FUNC      3
+#define STATE_VSLOT     4
+#define STATE_OPTIONS_K 6
+#define STATE_OPTIONS_V 7
+#define STATE_TERMINAL  8
+int xlu_pci_parse_bdf(XLU_Config *cfg, libxl_device_pci *pcidev, const char *str)
+{
+    unsigned state = STATE_DOMAIN;
+    unsigned dom, bus, dev, func, vslot = 0;
+    char *buf2, *tok, *ptr, *end, *optkey = NULL;
+
+    if ( NULL == (buf2 = ptr = strdup(str)) )
+        return ERROR_NOMEM;
+
+    for(tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++) {
+        switch(state) {
+        case STATE_DOMAIN:
+            if ( *ptr == ':' ) {
+                state = STATE_BUS;
+                *ptr = '\0';
+                if ( hex_convert(tok, &dom, 0xffff) )
+                    goto parse_error;
+                tok = ptr + 1;
+            }
+            break;
+        case STATE_BUS:
+            if ( *ptr == ':' ) {
+                state = STATE_DEV;
+                *ptr = '\0';
+                if ( hex_convert(tok, &bus, 0xff) )
+                    goto parse_error;
+                tok = ptr + 1;
+            }else if ( *ptr == '.' ) {
+                state = STATE_FUNC;
+                *ptr = '\0';
+                if ( dom & ~0xff )
+                    goto parse_error;
+                bus = dom;
+                dom = 0;
+                if ( hex_convert(tok, &dev, 0xff) )
+                    goto parse_error;
+                tok = ptr + 1;
+            }
+            break;
+        case STATE_DEV:
+            if ( *ptr == '.' ) {
+                state = STATE_FUNC;
+                *ptr = '\0';
+                if ( hex_convert(tok, &dev, 0xff) )
+                    goto parse_error;
+                tok = ptr + 1;
+            }
+            break;
+        case STATE_FUNC:
+            if ( *ptr == '\0' || *ptr == '@' || *ptr == ',' ) {
+                switch( *ptr ) {
+                case '\0':
+                    state = STATE_TERMINAL;
+                    break;
+                case '@':
+                    state = STATE_VSLOT;
+                    break;
+                case ',':
+                    state = STATE_OPTIONS_K;
+                    break;
+                }
+                *ptr = '\0';
+                if ( !strcmp(tok, "*") ) {
+                    pcidev->vfunc_mask = LIBXL_PCI_FUNC_ALL;
+                }else{
+                    if ( hex_convert(tok, &func, 0x7) )
+                        goto parse_error;
+                    pcidev->vfunc_mask = (1 << 0);
+                }
+                tok = ptr + 1;
+            }
+            break;
+        case STATE_VSLOT:
+            if ( *ptr == '\0' || *ptr == ',' ) {
+                state = ( *ptr == ',' ) ? STATE_OPTIONS_K : STATE_TERMINAL;
+                *ptr = '\0';
+                if ( hex_convert(tok, &vslot, 0xff) )
+                    goto parse_error;
+                tok = ptr + 1;
+            }
+            break;
+        case STATE_OPTIONS_K:
+            if ( *ptr == '=' ) {
+                state = STATE_OPTIONS_V;
+                *ptr = '\0';
+                optkey = tok;
+                tok = ptr + 1;
+            }
+            break;
+        case STATE_OPTIONS_V:
+            if ( *ptr == ',' || *ptr == '\0' ) {
+                state = (*ptr == ',') ? STATE_OPTIONS_K : STATE_TERMINAL;
+                *ptr = '\0';
+                if ( !strcmp(optkey, "msitranslate") ) {
+                    pcidev->msitranslate = atoi(tok);
+                }else if ( !strcmp(optkey, "power_mgmt") ) {
+                    pcidev->power_mgmt = atoi(tok);
+                }else{
+                    XLU__PCI_ERR(cfg, "Unknown PCI BDF option: %s", optkey);
+                }
+                tok = ptr + 1;
+            }
+        default:
+            break;
+        }
+    }
+
+    free(buf2);
+
+    if ( tok != ptr || state != STATE_TERMINAL )
+        goto parse_error;
+
+    /* Just a pretty way to fill in the values */
+    libxl_pci_dev_init(pcidev, dom, bus, dev, func, vslot << 3);
+
+    return 0;
+
+parse_error:
+    return ERROR_INVAL;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff -r f744e82ea740 -r 5386937e6c5c tools/libxl/libxlutil.h
--- a/tools/libxl/libxlutil.h	Wed Feb 29 16:30:34 2012 +0000
+++ b/tools/libxl/libxlutil.h	Mon Apr 02 11:29:34 2012 +0100
@@ -88,6 +88,16 @@ int xlu_disk_parse(XLU_Config *cfg, int 
    * resulting disk struct is used with libxl.
    */
 
+/*
+ * PCI specification parsing
+ */
+int xlu_pci_parse_bdf(XLU_Config *cfg, libxl_device_pci *pcidev, const char *str);
+/* */ 
+
+int xlu_pci_dev_init(libxl_device_pci *pcidev, unsigned int domain,
+                     unsigned int bus, unsigned int dev,
+                     unsigned int func, unsigned int vdevfn);
+
 
 #endif /* LIBXLUTIL_H */
 
diff -r f744e82ea740 -r 5386937e6c5c tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Wed Feb 29 16:30:34 2012 +0000
+++ b/tools/libxl/xl_cmdimpl.c	Mon Apr 02 11:29:34 2012 +0100
@@ -1005,7 +1005,7 @@ skip_vfb:
 
             pcidev->msitranslate = pci_msitranslate;
             pcidev->power_mgmt = pci_power_mgmt;
-            if (!libxl_device_pci_parse_bdf(ctx, pcidev, buf))
+            if (!xlu_pci_parse_bdf(config, pcidev, buf))
                 d_config->num_pcidevs++;
         }
         if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV)
@@ -2217,11 +2217,16 @@ int main_pcilist(int argc, char **argv)
 static void pcidetach(const char *dom, const char *bdf, int force)
 {
     libxl_device_pci pcidev;
+    XLU_Config *config;
 
     find_domain(dom);
 
     memset(&pcidev, 0x00, sizeof(pcidev));
-    if (libxl_device_pci_parse_bdf(ctx, &pcidev, bdf)) {
+    
+    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-detach: malformed BDF specification \"%s\"\n", bdf);
         exit(2);
     }
@@ -2257,11 +2262,16 @@ int main_pcidetach(int argc, char **argv
 static void pciattach(const char *dom, const char *bdf, const char *vs)
 {
     libxl_device_pci pcidev;
+    XLU_Config *config;
 
     find_domain(dom);
 
     memset(&pcidev, 0x00, sizeof(pcidev));
-    if (libxl_device_pci_parse_bdf(ctx, &pcidev, bdf)) {
+
+    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-attach: malformed BDF specification \"%s\"\n", bdf);
         exit(2);
     }
diff -r f744e82ea740 -r 5386937e6c5c tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c	Wed Feb 29 16:30:34 2012 +0000
+++ b/tools/python/xen/lowlevel/xl/xl.c	Mon Apr 02 11:29:34 2012 +0100
@@ -40,6 +40,7 @@
 
 #include <libxl.h>
 #include <libxl_utils.h>
+#include <libxlutil.h>
 
 #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
 
@@ -556,7 +557,7 @@ static PyObject *pyxl_pci_parse(XlObject
         return NULL;
     }
 
-    if ( libxl_device_pci_parse_bdf(self->ctx, &pci->obj, str) ) {
+    if ( xlu_pci_parse_bdf(NULL, &pci->obj, str) ) {
         PyErr_SetString(xl_error_obj, "cannot parse pci device spec (BDF)");
         Py_DECREF(pci);
         return NULL;

[PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough

[George Dunlap]

# HG changeset patch
# User George Dunlap <george.dunlap@eu.citrix.com>
# Date 1333363500 -3600
# Node ID 62b1030a2485536caf995b825bee9e8255f914b3
# Parent  5386937e6c5c9afaa8a3cd56d391dcc9e40d0596
xl,libxl: Add per-device and global permissive config options for pci passthrough

By default pciback only allows PV guests to write "known safe" values into
PCI config space.  But many devices require writes to other areas of config
space in order to operate properly.  One way to do that is with the "quirks"
interface, which specifies areas known safe to a particular device; the
other way is to mark a device as "permissive", which tells pciback to allow
all config space writes for that domain and device.

This adds a "permissive" flag to the libxl_pci struct and teaches libxl how
to write the appropriate value into sysfs to enable the permissive feature for
devices being passed through.  It also adds the permissive config options either
on a per-device basis, or as a global option in the xl command-line.

Because of the potential stability and security implications of enabling
permissive, the flag is left off by default.

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

diff -r 5386937e6c5c -r 62b1030a2485 docs/man/xl.cfg.pod.5
--- a/docs/man/xl.cfg.pod.5	Mon Apr 02 11:29:34 2012 +0100
+++ b/docs/man/xl.cfg.pod.5	Mon Apr 02 11:45:00 2012 +0100
@@ -294,10 +294,25 @@ XXX
 
 XXX
 
+=item B<permissive=BOOLEAN>
+
+By default pciback only allows PV guests to write "known safe" values into
+PCI config space.  But many devices require writes to other areas of config
+space in order to operate properly.  This tells the pciback driver to
+allow all writes to PCI config space for this domain and this device.  This
+option should be enabled with caution, as there may be stability or security 
+implications of doing so.
+
 =back
 
 =back
 
+=item B<pci_permissive=BOOLEAN>
+
+Changes the default value of 'permissive' for all PCI devices for this
+VM.  This can still be overriden on a per-device basis. See the
+"pci=" section for more information on the "permissive" flag.
+
 =back
 
 =head2 Paravirtualised (PV) Guest Specific Options
diff -r 5386937e6c5c -r 62b1030a2485 tools/libxl/libxl_pci.c
--- a/tools/libxl/libxl_pci.c	Mon Apr 02 11:29:34 2012 +0100
+++ b/tools/libxl/libxl_pci.c	Mon Apr 02 11:45:00 2012 +0100
@@ -55,7 +55,10 @@ static void libxl_create_pci_backend_dev
     if (pcidev->vdevfn)
         flexarray_append_pair(back, libxl__sprintf(gc, "vdevfn-%d", num), libxl__sprintf(gc, "%x", pcidev->vdevfn));
     flexarray_append(back, libxl__sprintf(gc, "opts-%d", num));
-    flexarray_append(back, libxl__sprintf(gc, "msitranslate=%d,power_mgmt=%d", pcidev->msitranslate, pcidev->power_mgmt));
+    flexarray_append(back,
+              libxl__sprintf(gc, "msitranslate=%d,power_mgmt=%d,permissive=%d",
+                             pcidev->msitranslate, pcidev->power_mgmt,
+                             pcidev->permissive));
     flexarray_append_pair(back, libxl__sprintf(gc, "state-%d", num), libxl__sprintf(gc, "%d", 1));
 }
 
@@ -565,6 +568,29 @@ static int do_pci_add(libxl__gc *gc, uin
             }
         }
         fclose(f);
+
+        /* 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);
+                goto out;
+            }
+ 
+            buf = libxl__sprintf(gc, PCI_BDF, pcidev->domain, pcidev->bus,
+                                 pcidev->dev, pcidev->func);
+            rc = write(fd, buf, strlen(buf));
+            if (rc < 0)
+                LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "write to %s returned %d",
+                                 sysfs_path, rc);
+            close(fd);
+            goto out;
+        }
         break;
     }
     default:
@@ -958,6 +984,9 @@ static void libxl__device_pci_from_xs_be
             } else if (!strcmp(p, "power_mgmt")) {
                 p = strtok_r(NULL, ",=", &saveptr);
                 pci->power_mgmt = atoi(p);
+            } else if (!strcmp(p, "permissive")) {
+                p = strtok_r(NULL, ",=", &saveptr);
+                pci->permissive = atoi(p);
             }
         } while ((p = strtok_r(NULL, ",=", &saveptr)) != NULL);
     }
diff -r 5386937e6c5c -r 62b1030a2485 tools/libxl/libxl_types.idl
--- a/tools/libxl/libxl_types.idl	Mon Apr 02 11:29:34 2012 +0100
+++ b/tools/libxl/libxl_types.idl	Mon Apr 02 11:45:00 2012 +0100
@@ -352,6 +352,7 @@ libxl_device_pci = Struct("device_pci", 
     ("vfunc_mask", uint32),
     ("msitranslate", bool),
     ("power_mgmt", bool),
+    ("permissive", bool),
     ])
 
 libxl_diskinfo = Struct("diskinfo", [
diff -r 5386937e6c5c -r 62b1030a2485 tools/libxl/libxlu_pci.c
--- a/tools/libxl/libxlu_pci.c	Mon Apr 02 11:29:34 2012 +0100
+++ b/tools/libxl/libxlu_pci.c	Mon Apr 02 11:45:00 2012 +0100
@@ -127,6 +127,8 @@ int xlu_pci_parse_bdf(XLU_Config *cfg, l
                     pcidev->msitranslate = atoi(tok);
                 }else if ( !strcmp(optkey, "power_mgmt") ) {
                     pcidev->power_mgmt = atoi(tok);
+                }else if ( !strcmp(optkey, "permissive") ) {
+                    pcidev->permissive = atoi(tok);
                 }else{
                     XLU__PCI_ERR(cfg, "Unknown PCI BDF option: %s", optkey);
                 }
diff -r 5386937e6c5c -r 62b1030a2485 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Mon Apr 02 11:29:34 2012 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Mon Apr 02 11:45:00 2012 +0100
@@ -518,6 +518,7 @@ static void parse_config_data(const char
     XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids;
     int pci_power_mgmt = 0;
     int pci_msitranslate = 1;
+    int pci_permissive = 0;
     int e;
 
     libxl_domain_create_info *c_info = &d_config->c_info;
@@ -986,6 +987,9 @@ skip_vfb:
     if (!xlu_cfg_get_long (config, "pci_power_mgmt", &l, 0))
         pci_power_mgmt = l;
 
+    if (!xlu_cfg_get_long (config, "pci_permissive", &l, 0))
+        pci_permissive = l;
+
     /* To be reworked (automatically enabled) once the auto ballooning
      * after guest starts is done (with PCI devices passed in). */
     if (c_info->type == LIBXL_DOMAIN_TYPE_PV) {
@@ -1005,6 +1009,7 @@ skip_vfb:
 
             pcidev->msitranslate = pci_msitranslate;
             pcidev->power_mgmt = pci_power_mgmt;
+            pcidev->permissive = pci_permissive;
             if (!xlu_pci_parse_bdf(config, pcidev, buf))
                 d_config->num_pcidevs++;
         }

Re: [PATCH 1 of 2] libxl: Move bdf parsing into libxlu

[Ian Campbell]

On Mon, 2012-04-02 at 11:47 +0100, George Dunlap wrote:
> # HG changeset patch
> # User George Dunlap <george.dunlap@eu.citrix.com>
> # Date 1333362574 -3600
> # Node ID 5386937e6c5c9afaa8a3cd56d391dcc9e40d0596
> # Parent  f744e82ea74075983de6d5b0ad0cf7ccacf999a2
> libxl: Move bdf parsing into libxlu
> 
> Config parsing functions do not properly belong in libxl.  Move them into
> libxlu so that others can use them or not as they see fit.
> 
> No functional changes.  One side-effect was making public a private libxl
> utility function which just set the elements of a structure from the  function
> arguments passed in.
> [...]
> diff -r f744e82ea740 -r 5386937e6c5c tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h       Wed Feb 29 16:30:34 2012 +0000
> +++ b/tools/libxl/libxl.h       Mon Apr 02 11:29:34 2012 +0100
> @@ -573,13 +573,10 @@ int libxl_device_pci_add(libxl_ctx *ctx,
>  int libxl_device_pci_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev);
>  int libxl_device_pci_destroy(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev);
>  libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid, int *num);
> -
> -/*
> - * Parse a PCI BDF into a PCI device structure.
> - */
> -int libxl_device_pci_parse_bdf(libxl_ctx *ctx,
> -                               libxl_device_pci *pcidev,
> -                               const char *str);
> +/* Just initialize the structure elements with the arguments provided. */
> +int libxl_pci_dev_init(libxl_device_pci *pcidev, unsigned int domain,
> +                       unsigned int bus, unsigned int dev,
> +                       unsigned int func, unsigned int vdevfn);

libxl_<type>_init has a particular meaning described further up in this
header. Although you haven't actually used <type> here so it doesn't
conflict the general convention is to use the type name as a prefix.

Does this function actually add all that much value? The users of it
could either open code it or have a local version.

Otherwise I think this patch looks good, thanks.

Ian.

Re: [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough

[Ian Campbell]

On Mon, 2012-04-02 at 11:47 +0100, George Dunlap wrote:
> # HG changeset patch
> # User George Dunlap <george.dunlap@eu.citrix.com>
> # Date 1333363500 -3600
> # Node ID 62b1030a2485536caf995b825bee9e8255f914b3
> # Parent  5386937e6c5c9afaa8a3cd56d391dcc9e40d0596
> xl,libxl: Add per-device and global permissive config options for pci passthrough
> 
> By default pciback only allows PV guests to write "known safe" values into
> PCI config space.  But many devices require writes to other areas of config
> space in order to operate properly.  One way to do that is with the "quirks"
> interface, which specifies areas known safe to a particular device; the
> other way is to mark a device as "permissive", which tells pciback to allow
> all config space writes for that domain and device.
> 
> This adds a "permissive" flag to the libxl_pci struct and teaches libxl how
> to write the appropriate value into sysfs to enable the permissive feature for
> devices being passed through.  It also adds the permissive config options either
> on a per-device basis, or as a global option in the xl command-line.
> 
> Because of the potential stability and security implications of enabling
> permissive, the flag is left off by default.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> diff -r 5386937e6c5c -r 62b1030a2485 docs/man/xl.cfg.pod.5
> --- a/docs/man/xl.cfg.pod.5	Mon Apr 02 11:29:34 2012 +0100
> +++ b/docs/man/xl.cfg.pod.5	Mon Apr 02 11:45:00 2012 +0100
> @@ -294,10 +294,25 @@ XXX
>  
>  XXX
>  
> +=item B<permissive=BOOLEAN>
> +
> +By default pciback only allows PV guests to write "known safe" values into
> +PCI config space.  But many devices require writes to other areas of config
> +space in order to operate properly.  This tells the pciback driver to
> +allow all writes to PCI config space for this domain and this device.  This
> +option should be enabled with caution, as there may be stability or security 
> +implications of doing so.
> +
>  =back
>  
>  =back
>  
> +=item B<pci_permissive=BOOLEAN>
> +
> +Changes the default value of 'permissive' for all PCI devices for this
> +VM.  This can still be overriden on a per-device basis. See the
> +"pci=" section for more information on the "permissive" flag.
> +
>  =back
>  
>  =head2 Paravirtualised (PV) Guest Specific Options
> diff -r 5386937e6c5c -r 62b1030a2485 tools/libxl/libxl_pci.c
> --- a/tools/libxl/libxl_pci.c	Mon Apr 02 11:29:34 2012 +0100
> +++ b/tools/libxl/libxl_pci.c	Mon Apr 02 11:45:00 2012 +0100
> @@ -55,7 +55,10 @@ static void libxl_create_pci_backend_dev
>      if (pcidev->vdevfn)
>          flexarray_append_pair(back, libxl__sprintf(gc, "vdevfn-%d", num), libxl__sprintf(gc, "%x", pcidev->vdevfn));
>      flexarray_append(back, libxl__sprintf(gc, "opts-%d", num));
> -    flexarray_append(back, libxl__sprintf(gc, "msitranslate=%d,power_mgmt=%d", pcidev->msitranslate, pcidev->power_mgmt));
> +    flexarray_append(back,
> +              libxl__sprintf(gc, "msitranslate=%d,power_mgmt=%d,permissive=%d",
> +                             pcidev->msitranslate, pcidev->power_mgmt,
> +                             pcidev->permissive));

Since we are already writing this key, and AFAICT this matches xend's
behaviour, I think it is correct to add the new parameter here. But...

Does anything actually read this key? I can't find anything in pciback
or qemu. 

>      flexarray_append_pair(back, libxl__sprintf(gc, "state-%d", num), libxl__sprintf(gc, "%d", 1));
>  }
>  
> @@ -565,6 +568,29 @@ static int do_pci_add(libxl__gc *gc, uin
>              }
>          }
>          fclose(f);
> +
> +        /* 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);
> +                goto out;

Why not either fall through (i.e. accept this as a soft failure) or
return an actual error here?

FWIW I think the case where we cannot open the sysfs "irq" node and
"goto out" is also similarly wrong.

> +            }
> + 
> +            buf = libxl__sprintf(gc, PCI_BDF, pcidev->domain, pcidev->bus,
> +                                 pcidev->dev, pcidev->func);
> +            rc = write(fd, buf, strlen(buf));
> +            if (rc < 0)
> +                LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "write to %s returned %d",
> +                                 sysfs_path, rc);
> +            close(fd);
> +            goto out;

I don't think this makes sense, out is the next thing we actually get to
anyway and there is a "break" out of the switch right below.

> +        }
>          break;
>      }
>      default:

Ian.

Re: [PATCH 1 of 2] libxl: Move bdf parsing into libxlu

[George Dunlap]

On 02/04/12 12:05, Ian Campbell wrote:
> On Mon, 2012-04-02 at 11:47 +0100, George Dunlap wrote:
>> # HG changeset patch
>> # User George Dunlap<george.dunlap@eu.citrix.com>
>> # Date 1333362574 -3600
>> # Node ID 5386937e6c5c9afaa8a3cd56d391dcc9e40d0596
>> # Parent  f744e82ea74075983de6d5b0ad0cf7ccacf999a2
>> libxl: Move bdf parsing into libxlu
>>
>> Config parsing functions do not properly belong in libxl.  Move them into
>> libxlu so that others can use them or not as they see fit.
>>
>> No functional changes.  One side-effect was making public a private libxl
>> utility function which just set the elements of a structure from the  function
>> arguments passed in.
>> [...]
>> diff -r f744e82ea740 -r 5386937e6c5c tools/libxl/libxl.h
>> --- a/tools/libxl/libxl.h       Wed Feb 29 16:30:34 2012 +0000
>> +++ b/tools/libxl/libxl.h       Mon Apr 02 11:29:34 2012 +0100
>> @@ -573,13 +573,10 @@ int libxl_device_pci_add(libxl_ctx *ctx,
>>   int libxl_device_pci_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev);
>>   int libxl_device_pci_destroy(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev);
>>   libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid, int *num);
>> -
>> -/*
>> - * Parse a PCI BDF into a PCI device structure.
>> - */
>> -int libxl_device_pci_parse_bdf(libxl_ctx *ctx,
>> -                               libxl_device_pci *pcidev,
>> -                               const char *str);
>> +/* Just initialize the structure elements with the arguments provided. */
>> +int libxl_pci_dev_init(libxl_device_pci *pcidev, unsigned int domain,
>> +                       unsigned int bus, unsigned int dev,
>> +                       unsigned int func, unsigned int vdevfn);
> libxl_<type>_init has a particular meaning described further up in this
> header. Although you haven't actually used<type>  here so it doesn't
> conflict the general convention is to use the type name as a prefix.
>
> Does this function actually add all that much value? The users of it
> could either open code it or have a local version.
You know, I think I'll just make two local copies of the function.  I'll 
also give it a less misleading name.

  -George

Re: [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough

[Ian Jackson]

George Dunlap writes ("[Xen-devel] [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough"):
> +By default pciback only allows PV guests to write "known safe" values into
> +PCI config space.  But many devices require writes to other areas of config
> +space in order to operate properly.  This tells the pciback driver to
> +allow all writes to PCI config space for this domain and this device.  This
> +option should be enabled with caution, as there may be stability or security 
> +implications of doing so.

Is this security warning not overly mealy-mouthed ?  Surely it should
be more definite.

> +Changes the default value of 'permissive' for all PCI devices for this
> +VM.  This can still be overriden on a per-device basis. See the
> +"pci=" section for more information on the "permissive" flag.

And this should mention it as well I think.

> +                LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "write to %s returned %d",

Please keep the lines to 75-80 characters at most.

I think you should consider breakibg out the sysfs writing function
and refactoring with the very similar code in libxl__device_pci_reset,
rather than introducing yet another clone.

Ian.

Re: [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough

[George Dunlap]

On 02/04/12 12:30, Ian Campbell wrote:
> On Mon, 2012-04-02 at 11:47 +0100, George Dunlap wrote:
>> # HG changeset patch
>> # User George Dunlap<george.dunlap@eu.citrix.com>
>> # Date 1333363500 -3600
>> # Node ID 62b1030a2485536caf995b825bee9e8255f914b3
>> # Parent  5386937e6c5c9afaa8a3cd56d391dcc9e40d0596
>> xl,libxl: Add per-device and global permissive config options for pci passthrough
>>
>> By default pciback only allows PV guests to write "known safe" values into
>> PCI config space.  But many devices require writes to other areas of config
>> space in order to operate properly.  One way to do that is with the "quirks"
>> interface, which specifies areas known safe to a particular device; the
>> other way is to mark a device as "permissive", which tells pciback to allow
>> all config space writes for that domain and device.
>>
>> This adds a "permissive" flag to the libxl_pci struct and teaches libxl how
>> to write the appropriate value into sysfs to enable the permissive feature for
>> devices being passed through.  It also adds the permissive config options either
>> on a per-device basis, or as a global option in the xl command-line.
>>
>> Because of the potential stability and security implications of enabling
>> permissive, the flag is left off by default.
>>
>> Signed-off-by: George Dunlap<george.dunlap@eu.citrix.com>
>>
>> diff -r 5386937e6c5c -r 62b1030a2485 docs/man/xl.cfg.pod.5
>> --- a/docs/man/xl.cfg.pod.5	Mon Apr 02 11:29:34 2012 +0100
>> +++ b/docs/man/xl.cfg.pod.5	Mon Apr 02 11:45:00 2012 +0100
>> @@ -294,10 +294,25 @@ XXX
>>
>>   XXX
>>
>> +=item B<permissive=BOOLEAN>
>> +
>> +By default pciback only allows PV guests to write "known safe" values into
>> +PCI config space.  But many devices require writes to other areas of config
>> +space in order to operate properly.  This tells the pciback driver to
>> +allow all writes to PCI config space for this domain and this device.  This
>> +option should be enabled with caution, as there may be stability or security
>> +implications of doing so.
>> +
>>   =back
>>
>>   =back
>>
>> +=item B<pci_permissive=BOOLEAN>
>> +
>> +Changes the default value of 'permissive' for all PCI devices for this
>> +VM.  This can still be overriden on a per-device basis. See the
>> +"pci=" section for more information on the "permissive" flag.
>> +
>>   =back
>>
>>   =head2 Paravirtualised (PV) Guest Specific Options
>> diff -r 5386937e6c5c -r 62b1030a2485 tools/libxl/libxl_pci.c
>> --- a/tools/libxl/libxl_pci.c	Mon Apr 02 11:29:34 2012 +0100
>> +++ b/tools/libxl/libxl_pci.c	Mon Apr 02 11:45:00 2012 +0100
>> @@ -55,7 +55,10 @@ static void libxl_create_pci_backend_dev
>>       if (pcidev->vdevfn)
>>           flexarray_append_pair(back, libxl__sprintf(gc, "vdevfn-%d", num), libxl__sprintf(gc, "%x", pcidev->vdevfn));
>>       flexarray_append(back, libxl__sprintf(gc, "opts-%d", num));
>> -    flexarray_append(back, libxl__sprintf(gc, "msitranslate=%d,power_mgmt=%d", pcidev->msitranslate, pcidev->power_mgmt));
>> +    flexarray_append(back,
>> +              libxl__sprintf(gc, "msitranslate=%d,power_mgmt=%d,permissive=%d",
>> +                             pcidev->msitranslate, pcidev->power_mgmt,
>> +                             pcidev->permissive));
> Since we are already writing this key, and AFAICT this matches xend's
> behaviour, I think it is correct to add the new parameter here. But...
>
> Does anything actually read this key? I can't find anything in pciback
> or qemu.
No idea -- I was just following suit.  In any case, it's probably not a 
bad idea to have this kind of thing in there to help debug stuff.

Let me know if you want to do something else, otherwise I'll keep it in 
for my next patch series.
>>       flexarray_append_pair(back, libxl__sprintf(gc, "state-%d", num), libxl__sprintf(gc, "%d", 1));
>>   }
>>
>> @@ -565,6 +568,29 @@ static int do_pci_add(libxl__gc *gc, uin
>>               }
>>           }
>>           fclose(f);
>> +
>> +        /* 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);
>> +                goto out;
> Why not either fall through (i.e. accept this as a soft failure) or
> return an actual error here?
>
> FWIW I think the case where we cannot open the sysfs "irq" node and
> "goto out" is also similarly wrong.

>
>> +            }
>> +
>> +            buf = libxl__sprintf(gc, PCI_BDF, pcidev->domain, pcidev->bus,
>> +                                 pcidev->dev, pcidev->func);
>> +            rc = write(fd, buf, strlen(buf));
>> +            if (rc<  0)
>> +                LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "write to %s returned %d",
>> +                                 sysfs_path, rc);
>> +            close(fd);
>> +            goto out;
> I don't think this makes sense, out is the next thing we actually get to
> anyway and there is a "break" out of the switch right below.
Sorry -- yeah, I'm not seeing how that code got generated. :-/

I think what I'm going to do is have it return ERROR_FAIL if either the 
open or the write fails.  In general, if the user has asked specifically 
for something to happen and it can't happen, there should be an error, 
so the user can decide if he doesn't care so much, or fix things so it 
can happen.

  -George

Re: [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough

[Ian Campbell]

> >> diff -r 5386937e6c5c -r 62b1030a2485 tools/libxl/libxl_pci.c
> >> --- a/tools/libxl/libxl_pci.c	Mon Apr 02 11:29:34 2012 +0100
> >> +++ b/tools/libxl/libxl_pci.c	Mon Apr 02 11:45:00 2012 +0100
> >> @@ -55,7 +55,10 @@ static void libxl_create_pci_backend_dev
> >>       if (pcidev->vdevfn)
> >>           flexarray_append_pair(back, libxl__sprintf(gc, "vdevfn-%d", num), libxl__sprintf(gc, "%x", pcidev->vdevfn));
> >>       flexarray_append(back, libxl__sprintf(gc, "opts-%d", num));
> >> -    flexarray_append(back, libxl__sprintf(gc, "msitranslate=%d,power_mgmt=%d", pcidev->msitranslate, pcidev->power_mgmt));
> >> +    flexarray_append(back,
> >> +              libxl__sprintf(gc, "msitranslate=%d,power_mgmt=%d,permissive=%d",
> >> +                             pcidev->msitranslate, pcidev->power_mgmt,
> >> +                             pcidev->permissive));
> > Since we are already writing this key, and AFAICT this matches xend's
> > behaviour, I think it is correct to add the new parameter here. But...
> >
> > Does anything actually read this key? I can't find anything in pciback
> > or qemu.
> No idea -- I was just following suit.  In any case, it's probably not a 
> bad idea to have this kind of thing in there to help debug stuff.
> 
> Let me know if you want to do something else, otherwise I'll keep it in 
> for my next patch series.

It's fine, I was just wondering if anyone knew what it was for.

Ian.

Re: [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough

[George Dunlap]

On 02/04/12 16:20, Ian Jackson wrote:
> George Dunlap writes ("[Xen-devel] [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough"):
>> +By default pciback only allows PV guests to write "known safe" values into
>> +PCI config space.  But many devices require writes to other areas of config
>> +space in order to operate properly.  This tells the pciback driver to
>> +allow all writes to PCI config space for this domain and this device.  This
>> +option should be enabled with caution, as there may be stability or security
>> +implications of doing so.
> Is this security warning not overly mealy-mouthed ?  Surely it should
> be more definite.
I'm not sure how we can make it more definite.  What's possible (i.e., 
the security implications) entirely depends on the card; and what's 
likely (i.e., the stability implications) entirely depends on the card 
and the driver.  Short of giving a short discourse on the vices of 
various cards PCI config space (which is entirely inappropriate for a 
man page, IMHO), I'm not sure what more we can say.
>> +Changes the default value of 'permissive' for all PCI devices for this
>> +VM.  This can still be overriden on a per-device basis. See the
>> +"pci=" section for more information on the "permissive" flag.
> And this should mention it as well I think.
I thought it was unnecessary to duplicate, but I can do so if you prefer.
>
>> +                LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "write to %s returned %d",
> Please keep the lines to 75-80 characters at most.
Ack.
>
> I think you should consider breakibg out the sysfs writing function
> and refactoring with the very similar code in libxl__device_pci_reset,
> rather than introducing yet another clone.
I shall consider it. :-)

  -George

Re: [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough

[Ian Jackson]

George Dunlap writes ("Re: [Xen-devel] [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough"):
> I'm not sure how we can make it more definite.  What's possible (i.e., 
> the security implications) entirely depends on the card; and what's 
> likely (i.e., the stability implications) entirely depends on the card 
> and the driver.  Short of giving a short discourse on the vices of 
> various cards PCI config space (which is entirely inappropriate for a 
> man page, IMHO), I'm not sure what more we can say.

Is it generally or usually the case that this option will more
completely expose the host ?

> I thought it was unnecessary to duplicate, but I can do so if you prefer.

I guess that depends on how strong a statement it is.

> > I think you should consider breakibg out the sysfs writing function
> > and refactoring with the very similar code in libxl__device_pci_reset,
> > rather than introducing yet another clone.
>
> I shall consider it. :-)

Thanks :-).

Ian.

Re: [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough

[George Dunlap]

On 02/04/12 16:51, Ian Jackson wrote:
> George Dunlap writes ("Re: [Xen-devel] [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough"):
>> I'm not sure how we can make it more definite.  What's possible (i.e.,
>> the security implications) entirely depends on the card; and what's
>> likely (i.e., the stability implications) entirely depends on the card
>> and the driver.  Short of giving a short discourse on the vices of
>> various cards PCI config space (which is entirely inappropriate for a
>> man page, IMHO), I'm not sure what more we can say.
> Is it generally or usually the case that this option will more
> completely expose the host ?
So, worst-case, the guest driver can make the card do anything a card 
can actually do.  Most of the things a card can do can be mitigated by 
the IOMMU.  But there may be some things which are not; and there are 
some people still running older hardware that either doesn't have an 
IOMMU, or whose IOMMU cannot handle important cases (e.g., Intel boxes 
with VTD but no interrupt remapping, if you recall the security issue 
related to this last year).

One of the examples Stefano gave of config stuff that can cause problems 
is the power management features: if the guest driver powers down the 
card, then when libxl tries to reset the card, it generates a PCI error 
interrupt.  This used to crash Xen. (It's now been fixed.)

But on the other hand, how many cards even have these kinds of dangerous 
capabilities in their PCI registers?  Most of them are probably just 
fine.  And in the case of driver domains, most people will be running 
trusted software anyway; the driver will be the same in the domU as the 
dom0.

Still, can't very well just turn things on by default and hope for the 
best; people need to know that they're doing something potentially 
dangerous.  But we can't really tell people how dangerous this thing 
might be, because we don't actually know how many cards might actually 
be dangerous, and we don't know what kind of software they're allowing 
access to the card.  And again, we can't just not give the option, 
because many cards need it to run, and most of the time it's just fine.

This is probably worth doing some more investigation and writing up in a 
doc and/or a wiki page somewhere; but I'm not sure we can do more in a 
man page than give a necessarily unspecific warning.

  -George
>> I thought it was unnecessary to duplicate, but I can do so if you prefer.
> I guess that depends on how strong a statement it is.
>
>>> I think you should consider breakibg out the sysfs writing function
>>> and refactoring with the very similar code in libxl__device_pci_reset,
>>> rather than introducing yet another clone.
>> I shall consider it. :-)
> Thanks :-).
>
> Ian.

Re: [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough

[Ian Jackson]

George Dunlap writes ("Re: [Xen-devel] [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough"):
> This is probably worth doing some more investigation and writing up in a 
> doc and/or a wiki page somewhere; but I'm not sure we can do more in a 
> man page than give a necessarily unspecific warning.

OK.  I guess that's fine as it is then.

Ian.

Re: [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough

[George Dunlap]

On 02/04/12 16:51, Ian Jackson wrote:
> George Dunlap writes ("Re: [Xen-devel] [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough"):
>> I'm not sure how we can make it more definite.  What's possible (i.e.,
>> the security implications) entirely depends on the card; and what's
>> likely (i.e., the stability implications) entirely depends on the card
>> and the driver.  Short of giving a short discourse on the vices of
>> various cards PCI config space (which is entirely inappropriate for a
>> man page, IMHO), I'm not sure what more we can say.
> Is it generally or usually the case that this option will more
> completely expose the host ?
>
>> I thought it was unnecessary to duplicate, but I can do so if you prefer.
> I guess that depends on how strong a statement it is.
>
>>> I think you should consider breakibg out the sysfs writing function
>>> and refactoring with the very similar code in libxl__device_pci_reset,
>>> rather than introducing yet another clone.
>> I shall consider it. :-)
I think for this patch series I'm probably going to leave it; I'll work 
on it when I add the PCI rebinding stuff.  (Otherwise there's the 
possibility I may end up having to refactor it again.)

  -George

[PATCH 0 of 2] [v2] Add per-device and global permissive config options for pci passthrough

[George Dunlap]

This patch series adds the "permissive" option for passed-through devices
which access config space out of the "known good" PCI config space.

v2:
- Added patch to move bdf parsing to libxlu
- Addressed some formatting comments

v3:
- Don't make the struct initialization utility function public
- Return error on failure
- Wrap a long line
- Clarify security warning / recommendation

Re: [PATCH 0 of 2] [v2] Add per-device and global permissive config options for pci passthrough

[George Dunlap]

BTW, that should be "[v3]"...

On Tue, Apr 3, 2012 at 2:54 PM, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> This patch series adds the "permissive" option for passed-through devices
> which access config space out of the "known good" PCI config space.
>
> v2:
> - Added patch to move bdf parsing to libxlu
> - Addressed some formatting comments
>
> v3:
> - Don't make the struct initialization utility function public
> - Return error on failure
> - Wrap a long line
> - Clarify security warning / recommendation
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 0 of 2] [v2] Add per-device and global permissive config options for pci passthrough

[Ian Jackson]

George Dunlap writes ("[Xen-devel] [PATCH 0 of 2] [v2] Add per-device and global permissive config options for pci passthrough"):
> This patch series adds the "permissive" option for passed-through devices
> which access config space out of the "known good" PCI config space.

Applied both, thanks.

Ian.