Re: [PATCH v6 07/30] hw/sh4: Change debug printfs to traces

[Philippe Mathieu-Daudé]

On 10/29/21 23:02, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  hw/char/sh_serial.c   | 13 ++-----
>  hw/char/trace-events  |  4 +++
>  hw/intc/sh_intc.c     | 79 +++++++++++--------------------------------
>  hw/intc/trace-events  |  8 +++++
>  hw/sh4/sh7750.c       |  8 ++---
>  hw/sh4/trace-events   |  3 ++
>  hw/sh4/trace.h        |  1 +
>  hw/timer/sh_timer.c   | 22 +++---------
>  hw/timer/trace-events |  5 +++
>  meson.build           |  1 +
>  10 files changed, 52 insertions(+), 92 deletions(-)
>  create mode 100644 hw/sh4/trace-events
>  create mode 100644 hw/sh4/trace.h

> diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
> index eddad7c195..cd4e07dcba 100644
> --- a/hw/intc/sh_intc.c
> +++ b/hw/intc/sh_intc.c
> @@ -9,13 +9,12 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/log.h"
>  #include "cpu.h"
>  #include "hw/sh4/sh_intc.h"
>  #include "hw/irq.h"
>  #include "hw/sh4/sh.h"
> -
> -//#define DEBUG_INTC
> -//#define DEBUG_INTC_SOURCES
> +#include "trace.h"
>  
>  #define INTC_A7(x) ((x) & 0x1fffffff)
>  
> @@ -57,20 +56,14 @@ void sh_intc_toggle_source(struct intc_source *source,
>          }
>      }
>  
> -  if (enable_changed || assert_adj || pending_changed) {
> -#ifdef DEBUG_INTC_SOURCES
> -            printf("sh_intc: (%d/%d/%d/%d) interrupt source 0x%x %s%s%s\n",
> -                   source->parent->pending,
> -                   source->asserted,
> -                   source->enable_count,
> -                   source->enable_max,
> -                   source->vect,
> -                   source->asserted ? "asserted " :
> -                   assert_adj ? "deasserted" : "",
> -                   enable_changed == 1 ? "enabled " :
> -                   enable_changed == -1 ? "disabled " : "",
> -                   source->pending ? "pending" : "");
> -#endif
> +    if (enable_changed || assert_adj || pending_changed) {
> +        trace_sh_intc_sources(source->parent->pending, source->asserted,
> +                              source->enable_count, source->enable_max,
> +                              source->vect, source->asserted ? "asserted " :
> +                              assert_adj ? "deasserted" : "",
> +                              enable_changed == 1 ? "enabled " :
> +                              enable_changed == -1 ? "disabled " : "",
> +                              source->pending ? "pending" : "");
>      }
>  }
>  
> @@ -101,10 +94,7 @@ int sh_intc_get_pending_vector(struct intc_desc *desc, 
> int imask)
>          struct intc_source *source = desc->sources + i;
>  
>          if (source->pending) {
> -#ifdef DEBUG_INTC_SOURCES
> -            printf("sh_intc: (%d) returning interrupt source 0x%x\n",
> -                   desc->pending, source->vect);
> -#endif
> +            trace_sh_intc_pending(desc->pending, source->vect);
>              return source->vect;
>          }
>      }
> @@ -199,30 +189,22 @@ static void sh_intc_toggle_mask(struct intc_desc *desc, 
> intc_enum id,
>          return;
>      }
>      if (!source->next_enum_id && (!source->enable_max || !source->vect)) {
> -#ifdef DEBUG_INTC_SOURCES
> -        printf("sh_intc: reserved interrupt source %d modified\n", id);
> -#endif
> +        qemu_log_mask(LOG_UNIMP,
> +                      "sh_intc: reserved interrupt source %d modified\n", 
> id);
>          return;
>      }
>  
>      if (source->vect) {
>          sh_intc_toggle_source(source, enable ? 1 : -1, 0);
>      }
> -#ifdef DEBUG_INTC
> -    else {
> -        printf("setting interrupt group %d to %d\n", id, !!enable);
> -    }
> -#endif
>  
>      if ((is_group || !source->vect) && source->next_enum_id) {
>          sh_intc_toggle_mask(desc, source->next_enum_id, enable, 1);
>      }
>  
> -#ifdef DEBUG_INTC
>      if (!source->vect) {
> -        printf("setting interrupt group %d to %d - done\n", id, !!enable);
> +        trace_sh_intc_set(id, !!enable);
>      }
> -#endif
>  }
>  
>  static uint64_t sh_intc_read(void *opaque, hwaddr offset,
> @@ -235,12 +217,9 @@ static uint64_t sh_intc_read(void *opaque, hwaddr offset,
>      unsigned int mode = 0;
>      unsigned long *valuep;
>  
> -#ifdef DEBUG_INTC
> -    printf("sh_intc_read 0x%lx\n", (unsigned long) offset);
> -#endif
> -
>      sh_intc_locate(desc, (unsigned long)offset, &valuep,
>                     &enum_ids, &first, &width, &mode);
> +    trace_sh_intc_read(size, offset, *valuep);
>      return *valuep;
>  }
>  
> @@ -256,13 +235,9 @@ static void sh_intc_write(void *opaque, hwaddr offset,
>      unsigned long *valuep;
>      unsigned long mask;
>  
> -#ifdef DEBUG_INTC
> -    printf("sh_intc_write 0x%lx 0x%08x\n", (unsigned long) offset, value);
> -#endif
> -
> +    trace_sh_intc_write(size, offset, value);
>      sh_intc_locate(desc, (unsigned long)offset, &valuep,
>                     &enum_ids, &first, &width, &mode);
> -
>      switch (mode) {
>      case INTC_MODE_ENABLE_REG | INTC_MODE_IS_PRIO:
>          break;
> @@ -282,18 +257,10 @@ static void sh_intc_write(void *opaque, hwaddr offset,
>          if ((*valuep & mask) == (value & mask)) {
>              continue;
>          }
> -#if 0
> -        printf("k = %d, first = %d, enum = %d, mask = 0x%08x\n",
> -               k, first, enum_ids[k], (unsigned int)mask);
> -#endif
>          sh_intc_toggle_mask(desc, enum_ids[k], value & mask, 0);
>      }
>  
>      *valuep = value;
> -
> -#ifdef DEBUG_INTC
> -    printf("sh_intc_write 0x%lx -> 0x%08x\n", (unsigned long) offset, value);
> -#endif
>  }
>  
>  static const MemoryRegionOps sh_intc_ops = {
> @@ -416,11 +383,8 @@ void sh_intc_register_sources(struct intc_desc *desc,
>          s = sh_intc_source(desc, vect->enum_id);
>          if (s) {
>              s->vect = vect->vect;
> -
> -#ifdef DEBUG_INTC_SOURCES
> -            printf("sh_intc: registered source %d -> 0x%04x (%d/%d)\n",
> -                   vect->enum_id, s->vect, s->enable_count, s->enable_max);
> -#endif
> +            trace_sh_intc_register("source", vect->enum_id, s->vect,
> +                                   s->enable_count, s->enable_max);
>          }
>      }
>  
> @@ -438,11 +402,8 @@ void sh_intc_register_sources(struct intc_desc *desc,
>                  s = sh_intc_source(desc, gr->enum_ids[k - 1]);
>                  s->next_enum_id = gr->enum_ids[k];
>              }
> -
> -#ifdef DEBUG_INTC_SOURCES
> -            printf("sh_intc: registered group %d (%d/%d)\n",
> -                   gr->enum_id, s->enable_count, s->enable_max);
> -#endif
> +            trace_sh_intc_register("group", gr->enum_id, 0xffff,
> +                                   s->enable_count, s->enable_max);
>          }
>      }
>  }
> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
> index 6a17d38998..9c7e41f41c 100644
> --- a/hw/intc/trace-events
> +++ b/hw/intc/trace-events
> @@ -238,3 +238,11 @@ goldfish_pic_write(void *dev, int idx, unsigned int 
> addr, unsigned int size, uin
>  goldfish_pic_reset(void *dev, int idx) "pic: %p goldfish-irq.%d"
>  goldfish_pic_realize(void *dev, int idx) "pic: %p goldfish-irq.%d"
>  goldfish_pic_instance_init(void *dev) "pic: %p goldfish-irq"
> +
> +# sh_intc.c
> +sh_intc_sources(int p, int a, int c, int m, unsigned short v, const char 
> *s1, const char *s2, const char *s3) "(%d/%d/%d/%d) interrupt source 0x%x 
> %s%s%s"
> +sh_intc_pending(int p, unsigned short v) "(%d) returning interrupt source 
> 0x%x"
> +sh_intc_register(const char *s, int id, unsigned short v, int c, int m) "%s 
> %d -> 0x%04x (%d/%d)"
> +sh_intc_read(unsigned size, uint64_t offset, unsigned long val) "size %d 
> 0x%" PRIx64 " -> 0x%" PRIx64
> +sh_intc_write(unsigned size, uint64_t offset, unsigned long val) "size %d 
> 0x%" PRIx64 " <- 0x%" PRIx64
> +sh_intc_set(int id, int enable) "setting interrupt group %d to %d"

I had to include the following diff to successfully build on 32-bit
hosts:

-- >8 --
diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
index 1bc9948be30..c9b0b0c1ecc 100644
--- a/hw/intc/sh_intc.c
+++ b/hw/intc/sh_intc.c
@@ -206,7 +206,7 @@ static uint64_t sh_intc_read(void *opaque, hwaddr
offset, unsigned size)

     sh_intc_locate(desc, (unsigned long)offset, &valuep,
                    &enum_ids, &first, &width, &mode);
-    trace_sh_intc_read(size, offset, *valuep);
+    trace_sh_intc_read(size, (uint64_t)offset, *valuep);
     return *valuep;
 }

@@ -222,7 +222,7 @@ static void sh_intc_write(void *opaque, hwaddr offset,
     unsigned int k;
     unsigned long mask;

-    trace_sh_intc_write(size, offset, value);
+    trace_sh_intc_write(size, (uint64_t)offset, value);
     sh_intc_locate(desc, (unsigned long)offset, &valuep,
                    &enum_ids, &first, &width, &mode);
     switch (mode) {
diff --git a/hw/intc/trace-events b/hw/intc/trace-events
index 9c7e41f41cf..9aba7e3a7a4 100644
--- a/hw/intc/trace-events
+++ b/hw/intc/trace-events
@@ -242,7 +242,7 @@ goldfish_pic_instance_init(void *dev) "pic: %p
goldfish-irq"
 # sh_intc.c
 sh_intc_sources(int p, int a, int c, int m, unsigned short v, const
char *s1, const char *s2, const char *s3) "(%d/%d/%d/%d) interrupt
source 0x%x %s%s%s"
 sh_intc_pending(int p, unsigned short v) "(%d) returning interrupt
source 0x%x"
-sh_intc_register(const char *s, int id, unsigned short v, int c, int m)
"%s %d -> 0x%04x (%d/%d)"
-sh_intc_read(unsigned size, uint64_t offset, unsigned long val) "size
%d 0x%" PRIx64 " -> 0x%" PRIx64
-sh_intc_write(unsigned size, uint64_t offset, unsigned long val) "size
%d 0x%" PRIx64 " <- 0x%" PRIx64
+sh_intc_register(const char *s, int id, unsigned short v, int c, int m)
"%s %u -> 0x%04x (%d/%d)"
+sh_intc_read(unsigned size, uint64_t offset, unsigned long val) "size
%u 0x%" PRIx64 " -> 0x%lx"
+sh_intc_write(unsigned size, uint64_t offset, unsigned long val) "size
%u 0x%" PRIx64 " <- 0x%lx"
 sh_intc_set(int id, int enable) "setting interrupt group %d to %d"
---

Next time please run a CI pipeline.