Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Xen: Devel

[PATCH 3 of 4] libxl: Introduce pci_assignable_add and pci_assignable_remove

 

 

Xen devel RSS feed   Index | Next | Previous | View Threaded


george.dunlap at eu

May 9, 2012, 3:28 AM

Post #1 of 6 (173 views)
Permalink
[PATCH 3 of 4] libxl: Introduce pci_assignable_add and pci_assignable_remove

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
* If necessary, create a slot for it in pciback
* 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 [at] eu>

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);
+ *dp = realpath(spath, *dp);
+ if ( !*dp ) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "realpath() failed");
+ 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");
+ 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)
+{
+ 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) {
+ 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");
+ 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!");
+ 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!");
+ 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");
+ return ERROR_FAIL;
+ }
+ }
+ return 0;
+}
+
+/* FIXME */
+#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);
+ 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);
+ 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;
+}
+
+
+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 [at] lists
http://lists.xen.org/xen-devel


Ian.Campbell at citrix

May 10, 2012, 4:19 AM

Post #2 of 6 (171 views)
Permalink
Re: [PATCH 3 of 4] libxl: Introduce pci_assignable_add and pci_assignable_remove [In reply to]

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 [at] eu>
>
> 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 [at] lists
> http://lists.xen.org/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


george.dunlap at eu

May 10, 2012, 7:55 AM

Post #3 of 6 (171 views)
Permalink
Re: [PATCH 3 of 4] libxl: Introduce pci_assignable_add and pci_assignable_remove [In reply to]

On 10/05/12 12:19, Ian Campbell wrote:
> 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?
The xl command does in fact do this (i.e., always passes '1' here). I
considered just taking this option out, as you suggest, but I thought
it might be useful for the libxl implementation to have more flexibility.
>> * 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...
I have no idea what the "slot" thing is for. What I've determined by
experimentation is:
* Before "echo $BDF > .../pciback/bind" will work, $BDF must be listed
in pciback/slots
* The way to get $BDF listed in pciback/slots is "echo $BDF >
pciback/new_slot"
* The above command is not idempotent; if you do it twice, you'll get
two entries of $BDF in pciback/slots

I wasn't sure if having two slots would be a problem or not; so I did
the conservative thing, and check for the existence of $BDF in
pciback/slots first, only creating a new slot if there is not already an
existing slot.

So "if necessary" means, "if the device doesn't already have a slot".

>> + 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?
I don't really follow. What exactly is it you're proposing?
>> + *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().
Done.
>
> 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 ;-).
Yeah, that's a lot simpler, and easier to read. Done.
>> + 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.
Done.
>> +/* 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?
Technically, yes. You can't be bound without a slot; but you can have a
slot without being bound. I don't know exactly what the "slot"
functionality is for, and why it's a separate step, but that's the way
it is at the moment.
>> +{
>> + 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.
Ah, right -- I don't think I knew anything about the whole PCI domains
thing. Done.
>
>> + 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.
This way you get a sort of callback path; but I could take it out if you
want.
>
>> + 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.
ack
>
>> + 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.
Just doing ERRNO for all the callers makes more sense to me.
>> + /* 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
ack
>
>> + return ERROR_FAIL;
>> + }
>> + }
>> + return 0;
>> +}
>> +
>> +/* FIXME */
> Leftover?
Yeah, noticed this just after I sent it. :-)
>> +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.
TBH, I just looked at another bit of code that did xs transactions and
tried to follow suit. Since I only need one operation, I'll take out
the transaction stuff.
>> + 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)
Very well.
>
>> +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.
Not yet, but I don't think it hurts to have that flexibility.

Thanks for the detailed review.

-George

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


Ian.Campbell at citrix

May 10, 2012, 8:04 AM

Post #4 of 6 (171 views)
Permalink
Re: [PATCH 3 of 4] libxl: Introduce pci_assignable_add and pci_assignable_remove [In reply to]

On Thu, 2012-05-10 at 15:55 +0100, George Dunlap wrote:
> On 10/05/12 12:19, Ian Campbell wrote:
> > 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?
> The xl command does in fact do this (i.e., always passes '1' here). I
> considered just taking this option out, as you suggest, but I thought
> it might be useful for the libxl implementation to have more flexibility.

Oh, I see, I lost track of this being a libxl patch. That seems fine.

