esp: rework ESP emulation to use a SCSI phase-based state machine

[PATCH 09/88] esp: update TC check logic in do_dma_pdma_cb() to check for TC == 0

Posted by Mark Cave-Ayland 10 months ago

Invert the logic so that the end of DMA transfer check becomes one that checks
for TC == 0 in the from device path in do_dma_pdma_cb().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index fecfef7c89..63c828c1b2 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -602,21 +602,19 @@ static void do_dma_pdma_cb(ESPState *s)
             return;
         }
 
-        if (esp_get_tc(s) != 0) {
-            /* Copy device data to FIFO */
-            len = MIN(s->async_len, esp_get_tc(s));
-            len = MIN(len, fifo8_num_free(&s->fifo));
-            fifo8_push_all(&s->fifo, s->async_buf, len);
-            s->async_buf += len;
-            s->async_len -= len;
-            s->ti_size -= len;
-            esp_set_tc(s, esp_get_tc(s) - len);
-            return;
+        if (esp_get_tc(s) == 0) {
+            esp_lower_drq(s);
+            esp_dma_done(s);
         }
 
-        /* Partially filled a scsi buffer. Complete immediately.  */
-        esp_lower_drq(s);
-        esp_dma_done(s);
+        /* Copy device data to FIFO */
+        len = MIN(s->async_len, esp_get_tc(s));
+        len = MIN(len, fifo8_num_free(&s->fifo));
+        fifo8_push_all(&s->fifo, s->async_buf, len);
+        s->async_buf += len;
+        s->async_len -= len;
+        s->ti_size -= len;
+        esp_set_tc(s, esp_get_tc(s) - len);
     }
 }
 
-- 
2.39.2

Re: [PATCH 09/88] esp: update TC check logic in do_dma_pdma_cb() to check for TC == 0

Posted by Paolo Bonzini 9 months, 2 weeks ago

On Fri, Jan 12, 2024 at 1:55 PM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Invert the logic so that the end of DMA transfer check becomes one that checks
> for TC == 0 in the from device path in do_dma_pdma_cb().
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/scsi/esp.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index fecfef7c89..63c828c1b2 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -602,21 +602,19 @@ static void do_dma_pdma_cb(ESPState *s)
>              return;
>          }
>
> -        if (esp_get_tc(s) != 0) {
> -            /* Copy device data to FIFO */
> -            len = MIN(s->async_len, esp_get_tc(s));
> -            len = MIN(len, fifo8_num_free(&s->fifo));
> -            fifo8_push_all(&s->fifo, s->async_buf, len);
> -            s->async_buf += len;
> -            s->async_len -= len;
> -            s->ti_size -= len;
> -            esp_set_tc(s, esp_get_tc(s) - len);
> -            return;
> +        if (esp_get_tc(s) == 0) {
> +            esp_lower_drq(s);
> +            esp_dma_done(s);
>          }

I'm only doing a cursory review, but shouldn't there be a return here?

Paolo

>
> -        /* Partially filled a scsi buffer. Complete immediately.  */
> -        esp_lower_drq(s);
> -        esp_dma_done(s);
> +        /* Copy device data to FIFO */
> +        len = MIN(s->async_len, esp_get_tc(s));
> +        len = MIN(len, fifo8_num_free(&s->fifo));
> +        fifo8_push_all(&s->fifo, s->async_buf, len);
> +        s->async_buf += len;
> +        s->async_len -= len;
> +        s->ti_size -= len;
> +        esp_set_tc(s, esp_get_tc(s) - len);
>      }
>  }
>
> --
> 2.39.2
>

Re: [PATCH 09/88] esp: update TC check logic in do_dma_pdma_cb() to check for TC == 0

Posted by Mark Cave-Ayland 9 months, 2 weeks ago

On 01/02/2024 10:46, Paolo Bonzini wrote:

