Re: [PATCH V4 3/7] libxl: add pvusb API

["Chun Yan Liu" <cyliu@suse.com>]

>>> On 6/16/2015 at 01:47 AM, in message <557F0FA7.2060402@eu.citrix.com>, George
Dunlap <george.dunlap@eu.citrix.com> 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@suse.com> 
> > Signed-off-by: Simon Cao <caobosimon@gmail.com> 
> >  
> > --- 
> > 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@lists.xen.org 
> http://lists.xen.org/xen-devel 
>  
>