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 <js...@redhat.com>
> Cc: qemu-bl...@nongnu.org
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> ---
>
> 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
which I currently have staged in my IDE tree:
https://github.com/jnsnow/qemu/commits/ide
