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

[George Dunlap <george.dunlap@eu.citrix.com>]

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