Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Wei Liu]

On Thu, Sep 17, 2015 at 10:54:23AM +0100, George Dunlap wrote:
[...]
> > Hi, George,
> > 
> > I'm still confused about the expected look concerning PV/EMU type handling 
> > in
> > this patch series.
> > 
> > In earlier version, we tried to extract common things in libxl_usb.c and put
> > pvusb specific thing in libxl_pvusb.c, prefixed with pvusb_xxx. As you
> > suggested, we can leave that when EMU USB patch series added.
> > 
> > Now, about how to handle PV/EMU type in this patch series, I can think of 3 
> > ways:
> > 
> > 1. We define the enumeration (contains PV/AUTO only, user interface only 
> > allows
> > 'pv' or 'not specified', so we handle everything in 'pv' way without 
> > further 
> > check. Leave check and other adjusting things when EMU USB patch series 
> > added.
> > 
> > 2. We check domain type and set proper type if not specified (i.e. 'pv' for 
> > PV 
> > guest, 'emu' for HVM guest). In add/remove function, check if type='emu', 
> > report  
> > 'not supported' directly; otherwise, continue do following things. When EMU 
> > USB
> > patch serires added, need to extract common things and adjust the check 
> > place.
> > 
> > 3. Same as 2, but extract common things, only in PV/EMU USB specific part, 
> > check
> > type, if type='emu', report 'not supported'; otherwise, do pvusb work. When 
> > adding EMU USB patch series, only need to add EMU USB specific things in the
> > type='emu' branch.
> > 
> > Which one is expected? Or none?
> 
> So there are two questions here, first WRT the code, the second WRT the
> interface.
> 
> WRT the code, *normally* the first person to submit the code gets to
> have it easy, and the second person has to do all the work of
> refactoring.  So you would be completely within your rights to simply
> submit "libxl_usb.c", and make me refactor that into libxl_pvusb.c and
> whatever else (probably the qemu stuff would go in libxl_qmp.c).
> 
> Earlier I asked you as a favor to put things in libxl_pvusb.c, and you
> were kind enough to do so -- so thank you.  Just having things roughly
> where they might end up eventually has already been a big help.  I'll
> have to move some of the code around pretty much no matter what you do.
>  So I don't think it's worth making any more effort wrt the code itself.
> 
> WRT the interface -- if we do a release with PV defined, but not
> EMU/DEVICEMODEL, then when we add that option, we'll have to add Yet
> Another LIBL_HAS_BLAH.  I would personally like too avoid that.
> 

(Note that I didn't go through all emails)

I think adding yet another LIBXL_HAS_BLAH wouldn't be a problem. Chunyan
will have to add one anyway.

> As it happens, if you were to check this in now at the beginning of the
> cycle, it's very likely I could get the EMU side in before the release.
>  So it's *probably* OK to just write AUTO and PV.
> 

Again, extending the interface shouldn't be a problem -- we do that all
the time. So IMHO having AUTO and PV only is OK.

> I would personally prefer to play it safe and give all three interface
> elements (AUTO, PV, and EMU/DEVICEMODEL), and return ENOTSUPP (or
> whatever) for DEVICEMODEL until it's implemented.  But that's really a
> policy decision for the maintianers at this point.
> 

This works for me too. I don't really see a problem in choosing one way
or another TBH.

Wei.

>  -George

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Chun Yan Liu]

>>> On 9/9/2015 at 12:52 AM, in message <55ef1244@citrix.com>, George Dunlap
 wrote: 
> On 09/08/2015 03:17 PM, Ian Campbell wrote: 
> > On Mon, 2015-08-10 at 18:35 +0800, Chunyan Liu wrote: 
> >  
> > Sorry for the delay, between 4.6 freeze crunch, conference and vacation 
> > I've been a bit swamped. 
> >  
> > I'm just going to comment on the APIs (mainly public libxl.h and .idl) in 
> > this pass. 
> >  
> >> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h 
> >> index 5f9047c..05b6331 100644 
> >> --- a/tools/libxl/libxl.h 
> >> +++ b/tools/libxl/libxl.h 
> >> @@ -123,6 +123,23 @@ 
> >>  #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1 
> >>   
> >>  /* 
> >> + * LIBXL_HAVE_PVUSB indicates the functions for doing hot-plug of 
> >  
> > And cold-plug, no? 
>  
> So you should probably say something like "indicates functions for 
> plugging in USB devices through pvusb -- both hotplug and at domain 
> creation time." 
>  
> >> +libxl_usbctrl_type = Enumeration("usbctrl_type", [ 
> >> +(0, "AUTO"), 
> >  
> > What are the proposed semantics of using LIBXL_USBCTRL_TYPE_AUTO? 
>  
> Generally "DTRT".  Meaning: 
> 1. If your domain has no devicemodel, use PV. 
> 2. If your device has a devicemodel, and no PV drivers have peen 
> detected, use the devicemodel. 
> 3. If your device has a devicemodel, but PV drivers have been detected, 
> use PV. 
>  
> At the moment we don't have a way to check for PV drivers, so this just 
> collapses down to "PV for domains without a DM and DM for domains with a 
> DM." 
>  
> >  
> >> +(1, "PV"), 
> >> +(2, "QEMU"), 
> >  
> > Is "QEMU" what we want here, as opposed to, say, "EMU" (similar to NICs)? 
>  
> I had this as "DEVICEMODEL", since what we mean is that we want the 
> device model to provide access (and in theory in the future we may use a 
> different device model).  But "EMU" works for me too. 
>  
> > I think we probably don't want to go as fine grained as "XHCI" and "EHCI" 
> > etc, do we? I see we have a version field below, is it intended that there 
> > be some way to select between e.g. UHCI and OHCI (which IIRC are different 
> > USB 1.0 controllers). 
> >  
> > Maybe these questions should all be left aside for when QMEU support is 
> > actually added (AFAICT this field is just a placeholder)? In fact I glanced 
> > at the code and was surprised to find nothing checking for 
> > LIBXL_USBCTRL_TYPE at all, did I miss something? 
> >  
> > I think the two choices are: 
> >  
> > We can decide quickly and easily what the option(s) other than PV should be 
> > here and you include it in the IDL, but you would then need to check 
> > usbctrl->type == PV at various points, not silently treat all options as 
> > PV. 
> >  
> > Or this becomes a long conversation in which case I think your best bet 
> > would be to leave the enum with just the PV (and maybe AUTO) entries and 
> > leave the decision on the name for the emulated option to the series which 
> > implements that. 
>  
> I think the idea was to simply offer 1, 2, and 3 as options, and for the 
> devicemodel version, choose a suitable controller (or set of 
> controllers) for each option; similar to what usbversion= does now.

 
Hi, George,

I'm still confused about the expected look concerning PV/EMU type handling in
this patch series.

In earlier version, we tried to extract common things in libxl_usb.c and put
pvusb specific thing in libxl_pvusb.c, prefixed with pvusb_xxx. As you
suggested, we can leave that when EMU USB patch series added.

Now, about how to handle PV/EMU type in this patch series, I can think of 3 
ways:

1. We define the enumeration (contains PV/AUTO only, user interface only allows
'pv' or 'not specified', so we handle everything in 'pv' way without further 
check. Leave check and other adjusting things when EMU USB patch series added.

2. We check domain type and set proper type if not specified (i.e. 'pv' for PV 
guest, 'emu' for HVM guest). In add/remove function, check if type='emu', 
report  
'not supported' directly; otherwise, continue do following things. When EMU USB
patch serires added, need to extract common things and adjust the check place.

3. Same as 2, but extract common things, only in PV/EMU USB specific part, check
type, if type='emu', report 'not supported'; otherwise, do pvusb work. When 
adding EMU USB patch series, only need to add EMU USB specific things in the
type='emu' branch.

Which one is expected? Or none?

- Chunyan

>  
> >  
> >> +]) 
> >> + 
> >> +libxl_usbdev_type = Enumeration("usbdev_type", [ 
> >> +(0, "invalid"), 
> >> +(1, "hostdev"), 
> >> +]) 
> >> + 
> >> +libxl_device_usbctrl = Struct("device_usbctrl", [ 
> >> +("type", libxl_usbctrl_type), 
> >> +("devid", libxl_devid), 
> >> +("version", integer), 
> >> +("ports", integer), 
> >> +("backend_domid", libxl_domid), 
> >> +("backend_domname", string), 
> >> +   ]) 
> >> + 
> >> +libxl_device_usb = 

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Chun Yan Liu]

>>> On 9/9/2015 at 12:52 AM, in message <55ef1244@citrix.com>, George Dunlap
 wrote: 
> On 09/08/2015 03:17 PM, Ian Campbell wrote: 
> > On Mon, 2015-08-10 at 18:35 +0800, Chunyan Liu wrote: 
> >  
> > Sorry for the delay, between 4.6 freeze crunch, conference and vacation 
> > I've been a bit swamped. 
> >  
> > I'm just going to comment on the APIs (mainly public libxl.h and .idl) in 
> > this pass. 
> >  
> >> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h 
> >> index 5f9047c..05b6331 100644 
> >> --- a/tools/libxl/libxl.h 
> >> +++ b/tools/libxl/libxl.h 
> >> @@ -123,6 +123,23 @@ 
> >>  #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1 
> >>   
> >>  /* 
> >> + * LIBXL_HAVE_PVUSB indicates the functions for doing hot-plug of 
> >  
> > And cold-plug, no? 
>  
> So you should probably say something like "indicates functions for 
> plugging in USB devices through pvusb -- both hotplug and at domain 
> creation time." 
>  
> >> +libxl_usbctrl_type = Enumeration("usbctrl_type", [ 
> >> +(0, "AUTO"), 
> >  
> > What are the proposed semantics of using LIBXL_USBCTRL_TYPE_AUTO? 
>  
> Generally "DTRT".  Meaning: 
> 1. If your domain has no devicemodel, use PV. 
> 2. If your device has a devicemodel, and no PV drivers have peen 
> detected, use the devicemodel. 
> 3. If your device has a devicemodel, but PV drivers have been detected, 
> use PV. 
>  
> At the moment we don't have a way to check for PV drivers, so this just 
> collapses down to "PV for domains without a DM and DM for domains with a 
> DM." 
>  
> >  
> >> +(1, "PV"), 
> >> +(2, "QEMU"), 
> >  
> > Is "QEMU" what we want here, as opposed to, say, "EMU" (similar to NICs)? 
>  
> I had this as "DEVICEMODEL", since what we mean is that we want the 
> device model to provide access (and in theory in the future we may use a 
> different device model).  But "EMU" works for me too. 
>  
> > I think we probably don't want to go as fine grained as "XHCI" and "EHCI" 
> > etc, do we? I see we have a version field below, is it intended that there 
> > be some way to select between e.g. UHCI and OHCI (which IIRC are different 
> > USB 1.0 controllers). 
> >  
> > Maybe these questions should all be left aside for when QMEU support is 
> > actually added (AFAICT this field is just a placeholder)? In fact I glanced 
> > at the code and was surprised to find nothing checking for 
> > LIBXL_USBCTRL_TYPE at all, did I miss something? 
> >  
> > I think the two choices are: 
> >  
> > We can decide quickly and easily what the option(s) other than PV should be 
> > here and you include it in the IDL, but you would then need to check 
> > usbctrl->type == PV at various points, not silently treat all options as 
> > PV. 
> >  
> > Or this becomes a long conversation in which case I think your best bet 
> > would be to leave the enum with just the PV (and maybe AUTO) entries and 
> > leave the decision on the name for the emulated option to the series which 
> > implements that. 
>  
> I think the idea was to simply offer 1, 2, and 3 as options, and for the 
> devicemodel version, choose a suitable controller (or set of 
> controllers) for each option; similar to what usbversion= does now.

 
Hi, George,

I'm still confused about the expected look concerning PV/EMU type handling in
this patch series.

In earlier version, we tried to extract common things in libxl_usb.c and put
pvusb specific thing in libxl_pvusb.c, prefixed with pvusb_xxx. As you
suggested, we can leave that when EMU USB patch series added.

Now, about how to handle PV/EMU type in this patch series, I can think of 3 
ways:

1. We define the enumeration (contains PV/AUTO only, user interface only allows
'pv' or 'not specified', so we handle everything in 'pv' way without further 
check. Leave check and other adjusting things when EMU USB patch series added.

2. We check domain type and set proper type if not specified (i.e. 'pv' for PV 
guest, 'emu' for HVM guest). In add/remove function, check if type='emu', 
report  
'not supported' directly; otherwise, continue do following things. When EMU USB
patch serires added, need to extract common things and adjust the check place.

3. Same as 2, but extract common things, only in PV/EMU USB specific part, check
type, if type='emu', report 'not supported'; otherwise, do pvusb work. When 
adding EMU USB patch series, only need to add EMU USB specific things in the
type='emu' branch.

Which one is expected? Or none?

- Chunyan

>  
> >  
> >> +]) 
> >> + 
> >> +libxl_usbdev_type = Enumeration("usbdev_type", [ 
> >> +(0, "invalid"), 
> >> +(1, "hostdev"), 
> >> +]) 
> >> + 
> >> +libxl_device_usbctrl = Struct("device_usbctrl", [ 
> >> +("type", libxl_usbctrl_type), 
> >> +("devid", libxl_devid), 
> >> +("version", integer), 
> >> +("ports", integer), 
> >> +("backend_domid", libxl_domid), 
> >> +("backend_domname", string), 
> >> +   ]) 
> >> + 
> >> +libxl_device_usb = 

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[George Dunlap]

On 09/17/2015 09:19 AM, Chun Yan Liu wrote:
> 
> 
 On 9/9/2015 at 12:52 AM, in message <55ef1244@citrix.com>, George 
 Dunlap
>  wrote: 
>> On 09/08/2015 03:17 PM, Ian Campbell wrote: 
>>> On Mon, 2015-08-10 at 18:35 +0800, Chunyan Liu wrote: 
>>>  
>>> Sorry for the delay, between 4.6 freeze crunch, conference and vacation 
>>> I've been a bit swamped. 
>>>  
>>> I'm just going to comment on the APIs (mainly public libxl.h and .idl) in 
>>> this pass. 
>>>  
 diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h 
 index 5f9047c..05b6331 100644 
 --- a/tools/libxl/libxl.h 
 +++ b/tools/libxl/libxl.h 
 @@ -123,6 +123,23 @@ 
  #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1 
   
  /* 
 + * LIBXL_HAVE_PVUSB indicates the functions for doing hot-plug of 
>>>  
>>> And cold-plug, no? 
>>  
>> So you should probably say something like "indicates functions for 
>> plugging in USB devices through pvusb -- both hotplug and at domain 
>> creation time." 
>>  
 +libxl_usbctrl_type = Enumeration("usbctrl_type", [ 
 +(0, "AUTO"), 
>>>  
>>> What are the proposed semantics of using LIBXL_USBCTRL_TYPE_AUTO? 
>>  
>> Generally "DTRT".  Meaning: 
>> 1. If your domain has no devicemodel, use PV. 
>> 2. If your device has a devicemodel, and no PV drivers have peen 
>> detected, use the devicemodel. 
>> 3. If your device has a devicemodel, but PV drivers have been detected, 
>> use PV. 
>>  
>> At the moment we don't have a way to check for PV drivers, so this just 
>> collapses down to "PV for domains without a DM and DM for domains with a 
>> DM." 
>>  
>>>  
 +(1, "PV"), 
 +(2, "QEMU"), 
>>>  
>>> Is "QEMU" what we want here, as opposed to, say, "EMU" (similar to NICs)? 
>>  
>> I had this as "DEVICEMODEL", since what we mean is that we want the 
>> device model to provide access (and in theory in the future we may use a 
>> different device model).  But "EMU" works for me too. 
>>  
>>> I think we probably don't want to go as fine grained as "XHCI" and "EHCI" 
>>> etc, do we? I see we have a version field below, is it intended that there 
>>> be some way to select between e.g. UHCI and OHCI (which IIRC are different 
>>> USB 1.0 controllers). 
>>>  
>>> Maybe these questions should all be left aside for when QMEU support is 
>>> actually added (AFAICT this field is just a placeholder)? In fact I glanced 
>>> at the code and was surprised to find nothing checking for 
>>> LIBXL_USBCTRL_TYPE at all, did I miss something? 
>>>  
>>> I think the two choices are: 
>>>  
>>> We can decide quickly and easily what the option(s) other than PV should be 
>>> here and you include it in the IDL, but you would then need to check 
>>> usbctrl->type == PV at various points, not silently treat all options as 
>>> PV. 
>>>  
>>> Or this becomes a long conversation in which case I think your best bet 
>>> would be to leave the enum with just the PV (and maybe AUTO) entries and 
>>> leave the decision on the name for the emulated option to the series which 
>>> implements that. 
>>  
>> I think the idea was to simply offer 1, 2, and 3 as options, and for the 
>> devicemodel version, choose a suitable controller (or set of 
>> controllers) for each option; similar to what usbversion= does now.
> 
>  
> Hi, George,
> 
> I'm still confused about the expected look concerning PV/EMU type handling in
> this patch series.
> 
> In earlier version, we tried to extract common things in libxl_usb.c and put
> pvusb specific thing in libxl_pvusb.c, prefixed with pvusb_xxx. As you
> suggested, we can leave that when EMU USB patch series added.
> 
> Now, about how to handle PV/EMU type in this patch series, I can think of 3 
> ways:
> 
> 1. We define the enumeration (contains PV/AUTO only, user interface only 
> allows
> 'pv' or 'not specified', so we handle everything in 'pv' way without further 
> check. Leave check and other adjusting things when EMU USB patch series added.
> 
> 2. We check domain type and set proper type if not specified (i.e. 'pv' for 
> PV 
> guest, 'emu' for HVM guest). In add/remove function, check if type='emu', 
> report  
> 'not supported' directly; otherwise, continue do following things. When EMU 
> USB
> patch serires added, need to extract common things and adjust the check place.
> 
> 3. Same as 2, but extract common things, only in PV/EMU USB specific part, 
> check
> type, if type='emu', report 'not supported'; otherwise, do pvusb work. When 
> adding EMU USB patch series, only need to add EMU USB specific things in the
> type='emu' branch.
> 
> Which one is expected? Or none?

So there are two questions here, first WRT the code, the second WRT the
interface.

WRT the code, *normally* the first person to submit the code gets to
have it easy, and the second person has to do all the work of
refactoring.  So you would be completely within your rights to simply
submit "libxl_usb.c", and make me refactor that into libxl_pvusb.c 

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Chun Yan Liu]

>>> On 9/14/2015 at 10:03 PM, in message
<caflbxzayaqtejib3rfg8qhxjczqy8bbte0hxj+ft6abslf+...@mail.gmail.com>, George
Dunlap <george.dun...@eu.citrix.com> wrote: 
> On Mon, Sep 14, 2015 at 12:12 PM, Ian Jackson <ian.jack...@eu.citrix.com>  
> wrote: 
> > Juergen Gross writes ("Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb  
> API"): 
> >> On 09/14/2015 12:36 PM, George Dunlap wrote: 
> >> > Anyone want to look into the Linux source code to find out how big it 
> >> > will allow busnum / devnum to grow? 
> >> 
> >> drivers/usb/core/hcd.c is using a bitmap to find the next bus number 
> >> currently not in use. It's size is USB_MAXBUS which in turn has the 
> >> value 64. 
> >> 
> >> choose_devnum() in drivers/usb/core/hub.c is doing a similar job for 
> >> device numbers. Here the highest number supported is 127. 
> > 
> > We are defining an API, which shouldn't involve this kind of 
> > implementation-grobbling. 
> > 
> > At an API level, it seems that this Linux busnum is not documented to 
> > have any particular number or behaviour or range or anything.  We 
> > should use the biggest type we can use conveniently 
> > 
> > Do we need to worry that some bus might have 2^24 unplugs/plugs 
> > (perhaps in some kind of software emulation) and that we need to use a 
> > type which can hold a uint32_t or maybe even a uint64_t ? 
>  
> libusb is already a published API that supports uint8, or up to 255. 
> Following their lead seems like a reasonable thing to do.  If ever 
> that number goes above 255, basically every Linux program that touches 
> a USB device will need to be recompiled with a new version of libusb. 
>  
> Is there any reason for Linux to go above 255?  Things I can think of: 
>  
> 1. Users have more than 255 devices plugged into the same bus. 
>  
> 2. A security / confusion issue due to devnum reuse when users plug 
> and unplug devices hundreds of times. 
>  
> Both of these seem pretty unlikely. 
>  
> I would personally go with uint8, but int16 or int32 certainly won't hurt. 

So can we agree to use uint8 for hostbus and hostaddr as libusb does?

>  
>  -George 
>  
> ___ 
> Xen-devel mailing list 
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 
>  
>  



___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Chun Yan Liu]

>>> On 9/11/2015 at 09:26 PM, in message <1441978018.3549.33.ca...@citrix.com>, 
>>> Ian
Campbell  wrote: 
> On Thu, 2015-09-10 at 23:42 -0600, Chun Yan Liu wrote: 
> >  
> > > Do these fields have any particular size requirements arising from e.g.  
> the  
> > > USB spec or from possible dom0 implementations?  
> > >   
> > > If they have a well defined fixed size from a USB spec then maybe we 
> > > could  
> > > use the appropriate fixed size types?  
> >  
> > Di> dn't see the size limitation. In Linux kernel code, busnum and devnum  
> (here 
> > 'hostbus, hostaddr') are both 'int' type. 
>  
> Is that a Linux-specific implementation detail or a fundamental property of 
> USB? We should be designing the interface around Linux implementation 
> details. It seems like something in the USB spec ought to define precisely 
> the number of bits in both a bus number and a device address within that 
> bus. 

Have a look at USB 2.0 Spec, it has some description on Device Address: a 
seven-bit
value representing the address of the debvice on USB. (up to 127 devices). So 
 int8 is appropriate.
No description to Bus Num.

-Chunyan
>  
> >  And idProduct and idVendor are 'u16'. 
>  
> That's a USB spec thing, I think, so int16 in the IDL seems appropriate. 
>  
> Ian. 
>  
>  



___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Juergen Gross]

On 09/14/2015 12:36 PM, George Dunlap wrote:

On Mon, Sep 14, 2015 at 4:48 AM, Juergen Gross  wrote:

On 09/11/2015 04:41 PM, Ian Campbell wrote:


On Fri, 2015-09-11 at 16:18 +0200, Juergen Gross wrote:


On 09/11/2015 04:09 PM, Ian Campbell wrote:


On Fri, 2015-09-11 at 15:55 +0200, Juergen Gross wrote:


On 09/11/2015 03:26 PM, Ian Campbell wrote:


On Thu, 2015-09-10 at 23:42 -0600, Chun Yan Liu wrote:




Do these fields have any particular size requirements arising
from
e.g. the
USB spec or from possible dom0 implementations?

If they have a well defined fixed size from a USB spec then
maybe
we
could
use the appropriate fixed size types?



Di> dn't see the size limitation. In Linux kernel code, busnum
and
devnum (here
'hostbus, hostaddr') are both 'int' type.



Is that a Linux-specific implementation detail or a fundamental
property of
USB? We should be designing the interface around Linux
implementation
details. It seems like something in the USB spec ought to define
precisely
the number of bits in both a bus number and a device address within
that
bus.



The USB spec is only about _the_ bus. How many buses a host can
operate and how they are numbered is outside the USB spec.

Devices are addressed via their ports in the USB protocol. devnum
is a unique index for a device on the bus, the USB protocol
equivalent
is a list of ports of:
- 1 member in case of direct attached devices
- multiple members in case of hubs between bus and device



Thanks for the info. So an "address" in the USB protocol is actually a
"path" and "hostbus" is an implementation dependent shorthand for all
but
the last link in that path.



I'm not sure in which direction you are looking. "address" is a path.
A path is normally a list of ports starting at the host and walking
through all hubs until you reach the device. The "bus" is the root
of that path. So the number of buses the host knows of is the number
of USB host adapters without any hub.



OK, I thought I understood but the above suggests not.

In USB speak, the address is a list of port numbers, which you follow from
the host bus which is the root.

In Linux speak a "bus" is actually each hub along that path.



No. Each hub is just a port which happens to have more ports behind it.


Let me try a worked example and see if I've got it right. Lets take this
topology:

ROOT0
   |-PORT0 +--HUB1
   |-PORT1-,   |-PORT0 -- DEVICE A
   |   `-PORT1 -- DEVICE B
   |
   `--HUB2
   |-PORT0 -- DEVICE C
   `-PORT1 -- HUB3
   |-PORT0 -- DEVICE D
   `-PORT1 -x

ROOT1 -- ... other stuff

In the USB protocol there are two buses corresponding to ROOT0 and ROOT1.

So in the protocol the address of DEVICE D on the bus associated with
ROOT0
is [1,1,0], that is PORT1 on ROOT0 => PORT1 on HUB2 => PORT0 on HUB3.

DEVICE A is [0,0] on the bus associated with ROOT0, similarly.

In the Linux numbering scheme each ROOTn or HUBn is given a bus number,
somewhat arbitrarily (although I'm sure there is a scheme by which they
allocated). So perhaps:

ROOT0==BUS1



Correct.


HUB1==BUS2



No, Just Bus1-Port0 or Bus1:Devnum1


HUB2==BUS2



Bus1-Port1 or Bus1:Devnum2


HUB3==BUS4



Bus1-Port1.Port1 or Bus1:Devnum3


ROOT1==BUS42



Bus2


And in this scheme the address is hostbus+hostaddr, so DEVICE D is [3,0],
that is hostbus==3==HUB3, and port 0. And DEVICE A is [2,0]



Device D: Bus1-Port1.Port1.Port0 or e.g. Bus1:Devnum4
Device A: Bus1-Port0.Port0 or e.g. Bus1:Devnum5


Is that right?


One bus can have up to 31 ports.



So the answer is that hostaddr can be 5 bits?



5*8 (7 hubs and a device at the last level) == 40, that's the 1 trillion
I suggested before. Things are a little bit more complicated. A devnum
in a bus is never assigned twice. So when you plug in a device, it might
get devnum 6. Unplug it and replug it again will lead to devnum 7.


   In theory I think up to 7 cascaded
hubs are possible, but I don't think the resulting theoretical maximum
of about 1 trillion devices on a bus is to be considered. :-)



And this suggests that in principal a Linux hostbus could be 5*7 bits ==
35
bits, maybe. Or at least that any USB address can be encoded in that many
bits.



Busnum can grow to arbitrary values. A new USB bus detected will get a
new bus number. Removing it and adding it again will again use a new
number.


FWIW libusb seems to define these as uint8:

http://libusb.org/static/api-1.0/group__dev.html#gaf2718609d50c8ded2704e4051b3d2925

(I *think* that "bus number" and "device address" correspond to busnum
and devnum...)

Anyone want to look into the Linux source code to find out how big it
will allow busnum / devnum to grow?


drivers/usb/core/hcd.c is using a bitmap to find the next bus number
currently not in use. It's size is USB_MAXBUS which in turn has the
value 64.

choose_devnum() in drivers/usb/core/hub.c is doing a similar job for
device numbers. 

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Juergen Gross]

On 09/14/2015 01:12 PM, Ian Jackson wrote:

Juergen Gross writes ("Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API"):

On 09/14/2015 12:36 PM, George Dunlap wrote:

Anyone want to look into the Linux source code to find out how big it
will allow busnum / devnum to grow?


drivers/usb/core/hcd.c is using a bitmap to find the next bus number
currently not in use. It's size is USB_MAXBUS which in turn has the
value 64.

choose_devnum() in drivers/usb/core/hub.c is doing a similar job for
device numbers. Here the highest number supported is 127.


We are defining an API, which shouldn't involve this kind of
implementation-grobbling.

At an API level, it seems that this Linux busnum is not documented to
have any particular number or behaviour or range or anything.  We
should use the biggest type we can use conveniently.


Agreed.


Do we need to worry that some bus might have 2^24 unplugs/plugs
(perhaps in some kind of software emulation) and that we need to use a
type which can hold a uint32_t or maybe even a uint64_t ?


uint128_t ? ;-)

I think 24 bits should be more than enough. Nobody will accept such huge
numbers without any need: they are to be used by users.


Juergen


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[George Dunlap]

On Mon, Sep 14, 2015 at 4:48 AM, Juergen Gross  wrote:
> On 09/11/2015 04:41 PM, Ian Campbell wrote:
>>
>> On Fri, 2015-09-11 at 16:18 +0200, Juergen Gross wrote:
>>>
>>> On 09/11/2015 04:09 PM, Ian Campbell wrote:

 On Fri, 2015-09-11 at 15:55 +0200, Juergen Gross wrote:
>
> On 09/11/2015 03:26 PM, Ian Campbell wrote:
>>
>> On Thu, 2015-09-10 at 23:42 -0600, Chun Yan Liu wrote:
>>>
>>>
 Do these fields have any particular size requirements arising
 from
 e.g. the
 USB spec or from possible dom0 implementations?

 If they have a well defined fixed size from a USB spec then
 maybe
 we
 could
 use the appropriate fixed size types?
>>>
>>>
>>> Di> dn't see the size limitation. In Linux kernel code, busnum
>>> and
>>> devnum (here
>>> 'hostbus, hostaddr') are both 'int' type.
>>
>>
>> Is that a Linux-specific implementation detail or a fundamental
>> property of
>> USB? We should be designing the interface around Linux
>> implementation
>> details. It seems like something in the USB spec ought to define
>> precisely
>> the number of bits in both a bus number and a device address within
>> that
>> bus.
>
>
> The USB spec is only about _the_ bus. How many buses a host can
> operate and how they are numbered is outside the USB spec.
>
> Devices are addressed via their ports in the USB protocol. devnum
> is a unique index for a device on the bus, the USB protocol
> equivalent
> is a list of ports of:
> - 1 member in case of direct attached devices
> - multiple members in case of hubs between bus and device


 Thanks for the info. So an "address" in the USB protocol is actually a
 "path" and "hostbus" is an implementation dependent shorthand for all
 but
 the last link in that path.
>>>
>>>
>>> I'm not sure in which direction you are looking. "address" is a path.
>>> A path is normally a list of ports starting at the host and walking
>>> through all hubs until you reach the device. The "bus" is the root
>>> of that path. So the number of buses the host knows of is the number
>>> of USB host adapters without any hub.
>>
>>
>> OK, I thought I understood but the above suggests not.
>>
>> In USB speak, the address is a list of port numbers, which you follow from
>> the host bus which is the root.
>>
>> In Linux speak a "bus" is actually each hub along that path.
>
>
> No. Each hub is just a port which happens to have more ports behind it.
>
>> Let me try a worked example and see if I've got it right. Lets take this
>> topology:
>>
>> ROOT0
>>   |-PORT0 +--HUB1
>>   |-PORT1-,   |-PORT0 -- DEVICE A
>>   |   `-PORT1 -- DEVICE B
>>   |
>>   `--HUB2
>>   |-PORT0 -- DEVICE C
>>   `-PORT1 -- HUB3
>>   |-PORT0 -- DEVICE D
>>   `-PORT1 -x
>>
>> ROOT1 -- ... other stuff
>>
>> In the USB protocol there are two buses corresponding to ROOT0 and ROOT1.
>>
>> So in the protocol the address of DEVICE D on the bus associated with
>> ROOT0
>> is [1,1,0], that is PORT1 on ROOT0 => PORT1 on HUB2 => PORT0 on HUB3.
>>
>> DEVICE A is [0,0] on the bus associated with ROOT0, similarly.
>>
>> In the Linux numbering scheme each ROOTn or HUBn is given a bus number,
>> somewhat arbitrarily (although I'm sure there is a scheme by which they
>> allocated). So perhaps:
>>
>> ROOT0==BUS1
>
>
> Correct.
>
>> HUB1==BUS2
>
>
> No, Just Bus1-Port0 or Bus1:Devnum1
>
>> HUB2==BUS2
>
>
> Bus1-Port1 or Bus1:Devnum2
>
>> HUB3==BUS4
>
>
> Bus1-Port1.Port1 or Bus1:Devnum3
>
>> ROOT1==BUS42
>
>
> Bus2
>
>> And in this scheme the address is hostbus+hostaddr, so DEVICE D is [3,0],
>> that is hostbus==3==HUB3, and port 0. And DEVICE A is [2,0]
>
>
> Device D: Bus1-Port1.Port1.Port0 or e.g. Bus1:Devnum4
> Device A: Bus1-Port0.Port0 or e.g. Bus1:Devnum5
>
>> Is that right?
>>
>>> One bus can have up to 31 ports.
>>
>>
>> So the answer is that hostaddr can be 5 bits?
>
>
> 5*8 (7 hubs and a device at the last level) == 40, that's the 1 trillion
> I suggested before. Things are a little bit more complicated. A devnum
> in a bus is never assigned twice. So when you plug in a device, it might
> get devnum 6. Unplug it and replug it again will lead to devnum 7.
>
>>>   In theory I think up to 7 cascaded
>>> hubs are possible, but I don't think the resulting theoretical maximum
>>> of about 1 trillion devices on a bus is to be considered. :-)
>>
>>
>> And this suggests that in principal a Linux hostbus could be 5*7 bits ==
>> 35
>> bits, maybe. Or at least that any USB address can be encoded in that many
>> bits.
>
>
> Busnum can grow to arbitrary values. A new USB bus detected will get a
> new bus number. Removing it and adding it again will again use a new
> number.

FWIW libusb seems to define these as uint8:

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Ian Jackson]

Juergen Gross writes ("Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API"):
> On 09/14/2015 12:36 PM, George Dunlap wrote:
> > Anyone want to look into the Linux source code to find out how big it
> > will allow busnum / devnum to grow?
> 
> drivers/usb/core/hcd.c is using a bitmap to find the next bus number
> currently not in use. It's size is USB_MAXBUS which in turn has the
> value 64.
> 
> choose_devnum() in drivers/usb/core/hub.c is doing a similar job for
> device numbers. Here the highest number supported is 127.

We are defining an API, which shouldn't involve this kind of
implementation-grobbling.

At an API level, it seems that this Linux busnum is not documented to
have any particular number or behaviour or range or anything.  We
should use the biggest type we can use conveniently.

Do we need to worry that some bus might have 2^24 unplugs/plugs
(perhaps in some kind of software emulation) and that we need to use a
type which can hold a uint32_t or maybe even a uint64_t ?

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[George Dunlap]

On Mon, Sep 14, 2015 at 12:12 PM, Ian Jackson <ian.jack...@eu.citrix.com> wrote:
> Juergen Gross writes ("Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API"):
>> On 09/14/2015 12:36 PM, George Dunlap wrote:
>> > Anyone want to look into the Linux source code to find out how big it
>> > will allow busnum / devnum to grow?
>>
>> drivers/usb/core/hcd.c is using a bitmap to find the next bus number
>> currently not in use. It's size is USB_MAXBUS which in turn has the
>> value 64.
>>
>> choose_devnum() in drivers/usb/core/hub.c is doing a similar job for
>> device numbers. Here the highest number supported is 127.
>
> We are defining an API, which shouldn't involve this kind of
> implementation-grobbling.
>
> At an API level, it seems that this Linux busnum is not documented to
> have any particular number or behaviour or range or anything.  We
> should use the biggest type we can use conveniently
>
> Do we need to worry that some bus might have 2^24 unplugs/plugs
> (perhaps in some kind of software emulation) and that we need to use a
> type which can hold a uint32_t or maybe even a uint64_t ?

libusb is already a published API that supports uint8, or up to 255.
Following their lead seems like a reasonable thing to do.  If ever
that number goes above 255, basically every Linux program that touches
a USB device will need to be recompiled with a new version of libusb.

Is there any reason for Linux to go above 255?  Things I can think of:

1. Users have more than 255 devices plugged into the same bus.

2. A security / confusion issue due to devnum reuse when users plug
and unplug devices hundreds of times.

Both of these seem pretty unlikely.

I would personally go with uint8, but int16 or int32 certainly won't hurt.

 -George

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Juergen Gross]

On 09/11/2015 04:41 PM, Ian Campbell wrote:

On Fri, 2015-09-11 at 16:18 +0200, Juergen Gross wrote:

On 09/11/2015 04:09 PM, Ian Campbell wrote:

On Fri, 2015-09-11 at 15:55 +0200, Juergen Gross wrote:

On 09/11/2015 03:26 PM, Ian Campbell wrote:

On Thu, 2015-09-10 at 23:42 -0600, Chun Yan Liu wrote:



Do these fields have any particular size requirements arising
from
e.g. the
USB spec or from possible dom0 implementations?

If they have a well defined fixed size from a USB spec then
maybe
we
could
use the appropriate fixed size types?


Di> dn't see the size limitation. In Linux kernel code, busnum
and
devnum (here
'hostbus, hostaddr') are both 'int' type.


Is that a Linux-specific implementation detail or a fundamental
property of
USB? We should be designing the interface around Linux
implementation
details. It seems like something in the USB spec ought to define
precisely
the number of bits in both a bus number and a device address within
that
bus.


The USB spec is only about _the_ bus. How many buses a host can
operate and how they are numbered is outside the USB spec.

Devices are addressed via their ports in the USB protocol. devnum
is a unique index for a device on the bus, the USB protocol
equivalent
is a list of ports of:
- 1 member in case of direct attached devices
- multiple members in case of hubs between bus and device


Thanks for the info. So an "address" in the USB protocol is actually a
"path" and "hostbus" is an implementation dependent shorthand for all
but
the last link in that path.


I'm not sure in which direction you are looking. "address" is a path.
A path is normally a list of ports starting at the host and walking
through all hubs until you reach the device. The "bus" is the root
of that path. So the number of buses the host knows of is the number
of USB host adapters without any hub.


OK, I thought I understood but the above suggests not.

In USB speak, the address is a list of port numbers, which you follow from
the host bus which is the root.

In Linux speak a "bus" is actually each hub along that path.


No. Each hub is just a port which happens to have more ports behind it.


Let me try a worked example and see if I've got it right. Lets take this
topology:

ROOT0
  |-PORT0 +--HUB1
  |-PORT1-,   |-PORT0 -- DEVICE A
  |   `-PORT1 -- DEVICE B
  |
  `--HUB2
  |-PORT0 -- DEVICE C
  `-PORT1 -- HUB3
  |-PORT0 -- DEVICE D
  `-PORT1 -x

ROOT1 -- ... other stuff

In the USB protocol there are two buses corresponding to ROOT0 and ROOT1.

So in the protocol the address of DEVICE D on the bus associated with ROOT0
is [1,1,0], that is PORT1 on ROOT0 => PORT1 on HUB2 => PORT0 on HUB3.

DEVICE A is [0,0] on the bus associated with ROOT0, similarly.

In the Linux numbering scheme each ROOTn or HUBn is given a bus number,
somewhat arbitrarily (although I'm sure there is a scheme by which they
allocated). So perhaps:

ROOT0==BUS1


Correct.


HUB1==BUS2


No, Just Bus1-Port0 or Bus1:Devnum1


HUB2==BUS2


Bus1-Port1 or Bus1:Devnum2


HUB3==BUS4


Bus1-Port1.Port1 or Bus1:Devnum3


ROOT1==BUS42


Bus2


And in this scheme the address is hostbus+hostaddr, so DEVICE D is [3,0],
that is hostbus==3==HUB3, and port 0. And DEVICE A is [2,0]


Device D: Bus1-Port1.Port1.Port0 or e.g. Bus1:Devnum4
Device A: Bus1-Port0.Port0 or e.g. Bus1:Devnum5


Is that right?


One bus can have up to 31 ports.


So the answer is that hostaddr can be 5 bits?


5*8 (7 hubs and a device at the last level) == 40, that's the 1 trillion
I suggested before. Things are a little bit more complicated. A devnum
in a bus is never assigned twice. So when you plug in a device, it might
get devnum 6. Unplug it and replug it again will lead to devnum 7.


  In theory I think up to 7 cascaded
hubs are possible, but I don't think the resulting theoretical maximum
of about 1 trillion devices on a bus is to be considered. :-)


And this suggests that in principal a Linux hostbus could be 5*7 bits == 35
bits, maybe. Or at least that any USB address can be encoded in that many
bits.


Busnum can grow to arbitrary values. A new USB bus detected will get a
new bus number. Removing it and adding it again will again use a new
number.


Juergen


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Ian Campbell]

On Fri, 2015-09-11 at 15:55 +0200, Juergen Gross wrote:
> On 09/11/2015 03:26 PM, Ian Campbell wrote:
> > On Thu, 2015-09-10 at 23:42 -0600, Chun Yan Liu wrote:
> > > 
> > > > Do these fields have any particular size requirements arising from
> > > > e.g. the
> > > > USB spec or from possible dom0 implementations?
> > > > 
> > > > If they have a well defined fixed size from a USB spec then maybe
> > > > we
> > > > could
> > > > use the appropriate fixed size types?
> > > 
> > > Di> dn't see the size limitation. In Linux kernel code, busnum and
> > > devnum (here
> > > 'hostbus, hostaddr') are both 'int' type.
> > 
> > Is that a Linux-specific implementation detail or a fundamental
> > property of
> > USB? We should be designing the interface around Linux implementation
> > details. It seems like something in the USB spec ought to define
> > precisely
> > the number of bits in both a bus number and a device address within
> > that
> > bus.
> 
> The USB spec is only about _the_ bus. How many buses a host can
> operate and how they are numbered is outside the USB spec.
> 
> Devices are addressed via their ports in the USB protocol. devnum
> is a unique index for a device on the bus, the USB protocol equivalent
> is a list of ports of:
> - 1 member in case of direct attached devices
> - multiple members in case of hubs between bus and device

Thanks for the info. So an "address" in the USB protocol is actually a
"path" and "hostbus" is an implementation dependent shorthand for all but
the last link in that path.

What is the size of each element in the chain, that would seem to be the
correct size of "hostaddr".

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Ian Campbell]

On Thu, 2015-09-10 at 23:42 -0600, Chun Yan Liu wrote:
> 
> > Do these fields have any particular size requirements arising from e.g. the 
> > USB spec or from possible dom0 implementations? 
> >  
> > If they have a well defined fixed size from a USB spec then maybe we
> > could 
> > use the appropriate fixed size types? 
> 
> Di> dn't see the size limitation. In Linux kernel code, busnum and devnum 
> (here
> 'hostbus, hostaddr') are both 'int' type.

Is that a Linux-specific implementation detail or a fundamental property of
USB? We should be designing the interface around Linux implementation
details. It seems like something in the USB spec ought to define precisely
the number of bits in both a bus number and a device address within that
bus.

Note also that integer in the libxl IDL is signed 24 bits.

>  And idProduct and idVendor are 'u16'.

That's a USB spec thing, I think, so int16 in the IDL seems appropriate.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Juergen Gross]

On 09/11/2015 04:09 PM, Ian Campbell wrote:

On Fri, 2015-09-11 at 15:55 +0200, Juergen Gross wrote:

On 09/11/2015 03:26 PM, Ian Campbell wrote:

On Thu, 2015-09-10 at 23:42 -0600, Chun Yan Liu wrote:



Do these fields have any particular size requirements arising from
e.g. the
USB spec or from possible dom0 implementations?

If they have a well defined fixed size from a USB spec then maybe
we
could
use the appropriate fixed size types?


Di> dn't see the size limitation. In Linux kernel code, busnum and
devnum (here
'hostbus, hostaddr') are both 'int' type.


Is that a Linux-specific implementation detail or a fundamental
property of
USB? We should be designing the interface around Linux implementation
details. It seems like something in the USB spec ought to define
precisely
the number of bits in both a bus number and a device address within
that
bus.


The USB spec is only about _the_ bus. How many buses a host can
operate and how they are numbered is outside the USB spec.

Devices are addressed via their ports in the USB protocol. devnum
is a unique index for a device on the bus, the USB protocol equivalent
is a list of ports of:
- 1 member in case of direct attached devices
- multiple members in case of hubs between bus and device


Thanks for the info. So an "address" in the USB protocol is actually a
"path" and "hostbus" is an implementation dependent shorthand for all but
the last link in that path.


I'm not sure in which direction you are looking. "address" is a path.
A path is normally a list of ports starting at the host and walking
through all hubs until you reach the device. The "bus" is the root
of that path. So the number of buses the host knows of is the number
of USB host adapters without any hub.


What is the size of each element in the chain, that would seem to be the
correct size of "hostaddr".


One bus can have up to 31 ports. In theory I think up to 7 cascaded
hubs are possible, but I don't think the resulting theoretical maximum
of about 1 trillion devices on a bus is to be considered. :-)


Juergen

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Ian Campbell]

On Fri, 2015-09-11 at 16:18 +0200, Juergen Gross wrote:
> On 09/11/2015 04:09 PM, Ian Campbell wrote:
> > On Fri, 2015-09-11 at 15:55 +0200, Juergen Gross wrote:
> > > On 09/11/2015 03:26 PM, Ian Campbell wrote:
> > > > On Thu, 2015-09-10 at 23:42 -0600, Chun Yan Liu wrote:
> > > > > 
> > > > > > Do these fields have any particular size requirements arising
> > > > > > from
> > > > > > e.g. the
> > > > > > USB spec or from possible dom0 implementations?
> > > > > > 
> > > > > > If they have a well defined fixed size from a USB spec then
> > > > > > maybe
> > > > > > we
> > > > > > could
> > > > > > use the appropriate fixed size types?
> > > > > 
> > > > > Di> dn't see the size limitation. In Linux kernel code, busnum
> > > > > and
> > > > > devnum (here
> > > > > 'hostbus, hostaddr') are both 'int' type.
> > > > 
> > > > Is that a Linux-specific implementation detail or a fundamental
> > > > property of
> > > > USB? We should be designing the interface around Linux
> > > > implementation
> > > > details. It seems like something in the USB spec ought to define
> > > > precisely
> > > > the number of bits in both a bus number and a device address within
> > > > that
> > > > bus.
> > > 
> > > The USB spec is only about _the_ bus. How many buses a host can
> > > operate and how they are numbered is outside the USB spec.
> > > 
> > > Devices are addressed via their ports in the USB protocol. devnum
> > > is a unique index for a device on the bus, the USB protocol
> > > equivalent
> > > is a list of ports of:
> > > - 1 member in case of direct attached devices
> > > - multiple members in case of hubs between bus and device
> > 
> > Thanks for the info. So an "address" in the USB protocol is actually a
> > "path" and "hostbus" is an implementation dependent shorthand for all
> > but
> > the last link in that path.
> 
> I'm not sure in which direction you are looking. "address" is a path.
> A path is normally a list of ports starting at the host and walking
> through all hubs until you reach the device. The "bus" is the root
> of that path. So the number of buses the host knows of is the number
> of USB host adapters without any hub.

OK, I thought I understood but the above suggests not.

In USB speak, the address is a list of port numbers, which you follow from
the host bus which is the root.

In Linux speak a "bus" is actually each hub along that path.

Let me try a worked example and see if I've got it right. Lets take this
topology:

ROOT0
 |-PORT0 +--HUB1
 |-PORT1-,   |-PORT0 -- DEVICE A
 |   `-PORT1 -- DEVICE B
 |
 `--HUB2
 |-PORT0 -- DEVICE C
 `-PORT1 -- HUB3
 |-PORT0 -- DEVICE D
 `-PORT1 -x

ROOT1 -- ... other stuff

In the USB protocol there are two buses corresponding to ROOT0 and ROOT1.

So in the protocol the address of DEVICE D on the bus associated with ROOT0
is [1,1,0], that is PORT1 on ROOT0 => PORT1 on HUB2 => PORT0 on HUB3.

DEVICE A is [0,0] on the bus associated with ROOT0, similarly.

In the Linux numbering scheme each ROOTn or HUBn is given a bus number,
somewhat arbitrarily (although I'm sure there is a scheme by which they
allocated). So perhaps:

ROOT0==BUS1
HUB1==BUS2
HUB2==BUS2
HUB3==BUS4
ROOT1==BUS42

And in this scheme the address is hostbus+hostaddr, so DEVICE D is [3,0],
that is hostbus==3==HUB3, and port 0. And DEVICE A is [2,0]

Is that right?

> One bus can have up to 31 ports.

So the answer is that hostaddr can be 5 bits?

>  In theory I think up to 7 cascaded
> hubs are possible, but I don't think the resulting theoretical maximum
> of about 1 trillion devices on a bus is to be considered. :-)

And this suggests that in principal a Linux hostbus could be 5*7 bits == 35
bits, maybe. Or at least that any USB address can be encoded in that many
bits.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Ian Jackson]

Most of what you say is right, I think, but:

Ian Campbell writes ("Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API"):
> On Fri, 2015-09-11 at 16:18 +0200, Juergen Gross wrote:
> >  In theory I think up to 7 cascaded
> > hubs are possible, but I don't think the resulting theoretical maximum
> > of about 1 trillion devices on a bus is to be considered. :-)
> 
> And this suggests that in principal a Linux hostbus could be 5*7 bits == 35
> bits, maybe. Or at least that any USB address can be encoded in that many
> bits.

No.  The total possible number of `buses' in that sense
per root is:
1   root
  + 31  in theory a hub plugged into each of 31 ports
  + 31^2in theory a hub plugged into each port
  ...
  + 31^6
You could plug hubs into the last layer but the maximum depth is 7 so
you wouldn't be able to plug any devices into that last hub.

And of course the number of root hubs is not really limited by
anything meaningful.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Chun Yan Liu]

>>> On 9/8/2015 at 10:17 PM, in message <1441721852.24450.120.ca...@citrix.com>,
Ian Campbell  wrote: 
> On Mon, 2015-08-10 at 18:35 +0800, Chunyan Liu wrote: 
>  
> Sorry for the delay, between 4.6 freeze crunch, conference and vacation 
> I've been a bit swamped. 
>  
> I'm just going to comment on the APIs (mainly public libxl.h and .idl) in 
> this pass. 
>  
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h 
> > index 5f9047c..05b6331 100644 
> > --- a/tools/libxl/libxl.h 
> > +++ b/tools/libxl/libxl.h 
> > @@ -123,6 +123,23 @@ 
> >  #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1 
> >   
> >  /* 
> > + * LIBXL_HAVE_PVUSB indicates the functions for doing hot-plug of 
>  
> And cold-plug, no? 
>  
> > + * USB devices through pvusb. 
> > + * 
> > + * With this functionality, one can add/remove USB controllers to/from 
> > + * guest, and attach/detach USB devices to/from USB controllers. To add 
> > + * USB controllers and USB devices, one can either adding USB controllers 
> > + * first and then attaching USB devices to some USB controller, or adding 
> > + * USB devices to guest directly, it will automatically create a USB 
> > + * controller for USB devices to attach. To remove USB controllers or USB 
> > + * devices, one can either remove USB devices under USB controller one by 
> > + * one and then remove USB controller, or remove USB controller directly, 
> > + * it will remove all USB devices under it automatically. 
>  
> I think this API documentation belongs alongside the API declarations (i.e 
> the prototypes) rather than hidden away next to the feature flag. 
>  
> > + * 
> > + */ 
> > +#define LIBXL_HAVE_PVUSB 1 
> > + 
> > +/* 
> >   * LIBXL_HAVE_BUILDINFO_HVM_VENDOR_DEVICE indicates that the 
> >   * libxl_vendor_device field is present in the hvm sections of 
> >   * libxl_domain_build_info. This field tells libxl which 
> > @@ -1389,6 +1406,54 @@ 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 Controllers*/ 
> >  
> [] 
>  
> Seem fine. 
>  
> > + 
> > +/* USB Devices */ 
> > +int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_usb  
> *usb, 
> > + const libxl_asyncop_how *ao_how) 
> > + LIBXL_EXTERNAL_CALLERS_ONLY; 
> > + 
> > +int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid,  
> libxl_device_usb *usb, 
> > +const libxl_asyncop_how *ao_how) 
> > +LIBXL_EXTERNAL_CALLERS_ONLY; 
> > + 
> > +libxl_device_usb * 
> > +libxl_device_usb_list(libxl_ctx *ctx, uint32_t domid, int *num); 
> > + 
> > +libxl_device_usb * 
> > +libxl_device_usb_list_per_usbctrl(libxl_ctx *ctx, uint32_t domid, 
> > +  libxl_devid usbctrl, int *num); 
>  
> I'd probably say "..._for_usbctrl" or "..._by_usbctrl", but that's just 
> nitpicking. 
>  
> > + 
> > +void libxl_device_usb_list_free(libxl_device_usb *list, int nr); 
> > + 
> > +int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid, 
> > + libxl_device_usb *usb, 
> > + libxl_usbinfo *usbinfo); 
> > + 
> >  /* Network Interfaces */ 
> >  int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, 
> > libxl_device_nic *nic, 
> >   const libxl_asyncop_how *ao_how) 
> [...] 
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl 
> > index ef346e7..ef10484 100644 
> > --- a/tools/libxl/libxl_types.idl 
> > +++ b/tools/libxl/libxl_types.idl 
> > @@ -594,6 +594,37 @@ libxl_device_rdm = Struct("device_rdm", [ 
> >  ("policy", libxl_rdm_reserve_policy), 
> >  ]) 
> >   
> > +libxl_usbctrl_type = Enumeration("usbctrl_type", [ 
> > +(0, "AUTO"), 
>  
> What are the proposed semantics of using LIBXL_USBCTRL_TYPE_AUTO? 
>  
> > +(1, "PV"), 
> > +(2, "QEMU"), 
>  
> Is "QEMU" what we want here, as opposed to, say, "EMU" (similar to NICs)? 
>  
> I think we probably don't want to go as fine grained as "XHCI" and "EHCI" 
> etc, do we? I see we have a version field below, is it intended that there 
> be some way to select between e.g. UHCI and OHCI (which IIRC are different 
> USB 1.0 controllers). 
>  
> Maybe these questions should all be left aside for when QMEU support is 
> actually added (AFAICT this field is just a placeholder)? In fact I glanced 
> at the code and was surprised to find nothing checking for 
> LIBXL_USBCTRL_TYPE at all, did I miss something? 
>  
> I think the two choices are: 
>  
> We can decide quickly and easily what the option(s) other than PV should be 
> here and you include it in the IDL, but you would then need to check 
> usbctrl->type == PV at various points, not silently treat all options as 
> PV. 
>  
> Or this becomes a long conversation in which 

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Chun Yan Liu]

>>> On 9/9/2015 at 12:52 AM, in message <55ef1244@citrix.com>, George Dunlap
 wrote: 
> On 09/08/2015 03:17 PM, Ian Campbell wrote: 
> > On Mon, 2015-08-10 at 18:35 +0800, Chunyan Liu wrote: 
> >  
> > Sorry for the delay, between 4.6 freeze crunch, conference and vacation 
> > I've been a bit swamped. 
> >  
> > I'm just going to comment on the APIs (mainly public libxl.h and .idl) in 
> > this pass. 
> >  
> >> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h 
> >> index 5f9047c..05b6331 100644 
> >> --- a/tools/libxl/libxl.h 
> >> +++ b/tools/libxl/libxl.h 
> >> @@ -123,6 +123,23 @@ 
> >>  #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1 
> >>   
> >>  /* 
> >> + * LIBXL_HAVE_PVUSB indicates the functions for doing hot-plug of 
> >  
> > And cold-plug, no? 
>  
> So you should probably say something like "indicates functions for 
> plugging in USB devices through pvusb -- both hotplug and at domain 
> creation time." 

Thanks. Will clarify.

>  
> >> +libxl_usbctrl_type = Enumeration("usbctrl_type", [ 
> >> +(0, "AUTO"), 
> >  
> > What are the proposed semantics of using LIBXL_USBCTRL_TYPE_AUTO? 
>  
> Generally "DTRT".  Meaning: 
> 1. If your domain has no devicemodel, use PV. 
> 2. If your device has a devicemodel, and no PV drivers have peen 
> detected, use the devicemodel. 
> 3. If your device has a devicemodel, but PV drivers have been detected, 
> use PV. 
>  
> At the moment we don't have a way to check for PV drivers, so this just 
> collapses down to "PV for domains without a DM and DM for domains with a 
> DM." 

Better to be: by default, PV for PV guest and DM for HVM guest.

Thanks,
Chunyan

>  
> >  
> >> +(1, "PV"), 
> >> +(2, "QEMU"), 
> >  
> > Is "QEMU" what we want here, as opposed to, say, "EMU" (similar to NICs)? 
>  
> I had this as "DEVICEMODEL", since what we mean is that we want the 
> device model to provide access (and in theory in the future we may use a 
> different device model).  But "EMU" works for me too. 
>  
> > I think we probably don't want to go as fine grained as "XHCI" and "EHCI" 
> > etc, do we? I see we have a version field below, is it intended that there 
> > be some way to select between e.g. UHCI and OHCI (which IIRC are different 
> > USB 1.0 controllers). 
> >  
> > Maybe these questions should all be left aside for when QMEU support is 
> > actually added (AFAICT this field is just a placeholder)? In fact I glanced 
> > at the code and was surprised to find nothing checking for 
> > LIBXL_USBCTRL_TYPE at all, did I miss something? 
> >  
> > I think the two choices are: 
> >  
> > We can decide quickly and easily what the option(s) other than PV should be 
> > here and you include it in the IDL, but you would then need to check 
> > usbctrl->type == PV at various points, not silently treat all options as 
> > PV. 
> >  
> > Or this becomes a long conversation in which case I think your best bet 
> > would be to leave the enum with just the PV (and maybe AUTO) entries and 
> > leave the decision on the name for the emulated option to the series which 
> > implements that. 
>  
> I think the idea was to simply offer 1, 2, and 3 as options, and for the 
> devicemodel version, choose a suitable controller (or set of 
> controllers) for each option; similar to what usbversion= does now. 
>  
> >  
> >> +]) 
> >> + 
> >> +libxl_usbdev_type = Enumeration("usbdev_type", [ 
> >> +(0, "invalid"), 
> >> +(1, "hostdev"), 
> >> +]) 
> >> + 
> >> +libxl_device_usbctrl = Struct("device_usbctrl", [ 
> >> +("type", libxl_usbctrl_type), 
> >> +("devid", libxl_devid), 
> >> +("version", integer), 
> >> +("ports", integer), 
> >> +("backend_domid", libxl_domid), 
> >> +("backend_domname", string), 
> >> +   ]) 
> >> + 
> >> +libxl_device_usb = Struct("device_usb", [ 
> >> +("ctrl", libxl_devid), 
> >> +("port", integer), 
> >> +("u", KeyedUnion(None, libxl_usbdev_type, "devtype", 
> >> +   [("hostdev", Struct(None, [ 
> >> + ("hostbus",   integer), 
> >> + ("hostaddr",  integer)])), 
> >> +("invalid", None), 
> >  
> > AIUI this is what was agreed to, i.e. an enum with only one real option, in 
> > order to leave a space for new devtypes without major API overhaul. 
> >  
> > Please can you confirm that hostbus and hostaddr are both flat integer 
> > namespaces (i.e. there is no structure to the bits within either, they are 
> > just a number). 
>  
> I can confirm this. 
>  
>  -George 
>  
>  
>  


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Ian Campbell]

On Mon, 2015-08-10 at 18:35 +0800, Chunyan Liu wrote:

Sorry for the delay, between 4.6 freeze crunch, conference and vacation
I've been a bit swamped.

I'm just going to comment on the APIs (mainly public libxl.h and .idl) in
this pass.

> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 5f9047c..05b6331 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -123,6 +123,23 @@
>  #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
>  
>  /*
> + * LIBXL_HAVE_PVUSB indicates the functions for doing hot-plug of

And cold-plug, no?

> + * USB devices through pvusb.
> + *
> + * With this functionality, one can add/remove USB controllers to/from
> + * guest, and attach/detach USB devices to/from USB controllers. To add
> + * USB controllers and USB devices, one can either adding USB controllers
> + * first and then attaching USB devices to some USB controller, or adding
> + * USB devices to guest directly, it will automatically create a USB
> + * controller for USB devices to attach. To remove USB controllers or USB
> + * devices, one can either remove USB devices under USB controller one by
> + * one and then remove USB controller, or remove USB controller directly,
> + * it will remove all USB devices under it automatically.

I think this API documentation belongs alongside the API declarations (i.e
the prototypes) rather than hidden away next to the feature flag.

> + *
> + */
> +#define LIBXL_HAVE_PVUSB 1
> +
> +/*
>   * LIBXL_HAVE_BUILDINFO_HVM_VENDOR_DEVICE indicates that the
>   * libxl_vendor_device field is present in the hvm sections of
>   * libxl_domain_build_info. This field tells libxl which
> @@ -1389,6 +1406,54 @@ 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 Controllers*/
> 
[]

Seem fine.

> +
> +/* USB Devices */
> +int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_usb 
> *usb,
> + const libxl_asyncop_how *ao_how)
> + LIBXL_EXTERNAL_CALLERS_ONLY;
> +
> +int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_usb 
> *usb,
> +const libxl_asyncop_how *ao_how)
> +LIBXL_EXTERNAL_CALLERS_ONLY;
> +
> +libxl_device_usb *
> +libxl_device_usb_list(libxl_ctx *ctx, uint32_t domid, int *num);
> +
> +libxl_device_usb *
> +libxl_device_usb_list_per_usbctrl(libxl_ctx *ctx, uint32_t domid,
> +  libxl_devid usbctrl, int *num);

I'd probably say "..._for_usbctrl" or "..._by_usbctrl", but that's just
nitpicking.

> +
> +void libxl_device_usb_list_free(libxl_device_usb *list, int nr);
> +
> +int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid,
> + libxl_device_usb *usb,
> + libxl_usbinfo *usbinfo);
> +
>  /* Network Interfaces */
>  int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid,
> libxl_device_nic *nic,
>   const libxl_asyncop_how *ao_how)
[...]
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index ef346e7..ef10484 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -594,6 +594,37 @@ libxl_device_rdm = Struct("device_rdm", [
>  ("policy", libxl_rdm_reserve_policy),
>  ])
>  
> +libxl_usbctrl_type = Enumeration("usbctrl_type", [
> +(0, "AUTO"),

What are the proposed semantics of using LIBXL_USBCTRL_TYPE_AUTO?

> +(1, "PV"),
> +(2, "QEMU"),

Is "QEMU" what we want here, as opposed to, say, "EMU" (similar to NICs)?

I think we probably don't want to go as fine grained as "XHCI" and "EHCI"
etc, do we? I see we have a version field below, is it intended that there
be some way to select between e.g. UHCI and OHCI (which IIRC are different
USB 1.0 controllers).

Maybe these questions should all be left aside for when QMEU support is
actually added (AFAICT this field is just a placeholder)? In fact I glanced
at the code and was surprised to find nothing checking for
LIBXL_USBCTRL_TYPE at all, did I miss something?

I think the two choices are:

We can decide quickly and easily what the option(s) other than PV should be
here and you include it in the IDL, but you would then need to check
usbctrl->type == PV at various points, not silently treat all options as
PV.

Or this becomes a long conversation in which case I think your best bet
would be to leave the enum with just the PV (and maybe AUTO) entries and
leave the decision on the name for the emulated option to the series which
implements that.

> +])
> +
> +libxl_usbdev_type = Enumeration("usbdev_type", [
> +(0, "invalid"),
> +(1, "hostdev"),
> +])
> +
> +libxl_device_usbctrl = Struct("device_usbctrl", [
> +("type", libxl_usbctrl_type),
> +("devid", libxl_devid),
> +("version", integer),
> +("ports", integer),
> 

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[George Dunlap]

On 09/08/2015 03:17 PM, Ian Campbell wrote:
> On Mon, 2015-08-10 at 18:35 +0800, Chunyan Liu wrote:
> 
> Sorry for the delay, between 4.6 freeze crunch, conference and vacation
> I've been a bit swamped.
> 
> I'm just going to comment on the APIs (mainly public libxl.h and .idl) in
> this pass.
> 
>> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
>> index 5f9047c..05b6331 100644
>> --- a/tools/libxl/libxl.h
>> +++ b/tools/libxl/libxl.h
>> @@ -123,6 +123,23 @@
>>  #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
>>  
>>  /*
>> + * LIBXL_HAVE_PVUSB indicates the functions for doing hot-plug of
> 
> And cold-plug, no?

So you should probably say something like "indicates functions for
plugging in USB devices through pvusb -- both hotplug and at domain
creation time."

>> +libxl_usbctrl_type = Enumeration("usbctrl_type", [
>> +(0, "AUTO"),
> 
> What are the proposed semantics of using LIBXL_USBCTRL_TYPE_AUTO?

Generally "DTRT".  Meaning:
1. If your domain has no devicemodel, use PV.
2. If your device has a devicemodel, and no PV drivers have peen
detected, use the devicemodel.
3. If your device has a devicemodel, but PV drivers have been detected,
use PV.

At the moment we don't have a way to check for PV drivers, so this just
collapses down to "PV for domains without a DM and DM for domains with a
DM."

> 
>> +(1, "PV"),
>> +(2, "QEMU"),
> 
> Is "QEMU" what we want here, as opposed to, say, "EMU" (similar to NICs)?

I had this as "DEVICEMODEL", since what we mean is that we want the
device model to provide access (and in theory in the future we may use a
different device model).  But "EMU" works for me too.

> I think we probably don't want to go as fine grained as "XHCI" and "EHCI"
> etc, do we? I see we have a version field below, is it intended that there
> be some way to select between e.g. UHCI and OHCI (which IIRC are different
> USB 1.0 controllers).
> 
> Maybe these questions should all be left aside for when QMEU support is
> actually added (AFAICT this field is just a placeholder)? In fact I glanced
> at the code and was surprised to find nothing checking for
> LIBXL_USBCTRL_TYPE at all, did I miss something?
> 
> I think the two choices are:
> 
> We can decide quickly and easily what the option(s) other than PV should be
> here and you include it in the IDL, but you would then need to check
> usbctrl->type == PV at various points, not silently treat all options as
> PV.
> 
> Or this becomes a long conversation in which case I think your best bet
> would be to leave the enum with just the PV (and maybe AUTO) entries and
> leave the decision on the name for the emulated option to the series which
> implements that.

I think the idea was to simply offer 1, 2, and 3 as options, and for the
devicemodel version, choose a suitable controller (or set of
controllers) for each option; similar to what usbversion= does now.

> 
>> +])
>> +
>> +libxl_usbdev_type = Enumeration("usbdev_type", [
>> +(0, "invalid"),
>> +(1, "hostdev"),
>> +])
>> +
>> +libxl_device_usbctrl = Struct("device_usbctrl", [
>> +("type", libxl_usbctrl_type),
>> +("devid", libxl_devid),
>> +("version", integer),
>> +("ports", integer),
>> +("backend_domid", libxl_domid),
>> +("backend_domname", string),
>> +   ])
>> +
>> +libxl_device_usb = Struct("device_usb", [
>> +("ctrl", libxl_devid),
>> +("port", integer),
>> +("u", KeyedUnion(None, libxl_usbdev_type, "devtype",
>> +   [("hostdev", Struct(None, [
>> + ("hostbus",   integer),
>> + ("hostaddr",  integer)])),
>> +("invalid", None),
> 
> AIUI this is what was agreed to, i.e. an enum with only one real option, in
> order to leave a space for new devtypes without major API overhaul.
> 
> Please can you confirm that hostbus and hostaddr are both flat integer
> namespaces (i.e. there is no structure to the bits within either, they are
> just a number).

I can confirm this.

 -George


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Chun Yan Liu]

 On 8/13/2015 at 05:09 PM, in message
20150813090938.gi7...@zion.uk.xensource.com, Wei Liu wei.l...@citrix.com
wrote: 
 On Tue, Aug 11, 2015 at 08:24:01PM -0600, Chun Yan Liu wrote: 
   
   
   On 8/11/2015 at 07:27 PM, in message 
  2015082702.gf7...@zion.uk.xensource.com, Wei Liu 
  wei.l...@citrix.com 
  wrote:  
   On Mon, Aug 10, 2015 at 06:35:24PM +0800, Chunyan Liu wrote:  
Add pvusb APIs, including:  
 - attach/detach (create/destroy) virtual usb controller.  
 - attach/detach usb device  
 - list usb controller and usb devices  
 - some other helper functions  
  
Signed-off-by: Chunyan Liu cy...@suse.com  
Signed-off-by: Simon Cao caobosi...@gmail.com  
  
---  
changes:  
  - Address George's comments:  
  * Update libxl_device_usb_getinfo to read ctrl/port only and  
get other information.  
  * Update backend path according to xenstore frontend 'xxx/backend'  
entry instead of using TOOLSTACK_DOMID.  
  * Use 'type' to indicate qemu/pv instead of previous naming 
'protocol'.  
  
  * Add USB 'devtype' union, currently only includes hostdev  
  
 
   I will leave this to Ian and George since they had strong opinions on  
   this.  
 
   I only skimmed this patch. Some comments below.  
 
   [...]  
+  
+int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid,  
+ libxl_device_usb *usb,  
+ libxl_usbinfo *usbinfo);  
+  
 /* Network Interfaces */  
 int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid,  
 libxl_device_nic   
   *nic,  
  const libxl_asyncop_how *ao_how)  
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c  
index bee5ed5..935f25b 100644  
--- a/tools/libxl/libxl_device.c  
+++ b/tools/libxl/libxl_device.c  
@@ -676,6 +676,10 @@ void libxl__devices_destroy(libxl__egc *egc,   
   libxl__devices_remove_state *drs)  
 aodev-action = LIBXL__DEVICE_ACTION_REMOVE;  
 aodev-dev = dev;  
 aodev-force = drs-force;  
+if (dev-backend_kind == LIBXL__DEVICE_KIND_VUSB) {  
+libxl__initiate_device_usbctrl_remove(egc, aodev); 
 
+continue;  
+}  
 
   Is there a risk that this races with individual device removal? I think  
   you get away with it because removal of individual device is idempotent?  
   
  You mean races with other device removal (like 'vbd') ? Yes, it is  
 idempotent. 
  Only for 'vusb' (corresponding to USB controller), before removing USB  
 controller 
  it will first removing all USB devices under it.  
   
  
 No. What I mean is, the removal of usbctrl triggers removal of all 
 assigned usb devices. And then this function initiates removal of 
 assigned usb devices again. Is this a possible scenario? 
  
 
 libxl__initiate_device_remove(egc, aodev);  
 }  
 }  
diff --git a/tools/libxl/libxl_internal.h 
b/tools/libxl/libxl_internal.h  
index f98f089..5be3b3a 100644  
--- a/tools/libxl/libxl_internal.h  
+++ b/tools/libxl/libxl_internal.h  
@@ -2553,6 +2553,14 @@ _hidden void libxl__device_vtpm_add(libxl__egc  
 *egc,   
   uint32_t domid,  
libxl_device_vtpm *vtpm,  
libxl__ao_device *aodev);  
   
+_hidden void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t 
domid,  
+   libxl_device_usbctrl *usbctrl,  
+   libxl__ao_device *aodev);  
+  
+_hidden void libxl__device_usb_add(libxl__egc *egc, uint32_t domid,  
+   libxl_device_usb *usb,  
+   libxl__ao_device *aodev);  
+  
 /* Internal function to connect a vkb device */  
 _hidden int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid,  
   libxl_device_vkb *vkb);  
@@ -2585,6 +2593,13 @@ _hidden void   
   libxl__wait_device_connection(libxl__egc*,  
 _hidden void libxl__initiate_device_remove(libxl__egc *egc,  
libxl__ao_device *aodev);  
   
+_hidden int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,  
   [...]  
+void libxl__device_usb_add(libxl__egc *egc, uint32_t domid,  
+   libxl_device_usb *usb,  
+   libxl__ao_device *aodev)  
+{  
+STATE_AO_GC(aodev-ao);  
+int rc = -1;  
+char *busid = NULL;  
+  
+assert(usb-u.hostdev.hostbus  0  usb-u.hostdev.hostaddr  0); 
 
+  
+busid = usb_busaddr_to_busid(gc, usb-u.hostdev.hostbus,  
+   

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Wei Liu]

On Tue, Aug 11, 2015 at 08:24:01PM -0600, Chun Yan Liu wrote:
 
 
  On 8/11/2015 at 07:27 PM, in message
 2015082702.gf7...@zion.uk.xensource.com, Wei Liu wei.l...@citrix.com
 wrote: 
  On Mon, Aug 10, 2015 at 06:35:24PM +0800, Chunyan Liu wrote: 
   Add pvusb APIs, including: 
- attach/detach (create/destroy) virtual usb controller. 
- attach/detach usb device 
- list usb controller and usb devices 
- some other helper functions 

   Signed-off-by: Chunyan Liu cy...@suse.com 
   Signed-off-by: Simon Cao caobosi...@gmail.com 

   --- 
   changes: 
 - Address George's comments: 
 * Update libxl_device_usb_getinfo to read ctrl/port only and 
   get other information. 
 * Update backend path according to xenstore frontend 'xxx/backend' 
   entry instead of using TOOLSTACK_DOMID. 
 * Use 'type' to indicate qemu/pv instead of previous naming 'protocol'. 
 * Add USB 'devtype' union, currently only includes hostdev 

   
  I will leave this to Ian and George since they had strong opinions on 
  this. 
   
  I only skimmed this patch. Some comments below. 
   
  [...] 
   + 
   +int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid, 
   + libxl_device_usb *usb, 
   + libxl_usbinfo *usbinfo); 
   + 
/* Network Interfaces */ 
int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, 
   libxl_device_nic  
  *nic, 
 const libxl_asyncop_how *ao_how) 
   diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c 
   index bee5ed5..935f25b 100644 
   --- a/tools/libxl/libxl_device.c 
   +++ b/tools/libxl/libxl_device.c 
   @@ -676,6 +676,10 @@ void libxl__devices_destroy(libxl__egc *egc,  
  libxl__devices_remove_state *drs) 
aodev-action = LIBXL__DEVICE_ACTION_REMOVE; 
aodev-dev = dev; 
aodev-force = drs-force; 
   +if (dev-backend_kind == LIBXL__DEVICE_KIND_VUSB) { 
   +libxl__initiate_device_usbctrl_remove(egc, aodev); 
   +continue; 
   +} 
   
  Is there a risk that this races with individual device removal? I think 
  you get away with it because removal of individual device is idempotent? 
 
 You mean races with other device removal (like 'vbd') ? Yes, it is idempotent.
 Only for 'vusb' (corresponding to USB controller), before removing USB 
 controller
 it will first removing all USB devices under it. 
 

No. What I mean is, the removal of usbctrl triggers removal of all
assigned usb devices. And then this function initiates removal of
assigned usb devices again. Is this a possible scenario?

   
libxl__initiate_device_remove(egc, aodev); 
} 
} 
   diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h 
   index f98f089..5be3b3a 100644 
   --- a/tools/libxl/libxl_internal.h 
   +++ b/tools/libxl/libxl_internal.h 
   @@ -2553,6 +2553,14 @@ _hidden void libxl__device_vtpm_add(libxl__egc 
   *egc,  
  uint32_t domid, 
   libxl_device_vtpm *vtpm, 
   libxl__ao_device *aodev); 
 
   +_hidden void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid, 
   +   libxl_device_usbctrl *usbctrl, 
   +   libxl__ao_device *aodev); 
   + 
   +_hidden void libxl__device_usb_add(libxl__egc *egc, uint32_t domid, 
   +   libxl_device_usb *usb, 
   +   libxl__ao_device *aodev); 
   + 
