[Qemu-devel] [PATCHv4 0/5] fw_cfg: qdev-related tidy-ups

[Qemu-devel] [PATCHv4 0/5] fw_cfg: qdev-related tidy-ups

[Mark Cave-Ayland]

As part of some ongoing sun4u work, I need to be able to wire the fw_cfg
IO interface to a separate IO space by instantiating the qdev device instead
of calling fw_cfg_init_io(). This patchset brings FW_CFG_IO in line with
FW_CFG_MEM and tidies up the realize methods accordingly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

v4:
- Undo accidental typedef change in patch 5 caught in v3 rework

v3:
- Rework patch 1 to use sysbus_add_io() as suggested by Laszlo
- Add Reviewed-By from Laszlo for patch 2
- Fix assert() when instantiating > 1 fw_cfg device (new patch 3)
- Rename fw_cfg_init1() to fw_cfg_common_realize() as part of patch 4

v2:
- Fix the QOM bug in patch 1 as indicated by Laszlo
- Minimise code churn compared to v1


Mark Cave-Ayland (5):
  fw_cfg: don't map the fw_cfg IO ports in fw_cfg_io_realize()
  fw_cfg: move setting of FW_CFG_VERSION_DMA bit to fw_cfg_init1()
  fw_cfg: move assert() and linking of fw_cfg device to the machine
    into instance_init()
  fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers
  fw_cfg: move QOM type defines and fw_cfg types into fw_cfg.h

 hw/nvram/fw_cfg.c         |  113 +++++++++++++--------------------------------
 include/hw/nvram/fw_cfg.h |   58 +++++++++++++++++++++++
 2 files changed, 90 insertions(+), 81 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCHv4 1/5] fw_cfg: don't map the fw_cfg IO ports in fw_cfg_io_realize()

[Mark Cave-Ayland]

As indicated by Laszlo it is a QOM bug for the realize() method to actually
map the device. Set up the IO regions within fw_cfg_io_realize() and defer
the mapping with sysbus_add_io() to the caller, as already done in
fw_cfg_init_mem_wide().

