Re: [Xen-devel] [PATCH RFC V2 3/5] libxl: add pvusb API

2015-03-20 Thread Chun Yan Liu 


 On 3/7/2015 at 12:50 AM, in message
caflbxzzfzl2f4qnuwgbpza8v1ffgvvkbgn+gco6ceekvbf6...@mail.gmail.com, George
Dunlap george.dun...@eu.citrix.com wrote: 
 On Mon, Jan 19, 2015 at 8:28 AM, Chunyan Liu cy...@suse.com wrote: 
  diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h 
  index 0a123f1..2e89244 100644 
  --- a/tools/libxl/libxl.h 
  +++ b/tools/libxl/libxl.h 
  
  +int libxl_intf_to_device_usb(libxl_ctx *ctx, uint32_t domid, 
  +char *intf, libxl_device_usb *usb) 
  +LIBXL_EXTERNAL_CALLERS_ONLY; 
  
 I guess this function will go away? 

With using bus:addr instead of sysfs interface, this will be removed.

  
  diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c 
  new file mode 100644 
  index 000..830a846 
  --- /dev/null 
  +++ b/tools/libxl/libxl_usb.c 
  @@ -0,0 +1,1277 @@ 
  
  +static int libxl__usbport_add_xenstore(libxl__gc *gc, 
  +   xs_transaction_t tran, 
  +   uint32_t domid, 
  +   libxl_device_usbctrl *usbctrl) 
  
 Would it be too much to ask to have all the pvusb-specific stuff in a 
 separate file -- say, libxl_pvusb.c or something?  That would make 
 it a lot easier when I add in the HVM USB stuff. 

Sure.

  
 (Just to be clear, I'm asking as a favor -- it's policy that the first 
 mover gets to have it easier, and people who want to come and add 
 something later are the ones who have to do the refactoring.) 
  
  +{ 
  +char *path; 
  +int i; 
  + 
  +path = GCSPRINTF(%s/backend/vusb/%d/%d/port, 
  + libxl__xs_get_dompath(gc, 0), domid, usbctrl-devid); 
  + 
  +libxl__xs_mkdir(gc, tran, path, NULL, 0); 
  + 
  +for (i = 1; i = usbctrl-num_ports; i++) { 
  +if (libxl__xs_write_checked(gc, tran, GCSPRINTF(%s/%d, path, i), 
   
 )) 
  +return ERROR_FAIL; 
  +} 
  + 
  +return 0; 
  +} 
  + 
  +static int libxl__usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid, 
  +   libxl_device_usbctrl *usbctrl) 
  +{ 
  +libxl_ctx *ctx = libxl__gc_owner(gc); 
  +flexarray_t *front; 
  +flexarray_t *back; 
  +libxl__device *device; 
  +xs_transaction_t tran; 
  +int rc = 0; 
  + 
  +GCNEW(device); 
  +rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device); 
  +if (rc) goto out; 
  + 
  +front = flexarray_make(gc, 4, 1); 
  +back = flexarray_make(gc, 12, 1); 
  + 
  +flexarray_append(back, frontend-id); 
  +flexarray_append(back, libxl__sprintf(gc, %d, domid)); 
  +flexarray_append(back, online); 
  +flexarray_append(back, 1); 
  +flexarray_append(back, state); 
  +flexarray_append(back, libxl__sprintf(gc, %d, 1)); 
  +flexarray_append(back, usb-ver); 
  +flexarray_append(back, libxl__sprintf(gc, %d, 
  usbctrl-usb_version)); 
  +flexarray_append(back, num-ports); 
  +flexarray_append(back, libxl__sprintf(gc, %d, usbctrl-num_ports)); 
  
 So how much of this is pvusb-specific, and how much would be shared 
 between DEVICEMODEL?  Because this bit looks specifically like the 
 stuff used to set up the pvusb connection... 

To share with qemu emulated usb controller? I think pvusb backend driver will
probe things under backend/vusb/* and setup connection with pvusb frontend
driver, qemu emulated usb controller should not be placed there at all maybe.

  
  +flexarray_append(back, type); 
  +switch(usbctrl-type) { 
  +case LIBXL_USBCTRL_TYPE_PV:{ 
  +flexarray_append(back, PVUSB); 
  +break; 
  +} 
  +case LIBXL_USBCTRL_TYPE_DEVICEMODEL: { 
  +flexarray_append(back, IOEMU); 
  +break; 
  +} 
  +default: 
  +/* not supported */ 
  +rc = ERROR_FAIL; 
  +goto out; 
  +} 
  
 ...but this looks like it's trying to be shared between PVUSB and 
 DEVICEMODEL.  I'm pretty sure this isn't going to work long-run, 
 because if we were to write all this stuff for a devicemodel, wouldn't 
 the pvusb back-end take this as setting up a new pvusb port? 

