|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V4 3/7] libxl: add pvusb API
>>> On 6/16/2015 at 01:47 AM, in message <557F0FA7.2060402@xxxxxxxxxxxxx>,
>>> George
Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:
> 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.
Agree. Will update.
>
> [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?
To be strict, it should be libxl_devid. But libxl_devid is actually 'int'. I
see most
current APIs also uses 'int'. Like libxl_devid_to_device_vtpm API, devid also
uses
'int'. Anyway, I can update :)
>
>
> > +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?
That is libxl__device_usb_set_default_usbctrl's work, it will try to find
an available port for USB device. If there is no available port, then it will
create a new USB controller and by default uses its first port.
>
> It seems like it would be better to have the code treat port 0 as
> "automatically choose a port for me".
Here we set port=1 because port number is starting from 1. Like, if there
are 4 ports, port number will be 1, 2, 3, 4 (not 0,1 ,2, 3). Since xend, it
behaves like this. I think that's what pvusb driver needs. Juergen, right?
>
> (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.
Do you think <ctrl,port> is better? That means in qemu emulated way,
user also need to know the <ctrl, port> info of the USB device. In the past,
for usb-add or usb-delete, <ctrl, port> info is hidden to user, it used
bus.addr or verndorid.deviceid.
>
> > +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.
Agree. Will update.
Thanks,
Chunyan
>
> -George
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |