[Qemu-devel] [PATCH v5 00/11] fw_cfg, bootorder, and UEFI+'-kernel' on arm/virt

[Qemu-devel] [PATCH v5 00/11] fw_cfg, bootorder, and UEFI+'-kernel' on arm/virt

[Laszlo Ersek]

Addressing review comments received for v4 (with many thanks).

Deeper changes in v5 include (see also the notes per patch):

- The I/O port mapping and the MMIO mapping have been split into
  separate QOM subclasses. This is new territory for me (what is not,
  heh), so bear with me.

  I've done this because a wide data I/O port didn't seem overly useful,
  but marrying it with the overlapped selector/data case was certainly a
  huge mess. This is just so much cleaner. (Assuming I got it right.)

- The memory mapped selector and data registers are explicitly big
  endian now.

- The memory mapped data register performs its own value
  (de)composition, the memory subsystem is left out from that role.

Thanks,
Laszlo  

Laszlo Ersek (10):
  fw_cfg: hard separation between the MMIO and I/O port mappings
  fw_cfg: move boards to fw_cfg_init_io() / fw_cfg_init_mem()
  fw_cfg_mem: max access size and region size are the same for data
    register
  fw_cfg_mem: flip ctl_mem_ops and data_mem_ops to DEVICE_BIG_ENDIAN
  fw_cfg_mem: introduce the "data_width" property
  fw_cfg_mem: expose the "data_width" property with
    fw_cfg_init_mem_wide()
  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

Paolo Bonzini (1):
  exec: allows 8-byte accesses in subpage_ops

 include/hw/arm/arm.h      |   5 +
 include/hw/loader.h       |   9 ++
 include/hw/nvram/fw_cfg.h |   6 +-
 include/qemu/typedefs.h   |   2 +
 exec.c                    |  13 ++-
 hw/arm/boot.c             |  88 +++++++++++++++-
 hw/arm/virt.c             |  22 ++++
 hw/core/loader.c          |  30 ++++--
 hw/i386/pc.c              |   4 +-
 hw/nvram/fw_cfg.c         | 250 +++++++++++++++++++++++++++++++++++-----------
 hw/ppc/mac_newworld.c     |   2 +-
 hw/ppc/mac_oldworld.c     |   2 +-
 hw/sparc/sun4m.c          |   2 +-
 hw/sparc64/sun4u.c        |   2 +-
 14 files changed, 354 insertions(+), 83 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v5 01/11] fw_cfg: hard separation between the MMIO and I/O port mappings

[Laszlo Ersek]

We are going to introduce a wide data register for fw_cfg, but only for
the MMIO mapped device. The wide data register will also require the
tightening of endiannesses.

However we don't want to touch the I/O port mapped fw_cfg device at all.

Currently QEMU provides a single fw_cfg device type that can handle both
I/O port and MMIO mapping. This flexibility is not actually exploited by
any board in the tree, but it renders restricting the above changes to
MMIO very hard.

Therefore, let's derive two classes from TYPE_FW_CFG: TYPE_FW_CFG_IO and
TYPE_FW_CFG_MEM.

TYPE_FW_CFG_IO incorporates the base I/O port and the related combined
MemoryRegion. (NB: all boards in the tree that use the I/O port mapped
flavor opt for the combined mapping; that is, when the data port overlays
the high address byte of the selector port. Therefore we can drop the
capability to map those I/O ports separately.)

TYPE_FW_CFG_MEM incorporates the base addresses for the MMIO selector and
data registers, and their respective MemoryRegions.

The "realize" and "props" class members are specific to each new derived
class, and become unused for the base class. The base class retains the
"reset" member and the "vmsd" member, because the reset functionality and
the set of migrated data are not specific to the mapping.

The new functions fw_cfg_init_io() and fw_cfg_init_mem() expose the
possible mappings in separation. For now fw_cfg_init() is retained as a
compatibility shim that enforces the above assumptions.

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

Notes:
    v5:
    - new in v5 [Laszlo]

 include/hw/nvram/fw_cfg.h |   2 +
 include/qemu/typedefs.h   |   2 +
 hw/nvram/fw_cfg.c         | 183 ++++++++++++++++++++++++++++++++--------------
 3 files changed, 134 insertions(+), 53 deletions(-)

diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 56e1ed7..fcc88ea 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -79,8 +79,10 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
 void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
                          size_t len);
 FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
                         hwaddr crl_addr, hwaddr data_addr);
+FWCfgState *fw_cfg_init_io(uint32_t iobase);
+FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr);
 
 FWCfgState *fw_cfg_find(void);
 
 #endif /* NO_QEMU_PROTOS */
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 57ff47f..f2bbaaf 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -21,8 +21,10 @@ typedef struct DisplayChangeListener DisplayChangeListener;
 typedef struct DisplayState DisplayState;
 typedef struct DisplaySurface DisplaySurface;
 typedef struct DriveInfo DriveInfo;
 typedef struct EventNotifier EventNotifier;
+typedef struct FWCfgIoState FWCfgIoState;
+typedef struct FWCfgMemState FWCfgMemState;
 typedef struct FWCfgState FWCfgState;
 typedef struct HCIInfo HCIInfo;
 typedef struct I2CBus I2CBus;
 typedef struct I2SCodec I2SCodec;
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index c4b78ed..0035fe6 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -31,12 +31,18 @@
 #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)
+
+#define TYPE_FW_CFG     "fw_cfg"
+#define TYPE_FW_CFG_IO  "fw_cfg_io"
+#define TYPE_FW_CFG_MEM "fw_cfg_mem"
+
+#define FW_CFG(obj)     OBJECT_CHECK(FWCfgState,    (obj), TYPE_FW_CFG)
+#define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
+#define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
 
 typedef struct FWCfgEntry {
     uint32_t len;
     uint8_t *data;
@@ -49,17 +55,33 @@ struct FWCfgState {
     /*< private >*/
     SysBusDevice parent_obj;
     /*< public >*/
 
-    MemoryRegion ctl_iomem, data_iomem, comb_iomem;
-    uint32_t ctl_iobase, data_iobase;
     FWCfgEntry entries[2][FW_CFG_MAX_ENTRY];
     FWCfgFiles *files;
     uint16_t cur_entry;
     uint32_t cur_offset;
     Notifier machine_ready;
 };
 
+struct FWCfgIoState {
+    /*< private >*/
+    FWCfgState parent_obj;
+    /*< public >*/
+
+    MemoryRegion comb_iomem;
+    uint32_t iobase;
+};
+
+struct FWCfgMemState {
+    /*< private >*/
+    FWCfgState parent_obj;
+    /*< public >*/
+
+    MemoryRegion ctl_iomem, data_iomem;
+    hwaddr ctl_addr, data_addr;
+};
+
 #define JPG_FILE 0
 #define BMP_FILE 1
 
 static char *read_splashfile(char *filename, gsize *file_sizep,
@@ -559,34 +581,22 @@ 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)
+
+
+static FWCfgState *fw_cfg_init1(DeviceState *dev)
 {
-    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);
-    d = SYS_BUS_DEVICE(dev);
-
     s = FW_CFG(dev);
 
     assert(!object_resolve_path(FW_CFG_PATH, NULL));
 
     object_property_add_child(qdev_get_machine(), FW_CFG_NAME, OBJECT(s), NULL);
 
     qdev_init_nofail(dev);
 
-    if (ctl_addr) {
-        sysbus_mmio_map(d, 0, ctl_addr);
-    }
-    if (data_addr) {
-        sysbus_mmio_map(d, 1, data_addr);
-    }
     fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
     fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, 16);
     fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)(display_type == DT_NOGRAPHIC));
     fw_cfg_add_i16(s, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
@@ -599,46 +609,44 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
 
     return s;
 }
 
