From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49133) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZmQeE-0006uS-2v for qemu-devel@nongnu.org; Wed, 14 Oct 2015 14:19:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZmQdw-0001q0-IJ for qemu-devel@nongnu.org; Wed, 14 Oct 2015 14:19:20 -0400 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:48162 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZmQdw-0001pS-7u for qemu-devel@nongnu.org; Wed, 14 Oct 2015 14:19:16 -0400 Message-ID: <561E9C9E.6090407@kamp.de> Date: Wed, 14 Oct 2015 20:19:10 +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> In-Reply-To: <56169D7F.5010403@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 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