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

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

[Laszlo Ersek]

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 <lersek@redhat.com>
---

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("/fw-cfg@%" 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;
-- 
1.8.3.1

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

[Peter Maydell]

On 30 November 2014 at 16:59, Laszlo Ersek <lersek@redhat.com> 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 <lersek@redhat.com>
> ---
>
> 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 },

Where does this 0x1000 size come from? It's not the size
of the memory region the fw_cfg device exports, and it
means we'll end up telling the f/w in the dtb that the
accessible region is bigger than it really is.

Otherwise this patch looks good to me.

-- PMM

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

[Laszlo Ersek]

On 12/05/14 19:42, Peter Maydell wrote:
> On 30 November 2014 at 16:59, Laszlo Ersek <lersek@redhat.com> 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 <lersek@redhat.com>
>> ---
>>
>> 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 },
> 
> Where does this 0x1000 size come from? It's not the size
> of the memory region the fw_cfg device exports, and it
> means we'll end up telling the f/w in the dtb that the
> accessible region is bigger than it really is.

This has been specifically requested in

http://thread.gmane.org/gmane.linux.drivers.devicetree/101173/focus=101176

See also the list of changes in

http://thread.gmane.org/gmane.linux.drivers.devicetree/101276/focus=101277

> Otherwise this patch looks good to me.
> 
> -- PMM
> 

Thanks
Laszlo

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 <lersek@redhat.com>
> ---
> 
> 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("/fw-cfg@%" 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("/fw-cfg@%" 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

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

[Peter Maydell]

On 8 December 2014 at 14:01, Laszlo Ersek <lersek@redhat.com> wrote:
> 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.)

I don't have an in-principle objection. I do require that
whatever mechanism we provide is usable by 32 bit guests
as well as 64 bit guests. You'll also want the access instruction
to be one where the hardware provides instruction syndrome information
to KVM about the faulting load/store, which means you can only use
AArch64 loads and stores of a single general-purpose register, and
AArch32 instructions LDR, LDA, LDRT, LDRSH, LDRSHT, LDRH, LDAH, LDRHT,
LDRSB, LDRSBT, LDRB, LDAB, LDRBT, STR, STL, STRT, STRH, STLH, STRHT,
STRB, STLB, or STRBT.

Note that this rules out LDP.

We also need to make sure it works with TCG QEMU. (64-bit access
to devices is something we've needed previously in ARM QEMU, so
though in theory it should work it would need testing.)

The other thing you could do is put image and initrd in to guest
memory as we do currently and have the fw_cfg just pass their
addresses. That would require that UEFI not have already stomped
on that part of RAM by the time you get round to looking at it,
though, and I can imagine that might not be feasible.

-- PMM

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

[Peter Maydell]

On 8 December 2014 at 14:41, Peter Maydell <peter.maydell@linaro.org> wrote:
> We also need to make sure it works with TCG QEMU. (64-bit access
> to devices is something we've needed previously in ARM QEMU, so
> though in theory it should work it would need testing.)

"is not something we've needed previously", obviously.

-- PMM

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

[Laszlo Ersek]

On 12/08/14 15:41, Peter Maydell wrote:
> On 8 December 2014 at 14:01, Laszlo Ersek <lersek@redhat.com> wrote:
>> 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.)
> 
> I don't have an in-principle objection. I do require that
> whatever mechanism we provide is usable by 32 bit guests
> as well as 64 bit guests.

The idea is that in the following:

  fw_cfg_data_mem_read()
    fw_cfg_read()

fw_cfg_data_mem_read() doesn't care about either "addr" or "size", which
happens to be the best for the current problem.

According to the documentation on "MemoryRegionOps" in
"include/exec/memory.h" (and also to my testing in practice), if you set
.valid.max_access_size to something large (definitely up to 8, but maybe
even up to 16?), but keep .impl.max_access_size at something smaller,
then "memory.c" will split up the access for you automatically.
(Ultimately turning the bigger access to several sequential calls to
fw_cfg_read(), which happens to be correct.)

