| Message ID | 20180417153945.20737-3-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | atapi: change unlimited recursion to while loop | expand |
On 04/17/2018 11:39 AM, Paolo Bonzini wrote: > Now that end_transfer_func is a tail call in ahci_start_transfer, > formalize the fact that the callback (of which ahci_start_transfer is > the sole implementation) takes care of the transfer too: rename it to > pio_transfer and, if it is present, call the end_transfer_func as soon > as it returns. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/ide/ahci.c | 15 +++++++-------- > hw/ide/core.c | 8 +++++--- > hw/ide/trace-events | 2 +- > include/hw/ide/internal.h | 2 +- > 4 files changed, 14 insertions(+), 13 deletions(-) > > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c > index 45ce195fb8..10bcc65308 100644 > --- a/hw/ide/ahci.c > +++ b/hw/ide/ahci.c > @@ -1280,8 +1280,8 @@ out: > return 0; > } > > -/* DMA dev <-> ram */ > -static void ahci_start_transfer(IDEDMA *dma) > +/* Transfer PIO data between RAM and device */ > +static void ahci_pio_transfer(IDEDMA *dma) > { > AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma); > IDEState *s = &ad->port.ifs[0]; > @@ -1304,9 +1304,9 @@ static void ahci_start_transfer(IDEDMA *dma) > has_sglist = 1; > } > > - trace_ahci_start_transfer(ad->hba, ad->port_no, is_write ? "writ" : "read", > - size, is_atapi ? "atapi" : "ata", > - has_sglist ? "" : "o"); > + trace_ahci_pio_transfer(ad->hba, ad->port_no, is_write ? "writ" : "read", > + size, is_atapi ? "atapi" : "ata", > + has_sglist ? "" : "o"); > > if (has_sglist && size) { > if (is_write) { > @@ -1319,12 +1319,11 @@ static void ahci_start_transfer(IDEDMA *dma) > /* Update number of transferred bytes, destroy sglist */ > dma_buf_commit(s, size); > fis_len = le32_to_cpu(ad->cur_cmd->status); > + > out: > /* declare that we processed everything */ > s->data_ptr = s->data_end; > ahci_write_fis_pio(ad, fis_len); > - > - s->end_transfer_func(s); > } > > static void ahci_start_dma(IDEDMA *dma, IDEState *s, > @@ -1440,7 +1439,7 @@ static const IDEDMAOps ahci_dma_ops = { > .start_dma = ahci_start_dma, > .restart = ahci_restart, > .restart_dma = ahci_restart_dma, > - .start_transfer = ahci_start_transfer, > + .pio_transfer = ahci_pio_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 866c659498..7932b7c069 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -527,16 +527,18 @@ 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->pio_transfer) { > + s->end_transfer_func = end_transfer_func; > + return; > } > + s->bus->dma->ops->pio_transfer(s->bus->dma); > + end_transfer_func(s); Does not setting s->end_transfer_func mess with some of our dumb hacks in e.g. ide_restart_bh or ide_is_pio_out?
On 02/06/2018 02:24, John Snow wrote: >> - if (s->bus->dma->ops->start_transfer) { >> - s->bus->dma->ops->start_transfer(s->bus->dma); >> + if (!s->bus->dma->ops->pio_transfer) { >> + s->end_transfer_func = end_transfer_func; >> + return; >> } >> + s->bus->dma->ops->pio_transfer(s->bus->dma); >> + end_transfer_func(s); > Does not setting s->end_transfer_func mess with some of our dumb hacks > in e.g. ide_restart_bh or ide_is_pio_out? No, ide_is_pio_out is not used by AHCI and ide_restart_bh looks at the flags passed to ide_handle_rw_error. Thanks, Paolo
On 06/04/2018 11:48 AM, Paolo Bonzini wrote: > On 02/06/2018 02:24, John Snow wrote: >>> - if (s->bus->dma->ops->start_transfer) { >>> - s->bus->dma->ops->start_transfer(s->bus->dma); >>> + if (!s->bus->dma->ops->pio_transfer) { >>> + s->end_transfer_func = end_transfer_func; >>> + return; >>> } >>> + s->bus->dma->ops->pio_transfer(s->bus->dma); >>> + end_transfer_func(s); >> Does not setting s->end_transfer_func mess with some of our dumb hacks >> in e.g. ide_restart_bh or ide_is_pio_out? > > No, ide_is_pio_out is not used by AHCI and ide_restart_bh looks at the > flags passed to ide_handle_rw_error. > > Thanks, > > Paolo > Yes, that's right -- ide_restart_bh relies less on this now (thanks to you, if I recall correctly) and ide_is_pio_out is only used by ide_data_(read|write)[lw] ... Hmm, it'd be nice to get rid of our reliance on ETF to determine state, but that's nothing you've caused. Reviewed-by: John Snow <jsnow@redhat.com>
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 45ce195fb8..10bcc65308 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1280,8 +1280,8 @@ out: return 0; } -/* DMA dev <-> ram */ -static void ahci_start_transfer(IDEDMA *dma) +/* Transfer PIO data between RAM and device */ +static void ahci_pio_transfer(IDEDMA *dma) { AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma); IDEState *s = &ad->port.ifs[0]; @@ -1304,9 +1304,9 @@ static void ahci_start_transfer(IDEDMA *dma) has_sglist = 1; } - trace_ahci_start_transfer(ad->hba, ad->port_no, is_write ? "writ" : "read", - size, is_atapi ? "atapi" : "ata", - has_sglist ? "" : "o"); + trace_ahci_pio_transfer(ad->hba, ad->port_no, is_write ? "writ" : "read", + size, is_atapi ? "atapi" : "ata", + has_sglist ? "" : "o"); if (has_sglist && size) { if (is_write) { @@ -1319,12 +1319,11 @@ static void ahci_start_transfer(IDEDMA *dma) /* Update number of transferred bytes, destroy sglist */ dma_buf_commit(s, size); fis_len = le32_to_cpu(ad->cur_cmd->status); + out: /* declare that we processed everything */ s->data_ptr = s->data_end; ahci_write_fis_pio(ad, fis_len); - - s->end_transfer_func(s); } static void ahci_start_dma(IDEDMA *dma, IDEState *s, @@ -1440,7 +1439,7 @@ static const IDEDMAOps ahci_dma_ops = { .start_dma = ahci_start_dma, .restart = ahci_restart, .restart_dma = ahci_restart_dma, - .start_transfer = ahci_start_transfer, + .pio_transfer = ahci_pio_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 866c659498..7932b7c069 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -527,16 +527,18 @@ 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->pio_transfer) { + s->end_transfer_func = end_transfer_func; + return; } + s->bus->dma->ops->pio_transfer(s->bus->dma); + end_transfer_func(s); } static void ide_cmd_done(IDEState *s) diff --git a/hw/ide/trace-events b/hw/ide/trace-events index 0c39cabe72..77814618ee 100644 --- a/hw/ide/trace-events +++ b/hw/ide/trace-events @@ -101,7 +101,7 @@ handle_cmd_badport(void *s, int port) "ahci(%p)[%d]: guest accessed unused port" handle_cmd_badfis(void *s, int port) "ahci(%p)[%d]: guest provided an invalid cmd FIS" handle_cmd_badmap(void *s, int port, uint64_t len) "ahci(%p)[%d]: dma_memory_map failed, 0x%02"PRIx64" != 0x80" handle_cmd_unhandled_fis(void *s, int port, uint8_t b0, uint8_t b1, uint8_t b2) "ahci(%p)[%d]: unhandled FIS type. cmd_fis: 0x%02x-%02x-%02x" -ahci_start_transfer(void *s, int port, const char *rw, uint32_t size, const char *tgt, const char *sgl) "ahci(%p)[%d]: %sing %d bytes on %s w/%s sglist" +ahci_pio_transfer(void *s, int port, const char *rw, uint32_t size, const char *tgt, const char *sgl) "ahci(%p)[%d]: %sing %d bytes on %s w/%s sglist" ahci_start_dma(void *s, int port) "ahci(%p)[%d]: start dma" ahci_dma_prepare_buf(void *s, int port, int32_t io_buffer_size, int32_t limit) "ahci(%p)[%d]: prepare buf limit=%"PRId32" prepared=%"PRId32 ahci_dma_prepare_buf_fail(void *s, int port) "ahci(%p)[%d]: sglist population failed" diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h index 88212f59df..f3de6f9b73 100644 --- a/include/hw/ide/internal.h +++ b/include/hw/ide/internal.h @@ -444,7 +444,7 @@ struct IDEState { struct IDEDMAOps { DMAStartFunc *start_dma; - DMAVoidFunc *start_transfer; + DMAVoidFunc *pio_transfer; DMAInt32Func *prepare_buf; DMAu32Func *commit_buf; DMAIntFunc *rw_buf;
Now that end_transfer_func is a tail call in ahci_start_transfer, formalize the fact that the callback (of which ahci_start_transfer is the sole implementation) takes care of the transfer too: rename it to pio_transfer and, if it is present, call the end_transfer_func as soon as it returns. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/ide/ahci.c | 15 +++++++-------- hw/ide/core.c | 8 +++++--- hw/ide/trace-events | 2 +- include/hw/ide/internal.h | 2 +- 4 files changed, 14 insertions(+), 13 deletions(-)