diff mbox series

[v2,15/42] esp: introduce esp_pdma_read() and esp_pdma_write() functions

Message ID 20210209193018.31339-16-mark.cave-ayland@ilande.co.uk
State New
Headers show
Series esp: consolidate PDMA transfer buffers and other fixes | expand

Commit Message

Mark Cave-Ayland Feb. 9, 2021, 7:29 p.m. UTC 
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

Comments

Philippe Mathieu-Daudé Feb. 10, 2021, 10:51 p.m. UTC | #1 
Hi Mark,

On 2/9/21 8:29 PM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/scsi/esp.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index e7cf36f4b8..b0cba889a9 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -151,6 +151,20 @@ static uint8_t *get_pdma_buf(ESPState *s)
>      return NULL;
>  }
>  

Can you add get_pdma_len() (similar to get_pdma_buf) and ...

> +static uint8_t esp_pdma_read(ESPState *s)
> +{
> +    uint8_t *buf = get_pdma_buf(s);
> +

       assert(s->pdma_cur < get_pdma_len(s));

> +    return buf[s->pdma_cur++];
> +}
> +
> +static void esp_pdma_write(ESPState *s, uint8_t val)
> +{
> +    uint8_t *buf = get_pdma_buf(s);
> +

Ditto.

> +    buf[s->pdma_cur++] = val;
> +}

Otherwise:

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Mark Cave-Ayland Feb. 11, 2021, 8:01 a.m. UTC | #2 
On 10/02/2021 22:51, Philippe Mathieu-Daudé wrote:

> Hi Mark,
> 
> On 2/9/21 8:29 PM, Mark Cave-Ayland wrote:
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/scsi/esp.c | 28 ++++++++++++++++++++--------
>>   1 file changed, 20 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>> index e7cf36f4b8..b0cba889a9 100644
>> --- a/hw/scsi/esp.c
>> +++ b/hw/scsi/esp.c
>> @@ -151,6 +151,20 @@ static uint8_t *get_pdma_buf(ESPState *s)
>>       return NULL;
>>   }
>>   
> 
> Can you add get_pdma_len() (similar to get_pdma_buf) and ...
> 
>> +static uint8_t esp_pdma_read(ESPState *s)
>> +{
>> +    uint8_t *buf = get_pdma_buf(s);
>> +
> 
>         assert(s->pdma_cur < get_pdma_len(s));
> 
>> +    return buf[s->pdma_cur++];
>> +}
>> +
>> +static void esp_pdma_write(ESPState *s, uint8_t val)
>> +{
>> +    uint8_t *buf = get_pdma_buf(s);
>> +
> 
> Ditto.
> 
>> +    buf[s->pdma_cur++] = val;
>> +}
> 
> Otherwise:
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

One of the main purposes of this patchset is actually to completely remove all of 
these pdma_* variables and integrate everything into the existing FIFO and cmd 
buffers, so if you continue reading through the patchset you'll see that this soon 
disappears.

Even better towards the end you can see that both of these buffers are eventually 
replaced with QEMU's Fifo8 which has in-built assert()s to protect from underflow and 
overflow which should protect against memory corruption.


ATB,

Mark.
Philippe Mathieu-Daudé Feb. 11, 2021, 10:09 a.m. UTC | #3 
On 2/11/21 9:01 AM, Mark Cave-Ayland wrote:
> On 10/02/2021 22:51, Philippe Mathieu-Daudé wrote:
> 
>> Hi Mark,
>>
>> On 2/9/21 8:29 PM, Mark Cave-Ayland wrote:
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>   hw/scsi/esp.c | 28 ++++++++++++++++++++--------
>>>   1 file changed, 20 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>>> index e7cf36f4b8..b0cba889a9 100644
>>> --- a/hw/scsi/esp.c
>>> +++ b/hw/scsi/esp.c
>>> @@ -151,6 +151,20 @@ static uint8_t *get_pdma_buf(ESPState *s)
>>>       return NULL;
>>>   }
>>>   
>>
>> Can you add get_pdma_len() (similar to get_pdma_buf) and ...
>>
>>> +static uint8_t esp_pdma_read(ESPState *s)
>>> +{
>>> +    uint8_t *buf = get_pdma_buf(s);
>>> +
>>
>>         assert(s->pdma_cur < get_pdma_len(s));
>>
>>> +    return buf[s->pdma_cur++];
>>> +}
>>> +
>>> +static void esp_pdma_write(ESPState *s, uint8_t val)
>>> +{
>>> +    uint8_t *buf = get_pdma_buf(s);
>>> +
>>
>> Ditto.
>>
>>> +    buf[s->pdma_cur++] = val;
>>> +}
>>
>> Otherwise:
>>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> One of the main purposes of this patchset is actually to completely
> remove all of these pdma_* variables and integrate everything into the
> existing FIFO and cmd buffers, so if you continue reading through the
> patchset you'll see that this soon disappears.
> 
> Even better towards the end you can see that both of these buffers are
> eventually replaced with QEMU's Fifo8 which has in-built assert()s to
> protect from underflow and overflow which should protect against memory
> corruption.

OK great! I planed to review half of it (21/42) but was too tired so
stopped at this one :D My bad for missing in the cover:

- Now that the TC register represents the authoritative DMA transfer
  length, patches 14-25 work to eliminate the separate PDMA variables
  pdma_start, pdma_cur, pdma_len and separate PDMA buffers PDMA and CMD.
  The PDMA position variables can be replaced by the existing ESP cmdlen
  and ti_wptr/ti_rptr, whilst the FIFO (TI) buffer is used for incoming
  data with commands being accumulated in cmdbuf as per standard DMA
  requests.

Regards,

