From: George Dunlap <george.dunlap@eu.citrix.com>
To: Chun Yan Liu <cyliu@suse.com>, xen-devel@lists.xen.org
Cc: Ian.Jackson@eu.citrix.com, Simon Cao <caobosimon@gmail.com>,
wei.liu2@citrix.com, ian.campbell@citrix.com
Subject: Re: [PATCH V4 3/7] libxl: add pvusb API
Date: Tue, 23 Jun 2015 12:29:03 +0100 [thread overview]
Message-ID: <558942FF.5060702@eu.citrix.com> (raw)
In-Reply-To: <5589A2FB02000066000DB285@relay2.provo.novell.com>
On 06/23/2015 11:18 AM, Chun Yan Liu wrote:
>>>> On 6/16/2015 at 01:47 AM, in message <557F0FA7.2060402@eu.citrix.com>, George
> Dunlap <george.dunlap@eu.citrix.com> wrote:
>>> +static int libxl__device_usb_setdefault(libxl__gc *gc, uint32_t domid,
>>> + libxl_device_usb *usb,
>>> + bool update_json)
>>> +{
>>> + char *be_path, *tmp;
>>> +
>>> + if (usb->ctrl == -1) {
>>> + int ret = libxl__device_usb_set_default_usbctrl(gc, domid, usb);
>>> + /* If no existing ctrl to host this usb device, setup a new one */
>>> + if (ret) {
>>> + libxl_device_usbctrl usbctrl;
>>> + libxl_device_usbctrl_init(&usbctrl);
>>> + if (libxl_device_usbctrl_add_common(CTX, domid, &usbctrl,
>>> + 0, update_json)) {
>>> + LOG(ERROR, "Failed to create usb controller");
>>> + return ERROR_INVAL;
>>> + }
>>> + usb->ctrl = usbctrl.devid;
>>> + usb->port = 1;
>>> + libxl_device_usbctrl_dispose(&usbctrl);
>>> + }
>>> + }
>>
>> Sorry for not noticing this before -- it looks like if you set
>> usb->ctrl to -1, it will automatically choose both a controller and a
>> port number. But what if you want to specify that you want a particular
>> controller (for example, if you want to specify the PV controller rather
>> than the emulated one, or vice versa), but didn't want to have to
>> manually keep track of which ports were free?
>
> That is libxl__device_usb_set_default_usbctrl's work, it will try to find
> an available port for USB device. If there is no available port, then it will
> create a new USB controller and by default uses its first port.
>>
>> It seems like it would be better to have the code treat port 0 as
>> "automatically choose a port for me".
>
> Here we set port=1 because port number is starting from 1. Like, if there
> are 4 ports, port number will be 1, 2, 3, 4 (not 0,1 ,2, 3). Since xend, it
> behaves like this. I think that's what pvusb driver needs. Juergen, right?
I think you misunderstood what I was saying.
There are three cases I think we want to consider:
1. The caller knows both the controller and the port number they want to
assign it to.
2. The caller knows what controller they want to assign it to, but they
want libxl to choose the port number automatically.
3. The caller wants libxl to choose both the controller and the port
automatically.
3a. A controller already exists with a free port
3b. No controllers with free ports exist.
Your code handles #1 properly -- if both controller and port number are
set, it will try to add the USB device to that specific controller and
port number.
Your code also handles #3a properly -- if the controller is not set, it
will automatically choose a controller and a port number. It also
handles #3b properly -- if there are no controllers, or if the
controllers have no ports available, then it will create a new one and
assign it to port 1.
Your code does not, however, handle the second case -- where the user
knows they want to plug it into a specific controller (for instance, the
PVUSB one rather than the emulated one), but don't want to keep track of
which ports are available.
Since ports start as 1, then '0' is an invalid number for a port (just
as -1 is an invalid id for a controller). I was suggesting that you
could use '0' as a magic number meaning, "Please choose a port for me."
However, after our discussion here about providing "helpers" to
translate stuff for the libxl interface, if the other maintainers will
like the "automatically create a controller" functionality at all; or if
they would rather we (again) provide helper functions so that the
toolstack can do this work.
>>> +static int libxl__device_usb_remove(libxl__gc *gc, uint32_t domid,
>>> + libxl_device_usb *usb)
>>> +{
>>> + libxl_device_usb *usbs = NULL;
>>> + libxl_device_usb *usb_find = NULL;
>>> + int i, num = 0, rc;
>>> +
>>> + assert(usb->busid || (usb->hostbus > 0 && usb->hostaddr > 0));
>>> +
>>> + if (!usb->busid) {
>>> + usb->busid = usb_busaddr_to_busid(gc, usb->hostbus, usb->hostaddr);
>>> + if (!usb->busid) {
>>> + LOG(ERROR, "USB device doesn't exist in sysfs");
>>> + return ERROR_INVAL;
>>> + }
>>> + }
>>
>> So here you're keying the removal on the *host* idea of what the device
>> is. But the standard would be to key this on the *guest* idea of what
>> the device is. When you're doing disk removal, you don't say
>>
>> "xl block-detach 1 /images/foo.img"
>>
>> that is, the path to the disk image; you say
>>
>> "xl block-detach 1 xvda"
>>
>> that is, the image as seen from the guest.
>>
>> Since there is no devid, you should make it possible to remove by
>> <ctrl,port>. Removing also by hostbus:hostaddr seems like useful
>> functionality, so it's probably not bad to keep it; but the <ctrl,port>
>> should be the main functionality.
>
> Do you think <ctrl,port> is better? That means in qemu emulated way,
> user also need to know the <ctrl, port> info of the USB device. In the past,
> for usb-add or usb-delete, <ctrl, port> info is hidden to user, it used
> bus.addr or verndorid.deviceid.
Hmm -- you know, you're right. For vif, vfb, and vkb it seems to use
devid (which is libxl specific, but indexed per vm); disk uses vdev
(which is indexed per vm); but pci pass-through uses BDF, which is based
on the host. (And that's what my HVM series from last year did as well.)
BTW, to remove a device over qmp, you need the name you gave the device
when you assigned it. But at least as of a year ago, there was no way
to query qemu to see which devices you had assigned; so you have to
store that information away in xenstore somewhere anyway. So from a
qemu perspective, having a devid or guest <ctrl,port> mapping isn't a
big deal.
Also, from a UI perspective: it should be easy for xl to translate host
bus.addr into guest <ctrl,port>, so I don't think that's as big a deal.
The main reason I have for wanting to do it at the guest level is so
that we can use the same interface for things which have no host handle
-- i.e., plugging or unplugging a virtual tablet, or maybe a USB disk.
-George
next prev parent reply other threads:[~2015-06-23 11:29 UTC|newest]
Thread overview: 88+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-10 3:20 [PATCH V4 0/7] xen pvusb toolstack work Chunyan Liu
2015-06-10 3:20 ` [PATCH V4 1/7] libxl: export some functions for pvusb use Chunyan Liu
2015-06-11 16:08 ` Ian Jackson
2015-06-11 16:28 ` Wei Liu
2015-06-12 15:14 ` Ian Jackson
2015-06-10 3:20 ` [PATCH V4 2/7] libxl_read_file_contents: add new entry to read sysfs file Chunyan Liu
2015-06-11 16:16 ` Ian Jackson
2015-06-12 7:00 ` Chun Yan Liu
2015-06-12 15:11 ` Ian Jackson
2015-06-10 3:20 ` [PATCH V4 3/7] libxl: add pvusb API Chunyan Liu
2015-06-11 15:00 ` Juergen Gross
2015-06-11 16:07 ` Ian Jackson
2015-06-11 16:42 ` Ian Jackson
2015-06-12 7:39 ` Chun Yan Liu
2015-06-12 8:06 ` Chun Yan Liu
2015-06-12 11:22 ` Ian Jackson
2015-06-15 14:17 ` George Dunlap
2015-06-15 14:25 ` Jürgen Groß
2015-06-15 14:34 ` George Dunlap
2015-06-15 18:26 ` Juergen Gross
2015-06-16 10:30 ` George Dunlap
2015-06-16 10:51 ` Juergen Gross
2015-06-16 11:11 ` George Dunlap
2015-06-16 11:19 ` Juergen Gross
2015-06-16 11:23 ` George Dunlap
2015-06-16 11:44 ` Ian Jackson
2015-06-17 11:24 ` Ian Campbell
2015-06-18 11:50 ` George Dunlap
2015-06-18 12:08 ` George Dunlap
2015-06-18 13:03 ` Juergen Gross
2015-06-22 13:29 ` Proposed plan for libxl USB interface (was Re: [PATCH V4 3/7] libxl: add pvusb API) George Dunlap
2015-06-22 14:14 ` Juergen Gross
2015-06-22 14:22 ` Ian Jackson
2015-06-23 2:42 ` Chun Yan Liu
2015-06-23 2:43 ` Chun Yan Liu
2015-06-23 2:44 ` Chun Yan Liu
2015-06-16 10:41 ` [PATCH V4 3/7] libxl: add pvusb API Ian Jackson
2015-06-16 10:56 ` Jürgen Groß
2015-06-16 11:03 ` George Dunlap
2015-06-16 11:10 ` Ian Jackson
2015-06-16 11:25 ` Juergen Gross
2015-06-16 11:45 ` George Dunlap
2015-06-16 12:02 ` Ian Jackson
2015-06-16 13:19 ` George Dunlap
2015-06-16 13:32 ` Juergen Gross
2015-06-16 13:37 ` [PATCH V4 3/7] libxl: add pvusb API [and 1 more messages] Ian Jackson
2015-06-16 14:41 ` George Dunlap
2015-06-16 15:58 ` Sander Eikelenboom
2015-06-16 15:59 ` Ian Jackson
2015-06-16 16:34 ` George Dunlap
2015-06-17 3:59 ` Juergen Gross
2015-06-17 10:27 ` George Dunlap
2015-06-18 6:24 ` Chun Yan Liu
2015-06-16 11:45 ` [PATCH V4 3/7] libxl: add pvusb API Ian Jackson
2015-06-16 13:06 ` Juergen Gross
2015-06-16 13:09 ` George Dunlap
2015-06-16 13:23 ` Juergen Gross
2015-06-16 13:29 ` George Dunlap
2015-06-16 13:49 ` Juergen Gross
2015-06-16 14:06 ` George Dunlap
2015-06-16 14:20 ` Juergen Gross
2015-06-16 14:37 ` George Dunlap
2015-06-17 11:34 ` Ian Campbell
2015-06-17 11:40 ` Juergen Gross
2015-06-18 6:20 ` Chun Yan Liu
2015-06-18 7:02 ` Juergen Gross
2015-06-18 8:50 ` Ian Campbell
2015-06-18 13:02 ` Juergen Gross
2015-06-16 15:38 ` George Dunlap
2015-06-16 11:01 ` George Dunlap
2015-06-16 11:12 ` Ian Jackson
2015-06-16 11:21 ` George Dunlap
2015-06-16 16:32 ` Ian Jackson
2015-06-16 16:39 ` George Dunlap
2015-06-16 16:51 ` Ross Philipson
2015-06-17 4:03 ` Jürgen Groß
2015-06-17 13:48 ` Ross Philipson
2015-06-15 17:47 ` George Dunlap
2015-06-23 10:18 ` Chun Yan Liu
2015-06-23 11:29 ` George Dunlap [this message]
2015-06-24 2:26 ` Chun Yan Liu
2015-06-10 3:20 ` [PATCH V4 4/7] libxl: add libxl_device_usb_assignable_list API Chunyan Liu
2015-06-10 3:20 ` [PATCH V4 5/7] xl: add pvusb commands Chunyan Liu
2015-06-12 7:37 ` Juergen Gross
2015-06-12 8:03 ` Chun Yan Liu
2015-06-12 8:22 ` Juergen Gross
2015-06-10 3:20 ` [PATCH V4 6/7] xl: add usb-assignable-list command Chunyan Liu
2015-06-10 3:20 ` [PATCH V4 7/7] domcreate: support pvusb in configuration file Chunyan Liu
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=558942FF.5060702@eu.citrix.com \
--to=george.dunlap@eu.citrix.com \
--cc=Ian.Jackson@eu.citrix.com \
--cc=caobosimon@gmail.com \
--cc=cyliu@suse.com \
--cc=ian.campbell@citrix.com \
--cc=wei.liu2@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.