* [PATCH 0 of 2] [v2] Add per-device and global permissive config options for pci passthrough
@ 2012-04-03 13:54 George Dunlap
2012-04-03 13:54 ` [PATCH 1 of 2] libxl: Move bdf parsing into libxlu George Dunlap
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: George Dunlap @ 2012-04-03 13:54 UTC (permalink / raw)
To: xen-devel; +Cc: george.dunlap
This patch series adds the "permissive" option for passed-through devices
which access config space out of the "known good" PCI config space.
v2:
- Added patch to move bdf parsing to libxlu
- Addressed some formatting comments
v3:
- Don't make the struct initialization utility function public
- Return error on failure
- Wrap a long line
- Clarify security warning / recommendation
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1 of 2] libxl: Move bdf parsing into libxlu 2012-04-03 13:54 [PATCH 0 of 2] [v2] Add per-device and global permissive config options for pci passthrough George Dunlap @ 2012-04-03 13:54 ` George Dunlap 2012-04-03 14:34 ` Ian Jackson 2012-04-03 13:54 ` [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough George Dunlap ` (2 subsequent siblings) 3 siblings, 1 reply; 10+ messages in thread From: George Dunlap @ 2012-04-03 13:54 UTC (permalink / raw) To: xen-devel; +Cc: george.dunlap Config parsing functions do not properly belong in libxl. Move them into libxlu so that others can use them or not as they see fit. No functional changes. One side-effect was making public a private libxl utility function which just set the elements of a structure from the function arguments passed in. v2: - Didn't make libxl_pci_dev_init() a public function; renamed to be more descriptive Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com> diff -r f744e82ea740 -r 74bb52e4f6a6 tools/libxl/Makefile --- a/tools/libxl/Makefile Wed Feb 29 16:30:34 2012 +0000 +++ b/tools/libxl/Makefile Mon Apr 02 15:54:08 2012 +0100 @@ -57,7 +57,7 @@ LIBXL_OBJS += _libxl_types.o libxl_flask AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \ - libxlu_disk_l.o libxlu_disk.o + libxlu_disk_l.o libxlu_disk.o libxlu_pci.o $(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h CLIENTS = xl testidl diff -r f744e82ea740 -r 74bb52e4f6a6 tools/libxl/libxl.h --- a/tools/libxl/libxl.h Wed Feb 29 16:30:34 2012 +0000 +++ b/tools/libxl/libxl.h Mon Apr 02 15:54:08 2012 +0100 @@ -575,13 +575,6 @@ int libxl_device_pci_destroy(libxl_ctx * libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid, int *num); /* - * Parse a PCI BDF into a PCI device structure. - */ -int libxl_device_pci_parse_bdf(libxl_ctx *ctx, - libxl_device_pci *pcidev, - const char *str); - -/* * Similar to libxl_device_pci_list but returns all devices which * could be assigned to a domain (i.e. are bound to the backend * driver) but are not currently. diff -r f744e82ea740 -r 74bb52e4f6a6 tools/libxl/libxl_pci.c --- a/tools/libxl/libxl_pci.c Wed Feb 29 16:30:34 2012 +0000 +++ b/tools/libxl/libxl_pci.c Mon Apr 02 15:54:08 2012 +0100 @@ -34,9 +34,9 @@ static unsigned int pcidev_encode_bdf(li return value; } -static int pcidev_init(libxl_device_pci *pcidev, unsigned int domain, - unsigned int bus, unsigned int dev, - unsigned int func, unsigned int vdevfn) +static int pcidev_struct_fill(libxl_device_pci *pcidev, unsigned int domain, + unsigned int bus, unsigned int dev, + unsigned int func, unsigned int vdevfn) { pcidev->domain = domain; pcidev->bus = bus; @@ -46,149 +46,6 @@ static int pcidev_init(libxl_device_pci return 0; } -static int hex_convert(const char *str, unsigned int *val, unsigned int mask) -{ - unsigned long ret; - char *end; - - ret = strtoul(str, &end, 16); - if ( end == str || *end != '\0' ) - return -1; - if ( ret & ~mask ) - return -1; - *val = (unsigned int)ret & mask; - return 0; -} - -#define STATE_DOMAIN 0 -#define STATE_BUS 1 -#define STATE_DEV 2 -#define STATE_FUNC 3 -#define STATE_VSLOT 4 -#define STATE_OPTIONS_K 6 -#define STATE_OPTIONS_V 7 -#define STATE_TERMINAL 8 -int libxl_device_pci_parse_bdf(libxl_ctx *ctx, libxl_device_pci *pcidev, const char *str) -{ - unsigned state = STATE_DOMAIN; - unsigned dom, bus, dev, func, vslot = 0; - char *buf2, *tok, *ptr, *end, *optkey = NULL; - - if ( NULL == (buf2 = ptr = strdup(str)) ) - return ERROR_NOMEM; - - for(tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++) { - switch(state) { - case STATE_DOMAIN: - if ( *ptr == ':' ) { - state = STATE_BUS; - *ptr = '\0'; - if ( hex_convert(tok, &dom, 0xffff) ) - goto parse_error; - tok = ptr + 1; - } - break; - case STATE_BUS: - if ( *ptr == ':' ) { - state = STATE_DEV; - *ptr = '\0'; - if ( hex_convert(tok, &bus, 0xff) ) - goto parse_error; - tok = ptr + 1; - }else if ( *ptr == '.' ) { - state = STATE_FUNC; - *ptr = '\0'; - if ( dom & ~0xff ) - goto parse_error; - bus = dom; - dom = 0; - if ( hex_convert(tok, &dev, 0xff) ) - goto parse_error; - tok = ptr + 1; - } - break; - case STATE_DEV: - if ( *ptr == '.' ) { - state = STATE_FUNC; - *ptr = '\0'; - if ( hex_convert(tok, &dev, 0xff) ) - goto parse_error; - tok = ptr + 1; - } - break; - case STATE_FUNC: - if ( *ptr == '\0' || *ptr == '@' || *ptr == ',' ) { - switch( *ptr ) { - case '\0': - state = STATE_TERMINAL; - break; - case '@': - state = STATE_VSLOT; - break; - case ',': - state = STATE_OPTIONS_K; - break; - } - *ptr = '\0'; - if ( !strcmp(tok, "*") ) { - pcidev->vfunc_mask = LIBXL_PCI_FUNC_ALL; - }else{ - if ( hex_convert(tok, &func, 0x7) ) - goto parse_error; - pcidev->vfunc_mask = (1 << 0); - } - tok = ptr + 1; - } - break; - case STATE_VSLOT: - if ( *ptr == '\0' || *ptr == ',' ) { - state = ( *ptr == ',' ) ? STATE_OPTIONS_K : STATE_TERMINAL; - *ptr = '\0'; - if ( hex_convert(tok, &vslot, 0xff) ) - goto parse_error; - tok = ptr + 1; - } - break; - case STATE_OPTIONS_K: - if ( *ptr == '=' ) { - state = STATE_OPTIONS_V; - *ptr = '\0'; - optkey = tok; - tok = ptr + 1; - } - break; - case STATE_OPTIONS_V: - if ( *ptr == ',' || *ptr == '\0' ) { - state = (*ptr == ',') ? STATE_OPTIONS_K : STATE_TERMINAL; - *ptr = '\0'; - if ( !strcmp(optkey, "msitranslate") ) { - pcidev->msitranslate = atoi(tok); - }else if ( !strcmp(optkey, "power_mgmt") ) { - pcidev->power_mgmt = atoi(tok); - }else{ - LIBXL__LOG(ctx, LIBXL__LOG_WARNING, - "Unknown PCI BDF option: %s", optkey); - } - tok = ptr + 1; - } - default: - break; - } - } - - free(buf2); - - if ( tok != ptr || state != STATE_TERMINAL ) - goto parse_error; - - pcidev_init(pcidev, dom, bus, dev, func, vslot << 3); - - return 0; - -parse_error: - return ERROR_INVAL; -} - static void libxl_create_pci_backend_device(libxl__gc *gc, flexarray_t *back, int num, libxl_device_pci *pcidev) { flexarray_append(back, libxl__sprintf(gc, "key-%d", num)); @@ -436,7 +293,7 @@ static int get_all_assigned_devices(libx *list = realloc(*list, sizeof(libxl_device_pci) * ((*num) + 1)); if (*list == NULL) return ERROR_NOMEM; - pcidev_init(*list + *num, dom, bus, dev, func, 0); + pcidev_struct_fill(*list + *num, dom, bus, dev, func, 0); (*num)++; } } @@ -507,7 +364,7 @@ libxl_device_pci *libxl_device_pci_list_ new = pcidevs + *num; memset(new, 0, sizeof(*new)); - pcidev_init(new, dom, bus, dev, func, 0); + pcidev_struct_fill(new, dom, bus, dev, func, 0); (*num)++; } @@ -1086,7 +943,7 @@ static void libxl__device_pci_from_xs_be if (s) vdevfn = strtol(s, (char **) NULL, 16); - pcidev_init(pci, domain, bus, dev, func, vdevfn); + pcidev_struct_fill(pci, domain, bus, dev, func, vdevfn); s = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/opts-%d", be_path, nr)); if (s) { diff -r f744e82ea740 -r 74bb52e4f6a6 tools/libxl/libxlu_pci.c --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tools/libxl/libxlu_pci.c Mon Apr 02 15:54:08 2012 +0100 @@ -0,0 +1,172 @@ +#include "libxl_osdeps.h" /* must come before any other headers */ +#include "libxlu_internal.h" +#include "libxlu_disk_l.h" +#include "libxlu_disk_i.h" +#include "libxlu_cfg_i.h" + + +#define XLU__PCI_ERR(_c, _x, _a...) \ + if((_c) && (_c)->report) fprintf((_c)->report, _x, ##_a) + +static int hex_convert(const char *str, unsigned int *val, unsigned int mask) +{ + unsigned long ret; + char *end; + + ret = strtoul(str, &end, 16); + if ( end == str || *end != '\0' ) + return -1; + if ( ret & ~mask ) + return -1; + *val = (unsigned int)ret & mask; + return 0; +} + +static int pcidev_struct_fill(libxl_device_pci *pcidev, unsigned int domain, + unsigned int bus, unsigned int dev, + unsigned int func, unsigned int vdevfn) +{ + pcidev->domain = domain; + pcidev->bus = bus; + pcidev->dev = dev; + pcidev->func = func; + pcidev->vdevfn = vdevfn; + return 0; +} + +#define STATE_DOMAIN 0 +#define STATE_BUS 1 +#define STATE_DEV 2 +#define STATE_FUNC 3 +#define STATE_VSLOT 4 +#define STATE_OPTIONS_K 6 +#define STATE_OPTIONS_V 7 +#define STATE_TERMINAL 8 +int xlu_pci_parse_bdf(XLU_Config *cfg, libxl_device_pci *pcidev, const char *str) +{ + unsigned state = STATE_DOMAIN; + unsigned dom, bus, dev, func, vslot = 0; + char *buf2, *tok, *ptr, *end, *optkey = NULL; + + if ( NULL == (buf2 = ptr = strdup(str)) ) + return ERROR_NOMEM; + + for(tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++) { + switch(state) { + case STATE_DOMAIN: + if ( *ptr == ':' ) { + state = STATE_BUS; + *ptr = '\0'; + if ( hex_convert(tok, &dom, 0xffff) ) + goto parse_error; + tok = ptr + 1; + } + break; + case STATE_BUS: + if ( *ptr == ':' ) { + state = STATE_DEV; + *ptr = '\0'; + if ( hex_convert(tok, &bus, 0xff) ) + goto parse_error; + tok = ptr + 1; + }else if ( *ptr == '.' ) { + state = STATE_FUNC; + *ptr = '\0'; + if ( dom & ~0xff ) + goto parse_error; + bus = dom; + dom = 0; + if ( hex_convert(tok, &dev, 0xff) ) + goto parse_error; + tok = ptr + 1; + } + break; + case STATE_DEV: + if ( *ptr == '.' ) { + state = STATE_FUNC; + *ptr = '\0'; + if ( hex_convert(tok, &dev, 0xff) ) + goto parse_error; + tok = ptr + 1; + } + break; + case STATE_FUNC: + if ( *ptr == '\0' || *ptr == '@' || *ptr == ',' ) { + switch( *ptr ) { + case '\0': + state = STATE_TERMINAL; + break; + case '@': + state = STATE_VSLOT; + break; + case ',': + state = STATE_OPTIONS_K; + break; + } + *ptr = '\0'; + if ( !strcmp(tok, "*") ) { + pcidev->vfunc_mask = LIBXL_PCI_FUNC_ALL; + }else{ + if ( hex_convert(tok, &func, 0x7) ) + goto parse_error; + pcidev->vfunc_mask = (1 << 0); + } + tok = ptr + 1; + } + break; + case STATE_VSLOT: + if ( *ptr == '\0' || *ptr == ',' ) { + state = ( *ptr == ',' ) ? STATE_OPTIONS_K : STATE_TERMINAL; + *ptr = '\0'; + if ( hex_convert(tok, &vslot, 0xff) ) + goto parse_error; + tok = ptr + 1; + } + break; + case STATE_OPTIONS_K: + if ( *ptr == '=' ) { + state = STATE_OPTIONS_V; + *ptr = '\0'; + optkey = tok; + tok = ptr + 1; + } + break; + case STATE_OPTIONS_V: + if ( *ptr == ',' || *ptr == '\0' ) { + state = (*ptr == ',') ? STATE_OPTIONS_K : STATE_TERMINAL; + *ptr = '\0'; + if ( !strcmp(optkey, "msitranslate") ) { + pcidev->msitranslate = atoi(tok); + }else if ( !strcmp(optkey, "power_mgmt") ) { + pcidev->power_mgmt = atoi(tok); + }else{ + XLU__PCI_ERR(cfg, "Unknown PCI BDF option: %s", optkey); + } + tok = ptr + 1; + } + default: + break; + } + } + + free(buf2); + + if ( tok != ptr || state != STATE_TERMINAL ) + goto parse_error; + + /* Just a pretty way to fill in the values */ + pcidev_struct_fill(pcidev, dom, bus, dev, func, vslot << 3); + + return 0; + +parse_error: + return ERROR_INVAL; +} + +/* + * Local variables: + * mode: C + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff -r f744e82ea740 -r 74bb52e4f6a6 tools/libxl/libxlutil.h --- a/tools/libxl/libxlutil.h Wed Feb 29 16:30:34 2012 +0000 +++ b/tools/libxl/libxlutil.h Mon Apr 02 15:54:08 2012 +0100 @@ -88,6 +88,11 @@ int xlu_disk_parse(XLU_Config *cfg, int * resulting disk struct is used with libxl. */ +/* + * PCI specification parsing + */ +int xlu_pci_parse_bdf(XLU_Config *cfg, libxl_device_pci *pcidev, const char *str); + #endif /* LIBXLUTIL_H */ diff -r f744e82ea740 -r 74bb52e4f6a6 tools/libxl/xl_cmdimpl.c --- a/tools/libxl/xl_cmdimpl.c Wed Feb 29 16:30:34 2012 +0000 +++ b/tools/libxl/xl_cmdimpl.c Mon Apr 02 15:54:08 2012 +0100 @@ -1005,7 +1005,7 @@ skip_vfb: pcidev->msitranslate = pci_msitranslate; pcidev->power_mgmt = pci_power_mgmt; - if (!libxl_device_pci_parse_bdf(ctx, pcidev, buf)) + if (!xlu_pci_parse_bdf(config, pcidev, buf)) d_config->num_pcidevs++; } if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV) @@ -2217,11 +2217,16 @@ int main_pcilist(int argc, char **argv) static void pcidetach(const char *dom, const char *bdf, int force) { libxl_device_pci pcidev; + XLU_Config *config; find_domain(dom); memset(&pcidev, 0x00, sizeof(pcidev)); - if (libxl_device_pci_parse_bdf(ctx, &pcidev, bdf)) { + + config = xlu_cfg_init(stderr, "command line"); + if (!config) { perror("xlu_cfg_inig"); exit(-1); } + + if (xlu_pci_parse_bdf(config, &pcidev, bdf)) { fprintf(stderr, "pci-detach: malformed BDF specification \"%s\"\n", bdf); exit(2); } @@ -2257,11 +2262,16 @@ int main_pcidetach(int argc, char **argv static void pciattach(const char *dom, const char *bdf, const char *vs) { libxl_device_pci pcidev; + XLU_Config *config; find_domain(dom); memset(&pcidev, 0x00, sizeof(pcidev)); - if (libxl_device_pci_parse_bdf(ctx, &pcidev, bdf)) { + + config = xlu_cfg_init(stderr, "command line"); + if (!config) { perror("xlu_cfg_inig"); exit(-1); } + + if (xlu_pci_parse_bdf(config, &pcidev, bdf)) { fprintf(stderr, "pci-attach: malformed BDF specification \"%s\"\n", bdf); exit(2); } diff -r f744e82ea740 -r 74bb52e4f6a6 tools/python/xen/lowlevel/xl/xl.c --- a/tools/python/xen/lowlevel/xl/xl.c Wed Feb 29 16:30:34 2012 +0000 +++ b/tools/python/xen/lowlevel/xl/xl.c Mon Apr 02 15:54:08 2012 +0100 @@ -40,6 +40,7 @@ #include <libxl.h> #include <libxl_utils.h> +#include <libxlutil.h> #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) @@ -556,7 +557,7 @@ static PyObject *pyxl_pci_parse(XlObject return NULL; } - if ( libxl_device_pci_parse_bdf(self->ctx, &pci->obj, str) ) { + if ( xlu_pci_parse_bdf(NULL, &pci->obj, str) ) { PyErr_SetString(xl_error_obj, "cannot parse pci device spec (BDF)"); Py_DECREF(pci); return NULL; ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1 of 2] libxl: Move bdf parsing into libxlu 2012-04-03 13:54 ` [PATCH 1 of 2] libxl: Move bdf parsing into libxlu George Dunlap @ 2012-04-03 14:34 ` Ian Jackson 0 siblings, 0 replies; 10+ messages in thread From: Ian Jackson @ 2012-04-03 14:34 UTC (permalink / raw) To: George Dunlap; +Cc: xen-devel George Dunlap writes ("[Xen-devel] [PATCH 1 of 2] libxl: Move bdf parsing into libxlu"): > Config parsing functions do not properly belong in libxl. Move them into > libxlu so that others can use them or not as they see fit. Acked-by: Ian Jackson <ian.jackson@eu.citrix.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough 2012-04-03 13:54 [PATCH 0 of 2] [v2] Add per-device and global permissive config options for pci passthrough George Dunlap 2012-04-03 13:54 ` [PATCH 1 of 2] libxl: Move bdf parsing into libxlu George Dunlap @ 2012-04-03 13:54 ` George Dunlap 2012-04-03 14:34 ` Ian Jackson 2012-04-03 14:52 ` [PATCH 0 of 2] [v2] " George Dunlap 2012-04-04 15:07 ` Ian Jackson 3 siblings, 1 reply; 10+ messages in thread From: George Dunlap @ 2012-04-03 13:54 UTC (permalink / raw) To: xen-devel; +Cc: george.dunlap By default pciback only allows PV guests to write "known safe" values into PCI config space. But many devices require writes to other areas of config space in order to operate properly. One way to do that is with the "quirks" interface, which specifies areas known safe to a particular device; the other way is to mark a device as "permissive", which tells pciback to allow all config space writes for that domain and device. This adds a "permissive" flag to the libxl_pci struct and teaches libxl how to write the appropriate value into sysfs to enable the permissive feature for devices being passed through. It also adds the permissive config options either on a per-device basis, or as a global option in the xl command-line. Because of the potential stability and security implications of enabling permissive, the flag is left off by default. v3: - Return error on failure - Wrap a long line - Clarify security warning / recommendation Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com> diff -r 74bb52e4f6a6 -r 0625fe50a2ee docs/man/xl.cfg.pod.5 --- a/docs/man/xl.cfg.pod.5 Mon Apr 02 15:54:08 2012 +0100 +++ b/docs/man/xl.cfg.pod.5 Tue Apr 03 14:46:27 2012 +0100 @@ -294,10 +294,31 @@ XXX XXX +=item B<permissive=BOOLEAN> + +(PV only) By default pciback only allows PV guests to write "known +safe" values into PCI config space. But many devices require writes +to other areas of config space in order to operate properly. This +tells the pciback driver to allow all writes to PCI config space of +this device by this domain. This option should be enabled with +caution: it gives the guest much more control over the device, which +may have security or stability implications. It is recommended to +enable this option only for trusted VMs under administrator control. + =back =back +=item B<pci_permissive=BOOLEAN> + +(PV only) Changes the default value of 'permissive' for all PCI +devices for this VM. This can still be overriden on a per-device +basis. This option should be enabled with caution: it gives the guest +much more control over the device, which may have security or +stability implications. It is recommended to enable this option only +for trusted VMs under administrator control. See the "pci=" section +for more information on the "permissive" flag. + =back =head2 Paravirtualised (PV) Guest Specific Options diff -r 74bb52e4f6a6 -r 0625fe50a2ee tools/libxl/libxl_pci.c --- a/tools/libxl/libxl_pci.c Mon Apr 02 15:54:08 2012 +0100 +++ b/tools/libxl/libxl_pci.c Tue Apr 03 14:46:27 2012 +0100 @@ -55,7 +55,10 @@ static void libxl_create_pci_backend_dev if (pcidev->vdevfn) flexarray_append_pair(back, libxl__sprintf(gc, "vdevfn-%d", num), libxl__sprintf(gc, "%x", pcidev->vdevfn)); flexarray_append(back, libxl__sprintf(gc, "opts-%d", num)); - flexarray_append(back, libxl__sprintf(gc, "msitranslate=%d,power_mgmt=%d", pcidev->msitranslate, pcidev->power_mgmt)); + flexarray_append(back, + libxl__sprintf(gc, "msitranslate=%d,power_mgmt=%d,permissive=%d", + pcidev->msitranslate, pcidev->power_mgmt, + pcidev->permissive)); flexarray_append_pair(back, libxl__sprintf(gc, "state-%d", num), libxl__sprintf(gc, "%d", 1)); } @@ -565,6 +568,31 @@ static int do_pci_add(libxl__gc *gc, uin } } fclose(f); + + /* Don't restrict writes to the PCI config space from this VM */ + if (pcidev->permissive) { + int fd; + char *buf; + + sysfs_path = libxl__sprintf(gc, SYSFS_PCIBACK_DRIVER"/permissive"); + fd = open(sysfs_path, O_WRONLY); + if (fd < 0) { + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn't open %s", + sysfs_path); + return ERROR_FAIL; + } + + buf = libxl__sprintf(gc, PCI_BDF, pcidev->domain, pcidev->bus, + pcidev->dev, pcidev->func); + rc = write(fd, buf, strlen(buf)); + /* Annoying to have two if's, but we need the errno */ + if (rc < 0) + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, + "write to %s returned %d", sysfs_path, rc); + close(fd); + if (rc < 0) + return ERROR_FAIL; + } break; } default: @@ -958,6 +986,9 @@ static void libxl__device_pci_from_xs_be } else if (!strcmp(p, "power_mgmt")) { p = strtok_r(NULL, ",=", &saveptr); pci->power_mgmt = atoi(p); + } else if (!strcmp(p, "permissive")) { + p = strtok_r(NULL, ",=", &saveptr); + pci->permissive = atoi(p); } } while ((p = strtok_r(NULL, ",=", &saveptr)) != NULL); } diff -r 74bb52e4f6a6 -r 0625fe50a2ee tools/libxl/libxl_types.idl --- a/tools/libxl/libxl_types.idl Mon Apr 02 15:54:08 2012 +0100 +++ b/tools/libxl/libxl_types.idl Tue Apr 03 14:46:27 2012 +0100 @@ -352,6 +352,7 @@ libxl_device_pci = Struct("device_pci", ("vfunc_mask", uint32), ("msitranslate", bool), ("power_mgmt", bool), + ("permissive", bool), ]) libxl_diskinfo = Struct("diskinfo", [ diff -r 74bb52e4f6a6 -r 0625fe50a2ee tools/libxl/libxlu_pci.c --- a/tools/libxl/libxlu_pci.c Mon Apr 02 15:54:08 2012 +0100 +++ b/tools/libxl/libxlu_pci.c Tue Apr 03 14:46:27 2012 +0100 @@ -139,6 +139,8 @@ int xlu_pci_parse_bdf(XLU_Config *cfg, l pcidev->msitranslate = atoi(tok); }else if ( !strcmp(optkey, "power_mgmt") ) { pcidev->power_mgmt = atoi(tok); + }else if ( !strcmp(optkey, "permissive") ) { + pcidev->permissive = atoi(tok); }else{ XLU__PCI_ERR(cfg, "Unknown PCI BDF option: %s", optkey); } diff -r 74bb52e4f6a6 -r 0625fe50a2ee tools/libxl/xl_cmdimpl.c --- a/tools/libxl/xl_cmdimpl.c Mon Apr 02 15:54:08 2012 +0100 +++ b/tools/libxl/xl_cmdimpl.c Tue Apr 03 14:46:27 2012 +0100 @@ -518,6 +518,7 @@ static void parse_config_data(const char XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids; int pci_power_mgmt = 0; int pci_msitranslate = 1; + int pci_permissive = 0; int e; libxl_domain_create_info *c_info = &d_config->c_info; @@ -986,6 +987,9 @@ skip_vfb: if (!xlu_cfg_get_long (config, "pci_power_mgmt", &l, 0)) pci_power_mgmt = l; + if (!xlu_cfg_get_long (config, "pci_permissive", &l, 0)) + pci_permissive = l; + /* To be reworked (automatically enabled) once the auto ballooning * after guest starts is done (with PCI devices passed in). */ if (c_info->type == LIBXL_DOMAIN_TYPE_PV) { @@ -1005,6 +1009,7 @@ skip_vfb: pcidev->msitranslate = pci_msitranslate; pcidev->power_mgmt = pci_power_mgmt; + pcidev->permissive = pci_permissive; if (!xlu_pci_parse_bdf(config, pcidev, buf)) d_config->num_pcidevs++; } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough 2012-04-03 13:54 ` [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough George Dunlap @ 2012-04-03 14:34 ` Ian Jackson 0 siblings, 0 replies; 10+ messages in thread From: Ian Jackson @ 2012-04-03 14:34 UTC (permalink / raw) To: George Dunlap; +Cc: xen-devel George Dunlap writes ("[Xen-devel] [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough"): > By default pciback only allows PV guests to write "known safe" values into > PCI config space. But many devices require writes to other areas of config > space in order to operate properly. One way to do that is with the "quirks" > interface, which specifies areas known safe to a particular device; the > other way is to mark a device as "permissive", which tells pciback to allow > all config space writes for that domain and device. Acked-by: Ian Jackson <ian.jackson@eu.citrix.com> Ian. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0 of 2] [v2] Add per-device and global permissive config options for pci passthrough 2012-04-03 13:54 [PATCH 0 of 2] [v2] Add per-device and global permissive config options for pci passthrough George Dunlap 2012-04-03 13:54 ` [PATCH 1 of 2] libxl: Move bdf parsing into libxlu George Dunlap 2012-04-03 13:54 ` [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough George Dunlap @ 2012-04-03 14:52 ` George Dunlap 2012-04-04 15:07 ` Ian Jackson 3 siblings, 0 replies; 10+ messages in thread From: George Dunlap @ 2012-04-03 14:52 UTC (permalink / raw) To: xen-devel; +Cc: george.dunlap BTW, that should be "[v3]"... On Tue, Apr 3, 2012 at 2:54 PM, George Dunlap <george.dunlap@eu.citrix.com> wrote: > This patch series adds the "permissive" option for passed-through devices > which access config space out of the "known good" PCI config space. > > v2: > - Added patch to move bdf parsing to libxlu > - Addressed some formatting comments > > v3: > - Don't make the struct initialization utility function public > - Return error on failure > - Wrap a long line > - Clarify security warning / recommendation > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0 of 2] [v2] Add per-device and global permissive config options for pci passthrough 2012-04-03 13:54 [PATCH 0 of 2] [v2] Add per-device and global permissive config options for pci passthrough George Dunlap ` (2 preceding siblings ...) 2012-04-03 14:52 ` [PATCH 0 of 2] [v2] " George Dunlap @ 2012-04-04 15:07 ` Ian Jackson 3 siblings, 0 replies; 10+ messages in thread From: Ian Jackson @ 2012-04-04 15:07 UTC (permalink / raw) To: George Dunlap; +Cc: xen-devel George Dunlap writes ("[Xen-devel] [PATCH 0 of 2] [v2] Add per-device and global permissive config options for pci passthrough"): > This patch series adds the "permissive" option for passed-through devices > which access config space out of the "known good" PCI config space. Applied both, thanks. Ian. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 0 of 2] [v2] Add per-device and global permissive config options for pci passthrough @ 2012-04-02 10:47 George Dunlap 2012-04-02 10:47 ` [PATCH 1 of 2] libxl: Move bdf parsing into libxlu George Dunlap 0 siblings, 1 reply; 10+ messages in thread From: George Dunlap @ 2012-04-02 10:47 UTC (permalink / raw) To: xen-devel; +Cc: george.dunlap This patch series adds the "permissive" option for passed-through devices which access config space out of the "known good" PCI config space. v2: - Added patch to move bdf parsing to libxlu - Addressed some formatting comments ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1 of 2] libxl: Move bdf parsing into libxlu 2012-04-02 10:47 George Dunlap @ 2012-04-02 10:47 ` George Dunlap 2012-04-02 11:05 ` Ian Campbell 0 siblings, 1 reply; 10+ messages in thread From: George Dunlap @ 2012-04-02 10:47 UTC (permalink / raw) To: xen-devel; +Cc: george.dunlap # HG changeset patch # User George Dunlap <george.dunlap@eu.citrix.com> # Date 1333362574 -3600 # Node ID 5386937e6c5c9afaa8a3cd56d391dcc9e40d0596 # Parent f744e82ea74075983de6d5b0ad0cf7ccacf999a2 libxl: Move bdf parsing into libxlu Config parsing functions do not properly belong in libxl. Move them into libxlu so that others can use them or not as they see fit. No functional changes. One side-effect was making public a private libxl utility function which just set the elements of a structure from the function arguments passed in. Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com> diff -r f744e82ea740 -r 5386937e6c5c tools/libxl/Makefile --- a/tools/libxl/Makefile Wed Feb 29 16:30:34 2012 +0000 +++ b/tools/libxl/Makefile Mon Apr 02 11:29:34 2012 +0100 @@ -57,7 +57,7 @@ LIBXL_OBJS += _libxl_types.o libxl_flask AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \ - libxlu_disk_l.o libxlu_disk.o + libxlu_disk_l.o libxlu_disk.o libxlu_pci.o $(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h CLIENTS = xl testidl diff -r f744e82ea740 -r 5386937e6c5c tools/libxl/libxl.h --- a/tools/libxl/libxl.h Wed Feb 29 16:30:34 2012 +0000 +++ b/tools/libxl/libxl.h Mon Apr 02 11:29:34 2012 +0100 @@ -573,13 +573,10 @@ int libxl_device_pci_add(libxl_ctx *ctx, int libxl_device_pci_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev); int libxl_device_pci_destroy(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev); libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid, int *num); - -/* - * Parse a PCI BDF into a PCI device structure. - */ -int libxl_device_pci_parse_bdf(libxl_ctx *ctx, - libxl_device_pci *pcidev, - const char *str); +/* Just initialize the structure elements with the arguments provided. */ +int libxl_pci_dev_init(libxl_device_pci *pcidev, unsigned int domain, + unsigned int bus, unsigned int dev, + unsigned int func, unsigned int vdevfn); /* * Similar to libxl_device_pci_list but returns all devices which diff -r f744e82ea740 -r 5386937e6c5c tools/libxl/libxl_pci.c --- a/tools/libxl/libxl_pci.c Wed Feb 29 16:30:34 2012 +0000 +++ b/tools/libxl/libxl_pci.c Mon Apr 02 11:29:34 2012 +0100 @@ -34,7 +34,7 @@ static unsigned int pcidev_encode_bdf(li return value; } -static int pcidev_init(libxl_device_pci *pcidev, unsigned int domain, +int libxl_pci_dev_init(libxl_device_pci *pcidev, unsigned int domain, unsigned int bus, unsigned int dev, unsigned int func, unsigned int vdevfn) { @@ -46,149 +46,6 @@ static int pcidev_init(libxl_device_pci return 0; } -static int hex_convert(const char *str, unsigned int *val, unsigned int mask) -{ - unsigned long ret; - char *end; - - ret = strtoul(str, &end, 16); - if ( end == str || *end != '\0' ) - return -1; - if ( ret & ~mask ) - return -1; - *val = (unsigned int)ret & mask; - return 0; -} - -#define STATE_DOMAIN 0 -#define STATE_BUS 1 -#define STATE_DEV 2 -#define STATE_FUNC 3 -#define STATE_VSLOT 4 -#define STATE_OPTIONS_K 6 -#define STATE_OPTIONS_V 7 -#define STATE_TERMINAL 8 -int libxl_device_pci_parse_bdf(libxl_ctx *ctx, libxl_device_pci *pcidev, const char *str) -{ - unsigned state = STATE_DOMAIN; - unsigned dom, bus, dev, func, vslot = 0; - char *buf2, *tok, *ptr, *end, *optkey = NULL; - - if ( NULL == (buf2 = ptr = strdup(str)) ) - return ERROR_NOMEM; - - for(tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++) { - switch(state) { - case STATE_DOMAIN: - if ( *ptr == ':' ) { - state = STATE_BUS; - *ptr = '\0'; - if ( hex_convert(tok, &dom, 0xffff) ) - goto parse_error; - tok = ptr + 1; - } - break; - case STATE_BUS: - if ( *ptr == ':' ) { - state = STATE_DEV; - *ptr = '\0'; - if ( hex_convert(tok, &bus, 0xff) ) - goto parse_error; - tok = ptr + 1; - }else if ( *ptr == '.' ) { - state = STATE_FUNC; - *ptr = '\0'; - if ( dom & ~0xff ) - goto parse_error; - bus = dom; - dom = 0; - if ( hex_convert(tok, &dev, 0xff) ) - goto parse_error; - tok = ptr + 1; - } - break; - case STATE_DEV: - if ( *ptr == '.' ) { - state = STATE_FUNC; - *ptr = '\0'; - if ( hex_convert(tok, &dev, 0xff) ) - goto parse_error; - tok = ptr + 1; - } - break; - case STATE_FUNC: - if ( *ptr == '\0' || *ptr == '@' || *ptr == ',' ) { - switch( *ptr ) { - case '\0': - state = STATE_TERMINAL; - break; - case '@': - state = STATE_VSLOT; - break; - case ',': - state = STATE_OPTIONS_K; - break; - } - *ptr = '\0'; - if ( !strcmp(tok, "*") ) { - pcidev->vfunc_mask = LIBXL_PCI_FUNC_ALL; - }else{ - if ( hex_convert(tok, &func, 0x7) ) - goto parse_error; - pcidev->vfunc_mask = (1 << 0); - } - tok = ptr + 1; - } - break; - case STATE_VSLOT: - if ( *ptr == '\0' || *ptr == ',' ) { - state = ( *ptr == ',' ) ? STATE_OPTIONS_K : STATE_TERMINAL; - *ptr = '\0'; - if ( hex_convert(tok, &vslot, 0xff) ) - goto parse_error; - tok = ptr + 1; - } - break; - case STATE_OPTIONS_K: - if ( *ptr == '=' ) { - state = STATE_OPTIONS_V; - *ptr = '\0'; - optkey = tok; - tok = ptr + 1; - } - break; - case STATE_OPTIONS_V: - if ( *ptr == ',' || *ptr == '\0' ) { - state = (*ptr == ',') ? STATE_OPTIONS_K : STATE_TERMINAL; - *ptr = '\0'; - if ( !strcmp(optkey, "msitranslate") ) { - pcidev->msitranslate = atoi(tok); - }else if ( !strcmp(optkey, "power_mgmt") ) { - pcidev->power_mgmt = atoi(tok); - }else{ - LIBXL__LOG(ctx, LIBXL__LOG_WARNING, - "Unknown PCI BDF option: %s", optkey); - } - tok = ptr + 1; - } - default: - break; - } - } - - free(buf2); - - if ( tok != ptr || state != STATE_TERMINAL ) - goto parse_error; - - pcidev_init(pcidev, dom, bus, dev, func, vslot << 3); - - return 0; - -parse_error: - return ERROR_INVAL; -} - static void libxl_create_pci_backend_device(libxl__gc *gc, flexarray_t *back, int num, libxl_device_pci *pcidev) { flexarray_append(back, libxl__sprintf(gc, "key-%d", num)); @@ -436,7 +293,7 @@ static int get_all_assigned_devices(libx *list = realloc(*list, sizeof(libxl_device_pci) * ((*num) + 1)); if (*list == NULL) return ERROR_NOMEM; - pcidev_init(*list + *num, dom, bus, dev, func, 0); + libxl_pci_dev_init(*list + *num, dom, bus, dev, func, 0); (*num)++; } } @@ -507,7 +364,7 @@ libxl_device_pci *libxl_device_pci_list_ new = pcidevs + *num; memset(new, 0, sizeof(*new)); - pcidev_init(new, dom, bus, dev, func, 0); + libxl_pci_dev_init(new, dom, bus, dev, func, 0); (*num)++; } @@ -1086,7 +943,7 @@ static void libxl__device_pci_from_xs_be if (s) vdevfn = strtol(s, (char **) NULL, 16); - pcidev_init(pci, domain, bus, dev, func, vdevfn); + libxl_pci_dev_init(pci, domain, bus, dev, func, vdevfn); s = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/opts-%d", be_path, nr)); if (s) { diff -r f744e82ea740 -r 5386937e6c5c tools/libxl/libxlu_pci.c --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tools/libxl/libxlu_pci.c Mon Apr 02 11:29:34 2012 +0100 @@ -0,0 +1,160 @@ +#include "libxl_osdeps.h" /* must come before any other headers */ +#include "libxlu_internal.h" +#include "libxlu_disk_l.h" +#include "libxlu_disk_i.h" +#include "libxlu_cfg_i.h" + + +#define XLU__PCI_ERR(_c, _x, _a...) \ + if((_c) && (_c)->report) fprintf((_c)->report, _x, ##_a) + +static int hex_convert(const char *str, unsigned int *val, unsigned int mask) +{ + unsigned long ret; + char *end; + + ret = strtoul(str, &end, 16); + if ( end == str || *end != '\0' ) + return -1; + if ( ret & ~mask ) + return -1; + *val = (unsigned int)ret & mask; + return 0; +} + +#define STATE_DOMAIN 0 +#define STATE_BUS 1 +#define STATE_DEV 2 +#define STATE_FUNC 3 +#define STATE_VSLOT 4 +#define STATE_OPTIONS_K 6 +#define STATE_OPTIONS_V 7 +#define STATE_TERMINAL 8 +int xlu_pci_parse_bdf(XLU_Config *cfg, libxl_device_pci *pcidev, const char *str) +{ + unsigned state = STATE_DOMAIN; + unsigned dom, bus, dev, func, vslot = 0; + char *buf2, *tok, *ptr, *end, *optkey = NULL; + + if ( NULL == (buf2 = ptr = strdup(str)) ) + return ERROR_NOMEM; + + for(tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++) { + switch(state) { + case STATE_DOMAIN: + if ( *ptr == ':' ) { + state = STATE_BUS; + *ptr = '\0'; + if ( hex_convert(tok, &dom, 0xffff) ) + goto parse_error; + tok = ptr + 1; + } + break; + case STATE_BUS: + if ( *ptr == ':' ) { + state = STATE_DEV; + *ptr = '\0'; + if ( hex_convert(tok, &bus, 0xff) ) + goto parse_error; + tok = ptr + 1; + }else if ( *ptr == '.' ) { + state = STATE_FUNC; + *ptr = '\0'; + if ( dom & ~0xff ) + goto parse_error; + bus = dom; + dom = 0; + if ( hex_convert(tok, &dev, 0xff) ) + goto parse_error; + tok = ptr + 1; + } + break; + case STATE_DEV: + if ( *ptr == '.' ) { + state = STATE_FUNC; + *ptr = '\0'; + if ( hex_convert(tok, &dev, 0xff) ) + goto parse_error; + tok = ptr + 1; + } + break; + case STATE_FUNC: + if ( *ptr == '\0' || *ptr == '@' || *ptr == ',' ) { + switch( *ptr ) { + case '\0': + state = STATE_TERMINAL; + break; + case '@': + state = STATE_VSLOT; + break; + case ',': + state = STATE_OPTIONS_K; + break; + } + *ptr = '\0'; + if ( !strcmp(tok, "*") ) { + pcidev->vfunc_mask = LIBXL_PCI_FUNC_ALL; + }else{ + if ( hex_convert(tok, &func, 0x7) ) + goto parse_error; + pcidev->vfunc_mask = (1 << 0); + } + tok = ptr + 1; + } + break; + case STATE_VSLOT: + if ( *ptr == '\0' || *ptr == ',' ) { + state = ( *ptr == ',' ) ? STATE_OPTIONS_K : STATE_TERMINAL; + *ptr = '\0'; + if ( hex_convert(tok, &vslot, 0xff) ) + goto parse_error; + tok = ptr + 1; + } + break; + case STATE_OPTIONS_K: + if ( *ptr == '=' ) { + state = STATE_OPTIONS_V; + *ptr = '\0'; + optkey = tok; + tok = ptr + 1; + } + break; + case STATE_OPTIONS_V: + if ( *ptr == ',' || *ptr == '\0' ) { + state = (*ptr == ',') ? STATE_OPTIONS_K : STATE_TERMINAL; + *ptr = '\0'; + if ( !strcmp(optkey, "msitranslate") ) { + pcidev->msitranslate = atoi(tok); + }else if ( !strcmp(optkey, "power_mgmt") ) { + pcidev->power_mgmt = atoi(tok); + }else{ + XLU__PCI_ERR(cfg, "Unknown PCI BDF option: %s", optkey); + } + tok = ptr + 1; + } + default: + break; + } + } + + free(buf2); + + if ( tok != ptr || state != STATE_TERMINAL ) + goto parse_error; + + /* Just a pretty way to fill in the values */ + libxl_pci_dev_init(pcidev, dom, bus, dev, func, vslot << 3); + + return 0; + +parse_error: + return ERROR_INVAL; +} + +/* + * Local variables: + * mode: C + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff -r f744e82ea740 -r 5386937e6c5c tools/libxl/libxlutil.h --- a/tools/libxl/libxlutil.h Wed Feb 29 16:30:34 2012 +0000 +++ b/tools/libxl/libxlutil.h Mon Apr 02 11:29:34 2012 +0100 @@ -88,6 +88,16 @@ int xlu_disk_parse(XLU_Config *cfg, int * resulting disk struct is used with libxl. */ +/* + * PCI specification parsing + */ +int xlu_pci_parse_bdf(XLU_Config *cfg, libxl_device_pci *pcidev, const char *str); +/* */ + +int xlu_pci_dev_init(libxl_device_pci *pcidev, unsigned int domain, + unsigned int bus, unsigned int dev, + unsigned int func, unsigned int vdevfn); + #endif /* LIBXLUTIL_H */ diff -r f744e82ea740 -r 5386937e6c5c tools/libxl/xl_cmdimpl.c --- a/tools/libxl/xl_cmdimpl.c Wed Feb 29 16:30:34 2012 +0000 +++ b/tools/libxl/xl_cmdimpl.c Mon Apr 02 11:29:34 2012 +0100 @@ -1005,7 +1005,7 @@ skip_vfb: pcidev->msitranslate = pci_msitranslate; pcidev->power_mgmt = pci_power_mgmt; - if (!libxl_device_pci_parse_bdf(ctx, pcidev, buf)) + if (!xlu_pci_parse_bdf(config, pcidev, buf)) d_config->num_pcidevs++; } if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV) @@ -2217,11 +2217,16 @@ int main_pcilist(int argc, char **argv) static void pcidetach(const char *dom, const char *bdf, int force) { libxl_device_pci pcidev; + XLU_Config *config; find_domain(dom); memset(&pcidev, 0x00, sizeof(pcidev)); - if (libxl_device_pci_parse_bdf(ctx, &pcidev, bdf)) { + + config = xlu_cfg_init(stderr, "command line"); + if (!config) { perror("xlu_cfg_inig"); exit(-1); } + + if (xlu_pci_parse_bdf(config, &pcidev, bdf)) { fprintf(stderr, "pci-detach: malformed BDF specification \"%s\"\n", bdf); exit(2); } @@ -2257,11 +2262,16 @@ int main_pcidetach(int argc, char **argv static void pciattach(const char *dom, const char *bdf, const char *vs) { libxl_device_pci pcidev; + XLU_Config *config; find_domain(dom); memset(&pcidev, 0x00, sizeof(pcidev)); - if (libxl_device_pci_parse_bdf(ctx, &pcidev, bdf)) { + + config = xlu_cfg_init(stderr, "command line"); + if (!config) { perror("xlu_cfg_inig"); exit(-1); } + + if (xlu_pci_parse_bdf(config, &pcidev, bdf)) { fprintf(stderr, "pci-attach: malformed BDF specification \"%s\"\n", bdf); exit(2); } diff -r f744e82ea740 -r 5386937e6c5c tools/python/xen/lowlevel/xl/xl.c --- a/tools/python/xen/lowlevel/xl/xl.c Wed Feb 29 16:30:34 2012 +0000 +++ b/tools/python/xen/lowlevel/xl/xl.c Mon Apr 02 11:29:34 2012 +0100 @@ -40,6 +40,7 @@ #include <libxl.h> #include <libxl_utils.h> +#include <libxlutil.h> #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) @@ -556,7 +557,7 @@ static PyObject *pyxl_pci_parse(XlObject return NULL; } - if ( libxl_device_pci_parse_bdf(self->ctx, &pci->obj, str) ) { + if ( xlu_pci_parse_bdf(NULL, &pci->obj, str) ) { PyErr_SetString(xl_error_obj, "cannot parse pci device spec (BDF)"); Py_DECREF(pci); return NULL; ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1 of 2] libxl: Move bdf parsing into libxlu 2012-04-02 10:47 ` [PATCH 1 of 2] libxl: Move bdf parsing into libxlu George Dunlap @ 2012-04-02 11:05 ` Ian Campbell 2012-04-02 14:45 ` George Dunlap 0 siblings, 1 reply; 10+ messages in thread From: Ian Campbell @ 2012-04-02 11:05 UTC (permalink / raw) To: George Dunlap; +Cc: xen-devel@lists.xensource.com On Mon, 2012-04-02 at 11:47 +0100, George Dunlap wrote: > # HG changeset patch > # User George Dunlap <george.dunlap@eu.citrix.com> > # Date 1333362574 -3600 > # Node ID 5386937e6c5c9afaa8a3cd56d391dcc9e40d0596 > # Parent f744e82ea74075983de6d5b0ad0cf7ccacf999a2 > libxl: Move bdf parsing into libxlu > > Config parsing functions do not properly belong in libxl. Move them into > libxlu so that others can use them or not as they see fit. > > No functional changes. One side-effect was making public a private libxl > utility function which just set the elements of a structure from the function > arguments passed in. > [...] > diff -r f744e82ea740 -r 5386937e6c5c tools/libxl/libxl.h > --- a/tools/libxl/libxl.h Wed Feb 29 16:30:34 2012 +0000 > +++ b/tools/libxl/libxl.h Mon Apr 02 11:29:34 2012 +0100 > @@ -573,13 +573,10 @@ int libxl_device_pci_add(libxl_ctx *ctx, > int libxl_device_pci_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev); > int libxl_device_pci_destroy(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev); > libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid, int *num); > - > -/* > - * Parse a PCI BDF into a PCI device structure. > - */ > -int libxl_device_pci_parse_bdf(libxl_ctx *ctx, > - libxl_device_pci *pcidev, > - const char *str); > +/* Just initialize the structure elements with the arguments provided. */ > +int libxl_pci_dev_init(libxl_device_pci *pcidev, unsigned int domain, > + unsigned int bus, unsigned int dev, > + unsigned int func, unsigned int vdevfn); libxl_<type>_init has a particular meaning described further up in this header. Although you haven't actually used <type> here so it doesn't conflict the general convention is to use the type name as a prefix. Does this function actually add all that much value? The users of it could either open code it or have a local version. Otherwise I think this patch looks good, thanks. Ian. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1 of 2] libxl: Move bdf parsing into libxlu 2012-04-02 11:05 ` Ian Campbell @ 2012-04-02 14:45 ` George Dunlap 0 siblings, 0 replies; 10+ messages in thread From: George Dunlap @ 2012-04-02 14:45 UTC (permalink / raw) To: Ian Campbell; +Cc: xen-devel@lists.xensource.com On 02/04/12 12:05, Ian Campbell wrote: > On Mon, 2012-04-02 at 11:47 +0100, George Dunlap wrote: >> # HG changeset patch >> # User George Dunlap<george.dunlap@eu.citrix.com> >> # Date 1333362574 -3600 >> # Node ID 5386937e6c5c9afaa8a3cd56d391dcc9e40d0596 >> # Parent f744e82ea74075983de6d5b0ad0cf7ccacf999a2 >> libxl: Move bdf parsing into libxlu >> >> Config parsing functions do not properly belong in libxl. Move them into >> libxlu so that others can use them or not as they see fit. >> >> No functional changes. One side-effect was making public a private libxl >> utility function which just set the elements of a structure from the function >> arguments passed in. >> [...] >> diff -r f744e82ea740 -r 5386937e6c5c tools/libxl/libxl.h >> --- a/tools/libxl/libxl.h Wed Feb 29 16:30:34 2012 +0000 >> +++ b/tools/libxl/libxl.h Mon Apr 02 11:29:34 2012 +0100 >> @@ -573,13 +573,10 @@ int libxl_device_pci_add(libxl_ctx *ctx, >> int libxl_device_pci_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev); >> int libxl_device_pci_destroy(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev); >> libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid, int *num); >> - >> -/* >> - * Parse a PCI BDF into a PCI device structure. >> - */ >> -int libxl_device_pci_parse_bdf(libxl_ctx *ctx, >> - libxl_device_pci *pcidev, >> - const char *str); >> +/* Just initialize the structure elements with the arguments provided. */ >> +int libxl_pci_dev_init(libxl_device_pci *pcidev, unsigned int domain, >> + unsigned int bus, unsigned int dev, >> + unsigned int func, unsigned int vdevfn); > libxl_<type>_init has a particular meaning described further up in this > header. Although you haven't actually used<type> here so it doesn't > conflict the general convention is to use the type name as a prefix. > > Does this function actually add all that much value? The users of it > could either open code it or have a local version. You know, I think I'll just make two local copies of the function. I'll also give it a less misleading name. -George ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-04-04 15:07 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-04-03 13:54 [PATCH 0 of 2] [v2] Add per-device and global permissive config options for pci passthrough George Dunlap 2012-04-03 13:54 ` [PATCH 1 of 2] libxl: Move bdf parsing into libxlu George Dunlap 2012-04-03 14:34 ` Ian Jackson 2012-04-03 13:54 ` [PATCH 2 of 2] xl, libxl: Add per-device and global permissive config options for pci passthrough George Dunlap 2012-04-03 14:34 ` Ian Jackson 2012-04-03 14:52 ` [PATCH 0 of 2] [v2] " George Dunlap 2012-04-04 15:07 ` Ian Jackson -- strict thread matches above, loose matches on Subject: below -- 2012-04-02 10:47 George Dunlap 2012-04-02 10:47 ` [PATCH 1 of 2] libxl: Move bdf parsing into libxlu George Dunlap 2012-04-02 11:05 ` Ian Campbell 2012-04-02 14:45 ` George Dunlap
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).