[PATCH 00/88] esp: rework ESP emulation to use a SCSI phase-based state machine

[PATCH 00/88] esp: rework ESP emulation to use a SCSI phase-based state machine

[Mark Cave-Ayland]

The ESP SCSI chip fundamentally consists of a FIFO for transferring data to/from
the SCSI bus along with a command sequencer which automates various processes such
as selection, message/command transfer and data transfer. What makes this chip
particularly interesting is that the FIFO is also used as a buffer for DMA transfers
which makes it possible to mix and match DMA and non-DMA transfers when sending and
receiving data to/from the SCSI bus.

Whilst the current ESP emulation works well for most cases, there are several
problems that I've encountered whilst trying to debug various guest issues:


1) Multiple code paths for non-DMA, DMA and pDMA

Consider a SCSI request being submitted by the guest: if it is a DMA SCSI
request then it takes one path through get_cmd(), and a different path through
get_cmd() for pDMA. This then leads to the DMA code path being called for DMA
and the pDMA codepath being called for each FIFO write. Finally once you add
in the non-DMA path you end up with a total of 5 potential codepaths for any
given SCSI request.

2) Manual handling of STAT_TC flag

According to the datasheet the STAT_TC flag in ESP_RSTAT is set when the DMA
transfer counter reaches zero. The current code handles this manually in
multiple places, including where it shouldn't be necessary.

3) Deferred interrupts are used only for reads and not writes

The ESP emulation currently makes use of a deferred interrupt when submitting
read commands i.e. the interrupt flags are not updated immediately but when
the QEMU SCSI layer indicates it is ready. This works well for reads, but isn't
implemented for writes which can cause problems when the host is heavily loaded
or a large SCSI write is requested.

4) Padding of pDMA TI transfers to a minimum of 16 bytes

In order to allow Classic MacOS to boot successfully there is a workaround that
pads all pDMA TI transfers to a minimum of 16 bytes (see commit 7aa6baee7c
"esp: add support for unaligned accesses"). This feature is not documented and it
turns out that this is a symptom of incorrect handling of transfer
overflow/underflow conditions.

5) Duplication of some non-DMA logic in esp_reg_read() and esp_reg_write()

When reading and writing to the FIFO there is some duplication of logic from
esp_do_nodma() in esp_reg_read() and esp_reg_write(). This should be eliminated
in favour of a single state machine for handling non-DMA FIFO accesses in a
single place.


The series here reworks the ESP emulation to use a SCSI phase-based state machine
with just two paths: one for DMA/pDMA requests, and another for non-DMA requests.
The pDMA callbacks are completely removed so that there is now only a single path
for DMA requests. As part of this work the manual STAT_TC handling is removed, and a
couple of bugs in the SCSI phase selection are fixed to allow proper transfer
underflow/overflow detection and recovery. 

I've tested the series with all of my available ESP images and compatibility is
greatly improved with these changes: I believe that this series should also fix a
few GitLab issues including https://gitlab.com/qemu-project/qemu/-/issues/1831,
https://gitlab.com/qemu-project/qemu/-/issues/611 and
https://gitlab.com/qemu-project/qemu/-/issues/1127. In addition it allows Aurelien's
really old SPARC Linux images to boot once again, and also the NeXTCube machine can
now boot, load its kernel from disk and start to execute it.

The series breaks down into the following approximate sections:

  - Patches 1-6 do some initial tidy-up for ESP select commands

  - Patches 7-16 implement consolidation of the STAT_TC flag handling

  - Patches 17-36 start to bring esp_do_dma() and do_pdma_dma() into line with each
    other, removing unneeded hacks and fixing up end-of-phase logic

  - Patches 37-56 move the MESSAGE OUT and COMMAND phase processing to the new SCSI
    phase-based state machine, allowing the removal of the pDMA-specific callbacks

  - Patches 57-62 tidy up the end of SCSI command logic and adds deferred interrupt
    handling for the DATA IN phase to complement the existing DATA OUT phase
    implementation

  - Patches 63-64 remove the ti_cmd field which is now no longer required

  - Patches 65-67 move the last bit of MESSAGE OUT and COMMAND phase processing to
    the new SCSI phase-based state machine which allows get_cmd() to be removed

  - Patches 68-69 move the STATUS and MESSAGE IN phase processing to the new SCSI
    phase-based state machine

  - Patches 70-72 fix the reset of the ESP_RINTR/ESP_STAT registers which allows the
    TC underflow detection to work correctly

  - Patch 73 removes the restriction on FIFO read access if the DMA routines are not
    defined (which also allows the NeXTCube to pass its self-test)

  - Patches 74-77 update the state machine to allow more combinations of mixed TI
    and FIFO reads/writes exposed by my test images

  - Patches 78-88 are final tidy-ups and improvements, along with adding an
    implementation of the "Transfer Pad" command

There is a migration break for the Q800 machine here as the series removes the pDMA
subsection: this seems the safest option as it isn't possible to map the old pDMA
callback enum directly to the new state machine. In theory there is a small window
whereby the normal DMA state may miss setting data_ready correctly, but I haven't
been able to recreate this during my tests.


Notes for reviewers:

This series is not a particularly easy one to review without having good in-depth
knowledge of the ESP device. The biggest issue I found is that due to the state
machines used by the client upon receiving an interrupt, a change to a ESP register
in one place can cause a completely unrelated error elsewhere.

A prime example of this is the consolidation of the STAT_TC flag: updating the
emulated device to behave as described in the datasheet caused other failures until
other fixes/updates could be made. This is why the work is done over 2 separate
commits with a gap in-between to keep the series bisectible, clearly indicating how
sensitive this series is to commit order.

In order to implement the changes I decided to focus on 3 main test images that should
pass for each commit to keep the series bisectible:

1) Modern Linux kernel with am53c974.c (hppa)
2) Modern Linux kernel with mac_esp.c (q800)
3) Classic MacOS 8.0

Other images may stop working during parts of the series, which sometimes is the
only option as otherwise implementing a compatibility workaround would be
prohibitively time-consuming or impossible at that point in the series. By the
end of the series all of my test images work correctly, including a few extra that
don't work with QEMU current git master.

Finally it is worth reiterating that in its current form this series fixes several
significant bugs and improves the compatibility of the ESP emulation, and these changes
make future fixes considerably easier compared with the existing implementation.

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


Mark Cave-Ayland (88):
  esp: don't clear cmdfifo when esp_select() fails in get_cmd()
  esp: move existing request cancel check into esp_select()
  esp.c: add FIFO wraparound support to esp_fifo_pop_buf()
  esp: remove FIFO clear from esp_select()
  esp: move esp_select() to ESP selection commands from get_cmd()
  esp: update esp_set_tc() to set STAT_TC flag
  esp: start removal of manual STAT_TC setting when transfer counter
    reaches zero
  esp: move command execution logic to new esp_run_cmd() function
  esp: update TC check logic in do_dma_pdma_cb() to check for TC == 0
  esp: move buffer and TC logic into separate to/from device paths in
    esp_do_dma()
  esp.c: remove unused case from esp_pdma_read()
  esp.c: don't accumulate directly into cmdfifo
  esp.c: decrement the TC during MESSAGE OUT and COMMAND phases
  esp.c: introduce esp_set_phase() helper function
  esp.c: remove another set of manual STAT_TC updates
  esp.c: remove MacOS TI workaround that pads FIFO transfers to
    ESP_FIFO_SZ
  esp.c: don't reset the TC and ESP_RSEQ state when executing a SCSI
    command
  esp.c: don't clear RFLAGS register when DMA is complete
  esp: remove zero transfer size check from esp_do_dma()
  esp.c: update condition for esp_dma_done() in esp_do_dma() from device
    path
  esp.c: update condition for esp_dma_done() in esp_do_dma() to device
    path
  esp.c: ensure that the PDMA callback is called for every device read
  esp.c: don't immediately raise INTR_BS if SCSI data needed in
    esp_do_dma()
  esp.c: remove TC adjustment in esp_do_dma() from device path
  esp.c: remove unaligned adjustment in do_dma_pdma_cb() to device path
  esp.c: remove unneeded if() check in esp_transfer_data()
  esp.c: update end of transfer logic at the end of esp_transfer_data()
  esp.c: consolidate async_len and TC == 0 checks in do_dma_pdma_cb()
    and esp_do_dma()
  esp.c: fix premature end of phase logic esp_command_complete
  esp.c: move TC and FIFO check logic into esp_dma_done()
  esp.c: rename esp_dma_done() to esp_dma_ti_check()
  esp.c: copy PDMA logic for transfers to device from do_dma_pdma_cb()
    to esp_do_dma()
  esp.c: copy logic for do_cmd transfers from do_dma_pdma_cb() to
    esp_do_dma()
  esp.c: update esp_do_dma() bypass if async_len is zero to include
    non-zero transfer check
  esp.c: move end of SCSI transfer check after TC adjustment in
    do_dma_pdma_cb()
  esp.c: remove s_without_satn_pdma_cb() PDMA callback
  esp.c: introduce esp_get_phase() function
  esp.c: convert esp_do_dma() to switch statement based upon SCSI phase
  esp.c: convert do_dma_pdma_db() to switch statement based upon SCSI
    phase
  esp.c: convert esp_do_nodma() to switch statement based upon SCSI
    phase
  esp.c: convert esp_do_dma() do_cmd path to check for SCSI phase
    instead
  esp.c: convert do_dma_pdma_cb() do_cmd path to check for SCSI phase
    instead
  esp.c: convert esp_do_nodma() do_cmd path to check for SCSI phase
    instead
  esp.c: convert esp_reg_write() do_cmd path to check for SCSI phase
    instead
  esp.c: remove do_cmd from ESPState
  esp.c: untangle MESSAGE OUT and COMMAND phase logic in esp_do_dma()
  esp.c: untangle MESSAGE OUT and COMMAND phase logic in
    do_dma_pdma_cb()
  esp.c: untangle MESSAGE OUT and COMMAND phase logic in esp_do_nodma()
  esp.c: move CMD_SELATN end of message phase detection to esp_do_dma()
    and do_dma_pdma_cb()
  esp.c: move CMD_TI end of message phase detection to esp_do_dma() and
    do_dma_pdma_cb()
  esp.c: don't use get_cmd() for CMD_SEL DMA commands
  esp.c: move CMD_SELATNS end of command logic to esp_do_dma() and
    do_dma_pdma_cb()
  esp.c: replace do_dma_pdma_cb() with esp_do_dma()
  esp.c: move CMD_ICCS command logic to esp_do_dma()
  esp.c: always use esp_do_dma() in pdma_cb()
  esp.c: remove unused PDMA callback implementation
  esp.c: rename data_in_ready to to data_ready
  esp.c: separate logic based upon ESP command in esp_command_complete()
  esp.c: separate logic based upon ESP command in esp_transfer_data()
  esp.c: use deferred interrupts for both DATA IN and DATA OUT phases
  esp.c: remove DATA IN phase logic when reading from FIFO
  esp.c: zero command register when TI command terminates due to phase
    change
  esp.c: remove unneeded ti_cmd field
  esp.c: don't raise INTR_BS interrupt in DATA IN phase until TI command
    issued
  esp.c: move non-DMA TI logic to separate esp_nodma_ti_dataout()
    function
  esp.c: process non-DMA FIFO writes in esp_do_nodma()
  esp.c: replace get_cmd() with esp_do_nodma()
  esp.c: move write_response() non-DMA logic to esp_do_nodma()
  esp.c: consolidate end of command sequence after ICCS command
  esp.c: ensure that STAT_INT is cleared when reading ESP_RINTR
  esp.c: don't clear the SCSI phase when reading ESP_RINTR
  esp.c: handle TC underflow for DMA SCSI requests
  esp.c: remove restriction on FIFO read access when DMA memory routines
    defined
  esp.c: handle non-DMA FIFO writes used to terminate DMA commands
  esp.c: improve ESP_RSEQ logic consolidation
  esp.c: only transfer non-DMA COMMAND phase data for specific commands
  esp.c: only transfer non-DMA MESSAGE OUT phase data for specific
    commands
  esp.c: consolidate DMA and PDMA logic in DATA OUT phase
  esp.c: consolidate DMA and PDMA logic in DATA IN phase
  esp.c: consolidate DMA and PDMA logic in MESSAGE OUT phase
  esp.c: remove redundant n variable in PDMA COMMAND phase
  esp.c: consolidate DMA and PDMA logic in STATUS and MESSAGE IN phases
  esp.c: replace n variable with len in esp_do_nodma()
  esp.c: implement DMA Transfer Pad command for DATA phases
  esp.c: rename irq_data IRQ to drq_irq
  esp.c: keep track of the DRQ state during DMA
  esp.c: switch TypeInfo registration to use DEFINE_TYPES() macro
  esp.c: add my copyright to the file

 hw/scsi/esp.c         | 1371 ++++++++++++++++++++++-------------------
 hw/scsi/trace-events  |    1 +
 include/hw/scsi/esp.h |   18 +-
 3 files changed, 727 insertions(+), 663 deletions(-)

-- 
2.39.2

[PATCH 01/88] esp: don't clear cmdfifo when esp_select() fails in get_cmd()

[Mark Cave-Ayland]

The FIFO contents should not be affected if the target selection fails.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 3a1c9f7c3b..68d07edc05 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -252,11 +252,9 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
             fifo8_push_all(&s->cmdfifo, buf, dmalen);
         } else {
             if (esp_select(s) < 0) {
-                fifo8_reset(&s->cmdfifo);
                 return -1;
             }
             esp_raise_drq(s);
-            fifo8_reset(&s->cmdfifo);
             return 0;
         }
     } else {
@@ -271,7 +269,6 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
     trace_esp_get_cmd(dmalen, target);
 
     if (esp_select(s) < 0) {
-        fifo8_reset(&s->cmdfifo);
         return -1;
     }
     return dmalen;
-- 
2.39.2

Re: [PATCH 01/88] esp: don't clear cmdfifo when esp_select() fails in get_cmd()

[Helge Deller]

On 1/12/24 13:52, Mark Cave-Ayland wrote:
> The FIFO contents should not be affected if the target selection fails.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

I built and tested this series together with your "esp-pci: fixes for
Linux and MS-DOS" on top of git head and they fixed all outstanding issues.

So, you may add
Tested-by: Helge Deller <deller@gmx.de>
to both.

Thanks!
Helge

[PATCH 02/88] esp: move existing request cancel check into esp_select()

[Mark Cave-Ayland]

Since get_cmd() can be called multiple times during a mixed FIFO/DMA request,
move the existing request cancel check into esp_select() which always occurs
at the start of new SCSI request.

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 68d07edc05..b382865426 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -209,6 +209,11 @@ static int esp_select(ESPState *s)
     s->ti_size = 0;
     fifo8_reset(&s->fifo);
 
+    if (s->current_req) {
+        /* Started a new command before the old one finished. Cancel it. */
+        scsi_req_cancel(s->current_req);
+    }
+
     s->current_dev = scsi_device_find(&s->bus, 0, target, 0);
     if (!s->current_dev) {
         /* No such drive */
@@ -235,11 +240,6 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
     uint32_t dmalen, n;
     int target;
 
-    if (s->current_req) {
-        /* Started a new command before the old one finished.  Cancel it.  */
-        scsi_req_cancel(s->current_req);
-    }
-
     target = s->wregs[ESP_WBUSID] & BUSID_DID;
     if (s->dma) {
         dmalen = MIN(esp_get_tc(s), maxlen);
-- 
2.39.2

[PATCH 03/88] esp.c: add FIFO wraparound support to esp_fifo_pop_buf()

[Mark Cave-Ayland]

The fifo8_pop_buf() function returns a pointer to the FIFO buffer up to the
specified length. Since the FIFO buffer is modelled as an array then once
the FIFO wraps around, only the continuous portion of the buffer can be
returned.

In future the use of continuous and unaligned accesses will advance the
internal FIFO head pointer, so modify esp_fifo_pop_buf() to ensure that
any wraparound content is also returned up to the requested length.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index b382865426..8d8f6a817a 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -121,17 +121,30 @@ static uint8_t esp_fifo_pop(Fifo8 *fifo)
 static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
 {
     const uint8_t *buf;
-    uint32_t n;
+    uint32_t n, n2;
+    int len;
 
     if (maxlen == 0) {
         return 0;
     }
 
-    buf = fifo8_pop_buf(fifo, maxlen, &n);
+    len = maxlen;
+    buf = fifo8_pop_buf(fifo, len, &n);
     if (dest) {
         memcpy(dest, buf, n);
     }
 
+    /* Add FIFO wraparound if needed */
+    len -= n;
+    len = MIN(len, fifo8_num_used(fifo));
+    if (len) {
+        buf = fifo8_pop_buf(fifo, len, &n2);
+        if (dest) {
+            memcpy(&dest[n], buf, n2);
+        }
+        n += n2;
+    }
+
     return n;
 }
 
-- 
2.39.2

[PATCH 04/88] esp: remove FIFO clear from esp_select()

[Mark Cave-Ayland]

The FIFO contents should not be affected by performing SCSI target selection.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 8d8f6a817a..89fce05e58 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -220,7 +220,6 @@ static int esp_select(ESPState *s)
     target = s->wregs[ESP_WBUSID] & BUSID_DID;
 
     s->ti_size = 0;
-    fifo8_reset(&s->fifo);
 
     if (s->current_req) {
         /* Started a new command before the old one finished. Cancel it. */
-- 
2.39.2

[PATCH 05/88] esp: move esp_select() to ESP selection commands from get_cmd()

[Mark Cave-Ayland]

Since the DREQ value depends upon the result of the selection process, add a
workaround to each esp_select() to manually assert DREQ durring the MESSAGE OUT
and COMMAND phases.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 89fce05e58..8c1c6bfc1c 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -263,10 +263,6 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
             dmalen = MIN(fifo8_num_free(&s->cmdfifo), dmalen);
             fifo8_push_all(&s->cmdfifo, buf, dmalen);
         } else {
-            if (esp_select(s) < 0) {
-                return -1;
-            }
-            esp_raise_drq(s);
             return 0;
         }
     } else {
@@ -280,9 +276,6 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
     }
     trace_esp_get_cmd(dmalen, target);
 
-    if (esp_select(s) < 0) {
-        return -1;
-    }
     return dmalen;
 }
 
@@ -380,12 +373,18 @@ static void handle_satn(ESPState *s)
         return;
     }
     esp_set_pdma_cb(s, SATN_PDMA_CB);
+    if (esp_select(s) < 0) {
+        return;
+    }
     cmdlen = get_cmd(s, ESP_CMDFIFO_SZ);
     if (cmdlen > 0) {
         s->cmdfifo_cdb_offset = 1;
         s->do_cmd = 0;
         do_cmd(s);
     } else if (cmdlen == 0) {
+        if (s->dma) {
+            esp_raise_drq(s);
+        }
         s->do_cmd = 1;
         /* Target present, but no cmd yet - switch to command phase */
         s->rregs[ESP_RSEQ] = SEQ_CD;
@@ -411,12 +410,18 @@ static void handle_s_without_atn(ESPState *s)
         return;
     }
     esp_set_pdma_cb(s, S_WITHOUT_SATN_PDMA_CB);
+    if (esp_select(s) < 0) {
+        return;
+    }
     cmdlen = get_cmd(s, ESP_CMDFIFO_SZ);
     if (cmdlen > 0) {
         s->cmdfifo_cdb_offset = 0;
         s->do_cmd = 0;
         do_cmd(s);
     } else if (cmdlen == 0) {
+        if (s->dma) {
+            esp_raise_drq(s);
+        }
         s->do_cmd = 1;
         /* Target present, but no cmd yet - switch to command phase */
         s->rregs[ESP_RSEQ] = SEQ_CD;
@@ -446,6 +451,9 @@ static void handle_satn_stop(ESPState *s)
         return;
     }
     esp_set_pdma_cb(s, SATN_STOP_PDMA_CB);
+    if (esp_select(s) < 0) {
+        return;
+    }
     cmdlen = get_cmd(s, 1);
     if (cmdlen > 0) {
         trace_esp_handle_satn_stop(fifo8_num_used(&s->cmdfifo));
@@ -456,6 +464,9 @@ static void handle_satn_stop(ESPState *s)
         s->rregs[ESP_RSEQ] = SEQ_MO;
         esp_raise_irq(s);
     } else if (cmdlen == 0) {
+        if (s->dma) {
+            esp_raise_drq(s);
+        }
         s->do_cmd = 1;
         /* Target present, switch to message out phase */
         s->rregs[ESP_RSEQ] = SEQ_MO;
-- 
2.39.2

[PATCH 06/88] esp: update esp_set_tc() to set STAT_TC flag

[Mark Cave-Ayland]

This flag is set once the transfer counter counts down to zero.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 8c1c6bfc1c..c7b79a2949 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -161,9 +161,15 @@ static uint32_t esp_get_tc(ESPState *s)
 
 static void esp_set_tc(ESPState *s, uint32_t dmalen)
 {
+    uint32_t old_tc = esp_get_tc(s);
+
     s->rregs[ESP_TCLO] = dmalen;
     s->rregs[ESP_TCMID] = dmalen >> 8;
     s->rregs[ESP_TCHI] = dmalen >> 16;
+
+    if (old_tc && dmalen == 0) {
+        s->rregs[ESP_RSTAT] |= STAT_TC;
+    }
 }
 
 static uint32_t esp_get_stc(ESPState *s)
-- 
2.39.2

[PATCH 07/88] esp: start removal of manual STAT_TC setting when transfer counter reaches zero

[Mark Cave-Ayland]

This should be exclusively managed by esp_set_tc() rather than being manually
set in multiple places. Start by removing the occurrences exclusive to PDMA
and command completion which are those that can be currently removed without
affecting any test images.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index c7b79a2949..e717b2e216 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -611,11 +611,6 @@ static void do_dma_pdma_cb(ESPState *s)
             s->async_len -= len;
             s->ti_size -= len;
             esp_set_tc(s, esp_get_tc(s) - len);
-
-            if (esp_get_tc(s) == 0) {
-                /* Indicate transfer to FIFO is complete */
-                 s->rregs[ESP_RSTAT] |= STAT_TC;
-            }
             return;
         }
 
@@ -720,9 +715,6 @@ static void esp_do_dma(ESPState *s)
             esp_set_tc(s, esp_get_tc(s) - len);
             esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
             esp_raise_drq(s);
-
-            /* Indicate transfer to FIFO is complete */
-            s->rregs[ESP_RSTAT] |= STAT_TC;
             return;
         }
     }
@@ -870,7 +862,8 @@ void esp_command_complete(SCSIRequest *req, size_t resid)
      * transfers from the target the last byte is still in the FIFO
      */
     if (s->ti_size == 0) {
-        s->rregs[ESP_RSTAT] = STAT_TC | STAT_ST;
+        s->rregs[ESP_RSTAT] &= ~7;
+        s->rregs[ESP_RSTAT] |= STAT_ST;
         esp_dma_done(s);
         esp_lower_drq(s);
     }
-- 
2.39.2

[PATCH 08/88] esp: move command execution logic to new esp_run_cmd() function

[Mark Cave-Ayland]

This helps to simplify esp_reg_write() and potentially allows for a 2-level
deep FIFO to be implemented in future.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index e717b2e216..fecfef7c89 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -980,6 +980,97 @@ static void parent_esp_reset(ESPState *s, int irq, int level)
     }
 }
 
+static void esp_run_cmd(ESPState *s)
+{
+    uint8_t cmd = s->rregs[ESP_CMD];
+
+    if (cmd & CMD_DMA) {
+        s->dma = 1;
+        /* Reload DMA counter.  */
+        if (esp_get_stc(s) == 0) {
+            esp_set_tc(s, 0x10000);
+        } else {
+            esp_set_tc(s, esp_get_stc(s));
+        }
+    } else {
+        s->dma = 0;
+    }
+    switch (cmd & CMD_CMD) {
+    case CMD_NOP:
+        trace_esp_mem_writeb_cmd_nop(cmd);
+        break;
+    case CMD_FLUSH:
+        trace_esp_mem_writeb_cmd_flush(cmd);
+        fifo8_reset(&s->fifo);
+        break;
+    case CMD_RESET:
+        trace_esp_mem_writeb_cmd_reset(cmd);
+        esp_soft_reset(s);
+        break;
+    case CMD_BUSRESET:
+        trace_esp_mem_writeb_cmd_bus_reset(cmd);
+        esp_bus_reset(s);
+        if (!(s->wregs[ESP_CFG1] & CFG1_RESREPT)) {
+            s->rregs[ESP_RINTR] |= INTR_RST;
+            esp_raise_irq(s);
+        }
+        break;
+    case CMD_TI:
+        trace_esp_mem_writeb_cmd_ti(cmd);
+        handle_ti(s);
+        break;
+    case CMD_ICCS:
+        trace_esp_mem_writeb_cmd_iccs(cmd);
+        write_response(s);
+        s->rregs[ESP_RINTR] |= INTR_FC;
+        s->rregs[ESP_RSTAT] |= STAT_MI;
+        break;
+    case CMD_MSGACC:
+        trace_esp_mem_writeb_cmd_msgacc(cmd);
+        s->rregs[ESP_RINTR] |= INTR_DC;
+        s->rregs[ESP_RSEQ] = 0;
+        s->rregs[ESP_RFLAGS] = 0;
+        esp_raise_irq(s);
+        break;
+    case CMD_PAD:
+        trace_esp_mem_writeb_cmd_pad(cmd);
+        s->rregs[ESP_RSTAT] = STAT_TC;
+        s->rregs[ESP_RINTR] |= INTR_FC;
+        s->rregs[ESP_RSEQ] = 0;
+        break;
+    case CMD_SATN:
+        trace_esp_mem_writeb_cmd_satn(cmd);
+        break;
+    case CMD_RSTATN:
+        trace_esp_mem_writeb_cmd_rstatn(cmd);
+        break;
+    case CMD_SEL:
+        trace_esp_mem_writeb_cmd_sel(cmd);
+        handle_s_without_atn(s);
+        break;
+    case CMD_SELATN:
+        trace_esp_mem_writeb_cmd_selatn(cmd);
+        handle_satn(s);
+        break;
+    case CMD_SELATNS:
+        trace_esp_mem_writeb_cmd_selatns(cmd);
+        handle_satn_stop(s);
+        break;
+    case CMD_ENSEL:
+        trace_esp_mem_writeb_cmd_ensel(cmd);
+        s->rregs[ESP_RINTR] = 0;
+        break;
+    case CMD_DISSEL:
+        trace_esp_mem_writeb_cmd_dissel(cmd);
+        s->rregs[ESP_RINTR] = 0;
+        esp_raise_irq(s);
+        break;
+    default:
+        trace_esp_error_unhandled_command(cmd);
+        break;
+    }
+}
+
 uint64_t esp_reg_read(ESPState *s, uint32_t saddr)
 {
     uint32_t val;
@@ -1076,91 +1167,7 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val)
         break;
     case ESP_CMD:
         s->rregs[saddr] = val;
-        if (val & CMD_DMA) {
-            s->dma = 1;
-            /* Reload DMA counter.  */
-            if (esp_get_stc(s) == 0) {
-                esp_set_tc(s, 0x10000);
-            } else {
-                esp_set_tc(s, esp_get_stc(s));
-            }
-        } else {
-            s->dma = 0;
-        }
-        switch (val & CMD_CMD) {
-        case CMD_NOP:
-            trace_esp_mem_writeb_cmd_nop(val);
-            break;
-        case CMD_FLUSH:
-            trace_esp_mem_writeb_cmd_flush(val);
-            fifo8_reset(&s->fifo);
-            break;
-        case CMD_RESET:
-            trace_esp_mem_writeb_cmd_reset(val);
-            esp_soft_reset(s);
-            break;
-        case CMD_BUSRESET:
-            trace_esp_mem_writeb_cmd_bus_reset(val);
-            esp_bus_reset(s);
-            if (!(s->wregs[ESP_CFG1] & CFG1_RESREPT)) {
-                s->rregs[ESP_RINTR] |= INTR_RST;
-                esp_raise_irq(s);
-            }
-            break;
-        case CMD_TI:
-            trace_esp_mem_writeb_cmd_ti(val);
-            handle_ti(s);
-            break;
-        case CMD_ICCS:
-            trace_esp_mem_writeb_cmd_iccs(val);
-            write_response(s);
-            s->rregs[ESP_RINTR] |= INTR_FC;
-            s->rregs[ESP_RSTAT] |= STAT_MI;
-            break;
-        case CMD_MSGACC:
-            trace_esp_mem_writeb_cmd_msgacc(val);
-            s->rregs[ESP_RINTR] |= INTR_DC;
-            s->rregs[ESP_RSEQ] = 0;
-            s->rregs[ESP_RFLAGS] = 0;
-            esp_raise_irq(s);
-            break;
-        case CMD_PAD:
-            trace_esp_mem_writeb_cmd_pad(val);
-            s->rregs[ESP_RSTAT] = STAT_TC;
-            s->rregs[ESP_RINTR] |= INTR_FC;
-            s->rregs[ESP_RSEQ] = 0;
-            break;
-        case CMD_SATN:
-            trace_esp_mem_writeb_cmd_satn(val);
-            break;
-        case CMD_RSTATN:
-            trace_esp_mem_writeb_cmd_rstatn(val);
-            break;
-        case CMD_SEL:
-            trace_esp_mem_writeb_cmd_sel(val);
-            handle_s_without_atn(s);
-            break;
-        case CMD_SELATN:
-            trace_esp_mem_writeb_cmd_selatn(val);
-            handle_satn(s);
-            break;
-        case CMD_SELATNS:
-            trace_esp_mem_writeb_cmd_selatns(val);
-            handle_satn_stop(s);
-            break;
-        case CMD_ENSEL:
-            trace_esp_mem_writeb_cmd_ensel(val);
-            s->rregs[ESP_RINTR] = 0;
-            break;
-        case CMD_DISSEL:
-            trace_esp_mem_writeb_cmd_dissel(val);
-            s->rregs[ESP_RINTR] = 0;
-            esp_raise_irq(s);
-            break;
-        default:
-            trace_esp_error_unhandled_command(val);
-            break;
-        }
+        esp_run_cmd(s);
         break;
     case ESP_WBUSID ... ESP_WSYNO:
         break;
-- 
2.39.2

[PATCH 09/88] esp: update TC check logic in do_dma_pdma_cb() to check for TC == 0

[Mark Cave-Ayland]

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-ayland@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);
         }
 
-        /* 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

Re: [PATCH 09/88] esp: update TC check logic in do_dma_pdma_cb() to check for TC == 0

[Paolo Bonzini]

On Fri, Jan 12, 2024 at 1:55 PM Mark Cave-Ayland
<mark.cave-ayland@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-ayland@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

>
> -        /* 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
>

Re: [PATCH 09/88] esp: update TC check logic in do_dma_pdma_cb() to check for TC == 0

[Mark Cave-Ayland]

On 01/02/2024 10:46, Paolo Bonzini wrote:

> On Fri, Jan 12, 2024 at 1:55 PM Mark Cave-Ayland
> <mark.cave-ayland@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-ayland@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.

Re: [PATCH 09/88] esp: update TC check logic in do_dma_pdma_cb() to check for TC == 0

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 3674 bytes --]

Il gio 1 feb 2024, 12:25 Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
ha scritto:

> On 01/02/2024 10:46, Paolo Bonzini wrote:
>
> > On Fri, Jan 12, 2024 at 1:55 PM Mark Cave-Ayland
> > <mark.cave-ayland@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-ayland@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.
>

Absolutely, just noticing that there is a functional change but the commit
message showed it as a refactoring only.

The fact that this is bisectable is quite insane, and I am not asking for
any code changes. TBH I wouldn't have blamed you if you just sent a patch
to create esp2.c and a patch to delete esp.c...

Paolo


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.
>
>

[-- Attachment #2: Type: text/html, Size: 5409 bytes --]

Re: [PATCH 09/88] esp: update TC check logic in do_dma_pdma_cb() to check for TC == 0

[Mark Cave-Ayland]

On 01/02/2024 11:36, Paolo Bonzini wrote:

> Il gio 1 feb 2024, 12:25 Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk 
> <mailto:mark.cave-ayland@ilande.co.uk>> ha scritto:
> 
>     On 01/02/2024 10:46, Paolo Bonzini wrote:
> 
>      > On Fri, Jan 12, 2024 at 1:55 PM Mark Cave-Ayland
>      > <mark.cave-ayland@ilande.co.uk <mailto:mark.cave-ayland@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-ayland@ilande.co.uk
>     <mailto:mark.cave-ayland@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.
> 
> 
> Absolutely, just noticing that there is a functional change but the commit message 
> showed it as a refactoring only.

Would you like me to come up with a reworded commit message here?

> The fact that this is bisectable is quite insane, and I am not asking for any code 
> changes. TBH I wouldn't have blamed you if you just sent a patch to create esp2.c and 
> a patch to delete esp.c...

Heh... spending a chunk of time rewriting the emulation of a 30 year-old SCSI 
controller is probably an odd choice, but the result is something that is 
considerably more maintainable than the current implementation. Definitely having a 
bisectible history helps in the case of finding a regression, but so far I'd say the 
result is a huge improvement on what is already there.

Anything else that you'd like me to change before the series can be considered for merge?


ATB,

Mark.

Re: [PATCH 09/88] esp: update TC check logic in do_dma_pdma_cb() to check for TC == 0

[Paolo Bonzini]

On Thu, Feb 8, 2024 at 10:46 AM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 01/02/2024 11:36, Paolo Bonzini wrote:
>
> > Il gio 1 feb 2024, 12:25 Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk
> > <mailto:mark.cave-ayland@ilande.co.uk>> ha scritto:
> >
> >     On 01/02/2024 10:46, Paolo Bonzini wrote:
> >
> >      > On Fri, Jan 12, 2024 at 1:55 PM Mark Cave-Ayland
> >      > <mark.cave-ayland@ilande.co.uk <mailto:mark.cave-ayland@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-ayland@ilande.co.uk
> >     <mailto:mark.cave-ayland@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.
> >
> >
> > Absolutely, just noticing that there is a functional change but the commit message
> > showed it as a refactoring only.
>
> Would you like me to come up with a reworded commit message here?

If you want to change it, it's okay because len is zero at this point
and the code after the "if" therefore does nothing. And the "if" will
become esp_dma_ti_check() so adding a return is kind of messy here.

> > The fact that this is bisectable is quite insane, and I am not asking for any code
> > changes. TBH I wouldn't have blamed you if you just sent a patch to create esp2.c and
> > a patch to delete esp.c...
>
> Heh... spending a chunk of time rewriting the emulation of a 30 year-old SCSI
> controller is probably an odd choice, but the result is something that is
> considerably more maintainable than the current implementation.

I absolutely agree, you went above and beyond and sorry if it wasn't
clear from the joke.

> Anything else that you'd like me to change before the series can be considered for merge?

No, go ahead!

Paolo

Re: [PATCH 09/88] esp: update TC check logic in do_dma_pdma_cb() to check for TC == 0

[Mark Cave-Ayland]

On 08/02/2024 18:11, Paolo Bonzini wrote:

> On Thu, Feb 8, 2024 at 10:46 AM Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> On 01/02/2024 11:36, Paolo Bonzini wrote:
>>
>>> Il gio 1 feb 2024, 12:25 Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk
>>> <mailto:mark.cave-ayland@ilande.co.uk>> ha scritto:
>>>
>>>      On 01/02/2024 10:46, Paolo Bonzini wrote:
>>>
>>>       > On Fri, Jan 12, 2024 at 1:55 PM Mark Cave-Ayland
>>>       > <mark.cave-ayland@ilande.co.uk <mailto:mark.cave-ayland@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-ayland@ilande.co.uk
>>>      <mailto:mark.cave-ayland@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.
>>>
>>>
>>> Absolutely, just noticing that there is a functional change but the commit message
>>> showed it as a refactoring only.
>>
>> Would you like me to come up with a reworded commit message here?
> 
> If you want to change it, it's okay because len is zero at this point
> and the code after the "if" therefore does nothing. And the "if" will
> become esp_dma_ti_check() so adding a return is kind of messy here.

Okay I'll leave it as it is - I'm not too worried about the detail here, since that 
particular section ends up being removed.

>>> The fact that this is bisectable is quite insane, and I am not asking for any code
>>> changes. TBH I wouldn't have blamed you if you just sent a patch to create esp2.c and
>>> a patch to delete esp.c...
>>
>> Heh... spending a chunk of time rewriting the emulation of a 30 year-old SCSI
>> controller is probably an odd choice, but the result is something that is
>> considerably more maintainable than the current implementation.
> 
> I absolutely agree, you went above and beyond and sorry if it wasn't
> clear from the joke.

No worries, I wasn't taking it seriously either :)

>> Anything else that you'd like me to change before the series can be considered for merge?
> 
> No, go ahead!

Thanks! I'll take that as a nod to send a PR over the next few days.


ATB,

Mark.

[PATCH 10/88] esp: move buffer and TC logic into separate to/from device paths in esp_do_dma()

[Mark Cave-Ayland]

The ultimate aim is to for esp_do_dma() behaviour to be determined by the SCSI
bus phase, in which case it is necessary to have separate to/from device paths.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 63c828c1b2..9893840255 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -678,14 +678,53 @@ static void esp_do_dma(ESPState *s)
     if (to_device) {
         if (s->dma_memory_read) {
             s->dma_memory_read(s->dma_opaque, s->async_buf, len);
+
+            esp_set_tc(s, esp_get_tc(s) - len);
+            s->async_buf += len;
+            s->async_len -= len;
+            s->ti_size += len;
+
+            if (s->async_len == 0) {
+                scsi_req_continue(s->current_req);
+                /*
+                 * If there is still data to be read from the device then
+                 * complete the DMA operation immediately.  Otherwise defer
+                 * until the scsi layer has completed.
+                 */
+                return;
+            }
+
+            /* Partially filled a scsi buffer. Complete immediately.  */
+            esp_dma_done(s);
+            esp_lower_drq(s);
         } else {
             esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
             esp_raise_drq(s);
-            return;
         }
     } else {
         if (s->dma_memory_write) {
             s->dma_memory_write(s->dma_opaque, s->async_buf, len);
+
+            esp_set_tc(s, esp_get_tc(s) - len);
+            s->async_buf += len;
+            s->async_len -= len;
+            s->ti_size -= len;
+
+            if (s->async_len == 0) {
+                scsi_req_continue(s->current_req);
+                /*
+                 * If there is still data to be read from the device then
+                 * complete the DMA operation immediately.  Otherwise defer
+                 * until the scsi layer has completed.
+                 */
+                if (esp_get_tc(s) != 0 || s->ti_size == 0) {
+                    return;
+                }
+            }
+
+            /* Partially filled a scsi buffer. Complete immediately.  */
+            esp_dma_done(s);
+            esp_lower_drq(s);
         } else {
             /* Adjust TC for any leftover data in the FIFO */
             if (!fifo8_is_empty(&s->fifo)) {
@@ -713,32 +752,8 @@ static void esp_do_dma(ESPState *s)
             esp_set_tc(s, esp_get_tc(s) - len);
             esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
             esp_raise_drq(s);
-            return;
         }
     }
-    esp_set_tc(s, esp_get_tc(s) - len);
-    s->async_buf += len;
-    s->async_len -= len;
-    if (to_device) {
-        s->ti_size += len;
-    } else {
-        s->ti_size -= len;
-    }
-    if (s->async_len == 0) {
-        scsi_req_continue(s->current_req);
-        /*
-         * If there is still data to be read from the device then
-         * complete the DMA operation immediately.  Otherwise defer
-         * until the scsi layer has completed.
-         */
-        if (to_device || esp_get_tc(s) != 0 || s->ti_size == 0) {
-            return;
-        }
-    }
-
-    /* Partially filled a scsi buffer. Complete immediately.  */
-    esp_dma_done(s);
-    esp_lower_drq(s);
 }
 
 static void esp_do_nodma(ESPState *s)
-- 
2.39.2

[PATCH 11/88] esp.c: remove unused case from esp_pdma_read()

[Mark Cave-Ayland]

The do_cmd variable is only set for the MESSAGE OUT and COMMAND phases i.e.
those which involve transfers from the host to the SCSI bus, and so the unused
case can be removed.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 9893840255..6191c17f10 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -187,12 +187,7 @@ static uint8_t esp_pdma_read(ESPState *s)
 {
     uint8_t val;
 
-    if (s->do_cmd) {
-        val = esp_fifo_pop(&s->cmdfifo);
-    } else {
-        val = esp_fifo_pop(&s->fifo);
-    }
-
+    val = esp_fifo_pop(&s->fifo);
     return val;
 }
 
-- 
2.39.2

[PATCH 12/88] esp.c: don't accumulate directly into cmdfifo

[Mark Cave-Ayland]

Instead accumulate in the real FIFO as done in real hardware, and then transfer
to cmdfifo when we're ready to process the MESSAGE OUT and COMMAND phase data.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 6191c17f10..9e9bbe8431 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -199,11 +199,7 @@ static void esp_pdma_write(ESPState *s, uint8_t val)
         return;
     }
 
-    if (s->do_cmd) {
-        esp_fifo_push(&s->cmdfifo, val);
-    } else {
-        esp_fifo_push(&s->fifo, val);
-    }
+    esp_fifo_push(&s->fifo, val);
 
     dmalen--;
     esp_set_tc(s, dmalen);
