From: "Chun Yan Liu" <cyliu@suse.com>
To: 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, 10 Sep 2015 23:42:07 -0600 [thread overview]
Message-ID: <55F2F64F02000066000508CA@relay2.provo.novell.com> (raw)
In-Reply-To: <1441721852.24450.120.camel@citrix.com>
>>> On 9/8/2015 at 10:17 PM, in message <1441721852.24450.120.camel@citrix.com>,
Ian Campbell <ian.campbell@citrix.com> 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?
>
> > + * USB devices through pvusb.
> > + *
> > + * With this functionality, one can add/remove USB controllers to/from
> > + * guest, and attach/detach USB devices to/from USB controllers. To add
> > + * USB controllers and USB devices, one can either adding USB controllers
> > + * first and then attaching USB devices to some USB controller, or adding
> > + * USB devices to guest directly, it will automatically create a USB
> > + * controller for USB devices to attach. To remove USB controllers or USB
> > + * devices, one can either remove USB devices under USB controller one by
> > + * one and then remove USB controller, or remove USB controller directly,
> > + * it will remove all USB devices under it automatically.
>
> I think this API documentation belongs alongside the API declarations (i.e
> the prototypes) rather than hidden away next to the feature flag.
>
> > + *
> > + */
> > +#define LIBXL_HAVE_PVUSB 1
> > +
> > +/*
> > * LIBXL_HAVE_BUILDINFO_HVM_VENDOR_DEVICE indicates that the
> > * libxl_vendor_device field is present in the hvm sections of
> > * libxl_domain_build_info. This field tells libxl which
> > @@ -1389,6 +1406,54 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t
> > domid, libxl_device_disk *disk,
> > const libxl_asyncop_how *ao_how)
> > LIBXL_EXTERNAL_CALLERS_ONLY;
> >
> > +/* USB Controllers*/
> >
> [....]
>
> Seem fine.
>
> > +
> > +/* USB Devices */
> > +int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_usb
> *usb,
> > + const libxl_asyncop_how *ao_how)
> > + LIBXL_EXTERNAL_CALLERS_ONLY;
> > +
> > +int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid,
> libxl_device_usb *usb,
> > + const libxl_asyncop_how *ao_how)
> > + LIBXL_EXTERNAL_CALLERS_ONLY;
> > +
> > +libxl_device_usb *
> > +libxl_device_usb_list(libxl_ctx *ctx, uint32_t domid, int *num);
> > +
> > +libxl_device_usb *
> > +libxl_device_usb_list_per_usbctrl(libxl_ctx *ctx, uint32_t domid,
> > + libxl_devid usbctrl, int *num);
>
> I'd probably say "..._for_usbctrl" or "..._by_usbctrl", but that's just
> nitpicking.
>
> > +
> > +void libxl_device_usb_list_free(libxl_device_usb *list, int nr);
> > +
> > +int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid,
> > + libxl_device_usb *usb,
> > + libxl_usbinfo *usbinfo);
> > +
> > /* Network Interfaces */
> > int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid,
> > libxl_device_nic *nic,
> > const libxl_asyncop_how *ao_how)
> [...]
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index ef346e7..ef10484 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -594,6 +594,37 @@ libxl_device_rdm = Struct("device_rdm", [
> > ("policy", libxl_rdm_reserve_policy),
> > ])
> >
> > +libxl_usbctrl_type = Enumeration("usbctrl_type", [
> > + (0, "AUTO"),
>
> What are the proposed semantics of using LIBXL_USBCTRL_TYPE_AUTO?
>
> > + (1, "PV"),
> > + (2, "QEMU"),
>
> Is "QEMU" what we want here, as opposed to, say, "EMU" (similar to NICs)?
>
> 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.
>
> > + ])
> > +
> > +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).
>
> Do these fields have any particular size requirements arising from e.g. the
> USB spec or from possible dom0 implementations?
>
> If they have a well defined fixed size from a USB spec then maybe we could
> use the appropriate fixed size types?
Didn't see the size limitation. In Linux kernel code, busnum and devnum (here
'hostbus, hostaddr') are both 'int' type. And idProduct and idVendor are 'u16'.
- Chunyan
>
> > + ])),
> > + ])
> > +
> > libxl_device_dtdev = Struct("device_dtdev", [
> > ("path", string),
> > ])
> > @@ -626,6 +657,8 @@ libxl_domain_config = Struct("domain_config", [
> > ("pcidevs", Array(libxl_device_pci, "num_pcidevs")),
> > ("rdms", Array(libxl_device_rdm, "num_rdms")),
> > ("dtdevs", Array(libxl_device_dtdev, "num_dtdevs")),
> > + ("usbctrls", Array(libxl_device_usbctrl, "num_usbctrls")),
> > + ("usbs", Array(libxl_device_usb, "num_usbs")),
> > ("vfbs", Array(libxl_device_vfb, "num_vfbs")),
> > ("vkbs", Array(libxl_device_vkb, "num_vkbs")),
> > ("vtpms", Array(libxl_device_vtpm, "num_vtpms")),
> > @@ -674,6 +707,32 @@ libxl_vtpminfo = Struct("vtpminfo", [
> > ("uuid", libxl_uuid),
> > ], dir=DIR_OUT)
> >
> > +libxl_usbctrlinfo = Struct("usbctrlinfo", [
> > + ("type", libxl_usbctrl_type),
> > + ("devid", libxl_devid),
> > + ("version", integer),
> > + ("ports", integer),
> > + ("backend", string),
> > + ("backend_id", uint32),
> > + ("frontend", string),
> > + ("frontend_id", uint32),
> > + ("state", integer),
> > + ("evtch", integer),
> > + ("ref_urb", integer),
> > + ("ref_conn", integer),
> > + ], dir=DIR_OUT)
> > +
> > +libxl_usbinfo = Struct("usbinfo", [
> > + ("ctrl", libxl_devid),
> > + ("port", integer),
> > + ("busnum", integer),
> > + ("devnum", integer),
> > + ("idVendor", integer),
> > + ("idProduct", integer),
>
> I think id* are 16 bits? uint16 might be better then.
>
> > + ("prod", string),
> > + ("manuf", string),
> > + ], dir=DIR_OUT)
> > +
> > libxl_vcpuinfo = Struct("vcpuinfo", [
> > ("vcpuid", uint32),
> > ("cpu", uint32),
>
>
>
next prev parent reply other threads:[~2015-09-11 5:42 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
2015-09-29 17:19 ` Wei Liu
2015-09-17 8:20 ` Chun Yan Liu
2015-09-11 5:42 ` Chun Yan Liu [this message]
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=55F2F64F02000066000508CA@relay2.provo.novell.com \
--to=cyliu@suse.com \
--cc=Ian.Jackson@eu.citrix.com \
--cc=JFEHLIG@suse.com \
--cc=JGross@suse.com \
--cc=caobosimon@gmail.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.