[Qemu-devel] [PATCHv2 0/4] fw_cfg: qdev-related tidy-ups

[Qemu-devel] [PATCHv2 0/4] 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>

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


Mark Cave-Ayland (4):
  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 qdev_init_nofail() from fw_cfg_init1() to callers
  fw_cfg: move QOM type defines into fw_cfg.h

 hw/nvram/fw_cfg.c         |   44 +++++++++++++++++++++-----------------------
 include/hw/nvram/fw_cfg.h |    8 ++++++++
 2 files changed, 29 insertions(+), 23 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCHv2 1/4] 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 with sysbus_init_mmio() and defer
the mapping to the caller, as already done in fw_cfg_init_mem_wide().

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

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 316fca9..be5b04e 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -936,6 +936,7 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
                                 AddressSpace *dma_as)
 {
     DeviceState *dev;
+    SysBusDevice *sbd;
     FWCfgState *s;
     uint32_t version = FW_CFG_VERSION;
     bool dma_requested = dma_iobase && dma_as;
@@ -948,12 +949,17 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
     }
 
     fw_cfg_init1(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));
 
         version |= FW_CFG_VERSION_DMA;
     }
@@ -1085,13 +1091,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] [PATCHv2 2/4] 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>
---
 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 be5b04e..e1aa4fc 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);
 }
@@ -938,7 +945,6 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
     DeviceState *dev;
     SysBusDevice *sbd;
     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, sysbus_mmio_get_region(sbd, 1));
-
-        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] [PATCHv2 3/4] 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.

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

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index e1aa4fc..6c21e43 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -920,8 +920,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);
@@ -954,7 +952,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);
     sysbus_add_io(sbd, iobase, sysbus_mmio_get_region(sbd, 0));
@@ -991,7 +989,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);
@@ -1097,6 +1095,8 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
                               sizeof(dma_addr_t));
         sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
     }
+
+    fw_cfg_init1(dev);
 }
 
 static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
@@ -1163,6 +1163,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_init1(dev);
 }
 
 static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
-- 
1.7.10.4

[Qemu-devel] [PATCHv2 4/4] fw_cfg: move QOM type defines 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         |    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 6c21e43..22a8404 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] [PATCHv2 1/4] fw_cfg: don't map the fw_cfg IO ports in fw_cfg_io_realize()

[Laszlo Ersek]

On 06/12/17 23:21, 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 with sysbus_init_mmio() and defer
> the mapping to the caller, as already done in fw_cfg_init_mem_wide().
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/nvram/fw_cfg.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 316fca9..be5b04e 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -936,6 +936,7 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>                                  AddressSpace *dma_as)
>  {
>      DeviceState *dev;
> +    SysBusDevice *sbd;
>      FWCfgState *s;
>      uint32_t version = FW_CFG_VERSION;
>      bool dma_requested = dma_iobase && dma_as;
> @@ -948,12 +949,17 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>      }
>
>      fw_cfg_init1(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));
>
>          version |= FW_CFG_VERSION_DMA;
>      }
> @@ -1085,13 +1091,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);
>      }
>  }
>
>

This does mostly what I had in mind, but why are the sysbus_init_mmio()
"replacements" necessary?

This is what sysbus_init_mmio() does:

    assert(dev->num_mmio < QDEV_MAX_MMIO);
    n = dev->num_mmio++;
    dev->mmio[n].addr = -1;
    dev->mmio[n].memory = memory;

But we don't have MMIO regions here, we have IO ports. This is all what
sysbus_add_io() does:

    memory_region_add_subregion(get_system_io(), addr, mem);

It doesn't do anything related to SysBusDevice.mmio[] and mmio.num_mmio.

This patch, as written, changes "num_mmio" from zero to nonzero, and
that could have a bunch of unexpected consequences for
"hw/core/sysbus.c":
- sysbus_has_mmio() would return true
- sysbus_dev_print() produces different output
- sysbus_get_fw_dev_path() formats a different OpenFW device path
  fragment

Instead, can we just move the sysbus_add_io() function calls *without*
replacements in fw_cfg_io_realize()?

In fw_cfg_init_io_dma(), you know the object type exactly -- you just
created it with TYPE_FW_CFG_IO --, so after device realization, you can
cast the type as narrowly as necessary, and refer to the fields by name.
Something like (build-tested only):

> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 316fca9bc1da..a28ce1eacd63 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -96,7 +96,6 @@ struct FWCfgIoState {
>      /*< public >*/
>
>      MemoryRegion comb_iomem;
> -    uint32_t iobase, dma_iobase;
>  };
>
>  struct FWCfgMemState {
> @@ -936,24 +935,29 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>                                  AddressSpace *dma_as)
>  {
>      DeviceState *dev;
> +    SysBusDevice *sbd;
>      FWCfgState *s;
> +    FWCfgIoState *ios;
>      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);
>      s = FW_CFG(dev);
> +    ios = FW_CFG_IO(dev);
> +
> +    sysbus_add_io(sbd, iobase, &ios->comb_iomem);
>
>      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 +1063,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 +1073,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 +1086,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);
>      }
>  }
>

It turns out that we introduced the "iobase" and "dma_iobase" properties
*solely* so that we could pass arguments to fw_cfg_io_realize(). But
fw_cfg_io_realize() only needed those parameters for the *wrong* purpose
(namely calling sysbus_add_io()). By eliminating the sysbus_add_io()
calls from fw_cfg_io_realize(), the related properties become
unnecessary too.

(NB, setting the "dma_enabled" property in fw_cfg_init_io_dma(), and
setting "data_width" and "dma_enabled" in fw_cfg_init_mem_wide(), remain
necessary, because those aren't related to region mapping.)

What do you think?

Thanks!
Laszlo

Re: [Qemu-devel] [PATCHv2 2/4] fw_cfg: move setting of FW_CFG_VERSION_DMA bit to fw_cfg_init1()

[Laszlo Ersek]

On 06/12/17 23:21, Mark Cave-Ayland wrote:
> 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>
> ---
>  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 be5b04e..e1aa4fc 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);
>  }
> @@ -938,7 +945,6 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>      DeviceState *dev;
>      SysBusDevice *sbd;
>      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, sysbus_mmio_get_region(sbd, 1));
> -
> -        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;
>  }
>  
> 

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

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

[Laszlo Ersek]

On 06/12/17 23:21, 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.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/nvram/fw_cfg.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index e1aa4fc..6c21e43 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -920,8 +920,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);
> @@ -954,7 +952,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);
>      sysbus_add_io(sbd, iobase, sysbus_mmio_get_region(sbd, 0));
> @@ -991,7 +989,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);
> @@ -1097,6 +1095,8 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
>                                sizeof(dma_addr_t));
>          sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
>      }
> +
> +    fw_cfg_init1(dev);
>  }
>  
>  static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
> @@ -1163,6 +1163,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_init1(dev);
>  }
>  
>  static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
> 

This looks good to me generally, but I'm concerned about the part of
fw_cfg_init1() that precedes the original qdev_init_nofail() call, namely

    assert(!object_resolve_path(FW_CFG_PATH, NULL));

    object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s),
NULL);

The object_property_add_child() call creates a machine-global link to
the sole fw-cfg device, so that *other* code can find the fw-cfg device
by calling object_resolve_path(). (The same way that we assert fails
right before the creation, i.e. we don't try to create several fw_cfg
devices.)

I feel that this link creation does not belong in device realize
methods, but to board code. I feel that these two steps should be
factored out to a separate helper function, and then called from:

- fw_cfg_init_io_dma(), just before the new qdev_init_nofail() call site,
- fw_cfg_init_mem_wide(), just before the new qdev_init_nofail() call site,
- before any similar qdev_init_nofail() call sites, such as in
<https://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg02658.html>.

Again this is just a gut feeling, comments / opinions welcome.

Thanks,
Laszlo

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

[Laszlo Ersek]

On 06/12/17 23:21, 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         |    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 6c21e43..22a8404 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 */
> 

With the concern that I voiced under patch #3 (namely that board code
might be required to set up the machine-wide link to the sole fw-cfg
device manually, outside of realize):

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

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

[Laszlo Ersek]

On 06/13/17 00:27, Laszlo Ersek wrote:

> In fw_cfg_init_io_dma(), you know the object type exactly -- you just
> created it with TYPE_FW_CFG_IO --, so after device realization, you can
> cast the type as narrowly as necessary, and refer to the fields by name.

I understand that in sun4u code like
<https://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg02658.html>
you couldn't access those fields by name (such as "comb_iomem") immediately.

I can think of two ways out:

- Instead of accessing the field from the outside, for the board
mapping, pass the parent memory region in to the helper. (This is my
previous suggestion, and you didn't like it :) )

- or else, in patch v2 4/4, make the guts of FWCfgState / FWCfgIoState /
FWCfgMemState public too. Then board code can get at
"FWCfgIoState.comb_iomem" directly.

Thanks
Laszlo

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

[Mark Cave-Ayland]

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

> It turns out that we introduced the "iobase" and "dma_iobase" properties
> *solely* so that we could pass arguments to fw_cfg_io_realize(). But
> fw_cfg_io_realize() only needed those parameters for the *wrong* purpose
> (namely calling sysbus_add_io()). By eliminating the sysbus_add_io()
> calls from fw_cfg_io_realize(), the related properties become
> unnecessary too.
> 
> (NB, setting the "dma_enabled" property in fw_cfg_init_io_dma(), and
> setting "data_width" and "dma_enabled" in fw_cfg_init_mem_wide(), remain
> necessary, because those aren't related to region mapping.)
> 
> What do you think?

Yeah, I like the sound of this approach much better since it keeps the
functionality similar between both the memory and ioport instances. I've
got something basic going but I don't think I'm going to have time to
finish it tonight.


ATB,

Mark.

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

[Paolo Bonzini]

On 12/06/2017 23:21, 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 with sysbus_init_mmio() and defer
> the mapping to the caller, as already done in fw_cfg_init_mem_wide().

... sort of.

The idea is that the ISA bridge (including all the legacy I/O devices,
of which fw_cfg part) does subtractive decoding, i.e. "if nobody else
wants it, I'll take it".  So that's why fw_cfg's realize() maps I/O
ports, and why the API is sysbus_add_io.

Sysbus MMIO maps a different hardware concept, where the "base" is
decoded by the SoC and forwarded to the component at that address.  This
is represented by the sysbus_init_mmio/sysbus_mmio_map pair.

Documentation for this would be welcome, but sysbus.h doesn't have many
function comments. :(

Paolo

Re: [Qemu-devel] [PATCHv2 0/4] fw_cfg: qdev-related tidy-ups

[Paolo Bonzini]

On 12/06/2017 23:21, 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.

Could you use FW_CFG_MEM instead, and add a new function

void sysbus_mmio_map_subregion(SysBusDevice *dev, int n,
                               MemoryRegion *mr, hwaddr addr)

?

Thanks,

Paolo

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

[Mark Cave-Ayland]

On 12/06/17 23:50, Laszlo Ersek wrote:

> On 06/12/17 23:21, 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.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/nvram/fw_cfg.c |   10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index e1aa4fc..6c21e43 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -920,8 +920,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);
>> @@ -954,7 +952,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);
>>      sysbus_add_io(sbd, iobase, sysbus_mmio_get_region(sbd, 0));
>> @@ -991,7 +989,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);
>> @@ -1097,6 +1095,8 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
>>                                sizeof(dma_addr_t));
>>          sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
>>      }
>> +
>> +    fw_cfg_init1(dev);
>>  }
>>  
>>  static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
>> @@ -1163,6 +1163,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_init1(dev);
>>  }
>>  
>>  static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
>>
> 
> This looks good to me generally, but I'm concerned about the part of
> fw_cfg_init1() that precedes the original qdev_init_nofail() call, namely
> 
>     assert(!object_resolve_path(FW_CFG_PATH, NULL));
> 
>     object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s),
> NULL);
> 
> The object_property_add_child() call creates a machine-global link to
> the sole fw-cfg device, so that *other* code can find the fw-cfg device
> by calling object_resolve_path(). (The same way that we assert fails
> right before the creation, i.e. we don't try to create several fw_cfg
> devices.)
> 
> I feel that this link creation does not belong in device realize
> methods, but to board code. I feel that these two steps should be
> factored out to a separate helper function, and then called from:
> 
> - fw_cfg_init_io_dma(), just before the new qdev_init_nofail() call site,
> - fw_cfg_init_mem_wide(), just before the new qdev_init_nofail() call site,
> - before any similar qdev_init_nofail() call sites, such as in
> <https://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg02658.html>.
> 
> Again this is just a gut feeling, comments / opinions welcome.

After testing the assert() in a few different scenarios, I much prefer
the existing approach with a fixed fw_cfg path at /machine/fw_cfg.

The reason for this is that with existing code, any attempt to init a
second fw_cfg device will assert immediately, whereas if the board is
responsible for wiring up the fw_cfg device then it's possible that a
caller will either a) forget to call a helper function or b) wire up 2
fw_cfg in different places in the object tree, e.g. one calling
fw_cfg_init_io() and another instantiated directly via QOM as per what
I'm trying to do with sun4u.

Taking all this into account, I'm working on a v3 patchset that I hope
to post to the list later today.


ATB,

Mark.

Re: [Qemu-devel] [PATCHv2 0/4] fw_cfg: qdev-related tidy-ups

[Mark Cave-Ayland]

On 14/06/17 13:38, Paolo Bonzini wrote:

> On 12/06/2017 23:21, 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.
> 
> Could you use FW_CFG_MEM instead, and add a new function
> 
> void sysbus_mmio_map_subregion(SysBusDevice *dev, int n,
>                                MemoryRegion *mr, hwaddr addr)
> 
> ?

Hmmm possibly that could work with some wrappers to emulate the ioport
accesses, however Laszlo wasn't too keen on the ioport regions appearing
as MMIO in the device tree and I believe that the final patchset will
offer some useful cleanups. There's also the argument that outside of
OpenBIOS I don't have a large set of clients for testing so I'd much
prefer to leave access-related changes if at all possible.


ATB,

Mark.