Re: [PATCH v2 18/42] esp: accumulate SCSI commands for PDMA transfers in cmdbuf instead of pdma_buf

[Laurent Vivier <laurent@vivier.eu>]

Le 09/02/2021 à 20:29, Mark Cave-Ayland a écrit :
> ESP SCSI commands are already accumulated in cmdbuf and so there is no need to
> keep a separate pdma_buf buffer. Accumulate SCSI commands for PDMA transfers in
> cmdbuf instead of pdma_buf so update cmdlen accordingly and change pdma_origin
> for PDMA transfers to CMD which allows the PDMA origin to be removed.
> 
> This commit also removes a stray memcpy() from get_cmd() which is a no-op because
> cmdlen is always zero at the start of a command.
> 
> Notionally the removal of pdma_buf from vmstate_esp_pdma also breaks migration
> compatibility for the PDMA subsection until its complete removal by the end of
> the series.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/scsi/esp.c         | 56 +++++++++++++++++++------------------------
>  include/hw/scsi/esp.h |  2 --
>  2 files changed, 25 insertions(+), 33 deletions(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index 7134c0aff4..b846f022fb 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -139,8 +139,6 @@ static void set_pdma(ESPState *s, enum pdma_origin_id origin,
>  static uint8_t *get_pdma_buf(ESPState *s)
>  {
>      switch (s->pdma_origin) {
> -    case PDMA:
> -        return s->pdma_buf;
>      case TI:
>          return s->ti_buf;
>      case CMD:
> @@ -161,14 +159,12 @@ static uint8_t esp_pdma_read(ESPState *s)
>      }
>  
>      switch (s->pdma_origin) {
> -    case PDMA:
> -        val = s->pdma_buf[s->pdma_cur++];
> -        break;
>      case TI:
>          val = s->ti_buf[s->pdma_cur++];
>          break;
>      case CMD:
> -        val = s->cmdbuf[s->pdma_cur++];
> +        val = s->cmdbuf[s->cmdlen++];
> +        s->pdma_cur++;
>          break;
>      case ASYNC:
>          val = s->async_buf[s->pdma_cur++];
> @@ -193,14 +189,12 @@ static void esp_pdma_write(ESPState *s, uint8_t val)
>      }
>  
>      switch (s->pdma_origin) {
> -    case PDMA:
> -        s->pdma_buf[s->pdma_cur++] = val;
> -        break;
>      case TI:
>          s->ti_buf[s->pdma_cur++] = val;
>          break;
>      case CMD:
> -        s->cmdbuf[s->pdma_cur++] = val;
> +        s->cmdbuf[s->cmdlen++] = val;
> +        s->pdma_cur++;
>          break;
>      case ASYNC:
>          s->async_buf[s->pdma_cur++] = val;
> @@ -256,8 +250,7 @@ static uint32_t get_cmd(ESPState *s, uint8_t *buf, uint8_t buflen)
>          if (s->dma_memory_read) {
>              s->dma_memory_read(s->dma_opaque, buf, dmalen);
>          } else {
> -            memcpy(s->pdma_buf, buf, dmalen);
> -            set_pdma(s, PDMA, 0, dmalen);
> +            set_pdma(s, CMD, 0, dmalen);
>              esp_raise_drq(s);
>              return 0;
>          }
> @@ -316,24 +309,24 @@ static void satn_pdma_cb(ESPState *s)
>      if (get_cmd_cb(s) < 0) {
>          return;
>      }
> -    if (s->pdma_cur != s->pdma_start) {
> -        do_cmd(s, get_pdma_buf(s) + s->pdma_start);
> +    s->do_cmd = 0;
> +    if (s->cmdlen) {
> +        do_cmd(s, s->cmdbuf);
>      }
>  }
>  
>  static void handle_satn(ESPState *s)
>  {
> -    uint8_t buf[32];
> -    int len;
> -
>      if (s->dma && !s->dma_enabled) {
>          s->dma_cb = handle_satn;
>          return;
>      }
>      s->pdma_cb = satn_pdma_cb;
> -    len = get_cmd(s, buf, sizeof(buf));
> -    if (len) {
> -        do_cmd(s, buf);
> +    s->cmdlen = get_cmd(s, s->cmdbuf, sizeof(s->cmdbuf));
> +    if (s->cmdlen) {
> +        do_cmd(s, s->cmdbuf);
> +    } else {
> +        s->do_cmd = 1;
>      }
>  }
>  
> @@ -342,24 +335,24 @@ static void s_without_satn_pdma_cb(ESPState *s)
>      if (get_cmd_cb(s) < 0) {
>          return;
>      }
> -    if (s->pdma_cur != s->pdma_start) {
> +    s->do_cmd = 0;
> +    if (s->cmdlen) {
>          do_busid_cmd(s, get_pdma_buf(s) + s->pdma_start, 0);
>      }
>  }
>  
>  static void handle_s_without_atn(ESPState *s)
>  {
> -    uint8_t buf[32];
> -    int len;
> -
>      if (s->dma && !s->dma_enabled) {
>          s->dma_cb = handle_s_without_atn;
>          return;
>      }
>      s->pdma_cb = s_without_satn_pdma_cb;
> -    len = get_cmd(s, buf, sizeof(buf));
> -    if (len) {
> -        do_busid_cmd(s, buf, 0);
> +    s->cmdlen = get_cmd(s, s->cmdbuf, sizeof(s->cmdbuf));
> +    if (s->cmdlen) {
> +        do_busid_cmd(s, s->cmdbuf, 0);
> +    } else {
> +        s->do_cmd = 1;
>      }
>  }
>  
> @@ -368,7 +361,7 @@ static void satn_stop_pdma_cb(ESPState *s)
>      if (get_cmd_cb(s) < 0) {
>          return;
>      }
> -    s->cmdlen = s->pdma_cur - s->pdma_start;
> +    s->do_cmd = 0;
>      if (s->cmdlen) {
>          trace_esp_handle_satn_stop(s->cmdlen);
>          s->do_cmd = 1;
> @@ -394,6 +387,8 @@ static void handle_satn_stop(ESPState *s)
>          s->rregs[ESP_RINTR] = INTR_BS | INTR_FC;
>          s->rregs[ESP_RSEQ] = SEQ_CD;
>          esp_raise_irq(s);
> +    } else {
> +        s->do_cmd = 1;
>      }
>  }
>  
> @@ -865,11 +860,10 @@ static bool esp_pdma_needed(void *opaque)
>  
>  static const VMStateDescription vmstate_esp_pdma = {
>      .name = "esp/pdma",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .needed = esp_pdma_needed,
>      .fields = (VMStateField[]) {
> -        VMSTATE_BUFFER(pdma_buf, ESPState),
>          VMSTATE_INT32(pdma_origin, ESPState),
>          VMSTATE_UINT32(pdma_len, ESPState),
>          VMSTATE_UINT32(pdma_start, ESPState),
> diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
> index 9fad320513..c323d43f70 100644
> --- a/include/hw/scsi/esp.h
> +++ b/include/hw/scsi/esp.h
> @@ -16,7 +16,6 @@ typedef void (*ESPDMAMemoryReadWriteFunc)(void *opaque, uint8_t *buf, int len);
>  typedef struct ESPState ESPState;
>  
>  enum pdma_origin_id {
> -    PDMA,
>      TI,
>      CMD,
>      ASYNC,
> @@ -57,7 +56,6 @@ struct ESPState {
>      ESPDMAMemoryReadWriteFunc dma_memory_write;
>      void *dma_opaque;
>      void (*dma_cb)(ESPState *s);
> -    uint8_t pdma_buf[32];
>      int pdma_origin;
>      uint32_t pdma_len;
>      uint32_t pdma_start;
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>