xl PCI passthrough updates v3

 tools/libxl/libxl.h     |    1 +
 tools/libxl/libxl_pci.c |  118 ++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 111 insertions(+), 8 deletions(-)

# HG changeset patch
# User Gianni Tedesco <[email protected]> # Date 1280760698 -3600
# Branch pci-patches-v3
# Node ID 6379343447e58cc7b83e27beba9c35e62c232d9d # Parent eb8ee481bc063b18b39feaa76d9b757331ed3a9f
xl: implement multifunction device PCI pass-through

Implement PCI pass-through for multi-function devices. The supported BDF notation is: BB:DD.* - therefore passing-through a subset of functions or remapping the function numbers is not supported. Largely because I don't see how that can ever be safe or sane (perhaps for SR-IOV cards?)

Letting qemu automatically select the virtual slot is not supported since I don't believe that qemu-dm can actually handle that case.

Signed-off-by Gianni Tedesco <[email protected]>

diff -r eb8ee481bc06 -r 6379343447e5 tools/libxl/libxl.h

tools/libxl/libxl.h +1 -0

@@ -308,6 +308,7 @@ typedef struct {
     unsigned int vdevfn;
     bool msitranslate;
     bool power_mgmt;
+    bool multifunction;
 } libxl_device_pci;
 
 enum {

diff -r eb8ee481bc06 -r 6379343447e5 tools/libxl/libxl_pci.c

tools/libxl/libxl_pci.c +0 -2

@@ -136,8 +136,13 @@ int libxl_device_pci_parse_bdf(libxl_ctx
                     break;
                 }
                 *ptr = '\0';
-                if ( hex_convert(tok, &func, 0x7) )
-                    goto parse_error;

tools/libxl/libxl_pci.c +44 -0

+    struct dirent *de;
+    DIR *dir;
+
+    *func_mask = 0;
+
+    dir = opendir(SYSFS_PCI_DEV);
+    if ( NULL == dir ) {
+        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Couldn't open %s", SYSFS_PCI_DEV);
+        return -1;
+    }
+
+    while( (de = readdir(dir)) ) {
+        unsigned dom, bus, dev, func;
+        struct stat st;
+        char *path;
+
+        if ( sscanf(de->d_name, PCI_BDF, &dom, &bus, &dev, &func) != 4 )
+            continue;
+        if ( pcidev->domain != dom )
+            continue;
+        if ( pcidev->bus != bus )
+            continue;
+        if ( pcidev->dev != dev )
+            continue;
+
+        path = libxl_sprintf(ctx, "%s/" PCI_BDF, SYSFS_PCIBACK_DRIVER, dom, bus, dev, func);
+        if ( lstat(path, &st) ) {
+            if ( errno == ENOENT )
+                XL_LOG(ctx, XL_LOG_ERROR, PCI_BDF " is not assigned to pciback driver",
+                       dom, bus, dev, func);
+            else
+                XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Couldn't lstat %s", path);
+            closedir(dir);
+            return -1;
+        }
+        (*func_mask) |= (1 << func);
+    }
+
+    closedir(dir);
+    return 0;
+}
+
 static int pci_ins_check(libxl_ctx *ctx, uint32_t domid, const char *state, void *priv)
 {
     char *orig_state = priv;

@@ -679,10 +728,32 @@ int libxl_device_pci_add(libxl_ctx *ctx,
             return rc;
     }
 
+    if ( pcidev->multifunction ) {
+        unsigned int i, orig_vdev, func_mask, rc = 0;

tools/libxl/libxl_pci.c +24 -6

+            if ( (1 << i) & func_mask ) {
+                pcidev->func = i;
+                pcidev->vdevfn = orig_vdev | i;
+                if ( do_pci_add(ctx, domid, pcidev) )
+                    rc = ERROR_FAIL;
+            }
+        }
+        return rc;
+    }
+
     return do_pci_add(ctx, domid, pcidev);
 }
 
