| Message ID | 20240324191707.623175-1-mark.cave-ayland@ilande.co.uk |
|---|---|
| Headers | show |
| Series | esp: avoid explicit setting of DRQ within ESP state machine | expand |
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 >
Hi Mark, On 24/3/24 20:16, Mark Cave-Ayland wrote: > 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 I have to prepare another PR for rc3, do you want me to take this series? Regards, Phil.
On 04/04/2024 11:28, Philippe Mathieu-Daudé wrote: > Hi Mark, > > On 24/3/24 20:16, Mark Cave-Ayland wrote: > >> 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 > > I have to prepare another PR for rc3, do you want me to take > this series? Hi Phil, Thanks for the offer, but amazingly I have a little bit of time this afternoon(!), so I'll try and send a PR later today. ATB, Mark.
[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-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> 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(-)