Re: [PATCH v4 4/8] ppc/e500: Use start-powered-off CPUState property

[Thiago Jung Bauermann]

Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:

> Hello Philippe,
>
> Thanks for your review.
>
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>
>> On 8/18/20 9:22 AM, Philippe Mathieu-Daudé wrote:
>>>> @@ -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().

I'll change [*] to &error_fatal as well, for consistency.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center