[PATCH v2 00/16] [for-9.0] esp: avoid explicit setting of DRQ within ESP state machine

[PATCH v2 00/16] [for-9.0] esp: avoid explicit setting of DRQ within ESP state machine

[Mark Cave-Ayland]

[MCA: Since v1 I've received a few reports of FIFO assert()s being triggered and a
cmdfifo buffer overflow discovered by fuzzing the updated ESP code. The updating of
all FIFO push/pop operations to use the esp_fifo_*() functions in this series
provides protection against this, and in conjunction with new patches 9-11 is
enough to prevent all qtest reproducers that I have been sent.

For these reasons I would recommend that this series be applied for 9.0. Many thanks
to Chuhong Yuan <hslester96@gmail.com> for reporting the issues and providing qtest
reproducers.]

The ESP device has a DRQ (DMA request) signal that is used to handle flow control
during DMA transfers. At the moment the DRQ signal is explicitly raised and
lowered at various points in the ESP state machine as required, rather than
implementing the logic described in the datasheet:

"DREQ will remain true as long as either the FIFO contains at least one word (or
one byte if 8-bit mode) to send to memory during DMA read, or has room for one more
word (or byte if 8-bit mode) in the FIFO during DMA write."

This series updates the ESP device to use the same logic as described above which
also fixes a couple of outstanding GitLab issues related to recovery handling
during FIFO overflow on 68k Macs using PDMA.

Patches 1-3 update existing users of esp_fifo_pop_buf() and esp_fifo_pop() with
the internal cmdfifo to use the underlying Fifo8 device directly. The aim is
to ensure that all the esp_fifo_*() functions only operate on the ESP's hardware
FIFO.

Patches 4-5 update esp_fifo_push() and esp_fifo_pop() to take ESPState directly
as a parameter to prevent any usage that doesn't reference the ESP hardware
FIFO.

Patch 6 ensures that any usage of fifo8_push() for the ESP hardware FIFO is
updated to use esp_fifo_push() instead.

Patch 7 updates esp_fifo_pop_buf() to take ESPState directly as a parameter
to prevent any usage that doesn't reference the ESP hardware FIFO.

Patch 8 introduces the esp_fifo_push_buf() function for pushing multiple bytes
to the ESP hardware FIFO, and updates callers to use it accordingly.

Patches 9-11 incorporate additional protection to prevent FIFO assert()s and a
cmdfifo buffer overflow discovered via fuzzing.

Patch 12 is just code movement which avoids the use of a forward declaration whilst
also making it easier to locate the mapping between ESP SCSI phases and their
names.

Patches 13 introduce a new esp_update_drq() function that implements the above
DRQ logic which is called by both esp_fifo_{push,pop}_buf().

Patch 14 updates esp_fifo_push() and esp_fifo_pop() to use the new esp_update_drq()
function. At this point all reads/writes to the ESP FIFO use the esp_fifo_*()
functions and will set DRQ correctly.

Patch 15 is a small update to the logic in esp_pdma_write() to ensure that
esp_fifo_push() is always called for PDMA writes to the FIFO, thereby ensuring
that esp_update_drq() remains correct even in the case of FIFO overflow.

Finally patch 16 removes all manual calls to esp_raise_drq() and esp_lower_drq()
since the DRQ signal is now updated correctly upon each FIFO read/write access.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

v2:
- Rebase onto master
- Add patches 9-12 to handle FIFO assert()s and cmdfifo overflow as reported by
  Chuhong Yuan <hslester96@gmail.com>


Mark Cave-Ayland (16):
  esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_command_phase()
  esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_message_phase()
  esp.c: replace cmdfifo use of esp_fifo_pop() in do_message_phase()
  esp.c: change esp_fifo_push() to take ESPState
  esp.c: change esp_fifo_pop() to take ESPState
  esp.c: use esp_fifo_push() instead of fifo8_push()
  esp.c: change esp_fifo_pop_buf() to take ESPState
  esp.c: introduce esp_fifo_push_buf() function for pushing to the FIFO
  esp.c: don't assert() if FIFO empty when executing non-DMA SELATNS
  esp.c: don't assert() if FIFO empty when executing esp_cdb_length()
  esp.c: don't overflow cmdfifo if cmdfifo_cdb_offset >= ESP_CMDFIFO_SZ
  esp.c: move esp_set_phase() and esp_get_phase() towards the beginning
    of the file
  esp.c: introduce esp_update_drq() and update esp_fifo_{push,pop}_buf()
    to use it
  esp.c: update esp_fifo_{push,pop}() to call esp_update_drq()
  esp.c: ensure esp_pdma_write() always calls esp_fifo_push()
  esp.c: remove explicit setting of DRQ within ESP state machine

 hw/scsi/esp.c | 193 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 123 insertions(+), 70 deletions(-)

-- 
2.39.2

[PATCH v2 01/16] esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_command_phase()

[Mark Cave-Ayland]

The aim is to restrict the esp_fifo_*() functions so that they only operate on
the hardware FIFO. When reading from cmdfifo in do_command_phase() use the
underlying Fifo8 functions directly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 590ff99744..f8230c74b3 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -265,7 +265,7 @@ static void esp_do_nodma(ESPState *s);
 
 static void do_command_phase(ESPState *s)
 {
-    uint32_t cmdlen;
+    uint32_t cmdlen, n;
     int32_t datalen;
     SCSIDevice *current_lun;
     uint8_t buf[ESP_CMDFIFO_SZ];
@@ -275,7 +275,7 @@ static void do_command_phase(ESPState *s)
     if (!cmdlen || !s->current_dev) {
         return;
     }
-    esp_fifo_pop_buf(&s->cmdfifo, buf, cmdlen);
+    memcpy(buf, fifo8_pop_buf(&s->cmdfifo, cmdlen, &n), cmdlen);
 
     current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, s->lun);
     if (!current_lun) {
-- 
2.39.2

[PATCH v2 02/16] esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_message_phase()

[Mark Cave-Ayland]

The aim is to restrict the esp_fifo_*() functions so that they only operate on
the hardware FIFO. When reading from cmdfifo in do_message_phase() use the
underlying Fifo8 functions directly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index f8230c74b3..100560244b 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -309,6 +309,8 @@ static void do_command_phase(ESPState *s)
 
 static void do_message_phase(ESPState *s)
 {
+    uint32_t n;
+
     if (s->cmdfifo_cdb_offset) {
         uint8_t message = esp_fifo_pop(&s->cmdfifo);
 
@@ -320,7 +322,10 @@ static void do_message_phase(ESPState *s)
     /* Ignore extended messages for now */
     if (s->cmdfifo_cdb_offset) {
         int len = MIN(s->cmdfifo_cdb_offset, fifo8_num_used(&s->cmdfifo));
-        esp_fifo_pop_buf(&s->cmdfifo, NULL, len);
+
+        if (len) {
+            fifo8_pop_buf(&s->cmdfifo, len, &n);
+        }
         s->cmdfifo_cdb_offset = 0;
     }
 }
-- 
2.39.2

[PATCH v2 03/16] esp.c: replace cmdfifo use of esp_fifo_pop() in do_message_phase()

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 100560244b..7a24515bb9 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -312,7 +312,8 @@ static void do_message_phase(ESPState *s)
     uint32_t n;
 
     if (s->cmdfifo_cdb_offset) {
-        uint8_t message = esp_fifo_pop(&s->cmdfifo);
+        uint8_t message = fifo8_is_empty(&s->cmdfifo) ? 0 :
+                          fifo8_pop(&s->cmdfifo);
 
         trace_esp_do_identify(message);
         s->lun = message & 7;
-- 
2.39.2

[PATCH v2 04/16] esp.c: change esp_fifo_push() to take ESPState

[Mark Cave-Ayland]

Now that all users of esp_fifo_push() operate on the main FIFO there is no need
to pass the FIFO explicitly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 7a24515bb9..b898e43e2b 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -106,14 +106,14 @@ void esp_request_cancelled(SCSIRequest *req)
     }
 }
 
-static void esp_fifo_push(Fifo8 *fifo, uint8_t val)
+static void esp_fifo_push(ESPState *s, uint8_t val)
 {
-    if (fifo8_num_used(fifo) == fifo->capacity) {
+    if (fifo8_num_used(&s->fifo) == s->fifo.capacity) {
         trace_esp_error_fifo_overrun();
         return;
     }
 
-    fifo8_push(fifo, val);
+    fifo8_push(&s->fifo, val);
 }
 
 static uint8_t esp_fifo_pop(Fifo8 *fifo)
@@ -224,7 +224,7 @@ static void esp_pdma_write(ESPState *s, uint8_t val)
         return;
     }
 
-    esp_fifo_push(&s->fifo, val);
+    esp_fifo_push(s, val);
 
     dmalen--;
     esp_set_tc(s, dmalen);
@@ -1240,7 +1240,7 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val)
         break;
     case ESP_FIFO:
         if (!fifo8_is_full(&s->fifo)) {
-            esp_fifo_push(&s->fifo, val);
+            esp_fifo_push(s, val);
         }
         esp_do_nodma(s);
         break;
-- 
2.39.2

[PATCH v2 05/16] esp.c: change esp_fifo_pop() to take ESPState

[Mark Cave-Ayland]

Now that all users of esp_fifo_pop() operate on the main FIFO there is no need
to pass the FIFO explicitly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index b898e43e2b..0e42ff50e7 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -116,13 +116,13 @@ static void esp_fifo_push(ESPState *s, uint8_t val)
     fifo8_push(&s->fifo, val);
 }
 
-static uint8_t esp_fifo_pop(Fifo8 *fifo)
+static uint8_t esp_fifo_pop(ESPState *s)
 {
-    if (fifo8_is_empty(fifo)) {
+    if (fifo8_is_empty(&s->fifo)) {
         return 0;
     }
 
-    return fifo8_pop(fifo);
+    return fifo8_pop(&s->fifo);
 }
 
 static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
@@ -212,7 +212,7 @@ static uint8_t esp_pdma_read(ESPState *s)
 {
     uint8_t val;
 
-    val = esp_fifo_pop(&s->fifo);
+    val = esp_fifo_pop(s);
     return val;
 }
 
@@ -1184,7 +1184,7 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr)
 
     switch (saddr) {
     case ESP_FIFO:
-        s->rregs[ESP_FIFO] = esp_fifo_pop(&s->fifo);
+        s->rregs[ESP_FIFO] = esp_fifo_pop(s);
         val = s->rregs[ESP_FIFO];
         break;
     case ESP_RINTR:
-- 
2.39.2

[PATCH v2 06/16] esp.c: use esp_fifo_push() instead of fifo8_push()

[Mark Cave-Ayland]

There are still a few places that use fifo8_push() instead of esp_fifo_push() in
order to push a value into the FIFO. Update those places to use esp_fifo_push()
instead.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 0e42ff50e7..fb2ceca36a 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -858,7 +858,7 @@ static void esp_do_nodma(ESPState *s)
             return;
         }
         if (fifo8_is_empty(&s->fifo)) {
-            fifo8_push(&s->fifo, s->async_buf[0]);
+            esp_fifo_push(s, s->async_buf[0]);
             s->async_buf++;
             s->async_len--;
             s->ti_size--;
@@ -881,7 +881,7 @@ static void esp_do_nodma(ESPState *s)
     case STAT_ST:
         switch (s->rregs[ESP_CMD]) {
         case CMD_ICCS:
-            fifo8_push(&s->fifo, s->status);
+            esp_fifo_push(s, s->status);
             esp_set_phase(s, STAT_MI);
 
             /* Process any message in phase data */
@@ -893,7 +893,7 @@ static void esp_do_nodma(ESPState *s)
     case STAT_MI:
         switch (s->rregs[ESP_CMD]) {
         case CMD_ICCS:
-            fifo8_push(&s->fifo, 0);
+            esp_fifo_push(s, 0);
 
             /* Raise end of command interrupt */
             s->rregs[ESP_RINTR] |= INTR_FC;
-- 
2.39.2

[PATCH v2 07/16] esp.c: change esp_fifo_pop_buf() to take ESPState

[Mark Cave-Ayland]

Now that all users of esp_fifo_pop_buf() operate on the main FIFO there is no
need to pass the FIFO explicitly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index fb2ceca36a..4d9220ab22 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -125,7 +125,7 @@ static uint8_t esp_fifo_pop(ESPState *s)
     return fifo8_pop(&s->fifo);
 }
 
-static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
+static uint32_t esp_fifo_pop_buf(ESPState *s, uint8_t *dest, int maxlen)
 {
     const uint8_t *buf;
     uint32_t n, n2;
@@ -136,16 +136,16 @@ static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
     }
 
     len = maxlen;
-    buf = fifo8_pop_buf(fifo, len, &n);
+    buf = fifo8_pop_buf(&s->fifo, len, &n);
     if (dest) {
         memcpy(dest, buf, n);
     }
 
     /* Add FIFO wraparound if needed */
     len -= n;
-    len = MIN(len, fifo8_num_used(fifo));
+    len = MIN(len, fifo8_num_used(&s->fifo));
     if (len) {
-        buf = fifo8_pop_buf(fifo, len, &n2);
+        buf = fifo8_pop_buf(&s->fifo, len, &n2);
         if (dest) {
             memcpy(&dest[n], buf, n2);
         }
@@ -459,7 +459,7 @@ static void esp_do_dma(ESPState *s)
             s->dma_memory_read(s->dma_opaque, buf, len);
             esp_set_tc(s, esp_get_tc(s) - len);
         } else {
-            len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
             esp_raise_drq(s);
         }
@@ -515,7 +515,7 @@ static void esp_do_dma(ESPState *s)
             fifo8_push_all(&s->cmdfifo, buf, len);
             esp_set_tc(s, esp_get_tc(s) - len);
         } else {
-            len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
             fifo8_push_all(&s->cmdfifo, buf, len);
             esp_raise_drq(s);
@@ -549,7 +549,7 @@ static void esp_do_dma(ESPState *s)
                 /* Copy FIFO data to device */
                 len = MIN(s->async_len, ESP_FIFO_SZ);
                 len = MIN(len, fifo8_num_used(&s->fifo));
-                len = esp_fifo_pop_buf(&s->fifo, s->async_buf, len);
+                len = esp_fifo_pop_buf(s, s->async_buf, len);
                 esp_raise_drq(s);
             }
 
@@ -713,7 +713,7 @@ static void esp_nodma_ti_dataout(ESPState *s)
     }
     len = MIN(s->async_len, ESP_FIFO_SZ);
     len = MIN(len, fifo8_num_used(&s->fifo));
-    esp_fifo_pop_buf(&s->fifo, s->async_buf, len);
+    esp_fifo_pop_buf(s, s->async_buf, len);
     s->async_buf += len;
     s->async_len -= len;
     s->ti_size += len;
@@ -738,7 +738,7 @@ static void esp_do_nodma(ESPState *s)
         switch (s->rregs[ESP_CMD]) {
         case CMD_SELATN:
             /* Copy FIFO into cmdfifo */
-            len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
             fifo8_push_all(&s->cmdfifo, buf, len);
 
@@ -757,7 +757,7 @@ static void esp_do_nodma(ESPState *s)
 
         case CMD_SELATNS:
             /* Copy one byte from FIFO into cmdfifo */
-            len = esp_fifo_pop_buf(&s->fifo, buf, 1);
+            len = esp_fifo_pop_buf(s, buf, 1);
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
             fifo8_push_all(&s->cmdfifo, buf, len);
 
@@ -774,7 +774,7 @@ static void esp_do_nodma(ESPState *s)
 
         case CMD_TI:
             /* Copy FIFO into cmdfifo */
-            len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
             fifo8_push_all(&s->cmdfifo, buf, len);
 
@@ -792,7 +792,7 @@ static void esp_do_nodma(ESPState *s)
         switch (s->rregs[ESP_CMD]) {
         case CMD_TI:
             /* Copy FIFO into cmdfifo */
-            len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
             fifo8_push_all(&s->cmdfifo, buf, len);
 
@@ -821,7 +821,7 @@ static void esp_do_nodma(ESPState *s)
         case CMD_SEL | CMD_DMA:
         case CMD_SELATN | CMD_DMA:
             /* Copy FIFO into cmdfifo */
-            len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
             fifo8_push_all(&s->cmdfifo, buf, len);
 
@@ -836,7 +836,7 @@ static void esp_do_nodma(ESPState *s)
         case CMD_SEL:
         case CMD_SELATN:
             /* FIFO already contain entire CDB: copy to cmdfifo and execute */
-            len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
             fifo8_push_all(&s->cmdfifo, buf, len);
 
-- 
2.39.2

[PATCH v2 08/16] esp.c: introduce esp_fifo_push_buf() function for pushing to the FIFO

[Mark Cave-Ayland]

Instead of pushing data into the FIFO directly with fifo8_push_all(), add a new
esp_fifo_push_buf() function and use it accordingly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 4d9220ab22..6b7a972947 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -116,6 +116,11 @@ static void esp_fifo_push(ESPState *s, uint8_t val)
     fifo8_push(&s->fifo, val);
 }
 
+static void esp_fifo_push_buf(ESPState *s, uint8_t *buf, int len)
+{
+    fifo8_push_all(&s->fifo, buf, len);
+}
+
 static uint8_t esp_fifo_pop(ESPState *s)
 {
     if (fifo8_is_empty(&s->fifo)) {
@@ -601,7 +606,7 @@ static void esp_do_dma(ESPState *s)
             } else {
                 /* Copy device data to FIFO */
                 len = MIN(len, fifo8_num_free(&s->fifo));
-                fifo8_push_all(&s->fifo, s->async_buf, len);
+                esp_fifo_push_buf(s, s->async_buf, len);
                 esp_raise_drq(s);
             }
 
@@ -650,7 +655,7 @@ static void esp_do_dma(ESPState *s)
                 if (s->dma_memory_write) {
                     s->dma_memory_write(s->dma_opaque, buf, len);
                 } else {
-                    fifo8_push_all(&s->fifo, buf, len);
+                    esp_fifo_push_buf(s, buf, len);
                 }
 
                 esp_set_tc(s, esp_get_tc(s) - len);
@@ -685,7 +690,7 @@ static void esp_do_dma(ESPState *s)
                 if (s->dma_memory_write) {
                     s->dma_memory_write(s->dma_opaque, buf, len);
                 } else {
-                    fifo8_push_all(&s->fifo, buf, len);
+                    esp_fifo_push_buf(s, buf, len);
                 }
 
                 esp_set_tc(s, esp_get_tc(s) - len);
-- 
2.39.2

[PATCH v2 09/16] esp.c: don't assert() if FIFO empty when executing non-DMA SELATNS

[Mark Cave-Ayland]

The current logic assumes that at least 1 byte is present in the FIFO when
executing a non-DMA SELATNS command, but this may not be the case if the
guest executes an invalid ESP command sequence.

Reported-by: Chuhong Yuan <hslester96@gmail.com>
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 6b7a972947..55143a1208 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -762,7 +762,8 @@ static void esp_do_nodma(ESPState *s)
 
         case CMD_SELATNS:
             /* Copy one byte from FIFO into cmdfifo */
-            len = esp_fifo_pop_buf(s, buf, 1);
+            len = esp_fifo_pop_buf(s, buf,
+                                   MIN(fifo8_num_used(&s->fifo), 1));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
             fifo8_push_all(&s->cmdfifo, buf, len);
 
-- 
2.39.2

[PATCH v2 10/16] esp.c: don't assert() if FIFO empty when executing esp_cdb_length()

[Mark Cave-Ayland]

This does not happen during normal usage, but can occur if the guest issues an
invalid ESP command sequence.

Reported-by: Chuhong Yuan <hslester96@gmail.com>
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 55143a1208..0050493e18 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -431,7 +431,7 @@ static int esp_cdb_length(ESPState *s)
     int cmdlen, len;
 
     cmdlen = fifo8_num_used(&s->cmdfifo);
-    if (cmdlen < s->cmdfifo_cdb_offset) {
+    if (cmdlen == 0 || cmdlen < s->cmdfifo_cdb_offset) {
         return 0;
     }
 
-- 
2.39.2

[PATCH v2 11/16] esp.c: don't overflow cmdfifo if cmdfifo_cdb_offset >= ESP_CMDFIFO_SZ

[Mark Cave-Ayland]

If cmdfifo contains ESP_CMDFIFO_SZ bytes and cmdfifo_cdb_offset is also
ESP_CMDFIFO_SZ then if the guest issues an ESP command sequence that invokes
esp_cdb_length(), scsi_cdb_length() can read one byte beyond the end of the
FIFO buffer.

Add an extra length check to esp_cdb_length() to prevent reading past the
end of the cmdfifo data in this case.

Reported-by: Chuhong Yuan <hslester96@gmail.com>
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 0050493e18..05784b3f77 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -431,7 +431,8 @@ static int esp_cdb_length(ESPState *s)
     int cmdlen, len;
 
     cmdlen = fifo8_num_used(&s->cmdfifo);
-    if (cmdlen == 0 || cmdlen < s->cmdfifo_cdb_offset) {
+    if (cmdlen == 0 || cmdlen < s->cmdfifo_cdb_offset ||
+            cmdlen >= ESP_CMDFIFO_SZ) {
         return 0;
     }
 
-- 
2.39.2

[PATCH v2 12/16] esp.c: move esp_set_phase() and esp_get_phase() towards the beginning of the file

[Mark Cave-Ayland]

This allows these functions to be used earlier in the file without needing a
separate forward declaration.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 05784b3f77..86145256ea 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -79,6 +79,24 @@ static void esp_lower_drq(ESPState *s)
     }
 }
 
+static const char *esp_phase_names[8] = {
+    "DATA OUT", "DATA IN", "COMMAND", "STATUS",
+    "(reserved)", "(reserved)", "MESSAGE OUT", "MESSAGE IN"
+};
+
+static void esp_set_phase(ESPState *s, uint8_t phase)
+{
+    s->rregs[ESP_RSTAT] &= ~7;
+    s->rregs[ESP_RSTAT] |= phase;
+
+    trace_esp_set_phase(esp_phase_names[phase]);
+}
+
+static uint8_t esp_get_phase(ESPState *s)
+{
+    return s->rregs[ESP_RSTAT] & 7;
+}
+
 void esp_dma_enable(ESPState *s, int irq, int level)
 {
     if (level) {
@@ -195,24 +213,6 @@ static uint32_t esp_get_stc(ESPState *s)
     return dmalen;
 }
 
-static const char *esp_phase_names[8] = {
-    "DATA OUT", "DATA IN", "COMMAND", "STATUS",
-    "(reserved)", "(reserved)", "MESSAGE OUT", "MESSAGE IN"
-};
-
-static void esp_set_phase(ESPState *s, uint8_t phase)
-{
-    s->rregs[ESP_RSTAT] &= ~7;
-    s->rregs[ESP_RSTAT] |= phase;
-
-    trace_esp_set_phase(esp_phase_names[phase]);
-}
-
-static uint8_t esp_get_phase(ESPState *s)
-{
-    return s->rregs[ESP_RSTAT] & 7;
-}
-
 static uint8_t esp_pdma_read(ESPState *s)
 {
     uint8_t val;
-- 
2.39.2

[PATCH v2 13/16] esp.c: introduce esp_update_drq() and update esp_fifo_{push, pop}_buf() to use it

[Mark Cave-Ayland]

This new function sets the DRQ line correctly according to the current transfer
mode, direction and FIFO contents. Update esp_fifo_push_buf() and esp_fifo_pop_buf()
to use it so that DRQ is always set correctly when reading/writing multiple bytes
to/from the FIFO.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 86145256ea..53a1c7ceaf 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -124,6 +124,48 @@ void esp_request_cancelled(SCSIRequest *req)
     }
 }
 
+static void esp_update_drq(ESPState *s)
+{
+    bool to_device;
+
+    switch (esp_get_phase(s)) {
+    case STAT_MO:
+    case STAT_CD:
+    case STAT_DO:
+        to_device = true;
+        break;
+
+    case STAT_DI:
+    case STAT_ST:
+    case STAT_MI:
+        to_device = false;
+        break;
+
+    default:
+        return;
+    }
+
+    if (s->dma) {
+        /* DMA request so update DRQ according to transfer direction */
+        if (to_device) {
+            if (fifo8_num_free(&s->fifo) < 2) {
+                esp_lower_drq(s);
+            } else {
+                esp_raise_drq(s);
+            }
+        } else {
+            if (fifo8_num_used(&s->fifo) < 2) {
+                esp_lower_drq(s);
+            } else {
+                esp_raise_drq(s);
+            }
+        }
+    } else {
+        /* Not a DMA request */
+        esp_lower_drq(s);
+    }
+}
+
 static void esp_fifo_push(ESPState *s, uint8_t val)
 {
     if (fifo8_num_used(&s->fifo) == s->fifo.capacity) {
@@ -137,6 +179,7 @@ static void esp_fifo_push(ESPState *s, uint8_t val)
 static void esp_fifo_push_buf(ESPState *s, uint8_t *buf, int len)
 {
     fifo8_push_all(&s->fifo, buf, len);
+    esp_update_drq(s);
 }
 
 static uint8_t esp_fifo_pop(ESPState *s)
@@ -155,6 +198,7 @@ static uint32_t esp_fifo_pop_buf(ESPState *s, uint8_t *dest, int maxlen)
     int len;
 
     if (maxlen == 0) {
+        esp_update_drq(s);
         return 0;
     }
 
@@ -175,6 +219,7 @@ static uint32_t esp_fifo_pop_buf(ESPState *s, uint8_t *dest, int maxlen)
         n += n2;
     }
 
+    esp_update_drq(s);
     return n;
 }
 
-- 
2.39.2

[PATCH v2 14/16] esp.c: update esp_fifo_{push, pop}() to call esp_update_drq()

[Mark Cave-Ayland]

This ensures that the DRQ line is always set correctly when reading/writing
single bytes to/from the FIFO.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 53a1c7ceaf..52a69599b2 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -170,10 +170,11 @@ static void esp_fifo_push(ESPState *s, uint8_t val)
 {
     if (fifo8_num_used(&s->fifo) == s->fifo.capacity) {
         trace_esp_error_fifo_overrun();
-        return;
+    } else {
+        fifo8_push(&s->fifo, val);
     }
 
-    fifo8_push(&s->fifo, val);
+    esp_update_drq(s);
 }
 
 static void esp_fifo_push_buf(ESPState *s, uint8_t *buf, int len)
@@ -184,11 +185,16 @@ static void esp_fifo_push_buf(ESPState *s, uint8_t *buf, int len)
 
 static uint8_t esp_fifo_pop(ESPState *s)
 {
+    uint8_t val;
+
     if (fifo8_is_empty(&s->fifo)) {
-        return 0;
+        val = 0;
+    } else {
+        val = fifo8_pop(&s->fifo);
     }
 
-    return fifo8_pop(&s->fifo);
+    esp_update_drq(s);
+    return val;
 }
 
 static uint32_t esp_fifo_pop_buf(ESPState *s, uint8_t *dest, int maxlen)
-- 
2.39.2

[PATCH v2 15/16] esp.c: ensure esp_pdma_write() always calls esp_fifo_push()

[Mark Cave-Ayland]

This ensures that esp_update_drq() is called via esp_fifo_push() whenever the
host uses PDMA to transfer data to a SCSI device.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 52a69599b2..68346ceaeb 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -276,14 +276,12 @@ static void esp_pdma_write(ESPState *s, uint8_t val)
 {
     uint32_t dmalen = esp_get_tc(s);
 
-    if (dmalen == 0) {
-        return;
-    }
-
     esp_fifo_push(s, val);
 
-    dmalen--;
-    esp_set_tc(s, dmalen);
+    if (dmalen && s->drq_state) {
+        dmalen--;
+        esp_set_tc(s, dmalen);
+    }
 }
 
 static int esp_select(ESPState *s)
-- 
2.39.2

[PATCH v2 16/16] esp.c: remove explicit setting of DRQ within ESP state machine

[Mark Cave-Ayland]

Now the esp_update_drq() is called for all reads/writes to the FIFO, there is
no need to manually raise and lower the DRQ signal.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/611
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1831
---
 hw/scsi/esp.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 68346ceaeb..2b479b1b5a 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -496,7 +496,6 @@ static void esp_dma_ti_check(ESPState *s)
     if (esp_get_tc(s) == 0 && fifo8_num_used(&s->fifo) < 2) {
         s->rregs[ESP_RINTR] |= INTR_BS;
         esp_raise_irq(s);
-        esp_lower_drq(s);
     }
 }
 
@@ -516,7 +515,6 @@ static void esp_do_dma(ESPState *s)
         } else {
             len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
-            esp_raise_drq(s);
         }
 
         fifo8_push_all(&s->cmdfifo, buf, len);
@@ -573,7 +571,6 @@ static void esp_do_dma(ESPState *s)
             len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
             fifo8_push_all(&s->cmdfifo, buf, len);
-            esp_raise_drq(s);
         }
         trace_esp_handle_ti_cmd(cmdlen);
         s->ti_size = 0;
@@ -605,7 +602,6 @@ static void esp_do_dma(ESPState *s)
                 len = MIN(s->async_len, ESP_FIFO_SZ);
                 len = MIN(len, fifo8_num_used(&s->fifo));
                 len = esp_fifo_pop_buf(s, s->async_buf, len);
-                esp_raise_drq(s);
             }
 
             s->async_buf += len;
@@ -657,7 +653,6 @@ static void esp_do_dma(ESPState *s)
                 /* Copy device data to FIFO */
                 len = MIN(len, fifo8_num_free(&s->fifo));
                 esp_fifo_push_buf(s, s->async_buf, len);
-                esp_raise_drq(s);
             }
 
             s->async_buf += len;
@@ -723,7 +718,6 @@ static void esp_do_dma(ESPState *s)
             if (fifo8_num_used(&s->fifo) < 2) {
                 s->rregs[ESP_RINTR] |= INTR_BS;
                 esp_raise_irq(s);
-                esp_lower_drq(s);
             }
             break;
         }
@@ -1013,9 +1007,6 @@ void esp_command_complete(SCSIRequest *req, size_t resid)
     s->rregs[ESP_RINTR] |= INTR_BS;
     esp_raise_irq(s);
 
-    /* Ensure DRQ is set correctly for TC underflow or normal completion */
-    esp_dma_ti_check(s);
-
     if (s->current_req) {
         scsi_req_unref(s->current_req);
         s->current_req = NULL;
-- 
2.39.2

Re: [PATCH v2 03/16] esp.c: replace cmdfifo use of esp_fifo_pop() in do_message_phase()

[Philippe Mathieu-Daudé]

On 13/3/24 09:57, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 05/16] esp.c: change esp_fifo_pop() to take ESPState

[Philippe Mathieu-Daudé]

On 13/3/24 09:57, Mark Cave-Ayland wrote:
> Now that all users of esp_fifo_pop() operate on the main FIFO there is no need
> to pass the FIFO explicitly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 04/16] esp.c: change esp_fifo_push() to take ESPState

[Philippe Mathieu-Daudé]

On 13/3/24 09:57, Mark Cave-Ayland wrote:
> Now that all users of esp_fifo_push() operate on the main FIFO there is no need
> to pass the FIFO explicitly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 06/16] esp.c: use esp_fifo_push() instead of fifo8_push()

[Philippe Mathieu-Daudé]

On 13/3/24 09:58, Mark Cave-Ayland wrote:
> There are still a few places that use fifo8_push() instead of esp_fifo_push() in
> order to push a value into the FIFO. Update those places to use esp_fifo_push()
> instead.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 07/16] esp.c: change esp_fifo_pop_buf() to take ESPState

[Philippe Mathieu-Daudé]

On 13/3/24 09:58, Mark Cave-Ayland wrote:
> Now that all users of esp_fifo_pop_buf() operate on the main FIFO there is no
> need to pass the FIFO explicitly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 09/16] esp.c: don't assert() if FIFO empty when executing non-DMA SELATNS

[Philippe Mathieu-Daudé]

On 13/3/24 09:58, Mark Cave-Ayland wrote:
> The current logic assumes that at least 1 byte is present in the FIFO when
> executing a non-DMA SELATNS command, but this may not be the case if the
> guest executes an invalid ESP command sequence.
> 
> Reported-by: Chuhong Yuan <hslester96@gmail.com>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 08/16] esp.c: introduce esp_fifo_push_buf() function for pushing to the FIFO

[Philippe Mathieu-Daudé]

On 13/3/24 09:58, Mark Cave-Ayland wrote:
> Instead of pushing data into the FIFO directly with fifo8_push_all(), add a new
> esp_fifo_push_buf() function and use it accordingly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 12/16] esp.c: move esp_set_phase() and esp_get_phase() towards the beginning of the file

[Philippe Mathieu-Daudé]

On 13/3/24 09:58, Mark Cave-Ayland wrote:
> This allows these functions to be used earlier in the file without needing a
> separate forward declaration.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 36 ++++++++++++++++++------------------
>   1 file changed, 18 insertions(+), 18 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 13/16] esp.c: introduce esp_update_drq() and update esp_fifo_{push, pop}_buf() to use it

[Philippe Mathieu-Daudé]

On 13/3/24 09:58, Mark Cave-Ayland wrote:
> This new function sets the DRQ line correctly according to the current transfer
> mode, direction and FIFO contents. Update esp_fifo_push_buf() and esp_fifo_pop_buf()
> to use it so that DRQ is always set correctly when reading/writing multiple bytes
> to/from the FIFO.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 45 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 14/16] esp.c: update esp_fifo_{push, pop}() to call esp_update_drq()

[Philippe Mathieu-Daudé]

On 13/3/24 09:58, Mark Cave-Ayland wrote:
> This ensures that the DRQ line is always set correctly when reading/writing
> single bytes to/from the FIFO.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 16/16] esp.c: remove explicit setting of DRQ within ESP state machine

[Philippe Mathieu-Daudé]

On 13/3/24 09:58, Mark Cave-Ayland wrote:
> Now the esp_update_drq() is called for all reads/writes to the FIFO, there is
> no need to manually raise and lower the DRQ signal.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/611
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1831
> ---
>   hw/scsi/esp.c | 9 ---------
>   1 file changed, 9 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 01/16] esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_command_phase()

[Philippe Mathieu-Daudé]

