Re: [RFC PATCH] target/ppc: don't print TB in ppc_cpu_dump_state if it's not initialized

[Daniel Henrique Barboza]

On 7/12/22 16:25, Matheus Ferst wrote:
> When using "-machine none", env->tb_env is not allocated, causing the
> segmentation fault reported in issue #85 (launchpad bug #811683). To
> avoid this problem, check if the pointer != NULL before calling the
> methods to print TBU/TBL/DECR.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/85
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> ---
> This patch fixes the reported problem, but may be an incomplete solution
> since many other places dereference env->tb_env without checking for
> NULL. AFAICS, "-machine none" is the only way to trigger this problem,
> and I'm not familiar with the use-cases for this option.

The "none"  machine type is mainly used by libvirt to do instrospection
of the available options/capabilities of the QEMU binary. It starts a QEMU
process like the following:

./x86_64-softmmu/qemu-system-x86_64 -S -no-user-config -nodefaults \
      -nographic -machine none,accel=kvm:tcg -daemonize

And then it uses QMP to probe the binary.

Aside from this libvirt usage I am not aware of anyone else using -machine
none extensively.


> Should we stop assuming env->tb_env != NULL and add checks everywhere?
> Or should we find a way to provide Time Base/Decrementer for
> "-machine none"?
> ---

Are there other cases where env->tb_env can be NULL, aside from the case
reported in the bug?

I don't mind the bug fix, but I'm not fond of the idea of adding additional
checks because of this particular issue. I mean, the bug is using  the 'prep'
machine that Thomas removed year ago in b2ce76a0730. If there's no other
foreseeable problem, that we care about, with env->tb_env being NULL, IMO
let's fix the bug and move on.



Thanks,


Daniel




>   target/ppc/cpu_init.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 86ad28466a..7e96baac9f 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -7476,18 +7476,18 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int 
> flags)
>                    "%08x iidx %d didx %d\n",
>                    env->msr, env->spr[SPR_HID0], env->hflags,
>                    cpu_mmu_index(env, true), cpu_mmu_index(env, false));
> -#if !defined(NO_TIMER_DUMP)
> -    qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
> +    if (env->tb_env) {
> +        qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
>   #if !defined(CONFIG_USER_ONLY)
> -                 " DECR " TARGET_FMT_lu
> +                     " DECR " TARGET_FMT_lu
>   #endif
> -                 "\n",
> -                 cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env)
> +                     "\n",
> +                     cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env)
>   #if !defined(CONFIG_USER_ONLY)
> -                 , cpu_ppc_load_decr(env)
> -#endif
> -        );
> +                     , cpu_ppc_load_decr(env)
>   #endif
> +            );
> +    }
>       for (i = 0; i < 32; i++) {
>           if ((i & (RGPL - 1)) == 0) {
>               qemu_fprintf(f, "GPR%02d", i);