[PATCH v2] esp: correctly terminate successful SCSI commands

[Mark Cave-Ayland]

An upcoming series for QEMU's ESP emulation will check that ESP commands are
only issued according to their valid states as listed in the datasheet, and
otherwise generate an illegal command interrupt.

Currently if a SCSI command is expected to return no data or transfers data
successfully, the ASC is not returned to its disconnected state. This means
that any subsequent ESP initiator commands should be rejected until the SCSI
bus transaction is terminated.

Update the ESP driver so that if a SCSI command is successful then the ICCS
and MSGACC commands are issued by the host to complete the SCSI transaction.
This ensures that the ASC is returned to the disconnected state which allows
the ESP to accept subsequent SCSI commands without generating an illegal
command interrupt.

Signed-off-by: Mark Cave-Ayland 
---
 drivers/esp.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

v2:
- Fix typo in commit message spotted by Zoltan

diff --git a/drivers/esp.c b/drivers/esp.c
index 0880ab2..b1b10f4 100644
--- a/drivers/esp.c
+++ b/drivers/esp.c
@@ -113,7 +113,8 @@ do_command(esp_private_t *esp, sd_private_t *sd, int cmdlen, int replylen)
         return status;
     }
     if (replylen == 0) {
-        return 0;
+        /* No reply expected, ICCS and MSGACC to complete */
+        goto done;
     }
     /* Target went to status phase instead of data phase? */
     if ((status & ESP_STAT_PMASK) == ESP_STATP) {
@@ -139,10 +140,18 @@ do_command(esp_private_t *esp, sd_private_t *sd, int cmdlen, int replylen)

DPRINTF("do_command_reply: status 0x%x\n", status);

-    if ((status & ESP_STAT_TCNT) != ESP_STAT_TCNT)
+    if ((status & ESP_STAT_TCNT) != ESP_STAT_TCNT) {
         return status;
-    else
-        return 0; // OK
+    }
+
+done:
+    // ICCS and MSGACC sequence to complete
+    esp->ll->regs[ESP_CMD] = ESP_CMD_ICCSEQ;
+    (void)esp->ll->regs[ESP_FDATA];  // Ignore 1st byte for now
+    (void)esp->ll->regs[ESP_FDATA];  // Ignore 2nd byte for now
+    esp->ll->regs[ESP_CMD] = ESP_CMD_MOK;
+
+    return 0; // OK
 }

// offset is in sectors

[Mark Cave-Ayland]

On 08/09/2024 13:03, Mark Cave-Ayland wrote:
> An upcoming series for QEMU's ESP emulation will check that ESP commands are
> only issued according to their valid states as listed in the datasheet, and
> otherwise generate an illegal command interrupt.
>
> Currently if a SCSI command is expected to return no data or transfers data
> successfully, the ASC is not returned to its disconnected state. This means
> that any subsequent ESP initiator commands should be rejected until the SCSI
> bus transaction is terminated.
>
> Update the ESP driver so that if a SCSI command is successful then the ICCS
> and MSGACC commands are issued by the host to complete the SCSI transaction.
> This ensures that the ASC is returned to the disconnected state which allows
> the ESP to accept subsequent SCSI commands without generating an illegal
> command interrupt.
> Signed-off-by: Mark Cave-Ayland
>
> drivers/esp.c | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)
>
> v2:
> Fix typo in commit message spotted by Zoltan
>
> diff --git a/drivers/esp.c b/drivers/esp.c
> index 0880ab2..b1b10f4 100644
> --- a/drivers/esp.c
> +++ b/drivers/esp.c
> @@ -113,7 +113,8 @@ do_command(esp_private_t *esp, sd_private_t *sd, int cmdlen, int replylen)
>           return status;
>       }
>       if (replylen == 0) {
>    return 0;
>    /* No reply expected, ICCS and MSGACC to complete */
>    goto done;
> }
> /* Target went to status phase instead of data phase? */
> if ((status & ESP_STAT_PMASK) == ESP_STATP) {
>
> @@ -139,10 +140,18 @@ do_command(esp_private_t *esp, sd_private_t *sd, int cmdlen, int replylen)
>   DPRINTF("do_command_reply: status 0x%x\n", status);
>
> if ((status & ESP_STAT_TCNT) != ESP_STAT_TCNT)
> if ((status & ESP_STAT_TCNT) != ESP_STAT_TCNT) {
>      return status;
> else
>    return 0; // OK
> }
>
> +done:
>
> // ICCS and MSGACC sequence to complete
>
> esp->ll->regs[ESP_CMD] = ESP_CMD_ICCSEQ;
>
> (void)esp->ll->regs[ESP_FDATA];  // Ignore 1st byte for now
>
> (void)esp->ll->regs[ESP_FDATA];  // Ignore 2nd byte for now
>
> esp->ll->regs[ESP_CMD] = ESP_CMD_MOK;
>
> return 0; // OK
> }
>
> // offset is in sectors

During testing I've found that this patch can still generate errors with full 
debugging enabled for some bootloaders (e.g. NeXTStep) when QEMU is updated to reject 
invalid ESP commands. This is due to the extra delays causing the ESP to transition 
to the new state *before* the asynchronous I/O callback completes.

It looks like I'll have to update the patch to poll the ESP interrupt register to 
ensure the current ESP command has finished executing before continuing with the next 
ESP command.

ATB,

Mark.