Hello Philippe,
Thanks for your review. Philippe Mathieu-Daudé <phi...@redhat.com> writes: > On 8/18/20 9:22 AM, Philippe Mathieu-Daudé wrote: >> On 8/18/20 5:33 AM, 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() because cpu_create() realizes the CPU and it's not possible >>> to set a property after the object is realized. >>> >>> Signed-off-by: Thiago Jung Bauermann <bauer...@linux.ibm.com> >>> --- >>> hw/ppc/e500.c | 19 +++++++++++++++---- >>> 1 file changed, 15 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c >>> index ab9884e315..0077aca74d 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; >>> } >>> >>> @@ -864,8 +861,9 @@ void ppce500_init(MachineState *machine) >>> PowerPCCPU *cpu; >>> CPUState *cs; >>> qemu_irq *input; >>> + Error *err = NULL; >>> >>> - cpu = POWERPC_CPU(cpu_create(machine->cpu_type)); >>> + cpu = POWERPC_CPU(object_new(machine->cpu_type)); >>> env = &cpu->env; >>> cs = CPU(cpu); >>> >>> @@ -897,6 +895,19 @@ 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_abort); >> >> [*] >> >>> + } >>> + >>> + if (!qdev_realize(DEVICE(cs), NULL, &err)) { >>> + error_report_err(err); >>> + object_unref(OBJECT(cs)); >>> + exit(EXIT_FAILURE); >>> } >> >> The last 4 lines are equivalent to: >> >> qdev_realize(DEVICE(cs), NULL, &error_fatal)) { > > I meant: > > qdev_realize(DEVICE(cs), NULL, &error_fatal); Ah! Thanks for pointing it out. I'll use that (along with qdev_realize_and_unref()). > >> >> This is also the preferred form, as we can not propagate errors >> from the machine_init() handler. >> >> Since you use &error_abort in [*], maybe you want to use it here too. I think &error_fatal is better since it preserves the behavior from cpu_create(). -- Thiago Jung Bauermann IBM Linux Technology Center