Re: Crashes with qemu-system-ppc64

[Greg Kurz <groug@kaod.org>]

On Wed, 24 Mar 2021 00:35:05 +0100
Philippe Mathieu-Daudé <f4bug@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 <thuth@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.maydell@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.maydell@linaro.org>
> >     Signed-off-by: Paolo Bonzini <pbonzini@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
> >>
> >>
> > 
> > 
>