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

[Chun Yan Liu]

On 10/13/2015 09:15 PM, George Dunlap wrote:

On 13/10/15 02:46, Chun Yan Liu wrote:


On 10/12/2015 09:46 PM, George Dunlap wrote:

On 12/10/15 08:19, Chun Yan Liu wrote:

+
+usbinfo->devnum = usb->u.hostdev.hostaddr;
+usbinfo->busnum = usb->u.hostdev.hostbus;
+
+busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
+ usb->u.hostdev.hostaddr);
+if (!busid) {
+rc = ERROR_FAIL;
+goto out;
+}
+
+filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idVendor", busid);
+if (!libxl_read_sysfs_file_contents(ctx, filename, , NULL))
+sscanf(buf, "%" SCNx16, >idVendor);
+
+filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idProduct", busid);
+if (!libxl_read_sysfs_file_contents(ctx, filename, , NULL))
+sscanf(buf, "%" SCNx16, >idProduct);
+
+filename = GCSPRINTF(SYSFS_USB_DEV"/%s/manufacturer", busid);
+if (!libxl_read_sysfs_file_contents(ctx, filename, ,
) &&
+buflen > 0) {
+/* replace \n to \0 */
+if (((char *)buf)[buflen - 1] == '\n')
+((char *)buf)[buflen - 1] = '\0';
+usbinfo->manuf = libxl__strdup(NOGC, buf);
+   }
+
+filename = GCSPRINTF(SYSFS_USB_DEV"/%s/product", busid);
+if (!libxl_read_sysfs_file_contents(ctx, filename, ,
) &&
+buflen > 0) {
+/* replace \n to \0 */
+if (((char *)buf)[buflen - 1] == '\n')
+((char *)buf)[buflen - 1] = '\0';
+usbinfo->prod = libxl__strdup(NOGC, buf);
+}

   Basically, starting here, we have information which won't be
available
if we're using a pvusb driver domain.
   This information is nice-to-have, but I don't think this
information is
directly relevant to libxl or xl; the funcitonality to get this
information is available from other libraries like libusb.  I'm
inclined
to say that if we want to have pvusb driver domains (and I think we
do),
we should just get rid of this information.

For command 'xl usb-list', those information should be reported to user.
Do you mean we could call libusb to get thoes information directly in
xl toolstack and get rid of this function?

I think we can keep the function, since every other device type has the
function XXX_getinfo. But here we could check backend_domid, for
backend=dom0, doing above work; for backend!=dom0 (e.g. pvusb
driver domain, no matter how, it should also be able to let users
getting
those information. Can add code  in future.)

Does xl need that information?  Can't the user get that information from
lsusb?

In any case, I can see why it would be *useful* to have in xl.  But
about having it in libxl, I think this question sort of goes along with
the question about the next patch -- whether libxl should be in the
business of providing general information about the USB devices it's
handling, or whether it should limit itself to doing what is absolutely
necessary to talk to usbback.

There's a part of me that sees the point of it: it's not actually that
much extra code (at least for Linux), and it makes it easy to add some
very useful features to xl.

On the other hand, it's not portable to other OSes.  At the moment of
course pvusb isn't portable either, but once we have qemu USB (providing
either emulated or PV usb) then I *think* most of the other
functionality will Just Work on any platform that can compile qemu
(incl. FreeBSD, NetBSD, ), won't it?  The code you're introducing here
would have to be re-implented for those platforms, and for every new
platform that wanted to include this functionality, wouldn't it?

So, about the portability problem, I think it's back to: do need to
update code to call libusb instead of reading sysfs now? Except
for this function, still have places reading sysfs to get hostbus,
hostaddr, vendorId, deviceId. Those are not portable for other
platform.

I realize I didn't give you very clear guidance; I guess I was hoping to
get an opinion from the tools maintainers.  Or perhaps, I was hoping to
let them be the "bad guys" and say, "You can't have this feature in
libxl", so I wouldn't have to. :-)

In the absence of guidance to the contrary, I suggest that patch series
should focus on getting the core pvusb functionality in, without the
extra usb-querying bits.  Then we can discuss a further series which
either adds the usb querying functionality to libxl, or implement it in
xl using libusb.

OK. Got it. Thanks.

-Chunyan



  -George




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

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

[George Dunlap]

On 13/10/15 02:46, Chun Yan Liu wrote:
> 
> 
> On 10/12/2015 09:46 PM, George Dunlap wrote:
>> On 12/10/15 08:19, Chun Yan Liu wrote:
> +
> +usbinfo->devnum = usb->u.hostdev.hostaddr;
> +usbinfo->busnum = usb->u.hostdev.hostbus;
> +
> +busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
> + usb->u.hostdev.hostaddr);
> +if (!busid) {
> +rc = ERROR_FAIL;
> +goto out;
> +}
> +
> +filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idVendor", busid);
> +if (!libxl_read_sysfs_file_contents(ctx, filename, , NULL))
> +sscanf(buf, "%" SCNx16, >idVendor);
> +
> +filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idProduct", busid);
> +if (!libxl_read_sysfs_file_contents(ctx, filename, , NULL))
> +sscanf(buf, "%" SCNx16, >idProduct);
> +
> +filename = GCSPRINTF(SYSFS_USB_DEV"/%s/manufacturer", busid);
> +if (!libxl_read_sysfs_file_contents(ctx, filename, ,
> ) &&
> +buflen > 0) {
> +/* replace \n to \0 */
> +if (((char *)buf)[buflen - 1] == '\n')
> +((char *)buf)[buflen - 1] = '\0';
> +usbinfo->manuf = libxl__strdup(NOGC, buf);
> +   }
> +
> +filename = GCSPRINTF(SYSFS_USB_DEV"/%s/product", busid);
> +if (!libxl_read_sysfs_file_contents(ctx, filename, ,
> ) &&
> +buflen > 0) {
> +/* replace \n to \0 */
> +if (((char *)buf)[buflen - 1] == '\n')
> +((char *)buf)[buflen - 1] = '\0';
> +usbinfo->prod = libxl__strdup(NOGC, buf);
> +}
   Basically, starting here, we have information which won't be
 available
 if we're using a pvusb driver domain.
   This information is nice-to-have, but I don't think this
 information is
 directly relevant to libxl or xl; the funcitonality to get this
 information is available from other libraries like libusb.  I'm
 inclined
 to say that if we want to have pvusb driver domains (and I think we
 do),
 we should just get rid of this information.
>>> For command 'xl usb-list', those information should be reported to user.
>>> Do you mean we could call libusb to get thoes information directly in
>>> xl toolstack and get rid of this function?
>>>
>>> I think we can keep the function, since every other device type has the
>>> function XXX_getinfo. But here we could check backend_domid, for
>>> backend=dom0, doing above work; for backend!=dom0 (e.g. pvusb
>>> driver domain, no matter how, it should also be able to let users
>>> getting
>>> those information. Can add code  in future.)
>> Does xl need that information?  Can't the user get that information from
>> lsusb?
>>
>> In any case, I can see why it would be *useful* to have in xl.  But
>> about having it in libxl, I think this question sort of goes along with
>> the question about the next patch -- whether libxl should be in the
>> business of providing general information about the USB devices it's
>> handling, or whether it should limit itself to doing what is absolutely
>> necessary to talk to usbback.
>>
>> There's a part of me that sees the point of it: it's not actually that
>> much extra code (at least for Linux), and it makes it easy to add some
>> very useful features to xl.
>>
>> On the other hand, it's not portable to other OSes.  At the moment of
>> course pvusb isn't portable either, but once we have qemu USB (providing
>> either emulated or PV usb) then I *think* most of the other
>> functionality will Just Work on any platform that can compile qemu
>> (incl. FreeBSD, NetBSD, ), won't it?  The code you're introducing here
>> would have to be re-implented for those platforms, and for every new
>> platform that wanted to include this functionality, wouldn't it?
> So, about the portability problem, I think it's back to: do need to
> update code to call libusb instead of reading sysfs now? Except
> for this function, still have places reading sysfs to get hostbus,
> hostaddr, vendorId, deviceId. Those are not portable for other
> platform.

I realize I didn't give you very clear guidance; I guess I was hoping to
get an opinion from the tools maintainers.  Or perhaps, I was hoping to
let them be the "bad guys" and say, "You can't have this feature in
libxl", so I wouldn't have to. :-)

In the absence of guidance to the contrary, I suggest that patch series
should focus on getting the core pvusb functionality in, without the
extra usb-querying bits.  Then we can discuss a further series which
either adds the usb querying functionality to libxl, or implement it in
xl using libusb.

 -George

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

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

[George Dunlap]

On 13/10/15 14:15, George Dunlap wrote:
> On 13/10/15 02:46, Chun Yan Liu wrote:
>>
>>
>> On 10/12/2015 09:46 PM, George Dunlap wrote:
>>> On 12/10/15 08:19, Chun Yan Liu wrote:
>> +
>> +usbinfo->devnum = usb->u.hostdev.hostaddr;
>> +usbinfo->busnum = usb->u.hostdev.hostbus;
>> +
>> +busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
>> + usb->u.hostdev.hostaddr);
>> +if (!busid) {
>> +rc = ERROR_FAIL;
>> +goto out;
>> +}
>> +
>> +filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idVendor", busid);
>> +if (!libxl_read_sysfs_file_contents(ctx, filename, , NULL))
>> +sscanf(buf, "%" SCNx16, >idVendor);
>> +
>> +filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idProduct", busid);
>> +if (!libxl_read_sysfs_file_contents(ctx, filename, , NULL))
>> +sscanf(buf, "%" SCNx16, >idProduct);
>> +
>> +filename = GCSPRINTF(SYSFS_USB_DEV"/%s/manufacturer", busid);
>> +if (!libxl_read_sysfs_file_contents(ctx, filename, ,
>> ) &&
>> +buflen > 0) {
>> +/* replace \n to \0 */
>> +if (((char *)buf)[buflen - 1] == '\n')
>> +((char *)buf)[buflen - 1] = '\0';
>> +usbinfo->manuf = libxl__strdup(NOGC, buf);
>> +   }
>> +
>> +filename = GCSPRINTF(SYSFS_USB_DEV"/%s/product", busid);
>> +if (!libxl_read_sysfs_file_contents(ctx, filename, ,
>> ) &&
>> +buflen > 0) {
>> +/* replace \n to \0 */
>> +if (((char *)buf)[buflen - 1] == '\n')
>> +((char *)buf)[buflen - 1] = '\0';
>> +usbinfo->prod = libxl__strdup(NOGC, buf);
>> +}
>   Basically, starting here, we have information which won't be
> available
> if we're using a pvusb driver domain.
>   This information is nice-to-have, but I don't think this
> information is
> directly relevant to libxl or xl; the funcitonality to get this
> information is available from other libraries like libusb.  I'm
> inclined
> to say that if we want to have pvusb driver domains (and I think we
> do),
> we should just get rid of this information.
 For command 'xl usb-list', those information should be reported to user.
 Do you mean we could call libusb to get thoes information directly in
 xl toolstack and get rid of this function?

 I think we can keep the function, since every other device type has the
 function XXX_getinfo. But here we could check backend_domid, for
 backend=dom0, doing above work; for backend!=dom0 (e.g. pvusb
 driver domain, no matter how, it should also be able to let users
 getting
 those information. Can add code  in future.)