So, if a 32-bit guest never "submits" an access wider than 32-bit, then
that's the largest that "memory.c" will slice and dice.

> You'll also want the access instruction
> to be one where the hardware provides instruction syndrome information
> to KVM about the faulting load/store,

Yes, I definitely remembered this; I just didn't know how to name it
precisely.

> which means you can only use
> AArch64 loads and stores of a single general-purpose register, and
> AArch32 instructions LDR, LDA, LDRT, LDRSH, LDRSHT, LDRH, LDAH, LDRHT,
> LDRSB, LDRSBT, LDRB, LDAB, LDRBT, STR, STL, STRT, STRH, STLH, STRHT,
> STRB, STLB, or STRBT.
> 
> Note that this rules out LDP.

Yes, I was concerned about this (Drew can confirm :)) Thank you very
much for making this clear; this will definitely help with the guest
code. Also we don't have to figure out if .valid.max_access_size=16
would work at all.

> We also need to make sure it works with TCG QEMU. (64-bit access
> to devices is something we've needed previously in ARM QEMU, so
> though in theory it should work it would need testing.)

(Factoring in the typo correction from your next mail:)

Good point. I just tested TCG. While the speedup is similar, the data
that is transferred looks corrupt.

> The other thing you could do is put image and initrd in to guest
> memory as we do currently and have the fw_cfg just pass their
> addresses. That would require that UEFI not have already stomped
> on that part of RAM by the time you get round to looking at it,
> though, and I can imagine that might not be feasible.

Yes, I thought of this. It's not very different from how the initial DTB
is handled right now, theoretically.

The difference is that the DTB is small. It is placed at 0x4000_0000
(1GB, base of DRAM). The earliest phase of the firmare (SEC and first
half of PEI) uses memory at the fixed address 0x4007_c000 (1GB + 496
KB). I would really not want to change that in the firmware.

http://thread.gmane.org/gmane.comp.bios.tianocore.devel/8969/focus=9029

We could place the kernel image and the initrd elsewhere, but (a)
placing stuff in the DRAM in advance puts minimum DRAM size requirements
on the guest (*), which is hard to verify / enforce in the firmware with
blobs of wildly varying size, and more importantly, (b) auditing the
safety of the initial DTB was already the hardest part of the firmware
patchset review by far. (No doubt it was one of the hardest things to
implement for Ard too.)

(*) See later parts in the same message, wrt. "second world". Later
phases need a good chunk of memory counting downwards from the top. See
also the SIZE_128MB assert near
<https://github.com/tianocore/edk2/blob/master/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLib/Virt.c#L174>.

The nice thing about fw_cfg is that it lives in the "128MB..256MB [...]
miscellaneous device I/O" space. It's basically impossible to collide
with that in UEFI, yet it can give you various blobs (and qemu can even
compute those dynamically, see ACPI rebuild after PCI enumeration on
x86). I'd *really* like to stick with that.

I'll try to figure out what's wrong with TCG and come up with a patch
for the second data register.

Thanks!
Laszlo

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

[Laszlo Ersek]

On 12/08/14 20:39, Laszlo Ersek wrote:
> On 12/08/14 15:41, Peter Maydell wrote:
>> On 8 December 2014 at 14:01, Laszlo Ersek <lersek@redhat.com> wrote:
>>> 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.)
>>
>> I don't have an in-principle objection. I do require that
>> whatever mechanism we provide is usable by 32 bit guests
>> as well as 64 bit guests.
> 
> The idea is that in the following:
> 
>   fw_cfg_data_mem_read()
>     fw_cfg_read()
> 
> fw_cfg_data_mem_read() doesn't care about either "addr" or "size", which
> happens to be the best for the current problem.
> 
> According to the documentation on "MemoryRegionOps" in
> "include/exec/memory.h" (and also to my testing in practice), if you set
> .valid.max_access_size to something large (definitely up to 8, but maybe
> even up to 16?), but keep .impl.max_access_size at something smaller,
> then "memory.c" will split up the access for you automatically.
> (Ultimately turning the bigger access to several sequential calls to
> fw_cfg_read(), which happens to be correct.)
> 
> So, if a 32-bit guest never "submits" an access wider than 32-bit, then
> that's the largest that "memory.c" will slice and dice.
> 
>> You'll also want the access instruction
>> to be one where the hardware provides instruction syndrome information
>> to KVM about the faulting load/store,
> 
> Yes, I definitely remembered this; I just didn't know how to name it
> precisely.
> 
>> which means you can only use
>> AArch64 loads and stores of a single general-purpose register, and
>> AArch32 instructions LDR, LDA, LDRT, LDRSH, LDRSHT, LDRH, LDAH, LDRHT,
>> LDRSB, LDRSBT, LDRB, LDAB, LDRBT, STR, STL, STRT, STRH, STLH, STRHT,
>> STRB, STLB, or STRBT.
>>
>> Note that this rules out LDP.
> 
> Yes, I was concerned about this (Drew can confirm :)) Thank you very
> much for making this clear; this will definitely help with the guest
> code. Also we don't have to figure out if .valid.max_access_size=16
> would work at all.
> 
>> We also need to make sure it works with TCG QEMU. (64-bit access
>> to devices is something we've needed previously in ARM QEMU, so
>> though in theory it should work it would need testing.)
> 
> (Factoring in the typo correction from your next mail:)
> 
> Good point. I just tested TCG. While the speedup is similar, the data
> that is transferred looks corrupt.

Okay, TCG is not the culprit, it seems to work okay. I messed up the
memory region registration in the previous patch. On x86_64 the 8-byte
access is broken up into two 4-byte accesses, and the second one of
those is not accepted as a valid access, and it qualifies as an
unassigned read.

So the following in addition makes it work on TCG (x86_64) too:

-----------------
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 7147fea..c2bc44c 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -31,7 +31,7 @@
 #include "qemu/config-file.h"

 #define FW_CFG_SIZE 2
-#define FW_CFG_DATA_SIZE 1
+#define FW_CFG_DATA_SIZE 8
 #define TYPE_FW_CFG "fw_cfg"
 #define FW_CFG_NAME "fw_cfg"
 #define FW_CFG_PATH "/machine/" FW_CFG_NAME
-----------------

It affects the memory_region_init_io() call in fw_cfg_initfn().

I hope to submit a small v3 series soon.

Thanks,
Laszlo

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

[Peter Maydell]

On 8 December 2014 at 21:19, Laszlo Ersek <lersek@redhat.com> wrote:
> So the following in addition makes it work on TCG (x86_64) too:
>
> -----------------
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 7147fea..c2bc44c 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -31,7 +31,7 @@
>  #include "qemu/config-file.h"
>
>  #define FW_CFG_SIZE 2
> -#define FW_CFG_DATA_SIZE 1
> +#define FW_CFG_DATA_SIZE 8
>  #define TYPE_FW_CFG "fw_cfg"
>  #define FW_CFG_NAME "fw_cfg"
>  #define FW_CFG_PATH "/machine/" FW_CFG_NAME
> -----------------
>
> It affects the memory_region_init_io() call in fw_cfg_initfn().
>
> I hope to submit a small v3 series soon.

If you do that don't you now try to define an ioport on
x86 that's 8 bytes wide? You probably also need to check
whether the ppc and sparc boards that use mmio fw_cfg
can handle the wider data register.

-- PMM

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

[Laszlo Ersek]

On 12/08/14 22:34, Peter Maydell wrote:
> On 8 December 2014 at 21:19, Laszlo Ersek <lersek@redhat.com> wrote:
>> So the following in addition makes it work on TCG (x86_64) too:
>>
>> -----------------
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 7147fea..c2bc44c 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -31,7 +31,7 @@
>>  #include "qemu/config-file.h"
>>
>>  #define FW_CFG_SIZE 2
>> -#define FW_CFG_DATA_SIZE 1
>> +#define FW_CFG_DATA_SIZE 8
>>  #define TYPE_FW_CFG "fw_cfg"
>>  #define FW_CFG_NAME "fw_cfg"
>>  #define FW_CFG_PATH "/machine/" FW_CFG_NAME
>> -----------------
>>
>> It affects the memory_region_init_io() call in fw_cfg_initfn().
>>
>> I hope to submit a small v3 series soon.
> 
> If you do that don't you now try to define an ioport on
> x86 that's 8 bytes wide? You probably also need to check
> whether the ppc and sparc boards that use mmio fw_cfg
> can handle the wider data register.

