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>