|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 3 of 4] libxl: Introduce pci_assignable_add and pci_assignable_remove
On Wed, 2012-05-09 at 11:28 +0100, George Dunlap wrote:
> Introduce libxl helper functions to prepare devices to be passed through to
> guests. This is meant to replace of all the manual sysfs commands which
> are currently required.
>
> pci_assignable_add accepts a BDF for a device and will:
> * Unbind a device from its current driver, if any
> * If "rebind" is set, it will store the path of the driver from which we
> unplugged it in /libxl/pciback/$BDF/driver_path
Since you don't know whether the user is going to pass -r to
assignable_remove why not always store this?
> * If necessary, create a slot for it in pciback
I must confess I'm a bit fuzzy on the relationship between slots and
bindings, where does the "if necessary" come into it?
I was wondering while reading the patch if unconditionally adding a
removing the slot might simplify a bunch of stuff, but I suspect I just
don't appreciate some particular aspect of how pciback works...
> * Bind the device to pciback
>
> At this point it will show up in pci_assignable_list, and is ready to be
> passed
> through to a guest.
>
> pci_assignable_remove accepts a BDF for a device and will:
> * Unbind the device from pciback
> * Remove the slot from pciback
> * If "rebind" is set, and /libx/pciback/$BDF/driver_path exists, it will
> attempt to rebind the device to its original driver.
>
> NB that "$BDF" in this case uses dashes instead of : and ., because : and .
> are
> illegal characters in xenstore paths.
>
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
>
> diff -r 5b5070d487d9 -r 17a5b562d1e7 tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h Wed May 09 11:21:19 2012 +0100
> +++ b/tools/libxl/libxl.h Wed May 09 11:21:28 2012 +0100
> @@ -658,10 +658,25 @@ int libxl_device_pci_destroy(libxl_ctx *
> libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid, int
> *num);
>
> /*
> - * 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.
> + * Functions related to making devices assignable -- that is, bound to
> + * the pciback driver, ready to be given to a guest via
> + * libxl_pci_device_add.
> + *
> + * - ..._add() will unbind the device from its current driver (if
> + * already bound) and re-bind it to pciback; at that point it will be
> + * ready to be assigned to a VM. If rebind is set, it will store the
> + * path to the old driver in xenstore so that it can be handed back to
> + * dom0 on restore.
> + *
> + * - ..._remove() will unbind the device from pciback, and if
> + * rebind is non-zero, attempt to assign it back to the driver
> + * from whence it came.
> + *
> + * - ..._list() will return a list of the PCI devices available to be
> + * assigned.
> */
> +int libxl_device_pci_assignable_add(libxl_ctx *ctx, libxl_device_pci
> *pcidev, int rebind);
> +int libxl_device_pci_assignable_remove(libxl_ctx *ctx, libxl_device_pci
> *pcidev, int rebind);
> libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num);
>
> /* CPUID handling */
> diff -r 5b5070d487d9 -r 17a5b562d1e7 tools/libxl/libxl_pci.c
> --- a/tools/libxl/libxl_pci.c Wed May 09 11:21:19 2012 +0100
> +++ b/tools/libxl/libxl_pci.c Wed May 09 11:21:28 2012 +0100
> @@ -21,6 +21,7 @@
> #define PCI_BDF "%04x:%02x:%02x.%01x"
> #define PCI_BDF_SHORT "%02x:%02x.%01x"
> #define PCI_BDF_VDEVFN "%04x:%02x:%02x.%01x@%02x"
> +#define PCI_BDF_XSPATH "%04x-%02x-%02x-%01x"
>
> static unsigned int pcidev_encode_bdf(libxl_device_pci *pcidev)
> {
> @@ -408,6 +409,347 @@ out:
> return pcidevs;
> }
>
> +/* Unbind device from its current driver, if any. If driver_path is
> non-NULL,
> + * store the path to the original driver in it. */
> +static int sysfs_dev_unbind(libxl__gc *gc, libxl_device_pci *pcidev, char
> **driver_path)
> +{
> + libxl_ctx *ctx = libxl__gc_owner(gc);
> + char * spath, *_dp, **dp;
> + struct stat st;
> +
> + if ( driver_path )
> + dp = driver_path;
> + else
> + dp = &_dp;
> +
> + spath = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/driver",
> + pcidev->domain,
> + pcidev->bus,
> + pcidev->dev,
> + pcidev->func);
> + if ( !lstat(spath, &st) ) {
> + /* Find the canonical path to the driver. */
> + *dp = libxl__zalloc(gc, PATH_MAX);
Should we be actually using fpathconf / sysconf here?
> + *dp = realpath(spath, *dp);
> + if ( !*dp ) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "realpath() failed");
Since errno is meaningful you want LIBXL__LOGERRNO here. Or the short
form LOGE().
Where you have pointer output params (like driver_path here) in general
I think it is preferable to do everything with a local (single
indirection) pointer and only update the output parameter on success.
This means you avoid leaking/exposing a partial result on error but also
means you can drop a lot of "*" from around the function.
Maybe the gc makes this moot, but the "if ( driver_path )" stuff at the
top of the fn took me several seconds to work out also ;-).
> + return -1;
> + }
> +
> + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Driver re-plug path: %s",
> + *dp);
> +
> + /* Unbind from the old driver */
> + spath = libxl__sprintf(gc, "%s/unbind", *dp);
> + if ( sysfs_write_bdf(gc, spath, pcidev) < 0 ) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Couldn't unbind device");
Not sure what errno is like here, worth printing it. Looking back at
patch #1 I suspect sysfs_write_bdf should preserve errno over the call
to close, so that suitable reporting can happen in the caller.
> + return -1;
> + }
> + }
> + return 0;
> +}
> +
> +/* Scan through /sys/.../pciback/slots looking for pcidev's BDF */
> +static int pciback_dev_has_slot(libxl__gc *gc, libxl_device_pci *pcidev)
Is the concept of "having a slot" distinct from being bound to pciback?
> +{
> + libxl_ctx *ctx = libxl__gc_owner(gc);
> + FILE *f;
> + int rc = 0;
> + unsigned bus, dev, func;
> +
> + f = fopen(SYSFS_PCIBACK_DRIVER"/slots", "r");
> +
> + if (f == NULL) {
> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn't open %s",
> + SYSFS_PCIBACK_DRIVER"/slots");
> + return ERROR_FAIL;
> + }
> +
> + while(fscanf(f, "0000:%x:%x.%x\n", &bus, &dev, &func)==3) {
Jan recently added support for PCI domains, does that have any impact on
the hardcoded 0000 here? I suppose that would require PCI domains
support in the dom0 kernel -- but that seems likely to already be there
(or be imminent)
I think the 0000 should be %x into domain compared with pcidev->domain.
> + if(bus == pcidev->bus
> + && dev == pcidev->dev
> + && func == pcidev->func) {
> + rc = 1;
> + goto out;
> + }
> + }
> +out:
> + fclose(f);
> + return rc;
> +}
> +
> +static int pciback_dev_is_assigned(libxl__gc *gc, libxl_device_pci *pcidev)
> +{
> + libxl_ctx *ctx = libxl__gc_owner(gc);
> + char * spath;
> + int rc;
> + struct stat st;
> +
> + spath = libxl__sprintf(gc, SYSFS_PCIBACK_DRIVER"/"PCI_BDF,
> + pcidev->domain, pcidev->bus,
> + pcidev->dev, pcidev->func);
> + rc = lstat(spath, &st);
> +
> + if( rc == 0 )
> + return 1;
> + if ( rc < 0 && errno == ENOENT )
> + return 0;
> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Accessing %s", spath);
> + return -1;
> +}
> +
> +static int pciback_dev_assign(libxl__gc *gc, libxl_device_pci *pcidev)
> +{
> + libxl_ctx *ctx = libxl__gc_owner(gc);
> + int rc;
> +
> + if ( (rc=pciback_dev_has_slot(gc, pcidev)) < 0 ) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> + "Error checking for pciback slot");
Log errno?
Also pciback_dev_has_slot itself always logs on error, you probably
don't need both.
> + return ERROR_FAIL;
> + } else if (rc == 0) {
> + if ( sysfs_write_bdf(gc, SYSFS_PCIBACK_DRIVER"/new_slot",
> + pcidev) < 0 ) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> + "Couldn't bind device to pciback!");
ERRNO here too.
> + return ERROR_FAIL;
> + }
> + }
> +
> + if ( sysfs_write_bdf(gc, SYSFS_PCIBACK_DRIVER"/bind", pcidev) < 0 ) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Couldn't bind device to
> pciback!");
ERRNO here too. Or since there are loads of these perhaps sysfs_write_bf
should log on failure, either just the fact of the failed write to a
path (which implies the underlying failure was to bind/unbind) or you
could add a "const char *what" param to use in logging.
> + return ERROR_FAIL;
> + }
> + return 0;
> +}
> +
> +static int pciback_dev_unassign(libxl__gc *gc, libxl_device_pci *pcidev)
> +{
> + libxl_ctx *ctx = libxl__gc_owner(gc);
> +
> + /* Remove from pciback */
> + if ( sysfs_dev_unbind(gc, pcidev, NULL) < 0 ) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Couldn't unbind device!");
> + return ERROR_FAIL;
> + }
> +
> + /* Remove slot if necessary */
> + if ( pciback_dev_has_slot(gc, pcidev) > 0 ) {
> + if ( sysfs_write_bdf(gc, SYSFS_PCIBACK_DRIVER"/remove_slot",
> + pcidev) < 0 ) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> + "Couldn't remove pciback slot");
ERRNO
> + return ERROR_FAIL;
> + }
> + }
> + return 0;
> +}
> +
> +/* FIXME */
Leftover?
> +#define PCIBACK_INFO_PATH "/libxl/pciback"
> +
> +static void pci_assignable_driver_path_write(libxl__gc *gc,
> + libxl_device_pci *pcidev,
> + char *driver_path)
> +{
> + libxl_ctx *ctx = libxl__gc_owner(gc);
> + char *path;
> + xs_transaction_t t = 0;
> + struct xs_permissions perm[1];
> +
> + perm[0].id = 0;
> + perm[0].perms = XS_PERM_NONE;
> +
> +retry:
> + t = xs_transaction_start(ctx->xsh);
> +
> + path = libxl__sprintf(gc,
> PCIBACK_INFO_PATH"/"PCI_BDF_XSPATH"/driver_path",
> + pcidev->domain,
> + pcidev->bus,
> + pcidev->dev,
> + pcidev->func);
> + xs_rm(ctx->xsh, t, path);
Why do you need to rm first? Won't the write just replace whatever it is
(and that means the need for a transaction goes away too)
In any case you should create path outside the retry loop instead of
needlessly recreating it each time around.
> + if ( libxl__xs_write(gc, XBT_NULL, path, "%s", driver_path) < 0 ) {
> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> + "Write of %s to node %s failed",
> + driver_path, path);
> + }
> +
> + if (!xs_transaction_end(ctx->xsh, t, 0)) {
> + if (errno == EAGAIN) {
> + t = 0;
> + goto retry;
> + }
> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> + "xenstore transaction commit failed; device "
> + " will not be rebound to original driver.");
> + }
> +}
> +
> +static char * pci_assignable_driver_path_read(libxl__gc *gc,
> + libxl_device_pci *pcidev)
> +{
> + return libxl__xs_read(gc, XBT_NULL,
> + libxl__sprintf(gc,
> + PCIBACK_INFO_PATH "/" PCI_BDF_XSPATH
> "/driver_path",
> + pcidev->domain,
> + pcidev->bus,
> + pcidev->dev,
> + pcidev->func));
> +}
> +
> +static void pci_assignable_driver_path_remove(libxl__gc *gc,
> + libxl_device_pci *pcidev)
> +{
> + libxl_ctx *ctx = libxl__gc_owner(gc);
> + char * path;
> + xs_transaction_t t;
> +
> + /* Remove the xenstore entry */
> +retry:
> + t = xs_transaction_start(ctx->xsh);
> + path = libxl__sprintf(gc, PCIBACK_INFO_PATH "/" PCI_BDF_XSPATH,
> + pcidev->domain,
> + pcidev->bus,
> + pcidev->dev,
> + pcidev->func);
> + xs_rm(ctx->xsh, t, path);
You don't need a transaction for a single operation. (and if you did
then "path = ..." could have been hoisted out)
> + if (!xs_transaction_end(ctx->xsh, t, 0)) {
> + if (errno == EAGAIN)
> + goto retry;
> + else
> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> + "Failed to remove xenstore entry");
> + }
> +}
> +
> +static int libxl__device_pci_assignable_add(libxl__gc *gc,
> + libxl_device_pci *pcidev,
> + int rebind)
> +{
> + libxl_ctx *ctx = libxl__gc_owner(gc);
> + unsigned dom, bus, dev, func;
> + char *spath, *driver_path = NULL;
> + struct stat st;
> +
> + /* Local copy for convenience */
> + dom = pcidev->domain;
> + bus = pcidev->bus;
> + dev = pcidev->dev;
> + func = pcidev->func;
> +
> + /* See if the device exists */
> + spath = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF, dom, bus, dev, func);
> + if ( lstat(spath, &st) ) {
> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn't lstat %s", spath);
> + return ERROR_FAIL;
> + }
> +
> + /* Check to see if it's already assigned to pciback */
> + if ( pciback_dev_is_assigned(gc, pcidev) ) {
> + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, PCI_BDF" already assigned to
> pciback",
> + dom, bus, dev, func);
> + return 0;
> + }
> +
> + /* Check to see if there's already a driver that we need to unbind from
> */
> + if ( sysfs_dev_unbind(gc, pcidev, &driver_path ) ) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Couldn't unbind "PCI_BDF" from
> driver",
> + dom, bus, dev, func);
> + return ERROR_FAIL;
> + }
> +
> + /* Store driver_path for rebinding to dom0 */
> + if ( rebind ) {
> + if ( driver_path ) {
> + pci_assignable_driver_path_write(gc, pcidev, driver_path);
> + } else {
> + LIBXL__LOG(ctx, LIBXL__LOG_WARNING,
> + PCI_BDF" not bound to a driver, will not be rebound.",
> + dom, bus, dev, func);
> + }
> + }
> +
> + if ( pciback_dev_assign(gc, pcidev) ) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Couldn't bind device to
> pciback!");
> + return ERROR_FAIL;
> + }
> +
> + return 0;
> +}
> +
> +static int libxl__device_pci_assignable_remove(libxl__gc *gc,
> + libxl_device_pci *pcidev,
> + int rebind)
> +{
> + libxl_ctx *ctx = libxl__gc_owner(gc);
> + int rc;
> + char *driver_path;
> +
> + /* Unbind from pciback */
> + if ( (rc=pciback_dev_is_assigned(gc, pcidev)) < 0 ) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Checking if pciback was
> assigned");
> + return ERROR_FAIL;
> + } else if ( rc ) {
> + pciback_dev_unassign(gc, pcidev);
> + } else {
> + LIBXL__LOG(ctx, LIBXL__LOG_WARNING,
> + "Not bound to pciback");
> + }
> +
> + /* Rebind if necessary */
> + driver_path = pci_assignable_driver_path_read(gc, pcidev);
> +
> + if ( driver_path ) {
> + if ( rebind ) {
> + LIBXL__LOG(ctx, LIBXL__LOG_INFO, "Rebinding to driver at %s",
> + driver_path);
> +
> + if ( sysfs_write_bdf(gc,
> + libxl__sprintf(gc, "%s/bind", driver_path),
> + pcidev) < 0 ) {
> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> + "Couldn't bind device to %s", driver_path);
> + return -1;
> + }
> + }
> +
> + pci_assignable_driver_path_remove(gc, pcidev);
> + } else {
> + if ( rebind ) {
> + LIBXL__LOG(ctx, LIBXL__LOG_WARNING,
> + "Couldn't find path for original driver; not
> rebinding");
> + }
> + }
> +
> + return 0;
> +}
> +
> +int libxl_device_pci_assignable_add(libxl_ctx *ctx, libxl_device_pci *pcidev,
> + int rebind)
> +{
> + GC_INIT(ctx);
> + int rc;
> +
> + rc = libxl__device_pci_assignable_add(gc, pcidev, rebind);
> +
> + GC_FREE;
> + return rc;
> +}
Are there internal callers of libxl__device_pci_assignable_add/remove?
If not then there's no reason to split into internal/external functions.
Doesn't matter much I guess.
> +
> +
> +int libxl_device_pci_assignable_remove(libxl_ctx *ctx, libxl_device_pci
> *pcidev,
> + int rebind)
> +{
> + GC_INIT(ctx);
> + int rc;
> +
> + rc = libxl__device_pci_assignable_remove(gc, pcidev, rebind);
> +
> + GC_FREE;
> + return rc;
> +}
> +
> /*
> * This function checks that all functions of a device are bound to pciback
> * driver. It also initialises a bit-mask of which function numbers are
> present
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |