From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap 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 Message-ID: <51643FED.8070503@eu.citrix.com> References: <1365440740-14012-1-git-send-email-george.dunlap@eu.citrix.com> <20836.8767.759691.699623@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20836.8767.759691.699623@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: Roger Pau Monne , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org 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