[Qemu-devel] [PATCH v3 0/7] fw_cfg, bootorder, and UEFI+'-kernel' on arm/virt

[Qemu-devel] [PATCH v3 0/7] fw_cfg, bootorder, and UEFI+'-kernel' on arm/virt

[Laszlo Ersek]

v3 fuses the two earlier patchsets in dependency order, seeks to address
Peter's comment (for v2) about the size of the fw_cfg region in the DTB,
and strives to implement Drew Jones' idea about the AWAP (as wide as
possible) access to the MMIO data register.

Changes are mentioned per patch too.

Tested on aarch64 KVM, x86_64 TCG, and aarch64 TCG.

Thanks
Laszlo

Laszlo Ersek (7):
  fw_cfg: max access size and region size are the same for MMIO data reg
  fw_cfg: introduce the "data_memwidth" property
  fw_cfg: expose the "data_memwidth" prop with
    fw_cfg_init_data_memwidth()
  arm: add fw_cfg to "virt" board
  hw/loader: split out load_image_gzipped_buffer()
  hw/arm: pass pristine kernel image to guest firmware over fw_cfg
  hw/arm/virt: enable passing of EFI-stubbed kernel to guest UEFI
    firmware

 include/hw/arm/arm.h      |  5 +++
 include/hw/loader.h       |  9 +++++
 include/hw/nvram/fw_cfg.h |  3 ++
 hw/arm/boot.c             | 91 ++++++++++++++++++++++++++++++++++++++++++++---
 hw/arm/virt.c             | 22 ++++++++++++
 hw/core/loader.c          | 30 +++++++++++-----
 hw/nvram/fw_cfg.c         | 31 ++++++++++++----
 7 files changed, 171 insertions(+), 20 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/7] fw_cfg: max access size and region size are the same for MMIO data reg

[Laszlo Ersek]

Make it clear that the maximum access size to the MMIO data register
determines the full size of the memory region.

Currently the max access size is 1. Ensure that if a larger size were used
in "fw_cfg_data_mem_ops.valid.max_access_size", the memory subsystem would
split the access to byte-sized accesses internally, in increasing address
order.

fw_cfg_data_mem_read() and fw_cfg_data_mem_write() don't care about
"address" or "size"; they just call the sequential fw_cfg_read() and
fw_cfg_write() functions, correspondingly. Therefore the automatic
splitting is just right. (The endianness of "fw_cfg_data_mem_ops" is
native.)

This patch doesn't change behavior.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v3:
    - new in v3 [Drew Jones]

 hw/nvram/fw_cfg.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index a7122ee..23ea0fe 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -30,9 +30,8 @@
 #include "qemu/error-report.h"
 #include "qemu/config-file.h"
 
 #define FW_CFG_SIZE 2
-#define FW_CFG_DATA_SIZE 1
 #define TYPE_FW_CFG "fw_cfg"
 #define FW_CFG_NAME "fw_cfg"
 #define FW_CFG_PATH "/machine/" FW_CFG_NAME
 #define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG)
@@ -323,8 +322,9 @@ static const MemoryRegionOps fw_cfg_data_mem_ops = {
     .valid = {
         .min_access_size = 1,
         .max_access_size = 1,
     },
+    .impl.max_access_size = 1,
 };
 
 static const MemoryRegionOps fw_cfg_comb_mem_ops = {
     .read = fw_cfg_comb_read,
@@ -608,9 +608,10 @@ static void fw_cfg_initfn(Object *obj)
     memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops, s,
                           "fwcfg.ctl", FW_CFG_SIZE);
     sysbus_init_mmio(sbd, &s->ctl_iomem);
     memory_region_init_io(&s->data_iomem, OBJECT(s), &fw_cfg_data_mem_ops, s,
-                          "fwcfg.data", FW_CFG_DATA_SIZE);
+                          "fwcfg.data",
+                          fw_cfg_data_mem_ops.valid.max_access_size);
     sysbus_init_mmio(sbd, &s->data_iomem);
     /* In case ctl and data overlap: */
     memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops, s,
                           "fwcfg", FW_CFG_SIZE);
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 2/7] fw_cfg: introduce the "data_memwidth" property

[Laszlo Ersek]

The "data_memwidth" property is capable of changing the maximum valid
access size to the MMIO data register, and (corresponding to the previous
patch) resizes the memory region similarly, at device realization time.

(Because "data_iomem" is configured and installed dynamically now, we must
delay those steps to the realize callback.)

The default value of "data_memwidth" is set so that we don't yet diverge
from "fw_cfg_data_mem_ops".

Most of the fw_cfg users will stick with the default, and for them we
should continue using the statically allocated "fw_cfg_data_mem_ops". This
is beneficial for debugging because gdb can resolve pointers referencing
static objects to the names of those objects.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v3:
    - new in v3 [Drew Jones]

 hw/nvram/fw_cfg.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 23ea0fe..6073f16 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -50,8 +50,9 @@ struct FWCfgState {
     /*< public >*/
 
     MemoryRegion ctl_iomem, data_iomem, comb_iomem;
     uint32_t ctl_iobase, data_iobase;
+    uint32_t data_memwidth;
     FWCfgEntry entries[2][FW_CFG_MAX_ENTRY];
     FWCfgFiles *files;
     uint16_t cur_entry;
     uint32_t cur_offset;
@@ -569,8 +570,10 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
 
     dev = qdev_create(NULL, TYPE_FW_CFG);
     qdev_prop_set_uint32(dev, "ctl_iobase", ctl_port);
     qdev_prop_set_uint32(dev, "data_iobase", data_port);
+    qdev_prop_set_uint32(dev, "data_memwidth",
+                         fw_cfg_data_mem_ops.valid.max_access_size);
     d = SYS_BUS_DEVICE(dev);
 
     s = FW_CFG(dev);
 
@@ -607,12 +610,8 @@ static void fw_cfg_initfn(Object *obj)
 
     memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops, s,
                           "fwcfg.ctl", FW_CFG_SIZE);
     sysbus_init_mmio(sbd, &s->ctl_iomem);
-    memory_region_init_io(&s->data_iomem, OBJECT(s), &fw_cfg_data_mem_ops, s,
-                          "fwcfg.data",
-                          fw_cfg_data_mem_ops.valid.max_access_size);
-    sysbus_init_mmio(sbd, &s->data_iomem);
     /* In case ctl and data overlap: */
     memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops, s,
                           "fwcfg", FW_CFG_SIZE);
 }
