[PATCH 0/5] esp: fixes for MacOS toolbox ROM

[PATCH 0/5] esp: fixes for MacOS toolbox ROM

[Mark Cave-Ayland]

This patchset contains more ESP fixes from my attempts to boot MacOS under
the QEMU q800 machine (along with a related NetBSD fix).

With these patches it is possible for the MacOS toolbox ROM and MacOS drivers
to detect and access SCSI drives and CDROMs during the MacOS boot process.

This patchset has been tested on top of the ESP fix series posted yesterday
(see https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg05763.html) with
the extended set of ESP test images without noticing any regressions.

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

[q800-macos-upstream patchset series: 1]


Mark Cave-Ayland (5):
  esp: allow non-DMA callback in esp_transfer_data() initial transfer
  esp: handle non-DMA transfers from the target one byte at a time
  esp: ensure PDMA write transfers are flushed from the FIFO to the
    target immediately
  esp: revert 75ef849696 "esp: correctly fill bus id with requested lun"
  esp: correctly accumulate extended messages for PDMA

 hw/scsi/esp.c | 137 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 83 insertions(+), 54 deletions(-)

-- 
2.20.1

[PATCH 1/5] esp: allow non-DMA callback in esp_transfer_data() initial transfer

[Mark Cave-Ayland]

The current implementation only resumes DMA transfers when incoming data is
received from the target device, but this is also required for non-DMA transfers
with the next set of non-DMA changes.

Rather than duplicate the DMA/non-DMA dispatch logic in the initial transfer
section, update the code so that the initial transfer section can just
fallthrough to the main DMA/non-DMA dispatch logic.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index b668acef82..a611718769 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -804,16 +804,6 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
         s->rregs[ESP_RSTAT] |= STAT_TC;
         s->rregs[ESP_RINTR] |= INTR_BS;
         esp_raise_irq(s);
-
-        /*
-         * If data is ready to transfer and the TI command has already
-         * been executed, start DMA immediately. Otherwise DMA will start
-         * when host sends the TI command
-         */
-        if (s->ti_size && (s->rregs[ESP_CMD] == (CMD_TI | CMD_DMA))) {
-            esp_do_dma(s);
-        }
-        return;
     }
 
     if (s->ti_cmd == 0) {
@@ -827,7 +817,7 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
         return;
     }
 
-    if (s->ti_cmd & CMD_DMA) {
+    if (s->ti_cmd == (CMD_TI | CMD_DMA)) {
         if (dmalen) {
             esp_do_dma(s);
         } else if (s->ti_size <= 0) {
@@ -838,7 +828,7 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
             esp_dma_done(s);
             esp_lower_drq(s);
         }
-    } else {
+    } else if (s->ti_cmd == CMD_TI) {
         esp_do_nodma(s);
     }
 }
-- 
2.20.1

[PATCH 2/5] esp: handle non-DMA transfers from the target one byte at a time

[Mark Cave-Ayland]

The initial implementation of non-DMA transfers was based upon analysis of traces
from the MacOS toolbox ROM for handling unaligned reads but missed one key
aspect - during a non-DMA transfer from the target, the bus service interrupt
should be raised for every single byte received from the bus and not just at either
the end of the transfer or when the FIFO is full.

Adjust the non-DMA code accordingly so that esp_do_nodma() is called for every byte
received from the target. This also includes special handling for managing the change
from DATA IN to STATUS phase as this needs to occur when the final byte is read out
from the FIFO, and not at the end of the transfer of the last byte into the FIFO.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index a611718769..f2fd4e443a 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -740,20 +740,17 @@ static void esp_do_nodma(ESPState *s)
         s->async_len -= len;
         s->ti_size += len;
     } else {
-        len = MIN(s->ti_size, s->async_len);
-        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;
+        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);
-
-        if (to_device || s->ti_size == 0) {
-            return;
-        }
+        return;
     }
 
     s->rregs[ESP_RINTR] |= INTR_BS;
@@ -763,20 +760,37 @@ static void esp_do_nodma(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);
 
     trace_esp_command_complete();
-    if (s->ti_size != 0) {
-        trace_esp_command_complete_unexpected();
+
+    /*
+     * Non-DMA transfers from the target will leave the last byte in
+     * the FIFO so don't reset ti_size in this case
+     */
+    if (s->dma || to_device) {
+        if (s->ti_size != 0) {
+            trace_esp_command_complete_unexpected();
+        }
+        s->ti_size = 0;
     }
-    s->ti_size = 0;
+
     s->async_len = 0;
     if (req->status) {
         trace_esp_command_complete_fail();
     }
     s->status = req->status;
-    s->rregs[ESP_RSTAT] = STAT_ST;
-    esp_dma_done(s);
-    esp_lower_drq(s);
+
+    /*
+     * If the transfer is finished, switch to status phase. For non-DMA
+     * 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;
+        esp_dma_done(s);
+        esp_lower_drq(s);
+    }
+
     if (s->current_req) {
         scsi_req_unref(s->current_req);
         s->current_req = NULL;
@@ -895,6 +909,17 @@ 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 (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
+                     */
+                    s->rregs[ESP_RSTAT] = STAT_TC | STAT_ST;
+                }
+            }
             s->rregs[ESP_FIFO] = esp_fifo_pop(&s->fifo);
         }
         val = s->rregs[ESP_FIFO];