>>> Does xl need that information?  Can't the user get that information from
>>> lsusb?
>>>
>>> In any case, I can see why it would be *useful* to have in xl.  But
>>> about having it in libxl, I think this question sort of goes along with
>>> the question about the next patch -- whether libxl should be in the
>>> business of providing general information about the USB devices it's
>>> handling, or whether it should limit itself to doing what is absolutely
>>> necessary to talk to usbback.
>>>
>>> There's a part of me that sees the point of it: it's not actually that
>>> much extra code (at least for Linux), and it makes it easy to add some
>>> very useful features to xl.
>>>
>>> On the other hand, it's not portable to other OSes.  At the moment of
>>> course pvusb isn't portable either, but once we have qemu USB (providing
>>> either emulated or PV usb) then I *think* most of the other
>>> functionality will Just Work on any platform that can compile qemu
>>> (incl. FreeBSD, NetBSD, ), won't it?  The code you're introducing here
>>> would have to be re-implented for those platforms, and for every new
>>> platform that wanted to include this functionality, wouldn't it?
>> So, about the portability problem, I think it's back to: do need to
>> update code to call libusb instead of reading sysfs now? Except
>> for this function, still have places reading sysfs to get hostbus,
>> hostaddr, vendorId, deviceId. Those are not portable for other
>> platform.
> 
> I realize I didn't give you very clear guidance; I guess I was hoping to
> get an opinion from the tools maintainers.  Or perhaps, I was hoping to
> let them be the "bad guys" and say, "You can't have this feature in
> libxl", so I wouldn't have to. :-)
> 
> In the absence of guidance to the contrary, I suggest that patch series
> should focus on getting the core pvusb functionality in, without the
> extra usb-querying bits.  Then we can discuss a further series which
> either adds the usb querying functionality to libxl, or implement it in
> xl using libusb.

Just to be clear: Reading sysfs to actually implement the core pvusb
protocol is fine, because at the moment there *is* only a 

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

[Ian Campbell]

On Tue, 2015-10-13 at 14:15 +0100, George Dunlap wrote:
> In the absence of guidance to the contrary, I suggest that patch series
> should focus on getting the core pvusb functionality in, without the
> extra usb-querying bits.  Then we can discuss a further series which
> either adds the usb querying functionality to libxl, or implement it in
> xl using libusb.

That would seem sensible to me. Lets get the (hopefully) uncontroversial
core functionality done and then worry about any extras etc, since they
seem to be preventing us from making forward progress at the moment.

Ian.

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

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

[Chun Yan Liu]

>>> On 10/1/2015 at 01:55 AM, in message <560c2204.9030...@citrix.com>, George
Dunlap  wrote: 
> On 25/09/15 03:11, Chunyan Liu wrote: 
> > Add pvusb APIs, including: 
> >  - attach/detach (create/destroy) virtual usb controller. 
> >  - attach/detach usb device 
> >  - list usb controllers and usb devices 
> >  - get information of usb controller and usb device 
> >  - some other helper functions 
> >  
> > Signed-off-by: Chunyan Liu  
> > Signed-off-by: Simon Cao  
>  
> Hey Chunyan!  This looks pretty close to being ready to check in to me. 
>  
> There are four basic comments I have.  I'm keen to get this series in so 
> that we can start doing more collaborative improvement; so I'll give my 
> comments, and then talk about timing and a plan to get this in as 
> quikcly as possible at the bottom. 
>  
>  
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c 
> > index aea4887..1e2c63e 100644 
> > --- a/tools/libxl/libxl.c 
> > +++ b/tools/libxl/libxl.c 
> > @@ -4192,11 +4192,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); \ 
>  
> So this seems to be exactly the same as DEFINE_DEVICE_REMOVE(), except 
> that you call libxl__initiate_device_usbctrl_remove() here rather than 
> libxl__initiate_device_remove(). 
>  
> It seems like it might be better to have a separate patch renaming 
> libxl__initiate_device_remove to libxl__initiate_device_generic_remove 
> (or something like that), and then add a parameter to the definition 
> above making it so that the definitions above pass in "generic". 
>  
> > 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; 
> > +} 
> >  libxl__initiate_device_remove(egc, aodev); 
>  
> I think this would probably be more clear if you did: 
>  
> if(dev->backend_kind == LIBXL__DEVICE_KIND_VUSB) 
>   libxl__initiate_device_usbctl_remove() 
> else 
>   libxl__initiate_device_remove() 
>  
> > @@ -3951,7 +3966,10 @@ static inline void  
> libxl__update_config_vtpm(libxl__gc *gc, 
> >  #define COMPARE_PCI(a, b) ((a)->func == (b)->func &&\ 
> > (a)->bus == (b)->bus &&  \ 
> > (a)->dev == (b)->dev) 
> > - 
> > +#define COMPARE_USB(a, b) ((a)->u.hostdev.hostbus == 
> > (b)->u.hostdev.hostbus && \ 
> > +   (a)->u.hostdev.hostaddr == 
> > (b)->u.hostdev.hostaddr) 
>  
> 

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

[Chun Yan Liu]

>>> On 10/8/2015 at 10:41 PM, in message
<22038.32910.375958.407...@mariner.uk.xensource.com>, Ian Jackson
 wrote: 
