Re: [PATCH 2/2] cpu/core: Fix "help" of CPU core device types

[Thomas Huth <thuth@redhat.com>]

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