Re: [Xen-devel] [PATCH RFC v1] libxl: Introduce a template for devices with a controller

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Thu, 2015-05-21 at 18:07 +0100, George Dunlap wrote:
> We have several outstanding patch series which add devices that have
> two levels: a controller and individual devices attached to that
> controller.
> 
> In the interest of consistency, this patch introduces a section that
> sketches out a template for interfaces for such devices.

Thanks for taking this on!

> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> ---
> CC: Ian Campbell <ian.campbell@xxxxxxxxxx>
> CC: Ian Jackson <ian.jackson@xxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Juergen Gross <jgross@xxxxxxxx>
> CC: Chun Yan Liu <cyliu@xxxxxxxx>
> CC: Olaf Hering <olaf@xxxxxxxxx>
> 
> So, this is definitely RFC -- I tried to spec things out in a way that
> made sense, but I often just chose something that I thought would be a
> sensible starting point for discussion.
> 
> This spec looks a lot more like the PVUSB spec than the PVSCSI spec,
> in part because I think the PVUSB spec has already had a lot more
> thought that's gone into it.
> 
> A couple of random points to discuss:
> 
> * Calling things "controllers", using <type>ctrl for the device name,
>   and using "ctrl" as the field name for the devid of the controller
>   in the individual devices.

Controllers/ctrl is fine, as would Bus/bus. Or paint it pink ;-)

> * I've said that having an index (port, lun, whatever) is optional.
>   Do we want to make that requred?  Do we want it to have a consistent
>   name?  In the case of emulated USB, we can't really specify to qemu
>   what port the device gets attached to, so I'm tempted to say it's
>   not required; but even there we could always give it a port number
>   just for name's sake.

At the moment devid is not universally used, e.g. disk has ("vdev",
string), instead.

For devices (or controllers) where a flat integer devid space doesn't
make sense I think it would be fine to have something else, like a
<type>-specific devid type and a corresponding <type>ctrl specific
ctrlid type, which might be a (small) struct in either case and need not
be uniform across <type> and <type>ctrl nor among different <type>s.

Having this option of a more structured type might also help with the
conversation (in a subthread) regarding where in the
controller/bus/device/function the split between the two id spaces is,
since you aren't constrained to a single int at two levels trying to
describe 4 levels.

Using a string as disks do is probably best not considered as a
precedent here, but could be appropriate on a device-by-device basis.

> * Naming sub-devices.  We need to have a way to uniquely name both
>   controllers and subdevices.  Here I've said that we will have both
>   <type>ctrl and <type> devid namespaces, mainly because in the
>   previous point I opted not to require an index.  Another option
>   would be not to have another devid namespace, but to use
>   <ctrl,index> as the unique identifier.  (This would mean requiring
>   the index/port/lun specification above.)

I'm not quite sure what you are getting at here, in particular where we
will "have both <type>ctrl and <type> devid namespaces", in the structs
or in the protoptyes.

I was imaging that where the current templates have (ctx, domid, device)
that a controller related function would be (ctx, domid, ctrl) and the
device related function would become (ctx, domid, ctrlid, device). ctrl
and device would contain their respective ctrlid and devid fields.

Perhaps it might be clearer if the proposal included specific function
prototype templates along the lines of the existing
"libxl_device_<type>_add(ctx, domid, device)".

I think you were trying to avoid duplication by presenting this new
scheme as an extension to what is currently written there. It might be
clearer to simply present it as a separate alternative or to list both
possible prototypes next to the current section where things differ in
the two schemes? (and describe it in the text)

Or perhaps to insert some optional parameters into the existing ones
where it makes sense:
        libxl_device_<type>_add(ctx, domid, [ctrlid, ]device)

Not sure which would be least confusing...

> * libxl_device_<type>_list listing devices across all controllers.  I
>   think this is the most practical thing to do, but one could imagine
>   wanting to query by controller ID instead.

I agree with listing everything, I think.

If we find a need to only list per controller ID then we can add an
alternative (list_by_ctrl, device_<type>ctrl_list_devices, or whatever).

> Feedback welcome.

Having said all that I don't seem to have any more comments on the
actual text itself, apart from one typo.

> ---
>  tools/libxl/libxl.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 2ed7194..d757845 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1234,6 +1234,52 @@ void libxl_vtpminfo_list_free(libxl_vtpminfo *, int 
> nr_vtpms);
>   *
>   *   This function does not interact with the guest and therefore
>   *   cannot block on the guest.
> + *
> + * Controllers
> + * -----------
> + *
> + * Most devices are treated individually.  Some devices however, like
> + * USB or SCSI, inherently have the need to have "busses" or
> + * "controllers" to which individual devices can be attached.
> + *
> + * In that case, for each <type>, there will be two sets of the
> + * functions, types, and devid namespaces outlined above: one based on
> + * '<type>', and one based on '<type>ctrl'.
> + *
> + * In those cases, libxl_device_<type>ctrl_<function> will act more or
> + * less like top-level non-bus devices: they will either create or
> + * accept a libxl_devid which will be unique within the
> + * <type>ctrl libxl_devid namespace.
> + *
> + * Second-level devices which will be attached to a controller will
> + * include in their libxl_device_<type> a field called ctrl, which
> + * will be the libxl_devid of the corresponding controller.  It may also
> + * include an index onto that bus, that represents (for example) a USB
> + * port or a SCSI LUN.
> + *
> + * These second-level devices will also have their own devid which
> + * will be unique within the <type> devid namespace, and will be used
> + * for queries or removals.
> + *
> + * In the case where there are multiple different ways to implement a
> + * given device -- for instance, one which is fully PV and one which
> + * uses an emulator -- the controller will contain a field which
> + * specifies what type of implementation is used.  The implementations
> + * of individual devices will be known by the controller to which they are
> + * attached.
> + *
> + * If libxl_device_<type>_add receives an uninitialized ctrl devid, it
> + * may return an error.  Or it may (but is not required to) choose to
> + * automatically choose a suitable controller to which to attach the
> + * new device.  It may also (but is not required to) automatically
> + * create a new controller if no suitable controllers exist.
> + * Individual devices should document their behavior.
> + *
> + * libxl_device_<type>ctrl_list will list all controllers for the domain.
> + *
> + * libxl_device_<type>_list will list all devices for all controllers
> + * for the domain.  The individual libxl_device_<type> will include
> + * the devid of the controller to which it is attached.

[ijc -- inserted into, hopefully correct, place from following mail]
> For each type, the domain config file will contain a single list of
> controllers, and a single list of devices.  libxl will first iterate
> through the list adding the controlllers, then iterate through the list

"controllers"

> adding each device to the ctrl listed.  If libxl_device_<type>_add
> automatically creates controllers as necessary, then it is permissible
> for the controller list to be empty and the device list to have devices
> (with the ctrl field uninitialized).




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel