* Re: [PATCH] esp: store lun coming from the MESSAGE OUT phase
2021-06-11 11:57 [PATCH] esp: store lun coming from the MESSAGE OUT phase Paolo Bonzini
@ 2021-06-13 10:40 ` Mark Cave-Ayland
2021-06-14 8:16 ` Paolo Bonzini
0 siblings, 1 reply; 3+ messages in thread
From: Mark Cave-Ayland @ 2021-06-13 10:40 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
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.
^ permalink raw reply related [flat|nested] 3+ messages in thread