Re: [Xen-devel] [PATCH V3 6/6] refactor codes to unify pvusb and qemu emulated usb

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Sun, Apr 19, 2015 at 4:50 AM, Chunyan Liu <cyliu@xxxxxxxx> wrote:
> Now we have pvusb implementation. To merge with future qemu emulated
> usb work, refactor codes:
>  - define 'protocol' (type) to indicate pvusb or qemu, add 'protocol' to
> usb controller and usb device structures, add 'type' to xl interface
> usb-attach|detach.
>  - extract common codes for both qemu and pvusb from libxl_pvusb.c to
> libxl_usb.c, and adjust libxl_pvusb.c codes accordingly.
>
> Signed-off-by: Chunyan Liu <cyliu@xxxxxxxx>

Thank you for doing this, Chunyan.

A couple of things.

First, I'm not really a fan of having a patch in a series go through
and rearrange things like this; it seems like it would be better to
fold these changes into patches 3-5 of this series.

Second, I've got a number of comments on the "refactoring" bit -- what
you move into libxl_usb.c and how.  But the refactoring bit isn't
really for your feature, and I'll probably have to adjust it anyway
when I actually go to add the qemu side; it seems a bit unfair and a
bit inefficient to ask you to redo the refactoring at this point.

On the other hand, I don't like the idea of checking in an interface
that I know I don't want set in stone, particularly with less than 2
months left until the code freeze.

So what about this: Take the interface-related changes from this patch
(with my comments) and move them back to the earlier patches in the
series; and then just put the minimum amount of code in those patches
to handle them.  The interface is an enum, so if there's only one
option, you should be able to assume that protocol==PV; and it should
be OK internally to assume that all controllers and devices are
actually PV.  Leave the file named "libxl_pvusb.c", but don't bother
(yet) adding a libxl_usb.c.  I'll do that when I add the HVM side of
things.

What do you think?

Further comments below...

> ---
>  tools/libxl/Makefile         |   2 +-
>  tools/libxl/libxl_internal.h |   4 +
>  tools/libxl/libxl_pvusb.c    | 169 +-------------------------------
>  tools/libxl/libxl_types.idl  |  11 +++
>  tools/libxl/libxl_usb.c      | 224 
> +++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/xl_cmdimpl.c     |  54 +++++++++--
>  tools/libxl/xl_cmdtable.c    |   4 +-
>  7 files changed, 293 insertions(+), 175 deletions(-)
>  create mode 100644 tools/libxl/libxl_usb.c
>
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index d52281f..f786fcf 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -95,7 +95,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o 
> libxl_pci.o \
>                         libxl_internal.o libxl_utils.o libxl_uuid.o \
>                         libxl_json.o libxl_aoutils.o libxl_numa.o 
> libxl_vnuma.o \
>                         libxl_save_callout.o _libxl_save_msgs_callout.o \
> -                       libxl_qmp.o libxl_event.o libxl_fork.o libxl_pvusb.o 
> $(LIBXL_OBJS-y)
> +                       libxl_qmp.o libxl_event.o libxl_fork.o libxl_pvusb.o 
> libxl_usb.o $(LIBXL_OBJS-y)
>  LIBXL_OBJS += libxl_genid.o
>  LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
>
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index f426ed8..2ee058b 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2428,6 +2428,10 @@ _hidden int libxl__device_usbctrl_add(libxl__gc *gc, 
> uint32_t domid,
>  _hidden int libxl__device_usb_add(libxl__gc *gc, uint32_t domid,
>                              libxl_device_usb *usb);
>  _hidden int libxl__device_usb_destroy_all(libxl__gc *gc, uint32_t domid);
> +_hidden bool pv_is_usb_assigned(libxl__gc *gc, libxl_device_usb *usb);
> +_hidden int pv_usb_add(libxl__gc *gc, uint32_t domid, libxl_device_usb *usb);
> +_hidden int pv_usb_remove(libxl__gc *gc, uint32_t domid, libxl_device_usb 
> *usb);
> +_hidden int pv_usb_destroy_all(libxl__gc *gc, uint32_t domid);
>
>  /* Internal function to connect a vkb device */
>  _hidden int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid,
> diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
> index 4e4975a..3dfd2bd 100644
> --- a/tools/libxl/libxl_pvusb.c
> +++ b/tools/libxl/libxl_pvusb.c
> @@ -16,8 +16,6 @@
>
>  #define USBBACK_INFO_PATH "/libxl/usbback"
>
> -#define USBHUB_CLASS_CODE 0x09
> -
>  static int libxl__device_usbctrl_setdefault(libxl__gc *gc, uint32_t domid,
>                                              libxl_device_usbctrl *usbctrl)
>  {
> @@ -338,6 +336,7 @@ int libxl_device_usbctrl_getinfo(libxl_ctx *ctx, uint32_t 
> domid,
>      usbctrlinfo->devid = usbctrl->devid;
>      usbctrlinfo->ports = usbctrl->ports;
>      usbctrlinfo->version = usbctrl->version;
> +    usbctrlinfo->protocol = usbctrl->protocol;

Simillar to my comment on patch 3/6: this should only read
usbctrl->devid.  Here you can assume protocol=PV for the time being.

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index a6db614..df56303 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -533,7 +533,15 @@ libxl_device_pci = Struct("device_pci", [
>      ("seize", bool),
>      ])
>
> +libxl_usb_protocol = Enumeration("usb_protocol", [
> +    (0, "AUTO"),
> +    (1, "PV"),
> +    (2, "DEVICEMODEL"),
> +    ], init_val = "LIBXL_USB_PROTOCOL_PV")

I think this should probably default to AUTO, and then have something
which changes AUTO to PV unconditionally (which I'll refactor later).

> +
> +
>  libxl_device_usbctrl = Struct("device_usbctrl", [
> +    ("protocol", libxl_usb_protocol),
>      ("devid", libxl_devid),
>      ("version", integer),
>      ("ports", integer),
> @@ -542,6 +550,7 @@ libxl_device_usbctrl = Struct("device_usbctrl", [
>     ])
>
>  libxl_device_usb = Struct("device_usb", [
> +    ("protocol", libxl_usb_protocol),
>      ("ctrl", libxl_devid),
>      ("port", integer),
>      ("busid", string),

I don't think we need a protocol for usb -- just for usbctrl.  I think
the protocol for usb should be defined by the controller it's using.

> @@ -626,6 +635,7 @@ libxl_vtpminfo = Struct("vtpminfo", [
>      ], dir=DIR_OUT)
>
>  libxl_usbctrlinfo = Struct("usbctrlinfo", [
> +    ("protocol", libxl_usb_protocol),
>      ("devid", libxl_devid),
>      ("version", integer),
>      ("ports", integer),
> @@ -640,6 +650,7 @@ libxl_usbctrlinfo = Struct("usbctrlinfo", [
>      ], dir=DIR_OUT)
>
>  libxl_usbinfo = Struct("usbinfo", [
> +    ("protocol", libxl_usb_protocol),
>      ("busnum", integer),
>      ("devnum", integer),
>      ("idVendor", integer),

Same here -- I don't think we need the protocol for "usbinfo" if we
have the controller id.

> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 750377f..f41ac6c 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3400,7 +3400,16 @@ int main_usbattach(int argc, char **argv)
>      }
>
>      while (argc > optind) {
> -        if (MATCH_OPTION("controller", argv[optind], oparg)) {
> +        if (MATCH_OPTION("type", argv[optind], oparg)) {
> +            if (!strcmp(oparg, "pv")) {
> +               usb.protocol = LIBXL_USB_PROTOCOL_PV;
> +            } else if (!strcmp(oparg, "qemu")) {
> +               usb.protocol = LIBXL_USB_PROTOCOL_DEVICEMODEL;
> +            } else {
> +               fprintf(stderr, "unrecognized type `%s'\n", oparg);
> +               exit(-1);
> +            }

We want to move this from main_usbattach to main_usbctrl_attach.  No
need to specify the type on detach -- libxl should be able to look
that up.

And here you want to use libxl_usb_protocol_from_string(), which is
generated automatically by the idl.


> +static const char *get_usb_protocol_string(libxl_usb_protocol type)
> +{
> +    switch (type) {
> +    case LIBXL_USB_PROTOCOL_PV:
> +        return "pv";
> +    case LIBXL_USB_PROTOCOL_DEVICEMODEL:
> +        return "qemu";
> +    default:
> +        return "invalid";
> +    }
> +}

And you don't need this, because the IDL will automatically generate
libxl_usb_protocol_to_string()...

>          if (!libxl_device_usbctrl_getinfo(ctx, domid,
>                                  &usbctrls[i], &usbctrlinfo)) {
> -            printf("%-6d %-3d %-5d %-7d %-5d %-30s\n",
> +            printf("%-6d %-6s %-3d %-5d %-7d %-5d %-30s\n",
>                      usbctrlinfo.devid,
> +                    get_usb_protocol_string(usbctrlinfo.protocol),
>                      usbctrlinfo.backend_id, usbctrlinfo.state,
>                      usbctrlinfo.version, usbctrlinfo.ports,
>                      usbctrlinfo.backend);

...which you can use here.

> @@ -3517,6 +3552,9 @@ int main_usblist(int argc, char **argv)
>      }
>
>      free(usbctrls);
> +
> +    /* TODO list qemu usb device info*/
> +

No need for this.

>      return 0;
>  }
>
> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index da30ae0..4054911 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -557,12 +557,12 @@ struct cmd_spec cmd_table[] = {
>      { "usb-attach",
>        &main_usbattach, 1, 2,
>        "Attach a USB device to a domain",
> -      "<Domain> <bus.addr> [controller=<DevId> [port=<port>]]",
> +      "<Domain> <bus.addr> [type=pv|qemu] [controller=<DevId> 
> [port=<port>]]",

I think for now you should just leave this as "pv".  Only advertise
types that are actually supported.

 -George

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