Yes, I think you are right. This place should only allow pvusb type.
Qemu emulated usb controller should not be stored here.

  
 Also, you don't seem to be storing or retreiving usbctrl-name here -- 
 if we want that to be part of the interface we need to use it.  (I 
 think we wanted that so that it could default to something like pv-0, 
 emul-1, c). 

First I wonder usbctrl-name is really needed. If just for interface, we can
use devid (index), it could be 0, 1, and with usb-list one knows info like:
0 is pv, 1 is emulated. But if it helps a lot with a 'name', surely we can
add :)

  
 In general, I don't think libxl should be storing stuff in the pvusb 
 front/back xenstore directories not related to that protocol.  We 
 should store extraneous information in a libxl-specific directory. 
 You can see

Re: [Xen-devel] [PATCH RFC V2 3/5] libxl: add pvusb API

2015-03-17 Thread Juergen Gross 

Hi Chunyan,

I've found another problem while trying to write a qemu based pvUSB
backend.

On 01/19/2015 09:28 AM, Chunyan Liu wrote:

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

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


...


diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
new file mode 100644
index 000..830a846
--- /dev/null
+++ b/tools/libxl/libxl_usb.c


...


+/* xenstore usb data */
+static int libxl__device_usb_add_xenstore(libxl__gc *gc, uint32_t domid,
+  libxl_device_usb *usb)
+{
+libxl_ctx *ctx = CTX;
+char *be_path;
+int rc;
+libxl_domain_config d_config;
+libxl_device_usb usb_saved;
+libxl__domain_userdata_lock *lock = NULL;
+
+libxl_domain_config_init(d_config);
+libxl_device_usb_init(usb_saved);
+libxl_device_usb_copy(CTX, usb_saved, usb);
+
+be_path = libxl__sprintf(gc, %s/backend/vusb/%d/%d,
+libxl__xs_get_dompath(gc, 0), domid, usb-ctrl);
+if (libxl__wait_for_backend(gc, be_path, 4)  0) {


Don't do this! That's the reason I had to change my backend driver in
order to support assignment of a usb device via config file. Normally
the backend will witch to state 4 only after the frontend is started.

You can just remove waiting for the backend here. The backend has to
check all ports when it is changing is state to 4 (connected).


+rc = ERROR_FAIL;
+goto out;
+}
+
+lock = libxl__lock_domain_userdata(gc, domid);
+if (!lock) {
+rc = ERROR_LOCK_FAIL;
+goto out;
+}
+
+rc = libxl__get_domain_configuration(gc, domid, d_config);
+if (rc) goto out;
+
+DEVICE_ADD(usb, usbs, domid, usb_saved, COMPARE_USB, d_config);
+
+rc = libxl__set_domain_configuration(gc, domid, d_config);
+if (rc) goto out;
+
+be_path = libxl__sprintf(gc, %s/port/%d, be_path, usb-port);
+LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, Adding new usb device to xenstore);
+if (libxl__xs_write_checked(gc, XBT_NULL, be_path, usb-intf)) {
+rc = ERROR_FAIL;
+goto out;
+}
+
+rc = 0;
+
+out:
+if (lock) libxl__unlock_domain_userdata(lock);
+libxl_device_usb_dispose(usb_saved);
+libxl_domain_config_dispose(d_config);
+return rc;
+
+}
+
+static int libxl__device_usb_remove_xenstore(libxl__gc *gc, uint32_t domid,
+ libxl_device_usb *usb)
+{
+libxl_ctx *ctx = CTX;
+char *be_path;
+
+be_path = libxl__sprintf(gc, %s/backend/vusb/%d/%d,
+libxl__xs_get_dompath(gc, 0), domid, usb-ctrl);
+if (libxl__wait_for_backend(gc, be_path, 4)  0)


Remove this one, too.

Juergen

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

Re: [Xen-devel] [PATCH RFC V2 3/5] libxl: add pvusb API

2015-03-09 Thread Ian Campbell 
On Mon, 2015-03-09 at 10:17 +, George Dunlap wrote:
 On 03/09/2015 09:39 AM, Ian Campbell wrote:
  On Fri, 2015-03-06 at 16:50 +, George Dunlap wrote:
  +libxl_device_usb *
  +libxl_device_usb_assignable_list(libxl_ctx *ctx, int *num)
 
  I'm a bit ambivalent about this one.  For people using xl, lsusb
  should be just fine.  Is anyone building a toolstack on top of libxl
  directly going to need this functionality?  Would libvirt use this
  functionality, for instance, or would it use libusb?  Or just leave
  that to the caller?
  
  Do USB devices not need to be bound to some sort of placeholder driver
  (a la xen-pciback) or at least unbound from their actual driver before
  they can be assigned to a guest?
  
  If so then I would expect a function such as this to list the subset of
  devices which have been correctly configured for USB passthrough, which
  would be a subset of the output of lsusb I think.
 
 Are you saying you think this is what the function *should* do, or are
 you saying that you think it should be renamed because that's what the
 name would lead you to expect it did?

I was saying that's what I would have expected, but also asking how USB
passthru worked because I didn't know.

 This code, following xend's lead, seems to do the plugging stuff
 automatically; similar to the seize=1 flag I added for PCI devices.  I
 think this is probably the best interface here; the reason it's not
 enabled by default in the PCI case is that it's so dangerous (you might
 yank out your own hard disk by accident).
 
 Given this auto-seize is what xend did, and what qemu does (and what
 VirtualBox does, and what presumably every other virtualization-related
 software does), I think adding the make assignable step would be
 unnecessary and confusing.

