Message ID | 20180223152640.11459-3-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
Series | None | expand |
On 02/23/2018 10:26 AM, Paolo Bonzini wrote: > The callback must be invoked once we get out of the DRQ phase; because > all end_transfer_funcs end up invoking ide_transfer_stop, call it there. > While at it, remove the "notify" argument from ide_transfer_halt; the > code can simply be moved to ide_transfer_stop. > > Old PATA controllers have no end_transfer callback, so there is no change > there. For AHCI the end_transfer_func already called ide_transfer_stop > so the effect is that the PIO FIS is written before the D2H FIS rather > than after, which is arguably an improvement. > MMK, so this is the "PIO Setup FIS" which I... actually, I was never sure what role this was supposed to play *exactly*. "Used by the device to provide the host adapter with sufficient information regarding a Programmed Input/Output (PIO) data phase to allow the host adapter to efficiently handle PIO data transfers." So in a perfect world, the SATA layer generates this *for* the AHCI adapter, but really both of those models are smooshed into one layer for us, so we just generate this so the host driver doesn't freak out. "For PIO data transfers, the device shall send to the host a PIO Setup – Device to Host FIS just before each and every data transfer FIS that is required to complete the data transfer. Data transfers from Host to Device as well as data transfers from Device to Host shall follow this algorithm." Well, we don't emulate Data FIS packets and I don't *think* those get cached in the received FIS data structure area for the AHCI HBA, so we just skip those. "Because of the stringent timing constraints in the ATA Standard, the PIO Setup FIS includes both the starting and ending status values. These are used by the host adapter to first signal to host software readiness for PIO write data (BSY bit is cleared to zero and DRQ bit is set to one), and following the PIO write burst to properly signal host software by clearing the DRQ bit to zero and possibly setting the BSY bit to one." This part confuses me. The starting and ending status values? How would we know the ending status value if this gets sent by the device before a transfer? > However, ahci_end_transfer is now called _after_ the DRQ_STAT has been > cleared, so remove the status check in ahci_end_transfer; it was only > there to ensure the FIS was not written more than once, and this cannot > happen anymore. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/ide/ahci.c | 7 ++----- > hw/ide/core.c | 15 +++++++-------- > 2 files changed, 9 insertions(+), 13 deletions(-) > > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c > index 937bad55fb..c3c6843b76 100644 > --- a/hw/ide/ahci.c > +++ b/hw/ide/ahci.c > @@ -1326,12 +1326,9 @@ out: > static void ahci_end_transfer(IDEDMA *dma) > { > AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma); > - IDEState *s = &ad->port.ifs[0]; > > - if (!(s->status & DRQ_STAT)) { > - /* done with PIO send/receive */ > - ahci_write_fis_pio(ad, le32_to_cpu(ad->cur_cmd->status)); > - } > + /* done with PIO send/receive */ > + ahci_write_fis_pio(ad, le32_to_cpu(ad->cur_cmd->status)); > } > > static void ahci_start_dma(IDEDMA *dma, IDEState *s, > diff --git a/hw/ide/core.c b/hw/ide/core.c > index 92f4424dc3..c4710a6f55 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -544,7 +544,6 @@ void ide_transfer_start(IDEState *s, uint8_t *buf, int size, > } > s->bus->dma->ops->start_transfer(s->bus->dma); > end_transfer_func(s); > - s->bus->dma->ops->end_transfer(s->bus->dma); > } > > static void ide_cmd_done(IDEState *s) > @@ -555,26 +554,26 @@ static void ide_cmd_done(IDEState *s) > } > > static void ide_transfer_halt(IDEState *s, > - void(*end_transfer_func)(IDEState *), > - bool notify) > + void(*end_transfer_func)(IDEState *)) > { > s->end_transfer_func = end_transfer_func; > s->data_ptr = s->io_buffer; > s->data_end = s->io_buffer; > s->status &= ~DRQ_STAT; > - if (notify) { > - ide_cmd_done(s); > - } > } > > void ide_transfer_stop(IDEState *s) > { > - ide_transfer_halt(s, ide_transfer_stop, true); > + ide_transfer_halt(s, ide_transfer_stop); > + if (s->bus->dma->ops->end_transfer) { > + s->bus->dma->ops->end_transfer(s->bus->dma); > + } > + ide_cmd_done(s); > } > > static void ide_transfer_cancel(IDEState *s) > { > - ide_transfer_halt(s, ide_transfer_cancel, false); > + ide_transfer_halt(s, ide_transfer_cancel); > } > > int64_t ide_get_sector(IDEState *s) > Seems sane, with some lingering questions about what the PIO Setup FIS is supposed to do to begin with still remaining.
On 20/03/2018 23:11, John Snow wrote: > Seems sane, with some lingering questions about what the PIO Setup FIS > is supposed to do to begin with still remaining. I agree here too. Smashing the ATA and controller in the same device makes things tricky, so I tried to make these patches pure code motion, as much as I could. Paolo
On 03/21/2018 01:39 AM, Paolo Bonzini wrote: > On 20/03/2018 23:11, John Snow wrote: >> Seems sane, with some lingering questions about what the PIO Setup FIS >> is supposed to do to begin with still remaining. > > I agree here too. Smashing the ATA and controller in the same device > makes things tricky, so I tried to make these patches pure code motion, > as much as I could. > > Paolo > Oh, I understand. There's a reason I haven't touched the PIO Setup FIS much when I cleaned the device up before. I was just hoping you'd magically know more about it. :)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 937bad55fb..c3c6843b76 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1326,12 +1326,9 @@ out: static void ahci_end_transfer(IDEDMA *dma) { AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma); - IDEState *s = &ad->port.ifs[0]; - if (!(s->status & DRQ_STAT)) { - /* done with PIO send/receive */ - ahci_write_fis_pio(ad, le32_to_cpu(ad->cur_cmd->status)); - } + /* done with PIO send/receive */ + ahci_write_fis_pio(ad, le32_to_cpu(ad->cur_cmd->status)); } static void ahci_start_dma(IDEDMA *dma, IDEState *s, diff --git a/hw/ide/core.c b/hw/ide/core.c index 92f4424dc3..c4710a6f55 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -544,7 +544,6 @@ void ide_transfer_start(IDEState *s, uint8_t *buf, int size, } s->bus->dma->ops->start_transfer(s->bus->dma); end_transfer_func(s); - s->bus->dma->ops->end_transfer(s->bus->dma); } static void ide_cmd_done(IDEState *s) @@ -555,26 +554,26 @@ static void ide_cmd_done(IDEState *s) } static void ide_transfer_halt(IDEState *s, - void(*end_transfer_func)(IDEState *), - bool notify) + void(*end_transfer_func)(IDEState *)) { s->end_transfer_func = end_transfer_func; s->data_ptr = s->io_buffer; s->data_end = s->io_buffer; s->status &= ~DRQ_STAT; - if (notify) { - ide_cmd_done(s); - } } void ide_transfer_stop(IDEState *s) { - ide_transfer_halt(s, ide_transfer_stop, true); + ide_transfer_halt(s, ide_transfer_stop); + if (s->bus->dma->ops->end_transfer) { + s->bus->dma->ops->end_transfer(s->bus->dma); + } + ide_cmd_done(s); } static void ide_transfer_cancel(IDEState *s) { - ide_transfer_halt(s, ide_transfer_cancel, false); + ide_transfer_halt(s, ide_transfer_cancel); } int64_t ide_get_sector(IDEState *s)
The callback must be invoked once we get out of the DRQ phase; because all end_transfer_funcs end up invoking ide_transfer_stop, call it there. While at it, remove the "notify" argument from ide_transfer_halt; the code can simply be moved to ide_transfer_stop. Old PATA controllers have no end_transfer callback, so there is no change there. For AHCI the end_transfer_func already called ide_transfer_stop so the effect is that the PIO FIS is written before the D2H FIS rather than after, which is arguably an improvement. However, ahci_end_transfer is now called _after_ the DRQ_STAT has been cleared, so remove the status check in ahci_end_transfer; it was only there to ensure the FIS was not written more than once, and this cannot happen anymore. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/ide/ahci.c | 7 ++----- hw/ide/core.c | 15 +++++++-------- 2 files changed, 9 insertions(+), 13 deletions(-)