| Message ID | 20220713183847.41797-1-matheus.ferst@eldorado.org.br |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] target/ppc: check tb_env != 0 before printing TBU/TBL/DECR | expand |
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); > } >
On 14/07/2022 10:35, Daniel Henrique Barboza wrote: > 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? > This is the only reference to this macro since it was added in d9bce9d99f46. I suppose it was manually defined, but the only discussion[1] I could find around this patch doesn't mention it. I don't see any other reason to define it other than avoiding segfaults in machines that don't allocate env_tb, but we can keep it if you prefer. >> - 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 > > Sure, I'll change that in v3. [1] https://lists.gnu.org/archive/html/qemu-devel/2007-03/msg00239.html Thanks, Matheus K. Ferst Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/> Analista de Software Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
On 7/14/22 13:17, Matheus K. Ferst wrote: > On 14/07/2022 10:35, Daniel Henrique Barboza wrote: >> 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? >> > > This is the only reference to this macro since it was added in d9bce9d99f46. I suppose it was manually defined, but the only discussion[1] I could find around this patch doesn't mention it. I don't see any other reason to define it other than avoiding segfaults in machines that don't allocate env_tb, but we can keep it if you prefer. Thanks for the explanation. And nah, no need to keep it around since you're already gating the code by checking env->tb_env. Daniel > >>> - 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 >> >> > > Sure, I'll change that in v3. > > [1] https://lists.gnu.org/archive/html/qemu-devel/2007-03/msg00239.html > > Thanks, > Matheus K. Ferst > Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/> > Analista de Software > Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
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); 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); }
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(-)