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

[Thiago Jung Bauermann]

Hi Igor,

Thank you for reviewing these patches, and the tips you provided here
and on other messages on how to fix the refcount issues.

Igor Mammedov <imammedo@redhat.com> writes:

> On Tue, 18 Aug 2020 00:33:19 -0300
> Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote:
>
> [...]
>
>> 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.
>
> cpu_create was introduced to remove code duplication in simple cases where
> we do not need to set properties on created cpu.
>
> returning back to manual object_new + realize() is fine if it 's only
> small set of of boards. If it's tree-wide change then that would bring
> back all code duplication that cpu_create() got rid of.

This is only necessary for boards where the secondary CPUs start powered
off, so it's not a tree-wide change.

> An alternative way is to use 'hotplug' callbacks to let board set
> additional properties before cpu's realize is called.
>
> example:
>   hw/ppc/spapr.c:
>    spapr_machine_class_init()
>      mc->get_hotplug_handler = spapr_get_hotplug_handler;
>      hc->pre_plug = spapr_machine_device_pre_plug;
>    ...
>    static const TypeInfo spapr_machine_info = {
>    ...
>         { TYPE_HOTPLUG_HANDLER },
>
> that might work in generic case if it is put into generic machine code,
> and existing users of mc->get_hotplug_handler/hc->pre_plug were taken care of.
> In which case board would only need to set MachineClass:cpu-start-powered-of
> to gate property setting.

Thank you for this idea. Though if possible I'd like to keep the new
code as similar as possible to the original code to minimize unwanted
"perturbations" in how and when objects are created and initialized.

>> Signed-off-by: Thiago Jung Bauermann <bauerman@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);
>>          }
>
> btw:
> board leaks cpu reference (from cpu_create()/object_new()) since 
> qdev_realize()
> adds it's own and the caller of object_new() is suposed to free the original 
> one.
>
> in this case qdev_realize_and_unref() fits nicely.

I will make this change.
--
Thiago Jung Bauermann
IBM Linux Technology Center