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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel