From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49882) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZmQgb-00075Z-0Y for qemu-devel@nongnu.org; Wed, 14 Oct 2015 14:22:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZmQga-0002Zd-3d for qemu-devel@nongnu.org; Wed, 14 Oct 2015 14:22:01 -0400 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> From: John Snow Message-ID: <561E9D3F.9030701@redhat.com> Date: Wed, 14 Oct 2015 14:21:51 -0400 MIME-Version: 1.0 In-Reply-To: <561E9C9E.6090407@kamp.de> 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: Peter Lieven , qemu-devel@nongnu.org, qemu-block@nongnu.org Cc: kwolf@redhat.com, stefanha@gmail.com, jcody@redhat.com 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! --js