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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Wed, Jun 10, 2015 at 4:20 AM, Chunyan Liu <cyliu@xxxxxxxx> wrote:
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 23f27d4..4561e1b 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -541,6 +541,29 @@ libxl_device_pci = Struct("device_pci", [
>      ("seize", bool),
>      ])
>
> +libxl_usb_protocol = Enumeration("usb_protocol", [
> +    (0, "AUTO"),
> +    (1, "PV"),
> +    (2, "QEMU"),
> +    ])
> +
> +libxl_device_usbctrl = Struct("device_usbctrl", [
> +    ("protocol", libxl_usb_protocol),
> +    ("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),
> +    ("busid", string),
> +    ("hostbus",   integer),
> +    ("hostaddr",  integer),
> +    ])

Ian / Ian / Wei / Jim:

Question about the design of the interface here.

The way that most systems in Linux specify particular USB devices is
with the bus:addr format.  It's the output you get when you run tools
like lsusb, for example, and it's the interface that qemu (and thus
KVM) uses when talking about host devices to assign.  bus:addr might
look like "002:006".

But the bus:addr is a "public api" for Linux; internally, it has a
more structured format, which contains more about the USB topology.
Chunyan is calling this "busid".  An example is something like this:
"2-3.1.1:1.0".

Internally, pvusb needs "busid" in order to find the right sysfs
files.  qemu, on the other hand, does not take busid; so the
devicemodel / HVM implementation of USB would need bus:addr
internally.

Converting bus:addr to busid involves looping through all devices and
seeing which one matches, so it's inefficient to do it every time we
need to do some operation. (See libxl_pvusb.c:usb_busaddr_to_busid().)
 Converting busid to bus:addr is simpler: you read the bus and addr
keys from sysfs using the busid.

I think for a public UI (i.e., xl), bus:addr is really the only
option; I'm pretty sure that's the main interface libvirt provides as
well.

At the libxl level, we basically have 3 options:

1. Have the libxl layer accept bus:addr.  For the pvusb code,
translate bus:addr to busid and place it in an internal structure.

2. Have the libxl layer accept busid.  For the devicemodel code,
 2a. ...translate busid to bus:addr and place it in an internal structure.
 2b. ...translate it to bus:addr when it's used (i.e., when speaking
over qmp to qemu).

3. Have the libxl layer accept both busid and bus:addr.  Translate as
necessary and store in the libxl_device_usb struct.

For #2, I think we need to provide a helper function to convert
bus:addr into busid, since bus:addr is the interface most toolstacks
will want to expose to their users.

Chunyan's initial submission had #2 (without a or b, since the
devicemodel code doesn't exist yet).  I suggested #1, and she pushed
back by compromising (#3).

The advantage of #3 internally is that the functions can do the
translation once (if necessary), and can then pass around the public
libxl_device_usb struct as-is without needing any extra parameters or
a separate libxl_device_usb_internal.  The disadvantage, I think, is
that from an interface perspective, it's fairly pointless to have
both.  busid doesn't really give you any better or more control than
the other, and it's not any more convenient for the user (in fact it's
less convenient because it's more difficult to find).

I *think* 2b wouldn't be terrible from a performance point of view,
since (I think) you'd probably only have to do the translation once
before calling the qmp command.

I still prefer #1; but if people thought #3 or #2b would be
preferable, I could live with that.

Any thoughts?

 -George

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