Re: [PATCH RFC V2 1/5] libxl: add pvusb definitions

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

On 01/19/2015 08:28 AM, Chunyan Liu wrote:
> To attach a usb device, a virtual usb controller should be created first.
> This patch defines usbctrl and usbdevice related structs.
> 
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> Signed-off-by: Simon Cao <caobosimon@gmail.com>

Chunyan, thanks for picking up this work!

A couple of things.  First, I think that having the IDL stuff separate
from the patches where they are used actually makes it *harder* to
review, because you can't easily go to the code where it's used and see
what's actually happening.

I think that the IDL stuff used in patch 3 should be in patch 3; and the
domain creation IDL stuff should be included in patch 5.

> ---
>  tools/libxl/libxl_types.idl          | 58 +++++++++++++++++++++++++++++++++++-
>  tools/libxl/libxl_types_internal.idl |  1 +
>  2 files changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 1214d2e..0639434 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -111,6 +111,16 @@ libxl_nic_type = Enumeration("nic_type", [
>      (2, "VIF"),
>      ])
>  
> +libxl_usbctrl_type = Enumeration("usbctrl_type",[
> +    (0, "AUTO"),
> +    (1, "PV"),
> +    (2, "DEVICEMODEL"),
> +    ])
> +
> +libxl_usb_type = Enumeration("device_usb_type", [
> +    (1, "HOSTDEV"),
> +    ])
> +
>  libxl_action_on_shutdown = Enumeration("action_on_shutdown", [
>      (1, "DESTROY"),
>  
> @@ -394,7 +404,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      ("ioports",          Array(libxl_ioport_range, "num_ioports")),
>      ("irqs",             Array(uint32, "num_irqs")),
>      ("iomem",            Array(libxl_iomem_range, "num_iomem")),
> -    ("claim_mode",	     libxl_defbool),
> +    ("claim_mode",       libxl_defbool),

Spurious whitespace change -- please kill this.

>      ("event_channels",   uint32),
>      ("kernel",           string),
>      ("cmdline",          string),
> @@ -521,6 +531,27 @@ libxl_device_pci = Struct("device_pci", [
>      ("seize", bool),
>      ])
>  
> +libxl_device_usbctrl = Struct("device_usbctrl", [
> +    ("name", string),
> +    ("type", libxl_usbctrl_type),
> +    ("backend_domid", libxl_domid),
> +    ("backend_domname", string),
> +    ("devid", libxl_devid),
> +    ("usb_version", uint8),
> +    ("num_ports", uint8),
> +    ])
> +
> +libxl_device_usb = Struct("device_usb", [
> +    ("ctrl", integer),
> +    ("port", integer),
> +    ("intf", string),
> +    ("u", KeyedUnion(None, libxl_usb_type, "type",
> +        [("hostdev", Struct(None, [
> +            ("hostbus",   integer),
> +            ("hostaddr",  integer) ]))
> +        ]))
> +    ])

So "intf" here is wrong.  To begin with, it's information specific to
the "hostdev" type; so it would go under the "type" keyed union under
"hostdev".

Secondly, this requires people to figure out that their media reader has
an intf of "1-2.1.1:1.0".  I don't think that's a good idea, for two
reasons: one, it just seems like a really hard interface to use.  I
couldn't find any straightforward tools to map USB devices onto intf;
tools like "lsusb" instead give you a bus:addr combination.  Secondly,
it's inconsistent with qemu -- which means we'd either have to have two
different ways of specifying the device, or we'd need to translate from
"intf" back into bus:addr

I think the right thing to do here is to take "intf" out of this struct,
and to translate "bus:addr" into intf internally.

It looks like the values qemu and lsusb use can be found in "busnum" and
"devnum" in the sysfs files.  You've already got code to scan those
devices; you just need to add a bit of code to find which of those
corresponds to a given "hostbus:hostaddr" combination.

> +
>  libxl_device_vtpm = Struct("device_vtpm", [
>      ("backend_domid",    libxl_domid),
>      ("backend_domname",  string),
> @@ -547,6 +578,7 @@ libxl_domain_config = Struct("domain_config", [
>      ("disks", Array(libxl_device_disk, "num_disks")),
>      ("nics", Array(libxl_device_nic, "num_nics")),
>      ("pcidevs", Array(libxl_device_pci, "num_pcidevs")),
> +    ("usbs", Array(libxl_device_usb, "num_usbs")),

Any reason you don't make it possible to specify usb controllers as well?

Thanks again for picking this up.  I'll give feedback on the other
patches tomorrow.

 -George