@@ -620,9 +619,20 @@ static void fw_cfg_initfn(Object *obj)
 static void fw_cfg_realize(DeviceState *dev, Error **errp)
 {
     FWCfgState *s = FW_CFG(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    const MemoryRegionOps *data_mem_ops = &fw_cfg_data_mem_ops;
 
+    if (s->data_memwidth > data_mem_ops->valid.max_access_size) {
+        MemoryRegionOps *ops;
+
+        ops = g_memdup(data_mem_ops, sizeof(*data_mem_ops));
+        ops->valid.max_access_size = s->data_memwidth;
+        data_mem_ops = ops;
+    }
+    memory_region_init_io(&s->data_iomem, OBJECT(s), data_mem_ops, s,
+                          "fwcfg.data", data_mem_ops->valid.max_access_size);
+    sysbus_init_mmio(sbd, &s->data_iomem);
 
     if (s->ctl_iobase + 1 == s->data_iobase) {
         sysbus_add_io(sbd, s->ctl_iobase, &s->comb_iomem);
     } else {
@@ -637,8 +647,9 @@ static void fw_cfg_realize(DeviceState *dev, Error **errp)
 
 static Property fw_cfg_properties[] = {
     DEFINE_PROP_UINT32("ctl_iobase", FWCfgState, ctl_iobase, -1),
     DEFINE_PROP_UINT32("data_iobase", FWCfgState, data_iobase, -1),
+    DEFINE_PROP_UINT32("data_memwidth", FWCfgState, data_memwidth, -1),
     DEFINE_PROP_END_OF_LIST(),
 };
 
 FWCfgState *fw_cfg_find(void)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 2/7] fw_cfg: introduce the "data_memwidth" property

[Peter Maydell]

On 9 December 2014 at 01:13, Laszlo Ersek <lersek@redhat.com> wrote:
> The "data_memwidth" property is capable of changing the maximum valid
> access size to the MMIO data register, and (corresponding to the previous
> patch) resizes the memory region similarly, at device realization time.
>
> (Because "data_iomem" is configured and installed dynamically now, we must
> delay those steps to the realize callback.)
>
> The default value of "data_memwidth" is set so that we don't yet diverge
> from "fw_cfg_data_mem_ops".
>
> Most of the fw_cfg users will stick with the default, and for them we
> should continue using the statically allocated "fw_cfg_data_mem_ops". This
> is beneficial for debugging because gdb can resolve pointers referencing
> static objects to the names of those objects.
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Mostly looks good to me. A few nits:

> +    qdev_prop_set_uint32(dev, "data_memwidth",
> +                         fw_cfg_data_mem_ops.valid.max_access_size);

Why not just make this the default value of the property, rather
than setting the default value to -1 and always manually overriding it?

> @@ -607,12 +610,8 @@ static void fw_cfg_initfn(Object *obj)
>
>      memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops, s,
>                            "fwcfg.ctl", FW_CFG_SIZE);
>      sysbus_init_mmio(sbd, &s->ctl_iomem);
> -    memory_region_init_io(&s->data_iomem, OBJECT(s), &fw_cfg_data_mem_ops, s,
> -                          "fwcfg.data",
> -                          fw_cfg_data_mem_ops.valid.max_access_size);
> -    sysbus_init_mmio(sbd, &s->data_iomem);

>      /* In case ctl and data overlap: */
>      memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops, s,
>                            "fwcfg", FW_CFG_SIZE);
>  }
> @@ -620,9 +619,20 @@ static void fw_cfg_initfn(Object *obj)
>  static void fw_cfg_realize(DeviceState *dev, Error **errp)
>  {
>      FWCfgState *s = FW_CFG(dev);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    const MemoryRegionOps *data_mem_ops = &fw_cfg_data_mem_ops;
>
> +    if (s->data_memwidth > data_mem_ops->valid.max_access_size) {
> +        MemoryRegionOps *ops;
> +
> +        ops = g_memdup(data_mem_ops, sizeof(*data_mem_ops));
> +        ops->valid.max_access_size = s->data_memwidth;
> +        data_mem_ops = ops;
> +    }
> +    memory_region_init_io(&s->data_iomem, OBJECT(s), data_mem_ops, s,
> +                          "fwcfg.data", data_mem_ops->valid.max_access_size);
> +    sysbus_init_mmio(sbd, &s->data_iomem);

The property changes the width of the data port, but only in
the case where it's not combined with the control port
(there the data width remains always 1). Is it worth throwing
an error in realize if the caller tried to set data_memwidth
and also use the combined-port? (Possibly even if the caller
set data_memwidth and tried to use data_iobase at all? Does
it make sense to define an AWAP I/O port ?)

>
>      if (s->ctl_iobase + 1 == s->data_iobase) {
>          sysbus_add_io(sbd, s->ctl_iobase, &s->comb_iomem);
>      } else {
> @@ -637,8 +647,9 @@ static void fw_cfg_realize(DeviceState *dev, Error **errp)
>
>  static Property fw_cfg_properties[] = {
>      DEFINE_PROP_UINT32("ctl_iobase", FWCfgState, ctl_iobase, -1),
>      DEFINE_PROP_UINT32("data_iobase", FWCfgState, data_iobase, -1),
> +    DEFINE_PROP_UINT32("data_memwidth", FWCfgState, data_memwidth, -1),
>      DEFINE_PROP_END_OF_LIST(),
>  };

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 2/7] fw_cfg: introduce the "data_memwidth" property

[Laszlo Ersek]

On 12/12/14 13:49, Peter Maydell wrote:
> On 9 December 2014 at 01:13, Laszlo Ersek <lersek@redhat.com> wrote:
>> The "data_memwidth" property is capable of changing the maximum valid
>> access size to the MMIO data register, and (corresponding to the previous
>> patch) resizes the memory region similarly, at device realization time.
>>
>> (Because "data_iomem" is configured and installed dynamically now, we must
>> delay those steps to the realize callback.)
>>
>> The default value of "data_memwidth" is set so that we don't yet diverge
>> from "fw_cfg_data_mem_ops".
>>
>> Most of the fw_cfg users will stick with the default, and for them we
>> should continue using the statically allocated "fw_cfg_data_mem_ops". This
>> is beneficial for debugging because gdb can resolve pointers referencing
>> static objects to the names of those objects.
>>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> 
> Mostly looks good to me. A few nits:
> 
>> +    qdev_prop_set_uint32(dev, "data_memwidth",
>> +                         fw_cfg_data_mem_ops.valid.max_access_size);
> 
> Why not just make this the default value of the property, rather
> than setting the default value to -1 and always manually overriding it?

This hunk just prepares the ground for the next patch, where the
property will be set from a new function parameter. Ultimately I wanted
to leave the default value of the property at -1, for consistency with
the other two properties.

> 
>> @@ -607,12 +610,8 @@ static void fw_cfg_initfn(Object *obj)
>>
>>      memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops, s,
>>                            "fwcfg.ctl", FW_CFG_SIZE);
>>      sysbus_init_mmio(sbd, &s->ctl_iomem);
>> -    memory_region_init_io(&s->data_iomem, OBJECT(s), &fw_cfg_data_mem_ops, s,
>> -                          "fwcfg.data",
>> -                          fw_cfg_data_mem_ops.valid.max_access_size);
>> -    sysbus_init_mmio(sbd, &s->data_iomem);
> 
>>      /* In case ctl and data overlap: */
>>      memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops, s,
>>                            "fwcfg", FW_CFG_SIZE);
>>  }
>> @@ -620,9 +619,20 @@ static void fw_cfg_initfn(Object *obj)
>>  static void fw_cfg_realize(DeviceState *dev, Error **errp)
>>  {
>>      FWCfgState *s = FW_CFG(dev);
>>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> +    const MemoryRegionOps *data_mem_ops = &fw_cfg_data_mem_ops;
>>
>> +    if (s->data_memwidth > data_mem_ops->valid.max_access_size) {
>> +        MemoryRegionOps *ops;
>> +
>> +        ops = g_memdup(data_mem_ops, sizeof(*data_mem_ops));
>> +        ops->valid.max_access_size = s->data_memwidth;
>> +        data_mem_ops = ops;
>> +    }
>> +    memory_region_init_io(&s->data_iomem, OBJECT(s), data_mem_ops, s,
>> +                          "fwcfg.data", data_mem_ops->valid.max_access_size);
>> +    sysbus_init_mmio(sbd, &s->data_iomem);
> 
> The property changes the width of the data port, but only in
> the case where it's not combined with the control port
> (there the data width remains always 1). Is it worth throwing
> an error in realize if the caller tried to set data_memwidth
> and also use the combined-port? (Possibly even if the caller
> set data_memwidth and tried to use data_iobase at all? Does
> it make sense to define an AWAP I/O port ?)

I considered "locking down" the new interface, and preventing callers
from passing in any IO ports when they want a wider MMIO data register.
I didn't do that because someone might want a wider ioport mapping at
some point (although no current such user exists and I couldn't name
what the advantage would be in it). Unless you use the combined thing,
the wide data register should work with the ioport mapping too.

The combined case I thought to leave simply undefined.

If you want, I can set an error, but then I'd prefer to prevent callers
from passing IO ports through the new data_memwidth-taking functions.

Thanks
Laszlo

> 
>>
>>      if (s->ctl_iobase + 1 == s->data_iobase) {
>>          sysbus_add_io(sbd, s->ctl_iobase, &s->comb_iomem);
>>      } else {
>> @@ -637,8 +647,9 @@ static void fw_cfg_realize(DeviceState *dev, Error **errp)
>>
>>  static Property fw_cfg_properties[] = {
>>      DEFINE_PROP_UINT32("ctl_iobase", FWCfgState, ctl_iobase, -1),
>>      DEFINE_PROP_UINT32("data_iobase", FWCfgState, data_iobase, -1),
>> +    DEFINE_PROP_UINT32("data_memwidth", FWCfgState, data_memwidth, -1),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH v3 2/7] fw_cfg: introduce the "data_memwidth" property

[Peter Maydell]

On 12 December 2014 at 13:39, Laszlo Ersek <lersek@redhat.com> wrote:
> I considered "locking down" the new interface, and preventing callers
> from passing in any IO ports when they want a wider MMIO data register.
> I didn't do that because someone might want a wider ioport mapping at
> some point (although no current such user exists and I couldn't name
> what the advantage would be in it). Unless you use the combined thing,
> the wide data register should work with the ioport mapping too.
>
> The combined case I thought to leave simply undefined.
>
> If you want, I can set an error, but then I'd prefer to prevent callers
> from passing IO ports through the new data_memwidth-taking functions.

Yeah, I don't think we need to make the combined case work, but it
does seem worth at least making it fail cleanly if anybody tries it,
rather than silently doing the wrong thing.

-- PMM

[Qemu-devel] [PATCH v3 3/7] fw_cfg: expose the "data_memwidth" prop with fw_cfg_init_data_memwidth()

[Laszlo Ersek]

We rebase fw_cfg_init() to the new function for compatibility with current
callers.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v3:
    - new in v3 [Drew Jones]

 include/hw/nvram/fw_cfg.h |  3 +++
 hw/nvram/fw_cfg.c         | 15 +++++++++++----
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 56e1ed7..fd7c35c 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -77,8 +77,11 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
                               FWCfgReadCallback callback, void *callback_opaque,
                               void *data, size_t len);
 void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
                          size_t len);
+FWCfgState *fw_cfg_init_data_memwidth(uint32_t ctl_port, uint32_t data_port,
+                                      hwaddr ctl_addr, hwaddr data_addr,
+                                      uint32_t data_memwidth);
 FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
                         hwaddr crl_addr, hwaddr data_addr);
 
 FWCfgState *fw_cfg_find(void);
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 6073f16..55b7f41 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -560,20 +560,20 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data)
     FWCfgState *s = container_of(n, FWCfgState, machine_ready);
     qemu_register_reset(fw_cfg_machine_reset, s);
 }
 
-FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
-                        hwaddr ctl_addr, hwaddr data_addr)
+FWCfgState *fw_cfg_init_data_memwidth(uint32_t ctl_port, uint32_t data_port,
+                                      hwaddr ctl_addr, hwaddr data_addr,
+                                      uint32_t data_memwidth)
 {
     DeviceState *dev;
     SysBusDevice *d;
     FWCfgState *s;
 
     dev = qdev_create(NULL, TYPE_FW_CFG);
     qdev_prop_set_uint32(dev, "ctl_iobase", ctl_port);
     qdev_prop_set_uint32(dev, "data_iobase", data_port);
-    qdev_prop_set_uint32(dev, "data_memwidth",
-                         fw_cfg_data_mem_ops.valid.max_access_size);
+    qdev_prop_set_uint32(dev, "data_memwidth", data_memwidth);
     d = SYS_BUS_DEVICE(dev);
 
     s = FW_CFG(dev);
 
@@ -602,8 +602,15 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
 
     return s;
 }
 
+FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
+                        hwaddr ctl_addr, hwaddr data_addr)
+{
+    return fw_cfg_init_data_memwidth(ctl_port, data_port, ctl_addr, data_addr,
+                                     fw_cfg_data_mem_ops.valid.max_access_size);
+}
+
 static void fw_cfg_initfn(Object *obj)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
     FWCfgState *s = FW_CFG(obj);
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 4/7] 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.

Because MMIO access is slow on ARM KVM, we enable the guest, with
fw_cfg_init_data_memwidth(), to transfer up to 8 bytes with a single
access. This has been measured to speed up transfers up to 7.5-fold,
relative to single byte data access, on both ARM KVM and x86_64 TCG.

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:
    v3:
    - enable 8-byte wide access to fw_cfg MMIO data port [Drew]
    - expose the exact size of the fused region in the DTB [Peter]
    - reorder data register and control register so that we can keep the
      region contiguous and still conform to alignment requirements [Laszlo]

 hw/arm/virt.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 314e55b..274aaae 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -67,8 +67,9 @@ enum {
     VIRT_GIC_CPU,
     VIRT_UART,
     VIRT_MMIO,
     VIRT_RTC,
+    VIRT_FW_CFG,
 };
 
 typedef struct MemMapEntry {
     hwaddr base;
@@ -106,8 +107,9 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_GIC_DIST] =   { 0x08000000, 0x00010000 },
     [VIRT_GIC_CPU] =    { 0x08010000, 0x00010000 },
     [VIRT_UART] =       { 0x09000000, 0x00001000 },
     [VIRT_RTC] =        { 0x09010000, 0x00001000 },
+    [VIRT_FW_CFG] =     { 0x09020000, 0x0000000a },
     [VIRT_MMIO] =       { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     /* 0x10000000 .. 0x40000000 reserved for PCI */
     [VIRT_MEM] =        { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
@@ -518,8 +520,25 @@ static void create_flash(const VirtBoardInfo *vbi)
     qemu_fdt_setprop_cell(vbi->fdt, nodename, "bank-width", 4);
     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_data_memwidth(0, 0, base + 8, base, 8);
+
+    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;
 
@@ -603,8 +622,10 @@ static void machvirt_init(MachineState *machine)
      * no backend is created the transport will just sit harmlessly idle.
      */
     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;
     vbi->bootinfo.initrd_filename = machine->initrd_filename;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 4/7] arm: add fw_cfg to "virt" board

[Peter Maydell]

On 9 December 2014 at 01:13, 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.
>
> Because MMIO access is slow on ARM KVM, we enable the guest, with
> fw_cfg_init_data_memwidth(), to transfer up to 8 bytes with a single
> access. This has been measured to speed up transfers up to 7.5-fold,
> relative to single byte data access, on both ARM KVM and x86_64 TCG.
>
> 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:
>     v3:
>     - enable 8-byte wide access to fw_cfg MMIO data port [Drew]
>     - expose the exact size of the fused region in the DTB [Peter]
>     - reorder data register and control register so that we can keep the
>       region contiguous and still conform to alignment requirements [Laszlo]
>
>  hw/arm/virt.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 314e55b..274aaae 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -67,8 +67,9 @@ enum {
>      VIRT_GIC_CPU,
>      VIRT_UART,
>      VIRT_MMIO,
>      VIRT_RTC,
> +    VIRT_FW_CFG,
>  };
>
>  typedef struct MemMapEntry {
>      hwaddr base;
> @@ -106,8 +107,9 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_GIC_DIST] =   { 0x08000000, 0x00010000 },
>      [VIRT_GIC_CPU] =    { 0x08010000, 0x00010000 },
>      [VIRT_UART] =       { 0x09000000, 0x00001000 },
>      [VIRT_RTC] =        { 0x09010000, 0x00001000 },
> +    [VIRT_FW_CFG] =     { 0x09020000, 0x0000000a },
>      [VIRT_MMIO] =       { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      /* 0x10000000 .. 0x40000000 reserved for PCI */
>      [VIRT_MEM] =        { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
> @@ -518,8 +520,25 @@ static void create_flash(const VirtBoardInfo *vbi)
>      qemu_fdt_setprop_cell(vbi->fdt, nodename, "bank-width", 4);
>      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_data_memwidth(0, 0, base + 8, base, 8);

fw_cfg_init_data_memwidth() adds a bunch of FW_CFG keys
(bootsplash stuff, reboot timeouts, etc). Are you happy that
they all make sense to export on ARM? (I guess we're already doing
that on the other non-x86 fw_cfg users.)

Assuming so,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH v3 4/7] arm: add fw_cfg to "virt" board

[Laszlo Ersek]

On 12/12/14 13:55, Peter Maydell wrote:
> On 9 December 2014 at 01:13, 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.
>>
>> Because MMIO access is slow on ARM KVM, we enable the guest, with
>> fw_cfg_init_data_memwidth(), to transfer up to 8 bytes with a single
>> access. This has been measured to speed up transfers up to 7.5-fold,
>> relative to single byte data access, on both ARM KVM and x86_64 TCG.
>>
>> 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:
>>     v3:
>>     - enable 8-byte wide access to fw_cfg MMIO data port [Drew]
>>     - expose the exact size of the fused region in the DTB [Peter]
>>     - reorder data register and control register so that we can keep the
>>       region contiguous and still conform to alignment requirements [Laszlo]
>>
>>  hw/arm/virt.c | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 314e55b..274aaae 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -67,8 +67,9 @@ enum {
>>      VIRT_GIC_CPU,
>>      VIRT_UART,
>>      VIRT_MMIO,
>>      VIRT_RTC,
>> +    VIRT_FW_CFG,
>>  };
>>
>>  typedef struct MemMapEntry {
>>      hwaddr base;
>> @@ -106,8 +107,9 @@ static const MemMapEntry a15memmap[] = {
>>      [VIRT_GIC_DIST] =   { 0x08000000, 0x00010000 },
>>      [VIRT_GIC_CPU] =    { 0x08010000, 0x00010000 },
>>      [VIRT_UART] =       { 0x09000000, 0x00001000 },
>>      [VIRT_RTC] =        { 0x09010000, 0x00001000 },
>> +    [VIRT_FW_CFG] =     { 0x09020000, 0x0000000a },
>>      [VIRT_MMIO] =       { 0x0a000000, 0x00000200 },
>>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>>      /* 0x10000000 .. 0x40000000 reserved for PCI */
>>      [VIRT_MEM] =        { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
>> @@ -518,8 +520,25 @@ static void create_flash(const VirtBoardInfo *vbi)
>>      qemu_fdt_setprop_cell(vbi->fdt, nodename, "bank-width", 4);
>>      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_data_memwidth(0, 0, base + 8, base, 8);
> 
> fw_cfg_init_data_memwidth() adds a bunch of FW_CFG keys
> (bootsplash stuff, reboot timeouts, etc). Are you happy that
> they all make sense to export on ARM? (I guess we're already doing
> that on the other non-x86 fw_cfg users.)

Yes, some of those I'm actually happy about (because I can immediately
retrieve them in the firmware, without doing more qemu work); and the
rest doesn't hurt (I might utilize them later on).

> 
> Assuming so,
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> -- PMM
> 

Thanks!
Laszlo

[Qemu-devel] [PATCH v3 5/7] hw/loader: split out load_image_gzipped_buffer()

[Laszlo Ersek]

In the next patch we'd like to reuse the image decompression facility
without installing the output as a ROM at a specific guest-phys address.

In addition, expose LOAD_IMAGE_MAX_GUNZIP_BYTES, because that's a
straightforward "max_sz" argument for the new load_image_gzipped_buffer().

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v3:
    - unchanged

 include/hw/loader.h |  9 +++++++++
 hw/core/loader.c    | 30 +++++++++++++++++++++---------
 2 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/include/hw/loader.h b/include/hw/loader.h
index 6481639..8997620 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -15,8 +15,17 @@ int get_image_size(const char *filename);
 int load_image(const char *filename, uint8_t *addr); /* deprecated */
 ssize_t load_image_size(const char *filename, void *addr, size_t size);
 int load_image_targphys(const char *filename, hwaddr,
                         uint64_t max_sz);
+
+/* This is the limit on the maximum uncompressed image size that
+ * load_image_gzipped_buffer() and load_image_gzipped() will read. It prevents
+ * g_malloc() in those functions from allocating a huge amount of memory.
+ */
+#define LOAD_IMAGE_MAX_GUNZIP_BYTES (256 << 20)
+
+int load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
+                              uint8_t **buffer);
 int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
 
 #define ELF_LOAD_FAILED       -1
 #define ELF_LOAD_NOT_ELF      -2
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 7527fd3..933ab64 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -613,16 +613,11 @@ int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz)
     return load_uboot_image(filename, NULL, &addr, NULL, IH_TYPE_RAMDISK,
                             NULL, NULL);
 }
 
-/* This simply prevents g_malloc in the function below from allocating
- * a huge amount of memory, by placing a limit on the maximum
- * uncompressed image size that load_image_gzipped will read.
- */
-#define LOAD_IMAGE_MAX_GUNZIP_BYTES (256 << 20)
-
-/* Load a gzip-compressed kernel. */
-int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz)
+/* Load a gzip-compressed kernel to a dynamically allocated buffer. */
+int load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
+                              uint8_t **buffer)
 {
     uint8_t *compressed_data = NULL;
     uint8_t *data = NULL;
     gsize len;
@@ -652,17 +647,34 @@ int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz)
                 filename);
         goto out;
     }
 
-    rom_add_blob_fixed(filename, data, bytes, addr);
+    /* trim to actual size and return to caller */
+    *buffer = g_realloc(data, bytes);
     ret = bytes;
+    /* ownership has been transferred to caller */
+    data = NULL;
 
  out:
     g_free(compressed_data);
     g_free(data);
     return ret;
 }
 
