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
