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

[Ian Campbell <ian.campbell@citrix.com>]

On Mon, 2015-01-19 at 16:28 +0800, Chunyan Liu wrote:

Sorry for the long delay in replying.

> To attach a usb device, a virtual usb controller should be created first.
> This patch defines usbctrl and usbdevice related structs.

Per <54CA17DF0200006600095E3D@relay2.provo.novell.com> please could you
mention here that the HVM guest related parts (i.e.
LIBXL_USBCTRL_TYPE_DEVICEMODEL) and libxl_usb_type are placeholders for
emulated HVM support.

In fact I wonder if it should just be omitted, we will need a LIBXL_HAVE
for HVM USB support anyway once it is implemented so we can add the enum
then.

Or will we -- do we think returning an error for such an HVM guest with
USB devices configured now is acceptable and for it to silently start
working at some point in the future is OK?

> 
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> Signed-off-by: Simon Cao <caobosimon@gmail.com>
> ---
>  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
> @@ -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),

I think int would be fine for both of these last two (and is a bit
kinder to language bindings).

> +    ])
> +
> +libxl_device_usb = Struct("device_usb", [
> +    ("ctrl", integer),

Is this an index into something? If so what?

There seems to be no usbctrl array added to the domain_config struct, so
I'm unsure how this is used.

> +    ("port", integer),

Port on the hub?
> +    ("intf", string),

What is this one? (This may just be my lack of usb knowledge)

> +    ("u", KeyedUnion(None, libxl_usb_type, "type",
> +        [("hostdev", Struct(None, [
> +            ("hostbus",   integer),
> +            ("hostaddr",  integer) ]))
> +        ]))
> +    ])
> +
> @@ -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")),

So, I'm unsure how this interacts with the controllers, which it doesn't
seem to be possible to specify at domain build time.

You pointed me to
http://www.redhat.com/archives/libvir-list/2014-June/msg00038.html but
having had a look through I can't see it.

For v3 please could you give an overview/summary of how it fits together
in the 0/N patch.