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

[John Snow]

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 <address@hidden>
> Cc: address@hidden
> Signed-off-by: Laszlo Ersek <address@hidden>
> ---
> 
> 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