From: George Dunlap <george.dunlap@citrix.com>
To: Chun Yan Liu <cyliu@suse.com>,
Ian Campbell <ian.campbell@citrix.com>,
xen-devel@lists.xen.org
Cc: Juergen Gross <JGross@suse.com>,
wei.liu2@citrix.com, george.dunlap@eu.citrix.com,
Ian.Jackson@eu.citrix.com, Jim Fehlig <JFEHLIG@suse.com>,
Simon Cao <caobosimon@gmail.com>
Subject: Re: [PATCH V6 3/7] libxl: add pvusb API
Date: Thu, 17 Sep 2015 10:54:23 +0100 [thread overview]
Message-ID: <55FA8DCF.5070405@citrix.com> (raw)
In-Reply-To: <55FB04340200006600050E6D@relay2.provo.novell.com>
On 09/17/2015 09:19 AM, Chun Yan Liu wrote:
>
>
>>>> On 9/9/2015 at 12:52 AM, in message <55EF1244.107@citrix.com>, George Dunlap
> <george.dunlap@citrix.com> 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
next prev parent reply other threads:[~2015-09-17 9:54 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-10 10:35 [PATCH V6 0/7] xen pvusb toolstack work Chunyan Liu
2015-08-10 10:35 ` [PATCH V6 1/7] libxl: export some functions for pvusb use Chunyan Liu
2015-08-11 11:26 ` Wei Liu
2015-08-10 10:35 ` [PATCH V6 2/7] libxl_read_file_contents: add new entry to read sysfs file Chunyan Liu
2015-08-11 11:26 ` Wei Liu
2015-08-12 2:37 ` Chun Yan Liu
2015-08-13 9:11 ` Wei Liu
2015-08-10 10:35 ` [PATCH V6 3/7] libxl: add pvusb API Chunyan Liu
2015-08-11 11:27 ` Wei Liu
2015-08-12 2:24 ` Chun Yan Liu
2015-08-13 9:09 ` Wei Liu
2015-08-14 1:49 ` Chun Yan Liu
2015-08-18 2:31 ` Chun Yan Liu
2015-08-31 6:10 ` Chun Yan Liu
2015-09-08 14:17 ` Ian Campbell
2015-09-08 16:52 ` George Dunlap
2015-09-09 7:38 ` Chun Yan Liu
2015-09-17 8:19 ` Chun Yan Liu
2015-09-17 9:54 ` George Dunlap [this message]
2015-09-29 17:19 ` Wei Liu
2015-09-17 8:20 ` Chun Yan Liu
2015-09-11 5:42 ` Chun Yan Liu
2015-09-11 13:26 ` Ian Campbell
2015-09-11 13:55 ` Juergen Gross
2015-09-11 14:09 ` Ian Campbell
2015-09-11 14:18 ` Juergen Gross
2015-09-11 14:41 ` Ian Campbell
2015-09-11 15:42 ` Ian Jackson
2015-09-14 3:48 ` Juergen Gross
2015-09-14 10:36 ` George Dunlap
2015-09-14 10:53 ` Juergen Gross
2015-09-14 11:12 ` Ian Jackson
2015-09-14 11:23 ` Juergen Gross
2015-09-14 14:03 ` George Dunlap
2015-09-17 8:24 ` Chun Yan Liu
2015-09-15 8:14 ` Chun Yan Liu
2015-08-10 10:35 ` [PATCH V6 4/7] libxl: add libxl_device_usb_assignable_list API Chunyan Liu
2015-08-10 10:35 ` [PATCH V6 5/7] xl: add pvusb commands Chunyan Liu
2015-08-10 10:35 ` [PATCH V6 6/7] xl: add usb-assignable-list command Chunyan Liu
2015-08-10 10:35 ` [PATCH V6 7/7] domcreate: support pvusb in configuration file Chunyan Liu
2015-08-11 11:27 ` Wei Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55FA8DCF.5070405@citrix.com \
--to=george.dunlap@citrix.com \
--cc=Ian.Jackson@eu.citrix.com \
--cc=JFEHLIG@suse.com \
--cc=JGross@suse.com \
--cc=caobosimon@gmail.com \
--cc=cyliu@suse.com \
--cc=george.dunlap@eu.citrix.com \
--cc=ian.campbell@citrix.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.