Re: [Qemu-devel] [PATCH 08/11] aspeed/smc: add support for DMAs

[Peter Maydell]

On 31 August 2018 at 11:38, Cédric Le Goater <address@hidden> wrote:
> The FMC controller on the Aspeed SoCs support DMA to access the flash
> modules. It can operate in a normal mode, to copy to or from the flash
> module mapping window, or in a checksum calculation mode, to evaluate
> the best clock settings for reads.
>
> Our primary need is to support the checksum calculation mode and the
> model only implements synchronous DMA accesses. Something to improve
> in the future.
>
> Signed-off-by: Cédric Le Goater <address@hidden>

>
> +/*
> + * Accumulate the result of the reads to provide a checksum that will
> + * be used to validate the read timing settings.
> + */
> +static void aspeed_smc_dma_checksum(AspeedSMCState *s)
> +{
> +    uint32_t data;
> +
> +    if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: invalid direction for DMA checksum\n",  __func__);
> +        return;
> +    }
> +
> +    while (s->regs[R_DMA_LEN]) {
> +        cpu_physical_memory_read(s->regs[R_DMA_FLASH_ADDR], &data, 4);

Ideally DMAing devices should not use cpu_physical_memory_*().
It's better to have the device take a MemoryRegion which it uses to
create an AddressSpace that it can use address_space_ld*. This (a)
allows you to correctly model that the DMAing device can't necessarily
see the same set of devices/memory that the CPU does and (b) detect
and handle read/write failures.

Commit 112a829f8f0ad is an example of converting a DMA controller from
old style cpu_physical_memory_*  to taking a MemoryRegion and using it.
(It doesn't do checking for read/write failure, but you should, by
passing a non-null MemTxResult* final argument.)

thanks
-- PMM