From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH V4 3/7] libxl: add pvusb API Date: Mon, 15 Jun 2015 15:17:51 +0100 Message-ID: References: <1433906441-3280-1-git-send-email-cyliu@suse.com> <1433906441-3280-4-git-send-email-cyliu@suse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1433906441-3280-4-git-send-email-cyliu@suse.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: Chunyan Liu Cc: Wei Liu , Ian Campbell , Ian Jackson , "xen-devel@lists.xen.org" , Jim Fehlig , Simon Cao List-Id: xen-devel@lists.xenproject.org On Wed, Jun 10, 2015 at 4:20 AM, Chunyan Liu wrote: > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index 23f27d4..4561e1b 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -541,6 +541,29 @@ libxl_device_pci = Struct("device_pci", [ > ("seize", bool), > ]) > > +libxl_usb_protocol = Enumeration("usb_protocol", [ > + (0, "AUTO"), > + (1, "PV"), > + (2, "QEMU"), > + ]) > + > +libxl_device_usbctrl = Struct("device_usbctrl", [ > + ("protocol", libxl_usb_protocol), > + ("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), > + ("busid", string), > + ("hostbus", integer), > + ("hostaddr", integer), > + ]) Ian / Ian / Wei / Jim: Question about the design of the interface here. The way that most systems in Linux specify particular USB devices is with the bus:addr format. It's the output you get when you run tools like lsusb, for example, and it's the interface that qemu (and thus KVM) uses when talking about host devices to assign. bus:addr might look like "002:006". But the bus:addr is a "public api" for Linux; internally, it has a more structured format, which contains more about the USB topology. Chunyan is calling this "busid". An example is something like this: "2-3.1.1:1.0". Internally, pvusb needs "busid" in order to find the right sysfs files. qemu, on the other hand, does not take busid; so the devicemodel / HVM implementation of USB would need bus:addr internally. Converting bus:addr to busid involves looping through all devices and seeing which one matches, so it's inefficient to do it every time we need to do some operation. (See libxl_pvusb.c:usb_busaddr_to_busid().) Converting busid to bus:addr is simpler: you read the bus and addr keys from sysfs using the busid. I think for a public UI (i.e., xl), bus:addr is really the only option; I'm pretty sure that's the main interface libvirt provides as well. At the libxl level, we basically have 3 options: 1. Have the libxl layer accept bus:addr. For the pvusb code, translate bus:addr to busid and place it in an internal structure. 2. Have the libxl layer accept busid. For the devicemodel code, 2a. ...translate busid to bus:addr and place it in an internal structure. 2b. ...translate it to bus:addr when it's used (i.e., when speaking over qmp to qemu). 3. Have the libxl layer accept both busid and bus:addr. Translate as necessary and store in the libxl_device_usb struct. For #2, I think we need to provide a helper function to convert bus:addr into busid, since bus:addr is the interface most toolstacks will want to expose to their users. Chunyan's initial submission had #2 (without a or b, since the devicemodel code doesn't exist yet). I suggested #1, and she pushed back by compromising (#3). The advantage of #3 internally is that the functions can do the translation once (if necessary), and can then pass around the public libxl_device_usb struct as-is without needing any extra parameters or a separate libxl_device_usb_internal. The disadvantage, I think, is that from an interface perspective, it's fairly pointless to have both. busid doesn't really give you any better or more control than the other, and it's not any more convenient for the user (in fact it's less convenient because it's more difficult to find). I *think* 2b wouldn't be terrible from a performance point of view, since (I think) you'd probably only have to do the translation once before calling the qmp command. I still prefer #1; but if people thought #3 or #2b would be preferable, I could live with that. Any thoughts? -George