hw/scsi/esp.c | 39 +++++++++++++++++++++++---------------- hw/scsi/trace-events | 3 ++- include/hw/scsi/esp.h | 1 + 3 files changed, 26 insertions(+), 17 deletions(-)
The LUN is selected with an IDENTIFY message, and persists
until the next message out phase. Instead of passing it to
do_busid_cmd, store it in ESPState. Because do_cmd can simply
skip the message out phase if cmdfifo_cdb_offset is zero, it
can now be used for the S without ATN cases as well.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi/esp.c | 39 +++++++++++++++++++++++----------------
hw/scsi/trace-events | 3 ++-
include/hw/scsi/esp.h | 1 +
3 files changed, 26 insertions(+), 17 deletions(-)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 3e6f4094fc..1f1c02de79 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -221,7 +221,7 @@ static int esp_select(ESPState *s)
/*
* Note that we deliberately don't raise the IRQ here: this will be done
- * either in do_busid_cmd() for DATA OUT transfers or by the deferred
+ * either in do_command_phase() for DATA OUT transfers or by the deferred
* IRQ mechanism in esp_transfer_data() for DATA IN transfers
*/
s->rregs[ESP_RINTR] |= INTR_FC;
@@ -272,24 +272,22 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
return dmalen;
}
-static void do_busid_cmd(ESPState *s, uint8_t busid)
+static void do_command_phase(ESPState *s)
{
uint32_t cmdlen;
int32_t datalen;
- int lun;
SCSIDevice *current_lun;
uint8_t buf[ESP_CMDFIFO_SZ];
- trace_esp_do_busid_cmd(busid);
- lun = busid & 7;
+ trace_esp_do_command_phase(s->lun);
cmdlen = fifo8_num_used(&s->cmdfifo);
if (!cmdlen || !s->current_dev) {
return;
}
esp_fifo_pop_buf(&s->cmdfifo, buf, cmdlen);
- current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, lun);
- s->current_req = scsi_req_new(current_lun, 0, lun, buf, s);
+ current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, s->lun);
+ s->current_req = scsi_req_new(current_lun, 0, s->lun, buf, s);
datalen = scsi_req_enqueue(s->current_req);
s->ti_size = datalen;
fifo8_reset(&s->cmdfifo);
@@ -316,21 +314,29 @@ static void do_busid_cmd(ESPState *s, uint8_t busid)
}
}
-static void do_cmd(ESPState *s)
+static void do_message_phase(ESPState *s)
{
- uint8_t busid = esp_fifo_pop(&s->cmdfifo);
- int len;
+ if (s->cmdfifo_cdb_offset) {
+ uint8_t message = esp_fifo_pop(&s->cmdfifo);
- s->cmdfifo_cdb_offset--;
+ trace_esp_do_identify(message);
+ s->lun = message & 7;
+ s->cmdfifo_cdb_offset--;
+ }
/* Ignore extended messages for now */
if (s->cmdfifo_cdb_offset) {
- len = MIN(s->cmdfifo_cdb_offset, fifo8_num_used(&s->cmdfifo));
+ int len = MIN(s->cmdfifo_cdb_offset, fifo8_num_used(&s->cmdfifo));
esp_fifo_pop_buf(&s->cmdfifo, NULL, len);
s->cmdfifo_cdb_offset = 0;
}
+}
- do_busid_cmd(s, busid);
+static void do_cmd(ESPState *s)
+{
+ do_message_phase(s);
+ assert(s->cmdfifo_cdb_offset == 0);
+ do_command_phase(s);
}
static void satn_pdma_cb(ESPState *s)
@@ -369,7 +375,7 @@ static void s_without_satn_pdma_cb(ESPState *s)
if (!esp_get_tc(s) && !fifo8_is_empty(&s->cmdfifo)) {
s->cmdfifo_cdb_offset = 0;
s->do_cmd = 0;
- do_busid_cmd(s, 0);
+ do_cmd(s);
}
}
@@ -386,7 +392,7 @@ static void handle_s_without_atn(ESPState *s)
if (cmdlen > 0) {
s->cmdfifo_cdb_offset = 0;
s->do_cmd = 0;
- do_busid_cmd(s, 0);
+ do_cmd(s);
} else if (cmdlen == 0) {
s->do_cmd = 1;
/* Target present, but no cmd yet - switch to command phase */
@@ -1168,7 +1174,7 @@ static int esp_post_load(void *opaque, int version_id)
const VMStateDescription vmstate_esp = {
.name = "esp",
- .version_id = 5,
+ .version_id = 6,
.minimum_version_id = 3,
.post_load = esp_post_load,
.fields = (VMStateField[]) {
@@ -1197,6 +1203,7 @@ const VMStateDescription vmstate_esp = {
VMSTATE_FIFO8_TEST(fifo, ESPState, esp_is_version_5),
VMSTATE_FIFO8_TEST(cmdfifo, ESPState, esp_is_version_5),
VMSTATE_UINT8_TEST(ti_cmd, ESPState, esp_is_version_5),
+ VMSTATE_UINT8_V(lun, ESPState, 6),
VMSTATE_END_OF_LIST()
},
};
diff --git a/hw/scsi/trace-events b/hw/scsi/trace-events
index 1a27e141ae..92d5b40f89 100644
--- a/hw/scsi/trace-events
+++ b/hw/scsi/trace-events
@@ -166,7 +166,8 @@ esp_dma_disable(void) "Lower enable"
esp_pdma_read(int size) "pDMA read %u bytes"
esp_pdma_write(int size) "pDMA write %u bytes"
esp_get_cmd(uint32_t dmalen, int target) "len %d target %d"
-esp_do_busid_cmd(uint8_t busid) "busid 0x%x"
+esp_do_command_phase(uint8_t busid) "busid 0x%x"
+esp_do_identify(uint8_t byte) "0x%x"
esp_handle_satn_stop(uint32_t cmdlen) "cmdlen %d"
esp_write_response(uint32_t status) "Transfer status (status=%d)"
esp_do_dma(uint32_t cmdlen, uint32_t len) "command len %d + %d"
diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
index aada3680b7..b1ec27612f 100644
--- a/include/hw/scsi/esp.h
+++ b/include/hw/scsi/esp.h
@@ -37,6 +37,7 @@ struct ESPState {
SCSIRequest *current_req;
Fifo8 cmdfifo;
uint8_t cmdfifo_cdb_offset;
+ uint8_t lun;
uint32_t do_cmd;
bool data_in_ready;
--
2.31.1
On 11/06/2021 12:57, Paolo Bonzini wrote:
> The LUN is selected with an IDENTIFY message, and persists
> until the next message out phase. Instead of passing it to
> do_busid_cmd, store it in ESPState. Because do_cmd can simply
> skip the message out phase if cmdfifo_cdb_offset is zero, it
> can now be used for the S without ATN cases as well.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/scsi/esp.c | 39 +++++++++++++++++++++++----------------
> hw/scsi/trace-events | 3 ++-
> include/hw/scsi/esp.h | 1 +
> 3 files changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index 3e6f4094fc..1f1c02de79 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -221,7 +221,7 @@ static int esp_select(ESPState *s)
>
> /*
> * Note that we deliberately don't raise the IRQ here: this will be done
> - * either in do_busid_cmd() for DATA OUT transfers or by the deferred
> + * either in do_command_phase() for DATA OUT transfers or by the deferred
> * IRQ mechanism in esp_transfer_data() for DATA IN transfers
> */
> s->rregs[ESP_RINTR] |= INTR_FC;
> @@ -272,24 +272,22 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
> return dmalen;
> }
>
> -static void do_busid_cmd(ESPState *s, uint8_t busid)
> +static void do_command_phase(ESPState *s)
> {
> uint32_t cmdlen;
> int32_t datalen;
> - int lun;
> SCSIDevice *current_lun;
> uint8_t buf[ESP_CMDFIFO_SZ];
>
> - trace_esp_do_busid_cmd(busid);
> - lun = busid & 7;
> + trace_esp_do_command_phase(s->lun);
> cmdlen = fifo8_num_used(&s->cmdfifo);
> if (!cmdlen || !s->current_dev) {
> return;
> }
> esp_fifo_pop_buf(&s->cmdfifo, buf, cmdlen);
>
> - current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, lun);
> - s->current_req = scsi_req_new(current_lun, 0, lun, buf, s);
> + current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, s->lun);
> + s->current_req = scsi_req_new(current_lun, 0, s->lun, buf, s);
> datalen = scsi_req_enqueue(s->current_req);
> s->ti_size = datalen;
> fifo8_reset(&s->cmdfifo);
> @@ -316,21 +314,29 @@ static void do_busid_cmd(ESPState *s, uint8_t busid)
> }
> }
>
> -static void do_cmd(ESPState *s)
> +static void do_message_phase(ESPState *s)
> {
> - uint8_t busid = esp_fifo_pop(&s->cmdfifo);
> - int len;
> + if (s->cmdfifo_cdb_offset) {
> + uint8_t message = esp_fifo_pop(&s->cmdfifo);
>
> - s->cmdfifo_cdb_offset--;
> + trace_esp_do_identify(message);
> + s->lun = message & 7;
> + s->cmdfifo_cdb_offset--;
> + }
>
> /* Ignore extended messages for now */
> if (s->cmdfifo_cdb_offset) {
> - len = MIN(s->cmdfifo_cdb_offset, fifo8_num_used(&s->cmdfifo));
> + int len = MIN(s->cmdfifo_cdb_offset, fifo8_num_used(&s->cmdfifo));
> esp_fifo_pop_buf(&s->cmdfifo, NULL, len);
> s->cmdfifo_cdb_offset = 0;
> }
> +}
>
> - do_busid_cmd(s, busid);
> +static void do_cmd(ESPState *s)
> +{
> + do_message_phase(s);
> + assert(s->cmdfifo_cdb_offset == 0);
> + do_command_phase(s);
> }
>
> static void satn_pdma_cb(ESPState *s)
> @@ -369,7 +375,7 @@ static void s_without_satn_pdma_cb(ESPState *s)
> if (!esp_get_tc(s) && !fifo8_is_empty(&s->cmdfifo)) {
> s->cmdfifo_cdb_offset = 0;
> s->do_cmd = 0;
> - do_busid_cmd(s, 0);
> + do_cmd(s);
> }
> }
>
> @@ -386,7 +392,7 @@ static void handle_s_without_atn(ESPState *s)
> if (cmdlen > 0) {
> s->cmdfifo_cdb_offset = 0;
> s->do_cmd = 0;
> - do_busid_cmd(s, 0);
> + do_cmd(s);
> } else if (cmdlen == 0) {
> s->do_cmd = 1;
> /* Target present, but no cmd yet - switch to command phase */
> @@ -1168,7 +1174,7 @@ static int esp_post_load(void *opaque, int version_id)
>
> const VMStateDescription vmstate_esp = {
> .name = "esp",
> - .version_id = 5,
> + .version_id = 6,
> .minimum_version_id = 3,
> .post_load = esp_post_load,
> .fields = (VMStateField[]) {
> @@ -1197,6 +1203,7 @@ const VMStateDescription vmstate_esp = {
> VMSTATE_FIFO8_TEST(fifo, ESPState, esp_is_version_5),
> VMSTATE_FIFO8_TEST(cmdfifo, ESPState, esp_is_version_5),
> VMSTATE_UINT8_TEST(ti_cmd, ESPState, esp_is_version_5),
> + VMSTATE_UINT8_V(lun, ESPState, 6),
> VMSTATE_END_OF_LIST()
> },
> };
> diff --git a/hw/scsi/trace-events b/hw/scsi/trace-events
> index 1a27e141ae..92d5b40f89 100644
> --- a/hw/scsi/trace-events
> +++ b/hw/scsi/trace-events
> @@ -166,7 +166,8 @@ esp_dma_disable(void) "Lower enable"
> esp_pdma_read(int size) "pDMA read %u bytes"
> esp_pdma_write(int size) "pDMA write %u bytes"
> esp_get_cmd(uint32_t dmalen, int target) "len %d target %d"
> -esp_do_busid_cmd(uint8_t busid) "busid 0x%x"
> +esp_do_command_phase(uint8_t busid) "busid 0x%x"
> +esp_do_identify(uint8_t byte) "0x%x"
> esp_handle_satn_stop(uint32_t cmdlen) "cmdlen %d"
> esp_write_response(uint32_t status) "Transfer status (status=%d)"
> esp_do_dma(uint32_t cmdlen, uint32_t len) "command len %d + %d"
> diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
> index aada3680b7..b1ec27612f 100644
> --- a/include/hw/scsi/esp.h
> +++ b/include/hw/scsi/esp.h
> @@ -37,6 +37,7 @@ struct ESPState {
> SCSIRequest *current_req;
> Fifo8 cmdfifo;
> uint8_t cmdfifo_cdb_offset;
> + uint8_t lun;
> uint32_t do_cmd;
>
> bool data_in_ready;
Functionally the patch passes all my boot tests, but fails when attempting to load a
migration from an earlier version. The first reason is that I made a mistake in the
version check in esp_is_version_5() which prevents tested fields from appearing in
the migration stream if the VMStateDescription has a version_id > 5. This is fixed by
the patch I just posted.
Unfortunately the VMSTATE_*_V() macros don't work in ESPState because ESPState is
currently embedded in both sysbusespscsi and pciespscsi using VMSTATE_STRUCT() where
the version of the vmstate_esp VMStateDescription does not match those in the
vmstate_sysbus_esp_scsi or vmstate_esp_pci_scsi VMStateDescriptions. This is
currently handled by adding an explicit mig_version_id field containing the
vmstate_esp.version_id field and testing accordingly.
The fix is to use the same logic as esp_is_version_5() when adding the new field to
vmstate_esp. I've tested the changes below squashed into your patch, along with the
just posted fix for esp_is_version_5(), and confirmed that I can reload old
qemu-system-sparc images from 5.2 and 6.0 as well as git master.
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 39756ddd99..4737c34f63 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -1123,6 +1123,14 @@ static bool esp_is_version_5(void *opaque, int version_id)
return version_id >= 5;
}
+static bool esp_is_version_6(void *opaque, int version_id)
+{
+ ESPState *s = ESP(opaque);
+
+ version_id = MIN(version_id, s->mig_version_id);
+ return version_id >= 6;
+}
+
int esp_pre_save(void *opaque)
{
ESPState *s = ESP(object_resolve_path_component(
@@ -1189,6 +1197,7 @@ const VMStateDescription vmstate_esp = {
VMSTATE_FIFO8_TEST(fifo, ESPState, esp_is_version_5),
VMSTATE_FIFO8_TEST(cmdfifo, ESPState, esp_is_version_5),
VMSTATE_UINT8_TEST(ti_cmd, ESPState, esp_is_version_5),
+ VMSTATE_UINT8_TEST(lun, ESPState, esp_is_version_6),
VMSTATE_END_OF_LIST()
},
};
Anyhow for your patch with my esp_is_version_5() fix and the changes above:
Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
ATB,
Mark.
On 13/06/21 12:40, Mark Cave-Ayland wrote: > > Unfortunately the VMSTATE_*_V() macros don't work in ESPState because > ESPState is currently embedded in both sysbusespscsi and pciespscsi > using VMSTATE_STRUCT() where the version of the vmstate_esp > VMStateDescription does not match those in the vmstate_sysbus_esp_scsi > or vmstate_esp_pci_scsi VMStateDescriptions. This is currently handled > by adding an explicit mig_version_id field containing the > vmstate_esp.version_id field and testing accordingly. > > The fix is to use the same logic as esp_is_version_5() when adding the > new field to vmstate_esp. I've tested the changes below squashed into > your patch, along with the just posted fix for esp_is_version_5(), and > confirmed that I can reload old qemu-system-sparc images from 5.2 and > 6.0 as well as git master. Ah, ok. So I'll squash this fix, thanks! Paolo
© 2016 - 2024 Red Hat, Inc.