Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

["Chun Yan Liu" <cyliu@xxxxxxxx>]

>>> On 9/9/2015 at 12:52 AM, in message <55EF1244.107@xxxxxxxxxx>, George Dunlap
<george.dunlap@xxxxxxxxxx> 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 
>  
>  
>  


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel