RFC: Proposed libxl USB hot-plug interface

RFC: Proposed libxl USB hot-plug interface

[George Dunlap]

OK, based on the feedback, what about an interface like the following?

The idea would be that long-term, AUTO would always do PV for PV VMs, 
and would somehow decide whether to do HVM or PV for HVM domains.

The "type" and the "union" fields are designed to allow the interface to 
be extended to include other types of devices, such as adding emulated 
tablets, mice, keyboards, usb sticks, &c.

"list" should return all available USB devices including the "handle" 
that can be used to remove a device.

Thoughts?

  -George

#define LIBXL_DEVICE_HOST_USB_ANY (-1)
int libxl_usb_add(libxl_ctx *ctx, uint32_t domid,
                   libxl_device_usb *dev,
                   const libxl_asyncop_how *ao_how)
                   LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_usb_del(libxl_ctx *ctx, uint32_t domid,
                   uint64_t handle,
                   const libxl_asyncop_how *ao_how)
                   LIBXL_EXTERNAL_CALLERS_ONLY;
int libxl_usb_list(libxl_ctx *ctx, uint32_t domid,
                    libxl_device_usb **dev,
                    const libxl_asyncop_how *ao_how)
                    LIBXL_EXTERNAL_CALLERS_ONLY;

struct libxl_device_usb {
     uint16_t protocol; /* AUTO, PV, HVM */
     uint16_t type; /* HOST; later can be emulated devices like tablet, 
disk, &c */
     uint32_t backend_domain_id; /* For PVUSB */
     uint64_t handle;  /* OUT: Unique (per domain) handle that must be 
used to remove a device */
     int
     union {
         struct {
             int hostbus, hostaddr, vendorid, productid;
         } host;
     };
};

Re: RFC: Proposed libxl USB hot-plug interface

[Ian Jackson]

George Dunlap writes ("RFC: Proposed libxl USB hot-plug interface"):
> OK, based on the feedback, what about an interface like the following?

This looks plausible.

> The idea would be that long-term, AUTO would always do PV for PV VMs, 
> and would somehow decide whether to do HVM or PV for HVM domains.

Right.

> The "type" and the "union" fields are designed to allow the interface to 
> be extended to include other types of devices, such as adding emulated 
> tablets, mice, keyboards, usb sticks, &c.

Right.

> "list" should return all available USB devices including the "handle" 
> that can be used to remove a device.

> struct libxl_device_usb {
>      uint16_t protocol; /* AUTO, PV, HVM */
>      uint16_t type; /* HOST; later can be emulated devices like tablet, 
> disk, &c */
>      uint32_t backend_domain_id; /* For PVUSB */

I don't see why the backend domain is necessarily invalid for hvm
domains.  In the stub-dm, you're going to have both an emulated
interface (at the guest-dm interface) and a PV one (at the dm-backend
interface).  (Not that this will necessarily be implemented right
away!)

>      uint64_t handle;  /* OUT: Unique (per domain) handle that must be 
> used to remove a device */

I'm not sure mixing in and out parameters in the single struct is a
good idea.

>      int
>      union {
>          struct {
>              int hostbus, hostaddr, vendorid, productid;
>          } host;

This needs some more documentation.  Why is it a union ?  What are its
other options, etc. ?

I guess you can set the values to -1 to mean "wildcard" ?

Ian.

Re: RFC: Proposed libxl USB hot-plug interface

[George Dunlap]

On 03/04/13 16:46, Ian Jackson wrote:
> George Dunlap writes ("RFC: Proposed libxl USB hot-plug interface"):
>> OK, based on the feedback, what about an interface like the following?
> This looks plausible.
>
>> The idea would be that long-term, AUTO would always do PV for PV VMs,
>> and would somehow decide whether to do HVM or PV for HVM domains.
> Right.
>
>> The "type" and the "union" fields are designed to allow the interface to
>> be extended to include other types of devices, such as adding emulated
>> tablets, mice, keyboards, usb sticks, &c.
> Right.
>
>> "list" should return all available USB devices including the "handle"
>> that can be used to remove a device.
>> struct libxl_device_usb {
>>       uint16_t protocol; /* AUTO, PV, HVM */
>>       uint16_t type; /* HOST; later can be emulated devices like tablet,
>> disk, &c */
>>       uint32_t backend_domain_id; /* For PVUSB */
> I don't see why the backend domain is necessarily invalid for hvm
> domains.  In the stub-dm, you're going to have both an emulated
> interface (at the guest-dm interface) and a PV one (at the dm-backend
> interface).  (Not that this will necessarily be implemented right
> away!)

Hmm -- I hadn't thought of using qemu-usb in a stubdom.  But in any 
case, I think the comment is still valid -- the backend_domain_id is 
being used for the PVUSB part of the "connection".  (And if an HVM 
domain has a usbfront drivers, it can of course use this field as well.)

>
>>       uint64_t handle;  /* OUT: Unique (per domain) handle that must be
>> used to remove a device */
> I'm not sure mixing in and out parameters in the single struct is a
> good idea.

Well it's particularly interesting I think to have it here for the 
"usb-list" function (in which case the entire structure is written by 
the library).  We could I suppose have usb-add just put it in the return 
value instead of modifying the structure in-place.
>>       int
>>       union {
>>           struct {
>>               int hostbus, hostaddr, vendorid, productid;
>>           } host;
> This needs some more documentation.  Why is it a union ?  What are its
> other options, etc. ?

This was just meant to be a brief description to talk about -- 
definitely not the final version. :-)

Initially this would be a union because I had envisioned something like 
this:

union {
     struct { blah blah } host;
     struct { blah blah } wacom-tablet;
     struct { blah blah blah } disk;
};

Then it's the same functions to add/remove/list an emulated tablet, or 
an emulated USB disk.

> I guess you can set the values to -1 to mean "wildcard" ?

or LIBXL_USB_DEVICE_HOST_ANY, which is #defined to -1. :-)

  -George

Re: RFC: Proposed libxl USB hot-plug interface

[Ian Jackson]

George Dunlap writes ("Re: RFC: Proposed libxl USB hot-plug interface"):
> On 03/04/13 16:46, Ian Jackson wrote:
> > I don't see why the backend domain is necessarily invalid for hvm
> > domains.  In the stub-dm, you're going to have both an emulated
> > interface (at the guest-dm interface) and a PV one (at the dm-backend
> > interface).  (Not that this will necessarily be implemented right
> > away!)
> 
> Hmm -- I hadn't thought of using qemu-usb in a stubdom.  But in any 
> case, I think the comment is still valid -- the backend_domain_id is 
> being used for the PVUSB part of the "connection".  (And if an HVM 
> domain has a usbfront drivers, it can of course use this field as well.)

Right.  I think though that the comment is misleading since it appears
to be referring to the protocol field, not some kind of other pv
connection in the innards.

> > I'm not sure mixing in and out parameters in the single struct is a
> > good idea.
> 
> Well it's particularly interesting I think to have it here for the 
> "usb-list" function (in which case the entire structure is written by 
> the library).

Ah yes, I see.

>  We could I suppose have usb-add just put it in the return 
> value instead of modifying the structure in-place.

Maybe that would be better.  I think in-place modifying of individual
structure fields is not really a good idea for the libxl api.  These
structs should be thought of as representations of types, not grab
bags of arguments.

> Initially this would be a union because I had envisioned something like 
> this:
> 
> union {
>      struct { blah blah } host;
>      struct { blah blah } wacom-tablet;
>      struct { blah blah blah } disk;
> };

Right, that makes sense.

> Then it's the same functions to add/remove/list an emulated tablet, or 
> an emulated USB disk.
> 
> > I guess you can set the values to -1 to mean "wildcard" ?
> 
> or LIBXL_USB_DEVICE_HOST_ANY, which is #defined to -1. :-)

Right.

Ian.

Re: RFC: Proposed libxl USB hot-plug interface

[Ian Campbell]

On Wed, 2013-04-03 at 16:11 +0100, George Dunlap wrote:
> OK, based on the feedback, what about an interface like the following?
> 
> The idea would be that long-term, AUTO would always do PV for PV VMs, 
> and would somehow decide whether to do HVM or PV for HVM domains.
> 
> The "type" and the "union" fields are designed to allow the interface to 
> be extended to include other types of devices, such as adding emulated 
> tablets, mice, keyboards, usb sticks, &c.
> 
> "list" should return all available USB devices including the "handle" 
> that can be used to remove a device.
> 
> Thoughts?
> 
>   -George
> 
> #define LIBXL_DEVICE_HOST_USB_ANY (-1)
> int libxl_usb_add(libxl_ctx *ctx, uint32_t domid,
>                    libxl_device_usb *dev,
>                    const libxl_asyncop_how *ao_how)
>                    LIBXL_EXTERNAL_CALLERS_ONLY;
> int libxl_usb_del(libxl_ctx *ctx, uint32_t domid,
>                    uint64_t handle,
>                    const libxl_asyncop_how *ao_how)
>                    LIBXL_EXTERNAL_CALLERS_ONLY;
> int libxl_usb_list(libxl_ctx *ctx, uint32_t domid,
>                     libxl_device_usb **dev,
>                     const libxl_asyncop_how *ao_how)
>                     LIBXL_EXTERNAL_CALLERS_ONLY;
> 
> struct libxl_device_usb {

I assume you will actually do this via the libxl IDL when implementing?

>      uint16_t protocol; /* AUTO, PV, HVM */

Should be an enum I think.

>      uint16_t type; /* HOST; later can be emulated devices like tablet, 
> disk, &c */

And IIUC so should this.

>      uint32_t backend_domain_id; /* For PVUSB */

All other libxl_device_* use "backend_domid"

>      uint64_t handle;  /* OUT: Unique (per domain) handle that must be 
> used to remove a device */

This seem analogous to other device's libxl_devid?

>      int

Is this the key for the following union? If so then it should be an
appropriate enum.

>      union {
>          struct {
>              int hostbus, hostaddr, vendorid, productid;
>          } host;
>      };
> };
>