-int libxl_device_pci_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev)
+static int do_pci_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev)
 {
     libxl_device_pci *assigned;
     char *path;

@@ -711,10 +782,15 @@ int libxl_device_pci_remove(libxl_ctx *c
         libxl_xs_write(ctx, XBT_NULL, path, PCI_BDF, pcidev->domain,
                        pcidev->bus, pcidev->dev, pcidev->func);
         path = libxl_sprintf(ctx, "/local/domain/0/device-model/%d/command", domid);
-        xs_write(ctx->xsh, XBT_NULL, path, "pci-rem", strlen("pci-rem"));
-        if (libxl_wait_for_device_model(ctx, domid, "pci-removed", NULL, NULL) < 0) {
-            XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn't respond in time");
-            return ERROR_FAIL;
+
+        /* Remove all functions at once atomically by only signalling
+         * device-model for function 0 */
+        if ( !pcidev->multifunction || pcidev->func == 0 ) {
+            xs_write(ctx->xsh, XBT_NULL, path, "pci-rem", strlen("pci-rem"));
+            if (libxl_wait_for_device_model(ctx, domid, "pci-removed", NULL, NULL) < 0) {
+                XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn't respond in time");
+                return ERROR_FAIL;
+            }
         }
         path = libxl_sprintf(ctx, "/local/domain/0/device-model/%d/state", domid);
         xs_write(ctx->xsh, XBT_NULL, path, state, strlen(state));

@@ -769,7 +845,10 @@ skip1:
         fclose(f);
     }
 out:
-    libxl_device_pci_reset(ctx, pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+    /* don't do multiple resets while some functions are still passed through */
+    if ( !pcidev->multifunction || pcidev->func == 0 ) {
+        libxl_device_pci_reset(ctx, pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+    }
 
     if (!libxl_is_stubdom(ctx, domid, NULL)) {
         rc = xc_deassign_device(ctx->xch, domid, pcidev->value);

@@ -786,6 +865,29 @@ out:
     return 0;
 }
 

tools/libxl/libxl_pci.c +6 -0

+            if ( (1 << i) & func_mask ) {
+                pcidev->func = i;
+                pcidev->vdevfn = orig_vdev | i;
+                if ( do_pci_remove(ctx, domid, pcidev) )
+                    rc = ERROR_FAIL;
+            }

Quoted 50 lines · On Mon, 2 Aug 2010, Gianni Tedesco (3P) wrote:

 tools/libxl/libxl.h     |    1 +
 tools/libxl/libxl_pci.c |  118 ++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 111 insertions(+), 8 deletions(-)

# HG changeset patch
# User Gianni Tedesco <[email protected]> # Date 1280760698 -3600
# Branch pci-patches-v3
# Node ID 6379343447e58cc7b83e27beba9c35e62c232d9d # Parent eb8ee481bc063b18b39feaa76d9b757331ed3a9f
xl: implement multifunction device PCI pass-through

Implement PCI pass-through for multi-function devices. The supported BDF notation is: BB:DD.* - therefore passing-through a subset of functions or remapping the function numbers is not supported. Largely because I don't see how that can ever be safe or sane (perhaps for SR-IOV cards?)

Letting qemu automatically select the virtual slot is not supported since I don't believe that qemu-dm can actually handle that case.

Signed-off-by Gianni Tedesco <[email protected]>

diff -r eb8ee481bc06 -r 6379343447e5 tools/libxl/libxl.h

tools/libxl/libxl.h +1 -0

@@ -308,6 +308,7 @@ typedef struct {
     unsigned int vdevfn;
     bool msitranslate;
     bool power_mgmt;
+    bool multifunction;
 } libxl_device_pci;
 
 enum {

diff -r eb8ee481bc06 -r 6379343447e5 tools/libxl/libxl_pci.c

tools/libxl/libxl_pci.c +7 -2

@@ -136,8 +136,13 @@ int libxl_device_pci_parse_bdf(libxl_ctx
                     break;
                 }
                 *ptr = '\0';
-                if ( hex_convert(tok, &func, 0x7) )
-                    goto parse_error;
+                if ( !strcmp(tok, "*") ) {
+                    pcidev->multifunction = 1;
+                }else{
+                    if ( hex_convert(tok, &func, 0x7) )
+                        goto parse_error;
+                    pcidev->multifunction = 0;
+                }

tools/libxl/libxl_pci.c int libxl_device_pci_parse_bdf(libxl_ctx · line 148

Quoted 3 lines

tools/libxl/libxl_pci.c +0 -0

                 tok = ptr + 1;
             }
             break;

Couldn't you add the func_mask somewhere in libxl_device_pci, instead of calling pci_multifunction_check multilple times?
Would be possible to make the normal passthrough case just a subset of the general multifunction passthrough case to simplify the code?

tools/libxl/libxl_pci.c int libxl_device_pci_list_assignable(lib · line 540

Quoted 7 lines

tools/libxl/libxl_pci.c +2 -0

@@ -531,6 +536,50 @@ int libxl_device_pci_list_assignable(lib
     return 0;
 }
 
+static int pci_multifunction_check(libxl_ctx *ctx, libxl_device_pci *pcidev, unsigned int *func_mask)
+{

a comment on top of this function to explain what it is actually checking would be nice

Quoted 49 lines

tools/libxl/libxl_pci.c +42 -0

+    struct dirent *de;
+    DIR *dir;
+
+    *func_mask = 0;
+
+    dir = opendir(SYSFS_PCI_DEV);
+    if ( NULL == dir ) {
+        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Couldn't open %s", SYSFS_PCI_DEV);
+        return -1;
+    }
+
+    while( (de = readdir(dir)) ) {
+        unsigned dom, bus, dev, func;
+        struct stat st;
+        char *path;
+
+        if ( sscanf(de->d_name, PCI_BDF, &dom, &bus, &dev, &func) != 4 )
+            continue;
+        if ( pcidev->domain != dom )
+            continue;
+        if ( pcidev->bus != bus )
+            continue;
+        if ( pcidev->dev != dev )
+            continue;
+
+        path = libxl_sprintf(ctx, "%s/" PCI_BDF, SYSFS_PCIBACK_DRIVER, dom, bus, dev, func);
+        if ( lstat(path, &st) ) {
+            if ( errno == ENOENT )
+                XL_LOG(ctx, XL_LOG_ERROR, PCI_BDF " is not assigned to pciback driver",
+                       dom, bus, dev, func);
+            else
+                XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Couldn't lstat %s", path);
+            closedir(dir);
+            return -1;
+        }
+        (*func_mask) |= (1 << func);
+    }
+
+    closedir(dir);
+    return 0;
+}
+
 static int pci_ins_check(libxl_ctx *ctx, uint32_t domid, const char *state, void *priv)
 {
     char *orig_state = priv;

@@ -679,10 +728,32 @@ int libxl_device_pci_add(libxl_ctx *ctx,
             return rc;
     }
 

tools/libxl/libxl_pci.c int libxl_device_pci_add(libxl_ctx *ctx, · line 742

Quoted 12 lines

tools/libxl/libxl_pci.c +12 -0

+    if ( pcidev->multifunction ) {
+        unsigned int i, orig_vdev, func_mask, rc = 0;
+        orig_vdev = pcidev->vdevfn & ~7U;
+
+        if ( pci_multifunction_check(ctx, pcidev, &func_mask) )
+            return ERROR_FAIL;
+        if ( !(pcidev->vdevfn >> 3) ) {
+            XL_LOG(ctx, XL_LOG_ERROR, "Must specify a v-slot for multi-function devices");
+            return ERROR_FAIL;
+        }
+
+        for(i = 7; i < 8; --i) {

shouldn't this be for(i = 7; i >= 0; --i)?

Quoted 54 lines

tools/libxl/libxl_pci.c +24 -6

+            if ( (1 << i) & func_mask ) {
+                pcidev->func = i;
+                pcidev->vdevfn = orig_vdev | i;
+                if ( do_pci_add(ctx, domid, pcidev) )
+                    rc = ERROR_FAIL;
+            }
+        }
+        return rc;
+    }
+
     return do_pci_add(ctx, domid, pcidev);
 }
 
-int libxl_device_pci_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev)
+static int do_pci_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev)
 {
     libxl_device_pci *assigned;
     char *path;

@@ -711,10 +782,15 @@ int libxl_device_pci_remove(libxl_ctx *c
         libxl_xs_write(ctx, XBT_NULL, path, PCI_BDF, pcidev->domain,
                        pcidev->bus, pcidev->dev, pcidev->func);
         path = libxl_sprintf(ctx, "/local/domain/0/device-model/%d/command", domid);
-        xs_write(ctx->xsh, XBT_NULL, path, "pci-rem", strlen("pci-rem"));
-        if (libxl_wait_for_device_model(ctx, domid, "pci-removed", NULL, NULL) < 0) {
-            XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn't respond in time");
-            return ERROR_FAIL;
+
+        /* Remove all functions at once atomically by only signalling
+         * device-model for function 0 */
+        if ( !pcidev->multifunction || pcidev->func == 0 ) {
+            xs_write(ctx->xsh, XBT_NULL, path, "pci-rem", strlen("pci-rem"));
+            if (libxl_wait_for_device_model(ctx, domid, "pci-removed", NULL, NULL) < 0) {
+                XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn't respond in time");
+                return ERROR_FAIL;
+            }
         }
         path = libxl_sprintf(ctx, "/local/domain/0/device-model/%d/state", domid);
         xs_write(ctx->xsh, XBT_NULL, path, state, strlen(state));

@@ -769,7 +845,10 @@ skip1:
         fclose(f);
     }
 out:
-    libxl_device_pci_reset(ctx, pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+    /* don't do multiple resets while some functions are still passed through */
+    if ( !pcidev->multifunction || pcidev->func == 0 ) {
+        libxl_device_pci_reset(ctx, pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+    }
 
     if (!libxl_is_stubdom(ctx, domid, NULL)) {
         rc = xc_deassign_device(ctx->xch, domid, pcidev->value);

@@ -786,6 +865,29 @@ out:
     return 0;
 }
 

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

Quoted 10 lines

tools/libxl/libxl_pci.c +10 -0

+int libxl_device_pci_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev)
+{
+    if ( pcidev->multifunction ) {
+        unsigned int i, orig_vdev, func_mask, rc;
+        orig_vdev = pcidev->vdevfn & ~7U;
+
+        if ( pci_multifunction_check(ctx, pcidev, &func_mask) )
+            return ERROR_FAIL;
+
+        for(i = 7; i < 8; --i) {

same here

Quoted 12 lines

tools/libxl/libxl_pci.c +12 -0

+            if ( (1 << i) & func_mask ) {
+                pcidev->func = i;
+                pcidev->vdevfn = orig_vdev | i;
+                if ( do_pci_remove(ctx, domid, pcidev) )
+                    rc = ERROR_FAIL;
+            }
+        }
+        return rc;
+    }
+
+    return do_pci_remove(ctx, domid, pcidev);
+}

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

Quoted 5 lines

tools/libxl/libxl_pci.c +1 -0

+
 int libxl_device_pci_list_assigned(libxl_ctx *ctx, libxl_device_pci **list, uint32_t domid, int *num)
 {
     char *be_path, *num_devs, *xsdev, *xsvdevfn, *xsopts;

applied all the other patches in the series

Quoted 51 lines · Stefano Stabellini

Quoted 50 lines · On Mon, 2 Aug 2010, Gianni Tedesco (3P) wrote:

 tools/libxl/libxl.h     |    1 +
 tools/libxl/libxl_pci.c |  118 ++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 111 insertions(+), 8 deletions(-)

# HG changeset patch
# User Gianni Tedesco <[email protected]> # Date 1280760698 -3600
# Branch pci-patches-v3
# Node ID 6379343447e58cc7b83e27beba9c35e62c232d9d # Parent eb8ee481bc063b18b39feaa76d9b757331ed3a9f
xl: implement multifunction device PCI pass-through

Implement PCI pass-through for multi-function devices. The supported BDF notation is: BB:DD.* - therefore passing-through a subset of functions or remapping the function numbers is not supported. Largely because I don't see how that can ever be safe or sane (perhaps for SR-IOV cards?)

Letting qemu automatically select the virtual slot is not supported since I don't believe that qemu-dm can actually handle that case.

Signed-off-by Gianni Tedesco <[email protected]>

diff -r eb8ee481bc06 -r 6379343447e5 tools/libxl/libxl.h

tools/libxl/libxl.h +1 -0

@@ -308,6 +308,7 @@ typedef struct {
     unsigned int vdevfn;
     bool msitranslate;
     bool power_mgmt;
+    bool multifunction;
 } libxl_device_pci;
 
 enum {

diff -r eb8ee481bc06 -r 6379343447e5 tools/libxl/libxl_pci.c

tools/libxl/libxl_pci.c +7 -2

@@ -136,8 +136,13 @@ int libxl_device_pci_parse_bdf(libxl_ctx
                     break;
                 }
                 *ptr = '\0';
-                if ( hex_convert(tok, &func, 0x7) )
-                    goto parse_error;
+                if ( !strcmp(tok, "*") ) {
+                    pcidev->multifunction = 1;
+                }else{
+                    if ( hex_convert(tok, &func, 0x7) )
+                        goto parse_error;
+                    pcidev->multifunction = 0;
+                }

Quoted 8 lines

tools/libxl/libxl_pci.c int libxl_device_pci_parse_bdf(libxl_ctx · line 148

Quoted 3 lines

tools/libxl/libxl_pci.c +0 -0

                 tok = ptr + 1;
             }
             break;

Couldn't you add the func_mask somewhere in libxl_device_pci, instead of calling pci_multifunction_check multilple times?
Would be possible to make the normal passthrough case just a subset of the general multifunction passthrough case to simplify the code?

Not sure what you mean by this, the parsing parts don't (and oughtn't) frob about with sysfs nodes etc. The pci_multifunction_check function is called once per device add and once per device remove iff the parser had seen XX:YY.* notation indicating multifunction devices.

Quoted 9 lines

tools/libxl/libxl_pci.c int libxl_device_pci_list_assignable(lib · line 540

Quoted 7 lines

tools/libxl/libxl_pci.c +2 -0

@@ -531,6 +536,50 @@ int libxl_device_pci_list_assignable(lib
     return 0;
 }
 
+static int pci_multifunction_check(libxl_ctx *ctx, libxl_device_pci *pcidev, unsigned int *func_mask)
+{

a comment on top of this function to explain what it is actually checking would be nice

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 on that device.

I shall add the comment in next rev.

Quoted 49 lines

Quoted 49 lines

tools/libxl/libxl_pci.c +42 -0

+    struct dirent *de;
+    DIR *dir;
+
+    *func_mask = 0;
+
+    dir = opendir(SYSFS_PCI_DEV);
+    if ( NULL == dir ) {
+        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Couldn't open %s", SYSFS_PCI_DEV);
+        return -1;
+    }
+
+    while( (de = readdir(dir)) ) {
+        unsigned dom, bus, dev, func;
+        struct stat st;
+        char *path;
+
+        if ( sscanf(de->d_name, PCI_BDF, &dom, &bus, &dev, &func) != 4 )
+            continue;
+        if ( pcidev->domain != dom )
+            continue;
+        if ( pcidev->bus != bus )
+            continue;
+        if ( pcidev->dev != dev )
+            continue;
+
+        path = libxl_sprintf(ctx, "%s/" PCI_BDF, SYSFS_PCIBACK_DRIVER, dom, bus, dev, func);
+        if ( lstat(path, &st) ) {
+            if ( errno == ENOENT )
+                XL_LOG(ctx, XL_LOG_ERROR, PCI_BDF " is not assigned to pciback driver",
+                       dom, bus, dev, func);
+            else
+                XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Couldn't lstat %s", path);
+            closedir(dir);
+            return -1;
+        }
+        (*func_mask) |= (1 << func);
+    }
+
+    closedir(dir);
+    return 0;
+}
+
 static int pci_ins_check(libxl_ctx *ctx, uint32_t domid, const char *state, void *priv)
 {
     char *orig_state = priv;

@@ -679,10 +728,32 @@ int libxl_device_pci_add(libxl_ctx *ctx,
             return rc;
     }
 

Quoted 14 lines

tools/libxl/libxl_pci.c int libxl_device_pci_add(libxl_ctx *ctx, · line 742

Quoted 12 lines

tools/libxl/libxl_pci.c +12 -0

+    if ( pcidev->multifunction ) {
+        unsigned int i, orig_vdev, func_mask, rc = 0;
+        orig_vdev = pcidev->vdevfn & ~7U;
+
+        if ( pci_multifunction_check(ctx, pcidev, &func_mask) )
+            return ERROR_FAIL;
+        if ( !(pcidev->vdevfn >> 3) ) {
+            XL_LOG(ctx, XL_LOG_ERROR, "Must specify a v-slot for multi-function devices");
+            return ERROR_FAIL;
+        }
+
+        for(i = 7; i < 8; --i) {

shouldn't this be for(i = 7; i >= 0; --i)?

That will generate a compiler warning since unsigned int's are always >=
0. The code relies on an underflow condition. However, I could change it to int if you think that makes it clearer.

Quoted 79 lines

Quoted 54 lines

tools/libxl/libxl_pci.c +24 -6

+            if ( (1 << i) & func_mask ) {
+                pcidev->func = i;
+                pcidev->vdevfn = orig_vdev | i;
+                if ( do_pci_add(ctx, domid, pcidev) )
+                    rc = ERROR_FAIL;
+            }
+        }
+        return rc;
+    }
+
     return do_pci_add(ctx, domid, pcidev);
 }
 
-int libxl_device_pci_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev)
+static int do_pci_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev)
 {
     libxl_device_pci *assigned;
     char *path;

@@ -711,10 +782,15 @@ int libxl_device_pci_remove(libxl_ctx *c
         libxl_xs_write(ctx, XBT_NULL, path, PCI_BDF, pcidev->domain,
                        pcidev->bus, pcidev->dev, pcidev->func);
         path = libxl_sprintf(ctx, "/local/domain/0/device-model/%d/command", domid);
-        xs_write(ctx->xsh, XBT_NULL, path, "pci-rem", strlen("pci-rem"));
-        if (libxl_wait_for_device_model(ctx, domid, "pci-removed", NULL, NULL) < 0) {
-            XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn't respond in time");
-            return ERROR_FAIL;
+
+        /* Remove all functions at once atomically by only signalling
+         * device-model for function 0 */
+        if ( !pcidev->multifunction || pcidev->func == 0 ) {
+            xs_write(ctx->xsh, XBT_NULL, path, "pci-rem", strlen("pci-rem"));
+            if (libxl_wait_for_device_model(ctx, domid, "pci-removed", NULL, NULL) < 0) {
+                XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn't respond in time");
+                return ERROR_FAIL;
+            }
         }
         path = libxl_sprintf(ctx, "/local/domain/0/device-model/%d/state", domid);
         xs_write(ctx->xsh, XBT_NULL, path, state, strlen(state));

@@ -769,7 +845,10 @@ skip1:
         fclose(f);
     }
 out:
-    libxl_device_pci_reset(ctx, pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+    /* don't do multiple resets while some functions are still passed through */
+    if ( !pcidev->multifunction || pcidev->func == 0 ) {
+        libxl_device_pci_reset(ctx, pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+    }
 
     if (!libxl_is_stubdom(ctx, domid, NULL)) {
         rc = xc_deassign_device(ctx->xch, domid, pcidev->value);

@@ -786,6 +865,29 @@ out:
     return 0;
 }
 

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

Quoted 10 lines

tools/libxl/libxl_pci.c +10 -0

+int libxl_device_pci_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev)
+{
+    if ( pcidev->multifunction ) {
+        unsigned int i, orig_vdev, func_mask, rc;
+        orig_vdev = pcidev->vdevfn & ~7U;
+
+        if ( pci_multifunction_check(ctx, pcidev, &func_mask) )
+            return ERROR_FAIL;
+
+        for(i = 7; i < 8; --i) {

same here

Quoted 12 lines

tools/libxl/libxl_pci.c +12 -0

+            if ( (1 << i) & func_mask ) {
+                pcidev->func = i;
+                pcidev->vdevfn = orig_vdev | i;
+                if ( do_pci_remove(ctx, domid, pcidev) )
+                    rc = ERROR_FAIL;
+            }
+        }
+        return rc;
+    }
+
+    return do_pci_remove(ctx, domid, pcidev);
+}

Quoted 8 lines

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

Quoted 5 lines

tools/libxl/libxl_pci.c +1 -0

+
 int libxl_device_pci_list_assigned(libxl_ctx *ctx, libxl_device_pci **list, uint32_t domid, int *num)
 {
     char *be_path, *num_devs, *xsdev, *xsvdevfn, *xsopts;

applied all the other patches in the series

OK thanks.

I will also need to handle cleanup in the error path of multifunction device addition as discussed. This can actually happen quite easily if function 0 alone is assigned to another domain because pci_multifunction_check() doesn't look in xenstore and also because of missing safety checks. Specifically, when adding a non-multifunction device we need a check to make sure that the physical device really has only one function.

I am thinking that we probably ought to scrap the XX:YY.* notation and enforce that function is ALWAYS set to 0. Then when adding/removing the device we should check for additional functions and handle them transparently to the user. As I said in the commit message I am unaware of any valid uses for separating functions but I need to look at how SR-IOV works.

Quoted 61 lines · On Wed, 4 Aug 2010, Gianni Tedesco (3P) wrote:

Quoted 51 lines · Stefano Stabellini

Quoted 50 lines · On Mon, 2 Aug 2010, Gianni Tedesco (3P) wrote:

 tools/libxl/libxl.h     |    1 +
 tools/libxl/libxl_pci.c |  118 ++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 111 insertions(+), 8 deletions(-)

# HG changeset patch
# User Gianni Tedesco <[email protected]> # Date 1280760698 -3600
# Branch pci-patches-v3
# Node ID 6379343447e58cc7b83e27beba9c35e62c232d9d # Parent eb8ee481bc063b18b39feaa76d9b757331ed3a9f
xl: implement multifunction device PCI pass-through

Implement PCI pass-through for multi-function devices. The supported BDF notation is: BB:DD.* - therefore passing-through a subset of functions or remapping the function numbers is not supported. Largely because I don't see how that can ever be safe or sane (perhaps for SR-IOV cards?)

Letting qemu automatically select the virtual slot is not supported since I don't believe that qemu-dm can actually handle that case.

Signed-off-by Gianni Tedesco <[email protected]>

diff -r eb8ee481bc06 -r 6379343447e5 tools/libxl/libxl.h

tools/libxl/libxl.h +1 -0

@@ -308,6 +308,7 @@ typedef struct {
     unsigned int vdevfn;
     bool msitranslate;
     bool power_mgmt;
+    bool multifunction;
 } libxl_device_pci;
 
 enum {

diff -r eb8ee481bc06 -r 6379343447e5 tools/libxl/libxl_pci.c

tools/libxl/libxl_pci.c +7 -2

@@ -136,8 +136,13 @@ int libxl_device_pci_parse_bdf(libxl_ctx
                     break;
                 }
                 *ptr = '\0';
-                if ( hex_convert(tok, &func, 0x7) )
-                    goto parse_error;
+                if ( !strcmp(tok, "*") ) {
+                    pcidev->multifunction = 1;
+                }else{
+                    if ( hex_convert(tok, &func, 0x7) )
+                        goto parse_error;
+                    pcidev->multifunction = 0;
+                }

Quoted 8 lines

tools/libxl/libxl_pci.c int libxl_device_pci_parse_bdf(libxl_ctx · line 148

Quoted 3 lines

tools/libxl/libxl_pci.c +0 -0

                 tok = ptr + 1;
             }
             break;

Couldn't you add the func_mask somewhere in libxl_device_pci, instead of calling pci_multifunction_check multilple times?
Would be possible to make the normal passthrough case just a subset of the general multifunction passthrough case to simplify the code?

Quoted 5 lines

Not sure what you mean by this, the parsing parts don't (and oughtn't) frob about with sysfs nodes etc. The pci_multifunction_check function is called once per device add and once per device remove iff the parser had seen XX:YY.* notation indicating multifunction devices.

I mean we could have a mask instead of an interger in libxl_device_pci to specify which functions should be assigned and in the single function case the mask will have only one bit set.
At this point single function passthrough become just a case of multifunction passthrough.
I am not entirely sure that this approach would produce better code though.

Quoted 81 lines

Quoted 10 lines

tools/libxl/libxl_pci.c int libxl_device_pci_list_assignable(lib · line 540

Quoted 8 lines

tools/libxl/libxl_pci.c +2 -0

@@ -531,6 +536,50 @@ int libxl_device_pci_list_assignable(lib
     return 0;
 }
 
+static int pci_multifunction_check(libxl_ctx *ctx, libxl_device_pci *pcidev, unsigned int *func_mask)
+{

a comment on top of this function to explain what it is actually checking would be nice

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 on that device.

I shall add the comment in next rev.

Quoted 63 lines

Quoted 51 lines

tools/libxl/libxl_pci.c +44 -0

+    struct dirent *de;
+    DIR *dir;
+
+    *func_mask = 0;
+
+    dir = opendir(SYSFS_PCI_DEV);
+    if ( NULL == dir ) {
+        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Couldn't open %s", SYSFS_PCI_DEV);
+        return -1;
+    }
+
+    while( (de = readdir(dir)) ) {
+        unsigned dom, bus, dev, func;
+        struct stat st;
+        char *path;
+
+        if ( sscanf(de->d_name, PCI_BDF, &dom, &bus, &dev, &func) != 4 )
+            continue;
+        if ( pcidev->domain != dom )
+            continue;
+        if ( pcidev->bus != bus )
+            continue;
+        if ( pcidev->dev != dev )
+            continue;
+
+        path = libxl_sprintf(ctx, "%s/" PCI_BDF, SYSFS_PCIBACK_DRIVER, dom, bus, dev, func);
+        if ( lstat(path, &st) ) {
+            if ( errno == ENOENT )
+                XL_LOG(ctx, XL_LOG_ERROR, PCI_BDF " is not assigned to pciback driver",
+                       dom, bus, dev, func);
+            else
+                XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Couldn't lstat %s", path);
+            closedir(dir);
+            return -1;
+        }
+        (*func_mask) |= (1 << func);
+    }
+
+    closedir(dir);
+    return 0;
+}
+
 static int pci_ins_check(libxl_ctx *ctx, uint32_t domid, const char *state, void *priv)
 {
     char *orig_state = priv;

@@ -679,10 +728,32 @@ int libxl_device_pci_add(libxl_ctx *ctx,
             return rc;
     }
 
+    if ( pcidev->multifunction ) {
+        unsigned int i, orig_vdev, func_mask, rc = 0;

tools/libxl/libxl_pci.c int libxl_device_pci_add(libxl_ctx *ctx, · line 742

Quoted 10 lines

tools/libxl/libxl_pci.c +10 -0

+        orig_vdev = pcidev->vdevfn & ~7U;
+
+        if ( pci_multifunction_check(ctx, pcidev, &func_mask) )
+            return ERROR_FAIL;
+        if ( !(pcidev->vdevfn >> 3) ) {
+            XL_LOG(ctx, XL_LOG_ERROR, "Must specify a v-slot for multi-function devices");
+            return ERROR_FAIL;
+        }
+
+        for(i = 7; i < 8; --i) {

shouldn't this be for(i = 7; i >= 0; --i)?

Quoted 4 lines

That will generate a compiler warning since unsigned int's are always >=
0. The code relies on an underflow condition. However, I could change it to int if you think that makes it clearer.

yeah please do that

Quoted 98 lines

Quoted 79 lines

Quoted 54 lines

tools/libxl/libxl_pci.c +24 -6

+            if ( (1 << i) & func_mask ) {
+                pcidev->func = i;
+                pcidev->vdevfn = orig_vdev | i;
+                if ( do_pci_add(ctx, domid, pcidev) )
+                    rc = ERROR_FAIL;
+            }
+        }
+        return rc;
+    }
+
     return do_pci_add(ctx, domid, pcidev);
 }
 
-int libxl_device_pci_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev)
+static int do_pci_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev)
 {
     libxl_device_pci *assigned;
     char *path;

@@ -711,10 +782,15 @@ int libxl_device_pci_remove(libxl_ctx *c
         libxl_xs_write(ctx, XBT_NULL, path, PCI_BDF, pcidev->domain,
                        pcidev->bus, pcidev->dev, pcidev->func);
         path = libxl_sprintf(ctx, "/local/domain/0/device-model/%d/command", domid);
-        xs_write(ctx->xsh, XBT_NULL, path, "pci-rem", strlen("pci-rem"));
-        if (libxl_wait_for_device_model(ctx, domid, "pci-removed", NULL, NULL) < 0) {
-            XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn't respond in time");
-            return ERROR_FAIL;
+
+        /* Remove all functions at once atomically by only signalling
+         * device-model for function 0 */
+        if ( !pcidev->multifunction || pcidev->func == 0 ) {
+            xs_write(ctx->xsh, XBT_NULL, path, "pci-rem", strlen("pci-rem"));
+            if (libxl_wait_for_device_model(ctx, domid, "pci-removed", NULL, NULL) < 0) {
+                XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn't respond in time");
+                return ERROR_FAIL;
+            }
         }
         path = libxl_sprintf(ctx, "/local/domain/0/device-model/%d/state", domid);
         xs_write(ctx->xsh, XBT_NULL, path, state, strlen(state));

@@ -769,7 +845,10 @@ skip1:
         fclose(f);
     }
 out:
-    libxl_device_pci_reset(ctx, pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+    /* don't do multiple resets while some functions are still passed through */
+    if ( !pcidev->multifunction || pcidev->func == 0 ) {
+        libxl_device_pci_reset(ctx, pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
+    }
 
     if (!libxl_is_stubdom(ctx, domid, NULL)) {
         rc = xc_deassign_device(ctx->xch, domid, pcidev->value);

@@ -786,6 +865,29 @@ out:
     return 0;
 }
 

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

Quoted 10 lines

tools/libxl/libxl_pci.c +10 -0

+int libxl_device_pci_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev)
+{
+    if ( pcidev->multifunction ) {
+        unsigned int i, orig_vdev, func_mask, rc;
+        orig_vdev = pcidev->vdevfn & ~7U;
+
+        if ( pci_multifunction_check(ctx, pcidev, &func_mask) )
+            return ERROR_FAIL;
+
+        for(i = 7; i < 8; --i) {

same here

Quoted 12 lines

tools/libxl/libxl_pci.c +12 -0

+            if ( (1 << i) & func_mask ) {
+                pcidev->func = i;
+                pcidev->vdevfn = orig_vdev | i;
+                if ( do_pci_remove(ctx, domid, pcidev) )
+                    rc = ERROR_FAIL;
+            }
+        }
+        return rc;
+    }
+
+    return do_pci_remove(ctx, domid, pcidev);
+}

Quoted 8 lines

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

Quoted 5 lines

tools/libxl/libxl_pci.c +1 -0

+
 int libxl_device_pci_list_assigned(libxl_ctx *ctx, libxl_device_pci **list, uint32_t domid, int *num)
 {
     char *be_path, *num_devs, *xsdev, *xsvdevfn, *xsopts;

applied all the other patches in the series

OK thanks.

I will also need to handle cleanup in the error path of multifunction device addition as discussed. This can actually happen quite easily if function 0 alone is assigned to another domain because pci_multifunction_check() doesn't look in xenstore and also because of missing safety checks. Specifically, when adding a non-multifunction device we need a check to make sure that the physical device really has only one function.

Quoted 8 lines

I am thinking that we probably ought to scrap the XX:YY.* notation and enforce that function is ALWAYS set to 0. Then when adding/removing the device we should check for additional functions and handle them transparently to the user. As I said in the commit message I am unaware of any valid uses for separating functions but I need to look at how SR-IOV works.

With SR-IOV devices you can easily have valid BDFs like 0000:05:1c.2, so enforcing that function is always 0 is not a good idea.