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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 09/17/2015 09:19 AM, Chun Yan Liu wrote:
> 
> 
>>>> 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?

So there are two questions here, first WRT the code, the second WRT the
interface.

WRT the code, *normally* the first person to submit the code gets to
have it easy, and the second person has to do all the work of
refactoring.  So you would be completely within your rights to simply
submit "libxl_usb.c", and make me refactor that into libxl_pvusb.c and
whatever else (probably the qemu stuff would go in libxl_qmp.c).

Earlier I asked you as a favor to put things in libxl_pvusb.c, and you
were kind enough to do so -- so thank you.  Just having things roughly
where they might end up eventually has already been a big help.  I'll
have to move some of the code around pretty much no matter what you do.
 So I don't think it's worth making any more effort wrt the code itself.

WRT the interface -- if we do a release with PV defined, but not
EMU/DEVICEMODEL, then when we add that option, we'll have to add Yet
Another LIBL_HAS_BLAH.  I would personally like too avoid that.

As it happens, if you were to check this in now at the beginning of the
cycle, it's very likely I could get the EMU side in before the release.
 So it's *probably* OK to just write AUTO and PV.

I would personally prefer to play it safe and give all three interface
elements (AUTO, PV, and EMU/DEVICEMODEL), and return ENOTSUPP (or
whatever) for DEVICEMODEL until it's implemented.  But that's really a
policy decision for the maintianers at this point.

 -George

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