[Qemu-devel] [PATCH 0/6] atapi: change unlimited recursion to while loop

[Qemu-devel] [PATCH 0/6] atapi: change unlimited recursion to while loop

[Paolo Bonzini]

Real hardware doesn't have an unlimited stack, so the unlimited
recursion in the ATAPI code smells a bit.  In fact, the call to
ide_transfer_start easily becomes a tail call with a small change
to the code (patch 5); however, we also need to turn the call back to
ide_atapi_cmd_reply_end into another tail call before turning the
(double) tail recursion into a while loop.

In particular, patch 1 ensures that the call to the end_transfer_func is
the last thing in ide_transfer_start.  To do so, it moves the write of
the PIO Setup FIS before the PIO transfer, which actually makes sense:
the FIS is sent by the device to inform the AHCI about the transfer,
so it cannot come after!  This is the main change from the RFC, and
it simplifies the rest of the series (the RFC had to introduce an
"end_transfer" callback just for writing the PIO Setup FIS).

I tested this manually with READ CD commands sent through sg_raw,
and the existing AHCI tests still pass.

Paolo Bonzini (6):
  ahci: move PIO Setup FIS before transfer, fix it for ATAPI commands
  ide: push end_transfer_func out of start_transfer callback, rename
    callback
  ide: call ide_cmd_done from ide_transfer_stop
  ide: make ide_transfer_stop idempotent
  atapi: call ide_set_irq before ide_transfer_start
  ide: introduce ide_transfer_start_norecurse

 hw/ide/ahci.c             | 30 ++++++++++++--------------
 hw/ide/atapi.c            | 44 +++++++++++++++++++++------------------
 hw/ide/core.c             | 39 +++++++++++++++++-----------------
 hw/ide/trace-events       |  2 +-
 include/hw/ide/internal.h |  4 +++-
 tests/libqos/ahci.c       | 30 ++++++++++++++++++++++----
 tests/libqos/ahci.h       |  2 ++
 7 files changed, 89 insertions(+), 62 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH 1/6] ahci: move PIO Setup FIS before transfer, fix it for ATAPI commands

[Paolo Bonzini]

The PIO Setup FIS is written in the PIO:Entry state, which comes before
the ATA and ATAPI data transfer states.  As a result, the PIO Setup FIS
interrupt is now raised before DMA ends for ATAPI commands, and tests have
to be adjusted.

This is also hinted by the description of the command header in the AHCI
specification, where the "A" bit is described as

    When ‘1’, indicates that a PIO setup FIS shall be sent by the device
    indicating a transfer for the ATAPI command.

and also by the description of the ACMD (ATAPI command region):

    The ATAPI command must be either 12 or 16 bytes in length. The length
    transmitted by the HBA is determined by the PIO setup FIS that is sent
    by the device requesting the ATAPI command.

QEMU, which conflates the "generator" and the "receiver" of the FIS into
one device, always uses ATAPI_PACKET_SIZE, aka 12, for the length.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/ahci.c       | 15 ++++++---------
 tests/libqos/ahci.c | 30 ++++++++++++++++++++++++++----
 tests/libqos/ahci.h |  2 ++
 3 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index e22d7be05f..45ce195fb8 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1291,11 +1291,12 @@ static void ahci_start_transfer(IDEDMA *dma)
     int is_write = opts & AHCI_CMD_WRITE;
     int is_atapi = opts & AHCI_CMD_ATAPI;
     int has_sglist = 0;
+    uint16_t fis_len;
 
     if (is_atapi && !ad->done_atapi_packet) {
         /* already prepopulated iobuffer */
         ad->done_atapi_packet = true;
-        size = 0;
+        fis_len = size;
         goto out;
     }
 
@@ -1315,19 +1316,15 @@ static void ahci_start_transfer(IDEDMA *dma)
         }
     }
 
+    /* Update number of transferred bytes, destroy sglist */
+    dma_buf_commit(s, size);
+    fis_len = le32_to_cpu(ad->cur_cmd->status);
 out:
     /* declare that we processed everything */
     s->data_ptr = s->data_end;
-
-    /* Update number of transferred bytes, destroy sglist */
-    dma_buf_commit(s, size);
+    ahci_write_fis_pio(ad, fis_len);
 
     s->end_transfer_func(s);
-
-    if (!(s->status & DRQ_STAT)) {
-        /* done with PIO send/receive */
-        ahci_write_fis_pio(ad, le32_to_cpu(ad->cur_cmd->status));
-    }
 }
 
 static void ahci_start_dma(IDEDMA *dma, IDEState *s,
diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index bc201d762b..04f33e246c 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -477,6 +477,23 @@ void ahci_port_check_d2h_sanity(AHCIQState *ahci, uint8_t port, uint8_t slot)
     g_free(d2h);
 }
 
+void ahci_port_check_pio_sanity_atapi(AHCIQState *ahci, uint8_t port,
+                                      uint8_t slot)
+{
+    PIOSetupFIS *pio = g_malloc0(0x20);
+
+    /* We cannot check the Status or E_Status registers, because
+     * the status may have again changed between the PIO Setup FIS
+     * and the conclusion of the command with the D2H Register FIS. */
+    qtest_memread(ahci->parent->qts, ahci->port[port].fb + 0x20, pio, 0x20);
+    g_assert_cmphex(pio->fis_type, ==, 0x5f);
+
+    g_assert(le16_to_cpu(pio->tx_count) == 12 ||
+             le16_to_cpu(pio->tx_count) == 16);
+
+    g_free(pio);
+}
+
 void ahci_port_check_pio_sanity(AHCIQState *ahci, uint8_t port,
                                 uint8_t slot, size_t buffsize)
 {
@@ -831,9 +848,9 @@ void ahci_command_enable_atapi_dma(AHCICommand *cmd)
     RegH2DFIS *fis = &(cmd->fis);
     g_assert(cmd->props->atapi);
     fis->feature_low |= 0x01;
-    cmd->interrupts &= ~AHCI_PX_IS_PSS;
+    /* PIO is still used to transfer the ATAPI command */
+    g_assert(cmd->props->pio);
     cmd->props->dma = true;
-    cmd->props->pio = false;
     /* BUG: We expect the DMA Setup interrupt for DMA commands */
     /* cmd->interrupts |= AHCI_PX_IS_DSS; */
 }
@@ -845,7 +862,7 @@ AHCICommand *ahci_command_create(uint8_t command_name)
 
     g_assert(props);
     cmd = g_new0(AHCICommand, 1);
-    g_assert(!(props->dma && props->pio));
+    g_assert(!(props->dma && props->pio) || props->atapi);
     g_assert(!(props->lba28 && props->lba48));
     g_assert(!(props->read && props->write));
     g_assert(!props->size || props->data);
@@ -1216,7 +1233,12 @@ void ahci_command_verify(AHCIQState *ahci, AHCICommand *cmd)
         ahci_port_check_d2h_sanity(ahci, port, slot);
     }
     if (cmd->props->pio) {
-        ahci_port_check_pio_sanity(ahci, port, slot, cmd->xbytes);
+        /* For ATAPI, we might have only used PIO for the command.  */
+        if (cmd->props->atapi && (!cmd->xbytes || cmd->props->dma)) {
+            ahci_port_check_pio_sanity_atapi(ahci, port, slot);
+        } else {
+            ahci_port_check_pio_sanity(ahci, port, slot, cmd->xbytes);
+        }
     }
 }
 
diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h
index 715ca1e226..cdba8099dd 100644
--- a/tests/libqos/ahci.h
+++ b/tests/libqos/ahci.h
@@ -598,6 +598,8 @@ void ahci_port_check_nonbusy(AHCIQState *ahci, uint8_t port, uint8_t slot);
 void ahci_port_check_d2h_sanity(AHCIQState *ahci, uint8_t port, uint8_t slot);
 void ahci_port_check_pio_sanity(AHCIQState *ahci, uint8_t port,
                                 uint8_t slot, size_t buffsize);
+void ahci_port_check_pio_sanity_atapi(AHCIQState *ahci, uint8_t port,
+                                      uint8_t slot);
 void ahci_port_check_cmd_sanity(AHCIQState *ahci, AHCICommand *cmd);
 
 /* Misc */
-- 
2.17.0

[Qemu-devel] [PATCH 2/6] ide: push end_transfer_func out of start_transfer callback, rename callback

[Paolo Bonzini]

Now that end_transfer_func is a tail call in ahci_start_transfer,
formalize the fact that the callback (of which ahci_start_transfer is
the sole implementation) takes care of the transfer too: rename it to
pio_transfer and, if it is present, call the end_transfer_func as soon
as it returns.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/ahci.c             | 15 +++++++--------
 hw/ide/core.c             |  8 +++++---
 hw/ide/trace-events       |  2 +-
 include/hw/ide/internal.h |  2 +-
 4 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 45ce195fb8..10bcc65308 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1280,8 +1280,8 @@ out:
     return 0;
 }
 
-/* DMA dev <-> ram */
-static void ahci_start_transfer(IDEDMA *dma)
+/* Transfer PIO data between RAM and device */
+static void ahci_pio_transfer(IDEDMA *dma)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
     IDEState *s = &ad->port.ifs[0];
@@ -1304,9 +1304,9 @@ static void ahci_start_transfer(IDEDMA *dma)
         has_sglist = 1;
     }
 
-    trace_ahci_start_transfer(ad->hba, ad->port_no, is_write ? "writ" : "read",
-                              size, is_atapi ? "atapi" : "ata",
-                              has_sglist ? "" : "o");
+    trace_ahci_pio_transfer(ad->hba, ad->port_no, is_write ? "writ" : "read",
+                            size, is_atapi ? "atapi" : "ata",
+                            has_sglist ? "" : "o");
 
     if (has_sglist && size) {
         if (is_write) {
@@ -1319,12 +1319,11 @@ static void ahci_start_transfer(IDEDMA *dma)
     /* Update number of transferred bytes, destroy sglist */
     dma_buf_commit(s, size);
     fis_len = le32_to_cpu(ad->cur_cmd->status);
+
 out:
     /* declare that we processed everything */
     s->data_ptr = s->data_end;
     ahci_write_fis_pio(ad, fis_len);
-
-    s->end_transfer_func(s);
 }
 
 static void ahci_start_dma(IDEDMA *dma, IDEState *s,
@@ -1440,7 +1439,7 @@ static const IDEDMAOps ahci_dma_ops = {
     .start_dma = ahci_start_dma,
     .restart = ahci_restart,
     .restart_dma = ahci_restart_dma,
-    .start_transfer = ahci_start_transfer,
+    .pio_transfer = ahci_pio_transfer,
     .prepare_buf = ahci_dma_prepare_buf,
     .commit_buf = ahci_commit_buf,
     .rw_buf = ahci_dma_rw_buf,
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 866c659498..7932b7c069 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -527,16 +527,18 @@ static void ide_clear_retry(IDEState *s)
 void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
                         EndTransferFunc *end_transfer_func)
 {
-    s->end_transfer_func = end_transfer_func;
     s->data_ptr = buf;
     s->data_end = buf + size;
     ide_set_retry(s);
     if (!(s->status & ERR_STAT)) {
         s->status |= DRQ_STAT;
     }
-    if (s->bus->dma->ops->start_transfer) {
-        s->bus->dma->ops->start_transfer(s->bus->dma);
+    if (!s->bus->dma->ops->pio_transfer) {
+        s->end_transfer_func = end_transfer_func;
+        return;
     }
+    s->bus->dma->ops->pio_transfer(s->bus->dma);
+    end_transfer_func(s);
 }
 
 static void ide_cmd_done(IDEState *s)
diff --git a/hw/ide/trace-events b/hw/ide/trace-events
index 0c39cabe72..77814618ee 100644
--- a/hw/ide/trace-events
+++ b/hw/ide/trace-events
@@ -101,7 +101,7 @@ handle_cmd_badport(void *s, int port) "ahci(%p)[%d]: guest accessed unused port"
 handle_cmd_badfis(void *s, int port) "ahci(%p)[%d]: guest provided an invalid cmd FIS"
 handle_cmd_badmap(void *s, int port, uint64_t len) "ahci(%p)[%d]: dma_memory_map failed, 0x%02"PRIx64" != 0x80"
 handle_cmd_unhandled_fis(void *s, int port, uint8_t b0, uint8_t b1, uint8_t b2) "ahci(%p)[%d]: unhandled FIS type. cmd_fis: 0x%02x-%02x-%02x"
-ahci_start_transfer(void *s, int port, const char *rw, uint32_t size, const char *tgt, const char *sgl) "ahci(%p)[%d]: %sing %d bytes on %s w/%s sglist"
+ahci_pio_transfer(void *s, int port, const char *rw, uint32_t size, const char *tgt, const char *sgl) "ahci(%p)[%d]: %sing %d bytes on %s w/%s sglist"
 ahci_start_dma(void *s, int port) "ahci(%p)[%d]: start dma"
 ahci_dma_prepare_buf(void *s, int port, int32_t io_buffer_size, int32_t limit) "ahci(%p)[%d]: prepare buf limit=%"PRId32" prepared=%"PRId32
 ahci_dma_prepare_buf_fail(void *s, int port) "ahci(%p)[%d]: sglist population failed"
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 88212f59df..f3de6f9b73 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -444,7 +444,7 @@ struct IDEState {
 
 struct IDEDMAOps {
     DMAStartFunc *start_dma;
-    DMAVoidFunc *start_transfer;
+    DMAVoidFunc *pio_transfer;
     DMAInt32Func *prepare_buf;
     DMAu32Func *commit_buf;
     DMAIntFunc *rw_buf;
-- 
2.17.0

[Qemu-devel] [PATCH 3/6] ide: call ide_cmd_done from ide_transfer_stop

[Paolo Bonzini]

The code can simply be moved to the sole caller that has notify == true.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/core.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 7932b7c069..edda171b47 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -549,26 +549,23 @@ static void ide_cmd_done(IDEState *s)
 }
 
 static void ide_transfer_halt(IDEState *s,
-                              void(*end_transfer_func)(IDEState *),
-                              bool notify)
+                              void(*end_transfer_func)(IDEState *))
 {
     s->end_transfer_func = end_transfer_func;
     s->data_ptr = s->io_buffer;
     s->data_end = s->io_buffer;
     s->status &= ~DRQ_STAT;
-    if (notify) {
-        ide_cmd_done(s);
-    }
 }
 
 void ide_transfer_stop(IDEState *s)
 {
-    ide_transfer_halt(s, ide_transfer_stop, true);
+    ide_transfer_halt(s, ide_transfer_stop);
+    ide_cmd_done(s);
 }
 
 static void ide_transfer_cancel(IDEState *s)
 {
-    ide_transfer_halt(s, ide_transfer_cancel, false);
+    ide_transfer_halt(s, ide_transfer_cancel);
 }
 
 int64_t ide_get_sector(IDEState *s)
-- 
2.17.0

[Qemu-devel] [PATCH 4/6] ide: make ide_transfer_stop idempotent

[Paolo Bonzini]

There is code checking s->end_transfer_func and it was not taught about
ide_transfer_cancel.  We can just use ide_transfer_stop because
s->end_transfer_func is only ever called in the DRQ phase.

ide_transfer_cancel can then be removed, since it would just be
calling ide_transfer_halt.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/core.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index edda171b47..bc3648eb13 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -548,10 +548,9 @@ static void ide_cmd_done(IDEState *s)
     }
 }
 
-static void ide_transfer_halt(IDEState *s,
-                              void(*end_transfer_func)(IDEState *))
+static void ide_transfer_halt(IDEState *s)
 {
-    s->end_transfer_func = end_transfer_func;
+    s->end_transfer_func = ide_transfer_stop;
     s->data_ptr = s->io_buffer;
     s->data_end = s->io_buffer;
     s->status &= ~DRQ_STAT;
@@ -559,15 +558,10 @@ static void ide_transfer_halt(IDEState *s,
 
 void ide_transfer_stop(IDEState *s)
 {
-    ide_transfer_halt(s, ide_transfer_stop);
+    ide_transfer_halt(s);
     ide_cmd_done(s);
 }
 
-static void ide_transfer_cancel(IDEState *s)
-{
-    ide_transfer_halt(s, ide_transfer_cancel);
-}
-
 int64_t ide_get_sector(IDEState *s)
 {
     int64_t sector_num;
@@ -1362,7 +1356,7 @@ static bool cmd_nop(IDEState *s, uint8_t cmd)
 static bool cmd_device_reset(IDEState *s, uint8_t cmd)
 {
     /* Halt PIO (in the DRQ phase), then DMA */
-    ide_transfer_cancel(s);
+    ide_transfer_halt(s);
     ide_cancel_dma_sync(s);
 
     /* Reset any PIO commands, reset signature, etc */
-- 
2.17.0

[Qemu-devel] [PATCH 5/6] atapi: call ide_set_irq before ide_transfer_start

[Paolo Bonzini]

The ATAPI_INT_REASON_IO interrupt is raised when I/O starts, but in the
AHCI case ide_set_irq was actually called at the end of a mutual recursion.
Move it early, with the side effect that ide_transfer_start becomes a tail
call in ide_atapi_cmd_reply_end.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/atapi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index c0509c8bf5..7168ff55a7 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -287,6 +287,7 @@ void ide_atapi_cmd_reply_end(IDEState *s)
         } else {
             /* a new transfer is needed */
             s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO;
+            ide_set_irq(s->bus);
             byte_count_limit = atapi_byte_count_limit(s);
             trace_ide_atapi_cmd_reply_end_bcl(s, byte_count_limit);
             size = s->packet_transfer_size;
@@ -304,13 +305,12 @@ void ide_atapi_cmd_reply_end(IDEState *s)
                 if (size > (s->cd_sector_size - s->io_buffer_index))
                     size = (s->cd_sector_size - s->io_buffer_index);
             }
+            trace_ide_atapi_cmd_reply_end_new(s, s->status);
             s->packet_transfer_size -= size;
             s->elementary_transfer_size -= size;
             s->io_buffer_index += size;
             ide_transfer_start(s, s->io_buffer + s->io_buffer_index - size,
                                size, ide_atapi_cmd_reply_end);
-            ide_set_irq(s->bus);
-            trace_ide_atapi_cmd_reply_end_new(s, s->status);
         }
     }
 }
-- 
2.17.0

[Qemu-devel] [PATCH 6/6] ide: introduce ide_transfer_start_norecurse

[Paolo Bonzini]

For the case where the end_transfer_func is also the caller of
ide_transfer_start, the mutual recursion can lead to unlimited
stack usage.  Introduce a new version that can be used to change
tail recursion into a loop, and use it in trace_ide_atapi_cmd_reply_end.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/atapi.c            | 42 +++++++++++++++++++++------------------
 hw/ide/core.c             | 16 +++++++++++----
 include/hw/ide/internal.h |  2 ++
 3 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 7168ff55a7..39e473f9c2 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -245,15 +245,11 @@ static uint16_t atapi_byte_count_limit(IDEState *s)
 void ide_atapi_cmd_reply_end(IDEState *s)
 {
     int byte_count_limit, size, ret;
-    trace_ide_atapi_cmd_reply_end(s, s->packet_transfer_size,
-                                  s->elementary_transfer_size,
-                                  s->io_buffer_index);
-    if (s->packet_transfer_size <= 0) {
-        /* end of transfer */
-        ide_atapi_cmd_ok(s);
-        ide_set_irq(s->bus);
-        trace_ide_atapi_cmd_reply_end_eot(s, s->status);
-    } else {
+    while (s->packet_transfer_size > 0) {
+        trace_ide_atapi_cmd_reply_end(s, s->packet_transfer_size,
+                                      s->elementary_transfer_size,
+                                      s->io_buffer_index);
+
         /* see if a new sector must be read */
         if (s->lba != -1 && s->io_buffer_index >= s->cd_sector_size) {
             if (!s->elementary_transfer_size) {
@@ -279,11 +275,6 @@ void ide_atapi_cmd_reply_end(IDEState *s)
             size = s->cd_sector_size - s->io_buffer_index;
             if (size > s->elementary_transfer_size)
                 size = s->elementary_transfer_size;
-            s->packet_transfer_size -= size;
-            s->elementary_transfer_size -= size;
-            s->io_buffer_index += size;
-            ide_transfer_start(s, s->io_buffer + s->io_buffer_index - size,
-                               size, ide_atapi_cmd_reply_end);
         } else {
             /* a new transfer is needed */
             s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO;
@@ -306,13 +297,26 @@ void ide_atapi_cmd_reply_end(IDEState *s)
                     size = (s->cd_sector_size - s->io_buffer_index);
             }
             trace_ide_atapi_cmd_reply_end_new(s, s->status);
-            s->packet_transfer_size -= size;
-            s->elementary_transfer_size -= size;
-            s->io_buffer_index += size;
-            ide_transfer_start(s, s->io_buffer + s->io_buffer_index - size,
-                               size, ide_atapi_cmd_reply_end);
+        }
+        s->packet_transfer_size -= size;
+        s->elementary_transfer_size -= size;
+        s->io_buffer_index += size;
+
+        /* Some adapters process PIO data right away.  In that case, we need
+         * to avoid mutual recursion between ide_transfer_start
+         * and ide_atapi_cmd_reply_end.
+         */
+        if (!ide_transfer_start_norecurse(s,
+                                          s->io_buffer + s->io_buffer_index - size,
+                                          size, ide_atapi_cmd_reply_end)) {
+            return;
         }
     }
+
+    /* end of transfer */
+    trace_ide_atapi_cmd_reply_end_eot(s, s->status);
+    ide_atapi_cmd_ok(s);
+    ide_set_irq(s->bus);
 }
 
 /* send a reply of 'size' bytes in s->io_buffer to an ATAPI command */
diff --git a/hw/ide/core.c b/hw/ide/core.c
index bc3648eb13..9884df0e2e 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -524,8 +524,8 @@ static void ide_clear_retry(IDEState *s)
 }
 
 /* prepare data transfer and tell what to do after */
-void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
-                        EndTransferFunc *end_transfer_func)
+bool ide_transfer_start_norecurse(IDEState *s, uint8_t *buf, int size,
+                                  EndTransferFunc *end_transfer_func)
 {
     s->data_ptr = buf;
     s->data_end = buf + size;
@@ -535,10 +535,18 @@ void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
     }
     if (!s->bus->dma->ops->pio_transfer) {
         s->end_transfer_func = end_transfer_func;
-        return;
+        return false;
     }
     s->bus->dma->ops->pio_transfer(s->bus->dma);
