Re: [PATCH 00/88] esp: rework ESP emulation to use a SCSI phase-based state machine

[Thomas Huth <thuth@redhat.com>]

On 12/01/2024 13.52, Mark Cave-Ayland wrote:
> The ESP SCSI chip fundamentally consists of a FIFO for transferring data to/from
> the SCSI bus along with a command sequencer which automates various processes such
> as selection, message/command transfer and data transfer. What makes this chip
> particularly interesting is that the FIFO is also used as a buffer for DMA transfers
> which makes it possible to mix and match DMA and non-DMA transfers when sending and
> receiving data to/from the SCSI bus.
> 
> Whilst the current ESP emulation works well for most cases, there are several
> problems that I've encountered whilst trying to debug various guest issues:
> 
> 
> 1) Multiple code paths for non-DMA, DMA and pDMA
> 
> Consider a SCSI request being submitted by the guest: if it is a DMA SCSI
> request then it takes one path through get_cmd(), and a different path through
> get_cmd() for pDMA. This then leads to the DMA code path being called for DMA
> and the pDMA codepath being called for each FIFO write. Finally once you add
> in the non-DMA path you end up with a total of 5 potential codepaths for any
> given SCSI request.
> 
> 2) Manual handling of STAT_TC flag
> 
> According to the datasheet the STAT_TC flag in ESP_RSTAT is set when the DMA
> transfer counter reaches zero. The current code handles this manually in
> multiple places, including where it shouldn't be necessary.
> 
> 3) Deferred interrupts are used only for reads and not writes
> 
> The ESP emulation currently makes use of a deferred interrupt when submitting
> read commands i.e. the interrupt flags are not updated immediately but when
> the QEMU SCSI layer indicates it is ready. This works well for reads, but isn't
> implemented for writes which can cause problems when the host is heavily loaded
> or a large SCSI write is requested.
> 
> 4) Padding of pDMA TI transfers to a minimum of 16 bytes
> 
> In order to allow Classic MacOS to boot successfully there is a workaround that
> pads all pDMA TI transfers to a minimum of 16 bytes (see commit 7aa6baee7c
> "esp: add support for unaligned accesses"). This feature is not documented and it
> turns out that this is a symptom of incorrect handling of transfer
> overflow/underflow conditions.
> 
> 5) Duplication of some non-DMA logic in esp_reg_read() and esp_reg_write()
> 
> When reading and writing to the FIFO there is some duplication of logic from
> esp_do_nodma() in esp_reg_read() and esp_reg_write(). This should be eliminated
> in favour of a single state machine for handling non-DMA FIFO accesses in a
> single place.
> 
> 
> The series here reworks the ESP emulation to use a SCSI phase-based state machine
> with just two paths: one for DMA/pDMA requests, and another for non-DMA requests.
> The pDMA callbacks are completely removed so that there is now only a single path
> for DMA requests. As part of this work the manual STAT_TC handling is removed, and a
> couple of bugs in the SCSI phase selection are fixed to allow proper transfer
> underflow/overflow detection and recovery.
> 
> I've tested the series with all of my available ESP images and compatibility is
> greatly improved with these changes: I believe that this series should also fix a
> few GitLab issues including https://gitlab.com/qemu-project/qemu/-/issues/1831,
> https://gitlab.com/qemu-project/qemu/-/issues/611 and
> https://gitlab.com/qemu-project/qemu/-/issues/1127. In addition it allows Aurelien's
> really old SPARC Linux images to boot once again, and also the NeXTCube machine can
> now boot, load its kernel from disk and start to execute it.

FWIW, I did some sanity checking by installing an old Fedora 12 onto a SCSI 
disk attached to the am53c974 and dc390 controllers, and everything still 
works fine with your patches. Thus feel free to add:

Tested-by: Thomas Huth <thuth@redhat.com>