Re: [Qemu-devel] [PATCH v3 4/5] Enable fw_cfg DMA interface for ARM

[Laszlo Ersek]

On 09/18/15 22:24, Marc Marí wrote:
> On Fri, 18 Sep 2015 22:16:46 +0200
> Laszlo Ersek <address@hidden> wrote:
> 
>> On 09/18/15 10:58, Marc Marí wrote:
>>> Enable the fw_cfg DMA interface for the ARM virt machine.
>>>
>>> Based on Gerd Hoffman's initial implementation.
>>>
>>> Signed-off-by: Marc Marí <address@hidden>
>>> ---
>>>  hw/arm/virt.c | 9 +++++----
>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index 3568107..47f4ad3 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -113,7 +113,7 @@ static const MemMapEntry a15memmap[] = {
>>>      [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
>>>      [VIRT_UART] =               { 0x09000000, 0x00001000 },
>>>      [VIRT_RTC] =                { 0x09010000, 0x00001000 },
>>> -    [VIRT_FW_CFG] =             { 0x09020000, 0x0000000a },
>>> +    [VIRT_FW_CFG] =             { 0x09020000, 0x00000014 },
>>
>> Okay, Laszlo is the hateful reviewer. Sorry about that. I'm late, yes.
>>
>> But: this says 0x00000014, ie 20 bytes in decimal. I don't think
>> that's correct; it should be 0x18 -- 24 bytes in decimal. From patch
>> #2: "DMA Address address:       Base + 16 (8 bytes)".
> 
> It's not your problem if I don't know how to count. So don't
> apologize :).
> 
> And it's better to catch this stupid little mistakes now.

Got some good news: with those two fixups in place (register block size
corrected, and dma_enabled set via device property), I could test the
AAVMF / ArmVirtPkg / <insert your favorite synonym here> patches.

On my APM Mustang, downloading a decompressed kernel (14,475,776 bytes),
a decompressed initrd (18,177,264), and a cmdline (104 bytes :)), in
total 32,653,144 bytes, takes approx. 24 seconds with the 8-byte wide
MMIO data register. (Yeah, it's *really* slow.)

Using the DMA interface, the same takes about 52 milliseconds, and that
still includes one progress message per 1 MB downloaded :)

It's a factor of approx. 450. Not bad. Not bad. :)

Thanks
Laszlo


> Thanks
> Marc
> 
>> Thanks (and I'm sorry about being late!)
>> Laszlo
>>
>>>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>>>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of
>>> that size */ [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
>>> @@ -651,13 +651,13 @@ static void create_flash(const VirtBoardInfo
>>> *vbi) g_free(nodename);
>>>  }
>>>  
>>> -static void create_fw_cfg(const VirtBoardInfo *vbi)
>>> +static void create_fw_cfg(const VirtBoardInfo *vbi, AddressSpace
>>> *as) {
>>>      hwaddr base = vbi->memmap[VIRT_FW_CFG].base;
>>>      hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
>>>      char *nodename;
>>>  
>>> -    fw_cfg_init_mem_wide(base + 8, base, 8, 0, NULL);
>>> +    fw_cfg_init_mem_wide(base + 8, base, 8, base + 16, as);
>>>  
>>>      nodename = g_strdup_printf("/address@hidden" PRIx64, base);
>>>      qemu_fdt_add_subnode(vbi->fdt, nodename);
>>> @@ -919,6 +919,7 @@ static void machvirt_init(MachineState *machine)
>>>  
>>>      create_fdt(vbi);
>>>  
>>> +
>>>      for (n = 0; n < smp_cpus; n++) {
>>>          ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU,
>>> cpustr[0]); CPUClass *cc = CPU_CLASS(oc);
>>> @@ -984,7 +985,7 @@ static void machvirt_init(MachineState *machine)
>>>       */
>>>      create_virtio_devices(vbi, pic);
>>>  
>>> -    create_fw_cfg(vbi);
>>> +    create_fw_cfg(vbi, &address_space_memory);
>>>      rom_set_fw(fw_cfg_find());
>>>  
>>>      guest_info->smp_cpus = smp_cpus;
>>>
>>
>