[PATCH 0/2] esp-scsi: fix reliance on QEMU ESP SCSI implementation bugs

[Mark Cave-Ayland]

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 (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(-)

[PATCH 1/2] esp-scsi: flush FIFO before sending SCSI command

[Mark Cave-Ayland]

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 
---
 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

[PATCH 1/2] esp-scsi: flush FIFO before sending SCSI command

[Philippe Mathieu-Daudé]

On 29/7/23 15:04, 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
>
> src/hw/esp-scsi.c | 4 ++++
>   1 file changed, 4 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé

[PATCH 2/2] esp-scsi: check for INTR_BS/INTR_FC instead of STAT_TC for command completion

[Mark Cave-Ayland]

The ESP SELATN command used to send SCSI commands from the ESP to thes 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 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.

Signed-off-by: Mark Cave-Ayland 
---
 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.  */

[PATCH 2/2] esp-scsi: check for INTR_BS/INTR_FC instead of STAT_TC for command completion

[Paul Menzel]

[Cc: +Paolo as original author of the driver]

Dear Mark,

Thank you for your patch.

Am 29.07.23 um 15:04 schrieb Mark Cave-Ayland:
> The ESP SELATN command used to send SCSI commands from the ESP to thes SCSI bus

s/thes/the/
> 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.

I’d appreciated a link to these QEMU patches, describing the problem in 
QEMU.
> According to the NCR datasheet the INTR_BS/INTR_FC bits are set when the SELATN

Could you please mention the full name and revision of the datasheet?
> 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.

It’d be great, if you added the QEMU commands how to test your patch.

Lastly, will SeaBIOS built with your patches still work with older QEMU 
versions without your QEMU patches?
> Signed-off-by: Mark Cave-Ayland
>
> 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.  */

Reviewed-by: Paul Menzel

Kind regards,

Paul

[PATCH 2/2] esp-scsi: check for INTR_BS/INTR_FC instead of STAT_TC for command completion

[Mark Cave-Ayland]

On 30/07/2023 08:47, Paul Menzel wrote:
> [Cc: +Paolo as original author of the driver]
>
> Dear Mark,
>
> Thank you for your patch.
>
> Am 29.07.23 um 15:04 schrieb Mark Cave-Ayland:
> > The ESP SELATN command used to send SCSI commands from the ESP to thes SCSI bus
>
> s/thes/the/

Ooops indeed, just a typo. As this is my first patch posted to the SeaBIOS list, is 
this something that a maintainer can fix up when applying the patch, or does it need 
a series repost?
> > 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.
>
> I’d appreciated a link to these QEMU patches, describing the problem in QEMU.

As mentioned on the cover letter, this is currently a WIP following on from my 
earlier ESP series at 
https://patchew.org/QEMU/[e-mail]... which 
was to enable QEMU's ESP emulation to boot MacOS classic.

Whilst the current implementation works, it's main problem is that there are 
currently 4 different variations of the DMA transfer code: one for real DMA, another 
for Mac "pseudo-DMA" (PDMA) and then each of those has a separate implementation for 
MESSAGE OUT and COMMAND phases.

I have a WIP series that goes most of the way to consolidating everything down to a 
single implementation, but progress had been paused due to the SeaBIOS boot test 
failing. Now having looked at the SeaBIOS ESP SCSI driver I'm sure that the bug is 
there, and that's why I submitted the patches here first with the aim of getting the 
SeaBIOS fixes into QEMU first so I can finish off and submit the ESP series to QEMU.

The underlying issue resulting in this series is that QEMU changes the STAT_TC bit 
(and the DMA counter) in several places where it shouldn't, and this has a subtle 
effect on various OS driver state machines. The aim of my WIP changes is to fix this 
so that QEMU's implementation matches the datasheet in this regard.
> > According to the NCR datasheet the INTR_BS/INTR_FC bits are set when the SELATN
>
> Could you please mention the full name and revision of the datasheet?

The datasheet I have here is entitled "NCR 53C94, 53C95, 53C96 Advanced SCSI 
Controller" and has the filename NCR_53C94_53C95_53C96_Data_Sheet_Feb90.pdf.
> > 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.
>
> It’d be great, if you added the QEMU commands how to test your patch.

Sure! The QEMU command line I'm using to test SeaBIOS is:

./qemu-system-x86_64 -accel kvm -accel tcg -no-shutdown -device am53c974 \
   -device scsi-cd,drive=cd1 \
   -drive if=none,id=cd1,format=raw,file=debian-10.6.0-amd64-netinst.iso

If SeaBIOS boots to the Debian installer menu then all is working well.
> Lastly, will SeaBIOS built with your patches still work with older QEMU versions 
> without your QEMU patches?

Yes indeed, and this is noted on the cover letter of the series.
> > Signed-off-by: Mark Cave-Ayland
> >
> > 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.  */
>
> Reviewed-by: Paul Menzel

Thanks!
> Kind regards,
>
> Paul
> _______________________________________________
> SeaBIOS mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]

ATB,

Mark.

[PATCH 2/2] esp-scsi: check for INTR_BS/INTR_FC instead of STAT_TC for command completion

[Paul Menzel]

Dear Mark,

Am 30.07.23 um 10:56 schrieb Mark Cave-Ayland:
> On 30/07/2023 08:47, Paul Menzel wrote:
> > [Cc: +Paolo as original author of the driver]

[…]
> > Am 29.07.23 um 15:04 schrieb Mark Cave-Ayland:
> > > The ESP SELATN command used to send SCSI commands from the ESP to 
> > > thes SCSI bus
> >
> > s/thes/the/
>
> Ooops indeed, just a typo. As this is my first patch posted to the 
> SeaBIOS list, is this something that a maintainer can fix up when 
> applying the patch, or does it need a series repost?

I’d just repost as v2.
> > > 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.
> >
> > I’d appreciated a link to these QEMU patches, describing the problem 
> > in QEMU.
>
> As mentioned on the cover letter, this is currently a WIP following on 
> from my earlier ESP series at 
> https://patchew.org/QEMU/[e-mail]... which was to enable QEMU's ESP emulation to boot MacOS classic.

I see. The “problem“ is, that in SeaBIOS the info from the cover letter 
won’t get added to the git archive, so I prefer to have the information 
in the commit message. Maybe also link additionally link to the commit 
in GitLab, which unfortunately also does not contain the information 
from the cover letter, and mention that it was added in March 2021.
> Whilst the current implementation works, it's main problem is that there 
> are currently 4 different variations of the DMA transfer code: one for 
> real DMA, another for Mac "pseudo-DMA" (PDMA) and then each of those has 
> a separate implementation for MESSAGE OUT and COMMAND phases.
>
> I have a WIP series that goes most of the way to consolidating 
> everything down to a single implementation, but progress had been paused 
> due to the SeaBIOS boot test failing. Now having looked at the SeaBIOS 
> ESP SCSI driver I'm sure that the bug is there, and that's why I 
> submitted the patches here first with the aim of getting the SeaBIOS 
> fixes into QEMU first so I can finish off and submit the ESP series to 
> QEMU.
>
> The underlying issue resulting in this series is that QEMU changes the 
> STAT_TC bit (and the DMA counter) in several places where it shouldn't, 
> and this has a subtle effect on various OS driver state machines. The 
> aim of my WIP changes is to fix this so that QEMU's implementation 
> matches the datasheet in this regard.

Understood.
> > > According to the NCR datasheet the INTR_BS/INTR_FC bits are set when 
> > > the SELATN
> >
> > Could you please mention the full name and revision of the datasheet?
>
> The datasheet I have here is entitled "NCR 53C94, 53C95, 53C96 Advanced 
> SCSI Controller" and has the filename 
> NCR_53C94_53C95_53C96_Data_Sheet_Feb90.pdf.

Thank you. It’d be great, if you added that.
> > > 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.
> >
> > It’d be great, if you added the QEMU commands how to test your patch.
>
> Sure! The QEMU command line I'm using to test SeaBIOS is:
>
> ./qemu-system-x86_64 -accel kvm -accel tcg -no-shutdown -device am53c974 \
>    -device scsi-cd,drive=cd1 \
>    -drive if=none,id=cd1,format=raw,file=debian-10.6.0-amd64-netinst.iso
>
> If SeaBIOS boots to the Debian installer menu then all is working well.

Thank you.
> > Lastly, will SeaBIOS built with your patches still work with older 
> > QEMU versions without your QEMU patches?
>
> Yes indeed, and this is noted on the cover letter of the series.

Right, I missed that.
> > > Signed-off-by: Mark Cave-Ayland
> > >
> > > src/hw/esp-scsi.c | 36 ++++++++++++++++++++++--------------
> > >   1 file changed, 22 insertions(+), 14 deletions(-)

[…]

Kind regards,

Paul

[PATCH 2/2] esp-scsi: check for INTR_BS/INTR_FC instead of STAT_TC for command completion

[Mark Cave-Ayland]

On 01/08/2023 03:07, Paul Menzel wrote:
> Dear Mark,
>
> Am 30.07.23 um 10:56 schrieb Mark Cave-Ayland:
> > On 30/07/2023 08:47, Paul Menzel wrote:
> > > [Cc: +Paolo as original author of the driver]
>
> […]
> > > Am 29.07.23 um 15:04 schrieb Mark Cave-Ayland:
> > > > The ESP SELATN command used to send SCSI commands from the ESP to thes SCSI bus
> > >
> > > s/thes/the/
> >
> > Ooops indeed, just a typo. As this is my first patch posted to the SeaBIOS list, is 
> > this something that a maintainer can fix up when applying the patch, or does it 
> > need a series repost?
>
> I’d just repost as v2.

No worries, I shall aim to do that shortly.
> > > > 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.
> > >
> > > I’d appreciated a link to these QEMU patches, describing the problem in QEMU.
> >
> > As mentioned on the cover letter, this is currently a WIP following on from my 
> > earlier ESP series at 
> > https://patchew.org/QEMU/[e-mail]... which 
> > was to enable QEMU's ESP emulation to boot MacOS classic.
>
> I see. The “problem“ is, that in SeaBIOS the info from the cover letter won’t get 
> added to the git archive, so I prefer to have the information in the commit message. 
> Maybe also link additionally link to the commit in GitLab, which unfortunately also 
> does not contain the information from the cover letter, and mention that it was added 
> in March 2021.

Well the above link was to a previous series of improvements to QEMU's ESP emulation 
just to give an idea as to the work I've been doing, and isn't directly related to 
this series. So it's not something that is relevant to the commit message.

I'd say really the explanation in this commit message covers all the relevant 
details: QEMU currently has bugs related to the DMA counter and STAT_TC, and 
unfortunately the esp-scsi driver currently relies on them to work.
> > Whilst the current implementation works, it's main problem is that there are 
> > currently 4 different variations of the DMA transfer code: one for real DMA, 
> > another for Mac "pseudo-DMA" (PDMA) and then each of those has a separate 
> > implementation for MESSAGE OUT and COMMAND phases.
> >
> > I have a WIP series that goes most of the way to consolidating everything down to a 
> > single implementation, but progress had been paused due to the SeaBIOS boot test 
> > failing. Now having looked at the SeaBIOS ESP SCSI driver I'm sure that the bug is 
> > there, and that's why I submitted the patches here first with the aim of getting 
> > the SeaBIOS fixes into QEMU first so I can finish off and submit the ESP series to 
> > QEMU.
> >
> > The underlying issue resulting in this series is that QEMU changes the STAT_TC bit 
> > (and the DMA counter) in several places where it shouldn't, and this has a subtle 
> > effect on various OS driver state machines. The aim of my WIP changes is to fix 
> > this so that QEMU's implementation matches the datasheet in this regard.
>
> Understood.
> > > > According to the NCR datasheet the INTR_BS/INTR_FC bits are set when the SELATN
> > >
> > > Could you please mention the full name and revision of the datasheet?
> >
> > The datasheet I have here is entitled "NCR 53C94, 53C95, 53C96 Advanced SCSI 
> > Controller" and has the filename NCR_53C94_53C95_53C96_Data_Sheet_Feb90.pdf.
>
> Thank you. It’d be great, if you added that.

Yes, I can do that.
> > > > 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.
> > >
> > > It’d be great, if you added the QEMU commands how to test your patch.
> >
> > Sure! The QEMU command line I'm using to test SeaBIOS is:
> >
> > ./qemu-system-x86_64 -accel kvm -accel tcg -no-shutdown -device am53c974 \
> >    -device scsi-cd,drive=cd1 \
> >    -drive if=none,id=cd1,format=raw,file=debian-10.6.0-amd64-netinst.iso
> >
> > If SeaBIOS boots to the Debian installer menu then all is working well.
>
> Thank you.
> > > Lastly, will SeaBIOS built with your patches still work with older QEMU versions 
> > > without your QEMU patches?
> >
> > Yes indeed, and this is noted on the cover letter of the series.
>
> Right, I missed that.
> > > > Signed-off-by: Mark Cave-Ayland
> > > >
> > > > src/hw/esp-scsi.c | 36 ++++++++++++++++++++++--------------
> > > >   1 file changed, 22 insertions(+), 14 deletions(-)
>
> […]

ATB,

Mark.