From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43703) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZkEKi-0001yJ-9h for qemu-devel@nongnu.org; Thu, 08 Oct 2015 12:46:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZkEKT-0004Dh-9Q for qemu-devel@nongnu.org; Thu, 08 Oct 2015 12:46:20 -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> From: John Snow Message-ID: <56169D7F.5010403@redhat.com> Date: Thu, 8 Oct 2015 12:44:47 -0400 MIME-Version: 1.0 In-Reply-To: <56165C60.1010806@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/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.