-    end_transfer_func(s);
+    return true;
+}
+
+void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
+                        EndTransferFunc *end_transfer_func)
+{
+    if (ide_transfer_start_norecurse(s, buf, size, end_transfer_func)) {
+        end_transfer_func(s);
+    }
 }
 
 static void ide_cmd_done(IDEState *s)
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index f3de6f9b73..594081e57f 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -623,6 +623,8 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val);
 
 void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
                         EndTransferFunc *end_transfer_func);
+bool ide_transfer_start_norecurse(IDEState *s, uint8_t *buf, int size,
+                                  EndTransferFunc *end_transfer_func);
 void ide_transfer_stop(IDEState *s);
 void ide_set_inactive(IDEState *s, bool more);
 BlockAIOCB *ide_issue_trim(
-- 
2.17.0

Re: [Qemu-devel] [PATCH 2/6] ide: push end_transfer_func out of start_transfer callback, rename callback

[John Snow]

On 04/17/2018 11:39 AM, Paolo Bonzini wrote:
> Now that end_transfer_func is a tail call in ahci_start_transfer,
> formalize the fact that the callback (of which ahci_start_transfer is
> the sole implementation) takes care of the transfer too: rename it to
> pio_transfer and, if it is present, call the end_transfer_func as soon
> as it returns.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/ide/ahci.c             | 15 +++++++--------
>  hw/ide/core.c             |  8 +++++---
>  hw/ide/trace-events       |  2 +-
>  include/hw/ide/internal.h |  2 +-
>  4 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 45ce195fb8..10bcc65308 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1280,8 +1280,8 @@ out:
>      return 0;
>  }
>  
> -/* DMA dev <-> ram */
> -static void ahci_start_transfer(IDEDMA *dma)
> +/* Transfer PIO data between RAM and device */
> +static void ahci_pio_transfer(IDEDMA *dma)
>  {
>      AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
>      IDEState *s = &ad->port.ifs[0];
> @@ -1304,9 +1304,9 @@ static void ahci_start_transfer(IDEDMA *dma)
>          has_sglist = 1;
>      }
>  
> -    trace_ahci_start_transfer(ad->hba, ad->port_no, is_write ? "writ" : "read",
> -                              size, is_atapi ? "atapi" : "ata",
> -                              has_sglist ? "" : "o");
> +    trace_ahci_pio_transfer(ad->hba, ad->port_no, is_write ? "writ" : "read",
> +                            size, is_atapi ? "atapi" : "ata",
> +                            has_sglist ? "" : "o");
>  
>      if (has_sglist && size) {
>          if (is_write) {
> @@ -1319,12 +1319,11 @@ static void ahci_start_transfer(IDEDMA *dma)
>      /* Update number of transferred bytes, destroy sglist */
>      dma_buf_commit(s, size);
>      fis_len = le32_to_cpu(ad->cur_cmd->status);
> +
>  out:
>      /* declare that we processed everything */
>      s->data_ptr = s->data_end;
>      ahci_write_fis_pio(ad, fis_len);
> -
> -    s->end_transfer_func(s);
>  }
>  
>  static void ahci_start_dma(IDEDMA *dma, IDEState *s,
> @@ -1440,7 +1439,7 @@ static const IDEDMAOps ahci_dma_ops = {
>      .start_dma = ahci_start_dma,
>      .restart = ahci_restart,
>      .restart_dma = ahci_restart_dma,
> -    .start_transfer = ahci_start_transfer,
> +    .pio_transfer = ahci_pio_transfer,
>      .prepare_buf = ahci_dma_prepare_buf,
>      .commit_buf = ahci_commit_buf,
>      .rw_buf = ahci_dma_rw_buf,
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 866c659498..7932b7c069 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -527,16 +527,18 @@ static void ide_clear_retry(IDEState *s)
>  void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
>                          EndTransferFunc *end_transfer_func)
>  {
> -    s->end_transfer_func = end_transfer_func;
>      s->data_ptr = buf;
>      s->data_end = buf + size;
>      ide_set_retry(s);
>      if (!(s->status & ERR_STAT)) {
>          s->status |= DRQ_STAT;
>      }
> -    if (s->bus->dma->ops->start_transfer) {
> -        s->bus->dma->ops->start_transfer(s->bus->dma);
> +    if (!s->bus->dma->ops->pio_transfer) {
> +        s->end_transfer_func = end_transfer_func;
> +        return;
>      }
> +    s->bus->dma->ops->pio_transfer(s->bus->dma);
> +    end_transfer_func(s);

Does not setting s->end_transfer_func mess with some of our dumb hacks
in e.g. ide_restart_bh or ide_is_pio_out?

Re: [Qemu-devel] [PATCH 3/6] ide: call ide_cmd_done from ide_transfer_stop

[John Snow]

On 04/17/2018 11:39 AM, Paolo Bonzini wrote:
> The code can simply be moved to the sole caller that has notify == true.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 4/6] ide: make ide_transfer_stop idempotent

[John Snow]

On 04/17/2018 11:39 AM, Paolo Bonzini wrote:
> There is code checking s->end_transfer_func and it was not taught about
> ide_transfer_cancel.  We can just use ide_transfer_stop because
> s->end_transfer_func is only ever called in the DRQ phase.
> 
> ide_transfer_cancel can then be removed, since it would just be
> calling ide_transfer_halt.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 5/6] atapi: call ide_set_irq before ide_transfer_start

[John Snow]

On 04/17/2018 11:39 AM, Paolo Bonzini wrote:
> The ATAPI_INT_REASON_IO interrupt is raised when I/O starts, but in the

This one is benefit of the doubt for me.

(I still can't quite track down our usage of the nsector register for
this, it doesn't seem supported by cmd_packet in ATA8 ... It says that
Interrupt Reason is its own "field" but that doesn't help me know which
register it's supposed to be stored in... Oh, ATA7 says that Interrupt
Reason is stored in "Sector Count." (*sigh* -- How are you supposed to
tell that from the ATA8 doc? What part of the spec tells you how to
actually read out the "Interrupt Reason" field?))

(I am still not sure which spec says when we can interrupt in ATAPI,
either. I guess that's in SCSI somewhere, probably?)

> AHCI case ide_set_irq was actually called at the end of a mutual recursion.
> Move it early, with the side effect that ide_transfer_start becomes a tail
> call in ide_atapi_cmd_reply_end.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/ide/atapi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index c0509c8bf5..7168ff55a7 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -287,6 +287,7 @@ void ide_atapi_cmd_reply_end(IDEState *s)
>          } else {
>              /* a new transfer is needed */
>              s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO;
> +            ide_set_irq(s->bus);
>              byte_count_limit = atapi_byte_count_limit(s);
>              trace_ide_atapi_cmd_reply_end_bcl(s, byte_count_limit);
>              size = s->packet_transfer_size;
> @@ -304,13 +305,12 @@ void ide_atapi_cmd_reply_end(IDEState *s)
>                  if (size > (s->cd_sector_size - s->io_buffer_index))
>                      size = (s->cd_sector_size - s->io_buffer_index);
>              }
> +            trace_ide_atapi_cmd_reply_end_new(s, s->status);
>              s->packet_transfer_size -= size;
>              s->elementary_transfer_size -= size;
>              s->io_buffer_index += size;
>              ide_transfer_start(s, s->io_buffer + s->io_buffer_index - size,
>                                 size, ide_atapi_cmd_reply_end);
> -            ide_set_irq(s->bus);
> -            trace_ide_atapi_cmd_reply_end_new(s, s->status);
>          }
>      }
>  }
> 

