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 14:32:53 +0100 Message-ID: <5177DF05.50907@eu.citrix.com> References: <1366387166-21197-1-git-send-email-george.dunlap@eu.citrix.com> <1366807135.20256.328.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: <1366807135.20256.328.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: Anthony Perard , "sstanisi@cbnco.com" , Roger Pau Monne , Ian Jackson , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 24/04/13 13:38, Ian Campbell wrote: > I didn't see an implementation of libxl__device_usb_setdefault? You mean, for the internal structure? > > On Fri, 2013-04-19 at 16:59 +0100, George Dunlap wrote: >> xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/vm", dom_path), vm_path, strlen(vm_path)); >> rc = libxl__domain_rename(gc, *domid, 0, info->name, t); >> if (rc) >> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h >> index 3ba3a21..d2e00fa 100644 >> --- a/tools/libxl/libxl_internal.h >> +++ b/tools/libxl/libxl_internal.h >> @@ -1412,6 +1412,24 @@ _hidden int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename); >> /* Set dirty bitmap logging status */ >> _hidden int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool enable); >> _hidden int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid, const libxl_device_disk *disk); >> +/* Same as normal, but "translated" */ > You can use the IDL to do internal stuff too -- see > tools/libxl/libxl_types_internal.idl > > What does "translated" mean? Which fields and how? Might it be better to > include a pointer to the original libxl_device_usb instead of > duplicating some/all of these fields? I think one of the biggest thing was avoiding having to look up the stubdomain in a bunch of different functions -- that's the dm_domid. There's also the translation of "AUTO" protocol into PV or HVM, and (originally) the translation of BACKEND_DEFAULT into the actual backend domain. If we were willing to have the library change the protocol in the structure passed to it, then a pointer might work as well. > >> +typedef struct libxl__device_usb { >> + libxl_usb_protocol protocol; >> + libxl_domid target_domid; >> + libxl_domid backend_domid; >> + libxl_domid dm_domid; > The only use of this I see is: > + usbdev->dm_domid = libxl_get_stubdom_id(ctx, domid); > ... > + if (usbdev->dm_domid != 0) { > > twice, both times all in the same function so you could just use a local > variable. A big part of that is because we don't support stubdomains yet. When we do, there will be more complicated cases to consider. > If you remove this and pass target_domid as a parameter to various > functions (which is the convention in libxl) then the need for this > weird internal/external representation goes away. There's also the resolution of protocol from ANY into PV or HVM; but if we're willing to change the structure passed in by the caller, then yes, we can get rid of this struct for now and consider re-introducing it if/when we actually need it. > >> + libxl_device_usb_type type; >> + union { >> + struct { >> + int hostbus; >> + int hostaddr; >> + } hostdev; >> + } u; >> +} libxl__device_usb; >> +_hidden int libxl__qmp_usb_add(libxl__gc *gc, int domid, >> + libxl__device_usb *dev); >> +_hidden int libxl__qmp_usb_remove(libxl__gc *gc, int domid, >> + libxl__device_usb *dev); >> /* close and free the QMP handler */ >> _hidden void libxl__qmp_close(libxl__qmp_handler *qmp); >> /* remove the socket file, if the file has already been removed, >> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c >> index 644d2c0..9ad3e59 100644 >> --- a/tools/libxl/libxl_qmp.c >> +++ b/tools/libxl/libxl_qmp.c > I'd like Anthony's feedback on the QMP bits (CCd) > >> @@ -42,6 +42,7 @@ >> >> #define QMP_RECEIVE_BUFFER_SIZE 4096 >> #define PCI_PT_QDEV_ID "pci-pt-%02x_%02x.%01x" >> +#define HOST_USB_QDEV_ID "usb-hostdev-%04x.%04x" >> >> typedef int (*qmp_callback_t)(libxl__qmp_handler *qmp, >> const libxl__json_object *tree, >> @@ -929,6 +930,70 @@ int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid, >> } >> } >> >> +static int libxl__qmp_usb_hostdev_add(libxl__gc *gc, int domid, >> + libxl__device_usb *dev) >> +{ >> + libxl__json_object *args = NULL; >> + char *id; >> + >> + id = GCSPRINTF(HOST_USB_QDEV_ID, >> + (uint16_t)dev->u.hostdev.hostbus, >> + (uint16_t)dev->u.hostdev.hostaddr); > You could make these uint16 in the IDL if you wanted to restrict it like > that. Even without that is the cast really necessary though, given the > %x format string? > > If you do use uint16_t then you probably want to use PRIx16 too. This actually a vestigial artifact from the fact that you could originally specify ANY in the field, which was encoded as -1. I'll just take out now. >> + >> + qmp_parameters_add_string(gc, &args, "driver", "usb-host"); >> + QMP_PARAMETERS_SPRINTF(&args, "hostbus", "0x%x", dev->u.hostdev.hostbus); >> + QMP_PARAMETERS_SPRINTF(&args, "hostaddr", "0x%x", dev->u.hostdev.hostaddr); >> + >> + qmp_parameters_add_string(gc, &args, "id", id); >> + >> + return qmp_run_command(gc, domid, "device_add", args, NULL, NULL); >> +} >> + >> +int libxl__qmp_usb_add(libxl__gc *gc, int domid, libxl__device_usb *usbdev) >> +{ >> + int rc; >> + switch (usbdev->type) { >> + case LIBXL_DEVICE_USB_TYPE_HOSTDEV: > Will this function ever handle anything other than HOSTDEV? AIUI the > only other potential type is PV which won't come anywhere near here? You're mixing up protocol and device type. PV will only ever handle HOSTDEV types, but qmp allows a wide range of virtual devices: tablets, mice, keyboards, thumb drives, &c. I'd like at some point for all USB devices specified in the config file to be able to be listed and hot-plugged / hot-unplugged. > > (same comments on the remove bits) > >> 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'}), >> + ("u", KeyedUnion(None, libxl_device_usb_type, "type", >> + [("hostdev", Struct(None, [ >> + ("hostbus", integer), >> + ("hostaddr", integer) ])) >> + ])) >> + ]) >> + >> libxl_domain_config = Struct("domain_config", [ >> ("c_info", libxl_domain_create_info), >> ("b_info", libxl_domain_build_info), > Do you not want to add a list of USB devices to the domain_build_info? I don't think I can get that functionality working for 4.3. It's definitely on my plans for the future, though. > >> diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c >> new file mode 100644 >> index 0000000..c320487 >> --- /dev/null >> +++ b/tools/libxl/libxl_usb.c >> @@ -0,0 +1,526 @@ >> +/* >> + * Copyright (C) 2009 Citrix Ltd. >> + * Author Vincent Hanquez >> + * Author Stefano Stabellini > Probably not true? Haha -- oops... > >> +/* >> + * /libxl//usb//{type, protocol, backend, [typeinfo]} >> + */ >> +#define USB_INFO_PATH "%s/usb" > This duplicates something you added in libxl_create.c I think. I nearly > suggested moving that code into this file as a libxl__domain_usb_init or > something like that. The other option would be to make a function like libxl__xs_libxl_path(), but that seemed a bit heavy-handed to me. I guess libxl__domain_usb_init sounds good. > >> +#define USB_HOSTDEV_ID "hostdev-%04x-%04x" >> + >> +/* >> + * Just use plain numbers for now. Replace these with strings at some point. >> + */ >> +static char * hostdev_xs_path(libxl__gc *gc, uint32_t domid, >> + libxl__device_usb *usbdev) >> +{ >> + return GCSPRINTF(USB_INFO_PATH "/%s", >> + libxl__xs_libxl_path(gc, domid), >> + GCSPRINTF(USB_HOSTDEV_ID, >> + (uint16_t)usbdev->u.hostdev.hostbus, >> + (uint16_t)usbdev->u.hostdev.hostaddr)); >> +} >> + >> +static void hostdev_xs_append_params(libxl__gc *gc, libxl__device_usb *usbdev, >> + flexarray_t *a) >> +{ >> + flexarray_append_pair(a, "hostbus", >> + GCSPRINTF("%d", >> + usbdev->u.hostdev.hostbus)); >> + flexarray_append_pair(a, "hostaddr", >> + GCSPRINTF("%d", >> + usbdev->u.hostdev.hostaddr)); > Are the second and third lines > 80 chars in total? You mean, "Why did you break the GSPRINTF into two lines?" Yes, because on one line it's > 80 chars. > > [...] >> +static int usb_add_xenstore(libxl__gc *gc, uint32_t domid, >> + libxl__device_usb *usbdev) >> +{ > [...] >> + default: >> + LOG(ERROR, "Invalid device type: %d", usbdev->type); >> + return ERROR_FAIL; >> + } >> + >> + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Adding new usb device to xenstore"); > You use a mixture of this and the LOG(DEBUG,...) form in this patch, we > tend to prefer the latter for new code. Ack. > >> + >> + for(;;) { >> + rc = libxl__xs_transaction_start(gc, &t); >> + if (rc) goto out; >> + >> + /* Helpfully, mkdir returns 0 on failure, 1 on success */ >> + if (!libxl__xs_mkdir(gc, t, dev_path, noperm, ARRAY_SIZE(noperm))) { >> + rc = ERROR_FAIL; >> + goto out; >> + } >> + >> + /* And libxl__xs_writev *always* returns 0 no matter what */ > Strictly speaking the *current* implementation always returns 0, but if > someone changes that your code will break. But none of the other callers > check it either, so meh ;-) In fact, I already have tried changing it, and the result is that domain creation fails -- so whoever does change it will have a lot of cleaning up to do anyway. :-) (This is on my to-do list for the 4.4 dev cycle.) >> + libxl__xs_writev(gc, t, dev_path, >> + libxl__xs_kvs_of_flexarray(gc, dev, dev->count)); >> + >> + rc = libxl__xs_transaction_commit(gc, &t); >> + if (!rc) break; >> + if (rc <0) goto out; >> + } >> + >> +out: > According to libxl_internal.h you need to call > libxl__xs_transaction_abort on the error path. Good catch -- sorry I missed that. > > [...] > >> + >> +static int is_usbdev_type_hostdev_equal(libxl__device_usb *a, >> + libxl__device_usb *b) >> +{ >> + if (!memcmp(&a->u.hostdev, &b->u.hostdev, sizeof(a->u.hostdev))) > I'm not 100% sure it is permissible to compare structs for equality in > this way. What if the compiler decides to add some padding which doesn't > get initialised? > > I'm afraid this probably means you have to compare field by field. Ack. > >> + return 1; >> + else >> + return 0; >> +} >> +static void usbdev_ext_to_int(libxl__device_usb *dev_int, >> + libxl_device_usb *dev_ext) >> +{ >> + dev_int->protocol = dev_ext->protocol; >> + >> + if (dev_ext->backend_domid == LIBXL_DEVICE_USB_BACKEND_DEFAULT) >> + dev_int->backend_domid = 0; >> + else >> + dev_int->backend_domid = dev_ext->backend_domid; >> + >> + dev_int->type = dev_ext->type; >> + memcpy(&dev_int->u, &dev_ext->u, sizeof(dev_ext->u)); > I don't think you can be sure that these two things have been laid out > the same by the compiler. Ack. > > If you make dev_ext->backend_domid default to 0 then I'm not entirely > sure what the point of this internal/external distinction is. > >> +static int libxl__device_usb_add(libxl__gc *gc, uint32_t domid, >> + libxl_device_usb *dev_ext) >> +{ >> + libxl_ctx *ctx = libxl__gc_owner(gc); >> + libxl__device_usb *assigned, _usbdev, *usbdev; >> + int rc = ERROR_FAIL, num_assigned; >> + libxl_domain_type domtype = libxl__domain_type(gc, domid); >> + >> + /* Interpret incoming */ >> + usbdev = &_usbdev; >> + >> + usbdev->target_domid = domid; >> + usbdev->dm_domid = libxl_get_stubdom_id(ctx, domid); >> + >> + usbdev_ext_to_int(usbdev, dev_ext); >> + >> + if (usbdev->protocol == LIBXL_USB_PROTOCOL_AUTO) { >> + if (domtype == LIBXL_DOMAIN_TYPE_PV) { >> + usbdev->protocol = LIBXL_USB_PROTOCOL_PV; >> + } else if (domtype == LIBXL_DOMAIN_TYPE_HVM) { >> + /* FIXME: See if we can detect PV frontend */ >> + usbdev->protocol = LIBXL_USB_PROTOCOL_DEVICEMODEL; >> + } >> + } >> + >> + /* Check to make sure we're doing something that's impemented */ > "implemented" > >> + if (usbdev->protocol != LIBXL_USB_PROTOCOL_DEVICEMODEL) { >> + rc = ERROR_FAIL; >> + LOG(ERROR, "Protocol not implemented"); >> + goto out; >> + } >> + >> + if (usbdev->dm_domid != 0) { >> + rc = ERROR_FAIL; >> + LOG(ERROR, "Stubdoms not yet supported"); >> + goto out; >> + } >> + >> + /* Double-check to make sure it's not already assigned */ >> + rc = get_assigned_devices(gc, domid, &assigned, &num_assigned); >> + if (rc) { >> + LOG(ERROR, "cannot determine if device is assigned," >> + " refusing to continue"); >> + goto out; >> + } >> + if (is_usbdev_in_array(assigned, num_assigned, usbdev)) { >> + LOG(ERROR, "USB device already attached to a domain"); >> + rc = ERROR_FAIL; >> + goto out; >> + } >> + >> + /* Do the add */ >> + if (do_usb_add(gc, domid, usbdev)) >> + rc = ERROR_FAIL; >> + >> +out: >> + return rc; >> +} >> + >> +int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid, >> + libxl_device_usb *usbdev, >> + const libxl_asyncop_how *ao_how) >> +{ >> + AO_CREATE(ctx, domid, ao_how); >> + int rc; >> + rc = libxl__device_usb_add(gc, domid, usbdev); >> + libxl__ao_complete(egc, ao, rc); >> + return AO_INPROGRESS; >> +} > Please define this (and the remove) using the DEFINE_DEVICE_ADD/REMOVE > macros in libxl.c. Not least because I'm not convinced your handling of > the AO stuff is correct (or indeed present). > > This will probably require the libxl__device_usb_add/remove to change to > do the AIO stuff too -- i.e. at the end: > aodev->rc = rc; > if (rc) aodev->callback(egc, aodev); > return; These follow the template set by libxl_pci.c:libxl_device_pci_{add,remove,destroy}(), which were chosen specifically because they essentially do a null ASYNC, but interface-wise allow for real async to be added in the future. -George