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

[Philippe Mathieu-Daudé]

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.