[Qemu-devel] [PATCH v2] scsi: esp: remove handling of SATN/STOP

* [Qemu-devel] [PATCH v2] scsi: esp: remove handling of SATN/STOP
@ 2016-06-17  9:55 Paolo Bonzini
  2016-06-17 11:08 ` P J P
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Paolo Bonzini @ 2016-06-17  9:55 UTC (permalink / raw)
  To: qemu-devel; +Cc: amit.shah, Hervé Poussineau, ppandit

The implementation of SATN/STOP is completely busted.  The idea
would be that the next DMA read is for a SCSI message and after
that the adapter would transition to the command phase.

The recent fix to SATN/STOP broke migration, which is one more
reason to drop SATN/STOP handling completely.  It is only used
in practice to send 3-byte messages (target number + tag type
+ tag number) for tagged command queuing on adapters that lack
the SATN3 command, and we do not advertise support for TCQ.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/esp.c         | 58 +++++++++------------------------------------------
 include/hw/scsi/esp.h |  4 ----
 2 files changed, 10 insertions(+), 52 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index baa0a2c..d74f8d6 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -192,23 +192,6 @@ static void handle_s_without_atn(ESPState *s)
     }
 }
 
-static void handle_satn_stop(ESPState *s)
-{
-    if (s->dma && !s->dma_enabled) {
-        s->dma_cb = handle_satn_stop;
-        return;
-    }
-    s->cmdlen = get_cmd(s, s->cmdbuf, sizeof(s->cmdbuf));
-    if (s->cmdlen) {
-        trace_esp_handle_satn_stop(s->cmdlen);
-        s->do_cmd = 1;
-        s->rregs[ESP_RSTAT] = STAT_TC | STAT_CD;
-        s->rregs[ESP_RINTR] = INTR_BS | INTR_FC;
-        s->rregs[ESP_RSEQ] = SEQ_CD;
-        esp_raise_irq(s);
-    }
-}
-
 static void write_response(ESPState *s)
 {
     trace_esp_write_response(s->status);
@@ -246,13 +229,6 @@ static void esp_do_dma(ESPState *s)
     int to_device;
 
     len = s->dma_left;
-    if (s->do_cmd) {
-        trace_esp_do_dma(s->cmdlen, len);
-        assert (s->cmdlen <= sizeof(s->cmdbuf) &&
-                len <= sizeof(s->cmdbuf) - s->cmdlen);
-        s->dma_memory_read(s->dma_opaque, &s->cmdbuf[s->cmdlen], len);
-        return;
-    }
     if (s->async_len == 0) {
         /* Defer until data is available.  */
         return;
@@ -316,7 +292,6 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
 {
     ESPState *s = req->hba_private;
 
-    assert(!s->do_cmd);
     trace_esp_transfer_data(s->dma_left, s->ti_size);
     s->async_len = len;
     s->async_buf = scsi_req_get_buf(req);
@@ -346,9 +321,7 @@ static void handle_ti(ESPState *s)
     }
     s->dma_counter = dmalen;
 
-    if (s->do_cmd)
-        minlen = (dmalen < ESP_CMDBUF_SZ) ? dmalen : ESP_CMDBUF_SZ;
-    else if (s->ti_size < 0)
+    if (s->ti_size < 0)
         minlen = (dmalen < -s->ti_size) ? dmalen : -s->ti_size;
     else
         minlen = (dmalen < s->ti_size) ? dmalen : s->ti_size;
@@ -358,13 +331,6 @@ static void handle_ti(ESPState *s)
         s->rregs[ESP_RSTAT] &= ~STAT_TC;
         esp_do_dma(s);
     }
-    if (s->do_cmd) {
-        trace_esp_handle_ti_cmd(s->cmdlen);
-        s->ti_size = 0;
-        s->cmdlen = 0;
-        s->do_cmd = 0;
-        do_cmd(s, s->cmdbuf);
-    }
 }
 
 void esp_hard_reset(ESPState *s)
@@ -376,7 +342,6 @@ void esp_hard_reset(ESPState *s)
     s->ti_rptr = 0;
     s->ti_wptr = 0;
     s->dma = 0;
-    s->do_cmd = 0;
     s->dma_cb = NULL;
 
     s->rregs[ESP_CFG1] = 7;
@@ -450,13 +415,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 (s->cmdlen < ESP_CMDBUF_SZ) {
-                s->cmdbuf[s->cmdlen++] = val & 0xff;
-            } else {
-                trace_esp_error_fifo_overrun();
-            }
-        } else if (s->ti_wptr == TI_BUFSZ - 1) {
+        if (s->ti_wptr == TI_BUFSZ - 1) {
             trace_esp_error_fifo_overrun();
         } else {
             s->ti_size++;
@@ -534,8 +493,7 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val)
             break;
         case CMD_SELATNS:
             trace_esp_mem_writeb_cmd_selatns(val);
-            handle_satn_stop(s);
-            break;
+            goto unhandled;
         case CMD_ENSEL:
             trace_esp_mem_writeb_cmd_ensel(val);
             s->rregs[ESP_RINTR] = 0;
@@ -546,6 +504,7 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val)
             esp_raise_irq(s);
             break;
         default:
+        unhandled:
             trace_esp_error_unhandled_command(val);
             break;
         }
@@ -585,9 +544,12 @@ const VMStateDescription vmstate_esp = {
         VMSTATE_BUFFER(ti_buf, ESPState),
         VMSTATE_UINT32(status, ESPState),
         VMSTATE_UINT32(dma, ESPState),
-        VMSTATE_BUFFER(cmdbuf, ESPState),
-        VMSTATE_UINT32(cmdlen, ESPState),
-        VMSTATE_UINT32(do_cmd, ESPState),
+        /* Used to be cmdbuf, cmdlen, do_cmd, but the implementation
+         * of "Select with ATN and stop" was totally busted.
+         */
+        VMSTATE_UNUSED(16),
+        VMSTATE_UNUSED(4),
+        VMSTATE_UNUSED(4),
         VMSTATE_UINT32(dma_left, ESPState),
         VMSTATE_END_OF_LIST()
     }
diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
index d2c4886..61cb8b4 100644
--- a/include/hw/scsi/esp.h
+++ b/include/hw/scsi/esp.h
@@ -14,7 +14,6 @@ void esp_init(hwaddr espaddr, int it_shift,
 
 #define ESP_REGS 16
 #define TI_BUFSZ 16
-#define ESP_CMDBUF_SZ 32
 
 typedef struct ESPState ESPState;
 
@@ -32,9 +31,6 @@ struct ESPState {
     SCSIBus bus;
     SCSIDevice *current_dev;
     SCSIRequest *current_req;
-    uint8_t cmdbuf[ESP_CMDBUF_SZ];
-    uint32_t cmdlen;
-    uint32_t do_cmd;
 
     /* The amount of data left in the current DMA transfer.  */
     uint32_t dma_left;
-- 
2.5.5

^ permalink raw reply related	[flat|nested] 7+ messages in thread