From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44186) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TLXD2-0005g5-Na for qemu-devel@nongnu.org; Tue, 09 Oct 2012 06:38:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TLXCy-0006kW-7T for qemu-devel@nongnu.org; Tue, 09 Oct 2012 06:38:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:27467) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TLXCx-0006kS-Tu for qemu-devel@nongnu.org; Tue, 09 Oct 2012 06:38:40 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q99AccAW022819 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 9 Oct 2012 06:38:39 -0400 Message-ID: <5073FF0A.6080304@redhat.com> Date: Tue, 09 Oct 2012 12:40:10 +0200 From: Hans de Goede MIME-Version: 1.0 References: <1349682696-3046-1-git-send-email-hdegoede@redhat.com> <1349682696-3046-5-git-send-email-hdegoede@redhat.com> <5072CC04.4010300@redhat.com> <5072E838.8080104@redhat.com> <5073ECB6.4090408@redhat.com> In-Reply-To: <5073ECB6.4090408@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 04/12] usb: Add support for input pipelining List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: qemu-devel@nongnu.org Hi, On 10/09/2012 11:21 AM, Gerd Hoffmann wrote: >> 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