Re: [xen-devel][PATCH RFC] libxl: New xl interfaces for managing USB devices

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

[-- Attachment #1.1: Type: text/plain, Size: 7675 bytes --]

On 07/03/2014 01:56 PM, Simon Cao wrote:
>
>         ---
>           docs/man/xl.pod.1           |   39
>         +++++++++++++++++++++++++++++++++++++++
>           tools/libxl/libxl.h         |   26 ++++++++++++++++++++++++++
>           tools/libxl/libxl_types.idl |   27 +++++++++++++++++++++++++++
>           tools/libxl/xl_cmdtable.c   |   33
>         +++++++++++++++++++++++++++++++++
>           4 files changed, 125 insertions(+)
>
>         diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
>         index 30bd4bf..db76ce2 100644
>         --- a/docs/man/xl.pod.1
>         +++ b/docs/man/xl.pod.1
>         @@ -1194,6 +1194,45 @@ List virtual network interfaces for a
>         domain.
>             =back
>           +=head2 USB DEVICES
>         +
>         +=over 4
>         +
>         +=item B<usb-assginable-list>
>
>
>     Nit: typo.
>
>     I'm not really sure we need this one.  The only other time we have
>     this is for PCI devices; but that's because PCI devices have an
>     extra step: they must first be seized by pciback, and then
>     assigned to the domain.
>
>     There is no equivalent (AFAIK) for USB devices to this; so this
>     would just be duplicating the functionality of lsusb.  There's no
>     "pci-list-seizable-devices" to show what devices can be passed to
>     "pci-assignable-add"; you just have to use lspci.  We should
>     follow suit here.
>
>
> I list this command here because it exists in the previous xm/xend 
> commands. Since the devices listed by "lsusb" isn't all assignable, 
> such as the USB hubs, "usb-assginable-list" will list all the 
> assignable devices in Dom0 that can be attached to guest VM in a 
> manner that is suitable to usb-attach.

OK -- well I think that's a bit incidental to the core functionality.  I 
would put off working on this until the core functionality is working; 
and then I'd put it in its own patch, so we can consider whether to 
include it separately.


>
>         +
>         +=item B<usb-controller-create> I<usb-version>
>         I<number_of_ports> [I<controller_type>]
>         +
>         +Creates a new usb virtual controller for the domain specified
>         by I<domain-id>.
>         +I<usb-version> describes the version of USB controller to
>         create, i.e., 1, 2 or 3,
>         +I<number_of_ports> describers the number of ports in the USB
>         virtual controller, and
>         +I<controller_type> describes the type of USB controller,
>         PVUSB or EMULATED.
>         +
>         +=item B<usb-controller-destroy> I<domain-id> I<controller-name>
>         +
>         +Destroies the virtual USB controller specified by
>         I<controller-name> in I<domain-id>.
>
>
>     In keeping with the other device commands, these should be
>     "usb-controller-attach" and "usb-controller-detach".  Other than
>     that, looks good.
>
> Since the controller is created from NULL, not just attaching a device 
> from Dom0 to DomU, maybe "create" and "destroy" are better ?

Yeah, I understand the logic.  But that's how we name network devices, 
for instance, which are created from nothing before being attached.  In 
any case, you're still combining the "create" and the "attach" operation 
into one.  Having "attach" mean "create and attach" isn't really that 
different from having "create" mean "create and attach".

Anyway, I'm pretty sure the maintainers will insist on attach / detach 
before accepting it (they asked me to change it when I posted it a year 
ago), so you might as well just do it now. :-)

>
>
>         +
>         +=item B<usb-attach> I<domain-id> I<hostbus.hostaddr>
>         [I<controller-name>]
>         +
>         +Attaches USB device specified by I<hostbus.hostaddr> to USB
>         controller specified by
>         +I<controller-name> in domain I<domain-id>.
>         +
>         +=item B<usb-detach> I<domain-id>  I<hostbus.hostaddr>
>         +
>         +Detaches the USB device I<hostbus.hostaddr> in domain
>         I<domain-id>.
>
>
>     We want to use the same interface to be able to add other kinds of
>     devices (tablets, mice, keyboards, maybe mass storage) in the
>     future. I think for now this should be something like
>     "hostdev:<hostbus.hostaddr>".
>
>     Other than that, looks good. 
>
> In this case, to specify a hostdev device we can use "hostdev = 
> <hostbus.hostaddr>".

OK -- well I'm sure there will be comments on the exact syntax before 
the patches are finally accepted. :-)

>
>
>         +
>         +libxl_device_usb = Struct("device_usb", [
>         +       ("controller_name", string),
>         +       ("type", libxl_usb_type),
>         +       ("bus", uint8),
>         +       ("dev", uint8),
>         +       ])
>
>
>     Is there a reason you got rid of the KeyedUnion construct?
>
> I don't know the benefit of using KeyedUnion construct. Please tell me 
> the benefit of it.

So this idl code:

libxl_device_usb = Struct("device_usb", [
         ("protocol", libxl_device_usb_protocol,
          {'init_val': 'LIBXL_USB_PROTOCOL_AUTO'}),
         ("backend_domid",    libxl_domid),
         ("backend_domname",  string),
         ("u", KeyedUnion(None, libxl_device_usb_type, "type",
                          [("hostdev", Struct(None, [
                                 ("hostbus",   integer),
                                 ("hostaddr",  integer) ]))
                           ]))
     ])

Generated this header code:

typedef struct libxl_device_usb {
     libxl_usb_protocol protocol;
     libxl_domid backend_domid;
     char * backend_domname;
     libxl_device_usb_type type;
     union {
         struct {
             int hostbus;
             int hostaddr;
         } hostdev;
     } u;
} libxl_device_usb;
void libxl_device_usb_dispose(libxl_device_usb *p);
void libxl_device_usb_init(libxl_device_usb *p);
void libxl_device_usb_init_type(libxl_device_usb *p, 
libxl_device_usb_type type);
char *libxl_device_usb_to_json(libxl_ctx *ctx, libxl_device_usb *p);

So first of all it makes a union; so that if we eventually have, say, a 
mass storage type, you won't just have to toss all the possible 
different parameters into the struct together; it will only be as long 
as the longest type.  Secondly, it tells the idl compiler that the value 
in "type" tells you what kind of union you're dealing with, so that the 
"usb_to_json" functions get build properly.  For example, the above idl 
code generates the following code snippet in 
_libxl_types.c:libxl_device_usb_gen_json():

     switch (p->type) {
     case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
         s = yajl_gen_map_open(hand);
         if (s != yajl_gen_status_ok)
             goto out;
         s = yajl_gen_string(hand, (const unsigned char *)"hostbus", 
sizeof("hostbus")-1);
         if (s != yajl_gen_status_ok)
             goto out;
         s = yajl_gen_integer(hand, p->u.hostdev.hostbus);
         if (s != yajl_gen_status_ok)
             goto out;
         s = yajl_gen_string(hand, (const unsigned char *)"hostaddr", 
sizeof("hostaddr")-1);
         if (s != yajl_gen_status_ok)
             goto out;
         s = yajl_gen_integer(hand, p->u.hostdev.hostaddr);
         if (s != yajl_gen_status_ok)
             goto out;
         s = yajl_gen_map_close(hand);
         if (s != yajl_gen_status_ok)
             goto out;
         break;
     }

So it does specific things based on what "type" is set to: if it's set 
to "hostdev" it outputs hostbus and hostaddr; if we added a mass storage 
type, and type was set to "storage", it would only output the 
information relevant to the mass storage device.

Cool, look forward to the code. :-)

  -George

[-- Attachment #1.2: Type: text/html, Size: 16349 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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