[Qemu-devel] [PATCH 0/6] fw_cfg: qdev-related tidy-ups

[Qemu-devel] [PATCH 0/6] 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>

Mark Cave-Ayland (6):
  fw_cfg: move initialisation of FWCfgState into instance_init
  fw_cfg: move qdev_init_nofail() out from fw_cfg_init1() into callers
  fw_cfg: move setting of FW_CFG_ID into fw_cfg_init1()
  fw_cfg: move fw_cfg_init1() into the fw_cfg_*_realize() functions
  fw_cfg: use sysbus_init_mmio() in fw_cfg_io_realize()
  fw_cfg: move QOM type defines into fw_cfg.h

 hw/nvram/fw_cfg.c         |   60 ++++++++++++++++++++++++---------------------
 include/hw/nvram/fw_cfg.h |    8 ++++++
 2 files changed, 40 insertions(+), 28 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/6] fw_cfg: move initialisation of FWCfgState into instance_init

[Mark Cave-Ayland]

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 316fca9..144e0c6 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -1017,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)
+{
+    FWCfgState *s = FW_CFG(obj);
+
+    s->entries[0] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
+    s->entries[1] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
+    s->entry_order = g_new0(int, fw_cfg_max_entry(s));
+}
+
 static void fw_cfg_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -1030,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,
 };
 
@@ -1052,10 +1062,6 @@ static void fw_cfg_file_slots_allocate(FWCfgState *s, Error **errp)
                    file_slots_max);
         return;
     }
-
-    s->entries[0] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
-    s->entries[1] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
-    s->entry_order = g_new0(int, fw_cfg_max_entry(s));
 }
 
 static Property fw_cfg_io_properties[] = {
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/6] fw_cfg: move qdev_init_nofail() out from fw_cfg_init1() into callers

[Mark Cave-Ayland]

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

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 144e0c6..1313bfd 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -919,8 +919,6 @@ static void fw_cfg_init1(DeviceState *dev)
 
     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);
     fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16);
     fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)!machine->enable_graphics);
@@ -948,6 +946,8 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
     }
 
     fw_cfg_init1(dev);
+    qdev_init_nofail(dev);
+
     s = FW_CFG(dev);
 
     if (s->dma_enabled) {
@@ -985,6 +985,7 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
     }
 
     fw_cfg_init1(dev);
+    qdev_init_nofail(dev);
 
     sbd = SYS_BUS_DEVICE(dev);
     sysbus_mmio_map(sbd, 0, ctl_addr);
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/6] fw_cfg: move setting of FW_CFG_ID into fw_cfg_init1()

[Mark Cave-Ayland]

The dma_enabled property enables us to set the FW_CFG_ID version
accordingly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 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 1313bfd..f7b78a9 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -914,12 +914,19 @@ 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));
 
     object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s), NULL);
 
     fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
+
+    if (s->dma_enabled) {
+        version |= FW_CFG_VERSION_DMA;
+    }
+
+    fw_cfg_add_i32(s, FW_CFG_ID, version);
     fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16);
     fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)!machine->enable_graphics);
     fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
@@ -935,7 +942,6 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
 {
     DeviceState *dev;
     FWCfgState *s;
-    uint32_t version = FW_CFG_VERSION;
     bool dma_requested = dma_iobase && dma_as;
 
     dev = qdev_create(NULL, TYPE_FW_CFG_IO);
@@ -954,12 +960,8 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
         /* 64 bits for the address field */
         s->dma_as = dma_as;
         s->dma_addr = 0;
-
-        version |= FW_CFG_VERSION_DMA;
     }
 
-    fw_cfg_add_i32(s, FW_CFG_ID, version);
-
     return s;
 }
 
@@ -975,7 +977,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);
@@ -997,11 +998,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] [PATCH 4/6] fw_cfg: move fw_cfg_init1() into the fw_cfg_*_realize() functions

[Mark Cave-Ayland]

And rename to fw_cfg_common_realize() which better describes its role.

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

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index f7b78a9..87b4392 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -910,7 +910,7 @@ 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());
@@ -951,7 +951,6 @@ 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);
 
     s = FW_CFG(dev);
@@ -985,7 +984,6 @@ 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);
@@ -1085,6 +1083,8 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    fw_cfg_common_realize(dev);
+
     /* when using port i/o, the 8-bit data register ALWAYS overlaps
      * with half of the 16-bit control register. Hence, the total size
      * of the i/o region used is FW_CFG_CTL_SIZE */
@@ -1138,6 +1138,8 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    fw_cfg_common_realize(dev);
+
     memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops,
                           FW_CFG(s), "fwcfg.ctl", FW_CFG_CTL_SIZE);
     sysbus_init_mmio(sbd, &s->ctl_iomem);
-- 
1.7.10.4

[Qemu-devel] [PATCH 5/6] fw_cfg: use sysbus_init_mmio() in fw_cfg_io_realize()

[Mark Cave-Ayland]

This brings the function in line with fw_cfg_mem_realize(), deferring the
actual mapping until outside of the realize function.

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

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 87b4392..4159316 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -941,6 +941,7 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
                                 AddressSpace *dma_as)
 {
     DeviceState *dev;
+    SysBusDevice *sbd;
     FWCfgState *s;
     bool dma_requested = dma_iobase && dma_as;
 
@@ -953,12 +954,16 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
 
     qdev_init_nofail(dev);
 
+    sbd = SYS_BUS_DEVICE(dev);
+    sysbus_add_io(sbd, iobase, sysbus_mmio_get_region(sbd, 0));
+
     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, sysbus_mmio_get_region(sbd, 1));
     }
 
     return s;
@@ -1090,13 +1095,13 @@ 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);
+    sysbus_init_mmio(sbd, &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);
+        sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
     }
 }
 
-- 
1.7.10.4

[Qemu-devel] [PATCH 6/6] fw_cfg: move QOM type defines into fw_cfg.h

