From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest Date: Wed, 24 Apr 2013 13:48:50 +0100 Message-ID: <5177D4B2.5040904@eu.citrix.com> References: <1366387166-21197-1-git-send-email-george.dunlap@eu.citrix.com> <1366804561.20256.296.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1366804561.20256.296.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: "sstanisi@cbnco.com" , Roger Pau Monne , Ian Jackson , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 24/04/13 12:56, Ian Campbell wrote: > On Fri, 2013-04-19 at 16:59 +0100, George Dunlap wrote: >> This patch exposes a generic interface which can be expanded in the >> future to implement USB for PVUSB, qemu, and stubdoms. It can also be >> extended to include other types of USB other than host USB (for example, >> tablets, mice, or keyboards). >> >> For each device removed or added, one of two protocols is available: >> * PVUSB >> * qemu (DEVICEMODEL) >> >> The caller can additionally specify "AUTO", in which case the library will >> try to determine the best protocol automatically. >> >> At the moment, the only protocol implemented is DEVICEMODEL, and the only >> device type impelmented is HOSTDEV. >> >> This uses the qmp functionality, and is thus only available for >> qemu-xen, not qemu-traditional. >> >> History: >> v6: >> - Return a proper error code if libxl__xs_mkdir fails >> - Make casts cuddly >> - Add a space after switch statements >> v5: >> - fix erroneous use of NOGC in qmp functions >> - Don't check return of libxl__sprintf in libxl_create.c >> - Check return values of new xs actions in libxl_create.c >> - Use updated template for xenstore transactions, do checks >> - use xs_read_checked >> - Fixed if (x) spacing to match coding style >> - use GCSFPRINTF >> - use libxl__calloc >> - Fix comment for LIBXL_HAVE_USB >> - use idl-generated *_from_string() and *_to_string() functions >> >> Signed-off-by: George Dunlap >> CC: Ian Jackson >> CC: Roger Pau Monne >> CC: sstanisi@cbnco.com >> --- >> tools/libxl/Makefile | 2 +- >> tools/libxl/libxl.h | 37 +++ >> tools/libxl/libxl_create.c | 13 +- >> tools/libxl/libxl_internal.h | 18 ++ >> tools/libxl/libxl_qmp.c | 65 +++++ >> tools/libxl/libxl_types.idl | 22 ++ >> tools/libxl/libxl_usb.c | 526 ++++++++++++++++++++++++++++++++++++++++ >> tools/ocaml/libs/xl/genwrap.py | 1 + > I've only commented on the interface bits (libxl.h, libxl_types.idl) > here. I'll go over the implementation next. > >> 8 files changed, 682 insertions(+), 2 deletions(-) >> create mode 100644 tools/libxl/libxl_usb.c >> >> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h >> index 4922313..b6edd15 100644 >> --- a/tools/libxl/libxl.h >> +++ b/tools/libxl/libxl.h >> @@ -75,6 +75,12 @@ >> #define LIBXL_HAVE_FIRMWARE_PASSTHROUGH 1 >> >> /* >> + * LIBXL_HAVE_USB indicates the functions for doing hot-plug of >> + * USB devices. >> + */ >> +#define LIBXL_HAVE_USB 1 > LIBXL_HAVE_DEVICE_USB would be consistent with the struct/interface > naming. Not that I expect this to be ambiguous I guess. Might as well make it consistent, as it's pretty easy. :-) > >> + >> +/* >> * libxl ABI compatibility >> * >> * The only guarantee which libxl makes regarding ABI compatibility >> @@ -735,6 +741,37 @@ 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 >> + * >> + * For each device removed or added, one of these protocols is available: >> + * - PV (i.e., PVUSB) >> + * - DEVICEMODEL (i.e, qemu) >> + * >> + * PV is available for either PV or HVM domains. DEVICEMODEL is only >> + * available for HVM domains. The caller can additionally specify >> + * "AUTO", in which case the library will try to determine the best >> + * protocol automatically. >> + * >> + * At the moment, the only protocol implemented is DEVICEMODEL, and the only >> + * device type impelmented is HOSTDEV. > implemented > > If PV isn't implemented I think we should leave it out of the API for > now, when it is implemented it will need a LIBXL_HAVE_DEVICE_USB_TYPE_PV > or whatever anyway. If we return -ENOTIMP or -ENOSYS from usb_add, would that be sufficient? >> + * >> + * This uses the qmp functionality, and is thus only available for >> + * qemu-xen, not qemu-traditional. >> + */ >> +#define LIBXL_DEVICE_USB_BACKEND_DEFAULT (-1) >> +int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid, >> + libxl_device_usb *dev, >> + const libxl_asyncop_how *ao_how) >> + LIBXL_EXTERNAL_CALLERS_ONLY; >> +int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid, >> + libxl_device_usb *dev, >> + const libxl_asyncop_how *ao_how) >> + LIBXL_EXTERNAL_CALLERS_ONLY; > No _destroy or _getinfo? > > _getinfo might be optional if there isn't any interesting info, but > _destroy is a must IMHO. I'm not exactly sure what the difference at this point would be between remove and destroy. > >> +libxl_device_usb *libxl_device_usb_list(libxl_ctx *ctx, uint32_t domid, >> + int *num) >> + LIBXL_EXTERNAL_CALLERS_ONLY; >> + >> /* 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 fcb1ecd..3c6a709 100644 >> --- a/tools/libxl/libxl_types.idl >> +++ b/tools/libxl/libxl_types.idl >> @@ -408,6 +408,28 @@ libxl_device_vtpm = Struct("device_vtpm", [ >> ("uuid", libxl_uuid), >> ]) >> >> +libxl_device_usb_protocol = Enumeration("usb_protocol", [ >> + (0, "AUTO"), >> + (1, "PV"), >> + (2, "DEVICEMODEL"), >> + ]) >> + >> +libxl_device_usb_type = Enumeration("device_usb_type", [ >> + (1, "HOSTDEV"), >> + ]) >> + >> +libxl_device_usb = Struct("device_usb", [ >> + ("protocol", libxl_device_usb_protocol, >> + {'init_val': 'LIBXL_USB_PROTOCOL_AUTO'}), >> + ("backend_domid", libxl_domid, >> + {'init_val': 'LIBXL_DEVICE_USB_BACKEND_DEFAULT'}), > I think now that ef496b81f0336f09968a318e7f81151dd4f5a0cc has gone in we > should have a backend_domname (and appropriate handling) for new > devices. Ack. > I don't think you need to set a default for a libxl_domid, the implicit > default is 0 and if we wanted to be explicit we should do it on the > libxl_domid type so it is consistent for all devices. Likewise I don't > think you need LIBXL_DEVICE_USB_BACKEND_DEFAULT == -1 and the handling > to make that 0 internally -- just make the default 0 and let the user > change if they like (this is how the other devices work). I think my idea was that someday you may want to say, "Have backends default to driver domain [foo]." In which case, you'd want to be able to specify the difference between "connect to the default" and "connect to domain 0". But maybe the whole "default" thing is too high-level for libxl, and I should just make the caller set the actual domain (and deal with defaults itself). -George