Phil.
Laurent Vivier March 1, 2021, 10:06 p.m. UTC | #4 
Le 09/02/2021 à 20:29, Mark Cave-Ayland a écrit :
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/scsi/esp.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index e7cf36f4b8..b0cba889a9 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -151,6 +151,20 @@ static uint8_t *get_pdma_buf(ESPState *s)
>      return NULL;
>  }
>  
> +static uint8_t esp_pdma_read(ESPState *s)
> +{
> +    uint8_t *buf = get_pdma_buf(s);
> +
> +    return buf[s->pdma_cur++];
> +}
> +
> +static void esp_pdma_write(ESPState *s, uint8_t val)
> +{
> +    uint8_t *buf = get_pdma_buf(s);
> +
> +    buf[s->pdma_cur++] = val;
> +}
> +
>  static int get_cmd_cb(ESPState *s)
>  {
>      int target;
> @@ -910,7 +924,6 @@ static void sysbus_esp_pdma_write(void *opaque, hwaddr addr,
>      SysBusESPState *sysbus = opaque;
>      ESPState *s = ESP(&sysbus->esp);
>      uint32_t dmalen = esp_get_tc(s);
> -    uint8_t *buf = get_pdma_buf(s);
>  
>      trace_esp_pdma_write(size);
>  
> @@ -919,13 +932,13 @@ static void sysbus_esp_pdma_write(void *opaque, hwaddr addr,
>      }
>      switch (size) {
>      case 1:
> -        buf[s->pdma_cur++] = val;
> +        esp_pdma_write(s, val);
>          s->pdma_len--;
>          dmalen--;
>          break;
>      case 2:
> -        buf[s->pdma_cur++] = val >> 8;
> -        buf[s->pdma_cur++] = val;
> +        esp_pdma_write(s, val >> 8);
> +        esp_pdma_write(s, val);
>          s->pdma_len -= 2;
>          dmalen -= 2;
>          break;
> @@ -944,7 +957,6 @@ static uint64_t sysbus_esp_pdma_read(void *opaque, hwaddr addr,
>      SysBusESPState *sysbus = opaque;
>      ESPState *s = ESP(&sysbus->esp);
>      uint32_t dmalen = esp_get_tc(s);
> -    uint8_t *buf = get_pdma_buf(s);
>      uint64_t val = 0;
>  
>      trace_esp_pdma_read(size);
> @@ -954,13 +966,13 @@ static uint64_t sysbus_esp_pdma_read(void *opaque, hwaddr addr,
>      }
>      switch (size) {
>      case 1:
> -        val = buf[s->pdma_cur++];
> +        val = esp_pdma_read(s);
>          s->pdma_len--;
>          dmalen--;
>          break;
>      case 2:
> -        val = buf[s->pdma_cur++];
> -        val = (val << 8) | buf[s->pdma_cur++];
> +        val = esp_pdma_read(s);
> +        val = (val << 8) | esp_pdma_read(s);
>          s->pdma_len -= 2;
>          dmalen -= 2;
>          break;
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>
diff mbox series

Patch

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index e7cf36f4b8..b0cba889a9 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -151,6 +151,20 @@  static uint8_t *get_pdma_buf(ESPState *s)
     return NULL;
 }
 
+static uint8_t esp_pdma_read(ESPState *s)
+{
+    uint8_t *buf = get_pdma_buf(s);
+
+    return buf[s->pdma_cur++];
+}
+
+static void esp_pdma_write(ESPState *s, uint8_t val)
+{
+    uint8_t *buf = get_pdma_buf(s);
+
+    buf[s->pdma_cur++] = val;
+}
+
 static int get_cmd_cb(ESPState *s)
 {
     int target;
@@ -910,7 +924,6 @@  static void sysbus_esp_pdma_write(void *opaque, hwaddr addr,
     SysBusESPState *sysbus = opaque;
     ESPState *s = ESP(&sysbus->esp);
     uint32_t dmalen = esp_get_tc(s);
-    uint8_t *buf = get_pdma_buf(s);
 
     trace_esp_pdma_write(size);
 
@@ -919,13 +932,13 @@  static void sysbus_esp_pdma_write(void *opaque, hwaddr addr,
     }
     switch (size) {
     case 1:
-        buf[s->pdma_cur++] = val;
+        esp_pdma_write(s, val);
         s->pdma_len--;
         dmalen--;
         break;
     case 2:
-        buf[s->pdma_cur++] = val >> 8;
-        buf[s->pdma_cur++] = val;
+        esp_pdma_write(s, val >> 8);
+        esp_pdma_write(s, val);
         s->pdma_len -= 2;
         dmalen -= 2;
         break;
@@ -944,7 +957,6 @@  static uint64_t sysbus_esp_pdma_read(void *opaque, hwaddr addr,
     SysBusESPState *sysbus = opaque;
     ESPState *s = ESP(&sysbus->esp);
     uint32_t dmalen = esp_get_tc(s);
-    uint8_t *buf = get_pdma_buf(s);
     uint64_t val = 0;
 
     trace_esp_pdma_read(size);
@@ -954,13 +966,13 @@  static uint64_t sysbus_esp_pdma_read(void *opaque, hwaddr addr,
     }
     switch (size) {
     case 1:
-        val = buf[s->pdma_cur++];
+        val = esp_pdma_read(s);
         s->pdma_len--;
         dmalen--;
         break;
     case 2:
-        val = buf[s->pdma_cur++];
-        val = (val << 8) | buf[s->pdma_cur++];
+        val = esp_pdma_read(s);
+        val = (val << 8) | esp_pdma_read(s);
         s->pdma_len -= 2;
         dmalen -= 2;
         break;