> On Fri, Jan 12, 2024 at 1:55 PM Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> Invert the logic so that the end of DMA transfer check becomes one that checks
>> for TC == 0 in the from device path in do_dma_pdma_cb().
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/scsi/esp.c | 24 +++++++++++-------------
>>   1 file changed, 11 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>> index fecfef7c89..63c828c1b2 100644
>> --- a/hw/scsi/esp.c
>> +++ b/hw/scsi/esp.c
>> @@ -602,21 +602,19 @@ static void do_dma_pdma_cb(ESPState *s)
>>               return;
>>           }
>>
>> -        if (esp_get_tc(s) != 0) {
>> -            /* Copy device data to FIFO */
>> -            len = MIN(s->async_len, esp_get_tc(s));
>> -            len = MIN(len, fifo8_num_free(&s->fifo));
>> -            fifo8_push_all(&s->fifo, s->async_buf, len);
>> -            s->async_buf += len;
>> -            s->async_len -= len;
>> -            s->ti_size -= len;
>> -            esp_set_tc(s, esp_get_tc(s) - len);
>> -            return;
>> +        if (esp_get_tc(s) == 0) {
>> +            esp_lower_drq(s);
>> +            esp_dma_done(s);
>>           }
> 
> I'm only doing a cursory review, but shouldn't there be a return here?
> 
> Paolo

(goes and looks)

Possibly there should, but my guess is that adding the return at that particular 
point in time at the series broke one of my reference images. In particular MacOS is 
notorious for requesting data transfers of X len, and setting the TC either too high 
or too low and let the in-built OS recovery logic handle these cases.

The tricky part is that as per the cover note, making expected changes at an earlier 
point in the series tends to cause things to break. Another thing to bear in mind is 
that one of the main objectives of the series to completely remove all the 
PDMA-specific callbacks including do_dma_pdma_cb(), so you'll see this function 
disappear completely in a later patch.

It probably helps to compare the existing code at 
https://gitlab.com/qemu-project/qemu/-/blob/master/hw/scsi/esp.c against the version 
from this series at 
https://gitlab.com/mcayland/qemu/-/blob/esp-rework-v2/hw/scsi/esp.c to get a feeling 
where the series is going, as in order to keep my reference images bisectable the 
journey from start to finish occurs in a fairly roundabout way.

>> -        /* Partially filled a scsi buffer. Complete immediately.  */
>> -        esp_lower_drq(s);
>> -        esp_dma_done(s);
>> +        /* Copy device data to FIFO */
>> +        len = MIN(s->async_len, esp_get_tc(s));
>> +        len = MIN(len, fifo8_num_free(&s->fifo));
>> +        fifo8_push_all(&s->fifo, s->async_buf, len);
>> +        s->async_buf += len;
>> +        s->async_len -= len;
>> +        s->ti_size -= len;
>> +        esp_set_tc(s, esp_get_tc(s) - len);
>>       }
>>   }
>>
>> --
>> 2.39.2

ATB,

Mark.

Re: [PATCH 09/88] esp: update TC check logic in do_dma_pdma_cb() to check for TC == 0

Posted by Paolo Bonzini 9 months, 2 weeks ago

Il gio 1 feb 2024, 12:25 Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
ha scritto:

> On 01/02/2024 10:46, Paolo Bonzini wrote:
>
> > On Fri, Jan 12, 2024 at 1:55 PM Mark Cave-Ayland
> > <mark.cave-ayland@ilande.co.uk> wrote:
> >>
> >> Invert the logic so that the end of DMA transfer check becomes one that
> checks
> >> for TC == 0 in the from device path in do_dma_pdma_cb().
> >>
> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >> ---
> >>   hw/scsi/esp.c | 24 +++++++++++-------------
> >>   1 file changed, 11 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> >> index fecfef7c89..63c828c1b2 100644
> >> --- a/hw/scsi/esp.c
> >> +++ b/hw/scsi/esp.c
> >> @@ -602,21 +602,19 @@ static void do_dma_pdma_cb(ESPState *s)
> >>               return;
> >>           }
> >>
> >> -        if (esp_get_tc(s) != 0) {
> >> -            /* Copy device data to FIFO */
> >> -            len = MIN(s->async_len, esp_get_tc(s));
> >> -            len = MIN(len, fifo8_num_free(&s->fifo));
> >> -            fifo8_push_all(&s->fifo, s->async_buf, len);
> >> -            s->async_buf += len;
> >> -            s->async_len -= len;
> >> -            s->ti_size -= len;
> >> -            esp_set_tc(s, esp_get_tc(s) - len);
> >> -            return;
> >> +        if (esp_get_tc(s) == 0) {
> >> +            esp_lower_drq(s);
> >> +            esp_dma_done(s);
> >>           }
> >
> > I'm only doing a cursory review, but shouldn't there be a return here?
> >
> > Paolo
>
> (goes and looks)
>
> Possibly there should, but my guess is that adding the return at that
> particular
> point in time at the series broke one of my reference images. In
> particular MacOS is
> notorious for requesting data transfers of X len, and setting the TC
> either too high
> or too low and let the in-built OS recovery logic handle these cases.
>

Absolutely, just noticing that there is a functional change but the commit
message showed it as a refactoring only.

The fact that this is bisectable is quite insane, and I am not asking for
any code changes. TBH I wouldn't have blamed you if you just sent a patch
to create esp2.c and a patch to delete esp.c...

Paolo


The tricky part is that as per the cover note, making expected changes at
> an earlier
> point in the series tends to cause things to break. Another thing to bear
> in mind is
> that one of the main objectives of the series to completely remove all the
> PDMA-specific callbacks including do_dma_pdma_cb(), so you'll see this
> function
> disappear completely in a later patch.
>
> It probably helps to compare the existing code at
> https://gitlab.com/qemu-project/qemu/-/blob/master/hw/scsi/esp.c against
> the version
> from this series at
> https://gitlab.com/mcayland/qemu/-/blob/esp-rework-v2/hw/scsi/esp.c to
> get a feeling
> where the series is going, as in order to keep my reference images
> bisectable the
> journey from start to finish occurs in a fairly roundabout way.
>
> >> -        /* Partially filled a scsi buffer. Complete immediately.  */
> >> -        esp_lower_drq(s);
> >> -        esp_dma_done(s);
> >> +        /* Copy device data to FIFO */
> >> +        len = MIN(s->async_len, esp_get_tc(s));
> >> +        len = MIN(len, fifo8_num_free(&s->fifo));
> >> +        fifo8_push_all(&s->fifo, s->async_buf, len);
> >> +        s->async_buf += len;
> >> +        s->async_len -= len;
> >> +        s->ti_size -= len;
> >> +        esp_set_tc(s, esp_get_tc(s) - len);
> >>       }
> >>   }
> >>
> >> --
> >> 2.39.2
>
>
> ATB,
>
> Mark.
>
>

Re: [PATCH 09/88] esp: update TC check logic in do_dma_pdma_cb() to check for TC == 0

Posted by Mark Cave-Ayland 9 months, 1 week ago

On 01/02/2024 11:36, Paolo Bonzini wrote:

> Il gio 1 feb 2024, 12:25 Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk 
> <mailto:mark.cave-ayland@ilande.co.uk>> ha scritto:
> 
>     On 01/02/2024 10:46, Paolo Bonzini wrote:
> 
>      > On Fri, Jan 12, 2024 at 1:55 PM Mark Cave-Ayland
>      > <mark.cave-ayland@ilande.co.uk <mailto:mark.cave-ayland@ilande.co.uk>> wrote:
>      >>
>      >> Invert the logic so that the end of DMA transfer check becomes one that checks
>      >> for TC == 0 in the from device path in do_dma_pdma_cb().
>      >>
>      >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk
>     <mailto:mark.cave-ayland@ilande.co.uk>>
>      >> ---
>      >>   hw/scsi/esp.c | 24 +++++++++++-------------
>      >>   1 file changed, 11 insertions(+), 13 deletions(-)
>      >>
>      >> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>      >> index fecfef7c89..63c828c1b2 100644
>      >> --- a/hw/scsi/esp.c
>      >> +++ b/hw/scsi/esp.c
>      >> @@ -602,21 +602,19 @@ static void do_dma_pdma_cb(ESPState *s)
>      >>               return;
>      >>           }
>      >>
>      >> -        if (esp_get_tc(s) != 0) {
>      >> -            /* Copy device data to FIFO */
>      >> -            len = MIN(s->async_len, esp_get_tc(s));
>      >> -            len = MIN(len, fifo8_num_free(&s->fifo));
>      >> -            fifo8_push_all(&s->fifo, s->async_buf, len);
>      >> -            s->async_buf += len;
>      >> -            s->async_len -= len;
>      >> -            s->ti_size -= len;
>      >> -            esp_set_tc(s, esp_get_tc(s) - len);
>      >> -            return;
>      >> +        if (esp_get_tc(s) == 0) {
>      >> +            esp_lower_drq(s);
>      >> +            esp_dma_done(s);
>      >>           }
>      >
>      > I'm only doing a cursory review, but shouldn't there be a return here?
>      >
>      > Paolo
> 
>     (goes and looks)
> 
>     Possibly there should, but my guess is that adding the return at that particular
>     point in time at the series broke one of my reference images. In particular MacOS is
>     notorious for requesting data transfers of X len, and setting the TC either too high
>     or too low and let the in-built OS recovery logic handle these cases.
> 
> 
> Absolutely, just noticing that there is a functional change but the commit message 
> showed it as a refactoring only.

Would you like me to come up with a reworded commit message here?

> The fact that this is bisectable is quite insane, and I am not asking for any code 
> changes. TBH I wouldn't have blamed you if you just sent a patch to create esp2.c and 
> a patch to delete esp.c...

Heh... spending a chunk of time rewriting the emulation of a 30 year-old SCSI 
controller is probably an odd choice, but the result is something that is 
considerably more maintainable than the current implementation. Definitely having a 
bisectible history helps in the case of finding a regression, but so far I'd say the 
result is a huge improvement on what is already there.

Anything else that you'd like me to change before the series can be considered for merge?


ATB,

Mark.

Re: [PATCH 09/88] esp: update TC check logic in do_dma_pdma_cb() to check for TC == 0

Posted by Paolo Bonzini 9 months, 1 week ago

On Thu, Feb 8, 2024 at 10:46 AM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 01/02/2024 11:36, Paolo Bonzini wrote:
>
> > Il gio 1 feb 2024, 12:25 Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk
> > <mailto:mark.cave-ayland@ilande.co.uk>> ha scritto:
> >
> >     On 01/02/2024 10:46, Paolo Bonzini wrote:
> >
> >      > On Fri, Jan 12, 2024 at 1:55 PM Mark Cave-Ayland
> >      > <mark.cave-ayland@ilande.co.uk <mailto:mark.cave-ayland@ilande.co.uk>> wrote:
> >      >>
> >      >> Invert the logic so that the end of DMA transfer check becomes one that checks
> >      >> for TC == 0 in the from device path in do_dma_pdma_cb().
> >      >>
> >      >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk
> >     <mailto:mark.cave-ayland@ilande.co.uk>>
> >      >> ---
> >      >>   hw/scsi/esp.c | 24 +++++++++++-------------
> >      >>   1 file changed, 11 insertions(+), 13 deletions(-)
> >      >>
> >      >> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> >      >> index fecfef7c89..63c828c1b2 100644
> >      >> --- a/hw/scsi/esp.c
> >      >> +++ b/hw/scsi/esp.c
> >      >> @@ -602,21 +602,19 @@ static void do_dma_pdma_cb(ESPState *s)
> >      >>               return;
> >      >>           }
> >      >>
> >      >> -        if (esp_get_tc(s) != 0) {
> >      >> -            /* Copy device data to FIFO */
> >      >> -            len = MIN(s->async_len, esp_get_tc(s));
> >      >> -            len = MIN(len, fifo8_num_free(&s->fifo));
> >      >> -            fifo8_push_all(&s->fifo, s->async_buf, len);
> >      >> -            s->async_buf += len;
> >      >> -            s->async_len -= len;
> >      >> -            s->ti_size -= len;
> >      >> -            esp_set_tc(s, esp_get_tc(s) - len);
> >      >> -            return;
> >      >> +        if (esp_get_tc(s) == 0) {
> >      >> +            esp_lower_drq(s);
> >      >> +            esp_dma_done(s);
> >      >>           }
> >      >
> >      > I'm only doing a cursory review, but shouldn't there be a return here?
> >      >
> >      > Paolo
> >
> >     (goes and looks)
> >
> >     Possibly there should, but my guess is that adding the return at that particular
> >     point in time at the series broke one of my reference images. In particular MacOS is
> >     notorious for requesting data transfers of X len, and setting the TC either too high
> >     or too low and let the in-built OS recovery logic handle these cases.
> >
> >
> > Absolutely, just noticing that there is a functional change but the commit message
> > showed it as a refactoring only.
>
> Would you like me to come up with a reworded commit message here?

If you want to change it, it's okay because len is zero at this point
and the code after the "if" therefore does nothing. And the "if" will
become esp_dma_ti_check() so adding a return is kind of messy here.

> > The fact that this is bisectable is quite insane, and I am not asking for any code
> > changes. TBH I wouldn't have blamed you if you just sent a patch to create esp2.c and
> > a patch to delete esp.c...
>
> Heh... spending a chunk of time rewriting the emulation of a 30 year-old SCSI
> controller is probably an odd choice, but the result is something that is
> considerably more maintainable than the current implementation.

I absolutely agree, you went above and beyond and sorry if it wasn't
clear from the joke.

> Anything else that you'd like me to change before the series can be considered for merge?

No, go ahead!

Paolo

Re: [PATCH 09/88] esp: update TC check logic in do_dma_pdma_cb() to check for TC == 0

Posted by Mark Cave-Ayland 9 months, 1 week ago

On 08/02/2024 18:11, Paolo Bonzini wrote:

> On Thu, Feb 8, 2024 at 10:46 AM Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> On 01/02/2024 11:36, Paolo Bonzini wrote:
>>
>>> Il gio 1 feb 2024, 12:25 Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk
>>> <mailto:mark.cave-ayland@ilande.co.uk>> ha scritto:
>>>
>>>      On 01/02/2024 10:46, Paolo Bonzini wrote:
>>>
>>>       > On Fri, Jan 12, 2024 at 1:55 PM Mark Cave-Ayland
>>>       > <mark.cave-ayland@ilande.co.uk <mailto:mark.cave-ayland@ilande.co.uk>> wrote:
>>>       >>
>>>       >> Invert the logic so that the end of DMA transfer check becomes one that checks
>>>       >> for TC == 0 in the from device path in do_dma_pdma_cb().
>>>       >>
>>>       >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk
>>>      <mailto:mark.cave-ayland@ilande.co.uk>>
>>>       >> ---
>>>       >>   hw/scsi/esp.c | 24 +++++++++++-------------
>>>       >>   1 file changed, 11 insertions(+), 13 deletions(-)
>>>       >>
>>>       >> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>>>       >> index fecfef7c89..63c828c1b2 100644
>>>       >> --- a/hw/scsi/esp.c
>>>       >> +++ b/hw/scsi/esp.c
>>>       >> @@ -602,21 +602,19 @@ static void do_dma_pdma_cb(ESPState *s)
>>>       >>               return;
>>>       >>           }
>>>       >>
>>>       >> -        if (esp_get_tc(s) != 0) {
>>>       >> -            /* Copy device data to FIFO */
>>>       >> -            len = MIN(s->async_len, esp_get_tc(s));
>>>       >> -            len = MIN(len, fifo8_num_free(&s->fifo));
>>>       >> -            fifo8_push_all(&s->fifo, s->async_buf, len);
>>>       >> -            s->async_buf += len;
>>>       >> -            s->async_len -= len;
>>>       >> -            s->ti_size -= len;
>>>       >> -            esp_set_tc(s, esp_get_tc(s) - len);
>>>       >> -            return;
>>>       >> +        if (esp_get_tc(s) == 0) {
>>>       >> +            esp_lower_drq(s);
>>>       >> +            esp_dma_done(s);
>>>       >>           }
>>>       >
>>>       > I'm only doing a cursory review, but shouldn't there be a return here?
>>>       >
>>>       > Paolo
>>>
>>>      (goes and looks)
>>>
>>>      Possibly there should, but my guess is that adding the return at that particular
>>>      point in time at the series broke one of my reference images. In particular MacOS is
>>>      notorious for requesting data transfers of X len, and setting the TC either too high
>>>      or too low and let the in-built OS recovery logic handle these cases.
>>>
>>>
>>> Absolutely, just noticing that there is a functional change but the commit message
>>> showed it as a refactoring only.
>>
>> Would you like me to come up with a reworded commit message here?
> 
> If you want to change it, it's okay because len is zero at this point
> and the code after the "if" therefore does nothing. And the "if" will
> become esp_dma_ti_check() so adding a return is kind of messy here.

Okay I'll leave it as it is - I'm not too worried about the detail here, since that 
particular section ends up being removed.

>>> The fact that this is bisectable is quite insane, and I am not asking for any code
>>> changes. TBH I wouldn't have blamed you if you just sent a patch to create esp2.c and
>>> a patch to delete esp.c...
>>
>> Heh... spending a chunk of time rewriting the emulation of a 30 year-old SCSI
>> controller is probably an odd choice, but the result is something that is
>> considerably more maintainable than the current implementation.
> 
> I absolutely agree, you went above and beyond and sorry if it wasn't
> clear from the joke.

No worries, I wasn't taking it seriously either :)

>> Anything else that you'd like me to change before the series can be considered for merge?
> 
> No, go ahead!

Thanks! I'll take that as a nod to send a PR over the next few days.


ATB,

Mark.