From d103d0db718218463b634616efdde6613e4e8412 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Sun, 24 Mar 2024 19:16:50 +0000 Subject: [PATCH 01/17] esp.c: move esp_fifo_pop_buf() internals to new esp_fifo8_pop_buf() function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update esp_fifo_pop_buf() to be a simple wrapper onto the new esp_fifo8_pop_buf() function. Signed-off-by: Mark Cave-Ayland Reviewed-by: Paolo Bonzini Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20240324191707.623175-2-mark.cave-ayland@ilande.co.uk> Signed-off-by: Mark Cave-Ayland --- hw/scsi/esp.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 590ff99744..1b7b118a0b 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -125,7 +125,7 @@ static uint8_t esp_fifo_pop(Fifo8 *fifo) return fifo8_pop(fifo); } -static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen) +static uint32_t esp_fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen) { const uint8_t *buf; uint32_t n, n2; @@ -155,6 +155,11 @@ static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen) return n; } +static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen) +{ + return esp_fifo8_pop_buf(fifo, dest, maxlen); +} + static uint32_t esp_get_tc(ESPState *s) { uint32_t dmalen; From f87d048705bfc6bd014edc4c19c55e9beeb4723c Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Sun, 24 Mar 2024 19:16:51 +0000 Subject: [PATCH 02/17] esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in do_command_phase() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The aim is to restrict the esp_fifo_*() functions so that they only operate on the hardware FIFO. When reading from cmdfifo in do_command_phase() use the underlying esp_fifo8_pop_buf() function directly. Signed-off-by: Mark Cave-Ayland Reviewed-by: Paolo Bonzini Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20240324191707.623175-3-mark.cave-ayland@ilande.co.uk> Signed-off-by: Mark Cave-Ayland --- hw/scsi/esp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 1b7b118a0b..ff51145da7 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -280,7 +280,7 @@ static void do_command_phase(ESPState *s) if (!cmdlen || !s->current_dev) { return; } - esp_fifo_pop_buf(&s->cmdfifo, buf, cmdlen); + esp_fifo8_pop_buf(&s->cmdfifo, buf, cmdlen); current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, s->lun); if (!current_lun) { From 2260402be14a1e1e64d479e378e583239d70de58 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Sun, 24 Mar 2024 19:16:52 +0000 Subject: [PATCH 03/17] esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in do_message_phase() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The aim is to restrict the esp_fifo_*() functions so that they only operate on the hardware FIFO. When reading from cmdfifo in do_message_phase() use the underlying esp_fifo8_pop_buf() function directly. Signed-off-by: Mark Cave-Ayland Reviewed-by: Paolo Bonzini Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20240324191707.623175-4-mark.cave-ayland@ilande.co.uk> Signed-off-by: Mark Cave-Ayland --- hw/scsi/esp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index ff51145da7..9386704a58 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -325,7 +325,7 @@ static void do_message_phase(ESPState *s) /* Ignore extended messages for now */ if (s->cmdfifo_cdb_offset) { int len = MIN(s->cmdfifo_cdb_offset, fifo8_num_used(&s->cmdfifo)); - esp_fifo_pop_buf(&s->cmdfifo, NULL, len); + esp_fifo8_pop_buf(&s->cmdfifo, NULL, len); s->cmdfifo_cdb_offset = 0; } } From 1828000b48148d6136149ede46e124aaad5faa8e Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Sun, 24 Mar 2024 19:16:53 +0000 Subject: [PATCH 04/17] esp.c: replace cmdfifo use of esp_fifo_pop() in do_message_phase() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Mark Cave-Ayland Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Paolo Bonzini Message-Id: <20240324191707.623175-5-mark.cave-ayland@ilande.co.uk> Signed-off-by: Mark Cave-Ayland --- hw/scsi/esp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 9386704a58..5b169b3720 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -315,7 +315,8 @@ static void do_command_phase(ESPState *s) static void do_message_phase(ESPState *s) { if (s->cmdfifo_cdb_offset) { - uint8_t message = esp_fifo_pop(&s->cmdfifo); + uint8_t message = fifo8_is_empty(&s->cmdfifo) ? 0 : + fifo8_pop(&s->cmdfifo); trace_esp_do_identify(message); s->lun = message & 7; From 0e7dbe29c29ec4948e0d8dce6b8ca95b571c6700 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Sun, 24 Mar 2024 19:16:54 +0000 Subject: [PATCH 05/17] esp.c: change esp_fifo_push() to take ESPState MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that all users of esp_fifo_push() operate on the main FIFO there is no need to pass the FIFO explicitly. Signed-off-by: Mark Cave-Ayland Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Paolo Bonzini Message-Id: <20240324191707.623175-6-mark.cave-ayland@ilande.co.uk> Signed-off-by: Mark Cave-Ayland --- hw/scsi/esp.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 5b169b3720..7e3338815b 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -106,14 +106,14 @@ void esp_request_cancelled(SCSIRequest *req) } } -static void esp_fifo_push(Fifo8 *fifo, uint8_t val) +static void esp_fifo_push(ESPState *s, uint8_t val) { - if (fifo8_num_used(fifo) == fifo->capacity) { + if (fifo8_num_used(&s->fifo) == s->fifo.capacity) { trace_esp_error_fifo_overrun(); return; } - fifo8_push(fifo, val); + fifo8_push(&s->fifo, val); } static uint8_t esp_fifo_pop(Fifo8 *fifo) @@ -229,7 +229,7 @@ static void esp_pdma_write(ESPState *s, uint8_t val) return; } - esp_fifo_push(&s->fifo, val); + esp_fifo_push(s, val); dmalen--; esp_set_tc(s, dmalen); @@ -1240,7 +1240,7 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val) break; case ESP_FIFO: if (!fifo8_is_full(&s->fifo)) { - esp_fifo_push(&s->fifo, val); + esp_fifo_push(s, val); } esp_do_nodma(s); break; From 61fa150d12abe88fb87ef86126fca8ccdf94eb5e Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Sun, 24 Mar 2024 19:16:55 +0000 Subject: [PATCH 06/17] esp.c: change esp_fifo_pop() to take ESPState MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that all users of esp_fifo_pop() operate on the main FIFO there is no need to pass the FIFO explicitly. Signed-off-by: Mark Cave-Ayland Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Paolo Bonzini Message-Id: <20240324191707.623175-7-mark.cave-ayland@ilande.co.uk> Signed-off-by: Mark Cave-Ayland --- hw/scsi/esp.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 7e3338815b..d474268438 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -116,13 +116,13 @@ static void esp_fifo_push(ESPState *s, uint8_t val) fifo8_push(&s->fifo, val); } -static uint8_t esp_fifo_pop(Fifo8 *fifo) +static uint8_t esp_fifo_pop(ESPState *s) { - if (fifo8_is_empty(fifo)) { + if (fifo8_is_empty(&s->fifo)) { return 0; } - return fifo8_pop(fifo); + return fifo8_pop(&s->fifo); } static uint32_t esp_fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen) @@ -217,7 +217,7 @@ static uint8_t esp_pdma_read(ESPState *s) { uint8_t val; - val = esp_fifo_pop(&s->fifo); + val = esp_fifo_pop(s); return val; } @@ -1184,7 +1184,7 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr) switch (saddr) { case ESP_FIFO: - s->rregs[ESP_FIFO] = esp_fifo_pop(&s->fifo); + s->rregs[ESP_FIFO] = esp_fifo_pop(s); val = s->rregs[ESP_FIFO]; break; case ESP_RINTR: From 1f46d1c3a530a9180b3bc202b99615c5a04938af Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Sun, 24 Mar 2024 19:16:56 +0000 Subject: [PATCH 07/17] esp.c: use esp_fifo_push() instead of fifo8_push() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There are still a few places that use fifo8_push() instead of esp_fifo_push() in order to push a value into the FIFO. Update those places to use esp_fifo_push() instead. Signed-off-by: Mark Cave-Ayland Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Paolo Bonzini Message-Id: <20240324191707.623175-8-mark.cave-ayland@ilande.co.uk> Signed-off-by: Mark Cave-Ayland --- hw/scsi/esp.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index d474268438..8d2d36d56c 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -858,7 +858,7 @@ static void esp_do_nodma(ESPState *s) return; } if (fifo8_is_empty(&s->fifo)) { - fifo8_push(&s->fifo, s->async_buf[0]); + esp_fifo_push(s, s->async_buf[0]); s->async_buf++; s->async_len--; s->ti_size--; @@ -881,7 +881,7 @@ static void esp_do_nodma(ESPState *s) case STAT_ST: switch (s->rregs[ESP_CMD]) { case CMD_ICCS: - fifo8_push(&s->fifo, s->status); + esp_fifo_push(s, s->status); esp_set_phase(s, STAT_MI); /* Process any message in phase data */ @@ -893,7 +893,7 @@ static void esp_do_nodma(ESPState *s) case STAT_MI: switch (s->rregs[ESP_CMD]) { case CMD_ICCS: - fifo8_push(&s->fifo, 0); + esp_fifo_push(s, 0); /* Raise end of command interrupt */ s->rregs[ESP_RINTR] |= INTR_FC; From da8381260bc381b25a84c2d8515c6cc7b6764562 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Sun, 24 Mar 2024 19:16:57 +0000 Subject: [PATCH 08/17] esp.c: change esp_fifo_pop_buf() to take ESPState MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that all users of esp_fifo_pop_buf() operate on the main FIFO there is no need to pass the FIFO explicitly. Signed-off-by: Mark Cave-Ayland Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Paolo Bonzini Message-Id: <20240324191707.623175-9-mark.cave-ayland@ilande.co.uk> Signed-off-by: Mark Cave-Ayland --- hw/scsi/esp.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 8d2d36d56c..83b621ee0f 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -155,9 +155,9 @@ static uint32_t esp_fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen) return n; } -static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen) +static uint32_t esp_fifo_pop_buf(ESPState *s, uint8_t *dest, int maxlen) { - return esp_fifo8_pop_buf(fifo, dest, maxlen); + return esp_fifo8_pop_buf(&s->fifo, dest, maxlen); } static uint32_t esp_get_tc(ESPState *s) @@ -459,7 +459,7 @@ static void esp_do_dma(ESPState *s) s->dma_memory_read(s->dma_opaque, buf, len); esp_set_tc(s, esp_get_tc(s) - len); } else { - len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo)); + len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo)); len = MIN(fifo8_num_free(&s->cmdfifo), len); esp_raise_drq(s); } @@ -515,7 +515,7 @@ static void esp_do_dma(ESPState *s) fifo8_push_all(&s->cmdfifo, buf, len); esp_set_tc(s, esp_get_tc(s) - len); } else { - len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo)); + len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo)); len = MIN(fifo8_num_free(&s->cmdfifo), len); fifo8_push_all(&s->cmdfifo, buf, len); esp_raise_drq(s); @@ -549,7 +549,7 @@ static void esp_do_dma(ESPState *s) /* Copy FIFO data to device */ len = MIN(s->async_len, ESP_FIFO_SZ); len = MIN(len, fifo8_num_used(&s->fifo)); - len = esp_fifo_pop_buf(&s->fifo, s->async_buf, len); + len = esp_fifo_pop_buf(s, s->async_buf, len); esp_raise_drq(s); } @@ -713,7 +713,7 @@ static void esp_nodma_ti_dataout(ESPState *s) } len = MIN(s->async_len, ESP_FIFO_SZ); len = MIN(len, fifo8_num_used(&s->fifo)); - esp_fifo_pop_buf(&s->fifo, s->async_buf, len); + esp_fifo_pop_buf(s, s->async_buf, len); s->async_buf += len; s->async_len -= len; s->ti_size += len; @@ -738,7 +738,7 @@ static void esp_do_nodma(ESPState *s) switch (s->rregs[ESP_CMD]) { case CMD_SELATN: /* Copy FIFO into cmdfifo */ - len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo)); + len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo)); len = MIN(fifo8_num_free(&s->cmdfifo), len); fifo8_push_all(&s->cmdfifo, buf, len); @@ -757,7 +757,7 @@ static void esp_do_nodma(ESPState *s) case CMD_SELATNS: /* Copy one byte from FIFO into cmdfifo */ - len = esp_fifo_pop_buf(&s->fifo, buf, 1); + len = esp_fifo_pop_buf(s, buf, 1); len = MIN(fifo8_num_free(&s->cmdfifo), len); fifo8_push_all(&s->cmdfifo, buf, len); @@ -774,7 +774,7 @@ static void esp_do_nodma(ESPState *s) case CMD_TI: /* Copy FIFO into cmdfifo */ - len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo)); + len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo)); len = MIN(fifo8_num_free(&s->cmdfifo), len); fifo8_push_all(&s->cmdfifo, buf, len); @@ -792,7 +792,7 @@ static void esp_do_nodma(ESPState *s) switch (s->rregs[ESP_CMD]) { case CMD_TI: /* Copy FIFO into cmdfifo */ - len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo)); + len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo)); len = MIN(fifo8_num_free(&s->cmdfifo), len); fifo8_push_all(&s->cmdfifo, buf, len); @@ -821,7 +821,7 @@ static void esp_do_nodma(ESPState *s) case CMD_SEL | CMD_DMA: case CMD_SELATN | CMD_DMA: /* Copy FIFO into cmdfifo */ - len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo)); + len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo)); len = MIN(fifo8_num_free(&s->cmdfifo), len); fifo8_push_all(&s->cmdfifo, buf, len); @@ -836,7 +836,7 @@ static void esp_do_nodma(ESPState *s) case CMD_SEL: case CMD_SELATN: /* FIFO already contain entire CDB: copy to cmdfifo and execute */ - len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo)); + len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo)); len = MIN(fifo8_num_free(&s->cmdfifo), len); fifo8_push_all(&s->cmdfifo, buf, len); From 266170f91f9079c102dafb252497f3bae5e844ee Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Sun, 24 Mar 2024 19:16:58 +0000 Subject: [PATCH 09/17] esp.c: introduce esp_fifo_push_buf() function for pushing to the FIFO MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of pushing data into the FIFO directly with fifo8_push_all(), add a new esp_fifo_push_buf() function and use it accordingly. Signed-off-by: Mark Cave-Ayland Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Paolo Bonzini Message-Id: <20240324191707.623175-10-mark.cave-ayland@ilande.co.uk> Signed-off-by: Mark Cave-Ayland --- hw/scsi/esp.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 83b621ee0f..1aac8f5564 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -116,6 +116,11 @@ static void esp_fifo_push(ESPState *s, uint8_t val) fifo8_push(&s->fifo, val); } +static void esp_fifo_push_buf(ESPState *s, uint8_t *buf, int len) +{ + fifo8_push_all(&s->fifo, buf, len); +} + static uint8_t esp_fifo_pop(ESPState *s) { if (fifo8_is_empty(&s->fifo)) { @@ -601,7 +606,7 @@ static void esp_do_dma(ESPState *s) } else { /* Copy device data to FIFO */ len = MIN(len, fifo8_num_free(&s->fifo)); - fifo8_push_all(&s->fifo, s->async_buf, len); + esp_fifo_push_buf(s, s->async_buf, len); esp_raise_drq(s); } @@ -650,7 +655,7 @@ static void esp_do_dma(ESPState *s) if (s->dma_memory_write) { s->dma_memory_write(s->dma_opaque, buf, len); } else { - fifo8_push_all(&s->fifo, buf, len); + esp_fifo_push_buf(s, buf, len); } esp_set_tc(s, esp_get_tc(s) - len); @@ -685,7 +690,7 @@ static void esp_do_dma(ESPState *s) if (s->dma_memory_write) { s->dma_memory_write(s->dma_opaque, buf, len); } else { - fifo8_push_all(&s->fifo, buf, len); + esp_fifo_push_buf(s, buf, len); } esp_set_tc(s, esp_get_tc(s) - len); From 5a50644e4763b6e8370eddc10d30d87134a91167 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Sun, 24 Mar 2024 19:16:59 +0000 Subject: [PATCH 10/17] esp.c: don't assert() if FIFO empty when executing non-DMA SELATNS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The current logic assumes that at least 1 byte is present in the FIFO when executing a non-DMA SELATNS command, but this may not be the case if the guest executes an invalid ESP command sequence. Reported-by: Chuhong Yuan Signed-off-by: Mark Cave-Ayland Reviewed-by: Paolo Bonzini Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20240324191707.623175-11-mark.cave-ayland@ilande.co.uk> Signed-off-by: Mark Cave-Ayland --- hw/scsi/esp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 1aac8f5564..f3aa5364cf 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -762,7 +762,8 @@ static void esp_do_nodma(ESPState *s) case CMD_SELATNS: /* Copy one byte from FIFO into cmdfifo */ - len = esp_fifo_pop_buf(s, buf, 1); + len = esp_fifo_pop_buf(s, buf, + MIN(fifo8_num_used(&s->fifo), 1)); len = MIN(fifo8_num_free(&s->cmdfifo), len); fifo8_push_all(&s->cmdfifo, buf, len); From 5aa0df4067cfc9bf74f08787d31b007957c7d899 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Sun, 24 Mar 2024 19:17:00 +0000 Subject: [PATCH 11/17] esp.c: rework esp_cdb_length() into esp_cdb_ready() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The esp_cdb_length() function is only used as part of a calculation to determine whether the cmdfifo contains an entire SCSI CDB. Rework esp_cdb_length() into a new esp_cdb_ready() function which both enables us to handle the case where scsi_cdb_length() returns -1, plus simplify the logic for its callers. Suggested-by: Paolo Bonzini Signed-off-by: Mark Cave-Ayland Reviewed-by: Paolo Bonzini Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20240324191707.623175-12-mark.cave-ayland@ilande.co.uk> Signed-off-by: Mark Cave-Ayland --- hw/scsi/esp.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index f3aa5364cf..f47abc36d6 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -425,20 +425,20 @@ static void write_response(ESPState *s) } } -static int esp_cdb_length(ESPState *s) +static bool esp_cdb_ready(ESPState *s) { + int len = fifo8_num_used(&s->cmdfifo) - s->cmdfifo_cdb_offset; const uint8_t *pbuf; - int cmdlen, len; + int cdblen; - cmdlen = fifo8_num_used(&s->cmdfifo); - if (cmdlen < s->cmdfifo_cdb_offset) { - return 0; + if (len <= 0) { + return false; } - pbuf = fifo8_peek_buf(&s->cmdfifo, cmdlen, NULL); - len = scsi_cdb_length((uint8_t *)&pbuf[s->cmdfifo_cdb_offset]); + pbuf = fifo8_peek_buf(&s->cmdfifo, len, NULL); + cdblen = scsi_cdb_length((uint8_t *)&pbuf[s->cmdfifo_cdb_offset]); - return len; + return cdblen < 0 ? false : (len >= cdblen); } static void esp_dma_ti_check(ESPState *s) @@ -806,10 +806,9 @@ static void esp_do_nodma(ESPState *s) trace_esp_handle_ti_cmd(cmdlen); /* CDB may be transferred in one or more TI commands */ - if (esp_cdb_length(s) && esp_cdb_length(s) == - fifo8_num_used(&s->cmdfifo) - s->cmdfifo_cdb_offset) { - /* Command has been received */ - do_cmd(s); + if (esp_cdb_ready(s)) { + /* Command has been received */ + do_cmd(s); } else { /* * If data was transferred from the FIFO then raise bus @@ -832,10 +831,9 @@ static void esp_do_nodma(ESPState *s) fifo8_push_all(&s->cmdfifo, buf, len); /* Handle when DMA transfer is terminated by non-DMA FIFO write */ - if (esp_cdb_length(s) && esp_cdb_length(s) == - fifo8_num_used(&s->cmdfifo) - s->cmdfifo_cdb_offset) { - /* Command has been received */ - do_cmd(s); + if (esp_cdb_ready(s)) { + /* Command has been received */ + do_cmd(s); } break; From 3cc70889a35a972358e9b0e768a06b7159def1f3 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Sun, 24 Mar 2024 19:17:01 +0000 Subject: [PATCH 12/17] esp.c: prevent cmdfifo overflow in esp_cdb_ready() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit During normal use the cmdfifo will never wrap internally and cmdfifo_cdb_offset will always indicate the start of the SCSI CDB. However it is possible that a malicious guest could issue an invalid ESP command sequence such that cmdfifo wraps internally and cmdfifo_cdb_offset could point beyond the end of the FIFO data buffer. Add an extra check to fifo8_peek_buf() to ensure that if the cmdfifo has wrapped internally then esp_cdb_ready() will exit rather than allow scsi_cdb_length() to access data outside the cmdfifo data buffer. Reported-by: Chuhong Yuan Signed-off-by: Mark Cave-Ayland Reviewed-by: Paolo Bonzini Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20240324191707.623175-13-mark.cave-ayland@ilande.co.uk> Signed-off-by: Mark Cave-Ayland --- hw/scsi/esp.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index f47abc36d6..d8db33b921 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -429,13 +429,23 @@ static bool esp_cdb_ready(ESPState *s) { int len = fifo8_num_used(&s->cmdfifo) - s->cmdfifo_cdb_offset; const uint8_t *pbuf; + uint32_t n; int cdblen; if (len <= 0) { return false; } - pbuf = fifo8_peek_buf(&s->cmdfifo, len, NULL); + pbuf = fifo8_peek_buf(&s->cmdfifo, len, &n); + if (n < len) { + /* + * In normal use the cmdfifo should never wrap, but include this check + * to prevent a malicious guest from reading past the end of the + * cmdfifo data buffer below + */ + return false; + } + cdblen = scsi_cdb_length((uint8_t *)&pbuf[s->cmdfifo_cdb_offset]); return cdblen < 0 ? false : (len >= cdblen); From 2c1017bfc28b792dd03ea2aaa7453ec20ab5f7ec Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Sun, 24 Mar 2024 19:17:02 +0000 Subject: [PATCH 13/17] esp.c: move esp_set_phase() and esp_get_phase() towards the beginning of the file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This allows these functions to be used earlier in the file without needing a separate forward declaration. Signed-off-by: Mark Cave-Ayland Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Paolo Bonzini Message-Id: <20240324191707.623175-14-mark.cave-ayland@ilande.co.uk> Signed-off-by: Mark Cave-Ayland --- hw/scsi/esp.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index d8db33b921..9e35c00927 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -79,6 +79,24 @@ static void esp_lower_drq(ESPState *s) } } +static const char *esp_phase_names[8] = { + "DATA OUT", "DATA IN", "COMMAND", "STATUS", + "(reserved)", "(reserved)", "MESSAGE OUT", "MESSAGE IN" +}; + +static void esp_set_phase(ESPState *s, uint8_t phase) +{ + s->rregs[ESP_RSTAT] &= ~7; + s->rregs[ESP_RSTAT] |= phase; + + trace_esp_set_phase(esp_phase_names[phase]); +} + +static uint8_t esp_get_phase(ESPState *s) +{ + return s->rregs[ESP_RSTAT] & 7; +} + void esp_dma_enable(ESPState *s, int irq, int level) { if (level) { @@ -200,24 +218,6 @@ static uint32_t esp_get_stc(ESPState *s) return dmalen; } -static const char *esp_phase_names[8] = { - "DATA OUT", "DATA IN", "COMMAND", "STATUS", - "(reserved)", "(reserved)", "MESSAGE OUT", "MESSAGE IN" -}; - -static void esp_set_phase(ESPState *s, uint8_t phase) -{ - s->rregs[ESP_RSTAT] &= ~7; - s->rregs[ESP_RSTAT] |= phase; - - trace_esp_set_phase(esp_phase_names[phase]); -} - -static uint8_t esp_get_phase(ESPState *s) -{ - return s->rregs[ESP_RSTAT] & 7; -} - static uint8_t esp_pdma_read(ESPState *s) { uint8_t val; From 743d8736458d3f939fb957835f42ecc3e2d0f75c Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Sun, 24 Mar 2024 19:17:03 +0000 Subject: [PATCH 14/17] esp.c: introduce esp_update_drq() and update esp_fifo_{push, pop}_buf() to use it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This new function sets the DRQ line correctly according to the current transfer mode, direction and FIFO contents. Update esp_fifo_push_buf() and esp_fifo_pop_buf() to use it so that DRQ is always set correctly when reading/writing multiple bytes to/from the FIFO. Signed-off-by: Mark Cave-Ayland Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Paolo Bonzini Message-Id: <20240324191707.623175-15-mark.cave-ayland@ilande.co.uk> Signed-off-by: Mark Cave-Ayland --- hw/scsi/esp.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 9e35c00927..6fd1a12f23 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -124,6 +124,48 @@ void esp_request_cancelled(SCSIRequest *req) } } +static void esp_update_drq(ESPState *s) +{ + bool to_device; + + switch (esp_get_phase(s)) { + case STAT_MO: + case STAT_CD: + case STAT_DO: + to_device = true; + break; + + case STAT_DI: + case STAT_ST: + case STAT_MI: + to_device = false; + break; + + default: + return; + } + + if (s->dma) { + /* DMA request so update DRQ according to transfer direction */ + if (to_device) { + if (fifo8_num_free(&s->fifo) < 2) { + esp_lower_drq(s); + } else { + esp_raise_drq(s); + } + } else { + if (fifo8_num_used(&s->fifo) < 2) { + esp_lower_drq(s); + } else { + esp_raise_drq(s); + } + } + } else { + /* Not a DMA request */ + esp_lower_drq(s); + } +} + static void esp_fifo_push(ESPState *s, uint8_t val) { if (fifo8_num_used(&s->fifo) == s->fifo.capacity) { @@ -137,6 +179,7 @@ static void esp_fifo_push(ESPState *s, uint8_t val) static void esp_fifo_push_buf(ESPState *s, uint8_t *buf, int len) { fifo8_push_all(&s->fifo, buf, len); + esp_update_drq(s); } static uint8_t esp_fifo_pop(ESPState *s) @@ -180,7 +223,10 @@ static uint32_t esp_fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen) static uint32_t esp_fifo_pop_buf(ESPState *s, uint8_t *dest, int maxlen) { - return esp_fifo8_pop_buf(&s->fifo, dest, maxlen); + uint32_t len = esp_fifo8_pop_buf(&s->fifo, dest, maxlen); + + esp_update_drq(s); + return len; } static uint32_t esp_get_tc(ESPState *s) From ffa3a5f2bedb2cdd7fb1c5c0f6702fd6eb0f5962 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Sun, 24 Mar 2024 19:17:04 +0000 Subject: [PATCH 15/17] esp.c: update esp_fifo_{push, pop}() to call esp_update_drq() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This ensures that the DRQ line is always set correctly when reading/writing single bytes to/from the FIFO. Signed-off-by: Mark Cave-Ayland Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Paolo Bonzini Message-Id: <20240324191707.623175-16-mark.cave-ayland@ilande.co.uk> Signed-off-by: Mark Cave-Ayland --- hw/scsi/esp.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 6fd1a12f23..4895181ec1 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -170,10 +170,11 @@ static void esp_fifo_push(ESPState *s, uint8_t val) { if (fifo8_num_used(&s->fifo) == s->fifo.capacity) { trace_esp_error_fifo_overrun(); - return; + } else { + fifo8_push(&s->fifo, val); } - fifo8_push(&s->fifo, val); + esp_update_drq(s); } static void esp_fifo_push_buf(ESPState *s, uint8_t *buf, int len) @@ -184,11 +185,16 @@ static void esp_fifo_push_buf(ESPState *s, uint8_t *buf, int len) static uint8_t esp_fifo_pop(ESPState *s) { + uint8_t val; + if (fifo8_is_empty(&s->fifo)) { - return 0; + val = 0; + } else { + val = fifo8_pop(&s->fifo); } - return fifo8_pop(&s->fifo); + esp_update_drq(s); + return val; } static uint32_t esp_fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen) From 60c572502cbb89f1f46c2127794f956220e5dbab Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Sun, 24 Mar 2024 19:17:05 +0000 Subject: [PATCH 16/17] esp.c: ensure esp_pdma_write() always calls esp_fifo_push() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This ensures that esp_update_drq() is called via esp_fifo_push() whenever the host uses PDMA to transfer data to a SCSI device. Signed-off-by: Mark Cave-Ayland Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Paolo Bonzini Message-Id: <20240324191707.623175-17-mark.cave-ayland@ilande.co.uk> Signed-off-by: Mark Cave-Ayland --- hw/scsi/esp.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 4895181ec1..04dfd90090 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -282,14 +282,12 @@ static void esp_pdma_write(ESPState *s, uint8_t val) { uint32_t dmalen = esp_get_tc(s); - if (dmalen == 0) { - return; - } - esp_fifo_push(s, val); - dmalen--; - esp_set_tc(s, dmalen); + if (dmalen && s->drq_state) { + dmalen--; + esp_set_tc(s, dmalen); + } } static int esp_select(ESPState *s) From d7fe931818d5e9aa70d08056c43b496ce789ba64 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Sun, 24 Mar 2024 19:17:06 +0000 Subject: [PATCH 17/17] esp.c: remove explicit setting of DRQ within ESP state machine Now the esp_update_drq() is called for all reads/writes to the FIFO, there is no need to manually raise and lower the DRQ signal. Signed-off-by: Mark Cave-Ayland Resolves: https://gitlab.com/qemu-project/qemu/-/issues/611 Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1831 Reviewed-by: Paolo Bonzini Message-Id: <20240324191707.623175-18-mark.cave-ayland@ilande.co.uk> Signed-off-by: Mark Cave-Ayland --- hw/scsi/esp.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 04dfd90090..5d9b52632e 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -506,7 +506,6 @@ static void esp_dma_ti_check(ESPState *s) if (esp_get_tc(s) == 0 && fifo8_num_used(&s->fifo) < 2) { s->rregs[ESP_RINTR] |= INTR_BS; esp_raise_irq(s); - esp_lower_drq(s); } } @@ -526,7 +525,6 @@ static void esp_do_dma(ESPState *s) } else { len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo)); len = MIN(fifo8_num_free(&s->cmdfifo), len); - esp_raise_drq(s); } fifo8_push_all(&s->cmdfifo, buf, len); @@ -583,7 +581,6 @@ static void esp_do_dma(ESPState *s) len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo)); len = MIN(fifo8_num_free(&s->cmdfifo), len); fifo8_push_all(&s->cmdfifo, buf, len); - esp_raise_drq(s); } trace_esp_handle_ti_cmd(cmdlen); s->ti_size = 0; @@ -615,7 +612,6 @@ static void esp_do_dma(ESPState *s) len = MIN(s->async_len, ESP_FIFO_SZ); len = MIN(len, fifo8_num_used(&s->fifo)); len = esp_fifo_pop_buf(s, s->async_buf, len); - esp_raise_drq(s); } s->async_buf += len; @@ -667,7 +663,6 @@ static void esp_do_dma(ESPState *s) /* Copy device data to FIFO */ len = MIN(len, fifo8_num_free(&s->fifo)); esp_fifo_push_buf(s, s->async_buf, len); - esp_raise_drq(s); } s->async_buf += len; @@ -733,7 +728,6 @@ static void esp_do_dma(ESPState *s) if (fifo8_num_used(&s->fifo) < 2) { s->rregs[ESP_RINTR] |= INTR_BS; esp_raise_irq(s); - esp_lower_drq(s); } break; } @@ -1021,9 +1015,6 @@ void esp_command_complete(SCSIRequest *req, size_t resid) s->rregs[ESP_RINTR] |= INTR_BS; esp_raise_irq(s); - /* Ensure DRQ is set correctly for TC underflow or normal completion */ - esp_dma_ti_check(s); - if (s->current_req) { scsi_req_unref(s->current_req); s->current_req = NULL;