@@ -945,15 +970,18 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val)
     case ESP_FIFO:
         if (s->do_cmd) {
             esp_fifo_push(&s->cmdfifo, val);
+
+            /*
+             * 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 {
             esp_fifo_push(&s->fifo, val);
         }
-
-        /* Non-DMA transfers raise an interrupt after every byte */
-        if (s->rregs[ESP_CMD] == CMD_TI) {
-            s->rregs[ESP_RINTR] |= INTR_FC | INTR_BS;
-            esp_raise_irq(s);
-        }
         break;
     case ESP_CMD:
         s->rregs[saddr] = val;
-- 
2.20.1

[PATCH 3/5] esp: ensure PDMA write transfers are flushed from the FIFO to the target immediately

[Mark Cave-Ayland]

After each PDMA write transfer the MacOS CDROM driver waits until the FIFO is empty
(i.e. its contents have been written out to the SCSI bus) by polling the FIFO count
register until it reads 0. This doesn't work with the current PDMA write
implementation which waits until either the FIFO is full or the transfer is complete
before invoking the PDMA callback to process the FIFO contents.

Change the PDMA write transfer logic so that the PDMA callback is invoked after each
PDMA write to transfer the FIFO contents to the target buffer immediately, and hence
avoid getting stuck in the FIFO count register polling loop.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index f2fd4e443a..afb4a7f9f1 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -338,9 +338,9 @@ static void do_cmd(ESPState *s)
 
 static void satn_pdma_cb(ESPState *s)
 {
-    s->do_cmd = 0;
-    if (!fifo8_is_empty(&s->cmdfifo)) {
+    if (!esp_get_tc(s) && !fifo8_is_empty(&s->cmdfifo)) {
         s->cmdfifo_cdb_offset = 1;
+        s->do_cmd = 0;
         do_cmd(s);
     }
 }
@@ -369,12 +369,9 @@ static void handle_satn(ESPState *s)
 
 static void s_without_satn_pdma_cb(ESPState *s)
 {
-    uint32_t len;
-
-    s->do_cmd = 0;
-    len = fifo8_num_used(&s->cmdfifo);
-    if (len) {
+    if (!esp_get_tc(s) && !fifo8_is_empty(&s->cmdfifo)) {
         s->cmdfifo_cdb_offset = 0;
+        s->do_cmd = 0;
         do_busid_cmd(s, 0);
     }
 }
@@ -403,8 +400,7 @@ static void handle_s_without_atn(ESPState *s)
 
 static void satn_stop_pdma_cb(ESPState *s)
 {
-    s->do_cmd = 0;
-    if (!fifo8_is_empty(&s->cmdfifo)) {
+    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;
@@ -494,6 +490,11 @@ static void do_dma_pdma_cb(ESPState *s)
     uint32_t n;
 
     if (s->do_cmd) {
+        /* Ensure we have received complete command after SATN and stop */
+        if (esp_get_tc(s) || fifo8_is_empty(&s->cmdfifo)) {
+            return;
+        }
+
         s->ti_size = 0;
         s->do_cmd = 0;
         do_cmd(s);
@@ -1213,7 +1214,6 @@ static void sysbus_esp_pdma_write(void *opaque, hwaddr addr,
 {
     SysBusESPState *sysbus = opaque;
     ESPState *s = ESP(&sysbus->esp);
-    uint32_t dmalen;
 
     trace_esp_pdma_write(size);
 
@@ -1226,10 +1226,7 @@ static void sysbus_esp_pdma_write(void *opaque, hwaddr addr,
         esp_pdma_write(s, val);
         break;
     }
-    dmalen = esp_get_tc(s);
-    if (dmalen == 0 || fifo8_num_free(&s->fifo) < 2) {
-        s->pdma_cb(s);
-    }
+    s->pdma_cb(s);
 }
 
 static uint64_t sysbus_esp_pdma_read(void *opaque, hwaddr addr,
-- 
2.20.1

[PATCH 4/5] esp: revert 75ef849696 "esp: correctly fill bus id with requested lun"

[Mark Cave-Ayland]

This commit from nearly 10 years ago no longer appears to be required and in its
current form prevents the MacOS CDROM driver from detecting the CDROM drive. The
error is caused by the MacOS CDROM driver sending this CDB without DMA:

    0x12 0x00 0x00 0x00 0x05 0x00 (INQUIRY)

This is a valid INQUIRY command, however with this logic present the 3rd byte
(0x0) is copied over the 1st byte (0x12) which silently converts the INQUIRY
command to a TEST UNIT READY command before passing it to the QEMU SCSI layer.
Since the TEST UNIT READY command has a zero length response the MacOS CDROM
driver never receives a response and assumes the CDROM is not present.

Resolve the issue by reverting the original commit which I'm fairly sure is now
obsolete so that the original MacOS CDB is passed unaltered to the QEMU SCSI
layer.

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 afb4a7f9f1..a6f7c6c1bf 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -260,9 +260,6 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
             return 0;
         }
         n = esp_fifo_pop_buf(&s->fifo, buf, dmalen);
-        if (n >= 3) {
-            buf[0] = buf[2] >> 5;
-        }
         n = MIN(fifo8_num_free(&s->cmdfifo), n);
         fifo8_push_all(&s->cmdfifo, buf, n);
     }
-- 
2.20.1

[PATCH 5/5] esp: correctly accumulate extended messages for PDMA

[Mark Cave-Ayland]

Commit 799d90d818 "esp: transition to message out phase after SATN and stop
command" added logic to correctly handle extended messages for DMA requests
but not for PDMA requests.

Apply the same logic in esp_do_dma() to do_dma_pdma_cb() so that extended
messages terminated with a PDMA request are accumulated correctly. This allows
the ESP device to respond correctly to the SDTR negotiation initiated by the
NetBSD ESP driver without causing errors and timeouts on boot.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index a6f7c6c1bf..2063bf1786 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -493,9 +493,26 @@ static void do_dma_pdma_cb(ESPState *s)
         }
 
         s->ti_size = 0;
-        s->do_cmd = 0;
-        do_cmd(s);
-        esp_lower_drq(s);
+        if ((s->rregs[ESP_RSTAT] & 7) == STAT_CD) {
+            /* No command received */
+            if (s->cmdfifo_cdb_offset == fifo8_num_used(&s->cmdfifo)) {
+                return;
+            }
+
+            /* Command has been received */
+            s->do_cmd = 0;
+            do_cmd(s);
+        } else {
+            /*
+             * Extra message out bytes received: update cmdfifo_cdb_offset
+             * and then switch to commmand phase
+             */
+            s->cmdfifo_cdb_offset = fifo8_num_used(&s->cmdfifo);
+            s->rregs[ESP_RSTAT] = STAT_TC | STAT_CD;
+            s->rregs[ESP_RSEQ] = SEQ_CD;
+            s->rregs[ESP_RINTR] |= INTR_BS;
+            esp_raise_irq(s);
+        }
         return;
     }
 
-- 
2.20.1

Re: [PATCH 0/5] esp: fixes for MacOS toolbox ROM

[Mark Cave-Ayland]

On 19/05/2021 11:07, Mark Cave-Ayland wrote:

> This patchset contains more ESP fixes from my attempts to boot MacOS under
> the QEMU q800 machine (along with a related NetBSD fix).
> 
> With these patches it is possible for the MacOS toolbox ROM and MacOS drivers
> to detect and access SCSI drives and CDROMs during the MacOS boot process.
> 
> This patchset has been tested on top of the ESP fix series posted yesterday
> (see https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg05763.html) with
> the extended set of ESP test images without noticing any regressions.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> [q800-macos-upstream patchset series: 1]
> 
> 
> Mark Cave-Ayland (5):
>    esp: allow non-DMA callback in esp_transfer_data() initial transfer
>    esp: handle non-DMA transfers from the target one byte at a time
>    esp: ensure PDMA write transfers are flushed from the FIFO to the
>      target immediately
>    esp: revert 75ef849696 "esp: correctly fill bus id with requested lun"
>    esp: correctly accumulate extended messages for PDMA
> 
>   hw/scsi/esp.c | 137 ++++++++++++++++++++++++++++++--------------------
>   1 file changed, 83 insertions(+), 54 deletions(-)

Ping? I'd be particularly interested if anyone could clarify the history around the 
code removed by patch 4...


ATB,

Mark.

Re: [PATCH 0/5] esp: fixes for MacOS toolbox ROM

[Mark Cave-Ayland]

On 28/05/2021 08:11, Mark Cave-Ayland wrote:

> On 19/05/2021 11:07, Mark Cave-Ayland wrote:
> 
>> This patchset contains more ESP fixes from my attempts to boot MacOS under
>> the QEMU q800 machine (along with a related NetBSD fix).
>>
>> With these patches it is possible for the MacOS toolbox ROM and MacOS drivers
>> to detect and access SCSI drives and CDROMs during the MacOS boot process.
>>
>> This patchset has been tested on top of the ESP fix series posted yesterday
>> (see https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg05763.html) with
>> the extended set of ESP test images without noticing any regressions.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>> [q800-macos-upstream patchset series: 1]
>>
>>
>> Mark Cave-Ayland (5):
>>    esp: allow non-DMA callback in esp_transfer_data() initial transfer
>>    esp: handle non-DMA transfers from the target one byte at a time
>>    esp: ensure PDMA write transfers are flushed from the FIFO to the
>>      target immediately
>>    esp: revert 75ef849696 "esp: correctly fill bus id with requested lun"
>>    esp: correctly accumulate extended messages for PDMA
>>
>>   hw/scsi/esp.c | 137 ++++++++++++++++++++++++++++++--------------------
>>   1 file changed, 83 insertions(+), 54 deletions(-)
> 
> Ping? I'd be particularly interested if anyone could clarify the history around the 
> code removed by patch 4...

Ping again?


ATB,

Mark.

Re: [PATCH 4/5] esp: revert 75ef849696 "esp: correctly fill bus id with requested lun"

[Paolo Bonzini]

On 19/05/21 12:08, Mark Cave-Ayland wrote:
> This commit from nearly 10 years ago no longer appears to be required and in its
> current form prevents the MacOS CDROM driver from detecting the CDROM drive. The
> error is caused by the MacOS CDROM driver sending this CDB without DMA:
> 
>      0x12 0x00 0x00 0x00 0x05 0x00 (INQUIRY)
> 
> This is a valid INQUIRY command, however with this logic present the 3rd byte
> (0x0) is copied over the 1st byte (0x12) which silently converts the INQUIRY
> command to a TEST UNIT READY command before passing it to the QEMU SCSI layer.
> Since the TEST UNIT READY command has a zero length response the MacOS CDROM
> driver never receives a response and assumes the CDROM is not present.
> 
> Resolve the issue by reverting the original commit which I'm fairly sure is now
> obsolete so that the original MacOS CDB is passed unaltered to the QEMU SCSI
> layer.
> 
> 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 afb4a7f9f1..a6f7c6c1bf 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -260,9 +260,6 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
>               return 0;
>           }
>           n = esp_fifo_pop_buf(&s->fifo, buf, dmalen);
> -        if (n >= 3) {
> -            buf[0] = buf[2] >> 5;
> -        }
>           n = MIN(fifo8_num_free(&s->cmdfifo), n);
>           fifo8_push_all(&s->cmdfifo, buf, n);
>       }
> 

This is probably related to S vs SATN, and the bug is that it's doing it 
even in the S case where cmdfifo_cdb_offset is zero.  You can see that 
the flow is

bus[0] = bus[2] >> 5;
    -> busid = esp_fifo_pop(&s->cmdfifo);    [do_cmd]
         -> lun = busid & 7;                 [do_busid_cmd]

However it does seem bogus.

Perhaps the "S without ATN" cases (handle_s_without_atn, 
s_without_satn_pdma_cb) should do something like

    busid = (busid & ~7) | (buf[2] >> 5);

before calling do_busid_cmd?

Paolo

Re: [PATCH 0/5] esp: fixes for MacOS toolbox ROM

[Paolo Bonzini]

On 19/05/21 12:07, Mark Cave-Ayland wrote:
> This patchset contains more ESP fixes from my attempts to boot MacOS under
> the QEMU q800 machine (along with a related NetBSD fix).
> 
> With these patches it is possible for the MacOS toolbox ROM and MacOS drivers
> to detect and access SCSI drives and CDROMs during the MacOS boot process.
> 
> This patchset has been tested on top of the ESP fix series posted yesterday
> (see https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg05763.html) with
> the extended set of ESP test images without noticing any regressions.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> [q800-macos-upstream patchset series: 1]
> 
> 
> Mark Cave-Ayland (5):
>    esp: allow non-DMA callback in esp_transfer_data() initial transfer
>    esp: handle non-DMA transfers from the target one byte at a time
>    esp: ensure PDMA write transfers are flushed from the FIFO to the
>      target immediately
>    esp: revert 75ef849696 "esp: correctly fill bus id with requested lun"
>    esp: correctly accumulate extended messages for PDMA
> 
>   hw/scsi/esp.c | 137 ++++++++++++++++++++++++++++++--------------------
>   1 file changed, 83 insertions(+), 54 deletions(-)
> 

Queued except for patch 4 (waiting for your comments and possibly a v2), 
thanks.

Paolo

Re: [PATCH 4/5] esp: revert 75ef849696 "esp: correctly fill bus id with requested lun"

[Mark Cave-Ayland]

On 09/06/2021 13:13, Paolo Bonzini wrote:

> On 19/05/21 12:08, Mark Cave-Ayland wrote:
>> This commit from nearly 10 years ago no longer appears to be required and in its
>> current form prevents the MacOS CDROM driver from detecting the CDROM drive. The
>> error is caused by the MacOS CDROM driver sending this CDB without DMA:
>>
>>      0x12 0x00 0x00 0x00 0x05 0x00 (INQUIRY)
>>
>> This is a valid INQUIRY command, however with this logic present the 3rd byte
>> (0x0) is copied over the 1st byte (0x12) which silently converts the INQUIRY
>> command to a TEST UNIT READY command before passing it to the QEMU SCSI layer.
>> Since the TEST UNIT READY command has a zero length response the MacOS CDROM
>> driver never receives a response and assumes the CDROM is not present.
>>
>> Resolve the issue by reverting the original commit which I'm fairly sure is now
>> obsolete so that the original MacOS CDB is passed unaltered to the QEMU SCSI
>> layer.
>>
>> 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 afb4a7f9f1..a6f7c6c1bf 100644
>> --- a/hw/scsi/esp.c
>> +++ b/hw/scsi/esp.c
>> @@ -260,9 +260,6 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
>>               return 0;
>>           }
>>           n = esp_fifo_pop_buf(&s->fifo, buf, dmalen);
>> -        if (n >= 3) {
>> -            buf[0] = buf[2] >> 5;
>> -        }
>>           n = MIN(fifo8_num_free(&s->cmdfifo), n);
>>           fifo8_push_all(&s->cmdfifo, buf, n);
>>       }
>>
> 
> This is probably related to S vs SATN, and the bug is that it's doing it even in the 
> S case where cmdfifo_cdb_offset is zero.  You can see that the flow is
> 
> bus[0] = bus[2] >> 5;
>     -> busid = esp_fifo_pop(&s->cmdfifo);    [do_cmd]
>          -> lun = busid & 7;                 [do_busid_cmd]
> 
> However it does seem bogus.
> 
> Perhaps the "S without ATN" cases (handle_s_without_atn, s_without_satn_pdma_cb) 
> should do something like
> 
>     busid = (busid & ~7) | (buf[2] >> 5);
> 
> before calling do_busid_cmd?

