From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42414) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zn2gr-0004mt-2K for qemu-devel@nongnu.org; Fri, 16 Oct 2015 06:56:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zn2gl-0004GZ-I7 for qemu-devel@nongnu.org; Fri, 16 Oct 2015 06:56:48 -0400 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:35611 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zn2gl-0004Fg-8s for qemu-devel@nongnu.org; Fri, 16 Oct 2015 06:56:43 -0400 Message-ID: <5620D7E5.1090803@kamp.de> Date: Fri, 16 Oct 2015 12:56:37 +0200 From: Peter Lieven MIME-Version: 1.0 References: <1442838328-23117-1-git-send-email-pl@kamp.de> <1442838328-23117-2-git-send-email-pl@kamp.de> <5612E873.1090503@redhat.com> <56138A76.1030804@kamp.de> <56154B71.1060001@redhat.com> <56165C60.1010806@kamp.de> <56169D7F.5010403@redhat.com> <561E9C9E.6090407@kamp.de> <561E9D3F.9030701@redhat.com> In-Reply-To: <561E9D3F.9030701@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/5] ide/atapi: make PIO read requests async List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-devel@nongnu.org, qemu-block@nongnu.org Cc: kwolf@redhat.com, stefanha@gmail.com, jcody@redhat.com Am 14.10.2015 um 20:21 schrieb John Snow: > > On 10/14/2015 02:19 PM, Peter Lieven wrote: >> Am 08.10.2015 um 18:44 schrieb John Snow: >>> On 10/08/2015 08:06 AM, Peter Lieven wrote: >>>> Hi all, >>>> >>>> short summary from my side. The whole thing seems to get complicated, >>>> let me explain why: >>>> >>>> 1) During review I found that the code in ide_atapi_cmd_reply_end can't >>>> work correctly if the >>>> byte_count_limit is not a divider or a multiple of cd_sector_size. The >>>> reason is that as soon >>>> as we load the next sector we start at io_buffer offset 0 overwriting >>>> whatever is left in there >>>> for transfer. We also reset the io_buffer_index to 0 which means if we >>>> continue with the >>>> elementary transfer we always transfer a whole sector (of corrupt data) >>>> regardless if we >>>> are allowed to transfer that much data. Before we consider fixing this I >>>> wonder if it >>>> is legal at all to have an unaligned byte_count_limit. It obviously has >>>> never caused trouble in >>>> practice so maybe its not happening in real life. >>>> >>> I had overlooked that part. Good catch. I do suspect that in practice >>> nobody will be asking for bizarre values. >>> >>> There's no rule against an unaligned byte_count_limit as far as I have >>> read, but suspect nobody would have a reason to use it in practice. >>> >>>> 2) I found that whatever cool optimization I put in to buffer multiple >>>> sectors at once I end >>>> up with code that breaks migration because older versions would either >>>> not fill the io_buffer >>>> as expected or we introduce variables that older versions do not >>>> understand. This will >>>> lead to problems if we migrate in the middle of a transfer. >>>> >>> Ech. This sounds like a bit of a problem. I'll need to think about this >>> one... >>> >>>> 3) My current plan to get this patch to a useful state would be to use >>>> my initial patch and just >>>> change the code to use a sync request if we need to buffer additional >>>> sectors in an elementary >>>> transfer. I found that in real world operating systems the >>>> byte_count_limit seems to be equal to >>>> the cd_sector_size. After all its just a PIO transfer an operating >>>> system will likely switch to DMA >>>> as soon as the kernel ist loaded. >>>> >>>> Thanks, >>>> Peter >>>> >>> It sounds like that might be "good enough" for now, and won't make >>> behavior *worse* than it currently is. You can adjust the test I had >>> checked in to not use a "tricky" value and we can amend support for this >>> later if desired. >> Have you had a chance to look at the series with the "good enough" fix? >> >> Thanks, >> Peter >> > Will do so Friday, thanks! Thank you, Peter