From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49948) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZcDtb-0005ws-6y for qemu-devel@nongnu.org; Wed, 16 Sep 2015 10:41:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZcDtW-0002IL-Rb for qemu-devel@nongnu.org; Wed, 16 Sep 2015 10:41:15 -0400 References: <1442369846-31890-1-git-send-email-lersek@redhat.com> <55F97890.6080807@redhat.com> From: Laszlo Ersek Message-ID: <55F97F7D.3050801@redhat.com> Date: Wed, 16 Sep 2015 16:41:01 +0200 MIME-Version: 1.0 In-Reply-To: <55F97890.6080807@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] hw/ide/ahci: advance IO buffer offset in multi-sector PIO transfer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-devel@nongnu.org Cc: qemu-block@nongnu.org On 09/16/15 16:11, John Snow wrote: > > > On 09/15/2015 10:17 PM, Laszlo Ersek wrote: >> The "MdeModulePkg/Bus/Ata/AtaAtapiPassThru" driver in edk2 submits a three >> sector long PIO read, when booting off various Fedora installer ISOs in >> UEFI mode. With DEBUG_IDE, DEBUG_IDE_ATAPI, DEBUG_AIO and DEBUG_AHCI >> enabled, plus a >> >> DPRINTF(ad->port_no, "offset=%d\n", offset); >> >> at the beginning of ahci_populate_sglist(), we get the following debug >> output: >> >>> fis: >>> 00:27 80 a0 00 00 fe ff e0 00 00 00 00 00 00 00 00 >>> 10:00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >>> 20:00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >>> 30:00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >>> 40:28 00 00 00 00 38 00 00 03 00 00 00 00 00 00 00 >>> 50:00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >>> 60:00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >>> 70:00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >>> fis: >>> 00:28 00 00 00 00 38 00 00 03 00 00 00 00 00 00 00 >>> ide: CMD=a0 >>> ATAPI limit=0xfffe packet: 28 00 00 00 00 38 00 00 03 00 00 00 >>> read pio: LBA=56 nb_sectors=3 >>> reply: tx_size=6144 elem_tx_size=0 index=2048 >>> byte_count_limit=65534 >>> ahci: ahci_populate_sglist: [0] offset=0 >>> ahci: ahci_dma_prepare_buf: [0] len=0x800 >>> ahci: ahci_start_transfer: [0] reading 2048 bytes on atapi w/ sglist >>> reply: tx_size=4096 elem_tx_size=4096 index=2048 >>> ahci: ahci_populate_sglist: [0] offset=0 >>> ahci: ahci_dma_prepare_buf: [0] len=0x800 >>> ahci: ahci_start_transfer: [0] reading 2048 bytes on atapi w/ sglist >>> reply: tx_size=2048 elem_tx_size=2048 index=2048 >>> ahci: ahci_populate_sglist: [0] offset=0 >>> ahci: ahci_dma_prepare_buf: [0] len=0x800 >>> ahci: ahci_start_transfer: [0] reading 2048 bytes on atapi w/ sglist >>> reply: tx_size=0 elem_tx_size=0 index=2048 >>> ahci: ahci_cmd_done: [0] cmd done >>> [...] >> >> The following functions play recursive ping-pong, because >> ide_atapi_cmd_reply_end() segments the request into individual 2KB >> sectors: >> >> ide_transfer_start() <-----------------------+ >> ahci_start_transfer() via funcptr | >> | >> ahci_dma_prepare_buf() | >> ahci_populate_sglist() | >> | >> dma_buf_read() | >> | >> ahci_commit_buf() | >> | >> ide_atapi_cmd_reply_end() via funcptr | >> ide_transfer_start() ------------------+ >> >> The ahci_populate_sglist() correctly sets up the scatter-gather list for >> dma_buf_read(), based on the Physical Region Descriptors passed in by the >> guest. However, the offset into that scatter-gather list remains constant >> zero as ide_atapi_cmd_reply_end() wades through every sector of the three >> sector long PIO transfer. >> >> The consequence is that the first 2KB of the guest buffer(s), speaking >> "linearizedly", is repeatedly overwritten with the next CD-ROM sector. At >> the end of the transfer, the sector last read is visible in the first 2KB >> of the guest buffer(s), and the rest of the guest buffer(s) remains >> unwritten. >> >> Looking at the DMA request path; especially comparing the context of >> ahci_commit_buf() between its two callers ahci_dma_rw_buf() and >> ahci_start_transfer(), it seems like the latter forgets to advance >> "s->io_buffer_offset". >> >> Adding that increment enables the guest to receive valid data. >> >> Cc: John Snow >> Cc: qemu-block@nongnu.org >> Signed-off-by: Laszlo Ersek >> --- >> >> Notes: >> I spent the better half of the night on this bug, so please be gentle. >> :) >> > > Oh no :( > >> hw/ide/ahci.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c >> index 44f6e27..b975c9f 100644 >> --- a/hw/ide/ahci.c >> +++ b/hw/ide/ahci.c >> @@ -1291,6 +1291,8 @@ out: >> /* Update number of transferred bytes, destroy sglist */ >> ahci_commit_buf(dma, size); >> >> + s->io_buffer_offset += size; >> + >> s->end_transfer_func(s); >> >> if (!(s->status & DRQ_STAT)) { >> > > Whoops, I think this does the same thing as: > [Qemu-devel] [PATCH 1/1] ide: unify io_buffer_offset increments For that patch, currently with commit hash 38526a48bb40e3b2a045ca5a9418d1a9bfc2aeb2 in your tree: Tested-by: Laszlo Ersek Thanks! Laszlo > which I currently have staged in my IDE tree: > https://github.com/jnsnow/qemu/commits/ide >