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 <mark.cave-ayl...@ilande.co.uk> wrote: >> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> >>> --- >>> 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