> Chunyan Liu writes ("[PATCH V7 3/7] libxl: add pvusb API"): 
> > Add pvusb APIs, including: 
> ... 
>  
> > +/* Utility to read backend xenstore keys */ 
> > +#define READ_BACKEND(tgc, subpath)\ 
> > +libxl__xs_read(tgc, XBT_NULL, GCSPRINTF("%s/" subpath,  
> be_path)) 
> > + 
> > +/* Utility to read frontend xenstore keys */ 
> > +#define READ_FRONTEND(tgc, subpath)   \ 
> > +libxl__xs_read(tgc, XBT_NULL, GCSPRINTF("%s/" subpath,  
> fe_path)) 
> > + 
>  
> Thanks for trying to reduce repetition.  But I think these macros need 
> to be improved.  I'm am not particularly keen on them in this form, 
> for a number of reasons. 
>  
>  * They implicitly rely on variables (be_path and fe_path) being in 
>scope but there is no associated doc comment.  That would be OK if 
>they were macros defined within a single function and #undef'd 
>before the function end. 
>  
>  * Their functionality is not really usb-specific.  If this is a good 
>thing to do they should be in libxl_internal.h. 
>  
>  * They should use libxl__xs_read_checked.  That means they should 
>probably also implicitly assume that rc is in scope, and `goto out' 
>on error.  (There are many calls to libxl__xs_read here to which 
>the same observation applies.) 
>  
>  * I think there is no reason for these to take a `tgc' parameter. 
>All of the call sites pass exactly `gc'.  If these macros are going 
>to make assumptions about what's in their scope, `gc' is a very 
>good candidate (which many existing macros rely on). 
>  
> You can go two routes with this: make much more local macros, and 
> #undef them.  Or formalise these macros and widen their scope to the 
> whole of libxl.  I think the latter would probably be best. 

Sorry for replying late, the mail server were exchanged and had some
issues these days.

For the macro, I think maybe it's better to use local macros within function.
The macro itself doesn't do much work but a libxl__xs_read. Local macro
can have more flexibility, like can adding extra 'goto out' or error handling
or other repeated codes to the macro according to specific function.

