[Qemu-devel] [PATCHv9 0/3] fw_cfg: qdev-related tidy-ups

[Qemu-devel] [PATCHv9 0/3] 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>
Depends-on: https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg01994.html <20170707213052.13087-1-ehabkost@redhat.com>

v9:
- Rebase onto master
- Reduce comment verbosity for fw_cfg_find()
- Remove local_err parameter from fw_cfg_common_realize() callers
  (instead use errp directly)

v8:
- Rebase onto master
- Drop patches 1 and 2 since they have already been applied
- Drop patch 4 since fw_cfg_unattached_at_realize() isn't required (it was a
  bug in object_resolve_path_type())
- Add comment on the return value of fw_cfg_find()
- Add Reviewed-By from Igor

v7:
- Remove instance_init() function with assert()
- Switch fw_cfg_find() over to use object_resolve_path_type() which removes the
  need for the fw_cfg device to exist at a fixed QOM path
- Switch check for existence of another fw_cfg device over to use the new
  fw_cfg_find()
- Add check for fw_cfg parent at realize time

v6:
- Revert move of FWCfgEntry from fw_cfg.c to fw_cfg.h from v5
- Add Reviewed-by tag from Laszlo for patch 5
- Add Tested-by tags from Laszlo for the series

v5:
- Remove unused FWCfgIoState iobase and dma_iobase fields
- Add Reviewed-By tags from Laszlo
- Update commit message in patch 5 as suggested by Laszlo
- Move FWCfgEntry typedef from fw_cfg.h to typedefs.h with the others

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

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

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


Mark Cave-Ayland (3):
  fw_cfg: switch fw_cfg_find() to locate the fw_cfg device by type
    rather than path
  fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers
  fw_cfg: move QOM type defines and fw_cfg types into fw_cfg.h

 hw/nvram/fw_cfg.c         |   79 +++++++++++----------------------------------
 include/hw/nvram/fw_cfg.h |   50 ++++++++++++++++++++++++++++
 include/qemu/typedefs.h   |    1 +
 3 files changed, 70 insertions(+), 60 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCHv9 1/3] fw_cfg: switch fw_cfg_find() to locate the fw_cfg device by type rather than path

[Mark Cave-Ayland]

This will enable the fw_cfg device to be placed anywhere within the QOM tree
regardless of its machine location.

Note that we also add a comment to document the behaviour that we return NULL to
indicate failure where either no fw_cfg device or multiple fw_cfg devices are
found.

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

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 99bdbc2..ac52b50 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -1017,7 +1017,8 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr)
 
 FWCfgState *fw_cfg_find(void)
 {
-    return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
+    /* Returns NULL unless there is exactly one fw_cfg device */
+    return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
 }
 
 static void fw_cfg_class_init(ObjectClass *klass, void *data)
-- 
1.7.10.4

[Qemu-devel] [PATCHv9 2/3] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers

[Mark Cave-Ayland]

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

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

Since it is now the responsibility of the machine to wire up the fw_cfg device
it is necessary to introduce a object_property_add_child() call into
fw_cfg_init_io() and fw_cfg_init_mem() to link the fw_cfg device to the root
machine object as before.

Finally with the previous change to fw_cfg_find() we can now remove the
assert() preventing multiple fw_cfg devices being instantiated and replace
them with a simple call to fw_cfg_find() at realize time instead. This allows
us to remove FW_CFG_NAME and FW_CFG_PATH since they are no longer required.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/nvram/fw_cfg.c |   27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index ac52b50..b850ac3 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -37,9 +37,6 @@
 
 #define FW_CFG_FILE_SLOTS_DFLT 0x20
 
-#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"
@@ -909,17 +906,16 @@ 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, Error **errp)
 {
     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);
-
-    qdev_init_nofail(dev);
+    if (!fw_cfg_find()) {
+        error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG);
+        return;
+    }
 
     fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
     fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16);
@@ -952,7 +948,9 @@ 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);
+    object_property_add_child(OBJECT(qdev_get_machine()), TYPE_FW_CFG,
+                              OBJECT(dev), NULL);
+    qdev_init_nofail(dev);
 
     sbd = SYS_BUS_DEVICE(dev);
     ios = FW_CFG_IO(dev);
@@ -990,7 +988,9 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
         qdev_prop_set_bit(dev, "dma_enabled", false);
     }
 
-    fw_cfg_init1(dev);
+    object_property_add_child(OBJECT(qdev_get_machine()), TYPE_FW_CFG,
+                              OBJECT(dev), NULL);
+    qdev_init_nofail(dev);
 
     sbd = SYS_BUS_DEVICE(dev);
     sysbus_mmio_map(sbd, 0, ctl_addr);
@@ -1021,6 +1021,7 @@ FWCfgState *fw_cfg_find(void)
     return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
 }
 
+
 static void fw_cfg_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -1092,6 +1093,8 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
                               &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
                               sizeof(dma_addr_t));
     }
+
+    fw_cfg_common_realize(dev, errp);
 }
 
 static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
@@ -1158,6 +1161,8 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
                               sizeof(dma_addr_t));
         sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
     }
+
+    fw_cfg_common_realize(dev, errp);
 }
 
 static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
-- 
1.7.10.4

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

[Mark Cave-Ayland]

By exposing FWCfgIoState and FWCfgMemState internals we allow the possibility
for the internal MemoryRegion fields to be mapped by name for boards that wish
to wire up the fw_cfg device themselves.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/nvram/fw_cfg.c         |   49 +-------------------------------------------
 include/hw/nvram/fw_cfg.h |   50 +++++++++++++++++++++++++++++++++++++++++++++
 include/qemu/typedefs.h   |    1 +
 3 files changed, 52 insertions(+), 48 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index b850ac3..2375f21 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -37,14 +37,6 @@
 
 #define FW_CFG_FILE_SLOTS_DFLT 0x20
 
-#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
@@ -58,51 +50,12 @@
 
 #define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */
 
-typedef struct FWCfgEntry {
+struct FWCfgEntry {
     uint32_t len;
     bool allow_write;
     uint8_t *data;
     void *callback_opaque;
     FWCfgReadCallback read_callback;
-} FWCfgEntry;
-
-struct FWCfgState {
-    /*< private >*/
-    SysBusDevice parent_obj;
-    /*< public >*/
-
-    uint16_t file_slots;
-    FWCfgEntry *entries[2];
-    int *entry_order;
-    FWCfgFiles *files;
-    uint16_t cur_entry;
-    uint32_t cur_offset;
-    Notifier machine_ready;
-
-    int fw_cfg_order_override;
-
-    bool dma_enabled;
-    dma_addr_t dma_addr;
-    AddressSpace *dma_as;
-    MemoryRegion dma_iomem;
-};
-
-struct FWCfgIoState {
-    /*< private >*/
-    FWCfgState parent_obj;
-    /*< public >*/
-
-    MemoryRegion comb_iomem;
-};
-
-struct FWCfgMemState {
-    /*< private >*/
-    FWCfgState parent_obj;
-    /*< public >*/
-
-    MemoryRegion ctl_iomem, data_iomem;
-    uint32_t data_width;
-    MemoryRegionOps wide_data_ops;
 };
 
 #define JPG_FILE 0
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index b980cba..b77ea48 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -1,8 +1,19 @@
 #ifndef FW_CFG_H
 #define FW_CFG_H
 
+#include "qemu/typedefs.h"
 #include "exec/hwaddr.h"
 #include "hw/nvram/fw_cfg_keys.h"
+#include "hw/sysbus.h"
+#include "sysemu/dma.h"
+
+#define TYPE_FW_CFG     "fw_cfg"
+#define TYPE_FW_CFG_IO  "fw_cfg_io"
+#define TYPE_FW_CFG_MEM "fw_cfg_mem"
+
+#define FW_CFG(obj)     OBJECT_CHECK(FWCfgState,    (obj), TYPE_FW_CFG)
+#define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
+#define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
 
 typedef struct FWCfgFile {
     uint32_t  size;        /* file size */
@@ -35,6 +46,45 @@ typedef struct FWCfgDmaAccess {
 
 typedef void (*FWCfgReadCallback)(void *opaque);
 
+struct FWCfgState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
+    uint16_t file_slots;
+    FWCfgEntry *entries[2];
+    int *entry_order;
+    FWCfgFiles *files;
+    uint16_t cur_entry;
+    uint32_t cur_offset;
+    Notifier machine_ready;
+
+    int fw_cfg_order_override;
+
+    bool dma_enabled;
+    dma_addr_t dma_addr;
+    AddressSpace *dma_as;
+    MemoryRegion dma_iomem;
+};
+
+struct FWCfgIoState {
+    /*< private >*/
+    FWCfgState parent_obj;
+    /*< public >*/
+
+    MemoryRegion comb_iomem;
+};
+
+struct FWCfgMemState {
+    /*< private >*/
+    FWCfgState parent_obj;
+    /*< public >*/
+
+    MemoryRegion ctl_iomem, data_iomem;
+    uint32_t data_width;
+    MemoryRegionOps wide_data_ops;
+};
+
 /**
  * fw_cfg_add_bytes:
  * @s: fw_cfg device being modified
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 2706aab..0a23020 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -30,6 +30,7 @@ typedef struct DisplaySurface DisplaySurface;
 typedef struct DriveInfo DriveInfo;
 typedef struct Error Error;
 typedef struct EventNotifier EventNotifier;
+typedef struct FWCfgEntry FWCfgEntry;
 typedef struct FWCfgIoState FWCfgIoState;
 typedef struct FWCfgMemState FWCfgMemState;
 typedef struct FWCfgState FWCfgState;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCHv9 1/3] fw_cfg: switch fw_cfg_find() to locate the fw_cfg device by type rather than path

[Igor Mammedov]

On Fri, 14 Jul 2017 10:40:06 +0100
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:

> This will enable the fw_cfg device to be placed anywhere within the QOM tree
> regardless of its machine location.
> 
> Note that we also add a comment to document the behaviour that we return NULL to
> indicate failure where either no fw_cfg device or multiple fw_cfg devices are
> found.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/nvram/fw_cfg.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 99bdbc2..ac52b50 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -1017,7 +1017,8 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr)
>  
>  FWCfgState *fw_cfg_find(void)
>  {
> -    return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
> +    /* Returns NULL unless there is exactly one fw_cfg device */
> +    return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
>  }
>  
>  static void fw_cfg_class_init(ObjectClass *klass, void *data)

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

[Eduardo Habkost]

On Fri, Jul 14, 2017 at 10:40:07AM +0100, Mark Cave-Ayland wrote:
> When looking to instantiate a TYPE_FW_CFG_MEM or TYPE_FW_CFG_IO device to be
> able to wire it up differently, it is much more convenient for the caller to
> instantiate the device and have the fw_cfg default files already preloaded
> during realize.
> 
> Move fw_cfg_init1() to the end of both the fw_cfg_mem_realize() and
> fw_cfg_io_realize() functions so it no longer needs to be called manually
> when instantiating the device, and also rename it to fw_cfg_common_realize()
> which better describes its new purpose.
> 
> Since it is now the responsibility of the machine to wire up the fw_cfg device
> it is necessary to introduce a object_property_add_child() call into
> fw_cfg_init_io() and fw_cfg_init_mem() to link the fw_cfg device to the root
> machine object as before.
> 
> Finally with the previous change to fw_cfg_find() we can now remove the
> assert() preventing multiple fw_cfg devices being instantiated and replace
> them with a simple call to fw_cfg_find() at realize time instead. This allows
> us to remove FW_CFG_NAME and FW_CFG_PATH since they are no longer required.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [PATCHv9 1/3] fw_cfg: switch fw_cfg_find() to locate the fw_cfg device by type rather than path

[Eduardo Habkost]

On Fri, Jul 14, 2017 at 10:40:06AM +0100, Mark Cave-Ayland wrote:
> This will enable the fw_cfg device to be placed anywhere within the QOM tree
> regardless of its machine location.
> 
> Note that we also add a comment to document the behaviour that we return NULL to
> indicate failure where either no fw_cfg device or multiple fw_cfg devices are
> found.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Jul 14, 2017 at 10:40:08AM +0100, Mark Cave-Ayland wrote:
> By exposing FWCfgIoState and FWCfgMemState internals we allow the possibility
> for the internal MemoryRegion fields to be mapped by name for boards that wish
> to wire up the fw_cfg device themselves.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
[...]
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index b980cba..b77ea48 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -1,8 +1,19 @@
>  #ifndef FW_CFG_H
>  #define FW_CFG_H
>  
> +#include "qemu/typedefs.h"
>  #include "exec/hwaddr.h"
>  #include "hw/nvram/fw_cfg_keys.h"
> +#include "hw/sysbus.h"
> +#include "sysemu/dma.h"
> +
> +#define TYPE_FW_CFG     "fw_cfg"
> +#define TYPE_FW_CFG_IO  "fw_cfg_io"
> +#define TYPE_FW_CFG_MEM "fw_cfg_mem"
> +
> +#define FW_CFG(obj)     OBJECT_CHECK(FWCfgState,    (obj), TYPE_FW_CFG)
> +#define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
> +#define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
>  
>  typedef struct FWCfgFile {
>      uint32_t  size;        /* file size */
> @@ -35,6 +46,45 @@ typedef struct FWCfgDmaAccess {
>  
>  typedef void (*FWCfgReadCallback)(void *opaque);
>  
> +struct FWCfgState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +    /*< public >*/
> +
> +    uint16_t file_slots;
> +    FWCfgEntry *entries[2];
> +    int *entry_order;
> +    FWCfgFiles *files;
> +    uint16_t cur_entry;
> +    uint32_t cur_offset;
> +    Notifier machine_ready;
> +
> +    int fw_cfg_order_override;
> +
> +    bool dma_enabled;
> +    dma_addr_t dma_addr;
> +    AddressSpace *dma_as;
> +    MemoryRegion dma_iomem;
> +};
> +
> +struct FWCfgIoState {
> +    /*< private >*/
> +    FWCfgState parent_obj;
> +    /*< public >*/
> +
> +    MemoryRegion comb_iomem;
> +};
> +
> +struct FWCfgMemState {
> +    /*< private >*/
> +    FWCfgState parent_obj;
> +    /*< public >*/
> +
> +    MemoryRegion ctl_iomem, data_iomem;
> +    uint32_t data_width;
> +    MemoryRegionOps wide_data_ops;
> +};

Why do you need the full struct declaration to be exposed in the
header?  The memory regions are supposed to be visible as QOM
children to the fw_cfg device, already.

-- 
Eduardo

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

[Laszlo Ersek]

On 07/14/17 20:09, Eduardo Habkost wrote:
> On Fri, Jul 14, 2017 at 10:40:08AM +0100, Mark Cave-Ayland wrote:
>> By exposing FWCfgIoState and FWCfgMemState internals we allow the possibility
>> for the internal MemoryRegion fields to be mapped by name for boards that wish
>> to wire up the fw_cfg device themselves.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> [...]
>> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
>> index b980cba..b77ea48 100644
>> --- a/include/hw/nvram/fw_cfg.h
>> +++ b/include/hw/nvram/fw_cfg.h
>> @@ -1,8 +1,19 @@
>>  #ifndef FW_CFG_H
>>  #define FW_CFG_H
>>  
>> +#include "qemu/typedefs.h"
>>  #include "exec/hwaddr.h"
>>  #include "hw/nvram/fw_cfg_keys.h"
>> +#include "hw/sysbus.h"
>> +#include "sysemu/dma.h"
>> +
>> +#define TYPE_FW_CFG     "fw_cfg"
>> +#define TYPE_FW_CFG_IO  "fw_cfg_io"
>> +#define TYPE_FW_CFG_MEM "fw_cfg_mem"
>> +
>> +#define FW_CFG(obj)     OBJECT_CHECK(FWCfgState,    (obj), TYPE_FW_CFG)
>> +#define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
>> +#define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
>>  
>>  typedef struct FWCfgFile {
>>      uint32_t  size;        /* file size */
>> @@ -35,6 +46,45 @@ typedef struct FWCfgDmaAccess {
>>  
>>  typedef void (*FWCfgReadCallback)(void *opaque);
>>  
>> +struct FWCfgState {
>> +    /*< private >*/
>> +    SysBusDevice parent_obj;
>> +    /*< public >*/
>> +
>> +    uint16_t file_slots;
>> +    FWCfgEntry *entries[2];
>> +    int *entry_order;
>> +    FWCfgFiles *files;
>> +    uint16_t cur_entry;
>> +    uint32_t cur_offset;
>> +    Notifier machine_ready;
>> +
>> +    int fw_cfg_order_override;
>> +
>> +    bool dma_enabled;
>> +    dma_addr_t dma_addr;
>> +    AddressSpace *dma_as;
>> +    MemoryRegion dma_iomem;
>> +};
>> +
>> +struct FWCfgIoState {
>> +    /*< private >*/
>> +    FWCfgState parent_obj;
>> +    /*< public >*/
>> +
>> +    MemoryRegion comb_iomem;
>> +};
>> +
>> +struct FWCfgMemState {
>> +    /*< private >*/
>> +    FWCfgState parent_obj;
>> +    /*< public >*/
>> +
>> +    MemoryRegion ctl_iomem, data_iomem;
>> +    uint32_t data_width;
>> +    MemoryRegionOps wide_data_ops;
>> +};
> 
> Why do you need the full struct declaration to be exposed in the
> header?

Different board code wants to hook up "comb_iomem" manually to different
address spaces, so they need to access the field directly. This is the
ultimate goal of the entire exercise, IIRC.

> The memory regions are supposed to be visible as QOM
> children to the fw_cfg device, already.

I don't understand this. How else can board code work with "comb_iomem"
than described above? If there is a way, I agree it would be preferable.

Thanks
Laszlo

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

[Eduardo Habkost]

On Fri, Jul 14, 2017 at 08:28:37PM +0200, Laszlo Ersek wrote:
> On 07/14/17 20:09, Eduardo Habkost wrote:
> > On Fri, Jul 14, 2017 at 10:40:08AM +0100, Mark Cave-Ayland wrote:
> >> By exposing FWCfgIoState and FWCfgMemState internals we allow the possibility
> >> for the internal MemoryRegion fields to be mapped by name for boards that wish
> >> to wire up the fw_cfg device themselves.
> >>
> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > [...]
> >> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> >> index b980cba..b77ea48 100644
> >> --- a/include/hw/nvram/fw_cfg.h
> >> +++ b/include/hw/nvram/fw_cfg.h
> >> @@ -1,8 +1,19 @@
> >>  #ifndef FW_CFG_H
> >>  #define FW_CFG_H
> >>  
> >> +#include "qemu/typedefs.h"
> >>  #include "exec/hwaddr.h"
> >>  #include "hw/nvram/fw_cfg_keys.h"
> >> +#include "hw/sysbus.h"
> >> +#include "sysemu/dma.h"
> >> +
> >> +#define TYPE_FW_CFG     "fw_cfg"
> >> +#define TYPE_FW_CFG_IO  "fw_cfg_io"
> >> +#define TYPE_FW_CFG_MEM "fw_cfg_mem"
> >> +
> >> +#define FW_CFG(obj)     OBJECT_CHECK(FWCfgState,    (obj), TYPE_FW_CFG)
> >> +#define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
> >> +#define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
> >>  
> >>  typedef struct FWCfgFile {
> >>      uint32_t  size;        /* file size */
> >> @@ -35,6 +46,45 @@ typedef struct FWCfgDmaAccess {
> >>  
> >>  typedef void (*FWCfgReadCallback)(void *opaque);
> >>  
> >> +struct FWCfgState {
> >> +    /*< private >*/
> >> +    SysBusDevice parent_obj;
> >> +    /*< public >*/
> >> +
> >> +    uint16_t file_slots;
> >> +    FWCfgEntry *entries[2];
> >> +    int *entry_order;
> >> +    FWCfgFiles *files;
> >> +    uint16_t cur_entry;
> >> +    uint32_t cur_offset;
> >> +    Notifier machine_ready;
> >> +
> >> +    int fw_cfg_order_override;
> >> +
> >> +    bool dma_enabled;
> >> +    dma_addr_t dma_addr;
> >> +    AddressSpace *dma_as;
> >> +    MemoryRegion dma_iomem;
> >> +};
> >> +
> >> +struct FWCfgIoState {
> >> +    /*< private >*/
> >> +    FWCfgState parent_obj;
> >> +    /*< public >*/
> >> +
> >> +    MemoryRegion comb_iomem;
> >> +};
> >> +
> >> +struct FWCfgMemState {
> >> +    /*< private >*/
> >> +    FWCfgState parent_obj;
> >> +    /*< public >*/
> >> +
> >> +    MemoryRegion ctl_iomem, data_iomem;
> >> +    uint32_t data_width;
> >> +    MemoryRegionOps wide_data_ops;
> >> +};
> > 
> > Why do you need the full struct declaration to be exposed in the
> > header?
> 
> Different board code wants to hook up "comb_iomem" manually to different
> address spaces, so they need to access the field directly. This is the
> ultimate goal of the entire exercise, IIRC.
> 
> > The memory regions are supposed to be visible as QOM
> > children to the fw_cfg device, already.
> 
> I don't understand this. How else can board code work with "comb_iomem"
> than described above? If there is a way, I agree it would be preferable.

object_resolve_path_component(fw_cfg, "fwcfg[0]") and
object_resolve_path_component(fw_cfg, "fwcfg.dma[0]") should
return fw_cfg->comb_iomem and fw_cfg->dma_iomem, respectively.

I don't know why those names were chosen, though.  Probably it's
a good idea to call object_property_add_child() manually with
more appropriate names inside the fw_cfg code instead of letting
memory_region_init() pick the child name.

-- 
Eduardo

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

[Mark Cave-Ayland]

On 14/07/17 19:56, Eduardo Habkost wrote:

>>> Why do you need the full struct declaration to be exposed in the
>>> header?
>>
>> Different board code wants to hook up "comb_iomem" manually to different
>> address spaces, so they need to access the field directly. This is the
>> ultimate goal of the entire exercise, IIRC.

Yes, that is correct (and I believe is mentioned in the cover letter, too).

>>> The memory regions are supposed to be visible as QOM
>>> children to the fw_cfg device, already.
>>
>> I don't understand this. How else can board code work with "comb_iomem"
>> than described above? If there is a way, I agree it would be preferable.
> 
> object_resolve_path_component(fw_cfg, "fwcfg[0]") and
> object_resolve_path_component(fw_cfg, "fwcfg.dma[0]") should
> return fw_cfg->comb_iomem and fw_cfg->dma_iomem, respectively.
> 
> I don't know why those names were chosen, though.  Probably it's
> a good idea to call object_property_add_child() manually with
> more appropriate names inside the fw_cfg code instead of letting
> memory_region_init() pick the child name.

That's interesting. I did a grep of the codebase for
object_resolve_path_component and struggled to find an instance where it
was being used to provide access to a MemoryRegion. Even if it's an
available feature, it's certainly not one that is widely known about.

In terms of the patch, is the v9 revision suitable for commit or does
this constitute a NACK?


ATB,

Mark.

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

[Igor Mammedov]

On Sun, 16 Jul 2017 20:12:13 +0100
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:

> On 14/07/17 19:56, Eduardo Habkost wrote:
> 
> >>> Why do you need the full struct declaration to be exposed in the
> >>> header?  
> >>
> >> Different board code wants to hook up "comb_iomem" manually to different
> >> address spaces, so they need to access the field directly. This is the
> >> ultimate goal of the entire exercise, IIRC.  
> 
> Yes, that is correct (and I believe is mentioned in the cover letter, too).
> 
> >>> The memory regions are supposed to be visible as QOM
> >>> children to the fw_cfg device, already.  
> >>
> >> I don't understand this. How else can board code work with "comb_iomem"
> >> than described above? If there is a way, I agree it would be preferable.  
> > 
> > object_resolve_path_component(fw_cfg, "fwcfg[0]") and
> > object_resolve_path_component(fw_cfg, "fwcfg.dma[0]") should
> > return fw_cfg->comb_iomem and fw_cfg->dma_iomem, respectively.
> > 
> > I don't know why those names were chosen, though.  Probably it's
> > a good idea to call object_property_add_child() manually with
> > more appropriate names inside the fw_cfg code instead of letting
> > memory_region_init() pick the child name.  
> 
> That's interesting. I did a grep of the codebase for
> object_resolve_path_component and struggled to find an instance where it
> was being used to provide access to a MemoryRegion. Even if it's an
> available feature, it's certainly not one that is widely known about.
Agreed,

we haven't used suggested approach before and I don't see
benefits it will bring in fwcfg case.

Eduardo,

Could you just apply v9, which seems to be good enough and
does the intended job before 2.10 goes into soft-freeze?
we always could do object_resolve_path_component() on top
if it makes sense.


> 
> In terms of the patch, is the v9 revision suitable for commit or does
> this constitute a NACK?
> 
> 
> ATB,
> 
> Mark.

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

[Eduardo Habkost]

On Sun, Jul 16, 2017 at 08:12:13PM +0100, Mark Cave-Ayland wrote:
> On 14/07/17 19:56, Eduardo Habkost wrote:
> 
> >>> Why do you need the full struct declaration to be exposed in the
> >>> header?
> >>
> >> Different board code wants to hook up "comb_iomem" manually to different
> >> address spaces, so they need to access the field directly. This is the
> >> ultimate goal of the entire exercise, IIRC.
> 
> Yes, that is correct (and I believe is mentioned in the cover letter, too).
> 
> >>> The memory regions are supposed to be visible as QOM
> >>> children to the fw_cfg device, already.
> >>
> >> I don't understand this. How else can board code work with "comb_iomem"
> >> than described above? If there is a way, I agree it would be preferable.
> > 
> > object_resolve_path_component(fw_cfg, "fwcfg[0]") and
> > object_resolve_path_component(fw_cfg, "fwcfg.dma[0]") should
> > return fw_cfg->comb_iomem and fw_cfg->dma_iomem, respectively.
> > 
> > I don't know why those names were chosen, though.  Probably it's
> > a good idea to call object_property_add_child() manually with
> > more appropriate names inside the fw_cfg code instead of letting
> > memory_region_init() pick the child name.
> 
> That's interesting. I did a grep of the codebase for
> object_resolve_path_component and struggled to find an instance where it
> was being used to provide access to a MemoryRegion. Even if it's an
> available feature, it's certainly not one that is widely known about.
> 
> In terms of the patch, is the v9 revision suitable for commit or does
> this constitute a NACK?

It's not a NACK.  Using QOM properties instead of direct struct
access may be nice (I'm not 100% sure, either), but not required.

-- 
Eduardo

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

[Eduardo Habkost]

On Mon, Jul 17, 2017 at 01:03:11PM +0200, Igor Mammedov wrote:
> On Sun, 16 Jul 2017 20:12:13 +0100
> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> 
> > On 14/07/17 19:56, Eduardo Habkost wrote:
> > 
> > >>> Why do you need the full struct declaration to be exposed in the
> > >>> header?  
> > >>
> > >> Different board code wants to hook up "comb_iomem" manually to different
> > >> address spaces, so they need to access the field directly. This is the
> > >> ultimate goal of the entire exercise, IIRC.  
> > 
> > Yes, that is correct (and I believe is mentioned in the cover letter, too).
> > 
> > >>> The memory regions are supposed to be visible as QOM
> > >>> children to the fw_cfg device, already.  
> > >>
> > >> I don't understand this. How else can board code work with "comb_iomem"
> > >> than described above? If there is a way, I agree it would be preferable.  
> > > 
> > > object_resolve_path_component(fw_cfg, "fwcfg[0]") and
> > > object_resolve_path_component(fw_cfg, "fwcfg.dma[0]") should
> > > return fw_cfg->comb_iomem and fw_cfg->dma_iomem, respectively.
> > > 
> > > I don't know why those names were chosen, though.  Probably it's
> > > a good idea to call object_property_add_child() manually with
> > > more appropriate names inside the fw_cfg code instead of letting
> > > memory_region_init() pick the child name.  
> > 
> > That's interesting. I did a grep of the codebase for
> > object_resolve_path_component and struggled to find an instance where it
> > was being used to provide access to a MemoryRegion. Even if it's an
> > available feature, it's certainly not one that is widely known about.
> Agreed,
> 
> we haven't used suggested approach before and I don't see
> benefits it will bring in fwcfg case.
> 
> Eduardo,
> 
> Could you just apply v9, which seems to be good enough and
> does the intended job before 2.10 goes into soft-freeze?
> we always could do object_resolve_path_component() on top
> if it makes sense.

I was hoping Michael would apply it (as fw_cfg is currently
closer to PC code than to i386 or machine core).  But I can apply
it if he didn't queue it yet.

-- 
Eduardo