From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH 3 of 4] libxl: Introduce pci_assignable_add and pci_assignable_remove Date: Thu, 10 May 2012 15:55:43 +0100 Message-ID: <4FABD6EF.4020607@eu.citrix.com> References: <17a5b562d1e79d094c58.1336559323@kodo2> <1336648786.7098.89.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1336648786.7098.89.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org 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