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

[George Dunlap <george.dunlap@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