From: George Dunlap <george.dunlap@citrix.com>
To: Ian Campbell <ian.campbell@citrix.com>,
Chunyan Liu <cyliu@suse.com>,
xen-devel@lists.xen.org
Cc: jgross@suse.com, wei.liu2@citrix.com,
george.dunlap@eu.citrix.com, Ian.Jackson@eu.citrix.com,
jfehlig@suse.com, Simon Cao <caobosimon@gmail.com>
Subject: Re: [PATCH V6 3/7] libxl: add pvusb API
Date: Tue, 8 Sep 2015 17:52:20 +0100 [thread overview]
Message-ID: <55EF1244.107@citrix.com> (raw)
In-Reply-To: <1441721852.24450.120.camel@citrix.com>
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.
>
>> + ])
>> +
>> +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
next prev parent reply other threads:[~2015-09-08 16:52 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 [this message]
2015-09-09 7:38 ` Chun Yan Liu
2015-09-17 8:19 ` Chun Yan Liu
2015-09-17 9:54 ` George Dunlap
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=55EF1244.107@citrix.com \
--to=george.dunlap@citrix.com \
--cc=Ian.Jackson@eu.citrix.com \
--cc=caobosimon@gmail.com \
--cc=cyliu@suse.com \
--cc=george.dunlap@eu.citrix.com \
--cc=ian.campbell@citrix.com \
--cc=jfehlig@suse.com \
--cc=jgross@suse.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.