[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

On 24/04/13 13:53, Ian Campbell wrote:
On Wed, 2013-04-24 at 13:48 +0100, George Dunlap wrote:

    * libxl ABI compatibility
    * The only guarantee which libxl makes regarding ABI compatibility
@@ -735,6 +741,37 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, 
libxl_device_disk *disk,
                          const libxl_asyncop_how *ao_how)

+ * USB
+ *
+ * For each device removed or added, one of these protocols is available:
+ * - PV (i.e., PVUSB)
+ * - DEVICEMODEL (i.e, qemu)
+ *
+ * PV is available for either PV or HVM domains.  DEVICEMODEL is only
+ * available for HVM domains.  The caller can additionally specify
+ * "AUTO", in which case the library will try to determine the best
+ * protocol automatically.
+ *
+ * At the moment, the only protocol implemented is DEVICEMODEL, and the only
+ * device type impelmented is HOSTDEV.

If PV isn't implemented I think we should leave it out of the API for
now, when it is implemented it will need a LIBXL_HAVE_DEVICE_USB_TYPE_PV
or whatever anyway.
If we return -ENOTIMP or -ENOSYS from usb_add, would that be sufficient?
How would the application know whether it should expose this
functionality to its users or not?

It'd be pretty lame for them (either the app or the end user) to have to
try it and see if it worked.

On the whole, I'm beginning to think that we should just punt the whole USB thing to 4.4 and do it properly the first time, instead of doing it piecemeal.

+ *
+ * This uses the qmp functionality, and is thus only available for
+ * qemu-xen, not qemu-traditional.
+ */
+int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid,
+                         libxl_device_usb *dev,
+                         const libxl_asyncop_how *ao_how)
+                         LIBXL_EXTERNAL_CALLERS_ONLY;
+int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid,
+                            libxl_device_usb *dev,
+                            const libxl_asyncop_how *ao_how)
+                            LIBXL_EXTERNAL_CALLERS_ONLY;
No _destroy or _getinfo?

_getinfo might be optional if there isn't any interesting info, but
_destroy is a must IMHO.
I'm not exactly sure what the difference at this point would be between
remove and destroy.
 From libxl.h:

  * libxl_device_<type>_remove(ctx, domid, device):
  *   Removes the given device from the specified domain by performing
  *   an orderly unplug with guest co-operation. This requires that the
  *   guest is running.
  *   This method is currently synchronous and therefore can block
  *   while interacting with the guest.
  * libxl_device_<type>_destroy(ctx, domid, device):
  *   Removes the given device from the specified domain without guest
  *   co-operation. It is guest specific what affect this will have on
  *   a running guest.
  *   This function does not interact with the guest and therefore
  *   cannot block on the guest.

I saw that, but AFAICT there is not and never will be a distinguishable difference.

I see that the same is true for pci devices, as there are two functions which call the same function internally. I'll do the same thing. :-)

I don't think you need to set a default for a libxl_domid, the implicit
default is 0 and if we wanted to be explicit we should do it on the
libxl_domid type so it is consistent for all devices. Likewise I don't
think you need LIBXL_DEVICE_USB_BACKEND_DEFAULT == -1 and the handling
to make that 0 internally -- just make the default 0 and let the user
change if they like (this is how the other devices work).
I think my idea was that someday you may want to say, "Have backends
default to driver domain [foo]."  In which case, you'd want to be able
to specify the difference between "connect to the default" and "connect
to domain 0".
We'll need to fix all the devices when this happens, so I think it is
better for USB to be consistent with them for now.

But maybe the whole "default" thing is too high-level for libxl, and I
should just make the caller set the actual domain (and deal with
defaults itself).
Until you support a driver domain for these devices the caller (xl)
should just leave this field alone as initilised by
libxl_device_usb_init. i.e. you should only set it if the user supplied
a backenddomid= option in the cfg (which you don't implement, so you
should never set it).



Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.