From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH V4 3/7] libxl: add pvusb API Date: Tue, 23 Jun 2015 12:29:03 +0100 Message-ID: <558942FF.5060702@eu.citrix.com> References: <1433906441-3280-1-git-send-email-cyliu@suse.com> <1433906441-3280-4-git-send-email-cyliu@suse.com> <557F0FA7.2060402@eu.citrix.com> <5589A2FB02000066000DB285@relay2.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5589A2FB02000066000DB285@relay2.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Chun Yan Liu , xen-devel@lists.xen.org Cc: Ian.Jackson@eu.citrix.com, Simon Cao , wei.liu2@citrix.com, ian.campbell@citrix.com List-Id: xen-devel@lists.xenproject.org On 06/23/2015 11:18 AM, Chun Yan Liu wrote: >>>> On 6/16/2015 at 01:47 AM, in message <557F0FA7.2060402@eu.citrix.com>, George > Dunlap 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 >> . Removing also by hostbus:hostaddr seems like useful >> functionality, so it's probably not bad to keep it; but the >> should be the main functionality. > > Do you think is better? That means in qemu emulated way, > user also need to know the info of the USB device. In the past, > for usb-add or usb-delete, 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 mapping isn't a big deal. Also, from a UI perspective: it should be easy for xl to translate host bus.addr into guest , 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