[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [RFC PATCH 0/5] atapi: change unlimited re
Re: [Qemu-block] [Qemu-devel] [RFC PATCH 0/5] atapi: change unlimited recursion to while loop
Fri, 23 Mar 2018 21:17:44 +0100
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0
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 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.