[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/6] fw_cfg: move initialisation of FWCfgState i
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH 1/6] fw_cfg: move initialisation of FWCfgState into instance_init |
Date: |
Mon, 12 Jun 2017 20:27:24 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 |
Hi Mark,
On 06/12/17 13:45, Mark Cave-Ayland wrote:
> On 12/06/17 12:20, Igor Mammedov wrote:
>
>> On Sat, 10 Jun 2017 13:30:16 +0100
>> Mark Cave-Ayland <address@hidden> wrote:
>>
>>> Signed-off-by: Mark Cave-Ayland <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 316fca9..144e0c6 100644
>>> --- a/hw/nvram/fw_cfg.c
>>> +++ b/hw/nvram/fw_cfg.c
>>> @@ -1017,6 +1017,15 @@ FWCfgState *fw_cfg_find(void)
>>> return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
>>> }
>>>
>>> +static void fw_cfg_init(Object *obj)
>>> +{
>>> + FWCfgState *s = FW_CFG(obj);
>>> +
>>> + s->entries[0] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
>>> + s->entries[1] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
>>> + s->entry_order = g_new0(int, fw_cfg_max_entry(s));
>> it doesn't seem right,
>>
>> consider,
>> object_new(fwcfg)
>> 1: fw_cfg_init -> g_new0(FWCfgEntry, fw_cfg_max_entry(s) ->
>> FW_CFG_FILE_SLOTS_DFLT);
>> 2: set property x-file-slots
>> 3: realize -> fw_cfg_file_slots_allocate()
>>
>>> @@ -1052,10 +1062,6 @@ static void fw_cfg_file_slots_allocate(FWCfgState
>>> *s, Error **errp)
>>> file_slots_max);
>>> return;
>>> }
>>> -
>>> - s->entries[0] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
>>> - s->entries[1] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
>>> - s->entry_order = g_new0(int, fw_cfg_max_entry(s));
>> opps, s->entries doesn't account for new values of x-file-slots
>>
>> So question is why this patch is needed at all (it needs some reasoning in
>> commit message)?
>>
>> So for now NACK and I'd suggest to drop this patch.
>
> Right I missed the x-file-slots property when I was looking at this,
> mainly because all of the existing callers went through
> fw_cfg_init_mem() and fw_cfg_init_io(). I do see now though that the
> property is referenced in compat.h.
>
> The reason for moving the initialisation is that if you apply patch 2 on
> its own then you get hit by the following assert on qemu-system-sparc64
> due to uninitialised data:
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x000000000062f81d in fw_cfg_add_bytes_read_callback (s=0x1e85410,
> key=0, callback=0, callback_opaque=0x0, data=0x9f6c28, len=4,
> read_only=true) at hw/nvram/fw_cfg.c:633
> 633 assert(s->entries[arch][key].data == NULL); /* avoid key
> conflict */
> (gdb) bt
> #0 0x000000000062f81d in fw_cfg_add_bytes_read_callback (s=0x1e85410,
> key=0, callback=0, callback_opaque=0x0, data=0x9f6c28, len=4,
> read_only=true) at hw/nvram/fw_cfg.c:633
> #1 0x000000000062fac8 in fw_cfg_add_bytes (s=0x1e85410, key=0,
> data=0x9f6c28, len=4) at hw/nvram/fw_cfg.c:664
> #2 0x00000000006307a8 in fw_cfg_init1 (dev=0x1e85410) at
> hw/nvram/fw_cfg.c:922
> #3 0x00000000006308fe in fw_cfg_init_io_dma (iobase=1296, dma_iobase=0,
> dma_as=0x0) at hw/nvram/fw_cfg.c:948
> #4 0x00000000006309bc in fw_cfg_init_io (iobase=1296) at
> hw/nvram/fw_cfg.c:968
> #5 0x00000000004f31e9 in sun4uv_init (address_space_mem=0x132df20,
> machine=0x132c8c0, hwdef=0x8bf400) at
> /home/build/src/qemu/git/qemu/hw/sparc64/sun4u.c:515
> #6 0x00000000004f3403 in sun4u_init (machine=0x132c8c0) at
> /home/build/src/qemu/git/qemu/hw/sparc64/sun4u.c:565
> #7 0x00000000005c0d26 in machine_run_board_init (machine=0x132c8c0) at
> hw/core/machine.c:760
>
>
> #8 0x0000000000532e56 in main (argc=2, argv=0x7fffffffeaa8,
> envp=0x7fffffffeac0) at vl.c:4594
>
> Based upon this what do you think the best solution would be?
if you apply patch #2 without patch #1, then the above SIGSEGV will hit
on all fw_cfg using targets / machine types, not just
qemu-system-sparc64. The reason is that, after patch #2 (without patch
#1 applied) you have
fw_cfg_init1()
...
fw_cfg_add_bytes_read_callback()
s->entries[arch][key].data
qdev_init_nofail()
fw_cfg_file_slots_allocate()
IOW, the s->entries array is now dynamically allocated (after the
introduction of the x-file-slots property):
FWCfgEntry *entries[2];
and it is allocated in fw_cfg_file_slots_allocate(), called from
realize. But with patch #2 applied in isolation, you perform the first
dereference (from fw_cfg_init1(), indirectly) before allocation, that's
why it crashes.
Ultimately we need the following order:
(1) set properties (from device defaults, from device compat settings,
and from the command line; and also directly from code, such as in
fw_cfg_init_io_dma())
(2) call qdev_init_nofail() -- this will consume the properties from
(1), including the x-file-slots property, for allocating "s->entries" in
fw_cfg_file_slots_allocate()
(3) call fw_cfg_add_*() -- now that the device has been realized and is
usable.
This means that
- patch #1 should be dropped,
- and in patch #2, you have to push the rest of fw_cfg_init1() --
everything that is after the original qdev_init_nofail() call -- to the
end of the realize functions.
Alternatively, in patch #2, you have to split fw_cfg_init1() into two
parts, fw_cfg_init1() and fw_cfg_init2(). fw_cfg_init1() would keep
everything that's *before* the original qdev_init_nofail() call, and
fw_cfg_init2() would get everything *after*. Then, in
fw_cfg_init_io_dma() and fw_cfg_init_mem_wide(), you'd do
fw_cfg_init1(dev);
qdev_init_nofail(dev);
fw_cfg_init2(dev);
In short, you cannot reorder steps (2) and (3) against each other -- you
cannot add fw_cfg entries before you realize the device -- and the crash
happens because that's exactly what patch #2 does at the moment.
And, patch #1 is not the right fix (you can allocate s->entries only in
realize, because the size depends on a property, which you can only
access in realize). The right fix is to drop patch #1 and to split
fw_cfg_init1() like described above.
... Hmmm, wait a second, while the above approach would fix patch #2, in
fact I think patch #2 doesn't have the right *goal*.
I'll comment under patch #2.
Thanks
Laszlo
[Qemu-devel] [PATCH 5/6] fw_cfg: use sysbus_init_mmio() in fw_cfg_io_realize(), Mark Cave-Ayland, 2017/06/10
[Qemu-devel] [PATCH 3/6] fw_cfg: move setting of FW_CFG_ID into fw_cfg_init1(), Mark Cave-Ayland, 2017/06/10
[Qemu-devel] [PATCH 4/6] fw_cfg: move fw_cfg_init1() into the fw_cfg_*_realize() functions, Mark Cave-Ayland, 2017/06/10