+/* Load a gzip-compressed kernel. */
+int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz)
+{
+    int bytes;
+    uint8_t *data;
+
+    bytes = load_image_gzipped_buffer (filename, max_sz, &data);
+    if (bytes != -1) {
+        rom_add_blob_fixed(filename, data, bytes, addr);
+        g_free(data);
+    }
+    return bytes;
+}
+
 /*
  * Functions for reboot-persistent memory regions.
  *  - used for vga bios and option roms.
  *  - also linux kernel (-kernel / -initrd).
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 5/7] hw/loader: split out load_image_gzipped_buffer()

[Peter Maydell]

On 9 December 2014 at 01:13, Laszlo Ersek <lersek@redhat.com> wrote:
> In the next patch we'd like to reuse the image decompression facility
> without installing the output as a ROM at a specific guest-phys address.
>
> In addition, expose LOAD_IMAGE_MAX_GUNZIP_BYTES, because that's a
> straightforward "max_sz" argument for the new load_image_gzipped_buffer().
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---

> +int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz)
> +{
> +    int bytes;
> +    uint8_t *data;
> +
> +    bytes = load_image_gzipped_buffer (filename, max_sz, &data);

Doesn't checkpatch complain about the space before the "(" here?

Otherwise:
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH v3 5/7] hw/loader: split out load_image_gzipped_buffer()

[Laszlo Ersek]

On 12/12/14 14:11, Peter Maydell wrote:
> On 9 December 2014 at 01:13, Laszlo Ersek <lersek@redhat.com> wrote:
>> In the next patch we'd like to reuse the image decompression facility
>> without installing the output as a ROM at a specific guest-phys address.
>>
>> In addition, expose LOAD_IMAGE_MAX_GUNZIP_BYTES, because that's a
>> straightforward "max_sz" argument for the new load_image_gzipped_buffer().
>>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
> 
>> +int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz)
>> +{
>> +    int bytes;
>> +    uint8_t *data;
>> +
>> +    bytes = load_image_gzipped_buffer (filename, max_sz, &data);
> 
> Doesn't checkpatch complain about the space before the "(" here?

(Checkpatch? What's that? :))

It sure does.

WARNING: space prohibited between function name and open parenthesis '('

Obviously, the edk2 coding style *requires* that damn space. I got so
used to it that it slipped in here.

Thanks!
Laszlo

> 
> Otherwise:
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> -- PMM
> 

[Qemu-devel] [PATCH v3 6/7] hw/arm: pass pristine kernel image to guest firmware over fw_cfg

[Laszlo Ersek]

Introduce the new boolean field "arm_boot_info.firmware_loaded". When this
field is set, it means that the portion of guest DRAM that the VCPU
normally starts to execute, or the pflash chip that the VCPU normally
starts to execute, has been populated by board-specific code with
full-fledged guest firmware code, before the board calls
arm_load_kernel().

Simultaneously, "arm_boot_info.firmware_loaded" guarantees that the board
code has set up the global firmware config instance, for arm_load_kernel()
to find with fw_cfg_find().

Guest kernel (-kernel) and guest firmware (-bios, -pflash) has always been
possible to specify independently on the command line. The following cases
should be considered:

nr  -bios    -pflash  -kernel  description
             unit#0
--  -------  -------  -------  -------------------------------------------
1   present  present  absent   Board code rejects this case, -bios and
    present  present  present  -pflash unit#0 are exclusive. Left intact
                               by this patch.

2   absent   absent   present  Traditional kernel loading, with qemu's
                               minimal board firmware. Left intact by this
                               patch.

3   absent   present  absent   Preexistent case for booting guest firmware
    present  absent   absent   loaded with -bios or -pflash. Left intact
                               by this patch.

4   absent   absent   absent   Preexistent case for not loading any
                               firmware or kernel up-front. Left intact by
                               this patch.

5   present  absent   present  New case introduced by this patch: kernel
    absent   present  present  image is passed to externally loaded
                               firmware in unmodified form, using fw_cfg.

An easy way to see that this patch doesn't interfere with existing cases
is to realize that "info->firmware_loaded" is constant zero at this point.
Which makes the "outer" condition unchanged, and the "inner" condition
(with the fw_cfg-related code) dead.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v3:
    - unchanged

 include/hw/arm/arm.h |  5 +++
 hw/arm/boot.c        | 91 +++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 91 insertions(+), 5 deletions(-)

diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
index cefc9e6..dd69d66 100644
--- a/include/hw/arm/arm.h
+++ b/include/hw/arm/arm.h
@@ -65,8 +65,13 @@ struct arm_boot_info {
     int is_linux;
     hwaddr initrd_start;
     hwaddr initrd_size;
     hwaddr entry;
+
+    /* Boot firmware has been loaded, typically at address 0, with -bios or
+     * -pflash. It also implies that fw_cfg_find() will succeed.
+     */
+    bool firmware_loaded;
 };
 void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info);
 
 /* Multiplication factor to convert from system clock ticks to qemu timer
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 0014c34..01d6d3d 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -475,8 +475,62 @@ static void do_cpu_reset(void *opaque)
         }
     }
 }
 
+/**
+ * load_image_to_fw_cfg() - Load an image file into an fw_cfg entry identified
+ *                          by key.
+ * @fw_cfg:     The firmware config instance to store the data in.
+ * @size_key:   The firmware config key to store the size of the loaded data
+ *              under, with fw_cfg_add_i32().
+ * @data_key:   The firmware config key to store the loaded data under, with
+ *              fw_cfg_add_bytes().
+ * @image_name: The name of the image file to load. If it is NULL, the function
+ *              returns without doing anything.
+ * @decompress: Whether the image should be  decompressed (gunzipped) before
+ *              adding it to fw_cfg.
+ *
+ * In case of failure, the function prints an error message to stderr and the
+ * process exits with status 1.
+ */
+static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
+                                 uint16_t data_key, const char *image_name,
+                                 bool decompress)
+{
+    size_t size;
+    uint8_t *data;
+
+    if (image_name == NULL) {
+        return;
+    }
+
+    if (decompress) {
+        int bytes;
+
+        bytes = load_image_gzipped_buffer (image_name,
+                                           LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);
+        if (bytes == -1) {
+            fprintf(stderr, "failed to load and decompress \"%s\"\n",
+                    image_name);
+            exit(1);
+        }
+        size = bytes;
+    } else {
+        gchar *contents;
+        gsize length;
+
+        if (!g_file_get_contents(image_name, &contents, &length, NULL)) {
+            fprintf(stderr, "failed to load \"%s\"\n", image_name);
+            exit(1);
+        }
+        size = length;
+        data = (uint8_t *)contents;
+    }
+
+    fw_cfg_add_i32(fw_cfg, size_key, size);
+    fw_cfg_add_bytes(fw_cfg, data_key, data, size);
+}
+
 void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
 {
     CPUState *cs;
     int kernel_size;
@@ -497,21 +551,48 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
         qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
     }
 
     /* Load the kernel.  */
