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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

I didn't see an implementation of libxl__device_usb_setdefault?

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?

> +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.

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.

> +    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.

> +
> +    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?

The switch only really makes sense if there are going to be several
valid options, if its HOSTDEV or nothing then an 
        if (type != HOSTDEV)
                return ERROR_INVAL
would be preferable IMO, and I would do that in the hostdev_add function
and get rid of this wrapper

(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?

> 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@xxxxxxxxxxxxx>
> + * Author Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>

Probably not true?

> +/*
> + * /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.

> +#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?

[...]
> +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.

> +
> +    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 ;-)

> +        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.

[...]

> +
> +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.

> +        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.

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;

Ian.


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