[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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> 
usb-list   <domain-id>

Feel free to give me any commnets.

Hey Simon,

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


Design the new xl interfaces to managing USB devices,

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

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
=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)
+/* USB Devices*/
+int libxl_device_usb_controller_create(libxl_ctxlibxl_ctx *ctx, uint32_t domid,
+                                                libxl_device_usb_controller 
+                         const libxl_asyncop_how *ao_how)
+                         LIBXL_EXTERNAL_CALLERS_ONLY;
+int libxl_device_usb_controller_destroy(libxl_ctxlibxl_ctx *ctx, uint32_t 
+                         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 
                           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!


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.