| Message ID | 20200818033323.336912-5-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 ppce500_cpu_reset_sec(), 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/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); > } The last 4 lines are equivalent to: qdev_realize(DEVICE(cs), NULL, &error_fatal)) { This is also the preferred form, as we can not propagate errors from the machine_init() handler. Since you use &error_abort in [*], maybe you want to use it here too. > } > >
On 8/18/20 9:22 AM, Philippe Mathieu-Daudé wrote: > On 8/18/20 5:33 AM, Thiago Jung Bauermann wrote: >> Instead of setting CPUState::halted to 1 in ppce500_cpu_reset_sec(), 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/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); >> } > > The last 4 lines are equivalent to: > > qdev_realize(DEVICE(cs), NULL, &error_fatal)) { I meant: qdev_realize(DEVICE(cs), NULL, &error_fatal); > > This is also the preferred form, as we can not propagate errors > from the machine_init() handler. > > Since you use &error_abort in [*], maybe you want to use it here too. > >> } >> >> >
On Tue, 18 Aug 2020 09:22:05 +0200 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 8/18/20 5:33 AM, Thiago Jung Bauermann wrote: > > Instead of setting CPUState::halted to 1 in ppce500_cpu_reset_sec(), 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/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); > > } > > The last 4 lines are equivalent to: > > qdev_realize(DEVICE(cs), NULL, &error_fatal)) { > > This is also the preferred form, as we can not propagate errors > from the machine_init() handler. not exactly, it will leak refference, but we are dying anyways so who cares. > > Since you use &error_abort in [*], maybe you want to use it here too. > > > } > > > > > >
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. 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. > 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. > } > >
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
Hello Philippe, Thanks for your review. Philippe Mathieu-Daudé <philmd@redhat.com> writes: > On 8/18/20 9:22 AM, Philippe Mathieu-Daudé wrote: >> On 8/18/20 5:33 AM, Thiago Jung Bauermann wrote: >>> Instead of setting CPUState::halted to 1 in ppce500_cpu_reset_sec(), 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/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); >>> } >> >> The last 4 lines are equivalent to: >> >> qdev_realize(DEVICE(cs), NULL, &error_fatal)) { > > I meant: > > qdev_realize(DEVICE(cs), NULL, &error_fatal); Ah! Thanks for pointing it out. I'll use that (along with qdev_realize_and_unref()). > >> >> This is also the preferred form, as we can not propagate errors >> from the machine_init() handler. >> >> Since you use &error_abort in [*], maybe you want to use it here too. I think &error_fatal is better since it preserves the behavior from cpu_create(). -- Thiago Jung Bauermann IBM Linux Technology Center
Thiago Jung Bauermann <bauerman@linux.ibm.com> writes: > Hello Philippe, > > Thanks for your review. > > Philippe Mathieu-Daudé <philmd@redhat.com> writes: > >> On 8/18/20 9:22 AM, Philippe Mathieu-Daudé wrote: >>>> @@ -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); >>>> } >>> >>> The last 4 lines are equivalent to: >>> >>> qdev_realize(DEVICE(cs), NULL, &error_fatal)) { >> >> I meant: >> >> qdev_realize(DEVICE(cs), NULL, &error_fatal); > > Ah! Thanks for pointing it out. I'll use that (along with > qdev_realize_and_unref()). > >> >>> >>> This is also the preferred form, as we can not propagate errors >>> from the machine_init() handler. >>> >>> Since you use &error_abort in [*], maybe you want to use it here too. > > I think &error_fatal is better since it preserves the behavior from > cpu_create(). I'll change [*] to &error_fatal as well, for consistency.
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); } }
Instead of setting CPUState::halted to 1 in ppce500_cpu_reset_sec(), 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/ppc/e500.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)