@@ -358,6 +354,14 @@ static void do_cmd(ESPState *s)
 
 static void satn_pdma_cb(ESPState *s)
 {
+    uint8_t buf[ESP_FIFO_SZ];
+    int n;
+
+    /* Copy FIFO into cmdfifo */
+    n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+    n = MIN(fifo8_num_free(&s->cmdfifo), n);
+    fifo8_push_all(&s->cmdfifo, buf, n);
+
     if (!esp_get_tc(s) && !fifo8_is_empty(&s->cmdfifo)) {
         s->cmdfifo_cdb_offset = 1;
         s->do_cmd = 0;
@@ -395,6 +399,14 @@ static void handle_satn(ESPState *s)
 
 static void s_without_satn_pdma_cb(ESPState *s)
 {
+    uint8_t buf[ESP_FIFO_SZ];
+    int n;
+
+    /* Copy FIFO into cmdfifo */
+    n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+    n = MIN(fifo8_num_free(&s->cmdfifo), n);
+    fifo8_push_all(&s->cmdfifo, buf, n);
+
     if (!esp_get_tc(s) && !fifo8_is_empty(&s->cmdfifo)) {
         s->cmdfifo_cdb_offset = 0;
         s->do_cmd = 0;
@@ -432,6 +444,14 @@ static void handle_s_without_atn(ESPState *s)
 
 static void satn_stop_pdma_cb(ESPState *s)
 {
+    uint8_t buf[ESP_FIFO_SZ];
+    int n;
+
+    /* Copy FIFO into cmdfifo */
+    n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+    n = MIN(fifo8_num_free(&s->cmdfifo), n);
+    fifo8_push_all(&s->cmdfifo, buf, n);
+
     if (!esp_get_tc(s) && !fifo8_is_empty(&s->cmdfifo)) {
         trace_esp_handle_satn_stop(fifo8_num_used(&s->cmdfifo));
         s->do_cmd = 1;
@@ -523,10 +543,16 @@ static void esp_dma_done(ESPState *s)
 static void do_dma_pdma_cb(ESPState *s)
 {
     int to_device = ((s->rregs[ESP_RSTAT] & 7) == STAT_DO);
+    uint8_t buf[ESP_CMDFIFO_SZ];
     int len;
     uint32_t n;
 
     if (s->do_cmd) {
+        /* Copy FIFO into cmdfifo */
+        n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+        n = MIN(fifo8_num_free(&s->cmdfifo), n);
+        fifo8_push_all(&s->cmdfifo, buf, n);
+
         /* Ensure we have received complete command after SATN and stop */
         if (esp_get_tc(s) || fifo8_is_empty(&s->cmdfifo)) {
             return;
@@ -754,10 +780,16 @@ static void esp_do_dma(ESPState *s)
 static void esp_do_nodma(ESPState *s)
 {
     int to_device = ((s->rregs[ESP_RSTAT] & 7) == STAT_DO);
+    uint8_t buf[ESP_FIFO_SZ];
     uint32_t cmdlen;
-    int len;
+    int len, n;
 
     if (s->do_cmd) {
+        /* Copy FIFO into cmdfifo */
+        n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+        n = MIN(fifo8_num_free(&s->cmdfifo), n);
+        fifo8_push_all(&s->cmdfifo, buf, n);
+
         cmdlen = fifo8_num_used(&s->cmdfifo);
         trace_esp_handle_ti_cmd(cmdlen);
         s->ti_size = 0;
@@ -1159,7 +1191,10 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val)
         break;
     case ESP_FIFO:
         if (s->do_cmd) {
-            esp_fifo_push(&s->cmdfifo, val);
+            if (!fifo8_is_full(&s->fifo)) {
+                esp_fifo_push(&s->fifo, val);
+                esp_fifo_push(&s->cmdfifo, fifo8_pop(&s->fifo));
+            }
 
             /*
              * If any unexpected message out/command phase data is
-- 
2.39.2

[PATCH 13/88] esp.c: decrement the TC during MESSAGE OUT and COMMAND phases

[Mark Cave-Ayland]

This is to ensure that STAT_TC is triggered during the right parts of the
transfer when it is controlled exclusively by the TC.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 9e9bbe8431..f08b816aba 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -259,6 +259,7 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
             s->dma_memory_read(s->dma_opaque, buf, dmalen);
             dmalen = MIN(fifo8_num_free(&s->cmdfifo), dmalen);
             fifo8_push_all(&s->cmdfifo, buf, dmalen);
+            esp_set_tc(s, esp_get_tc(s) - dmalen);
         } else {
             return 0;
         }
@@ -657,6 +658,7 @@ static void esp_do_dma(ESPState *s)
             len = MIN(len, fifo8_num_free(&s->cmdfifo));
             s->dma_memory_read(s->dma_opaque, buf, len);
             fifo8_push_all(&s->cmdfifo, buf, len);
+            esp_set_tc(s, esp_get_tc(s) - len);
         } else {
             esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
             esp_raise_drq(s);
-- 
2.39.2

[PATCH 14/88] esp.c: introduce esp_set_phase() helper function

[Mark Cave-Ayland]

This function is used to set the current SCSI bus phase in the ESP_RSTAT register
without affecting any of flag bits.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index f08b816aba..3fc7417d7c 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -183,6 +183,19 @@ 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_pdma_read(ESPState *s)
 {
     uint8_t val;
@@ -316,9 +329,9 @@ static void do_command_phase(ESPState *s)
              * complete before raising the command completion interrupt
              */
             s->data_in_ready = false;
-            s->rregs[ESP_RSTAT] |= STAT_DI;
+            esp_set_phase(s, STAT_DI);
         } else {
-            s->rregs[ESP_RSTAT] |= STAT_DO;
+            esp_set_phase(s, STAT_DO);
             s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
             esp_raise_irq(s);
             esp_lower_drq(s);
@@ -394,7 +407,7 @@ static void handle_satn(ESPState *s)
         s->do_cmd = 1;
         /* Target present, but no cmd yet - switch to command phase */
         s->rregs[ESP_RSEQ] = SEQ_CD;
-        s->rregs[ESP_RSTAT] = STAT_CD;
+        esp_set_phase(s, STAT_CD);
     }
 }
 
@@ -439,7 +452,7 @@ static void handle_s_without_atn(ESPState *s)
         s->do_cmd = 1;
         /* Target present, but no cmd yet - switch to command phase */
         s->rregs[ESP_RSEQ] = SEQ_CD;
-        s->rregs[ESP_RSTAT] = STAT_CD;
+        esp_set_phase(s, STAT_CD);
     }
 }
 
@@ -457,7 +470,8 @@ static void satn_stop_pdma_cb(ESPState *s)
         trace_esp_handle_satn_stop(fifo8_num_used(&s->cmdfifo));
         s->do_cmd = 1;
         s->cmdfifo_cdb_offset = 1;
-        s->rregs[ESP_RSTAT] = STAT_TC | STAT_CD;
+        esp_set_phase(s, STAT_CD);
+        s->rregs[ESP_RSTAT] |= STAT_TC;
         s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
         s->rregs[ESP_RSEQ] = SEQ_CD;
         esp_raise_irq(s);
@@ -481,7 +495,7 @@ static void handle_satn_stop(ESPState *s)
         trace_esp_handle_satn_stop(fifo8_num_used(&s->cmdfifo));
         s->do_cmd = 1;
         s->cmdfifo_cdb_offset = 1;
-        s->rregs[ESP_RSTAT] = STAT_MO;
+        esp_set_phase(s, STAT_MO);
         s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
         s->rregs[ESP_RSEQ] = SEQ_MO;
         esp_raise_irq(s);
@@ -492,13 +506,14 @@ static void handle_satn_stop(ESPState *s)
         s->do_cmd = 1;
         /* Target present, switch to message out phase */
         s->rregs[ESP_RSEQ] = SEQ_MO;
-        s->rregs[ESP_RSTAT] = STAT_MO;
+        esp_set_phase(s, STAT_MO);
     }
 }
 
 static void write_response_pdma_cb(ESPState *s)
 {
-    s->rregs[ESP_RSTAT] = STAT_TC | STAT_ST;
+    esp_set_phase(s, STAT_ST);
+    s->rregs[ESP_RSTAT] |= STAT_TC;
     s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
     s->rregs[ESP_RSEQ] = SEQ_CD;
     esp_raise_irq(s);
@@ -516,7 +531,8 @@ static void write_response(ESPState *s)
     if (s->dma) {
         if (s->dma_memory_write) {
             s->dma_memory_write(s->dma_opaque, buf, 2);
-            s->rregs[ESP_RSTAT] = STAT_TC | STAT_ST;
+            esp_set_phase(s, STAT_ST);
+            s->rregs[ESP_RSTAT] |= STAT_TC;
             s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
             s->rregs[ESP_RSEQ] = SEQ_CD;
         } else {
@@ -575,7 +591,8 @@ static void do_dma_pdma_cb(ESPState *s)
              * and then switch to command phase
              */
             s->cmdfifo_cdb_offset = fifo8_num_used(&s->cmdfifo);
-            s->rregs[ESP_RSTAT] = STAT_TC | STAT_CD;
+            esp_set_phase(s, STAT_CD);
+            s->rregs[ESP_RSTAT] |= STAT_TC;
             s->rregs[ESP_RSEQ] = SEQ_CD;
             s->rregs[ESP_RINTR] |= INTR_BS;
             esp_raise_irq(s);
@@ -681,7 +698,8 @@ static void esp_do_dma(ESPState *s)
              * and then switch to command phase
              */
             s->cmdfifo_cdb_offset = fifo8_num_used(&s->cmdfifo);
-            s->rregs[ESP_RSTAT] = STAT_TC | STAT_CD;
+            esp_set_phase(s, STAT_CD);
+            s->rregs[ESP_RSTAT] |= STAT_TC;
             s->rregs[ESP_RSEQ] = SEQ_CD;
             s->rregs[ESP_RINTR] |= INTR_BS;
             esp_raise_irq(s);
@@ -810,7 +828,8 @@ static void esp_do_nodma(ESPState *s)
              * and then switch to command phase
              */
             s->cmdfifo_cdb_offset = fifo8_num_used(&s->cmdfifo);
-            s->rregs[ESP_RSTAT] = STAT_TC | STAT_CD;
+            esp_set_phase(s, STAT_CD);
+            s->rregs[ESP_RSTAT] |= STAT_TC;
             s->rregs[ESP_RSEQ] = SEQ_CD;
             s->rregs[ESP_RINTR] |= INTR_BS;
             esp_raise_irq(s);
@@ -904,8 +923,7 @@ void esp_command_complete(SCSIRequest *req, size_t resid)
      * transfers from the target the last byte is still in the FIFO
      */
     if (s->ti_size == 0) {
-        s->rregs[ESP_RSTAT] &= ~7;
-        s->rregs[ESP_RSTAT] |= STAT_ST;
+        esp_set_phase(s, STAT_ST);
         esp_dma_done(s);
         esp_lower_drq(s);
     }
@@ -1065,7 +1083,7 @@ static void esp_run_cmd(ESPState *s)
         trace_esp_mem_writeb_cmd_iccs(cmd);
         write_response(s);
         s->rregs[ESP_RINTR] |= INTR_FC;
-        s->rregs[ESP_RSTAT] |= STAT_MI;
+        esp_set_phase(s, STAT_MI);
         break;
     case CMD_MSGACC:
         trace_esp_mem_writeb_cmd_msgacc(cmd);
@@ -1133,7 +1151,8 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr)
                      * The last byte of a non-DMA transfer has been read out
                      * of the FIFO so switch to status phase
                      */
-                    s->rregs[ESP_RSTAT] = STAT_TC | STAT_ST;
+                    esp_set_phase(s, STAT_ST);
+                    s->rregs[ESP_RSTAT] |= STAT_TC;
                 }
             }
             s->rregs[ESP_FIFO] = esp_fifo_pop(&s->fifo);
diff --git a/hw/scsi/trace-events b/hw/scsi/trace-events
index bdd4e2c7c7..d72f741ed8 100644
--- a/hw/scsi/trace-events
+++ b/hw/scsi/trace-events
@@ -197,6 +197,7 @@ esp_mem_writeb_cmd_selatns(uint32_t val) "Select with ATN & stop (0x%2.2x)"
 esp_mem_writeb_cmd_ensel(uint32_t val) "Enable selection (0x%2.2x)"
 esp_mem_writeb_cmd_dissel(uint32_t val) "Disable selection (0x%2.2x)"
 esp_mem_writeb_cmd_ti(uint32_t val) "Transfer Information (0x%2.2x)"
+esp_set_phase(const char *phase) "setting bus phase to %s"
 
 # esp-pci.c
 esp_pci_error_invalid_dma_direction(void) "invalid DMA transfer direction"
-- 
2.39.2

[PATCH 15/88] esp.c: remove another set of manual STAT_TC updates

[Mark Cave-Ayland]

Following on from the recent changes to when the TC is updated, it is now
possible to remove another set of manual STAT_TC updates so that its state
is now managed within esp_set_tc().

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 3fc7417d7c..6fd5c8767a 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -513,7 +513,6 @@ static void handle_satn_stop(ESPState *s)
 static void write_response_pdma_cb(ESPState *s)
 {
     esp_set_phase(s, STAT_ST);
-    s->rregs[ESP_RSTAT] |= STAT_TC;
     s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
     s->rregs[ESP_RSEQ] = SEQ_CD;
     esp_raise_irq(s);
@@ -532,7 +531,6 @@ static void write_response(ESPState *s)
         if (s->dma_memory_write) {
             s->dma_memory_write(s->dma_opaque, buf, 2);
             esp_set_phase(s, STAT_ST);
-            s->rregs[ESP_RSTAT] |= STAT_TC;
             s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
             s->rregs[ESP_RSEQ] = SEQ_CD;
         } else {
@@ -550,10 +548,8 @@ static void write_response(ESPState *s)
 
 static void esp_dma_done(ESPState *s)
 {
-    s->rregs[ESP_RSTAT] |= STAT_TC;
     s->rregs[ESP_RINTR] |= INTR_BS;
     s->rregs[ESP_RFLAGS] = 0;
-    esp_set_tc(s, 0);
     esp_raise_irq(s);
 }
 
@@ -592,7 +588,6 @@ static void do_dma_pdma_cb(ESPState *s)
              */
             s->cmdfifo_cdb_offset = fifo8_num_used(&s->cmdfifo);
             esp_set_phase(s, STAT_CD);
-            s->rregs[ESP_RSTAT] |= STAT_TC;
             s->rregs[ESP_RSEQ] = SEQ_CD;
             s->rregs[ESP_RINTR] |= INTR_BS;
             esp_raise_irq(s);
@@ -699,7 +694,6 @@ static void esp_do_dma(ESPState *s)
              */
             s->cmdfifo_cdb_offset = fifo8_num_used(&s->cmdfifo);
             esp_set_phase(s, STAT_CD);
-            s->rregs[ESP_RSTAT] |= STAT_TC;
             s->rregs[ESP_RSEQ] = SEQ_CD;
             s->rregs[ESP_RINTR] |= INTR_BS;
             esp_raise_irq(s);
@@ -829,7 +823,6 @@ static void esp_do_nodma(ESPState *s)
              */
             s->cmdfifo_cdb_offset = fifo8_num_used(&s->cmdfifo);
             esp_set_phase(s, STAT_CD);
-            s->rregs[ESP_RSTAT] |= STAT_TC;
             s->rregs[ESP_RSEQ] = SEQ_CD;
             s->rregs[ESP_RINTR] |= INTR_BS;
             esp_raise_irq(s);
@@ -952,7 +945,6 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
          * completion interrupt
          */
         s->data_in_ready = true;
-        s->rregs[ESP_RSTAT] |= STAT_TC;
         s->rregs[ESP_RINTR] |= INTR_BS;
         esp_raise_irq(s);
     }
@@ -997,7 +989,6 @@ static void handle_ti(ESPState *s)
     if (s->dma) {
         dmalen = esp_get_tc(s);
         trace_esp_handle_ti(dmalen);
-        s->rregs[ESP_RSTAT] &= ~STAT_TC;
         esp_do_dma(s);
     } else {
         trace_esp_handle_ti(s->ti_size);
@@ -1152,7 +1143,6 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr)
                      * of the FIFO so switch to status phase
                      */
                     esp_set_phase(s, STAT_ST);
-                    s->rregs[ESP_RSTAT] |= STAT_TC;
                 }
             }
             s->rregs[ESP_FIFO] = esp_fifo_pop(&s->fifo);
-- 
2.39.2

[PATCH 16/88] esp.c: remove MacOS TI workaround that pads FIFO transfers to ESP_FIFO_SZ

[Mark Cave-Ayland]

This workaround is no longer required with the current code and so can be
removed.

[Note: whilst MacOS itself can boot correctly, removing this hack prevents
a bootable EMILE CDROM from working. This is caused by a separate bug which
will be fixed by a subsequent patch]

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 6fd5c8767a..f41b2421f9 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -772,18 +772,6 @@ static void esp_do_dma(ESPState *s)
             s->async_buf += len;
             s->async_len -= len;
             s->ti_size -= len;
-
-            /*
-             * MacOS toolbox uses a TI length of 16 bytes for all commands, so
-             * commands shorter than this must be padded accordingly
-             */
-            if (len < esp_get_tc(s) && esp_get_tc(s) <= ESP_FIFO_SZ) {
-                while (fifo8_num_used(&s->fifo) < ESP_FIFO_SZ) {
-                    esp_fifo_push(&s->fifo, 0);
-                    len++;
-                }
-            }
-
             esp_set_tc(s, esp_get_tc(s) - len);
             esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
             esp_raise_drq(s);
-- 
2.39.2

[PATCH 17/88] esp.c: don't reset the TC and ESP_RSEQ state when executing a SCSI command

[Mark Cave-Ayland]

There is no need to manually reset these values as the ESP emulation now
correctly handles them within its existing logic.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index f41b2421f9..a4a1f41a40 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -319,10 +319,7 @@ static void do_command_phase(ESPState *s)
     s->ti_size = datalen;
     fifo8_reset(&s->cmdfifo);
     if (datalen != 0) {
-        s->rregs[ESP_RSTAT] = STAT_TC;
-        s->rregs[ESP_RSEQ] = SEQ_CD;
         s->ti_cmd = 0;
-        esp_set_tc(s, 0);
         if (datalen > 0) {
             /*
              * Switch to DATA IN phase but wait until initial data xfer is
-- 
2.39.2

[PATCH 18/88] esp.c: don't clear RFLAGS register when DMA is complete

[Mark Cave-Ayland]

The internal state of the ESP sequencer is not affected when raising an interrupt
to indicate the end of a DMA transfer.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index a4a1f41a40..5b9c3f1e5e 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -546,7 +546,6 @@ static void write_response(ESPState *s)
 static void esp_dma_done(ESPState *s)
 {
     s->rregs[ESP_RINTR] |= INTR_BS;
-    s->rregs[ESP_RFLAGS] = 0;
     esp_raise_irq(s);
 }
 
-- 
2.39.2

[PATCH 19/88] esp: remove zero transfer size check from esp_do_dma()

[Mark Cave-Ayland]

The TI end of transfer interrupt only occurs when the TC reaches zero and is
not related to the SCSI layer transfer.

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 5b9c3f1e5e..dc515e6435 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -748,7 +748,7 @@ static void esp_do_dma(ESPState *s)
                  * complete the DMA operation immediately.  Otherwise defer
                  * until the scsi layer has completed.
                  */
-                if (esp_get_tc(s) != 0 || s->ti_size == 0) {
+                if (esp_get_tc(s) != 0) {
                     return;
                 }
             }
-- 
2.39.2

[PATCH 20/88] esp.c: update condition for esp_dma_done() in esp_do_dma() from device path

[Mark Cave-Ayland]

No change to the condition itself, other than to clarify that esp_dma_done()
must be called when TC is zero.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index dc515e6435..96723efcf3 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -743,19 +743,13 @@ static void esp_do_dma(ESPState *s)
 
             if (s->async_len == 0) {
                 scsi_req_continue(s->current_req);
-                /*
-                 * If there is still data to be read from the device then
-                 * complete the DMA operation immediately.  Otherwise defer
-                 * until the scsi layer has completed.
-                 */
-                if (esp_get_tc(s) != 0) {
-                    return;
-                }
             }
 
-            /* Partially filled a scsi buffer. Complete immediately.  */
-            esp_dma_done(s);
-            esp_lower_drq(s);
+            if (esp_get_tc(s) == 0) {
+                /* Partially filled a scsi buffer. Complete immediately.  */
+                esp_dma_done(s);
+                esp_lower_drq(s);
+            }
         } else {
             /* Adjust TC for any leftover data in the FIFO */
             if (!fifo8_is_empty(&s->fifo)) {
-- 
2.39.2

[PATCH 21/88] esp.c: update condition for esp_dma_done() in esp_do_dma() to device path

[Mark Cave-Ayland]

Ensure that esp_dma_done() is only called when TC is zero, which is currently
always the case for DMA transfers.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 96723efcf3..06be9f2e74 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -725,9 +725,11 @@ static void esp_do_dma(ESPState *s)
                 return;
             }
 
-            /* Partially filled a scsi buffer. Complete immediately.  */
-            esp_dma_done(s);
-            esp_lower_drq(s);
+            if (esp_get_tc(s) == 0) {
+                /* Partially filled a scsi buffer. Complete immediately.  */
+                esp_dma_done(s);
+                esp_lower_drq(s);
+            }
         } else {
             esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
             esp_raise_drq(s);
-- 
2.39.2

[PATCH 22/88] esp.c: ensure that the PDMA callback is called for every device read

[Mark Cave-Ayland]

Rather than wait for the FIFO to fill up before calling the PDMA callback, push
that logic directly into the from_device logic in do_dma_pdma_cb().

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 06be9f2e74..d80a38daa0 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -625,14 +625,14 @@ static void do_dma_pdma_cb(ESPState *s)
 
         return;
     } else {
-        if (s->async_len == 0) {
+        if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
             /* Defer until the scsi layer has completed */
             scsi_req_continue(s->current_req);
             s->data_in_ready = false;
             return;
         }
 
-        if (esp_get_tc(s) == 0) {
+        if (esp_get_tc(s) == 0 && fifo8_num_used(&s->fifo) < 2) {
             esp_lower_drq(s);
             esp_dma_done(s);
         }
@@ -1419,9 +1419,7 @@ static uint64_t sysbus_esp_pdma_read(void *opaque, hwaddr addr,
         val = (val << 8) | esp_pdma_read(s);
         break;
     }
-    if (fifo8_num_used(&s->fifo) < 2) {
-        esp_pdma_cb(s);
-    }
+    esp_pdma_cb(s);
     return val;
 }
 
-- 
2.39.2

[PATCH 23/88] esp.c: don't immediately raise INTR_BS if SCSI data needed in esp_do_dma()

[Mark Cave-Ayland]

In the case when more data is requested from the SCSI layer during a DMA data
transfer from a device, don't immediately fall through to the TC check logic.
Otherwise when TC is zero INTR_BS will be raised immediately rather than when
the next set of SCSI data is ready.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index d80a38daa0..1f9902aec0 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -745,6 +745,7 @@ static void esp_do_dma(ESPState *s)
 
             if (s->async_len == 0) {
                 scsi_req_continue(s->current_req);
+                return;
             }
 
             if (esp_get_tc(s) == 0) {
-- 
2.39.2

[PATCH 24/88] esp.c: remove TC adjustment in esp_do_dma() from device path

[Mark Cave-Ayland]

Now that the TC is updated for each PDMA access (rather than once the FIFO is
full) there is no need to adjust the TC at start of each DMA transfer if the
FIFO is not empty.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 1f9902aec0..ec82097a01 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -754,11 +754,6 @@ static void esp_do_dma(ESPState *s)
                 esp_lower_drq(s);
             }
         } else {
-            /* Adjust TC for any leftover data in the FIFO */
-            if (!fifo8_is_empty(&s->fifo)) {
-                esp_set_tc(s, esp_get_tc(s) - fifo8_num_used(&s->fifo));
-            }
-
             /* Copy device data to FIFO */
             len = MIN(len, fifo8_num_free(&s->fifo));
             fifo8_push_all(&s->fifo, s->async_buf, len);
-- 
2.39.2

[PATCH 25/88] esp.c: remove unaligned adjustment in do_dma_pdma_cb() to device path

[Mark Cave-Ayland]

With the latest changes this condition cannot occur anymore and so the logic
can be completely removed.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index ec82097a01..3db90c9ab7 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -604,15 +604,6 @@ static void do_dma_pdma_cb(ESPState *s)
         s->async_len -= n;
         s->ti_size += n;
 
-        if (n < len) {
-            /* Unaligned accesses can cause FIFO wraparound */
-            len = len - n;
-            n = esp_fifo_pop_buf(&s->fifo, s->async_buf, len);
-            s->async_buf += n;
-            s->async_len -= n;
-            s->ti_size += n;
-        }
-
         if (s->async_len == 0) {
             scsi_req_continue(s->current_req);
             return;
-- 
2.39.2

[PATCH 26/88] esp.c: remove unneeded if() check in esp_transfer_data()

[Mark Cave-Ayland]

The following ti_cmd checks ensure that only DMA and non-DMA TI commmands will
can call into the esp_do_dma() and esp_do_nodma() callbacks.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 3db90c9ab7..96123c5f7d 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -916,16 +916,13 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
         esp_raise_irq(s);
     }
 
-    if (s->ti_cmd == 0) {
-        /*
-         * Always perform the initial transfer upon reception of the next TI
-         * command to ensure the DMA/non-DMA status of the command is correct.
-         * It is not possible to use s->dma directly in the section below as
-         * some OSs send non-DMA NOP commands after a DMA transfer. Hence if the
-         * async data transfer is delayed then s->dma is set incorrectly.
-         */
-        return;
-    }
+    /*
+     * Always perform the initial transfer upon reception of the next TI
+     * command to ensure the DMA/non-DMA status of the command is correct.
+     * It is not possible to use s->dma directly in the section below as
+     * some OSs send non-DMA NOP commands after a DMA transfer. Hence if the
+     * async data transfer is delayed then s->dma is set incorrectly.
+     */
 
     if (s->ti_cmd == (CMD_TI | CMD_DMA)) {
         if (dmalen) {
-- 
2.39.2

[PATCH 27/88] esp.c: update end of transfer logic at the end of esp_transfer_data()

[Mark Cave-Ayland]

Since esp_dma_done() is called in both cases, and ti_size cannot be zero
(otherwise esp_command_complete() would have been called instead), replace
the conditional logic with a single call to esp_dma_done().

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 96123c5f7d..6b0811d3ce 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -925,16 +925,10 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
      */
 
     if (s->ti_cmd == (CMD_TI | CMD_DMA)) {
-        if (dmalen) {
-            esp_do_dma(s);
-        } else if (s->ti_size <= 0) {
-            /*
-             * If this was the last part of a DMA transfer then the
-             * completion interrupt is deferred to here.
-             */
-            esp_dma_done(s);
-            esp_lower_drq(s);
-        }
+        /* When the SCSI layer returns more data, raise deferred INTR_BS */
+        esp_dma_done(s);
+
+        esp_do_dma(s);
     } else if (s->ti_cmd == CMD_TI) {
         esp_do_nodma(s);
     }
-- 
2.39.2

[PATCH 28/88] esp.c: consolidate async_len and TC == 0 checks in do_dma_pdma_cb() and esp_do_dma()

[Mark Cave-Ayland]

Ensure that the async_len checks for requesting data from the SCSI layer and
the TC == 0 checks to detect the end of the DMA transfer are consistent in both
do_dma_pdma_cb() and esp_do_dma(). In particular this involves adding the check
to see if the FIFO is at its low threshold since PDMA and mixed DMA and non-DMA
requests can leave data remaining in the FIFO.

At the same time update all the comments so that they are also consistent between
all similar code paths.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 6b0811d3ce..f20026c3dc 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -604,12 +604,13 @@ static void do_dma_pdma_cb(ESPState *s)
         s->async_len -= n;
         s->ti_size += n;
 
-        if (s->async_len == 0) {
+        if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
+            /* Defer until the scsi layer has completed */
             scsi_req_continue(s->current_req);
             return;
         }
 
-        if (esp_get_tc(s) == 0) {
+        if (esp_get_tc(s) == 0 && fifo8_num_used(&s->fifo) < 2) {
             esp_lower_drq(s);
             esp_dma_done(s);
         }
@@ -706,24 +707,30 @@ static void esp_do_dma(ESPState *s)
             s->async_len -= len;
             s->ti_size += len;
 
-            if (s->async_len == 0) {
+            if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
+                /* Defer until the scsi layer has completed */
                 scsi_req_continue(s->current_req);
-                /*
-                 * If there is still data to be read from the device then
-                 * complete the DMA operation immediately.  Otherwise defer
-                 * until the scsi layer has completed.
-                 */
                 return;
             }
 
-            if (esp_get_tc(s) == 0) {
-                /* Partially filled a scsi buffer. Complete immediately.  */
+            if (esp_get_tc(s) == 0 && fifo8_num_used(&s->fifo) < 2) {
                 esp_dma_done(s);
                 esp_lower_drq(s);
             }
         } else {
             esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
             esp_raise_drq(s);
+
+            if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
+                /* Defer until the scsi layer has completed */
+                scsi_req_continue(s->current_req);
+                return;
+            }
+
+            if (esp_get_tc(s) == 0 && fifo8_num_used(&s->fifo) < 2) {
+                esp_dma_done(s);
+                esp_lower_drq(s);
+            }
         }
     } else {
         if (s->dma_memory_write) {
@@ -734,13 +741,13 @@ static void esp_do_dma(ESPState *s)
             s->async_len -= len;
             s->ti_size -= len;
 
-            if (s->async_len == 0) {
+            if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
+                /* Defer until the scsi layer has completed */
                 scsi_req_continue(s->current_req);
                 return;
             }
 
-            if (esp_get_tc(s) == 0) {
-                /* Partially filled a scsi buffer. Complete immediately.  */
+            if (esp_get_tc(s) == 0 && fifo8_num_used(&s->fifo) < 2) {
                 esp_dma_done(s);
                 esp_lower_drq(s);
             }
@@ -754,6 +761,17 @@ static void esp_do_dma(ESPState *s)
             esp_set_tc(s, esp_get_tc(s) - len);
             esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
             esp_raise_drq(s);
+
+            if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
+                /* Defer until the scsi layer has completed */
+                scsi_req_continue(s->current_req);
+                return;
+            }
+
+            if (esp_get_tc(s) == 0 && fifo8_num_used(&s->fifo) < 2) {
+                esp_lower_drq(s);
+                esp_dma_done(s);
+            }
         }
     }
 }
-- 
2.39.2

[PATCH 29/88] esp.c: fix premature end of phase logic esp_command_complete

[Mark Cave-Ayland]

There are two cases here: the first is when the TI command underflows, in which
case we raise INTR_BS to indicate an early change of phase, and the second is
when the TI command overflows because the host requested a transfer for more
data than is available. In the latter case force TC to zero so that the TI
completion logic executes correctly.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index f20026c3dc..c6151d306e 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -887,7 +887,6 @@ void esp_command_complete(SCSIRequest *req, size_t resid)
         if (s->ti_size != 0) {
             trace_esp_command_complete_unexpected();
         }
-        s->ti_size = 0;
     }
 
     s->async_len = 0;
@@ -897,13 +896,26 @@ void esp_command_complete(SCSIRequest *req, size_t resid)
     s->status = req->status;
 
     /*
-     * If the transfer is finished, switch to status phase. For non-DMA
-     * transfers from the target the last byte is still in the FIFO
+     * Switch to status phase. For non-DMA transfers from the target the last
+     * byte is still in the FIFO
      */
+    esp_set_phase(s, STAT_ST);
     if (s->ti_size == 0) {
-        esp_set_phase(s, STAT_ST);
+        /*
+         * Transfer complete: force TC to zero just in case a TI command was
+         * requested for more data than the command returns (Solaris 8 does
+         * this)
+         */
+        esp_set_tc(s, 0);
         esp_dma_done(s);
-        esp_lower_drq(s);
+    } else {
+        /*
+         * Transfer truncated: raise INTR_BS to indicate early change of
+         * phase
+         */
+        s->rregs[ESP_RINTR] |= INTR_BS;
+        esp_raise_irq(s);
+        s->ti_size = 0;
     }
 
     if (s->current_req) {
-- 
2.39.2

[PATCH 30/88] esp.c: move TC and FIFO check logic into esp_dma_done()

[Mark Cave-Ayland]

This helps simplify the existing implementation.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index c6151d306e..4d58a49c73 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -545,8 +545,11 @@ static void write_response(ESPState *s)
 
 static void esp_dma_done(ESPState *s)
 {
-    s->rregs[ESP_RINTR] |= INTR_BS;
-    esp_raise_irq(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);
+    }
 }
 
 static void do_dma_pdma_cb(ESPState *s)
@@ -610,12 +613,7 @@ static void do_dma_pdma_cb(ESPState *s)
             return;
         }
 
-        if (esp_get_tc(s) == 0 && fifo8_num_used(&s->fifo) < 2) {
-            esp_lower_drq(s);
-            esp_dma_done(s);
-        }
-
-        return;
+        esp_dma_done(s);
     } else {
         if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
             /* Defer until the scsi layer has completed */
@@ -624,10 +622,7 @@ static void do_dma_pdma_cb(ESPState *s)
             return;
         }
 
-        if (esp_get_tc(s) == 0 && fifo8_num_used(&s->fifo) < 2) {
-            esp_lower_drq(s);
-            esp_dma_done(s);
-        }
+        esp_dma_done(s);
 
         /* Copy device data to FIFO */
         len = MIN(s->async_len, esp_get_tc(s));
@@ -713,10 +708,7 @@ static void esp_do_dma(ESPState *s)
                 return;
             }
 
-            if (esp_get_tc(s) == 0 && fifo8_num_used(&s->fifo) < 2) {
-                esp_dma_done(s);
-                esp_lower_drq(s);
-            }
+            esp_dma_done(s);
         } else {
             esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
             esp_raise_drq(s);
@@ -727,10 +719,7 @@ static void esp_do_dma(ESPState *s)
                 return;
             }
 
-            if (esp_get_tc(s) == 0 && fifo8_num_used(&s->fifo) < 2) {
-                esp_dma_done(s);
-                esp_lower_drq(s);
-            }
+            esp_dma_done(s);
         }
     } else {
         if (s->dma_memory_write) {
@@ -747,10 +736,7 @@ static void esp_do_dma(ESPState *s)
                 return;
             }
 
-            if (esp_get_tc(s) == 0 && fifo8_num_used(&s->fifo) < 2) {
-                esp_dma_done(s);
-                esp_lower_drq(s);
-            }
+            esp_dma_done(s);
         } else {
             /* Copy device data to FIFO */
             len = MIN(len, fifo8_num_free(&s->fifo));
@@ -768,10 +754,7 @@ static void esp_do_dma(ESPState *s)
                 return;
             }
 
-            if (esp_get_tc(s) == 0 && fifo8_num_used(&s->fifo) < 2) {
-                esp_lower_drq(s);
-                esp_dma_done(s);
-            }
+            esp_dma_done(s);
         }
     }
 }
-- 
2.39.2

[PATCH 31/88] esp.c: rename esp_dma_done() to esp_dma_ti_check()

[Mark Cave-Ayland]

This is because a single DMA request can be transferred using multiple TI
commands, and so a TC equal to zero may not represent the completion of
the SCSI DMA command.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 4d58a49c73..14c6d05253 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -543,7 +543,7 @@ static void write_response(ESPState *s)
     esp_raise_irq(s);
 }
 
-static void esp_dma_done(ESPState *s)
+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;
@@ -613,7 +613,7 @@ static void do_dma_pdma_cb(ESPState *s)
             return;
         }
 
-        esp_dma_done(s);
+        esp_dma_ti_check(s);
     } else {
         if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
             /* Defer until the scsi layer has completed */
@@ -622,7 +622,7 @@ static void do_dma_pdma_cb(ESPState *s)
             return;
         }
 
-        esp_dma_done(s);
+        esp_dma_ti_check(s);
 
         /* Copy device data to FIFO */
         len = MIN(s->async_len, esp_get_tc(s));
@@ -708,7 +708,7 @@ static void esp_do_dma(ESPState *s)
                 return;
             }
 
-            esp_dma_done(s);
+            esp_dma_ti_check(s);
         } else {
             esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
             esp_raise_drq(s);
@@ -719,7 +719,7 @@ static void esp_do_dma(ESPState *s)
                 return;
             }
 
-            esp_dma_done(s);
+            esp_dma_ti_check(s);
         }
     } else {
         if (s->dma_memory_write) {
@@ -736,7 +736,7 @@ static void esp_do_dma(ESPState *s)
                 return;
             }
 
-            esp_dma_done(s);
+            esp_dma_ti_check(s);
         } else {
             /* Copy device data to FIFO */
             len = MIN(len, fifo8_num_free(&s->fifo));
@@ -754,7 +754,7 @@ static void esp_do_dma(ESPState *s)
                 return;
             }
 
-            esp_dma_done(s);
+            esp_dma_ti_check(s);
         }
     }
 }
@@ -890,7 +890,7 @@ void esp_command_complete(SCSIRequest *req, size_t resid)
          * this)
          */
         esp_set_tc(s, 0);
-        esp_dma_done(s);
+        esp_dma_ti_check(s);
     } else {
         /*
          * Transfer truncated: raise INTR_BS to indicate early change of
@@ -939,7 +939,7 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
 
     if (s->ti_cmd == (CMD_TI | CMD_DMA)) {
         /* When the SCSI layer returns more data, raise deferred INTR_BS */
-        esp_dma_done(s);
+        esp_dma_ti_check(s);
 
         esp_do_dma(s);
     } else if (s->ti_cmd == CMD_TI) {
-- 
2.39.2

[PATCH 32/88] esp.c: copy PDMA logic for transfers to device from do_dma_pdma_cb() to esp_do_dma()

[Mark Cave-Ayland]

This is so that PDMA transfers can be performend by esp_do_dma() as well as
do_dma_pdma_cb().

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 14c6d05253..65b4baab83 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -640,6 +640,7 @@ static void esp_do_dma(ESPState *s)
     uint32_t len, cmdlen;
     int to_device = ((s->rregs[ESP_RSTAT] & 7) == STAT_DO);
     uint8_t buf[ESP_CMDFIFO_SZ];
+    int n;
 
     len = esp_get_tc(s);
     if (s->do_cmd) {
@@ -710,6 +711,14 @@ static void esp_do_dma(ESPState *s)
 
             esp_dma_ti_check(s);
         } else {
+            /* Copy FIFO data to device */
+            len = MIN(s->async_len, ESP_FIFO_SZ);
+            len = MIN(len, fifo8_num_used(&s->fifo));
+            n = esp_fifo_pop_buf(&s->fifo, s->async_buf, len);
+            s->async_buf += n;
+            s->async_len -= n;
+            s->ti_size += n;
+
             esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
             esp_raise_drq(s);
 
-- 
2.39.2

[PATCH 33/88] esp.c: copy logic for do_cmd transfers from do_dma_pdma_cb() to esp_do_dma()

[Mark Cave-Ayland]

This is so that PDMA transfers can be performend by esp_do_dma() as well as
do_dma_pdma_cb().

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 65b4baab83..14284ef54a 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -656,9 +656,18 @@ static void esp_do_dma(ESPState *s)
             fifo8_push_all(&s->cmdfifo, buf, len);
             esp_set_tc(s, esp_get_tc(s) - len);
         } else {
+            n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            n = MIN(fifo8_num_free(&s->cmdfifo), n);
+            fifo8_push_all(&s->cmdfifo, buf, n);
+            esp_set_tc(s, esp_get_tc(s) - n);
+
             esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
             esp_raise_drq(s);
-            return;
+
+            /* Ensure we have received complete command after SATN and stop */
+            if (esp_get_tc(s) || fifo8_is_empty(&s->cmdfifo)) {
+                return;
+            }
         }
         trace_esp_handle_ti_cmd(cmdlen);
         s->ti_size = 0;
-- 
2.39.2

[PATCH 34/88] esp.c: update esp_do_dma() bypass if async_len is zero to include non-zero transfer check

[Mark Cave-Ayland]

In the PDMA case the last transfer from the device to the FIFO has occurred
(async_len is zero) but esp_do_dma() is still being called to drain the
remaining FIFO contents.

The additional non-zero transfer check ensures that we still defer the SCSI
layer in the case where we are waiting for data for a TI command or a DMA
enable signal.

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 14284ef54a..9647be4cc3 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -696,7 +696,7 @@ static void esp_do_dma(ESPState *s)
     if (!s->current_req) {
         return;
     }
-    if (s->async_len == 0) {
+    if (s->async_len == 0 && esp_get_tc(s) && s->ti_size) {
         /* Defer until data is available.  */
         return;
     }
-- 
2.39.2

[PATCH 35/88] esp.c: move end of SCSI transfer check after TC adjustment in do_dma_pdma_cb()

[Mark Cave-Ayland]

Now it is possible to move the end of SCSI transfer check to after the TC
adjustment in do_dma_pdma_cb() when transferring data from the device
without triggering an assert() in the SCSI code. This brings this check in
line with all the others in esp_do_dma() and do_dma_pdma_cb().

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 9647be4cc3..df4d5f8811 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -615,15 +615,6 @@ static void do_dma_pdma_cb(ESPState *s)
 
         esp_dma_ti_check(s);
     } else {
-        if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
-            /* Defer until the scsi layer has completed */
-            scsi_req_continue(s->current_req);
-            s->data_in_ready = false;
-            return;
-        }
-
-        esp_dma_ti_check(s);
-
         /* Copy device data to FIFO */
         len = MIN(s->async_len, esp_get_tc(s));
         len = MIN(len, fifo8_num_free(&s->fifo));
@@ -632,6 +623,15 @@ static void do_dma_pdma_cb(ESPState *s)
         s->async_len -= len;
         s->ti_size -= len;
         esp_set_tc(s, esp_get_tc(s) - len);
+
+        if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
+            /* Defer until the scsi layer has completed */
+            scsi_req_continue(s->current_req);
+            s->data_in_ready = false;
+            return;
+        }
+
+        esp_dma_ti_check(s);
     }
 }
 
-- 
2.39.2

[PATCH 36/88] esp.c: remove s_without_satn_pdma_cb() PDMA callback

[Mark Cave-Ayland]

This can now be handled by the existing do_dma_pdma_cb() function.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index df4d5f8811..16cb6c72fd 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -408,23 +408,6 @@ static void handle_satn(ESPState *s)
     }
 }
 
-static void s_without_satn_pdma_cb(ESPState *s)
-{
-    uint8_t buf[ESP_FIFO_SZ];
-    int n;
-
-    /* Copy FIFO into cmdfifo */
-    n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
-    n = MIN(fifo8_num_free(&s->cmdfifo), n);
-    fifo8_push_all(&s->cmdfifo, buf, n);
-
-    if (!esp_get_tc(s) && !fifo8_is_empty(&s->cmdfifo)) {
-        s->cmdfifo_cdb_offset = 0;
-        s->do_cmd = 0;
-        do_cmd(s);
-    }
-}
-
 static void handle_s_without_atn(ESPState *s)
 {
     int32_t cmdlen;
@@ -433,7 +416,7 @@ static void handle_s_without_atn(ESPState *s)
         s->dma_cb = handle_s_without_atn;
         return;
     }
-    esp_set_pdma_cb(s, S_WITHOUT_SATN_PDMA_CB);
+    esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
     if (esp_select(s) < 0) {
         return;
     }
@@ -856,9 +839,6 @@ static void esp_pdma_cb(ESPState *s)
     case SATN_PDMA_CB:
         satn_pdma_cb(s);
         break;
-    case S_WITHOUT_SATN_PDMA_CB:
-        s_without_satn_pdma_cb(s);
-        break;
     case SATN_STOP_PDMA_CB:
         satn_stop_pdma_cb(s);
         break;
diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
index 13b17496f8..b727181da9 100644
--- a/include/hw/scsi/esp.h
+++ b/include/hw/scsi/esp.h
@@ -153,7 +153,6 @@ struct SysBusESPState {
 /* PDMA callbacks */
 enum pdma_cb {
     SATN_PDMA_CB = 0,
-    S_WITHOUT_SATN_PDMA_CB = 1,
     SATN_STOP_PDMA_CB = 2,
     WRITE_RESPONSE_PDMA_CB = 3,
     DO_DMA_PDMA_CB = 4
-- 
2.39.2

[PATCH 37/88] esp.c: introduce esp_get_phase() function

[Mark Cave-Ayland]

Make use of this new function in all places where the SCSI phase bits are
manually masked from the ESP_RSTAT register.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 16cb6c72fd..de8d98082a 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -196,6 +196,11 @@ static void esp_set_phase(ESPState *s, uint8_t 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;
@@ -537,7 +542,7 @@ static void esp_dma_ti_check(ESPState *s)
 
 static void do_dma_pdma_cb(ESPState *s)
 {
-    int to_device = ((s->rregs[ESP_RSTAT] & 7) == STAT_DO);
+    int to_device = (esp_get_phase(s) == STAT_DO);
     uint8_t buf[ESP_CMDFIFO_SZ];
     int len;
     uint32_t n;
@@ -554,7 +559,7 @@ static void do_dma_pdma_cb(ESPState *s)
         }
 
         s->ti_size = 0;
-        if ((s->rregs[ESP_RSTAT] & 7) == STAT_CD) {
+        if (esp_get_phase(s) == STAT_CD) {
             /* No command received */
             if (s->cmdfifo_cdb_offset == fifo8_num_used(&s->cmdfifo)) {
                 return;
@@ -621,7 +626,7 @@ static void do_dma_pdma_cb(ESPState *s)
 static void esp_do_dma(ESPState *s)
 {
     uint32_t len, cmdlen;
-    int to_device = ((s->rregs[ESP_RSTAT] & 7) == STAT_DO);
+    int to_device = (esp_get_phase(s) == STAT_DO);
     uint8_t buf[ESP_CMDFIFO_SZ];
     int n;
 
@@ -654,7 +659,7 @@ static void esp_do_dma(ESPState *s)
         }
         trace_esp_handle_ti_cmd(cmdlen);
         s->ti_size = 0;
-        if ((s->rregs[ESP_RSTAT] & 7) == STAT_CD) {
+        if (esp_get_phase(s) == STAT_CD) {
             /* No command received */
             if (s->cmdfifo_cdb_offset == fifo8_num_used(&s->cmdfifo)) {
                 return;
@@ -762,7 +767,7 @@ static void esp_do_dma(ESPState *s)
 
 static void esp_do_nodma(ESPState *s)
 {
-    int to_device = ((s->rregs[ESP_RSTAT] & 7) == STAT_DO);
+    int to_device = (esp_get_phase(s) == STAT_DO);
     uint8_t buf[ESP_FIFO_SZ];
     uint32_t cmdlen;
     int len, n;
@@ -776,7 +781,7 @@ static void esp_do_nodma(ESPState *s)
         cmdlen = fifo8_num_used(&s->cmdfifo);
         trace_esp_handle_ti_cmd(cmdlen);
         s->ti_size = 0;
-        if ((s->rregs[ESP_RSTAT] & 7) == STAT_CD) {
+        if (esp_get_phase(s) == STAT_CD) {
             /* No command received */
             if (s->cmdfifo_cdb_offset == fifo8_num_used(&s->cmdfifo)) {
                 return;
@@ -856,7 +861,7 @@ static void esp_pdma_cb(ESPState *s)
 void esp_command_complete(SCSIRequest *req, size_t resid)
 {
     ESPState *s = req->hba_private;
-    int to_device = ((s->rregs[ESP_RSTAT] & 7) == STAT_DO);
+    int to_device = (esp_get_phase(s) == STAT_DO);
 
     trace_esp_command_complete();
 
@@ -909,7 +914,7 @@ void esp_command_complete(SCSIRequest *req, size_t resid)
 void esp_transfer_data(SCSIRequest *req, uint32_t len)
 {
     ESPState *s = req->hba_private;
-    int to_device = ((s->rregs[ESP_RSTAT] & 7) == STAT_DO);
+    int to_device = (esp_get_phase(s) == STAT_DO);
     uint32_t dmalen = esp_get_tc(s);
 
     assert(!s->do_cmd);
@@ -1103,7 +1108,7 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr)
             qemu_log_mask(LOG_UNIMP, "esp: PIO data read not implemented\n");
             s->rregs[ESP_FIFO] = 0;
         } else {
-            if ((s->rregs[ESP_RSTAT] & 0x7) == STAT_DI) {
+            if (esp_get_phase(s) == STAT_DI) {
                 if (s->ti_size) {
                     esp_do_nodma(s);
                 } else {
-- 
2.39.2

Re: [PATCH 37/88] esp.c: introduce esp_get_phase() function

[Philippe Mathieu-Daudé]

On 12/1/24 13:53, Mark Cave-Ayland wrote:
> Make use of this new function in all places where the SCSI phase bits are
> manually masked from the ESP_RSTAT register.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 23 ++++++++++++++---------
>   1 file changed, 14 insertions(+), 9 deletions(-)

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

[PATCH 38/88] esp.c: convert esp_do_dma() to switch statement based upon SCSI phase

[Mark Cave-Ayland]

Currently only the DATA IN and DATA OUT phases are supported.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index de8d98082a..67d1d39db2 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -626,7 +626,6 @@ static void do_dma_pdma_cb(ESPState *s)
 static void esp_do_dma(ESPState *s)
 {
     uint32_t len, cmdlen;
-    int to_device = (esp_get_phase(s) == STAT_DO);
     uint8_t buf[ESP_CMDFIFO_SZ];
     int n;
 
@@ -681,17 +680,19 @@ static void esp_do_dma(ESPState *s)
         }
         return;
     }
-    if (!s->current_req) {
-        return;
-    }
-    if (s->async_len == 0 && esp_get_tc(s) && s->ti_size) {
-        /* Defer until data is available.  */
-        return;
-    }
-    if (len > s->async_len) {
-        len = s->async_len;
-    }
-    if (to_device) {
+
+    switch (esp_get_phase(s)) {
+    case STAT_DO:
+        if (!s->current_req) {
+            return;
+        }
+        if (s->async_len == 0 && esp_get_tc(s) && s->ti_size) {
+            /* Defer until data is available.  */
+            return;
+        }
+        if (len > s->async_len) {
+            len = s->async_len;
+        }
         if (s->dma_memory_read) {
             s->dma_memory_read(s->dma_opaque, s->async_buf, len);
 
@@ -727,7 +728,19 @@ static void esp_do_dma(ESPState *s)
 
             esp_dma_ti_check(s);
         }
-    } else {
+        break;
+
+    case STAT_DI:
+        if (!s->current_req) {
+            return;
+        }
+        if (s->async_len == 0 && esp_get_tc(s) && s->ti_size) {
+            /* Defer until data is available.  */
+            return;
+        }
+        if (len > s->async_len) {
+            len = s->async_len;
+        }
         if (s->dma_memory_write) {
             s->dma_memory_write(s->dma_opaque, s->async_buf, len);
 
@@ -762,6 +775,7 @@ static void esp_do_dma(ESPState *s)
 
             esp_dma_ti_check(s);
         }
+        break;
     }
 }
 
-- 
2.39.2

[PATCH 39/88] esp.c: convert do_dma_pdma_db() to switch statement based upon SCSI phase

[Mark Cave-Ayland]

Currently only the DATA IN and DATA OUT phases are supported.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 67d1d39db2..f6d05b0de7 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -542,7 +542,6 @@ static void esp_dma_ti_check(ESPState *s)
 
 static void do_dma_pdma_cb(ESPState *s)
 {
-    int to_device = (esp_get_phase(s) == STAT_DO);
     uint8_t buf[ESP_CMDFIFO_SZ];
     int len;
     uint32_t n;
@@ -582,11 +581,11 @@ static void do_dma_pdma_cb(ESPState *s)
         return;
     }
 
-    if (!s->current_req) {
-        return;
-    }
-
-    if (to_device) {
+    switch (esp_get_phase(s)) {
+    case STAT_DO:
+        if (!s->current_req) {
+            return;
+        }
         /* Copy FIFO data to device */
         len = MIN(s->async_len, ESP_FIFO_SZ);
         len = MIN(len, fifo8_num_used(&s->fifo));
@@ -602,7 +601,12 @@ static void do_dma_pdma_cb(ESPState *s)
         }
 
         esp_dma_ti_check(s);
-    } else {
+        break;
+
+    case STAT_DI:
+        if (!s->current_req) {
+            return;
+        }
         /* Copy device data to FIFO */
         len = MIN(s->async_len, esp_get_tc(s));
         len = MIN(len, fifo8_num_free(&s->fifo));
@@ -620,6 +624,7 @@ static void do_dma_pdma_cb(ESPState *s)
         }
 
         esp_dma_ti_check(s);
+        break;
     }
 }
 
-- 
2.39.2

[PATCH 40/88] esp.c: convert esp_do_nodma() to switch statement based upon SCSI phase

[Mark Cave-Ayland]

Currently only the DATA IN and DATA OUT phases are supported.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index f6d05b0de7..c1b44e5f18 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -786,7 +786,6 @@ static void esp_do_dma(ESPState *s)
 
 static void esp_do_nodma(ESPState *s)
 {
-    int to_device = (esp_get_phase(s) == STAT_DO);
     uint8_t buf[ESP_FIFO_SZ];
     uint32_t cmdlen;
     int len, n;
@@ -823,38 +822,55 @@ static void esp_do_nodma(ESPState *s)
         return;
     }
 
-    if (!s->current_req) {
-        return;
-    }
-
-    if (s->async_len == 0) {
-        /* Defer until data is available.  */
-        return;
-    }
-
-    if (to_device) {
+    switch (esp_get_phase(s)) {
+    case STAT_DO:
+        if (!s->current_req) {
+            return;
+        }
+        if (s->async_len == 0) {
+            /* Defer until data is available.  */
+            return;
+        }
         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);
         s->async_buf += len;
         s->async_len -= len;
         s->ti_size += len;
-    } else {
+
+        if (s->async_len == 0) {
+            scsi_req_continue(s->current_req);
+            return;
+        }
+
+        s->rregs[ESP_RINTR] |= INTR_BS;
+        esp_raise_irq(s);
+        break;
+
+    case STAT_DI:
+        if (!s->current_req) {
+            return;
+        }
+        if (s->async_len == 0) {
+            /* Defer until data is available.  */
+            return;
+        }
         if (fifo8_is_empty(&s->fifo)) {
             fifo8_push(&s->fifo, s->async_buf[0]);
             s->async_buf++;
             s->async_len--;
             s->ti_size--;
         }
-    }
 
-    if (s->async_len == 0) {
-        scsi_req_continue(s->current_req);
-        return;
-    }
+        if (s->async_len == 0) {
+            scsi_req_continue(s->current_req);
+            return;
+        }
 
-    s->rregs[ESP_RINTR] |= INTR_BS;
-    esp_raise_irq(s);
+        s->rregs[ESP_RINTR] |= INTR_BS;
+        esp_raise_irq(s);
+        break;
+    }
 }
 
 static void esp_pdma_cb(ESPState *s)
-- 
2.39.2

[PATCH 41/88] esp.c: convert esp_do_dma() do_cmd path to check for SCSI phase instead

[Mark Cave-Ayland]

Currently do_cmd is used to determine whether MESSAGE OUT and COMMAND phase data
is being accumulated in cmdfifo. Update esp_do_dma() to check directly for these
two SCSI phases instead.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index c1b44e5f18..22739d3875 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -635,11 +635,10 @@ static void esp_do_dma(ESPState *s)
     int n;
 
     len = esp_get_tc(s);
-    if (s->do_cmd) {
-        /*
-         * handle_ti_cmd() case: esp_do_dma() is called only from
-         * handle_ti_cmd() with do_cmd != NULL (see the assert())
-         */
+
+    switch (esp_get_phase(s)) {
+    case STAT_MO:
+    case STAT_CD:
         cmdlen = fifo8_num_used(&s->cmdfifo);
         trace_esp_do_dma(cmdlen, len);
         if (s->dma_memory_read) {
@@ -683,10 +682,8 @@ static void esp_do_dma(ESPState *s)
             s->rregs[ESP_RINTR] |= INTR_BS;
             esp_raise_irq(s);
         }
-        return;
-    }
+        break;
 
-    switch (esp_get_phase(s)) {
     case STAT_DO:
         if (!s->current_req) {
             return;
-- 
2.39.2

[PATCH 42/88] esp.c: convert do_dma_pdma_cb() do_cmd path to check for SCSI phase instead

[Mark Cave-Ayland]

Currently do_cmd is used to determine whether MESSAGE OUT and COMMAND phase data
is being accumulated in cmdfifo. Update esp_do_dma() to check directly for these
two SCSI phases instead.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 22739d3875..b48e020689 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -546,7 +546,9 @@ static void do_dma_pdma_cb(ESPState *s)
     int len;
     uint32_t n;
 
-    if (s->do_cmd) {
+    switch (esp_get_phase(s)) {
+    case STAT_MO:
+    case STAT_CD:
         /* Copy FIFO into cmdfifo */
         n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
         n = MIN(fifo8_num_free(&s->cmdfifo), n);
@@ -578,10 +580,8 @@ static void do_dma_pdma_cb(ESPState *s)
             s->rregs[ESP_RINTR] |= INTR_BS;
             esp_raise_irq(s);
         }
-        return;
-    }
+        break;
 
-    switch (esp_get_phase(s)) {
     case STAT_DO:
         if (!s->current_req) {
             return;
-- 
2.39.2

[PATCH 43/88] esp.c: convert esp_do_nodma() do_cmd path to check for SCSI phase instead

[Mark Cave-Ayland]

Currently do_cmd is used to determine whether MESSAGE OUT and COMMAND phase data
is being accumulated in cmdfifo. Update esp_do_dma() to check directly for these
two SCSI phases instead.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index b48e020689..14759ada78 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -787,7 +787,9 @@ static void esp_do_nodma(ESPState *s)
     uint32_t cmdlen;
     int len, n;
 
-    if (s->do_cmd) {
+    switch (esp_get_phase(s)) {
+    case STAT_MO:
+    case STAT_CD:
         /* Copy FIFO into cmdfifo */
         n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
         n = MIN(fifo8_num_free(&s->cmdfifo), n);
@@ -816,10 +818,8 @@ static void esp_do_nodma(ESPState *s)
             s->rregs[ESP_RINTR] |= INTR_BS;
             esp_raise_irq(s);
         }
-        return;
-    }
+        break;
 
-    switch (esp_get_phase(s)) {
     case STAT_DO:
         if (!s->current_req) {
             return;
-- 
2.39.2

[PATCH 44/88] esp.c: convert esp_reg_write() do_cmd path to check for SCSI phase instead

[Mark Cave-Ayland]

Currently do_cmd is used to determine whether MESSAGE OUT and COMMAND phase data
is being accumulated in cmdfifo. Update esp_do_dma() to check directly for these
two SCSI phases instead.

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 14759ada78..e679b1c39b 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -1207,7 +1207,7 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val)
         s->rregs[ESP_RSTAT] &= ~STAT_TC;
         break;
     case ESP_FIFO:
-        if (s->do_cmd) {
+        if (esp_get_phase(s) == STAT_MO || esp_get_phase(s) == STAT_CD) {
             if (!fifo8_is_full(&s->fifo)) {
                 esp_fifo_push(&s->fifo, val);
                 esp_fifo_push(&s->cmdfifo, fifo8_pop(&s->fifo));
-- 
2.39.2

[PATCH 45/88] esp.c: remove do_cmd from ESPState

[Mark Cave-Ayland]

Now that the accumulation of the CDB is handled by SCSI phase, there is no need
for a separate variable to control it.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index e679b1c39b..1f7dff4ca6 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -380,7 +380,6 @@ static void satn_pdma_cb(ESPState *s)
 
     if (!esp_get_tc(s) && !fifo8_is_empty(&s->cmdfifo)) {
         s->cmdfifo_cdb_offset = 1;
-        s->do_cmd = 0;
         do_cmd(s);
     }
 }
@@ -400,13 +399,11 @@ static void handle_satn(ESPState *s)
     cmdlen = get_cmd(s, ESP_CMDFIFO_SZ);
     if (cmdlen > 0) {
         s->cmdfifo_cdb_offset = 1;
-        s->do_cmd = 0;
         do_cmd(s);
     } else if (cmdlen == 0) {
         if (s->dma) {
             esp_raise_drq(s);
         }
-        s->do_cmd = 1;
         /* Target present, but no cmd yet - switch to command phase */
         s->rregs[ESP_RSEQ] = SEQ_CD;
         esp_set_phase(s, STAT_CD);
@@ -428,13 +425,11 @@ static void handle_s_without_atn(ESPState *s)
     cmdlen = get_cmd(s, ESP_CMDFIFO_SZ);
     if (cmdlen > 0) {
         s->cmdfifo_cdb_offset = 0;
-        s->do_cmd = 0;
         do_cmd(s);
     } else if (cmdlen == 0) {
         if (s->dma) {
             esp_raise_drq(s);
         }
-        s->do_cmd = 1;
         /* Target present, but no cmd yet - switch to command phase */
         s->rregs[ESP_RSEQ] = SEQ_CD;
         esp_set_phase(s, STAT_CD);
@@ -453,7 +448,6 @@ static void satn_stop_pdma_cb(ESPState *s)
 
     if (!esp_get_tc(s) && !fifo8_is_empty(&s->cmdfifo)) {
         trace_esp_handle_satn_stop(fifo8_num_used(&s->cmdfifo));
-        s->do_cmd = 1;
         s->cmdfifo_cdb_offset = 1;
         esp_set_phase(s, STAT_CD);
         s->rregs[ESP_RSTAT] |= STAT_TC;
@@ -478,7 +472,6 @@ static void handle_satn_stop(ESPState *s)
     cmdlen = get_cmd(s, 1);
     if (cmdlen > 0) {
         trace_esp_handle_satn_stop(fifo8_num_used(&s->cmdfifo));
-        s->do_cmd = 1;
         s->cmdfifo_cdb_offset = 1;
         esp_set_phase(s, STAT_MO);
         s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
@@ -488,7 +481,6 @@ static void handle_satn_stop(ESPState *s)
         if (s->dma) {
             esp_raise_drq(s);
         }
-        s->do_cmd = 1;
         /* Target present, switch to message out phase */
         s->rregs[ESP_RSEQ] = SEQ_MO;
         esp_set_phase(s, STAT_MO);
@@ -567,7 +559,6 @@ static void do_dma_pdma_cb(ESPState *s)
             }
 
             /* Command has been received */
-            s->do_cmd = 0;
             do_cmd(s);
         } else {
             /*
@@ -669,7 +660,6 @@ static void esp_do_dma(ESPState *s)
             }
 
             /* Command has been received */
-            s->do_cmd = 0;
             do_cmd(s);
         } else {
             /*
@@ -805,7 +795,6 @@ static void esp_do_nodma(ESPState *s)
             }
 
             /* Command has been received */
-            s->do_cmd = 0;
             do_cmd(s);
         } else {
             /*
@@ -949,7 +938,6 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
     int to_device = (esp_get_phase(s) == STAT_DO);
     uint32_t dmalen = esp_get_tc(s);
 
-    assert(!s->do_cmd);
     trace_esp_transfer_data(dmalen, s->ti_size);
     s->async_len = len;
     s->async_buf = scsi_req_get_buf(req);
@@ -1012,7 +1000,6 @@ void esp_hard_reset(ESPState *s)
     fifo8_reset(&s->fifo);
     fifo8_reset(&s->cmdfifo);
     s->dma = 0;
-    s->do_cmd = 0;
     s->dma_cb = NULL;
 
     s->rregs[ESP_CFG1] = 7;
-- 
2.39.2

[PATCH 46/88] esp.c: untangle MESSAGE OUT and COMMAND phase logic in esp_do_dma()

[Mark Cave-Ayland]

This makes it clearer that ATN is asserted until the end of the next TI command
in the MESSAGE OUT phase.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 1f7dff4ca6..2f54fd1285 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -629,6 +629,31 @@ static void esp_do_dma(ESPState *s)
 
     switch (esp_get_phase(s)) {
     case STAT_MO:
+        if (s->dma_memory_read) {
+            len = MIN(len, fifo8_num_free(&s->cmdfifo));
+            s->dma_memory_read(s->dma_opaque, buf, len);
+            fifo8_push_all(&s->cmdfifo, buf, len);
+            esp_set_tc(s, esp_get_tc(s) - len);
+            s->cmdfifo_cdb_offset += len;
+        } else {
+            n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            n = MIN(fifo8_num_free(&s->cmdfifo), n);
+            fifo8_push_all(&s->cmdfifo, buf, n);
+            s->cmdfifo_cdb_offset += n;
+        }
+
+        esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
+        esp_raise_drq(s);
+
+        /* ATN remains asserted until TC == 0 */
+        if (esp_get_tc(s) == 0) {
+            esp_set_phase(s, STAT_CD);
+            s->rregs[ESP_RSEQ] = SEQ_CD;
+            s->rregs[ESP_RINTR] |= INTR_BS;
+            esp_raise_irq(s);
+        }
+        break;
+
     case STAT_CD:
         cmdlen = fifo8_num_used(&s->cmdfifo);
         trace_esp_do_dma(cmdlen, len);
@@ -641,36 +666,15 @@ static void esp_do_dma(ESPState *s)
             n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
             n = MIN(fifo8_num_free(&s->cmdfifo), n);
             fifo8_push_all(&s->cmdfifo, buf, n);
-            esp_set_tc(s, esp_get_tc(s) - n);
 
             esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
             esp_raise_drq(s);
-
-            /* Ensure we have received complete command after SATN and stop */
-            if (esp_get_tc(s) || fifo8_is_empty(&s->cmdfifo)) {
-                return;
-            }
         }
         trace_esp_handle_ti_cmd(cmdlen);
         s->ti_size = 0;
-        if (esp_get_phase(s) == STAT_CD) {
-            /* No command received */
-            if (s->cmdfifo_cdb_offset == fifo8_num_used(&s->cmdfifo)) {
-                return;
-            }
-
+        if (esp_get_tc(s) == 0) {
             /* Command has been received */
             do_cmd(s);
-        } else {
-            /*
-             * Extra message out bytes received: update cmdfifo_cdb_offset
-             * and then switch to command phase
-             */
-            s->cmdfifo_cdb_offset = fifo8_num_used(&s->cmdfifo);
-            esp_set_phase(s, STAT_CD);
-            s->rregs[ESP_RSEQ] = SEQ_CD;
-            s->rregs[ESP_RINTR] |= INTR_BS;
-            esp_raise_irq(s);
         }
         break;
 
-- 
2.39.2

[PATCH 47/88] esp.c: untangle MESSAGE OUT and COMMAND phase logic in do_dma_pdma_cb()

[Mark Cave-Ayland]

This makes it clearer that ATN is asserted until the end of the next TI command
in the MESSAGE OUT phase.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 2f54fd1285..7ab195f884 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -536,36 +536,30 @@ static void do_dma_pdma_cb(ESPState *s)
 {
     uint8_t buf[ESP_CMDFIFO_SZ];
     int len;
-    uint32_t n;
+    uint32_t n, cmdlen;
+
+    len = esp_get_tc(s);
 
     switch (esp_get_phase(s)) {
     case STAT_MO:
-    case STAT_CD:
-        /* Copy FIFO into cmdfifo */
-        n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
-        n = MIN(fifo8_num_free(&s->cmdfifo), n);
-        fifo8_push_all(&s->cmdfifo, buf, n);
-
-        /* Ensure we have received complete command after SATN and stop */
-        if (esp_get_tc(s) || fifo8_is_empty(&s->cmdfifo)) {
-            return;
+        if (s->dma_memory_read) {
+            len = MIN(len, fifo8_num_free(&s->cmdfifo));
+            s->dma_memory_read(s->dma_opaque, buf, len);
+            fifo8_push_all(&s->cmdfifo, buf, len);
+            esp_set_tc(s, esp_get_tc(s) - len);
+            s->cmdfifo_cdb_offset += len;
+        } else {
+            n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            n = MIN(fifo8_num_free(&s->cmdfifo), n);
+            fifo8_push_all(&s->cmdfifo, buf, n);
+            s->cmdfifo_cdb_offset += n;
         }
 
-        s->ti_size = 0;
-        if (esp_get_phase(s) == STAT_CD) {
-            /* No command received */
-            if (s->cmdfifo_cdb_offset == fifo8_num_used(&s->cmdfifo)) {
-                return;
-            }
+        esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
+        esp_raise_drq(s);
 
-            /* Command has been received */
-            do_cmd(s);
-        } else {
-            /*
-             * Extra message out bytes received: update cmdfifo_cdb_offset
-             * and then switch to command phase
-             */
-            s->cmdfifo_cdb_offset = fifo8_num_used(&s->cmdfifo);
+        /* ATN remains asserted until TC == 0 */
+        if (esp_get_tc(s) == 0) {
             esp_set_phase(s, STAT_CD);
             s->rregs[ESP_RSEQ] = SEQ_CD;
             s->rregs[ESP_RINTR] |= INTR_BS;
@@ -573,6 +567,30 @@ static void do_dma_pdma_cb(ESPState *s)
         }
         break;
 
+    case STAT_CD:
+        cmdlen = fifo8_num_used(&s->cmdfifo);
+        trace_esp_do_dma(cmdlen, len);
+        if (s->dma_memory_read) {
+            len = MIN(len, fifo8_num_free(&s->cmdfifo));
+            s->dma_memory_read(s->dma_opaque, buf, len);
+            fifo8_push_all(&s->cmdfifo, buf, len);
+            esp_set_tc(s, esp_get_tc(s) - len);
+        } else {
+            n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            n = MIN(fifo8_num_free(&s->cmdfifo), n);
+            fifo8_push_all(&s->cmdfifo, buf, n);
+
+            esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
+            esp_raise_drq(s);
+        }
+        trace_esp_handle_ti_cmd(cmdlen);
+        s->ti_size = 0;
+        if (esp_get_tc(s) == 0) {
+            /* Command has been received */
+            do_cmd(s);
+        }
+        break;
+
     case STAT_DO:
         if (!s->current_req) {
             return;
-- 
2.39.2

[PATCH 48/88] esp.c: untangle MESSAGE OUT and COMMAND phase logic in esp_do_nodma()

[Mark Cave-Ayland]

This makes it clearer that ATN is asserted until the end of the next TI command
in the MESSAGE OUT phase.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 7ab195f884..81144ace83 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -801,6 +801,23 @@ static void esp_do_nodma(ESPState *s)
 
     switch (esp_get_phase(s)) {
     case STAT_MO:
+        /* Copy FIFO into cmdfifo */
+        n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+        n = MIN(fifo8_num_free(&s->cmdfifo), n);
+        fifo8_push_all(&s->cmdfifo, buf, n);
+        s->cmdfifo_cdb_offset += n;
+
+        /*
+         * Extra message out bytes received: update cmdfifo_cdb_offset
+         * and then switch to command phase
+         */
+        s->cmdfifo_cdb_offset = fifo8_num_used(&s->cmdfifo);
+        esp_set_phase(s, STAT_CD);
+        s->rregs[ESP_RSEQ] = SEQ_CD;
+        s->rregs[ESP_RINTR] |= INTR_BS;
+        esp_raise_irq(s);
+        break;
+
     case STAT_CD:
         /* Copy FIFO into cmdfifo */
         n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
@@ -810,25 +827,14 @@ static void esp_do_nodma(ESPState *s)
         cmdlen = fifo8_num_used(&s->cmdfifo);
         trace_esp_handle_ti_cmd(cmdlen);
         s->ti_size = 0;
-        if (esp_get_phase(s) == STAT_CD) {
-            /* No command received */
-            if (s->cmdfifo_cdb_offset == fifo8_num_used(&s->cmdfifo)) {
-                return;
-            }
 
-            /* Command has been received */
-            do_cmd(s);
-        } else {
-            /*
-             * Extra message out bytes received: update cmdfifo_cdb_offset
-             * and then switch to command phase
-             */
-            s->cmdfifo_cdb_offset = fifo8_num_used(&s->cmdfifo);
-            esp_set_phase(s, STAT_CD);
-            s->rregs[ESP_RSEQ] = SEQ_CD;
-            s->rregs[ESP_RINTR] |= INTR_BS;
-            esp_raise_irq(s);
+        /* No command received */
+        if (s->cmdfifo_cdb_offset == fifo8_num_used(&s->cmdfifo)) {
+            return;
         }
+
+        /* Command has been received */
+        do_cmd(s);
         break;
 
     case STAT_DO:
-- 
2.39.2

[PATCH 49/88] esp.c: move CMD_SELATN end of message phase detection to esp_do_dma() and do_dma_pdma_cb()

[Mark Cave-Ayland]

The special logic in satn_pdma_cb() is now no longer required since esp_do_dma()
can be used as a direct replacement.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 81144ace83..f8c20d0584 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -261,6 +261,9 @@ static int esp_select(ESPState *s)
     return 0;
 }
 
+static void esp_do_dma(ESPState *s);
+static void esp_do_nodma(ESPState *s);
+
 static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
 {
     uint8_t buf[ESP_CMDFIFO_SZ];
@@ -368,45 +371,26 @@ static void do_cmd(ESPState *s)
     do_command_phase(s);
 }
 
-static void satn_pdma_cb(ESPState *s)
-{
-    uint8_t buf[ESP_FIFO_SZ];
-    int n;
-
-    /* Copy FIFO into cmdfifo */
-    n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
-    n = MIN(fifo8_num_free(&s->cmdfifo), n);
-    fifo8_push_all(&s->cmdfifo, buf, n);
-
-    if (!esp_get_tc(s) && !fifo8_is_empty(&s->cmdfifo)) {
-        s->cmdfifo_cdb_offset = 1;
-        do_cmd(s);
-    }
-}
-
 static void handle_satn(ESPState *s)
 {
-    int32_t cmdlen;
-
     if (s->dma && !s->dma_enabled) {
         s->dma_cb = handle_satn;
         return;
     }
-    esp_set_pdma_cb(s, SATN_PDMA_CB);
+    esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
     if (esp_select(s) < 0) {
         return;
     }
-    cmdlen = get_cmd(s, ESP_CMDFIFO_SZ);
-    if (cmdlen > 0) {
-        s->cmdfifo_cdb_offset = 1;
-        do_cmd(s);
-    } else if (cmdlen == 0) {
-        if (s->dma) {
-            esp_raise_drq(s);
+
+    esp_set_phase(s, STAT_MO);
+
+    if (s->dma) {
+        esp_do_dma(s);
+    } else {
+        if (get_cmd(s, ESP_CMDFIFO_SZ)) {
+            s->cmdfifo_cdb_offset = 1;
+            do_cmd(s);
         }
-        /* Target present, but no cmd yet - switch to command phase */
-        s->rregs[ESP_RSEQ] = SEQ_CD;
-        esp_set_phase(s, STAT_CD);
     }
 }
 
@@ -558,6 +542,21 @@ static void do_dma_pdma_cb(ESPState *s)
         esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
         esp_raise_drq(s);
 
+        switch (s->rregs[ESP_CMD]) {
+        case CMD_SELATN | CMD_DMA:
+            if (fifo8_num_used(&s->cmdfifo) >= 1) {
+                /* First byte received, switch to command phase */
+                esp_set_phase(s, STAT_CD);
+                s->cmdfifo_cdb_offset = 1;
+
+                if (fifo8_num_used(&s->cmdfifo) > 1) {
+                    /* Process any additional command phase data */
+                    esp_do_dma(s);
+                }
+            }
+            break;
+        }
+
         /* ATN remains asserted until TC == 0 */
         if (esp_get_tc(s) == 0) {
             esp_set_phase(s, STAT_CD);
@@ -663,6 +662,21 @@ static void esp_do_dma(ESPState *s)
         esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
         esp_raise_drq(s);
 
+        switch (s->rregs[ESP_CMD]) {
+        case CMD_SELATN | CMD_DMA:
+            if (fifo8_num_used(&s->cmdfifo) >= 1) {
+                /* First byte received, switch to command phase */
+                esp_set_phase(s, STAT_CD);
+                s->cmdfifo_cdb_offset = 1;
+
+                if (fifo8_num_used(&s->cmdfifo) > 1) {
+                    /* Process any additional command phase data */
+                    esp_do_dma(s);
+                }
+            }
+            break;
+        }
+
         /* ATN remains asserted until TC == 0 */
         if (esp_get_tc(s) == 0) {
             esp_set_phase(s, STAT_CD);
@@ -890,9 +904,6 @@ static void esp_do_nodma(ESPState *s)
 static void esp_pdma_cb(ESPState *s)
 {
     switch (s->pdma_cb) {
-    case SATN_PDMA_CB:
-        satn_pdma_cb(s);
-        break;
     case SATN_STOP_PDMA_CB:
         satn_stop_pdma_cb(s);
         break;
diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
index b727181da9..9945645837 100644
--- a/include/hw/scsi/esp.h
+++ b/include/hw/scsi/esp.h
@@ -152,7 +152,6 @@ struct SysBusESPState {
 
 /* PDMA callbacks */
 enum pdma_cb {
-    SATN_PDMA_CB = 0,
     SATN_STOP_PDMA_CB = 2,
     WRITE_RESPONSE_PDMA_CB = 3,
     DO_DMA_PDMA_CB = 4
-- 
2.39.2

[PATCH 50/88] esp.c: move CMD_TI end of message phase detection to esp_do_dma() and do_dma_pdma_cb()

[Mark Cave-Ayland]

The existing check for TC == 0 is only valid during a TI command.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index f8c20d0584..9f787d12a8 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -555,14 +555,16 @@ static void do_dma_pdma_cb(ESPState *s)
                 }
             }
             break;
-        }
 
-        /* ATN remains asserted until TC == 0 */
-        if (esp_get_tc(s) == 0) {
-            esp_set_phase(s, STAT_CD);
-            s->rregs[ESP_RSEQ] = SEQ_CD;
-            s->rregs[ESP_RINTR] |= INTR_BS;
-            esp_raise_irq(s);
+        case CMD_TI | CMD_DMA:
+            /* ATN remains asserted until TC == 0 */
+            if (esp_get_tc(s) == 0) {
+                esp_set_phase(s, STAT_CD);
+                s->rregs[ESP_RSEQ] = SEQ_CD;
+                s->rregs[ESP_RINTR] |= INTR_BS;
+                esp_raise_irq(s);
+            }
+            break;
         }
         break;
 
@@ -675,14 +677,16 @@ static void esp_do_dma(ESPState *s)
                 }
             }
             break;
-        }
 
-        /* ATN remains asserted until TC == 0 */
-        if (esp_get_tc(s) == 0) {
-            esp_set_phase(s, STAT_CD);
-            s->rregs[ESP_RSEQ] = SEQ_CD;
-            s->rregs[ESP_RINTR] |= INTR_BS;
-            esp_raise_irq(s);
+        case CMD_TI | CMD_DMA:
+            /* ATN remains asserted until TC == 0 */
+            if (esp_get_tc(s) == 0) {
+                esp_set_phase(s, STAT_CD);
+                s->rregs[ESP_RSEQ] = SEQ_CD;
+                s->rregs[ESP_RINTR] |= INTR_BS;
+                esp_raise_irq(s);
+            }
+            break;
         }
         break;
 
-- 
2.39.2

[PATCH 51/88] esp.c: don't use get_cmd() for CMD_SEL DMA commands

[Mark Cave-Ayland]

This can now be done using the existing logic in esp_do_dma() and do_dma_pdma_cb().

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 9f787d12a8..3cf8b2b4eb 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -396,8 +396,6 @@ static void handle_satn(ESPState *s)
 
 static void handle_s_without_atn(ESPState *s)
 {
-    int32_t cmdlen;
-
     if (s->dma && !s->dma_enabled) {
         s->dma_cb = handle_s_without_atn;
         return;
@@ -406,17 +404,17 @@ static void handle_s_without_atn(ESPState *s)
     if (esp_select(s) < 0) {
         return;
     }
-    cmdlen = get_cmd(s, ESP_CMDFIFO_SZ);
-    if (cmdlen > 0) {
-        s->cmdfifo_cdb_offset = 0;
-        do_cmd(s);
-    } else if (cmdlen == 0) {
-        if (s->dma) {
-            esp_raise_drq(s);
+
+    esp_set_phase(s, STAT_CD);
+    s->rregs[ESP_RSEQ] = SEQ_CD;
+    s->cmdfifo_cdb_offset = 0;
+
+    if (s->dma) {
+        esp_do_dma(s);
+    } else {
+        if (get_cmd(s, ESP_CMDFIFO_SZ)) {
+            do_cmd(s);
         }
-        /* Target present, but no cmd yet - switch to command phase */
-        s->rregs[ESP_RSEQ] = SEQ_CD;
-        esp_set_phase(s, STAT_CD);
     }
 }
 
-- 
2.39.2

[PATCH 52/88] esp.c: move CMD_SELATNS end of command logic to esp_do_dma() and do_dma_pdma_cb()

[Mark Cave-Ayland]

The special logic in satn_stop_pdma_cb() is now no longer required since
esp_do_dma() can be used as a direct replacement.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 3cf8b2b4eb..29e3869173 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -418,54 +418,31 @@ static void handle_s_without_atn(ESPState *s)
     }
 }
 
-static void satn_stop_pdma_cb(ESPState *s)
-{
-    uint8_t buf[ESP_FIFO_SZ];
-    int n;
-
-    /* Copy FIFO into cmdfifo */
-    n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
-    n = MIN(fifo8_num_free(&s->cmdfifo), n);
-    fifo8_push_all(&s->cmdfifo, buf, n);
-
-    if (!esp_get_tc(s) && !fifo8_is_empty(&s->cmdfifo)) {
-        trace_esp_handle_satn_stop(fifo8_num_used(&s->cmdfifo));
-        s->cmdfifo_cdb_offset = 1;
-        esp_set_phase(s, STAT_CD);
-        s->rregs[ESP_RSTAT] |= STAT_TC;
-        s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
-        s->rregs[ESP_RSEQ] = SEQ_CD;
-        esp_raise_irq(s);
-    }
-}
-
 static void handle_satn_stop(ESPState *s)
 {
-    int32_t cmdlen;
-
     if (s->dma && !s->dma_enabled) {
         s->dma_cb = handle_satn_stop;
         return;
     }
-    esp_set_pdma_cb(s, SATN_STOP_PDMA_CB);
+    esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
     if (esp_select(s) < 0) {
         return;
     }
-    cmdlen = get_cmd(s, 1);
-    if (cmdlen > 0) {
-        trace_esp_handle_satn_stop(fifo8_num_used(&s->cmdfifo));
-        s->cmdfifo_cdb_offset = 1;
-        esp_set_phase(s, STAT_MO);
-        s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
-        s->rregs[ESP_RSEQ] = SEQ_MO;
-        esp_raise_irq(s);
-    } else if (cmdlen == 0) {
-        if (s->dma) {
-            esp_raise_drq(s);
+
+    esp_set_phase(s, STAT_MO);
+    s->rregs[ESP_RSEQ] = SEQ_MO;
+
+    if (s->dma) {
+        esp_do_dma(s);
+    } else {
+        if (get_cmd(s, 1)) {
+            trace_esp_handle_satn_stop(fifo8_num_used(&s->cmdfifo));
+
+            /* Raise command completion interrupt */
+            s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
+            s->rregs[ESP_RSEQ] = SEQ_MO;
+            esp_raise_irq(s);
         }
-        /* Target present, switch to message out phase */
-        s->rregs[ESP_RSEQ] = SEQ_MO;
-        esp_set_phase(s, STAT_MO);
     }
 }
 
@@ -554,6 +531,19 @@ static void do_dma_pdma_cb(ESPState *s)
             }
             break;
 
+        case CMD_SELATNS | CMD_DMA:
+            if (fifo8_num_used(&s->cmdfifo) == 1) {
+                /* First byte received, stop in message out phase */
+                esp_set_phase(s, STAT_CD);
+                s->cmdfifo_cdb_offset = 1;
+
+                /* Raise command completion interrupt */
+                s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
+                s->rregs[ESP_RSEQ] = SEQ_CD;
+                esp_raise_irq(s);
+            }
+            break;
+
         case CMD_TI | CMD_DMA:
             /* ATN remains asserted until TC == 0 */
             if (esp_get_tc(s) == 0) {
@@ -676,6 +666,19 @@ static void esp_do_dma(ESPState *s)
             }
             break;
 
+        case CMD_SELATNS | CMD_DMA:
+            if (fifo8_num_used(&s->cmdfifo) == 1) {
+                /* First byte received, stop in message out phase */
+                esp_set_phase(s, STAT_CD);
+                s->cmdfifo_cdb_offset = 1;
+
+                /* Raise command completion interrupt */
+                s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
+                s->rregs[ESP_RSEQ] = SEQ_CD;
+                esp_raise_irq(s);
+            }
+            break;
+
         case CMD_TI | CMD_DMA:
             /* ATN remains asserted until TC == 0 */
             if (esp_get_tc(s) == 0) {
@@ -906,9 +909,6 @@ static void esp_do_nodma(ESPState *s)
 static void esp_pdma_cb(ESPState *s)
 {
     switch (s->pdma_cb) {
-    case SATN_STOP_PDMA_CB:
-        satn_stop_pdma_cb(s);
-        break;
     case WRITE_RESPONSE_PDMA_CB:
         write_response_pdma_cb(s);
         break;
diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
index 9945645837..a4b2ed115c 100644
--- a/include/hw/scsi/esp.h
+++ b/include/hw/scsi/esp.h
@@ -152,7 +152,6 @@ struct SysBusESPState {
 
 /* PDMA callbacks */
 enum pdma_cb {
-    SATN_STOP_PDMA_CB = 2,
     WRITE_RESPONSE_PDMA_CB = 3,
     DO_DMA_PDMA_CB = 4
 };
-- 
2.39.2

[PATCH 53/88] esp.c: replace do_dma_pdma_cb() with esp_do_dma()

[Mark Cave-Ayland]

Now that the DMA logic is identical between do_dma_pdma_cb() and esp_do_dma()
we can replace do_dma_pdma_cb() with esp_do_dma().

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 29e3869173..f69b2709fc 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -491,141 +491,6 @@ static void esp_dma_ti_check(ESPState *s)
     }
 }
 
-static void do_dma_pdma_cb(ESPState *s)
-{
-    uint8_t buf[ESP_CMDFIFO_SZ];
-    int len;
-    uint32_t n, cmdlen;
-
-    len = esp_get_tc(s);
-
-    switch (esp_get_phase(s)) {
-    case STAT_MO:
-        if (s->dma_memory_read) {
-            len = MIN(len, fifo8_num_free(&s->cmdfifo));
-            s->dma_memory_read(s->dma_opaque, buf, len);
-            fifo8_push_all(&s->cmdfifo, buf, len);
-            esp_set_tc(s, esp_get_tc(s) - len);
-            s->cmdfifo_cdb_offset += len;
-        } else {
-            n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
-            n = MIN(fifo8_num_free(&s->cmdfifo), n);
-            fifo8_push_all(&s->cmdfifo, buf, n);
-            s->cmdfifo_cdb_offset += n;
-        }
-
-        esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
-        esp_raise_drq(s);
-
-        switch (s->rregs[ESP_CMD]) {
-        case CMD_SELATN | CMD_DMA:
-            if (fifo8_num_used(&s->cmdfifo) >= 1) {
-                /* First byte received, switch to command phase */
-                esp_set_phase(s, STAT_CD);
-                s->cmdfifo_cdb_offset = 1;
-
-                if (fifo8_num_used(&s->cmdfifo) > 1) {
-                    /* Process any additional command phase data */
-                    esp_do_dma(s);
-                }
-            }
-            break;
-
-        case CMD_SELATNS | CMD_DMA:
-            if (fifo8_num_used(&s->cmdfifo) == 1) {
-                /* First byte received, stop in message out phase */
-                esp_set_phase(s, STAT_CD);
-                s->cmdfifo_cdb_offset = 1;
-
-                /* Raise command completion interrupt */
-                s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
-                s->rregs[ESP_RSEQ] = SEQ_CD;
-                esp_raise_irq(s);
-            }
-            break;
-
-        case CMD_TI | CMD_DMA:
-            /* ATN remains asserted until TC == 0 */
-            if (esp_get_tc(s) == 0) {
-                esp_set_phase(s, STAT_CD);
-                s->rregs[ESP_RSEQ] = SEQ_CD;
-                s->rregs[ESP_RINTR] |= INTR_BS;
-                esp_raise_irq(s);
-            }
-            break;
-        }
-        break;
-
-    case STAT_CD:
-        cmdlen = fifo8_num_used(&s->cmdfifo);
-        trace_esp_do_dma(cmdlen, len);
-        if (s->dma_memory_read) {
-            len = MIN(len, fifo8_num_free(&s->cmdfifo));
-            s->dma_memory_read(s->dma_opaque, buf, len);
-            fifo8_push_all(&s->cmdfifo, buf, len);
-            esp_set_tc(s, esp_get_tc(s) - len);
-        } else {
-            n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
-            n = MIN(fifo8_num_free(&s->cmdfifo), n);
-            fifo8_push_all(&s->cmdfifo, buf, n);
-
-            esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
-            esp_raise_drq(s);
-        }
-        trace_esp_handle_ti_cmd(cmdlen);
-        s->ti_size = 0;
-        if (esp_get_tc(s) == 0) {
-            /* Command has been received */
-            do_cmd(s);
-        }
-        break;
-
-    case STAT_DO:
-        if (!s->current_req) {
-            return;
-        }
-        /* Copy FIFO data to device */
-        len = MIN(s->async_len, ESP_FIFO_SZ);
-        len = MIN(len, fifo8_num_used(&s->fifo));
-        n = esp_fifo_pop_buf(&s->fifo, s->async_buf, len);
-        s->async_buf += n;
-        s->async_len -= n;
-        s->ti_size += n;
-
-        if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
-            /* Defer until the scsi layer has completed */
-            scsi_req_continue(s->current_req);
-            return;
-        }
-
-        esp_dma_ti_check(s);
-        break;
-
-    case STAT_DI:
-        if (!s->current_req) {
-            return;
-        }
-        /* 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);
-
-        if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
-            /* Defer until the scsi layer has completed */
-            scsi_req_continue(s->current_req);
-            s->data_in_ready = false;
-            return;
-        }
-
-        esp_dma_ti_check(s);
-        break;
-    }
-}
-
 static void esp_do_dma(ESPState *s)
 {
     uint32_t len, cmdlen;
@@ -669,7 +534,6 @@ static void esp_do_dma(ESPState *s)
         case CMD_SELATNS | CMD_DMA:
             if (fifo8_num_used(&s->cmdfifo) == 1) {
                 /* First byte received, stop in message out phase */
-                esp_set_phase(s, STAT_CD);
                 s->cmdfifo_cdb_offset = 1;
 
                 /* Raise command completion interrupt */
@@ -913,7 +777,7 @@ static void esp_pdma_cb(ESPState *s)
         write_response_pdma_cb(s);
         break;
     case DO_DMA_PDMA_CB:
-        do_dma_pdma_cb(s);
+        esp_do_dma(s);
         break;
     default:
         g_assert_not_reached();
-- 
2.39.2

[PATCH 54/88] esp.c: move CMD_ICCS command logic to esp_do_dma()

[Mark Cave-Ayland]

The special logic in write_response_pdma_cb() is now no longer required since
esp_do_dma() can be used as a direct replacement.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index f69b2709fc..c6e5ddd537 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -446,40 +446,23 @@ static void handle_satn_stop(ESPState *s)
     }
 }
 
-static void write_response_pdma_cb(ESPState *s)
-{
-    esp_set_phase(s, STAT_ST);
-    s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
-    s->rregs[ESP_RSEQ] = SEQ_CD;
-    esp_raise_irq(s);
-}
-
 static void write_response(ESPState *s)
 {
     uint8_t buf[2];
 
     trace_esp_write_response(s->status);
 
-    buf[0] = s->status;
-    buf[1] = 0;
-
     if (s->dma) {
-        if (s->dma_memory_write) {
-            s->dma_memory_write(s->dma_opaque, buf, 2);
-            esp_set_phase(s, STAT_ST);
-            s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
-            s->rregs[ESP_RSEQ] = SEQ_CD;
-        } else {
-            esp_set_pdma_cb(s, WRITE_RESPONSE_PDMA_CB);
-            esp_raise_drq(s);
-            return;
-        }
+        esp_do_dma(s);
     } else {
+        buf[0] = s->status;
+        buf[1] = 0;
+
         fifo8_reset(&s->fifo);
         fifo8_push_all(&s->fifo, buf, 2);
         s->rregs[ESP_RFLAGS] = 2;
+        esp_raise_irq(s);
     }
-    esp_raise_irq(s);
 }
 
 static void esp_dma_ti_check(ESPState *s)
@@ -673,6 +656,58 @@ static void esp_do_dma(ESPState *s)
             esp_dma_ti_check(s);
         }
         break;
+
+    case STAT_ST:
+        switch (s->rregs[ESP_CMD]) {
+        case CMD_ICCS | CMD_DMA:
+            len = MIN(len, 1);
+
+            if (len) {
+                buf[0] = s->status;
+
+                if (s->dma_memory_write) {
+                    s->dma_memory_write(s->dma_opaque, buf, len);
+                    esp_set_tc(s, esp_get_tc(s) - len);
+                } else {
+                    fifo8_push_all(&s->fifo, buf, len);
+                    esp_set_tc(s, esp_get_tc(s) - len);
+                }
+
+                esp_set_phase(s, STAT_MI);
+
+                if (esp_get_tc(s) > 0) {
+                    /* Process any message in phase data */
+                    esp_do_dma(s);
+                }
+            }
+            break;
+        }
+        break;
+
+    case STAT_MI:
+        switch (s->rregs[ESP_CMD]) {
+        case CMD_ICCS | CMD_DMA:
+            len = MIN(len, 1);
+
+            if (len) {
+                buf[0] = 0;
+
+                if (s->dma_memory_write) {
+                    s->dma_memory_write(s->dma_opaque, buf, len);
+                    esp_set_tc(s, esp_get_tc(s) - len);
+                } else {
+                    fifo8_push_all(&s->fifo, buf, len);
+                    esp_set_tc(s, esp_get_tc(s) - len);
+                }
+
+                /* Raise end of command interrupt */
+                s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
+                s->rregs[ESP_RSEQ] = SEQ_CD;
+                esp_raise_irq(s);
+            }
+            break;
+        }
+        break;
     }
 }
 
@@ -773,9 +808,6 @@ static void esp_do_nodma(ESPState *s)
 static void esp_pdma_cb(ESPState *s)
 {
     switch (s->pdma_cb) {
-    case WRITE_RESPONSE_PDMA_CB:
-        write_response_pdma_cb(s);
-        break;
     case DO_DMA_PDMA_CB:
         esp_do_dma(s);
         break;
diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
index a4b2ed115c..0207fdd7a6 100644
--- a/include/hw/scsi/esp.h
+++ b/include/hw/scsi/esp.h
@@ -152,7 +152,6 @@ struct SysBusESPState {
 
 /* PDMA callbacks */
 enum pdma_cb {
-    WRITE_RESPONSE_PDMA_CB = 3,
     DO_DMA_PDMA_CB = 4
 };
 
-- 
2.39.2

[PATCH 55/88] esp.c: always use esp_do_dma() in pdma_cb()

[Mark Cave-Ayland]

There is now only a single implementation contained within esp_do_dma() so
call it directly.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index c6e5ddd537..bdbdb209f7 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -807,13 +807,7 @@ static void esp_do_nodma(ESPState *s)
 
 static void esp_pdma_cb(ESPState *s)
 {
-    switch (s->pdma_cb) {
-    case DO_DMA_PDMA_CB:
-        esp_do_dma(s);
-        break;
-    default:
-        g_assert_not_reached();
-    }
+    esp_do_dma(s);
 }
 
 void esp_command_complete(SCSIRequest *req, size_t resid)
-- 
2.39.2

[PATCH 56/88] esp.c: remove unused PDMA callback implementation

[Mark Cave-Ayland]

Note that this is a migration break for the q800 machine because the extra PDMA
information is no longer included.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c         | 55 ++++---------------------------------------
 include/hw/scsi/esp.h |  6 -----
 2 files changed, 5 insertions(+), 56 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index bdbdb209f7..5061c9d5a1 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -223,11 +223,6 @@ static void esp_pdma_write(ESPState *s, uint8_t val)
     esp_set_tc(s, dmalen);
 }
 
-static void esp_set_pdma_cb(ESPState *s, enum pdma_cb cb)
-{
-    s->pdma_cb = cb;
-}
-
 static int esp_select(ESPState *s)
 {
     int target;
@@ -377,7 +372,7 @@ static void handle_satn(ESPState *s)
         s->dma_cb = handle_satn;
         return;
     }
-    esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
+
     if (esp_select(s) < 0) {
         return;
     }
@@ -400,7 +395,7 @@ static void handle_s_without_atn(ESPState *s)
         s->dma_cb = handle_s_without_atn;
         return;
     }
-    esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
+
     if (esp_select(s) < 0) {
         return;
     }
@@ -424,7 +419,7 @@ static void handle_satn_stop(ESPState *s)
         s->dma_cb = handle_satn_stop;
         return;
     }
-    esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
+
     if (esp_select(s) < 0) {
         return;
     }
@@ -497,7 +492,6 @@ static void esp_do_dma(ESPState *s)
             s->cmdfifo_cdb_offset += n;
         }
 
-        esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
         esp_raise_drq(s);
 
         switch (s->rregs[ESP_CMD]) {
@@ -551,7 +545,6 @@ static void esp_do_dma(ESPState *s)
             n = MIN(fifo8_num_free(&s->cmdfifo), n);
             fifo8_push_all(&s->cmdfifo, buf, n);
 
-            esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
             esp_raise_drq(s);
         }
         trace_esp_handle_ti_cmd(cmdlen);
@@ -597,7 +590,6 @@ static void esp_do_dma(ESPState *s)
             s->async_len -= n;
             s->ti_size += n;
 
-            esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
             esp_raise_drq(s);
 
             if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
@@ -644,7 +636,6 @@ static void esp_do_dma(ESPState *s)
             s->async_len -= len;
             s->ti_size -= len;
             esp_set_tc(s, esp_get_tc(s) - len);
-            esp_set_pdma_cb(s, DO_DMA_PDMA_CB);
             esp_raise_drq(s);
 
             if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
@@ -805,11 +796,6 @@ static void esp_do_nodma(ESPState *s)
     }
 }
 
-static void esp_pdma_cb(ESPState *s)
-{
-    esp_do_dma(s);
-}
-
 void esp_command_complete(SCSIRequest *req, size_t resid)
 {
     ESPState *s = req->hba_private;
@@ -1229,33 +1215,6 @@ static int esp_post_load(void *opaque, int version_id)
     return 0;
 }
 
-/*
- * PDMA (or pseudo-DMA) is only used on the Macintosh and requires the
- * guest CPU to perform the transfers between the SCSI bus and memory
- * itself. This is indicated by the dma_memory_read and dma_memory_write
- * functions being NULL (in contrast to the ESP PCI device) whilst
- * dma_enabled is still set.
- */
-
-static bool esp_pdma_needed(void *opaque)
-{
-    ESPState *s = ESP(opaque);
-
-    return s->dma_memory_read == NULL && s->dma_memory_write == NULL &&
-           s->dma_enabled;
-}
-
-static const VMStateDescription vmstate_esp_pdma = {
-    .name = "esp/pdma",
-    .version_id = 0,
-    .minimum_version_id = 0,
-    .needed = esp_pdma_needed,
-    .fields = (const VMStateField[]) {
-        VMSTATE_UINT8(pdma_cb, ESPState),
-        VMSTATE_END_OF_LIST()
-    }
-};
-
 const VMStateDescription vmstate_esp = {
     .name = "esp",
     .version_id = 6,
@@ -1290,10 +1249,6 @@ const VMStateDescription vmstate_esp = {
         VMSTATE_UINT8_TEST(lun, ESPState, esp_is_version_6),
         VMSTATE_END_OF_LIST()
     },
-    .subsections = (const VMStateDescription * const []) {
-        &vmstate_esp_pdma,
-        NULL
-    }
 };
 
 static void sysbus_esp_mem_write(void *opaque, hwaddr addr,
@@ -1342,7 +1297,7 @@ static void sysbus_esp_pdma_write(void *opaque, hwaddr addr,
         esp_pdma_write(s, val);
         break;
     }
-    esp_pdma_cb(s);
+    esp_do_dma(s);
 }
 
 static uint64_t sysbus_esp_pdma_read(void *opaque, hwaddr addr,
@@ -1363,7 +1318,7 @@ static uint64_t sysbus_esp_pdma_read(void *opaque, hwaddr addr,
         val = (val << 8) | esp_pdma_read(s);
         break;
     }
-    esp_pdma_cb(s);
+    esp_do_dma(s);
     return val;
 }
 
diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
index 0207fdd7a6..6f942864a6 100644
--- a/include/hw/scsi/esp.h
+++ b/include/hw/scsi/esp.h
@@ -51,7 +51,6 @@ struct ESPState {
     ESPDMAMemoryReadWriteFunc dma_memory_write;
     void *dma_opaque;
     void (*dma_cb)(ESPState *s);
-    uint8_t pdma_cb;
 
     uint8_t mig_version_id;
 
@@ -150,11 +149,6 @@ struct SysBusESPState {
 #define TCHI_FAS100A 0x4
 #define TCHI_AM53C974 0x12
 
-/* PDMA callbacks */
-enum pdma_cb {
-    DO_DMA_PDMA_CB = 4
-};
-
 void esp_dma_enable(ESPState *s, int irq, int level);
 void esp_request_cancelled(SCSIRequest *req);
 void esp_command_complete(SCSIRequest *req, size_t resid);
-- 
2.39.2

[PATCH 57/88] esp.c: rename data_in_ready to to data_ready

[Mark Cave-Ayland]

This field is currently used to handle deferred interrupts for the DATA IN phase
but the code will soon be updated to do the same for the DATA OUT phase.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 5061c9d5a1..73c723afcc 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -328,7 +328,7 @@ static void do_command_phase(ESPState *s)
              * Switch to DATA IN phase but wait until initial data xfer is
              * complete before raising the command completion interrupt
              */
-            s->data_in_ready = false;
+            s->data_ready = false;
             esp_set_phase(s, STAT_DI);
         } else {
             esp_set_phase(s, STAT_DO);
@@ -859,12 +859,12 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
     s->async_len = len;
     s->async_buf = scsi_req_get_buf(req);
 
-    if (!to_device && !s->data_in_ready) {
+    if (!to_device && !s->data_ready) {
         /*
          * Initial incoming data xfer is complete so raise command
          * completion interrupt
          */
-        s->data_in_ready = true;
+        s->data_ready = true;
         s->rregs[ESP_RINTR] |= INTR_BS;
         esp_raise_irq(s);
     }
@@ -1241,7 +1241,7 @@ const VMStateDescription vmstate_esp = {
         VMSTATE_UINT32_TEST(mig_cmdlen, ESPState, esp_is_before_version_5),
         VMSTATE_UINT32(do_cmd, ESPState),
         VMSTATE_UINT32_TEST(mig_dma_left, ESPState, esp_is_before_version_5),
-        VMSTATE_BOOL_TEST(data_in_ready, ESPState, esp_is_version_5),
+        VMSTATE_BOOL_TEST(data_ready, ESPState, esp_is_version_5),
         VMSTATE_UINT8_TEST(cmdfifo_cdb_offset, ESPState, esp_is_version_5),
         VMSTATE_FIFO8_TEST(fifo, ESPState, esp_is_version_5),
         VMSTATE_FIFO8_TEST(cmdfifo, ESPState, esp_is_version_5),
diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
index 6f942864a6..1036606943 100644
--- a/include/hw/scsi/esp.h
+++ b/include/hw/scsi/esp.h
@@ -40,7 +40,7 @@ struct ESPState {
     uint8_t lun;
     uint32_t do_cmd;
 
-    bool data_in_ready;
+    bool data_ready;
     uint8_t ti_cmd;
     int dma_enabled;
 
-- 
2.39.2

[PATCH 58/88] esp.c: separate logic based upon ESP command in esp_command_complete()

[Mark Cave-Ayland]

The handling of the INTR_FC and INTR_BS bits is different depending upon the
last command executed by the ESP. Note that currently INTR_FC is managed
elsewhere, but that will change soon.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 73c723afcc..75538f5859 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -823,25 +823,27 @@ void esp_command_complete(SCSIRequest *req, size_t resid)
      * Switch to status phase. For non-DMA transfers from the target the last
      * byte is still in the FIFO
      */
-    esp_set_phase(s, STAT_ST);
-    if (s->ti_size == 0) {
-        /*
-         * Transfer complete: force TC to zero just in case a TI command was
-         * requested for more data than the command returns (Solaris 8 does
-         * this)
-         */
-        esp_set_tc(s, 0);
-        esp_dma_ti_check(s);
-    } else {
+    s->ti_size = 0;
+
+    switch (s->rregs[ESP_CMD]) {
+    case CMD_SEL | CMD_DMA:
+    case CMD_SEL:
+    case CMD_SELATN | CMD_DMA:
+    case CMD_SELATN:
         /*
-         * Transfer truncated: raise INTR_BS to indicate early change of
-         * phase
+         * No data phase for sequencer command so raise deferred bus service
+         * interrupt
          */
         s->rregs[ESP_RINTR] |= INTR_BS;
-        esp_raise_irq(s);
-        s->ti_size = 0;
+        break;
     }
 
+    /* Raise bus service interrupt to indicate change to STATUS phase */
+    esp_set_phase(s, STAT_ST);
+    s->rregs[ESP_RINTR] |= INTR_BS;
+    esp_raise_irq(s);
+    esp_lower_drq(s);
+
     if (s->current_req) {
         scsi_req_unref(s->current_req);
         s->current_req = NULL;
-- 
2.39.2

[PATCH 59/88] esp.c: separate logic based upon ESP command in esp_transfer_data()

[Mark Cave-Ayland]

The handling of the INTR_FC and INTR_BS bits is different depending upon the
last command executed by the ESP. Note that currently INTR_FC is managed
elsewhere, but that will change soon.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 75538f5859..b6cf1b43db 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -862,13 +862,33 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
     s->async_buf = scsi_req_get_buf(req);
 
     if (!to_device && !s->data_ready) {
-        /*
-         * Initial incoming data xfer is complete so raise command
-         * completion interrupt
-         */
         s->data_ready = true;
-        s->rregs[ESP_RINTR] |= INTR_BS;
-        esp_raise_irq(s);
+
+        switch (s->rregs[ESP_CMD]) {
+        case CMD_SEL | CMD_DMA:
+        case CMD_SEL:
+        case CMD_SELATN | CMD_DMA:
+        case CMD_SELATN:
+        case CMD_SELATNS | CMD_DMA:
+        case CMD_SELATNS:
+            /*
+             * Initial incoming data xfer is complete so raise command
+             * completion interrupt
+             */
+             s->rregs[ESP_RINTR] |= INTR_BS;
+             esp_raise_irq(s);
+             break;
+
+        case CMD_TI | CMD_DMA:
+        case CMD_TI:
+            /*
+             * Bus service interrupt raised because of initial change to
+             * DATA phase
+             */
+            s->rregs[ESP_RINTR] |= INTR_BS;
+            esp_raise_irq(s);
+            break;
+        }
     }
 
     /*
-- 
2.39.2

[PATCH 60/88] esp.c: use deferred interrupts for both DATA IN and DATA OUT phases

[Mark Cave-Ayland]

This brings DATA OUT transfers in line with DATA IN transfers by ensuring that
the guest visible function complete interrupt is only set once the SCSI layer
has returned.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index b6cf1b43db..d71465718c 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -248,10 +248,8 @@ static int esp_select(ESPState *s)
 
     /*
      * Note that we deliberately don't raise the IRQ here: this will be done
-     * either in do_command_phase() for DATA OUT transfers or by the deferred
-     * IRQ mechanism in esp_transfer_data() for DATA IN transfers
+     * either in esp_transfer_data() or esp_command_complete()
      */
-    s->rregs[ESP_RINTR] |= INTR_FC;
     s->rregs[ESP_RSEQ] = SEQ_CD;
     return 0;
 }
@@ -321,20 +319,17 @@ static void do_command_phase(ESPState *s)
     datalen = scsi_req_enqueue(s->current_req);
     s->ti_size = datalen;
     fifo8_reset(&s->cmdfifo);
+    s->data_ready = false;
     if (datalen != 0) {
         s->ti_cmd = 0;
+        /*
+         * Switch to DATA phase but wait until initial data xfer is
+         * complete before raising the command completion interrupt
+         */
         if (datalen > 0) {
-            /*
-             * Switch to DATA IN phase but wait until initial data xfer is
-             * complete before raising the command completion interrupt
-             */
-            s->data_ready = false;
             esp_set_phase(s, STAT_DI);
         } else {
             esp_set_phase(s, STAT_DO);
-            s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
-            esp_raise_irq(s);
-            esp_lower_drq(s);
         }
         scsi_req_continue(s->current_req);
         return;
@@ -832,9 +827,9 @@ void esp_command_complete(SCSIRequest *req, size_t resid)
     case CMD_SELATN:
         /*
          * No data phase for sequencer command so raise deferred bus service
-         * interrupt
+         * and function complete interrupt
          */
-        s->rregs[ESP_RINTR] |= INTR_BS;
+        s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
         break;
     }
 
@@ -854,14 +849,13 @@ void esp_command_complete(SCSIRequest *req, size_t resid)
 void esp_transfer_data(SCSIRequest *req, uint32_t len)
 {
     ESPState *s = req->hba_private;
-    int to_device = (esp_get_phase(s) == STAT_DO);
     uint32_t dmalen = esp_get_tc(s);
 
     trace_esp_transfer_data(dmalen, s->ti_size);
     s->async_len = len;
     s->async_buf = scsi_req_get_buf(req);
 
-    if (!to_device && !s->data_ready) {
+    if (!s->data_ready) {
         s->data_ready = true;
 
         switch (s->rregs[ESP_CMD]) {
@@ -869,6 +863,13 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
         case CMD_SEL:
         case CMD_SELATN | CMD_DMA:
         case CMD_SELATN:
+            /*
+             * Initial incoming data xfer is complete for sequencer command
+             * so raise deferred bus service and function complete interrupt
+             */
+             s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
+             break;
+
         case CMD_SELATNS | CMD_DMA:
         case CMD_SELATNS:
             /*
@@ -876,7 +877,6 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
              * completion interrupt
              */
              s->rregs[ESP_RINTR] |= INTR_BS;
-             esp_raise_irq(s);
              break;
 
         case CMD_TI | CMD_DMA:
@@ -886,9 +886,10 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
              * DATA phase
              */
             s->rregs[ESP_RINTR] |= INTR_BS;
-            esp_raise_irq(s);
             break;
         }
+
+        esp_raise_irq(s);
     }
 
     /*
-- 
2.39.2

[PATCH 61/88] esp.c: remove DATA IN phase logic when reading from FIFO

[Mark Cave-Ayland]

Whilst the FIFO is used a storage buffer for both DMA and non-DMA requests, the
loading and unloading is managed directly issuing commands to the ESP. As a
result there is no need to manually invoke the non-DMA command handler.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index d71465718c..4c1ca63a57 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -1067,17 +1067,6 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr)
             qemu_log_mask(LOG_UNIMP, "esp: PIO data read not implemented\n");
             s->rregs[ESP_FIFO] = 0;
         } else {
-            if (esp_get_phase(s) == STAT_DI) {
-                if (s->ti_size) {
-                    esp_do_nodma(s);
-                } else {
-                    /*
-                     * The last byte of a non-DMA transfer has been read out
-                     * of the FIFO so switch to status phase
-                     */
-                    esp_set_phase(s, STAT_ST);
-                }
-            }
             s->rregs[ESP_FIFO] = esp_fifo_pop(&s->fifo);
         }
         val = s->rregs[ESP_FIFO];
-- 
2.39.2

[PATCH 62/88] esp.c: zero command register when TI command terminates due to phase change

[Mark Cave-Ayland]

This is the behaviour documented in the datasheet and allows the state machine
to correctly process multiple consecutive TI commands.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 4c1ca63a57..ccb8ad4bae 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -519,6 +519,7 @@ static void esp_do_dma(ESPState *s)
             /* ATN remains asserted until TC == 0 */
             if (esp_get_tc(s) == 0) {
                 esp_set_phase(s, STAT_CD);
+                s->rregs[ESP_CMD] = 0;
                 s->rregs[ESP_RSEQ] = SEQ_CD;
                 s->rregs[ESP_RINTR] |= INTR_BS;
                 esp_raise_irq(s);
@@ -717,6 +718,7 @@ static void esp_do_nodma(ESPState *s)
          */
         s->cmdfifo_cdb_offset = fifo8_num_used(&s->cmdfifo);
         esp_set_phase(s, STAT_CD);
+        s->rregs[ESP_CMD] = 0;
         s->rregs[ESP_RSEQ] = SEQ_CD;
         s->rregs[ESP_RINTR] |= INTR_BS;
         esp_raise_irq(s);
@@ -831,6 +833,11 @@ void esp_command_complete(SCSIRequest *req, size_t resid)
          */
         s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
         break;
+
+    case CMD_TI | CMD_DMA:
+    case CMD_TI:
+        s->rregs[ESP_CMD] = 0;
+        break;
     }
 
     /* Raise bus service interrupt to indicate change to STATUS phase */
@@ -885,6 +892,7 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
              * Bus service interrupt raised because of initial change to
              * DATA phase
              */
+            s->rregs[ESP_CMD] = 0;
             s->rregs[ESP_RINTR] |= INTR_BS;
             break;
         }
-- 
2.39.2

[PATCH 63/88] esp.c: remove unneeded ti_cmd field

[Mark Cave-Ayland]

According to the datasheet the previous ESP command remains in the ESP_CMD
register, which caused a problem when consecutive TI commands were issued as
it becomes impossible for the state machine to know when the first TI
command finishes.

This was the original reason for introducing the ti_cmd field which kept
track of the last written command for this purpose. However closer reading
of the datasheet shows that a TI command that terminates due to a change of
SCSI target phase resets the ESP_CMD register to zero which solves this
problem.

Now that this has been fixed in the previous commit, remove the unneeded
ti_cmd field and access the ESP_CMD register directly instead. Bump the
vmstate_esp version to indicate that the ti_cmd field is no longer included.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c         | 19 +++++++++++++------
 include/hw/scsi/esp.h |  3 ++-
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index ccb8ad4bae..bcebd00831 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -321,7 +321,6 @@ static void do_command_phase(ESPState *s)
     fifo8_reset(&s->cmdfifo);
     s->data_ready = false;
     if (datalen != 0) {
-        s->ti_cmd = 0;
         /*
          * Switch to DATA phase but wait until initial data xfer is
          * complete before raising the command completion interrupt
@@ -908,12 +907,12 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
      * async data transfer is delayed then s->dma is set incorrectly.
      */
 
-    if (s->ti_cmd == (CMD_TI | CMD_DMA)) {
+    if (s->rregs[ESP_CMD] == (CMD_TI | CMD_DMA)) {
         /* When the SCSI layer returns more data, raise deferred INTR_BS */
         esp_dma_ti_check(s);
 
         esp_do_dma(s);
-    } else if (s->ti_cmd == CMD_TI) {
+    } else if (s->rregs[ESP_CMD] == CMD_TI) {
         esp_do_nodma(s);
     }
 }
@@ -927,7 +926,6 @@ static void handle_ti(ESPState *s)
         return;
     }
 
-    s->ti_cmd = s->rregs[ESP_CMD];
     if (s->dma) {
         dmalen = esp_get_tc(s);
         trace_esp_handle_ti(dmalen);
@@ -1200,6 +1198,14 @@ static bool esp_is_version_6(void *opaque, int version_id)
     return version_id >= 6;
 }
 
+static bool esp_is_between_version_5_and_6(void *opaque, int version_id)
+{
+    ESPState *s = ESP(opaque);
+
+    version_id = MIN(version_id, s->mig_version_id);
+    return version_id >= 5 && version_id <= 6;
+}
+
 int esp_pre_save(void *opaque)
 {
     ESPState *s = ESP(object_resolve_path_component(
@@ -1237,7 +1243,7 @@ static int esp_post_load(void *opaque, int version_id)
 
 const VMStateDescription vmstate_esp = {
     .name = "esp",
-    .version_id = 6,
+    .version_id = 7,
     .minimum_version_id = 3,
     .post_load = esp_post_load,
     .fields = (const VMStateField[]) {
@@ -1265,7 +1271,8 @@ const VMStateDescription vmstate_esp = {
         VMSTATE_UINT8_TEST(cmdfifo_cdb_offset, ESPState, esp_is_version_5),
         VMSTATE_FIFO8_TEST(fifo, ESPState, esp_is_version_5),
         VMSTATE_FIFO8_TEST(cmdfifo, ESPState, esp_is_version_5),
-        VMSTATE_UINT8_TEST(ti_cmd, ESPState, esp_is_version_5),
+        VMSTATE_UINT8_TEST(mig_ti_cmd, ESPState,
+                           esp_is_between_version_5_and_6),
         VMSTATE_UINT8_TEST(lun, ESPState, esp_is_version_6),
         VMSTATE_END_OF_LIST()
     },
diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
index 1036606943..39b416f538 100644
--- a/include/hw/scsi/esp.h
+++ b/include/hw/scsi/esp.h
@@ -41,7 +41,6 @@ struct ESPState {
     uint32_t do_cmd;
 
     bool data_ready;
-    uint8_t ti_cmd;
     int dma_enabled;
 
     uint32_t async_len;
@@ -62,6 +61,8 @@ struct ESPState {
     uint8_t mig_ti_buf[ESP_FIFO_SZ];
     uint8_t mig_cmdbuf[ESP_CMDFIFO_SZ];
     uint32_t mig_cmdlen;
+
+    uint8_t mig_ti_cmd;
 };
 
 #define TYPE_SYSBUS_ESP "sysbus-esp"
-- 
2.39.2

[PATCH 64/88] esp.c: don't raise INTR_BS interrupt in DATA IN phase until TI command issued

[Mark Cave-Ayland]

In the case where a SCSI command with a DATA IN phase has been issued, the host
may preload the FIFO with unaligned bytes before issuing the main DMA transfer.

When accumulating data in the FIFO don't raise the INTR_BS interrupt until the
TI command is issued, otherwise the unexpected interrupt can confuse the host.
In particular this is needed to prevent the MacOS Disk Utility from failing
when switching non-DMA transfers to use esp_do_nodma().

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index bcebd00831..dd6bf6f033 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -786,6 +786,11 @@ static void esp_do_nodma(ESPState *s)
             return;
         }
 
+        /* If preloading the FIFO, defer until TI command issued */
+        if (s->rregs[ESP_CMD] != CMD_TI) {
+            return;
+        }
+
         s->rregs[ESP_RINTR] |= INTR_BS;
         esp_raise_irq(s);
         break;
-- 
2.39.2

[PATCH 65/88] esp.c: move non-DMA TI logic to separate esp_nodma_ti_dataout() function

[Mark Cave-Ayland]

This is to allow the logic to be moved during the next commit.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index dd6bf6f033..97e48e9526 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -697,11 +697,38 @@ static void esp_do_dma(ESPState *s)
     }
 }
 
+static void esp_nodma_ti_dataout(ESPState *s)
+{
+    int len;
+
+    if (!s->current_req) {
+        return;
+    }
+    if (s->async_len == 0) {
+        /* Defer until data is available.  */
+        return;
+    }
+    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);
+    s->async_buf += len;
+    s->async_len -= len;
+    s->ti_size += len;
+
+    if (s->async_len == 0) {
+        scsi_req_continue(s->current_req);
+        return;
+    }
+
+    s->rregs[ESP_RINTR] |= INTR_BS;
+    esp_raise_irq(s);
+}
+
 static void esp_do_nodma(ESPState *s)
 {
     uint8_t buf[ESP_FIFO_SZ];
     uint32_t cmdlen;
-    int len, n;
+    int n;
 
     switch (esp_get_phase(s)) {
     case STAT_MO:
@@ -743,27 +770,7 @@ static void esp_do_nodma(ESPState *s)
         break;
 
     case STAT_DO:
-        if (!s->current_req) {
-            return;
-        }
-        if (s->async_len == 0) {
-            /* Defer until data is available.  */
-            return;
-        }
-        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);
-        s->async_buf += len;
-        s->async_len -= len;
-        s->ti_size += len;
-
-        if (s->async_len == 0) {
-            scsi_req_continue(s->current_req);
-            return;
-        }
-
-        s->rregs[ESP_RINTR] |= INTR_BS;
-        esp_raise_irq(s);
+        esp_nodma_ti_dataout(s);
         break;
 
     case STAT_DI:
-- 
2.39.2

[PATCH 66/88] esp.c: process non-DMA FIFO writes in esp_do_nodma()

[Mark Cave-Ayland]

Currently any write to the ESP FIFO in the MESSAGE OUT or COMMAND phases will
manually raise the bus service interrupt. Instead of duplicating the interrupt
logic in esp_reg_write(), update esp_do_nodma() to correctly process incoming
FIFO data during the MESSAGE OUT and COMMAND phases. Part of this change is to
call esp_nodma_ti_dataout() from handle_ti() to ensure that the DATA OUT phase
FIFO transfer only occurs when executing a non-DMA TI command instead of for
each byte entering the FIFO.

One slight complication is that NextSTEP uses multiple TI commands to transfer
the CDB one byte at a time (as opposed to loading the FIFO and using a single
TI command), so it is necessary to determine the expected length of the SCSI
CDB being received. This is handled by the introduction of a new
esp_cdb_length() function which returns the expected SCSI CDB length based
upon the first command byte.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 97e48e9526..5bb8cc4ea7 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -420,6 +420,7 @@ static void handle_satn_stop(ESPState *s)
 
     esp_set_phase(s, STAT_MO);
     s->rregs[ESP_RSEQ] = SEQ_MO;
+    s->cmdfifo_cdb_offset = 0;
 
     if (s->dma) {
         esp_do_dma(s);
@@ -454,6 +455,22 @@ static void write_response(ESPState *s)
     }
 }
 
+static int esp_cdb_length(ESPState *s)
+{
+    const uint8_t *pbuf;
+    int cmdlen, len;
+
+    cmdlen = fifo8_num_used(&s->cmdfifo);
+    if (cmdlen < s->cmdfifo_cdb_offset) {
+        return 0;
+    }
+
+    pbuf = fifo8_peek_buf(&s->cmdfifo, cmdlen, NULL);
+    len = scsi_cdb_length((uint8_t *)&pbuf[s->cmdfifo_cdb_offset]);
+
+    return len;
+}
+
 static void esp_dma_ti_check(ESPState *s)
 {
     if (esp_get_tc(s) == 0 && fifo8_num_used(&s->fifo) < 2) {
@@ -738,16 +755,40 @@ static void esp_do_nodma(ESPState *s)
         fifo8_push_all(&s->cmdfifo, buf, n);
         s->cmdfifo_cdb_offset += n;
 
-        /*
-         * Extra message out bytes received: update cmdfifo_cdb_offset
-         * and then switch to command phase
-         */
-        s->cmdfifo_cdb_offset = fifo8_num_used(&s->cmdfifo);
-        esp_set_phase(s, STAT_CD);
-        s->rregs[ESP_CMD] = 0;
-        s->rregs[ESP_RSEQ] = SEQ_CD;
-        s->rregs[ESP_RINTR] |= INTR_BS;
-        esp_raise_irq(s);
+        switch (s->rregs[ESP_CMD]) {
+        case CMD_SELATN:
+            if (fifo8_num_used(&s->cmdfifo) >= 1) {
+                /* First byte received, switch to command phase */
+                esp_set_phase(s, STAT_CD);
+                s->cmdfifo_cdb_offset = 1;
+
+                if (fifo8_num_used(&s->cmdfifo) > 1) {
+                    /* Process any additional command phase data */
+                    esp_do_nodma(s);
+                }
+            }
+            break;
+
+        case CMD_SELATNS:
+            if (fifo8_num_used(&s->cmdfifo) == 1) {
+                /* First byte received, stop in message out phase */
+                s->cmdfifo_cdb_offset = 1;
+
+                /* Raise command completion interrupt */
+                s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
+                esp_raise_irq(s);
+            }
+            break;
+
+        case CMD_TI:
+            /* ATN remains asserted until FIFO empty */
+            s->cmdfifo_cdb_offset = fifo8_num_used(&s->cmdfifo);
+            esp_set_phase(s, STAT_CD);
+            s->rregs[ESP_CMD] = 0;
+            s->rregs[ESP_RINTR] |= INTR_BS;
+            esp_raise_irq(s);
+            break;
+        }
         break;
 
     case STAT_CD:
@@ -756,21 +797,40 @@ static void esp_do_nodma(ESPState *s)
         n = MIN(fifo8_num_free(&s->cmdfifo), n);
         fifo8_push_all(&s->cmdfifo, buf, n);
 
-        cmdlen = fifo8_num_used(&s->cmdfifo);
-        trace_esp_handle_ti_cmd(cmdlen);
-        s->ti_size = 0;
+        switch (s->rregs[ESP_CMD]) {
+        case CMD_TI:
+            cmdlen = fifo8_num_used(&s->cmdfifo);
+            trace_esp_handle_ti_cmd(cmdlen);
+
+            /* CDB may be transferred in one or more TI commands */
+            if (esp_cdb_length(s) && esp_cdb_length(s) ==
+                fifo8_num_used(&s->cmdfifo) - s->cmdfifo_cdb_offset) {
+                    /* Command has been received */
+                    do_cmd(s);
+            } else {
+                /*
+                 * If data was transferred from the FIFO then raise bus
+                 * service interrupt to indicate transfer complete. Otherwise
+                 * defer until the next FIFO write.
+                 */
+                if (n) {
+                    /* Raise interrupt to indicate transfer complete */
+                    s->rregs[ESP_RINTR] |= INTR_BS;
+                    esp_raise_irq(s);
+                }
+            }
+            break;
 
-        /* No command received */
-        if (s->cmdfifo_cdb_offset == fifo8_num_used(&s->cmdfifo)) {
-            return;
+        case CMD_SEL:
+        case CMD_SELATN:
+            /* FIFO already contain entire CDB */
+            do_cmd(s);
+            break;
         }
-
-        /* Command has been received */
-        do_cmd(s);
         break;
 
     case STAT_DO:
-        esp_nodma_ti_dataout(s);
+        /* Accumulate data in FIFO until non-DMA TI is executed */
         break;
 
     case STAT_DI:
@@ -945,6 +1005,10 @@ static void handle_ti(ESPState *s)
     } else {
         trace_esp_handle_ti(s->ti_size);
         esp_do_nodma(s);
+
+        if (esp_get_phase(s) == STAT_DO) {
+            esp_nodma_ti_dataout(s);
+        }
     }
 }
 
@@ -1141,23 +1205,10 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val)
         s->rregs[ESP_RSTAT] &= ~STAT_TC;
         break;
     case ESP_FIFO:
-        if (esp_get_phase(s) == STAT_MO || esp_get_phase(s) == STAT_CD) {
-            if (!fifo8_is_full(&s->fifo)) {
-                esp_fifo_push(&s->fifo, val);
-                esp_fifo_push(&s->cmdfifo, fifo8_pop(&s->fifo));
-            }
-
-            /*
-             * If any unexpected message out/command phase data is
-             * transferred using non-DMA, raise the interrupt
-             */
-            if (s->rregs[ESP_CMD] == CMD_TI) {
-                s->rregs[ESP_RINTR] |= INTR_BS;
-                esp_raise_irq(s);
-            }
-        } else {
+        if (!fifo8_is_full(&s->fifo)) {
             esp_fifo_push(&s->fifo, val);
         }
+        esp_do_nodma(s);
         break;
     case ESP_CMD:
         s->rregs[saddr] = val;
-- 
2.39.2

[PATCH 67/88] esp.c: replace get_cmd() with esp_do_nodma()

[Mark Cave-Ayland]

Now that the esp_do_nodma() state machine correctly handles incoming FIFO
data, all remaining users of get_cmd() can be replaced with esp_do_nodma()
and the get_cmd() function removed completely.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 5bb8cc4ea7..277eb8647b 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -257,40 +257,6 @@ static int esp_select(ESPState *s)
 static void esp_do_dma(ESPState *s);
 static void esp_do_nodma(ESPState *s);
 
-static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
-{
-    uint8_t buf[ESP_CMDFIFO_SZ];
-    uint32_t dmalen, n;
-    int target;
-
-    target = s->wregs[ESP_WBUSID] & BUSID_DID;
-    if (s->dma) {
-        dmalen = MIN(esp_get_tc(s), maxlen);
-        if (dmalen == 0) {
-            return 0;
-        }
-        if (s->dma_memory_read) {
-            s->dma_memory_read(s->dma_opaque, buf, dmalen);
-            dmalen = MIN(fifo8_num_free(&s->cmdfifo), dmalen);
-            fifo8_push_all(&s->cmdfifo, buf, dmalen);
-            esp_set_tc(s, esp_get_tc(s) - dmalen);
-        } else {
-            return 0;
-        }
-    } else {
-        dmalen = MIN(fifo8_num_used(&s->fifo), maxlen);
-        if (dmalen == 0) {
-            return 0;
-        }
-        n = esp_fifo_pop_buf(&s->fifo, buf, dmalen);
-        n = MIN(fifo8_num_free(&s->cmdfifo), n);
-        fifo8_push_all(&s->cmdfifo, buf, n);
-    }
-    trace_esp_get_cmd(dmalen, target);
-
-    return dmalen;
-}
-
 static void do_command_phase(ESPState *s)
 {
     uint32_t cmdlen;
@@ -376,10 +342,7 @@ static void handle_satn(ESPState *s)
     if (s->dma) {
         esp_do_dma(s);
     } else {
-        if (get_cmd(s, ESP_CMDFIFO_SZ)) {
-            s->cmdfifo_cdb_offset = 1;
-            do_cmd(s);
-        }
+        esp_do_nodma(s);
     }
 }
 
@@ -401,9 +364,7 @@ static void handle_s_without_atn(ESPState *s)
     if (s->dma) {
         esp_do_dma(s);
     } else {
-        if (get_cmd(s, ESP_CMDFIFO_SZ)) {
-            do_cmd(s);
-        }
+        esp_do_nodma(s);
     }
 }
 
@@ -425,14 +386,7 @@ static void handle_satn_stop(ESPState *s)
     if (s->dma) {
         esp_do_dma(s);
     } else {
-        if (get_cmd(s, 1)) {
-            trace_esp_handle_satn_stop(fifo8_num_used(&s->cmdfifo));
-
-            /* Raise command completion interrupt */
-            s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
-            s->rregs[ESP_RSEQ] = SEQ_MO;
-            esp_raise_irq(s);
-        }
+        esp_do_nodma(s);
     }
 }
 
@@ -770,7 +724,7 @@ static void esp_do_nodma(ESPState *s)
             break;
 
         case CMD_SELATNS:
-            if (fifo8_num_used(&s->cmdfifo) == 1) {
+            if (fifo8_num_used(&s->cmdfifo) >= 1) {
                 /* First byte received, stop in message out phase */
                 s->cmdfifo_cdb_offset = 1;
 
-- 
2.39.2

[PATCH 68/88] esp.c: move write_response() non-DMA logic to esp_do_nodma()

[Mark Cave-Ayland]

This moves the remaining non-DMA STATUS and MESSAGE IN phase logic from
write_response() to esp_do_nodma(). Note that we can also now drop the extra
fifo_reset() which is no longer required.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 277eb8647b..824ebe9ff0 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -392,20 +392,12 @@ static void handle_satn_stop(ESPState *s)
 
 static void write_response(ESPState *s)
 {
-    uint8_t buf[2];
-
     trace_esp_write_response(s->status);
 
     if (s->dma) {
         esp_do_dma(s);
     } else {
-        buf[0] = s->status;
-        buf[1] = 0;
-
-        fifo8_reset(&s->fifo);
-        fifo8_push_all(&s->fifo, buf, 2);
-        s->rregs[ESP_RFLAGS] = 2;
-        esp_raise_irq(s);
+        esp_do_nodma(s);
     }
 }
 
@@ -815,6 +807,28 @@ static void esp_do_nodma(ESPState *s)
         s->rregs[ESP_RINTR] |= INTR_BS;
         esp_raise_irq(s);
         break;
+
+    case STAT_ST:
+        switch (s->rregs[ESP_CMD]) {
+        case CMD_ICCS:
+            fifo8_push(&s->fifo, s->status);
+            esp_set_phase(s, STAT_MI);
+
+            /* Process any message in phase data */
+            esp_do_nodma(s);
+            break;
+        }
+        break;
+
+    case STAT_MI:
+        switch (s->rregs[ESP_CMD]) {
+        case CMD_ICCS:
+            fifo8_push(&s->fifo, 0);
+
+            esp_raise_irq(s);
+            break;
+        }
+        break;
     }
 }
 
-- 
2.39.2

[PATCH 69/88] esp.c: consolidate end of command sequence after ICCS command

[Mark Cave-Ayland]

The end of command sequences for the ICCS command are currently different
between the DMA and non-DMA versions, and also different from the description
in the datasheet.

Update the sequence so that only INTR_FC is asserted in both cases, and keep
all the logic in esp_do_dma() and esp_do_nodma() rather than having some of
it within esp_run_cmd().

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 824ebe9ff0..6c62417985 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -650,8 +650,7 @@ static void esp_do_dma(ESPState *s)
                 }
 
                 /* Raise end of command interrupt */
-                s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
-                s->rregs[ESP_RSEQ] = SEQ_CD;
+                s->rregs[ESP_RINTR] |= INTR_FC;
                 esp_raise_irq(s);
             }
             break;
@@ -825,6 +824,8 @@ static void esp_do_nodma(ESPState *s)
         case CMD_ICCS:
             fifo8_push(&s->fifo, 0);
 
+            /* Raise end of command interrupt */
+            s->rregs[ESP_RINTR] |= INTR_FC;
             esp_raise_irq(s);
             break;
         }
@@ -1056,8 +1057,6 @@ static void esp_run_cmd(ESPState *s)
     case CMD_ICCS:
         trace_esp_mem_writeb_cmd_iccs(cmd);
         write_response(s);
-        s->rregs[ESP_RINTR] |= INTR_FC;
-        esp_set_phase(s, STAT_MI);
         break;
     case CMD_MSGACC:
         trace_esp_mem_writeb_cmd_msgacc(cmd);
-- 
2.39.2

[PATCH 70/88] esp.c: ensure that STAT_INT is cleared when reading ESP_RINTR

[Mark Cave-Ayland]

Both esp_raise_irq() and esp_lower_irq() check the STAT_INT bit in ESP_RSTAT
to ensure that the IRQ is raised or lowered if its state changes. When reading
ESP_RINTR, esp_lower_irq() was being called *after* ESP_RSTAT had been
cleared meaning that STAT_INT was already clear, and so if STAT_INT was
asserted beforehand then the esp_lower_irq() would have no effect.

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 6c62417985..604fb9235d 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -1127,6 +1127,7 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr)
          */
         val = s->rregs[ESP_RINTR];
         s->rregs[ESP_RINTR] = 0;
+        esp_lower_irq(s);
         s->rregs[ESP_RSTAT] &= ~STAT_TC;
         /*
          * According to the datasheet ESP_RSEQ should be cleared, but as the
@@ -1137,7 +1138,6 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr)
          *
          * s->rregs[ESP_RSEQ] = SEQ_0;
          */
-        esp_lower_irq(s);
         break;
     case ESP_TCHI:
         /* Return the unique id if the value has never been written */
-- 
2.39.2

[PATCH 71/88] esp.c: don't clear the SCSI phase when reading ESP_RINTR

[Mark Cave-Ayland]

According to the documentation ESP_RSTAT is cleared (except the STAT_TC bit)
when ESP_RINTR is read. This should not include the SCSI bus phase bits which
are currently live from the SCSI bus, otherwise the current SCSI phase is lost
when clearing an end-of-transfer interrupt.

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 604fb9235d..8ea100ee9c 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -1128,7 +1128,7 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr)
         val = s->rregs[ESP_RINTR];
         s->rregs[ESP_RINTR] = 0;
         esp_lower_irq(s);
-        s->rregs[ESP_RSTAT] &= ~STAT_TC;
+        s->rregs[ESP_RSTAT] &= STAT_TC | 7;
         /*
          * According to the datasheet ESP_RSEQ should be cleared, but as the
          * emulation currently defers information transfers to the next TI
-- 
2.39.2

[PATCH 72/88] esp.c: handle TC underflow for DMA SCSI requests

[Mark Cave-Ayland]

Detect the case where the guest underflows TC by requesting a DMA transfer which
is larger than the available data. If this case is detected, immediately
complete the SCSI request and handle any remaining FIFO accesses in the STATUS
phase by raising INTR_BS once the FIFO is below the threshold.

Note that handling the premature SCSI bus phase change in the case of TC
underflow fixes booting EMILE on m68k once again.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 8ea100ee9c..a3e18bb3d7 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -579,6 +579,12 @@ static void esp_do_dma(ESPState *s)
             s->async_len -= len;
             s->ti_size -= len;
 
+            if (s->async_len == 0 && s->ti_size == 0 && esp_get_tc(s)) {
+                /* If the guest underflows TC then terminate SCSI request */
+                scsi_req_continue(s->current_req);
+                return;
+            }
+
             if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
                 /* Defer until the scsi layer has completed */
                 scsi_req_continue(s->current_req);
@@ -596,6 +602,12 @@ static void esp_do_dma(ESPState *s)
             esp_set_tc(s, esp_get_tc(s) - len);
             esp_raise_drq(s);
 
+            if (s->async_len == 0 && s->ti_size == 0 && esp_get_tc(s)) {
+                /* If the guest underflows TC then terminate SCSI request */
+                scsi_req_continue(s->current_req);
+                return;
+            }
+
             if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
                 /* Defer until the scsi layer has completed */
                 scsi_req_continue(s->current_req);
@@ -630,6 +642,15 @@ static void esp_do_dma(ESPState *s)
                 }
             }
             break;
+
+        default:
+            /* Consume remaining data if the guest underflows TC */
+            if (fifo8_num_used(&s->fifo) < 2) {
+                s->rregs[ESP_RINTR] |= INTR_BS;
+                esp_raise_irq(s);
+                esp_lower_drq(s);
+            }
+            break;
         }
         break;
 
@@ -884,7 +905,9 @@ void esp_command_complete(SCSIRequest *req, size_t resid)
     esp_set_phase(s, STAT_ST);
     s->rregs[ESP_RINTR] |= INTR_BS;
     esp_raise_irq(s);
-    esp_lower_drq(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);
-- 
2.39.2

[PATCH 73/88] esp.c: remove restriction on FIFO read access when DMA memory routines defined

[Mark Cave-Ayland]

The latest state machines can handle mixing DMA and non-DMA FIFO access for all
SCSI phases except DATA IN and DATA OUT. For DATA IN and DATA OUT phases, the
transfer is complete when TC == 0 and the updated logic will now handle TC
underflow correctly, which makes it just about impossible to manually manipulate
the FIFO during a DMA transfer.

Remove the restriction on FIFO read access when DMA memory routines are defined
which also allows the NeXTCube machine to pass its self-test.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index a3e18bb3d7..f9d848171f 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -1133,14 +1133,7 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr)
 
     switch (saddr) {
     case ESP_FIFO:
-        if (s->dma_memory_read && s->dma_memory_write &&
-                (s->rregs[ESP_RSTAT] & STAT_PIO_MASK) == 0) {
-            /* Data out.  */
-            qemu_log_mask(LOG_UNIMP, "esp: PIO data read not implemented\n");
-            s->rregs[ESP_FIFO] = 0;
-        } else {
-            s->rregs[ESP_FIFO] = esp_fifo_pop(&s->fifo);
-        }
+        s->rregs[ESP_FIFO] = esp_fifo_pop(&s->fifo);
         val = s->rregs[ESP_FIFO];
         break;
     case ESP_RINTR:
-- 
2.39.2

[PATCH 74/88] esp.c: handle non-DMA FIFO writes used to terminate DMA commands

[Mark Cave-Ayland]

Certain versions of MacOS send the first 5 bytes of the CDB using DMA and then
send the last byte of the CDB by writing to the FIFO. Update the non-DMA state
machine to detect the end of the CDB and execute the SCSI command using similar
logic as that which already exists for transferring the remainder of the CDB
using the ESP TI command.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index f9d848171f..aa7dec71e3 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -787,6 +787,16 @@ static void esp_do_nodma(ESPState *s)
             }
             break;
 
+        case CMD_SEL | CMD_DMA:
+        case CMD_SELATN | CMD_DMA:
+            /* Handle when DMA transfer is terminated by non-DMA FIFO write */
+            if (esp_cdb_length(s) && esp_cdb_length(s) ==
+                fifo8_num_used(&s->cmdfifo) - s->cmdfifo_cdb_offset) {
+                    /* Command has been received */
+                    do_cmd(s);
+            }
+            break;
+
         case CMD_SEL:
         case CMD_SELATN:
             /* FIFO already contain entire CDB */
-- 
2.39.2

[PATCH 75/88] esp.c: improve ESP_RSEQ logic consolidation

[Mark Cave-Ayland]

The ESP_RSEQ logic is scattered in a few places throughout the ESP state machine
which is mainly because the ESP_RSEQ register isn't always reset when executing
an ESP select command. Once this is done, the ESP_RSEQ register only needs to be
updated at the point where the sequencer command completes.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index aa7dec71e3..ca26415d5f 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -230,6 +230,7 @@ static int esp_select(ESPState *s)
     target = s->wregs[ESP_WBUSID] & BUSID_DID;
 
     s->ti_size = 0;
+    s->rregs[ESP_RSEQ] = SEQ_0;
 
     if (s->current_req) {
         /* Started a new command before the old one finished. Cancel it. */
@@ -241,7 +242,6 @@ static int esp_select(ESPState *s)
         /* No such drive */
         s->rregs[ESP_RSTAT] = 0;
         s->rregs[ESP_RINTR] = INTR_DC;
-        s->rregs[ESP_RSEQ] = SEQ_0;
         esp_raise_irq(s);
         return -1;
     }
@@ -250,7 +250,6 @@ static int esp_select(ESPState *s)
      * Note that we deliberately don't raise the IRQ here: this will be done
      * either in esp_transfer_data() or esp_command_complete()
      */
-    s->rregs[ESP_RSEQ] = SEQ_CD;
     return 0;
 }
 
@@ -358,7 +357,6 @@ static void handle_s_without_atn(ESPState *s)
     }
 
     esp_set_phase(s, STAT_CD);
-    s->rregs[ESP_RSEQ] = SEQ_CD;
     s->cmdfifo_cdb_offset = 0;
 
     if (s->dma) {
@@ -380,7 +378,6 @@ static void handle_satn_stop(ESPState *s)
     }
 
     esp_set_phase(s, STAT_MO);
-    s->rregs[ESP_RSEQ] = SEQ_MO;
     s->cmdfifo_cdb_offset = 0;
 
     if (s->dma) {
@@ -456,6 +453,7 @@ static void esp_do_dma(ESPState *s)
             if (fifo8_num_used(&s->cmdfifo) >= 1) {
                 /* First byte received, switch to command phase */
                 esp_set_phase(s, STAT_CD);
+                s->rregs[ESP_RSEQ] = SEQ_CD;
                 s->cmdfifo_cdb_offset = 1;
 
                 if (fifo8_num_used(&s->cmdfifo) > 1) {
@@ -468,11 +466,11 @@ static void esp_do_dma(ESPState *s)
         case CMD_SELATNS | CMD_DMA:
             if (fifo8_num_used(&s->cmdfifo) == 1) {
                 /* First byte received, stop in message out phase */
+                s->rregs[ESP_RSEQ] = SEQ_MO;
                 s->cmdfifo_cdb_offset = 1;
 
                 /* Raise command completion interrupt */
                 s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
-                s->rregs[ESP_RSEQ] = SEQ_CD;
                 esp_raise_irq(s);
             }
             break;
@@ -482,7 +480,6 @@ static void esp_do_dma(ESPState *s)
             if (esp_get_tc(s) == 0) {
                 esp_set_phase(s, STAT_CD);
                 s->rregs[ESP_CMD] = 0;
-                s->rregs[ESP_RSEQ] = SEQ_CD;
                 s->rregs[ESP_RINTR] |= INTR_BS;
                 esp_raise_irq(s);
             }
@@ -726,6 +723,7 @@ static void esp_do_nodma(ESPState *s)
             if (fifo8_num_used(&s->cmdfifo) >= 1) {
                 /* First byte received, switch to command phase */
                 esp_set_phase(s, STAT_CD);
+                s->rregs[ESP_RSEQ] = SEQ_CD;
                 s->cmdfifo_cdb_offset = 1;
 
                 if (fifo8_num_used(&s->cmdfifo) > 1) {
@@ -738,6 +736,7 @@ static void esp_do_nodma(ESPState *s)
         case CMD_SELATNS:
             if (fifo8_num_used(&s->cmdfifo) >= 1) {
                 /* First byte received, stop in message out phase */
+                s->rregs[ESP_RSEQ] = SEQ_MO;
                 s->cmdfifo_cdb_offset = 1;
 
                 /* Raise command completion interrupt */
@@ -903,6 +902,7 @@ void esp_command_complete(SCSIRequest *req, size_t resid)
          * and function complete interrupt
          */
         s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
+        s->rregs[ESP_RSEQ] = SEQ_CD;
         break;
 
     case CMD_TI | CMD_DMA:
@@ -948,6 +948,7 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
              * so raise deferred bus service and function complete interrupt
              */
              s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
+             s->rregs[ESP_RSEQ] = SEQ_CD;
              break;
 
         case CMD_SELATNS | CMD_DMA:
@@ -957,6 +958,7 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
              * completion interrupt
              */
              s->rregs[ESP_RINTR] |= INTR_BS;
+             s->rregs[ESP_RSEQ] = SEQ_MO;
              break;
 
         case CMD_TI | CMD_DMA:
-- 
2.39.2

[PATCH 76/88] esp.c: only transfer non-DMA COMMAND phase data for specific commands

[Mark Cave-Ayland]

The contents of the FIFO should only be copied to cmdfifo for ESP commands that
are sending data to the SCSI bus, which are the SEL_* commands and the TI
command. Otherwise any incoming data should be held in the FIFO as normal.

This fixes booting of really old 32-bit SPARC Linux kernels such as Aurelien's
debian_etch_sparc_small.qcow2 test image.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index ca26415d5f..17e2db442c 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -757,13 +757,13 @@ static void esp_do_nodma(ESPState *s)
         break;
 
     case STAT_CD:
-        /* Copy FIFO into cmdfifo */
-        n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
-        n = MIN(fifo8_num_free(&s->cmdfifo), n);
-        fifo8_push_all(&s->cmdfifo, buf, n);
-
         switch (s->rregs[ESP_CMD]) {
         case CMD_TI:
+            /* Copy FIFO into cmdfifo */
+            n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            n = MIN(fifo8_num_free(&s->cmdfifo), n);
+            fifo8_push_all(&s->cmdfifo, buf, n);
+
             cmdlen = fifo8_num_used(&s->cmdfifo);
             trace_esp_handle_ti_cmd(cmdlen);
 
@@ -788,6 +788,11 @@ static void esp_do_nodma(ESPState *s)
 
         case CMD_SEL | CMD_DMA:
         case CMD_SELATN | CMD_DMA:
+            /* Copy FIFO into cmdfifo */
+            n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            n = MIN(fifo8_num_free(&s->cmdfifo), n);
+            fifo8_push_all(&s->cmdfifo, buf, n);
+
             /* Handle when DMA transfer is terminated by non-DMA FIFO write */
             if (esp_cdb_length(s) && esp_cdb_length(s) ==
                 fifo8_num_used(&s->cmdfifo) - s->cmdfifo_cdb_offset) {
@@ -798,7 +803,11 @@ static void esp_do_nodma(ESPState *s)
 
         case CMD_SEL:
         case CMD_SELATN:
-            /* FIFO already contain entire CDB */
+            /* FIFO already contain entire CDB: copy to cmdfifo and execute */
+            n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            n = MIN(fifo8_num_free(&s->cmdfifo), n);
+            fifo8_push_all(&s->cmdfifo, buf, n);
+
             do_cmd(s);
             break;
         }
-- 
2.39.2

[PATCH 77/88] esp.c: only transfer non-DMA MESSAGE OUT phase data for specific commands

[Mark Cave-Ayland]

The contents of the FIFO should only be copied to cmdfifo for ESP commands that
are sending data to the SCSI bus, which are the SEL_* commands and the TI
command. Otherwise any incoming data should be held in the FIFO as normal.

This fixes booting of NetBSD m68k under the Q800 machine once again.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 17e2db442c..d63039af89 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -712,14 +712,13 @@ static void esp_do_nodma(ESPState *s)
 
     switch (esp_get_phase(s)) {
     case STAT_MO:
-        /* Copy FIFO into cmdfifo */
-        n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
-        n = MIN(fifo8_num_free(&s->cmdfifo), n);
-        fifo8_push_all(&s->cmdfifo, buf, n);
-        s->cmdfifo_cdb_offset += n;
-
         switch (s->rregs[ESP_CMD]) {
         case CMD_SELATN:
+            /* Copy FIFO into cmdfifo */
+            n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            n = MIN(fifo8_num_free(&s->cmdfifo), n);
+            fifo8_push_all(&s->cmdfifo, buf, n);
+
             if (fifo8_num_used(&s->cmdfifo) >= 1) {
                 /* First byte received, switch to command phase */
                 esp_set_phase(s, STAT_CD);
@@ -734,6 +733,11 @@ static void esp_do_nodma(ESPState *s)
             break;
 
         case CMD_SELATNS:
+            /* Copy one byte from FIFO into cmdfifo */
+            n = esp_fifo_pop_buf(&s->fifo, buf, 1);
+            n = MIN(fifo8_num_free(&s->cmdfifo), n);
+            fifo8_push_all(&s->cmdfifo, buf, n);
+
             if (fifo8_num_used(&s->cmdfifo) >= 1) {
                 /* First byte received, stop in message out phase */
                 s->rregs[ESP_RSEQ] = SEQ_MO;
@@ -746,6 +750,11 @@ static void esp_do_nodma(ESPState *s)
             break;
 
         case CMD_TI:
+            /* Copy FIFO into cmdfifo */
+            n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            n = MIN(fifo8_num_free(&s->cmdfifo), n);
+            fifo8_push_all(&s->cmdfifo, buf, n);
+
             /* ATN remains asserted until FIFO empty */
             s->cmdfifo_cdb_offset = fifo8_num_used(&s->cmdfifo);
             esp_set_phase(s, STAT_CD);
-- 
2.39.2

[PATCH 78/88] esp.c: consolidate DMA and PDMA logic in DATA OUT phase

[Mark Cave-Ayland]

This allows the removal of duplicate logic shared between the two implementations.
Note that we restrict esp_raise_drq() to PDMA to help reduce the log verbosity
for normal DMA.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index d63039af89..394774c379 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -521,40 +521,29 @@ static void esp_do_dma(ESPState *s)
         if (len > s->async_len) {
             len = s->async_len;
         }
+
         if (s->dma_memory_read) {
             s->dma_memory_read(s->dma_opaque, s->async_buf, len);
-
             esp_set_tc(s, esp_get_tc(s) - len);
-            s->async_buf += len;
-            s->async_len -= len;
-            s->ti_size += len;
-
-            if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
-                /* Defer until the scsi layer has completed */
-                scsi_req_continue(s->current_req);
-                return;
-            }
-
-            esp_dma_ti_check(s);
         } else {
             /* Copy FIFO data to device */
             len = MIN(s->async_len, ESP_FIFO_SZ);
             len = MIN(len, fifo8_num_used(&s->fifo));
-            n = esp_fifo_pop_buf(&s->fifo, s->async_buf, len);
-            s->async_buf += n;
-            s->async_len -= n;
-            s->ti_size += n;
-
+            len = esp_fifo_pop_buf(&s->fifo, s->async_buf, len);
             esp_raise_drq(s);
+        }
 
-            if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
-                /* Defer until the scsi layer has completed */
-                scsi_req_continue(s->current_req);
-                return;
-            }
+        s->async_buf += len;
+        s->async_len -= len;
+        s->ti_size += len;
 
-            esp_dma_ti_check(s);
+        if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
+            /* Defer until the scsi layer has completed */
+            scsi_req_continue(s->current_req);
+            return;
         }
+
+        esp_dma_ti_check(s);
         break;
 
     case STAT_DI:
-- 
2.39.2

[PATCH 79/88] esp.c: consolidate DMA and PDMA logic in DATA IN phase

[Mark Cave-Ayland]

This allows the removal of duplicate logic shared between the two implementations.
Note that we restrict esp_raise_drq() to PDMA to help reduce the log verbosity
for normal DMA.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 394774c379..49fc059eaa 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -557,51 +557,34 @@ static void esp_do_dma(ESPState *s)
         if (len > s->async_len) {
             len = s->async_len;
         }
+
         if (s->dma_memory_write) {
             s->dma_memory_write(s->dma_opaque, s->async_buf, len);
-
-            esp_set_tc(s, esp_get_tc(s) - len);
-            s->async_buf += len;
-            s->async_len -= len;
-            s->ti_size -= len;
-
-            if (s->async_len == 0 && s->ti_size == 0 && esp_get_tc(s)) {
-                /* If the guest underflows TC then terminate SCSI request */
-                scsi_req_continue(s->current_req);
-                return;
-            }
-
-            if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
-                /* Defer until the scsi layer has completed */
-                scsi_req_continue(s->current_req);
-                return;
-            }
-
-            esp_dma_ti_check(s);
         } else {
             /* Copy device data to FIFO */
             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);
             esp_raise_drq(s);
+        }
 
-            if (s->async_len == 0 && s->ti_size == 0 && esp_get_tc(s)) {
-                /* If the guest underflows TC then terminate SCSI request */
-                scsi_req_continue(s->current_req);
-                return;
-            }
+        s->async_buf += len;
+        s->async_len -= len;
+        s->ti_size -= len;
+        esp_set_tc(s, esp_get_tc(s) - len);
 
-            if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
-                /* Defer until the scsi layer has completed */
-                scsi_req_continue(s->current_req);
-                return;
-            }
+        if (s->async_len == 0 && s->ti_size == 0 && esp_get_tc(s)) {
+            /* If the guest underflows TC then terminate SCSI request */
+            scsi_req_continue(s->current_req);
+            return;
+        }
 
-            esp_dma_ti_check(s);
+        if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
+            /* Defer until the scsi layer has completed */
+            scsi_req_continue(s->current_req);
+            return;
         }
+
+        esp_dma_ti_check(s);
         break;
 
     case STAT_ST:
-- 
2.39.2

[PATCH 80/88] esp.c: consolidate DMA and PDMA logic in MESSAGE OUT phase

[Mark Cave-Ayland]

This allows the removal of duplicate logic shared between the two implementations.
Note that we restrict esp_raise_drq() to PDMA to help reduce the log verbosity
for normal DMA.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 49fc059eaa..ae65c2ef37 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -436,17 +436,15 @@ static void esp_do_dma(ESPState *s)
         if (s->dma_memory_read) {
             len = MIN(len, fifo8_num_free(&s->cmdfifo));
             s->dma_memory_read(s->dma_opaque, buf, len);
-            fifo8_push_all(&s->cmdfifo, buf, len);
             esp_set_tc(s, esp_get_tc(s) - len);
-            s->cmdfifo_cdb_offset += len;
         } else {
-            n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
-            n = MIN(fifo8_num_free(&s->cmdfifo), n);
-            fifo8_push_all(&s->cmdfifo, buf, n);
-            s->cmdfifo_cdb_offset += n;
+            len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            len = MIN(fifo8_num_free(&s->cmdfifo), len);
+            esp_raise_drq(s);
         }
 
-        esp_raise_drq(s);
+        fifo8_push_all(&s->cmdfifo, buf, len);
+        s->cmdfifo_cdb_offset += len;
 
         switch (s->rregs[ESP_CMD]) {
         case CMD_SELATN | CMD_DMA:
-- 
2.39.2

[PATCH 81/88] esp.c: remove redundant n variable in PDMA COMMAND phase

[Mark Cave-Ayland]

This variable can be replaced by the existing len variable.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index ae65c2ef37..8ef6d203e0 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -427,7 +427,6 @@ static void esp_do_dma(ESPState *s)
 {
     uint32_t len, cmdlen;
     uint8_t buf[ESP_CMDFIFO_SZ];
-    int n;
 
     len = esp_get_tc(s);
 
@@ -494,10 +493,9 @@ static void esp_do_dma(ESPState *s)
             fifo8_push_all(&s->cmdfifo, buf, len);
             esp_set_tc(s, esp_get_tc(s) - len);
         } else {
-            n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
-            n = MIN(fifo8_num_free(&s->cmdfifo), n);
-            fifo8_push_all(&s->cmdfifo, buf, n);
-
+            len = esp_fifo_pop_buf(&s->fifo, 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);
-- 
2.39.2

[PATCH 82/88] esp.c: consolidate DMA and PDMA logic in STATUS and MESSAGE IN phases

[Mark Cave-Ayland]

This allows the removal of duplicate logic shared between the two implementations.
Note that we restrict esp_raise_drq() to PDMA to help reduce the log verbosity
for normal DMA.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 8ef6d203e0..879e311bc4 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -593,12 +593,11 @@ static void esp_do_dma(ESPState *s)
 
                 if (s->dma_memory_write) {
                     s->dma_memory_write(s->dma_opaque, buf, len);
-                    esp_set_tc(s, esp_get_tc(s) - len);
                 } else {
                     fifo8_push_all(&s->fifo, buf, len);
-                    esp_set_tc(s, esp_get_tc(s) - len);
                 }
 
+                esp_set_tc(s, esp_get_tc(s) - len);
                 esp_set_phase(s, STAT_MI);
 
                 if (esp_get_tc(s) > 0) {
@@ -629,12 +628,12 @@ static void esp_do_dma(ESPState *s)
 
                 if (s->dma_memory_write) {
                     s->dma_memory_write(s->dma_opaque, buf, len);
-                    esp_set_tc(s, esp_get_tc(s) - len);
                 } else {
                     fifo8_push_all(&s->fifo, buf, len);
-                    esp_set_tc(s, esp_get_tc(s) - len);
                 }
 
+                esp_set_tc(s, esp_get_tc(s) - len);
+
                 /* Raise end of command interrupt */
                 s->rregs[ESP_RINTR] |= INTR_FC;
                 esp_raise_irq(s);
-- 
2.39.2

[PATCH 83/88] esp.c: replace n variable with len in esp_do_nodma()

[Mark Cave-Ayland]

This brings esp_do_nodma() in line with esp_do_dma().

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 879e311bc4..73379a3c65 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -675,16 +675,16 @@ static void esp_do_nodma(ESPState *s)
 {
     uint8_t buf[ESP_FIFO_SZ];
     uint32_t cmdlen;
-    int n;
+    int len;
 
     switch (esp_get_phase(s)) {
     case STAT_MO:
         switch (s->rregs[ESP_CMD]) {
         case CMD_SELATN:
             /* Copy FIFO into cmdfifo */
-            n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
-            n = MIN(fifo8_num_free(&s->cmdfifo), n);
-            fifo8_push_all(&s->cmdfifo, buf, n);
+            len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            len = MIN(fifo8_num_free(&s->cmdfifo), len);
+            fifo8_push_all(&s->cmdfifo, buf, len);
 
             if (fifo8_num_used(&s->cmdfifo) >= 1) {
                 /* First byte received, switch to command phase */
@@ -701,9 +701,9 @@ static void esp_do_nodma(ESPState *s)
 
         case CMD_SELATNS:
             /* Copy one byte from FIFO into cmdfifo */
-            n = esp_fifo_pop_buf(&s->fifo, buf, 1);
-            n = MIN(fifo8_num_free(&s->cmdfifo), n);
-            fifo8_push_all(&s->cmdfifo, buf, n);
+            len = esp_fifo_pop_buf(&s->fifo, buf, 1);
+            len = MIN(fifo8_num_free(&s->cmdfifo), len);
+            fifo8_push_all(&s->cmdfifo, buf, len);
 
             if (fifo8_num_used(&s->cmdfifo) >= 1) {
                 /* First byte received, stop in message out phase */
@@ -718,9 +718,9 @@ static void esp_do_nodma(ESPState *s)
 
         case CMD_TI:
             /* Copy FIFO into cmdfifo */
-            n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
-            n = MIN(fifo8_num_free(&s->cmdfifo), n);
-            fifo8_push_all(&s->cmdfifo, buf, n);
+            len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            len = MIN(fifo8_num_free(&s->cmdfifo), len);
+            fifo8_push_all(&s->cmdfifo, buf, len);
 
             /* ATN remains asserted until FIFO empty */
             s->cmdfifo_cdb_offset = fifo8_num_used(&s->cmdfifo);
@@ -736,9 +736,9 @@ static void esp_do_nodma(ESPState *s)
         switch (s->rregs[ESP_CMD]) {
         case CMD_TI:
             /* Copy FIFO into cmdfifo */
-            n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
-            n = MIN(fifo8_num_free(&s->cmdfifo), n);
-            fifo8_push_all(&s->cmdfifo, buf, n);
+            len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            len = MIN(fifo8_num_free(&s->cmdfifo), len);
+            fifo8_push_all(&s->cmdfifo, buf, len);
 
             cmdlen = fifo8_num_used(&s->cmdfifo);
             trace_esp_handle_ti_cmd(cmdlen);
@@ -754,7 +754,7 @@ static void esp_do_nodma(ESPState *s)
                  * service interrupt to indicate transfer complete. Otherwise
                  * defer until the next FIFO write.
                  */
-                if (n) {
+                if (len) {
                     /* Raise interrupt to indicate transfer complete */
                     s->rregs[ESP_RINTR] |= INTR_BS;
                     esp_raise_irq(s);
@@ -765,9 +765,9 @@ static void esp_do_nodma(ESPState *s)
         case CMD_SEL | CMD_DMA:
         case CMD_SELATN | CMD_DMA:
             /* Copy FIFO into cmdfifo */
-            n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
-            n = MIN(fifo8_num_free(&s->cmdfifo), n);
-            fifo8_push_all(&s->cmdfifo, buf, n);
+            len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            len = MIN(fifo8_num_free(&s->cmdfifo), len);
+            fifo8_push_all(&s->cmdfifo, buf, len);
 
             /* Handle when DMA transfer is terminated by non-DMA FIFO write */
             if (esp_cdb_length(s) && esp_cdb_length(s) ==
@@ -780,9 +780,9 @@ static void esp_do_nodma(ESPState *s)
         case CMD_SEL:
         case CMD_SELATN:
             /* FIFO already contain entire CDB: copy to cmdfifo and execute */
-            n = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
-            n = MIN(fifo8_num_free(&s->cmdfifo), n);
-            fifo8_push_all(&s->cmdfifo, buf, n);
+            len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            len = MIN(fifo8_num_free(&s->cmdfifo), len);
+            fifo8_push_all(&s->cmdfifo, buf, len);
 
             do_cmd(s);
             break;
-- 
2.39.2

[PATCH 84/88] esp.c: implement DMA Transfer Pad command for DATA phases

[Mark Cave-Ayland]

The Transfer Pad command is used to either drop incoming FIFO data during the
DATA IN phase or generate a series of zero bytes in the FIFO during the DATA
OUT phase.

Implement the DMA Transfer Pad command for the DATA phases which is used by
the NeXTCube firmware in the DATA IN phase to ignore part of the incoming SCSI
data as it is copied into memory.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 73379a3c65..5583b3eb56 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -387,6 +387,15 @@ static void handle_satn_stop(ESPState *s)
     }
 }
 
+static void handle_pad(ESPState *s)
+{
+    if (s->dma) {
+        esp_do_dma(s);
+    } else {
+        esp_do_nodma(s);
+    }
+}
+
 static void write_response(ESPState *s)
 {
     trace_esp_write_response(s->status);
@@ -518,20 +527,38 @@ static void esp_do_dma(ESPState *s)
             len = s->async_len;
         }
 
-        if (s->dma_memory_read) {
-            s->dma_memory_read(s->dma_opaque, s->async_buf, len);
-            esp_set_tc(s, esp_get_tc(s) - len);
-        } else {
-            /* 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);
-            esp_raise_drq(s);
-        }
+        switch (s->rregs[ESP_CMD]) {
+        case CMD_TI | CMD_DMA:
+            if (s->dma_memory_read) {
+                s->dma_memory_read(s->dma_opaque, s->async_buf, len);
+                esp_set_tc(s, esp_get_tc(s) - len);
+            } else {
+                /* 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);
+                esp_raise_drq(s);
+            }
 
-        s->async_buf += len;
-        s->async_len -= len;
-        s->ti_size += len;
+            s->async_buf += len;
+            s->async_len -= len;
+            s->ti_size += len;
+            break;
+
+        case CMD_PAD | CMD_DMA:
+            /* Copy TC zero bytes into the incoming stream */
+            if (!s->dma_memory_read) {
+                len = MIN(s->async_len, ESP_FIFO_SZ);
+                len = MIN(len, fifo8_num_free(&s->fifo));
+            }
+
+            memset(s->async_buf, 0, len);
+
+            s->async_buf += len;
+            s->async_len -= len;
+            s->ti_size += len;
+            break;
+        }
 
         if (s->async_len == 0 && fifo8_num_used(&s->fifo) < 2) {
             /* Defer until the scsi layer has completed */
@@ -554,19 +581,35 @@ static void esp_do_dma(ESPState *s)
             len = s->async_len;
         }
 
-        if (s->dma_memory_write) {
-            s->dma_memory_write(s->dma_opaque, s->async_buf, len);
-        } else {
-            /* Copy device data to FIFO */
-            len = MIN(len, fifo8_num_free(&s->fifo));
-            fifo8_push_all(&s->fifo, s->async_buf, len);
-            esp_raise_drq(s);
-        }
+        switch (s->rregs[ESP_CMD]) {
+        case CMD_TI | CMD_DMA:
+            if (s->dma_memory_write) {
+                s->dma_memory_write(s->dma_opaque, s->async_buf, len);
+            } else {
+                /* Copy device data to FIFO */
+                len = MIN(len, fifo8_num_free(&s->fifo));
+                fifo8_push_all(&s->fifo, s->async_buf, len);
+                esp_raise_drq(s);
+            }
+
+            s->async_buf += len;
+            s->async_len -= len;
+            s->ti_size -= len;
+            esp_set_tc(s, esp_get_tc(s) - len);
+            break;
+
+        case CMD_PAD | CMD_DMA:
+            /* Drop TC bytes from the incoming stream */
+            if (!s->dma_memory_write) {
+                len = MIN(len, fifo8_num_free(&s->fifo));
+            }
 
-        s->async_buf += len;
-        s->async_len -= len;
-        s->ti_size -= len;
-        esp_set_tc(s, esp_get_tc(s) - len);
+            s->async_buf += len;
+            s->async_len -= len;
+            s->ti_size -= len;
+            esp_set_tc(s, esp_get_tc(s) - len);
+            break;
+        }
 
         if (s->async_len == 0 && s->ti_size == 0 && esp_get_tc(s)) {
             /* If the guest underflows TC then terminate SCSI request */
@@ -1087,9 +1130,7 @@ static void esp_run_cmd(ESPState *s)
         break;
     case CMD_PAD:
         trace_esp_mem_writeb_cmd_pad(cmd);
-        s->rregs[ESP_RSTAT] = STAT_TC;
-        s->rregs[ESP_RINTR] |= INTR_FC;
-        s->rregs[ESP_RSEQ] = 0;
+        handle_pad(s);
         break;
     case CMD_SATN:
         trace_esp_mem_writeb_cmd_satn(cmd);
-- 
2.39.2

[PATCH 85/88] esp.c: rename irq_data IRQ to drq_irq

[Mark Cave-Ayland]

The IRQ represented by irq_data is actually the DRQ (DMA request) line so rename
it accordingly.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 5583b3eb56..fb68606f00 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -62,13 +62,13 @@ static void esp_lower_irq(ESPState *s)
 
 static void esp_raise_drq(ESPState *s)
 {
-    qemu_irq_raise(s->irq_data);
+    qemu_irq_raise(s->drq_irq);
     trace_esp_raise_drq();
 }
 
 static void esp_lower_drq(ESPState *s)
 {
-    qemu_irq_lower(s->irq_data);
+    qemu_irq_lower(s->drq_irq);
     trace_esp_lower_drq();
 }
 
@@ -1062,7 +1062,7 @@ void esp_hard_reset(ESPState *s)
 static void esp_soft_reset(ESPState *s)
 {
     qemu_irq_lower(s->irq);
-    qemu_irq_lower(s->irq_data);
+    qemu_irq_lower(s->drq_irq);
     esp_hard_reset(s);
 }
 
@@ -1489,7 +1489,7 @@ static void sysbus_esp_realize(DeviceState *dev, Error **errp)
     }
 
     sysbus_init_irq(sbd, &s->irq);
-    sysbus_init_irq(sbd, &s->irq_data);
+    sysbus_init_irq(sbd, &s->drq_irq);
     assert(sysbus->it_shift != -1);
 
     s->chip_id = TCHI_FAS100A;
diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
index 39b416f538..c6e8b64e20 100644
--- a/include/hw/scsi/esp.h
+++ b/include/hw/scsi/esp.h
@@ -25,7 +25,7 @@ struct ESPState {
     uint8_t rregs[ESP_REGS];
     uint8_t wregs[ESP_REGS];
     qemu_irq irq;
-    qemu_irq irq_data;
+    qemu_irq drq_irq;
     uint8_t chip_id;
     bool tchi_written;
     int32_t ti_size;
-- 
2.39.2

[PATCH 86/88] esp.c: keep track of the DRQ state during DMA

[Mark Cave-Ayland]

Currently the DRQ IRQ is updated every time DMA data is sent/received which
is both inefficient and causes excessive logging of the DRQ state. Add a
new drq_state bool that only updates the DRQ IRQ if its state changes.

This commit adds the new drq_state bool to the migration state: since the
version number has already been increased earlier in the series, there is
no need to repeat it again here. The DRQ IRQ is (currently) only used for
PDMA transfers which already have a migration break in this series so
there are no problems setting its value post-load.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index fb68606f00..04615d8b5f 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -62,14 +62,20 @@ static void esp_lower_irq(ESPState *s)
 
 static void esp_raise_drq(ESPState *s)
 {
-    qemu_irq_raise(s->drq_irq);
-    trace_esp_raise_drq();
+    if (!(s->drq_state)) {
+        qemu_irq_raise(s->drq_irq);
+        trace_esp_raise_drq();
+        s->drq_state = true;
+    }
 }
 
 static void esp_lower_drq(ESPState *s)
 {
-    qemu_irq_lower(s->drq_irq);
-    trace_esp_lower_drq();
+    if (s->drq_state) {
+        qemu_irq_lower(s->drq_irq);
+        trace_esp_lower_drq();
+        s->drq_state = false;
+    }
 }
 
 void esp_dma_enable(ESPState *s, int irq, int level)
@@ -1358,6 +1364,7 @@ const VMStateDescription vmstate_esp = {
         VMSTATE_UINT8_TEST(mig_ti_cmd, ESPState,
                            esp_is_between_version_5_and_6),
         VMSTATE_UINT8_TEST(lun, ESPState, esp_is_version_6),
+        VMSTATE_BOOL(drq_state, ESPState),
         VMSTATE_END_OF_LIST()
     },
 };
diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
index c6e8b64e20..533d856aa3 100644
--- a/include/hw/scsi/esp.h
+++ b/include/hw/scsi/esp.h
@@ -26,6 +26,7 @@ struct ESPState {
     uint8_t wregs[ESP_REGS];
     qemu_irq irq;
     qemu_irq drq_irq;
+    bool drq_state;
     uint8_t chip_id;
     bool tchi_written;
     int32_t ti_size;
-- 
2.39.2

[PATCH 87/88] esp.c: switch TypeInfo registration to use DEFINE_TYPES() macro

[Mark Cave-Ayland]

The use of the DEFINE_TYPES() macro will soon be recommended over the use of
calling type_init() directly.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 04615d8b5f..b8762d5ee0 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -1549,14 +1549,6 @@ static void sysbus_esp_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
 
-static const TypeInfo sysbus_esp_info = {
-    .name          = TYPE_SYSBUS_ESP,
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_init = sysbus_esp_init,
-    .instance_size = sizeof(SysBusESPState),
-    .class_init    = sysbus_esp_class_init,
-};
-
 static void esp_finalize(Object *obj)
 {
     ESPState *s = ESP(obj);
@@ -1582,19 +1574,22 @@ static void esp_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
 
-static const TypeInfo esp_info = {
-    .name = TYPE_ESP,
-    .parent = TYPE_DEVICE,
-    .instance_init = esp_init,
-    .instance_finalize = esp_finalize,
-    .instance_size = sizeof(ESPState),
-    .class_init = esp_class_init,
+static const TypeInfo esp_info_types[] = {
+    {
+        .name          = TYPE_SYSBUS_ESP,
+        .parent        = TYPE_SYS_BUS_DEVICE,
+        .instance_init = sysbus_esp_init,
+        .instance_size = sizeof(SysBusESPState),
+        .class_init    = sysbus_esp_class_init,
+    },
+    {
+        .name = TYPE_ESP,
+        .parent = TYPE_DEVICE,
+        .instance_init = esp_init,
+        .instance_finalize = esp_finalize,
+        .instance_size = sizeof(ESPState),
+        .class_init = esp_class_init,
+    },
 };
 
-static void esp_register_types(void)
-{
-    type_register_static(&sysbus_esp_info);
-    type_register_static(&esp_info);
-}
-
-type_init(esp_register_types)
+DEFINE_TYPES(esp_info_types)
-- 
2.39.2

Re: [PATCH 87/88] esp.c: switch TypeInfo registration to use DEFINE_TYPES() macro

[Philippe Mathieu-Daudé]

On 12/1/24 13:54, Mark Cave-Ayland wrote:
> The use of the DEFINE_TYPES() macro will soon be recommended over the use of
> calling type_init() directly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 39 +++++++++++++++++----------------------
>   1 file changed, 17 insertions(+), 22 deletions(-)

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

[PATCH 88/88] esp.c: add my copyright to the file

[Mark Cave-Ayland]

This series has involved rewriting and/or updating a considerable part of the ESP
emulation so update the copyright in esp.c to reflect this.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index b8762d5ee0..590ff99744 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -3,6 +3,7 @@
  *
  * Copyright (c) 2005-2006 Fabrice Bellard
  * Copyright (c) 2012 Herve Poussineau
+ * Copyright (c) 2023 Mark Cave-Ayland
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
-- 
2.39.2

Re: [PATCH 00/88] esp: rework ESP emulation to use a SCSI phase-based state machine

[Mark Cave-Ayland]

On 12/01/2024 12:52, Mark Cave-Ayland wrote:

> The ESP SCSI chip fundamentally consists of a FIFO for transferring data to/from
> the SCSI bus along with a command sequencer which automates various processes such
> as selection, message/command transfer and data transfer. What makes this chip
> particularly interesting is that the FIFO is also used as a buffer for DMA transfers
> which makes it possible to mix and match DMA and non-DMA transfers when sending and
> receiving data to/from the SCSI bus.
> 
> Whilst the current ESP emulation works well for most cases, there are several
> problems that I've encountered whilst trying to debug various guest issues:
> 
> 
> 1) Multiple code paths for non-DMA, DMA and pDMA
> 
> Consider a SCSI request being submitted by the guest: if it is a DMA SCSI
> request then it takes one path through get_cmd(), and a different path through
> get_cmd() for pDMA. This then leads to the DMA code path being called for DMA
> and the pDMA codepath being called for each FIFO write. Finally once you add
> in the non-DMA path you end up with a total of 5 potential codepaths for any
> given SCSI request.
> 
> 2) Manual handling of STAT_TC flag
> 
> According to the datasheet the STAT_TC flag in ESP_RSTAT is set when the DMA
> transfer counter reaches zero. The current code handles this manually in
> multiple places, including where it shouldn't be necessary.
> 
> 3) Deferred interrupts are used only for reads and not writes
> 
> The ESP emulation currently makes use of a deferred interrupt when submitting
> read commands i.e. the interrupt flags are not updated immediately but when
> the QEMU SCSI layer indicates it is ready. This works well for reads, but isn't
> implemented for writes which can cause problems when the host is heavily loaded
> or a large SCSI write is requested.
> 
> 4) Padding of pDMA TI transfers to a minimum of 16 bytes
> 
> In order to allow Classic MacOS to boot successfully there is a workaround that
> pads all pDMA TI transfers to a minimum of 16 bytes (see commit 7aa6baee7c
> "esp: add support for unaligned accesses"). This feature is not documented and it
> turns out that this is a symptom of incorrect handling of transfer
> overflow/underflow conditions.
> 
> 5) Duplication of some non-DMA logic in esp_reg_read() and esp_reg_write()
> 
> When reading and writing to the FIFO there is some duplication of logic from
> esp_do_nodma() in esp_reg_read() and esp_reg_write(). This should be eliminated
> in favour of a single state machine for handling non-DMA FIFO accesses in a
> single place.
> 
> 
> The series here reworks the ESP emulation to use a SCSI phase-based state machine
> with just two paths: one for DMA/pDMA requests, and another for non-DMA requests.
> The pDMA callbacks are completely removed so that there is now only a single path
> for DMA requests. As part of this work the manual STAT_TC handling is removed, and a
> couple of bugs in the SCSI phase selection are fixed to allow proper transfer
> underflow/overflow detection and recovery.
> 
> I've tested the series with all of my available ESP images and compatibility is
> greatly improved with these changes: I believe that this series should also fix a
> few GitLab issues including https://gitlab.com/qemu-project/qemu/-/issues/1831,
> https://gitlab.com/qemu-project/qemu/-/issues/611 and
> https://gitlab.com/qemu-project/qemu/-/issues/1127. In addition it allows Aurelien's
> really old SPARC Linux images to boot once again, and also the NeXTCube machine can
> now boot, load its kernel from disk and start to execute it.
> 
> The series breaks down into the following approximate sections:
> 
>    - Patches 1-6 do some initial tidy-up for ESP select commands
> 
>    - Patches 7-16 implement consolidation of the STAT_TC flag handling
> 
>    - Patches 17-36 start to bring esp_do_dma() and do_pdma_dma() into line with each
>      other, removing unneeded hacks and fixing up end-of-phase logic
> 
>    - Patches 37-56 move the MESSAGE OUT and COMMAND phase processing to the new SCSI
>      phase-based state machine, allowing the removal of the pDMA-specific callbacks
> 
>    - Patches 57-62 tidy up the end of SCSI command logic and adds deferred interrupt
>      handling for the DATA IN phase to complement the existing DATA OUT phase
>      implementation
> 
>    - Patches 63-64 remove the ti_cmd field which is now no longer required
> 
>    - Patches 65-67 move the last bit of MESSAGE OUT and COMMAND phase processing to
>      the new SCSI phase-based state machine which allows get_cmd() to be removed
> 
>    - Patches 68-69 move the STATUS and MESSAGE IN phase processing to the new SCSI
>      phase-based state machine
> 
>    - Patches 70-72 fix the reset of the ESP_RINTR/ESP_STAT registers which allows the
>      TC underflow detection to work correctly
> 
>    - Patch 73 removes the restriction on FIFO read access if the DMA routines are not
>      defined (which also allows the NeXTCube to pass its self-test)
> 
>    - Patches 74-77 update the state machine to allow more combinations of mixed TI
>      and FIFO reads/writes exposed by my test images
> 
>    - Patches 78-88 are final tidy-ups and improvements, along with adding an
>      implementation of the "Transfer Pad" command
> 
> There is a migration break for the Q800 machine here as the series removes the pDMA
> subsection: this seems the safest option as it isn't possible to map the old pDMA
> callback enum directly to the new state machine. In theory there is a small window
> whereby the normal DMA state may miss setting data_ready correctly, but I haven't
> been able to recreate this during my tests.
> 
> 
> Notes for reviewers:
> 
> This series is not a particularly easy one to review without having good in-depth
> knowledge of the ESP device. The biggest issue I found is that due to the state
> machines used by the client upon receiving an interrupt, a change to a ESP register
> in one place can cause a completely unrelated error elsewhere.
> 
> A prime example of this is the consolidation of the STAT_TC flag: updating the
> emulated device to behave as described in the datasheet caused other failures until
> other fixes/updates could be made. This is why the work is done over 2 separate
> commits with a gap in-between to keep the series bisectible, clearly indicating how
> sensitive this series is to commit order.
> 
> In order to implement the changes I decided to focus on 3 main test images that should
> pass for each commit to keep the series bisectible:
> 
> 1) Modern Linux kernel with am53c974.c (hppa)
> 2) Modern Linux kernel with mac_esp.c (q800)
> 3) Classic MacOS 8.0
> 
> Other images may stop working during parts of the series, which sometimes is the
> only option as otherwise implementing a compatibility workaround would be
> prohibitively time-consuming or impossible at that point in the series. By the
> end of the series all of my test images work correctly, including a few extra that
> don't work with QEMU current git master.
> 
> Finally it is worth reiterating that in its current form this series fixes several
> significant bugs and improves the compatibility of the ESP emulation, and these changes
> make future fixes considerably easier compared with the existing implementation.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> 
> Mark Cave-Ayland (88):
>    esp: don't clear cmdfifo when esp_select() fails in get_cmd()
>    esp: move existing request cancel check into esp_select()
>    esp.c: add FIFO wraparound support to esp_fifo_pop_buf()
>    esp: remove FIFO clear from esp_select()
>    esp: move esp_select() to ESP selection commands from get_cmd()
>    esp: update esp_set_tc() to set STAT_TC flag
>    esp: start removal of manual STAT_TC setting when transfer counter
>      reaches zero
>    esp: move command execution logic to new esp_run_cmd() function
>    esp: update TC check logic in do_dma_pdma_cb() to check for TC == 0
>    esp: move buffer and TC logic into separate to/from device paths in
>      esp_do_dma()
>    esp.c: remove unused case from esp_pdma_read()
>    esp.c: don't accumulate directly into cmdfifo
>    esp.c: decrement the TC during MESSAGE OUT and COMMAND phases
>    esp.c: introduce esp_set_phase() helper function
>    esp.c: remove another set of manual STAT_TC updates
>    esp.c: remove MacOS TI workaround that pads FIFO transfers to
>      ESP_FIFO_SZ
>    esp.c: don't reset the TC and ESP_RSEQ state when executing a SCSI
>      command
>    esp.c: don't clear RFLAGS register when DMA is complete
>    esp: remove zero transfer size check from esp_do_dma()
>    esp.c: update condition for esp_dma_done() in esp_do_dma() from device
>      path
>    esp.c: update condition for esp_dma_done() in esp_do_dma() to device
>      path
>    esp.c: ensure that the PDMA callback is called for every device read
>    esp.c: don't immediately raise INTR_BS if SCSI data needed in
>      esp_do_dma()
>    esp.c: remove TC adjustment in esp_do_dma() from device path
>    esp.c: remove unaligned adjustment in do_dma_pdma_cb() to device path
>    esp.c: remove unneeded if() check in esp_transfer_data()
>    esp.c: update end of transfer logic at the end of esp_transfer_data()
>    esp.c: consolidate async_len and TC == 0 checks in do_dma_pdma_cb()
>      and esp_do_dma()
>    esp.c: fix premature end of phase logic esp_command_complete
>    esp.c: move TC and FIFO check logic into esp_dma_done()
>    esp.c: rename esp_dma_done() to esp_dma_ti_check()
>    esp.c: copy PDMA logic for transfers to device from do_dma_pdma_cb()
>      to esp_do_dma()
>    esp.c: copy logic for do_cmd transfers from do_dma_pdma_cb() to
>      esp_do_dma()
>    esp.c: update esp_do_dma() bypass if async_len is zero to include
>      non-zero transfer check
>    esp.c: move end of SCSI transfer check after TC adjustment in
>      do_dma_pdma_cb()
>    esp.c: remove s_without_satn_pdma_cb() PDMA callback
>    esp.c: introduce esp_get_phase() function
>    esp.c: convert esp_do_dma() to switch statement based upon SCSI phase
>    esp.c: convert do_dma_pdma_db() to switch statement based upon SCSI
>      phase
>    esp.c: convert esp_do_nodma() to switch statement based upon SCSI
>      phase
>    esp.c: convert esp_do_dma() do_cmd path to check for SCSI phase
>      instead
>    esp.c: convert do_dma_pdma_cb() do_cmd path to check for SCSI phase
>      instead
>    esp.c: convert esp_do_nodma() do_cmd path to check for SCSI phase
>      instead
>    esp.c: convert esp_reg_write() do_cmd path to check for SCSI phase
>      instead
>    esp.c: remove do_cmd from ESPState
>    esp.c: untangle MESSAGE OUT and COMMAND phase logic in esp_do_dma()
>    esp.c: untangle MESSAGE OUT and COMMAND phase logic in
>      do_dma_pdma_cb()
>    esp.c: untangle MESSAGE OUT and COMMAND phase logic in esp_do_nodma()
>    esp.c: move CMD_SELATN end of message phase detection to esp_do_dma()
>      and do_dma_pdma_cb()
>    esp.c: move CMD_TI end of message phase detection to esp_do_dma() and
>      do_dma_pdma_cb()
>    esp.c: don't use get_cmd() for CMD_SEL DMA commands
>    esp.c: move CMD_SELATNS end of command logic to esp_do_dma() and
>      do_dma_pdma_cb()
>    esp.c: replace do_dma_pdma_cb() with esp_do_dma()
>    esp.c: move CMD_ICCS command logic to esp_do_dma()
>    esp.c: always use esp_do_dma() in pdma_cb()
>    esp.c: remove unused PDMA callback implementation
>    esp.c: rename data_in_ready to to data_ready
>    esp.c: separate logic based upon ESP command in esp_command_complete()
>    esp.c: separate logic based upon ESP command in esp_transfer_data()
>    esp.c: use deferred interrupts for both DATA IN and DATA OUT phases
>    esp.c: remove DATA IN phase logic when reading from FIFO
>    esp.c: zero command register when TI command terminates due to phase
>      change
>    esp.c: remove unneeded ti_cmd field
>    esp.c: don't raise INTR_BS interrupt in DATA IN phase until TI command
>      issued
>    esp.c: move non-DMA TI logic to separate esp_nodma_ti_dataout()
>      function
>    esp.c: process non-DMA FIFO writes in esp_do_nodma()
>    esp.c: replace get_cmd() with esp_do_nodma()
>    esp.c: move write_response() non-DMA logic to esp_do_nodma()
>    esp.c: consolidate end of command sequence after ICCS command
>    esp.c: ensure that STAT_INT is cleared when reading ESP_RINTR
>    esp.c: don't clear the SCSI phase when reading ESP_RINTR
>    esp.c: handle TC underflow for DMA SCSI requests
>    esp.c: remove restriction on FIFO read access when DMA memory routines
>      defined
>    esp.c: handle non-DMA FIFO writes used to terminate DMA commands
>    esp.c: improve ESP_RSEQ logic consolidation
>    esp.c: only transfer non-DMA COMMAND phase data for specific commands
>    esp.c: only transfer non-DMA MESSAGE OUT phase data for specific
>      commands
>    esp.c: consolidate DMA and PDMA logic in DATA OUT phase
>    esp.c: consolidate DMA and PDMA logic in DATA IN phase
>    esp.c: consolidate DMA and PDMA logic in MESSAGE OUT phase
>    esp.c: remove redundant n variable in PDMA COMMAND phase
>    esp.c: consolidate DMA and PDMA logic in STATUS and MESSAGE IN phases
>    esp.c: replace n variable with len in esp_do_nodma()
>    esp.c: implement DMA Transfer Pad command for DATA phases
>    esp.c: rename irq_data IRQ to drq_irq
>    esp.c: keep track of the DRQ state during DMA
>    esp.c: switch TypeInfo registration to use DEFINE_TYPES() macro
>    esp.c: add my copyright to the file
> 
>   hw/scsi/esp.c         | 1371 ++++++++++++++++++++++-------------------
>   hw/scsi/trace-events  |    1 +
>   include/hw/scsi/esp.h |   18 +-
>   3 files changed, 727 insertions(+), 663 deletions(-)

Any other thoughts on this series, particularly from SCSI maintainers? I appreciate 
that it's not the easiest one to review, and I'm grateful for the T-B tags received 
so far.


ATB,

Mark.

Re: [PATCH 00/88] esp: rework ESP emulation to use a SCSI phase-based state machine

[Thomas Huth]

On 12/01/2024 13.52, Mark Cave-Ayland wrote:
> The ESP SCSI chip fundamentally consists of a FIFO for transferring data to/from
> the SCSI bus along with a command sequencer which automates various processes such
> as selection, message/command transfer and data transfer. What makes this chip
> particularly interesting is that the FIFO is also used as a buffer for DMA transfers
> which makes it possible to mix and match DMA and non-DMA transfers when sending and
> receiving data to/from the SCSI bus.
> 
> Whilst the current ESP emulation works well for most cases, there are several
> problems that I've encountered whilst trying to debug various guest issues:
> 
> 
> 1) Multiple code paths for non-DMA, DMA and pDMA
> 
> Consider a SCSI request being submitted by the guest: if it is a DMA SCSI
> request then it takes one path through get_cmd(), and a different path through
> get_cmd() for pDMA. This then leads to the DMA code path being called for DMA
> and the pDMA codepath being called for each FIFO write. Finally once you add
> in the non-DMA path you end up with a total of 5 potential codepaths for any
> given SCSI request.
> 
> 2) Manual handling of STAT_TC flag
> 
> According to the datasheet the STAT_TC flag in ESP_RSTAT is set when the DMA
> transfer counter reaches zero. The current code handles this manually in
> multiple places, including where it shouldn't be necessary.
> 
> 3) Deferred interrupts are used only for reads and not writes
> 
> The ESP emulation currently makes use of a deferred interrupt when submitting
> read commands i.e. the interrupt flags are not updated immediately but when
> the QEMU SCSI layer indicates it is ready. This works well for reads, but isn't
> implemented for writes which can cause problems when the host is heavily loaded
> or a large SCSI write is requested.
> 
> 4) Padding of pDMA TI transfers to a minimum of 16 bytes
> 
> In order to allow Classic MacOS to boot successfully there is a workaround that
> pads all pDMA TI transfers to a minimum of 16 bytes (see commit 7aa6baee7c
> "esp: add support for unaligned accesses"). This feature is not documented and it
> turns out that this is a symptom of incorrect handling of transfer
> overflow/underflow conditions.
> 
> 5) Duplication of some non-DMA logic in esp_reg_read() and esp_reg_write()
> 
> When reading and writing to the FIFO there is some duplication of logic from
> esp_do_nodma() in esp_reg_read() and esp_reg_write(). This should be eliminated
> in favour of a single state machine for handling non-DMA FIFO accesses in a
> single place.
> 
> 
> The series here reworks the ESP emulation to use a SCSI phase-based state machine
> with just two paths: one for DMA/pDMA requests, and another for non-DMA requests.
> The pDMA callbacks are completely removed so that there is now only a single path
> for DMA requests. As part of this work the manual STAT_TC handling is removed, and a
> couple of bugs in the SCSI phase selection are fixed to allow proper transfer
> underflow/overflow detection and recovery.
> 
> I've tested the series with all of my available ESP images and compatibility is
> greatly improved with these changes: I believe that this series should also fix a
> few GitLab issues including https://gitlab.com/qemu-project/qemu/-/issues/1831,
> https://gitlab.com/qemu-project/qemu/-/issues/611 and
> https://gitlab.com/qemu-project/qemu/-/issues/1127. In addition it allows Aurelien's
> really old SPARC Linux images to boot once again, and also the NeXTCube machine can
> now boot, load its kernel from disk and start to execute it.

FWIW, I did some sanity checking by installing an old Fedora 12 onto a SCSI 
disk attached to the am53c974 and dc390 controllers, and everything still 
works fine with your patches. Thus feel free to add:

Tested-by: Thomas Huth <thuth@redhat.com>