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

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

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)
>>>>                           LIBXL_EXTERNAL_CALLERS_ONLY;
>>>>
>>>> +/*
>>>> + * 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.
>>>                    implemented
>>>
>>> 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.
>>>> + */
>>>> +#define LIBXL_DEVICE_USB_BACKEND_DEFAULT (-1)
>>>> +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).

Ack.

  -George