Re: [Qemu-devel] [PATCH v2] arm: add fw_cfg to "virt" board

[Laszlo Ersek]

On 11/30/14 17:59, Laszlo Ersek wrote:
> fw_cfg already supports exposure over MMIO (used in ppc/mac_newworld.c,
> ppc/mac_oldworld.c, sparc/sun4m.c); we can easily add it to the "virt"
> board.
> 
> The mmio register block of fw_cfg is advertized in the device tree. As
> base address we pick 0x09020000, which conforms to the comment preceding
> "a15memmap": it falls in the miscellaneous device I/O range 128MB..256MB,
> and it is aligned at 64KB. The DTB properties follow the documentation in
> the Linux source file "Documentation/devicetree/bindings/arm/fw-cfg.txt".
> 
> fw_cfg automatically exports a number of files to the guest; for example,
> "bootorder" (see fw_cfg_machine_reset()).
> 
> Signed-off-by: Laszlo Ersek <address@hidden>
> ---
> 
> Notes:
>     v2:
>     - use a single mmio region of size 0x1000
>     - set "compatible" property to "qemu,fw-cfg-mmio"
> 
>  hw/arm/virt.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 314e55b..af794ea 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -68,6 +68,7 @@ enum {
>      VIRT_UART,
>      VIRT_MMIO,
>      VIRT_RTC,
> +    VIRT_FW_CFG,
>  };
>  
>  typedef struct MemMapEntry {
> @@ -107,6 +108,7 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_GIC_CPU] =    { 0x08010000, 0x00010000 },
>      [VIRT_UART] =       { 0x09000000, 0x00001000 },
>      [VIRT_RTC] =        { 0x09010000, 0x00001000 },
> +    [VIRT_FW_CFG] =     { 0x09020000, 0x00001000 },
>      [VIRT_MMIO] =       { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size 
> */
>      /* 0x10000000 .. 0x40000000 reserved for PCI */
> @@ -519,6 +521,23 @@ static void create_flash(const VirtBoardInfo *vbi)
>      g_free(nodename);
>  }
>  
> +static void create_fw_cfg(const VirtBoardInfo *vbi)
> +{
> +    hwaddr base = vbi->memmap[VIRT_FW_CFG].base;
> +    hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
> +    char *nodename;
> +
> +    fw_cfg_init(0, 0, base, base + 2);
> +
> +    nodename = g_strdup_printf("/address@hidden" PRIx64, base);
> +    qemu_fdt_add_subnode(vbi->fdt, nodename);
> +    qemu_fdt_setprop_string(vbi->fdt, nodename,
> +                            "compatible", "qemu,fw-cfg-mmio");
> +    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
> +                                 2, base, 2, size);
> +    g_free(nodename);
> +}
> +
>  static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
>  {
>      const VirtBoardInfo *board = (const VirtBoardInfo *)binfo;
> @@ -604,6 +623,8 @@ static void machvirt_init(MachineState *machine)
>       */
>      create_virtio_devices(vbi, pic);
>  
> +    create_fw_cfg(vbi);
> +
>      vbi->bootinfo.ram_size = machine->ram_size;
>      vbi->bootinfo.kernel_filename = machine->kernel_filename;
>      vbi->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> 

So... after playing with this thing for some time, it's become clear
that "MMIO traps" are painfully slow on the aarch64 platform we've been
working on (using KVM).

The original approach in my guest UEFI patch was a simple loop that
exerted byte-wise access to the fw_cfg device's data register (the only
kind of access that fw_cfg allows ATM). Downloading a kernel image plus
an initrd image byte for byte, which together can total between 30MB and
50MB, takes simply forever.

Drew suggested to try wider accesses. With the following PoC patch:

-------------------
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 584c40d..739e19c 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -527,7 +527,7 @@ static void create_fw_cfg(const VirtBoardInfo *vbi)
     hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
     char *nodename;

-    fw_cfg_init(0, 0, base, base + 2);
+    fw_cfg_init(0, 0, base, base + 8);

     nodename = g_strdup_printf("/address@hidden" PRIx64, base);
     qemu_fdt_add_subnode(vbi->fdt, nodename);
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index a7122ee..7147fea 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -322,8 +322,9 @@ static const MemoryRegionOps fw_cfg_data_mem_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
     .valid = {
         .min_access_size = 1,
-        .max_access_size = 1,
+        .max_access_size = 8,
     },
+    .impl.max_access_size = 1,
 };

 static const MemoryRegionOps fw_cfg_comb_mem_ops = {
-------------------

(and a corresponding firmware-side patch), we managed to speed it up by
a factor of 7.5.

Peter, can we introduce a second, 64-bit wide, data register, for
fw_cfg? (Or even two -- Drew suggested the LDP instruction for the guest.)

... BTW, re fw_cfg slowness, Rich mentioned this thread from 2010:

http://thread.gmane.org/gmane.comp.emulators.qemu/77582

Thanks
Laszlo