Message ID | 20200722035016.469075-9-bauerman@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | Generalize start-powered-off property from ARM | expand |
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 <bauerman@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(). > - } 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? > + 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. I'd use directly: object_property_set_bool(OBJECT(cpu), "start-powered-off", !!id, &error_abort); > } > *cpu_irqs = qemu_allocate_irqs(cpu_set_irq, cpu, MAX_PILS); >
Philippe Mathieu-Daudé <philmd@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 <bauerman@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
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); - } 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); + if (id != 0) { + object_property_set_bool(OBJECT(cpu), "start-powered-off", true, &error_abort); } *cpu_irqs = qemu_allocate_irqs(cpu_set_irq, cpu, MAX_PILS);
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 <bauerman@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.