[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCHv6 3/5] fw_cfg: move assert() and linking of fw_c
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCHv6 3/5] fw_cfg: move assert() and linking of fw_cfg device to the machine into instance_init() |
Date: |
Mon, 19 Jun 2017 16:56:38 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 |
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 <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.
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
>>
>
- [Qemu-devel] [PATCHv6 0/5] fw_cfg: qdev-related tidy-ups, Mark Cave-Ayland, 2017/06/19
- [Qemu-devel] [PATCHv6 2/5] fw_cfg: move setting of FW_CFG_VERSION_DMA bit to fw_cfg_init1(), Mark Cave-Ayland, 2017/06/19
- [Qemu-devel] [PATCHv6 1/5] fw_cfg: don't map the fw_cfg IO ports in fw_cfg_io_realize(), Mark Cave-Ayland, 2017/06/19
- [Qemu-devel] [PATCHv6 3/5] fw_cfg: move assert() and linking of fw_cfg device to the machine into instance_init(), Mark Cave-Ayland, 2017/06/19
- Re: [Qemu-devel] [PATCHv6 3/5] fw_cfg: move assert() and linking of fw_cfg device to the machine into instance_init(), Eduardo Habkost, 2017/06/19
- Re: [Qemu-devel] [PATCHv6 3/5] fw_cfg: move assert() and linking of fw_cfg device to the machine into instance_init(),
Laszlo Ersek <=
- 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, 2017/06/19
- Re: [Qemu-devel] [PATCHv6 3/5] fw_cfg: move assert() and linking of fw_cfg device to the machine into instance_init(), Laszlo Ersek, 2017/06/19
- 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, 2017/06/19
- Re: [Qemu-devel] [PATCHv6 3/5] fw_cfg: move assert() and linking of fw_cfg device to the machine into instance_init(), Laszlo Ersek, 2017/06/19
- 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, 2017/06/21
- Re: [Qemu-devel] [PATCHv6 3/5] fw_cfg: move assert() and linking of fw_cfg device to the machine into instance_init(), Laszlo Ersek, 2017/06/21
- Re: [Qemu-devel] [PATCHv6 3/5] fw_cfg: move assert() and linking of fw_cfg device to the machine into instance_init(), Eduardo Habkost, 2017/06/21
- 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, 2017/06/21
- Re: [Qemu-devel] [PATCHv6 3/5] fw_cfg: move assert() and linking of fw_cfg device to the machine into instance_init(), Eduardo Habkost, 2017/06/21
- 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, 2017/06/23