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

[Qemu-devel] [RFC PATCH 0/5] 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 4).  The remaining four patches move code around
so as to the turn the call back to ide_atapi_cmd_reply_end into
another tail call, and then convert the (double) tail recursion into
a while loop.

I'm not sure how this can be tested, apart from adding a READ CD
test to ahci-test (which I don't really have time for now, hence
the RFC tag).  The existing AHCI tests still pass, so patches 1-3
aren't complete crap.

Paolo

Paolo Bonzini (5):
  ide: push call to end_transfer_func out of start_transfer callback
  ide: push end_transfer callback to ide_transfer_halt
  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             | 12 +++++++-----
 hw/ide/atapi.c            | 37 ++++++++++++++++++++-----------------
 hw/ide/core.c             | 37 +++++++++++++++++++++++--------------
 include/hw/ide/internal.h |  3 +++
 4 files changed, 53 insertions(+), 36 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/5] ide: push call to end_transfer_func out of start_transfer callback

[Paolo Bonzini]

Split the PIO transfer across two callbacks, thus pushing the (possibly
recursive) call to end_transfer_func up one level and out of the
AHCI-specific code.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/ahci.c             | 7 ++++++-
 hw/ide/core.c             | 9 ++++++---
 include/hw/ide/internal.h | 1 +
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index e22d7be05f..937bad55fb 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1321,8 +1321,12 @@ out:
 
     /* Update number of transferred bytes, destroy sglist */
     dma_buf_commit(s, size);
+}
 
-    s->end_transfer_func(s);
+static void ahci_end_transfer(IDEDMA *dma)
+{
+    AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
+    IDEState *s = &ad->port.ifs[0];
 
     if (!(s->status & DRQ_STAT)) {
         /* done with PIO send/receive */
@@ -1444,6 +1448,7 @@ static const IDEDMAOps ahci_dma_ops = {
     .restart = ahci_restart,
     .restart_dma = ahci_restart_dma,
     .start_transfer = ahci_start_transfer,
+    .end_transfer = ahci_end_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 257b429381..92f4424dc3 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -532,16 +532,19 @@ 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->start_transfer) {
+        s->end_transfer_func = end_transfer_func;
+        return;
     }
+    s->bus->dma->ops->start_transfer(s->bus->dma);
+    end_transfer_func(s);
+    s->bus->dma->ops->end_transfer(s->bus->dma);
 }
 
 static void ide_cmd_done(IDEState *s)
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 88212f59df..efaabbd815 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -445,6 +445,7 @@ struct IDEState {
 struct IDEDMAOps {
     DMAStartFunc *start_dma;
     DMAVoidFunc *start_transfer;
+    DMAVoidFunc *end_transfer;
     DMAInt32Func *prepare_buf;
     DMAu32Func *commit_buf;
     DMAIntFunc *rw_buf;
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/5] ide: push call to end_transfer_func out of start_transfer callback

[John Snow]

On 02/23/2018 10:26 AM, Paolo Bonzini wrote:
> Split the PIO transfer across two callbacks, thus pushing the (possibly
> recursive) call to end_transfer_func up one level and out of the
> AHCI-specific code.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/ide/ahci.c             | 7 ++++++-
>  hw/ide/core.c             | 9 ++++++---
>  include/hw/ide/internal.h | 1 +
>  3 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index e22d7be05f..937bad55fb 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1321,8 +1321,12 @@ out:
>  
>      /* Update number of transferred bytes, destroy sglist */
>      dma_buf_commit(s, size);
> +}
>  
> -    s->end_transfer_func(s);
> +static void ahci_end_transfer(IDEDMA *dma)
> +{
> +    AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
> +    IDEState *s = &ad->port.ifs[0];
>  
>      if (!(s->status & DRQ_STAT)) {
>          /* done with PIO send/receive */
> @@ -1444,6 +1448,7 @@ static const IDEDMAOps ahci_dma_ops = {
>      .restart = ahci_restart,
>      .restart_dma = ahci_restart_dma,
>      .start_transfer = ahci_start_transfer,
> +    .end_transfer = ahci_end_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 257b429381..92f4424dc3 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -532,16 +532,19 @@ 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->start_transfer) {
> +        s->end_transfer_func = end_transfer_func;
> +        return;
>      }
> +    s->bus->dma->ops->start_transfer(s->bus->dma);
> +    end_transfer_func(s);

wow, this makes me really uneasy to look at. The assumption now is: "If
you have a start_transfer method, that method if successful implies that
the transfer is *COMPLETED*."

It's implicit here, but I think anyone but the two of us would probably
not understand that implication. (Or me in three months.) What can we do
about that? Since AHCI is the only interface that uses this callback, I
wonder if we can't just rename it to something more obvious.

perform_transfer ? do_transfer ?

Under normal circumstances this function really does just "start" a
transfer and it's not obvious from context that some adapters have
synchronous methods to finish the transfer without further PIO pingpong
with the guest.

> +    s->bus->dma->ops->end_transfer(s->bus->dma);
>  }
>  
>  static void ide_cmd_done(IDEState *s)
> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
> index 88212f59df..efaabbd815 100644
> --- a/include/hw/ide/internal.h
> +++ b/include/hw/ide/internal.h
> @@ -445,6 +445,7 @@ struct IDEState {
>  struct IDEDMAOps {
>      DMAStartFunc *start_dma;
>      DMAVoidFunc *start_transfer;
> +    DMAVoidFunc *end_transfer;
>      DMAInt32Func *prepare_buf;
>      DMAu32Func *commit_buf;
>      DMAIntFunc *rw_buf;
> 

Technically-OK-but-my-sadness-increased: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 1/5] ide: push call to end_transfer_func out of start_transfer callback

[Paolo Bonzini]

On 20/03/2018 22:46, John Snow wrote:
>>      }
>> -    if (s->bus->dma->ops->start_transfer) {
>> -        s->bus->dma->ops->start_transfer(s->bus->dma);
>> +    if (!s->bus->dma->ops->start_transfer) {
>> +        s->end_transfer_func = end_transfer_func;
>> +        return;
>>      }
>> +    s->bus->dma->ops->start_transfer(s->bus->dma);
>> +    end_transfer_func(s);
> wow, this makes me really uneasy to look at. The assumption now is: "If
> you have a start_transfer method, that method if successful implies that
> the transfer is *COMPLETED*."
> 
> It's implicit here, but I think anyone but the two of us would probably
> not understand that implication. (Or me in three months.) What can we do
> about that? Since AHCI is the only interface that uses this callback, I
> wonder if we can't just rename it to something more obvious.

You are completely right, it should be renamed to pio_transfer.

> Under normal circumstances this function really does just "start" a
> transfer and it's not obvious from context that some adapters have
> synchronous methods to finish the transfer without further PIO pingpong
> with the guest.

Yeah, though it is already doing it---the patch is only unhiding the
mutual recursion by pulling it one level up.

Paolo

[Qemu-devel] [PATCH 2/5] ide: push end_transfer callback to ide_transfer_halt

[Paolo Bonzini]

The callback must be invoked once we get out of the DRQ phase; because
all end_transfer_funcs end up invoking ide_transfer_stop, call it there.
While at it, remove the "notify" argument from ide_transfer_halt; the
code can simply be moved to ide_transfer_stop.

Old PATA controllers have no end_transfer callback, so there is no change
there.  For AHCI the end_transfer_func already called ide_transfer_stop
so the effect is that the PIO FIS is written before the D2H FIS rather
than after, which is arguably an improvement.

However, ahci_end_transfer is now called _after_ the DRQ_STAT has been
cleared, so remove the status check in ahci_end_transfer; it was only
there to ensure the FIS was not written more than once, and this cannot
happen anymore.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/ahci.c |  7 ++-----
 hw/ide/core.c | 15 +++++++--------
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 937bad55fb..c3c6843b76 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1326,12 +1326,9 @@ out:
 static void ahci_end_transfer(IDEDMA *dma)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
-    IDEState *s = &ad->port.ifs[0];
 
-    if (!(s->status & DRQ_STAT)) {
-        /* done with PIO send/receive */
-        ahci_write_fis_pio(ad, le32_to_cpu(ad->cur_cmd->status));
-    }
+    /* 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/hw/ide/core.c b/hw/ide/core.c
index 92f4424dc3..c4710a6f55 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -544,7 +544,6 @@ void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
     }
     s->bus->dma->ops->start_transfer(s->bus->dma);
     end_transfer_func(s);
-    s->bus->dma->ops->end_transfer(s->bus->dma);
 }
 
 static void ide_cmd_done(IDEState *s)
@@ -555,26 +554,26 @@ 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);
+    if (s->bus->dma->ops->end_transfer) {
+        s->bus->dma->ops->end_transfer(s->bus->dma);
+    }
+    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 2/5] ide: push end_transfer callback to ide_transfer_halt

[John Snow]

On 02/23/2018 10:26 AM, Paolo Bonzini wrote:
> The callback must be invoked once we get out of the DRQ phase; because
> all end_transfer_funcs end up invoking ide_transfer_stop, call it there.
> While at it, remove the "notify" argument from ide_transfer_halt; the
> code can simply be moved to ide_transfer_stop.
> 
> Old PATA controllers have no end_transfer callback, so there is no change
> there.  For AHCI the end_transfer_func already called ide_transfer_stop
> so the effect is that the PIO FIS is written before the D2H FIS rather
> than after, which is arguably an improvement.
> 

MMK, so this is the "PIO Setup FIS" which I... actually, I was never
sure what role this was supposed to play *exactly*.

"Used by the device to provide the host adapter with sufficient
information regarding a Programmed Input/Output (PIO) data phase to
allow the host adapter to efficiently handle PIO data transfers."

So in a perfect world, the SATA layer generates this *for* the AHCI
adapter, but really both of those models are smooshed into one layer for
us, so we just generate this so the host driver doesn't freak out.

"For PIO data transfers, the device shall send to the host a PIO Setup –
Device to Host FIS just before each and every data transfer FIS that is
required to complete the data transfer. Data transfers from Host to
Device as well as data transfers from Device to Host shall follow this
algorithm."

Well, we don't emulate Data FIS packets and I don't *think* those get
cached in the received FIS data structure area for the AHCI HBA, so we
just skip those.

"Because of the stringent timing constraints in the ATA Standard, the
PIO Setup FIS includes both the starting and ending status values. These
are used by the host adapter to first signal to host software readiness
for PIO write data (BSY bit is cleared to zero and DRQ bit is set to
one), and following the PIO write burst to properly signal host software
by clearing the DRQ bit to zero and possibly setting the BSY bit to one."

This part confuses me. The starting and ending status values? How would
we know the ending status value if this gets sent by the device before a
transfer?

> However, ahci_end_transfer is now called _after_ the DRQ_STAT has been
> cleared, so remove the status check in ahci_end_transfer; it was only
> there to ensure the FIS was not written more than once, and this cannot
> happen anymore.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/ide/ahci.c |  7 ++-----
>  hw/ide/core.c | 15 +++++++--------
>  2 files changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 937bad55fb..c3c6843b76 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1326,12 +1326,9 @@ out:
>  static void ahci_end_transfer(IDEDMA *dma)
>  {
>      AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
> -    IDEState *s = &ad->port.ifs[0];
>  
> -    if (!(s->status & DRQ_STAT)) {
> -        /* done with PIO send/receive */
> -        ahci_write_fis_pio(ad, le32_to_cpu(ad->cur_cmd->status));
> -    }
> +    /* 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/hw/ide/core.c b/hw/ide/core.c
> index 92f4424dc3..c4710a6f55 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -544,7 +544,6 @@ void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
>      }
>      s->bus->dma->ops->start_transfer(s->bus->dma);
>      end_transfer_func(s);
> -    s->bus->dma->ops->end_transfer(s->bus->dma);
>  }
>  
>  static void ide_cmd_done(IDEState *s)
> @@ -555,26 +554,26 @@ 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);
> +    if (s->bus->dma->ops->end_transfer) {
> +        s->bus->dma->ops->end_transfer(s->bus->dma);
> +    }
> +    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)
> 

Seems sane, with some lingering questions about what the PIO Setup FIS
is supposed to do to begin with still remaining.

Re: [Qemu-devel] [PATCH 2/5] ide: push end_transfer callback to ide_transfer_halt

[Paolo Bonzini]

On 20/03/2018 23:11, John Snow wrote:
> Seems sane, with some lingering questions about what the PIO Setup FIS
> is supposed to do to begin with still remaining.

I agree here too.  Smashing the ATA and controller in the same device
makes things tricky, so I tried to make these patches pure code motion,
as much as I could.

Paolo

Re: [Qemu-devel] [PATCH 2/5] ide: push end_transfer callback to ide_transfer_halt

[John Snow]

On 03/21/2018 01:39 AM, Paolo Bonzini wrote:
> On 20/03/2018 23:11, John Snow wrote:
>> Seems sane, with some lingering questions about what the PIO Setup FIS
>> is supposed to do to begin with still remaining.
> 
> I agree here too.  Smashing the ATA and controller in the same device
> makes things tricky, so I tried to make these patches pure code motion,
> as much as I could.
> 
> Paolo
> 

Oh, I understand. There's a reason I haven't touched the PIO Setup FIS
much when I cleaned the device up before.

I was just hoping you'd magically know more about it.

:)

[Qemu-devel] [PATCH 3/5] ide: do not set s->end_transfer_func to ide_transfer_cancel

[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: after
ide_transfer_cancel, the value of s->end_transfer_func is only used
as a marker and never used to actually invoke the function.

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

diff --git a/hw/ide/core.c b/hw/ide/core.c
index c4710a6f55..447d9624df 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -553,10 +553,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;
@@ -564,7 +563,7 @@ static void ide_transfer_halt(IDEState *s,
 
 void ide_transfer_stop(IDEState *s)
 {
-    ide_transfer_halt(s, ide_transfer_stop);
+    ide_transfer_halt(s);
     if (s->bus->dma->ops->end_transfer) {
         s->bus->dma->ops->end_transfer(s->bus->dma);
     }
@@ -573,7 +572,7 @@ void ide_transfer_stop(IDEState *s)
 
 static void ide_transfer_cancel(IDEState *s)
 {
-    ide_transfer_halt(s, ide_transfer_cancel);
+    ide_transfer_halt(s);
 }
 
 int64_t ide_get_sector(IDEState *s)
-- 
2.14.3

Re: [Qemu-devel] [PATCH 3/5] ide: do not set s->end_transfer_func to ide_transfer_cancel

[John Snow]

On 02/23/2018 10:26 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: after
> ide_transfer_cancel, the value of s->end_transfer_func is only used
> as a marker and never used to actually invoke the function.
> 

I think you're right, but I think it's also pretty non-obvious to a
reader that a callback might be used in this way.

"John, it's your component dude, why don't you fix that?"

Err, I'm afraid of disturbing the delicate balance of spaghetti code.

*cough*

Anyway, a little comment explaining that we don't actually *expect* that
callback to actually get called would probably answer a question or two
before they arise.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/ide/core.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index c4710a6f55..447d9624df 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -553,10 +553,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;
> @@ -564,7 +563,7 @@ static void ide_transfer_halt(IDEState *s,
>  
>  void ide_transfer_stop(IDEState *s)
>  {
> -    ide_transfer_halt(s, ide_transfer_stop);
> +    ide_transfer_halt(s);
>      if (s->bus->dma->ops->end_transfer) {
>          s->bus->dma->ops->end_transfer(s->bus->dma);
>      }
> @@ -573,7 +572,7 @@ void ide_transfer_stop(IDEState *s)
>  
>  static void ide_transfer_cancel(IDEState *s)
>  {
> -    ide_transfer_halt(s, ide_transfer_cancel);
> +    ide_transfer_halt(s);
>  }
>  
>  int64_t ide_get_sector(IDEState *s)
> 

"LGTM"

[Qemu-devel] [PATCH 4/5] 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..be99a929cf 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;
@@ -307,10 +308,9 @@ void ide_atapi_cmd_reply_end(IDEState *s)
             s->packet_transfer_size -= size;
             s->elementary_transfer_size -= size;
             s->io_buffer_index += size;
+            trace_ide_atapi_cmd_reply_end_new(s, s->status);
             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.14.3

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

[John Snow]

On 02/23/2018 10:26 AM, Paolo Bonzini wrote:
> 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.
> 

Do you know in which spec it specifies that we should interrupt?

Seems okay, but I wanted to double check the wording in the spec (was
looking in scsi MMC and ATA8) and couldn't find it quickly.

> 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..be99a929cf 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;
> @@ -307,10 +308,9 @@ void ide_atapi_cmd_reply_end(IDEState *s)
>              s->packet_transfer_size -= size;
>              s->elementary_transfer_size -= size;
>              s->io_buffer_index += size;
> +            trace_ide_atapi_cmd_reply_end_new(s, s->status);
>              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);
>          }
>      }
>  }
> 

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

[Paolo Bonzini]

On 21/03/2018 01:35, John Snow wrote:
>> 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.
>>
> Do you know in which spec it specifies that we should interrupt?
> 
> Seems okay, but I wanted to double check the wording in the spec (was
> looking in scsi MMC and ATA8) and couldn't find it quickly.

I have an "Interrupt Reason" field in my copy of the ATA/ATAPI spec:

---
6.4.3 Input/Output (I/O) bit
The Input/Output bit shall be cleared to zero if the transfer is to the
device. The Input/Output bit shall be set to one if the transfer is to
the host. In ATA/ATAPI-7 this bit was documented as the I/O bit.
---

I suppose the commit message needs some improvement---the point is that
since this is a READ, the I/O bit must be set before we start the
transfer.  It can be done after the disk I/O starts, but it must be done
before the PIO transfer starts; in other words, the bug is only there
for AHCI.

Paolo

[Qemu-devel] [PATCH 5/5] 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            | 35 +++++++++++++++++++----------------
 hw/ide/core.c             | 16 ++++++++++++----
 include/hw/ide/internal.h |  2 ++
 3 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index be99a929cf..4df4a66bbe 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -248,12 +248,7 @@ void ide_atapi_cmd_reply_end(IDEState *s)
     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) {
         /* 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 +274,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;
@@ -305,14 +295,27 @@ 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;
             trace_ide_atapi_cmd_reply_end_new(s, s->status);
-            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 */
+    ide_atapi_cmd_ok(s);
+    ide_set_irq(s->bus);
+    trace_ide_atapi_cmd_reply_end_eot(s, s->status);
 }
 
 /* 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 447d9624df..ddefeb086d 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -529,8 +529,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;
@@ -540,10 +540,18 @@ void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
     }
     if (!s->bus->dma->ops->start_transfer) {
         s->end_transfer_func = end_transfer_func;
-        return;
+        return false;
     }
     s->bus->dma->ops->start_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 efaabbd815..1bd93d0a30 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -624,6 +624,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] [RFC PATCH 0/5] atapi: change unlimited recursion to while loop

[John Snow]

On 02/23/2018 10:26 AM, Paolo Bonzini 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 4).  The remaining four patches move code around
> so as to the turn the call back to ide_atapi_cmd_reply_end into
> another tail call, and then convert the (double) tail recursion into
> a while loop.
> 
> I'm not sure how this can be tested, apart from adding a READ CD
> test to ahci-test (which I don't really have time for now, hence
> the RFC tag).  The existing AHCI tests still pass, so patches 1-3
> aren't complete crap.
> 
> Paolo
> 
> Paolo Bonzini (5):
>   ide: push call to end_transfer_func out of start_transfer callback
>   ide: push end_transfer callback to ide_transfer_halt
>   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             | 12 +++++++-----
>  hw/ide/atapi.c            | 37 ++++++++++++++++++++-----------------
>  hw/ide/core.c             | 37 +++++++++++++++++++++++--------------
>  include/hw/ide/internal.h |  3 +++
>  4 files changed, 53 insertions(+), 36 deletions(-)
> 

ACK receipt, I will get to this soon, sorry!

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

[John Snow]

On 02/23/2018 10:26 AM, Paolo Bonzini 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 4).  The remaining four patches move code around
> so as to the turn the call back to ide_atapi_cmd_reply_end into
> another tail call, and then convert the (double) tail recursion into
> a while loop.
> 
> I'm not sure how this can be tested, apart from adding a READ CD
> test to ahci-test (which I don't really have time for now, hence
> the RFC tag).  The existing AHCI tests still pass, so patches 1-3
> aren't complete crap.
> 
> Paolo
> 
> Paolo Bonzini (5):
>   ide: push call to end_transfer_func out of start_transfer callback
>   ide: push end_transfer callback to ide_transfer_halt
>   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             | 12 +++++++-----
>  hw/ide/atapi.c            | 37 ++++++++++++++++++++-----------------
>  hw/ide/core.c             | 37 +++++++++++++++++++++++--------------
>  include/hw/ide/internal.h |  3 +++
>  4 files changed, 53 insertions(+), 36 deletions(-)
> 

LGTM; only comments wound up being naming.

--js

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

[Paolo Bonzini]

On 23/03/2018 21:08, John Snow wrote:
> 
> 
> On 02/23/2018 10:26 AM, Paolo Bonzini 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 4).  The remaining four patches move code around
>> so as to the turn the call back to ide_atapi_cmd_reply_end into
>> another tail call, and then convert the (double) tail recursion into
>> a while loop.
>>
>> I'm not sure how this can be tested, apart from adding a READ CD
>> test to ahci-test (which I don't really have time for now, hence
>> the RFC tag).  The existing AHCI tests still pass, so patches 1-3
>> aren't complete crap.
>>
>> Paolo
>>
>> Paolo Bonzini (5):
>>   ide: push call to end_transfer_func out of start_transfer callback
>>   ide: push end_transfer callback to ide_transfer_halt
>>   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             | 12 +++++++-----
>>  hw/ide/atapi.c            | 37 ++++++++++++++++++++-----------------
>>  hw/ide/core.c             | 37 +++++++++++++++++++++++--------------
>>  include/hw/ide/internal.h |  3 +++
>>  4 files changed, 53 insertions(+), 36 deletions(-)
>>
> 
> LGTM; only comments wound up being naming.

The "PIO setup" FIS though should be sent at the *beginning* of data
transfer according to the spec.  And if that is fixed a bunch of things
are simpler (no end_transfer callback!).  I'll test and send next week.

Paolo

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

[John Snow]

On 03/23/2018 04:17 PM, Paolo Bonzini wrote:
> On 23/03/2018 21:08, John Snow wrote:
>>
>>
>> On 02/23/2018 10:26 AM, Paolo Bonzini 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 4).  The remaining four patches move code around
>>> so as to the turn the call back to ide_atapi_cmd_reply_end into
>>> another tail call, and then convert the (double) tail recursion into
>>> a while loop.
>>>
>>> I'm not sure how this can be tested, apart from adding a READ CD
>>> test to ahci-test (which I don't really have time for now, hence
>>> the RFC tag).  The existing AHCI tests still pass, so patches 1-3
>>> aren't complete crap.
>>>
>>> Paolo
>>>
>>> Paolo Bonzini (5):
>>>   ide: push call to end_transfer_func out of start_transfer callback
>>>   ide: push end_transfer callback to ide_transfer_halt
>>>   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             | 12 +++++++-----
>>>  hw/ide/atapi.c            | 37 ++++++++++++++++++++-----------------
>>>  hw/ide/core.c             | 37 +++++++++++++++++++++++--------------
>>>  include/hw/ide/internal.h |  3 +++
>>>  4 files changed, 53 insertions(+), 36 deletions(-)
>>>
>>
>> LGTM; only comments wound up being naming.
> 
> The "PIO setup" FIS though should be sent at the *beginning* of data
> transfer according to the spec.  And if that is fixed a bunch of things
> are simpler (no end_transfer callback!).  I'll test and send next week.
> 
> Paolo
> 

My naive understanding is that it gets sent at the beginning to inform
the transfer -- but I'm not sure what the values of the frame should
actually be since it specifies it should also set what the value of the
registers ought to be after the transfer -- and I don't know how to
interpret that.

Is that an "expected value" or does that mean that the device (like the
SATA device) is expected to buffer up the transfer first, then send the
PIO Setup FIS frame and thus it already knows if it succeeded or failed
in buffering the data?

It's not tremendously clear to me -- but as long as at the end of the
transfer AHCI's mirror for the ATA registers match our core register
values, then it's probably fine...?