> 
>
> Perhaps something like: 
>  
> /* 
>  * const char *READ_SUBPATH(const char *febe_path, const char *subpath); 
>  * 
>  * Expects in scope: 
>  *int rc; 
>  *libxl__gc *gc; 
>  *out: 
>  * 
>  * Reads febe_path/subpath from xenstore.  Does not use a transaction. 
>  * On xenstore errors, sets rc and does `goto out'. 
>  * If the path does not exist, returns NULL. 
>  */ 
> #define READ_SUBPATH(febe_path, subpath) ... 
>  
> What do you think ? 
>  
>  
> > +/* AO operation to add a usb controller. 
> > + * 
> > + * Generally, it does: 
> > + * 1) fill in necessary usb controler information with default value 
> > + * 2) write usb controller frontend/backend info to xenstore, update json 
> > + *config file if necessary. 
> > + * 3) wait for device connection. PVUSB frontend and backend driver will 
> > + *probe xenstore paths and build connection between frontend and  i
> backend. 
> > + */ 
> > +void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid, 
> > +   libxl_device_usbctrl *usbctrl, 
> > +   libxl__ao_device *aodev) 
> > +{ 
> > +STATE_AO_GC(aodev->ao); 
> > +libxl__device *device; 
> > +int rc; 
> > + 
> > +rc = libxl__device_usbctrl_setdefault(gc, domid, usbctrl); 
> > +if (rc < 0) goto out; 
> > + 
> > +if (usbctrl->devid == -1) { 
> > +usbctrl->devid = libxl__device_nextid(gc, domid, "vusb"); 
> > +if (usbctrl->devid < 0) { 
> > +rc = ERROR_FAIL; 
> > +goto out; 
> > +} 
> > +} 
> > + 
> > +if (usbctrl->type != LIBXL_USBCTRL_TYPE_PV) { 
> > +LOG(ERROR, "Unsupported USB controller type"); 
> > +rc = ERROR_FAIL; 
> > +goto out; 
> > +} 
> > + 
> > +rc = libxl__device_usbctrl_add_xenstore(gc, domid, usbctrl, 
> > +aodev->update_json); 
> > +if (rc) goto out; 
> > + 
> > +GCNEW(device); 
> > +rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device); 
> > +if (rc) goto out; 
> > + 
> > +aodev->dev = device; 
> > +aodev->action = LIBXL__DEVICE_ACTION_ADD; 
> > +libxl__wait_device_connection(egc, aodev); 
> > +rc = 0; 
> > + 
> > +out: 
> > +aodev->rc = rc; 
> > +if (rc) aodev->callback(egc, aodev); 
>  
> This is wrong (even though it works with the remaining code as it 
> stands), I'm afraid. 
>  
> The right pattern is to `return 0' after 
> libxl__wait_device_connection, because libxl__wait_device_connection 
> will always make a callback.

It's OK to 'return 0' directly fater 

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

[George Dunlap]

On 12/10/15 08:19, Chun Yan Liu wrote:
>>> + 
>>> +usbinfo->devnum = usb->u.hostdev.hostaddr; 
>>> +usbinfo->busnum = usb->u.hostdev.hostbus; 
>>> + 
>>> +busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus, 
>>> + usb->u.hostdev.hostaddr); 
>>> +if (!busid) { 
>>> +rc = ERROR_FAIL; 
>>> +goto out; 
>>> +} 
>>> + 
>>> +filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idVendor", busid); 
>>> +if (!libxl_read_sysfs_file_contents(ctx, filename, , NULL)) 
>>> +sscanf(buf, "%" SCNx16, >idVendor); 
>>> + 
>>> +filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idProduct", busid); 
>>> +if (!libxl_read_sysfs_file_contents(ctx, filename, , NULL)) 
>>> +sscanf(buf, "%" SCNx16, >idProduct); 
>>> + 
>>> +filename = GCSPRINTF(SYSFS_USB_DEV"/%s/manufacturer", busid); 
>>> +if (!libxl_read_sysfs_file_contents(ctx, filename, , ) && 
>>> +buflen > 0) { 
>>> +/* replace \n to \0 */ 
>>> +if (((char *)buf)[buflen - 1] == '\n') 
>>> +((char *)buf)[buflen - 1] = '\0'; 
>>> +usbinfo->manuf = libxl__strdup(NOGC, buf); 
>>> +   } 
>>> + 
>>> +filename = GCSPRINTF(SYSFS_USB_DEV"/%s/product", busid); 
>>> +if (!libxl_read_sysfs_file_contents(ctx, filename, , ) && 
>>> +buflen > 0) { 
>>> +/* replace \n to \0 */ 
>>> +if (((char *)buf)[buflen - 1] == '\n') 
>>> +((char *)buf)[buflen - 1] = '\0'; 
>>> +usbinfo->prod = libxl__strdup(NOGC, buf); 
>>> +} 
>>  
>> Basically, starting here, we have information which won't be available 
>> if we're using a pvusb driver domain. 
>>  
>> This information is nice-to-have, but I don't think this information is 
>> directly relevant to libxl or xl; the funcitonality to get this 
>> information is available from other libraries like libusb.  I'm inclined 
>> to say that if we want to have pvusb driver domains (and I think we do), 
>> we should just get rid of this information.
> 
> For command 'xl usb-list', those information should be reported to user.
> Do you mean we could call libusb to get thoes information directly in
> xl toolstack and get rid of this function?
> 
> I think we can keep the function, since every other device type has the
> function XXX_getinfo. But here we could check backend_domid, for
> backend=dom0, doing above work; for backend!=dom0 (e.g. pvusb
> driver domain, no matter how, it should also be able to let users getting
> those information. Can add code  in future.)

Does xl need that information?  Can't the user get that information from
lsusb?

In any case, I can see why it would be *useful* to have in xl.  But
about having it in libxl, I think this question sort of goes along with
the question about the next patch -- whether libxl should be in the
business of providing general information about the USB devices it's
handling, or whether it should limit itself to doing what is absolutely
necessary to talk to usbback.

There's a part of me that sees the point of it: it's not actually that
much extra code (at least for Linux), and it makes it easy to add some
very useful features to xl.

On the other hand, it's not portable to other OSes.  At the moment of
course pvusb isn't portable either, but once we have qemu USB (providing
either emulated or PV usb) then I *think* most of the other
functionality will Just Work on any platform that can compile qemu
(incl. FreeBSD, NetBSD, ), won't it?  The code you're introducing here
would have to be re-implented for those platforms, and for every new
platform that wanted to include this functionality, wouldn't it?

The libusb interface does look a bit clunky; it would certainly be more
convenient for developers to use the interface you've provided here.

 -George

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

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

[Chun Yan Liu]

On 10/12/2015 09:46 PM, George Dunlap wrote:

On 12/10/15 08:19, Chun Yan Liu wrote:

+
+usbinfo->devnum = usb->u.hostdev.hostaddr;
+usbinfo->busnum = usb->u.hostdev.hostbus;
+
+busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
+ usb->u.hostdev.hostaddr);
+if (!busid) {
+rc = ERROR_FAIL;
+goto out;
+}
+
+filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idVendor", busid);
+if (!libxl_read_sysfs_file_contents(ctx, filename, , NULL))
+sscanf(buf, "%" SCNx16, >idVendor);
+
+filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idProduct", busid);
+if (!libxl_read_sysfs_file_contents(ctx, filename, , NULL))
+sscanf(buf, "%" SCNx16, >idProduct);
+
+filename = GCSPRINTF(SYSFS_USB_DEV"/%s/manufacturer", busid);
+if (!libxl_read_sysfs_file_contents(ctx, filename, , ) &&
+buflen > 0) {
+/* replace \n to \0 */
+if (((char *)buf)[buflen - 1] == '\n')
+((char *)buf)[buflen - 1] = '\0';
+usbinfo->manuf = libxl__strdup(NOGC, buf);
+   }
+
+filename = GCSPRINTF(SYSFS_USB_DEV"/%s/product", busid);
+if (!libxl_read_sysfs_file_contents(ctx, filename, , ) &&
+buflen > 0) {
+/* replace \n to \0 */
+if (((char *)buf)[buflen - 1] == '\n')
+((char *)buf)[buflen - 1] = '\0';
+usbinfo->prod = libxl__strdup(NOGC, buf);
+}
  
Basically, starting here, we have information which won't be available

if we're using a pvusb driver domain.
  
This information is nice-to-have, but I don't think this information is

directly relevant to libxl or xl; the funcitonality to get this
information is available from other libraries like libusb.  I'm inclined
to say that if we want to have pvusb driver domains (and I think we do),
we should just get rid of this information.

For command 'xl usb-list', those information should be reported to user.
Do you mean we could call libusb to get thoes information directly in
xl toolstack and get rid of this function?

I think we can keep the function, since every other device type has the
function XXX_getinfo. But here we could check backend_domid, for
backend=dom0, doing above work; for backend!=dom0 (e.g. pvusb
driver domain, no matter how, it should also be able to let users getting
those information. Can add code  in future.)

Does xl need that information?  Can't the user get that information from
lsusb?

In any case, I can see why it would be *useful* to have in xl.  But
about having it in libxl, I think this question sort of goes along with
the question about the next patch -- whether libxl should be in the
business of providing general information about the USB devices it's
handling, or whether it should limit itself to doing what is absolutely
necessary to talk to usbback.

There's a part of me that sees the point of it: it's not actually that
much extra code (at least for Linux), and it makes it easy to add some
very useful features to xl.

On the other hand, it's not portable to other OSes.  At the moment of
course pvusb isn't portable either, but once we have qemu USB (providing
either emulated or PV usb) then I *think* most of the other
functionality will Just Work on any platform that can compile qemu
(incl. FreeBSD, NetBSD, ), won't it?  The code you're introducing here
would have to be re-implented for those platforms, and for every new
platform that wanted to include this functionality, wouldn't it?

So, about the portability problem, I think it's back to: do need to
update code to call libusb instead of reading sysfs now? Except
for this function, still have places reading sysfs to get hostbus,
hostaddr, vendorId, deviceId. Those are not portable for other
platform.

And about getting rid of the function, currently it's only needed in
'xl usb-list' and 'xl usb-assignable-list', if not providing general
information, yes, we can remove it.

-Chunyan



The libusb interface does look a bit clunky; it would certainly be more
convenient for developers to use the interface you've provided here.

  -George




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

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

[Chun Yan Liu]

>>> On 10/1/2015 at 01:55 AM, in message <560c2204.9030...@citrix.com>, George
Dunlap  wrote: 
> On 25/09/15 03:11, Chunyan Liu wrote: 
> > Add pvusb APIs, including: 
> >  - attach/detach (create/destroy) virtual usb controller. 
> >  - attach/detach usb device 
> >  - list usb controllers and usb devices 
> >  - get information of usb controller and usb device 
> >  - some other helper functions 
> >  
> > Signed-off-by: Chunyan Liu  
> > Signed-off-by: Simon Cao  
>  
> Hey Chunyan!  This looks pretty close to being ready to check in to me. 
>  
> There are four basic comments I have.  I'm keen to get this series in so 
> that we can start doing more collaborative improvement; so I'll give my 
> comments, and then talk about timing and a plan to get this in as 
> quikcly as possible at the bottom. 
>  
>  
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c 
> > index aea4887..1e2c63e 100644 
> > --- a/tools/libxl/libxl.c 
> > +++ b/tools/libxl/libxl.c 
> > @@ -4192,11 +4192,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); \ 
>  
> So this seems to be exactly the same as DEFINE_DEVICE_REMOVE(), except 
> that you call libxl__initiate_device_usbctrl_remove() here rather than 
> libxl__initiate_device_remove(). 
>  
> It seems like it might be better to have a separate patch renaming 
> libxl__initiate_device_remove to libxl__initiate_device_generic_remove 
> (or something like that), and then add a parameter to the definition 
> above making it so that the definitions above pass in "generic". 
>  
> > 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; 
> > +} 
> >  libxl__initiate_device_remove(egc, aodev); 
>  
> I think this would probably be more clear if you did: 
>  
> if(dev->backend_kind == LIBXL__DEVICE_KIND_VUSB) 
>   libxl__initiate_device_usbctl_remove() 
> else 
>   libxl__initiate_device_remove() 
>  
> > @@ -3951,7 +3966,10 @@ static inline void  
> libxl__update_config_vtpm(libxl__gc *gc, 
> >  #define COMPARE_PCI(a, b) ((a)->func == (b)->func &&\ 
> > (a)->bus == (b)->bus &&  \ 
> > (a)->dev == (b)->dev) 
> > - 
> > +#define COMPARE_USB(a, b) ((a)->u.hostdev.hostbus == 
> > (b)->u.hostdev.hostbus && \ 
> > +   (a)->u.hostdev.hostaddr == 
> > (b)->u.hostdev.hostaddr) 
>  
> 

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

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH V7 3/7] libxl: add pvusb API"):
> Since this is part of a new "controller" abstraction we do in theory have
> the freedom to do things differently, but it seems to me that having
> something as basic as the list operation differ for devices vs. controller
> would do more harm than good even if the controller interface is strictly
> better. IOW I'm willing to be convinced otherwise but right now I'm pretty
> sure how it is above is the preferable extension to our API.

Hrm.  OK, I guess :-/.

> > > +tmp = READ_BACKEND(gc, "num-ports");
> > > +if (!tmp) goto outerr;
> > > +usbctrl->ports = atoi(tmp);
> > 
> > There are 4 nearly identical stanzas here.  I think a more
> > comprehensive would be helpful.  Maybe a global macro READ_SUBPATH_INT
>^MACRO?

Yes.

Ian.

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

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

[Ian Jackson]

Chunyan Liu writes ("[PATCH V7 3/7] libxl: add pvusb API"):
> Add pvusb APIs, including:
...

> +/* Utility to read backend xenstore keys */
> +#define READ_BACKEND(tgc, subpath)\
> +libxl__xs_read(tgc, XBT_NULL, GCSPRINTF("%s/" subpath, be_path))
> +
> +/* Utility to read frontend xenstore keys */
> +#define READ_FRONTEND(tgc, subpath)   \
> +libxl__xs_read(tgc, XBT_NULL, GCSPRINTF("%s/" subpath, fe_path))
> +

Thanks for trying to reduce repetition.  But I think these macros need
to be improved.  I'm am not particularly keen on them in this form,
for a number of reasons.

 * They implicitly rely on variables (be_path and fe_path) being in
   scope but there is no associated doc comment.  That would be OK if
   they were macros defined within a single function and #undef'd
   before the function end.

 * Their functionality is not really usb-specific.  If this is a good
   thing to do they should be in libxl_internal.h.

 * They should use libxl__xs_read_checked.  That means they should
   probably also implicitly assume that rc is in scope, and `goto out'
   on error.  (There are many calls to libxl__xs_read here to which
   the same observation applies.)

 * I think there is no reason for these to take a `tgc' parameter.
   All of the call sites pass exactly `gc'.  If these macros are going
   to make assumptions about what's in their scope, `gc' is a very
   good candidate (which many existing macros rely on).

