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

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

>>> On 6/23/2015 at 07:29 PM, in message <558942FF.5060702@xxxxxxxxxxxxx>, 
>>> George
Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote: 
> On 06/23/2015 11:18 AM, Chun Yan Liu wrote: 
>>>>> On 6/16/2015 at 01:47 AM, in message <557F0FA7.2060402@xxxxxxxxxxxxx>, 
>>>>> George 
> > Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:  
> >>> +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? 
>  
> I think you misunderstood what I was saying. 
>  
> There are three cases I think we want to consider: 
>  
> 1. The caller knows both the controller and the port number they want to 
> assign it to. 
>  
> 2. The caller knows what controller they want to assign it to, but they 
> want libxl to choose the port number automatically. 
>  
> 3. The caller wants libxl to choose both the controller and the port 
> automatically. 
>   3a. A controller already exists with a free port 
>   3b. No controllers with free ports exist. 
>  
> Your code handles #1 properly -- if both controller and port number are 
> set, it will try to add the USB device to that specific controller and 
> port number. 
>  
> Your code also handles #3a properly -- if the controller is not set, it 
> will automatically choose a controller and a port number.  It also 
> handles #3b properly -- if there are no controllers, or if the 
> controllers have no ports available, then it will create a new one and 
> assign it to port 1. 
>  
> Your code does not, however, handle the second case -- where the user 
> knows they want to plug it into a specific controller (for instance, the 
> PVUSB one rather than the emulated one), but don't want to keep track of 
> which ports are available. 
>  
> Since ports start as 1, then '0' is an invalid number for a port (just 
> as -1 is an invalid id for a controller).  I was suggesting that you 
> could use '0' as a magic number meaning, "Please choose a port for me." 

Oh, I see. Thanks for explanation. This IS what we didn't cover, currently only
support either specifying both controller and port, or both not. If user 
specifies
controller and port, then we will use that controller and port, if it's not 
available,
we just report error, but won't choose a default available one for it. Choosing
a port or creating a new controller only happens when use doesn't specify
controller and port.

I think case 2 is very useful. I will update code to cover.

>  
> However, after our discussion here about providing "helpers" to 
> translate stuff for the libxl interface, if the other maintainers will 
> like the "automatically create a controller" functionality at all; or if 
> they would rather we (again) provide helper functions so that the 
> toolstack can do this work. 
>  
> >>> +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. 
>  
> Hmm -- you know, you're right.  For vif, vfb, and vkb it seems to use 
> devid (which is libxl specific, but indexed per vm); disk uses vdev 
> (which is indexed per vm); but pci pass-through uses BDF, which is based 
> on the host.  (And that's what my HVM series from last year did as well.) 
>  
> BTW, to remove a device over qmp, you need the name you gave the device 
> when you assigned it.  But at least as of a year ago, there was no way 
> to query qemu to see which devices you had assigned; so you have to 
> store that information away in xenstore somewhere anyway.  So from a 
> qemu perspective, having a devid or guest <ctrl,port> mapping isn't a 
> big deal.

OK. If qemu side has no problem, I'll change the interface to be <ctrl, port>.

Thanks,
Chunyan

 >  
> Also, from a UI perspective: it should be easy for xl to translate host 
> bus.addr into guest <ctrl,port>, so I don't think that's as big a deal. 
>  
> The main reason I have for wanting to do it at the guest level is so 
> that we can use the same interface for things which have no host handle 
> -- i.e., plugging or unplugging a virtual tablet, or maybe a USB disk. 
>  
>  -George 
>  
>  
>  


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel