| Message ID | 20200819164306.625357-5-bauerman@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Generalize start-powered-off property from ARM | expand |
Hello, On 8/19/20 6:43 PM, Thiago Jung Bauermann wrote: > Instead of setting CPUState::halted to 1 in ppce500_cpu_reset_sec(), use > the start-powered-off property which makes cpu_common_reset() initialize it > to 1 in common code. > > Also change creation of CPU object from cpu_create() to object_new() and > qdev_realize_and_unref() because cpu_create() realizes the CPU and it's not > possible to set a property after the object is realized. > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> This is breaking make check : tests/qtest/libqtest.c:175: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped) ERROR boot-serial-test - too few tests run (expected 7, got 0) make: *** [/home/legoater/work/qemu/qemu-powernv-5.2.git/tests/Makefile.include:650: check-qtest-ppc64] Error 1 make: *** Waiting for unfinished jobs.... gdb --args build/ppc64-softmmu/qemu-system-ppc64 -display none -M ppce500 ... Thread 1 "qemu-system-ppc" received signal SIGSEGV, Segmentation fault. 0x000055555596ebf2 in ppce500_init (machine=0x5555567aa6e0) at /home/legoater/work/qemu/qemu-powernv-5.2.git/hw/ppc/e500.c:880 880 irqs[i].irq[OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT]; AFAIUI, 'input is not initialized since the CPU is not yet realized. C. > --- > hw/ppc/e500.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c > index ab9884e315..d7b803ef26 100644 > --- a/hw/ppc/e500.c > +++ b/hw/ppc/e500.c > @@ -704,9 +704,6 @@ static void ppce500_cpu_reset_sec(void *opaque) > > cpu_reset(cs); > > - /* Secondary CPU starts in halted state for now. Needs to change when > - implementing non-kernel boot. */ > - cs->halted = 1; > cs->exception_index = EXCP_HLT; > } > > @@ -865,7 +862,7 @@ void ppce500_init(MachineState *machine) > CPUState *cs; > qemu_irq *input; > > - cpu = POWERPC_CPU(cpu_create(machine->cpu_type)); > + cpu = POWERPC_CPU(object_new(machine->cpu_type)); > env = &cpu->env; > cs = CPU(cpu); > > @@ -897,7 +894,16 @@ void ppce500_init(MachineState *machine) > } else { > /* Secondary CPUs */ > qemu_register_reset(ppce500_cpu_reset_sec, cpu); > + > + /* > + * Secondary CPU starts in halted state for now. Needs to change > + * when implementing non-kernel boot. > + */ > + object_property_set_bool(OBJECT(cs), "start-powered-off", true, > + &error_fatal); > } > + > + qdev_realize_and_unref(DEVICE(cs), NULL, &error_fatal); > } > > env = firstenv; >
On 8/22/20 10:59 AM, Cédric Le Goater wrote: > Hello, > > On 8/19/20 6:43 PM, Thiago Jung Bauermann wrote: >> Instead of setting CPUState::halted to 1 in ppce500_cpu_reset_sec(), use >> the start-powered-off property which makes cpu_common_reset() initialize it >> to 1 in common code. >> >> Also change creation of CPU object from cpu_create() to object_new() and >> qdev_realize_and_unref() because cpu_create() realizes the CPU and it's not >> possible to set a property after the object is realized. >> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > > This is breaking make check : > > tests/qtest/libqtest.c:175: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped) > ERROR boot-serial-test - too few tests run (expected 7, got 0) > make: *** [/home/legoater/work/qemu/qemu-powernv-5.2.git/tests/Makefile.include:650: check-qtest-ppc64] Error 1 > make: *** Waiting for unfinished jobs.... > > > gdb --args build/ppc64-softmmu/qemu-system-ppc64 -display none -M ppce500 > ... > Thread 1 "qemu-system-ppc" received signal SIGSEGV, Segmentation fault. > 0x000055555596ebf2 in ppce500_init (machine=0x5555567aa6e0) > at /home/legoater/work/qemu/qemu-powernv-5.2.git/hw/ppc/e500.c:880 > 880 irqs[i].irq[OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT]; > > > AFAIUI, 'input is not initialized since the CPU is not yet realized. Thiago, see ad938fc1d53 ("hw/arm/palm.c: Encapsulate misc GPIO handling in a device") and eventually f8a865d36dc ("hw/arm/allwinner-a10: Simplify by passing IRQs with qdev_pass_gpios") to get an idea how you can fix that. > > C. > >> --- >> hw/ppc/e500.c | 14 ++++++++++---- >> 1 file changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c >> index ab9884e315..d7b803ef26 100644 >> --- a/hw/ppc/e500.c >> +++ b/hw/ppc/e500.c >> @@ -704,9 +704,6 @@ static void ppce500_cpu_reset_sec(void *opaque) >> >> cpu_reset(cs); >> >> - /* Secondary CPU starts in halted state for now. Needs to change when >> - implementing non-kernel boot. */ >> - cs->halted = 1; >> cs->exception_index = EXCP_HLT; >> } >> >> @@ -865,7 +862,7 @@ void ppce500_init(MachineState *machine) >> CPUState *cs; >> qemu_irq *input; >> >> - cpu = POWERPC_CPU(cpu_create(machine->cpu_type)); >> + cpu = POWERPC_CPU(object_new(machine->cpu_type)); >> env = &cpu->env; >> cs = CPU(cpu); >> >> @@ -897,7 +894,16 @@ void ppce500_init(MachineState *machine) >> } else { >> /* Secondary CPUs */ >> qemu_register_reset(ppce500_cpu_reset_sec, cpu); >> + >> + /* >> + * Secondary CPU starts in halted state for now. Needs to change >> + * when implementing non-kernel boot. >> + */ >> + object_property_set_bool(OBJECT(cs), "start-powered-off", true, >> + &error_fatal); >> } >> + >> + qdev_realize_and_unref(DEVICE(cs), NULL, &error_fatal); >> } >> >> env = firstenv; >> >
On Sat, Aug 22, 2020 at 10:59:56AM +0200, Cédric Le Goater wrote: > Hello, > > On 8/19/20 6:43 PM, Thiago Jung Bauermann wrote: > > Instead of setting CPUState::halted to 1 in ppce500_cpu_reset_sec(), use > > the start-powered-off property which makes cpu_common_reset() initialize it > > to 1 in common code. > > > > Also change creation of CPU object from cpu_create() to object_new() and > > qdev_realize_and_unref() because cpu_create() realizes the CPU and it's not > > possible to set a property after the object is realized. > > > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > > This is breaking make check : > > tests/qtest/libqtest.c:175: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped) > ERROR boot-serial-test - too few tests run (expected 7, got 0) > make: *** [/home/legoater/work/qemu/qemu-powernv-5.2.git/tests/Makefile.include:650: check-qtest-ppc64] Error 1 > make: *** Waiting for unfinished jobs.... > > > gdb --args build/ppc64-softmmu/qemu-system-ppc64 -display none -M ppce500 > ... > Thread 1 "qemu-system-ppc" received signal SIGSEGV, Segmentation fault. > 0x000055555596ebf2 in ppce500_init (machine=0x5555567aa6e0) > at /home/legoater/work/qemu/qemu-powernv-5.2.git/hw/ppc/e500.c:880 > 880 irqs[i].irq[OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT]; > > > AFAIUI, 'input is not initialized since the CPU is not yet > realized. Sigh. For future reference, Thiago, running an all-targets make check is pretty much a minimum bar to test before posting.
David Gibson <david@gibson.dropbear.id.au> writes: > On Sat, Aug 22, 2020 at 10:59:56AM +0200, Cédric Le Goater wrote: >> Hello, >> >> On 8/19/20 6:43 PM, Thiago Jung Bauermann wrote: >> > Instead of setting CPUState::halted to 1 in ppce500_cpu_reset_sec(), use >> > the start-powered-off property which makes cpu_common_reset() initialize it >> > to 1 in common code. >> > >> > Also change creation of CPU object from cpu_create() to object_new() and >> > qdev_realize_and_unref() because cpu_create() realizes the CPU and it's not >> > possible to set a property after the object is realized. >> > >> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> >> >> >> This is breaking make check : >> >> tests/qtest/libqtest.c:175: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped) >> ERROR boot-serial-test - too few tests run (expected 7, got 0) >> make: *** [/home/legoater/work/qemu/qemu-powernv-5.2.git/tests/Makefile.include:650: check-qtest-ppc64] Error 1 >> make: *** Waiting for unfinished jobs.... >> >> >> gdb --args build/ppc64-softmmu/qemu-system-ppc64 -display none -M ppce500 >> ... >> Thread 1 "qemu-system-ppc" received signal SIGSEGV, Segmentation fault. >> 0x000055555596ebf2 in ppce500_init (machine=0x5555567aa6e0) >> at /home/legoater/work/qemu/qemu-powernv-5.2.git/hw/ppc/e500.c:880 >> 880 irqs[i].irq[OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT]; >> >> >> AFAIUI, 'input is not initialized since the CPU is not yet >> realized. > > Sigh. For future reference, Thiago, running an all-targets make check > is pretty much a minimum bar to test before posting. Sorry for the breakage. I'm working on it.
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > On 8/22/20 10:59 AM, Cédric Le Goater wrote: >> Hello, >> >> On 8/19/20 6:43 PM, Thiago Jung Bauermann wrote: >>> Instead of setting CPUState::halted to 1 in ppce500_cpu_reset_sec(), use >>> the start-powered-off property which makes cpu_common_reset() initialize it >>> to 1 in common code. >>> >>> Also change creation of CPU object from cpu_create() to object_new() and >>> qdev_realize_and_unref() because cpu_create() realizes the CPU and it's not >>> possible to set a property after the object is realized. >>> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> >> >> >> This is breaking make check : >> >> tests/qtest/libqtest.c:175: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped) >> ERROR boot-serial-test - too few tests run (expected 7, got 0) >> make: *** [/home/legoater/work/qemu/qemu-powernv-5.2.git/tests/Makefile.include:650: check-qtest-ppc64] Error 1 >> make: *** Waiting for unfinished jobs.... >> >> >> gdb --args build/ppc64-softmmu/qemu-system-ppc64 -display none -M ppce500 >> ... >> Thread 1 "qemu-system-ppc" received signal SIGSEGV, Segmentation fault. >> 0x000055555596ebf2 in ppce500_init (machine=0x5555567aa6e0) >> at /home/legoater/work/qemu/qemu-powernv-5.2.git/hw/ppc/e500.c:880 >> 880 irqs[i].irq[OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT]; >> >> >> AFAIUI, 'input is not initialized since the CPU is not yet realized. > > Thiago, see ad938fc1d53 ("hw/arm/palm.c: Encapsulate misc GPIO handling > in a device") and eventually f8a865d36dc ("hw/arm/allwinner-a10: > Simplify by passing IRQs with qdev_pass_gpios") to get an idea how you > can fix that. Thanks for the references, those are very useful.
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > On 8/22/20 10:59 AM, Cédric Le Goater wrote: >> Hello, >> >> On 8/19/20 6:43 PM, Thiago Jung Bauermann wrote: >>> Instead of setting CPUState::halted to 1 in ppce500_cpu_reset_sec(), use >>> the start-powered-off property which makes cpu_common_reset() initialize it >>> to 1 in common code. >>> >>> Also change creation of CPU object from cpu_create() to object_new() and >>> qdev_realize_and_unref() because cpu_create() realizes the CPU and it's not >>> possible to set a property after the object is realized. >>> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> >> >> >> This is breaking make check : >> >> tests/qtest/libqtest.c:175: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped) >> ERROR boot-serial-test - too few tests run (expected 7, got 0) >> make: *** [/home/legoater/work/qemu/qemu-powernv-5.2.git/tests/Makefile.include:650: check-qtest-ppc64] Error 1 >> make: *** Waiting for unfinished jobs.... >> >> >> gdb --args build/ppc64-softmmu/qemu-system-ppc64 -display none -M ppce500 >> ... >> Thread 1 "qemu-system-ppc" received signal SIGSEGV, Segmentation fault. >> 0x000055555596ebf2 in ppce500_init (machine=0x5555567aa6e0) >> at /home/legoater/work/qemu/qemu-powernv-5.2.git/hw/ppc/e500.c:880 >> 880 irqs[i].irq[OPENPIC_OUTPUT_INT] = input[PPCE500_INPUT_INT]; >> >> >> AFAIUI, 'input is not initialized since the CPU is not yet realized. > > Thiago, see ad938fc1d53 ("hw/arm/palm.c: Encapsulate misc GPIO handling > in a device") and eventually f8a865d36dc ("hw/arm/allwinner-a10: > Simplify by passing IRQs with qdev_pass_gpios") to get an idea how you > can fix that. I ended up not following this route. There were other patches in this series which also caused problems in make check, but in those cases it wasn't related to IRQ setup. I started feeling like I had fallen into a rabbit hole so I opted instead to solve these problems by minimizing the consequences of the changes made by this patch series.
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index ab9884e315..d7b803ef26 100644 --- a/hw/ppc/e500.c +++ b/hw/ppc/e500.c @@ -704,9 +704,6 @@ static void ppce500_cpu_reset_sec(void *opaque) cpu_reset(cs); - /* Secondary CPU starts in halted state for now. Needs to change when - implementing non-kernel boot. */ - cs->halted = 1; cs->exception_index = EXCP_HLT; } @@ -865,7 +862,7 @@ void ppce500_init(MachineState *machine) CPUState *cs; qemu_irq *input; - cpu = POWERPC_CPU(cpu_create(machine->cpu_type)); + cpu = POWERPC_CPU(object_new(machine->cpu_type)); env = &cpu->env; cs = CPU(cpu); @@ -897,7 +894,16 @@ void ppce500_init(MachineState *machine) } else { /* Secondary CPUs */ qemu_register_reset(ppce500_cpu_reset_sec, cpu); + + /* + * Secondary CPU starts in halted state for now. Needs to change + * when implementing non-kernel boot. + */ + object_property_set_bool(OBJECT(cs), "start-powered-off", true, + &error_fatal); } + + qdev_realize_and_unref(DEVICE(cs), NULL, &error_fatal); } env = firstenv;