Re: [PATCH] libxl_pci: check that host device is assignable before adding to the domain

[Ian Campbell <Ian.Campbell@citrix.com>]

Please don't top post.

On Tue, 2012-01-17 at 14:05 +0000, djmagee@mageenet.net wrote:
> That was my first thought, but this was also the first thing in libxl
> I touched and wasn't sure what would happen if I ended up nesting
> GC_INITs.  If it's safe to do then I can call
> libxl_device_pci_list_assignable, otherwise I'd have to pull the meat
> out and put it in another function.  What's the best way to do it?

You can use libxl__gc_owner(gc) (or the helpful CTX macro) to get a ctx
from a gc which you use to call an externally visible function from
within libxl -- it'll do the right thing (TM).

Ian.

> 
> Doug
> 
> -----Original Message-----
> From: Ian Campbell [mailto:Ian.Campbell@citrix.com] 
> Sent: Tuesday, January 17, 2012 4:47 AM
> To: djmagee@mageenet.net
> Cc: xen-devel@lists.xensource.com; Ian Jackson; Stefano Stabellini
> Subject: Re: [Xen-devel] [PATCH] libxl_pci: check that host device is assignable before adding to the domain
> 
> On Mon, 2012-01-16 at 22:36 +0000, Doug Magee wrote:
> > Previously, on ..._pci_add, libxl only checks that a device is not assigned to another domain. This quick patch checks that the device is also owned by pciback, otherwise the call fails.
> > 
> > Signed-off-by: Doug Magee <djmagee@mageenet.net>
> 
> Thanks Doug. Would this be better done by adding a call to
> libxl_device_pci_list_assignable and looking for the device in it? In
> fact from the looks of things this could replace the existing call to
> get_all_assigned_devices from .._pci_add since
> libxl_device_pci_list_assignable already omits devices which are
> assigned to another domain.
> 
> Ian.
> 
> > 
> > diff -r 5b2676ac1321 -r 301cc006677f tools/libxl/libxl_pci.c
> > --- a/tools/libxl/libxl_pci.c	Mon Jan 09 16:01:44 2012 +0100
> > +++ b/tools/libxl/libxl_pci.c	Mon Jan 16 17:31:25 2012 -0500
> > @@ -796,6 +796,9 @@ int libxl__device_pci_add(libxl__gc *gc,
> >      libxl_device_pci *assigned;
> >      int num_assigned, i, rc;
> >      int stubdomid = 0;
> > +    struct dirent *de;
> > +    DIR *dir;
> > +    int assignable = 0;
> >  
> >      rc = get_all_assigned_devices(gc, &assigned, &num_assigned);
> >      if ( rc ) {
> > @@ -809,6 +812,35 @@ int libxl__device_pci_add(libxl__gc *gc,
> >          goto out;
> >      }
> >  
> > +    dir = opendir(SYSFS_PCIBACK_DRIVER);
> > +    if ( NULL == dir ) {
> > +        if ( errno == ENOENT ) {
> > +            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Looks like pciback driver not loaded");
> > +        }else{
> > +            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn't open %s", SYSFS_PCIBACK_DRIVER);
> > +        }
> > +        rc = ERROR_FAIL;
> > +        goto out_closedir;
> > +    }
> > +
> > +    while( (de = readdir(dir)) ) {
> > +        unsigned dom, bus, dev, func;
> > +        if ( sscanf(de->d_name, PCI_BDF, &dom, &bus, &dev, &func) != 4 )
> > +            continue;
> > +        if ( dom == pcidev->domain && bus == pcidev->bus &&
> > +              dev == pcidev->dev && func == pcidev->func ) {
> > +            assignable = 1;
> > +            break;
> > +        }
> > +    }
> > +
> > +    if ( !assignable ) {
> > +        rc = ERROR_FAIL;
> > +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "PCI device not owned by pciback");
> > +        goto out_closedir;
> > +    }
> > +
> > +
> >      libxl__device_pci_reset(gc, pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
> >  
> >      stubdomid = libxl_get_stubdom_id(ctx, domid);
> > @@ -817,7 +849,7 @@ int libxl__device_pci_add(libxl__gc *gc,
> >          /* stubdomain is always running by now, even at create time */
> >          rc = do_pci_add(gc, stubdomid, &pcidev_s, 0);
> >          if ( rc )
> > -            goto out;
> > +            goto out_closedir;
> >      }
> >  
> >      orig_vdev = pcidev->vdevfn & ~7U;
> > @@ -826,11 +858,11 @@ int libxl__device_pci_add(libxl__gc *gc,
> >          if ( !(pcidev->vdevfn >> 3) ) {
> >              LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Must specify a v-slot for multi-function devices");
> >              rc = ERROR_INVAL;
> > -            goto out;
> > +            goto out_closedir;
> >          }
> >          if ( pci_multifunction_check(gc, pcidev, &pfunc_mask) ) {
> >              rc = ERROR_FAIL;
> > -            goto out;
> > +            goto out_closedir;
> >          }
> >          pcidev->vfunc_mask &= pfunc_mask;
> >          /* so now vfunc_mask == pfunc_mask */
> > @@ -855,6 +887,8 @@ int libxl__device_pci_add(libxl__gc *gc,
> >          }
> >      }
> >  
> > +out_closedir:
> > +    closedir(dir);
> >  out:
> >      return rc;
> >  }
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel
> 
>