-    if (!info->kernel_filename) {
+    if (!info->kernel_filename || info->firmware_loaded) {
 
         if (have_dtb(info)) {
-            /* If we have a device tree blob, but no kernel to supply it to,
-             * copy it to the base of RAM for a bootloader to pick up.
+            /* If we have a device tree blob, but no kernel to supply it to (or
+             * the kernel is supposed to be loaded by the bootloader), copy the
+             * DTB to the base of RAM for the bootloader to pick up.
              */
             if (load_dtb(info->loader_start, info, 0) < 0) {
                 exit(1);
             }
         }
 
-        /* If no kernel specified, do nothing; we will start from address 0
-         * (typically a boot ROM image) in the same way as hardware.
+        if (info->kernel_filename) {
+            FWCfgState *fw_cfg;
+            bool decompress_kernel;
+
+            fw_cfg = fw_cfg_find();
+            decompress_kernel = arm_feature(&cpu->env, ARM_FEATURE_AARCH64);
+
+            /* Expose the kernel, the command line, and the initrd in fw_cfg.
+             * We don't process them here at all, it's all left to the
+             * firmware.
+             */
+            load_image_to_fw_cfg(fw_cfg,
+                                 FW_CFG_KERNEL_SIZE, FW_CFG_KERNEL_DATA,
+                                 info->kernel_filename, decompress_kernel);
+            load_image_to_fw_cfg(fw_cfg,
+                                 FW_CFG_INITRD_SIZE, FW_CFG_INITRD_DATA,
+                                 info->initrd_filename, false);
+
+            if (info->kernel_cmdline) {
+                fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE,
+                               strlen(info->kernel_cmdline) + 1);
+                fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA,
+                                  info->kernel_cmdline);
+            }
+        }
+
+        /* We will start from address 0 (typically a boot ROM image) in the
+         * same way as hardware.
          */
         return;
     }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 6/7] hw/arm: pass pristine kernel image to guest firmware over fw_cfg

[Peter Maydell]

