[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [Patch V1 3/3] xen: add pvUSB backend



On 09/07/2015 07:38 PM, Stefano Stabellini wrote:
On Thu, 3 Sep 2015, Juergen Gross wrote:
Add a backend for para-virtualized USB devices for xen domains.

The backend is using host-libusb to forward USB requests from a
domain via libusb to the real device(s) passed through.

Signed-off-by: Juergen Gross <jgross@xxxxxxxx>

Aside from a few minor comments below, this looks pretty good from a Xen
perspective, however I am not too qualified to review the details of the
pvusb protocol or the way the requests are handled internally in QEMU.

I would be glad if Gerd could take a look at this too.

Juergen, if we commit this, would you be happy to maintain it going
forward?

Sure.

diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
new file mode 100644
index 0000000..2570bd7
--- /dev/null
+++ b/hw/usb/xen-usb.c
...
+#define TR(fmt, args...)                                            \
+    {                                                               \
+        struct timeval tv;                                          \
+                                                                    \
+        gettimeofday(&tv, NULL);                                    \
+        fprintf(stderr, "%8ld.%06ld xen-usb(%s):" fmt, tv.tv_sec,   \
+                tv.tv_usec, __func__, ##args);                      \

Please use xen_be_printf. I am not sure I would go as far as using
gettimeofday, but I can see it could be useful here.

Okay. I'll keep the time information as it has been proved to be really
valuable.

+    }
+#define TR_REQ(fmt, args...) { if (tr_debug & 1) TR(fmt, ##args) }
+#define TR_BUS(fmt, args...) { if (tr_debug & 2) TR(fmt, ##args) }

I would drop these and just use the loglevels of xen_be_printf.

Hmm, something like:

TR     -> level 1
TR_BUS -> level 2
TR_REQ -> level 3

Is it possible to control trace levels per device? If not, trying to
catch an error requiring to trace on request level might be hard as
concurrent qdisk trace entries could affect timing severely.

...
+static void xen_bus_child_detach(USBPort *port, USBDevice *child)
+{
+    TR_BUS("called\n");
+}
+
+static void xen_bus_complete(USBPort *port, USBPacket *packet)
+{
+    TR_REQ("called\n");

Could you please either remove these debug messages or make them more
informative.

Which information are you missing? As long as the TR_* macros aren't
changed they'll trace the function as well. In the context of other
traces written they have been completely valuable.


Juergen


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.