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 <address@hidden>
>> Reviewed-by: Laszlo Ersek <address@hidden>
>> Tested-by: Laszlo Ersek <address@hidden>
>> ---
>>  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.