Re: [PATCH 00/25] esp: consolidate PDMA transfer buffers

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 30/12/2020 15:37, Mark Cave-Ayland wrote:

> This patch series comes from an experimental branch that I've been working on
> to try and boot a MacOS toolbox ROM under the QEMU q800 machine. The effort is
> far from complete, but it seems worth submitting these patches separately since
> they are limited to the ESP device and form a substantial part of the work to
> date.
> 
> As part of Laurent's recent q800 work so-called PDMA (pseudo-DMA) support was
> added to the ESP device. This is whereby the DREQ (DMA request) line is used
> to signal to the host CPU that it can transfer data to/from the device over
> the SCSI bus.
> 
> The existing PDMA tracks 4 separate transfer data sources as indicated by the
> ESP pdma_origin variable: PDMA, TI, CMD and ASYNC with an independent variable
> pdma_len to store the transfer length. This works well with Linux which uses a
> single PDMA request to transfer a number of sectors in a single request.
> 
> Unfortunately the MacOS toolbox ROM has other ideas here: it sends data to the
> ESP as a mixture of FIFO and PDMA transfers and then uses a mixture of the FIFO
> and DMA counters to confirm that the correct number of bytes have been
> transferred. For this to work correctly the PDMA buffers and separate pdma_len
> transfer counter must be consolidated into the FIFO to allow mixing of both
> types of transfer within a single request.
> 
> The patchset is split into several sections:
> 
> - Patches 1-4 are minor patches which make esp.c checkpatch friendly whilst also
>    fixing up some trace events ready for later patches in the series
>    
> - Patches 5-11 unify the DMA transfer count. In particular there are 2 synthetic
>    variables dma_counter and dma_left within ESPState which do not need to exist.
>    DMA transfer lengths are programmed into the TC (transfer count) register which is
>    decremented for each byte transferred, generating an interrupt when it reaches zero.
>    These patches add helper functions to read the TC and STC registers directly and
>    remove these synthetic variables so that the DMA transfer length is now tracked in
>    a single place.
> 
> - Now that the TC register represents the authoritative DMA transfer length, patches
>    12-20 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.
> 
> - Patches 21 and 22 fix the detection of missing SCSI targets by the MacOS toolbox ROM
>    on startup at which point it will attempt to start reading information from a CDROM
>    attached to the q800 machine.
> 
> - Patch 23 is the main rework of the PDMA buffer transfers: instead of tracking the
>    SCSI transfers using a separate ASYNC pdma_origin, the contents of the ESPState
>    async_buf are copied to the FIFO buffer in 16-byte chunks with the transfer status
>    and IRQs being set accordingly.
> 
> - Patch 24 removes the last separate PDMA variable pdma_origin, including the separate
>    PDMA migration subsection which is no longer required (see note below about migration
>    compatibility).
>    
> - Finally patch 25 enables 4 byte PDMA reads/writes over the SCSI bus which are used
>    by MacOS when reading the next stage bootloader from CDROM (this is an increase from
>    2 bytes currently implemented and used by Linux).
> 
> 
> Testing
> =======
> 
> I've tested this on my SPARC32 OpenBIOS images which include Linux, OpenBSD, NetBSD,
> and Solaris and all of these continue to boot as before.
> 
> Similarly the q800 m68k Linux test image still boots as before with these patches
> applied. It is possible with lots of hacks to load Laurent's EMILE bootloader using
> a MacOS toolbox ROM - the hope is to try and start upstreaming more of these changes
> as time allows.
> 
> 
> Migration
> =========
> 
> The patchset ensures that ESP devices without PDMA (i.e. everything except the q800
> machine) will migrate successfully. This is fairly simple: the only change required
> here is to copy the old synthetic dma_left value over into the TC.
> 
> Unfortunately migrating the PDMA subsection is a lot harder due to the change in the
> way that the DMA TC and changes to the point at which transfer counters are updated.
> For this reason the patchset will not migrate from older q800 snapshots: I don't
> believe this to be a problem since some devices are still missing VMStateDescription
> plus there are likely to be more breaking changes as the q800 machine matures.
> 
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> 
> Mark Cave-Ayland (25):
>    esp: checkpatch fixes
>    esp: add trace event when receiving a TI command
>    esp: fix esp_reg_read() trace event
>    esp: add PDMA trace events
>    esp: determine transfer direction directly from SCSI phase
>    esp: introduce esp_get_tc() and esp_set_tc()
>    esp: introduce esp_get_stc()
>    esp: apply transfer length adjustment when STC is zero at TC load time
>    esp: remove dma_counter from ESPState
>    esp: remove dma_left from ESPState
>    esp: remove minlen restriction in handle_ti
>    esp: introduce esp_pdma_read() and esp_pdma_write() functions
>    esp: use pdma_origin directly in esp_pdma_read()/esp_pdma_write()
>    esp: move pdma_len and TC logic into esp_pdma_read()/esp_pdma_write()
>    esp: accumulate SCSI commands for PDMA transfers in cmdbuf instead of
>      pdma_buf
>    esp: remove redundant pdma_start from ESPState
>    esp: move PDMA length adjustments into
>      esp_pdma_read()/esp_pdma_write()
>    esp: use ti_wptr/ti_rptr to manage the current FIFO position for PDMA
>    esp: use in-built TC to determine PDMA transfer length
>    esp: remove CMD pdma_origin
>    esp: rename get_cmd_cb() to esp_select()
>    esp: fix PDMA target selection
>    esp: use FIFO for PDMA transfers between initiator and device
>    esp: remove pdma_origin from ESPState
>    esp: add 4 byte PDMA read and write transfers
> 
>   hw/scsi/esp.c         | 461 +++++++++++++++++++++++++-----------------
>   hw/scsi/trace-events  |   5 +
>   include/hw/scsi/esp.h |  20 +-
>   3 files changed, 279 insertions(+), 207 deletions(-)

Please ignore this patchset for now - whilst the changes allow mixed PDMA and FIFO 
requests, they assume that the FIFO request sizes have the same alignment as the PDMA 
request sizes. I've just found a case where this isn't true in the MacOS toolbox ROM, 
so I'll investigate further and then resubmit.


ATB,

Mark.