[PATCHv6 0/5] fw_cfg: qdev-related tidy-ups

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
28 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCHv6 0/5] fw_cfg: qdev-related tidy-ups

Mark Cave-Ayland-2
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 <[hidden email]>

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 (5):
  fw_cfg: don't map the fw_cfg IO ports in fw_cfg_io_realize()
  fw_cfg: move setting of FW_CFG_VERSION_DMA bit to fw_cfg_init1()
  fw_cfg: move assert() and linking of fw_cfg device to the machine
    into instance_init()
  fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers
  fw_cfg: move QOM type defines and fw_cfg types into fw_cfg.h

 hw/nvram/fw_cfg.c         |  107 ++++++++++++++-------------------------------
 include/hw/nvram/fw_cfg.h |   50 +++++++++++++++++++++
 include/qemu/typedefs.h   |    1 +
 3 files changed, 84 insertions(+), 74 deletions(-)

--
1.7.10.4


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCHv6 1/5] fw_cfg: don't map the fw_cfg IO ports in fw_cfg_io_realize()

Mark Cave-Ayland-2
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 <[hidden email]>
Reviewed-by: Laszlo Ersek <[hidden email]>
Tested-by: Laszlo Ersek <[hidden email]>
---
 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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCHv6 2/5] fw_cfg: move setting of FW_CFG_VERSION_DMA bit to fw_cfg_init1()

Mark Cave-Ayland-2
In reply to this post by Mark Cave-Ayland-2
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 <[hidden email]>
Reviewed-by: Laszlo Ersek <[hidden email]>
Tested-by: Laszlo Ersek <[hidden email]>
---
 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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mark Cave-Ayland-2
In reply to this post by Mark Cave-Ayland-2
In preparation for calling fw_cfg_init1() during realize rather than during
init, move the assert() checking for existing fw_cfg devices and the linking
of the device to the machine with object_property_add_child() to a new
fw_cfg instance_init() function.

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

Signed-off-by: Mark Cave-Ayland <[hidden email]>
Reviewed-by: Laszlo Ersek <[hidden email]>
Tested-by: Laszlo Ersek <[hidden email]>
---
 hw/nvram/fw_cfg.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCHv6 4/5] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers

Mark Cave-Ayland-2
In reply to this post by Mark Cave-Ayland-2
When looking to instantiate a TYPE_FW_CFG_MEM or TYPE_FW_CFG_IO device to be
able to wire it up differently, it is much more convenient for the caller to
instantiate the device and have the fw_cfg default files already preloaded
during realize.

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

Signed-off-by: Mark Cave-Ayland <[hidden email]>
Reviewed-by: Laszlo Ersek <[hidden email]>
Tested-by: Laszlo Ersek <[hidden email]>
---
 hw/nvram/fw_cfg.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index af45012..df99903 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -909,14 +909,12 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data)
 
 
 
-static void fw_cfg_init1(DeviceState *dev)
+static void fw_cfg_common_realize(DeviceState *dev)
 {
     FWCfgState *s = FW_CFG(dev);
     MachineState *machine = MACHINE(qdev_get_machine());
     uint32_t version = FW_CFG_VERSION;
 
-    qdev_init_nofail(dev);
-
     fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
     fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16);
     fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)!machine->enable_graphics);
@@ -948,7 +946,7 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
         qdev_prop_set_bit(dev, "dma_enabled", false);
     }
 
-    fw_cfg_init1(dev);
+    qdev_init_nofail(dev);
 
     sbd = SYS_BUS_DEVICE(dev);
     ios = FW_CFG_IO(dev);
@@ -986,7 +984,7 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
         qdev_prop_set_bit(dev, "dma_enabled", false);
     }
 
-    fw_cfg_init1(dev);
+    qdev_init_nofail(dev);
 
     sbd = SYS_BUS_DEVICE(dev);
     sysbus_mmio_map(sbd, 0, ctl_addr);
@@ -1097,6 +1095,8 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
                               &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
                               sizeof(dma_addr_t));
     }
+
+    fw_cfg_common_realize(dev);
 }
 
 static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
@@ -1163,6 +1163,8 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
                               sizeof(dma_addr_t));
         sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
     }
+
+    fw_cfg_common_realize(dev);
 }
 
 static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
--
1.7.10.4


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCHv6 5/5] fw_cfg: move QOM type defines and fw_cfg types into fw_cfg.h

Mark Cave-Ayland-2
In reply to this post by Mark Cave-Ayland-2
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 <[hidden email]>
Reviewed-by: Laszlo Ersek <[hidden email]>
Tested-by: Laszlo Ersek <[hidden email]>
---
 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 df99903..9b0aaa2 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -40,14 +40,6 @@
 #define FW_CFG_NAME "fw_cfg"
 #define FW_CFG_PATH "/machine/" FW_CFG_NAME
 
-#define TYPE_FW_CFG     "fw_cfg"
-#define TYPE_FW_CFG_IO  "fw_cfg_io"
-#define TYPE_FW_CFG_MEM "fw_cfg_mem"
-
-#define FW_CFG(obj)     OBJECT_CHECK(FWCfgState,    (obj), TYPE_FW_CFG)
-#define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
-#define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
-
 /* FW_CFG_VERSION bits */
 #define FW_CFG_VERSION      0x01
 #define FW_CFG_VERSION_DMA  0x02
@@ -61,51 +53,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 f745d5f..2db2918 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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCHv6 1/5] fw_cfg: don't map the fw_cfg IO ports in fw_cfg_io_realize()

Eduardo Habkost-2
In reply to this post by Mark Cave-Ayland-2
On Mon, Jun 19, 2017 at 01:59:05PM +0100, Mark Cave-Ayland wrote:

> As indicated by Laszlo it is a QOM bug for the realize() method to actually
> map the device. Set up the IO regions within fw_cfg_io_realize() and defer
> the mapping with sysbus_add_io() to the caller, as already done in
> fw_cfg_init_mem_wide().
>
> This makes the iobase and dma_iobase properties now obsolete so they can be
> removed.
>
> Signed-off-by: Mark Cave-Ayland <[hidden email]>
> Reviewed-by: Laszlo Ersek <[hidden email]>
> Tested-by: Laszlo Ersek <[hidden email]>

Reviewed-by: Eduardo Habkost <[hidden email]>

--
Eduardo

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCHv6 2/5] fw_cfg: move setting of FW_CFG_VERSION_DMA bit to fw_cfg_init1()

Eduardo Habkost-2
In reply to this post by Mark Cave-Ayland-2
On Mon, Jun 19, 2017 at 01:59:06PM +0100, Mark Cave-Ayland wrote:
> The setting of the FW_CFG_VERSION_DMA bit is the same across both the
> TYPE_FW_CFG_MEM and TYPE_FW_CFG_IO devices, so unify the logic in
> fw_cfg_init1().
>
> Signed-off-by: Mark Cave-Ayland <[hidden email]>
> Reviewed-by: Laszlo Ersek <[hidden email]>
> Tested-by: Laszlo Ersek <[hidden email]>

Reviewed-by: Eduardo Habkost <[hidden email]>

--
Eduardo

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Eduardo Habkost-2
In reply to this post by Mark Cave-Ayland-2
On Mon, Jun 19, 2017 at 01:59:07PM +0100, Mark Cave-Ayland wrote:

> In preparation for calling fw_cfg_init1() during realize rather than during
> init, move the assert() checking for existing fw_cfg devices and the linking
> of the device to the machine with object_property_add_child() to a new
> fw_cfg instance_init() function.
>
> This guarantees that we will still assert() correctly if more than one fw_cfg
> device is instantiated by accident.
>
> Signed-off-by: Mark Cave-Ayland <[hidden email]>
> Reviewed-by: Laszlo Ersek <[hidden email]>
> Tested-by: Laszlo Ersek <[hidden email]>
> ---
>  hw/nvram/fw_cfg.c |   14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 99bdbc2..af45012 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -915,10 +915,6 @@ static void fw_cfg_init1(DeviceState *dev)
>      MachineState *machine = MACHINE(qdev_get_machine());
>      uint32_t version = FW_CFG_VERSION;
>  
> -    assert(!object_resolve_path(FW_CFG_PATH, NULL));
> -
> -    object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s), NULL);
> -
>      qdev_init_nofail(dev);
>  
>      fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
> @@ -1020,6 +1016,15 @@ FWCfgState *fw_cfg_find(void)
>      return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
>  }
>  
> +static void fw_cfg_init(Object *obj)
> +{
> +    MachineState *machine = MACHINE(qdev_get_machine());
> +
> +    assert(!object_resolve_path(FW_CFG_PATH, NULL));
> +
> +    object_property_add_child(OBJECT(machine), FW_CFG_NAME, obj, NULL);

I don't think this belongs to instance_init.  We must always be
able to instantiate objects without crashing QEMU or affecting
QEMU global state.  This patch makes device-list-properties
crash:

  $ qemu-system-x86_64 -display none -qmp unix:/tmp/qmp,server,nowait &
  [1] 2848
  $ echo 'device-list-properties typename=fw_cfg_mem' | ./scripts/qmp/qmp-shell /tmp/qmp
  Welcome to the QMP low-level shell!
  Connected to QEMU 2.9.50
 
  qemu-system-x86_64: qemu/hw/nvram/fw_cfg.c:974: fw_cfg_init: Assertion `!object_resolve_path(FW_CFG_PATH, NULL)' failed.
  (QEMU) Disconnected
  [1]+  Aborted                 (core dumped) qemu-system-x86_64 -display none -qmp unix:/tmp/qmp,server,nowait
  $


I suggest moving this check to realize, like the rest of
fw_cfg_init1(), but change it to do proper error reporting
instead of asserting.

> +}
> +
>  static void fw_cfg_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -1033,6 +1038,7 @@ static const TypeInfo fw_cfg_info = {
>      .parent        = TYPE_SYS_BUS_DEVICE,
>      .abstract      = true,
>      .instance_size = sizeof(FWCfgState),
> +    .instance_init = fw_cfg_init,
>      .class_init    = fw_cfg_class_init,
>  };
>  
> --
> 1.7.10.4
>

--
Eduardo

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Laszlo Ersek (Red Hat)
On 06/19/17 16:28, Eduardo Habkost wrote:

> On Mon, Jun 19, 2017 at 01:59:07PM +0100, Mark Cave-Ayland wrote:
>> In preparation for calling fw_cfg_init1() during realize rather than during
>> init, move the assert() checking for existing fw_cfg devices and the linking
>> of the device to the machine with object_property_add_child() to a new
>> fw_cfg instance_init() function.
>>
>> This guarantees that we will still assert() correctly if more than one fw_cfg
>> device is instantiated by accident.
>>
>> Signed-off-by: Mark Cave-Ayland <[hidden email]>
>> Reviewed-by: Laszlo Ersek <[hidden email]>
>> Tested-by: Laszlo Ersek <[hidden email]>
>> ---
>>  hw/nvram/fw_cfg.c |   14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 99bdbc2..af45012 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -915,10 +915,6 @@ static void fw_cfg_init1(DeviceState *dev)
>>      MachineState *machine = MACHINE(qdev_get_machine());
>>      uint32_t version = FW_CFG_VERSION;
>>  
>> -    assert(!object_resolve_path(FW_CFG_PATH, NULL));
>> -
>> -    object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s), NULL);
>> -
>>      qdev_init_nofail(dev);
>>  
>>      fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
>> @@ -1020,6 +1016,15 @@ FWCfgState *fw_cfg_find(void)
>>      return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
>>  }
>>  
>> +static void fw_cfg_init(Object *obj)
>> +{
>> +    MachineState *machine = MACHINE(qdev_get_machine());
>> +
>> +    assert(!object_resolve_path(FW_CFG_PATH, NULL));
>> +
>> +    object_property_add_child(OBJECT(machine), FW_CFG_NAME, obj, NULL);
>
> I don't think this belongs to instance_init.  We must always be
> able to instantiate objects without crashing QEMU or affecting
> QEMU global state.  This patch makes device-list-properties
> crash:
>
>   $ qemu-system-x86_64 -display none -qmp unix:/tmp/qmp,server,nowait &
>   [1] 2848
>   $ echo 'device-list-properties typename=fw_cfg_mem' | ./scripts/qmp/qmp-shell /tmp/qmp
>   Welcome to the QMP low-level shell!
>   Connected to QEMU 2.9.50
>  
>   qemu-system-x86_64: qemu/hw/nvram/fw_cfg.c:974: fw_cfg_init: Assertion `!object_resolve_path(FW_CFG_PATH, NULL)' failed.
>   (QEMU) Disconnected
>   [1]+  Aborted                 (core dumped) qemu-system-x86_64 -display none -qmp unix:/tmp/qmp,server,nowait
>   $
>
>
> I suggest moving this check to realize, like the rest of
> fw_cfg_init1(), but change it to do proper error reporting
> instead of asserting.

Originally I argued against that, but as I said back then (I think?) I
didn't have a better reason for that comment of mine than a gut feeling.
So this feedback is definitely welcome by me. (Mark: sorry about the
churn, I made it clear up-front that I wasn't a QOM expert...)

Thanks
Laszlo

>
>> +}
>> +
>>  static void fw_cfg_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -1033,6 +1038,7 @@ static const TypeInfo fw_cfg_info = {
>>      .parent        = TYPE_SYS_BUS_DEVICE,
>>      .abstract      = true,
>>      .instance_size = sizeof(FWCfgState),
>> +    .instance_init = fw_cfg_init,
>>      .class_init    = fw_cfg_class_init,
>>  };
>>  
>> --
>> 1.7.10.4
>>
>


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mark Cave-Ayland-2
In reply to this post by Eduardo Habkost-2
On 19/06/17 15:28, Eduardo Habkost wrote:

> On Mon, Jun 19, 2017 at 01:59:07PM +0100, Mark Cave-Ayland wrote:
>> In preparation for calling fw_cfg_init1() during realize rather than during
>> init, move the assert() checking for existing fw_cfg devices and the linking
>> of the device to the machine with object_property_add_child() to a new
>> fw_cfg instance_init() function.
>>
>> This guarantees that we will still assert() correctly if more than one fw_cfg
>> device is instantiated by accident.
>>
>> Signed-off-by: Mark Cave-Ayland <[hidden email]>
>> Reviewed-by: Laszlo Ersek <[hidden email]>
>> Tested-by: Laszlo Ersek <[hidden email]>
>> ---
>>  hw/nvram/fw_cfg.c |   14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 99bdbc2..af45012 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -915,10 +915,6 @@ static void fw_cfg_init1(DeviceState *dev)
>>      MachineState *machine = MACHINE(qdev_get_machine());
>>      uint32_t version = FW_CFG_VERSION;
>>  
>> -    assert(!object_resolve_path(FW_CFG_PATH, NULL));
>> -
>> -    object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s), NULL);
>> -
>>      qdev_init_nofail(dev);
>>  
>>      fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
>> @@ -1020,6 +1016,15 @@ FWCfgState *fw_cfg_find(void)
>>      return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
>>  }
>>  
>> +static void fw_cfg_init(Object *obj)
>> +{
>> +    MachineState *machine = MACHINE(qdev_get_machine());
>> +
>> +    assert(!object_resolve_path(FW_CFG_PATH, NULL));
>> +
>> +    object_property_add_child(OBJECT(machine), FW_CFG_NAME, obj, NULL);
>
> I don't think this belongs to instance_init.  We must always be
> able to instantiate objects without crashing QEMU or affecting
> QEMU global state.  This patch makes device-list-properties
> crash:
>
>   $ qemu-system-x86_64 -display none -qmp unix:/tmp/qmp,server,nowait &
>   [1] 2848
>   $ echo 'device-list-properties typename=fw_cfg_mem' | ./scripts/qmp/qmp-shell /tmp/qmp
>   Welcome to the QMP low-level shell!
>   Connected to QEMU 2.9.50
>  
>   qemu-system-x86_64: qemu/hw/nvram/fw_cfg.c:974: fw_cfg_init: Assertion `!object_resolve_path(FW_CFG_PATH, NULL)' failed.
>   (QEMU) Disconnected
>   [1]+  Aborted                 (core dumped) qemu-system-x86_64 -display none -qmp unix:/tmp/qmp,server,nowait
>   $
>
>
> I suggest moving this check to realize, like the rest of
> fw_cfg_init1(), but change it to do proper error reporting
> instead of asserting.

In that case what do you think is the best way to prevent realization of
a second instance? With some playing I've managed to come up with the
following (partial) diff that seems to work in some simple local tests:


diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 9b0aaa2..e678ec0 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -862,12 +862,20 @@ static void fw_cfg_machine_ready(struct Notifier
*n, void *data)

-static void fw_cfg_common_realize(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;
-
+    Object *obj;
+
+    obj = object_resolve_path(FW_CFG_PATH, NULL);
+    if (obj != OBJECT(dev)) {
+        error_setg(errp, "Only one fw_cfg device can be instantiated per "
+                         "machine");
+        return;
+    }
+
     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);


What seems to happen is that calling object_property_add_child() only
succeeds for the first instance and so a simple comparison is enough to
determine that the device already exists at FW_CFG_PATH. Or is this a
fairly terrible (ab)use of the QOM APIs?


ATB,

Mark.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Laszlo Ersek (Red Hat)
On 06/19/17 18:57, Mark Cave-Ayland wrote:

> On 19/06/17 15:28, Eduardo Habkost wrote:
>
>> On Mon, Jun 19, 2017 at 01:59:07PM +0100, Mark Cave-Ayland wrote:
>>> In preparation for calling fw_cfg_init1() during realize rather than during
>>> init, move the assert() checking for existing fw_cfg devices and the linking
>>> of the device to the machine with object_property_add_child() to a new
>>> fw_cfg instance_init() function.
>>>
>>> This guarantees that we will still assert() correctly if more than one fw_cfg
>>> device is instantiated by accident.
>>>
>>> Signed-off-by: Mark Cave-Ayland <[hidden email]>
>>> Reviewed-by: Laszlo Ersek <[hidden email]>
>>> Tested-by: Laszlo Ersek <[hidden email]>
>>> ---
>>>  hw/nvram/fw_cfg.c |   14 ++++++++++----
>>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>>> index 99bdbc2..af45012 100644
>>> --- a/hw/nvram/fw_cfg.c
>>> +++ b/hw/nvram/fw_cfg.c
>>> @@ -915,10 +915,6 @@ static void fw_cfg_init1(DeviceState *dev)
>>>      MachineState *machine = MACHINE(qdev_get_machine());
>>>      uint32_t version = FW_CFG_VERSION;
>>>  
>>> -    assert(!object_resolve_path(FW_CFG_PATH, NULL));
>>> -
>>> -    object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s), NULL);
>>> -
>>>      qdev_init_nofail(dev);
>>>  
>>>      fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
>>> @@ -1020,6 +1016,15 @@ FWCfgState *fw_cfg_find(void)
>>>      return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
>>>  }
>>>  
>>> +static void fw_cfg_init(Object *obj)
>>> +{
>>> +    MachineState *machine = MACHINE(qdev_get_machine());
>>> +
>>> +    assert(!object_resolve_path(FW_CFG_PATH, NULL));
>>> +
>>> +    object_property_add_child(OBJECT(machine), FW_CFG_NAME, obj, NULL);
>>
>> I don't think this belongs to instance_init.  We must always be
>> able to instantiate objects without crashing QEMU or affecting
>> QEMU global state.  This patch makes device-list-properties
>> crash:
>>
>>   $ qemu-system-x86_64 -display none -qmp unix:/tmp/qmp,server,nowait &
>>   [1] 2848
>>   $ echo 'device-list-properties typename=fw_cfg_mem' | ./scripts/qmp/qmp-shell /tmp/qmp
>>   Welcome to the QMP low-level shell!
>>   Connected to QEMU 2.9.50
>>  
>>   qemu-system-x86_64: qemu/hw/nvram/fw_cfg.c:974: fw_cfg_init: Assertion `!object_resolve_path(FW_CFG_PATH, NULL)' failed.
>>   (QEMU) Disconnected
>>   [1]+  Aborted                 (core dumped) qemu-system-x86_64 -display none -qmp unix:/tmp/qmp,server,nowait
>>   $
>>
>>
>> I suggest moving this check to realize, like the rest of
>> fw_cfg_init1(), but change it to do proper error reporting
>> instead of asserting.
>
> In that case what do you think is the best way to prevent realization of
> a second instance? With some playing I've managed to come up with the
> following (partial) diff that seems to work in some simple local tests:
>
>
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 9b0aaa2..e678ec0 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -862,12 +862,20 @@ static void fw_cfg_machine_ready(struct Notifier
> *n, void *data)
>
> -static void fw_cfg_common_realize(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;
> -
> +    Object *obj;
> +
> +    obj = object_resolve_path(FW_CFG_PATH, NULL);
> +    if (obj != OBJECT(dev)) {
> +        error_setg(errp, "Only one fw_cfg device can be instantiated per "
> +                         "machine");
> +        return;
> +    }
> +
>      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);
>
>
> What seems to happen is that calling object_property_add_child() only
> succeeds for the first instance and so a simple comparison is enough to
> determine that the device already exists at FW_CFG_PATH. Or is this a
> fairly terrible (ab)use of the QOM APIs?

This has jogged my memory about how we ensure "at most one" for the
vmgenid device. Please see:

  vmgenid_realize()    [hw/acpi/vmgenid.c]
    find_vmgenid_dev() [include/hw/acpi/vmgenid.h]

... I was quite silly not to think of this on my own now, despite having
authored commit f92063028a0e ("hw/acpi/vmgenid: prevent more than one
vmgenid device", 2017-03-20) :/

Thanks,
Laszlo

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mark Cave-Ayland-2
On 19/06/17 18:09, Laszlo Ersek wrote:

>> What seems to happen is that calling object_property_add_child() only
>> succeeds for the first instance and so a simple comparison is enough to
>> determine that the device already exists at FW_CFG_PATH. Or is this a
>> fairly terrible (ab)use of the QOM APIs?
>
> This has jogged my memory about how we ensure "at most one" for the
> vmgenid device. Please see:
>
>   vmgenid_realize()    [hw/acpi/vmgenid.c]
>     find_vmgenid_dev() [include/hw/acpi/vmgenid.h]
>
> ... I was quite silly not to think of this on my own now, despite having
> authored commit f92063028a0e ("hw/acpi/vmgenid: prevent more than one
> vmgenid device", 2017-03-20) :/

Right that definitely helps - the following code seems to work correctly
when trying to instantiate a mixture of fw_cfg_io and/or fw_cfg_mem types:

if (!object_resolve_path_type("", TYPE_FW_CFG, NULL)) {
    error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG);
    return;
}

I've also copied the wording from the above commit to make everything
consistent. Does that seem okay? If so, I'll fold it into a v7 patchset.


ATB,

Mark.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Laszlo Ersek (Red Hat)
On 06/19/17 20:49, Mark Cave-Ayland wrote:

> On 19/06/17 18:09, Laszlo Ersek wrote:
>
>>> What seems to happen is that calling object_property_add_child() only
>>> succeeds for the first instance and so a simple comparison is enough to
>>> determine that the device already exists at FW_CFG_PATH. Or is this a
>>> fairly terrible (ab)use of the QOM APIs?
>>
>> This has jogged my memory about how we ensure "at most one" for the
>> vmgenid device. Please see:
>>
>>   vmgenid_realize()    [hw/acpi/vmgenid.c]
>>     find_vmgenid_dev() [include/hw/acpi/vmgenid.h]
>>
>> ... I was quite silly not to think of this on my own now, despite having
>> authored commit f92063028a0e ("hw/acpi/vmgenid: prevent more than one
>> vmgenid device", 2017-03-20) :/
>
> Right that definitely helps - the following code seems to work correctly
> when trying to instantiate a mixture of fw_cfg_io and/or fw_cfg_mem types:
>
> if (!object_resolve_path_type("", TYPE_FW_CFG, NULL)) {
>     error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG);
>     return;
> }
>
> I've also copied the wording from the above commit to make everything
> consistent. Does that seem okay? If so, I'll fold it into a v7 patchset.

It looks good to me, but please await Eduardo's reply as well.

In particular, it should be confirmed that object_resolve_path_type()
matches instances of *subclasses* as well (as I expect it would). Your
test results confirm that; let's make sure it is intentional behavior.
Eduardo (or someone else on the CC list), can you please comment on that?

Thanks!
Laszlo

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCHv6 2/5] fw_cfg: move setting of FW_CFG_VERSION_DMA bit to fw_cfg_init1()

Philippe Mathieu-Daudé
In reply to this post by Mark Cave-Ayland-2
On 06/19/2017 09:59 AM, Mark Cave-Ayland wrote:
> The setting of the FW_CFG_VERSION_DMA bit is the same across both the
> TYPE_FW_CFG_MEM and TYPE_FW_CFG_IO devices, so unify the logic in
> fw_cfg_init1().
>
> Signed-off-by: Mark Cave-Ayland <[hidden email]>
> Reviewed-by: Laszlo Ersek <[hidden email]>
> Tested-by: Laszlo Ersek <[hidden email]>

Reviewed-by: Philippe Mathieu-Daudé <[hidden email]>

> ---
>  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;
>  }
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mark Cave-Ayland-2
In reply to this post by Laszlo Ersek (Red Hat)
On 19/06/17 23:43, Laszlo Ersek wrote:

> It looks good to me, but please await Eduardo's reply as well.
>
> In particular, it should be confirmed that object_resolve_path_type()
> matches instances of *subclasses* as well (as I expect it would). Your
> test results confirm that; let's make sure it is intentional behavior.
> Eduardo (or someone else on the CC list), can you please comment on that?

Thanks Laszlo. Eduardo, can you confirm this is the correct behaviour?

I now have a v7 patchset ready to go (currently hosted at
https://github.com/mcayland/qemu/tree/fwcfg7 for the curious). Laszlo,
I've currently left off your Tested-by tag since I'm not sure it's still
valid for less-than-trivial changes - if you're happy for me to re-add
it before I send the v7 patchset to the list, please let me know.


ATB,

Mark.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Laszlo Ersek (Red Hat)
On 06/21/17 08:58, Mark Cave-Ayland wrote:

> On 19/06/17 23:43, Laszlo Ersek wrote:
>
>> It looks good to me, but please await Eduardo's reply as well.
>>
>> In particular, it should be confirmed that object_resolve_path_type()
>> matches instances of *subclasses* as well (as I expect it would). Your
>> test results confirm that; let's make sure it is intentional behavior.
>> Eduardo (or someone else on the CC list), can you please comment on that?
>
> Thanks Laszlo. Eduardo, can you confirm this is the correct behaviour?
>
> I now have a v7 patchset ready to go (currently hosted at
> https://github.com/mcayland/qemu/tree/fwcfg7 for the curious). Laszlo,
> I've currently left off your Tested-by tag since I'm not sure it's still
> valid for less-than-trivial changes - if you're happy for me to re-add
> it before I send the v7 patchset to the list, please let me know.

I intend to test v7 when you post it.

Thanks,
Laszlo

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Eduardo Habkost-2
On Wed, Jun 21, 2017 at 09:48:16AM +0200, Laszlo Ersek wrote:

> On 06/21/17 08:58, Mark Cave-Ayland wrote:
> > On 19/06/17 23:43, Laszlo Ersek wrote:
> >
> >> It looks good to me, but please await Eduardo's reply as well.
> >>
> >> In particular, it should be confirmed that object_resolve_path_type()
> >> matches instances of *subclasses* as well (as I expect it would). Your
> >> test results confirm that; let's make sure it is intentional behavior.
> >> Eduardo (or someone else on the CC list), can you please comment on that?
> >
> > Thanks Laszlo. Eduardo, can you confirm this is the correct behaviour?

Sorry for taking so long to reply.  Yes, it should be the correct
behavior.  It's how it's documented:

"This is similar to object_resolve_path.  However, when looking
for a partial path only matches that implement the given type are
considered.  This restricts the search and avoids spuriously
flagging matches as ambiguous."

(Key part here is "implement the given type").

Approach used by commit f92063028a "hw/acpi/vmgenid: prevent more
than one vmgenid device" looks good to me.

> >
> > I now have a v7 patchset ready to go (currently hosted at
> > https://github.com/mcayland/qemu/tree/fwcfg7 for the curious). Laszlo,
> > I've currently left off your Tested-by tag since I'm not sure it's still
> > valid for less-than-trivial changes - if you're happy for me to re-add
> > it before I send the v7 patchset to the list, please let me know.
>
> I intend to test v7 when you post it.

I still see the instance_init assert() in that branch (commit
17d75643f880).  Is that correct?

--
Eduardo

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mark Cave-Ayland-2
On 21/06/17 12:36, Eduardo Habkost wrote:

> On Wed, Jun 21, 2017 at 09:48:16AM +0200, Laszlo Ersek wrote:
>> On 06/21/17 08:58, Mark Cave-Ayland wrote:
>>> On 19/06/17 23:43, Laszlo Ersek wrote:
>>>
>>>> It looks good to me, but please await Eduardo's reply as well.
>>>>
>>>> In particular, it should be confirmed that object_resolve_path_type()
>>>> matches instances of *subclasses* as well (as I expect it would). Your
>>>> test results confirm that; let's make sure it is intentional behavior.
>>>> Eduardo (or someone else on the CC list), can you please comment on that?
>>>
>>> Thanks Laszlo. Eduardo, can you confirm this is the correct behaviour?
>
> Sorry for taking so long to reply.  Yes, it should be the correct
> behavior.  It's how it's documented:
>
> "This is similar to object_resolve_path.  However, when looking
> for a partial path only matches that implement the given type are
> considered.  This restricts the search and avoids spuriously
> flagging matches as ambiguous."
>
> (Key part here is "implement the given type").
>
> Approach used by commit f92063028a "hw/acpi/vmgenid: prevent more
> than one vmgenid device" looks good to me.

That is great news, thanks for confirming this.

>>>
>>> I now have a v7 patchset ready to go (currently hosted at
>>> https://github.com/mcayland/qemu/tree/fwcfg7 for the curious). Laszlo,
>>> I've currently left off your Tested-by tag since I'm not sure it's still
>>> valid for less-than-trivial changes - if you're happy for me to re-add
>>> it before I send the v7 patchset to the list, please let me know.
>>
>> I intend to test v7 when you post it.
>
> I still see the instance_init assert() in that branch (commit
> 17d75643f880).  Is that correct?

Yes that was the intention. In 17d75643f880 both the assert() and
object_property_add_child() are moved into the instance_init() function,
and then in the follow-up commit eddedb5 the assert() is removed from
instance_init() and the object_resolve_path_type() check added into
fw_cfg_init1() as part of its conversion into the
fw_cfg_common_realize() function.

One last question which might avoid a future v8 revision: does the error
handling in eddedb5 for the fw_cfg_*_realize() functions look correct?

The existing check for fw_cfg_file_slots_allocate() uses a local_err
Error variable, whereas I've seen a mixture of callers using both this
approach and using the errp Error variable directly. Is there any reason
to prefer one over the other? Currently I'm also using local_err in
order to keep the fw_cfg_*_realize() functions consistent.


ATB,

Mark.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Eduardo Habkost-2
On Wed, Jun 21, 2017 at 01:17:06PM +0100, Mark Cave-Ayland wrote:

> On 21/06/17 12:36, Eduardo Habkost wrote:
>
> > On Wed, Jun 21, 2017 at 09:48:16AM +0200, Laszlo Ersek wrote:
> >> On 06/21/17 08:58, Mark Cave-Ayland wrote:
> >>> On 19/06/17 23:43, Laszlo Ersek wrote:
> >>>
> >>>> It looks good to me, but please await Eduardo's reply as well.
> >>>>
> >>>> In particular, it should be confirmed that object_resolve_path_type()
> >>>> matches instances of *subclasses* as well (as I expect it would). Your
> >>>> test results confirm that; let's make sure it is intentional behavior.
> >>>> Eduardo (or someone else on the CC list), can you please comment on that?
> >>>
> >>> Thanks Laszlo. Eduardo, can you confirm this is the correct behaviour?
> >
> > Sorry for taking so long to reply.  Yes, it should be the correct
> > behavior.  It's how it's documented:
> >
> > "This is similar to object_resolve_path.  However, when looking
> > for a partial path only matches that implement the given type are
> > considered.  This restricts the search and avoids spuriously
> > flagging matches as ambiguous."
> >
> > (Key part here is "implement the given type").
> >
> > Approach used by commit f92063028a "hw/acpi/vmgenid: prevent more
> > than one vmgenid device" looks good to me.
>
> That is great news, thanks for confirming this.
>
> >>>
> >>> I now have a v7 patchset ready to go (currently hosted at
> >>> https://github.com/mcayland/qemu/tree/fwcfg7 for the curious). Laszlo,
> >>> I've currently left off your Tested-by tag since I'm not sure it's still
> >>> valid for less-than-trivial changes - if you're happy for me to re-add
> >>> it before I send the v7 patchset to the list, please let me know.
> >>
> >> I intend to test v7 when you post it.
> >
> > I still see the instance_init assert() in that branch (commit
> > 17d75643f880).  Is that correct?
>
> Yes that was the intention. In 17d75643f880 both the assert() and
> object_property_add_child() are moved into the instance_init() function,
> and then in the follow-up commit eddedb5 the assert() is removed from
> instance_init() and the object_resolve_path_type() check added into
> fw_cfg_init1() as part of its conversion into the
> fw_cfg_common_realize() function.

We can't move assert() + linking to instance_init even if it's
just temporary, as it makes device-list-properties crash.

Just linking in instance_init is also a problem, because
instance_init should never affect global QEMU state.

You could move linking to realize as well, but: just like you
already moved sysbus_add_io() calls outside realize, I believe
linking belongs outside realize too.  I need to re-read the
discussion threads to understand the motivation behind that.

>
> One last question which might avoid a future v8 revision: does the error
> handling in eddedb5 for the fw_cfg_*_realize() functions look correct?

The error handling looks correct to me.

>
> The existing check for fw_cfg_file_slots_allocate() uses a local_err
> Error variable, whereas I've seen a mixture of callers using both this
> approach and using the errp Error variable directly. Is there any reason
> to prefer one over the other? Currently I'm also using local_err in
> order to keep the fw_cfg_*_realize() functions consistent.

You need a local_err variable if you need to handle/check for
errors before propagating them.

Quoting qapi/error.h:

  Create a new error and pass it to the caller:
      error_setg(errp, "situation normal, all fouled up");

  Call a function and receive an error from it:
      Error *err = NULL;
      foo(arg, &err);
      if (err) {
          handle the error...
      }

  [...]

  Receive an error and pass it on to the caller:
      Error *err = NULL;
      foo(arg, &err);
      if (err) {
          handle the error...
          error_propagate(errp, err);
      }
  where Error **errp is a parameter, by convention the last one.

  Do *not* "optimize" this to
      foo(arg, errp);
      if (*errp) { // WRONG!
          handle the error...
      }
  because errp may be NULL!

  But when all you do with the error is pass it on, please use
      foo(arg, errp);
  for readability.

In fw_cfg_*_realize() you can call fw_cfg_common_realize(dev,
errp) directly, but it won't be a problem if you use local_err
just to keep it consistent with the other error checks in the
function.

--
Eduardo

12
Loading...