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
v2: - Fix typo in patch 2 commit message - Add reference to datasheet in patch 2 commit message as requested by Paul
Mark Cave-Ayland (2): esp-scsi: flush FIFO before sending SCSI command esp-scsi: check for INTR_BS/INTR_FC instead of STAT_TC for command completion
src/hw/esp-scsi.c | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-)
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 --- 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
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 --- 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..2d2d915 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,22 +118,27 @@ 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; + } + } }
/* At end of DMA TC is set again -> complete command. */
On 2/8/23 11:48, 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
src/hw/esp-scsi.c | 4 ++++ 1 file changed, 4 insertions(+)
Reviewed-by: Philippe Mathieu-Daudé philmd@linaro.org
On 8/2/23 11:48, 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
v2:
- Fix typo in patch 2 commit message
- Add reference to datasheet in patch 2 commit message as requested by Paul
Mark Cave-Ayland (2): esp-scsi: flush FIFO before sending SCSI command esp-scsi: check for INTR_BS/INTR_FC instead of STAT_TC for command completion
src/hw/esp-scsi.c | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-)
Reviewed-by: Paolo Bonzini pbonzini@redhat.com
On 02/08/2023 10:48, 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
v2:
- Fix typo in patch 2 commit message
- Add reference to datasheet in patch 2 commit message as requested by Paul
Mark Cave-Ayland (2): esp-scsi: flush FIFO before sending SCSI command esp-scsi: check for INTR_BS/INTR_FC instead of STAT_TC for command completion
src/hw/esp-scsi.c | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-)
With a bit more time poking at QEMU's ESP emulation at the weekend, I found one more problem in that QEMU currently always sets STAT_TC after issuing every SCSI request.
Hence we need an extra fix to the esp-scsi state machine to handle the case where there is no DATA phase e.g. Test Unit Ready with a non-DMA SELATN command for when this is eventually fixed in QEMU.
ATB,
Mark.
-
Mark Cave-Ayland -
Paolo Bonzini -
Philippe Mathieu-Daudé