On 01/02/2024 10:46, Paolo Bonzini wrote:
On Fri, Jan 12, 2024 at 1:55 PM Mark Cave-Ayland
<mark.cave-ayl...@ilande.co.uk> wrote:
Invert the logic so that the end of DMA transfer check becomes one that checks
for TC == 0 in the from device path in do_dma_pdma_cb().
Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
---
hw/scsi/esp.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index fecfef7c89..63c828c1b2 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -602,21 +602,19 @@ static void do_dma_pdma_cb(ESPState *s)
return;
}
- if (esp_get_tc(s) != 0) {
- /* Copy device data to FIFO */
- len = MIN(s->async_len, esp_get_tc(s));
- len = MIN(len, fifo8_num_free(&s->fifo));
- fifo8_push_all(&s->fifo, s->async_buf, len);
- s->async_buf += len;
- s->async_len -= len;
- s->ti_size -= len;
- esp_set_tc(s, esp_get_tc(s) - len);
- return;
+ if (esp_get_tc(s) == 0) {
+ esp_lower_drq(s);
+ esp_dma_done(s);
}
I'm only doing a cursory review, but shouldn't there be a return here?
Paolo
(goes and looks)
Possibly there should, but my guess is that adding the return at that particular
point in time at the series broke one of my reference images. In particular MacOS is
notorious for requesting data transfers of X len, and setting the TC either too high
or too low and let the in-built OS recovery logic handle these cases.
The tricky part is that as per the cover note, making expected changes at an earlier
point in the series tends to cause things to break. Another thing to bear in mind is
that one of the main objectives of the series to completely remove all the
PDMA-specific callbacks including do_dma_pdma_cb(), so you'll see this function
disappear completely in a later patch.
It probably helps to compare the existing code at
https://gitlab.com/qemu-project/qemu/-/blob/master/hw/scsi/esp.c against the version
from this series at
https://gitlab.com/mcayland/qemu/-/blob/esp-rework-v2/hw/scsi/esp.c to get a feeling
where the series is going, as in order to keep my reference images bisectable the
journey from start to finish occurs in a fairly roundabout way.
- /* Partially filled a scsi buffer. Complete immediately. */
- esp_lower_drq(s);
- esp_dma_done(s);
+ /* Copy device data to FIFO */
+ len = MIN(s->async_len, esp_get_tc(s));
+ len = MIN(len, fifo8_num_free(&s->fifo));
+ fifo8_push_all(&s->fifo, s->async_buf, len);
+ s->async_buf += len;
+ s->async_len -= len;
+ s->ti_size -= len;
+ esp_set_tc(s, esp_get_tc(s) - len);
}
}
--
2.39.2
ATB,
Mark.
