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

[Qemu-devel] [PATCHv6 0/5] 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>

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

[Qemu-devel] [PATCHv6 1/5] 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>
Tested-by: Laszlo Ersek <lersek@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

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

[Eduardo Habkost]

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 <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Tested-by: Laszlo Ersek <lersek@redhat.com>

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

-- 
Eduardo

[Qemu-devel] [PATCHv6 2/5] 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>
Tested-by: Laszlo Ersek <lersek@redhat.com>
---
 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

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

[Eduardo Habkost]

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 <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Tested-by: Laszlo Ersek <lersek@redhat.com>

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

-- 
Eduardo

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

[Philippe Mathieu-Daudé]

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 <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Tested-by: Laszlo Ersek <lersek@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;
>  }
>
>

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

[Mark Cave-Ayland]

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 <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>
---
 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

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

[Eduardo Habkost]

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 <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  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

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

[Laszlo Ersek]

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 <mark.cave-ayland@ilande.co.uk>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> Tested-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  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
>>
> 

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

[Mark Cave-Ayland]

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 <mark.cave-ayland@ilande.co.uk>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> Tested-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  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.

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

[Laszlo Ersek]

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 <mark.cave-ayland@ilande.co.uk>
>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>> Tested-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>  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

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

[Mark Cave-Ayland]

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.

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

[Laszlo Ersek]

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

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

[Mark Cave-Ayland]

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.

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

[Laszlo Ersek]

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

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

[Eduardo Habkost]

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

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

[Mark Cave-Ayland]

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.

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

[Eduardo Habkost]

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

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

[Mark Cave-Ayland]

On 21/06/17 14:23, Eduardo Habkost wrote:

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

Ultimately the question we're trying to answer is "has someone
instantiated another fw_cfg device for this machine?" and the way it
works currently is that fw_cfg_init_io() and fw_cfg_init_mem() attach
the fw_cfg device to the /machine object and then check after realize
whether a /machine/fw_cfg device already exists, aborting if it does.

So in the current implementation we're not actually concerned with the
placement of fw_cfg within the model itself, and indeed with a fixed
location in the QOM tree it's already completely wrong. If you take a
look at the QOM tree for the sparc/sparc64/ppc machines you'll see that
they really are very far from reality.

For me the use of object_resolve_path_type() during realize is a good
solution since regardless of the location of the fw_cfg we can always
detect whether we have more than one device instance.

However what seems unappealing about this is that while all existing
users which use fw_cfg_init_io() and fw_cfg_init_mem() are fine, in the
case where I am wiring up the device myself then for my sun4u example
the code looks like this:

dev = qdev_create(NULL, TYPE_FW_CFG_IO);
...
qdev_init_nofail(dev);
memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT,
                            &FW_CFG_IO(dev)->comb_iomem);

Here you can see that the device is active because it is mapped into the
correct IO address space, but notice I have forgotten to link it to the
QOM /machine object myself. Hence I can instantiate and map as many
instances as I like and never trigger the duplicate instance check which
makes the check fairly ineffective.

This makes me think that I'm still misunderstanding something, so I'd be
grateful for any further suggestions.

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

Thanks for the review, in that case I will leave it in its current form.


ATB,

Mark.

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

[Eduardo Habkost]

On Fri, Jun 23, 2017 at 09:12:01AM +0100, Mark Cave-Ayland wrote:
> On 21/06/17 14:23, Eduardo Habkost wrote:
> 
> >>>>> 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.
> 
> Ultimately the question we're trying to answer is "has someone
> instantiated another fw_cfg device for this machine?" and the way it
> works currently is that fw_cfg_init_io() and fw_cfg_init_mem() attach
> the fw_cfg device to the /machine object and then check after realize
> whether a /machine/fw_cfg device already exists, aborting if it does.
> 
> So in the current implementation we're not actually concerned with the
> placement of fw_cfg within the model itself, and indeed with a fixed
> location in the QOM tree it's already completely wrong. If you take a
> look at the QOM tree for the sparc/sparc64/ppc machines you'll see that
> they really are very far from reality.
> 
> For me the use of object_resolve_path_type() during realize is a good
> solution since regardless of the location of the fw_cfg we can always
> detect whether we have more than one device instance.
> 
> However what seems unappealing about this is that while all existing
> users which use fw_cfg_init_io() and fw_cfg_init_mem() are fine, in the
> case where I am wiring up the device myself then for my sun4u example
> the code looks like this:
> 
> dev = qdev_create(NULL, TYPE_FW_CFG_IO);
> ...
> qdev_init_nofail(dev);
> memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT,
>                             &FW_CFG_IO(dev)->comb_iomem);
> 
> Here you can see that the device is active because it is mapped into the
> correct IO address space, but notice I have forgotten to link it to the
> QOM /machine object myself. Hence I can instantiate and map as many
> instances as I like and never trigger the duplicate instance check which
> makes the check fairly ineffective.

This is a good point, but I have a question about that: will something
break if a machine decides to create two fw_cfg objects and map them to
different addresses?  If it won't break, I see no reason to try to
enforce a single instance in the device code.  If it will break, then a
check in realize is still a hack, but might be a good enough solution.

-- 
Eduardo

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

[Laszlo Ersek]

On 06/23/17 13:50, Eduardo Habkost wrote:
> On Fri, Jun 23, 2017 at 09:12:01AM +0100, Mark Cave-Ayland wrote:
>> On 21/06/17 14:23, Eduardo Habkost wrote:
>>
>>>>>>> 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.
>>
>> Ultimately the question we're trying to answer is "has someone
>> instantiated another fw_cfg device for this machine?" and the way it
>> works currently is that fw_cfg_init_io() and fw_cfg_init_mem() attach
>> the fw_cfg device to the /machine object and then check after realize
>> whether a /machine/fw_cfg device already exists, aborting if it does.
>>
>> So in the current implementation we're not actually concerned with the
>> placement of fw_cfg within the model itself, and indeed with a fixed
>> location in the QOM tree it's already completely wrong. If you take a
>> look at the QOM tree for the sparc/sparc64/ppc machines you'll see that
>> they really are very far from reality.
>>
>> For me the use of object_resolve_path_type() during realize is a good
>> solution since regardless of the location of the fw_cfg we can always
>> detect whether we have more than one device instance.
>>
>> However what seems unappealing about this is that while all existing
>> users which use fw_cfg_init_io() and fw_cfg_init_mem() are fine, in the
>> case where I am wiring up the device myself then for my sun4u example
>> the code looks like this:
>>
>> dev = qdev_create(NULL, TYPE_FW_CFG_IO);
>> ...
>> qdev_init_nofail(dev);
>> memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT,
>>                             &FW_CFG_IO(dev)->comb_iomem);
>>
>> Here you can see that the device is active because it is mapped into the
>> correct IO address space, but notice I have forgotten to link it to the
>> QOM /machine object myself. Hence I can instantiate and map as many
>> instances as I like and never trigger the duplicate instance check which
>> makes the check fairly ineffective.
> 
> This is a good point, but I have a question about that: will something
> break if a machine decides to create two fw_cfg objects and map them to
> different addresses?  If it won't break, I see no reason to try to
> enforce a single instance in the device code.  If it will break, then a
> check in realize is still a hack, but might be a good enough solution.
> 

(1) For the guest, it makes no sense to encounter two fw_cfg devices.
Also, a lot of the existent code in QEMU assumes at most one fw_cfg
device (for example, there is some related ACPI generation).

(2) Relatedly, the fw_cfg_find() helper function is used quite widely,
and it shouldn't break -- either due to more-than-one device instances,
or due to the one fw_cfg device being linked under a path that is
different from FW_CFG_PATH.

Thanks
Laszlo

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

[Eduardo Habkost]

On Fri, Jun 23, 2017 at 05:52:03PM +0200, Laszlo Ersek wrote:
> On 06/23/17 13:50, Eduardo Habkost wrote:
> > On Fri, Jun 23, 2017 at 09:12:01AM +0100, Mark Cave-Ayland wrote:
> >> On 21/06/17 14:23, Eduardo Habkost wrote:
> >>
> >>>>>>> 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.
> >>
> >> Ultimately the question we're trying to answer is "has someone
> >> instantiated another fw_cfg device for this machine?" and the way it
> >> works currently is that fw_cfg_init_io() and fw_cfg_init_mem() attach
> >> the fw_cfg device to the /machine object and then check after realize
> >> whether a /machine/fw_cfg device already exists, aborting if it does.
> >>
> >> So in the current implementation we're not actually concerned with the
> >> placement of fw_cfg within the model itself, and indeed with a fixed
> >> location in the QOM tree it's already completely wrong. If you take a
> >> look at the QOM tree for the sparc/sparc64/ppc machines you'll see that
> >> they really are very far from reality.
> >>
> >> For me the use of object_resolve_path_type() during realize is a good
> >> solution since regardless of the location of the fw_cfg we can always
> >> detect whether we have more than one device instance.
> >>
> >> However what seems unappealing about this is that while all existing
> >> users which use fw_cfg_init_io() and fw_cfg_init_mem() are fine, in the
> >> case where I am wiring up the device myself then for my sun4u example
> >> the code looks like this:
> >>
> >> dev = qdev_create(NULL, TYPE_FW_CFG_IO);
> >> ...
> >> qdev_init_nofail(dev);
> >> memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT,
> >>                             &FW_CFG_IO(dev)->comb_iomem);
> >>
> >> Here you can see that the device is active because it is mapped into the
> >> correct IO address space, but notice I have forgotten to link it to the
> >> QOM /machine object myself. Hence I can instantiate and map as many
> >> instances as I like and never trigger the duplicate instance check which
> >> makes the check fairly ineffective.
> > 
> > This is a good point, but I have a question about that: will something
> > break if a machine decides to create two fw_cfg objects and map them to
> > different addresses?  If it won't break, I see no reason to try to
> > enforce a single instance in the device code.  If it will break, then a
> > check in realize is still a hack, but might be a good enough solution.
> > 
> 
> (1) For the guest, it makes no sense to encounter two fw_cfg devices.
> Also, a lot of the existent code in QEMU assumes at most one fw_cfg
> device (for example, there is some related ACPI generation).

This is an argument for making board code ensure there's only one
device, and possibly for providing a helper that board code can use.
But it doesn't require validation on realize.

> 
> (2) Relatedly, the fw_cfg_find() helper function is used quite widely,
> and it shouldn't break -- either due to more-than-one device instances,
> or due to the one fw_cfg device being linked under a path that is
> different from FW_CFG_PATH.

This is also an argument for providing a helper that ensures
fw_cfg_find() will work, but doesn't require validation on realize.


All that said, I don't have a strong argument against doing it on
realize, except my gut feeling that this is not how qdev was
designed[1].  If doing it on realize is the simplest way to do it, I
won't be the one complaining.

[1] I believe the original intent was to make every single device
    user-creatable and define boards in a declarative way in config
    files.  We are very far from that goal.

-- 
Eduardo

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

[Laszlo Ersek]

On 06/23/17 18:10, Eduardo Habkost wrote:
> On Fri, Jun 23, 2017 at 05:52:03PM +0200, Laszlo Ersek wrote:
>> On 06/23/17 13:50, Eduardo Habkost wrote:
>>> On Fri, Jun 23, 2017 at 09:12:01AM +0100, Mark Cave-Ayland wrote:
>>>> On 21/06/17 14:23, Eduardo Habkost wrote:
>>>>
>>>>>>>>> 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.
>>>>
>>>> Ultimately the question we're trying to answer is "has someone
>>>> instantiated another fw_cfg device for this machine?" and the way it
>>>> works currently is that fw_cfg_init_io() and fw_cfg_init_mem() attach
>>>> the fw_cfg device to the /machine object and then check after realize
>>>> whether a /machine/fw_cfg device already exists, aborting if it does.
>>>>
>>>> So in the current implementation we're not actually concerned with the
>>>> placement of fw_cfg within the model itself, and indeed with a fixed
>>>> location in the QOM tree it's already completely wrong. If you take a
>>>> look at the QOM tree for the sparc/sparc64/ppc machines you'll see that
>>>> they really are very far from reality.
>>>>
>>>> For me the use of object_resolve_path_type() during realize is a good
>>>> solution since regardless of the location of the fw_cfg we can always
>>>> detect whether we have more than one device instance.
>>>>
>>>> However what seems unappealing about this is that while all existing
>>>> users which use fw_cfg_init_io() and fw_cfg_init_mem() are fine, in the
>>>> case where I am wiring up the device myself then for my sun4u example
>>>> the code looks like this:
>>>>
>>>> dev = qdev_create(NULL, TYPE_FW_CFG_IO);
>>>> ...
>>>> qdev_init_nofail(dev);
>>>> memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT,
>>>>                             &FW_CFG_IO(dev)->comb_iomem);
>>>>
>>>> Here you can see that the device is active because it is mapped into the
>>>> correct IO address space, but notice I have forgotten to link it to the
>>>> QOM /machine object myself. Hence I can instantiate and map as many
>>>> instances as I like and never trigger the duplicate instance check which
>>>> makes the check fairly ineffective.
>>>
>>> This is a good point, but I have a question about that: will something
>>> break if a machine decides to create two fw_cfg objects and map them to
>>> different addresses?  If it won't break, I see no reason to try to
>>> enforce a single instance in the device code.  If it will break, then a
>>> check in realize is still a hack, but might be a good enough solution.
>>>
>>
>> (1) For the guest, it makes no sense to encounter two fw_cfg devices.
>> Also, a lot of the existent code in QEMU assumes at most one fw_cfg
>> device (for example, there is some related ACPI generation).
> 
> This is an argument for making board code ensure there's only one
> device, and possibly for providing a helper that board code can use.
> But it doesn't require validation on realize.
> 
>>
>> (2) Relatedly, the fw_cfg_find() helper function is used quite widely,
>> and it shouldn't break -- either due to more-than-one device instances,
>> or due to the one fw_cfg device being linked under a path that is
>> different from FW_CFG_PATH.
> 
> This is also an argument for providing a helper that ensures
> fw_cfg_find() will work, but doesn't require validation on realize.

I agree.

If you read back the discussion threads under the earlier versions of
this patch set, you'll find that I argued for keeping the unicity stuff
-- and possibly some other stuff -- that currently resides in the
fw_cfg_init1() *helper* function outside of device code (realize or
otherwise).

I didn't disagree with the reorganization of the code, but I suggested
to preserve this stuff in helper functions, which board code could call.
This would clearly place the same burden on Mark's new sun4u board code
-- i.e., that new board code would *also* have to call these helper
function(s). Mark disliked this board requirement (he only wanted to
instantiate the device and be done with it, in board code); and we went
from there.

Really, please go back to the earlier discussion around fw_cfg_init1()
and you'll see my original point (which matches what you just voiced).

> All that said, I don't have a strong argument against doing it on
> realize, except my gut feeling that this is not how qdev was
> designed[1].  If doing it on realize is the simplest way to do it, I
> won't be the one complaining.
> 
> [1] I believe the original intent was to make every single device
>     user-creatable and define boards in a declarative way in config
>     files.  We are very far from that goal.

I'm fine either way, I just wanted to accommodate Mark's preference,
because he seemed to strongly dislike having to call any helper
functions from board code, beyond initing and realizing the device.

This is my recollection of the earlier discussion anyway.

Thanks
Laszlo

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

[Eduardo Habkost]

On Fri, Jun 23, 2017 at 06:48:40PM +0200, Laszlo Ersek wrote:
> On 06/23/17 18:10, Eduardo Habkost wrote:
> > On Fri, Jun 23, 2017 at 05:52:03PM +0200, Laszlo Ersek wrote:
> >> On 06/23/17 13:50, Eduardo Habkost wrote:
> >>> On Fri, Jun 23, 2017 at 09:12:01AM +0100, Mark Cave-Ayland wrote:
> >>>> On 21/06/17 14:23, Eduardo Habkost wrote:
> >>>>
> >>>>>>>>> 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.
> >>>>
> >>>> Ultimately the question we're trying to answer is "has someone
> >>>> instantiated another fw_cfg device for this machine?" and the way it
> >>>> works currently is that fw_cfg_init_io() and fw_cfg_init_mem() attach
> >>>> the fw_cfg device to the /machine object and then check after realize
> >>>> whether a /machine/fw_cfg device already exists, aborting if it does.
> >>>>
> >>>> So in the current implementation we're not actually concerned with the
> >>>> placement of fw_cfg within the model itself, and indeed with a fixed
> >>>> location in the QOM tree it's already completely wrong. If you take a
> >>>> look at the QOM tree for the sparc/sparc64/ppc machines you'll see that
> >>>> they really are very far from reality.
> >>>>
> >>>> For me the use of object_resolve_path_type() during realize is a good
> >>>> solution since regardless of the location of the fw_cfg we can always
> >>>> detect whether we have more than one device instance.
> >>>>
> >>>> However what seems unappealing about this is that while all existing
> >>>> users which use fw_cfg_init_io() and fw_cfg_init_mem() are fine, in the
> >>>> case where I am wiring up the device myself then for my sun4u example
> >>>> the code looks like this:
> >>>>
> >>>> dev = qdev_create(NULL, TYPE_FW_CFG_IO);
> >>>> ...
> >>>> qdev_init_nofail(dev);
> >>>> memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT,
> >>>>                             &FW_CFG_IO(dev)->comb_iomem);
> >>>>
> >>>> Here you can see that the device is active because it is mapped into the
> >>>> correct IO address space, but notice I have forgotten to link it to the
> >>>> QOM /machine object myself. Hence I can instantiate and map as many
> >>>> instances as I like and never trigger the duplicate instance check which
> >>>> makes the check fairly ineffective.
> >>>
> >>> This is a good point, but I have a question about that: will something
> >>> break if a machine decides to create two fw_cfg objects and map them to
> >>> different addresses?  If it won't break, I see no reason to try to
> >>> enforce a single instance in the device code.  If it will break, then a
> >>> check in realize is still a hack, but might be a good enough solution.
> >>>
> >>
> >> (1) For the guest, it makes no sense to encounter two fw_cfg devices.
> >> Also, a lot of the existent code in QEMU assumes at most one fw_cfg
> >> device (for example, there is some related ACPI generation).
> > 
> > This is an argument for making board code ensure there's only one
> > device, and possibly for providing a helper that board code can use.
> > But it doesn't require validation on realize.
> > 
> >>
> >> (2) Relatedly, the fw_cfg_find() helper function is used quite widely,
> >> and it shouldn't break -- either due to more-than-one device instances,
> >> or due to the one fw_cfg device being linked under a path that is
> >> different from FW_CFG_PATH.
> > 
> > This is also an argument for providing a helper that ensures
> > fw_cfg_find() will work, but doesn't require validation on realize.
> 
> I agree.
> 
> If you read back the discussion threads under the earlier versions of
> this patch set, you'll find that I argued for keeping the unicity stuff
> -- and possibly some other stuff -- that currently resides in the
> fw_cfg_init1() *helper* function outside of device code (realize or
> otherwise).
> 
> I didn't disagree with the reorganization of the code, but I suggested
> to preserve this stuff in helper functions, which board code could call.
> This would clearly place the same burden on Mark's new sun4u board code
> -- i.e., that new board code would *also* have to call these helper
> function(s). Mark disliked this board requirement (he only wanted to
> instantiate the device and be done with it, in board code); and we went
> from there.
> 
> Really, please go back to the earlier discussion around fw_cfg_init1()
> and you'll see my original point (which matches what you just voiced).

Yep.  I was just not sure validation on realize was necessary or
convenient.  It looks like we agree it is just convenient.


> 
> > All that said, I don't have a strong argument against doing it on
> > realize, except my gut feeling that this is not how qdev was
> > designed[1].  If doing it on realize is the simplest way to do it, I
> > won't be the one complaining.
> > 
> > [1] I believe the original intent was to make every single device
> >     user-creatable and define boards in a declarative way in config
> >     files.  We are very far from that goal.
> 
> I'm fine either way, I just wanted to accommodate Mark's preference,
> because he seemed to strongly dislike having to call any helper
> functions from board code, beyond initing and realizing the device.
> 
> This is my recollection of the earlier discussion anyway.

I think we are in agreement, then.  If there are no objections from
others against doing object_property_add_child() on realize, I'm also OK
with that.

-- 
Eduardo

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

[Mark Cave-Ayland]

On 23/06/17 19:50, Eduardo Habkost wrote:

>> Really, please go back to the earlier discussion around fw_cfg_init1()
>> and you'll see my original point (which matches what you just voiced).
> 
> Yep.  I was just not sure validation on realize was necessary or
> convenient.  It looks like we agree it is just convenient.
> 
> 
>>
>>> All that said, I don't have a strong argument against doing it on
>>> realize, except my gut feeling that this is not how qdev was
>>> designed[1].  If doing it on realize is the simplest way to do it, I
>>> won't be the one complaining.
>>>
>>> [1] I believe the original intent was to make every single device
>>>     user-creatable and define boards in a declarative way in config
>>>     files.  We are very far from that goal.
>>
>> I'm fine either way, I just wanted to accommodate Mark's preference,
>> because he seemed to strongly dislike having to call any helper
>> functions from board code, beyond initing and realizing the device.
>>
>> This is my recollection of the earlier discussion anyway.
> 
> I think we are in agreement, then.  If there are no objections from
> others against doing object_property_add_child() on realize, I'm also OK
> with that.

Just to clarify here that my objection wasn't so much about calling
helper functions from board code, it was that as the current patch
exposes the MemoryRegions in TYPE_FW_CFG_IO and TYPE_FW_CFG_MEM then as
per my example where the machine link is omitted then the check becomes
useless.

I can see that device_set_realized() will always set the device parent
to /machine/unattached before calling the realize function if the device
doesn't have a parent. So is it even possible to add the device via
object_property_add_child() to the machine during realize? Or could it
be done by making /machine/fw_cfg an alias to its real location in the
QOM tree at realize time without breaking the object_resolve_path_type()
check?

The other interesting option to explore is that since fw_cfg already has
a machine_ready notifier, the check could be moved there similar to as
done in hw/core/machine.c's error_on_sysbus_device() if the check
shouldn't be present in realize. That still doesn't answer the question
as to how to enforce that the device is correctly linked to the machine
though.


ATB,

Mark.

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

[Eduardo Habkost]

On Sun, Jun 25, 2017 at 07:58:04PM +0100, Mark Cave-Ayland wrote:
> On 23/06/17 19:50, Eduardo Habkost wrote:
> 
> >> Really, please go back to the earlier discussion around fw_cfg_init1()
> >> and you'll see my original point (which matches what you just voiced).
> > 
> > Yep.  I was just not sure validation on realize was necessary or
> > convenient.  It looks like we agree it is just convenient.
> > 
> > 
> >>
> >>> All that said, I don't have a strong argument against doing it on
> >>> realize, except my gut feeling that this is not how qdev was
> >>> designed[1].  If doing it on realize is the simplest way to do it, I
> >>> won't be the one complaining.
> >>>
> >>> [1] I believe the original intent was to make every single device
> >>>     user-creatable and define boards in a declarative way in config
> >>>     files.  We are very far from that goal.
> >>
> >> I'm fine either way, I just wanted to accommodate Mark's preference,
> >> because he seemed to strongly dislike having to call any helper
> >> functions from board code, beyond initing and realizing the device.
> >>
> >> This is my recollection of the earlier discussion anyway.
> > 
> > I think we are in agreement, then.  If there are no objections from
> > others against doing object_property_add_child() on realize, I'm also OK
> > with that.
> 
> Just to clarify here that my objection wasn't so much about calling
> helper functions from board code, it was that as the current patch
> exposes the MemoryRegions in TYPE_FW_CFG_IO and TYPE_FW_CFG_MEM then as
> per my example where the machine link is omitted then the check becomes
> useless.

I don't understand this part.  When and why would the check become
useless?

> 
> I can see that device_set_realized() will always set the device parent
> to /machine/unattached before calling the realize function if the device
> doesn't have a parent. So is it even possible to add the device via
> object_property_add_child() to the machine during realize? Or could it
> be done by making /machine/fw_cfg an alias to its real location in the
> QOM tree at realize time without breaking the object_resolve_path_type()
> check?

Well, if we can't do object_property_add_child() on ->instance_init()
and doing it on ->realize() would require a more complex solution
involving QOM links, I believe the simplest solution is to provide a
helper function.

> 
> The other interesting option to explore is that since fw_cfg already has
> a machine_ready notifier, the check could be moved there similar to as
> done in hw/core/machine.c's error_on_sysbus_device() if the check
> shouldn't be present in realize. That still doesn't answer the question
> as to how to enforce that the device is correctly linked to the machine
> though.

I think both manually mapping+linking from board code or calling a
helper function from board code would be acceptable.

-- 
Eduardo

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

[Mark Cave-Ayland]

On 27/06/17 01:49, Eduardo Habkost wrote:

> On Sun, Jun 25, 2017 at 07:58:04PM +0100, Mark Cave-Ayland wrote:
>> On 23/06/17 19:50, Eduardo Habkost wrote:
>>
>>>> Really, please go back to the earlier discussion around fw_cfg_init1()
>>>> and you'll see my original point (which matches what you just voiced).
>>>
>>> Yep.  I was just not sure validation on realize was necessary or
>>> convenient.  It looks like we agree it is just convenient.
>>>
>>>
>>>>
>>>>> All that said, I don't have a strong argument against doing it on
>>>>> realize, except my gut feeling that this is not how qdev was
>>>>> designed[1].  If doing it on realize is the simplest way to do it, I
>>>>> won't be the one complaining.
>>>>>
>>>>> [1] I believe the original intent was to make every single device
>>>>>     user-creatable and define boards in a declarative way in config
>>>>>     files.  We are very far from that goal.
>>>>
>>>> I'm fine either way, I just wanted to accommodate Mark's preference,
>>>> because he seemed to strongly dislike having to call any helper
>>>> functions from board code, beyond initing and realizing the device.
>>>>
>>>> This is my recollection of the earlier discussion anyway.
>>>
>>> I think we are in agreement, then.  If there are no objections from
>>> others against doing object_property_add_child() on realize, I'm also OK
>>> with that.
>>
>> Just to clarify here that my objection wasn't so much about calling
>> helper functions from board code, it was that as the current patch
>> exposes the MemoryRegions in TYPE_FW_CFG_IO and TYPE_FW_CFG_MEM then as
>> per my example where the machine link is omitted then the check becomes
>> useless.
> 
> I don't understand this part.  When and why would the check become
> useless?

Well because when using the standard QEMU pattern of
qdev_create()...qdev_init_nofail() it is possible to realize the device
and wire up its MemoryRegions manually as I have done with the sun4u
(i.e. it will respond to accesses) multiple times without calling the
helper function and triggering the check. The ingenious part here is
that only the developers who aren't aware that they have to manually
call the helper function will be the ones who get caught out trying to
instantiate the device multiple times ;)

I'm also aware that with QOM we've got 2 conflicting ideas with fw_cfg:
on the one hand we're saying that QOM hierarchy should at some level
represent the topology of the machine, i.e. for sun4u the fw_cfg device
should appear under the ebus device. whereas at the moment we assume a
fixed path of FW_CFG_PATH.

Based upon this I been thinking along the following lines:

1) Alter fw_cfg_find() to use object_resolve_path_type("", TYPE_FW_CFG,
NULL)

This solves the issue of allowing the QOM tree to best represent the
device topology as it will allow fw_cfg_find() to locate the fw_cfg
device regardless of its location, and hence it can be placed as
appropriate for each machine.

2) Add a check at the top of fw_cfg_common_realize() along the lines of:

if (object_unattached(OBJECT(dev)) {
    error_setg(errp, "%s device must be explicitly added as a child "
               object before realize", TYPE_FW_CFG);
    return;
}

This makes it obvious to the developer that they MUST wire up the fw_cfg
device to the machine before realize, and then if more than one device
is instantiated we can still trigger the existing error from my v7 branch:

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

I prefer this method because it is impossible for a developer to
accidentally realize the fw_cfg device without attaching it to the
machine first (i.e. fw_cfg_find() will always succeed if this is the
case if altered as per 1) above) and it can only be realized once. And
following the POLA the device can be wired up using
object_property_add_child() as per the numerous existing examples
throughout the QEMU codebase.

Now the minor issue with 2) is that I can't find an easy way to
determine if the device is unattached at realize time. I've tried
something like this:

if (!object_resolve_path_type("/machine/unattached",
    TYPE_FW_CFG, NULL)) {
   ...
   ...
}

However that doesn't work because as the rules differ between partial
and absolute path resolution, we end up resolving the container itself
as opposed to iterating down through the QOM tree. Is there an existing
solution for this or would I need to come up with something that
iterates over the container children to search for a TYPE_FW_CFG device
myself?


ATB,

Mark.

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

[Igor Mammedov]

On Wed, 28 Jun 2017 08:09:35 +0100
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:

> On 27/06/17 01:49, Eduardo Habkost wrote:
> 
> > On Sun, Jun 25, 2017 at 07:58:04PM +0100, Mark Cave-Ayland wrote:  
> >> On 23/06/17 19:50, Eduardo Habkost wrote:
> >>  
> >>>> Really, please go back to the earlier discussion around fw_cfg_init1()
> >>>> and you'll see my original point (which matches what you just voiced).  
> >>>
> >>> Yep.  I was just not sure validation on realize was necessary or
> >>> convenient.  It looks like we agree it is just convenient.
> >>>
> >>>  
> >>>>  
> >>>>> All that said, I don't have a strong argument against doing it on
> >>>>> realize, except my gut feeling that this is not how qdev was
> >>>>> designed[1].  If doing it on realize is the simplest way to do it, I
> >>>>> won't be the one complaining.
> >>>>>
> >>>>> [1] I believe the original intent was to make every single device
> >>>>>     user-creatable and define boards in a declarative way in config
> >>>>>     files.  We are very far from that goal.  
> >>>>
> >>>> I'm fine either way, I just wanted to accommodate Mark's preference,
> >>>> because he seemed to strongly dislike having to call any helper
> >>>> functions from board code, beyond initing and realizing the device.
> >>>>
> >>>> This is my recollection of the earlier discussion anyway.  
> >>>
> >>> I think we are in agreement, then.  If there are no objections from
> >>> others against doing object_property_add_child() on realize, I'm also OK
> >>> with that.  
> >>
> >> Just to clarify here that my objection wasn't so much about calling
> >> helper functions from board code, it was that as the current patch
> >> exposes the MemoryRegions in TYPE_FW_CFG_IO and TYPE_FW_CFG_MEM then as
> >> per my example where the machine link is omitted then the check becomes
> >> useless.  
> > 
> > I don't understand this part.  When and why would the check become
> > useless?  
> 
> Well because when using the standard QEMU pattern of
> qdev_create()...qdev_init_nofail() it is possible to realize the device
> and wire up its MemoryRegions manually as I have done with the sun4u
> (i.e. it will respond to accesses) multiple times without calling the
> helper function and triggering the check. The ingenious part here is
> that only the developers who aren't aware that they have to manually
> call the helper function will be the ones who get caught out trying to
> instantiate the device multiple times ;)
> 
> I'm also aware that with QOM we've got 2 conflicting ideas with fw_cfg:
> on the one hand we're saying that QOM hierarchy should at some level
> represent the topology of the machine, i.e. for sun4u the fw_cfg device
> should appear under the ebus device. whereas at the moment we assume a
> fixed path of FW_CFG_PATH.
> 
> Based upon this I been thinking along the following lines:
> 
> 1) Alter fw_cfg_find() to use object_resolve_path_type("", TYPE_FW_CFG,
> NULL)
I'd make use of the 3rd argument &ambiguous and assert on it
 
> This solves the issue of allowing the QOM tree to best represent the
> device topology as it will allow fw_cfg_find() to locate the fw_cfg
> device regardless of its location, and hence it can be placed as
> appropriate for each machine.
looks reasonable, that's what we were doing in a bunch of other cases


> 2) Add a check at the top of fw_cfg_common_realize() along the lines of:
> 
> if (object_unattached(OBJECT(dev)) {
>     error_setg(errp, "%s device must be explicitly added as a child "
>                object before realize", TYPE_FW_CFG);
>     return;
> }
that would be never trigger as ancestor's DEVICE.realize() always sets
parent if it wasn't set manually before child's realize is called.

in other words it's guarantied that device has parent by the time
realize() is run by device_set_realized()


> This makes it obvious to the developer that they MUST wire up the fw_cfg
> device to the machine before realize, and then if more than one device
> is instantiated we can still trigger the existing error from my v7 branch:
> 
> if (!object_resolve_path_type("", TYPE_FW_CFG, NULL)) {
>     error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG);
>     return;
> }
reuse fw_cfg_find() here?

> I prefer this method because it is impossible for a developer to
> accidentally realize the fw_cfg device without attaching it to the
> machine first (i.e. fw_cfg_find() will always succeed if this is the
> case if altered as per 1) above) and it can only be realized once. And
> following the POLA the device can be wired up using
> object_property_add_child() as per the numerous existing examples
> throughout the QEMU codebase.
> 
> Now the minor issue with 2) is that I can't find an easy way to
> determine if the device is unattached at realize time. I've tried
> something like this:
> 
> if (!object_resolve_path_type("/machine/unattached",
>     TYPE_FW_CFG, NULL)) {
>    ...
>    ...
> }
> 
> However that doesn't work because as the rules differ between partial
> and absolute path resolution, we end up resolving the container itself
> as opposed to iterating down through the QOM tree. Is there an existing
> solution for this or would I need to come up with something that
> iterates over the container children to search for a TYPE_FW_CFG device
> myself?
> 
> 
> ATB,
> 
> Mark.
> 
> 

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

[Laszlo Ersek]

On 06/28/17 16:12, Igor Mammedov wrote:
> On Wed, 28 Jun 2017 08:09:35 +0100
> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:

>> 1) Alter fw_cfg_find() to use object_resolve_path_type("", TYPE_FW_CFG,
>> NULL)
> I'd make use of the 3rd argument &ambiguous and assert on it

I vaguely recall playing with "ambiguous" in find_vmgenid_dev(), but it
didn't work as I expected (or, it wasn't necessary to use). Not sure
about the details, I only remember that, when calling
object_resolve_path_type() like above, from within a realize function,
the object under realization is already considered existent, so if we're
realizing the second (or later) instance of the class,
object_resolve_path_type() will return NULL (regardless of "ambiguous").
If we're realizing the very first instance, then
object_resolve_path_type() will return non-NULL.

I don't mind "ambiguous" if it can be made work fine, just thought that
I'd add this tidbit.

Thanks,
Laszlo

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

[Igor Mammedov]

On Wed, 28 Jun 2017 16:21:40 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 06/28/17 16:12, Igor Mammedov wrote:
> > On Wed, 28 Jun 2017 08:09:35 +0100
> > Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:  
> 
> >> 1) Alter fw_cfg_find() to use object_resolve_path_type("", TYPE_FW_CFG,
> >> NULL)  
> > I'd make use of the 3rd argument &ambiguous and assert on it  
> 
> I vaguely recall playing with "ambiguous" in find_vmgenid_dev(), but it
> didn't work as I expected (or, it wasn't necessary to use). Not sure
> about the details, I only remember that, when calling
> object_resolve_path_type() like above, from within a realize function,
> the object under realization is already considered existent, so if we're
> realizing the second (or later) instance of the class,
> object_resolve_path_type() will return NULL (regardless of "ambiguous").
> If we're realizing the very first instance, then
> object_resolve_path_type() will return non-NULL.
> 
> I don't mind "ambiguous" if it can be made work fine, just thought that
> I'd add this tidbit.


looking at object_resolve_partial_path()

        if (ambiguous && *ambiguous) {                                           
            return NULL;                                                         
        }

might make object_resolve_partial_path() return non NULL if ambiguous is NULL


object_resolve_partial_path(,,, NULL)
p0 - p00 - fw0
    |    |
    |      fw1
    |
     fw2

p00 returns NULL &&  only fw2 is found without knowing at all about p00 failure


> 
> Thanks,
> Laszlo

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

[Mark Cave-Ayland]

On 28/06/17 15:12, Igor Mammedov wrote:

>>> I don't understand this part.  When and why would the check become
>>> useless?  
>>
>> Well because when using the standard QEMU pattern of
>> qdev_create()...qdev_init_nofail() it is possible to realize the device
>> and wire up its MemoryRegions manually as I have done with the sun4u
>> (i.e. it will respond to accesses) multiple times without calling the
>> helper function and triggering the check. The ingenious part here is
>> that only the developers who aren't aware that they have to manually
>> call the helper function will be the ones who get caught out trying to
>> instantiate the device multiple times ;)
>>
>> I'm also aware that with QOM we've got 2 conflicting ideas with fw_cfg:
>> on the one hand we're saying that QOM hierarchy should at some level
>> represent the topology of the machine, i.e. for sun4u the fw_cfg device
>> should appear under the ebus device. whereas at the moment we assume a
>> fixed path of FW_CFG_PATH.
>>
>> Based upon this I been thinking along the following lines:
>>
>> 1) Alter fw_cfg_find() to use object_resolve_path_type("", TYPE_FW_CFG,
>> NULL)
> I'd make use of the 3rd argument &ambiguous and assert on it

I don't think this is relevant here, as one of the aims of the patchset
is to ensure that only one fw_cfg device is realized, since as Laszlo
points out this is an underlying assumption in the code.

>> This solves the issue of allowing the QOM tree to best represent the
>> device topology as it will allow fw_cfg_find() to locate the fw_cfg
>> device regardless of its location, and hence it can be placed as
>> appropriate for each machine.
> looks reasonable, that's what we were doing in a bunch of other cases

Okay.

>> 2) Add a check at the top of fw_cfg_common_realize() along the lines of:
>>
>> if (object_unattached(OBJECT(dev)) {
>>     error_setg(errp, "%s device must be explicitly added as a child "
>>                object before realize", TYPE_FW_CFG);
>>     return;
>> }
> that would be never trigger as ancestor's DEVICE.realize() always sets
> parent if it wasn't set manually before child's realize is called.
> 
> in other words it's guarantied that device has parent by the time
> realize() is run by device_set_realized()

Yes I understand that, although I may not have made it clear enough that
this is pseudo code (there is no object_unattached() function in QEMU).
If you look below you can see that the criteria I am using to
distinguish whether a device is a child or not is to see whether it
exists underneath /machine/unattached, since as you correctly point out
the parent is already set by the time the device is realized.

>> This makes it obvious to the developer that they MUST wire up the fw_cfg
>> device to the machine before realize, and then if more than one device
>> is instantiated we can still trigger the existing error from my v7 branch:
>>
>> if (!object_resolve_path_type("", TYPE_FW_CFG, NULL)) {
>>     error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG);
>>     return;
>> }
> reuse fw_cfg_find() here?

Yes that is entirely valid - I've made this change locally.

>> I prefer this method because it is impossible for a developer to
>> accidentally realize the fw_cfg device without attaching it to the
>> machine first (i.e. fw_cfg_find() will always succeed if this is the
>> case if altered as per 1) above) and it can only be realized once. And
>> following the POLA the device can be wired up using
>> object_property_add_child() as per the numerous existing examples
>> throughout the QEMU codebase.
>>
>> Now the minor issue with 2) is that I can't find an easy way to
>> determine if the device is unattached at realize time. I've tried
>> something like this:
>>
>> if (!object_resolve_path_type("/machine/unattached",
>>     TYPE_FW_CFG, NULL)) {
>>    ...
>>    ...
>> }
>>
>> However that doesn't work because as the rules differ between partial
>> and absolute path resolution, we end up resolving the container itself
>> as opposed to iterating down through the QOM tree. Is there an existing
>> solution for this or would I need to come up with something that
>> iterates over the container children to search for a TYPE_FW_CFG device
>> myself?

Actually the visitor function isn't too complicated at all and I have
something working now. Let me tidy up the patchset and if it looks good,
I'll re-post it as a v7.


ATB,

Mark.

[Qemu-devel] [PATCHv6 4/5] 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.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>
---
 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

[Qemu-devel] [PATCHv6 5/5] 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>
Tested-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 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