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

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

[John Snow]

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.

v2: reworked PIO Setup FIS based on spec reading, adjusted tests.

John Snow (2):
  libqos/ahci: track sector size
  ahci: move PIO Setup FIS before transfer, fix it for ATAPI commands

Paolo Bonzini (5):
  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             | 31 ++++++++++++-------------------
 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       | 45 +++++++++++++++++++++++++++------------------
 tests/libqos/ahci.h       |  3 +--
 7 files changed, 88 insertions(+), 80 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/7] libqos/ahci: track sector size

[John Snow]

It's not always 512, and it does wind up mattering for PIO tranfers,
because this means DRQ blocks are four times as big for ATAPI.
Replace an instance of 2048 with the correct define, too.

This patch by itself winds changing no behavior. fis->count is ignored
for CMD_PACKET, and sect_count only gets used in non-ATAPI cases.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/libqos/ahci.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index bc201d762b..63e1f9b92d 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -90,6 +90,7 @@ struct AHCICommand {
     uint32_t interrupts;
     uint64_t xbytes;
     uint32_t prd_size;
+    uint32_t sector_size;
     uint64_t buffer;
     AHCICommandProp *props;
     /* Data to be transferred to the guest */
@@ -796,7 +797,7 @@ static void command_header_init(AHCICommand *cmd)
 static void command_table_init(AHCICommand *cmd)
 {
     RegH2DFIS *fis = &(cmd->fis);
-    uint16_t sect_count = (cmd->xbytes / AHCI_SECTOR_SIZE);
+    uint16_t sect_count = (cmd->xbytes / cmd->sector_size);
 
     fis->fis_type = REG_H2D_FIS;
     fis->flags = REG_H2D_FIS_CMD; /* "Command" bit */
@@ -819,7 +820,7 @@ static void command_table_init(AHCICommand *cmd)
         if (cmd->props->lba28 || cmd->props->lba48) {
             fis->device = ATA_DEVICE_LBA;
         }
-        fis->count = (cmd->xbytes / AHCI_SECTOR_SIZE);
+        fis->count = (cmd->xbytes / cmd->sector_size);
     }
     fis->icc = 0x00;
     fis->control = 0x00;
@@ -857,6 +858,7 @@ AHCICommand *ahci_command_create(uint8_t command_name)
     cmd->xbytes = props->size;
     cmd->prd_size = 4096;
     cmd->buffer = 0xabad1dea;
+    cmd->sector_size = props->atapi ? ATAPI_SECTOR_SIZE : AHCI_SECTOR_SIZE;
 
     if (!cmd->props->ncq) {
         cmd->interrupts = AHCI_PX_IS_DHRS;
@@ -1033,7 +1035,7 @@ void ahci_command_set_buffer(AHCICommand *cmd, uint64_t buffer)
 static void ahci_atapi_set_size(AHCICommand *cmd, uint64_t xbytes)
 {
     unsigned char *cbd = cmd->atapi_cmd;
-    uint64_t nsectors = xbytes / 2048;
+    uint64_t nsectors = xbytes / ATAPI_SECTOR_SIZE;
     uint32_t tmp;
     g_assert(cbd);
 
@@ -1080,7 +1082,7 @@ void ahci_command_set_sizes(AHCICommand *cmd, uint64_t xbytes,
         cmd->prd_size = prd_size;
     }
     cmd->xbytes = xbytes;
-    sect_count = (cmd->xbytes / AHCI_SECTOR_SIZE);
+    sect_count = (cmd->xbytes / cmd->sector_size);
 
     if (cmd->props->ncq) {
         NCQFIS *nfis = (NCQFIS *)&(cmd->fis);
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/7] libqos/ahci: track sector size

[Philippe Mathieu-Daudé]

On 06/06/2018 04:09 PM, John Snow wrote:
> It's not always 512, and it does wind up mattering for PIO tranfers,
> because this means DRQ blocks are four times as big for ATAPI.
> Replace an instance of 2048 with the correct define, too.
> 
> This patch by itself winds changing no behavior. fis->count is ignored
> for CMD_PACKET, and sect_count only gets used in non-ATAPI cases.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/libqos/ahci.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
> index bc201d762b..63e1f9b92d 100644
> --- a/tests/libqos/ahci.c
> +++ b/tests/libqos/ahci.c
> @@ -90,6 +90,7 @@ struct AHCICommand {
>      uint32_t interrupts;
>      uint64_t xbytes;
>      uint32_t prd_size;
> +    uint32_t sector_size;
>      uint64_t buffer;
>      AHCICommandProp *props;
>      /* Data to be transferred to the guest */
> @@ -796,7 +797,7 @@ static void command_header_init(AHCICommand *cmd)
>  static void command_table_init(AHCICommand *cmd)
>  {
>      RegH2DFIS *fis = &(cmd->fis);
> -    uint16_t sect_count = (cmd->xbytes / AHCI_SECTOR_SIZE);
> +    uint16_t sect_count = (cmd->xbytes / cmd->sector_size);
>  
>      fis->fis_type = REG_H2D_FIS;
>      fis->flags = REG_H2D_FIS_CMD; /* "Command" bit */
> @@ -819,7 +820,7 @@ static void command_table_init(AHCICommand *cmd)
>          if (cmd->props->lba28 || cmd->props->lba48) {
>              fis->device = ATA_DEVICE_LBA;
>          }
> -        fis->count = (cmd->xbytes / AHCI_SECTOR_SIZE);
> +        fis->count = (cmd->xbytes / cmd->sector_size);
>      }
>      fis->icc = 0x00;
>      fis->control = 0x00;
> @@ -857,6 +858,7 @@ AHCICommand *ahci_command_create(uint8_t command_name)
>      cmd->xbytes = props->size;
>      cmd->prd_size = 4096;
>      cmd->buffer = 0xabad1dea;
> +    cmd->sector_size = props->atapi ? ATAPI_SECTOR_SIZE : AHCI_SECTOR_SIZE;
>  
>      if (!cmd->props->ncq) {
>          cmd->interrupts = AHCI_PX_IS_DHRS;
> @@ -1033,7 +1035,7 @@ void ahci_command_set_buffer(AHCICommand *cmd, uint64_t buffer)
>  static void ahci_atapi_set_size(AHCICommand *cmd, uint64_t xbytes)
>  {
>      unsigned char *cbd = cmd->atapi_cmd;
> -    uint64_t nsectors = xbytes / 2048;
> +    uint64_t nsectors = xbytes / ATAPI_SECTOR_SIZE;
>      uint32_t tmp;
>      g_assert(cbd);
>  
> @@ -1080,7 +1082,7 @@ void ahci_command_set_sizes(AHCICommand *cmd, uint64_t xbytes,
>          cmd->prd_size = prd_size;
>      }
>      cmd->xbytes = xbytes;
> -    sect_count = (cmd->xbytes / AHCI_SECTOR_SIZE);
> +    sect_count = (cmd->xbytes / cmd->sector_size);
>  
>      if (cmd->props->ncq) {
>          NCQFIS *nfis = (NCQFIS *)&(cmd->fis);
> 

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

[John Snow]

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>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c       | 18 ++++++------------
 tests/libqos/ahci.c | 35 +++++++++++++++++++++--------------
 tests/libqos/ahci.h |  3 +--
 3 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 24dbad5125..5871333686 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1198,7 +1198,6 @@ static void handle_reg_h2d_fis(AHCIState *s, int port,
             g_free(pretty_fis);
         }
         s->dev[port].done_atapi_packet = false;
-        /* XXX send PIO setup FIS */
     }
 
     ide_state->error = 0;
@@ -1292,10 +1291,12 @@ static void ahci_start_transfer(IDEDMA *dma)
     int is_atapi = opts & AHCI_CMD_ATAPI;
     int has_sglist = 0;
 
+    /* PIO FIS gets written prior to transfer */
+    ahci_write_fis_pio(ad, size);
+
     if (is_atapi && !ad->done_atapi_packet) {
         /* already prepopulated iobuffer */
         ad->done_atapi_packet = true;
-        size = 0;
         goto out;
     }
 
@@ -1315,19 +1316,12 @@ static void ahci_start_transfer(IDEDMA *dma)
         }
     }
 
-out:
-    /* declare that we processed everything */
-    s->data_ptr = s->data_end;
-
     /* Update number of transferred bytes, destroy sglist */
     dma_buf_commit(s, size);
-
+out:
+    /* declare that we processed everything */
+    s->data_ptr = s->data_end;
     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 63e1f9b92d..7264e085d0 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -478,10 +478,10 @@ void ahci_port_check_d2h_sanity(AHCIQState *ahci, uint8_t port, uint8_t slot)
     g_free(d2h);
 }
 
-void ahci_port_check_pio_sanity(AHCIQState *ahci, uint8_t port,
-                                uint8_t slot, size_t buffsize)
+void ahci_port_check_pio_sanity(AHCIQState *ahci, AHCICommand *cmd)
 {
     PIOSetupFIS *pio = g_malloc0(0x20);
+    uint8_t port = cmd->port;
 
     /* We cannot check the Status or E_Status registers, because
      * the status may have again changed between the PIO Setup FIS
@@ -489,15 +489,22 @@ void ahci_port_check_pio_sanity(AHCIQState *ahci, uint8_t port,
     qtest_memread(ahci->parent->qts, ahci->port[port].fb + 0x20, pio, 0x20);
     g_assert_cmphex(pio->fis_type, ==, 0x5f);
 
-    /* BUG: PIO Setup FIS as utilized by QEMU tries to fit the entire
-     * transfer size in a uint16_t field. The maximum transfer size can
-     * eclipse this; the field is meant to convey the size of data per
-     * each Data FIS, not the entire operation as a whole. For now,
-     * we will sanity check the broken case where applicable. */
-    if (buffsize <= UINT16_MAX) {
-        g_assert_cmphex(le16_to_cpu(pio->tx_count), ==, buffsize);
+    /* Data transferred by PIO will either be:
+     * (1) 12 or 16 bytes for an ATAPI command packet (QEMU always uses 12), or
+     * (2) Actual data from the drive.
+     * If we do both, (2) winds up erasing any evidence of (1).
+     */
+    if (cmd->props->atapi && (cmd->xbytes == 0 || cmd->props->dma)) {
+        g_assert(le16_to_cpu(pio->tx_count) == 12 ||
+                 le16_to_cpu(pio->tx_count) == 16);
+    } else {
+        /* The AHCI test suite here does not test any PIO command that specifies
+         * a DRQ block larger than one sector (like 0xC4), so this should always
+         * be one sector or less. */
+        size_t pio_len = ((cmd->xbytes % cmd->sector_size) ?
+                          (cmd->xbytes % cmd->sector_size) : cmd->sector_size);
+        g_assert_cmphex(le16_to_cpu(pio->tx_count), ==, pio_len);
     }
-
     g_free(pio);
 }
 
@@ -832,9 +839,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; */
 }
@@ -846,7 +853,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);
@@ -1218,7 +1225,7 @@ 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);
+        ahci_port_check_pio_sanity(ahci, cmd);
     }
 }
 
diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h
index 715ca1e226..13f6d87b75 100644
--- a/tests/libqos/ahci.h
+++ b/tests/libqos/ahci.h
@@ -596,8 +596,7 @@ void ahci_port_check_interrupts(AHCIQState *ahci, uint8_t port,
                                 uint32_t intr_mask);
 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(AHCIQState *ahci, AHCICommand *cmd);
 void ahci_port_check_cmd_sanity(AHCIQState *ahci, AHCICommand *cmd);
 
 /* Misc */
-- 
2.14.3

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

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

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>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c             | 13 ++++++-------
 hw/ide/core.c             |  8 +++++---
 hw/ide/trace-events       |  2 +-
 include/hw/ide/internal.h |  2 +-
 4 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 5871333686..89127f5fb6 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1279,8 +1279,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) {
@@ -1321,7 +1321,6 @@ static void ahci_start_transfer(IDEDMA *dma)
 out:
     /* declare that we processed everything */
     s->data_ptr = s->data_end;
-    s->end_transfer_func(s);
 }
 
 static void ahci_start_dma(IDEDMA *dma, IDEState *s,
@@ -1437,7 +1436,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 cc9ca28c33..1a6cb337bf 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -526,16 +526,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 5c0e59ec42..153a7a62c9 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.14.3

[Qemu-devel] [PATCH 4/7] ide: call ide_cmd_done from ide_transfer_stop

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

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

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@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 1a6cb337bf..54799ea6fb 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -548,26 +548,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.14.3

Re: [Qemu-devel] [PATCH 4/7] ide: call ide_cmd_done from ide_transfer_stop

[Philippe Mathieu-Daudé]

On 06/06/2018 04:09 PM, John Snow wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> The code can simply be moved to the sole caller that has notify == true.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  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 1a6cb337bf..54799ea6fb 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -548,26 +548,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)
> 

[Qemu-devel] [PATCH 5/7] ide: make ide_transfer_stop idempotent

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

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>
Signed-off-by: John Snow <jsnow@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 54799ea6fb..9c4864ae54 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -547,10 +547,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;
@@ -558,15 +557,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;
@@ -1361,7 +1355,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.14.3

Re: [Qemu-devel] [PATCH 5/7] ide: make ide_transfer_stop idempotent

[Philippe Mathieu-Daudé]

On 06/06/2018 04:09 PM, John Snow wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> 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>
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  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 54799ea6fb..9c4864ae54 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -547,10 +547,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;
> @@ -558,15 +557,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;
> @@ -1361,7 +1355,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 */
> 

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

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

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>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/atapi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 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);
             }
-            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);
+            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);
         }
     }
 }
-- 
2.14.3

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

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

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>
Signed-off-by: John Snow <jsnow@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 9c4864ae54..2c62efc536 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -523,8 +523,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;
@@ -534,10 +534,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.14.3

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

[John Snow]

MEH I forgot to v2 this.

On 06/06/2018 03:09 PM, John Snow wrote:
> 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.
> 
> v2: reworked PIO Setup FIS based on spec reading, adjusted tests.
> 
> John Snow (2):
>   libqos/ahci: track sector size
>   ahci: move PIO Setup FIS before transfer, fix it for ATAPI commands
> 
> Paolo Bonzini (5):
>   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             | 31 ++++++++++++-------------------
>  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       | 45 +++++++++++++++++++++++++++------------------
>  tests/libqos/ahci.h       |  3 +--
>  7 files changed, 88 insertions(+), 80 deletions(-)
> 

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

[John Snow]

On 06/06/2018 03:09 PM, John Snow wrote:
> 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.
> 
> v2: reworked PIO Setup FIS based on spec reading, adjusted tests.
> 
> John Snow (2):
>   libqos/ahci: track sector size
>   ahci: move PIO Setup FIS before transfer, fix it for ATAPI commands
> 
> Paolo Bonzini (5):
>   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             | 31 ++++++++++++-------------------
>  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       | 45 +++++++++++++++++++++++++++------------------
>  tests/libqos/ahci.h       |  3 +--
>  7 files changed, 88 insertions(+), 80 deletions(-)
> 

I know this is weird, but it will make it easier for me later:

Patches 3-7 (authored by Paolo):

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