The above was just a quick PoC. The series I'm about to post takes care
not to change anything (not even gdb experience) for clients that don't
request the wider register.

Thanks
Laszlo

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

[Christopher Covington]

Hi Laszlo,

On 12/08/2014 09:01 AM, Laszlo Ersek wrote:
> 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 <lersek@redhat.com>
>> ---
>>
>> 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("/fw-cfg@%" 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.

Just a thought--would it be possible to add a
DMA-the-whole-thing-to-this-address register to the simulated device?

Chris

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Peter Maydell]

On 8 December 2014 at 23:18, Christopher Covington <cov@codeaurora.org> wrote:
> Just a thought--would it be possible to add a
> DMA-the-whole-thing-to-this-address register to the simulated device?

That was an idea raised in the 2010 thread that Laszlo mentioned:
http://thread.gmane.org/gmane.comp.emulators.qemu/77582

It's a big old thread so I only skimmed it, but there seemed to
be some fairly heavy pushback on the idea -- has anything much
changed in the intervening time to make it a better plan this
time around?

-- PMM

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

[Laszlo Ersek]

On 12/09/14 00:18, Christopher Covington wrote:
> Hi Laszlo,
> 
> On 12/08/2014 09:01 AM, Laszlo Ersek wrote:
>> 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 <lersek@redhat.com>
>>> ---
>>>
>>> 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("/fw-cfg@%" 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.
> 
> Just a thought--would it be possible to add a
> DMA-the-whole-thing-to-this-address register to the simulated device?

That's exactly what Rich proposed in the 2010 thread that I referenced a
few hours ago.

http://thread.gmane.org/gmane.comp.emulators.qemu/77582/focus=77589

(search for '"DMA"-like')

The original patchset is archived at

http://thread.gmane.org/gmane.comp.emulators.qemu/76610/focus=76673

which was shot down; the rebased one got an (unrelated) nack three
months later.

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

Thanks
Laszlo

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

[Gerd Hoffmann]

  Hi,

> 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).

So, as we don't have compatibility requirements, and we also can't play
tricks like using x86 string instructions:  How about a completely
different, dma-style interface for fw_cfg access?

One register for the (physical) target address.
One register for the transfer size.
One register for the fw_cfg entry.
Possibly one register for the fw_cfg offset (not really needed, but
avoids the need for side effects such as writing fw_cfg entry register
clearing the offset).
One register to kick the transfer.

cheers,
  Gerd

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

[Laszlo Ersek]

On 12/09/14 10:31, Gerd Hoffmann wrote:
>   Hi,
> 
>> 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).
> 
> So, as we don't have compatibility requirements, and we also can't play
> tricks like using x86 string instructions:  How about a completely
> different, dma-style interface for fw_cfg access?
> 
> One register for the (physical) target address.
> One register for the transfer size.
> One register for the fw_cfg entry.
> Possibly one register for the fw_cfg offset (not really needed, but
> avoids the need for side effects such as writing fw_cfg entry register
> clearing the offset).
> One register to kick the transfer.

Again, this was the idea that Rich had in 2010 (see the links in the
discussion thus far). It was rejected back then (which is why I didn't
even try to resurrect it now), and Peter has now asked if anything has
changed that would make that approach more acceptable now.

"Avi and Gleb not hanging around so they can't block the approach now"
might or might not be such a change; I don't know. But, I tend to trust
their opinion, even if dates back to 2010 in this case.

Anyway I'm certainly not opposed to performance, so if someone can
thoroughly refute everything they said in that thread, go ahead.

Thanks!
Laszlo

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

[Peter Maydell]

On 9 December 2014 at 15:41, Laszlo Ersek <lersek@redhat.com> wrote:
> Again, this was the idea that Rich had in 2010 (see the links in the
> discussion thus far). It was rejected back then (which is why I didn't
> even try to resurrect it now), and Peter has now asked if anything has
> changed that would make that approach more acceptable now.
>
> "Avi and Gleb not hanging around so they can't block the approach now"
> might or might not be such a change; I don't know. But, I tend to trust
> their opinion, even if dates back to 2010 in this case.

Yeah, this is about my point of view. A direct-write-to-memory
fw_cfg doesn't seem too unreasonable to me, but it would be nice
to see a solidly argued case from its proponents to counterbalance
the previous rejection.

> Anyway I'm certainly not opposed to performance, so if someone can
> thoroughly refute everything they said in that thread, go ahead.

As far as I can tell the main line of argument was "if you're
using fw_cfg to transfer huge amounts of data you're doing
something wrong".

Is ARM much higher overhead than x86 for these accesses?

(Added Alex on cc as one of the few people from the previous
round of argument who's still active...)

-- PMM

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

[Richard W.M. Jones]

On Tue, Dec 09, 2014 at 03:47:44PM +0000, Peter Maydell wrote:
> On 9 December 2014 at 15:41, Laszlo Ersek <lersek@redhat.com> wrote:
> > Again, this was the idea that Rich had in 2010 (see the links in the
> > discussion thus far). It was rejected back then (which is why I didn't
> > even try to resurrect it now), and Peter has now asked if anything has
> > changed that would make that approach more acceptable now.
> >
> > "Avi and Gleb not hanging around so they can't block the approach now"
> > might or might not be such a change; I don't know. But, I tend to trust
> > their opinion, even if dates back to 2010 in this case.
> 
> Yeah, this is about my point of view. A direct-write-to-memory
> fw_cfg doesn't seem too unreasonable to me, but it would be nice
> to see a solidly argued case from its proponents to counterbalance
> the previous rejection.

The argument for is that it's essentially instantaneous.

This is a big deal for libguestfs where even our new initrd (1.5 MB)
takes a noticable fraction of a second to load, and we want to get our
total start-up time down to 3 seconds to match x86.

> > Anyway I'm certainly not opposed to performance, so if someone can
> > thoroughly refute everything they said in that thread, go ahead.
> 
> As far as I can tell the main line of argument was "if you're
> using fw_cfg to transfer huge amounts of data you're doing
> something wrong".
> 
> Is ARM much higher overhead than x86 for these accesses?

On a slightly related topic, virtio-mmio traps are slow:

https://bugzilla.redhat.com/show_bug.cgi?id=1123555

This also kills libguestfs performance -- throughput this time, rather
than start-up time.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

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

[Paolo Bonzini]

On 09/12/2014 16:54, Richard W.M. Jones wrote:
> On a slightly related topic, virtio-mmio traps are slow:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1123555

I suspect it's not virtio-mmio, but just KVM on ARM.

It would be nice to have a timing test for vmexits in kvm-unit-tests,
similar to what is already there for x86.

Paolo

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

[Andrew Jones]

On Wed, Dec 10, 2014 at 12:13:34PM +0100, Paolo Bonzini wrote:
> On 09/12/2014 16:54, Richard W.M. Jones wrote:
> > On a slightly related topic, virtio-mmio traps are slow:
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1123555
> 
> I suspect it's not virtio-mmio, but just KVM on ARM.
> 
> It would be nice to have a timing test for vmexits in kvm-unit-tests,
> similar to what is already there for x86.
> 

Christoffer had some for his initial kernel selftests, and at one
point he was porting them to an early version of kvm-unit-tests/arm.
We can resurrect that shortly, as I'll be posting kvm-unit-tests/arm64
today.

drew

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 <lersek@redhat.com>
> ---
> 
> 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("/fw-cfg@%" 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;
> 

Self-NAK, will send a new version.

Laszlo