From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chun Yan Liu" Subject: Re: [PATCH RFC V2 1/5] libxl: add pvusb definitions Date: Wed, 04 Mar 2015 23:07:19 -0700 Message-ID: <54F8631702000066000A61EE@relay2.provo.novell.com> References: <1421656131-19366-1-git-send-email-cyliu@suse.com> <1421656131-19366-2-git-send-email-cyliu@suse.com> <54F5EC4E.6020607@eu.citrix.com> <54F7329502000066000A56B0@relay2.provo.novell.com> <54F71992.5080306@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <54F71992.5080306@eu.citrix.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap , xen-devel@lists.xen.org Cc: ian.jackson@citrix.com, caobosimon@gmail.com, ian.campbell@citrix.com, lars.kurth@citrix.com List-Id: xen-devel@lists.xenproject.org >>> On 3/4/2015 at 10:41 PM, in message <54F71992.5080306@eu.citrix.com>, George Dunlap wrote: > On 03/04/2015 08:28 AM, Chun Yan Liu wrote: > > > > > >>>> On 3/4/2015 at 01:15 AM, in message <54F5EC4E.6020607@eu.citrix.com>, George > > Dunlap wrote: > >> On 01/19/2015 08:28 AM, Chunyan Liu wrote: > >>> To attach a usb device, a virtual usb controller should be created first. > >>> This patch defines usbctrl and usbdevice related structs. > >>> > >>> Signed-off-by: Chunyan Liu > >>> Signed-off-by: Simon Cao > >> > >> Chunyan, thanks for picking up this work! > >> > >> A couple of things. First, I think that having the IDL stuff separate > >> from the patches where they are used actually makes it *harder* to > >> review, because you can't easily go to the code where it's used and see > >> what's actually happening. > >> > >> I think that the IDL stuff used in patch 3 should be in patch 3; and the > >> domain creation IDL stuff should be included in patch 5. > > > > Tha's OK. I'll update. > > Great, thanks. > > >>> +libxl_device_usbctrl = Struct("device_usbctrl", [ > >>> + ("name", string), > >>> + ("type", libxl_usbctrl_type), > >>> + ("backend_domid", libxl_domid), > >>> + ("backend_domname", string), > >>> + ("devid", libxl_devid), > >>> + ("usb_version", uint8), > >>> + ("num_ports", uint8), > >>> + ]) > >>> + > >>> +libxl_device_usb = Struct("device_usb", [ > >>> + ("ctrl", integer), > >>> + ("port", integer), > >>> + ("intf", string), > >>> + ("u", KeyedUnion(None, libxl_usb_type, "type", > >>> + [("hostdev", Struct(None, [ > >>> + ("hostbus", integer), > >>> + ("hostaddr", integer) ])) > >>> + ])) > >>> + ]) > >> > >> So "intf" here is wrong. To begin with, it's information specific to > >> the "hostdev" type; so it would go under the "type" keyed union under > >> "hostdev". > >> > >> Secondly, this requires people to figure out that their media reader has > >> an intf of "1-2.1.1:1.0". I don't think that's a good idea, for two > >> reasons: one, it just seems like a really hard interface to use. I > >> couldn't find any straightforward tools to map USB devices onto intf; > > > > Right. One can only use usb-assignable-list for a fast look. That > > follows the old xend toolstack way. > > > >> tools like "lsusb" instead give you a bus:addr combination. Secondly, > >> it's inconsistent with qemu -- which means we'd either have to have two > >> different ways of specifying the device, or we'd need to translate from > >> "intf" back into bus:addr > > > > You are right. Using bus:addr could unify qemu and pvusb. I also thought > > about that. Only concern is it's different from old xend toolstack usage. > > If that doesn't affect, we can update to use bus:addr, no problem. > > Right, I see. > > I think overall that the bus:addr is a better interface; it's also > what's exposed by lsusb, qemu, and libvirt, AFAICT. So I definitely > think that at the libxl level that's what we should be using. Thanks. Got it. Will update. > > We're already defining a new config file format, right? So domain > configs that used pvusb with xend won't work with this patch series > anyway, correct? > > If we're not going to make something 100% compatible, I don't see any > particular value in making it 25% compatible. :-) > > >> I think the right thing to do here is to take "intf" out of this struct, > >> and to translate "bus:addr" into intf internally. > >> > >> It looks like the values qemu and lsusb use can be found in "busnum" and > >> "devnum" in the sysfs files. You've already got code to scan those > >> devices; you just need to add a bit of code to find which of those > >> corresponds to a given "hostbus:hostaddr" combination. > >> > >>> + > >>> libxl_device_vtpm = Struct("device_vtpm", [ > >>> ("backend_domid", libxl_domid), > >>> ("backend_domname", string), > >>> @@ -547,6 +578,7 @@ libxl_domain_config = Struct("domain_config", [ > >>> ("disks", Array(libxl_device_disk, "num_disks")), > >>> ("nics", Array(libxl_device_nic, "num_nics")), > >>> ("pcidevs", Array(libxl_device_pci, "num_pcidevs")), > >>> + ("usbs", Array(libxl_device_usb, "num_usbs")), > >> > >> Any reason you don't make it possible to specify usb controllers as well? > > > > For qemu emulated HVM usb device, usb controller is created by qemu, no > > way to specify it (?) Also I wonder if specifying usb controller is > necessary, > > seems it won't affect without usb controller here. Correct me if I'm wrong. > > If it's necessary, we can add it. > > On the contrary, there is a way to specify it. :-) See > "usbversion=[n]". At the moment we specify the usb device on the qemu > command-line; but I'm pretty sure that we can use qmp to hot-plug a USB > controller just like we can use it to hot-plug a USB device. > > qemu will automatically hot-plug a USB controller as necessary, similar > to your "automatically create a pvusb controller" functionality for > pvusb add. But there may be circumstances where we want to specify a > controller (for instance, if you want to be able to control what kind of > controller it is). > > My long-term vision is to have the USB stuff unified. Instead of > passing in USB devices on the qemu command-line, as we do now, we'd plug > them in after starting qemu but before letting the VM run (similar to > the way we do things with PCI pass-through). Thanks for explanation. So I'll add usb controllers to domain_config, and for that libxl_device_usbctrl should be finally able to represent pvusb controller and qemu emulated controller. Currently, it's defined to represent a pvusb controller, in future, when HVM qemu emulated USB is implemented, it could be extended. - Chunyan > > In any case, I agree with your design decision that you shouldn't *have* > to specify a controller. However, I think you should be able to specify > a controller if you wish. > > Adding that functionality to libxl should be pretty straightforward. > Coming up with a sensible way to specify it in the xl config file would > be a bit more work, and I would be open to the argument that it > shouldn't be a requirement for this series to go in. > > > -George > >