Re: [PATCH v3 00/17] [for-9.0] esp: avoid explicit setting of DRQ within ESP state machine

[Paolo Bonzini]

On Sun, Mar 24, 2024 at 8:17 PM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> Patches 1-4 update existing users of esp_fifo_pop_buf() and esp_fifo_pop() 
> with
> the internal cmdfifo to use the underlying Fifo8 device directly. The aim is
> to ensure that all the esp_fifo_*() functions only operate on the ESP's 
> hardware
> FIFO.
>
> Patches 5-6 update esp_fifo_push() and esp_fifo_pop() to take ESPState 
> directly
> as a parameter to prevent any usage that doesn't reference the ESP hardware
> FIFO.
>
> Patch 7 ensures that any usage of fifo8_push() for the ESP hardware FIFO is
> updated to use esp_fifo_push() instead.
>
> Patch 8 updates esp_fifo_pop_buf() to take ESPState directly as a parameter
> to prevent any usage that doesn't reference the ESP hardware FIFO.
>
> Patch 9 introduces the esp_fifo_push_buf() function for pushing multiple bytes
> to the ESP hardware FIFO, and updates callers to use it accordingly.
>
> Patches 10-12 incorporate additional protection to prevent FIFO assert()s and 
> a
> cmdfifo buffer overflow discovered via fuzzing.
>
> Patch 13 is just code movement which avoids the use of a forward declaration 
> whilst
> also making it easier to locate the mapping between ESP SCSI phases and their
> names.
>
> Patches 14 introduce a new esp_update_drq() function that implements the above
> DRQ logic which is called by both esp_fifo_{push,pop}_buf().
>
> Patch 15 updates esp_fifo_push() and esp_fifo_pop() to use the new 
> esp_update_drq()
> function. At this point all reads/writes to the ESP FIFO use the esp_fifo_*()
> functions and will set DRQ correctly.
>
> Patch 16 is a small update to the logic in esp_pdma_write() to ensure that
> esp_fifo_push() is always called for PDMA writes to the FIFO, thereby ensuring
> that esp_update_drq() remains correct even in the case of FIFO overflow.
>
> Finally patch 17 removes all manual calls to esp_raise_drq() and 
> esp_lower_drq()
> since the DRQ signal is now updated correctly upon each FIFO read/write 
> access.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> v3:
> - Rebase onto master
> - Add patch 1 to move the internals of esp_fifo_pop_buf() to a new
>   esp_fifo8_pop_buf() function. This allows the FIFO wrap logic to still be
>   used for managing cmdfifo
> - Rework esp_cdb_length() into esp_cdb_ready() as suggested by Paolo in patch
>   11
> - Update the logic in patch 12 to use the new esp_cdb_ready() function
>
> v2:
> - Rebase onto master
> - Add patches 9-12 to handle FIFO assert()s and cmdfifo overflow as reported 
> by
>   Chuhong Yuan <hslester96@gmail.com>
>
>
> Mark Cave-Ayland (17):
>   esp.c: move esp_fifo_pop_buf() internals to new esp_fifo8_pop_buf()
>     function
>   esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in
>     do_command_phase()
>   esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in
>     do_message_phase()
>   esp.c: replace cmdfifo use of esp_fifo_pop() in do_message_phase()
>   esp.c: change esp_fifo_push() to take ESPState
>   esp.c: change esp_fifo_pop() to take ESPState
>   esp.c: use esp_fifo_push() instead of fifo8_push()
>   esp.c: change esp_fifo_pop_buf() to take ESPState
>   esp.c: introduce esp_fifo_push_buf() function for pushing to the FIFO
>   esp.c: don't assert() if FIFO empty when executing non-DMA SELATNS
>   esp.c: rework esp_cdb_length() into esp_cdb_ready()
>   esp.c: prevent cmdfifo overflow in esp_cdb_ready()
>   esp.c: move esp_set_phase() and esp_get_phase() towards the beginning
>     of the file
>   esp.c: introduce esp_update_drq() and update esp_fifo_{push,pop}_buf()
>     to use it
>   esp.c: update esp_fifo_{push,pop}() to call esp_update_drq()
>   esp.c: ensure esp_pdma_write() always calls esp_fifo_push()
>   esp.c: remove explicit setting of DRQ within ESP state machine
>
>  hw/scsi/esp.c | 223 ++++++++++++++++++++++++++++++++------------------
>  1 file changed, 142 insertions(+), 81 deletions(-)
>
> --
> 2.39.2
>