-static void fw_cfg_initfn(Object *obj)
+
+FWCfgState *fw_cfg_init_io(uint32_t iobase)
+{
+    DeviceState *dev;
+
+    dev = qdev_create(NULL, TYPE_FW_CFG_IO);
+    qdev_prop_set_uint32(dev, "iobase", iobase);
+
+    return fw_cfg_init1(dev);
+}
+
+FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr)
 {
-    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
-    FWCfgState *s = FW_CFG(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);
-    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);
+    DeviceState *dev;
+
+    dev = qdev_create(NULL, TYPE_FW_CFG_MEM);
+    qdev_prop_set_uint64(dev, "ctl_addr", ctl_addr);
+    qdev_prop_set_uint64(dev, "data_addr", data_addr);
+
+    return fw_cfg_init1(dev);
 }
 
-static void fw_cfg_realize(DeviceState *dev, Error **errp)
+
+FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
+                        hwaddr crl_addr, hwaddr data_addr)
 {
-    FWCfgState *s = FW_CFG(dev);
-    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
-
-    if (s->ctl_iobase + 1 == s->data_iobase) {
-        sysbus_add_io(sbd, s->ctl_iobase, &s->comb_iomem);
-    } else {
-        if (s->ctl_iobase) {
-            sysbus_add_io(sbd, s->ctl_iobase, &s->ctl_iomem);
-        }
-        if (s->data_iobase) {
-            sysbus_add_io(sbd, s->data_iobase, &s->data_iomem);
-        }
+    if (ctl_port + 1 == data_port && crl_addr == 0 && data_addr == 0) {
+        return fw_cfg_init_io(ctl_port);
+    }
+    if (ctl_port == 0 && data_port == 0 && crl_addr != 0 && data_addr != 0) {
+        return fw_cfg_init_mem(crl_addr, data_addr);
     }
+    assert(false);
+    return NULL;
 }
 
-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_END_OF_LIST(),
-};
 
 FWCfgState *fw_cfg_find(void)
 {
     return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
@@ -647,24 +655,93 @@ FWCfgState *fw_cfg_find(void)
 static void fw_cfg_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    dc->realize = fw_cfg_realize;
     dc->reset = fw_cfg_reset;
     dc->vmsd = &vmstate_fw_cfg;
-    dc->props = fw_cfg_properties;
 }
 
 static const TypeInfo fw_cfg_info = {
     .name          = TYPE_FW_CFG,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(FWCfgState),
-    .instance_init = fw_cfg_initfn,
     .class_init    = fw_cfg_class_init,
 };
 
+
+static Property fw_cfg_io_properties[] = {
+    DEFINE_PROP_UINT32("iobase", FWCfgIoState, iobase, -1),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
+{
+    FWCfgIoState *s = FW_CFG_IO(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+
+    memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
+                          FW_CFG(s), "fwcfg", FW_CFG_SIZE);
+    sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
+}
+
+static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = fw_cfg_io_realize;
+    dc->props = fw_cfg_io_properties;
+}
+
+static const TypeInfo fw_cfg_io_info = {
+    .name          = TYPE_FW_CFG_IO,
+    .parent        = TYPE_FW_CFG,
+    .instance_size = sizeof(FWCfgIoState),
+    .class_init    = fw_cfg_io_class_init,
+};
+
+
+static Property fw_cfg_mem_properties[] = {
+    DEFINE_PROP_UINT64("ctl_addr", FWCfgMemState, ctl_addr, -1),
+    DEFINE_PROP_UINT64("data_addr", FWCfgMemState, data_addr, -1),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
+{
+    FWCfgMemState *s = FW_CFG_MEM(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+
+    memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops,
+                          FW_CFG(s), "fwcfg.ctl", FW_CFG_SIZE);
+    sysbus_init_mmio(sbd, &s->ctl_iomem);
+    sysbus_mmio_map(sbd, 0, s->ctl_addr);
+
+    memory_region_init_io(&s->data_iomem, OBJECT(s), &fw_cfg_data_mem_ops,
+                          FW_CFG(s), "fwcfg.data", FW_CFG_DATA_SIZE);
+    sysbus_init_mmio(sbd, &s->data_iomem);
+    sysbus_mmio_map(sbd, 1, s->data_addr);
+}
+
+static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = fw_cfg_mem_realize;
+    dc->props = fw_cfg_mem_properties;
+}
+
+static const TypeInfo fw_cfg_mem_info = {
+    .name          = TYPE_FW_CFG_MEM,
+    .parent        = TYPE_FW_CFG,
+    .instance_size = sizeof(FWCfgMemState),
+    .class_init    = fw_cfg_mem_class_init,
+};
+
+
 static void fw_cfg_register_types(void)
 {
     type_register_static(&fw_cfg_info);
+    type_register_static(&fw_cfg_io_info);
+    type_register_static(&fw_cfg_mem_info);
 }
 
 type_init(fw_cfg_register_types)
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 02/11] fw_cfg: move boards to fw_cfg_init_io() / fw_cfg_init_mem()

[Laszlo Ersek]

This allows us to drop the fw_cfg_init() shim and to enforce the possible
mappings at compile time.

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

Notes:
    v5:
    - new in v5 [Laszlo]

 include/hw/nvram/fw_cfg.h |  2 --
 hw/i386/pc.c              |  4 ++--
 hw/nvram/fw_cfg.c         | 14 --------------
 hw/ppc/mac_newworld.c     |  2 +-
 hw/ppc/mac_oldworld.c     |  2 +-
 hw/sparc/sun4m.c          |  2 +-
 hw/sparc64/sun4u.c        |  2 +-
 7 files changed, 6 insertions(+), 22 deletions(-)

diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index fcc88ea..a99586e 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -77,10 +77,8 @@ 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(uint32_t ctl_port, uint32_t data_port,
-                        hwaddr crl_addr, hwaddr data_addr);
 FWCfgState *fw_cfg_init_io(uint32_t iobase);
 FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr);
 
 FWCfgState *fw_cfg_find(void);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c0e55a6..21e12ea 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -646,9 +646,9 @@ static FWCfgState *bochs_bios_init(void)
     uint64_t *numa_fw_cfg;
     int i, j;
     unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
 
-    fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0);
+    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
     /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
      *
      * SeaBIOS needs FW_CFG_MAX_CPUS for CPU hotplug, but the CPU hotplug
      * QEMU<->SeaBIOS interface is not based on the "CPU index", but on the APIC
@@ -1167,9 +1167,9 @@ FWCfgState *xen_load_linux(const char *kernel_filename,
     FWCfgState *fw_cfg;
 
     assert(kernel_filename != NULL);
 
-    fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0);
+    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
     rom_set_fw(fw_cfg);
 
     load_linux(fw_cfg, kernel_filename, initrd_filename,
                kernel_cmdline, below_4g_mem_size);
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 0035fe6..880311c 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -632,22 +632,8 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr)
     return fw_cfg_init1(dev);
 }
 
 
-FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
-                        hwaddr crl_addr, hwaddr data_addr)
-{
-    if (ctl_port + 1 == data_port && crl_addr == 0 && data_addr == 0) {
-        return fw_cfg_init_io(ctl_port);
-    }
-    if (ctl_port == 0 && data_port == 0 && crl_addr != 0 && data_addr != 0) {
-        return fw_cfg_init_mem(crl_addr, data_addr);
-    }
-    assert(false);
-    return NULL;
-}
-
-
 FWCfgState *fw_cfg_find(void)
 {
     return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
 }
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 89aee71..5dac389 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -453,9 +453,9 @@ static void ppc_core99_init(MachineState *machine)
     nvr = MACIO_NVRAM(dev);
     pmac_format_nvram_partition(nvr, 0x2000);
     /* No PCI init: the BIOS will do it */
 
-    fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg = fw_cfg_init_mem(CFG_ADDR, CFG_ADDR + 2);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, machine_arch);
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 32c21a4..41fefb7 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -313,9 +313,9 @@ static void ppc_heathrow_init(MachineState *machine)
         graphic_depth = 15;
 
     /* No PCI init: the BIOS will do it */
 
-    fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg = fw_cfg_init_mem(CFG_ADDR, CFG_ADDR + 2);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, ARCH_HEATHROW);
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 8273199..a12d3c4 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -1083,9 +1083,9 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
     if (hwdef->ecc_base)
         ecc_init(hwdef->ecc_base, slavio_irq[28],
                  hwdef->ecc_version);
 
-    fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg = fw_cfg_init_mem(CFG_ADDR, CFG_ADDR + 2);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index f42112c..49fb678 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -891,9 +891,9 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
                            0,
                            graphic_width, graphic_height, graphic_depth,
                            (uint8_t *)&nd_table[0].macaddr);
 
-    fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0);
+    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 03/11] fw_cfg_mem: max access size and region size are the same for data register

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

This patch doesn't change behavior.

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

Notes:
    v5:
    - Remove the "fw_cfg_data_mem_ops.impl.max_access_size = 1"
      initialization, because we won't ask the memory subsystem to split
      accesses for us. [Paolo, Peter]
    
    v4:
    - unchanged
    
    v3:
    - new in v3 [Drew Jones]

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

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 880311c..5b4d9d4 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 FW_CFG_NAME "fw_cfg"
 #define FW_CFG_PATH "/machine/" FW_CFG_NAME
 
 #define TYPE_FW_CFG     "fw_cfg"
@@ -701,9 +700,10 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(sbd, &s->ctl_iomem);
     sysbus_mmio_map(sbd, 0, s->ctl_addr);
 
     memory_region_init_io(&s->data_iomem, OBJECT(s), &fw_cfg_data_mem_ops,
-                          FW_CFG(s), "fwcfg.data", FW_CFG_DATA_SIZE);
+                          FW_CFG(s), "fwcfg.data",
+                          fw_cfg_data_mem_ops.valid.max_access_size);
     sysbus_init_mmio(sbd, &s->data_iomem);
     sysbus_mmio_map(sbd, 1, s->data_addr);
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 04/11] fw_cfg_mem: flip ctl_mem_ops and data_mem_ops to DEVICE_BIG_ENDIAN

[Laszlo Ersek]

The standalone selector port (fw_cfg_ctl_mem_ops) is only used by big
endian guests to date (*), hence this change doesn't regress them. Paolo
and Alex have suggested / requested an explicit DEVICE_BIG_ENDIAN setting
here, for clarity.

(*) git grep -l fw_cfg_init_mem

    hw/nvram/fw_cfg.c
    hw/ppc/mac_newworld.c
    hw/ppc/mac_oldworld.c
    hw/sparc/sun4m.c
    include/hw/nvram/fw_cfg.h

The standalone data port (fw_cfg_data_mem_ops) has max_access_size 1 (for
now), hence changing its endianness doesn't change behavior for existing
guest code.

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

Notes:
    v5:
    - new in v5 [Paolo, Alex]

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

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 5b4d9d4..5ba49dd 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -332,16 +332,16 @@ static bool fw_cfg_comb_valid(void *opaque, hwaddr addr,
 }
 
 static const MemoryRegionOps fw_cfg_ctl_mem_ops = {
     .write = fw_cfg_ctl_mem_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
+    .endianness = DEVICE_BIG_ENDIAN,
     .valid.accepts = fw_cfg_ctl_mem_valid,
 };
 
 static const MemoryRegionOps fw_cfg_data_mem_ops = {
     .read = fw_cfg_data_mem_read,
     .write = fw_cfg_data_mem_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
+    .endianness = DEVICE_BIG_ENDIAN,
     .valid = {
         .min_access_size = 1,
         .max_access_size = 1,
     },
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 05/11] exec: allows 8-byte accesses in subpage_ops

[Laszlo Ersek]

From: Paolo Bonzini <pbonzini@redhat.com>

Otherwise fw_cfg accesses are split into 4-byte ones before they reach the
fw_cfg ops / handlers.

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

Notes:
    v5:
    - new in v5 [reported by Laszlo, fixed by Paolo]

 exec.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/exec.c b/exec.c
