Re: [Xen-devel] [xen-devel][PATCH RFC] libxl: New xl interfaces for managing USB devices

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 06/24/2014 03:31 PM, Bo Cao wrote:
> Hey, all
>
> This patch is mainly about managing USB devices both for PVUSB and 
> DEVICEMODEL.It mainly consits of two part, USB controller creation and 
> destruction and USB device's attachment and detachment.
>
> For USB controller:
> usb-controller-create  <domain-id> <usb-version> <number-of-ports> [controller-type] 
> usb-controller-destroy <domain-id> <controller-name>
>
> For USB device:
> usb-attach <domain-id> <hostbus.hostdev> [controller_name] usb-detach <domain-id> 
> <hostbus.hostdev>
> usb-list   <domain-id>
>
> Feel free to give me any commnets.
> Thanks!
Hey Simon,

Sorry, I meant to get to this last week.  Comments below.

> Simon
>
> /**************************************************************/
> Design the new xl interfaces to managing USB devices,
> both for PVUSB and DEVICEMODEL.
>
> Signed-off-by: Simon Cao <caobosimon@xxxxxxxxx>
> ---
> CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> CC: Ian Jackson <ian.jackson@xxxxxxxxxx>
> CC: Ian Campbell <ian.campbell@xxxxxxxxxx>
> CC: Pasi KÃrkkÃinen <pasik@xxxxxx>
> ---
>   docs/man/xl.pod.1           |   39 +++++++++++++++++++++++++++++++++++++++
>   tools/libxl/libxl.h         |   26 ++++++++++++++++++++++++++
>   tools/libxl/libxl_types.idl |   27 +++++++++++++++++++++++++++
>   tools/libxl/xl_cmdtable.c   |   33 +++++++++++++++++++++++++++++++++
>   4 files changed, 125 insertions(+)
>
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index 30bd4bf..db76ce2 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -1194,6 +1194,45 @@ List virtual network interfaces for a domain.
>   
>   =back
>   
> +=head2 USB DEVICES
> +
> +=over 4
> +
> +=item B<usb-assginable-list>
Nit: typo.

I'm not really sure we need this one.  The only other time we have this 
is for PCI devices; but that's because PCI devices have an extra step: 
they must first be seized by pciback, and then assigned to the domain.
There is no equivalent (AFAIK) for USB devices to this; so this would 
just be duplicating the functionality of lsusb.  There's no 
"pci-list-seizable-devices" to show what devices can be passed to 
"pci-assignable-add"; you just have to use lspci.  We should follow suit 
here.
> +
> +List all the assignable USB devices.
> +
> +=item B<usb-assigned-list>
> +
> +List all the USB devices that has been assigned.
I take it the difference between this one and "usb-list" is that 
"usb-assigned-list" is meant to list usb devices across all domains, 
whereas "usb-list" is meant to be for just a single domain?
We don't have this functionality for pci devices; if we want that I 
think we should probably just implement it as an option to usb-list 
(e.g., "xl usb-list -a" to list devices across all domains).
> +
> +=item B<usb-controller-create> I<usb-version> I<number_of_ports> 
> [I<controller_type>]
> +
> +Creates a new usb virtual controller for the domain specified by I<domain-id>.
> +I<usb-version> describes the version of USB controller to create, i.e., 1, 2 
> or 3,
> +I<number_of_ports> describers the number of ports in the USB virtual 
> controller, and
> +I<controller_type> describes the type of USB controller, PVUSB or EMULATED.
> +
> +=item B<usb-controller-destroy> I<domain-id> I<controller-name>
> +
> +Destroies the virtual USB controller specified by I<controller-name> in 
> I<domain-id>.
In keeping with the other device commands, these should be 
"usb-controller-attach" and "usb-controller-detach".  Other than that, 
looks good.
> +
> +=item B<usb-attach> I<domain-id> I<hostbus.hostaddr> [I<controller-name>]
> +
> +Attaches USB device specified by I<hostbus.hostaddr> to USB controller 
> specified by
> +I<controller-name> in domain I<domain-id>.
> +
> +=item B<usb-detach> I<domain-id>  I<hostbus.hostaddr>
> +
> +Detaches the USB device I<hostbus.hostaddr> in domain I<domain-id>.
We want to use the same interface to be able to add other kinds of 
devices (tablets, mice, keyboards, maybe mass storage) in the future. I 
think for now this should be something like "hostdev:<hostbus.hostaddr>".
Other than that, looks good.

