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