Philippe Mathieu-Daudé <phi...@redhat.com> writes:
> On 7/22/20 5:50 AM, Thiago Jung Bauermann wrote:
>> If we rely on cpu_common_reset() setting CPUState::halted according to the
>> start-powered-off property, both reset functions become equivalent and we
>> can use only one.
>>
>> Signed-off-by: Thiago Jung Bauermann <bauer...@linux.ibm.com>
>> ---
>> hw/sparc/sun4m.c | 21 ++++-----------------
>> 1 file changed, 4 insertions(+), 17 deletions(-)
>>
>> NB: I was only able to test that this patch builds. I wasn't able to
>> run it.
>>
>> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
>> index 7b3042a801..deb5e9f027 100644
>> --- a/hw/sparc/sun4m.c
>> +++ b/hw/sparc/sun4m.c
>> @@ -218,16 +218,7 @@ static void dummy_cpu_set_irq(void *opaque, int irq,
>> int level)
>> {
>> }
>>
>> -static void main_cpu_reset(void *opaque)
>> -{
>> - SPARCCPU *cpu = opaque;
>> - CPUState *cs = CPU(cpu);
>> -
>> - cpu_reset(cs);
>> - cs->halted = 0;
>> -}
>> -
>> -static void secondary_cpu_reset(void *opaque)
>> +static void sun4m_cpu_reset(void *opaque)
>> {
>> SPARCCPU *cpu = opaque;
>> CPUState *cs = CPU(cpu);
>> @@ -818,7 +809,6 @@ static const TypeInfo ram_info = {
>> static void cpu_devinit(const char *cpu_type, unsigned int id,
>> uint64_t prom_addr, qemu_irq **cpu_irqs)
>> {
>> - CPUState *cs;
>> SPARCCPU *cpu;
>> CPUSPARCState *env;
>>
>> @@ -826,12 +816,9 @@ static void cpu_devinit(const char *cpu_type, unsigned
>> int id,
>> env = &cpu->env;
>>
>> cpu_sparc_set_id(env, id);
>> - if (id == 0) {
>> - qemu_register_reset(main_cpu_reset, cpu);
>
> IMO it is easier to review this patch in 2, first drop main_cpu_reset
> as it is pointless (we rely on cpu_common_reset), then set the
> "start-powered-off" property and drop secondary_cpu_reset().
That's a good idea. I made those patches for v3.
>> - } else {
>> - qemu_register_reset(secondary_cpu_reset, cpu);
>> - cs = CPU(cpu);
>> - object_property_set_bool(OBJECT(cs), "start-powered-off", true,
>> + qemu_register_reset(sun4m_cpu_reset, cpu);
>
> Why do you still keep it?
I didn't know that not registering a reset function would cause
cpu_reset() to be caused.
>> + if (id != 0) {
>> + object_property_set_bool(OBJECT(cpu), "start-powered-off", true,
>> &error_abort);
>
> At this point the CPU is realized, so this is correct.
Great. Thanks for confirming!
> I'd use directly:
>
> object_property_set_bool(OBJECT(cpu), "start-powered-off", !!id,
> &error_abort);
I used a slight variation of your suggestion, with `id != 0` instead of
`!!id` because I think it makes the code easier to read.
--
Thiago Jung Bauermann
IBM Linux Technology Center
