Re: [PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest

[George Dunlap <george.dunlap@eu.citrix.com>]

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 <vincent.hanquez@eu.citrix.com>
>> + * Author Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Probably not true?

Haha -- oops...

>
>> +/*
>> + * /libxl/<domid>/usb/<devid>/{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