index 963481a..619e046 100644
--- a/exec.c
+++ b/exec.c
@@ -1767,9 +1767,9 @@ static const MemoryRegionOps watch_mem_ops = {
 static uint64_t subpage_read(void *opaque, hwaddr addr,
                              unsigned len)
 {
     subpage_t *subpage = opaque;
-    uint8_t buf[4];
+    uint8_t buf[8];
 
 #if defined(DEBUG_SUBPAGE)
     printf("%s: subpage %p len %u addr " TARGET_FMT_plx "\n", __func__,
            subpage, len, addr);
@@ -1781,8 +1781,10 @@ static uint64_t subpage_read(void *opaque, hwaddr addr,
     case 2:
         return lduw_p(buf);
     case 4:
         return ldl_p(buf);
+    case 8:
+        return ldq_p(buf);
     default:
         abort();
     }
 }
@@ -1790,9 +1792,9 @@ static uint64_t subpage_read(void *opaque, hwaddr addr,
 static void subpage_write(void *opaque, hwaddr addr,
                           uint64_t value, unsigned len)
 {
     subpage_t *subpage = opaque;
-    uint8_t buf[4];
+    uint8_t buf[8];
 
 #if defined(DEBUG_SUBPAGE)
     printf("%s: subpage %p len %u addr " TARGET_FMT_plx
            " value %"PRIx64"\n",
@@ -1807,8 +1809,11 @@ static void subpage_write(void *opaque, hwaddr addr,
         break;
     case 4:
         stl_p(buf, value);
         break;
+    case 8:
+        stq_p(buf, value);
+        break;
     default:
         abort();
     }
     address_space_write(subpage->as, addr + subpage->base, buf, len);
@@ -1829,8 +1834,12 @@ static bool subpage_accepts(void *opaque, hwaddr addr,
 
 static const MemoryRegionOps subpage_ops = {
     .read = subpage_read,
     .write = subpage_write,
+    .impl.min_access_size = 1,
+    .impl.max_access_size = 8,
+    .valid.min_access_size = 1,
+    .valid.max_access_size = 8,
     .valid.accepts = subpage_accepts,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 06/11] fw_cfg_mem: introduce the "data_width" property

[Laszlo Ersek]

The "data_width" property is capable of changing the maximum valid access
size to the MMIO data register, and resizes the memory region similarly,
at device realization time.

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_mem 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:
    v5:
    - the wide data register is restricted to the MMIO mapping now; no
      interference with the I/O port mapping [Laszlo]
    - the wide data ops aren't allocated separately with g_memdup() any
      longer [Alex, Peter]
    - the wide data register handles splitting / combining itself [Paolo,
      Peter]
    
    v4:
    - reject I/O port combining if data register is wider than 1 byte
      [Peter]
    
    v3:
    - new in v3 [Drew Jones]

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

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 5ba49dd..80f846f 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -77,8 +77,10 @@ struct FWCfgMemState {
     /*< public >*/
 
     MemoryRegion ctl_iomem, data_iomem;
     hwaddr ctl_addr, data_addr;
+    uint32_t data_width;
+    MemoryRegionOps wide_data_ops;
 };
 
 #define JPG_FILE 0
 #define BMP_FILE 1
@@ -284,15 +286,60 @@ static uint8_t fw_cfg_read(FWCfgState *s)
 
 static uint64_t fw_cfg_data_mem_read(void *opaque, hwaddr addr,
                                      unsigned size)
 {
-    return fw_cfg_read(opaque);
+    FWCfgState *s = opaque;
+    uint8_t buf[8];
+    unsigned i;
+
+    for (i = 0; i < size; ++i) {
+        buf[i] = fw_cfg_read(s);
+    }
+    switch (size) {
+    case 1:
+        return buf[0];
+    case 2:
+        return lduw_he_p(buf);
+    case 4:
+        return (uint32_t)ldl_he_p(buf);
+    case 8:
+        return ldq_he_p(buf);
+    }
+    abort();
 }
 
 static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
                                   uint64_t value, unsigned size)
 {
-    fw_cfg_write(opaque, (uint8_t)value);
+    FWCfgState *s = opaque;
+    uint8_t buf[8];
+    unsigned i;
+
+    switch (size) {
+    case 1:
+        buf[0] = value;
+        break;
+    case 2:
+        stw_he_p(buf, value);
+        break;
+    case 4:
+        stl_he_p(buf, value);
+        break;
+    case 8:
+        stq_he_p(buf, value);
+        break;
+    default:
+        abort();
+    }
+    for (i = 0; i < size; ++i) {
+        fw_cfg_write(s, buf[i]);
+    }
+}
+
+static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
+                                  unsigned size, bool is_write)
+{
+    return addr == 0;
 }
 
 static void fw_cfg_ctl_mem_write(void *opaque, hwaddr addr,
                                  uint64_t value, unsigned size)
@@ -343,8 +390,9 @@ static const MemoryRegionOps fw_cfg_data_mem_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
     .valid = {
         .min_access_size = 1,
         .max_access_size = 1,
+        .accepts = fw_cfg_data_mem_valid,
     },
 };
 
 static const MemoryRegionOps fw_cfg_comb_mem_ops = {
@@ -626,8 +674,10 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr)
 
     dev = qdev_create(NULL, TYPE_FW_CFG_MEM);
     qdev_prop_set_uint64(dev, "ctl_addr", ctl_addr);
     qdev_prop_set_uint64(dev, "data_addr", data_addr);
+    qdev_prop_set_uint32(dev, "data_width",
+                         fw_cfg_data_mem_ops.valid.max_access_size);
 
     return fw_cfg_init1(dev);
 }
 
@@ -686,24 +736,37 @@ static const TypeInfo fw_cfg_io_info = {
 
 static Property fw_cfg_mem_properties[] = {
     DEFINE_PROP_UINT64("ctl_addr", FWCfgMemState, ctl_addr, -1),
     DEFINE_PROP_UINT64("data_addr", FWCfgMemState, data_addr, -1),
+    DEFINE_PROP_UINT32("data_width", FWCfgMemState, data_width, -1),
     DEFINE_PROP_END_OF_LIST(),
 };
 
 static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
 {
     FWCfgMemState *s = FW_CFG_MEM(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    const MemoryRegionOps *data_ops = &fw_cfg_data_mem_ops;
 
     memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops,
                           FW_CFG(s), "fwcfg.ctl", FW_CFG_SIZE);
     sysbus_init_mmio(sbd, &s->ctl_iomem);
     sysbus_mmio_map(sbd, 0, s->ctl_addr);
 
-    memory_region_init_io(&s->data_iomem, OBJECT(s), &fw_cfg_data_mem_ops,
-                          FW_CFG(s), "fwcfg.data",
-                          fw_cfg_data_mem_ops.valid.max_access_size);
+    if (s->data_width > data_ops->valid.max_access_size) {
+        /* memberwise copy because the "old_mmio" member is const */
+        s->wide_data_ops.read       = data_ops->read;
+        s->wide_data_ops.write      = data_ops->write;
+        s->wide_data_ops.endianness = data_ops->endianness;
+        s->wide_data_ops.valid      = data_ops->valid;
+        s->wide_data_ops.impl       = data_ops->impl;
+
+        s->wide_data_ops.valid.max_access_size = s->data_width;
+        s->wide_data_ops.impl.max_access_size  = s->data_width;
+        data_ops = &s->wide_data_ops;
+    }
+    memory_region_init_io(&s->data_iomem, OBJECT(s), data_ops, FW_CFG(s),
+                          "fwcfg.data", data_ops->valid.max_access_size);
     sysbus_init_mmio(sbd, &s->data_iomem);
     sysbus_mmio_map(sbd, 1, s->data_addr);
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 07/11] fw_cfg_mem: expose the "data_width" property with fw_cfg_init_mem_wide()

[Laszlo Ersek]

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

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

Notes:
    v5:
    - just rebased
    
    v4:
    - unchanged
    
    v3:
    - new in v3 [Drew Jones]

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

diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index a99586e..6d8a8ac 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -79,8 +79,10 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
 void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
                          size_t len);
 FWCfgState *fw_cfg_init_io(uint32_t iobase);
 FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr);
+FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, hwaddr data_addr,
+                                 uint32_t data_width);
 
 FWCfgState *fw_cfg_find(void);
 
 #endif /* NO_QEMU_PROTOS */
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 80f846f..33ffd0f 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -667,21 +667,27 @@ FWCfgState *fw_cfg_init_io(uint32_t iobase)
 
     return fw_cfg_init1(dev);
 }
 
-FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr)
+FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, hwaddr data_addr,
+                                 uint32_t data_width)
 {
     DeviceState *dev;
 
     dev = qdev_create(NULL, TYPE_FW_CFG_MEM);
     qdev_prop_set_uint64(dev, "ctl_addr", ctl_addr);
     qdev_prop_set_uint64(dev, "data_addr", data_addr);
-    qdev_prop_set_uint32(dev, "data_width",
-                         fw_cfg_data_mem_ops.valid.max_access_size);
+    qdev_prop_set_uint32(dev, "data_width", data_width);
 
     return fw_cfg_init1(dev);
 }
 
+FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr)
+{
+    return fw_cfg_init_mem_wide(ctl_addr, data_addr,
+                                fw_cfg_data_mem_ops.valid.max_access_size);
+}
+
 
 FWCfgState *fw_cfg_find(void)
 {
     return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 08/11] 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_mem_wide(), 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---

Notes:
    v5:
    - rebased to call new function fw_cfg_init_mem_wide() -- minimal change,
      keeping Peter's R-b
    
    v4:
    - unchanged
    
    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..8af4aa0 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_mem_wide(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

[Qemu-devel] [PATCH v5 09/11] 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---

Notes:
    v5:
    - unchanged; keeping Peter's R-b
    
    v4:
    - drop space "between function name and open parenthesis '('" [Peter]
    
    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..f2b34da 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

[Qemu-devel] [PATCH v5 10/11] 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:
    v5:
    - load_image_to_fw_cfg(): fall back to plaintext loading if gunzipping
      fails [Alex]
    - not a trivial change, dropping Peter's R-b
    
    v4:
    - drop space "between function name and open parenthesis '('" [Peter]
    
    v3:
    - unchanged

 include/hw/arm/arm.h |  5 +++
 hw/arm/boot.c        | 88 +++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 88 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 e6a3c5b..17bdaee 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -477,8 +477,57 @@ 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.
+ * @try_decompress: Whether the image should be decompressed (gunzipped) before
+ *                  adding it to fw_cfg. If decompression fails, the image is
+ *                  loaded as-is.
+ *
+ * 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 try_decompress)
+{
+    size_t size = -1;
+    uint8_t *data;
+
+    if (image_name == NULL) {
+        return;
+    }
+
+    if (try_decompress) {
+        size = load_image_gzipped_buffer(image_name,
+                                         LOAD_IMAGE_MAX_GUNZIP_BYTES, &data);
+    }
+
+    if (size == (size_t)-1) {
+        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;
@@ -499,21 +548,50 @@ 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 try_decompressing_kernel;
+
+            fw_cfg = fw_cfg_find();
+            try_decompressing_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,
+                                 try_decompressing_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

[Qemu-devel] [PATCH v5 11/11] 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---

Notes:
    v5:
    - unchanged, keeping Peter's R-b
    
    v4:
    - unchanged
    
    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 8af4aa0..29fbdc1 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 v5 01/11] fw_cfg: hard separation between the MMIO and I/O port mappings

[Paolo Bonzini]

On 17/12/2014 22:10, Laszlo Ersek wrote:
> +static Property fw_cfg_mem_properties[] = {
> +    DEFINE_PROP_UINT64("ctl_addr", FWCfgMemState, ctl_addr, -1),
> +    DEFINE_PROP_UINT64("data_addr", FWCfgMemState, data_addr, -1),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
> +{
> +    FWCfgMemState *s = FW_CFG_MEM(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +
> +    memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops,
> +                          FW_CFG(s), "fwcfg.ctl", FW_CFG_SIZE);
> +    sysbus_init_mmio(sbd, &s->ctl_iomem);
> +    sysbus_mmio_map(sbd, 0, s->ctl_addr);
> +
> +    memory_region_init_io(&s->data_iomem, OBJECT(s), &fw_cfg_data_mem_ops,
> +                          FW_CFG(s), "fwcfg.data", FW_CFG_DATA_SIZE);
> +    sysbus_init_mmio(sbd, &s->data_iomem);
> +    sysbus_mmio_map(sbd, 1, s->data_addr);
> +}

Strictly speaking sysbus_mmio_map should be called by the caller, as in
the old fw_cfg_init---which lets you drop the properties too.

Doesn't prevent merging this series.

Paolo

Re: [Qemu-devel] [PATCH v5 02/11] fw_cfg: move boards to fw_cfg_init_io() / fw_cfg_init_mem()

[Paolo Bonzini]

On 17/12/2014 22:10, Laszlo Ersek wrote:
> This allows us to drop the fw_cfg_init() shim and to enforce the possible
> mappings at compile time.
> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     v5:
>     - new in v5 [Laszlo]
> 
>  include/hw/nvram/fw_cfg.h |  2 --
>  hw/i386/pc.c              |  4 ++--
>  hw/nvram/fw_cfg.c         | 14 --------------
>  hw/ppc/mac_newworld.c     |  2 +-
>  hw/ppc/mac_oldworld.c     |  2 +-
>  hw/sparc/sun4m.c          |  2 +-
>  hw/sparc64/sun4u.c        |  2 +-
>  7 files changed, 6 insertions(+), 22 deletions(-)
> 
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index fcc88ea..a99586e 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -77,10 +77,8 @@ 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(uint32_t ctl_port, uint32_t data_port,
> -                        hwaddr crl_addr, hwaddr data_addr);
>  FWCfgState *fw_cfg_init_io(uint32_t iobase);
>  FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr);
>  
>  FWCfgState *fw_cfg_find(void);
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index c0e55a6..21e12ea 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -646,9 +646,9 @@ static FWCfgState *bochs_bios_init(void)
>      uint64_t *numa_fw_cfg;
>      int i, j;
>      unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
>  
> -    fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0);
> +    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
>      /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
>       *
>       * SeaBIOS needs FW_CFG_MAX_CPUS for CPU hotplug, but the CPU hotplug
>       * QEMU<->SeaBIOS interface is not based on the "CPU index", but on the APIC
> @@ -1167,9 +1167,9 @@ FWCfgState *xen_load_linux(const char *kernel_filename,
>      FWCfgState *fw_cfg;
>  
>      assert(kernel_filename != NULL);
>  
> -    fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0);
> +    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
>      rom_set_fw(fw_cfg);
>  
>      load_linux(fw_cfg, kernel_filename, initrd_filename,
>                 kernel_cmdline, below_4g_mem_size);
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 0035fe6..880311c 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -632,22 +632,8 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr)
>      return fw_cfg_init1(dev);
>  }
>  
>  
> -FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
> -                        hwaddr crl_addr, hwaddr data_addr)
> -{
> -    if (ctl_port + 1 == data_port && crl_addr == 0 && data_addr == 0) {
> -        return fw_cfg_init_io(ctl_port);
> -    }
> -    if (ctl_port == 0 && data_port == 0 && crl_addr != 0 && data_addr != 0) {
> -        return fw_cfg_init_mem(crl_addr, data_addr);
> -    }
> -    assert(false);
> -    return NULL;
> -}
> -
> -
>  FWCfgState *fw_cfg_find(void)
>  {
>      return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
>  }
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 89aee71..5dac389 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -453,9 +453,9 @@ static void ppc_core99_init(MachineState *machine)
>      nvr = MACIO_NVRAM(dev);
>      pmac_format_nvram_partition(nvr, 0x2000);
>      /* No PCI init: the BIOS will do it */
>  
> -    fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
> +    fw_cfg = fw_cfg_init_mem(CFG_ADDR, CFG_ADDR + 2);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
>      fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
>      fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, machine_arch);
> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> index 32c21a4..41fefb7 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -313,9 +313,9 @@ static void ppc_heathrow_init(MachineState *machine)
>          graphic_depth = 15;
>  
>      /* No PCI init: the BIOS will do it */
>  
> -    fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
> +    fw_cfg = fw_cfg_init_mem(CFG_ADDR, CFG_ADDR + 2);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
>      fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
>      fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, ARCH_HEATHROW);
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index 8273199..a12d3c4 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -1083,9 +1083,9 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>      if (hwdef->ecc_base)
>          ecc_init(hwdef->ecc_base, slavio_irq[28],
>                   hwdef->ecc_version);
>  
> -    fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
> +    fw_cfg = fw_cfg_init_mem(CFG_ADDR, CFG_ADDR + 2);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
>      fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
>      fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index f42112c..49fb678 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -891,9 +891,9 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>                             0,
>                             graphic_width, graphic_height, graphic_depth,
>                             (uint8_t *)&nd_table[0].macaddr);
>  
> -    fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0);
> +    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
>      fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
>      fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
>      fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
> 

YAY!

Paolo

Re: [Qemu-devel] [PATCH v5 00/11] fw_cfg, bootorder, and UEFI+'-kernel' on arm/virt

[Paolo Bonzini]

On 17/12/2014 22:10, Laszlo Ersek wrote:
> Addressing review comments received for v4 (with many thanks).
> 
> Deeper changes in v5 include (see also the notes per patch):
> 
> - The I/O port mapping and the MMIO mapping have been split into
>   separate QOM subclasses. This is new territory for me (what is not,
>   heh), so bear with me.
> 
>   I've done this because a wide data I/O port didn't seem overly useful,
>   but marrying it with the overlapped selector/data case was certainly a
>   huge mess. This is just so much cleaner. (Assuming I got it right.)

Pretty much.

> - The memory mapped selector and data registers are explicitly big
>   endian now.
> 
> - The memory mapped data register performs its own value
>   (de)composition, the memory subsystem is left out from that role.

Patches 1-7

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH v5 01/11] fw_cfg: hard separation between the MMIO and I/O port mappings

[Peter Maydell]

On 18 December 2014 at 11:18, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 17/12/2014 22:10, Laszlo Ersek wrote:
>> +static Property fw_cfg_mem_properties[] = {
>> +    DEFINE_PROP_UINT64("ctl_addr", FWCfgMemState, ctl_addr, -1),
>> +    DEFINE_PROP_UINT64("data_addr", FWCfgMemState, data_addr, -1),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
>> +{
>> +    FWCfgMemState *s = FW_CFG_MEM(dev);
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> +
>> +    memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops,
>> +                          FW_CFG(s), "fwcfg.ctl", FW_CFG_SIZE);
>> +    sysbus_init_mmio(sbd, &s->ctl_iomem);
>> +    sysbus_mmio_map(sbd, 0, s->ctl_addr);
>> +
>> +    memory_region_init_io(&s->data_iomem, OBJECT(s), &fw_cfg_data_mem_ops,
>> +                          FW_CFG(s), "fwcfg.data", FW_CFG_DATA_SIZE);
>> +    sysbus_init_mmio(sbd, &s->data_iomem);
>> +    sysbus_mmio_map(sbd, 1, s->data_addr);
>> +}
>
> Strictly speaking sysbus_mmio_map should be called by the caller, as in
> the old fw_cfg_init---which lets you drop the properties too.

I'd really rather we got this correct -- mmio_map in a realize
function is wrong and if we merge this I'll have to immediately
write a bug fix patch to move it out again...

-- PMM

Re: [Qemu-devel] [PATCH v5 01/11] fw_cfg: hard separation between the MMIO and I/O port mappings

[Paolo Bonzini]

On 18/12/2014 13:58, Peter Maydell wrote:
> On 18 December 2014 at 11:18, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 17/12/2014 22:10, Laszlo Ersek wrote:
>>> +static Property fw_cfg_mem_properties[] = {
>>> +    DEFINE_PROP_UINT64("ctl_addr", FWCfgMemState, ctl_addr, -1),
>>> +    DEFINE_PROP_UINT64("data_addr", FWCfgMemState, data_addr, -1),
>>> +    DEFINE_PROP_END_OF_LIST(),
>>> +};
>>> +
>>> +static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    FWCfgMemState *s = FW_CFG_MEM(dev);
>>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>> +
>>> +    memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops,
>>> +                          FW_CFG(s), "fwcfg.ctl", FW_CFG_SIZE);
>>> +    sysbus_init_mmio(sbd, &s->ctl_iomem);
>>> +    sysbus_mmio_map(sbd, 0, s->ctl_addr);
>>> +
>>> +    memory_region_init_io(&s->data_iomem, OBJECT(s), &fw_cfg_data_mem_ops,
>>> +                          FW_CFG(s), "fwcfg.data", FW_CFG_DATA_SIZE);
>>> +    sysbus_init_mmio(sbd, &s->data_iomem);
>>> +    sysbus_mmio_map(sbd, 1, s->data_addr);
>>> +}
>>
>> Strictly speaking sysbus_mmio_map should be called by the caller, as in
>> the old fw_cfg_init---which lets you drop the properties too.
> 
> I'd really rather we got this correct -- mmio_map in a realize
> function is wrong and if we merge this I'll have to immediately
> write a bug fix patch to move it out again...

Laszlo is on vacation, I'll prepare a fixup patch tomorrow.

Paolo

Re: [Qemu-devel] [PATCH v5 00/11] fw_cfg, bootorder, and UEFI+'-kernel' on arm/virt

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 575 bytes --]

  Hi,

Attached is a patch drafting a dma interface for fw_cfg, lousily based
on the ideas I've outlined a few days ago on this list.  Applies on top
of this patch series.

Host side only, unfinished, untested.  Mainly sending this out as
notification that I've looked into this, to avoid duplicating work.
I'll go disappear into xmas vacation in a few hours and will not
continue with this until next year.

My plan is to coordinate in January with Laszlo how to go forward.  If
someone feels bored during the holidays feel free to grab it and run
with it.

cheers,
  Gerd


[-- Attachment #2: Type: text/x-patch, Size: 9050 bytes --]

>From 679b076610ada4229a735caf01dd1390ef336788 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Thu, 18 Dec 2014 17:03:52 +0100
Subject: [PATCH] [wip] fw_cfg dma interface

First draft of a fw_cfg dma interface.  Designed as add-on to the
extisting fw_cfg interface, i.e. there is no select register.  There
are four 32bit registers:  Target address (low and high bits), transfer
length, control register.

Protocol:

Probing for dma support being available:  Write target address, read it
back, verify you got back the value you wrote.

Kick a transfer:  Write select, target address and transfer length
registers (order doesn't matter).  Then flip read or write bit in the
control register to '1'.  Also make sure the error bit is cleared.

Check result:  Read control register:
   error bit set     ->  something went wrong.
   all bits cleared  ->  transfer finished successfully.
   otherwise         ->  transfer still in progress (doesn't happen
                         today due to implementation not being async,
                         but may in the future).

Target address goes up and transfer length goes down as the transfer
happens, so after a successfull transfer the length register is zero
and the address register points right after the memory block written.

If a partial transfer happened before an error occured the address and
length registers indicate how much data has been transfered
successfully.

Todo list:

 * figure which address space to use.
 * wind up in boards.
 * write guest support.
 * test the whole thing.

Possible improvements (can be done incremental):

 * Better error reporting.  Right now we throw errors on invalid
   addresses only.  We could also throw errors on invalid reads/writes
   instead of using fw_cfg_read (return zeros on error) and fw_cfg_write
   (silently discard data on error) behavior.
 * Use memcpy instead of calling fw_cfg_read in a loop.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/nvram/fw_cfg.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 165 insertions(+), 2 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 33ffd0f..75ce403 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -23,6 +23,7 @@
  */
 #include "hw/hw.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/dma.h"
 #include "hw/isa/isa.h"
 #include "hw/nvram/fw_cfg.h"
 #include "hw/sysbus.h"
@@ -42,6 +43,19 @@
 #define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
 #define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
 
+/* fw_cfg dma registers */
+#define FW_CFG_DMA_ADDR_LO        0
+#define FW_CFG_DMA_ADDR_HI        4
+#define FW_CFG_DMA_LENGTH         8
+#define FW_CFG_DMA_CONTROL       12
+#define FW_CFG_DMA_SIZE          16
+
+/* FW_CFG_DMA_CONTROL bits */
+#define FW_CFG_DMA_CTL_READ    0x01
+#define FW_CFG_DMA_CTL_WRITE   0x02
+#define FW_CFG_DMA_CTL_ERROR   0x04
+#define FW_CFG_DMA_CTL_MASK    0x07
+
 typedef struct FWCfgEntry {
     uint32_t len;
     uint8_t *data;
@@ -60,6 +74,12 @@ struct FWCfgState {
     uint16_t cur_entry;
     uint32_t cur_offset;
     Notifier machine_ready;
+
+    bool       dma_enabled;
+    AddressSpace *dma_as;
+    dma_addr_t dma_addr;
+    uint32_t   dma_len;
+    uint32_t   dma_ctl;
 };
 
 struct FWCfgIoState {
@@ -76,8 +96,8 @@ struct FWCfgMemState {
     FWCfgState parent_obj;
     /*< public >*/
 
-    MemoryRegion ctl_iomem, data_iomem;
-    hwaddr ctl_addr, data_addr;
+    MemoryRegion ctl_iomem, data_iomem, dma_iomem;
+    hwaddr ctl_addr, data_addr, dma_addr;
     uint32_t data_width;
     MemoryRegionOps wide_data_ops;
 };
@@ -335,6 +355,102 @@ static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
     }
 }
 
+static void fw_cfg_dma_transfer(FWCfgState *s)
+{
+    DMADirection direction;
+    dma_addr_t len;
+    uint8_t *ptr;
+    uint32_t i;
+
+    if ((s->dma_ctl & FW_CFG_DMA_CTL_READ) &&
+        (s->dma_ctl & FW_CFG_DMA_CTL_WRITE)) {
+        s->dma_ctl |= FW_CFG_DMA_CTL_ERROR;
+        return;
+    }
+
+    if (s->dma_ctl & FW_CFG_DMA_CTL_ERROR) {
+        return;
+    } else if (s->dma_ctl & FW_CFG_DMA_CTL_READ) {
+        direction = DMA_DIRECTION_FROM_DEVICE;
+    } else if (s->dma_ctl & FW_CFG_DMA_CTL_WRITE) {
+        direction = DMA_DIRECTION_TO_DEVICE;
+    } else {
+        return;
+    }
+
+    while (s->dma_len > 0) {
+        len = s->dma_len;
+        ptr = dma_memory_map(s->dma_as, s->dma_addr, &len, direction);
+        if (!ptr || !len) {
+            s->dma_ctl |= FW_CFG_DMA_CTL_ERROR;
+            return;
+        }
+
+        if (direction == DMA_DIRECTION_FROM_DEVICE) {
+            for (i = 0; i < len; i++) {
+                ptr[i] = fw_cfg_read(s);
+            }
+        } else {
+            for (i = 0; i < len; i++) {
+                fw_cfg_write(s, ptr[i]);
+            }
+        }
+
+        s->dma_addr += i;
+        s->dma_len  -= i;
+        dma_memory_unmap(s->dma_as, ptr, len, direction, i);
+    }
+    s->dma_ctl = 0;
+}
+
+static uint64_t fw_cfg_dma_mem_read(void *opaque, hwaddr addr,
+                                    unsigned size)
+{
+    FWCfgState *s = opaque;
+    uint64_t ret = 0;
+
+    switch (addr) {
+    case FW_CFG_DMA_ADDR_LO:
+        ret = s->dma_addr & 0xffffffff;
+        break;
+    case FW_CFG_DMA_ADDR_HI:
+        ret = (s->dma_addr >> 32) & 0xffffffff;
+        break;
+    case FW_CFG_DMA_LENGTH:
+        ret = s->dma_len;
+        break;
+    case FW_CFG_DMA_CONTROL:
+        ret = s->dma_ctl;
+        break;
+    }
+    return ret;
+}
+
+static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
+                                 uint64_t value, unsigned size)
+{
+    FWCfgState *s = opaque;
+
+    switch (addr) {
+    case FW_CFG_DMA_ADDR_LO:
+        s->dma_addr &= ~((dma_addr_t)0xffffffff);
+        s->dma_addr |= value;
+        break;
+    case FW_CFG_DMA_ADDR_HI:
+        s->dma_addr &= ~(((dma_addr_t)0xffffffff) << 32);
+        s->dma_addr |= ((dma_addr_t)value) << 32;
+        break;
+    case FW_CFG_DMA_LENGTH:
+        s->dma_len = value;
+        break;
+    case FW_CFG_DMA_CONTROL:
+        value &= FW_CFG_DMA_CTL_MASK;
+        s->dma_ctl = value;
+        fw_cfg_dma_transfer(s);
+        break;
+    }
+}
+
 static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
                                   unsigned size, bool is_write)
 {
@@ -402,6 +518,16 @@ static const MemoryRegionOps fw_cfg_comb_mem_ops = {
     .valid.accepts = fw_cfg_comb_valid,
 };
 
+static const MemoryRegionOps fw_cfg_dma_mem_ops = {
+    .read = fw_cfg_dma_mem_read,
+    .write = fw_cfg_dma_mem_write,
+    .endianness = DEVICE_BIG_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
+
 static void fw_cfg_reset(DeviceState *d)
 {
     FWCfgState *s = FW_CFG(d);
@@ -442,6 +568,23 @@ static bool is_version_1(void *opaque, int version_id)
     return version_id == 1;
 }
 
+static VMStateDescription vmstate_fw_cfg_dma = {
+    .name = "fw_cfg/dma",
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(dma_addr, FWCfgState),
+        VMSTATE_UINT32(dma_len, FWCfgState),
+        VMSTATE_UINT32(dma_ctl, FWCfgState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static bool fw_cfg_dma_enabled(void *opaque)
+{
+    FWCfgState *s = opaque;
+
+    return s->dma_enabled;
+}
+
 static const VMStateDescription vmstate_fw_cfg = {
     .name = "fw_cfg",
     .version_id = 2,
@@ -451,6 +594,14 @@ static const VMStateDescription vmstate_fw_cfg = {
         VMSTATE_UINT16_HACK(cur_offset, FWCfgState, is_version_1),
         VMSTATE_UINT32_V(cur_offset, FWCfgState, 2),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection[]) {
+        {
+            .vmsd   = &vmstate_fw_cfg_dma,
+            .needed = fw_cfg_dma_enabled,
+        }, {
+            /* end of list */
+        }
     }
 };
 
@@ -744,6 +895,7 @@ static Property fw_cfg_mem_properties[] = {
     DEFINE_PROP_UINT64("ctl_addr", FWCfgMemState, ctl_addr, -1),
     DEFINE_PROP_UINT64("data_addr", FWCfgMemState, data_addr, -1),
     DEFINE_PROP_UINT32("data_width", FWCfgMemState, data_width, -1),
+    DEFINE_PROP_UINT64("dma_addr", FWCfgMemState, dma_addr, -1),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -774,6 +926,17 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
                           "fwcfg.data", data_ops->valid.max_access_size);
     sysbus_init_mmio(sbd, &s->data_iomem);
     sysbus_mmio_map(sbd, 1, s->data_addr);
+
+    if (s->dma_addr != -1) {
+#if 0 /* FIXME */
+        FW_CFG(s)->dma_as = /* Hmm, which AddressSpace ??? */;
+#endif
+        memory_region_init_io(&s->dma_iomem, OBJECT(s), &fw_cfg_dma_mem_ops,
+                              FW_CFG(s), "fwcfg.dma", FW_CFG_DMA_SIZE);
+        sysbus_init_mmio(sbd, &s->dma_iomem);
+        sysbus_mmio_map(sbd, 1, s->dma_addr);
+        FW_CFG(s)->dma_enabled = true;
+    }
 }
 
 static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v5 00/11] fw_cfg, bootorder, and UEFI+'-kernel' on arm/virt

[Alexander Graf]

On 19.12.14 10:39, Gerd Hoffmann wrote:
>   Hi,
> 
> Attached is a patch drafting a dma interface for fw_cfg, lousily based
> on the ideas I've outlined a few days ago on this list.  Applies on top
> of this patch series.
> 
> Host side only, unfinished, untested.  Mainly sending this out as
> notification that I've looked into this, to avoid duplicating work.
> I'll go disappear into xmas vacation in a few hours and will not
> continue with this until next year.
> 
> My plan is to coordinate in January with Laszlo how to go forward.  If
> someone feels bored during the holidays feel free to grab it and run
> with it.

Nice first draft :).

The major thing I would do differently is the way to access the DMA
registers. Instead of making them even more PIO/MMIO ports, I'd rather
just make them one big 256 bit fw_cfg entry. That way we don't need to
modify (and synchronize) any machine port layout to support DMA.


Alex