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

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

[Bo Cao]

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!

Simon 

/**************************************************************/
Design the new xl interfaces to managing USB devices,
both for PVUSB and DEVICEMODEL.

Signed-off-by: Simon Cao <caobosimon@gmail.com>
---
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Pasi Kärkkäinen <pasik@iki.fi>
---
 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>
+
+List all the assignable USB devices.
+
+=item B<usb-assigned-list>
+
+List all the USB devices that has been assigned.
+
+=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>.
+
+=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>.
+
+=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;
+
+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;
+
+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),
+	])
+
+libxl_device_usb = Struct("device_usb", [
+	("controller_name", string),
+	("type", libxl_usb_type),
+	("bus", uint8),
+	("dev", uint8),
+	]) 
+	 
 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")),
     ("vfbs", Array(libxl_device_vfb, "num_vfbs")),
     ("vkbs", Array(libxl_device_vkb, "num_vkbs")),
     ("vtpms", Array(libxl_device_vtpm, "num_vtpms")),
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 4279b9f..6c0ae9a 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -194,6 +194,39 @@ struct cmd_spec cmd_table[] = {
       "Eject a cdrom from a guest's cd drive",
       "<Domain> <VirtualDevice>",
     },
+	{ "usb-assignable-list",
+	  &main_usb_assignable_list, 0, 0,
+	  "List all the assignable USB devices",
+	},
+	{ "usb-assigned-list",
+	  &main_usb_assigned_list. 0, 0,
+	  "List all the USB devices that have been assigned",
+	},
+	{ "usb-controller-create",
+	  &main_usb_controller_create, 1, 1,
+	  "Create a virtual USB controller for a domain",
+	  "<Domain> <UsbVersion['1'[1.1]|'2'[2.0]|'3'[3.0]]> <NumberOfPorts> [ControllerType['pv'|'emu']]",
+	}
+	{ "usb-controller-destroy",
+	  &main_usb_controller_destroy, 0, 1,
+	  "Destory the virtual USB controller specified by <ControllerName> for a domain",
+	  "<Domain> <ControllerName['pv0'|'hci0'|..]>",
+	},
+	{ "usb-attach",
+	  &main_usb_attach, 1, 1,
+	  "Attach a USB device to a domain",
+	  "<Domain> <Hostbus.Hostdev> [ControllerName['pv0'|'hci0']]",
+	},
+	{ "usb-detach",
+	  &main_usb_detach, 0, 1,
+	  "Detach a USB device from a domain",
+	  "<Domain> <Hostbus.Hostdev>",
+	},
+	{ "usb-list",
+	  &main_usb_list, 0, 0,
+	  "List information about USB devices for a domain",
+	  "<Domain>",
+	},
     { "mem-max",
       &main_memmax, 0, 1,
       "Set the maximum amount reservation for a domain",
-- 
1.7.10.4


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

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

[George Dunlap]

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@gmail.com>
> ---
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Pasi Kärkkäinen <pasik@iki.fi>
> ---
>   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@lists.xen.org
http://lists.xen.org/xen-devel

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

[Simon Cao]

[-- Attachment #1.1: Type: text/plain, Size: 10851 bytes --]

2014-07-02 19:07 GMT+08:00 George Dunlap <george.dunlap@eu.citrix.com>:

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@gmail.com>
>> ---
>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>> CC: Ian Jackson <ian.jackson@citrix.com>
>> CC: Ian Campbell <ian.campbell@citrix.com>
>> CC: Pasi Kärkkäinen <pasik@iki.fi>
>> ---
>>   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.
>
>
I list this command here because it exists in the previous xm/xend
commands. Since the devices listed by "lsusb" isn't all assignable, such as
the USB hubs, "usb-assginable-list" will list all the assignable devices in
Dom0 that can be attached to guest VM in a manner that is suitable to
usb-attach.


>
>  +
>> +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).


"usb-assigned-list" is used to list all the devices that have been assigned
to certain guest VMs so that users can choose from this list to detach the
USB device using  "usb-detach". It's mainly for the convenience of
usb-detach, but as you've said, "xl usb-list -a" is probably a better
implementation.

 +
>> +=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.
>
>
Since the controller is created from NULL, not just attaching a device from
Dom0 to DomU, maybe "create" and "destroy" are better ?


>  +
>> +=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.




In this case, to specify a hostdev device we can use "hostdev =
<hostbus.hostaddr>".


>  +
>> +=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.
>
>
Okay, I will add the "-destroy" function.


>
>  +
>> +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.
>
> I am using Vim, and I will replace tabs with 4 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?
>
> I don't know the benefit of using KeyedUnion construct. Please tell me the
benefit of it.


>
>  +
>>   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.)
>
> Yes, I will modify this.

