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

[Laszlo Ersek <lersek@redhat.com>]

On 06/12/17 23:21, Mark Cave-Ayland wrote:
> When looking to instantiate a TYPE_FW_CFG_MEM or TYPE_FW_CFG_IO device to be
> able to wire it up differently, it is much more convenient for the caller to
> instantiate the device and have the fw_cfg default files already preloaded
> during realize.
> 
> Move fw_cfg_init1() to the end of both the fw_cfg_mem_realize() and
> fw_cfg_io_realize() functions so it no longer needs to be called manually
> when instantiating the device.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/nvram/fw_cfg.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index e1aa4fc..6c21e43 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -920,8 +920,6 @@ static void fw_cfg_init1(DeviceState *dev)
>  
>      object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s), NULL);
>  
> -    qdev_init_nofail(dev);
> -
>      fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
>      fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16);
>      fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)!machine->enable_graphics);
> @@ -954,7 +952,7 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>          qdev_prop_set_bit(dev, "dma_enabled", false);
>      }
>  
> -    fw_cfg_init1(dev);
> +    qdev_init_nofail(dev);
>  
>      sbd = SYS_BUS_DEVICE(dev);
>      sysbus_add_io(sbd, iobase, sysbus_mmio_get_region(sbd, 0));
> @@ -991,7 +989,7 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
>          qdev_prop_set_bit(dev, "dma_enabled", false);
>      }
>  
> -    fw_cfg_init1(dev);
> +    qdev_init_nofail(dev);
>  
>      sbd = SYS_BUS_DEVICE(dev);
>      sysbus_mmio_map(sbd, 0, ctl_addr);
> @@ -1097,6 +1095,8 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
>                                sizeof(dma_addr_t));
>          sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
>      }
> +
> +    fw_cfg_init1(dev);
>  }
>  
>  static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
> @@ -1163,6 +1163,8 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
>                                sizeof(dma_addr_t));
>          sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
>      }
> +
> +    fw_cfg_init1(dev);
>  }
>  
>  static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
> 

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

    assert(!object_resolve_path(FW_CFG_PATH, NULL));

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

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

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

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

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

Thanks,
Laszlo