[Qemu-devel] [RESEND][PATCH 1/2] Log reset events

[Qemu-devel] [RESEND][PATCH 1/2] Log reset events

[Jan Kiszka]

[ Also available via git://git.kiszka.org/qemu.git queue/assorted ]

Original idea&code by Kevin Wolf, now split-up in two patches and added
more archs.

This patch introduces a flag to log CPU resets. Useful for tracing
unexpected resets (such as those triggered by x86 triple faults).

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Acked-by: Kevin Wolf <kwolf@suse.de>
---

 cpu-all.h               |    1 +
 exec.c                  |    2 ++
 target-arm/helper.c     |    6 ++++++
 target-cris/translate.c |    5 +++++
 target-i386/helper.c    |    5 +++++
 target-m68k/helper.c    |    5 +++++
 target-mips/translate.c |    5 +++++
 target-ppc/helper.c     |    8 ++++++--
 target-sh4/translate.c  |    5 +++++
 target-sparc/helper.c   |    5 +++++
 10 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/cpu-all.h b/cpu-all.h
index ee0a6e3..59c686a 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -816,6 +816,7 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr);
 #define CPU_LOG_PCALL      (1 << 6)
 #define CPU_LOG_IOPORT     (1 << 7)
 #define CPU_LOG_TB_CPU     (1 << 8)
+#define CPU_LOG_RESET      (1 << 9)
 
 /* define log items */
 typedef struct CPULogItem {
diff --git a/exec.c b/exec.c
index faa6333..fa2ebbd 100644
--- a/exec.c
+++ b/exec.c
@@ -1569,6 +1569,8 @@ const CPULogItem cpu_log_items[] = {
 #ifdef TARGET_I386
     { CPU_LOG_PCALL, "pcall",
       "show protected mode far calls/returns/exceptions" },
+    { CPU_LOG_RESET, "cpu_reset",
+      "show CPU state before CPU resets" },
 #endif
 #ifdef DEBUG_IOPORT
     { CPU_LOG_IOPORT, "ioport",
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 81663c8..90421bb 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -159,6 +159,12 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
 void cpu_reset(CPUARMState *env)
 {
     uint32_t id;
+
+    if (loglevel & CPU_LOG_RESET) {
+        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
+        cpu_dump_state(env, logfile, fprintf, 0);
+    }
+
     id = env->cp15.c0_cpuid;
     memset(env, 0, offsetof(CPUARMState, breakpoints));
     if (id)
diff --git a/target-cris/translate.c b/target-cris/translate.c
index 2ff6fe2..d6d6625 100644
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -3458,6 +3458,11 @@ CPUCRISState *cpu_cris_init (const char *cpu_model)
 
 void cpu_reset (CPUCRISState *env)
 {
+	if (loglevel & CPU_LOG_RESET) {
+		fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
+		cpu_dump_state(env, logfile, fprintf, 0);
+	}
+
 	memset(env, 0, offsetof(CPUCRISState, breakpoints));
 	tlb_flush(env, 1);
 
diff --git a/target-i386/helper.c b/target-i386/helper.c
index a28ab93..0e5b8ce 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -418,6 +418,11 @@ void cpu_reset(CPUX86State *env)
 {
     int i;
 
+    if (loglevel & CPU_LOG_RESET) {
+        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
+        cpu_dump_state(env, logfile, fprintf, X86_DUMP_FPU | X86_DUMP_CCOP);
+    }
+
     memset(env, 0, offsetof(CPUX86State, breakpoints));
 
     tlb_flush(env, 1);
diff --git a/target-m68k/helper.c b/target-m68k/helper.c
index ce56693..b708fbd 100644
--- a/target-m68k/helper.c
+++ b/target-m68k/helper.c
@@ -143,6 +143,11 @@ static int cpu_m68k_set_model(CPUM68KState *env, const char *name)
 
 void cpu_reset(CPUM68KState *env)
 {
+    if (loglevel & CPU_LOG_RESET) {
+        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
+        cpu_dump_state(env, logfile, fprintf, 0);
+    }
+
     memset(env, 0, offsetof(CPUM68KState, breakpoints));
 #if !defined (CONFIG_USER_ONLY)
     env->sr = 0x2700;
diff --git a/target-mips/translate.c b/target-mips/translate.c
index 692ea6d..7ba2528 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -8489,6 +8489,11 @@ CPUMIPSState *cpu_mips_init (const char *cpu_model)
 
 void cpu_reset (CPUMIPSState *env)
 {
+    if (loglevel & CPU_LOG_RESET) {
+        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
+        cpu_dump_state(env, logfile, fprintf, 0);
+    }
+
     memset(env, 0, offsetof(CPUMIPSState, breakpoints));
 
     tlb_flush(env, 1);
diff --git a/target-ppc/helper.c b/target-ppc/helper.c
index 2bf7650..813fe1c 100644
--- a/target-ppc/helper.c
+++ b/target-ppc/helper.c
@@ -2709,10 +2709,14 @@ void cpu_dump_rfi (target_ulong RA, target_ulong msr)
 
 void cpu_ppc_reset (void *opaque)
 {
-    CPUPPCState *env;
+    CPUPPCState *env = opaque;
     target_ulong msr;
 
-    env = opaque;
+    if (loglevel & CPU_LOG_RESET) {
+        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
+        cpu_dump_state(env, logfile, fprintf, 0);
+    }
+
     msr = (target_ulong)0;
     if (0) {
         /* XXX: find a suitable condition to enable the hypervisor mode */
diff --git a/target-sh4/translate.c b/target-sh4/translate.c
index 9137e38..93705df 100644
--- a/target-sh4/translate.c
+++ b/target-sh4/translate.c
@@ -184,6 +184,11 @@ void cpu_dump_state(CPUState * env, FILE * f,
 
 static void cpu_sh4_reset(CPUSH4State * env)
 {
+    if (loglevel & CPU_LOG_RESET) {
+        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
+        cpu_dump_state(env, logfile, fprintf, 0);
+    }
+
 #if defined(CONFIG_USER_ONLY)
     env->sr = 0;
 #else
diff --git a/target-sparc/helper.c b/target-sparc/helper.c
index d34b837..cb18bc9 100644
--- a/target-sparc/helper.c
+++ b/target-sparc/helper.c
@@ -639,6 +639,11 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr)
 
 void cpu_reset(CPUSPARCState *env)
 {
+    if (loglevel & CPU_LOG_RESET) {
+        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
+        cpu_dump_state(env, logfile, fprintf, 0);
+    }
+
     tlb_flush(env, 1);
     env->cwp = 0;
     env->wim = 1;

Re: [Qemu-devel] [RESEND][PATCH 1/2] Log reset events

[Laurent Desnogues]

On Fri, Jan 16, 2009 at 11:05 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
[...]
>
> This patch introduces a flag to log CPU resets. Useful for tracing
> unexpected resets (such as those triggered by x86 triple faults).

[...]

> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 81663c8..90421bb 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -159,6 +159,12 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
>  void cpu_reset(CPUARMState *env)
>  {
>     uint32_t id;
> +
> +    if (loglevel & CPU_LOG_RESET) {
> +        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
> +        cpu_dump_state(env, logfile, fprintf, 0);
> +    }

You should use the new log API which has been introduced yesterday
(look in qemu-common.h).


Laurent

[Qemu-devel] Re: [RESEND][PATCH 1/2] Log reset events

[Jan Kiszka]

Laurent Desnogues wrote:
> On Fri, Jan 16, 2009 at 11:05 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> [...]
>> This patch introduces a flag to log CPU resets. Useful for tracing
>> unexpected resets (such as those triggered by x86 triple faults).
> 
> [...]
> 
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 81663c8..90421bb 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -159,6 +159,12 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
>>  void cpu_reset(CPUARMState *env)
>>  {
>>     uint32_t id;
>> +
>> +    if (loglevel & CPU_LOG_RESET) {
>> +        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
>> +        cpu_dump_state(env, logfile, fprintf, 0);
>> +    }
> 
> You should use the new log API which has been introduced yesterday
> (look in qemu-common.h).
> 

Oh, OK, will check this.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT SE 26
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH -v2 1/2] Log reset events

[Jan Kiszka]

[ Updated to use new log API. ]

Original idea&code by Kevin Wolf, now split-up in two patches and added
more archs.

This patch introduces a flag to log CPU resets. Useful for tracing
unexpected resets (such as those triggered by x86 triple faults).

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Acked-by: Kevin Wolf <kwolf@suse.de>
---

 cpu-all.h               |    1 +
 exec.c                  |    2 ++
 target-arm/helper.c     |    6 ++++++
 target-cris/translate.c |    5 +++++
 target-i386/helper.c    |    5 +++++
 target-m68k/helper.c    |    5 +++++
 target-mips/translate.c |    5 +++++
 target-ppc/helper.c     |    8 ++++++--
 target-sh4/translate.c  |    5 +++++
 target-sparc/helper.c   |    5 +++++
 10 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/cpu-all.h b/cpu-all.h
index ee0a6e3..59c686a 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -816,6 +816,7 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr);
 #define CPU_LOG_PCALL      (1 << 6)
 #define CPU_LOG_IOPORT     (1 << 7)
 #define CPU_LOG_TB_CPU     (1 << 8)
+#define CPU_LOG_RESET      (1 << 9)
 
 /* define log items */
 typedef struct CPULogItem {
diff --git a/exec.c b/exec.c
index faa6333..fa2ebbd 100644
--- a/exec.c
+++ b/exec.c
@@ -1569,6 +1569,8 @@ const CPULogItem cpu_log_items[] = {
 #ifdef TARGET_I386
     { CPU_LOG_PCALL, "pcall",
       "show protected mode far calls/returns/exceptions" },
+    { CPU_LOG_RESET, "cpu_reset",
+      "show CPU state before CPU resets" },
 #endif
 #ifdef DEBUG_IOPORT
     { CPU_LOG_IOPORT, "ioport",
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 81663c8..a243d9b 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -159,6 +159,12 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
 void cpu_reset(CPUARMState *env)
 {
     uint32_t id;
+
+    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
+        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
+        cpu_dump_state(env, logfile, fprintf, 0);
+    }
+
     id = env->cp15.c0_cpuid;
     memset(env, 0, offsetof(CPUARMState, breakpoints));
     if (id)
diff --git a/target-cris/translate.c b/target-cris/translate.c
index 2ff6fe2..7e07916 100644
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -3458,6 +3458,11 @@ CPUCRISState *cpu_cris_init (const char *cpu_model)
 
 void cpu_reset (CPUCRISState *env)
 {
+	if (qemu_loglevel_mask(CPU_LOG_RESET)) {
+		fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
+		cpu_dump_state(env, logfile, fprintf, 0);
+	}
+
 	memset(env, 0, offsetof(CPUCRISState, breakpoints));
 	tlb_flush(env, 1);
 
diff --git a/target-i386/helper.c b/target-i386/helper.c
index a28ab93..8cdc01f 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -418,6 +418,11 @@ void cpu_reset(CPUX86State *env)
 {
     int i;
 
+    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
+        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
+        cpu_dump_state(env, logfile, fprintf, X86_DUMP_FPU | X86_DUMP_CCOP);
+    }
+
     memset(env, 0, offsetof(CPUX86State, breakpoints));
 
     tlb_flush(env, 1);
diff --git a/target-m68k/helper.c b/target-m68k/helper.c
index ce56693..d70e355 100644
--- a/target-m68k/helper.c
+++ b/target-m68k/helper.c
@@ -143,6 +143,11 @@ static int cpu_m68k_set_model(CPUM68KState *env, const char *name)
 
 void cpu_reset(CPUM68KState *env)
 {
+    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
+        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
+        cpu_dump_state(env, logfile, fprintf, 0);
+    }
+
     memset(env, 0, offsetof(CPUM68KState, breakpoints));
 #if !defined (CONFIG_USER_ONLY)
     env->sr = 0x2700;
diff --git a/target-mips/translate.c b/target-mips/translate.c
index 692ea6d..c9b7f5c 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -8489,6 +8489,11 @@ CPUMIPSState *cpu_mips_init (const char *cpu_model)
 
 void cpu_reset (CPUMIPSState *env)
 {
+    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
+        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
+        cpu_dump_state(env, logfile, fprintf, 0);
+    }
+
     memset(env, 0, offsetof(CPUMIPSState, breakpoints));
 
     tlb_flush(env, 1);
diff --git a/target-ppc/helper.c b/target-ppc/helper.c
index 2bf7650..613df0b 100644
--- a/target-ppc/helper.c
+++ b/target-ppc/helper.c
@@ -2709,10 +2709,14 @@ void cpu_dump_rfi (target_ulong RA, target_ulong msr)
 
 void cpu_ppc_reset (void *opaque)
 {
-    CPUPPCState *env;
+    CPUPPCState *env = opaque;
     target_ulong msr;
 
-    env = opaque;
+    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
+        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
+        cpu_dump_state(env, logfile, fprintf, 0);
+    }
+
     msr = (target_ulong)0;
     if (0) {
         /* XXX: find a suitable condition to enable the hypervisor mode */
diff --git a/target-sh4/translate.c b/target-sh4/translate.c
index 9137e38..690344a 100644
--- a/target-sh4/translate.c
+++ b/target-sh4/translate.c
@@ -184,6 +184,11 @@ void cpu_dump_state(CPUState * env, FILE * f,
 
 static void cpu_sh4_reset(CPUSH4State * env)
 {
+    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
+        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
+        cpu_dump_state(env, logfile, fprintf, 0);
+    }
+
 #if defined(CONFIG_USER_ONLY)
     env->sr = 0;
 #else
diff --git a/target-sparc/helper.c b/target-sparc/helper.c
index d34b837..8598249 100644
--- a/target-sparc/helper.c
+++ b/target-sparc/helper.c
@@ -639,6 +639,11 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr)
 
 void cpu_reset(CPUSPARCState *env)
 {
+    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
+        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
+        cpu_dump_state(env, logfile, fprintf, 0);
+    }
+
     tlb_flush(env, 1);
     env->cwp = 0;
     env->wim = 1;

Re: [Qemu-devel] [PATCH -v2 1/2] Log reset events

[Laurent Desnogues]

On Fri, Jan 16, 2009 at 1:14 PM, Kevin Wolf <kwolf@suse.de> wrote:
[..]
>> @@ -159,6 +159,12 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
>>  void cpu_reset(CPUARMState *env)
>>  {
>>      uint32_t id;
>> +
>> +    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
>> +        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
>> +        cpu_dump_state(env, logfile, fprintf, 0);
>
> Shouldn't that be qemu_log() and log_cpu_state() now?

Yes. Sorry Jan :)

A trick to check things are done properly is to make sure you
don't ever use logfile or loglevel variables.


Laurent

Re: [Qemu-devel] [PATCH -v2 1/2] Log reset events

[Kevin Wolf]

Jan Kiszka schrieb:
> [ Updated to use new log API. ]
> 
> Original idea&code by Kevin Wolf, now split-up in two patches and added
> more archs.
> 
> This patch introduces a flag to log CPU resets. Useful for tracing
> unexpected resets (such as those triggered by x86 triple faults).
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> Acked-by: Kevin Wolf <kwolf@suse.de>
> ---
> 
>  cpu-all.h               |    1 +
>  exec.c                  |    2 ++
>  target-arm/helper.c     |    6 ++++++
>  target-cris/translate.c |    5 +++++
>  target-i386/helper.c    |    5 +++++
>  target-m68k/helper.c    |    5 +++++
>  target-mips/translate.c |    5 +++++
>  target-ppc/helper.c     |    8 ++++++--
>  target-sh4/translate.c  |    5 +++++
>  target-sparc/helper.c   |    5 +++++
>  10 files changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/cpu-all.h b/cpu-all.h
> index ee0a6e3..59c686a 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -816,6 +816,7 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr);
>  #define CPU_LOG_PCALL      (1 << 6)
>  #define CPU_LOG_IOPORT     (1 << 7)
>  #define CPU_LOG_TB_CPU     (1 << 8)
> +#define CPU_LOG_RESET      (1 << 9)
>  
>  /* define log items */
>  typedef struct CPULogItem {
> diff --git a/exec.c b/exec.c
> index faa6333..fa2ebbd 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1569,6 +1569,8 @@ const CPULogItem cpu_log_items[] = {
>  #ifdef TARGET_I386
>      { CPU_LOG_PCALL, "pcall",
>        "show protected mode far calls/returns/exceptions" },
> +    { CPU_LOG_RESET, "cpu_reset",
> +      "show CPU state before CPU resets" },
>  #endif
>  #ifdef DEBUG_IOPORT
>      { CPU_LOG_IOPORT, "ioport",
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 81663c8..a243d9b 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -159,6 +159,12 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
>  void cpu_reset(CPUARMState *env)
>  {
>      uint32_t id;
> +
> +    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
> +        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
> +        cpu_dump_state(env, logfile, fprintf, 0);

Shouldn't that be qemu_log() and log_cpu_state() now?

Kevin

Re: [Qemu-devel] [PATCH -v2 1/2] Log reset events

[Laurent Desnogues]

On Fri, Jan 16, 2009 at 1:25 PM, Kevin Wolf <kwolf@suse.de> wrote:
> Maybe qemu_log() should be a function rather than a macro? Then loglevel
> and logfile could be static and the build would break if you try to use
> them outside the logging code.

I think the intent is to make sure no useless call is made if not needed.
Of course that'd mean conditionally defining "empty" macros which is
currently not the case.


Laurent

Re: [Qemu-devel] [PATCH -v2 1/2] Log reset events

[Kevin Wolf]

Laurent Desnogues schrieb:
> On Fri, Jan 16, 2009 at 1:14 PM, Kevin Wolf <kwolf@suse.de> wrote:
> [..]
>>> @@ -159,6 +159,12 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
>>>  void cpu_reset(CPUARMState *env)
>>>  {
>>>      uint32_t id;
>>> +
>>> +    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
>>> +        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
>>> +        cpu_dump_state(env, logfile, fprintf, 0);
>> Shouldn't that be qemu_log() and log_cpu_state() now?
> 
> Yes. Sorry Jan :)
> 
> A trick to check things are done properly is to make sure you
> don't ever use logfile or loglevel variables.

Maybe qemu_log() should be a function rather than a macro? Then loglevel
and logfile could be static and the build would break if you try to use
them outside the logging code.

Kevin

[Qemu-devel] Re: [PATCH -v2 1/2] Log reset events

[Jan Kiszka]

Kevin Wolf wrote:
> Laurent Desnogues schrieb:
>> On Fri, Jan 16, 2009 at 1:14 PM, Kevin Wolf <kwolf@suse.de> wrote:
>> [..]
>>>> @@ -159,6 +159,12 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
>>>>  void cpu_reset(CPUARMState *env)
>>>>  {
>>>>      uint32_t id;
>>>> +
>>>> +    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
>>>> +        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
>>>> +        cpu_dump_state(env, logfile, fprintf, 0);
>>> Shouldn't that be qemu_log() and log_cpu_state() now?
>> Yes. Sorry Jan :)

Well, the patch description misled me a bit ("global s/loglevel &
X/qemu_loglevel_mask(X)/"), but I also could have checked more carefully
what actually changed.

>>
>> A trick to check things are done properly is to make sure you
>> don't ever use logfile or loglevel variables.
> 
> Maybe qemu_log() should be a function rather than a macro? Then loglevel
> and logfile could be static and the build would break if you try to use
> them outside the logging code.

Sounds good (function-like macros are for corner cases only).

I will try to make logfile/loglevel static and fix my patch accordingly.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 26
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH -v2 1/2] Log reset events

[Jan Kiszka]

Jan Kiszka wrote:
> Kevin Wolf wrote:
>> Laurent Desnogues schrieb:
>>> On Fri, Jan 16, 2009 at 1:14 PM, Kevin Wolf <kwolf@suse.de> wrote:
>>> [..]
>>>>> @@ -159,6 +159,12 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
>>>>>  void cpu_reset(CPUARMState *env)
>>>>>  {
>>>>>      uint32_t id;
>>>>> +
>>>>> +    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
>>>>> +        fprintf(logfile, "CPU Reset (CPU %d)\n", env->cpu_index);
>>>>> +        cpu_dump_state(env, logfile, fprintf, 0);
>>>> Shouldn't that be qemu_log() and log_cpu_state() now?
>>> Yes. Sorry Jan :)
> 
> Well, the patch description misled me a bit ("global s/loglevel &
> X/qemu_loglevel_mask(X)/"), but I also could have checked more carefully
> what actually changed.
> 
>>> A trick to check things are done properly is to make sure you
>>> don't ever use logfile or loglevel variables.
>> Maybe qemu_log() should be a function rather than a macro? Then loglevel
>> and logfile could be static and the build would break if you try to use
>> them outside the logging code.
> 
> Sounds good (function-like macros are for corner cases only).
> 
> I will try to make logfile/loglevel static and fix my patch accordingly.

Making logfile and loglevel static means enforcing a function call just
to check if we perform logging. Would the additional overhead be
acceptable for all qemu_log* spots? Unless maintainers tell me "yes", I
will not touch this part for now.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 26
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH -v2 1/2] Log reset events

[Kevin Wolf]

Jan Kiszka schrieb:
> Making logfile and loglevel static means enforcing a function call just
> to check if we perform logging. Would the additional overhead be
> acceptable for all qemu_log* spots? Unless maintainers tell me "yes", I
> will not touch this part for now.

Well, seems you have a point... I guess that's not acceptable. But
leaving loglevel global and making logfile static should still be
possible, right?

Kevin

[Qemu-devel] [PATCH -v3 1/2] Log reset events

[Jan Kiszka]

[ Updated to use the new log API, but now consistently - I hope. ]

Original idea&code by Kevin Wolf, split-up in two patches and added more
archs.

This patch introduces a flag to log CPU resets. Useful for tracing
unexpected resets (such as those triggered by x86 triple faults).

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 cpu-all.h               |    1 +
 exec.c                  |    2 ++
 target-arm/helper.c     |    6 ++++++
 target-cris/translate.c |    5 +++++
 target-i386/helper.c    |    5 +++++
 target-m68k/helper.c    |    5 +++++
 target-mips/translate.c |    5 +++++
 target-ppc/helper.c     |    8 ++++++--
 target-sh4/translate.c  |    5 +++++
 target-sparc/helper.c   |    5 +++++
 10 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/cpu-all.h b/cpu-all.h
index ee0a6e3..59c686a 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -816,6 +816,7 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr);
 #define CPU_LOG_PCALL      (1 << 6)
 #define CPU_LOG_IOPORT     (1 << 7)
 #define CPU_LOG_TB_CPU     (1 << 8)
+#define CPU_LOG_RESET      (1 << 9)
 
 /* define log items */
 typedef struct CPULogItem {
diff --git a/exec.c b/exec.c
index faa6333..fa2ebbd 100644
--- a/exec.c
+++ b/exec.c
@@ -1569,6 +1569,8 @@ const CPULogItem cpu_log_items[] = {
 #ifdef TARGET_I386
     { CPU_LOG_PCALL, "pcall",
       "show protected mode far calls/returns/exceptions" },
+    { CPU_LOG_RESET, "cpu_reset",
+      "show CPU state before CPU resets" },
 #endif
 #ifdef DEBUG_IOPORT
     { CPU_LOG_IOPORT, "ioport",
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 81663c8..26fd6d0 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -159,6 +159,12 @@ static void cpu_reset_model_id(CPUARMState *env, uint32_t id)
 void cpu_reset(CPUARMState *env)
 {
     uint32_t id;
+
+    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
+        qemu_log("CPU Reset (CPU %d)\n", env->cpu_index);
+        log_cpu_state(env, 0);
+    }
+
     id = env->cp15.c0_cpuid;
     memset(env, 0, offsetof(CPUARMState, breakpoints));
     if (id)
diff --git a/target-cris/translate.c b/target-cris/translate.c
index 2ff6fe2..b6b987a 100644
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -3458,6 +3458,11 @@ CPUCRISState *cpu_cris_init (const char *cpu_model)
 
 void cpu_reset (CPUCRISState *env)
 {
+	if (qemu_loglevel_mask(CPU_LOG_RESET)) {
+		qemu_log("CPU Reset (CPU %d)\n", env->cpu_index);
+		log_cpu_state(env, 0);
+	}
+
 	memset(env, 0, offsetof(CPUCRISState, breakpoints));
 	tlb_flush(env, 1);
 
diff --git a/target-i386/helper.c b/target-i386/helper.c
index a28ab93..c2da767 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -418,6 +418,11 @@ void cpu_reset(CPUX86State *env)
 {
     int i;
 
+    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
+        qemu_log("CPU Reset (CPU %d)\n", env->cpu_index);
+        log_cpu_state(env, X86_DUMP_FPU | X86_DUMP_CCOP);
+    }
+
     memset(env, 0, offsetof(CPUX86State, breakpoints));
 
     tlb_flush(env, 1);
diff --git a/target-m68k/helper.c b/target-m68k/helper.c
index ce56693..076ea35 100644
--- a/target-m68k/helper.c
+++ b/target-m68k/helper.c
@@ -143,6 +143,11 @@ static int cpu_m68k_set_model(CPUM68KState *env, const char *name)
 
 void cpu_reset(CPUM68KState *env)
 {
+    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
+        qemu_log("CPU Reset (CPU %d)\n", env->cpu_index);
+        log_cpu_state(env, 0);
+    }
+
     memset(env, 0, offsetof(CPUM68KState, breakpoints));
 #if !defined (CONFIG_USER_ONLY)
     env->sr = 0x2700;
diff --git a/target-mips/translate.c b/target-mips/translate.c
index 692ea6d..b447b98 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -8489,6 +8489,11 @@ CPUMIPSState *cpu_mips_init (const char *cpu_model)
 
 void cpu_reset (CPUMIPSState *env)
 {
+    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
+        qemu_log("CPU Reset (CPU %d)\n", env->cpu_index);
+        log_cpu_state(env, 0);
+    }
+
     memset(env, 0, offsetof(CPUMIPSState, breakpoints));
 
     tlb_flush(env, 1);
diff --git a/target-ppc/helper.c b/target-ppc/helper.c
index 2bf7650..3bd1d45 100644
--- a/target-ppc/helper.c
+++ b/target-ppc/helper.c
@@ -2709,10 +2709,14 @@ void cpu_dump_rfi (target_ulong RA, target_ulong msr)
 
 void cpu_ppc_reset (void *opaque)
 {
-    CPUPPCState *env;
+    CPUPPCState *env = opaque;
     target_ulong msr;
 
-    env = opaque;
+    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
+        qemu_log("CPU Reset (CPU %d)\n", env->cpu_index);
+        log_cpu_state(env, 0);
+    }
+
     msr = (target_ulong)0;
     if (0) {
         /* XXX: find a suitable condition to enable the hypervisor mode */
diff --git a/target-sh4/translate.c b/target-sh4/translate.c
index 9137e38..6c9dff5 100644
--- a/target-sh4/translate.c
+++ b/target-sh4/translate.c
@@ -184,6 +184,11 @@ void cpu_dump_state(CPUState * env, FILE * f,
 
 static void cpu_sh4_reset(CPUSH4State * env)
 {
+    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
+        qemu_log("CPU Reset (CPU %d)\n", env->cpu_index);
+        log_cpu_state(env, 0);
+    }
+
 #if defined(CONFIG_USER_ONLY)
     env->sr = 0;
 #else
diff --git a/target-sparc/helper.c b/target-sparc/helper.c
index d34b837..a275880 100644
--- a/target-sparc/helper.c
+++ b/target-sparc/helper.c
@@ -639,6 +639,11 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr)
 
 void cpu_reset(CPUSPARCState *env)
 {
+    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
+        qemu_log("CPU Reset (CPU %d)\n", env->cpu_index);
+        log_cpu_state(env, 0);
+    }
+
     tlb_flush(env, 1);
     env->cwp = 0;
     env->wim = 1;

Re: [Qemu-devel] [PATCH -v3 1/2] Log reset events

[Kevin Wolf]

Jan Kiszka schrieb:
> [ Updated to use the new log API, but now consistently - I hope. ]
> 
> Original idea&code by Kevin Wolf, split-up in two patches and added more
> archs.
> 
> This patch introduces a flag to log CPU resets. Useful for tracing
> unexpected resets (such as those triggered by x86 triple faults).
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Looks good now. So here is my line back again... ;-)

Acked-by: Kevin Wolf <kwolf@suse.de>

[Qemu-devel] Re: [PATCH -v3 1/2] Log reset events

[Jan Kiszka]

Kevin Wolf wrote:
> Jan Kiszka schrieb:
>> [ Updated to use the new log API, but now consistently - I hope. ]
>>
>> Original idea&code by Kevin Wolf, split-up in two patches and added more
>> archs.
>>
>> This patch introduces a flag to log CPU resets. Useful for tracing
>> unexpected resets (such as those triggered by x86 triple faults).
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Looks good now. So here is my line back again... ;-)
> 
> Acked-by: Kevin Wolf <kwolf@suse.de>
> 

Puh! :)

Jan

-- 
Siemens AG, Corporate Technology, CT SE 26
Corporate Competence Center Embedded Linux