| Message ID | 1466157348-14488-1-git-send-email-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
+-- 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
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;
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(-)