> +
> +=item B<usb-list> I<domain-id>
> +
> +List all the virtual USB controllers and USB devices for the domain
> +specified by I<domain-id>.
> +
> += back
> +
>   =head2 VTPM DEVICES
>   
>   =over 4
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 17b8a7b..863c86c 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -950,6 +950,32 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, 
> libxl_device_disk *disk,
>                          const libxl_asyncop_how *ao_how)
>                          LIBXL_EXTERNAL_CALLERS_ONLY;
>   
> +/* USB Devices*/
> +int libxl_device_usb_controller_create(libxl_ctxlibxl_ctx *ctx, uint32_t domid,
> +                                                libxl_device_usb_controller 
> *dev,
> +                         const libxl_asyncop_how *ao_how)
> +                         LIBXL_EXTERNAL_CALLERS_ONLY;
> +
> +int libxl_device_usb_controller_destroy(libxl_ctxlibxl_ctx *ctx, uint32_t 
> domid,
> +                         libxl_device_usb_controller *dev,
> +                         const libxl_asyncop_how *ao_how)
> +                         LIBXL_EXTERNAL_CALLERS_ONLY;
These should be libxl_device_usb_controller_{add,remove,destroy,list}.  
Looks good otherwise.
> +
> +int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid,
> +                         libxl_device_usb *dev,
> +                         const libxl_asyncop_how *ao_how)
> +                         LIBXL_EXTERNAL_CALLERS_ONLY;
> +
> +int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid,
> +                            libxl_device_usb *dev,
> +                            const libxl_asyncop_how *ao_how)
> +                            LIBXL_EXTERNAL_CALLERS_ONLY;
You also need libxl_device_usb_destroy().

The difference in theory between remove and destroy is for things like 
PV disks: remove will remove the disk with the cooperation of the guest, 
but may fail if the guest is buggy or otherwise doesn't cooperate.  
Destoy will yank the disk out whether the guest likes it or not 
(potentially resulting in data corruption).
For a lot of devices (I think pci is like this) there's actually no 
distinction, and they both map to the same function internally.  Not 
sure what the PVUSB protocol is like in this regard; but in any case you 
need both functions even if there's no practical difference between the two.
> +
> +libxl_device_usb *libxl_device_usb_list(libxl_ctx *ctx, uint32_t domid,
> +                                        int *num)
> +                          LIBXL_EXTERNAL_CALLERS_ONLY;
> +
> +
>   /* Network Interfaces */
>   int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic 
> *nic,
>                            const libxl_asyncop_how *ao_how)
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index f0f6e34..28d896e 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -85,6 +85,16 @@ libxl_nic_type = Enumeration("nic_type", [
>       (2, "VIF"),
>       ])
>   
> +libxl_usb_controller_type = Enumeration("usb_controller_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"),
>   
> @@ -448,6 +458,22 @@ libxl_device_pci = Struct("device_pci", [
>       ("seize", bool),
>       ])
>   
> +libxl_device_usb_controller = Struct("device_usb_controller", [
> +       ("name", string),
> +       ("type", libxl_usb_controller_type),
> +       ("backend_domid", libxl_domid),
> +       ("backend_domname",  string),
> +       ("usb_version", uint8),
> +       ("num_ports", uint8),
> +       ])
Looks good.

Hmm, also just noticed that you're using hard tabs.  Xen coding style 
says you should only be using spaces for indentation; tabs are forbidden.
If you're using emacs, the comment at the end of most source files 
should do this automatically; if you're using a different editor, you'll 
have to figure out how to make it use only spaces.
> +
> +libxl_device_usb = Struct("device_usb", [
> +       ("controller_name", string),
> +       ("type", libxl_usb_type),
> +       ("bus", uint8),
> +       ("dev", uint8),
> +       ])
Is there a reason you got rid of the KeyedUnion construct?

> +       
>   libxl_device_vtpm = Struct("device_vtpm", [
>       ("backend_domid",    libxl_domid),
>       ("backend_domname",  string),
> @@ -462,6 +488,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")),
You need a list of controllers as well as a list of usb devices.

(You can still have the domain builder automatically create usb 
controllers as needed if the list is empty.)
Thanks -- look forward to seeing the code!

 -George


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