You can go two routes with this: make much more local macros, and
#undef them.  Or formalise these macros and widen their scope to the
whole of libxl.  I think the latter would probably be best.

Perhaps something like:

/*
 * const char *READ_SUBPATH(const char *febe_path, const char *subpath);
 *
 * Expects in scope:
 *int rc;
 *libxl__gc *gc;
 *out:
 *
 * Reads febe_path/subpath from xenstore.  Does not use a transaction.
 * On xenstore errors, sets rc and does `goto out'.
 * If the path does not exist, returns NULL.
 */
#define READ_SUBPATH(febe_path, subpath) ...

What do you think ?


> +/* AO operation to add a usb controller.
> + *
> + * Generally, it does:
> + * 1) fill in necessary usb controler information with default value
> + * 2) write usb controller frontend/backend info to xenstore, update json
> + *config file if necessary.
> + * 3) wait for device connection. PVUSB frontend and backend driver will
> + *probe xenstore paths and build connection between frontend and backend.
> + */
> +void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
> +   libxl_device_usbctrl *usbctrl,
> +   libxl__ao_device *aodev)
> +{
> +STATE_AO_GC(aodev->ao);
> +libxl__device *device;
> +int rc;
> +
> +rc = libxl__device_usbctrl_setdefault(gc, domid, usbctrl);
> +if (rc < 0) goto out;
> +
> +if (usbctrl->devid == -1) {
> +usbctrl->devid = libxl__device_nextid(gc, domid, "vusb");
> +if (usbctrl->devid < 0) {
> +rc = ERROR_FAIL;
> +goto out;
> +}
> +}
> +
> +if (usbctrl->type != LIBXL_USBCTRL_TYPE_PV) {
> +LOG(ERROR, "Unsupported USB controller type");
> +rc = ERROR_FAIL;
> +goto out;
> +}
> +
> +rc = libxl__device_usbctrl_add_xenstore(gc, domid, usbctrl,
> +aodev->update_json);
> +if (rc) goto out;
> +
> +GCNEW(device);
> +rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
> +if (rc) goto out;
> +
> +aodev->dev = device;
> +aodev->action = LIBXL__DEVICE_ACTION_ADD;
> +libxl__wait_device_connection(egc, aodev);
> +rc = 0;
> +
> +out:
> +aodev->rc = rc;
> +if (rc) aodev->callback(egc, aodev);

This is wrong (even though it works with the remaining code as it
stands), I'm afraid.

The right pattern is to `return 0' after
libxl__wait_device_connection, because libxl__wait_device_connection
will always make a callback.

Also the doc comment for this function should make it clear which of
the fields in aodev must be filled in by the caller.  After you do
that you should make sure that the call site obeys the rules.  (This
is not something I have checked right now because the rules aren't
stated.)

Both of these observations apply to the remove path as well.

> +/* AO function to remove a usb controller.
> + *
> + * Generally, it does:
> + * 1) check if the usb controller exists or not
> + * 2) remove all usb devices under controller
> + * 3) remove usb controller information from xenstore
> + */
> +void libxl__initiate_device_usbctrl_remove(libxl__egc *egc,
> +   libxl__ao_device *aodev)

What happens if this functionality is running to try to remove the
controller, at the same time as another process is trying to remove a
device from the controller, or (worse) add a device to the
controller 

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

[Ian Campbell]

On Thu, 2015-10-08 at 15:41 +0100, Ian Jackson wrote:

> > +libxl_device_usbctrl *
> > +libxl_device_usbctrl_list(libxl_ctx *ctx, uint32_t domid, int *num)
> > +{
> 
> This function should return an rc, and the list should come in an out
> parameter.

For better of worse libxl.h defines the general form of a device API to
include list looking like this (search for "Querying").

If this were for an actual device I'd be adamant that it should follow that
pattern.

Since this is part of a new "controller" abstraction we do in theory have
the freedom to do things differently, but it seems to me that having
something as basic as the list operation differ for devices vs. controller
would do more harm than good even if the controller interface is strictly
better. IOW I'm willing to be convinced otherwise but right now I'm pretty
sure how it is above is the preferable extension to our API.

> > +const char *tmp, *be_path;
> > +const char *fe_path = GCSPRINTF("%s/%s", path, *dir);
> > +
> > +libxl_device_usbctrl_init(usbctrl);
> > +usbctrl->devid = atoi(*dir);
> > +
> > +tmp = READ_FRONTEND(gc, "backend-id");
> > +if (!tmp) goto outerr;
> > +usbctrl->backend_domid = atoi(tmp);
> > +
> > +tmp = READ_BACKEND(gc, "usb-ver");
> > +if (!tmp) goto outerr;
> > +usbctrl->version = atoi(tmp);
> > +
> > +tmp = READ_BACKEND(gc, "num-ports");
> > +if (!tmp) goto outerr;
> > +usbctrl->ports = atoi(tmp);
> 
> There are 4 nearly identical stanzas here.  I think a more
> comprehensive would be helpful.  Maybe a global macro READ_SUBPATH_INT
   ^MACRO?

Ian.

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

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

[Ian Campbell]

On Wed, 2015-09-30 at 18:55 +0100, George Dunlap wrote:

> > +int libxl_devid_to_device_usbctrl(libxl_ctx *ctx,
> > +  uint32_t domid,
> > +  int devid,
> > +  libxl_device_usbctrl *usbctrl)
> > +{
> > +GC_INIT(ctx);
> > +libxl_device_usbctrl *usbctrls;
> > +int nb = 0;
> > +int i, rc = -1;
> > +
> > +usbctrls = libxl_device_usbctrl_list(ctx, domid, );
> > +if (!nb) goto out;
> > +
> > +for (i = 0; i < nb; i++) {
> > +if (devid == usbctrls[i].devid) {
> > +*usbctrl = usbctrls[i];
> 
> libxl maintainers: Is this kind of copying OK?
> 
> The analog functions for vtpm and nic both take very different
> approaches; both call libxl_device_[type]_init() and then fill in
> individual elements (albeit in different ways).

That depends on the memory lifecycle situation of usbctrls[i] and *usbctrl
vis-a-vis the gc and when they are frred.

Cut out of the context here is a 
+libxl_device_usbctrl_list_free(usbctrls, nb);

which is going to free any of the pointers in usbctrls[i] which have been
copied to usbctrl. So in this case no it is not ok.

You can't avoid the libxl_device_usbctrl_list_free, since you don't want to
leak all the other elements on the list, so copying seems to be the way to
go.

The IDL should have generated a copy function which can be used (by the
vtpm one too, but it predates the IDL making such things I think).

> 
> > +/*
> > + * USB add
> > + */
> > +static int do_usb_add(libxl__gc *gc, uint32_t domid,
> > +  libxl_device_usb *usbdev,
> > +  libxl__ao_device *aodev)
> > +{
> > +libxl_ctx *ctx = CTX;
> > +libxl_usbctrlinfo usbctrlinfo;
> > +libxl_device_usbctrl usbctrl;
> > +int rc;
> > +
> > +libxl_usbctrlinfo_init();
> > +usbctrl.devid = usbdev->ctrl;
> > +rc = libxl_device_usbctrl_getinfo(ctx, domid, ,
> > );
> > +if (rc)
> > +goto out;
> > +
> > +switch (usbctrlinfo.type) {
> > +case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
> > +LOG(ERROR, "Not supported");
> > +break;
> > +case LIBXL_USBCTRL_TYPE_PV:
> > +rc = libxl__device_usb_add_xenstore(gc, domid, usbdev,
> > +aodev->update_json);
> > +if (rc) goto out;
> > +
> > +rc = usbback_dev_assign(gc, usbdev);
> 
> This assumes that the usb controller is dom0; but the interface
> explicitly allows pvusb driver domains.
> 
> I think it would be enough here to do this check if the usbctrl device
> is dom0.  We should then assume that a pvusb driver domain will be
> configured with all usb devices assigned to usbback already.
> 
> I assume that there's a feedback mechanisms for backends for situations
> where the requested device can't be made, right?  For example, if you
> have a network driver domain and request a non-existent bridge?  If so,
> I think we can let the same mechanism worth for pvusb backends to say "I
> don't have that device available".

I think the b/e writes an error node in xenstore, which we already pickup
iirc.


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

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

[George Dunlap]

On 25/09/15 03:11, Chunyan Liu wrote:
> Add pvusb APIs, including:
>  - attach/detach (create/destroy) virtual usb controller.
>  - attach/detach usb device
>  - list usb controllers and usb devices
>  - get information of usb controller and usb device
>  - some other helper functions
> 
> Signed-off-by: Chunyan Liu 
> Signed-off-by: Simon Cao 

Hey Chunyan!  This looks pretty close to being ready to check in to me.

There are four basic comments I have.  I'm keen to get this series in so
that we can start doing more collaborative improvement; so I'll give my
comments, and then talk about timing and a plan to get this in as
quikcly as possible at the bottom.


> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index aea4887..1e2c63e 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4192,11 +4192,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); \

So this seems to be exactly the same as DEFINE_DEVICE_REMOVE(), except
that you call libxl__initiate_device_usbctrl_remove() here rather than
libxl__initiate_device_remove().

It seems like it might be better to have a separate patch renaming
libxl__initiate_device_remove to libxl__initiate_device_generic_remove
(or something like that), and then add a parameter to the definition
above making it so that the definitions above pass in "generic".

> 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;
> +}
>  libxl__initiate_device_remove(egc, aodev);

I think this would probably be more clear if you did:

if(dev->backend_kind == LIBXL__DEVICE_KIND_VUSB)
  libxl__initiate_device_usbctl_remove()
else
  libxl__initiate_device_remove()

> @@ -3951,7 +3966,10 @@ static inline void libxl__update_config_vtpm(libxl__gc 
> *gc,
>  #define COMPARE_PCI(a, b) ((a)->func == (b)->func &&\
> (a)->bus == (b)->bus &&  \
> (a)->dev == (b)->dev)
> -
> +#define COMPARE_USB(a, b) ((a)->u.hostdev.hostbus == (b)->u.hostdev.hostbus 
> && \
> +   (a)->u.hostdev.hostaddr == 
> (b)->u.hostdev.hostaddr)

Hmm... COMPARE_USB is used in three places:

1. Used in libxl.c:libxl_retrieve_domain_configuration() to de-duplicate
JSON and xenstore configuration

2. Used in libxl_pvusb.c:libxl__device_usb_add_xentore() to avoid adding
the same device twice

3. Used in libxl_pvusb.c:is_usbdev_in_array() to avoid assigning the
same host device

For #3, we pretty clearly want to be comparing hostbus/hostaddr.  (In
fact, you 

[Xen-devel] [PATCH V7 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 controllers and usb devices
 - get information of usb controller and usb device
 - some other helper functions

Signed-off-by: Chunyan Liu 
Signed-off-by: Simon Cao 
---
changes:
  - Update hostbus/hostaddr/idVendor/idProduct types in libxl_types.idl
  - Add PV/DEVICEMODEL check in libxl API, take George's HVM patch series
for reference.

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

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index c5ecec1..ef9ccd3 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 aea4887..1e2c63e 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4192,11 +4192,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) \
@@ -4228,6 +4271,12 @@ DEFINE_DEVICE_ADD(nic)
 /* vtpm */
 DEFINE_DEVICE_ADD(vtpm)
 
+/* usbctrl */
+DEFINE_DEVICE_ADD(usbctrl)
+
+/* usb */
+DEFINE_DEVICE_ADD(usb)
+
 #undef DEFINE_DEVICE_ADD