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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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 <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?

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?

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