From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH V6 3/7] libxl: add pvusb API Date: Tue, 8 Sep 2015 17:52:20 +0100 Message-ID: <55EF1244.107@citrix.com> References: <1439202928-24813-1-git-send-email-cyliu@suse.com> <1439202928-24813-4-git-send-email-cyliu@suse.com> <1441721852.24450.120.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1441721852.24450.120.camel@citrix.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: Ian Campbell , Chunyan Liu , xen-devel@lists.xen.org Cc: jgross@suse.com, wei.liu2@citrix.com, george.dunlap@eu.citrix.com, Ian.Jackson@eu.citrix.com, jfehlig@suse.com, Simon Cao List-Id: xen-devel@lists.xenproject.org On 09/08/2015 03:17 PM, Ian Campbell wrote: > On Mon, 2015-08-10 at 18:35 +0800, Chunyan Liu wrote: > > Sorry for the delay, between 4.6 freeze crunch, conference and vacation > I've been a bit swamped. > > I'm just going to comment on the APIs (mainly public libxl.h and .idl) in > this pass. > >> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h >> index 5f9047c..05b6331 100644 >> --- a/tools/libxl/libxl.h >> +++ b/tools/libxl/libxl.h >> @@ -123,6 +123,23 @@ >> #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1 >> >> /* >> + * LIBXL_HAVE_PVUSB indicates the functions for doing hot-plug of > > And cold-plug, no? So you should probably say something like "indicates functions for plugging in USB devices through pvusb -- both hotplug and at domain creation time." >> +libxl_usbctrl_type = Enumeration("usbctrl_type", [ >> + (0, "AUTO"), > > What are the proposed semantics of using LIBXL_USBCTRL_TYPE_AUTO? Generally "DTRT". Meaning: 1. If your domain has no devicemodel, use PV. 2. If your device has a devicemodel, and no PV drivers have peen detected, use the devicemodel. 3. If your device has a devicemodel, but PV drivers have been detected, use PV. At the moment we don't have a way to check for PV drivers, so this just collapses down to "PV for domains without a DM and DM for domains with a DM." > >> + (1, "PV"), >> + (2, "QEMU"), > > Is "QEMU" what we want here, as opposed to, say, "EMU" (similar to NICs)? I had this as "DEVICEMODEL", since what we mean is that we want the device model to provide access (and in theory in the future we may use a different device model). But "EMU" works for me too. > I think we probably don't want to go as fine grained as "XHCI" and "EHCI" > etc, do we? I see we have a version field below, is it intended that there > be some way to select between e.g. UHCI and OHCI (which IIRC are different > USB 1.0 controllers). > > Maybe these questions should all be left aside for when QMEU support is > actually added (AFAICT this field is just a placeholder)? In fact I glanced > at the code and was surprised to find nothing checking for > LIBXL_USBCTRL_TYPE at all, did I miss something? > > I think the two choices are: > > We can decide quickly and easily what the option(s) other than PV should be > here and you include it in the IDL, but you would then need to check > usbctrl->type == PV at various points, not silently treat all options as > PV. > > Or this becomes a long conversation in which case I think your best bet > would be to leave the enum with just the PV (and maybe AUTO) entries and > leave the decision on the name for the emulated option to the series which > implements that. I think the idea was to simply offer 1, 2, and 3 as options, and for the devicemodel version, choose a suitable controller (or set of controllers) for each option; similar to what usbversion= does now. > >> + ]) >> + >> +libxl_usbdev_type = Enumeration("usbdev_type", [ >> + (0, "invalid"), >> + (1, "hostdev"), >> + ]) >> + >> +libxl_device_usbctrl = Struct("device_usbctrl", [ >> + ("type", libxl_usbctrl_type), >> + ("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), >> + ("u", KeyedUnion(None, libxl_usbdev_type, "devtype", >> + [("hostdev", Struct(None, [ >> + ("hostbus", integer), >> + ("hostaddr", integer)])), >> + ("invalid", None), > > AIUI this is what was agreed to, i.e. an enum with only one real option, in > order to leave a space for new devtypes without major API overhaul. > > Please can you confirm that hostbus and hostaddr are both flat integer > namespaces (i.e. there is no structure to the bits within either, they are > just a number). I can confirm this. -George