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>]

On Wed, 2013-04-24 at 14:32 +0100, George Dunlap wrote:
> 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?

No, for the external structure, but called only internally in libxl.

>From libxl_internal.h:

/*
 * For each aggregate type which can be used as an input we provide:
 *
 * int libxl__<type>_setdefault(gc, <type> *p):
 *
 *     Idempotently sets any members of "p" which is currently set to
 *     a special value indicating that the defaults should be used
 *     (per libxl_<type>_init) to a specific value.
 *
 *     All libxl API functions are expected to have arranged for this
 *     to be called before using any values within these structures.
 */

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

Yet you look it up in the two exact places that you use it...

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

This is normal libxl behaviour, implemented via the setdefault function.

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

Yes, please just nuke it.

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

OK. BTW I saw some others that I didn't comment on, so you may want to
grep around a bit.

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

Ah, right.

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

It should just be a case of 
        ("usbdevs", Array(libxl_device_usb, "num_usbdevs")),
in the idl and
    for (i = 0; i < d_config->num_usbdevs; i++)
        libxl__device_usb_add(gc, domid, &d_config->usbdevs[i], 1);
in e.g. domcreate_attach_pci (which would then be better named
domcreate_attach_sync_devices or something).

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

OK.

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

Ah, OK then.

Ian.


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