[Xen-devel] Re: [PATCH 2 of 3] xl, implement pci-list-assignable-devices

[Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>]

On Wed, 2010-07-28 at 19:07 +0100, Stefano Stabellini wrote:
> > > > +libxl_device_pci *libxl_device_pci_list_assignable(struct libxl_ctx 
> > > > *ctx, int *num)
> > > > +{
> > > > +    libxl_device_pci *pcidevs = NULL;
> > > > +    libxl_device_pci *assigned;
> > > > +    int num_assigned;
> > > > +
> > > > +    *num = 0;
> > > > +
> > > > +    assigned = get_all_assigned_devices(ctx, &num_assigned);
> > > > +
> > > > +    pcidevs = scan_sys_pcidir(pcidevs, assigned, num_assigned,
> > > > +                              "/sys/bus/pci/drivers/pciback", num);
> > > > +    pcidevs = scan_sys_pcidir(pcidevs, assigned, num_assigned,
> > > > +                              "/sys/bus/pci/drivers/pcistub", num);
> > > > +
> > > > +    free(assigned);
> > > > +    return pcidevs;
> > > > +}
> > > > +
> > > 
> > > Same comment about returning an integer, also I don't like the way you
> > > are "assuming" scan_sys_pcidir is going to use realloc on pcidevs: that
> > > should be an implementation detail of scan_sys_pcidir that callers
> > > shouldn't rely upon.
> > 
> > Actually it's a static function with only one caller, this makes the
> > implementation of list_assignable as simple as possible.
> > 
> > > Consider that devices bound to pcistub cannot be assigned to PV guests,
> > > so I would remove scan_sys_pcidir on pcistub from here.
> > > Please define "/sys/bus/pci/drivers/pciback" in a macro.
> > 
> > But they can be assigned to HVM guests? Nothing in the add-device path
> > uses this list of devices anyway. What actually happens is a check
> > whether a device appears in the get_all_assigned_devices() list. Even
> > after this patch-set the user can always specify BDF's which aren't
> > really assignable (eg. the Host's PCI-to-ISA bridge or PCI root for
> > maximum amusement) That is to be addressed in a future patch.
> > 
> 
> Yes but libxl_device_pci_list_assignable is a public function, we don't
> know what libxl users are going to do with it.
> In particular it seems reasonable to call
> libxl_device_pci_list_assignable before assigning a device even if xl
> doesn't do it at the moment.

Yes but the realloc semantics don't apply to
libxl_device_pci_list_assignable only scan_sys_pcidir. Callers of the
former are free to do what they like with what is returned. My argument
is that implementation of the latter is wholly appropriate to it's only
caller. In other words, scan_sys_pcidir is not going to have any new
callers ever, as far as I can see anyway.

As for whether a device is assigned to picback or pcistub, I think the
semantics of list_assignable() ought to be that it returns anything
which may potentially be assignable at the time of calling. Unless we
want to implement some specific way of signalling which devices are
bound to which backend drivers. Personally I think these additional
details should be handled by letting an add-device fail with an
ERROR_INVAL so that callers don't need to know about backend-specific
details.

In other words I'd like to keep the code here as it is (modulo putting
sysfs paths in macros) Unless there's something deeper to this?

Gianni


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel