| Message ID | 1497097821-32754-2-git-send-email-mark.cave-ayland@ilande.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/10/2017 09:30 AM, Mark Cave-Ayland wrote: > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > 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)); > +} > + > static void fw_cfg_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -1030,6 +1039,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, > }; > > @@ -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)); > } > > static Property fw_cfg_io_properties[] = { >
On Sat, 10 Jun 2017 13:30:16 +0100 Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@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.
On 12/06/17 12:20, Igor Mammedov wrote: > On Sat, 10 Jun 2017 13:30:16 +0100 > Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@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? ATB, Mark.
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-ayland@ilande.co.uk> wrote: >> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@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
On 12/06/17 19:27, Laszlo Ersek wrote: >> 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. Yes that now makes sense - I think I managed to confuse myself when writing the patch since I grepped for the type and the calls to the fw_cfg_init_*() functions but managed to miss the x-file-slots property as pointed out by Igor :( Reading what you said above, I'd be inclined to move the default values from fw_cfg_init1() into a QOM method in the parent fw_cfg type as suggested by Igor, and then call the method right at the end of the realise function in order to load them. However it also sounds like you have some better thoughts for patch #2 so I'll wait for your reply to that first. ATB, Mark.
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)); +} + static void fw_cfg_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -1030,6 +1039,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, }; @@ -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)); } static Property fw_cfg_io_properties[] = {
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/nvram/fw_cfg.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)