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

[Laszlo Ersek]

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 <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

For that patch, currently with commit hash
38526a48bb40e3b2a045ca5a9418d1a9bfc2aeb2 in your tree:

Tested-by: Laszlo Ersek <address@hidden>

Thanks!
Laszlo

> which I currently have staged in my IDE tree:
> https://github.com/jnsnow/qemu/commits/ide
>