From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58944) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDtbB-00081N-1L for qemu-devel@nongnu.org; Fri, 17 Jun 2016 09:14:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bDtb4-0001Mk-U0 for qemu-devel@nongnu.org; Fri, 17 Jun 2016 09:14:11 -0400 Received: from chuckie.co.uk ([82.165.15.123]:53923 helo=s16892447.onlinehome-server.info) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDtb4-0001MX-Ij for qemu-devel@nongnu.org; Fri, 17 Jun 2016 09:14:06 -0400 References: <1466157348-14488-1-git-send-email-pbonzini@redhat.com> From: Mark Cave-Ayland Message-ID: <5763F769.3060403@ilande.co.uk> Date: Fri, 17 Jun 2016 14:13:13 +0100 MIME-Version: 1.0 In-Reply-To: <1466157348-14488-1-git-send-email-pbonzini@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] scsi: esp: remove handling of SATN/STOP List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , qemu-devel@nongnu.org Cc: amit.shah@redhat.com, =?UTF-8?Q?Herv=c3=a9_Poussineau?= , ppandit@redhat.com 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 > --- > 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.