Hi,
On 10/09/2012 11:21 AM, Gerd Hoffmann wrote:
<snip>
So lets try to split this discussion into multiple questions:
1) Can we agree that re-combining input packets back into their original
transfers as done at the guest device driver level, is useful and given
the serial <-> usb converters case, even necessary to have ?
Given that the USBFS_URB_SHORT_NOT_OK doesn't work there is no way
around that I guess.
Yes, short-not-ok not stopping the queue on a short packet is an
unpleasant surprise, it had all 3 of Alan Stern, Sarah Sharp and me
surprised quite a bit while working on fixing the problem some
people were seeing with redirecting some USB mass storage
devices, when those devices were plugged into an XHCI port.
2) If we agree on 1), what is the right place to do them ?
To be able to properly combine packets into one larger transfer, an
uncombine them again later, some coordination is needed between the
entity doing the combining / uncombining and the hcd code:
2a) The combining entity needs to know when the hcd is done with
queuing up packets for an ep.
Yes. We can add a USBDeviceClass method for that.
Ok, this assumes that the combining / uncombining gets moved down
into the usb-device level code. Which clearly has your preference,
but I don't completely agree with, so lets have that discussion first,
once we've decided what to do there, details like this will likely
sort out themselves.
2b) When uncombining it is possible that less packets are actually
used / filled with data, then were combined into one transfer. When
this happens the hcd code will stop processing the queue after the
short packet, and wait for the guest to restart it. When the guest
restarts the queue it will be modified and the un-used packets will
be gone from it. Since the hcd code keeps its own queue, when
when uncombining the hcd code needs to be told to forget about
the unused packets.
Yes. We can add a new USBPortOps for that, or reuse
USBPortOps->complete with USBPacket->result == USB_RET_NOT_USED.
I had the same thought about using a new result for this, thinking
more about this using a new result code is better, so I'll go go
that.
Note that currently we have the same problem on a halt of the ep
queue already, and currently we expect the hcd to completely empty
the queue on a stall. I'm planning to change this code path over
to also let the core tell the hcd to forget about the packets it
had queued beyond the stall, as this seems something to do at the
core level rather then reproducing it in every hcd emulation
separately.
The core can and should do that for packets it owns (USBPacketState ==
USB_PACKET_QUEUED) because they are not (yet) passed to the USBDevice.
Packets owned by USBDevice (USBPacketState == USB_PACKET_ASYNC) must be
handled by the USBDevice itself.
Getting offtopic a bit here, but this not how we currently handle
things, currently the hcd code cancels packets after a queue halt,
rather then the device-code itself. But this is another discussion,
so lets save this for later.
Given the necessary close interaction between the hcd code and
the combine / uncombine code + not wanting to reproduce the
same code in both host-linux.c and redirect.c I believe that
the usb core is the right place for this.
I think the combining should happen just before submitting the transfer
to usbfs, in usb-host. I'd like usb-host see what is really going on,
and the usb core not hiding this by magically creating jumbo packets.
Note that with my current code linux-host can see it is a combined packet
if it wants to already. But this all really boils down to the next bit:
Likewise I think usbredir should do the combining on the *server* side,
not in the qemu redir code.
I think that moving the combining to the server / usb-redir-host side is
*not* a good idea. There is nothing to be gained by doing so, and a lot to
loose:
1) The usb-redir-host has a lot less info to work with, as it does not have
access to the actual tds which lead to the packets being send. The protocol
can be extended to send what we need to do the combining now, but what if
later on it turns out we need more meta data for some corner case ...
2) Doing the combining at the usb-redir-host leads to more packets being send
and received, increasing the protocol overhead
3) Doing the combining at the usb-redir-host means that instead of a single
packet being received back, multiple will be send back, which may not all
be received in one go, causing multiple vm-exits rather then just one.
4) The combining/uncombining logic will be the same for usb-redir or
linux-host redirection, implementing this all twice and then keeping it
in sync is not a good way to spend our time.
Notice that arguments 2 and 3 could be made for combining output packets
too, but what we do their now is nice and KISS, and the possible speed
improvement is not worth the complication IMHO.
So again lets try to split the discussion in answering multiple questions:
1) I believe it is better, and would greatly prefer to, do the combining
on the qemu side rather then on the usb-redir-host side, do you agree?
2) If you agree with 1, then I assume you agree we will want to share
the combining code between host-linux.c (or host-* for that matter) and
redirect.c ?
3) If you agree with 2, then all we need is a place for the shared logic
to live, we could put it in a new file called input-pipeline.c ?
4) Since you would like to keep the core clean, I'm fine with moving the
calling of the combining-functions into the usb-redir device code
(and the same calls can then later be added to host-linux.c).
Regards,
Hans
