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

[Mark Cave-Ayland]

On 12/06/17 23:50, Laszlo Ersek wrote:

> 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 <address@hidden>
>> ---
>>  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.

After testing the assert() in a few different scenarios, I much prefer
the existing approach with a fixed fw_cfg path at /machine/fw_cfg.

The reason for this is that with existing code, any attempt to init a
second fw_cfg device will assert immediately, whereas if the board is
responsible for wiring up the fw_cfg device then it's possible that a
caller will either a) forget to call a helper function or b) wire up 2
fw_cfg in different places in the object tree, e.g. one calling
fw_cfg_init_io() and another instantiated directly via QOM as per what
I'm trying to do with sun4u.

Taking all this into account, I'm working on a v3 patchset that I hope
to post to the list later today.


ATB,

Mark.