Message ID | 20210409160339.500167-3-groug@kaod.org |
---|---|
State | New |
Headers | show |
Series | cpu/core: Fix "help" of CPU core device types | expand |
On Fri, Apr 09, 2021 at 06:03:39PM +0200, Greg Kurz wrote: > Calling qdev_get_machine() from a QOM instance_init function is > fragile because we can't be sure the machine object actually > exists. And this happens to break when passing ",help" on the > command line to get the list of properties for a CPU core > device types : > > $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help > qemu-system-ppc64: ../../hw/core/machine.c:1290: > qdev_get_machine: Assertion `machine != NULL' failed. > Aborted (core dumped) > > This used to work before QEMU 5.0, but commit 3df261b6676b > unwillingly introduced a subtle regression : the above command > line needs to create an instance but the instance_init function > of the base class calls qdev_get_machine() before > qemu_create_machine() has been called, which is a programming bug. > > Use current_machine instead. It is okay to skip the setting of > nr_thread in this case since only its type is displayed. > > Reported-by: Thomas Huth <thuth@redhat.com> > Fixes: 3df261b6676b ("softmmu/vl.c: Handle '-cpu help' and '-device help' before 'no default machine'") > Cc: peter.maydell@linaro.org > Signed-off-by: Greg Kurz <groug@kaod.org> Thanks! I'm queueing this one (without patch 1/2) for QEMU 6.0.
On 09/04/2021 18.03, Greg Kurz wrote: > Calling qdev_get_machine() from a QOM instance_init function is > fragile because we can't be sure the machine object actually > exists. And this happens to break when passing ",help" on the > command line to get the list of properties for a CPU core > device types : > > $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help > qemu-system-ppc64: ../../hw/core/machine.c:1290: > qdev_get_machine: Assertion `machine != NULL' failed. > Aborted (core dumped) > > This used to work before QEMU 5.0, but commit 3df261b6676b > unwillingly introduced a subtle regression : the above command > line needs to create an instance but the instance_init function > of the base class calls qdev_get_machine() before > qemu_create_machine() has been called, which is a programming bug. > > Use current_machine instead. It is okay to skip the setting of > nr_thread in this case since only its type is displayed. > > Reported-by: Thomas Huth <thuth@redhat.com> > Fixes: 3df261b6676b ("softmmu/vl.c: Handle '-cpu help' and '-device help' before 'no default machine'") > Cc: peter.maydell@linaro.org > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/cpu/core.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/hw/cpu/core.c b/hw/cpu/core.c > index 92d3b2fbad62..987607515574 100644 > --- a/hw/cpu/core.c > +++ b/hw/cpu/core.c > @@ -66,10 +66,16 @@ static void core_prop_set_nr_threads(Object *obj, Visitor *v, const char *name, > > static void cpu_core_instance_init(Object *obj) > { > - MachineState *ms = MACHINE(qdev_get_machine()); > CPUCore *core = CPU_CORE(obj); > > - core->nr_threads = ms->smp.threads; > + /* > + * Only '-device something-cpu-core,help' can get us there before > + * the machine has been created. We don't care to set nr_threads > + * in this case since it isn't used afterwards. > + */ > + if (current_machine) { > + core->nr_threads = current_machine->smp.threads; > + } > } Ack for QEMU 6.0 to get rid of the crash. But note that using current_machine might also be wrong in some cases. It's e.g. possible that the user started QEMU with a different machine type (e.g. -M g3beige) and then uses some QOM commands to introspect the available devices - in that case, this instance_init function will be executed with current_machine pointing to a G3 Mac - which is certainly also not what we want here. It likely does not crash, but still ... using current_machine or qdev_get_machine() in an instance_init() function is just wrong. This nr_thread stuff should likely be done in the realize function instead. Thomas
diff --git a/hw/cpu/core.c b/hw/cpu/core.c index 92d3b2fbad62..987607515574 100644 --- a/hw/cpu/core.c +++ b/hw/cpu/core.c @@ -66,10 +66,16 @@ static void core_prop_set_nr_threads(Object *obj, Visitor *v, const char *name, static void cpu_core_instance_init(Object *obj) { - MachineState *ms = MACHINE(qdev_get_machine()); CPUCore *core = CPU_CORE(obj); - core->nr_threads = ms->smp.threads; + /* + * Only '-device something-cpu-core,help' can get us there before + * the machine has been created. We don't care to set nr_threads + * in this case since it isn't used afterwards. + */ + if (current_machine) { + core->nr_threads = current_machine->smp.threads; + } } static void cpu_core_class_init(ObjectClass *oc, void *data)
Calling qdev_get_machine() from a QOM instance_init function is fragile because we can't be sure the machine object actually exists. And this happens to break when passing ",help" on the command line to get the list of properties for a CPU core device types : $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help qemu-system-ppc64: ../../hw/core/machine.c:1290: qdev_get_machine: Assertion `machine != NULL' failed. Aborted (core dumped) This used to work before QEMU 5.0, but commit 3df261b6676b unwillingly introduced a subtle regression : the above command line needs to create an instance but the instance_init function of the base class calls qdev_get_machine() before qemu_create_machine() has been called, which is a programming bug. Use current_machine instead. It is okay to skip the setting of nr_thread in this case since only its type is displayed. Reported-by: Thomas Huth <thuth@redhat.com> Fixes: 3df261b6676b ("softmmu/vl.c: Handle '-cpu help' and '-device help' before 'no default machine'") Cc: peter.maydell@linaro.org Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/cpu/core.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)