[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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




>>> On 9/8/2015 at 10:17 PM, in message <1441721852.24450.120.camel@xxxxxxxxxx>,
Ian Campbell <ian.campbell@xxxxxxxxxx> 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), 
>  
>  
>  


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.