* [Qemu-devel] [PATCH] hw/ide/ahci: advance IO buffer offset in multi-sector PIO transfer
@ 2015-09-16 2:17 Laszlo Ersek
2015-09-16 14:11 ` John Snow
0 siblings, 1 reply; 3+ messages in thread
From: Laszlo Ersek @ 2015-09-16 2:17 UTC (permalink / raw)
To: qemu-devel; +Cc: John Snow, qemu-block
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/ide/ahci: advance IO buffer offset in multi-sector PIO transfer
2015-09-16 2:17 [Qemu-devel] [PATCH] hw/ide/ahci: advance IO buffer offset in multi-sector PIO transfer Laszlo Ersek
@ 2015-09-16 14:11 ` John Snow
2015-09-16 14:41 ` Laszlo Ersek
0 siblings, 1 reply; 3+ messages in thread
From: John Snow @ 2015-09-16 14:11 UTC (permalink / raw)
To: Laszlo Ersek, qemu-devel; +Cc: qemu-block
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/ide/ahci: advance IO buffer offset in multi-sector PIO transfer
2015-09-16 14:11 ` John Snow
@ 2015-09-16 14:41 ` Laszlo Ersek
0 siblings, 0 replies; 3+ messages in thread
From: Laszlo Ersek @ 2015-09-16 14:41 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: qemu-block
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-09-16 14:41 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-16 2:17 [Qemu-devel] [PATCH] hw/ide/ahci: advance IO buffer offset in multi-sector PIO transfer Laszlo Ersek
2015-09-16 14:11 ` John Snow
2015-09-16 14:41 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).