[Qemu-devel] [PATCH] hw/ide/ahci: advance IO buffer offset in multi-sector PIO transfer

[Laszlo Ersek <lersek@redhat.com>]

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 <jsnow@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    I spent the better half of the night on this bug, so please be gentle.
    :)

 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)) {
-- 
1.8.3.1