[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API
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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |