On Wed, 24 Mar 2021 00:35:05 +0100 Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
> On 3/24/21 12:00 AM, Greg Kurz wrote: > > Cc'ing David > > > > On Tue, 23 Mar 2021 17:48:36 +0100 > > Thomas Huth <th...@redhat.com> wrote: > > > >> > >> In case anyone is interested in fixing those, there are two regressions > >> with > >> qemu-system-ppc64 in the current master branch: > >> > >> $ ./qemu-system-ppc64 -M ppce500 -device macio-oldworld > >> qemu-system-ppc64: ../../devel/qemu/softmmu/memory.c:2443: > >> memory_region_add_subregion_common: Assertion `!subregion->container' > >> failed. > >> > >> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help > >> /home/thuth/devel/qemu/include/hw/boards.h:24:MACHINE: Object > >> 0x5635bd53af10 > >> is not an instance of type machine > >> Aborted (core dumped) > >> > > > > I've bisected this one to: > > > > 3df261b6676b5850e93d6fab3f7a98f8ee8f19c5 is the first bad commit > > commit 3df261b6676b5850e93d6fab3f7a98f8ee8f19c5 > > Author: Peter Maydell <peter.mayd...@linaro.org> > > Date: Fri Mar 13 17:24:47 2020 +0000 > > > > softmmu/vl.c: Handle '-cpu help' and '-device help' before 'no default > > machine' > > > > Currently if you try to ask for the list of CPUs for a target > > architecture which does not specify a default machine type > > you just get an error: > > > > $ qemu-system-arm -cpu help > > qemu-system-arm: No machine specified, and there is no default > > Use -machine help to list supported machines > > > > Since the list of CPUs doesn't depend on the machine, this is > > unnecessarily unhelpful. "-device help" has a similar problem. > > > > Move the checks for "did the user ask for -cpu help or -device help" > > up so they precede the select_machine() call which checks that the > > user specified a valid machine type. > > > > Signed-off-by: Peter Maydell <peter.mayd...@linaro.org> > > Signed-off-by: Paolo Bonzini <pbonz...@redhat.com> > > > > softmmu/vl.c | 26 ++++++++++++++++---------- > > 1 file changed, 16 insertions(+), 10 deletions(-) > > bisect run success > > > > This change is fine but it unveils a bad assumption. > > > > 0 0x00007ffff64a3708 in raise () at /lib64/power9/libc.so.6 > > #1 0x00007ffff6483bcc in abort () at /lib64/power9/libc.so.6 > > #2 0x00000001008db940 in object_dynamic_cast_assert > > (obj=0x10126f670, typename=0x100c20380 "machine", file=0x100b34878 > > "/home/greg/Work/qemu/qemu-ppc/include/hw/boards.h", line=<optimized out>, > > func=0x100bcd320 <__func__.30338> "MACHINE") at ../../qom/object.c:883 > > #3 0x0000000100456e00 in MACHINE (obj=<optimized out>) at > > /home/greg/Work/qemu/qemu-ppc/include/hw/boards.h:24 > > #4 0x0000000100456e00 in cpu_core_instance_init (obj=0x10118e2c0) at > > ../../hw/cpu/core.c:69 > > #5 0x00000001008d9f44 in object_init_with_type (obj=obj@entry=0x10118e2c0, > > ti=0x1011fd470) at ../../qom/object.c:375 > > #6 0x00000001008d9f24 in object_init_with_type (obj=obj@entry=0x10118e2c0, > > ti=0x101211ad0) at ../../qom/object.c:371 > > #7 0x00000001008d9f24 in object_init_with_type (obj=obj@entry=0x10118e2c0, > > ti=ti@entry=0x101212760) at ../../qom/object.c:371 > > #8 0x00000001008dc474 in object_initialize_with_type > > (obj=obj@entry=0x10118e2c0, size=size@entry=160, > > type=type@entry=0x101212760) at ../../qom/object.c:517 > > #9 0x00000001008dc678 in object_new_with_type (type=0x101212760) at > > ../../qom/object.c:732 > > #10 0x00000001009fbad8 in qmp_device_list_properties (typename=<optimized > > out>, errp=<optimized out>) at ../../qom/qom-qmp-cmds.c:146 > > #11 0x00000001005a4bf0 in qdev_device_help (opts=0x10126c200) at > > ../../softmmu/qdev-monitor.c:285 > > #12 0x0000000100760afc in device_help_func (opaque=<optimized out>, > > opts=<optimized out>, errp=<optimized out>) at ../../softmmu/vl.c:1204 > > #13 0x0000000100ad1050 in qemu_opts_foreach (list=<optimized out>, > > func=0x100760ae0 <device_help_func>, opaque=0x0, errp=0x0) at > > ../../util/qemu-option.c:1167 > > #14 0x00000001007653cc in qemu_process_help_options () at > > ../../softmmu/vl.c:2451 > > #15 0x00000001007653cc in qemu_init (argc=<optimized out>, argv=<optimized > > out>, envp=<optimized out>) at ../../softmmu/vl.c:3521 > > #16 0x00000001002f4f88 in main (argc=<optimized out>, argv=<optimized out>, > > envp=<optimized out>) at ../../softmmu/main.c:49 > > > > Basically, "-device power8_v2.0-spapr-cpu-core,help" ends up > > instantiating an object of the "power8_v2.0-spapr-cpu-core" type, > > which derives from "cpu-core". The "cpu-core" type has an instance > > init function that assumes that qdev_get_machine() returns an object > > of type "machine"... > > > > static void cpu_core_instance_init(Object *obj) > > { > > MachineState *ms = MACHINE(qdev_get_machine()); > > ^^ > > ...here. > > > > qdev_get_machine() cannot return a valid machine type since > > select_machine() hasn't been called yet... an instance init > > function is probably not the best place to use qdev_get_machine() > > if any. > > Hmmm does this assert() matches your comment? > > -- >8 -- > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index cefc5eaa0a9..41cbee77d14 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -1130,6 +1130,8 @@ Object *qdev_get_machine(void) > { > static Object *dev; > > + assert(phase_check(PHASE_MACHINE_CREATED)); > + Heh... I didn't know about phase_check() but it could make sense to prevent early misuse of qdev_get_machine() indeed. > if (dev == NULL) { > dev = container_get(object_get_root(), "/machine"); > } > --- > > > > > CPUCore *core = CPU_CORE(obj); > > > > core->nr_threads = ms->smp.threads; > > } > > > > It seems that this should rather sit in a device realize function, > > when the machine type is known. > > Or maybe leave everything in the instance init function but rely on current_machine instead of qdev_get_machine(), i.e. something like: static void cpu_core_instance_init(Object *obj) { MachineState *ms = current_machine; CPUCore *core = CPU_CORE(obj); /* * This can be called with "-device some_cpu_core,help" before the * machine has been created. */ if (!ms) { core->nr_threads = 1; return; } core->nr_threads = ms->smp.threads; } > >> Thomas > >> > >> > > > > >