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í <mar...@redhat.com>
> ---
> 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 },
> [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("/fw-cfg@%" 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;
>
I got excited that the work got this far (thanks a lot for it, and I apologize
on falling back on the review), so I wanted to start writing the edk2 /
ArmVirtPkg client code for it.
I applied your v3 series on top of current master
(b12a84ce3c27e42c8f51c436aa196938d5cc2c71). First I wanted to see a new DTB:
$ qemu-system-aarch64 -machine virt,dumpdtb=xx.dtb
Unfortunately it crashes with a failed assertion:
qemu-system-aarch64: hw/core/sysbus.c:130: sysbus_mmio_map_common: Assertion `n
>= 0 && n < dev->num_mmio' failed.
The problem is that you have a third (conditional) sysbus_mmio_map() in
fw_cfg_init_mem_wide(), from patch #3, which would depend on the similarly
conditional sysbus_init_mmio() call in fw_cfg_mem_realize().
However, that prerequisite sysbus_init_mmio() is never executed in
fw_cfg_mem_realize(), because it would depend on the (FW_CFG(s)->dma_enabled)
field, which at that point *cannot* have been set at all.
So you have to set it through a property, because that's the only way you can
pass it to the realize method.
Please squash the following patch into patch #3:
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index d11d8c5..946abb5 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -799,9 +799,11 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
> SysBusDevice *sbd;
> FWCfgState *s;
> uint32_t version = FW_CFG_VERSION;
> + bool dma_enabled = dma_addr && dma_as;
>
> dev = qdev_create(NULL, TYPE_FW_CFG_MEM);
> qdev_prop_set_uint32(dev, "data_width", data_width);
> + qdev_prop_set_bit(dev, "dma_enabled", dma_enabled);
>
> fw_cfg_init1(dev);
>
> @@ -811,9 +813,8 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
>
> s = FW_CFG(dev);
>
> - if (dma_addr && dma_as) {
> + if (dma_enabled) {
> s->dma_as = dma_as;
> - s->dma_enabled = true;
> s->dma_addr = 0;
> sysbus_mmio_map(sbd, 2, dma_addr);
> version |= FW_CFG_VERSION_DMA;
> @@ -891,6 +892,8 @@ static const TypeInfo fw_cfg_io_info = {
>
> static Property fw_cfg_mem_properties[] = {
> DEFINE_PROP_UINT32("data_width", FWCfgMemState, data_width, -1),
> + DEFINE_PROP_BOOL("dma_enabled", FWCfgMemState, parent_obj.dma_enabled,
> + false),
> DEFINE_PROP_END_OF_LIST(),
> };
>
Thanks
Laszlo
