[PATCH v3 0/3] esp-scsi: fix reliance on QEMU ESP SCSI implementation bugs
Whilst trying to improve QEMU's ESP SCSI implementation with regard to the NCR datasheet, I noticed that SeaBIOS would fail to boot one of my Debian test images with my patches applied. Further investigation revealed that the SeaBIOS esp-scsi driver inadvertently relies on existing bugs in QEMU's ESP SCSI emulation to work correctly, so this series changes the driver to work as described in the NCR datasheet. With this series applied it is possible for the updated SeaBIOS to boot both QEMU current git HEAD as well as my local WIP branch containing various fixes and improvements to QEMU's ESP SCSI emulation. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> v3: - Add R-B tags for patches 1 and 2 from Paolo - Add patch 3 to handle another bug to be fixed in QEMU's ESP emulation v2: - Fix typo in patch 2 commit message - Add reference to datasheet in patch 2 commit message as requested by Paul Mark Cave-Ayland (3): esp-scsi: flush FIFO before sending SCSI command esp-scsi: check for INTR_BS/INTR_FC instead of STAT_TC for command completion esp-scsi: handle non-DMA SCSI commands with no data phase src/hw/esp-scsi.c | 49 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 14 deletions(-) -- 2.39.2
The ESP FIFO is used as a buffer for DMA requests and so isn't guaranteed to be empty in the case of SCSI errors or a mixed DMA/non-DMA request. Flush the FIFO before sending a SCSI command to guarantee that it is correctly positioned at the start of the FIFO. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> --- src/hw/esp-scsi.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/hw/esp-scsi.c b/src/hw/esp-scsi.c index cc25f22..e4815aa 100644 --- a/src/hw/esp-scsi.c +++ b/src/hw/esp-scsi.c @@ -46,6 +46,7 @@ #define ESP_DMA_WMAC 0x58c #define ESP_CMD_DMA 0x80 +#define ESP_CMD_FLUSH 0x01 #define ESP_CMD_RESET 0x02 #define ESP_CMD_TI 0x10 #define ESP_CMD_ICCS 0x11 @@ -96,6 +97,9 @@ esp_scsi_process_op(struct disk_op_s *op) outb(target, iobase + ESP_WBUSID); + /* Clear FIFO before sending command. */ + outb(ESP_CMD_FLUSH, iobase + ESP_CMD); + /* * We need to pass the LUN at the beginning of the command, and the FIFO * is only 16 bytes, so we cannot support 16-byte CDBs. The alternative -- 2.39.2
On 7/8/23 08:52, Mark Cave-Ayland wrote:
The ESP FIFO is used as a buffer for DMA requests and so isn't guaranteed to be empty in the case of SCSI errors or a mixed DMA/non-DMA request. Flush the FIFO before sending a SCSI command to guarantee that it is correctly positioned at the start of the FIFO.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> --- src/hw/esp-scsi.c | 4 ++++ 1 file changed, 4 insertions(+)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
The ESP SELATN command used to send SCSI commands from the ESP to the SCSI bus is not a DMA command and therefore does not affect the STAT_TC bit. The only reason this works at all is due to a bug in QEMU which (currently) always updates the STAT_TC bit in ESP_RSTAT regardless of the state of the ESP_CMD_DMA bit. According to the NCR datasheet [1] the INTR_BS/INTR_FC bits are set when the SELATN command has completed, so update the existing logic to check for these bits in ESP_RINTR instead. Note that the read of ESP_RINTR needs to be restricted to state == 0 as reading ESP_RINTR resets the ESP_RSTAT register which breaks the STAT_TC check when state == 1. This commit also includes an extra read of ESP_INTR to clear all the interrupt bits before submitting the SELATN command to ensure that we don't accidentally immediately progress to the data phase handling logic where ESP_RINTR bits have already been set by a previous ESP command. [1] "NCR 53C94, 53C95, 53C96 Advanced SCSI Controller" NCR_53C94_53C95_53C96_Data_Sheet_Feb90.pdf Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> --- src/hw/esp-scsi.c | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/src/hw/esp-scsi.c b/src/hw/esp-scsi.c index e4815aa..ebdbd02 100644 --- a/src/hw/esp-scsi.c +++ b/src/hw/esp-scsi.c @@ -57,6 +57,8 @@ #define ESP_STAT_MSG 0x04 #define ESP_STAT_TC 0x10 +#define ESP_INTR_FC 0x08 +#define ESP_INTR_BS 0x10 #define ESP_INTR_DC 0x20 struct esp_lun_s { @@ -97,8 +99,9 @@ esp_scsi_process_op(struct disk_op_s *op) outb(target, iobase + ESP_WBUSID); - /* Clear FIFO before sending command. */ + /* Clear FIFO and interrupts before sending command. */ outb(ESP_CMD_FLUSH, iobase + ESP_CMD); + inb(iobase + ESP_RINTR); /* * We need to pass the LUN at the beginning of the command, and the FIFO @@ -115,21 +118,26 @@ esp_scsi_process_op(struct disk_op_s *op) for (state = 0;;) { u8 stat = inb(iobase + ESP_RSTAT); + u8 intr; - /* Detect disconnected device. */ - if (state == 0 && (inb(iobase + ESP_RINTR) & ESP_INTR_DC)) { - return DISK_RET_ENOTREADY; - } + if (state == 0) { + intr = inb(iobase + ESP_RINTR); - /* HBA reads command, clears CD, sets TC -> do DMA if needed. */ - if (state == 0 && (stat & ESP_STAT_TC)) { - state++; - if (op->count && blocksize) { - /* Data phase. */ - u32 count = (u32)op->count * blocksize; - esp_scsi_dma(iobase, (u32)op->buf_fl, count, scsi_is_read(op)); - outb(ESP_CMD_TI | ESP_CMD_DMA, iobase + ESP_CMD); - continue; + /* Detect disconnected device. */ + if (intr & ESP_INTR_DC) { + return DISK_RET_ENOTREADY; + } + + /* HBA reads command, executes it, sets BS/FC -> do DMA if needed. */ + if (intr & (ESP_INTR_BS | ESP_INTR_FC)) { + state++; + if (op->count && blocksize) { + /* Data phase. */ + u32 count = (u32)op->count * blocksize; + esp_scsi_dma(iobase, (u32)op->buf_fl, count, scsi_is_read(op)); + outb(ESP_CMD_TI | ESP_CMD_DMA, iobase + ESP_CMD); + continue; + } } } -- 2.39.2
The existing esp-scsi state machine checks for the STAT_TC bit to exit state 1 but in the case where there is no data phase, a non-DMA command is executed which doesn't set STAT_TC. This only works because QEMU currently always sets STAT_TC just after issuing every SCSI command. Update the esp-scsi state machine so that in the case where there is no data phase, we immediately execute CMD_ICCS instead of waiting for STAT_TC to be set which will never happen with a non-DMA CMD_SELATN command. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- src/hw/esp-scsi.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/hw/esp-scsi.c b/src/hw/esp-scsi.c index ebdbd02..f19804f 100644 --- a/src/hw/esp-scsi.c +++ b/src/hw/esp-scsi.c @@ -137,19 +137,28 @@ esp_scsi_process_op(struct disk_op_s *op) esp_scsi_dma(iobase, (u32)op->buf_fl, count, scsi_is_read(op)); outb(ESP_CMD_TI | ESP_CMD_DMA, iobase + ESP_CMD); continue; + } else { + /* No data phase. */ + state++; } } } /* At end of DMA TC is set again -> complete command. */ if (state == 1 && (stat & ESP_STAT_TC)) { + state++; + continue; + } + + /* Request message in data. */ + if (state == 2) { state++; outb(ESP_CMD_ICCS, iobase + ESP_CMD); continue; } /* Finally read data from the message in phase. */ - if (state == 2 && (stat & ESP_STAT_MSG)) { + if (state == 3 && (stat & ESP_STAT_MSG)) { state++; status = inb(iobase + ESP_FIFO); inb(iobase + ESP_FIFO); -- 2.39.2
On Mon, Aug 07, 2023 at 07:52:57AM +0100, Mark Cave-Ayland wrote:
Whilst trying to improve QEMU's ESP SCSI implementation with regard to the NCR datasheet, I noticed that SeaBIOS would fail to boot one of my Debian test images with my patches applied.
Further investigation revealed that the SeaBIOS esp-scsi driver inadvertently relies on existing bugs in QEMU's ESP SCSI emulation to work correctly, so this series changes the driver to work as described in the NCR datasheet.
With this series applied it is possible for the updated SeaBIOS to boot both QEMU current git HEAD as well as my local WIP branch containing various fixes and improvements to QEMU's ESP SCSI emulation.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Series committed. thanks, Gerd
participants (3)
-
Gerd Hoffmann -
Mark Cave-Ayland -
Philippe Mathieu-Daudé