diff mbox

hw/ide/ahci: advance IO buffer offset in multi-sector PIO transfer

Message ID 1442369846-31890-1-git-send-email-lersek@redhat.com
State New
Headers show

Commit Message

Laszlo Ersek Sept. 16, 2015, 2:17 a.m. UTC 
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(+)

Comments

John Snow Sept. 16, 2015, 2:11 p.m. UTC | #1 
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 <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.
>     :)
> 

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
Laszlo Ersek Sept. 16, 2015, 2:41 p.m. UTC | #2 
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 <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.
>>     :)
>>
> 
> 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 <lersek@redhat.com>

Thanks!
Laszlo

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

Patch

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