Re: [PATCH v2] target/ppc: check tb_env != 0 before printing TBU/TBL/DECR

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 7/13/22 15:38, 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>
> ---
> v2:
>   - Added checks in monitor_get_decr, monitor_get_tbu, and monitor_get_tbl.
>   - Link to v1: https://lists.gnu.org/archive/html/qemu-ppc/2022-07/msg00173.html
> ---
>   target/ppc/cpu_init.c | 16 ++++++++--------
>   target/ppc/monitor.c  |  9 +++++++++
>   2 files changed, 17 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)

Why did you remove the NO_TIMER_DUMP check? Is it redundant with the env->tb_env
check?

> -    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
> +            );
> +    }

Not really a problem with your patch, but since you're changing this code, can
you please cleanse it from evil? I mean, look at this:


     if (env->tb_env) {
         qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
#if !defined(CONFIG_USER_ONLY)
                      " DECR " TARGET_FMT_lu
#endif
                      "\n",
                      cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env)
#if !defined(CONFIG_USER_ONLY)
                      , cpu_ppc_load_decr(env)
#endif
             );
     }


2 ifdef macros in the middle of qemu_fprintf() params? With one line starting
with a ', '?  Why are we trading sanity for 3 lines of code repetition?

We can --at least-- do something like this:

     if (env->tb_env) {
#if !defined(CONFIG_USER_ONLY)
         qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
                      " DECR " TARGET_FMT_lu
                      "\n",
                      cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env),
                      cpu_ppc_load_decr(env));
#else
         qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
                      "\n",
                      cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env));
#endif
     }


Thanks,


Daniel


>       for (i = 0; i < 32; i++) {
>           if ((i & (RGPL - 1)) == 0) {
>               qemu_fprintf(f, "GPR%02d", i);
> diff --git a/target/ppc/monitor.c b/target/ppc/monitor.c
> index 0b805ef6e9..8250b1304e 100644
> --- a/target/ppc/monitor.c
> +++ b/target/ppc/monitor.c
> @@ -55,6 +55,9 @@ static target_long monitor_get_decr(Monitor *mon, const struct MonitorDef *md,
>                                       int val)
>   {
>       CPUArchState *env = mon_get_cpu_env(mon);
> +    if (!env->tb_env) {
> +        return 0;
> +    }
>       return cpu_ppc_load_decr(env);
>   }
>   
> @@ -62,6 +65,9 @@ static target_long monitor_get_tbu(Monitor *mon, const struct MonitorDef *md,
>                                      int val)
>   {
>       CPUArchState *env = mon_get_cpu_env(mon);
> +    if (!env->tb_env) {
> +        return 0;
> +    }
>       return cpu_ppc_load_tbu(env);
>   }
>   
> @@ -69,6 +75,9 @@ static target_long monitor_get_tbl(Monitor *mon, const struct MonitorDef *md,
>                                      int val)
>   {
>       CPUArchState *env = mon_get_cpu_env(mon);
> +    if (!env->tb_env) {
> +        return 0;
> +    }
>       return cpu_ppc_load_tbl(env);
>   }
>