Re: [Qemu-devel] [RFC 4/7] enable fw_cfg dma for arm virt

[Laszlo Ersek <lersek@redhat.com>]

On 07/21/15 19:04, Peter Maydell wrote:
> On 21 July 2015 at 17:03, Marc Marí <markmb@redhat.com> wrote:
>> From: Gerd Hoffmann <kraxel@redhat.com>
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>  docs/specs/fw_cfg.txt |  6 ++++++
>>  hw/arm/virt.c         | 11 ++++++++---
>>  2 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
>> index 64d9192..eac83a1 100644
>> --- a/docs/specs/fw_cfg.txt
>> +++ b/docs/specs/fw_cfg.txt
>> @@ -167,6 +167,12 @@ If a partial transfer happened before an error occured the address and
>>  length registers indicate how much data has been transfered
>>  successfully.
>>
>> +== Register Locations ==
>> +
>> +=== arm ===
>> +
>> +classic fw_cfg base + 16
>> +
>>  = Host-side API =
>>
>>  The following functions are available to the QEMU programmer for adding
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 374660c..d05f6a4 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -606,13 +606,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)
>>  {
>>      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, 16, as);
>>
>>      nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
>>      qemu_fdt_add_subnode(vbi->fdt, nodename);
> 
> Don't we need to also update the device tree

Yes, we do (a15memmap[VIRT_FW_CFG]).

> (and the spec for
> that!)

Yes, we do ("Documentation/devicetree/bindings/arm/fw-cfg.txt").

> to indicate that we're providing a with-DMA fw-cfg
> device ?

Thanks
Laszlo

> 
>> @@ -800,6 +800,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";
>> @@ -837,6 +838,10 @@ static void machvirt_init(MachineState *machine)
>>          }
>>          cpuobj = object_new(object_class_get_name(oc));
>>
>> +        if (!as) {
>> +            as = CPU(cpuobj)->as;
>> +        }
> 
> Is this really the right AddressSpace to use? Fishing
> around in the CPU object seems a bit dubious...
> 
> thanks
> -- PMM
> 
>