[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Xen-devel] [Patch V1 3/3] xen: add pvUSB backend
From: |
Stefano Stabellini |
Subject: |
Re: [Qemu-devel] [Xen-devel] [Patch V1 3/3] xen: add pvUSB backend |
Date: |
Wed, 9 Sep 2015 14:13:27 +0100 |
User-agent: |
Alpine 2.02 (DEB 1266 2009-07-14) |
On Wed, 9 Sep 2015, Juergen Gross wrote:
> 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 <address@hidden>
> >
> > 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?
Yes, with xendev->debug.
> 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.
I am saying that "called" is not very informative as a debug message,
you might as well remove it and rearrage the calls to be just:
TS_BUS();
But if you want to trace the call, maybe you could use the trace_*
framework?
- Re: [Qemu-devel] [Patch V1 1/3] xen: introduce dummy system device, (continued)
[Qemu-devel] [Patch V1 3/3] xen: add pvUSB backend, Juergen Gross, 2015/09/03