[Qemu-devel] [PATCH 0/1] ahci: fix ATAPI PIO

[Qemu-devel] [PATCH 0/1] ahci: fix ATAPI PIO

[John Snow]

Fixes ATAPI PIO for transfers of greater than one sector,
and centralizes the fix applied for HDD PIO transfers
all in one place, finally.

________________________________________________________________________________

For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch ide-unify-pio
https://github.com/jnsnow/qemu/tree/ide-unify-pio

This version is tagged ide-unify-pio-v1:
https://github.com/jnsnow/qemu/releases/tag/ide-unify-pio-v1

John Snow (1):
  ide: unify io_buffer_offset increments

 hw/ide/ahci.c     | 22 +++++++---------------
 hw/ide/core.c     |  5 ++---
 hw/ide/internal.h |  1 +
 3 files changed, 10 insertions(+), 18 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/1] ide: unify io_buffer_offset increments

[John Snow]

IDEState's io_buffer_offset was originally added to keep track of offsets
in AHCI rather exclusively, but it was added to IDEState instead of an
AHCI-specific structure.

AHCI fakes all PIO transfers using DMA and a scatter-gather list. When
the core or atapi layers invoke HBA-specific mechanisms for transfers,
they do not always know that it is being backed by DMA or a sglist, so
this offset is not always updated by the HBA code everywhere.

If we modify it in dma_buf_commit, however, any HBA that needs to use
this offset to manage operating on only part of a sglist will have
access to it.

This will fix ATAPI PIO transfers performed through the AHCI HBA,
which were previously not modifying this value appropriately.

This will fix ATAPI PIO transfers larger than one sector.

Reported-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c     | 22 +++++++---------------
 hw/ide/core.c     |  5 ++---
 hw/ide/internal.h |  1 +
 3 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 48749c1..378ad60 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -50,7 +50,6 @@ static void ahci_reset_port(AHCIState *s, int port);
 static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis);
 static void ahci_init_d2h(AHCIDevice *ad);
 static int ahci_dma_prepare_buf(IDEDMA *dma, int32_t limit);
-static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes);
 static bool ahci_map_clb_address(AHCIDevice *ad);
 static bool ahci_map_fis_address(AHCIDevice *ad);
 static void ahci_unmap_clb_address(AHCIDevice *ad);
@@ -1290,7 +1289,7 @@ out:
     s->data_ptr = s->data_end;
 
     /* Update number of transferred bytes, destroy sglist */
-    ahci_commit_buf(dma, size);
+    dma_buf_commit(s, size);
 
     s->end_transfer_func(s);
 
@@ -1332,9 +1331,8 @@ static void ahci_restart(IDEDMA *dma)
 }
 
 /**
- * Called in DMA R/W chains to read the PRDT, utilizing ahci_populate_sglist.
- * Not currently invoked by PIO R/W chains,
- * which invoke ahci_populate_sglist via ahci_start_transfer.
+ * Called in DMA and PIO R/W chains to read the PRDT.
+ * Not shared with NCQ pathways.
  */
 static int32_t ahci_dma_prepare_buf(IDEDMA *dma, int32_t limit)
 {
@@ -1353,21 +1351,16 @@ static int32_t ahci_dma_prepare_buf(IDEDMA *dma, int32_t limit)
 }
 
 /**
- * Destroys the scatter-gather list,
- * and updates the command header with a bytes-read value.
- * called explicitly via ahci_dma_rw_buf (ATAPI DMA),
- * and ahci_start_transfer (PIO R/W),
- * and called via callback from ide_dma_cb for DMA R/W paths.
+ * Updates the command header with a bytes-read value.
+ * Called via dma_buf_commit, for both DMA and PIO paths.
+ * sglist destruction is handled within dma_buf_commit.
  */
 static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
-    IDEState *s = &ad->port.ifs[0];
 
     tx_bytes += le32_to_cpu(ad->cur_cmd->status);
     ad->cur_cmd->status = cpu_to_le32(tx_bytes);
-
-    qemu_sglist_destroy(&s->sg);
 }
 
 static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
@@ -1388,10 +1381,9 @@ static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
     }
 
     /* free sglist, update byte count */
-    ahci_commit_buf(dma, l);
+    dma_buf_commit(s, l);
 
     s->io_buffer_index += l;
-    s->io_buffer_offset += l;
 
     DPRINTF(ad->port_no, "len=%#x\n", l);
 
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 50449ca..8ba04df 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -591,7 +591,6 @@ static void ide_sector_read_cb(void *opaque, int ret)
     s->nsector -= n;
     /* Allow the guest to read the io_buffer */
     ide_transfer_start(s, s->io_buffer, n * BDRV_SECTOR_SIZE, ide_sector_read);
-    s->io_buffer_offset += 512 * n;
     ide_set_irq(s->bus);
 }
 
@@ -635,11 +634,12 @@ static void ide_sector_read(IDEState *s)
                                  ide_sector_read_cb, s);
 }
 
-static void dma_buf_commit(IDEState *s, uint32_t tx_bytes)
+void dma_buf_commit(IDEState *s, uint32_t tx_bytes)
 {
     if (s->bus->dma->ops->commit_buf) {
         s->bus->dma->ops->commit_buf(s->bus->dma, tx_bytes);
     }
+    s->io_buffer_offset += tx_bytes;
     qemu_sglist_destroy(&s->sg);
 }
 
@@ -842,7 +842,6 @@ static void ide_sector_write_cb(void *opaque, int ret)
         n = s->req_nb_sectors;
     }
     s->nsector -= n;
-    s->io_buffer_offset += 512 * n;
 
     ide_set_sector(s, ide_get_sector(s) + n);
     if (s->nsector == 0) {
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 30fdcbc..7288a67 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -536,6 +536,7 @@ int64_t ide_get_sector(IDEState *s);
 void ide_set_sector(IDEState *s, int64_t sector_num);
 
 void ide_start_dma(IDEState *s, BlockCompletionFunc *cb);
+void dma_buf_commit(IDEState *s, uint32_t tx_bytes);
 void ide_dma_error(IDEState *s);
 
 void ide_atapi_cmd_ok(IDEState *s);
-- 
2.4.3

Re: [Qemu-devel] [PATCH 1/1] ide: unify io_buffer_offset increments

[Paolo Bonzini]

On 26/08/2015 01:45, John Snow wrote:
> IDEState's io_buffer_offset was originally added to keep track of offsets
> in AHCI rather exclusively, but it was added to IDEState instead of an
> AHCI-specific structure.

Interesting to see something weird turn into feature. :)

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> AHCI fakes all PIO transfers using DMA and a scatter-gather list. When
> the core or atapi layers invoke HBA-specific mechanisms for transfers,
> they do not always know that it is being backed by DMA or a sglist, so
> this offset is not always updated by the HBA code everywhere.
> 
> If we modify it in dma_buf_commit, however, any HBA that needs to use
> this offset to manage operating on only part of a sglist will have
> access to it.
> 
> This will fix ATAPI PIO transfers performed through the AHCI HBA,
> which were previously not modifying this value appropriately.
> 
> This will fix ATAPI PIO transfers larger than one sector.

Re: [Qemu-devel] [PATCH 1/1] ide: unify io_buffer_offset increments

[John Snow]

On 09/07/2015 12:37 PM, Paolo Bonzini wrote:
> 
> 
> On 26/08/2015 01:45, John Snow wrote:
>> IDEState's io_buffer_offset was originally added to keep track of offsets
>> in AHCI rather exclusively, but it was added to IDEState instead of an
>> AHCI-specific structure.
> 
> Interesting to see something weird turn into feature. :)
> 

There's lots of stuff in places where it doesn't really belong in the
IDE universe, so it takes a while to sift through all the usages and
start to straighten it all out.

> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 

Grazie.

>> AHCI fakes all PIO transfers using DMA and a scatter-gather list. When
>> the core or atapi layers invoke HBA-specific mechanisms for transfers,
>> they do not always know that it is being backed by DMA or a sglist, so
>> this offset is not always updated by the HBA code everywhere.
>>
>> If we modify it in dma_buf_commit, however, any HBA that needs to use
>> this offset to manage operating on only part of a sglist will have
>> access to it.
>>
>> This will fix ATAPI PIO transfers performed through the AHCI HBA,
>> which were previously not modifying this value appropriately.
>>
>> This will fix ATAPI PIO transfers larger than one sector.
> 

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js