From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chun Yan Liu" Subject: Re: [PATCH V6 3/7] libxl: add pvusb API Date: Wed, 09 Sep 2015 01:38:41 -0600 Message-ID: <55F06EA1020000660005068B@relay2.provo.novell.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> <55EF1244.107@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55EF1244.107@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 , Ian Campbell , xen-devel@lists.xen.org Cc: Juergen Gross , wei.liu2@citrix.com, george.dunlap@eu.citrix.com, Ian.Jackson@eu.citrix.com, Jim Fehlig , Simon Cao List-Id: xen-devel@lists.xenproject.org >>> On 9/9/2015 at 12:52 AM, in message <55EF1244.107@citrix.com>, George Dunlap wrote: > 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." Thanks. Will clarify. > > >> +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." Better to be: by default, PV for PV guest and DM for HVM guest. Thanks, Chunyan > > > > >> + (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 > > >