| Message ID | 20200818033323.336912-6-bauerman@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Generalize start-powered-off property from ARM | expand |
On 8/18/20 5:33 AM, Thiago Jung Bauermann wrote: > Instead of setting CPUState::halted to 1 in main_cpu_reset(), 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 <bauerman@linux.ibm.com> > --- > hw/mips/cps.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/hw/mips/cps.c b/hw/mips/cps.c > index 615e1a1ad2..be85357dc0 100644 > --- a/hw/mips/cps.c > +++ b/hw/mips/cps.c > @@ -52,9 +52,6 @@ static void main_cpu_reset(void *opaque) > CPUState *cs = CPU(cpu); > > cpu_reset(cs); > - > - /* All VPs are halted on reset. Leave powering up to CPC. */ > - cs->halted = 1; > } > > static bool cpu_mips_itu_supported(CPUMIPSState *env) > @@ -76,7 +73,9 @@ static void mips_cps_realize(DeviceState *dev, Error **errp) > bool saar_present = false; > > for (i = 0; i < s->num_vp; i++) { > - cpu = MIPS_CPU(cpu_create(s->cpu_type)); > + Error *err = NULL; > + > + cpu = MIPS_CPU(object_new(s->cpu_type)); > > /* Init internal devices */ > cpu_mips_irq_init_cpu(cpu); > @@ -89,7 +88,16 @@ static void mips_cps_realize(DeviceState *dev, Error **errp) > env->itc_tag = mips_itu_get_tag_region(&s->itu); > env->itu = &s->itu; > } > + /* All VPs are halted on reset. Leave powering up to CPC. */ > + object_property_set_bool(OBJECT(cpu), "start-powered-off", true, > + &error_abort); > qemu_register_reset(main_cpu_reset, cpu); > + > + if (!qdev_realize(DEVICE(cpu), NULL, &err)) { > + error_report_err(err); > + object_unref(OBJECT(cpu)); > + exit(EXIT_FAILURE); > + } Here errp is available, so we can propagate the error to the caller: if (!qdev_realize(DEVICE(cpu), NULL, errp)) { return; } For example in hw/mips/boston.c: object_initialize_child(OBJECT(machine), "cps", &s->cps, TYPE_MIPS_CPS); object_property_set_str(OBJECT(&s->cps), "cpu-type", machine->cpu_type, &error_fatal); object_property_set_int(OBJECT(&s->cps), "num-vp", machine->smp.cpus, &error_fatal); sysbus_realize(SYS_BUS_DEVICE(&s->cps), &error_fatal); This will be propagated here ---------------^^^^^^^^^^^^^ > } > > cpu = MIPS_CPU(first_cpu); >
On 8/18/20 9:26 AM, Philippe Mathieu-Daudé wrote: > On 8/18/20 5:33 AM, Thiago Jung Bauermann wrote: >> Instead of setting CPUState::halted to 1 in main_cpu_reset(), 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 <bauerman@linux.ibm.com> >> --- >> hw/mips/cps.c | 16 ++++++++++++---- >> 1 file changed, 12 insertions(+), 4 deletions(-) >> >> diff --git a/hw/mips/cps.c b/hw/mips/cps.c >> index 615e1a1ad2..be85357dc0 100644 >> --- a/hw/mips/cps.c >> +++ b/hw/mips/cps.c >> @@ -52,9 +52,6 @@ static void main_cpu_reset(void *opaque) >> CPUState *cs = CPU(cpu); >> >> cpu_reset(cs); >> - >> - /* All VPs are halted on reset. Leave powering up to CPC. */ >> - cs->halted = 1; >> } >> >> static bool cpu_mips_itu_supported(CPUMIPSState *env) >> @@ -76,7 +73,9 @@ static void mips_cps_realize(DeviceState *dev, Error **errp) >> bool saar_present = false; >> >> for (i = 0; i < s->num_vp; i++) { >> - cpu = MIPS_CPU(cpu_create(s->cpu_type)); >> + Error *err = NULL; >> + >> + cpu = MIPS_CPU(object_new(s->cpu_type)); >> >> /* Init internal devices */ >> cpu_mips_irq_init_cpu(cpu); >> @@ -89,7 +88,16 @@ static void mips_cps_realize(DeviceState *dev, Error **errp) >> env->itc_tag = mips_itu_get_tag_region(&s->itu); >> env->itu = &s->itu; >> } >> + /* All VPs are halted on reset. Leave powering up to CPC. */ >> + object_property_set_bool(OBJECT(cpu), "start-powered-off", true, >> + &error_abort); >> qemu_register_reset(main_cpu_reset, cpu); >> + >> + if (!qdev_realize(DEVICE(cpu), NULL, &err)) { >> + error_report_err(err); >> + object_unref(OBJECT(cpu)); >> + exit(EXIT_FAILURE); >> + } > > Here errp is available, so we can propagate the error to the caller: > > if (!qdev_realize(DEVICE(cpu), NULL, errp)) { Igor corrected me in the previous patch, to avoid leaking the reference this snippet misses: object_unref(OBJECT(cpu)); > return; > } > > For example in hw/mips/boston.c: > > object_initialize_child(OBJECT(machine), "cps", &s->cps, TYPE_MIPS_CPS); > object_property_set_str(OBJECT(&s->cps), "cpu-type", machine->cpu_type, > &error_fatal); > object_property_set_int(OBJECT(&s->cps), "num-vp", machine->smp.cpus, > &error_fatal); > sysbus_realize(SYS_BUS_DEVICE(&s->cps), &error_fatal); > > This will be propagated here ---------------^^^^^^^^^^^^^ > >> } >> >> cpu = MIPS_CPU(first_cpu); >>
On 8/18/20 1:03 PM, Philippe Mathieu-Daudé wrote: > On 8/18/20 9:26 AM, Philippe Mathieu-Daudé wrote: >> On 8/18/20 5:33 AM, Thiago Jung Bauermann wrote: >>> Instead of setting CPUState::halted to 1 in main_cpu_reset(), 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 <bauerman@linux.ibm.com> >>> --- >>> hw/mips/cps.c | 16 ++++++++++++---- >>> 1 file changed, 12 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/mips/cps.c b/hw/mips/cps.c >>> index 615e1a1ad2..be85357dc0 100644 >>> --- a/hw/mips/cps.c >>> +++ b/hw/mips/cps.c >>> @@ -52,9 +52,6 @@ static void main_cpu_reset(void *opaque) >>> CPUState *cs = CPU(cpu); >>> >>> cpu_reset(cs); >>> - >>> - /* All VPs are halted on reset. Leave powering up to CPC. */ >>> - cs->halted = 1; >>> } >>> >>> static bool cpu_mips_itu_supported(CPUMIPSState *env) >>> @@ -76,7 +73,9 @@ static void mips_cps_realize(DeviceState *dev, Error **errp) >>> bool saar_present = false; >>> >>> for (i = 0; i < s->num_vp; i++) { >>> - cpu = MIPS_CPU(cpu_create(s->cpu_type)); >>> + Error *err = NULL; >>> + >>> + cpu = MIPS_CPU(object_new(s->cpu_type)); >>> >>> /* Init internal devices */ >>> cpu_mips_irq_init_cpu(cpu); >>> @@ -89,7 +88,16 @@ static void mips_cps_realize(DeviceState *dev, Error **errp) >>> env->itc_tag = mips_itu_get_tag_region(&s->itu); >>> env->itu = &s->itu; >>> } >>> + /* All VPs are halted on reset. Leave powering up to CPC. */ >>> + object_property_set_bool(OBJECT(cpu), "start-powered-off", true, >>> + &error_abort); >>> qemu_register_reset(main_cpu_reset, cpu); >>> + >>> + if (!qdev_realize(DEVICE(cpu), NULL, &err)) { >>> + error_report_err(err); >>> + object_unref(OBJECT(cpu)); >>> + exit(EXIT_FAILURE); >>> + } >> >> Here errp is available, so we can propagate the error to the caller: >> >> if (!qdev_realize(DEVICE(cpu), NULL, errp)) { > > Igor corrected me in the previous patch, to avoid leaking the > reference this snippet misses: > > object_unref(OBJECT(cpu)); Well this is simply: if (!qdev_realize_and_unref(DEVICE(cpu), NULL, errp)) { > >> return; >> } >> >> For example in hw/mips/boston.c: >> >> object_initialize_child(OBJECT(machine), "cps", &s->cps, TYPE_MIPS_CPS); >> object_property_set_str(OBJECT(&s->cps), "cpu-type", machine->cpu_type, >> &error_fatal); >> object_property_set_int(OBJECT(&s->cps), "num-vp", machine->smp.cpus, >> &error_fatal); >> sysbus_realize(SYS_BUS_DEVICE(&s->cps), &error_fatal); >> >> This will be propagated here ---------------^^^^^^^^^^^^^ >> >>> } >>> >>> cpu = MIPS_CPU(first_cpu); >>> >
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > On 8/18/20 5:33 AM, Thiago Jung Bauermann wrote: >> Instead of setting CPUState::halted to 1 in main_cpu_reset(), 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 <bauerman@linux.ibm.com> >> --- >> hw/mips/cps.c | 16 ++++++++++++---- >> 1 file changed, 12 insertions(+), 4 deletions(-) >> >> diff --git a/hw/mips/cps.c b/hw/mips/cps.c >> index 615e1a1ad2..be85357dc0 100644 >> --- a/hw/mips/cps.c >> +++ b/hw/mips/cps.c >> @@ -52,9 +52,6 @@ static void main_cpu_reset(void *opaque) >> CPUState *cs = CPU(cpu); >> >> cpu_reset(cs); >> - >> - /* All VPs are halted on reset. Leave powering up to CPC. */ >> - cs->halted = 1; >> } >> >> static bool cpu_mips_itu_supported(CPUMIPSState *env) >> @@ -76,7 +73,9 @@ static void mips_cps_realize(DeviceState *dev, Error **errp) >> bool saar_present = false; >> >> for (i = 0; i < s->num_vp; i++) { >> - cpu = MIPS_CPU(cpu_create(s->cpu_type)); >> + Error *err = NULL; >> + >> + cpu = MIPS_CPU(object_new(s->cpu_type)); >> >> /* Init internal devices */ >> cpu_mips_irq_init_cpu(cpu); >> @@ -89,7 +88,16 @@ static void mips_cps_realize(DeviceState *dev, Error **errp) >> env->itc_tag = mips_itu_get_tag_region(&s->itu); >> env->itu = &s->itu; >> } >> + /* All VPs are halted on reset. Leave powering up to CPC. */ >> + object_property_set_bool(OBJECT(cpu), "start-powered-off", true, >> + &error_abort); >> qemu_register_reset(main_cpu_reset, cpu); >> + >> + if (!qdev_realize(DEVICE(cpu), NULL, &err)) { >> + error_report_err(err); >> + object_unref(OBJECT(cpu)); >> + exit(EXIT_FAILURE); >> + } > > Here errp is available, so we can propagate the error to the caller: > > if (!qdev_realize(DEVICE(cpu), NULL, errp)) { > return; > } Ah, nice. I made this change (using qdev_realize_and_unref()). I also changed object_property_set_bool() to use errp as well instead of &error_abort (and also early return on error). > For example in hw/mips/boston.c: > > object_initialize_child(OBJECT(machine), "cps", &s->cps, TYPE_MIPS_CPS); > object_property_set_str(OBJECT(&s->cps), "cpu-type", machine->cpu_type, > &error_fatal); > object_property_set_int(OBJECT(&s->cps), "num-vp", machine->smp.cpus, > &error_fatal); > sysbus_realize(SYS_BUS_DEVICE(&s->cps), &error_fatal); > > This will be propagated here ---------------^^^^^^^^^^^^^ Interesting. Thanks for the explanation. -- Thiago Jung Bauermann IBM Linux Technology Center
diff --git a/hw/mips/cps.c b/hw/mips/cps.c index 615e1a1ad2..be85357dc0 100644 --- a/hw/mips/cps.c +++ b/hw/mips/cps.c @@ -52,9 +52,6 @@ static void main_cpu_reset(void *opaque) CPUState *cs = CPU(cpu); cpu_reset(cs); - - /* All VPs are halted on reset. Leave powering up to CPC. */ - cs->halted = 1; } static bool cpu_mips_itu_supported(CPUMIPSState *env) @@ -76,7 +73,9 @@ static void mips_cps_realize(DeviceState *dev, Error **errp) bool saar_present = false; for (i = 0; i < s->num_vp; i++) { - cpu = MIPS_CPU(cpu_create(s->cpu_type)); + Error *err = NULL; + + cpu = MIPS_CPU(object_new(s->cpu_type)); /* Init internal devices */ cpu_mips_irq_init_cpu(cpu); @@ -89,7 +88,16 @@ static void mips_cps_realize(DeviceState *dev, Error **errp) env->itc_tag = mips_itu_get_tag_region(&s->itu); env->itu = &s->itu; } + /* All VPs are halted on reset. Leave powering up to CPC. */ + object_property_set_bool(OBJECT(cpu), "start-powered-off", true, + &error_abort); qemu_register_reset(main_cpu_reset, cpu); + + if (!qdev_realize(DEVICE(cpu), NULL, &err)) { + error_report_err(err); + object_unref(OBJECT(cpu)); + exit(EXIT_FAILURE); + } } cpu = MIPS_CPU(first_cpu);
Instead of setting CPUState::halted to 1 in main_cpu_reset(), 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 <bauerman@linux.ibm.com> --- hw/mips/cps.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)