Re: [Qemu-devel] [PATCH 1/6] fw_cfg: move initialisation of FWCfgState into instance_init

[Mark Cave-Ayland]

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?


ATB,

Mark.