On 9 December 2014 at 01:13, Laszlo Ersek <lersek@redhat.com> wrote:
> Introduce the new boolean field "arm_boot_info.firmware_loaded". When this
> field is set, it means that the portion of guest DRAM that the VCPU
> normally starts to execute, or the pflash chip that the VCPU normally
> starts to execute, has been populated by board-specific code with
> full-fledged guest firmware code, before the board calls
> arm_load_kernel().
>
> Simultaneously, "arm_boot_info.firmware_loaded" guarantees that the board
> code has set up the global firmware config instance, for arm_load_kernel()
> to find with fw_cfg_find().
>
> Guest kernel (-kernel) and guest firmware (-bios, -pflash) has always been
> possible to specify independently on the command line. The following cases
> should be considered:
>
> nr  -bios    -pflash  -kernel  description
>              unit#0
> --  -------  -------  -------  -------------------------------------------
> 1   present  present  absent   Board code rejects this case, -bios and
>     present  present  present  -pflash unit#0 are exclusive. Left intact
>                                by this patch.
>
> 2   absent   absent   present  Traditional kernel loading, with qemu's
>                                minimal board firmware. Left intact by this
>                                patch.
>
> 3   absent   present  absent   Preexistent case for booting guest firmware
>     present  absent   absent   loaded with -bios or -pflash. Left intact
>                                by this patch.
>
> 4   absent   absent   absent   Preexistent case for not loading any
>                                firmware or kernel up-front. Left intact by
>                                this patch.
>
> 5   present  absent   present  New case introduced by this patch: kernel
>     absent   present  present  image is passed to externally loaded
>                                firmware in unmodified form, using fw_cfg.
>
> An easy way to see that this patch doesn't interfere with existing cases
> is to realize that "info->firmware_loaded" is constant zero at this point.
> Which makes the "outer" condition unchanged, and the "inner" condition
> (with the fw_cfg-related code) dead.
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
>     v3:
>     - unchanged
>
>  include/hw/arm/arm.h |  5 +++
>  hw/arm/boot.c        | 91 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 91 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
> index cefc9e6..dd69d66 100644
> --- a/include/hw/arm/arm.h
> +++ b/include/hw/arm/arm.h
> @@ -65,8 +65,13 @@ struct arm_boot_info {
>      int is_linux;
>      hwaddr initrd_start;
>      hwaddr initrd_size;
>      hwaddr entry;
> +
> +    /* Boot firmware has been loaded, typically at address 0, with -bios or
> +     * -pflash. It also implies that fw_cfg_find() will succeed.
> +     */
> +    bool firmware_loaded;
>  };
>  void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info);
>
>  /* Multiplication factor to convert from system clock ticks to qemu timer
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 0014c34..01d6d3d 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -475,8 +475,62 @@ static void do_cpu_reset(void *opaque)
>          }
>      }
>  }
>
> +/**
> + * load_image_to_fw_cfg() - Load an image file into an fw_cfg entry identified
> + *                          by key.
> + * @fw_cfg:     The firmware config instance to store the data in.
> + * @size_key:   The firmware config key to store the size of the loaded data
> + *              under, with fw_cfg_add_i32().
> + * @data_key:   The firmware config key to store the loaded data under, with
> + *              fw_cfg_add_bytes().
> + * @image_name: The name of the image file to load. If it is NULL, the function
> + *              returns without doing anything.
> + * @decompress: Whether the image should be  decompressed (gunzipped) before
> + *              adding it to fw_cfg.
> + *
> + * In case of failure, the function prints an error message to stderr and the
> + * process exits with status 1.
> + */
> +static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
> +                                 uint16_t data_key, const char *image_name,
> +                                 bool decompress)
> +{
> +    size_t size;
> +    uint8_t *data;
> +
> +    if (image_name == NULL) {
> +        return;
> +    }
> +
> +    if (decompress) {
> +        int bytes;
> +
> +        bytes = load_image_gzipped_buffer (image_name,
> +                                           LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);

Stray space again.

> +        if (bytes == -1) {
> +            fprintf(stderr, "failed to load and decompress \"%s\"\n",
> +                    image_name);
> +            exit(1);
> +        }
> +        size = bytes;
> +    } else {
> +        gchar *contents;
> +        gsize length;
> +
> +        if (!g_file_get_contents(image_name, &contents, &length, NULL)) {
> +            fprintf(stderr, "failed to load \"%s\"\n", image_name);
> +            exit(1);
> +        }
> +        size = length;
> +        data = (uint8_t *)contents;
> +    }
> +
> +    fw_cfg_add_i32(fw_cfg, size_key, size);
> +    fw_cfg_add_bytes(fw_cfg, data_key, data, size);

We have no way to free these buffers later but x86 does the same,
so never mind.

> +}
> +
>  void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>  {
>      CPUState *cs;
>      int kernel_size;
> @@ -497,21 +551,48 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>          qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
>      }
>
>      /* Load the kernel.  */
> -    if (!info->kernel_filename) {
> +    if (!info->kernel_filename || info->firmware_loaded) {
>
>          if (have_dtb(info)) {
> -            /* If we have a device tree blob, but no kernel to supply it to,
> -             * copy it to the base of RAM for a bootloader to pick up.
> +            /* If we have a device tree blob, but no kernel to supply it to (or
> +             * the kernel is supposed to be loaded by the bootloader), copy the
> +             * DTB to the base of RAM for the bootloader to pick up.
>               */
>              if (load_dtb(info->loader_start, info, 0) < 0) {
>                  exit(1);
>              }
>          }
>
> -        /* If no kernel specified, do nothing; we will start from address 0
> -         * (typically a boot ROM image) in the same way as hardware.
> +        if (info->kernel_filename) {
> +            FWCfgState *fw_cfg;
> +            bool decompress_kernel;
> +
> +            fw_cfg = fw_cfg_find();
> +            decompress_kernel = arm_feature(&cpu->env, ARM_FEATURE_AARCH64);

If we have a real bootloader in the UEFI firmware, why do we need
to do decompression for it? We only do this in the builtin bootloader
because QEMU is acting as the bootloader and has to support the
feature itself. I would have thought that the UEFI builtin kernel
booting support already supported decompressing the kernel...

Otherwise looks OK.

-- PMM

Re: [Qemu-devel] [PATCH v3 6/7] hw/arm: pass pristine kernel image to guest firmware over fw_cfg

[Laszlo Ersek]

On 12/12/14 14:20, Peter Maydell wrote:
> On 9 December 2014 at 01:13, Laszlo Ersek <lersek@redhat.com> wrote:
>> Introduce the new boolean field "arm_boot_info.firmware_loaded". When this
>> field is set, it means that the portion of guest DRAM that the VCPU
>> normally starts to execute, or the pflash chip that the VCPU normally
>> starts to execute, has been populated by board-specific code with
>> full-fledged guest firmware code, before the board calls
>> arm_load_kernel().
>>
>> Simultaneously, "arm_boot_info.firmware_loaded" guarantees that the board
>> code has set up the global firmware config instance, for arm_load_kernel()
>> to find with fw_cfg_find().
>>
>> Guest kernel (-kernel) and guest firmware (-bios, -pflash) has always been
>> possible to specify independently on the command line. The following cases
>> should be considered:
>>
>> nr  -bios    -pflash  -kernel  description
>>              unit#0
>> --  -------  -------  -------  -------------------------------------------
>> 1   present  present  absent   Board code rejects this case, -bios and
>>     present  present  present  -pflash unit#0 are exclusive. Left intact
>>                                by this patch.
>>
>> 2   absent   absent   present  Traditional kernel loading, with qemu's
>>                                minimal board firmware. Left intact by this
>>                                patch.
>>
>> 3   absent   present  absent   Preexistent case for booting guest firmware
>>     present  absent   absent   loaded with -bios or -pflash. Left intact
>>                                by this patch.
>>
>> 4   absent   absent   absent   Preexistent case for not loading any
>>                                firmware or kernel up-front. Left intact by
>>                                this patch.
>>
>> 5   present  absent   present  New case introduced by this patch: kernel
>>     absent   present  present  image is passed to externally loaded
>>                                firmware in unmodified form, using fw_cfg.
>>
>> An easy way to see that this patch doesn't interfere with existing cases
>> is to realize that "info->firmware_loaded" is constant zero at this point.
>> Which makes the "outer" condition unchanged, and the "inner" condition
>> (with the fw_cfg-related code) dead.
>>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>>     v3:
>>     - unchanged
>>
>>  include/hw/arm/arm.h |  5 +++
>>  hw/arm/boot.c        | 91 +++++++++++++++++++++++++++++++++++++++++++++++++---
>>  2 files changed, 91 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
>> index cefc9e6..dd69d66 100644
>> --- a/include/hw/arm/arm.h
>> +++ b/include/hw/arm/arm.h
>> @@ -65,8 +65,13 @@ struct arm_boot_info {
>>      int is_linux;
>>      hwaddr initrd_start;
>>      hwaddr initrd_size;
>>      hwaddr entry;
>> +
>> +    /* Boot firmware has been loaded, typically at address 0, with -bios or
>> +     * -pflash. It also implies that fw_cfg_find() will succeed.
>> +     */
>> +    bool firmware_loaded;
>>  };
>>  void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info);
>>
>>  /* Multiplication factor to convert from system clock ticks to qemu timer
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 0014c34..01d6d3d 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -475,8 +475,62 @@ static void do_cpu_reset(void *opaque)
>>          }
>>      }
>>  }
>>
>> +/**
>> + * load_image_to_fw_cfg() - Load an image file into an fw_cfg entry identified
>> + *                          by key.
>> + * @fw_cfg:     The firmware config instance to store the data in.
>> + * @size_key:   The firmware config key to store the size of the loaded data
>> + *              under, with fw_cfg_add_i32().
>> + * @data_key:   The firmware config key to store the loaded data under, with
>> + *              fw_cfg_add_bytes().
>> + * @image_name: The name of the image file to load. If it is NULL, the function
>> + *              returns without doing anything.
>> + * @decompress: Whether the image should be  decompressed (gunzipped) before
>> + *              adding it to fw_cfg.
>> + *
>> + * In case of failure, the function prints an error message to stderr and the
>> + * process exits with status 1.
>> + */
>> +static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
>> +                                 uint16_t data_key, const char *image_name,
>> +                                 bool decompress)
>> +{
>> +    size_t size;
>> +    uint8_t *data;
>> +
>> +    if (image_name == NULL) {
>> +        return;
>> +    }
>> +
>> +    if (decompress) {
>> +        int bytes;
>> +
>> +        bytes = load_image_gzipped_buffer (image_name,
>> +                                           LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);
> 
> Stray space again.

Will fix, thanks.

> 
>> +        if (bytes == -1) {
>> +            fprintf(stderr, "failed to load and decompress \"%s\"\n",
>> +                    image_name);
>> +            exit(1);
>> +        }
>> +        size = bytes;
>> +    } else {
>> +        gchar *contents;
>> +        gsize length;
>> +
>> +        if (!g_file_get_contents(image_name, &contents, &length, NULL)) {
>> +            fprintf(stderr, "failed to load \"%s\"\n", image_name);
>> +            exit(1);
>> +        }
>> +        size = length;
>> +        data = (uint8_t *)contents;
>> +    }
>> +
>> +    fw_cfg_add_i32(fw_cfg, size_key, size);
>> +    fw_cfg_add_bytes(fw_cfg, data_key, data, size);
> 
> We have no way to free these buffers later but x86 does the same,
> so never mind.

OK.

> 
>> +}
>> +
>>  void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>  {
>>      CPUState *cs;
>>      int kernel_size;
>> @@ -497,21 +551,48 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>          qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
>>      }
>>
>>      /* Load the kernel.  */
>> -    if (!info->kernel_filename) {
>> +    if (!info->kernel_filename || info->firmware_loaded) {
>>
>>          if (have_dtb(info)) {
>> -            /* If we have a device tree blob, but no kernel to supply it to,
>> -             * copy it to the base of RAM for a bootloader to pick up.
>> +            /* If we have a device tree blob, but no kernel to supply it to (or
>> +             * the kernel is supposed to be loaded by the bootloader), copy the
>> +             * DTB to the base of RAM for the bootloader to pick up.
>>               */
>>              if (load_dtb(info->loader_start, info, 0) < 0) {
>>                  exit(1);
>>              }
>>          }
>>
>> -        /* If no kernel specified, do nothing; we will start from address 0
>> -         * (typically a boot ROM image) in the same way as hardware.
>> +        if (info->kernel_filename) {
>> +            FWCfgState *fw_cfg;
>> +            bool decompress_kernel;
>> +
>> +            fw_cfg = fw_cfg_find();
>> +            decompress_kernel = arm_feature(&cpu->env, ARM_FEATURE_AARCH64);
> 
> If we have a real bootloader in the UEFI firmware, why do we need
> to do decompression for it? We only do this in the builtin bootloader
> because QEMU is acting as the bootloader and has to support the
> feature itself. I would have thought that the UEFI builtin kernel
> booting support already supported decompressing the kernel...

The "UEFI builtin kernel booting support" will qualify as "builtin" only
after my 2500 line patch series is merged in edk2.

http://thread.gmane.org/gmane.comp.bios.tianocore.devel/11651

Zlib decompression is not present in edk2 (it only has a TianoCore
variant of LZMA), and I didn't want to impede (in the community sense)
my edk2 patchset even more (ie. beyond its current size) by importing
libz too.

Thanks
Laszlo

> 
> Otherwise looks OK.
> 
> -- PMM
> 

Re: [Qemu-devel] [PATCH v3 6/7] hw/arm: pass pristine kernel image to guest firmware over fw_cfg

[Peter Maydell]

On 12 December 2014 at 13:52, Laszlo Ersek <lersek@redhat.com> wrote:
> On 12/12/14 14:20, Peter Maydell wrote:
>> If we have a real bootloader in the UEFI firmware, why do we need
>> to do decompression for it? We only do this in the builtin bootloader
>> because QEMU is acting as the bootloader and has to support the
>> feature itself. I would have thought that the UEFI builtin kernel
>> booting support already supported decompressing the kernel...
>
> The "UEFI builtin kernel booting support" will qualify as "builtin" only
> after my 2500 line patch series is merged in edk2.

I had in mind the support for UEFI loading kernels off
hard disks, which presumably is already present.

> Zlib decompression is not present in edk2 (it only has a TianoCore
> variant of LZMA), and I didn't want to impede (in the community sense)
> my edk2 patchset even more (ie. beyond its current size) by importing
> libz too.

Fair enough. This avoids odd inconsistency in compressed kernel
support between the f/w and non f/w setups, anyway.

If you fix the stray space then
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 6/7] hw/arm: pass pristine kernel image to guest firmware over fw_cfg

[Laszlo Ersek]

On 12/12/14 14:57, Peter Maydell wrote:
> On 12 December 2014 at 13:52, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 12/12/14 14:20, Peter Maydell wrote:
>>> If we have a real bootloader in the UEFI firmware, why do we need
>>> to do decompression for it? We only do this in the builtin bootloader
>>> because QEMU is acting as the bootloader and has to support the
>>> feature itself. I would have thought that the UEFI builtin kernel
>>> booting support already supported decompressing the kernel...
>>
>> The "UEFI builtin kernel booting support" will qualify as "builtin" only
>> after my 2500 line patch series is merged in edk2.
> 
> I had in mind the support for UEFI loading kernels off
> hard disks, which presumably is already present.

Ah, I see.

Sure, if you have a kernel image (with the EFI stub) that is an
*immediately executable* EFI binary, then you can just go to the UEFI
shell, navigate to the filesystem / directory that hosts that image, and
run it. (Similarly, PXE boot it etc.)

But in this case the EFI binary is compressed with gzip (for aarch64
kernels); you couldn't even run it from the UEFI shell.

Or else, if you thought of grub2 loading a kernel from the disk with
UEFI protocols -- that works too, but then it's grub that does the
decompression.

(There's a plethora of ways to boot UEFI kernels, and I've recently
asked Matt Fleming if he could write up a comprehensive blog post or
similar about all of them. Hopefully the community will be able to refer
to an authoritative summary sometime next year.)

> 
>> Zlib decompression is not present in edk2 (it only has a TianoCore
>> variant of LZMA), and I didn't want to impede (in the community sense)
>> my edk2 patchset even more (ie. beyond its current size) by importing
>> libz too.
> 
> Fair enough. This avoids odd inconsistency in compressed kernel
> support between the f/w and non f/w setups, anyway.
> 
> If you fix the stray space then
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> thanks
> -- PMM
> 

Awesome!

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v3 6/7] hw/arm: pass pristine kernel image to guest firmware over fw_cfg

[Richard W.M. Jones]

On Fri, Dec 12, 2014 at 03:10:42PM +0100, Laszlo Ersek wrote:
> Sure, if you have a kernel image (with the EFI stub) that is an
> *immediately executable* EFI binary, then you can just go to the UEFI
> shell, navigate to the filesystem / directory that hosts that image, and
> run it. (Similarly, PXE boot it etc.)
> 
> But in this case the EFI binary is compressed with gzip (for aarch64
> kernels); you couldn't even run it from the UEFI shell.

But ...

$ file /boot/vmlinuz-3.18.0-0.rc7.git0.1.rwmj10.fc22.aarch64 
/boot/vmlinuz-3.18.0-0.rc7.git0.1.rwmj10.fc22.aarch64: gzip compressed data, max compression, from Unix
$ uname -a
Linux mustang.home.annexia.org 3.18.0-0.rc7.git0.1.rwmj10.fc22.aarch64 #1 SMP Mon Dec 1 18:40:10 GMT 2014 aarch64 aarch64 aarch64 GNU/Linux

How does it work on the baremetal hardware?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Re: [Qemu-devel] [PATCH v3 6/7] hw/arm: pass pristine kernel image to guest firmware over fw_cfg

[Laszlo Ersek]

On 12/12/14 15:14, Richard W.M. Jones wrote:
> On Fri, Dec 12, 2014 at 03:10:42PM +0100, Laszlo Ersek wrote:
>> Sure, if you have a kernel image (with the EFI stub) that is an
>> *immediately executable* EFI binary, then you can just go to the UEFI
>> shell, navigate to the filesystem / directory that hosts that image, and
>> run it. (Similarly, PXE boot it etc.)
>>
>> But in this case the EFI binary is compressed with gzip (for aarch64
>> kernels); you couldn't even run it from the UEFI shell.
> 
> But ...
> 
> $ file /boot/vmlinuz-3.18.0-0.rc7.git0.1.rwmj10.fc22.aarch64 
> /boot/vmlinuz-3.18.0-0.rc7.git0.1.rwmj10.fc22.aarch64: gzip compressed data, max compression, from Unix
> $ uname -a
> Linux mustang.home.annexia.org 3.18.0-0.rc7.git0.1.rwmj10.fc22.aarch64 #1 SMP Mon Dec 1 18:40:10 GMT 2014 aarch64 aarch64 aarch64 GNU/Linux
> 
> How does it work on the baremetal hardware?

You boot it with grub, which is a UEFI application that loads the image
from disk with UEFI protocols, but then decompresses the image itself,
before running it. It is not the firmware itself that loads it with the
LoadImage() boot service.

Try decompressing the kernel manually with gunzip, and then running
"file" on it.

Thanks
Laszlo

[Qemu-devel] [PATCH v3 7/7] hw/arm/virt: enable passing of EFI-stubbed kernel to guest UEFI firmware

[Laszlo Ersek]

The virt board already ensures mutual exclusion between -bios and -pflash
unit#0; we only need to set "bootinfo.firmware_loaded", introduced in the
previous patch, if either of those options was used to load the guest
firmware.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v3:
    - unchanged

 hw/arm/virt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 274aaae..0b5ad7c 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -632,8 +632,9 @@ static void machvirt_init(MachineState *machine)
     vbi->bootinfo.nb_cpus = smp_cpus;
     vbi->bootinfo.board_id = -1;
     vbi->bootinfo.loader_start = vbi->memmap[VIRT_MEM].base;
     vbi->bootinfo.get_dtb = machvirt_dtb;
+    vbi->bootinfo.firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
     arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo);
 }
 
 static QEMUMachine machvirt_a15_machine = {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 7/7] hw/arm/virt: enable passing of EFI-stubbed kernel to guest UEFI firmware

[Peter Maydell]

On 9 December 2014 at 01:13, Laszlo Ersek <lersek@redhat.com> wrote:
> The virt board already ensures mutual exclusion between -bios and -pflash
> unit#0; we only need to set "bootinfo.firmware_loaded", introduced in the
> previous patch, if either of those options was used to load the guest
> firmware.
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM