[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 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().

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.
+/* 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.

+            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");

+            return ERROR_FAIL;
+        }
+    }
+    return 0;
+/* FIXME */
Yeah, noticed this just after I sent it. :-)
+    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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.