/* Internal function to connect a vkb device */ 
_hidden int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid, 
  libxl_device_vkb *vkb); 
   @@ -2585,6 +2593,13 @@ _hidden void  
  libxl__wait_device_connection(libxl__egc*, 
_hidden void libxl__initiate_device_remove(libxl__egc *egc, 
   libxl__ao_device *aodev); 
 
   +_hidden int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid, 
  [...] 
   +void libxl__device_usb_add(libxl__egc *egc, uint32_t domid, 
   +   libxl_device_usb *usb, 
   +   libxl__ao_device *aodev) 
   +{ 
   +STATE_AO_GC(aodev-ao); 
   +int rc = -1; 
   +char *busid = NULL; 
   + 
   +assert(usb-u.hostdev.hostbus  0  usb-u.hostdev.hostaddr  0); 
   + 
   +busid = usb_busaddr_to_busid(gc, usb-u.hostdev.hostbus, 
   + usb-u.hostdev.hostaddr); 
   +if (!busid) { 
   +LOG(ERROR, USB device doesn't exist in sysfs); 
   +goto out; 
   +} 
   + 
   +if (!is_usb_assignable(gc, usb)) { 
   +LOG(ERROR, USB device is not assignable.); 
   +goto out; 
   +} 
   + 
   +/* check usb device is already assigned */ 
   +

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Chun Yan Liu]

 On 8/13/2015 at 05:09 PM, in message
20150813090938.gi7...@zion.uk.xensource.com, Wei Liu wei.l...@citrix.com
wrote: 
 On Tue, Aug 11, 2015 at 08:24:01PM -0600, Chun Yan Liu wrote: 
   
   
   On 8/11/2015 at 07:27 PM, in message 
  2015082702.gf7...@zion.uk.xensource.com, Wei Liu 
  wei.l...@citrix.com 
  wrote:  
   On Mon, Aug 10, 2015 at 06:35:24PM +0800, Chunyan Liu wrote:  
Add pvusb APIs, including:  
 - attach/detach (create/destroy) virtual usb controller.  
 - attach/detach usb device  
 - list usb controller and usb devices  
 - some other helper functions  
  
Signed-off-by: Chunyan Liu cy...@suse.com  
Signed-off-by: Simon Cao caobosi...@gmail.com  
  
---  
changes:  
  - Address George's comments:  
  * Update libxl_device_usb_getinfo to read ctrl/port only and  
get other information.  
  * Update backend path according to xenstore frontend 'xxx/backend'  
entry instead of using TOOLSTACK_DOMID.  
  * Use 'type' to indicate qemu/pv instead of previous naming 
'protocol'.  
  
  * Add USB 'devtype' union, currently only includes hostdev  
  
 
   I will leave this to Ian and George since they had strong opinions on  
   this.  
 
   I only skimmed this patch. Some comments below.  
 
   [...]  
+  
+int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid,  
+ libxl_device_usb *usb,  
+ libxl_usbinfo *usbinfo);  
+  
 /* Network Interfaces */  
 int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid,  
 libxl_device_nic   
   *nic,  
  const libxl_asyncop_how *ao_how)  
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c  
index bee5ed5..935f25b 100644  
--- a/tools/libxl/libxl_device.c  
+++ b/tools/libxl/libxl_device.c  
@@ -676,6 +676,10 @@ void libxl__devices_destroy(libxl__egc *egc,   
   libxl__devices_remove_state *drs)  
 aodev-action = LIBXL__DEVICE_ACTION_REMOVE;  
 aodev-dev = dev;  
 aodev-force = drs-force;  
