All of lore.kernel.org
 help / color / mirror / Atom feed
From: George Dunlap <george.dunlap@eu.citrix.com>
To: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
Subject: Re: [PATCH RFC] libxl: Introduce functions to add and remove USB devices to an HVM guest
Date: Tue, 9 Apr 2013 17:21:01 +0100	[thread overview]
Message-ID: <51643FED.8070503@eu.citrix.com> (raw)
In-Reply-To: <20836.8767.759691.699623@mariner.uk.xensource.com>

On 09/04/13 15:14, Ian Jackson wrote:
> George Dunlap writes ("[Xen-devel] [PATCH RFC] libxl: Introduce functions to add and remove USB devices to an HVM guest"):
>> I'm mailing this intermediate form of v3 to get feedback before I go
>> all the way through the process of implementing the whole thing and
>> ironing out the bugs.
> Thanks.  The basic approach seems plausible.
>
>> For each device removed or added, one of three protocols is available:
>> * PVUSB
>> * qemu (DEVICEMODEL)
>> * stubdom (i.e., stubdom -> pvusb, domain -> stubdom qemu)
> You seem to be making this specifiable separately for each device,
> which doesn't seem like it could be right.
>
> I think it would be best to make the protocol (as specified in the
> libxl api, xl config files etc.), simply specify what the guest sees:
> PV or HVM.  In the case of HVM, that's the stub-dm if the guest has
> one or qemu in dom0 if not.

The dichotomy isn't PV / HVM; it's PVUSB vs qemu (or "devicemodel").  
Remember that HVM guests can have PVUSB front-ends, and thus can use the 
PVUSB protocol.

The reason for specifying STUBDOM I guess is that STUBDOM is really a 
special case, where it's really both a PVUSB  and a DEVICEMODEL; so in 
theory you could specify a backend_domid.

The question, I guess, is whether we should assume that the caller can 
be trusted to know whether the VM in question is using a stub domain or 
not.  It's certainly possible to make the interface only specify PVUSB 
or DEVICEMODEL, and to make it (someday) so that calling DEVICEMODEL for 
a stubdom will set up PVUSB for the stubdom, then tell the device model 
to make the device available via emulation all automatically.

But since we're not implementing stubdom at the moment anyway, we can 
probably just take out STUBDOM entirely and discuss whether to add a new 
protocol when we actually decide to implement it.
>> +#define LIBXL_DEVICE_HOST_USB_ANY (-1)
>> +#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;
> What are the semantics if multiple host devices match *dev ?
> How is the id returned ?

I think in general if it matches multiple devices then it should fail.  
That's what qemu will do, so at the moment that is implemented by 
default for us.

> I think dev should probably be
>    const libxl_device_usb *dev

Sure.

>> +    libxl__json_object *args = NULL;
>> +    char *id;
>> +
>> +    id = libxl__sprintf(NOGC, HOST_USB_QDEV_ID,
>> +                        (uint16_t) dev->u.hostdev.hostbus,
>> +                        (uint16_t) dev->u.hostdev.hostaddr,
>> +                        (uint16_t) dev->u.hostdev.vendorid,
>> +                        (uint16_t) dev->u.hostdev.productid);
> If hostbus and hostaddr aren't specified and we pass through multiple
> devices with the same vendor and product id, this won't yield unique
> ids.

If multiple devices match, then qemu will fail to add the device.

We could do a lot of this checking in the library itself rather than 
qemu. (In fact for PVUSB I think we're going to have to.)  But again, 
there's no way I'm going to get that coded up by Friday; getting that 
done would mean waiting until 4.4.

>> +struct enum_str {
>> +    int min,max;
>> +    const char * str[];
>> +};
>> +
>> +static char * __enum_to_str(libxl__gc *gc, struct enum_str *e, libxl_usb_protocol t)
> (Trailing whitespace, and needs wrapping.)
>
> This is a lot of enum-handling machinery.  Don't we have something
> similar already which could be pressed into service ?  If not then
> this should be in a separate file.
>
>> +struct enum_str enum_protocol = {
>> +    .min = 1,
>> +    .max = LIBXL_USB_PROTOCOL_STUBDOM,
>> +    .str = {
>> +        [LIBXL_USB_PROTOCOL_PV] = "pv",
>> +        [LIBXL_USB_PROTOCOL_DEVICEMODEL] = "dm",
>> +        [LIBXL_USB_PROTOCOL_STUBDOM] = "stubdom",
> Perhaps this should be generated by the idl compiler ?\

I'm not going to be able to make that happen by the feature freeze this 
Friday.

Perhaps just writing the raw numeric values for now?

>
>
>> +static char * create_hostdev_xenstore_entry(libxl__gc *gc, uint32_t domid, l
> ibxl_device_usb *usbdev, flexarray_t *a)
>> +{
>> +    char * path;
>> +
>> +    path = libxl__sprintf(gc, "/libxl/usb/%d/%s",
>> +                          domid,
>> +                          libxl__sprintf(gc, USB_HOSTDEV_ID,
>> +                                        (uint16_t)usbdev->u.hostdev.hostbus,
>> +                                        (uint16_t)usbdev->u.hostdev.hostaddr
> ,
>> +                                        (uint16_t)usbdev->u.hostdev.vendorid
> ,
>> +                                        (uint16_t)usbdev->u.hostdev.producti
> d));
>
> I stopped reading here because of the wrap damage I'm afraid.
> (Reproduced it by adding hard CRs where my MUA wraps it, so you can
> see what it looks like.)

Well the main purpose of this mail was to see if this could be 
implemented by Friday, or if it would need to wait until 4.4.  We've 
uncovered a couple of things which if required would mean waiting -- 
what do you think?  I'm fine with waiting -- that's why I sent the 
e-mail, so I could stop early if there was little chance of success. :-)

  -George

  reply	other threads:[~2013-04-09 16:21 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-04-08 17:05 [PATCH RFC] libxl: Introduce functions to add and remove USB devices to an HVM guest George Dunlap
2013-04-09 14:14 ` Ian Jackson
2013-04-09 16:21   ` George Dunlap [this message]
2013-04-09 16:30     ` Ian Jackson
2013-04-09 17:31       ` George Dunlap
2013-04-09 18:19         ` Ian Jackson
2013-04-10 13:49 Stefan
2013-04-10 13:55 ` George Dunlap
2013-04-10 14:07   ` jacek burghardt
2013-04-10 14:43     ` George Dunlap
2013-04-10 15:19       ` Stefan
2013-04-10 15:49         ` jacek burghardt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=51643FED.8070503@eu.citrix.com \
    --to=george.dunlap@eu.citrix.com \
    --cc=Ian.Jackson@eu.citrix.com \
    --cc=roger.pau@citrix.com \
    --cc=xen-devel@lists.xen.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.