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

[Simon Cao <caobosimon@xxxxxxxxx>]

2014-07-02 19:07 GMT+08:00 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.

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

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