This makes the iobase and dma_iobase properties now obsolete so they can be
removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/nvram/fw_cfg.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 316fca9..70a0d5f 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -936,24 +936,30 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
                                 AddressSpace *dma_as)
 {
     DeviceState *dev;
+    SysBusDevice *sbd;
+    FWCfgIoState *ios;
     FWCfgState *s;
     uint32_t version = FW_CFG_VERSION;
     bool dma_requested = dma_iobase && dma_as;
 
     dev = qdev_create(NULL, TYPE_FW_CFG_IO);
-    qdev_prop_set_uint32(dev, "iobase", iobase);
-    qdev_prop_set_uint32(dev, "dma_iobase", dma_iobase);
     if (!dma_requested) {
         qdev_prop_set_bit(dev, "dma_enabled", false);
     }
 
     fw_cfg_init1(dev);
+
+    sbd = SYS_BUS_DEVICE(dev);
+    ios = FW_CFG_IO(dev);
+    sysbus_add_io(sbd, iobase, &ios->comb_iomem);
+
     s = FW_CFG(dev);
 
     if (s->dma_enabled) {
         /* 64 bits for the address field */
         s->dma_as = dma_as;
         s->dma_addr = 0;
+        sysbus_add_io(sbd, dma_iobase, &s->dma_iomem);
 
         version |= FW_CFG_VERSION_DMA;
     }
@@ -1059,8 +1065,6 @@ static void fw_cfg_file_slots_allocate(FWCfgState *s, Error **errp)
 }
 
 static Property fw_cfg_io_properties[] = {
-    DEFINE_PROP_UINT32("iobase", FWCfgIoState, iobase, -1),
-    DEFINE_PROP_UINT32("dma_iobase", FWCfgIoState, dma_iobase, -1),
     DEFINE_PROP_BOOL("dma_enabled", FWCfgIoState, parent_obj.dma_enabled,
                      true),
     DEFINE_PROP_UINT16("x-file-slots", FWCfgIoState, parent_obj.file_slots,
@@ -1071,7 +1075,6 @@ static Property fw_cfg_io_properties[] = {
 static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
 {
     FWCfgIoState *s = FW_CFG_IO(dev);
-    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     Error *local_err = NULL;
 
     fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
@@ -1085,13 +1088,11 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
      * of the i/o region used is FW_CFG_CTL_SIZE */
     memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
                           FW_CFG(s), "fwcfg", FW_CFG_CTL_SIZE);
-    sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
 
     if (FW_CFG(s)->dma_enabled) {
         memory_region_init_io(&FW_CFG(s)->dma_iomem, OBJECT(s),
                               &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
                               sizeof(dma_addr_t));
-        sysbus_add_io(sbd, s->dma_iobase, &FW_CFG(s)->dma_iomem);
     }
 }
 
-- 
1.7.10.4

[Qemu-devel] [PATCHv4 2/5] fw_cfg: move setting of FW_CFG_VERSION_DMA bit to fw_cfg_init1()

[Mark Cave-Ayland]

The setting of the FW_CFG_VERSION_DMA bit is the same across both the
TYPE_FW_CFG_MEM and TYPE_FW_CFG_IO devices, so unify the logic in
fw_cfg_init1().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/nvram/fw_cfg.c |   16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 70a0d5f..e217239 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -914,6 +914,7 @@ static void fw_cfg_init1(DeviceState *dev)
 {
     FWCfgState *s = FW_CFG(dev);
     MachineState *machine = MACHINE(qdev_get_machine());
+    uint32_t version = FW_CFG_VERSION;
 
     assert(!object_resolve_path(FW_CFG_PATH, NULL));
 
@@ -928,6 +929,12 @@ static void fw_cfg_init1(DeviceState *dev)
     fw_cfg_bootsplash(s);
     fw_cfg_reboot(s);
 
+    if (s->dma_enabled) {
+        version |= FW_CFG_VERSION_DMA;
+    }
+
+    fw_cfg_add_i32(s, FW_CFG_ID, version);
+
     s->machine_ready.notify = fw_cfg_machine_ready;
     qemu_add_machine_init_done_notifier(&s->machine_ready);
 }
@@ -939,7 +946,6 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
     SysBusDevice *sbd;
     FWCfgIoState *ios;
     FWCfgState *s;
-    uint32_t version = FW_CFG_VERSION;
     bool dma_requested = dma_iobase && dma_as;
 
     dev = qdev_create(NULL, TYPE_FW_CFG_IO);
@@ -960,12 +966,8 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
         s->dma_as = dma_as;
         s->dma_addr = 0;
         sysbus_add_io(sbd, dma_iobase, &s->dma_iomem);
-
-        version |= FW_CFG_VERSION_DMA;
     }
 
-    fw_cfg_add_i32(s, FW_CFG_ID, version);
-
     return s;
 }
 
@@ -981,7 +983,6 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
     DeviceState *dev;
     SysBusDevice *sbd;
     FWCfgState *s;
-    uint32_t version = FW_CFG_VERSION;
     bool dma_requested = dma_addr && dma_as;
 
     dev = qdev_create(NULL, TYPE_FW_CFG_MEM);
@@ -1002,11 +1003,8 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
         s->dma_as = dma_as;
         s->dma_addr = 0;
         sysbus_mmio_map(sbd, 2, dma_addr);
-        version |= FW_CFG_VERSION_DMA;
     }
 
-    fw_cfg_add_i32(s, FW_CFG_ID, version);
-
     return s;
 }
 
-- 
1.7.10.4

[Qemu-devel] [PATCHv4 3/5] fw_cfg: move assert() and linking of fw_cfg device to the machine into instance_init()

[Mark Cave-Ayland]

In preparation for calling fw_cfg_init1() during realize rather than during
init, move the assert() checking for existing fw_cfg devices and the linking
of the device to the machine with object_property_add_child() to a new
fw_cfg instance_init() function.

This guarantees that we will still assert() correctly if more than one fw_cfg
device is instantiated by accident.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/nvram/fw_cfg.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index e217239..39fcc04 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -916,10 +916,6 @@ static void fw_cfg_init1(DeviceState *dev)
     MachineState *machine = MACHINE(qdev_get_machine());
     uint32_t version = FW_CFG_VERSION;
 
-    assert(!object_resolve_path(FW_CFG_PATH, NULL));
-
-    object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s), NULL);
-
     qdev_init_nofail(dev);
 
     fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
@@ -1021,6 +1017,15 @@ FWCfgState *fw_cfg_find(void)
     return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
 }
 
+static void fw_cfg_init(Object *obj)
+{
+    MachineState *machine = MACHINE(qdev_get_machine());
+
+    assert(!object_resolve_path(FW_CFG_PATH, NULL));
+
+    object_property_add_child(OBJECT(machine), FW_CFG_NAME, obj, NULL);
+}
+
 static void fw_cfg_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -1034,6 +1039,7 @@ static const TypeInfo fw_cfg_info = {
     .parent        = TYPE_SYS_BUS_DEVICE,
     .abstract      = true,
     .instance_size = sizeof(FWCfgState),
+    .instance_init = fw_cfg_init,
     .class_init    = fw_cfg_class_init,
 };
 
-- 
1.7.10.4

[Qemu-devel] [PATCHv4 4/5] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers

[Mark Cave-Ayland]

When looking to instantiate a TYPE_FW_CFG_MEM or TYPE_FW_CFG_IO device to be
able to wire it up differently, it is much more convenient for the caller to
instantiate the device and have the fw_cfg default files already preloaded
during realize.

Move fw_cfg_init1() to the end of both the fw_cfg_mem_realize() and
fw_cfg_io_realize() functions so it no longer needs to be called manually
when instantiating the device, and also rename it to fw_cfg_common_realize()
which better describes its new purpose.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/nvram/fw_cfg.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 39fcc04..b5f10ac 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -910,14 +910,12 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data)
 
 
 
-static void fw_cfg_init1(DeviceState *dev)
+static void fw_cfg_common_realize(DeviceState *dev)
 {
     FWCfgState *s = FW_CFG(dev);
     MachineState *machine = MACHINE(qdev_get_machine());
     uint32_t version = FW_CFG_VERSION;
 
-    qdev_init_nofail(dev);
-
     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)!machine->enable_graphics);
@@ -949,7 +947,7 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
         qdev_prop_set_bit(dev, "dma_enabled", false);
     }
 
-    fw_cfg_init1(dev);
+    qdev_init_nofail(dev);
 
     sbd = SYS_BUS_DEVICE(dev);
     ios = FW_CFG_IO(dev);
@@ -987,7 +985,7 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
         qdev_prop_set_bit(dev, "dma_enabled", false);
     }
 
-    fw_cfg_init1(dev);
+    qdev_init_nofail(dev);
 
     sbd = SYS_BUS_DEVICE(dev);
     sysbus_mmio_map(sbd, 0, ctl_addr);
@@ -1098,6 +1096,8 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
                               &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
                               sizeof(dma_addr_t));
     }
+
+    fw_cfg_common_realize(dev);
 }
 
 static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
@@ -1164,6 +1164,8 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
                               sizeof(dma_addr_t));
         sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
     }
+
+    fw_cfg_common_realize(dev);
 }
 
 static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
-- 
1.7.10.4

[Qemu-devel] [PATCHv4 5/5] fw_cfg: move QOM type defines and fw_cfg types into fw_cfg.h

[Mark Cave-Ayland]

This allows the device to be instantiated externally for boards that
wish to wire up the fw_cfg device themselves.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/nvram/fw_cfg.c         |   56 -------------------------------------------
 include/hw/nvram/fw_cfg.h |   58 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 56 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index b5f10ac..00771c9 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -40,14 +40,6 @@
 #define FW_CFG_NAME "fw_cfg"
 #define FW_CFG_PATH "/machine/" FW_CFG_NAME
 
-#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)
-
 /* FW_CFG_VERSION bits */
 #define FW_CFG_VERSION      0x01
 #define FW_CFG_VERSION_DMA  0x02
@@ -61,54 +53,6 @@
 
 #define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */
 
-typedef struct FWCfgEntry {
-    uint32_t len;
-    bool allow_write;
-    uint8_t *data;
-    void *callback_opaque;
-    FWCfgReadCallback read_callback;
-} FWCfgEntry;
-
-struct FWCfgState {
-    /*< private >*/
-    SysBusDevice parent_obj;
-    /*< public >*/
-
-    uint16_t file_slots;
-    FWCfgEntry *entries[2];
-    int *entry_order;
-    FWCfgFiles *files;
-    uint16_t cur_entry;
-    uint32_t cur_offset;
-    Notifier machine_ready;
-
-    int fw_cfg_order_override;
-
-    bool dma_enabled;
-    dma_addr_t dma_addr;
-    AddressSpace *dma_as;
-    MemoryRegion dma_iomem;
-};
-
-struct FWCfgIoState {
-    /*< private >*/
-    FWCfgState parent_obj;
-    /*< public >*/
-
-    MemoryRegion comb_iomem;
-    uint32_t iobase, dma_iobase;
-};
-
-struct FWCfgMemState {
-    /*< private >*/
-    FWCfgState parent_obj;
-    /*< public >*/
-
-    MemoryRegion ctl_iomem, data_iomem;
-    uint32_t data_width;
-    MemoryRegionOps wide_data_ops;
-};
-
 #define JPG_FILE 0
 #define BMP_FILE 1
 
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index b980cba..a16907a 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -3,6 +3,16 @@
 
 #include "exec/hwaddr.h"
 #include "hw/nvram/fw_cfg_keys.h"
+#include "hw/sysbus.h"
+#include "sysemu/dma.h"
+
+#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 FWCfgFile {
     uint32_t  size;        /* file size */
@@ -35,6 +45,54 @@ typedef struct FWCfgDmaAccess {
 
 typedef void (*FWCfgReadCallback)(void *opaque);
 
+typedef struct FWCfgEntry {
+    uint32_t len;
+    bool allow_write;
+    uint8_t *data;
+    void *callback_opaque;
+    FWCfgReadCallback read_callback;
+} FWCfgEntry;
+
+struct FWCfgState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
+    uint16_t file_slots;
+    FWCfgEntry *entries[2];
+    int *entry_order;
+    FWCfgFiles *files;
+    uint16_t cur_entry;
+    uint32_t cur_offset;
+    Notifier machine_ready;
+
+    int fw_cfg_order_override;
+
+    bool dma_enabled;
+    dma_addr_t dma_addr;
+    AddressSpace *dma_as;
+    MemoryRegion dma_iomem;
+};
+
+struct FWCfgIoState {
+    /*< private >*/
+    FWCfgState parent_obj;
+    /*< public >*/
+
+    MemoryRegion comb_iomem;
+    uint32_t iobase, dma_iobase;
+};
+
+struct FWCfgMemState {
+    /*< private >*/
+    FWCfgState parent_obj;
+    /*< public >*/
+
+    MemoryRegion ctl_iomem, data_iomem;
+    uint32_t data_width;
+    MemoryRegionOps wide_data_ops;
+};
+
 /**
  * fw_cfg_add_bytes:
  * @s: fw_cfg device being modified
-- 
1.7.10.4

Re: [Qemu-devel] [PATCHv4 1/5] fw_cfg: don't map the fw_cfg IO ports in fw_cfg_io_realize()

[Laszlo Ersek]

On 06/16/17 15:23, Mark Cave-Ayland wrote:
> As indicated by Laszlo it is a QOM bug for the realize() method to actually
> map the device. Set up the IO regions within fw_cfg_io_realize() and defer
> the mapping with sysbus_add_io() to the caller, as already done in
> fw_cfg_init_mem_wide().
> 
> This makes the iobase and dma_iobase properties now obsolete so they can be
> removed.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/nvram/fw_cfg.c |   15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 316fca9..70a0d5f 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -936,24 +936,30 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>                                  AddressSpace *dma_as)
>  {
>      DeviceState *dev;
> +    SysBusDevice *sbd;
> +    FWCfgIoState *ios;
>      FWCfgState *s;
>      uint32_t version = FW_CFG_VERSION;
>      bool dma_requested = dma_iobase && dma_as;
>  
>      dev = qdev_create(NULL, TYPE_FW_CFG_IO);
> -    qdev_prop_set_uint32(dev, "iobase", iobase);
> -    qdev_prop_set_uint32(dev, "dma_iobase", dma_iobase);
>      if (!dma_requested) {
>          qdev_prop_set_bit(dev, "dma_enabled", false);
>      }
>  
>      fw_cfg_init1(dev);
> +
> +    sbd = SYS_BUS_DEVICE(dev);
> +    ios = FW_CFG_IO(dev);
> +    sysbus_add_io(sbd, iobase, &ios->comb_iomem);
> +
>      s = FW_CFG(dev);
>  
>      if (s->dma_enabled) {
>          /* 64 bits for the address field */
>          s->dma_as = dma_as;
>          s->dma_addr = 0;
> +        sysbus_add_io(sbd, dma_iobase, &s->dma_iomem);
>  
>          version |= FW_CFG_VERSION_DMA;
>      }
> @@ -1059,8 +1065,6 @@ static void fw_cfg_file_slots_allocate(FWCfgState *s, Error **errp)
>  }
>  
>  static Property fw_cfg_io_properties[] = {
> -    DEFINE_PROP_UINT32("iobase", FWCfgIoState, iobase, -1),
> -    DEFINE_PROP_UINT32("dma_iobase", FWCfgIoState, dma_iobase, -1),
>      DEFINE_PROP_BOOL("dma_enabled", FWCfgIoState, parent_obj.dma_enabled,
>                       true),
>      DEFINE_PROP_UINT16("x-file-slots", FWCfgIoState, parent_obj.file_slots,
> @@ -1071,7 +1075,6 @@ static Property fw_cfg_io_properties[] = {
>  static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
>  {
>      FWCfgIoState *s = FW_CFG_IO(dev);
> -    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>      Error *local_err = NULL;
>  
>      fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
> @@ -1085,13 +1088,11 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
>       * of the i/o region used is FW_CFG_CTL_SIZE */
>      memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
>                            FW_CFG(s), "fwcfg", FW_CFG_CTL_SIZE);
> -    sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
>  
>      if (FW_CFG(s)->dma_enabled) {
>          memory_region_init_io(&FW_CFG(s)->dma_iomem, OBJECT(s),
>                                &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
>                                sizeof(dma_addr_t));
> -        sysbus_add_io(sbd, s->dma_iobase, &FW_CFG(s)->dma_iomem);
>      }
>  }
>  
> 

This patch looks good to me, but I think you should also remove the
"FWCfgIoState.iobase" and "FWCfgIoState.dma_iobase" fields altogether;
that is, from the definition of the "FWCfgIoState" structure. The fields
are no longer used after this patch. (And they are irrelevant for
migration too.)

With that update, you can add my

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

Thanks
Laszlo

Re: [Qemu-devel] [PATCHv4 3/5] fw_cfg: move assert() and linking of fw_cfg device to the machine into instance_init()

[Laszlo Ersek]

On 06/16/17 15:23, Mark Cave-Ayland wrote:
> In preparation for calling fw_cfg_init1() during realize rather than during
> init, move the assert() checking for existing fw_cfg devices and the linking
> of the device to the machine with object_property_add_child() to a new
> fw_cfg instance_init() function.
> 
> This guarantees that we will still assert() correctly if more than one fw_cfg
> device is instantiated by accident.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/nvram/fw_cfg.c |   14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index e217239..39fcc04 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -916,10 +916,6 @@ static void fw_cfg_init1(DeviceState *dev)
>      MachineState *machine = MACHINE(qdev_get_machine());
>      uint32_t version = FW_CFG_VERSION;
>  
> -    assert(!object_resolve_path(FW_CFG_PATH, NULL));
> -
> -    object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s), NULL);
> -
>      qdev_init_nofail(dev);
>  
>      fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
> @@ -1021,6 +1017,15 @@ FWCfgState *fw_cfg_find(void)
>      return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
>  }
>  
> +static void fw_cfg_init(Object *obj)
> +{
> +    MachineState *machine = MACHINE(qdev_get_machine());
> +
> +    assert(!object_resolve_path(FW_CFG_PATH, NULL));
> +
> +    object_property_add_child(OBJECT(machine), FW_CFG_NAME, obj, NULL);
> +}
> +
>  static void fw_cfg_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -1034,6 +1039,7 @@ static const TypeInfo fw_cfg_info = {
>      .parent        = TYPE_SYS_BUS_DEVICE,
>      .abstract      = true,
>      .instance_size = sizeof(FWCfgState),
> +    .instance_init = fw_cfg_init,
>      .class_init    = fw_cfg_class_init,
>  };
>  
> 

Hopefully I'm not wrong :) , but this looks like a surprisingly
attractive solution to me. Basically it sinks the assertion and the link
creation into the qdev_create() calls of fw_cfg_init_io_dma() and
fw_cfg_init_mem_wide(), which is good, because the assert / linking
preserve their relative order to the realization (i.e., they keep
preceding it).

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

Thanks
Laszlo

Re: [Qemu-devel] [PATCHv4 4/5] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers

[Laszlo Ersek]

On 06/16/17 15:23, Mark Cave-Ayland wrote:
> When looking to instantiate a TYPE_FW_CFG_MEM or TYPE_FW_CFG_IO device to be
> able to wire it up differently, it is much more convenient for the caller to
> instantiate the device and have the fw_cfg default files already preloaded
> during realize.
> 
> Move fw_cfg_init1() to the end of both the fw_cfg_mem_realize() and
> fw_cfg_io_realize() functions so it no longer needs to be called manually
> when instantiating the device, and also rename it to fw_cfg_common_realize()
> which better describes its new purpose.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/nvram/fw_cfg.c |   12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 39fcc04..b5f10ac 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -910,14 +910,12 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data)
>  
>  
>  
> -static void fw_cfg_init1(DeviceState *dev)
> +static void fw_cfg_common_realize(DeviceState *dev)
>  {
>      FWCfgState *s = FW_CFG(dev);
>      MachineState *machine = MACHINE(qdev_get_machine());
>      uint32_t version = FW_CFG_VERSION;
>  
> -    qdev_init_nofail(dev);
> -
>      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)!machine->enable_graphics);
> @@ -949,7 +947,7 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>          qdev_prop_set_bit(dev, "dma_enabled", false);
>      }
>  
> -    fw_cfg_init1(dev);
> +    qdev_init_nofail(dev);
>  
>      sbd = SYS_BUS_DEVICE(dev);
>      ios = FW_CFG_IO(dev);
> @@ -987,7 +985,7 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
>          qdev_prop_set_bit(dev, "dma_enabled", false);
>      }
>  
> -    fw_cfg_init1(dev);
> +    qdev_init_nofail(dev);
>  
>      sbd = SYS_BUS_DEVICE(dev);
>      sysbus_mmio_map(sbd, 0, ctl_addr);
> @@ -1098,6 +1096,8 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
>                                &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
>                                sizeof(dma_addr_t));
>      }
> +
> +    fw_cfg_common_realize(dev);
>  }
>  
>  static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
> @@ -1164,6 +1164,8 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
>                                sizeof(dma_addr_t));
>          sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
>      }
> +
> +    fw_cfg_common_realize(dev);
>  }
>  
>  static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
> 

Right, this looks good now. All the operations keep their relative
order, and whatever remains in fw_cfg_common_realize() seems appropriate
to me for calling from a realize method.

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

Thanks,
Laszlo

Re: [Qemu-devel] [PATCHv4 5/5] fw_cfg: move QOM type defines and fw_cfg types into fw_cfg.h

[Laszlo Ersek]

On 06/16/17 15:23, Mark Cave-Ayland wrote:
> This allows the device to be instantiated externally for boards that
> wish to wire up the fw_cfg device themselves.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/nvram/fw_cfg.c         |   56 -------------------------------------------
>  include/hw/nvram/fw_cfg.h |   58 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+), 56 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index b5f10ac..00771c9 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -40,14 +40,6 @@
>  #define FW_CFG_NAME "fw_cfg"
>  #define FW_CFG_PATH "/machine/" FW_CFG_NAME
>  
> -#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)
> -
>  /* FW_CFG_VERSION bits */
>  #define FW_CFG_VERSION      0x01
>  #define FW_CFG_VERSION_DMA  0x02
> @@ -61,54 +53,6 @@
>  
>  #define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */
>  
> -typedef struct FWCfgEntry {
> -    uint32_t len;
> -    bool allow_write;
> -    uint8_t *data;
> -    void *callback_opaque;
> -    FWCfgReadCallback read_callback;
> -} FWCfgEntry;
> -
> -struct FWCfgState {
> -    /*< private >*/
> -    SysBusDevice parent_obj;
> -    /*< public >*/
> -
> -    uint16_t file_slots;
> -    FWCfgEntry *entries[2];
> -    int *entry_order;
> -    FWCfgFiles *files;
> -    uint16_t cur_entry;
> -    uint32_t cur_offset;
> -    Notifier machine_ready;
> -
> -    int fw_cfg_order_override;
> -
> -    bool dma_enabled;
> -    dma_addr_t dma_addr;
> -    AddressSpace *dma_as;
> -    MemoryRegion dma_iomem;
> -};
> -
> -struct FWCfgIoState {
> -    /*< private >*/
> -    FWCfgState parent_obj;
> -    /*< public >*/
> -
> -    MemoryRegion comb_iomem;
> -    uint32_t iobase, dma_iobase;
> -};
> -
> -struct FWCfgMemState {
> -    /*< private >*/
> -    FWCfgState parent_obj;
> -    /*< public >*/
> -
> -    MemoryRegion ctl_iomem, data_iomem;
> -    uint32_t data_width;
> -    MemoryRegionOps wide_data_ops;
> -};
> -
>  #define JPG_FILE 0
>  #define BMP_FILE 1
>  
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index b980cba..a16907a 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -3,6 +3,16 @@
>  
>  #include "exec/hwaddr.h"
>  #include "hw/nvram/fw_cfg_keys.h"
> +#include "hw/sysbus.h"
> +#include "sysemu/dma.h"
> +
> +#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 FWCfgFile {
>      uint32_t  size;        /* file size */
> @@ -35,6 +45,54 @@ typedef struct FWCfgDmaAccess {
>  
>  typedef void (*FWCfgReadCallback)(void *opaque);
>  
> +typedef struct FWCfgEntry {
> +    uint32_t len;
> +    bool allow_write;
> +    uint8_t *data;
> +    void *callback_opaque;
> +    FWCfgReadCallback read_callback;
> +} FWCfgEntry;
> +
> +struct FWCfgState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +    /*< public >*/
> +
> +    uint16_t file_slots;
> +    FWCfgEntry *entries[2];
> +    int *entry_order;
> +    FWCfgFiles *files;
> +    uint16_t cur_entry;
> +    uint32_t cur_offset;
> +    Notifier machine_ready;
> +
> +    int fw_cfg_order_override;
> +
> +    bool dma_enabled;
> +    dma_addr_t dma_addr;
> +    AddressSpace *dma_as;
> +    MemoryRegion dma_iomem;
> +};
> +
> +struct FWCfgIoState {
> +    /*< private >*/
> +    FWCfgState parent_obj;
> +    /*< public >*/
> +
> +    MemoryRegion comb_iomem;
> +    uint32_t iobase, dma_iobase;
> +};
> +
> +struct FWCfgMemState {
> +    /*< private >*/
> +    FWCfgState parent_obj;
> +    /*< public >*/
> +
> +    MemoryRegion ctl_iomem, data_iomem;
> +    uint32_t data_width;
> +    MemoryRegionOps wide_data_ops;
> +};
> +
>  /**
>   * fw_cfg_add_bytes:
>   * @s: fw_cfg device being modified
> 

This patch is generally good, but I'd like to suggest improvements:

(1) In the commit message, please mention that we are exposing the
internals of FWCfgIoState and FWCfgMemState so that board code can map
the MemoryRegion fields (such as comb_iomem) *by name*.

(2) FWCfgEntry need not be moved from the C file to the header file,
since we never depend on the *size* of that structure. Instead, please
add a single line to "include/qemu/typedefs.h":

   typedef struct FWCfgEntry FWCfgEntry;

and modify

    typedef struct FWCfgEntry {
    ...
    } FWCfgEntry;

in "fw_cfg.c" to just

    struct FWCfgEntry {
    ...
    };

Then "fw_cfg.h" can simply include "typedefs.h" and say

    ...
    FWCfgEntry *entries[2];
    ...

IOW, it's fine to leave FWCfgEntry an incomplete type in "fw_cfg.h".

(3) When you fix up patch #1 like I requested, removing the
"FWCfgIoState.iobase" and "FWCfgIoState.dma_iobase" fields, please don't
forget to update this patch as well, so that you not re-introduce those
fields in the header file.

... I'm positively satisfied with this series (I plan to "grant" my R-b
for PATCH v5 5/5, with the above updates), but given that I'm no QOM
expert by any means, I'd like someone with more QOM experience to review
the patchset as well.

Thanks!
Laszlo

Re: [Qemu-devel] [PATCHv4 5/5] fw_cfg: move QOM type defines and fw_cfg types into fw_cfg.h

[Mark Cave-Ayland]

On 16/06/17 22:28, Laszlo Ersek wrote:

> This patch is generally good, but I'd like to suggest improvements:
> 
> (1) In the commit message, please mention that we are exposing the
> internals of FWCfgIoState and FWCfgMemState so that board code can map
> the MemoryRegion fields (such as comb_iomem) *by name*.
> 
> (2) FWCfgEntry need not be moved from the C file to the header file,
> since we never depend on the *size* of that structure. Instead, please
> add a single line to "include/qemu/typedefs.h":
> 
>    typedef struct FWCfgEntry FWCfgEntry;
> 
> and modify
> 
>     typedef struct FWCfgEntry {
>     ...
>     } FWCfgEntry;
> 
> in "fw_cfg.c" to just
> 
>     struct FWCfgEntry {
>     ...
>     };
> 
> Then "fw_cfg.h" can simply include "typedefs.h" and say
> 
>     ...
>     FWCfgEntry *entries[2];
>     ...
> 
> IOW, it's fine to leave FWCfgEntry an incomplete type in "fw_cfg.h".
> 
> (3) When you fix up patch #1 like I requested, removing the
> "FWCfgIoState.iobase" and "FWCfgIoState.dma_iobase" fields, please don't
> forget to update this patch as well, so that you not re-introduce those
> fields in the header file.
> 
> ... I'm positively satisfied with this series (I plan to "grant" my R-b
> for PATCH v5 5/5, with the above updates), but given that I'm no QOM
> expert by any means, I'd like someone with more QOM experience to review
> the patchset as well.

Thanks for the feedback! I've just revised the patchset based upon your
comments and will post the v5 to the list shortly.


ATB,

Mark.