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

[Juergen Gross <jgross@suse.com>]

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@suse.com>
>
> 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