Re: [Qemu-devel] [PATCH v2] qemu-log: add log category for MMU info

2014-10-13 Thread Alexander Graf 


On 10.10.14 16:47, Peter Maydell wrote:
 On 10 October 2014 11:59, Antony Pavlov antonynpav...@gmail.com wrote:
 Running barebox on qemu-system-mips* with '-d unimp' overloads
 stderr by very very many mips_cpu_handle_mmu_fault() messages:

   mips_cpu_handle_mmu_fault address=b80003fd ret 0 physical 180003fd 
 prot 3
   mips_cpu_handle_mmu_fault address=a0800884 ret 0 physical 00800884 
 prot 3
   mips_cpu_handle_mmu_fault pc a080cd80 ad b80003fd rw 0 mmu_idx 0

 So it's very difficult to find LOG_UNIMP message.

 The mips_cpu_handle_mmu_fault() messages appears on enabling ANY
 logging! It's not very handy.

 Adding separate log category for *_cpu_handle_mmu_fault()
 logging fixes the problem.

 Signed-off-by: Antony Pavlov antonynpav...@gmail.com
 
 This mostly looks good, thanks!
 
 I should also note that I'm happy for us to just implement
 the common-code (ie the log flag) and fix those targets
 which are particularly obnoxious about logging and/or
 easy to fix. We can always leave the other targets to
 update their code later (or you could update other targets
 in separate patches once the main one has gone in).
 
 A minor tweak:
 
 --- a/target-cris/helper.c
 +++ b/target-cris/helper.c
 @@ -30,9 +30,11 @@
  #ifdef CRIS_HELPER_DEBUG
  #define D(x) x
  #define D_LOG(...) qemu_log(__VA_ARGS__)
 +#define LOG_MMU(...) qemu_log_mask(CPU_LOG_MMU, __VA_ARGS__)
  #else
  #define D(x)
  #define D_LOG(...) do { } while (0)
 +#define LOG_MMU(...) do { } while (0)
  #endif
 
 Now this logging is configurably enablable at runtime,
 we should just call qemu_log_mask() directly, rather
 than wrapping it in a LOG_MMU macro which might be compiled
 out.

The MMU lookups are in a pretty hot path. Are you sure we always want to
have the log enabled checks and register pollution in there?


Alex

[Qemu-devel] [PATCH v2] qemu-log: add log category for MMU info

2014-10-10 Thread Antony Pavlov 
Running barebox on qemu-system-mips* with '-d unimp' overloads
stderr by very very many mips_cpu_handle_mmu_fault() messages:

  mips_cpu_handle_mmu_fault address=b80003fd ret 0 physical 180003fd 
prot 3
  mips_cpu_handle_mmu_fault address=a0800884 ret 0 physical 00800884 
prot 3
  mips_cpu_handle_mmu_fault pc a080cd80 ad b80003fd rw 0 mmu_idx 0

So it's very difficult to find LOG_UNIMP message.

The mips_cpu_handle_mmu_fault() messages appears on enabling ANY
logging! It's not very handy.

Adding separate log category for *_cpu_handle_mmu_fault()
logging fixes the problem.

Signed-off-by: Antony Pavlov antonynpav...@gmail.com
---

Changes since v1:

 * add cris, i386, microblaze, ppc, s390x, sparc and unicore32 support.

---

 cputlb.c   |  3 ++-
 include/qemu/log.h |  1 +
 qemu-log.c |  2 ++
 target-cris/helper.c   | 12 +++-
 target-i386/helper.c   | 18 +-
 target-microblaze/helper.c | 15 +++
 target-mips/helper.c   |  6 --
 target-ppc/mmu-hash32.c|  2 +-
 target-s390x/helper.c  |  4 ++--
 target-sparc/mmu_helper.c  | 14 ++
 target-unicore32/softmmu.c |  3 ++-
 11 files changed, 51 insertions(+), 29 deletions(-)

diff --git a/cputlb.c b/cputlb.c
index a55518a..3b271d4 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -270,7 +270,8 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
 assert(sz = TARGET_PAGE_SIZE);
 
 #if defined(DEBUG_TLB)
-printf(tlb_set_page: vaddr= TARGET_FMT_lx  paddr=0x TARGET_FMT_plx
+qemu_log_mask(CPU_LOG_MMU,
+   tlb_set_page: vaddr= TARGET_FMT_lx  paddr=0x TARGET_FMT_plx
 prot=%x idx=%d\n,
vaddr, paddr, prot, mmu_idx);
 #endif
diff --git a/include/qemu/log.h b/include/qemu/log.h
index d515424..195f665 100644
--- a/include/qemu/log.h
+++ b/include/qemu/log.h
@@ -40,6 +40,7 @@ static inline bool qemu_log_enabled(void)
 #define CPU_LOG_RESET  (1  9)
 #define LOG_UNIMP  (1  10)
 #define LOG_GUEST_ERROR(1  11)
+#define CPU_LOG_MMU(1  12)
 
 /* Returns true if a bit is set in the current loglevel mask
  */
diff --git a/qemu-log.c b/qemu-log.c
index 797f2af..05b5493 100644
--- a/qemu-log.c
+++ b/qemu-log.c
@@ -106,6 +106,8 @@ const QEMULogItem qemu_log_items[] = {
   show trace before each executed TB (lots of logs) },
 { CPU_LOG_TB_CPU, cpu,
   show CPU state before block translation },
+{ CPU_LOG_MMU, mmu,
+  log MMU-related activities },
 { CPU_LOG_PCALL, pcall,
   x86 only: show protected mode far calls/returns/exceptions },
 { CPU_LOG_RESET, cpu_reset,
diff --git a/target-cris/helper.c b/target-cris/helper.c
index e901c3a..e368e23 100644
--- a/target-cris/helper.c
+++ b/target-cris/helper.c
@@ -30,9 +30,11 @@
 #ifdef CRIS_HELPER_DEBUG
 #define D(x) x
 #define D_LOG(...) qemu_log(__VA_ARGS__)
+#define LOG_MMU(...) qemu_log_mask(CPU_LOG_MMU, __VA_ARGS__)
 #else
 #define D(x)
 #define D_LOG(...) do { } while (0)
+#define LOG_MMU(...) do { } while (0)
 #endif
 
 #if defined(CONFIG_USER_ONLY)
@@ -84,8 +86,8 @@ int cris_cpu_handle_mmu_fault(CPUState *cs, vaddr address, 
int rw,
 int r = -1;
 target_ulong phy;
 
-D(printf(%s addr=% VADDR_PRIx  pc=%x rw=%x\n,
- __func__, address, env-pc, rw));
+LOG_MMU(%s addr=% VADDR_PRIx  pc=%x rw=%x\n,
+__func__, address, env-pc, rw);
 miss = cris_mmu_translate(res, env, address  TARGET_PAGE_MASK,
   rw, mmu_idx, 0);
 if (miss) {
@@ -112,9 +114,9 @@ int cris_cpu_handle_mmu_fault(CPUState *cs, vaddr address, 
int rw,
 r = 0;
 }
 if (r  0) {
-D_LOG(%s returns %d irqreq=%x addr=% VADDR_PRIx  phy=%x vec=%x
-   pc=%x\n, __func__, r, cs-interrupt_request, address, res.phy,
-  res.bf_vec, env-pc);
+LOG_MMU(%s returns %d irqreq=%x addr=% VADDR_PRIx  phy=%x vec=%x
+ pc=%x\n, __func__, r, cs-interrupt_request, address,
+res.phy, res.bf_vec, env-pc);
 }
 return r;
 }
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 345bda1..ae159a3 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -27,6 +27,12 @@
 
 //#define DEBUG_MMU
 
+#ifdef DEBUG_MMU
+#  define LOG_MMU(...) qemu_log_mask(CPU_LOG_MMU, __VA_ARGS__)
+#else
+#  define LOG_MMU(...) do { } while (0)
+#endif
+
 static void cpu_x86_version(CPUX86State *env, int *family, int *model)
 {
 int cpuver = env-cpuid_version;
@@ -388,9 +394,7 @@ void x86_cpu_set_a20(X86CPU *cpu, int a20_state)
 if (a20_state != ((env-a20_mask  20)  1)) {
 CPUState *cs = CPU(cpu);
 
-#if defined(DEBUG_MMU)
-printf(A20 update: a20=%d\n, a20_state);
-#endif
+LOG_MMU(A20 update: a20=%d\n, a20_state);
 /* if the cpu is currently executing code, we must unlink it and
all the potentially executing TB */
 cpu_interrupt(cs, CPU_INTERRUPT_EXITTB);
@@ -407,9 +411,7 @@ void

Re: [Qemu-devel] [PATCH v2] qemu-log: add log category for MMU info

2014-10-10 Thread Peter Maydell 
On 10 October 2014 11:59, Antony Pavlov antonynpav...@gmail.com wrote:
 Running barebox on qemu-system-mips* with '-d unimp' overloads
 stderr by very very many mips_cpu_handle_mmu_fault() messages:

   mips_cpu_handle_mmu_fault address=b80003fd ret 0 physical 180003fd 
 prot 3
   mips_cpu_handle_mmu_fault address=a0800884 ret 0 physical 00800884 
 prot 3
   mips_cpu_handle_mmu_fault pc a080cd80 ad b80003fd rw 0 mmu_idx 0

 So it's very difficult to find LOG_UNIMP message.

 The mips_cpu_handle_mmu_fault() messages appears on enabling ANY
 logging! It's not very handy.

 Adding separate log category for *_cpu_handle_mmu_fault()
 logging fixes the problem.

 Signed-off-by: Antony Pavlov antonynpav...@gmail.com

This mostly looks good, thanks!

I should also note that I'm happy for us to just implement
the common-code (ie the log flag) and fix those targets
which are particularly obnoxious about logging and/or
easy to fix. We can always leave the other targets to
update their code later (or you could update other targets
in separate patches once the main one has gone in).

A minor tweak:

 --- a/target-cris/helper.c
 +++ b/target-cris/helper.c
 @@ -30,9 +30,11 @@
  #ifdef CRIS_HELPER_DEBUG
  #define D(x) x
  #define D_LOG(...) qemu_log(__VA_ARGS__)
 +#define LOG_MMU(...) qemu_log_mask(CPU_LOG_MMU, __VA_ARGS__)
  #else
  #define D(x)
  #define D_LOG(...) do { } while (0)
 +#define LOG_MMU(...) do { } while (0)
  #endif

Now this logging is configurably enablable at runtime,
we should just call qemu_log_mask() directly, rather
than wrapping it in a LOG_MMU macro which might be compiled
out.

 --- a/target-i386/helper.c
 +++ b/target-i386/helper.c
 @@ -27,6 +27,12 @@

  //#define DEBUG_MMU

 +#ifdef DEBUG_MMU
 +#  define LOG_MMU(...) qemu_log_mask(CPU_LOG_MMU, __VA_ARGS__)
 +#else
 +#  define LOG_MMU(...) do { } while (0)
 +#endif

Similarly here.

 --- a/target-microblaze/helper.c
 +++ b/target-microblaze/helper.c
 @@ -22,7 +22,14 @@
  #include qemu/host-utils.h

  #define D(x)
 -#define DMMU(x)
 +
 +#undef DEBUG_MMU
 +
 +#ifdef DEBUG_MMU
 +#  define LOG_MMU(...) qemu_log_mask(CPU_LOG_MMU, __VA_ARGS__)
 +#else
 +#  define LOG_MMU(...) do { } while (0)
 +#endif

and here.

 --- a/target-ppc/mmu-hash32.c
 +++ b/target-ppc/mmu-hash32.c

There are other PPC files which also do MMU logging;
we should either fix them all or none of them.

thanks
-- PMM