|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] Re: [PATCH 2 of 3] xl, implement pci-list-assignable-devices
On Tue, 27 Jul 2010, Gianni Tedesco (3P) wrote:
> tools/libxl/libxl.h | 1 +
> tools/libxl/libxl_pci.c | 123
> ++++++++++++++++++++++++++++++++++++++++++++++
> tools/libxl/xl.h | 1 +
> tools/libxl/xl_cmdimpl.c | 34 ++++++++++++
> tools/libxl/xl_cmdtable.c | 5 +
> 5 files changed, 164 insertions(+), 0 deletions(-)
>
>
> diff -r 20b2e15131c7 -r 214733749470 tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h Tue Jul 27 17:13:35 2010 +0100
> +++ b/tools/libxl/libxl.h Tue Jul 27 17:17:31 2010 +0100
> @@ -496,6 +496,7 @@ int libxl_device_pci_add(struct libxl_ct
> int libxl_device_pci_remove(struct libxl_ctx *ctx, uint32_t domid,
> libxl_device_pci *pcidev);
> int libxl_device_pci_shutdown(struct libxl_ctx *ctx, uint32_t domid);
> libxl_device_pci *libxl_device_pci_list(struct libxl_ctx *ctx, uint32_t
> domid, int *num);
> +libxl_device_pci *libxl_device_pci_list_assignable(struct libxl_ctx *ctx,
> int *num);
> int libxl_device_pci_init(libxl_device_pci *pcidev, unsigned int domain,
> unsigned int bus, unsigned int dev,
> unsigned int func, unsigned int vdevfn);
> diff -r 20b2e15131c7 -r 214733749470 tools/libxl/libxl_pci.c
> --- a/tools/libxl/libxl_pci.c Tue Jul 27 17:13:35 2010 +0100
> +++ b/tools/libxl/libxl_pci.c Tue Jul 27 17:17:31 2010 +0100
> @@ -28,6 +28,7 @@
> #include <unistd.h> /* for write, unlink and close */
> #include <stdint.h>
> #include <inttypes.h>
> +#include <dirent.h>
> #include <assert.h>
>
> #include "libxl.h"
> @@ -258,6 +259,75 @@ retry_transaction2:
> return 0;
> }
>
> +static libxl_device_pci *get_all_assigned_devices(struct libxl_ctx *ctx, int
> *num)
> +{
> + libxl_device_pci *new, *pcidevs = NULL;
> + char **domlist;
> + unsigned int nd, i;
> +
> + *num = 0;
> +
> + domlist = libxl_xs_directory(ctx, XBT_NULL, "/local/domain", &nd);
> + for(i = 0; i < nd; i++) {
> + char *path, *num_devs;
> +
> + path = libxl_sprintf(ctx,
> "/local/domain/0/backend/pci/%s/0/num_devs", domlist[i]);
> + num_devs = libxl_xs_read(ctx, XBT_NULL, path);
> + if ( num_devs ) {
> + int ndev = atoi(num_devs), j;
> + char *devpath, *bdf;
> +
> + for(j = 0; j < ndev; j++) {
> + devpath = libxl_sprintf(ctx,
> "/local/domain/0/backend/pci/%s/0/dev-%u",
> + domlist[i], j);
> + bdf = libxl_xs_read(ctx, XBT_NULL, devpath);
> + if ( bdf ) {
> + unsigned dom, bus, dev, func;
> + if ( sscanf(bdf, PCI_BDF, &dom, &bus, &dev, &func) != 4 )
> + continue;
> +
> + new = realloc(pcidevs, ((*num) + 1) * sizeof(*new));
> + if ( NULL == new )
> + continue;
> +
> + pcidevs = new;
> + new = pcidevs + *num;
> +
> + libxl_device_pci_init(new, dom, bus, dev, func, ~0);
> + (*num)++;
> + }
> + libxl_free(ctx, bdf);
> + libxl_free(ctx, devpath);
> + }
> + libxl_free(ctx, num_devs);
> + }
> + libxl_free(ctx, path);
> + }
> + libxl_free(ctx, domlist);
> +
> + return pcidevs;
> +}
You shouldn't use libxl_free explicitly on variables allocated in the
context. Actually I like explicit free's but we have to keep the style
consistent, so the memory management refactoring should come in as a
separate patch, and here there shouldn't be any libxl_free's.
The realloc is correct because it is used on a variable that is going
to be returned to the caller.
Also I would prefer this function to return an integer and take the
libxl_device_pci array as a parameter.
> +
> +static int is_assigned(libxl_device_pci *assigned, int num_assigned,
> + int dom, int bus, int dev, int func)
> +{
> + int i;
> +
> + for(i = 0; i < num_assigned; i++) {
> + if ( assigned[i].domain != dom )
> + continue;
> + if ( assigned[i].bus != bus )
> + continue;
> + if ( assigned[i].dev != dev )
> + continue;
> + if ( assigned[i].func != func )
> + continue;
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t domid,
> libxl_device_pci *pcidev)
> {
> char *path;
> @@ -466,6 +536,59 @@ out:
> return 0;
> }
>
> +static libxl_device_pci *scan_sys_pcidir(libxl_device_pci *pcidevs,
> libxl_device_pci *assigned,
> + int num_assigned, const char *path,
> int *num)
> +{
> + libxl_device_pci *new;
> + struct dirent *de;
> + DIR *dir;
> +
> + dir = opendir(path);
> + if ( NULL == dir )
> + return pcidevs;
> +
> + while( (de = readdir(dir)) ) {
> + unsigned dom, bus, dev, func;
> + if ( sscanf(de->d_name, PCI_BDF, &dom, &bus, &dev, &func) != 4 )
> + continue;
> +
> + if ( is_assigned(assigned, num_assigned, dom, bus, dev, func) )
> + continue;
> +
> + new = realloc(pcidevs, ((*num) + 1) * sizeof(*new));
> + if ( NULL == new )
> + continue;
> +
> + pcidevs = new;
> + new = pcidevs + *num;
> +
> + libxl_device_pci_init(new, dom, bus, dev, func, ~0);
> + (*num)++;
> + }
> +
> + closedir(dir);
> + return pcidevs;
> +}
> +
> +libxl_device_pci *libxl_device_pci_list_assignable(struct libxl_ctx *ctx,
> int *num)
> +{
> + libxl_device_pci *pcidevs = NULL;
> + libxl_device_pci *assigned;
> + int num_assigned;
> +
> + *num = 0;
> +
> + assigned = get_all_assigned_devices(ctx, &num_assigned);
> +
> + pcidevs = scan_sys_pcidir(pcidevs, assigned, num_assigned,
> + "/sys/bus/pci/drivers/pciback", num);
> + pcidevs = scan_sys_pcidir(pcidevs, assigned, num_assigned,
> + "/sys/bus/pci/drivers/pcistub", num);
> +
> + free(assigned);
> + return pcidevs;
> +}
> +
Same comment about returning an integer, also I don't like the way you
are "assuming" scan_sys_pcidir is going to use realloc on pcidevs: that
should be an implementation detail of scan_sys_pcidir that callers
shouldn't rely upon.
Consider that devices bound to pcistub cannot be assigned to PV guests,
so I would remove scan_sys_pcidir on pcistub from here.
Please define "/sys/bus/pci/drivers/pciback" in a macro.
> libxl_device_pci *libxl_device_pci_list(struct libxl_ctx *ctx, uint32_t
> domid, int *num)
> {
> char *be_path, *num_devs, *xsdev, *xsvdevfn, *xsopts;
> diff -r 20b2e15131c7 -r 214733749470 tools/libxl/xl.h
> --- a/tools/libxl/xl.h Tue Jul 27 17:13:35 2010 +0100
> +++ b/tools/libxl/xl.h Tue Jul 27 17:17:31 2010 +0100
> @@ -32,6 +32,7 @@ int main_cd_insert(int argc, char **argv
> int main_console(int argc, char **argv);
> int main_vncviewer(int argc, char **argv);
> int main_pcilist(int argc, char **argv);
> +int main_pcilist_assignable(int argc, char **argv);
> int main_pcidetach(int argc, char **argv);
> int main_pciattach(int argc, char **argv);
> int main_restore(int argc, char **argv);
> diff -r 20b2e15131c7 -r 214733749470 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c Tue Jul 27 17:13:35 2010 +0100
> +++ b/tools/libxl/xl_cmdimpl.c Tue Jul 27 17:17:31 2010 +0100
> @@ -1706,6 +1706,40 @@ int main_vncviewer(int argc, char **argv
> exit(0);
> }
>
> +void pcilist_assignable(void)
> +{
> + libxl_device_pci *pcidevs;
> + int num, i;
> +
> + pcidevs = libxl_device_pci_list_assignable(&ctx, &num);
> + if (!num)
> + return;
> + for (i = 0; i < num; i++) {
> + printf("%04x:%02x:%02x:%01x\n",
> + pcidevs[i].domain, pcidevs[i].bus, pcidevs[i].dev,
> pcidevs[i].func);
> + }
> + free(pcidevs);
> +}
> +
> +int main_pcilist_assignable(int argc, char **argv)
> +{
> + int opt;
> +
> + while ((opt = getopt(argc, argv, "h")) != -1) {
> + switch (opt) {
> + case 'h':
> + help("pci-list-assignable-devices");
> + exit(0);
> + default:
> + fprintf(stderr, "option not supported\n");
> + break;
> + }
> + }
> +
> + pcilist_assignable();
> + exit(0);
> +}
> +
> void pcilist(char *dom)
> {
> libxl_device_pci *pcidevs;
> diff -r 20b2e15131c7 -r 214733749470 tools/libxl/xl_cmdtable.c
> --- a/tools/libxl/xl_cmdtable.c Tue Jul 27 17:13:35 2010 +0100
> +++ b/tools/libxl/xl_cmdtable.c Tue Jul 27 17:17:31 2010 +0100
> @@ -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",
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |