|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V4 3/7] libxl: add pvusb API
On 06/10/2015 04:20 AM, Chunyan Liu wrote:
> Add pvusb APIs, including:
> - attach/detach (create/destroy) virtual usb controller.
> - attach/detach usb device
> - list usb controller and usb devices
> - some other helper functions
>
> Signed-off-by: Chunyan Liu <cyliu@xxxxxxxx>
> Signed-off-by: Simon Cao <caobosimon@xxxxxxxxx>
>
> ---
> changes:
> - make libxl_device_usbctrl_add async, to be consistent with
> libxl_device_usbctrl_remove, using MACROs DEFINE_DEVICE_ADD
> and DEFINE_DEVICES_ADD, adjusting related codes.
> - correct update_json related processing.
> - use libxl__* helper functions instead of calloc, realloc
> and strdup, etc. whereever possible.
> - merge protocol definition pv|qemu in this patch
> - treat it as warning at rebind failure instead of error in usb_remove
> - add documentation docs/misc/pvusb.txt to describe pvusb
> details (toolstack design, libxl design, xenstore info, etc.)
> - other fixes addring Wei and George's comments
>
> docs/misc/pvusb.txt | 243 +++++++
> tools/libxl/Makefile | 2 +-
> tools/libxl/libxl.c | 6 +
> tools/libxl/libxl.h | 65 ++
> tools/libxl/libxl_internal.h | 16 +-
> tools/libxl/libxl_osdeps.h | 13 +
> tools/libxl/libxl_pvusb.c | 1249
> ++++++++++++++++++++++++++++++++++
> tools/libxl/libxl_types.idl | 52 ++
> tools/libxl/libxl_types_internal.idl | 1 +
> tools/libxl/libxl_utils.c | 16 +
> 10 files changed, 1661 insertions(+), 2 deletions(-)
> create mode 100644 docs/misc/pvusb.txt
> create mode 100644 tools/libxl/libxl_pvusb.c
>
> diff --git a/docs/misc/pvusb.txt b/docs/misc/pvusb.txt
> new file mode 100644
> index 0000000..4cdf965
> --- /dev/null
> +++ b/docs/misc/pvusb.txt
> @@ -0,0 +1,243 @@
> +1. Overview
> +
> +There are two general methods for passing through individual host
> +devices to a guest. The first is via an emulated USB device
> +controller; the second is PVUSB.
> +
> +Additionally, there are two ways to add USB devices to a guest: via
> +the config file at domain creation time, and via hot-plug while the VM
> +is running.
> +
> +* Emulated USB
> +
> +In emulated USB, the device model (qemu) presents an emulated USB
> +controller to the guest. The device model process then grabs control
> +of the device from domain 0 and and passes the USB commands between
> +the guest OS and the host USB device.
> +
> +This method is only available to HVM domains, and is not available for
> +domains running with device model stubdomains.
> +
> +* PVUSB
> +
> +PVUSB uses a paravirtialized front-end/back-end interface, similar to
> +the traditional Xen PV network and disk protocols. In order to use
> +PVUSB, you need usbfront in your guest OS, and usbback in dom0 (or
> +your USB driver domain).
> +
> +Additionally, for easy use of PVUSB, you need support in the toolstack
> +to get the two sides to talk to each other.
I think this paragraph is probably unnecessary. By the time this is
checked in, the toolstack will have the necessary support.
> +2. Specifying a host USB device
> +
> +QEMU hmp commands allows USB devices to be specified either by their
s/hmp/qmp/; ?
> +bus address (in the form bus.device) or their device tag (in the form
> +vendorid:deviceid).
> +
> +Each way of specifying has its advantages:
> +
> + Specifying by device tag will always get the same device,
> +regardless of where the device ends up in the USB bus topology.
> +However, if there are two identical devices, it will not allow you to
> +specify which one.
> +
> + Specifying by bus address will always allow you to choose a
> +specific device, even if you have duplicates. However, the bus address
> +may change depending on which port you plugged the device into, and
> +possibly also after a reboot.
> +
> +To avoid duplication of vendorid:deviceid, we'll use bus address to
> +specify host USB device in xl toolstack.
This paragraph comparing the different ways of specifying devices makes
sense to include in the 0/N series summary, but not so much in a file
we're checking in.
> +
> +You can use lsusb to list the USB devices on the system:
> +
> +Bus 001 Device 003: ID 0424:2514 Standard Microsystems Corp. USB 2.0
> +Hub
> +Bus 003 Device 002: ID f617:0905
> +Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> +Bus 001 Device 004: ID 0424:2640 Standard Microsystems Corp. USB 2.0
> +Hub
> +Bus 001 Device 005: ID 0424:4060 Standard Microsystems Corp. Ultra
> +Fast Media Reader
> +Bus 001 Device 006: ID 046d:c016 Logitech, Inc. Optical Wheel Mouse
> +
> +To pass through the Logitec mouse, for instance, you could specify
> +1.6 (remove leading zeroes).
> +
> +Note: USB Hub could not be assigned to guest.
"USB hubs cannot be assigned to a guest."
[snip]
> +int libxl_device_usbctrl_getinfo(libxl_ctx *ctx, uint32_t domid,
> + libxl_device_usbctrl *usbctrl,
> + libxl_usbctrlinfo *usbctrlinfo)
> +{
> + GC_INIT(ctx);
> + char *dompath, *usbctrlpath;
> + char *val;
> + int rc = 0;
> +
> + usbctrlinfo->devid = usbctrl->devid;
> + usbctrlinfo->ports = usbctrl->ports;
> + usbctrlinfo->version = usbctrl->version;
> + usbctrlinfo->protocol = usbctrl->protocol;
You seem to have missed my message about this -- the only thing you are
allowed to read from usbctrl in this function is the devid. Everything
else you have to look up and give back to the user. That's the point of
the function -- the user has the devid and is asking *you* how many
ports there are, what usb version it is, &c.
[snip]
> +/* get usb devices under certain usb controller */
> +static int
> +libxl__device_usb_list_per_usbctrl(libxl__gc *gc, uint32_t domid, int
> usbctrl,
Should usbctrl be of type libxl_devid?
> +static int libxl__device_usb_setdefault(libxl__gc *gc, uint32_t domid,
> + libxl_device_usb *usb,
> + bool update_json)
> +{
> + char *be_path, *tmp;
> +
> + if (usb->ctrl == -1) {
> + int ret = libxl__device_usb_set_default_usbctrl(gc, domid, usb);
> + /* If no existing ctrl to host this usb device, setup a new one */
> + if (ret) {
> + libxl_device_usbctrl usbctrl;
> + libxl_device_usbctrl_init(&usbctrl);
> + if (libxl_device_usbctrl_add_common(CTX, domid, &usbctrl,
> + 0, update_json)) {
> + LOG(ERROR, "Failed to create usb controller");
> + return ERROR_INVAL;
> + }
> + usb->ctrl = usbctrl.devid;
> + usb->port = 1;
> + libxl_device_usbctrl_dispose(&usbctrl);
> + }
> + }
Sorry for not noticing this before -- it looks like if you set
usb->ctrl to -1, it will automatically choose both a controller and a
port number. But what if you want to specify that you want a particular
controller (for example, if you want to specify the PV controller rather
than the emulated one, or vice versa), but didn't want to have to
manually keep track of which ports were free?
It seems like it would be better to have the code treat port 0 as
"automatically choose a port for me".
(If this were the only thing holding it up I would say this wouldn't be
a blocker to going in.)
> +static int libxl__device_usb_remove(libxl__gc *gc, uint32_t domid,
> + libxl_device_usb *usb)
> +{
> + libxl_device_usb *usbs = NULL;
> + libxl_device_usb *usb_find = NULL;
> + int i, num = 0, rc;
> +
> + assert(usb->busid || (usb->hostbus > 0 && usb->hostaddr > 0));
> +
> + if (!usb->busid) {
> + usb->busid = usb_busaddr_to_busid(gc, usb->hostbus, usb->hostaddr);
> + if (!usb->busid) {
> + LOG(ERROR, "USB device doesn't exist in sysfs");
> + return ERROR_INVAL;
> + }
> + }
So here you're keying the removal on the *host* idea of what the device
is. But the standard would be to key this on the *guest* idea of what
the device is. When you're doing disk removal, you don't say
"xl block-detach 1 /images/foo.img"
that is, the path to the disk image; you say
"xl block-detach 1 xvda"
that is, the image as seen from the guest.
Since there is no devid, you should make it possible to remove by
<ctrl,port>. Removing also by hostbus:hostaddr seems like useful
functionality, so it's probably not bad to keep it; but the <ctrl,port>
should be the main functionality.
> +int libxl_device_usb_getinfo(libxl_ctx *ctx, libxl_device_usb *usb,
> + libxl_usbinfo *usbinfo)
> +{
> + GC_INIT(ctx);
> + char *filename;
> + void *buf = NULL;
> +
> + usbinfo->ctrl = usb->ctrl;
> + usbinfo->port = usb->port;
> +
> + if (usb->busid)
> + usbinfo->busid = libxl__strdup(NOGC, usb->busid);
> + else if (usb->hostbus > 0 && usb->hostaddr > 0)
> + usbinfo->busid = usb_busaddr_to_busid(gc, usb->hostbus,
> usb->hostaddr);
Similar to my libxl_device_usbctrl_getinfo() comment -- the purpose of
this function is to specify a minimal amount of information, and have
the library return and look up everything else. Since libxl_device_usb
doesn't have a devid, the "key" for this should probably be <ctrl,port>.
Which means, you are only allowed to read usb->{ctrl,port}; everything
else you have to look up.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |