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

[Qemu-devel] [PATCHv7 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>

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 (6):
  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: switch fw_cfg_find() to locate the fw_cfg device by type
    rather than path
  fw_cfg: add assert() to ensure the fw_cfg device has been added as a
    child property
  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         |  133 +++++++++++++++++++--------------------------
 include/hw/nvram/fw_cfg.h |   50 +++++++++++++++++
 include/qemu/typedefs.h   |    1 +
 3 files changed, 108 insertions(+), 76 deletions(-)

-- 
1.7.10.4

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

[Mark Cave-Ayland]

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

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

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

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 316fca9..4e4f71a 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,30 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
                                 AddressSpace *dma_as)
 {
     DeviceState *dev;
+    SysBusDevice *sbd;
+    FWCfgIoState *ios;
     FWCfgState *s;
     uint32_t version = FW_CFG_VERSION;
     bool dma_requested = dma_iobase && dma_as;
 
     dev = qdev_create(NULL, TYPE_FW_CFG_IO);
-    qdev_prop_set_uint32(dev, "iobase", iobase);
-    qdev_prop_set_uint32(dev, "dma_iobase", dma_iobase);
     if (!dma_requested) {
         qdev_prop_set_bit(dev, "dma_enabled", false);
     }
 
     fw_cfg_init1(dev);
+
+    sbd = SYS_BUS_DEVICE(dev);
+    ios = FW_CFG_IO(dev);
+    sysbus_add_io(sbd, iobase, &ios->comb_iomem);
+
     s = FW_CFG(dev);
 
     if (s->dma_enabled) {
         /* 64 bits for the address field */
         s->dma_as = dma_as;
         s->dma_addr = 0;
+        sysbus_add_io(sbd, dma_iobase, &s->dma_iomem);
 
         version |= FW_CFG_VERSION_DMA;
     }
@@ -1059,8 +1064,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 +1074,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 +1087,11 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
      * of the i/o region used is FW_CFG_CTL_SIZE */
     memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
                           FW_CFG(s), "fwcfg", FW_CFG_CTL_SIZE);
-    sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
 
     if (FW_CFG(s)->dma_enabled) {
         memory_region_init_io(&FW_CFG(s)->dma_iomem, OBJECT(s),
                               &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
                               sizeof(dma_addr_t));
-        sysbus_add_io(sbd, s->dma_iobase, &FW_CFG(s)->dma_iomem);
     }
 }
 
-- 
1.7.10.4

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

[Mark Cave-Ayland]

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

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
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 4e4f71a..99bdbc2 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -913,6 +913,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));
 
@@ -927,6 +928,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,
     SysBusDevice *sbd;
     FWCfgIoState *ios;
     FWCfgState *s;
-    uint32_t version = FW_CFG_VERSION;
     bool dma_requested = dma_iobase && dma_as;
 
     dev = qdev_create(NULL, TYPE_FW_CFG_IO);
@@ -959,12 +965,8 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
         s->dma_as = dma_as;
         s->dma_addr = 0;
         sysbus_add_io(sbd, dma_iobase, &s->dma_iomem);
-
-        version |= FW_CFG_VERSION_DMA;
     }
 
-    fw_cfg_add_i32(s, FW_CFG_ID, version);
-
     return s;
 }
 
@@ -980,7 +982,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);
@@ -1001,11 +1002,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] [PATCHv7 3/6] 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.

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

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 99bdbc2..0fe7404 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -1017,7 +1017,7 @@ 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));
+    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] [PATCHv7 4/6] fw_cfg: add assert() to ensure the fw_cfg device has been added as a child property

[Mark Cave-Ayland]

This will currently always succeed until the check is moved from init to realize.

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

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 0fe7404..2291121 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -907,6 +907,17 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data)
     qemu_register_reset(fw_cfg_machine_reset, s);
 }
 
+static int fw_cfg_unattached_foreach(Object *obj, void *opaque)
+{
+    return (object_dynamic_cast(obj, TYPE_FW_CFG) != NULL);
+}
+
+static int fw_cfg_unattached_at_realize(void)
+{
+    Object *obj = container_get(qdev_get_machine(), "/unattached");
+
+    return object_child_foreach(obj, fw_cfg_unattached_foreach, NULL);
+}
 
 
 static void fw_cfg_init1(DeviceState *dev)
@@ -921,6 +932,8 @@ static void fw_cfg_init1(DeviceState *dev)
 
     qdev_init_nofail(dev);
 
+    assert(!fw_cfg_unattached_at_realize());
+
     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);
-- 
1.7.10.4

[Qemu-devel] [PATCHv7 5/6] 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 we can now convert the asserts() preventing multiple fw_cfg devices
and unparented fw_cfg devices being instantiated and replace them with proper
error reporting at realize time. 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>
---
 hw/nvram/fw_cfg.c |   41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 2291121..31029ac 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"
@@ -920,19 +917,22 @@ static int fw_cfg_unattached_at_realize(void)
 }
 
 
-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;
+    }
 
-    assert(!fw_cfg_unattached_at_realize());
+    if (fw_cfg_unattached_at_realize()) {
+        error_setg(errp, "%s device must be added as a child device before "
+                   "realize", 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);
@@ -965,7 +965,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);
@@ -1003,7 +1005,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);
@@ -1033,6 +1037,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);
@@ -1104,6 +1109,12 @@ 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, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 }
 
 static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
@@ -1170,6 +1181,12 @@ 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, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 }
 
 static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
-- 
1.7.10.4

[Qemu-devel] [PATCHv7 6/6] 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 31029ac..35bdb7a 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] [PATCHv7 0/6] fw_cfg: qdev-related tidy-ups

[Gabriel L. Somlo]

On Thu, Jun 29, 2017 at 03:07:14PM +0100, 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.

Tested-by: Gabriel Somlo <somlo@cmu.edu>

Specifically, verified that passing blobs into the guest via
"-fw-cfg name=opt/foo,file=./bar" still shows up properly in
'/sys/firmware/qemu_fw_cfg/by_name/opt/foo/raw' on the guest.

Thanks much,
--Gabriel
 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> 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 (6):
>   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: switch fw_cfg_find() to locate the fw_cfg device by type
>     rather than path
>   fw_cfg: add assert() to ensure the fw_cfg device has been added as a
>     child property
>   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         |  133 +++++++++++++++++++--------------------------
>  include/hw/nvram/fw_cfg.h |   50 +++++++++++++++++
>  include/qemu/typedefs.h   |    1 +
>  3 files changed, 108 insertions(+), 76 deletions(-)
> 
> -- 
> 1.7.10.4
> 

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

[Igor Mammedov]

On Thu, 29 Jun 2017 15:07:19 +0100
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> 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 we can now convert the asserts() preventing multiple fw_cfg devices
> and unparented fw_cfg devices being instantiated and replace them with proper
> error reporting at realize time. 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>
> ---
>  hw/nvram/fw_cfg.c |   41 +++++++++++++++++++++++++++++------------
>  1 file changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 2291121..31029ac 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"
> @@ -920,19 +917,22 @@ static int fw_cfg_unattached_at_realize(void)
>  }
>  
>  
> -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()) {
maybe add comment that here, that fw_cfg_find() will return NULL if more than
1 device is found.


> +        error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG);
s/TYPE_FW_CFG/object_get_typename()/
so it would print leaf type name 

> +        return;
> +    }
>  
> -    assert(!fw_cfg_unattached_at_realize());
> +    if (fw_cfg_unattached_at_realize()) {
as I pointed out in v6, this condition will always be false,
I suggest to drop 4/6 patch and this hunk here so it won't to confuse
readers with assumption that condition might succeed.


> +        error_setg(errp, "%s device must be added as a child device before "
> +                   "realize", 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);
> @@ -965,7 +965,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);
> @@ -1003,7 +1005,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);
> @@ -1033,6 +1037,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);
> @@ -1104,6 +1109,12 @@ 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, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  }
>  
>  static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
> @@ -1170,6 +1181,12 @@ 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, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  }
>  
>  static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
patch looks good, with above comments fixed:

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

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

[Igor Mammedov]

On Thu, 29 Jun 2017 15:07:17 +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.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/nvram/fw_cfg.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 99bdbc2..0fe7404 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -1017,7 +1017,7 @@ 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));
> +    return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
I insist on using ambiguous argument

see why it's needed
 https://www.mail-archive.com/qemu-devel@nongnu.org/msg460692.html

>  }
>  
>  static void fw_cfg_class_init(ObjectClass *klass, void *data)

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

[Mark Cave-Ayland]

On 03/07/17 10:39, Igor Mammedov wrote:

> On Thu, 29 Jun 2017 15:07:19 +0100
> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> 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 we can now convert the asserts() preventing multiple fw_cfg devices
>> and unparented fw_cfg devices being instantiated and replace them with proper
>> error reporting at realize time. 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>
>> ---
>>  hw/nvram/fw_cfg.c |   41 +++++++++++++++++++++++++++++------------
>>  1 file changed, 29 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 2291121..31029ac 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"
>> @@ -920,19 +917,22 @@ static int fw_cfg_unattached_at_realize(void)
>>  }
>>  
>>  
>> -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()) {
> maybe add comment that here, that fw_cfg_find() will return NULL if more than
> 1 device is found.

This should be the same behaviour as before, i.e. a NULL means that the
fw_cfg device couldn't be found. It seems intuitive to me from the name
of the function exactly what it does, so I don't find the lack of
comment too confusing. Does anyone else have any thoughts here?

> 
>> +        error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG);
> s/TYPE_FW_CFG/object_get_typename()/
> so it would print leaf type name 

Previously the code would add the device to the machine at FW_CFG_PATH
which was fixed at /machine/fw_cfg regardless of whether it was
fw_cfg_io or fw_cfg_mem type (see the top of fw_cfg.c).

This was a deliberate attempt to preserve the existing behaviour and if
we were to give the leaf type name I think this would be misleading,
since it implies you could have one fw_cfg_io and one fw_cfg_mem which
isn't correct.

> 
>> +        return;
>> +    }
>>  
>> -    assert(!fw_cfg_unattached_at_realize());
>> +    if (fw_cfg_unattached_at_realize()) {
> as I pointed out in v6, this condition will always be false,
> I suggest to drop 4/6 patch and this hunk here so it won't to confuse
> readers with assumption that condition might succeed.

Definitely look more closely at the fw_cfg_unattached_at_realize()
implementation in patch 4. You'll see that the check to determine if the
device is attached does not check obj->parent but checks to see if the
device exists under /machine/unattached which is what the
device_set_realised() does if the device doesn't have a parent.

I can confirm that I've given this a fairly good test with parented and
non-parented objects and it seems to work well here. Does it not work
for you in testing?

> 
>> +        error_setg(errp, "%s device must be added as a child device before "
>> +                   "realize", 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);
>> @@ -965,7 +965,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);
>> @@ -1003,7 +1005,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);
>> @@ -1033,6 +1037,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);
>> @@ -1104,6 +1109,12 @@ 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, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>  }
>>  
>>  static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
>> @@ -1170,6 +1181,12 @@ 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, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>  }
>>  
>>  static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
> patch looks good, with above comments fixed:

Great, thanks! I think there is some misunderstanding about the points
above so I'd be interested to hear your further comments.

> Reviewed-by: Igor Mammedov <imammedo@redhat.com>


ATB,

Mark.

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

[Mark Cave-Ayland]

On 03/07/17 10:42, Igor Mammedov wrote:
> On Thu, 29 Jun 2017 15:07:17 +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.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/nvram/fw_cfg.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 99bdbc2..0fe7404 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -1017,7 +1017,7 @@ 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));
>> +    return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
> I insist on using ambiguous argument
> 
> see why it's needed
>  https://www.mail-archive.com/qemu-devel@nongnu.org/msg460692.html

Yes I did take into account your previous comment about the ambiguous
argument, but with the fw_cfg_unattached_at_realize() check added in the
v7 patchset, it is actually impossible to realize more than one fw_cfg
device, and at realize time that device must have been linked as a child
device to a parent device.

Hence it should be impossible for the above situation where more than
one fw_cfg device can exist in the QOM tree to occur. Have I missed
something in the logic which would prevent this from happening?


ATB,

Mark.

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

[Igor Mammedov]

On Tue, 4 Jul 2017 19:08:44 +0100
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:

> On 03/07/17 10:39, Igor Mammedov wrote:
> 
> > On Thu, 29 Jun 2017 15:07:19 +0100
> > Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> 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 we can now convert the asserts() preventing multiple fw_cfg devices
> >> and unparented fw_cfg devices being instantiated and replace them with proper
> >> error reporting at realize time. 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>
> >> ---
> >>  hw/nvram/fw_cfg.c |   41 +++++++++++++++++++++++++++++------------
> >>  1 file changed, 29 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> >> index 2291121..31029ac 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"
> >> @@ -920,19 +917,22 @@ static int fw_cfg_unattached_at_realize(void)
> >>  }
> >>  
> >>  
> >> -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()) {  
> > maybe add comment that here, that fw_cfg_find() will return NULL if more than
> > 1 device is found.  
> 
> This should be the same behaviour as before, i.e. a NULL means that the
> fw_cfg device couldn't be found. It seems intuitive to me from the name
> of the function exactly what it does, so I don't find the lack of
> comment too confusing. Does anyone else have any thoughts here?
intuitive meaning from the function name would be return NULL if nothing were found,
however it's not the case here.

taking in account that fwcfg in not user creatable device how about:

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 316fca9..8f6eef8 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -1014,7 +1014,10 @@ 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));
+    bool ambig = false;
+    FWCfgState *f = FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, &ambig));
+    assert(!ambig);
+    return f;
 }

or if we must to print user friendly error and fail realize gracefully
(not sure why) just add errp argument to function so it could report
error back to caller, then places that do not care much about graceful
exit would use error_abort as argument and realize would use
its local_error/errp argument.

   
> >> +        error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG);  
> > s/TYPE_FW_CFG/object_get_typename()/
> > so it would print leaf type name   
> 
> Previously the code would add the device to the machine at FW_CFG_PATH
> which was fixed at /machine/fw_cfg regardless of whether it was
> fw_cfg_io or fw_cfg_mem type (see the top of fw_cfg.c).
> 
> This was a deliberate attempt to preserve the existing behaviour and if
> we were to give the leaf type name I think this would be misleading,
> since it implies you could have one fw_cfg_io and one fw_cfg_mem which
> isn't correct.
I don't have strong preference here, considering that it's
hardcoded in board (not user specified) device,
developer that stumbles upon error should be able to dig out which
concrete type caused it.
   
> >> +        return;
> >> +    }
> >>  
> >> -    assert(!fw_cfg_unattached_at_realize());
> >> +    if (fw_cfg_unattached_at_realize()) {  
> > as I pointed out in v6, this condition will always be false,
> > I suggest to drop 4/6 patch and this hunk here so it won't to confuse
> > readers with assumption that condition might succeed.  
> 
> Definitely look more closely at the fw_cfg_unattached_at_realize()
> implementation in patch 4. You'll see that the check to determine if the
> device is attached does not check obj->parent but checks to see if the
> device exists under /machine/unattached which is what the
> device_set_realised() does if the device doesn't have a parent.
looking more fw_cfg_unattached_at_realize(),
 returns true if fwcfg is direct child of/machine/unattached
meaning implicit parent assignment.

As result, above condition essentially forbids having fwcfg under
/machine/unattached and forces user to explicitly set parent
outside of /machine/unattached or be a child of some other device.

Is this your intent (why)?



> I can confirm that I've given this a fairly good test with parented and
> non-parented objects and it seems to work well here. Does it not work
> for you in testing?
> 
> >   
> >> +        error_setg(errp, "%s device must be added as a child device before "
> >> +                   "realize", 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);
> >> @@ -965,7 +965,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);
> >> @@ -1003,7 +1005,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);
> >> @@ -1033,6 +1037,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);
> >> @@ -1104,6 +1109,12 @@ 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, &local_err);
> >> +    if (local_err) {
> >> +        error_propagate(errp, local_err);
> >> +        return;
> >> +    }
> >>  }
> >>  
> >>  static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
> >> @@ -1170,6 +1181,12 @@ 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, &local_err);
> >> +    if (local_err) {
> >> +        error_propagate(errp, local_err);
> >> +        return;
> >> +    }
> >>  }
> >>  
> >>  static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)  
> > patch looks good, with above comments fixed:  
> 
> Great, thanks! I think there is some misunderstanding about the points
> above so I'd be interested to hear your further comments.
> 
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>  
> 
> 
> ATB,
> 
> Mark.
> 

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

[Mark Cave-Ayland]

On 07/07/17 12:33, Igor Mammedov wrote:

> On Tue, 4 Jul 2017 19:08:44 +0100
> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> 
>> On 03/07/17 10:39, Igor Mammedov wrote:
>>
>>> On Thu, 29 Jun 2017 15:07:19 +0100
>>> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> 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 we can now convert the asserts() preventing multiple fw_cfg devices
>>>> and unparented fw_cfg devices being instantiated and replace them with proper
>>>> error reporting at realize time. 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>
>>>> ---
>>>>  hw/nvram/fw_cfg.c |   41 +++++++++++++++++++++++++++++------------
>>>>  1 file changed, 29 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>>>> index 2291121..31029ac 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"
>>>> @@ -920,19 +917,22 @@ static int fw_cfg_unattached_at_realize(void)
>>>>  }
>>>>  
>>>>  
>>>> -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()) {  
>>> maybe add comment that here, that fw_cfg_find() will return NULL if more than
>>> 1 device is found.  
>>
>> This should be the same behaviour as before, i.e. a NULL means that the
>> fw_cfg device couldn't be found. It seems intuitive to me from the name
>> of the function exactly what it does, so I don't find the lack of
>> comment too confusing. Does anyone else have any thoughts here?
>
> intuitive meaning from the function name would be return NULL if nothing were found,
> however it's not the case here.

The function with its current name has always existed, so all I'm doing
here is reusing it as per your comment on an earlier patchset.

> taking in account that fwcfg in not user creatable device how about:
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 316fca9..8f6eef8 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -1014,7 +1014,10 @@ 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));
> +    bool ambig = false;
> +    FWCfgState *f = FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, &ambig));
> +    assert(!ambig);
> +    return f;
>  }
> 
> or if we must to print user friendly error and fail realize gracefully
> (not sure why) just add errp argument to function so it could report
> error back to caller, then places that do not care much about graceful
> exit would use error_abort as argument and realize would use
> its local_error/errp argument.

My understanding from the thread was that we should only use assert()s
where there is no other choice so that any failures can be handled in a
more friendly manner.

Now as Laszlo pointed out, fw_cfg_find() is used externally to locate
the fw_cfg device in other parts of the QEMU codebase. Yes I agree that
it is possible to change the way in which it returns, however I would
argue that changing those semantics are outside of the scope of this patch.

>    
>>>> +        error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG);  
>>> s/TYPE_FW_CFG/object_get_typename()/
>>> so it would print leaf type name   
>>
>> Previously the code would add the device to the machine at FW_CFG_PATH
>> which was fixed at /machine/fw_cfg regardless of whether it was
>> fw_cfg_io or fw_cfg_mem type (see the top of fw_cfg.c).
>>
>> This was a deliberate attempt to preserve the existing behaviour and if
>> we were to give the leaf type name I think this would be misleading,
>> since it implies you could have one fw_cfg_io and one fw_cfg_mem which
>> isn't correct.
> I don't have strong preference here, considering that it's
> hardcoded in board (not user specified) device,
> developer that stumbles upon error should be able to dig out which
> concrete type caused it.

Okay if no-one else comments then I'll leave it as-is in order to
preserve the existing behaviour as much as possible.

>>>> +        return;
>>>> +    }
>>>>  
>>>> -    assert(!fw_cfg_unattached_at_realize());
>>>> +    if (fw_cfg_unattached_at_realize()) {  
>>> as I pointed out in v6, this condition will always be false,
>>> I suggest to drop 4/6 patch and this hunk here so it won't to confuse
>>> readers with assumption that condition might succeed.  
>>
>> Definitely look more closely at the fw_cfg_unattached_at_realize()
>> implementation in patch 4. You'll see that the check to determine if the
>> device is attached does not check obj->parent but checks to see if the
>> device exists under /machine/unattached which is what the
>> device_set_realised() does if the device doesn't have a parent.
>
> looking more fw_cfg_unattached_at_realize(),
>  returns true if fwcfg is direct child of/machine/unattached
> meaning implicit parent assignment.
> 
> As result, above condition essentially forbids having fwcfg under
> /machine/unattached and forces user to explicitly set parent
> outside of /machine/unattached or be a child of some other device.
> 
> Is this your intent (why)?

Yes that is entirely correct. All current fw_cfg users setup the device
using fw_cfg_init_io() and fw_cfg_init_mem() which is fine for those
cases because these functions attach the fw_cfg device directly to the
machine at /machine/fw_cfg. This makes it trivial to determine whether
or not an existing fw_cfg has been instantiated and prevent any more
instances, which Laszlo has stated is an underlying assumption for
fw_cfg_find().

In my particular use case for SPARC64, I need to move the fw_cfg device
behind a PCI bridge. Therefore in order to allow the QOM tree to reflect
the actual hardware DT then the fw_cfg device needs to be attached to
the PCI bridge and not the machine. Hence the check for an existing
device at /machine/fw_cfg is no longer good enough to determine if a
fw_cfg device already exists since if they do, they can be in several
different locations in the QOM tree.

This explains the change to fw_cfg_find() to make sure that we find any
other fw_cfg instances, no matter where they are in the QOM tree.

However this causes us a problem: if you instantiate the fw_cfg yourself
with qdev_create()...qdev_init_nofail() then you can potentially access
the underlying MemoryRegions directly and wire up the device without
attaching it to the QOM tree. This is an invalid configuration but
wouldn't be detected with fw_cfg_find().

The correct way to handle this is to wire up the device yourself with
object_property_add_child() but then you find the situation whereby the
people who know that you have to call object_property_add_child() when
adding the fw_cfg device are the ones who don't need the error message.

Therefore the solution was to enforce that the fw_cfg device has been
added to the QOM tree before realize because it solves all the problems:

1) The fw_cfg device can now be placed anywhere in the QOM tree, meaning
that the QOM tree can be a topologically correct representation of the
machine

2) By enforcing that the fw_cfg device has been parented, we guarantee
that the fw_cfg_find() check is correct and it is impossible to access a
fw_cfg device that hasn't been wired up to the machine

3) Since these checks are done at realize time, we can provide nice
friendly messages back to the developer to tell them what needs to be fixed

Finally for reference here is the current version of the code in my
outstanding sun4u patchset which wires up the fw_cfg device behind a PCI
bridge in hw/sparc64/sun4u:

  dev = qdev_create(NULL, TYPE_FW_CFG_IO);
  qdev_prop_set_bit(dev, "dma_enabled", false);
  object_property_add_child(OBJECT(ebus), TYPE_FW_CFG, OBJECT(dev),
                            NULL);
  qdev_init_nofail(dev);
  memory_region_add_subregion(pci_address_space_io(ebus),
                              BIOS_CFG_IOPORT,
                              &FW_CFG_IO(dev)->comb_iomem);

ATB,

Mark.

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

[Eduardo Habkost]

On Fri, Jul 07, 2017 at 01:33:20PM +0200, Igor Mammedov wrote:
> On Tue, 4 Jul 2017 19:08:44 +0100
> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> 
> > On 03/07/17 10:39, Igor Mammedov wrote:
> > 
> > > On Thu, 29 Jun 2017 15:07:19 +0100
> > > Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> 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 we can now convert the asserts() preventing multiple fw_cfg devices
> > >> and unparented fw_cfg devices being instantiated and replace them with proper
> > >> error reporting at realize time. 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>
> > >> ---
> > >>  hw/nvram/fw_cfg.c |   41 +++++++++++++++++++++++++++++------------
> > >>  1 file changed, 29 insertions(+), 12 deletions(-)
> > >>
> > >> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > >> index 2291121..31029ac 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"
> > >> @@ -920,19 +917,22 @@ static int fw_cfg_unattached_at_realize(void)
> > >>  }
> > >>  
> > >>  
> > >> -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()) {  
> > > maybe add comment that here, that fw_cfg_find() will return NULL if more than
> > > 1 device is found.  
> > 
> > This should be the same behaviour as before, i.e. a NULL means that the
> > fw_cfg device couldn't be found. It seems intuitive to me from the name
> > of the function exactly what it does, so I don't find the lack of
> > comment too confusing. Does anyone else have any thoughts here?
> intuitive meaning from the function name would be return NULL if nothing were found,
> however it's not the case here.
> 
> taking in account that fwcfg in not user creatable device how about:
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 316fca9..8f6eef8 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -1014,7 +1014,10 @@ 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));
> +    bool ambig = false;
> +    FWCfgState *f = FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, &ambig));
> +    assert(!ambig);
> +    return f;
>  }
> 
> or if we must to print user friendly error and fail realize gracefully
> (not sure why) just add errp argument to function so it could report
> error back to caller, then places that do not care much about graceful
> exit would use error_abort as argument and realize would use
> its local_error/errp argument.

I don't disagree with adding the assert(), but it looks like
making fw_cfg_find() return NULL if there are multiple devices
can be useful for realize.

In this case, it looks like Mark is relying on that in
fw_cfg_common_realize(): if multiple devices are created,
fw_cfg_find() will return NULL, and realize will fail.  This
sounds like a more graceful way to handle multiple-device
creation than crashing on fw_cfg_find().  This is the solution
used by find_vmgenid_dev()/vmgenid_realize(), BTW.

Either way, we have to choose: either we make fw_cfg_find() crash
when multiple devices exist (in this case, the fw_cfg_find() call
on realize will be useless), or we make it return NULL and
document it very clearly.


> 
>    
> > >> +        error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG);  
> > > s/TYPE_FW_CFG/object_get_typename()/
> > > so it would print leaf type name   

I disagree.  We allow at most one fw_cfg device (it doesn't
matter which type), not at most one device of each leaf type.
Saying "at most one fw_cfg_mem device is permitted" if 1
fw_cfg_io and 1 fw_cfg_mem device is created would be misleading.


> > 
> > Previously the code would add the device to the machine at FW_CFG_PATH
> > which was fixed at /machine/fw_cfg regardless of whether it was
> > fw_cfg_io or fw_cfg_mem type (see the top of fw_cfg.c).
> > 
> > This was a deliberate attempt to preserve the existing behaviour and if
> > we were to give the leaf type name I think this would be misleading,
> > since it implies you could have one fw_cfg_io and one fw_cfg_mem which
> > isn't correct.
> I don't have strong preference here, considering that it's
> hardcoded in board (not user specified) device,
> developer that stumbles upon error should be able to dig out which
> concrete type caused it.
>    
> > >> +        return;
> > >> +    }
> > >>  
> > >> -    assert(!fw_cfg_unattached_at_realize());
> > >> +    if (fw_cfg_unattached_at_realize()) {  
> > > as I pointed out in v6, this condition will always be false,
> > > I suggest to drop 4/6 patch and this hunk here so it won't to confuse
> > > readers with assumption that condition might succeed.  
> > 
> > Definitely look more closely at the fw_cfg_unattached_at_realize()
> > implementation in patch 4. You'll see that the check to determine if the
> > device is attached does not check obj->parent but checks to see if the
> > device exists under /machine/unattached which is what the
> > device_set_realised() does if the device doesn't have a parent.
> looking more fw_cfg_unattached_at_realize(),
>  returns true if fwcfg is direct child of/machine/unattached
> meaning implicit parent assignment.
> 
> As result, above condition essentially forbids having fwcfg under
> /machine/unattached and forces user to explicitly set parent
> outside of /machine/unattached or be a child of some other device.
> 
> Is this your intent (why)?

I'm confused by this part as well.  I still don't see the point
of fw_cfg_unattached_at_realize(), I need to re-read the patches
and commit messages to try to understand that.

If we are changing fw_cfg_find() to not care about the fw_cfg
device location, we don't need to care if it's in
/machine/unattached or not.

> [...]

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Jul 07, 2017 at 02:12:19PM +0100, Mark Cave-Ayland wrote:
> On 07/07/17 12:33, Igor Mammedov wrote:
> 
> > On Tue, 4 Jul 2017 19:08:44 +0100
> > Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> > 
> >> On 03/07/17 10:39, Igor Mammedov wrote:
> >>
> >>> On Thu, 29 Jun 2017 15:07:19 +0100
> >>> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> 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 we can now convert the asserts() preventing multiple fw_cfg devices
> >>>> and unparented fw_cfg devices being instantiated and replace them with proper
> >>>> error reporting at realize time. 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>
> >>>> ---
> >>>>  hw/nvram/fw_cfg.c |   41 +++++++++++++++++++++++++++++------------
> >>>>  1 file changed, 29 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> >>>> index 2291121..31029ac 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"
> >>>> @@ -920,19 +917,22 @@ static int fw_cfg_unattached_at_realize(void)
> >>>>  }
> >>>>  
> >>>>  
> >>>> -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()) {  
> >>> maybe add comment that here, that fw_cfg_find() will return NULL if more than
> >>> 1 device is found.  
> >>
> >> This should be the same behaviour as before, i.e. a NULL means that the
> >> fw_cfg device couldn't be found. It seems intuitive to me from the name
> >> of the function exactly what it does, so I don't find the lack of
> >> comment too confusing. Does anyone else have any thoughts here?
> >
> > intuitive meaning from the function name would be return NULL if nothing were found,
> > however it's not the case here.
> 
> The function with its current name has always existed, so all I'm doing
> here is reusing it as per your comment on an earlier patchset.
> 
> > taking in account that fwcfg in not user creatable device how about:
> > 
> > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > index 316fca9..8f6eef8 100644
> > --- a/hw/nvram/fw_cfg.c
> > +++ b/hw/nvram/fw_cfg.c
> > @@ -1014,7 +1014,10 @@ 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));
> > +    bool ambig = false;
> > +    FWCfgState *f = FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, &ambig));
> > +    assert(!ambig);
> > +    return f;
> >  }
> > 
> > or if we must to print user friendly error and fail realize gracefully
> > (not sure why) just add errp argument to function so it could report
> > error back to caller, then places that do not care much about graceful
> > exit would use error_abort as argument and realize would use
> > its local_error/errp argument.
> 
> My understanding from the thread was that we should only use assert()s
> where there is no other choice so that any failures can be handled in a
> more friendly manner.
> 
> Now as Laszlo pointed out, fw_cfg_find() is used externally to locate
> the fw_cfg device in other parts of the QEMU codebase. Yes I agree that
> it is possible to change the way in which it returns, however I would
> argue that changing those semantics are outside of the scope of this patch.
> 
> >    
> >>>> +        error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG);  
> >>> s/TYPE_FW_CFG/object_get_typename()/
> >>> so it would print leaf type name   
> >>
> >> Previously the code would add the device to the machine at FW_CFG_PATH
> >> which was fixed at /machine/fw_cfg regardless of whether it was
> >> fw_cfg_io or fw_cfg_mem type (see the top of fw_cfg.c).
> >>
> >> This was a deliberate attempt to preserve the existing behaviour and if
> >> we were to give the leaf type name I think this would be misleading,
> >> since it implies you could have one fw_cfg_io and one fw_cfg_mem which
> >> isn't correct.
> > I don't have strong preference here, considering that it's
> > hardcoded in board (not user specified) device,
> > developer that stumbles upon error should be able to dig out which
> > concrete type caused it.
> 
> Okay if no-one else comments then I'll leave it as-is in order to
> preserve the existing behaviour as much as possible.
> 
> >>>> +        return;
> >>>> +    }
> >>>>  
> >>>> -    assert(!fw_cfg_unattached_at_realize());
> >>>> +    if (fw_cfg_unattached_at_realize()) {  
> >>> as I pointed out in v6, this condition will always be false,
> >>> I suggest to drop 4/6 patch and this hunk here so it won't to confuse
> >>> readers with assumption that condition might succeed.  
> >>
> >> Definitely look more closely at the fw_cfg_unattached_at_realize()
> >> implementation in patch 4. You'll see that the check to determine if the
> >> device is attached does not check obj->parent but checks to see if the
> >> device exists under /machine/unattached which is what the
> >> device_set_realised() does if the device doesn't have a parent.
> >
> > looking more fw_cfg_unattached_at_realize(),
> >  returns true if fwcfg is direct child of/machine/unattached
> > meaning implicit parent assignment.
> > 
> > As result, above condition essentially forbids having fwcfg under
> > /machine/unattached and forces user to explicitly set parent
> > outside of /machine/unattached or be a child of some other device.
> > 
> > Is this your intent (why)?
> 
> Yes that is entirely correct. All current fw_cfg users setup the device
> using fw_cfg_init_io() and fw_cfg_init_mem() which is fine for those
> cases because these functions attach the fw_cfg device directly to the
> machine at /machine/fw_cfg. This makes it trivial to determine whether
> or not an existing fw_cfg has been instantiated and prevent any more
> instances, which Laszlo has stated is an underlying assumption for
> fw_cfg_find().
> 
> In my particular use case for SPARC64, I need to move the fw_cfg device
> behind a PCI bridge. Therefore in order to allow the QOM tree to reflect
> the actual hardware DT then the fw_cfg device needs to be attached to
> the PCI bridge and not the machine. Hence the check for an existing
> device at /machine/fw_cfg is no longer good enough to determine if a
> fw_cfg device already exists since if they do, they can be in several
> different locations in the QOM tree.
> 
> This explains the change to fw_cfg_find() to make sure that we find any
> other fw_cfg instances, no matter where they are in the QOM tree.
> 
> However this causes us a problem: if you instantiate the fw_cfg yourself
> with qdev_create()...qdev_init_nofail() then you can potentially access
> the underlying MemoryRegions directly and wire up the device without
> attaching it to the QOM tree. This is an invalid configuration but
> wouldn't be detected with fw_cfg_find().

Why is it an invalid configuration?  All we need is that the
device get wired correctly and that fw_cfg_find() find the device
correctly.  Your patch 3/6 makes fw_cfg_find() work on any path,
making fw_cfg_unattached_at_realize() unnecessary.

> 
> The correct way to handle this is to wire up the device yourself with
> object_property_add_child() but then you find the situation whereby the
> people who know that you have to call object_property_add_child() when
> adding the fw_cfg device are the ones who don't need the error message.
> 
> Therefore the solution was to enforce that the fw_cfg device has been
> added to the QOM tree before realize because it solves all the problems:
> 
> 1) The fw_cfg device can now be placed anywhere in the QOM tree, meaning
> that the QOM tree can be a topologically correct representation of the
> machine

While attaching the device explicitly is a good idea, we don't
need to make it a fatal error.

> 
> 2) By enforcing that the fw_cfg device has been parented, we guarantee
> that the fw_cfg_find() check is correct and it is impossible to access a
> fw_cfg device that hasn't been wired up to the machine

This is solved by patch 3/6.

> 
> 3) Since these checks are done at realize time, we can provide nice
> friendly messages back to the developer to tell them what needs to be fixed

I don't see what needs to be fixed.  It is not a bug to leave
fw_cfg in /machine/unattached, as long as fw_cfg_find() works
properly.

-- 
Eduardo

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

[Mark Cave-Ayland]

On 07/07/17 14:26, Eduardo Habkost wrote:

(cut)

>> However this causes us a problem: if you instantiate the fw_cfg yourself
>> with qdev_create()...qdev_init_nofail() then you can potentially access
>> the underlying MemoryRegions directly and wire up the device without
>> attaching it to the QOM tree. This is an invalid configuration but
>> wouldn't be detected with fw_cfg_find().
> 
> Why is it an invalid configuration?  All we need is that the
> device get wired correctly and that fw_cfg_find() find the device
> correctly.  Your patch 3/6 makes fw_cfg_find() work on any path,
> making fw_cfg_unattached_at_realize() unnecessary.

That's a good point actually. The fw_cfg_find() change came in fairly
recently but from memory that on its own wasn't enough to prevent
multiple fw_cfg devices in my local tests which is why added
fw_cfg_unattached_at_realize() (sadly I can't remember exactly which
test case failed).

So actually perhaps the reason I got sidetracked here was because I was
actually hitting the issue pointed out by Igor whereby with ambiguous
set to NULL you can miss detecting multiple instances?

>>
>> The correct way to handle this is to wire up the device yourself with
>> object_property_add_child() but then you find the situation whereby the
>> people who know that you have to call object_property_add_child() when
>> adding the fw_cfg device are the ones who don't need the error message.
>>
>> Therefore the solution was to enforce that the fw_cfg device has been
>> added to the QOM tree before realize because it solves all the problems:
>>
>> 1) The fw_cfg device can now be placed anywhere in the QOM tree, meaning
>> that the QOM tree can be a topologically correct representation of the
>> machine
> 
> While attaching the device explicitly is a good idea, we don't
> need to make it a fatal error.
> 
>>
>> 2) By enforcing that the fw_cfg device has been parented, we guarantee
>> that the fw_cfg_find() check is correct and it is impossible to access a
>> fw_cfg device that hasn't been wired up to the machine
> 
> This is solved by patch 3/6.
> 
>>
>> 3) Since these checks are done at realize time, we can provide nice
>> friendly messages back to the developer to tell them what needs to be fixed
> 
> I don't see what needs to be fixed.  It is not a bug to leave
> fw_cfg in /machine/unattached, as long as fw_cfg_find() works
> properly.

Yeah. I wonder if I've been leading myself astray down the wrong path
here? Let me do some more local tests without
fw_cfg_unattached_at_realize() and with an ambiguous argument in
fw_cfg_find().


ATB,

Mark.

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

[Igor Mammedov]

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

> On 07/07/17 12:33, Igor Mammedov wrote:
> 
> > On Tue, 4 Jul 2017 19:08:44 +0100
> > Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> >   
> >> On 03/07/17 10:39, Igor Mammedov wrote:
> >>  
> >>> On Thu, 29 Jun 2017 15:07:19 +0100
> >>> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> 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 we can now convert the asserts() preventing multiple fw_cfg devices
> >>>> and unparented fw_cfg devices being instantiated and replace them with proper
> >>>> error reporting at realize time. 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>
> >>>> ---
> >>>>  hw/nvram/fw_cfg.c |   41 +++++++++++++++++++++++++++++------------
> >>>>  1 file changed, 29 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> >>>> index 2291121..31029ac 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"
> >>>> @@ -920,19 +917,22 @@ static int fw_cfg_unattached_at_realize(void)
> >>>>  }
> >>>>  
> >>>>  
> >>>> -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()) {    
> >>> maybe add comment that here, that fw_cfg_find() will return NULL if more than
> >>> 1 device is found.    
> >>
> >> This should be the same behaviour as before, i.e. a NULL means that the
> >> fw_cfg device couldn't be found. It seems intuitive to me from the name
> >> of the function exactly what it does, so I don't find the lack of
> >> comment too confusing. Does anyone else have any thoughts here?  
> >
> > intuitive meaning from the function name would be return NULL if nothing were found,
> > however it's not the case here.  
> 
> The function with its current name has always existed, so all I'm doing
> here is reusing it as per your comment on an earlier patchset.
that's why I've asked for comment explaining what's going on here


> > taking in account that fwcfg in not user creatable device how about:
> > 
> > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > index 316fca9..8f6eef8 100644
> > --- a/hw/nvram/fw_cfg.c
> > +++ b/hw/nvram/fw_cfg.c
> > @@ -1014,7 +1014,10 @@ 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));
> > +    bool ambig = false;
> > +    FWCfgState *f = FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, &ambig));
> > +    assert(!ambig);
> > +    return f;
> >  }
> > 
> > or if we must to print user friendly error and fail realize gracefully
> > (not sure why) just add errp argument to function so it could report
> > error back to caller, then places that do not care much about graceful
> > exit would use error_abort as argument and realize would use
> > its local_error/errp argument.  
> 
> My understanding from the thread was that we should only use assert()s
> where there is no other choice so that any failures can be handled in a
> more friendly manner.
the rule I use to decide assert vs nice error handling:
1: try to avoid crash on hotplug path
2: if error could be induced by end user on startup, try to print nice error
   before dying
3: when error should not happen just assert or use error_abort
   which would print nice error message before dying.

> Now as Laszlo pointed out, fw_cfg_find() is used externally to locate
> the fw_cfg device in other parts of the QEMU codebase. Yes I agree that
> it is possible to change the way in which it returns, however I would
> argue that changing those semantics are outside of the scope of this patch.
I'd just kill qemu in fw_cfg case, which is small not intrusive change.

[...]

> >>>>  
> >>>> -    assert(!fw_cfg_unattached_at_realize());
> >>>> +    if (fw_cfg_unattached_at_realize()) {    
> >>> as I pointed out in v6, this condition will always be false,
> >>> I suggest to drop 4/6 patch and this hunk here so it won't to confuse
> >>> readers with assumption that condition might succeed.    
> >>
> >> Definitely look more closely at the fw_cfg_unattached_at_realize()
> >> implementation in patch 4. You'll see that the check to determine if the
> >> device is attached does not check obj->parent but checks to see if the
> >> device exists under /machine/unattached which is what the
> >> device_set_realised() does if the device doesn't have a parent.  
> >
> > looking more fw_cfg_unattached_at_realize(),
> >  returns true if fwcfg is direct child of/machine/unattached
> > meaning implicit parent assignment.
> > 
> > As result, above condition essentially forbids having fwcfg under
> > /machine/unattached and forces user to explicitly set parent
> > outside of /machine/unattached or be a child of some other device.
> > 
> > Is this your intent (why)?  
> 
> Yes that is entirely correct. All current fw_cfg users setup the device
> using fw_cfg_init_io() and fw_cfg_init_mem() which is fine for those
> cases because these functions attach the fw_cfg device directly to the
> machine at /machine/fw_cfg. This makes it trivial to determine whether
> or not an existing fw_cfg has been instantiated and prevent any more
> instances, which Laszlo has stated is an underlying assumption for
> fw_cfg_find().
> 
> In my particular use case for SPARC64, I need to move the fw_cfg device
> behind a PCI bridge. Therefore in order to allow the QOM tree to reflect
> the actual hardware DT then the fw_cfg device needs to be attached to
> the PCI bridge and not the machine. Hence the check for an existing
> device at /machine/fw_cfg is no longer good enough to determine if a
> fw_cfg device already exists since if they do, they can be in several
> different locations in the QOM tree.
> 
> This explains the change to fw_cfg_find() to make sure that we find any
> other fw_cfg instances, no matter where they are in the QOM tree.
without using ambiguous argument object_resolve_path_type() isn't
returning NULL in case of duplicates in different leafs of tree.

for reason, see https://www.mail-archive.com/qemu-devel@nongnu.org/msg460692.html
or look at object_resolve_partial_path() impl.

> However this causes us a problem: if you instantiate the fw_cfg yourself
> with qdev_create()...qdev_init_nofail() then you can potentially access
> the underlying MemoryRegions directly and wire up the device without
> attaching it to the QOM tree. This is an invalid configuration but
> wouldn't be detected with fw_cfg_find().
device_set_realized() makes sure that it's attached to QOM tree,

it's up to board to decide if default /machine/unattached parent
is sufficient or fwcfg should be placed explicitly under another
parent. it's not upto device to decide on it's location though.


> The correct way to handle this is to wire up the device yourself with
> object_property_add_child() but then you find the situation whereby the
> people who know that you have to call object_property_add_child() when
> adding the fw_cfg device are the ones who don't need the error message.
> 
> Therefore the solution was to enforce that the fw_cfg device has been
> added to the QOM tree before realize because it solves all the problems:
there is no need to enforce that as device_set_realized() guaranties
that device's a child of some QOM object (explicitly or implictly) by the time
leaf realize is called.


> 1) The fw_cfg device can now be placed anywhere in the QOM tree, meaning
> that the QOM tree can be a topologically correct representation of the
> machine
with this series it can't be placed under /machine/unattached
I don't see any reason to disable it.

> 2) By enforcing that the fw_cfg device has been parented, we guarantee
> that the fw_cfg_find() check is correct and it is impossible to access a
> fw_cfg device that hasn't been wired up to the machine
fw_cfg_find() check at realize or later time is always correct if you use
ambiguous argument so object_resolve_path_type could properly detect duplicates.


> 3) Since these checks are done at realize time, we can provide nice
> friendly messages back to the developer to tell them what needs to be fixed
error_set(error_abort, ...) should work nice for fwcfg usecase,
you get error message and a stack trace in core file pointing to a place
of the error.


> Finally for reference here is the current version of the code in my
> outstanding sun4u patchset which wires up the fw_cfg device behind a PCI
> bridge in hw/sparc64/sun4u:
> 
>   dev = qdev_create(NULL, TYPE_FW_CFG_IO);
>   qdev_prop_set_bit(dev, "dma_enabled", false);
>   object_property_add_child(OBJECT(ebus), TYPE_FW_CFG, OBJECT(dev),
>                             NULL);
>   qdev_init_nofail(dev);
>   memory_region_add_subregion(pci_address_space_io(ebus),
>                               BIOS_CFG_IOPORT,
>                               &FW_CFG_IO(dev)->comb_iomem);
looks fine,

so what I'd do is:
 * drop 4/6
 * make fw_cfg_find() use ambiguous argument and error_abort if ambiguous == true
 * from fw_cfg_common_realize() just call

     // if fw_cfg_find() returns NULL it means that device isn't in QOM tree
     // which shouldn't ever happen, fw_cfg_find() will abort itself if
     // another instance of device present in QOM tree.
     assert(fw_cfg_find());


> ATB,
> 
> Mark.
> 

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

[Eduardo Habkost]

On Fri, Jul 07, 2017 at 02:54:49PM +0100, Mark Cave-Ayland wrote:
> On 07/07/17 14:26, Eduardo Habkost wrote:
> 
> (cut)
> 
> >> However this causes us a problem: if you instantiate the fw_cfg yourself
> >> with qdev_create()...qdev_init_nofail() then you can potentially access
> >> the underlying MemoryRegions directly and wire up the device without
> >> attaching it to the QOM tree. This is an invalid configuration but
> >> wouldn't be detected with fw_cfg_find().
> > 
> > Why is it an invalid configuration?  All we need is that the
> > device get wired correctly and that fw_cfg_find() find the device
> > correctly.  Your patch 3/6 makes fw_cfg_find() work on any path,
> > making fw_cfg_unattached_at_realize() unnecessary.
> 
> That's a good point actually. The fw_cfg_find() change came in fairly
> recently but from memory that on its own wasn't enough to prevent
> multiple fw_cfg devices in my local tests which is why added
> fw_cfg_unattached_at_realize() (sadly I can't remember exactly which
> test case failed).
> 
> So actually perhaps the reason I got sidetracked here was because I was
> actually hitting the issue pointed out by Igor whereby with ambiguous
> set to NULL you can miss detecting multiple instances?
> 

Possibly.  If ambiguous is NULL, you need to be aware that
fw_cfg_find() will return NULL on realize if there are multiple
instances.


> >>
> >> The correct way to handle this is to wire up the device yourself with
> >> object_property_add_child() but then you find the situation whereby the
> >> people who know that you have to call object_property_add_child() when
> >> adding the fw_cfg device are the ones who don't need the error message.
> >>
> >> Therefore the solution was to enforce that the fw_cfg device has been
> >> added to the QOM tree before realize because it solves all the problems:
> >>
> >> 1) The fw_cfg device can now be placed anywhere in the QOM tree, meaning
> >> that the QOM tree can be a topologically correct representation of the
> >> machine
> > 
> > While attaching the device explicitly is a good idea, we don't
> > need to make it a fatal error.
> > 
> >>
> >> 2) By enforcing that the fw_cfg device has been parented, we guarantee
> >> that the fw_cfg_find() check is correct and it is impossible to access a
> >> fw_cfg device that hasn't been wired up to the machine
> > 
> > This is solved by patch 3/6.
> > 
> >>
> >> 3) Since these checks are done at realize time, we can provide nice
> >> friendly messages back to the developer to tell them what needs to be fixed
> > 
> > I don't see what needs to be fixed.  It is not a bug to leave
> > fw_cfg in /machine/unattached, as long as fw_cfg_find() works
> > properly.
> 
> Yeah. I wonder if I've been leading myself astray down the wrong path
> here? Let me do some more local tests without
> fw_cfg_unattached_at_realize() and with an ambiguous argument in
> fw_cfg_find().

Note that if you add assert(!ambiguous) to fw_cfg_find(), you
need to choose what to do when multiple devices are instantiated:
a) Calling fw_cfg_find() on realize, making it abort instead of
   returning an error;
b) Not calling fw_cfg_find() on realize, and returning an error
   using object_resolve_path_type() manually;
c) Not calling fw_cfg_find(), not returning an error, and letting
   QEMU abort when fw_cfg_find() is called by other code.

I think it's simpler to just do like vmgenid does and simply
return NULL when there are multiple devices.  We just need to
document that clearly, and be aware of that when calling
fw_cfg_find() on realize.  But I won't object if you choose
another option.

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Jul 07, 2017 at 04:44:53PM +0200, Igor Mammedov wrote:
[...]
> > 3) Since these checks are done at realize time, we can provide nice
> > friendly messages back to the developer to tell them what needs to be fixed
> error_set(error_abort, ...) should work nice for fwcfg usecase,
> you get error message and a stack trace in core file pointing to a place
> of the error.

error_setg(&error_abort, ...) is explicitly discouraged by
error_setg() documentation.

-- 
Eduardo

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

[Igor Mammedov]

On Fri, 7 Jul 2017 10:13:00 -0300
"Eduardo Habkost" <ehabkost@redhat.com> wrote:

> On Fri, Jul 07, 2017 at 01:33:20PM +0200, Igor Mammedov wrote:
> > On Tue, 4 Jul 2017 19:08:44 +0100
> > Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> >   
> > > On 03/07/17 10:39, Igor Mammedov wrote:
> > >   
> > > > On Thu, 29 Jun 2017 15:07:19 +0100
> > > > Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> 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 we can now convert the asserts() preventing multiple fw_cfg devices
> > > >> and unparented fw_cfg devices being instantiated and replace them with proper
> > > >> error reporting at realize time. 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>
> > > >> ---
> > > >>  hw/nvram/fw_cfg.c |   41 +++++++++++++++++++++++++++++------------
> > > >>  1 file changed, 29 insertions(+), 12 deletions(-)
> > > >>
> > > >> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > > >> index 2291121..31029ac 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"
> > > >> @@ -920,19 +917,22 @@ static int fw_cfg_unattached_at_realize(void)
> > > >>  }
> > > >>  
> > > >>  
> > > >> -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()) {    
> > > > maybe add comment that here, that fw_cfg_find() will return NULL if more than
> > > > 1 device is found.    
> > > 
> > > This should be the same behaviour as before, i.e. a NULL means that the
> > > fw_cfg device couldn't be found. It seems intuitive to me from the name
> > > of the function exactly what it does, so I don't find the lack of
> > > comment too confusing. Does anyone else have any thoughts here?  
> > intuitive meaning from the function name would be return NULL if nothing were found,
> > however it's not the case here.
> > 
> > taking in account that fwcfg in not user creatable device how about:
> > 
> > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > index 316fca9..8f6eef8 100644
> > --- a/hw/nvram/fw_cfg.c
> > +++ b/hw/nvram/fw_cfg.c
> > @@ -1014,7 +1014,10 @@ 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));
> > +    bool ambig = false;
> > +    FWCfgState *f = FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, &ambig));
> > +    assert(!ambig);
> > +    return f;
> >  }
> > 
> > or if we must to print user friendly error and fail realize gracefully
> > (not sure why) just add errp argument to function so it could report
> > error back to caller, then places that do not care much about graceful
> > exit would use error_abort as argument and realize would use
> > its local_error/errp argument.  
> 
> I don't disagree with adding the assert(), but it looks like
> making fw_cfg_find() return NULL if there are multiple devices
> can be useful for realize.
> 
> In this case, it looks like Mark is relying on that in
> fw_cfg_common_realize(): if multiple devices are created,
> fw_cfg_find() will return NULL, and realize will fail.  This
> sounds like a more graceful way to handle multiple-device
> creation than crashing on fw_cfg_find().  This is the solution
> used by find_vmgenid_dev()/vmgenid_realize(), BTW.

I suspect that find_vmgenid_dev() works by luck as it could be
placed only as /machine/peripheral-anon/foo1 or /machine/peripheral/foo2
  object_resolve_partial_path() : machine
           object_resolve_partial_path() : peripheral-anon => foo1
           object_resolve_partial_path() : peripheral => foo2
           if (found /* foo2 */) {
               if (obj /* foo1 */) {
                   return NULL;

[...]

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

[Eduardo Habkost]

On Fri, Jul 07, 2017 at 04:44:53PM +0200, Igor Mammedov wrote:
[...]
> > > taking in account that fwcfg in not user creatable device how about:
> > > 
> > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > > index 316fca9..8f6eef8 100644
> > > --- a/hw/nvram/fw_cfg.c
> > > +++ b/hw/nvram/fw_cfg.c
> > > @@ -1014,7 +1014,10 @@ 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));
> > > +    bool ambig = false;
> > > +    FWCfgState *f = FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, &ambig));
> > > +    assert(!ambig);
> > > +    return f;
> > >  }
> > > 
> > > or if we must to print user friendly error and fail realize gracefully
> > > (not sure why) just add errp argument to function so it could report
> > > error back to caller, then places that do not care much about graceful
> > > exit would use error_abort as argument and realize would use
> > > its local_error/errp argument.  
> > 
> > My understanding from the thread was that we should only use assert()s
> > where there is no other choice so that any failures can be handled in a
> > more friendly manner.
> the rule I use to decide assert vs nice error handling:
> 1: try to avoid crash on hotplug path
> 2: if error could be induced by end user on startup, try to print nice error
>    before dying
> 3: when error should not happen just assert or use error_abort
>    which would print nice error message before dying.

I would add this to the list:

If returning error instead of aborting is easy, return an error
and let the caller decide if it should use &error_abort or not.

> 
> > Now as Laszlo pointed out, fw_cfg_find() is used externally to locate
> > the fw_cfg device in other parts of the QEMU codebase. Yes I agree that
> > it is possible to change the way in which it returns, however I would
> > argue that changing those semantics are outside of the scope of this patch.
> I'd just kill qemu in fw_cfg case, which is small not intrusive change.
> 
> [...]
> 
> > >>>>  
> > >>>> -    assert(!fw_cfg_unattached_at_realize());
> > >>>> +    if (fw_cfg_unattached_at_realize()) {    
> > >>> as I pointed out in v6, this condition will always be false,
> > >>> I suggest to drop 4/6 patch and this hunk here so it won't to confuse
> > >>> readers with assumption that condition might succeed.    
> > >>
> > >> Definitely look more closely at the fw_cfg_unattached_at_realize()
> > >> implementation in patch 4. You'll see that the check to determine if the
> > >> device is attached does not check obj->parent but checks to see if the
> > >> device exists under /machine/unattached which is what the
> > >> device_set_realised() does if the device doesn't have a parent.  
> > >
> > > looking more fw_cfg_unattached_at_realize(),
> > >  returns true if fwcfg is direct child of/machine/unattached
> > > meaning implicit parent assignment.
> > > 
> > > As result, above condition essentially forbids having fwcfg under
> > > /machine/unattached and forces user to explicitly set parent
> > > outside of /machine/unattached or be a child of some other device.
> > > 
> > > Is this your intent (why)?  
> > 
> > Yes that is entirely correct. All current fw_cfg users setup the device
> > using fw_cfg_init_io() and fw_cfg_init_mem() which is fine for those
> > cases because these functions attach the fw_cfg device directly to the
> > machine at /machine/fw_cfg. This makes it trivial to determine whether
> > or not an existing fw_cfg has been instantiated and prevent any more
> > instances, which Laszlo has stated is an underlying assumption for
> > fw_cfg_find().
> > 
> > In my particular use case for SPARC64, I need to move the fw_cfg device
> > behind a PCI bridge. Therefore in order to allow the QOM tree to reflect
> > the actual hardware DT then the fw_cfg device needs to be attached to
> > the PCI bridge and not the machine. Hence the check for an existing
> > device at /machine/fw_cfg is no longer good enough to determine if a
> > fw_cfg device already exists since if they do, they can be in several
> > different locations in the QOM tree.
> > 
> > This explains the change to fw_cfg_find() to make sure that we find any
> > other fw_cfg instances, no matter where they are in the QOM tree.
> without using ambiguous argument object_resolve_path_type() isn't
> returning NULL in case of duplicates in different leafs of tree.

This doesn't sound right.  object_resolve_path_type() should
always return NULL if multiple matches are found.  See its
documentation.

> 
> for reason, see https://www.mail-archive.com/qemu-devel@nongnu.org/msg460692.html
> or look at object_resolve_partial_path() impl.
> 
[...]
> > Finally for reference here is the current version of the code in my
> > outstanding sun4u patchset which wires up the fw_cfg device behind a PCI
> > bridge in hw/sparc64/sun4u:
> > 
> >   dev = qdev_create(NULL, TYPE_FW_CFG_IO);
> >   qdev_prop_set_bit(dev, "dma_enabled", false);
> >   object_property_add_child(OBJECT(ebus), TYPE_FW_CFG, OBJECT(dev),
> >                             NULL);
> >   qdev_init_nofail(dev);
> >   memory_region_add_subregion(pci_address_space_io(ebus),
> >                               BIOS_CFG_IOPORT,
> >                               &FW_CFG_IO(dev)->comb_iomem);
> looks fine,
> 
> so what I'd do is:
>  * drop 4/6

Agreed on this point.  But:

>  * make fw_cfg_find() use ambiguous argument and error_abort if ambiguous == true
>  * from fw_cfg_common_realize() just call
> 
>      // if fw_cfg_find() returns NULL it means that device isn't in QOM tree
>      // which shouldn't ever happen, fw_cfg_find() will abort itself if
>      // another instance of device present in QOM tree.
>      assert(fw_cfg_find());

That would work, but I don't see why doing that if just returning
NULL would: 1) make the code in fw_cfg_find() simpler and
shorter; 2) make realize error handling friendlier (returning
error instead of aborting).  We just need to document that
explicitly in fw_cfg_find() (see find_vmgenid_dev() for an
example).

If you still want to make realize abort instead of returning
error, you don't even need assert(ambiguous) on fw_cfg_find().
All you need is this on realize:

   assert(fw_cfg_find() == dev);

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Jul 07, 2017 at 04:58:17PM +0200, Igor Mammedov wrote:
> On Fri, 7 Jul 2017 10:13:00 -0300
> "Eduardo Habkost" <ehabkost@redhat.com> wrote:
[...]
> > I don't disagree with adding the assert(), but it looks like
> > making fw_cfg_find() return NULL if there are multiple devices
> > can be useful for realize.
> > 
> > In this case, it looks like Mark is relying on that in
> > fw_cfg_common_realize(): if multiple devices are created,
> > fw_cfg_find() will return NULL, and realize will fail.  This
> > sounds like a more graceful way to handle multiple-device
> > creation than crashing on fw_cfg_find().  This is the solution
> > used by find_vmgenid_dev()/vmgenid_realize(), BTW.
> 
> I suspect that find_vmgenid_dev() works by luck as it could be
> placed only as /machine/peripheral-anon/foo1 or /machine/peripheral/foo2
>   object_resolve_partial_path() : machine
>            object_resolve_partial_path() : peripheral-anon => foo1
>            object_resolve_partial_path() : peripheral => foo2
>            if (found /* foo2 */) {
>                if (obj /* foo1 */) {
>                    return NULL;

I don't think this is luck: object_resolve_partial_path() is
explicitly documented to always return NULL if multiple matches
are found, and I don't see any bug in its implementation that
would break that functionality.

-- 
Eduardo

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

[Mark Cave-Ayland]

On 07/07/17 14:13, Eduardo Habkost wrote:

>>> This should be the same behaviour as before, i.e. a NULL means that the
>>> fw_cfg device couldn't be found. It seems intuitive to me from the name
>>> of the function exactly what it does, so I don't find the lack of
>>> comment too confusing. Does anyone else have any thoughts here?
>> intuitive meaning from the function name would be return NULL if nothing were found,
>> however it's not the case here.
>>
>> taking in account that fwcfg in not user creatable device how about:
>>
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 316fca9..8f6eef8 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -1014,7 +1014,10 @@ 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));
>> +    bool ambig = false;
>> +    FWCfgState *f = FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, &ambig));
>> +    assert(!ambig);
>> +    return f;
>>  }
>>
>> or if we must to print user friendly error and fail realize gracefully
>> (not sure why) just add errp argument to function so it could report
>> error back to caller, then places that do not care much about graceful
>> exit would use error_abort as argument and realize would use
>> its local_error/errp argument.
> 
> I don't disagree with adding the assert(), but it looks like
> making fw_cfg_find() return NULL if there are multiple devices
> can be useful for realize.
> 
> In this case, it looks like Mark is relying on that in
> fw_cfg_common_realize(): if multiple devices are created,
> fw_cfg_find() will return NULL, and realize will fail.  This
> sounds like a more graceful way to handle multiple-device
> creation than crashing on fw_cfg_find().  This is the solution
> used by find_vmgenid_dev()/vmgenid_realize(), BTW.
> 
> Either way, we have to choose: either we make fw_cfg_find() crash
> when multiple devices exist (in this case, the fw_cfg_find() call
> on realize will be useless), or we make it return NULL and
> document it very clearly.

My personal preference too would be to keep the existing behaviour i.e.
NULL indicates failure and add a comment explaining how this also
matches the behaviour of object_resolve_path_type(). Then it is up the
caller to decide how to handle the severity as they wish.

>>>>> +        error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG);  
>>>> s/TYPE_FW_CFG/object_get_typename()/
>>>> so it would print leaf type name   
> 
> I disagree.  We allow at most one fw_cfg device (it doesn't
> matter which type), not at most one device of each leaf type.
> Saying "at most one fw_cfg_mem device is permitted" if 1
> fw_cfg_io and 1 fw_cfg_mem device is created would be misleading.

Yes, I agree with you here. FWIW I've just done some more testing with
patch 4 dropped and it seems to be working well, i.e. I can't manage to
reproduce the failure case I was seeing before. Slightly annoying, but
also promising.


ATB,

Mark.

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

[Mark Cave-Ayland]

On 07/07/17 15:48, Eduardo Habkost wrote:

>>> I don't see what needs to be fixed.  It is not a bug to leave
>>> fw_cfg in /machine/unattached, as long as fw_cfg_find() works
>>> properly.
>>
>> Yeah. I wonder if I've been leading myself astray down the wrong path
>> here? Let me do some more local tests without
>> fw_cfg_unattached_at_realize() and with an ambiguous argument in
>> fw_cfg_find().
> 
> Note that if you add assert(!ambiguous) to fw_cfg_find(), you
> need to choose what to do when multiple devices are instantiated:
> a) Calling fw_cfg_find() on realize, making it abort instead of
>    returning an error;
> b) Not calling fw_cfg_find() on realize, and returning an error
>    using object_resolve_path_type() manually;
> c) Not calling fw_cfg_find(), not returning an error, and letting
>    QEMU abort when fw_cfg_find() is called by other code.
> 
> I think it's simpler to just do like vmgenid does and simply
> return NULL when there are multiple devices.  We just need to
> document that clearly, and be aware of that when calling
> fw_cfg_find() on realize.  But I won't object if you choose
> another option.

Yes, I agree. I should add that I've been testing without the ambiguous
argument and everything seems to be working now...


ATB,

Mark.

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

[Mark Cave-Ayland]

On 07/07/17 16:07, Eduardo Habkost wrote:

>> looks fine,
>>
>> so what I'd do is:
>>  * drop 4/6

Yes.

> Agreed on this point.  But:
> 
>>  * make fw_cfg_find() use ambiguous argument and error_abort if ambiguous == true

During my latest tests I've found that everything works fine without the
ambiguous argument.

Do we still want to keep it? And I don't think error_abort() is the
right thing to do here, I'd much rather return NULL and add a suitable
comment.

>>  * from fw_cfg_common_realize() just call
>>
>>      // if fw_cfg_find() returns NULL it means that device isn't in QOM tree
>>      // which shouldn't ever happen, fw_cfg_find() will abort itself if
>>      // another instance of device present in QOM tree.
>>      assert(fw_cfg_find());
> 
> That would work, but I don't see why doing that if just returning
> NULL would: 1) make the code in fw_cfg_find() simpler and
> shorter; 2) make realize error handling friendlier (returning
> error instead of aborting).  We just need to document that
> explicitly in fw_cfg_find() (see find_vmgenid_dev() for an
> example).
> 
> If you still want to make realize abort instead of returning
> error, you don't even need assert(ambiguous) on fw_cfg_find().
> All you need is this on realize:
> 
>    assert(fw_cfg_find() == dev);

I'm definitely not a fan of the assert()...


ATB,

Mark.

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

[Igor Mammedov]

On Fri, 7 Jul 2017 12:09:56 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Jul 07, 2017 at 04:58:17PM +0200, Igor Mammedov wrote:
> > On Fri, 7 Jul 2017 10:13:00 -0300
> > "Eduardo Habkost" <ehabkost@redhat.com> wrote:
> [...]
> > > I don't disagree with adding the assert(), but it looks like
> > > making fw_cfg_find() return NULL if there are multiple devices
> > > can be useful for realize.
> > > 
> > > In this case, it looks like Mark is relying on that in
> > > fw_cfg_common_realize(): if multiple devices are created,
> > > fw_cfg_find() will return NULL, and realize will fail.  This
> > > sounds like a more graceful way to handle multiple-device
> > > creation than crashing on fw_cfg_find().  This is the solution
> > > used by find_vmgenid_dev()/vmgenid_realize(), BTW.
> > 
> > I suspect that find_vmgenid_dev() works by luck as it could be
> > placed only as /machine/peripheral-anon/foo1 or /machine/peripheral/foo2
> >   object_resolve_partial_path() : machine
> >            object_resolve_partial_path() : peripheral-anon => foo1
> >            object_resolve_partial_path() : peripheral => foo2
> >            if (found /* foo2 */) {
> >                if (obj /* foo1 */) {
> >                    return NULL;
> 
> I don't think this is luck: object_resolve_partial_path() is
> explicitly documented to always return NULL if multiple matches
> are found, and I don't see any bug in its implementation that
> would break that functionality.

Maybe I'm reading it wrong, but consider following:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg460692.html

it looks to me that using ambiguous argument is necessary for duplicate detection to work correctly.

> 

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

[Eduardo Habkost]

On Fri, Jul 07, 2017 at 08:18:20PM +0200, Igor Mammedov wrote:
> On Fri, 7 Jul 2017 12:09:56 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Fri, Jul 07, 2017 at 04:58:17PM +0200, Igor Mammedov wrote:
> > > On Fri, 7 Jul 2017 10:13:00 -0300
> > > "Eduardo Habkost" <ehabkost@redhat.com> wrote:
> > [...]
> > > > I don't disagree with adding the assert(), but it looks like
> > > > making fw_cfg_find() return NULL if there are multiple devices
> > > > can be useful for realize.
> > > > 
> > > > In this case, it looks like Mark is relying on that in
> > > > fw_cfg_common_realize(): if multiple devices are created,
> > > > fw_cfg_find() will return NULL, and realize will fail.  This
> > > > sounds like a more graceful way to handle multiple-device
> > > > creation than crashing on fw_cfg_find().  This is the solution
> > > > used by find_vmgenid_dev()/vmgenid_realize(), BTW.
> > > 
> > > I suspect that find_vmgenid_dev() works by luck as it could be
> > > placed only as /machine/peripheral-anon/foo1 or /machine/peripheral/foo2
> > >   object_resolve_partial_path() : machine
> > >            object_resolve_partial_path() : peripheral-anon => foo1
> > >            object_resolve_partial_path() : peripheral => foo2
> > >            if (found /* foo2 */) {
> > >                if (obj /* foo1 */) {
> > >                    return NULL;
> > 
> > I don't think this is luck: object_resolve_partial_path() is
> > explicitly documented to always return NULL if multiple matches
> > are found, and I don't see any bug in its implementation that
> > would break that functionality.
> 
> Maybe I'm reading it wrong, but consider following:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg460692.html
> 
> it looks to me that using ambiguous argument is necessary for
> duplicate detection to work correctly.

Oh, good catch, I think I see the bug now.  We need to write a
test case and fix that.

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Jul 07, 2017 at 04:30:29PM -0300, Eduardo Habkost wrote:
> On Fri, Jul 07, 2017 at 08:18:20PM +0200, Igor Mammedov wrote:
> > On Fri, 7 Jul 2017 12:09:56 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Fri, Jul 07, 2017 at 04:58:17PM +0200, Igor Mammedov wrote:
> > > > On Fri, 7 Jul 2017 10:13:00 -0300
> > > > "Eduardo Habkost" <ehabkost@redhat.com> wrote:
> > > [...]
> > > > > I don't disagree with adding the assert(), but it looks like
> > > > > making fw_cfg_find() return NULL if there are multiple devices
> > > > > can be useful for realize.
> > > > > 
> > > > > In this case, it looks like Mark is relying on that in
> > > > > fw_cfg_common_realize(): if multiple devices are created,
> > > > > fw_cfg_find() will return NULL, and realize will fail.  This
> > > > > sounds like a more graceful way to handle multiple-device
> > > > > creation than crashing on fw_cfg_find().  This is the solution
> > > > > used by find_vmgenid_dev()/vmgenid_realize(), BTW.
> > > > 
> > > > I suspect that find_vmgenid_dev() works by luck as it could be
> > > > placed only as /machine/peripheral-anon/foo1 or /machine/peripheral/foo2
> > > >   object_resolve_partial_path() : machine
> > > >            object_resolve_partial_path() : peripheral-anon => foo1
> > > >            object_resolve_partial_path() : peripheral => foo2
> > > >            if (found /* foo2 */) {
> > > >                if (obj /* foo1 */) {
> > > >                    return NULL;
> > > 
> > > I don't think this is luck: object_resolve_partial_path() is
> > > explicitly documented to always return NULL if multiple matches
> > > are found, and I don't see any bug in its implementation that
> > > would break that functionality.
> > 
> > Maybe I'm reading it wrong, but consider following:
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg460692.html
> > 
> > it looks to me that using ambiguous argument is necessary for
> > duplicate detection to work correctly.
> 
> Oh, good catch, I think I see the bug now.  We need to write a
> test case and fix that.

I could reproduce it with the following test case.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 8e432e9..c320cff 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -568,6 +568,29 @@ static void test_dummy_delchild(void)
     object_unparent(OBJECT(dev));
 }
 
+static void test_qom_partial_path(void)
+{
+    Object *root = object_get_objects_root();
+    Object *o   = object_new(TYPE_DUMMY);
+    Object *o1_1 = object_new(TYPE_DUMMY);
+    Object *o1_2 = object_new(TYPE_DUMMY);
+    Object *o2   = object_new(TYPE_DUMMY);
+
+    object_property_add_child(root, "o", o, &error_abort);
+    object_property_add_child(o, "o1", o1_1, &error_abort);
+    object_property_add_child(o, "o2", o1_2, &error_abort);
+    object_property_add_child(root, "o2", o2, &error_abort);
+
+    g_assert(!object_resolve_path_type("", TYPE_DUMMY, NULL));
+    g_assert(!object_resolve_path("o2", NULL));
+    g_assert(object_resolve_path("o1", NULL) == o1_1);
+
+    object_unref(o);
+    object_unref(o2);
+    object_unref(o1_1);
+    object_unref(o1_2);
+}
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -585,6 +608,7 @@ int main(int argc, char **argv)
     g_test_add_func("/qom/proplist/getenum", test_dummy_getenum);
     g_test_add_func("/qom/proplist/iterator", test_dummy_iterator);
     g_test_add_func("/qom/proplist/delchild", test_dummy_delchild);
+    g_test_add_func("/qom/path/resolve/partial", test_qom_partial_path);
 
     return g_test_run();
 }

-- 
Eduardo

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

[Laszlo Ersek]

On 07/07/17 21:54, Eduardo Habkost wrote:
> On Fri, Jul 07, 2017 at 04:30:29PM -0300, Eduardo Habkost wrote:
>> On Fri, Jul 07, 2017 at 08:18:20PM +0200, Igor Mammedov wrote:
>>> On Fri, 7 Jul 2017 12:09:56 -0300
>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>
>>>> On Fri, Jul 07, 2017 at 04:58:17PM +0200, Igor Mammedov wrote:
>>>>> On Fri, 7 Jul 2017 10:13:00 -0300
>>>>> "Eduardo Habkost" <ehabkost@redhat.com> wrote:
>>>> [...]
>>>>>> I don't disagree with adding the assert(), but it looks like
>>>>>> making fw_cfg_find() return NULL if there are multiple devices
>>>>>> can be useful for realize.
>>>>>>
>>>>>> In this case, it looks like Mark is relying on that in
>>>>>> fw_cfg_common_realize(): if multiple devices are created,
>>>>>> fw_cfg_find() will return NULL, and realize will fail.  This
>>>>>> sounds like a more graceful way to handle multiple-device
>>>>>> creation than crashing on fw_cfg_find().  This is the solution
>>>>>> used by find_vmgenid_dev()/vmgenid_realize(), BTW.
>>>>>
>>>>> I suspect that find_vmgenid_dev() works by luck as it could be
>>>>> placed only as /machine/peripheral-anon/foo1 or /machine/peripheral/foo2
>>>>>   object_resolve_partial_path() : machine
>>>>>            object_resolve_partial_path() : peripheral-anon => foo1
>>>>>            object_resolve_partial_path() : peripheral => foo2
>>>>>            if (found /* foo2 */) {
>>>>>                if (obj /* foo1 */) {
>>>>>                    return NULL;
>>>>
>>>> I don't think this is luck: object_resolve_partial_path() is
>>>> explicitly documented to always return NULL if multiple matches
>>>> are found, and I don't see any bug in its implementation that
>>>> would break that functionality.
>>>
>>> Maybe I'm reading it wrong, but consider following:
>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg460692.html
>>>
>>> it looks to me that using ambiguous argument is necessary for
>>> duplicate detection to work correctly.
>>
>> Oh, good catch, I think I see the bug now.  We need to write a
>> test case and fix that.
> 
> I could reproduce it with the following test case.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> index 8e432e9..c320cff 100644
> --- a/tests/check-qom-proplist.c
> +++ b/tests/check-qom-proplist.c
> @@ -568,6 +568,29 @@ static void test_dummy_delchild(void)
>      object_unparent(OBJECT(dev));
>  }
>  
> +static void test_qom_partial_path(void)
> +{
> +    Object *root = object_get_objects_root();
> +    Object *o   = object_new(TYPE_DUMMY);
> +    Object *o1_1 = object_new(TYPE_DUMMY);
> +    Object *o1_2 = object_new(TYPE_DUMMY);
> +    Object *o2   = object_new(TYPE_DUMMY);
> +
> +    object_property_add_child(root, "o", o, &error_abort);
> +    object_property_add_child(o, "o1", o1_1, &error_abort);
> +    object_property_add_child(o, "o2", o1_2, &error_abort);
> +    object_property_add_child(root, "o2", o2, &error_abort);
> +
> +    g_assert(!object_resolve_path_type("", TYPE_DUMMY, NULL));
> +    g_assert(!object_resolve_path("o2", NULL));
> +    g_assert(object_resolve_path("o1", NULL) == o1_1);
> +
> +    object_unref(o);
> +    object_unref(o2);
> +    object_unref(o1_1);
> +    object_unref(o1_2);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      g_test_init(&argc, &argv, NULL);
> @@ -585,6 +608,7 @@ int main(int argc, char **argv)
>      g_test_add_func("/qom/proplist/getenum", test_dummy_getenum);
>      g_test_add_func("/qom/proplist/iterator", test_dummy_iterator);
>      g_test_add_func("/qom/proplist/delchild", test_dummy_delchild);
> +    g_test_add_func("/qom/path/resolve/partial", test_qom_partial_path);
>  
>      return g_test_run();
>  }
> 

Meta comment: variable names like "o" and "l" are terrible (they look
like 0 and 1, dependent on your font). I suggest "obj".

Thanks
Laszlo

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

[Igor Mammedov]

On Fri, 7 Jul 2017 12:07:07 -0300
"Eduardo Habkost" <ehabkost@redhat.com> wrote:

> On Fri, Jul 07, 2017 at 04:44:53PM +0200, Igor Mammedov wrote:
> [...]
> > > > taking in account that fwcfg in not user creatable device how about:
> > > > 
> > > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > > > index 316fca9..8f6eef8 100644
> > > > --- a/hw/nvram/fw_cfg.c
> > > > +++ b/hw/nvram/fw_cfg.c
> > > > @@ -1014,7 +1014,10 @@ 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));
> > > > +    bool ambig = false;
> > > > +    FWCfgState *f = FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, &ambig));
> > > > +    assert(!ambig);
> > > > +    return f;
> > > >  }
> > > > 
> > > > or if we must to print user friendly error and fail realize gracefully
> > > > (not sure why) just add errp argument to function so it could report
> > > > error back to caller, then places that do not care much about graceful
> > > > exit would use error_abort as argument and realize would use
> > > > its local_error/errp argument.    
> > > 
> > > My understanding from the thread was that we should only use assert()s
> > > where there is no other choice so that any failures can be handled in a
> > > more friendly manner.  
> > the rule I use to decide assert vs nice error handling:
> > 1: try to avoid crash on hotplug path
> > 2: if error could be induced by end user on startup, try to print nice error
> >    before dying
> > 3: when error should not happen just assert or use error_abort
> >    which would print nice error message before dying.  
> 
> I would add this to the list:
> 
> If returning error instead of aborting is easy, return an error
> and let the caller decide if it should use &error_abort or not.
agreed, there aren't that many callers of fw_cfg_find()
so I'd just add error argument to it and handle error at call sites.

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

[Igor Mammedov]

On Fri, 7 Jul 2017 17:20:25 +0100
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:

> On 07/07/17 16:07, Eduardo Habkost wrote:
> 
> >> looks fine,
> >>
> >> so what I'd do is:
> >>  * drop 4/6  
> 
> Yes.
> 
> > Agreed on this point.  But:
> >   
> >>  * make fw_cfg_find() use ambiguous argument and error_abort if ambiguous == true  
> 
> During my latest tests I've found that everything works fine without the
> ambiguous argument.
> 
> Do we still want to keep it? And I don't think error_abort() is the
> right thing to do here, I'd much rather return NULL and add a suitable
> comment.
I'd still use ambiguous argument and since you prefer not to assert
I'd add errp argument to fw_cfg_find() and handle error at callsites.

Just returning NULL isn't sufficient if you need to distinguish
'not found' vs 'duplicate' usecases, additionally  'not found'
in most cases isn't even error but 'duplicate' definitely is.

Aborting on diplicate in fw_cfg_find() is fine and would
help to avoid touching current callers if you wish to limit
patches scope, but you can go with proper error propagating
route if you wish.

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

[Eduardo Habkost]

On Mon, Jul 10, 2017 at 09:49:38AM +0200, Igor Mammedov wrote:
> On Fri, 7 Jul 2017 12:07:07 -0300
> "Eduardo Habkost" <ehabkost@redhat.com> wrote:
> 
> > On Fri, Jul 07, 2017 at 04:44:53PM +0200, Igor Mammedov wrote:
> > [...]
> > > > > taking in account that fwcfg in not user creatable device how about:
> > > > > 
> > > > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > > > > index 316fca9..8f6eef8 100644
> > > > > --- a/hw/nvram/fw_cfg.c
> > > > > +++ b/hw/nvram/fw_cfg.c
> > > > > @@ -1014,7 +1014,10 @@ 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));
> > > > > +    bool ambig = false;
> > > > > +    FWCfgState *f = FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, &ambig));
> > > > > +    assert(!ambig);
> > > > > +    return f;
> > > > >  }
> > > > > 
> > > > > or if we must to print user friendly error and fail realize gracefully
> > > > > (not sure why) just add errp argument to function so it could report
> > > > > error back to caller, then places that do not care much about graceful
> > > > > exit would use error_abort as argument and realize would use
> > > > > its local_error/errp argument.    
> > > > 
> > > > My understanding from the thread was that we should only use assert()s
> > > > where there is no other choice so that any failures can be handled in a
> > > > more friendly manner.  
> > > the rule I use to decide assert vs nice error handling:
> > > 1: try to avoid crash on hotplug path
> > > 2: if error could be induced by end user on startup, try to print nice error
> > >    before dying
> > > 3: when error should not happen just assert or use error_abort
> > >    which would print nice error message before dying.  
> > 
> > I would add this to the list:
> > 
> > If returning error instead of aborting is easy, return an error
> > and let the caller decide if it should use &error_abort or not.
> agreed, there aren't that many callers of fw_cfg_find()
> so I'd just add error argument to it and handle error at call sites.

I was talking about realizefn, which already has a errp
parameter.  IMO a errp parameter to fw_cfg_find() would be
overkill.

(And an assert(!ambiguous) would make fw_cfg_find() less useful
because then it won't be usable by realizefn).

-- 
Eduardo

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

[Eduardo Habkost]

On Mon, Jul 10, 2017 at 10:01:47AM +0200, Igor Mammedov wrote:
> On Fri, 7 Jul 2017 17:20:25 +0100
> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> 
> > On 07/07/17 16:07, Eduardo Habkost wrote:
> > 
> > >> looks fine,
> > >>
> > >> so what I'd do is:
> > >>  * drop 4/6  
> > 
> > Yes.
> > 
> > > Agreed on this point.  But:
> > >   
> > >>  * make fw_cfg_find() use ambiguous argument and error_abort if ambiguous == true  
> > 
> > During my latest tests I've found that everything works fine without the
> > ambiguous argument.
> > 
> > Do we still want to keep it? And I don't think error_abort() is the
> > right thing to do here, I'd much rather return NULL and add a suitable
> > comment.
> I'd still use ambiguous argument and since you prefer not to assert
> I'd add errp argument to fw_cfg_find() and handle error at callsites.
> 
> Just returning NULL isn't sufficient if you need to distinguish
> 'not found' vs 'duplicate' usecases, additionally  'not found'
> in most cases isn't even error but 'duplicate' definitely is.
> 
> Aborting on diplicate in fw_cfg_find() is fine and would
> help to avoid touching current callers if you wish to limit
> patches scope, but you can go with proper error propagating
> route if you wish.

Just making realize refuse to create two devices sounds much
simpler to me.  No need to make fw_cfg_find() more complex (if we
add errp argument to it) or less useful (if we add
assert(!ambiguous) to it).

-- 
Eduardo

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

[Igor Mammedov]

On Mon, 10 Jul 2017 11:53:31 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Jul 10, 2017 at 10:01:47AM +0200, Igor Mammedov wrote:
> > On Fri, 7 Jul 2017 17:20:25 +0100
> > Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> >   
> > > On 07/07/17 16:07, Eduardo Habkost wrote:
> > >   
> > > >> looks fine,
> > > >>
> > > >> so what I'd do is:
> > > >>  * drop 4/6    
> > > 
> > > Yes.
> > >   
> > > > Agreed on this point.  But:
> > > >     
> > > >>  * make fw_cfg_find() use ambiguous argument and error_abort if ambiguous == true    
> > > 
> > > During my latest tests I've found that everything works fine without the
> > > ambiguous argument.
> > > 
> > > Do we still want to keep it? And I don't think error_abort() is the
> > > right thing to do here, I'd much rather return NULL and add a suitable
> > > comment.  
> > I'd still use ambiguous argument and since you prefer not to assert
> > I'd add errp argument to fw_cfg_find() and handle error at callsites.
> > 
> > Just returning NULL isn't sufficient if you need to distinguish
> > 'not found' vs 'duplicate' usecases, additionally  'not found'
> > in most cases isn't even error but 'duplicate' definitely is.
> > 
> > Aborting on diplicate in fw_cfg_find() is fine and would
> > help to avoid touching current callers if you wish to limit
> > patches scope, but you can go with proper error propagating
> > route if you wish.  
> 
> Just making realize refuse to create two devices sounds much
> simpler to me.  No need to make fw_cfg_find() more complex (if we
> add errp argument to it) or less useful (if we add
> assert(!ambiguous) to it).
the problem here was a error message to print if fw_cfg_find()
returns NULL for missing or duplicate, if we need to print
precise error we would need proper error handling.

Considering to fw_cfg is builtin device I'd prefer just
assert in fw_cfg_find() on duplicate (all the callers consider it as error)
and let developer to deal with assert if it is triggered.

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

[Eduardo Habkost]

On Mon, Jul 10, 2017 at 05:23:36PM +0200, Igor Mammedov wrote:
> On Mon, 10 Jul 2017 11:53:31 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Jul 10, 2017 at 10:01:47AM +0200, Igor Mammedov wrote:
> > > On Fri, 7 Jul 2017 17:20:25 +0100
> > > Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> > >   
> > > > On 07/07/17 16:07, Eduardo Habkost wrote:
> > > >   
> > > > >> looks fine,
> > > > >>
> > > > >> so what I'd do is:
> > > > >>  * drop 4/6    
> > > > 
> > > > Yes.
> > > >   
> > > > > Agreed on this point.  But:
> > > > >     
> > > > >>  * make fw_cfg_find() use ambiguous argument and error_abort if ambiguous == true    
> > > > 
> > > > During my latest tests I've found that everything works fine without the
> > > > ambiguous argument.
> > > > 
> > > > Do we still want to keep it? And I don't think error_abort() is the
> > > > right thing to do here, I'd much rather return NULL and add a suitable
> > > > comment.  
> > > I'd still use ambiguous argument and since you prefer not to assert
> > > I'd add errp argument to fw_cfg_find() and handle error at callsites.
> > > 
> > > Just returning NULL isn't sufficient if you need to distinguish
> > > 'not found' vs 'duplicate' usecases, additionally  'not found'
> > > in most cases isn't even error but 'duplicate' definitely is.
> > > 
> > > Aborting on diplicate in fw_cfg_find() is fine and would
> > > help to avoid touching current callers if you wish to limit
> > > patches scope, but you can go with proper error propagating
> > > route if you wish.  
> > 
> > Just making realize refuse to create two devices sounds much
> > simpler to me.  No need to make fw_cfg_find() more complex (if we
> > add errp argument to it) or less useful (if we add
> > assert(!ambiguous) to it).
> the problem here was a error message to print if fw_cfg_find()
> returns NULL for missing or duplicate, if we need to print
> precise error we would need proper error handling.

I don't see where we would need a precise error message, except
for realizefn (where the only case fw_cfg_find() would return
NULL is for duplicate devices).

> 
> Considering to fw_cfg is builtin device I'd prefer just
> assert in fw_cfg_find() on duplicate (all the callers consider it as error)
> and let developer to deal with assert if it is triggered.

Except that it would make it more difficult for realizefn to
return a proper error message.

Anyway, I am not completely against adding assert(!ambiguous) to
fw_cfg_find() if Mark wants to follow your advice.  I just think
it's not necessary.  I will only continue discussing this if I
see issues in the next version of the series.

-- 
Eduardo

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

[Mark Cave-Ayland]

On 10/07/17 18:38, Eduardo Habkost wrote:

> On Mon, Jul 10, 2017 at 05:23:36PM +0200, Igor Mammedov wrote:
>> On Mon, 10 Jul 2017 11:53:31 -0300
>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>
>>> On Mon, Jul 10, 2017 at 10:01:47AM +0200, Igor Mammedov wrote:
>>>> On Fri, 7 Jul 2017 17:20:25 +0100
>>>> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
>>>>   
>>>>> On 07/07/17 16:07, Eduardo Habkost wrote:
>>>>>   
>>>>>>> looks fine,
>>>>>>>
>>>>>>> so what I'd do is:
>>>>>>>  * drop 4/6    
>>>>>
>>>>> Yes.
>>>>>   
>>>>>> Agreed on this point.  But:
>>>>>>     
>>>>>>>  * make fw_cfg_find() use ambiguous argument and error_abort if ambiguous == true    
>>>>>
>>>>> During my latest tests I've found that everything works fine without the
>>>>> ambiguous argument.
>>>>>
>>>>> Do we still want to keep it? And I don't think error_abort() is the
>>>>> right thing to do here, I'd much rather return NULL and add a suitable
>>>>> comment.  
>>>> I'd still use ambiguous argument and since you prefer not to assert
>>>> I'd add errp argument to fw_cfg_find() and handle error at callsites.
>>>>
>>>> Just returning NULL isn't sufficient if you need to distinguish
>>>> 'not found' vs 'duplicate' usecases, additionally  'not found'
>>>> in most cases isn't even error but 'duplicate' definitely is.
>>>>
>>>> Aborting on diplicate in fw_cfg_find() is fine and would
>>>> help to avoid touching current callers if you wish to limit
>>>> patches scope, but you can go with proper error propagating
>>>> route if you wish.  
>>>
>>> Just making realize refuse to create two devices sounds much
>>> simpler to me.  No need to make fw_cfg_find() more complex (if we
>>> add errp argument to it) or less useful (if we add
>>> assert(!ambiguous) to it).
>> the problem here was a error message to print if fw_cfg_find()
>> returns NULL for missing or duplicate, if we need to print
>> precise error we would need proper error handling.
> 
> I don't see where we would need a precise error message, except
> for realizefn (where the only case fw_cfg_find() would return
> NULL is for duplicate devices).
> 
>>
>> Considering to fw_cfg is builtin device I'd prefer just
>> assert in fw_cfg_find() on duplicate (all the callers consider it as error)
>> and let developer to deal with assert if it is triggered.
> 
> Except that it would make it more difficult for realizefn to
> return a proper error message.
> 
> Anyway, I am not completely against adding assert(!ambiguous) to
> fw_cfg_find() if Mark wants to follow your advice.  I just think
> it's not necessary.  I will only continue discussing this if I
> see issues in the next version of the series.

I agree that it's also not necessary. The aim of the patch is to get the
fw_cfg device to the point where it can be instantiated directly via
qdev - and while the patch does use fw_cfg_find() to help with that,
fw_cfg_find() has other callers too and I feel that it goes beyond the
scope of the patch to start changing those semantics too.

As freeze is coming up next week, I will post a v8 shortly.


ATB,

Mark.