Introduce basic PCI safety checks in libxl and centralize BDF parsing

PATCH New Gianni Tedesco 4 participants 19 messages Jul 28, 2010 – Jul 30, 2010

lore

Share & tools

lore

Gianni Tedesco

author

2010-07-28 19:40 UTC #0 lore raw

PATCH 1/1 Introduce basic PCI safety checks in libxl and centralize BDF parsing

This patch series follows on from previous patch moving PCI code in to libxl_pci.c

Changes from previous patch-series are thus:
1. Move pci FLR function in to libxl_pci.c - got it all in now.
2. Properly initialize pci device structs in new code
3. Incorporate suggestions from Stefano wrt. API and putting sysfs paths in macros.
4. Rename libxl_device_pci_list to libxl_device_pci_list_assigned due to change in parameters for consistency with the rest of libxl PCI API.
5. Also introduced a patch to centralise parsing of PCI BDF's and allow omission of the PCI domain as a short-hand for both config files and hot-plug command parameters. This also fixes an infinite loop in xl create if there is a parse error in the pci config.

Gianni Tedesco

author

2010-07-28 19:40 UTC #1 lore raw

PATCH 1/1 Move libxl_device_pci_reset in to libxl_pci.c

 tools/libxl/libxl_device.c |  34 ----------------------------------
 tools/libxl/libxl_pci.c    |  33 +++++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 34 deletions(-)

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

tools/libxl/libxl_device.c +0 ~~-34~~

diff -r ebede381efe8 -r e2520c8b96b7 tools/libxl/libxl_device.c
@@ -377,40 +377,6 @@ int libxl_device_del(struct libxl_ctx *c
     return 0;
 }
 
-int libxl_device_pci_reset(struct libxl_ctx *ctx, unsigned int domain, unsigned int bus,
-                         unsigned int dev, unsigned int func)
-{
-    char *reset = "/sys/bus/pci/drivers/pciback/do_flr";
-    int fd, rc;
-
-    fd = open(reset, O_WRONLY);
-    if (fd > 0) {
-        char *buf = libxl_sprintf(ctx, PCI_BDF, domain, bus, dev, func);
-        rc = write(fd, buf, strlen(buf));
-        if (rc < 0)
-            XL_LOG(ctx, XL_LOG_ERROR, "write to %s returned %d", reset, rc);
-        close(fd);
-        return rc < 0 ? rc : 0;
-    }
-    if (errno != ENOENT)
-        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Failed to access pciback path %s", reset);
-    reset = libxl_sprintf(ctx, "/sys/bus/pci/devices/"PCI_BDF"/reset", domain, bus, dev, func);
-    fd = open(reset, O_WRONLY);
-    if (fd > 0) {
-        rc = write(fd, "1", 1);
-        if (rc < 0)
-            XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "write to %s returned %d", reset, rc);
-        close(fd);
-        return rc < 0 ? rc : 0;
-    }
-    if (errno == ENOENT) {
-        XL_LOG(ctx, XL_LOG_ERROR, "The kernel doesn't support PCI device reset from sysfs");
-    } else {
-        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Failed to access reset path %s", reset);
-    }
-    return -1;
-}
-
 int libxl_wait_for_device_model(struct libxl_ctx *ctx,
                                 uint32_t domid, char *state,
                                 int (*check_callback)(struct libxl_ctx *ctx,

tools/libxl/libxl_pci.c +33 -0

diff -r ebede381efe8 -r e2520c8b96b7 tools/libxl/libxl_pci.c
@@ -536,3 +536,36 @@ int libxl_device_pci_init(libxl_device_p
     return 0;
 }
 
+int libxl_device_pci_reset(struct libxl_ctx *ctx, unsigned int domain, unsigned int bus,
+                         unsigned int dev, unsigned int func)
+{
+    char *reset = "/sys/bus/pci/drivers/pciback/do_flr";
+    int fd, rc;
+
+    fd = open(reset, O_WRONLY);
+    if (fd > 0) {
+        char *buf = libxl_sprintf(ctx, PCI_BDF, domain, bus, dev, func);
+        rc = write(fd, buf, strlen(buf));
+        if (rc < 0)
+            XL_LOG(ctx, XL_LOG_ERROR, "write to %s returned %d", reset, rc);
+        close(fd);
+        return rc < 0 ? rc : 0;
+    }
+    if (errno != ENOENT)
+        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Failed to access pciback path %s", reset);
+    reset = libxl_sprintf(ctx, "/sys/bus/pci/devices/"PCI_BDF"/reset", domain, bus, dev, func);
+    fd = open(reset, O_WRONLY);
+    if (fd > 0) {
+        rc = write(fd, "1", 1);
+        if (rc < 0)
+            XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "write to %s returned %d", reset, rc);
+        close(fd);
+        return rc < 0 ? rc : 0;
+    }
+    if (errno == ENOENT) {
+        XL_LOG(ctx, XL_LOG_ERROR, "The kernel doesn't support PCI device reset from sysfs");
+    } else {
+        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Failed to access reset path %s", reset);
+    }
+    return -1;
+}

Ian Jackson

2010-07-29 15:37 UTC #2 lore raw

Re: [PATCH 1 of 3] Move libxl_device_pci_reset in to libxl_pci.c

Gianni Tedesco writes ("[Xen-devel] [PATCH 1 of 3] Move libxl_device_pci_reset in to libxl_pci.c"):

Quoted 3 lines

 tools/libxl/libxl_device.c |  34 ----------------------------------
 tools/libxl/libxl_pci.c    |  33 +++++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 34 deletions(-)

This patch does not apply to tip. Have you pulled recently ? I guess Ian Campbell's struct typedef consistency patch is causing the conflicts.

Ian.

Gianni Tedesco

author

2010-07-29 15:42 UTC #3 lore raw

Re: [PATCH 1 of 3] Move libxl_device_pci_reset in to libxl_pci.c

Quoted 8 lines · Ian Jackson

Gianni Tedesco writes ("[Xen-devel] [PATCH 1 of 3] Move libxl_device_pci_reset in to libxl_pci.c"):
Quoted 3 lines
 tools/libxl/libxl_device.c |  34 ----------------------------------
 tools/libxl/libxl_pci.c    |  33 +++++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 34 deletions(-)
This patch does not apply to tip. Have you pulled recently ? I guess Ian Campbell's struct typedef consistency patch is causing the conflicts.

Damnit, pulled (cloned afresh infact) right before putting together the patch set. Things are just moving to fast to keep up :)

Will re-base and re-send.

Gianni Tedesco

author

2010-07-29 15:48 UTC #4 lore raw

Re: [PATCH 1 of 3] Move libxl_device_pci_reset in to libxl_pci.c

In reply to Gianni Tedesco

Quoted 6 lines · Gianni Tedesco

tools/libxl/libxl_device.c | 34 ----------------------------------
 tools/libxl/libxl_pci.c    |  33 +++++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 34 deletions(-)
Signed-off-by Gianni Tedesco <[email protected]>

tools/libxl/libxl_device.c +0 ~~-34~~

diff -r ebede381efe8 -r 6d858d8546d4 tools/libxl/libxl_device.c
@@ -377,40 +377,6 @@ int libxl_device_del(libxl_ctx *c
     return 0;
 }
 
-int libxl_device_pci_reset(libxl_ctx *ctx, unsigned int domain, unsigned int bus,
-                         unsigned int dev, unsigned int func)
-{
-    char *reset = "/sys/bus/pci/drivers/pciback/do_flr";
-    int fd, rc;
-
-    fd = open(reset, O_WRONLY);
-    if (fd > 0) {
-        char *buf = libxl_sprintf(ctx, PCI_BDF, domain, bus, dev, func);
-        rc = write(fd, buf, strlen(buf));
-        if (rc < 0)
-            XL_LOG(ctx, XL_LOG_ERROR, "write to %s returned %d", reset, rc);
-        close(fd);
-        return rc < 0 ? rc : 0;
-    }
-    if (errno != ENOENT)
-        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Failed to access pciback path %s", reset);
-    reset = libxl_sprintf(ctx, "/sys/bus/pci/devices/"PCI_BDF"/reset", domain, bus, dev, func);
-    fd = open(reset, O_WRONLY);
-    if (fd > 0) {
-        rc = write(fd, "1", 1);
-        if (rc < 0)
-            XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "write to %s returned %d", reset, rc);
-        close(fd);
-        return rc < 0 ? rc : 0;
-    }
-    if (errno == ENOENT) {
-        XL_LOG(ctx, XL_LOG_ERROR, "The kernel doesn't support PCI device reset from sysfs");
-    } else {
-        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Failed to access reset path %s", reset);
-    }
-    return -1;
-}
-
 int libxl_wait_for_device_model(libxl_ctx *ctx,
                                 uint32_t domid, char *state,
                                 int (*check_callback)(libxl_ctx *ctx,

tools/libxl/libxl_pci.c +33 -0

diff -r ebede381efe8 -r 6d858d8546d4 tools/libxl/libxl_pci.c
@@ -536,3 +536,36 @@ int libxl_device_pci_init(libxl_device_p
     return 0;
 }
 
+int libxl_device_pci_reset(libxl_ctx *ctx, unsigned int domain, unsigned int bus,
+                         unsigned int dev, unsigned int func)
+{
+    char *reset = "/sys/bus/pci/drivers/pciback/do_flr";
+    int fd, rc;
+
+    fd = open(reset, O_WRONLY);
+    if (fd > 0) {
+        char *buf = libxl_sprintf(ctx, PCI_BDF, domain, bus, dev, func);
+        rc = write(fd, buf, strlen(buf));
+        if (rc < 0)
+            XL_LOG(ctx, XL_LOG_ERROR, "write to %s returned %d", reset, rc);
+        close(fd);
+        return rc < 0 ? rc : 0;
+    }
+    if (errno != ENOENT)
+        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Failed to access pciback path %s", reset);
+    reset = libxl_sprintf(ctx, "/sys/bus/pci/devices/"PCI_BDF"/reset", domain, bus, dev, func);
+    fd = open(reset, O_WRONLY);
+    if (fd > 0) {
+        rc = write(fd, "1", 1);
+        if (rc < 0)
+            XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "write to %s returned %d", reset, rc);
+        close(fd);
+        return rc < 0 ? rc : 0;
+    }
+    if (errno == ENOENT) {
+        XL_LOG(ctx, XL_LOG_ERROR, "The kernel doesn't support PCI device reset from sysfs");
+    } else {
+        XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Failed to access reset path %s", reset);
+    }
+    return -1;
+}

Gianni Tedesco

author

2010-07-28 19:40 UTC #5 lore raw

PATCH 1/1 Check whether a PCI device is assignable before assigning it do a domU

In reply to Gianni Tedesco

 tools/libxl/libxl.h          |    3 +-
 tools/libxl/libxl_internal.h |    3 +
 tools/libxl/libxl_pci.c      |  186 +++++++++++++++++++++++++++++++++++++++---
 tools/libxl/xl.h             |    1 +
 tools/libxl/xl_cmdimpl.c     |   36 +++++++-
 tools/libxl/xl_cmdtable.c    |    5 +
 6 files changed, 214 insertions(+), 20 deletions(-)

Implement a new libxl function libxl_device_pci_list_assignable. This is used to implement the xl list-assignable-pci-devices command and part of the implementation is used to make sure that PCI devices are not multiply assigned to one or more domU's before doing the passthrough assignment.

The function libxl_device_pci_list changes to libxl_device_pci_list_assigned due to a parameter change for consistency with pci_list_assignable.

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

tools/libxl/libxl.h +2 -1

diff -r e2520c8b96b7 -r 099e4eb3803a tools/libxl/libxl.h
@@ -521,7 +521,8 @@ int libxl_device_vfb_hard_shutdown(struc
 int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev);
 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);
+int libxl_device_pci_list_assigned(struct libxl_ctx *ctx, libxl_device_pci **list, uint32_t domid, int *num);
+int libxl_device_pci_list_assignable(struct libxl_ctx *ctx, libxl_device_pci **list, 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);

tools/libxl/libxl_internal.h +3 -0

diff -r e2520c8b96b7 -r 099e4eb3803a tools/libxl/libxl_internal.h
@@ -91,6 +91,9 @@ typedef struct {
 #define XC_PCI_BDF             "0x%x, 0x%x, 0x%x, 0x%x"
 #define AUTO_PHP_SLOT          0x100
 #define SYSFS_PCI_DEV          "/sys/bus/pci/devices"
+#define SYSFS_PCIBACK_DRIVER   "/sys/bus/pci/drivers/pciback"
+#define SYSFS_PCISTUB_DRIVER   "/sys/bus/pci/drivers/pcistub"
+
 #define PROC_PCI_NUM_RESOURCES 7
 #define PCI_BAR_IO             0x01

tools/libxl/libxl_pci.c +169 ~~-17~~

232 lines not on this page · raw message

tools/libxl/xl.h +1 -0

diff -r e2520c8b96b7 -r 099e4eb3803a tools/libxl/xl.h
@@ -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);

tools/libxl/xl_cmdimpl.c +34 -2

diff -r e2520c8b96b7 -r 099e4eb3803a tools/libxl/xl_cmdimpl.c
@@ -1920,6 +1920,39 @@ int main_vncviewer(int argc, char **argv
     exit(0);
 }
 
+void pcilist_assignable(void)
+{
+    libxl_device_pci *pcidevs;
+    int num, i;
+
+    if ( libxl_device_pci_list_assignable(&ctx, &pcidevs, &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;

@@ -1927,8 +1960,7 @@ void pcilist(char *dom)
 
     find_domain(dom);
 
-    pcidevs = libxl_device_pci_list(&ctx, domid, &num);
-    if (!num)
+    if (libxl_device_pci_list_assigned(&ctx, &pcidevs, domid, &num))
         return;
     printf("VFn  domain bus  slot func\n");
     for (i = 0; i < num; i++) {

tools/libxl/xl_cmdtable.c +5 -0

diff -r e2520c8b96b7 -r 099e4eb3803a tools/libxl/xl_cmdtable.c
@@ -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",

Stefano Stabellini

2010-07-29 14:36 UTC #6 lore raw

Re: [PATCH 2 of 3] Check whether a PCI device is assignable before assigning it do a domU

Quoted 57 lines · On Wed, 28 Jul 2010, Gianni Tedesco (3P) wrote:

 tools/libxl/libxl.h          |    3 +-
 tools/libxl/libxl_internal.h |    3 +
 tools/libxl/libxl_pci.c      |  186 +++++++++++++++++++++++++++++++++++++++---
 tools/libxl/xl.h             |    1 +
 tools/libxl/xl_cmdimpl.c     |   36 +++++++-
 tools/libxl/xl_cmdtable.c    |    5 +
 6 files changed, 214 insertions(+), 20 deletions(-)

The function libxl_device_pci_list changes to libxl_device_pci_list_assigned due to a parameter change for consistency with pci_list_assignable.

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

diff -r e2520c8b96b7 -r 099e4eb3803a tools/libxl/libxl.h

tools/libxl/libxl.h +2 -1

@@ -521,7 +521,8 @@ int libxl_device_vfb_hard_shutdown(struc
 int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev);
 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);
+int libxl_device_pci_list_assigned(struct libxl_ctx *ctx, libxl_device_pci **list, uint32_t domid, int *num);
+int libxl_device_pci_list_assignable(struct libxl_ctx *ctx, libxl_device_pci **list, 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 e2520c8b96b7 -r 099e4eb3803a tools/libxl/libxl_internal.h

tools/libxl/libxl_internal.h +3 -0

@@ -91,6 +91,9 @@ typedef struct {
 #define XC_PCI_BDF             "0x%x, 0x%x, 0x%x, 0x%x"
 #define AUTO_PHP_SLOT          0x100
 #define SYSFS_PCI_DEV          "/sys/bus/pci/devices"
+#define SYSFS_PCIBACK_DRIVER   "/sys/bus/pci/drivers/pciback"
+#define SYSFS_PCISTUB_DRIVER   "/sys/bus/pci/drivers/pcistub"
+
 #define PROC_PCI_NUM_RESOURCES 7
 #define PCI_BAR_IO             0x01

diff -r e2520c8b96b7 -r 099e4eb3803a tools/libxl/libxl_pci.c

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

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

Quoted 5 lines

tools/libxl/libxl_pci.c +1 -1

@@ -258,22 +259,77 @@ retry_transaction2:
     return 0;
 }
 
-int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev)
+int get_all_assigned_devices(struct libxl_ctx *ctx, libxl_device_pci **list, int *num)

shouldn't this be static?

Quoted 171 lines

tools/libxl/libxl_pci.c +141 ~~-12~~

+{
+    libxl_device_pci *pcidevs = NULL;
+    char **domlist;
+    unsigned int nd = 0, i;
+
+    *list = NULL;
+    *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;
+
+            pcidevs = calloc(sizeof(*pcidevs), ndev);
+            for(j = (pcidevs) ? 0 : ndev; 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;
+
+                    libxl_device_pci_init(pcidevs + *num, dom, bus, dev, func, 0);
+                    (*num)++;
+                }
+            }
+        }
+    }
+
+    if ( 0 == *num ) {
+        free(pcidevs);
+        pcidevs = NULL;
+    }else{
+        *list = pcidevs;
+    }
+
+    return 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;
+}
+
+static int do_pci_add(struct libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev)
 {
     char *path;
     char *state, *vdevfn;
     int rc, hvm;
-    int stubdomid = 0;
-
-    /* TODO: check if the device can be assigned */
-
-    libxl_device_pci_reset(ctx, pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
-
-    stubdomid = libxl_get_stubdom_id(ctx, domid);
-    if (stubdomid != 0) {
-        libxl_device_pci pcidev_s = *pcidev;
-        libxl_device_pci_add(ctx, stubdomid, &pcidev_s);
-    }
 
     hvm = is_hvm(ctx, domid);
     if (hvm) {

@@ -370,6 +426,38 @@ out:
     return 0;
 }
 
+int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev)
+{
+    libxl_device_pci *assigned;
+    int num_assigned, rc;
+    int stubdomid = 0;
+
+    rc = get_all_assigned_devices(ctx, &assigned, &num_assigned);
+    if ( rc ) {
+        XL_LOG(ctx, XL_LOG_ERROR, "cannot determine if device is assigned, refusing to continue");
+        return ERROR_FAIL;
+    }
+    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);
+
+    stubdomid = libxl_get_stubdom_id(ctx, domid);
+    if (stubdomid != 0) {
+        libxl_device_pci pcidev_s = *pcidev;
+        rc = do_pci_add(ctx, stubdomid, &pcidev_s);
+        if ( rc )
+            return rc;
+    }
+
+    return do_pci_add(ctx, domid, pcidev);
+}
+
 int libxl_device_pci_remove(struct libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev)
 {
     char *path;

@@ -466,7 +554,66 @@ out:
     return 0;
 }
 
-libxl_device_pci *libxl_device_pci_list(struct libxl_ctx *ctx, uint32_t domid, int *num)
+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;
+
+        memset(new, 0, sizeof(*new));
+        libxl_device_pci_init(new, dom, bus, dev, func, 0);
+        (*num)++;
+    }
+
+    closedir(dir);
+    return pcidevs;
+}
+
+int libxl_device_pci_list_assignable(struct libxl_ctx *ctx, libxl_device_pci **list, int *num)
+{
+    libxl_device_pci *pcidevs = NULL;
+    libxl_device_pci *assigned;
+    int num_assigned, rc;
+
+    *num = 0;
+    *list = NULL;

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

Quoted 10 lines

tools/libxl/libxl_pci.c +10 -0

+
+    rc = get_all_assigned_devices(ctx, &assigned, &num_assigned);
+    if ( rc )
+        return rc;
+
+    pcidevs = scan_sys_pcidir(pcidevs, assigned, num_assigned,
+                              SYSFS_PCIBACK_DRIVER, num);
+    pcidevs = scan_sys_pcidir(pcidevs, assigned, num_assigned,
+                              SYSFS_PCISTUB_DRIVER, num);
+

I still think we should remove pcistub scanning from here: when running on xen people should use pciback anyway.

Quoted 123 lines

tools/libxl/libxl_pci.c +16 -4

+    free(assigned);
+    if ( *num )
+        *list = pcidevs;
+    return 0;
+}
+
+int libxl_device_pci_list_assigned(struct libxl_ctx *ctx, libxl_device_pci **list, uint32_t domid, int *num)
 {
     char *be_path, *num_devs, *xsdev, *xsvdevfn, *xsopts;
     int n, i;

@@ -477,7 +624,8 @@ libxl_device_pci *libxl_device_pci_list(
     num_devs = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/num_devs", be_path));
     if (!num_devs) {
         *num = 0;
-        return NULL;
+        *list = NULL;
+        return ERROR_FAIL;
     }
     n = atoi(num_devs);
     pcidevs = calloc(n, sizeof(libxl_device_pci));

@@ -507,15 +655,19 @@ libxl_device_pci *libxl_device_pci_list(
             } while ((p = strtok_r(NULL, ",=", &saveptr)) != NULL);
         }
     }
