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

[Peter Maydell]

On 31 August 2015 at 10:10, Marc Marí <address@hidden> 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 | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index b88c104..54d5f54 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -111,7 +111,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 },
>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size 
> */
>      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> @@ -614,13 +614,13 @@ static void create_flash(const VirtBoardInfo *vbi)
>      g_free(nodename);
>  }
>
> -static void create_fw_cfg(const VirtBoardInfo *vbi)
> +static void create_fw_cfg(AddressSpace *as, const VirtBoardInfo *vbi)

Please keep vbi as the first argument; this matches the other functions
in this file.

Calling the argument dma_as would probably be a little more informative.

>  {
>      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);
> @@ -808,6 +808,7 @@ static void machvirt_init(MachineState *machine)
>      VirtGuestInfoState *guest_info_state = g_malloc0(sizeof 
> *guest_info_state);
>      VirtGuestInfo *guest_info = &guest_info_state->info;
>      char **cpustr;
> +    AddressSpace *as = NULL;
>
>      if (!cpu_model) {
>          cpu_model = "cortex-a15";
> @@ -845,6 +846,10 @@ static void machvirt_init(MachineState *machine)
>          }
>          cpuobj = object_new(object_class_get_name(oc));
>
> +        if (!as) {
> +            as = CPU(cpuobj)->as;
> +        }

This seems like a weird thing to set the fw_cfg DMA address
space to. (Either the fw_cfg device shouldn't care which CPU
it is being accessing by and shouldn't use any particular CPU's
address space, or it needs to really care about the CPU
that's doing any particular write to it and use that exact
CPU's address space, selected at runtime. The former is the
most likely and matches what actual DMA hardware devices
will do.)

Why not just use address_space_memory ?

> +
>          /* Handle any CPU options specified by the user */
>          cc->parse_features(CPU(cpuobj), cpuopts, &err);
>          g_free(cpuopts);
> @@ -897,7 +902,7 @@ static void machvirt_init(MachineState *machine)
>       */
>      create_virtio_devices(vbi, pic);
>
> -    create_fw_cfg(vbi);
> +    create_fw_cfg(as, vbi);
>      rom_set_fw(fw_cfg_find());
>
>      guest_info->smp_cpus = smp_cpus;
> --
> 2.4.3

thanks
-- PMM