+if (dev-backend_kind == LIBXL__DEVICE_KIND_VUSB) {  
+libxl__initiate_device_usbctrl_remove(egc, aodev); 
 
+continue;  
+}  
 
   Is there a risk that this races with individual device removal? I think  
   you get away with it because removal of individual device is idempotent?  
   
  You mean races with other device removal (like 'vbd') ? Yes, it is  
 idempotent. 
  Only for 'vusb' (corresponding to USB controller), before removing USB  
 controller 
  it will first removing all USB devices under it.
h   
  
 No. What I mean is, the removal of usbctrl triggers removal of all 
 assigned usb devices. And then this function initiates removal of 
 assigned usb devices again. Is this a possible scenario? 

No, it's not possible. libxl__devices_destroy is used in domain destroy, it's
trying to scan each device type in xenstore and destroy them. Since USB device
is NOT presented as a separate device type but inside USB controller (which is
represented by a 'vusb' device in xenstore), so when scanning 'vusb' type, it
tries to destroy USB controller, within that it will destroy all USB devices 
under
that controller. No entry to remove USB device alone. 

Thanks,
Chunyan

  
 
 libxl__initiate_device_remove(egc, aodev);  
 }  
 }  
diff --git a/tools/libxl/libxl_internal.h 
b/tools/libxl/libxl_internal.h  
index f98f089..5be3b3a 100644  
--- a/tools/libxl/libxl_internal.h  
+++ b/tools/libxl/libxl_internal.h  
@@ -2553,6 +2553,14 @@ _hidden void libxl__device_vtpm_add(libxl__egc  
 *egc,   
   uint32_t domid,  
libxl_device_vtpm *vtpm,  
libxl__ao_device *aodev);  
   
+_hidden void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t 
domid,  
+   libxl_device_usbctrl *usbctrl,  
+   libxl__ao_device *aodev);  
+  
+_hidden void libxl__device_usb_add(libxl__egc *egc, uint32_t domid,  
+   libxl_device_usb *usb,  
+   libxl__ao_device *aodev);  
+  
 /* Internal function to connect a vkb device */  
 _hidden int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid,  
   libxl_device_vkb *vkb);  
@@ -2585,6 +2593,13 @@ _hidden void   
   libxl__wait_device_connection(libxl__egc*,  
 _hidden void libxl__initiate_device_remove(libxl__egc *egc,  
libxl__ao_device *aodev);  
   
+_hidden int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t 

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Wei Liu]

On Mon, Aug 10, 2015 at 06:35:24PM +0800, Chunyan Liu wrote:
 Add pvusb APIs, including:
  - attach/detach (create/destroy) virtual usb controller.
  - attach/detach usb device
  - list usb controller and usb devices
  - some other helper functions
 
 Signed-off-by: Chunyan Liu cy...@suse.com
 Signed-off-by: Simon Cao caobosi...@gmail.com
 
 ---
 changes:
   - Address George's comments:
   * Update libxl_device_usb_getinfo to read ctrl/port only and
 get other information.
   * Update backend path according to xenstore frontend 'xxx/backend'
 entry instead of using TOOLSTACK_DOMID.
   * Use 'type' to indicate qemu/pv instead of previous naming 'protocol'.
   * Add USB 'devtype' union, currently only includes hostdev
 

I will leave this to Ian and George since they had strong opinions on
this.

I only skimmed this patch. Some comments below.

[...]
 +
 +int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid,
 + libxl_device_usb *usb,
 + libxl_usbinfo *usbinfo);
 +
  /* Network Interfaces */
  int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic 
 *nic,
   const libxl_asyncop_how *ao_how)
 diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
 index bee5ed5..935f25b 100644
 --- a/tools/libxl/libxl_device.c
 +++ b/tools/libxl/libxl_device.c
 @@ -676,6 +676,10 @@ void libxl__devices_destroy(libxl__egc *egc, 
 libxl__devices_remove_state *drs)
  aodev-action = LIBXL__DEVICE_ACTION_REMOVE;
  aodev-dev = dev;
  aodev-force = drs-force;
 +if (dev-backend_kind == LIBXL__DEVICE_KIND_VUSB) {
 +libxl__initiate_device_usbctrl_remove(egc, aodev);
 +continue;
 +}

Is there a risk that this races with individual device removal? I think
you get away with it because removal of individual device is idempotent?

  libxl__initiate_device_remove(egc, aodev);
  }
  }
 diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
 index f98f089..5be3b3a 100644
 --- a/tools/libxl/libxl_internal.h
 +++ b/tools/libxl/libxl_internal.h
 @@ -2553,6 +2553,14 @@ _hidden void libxl__device_vtpm_add(libxl__egc *egc, 
 uint32_t domid,
 libxl_device_vtpm *vtpm,
 libxl__ao_device *aodev);
  
 +_hidden void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
 +   libxl_device_usbctrl *usbctrl,
 +   libxl__ao_device *aodev);
 +
 +_hidden void libxl__device_usb_add(libxl__egc *egc, uint32_t domid,
 +   libxl_device_usb *usb,
 +   libxl__ao_device *aodev);
 +
  /* Internal function to connect a vkb device */
  _hidden int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid,
libxl_device_vkb *vkb);
 @@ -2585,6 +2593,13 @@ _hidden void libxl__wait_device_connection(libxl__egc*,
  _hidden void libxl__initiate_device_remove(libxl__egc *egc,
 libxl__ao_device *aodev);
  
 +_hidden int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
[...]
 +void libxl__device_usb_add(libxl__egc *egc, uint32_t domid,
 +   libxl_device_usb *usb,
 +   libxl__ao_device *aodev)
 +{
 +STATE_AO_GC(aodev-ao);
 +int rc = -1;
 +char *busid = NULL;
 +
 +assert(usb-u.hostdev.hostbus  0  usb-u.hostdev.hostaddr  0);
 +
 +busid = usb_busaddr_to_busid(gc, usb-u.hostdev.hostbus,
 + usb-u.hostdev.hostaddr);
 +if (!busid) {
 +LOG(ERROR, USB device doesn't exist in sysfs);
 +goto out;
 +}
 +
 +if (!is_usb_assignable(gc, usb)) {
 +LOG(ERROR, USB device is not assignable.);
 +goto out;
 +}
 +
 +/* check usb device is already assigned */
 +if (is_usb_assigned(gc, usb)) {
 +LOG(ERROR, USB device is already attached to a domain.);
 +goto out;
 +}
 +
 +rc = libxl__device_usb_setdefault(gc, domid, usb, aodev-update_json);
 +if (rc) goto out;
 +
 +rc = libxl__device_usb_add_xenstore(gc, domid, usb, aodev-update_json);
 +if (rc) goto out;
 +
 +rc = usbback_dev_assign(gc, usb);
 +if (rc) {
 +libxl__device_usb_remove_xenstore(gc, domid, usb);
 +goto out;
 +}
 +
 +libxl__ao_complete(egc, ao, 0);
 +rc = 0;
 +
 +out:

You forget to complete ao in failure path.

But I'm not very familiar with the AO machinery, I will let Ian comment
on this.

Wei.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Chun Yan Liu]

 On 8/11/2015 at 07:27 PM, in message
2015082702.gf7...@zion.uk.xensource.com, Wei Liu wei.l...@citrix.com
wrote: 
 On Mon, Aug 10, 2015 at 06:35:24PM +0800, Chunyan Liu wrote: 
  Add pvusb APIs, including: 
   - attach/detach (create/destroy) virtual usb controller. 
   - attach/detach usb device 
   - list usb controller and usb devices 
   - some other helper functions 
   
  Signed-off-by: Chunyan Liu cy...@suse.com 
  Signed-off-by: Simon Cao caobosi...@gmail.com 
   
  --- 
  changes: 
- Address George's comments: 
* Update libxl_device_usb_getinfo to read ctrl/port only and 
  get other information. 
* Update backend path according to xenstore frontend 'xxx/backend' 
  entry instead of using TOOLSTACK_DOMID. 
* Use 'type' to indicate qemu/pv instead of previous naming 'protocol'. 
* Add USB 'devtype' union, currently only includes hostdev 
   
  
 I will leave this to Ian and George since they had strong opinions on 
 this. 
  
 I only skimmed this patch. Some comments below. 
  
 [...] 
  + 
  +int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid, 
  + libxl_device_usb *usb, 
  + libxl_usbinfo *usbinfo); 
  + 
   /* Network Interfaces */ 
   int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic  
 *nic, 
const libxl_asyncop_how *ao_how) 
  diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c 
  index bee5ed5..935f25b 100644 
  --- a/tools/libxl/libxl_device.c 
  +++ b/tools/libxl/libxl_device.c 
  @@ -676,6 +676,10 @@ void libxl__devices_destroy(libxl__egc *egc,  
 libxl__devices_remove_state *drs) 
   aodev-action = LIBXL__DEVICE_ACTION_REMOVE; 
   aodev-dev = dev; 
   aodev-force = drs-force; 
  +if (dev-backend_kind == LIBXL__DEVICE_KIND_VUSB) { 
  +libxl__initiate_device_usbctrl_remove(egc, aodev); 
  +continue; 
  +} 
  
 Is there a risk that this races with individual device removal? I think 
 you get away with it because removal of individual device is idempotent? 

You mean races with other device removal (like 'vbd') ? Yes, it is idempotent.
Only for 'vusb' (corresponding to USB controller), before removing USB 
controller
it will first removing all USB devices under it. 

  
   libxl__initiate_device_remove(egc, aodev); 
   } 
   } 
  diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h 
  index f98f089..5be3b3a 100644 
  --- a/tools/libxl/libxl_internal.h 
  +++ b/tools/libxl/libxl_internal.h 
  @@ -2553,6 +2553,14 @@ _hidden void libxl__device_vtpm_add(libxl__egc *egc, 
   
 uint32_t domid, 
  libxl_device_vtpm *vtpm, 
  libxl__ao_device *aodev); 

  +_hidden void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid, 
  +   libxl_device_usbctrl *usbctrl, 
  +   libxl__ao_device *aodev); 
  + 
  +_hidden void libxl__device_usb_add(libxl__egc *egc, uint32_t domid, 
  +   libxl_device_usb *usb, 
  +   libxl__ao_device *aodev); 
  + 
   /* Internal function to connect a vkb device */ 
   _hidden int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid, 
 libxl_device_vkb *vkb); 
  @@ -2585,6 +2593,13 @@ _hidden void  
 libxl__wait_device_connection(libxl__egc*, 
   _hidden void libxl__initiate_device_remove(libxl__egc *egc, 
  libxl__ao_device *aodev); 

  +_hidden int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid, 
 [...] 
  +void libxl__device_usb_add(libxl__egc *egc, uint32_t domid, 
  +   libxl_device_usb *usb, 
  +   libxl__ao_device *aodev) 
  +{ 
  +STATE_AO_GC(aodev-ao); 
  +int rc = -1; 
  +char *busid = NULL; 
  + 
  +assert(usb-u.hostdev.hostbus  0  usb-u.hostdev.hostaddr  0); 
  + 
  +busid = usb_busaddr_to_busid(gc, usb-u.hostdev.hostbus, 
  + usb-u.hostdev.hostaddr); 
  +if (!busid) { 
  +LOG(ERROR, USB device doesn't exist in sysfs); 
  +goto out; 
  +} 
  + 
  +if (!is_usb_assignable(gc, usb)) { 
  +LOG(ERROR, USB device is not assignable.); 
  +goto out; 
  +} 
  + 
  +/* check usb device is already assigned */ 
  +if (is_usb_assigned(gc, usb)) { 
  +LOG(ERROR, USB device is already attached to a domain.); 
  +goto out; 
  +} 
  + 
  +rc = libxl__device_usb_setdefault(gc, domid, usb, aodev-update_json); 
  +if (rc) goto out; 
  + 
  +rc = libxl__device_usb_add_xenstore(gc, domid, usb, 
  aodev-update_json); 
  +if (rc) goto out; 
  + 
  +rc = 

[Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Chunyan Liu]

Add pvusb APIs, including:
 - attach/detach (create/destroy) virtual usb controller.
 - attach/detach usb device
 - list usb controller and usb devices
 - some other helper functions

Signed-off-by: Chunyan Liu cy...@suse.com
Signed-off-by: Simon Cao caobosi...@gmail.com

---
changes:
  - Address George's comments:
  * Update libxl_device_usb_getinfo to read ctrl/port only and
get other information.
  * Update backend path according to xenstore frontend 'xxx/backend'
entry instead of using TOOLSTACK_DOMID.
  * Use 'type' to indicate qemu/pv instead of previous naming 'protocol'.
  * Add USB 'devtype' union, currently only includes hostdev

 tools/libxl/Makefile |2 +-
 tools/libxl/libxl.c  |   53 ++
 tools/libxl/libxl.h  |   65 ++
 tools/libxl/libxl_device.c   |4 +
 tools/libxl/libxl_internal.h |   20 +-
 tools/libxl/libxl_osdeps.h   |   13 +
 tools/libxl/libxl_pvusb.c| 1320 ++
 tools/libxl/libxl_types.idl  |   59 ++
 tools/libxl/libxl_types_internal.idl |1 +
 tools/libxl/libxl_utils.c|   16 +
 tools/libxl/libxl_utils.h|5 +
 11 files changed, 1556 insertions(+), 2 deletions(-)
 create mode 100644 tools/libxl/libxl_pvusb.c

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 9036076..cdb50fe 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -103,7 +103,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o 
libxl_pci.o \
libxl_stream_read.o libxl_stream_write.o \
libxl_save_callout.o _libxl_save_msgs_callout.o \
libxl_qmp.o libxl_event.o libxl_fork.o \
-   libxl_dom_suspend.o $(LIBXL_OBJS-y)
+   libxl_dom_suspend.o libxl_pvusb.o $(LIBXL_OBJS-y)
 LIBXL_OBJS += libxl_genid.o
 LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
 
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 006e8da..35843a8 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4179,11 +4179,54 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
 
 
/**/
 
+/* Macro for defining device remove/destroy functions for usbctrl */
+/* Following functions are defined:
+ * libxl_device_usbctrl_remove
+ * libxl_device_usbctrl_destroy
+ */
+
+#define DEFINE_DEVICE_REMOVE_EXT(type, removedestroy, f)\
+int libxl_device_##type##_##removedestroy(libxl_ctx *ctx,   \
+uint32_t domid, libxl_device_##type *type,  \
+const libxl_asyncop_how *ao_how)\
+{   \
+AO_CREATE(ctx, domid, ao_how);  \
+libxl__device *device;  \
+libxl__ao_device *aodev;\
+int rc; \
+\
+GCNEW(device);  \
+rc = libxl__device_from_##type(gc, domid, type, device);\
+if (rc != 0) goto out;  \
+\
+GCNEW(aodev);   \
+libxl__prepare_ao_device(ao, aodev);\
+aodev-action = LIBXL__DEVICE_ACTION_REMOVE;\
+aodev-dev = device;\
+aodev-callback = device_addrm_aocomplete;  \
+aodev-force = f;   \
+libxl__initiate_device_##type##_remove(egc, aodev); \
+\
+out:\
+if (rc) return AO_CREATE_FAIL(rc);  \
+return AO_INPROGRESS;   \
+}
+
+
+DEFINE_DEVICE_REMOVE_EXT(usbctrl, remove, 0)
+DEFINE_DEVICE_REMOVE_EXT(usbctrl, destroy, 1)
+
+#undef DEFINE_DEVICE_REMOVE_EXT
+
+/**/
+
 /* Macro for defining device addition functions in a compact way */
 /* The following functions are defined:
  * libxl_device_disk_add
  * libxl_device_nic_add
  * libxl_device_vtpm_add
+ * libxl_device_usbctrl_add
+ * libxl_device_usb_add
  */
 
 #define DEFINE_DEVICE_ADD(type) \
@@ -4215,6 +4258,12 @@ DEFINE_DEVICE_ADD(nic)
 /* vtpm */
 DEFINE_DEVICE_ADD(vtpm)
 
+/* usbctrl */
+DEFINE_DEVICE_ADD(usbctrl)