I agree, given non-optional auto-seize I'm not sure having a function to
enumerate the USB devices is all that useful, unless we happen to need
it internally and it is convenient to expose it.

However, it might be the sort of thing we could put in libxlutil, since
it is the sort of thing every toolstack would have to write for itself
otherwise.

Ian.


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

Re: [Xen-devel] [PATCH RFC V2 3/5] libxl: add pvusb API

2015-03-06 Thread George Dunlap 
On Mon, Jan 19, 2015 at 8:28 AM, Chunyan Liu cy...@suse.com wrote:
 diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
 index 0a123f1..2e89244 100644
 --- a/tools/libxl/libxl.h
 +++ b/tools/libxl/libxl.h

 +int libxl_intf_to_device_usb(libxl_ctx *ctx, uint32_t domid,
 +char *intf, libxl_device_usb *usb)
 +LIBXL_EXTERNAL_CALLERS_ONLY;

I guess this function will go away?

 diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
 new file mode 100644
 index 000..830a846
 --- /dev/null
 +++ b/tools/libxl/libxl_usb.c
 @@ -0,0 +1,1277 @@

 +static int libxl__usbport_add_xenstore(libxl__gc *gc,
 +   xs_transaction_t tran,
 +   uint32_t domid,
 +   libxl_device_usbctrl *usbctrl)

Would it be too much to ask to have all the pvusb-specific stuff in a
separate file -- say, libxl_pvusb.c or something?  That would make
it a lot easier when I add in the HVM USB stuff.

