From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH] libxl_pci: check that host device is assignable before adding to the domain Date: Tue, 17 Jan 2012 14:16:22 +0000 Message-ID: <1326809782.14689.114.camel@zakaz.uk.xensource.com> References: <301cc006677f645bf4cb.1326753363@mnetdjm4.mageenet.host> <1326793645.14689.75.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: "djmagee@mageenet.net" Cc: "xen-devel@lists.xensource.com" , Ian Jackson , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org 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 > > 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 > >