Re: [PATCH v4 5/8] mips/cps: Use start-powered-off CPUState property
Philippe Mathieu-Daudé 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 >> --- >> 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(>itu); >> env->itu = >itu; >> } >> +/* All VPs are halted on reset. Leave powering up to CPC. */ >> +object_property_set_bool(OBJECT(cpu), "start-powered-off", true, >> + _abort); >> qemu_register_reset(main_cpu_reset, cpu); >> + >> +if (!qdev_realize(DEVICE(cpu), NULL, )) { >> +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 _abort (and also early return on error). > For example in hw/mips/boston.c: > > object_initialize_child(OBJECT(machine), "cps", >cps, TYPE_MIPS_CPS); > object_property_set_str(OBJECT(>cps), "cpu-type", machine->cpu_type, > _fatal); > object_property_set_int(OBJECT(>cps), "num-vp", machine->smp.cpus, > _fatal); > sysbus_realize(SYS_BUS_DEVICE(>cps), _fatal); > > This will be propagated here ---^ Interesting. Thanks for the explanation. -- Thiago Jung Bauermann IBM Linux Technology Center
Re: [PATCH v4 5/8] mips/cps: Use start-powered-off CPUState property
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 >>> --- >>> 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(>itu); >>> env->itu = >itu; >>> } >>> +/* All VPs are halted on reset. Leave powering up to CPC. */ >>> +object_property_set_bool(OBJECT(cpu), "start-powered-off", true, >>> + _abort); >>> qemu_register_reset(main_cpu_reset, cpu); >>> + >>> +if (!qdev_realize(DEVICE(cpu), NULL, )) { >>> +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", >cps, TYPE_MIPS_CPS); >> object_property_set_str(OBJECT(>cps), "cpu-type", machine->cpu_type, >> _fatal); >> object_property_set_int(OBJECT(>cps), "num-vp", machine->smp.cpus, >> _fatal); >> sysbus_realize(SYS_BUS_DEVICE(>cps), _fatal); >> >> This will be propagated here ---^ >> >>> } >>> >>> cpu = MIPS_CPU(first_cpu); >>> >
Re: [PATCH v4 5/8] mips/cps: Use start-powered-off CPUState property
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 >> --- >> 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(>itu); >> env->itu = >itu; >> } >> +/* All VPs are halted on reset. Leave powering up to CPC. */ >> +object_property_set_bool(OBJECT(cpu), "start-powered-off", true, >> + _abort); >> qemu_register_reset(main_cpu_reset, cpu); >> + >> +if (!qdev_realize(DEVICE(cpu), NULL, )) { >> +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", >cps, TYPE_MIPS_CPS); > object_property_set_str(OBJECT(>cps), "cpu-type", machine->cpu_type, > _fatal); > object_property_set_int(OBJECT(>cps), "num-vp", machine->smp.cpus, > _fatal); > sysbus_realize(SYS_BUS_DEVICE(>cps), _fatal); > > This will be propagated here ---^ > >> } >> >> cpu = MIPS_CPU(first_cpu); >>
Re: [PATCH v4 5/8] mips/cps: Use start-powered-off CPUState property
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 > --- > 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(>itu); > env->itu = >itu; > } > +/* All VPs are halted on reset. Leave powering up to CPC. */ > +object_property_set_bool(OBJECT(cpu), "start-powered-off", true, > + _abort); > qemu_register_reset(main_cpu_reset, cpu); > + > +if (!qdev_realize(DEVICE(cpu), NULL, )) { > +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", >cps, TYPE_MIPS_CPS); object_property_set_str(OBJECT(>cps), "cpu-type", machine->cpu_type, _fatal); object_property_set_int(OBJECT(>cps), "num-vp", machine->smp.cpus, _fatal); sysbus_realize(SYS_BUS_DEVICE(>cps), _fatal); This will be propagated here ---^ > } > > cpu = MIPS_CPU(first_cpu); >