* [Qemu-devel] [PATCH] scsi: esp: remove handling of SATN/STOP
@ 2016-06-17 8:19 Paolo Bonzini
2016-06-17 9:08 ` Amit Shah
2016-06-17 9:40 ` P J P
0 siblings, 2 replies; 4+ messages in thread
From: Paolo Bonzini @ 2016-06-17 8:19 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, Hervé Poussineau, ppandit
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 | 3 ---
2 files changed, 10 insertions(+), 51 deletions(-)
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()
}
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 amount of data left in the current DMA transfer. */
uint32_t dma_left;
--
2.5.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] scsi: esp: remove handling of SATN/STOP
2016-06-17 8:19 [Qemu-devel] [PATCH] scsi: esp: remove handling of SATN/STOP Paolo Bonzini
@ 2016-06-17 9:08 ` Amit Shah
2016-06-17 9:55 ` Paolo Bonzini
2016-06-17 9:40 ` P J P
1 sibling, 1 reply; 4+ messages in thread
From: Amit Shah @ 2016-06-17 9:08 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Hervé Poussineau, ppandit
On (Fri) 17 Jun 2016 [10:19:17], 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>
[...]
> @@ -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),
Why 1?
Amit
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] scsi: esp: remove handling of SATN/STOP
2016-06-17 8:19 [Qemu-devel] [PATCH] scsi: esp: remove handling of SATN/STOP Paolo Bonzini
2016-06-17 9:08 ` Amit Shah
@ 2016-06-17 9:40 ` P J P
1 sibling, 0 replies; 4+ messages in thread
From: P J P @ 2016-06-17 9:40 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, amit.shah, Hervé Poussineau
+-- 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] scsi: esp: remove handling of SATN/STOP
2016-06-17 9:08 ` Amit Shah
@ 2016-06-17 9:55 ` Paolo Bonzini
0 siblings, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2016-06-17 9:55 UTC (permalink / raw)
To: Amit Shah; +Cc: Hervé Poussineau, qemu-devel, ppandit
On 17/06/2016 11:08, Amit Shah wrote:
> On (Fri) 17 Jun 2016 [10:19:17], 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>
>
> [...]
>
>> @@ -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),
>
> Why 1?
Because I thought it was a bool. My mistake.
Paolo
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-06-17 9:55 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-06-17 8:19 [Qemu-devel] [PATCH] scsi: esp: remove handling of SATN/STOP Paolo Bonzini
2016-06-17 9:08 ` Amit Shah
2016-06-17 9:55 ` Paolo Bonzini
2016-06-17 9:40 ` P J P
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).