Regards,

Bo

[-- Attachment #1.2: Type: text/html, Size: 17208 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[George Dunlap]

[-- Attachment #1.1: Type: text/plain, Size: 7675 bytes --]

On 07/03/2014 01:56 PM, Simon Cao wrote:
>
>         ---
>           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.
>
>
> I list this command here because it exists in the previous xm/xend 
> commands. Since the devices listed by "lsusb" isn't all assignable, 
> such as the USB hubs, "usb-assginable-list" will list all the 
> assignable devices in Dom0 that can be attached to guest VM in a 
> manner that is suitable to usb-attach.

OK -- well I think that's a bit incidental to the core functionality.  I 
would put off working on this until the core functionality is working; 
and then I'd put it in its own patch, so we can consider whether to 
include it separately.


>
>         +
>         +=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.
>
> Since the controller is created from NULL, not just attaching a device 
> from Dom0 to DomU, maybe "create" and "destroy" are better ?

Yeah, I understand the logic.  But that's how we name network devices, 
for instance, which are created from nothing before being attached.  In 
any case, you're still combining the "create" and the "attach" operation 
into one.  Having "attach" mean "create and attach" isn't really that 
different from having "create" mean "create and attach".

Anyway, I'm pretty sure the maintainers will insist on attach / detach 
before accepting it (they asked me to change it when I posted it a year 
ago), so you might as well just do it now. :-)

>
>
>         +
>         +=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. 
>
> In this case, to specify a hostdev device we can use "hostdev = 
> <hostbus.hostaddr>".

OK -- well I'm sure there will be comments on the exact syntax before 
the patches are finally accepted. :-)

>
>
>         +
>         +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?
>
> I don't know the benefit of using KeyedUnion construct. Please tell me 
> the benefit of it.

So this idl code:

libxl_device_usb = Struct("device_usb", [
         ("protocol", libxl_device_usb_protocol,
          {'init_val': 'LIBXL_USB_PROTOCOL_AUTO'}),
         ("backend_domid",    libxl_domid),
         ("backend_domname",  string),
         ("u", KeyedUnion(None, libxl_device_usb_type, "type",
                          [("hostdev", Struct(None, [
                                 ("hostbus",   integer),
                                 ("hostaddr",  integer) ]))
                           ]))
     ])

Generated this header code:

typedef struct libxl_device_usb {
     libxl_usb_protocol protocol;
     libxl_domid backend_domid;
     char * backend_domname;
     libxl_device_usb_type type;
     union {
         struct {
             int hostbus;
             int hostaddr;
         } hostdev;
     } u;
} libxl_device_usb;
void libxl_device_usb_dispose(libxl_device_usb *p);
void libxl_device_usb_init(libxl_device_usb *p);
void libxl_device_usb_init_type(libxl_device_usb *p, 
libxl_device_usb_type type);
char *libxl_device_usb_to_json(libxl_ctx *ctx, libxl_device_usb *p);

So first of all it makes a union; so that if we eventually have, say, a 
mass storage type, you won't just have to toss all the possible 
different parameters into the struct together; it will only be as long 
as the longest type.  Secondly, it tells the idl compiler that the value 
in "type" tells you what kind of union you're dealing with, so that the 
"usb_to_json" functions get build properly.  For example, the above idl 
code generates the following code snippet in 
_libxl_types.c:libxl_device_usb_gen_json():

     switch (p->type) {
     case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
         s = yajl_gen_map_open(hand);
         if (s != yajl_gen_status_ok)
             goto out;
         s = yajl_gen_string(hand, (const unsigned char *)"hostbus", 
sizeof("hostbus")-1);
         if (s != yajl_gen_status_ok)
             goto out;
         s = yajl_gen_integer(hand, p->u.hostdev.hostbus);
         if (s != yajl_gen_status_ok)
             goto out;
         s = yajl_gen_string(hand, (const unsigned char *)"hostaddr", 
sizeof("hostaddr")-1);
         if (s != yajl_gen_status_ok)
             goto out;
         s = yajl_gen_integer(hand, p->u.hostdev.hostaddr);
         if (s != yajl_gen_status_ok)
             goto out;
         s = yajl_gen_map_close(hand);
         if (s != yajl_gen_status_ok)
             goto out;
         break;
     }

So it does specific things based on what "type" is set to: if it's set 
to "hostdev" it outputs hostbus and hostaddr; if we added a mass storage 
type, and type was set to "storage", it would only output the 
information relevant to the mass storage device.

Cool, look forward to the code. :-)

  -George

[-- Attachment #1.2: Type: text/html, Size: 16349 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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