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

[P J P]

+-- On Fri, 17 Jun 2016, Paolo Bonzini wrote --+
| diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
| index baa0a2c..d11151f 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(1),
|          VMSTATE_UINT32(dma_left, ESPState),
|          VMSTATE_END_OF_LIST()
|      }

  Looks okay.

| diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
| index d2c4886..8967167 100644
| --- a/include/hw/scsi/esp.h
| +++ b/include/hw/scsi/esp.h
| @@ -32,9 +32,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 macro 'ESP_CMDBUF_SZ' can be removed.

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


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F