* [PATCH v2] target/ppc: check tb_env != 0 before printing TBU/TBL/DECR
@ 2022-07-13 18:38 Matheus Ferst
2022-07-14 13:35 ` Daniel Henrique Barboza
0 siblings, 1 reply; 4+ messages in thread
From: Matheus Ferst @ 2022-07-13 18:38 UTC (permalink / raw)
To: qemu-devel, qemu-ppc; +Cc: clg, danielhb413, david, groug, Matheus Ferst
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)
- 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);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] target/ppc: check tb_env != 0 before printing TBU/TBL/DECR
2022-07-13 18:38 [PATCH v2] target/ppc: check tb_env != 0 before printing TBU/TBL/DECR Matheus Ferst
@ 2022-07-14 13:35 ` Daniel Henrique Barboza
2022-07-14 16:17 ` Matheus K. Ferst
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Henrique Barboza @ 2022-07-14 13:35 UTC (permalink / raw)
To: Matheus Ferst, qemu-devel, qemu-ppc; +Cc: clg, david, groug
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);
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] target/ppc: check tb_env != 0 before printing TBU/TBL/DECR
2022-07-14 13:35 ` Daniel Henrique Barboza
@ 2022-07-14 16:17 ` Matheus K. Ferst
2022-07-14 16:30 ` Daniel Henrique Barboza
0 siblings, 1 reply; 4+ messages in thread
From: Matheus K. Ferst @ 2022-07-14 16:17 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-devel, qemu-ppc; +Cc: clg, david, groug
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>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] target/ppc: check tb_env != 0 before printing TBU/TBL/DECR
2022-07-14 16:17 ` Matheus K. Ferst
@ 2022-07-14 16:30 ` Daniel Henrique Barboza
0 siblings, 0 replies; 4+ messages in thread
From: Daniel Henrique Barboza @ 2022-07-14 16:30 UTC (permalink / raw)
To: Matheus K. Ferst, qemu-devel, qemu-ppc; +Cc: clg, david, groug
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>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-07-14 16:32 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-13 18:38 [PATCH v2] target/ppc: check tb_env != 0 before printing TBU/TBL/DECR Matheus Ferst
2022-07-14 13:35 ` Daniel Henrique Barboza
2022-07-14 16:17 ` Matheus K. Ferst
2022-07-14 16:30 ` Daniel Henrique Barboza
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).