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

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

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."

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.

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