On 13/3/24 09:57, Mark Cave-Ayland wrote:
> The aim is to restrict the esp_fifo_*() functions so that they only operate on
> the hardware FIFO. When reading from cmdfifo in do_command_phase() use the
> underlying Fifo8 functions directly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index 590ff99744..f8230c74b3 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -265,7 +265,7 @@ static void esp_do_nodma(ESPState *s);
>   
>   static void do_command_phase(ESPState *s)
>   {
> -    uint32_t cmdlen;
> +    uint32_t cmdlen, n;
>       int32_t datalen;
>       SCSIDevice *current_lun;
>       uint8_t buf[ESP_CMDFIFO_SZ];
> @@ -275,7 +275,7 @@ static void do_command_phase(ESPState *s)
>       if (!cmdlen || !s->current_dev) {
>           return;
>       }
> -    esp_fifo_pop_buf(&s->cmdfifo, buf, cmdlen);
> +    memcpy(buf, fifo8_pop_buf(&s->cmdfifo, cmdlen, &n), cmdlen);

'n' is unused, use NULL?

>   
>       current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, s->lun);
>       if (!current_lun) {

Re: [PATCH v2 02/16] esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_message_phase()

[Philippe Mathieu-Daudé]

On 13/3/24 09:57, Mark Cave-Ayland wrote:
> The aim is to restrict the esp_fifo_*() functions so that they only operate on
> the hardware FIFO. When reading from cmdfifo in do_message_phase() use the
> underlying Fifo8 functions directly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index f8230c74b3..100560244b 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -309,6 +309,8 @@ static void do_command_phase(ESPState *s)
>   
>   static void do_message_phase(ESPState *s)
>   {
> +    uint32_t n;
> +
>       if (s->cmdfifo_cdb_offset) {
>           uint8_t message = esp_fifo_pop(&s->cmdfifo);
>   
> @@ -320,7 +322,10 @@ static void do_message_phase(ESPState *s)
>       /* Ignore extended messages for now */
>       if (s->cmdfifo_cdb_offset) {
>           int len = MIN(s->cmdfifo_cdb_offset, fifo8_num_used(&s->cmdfifo));
> -        esp_fifo_pop_buf(&s->cmdfifo, NULL, len);
> +
> +        if (len) {
> +            fifo8_pop_buf(&s->cmdfifo, len, &n);

'n' is unused, use NULL?

> +        }
>           s->cmdfifo_cdb_offset = 0;
>       }
>   }

Re: [PATCH v2 15/16] esp.c: ensure esp_pdma_write() always calls esp_fifo_push()

[Philippe Mathieu-Daudé]

On 13/3/24 09:58, Mark Cave-Ayland wrote:
> This ensures that esp_update_drq() is called via esp_fifo_push() whenever the
> host uses PDMA to transfer data to a SCSI device.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 01/16] esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_command_phase()

[Mark Cave-Ayland]

On 13/03/2024 11:03, Philippe Mathieu-Daudé wrote:

> On 13/3/24 09:57, Mark Cave-Ayland wrote:
>> The aim is to restrict the esp_fifo_*() functions so that they only operate on
>> the hardware FIFO. When reading from cmdfifo in do_command_phase() use the
>> underlying Fifo8 functions directly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/scsi/esp.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>> index 590ff99744..f8230c74b3 100644
>> --- a/hw/scsi/esp.c
>> +++ b/hw/scsi/esp.c
>> @@ -265,7 +265,7 @@ static void esp_do_nodma(ESPState *s);
>>   static void do_command_phase(ESPState *s)
>>   {
>> -    uint32_t cmdlen;
>> +    uint32_t cmdlen, n;
>>       int32_t datalen;
>>       SCSIDevice *current_lun;
>>       uint8_t buf[ESP_CMDFIFO_SZ];
>> @@ -275,7 +275,7 @@ static void do_command_phase(ESPState *s)
>>       if (!cmdlen || !s->current_dev) {
>>           return;
>>       }
>> -    esp_fifo_pop_buf(&s->cmdfifo, buf, cmdlen);
>> +    memcpy(buf, fifo8_pop_buf(&s->cmdfifo, cmdlen, &n), cmdlen);
> 
> 'n' is unused, use NULL?

I was sure I had tried that before and it had failed, but I see that you made it work 
with NULL in commit cd04033dbe ("util/fifo8: Allow fifo8_pop_buf() to not populate 
popped length") - thanks!

I'll make the change for v3, but I'll wait a couple of days first to see if there are 
any further comments, in particular R-B tags for patches 10 and 11.

>>       current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, s->lun);
>>       if (!current_lun) {


ATB,

Mark.

Re: [PATCH v2 01/16] esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_command_phase()

[Philippe Mathieu-Daudé]

On 13/3/24 22:08, Mark Cave-Ayland wrote:
> On 13/03/2024 11:03, Philippe Mathieu-Daudé wrote:
> 
>> On 13/3/24 09:57, Mark Cave-Ayland wrote:
>>> The aim is to restrict the esp_fifo_*() functions so that they only 
>>> operate on
>>> the hardware FIFO. When reading from cmdfifo in do_command_phase() 
>>> use the
>>> underlying Fifo8 functions directly.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>   hw/scsi/esp.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>>> index 590ff99744..f8230c74b3 100644
>>> --- a/hw/scsi/esp.c
>>> +++ b/hw/scsi/esp.c
>>> @@ -265,7 +265,7 @@ static void esp_do_nodma(ESPState *s);
>>>   static void do_command_phase(ESPState *s)
>>>   {
>>> -    uint32_t cmdlen;
>>> +    uint32_t cmdlen, n;
>>>       int32_t datalen;
>>>       SCSIDevice *current_lun;
>>>       uint8_t buf[ESP_CMDFIFO_SZ];
>>> @@ -275,7 +275,7 @@ static void do_command_phase(ESPState *s)
>>>       if (!cmdlen || !s->current_dev) {
>>>           return;
>>>       }
>>> -    esp_fifo_pop_buf(&s->cmdfifo, buf, cmdlen);
>>> +    memcpy(buf, fifo8_pop_buf(&s->cmdfifo, cmdlen, &n), cmdlen);
>>
>> 'n' is unused, use NULL?
> 
> I was sure I had tried that before and it had failed, but I see that you 
> made it work with NULL in commit cd04033dbe ("util/fifo8: Allow 
> fifo8_pop_buf() to not populate popped length") - thanks!

Ah!

So using NULL in patches 1 and 2:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> I'll make the change for v3, but I'll wait a couple of days first to see 
> if there are any further comments, in particular R-B tags for patches 10 
> and 11.

I still have them in my TOREVIEW queue and need to digest them.

> 
>>>       current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, 
>>> s->lun);
>>>       if (!current_lun) {
> 
> 
> ATB,
> 
> Mark.
> 

Re: [PATCH v2 02/16] esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_message_phase()

[Philippe Mathieu-Daudé]

On 13/3/24 12:03, Philippe Mathieu-Daudé wrote:
> On 13/3/24 09:57, Mark Cave-Ayland wrote:
>> The aim is to restrict the esp_fifo_*() functions so that they only 
>> operate on
>> the hardware FIFO. When reading from cmdfifo in do_message_phase() use 
>> the
>> underlying Fifo8 functions directly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/scsi/esp.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>> index f8230c74b3..100560244b 100644
>> --- a/hw/scsi/esp.c
>> +++ b/hw/scsi/esp.c
>> @@ -309,6 +309,8 @@ static void do_command_phase(ESPState *s)
>>   static void do_message_phase(ESPState *s)
>>   {
>> +    uint32_t n;
>> +
>>       if (s->cmdfifo_cdb_offset) {
>>           uint8_t message = esp_fifo_pop(&s->cmdfifo);
>> @@ -320,7 +322,10 @@ static void do_message_phase(ESPState *s)
>>       /* Ignore extended messages for now */
>>       if (s->cmdfifo_cdb_offset) {
>>           int len = MIN(s->cmdfifo_cdb_offset, 
>> fifo8_num_used(&s->cmdfifo));
>> -        esp_fifo_pop_buf(&s->cmdfifo, NULL, len);
>> +
>> +        if (len) {
>> +            fifo8_pop_buf(&s->cmdfifo, len, &n);
> 
> 'n' is unused, use NULL?

Using NULL:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>