[Mark Cave-Ayland]

This allows the device to be instantiated externally.

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

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 4159316..b77de00 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
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index b980cba..e515698 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -4,6 +4,14 @@
 #include "exec/hwaddr.h"
 #include "hw/nvram/fw_cfg_keys.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 */
     uint16_t  select;      /* write this to 0x510 to read it */
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 6/6] fw_cfg: move QOM type defines into fw_cfg.h

[Philippe Mathieu-Daudé]

On 06/10/2017 09:30 AM, Mark Cave-Ayland wrote:
> This allows the device to be instantiated externally.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/nvram/fw_cfg.c         |    8 --------
>  include/hw/nvram/fw_cfg.h |    8 ++++++++
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 4159316..b77de00 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
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index b980cba..e515698 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -4,6 +4,14 @@
>  #include "exec/hwaddr.h"
>  #include "hw/nvram/fw_cfg_keys.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 */
>      uint16_t  select;      /* write this to 0x510 to read it */
>

Re: [Qemu-devel] [PATCH 1/6] fw_cfg: move initialisation of FWCfgState into instance_init

[Philippe Mathieu-Daudé]

On 06/10/2017 09:30 AM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  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 316fca9..144e0c6 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -1017,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)
> +{
> +    FWCfgState *s = FW_CFG(obj);
> +
> +    s->entries[0] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
> +    s->entries[1] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
> +    s->entry_order = g_new0(int, fw_cfg_max_entry(s));
> +}
> +
>  static void fw_cfg_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -1030,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,
>  };
>
> @@ -1052,10 +1062,6 @@ static void fw_cfg_file_slots_allocate(FWCfgState *s, Error **errp)
>                     file_slots_max);
>          return;
>      }
> -
> -    s->entries[0] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
> -    s->entries[1] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
> -    s->entry_order = g_new0(int, fw_cfg_max_entry(s));
>  }
>
>  static Property fw_cfg_io_properties[] = {
>

Re: [Qemu-devel] [PATCH 3/6] fw_cfg: move setting of FW_CFG_ID into fw_cfg_init1()

[Philippe Mathieu-Daudé]

On 06/10/2017 09:30 AM, Mark Cave-Ayland wrote:
> The dma_enabled property enables us to set the FW_CFG_ID version
> accordingly.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  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 1313bfd..f7b78a9 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -914,12 +914,19 @@ 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));
>
>      object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s), NULL);
>
>      fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
> +
> +    if (s->dma_enabled) {
> +        version |= FW_CFG_VERSION_DMA;
> +    }
> +
> +    fw_cfg_add_i32(s, FW_CFG_ID, version);
>      fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16);
>      fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)!machine->enable_graphics);
>      fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
> @@ -935,7 +942,6 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>  {
>      DeviceState *dev;
>      FWCfgState *s;
> -    uint32_t version = FW_CFG_VERSION;
>      bool dma_requested = dma_iobase && dma_as;
>
>      dev = qdev_create(NULL, TYPE_FW_CFG_IO);
> @@ -954,12 +960,8 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>          /* 64 bits for the address field */
>          s->dma_as = dma_as;
>          s->dma_addr = 0;
> -
> -        version |= FW_CFG_VERSION_DMA;
>      }
>
> -    fw_cfg_add_i32(s, FW_CFG_ID, version);
> -
>      return s;
>  }
>
> @@ -975,7 +977,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);
> @@ -997,11 +998,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;
>  }
>
>

Re: [Qemu-devel] [PATCH 0/6] fw_cfg: qdev-related tidy-ups

[Philippe Mathieu-Daudé]

Hi Mark,

Is it possible to reorder the 3rd patch (FW_CFG_ID) first or 2nd in the 
series? Mostly for cosmetic :)

On 06/10/2017 09:30 AM, Mark Cave-Ayland wrote:
> 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>
>
> Mark Cave-Ayland (6):
>   fw_cfg: move initialisation of FWCfgState into instance_init
>   fw_cfg: move qdev_init_nofail() out from fw_cfg_init1() into callers
>   fw_cfg: move setting of FW_CFG_ID into fw_cfg_init1()
>   fw_cfg: move fw_cfg_init1() into the fw_cfg_*_realize() functions
>   fw_cfg: use sysbus_init_mmio() in fw_cfg_io_realize()
>   fw_cfg: move QOM type defines into fw_cfg.h
>
>  hw/nvram/fw_cfg.c         |   60 ++++++++++++++++++++++++---------------------
>  include/hw/nvram/fw_cfg.h |    8 ++++++
>  2 files changed, 40 insertions(+), 28 deletions(-)
>

Re: [Qemu-devel] [PATCH 0/6] fw_cfg: qdev-related tidy-ups

[Mark Cave-Ayland]

On 10/06/17 19:15, Philippe Mathieu-Daudé wrote:

> Hi Mark,
> 
> Is it possible to reorder the 3rd patch (FW_CFG_ID) first or 2nd in the
> series? Mostly for cosmetic :)
> 
> On 06/10/2017 09:30 AM, Mark Cave-Ayland wrote:
>> 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>
>>
>> Mark Cave-Ayland (6):
>>   fw_cfg: move initialisation of FWCfgState into instance_init
>>   fw_cfg: move qdev_init_nofail() out from fw_cfg_init1() into callers
>>   fw_cfg: move setting of FW_CFG_ID into fw_cfg_init1()
>>   fw_cfg: move fw_cfg_init1() into the fw_cfg_*_realize() functions
>>   fw_cfg: use sysbus_init_mmio() in fw_cfg_io_realize()
>>   fw_cfg: move QOM type defines into fw_cfg.h
>>
>>  hw/nvram/fw_cfg.c         |   60
>> ++++++++++++++++++++++++---------------------
>>  include/hw/nvram/fw_cfg.h |    8 ++++++
>>  2 files changed, 40 insertions(+), 28 deletions(-)

I can't really get too excited about this - anyone else have any strong
opinions either way?


ATB,

Mark.

Re: [Qemu-devel] [PATCH 1/6] fw_cfg: move initialisation of FWCfgState into instance_init

[Igor Mammedov]

On Sat, 10 Jun 2017 13:30:16 +0100
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:

> 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 316fca9..144e0c6 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -1017,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)
> +{
> +    FWCfgState *s = FW_CFG(obj);
> +
> +    s->entries[0] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
> +    s->entries[1] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
> +    s->entry_order = g_new0(int, fw_cfg_max_entry(s));
it doesn't seem right,

consider,
 object_new(fwcfg)
   1: fw_cfg_init -> g_new0(FWCfgEntry, fw_cfg_max_entry(s) -> FW_CFG_FILE_SLOTS_DFLT);
   2: set property x-file-slots
   3: realize -> fw_cfg_file_slots_allocate()

> @@ -1052,10 +1062,6 @@ static void fw_cfg_file_slots_allocate(FWCfgState *s, Error **errp)
>                     file_slots_max);
>          return;
>      }
> -
> -    s->entries[0] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
> -    s->entries[1] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
> -    s->entry_order = g_new0(int, fw_cfg_max_entry(s));
opps, s->entries doesn't account for new values of x-file-slots

So question is why this patch is needed at all (it needs some reasoning in commit message)?

So for now NACK and I'd suggest to drop this patch.

Re: [Qemu-devel] [PATCH 3/6] fw_cfg: move setting of FW_CFG_ID into fw_cfg_init1()

[Igor Mammedov]

On Sat, 10 Jun 2017 13:30:18 +0100
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:

> The dma_enabled property enables us to set the FW_CFG_ID version
> accordingly.
it might be not safe as patch reorders FW_CFG_ID entry.

(I recall there were patches that sorted fwcfg entries,
but it's worth checking that it won't break migration)

CCing Gerd, maybe he would recall how it works.

> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  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 1313bfd..f7b78a9 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -914,12 +914,19 @@ 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));
>  
>      object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s), NULL);
>  
>      fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
> +
> +    if (s->dma_enabled) {
> +        version |= FW_CFG_VERSION_DMA;
> +    }
> +
> +    fw_cfg_add_i32(s, FW_CFG_ID, version);
>      fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16);
>      fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)!machine->enable_graphics);
>      fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
> @@ -935,7 +942,6 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>  {
>      DeviceState *dev;
>      FWCfgState *s;
> -    uint32_t version = FW_CFG_VERSION;
>      bool dma_requested = dma_iobase && dma_as;
>  
>      dev = qdev_create(NULL, TYPE_FW_CFG_IO);
> @@ -954,12 +960,8 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>          /* 64 bits for the address field */
>          s->dma_as = dma_as;
>          s->dma_addr = 0;
> -
> -        version |= FW_CFG_VERSION_DMA;
>      }
>  
> -    fw_cfg_add_i32(s, FW_CFG_ID, version);
> -
>      return s;
>  }
>  
> @@ -975,7 +977,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);
> @@ -997,11 +998,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;
>  }
>  

Re: [Qemu-devel] [PATCH 2/6] fw_cfg: move qdev_init_nofail() out from fw_cfg_init1() into callers

[Igor Mammedov]

On Sat, 10 Jun 2017 13:30:17 +0100
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:

patch needs a commit message saying why it's needed.
maybe add something similar to:

qdev_init_nofail() essentially 'realizes' object,
taking in account that fw_cfg_init1() will be moved to
realize in follow up patches, move qdev_init_nofail() outside of
fw_cfg_init1().



> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/nvram/fw_cfg.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 144e0c6..1313bfd 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -919,8 +919,6 @@ static void fw_cfg_init1(DeviceState *dev)
>  
>      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);
>      fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16);
>      fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)!machine->enable_graphics);
> @@ -948,6 +946,8 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>      }
>  
>      fw_cfg_init1(dev);
> +    qdev_init_nofail(dev);
> +
>      s = FW_CFG(dev);
>  
>      if (s->dma_enabled) {
> @@ -985,6 +985,7 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
>      }
>  
>      fw_cfg_init1(dev);
> +    qdev_init_nofail(dev);
>  
>      sbd = SYS_BUS_DEVICE(dev);
>      sysbus_mmio_map(sbd, 0, ctl_addr);

Re: [Qemu-devel] [PATCH 1/6] fw_cfg: move initialisation of FWCfgState into instance_init

[Mark Cave-Ayland]

On 12/06/17 12:20, Igor Mammedov wrote:

> On Sat, 10 Jun 2017 13:30:16 +0100
> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> 
>> 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 316fca9..144e0c6 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -1017,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)
>> +{
>> +    FWCfgState *s = FW_CFG(obj);
>> +
>> +    s->entries[0] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
>> +    s->entries[1] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
>> +    s->entry_order = g_new0(int, fw_cfg_max_entry(s));
> it doesn't seem right,
> 
> consider,
>  object_new(fwcfg)
>    1: fw_cfg_init -> g_new0(FWCfgEntry, fw_cfg_max_entry(s) -> FW_CFG_FILE_SLOTS_DFLT);
>    2: set property x-file-slots
>    3: realize -> fw_cfg_file_slots_allocate()
> 
>> @@ -1052,10 +1062,6 @@ static void fw_cfg_file_slots_allocate(FWCfgState *s, Error **errp)
>>                     file_slots_max);
>>          return;
>>      }
>> -
>> -    s->entries[0] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
>> -    s->entries[1] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
>> -    s->entry_order = g_new0(int, fw_cfg_max_entry(s));
> opps, s->entries doesn't account for new values of x-file-slots
> 
> So question is why this patch is needed at all (it needs some reasoning in commit message)?
> 
> So for now NACK and I'd suggest to drop this patch.

Right I missed the x-file-slots property when I was looking at this,
mainly because all of the existing callers went through
fw_cfg_init_mem() and fw_cfg_init_io(). I do see now though that the
property is referenced in compat.h.

The reason for moving the initialisation is that if you apply patch 2 on
its own then you get hit by the following assert on qemu-system-sparc64
due to uninitialised data:

Program received signal SIGSEGV, Segmentation fault.
0x000000000062f81d in fw_cfg_add_bytes_read_callback (s=0x1e85410,
key=0, callback=0, callback_opaque=0x0, data=0x9f6c28, len=4,
read_only=true) at hw/nvram/fw_cfg.c:633
633         assert(s->entries[arch][key].data == NULL); /* avoid key
conflict */
(gdb) bt
#0  0x000000000062f81d in fw_cfg_add_bytes_read_callback (s=0x1e85410,
key=0, callback=0, callback_opaque=0x0, data=0x9f6c28, len=4,
read_only=true) at hw/nvram/fw_cfg.c:633
#1  0x000000000062fac8 in fw_cfg_add_bytes (s=0x1e85410, key=0,
data=0x9f6c28, len=4) at hw/nvram/fw_cfg.c:664
#2  0x00000000006307a8 in fw_cfg_init1 (dev=0x1e85410) at
hw/nvram/fw_cfg.c:922
#3  0x00000000006308fe in fw_cfg_init_io_dma (iobase=1296, dma_iobase=0,
dma_as=0x0) at hw/nvram/fw_cfg.c:948
#4  0x00000000006309bc in fw_cfg_init_io (iobase=1296) at
hw/nvram/fw_cfg.c:968
#5  0x00000000004f31e9 in sun4uv_init (address_space_mem=0x132df20,
machine=0x132c8c0, hwdef=0x8bf400) at
/home/build/src/qemu/git/qemu/hw/sparc64/sun4u.c:515
#6  0x00000000004f3403 in sun4u_init (machine=0x132c8c0) at
/home/build/src/qemu/git/qemu/hw/sparc64/sun4u.c:565
#7  0x00000000005c0d26 in machine_run_board_init (machine=0x132c8c0) at
hw/core/machine.c:760


#8  0x0000000000532e56 in main (argc=2, argv=0x7fffffffeaa8,
envp=0x7fffffffeac0) at vl.c:4594

Based upon this what do you think the best solution would be?


ATB,

Mark.

Re: [Qemu-devel] [PATCH 3/6] fw_cfg: move setting of FW_CFG_ID into fw_cfg_init1()

[Mark Cave-Ayland]

On 12/06/17 12:37, Igor Mammedov wrote:

> On Sat, 10 Jun 2017 13:30:18 +0100
> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> 
>> The dma_enabled property enables us to set the FW_CFG_ID version
>> accordingly.
> it might be not safe as patch reorders FW_CFG_ID entry.
> 
> (I recall there were patches that sorted fwcfg entries,
> but it's worth checking that it won't break migration)
> 
> CCing Gerd, maybe he would recall how it works.

Ah so the order in fw_cfg needs to be preserved? (my assumption was that
the slot-id also specified the order). I'll have a look and see if I can
fix this with a simple re-ordering.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 2/6] fw_cfg: move qdev_init_nofail() out from fw_cfg_init1() into callers

[Mark Cave-Ayland]

On 12/06/17 12:43, Igor Mammedov wrote:

> On Sat, 10 Jun 2017 13:30:17 +0100
> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> 
> patch needs a commit message saying why it's needed.
> maybe add something similar to:
> 
> qdev_init_nofail() essentially 'realizes' object,
> taking in account that fw_cfg_init1() will be moved to
> realize in follow up patches, move qdev_init_nofail() outside of
> fw_cfg_init1().

Yes, I can alter the commit message to provide more explanation. For
some background the use case can be found in my follow-up sun4u patches
here (i.e. preparation for moving the ebus device behind a PCI bridge):

https://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg02658.html
https://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg02656.html


ATB,

Mark.

Re: [Qemu-devel] [PATCH 4/6] fw_cfg: move fw_cfg_init1() into the fw_cfg_*_realize() functions

[Igor Mammedov]

On Sat, 10 Jun 2017 13:30:19 +0100
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:

> And rename to fw_cfg_common_realize() which better describes its role.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/nvram/fw_cfg.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index f7b78a9..87b4392 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -910,7 +910,7 @@ 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());
> @@ -951,7 +951,6 @@ 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);
>  
>      s = FW_CFG(dev);
> @@ -985,7 +984,6 @@ 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);
> @@ -1085,6 +1083,8 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> +    fw_cfg_common_realize(dev);
For QOM objects it's done with inheritance, see 9f318f8f7e6 for example.

i.e. define in parent TYPE_FW_CFG.realize and call it from children.
You also can dedup fw_cfg_mem_realize/fw_cfg_mem_realize and
move common code into new fw_cfg_realize()

Re: [Qemu-devel] [PATCH 5/6] fw_cfg: use sysbus_init_mmio() in fw_cfg_io_realize()

[Igor Mammedov]

On Sat, 10 Jun 2017 13:30:20 +0100
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:

> This brings the function in line with fw_cfg_mem_realize(), deferring the
> actual mapping until outside of the realize function.
you are changing used API here, so add to commit message why is that

> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/nvram/fw_cfg.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 87b4392..4159316 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -941,6 +941,7 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>                                  AddressSpace *dma_as)
>  {
>      DeviceState *dev;
> +    SysBusDevice *sbd;
>      FWCfgState *s;
>      bool dma_requested = dma_iobase && dma_as;
>  
> @@ -953,12 +954,16 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>  
>      qdev_init_nofail(dev);
>  
> +    sbd = SYS_BUS_DEVICE(dev);
> +    sysbus_add_io(ssysbus_mmio_map_overlapbd, iobase, sysbus_mmio_get_region(sbd, 0));
sysbus_mmio_map()/sysbus_mmio_map_overlap() is a proper conterpart for sysbus_init_mmio
so I'd use that if APIs are switched.

> +
>      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, sysbus_mmio_get_region(sbd, 1));
>      }
>  
>      return s;
> @@ -1090,13 +1095,13 @@ 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);
> +    sysbus_init_mmio(sbd, &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);
> +        sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
>      }
>  }
>  

Re: [Qemu-devel] [PATCH 5/6] fw_cfg: use sysbus_init_mmio() in fw_cfg_io_realize()

[Mark Cave-Ayland]

On 12/06/17 13:16, Igor Mammedov wrote:

> On Sat, 10 Jun 2017 13:30:20 +0100
> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> 
>> This brings the function in line with fw_cfg_mem_realize(), deferring the
>> actual mapping until outside of the realize function.
> you are changing used API here, so add to commit message why is that

Okay I can add a comment mentioning that this is so we can wire up the
IO space ourselves?

>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/nvram/fw_cfg.c |    9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 87b4392..4159316 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -941,6 +941,7 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>>                                  AddressSpace *dma_as)
>>  {
>>      DeviceState *dev;
>> +    SysBusDevice *sbd;
>>      FWCfgState *s;
>>      bool dma_requested = dma_iobase && dma_as;
>>  
>> @@ -953,12 +954,16 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>>  
>>      qdev_init_nofail(dev);
>>  
>> +    sbd = SYS_BUS_DEVICE(dev);
>> +    sysbus_add_io(ssysbus_mmio_map_overlapbd, iobase, sysbus_mmio_get_region(sbd, 0));
> sysbus_mmio_map()/sysbus_mmio_map_overlap() is a proper conterpart for sysbus_init_mmio
> so I'd use that if APIs are switched.

Unfortunately we can't use sysbus_mmio_map() here because it maps the
resulting MemoryRegion into memory space instead of IO space.

The reason for using sysbus_init_mmio() here is that despite its name,
we can use sysbus_mmio_get_region() to obtain a reference to the
underlying s->comb_iomem MemoryRegion that the caller can use in order
to map the device into the desired IO space,

Otherwise if we use sysbus_add_io() at realize time as per the existing
code then the ioports are always mapped into system IO space which is
exactly the behaviour I'm trying to customise.

Note that this is how the m48t59 timer device is currently implemented
in hw/timer/m48t59.c.

> 
>> +
>>      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, sysbus_mmio_get_region(sbd, 1));
>>      }
>>  
>>      return s;
>> @@ -1090,13 +1095,13 @@ 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);
>> +    sysbus_init_mmio(sbd, &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);
>> +        sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
>>      }
>>  }
>>  


ATB,

Mark.

Re: [Qemu-devel] [PATCH 3/6] fw_cfg: move setting of FW_CFG_ID into fw_cfg_init1()

[Gerd Hoffmann]

On Mon, 2017-06-12 at 13:37 +0200, Igor Mammedov wrote:
> On Sat, 10 Jun 2017 13:30:18 +0100
> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> 
> > The dma_enabled property enables us to set the FW_CFG_ID version
> > accordingly.
> 
> it might be not safe as patch reorders FW_CFG_ID entry.
> 
> (I recall there were patches that sorted fwcfg entries,
> but it's worth checking that it won't break migration)
> 
> CCing Gerd, maybe he would recall how it works.

http://git.qemu.org/?p=qemu.git;a=blob;f=hw/nvram/fw_cfg.c;h=316fca9bc1
dadf8777ad9a9c238381702022bc56;hb=HEAD#l723

So, should be safe to shuffle around stuff.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/6] fw_cfg: move initialisation of FWCfgState into instance_init

[Laszlo Ersek]

Hi Mark,

On 06/12/17 13:45, Mark Cave-Ayland wrote:
> On 12/06/17 12:20, Igor Mammedov wrote:
> 
>> On Sat, 10 Jun 2017 13:30:16 +0100
>> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
>>
>>> 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 316fca9..144e0c6 100644
>>> --- a/hw/nvram/fw_cfg.c
>>> +++ b/hw/nvram/fw_cfg.c
>>> @@ -1017,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)
>>> +{
>>> +    FWCfgState *s = FW_CFG(obj);
>>> +
>>> +    s->entries[0] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
>>> +    s->entries[1] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
>>> +    s->entry_order = g_new0(int, fw_cfg_max_entry(s));
>> it doesn't seem right,
>>
>> consider,
>>  object_new(fwcfg)
>>    1: fw_cfg_init -> g_new0(FWCfgEntry, fw_cfg_max_entry(s) -> FW_CFG_FILE_SLOTS_DFLT);
>>    2: set property x-file-slots
>>    3: realize -> fw_cfg_file_slots_allocate()
>>
>>> @@ -1052,10 +1062,6 @@ static void fw_cfg_file_slots_allocate(FWCfgState *s, Error **errp)
>>>                     file_slots_max);
>>>          return;
>>>      }
>>> -
>>> -    s->entries[0] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
>>> -    s->entries[1] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
>>> -    s->entry_order = g_new0(int, fw_cfg_max_entry(s));
>> opps, s->entries doesn't account for new values of x-file-slots
>>
>> So question is why this patch is needed at all (it needs some reasoning in commit message)?
>>
>> So for now NACK and I'd suggest to drop this patch.
> 
> Right I missed the x-file-slots property when I was looking at this,
> mainly because all of the existing callers went through
> fw_cfg_init_mem() and fw_cfg_init_io(). I do see now though that the
> property is referenced in compat.h.
> 
> The reason for moving the initialisation is that if you apply patch 2 on
> its own then you get hit by the following assert on qemu-system-sparc64
> due to uninitialised data:
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x000000000062f81d in fw_cfg_add_bytes_read_callback (s=0x1e85410,
> key=0, callback=0, callback_opaque=0x0, data=0x9f6c28, len=4,
> read_only=true) at hw/nvram/fw_cfg.c:633
> 633         assert(s->entries[arch][key].data == NULL); /* avoid key
> conflict */
> (gdb) bt
> #0  0x000000000062f81d in fw_cfg_add_bytes_read_callback (s=0x1e85410,
> key=0, callback=0, callback_opaque=0x0, data=0x9f6c28, len=4,
> read_only=true) at hw/nvram/fw_cfg.c:633
> #1  0x000000000062fac8 in fw_cfg_add_bytes (s=0x1e85410, key=0,
> data=0x9f6c28, len=4) at hw/nvram/fw_cfg.c:664
> #2  0x00000000006307a8 in fw_cfg_init1 (dev=0x1e85410) at
> hw/nvram/fw_cfg.c:922
> #3  0x00000000006308fe in fw_cfg_init_io_dma (iobase=1296, dma_iobase=0,
> dma_as=0x0) at hw/nvram/fw_cfg.c:948
> #4  0x00000000006309bc in fw_cfg_init_io (iobase=1296) at
> hw/nvram/fw_cfg.c:968
> #5  0x00000000004f31e9 in sun4uv_init (address_space_mem=0x132df20,
> machine=0x132c8c0, hwdef=0x8bf400) at
> /home/build/src/qemu/git/qemu/hw/sparc64/sun4u.c:515
> #6  0x00000000004f3403 in sun4u_init (machine=0x132c8c0) at
> /home/build/src/qemu/git/qemu/hw/sparc64/sun4u.c:565
> #7  0x00000000005c0d26 in machine_run_board_init (machine=0x132c8c0) at
> hw/core/machine.c:760
> 
> 
> #8  0x0000000000532e56 in main (argc=2, argv=0x7fffffffeaa8,
> envp=0x7fffffffeac0) at vl.c:4594
> 
> Based upon this what do you think the best solution would be?


if you apply patch #2 without patch #1, then the above SIGSEGV will hit
on all fw_cfg using targets / machine types, not just
qemu-system-sparc64. The reason is that, after patch #2 (without patch
#1 applied) you have

  fw_cfg_init1()
    ...
      fw_cfg_add_bytes_read_callback()
        s->entries[arch][key].data
  qdev_init_nofail()
    fw_cfg_file_slots_allocate()

IOW, the s->entries array is now dynamically allocated (after the
introduction of the x-file-slots property):

    FWCfgEntry *entries[2];

and it is allocated in fw_cfg_file_slots_allocate(), called from
realize. But with patch #2 applied in isolation, you perform the first
dereference (from fw_cfg_init1(), indirectly) before allocation, that's
why it crashes.

Ultimately we need the following order:

(1) set properties (from device defaults, from device compat settings,
and from the command line; and also directly from code, such as in
fw_cfg_init_io_dma())

(2) call qdev_init_nofail() -- this will consume the properties from
(1), including the x-file-slots property, for allocating "s->entries" in
fw_cfg_file_slots_allocate()

(3) call fw_cfg_add_*() -- now that the device has been realized and is
usable.

This means that

- patch #1 should be dropped,

- and in patch #2, you have to push the rest of fw_cfg_init1() --
everything that is after the original qdev_init_nofail() call -- to the
end of the realize functions.


Alternatively, in patch #2, you have to split fw_cfg_init1() into two
parts, fw_cfg_init1() and fw_cfg_init2(). fw_cfg_init1() would keep
everything that's *before* the original qdev_init_nofail() call, and
fw_cfg_init2() would get everything *after*. Then, in
fw_cfg_init_io_dma() and fw_cfg_init_mem_wide(), you'd do

    fw_cfg_init1(dev);
    qdev_init_nofail(dev);
    fw_cfg_init2(dev);

In short, you cannot reorder steps (2) and (3) against each other -- you
cannot add fw_cfg entries before you realize the device -- and the crash
happens because that's exactly what patch #2 does at the moment.

And, patch #1 is not the right fix (you can allocate s->entries only in
realize, because the size depends on a property, which you can only
access in realize). The right fix is to drop patch #1 and to split
fw_cfg_init1() like described above.

... Hmmm, wait a second, while the above approach would fix patch #2, in
fact I think patch #2 doesn't have the right *goal*.

I'll comment under patch #2.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 1/6] fw_cfg: move initialisation of FWCfgState into instance_init

[Mark Cave-Ayland]

On 12/06/17 19:27, Laszlo Ersek wrote:

>> Based upon this what do you think the best solution would be?
> 
> 
> if you apply patch #2 without patch #1, then the above SIGSEGV will hit
> on all fw_cfg using targets / machine types, not just
> qemu-system-sparc64. The reason is that, after patch #2 (without patch
> #1 applied) you have
> 
>   fw_cfg_init1()
>     ...
>       fw_cfg_add_bytes_read_callback()
>         s->entries[arch][key].data
>   qdev_init_nofail()
>     fw_cfg_file_slots_allocate()
> 
> IOW, the s->entries array is now dynamically allocated (after the
> introduction of the x-file-slots property):
> 
>     FWCfgEntry *entries[2];
> 
> and it is allocated in fw_cfg_file_slots_allocate(), called from
> realize. But with patch #2 applied in isolation, you perform the first
> dereference (from fw_cfg_init1(), indirectly) before allocation, that's
> why it crashes.
> 
> Ultimately we need the following order:
> 
> (1) set properties (from device defaults, from device compat settings,
> and from the command line; and also directly from code, such as in
> fw_cfg_init_io_dma())
> 
> (2) call qdev_init_nofail() -- this will consume the properties from
> (1), including the x-file-slots property, for allocating "s->entries" in
> fw_cfg_file_slots_allocate()
> 
> (3) call fw_cfg_add_*() -- now that the device has been realized and is
> usable.
> 
> This means that
> 
> - patch #1 should be dropped,
> 
> - and in patch #2, you have to push the rest of fw_cfg_init1() --
> everything that is after the original qdev_init_nofail() call -- to the
> end of the realize functions.
> 
> 
> Alternatively, in patch #2, you have to split fw_cfg_init1() into two
> parts, fw_cfg_init1() and fw_cfg_init2(). fw_cfg_init1() would keep
> everything that's *before* the original qdev_init_nofail() call, and
> fw_cfg_init2() would get everything *after*. Then, in
> fw_cfg_init_io_dma() and fw_cfg_init_mem_wide(), you'd do
> 
>     fw_cfg_init1(dev);
>     qdev_init_nofail(dev);
>     fw_cfg_init2(dev);
> 
> In short, you cannot reorder steps (2) and (3) against each other -- you
> cannot add fw_cfg entries before you realize the device -- and the crash
> happens because that's exactly what patch #2 does at the moment.
> 
> And, patch #1 is not the right fix (you can allocate s->entries only in
> realize, because the size depends on a property, which you can only
> access in realize). The right fix is to drop patch #1 and to split
> fw_cfg_init1() like described above.
> 
> ... Hmmm, wait a second, while the above approach would fix patch #2, in
> fact I think patch #2 doesn't have the right *goal*.
> 
> I'll comment under patch #2.

Yes that now makes sense - I think I managed to confuse myself when
writing the patch since I grepped for the type and the calls to the
fw_cfg_init_*() functions but managed to miss the x-file-slots property
as pointed out by Igor :(

Reading what you said above, I'd be inclined to move the default values
from fw_cfg_init1() into a QOM method in the parent fw_cfg type as
suggested by Igor, and then call the method right at the end of the
realise function in order to load them.

However it also sounds like you have some better thoughts for patch #2
so I'll wait for your reply to that first.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 2/6] fw_cfg: move qdev_init_nofail() out from fw_cfg_init1() into callers

[Laszlo Ersek]

Adding Peter

On 06/12/17 13:54, Mark Cave-Ayland wrote:
> On 12/06/17 12:43, Igor Mammedov wrote:
> 
>> On Sat, 10 Jun 2017 13:30:17 +0100
>> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> patch needs a commit message saying why it's needed.
>> maybe add something similar to:
>>
>> qdev_init_nofail() essentially 'realizes' object,
>> taking in account that fw_cfg_init1() will be moved to
>> realize in follow up patches, move qdev_init_nofail() outside of
>> fw_cfg_init1().
> 
> Yes, I can alter the commit message to provide more explanation. For
> some background the use case can be found in my follow-up sun4u patches
> here (i.e. preparation for moving the ebus device behind a PCI bridge):
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg02658.html
> https://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg02656.html

I see.

So what you want to replace actually resides in fw_cfg_io_realize(). It
is the following set of function calls:

    sysbus_add_io(sbd, s->iobase, &s->comb_iomem);

        sysbus_add_io(sbd, s->dma_iobase, &FW_CFG(s)->dma_iomem);

which both translate to

    memory_region_add_subregion(get_system_io(), addr, mem);

And you want to replace the first parameter here, namely get_system_io().

I think your use case uncovered an actual QOM bug in
fw_cfg_io_realize(). Compare fw_cfg_mem_realize():

    sysbus_init_mmio(sbd, &s->ctl_iomem);

    sysbus_init_mmio(sbd, &s->data_iomem);

        sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);

But these function calls do not *map* subregions!

Now, I *distinctly* recall that, when we were working on
fw_cfg_mem_realize(), Paolo and Peter pointed out that, while MMIO and
IO region *creation* were "realize business", *mapping* those same
regions to address spaces (or higher level memory regions) was *board*
business... Yes, please see the following messages:

http://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg02839.html
http://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg02849.html

Ultimately, Paolo graciously fixed up this error in my then-v5 patch
(thanks again Paolo!); see:

- http://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg02851.html
- http://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg03254.html
- commit 5712db6ae510 ("fw_cfg: hard separation between the MMIO and
  I/O port mappings", 2014-12-22).

But, we all seem to have missed the exact same error in
fw_cfg_io_realize() at that time.

So here's my suggestion:

(1) Fix the QOM bug -- my mess :) -- at the very first. Move the
sysbus_add_io() calls from fw_cfg_io_realize() to fw_cfg_init_io_dma()
(which is a board helper function, not a realize function), after
fw_cfg_init1().

Notice that in fw_cfg_init_mem_wide(), we have

    fw_cfg_init1(dev);

    sbd = SYS_BUS_DEVICE(dev);
    sysbus_mmio_map(sbd, 0, ctl_addr);
    sysbus_mmio_map(sbd, 1, data_addr);

which is exactly what we should parallel in fw_cfg_init_io_dma().

(2) Once this is done, modify the fw_cfg_init_io_dma() function, so that
it takes a new MemoryRegion* parameter called "parent_io". Update all
current call sites to pass in NULL, and fw_cfg_init_io_dma() should do
something like:

  io = parent_io ? parent_io : get_system_io();
  memory_region_add_subregion(io, s->iobase, &s->comb_iomem);
  if (s->dma_enabled) {
    memory_region_add_subregion(io, s->dma_iobase, s->dma_iomem);
  }


Basically, fix the QOM bug first, by moving the region mapping out of
the realize function, to the board helper function. Then modify the
board helper function so that board code can pass in the parent_io region.

This should be two small patches, and the rest should be possible to
drop, I think.

Then, in your sun4u series, you can simply remove

    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);

and add

    fw_cfg = fw_cfg_init_io_dma(BIOS_CFG_IOPORT, 0, NULL,
                                pci_address_space_io(ebus));

... Upon re-reading your cover letter:

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

I agree that fw_cfg_init_io() should be synched with fw_cfg_init_mem(),
in that the region *mapping* should be moved from the realize function
to the board helper function.

Beyond that, I don't think that a whole lot of code movement /
reorganization is necessary. Simply empower the current board helper
function fw_cfg_init_io_dma() to take parent_io, and then use that from
sun4u board code.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 2/6] fw_cfg: move qdev_init_nofail() out from fw_cfg_init1() into callers

[Mark Cave-Ayland]

On 12/06/17 20:15, Laszlo Ersek wrote:

> Adding Peter
> 
> On 06/12/17 13:54, Mark Cave-Ayland wrote:
>> On 12/06/17 12:43, Igor Mammedov wrote:
>>
>>> On Sat, 10 Jun 2017 13:30:17 +0100
>>> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
>>>
>>> patch needs a commit message saying why it's needed.
>>> maybe add something similar to:
>>>
>>> qdev_init_nofail() essentially 'realizes' object,
>>> taking in account that fw_cfg_init1() will be moved to
>>> realize in follow up patches, move qdev_init_nofail() outside of
>>> fw_cfg_init1().
>>
>> Yes, I can alter the commit message to provide more explanation. For
>> some background the use case can be found in my follow-up sun4u patches
>> here (i.e. preparation for moving the ebus device behind a PCI bridge):
>>
>> https://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg02658.html
>> https://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg02656.html
> 
> I see.
> 
> So what you want to replace actually resides in fw_cfg_io_realize(). It
> is the following set of function calls:
> 
>     sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
> 
>         sysbus_add_io(sbd, s->dma_iobase, &FW_CFG(s)->dma_iomem);
> 
> which both translate to
> 
>     memory_region_add_subregion(get_system_io(), addr, mem);
> 
> And you want to replace the first parameter here, namely get_system_io().
> 
> I think your use case uncovered an actual QOM bug in
> fw_cfg_io_realize(). Compare fw_cfg_mem_realize():
> 
>     sysbus_init_mmio(sbd, &s->ctl_iomem);
> 
>     sysbus_init_mmio(sbd, &s->data_iomem);
> 
>         sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
> 
> But these function calls do not *map* subregions!
> 
> Now, I *distinctly* recall that, when we were working on
> fw_cfg_mem_realize(), Paolo and Peter pointed out that, while MMIO and
> IO region *creation* were "realize business", *mapping* those same
> regions to address spaces (or higher level memory regions) was *board*
> business... Yes, please see the following messages:
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg02839.html
> http://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg02849.html
> 
> Ultimately, Paolo graciously fixed up this error in my then-v5 patch
> (thanks again Paolo!); see:
> 
> - http://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg02851.html
> - http://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg03254.html
> - commit 5712db6ae510 ("fw_cfg: hard separation between the MMIO and
>   I/O port mappings", 2014-12-22).
> 
> But, we all seem to have missed the exact same error in
> fw_cfg_io_realize() at that time.
> 
> So here's my suggestion:
> 
> (1) Fix the QOM bug -- my mess :) -- at the very first. Move the
> sysbus_add_io() calls from fw_cfg_io_realize() to fw_cfg_init_io_dma()
> (which is a board helper function, not a realize function), after
> fw_cfg_init1().
> 
> Notice that in fw_cfg_init_mem_wide(), we have
> 
>     fw_cfg_init1(dev);
> 
>     sbd = SYS_BUS_DEVICE(dev);
>     sysbus_mmio_map(sbd, 0, ctl_addr);
>     sysbus_mmio_map(sbd, 1, data_addr);
> 
> which is exactly what we should parallel in fw_cfg_init_io_dma().
> 
> (2) Once this is done, modify the fw_cfg_init_io_dma() function, so that
> it takes a new MemoryRegion* parameter called "parent_io". Update all
> current call sites to pass in NULL, and fw_cfg_init_io_dma() should do
> something like:
> 
>   io = parent_io ? parent_io : get_system_io();
>   memory_region_add_subregion(io, s->iobase, &s->comb_iomem);
>   if (s->dma_enabled) {
>     memory_region_add_subregion(io, s->dma_iobase, s->dma_iomem);
>   }
> 
> 
> Basically, fix the QOM bug first, by moving the region mapping out of
> the realize function, to the board helper function. Then modify the
> board helper function so that board code can pass in the parent_io region.
> 
> This should be two small patches, and the rest should be possible to
> drop, I think.
> 
> Then, in your sun4u series, you can simply remove
> 
>     fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
> 
> and add
> 
>     fw_cfg = fw_cfg_init_io_dma(BIOS_CFG_IOPORT, 0, NULL,
>                                 pci_address_space_io(ebus));
> 
> ... Upon re-reading your cover letter:
> 
>> 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.
> 
> I agree that fw_cfg_init_io() should be synched with fw_cfg_init_mem(),
> in that the region *mapping* should be moved from the realize function
> to the board helper function.
> 
> Beyond that, I don't think that a whole lot of code movement /
> reorganization is necessary. Simply empower the current board helper
> function fw_cfg_init_io_dma() to take parent_io, and then use that from
> sun4u board code.

Thanks a lot for the clarification! I agree that (1) is definitely a
bug, however I'm not keen on (2) since by adding the parent MemoryRegion
as a parameter to fw_cfg_init_io_dma() then we've lost the symmetry
between that and fw_cfg_init_mem_wide(), and I do feel that realize
should be putting in the shared defaults itself.

Also the general "feel" of these APIs is that the _init() function sets
the defaults while instantiating the device with qdev_init_nofail()
allows you to wire up the device as required.

Now I understand this much better, let me try a v2 of this which fixes
(1) and moves fw_cfg_init1() to a QOM method in the parent as suggested
by Igor and see what you think.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 2/6] fw_cfg: move qdev_init_nofail() out from fw_cfg_init1() into callers

[Mark Cave-Ayland]

On 12/06/17 20:50, Mark Cave-Ayland wrote:

> Now I understand this much better, let me try a v2 of this which fixes
> (1) and moves fw_cfg_init1() to a QOM method in the parent as suggested
> by Igor and see what you think.

Experimenting with this some more, if the aim is to try and minimise
code movement/reorganisation then I don't think the QOM method will give
that much benefit for the work involved.

I've just created a v2 with the aim of minimising code churn which I'll
send along shortly.


ATB,

Mark.