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

[Cédric Le Goater]

On 09/18/2018 08:53 PM, Peter Maydell wrote:
> 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.)

OK. I will rework that part.

Thanks,

C.