Re: [PATCH V8 7/7] domcreate: support pvusb in configuration file

[George Dunlap <George.Dunlap@eu.citrix.com>]

On Wed, Oct 21, 2015 at 10:08 AM, Chunyan Liu <cyliu@suse.com> wrote:
> Add code to support pvusb in domain config file. One could specify
> usbctrl and usb in domain's configuration file and create domain,
> then usb controllers will be created and usb device would be attached
> to guest automatically.
>
> One could specify usb controllers and usb devices in config file
> like this:
> usbctrl=['version=2,ports=4', 'version=1, ports=4', ]
> usbdev=['hostbus=2, hostaddr=1, controller=0,port=1', ]
>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> Signed-off-by: Simon Cao <caobosimon@gmail.com>
>
> ---
> changes:
>   - change parse_usb_config and parse_usbctrl_config, following
>     parse_nic_config way, and move to previous patch
>   - update user interface to specify hostbus, hostaddr instead of
>     hostbus.hostaddr for future extension
>
>  docs/man/xl.cfg.pod.5        | 81 ++++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_create.c   | 73 +++++++++++++++++++++++++++++++++++++--
>  tools/libxl/libxl_device.c   |  4 +++
>  tools/libxl/libxl_internal.h |  8 +++++
>  tools/libxl/xl_cmdimpl.c     | 54 ++++++++++++++++++++++++++++-
>  5 files changed, 216 insertions(+), 4 deletions(-)
>
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index b63846a..f24c008 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -722,6 +722,87 @@ Note this may be overridden by rdm_policy option in PCI device configuration.
>
>  =back
>
> +=item B<usbctrl=[ "USBCTRL_SPEC_STRING", "USBCTRL_SPEC_STRING", ... ]>
> +
> +Specifies the USB controllers created for this guest. Each
> +B<USB_SPEC_STRING> has the form C<KEY=VALUE,KEY=VALUE,...> where:
> +
> +=over 4
> +
> +=item B<KEY=VALUE>
> +
> +Possible B<KEY>s are:
> +
> +=over 4
> +
> +=item B<type=TYPE>
> +
> +Specifies the protocol to implement USB controller, could be "pv" (indicates
> +PVUSB) or "qemu" (indicates QEMU emulated). Currently only "pv" is supported.

I think most of these should basically be copied from the descriptions
of the arguments in patch 5.

> +=item B<version=VERSION>
> +
> +Specifies version of the USB controller, could be 1 (USB1.1) or 2 (USB2.0).
> +Default is 2 (USB2.0).
> +
> +=item B<ports=PORTS>
> +
> +Specifies port number of the USB controller. Default is 8.
> +
> +Each USB controller will have an index starting from 0. On the same
> +controller, each port will have an index starting from 1.

I'd say:

"USB controler ids start from 0.  In line with the USB spec, however,
ports on a controller start from 1."

> +
> +E.g.
> +usbctrl=["version=1,ports=4", "version=2,ports=8",]
> +The first controller has:
> +controller index = 0, and port 1,2,3,4.
> +The second controller has:
> +controller index = 1, and port 1,2,3,4,5,6,7,8.

The example is good, but I'd use "controller id" rather than "controller index".

> +=back
> +
> +=back
> +
> +=item B<usbdev=[ "USB_SPEC_STRING", "USB_SPEC_STRING", ... ]>
> +
> +Specifies the host USB devices to passthrough to this guest. Each
> +B<USB_SPEC_STRING> has the form C<KEY=VALUE,KEY=VALUE,...> where:

"Specifiec the USB devices to be attached to the guest at boot."
(i.e., don't assume all devices are hostdev.)

> +=over 4
> +
> +=item B<KEY=VALUE>
> +
> +Possible B<KEY>s are:
> +
> +=over 4
> +
> +=item B<devtype=hostdev>
> +
> +Specifies USB device type. Currently only support 'hostdev'.
> +
> +=item B<hostbus=busnum>
> +
> +Specifies busnum of the USB device from the host perspective.
> +
> +=item B<hostaddr=devnum>
> +
> +Specifies devnum of the USB device from the host perspective.
> +
> +=item B<controller=CONTROLLER>
> +
> +Specifies USB controller index, to which controller the USB device is attached.
> +
> +=item B<port=PORT>
> +
> +Specifies USB port index, to which port the USB device is attached. B<port=PORT>
> +is valid only when B<controller=CONTROLLER> is specified. Without
> +B<controller=CONTROLLER>, it will find the first available USB controller:port
> +and use it. If there is no controller at all, it will create one.

I think the last sentence should be a separate paragraph, probably
after the "=back".  And a more idiomatic way to write it might be:

"If no controller is specified, an available controller:port
combination will be used.  If there are no available controller:port
options, a new controller will be created."

Only one other minor comment from me:

> +    if (!xlu_cfg_get_list(config, "usbctrl", &usbctrls, 0, 0)) {
> +        d_config->num_usbctrls = 0;
> +        d_config->usbctrls = NULL;
> +        while ((buf = xlu_cfg_get_listitem(usbctrls, d_config->num_usbctrls))
> +               != NULL) {

I don't know what the other maintainers think, but particularly given
that this is spanning a line, I would personally take out the
comparison, and just make it

 while ((buf = ...)) {
 ...
}

Other than that, this one looks good to me.

 -George