(Just to be clear, I'm asking as a favor -- it's policy that the first
mover gets to have it easier, and people who want to come and add
something later are the ones who have to do the refactoring.)

 +{
 +char *path;
 +int i;
 +
 +path = GCSPRINTF(%s/backend/vusb/%d/%d/port,
 + libxl__xs_get_dompath(gc, 0), domid, usbctrl-devid);
 +
 +libxl__xs_mkdir(gc, tran, path, NULL, 0);
 +
 +for (i = 1; i = usbctrl-num_ports; i++) {
 +if (libxl__xs_write_checked(gc, tran, GCSPRINTF(%s/%d, path, i), 
 ))
 +return ERROR_FAIL;
 +}
 +
 +return 0;
 +}
 +
 +static int libxl__usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
 +   libxl_device_usbctrl *usbctrl)
 +{
 +libxl_ctx *ctx = libxl__gc_owner(gc);
 +flexarray_t *front;
 +flexarray_t *back;
 +libxl__device *device;
 +xs_transaction_t tran;
 +int rc = 0;
 +
 +GCNEW(device);
 +rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
 +if (rc) goto out;
 +
 +front = flexarray_make(gc, 4, 1);
 +back = flexarray_make(gc, 12, 1);
 +
 +flexarray_append(back, frontend-id);
 +flexarray_append(back, libxl__sprintf(gc, %d, domid));
 +flexarray_append(back, online);
 +flexarray_append(back, 1);
 +flexarray_append(back, state);
 +flexarray_append(back, libxl__sprintf(gc, %d, 1));
 +flexarray_append(back, usb-ver);
 +flexarray_append(back, libxl__sprintf(gc, %d, usbctrl-usb_version));
 +flexarray_append(back, num-ports);
 +flexarray_append(back, libxl__sprintf(gc, %d, usbctrl-num_ports));

So how much of this is pvusb-specific, and how much would be shared
between DEVICEMODEL?  Because this bit looks specifically like the
stuff used to set up the pvusb connection...

 +flexarray_append(back, type);
 +switch(usbctrl-type) {
 +case LIBXL_USBCTRL_TYPE_PV:{
 +flexarray_append(back, PVUSB);
 +break;
 +}
 +case LIBXL_USBCTRL_TYPE_DEVICEMODEL: {
 +flexarray_append(back, IOEMU);
 +break;
 +}
 +default:
 +/* not supported */
 +rc = ERROR_FAIL;
 +goto out;
 +}

...but this looks like it's trying to be shared between PVUSB and
DEVICEMODEL.  I'm pretty sure this isn't going to work long-run,
because if we were to write all this stuff for a devicemodel, wouldn't
the pvusb back-end take this as setting up a new pvusb port?

Also, you don't seem to be storing or retreiving usbctrl-name here --
if we want that to be part of the interface we need to use it.  (I
think we wanted that so that it could default to something like pv-0,
emul-1, c).

In general, I don't think libxl should be storing stuff in the pvusb
front/back xenstore directories not related to that protocol.  We
should store extraneous information in a libxl-specific directory.
You can see an example of the kind of think I'm talking about in the
HVM USB patch I submitted last year (see usb_add_xenstore()):

http://lists.xen.org/archives/html/xen-devel/2014-06/msg00086.html

Alternately -- at the moment, the only extraneous information we've
got is the name of the controller; if you wanted to propose that we
get rid of the name field, then there wouldn't be any extra
information to store.

 +
 +flexarray_append(front, backend-id);
 +flexarray_append(front, libxl__sprintf(gc, %d, 
 usbctrl-backend_domid));
 +flexarray_append(front, state);
 +flexarray_append(front, libxl__sprintf(gc, %d, 1));
 +
 +retry_transaction:
 +tran = xs_transaction_start(ctx-xsh);
 +
 +libxl__device_generic_add(gc, tran, device,
 +  libxl__xs_kvs_of_flexarray(gc, back, 
 back-count),
 +  libxl__xs_kvs_of_flexarray(gc, front, 
 front-count),
 +  NULL);
 +libxl__usbport_add_xenstore(gc, tran, domid, usbctrl);
 +
 +if (!xs_transaction_end(ctx-xsh,

Re: [Xen-devel] [PATCH RFC V2 3/5] libxl: add pvusb API

2015-03-03 Thread Chun Yan Liu 


 On 3/3/2015 at 07:38 PM, in message 1425382696.24959.112.ca...@citrix.com,
Ian Campbell ian.campb...@citrix.com wrote: 
 On Mon, 2015-01-19 at 16:28 +0800, Chunyan Liu wrote: 
  Add pvusb APIs, including: 
   - attach/detach (create/destroy) virtual usb controller. 
   - attach/detach usb device 
   - list assignable usb devices in host 
   - some other helper functions 
   
  Signed-off-by: Chunyan Liu cy...@suse.com 
  Signed-off-by: Simon Cao caobosi...@gmail.com 
  --- 
   tools/libxl/Makefile |2 +- 
   tools/libxl/libxl.c  |2 + 
   tools/libxl/libxl.h  |   58 ++ 
   tools/libxl/libxl_internal.h |6 + 
   tools/libxl/libxl_usb.c  | 1277  
 ++ 
   tools/libxl/libxlu_cfg_y.c   |  464 --- 
   tools/libxl/libxlu_cfg_y.h   |   38 +- 
   7 files changed, 1623 insertions(+), 224 deletions(-) 
   create mode 100644 tools/libxl/libxl_usb.c 
   
  diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile 
  index b417372..08cdb12 100644 
  --- a/tools/libxl/Makefile 
  +++ b/tools/libxl/Makefile 
  @@ -95,7 +95,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o  
 libxl_dm.o libxl_pci.o \ 
  libxl_internal.o libxl_utils.o libxl_uuid.o \ 
  libxl_json.o libxl_aoutils.o libxl_numa.o \ 
  libxl_save_callout.o _libxl_save_msgs_callout.o \ 
  -   libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y) 
  +   libxl_qmp.o libxl_event.o libxl_fork.o libxl_usb.o 
  $(LIBXL_OBJS-y) 
  
 The contents of that file looks Linux specific, so I think you might 
 have to arrange for it to only be built there and also to provide a 
 libxl_nousb.c with stubs returning appropriate errors to be used on 
 other platforms. 
  
 Or it may be possible (even better) to refactor the linux specific bits 
 of libxl_usb.c into libxl_linux.c and leave the common stuff behind. 
  
 I thought libxl_pci.c would be a good example of this, but it doesn't 
 seem to have any conditional stuff, yet I expected it to be Linux 
 specific. I've no idea how that works :-(. Maybe usb can get away with 
 it too. 
  
  +int libxl_intf_to_device_usb(libxl_ctx *ctx, uint32_t domid, 
  +char *intf, libxl_device_usb *usb) 
  +LIBXL_EXTERNAL_CALLERS_ONLY; 
  
 I wasn't sure what an intf was on patch #1. hopefully your answer 
 there will help me understand what this is for! 

As in patch#1, it means syfs interface for the usb device, like: 2-1.6.
This function will be used when detaching a usb device, use indicates
2-1.6, it will return the corresponding libxl_device_usb structure.

  
  diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c 
  new file mode 100644 
  index 000..830a846 
  --- /dev/null 
  +++ b/tools/libxl/libxl_usb.c 
  
 Apart from my not yet understanding the interface semantics and the 
 potential for Linux-specificness mentioned above the actual code here 
 looks reasonably OK to me. I have smaller and larger comments below 
 though. 
  
  +static int libxl__device_usbctrl_setdefault(libxl__gc *gc, uint32_t domid, 
  +libxl_device_usbctrl *usbctrl) 
  +{ 
 [...] 
  +if(!usbctrl-backend_domid) 
  
 Missing space before (. 
  
  
  +static int libxl__usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid, 
  +   libxl_device_usbctrl *usbctrl) 
  +{ 
  +libxl_ctx *ctx = libxl__gc_owner(gc); 
  +flexarray_t *front; 
  +flexarray_t *back; 
  +libxl__device *device; 
  +xs_transaction_t tran; 
  +int rc = 0; 
  + 
  +GCNEW(device); 
  +rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device); 
  +if (rc) goto out; 
  + 
  +front = flexarray_make(gc, 4, 1); 
  +back = flexarray_make(gc, 12, 1); 
  + 
  +flexarray_append(back, frontend-id); 
  +flexarray_append(back, libxl__sprintf(gc, %d, domid)); 
  
 GCSPRINTF would be good for all of these (and in lots of other places 
 too).
  
 flexarray_append_pair is also nice for adding key+value at the same time 
 since it makes it easier to see what goes together. 

Got it. Will update all in next version.

  
  +retry_transaction: 
  +tran = xs_transaction_start(ctx-xsh); 
  
 Please follow the design pattern in e.g. libxl__device_vtpm_add or 
 device_disk_add and use libxl__xs_transaction_start/commit/abort here 
 inside a for (;;). 

OK. Got it.

  
  +static int libxl__device_usbctrl_add(libxl__gc *gc, uint32_t domid, 
  + libxl_device_usbctrl *usbctrl) 
  +{ 
  [...] 
  +if (libxl__usbctrl_add_xenstore(gc, domid, usbctrl)  0){ 
  
 Space before { please. 
  
  +static int 
  +libxl__device_usbctrl_remove_common(libxl_ctx *ctx, uint32_t domid, 
  +libxl_device_usbctrl *usbctrl, 
  +const

Re: [Xen-devel] [PATCH RFC V2 3/5] libxl: add pvusb API

2015-03-03 Thread Ian Campbell 
On Mon, 2015-01-19 at 16:28 +0800, Chunyan Liu wrote:
 Add pvusb APIs, including:
  - attach/detach (create/destroy) virtual usb controller.
  - attach/detach usb device
  - list assignable usb devices in host
  - some other helper functions
 
 Signed-off-by: Chunyan Liu cy...@suse.com
 Signed-off-by: Simon Cao caobosi...@gmail.com
 ---
  tools/libxl/Makefile |2 +-
  tools/libxl/libxl.c  |2 +
  tools/libxl/libxl.h  |   58 ++
  tools/libxl/libxl_internal.h |6 +
  tools/libxl/libxl_usb.c  | 1277 
 ++
  tools/libxl/libxlu_cfg_y.c   |  464 ---
  tools/libxl/libxlu_cfg_y.h   |   38 +-
  7 files changed, 1623 insertions(+), 224 deletions(-)
  create mode 100644 tools/libxl/libxl_usb.c
 
 diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
 index b417372..08cdb12 100644
 --- a/tools/libxl/Makefile
 +++ b/tools/libxl/Makefile
 @@ -95,7 +95,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o 
 libxl_pci.o \
   libxl_internal.o libxl_utils.o libxl_uuid.o \
   libxl_json.o libxl_aoutils.o libxl_numa.o \
   libxl_save_callout.o _libxl_save_msgs_callout.o \
 - libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y)
 + libxl_qmp.o libxl_event.o libxl_fork.o libxl_usb.o 
 $(LIBXL_OBJS-y)

The contents of that file looks Linux specific, so I think you might
have to arrange for it to only be built there and also to provide a
libxl_nousb.c with stubs returning appropriate errors to be used on
other platforms.

Or it may be possible (even better) to refactor the linux specific bits
of libxl_usb.c into libxl_linux.c and leave the common stuff behind.

I thought libxl_pci.c would be a good example of this, but it doesn't
seem to have any conditional stuff, yet I expected it to be Linux
specific. I've no idea how that works :-(. Maybe usb can get away with
it too.

 +int libxl_intf_to_device_usb(libxl_ctx *ctx, uint32_t domid,
 +char *intf, libxl_device_usb *usb)
 +LIBXL_EXTERNAL_CALLERS_ONLY;

I wasn't sure what an intf was on patch #1. hopefully your answer
there will help me understand what this is for!

 diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
 new file mode 100644
 index 000..830a846
 --- /dev/null
 +++ b/tools/libxl/libxl_usb.c

Apart from my not yet understanding the interface semantics and the
potential for Linux-specificness mentioned above the actual code here
looks reasonably OK to me. I have smaller and larger comments below
though.

 +static int libxl__device_usbctrl_setdefault(libxl__gc *gc, uint32_t domid,
 +libxl_device_usbctrl *usbctrl)
 +{
[...]
 +if(!usbctrl-backend_domid)

Missing space before (.


 +static int libxl__usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
 +   libxl_device_usbctrl *usbctrl)
 +{
 +libxl_ctx *ctx = libxl__gc_owner(gc);
 +flexarray_t *front;
 +flexarray_t *back;
 +libxl__device *device;
 +xs_transaction_t tran;
 +int rc = 0;
 +
 +GCNEW(device);
 +rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
 +if (rc) goto out;
 +
 +front = flexarray_make(gc, 4, 1);
 +back = flexarray_make(gc, 12, 1);
 +
 +flexarray_append(back, frontend-id);
 +flexarray_append(back, libxl__sprintf(gc, %d, domid));

GCSPRINTF would be good for all of these (and in lots of other places
too).

flexarray_append_pair is also nice for adding key+value at the same time
since it makes it easier to see what goes together.

 +retry_transaction:
 +tran = xs_transaction_start(ctx-xsh);

Please follow the design pattern in e.g. libxl__device_vtpm_add or
device_disk_add and use libxl__xs_transaction_start/commit/abort here
inside a for (;;).

 +static int libxl__device_usbctrl_add(libxl__gc *gc, uint32_t domid,
 + libxl_device_usbctrl *usbctrl)
 +{
 [...]
 +if (libxl__usbctrl_add_xenstore(gc, domid, usbctrl)  0){

Space before { please.

 +static int
 +libxl__device_usbctrl_remove_common(libxl_ctx *ctx, uint32_t domid,
 +libxl_device_usbctrl *usbctrl,
 +const libxl_asyncop_how *ao_how,
 +int force)
 +{
 [...]
 +for (i = 0; i  numusb; i++) {
 +if (libxl__device_usb_remove_common(gc, domid, usbs[i], 0)) {
 +fprintf(stderr, libxl_device_usb_remove failed.\n);

Use LOG*( please, not fprintf (this is true everywhere in libxl in case
I missed any other).

 +/* usb device functions */
 +
 +/* Following functions are to get assignable usb devices */
 +static int
 +libxl__device_usb_assigned_list(libxl__gc *gc,
 +libxl_device_usb **list, int *num)
 +{
 +char **domlist;
 +unsigned int nd =

Re: [Xen-devel] [PATCH RFC V2 3/5] libxl: add pvusb API

2015-02-10 Thread Jürgen Groß 

Just found other issues:

On 01/19/2015 09:28 AM, Chunyan Liu wrote:

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

Signed-off-by: Chunyan Liu cy...@suse.com
Signed-off-by: Simon Cao caobosi...@gmail.com
---
  tools/libxl/Makefile |2 +-
  tools/libxl/libxl.c  |2 +
  tools/libxl/libxl.h  |   58 ++
  tools/libxl/libxl_internal.h |6 +
  tools/libxl/libxl_usb.c  | 1277 ++
  tools/libxl/libxlu_cfg_y.c   |  464 ---
  tools/libxl/libxlu_cfg_y.h   |   38 +-
  7 files changed, 1623 insertions(+), 224 deletions(-)
  create mode 100644 tools/libxl/libxl_usb.c


...

+/*Get usb device information */
+static int get_usb_devnum (libxl__gc *gc, const char *intf, char *buf)
+{
+char *path;
+int rc = 0;
+FILE *fd;
+
+path = libxl__sprintf(gc, cat SYSFS_USB_DEVS_PATH/%s/devnum, intf);
+fd = popen(path, r);
+if (fgets(buf, 512, fd) == NULL || ferror(fd))
+rc = -1;
+pclose(fd);
+
+return rc;
+}
+
+static int get_usb_busnum(libxl__gc *gc, const char *intf, char *buf)
+{
+char *path;
+int rc = 0;
+FILE *fd;
+
+path = libxl__sprintf(gc, cat SYSFS_USB_DEVS_PATH/%s/busnum, intf);
+fd = popen(path, r);
+if (fgets(buf, 512, fd) == NULL || ferror(fd))
+rc = -1;
+pclose(fd);
+
+return rc;
+}
+
+static int get_usb_idVendor(libxl__gc *gc, const char *intf, char *buf)
+{
+char *path;
+int rc = 0;
+FILE *fd;
+
+path = libxl__sprintf(gc, cat SYSFS_USB_DEVS_PATH/%s/idVendor, intf);
+fd = popen(path, r);
+if (fgets(buf, 512, fd) == NULL || ferror(fd))
+rc = -1;
+pclose(fd);
+
+return rc;
+}
+
+static int get_usb_idProduct(libxl__gc *gc, const char *intf, char *buf)
+{
+char *path;
+int rc = 0;
+FILE *fd;
+
+path = libxl__sprintf(gc, cat SYSFS_USB_DEVS_PATH/%s/idProduct, intf);
+fd = popen(path, r);
+if (fgets(buf, 512, fd) == NULL || ferror(fd))
+rc = -1;
+pclose(fd);
+
+return rc;
+}
+
+static int get_usb_manufacturer(libxl__gc *gc, const char *intf, char *buf)
+{
+char *path;
+int rc = 0;
+FILE *fd;
+
+path = libxl__sprintf(gc, cat SYSFS_USB_DEVS_PATH/%s/manufacturer, 
intf);


File does not exist in newer kernels (checked 3.16 and 3.19).


+fd = popen(path, r);
+if (fgets(buf, 512, fd) == NULL || ferror(fd))
+rc = -1;


Another reason not to use popen(): file doesn't exist, but rc = 0.
And buf contains garbage.


+pclose(fd);
+
+return rc;
+}
+
+static int get_usb_product(libxl__gc *gc, const char *intf, char *buf)
+{
+char *path;
+int rc = 0;
+FILE *fd;
+
+path = libxl__sprintf(gc, cat SYSFS_USB_DEVS_PATH/%s/product, intf);


File does not exist in newer kernels (checked 3.16 and 3.19).


+fd = popen(path, r);
+if (fgets(buf, 512, fd) == NULL || ferror(fd))
+rc = -1;
+pclose(fd);
+
+return rc;
+}
+
+int libxl_device_usb_getinfo(libxl_ctx *ctx, char *intf, libxl_usbinfo 
*usbinfo)
+{
+GC_INIT(ctx);
+char buf[512];
+
+if (!get_usb_devnum(gc, intf, buf) )
+usbinfo-devnum = atoi(buf);
+
+if ( !get_usb_busnum(gc, intf, buf))
+usbinfo-bus = atoi(buf);
+
+if (!get_usb_idVendor(gc, intf, buf) )
+usbinfo-idVendor = atoi(buf);


atoi is wrong. idVendor in sysfs is a hex-string.


+
+if (!get_usb_idProduct(gc, intf, buf) )
+usbinfo-idProduct  = atoi(buf);


again hex-string.


+
+if (!get_usb_manufacturer(gc, intf, buf) )
+usbinfo-manuf = strdup(buf);
+
+if (!get_usb_product(gc, intf, buf) )
+usbinfo-prod = strdup(buf);
+
+GC_FREE;
+return 0;
+}



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

Re: [Xen-devel] [PATCH RFC V2 3/5] libxl: add pvusb API

2015-02-10 Thread Jürgen Groß 

On 01/19/2015 09:28 AM, Chunyan Liu wrote:

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

Signed-off-by: Chunyan Liu cy...@suse.com
Signed-off-by: Simon Cao caobosi...@gmail.com
---
  tools/libxl/Makefile |2 +-
  tools/libxl/libxl.c  |2 +
  tools/libxl/libxl.h  |   58 ++
  tools/libxl/libxl_internal.h |6 +
  tools/libxl/libxl_usb.c  | 1277 ++
  tools/libxl/libxlu_cfg_y.c   |  464 ---
  tools/libxl/libxlu_cfg_y.h   |   38 +-
  7 files changed, 1623 insertions(+), 224 deletions(-)
  create mode 100644 tools/libxl/libxl_usb.c

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index b417372..08cdb12 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -95,7 +95,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o 
libxl_pci.o \
libxl_internal.o libxl_utils.o libxl_uuid.o \
libxl_json.o libxl_aoutils.o libxl_numa.o \
libxl_save_callout.o _libxl_save_msgs_callout.o \
-   libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y)
+   libxl_qmp.o libxl_event.o libxl_fork.o libxl_usb.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 3cd13db..dd76ac3 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1594,6 +1594,8 @@ void libxl__destroy_domid(libxl__egc *egc, 
libxl__destroy_domid_state *dis)

  if (libxl__device_pci_destroy_all(gc, domid)  0)
  LIBXL__LOG(ctx, LIBXL__LOG_ERROR, pci shutdown failed for domid %d, 
domid);
+if (libxl__device_usb_destroy_all(gc, domid)  0)
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, usb shutdown failed for domid %d, 
domid);
  rc = xc_domain_pause(ctx-xch, domid);
  if (rc  0) {
  LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc, xc_domain_pause failed for 
%d, domid);
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 0a123f1..2e89244 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -98,6 +98,12 @@
  #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1

  /*
+ * LIBXL_HAVE_DEVICE_USB indicates the functions for doing hot-plug of
+ * USB devices.
+ */
+#define LIBXL_HAVE_DEVICE_USB 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
@@ -1168,6 +1174,56 @@ 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*/
+int libxl_device_usbctrl_add(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usbctrl *usbctrl,
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
+
+int libxl_device_usbctrl_remove(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usbctrl *usbctrl,
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
+
+int libxl_device_usbctrl_destroy(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usbctrl *usbctrl,
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
+
+libxl_device_usbctrl *libxl_device_usbctrl_list(libxl_ctx *ctx,
+uint32_t domid, int *num);
+
+int libxl_devid_to_device_usbctrl(libxl_ctx *ctx, uint32_t domid,
+int devid, libxl_device_usbctrl *usbctrl)
+LIBXL_EXTERNAL_CALLERS_ONLY;
+
+int libxl_device_usbctrl_getinfo(libxl_ctx *ctx, uint32_t domid,
+libxl_device_usbctrl *usbctrl,
+libxl_usbctrlinfo *usbctrlinfo)
+LIBXL_EXTERNAL_CALLERS_ONLY;
+
+/* 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;
+
+int libxl_device_usb_destroy(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 usbctrl, int *num);
+
+int

Re: [Xen-devel] [PATCH RFC V2 3/5] libxl: add pvusb API

2015-01-28 Thread Ian Campbell 
On Mon, 2015-01-19 at 16:28 +0800, Chunyan Liu wrote:
  tools/libxl/libxlu_cfg_y.c   |  464 ---
  tools/libxl/libxlu_cfg_y.h   |   38 +-

I think these are spurious changes caused by you having different
versions of flex/bison installed, could you arrange to omit these
please.



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

Re: [Xen-devel] [PATCH RFC V2 3/5] libxl: add pvusb API

2015-01-28 Thread Chun Yan Liu 


 On 1/28/2015 at 11:54 PM, in message 1422460493.5187.28.ca...@citrix.com, 
 Ian
Campbell ian.campb...@citrix.com wrote: 
 On Mon, 2015-01-19 at 16:28 +0800, Chunyan Liu wrote: 
   tools/libxl/libxlu_cfg_y.c   |  464 --- 
   tools/libxl/libxlu_cfg_y.h   |   38 +- 
  
 I think these are spurious changes caused by you having different 
 versions of flex/bison installed, could you arrange to omit these 
 please. 

Sorry,  will delete.

  
  
  
  



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

[Xen-devel] [PATCH RFC V2 3/5] libxl: add pvusb API

2015-01-19 Thread Chunyan Liu 
Add pvusb APIs, including:
 - attach/detach (create/destroy) virtual usb controller.
 - attach/detach usb device
 - list assignable usb devices in host
 - some other helper functions

Signed-off-by: Chunyan Liu cy...@suse.com
Signed-off-by: Simon Cao caobosi...@gmail.com
---
 tools/libxl/Makefile |2 +-
 tools/libxl/libxl.c  |2 +
 tools/libxl/libxl.h  |   58 ++
 tools/libxl/libxl_internal.h |6 +
 tools/libxl/libxl_usb.c  | 1277 ++
 tools/libxl/libxlu_cfg_y.c   |  464 ---
 tools/libxl/libxlu_cfg_y.h   |   38 +-
 7 files changed, 1623 insertions(+), 224 deletions(-)
 create mode 100644 tools/libxl/libxl_usb.c

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index b417372..08cdb12 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -95,7 +95,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o 
libxl_pci.o \
libxl_internal.o libxl_utils.o libxl_uuid.o \
libxl_json.o libxl_aoutils.o libxl_numa.o \
libxl_save_callout.o _libxl_save_msgs_callout.o \
-   libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y)
+   libxl_qmp.o libxl_event.o libxl_fork.o libxl_usb.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 3cd13db..dd76ac3 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1594,6 +1594,8 @@ void libxl__destroy_domid(libxl__egc *egc, 
libxl__destroy_domid_state *dis)
 
 if (libxl__device_pci_destroy_all(gc, domid)  0)
 LIBXL__LOG(ctx, LIBXL__LOG_ERROR, pci shutdown failed for domid %d, 
domid);
+if (libxl__device_usb_destroy_all(gc, domid)  0)
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, usb shutdown failed for domid %d, 
domid);
 rc = xc_domain_pause(ctx-xch, domid);
 if (rc  0) {
 LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc, xc_domain_pause failed 
for %d, domid);
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 0a123f1..2e89244 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -98,6 +98,12 @@
 #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
 
 /*
+ * LIBXL_HAVE_DEVICE_USB indicates the functions for doing hot-plug of
+ * USB devices.
+ */
+#define LIBXL_HAVE_DEVICE_USB 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
@@ -1168,6 +1174,56 @@ 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*/
+int libxl_device_usbctrl_add(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usbctrl *usbctrl,
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
+
+int libxl_device_usbctrl_remove(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usbctrl *usbctrl,
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
+
+int libxl_device_usbctrl_destroy(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usbctrl *usbctrl,
+ const libxl_asyncop_how *ao_how)
+ LIBXL_EXTERNAL_CALLERS_ONLY;
+
+libxl_device_usbctrl *libxl_device_usbctrl_list(libxl_ctx *ctx,
+uint32_t domid, int *num);
+
+int libxl_devid_to_device_usbctrl(libxl_ctx *ctx, uint32_t domid,
+int devid, libxl_device_usbctrl *usbctrl)
+LIBXL_EXTERNAL_CALLERS_ONLY;
+
+int libxl_device_usbctrl_getinfo(libxl_ctx *ctx, uint32_t domid,
+libxl_device_usbctrl *usbctrl,
+libxl_usbctrlinfo *usbctrlinfo)
+LIBXL_EXTERNAL_CALLERS_ONLY;
+
+/* 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;
+
+int libxl_device_usb_destroy(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 usbctrl, int *num);
+
+int libxl_intf_to_device_usb(libxl_ctx *ctx, uint32_t domid,
+