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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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?

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


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