> >> * 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...
> I have no idea what the "slot" thing is for. What I've determined by
> experimentation is:
> * Before "echo $BDF > .../pciback/bind" will work, $BDF must be listed
> in pciback/slots
> * The way to get $BDF listed in pciback/slots is "echo $BDF >
> pciback/new_slot"
> * The above command is not idempotent; if you do it twice, you'll get
> two entries of $BDF in pciback/slots
>
> I wasn't sure if having two slots would be a problem or not; so I did
> the conservative thing, and check for the existence of $BDF in
> pciback/slots first, only creating a new slot if there is not already an
> existing slot.
>
> So "if necessary" means, "if the device doesn't already have a slot".

OK, so it looks like the stuff to check all this is in fact necessary.

>
> >> + 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?
> I don't really follow. What exactly is it you're proposing?

PATH_MAX isn't really a constant these days, you can get the dynamic
value for a particular filesystem from fpathconf. I honestly don't know
how much of a concern this really is, especially given we are always
necessarily talking to sysfs.

> >> + 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.
> This way you get a sort of callback path; but I could take it out if you
> want.

I don't feel especially strongly, up to you (/knocks ball back over
net ;-) )

> >
> >> + return ERROR_FAIL;
> >> + }
> >> + }
> >> + return 0;
> >> +}
> >> +
> >> +/* FIXME */
> > Leftover?
> Yeah, noticed this just after I sent it. :-)

That's always the way...

> >> +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.
> Not yet, but I don't think it hurts to have that flexibility.

Sure.

> Thanks for the detailed review.

No problem. BTW, rather than writing done/ack dozens of times I normally
assume agreement if someone just trims the whole section.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


george.dunlap at eu

May 10, 2012, 9:29 AM

Post #5 of 6 (169 views)
Permalink
Re: [PATCH 3 of 4] libxl: Introduce pci_assignable_add and pci_assignable_remove [In reply to]

On 10/05/12 16:04, Ian Campbell wrote:
>>>> + 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?
>> I don't really follow. What exactly is it you're proposing?
> PATH_MAX isn't really a constant these days, you can get the dynamic
> value for a particular filesystem from fpathconf. I honestly don't know
> how much of a concern this really is, especially given we are always
> necessarily talking to sysfs.
Ah right -- I didn't get that you were referring to PATH_MAX. The
"realpath" manpage specifies: "The resulting path‐name is stored as a
null-terminated string, up to a maximum of PATH_MAX bytes, in the buffer
pointed to by resolved_path." That's why I used PATH_MAX in the
allocation. I would hope that if the manpage says PATH_MAX, it means
PATH_MAX, and not "some other thing which you can get by running this
complicated command I haven't mentioned". :-)

OK -- I've also added a comment explaining why I'm doing what I'm doing
with slots, which I'll include when I re-post the patch.

-George

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


Ian.Campbell at citrix

May 10, 2012, 9:45 AM

Post #6 of 6 (169 views)
Permalink
Re: [PATCH 3 of 4] libxl: Introduce pci_assignable_add and pci_assignable_remove [In reply to]

On Thu, 2012-05-10 at 17:29 +0100, George Dunlap wrote:
> On 10/05/12 16:04, Ian Campbell wrote:
> >>>> + 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?
> >> I don't really follow. What exactly is it you're proposing?
> > PATH_MAX isn't really a constant these days, you can get the dynamic
> > value for a particular filesystem from fpathconf. I honestly don't know
> > how much of a concern this really is, especially given we are always
> > necessarily talking to sysfs.
> Ah right -- I didn't get that you were referring to PATH_MAX. The
> "realpath" manpage specifies: "The resulting path‐name is stored as a
> null-terminated string, up to a maximum of PATH_MAX bytes, in the buffer
> pointed to by resolved_path." That's why I used PATH_MAX in the
> allocation. I would hope that if the manpage says PATH_MAX, it means
> PATH_MAX, and not "some other thing which you can get by running this
> complicated command I haven't mentioned". :-)

Yes, that's fair ;-)

I think the issue might be that some systems declare PATH_MAX to be
enormous (to cover all bases) and sysconf lets you use something more
realistic/relevant to the actual situation.

Looks like on Linux it is 4096, which I guess is ok.

> OK -- I've also added a comment explaining why I'm doing what I'm doing
> with slots, which I'll include when I re-post the patch.

Ta!

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

Xen devel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.