xl: implement basic PCI passthrough sanity checks

tools/libxl/libxl_pci.c +6 -0

+                bdf = libxl_xs_read(ctx, XBT_NULL, devpath);
+                if ( bdf ) {
+                    unsigned dom, bus, dev, func;
+                    if ( sscanf(bdf, PCI_BDF, &dom, &bus, &dev, &func) != 4 )
+                        continue;
+

 tools/libxl/libxl.h       |    1 +
 tools/libxl/libxl_pci.c   |  123 ++++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/xl.h          |    1 +
 tools/libxl/xl_cmdimpl.c  |   34 ++++++++++++
 tools/libxl/xl_cmdtable.c |    5 +
 5 files changed, 164 insertions(+), 0 deletions(-)

diff -r 20b2e15131c7 -r 214733749470 tools/libxl/libxl.h

tools/libxl/libxl.h +1 -0

@@ -496,6 +496,7 @@ int libxl_device_pci_add(struct libxl_ct
 int libxl_device_pci_remove(struct libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev);
 int libxl_device_pci_shutdown(struct libxl_ctx *ctx, uint32_t domid);
 libxl_device_pci *libxl_device_pci_list(struct libxl_ctx *ctx, uint32_t domid, int *num);
+libxl_device_pci *libxl_device_pci_list_assignable(struct libxl_ctx *ctx, int *num);
 int libxl_device_pci_init(libxl_device_pci *pcidev, unsigned int domain,
                           unsigned int bus, unsigned int dev,
                           unsigned int func, unsigned int vdevfn);

diff -r 20b2e15131c7 -r 214733749470 tools/libxl/libxl_pci.c

tools/libxl/libxl_pci.c +39 -0

@@ -28,6 +28,7 @@
 #include <unistd.h> /* for write, unlink and close */
 #include <stdint.h>
 #include <inttypes.h>
+#include <dirent.h>
 #include <assert.h>
 
 #include "libxl.h"

@@ -258,6 +259,75 @@ retry_transaction2:
     return 0;
 }
 
+static libxl_device_pci *get_all_assigned_devices(struct libxl_ctx *ctx, int *num)
+{
+    libxl_device_pci *new, *pcidevs = NULL;
+    char **domlist;
+    unsigned int nd, i;
+
+    *num = 0;
+
+    domlist = libxl_xs_directory(ctx, XBT_NULL, "/local/domain", &nd);
+    for(i = 0; i < nd; i++) {
+        char *path, *num_devs;
+
+        path = libxl_sprintf(ctx, "/local/domain/0/backend/pci/%s/0/num_devs", domlist[i]);
+        num_devs = libxl_xs_read(ctx, XBT_NULL, path);
+        if ( num_devs ) {
+            int ndev = atoi(num_devs), j;
+            char *devpath, *bdf;
+
+            for(j = 0; j < ndev; j++) {
+                devpath = libxl_sprintf(ctx, "/local/domain/0/backend/pci/%s/0/dev-%u",
+                                        domlist[i], j);
+                bdf = libxl_xs_read(ctx, XBT_NULL, devpath);
+                if ( bdf ) {
+                    unsigned dom, bus, dev, func;
+                    if ( sscanf(bdf, PCI_BDF, &dom, &bus, &dev, &func) != 4 )
+                        continue;
+
+                    new = realloc(pcidevs, ((*num) + 1) * sizeof(*new));
+                    if ( NULL == new )
+                        continue;
+
+                    pcidevs = new;
+                    new = pcidevs + *num;
+
+                    libxl_device_pci_init(new, dom, bus, dev, func, ~0);
+                    (*num)++;
+                }
+                libxl_free(ctx, bdf);

tools/libxl/libxl_pci.c +64 -0

+
+static int is_assigned(libxl_device_pci *assigned, int num_assigned,
+                       int dom, int bus, int dev, int func)
+{
+    int i;
+
+    for(i = 0; i < num_assigned; i++) {
+        if ( assigned[i].domain != dom )
+            continue;
+        if ( assigned[i].bus != bus )
+            continue;
+        if ( assigned[i].dev != dev )
+            continue;
+        if ( assigned[i].func != func )
+            continue;
+        return 1;
+    }
+
+    return 0;
+}
+
 int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev)
 {
     char *path;

@@ -466,6 +536,59 @@ out:
     return 0;
 }
 
+static libxl_device_pci *scan_sys_pcidir(libxl_device_pci *pcidevs, libxl_device_pci *assigned,
+                                         int num_assigned, const char *path, int *num)
+{
+    libxl_device_pci *new;
+    struct dirent *de;
+    DIR *dir;
+
+    dir = opendir(path);
+    if ( NULL == dir )
+        return pcidevs;
+
+    while( (de = readdir(dir)) ) {
+        unsigned dom, bus, dev, func;
+        if ( sscanf(de->d_name, PCI_BDF, &dom, &bus, &dev, &func) != 4 )
+            continue;
+
+        if ( is_assigned(assigned, num_assigned, dom, bus, dev, func) )
+            continue;
+
+        new = realloc(pcidevs, ((*num) + 1) * sizeof(*new));
+        if ( NULL == new )
+            continue;
+
+        pcidevs = new;
+        new = pcidevs + *num;
+
+        libxl_device_pci_init(new, dom, bus, dev, func, ~0);
+        (*num)++;
+    }
+
+    closedir(dir);
+    return pcidevs;
+}
+
+libxl_device_pci *libxl_device_pci_list_assignable(struct libxl_ctx *ctx, int *num)
+{
+    libxl_device_pci *pcidevs = NULL;
+    libxl_device_pci *assigned;
+    int num_assigned;
+
+    *num = 0;
+
+    assigned = get_all_assigned_devices(ctx, &num_assigned);

tools/libxl/libxl_pci.c +0 -0

 libxl_device_pci *libxl_device_pci_list(struct libxl_ctx *ctx, uint32_t domid, int *num)
 {
     char *be_path, *num_devs, *xsdev, *xsvdevfn, *xsopts;

diff -r 20b2e15131c7 -r 214733749470 tools/libxl/xl.h

tools/libxl/xl.h +1 -0

@@ -32,6 +32,7 @@ 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_restore(int argc, char **argv);

diff -r 20b2e15131c7 -r 214733749470 tools/libxl/xl_cmdimpl.c

tools/libxl/xl_cmdimpl.c +34 -0

@@ -1706,6 +1706,40 @@ int main_vncviewer(int argc, char **argv
     exit(0);
 }
 
+void pcilist_assignable(void)
+{
+    libxl_device_pci *pcidevs;
+    int num, i;
+
+    pcidevs = libxl_device_pci_list_assignable(&ctx, &num);
+    if (!num)
+        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);
+    }
+    free(pcidevs);
+}
+
+int main_pcilist_assignable(int argc, char **argv)
+{
+    int opt;
+
+    while ((opt = getopt(argc, argv, "h")) != -1) {
+        switch (opt) {
+        case 'h':
+            help("pci-list-assignable-devices");
+            exit(0);
+        default:
+            fprintf(stderr, "option not supported\n");
+            break;
+        }
+    }
+
+    pcilist_assignable();
+    exit(0);
+}
+
 void pcilist(char *dom)
 {
     libxl_device_pci *pcidevs;

diff -r 20b2e15131c7 -r 214733749470 tools/libxl/xl_cmdtable.c

tools/libxl/xl_cmdtable.c +5 -0

@@ -68,6 +68,11 @@ struct cmd_spec cmd_table[] = {
       "List pass-through pci devices for a domain",
       "<Domain>",
     },
+    { "pci-list-assignable-devices",
+      &main_pcilist_assignable,
+      "List all the assignable pci devices",
+      "",
+    },
     { "pause",
       &main_pause,
       "Pause execution of a domain",

Quoted 11 lines

tools/libxl/libxl_pci.c +11 -0

+                }
+                libxl_free(ctx, bdf);
+                libxl_free(ctx, devpath);
+            }
+            libxl_free(ctx, num_devs);
+        }
+        libxl_free(ctx, path);
+    }
+    libxl_free(ctx, domlist);
+
+    return pcidevs;

Quoted 1 line

+}

You shouldn't use libxl_free explicitly on variables allocated in the context. Actually I like explicit free's but we have to keep the style consistent, so the memory management refactoring should come in as a separate patch, and here there shouldn't be any libxl_free's. The realloc is correct because it is used on a variable that is going to be returned to the caller.

Also I would prefer this function to return an integer and take the libxl_device_pci array as a parameter.

Quoted 16 lines

tools/libxl/libxl_pci.c +16 -0

+libxl_device_pci *libxl_device_pci_list_assignable(struct libxl_ctx *ctx, int *num)
+{
+    libxl_device_pci *pcidevs = NULL;
+    libxl_device_pci *assigned;
+    int num_assigned;
+
+    *num = 0;
+
+    assigned = get_all_assigned_devices(ctx, &num_assigned);
+
+    pcidevs = scan_sys_pcidir(pcidevs, assigned, num_assigned,
+                              "/sys/bus/pci/drivers/pciback", num);
+    pcidevs = scan_sys_pcidir(pcidevs, assigned, num_assigned,
+                              "/sys/bus/pci/drivers/pcistub", num);
+
+    free(assigned);

tools/libxl/libxl_pci.c retry_transaction2: · line 310

Quoted 3 lines

tools/libxl/libxl_pci.c +3 -0

+    return pcidevs;
+}
+

Same comment about returning an integer, also I don't like the way you are "assuming" scan_sys_pcidir is going to use realloc on pcidevs: that should be an implementation detail of scan_sys_pcidir that callers shouldn't rely upon.

Consider that devices bound to pcistub cannot be assigned to PV guests, so I would remove scan_sys_pcidir on pcistub from here. Please define "/sys/bus/pci/drivers/pciback" in a macro.

Quoted 11 lines · Stefano Stabellini

Quoted 11 lines

tools/libxl/libxl_pci.c +11 -0

+                }
+                libxl_free(ctx, bdf);
+                libxl_free(ctx, devpath);
+            }
+            libxl_free(ctx, num_devs);
+        }
+        libxl_free(ctx, path);
+    }
+    libxl_free(ctx, domlist);
+
+    return pcidevs;

Quoted 8 lines

Quoted 1 line

+}

You shouldn't use libxl_free explicitly on variables allocated in the context. Actually I like explicit free's but we have to keep the style consistent, so the memory management refactoring should come in as a separate patch, and here there shouldn't be any libxl_free's. The realloc is correct because it is used on a variable that is going to be returned to the caller.

OK, this was written before the idea for refactoring memory management and just done out of habit. Are you saying just remove the libxl_free's and re-submit it like that?

Quoted 16 lines

Quoted 16 lines

tools/libxl/libxl_pci.c +16 -0

+libxl_device_pci *libxl_device_pci_list_assignable(struct libxl_ctx *ctx, int *num)
+{
+    libxl_device_pci *pcidevs = NULL;
+    libxl_device_pci *assigned;
+    int num_assigned;
+
+    *num = 0;
+
+    assigned = get_all_assigned_devices(ctx, &num_assigned);
+
+    pcidevs = scan_sys_pcidir(pcidevs, assigned, num_assigned,
+                              "/sys/bus/pci/drivers/pciback", num);
+    pcidevs = scan_sys_pcidir(pcidevs, assigned, num_assigned,
+                              "/sys/bus/pci/drivers/pcistub", num);
+
+    free(assigned);

Quoted 8 lines

tools/libxl/libxl_pci.c retry_transaction2: · line 310

Quoted 3 lines

tools/libxl/libxl_pci.c +3 -0

+    return pcidevs;
+}
+

Same comment about returning an integer, also I don't like the way you are "assuming" scan_sys_pcidir is going to use realloc on pcidevs: that should be an implementation detail of scan_sys_pcidir that callers shouldn't rely upon.

Actually it's a static function with only one caller, this makes the implementation of list_assignable as simple as possible.

Quoted 3 lines

Consider that devices bound to pcistub cannot be assigned to PV guests, so I would remove scan_sys_pcidir on pcistub from here. Please define "/sys/bus/pci/drivers/pciback" in a macro.

But they can be assigned to HVM guests? Nothing in the add-device path uses this list of devices anyway. What actually happens is a check whether a device appears in the get_all_assigned_devices() list. Even after this patch-set the user can always specify BDF's which aren't really assignable (eg. the Host's PCI-to-ISA bridge or PCI root for maximum amusement) That is to be addressed in a future patch.

Quoted 32 lines

Quoted 16 lines

Quoted 16 lines

tools/libxl/libxl_pci.c +16 -0

+libxl_device_pci *libxl_device_pci_list_assignable(struct libxl_ctx *ctx, int *num)
+{
+    libxl_device_pci *pcidevs = NULL;
+    libxl_device_pci *assigned;
+    int num_assigned;
+
+    *num = 0;
+
+    assigned = get_all_assigned_devices(ctx, &num_assigned);
+
+    pcidevs = scan_sys_pcidir(pcidevs, assigned, num_assigned,
+                              "/sys/bus/pci/drivers/pciback", num);
+    pcidevs = scan_sys_pcidir(pcidevs, assigned, num_assigned,
+                              "/sys/bus/pci/drivers/pcistub", num);
+
+    free(assigned);

Quoted 8 lines

tools/libxl/libxl_pci.c retry_transaction2: · line 310

Quoted 3 lines

tools/libxl/libxl_pci.c +3 -0

+    return pcidevs;
+}
+

Same comment about returning an integer, also I don't like the way you are "assuming" scan_sys_pcidir is going to use realloc on pcidevs: that should be an implementation detail of scan_sys_pcidir that callers shouldn't rely upon.

Actually it's a static function with only one caller, this makes the implementation of list_assignable as simple as possible.

Quoted 3 lines

Consider that devices bound to pcistub cannot be assigned to PV guests, so I would remove scan_sys_pcidir on pcistub from here. Please define "/sys/bus/pci/drivers/pciback" in a macro.

Quoted 7 lines

But they can be assigned to HVM guests? Nothing in the add-device path uses this list of devices anyway. What actually happens is a check whether a device appears in the get_all_assigned_devices() list. Even after this patch-set the user can always specify BDF's which aren't really assignable (eg. the Host's PCI-to-ISA bridge or PCI root for maximum amusement) That is to be addressed in a future patch.

Yes but libxl_device_pci_list_assignable is a public function, we don't know what libxl users are going to do with it.
In particular it seems reasonable to call libxl_device_pci_list_assignable before assigning a device even if xl doesn't do it at the moment.

 tools/libxl/libxl_pci.c |  11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

This prevents PCI devices from being added multiply to the same domain or multiple different domains simultaneously.

diff -r 214733749470 -r dbfc4f30efe2 tools/libxl/libxl_pci.c

tools/libxl/libxl_pci.c +10 -1

@@ -334,8 +334,17 @@ int libxl_device_pci_add(struct libxl_ct
     char *state, *vdevfn;
     int rc, hvm;
     int stubdomid = 0;
+    libxl_device_pci *assigned;
+    int num_assigned;
 
-    /* TODO: check if the device can be assigned */
+    assigned = get_all_assigned_devices(ctx, &num_assigned);
+    if ( is_assigned(assigned, num_assigned, pcidev->domain,
+                     pcidev->bus, pcidev->dev, pcidev->func) ) {
+        XL_LOG(ctx, XL_LOG_ERROR, "PCI device already attached to a domain");
+        free(assigned);
+        return ERROR_FAIL;
+    }
+    free(assigned);
 
     libxl_device_pci_reset(ctx, pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);