-    return pcidevs;
+    if ( *num )
+        *list = pcidevs;
+    return 0;
 }
 
 int libxl_device_pci_shutdown(struct libxl_ctx *ctx, uint32_t domid)
 {
     libxl_device_pci *pcidevs;
-    int num, i;
+    int num, i, rc;
 
-    pcidevs = libxl_device_pci_list(ctx, domid, &num);
+    rc = libxl_device_pci_list_assigned(ctx, &pcidevs, domid, &num);
+    if ( rc )
+        return rc;
     for (i = 0; i < num; i++) {
         if (libxl_device_pci_remove(ctx, domid, pcidevs + i) < 0)
             return ERROR_FAIL;

diff -r e2520c8b96b7 -r 099e4eb3803a 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 e2520c8b96b7 -r 099e4eb3803a tools/libxl/xl_cmdimpl.c

tools/libxl/xl_cmdimpl.c +34 -2

@@ -1920,6 +1920,39 @@ int main_vncviewer(int argc, char **argv
     exit(0);
 }
 
+void pcilist_assignable(void)
+{
+    libxl_device_pci *pcidevs;
+    int num, i;
+
+    if ( libxl_device_pci_list_assignable(&ctx, &pcidevs, &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;

@@ -1927,8 +1960,7 @@ void pcilist(char *dom)
 
     find_domain(dom);
 
-    pcidevs = libxl_device_pci_list(&ctx, domid, &num);
-    if (!num)
+    if (libxl_device_pci_list_assigned(&ctx, &pcidevs, domid, &num))
         return;
     printf("VFn  domain bus  slot func\n");
     for (i = 0; i < num; i++) {

diff -r e2520c8b96b7 -r 099e4eb3803a 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",

Gianni Tedesco

author

2010-07-29 15:32 UTC #7 lore raw

Re: [PATCH 2 of 3] Check whether a PCI device is assignable before assigning it do a domU

Quoted 4 lines · Stefano Stabellini

Review

Quoted 2 lines

(quoted diff) +1 -1

-int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev)
+int get_all_assigned_devices(struct libxl_ctx *ctx, libxl_device_pci **list, int *num)

shouldn't this be static?

Indeed yes:

8<----------------

The function libxl_device_pci_list changes to libxl_device_pci_list_assigned due to a parameter change for consistency with pci_list_assignable.

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

tools/libxl/libxl.h +2 -1

diff -r 5345595da04b tools/libxl/libxl.h
@@ -521,7 +521,8 @@ int libxl_device_vfb_hard_shutdown(struc
 int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev);
 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);
+int libxl_device_pci_list_assigned(struct libxl_ctx *ctx, libxl_device_pci **list, uint32_t domid, int *num);
+int libxl_device_pci_list_assignable(struct libxl_ctx *ctx, libxl_device_pci **list, 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);

tools/libxl/libxl_internal.h +3 -0

diff -r 5345595da04b tools/libxl/libxl_internal.h
@@ -91,6 +91,9 @@ typedef struct {
 #define XC_PCI_BDF             "0x%x, 0x%x, 0x%x, 0x%x"
 #define AUTO_PHP_SLOT          0x100
 #define SYSFS_PCI_DEV          "/sys/bus/pci/devices"
+#define SYSFS_PCIBACK_DRIVER   "/sys/bus/pci/drivers/pciback"
+#define SYSFS_PCISTUB_DRIVER   "/sys/bus/pci/drivers/pcistub"
+
 #define PROC_PCI_NUM_RESOURCES 7
 #define PCI_BAR_IO             0x01

tools/libxl/libxl_pci.c +169 ~~-17~~

232 lines not on this page · raw message

tools/libxl/xl.h +1 -0

diff -r 5345595da04b tools/libxl/xl.h
@@ -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);

tools/libxl/xl_cmdimpl.c +34 -2

diff -r 5345595da04b tools/libxl/xl_cmdimpl.c
@@ -1920,6 +1920,39 @@ int main_vncviewer(int argc, char **argv
     exit(0);
 }
 
+void pcilist_assignable(void)
+{
+    libxl_device_pci *pcidevs;
+    int num, i;
+
+    if ( libxl_device_pci_list_assignable(&ctx, &pcidevs, &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;

@@ -1927,8 +1960,7 @@ void pcilist(char *dom)
 
     find_domain(dom);
 
-    pcidevs = libxl_device_pci_list(&ctx, domid, &num);
-    if (!num)
+    if (libxl_device_pci_list_assigned(&ctx, &pcidevs, domid, &num))
         return;
     printf("VFn  domain bus  slot func\n");
     for (i = 0; i < num; i++) {

tools/libxl/xl_cmdtable.c +5 -0

diff -r 5345595da04b tools/libxl/xl_cmdtable.c
@@ -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",

Gianni Tedesco

author

2010-07-29 15:49 UTC #8 lore raw

Re: [PATCH 2 of 3] Check whether a PCI device is assignable before assigning it do a domU

In reply to Gianni Tedesco

Implement both of Stefanos suggestions this time:

Quoted 14 lines · Gianni Tedesco

tools/libxl/libxl.h | 3 +-
 tools/libxl/libxl_internal.h |    3 +
 tools/libxl/libxl_pci.c      |  186 +++++++++++++++++++++++++++++++++++++++---
 tools/libxl/xl.h             |    1 +
 tools/libxl/xl_cmdimpl.c     |   36 +++++++-
 tools/libxl/xl_cmdtable.c    |    5 +
 6 files changed, 214 insertions(+), 20 deletions(-)
Implement a new libxl function libxl_device_pci_list_assignable. This is used to implement the xl list-assignable-pci-devices command and part of the implementation is used to make sure that PCI devices are not multiply assigned to one or more domU's before doing the passthrough assignment.

Quoted 4 lines

The function libxl_device_pci_list changes to libxl_device_pci_list_assigned due to a parameter change for consistency with pci_list_assignable.
Signed-off-by Gianni Tedesco <[email protected]>

tools/libxl/libxl.h +2 -1

diff -r 43150fdd0106 tools/libxl/libxl.h
@@ -521,7 +521,8 @@ int libxl_device_vfb_hard_shutdown(libxl
 int libxl_device_pci_add(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev);
 int libxl_device_pci_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev);
 int libxl_device_pci_shutdown(libxl_ctx *ctx, uint32_t domid);
-libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid, int *num);
+int libxl_device_pci_list_assigned(libxl_ctx *ctx, libxl_device_pci **list, uint32_t domid, int *num);
+int libxl_device_pci_list_assignable(libxl_ctx *ctx, libxl_device_pci **list, 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);

tools/libxl/libxl_internal.h +2 -0

diff -r 43150fdd0106 tools/libxl/libxl_internal.h
@@ -91,6 +91,8 @@ typedef struct {
 #define XC_PCI_BDF             "0x%x, 0x%x, 0x%x, 0x%x"
 #define AUTO_PHP_SLOT          0x100
 #define SYSFS_PCI_DEV          "/sys/bus/pci/devices"
+#define SYSFS_PCIBACK_DRIVER   "/sys/bus/pci/drivers/pciback"
+
 #define PROC_PCI_NUM_RESOURCES 7
 #define PCI_BAR_IO             0x01

tools/libxl/libxl_pci.c +167 ~~-17~~

230 lines not on this page · raw message

tools/libxl/xl.h +1 -0

diff -r 43150fdd0106 tools/libxl/xl.h
@@ -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);

tools/libxl/xl_cmdimpl.c +34 -2

diff -r 43150fdd0106 tools/libxl/xl_cmdimpl.c
@@ -1920,6 +1920,39 @@ int main_vncviewer(int argc, char **argv
     exit(0);
 }
 
+void pcilist_assignable(void)
+{
+    libxl_device_pci *pcidevs;
+    int num, i;
+
+    if ( libxl_device_pci_list_assignable(&ctx, &pcidevs, &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;

@@ -1927,8 +1960,7 @@ void pcilist(char *dom)
 
     find_domain(dom);
 
-    pcidevs = libxl_device_pci_list(&ctx, domid, &num);
-    if (!num)
+    if (libxl_device_pci_list_assigned(&ctx, &pcidevs, domid, &num))
         return;
     printf("VFn  domain bus  slot func\n");
     for (i = 0; i < num; i++) {

tools/libxl/xl_cmdtable.c +5 -0

diff -r 43150fdd0106 tools/libxl/xl_cmdtable.c
@@ -69,6 +69,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",

Stefano Stabellini

Acked-by

2010-07-29 16:00 UTC #9 lore raw

Re: [PATCH 2 of 3] Check whether a PCI device is assignable before assigning it do a domU

Quoted 22 lines · On Thu, 29 Jul 2010, Gianni Tedesco (3P) wrote:

Implement both of Stefanos suggestions this time:
Quoted 18 lines · Gianni Tedesco
tools/libxl/libxl.h | 3 +-
 tools/libxl/libxl_internal.h |    3 +
 tools/libxl/libxl_pci.c      |  186 +++++++++++++++++++++++++++++++++++++++---
 tools/libxl/xl.h             |    1 +
 tools/libxl/xl_cmdimpl.c     |   36 +++++++-
 tools/libxl/xl_cmdtable.c    |    5 +
 6 files changed, 214 insertions(+), 20 deletions(-)
Implement a new libxl function libxl_device_pci_list_assignable. This is used to implement the xl list-assignable-pci-devices command and part of the implementation is used to make sure that PCI devices are not multiply assigned to one or more domU's before doing the passthrough assignment.
The function libxl_device_pci_list changes to libxl_device_pci_list_assigned due to a parameter change for consistency with pci_list_assignable.
Signed-off-by Gianni Tedesco <[email protected]>

Acked-by Stefano Stabellini <[email protected]>

Ian Campbell

2010-07-30 15:21 UTC #10 lore raw

Re: [PATCH 2 of 3] Check whether a PCI device is assignable before assigning it do a domU

In reply to Gianni Tedesco

tools/libxl/libxl_pci.c libxl_device_pci *libxl_device_pci_list( · line 673

Quoted 13 lines · Gianni Tedesco

int libxl_device_pci_shutdown(struct libxl_ctx *ctx, uint32_t domid)

 {
     libxl_device_pci *pcidevs;

tools/libxl/libxl_pci.c +4 -2

-    int num, i;
+    int num, i, rc;
 
-    pcidevs = libxl_device_pci_list(ctx, domid, &num);
+    rc = libxl_device_pci_list_assigned(ctx, &pcidevs, domid, &num);
+    if ( rc )
+        return rc;
     for (i = 0; i < num; i++) {
         if (libxl_device_pci_remove(ctx, domid, pcidevs + i) < 0)
             return ERROR_FAIL;

I have a feeling this is responsible for my getting this message:
libxl: error: libxl.c:851:libxl_domain_destroy pci shutdown failed for domid 2
when shutting down a PV domain which has no PCI devices assigned to it.

Presumably this is because libxl_device_pci_list_assigned() returns error if "backend/pci/%d/0/num_devs" does not exist in xenstore, which it won't for a domain with no devices passed through.

Ian.

Gianni Tedesco

author

2010-07-30 15:50 UTC #11 lore raw

Re: [PATCH 2 of 3] Check whether a PCI device is assignable before assigning it do a domU

Quoted 19 lines · Ian Campbell

tools/libxl/libxl_pci.c libxl_device_pci *libxl_device_pci_list( · line 673
Quoted 13 lines · Gianni Tedesco
int libxl_device_pci_shutdown(struct libxl_ctx *ctx, uint32_t domid)
 {
     libxl_device_pci *pcidevs;
tools/libxl/libxl_pci.c +4 -2
-    int num, i;
+    int num, i, rc;
 
-    pcidevs = libxl_device_pci_list(ctx, domid, &num);
+    rc = libxl_device_pci_list_assigned(ctx, &pcidevs, domid, &num);
+    if ( rc )
+        return rc;
     for (i = 0; i < num; i++) {
         if (libxl_device_pci_remove(ctx, domid, pcidevs + i) < 0)
             return ERROR_FAIL; 
I have a feeling this is responsible for my getting this message:
libxl: error: libxl.c:851:libxl_domain_destroy pci shutdown failed for domid 2
when shutting down a PV domain which has no PCI devices assigned to it.

Quoted 5 lines

Presumably this is because libxl_device_pci_list_assigned() returns error if "backend/pci/%d/0/num_devs" does not exist in xenstore, which it won't for a domain with no devices passed through.
Ian.

Thanks, spotted this late last night and fixed in my tree. Will be posting an updated patchbomb early next week.

Gianni Tedesco

author

2010-07-28 19:40 UTC #12 lore raw

PATCH 1/1 Centralize parsing of PCI BDF's in xl

In reply to Gianni Tedesco

 tools/libxl/libxl.h      |   6 +---
 tools/libxl/libxl_pci.c  |  71 +++++++++++++++++++++++++++++++++++++----------
 tools/libxl/xl_cmdimpl.c |  46 ++++--------------------------
 3 files changed, 64 insertions(+), 59 deletions(-)

Introduce a new libxl call libxl_device_pci_parse_bdf() and use it consistently.

This patch also fixes an infinite loop bug in xl create. If there is a parse-error on any pci config file entry then xl will attempt to skip it, but since num_pcidevs is used to index the config list and is not incremented when skipping, this caused an infinite loop. Solve that problem by introducing a new loop counter variable.

Note that virtual PCI slots are not parsed by the new code. However this is a step towards support for virtual slots and multi-function device assignment since only one location in the code will need to be changed now.

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

tools/libxl/libxl.h +1 -5

diff -r 099e4eb3803a -r e49597460a1a tools/libxl/libxl.h
@@ -516,16 +516,12 @@ int libxl_device_vfb_add(struct libxl_ct
 int libxl_device_vfb_clean_shutdown(struct libxl_ctx *ctx, uint32_t domid);
 int libxl_device_vfb_hard_shutdown(struct libxl_ctx *ctx, uint32_t domid);
 
-#define PCI_BDF                "%04x:%02x:%02x.%01x"
-#define PCI_BDF_VDEVFN         "%04x:%02x:%02x.%01x@%02x"
 int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev);
 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);
 int libxl_device_pci_list_assigned(struct libxl_ctx *ctx, libxl_device_pci **list, uint32_t domid, int *num);
 int libxl_device_pci_list_assignable(struct libxl_ctx *ctx, libxl_device_pci **list, 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);
+int libxl_device_pci_parse_bdf(libxl_device_pci *pcidev, const char *str);
 
 /*
  * Functions for allowing users of libxl to store private data

tools/libxl/libxl_pci.c +56 ~~-15~~

101 lines not on this page · raw message

tools/libxl/xl_cmdimpl.c +7 ~~-39~~

91 lines not on this page · raw message

Stefano Stabellini

Acked-by on v1 1/1

2010-07-29 14:42 UTC #13 lore raw

Re: [PATCH 3 of 3] Centralize parsing of PCI BDF's in xl

Quoted 15 lines · On Wed, 28 Jul 2010, Gianni Tedesco (3P) wrote:

 tools/libxl/libxl.h      |   6 +---
 tools/libxl/libxl_pci.c  |  71 +++++++++++++++++++++++++++++++++++++----------
 tools/libxl/xl_cmdimpl.c |  46 ++++--------------------------
 3 files changed, 64 insertions(+), 59 deletions(-)
Introduce a new libxl call libxl_device_pci_parse_bdf() and use it consistently.
This patch also fixes an infinite loop bug in xl create. If there is a parse-error on any pci config file entry then xl will attempt to skip it, but since num_pcidevs is used to index the config list and is not incremented when skipping, this caused an infinite loop. Solve that problem by introducing a new loop counter variable.

Quoted 6 lines

Note that virtual PCI slots are not parsed by the new code. However this is a step towards support for virtual slots and multi-function device assignment since only one location in the code will need to be changed now.
Signed-off-by Gianni Tedesco <[email protected]>

Acked-by Stefano Stabellini <[email protected]>

Gianni Tedesco

author

2010-07-29 15:33 UTC #14 lore raw

Re: [PATCH 3 of 3] Centralize parsing of PCI BDF's in xl

Quoted 20 lines · Stefano Stabellini

Quoted 19 lines · On Wed, 28 Jul 2010, Gianni Tedesco (3P) wrote:
 tools/libxl/libxl.h      |   6 +---
 tools/libxl/libxl_pci.c  |  71 +++++++++++++++++++++++++++++++++++++----------
 tools/libxl/xl_cmdimpl.c |  46 ++++--------------------------
 3 files changed, 64 insertions(+), 59 deletions(-)
Introduce a new libxl call libxl_device_pci_parse_bdf() and use it consistently.
This patch also fixes an infinite loop bug in xl create. If there is a parse-error on any pci config file entry then xl will attempt to skip it, but since num_pcidevs is used to index the config list and is not incremented when skipping, this caused an infinite loop. Solve that problem by introducing a new loop counter variable.
Note that virtual PCI slots are not parsed by the new code. However this is a step towards support for virtual slots and multi-function device assignment since only one location in the code will need to be changed now.

Quoted 5 lines

Quoted 2 lines
Signed-off-by Gianni Tedesco <[email protected]>
Acked-by Stefano Stabellini <[email protected]>

In that case please apply the following, fixed an incorrect removal of memset in pciattach/pcidetach.

tools/libxl/libxl.h +1 -5

diff -r bab9294a9b90 tools/libxl/libxl.h
@@ -516,16 +516,12 @@ int libxl_device_vfb_add(struct libxl_ct
 int libxl_device_vfb_clean_shutdown(struct libxl_ctx *ctx, uint32_t domid);
 int libxl_device_vfb_hard_shutdown(struct libxl_ctx *ctx, uint32_t domid);
 
-#define PCI_BDF                "%04x:%02x:%02x.%01x"
-#define PCI_BDF_VDEVFN         "%04x:%02x:%02x.%01x@%02x"
 int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev);
 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);
 int libxl_device_pci_list_assigned(struct libxl_ctx *ctx, libxl_device_pci **list, uint32_t domid, int *num);
 int libxl_device_pci_list_assignable(struct libxl_ctx *ctx, libxl_device_pci **list, 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);
+int libxl_device_pci_parse_bdf(libxl_device_pci *pcidev, const char *str);
 
 /*
  * Functions for allowing users of libxl to store private data

tools/libxl/libxl_pci.c +56 ~~-15~~

101 lines not on this page · raw message

tools/libxl/xl_cmdimpl.c +7 ~~-37~~

91 lines not on this page · raw message

Gianni Tedesco

author

2010-07-29 15:50 UTC #15 lore raw

Re: [PATCH 3 of 3] Centralize parsing of PCI BDF's in xl

In reply to Gianni Tedesco

Quoted 15 lines · Gianni Tedesco

tools/libxl/libxl.h      |   6 +---
 tools/libxl/libxl_pci.c  |  71 +++++++++++++++++++++++++++++++++++++----------
 tools/libxl/xl_cmdimpl.c |  46 ++++--------------------------
 3 files changed, 64 insertions(+), 59 deletions(-)
Introduce a new libxl call libxl_device_pci_parse_bdf() and use it consistently.
This patch also fixes an infinite loop bug in xl create. If there is a parse-error on any pci config file entry then xl will attempt to skip it, but since num_pcidevs is used to index the config list and is not incremented when skipping, this caused an infinite loop. Solve that problem by introducing a new loop counter variable.

Quoted 5 lines

Note that virtual PCI slots are not parsed by the new code. However this is a step towards support for virtual slots and multi-function device assignment since only one location in the code will need to be changed now.
Signed-off-by Gianni Tedesco <[email protected]>

tools/libxl/libxl.h +1 -5

diff -r bab9294a9b90 tools/libxl/libxl.h
@@ -516,16 +516,12 @@ int libxl_device_vfb_add(struct libxl_ct
 int libxl_device_vfb_clean_shutdown(libxl_ctx *ctx, uint32_t domid);
 int libxl_device_vfb_hard_shutdown(libxl_ctx *ctx, uint32_t domid);
 
-#define PCI_BDF                "%04x:%02x:%02x.%01x"
-#define PCI_BDF_VDEVFN         "%04x:%02x:%02x.%01x@%02x"
 int libxl_device_pci_add(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev);
 int libxl_device_pci_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev);
 int libxl_device_pci_shutdown(libxl_ctx *ctx, uint32_t domid);
 int libxl_device_pci_list_assigned(libxl_ctx *ctx, libxl_device_pci **list, uint32_t domid, int *num);
 int libxl_device_pci_list_assignable(libxl_ctx *ctx, libxl_device_pci **list, 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);
+int libxl_device_pci_parse_bdf(libxl_device_pci *pcidev, const char *str);
 
 /*
  * Functions for allowing users of libxl to store private data

tools/libxl/libxl_pci.c +56 ~~-15~~

101 lines not on this page · raw message

tools/libxl/xl_cmdimpl.c +7 ~~-37~~

91 lines not on this page · raw message

Ian Jackson

2010-07-29 17:54 UTC #16 lore raw

Re: [PATCH 3 of 3] Centralize parsing of PCI BDF's in xl

Gianni Tedesco writes ("Re: [Xen-devel] [PATCH 3 of 3] Centralize parsing of PCI BDF's in xl"):

tools/libxl/xl_cmdimpl.c static void printf_info(int domid, · line 488

Quoted 2 lines

tools/libxl/xl_cmdimpl.c +1 -1

-        printf("\t\t\t(pci dev "PCI_BDF_VDEVFN")\n",
+        printf("\t\t\t(pci dev %04x:%02x:%02x.%01x@%02x)\n",

I like this whole patch apart from this one line :-). Perhaps PCI_BDF_VDEVFN should be LIBXL_PCI_BDF_VDEVFN and be exported in libxl.h ?

Ian.

Gianni Tedesco

author

2010-07-29 17:56 UTC #17 lore raw

Re: [PATCH 3 of 3] Centralize parsing of PCI BDF's in xl

Quoted 9 lines · Ian Jackson

Gianni Tedesco writes ("Re: [Xen-devel] [PATCH 3 of 3] Centralize parsing of PCI BDF's in xl"):
tools/libxl/xl_cmdimpl.c static void printf_info(int domid, · line 488
Quoted 2 lines
tools/libxl/xl_cmdimpl.c +1 -1
-        printf("\t\t\t(pci dev "PCI_BDF_VDEVFN")\n",
+        printf("\t\t\t(pci dev %04x:%02x:%02x.%01x@%02x)\n",
I like this whole patch apart from this one line :-). Perhaps PCI_BDF_VDEVFN should be LIBXL_PCI_BDF_VDEVFN and be exported in libxl.h ?
Ian.

Yeah makes sense, although I am not so keen on exporting bits of format strings because it smells too much like policy. Perhaps xl could do it's own macro for that.

It's your call.

Gianni Tedesco

author

2010-07-29 18:15 UTC #18 lore raw

Re: [PATCH 3 of 3] Centralize parsing of PCI BDF's in xl

In reply to Ian Jackson

Quoted 9 lines · Ian Jackson

Gianni Tedesco writes ("Re: [Xen-devel] [PATCH 3 of 3] Centralize parsing of PCI BDF's in xl"):
tools/libxl/xl_cmdimpl.c static void printf_info(int domid, · line 488
Quoted 2 lines
tools/libxl/xl_cmdimpl.c +1 -1
-        printf("\t\t\t(pci dev "PCI_BDF_VDEVFN")\n",
+        printf("\t\t\t(pci dev %04x:%02x:%02x.%01x@%02x)\n",
I like this whole patch apart from this one line :-). Perhaps PCI_BDF_VDEVFN should be LIBXL_PCI_BDF_VDEVFN and be exported in libxl.h ?
Ian.

Actually thinking about this the format is confusing anyway since the actual notation would be:

printf("%04x:%02x:%02x.%01x=%01x@%02x", domain, bus, dev, func, vdevfn & 0x7, vdevfn >> 3);

Whatever you do with it shall be getting cleaned up anyway in my forthcoming patch-series which actually parses and implements virtual slots and multi-function devices with function remapping.