[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 <address@hidden>
---

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