Re: [Xen-devel] [PATCH V5 3/7] libxl: add pvusb API

["Chun Yan Liu" <cyliu@xxxxxxxx>]

>>> On 8/7/2015 at 01:21 AM, in message <55C39796.8000500@xxxxxxxxxx>, George
Dunlap <george.dunlap@xxxxxxxxxx> wrote: 
> On 08/06/2015 04:11 AM, Chun Yan Liu wrote: 
> > As 4.6 goes to bug fixing stage, maybe we can pick up this thread? :-) 
> >  
> > Beside to call for your precious review comments and suggestions so that we 
> >  
> can 
> > make progress, I also want to confirm about the previous discussed two TODO 
> > things: 
> > 1) use UDEV name rule to specify usb device uniquely even across reboot.  
> That 
> >     got consensus. Next thing is exposing that name to some sysfs entry,  
> right? 
>  
> So just to be clear, my understanding of the plan was that we try to fix 
> up the current patch series and check it in once the 4.7 window opens; 
> and that having the utility library to convert other names (including 
> this udev-style naming) into something libxl can use would be a separate 
> series. 

Got it.

>  
> I wasn't necessarily expecting you to work on it (since it wasn't your 
> idea), but if you're keen, I'm sure we'd be grateful. :-) 
>  
> > 2) use libusb instead of reading sysfs by ourselves. As George mentioned,  
> using 
> >     libusb is not simpler than reading sysfs; and if UDEV name is stored to 
> >  
> some 
> >     sysfs entry for us to retrieve, then we still need reading sysfs  
> things. Could we 
> >     get to a final decision? 
> > If these are settled down, I can update related code. 
>  
> I don't think that libusb would be particularly useful for the current 
> pvusb code, since 
> 1. It's already Linux-specific, 
> 2. We need to mess around with sysfs anyway. 
>  
> The same thing can't be said of the HVM path: I think it fairly likely 
> that the emulated pass-through will Just Work (or nearly so) on BSDs 
> (assuming that qemu itself works on the BSDs). 
>  
> I think it we write our utility library for converting 
> vendorid:productid[:serialno], bus-port, &c, then it might make sense to 
> use libusb if it makes it more portable. 
>  
> Regarding the code: 
>  
> Things are looking pretty close.  A couple of comments in-line: 
>  
> >>>> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c   
> >>>> index 93bb41e..9869a21 100644   
> >>>> --- a/tools/libxl/libxl_device.c   
> >>>> +++ b/tools/libxl/libxl_device.c   
> >>>> @@ -676,6 +676,10 @@ void libxl__devices_destroy(libxl__egc *egc,    
> >>>> libxl__devices_remove_state *drs)   
> >>>>                  aodev->action = LIBXL__DEVICE_ACTION_REMOVE;   
> >>>>                  aodev->dev = dev;   
> >>>>                  aodev->force = drs->force;   
> >>>> +                if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB) {   
> >>>> +                    libxl__initiate_device_usbctrl_remove(egc, aodev);  
> >>>>  
> >>>> +                    continue;   
> >>>> +                } 
>  
> I take it the reason we need to special-case this is that we need to go 
> through and un-assign all of the devices inside the controller first? 

Yes.

>  
> At some point we probably want to generalize this a bit, so that usb 
> controllers and vscsi controllers look the same (rather than both being 
> special-cased). 
>  
> But since this is internal, maybe we can wait for that design until we 
> actually have both types available. 
>  
> >>>> +static int   
> >>>> +libxl__device_usb_assigned_list(libxl__gc *gc,   
> >>>> +                                libxl_device_usb **list, int *num)   
> >>>> +{   
> >>>> +    char **domlist;   
> >>>> +    unsigned int nd = 0, i, j;   
> >>>> +    char *be_path;   
> >>>> +    libxl_device_usb *usb;   
> >>>> +   
> >>>> +    *list = NULL;   
> >>>> +    *num = 0;   
> >>>> +   
> >>>> +    domlist = libxl__xs_directory(gc, XBT_NULL, "/local/domain", &nd);  
> >>>>  
> >>>> +    be_path = GCSPRINTF("/local/domain/%d/backend/vusb",    
> >>>> LIBXL_TOOLSTACK_DOMID);   
>  
> Hmm, so this had made me realize that I don't think we're doing quite 
> the right thing for non-dom0 backends. 
>  
> First of all, things are a bit "interesting" for driver domain backends, 
> because the "namespace" of hostbus.hostaddr depends on the backend of 
> the virtual controller.  Which wouldn't be particularly interesting, 
> *except* that because the USB space is so dynamic, you normally have to 
> query the devices to find the hostbus.hostaddr; and you can't do any 
> queries from dom0 at the moment (except, for example, by ssh'ing into 
> the other vm).  So to even assign a hostbus.hostaddr device you have to 
> somehow learn, out-of-band, what those numbers are within the domain. 
>  
> Secondly, if the backend is in another domain, then all the stuff re 
> assigning a usb device to usbback can't be done by libxl in the 
> toolstack domain either.  Which means I'm pretty sure this stuff will 
> fail at the moment for USB driver domains trying to assign a 
> non-existent hostbus.hostaddr to the (possibly non-existent) dom0 usbback. 
>  
> A couple of ways forward: 
>  
> 1. Delete backend_domid and backend_name; add them back later when we 
> decide to implement proper USB driver domain support 
>  
> 2. Leave backend_domid and backend_name, but return an error if they are 
> not 0 and NULL respectively. 
>  
> 3. If backend_domid != TOOLSTACK_DOMID, then do the xenstore stuff in 
> libxl, but assume that someone else has figured out what the proper 
> hostbus.hostaddr are for the backend domain, and also assigned that 
> device to the usbback inside that domain. 
>  
> 4. Invent some protocol for talking to the backend, allowing you to 
> query assignable devices and assign devices to usbback in that domain 
> from the toolstack domain. 
>  
> #4 is probably more than we want to do at this point; and #1 I think 
> will cause us some hassle down the road if we ever *do* want to 
> implement usb driver domains. 
>  
> I would personally probably go for #2, unless we have some intention of 
> at least smoke-testing #3; but I can certainly see the advantages of 
> implementing #3 even if we don't end up testing it. 
>  
>  
> >>>> +int libxl_device_usb_getinfo(libxl_ctx *ctx, libxl_device_usb *usb,   
> >>>> +                             libxl_usbinfo *usbinfo)   
> >>>> +{   
> >>>> +    GC_INIT(ctx);   
> >>>> +    char *filename;   
> >>>> +    char *busid;   
> >>>> +    void *buf = NULL;   
> >>>> +    int buflen, rc;   
> >>>> +   
> >>>> +    usbinfo->ctrl = usb->ctrl;   
> >>>> +    usbinfo->port = usb->port;   
> >>>> +   
> >>>> +    busid = usb_busaddr_to_busid(gc, usb->hostbus, usb->hostaddr);   
>  
> If we're going to use "<ctrl,port>" for the "handle" here, then those 
> are the only fields we're allowed to *read* from the usb struct.  You 
> should do something like you do in ctrlport_to_device_usb() to convert 
> <ctrl,port> to busid.  (And I suppose then have hostbus and hostaddr in 
> the usbinfo struct as well.) 

OK. I'll adjust the codes.
Currently this is only used to print info. Current codes prints usbctrl info 
first,
then gets usb list from usbctrl, then calls this function, then print usb info.
I'll adjust that: put conversion from <ctrl,port> to usb device, and get info at
the same function. Don't need to get usb list and then get info in 2 steps.

> >>>>     
> >>>> +libxl_usb_protocol = Enumeration("usb_protocol", [   
> >>>> +    (0, "AUTO"),   
> >>>> +    (1, "PV"),   
> >>>> +    (2, "QEMU"),   
> >>>> +    ])   
> >>>> +   
> >>>> +libxl_device_usbctrl = Struct("device_usbctrl", [   
> >>>> +    ("protocol", libxl_usb_protocol),   
>  
> Still chewing on the name here.  Is "datapath" more descriptive? 
>  
> Oh, wait -- actually, now that we have the usbctrl/usb dichotomy, we 
> could in theory use "type" here to be "PV / IOEMU" (or perhaps

Currently in xl user interface, it is exposed to user as 'type', so maybe
just keep the same to use 'type' here. I'll update then. Naming it as 'protocol'
before is just to avoid confusing with usb device type.

> "usbctrltype", as in libxl_device_nic), and then use "devtype" in 
> libxl_device_usb to specify "hostdev", "tablet", &c. 
>  
> >>>> +    ("devid", libxl_devid),   
> >>>> +    ("version", integer),   
> >>>> +    ("ports", integer),   
> >>>> +    ("backend_domid", libxl_domid),   
> >>>> +    ("backend_domname", string),   
> >>>> +   ])   
> >>>> +   
> >>>> +libxl_device_usb = Struct("device_usb", [   
> >>>> +    ("ctrl", libxl_devid),   
> >>>> +    ("port", integer),   
> >>>> +    ("hostbus",   integer),   
> >>>> +    ("hostaddr",  integer),   
> >>>> +    ])   
>  
> I think we do want to plan for the future here by doing something like this: 
>  
> libxl_device_usb = Struct("device_usb", [ 
>     ("ctrl", libxl_devid), 
>     ("port", integer), 
>     ("u", KeyedUnion(None, libxl_device_usb_type, "devtype", 
>                       [("hostdev", Struct(None, [ 
>                              ("hostbus",   integer), 
>                              ("hostaddr",  integer) ])) 
>                        ])) 
>  ]) 
>  

Yes, that's the future look. For pvusb, currenlty with kernel pvusb driver, the
devtype is not really necessary. But I can add 'devtype' if it is preferred now.

Thanks very much!
-Chunyan

> That way we can add in more device types supported by qemu later.  (And 
> with juergen adding pvusb support to qemu, they might even be 
> appropriate for more than just the IOEMU path. :-) 


>  
> Thanks Chunyan, and sorry for the delay! 
>  
>  -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