BOD:

Acked-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 6/6] ide: introduce ide_transfer_start_norecurse

[John Snow]

On 04/17/2018 11:39 AM, Paolo Bonzini wrote:
> For the case where the end_transfer_func is also the caller of
> ide_transfer_start, the mutual recursion can lead to unlimited
> stack usage.  Introduce a new version that can be used to change
> tail recursion into a loop, and use it in trace_ide_atapi_cmd_reply_end.

probably you mean ide_atapi_cmd_reply_end

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

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 1/6] ahci: move PIO Setup FIS before transfer, fix it for ATAPI commands

[John Snow]

On 04/17/2018 11:39 AM, Paolo Bonzini wrote:
> The PIO Setup FIS is written in the PIO:Entry state, which comes before
> the ATA and ATAPI data transfer states.  As a result, the PIO Setup FIS
> interrupt is now raised before DMA ends for ATAPI commands, and tests have
> to be adjusted.
> 

Yes, this is correct.

Unfortunately you wandered into one of the messiest areas of AHCI :(

> This is also hinted by the description of the command header in the AHCI
> specification, where the "A" bit is described as
> 
>     When ‘1’, indicates that a PIO setup FIS shall be sent by the device
>     indicating a transfer for the ATAPI command.
> 
> and also by the description of the ACMD (ATAPI command region):
> 
>     The ATAPI command must be either 12 or 16 bytes in length. The length
>     transmitted by the HBA is determined by the PIO setup FIS that is sent
>     by the device requesting the ATAPI command.
> 

I wonder how this works in practice...?

PACKET gets delivered to a SATA device via the HBA, and a Register
Host-To-Device FIS.

The SATA device, seeing that it's a PACKET command, knows that it now
needs to begin listening for PIO data, because the packet is implicitly
transferred via PIO mechanisms.

The SATA device generates a PIO Setup FIS to indicate it is ready to
receive data. I suppose it fills it as such:

- FIS Type: 0x5F

- PM Port: 0

- D: 0, because this is a Host-to-Device PIO.
  (We never fill this any differently in the AHCI emulator right now.)

- I: "Interrupt bit line of the device." I don't think we track this
  correctly. We're copying in the IRQ status of the HBA here, which is
  wrong.

- Status: Should be the status register after receiving the H2D Register
  update FIS, but prior to the data transfer, I think. "New value of the
  Status register of the Command Block for initiation of host data
  transfer."
  I think this is being set correctly after this patch.

- Error: "Contains the new value of the Error register of the Command
  Block at the conclusion of all subsequent Data to Device frames."

  This is why we were sending out post-hoc PIO Setup FIS frames before,
  how would I know what the error register *will* be...? What?

- LBA: Presumably unimportant for the purposes of receiving a command
  PACKET, as we won't be writing it to disk, but a buffer. The values
  can be reported dutifully.

- Device: Just report the register value dutifully.

- Count: Likely just relays 0, as the H2D REG FIS should have updated
  that to zero as part of the PACKET command, as per ATA8 ACS3 7.21.3.
  In any case, just report the register value dutifully.

- E_Status: "Contains the new value of the Status register of the
  Command Block at the conclusion of the subsequent Data FIS."

  Again, how would I know...?

- Transfer Count: Should be 12, as per what we specified in 0xA1
  IDENTIFY PACKET DEVICE, see core.c line 234. Your patch gets this
  correct, as we'll actually report the PIO FIS for the packet itself.


What this patch does do, though, is change the context of the Status,
Error and E_Status registers to something different. Everything else
should be the same, but I'd feel better about taking this patch if I
understood what exactly this FIS packet was supposed to convey, but I don't.

> QEMU, which conflates the "generator" and the "receiver" of the FIS into
> one device, always uses ATAPI_PACKET_SIZE, aka 12, for the length.
> 

Yeah, it's pretty messy. We play both the SATA device and the HBA
receiver in one single function. A bad delineation in the code between
SCSI, ATA, SATA and AHCI makes it hard to follow.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/ide/ahci.c       | 15 ++++++---------
>  tests/libqos/ahci.c | 30 ++++++++++++++++++++++++++----
>  tests/libqos/ahci.h |  2 ++
>  3 files changed, 34 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index e22d7be05f..45ce195fb8 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1291,11 +1291,12 @@ static void ahci_start_transfer(IDEDMA *dma)
>      int is_write = opts & AHCI_CMD_WRITE;
>      int is_atapi = opts & AHCI_CMD_ATAPI;
>      int has_sglist = 0;
> +    uint16_t fis_len;
>  
>      if (is_atapi && !ad->done_atapi_packet) {
>          /* already prepopulated iobuffer */
>          ad->done_atapi_packet = true;
> -        size = 0;
> +        fis_len = size;
>          goto out;
>      }
>  
> @@ -1315,19 +1316,15 @@ static void ahci_start_transfer(IDEDMA *dma)
>          }
>      }
>  
> +    /* Update number of transferred bytes, destroy sglist */
> +    dma_buf_commit(s, size);
> +    fis_len = le32_to_cpu(ad->cur_cmd->status);
>  out:
>      /* declare that we processed everything */
>      s->data_ptr = s->data_end;
> -
> -    /* Update number of transferred bytes, destroy sglist */
> -    dma_buf_commit(s, size);
> +    ahci_write_fis_pio(ad, fis_len);
>  
>      s->end_transfer_func(s);
> -
> -    if (!(s->status & DRQ_STAT)) {
> -        /* done with PIO send/receive */
> -        ahci_write_fis_pio(ad, le32_to_cpu(ad->cur_cmd->status));
> -    }
>  }

So in the ATAPI command packet case, this just changes the size from 0
to the length of the FIS, avoids performing a useless dma_buf_commit,
and writes the PIO Setup FIS prior to invoking the ATAPI emulator.

OK! That's definitely better.

In the normal ATA/ATAPI data transfer sense, this still performs the
data transfer prior to sending the PIO Setup FIS, but doesn't
necessarily wait for the end of the command.

Next, the fis_len we now calculate is based on the cumulative transfer
size thus far and not the size of the Data FIS we're pretending to have
transferred. If we're sending multiple PIO Setup FIS'es per-command, we
might over-report the total number of bytes transferred.

I think the correct thing to do is probably actually to do one PIO Setup
FIS per PIO transfer that reports only the size of the chunk being
transferred, which means a lot of the PIO tests are wrong because they
can't rely on the PIO FIS to report the total transfer size.

>  
>  static void ahci_start_dma(IDEDMA *dma, IDEState *s,
> diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
> index bc201d762b..04f33e246c 100644
> --- a/tests/libqos/ahci.c
> +++ b/tests/libqos/ahci.c
> @@ -477,6 +477,23 @@ void ahci_port_check_d2h_sanity(AHCIQState *ahci, uint8_t port, uint8_t slot)
>      g_free(d2h);
>  }
>  
> +void ahci_port_check_pio_sanity_atapi(AHCIQState *ahci, uint8_t port,
> +                                      uint8_t slot)
> +{
> +    PIOSetupFIS *pio = g_malloc0(0x20);
> +
> +    /* We cannot check the Status or E_Status registers, because
> +     * the status may have again changed between the PIO Setup FIS
> +     * and the conclusion of the command with the D2H Register FIS. */
> +    qtest_memread(ahci->parent->qts, ahci->port[port].fb + 0x20, pio, 0x20);
> +    g_assert_cmphex(pio->fis_type, ==, 0x5f);
> +
> +    g_assert(le16_to_cpu(pio->tx_count) == 12 ||
> +             le16_to_cpu(pio->tx_count) == 16);
> +
> +    g_free(pio);
> +}
> +
>  void ahci_port_check_pio_sanity(AHCIQState *ahci, uint8_t port,
>                                  uint8_t slot, size_t buffsize)
>  {
> @@ -831,9 +848,9 @@ void ahci_command_enable_atapi_dma(AHCICommand *cmd)
>      RegH2DFIS *fis = &(cmd->fis);
>      g_assert(cmd->props->atapi);
>      fis->feature_low |= 0x01;
> -    cmd->interrupts &= ~AHCI_PX_IS_PSS;
> +    /* PIO is still used to transfer the ATAPI command */
> +    g_assert(cmd->props->pio);
>      cmd->props->dma = true;
> -    cmd->props->pio = false;
>      /* BUG: We expect the DMA Setup interrupt for DMA commands */
>      /* cmd->interrupts |= AHCI_PX_IS_DSS; */
>  }
> @@ -845,7 +862,7 @@ AHCICommand *ahci_command_create(uint8_t command_name)
>  
>      g_assert(props);
>      cmd = g_new0(AHCICommand, 1);
> -    g_assert(!(props->dma && props->pio));
> +    g_assert(!(props->dma && props->pio) || props->atapi);
>      g_assert(!(props->lba28 && props->lba48));
>      g_assert(!(props->read && props->write));
>      g_assert(!props->size || props->data);
> @@ -1216,7 +1233,12 @@ void ahci_command_verify(AHCIQState *ahci, AHCICommand *cmd)
>          ahci_port_check_d2h_sanity(ahci, port, slot);
>      }
>      if (cmd->props->pio) {
> -        ahci_port_check_pio_sanity(ahci, port, slot, cmd->xbytes);
> +        /* For ATAPI, we might have only used PIO for the command.  */
> +        if (cmd->props->atapi && (!cmd->xbytes || cmd->props->dma)) {
> +            ahci_port_check_pio_sanity_atapi(ahci, port, slot);
> +        } else {
> +            ahci_port_check_pio_sanity(ahci, port, slot, cmd->xbytes);
> +        }
>      }
>  }
>  
> diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h
> index 715ca1e226..cdba8099dd 100644
> --- a/tests/libqos/ahci.h
> +++ b/tests/libqos/ahci.h
> @@ -598,6 +598,8 @@ void ahci_port_check_nonbusy(AHCIQState *ahci, uint8_t port, uint8_t slot);
>  void ahci_port_check_d2h_sanity(AHCIQState *ahci, uint8_t port, uint8_t slot);
>  void ahci_port_check_pio_sanity(AHCIQState *ahci, uint8_t port,
>                                  uint8_t slot, size_t buffsize);
> +void ahci_port_check_pio_sanity_atapi(AHCIQState *ahci, uint8_t port,
> +                                      uint8_t slot);
>  void ahci_port_check_cmd_sanity(AHCIQState *ahci, AHCICommand *cmd);
>  
>  /* Misc */
> 

It's definitely no more wrong than what we do now, but I'm still
confused as hell over the right way to do them.

I might still stage it even though I am pretty sure it's wrong, because
I *know* the current behavior is very wrong and a lot of things still
seem to get along just fine :)

Let me know if you can help clarify my thinking on this one.
--js

Re: [Qemu-devel] [PATCH 2/6] ide: push end_transfer_func out of start_transfer callback, rename callback

[Paolo Bonzini]

On 02/06/2018 02:24, John Snow wrote:
>> -    if (s->bus->dma->ops->start_transfer) {
>> -        s->bus->dma->ops->start_transfer(s->bus->dma);
>> +    if (!s->bus->dma->ops->pio_transfer) {
>> +        s->end_transfer_func = end_transfer_func;
>> +        return;
>>      }
>> +    s->bus->dma->ops->pio_transfer(s->bus->dma);
>> +    end_transfer_func(s);
> Does not setting s->end_transfer_func mess with some of our dumb hacks
> in e.g. ide_restart_bh or ide_is_pio_out?

No, ide_is_pio_out is not used by AHCI and ide_restart_bh looks at the
flags passed to ide_handle_rw_error.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 1/6] ahci: move PIO Setup FIS before transfer, fix it for ATAPI commands

[Paolo Bonzini]

On 02/06/2018 03:22, John Snow wrote:
> - Status: Should be the status register after receiving the H2D Register
>   update FIS, but prior to the data transfer, I think. "New value of the
>   Status register of the Command Block for initiation of host data
>   transfer."
>   I think this is being set correctly after this patch.
> 
> - Error: "Contains the new value of the Error register of the Command
>   Block at the conclusion of all subsequent Data to Device frames."
> 
>   This is why we were sending out post-hoc PIO Setup FIS frames before,
>   how would I know what the error register *will* be...? What?

You don't, I guess.  Zero?

> - LBA: Presumably unimportant for the purposes of receiving a command
>   PACKET, as we won't be writing it to disk, but a buffer. The values
>   can be reported dutifully.
> 
> - Device: Just report the register value dutifully.
> 
> - Count: Likely just relays 0, as the H2D REG FIS should have updated
>   that to zero as part of the PACKET command, as per ATA8 ACS3 7.21.3.
>   In any case, just report the register value dutifully.
> 
> - E_Status: "Contains the new value of the Status register of the
>   Command Block at the conclusion of the subsequent Data FIS."
> 
>   Again, how would I know...?
> 
> - Transfer Count: Should be 12, as per what we specified in 0xA1
>   IDENTIFY PACKET DEVICE, see core.c line 234. Your patch gets this
>   correct, as we'll actually report the PIO FIS for the packet itself.
> 
> 
> What this patch does do, though, is change the context of the Status,
> Error and E_Status registers to something different. Everything else
> should be the same, but I'd feel better about taking this patch if I
> understood what exactly this FIS packet was supposed to convey, but I don't.

At least Status and Transfer Count are correct after this patch. :/

Paolo

Re: [Qemu-devel] [PATCH 6/6] ide: introduce ide_transfer_start_norecurse

[Paolo Bonzini]

On 02/06/2018 03:17, John Snow wrote:
> 
> 
> On 04/17/2018 11:39 AM, Paolo Bonzini wrote:
>> For the case where the end_transfer_func is also the caller of
>> ide_transfer_start, the mutual recursion can lead to unlimited
>> stack usage.  Introduce a new version that can be used to change
>> tail recursion into a loop, and use it in trace_ide_atapi_cmd_reply_end.
> 
> probably you mean ide_atapi_cmd_reply_end

Yeah.  Cut-and-paste...

Paolo

>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Reviewed-by: John Snow <jsnow@redhat.com>
> 

Re: [Qemu-devel] [PATCH 2/6] ide: push end_transfer_func out of start_transfer callback, rename callback

[John Snow]

On 06/04/2018 11:48 AM, Paolo Bonzini wrote:
> On 02/06/2018 02:24, John Snow wrote:
>>> -    if (s->bus->dma->ops->start_transfer) {
>>> -        s->bus->dma->ops->start_transfer(s->bus->dma);
>>> +    if (!s->bus->dma->ops->pio_transfer) {
>>> +        s->end_transfer_func = end_transfer_func;
>>> +        return;
>>>      }
>>> +    s->bus->dma->ops->pio_transfer(s->bus->dma);
>>> +    end_transfer_func(s);
>> Does not setting s->end_transfer_func mess with some of our dumb hacks
>> in e.g. ide_restart_bh or ide_is_pio_out?
> 
> No, ide_is_pio_out is not used by AHCI and ide_restart_bh looks at the
> flags passed to ide_handle_rw_error.
> 
> Thanks,
> 
> Paolo
> 

Yes, that's right -- ide_restart_bh relies less on this now (thanks to
you, if I recall correctly) and ide_is_pio_out is only used by
ide_data_(read|write)[lw] ...

Hmm, it'd be nice to get rid of our reliance on ETF to determine state,
but that's nothing you've caused.

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 1/6] ahci: move PIO Setup FIS before transfer, fix it for ATAPI commands

[John Snow]

On 06/04/2018 11:50 AM, Paolo Bonzini wrote:
> On 02/06/2018 03:22, John Snow wrote:
>> - Status: Should be the status register after receiving the H2D Register
>>   update FIS, but prior to the data transfer, I think. "New value of the
>>   Status register of the Command Block for initiation of host data
>>   transfer."
>>   I think this is being set correctly after this patch.
>>
>> - Error: "Contains the new value of the Error register of the Command
>>   Block at the conclusion of all subsequent Data to Device frames."
>>
>>   This is why we were sending out post-hoc PIO Setup FIS frames before,
>>   how would I know what the error register *will* be...? What?
> 
> You don't, I guess.  Zero?
> 

Yeah, I don't mean to hold it up based on other arcane stuff, I was just
briefly hoping that maybe you actually understood it so I could fix it
once and for all...

>> - LBA: Presumably unimportant for the purposes of receiving a command
>>   PACKET, as we won't be writing it to disk, but a buffer. The values
>>   can be reported dutifully.
>>
>> - Device: Just report the register value dutifully.
>>
>> - Count: Likely just relays 0, as the H2D REG FIS should have updated
>>   that to zero as part of the PACKET command, as per ATA8 ACS3 7.21.3.
>>   In any case, just report the register value dutifully.
>>
>> - E_Status: "Contains the new value of the Status register of the
>>   Command Block at the conclusion of the subsequent Data FIS."
>>
>>   Again, how would I know...?
>>
>> - Transfer Count: Should be 12, as per what we specified in 0xA1
>>   IDENTIFY PACKET DEVICE, see core.c line 234. Your patch gets this
>>   correct, as we'll actually report the PIO FIS for the packet itself.
>>
>>
>> What this patch does do, though, is change the context of the Status,
>> Error and E_Status registers to something different. Everything else
>> should be the same, but I'd feel better about taking this patch if I
>> understood what exactly this FIS packet was supposed to convey, but I don't.
> 
> At least Status and Transfer Count are correct after this patch. :/
> 
> Paolo
> 

How about ...

https://github.com/jnsnow/qemu/commit/0657390a2639b7cb533ca8b514c49c0edd3f4483

This sends out a PIO Setup FIS for every PIO transfer and adjusts the
tests accordingly. Presumably any sane driver gets end of transfer
status from the D2H Register Update FIS and ... probably ignores the PIO
Setup FIS entirely. I *hope*.

(Certainly with how wrong we've gotten it, it seems very likely that
nobody uses this.)

It's probably still wrong for the reasons I've outlined in my initial
reply here, but it's probably less wrong.

I can't think of a reason you'd want an AHCI device to interrupt so
much, but it's my sincere hope that

(1) No sane AHCI driver uses PIO, and
(2) If it does, it does not do so with the PSFIS interrupt on ...

*shrug*

--js

Re: [Qemu-devel] [PATCH 1/6] ahci: move PIO Setup FIS before transfer, fix it for ATAPI commands

[Paolo Bonzini]

On 05/06/2018 01:27, John Snow wrote:
> 
> 
> On 06/04/2018 11:50 AM, Paolo Bonzini wrote:
>> On 02/06/2018 03:22, John Snow wrote:
>>> - Status: Should be the status register after receiving the H2D Register
>>>   update FIS, but prior to the data transfer, I think. "New value of the
>>>   Status register of the Command Block for initiation of host data
>>>   transfer."
>>>   I think this is being set correctly after this patch.
>>>
>>> - Error: "Contains the new value of the Error register of the Command
>>>   Block at the conclusion of all subsequent Data to Device frames."
>>>
>>>   This is why we were sending out post-hoc PIO Setup FIS frames before,
>>>   how would I know what the error register *will* be...? What?
>>
>> You don't, I guess.  Zero?
>>
> 
> Yeah, I don't mean to hold it up based on other arcane stuff, I was just
> briefly hoping that maybe you actually understood it so I could fix it
> once and for all...
> 
>>> - LBA: Presumably unimportant for the purposes of receiving a command
>>>   PACKET, as we won't be writing it to disk, but a buffer. The values
>>>   can be reported dutifully.
>>>
>>> - Device: Just report the register value dutifully.
>>>
>>> - Count: Likely just relays 0, as the H2D REG FIS should have updated
>>>   that to zero as part of the PACKET command, as per ATA8 ACS3 7.21.3.
>>>   In any case, just report the register value dutifully.
>>>
>>> - E_Status: "Contains the new value of the Status register of the
>>>   Command Block at the conclusion of the subsequent Data FIS."
>>>
>>>   Again, how would I know...?
>>>
>>> - Transfer Count: Should be 12, as per what we specified in 0xA1
>>>   IDENTIFY PACKET DEVICE, see core.c line 234. Your patch gets this
>>>   correct, as we'll actually report the PIO FIS for the packet itself.
>>>
>>>
>>> What this patch does do, though, is change the context of the Status,
>>> Error and E_Status registers to something different. Everything else
>>> should be the same, but I'd feel better about taking this patch if I
>>> understood what exactly this FIS packet was supposed to convey, but I don't.
>>
>> At least Status and Transfer Count are correct after this patch. :/
>>
>> Paolo
>>
> 
> How about ...
> 
> https://github.com/jnsnow/qemu/commit/0657390a2639b7cb533ca8b514c49c0edd3f4483

Yes, it's sane.

Paolo