From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

In the cases where mixed DMA/non-DMA transfers are used or no data is
available, it is possible for the calculated transfer length to be zero.
Only call the dma_memory_read function where the transfer length is
non-zero to avoid invoking the DMA engine for a zero length transfer
which can have side-effects (along with generating additional tracing
noise).

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-ID: <20250711204636.542964-5-mark.cave-ayland@ilande.co.uk>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/scsi/esp.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 62ba4061492..ec9fcbeddf4 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -487,8 +487,10 @@ static void esp_do_dma(ESPState *s)
     case STAT_MO:
         if (s->dma_memory_read) {
             len = MIN(len, fifo8_num_free(&s->cmdfifo));
-            s->dma_memory_read(s->dma_opaque, buf, len);
-            esp_set_tc(s, esp_get_tc(s) - len);
+            if (len) {
+                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, buf, fifo8_num_used(&s->fifo));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
@@ -541,9 +543,11 @@ static void esp_do_dma(ESPState *s)
         trace_esp_do_dma(cmdlen, len);
         if (s->dma_memory_read) {
             len = MIN(len, fifo8_num_free(&s->cmdfifo));
-            s->dma_memory_read(s->dma_opaque, buf, len);
-            fifo8_push_all(&s->cmdfifo, buf, len);
-            esp_set_tc(s, esp_get_tc(s) - len);
+            if (len) {
+                s->dma_memory_read(s->dma_opaque, buf, len);
+                fifo8_push_all(&s->cmdfifo, buf, len);
+                esp_set_tc(s, esp_get_tc(s) - len);
+            }
         } else {
             len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
@@ -572,8 +576,10 @@ static void esp_do_dma(ESPState *s)
         switch (s->rregs[ESP_CMD]) {
         case CMD_TI | CMD_DMA:
             if (s->dma_memory_read) {
-                s->dma_memory_read(s->dma_opaque, s->async_buf, len);
-                esp_set_tc(s, esp_get_tc(s) - len);
+                if (len) {
+                    s->dma_memory_read(s->dma_opaque, s->async_buf, len);
+                    esp_set_tc(s, esp_get_tc(s) - len);
+                }
             } else {
                 /* Copy FIFO data to device */
                 len = MIN(s->async_len, ESP_FIFO_SZ);
-- 
2.49.0

Hi Mark,

On 15/7/25 08:19, Philippe Mathieu-Daudé wrote:
> From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> In the cases where mixed DMA/non-DMA transfers are used or no data is
> available, it is possible for the calculated transfer length to be zero.
> Only call the dma_memory_read function where the transfer length is
> non-zero to avoid invoking the DMA engine for a zero length transfer
> which can have side-effects (along with generating additional tracing
> noise).
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Message-ID: <20250711204636.542964-5-mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/scsi/esp.c | 20 +++++++++++++-------
>   1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index 62ba4061492..ec9fcbeddf4 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -487,8 +487,10 @@ static void esp_do_dma(ESPState *s)
>       case STAT_MO:
>           if (s->dma_memory_read) {
>               len = MIN(len, fifo8_num_free(&s->cmdfifo));
> -            s->dma_memory_read(s->dma_opaque, buf, len);
> -            esp_set_tc(s, esp_get_tc(s) - len);
> +            if (len) {
> +                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, buf, fifo8_num_used(&s->fifo));
>               len = MIN(fifo8_num_free(&s->cmdfifo), len);
             }
             fifo8_push_all(&s->cmdfifo, buf, len);

Coverity reported access to uninitialized buf[]:

 >>>     CID 1612373:         Uninitialized variables  (UNINIT)
 >>>     Using uninitialized value "*buf" when calling "fifo8_push_all".

Do you mind having a look?

Thanks,

Phil.

On Thu, 17 Jul 2025 at 12:51, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Mark,
>
> On 15/7/25 08:19, Philippe Mathieu-Daudé wrote:
> > From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >
> > In the cases where mixed DMA/non-DMA transfers are used or no data is
> > available, it is possible for the calculated transfer length to be zero.
> > Only call the dma_memory_read function where the transfer length is
> > non-zero to avoid invoking the DMA engine for a zero length transfer
> > which can have side-effects (along with generating additional tracing
> > noise).
> >
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Message-ID: <20250711204636.542964-5-mark.cave-ayland@ilande.co.uk>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >   hw/scsi/esp.c | 20 +++++++++++++-------
> >   1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> > index 62ba4061492..ec9fcbeddf4 100644
> > --- a/hw/scsi/esp.c
> > +++ b/hw/scsi/esp.c
> > @@ -487,8 +487,10 @@ static void esp_do_dma(ESPState *s)
> >       case STAT_MO:
> >           if (s->dma_memory_read) {
> >               len = MIN(len, fifo8_num_free(&s->cmdfifo));
> > -            s->dma_memory_read(s->dma_opaque, buf, len);
> > -            esp_set_tc(s, esp_get_tc(s) - len);
> > +            if (len) {
> > +                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, buf, fifo8_num_used(&s->fifo));
> >               len = MIN(fifo8_num_free(&s->cmdfifo), len);
>              }
>              fifo8_push_all(&s->cmdfifo, buf, len);
>
> Coverity reported access to uninitialized buf[]:
>
>  >>>     CID 1612373:         Uninitialized variables  (UNINIT)
>  >>>     Using uninitialized value "*buf" when calling "fifo8_push_all".
>
> Do you mind having a look?

I think this is a false positive (and marked it that way in
the Coverity Scan UI). Coverity is complaining that
we might access buf[] uninitialized, but in the code path
it is complaining about we know that len is zero. The
fifo8_push_all() does a "memcpy(&fifo->data[start], data, num)"
and if num is 0 that is valid and won't access anything in buf[].

We could I suppose make fifo8_push_all() return early for the
num == 0 case, just to make it clearer that it's supposed to work.

thanks
-- PMM

On 17/07/2025 12:58, Peter Maydell wrote:

> On Thu, 17 Jul 2025 at 12:51, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Mark,
>>
>> On 15/7/25 08:19, Philippe Mathieu-Daudé wrote:
>>> From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>
>>> In the cases where mixed DMA/non-DMA transfers are used or no data is
>>> available, it is possible for the calculated transfer length to be zero.
>>> Only call the dma_memory_read function where the transfer length is
>>> non-zero to avoid invoking the DMA engine for a zero length transfer
>>> which can have side-effects (along with generating additional tracing
>>> noise).
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Message-ID: <20250711204636.542964-5-mark.cave-ayland@ilande.co.uk>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>    hw/scsi/esp.c | 20 +++++++++++++-------
>>>    1 file changed, 13 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>>> index 62ba4061492..ec9fcbeddf4 100644
>>> --- a/hw/scsi/esp.c
>>> +++ b/hw/scsi/esp.c
>>> @@ -487,8 +487,10 @@ static void esp_do_dma(ESPState *s)
>>>        case STAT_MO:
>>>            if (s->dma_memory_read) {
>>>                len = MIN(len, fifo8_num_free(&s->cmdfifo));
>>> -            s->dma_memory_read(s->dma_opaque, buf, len);
>>> -            esp_set_tc(s, esp_get_tc(s) - len);
>>> +            if (len) {
>>> +                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, buf, fifo8_num_used(&s->fifo));
>>>                len = MIN(fifo8_num_free(&s->cmdfifo), len);
>>               }
>>               fifo8_push_all(&s->cmdfifo, buf, len);
>>
>> Coverity reported access to uninitialized buf[]:
>>
>>   >>>     CID 1612373:         Uninitialized variables  (UNINIT)
>>   >>>     Using uninitialized value "*buf" when calling "fifo8_push_all".
>>
>> Do you mind having a look?
> 
> I think this is a false positive (and marked it that way in
> the Coverity Scan UI). Coverity is complaining that
> we might access buf[] uninitialized, but in the code path
> it is complaining about we know that len is zero. The
> fifo8_push_all() does a "memcpy(&fifo->data[start], data, num)"
> and if num is 0 that is valid and won't access anything in buf[].
> 
> We could I suppose make fifo8_push_all() return early for the
> num == 0 case, just to make it clearer that it's supposed to work.

Thanks both!

I can certainly look to update fifo8_push_all() if you think it would be a better 
solution?


ATB,

Mark.