qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/16] [for-9.0] esp: avoid explicit setting of DRQ within ESP state machine
@ 2024-03-13  8:57 Mark Cave-Ayland
  2024-03-13  8:57 ` [PATCH v2 01/16] esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_command_phase() Mark Cave-Ayland
                   ` (15 more replies)
  0 siblings, 16 replies; 34+ messages in thread
From: Mark Cave-Ayland @ 2024-03-13  8:57 UTC (permalink / raw)
  To: pbonzini, fam, laurent, qemu-devel

[MCA: Since v1 I've received a few reports of FIFO assert()s being triggered and a
cmdfifo buffer overflow discovered by fuzzing the updated ESP code. The updating of
all FIFO push/pop operations to use the esp_fifo_*() functions in this series
provides protection against this, and in conjunction with new patches 9-11 is
enough to prevent all qtest reproducers that I have been sent.

For these reasons I would recommend that this series be applied for 9.0. Many thanks
to Chuhong Yuan <hslester96@gmail.com> for reporting the issues and providing qtest
reproducers.]

The ESP device has a DRQ (DMA request) signal that is used to handle flow control
during DMA transfers. At the moment the DRQ signal is explicitly raised and
lowered at various points in the ESP state machine as required, rather than
implementing the logic described in the datasheet:

"DREQ will remain true as long as either the FIFO contains at least one word (or
one byte if 8-bit mode) to send to memory during DMA read, or has room for one more
word (or byte if 8-bit mode) in the FIFO during DMA write."

This series updates the ESP device to use the same logic as described above which
also fixes a couple of outstanding GitLab issues related to recovery handling
during FIFO overflow on 68k Macs using PDMA.

Patches 1-3 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 4-5 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 6 ensures that any usage of fifo8_push() for the ESP hardware FIFO is
updated to use esp_fifo_push() instead.

Patch 7 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 8 introduces the esp_fifo_push_buf() function for pushing multiple bytes
to the ESP hardware FIFO, and updates callers to use it accordingly.

Patches 9-11 incorporate additional protection to prevent FIFO assert()s and a
cmdfifo buffer overflow discovered via fuzzing.

Patch 12 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 13 introduce a new esp_update_drq() function that implements the above
DRQ logic which is called by both esp_fifo_{push,pop}_buf().

Patch 14 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 15 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 16 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>

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 (16):
  esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_command_phase()
  esp.c: replace cmdfifo use of esp_fifo_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: don't assert() if FIFO empty when executing esp_cdb_length()
  esp.c: don't overflow cmdfifo if cmdfifo_cdb_offset >= ESP_CMDFIFO_SZ
  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 | 193 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 123 insertions(+), 70 deletions(-)

-- 
2.39.2



^ permalink raw reply	[flat|nested] 34+ messages in thread
end of thread, other threads:[~2024-03-13 21:41 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-13  8:57 [PATCH v2 00/16] [for-9.0] esp: avoid explicit setting of DRQ within ESP state machine Mark Cave-Ayland
2024-03-13  8:57 ` [PATCH v2 01/16] esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_command_phase() Mark Cave-Ayland
2024-03-13 11:03   ` Philippe Mathieu-Daudé
2024-03-13 21:08     ` Mark Cave-Ayland
2024-03-13 21:40       ` Philippe Mathieu-Daudé
2024-03-13  8:57 ` [PATCH v2 02/16] esp.c: replace cmdfifo use of esp_fifo_pop_buf() in do_message_phase() Mark Cave-Ayland
2024-03-13 11:03   ` Philippe Mathieu-Daudé
2024-03-13 21:41     ` Philippe Mathieu-Daudé
2024-03-13  8:57 ` [PATCH v2 03/16] esp.c: replace cmdfifo use of esp_fifo_pop() " Mark Cave-Ayland
2024-03-13 10:51   ` Philippe Mathieu-Daudé
2024-03-13  8:57 ` [PATCH v2 04/16] esp.c: change esp_fifo_push() to take ESPState Mark Cave-Ayland
2024-03-13 10:53   ` Philippe Mathieu-Daudé
2024-03-13  8:57 ` [PATCH v2 05/16] esp.c: change esp_fifo_pop() " Mark Cave-Ayland
2024-03-13 10:52   ` Philippe Mathieu-Daudé
2024-03-13  8:58 ` [PATCH v2 06/16] esp.c: use esp_fifo_push() instead of fifo8_push() Mark Cave-Ayland
2024-03-13 10:53   ` Philippe Mathieu-Daudé
2024-03-13  8:58 ` [PATCH v2 07/16] esp.c: change esp_fifo_pop_buf() to take ESPState Mark Cave-Ayland
2024-03-13 10:54   ` Philippe Mathieu-Daudé
2024-03-13  8:58 ` [PATCH v2 08/16] esp.c: introduce esp_fifo_push_buf() function for pushing to the FIFO Mark Cave-Ayland
2024-03-13 10:56   ` Philippe Mathieu-Daudé
2024-03-13  8:58 ` [PATCH v2 09/16] esp.c: don't assert() if FIFO empty when executing non-DMA SELATNS Mark Cave-Ayland
2024-03-13 10:56   ` Philippe Mathieu-Daudé
2024-03-13  8:58 ` [PATCH v2 10/16] esp.c: don't assert() if FIFO empty when executing esp_cdb_length() Mark Cave-Ayland
2024-03-13  8:58 ` [PATCH v2 11/16] esp.c: don't overflow cmdfifo if cmdfifo_cdb_offset >= ESP_CMDFIFO_SZ Mark Cave-Ayland
2024-03-13  8:58 ` [PATCH v2 12/16] esp.c: move esp_set_phase() and esp_get_phase() towards the beginning of the file Mark Cave-Ayland
2024-03-13 10:58   ` Philippe Mathieu-Daudé
2024-03-13  8:58 ` [PATCH v2 13/16] esp.c: introduce esp_update_drq() and update esp_fifo_{push, pop}_buf() to use it Mark Cave-Ayland
2024-03-13 11:01   ` Philippe Mathieu-Daudé
2024-03-13  8:58 ` [PATCH v2 14/16] esp.c: update esp_fifo_{push, pop}() to call esp_update_drq() Mark Cave-Ayland
2024-03-13 11:01   ` Philippe Mathieu-Daudé
2024-03-13  8:58 ` [PATCH v2 15/16] esp.c: ensure esp_pdma_write() always calls esp_fifo_push() Mark Cave-Ayland
2024-03-13 11:05   ` Philippe Mathieu-Daudé
2024-03-13  8:58 ` [PATCH v2 16/16] esp.c: remove explicit setting of DRQ within ESP state machine Mark Cave-Ayland
2024-03-13 11:01   ` Philippe Mathieu-Daudé

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).