[v2] scsi: esp: remove handling of SATN/STOP

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(-)

+-- On Fri, 17 Jun 2016, Paolo Bonzini wrote --+
| 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>
| ---

Looks good.
Reviewed-by: Prasad J Pandit <pjp@fedoraproject.org>

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

On (Fri) 17 Jun 2016 [11:55:48], Paolo Bonzini wrote:
> 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>

Thanks, the vmstate bits are fine.

		Amit

On 17/06/16 10:55, Paolo Bonzini wrote:

> 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;
> 

Unforunately this causes regressions on a few of my SPARC32 images:
NextStep, Solaris 1.1.2, NetBSD and Debian Etch all hang whilst
enumerating the SCSI bus with this patch applied.


ATB,

Mark.

On 17/06/2016 15:13, Mark Cave-Ayland wrote:
> Unforunately this causes regressions on a few of my SPARC32 images:
> NextStep, Solaris 1.1.2, NetBSD and Debian Etch all hang whilst
> enumerating the SCSI bus with this patch applied.

Ok, I'll either fix migration (adding a subsection) or bump the version
number.

Thanks,

Paolo

On 17/06/16 14:35, Paolo Bonzini wrote:

> On 17/06/2016 15:13, Mark Cave-Ayland wrote:
>> Unforunately this causes regressions on a few of my SPARC32 images:
>> NextStep, Solaris 1.1.2, NetBSD and Debian Etch all hang whilst
>> enumerating the SCSI bus with this patch applied.
> 
> Ok, I'll either fix migration (adding a subsection) or bump the version
> number.

Hi Paolo,

Just to clarify that these hangs aren't migration related, they occur
simply trying to boot the above images from ISOs with the patch applied.


ATB,

Mark.

On 17/06/2016 15:41, Mark Cave-Ayland wrote:
>> > 
>> > Ok, I'll either fix migration (adding a subsection) or bump the version
>> > number.
> Hi Paolo,
> 
> Just to clarify that these hangs aren't migration related, they occur
> simply trying to boot the above images from ISOs with the patch applied.

Yes---fixing migration or bumping the version number is an alternative
to this patch (which I did because I didn't want to fix migration :)).

Paolo