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." 
>  
> >> +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.

 
Hi, George,

I'm still confused about the expected look concerning PV/EMU type handling in
this patch series.

In earlier version, we tried to extract common things in libxl_usb.c and put
pvusb specific thing in libxl_pvusb.c, prefixed with pvusb_xxx. As you
suggested, we can leave that when EMU USB patch series added.

Now, about how to handle PV/EMU type in this patch series, I can think of 3 
ways:

1. We define the enumeration (contains PV/AUTO only, user interface only allows
'pv' or 'not specified', so we handle everything in 'pv' way without further 
check. Leave check and other adjusting things when EMU USB patch series added.

2. We check domain type and set proper type if not specified (i.e. 'pv' for PV 
guest, 'emu' for HVM guest). In add/remove function, check if type='emu', 
report  
'not supported' directly; otherwise, continue do following things. When EMU USB
patch serires added, need to extract common things and adjust the check place.

3. Same as 2, but extract common things, only in PV/EMU USB specific part, check
type, if type='emu', report 'not supported'; otherwise, do pvusb work. When 
adding EMU USB patch series, only need to add EMU USB specific things in the
type='emu' branch.

Which one is expected? Or none?

- Chunyan

>  
> >  
> >> +    ]) 
> >> + 
> >> +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 
>  
>  


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