That is entirely possible. For reference here is the sequence for the INQUIRY command 
with -trace 'esp*' -trace 'scsi*' without this patch applied:

esp_mem_writeb reg[3]: 0x41 -> 0x01
esp_mem_writeb_cmd_flush Flush FIFO (0x01)
esp_mem_writeb reg[7]: 0x00 -> 0x00
esp_mem_readb reg[12]: 0x04
esp_mem_writeb reg[12]: 0x04 -> 0x04
esp_mem_writeb reg[3]: 0x01 -> 0x44
esp_mem_writeb_cmd_ensel Enable selection (0x44)
esp_mem_readb reg[4]: 0x00
esp_mem_writeb reg[4]: 0x04 -> 0x03
esp_mem_readb reg[4]: 0x00
esp_mem_writeb reg[2]: 0x00 -> 0x12
esp_mem_readb reg[4]: 0x00
esp_mem_writeb reg[2]: 0x12 -> 0x00
esp_mem_readb reg[4]: 0x00
esp_mem_writeb reg[2]: 0x00 -> 0x00
esp_mem_readb reg[4]: 0x00
esp_mem_writeb reg[2]: 0x00 -> 0x00
esp_mem_readb reg[4]: 0x00
esp_mem_writeb reg[2]: 0x00 -> 0x05
esp_mem_readb reg[4]: 0x00
esp_mem_writeb reg[2]: 0x05 -> 0x00
esp_mem_readb reg[4]: 0x00
esp_mem_writeb reg[3]: 0x44 -> 0x41
esp_mem_writeb_cmd_sel Select without ATN (0x41)
esp_get_cmd len 6 target 3
esp_do_busid_cmd busid 0x0
scsi_req_parsed target 3 lun 0 tag 0 command 0 dir 0 length 0
scsi_req_parsed_lba target 3 lun 0 tag 0 command 0 lba 0
scsi_req_alloc target 3 lun 0 tag 0
scsi_disk_new_request Command: lun=0 tag=0x0 data= 0x00 0x00 0x00 0x00 0x05 0x00
scsi_test_unit_ready target 3 lun 0 tag 0
scsi_req_dequeue target 3 lun 0 tag 0
esp_command_complete SCSI Command complete

According to the datasheet the differences between S and SATN are:

S: sends 6, 10, or 12 byte CDB

SATN: sends 1 message phase byte followed by a 6, 10 or 12 byte CDB giving a transfer 
total of 7, 11 or 13 bytes

Due to the absence of the IDENTIFY byte in the S case I'm guessing from the patch 
that the LUN is in encoded in buf[1] (the top bits being "Reserved" according to my 
copy of the specification). The patch below passes a trivial test with MacOS - does 
it look right to you? If so, I'll run it around all my test images and submit a new 
patch if it doesn't cause any regressions.


diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index bfdb94292b..2163becb52 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -260,6 +260,9 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
              return 0;
          }
          n = esp_fifo_pop_buf(&s->fifo, buf, dmalen);
+        //if (n >= 3) {
+        //    buf[0] = buf[2] >> 5;
+        //}
          n = MIN(fifo8_num_free(&s->cmdfifo), n);
          fifo8_push_all(&s->cmdfifo, buf, n);
      }
@@ -366,16 +369,20 @@ static void handle_satn(ESPState *s)

  static void s_without_satn_pdma_cb(ESPState *s)
  {
+    uint8_t busid;
+
      if (!esp_get_tc(s) && !fifo8_is_empty(&s->cmdfifo)) {
          s->cmdfifo_cdb_offset = 0;
          s->do_cmd = 0;
-        do_busid_cmd(s, 0);
+        busid = s->cmdfifo.data[1] >> 5;
+        do_busid_cmd(s, busid);
      }
  }

  static void handle_s_without_atn(ESPState *s)
  {
      int32_t cmdlen;
+    uint8_t busid;

      if (s->dma && !s->dma_enabled) {
          s->dma_cb = handle_s_without_atn;
@@ -386,7 +393,8 @@ static void handle_s_without_atn(ESPState *s)
      if (cmdlen > 0) {
          s->cmdfifo_cdb_offset = 0;
          s->do_cmd = 0;
-        do_busid_cmd(s, 0);
+        busid = s->cmdfifo.data[1] >> 5;
+        do_busid_cmd(s, busid);
      } else if (cmdlen == 0) {
          s->do_cmd = 1;
          /* Target present, but no cmd yet - switch to command phase */


ATB,

Mark.

Re: [PATCH 4/5] esp: revert 75ef849696 "esp: correctly fill bus id with requested lun"

[Paolo Bonzini]

On 09/06/21 17:31, Mark Cave-Ayland wrote:
> Due to the absence of the IDENTIFY byte in the S case I'm guessing from 
> the patch that the LUN is in encoded in buf[1] (the top bits being 
> "Reserved" according to my copy of the specification).

They used to be the LUN many years ago.

>   static void s_without_satn_pdma_cb(ESPState *s)
>   {
> +    uint8_t busid;
> +
>       if (!esp_get_tc(s) && !fifo8_is_empty(&s->cmdfifo)) {
>           s->cmdfifo_cdb_offset = 0;
>           s->do_cmd = 0;
> -        do_busid_cmd(s, 0);
> +        busid = s->cmdfifo.data[1] >> 5;
> +        do_busid_cmd(s, busid);
>       }
>   }

Considering how obsolete the LUN-in-CDB case is (and now that I actually 
understand that the first byte is a message-phase byte), it is probably 
be more correct to keep the previous busid: with no message phase, the 
previously-selected LUN would be addressed.  I can send a patch for 
this, but it's orthogonal to this one so I queued it as well.

Paolo

Re: [PATCH 4/5] esp: revert 75ef849696 "esp: correctly fill bus id with requested lun"

[Mark Cave-Ayland]

On 11/06/2021 12:38, Paolo Bonzini wrote:

> On 09/06/21 17:31, Mark Cave-Ayland wrote:
>> Due to the absence of the IDENTIFY byte in the S case I'm guessing from the patch 
>> that the LUN is in encoded in buf[1] (the top bits being "Reserved" according to my 
>> copy of the specification).
> 
> They used to be the LUN many years ago.

Got it :)

>>   static void s_without_satn_pdma_cb(ESPState *s)
>>   {
>> +    uint8_t busid;
>> +
>>       if (!esp_get_tc(s) && !fifo8_is_empty(&s->cmdfifo)) {
>>           s->cmdfifo_cdb_offset = 0;
>>           s->do_cmd = 0;
>> -        do_busid_cmd(s, 0);
>> +        busid = s->cmdfifo.data[1] >> 5;
>> +        do_busid_cmd(s, busid);
>>       }
>>   }
> 
> Considering how obsolete the LUN-in-CDB case is (and now that I actually understand 
> that the first byte is a message-phase byte), it is probably be more correct to keep 
> the previous busid: with no message phase, the previously-selected LUN would be 
> addressed.  I can send a patch for this, but it's orthogonal to this one so I queued 
> it as well.

That was going to be my next question - how do you determine the LUN if there is no 
message phase byte? But that makes sense to me. I don't think that I have a 
LUN-in-CDB example in my set of ESP test images, but I'm happy to run them against 
your patch and make sure there are no regressions.


ATB,

Mark.