* [Qemu-devel] [PATCH 01/12] Refactor translation block CPU state handling
2008-11-03 10:35 [Qemu-devel] [PATCH 00/12] Enhance debugging support - 4th take Jan Kiszka
@ 2008-11-03 10:35 ` Jan Kiszka
2008-11-13 20:42 ` Anthony Liguori
2008-11-03 10:36 ` [Qemu-devel] [PATCH 02/12] Return appropriate watch message to gdb Jan Kiszka
` (12 subsequent siblings)
13 siblings, 1 reply; 28+ messages in thread
From: Jan Kiszka @ 2008-11-03 10:35 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
This patch refactors the way the CPU state is handled that is associated
with a TB. The basic motivation is to move more arch specific code out
of generic files. Specifically the long #ifdef clutter in tb_find_fast()
has to be overcome in order to avoid duplicating it for the gdb
watchpoint fixes (patch "Restore pc on watchpoint hits").
The approach taken here is to encapsulate the relevant CPU state in a
new structure called TBCPUState which is kept inside TranslationBlock
but also passed around when setting up new TBs. To fill a TBCPUState
based on the current CPUState, each arch has to provide
cpu_get_tb_cpu_state(CPUState *, TBCPUState *).
At this chance, the patch also converts the not really beautiful macro
CPU_PC_FROM_TB into a clean static inline function.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
cpu-exec.c | 96 ++++++++---------------------------------------
exec-all.h | 11 +++--
exec.c | 89 ++++++++++++++++---------------------------
target-alpha/cpu.h | 14 +++++-
target-alpha/translate.c | 2
target-arm/cpu.h | 19 ++++++++-
target-arm/translate.c | 4 -
target-cris/cpu.h | 16 ++++++-
target-cris/translate.c | 22 +++++-----
target-i386/cpu.h | 16 ++++++-
target-i386/translate.c | 22 +++++-----
target-m68k/cpu.h | 16 ++++++-
target-m68k/translate.c | 5 +-
target-mips/cpu.h | 18 ++++++--
target-mips/translate.c | 6 +-
target-ppc/cpu.h | 14 +++++-
target-ppc/translate.c | 4 -
target-sh4/cpu.h | 21 +++++++---
target-sh4/translate.c | 6 +-
target-sparc/cpu.h | 26 ++++++++++--
target-sparc/translate.c | 8 +--
21 files changed, 232 insertions(+), 203 deletions(-)
Index: b/cpu-exec.c
===================================================================
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -39,6 +39,8 @@
#endif
#endif
+#include <string.h>
+
#if defined(__sparc__) && !defined(HOST_SOLARIS)
// Work around ugly bugs in glibc that mangle global register contents
#undef env
@@ -100,8 +102,7 @@ static void cpu_exec_nocache(int max_cyc
if (max_cycles > CF_COUNT_MASK)
max_cycles = CF_COUNT_MASK;
- tb = tb_gen_code(env, orig_tb->pc, orig_tb->cs_base, orig_tb->flags,
- max_cycles);
+ tb = tb_gen_code(env, &orig_tb->cpu_state, max_cycles);
env->current_tb = tb;
/* execute the generated code */
next_tb = tcg_qemu_tb_exec(tb->tc_ptr);
@@ -109,20 +110,19 @@ static void cpu_exec_nocache(int max_cyc
if ((next_tb & 3) == 2) {
/* Restore PC. This may happen if async event occurs before
the TB starts executing. */
- CPU_PC_FROM_TB(env, tb);
+ cpu_pc_from_tb(env, tb);
}
tb_phys_invalidate(tb, -1);
tb_free(tb);
}
-static TranslationBlock *tb_find_slow(target_ulong pc,
- target_ulong cs_base,
- uint64_t flags)
+static TranslationBlock *tb_find_slow(TBCPUState *cpu_state)
{
TranslationBlock *tb, **ptb1;
unsigned int h;
- target_ulong phys_pc, phys_page1, phys_page2, virt_page2;
+ target_ulong pc, phys_pc, phys_page1, phys_page2, virt_page2;
+ pc = cpu_state->pc;
tb_invalidated_flag = 0;
regs_to_env(); /* XXX: do it just before cpu_gen_code() */
@@ -137,10 +137,10 @@ static TranslationBlock *tb_find_slow(ta
tb = *ptb1;
if (!tb)
goto not_found;
- if (tb->pc == pc &&
+ if (tb->cpu_state.pc == pc &&
tb->page_addr[0] == phys_page1 &&
- tb->cs_base == cs_base &&
- tb->flags == flags) {
+ tb->cpu_state.cs_base == cpu_state->cs_base &&
+ tb->cpu_state.flags == cpu_state->flags) {
/* check next page if needed */
if (tb->page_addr[1] != -1) {
virt_page2 = (pc & TARGET_PAGE_MASK) +
@@ -156,7 +156,7 @@ static TranslationBlock *tb_find_slow(ta
}
not_found:
/* if no translated code available, then translate it now */
- tb = tb_gen_code(env, pc, cs_base, flags, 0);
+ tb = tb_gen_code(env, cpu_state, 0);
found:
/* we add the TB in the virtual pc hash table */
@@ -167,76 +167,16 @@ static TranslationBlock *tb_find_slow(ta
static inline TranslationBlock *tb_find_fast(void)
{
TranslationBlock *tb;
- target_ulong cs_base, pc;
- uint64_t flags;
+ TBCPUState cpu_state;
/* we record a subset of the CPU state. It will
always be the same before a given translated block
is executed. */
-#if defined(TARGET_I386)
- flags = env->hflags;
- flags |= (env->eflags & (IOPL_MASK | TF_MASK | VM_MASK));
- cs_base = env->segs[R_CS].base;
- pc = cs_base + env->eip;
-#elif defined(TARGET_ARM)
- flags = env->thumb | (env->vfp.vec_len << 1)
- | (env->vfp.vec_stride << 4);
- if ((env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR)
- flags |= (1 << 6);
- if (env->vfp.xregs[ARM_VFP_FPEXC] & (1 << 30))
- flags |= (1 << 7);
- flags |= (env->condexec_bits << 8);
- cs_base = 0;
- pc = env->regs[15];
-#elif defined(TARGET_SPARC)
-#ifdef TARGET_SPARC64
- // AM . Combined FPU enable bits . PRIV . DMMU enabled . IMMU enabled
- flags = ((env->pstate & PS_AM) << 2)
- | (((env->pstate & PS_PEF) >> 1) | ((env->fprs & FPRS_FEF) << 2))
- | (env->pstate & PS_PRIV) | ((env->lsu & (DMMU_E | IMMU_E)) >> 2);
-#else
- // FPU enable . Supervisor
- flags = (env->psref << 4) | env->psrs;
-#endif
- cs_base = env->npc;
- pc = env->pc;
-#elif defined(TARGET_PPC)
- flags = env->hflags;
- cs_base = 0;
- pc = env->nip;
-#elif defined(TARGET_MIPS)
- flags = env->hflags & (MIPS_HFLAG_TMASK | MIPS_HFLAG_BMASK);
- cs_base = 0;
- pc = env->active_tc.PC;
-#elif defined(TARGET_M68K)
- flags = (env->fpcr & M68K_FPCR_PREC) /* Bit 6 */
- | (env->sr & SR_S) /* Bit 13 */
- | ((env->macsr >> 4) & 0xf); /* Bits 0-3 */
- cs_base = 0;
- pc = env->pc;
-#elif defined(TARGET_SH4)
- flags = (env->flags & (DELAY_SLOT | DELAY_SLOT_CONDITIONAL
- | DELAY_SLOT_TRUE | DELAY_SLOT_CLEARME)) /* Bits 0- 3 */
- | (env->fpscr & (FPSCR_FR | FPSCR_SZ | FPSCR_PR)) /* Bits 19-21 */
- | (env->sr & (SR_MD | SR_RB)); /* Bits 29-30 */
- cs_base = 0;
- pc = env->pc;
-#elif defined(TARGET_ALPHA)
- flags = env->ps;
- cs_base = 0;
- pc = env->pc;
-#elif defined(TARGET_CRIS)
- flags = env->pregs[PR_CCS] & (S_FLAG | P_FLAG | U_FLAG | X_FLAG);
- flags |= env->dslot;
- cs_base = 0;
- pc = env->pc;
-#else
-#error unsupported CPU
-#endif
- tb = env->tb_jmp_cache[tb_jmp_cache_hash_func(pc)];
- if (unlikely(!tb || tb->pc != pc || tb->cs_base != cs_base ||
- tb->flags != flags)) {
- tb = tb_find_slow(pc, cs_base, flags);
+ cpu_get_tb_cpu_state(env, &cpu_state);
+ tb = env->tb_jmp_cache[tb_jmp_cache_hash_func(cpu_state.pc)];
+ if (unlikely(!tb ||
+ memcmp(&tb->cpu_state, &cpu_state, sizeof(cpu_state)))) {
+ tb = tb_find_slow(&cpu_state);
}
return tb;
}
@@ -634,7 +574,7 @@ int cpu_exec(CPUState *env1)
int insns_left;
tb = (TranslationBlock *)(long)(next_tb & ~3);
/* Restore PC. */
- CPU_PC_FROM_TB(env, tb);
+ cpu_pc_from_tb(env, tb);
insns_left = env->icount_decr.u32;
if (env->icount_extra && insns_left >= 0) {
/* Refill decrementer and continue execution. */
Index: b/exec-all.h
===================================================================
--- a/exec-all.h
+++ b/exec-all.h
@@ -29,6 +29,7 @@
#define DISAS_UPDATE 2 /* cpu state was modified dynamically */
#define DISAS_TB_JUMP 3 /* only pc was modified statically */
+typedef struct TBCPUState TBCPUState;
typedef struct TranslationBlock TranslationBlock;
/* XXX: make safe guess about sizes */
@@ -78,9 +79,7 @@ int cpu_restore_state_copy(struct Transl
void *puc);
void cpu_resume_from_signal(CPUState *env1, void *puc);
void cpu_io_recompile(CPUState *env, void *retaddr);
-TranslationBlock *tb_gen_code(CPUState *env,
- target_ulong pc, target_ulong cs_base, int flags,
- int cflags);
+TranslationBlock *tb_gen_code(CPUState *env, TBCPUState *cpu_state, int cflags);
void cpu_exec_init(CPUState *env);
int page_unprotect(target_ulong address, unsigned long pc, void *puc);
void tb_invalidate_phys_page_range(target_phys_addr_t start, target_phys_addr_t end,
@@ -123,10 +122,14 @@ static inline int tlb_set_page(CPUState
#define USE_DIRECT_JUMP
#endif
-struct TranslationBlock {
+struct TBCPUState {
target_ulong pc; /* simulated PC corresponding to this block (EIP + CS base) */
target_ulong cs_base; /* CS base for this block */
uint64_t flags; /* flags defining in which context the code was generated */
+};
+
+struct TranslationBlock {
+ TBCPUState cpu_state;
uint16_t size; /* size of target code for this block (1 <=
size <= TARGET_PAGE_SIZE) */
uint16_t cflags; /* compile flags */
Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -737,7 +737,7 @@ void tb_phys_invalidate(TranslationBlock
TranslationBlock *tb1, *tb2;
/* remove the TB from the hash list */
- phys_pc = tb->page_addr[0] + (tb->pc & ~TARGET_PAGE_MASK);
+ phys_pc = tb->page_addr[0] + (tb->cpu_state.pc & ~TARGET_PAGE_MASK);
h = tb_phys_hash_func(phys_pc);
tb_remove(&tb_phys_hash[h], tb,
offsetof(TranslationBlock, phys_hash_next));
@@ -757,7 +757,7 @@ void tb_phys_invalidate(TranslationBlock
tb_invalidated_flag = 1;
/* remove the TB from the hash list */
- h = tb_jmp_cache_hash_func(tb->pc);
+ h = tb_jmp_cache_hash_func(tb->cpu_state.pc);
for(env = first_cpu; env != NULL; env = env->next_cpu) {
if (env->tb_jmp_cache[h] == tb)
env->tb_jmp_cache[h] = NULL;
@@ -828,28 +828,27 @@ static void build_page_bitmap(PageDesc *
if (n == 0) {
/* NOTE: tb_end may be after the end of the page, but
it is not a problem */
- tb_start = tb->pc & ~TARGET_PAGE_MASK;
+ tb_start = tb->cpu_state.pc & ~TARGET_PAGE_MASK;
tb_end = tb_start + tb->size;
if (tb_end > TARGET_PAGE_SIZE)
tb_end = TARGET_PAGE_SIZE;
} else {
tb_start = 0;
- tb_end = ((tb->pc + tb->size) & ~TARGET_PAGE_MASK);
+ tb_end = ((tb->cpu_state.pc + tb->size) & ~TARGET_PAGE_MASK);
}
set_bits(p->code_bitmap, tb_start, tb_end - tb_start);
tb = tb->page_next[n];
}
}
-TranslationBlock *tb_gen_code(CPUState *env,
- target_ulong pc, target_ulong cs_base,
- int flags, int cflags)
+TranslationBlock *tb_gen_code(CPUState *env, TBCPUState *cpu_state, int cflags)
{
TranslationBlock *tb;
uint8_t *tc_ptr;
- target_ulong phys_pc, phys_page2, virt_page2;
+ target_ulong pc, phys_pc, phys_page2, virt_page2;
int code_gen_size;
+ pc = cpu_state->pc;
phys_pc = get_phys_addr_code(env, pc);
tb = tb_alloc(pc);
if (!tb) {
@@ -862,8 +861,7 @@ TranslationBlock *tb_gen_code(CPUState *
}
tc_ptr = code_gen_ptr;
tb->tc_ptr = tc_ptr;
- tb->cs_base = cs_base;
- tb->flags = flags;
+ tb->cpu_state = *cpu_state;
tb->cflags = cflags;
cpu_gen_code(env, tb, &code_gen_size);
code_gen_ptr = (void *)(((unsigned long)code_gen_ptr + code_gen_size + CODE_GEN_ALIGN - 1) & ~(CODE_GEN_ALIGN - 1));
@@ -886,12 +884,17 @@ TranslationBlock *tb_gen_code(CPUState *
void tb_invalidate_phys_page_range(target_phys_addr_t start, target_phys_addr_t end,
int is_cpu_write_access)
{
- int n, current_tb_modified, current_tb_not_found, current_flags;
+ TranslationBlock *tb, *tb_next, *saved_tb;
CPUState *env = cpu_single_env;
- PageDesc *p;
- TranslationBlock *tb, *tb_next, *current_tb, *saved_tb;
target_ulong tb_start, tb_end;
- target_ulong current_pc, current_cs_base;
+ PageDesc *p;
+ int n;
+#ifdef TARGET_HAS_PRECISE_SMC
+ int current_tb_not_found = is_cpu_write_access;
+ TranslationBlock *current_tb = NULL;
+ int current_tb_modified = 0;
+ TBCPUState cpu_state;
+#endif /* TARGET_HAS_PRECISE_SMC */
p = page_find(start >> TARGET_PAGE_BITS);
if (!p)
@@ -905,12 +908,6 @@ void tb_invalidate_phys_page_range(targe
/* we remove all the TBs in the range [start, end[ */
/* XXX: see if in some cases it could be faster to invalidate all the code */
- current_tb_not_found = is_cpu_write_access;
- current_tb_modified = 0;
- current_tb = NULL; /* avoid warning */
- current_pc = 0; /* avoid warning */
- current_cs_base = 0; /* avoid warning */
- current_flags = 0; /* avoid warning */
tb = p->first_tb;
while (tb != NULL) {
n = (long)tb & 3;
@@ -920,11 +917,13 @@ void tb_invalidate_phys_page_range(targe
if (n == 0) {
/* NOTE: tb_end may be after the end of the page, but
it is not a problem */
- tb_start = tb->page_addr[0] + (tb->pc & ~TARGET_PAGE_MASK);
+ tb_start = tb->page_addr[0] +
+ (tb->cpu_state.pc & ~TARGET_PAGE_MASK);
tb_end = tb_start + tb->size;
} else {
tb_start = tb->page_addr[1];
- tb_end = tb_start + ((tb->pc + tb->size) & ~TARGET_PAGE_MASK);
+ tb_end = tb_start +
+ ((tb->cpu_state.pc + tb->size) & ~TARGET_PAGE_MASK);
}
if (!(tb_end <= start || tb_start >= end)) {
#ifdef TARGET_HAS_PRECISE_SMC
@@ -947,14 +946,7 @@ void tb_invalidate_phys_page_range(targe
current_tb_modified = 1;
cpu_restore_state(current_tb, env,
env->mem_io_pc, NULL);
-#if defined(TARGET_I386)
- current_flags = env->hflags;
- current_flags |= (env->eflags & (IOPL_MASK | TF_MASK | VM_MASK));
- current_cs_base = (target_ulong)env->segs[R_CS].base;
- current_pc = current_cs_base + env->eip;
-#else
-#error unsupported CPU
-#endif
+ cpu_get_tb_cpu_state(env, &cpu_state);
}
#endif /* TARGET_HAS_PRECISE_SMC */
/* we need to do that to handle the case where a signal
@@ -988,7 +980,7 @@ void tb_invalidate_phys_page_range(targe
modifying the memory. It will ensure that it cannot modify
itself */
env->current_tb = NULL;
- tb_gen_code(env, current_pc, current_cs_base, current_flags, 1);
+ tb_gen_code(env, &cpu_state, 1);
cpu_resume_from_signal(env, NULL);
}
#endif
@@ -1027,12 +1019,14 @@ static inline void tb_invalidate_phys_pa
static void tb_invalidate_phys_page(target_phys_addr_t addr,
unsigned long pc, void *puc)
{
- int n, current_flags, current_tb_modified;
- target_ulong current_pc, current_cs_base;
+ TranslationBlock *tb;
PageDesc *p;
- TranslationBlock *tb, *current_tb;
+ int n;
#ifdef TARGET_HAS_PRECISE_SMC
+ TranslationBlock *current_tb = NULL;
CPUState *env = cpu_single_env;
+ int current_tb_modified = 0;
+ TBCPUState cpu_state;
#endif
addr &= TARGET_PAGE_MASK;
@@ -1040,11 +1034,6 @@ static void tb_invalidate_phys_page(targ
if (!p)
return;
tb = p->first_tb;
- current_tb_modified = 0;
- current_tb = NULL;
- current_pc = 0; /* avoid warning */
- current_cs_base = 0; /* avoid warning */
- current_flags = 0; /* avoid warning */
#ifdef TARGET_HAS_PRECISE_SMC
if (tb && pc != 0) {
current_tb = tb_find_pc(pc);
@@ -1064,14 +1053,7 @@ static void tb_invalidate_phys_page(targ
current_tb_modified = 1;
cpu_restore_state(current_tb, env, pc, puc);
-#if defined(TARGET_I386)
- current_flags = env->hflags;
- current_flags |= (env->eflags & (IOPL_MASK | TF_MASK | VM_MASK));
- current_cs_base = (target_ulong)env->segs[R_CS].base;
- current_pc = current_cs_base + env->eip;
-#else
-#error unsupported CPU
-#endif
+ cpu_get_tb_cpu_state(env, &cpu_state);
}
#endif /* TARGET_HAS_PRECISE_SMC */
tb_phys_invalidate(tb, addr);
@@ -1084,7 +1066,7 @@ static void tb_invalidate_phys_page(targ
modifying the memory. It will ensure that it cannot modify
itself */
env->current_tb = NULL;
- tb_gen_code(env, current_pc, current_cs_base, current_flags, 1);
+ tb_gen_code(env, &cpu_state, 1);
cpu_resume_from_signal(env, puc);
}
#endif
@@ -1156,7 +1138,7 @@ TranslationBlock *tb_alloc(target_ulong
(code_gen_ptr - code_gen_buffer) >= code_gen_buffer_max_size)
return NULL;
tb = &tbs[nb_tbs++];
- tb->pc = pc;
+ tb->cpu_state.pc = pc;
tb->cflags = 0;
return tb;
}
@@ -3150,9 +3132,8 @@ int cpu_memory_rw_debug(CPUState *env, t
void cpu_io_recompile(CPUState *env, void *retaddr)
{
TranslationBlock *tb;
+ TBCPUState cpu_state;
uint32_t n, cflags;
- target_ulong pc, cs_base;
- uint64_t flags;
tb = tb_find_pc((unsigned long)retaddr);
if (!tb) {
@@ -3189,13 +3170,11 @@ void cpu_io_recompile(CPUState *env, voi
cpu_abort(env, "TB too big during recompile");
cflags = n | CF_LAST_IO;
- pc = tb->pc;
- cs_base = tb->cs_base;
- flags = tb->flags;
+ cpu_state = tb->cpu_state;
tb_phys_invalidate(tb, -1);
/* FIXME: In theory this could raise an exception. In practice
we have already translated the block once so it's probably ok. */
- tb_gen_code(env, pc, cs_base, flags, cflags);
+ tb_gen_code(env, &cpu_state, cflags);
/* TODO: If env->pc != tb->pc (i.e. the faulting instruction was not
the first in the TB) then we end up generating a whole new TB and
repeating the fault, which is horribly inefficient.
Index: b/target-alpha/cpu.h
===================================================================
--- a/target-alpha/cpu.h
+++ b/target-alpha/cpu.h
@@ -318,6 +318,7 @@ static inline void cpu_clone_regs(CPUSta
#endif
#include "cpu-all.h"
+#include "exec-all.h"
enum {
FEATURE_ASN = 0x00000001,
@@ -416,6 +417,15 @@ void call_pal (CPUState *env);
void call_pal (CPUState *env, int palcode);
#endif
-#define CPU_PC_FROM_TB(env, tb) env->pc = tb->pc
-
+static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
+{
+ env->pc = tb->cpu_state.pc;
+}
+
+static inline void cpu_get_tb_cpu_state(CPUState *env, TBCPUState *cpu_state)
+{
+ cpu_state->pc = env->pc;
+ cpu_state->cs_base = 0;
+ cpu_state->flags = env->ps;
+}
#endif /* !defined (__CPU_ALPHA_H__) */
Index: b/target-alpha/translate.c
===================================================================
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -2245,7 +2245,7 @@ static always_inline void gen_intermedia
int num_insns;
int max_insns;
- pc_start = tb->pc;
+ pc_start = tb->cpu_state.pc;
gen_opc_end = gen_opc_buf + OPC_MAX_SIZE;
ctx.pc = pc_start;
ctx.amask = env->amask;
Index: b/target-arm/cpu.h
===================================================================
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -415,8 +415,23 @@ static inline void cpu_clone_regs(CPUSta
}
#endif
-#define CPU_PC_FROM_TB(env, tb) env->regs[15] = tb->pc
-
#include "cpu-all.h"
+#include "exec-all.h"
+
+static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
+{
+ env->regs[15] = tb->cpu_state.pc;
+}
+static inline void cpu_get_tb_cpu_state(CPUState *env, TBCPUState *cpu_state)
+{
+ cpu_state->pc = env->regs[15];
+ cpu_state->cs_base = 0;
+ cpu_state->flags = env->thumb | (env->vfp.vec_len << 1)
+ | (env->vfp.vec_stride << 4) | (env->condexec_bits << 8);
+ if ((env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR)
+ cpu_state->flags |= (1 << 6);
+ if (env->vfp.xregs[ARM_VFP_FPEXC] & (1 << 30))
+ cpu_state->flags |= (1 << 7);
+}
#endif
Index: b/target-arm/translate.c
===================================================================
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -3375,7 +3375,7 @@ static inline void gen_goto_tb(DisasCont
TranslationBlock *tb;
tb = s->tb;
- if ((tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK)) {
+ if ((tb->cpu_state.pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK)) {
tcg_gen_goto_tb(n);
gen_set_pc_im(dest);
tcg_gen_exit_tb((long)tb + n);
@@ -8597,7 +8597,7 @@ static inline void gen_intermediate_code
num_temps = 0;
memset(temps, 0, sizeof(temps));
- pc_start = tb->pc;
+ pc_start = tb->cpu_state.pc;
dc->tb = tb;
Index: b/target-cris/cpu.h
===================================================================
--- a/target-cris/cpu.h
+++ b/target-cris/cpu.h
@@ -237,7 +237,19 @@ static inline void cpu_clone_regs(CPUSta
#define SFR_RW_MM_TLB_LO env->pregs[PR_SRS]][5
#define SFR_RW_MM_TLB_HI env->pregs[PR_SRS]][6
-#define CPU_PC_FROM_TB(env, tb) env->pc = tb->pc
-
#include "cpu-all.h"
+#include "exec-all.h"
+
+static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
+{
+ env->pc = tb->cpu_state.pc;
+}
+
+static inline void cpu_get_tb_cpu_state(CPUState *env, TBCPUState *cpu_state)
+{
+ cpu_state->pc = env->pc;
+ cpu_state->cs_base = 0;
+ cpu_state->flags = env->dslot |
+ (env->pregs[PR_CCS] & (S_FLAG | P_FLAG | U_FLAG | X_FLAG));
+}
#endif
Index: b/target-cris/translate.c
===================================================================
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -642,7 +642,8 @@ static void gen_goto_tb(DisasContext *dc
{
TranslationBlock *tb;
tb = dc->tb;
- if ((tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK)) {
+ if ((tb->cpu_state.pc & TARGET_PAGE_MASK) ==
+ (dest & TARGET_PAGE_MASK)) {
tcg_gen_goto_tb(n);
tcg_gen_movi_tl(env_pc, dest);
tcg_gen_exit_tb((long)tb + n);
@@ -3241,7 +3242,7 @@ gen_intermediate_code_internal(CPUState
/* Odd PC indicates that branch is rexecuting due to exception in the
* delayslot, like in real hw.
*/
- pc_start = tb->pc & ~1;
+ pc_start = tb->cpu_state.pc & ~1;
dc->env = env;
dc->tb = tb;
@@ -3253,7 +3254,7 @@ gen_intermediate_code_internal(CPUState
dc->singlestep_enabled = env->singlestep_enabled;
dc->flags_uptodate = 1;
dc->flagx_known = 1;
- dc->flags_x = tb->flags & X_FLAG;
+ dc->flags_x = tb->cpu_state.flags & X_FLAG;
dc->cc_x_uptodate = 0;
dc->cc_mask = 0;
dc->update_cc = 0;
@@ -3262,8 +3263,9 @@ gen_intermediate_code_internal(CPUState
dc->cc_size_uptodate = -1;
/* Decode TB flags. */
- dc->tb_flags = tb->flags & (S_FLAG | P_FLAG | U_FLAG | X_FLAG);
- dc->delayed_branch = !!(tb->flags & 7);
+ dc->tb_flags =
+ tb->cpu_state.flags & (S_FLAG | P_FLAG | U_FLAG | X_FLAG);
+ dc->delayed_branch = !!(tb->cpu_state.flags & 7);
if (dc->delayed_branch)
dc->jmp = JMP_INDIRECT;
else
@@ -3280,8 +3282,8 @@ gen_intermediate_code_internal(CPUState
"%x.%x.%x.%x\n"
"%x.%x.%x.%x\n",
search_pc, dc->pc, dc->ppc,
- (unsigned long long)tb->flags,
- env->btarget, (unsigned)tb->flags & 7,
+ (unsigned long long)tb->cpu_state.flags,
+ env->btarget, (unsigned)tb->cpu_state.flags & 7,
env->pregs[PR_CCS],
env->pregs[PR_PID], env->pregs[PR_USP],
env->regs[0], env->regs[1], env->regs[2], env->regs[3],
@@ -3342,7 +3344,7 @@ gen_intermediate_code_internal(CPUState
dc->delayed_branch--;
if (dc->delayed_branch == 0)
{
- if (tb->flags & 7)
+ if (tb->cpu_state.flags & 7)
t_gen_mov_env_TN(dslot,
tcg_const_tl(0));
if (dc->jmp == JMP_DIRECT) {
@@ -3358,7 +3360,7 @@ gen_intermediate_code_internal(CPUState
/* If we are rexecuting a branch due to exceptions on
delay slots dont break. */
- if (!(tb->pc & 1) && env->singlestep_enabled)
+ if (!(tb->cpu_state.pc & 1) && env->singlestep_enabled)
break;
} while (!dc->is_jmp && !dc->cpustate_changed
&& gen_opc_ptr < gen_opc_end
@@ -3374,7 +3376,7 @@ gen_intermediate_code_internal(CPUState
/* Force an update if the per-tb cpu state has changed. */
if (dc->is_jmp == DISAS_NEXT
&& (dc->cpustate_changed || !dc->flagx_known
- || (dc->flags_x != (tb->flags & X_FLAG)))) {
+ || (dc->flags_x != (tb->cpu_state.flags & X_FLAG)))) {
dc->is_jmp = DISAS_UPDATE;
tcg_gen_movi_tl(env_pc, npc);
}
Index: b/target-i386/cpu.h
===================================================================
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -774,10 +774,20 @@ static inline void cpu_clone_regs(CPUSta
}
#endif
-#define CPU_PC_FROM_TB(env, tb) env->eip = tb->pc - tb->cs_base
-
#include "cpu-all.h"
-
+#include "exec-all.h"
#include "svm.h"
+static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
+{
+ env->eip = tb->cpu_state.pc - tb->cpu_state.cs_base;
+}
+
+static inline void cpu_get_tb_cpu_state(CPUState *env, TBCPUState *cpu_state)
+{
+ cpu_state->cs_base = env->segs[R_CS].base;
+ cpu_state->pc = cpu_state->cs_base + env->eip;
+ cpu_state->flags =
+ env->hflags | (env->eflags & (IOPL_MASK | TF_MASK | VM_MASK));
+}
#endif /* CPU_I386_H */
Index: b/target-i386/translate.c
===================================================================
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -2210,7 +2210,7 @@ static inline void gen_goto_tb(DisasCont
pc = s->cs_base + eip;
tb = s->tb;
/* NOTE: we handle the case where the TB spans two pages here */
- if ((pc & TARGET_PAGE_MASK) == (tb->pc & TARGET_PAGE_MASK) ||
+ if ((pc & TARGET_PAGE_MASK) == (tb->cpu_state.pc & TARGET_PAGE_MASK) ||
(pc & TARGET_PAGE_MASK) == ((s->pc - 1) & TARGET_PAGE_MASK)) {
/* jump to same page: we can use a direct jump */
tcg_gen_goto_tb(tb_num);
@@ -2625,7 +2625,7 @@ static void gen_eob(DisasContext *s)
{
if (s->cc_op != CC_OP_DYNAMIC)
gen_op_set_cc_op(s->cc_op);
- if (s->tb->flags & HF_INHIBIT_IRQ_MASK) {
+ if (s->tb->cpu_state.flags & HF_INHIBIT_IRQ_MASK) {
tcg_gen_helper_0_0(helper_reset_inhibit_irq);
}
if (s->singlestep_enabled) {
@@ -4921,7 +4921,7 @@ static target_ulong disas_insn(DisasCont
/* if reg == SS, inhibit interrupts/trace. */
/* If several instructions disable interrupts, only the
_first_ does it */
- if (!(s->tb->flags & HF_INHIBIT_IRQ_MASK))
+ if (!(s->tb->cpu_state.flags & HF_INHIBIT_IRQ_MASK))
tcg_gen_helper_0_0(helper_set_inhibit_irq);
s->tf = 0;
}
@@ -4997,7 +4997,7 @@ static target_ulong disas_insn(DisasCont
/* if reg == SS, inhibit interrupts/trace */
/* If several instructions disable interrupts, only the
_first_ does it */
- if (!(s->tb->flags & HF_INHIBIT_IRQ_MASK))
+ if (!(s->tb->cpu_state.flags & HF_INHIBIT_IRQ_MASK))
tcg_gen_helper_0_0(helper_set_inhibit_irq);
s->tf = 0;
}
@@ -6597,7 +6597,7 @@ static target_ulong disas_insn(DisasCont
/* interruptions are enabled only the first insn after sti */
/* If several instructions disable interrupts, only the
_first_ does it */
- if (!(s->tb->flags & HF_INHIBIT_IRQ_MASK))
+ if (!(s->tb->cpu_state.flags & HF_INHIBIT_IRQ_MASK))
tcg_gen_helper_0_0(helper_set_inhibit_irq);
/* give a chance to handle pending irqs */
gen_jmp_im(s->pc - s->cs_base);
@@ -7541,9 +7541,9 @@ static inline void gen_intermediate_code
int max_insns;
/* generate intermediate code */
- pc_start = tb->pc;
- cs_base = tb->cs_base;
- flags = tb->flags;
+ pc_start = tb->cpu_state.pc;
+ cs_base = tb->cpu_state.cs_base;
+ flags = tb->cpu_state.flags;
cflags = tb->cflags;
dc->pe = (flags >> HF_PE_SHIFT) & 1;
@@ -7725,11 +7725,11 @@ void gen_pc_load(CPUState *env, Translat
}
}
fprintf(logfile, "spc=0x%08lx pc_pos=0x%x eip=" TARGET_FMT_lx " cs_base=%x\n",
- searched_pc, pc_pos, gen_opc_pc[pc_pos] - tb->cs_base,
- (uint32_t)tb->cs_base);
+ searched_pc, pc_pos, gen_opc_pc[pc_pos] - tb->cpu_state.cs_base,
+ (uint32_t)tb->cpu_state.cs_base);
}
#endif
- env->eip = gen_opc_pc[pc_pos] - tb->cs_base;
+ env->eip = gen_opc_pc[pc_pos] - tb->cpu_state.cs_base;
cc_op = gen_opc_cc_op[pc_pos];
if (cc_op != CC_OP_DYNAMIC)
env->cc_op = cc_op;
Index: b/target-m68k/cpu.h
===================================================================
--- a/target-m68k/cpu.h
+++ b/target-m68k/cpu.h
@@ -231,8 +231,20 @@ static inline void cpu_clone_regs(CPUSta
}
#endif
-#define CPU_PC_FROM_TB(env, tb) env->pc = tb->pc
-
#include "cpu-all.h"
+#include "exec-all.h"
+
+static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
+{
+ env->pc = tb->cpu_state.pc;
+}
+static inline void cpu_get_tb_cpu_state(CPUState *env, TBCPUState *cpu_state)
+{
+ cpu_state->pc = env->pc;
+ cpu_state->cs_base = 0;
+ cpu_state->flags = (env->fpcr & M68K_FPCR_PREC) /* Bit 6 */
+ | (env->sr & SR_S) /* Bit 13 */
+ | ((env->macsr >> 4) & 0xf); /* Bits 0-3 */
+}
#endif
Index: b/target-m68k/translate.c
===================================================================
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -874,7 +874,8 @@ static void gen_jmp_tb(DisasContext *s,
tb = s->tb;
if (unlikely(s->singlestep_enabled)) {
gen_exception(s, dest, EXCP_DEBUG);
- } else if ((tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK) ||
+ } else if ((tb->cpu_state.pc & TARGET_PAGE_MASK) ==
+ (dest & TARGET_PAGE_MASK) ||
(s->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK)) {
tcg_gen_goto_tb(n);
tcg_gen_movi_i32(QREG_PC, dest);
@@ -2924,7 +2925,7 @@ gen_intermediate_code_internal(CPUState
int max_insns;
/* generate intermediate code */
- pc_start = tb->pc;
+ pc_start = tb->cpu_state.pc;
dc->tb = tb;
Index: b/target-mips/cpu.h
===================================================================
--- a/target-mips/cpu.h
+++ b/target-mips/cpu.h
@@ -501,6 +501,7 @@ static inline void cpu_clone_regs(CPUSta
}
#include "cpu-all.h"
+#include "exec-all.h"
/* Memory access type :
* may be needed for precise access rights control and precise exceptions.
@@ -562,10 +563,17 @@ CPUMIPSState *cpu_mips_init(const char *
uint32_t cpu_mips_get_clock (void);
int cpu_mips_signal_handler(int host_signum, void *pinfo, void *puc);
-#define CPU_PC_FROM_TB(env, tb) do { \
- env->active_tc.PC = tb->pc; \
- env->hflags &= ~MIPS_HFLAG_BMASK; \
- env->hflags |= tb->flags & MIPS_HFLAG_BMASK; \
- } while (0)
+static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
+{
+ env->active_tc.PC = tb->cpu_state.pc;
+ env->hflags &= ~MIPS_HFLAG_BMASK;
+ env->hflags |= tb->cpu_state.flags & MIPS_HFLAG_BMASK;
+}
+static inline void cpu_get_tb_cpu_state(CPUState *env, TBCPUState *cpu_state)
+{
+ cpu_state->pc = env->active_tc.PC;
+ cpu_state->cs_base = 0;
+ cpu_state->flags = env->hflags & (MIPS_HFLAG_TMASK | MIPS_HFLAG_BMASK);
+}
#endif /* !defined (__MIPS_CPU_H__) */
Index: b/target-mips/translate.c
===================================================================
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -2475,7 +2475,7 @@ static inline void gen_goto_tb(DisasCont
{
TranslationBlock *tb;
tb = ctx->tb;
- if ((tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK)) {
+ if ((tb->cpu_state.pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK)) {
tcg_gen_goto_tb(n);
gen_save_pc(dest);
tcg_gen_exit_tb((long)tb + n);
@@ -8449,7 +8449,7 @@ gen_intermediate_code_internal (CPUState
if (search_pc && loglevel)
fprintf (logfile, "search pc %d\n", search_pc);
- pc_start = tb->pc;
+ pc_start = tb->cpu_state.pc;
/* Leave some spare opc slots for branch handling. */
gen_opc_end = gen_opc_buf + OPC_MAX_SIZE - 16;
ctx.pc = pc_start;
@@ -8457,7 +8457,7 @@ gen_intermediate_code_internal (CPUState
ctx.tb = tb;
ctx.bstate = BS_NONE;
/* Restore delay slot state from the tb context. */
- ctx.hflags = (uint32_t)tb->flags; /* FIXME: maybe use 64 bits here? */
+ ctx.hflags = (uint32_t)tb->cpu_state.flags; /* FIXME: maybe use 64 bits here? */
restore_cpu_state(env, &ctx);
if (env->user_mode_only)
ctx.mem_idx = MIPS_HFLAG_UM;
Index: b/target-ppc/cpu.h
===================================================================
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -822,9 +822,8 @@ static inline void cpu_clone_regs(CPUSta
}
#endif
-#define CPU_PC_FROM_TB(env, tb) env->nip = tb->pc
-
#include "cpu-all.h"
+#include "exec-all.h"
/*****************************************************************************/
/* CRF definitions */
@@ -1424,4 +1423,15 @@ enum {
/*****************************************************************************/
+static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
+{
+ env->nip = tb->cpu_state.pc;
+}
+
+static inline void cpu_get_tb_cpu_state(CPUState *env, TBCPUState *cpu_state)
+{
+ cpu_state->pc = env->nip;
+ cpu_state->cs_base = 0;
+ cpu_state->flags = env->hflags;
+}
#endif /* !defined (__CPU_PPC_H__) */
Index: b/target-ppc/translate.c
===================================================================
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -3470,7 +3470,7 @@ static always_inline void gen_goto_tb (D
if (!ctx->sf_mode)
dest = (uint32_t) dest;
#endif
- if ((tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK) &&
+ if ((tb->cpu_state.pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK) &&
likely(!ctx->singlestep_enabled)) {
tcg_gen_goto_tb(n);
tcg_gen_movi_tl(cpu_nip, dest & ~3);
@@ -6783,7 +6783,7 @@ static always_inline void gen_intermedia
int num_insns;
int max_insns;
- pc_start = tb->pc;
+ pc_start = tb->cpu_state.pc;
gen_opc_end = gen_opc_buf + OPC_MAX_SIZE;
#if defined(OPTIMIZE_FPRF_UPDATE)
gen_fprf_ptr = gen_fprf_buf;
Index: b/target-sh4/cpu.h
===================================================================
--- a/target-sh4/cpu.h
+++ b/target-sh4/cpu.h
@@ -173,12 +173,8 @@ static inline void cpu_clone_regs(CPUSta
}
#endif
-#define CPU_PC_FROM_TB(env, tb) do { \
- env->pc = tb->pc; \
- env->flags = tb->flags; \
- } while (0)
-
#include "cpu-all.h"
+#include "exec-all.h"
/* Memory access type */
enum {
@@ -269,4 +265,19 @@ static inline int cpu_ptel_pr (uint32_t
#define PTEA_TC (1 << 3)
#define cpu_ptea_tc(ptea) (((ptea) & PTEA_TC) >> 3)
+static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
+{
+ env->pc = tb->cpu_state.pc;
+ env->flags = tb->cpu_state.flags;
+}
+
+static inline void cpu_get_tb_cpu_state(CPUState *env, TBCPUState *cpu_state)
+{
+ cpu_state->pc = env->pc;
+ cpu_state->cs_base = 0;
+ cpu_state->flags = (env->flags & (DELAY_SLOT | DELAY_SLOT_CONDITIONAL
+ | DELAY_SLOT_TRUE | DELAY_SLOT_CLEARME)) /* Bits 0- 3 */
+ | (env->fpscr & (FPSCR_FR | FPSCR_SZ | FPSCR_PR)) /* Bits 19-21 */
+ | (env->sr & (SR_MD | SR_RB)); /* Bits 29-30 */
+}
#endif /* _CPU_SH4_H */
Index: b/target-sh4/translate.c
===================================================================
--- a/target-sh4/translate.c
+++ b/target-sh4/translate.c
@@ -261,7 +261,7 @@ static void gen_goto_tb(DisasContext * c
TranslationBlock *tb;
tb = ctx->tb;
- if ((tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK) &&
+ if ((tb->cpu_state.pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK) &&
!ctx->singlestep_enabled) {
/* Use a direct jump if in same page and singlestep not enabled */
tcg_gen_goto_tb(n);
@@ -1803,10 +1803,10 @@ gen_intermediate_code_internal(CPUState
int num_insns;
int max_insns;
- pc_start = tb->pc;
+ pc_start = tb->cpu_state.pc;
gen_opc_end = gen_opc_buf + OPC_MAX_SIZE;
ctx.pc = pc_start;
- ctx.flags = (uint32_t)tb->flags;
+ ctx.flags = (uint32_t)tb->cpu_state.flags;
ctx.bstate = BS_NONE;
ctx.sr = env->sr;
ctx.fpscr = env->fpscr;
Index: b/target-sparc/cpu.h
===================================================================
--- a/target-sparc/cpu.h
+++ b/target-sparc/cpu.h
@@ -491,12 +491,8 @@ static inline void cpu_clone_regs(CPUSta
}
#endif
-#define CPU_PC_FROM_TB(env, tb) do { \
- env->pc = tb->pc; \
- env->npc = tb->cs_base; \
- } while(0)
-
#include "cpu-all.h"
+#include "exec-all.h"
/* sum4m.c, sun4u.c */
void cpu_check_irqs(CPUSPARCState *env);
@@ -508,4 +504,24 @@ uint64_t cpu_tick_get_count(void *opaque
void cpu_tick_set_limit(void *opaque, uint64_t limit);
#endif
+static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
+{
+ env->pc = tb->cpu_state.pc;
+ env->npc = tb->cpu_state.cs_base;
+}
+
+static inline void cpu_get_tb_cpu_state(CPUState *env, TBCPUState *cpu_state)
+{
+ cpu_state->pc = env->pc;
+ cpu_state->cs_base = env->npc;
+#ifdef TARGET_SPARC64
+ // AM . Combined FPU enable bits . PRIV . DMMU enabled . IMMU enabled
+ cpu_state->flags = ((env->pstate & PS_AM) << 2)
+ | (((env->pstate & PS_PEF) >> 1) | ((env->fprs & FPRS_FEF) << 2))
+ | (env->pstate & PS_PRIV) | ((env->lsu & (DMMU_E | IMMU_E)) >> 2);
+#else
+ // FPU enable . Supervisor
+ cpu_state->flags = (env->psref << 4) | env->psrs;
+#endif
+}
#endif
Index: b/target-sparc/translate.c
===================================================================
--- a/target-sparc/translate.c
+++ b/target-sparc/translate.c
@@ -223,8 +223,8 @@ static inline void gen_goto_tb(DisasCont
TranslationBlock *tb;
tb = s->tb;
- if ((pc & TARGET_PAGE_MASK) == (tb->pc & TARGET_PAGE_MASK) &&
- (npc & TARGET_PAGE_MASK) == (tb->pc & TARGET_PAGE_MASK)) {
+ if ((pc & TARGET_PAGE_MASK) == (tb->cpu_state.pc & TARGET_PAGE_MASK) &&
+ (npc & TARGET_PAGE_MASK) == (tb->cpu_state.pc & TARGET_PAGE_MASK)) {
/* jump to same page: we can use a direct jump */
tcg_gen_goto_tb(tb_num);
tcg_gen_movi_tl(cpu_pc, pc);
@@ -4783,10 +4783,10 @@ static inline void gen_intermediate_code
memset(dc, 0, sizeof(DisasContext));
dc->tb = tb;
- pc_start = tb->pc;
+ pc_start = tb->cpu_state.pc;
dc->pc = pc_start;
last_pc = dc->pc;
- dc->npc = (target_ulong) tb->cs_base;
+ dc->npc = (target_ulong) tb->cpu_state.cs_base;
dc->mem_idx = cpu_mmu_index(env);
dc->def = env->def;
if ((dc->def->features & CPU_FEATURE_FLOAT))
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH 01/12] Refactor translation block CPU state handling
2008-11-03 10:35 ` [Qemu-devel] [PATCH 01/12] Refactor translation block CPU state handling Jan Kiszka
@ 2008-11-13 20:42 ` Anthony Liguori
0 siblings, 0 replies; 28+ messages in thread
From: Anthony Liguori @ 2008-11-13 20:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
Jan Kiszka wrote:
> This patch refactors the way the CPU state is handled that is associated
> with a TB. The basic motivation is to move more arch specific code out
> of generic files. Specifically the long #ifdef clutter in tb_find_fast()
> has to be overcome in order to avoid duplicating it for the gdb
> watchpoint fixes (patch "Restore pc on watchpoint hits").
>
> The approach taken here is to encapsulate the relevant CPU state in a
> new structure called TBCPUState which is kept inside TranslationBlock
> but also passed around when setting up new TBs. To fill a TBCPUState
> based on the current CPUState, each arch has to provide
> cpu_get_tb_cpu_state(CPUState *, TBCPUState *).
>
> At this chance, the patch also converts the not really beautiful macro
> CPU_PC_FROM_TB into a clean static inline function.
>
There are really three patches in one here. The first patch converts
CPU_PC_FROM_TB to a function. The second eliminates the #ifdef mess in
tb_find_fast by introducing cpu_get_tb_cpu_state(CPUState *,
TranslationBlock *), and the third moves the CPU specific state in
Translation block to a separate structure and changes the signature of
cpu_get_tb_cpu_state() to take the TBCPUState structure.
Can you split up this patch along these lines? All of these changes are
mechanical and they are easier to review/bisect as separate patches.
I'm not 100% convinced that the third patch is really that valuable.
Can you explain if there's any benefit to doing this other than readability?
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH 02/12] Return appropriate watch message to gdb
2008-11-03 10:35 [Qemu-devel] [PATCH 00/12] Enhance debugging support - 4th take Jan Kiszka
2008-11-03 10:35 ` [Qemu-devel] [PATCH 01/12] Refactor translation block CPU state handling Jan Kiszka
@ 2008-11-03 10:36 ` Jan Kiszka
2008-11-13 20:45 ` Anthony Liguori
2008-11-03 10:36 ` [Qemu-devel] [PATCH 03/12] Refactor and enhance break/watchpoint API Jan Kiszka
` (11 subsequent siblings)
13 siblings, 1 reply; 28+ messages in thread
From: Jan Kiszka @ 2008-11-03 10:36 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
Return the appropriate type prefix (r, a, none) when reporting
watchpoint hits to the gdb front-end.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
gdbstub.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
Index: b/gdbstub.c
===================================================================
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1493,6 +1493,7 @@ static void gdb_vm_stopped(void *opaque,
{
GDBState *s = opaque;
char buf[256];
+ const char *type;
int ret;
if (s->state == RS_SYSCALL)
@@ -1503,8 +1504,20 @@ static void gdb_vm_stopped(void *opaque,
if (reason == EXCP_DEBUG) {
if (s->env->watchpoint_hit) {
- snprintf(buf, sizeof(buf), "T%02xwatch:" TARGET_FMT_lx ";",
- SIGTRAP,
+ switch (s->env->watchpoint[s->env->watchpoint_hit - 1].flags &
+ (PAGE_READ | PAGE_WRITE)) {
+ case PAGE_READ:
+ type = "r";
+ break;
+ case PAGE_READ | PAGE_WRITE:
+ type = "a";
+ break;
+ default:
+ type = "";
+ break;
+ }
+ snprintf(buf, sizeof(buf), "T%02x%swatch:" TARGET_FMT_lx ";",
+ SIGTRAP, type,
s->env->watchpoint[s->env->watchpoint_hit - 1].vaddr);
put_packet(s, buf);
s->env->watchpoint_hit = 0;
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH 02/12] Return appropriate watch message to gdb
2008-11-03 10:36 ` [Qemu-devel] [PATCH 02/12] Return appropriate watch message to gdb Jan Kiszka
@ 2008-11-13 20:45 ` Anthony Liguori
2008-11-13 22:55 ` [Qemu-devel] " Jan Kiszka
0 siblings, 1 reply; 28+ messages in thread
From: Anthony Liguori @ 2008-11-13 20:45 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
Jan Kiszka wrote:
> Return the appropriate type prefix (r, a, none) when reporting
> watchpoint hits to the gdb front-end.
>
This doesn't build on it's own (there is no flags field in watchpoint).
Regards,
Anthony Liguori
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> gdbstub.c | 17 +++++++++++++++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> Index: b/gdbstub.c
> ===================================================================
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1493,6 +1493,7 @@ static void gdb_vm_stopped(void *opaque,
> {
> GDBState *s = opaque;
> char buf[256];
> + const char *type;
> int ret;
>
> if (s->state == RS_SYSCALL)
> @@ -1503,8 +1504,20 @@ static void gdb_vm_stopped(void *opaque,
>
> if (reason == EXCP_DEBUG) {
> if (s->env->watchpoint_hit) {
> - snprintf(buf, sizeof(buf), "T%02xwatch:" TARGET_FMT_lx ";",
> - SIGTRAP,
> + switch (s->env->watchpoint[s->env->watchpoint_hit - 1].flags &
> + (PAGE_READ | PAGE_WRITE)) {
> + case PAGE_READ:
> + type = "r";
> + break;
> + case PAGE_READ | PAGE_WRITE:
> + type = "a";
> + break;
> + default:
> + type = "";
> + break;
> + }
> + snprintf(buf, sizeof(buf), "T%02x%swatch:" TARGET_FMT_lx ";",
> + SIGTRAP, type,
> s->env->watchpoint[s->env->watchpoint_hit - 1].vaddr);
> put_packet(s, buf);
> s->env->watchpoint_hit = 0;
>
>
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread* [Qemu-devel] Re: [PATCH 02/12] Return appropriate watch message to gdb
2008-11-13 20:45 ` Anthony Liguori
@ 2008-11-13 22:55 ` Jan Kiszka
0 siblings, 0 replies; 28+ messages in thread
From: Jan Kiszka @ 2008-11-13 22:55 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
[-- Attachment #1: Type: text/plain, Size: 1943 bytes --]
Anthony Liguori wrote:
> Jan Kiszka wrote:
>> Return the appropriate type prefix (r, a, none) when reporting
>> watchpoint hits to the gdb front-end.
>>
>
> This doesn't build on it's own (there is no flags field in watchpoint).
Oh, typo while once moving this to the head of the queue: s/flags/type/.
Will fix, or reorder.
Jan
>
> Regards,
>
> Anthony Liguori
>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>> gdbstub.c | 17 +++++++++++++++--
>> 1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> Index: b/gdbstub.c
>> ===================================================================
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -1493,6 +1493,7 @@ static void gdb_vm_stopped(void *opaque,
>> {
>> GDBState *s = opaque;
>> char buf[256];
>> + const char *type;
>> int ret;
>>
>> if (s->state == RS_SYSCALL)
>> @@ -1503,8 +1504,20 @@ static void gdb_vm_stopped(void *opaque,
>>
>> if (reason == EXCP_DEBUG) {
>> if (s->env->watchpoint_hit) {
>> - snprintf(buf, sizeof(buf), "T%02xwatch:" TARGET_FMT_lx ";",
>> - SIGTRAP,
>> + switch (s->env->watchpoint[s->env->watchpoint_hit -
>> 1].flags &
>> + (PAGE_READ | PAGE_WRITE)) {
>> + case PAGE_READ:
>> + type = "r";
>> + break;
>> + case PAGE_READ | PAGE_WRITE:
>> + type = "a";
>> + break;
>> + default:
>> + type = "";
>> + break;
>> + }
>> + snprintf(buf, sizeof(buf), "T%02x%swatch:" TARGET_FMT_lx
>> ";",
>> + SIGTRAP, type,
>> s->env->watchpoint[s->env->watchpoint_hit -
>> 1].vaddr);
>> put_packet(s, buf);
>> s->env->watchpoint_hit = 0;
>>
>>
>>
>>
>
>
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 258 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH 03/12] Refactor and enhance break/watchpoint API
2008-11-03 10:35 [Qemu-devel] [PATCH 00/12] Enhance debugging support - 4th take Jan Kiszka
2008-11-03 10:35 ` [Qemu-devel] [PATCH 01/12] Refactor translation block CPU state handling Jan Kiszka
2008-11-03 10:36 ` [Qemu-devel] [PATCH 02/12] Return appropriate watch message to gdb Jan Kiszka
@ 2008-11-03 10:36 ` Jan Kiszka
2008-11-13 20:48 ` Anthony Liguori
2008-11-13 20:51 ` [Qemu-devel] " Anthony Liguori
2008-11-03 10:36 ` [Qemu-devel] [PATCH 04/12] Set mem_io_vaddr on io_read Jan Kiszka
` (10 subsequent siblings)
13 siblings, 2 replies; 28+ messages in thread
From: Jan Kiszka @ 2008-11-03 10:36 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
This patch prepares the QEMU cpu_watchpoint/breakpoint API to allow the
succeeding enhancements this series comes with.
First of all, it overcomes MAX_BREAKPOINTS/MAX_WATCHPOINTS by switching
to dynamically allocated data structures that are kept in linked lists.
This also allows to return a stable reference to the related objects,
required for later introduced x86 debug register support.
Breakpoints and watchpoints are stored with their full information set
and an additional flag field that makes them easily extensible for use
beyond pure guest debugging.
Finally, this restructuring lays the foundation for KVM to hook into
the debugging infrastructure, providing its own services where hardware
virtualization demands it. Once QEMUAccel is considered for merge,
those entry point should be included into its abstraction layer so that
accellerators can hook in even more cleanly.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
cpu-all.h | 23 +++--
cpu-defs.h | 26 +++---
exec.c | 201 ++++++++++++++++++++++++++++-------------------
gdbstub.c | 131 ++++++++++++++++--------------
target-alpha/translate.c | 7 -
target-arm/translate.c | 9 +-
target-cris/translate.c | 9 +-
target-i386/translate.c | 7 -
target-m68k/translate.c | 9 +-
target-mips/translate.c | 7 -
target-ppc/translate.c | 7 -
target-sh4/translate.c | 7 -
target-sparc/translate.c | 7 -
13 files changed, 264 insertions(+), 186 deletions(-)
Index: b/cpu-all.h
===================================================================
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -761,12 +761,23 @@ extern int use_icount;
void cpu_interrupt(CPUState *s, int mask);
void cpu_reset_interrupt(CPUState *env, int mask);
-int cpu_watchpoint_insert(CPUState *env, target_ulong addr, int type);
-int cpu_watchpoint_remove(CPUState *env, target_ulong addr);
-void cpu_watchpoint_remove_all(CPUState *env);
-int cpu_breakpoint_insert(CPUState *env, target_ulong pc);
-int cpu_breakpoint_remove(CPUState *env, target_ulong pc);
-void cpu_breakpoint_remove_all(CPUState *env);
+/* Breakpoint/watchpoint flags */
+#define BP_MEM_READ 0x01
+#define BP_MEM_WRITE 0x02
+#define BP_MEM_ACCESS (BP_MEM_READ | BP_MEM_WRITE)
+#define BP_GDB 0x10
+
+int cpu_breakpoint_insert(CPUState *env, target_ulong pc, int flags,
+ CPUBreakpoint **breakpoint);
+int cpu_breakpoint_remove(CPUState *env, target_ulong pc, int flags);
+void cpu_breakpoint_remove_by_ref(CPUState *env, CPUBreakpoint *breakpoint);
+void cpu_breakpoint_remove_all(CPUState *env, int mask);
+int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
+ int flags, CPUWatchpoint **watchpoint);
+int cpu_watchpoint_remove(CPUState *env, target_ulong addr,
+ target_ulong len, int flags);
+void cpu_watchpoint_remove_by_ref(CPUState *env, CPUWatchpoint *watchpoint);
+void cpu_watchpoint_remove_all(CPUState *env, int mask);
#define SSTEP_ENABLE 0x1 /* Enable simulated HW single stepping */
#define SSTEP_NOIRQ 0x2 /* Do not use IRQ while single stepping */
Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -537,7 +537,6 @@ void cpu_exec_init(CPUState *env)
cpu_index++;
}
env->cpu_index = cpu_index;
- env->nb_watchpoints = 0;
*penv = env;
#if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
register_savevm("cpu_common", cpu_index, CPU_COMMON_SAVE_VERSION,
@@ -1293,107 +1292,150 @@ static void breakpoint_invalidate(CPUSta
#endif
/* Add a watchpoint. */
-int cpu_watchpoint_insert(CPUState *env, target_ulong addr, int type)
+int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
+ int flags, CPUWatchpoint **watchpoint)
{
- int i;
+ CPUWatchpoint *wp;
- for (i = 0; i < env->nb_watchpoints; i++) {
- if (addr == env->watchpoint[i].vaddr)
- return 0;
- }
- if (env->nb_watchpoints >= MAX_WATCHPOINTS)
- return -1;
+ wp = qemu_malloc(sizeof(*wp));
+ if (!wp)
+ return -ENOBUFS;
+
+ wp->vaddr = addr;
+ wp->len_mask = 0;
+ wp->flags = flags;
+
+ wp->next = env->watchpoints;
+ wp->prev = NULL;
+ if (wp->next)
+ wp->next->prev = wp;
+ env->watchpoints = wp;
- i = env->nb_watchpoints++;
- env->watchpoint[i].vaddr = addr;
- env->watchpoint[i].type = type;
tlb_flush_page(env, addr);
/* FIXME: This flush is needed because of the hack to make memory ops
terminate the TB. It can be removed once the proper IO trap and
re-execute bits are in. */
tb_flush(env);
- return i;
-}
-/* Remove a watchpoint. */
-int cpu_watchpoint_remove(CPUState *env, target_ulong addr)
-{
- int i;
+ if (watchpoint)
+ *watchpoint = wp;
+ return 0;
+}
- for (i = 0; i < env->nb_watchpoints; i++) {
- if (addr == env->watchpoint[i].vaddr) {
- env->nb_watchpoints--;
- env->watchpoint[i] = env->watchpoint[env->nb_watchpoints];
- tlb_flush_page(env, addr);
+/* Remove a specific watchpoint. */
+int cpu_watchpoint_remove(CPUState *env, target_ulong addr, target_ulong len,
+ int flags)
+{
+ CPUWatchpoint *wp;
+
+ for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
+ if (addr == wp->vaddr && flags == wp->flags) {
+ cpu_watchpoint_remove_by_ref(env, wp);
return 0;
}
}
- return -1;
+ return -ENOENT;
}
-/* Remove all watchpoints. */
-void cpu_watchpoint_remove_all(CPUState *env) {
- int i;
+/* Remove a specific watchpoint by reference. */
+void cpu_watchpoint_remove_by_ref(CPUState *env, CPUWatchpoint *watchpoint)
+{
+ if (watchpoint->next)
+ watchpoint->next->prev = watchpoint->prev;
+ if (watchpoint->prev)
+ watchpoint->prev->next = watchpoint->next;
+ else
+ env->watchpoints = watchpoint->next;
- for (i = 0; i < env->nb_watchpoints; i++) {
- tlb_flush_page(env, env->watchpoint[i].vaddr);
- }
- env->nb_watchpoints = 0;
+ tlb_flush_page(env, watchpoint->vaddr);
+
+ qemu_free(watchpoint);
}
-/* add a breakpoint. EXCP_DEBUG is returned by the CPU loop if a
- breakpoint is reached */
-int cpu_breakpoint_insert(CPUState *env, target_ulong pc)
+/* Remove all matching watchpoints. */
+void cpu_watchpoint_remove_all(CPUState *env, int mask)
{
-#if defined(TARGET_HAS_ICE)
- int i;
+ CPUWatchpoint *wp;
- for(i = 0; i < env->nb_breakpoints; i++) {
- if (env->breakpoints[i] == pc)
- return 0;
- }
+ for (wp = env->watchpoints; wp != NULL; wp = wp->next)
+ if (wp->flags & mask)
+ cpu_watchpoint_remove_by_ref(env, wp);
+}
- if (env->nb_breakpoints >= MAX_BREAKPOINTS)
- return -1;
- env->breakpoints[env->nb_breakpoints++] = pc;
+/* Add a breakpoint. */
+int cpu_breakpoint_insert(CPUState *env, target_ulong pc, int flags,
+ CPUBreakpoint **breakpoint)
+{
+#if defined(TARGET_HAS_ICE)
+ CPUBreakpoint *bp;
+
+ bp = qemu_malloc(sizeof(*bp));
+ if (!bp)
+ return -ENOBUFS;
+
+ bp->pc = pc;
+ bp->flags = flags;
+
+ bp->next = env->breakpoints;
+ bp->prev = NULL;
+ if (bp->next)
+ bp->next->prev = bp;
+ env->breakpoints = bp;
breakpoint_invalidate(env, pc);
+
+ if (breakpoint)
+ *breakpoint = bp;
return 0;
#else
- return -1;
+ return -ENOSYS;
#endif
}
-/* remove all breakpoints */
-void cpu_breakpoint_remove_all(CPUState *env) {
+/* Remove a specific breakpoint. */
+int cpu_breakpoint_remove(CPUState *env, target_ulong pc, int flags)
+{
#if defined(TARGET_HAS_ICE)
- int i;
- for(i = 0; i < env->nb_breakpoints; i++) {
- breakpoint_invalidate(env, env->breakpoints[i]);
+ CPUBreakpoint *bp;
+
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+ if (bp->pc == pc && bp->flags == flags) {
+ cpu_breakpoint_remove_by_ref(env, bp);
+ return 0;
+ }
}
- env->nb_breakpoints = 0;
+ return -ENOENT;
+#else
+ return -ENOSYS;
#endif
}
-/* remove a breakpoint */
-int cpu_breakpoint_remove(CPUState *env, target_ulong pc)
+/* Remove a specific breakpoint by reference. */
+void cpu_breakpoint_remove_by_ref(CPUState *env, CPUBreakpoint *breakpoint)
{
#if defined(TARGET_HAS_ICE)
- int i;
- for(i = 0; i < env->nb_breakpoints; i++) {
- if (env->breakpoints[i] == pc)
- goto found;
- }
- return -1;
- found:
- env->nb_breakpoints--;
- if (i < env->nb_breakpoints)
- env->breakpoints[i] = env->breakpoints[env->nb_breakpoints];
+ if (breakpoint->next)
+ breakpoint->next->prev = breakpoint->prev;
+ if (breakpoint->prev)
+ breakpoint->prev->next = breakpoint->next;
+ else
+ env->breakpoints = breakpoint->next;
- breakpoint_invalidate(env, pc);
- return 0;
-#else
- return -1;
+ breakpoint_invalidate(env, breakpoint->pc);
+
+ qemu_free(breakpoint);
+#endif
+}
+
+/* Remove all matching breakpoints. */
+void cpu_breakpoint_remove_all(CPUState *env, int mask)
+{
+#if defined(TARGET_HAS_ICE)
+ CPUBreakpoint *bp;
+
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next)
+ if (bp->flags & mask)
+ cpu_breakpoint_remove_by_ref(env, bp);
#endif
}
@@ -1875,7 +1917,7 @@ int tlb_set_page_exec(CPUState *env, tar
target_phys_addr_t addend;
int ret;
CPUTLBEntry *te;
- int i;
+ CPUWatchpoint *wp;
target_phys_addr_t iotlb;
p = phys_page_find(paddr >> TARGET_PAGE_BITS);
@@ -1916,8 +1958,8 @@ int tlb_set_page_exec(CPUState *env, tar
code_address = address;
/* Make accesses to pages with watchpoints go via the
watchpoint trap routines. */
- for (i = 0; i < env->nb_watchpoints; i++) {
- if (vaddr == (env->watchpoint[i].vaddr & TARGET_PAGE_MASK)) {
+ for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
+ if (vaddr == (wp->vaddr & TARGET_PAGE_MASK)) {
iotlb = io_mem_watch + paddr;
/* TODO: The memory case can be optimized by not trapping
reads of pages with a write breakpoint. */
@@ -2447,13 +2489,12 @@ static void check_watchpoint(int offset,
{
CPUState *env = cpu_single_env;
target_ulong vaddr;
- int i;
+ CPUWatchpoint *wp;
vaddr = (env->mem_io_vaddr & TARGET_PAGE_MASK) + offset;
- for (i = 0; i < env->nb_watchpoints; i++) {
- if (vaddr == env->watchpoint[i].vaddr
- && (env->watchpoint[i].type & flags)) {
- env->watchpoint_hit = i + 1;
+ for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
+ if (vaddr == wp->vaddr && (wp->flags & flags)) {
+ env->watchpoint_hit = wp;
cpu_interrupt(env, CPU_INTERRUPT_DEBUG);
break;
}
@@ -2465,40 +2506,40 @@ static void check_watchpoint(int offset,
phys routines. */
static uint32_t watch_mem_readb(void *opaque, target_phys_addr_t addr)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, PAGE_READ);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
return ldub_phys(addr);
}
static uint32_t watch_mem_readw(void *opaque, target_phys_addr_t addr)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, PAGE_READ);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
return lduw_phys(addr);
}
static uint32_t watch_mem_readl(void *opaque, target_phys_addr_t addr)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, PAGE_READ);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
return ldl_phys(addr);
}
static void watch_mem_writeb(void *opaque, target_phys_addr_t addr,
uint32_t val)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, PAGE_WRITE);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
stb_phys(addr, val);
}
static void watch_mem_writew(void *opaque, target_phys_addr_t addr,
uint32_t val)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, PAGE_WRITE);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
stw_phys(addr, val);
}
static void watch_mem_writel(void *opaque, target_phys_addr_t addr,
uint32_t val)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, PAGE_WRITE);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
stl_phys(addr, val);
}
Index: b/gdbstub.c
===================================================================
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1145,10 +1145,64 @@ void gdb_register_coprocessor(CPUState *
}
}
+/* GDB breakpoint/watchpoint types */
+#define GDB_BREAKPOINT_SW 0
+#define GDB_BREAKPOINT_HW 1
+#define GDB_WATCHPOINT_WRITE 2
+#define GDB_WATCHPOINT_READ 3
+#define GDB_WATCHPOINT_ACCESS 4
+
+#ifndef CONFIG_USER_ONLY
+static const int xlat_gdb_type[] = {
+ [GDB_WATCHPOINT_WRITE] = BP_GDB | BP_MEM_WRITE,
+ [GDB_WATCHPOINT_READ] = BP_GDB | BP_MEM_READ,
+ [GDB_WATCHPOINT_ACCESS] = BP_GDB | BP_MEM_ACCESS,
+};
+#endif
+
+static int gdb_breakpoint_insert(CPUState *env, target_ulong addr,
+ target_ulong len, int type)
+{
+ switch (type) {
+ case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
+ return cpu_breakpoint_insert(env, addr, BP_GDB, NULL);
+#ifndef CONFIG_USER_ONLY
+ case GDB_WATCHPOINT_WRITE ... GDB_WATCHPOINT_ACCESS:
+ return cpu_watchpoint_insert(env, addr, len, xlat_gdb_type[type],
+ NULL);
+#endif
+ default:
+ return -ENOSYS;
+ }
+}
+
+static int gdb_breakpoint_remove(CPUState *env, target_ulong addr,
+ target_ulong len, int type)
+{
+ switch (type) {
+ case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
+ return cpu_breakpoint_remove(env, addr, BP_GDB);
+#ifndef CONFIG_USER_ONLY
+ case GDB_WATCHPOINT_WRITE ... GDB_WATCHPOINT_ACCESS:
+ return cpu_watchpoint_remove(env, addr, len, xlat_gdb_type[type]);
+#endif
+ default:
+ return -ENOSYS;
+ }
+}
+
+static void gdb_breakpoint_remove_all(CPUState *env)
+{
+ cpu_breakpoint_remove_all(env, BP_GDB);
+#ifndef CONFIG_USER_ONLY
+ cpu_watchpoint_remove_all(env, BP_GDB);
+#endif
+}
+
static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
{
const char *p;
- int ch, reg_size, type;
+ int ch, reg_size, type, res;
char buf[MAX_PACKET_LENGTH];
uint8_t mem_buf[MAX_PACKET_LENGTH];
uint8_t *registers;
@@ -1168,8 +1222,7 @@ static int gdb_handle_packet(GDBState *s
* because gdb is doing and initial connect and the state
* should be cleaned up.
*/
- cpu_breakpoint_remove_all(env);
- cpu_watchpoint_remove_all(env);
+ gdb_breakpoint_remove_all(env);
break;
case 'c':
if (*p != '\0') {
@@ -1203,8 +1256,7 @@ static int gdb_handle_packet(GDBState *s
exit(0);
case 'D':
/* Detach packet */
- cpu_breakpoint_remove_all(env);
- cpu_watchpoint_remove_all(env);
+ gdb_breakpoint_remove_all(env);
gdb_continue(s);
put_packet(s, "OK");
break;
@@ -1327,44 +1379,6 @@ static int gdb_handle_packet(GDBState *s
put_packet(s, "OK");
break;
case 'Z':
- type = strtoul(p, (char **)&p, 16);
- if (*p == ',')
- p++;
- addr = strtoull(p, (char **)&p, 16);
- if (*p == ',')
- p++;
- len = strtoull(p, (char **)&p, 16);
- switch (type) {
- case 0:
- case 1:
- if (cpu_breakpoint_insert(env, addr) < 0)
- goto breakpoint_error;
- put_packet(s, "OK");
- break;
-#ifndef CONFIG_USER_ONLY
- case 2:
- type = PAGE_WRITE;
- goto insert_watchpoint;
- case 3:
- type = PAGE_READ;
- goto insert_watchpoint;
- case 4:
- type = PAGE_READ | PAGE_WRITE;
- insert_watchpoint:
- if (cpu_watchpoint_insert(env, addr, type) < 0)
- goto breakpoint_error;
- put_packet(s, "OK");
- break;
-#endif
- default:
- put_packet(s, "");
- break;
- }
- break;
- breakpoint_error:
- put_packet(s, "E22");
- break;
-
case 'z':
type = strtoul(p, (char **)&p, 16);
if (*p == ',')
@@ -1373,17 +1387,16 @@ static int gdb_handle_packet(GDBState *s
if (*p == ',')
p++;
len = strtoull(p, (char **)&p, 16);
- if (type == 0 || type == 1) {
- cpu_breakpoint_remove(env, addr);
- put_packet(s, "OK");
-#ifndef CONFIG_USER_ONLY
- } else if (type >= 2 || type <= 4) {
- cpu_watchpoint_remove(env, addr);
- put_packet(s, "OK");
-#endif
- } else {
+ if (ch == 'Z')
+ res = gdb_breakpoint_insert(env, addr, len, type);
+ else
+ res = gdb_breakpoint_remove(env, addr, len, type);
+ if (res >= 0)
+ put_packet(s, "OK");
+ else if (res == -ENOSYS)
put_packet(s, "");
- }
+ else
+ put_packet(s, "E22");
break;
case 'q':
case 'Q':
@@ -1504,12 +1517,11 @@ static void gdb_vm_stopped(void *opaque,
if (reason == EXCP_DEBUG) {
if (s->env->watchpoint_hit) {
- switch (s->env->watchpoint[s->env->watchpoint_hit - 1].flags &
- (PAGE_READ | PAGE_WRITE)) {
- case PAGE_READ:
+ switch (s->env->watchpoint_hit->flags & BP_MEM_ACCESS) {
+ case BP_MEM_READ:
type = "r";
break;
- case PAGE_READ | PAGE_WRITE:
+ case BP_MEM_ACCESS:
type = "a";
break;
default:
@@ -1517,10 +1529,9 @@ static void gdb_vm_stopped(void *opaque,
break;
}
snprintf(buf, sizeof(buf), "T%02x%swatch:" TARGET_FMT_lx ";",
- SIGTRAP, type,
- s->env->watchpoint[s->env->watchpoint_hit - 1].vaddr);
+ SIGTRAP, type, s->env->watchpoint_hit->vaddr);
put_packet(s, buf);
- s->env->watchpoint_hit = 0;
+ s->env->watchpoint_hit = NULL;
return;
}
tb_flush(s->env);
Index: b/cpu-defs.h
===================================================================
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -82,8 +82,6 @@ typedef uint64_t target_phys_addr_t;
#define EXCP_HLT 0x10001 /* hlt instruction reached */
#define EXCP_DEBUG 0x10002 /* cpu stopped after a breakpoint or singlestep */
#define EXCP_HALTED 0x10003 /* cpu is halted (waiting for external event) */
-#define MAX_BREAKPOINTS 32
-#define MAX_WATCHPOINTS 32
#define TB_JMP_CACHE_BITS 12
#define TB_JMP_CACHE_SIZE (1 << TB_JMP_CACHE_BITS)
@@ -142,6 +140,19 @@ typedef struct icount_decr_u16 {
} icount_decr_u16;
#endif
+typedef struct CPUBreakpoint {
+ target_ulong pc;
+ int flags; /* BP_* */
+ struct CPUBreakpoint *prev, *next;
+} CPUBreakpoint;
+
+typedef struct CPUWatchpoint {
+ target_ulong vaddr;
+ target_ulong len_mask;
+ int flags; /* BP_* */
+ struct CPUWatchpoint *prev, *next;
+} CPUWatchpoint;
+
#define CPU_TEMP_BUF_NLONGS 128
#define CPU_COMMON \
struct TranslationBlock *current_tb; /* currently executing TB */ \
@@ -174,16 +185,11 @@ typedef struct icount_decr_u16 {
\
/* from this point: preserved by CPU reset */ \
/* ice debug support */ \
- target_ulong breakpoints[MAX_BREAKPOINTS]; \
- int nb_breakpoints; \
+ CPUBreakpoint *breakpoints; \
int singlestep_enabled; \
\
- struct { \
- target_ulong vaddr; \
- int type; /* PAGE_READ/PAGE_WRITE */ \
- } watchpoint[MAX_WATCHPOINTS]; \
- int nb_watchpoints; \
- int watchpoint_hit; \
+ CPUWatchpoint *watchpoints; \
+ CPUWatchpoint *watchpoint_hit; \
\
struct GDBRegisterState *gdb_regs; \
\
Index: b/target-i386/translate.c
===================================================================
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -7533,6 +7533,7 @@ static inline void gen_intermediate_code
DisasContext dc1, *dc = &dc1;
target_ulong pc_ptr;
uint16_t *gen_opc_end;
+ CPUBreakpoint *bp;
int j, lj, cflags;
uint64_t flags;
target_ulong pc_start;
@@ -7616,9 +7617,9 @@ static inline void gen_intermediate_code
gen_icount_start();
for(;;) {
- if (env->nb_breakpoints > 0) {
- for(j = 0; j < env->nb_breakpoints; j++) {
- if (env->breakpoints[j] == pc_ptr) {
+ if (unlikely(env->breakpoints)) {
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+ if (bp->pc == pc_ptr) {
gen_debug(dc, pc_ptr - dc->cs_base);
break;
}
Index: b/target-alpha/translate.c
===================================================================
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -2240,6 +2240,7 @@ static always_inline void gen_intermedia
target_ulong pc_start;
uint32_t insn;
uint16_t *gen_opc_end;
+ CPUBreakpoint *bp;
int j, lj = -1;
int ret;
int num_insns;
@@ -2262,9 +2263,9 @@ static always_inline void gen_intermedia
gen_icount_start();
for (ret = 0; ret == 0;) {
- if (env->nb_breakpoints > 0) {
- for(j = 0; j < env->nb_breakpoints; j++) {
- if (env->breakpoints[j] == ctx.pc) {
+ if (unlikely(env->breakpoints)) {
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+ if (bp->pc == ctx.pc) {
gen_excp(&ctx, EXCP_DEBUG, 0);
break;
}
Index: b/target-arm/translate.c
===================================================================
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -8586,6 +8586,7 @@ static inline void gen_intermediate_code
int search_pc)
{
DisasContext dc1, *dc = &dc1;
+ CPUBreakpoint *bp;
uint16_t *gen_opc_end;
int j, lj;
target_ulong pc_start;
@@ -8662,9 +8663,9 @@ static inline void gen_intermediate_code
}
#endif
- if (env->nb_breakpoints > 0) {
- for(j = 0; j < env->nb_breakpoints; j++) {
- if (env->breakpoints[j] == dc->pc) {
+ if (unlikely(env->breakpoints)) {
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+ if (bp->pc == dc->pc) {
gen_set_condexec(dc);
gen_set_pc_im(dc->pc);
gen_exception(EXCP_DEBUG);
@@ -8717,7 +8718,7 @@ static inline void gen_intermediate_code
/* Terminate the TB on memory ops if watchpoints are present. */
/* FIXME: This should be replacd by the deterministic execution
* IRQ raising bits. */
- if (dc->is_mem && env->nb_watchpoints)
+ if (dc->is_mem && env->watchpoints)
break;
/* Translation stops when a conditional branch is enoutered.
Index: b/target-cris/translate.c
===================================================================
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -3172,10 +3172,11 @@ cris_decoder(DisasContext *dc)
static void check_breakpoint(CPUState *env, DisasContext *dc)
{
- int j;
- if (env->nb_breakpoints > 0) {
- for(j = 0; j < env->nb_breakpoints; j++) {
- if (env->breakpoints[j] == dc->pc) {
+ CPUBreakpoint *bp;
+
+ if (unlikely(env->breakpoints)) {
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+ if (bp->pc == dc->pc) {
cris_evaluate_flags (dc);
tcg_gen_movi_tl(env_pc, dc->pc);
t_gen_raise_exception(EXCP_DEBUG);
Index: b/target-m68k/translate.c
===================================================================
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -2917,6 +2917,7 @@ gen_intermediate_code_internal(CPUState
{
DisasContext dc1, *dc = &dc1;
uint16_t *gen_opc_end;
+ CPUBreakpoint *bp;
int j, lj;
target_ulong pc_start;
int pc_offset;
@@ -2950,9 +2951,9 @@ gen_intermediate_code_internal(CPUState
do {
pc_offset = dc->pc - pc_start;
gen_throws_exception = NULL;
- if (env->nb_breakpoints > 0) {
- for(j = 0; j < env->nb_breakpoints; j++) {
- if (env->breakpoints[j] == dc->pc) {
+ if (unlikely(env->breakpoints)) {
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+ if (bp->pc == dc->pc) {
gen_exception(dc, dc->pc, EXCP_DEBUG);
dc->is_jmp = DISAS_JUMP;
break;
@@ -2982,7 +2983,7 @@ gen_intermediate_code_internal(CPUState
/* Terminate the TB on memory ops if watchpoints are present. */
/* FIXME: This should be replaced by the deterministic execution
* IRQ raising bits. */
- if (dc->is_mem && env->nb_watchpoints)
+ if (dc->is_mem && env->watchpoints)
break;
} while (!dc->is_jmp && gen_opc_ptr < gen_opc_end &&
!env->singlestep_enabled &&
Index: b/target-mips/translate.c
===================================================================
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -8442,6 +8442,7 @@ gen_intermediate_code_internal (CPUState
DisasContext ctx;
target_ulong pc_start;
uint16_t *gen_opc_end;
+ CPUBreakpoint *bp;
int j, lj = -1;
int num_insns;
int max_insns;
@@ -8481,9 +8482,9 @@ gen_intermediate_code_internal (CPUState
#endif
gen_icount_start();
while (ctx.bstate == BS_NONE) {
- if (env->nb_breakpoints > 0) {
- for(j = 0; j < env->nb_breakpoints; j++) {
- if (env->breakpoints[j] == ctx.pc) {
+ if (unlikely(env->breakpoints)) {
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+ if (bp->pc == ctx.pc) {
save_cpu_state(&ctx, 1);
ctx.bstate = BS_BRANCH;
tcg_gen_helper_0_i(do_raise_exception, EXCP_DEBUG);
Index: b/target-ppc/translate.c
===================================================================
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -6779,6 +6779,7 @@ static always_inline void gen_intermedia
target_ulong pc_start;
uint16_t *gen_opc_end;
int supervisor, little_endian;
+ CPUBreakpoint *bp;
int j, lj = -1;
int num_insns;
int max_insns;
@@ -6833,9 +6834,9 @@ static always_inline void gen_intermedia
gen_icount_start();
/* Set env in case of segfault during code fetch */
while (ctx.exception == POWERPC_EXCP_NONE && gen_opc_ptr < gen_opc_end) {
- if (unlikely(env->nb_breakpoints > 0)) {
- for (j = 0; j < env->nb_breakpoints; j++) {
- if (env->breakpoints[j] == ctx.nip) {
+ if (unlikely(env->breakpoints)) {
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+ if (bp->pc == ctx.nip) {
gen_update_nip(&ctx, ctx.nip);
gen_op_debug();
break;
Index: b/target-sh4/translate.c
===================================================================
--- a/target-sh4/translate.c
+++ b/target-sh4/translate.c
@@ -1799,6 +1799,7 @@ gen_intermediate_code_internal(CPUState
DisasContext ctx;
target_ulong pc_start;
static uint16_t *gen_opc_end;
+ CPUBreakpoint *bp;
int i, ii;
int num_insns;
int max_insns;
@@ -1832,9 +1833,9 @@ gen_intermediate_code_internal(CPUState
max_insns = CF_COUNT_MASK;
gen_icount_start();
while (ctx.bstate == BS_NONE && gen_opc_ptr < gen_opc_end) {
- if (env->nb_breakpoints > 0) {
- for (i = 0; i < env->nb_breakpoints; i++) {
- if (ctx.pc == env->breakpoints[i]) {
+ if (unlikely(env->breakpoints)) {
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+ if (ctx.pc == bp->pc) {
/* We have hit a breakpoint - make sure PC is up-to-date */
tcg_gen_movi_i32(cpu_pc, ctx.pc);
tcg_gen_helper_0_0(helper_debug);
Index: b/target-sparc/translate.c
===================================================================
--- a/target-sparc/translate.c
+++ b/target-sparc/translate.c
@@ -4777,6 +4777,7 @@ static inline void gen_intermediate_code
target_ulong pc_start, last_pc;
uint16_t *gen_opc_end;
DisasContext dc1, *dc = &dc1;
+ CPUBreakpoint *bp;
int j, lj = -1;
int num_insns;
int max_insns;
@@ -4814,9 +4815,9 @@ static inline void gen_intermediate_code
max_insns = CF_COUNT_MASK;
gen_icount_start();
do {
- if (env->nb_breakpoints > 0) {
- for(j = 0; j < env->nb_breakpoints; j++) {
- if (env->breakpoints[j] == dc->pc) {
+ if (unlikely(env->breakpoints)) {
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+ if (bp->pc == dc->pc) {
if (dc->pc != pc_start)
save_state(dc, cpu_cond);
tcg_gen_helper_0_0(helper_debug);
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH 03/12] Refactor and enhance break/watchpoint API
2008-11-03 10:36 ` [Qemu-devel] [PATCH 03/12] Refactor and enhance break/watchpoint API Jan Kiszka
@ 2008-11-13 20:48 ` Anthony Liguori
2008-11-13 22:56 ` [Qemu-devel] " Jan Kiszka
2008-11-13 20:51 ` [Qemu-devel] " Anthony Liguori
1 sibling, 1 reply; 28+ messages in thread
From: Anthony Liguori @ 2008-11-13 20:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
Jan Kiszka wrote:
> Index: b/gdbstub.c
> ===================================================================
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1145,10 +1145,64 @@ void gdb_register_coprocessor(CPUState *
> }
> }
>
> +/* GDB breakpoint/watchpoint types */
> +#define GDB_BREAKPOINT_SW 0
> +#define GDB_BREAKPOINT_HW 1
> +#define GDB_WATCHPOINT_WRITE 2
> +#define GDB_WATCHPOINT_READ 3
> +#define GDB_WATCHPOINT_ACCESS 4
> +
> +#ifndef CONFIG_USER_ONLY
> +static const int xlat_gdb_type[] = {
> + [GDB_WATCHPOINT_WRITE] = BP_GDB | BP_MEM_WRITE,
> + [GDB_WATCHPOINT_READ] = BP_GDB | BP_MEM_READ,
> + [GDB_WATCHPOINT_ACCESS] = BP_GDB | BP_MEM_ACCESS,
> +};
> +#endif
> +
> +static int gdb_breakpoint_insert(CPUState *env, target_ulong addr,
> + target_ulong len, int type)
> +{
> + switch (type) {
> + case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
>
We've avoided this GCCism pretty much. I don't think the code is
significantly cleaner with it so I think it's best to avoid it.
> +typedef struct CPUWatchpoint {
> + target_ulong vaddr;
> + target_ulong len_mask;
> + int flags; /* BP_* */
> + struct CPUWatchpoint *prev, *next;
> +} CPUWatchpoint;
> +
> #define CPU_TEMP_BUF_NLONGS 128
> #define CPU_COMMON \
> struct TranslationBlock *current_tb; /* currently executing TB */ \
> @@ -174,16 +185,11 @@ typedef struct icount_decr_u16 {
> \
> /* from this point: preserved by CPU reset */ \
> /* ice debug support */ \
> - target_ulong breakpoints[MAX_BREAKPOINTS]; \
> - int nb_breakpoints; \
> + CPUBreakpoint *breakpoints; \
> int singlestep_enabled; \
> \
> - struct { \
> - target_ulong vaddr; \
> - int type; /* PAGE_READ/PAGE_WRITE */ \
> - } watchpoint[MAX_WATCHPOINTS]; \
> - int nb_watchpoints; \
> - int watchpoint_hit; \
> + CPUWatchpoint *watchpoints; \
> + CPUWatchpoint *watchpoint_hit; \
>
The previous patch doesn't build because this patch is where you
introduce flags into watch points. Please make sure the whole series
builds and works at each patch. Otherwise bisecting breaks.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 28+ messages in thread* [Qemu-devel] Re: [PATCH 03/12] Refactor and enhance break/watchpoint API
2008-11-13 20:48 ` Anthony Liguori
@ 2008-11-13 22:56 ` Jan Kiszka
2008-11-14 2:24 ` Jamie Lokier
0 siblings, 1 reply; 28+ messages in thread
From: Jan Kiszka @ 2008-11-13 22:56 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
[-- Attachment #1: Type: text/plain, Size: 1360 bytes --]
Anthony Liguori wrote:
> Jan Kiszka wrote:
>> Index: b/gdbstub.c
>> ===================================================================
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -1145,10 +1145,64 @@ void gdb_register_coprocessor(CPUState *
>> }
>> }
>>
>> +/* GDB breakpoint/watchpoint types */
>> +#define GDB_BREAKPOINT_SW 0
>> +#define GDB_BREAKPOINT_HW 1
>> +#define GDB_WATCHPOINT_WRITE 2
>> +#define GDB_WATCHPOINT_READ 3
>> +#define GDB_WATCHPOINT_ACCESS 4
>> +
>> +#ifndef CONFIG_USER_ONLY
>> +static const int xlat_gdb_type[] = {
>> + [GDB_WATCHPOINT_WRITE] = BP_GDB | BP_MEM_WRITE,
>> + [GDB_WATCHPOINT_READ] = BP_GDB | BP_MEM_READ,
>> + [GDB_WATCHPOINT_ACCESS] = BP_GDB | BP_MEM_ACCESS,
>> +};
>> +#endif
>> +
>> +static int gdb_breakpoint_insert(CPUState *env, target_ulong addr,
>> + target_ulong len, int type)
>> +{
>> + switch (type) {
>> + case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
>>
>
> We've avoided this GCCism pretty much. I don't think the code is
> significantly cleaner with it so I think it's best to avoid it.
OK, I see the general problem. Restricting ourselves here is not a big
issue - but for my other series which tried hard to canonicalizes x86's
cpu_gdb_read/write_register, sigh...
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 258 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] Re: [PATCH 03/12] Refactor and enhance break/watchpoint API
2008-11-13 22:56 ` [Qemu-devel] " Jan Kiszka
@ 2008-11-14 2:24 ` Jamie Lokier
0 siblings, 0 replies; 28+ messages in thread
From: Jamie Lokier @ 2008-11-14 2:24 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
Jan Kiszka wrote:
> Anthony Liguori wrote:
> > Jan Kiszka wrote:
> >> Index: b/gdbstub.c
> >> ===================================================================
> >> --- a/gdbstub.c
> >> +++ b/gdbstub.c
> >> @@ -1145,10 +1145,64 @@ void gdb_register_coprocessor(CPUState *
> >> }
> >> }
> >>
> >> +/* GDB breakpoint/watchpoint types */
> >> +#define GDB_BREAKPOINT_SW 0
> >> +#define GDB_BREAKPOINT_HW 1
> >> +#define GDB_WATCHPOINT_WRITE 2
> >> +#define GDB_WATCHPOINT_READ 3
> >> +#define GDB_WATCHPOINT_ACCESS 4
> >> +
> >> +#ifndef CONFIG_USER_ONLY
> >> +static const int xlat_gdb_type[] = {
> >> + [GDB_WATCHPOINT_WRITE] = BP_GDB | BP_MEM_WRITE,
> >> + [GDB_WATCHPOINT_READ] = BP_GDB | BP_MEM_READ,
> >> + [GDB_WATCHPOINT_ACCESS] = BP_GDB | BP_MEM_ACCESS,
> >> +};
> >> +#endif
> >> +
> >> +static int gdb_breakpoint_insert(CPUState *env, target_ulong addr,
> >> + target_ulong len, int type)
> >> +{
> >> + switch (type) {
> >> + case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
> >>
> >
> > We've avoided this GCCism pretty much. I don't think the code is
> > significantly cleaner with it so I think it's best to avoid it.
>
> OK, I see the general problem. Restricting ourselves here is not a big
> issue - but for my other series which tried hard to canonicalizes x86's
> cpu_gdb_read/write_register, sigh...
I think the code would be clearer with separate cases in this instance anyway.
Usuallywhen you have an enumeration which is just labels without a
numerical purpose, seeing "..." or "<" on it is not clear which cases
are included, and it tends to break quietly when someone adds more values.
If there's a long sequence of cases which crops up often, then you can
use a macro "#define cases_FOO case FOO1: case FOO2: case FOO3:".
-- Jamie
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [PATCH 03/12] Refactor and enhance break/watchpoint API
2008-11-03 10:36 ` [Qemu-devel] [PATCH 03/12] Refactor and enhance break/watchpoint API Jan Kiszka
2008-11-13 20:48 ` Anthony Liguori
@ 2008-11-13 20:51 ` Anthony Liguori
2008-11-13 22:58 ` [Qemu-devel] " Jan Kiszka
1 sibling, 1 reply; 28+ messages in thread
From: Anthony Liguori @ 2008-11-13 20:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
Jan Kiszka wrote:
> This patch prepares the QEMU cpu_watchpoint/breakpoint API to allow the
> succeeding enhancements this series comes with.
>
> First of all, it overcomes MAX_BREAKPOINTS/MAX_WATCHPOINTS by switching
> to dynamically allocated data structures that are kept in linked lists.
> This also allows to return a stable reference to the related objects,
> required for later introduced x86 debug register support.
>
> Breakpoints and watchpoints are stored with their full information set
> and an additional flag field that makes them easily extensible for use
> beyond pure guest debugging.
>
> Finally, this restructuring lays the foundation for KVM to hook into
> the debugging infrastructure, providing its own services where hardware
> virtualization demands it. Once QEMUAccel is considered for merge,
> those entry point should be included into its abstraction layer so that
> accellerators can hook in even more cleanly.
>
We've merged KVM support (although not QEMUAccel), so perhaps you can
also add the KVM hooks in this series that you are thinking of?
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] Re: [PATCH 03/12] Refactor and enhance break/watchpoint API
2008-11-13 20:51 ` [Qemu-devel] " Anthony Liguori
@ 2008-11-13 22:58 ` Jan Kiszka
2008-11-15 8:29 ` Jan Kiszka
0 siblings, 1 reply; 28+ messages in thread
From: Jan Kiszka @ 2008-11-13 22:58 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
[-- Attachment #1: Type: text/plain, Size: 1613 bytes --]
Anthony Liguori wrote:
> Jan Kiszka wrote:
>> This patch prepares the QEMU cpu_watchpoint/breakpoint API to allow the
>> succeeding enhancements this series comes with.
>>
>> First of all, it overcomes MAX_BREAKPOINTS/MAX_WATCHPOINTS by switching
>> to dynamically allocated data structures that are kept in linked lists.
>> This also allows to return a stable reference to the related objects,
>> required for later introduced x86 debug register support.
>>
>> Breakpoints and watchpoints are stored with their full information set
>> and an additional flag field that makes them easily extensible for use
>> beyond pure guest debugging.
>>
>> Finally, this restructuring lays the foundation for KVM to hook into
>> the debugging infrastructure, providing its own services where hardware
>> virtualization demands it. Once QEMUAccel is considered for merge,
>> those entry point should be included into its abstraction layer so that
>> accellerators can hook in even more cleanly.
>>
>
> We've merged KVM support (although not QEMUAccel), so perhaps you can
> also add the KVM hooks in this series that you are thinking of?
I will check how much of my kvm patches for guest debugging can already
be ported over.
That topic is more complex - if you recall my according series - as kvm
requires kernel changes to gain full guest debugging support. I'm not
even sure if it makes sense to add support for the current kernel
interface as it is too restricted (no watchpoints, only 4 breakpoints).
However, will re-check if some patch re-ordering may help the migration.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 258 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] Re: [PATCH 03/12] Refactor and enhance break/watchpoint API
2008-11-13 22:58 ` [Qemu-devel] " Jan Kiszka
@ 2008-11-15 8:29 ` Jan Kiszka
2008-11-15 16:12 ` Anthony Liguori
0 siblings, 1 reply; 28+ messages in thread
From: Jan Kiszka @ 2008-11-15 8:29 UTC (permalink / raw)
To: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2510 bytes --]
Jan Kiszka wrote:
> Anthony Liguori wrote:
>> Jan Kiszka wrote:
>>> This patch prepares the QEMU cpu_watchpoint/breakpoint API to allow the
>>> succeeding enhancements this series comes with.
>>>
>>> First of all, it overcomes MAX_BREAKPOINTS/MAX_WATCHPOINTS by switching
>>> to dynamically allocated data structures that are kept in linked lists.
>>> This also allows to return a stable reference to the related objects,
>>> required for later introduced x86 debug register support.
>>>
>>> Breakpoints and watchpoints are stored with their full information set
>>> and an additional flag field that makes them easily extensible for use
>>> beyond pure guest debugging.
>>>
>>> Finally, this restructuring lays the foundation for KVM to hook into
>>> the debugging infrastructure, providing its own services where hardware
>>> virtualization demands it. Once QEMUAccel is considered for merge,
>>> those entry point should be included into its abstraction layer so that
>>> accellerators can hook in even more cleanly.
>>>
>> We've merged KVM support (although not QEMUAccel), so perhaps you can
>> also add the KVM hooks in this series that you are thinking of?
>
> I will check how much of my kvm patches for guest debugging can already
> be ported over.
>
> That topic is more complex - if you recall my according series - as kvm
> requires kernel changes to gain full guest debugging support. I'm not
> even sure if it makes sense to add support for the current kernel
> interface as it is too restricted (no watchpoints, only 4 breakpoints).
> However, will re-check if some patch re-ordering may help the migration.
>
I had a closer look meanwhile. Given the fact that there are no kvm bits
for supporting guest debugging in qemu yet and that it would take some
additional effort for me to establish this for the old interface, I
would like to skip this step and suggest a different roadmap instead:
o integrate plain qemu bits for full guest debugging (this series)
o wait for Avi to merge them into kvm-userspace
o rebase my kvm guest debugging series over kvm-userspace
(kernel bits are already up to date) and submit it for review/merge
o port stabilized kvm guest debugging over to qemu, but demanding the
new kernel ABI
OK? Time is a scarce resource, so I would really like to omit
stabilizing a feature based on an outdated interface to a still too
young (for daily use, including guest debugging) kvm support of qemu.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 03/12] Refactor and enhance break/watchpoint API
2008-11-15 8:29 ` Jan Kiszka
@ 2008-11-15 16:12 ` Anthony Liguori
0 siblings, 0 replies; 28+ messages in thread
From: Anthony Liguori @ 2008-11-15 16:12 UTC (permalink / raw)
To: qemu-devel
Jan Kiszka wrote:
> Jan Kiszka wrote:
>
>> Anthony Liguori wrote:
>>
>>> Jan Kiszka wrote:
>>>
>>>> This patch prepares the QEMU cpu_watchpoint/breakpoint API to allow the
>>>> succeeding enhancements this series comes with.
>>>>
>>>> First of all, it overcomes MAX_BREAKPOINTS/MAX_WATCHPOINTS by switching
>>>> to dynamically allocated data structures that are kept in linked lists.
>>>> This also allows to return a stable reference to the related objects,
>>>> required for later introduced x86 debug register support.
>>>>
>>>> Breakpoints and watchpoints are stored with their full information set
>>>> and an additional flag field that makes them easily extensible for use
>>>> beyond pure guest debugging.
>>>>
>>>> Finally, this restructuring lays the foundation for KVM to hook into
>>>> the debugging infrastructure, providing its own services where hardware
>>>> virtualization demands it. Once QEMUAccel is considered for merge,
>>>> those entry point should be included into its abstraction layer so that
>>>> accellerators can hook in even more cleanly.
>>>>
>>>>
>>> We've merged KVM support (although not QEMUAccel), so perhaps you can
>>> also add the KVM hooks in this series that you are thinking of?
>>>
>> I will check how much of my kvm patches for guest debugging can already
>> be ported over.
>>
>> That topic is more complex - if you recall my according series - as kvm
>> requires kernel changes to gain full guest debugging support. I'm not
>> even sure if it makes sense to add support for the current kernel
>> interface as it is too restricted (no watchpoints, only 4 breakpoints).
>> However, will re-check if some patch re-ordering may help the migration.
>>
>>
>
> I had a closer look meanwhile. Given the fact that there are no kvm bits
> for supporting guest debugging in qemu yet and that it would take some
> additional effort for me to establish this for the old interface, I
> would like to skip this step and suggest a different roadmap instead:
>
> o integrate plain qemu bits for full guest debugging (this series)
> o wait for Avi to merge them into kvm-userspace
> o rebase my kvm guest debugging series over kvm-userspace
> (kernel bits are already up to date) and submit it for review/merge
> o port stabilized kvm guest debugging over to qemu, but demanding the
> new kernel ABI
>
Yeah, that's fine.
Regards,
ANthony Liguori
> OK? Time is a scarce resource, so I would really like to omit
> stabilizing a feature based on an outdated interface to a still too
> young (for daily use, including guest debugging) kvm support of qemu.
>
> Jan
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH 04/12] Set mem_io_vaddr on io_read
2008-11-03 10:35 [Qemu-devel] [PATCH 00/12] Enhance debugging support - 4th take Jan Kiszka
` (2 preceding siblings ...)
2008-11-03 10:36 ` [Qemu-devel] [PATCH 03/12] Refactor and enhance break/watchpoint API Jan Kiszka
@ 2008-11-03 10:36 ` Jan Kiszka
2008-11-03 10:36 ` [Qemu-devel] [PATCH 05/12] Respect length of watchpoints Jan Kiszka
` (9 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Jan Kiszka @ 2008-11-03 10:36 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
Analogously to write accesses, we have to save the memory address also
on read accesses in order to support read watchpoints.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
softmmu_template.h | 1 +
1 file changed, 1 insertion(+)
Index: b/softmmu_template.h
===================================================================
--- a/softmmu_template.h
+++ b/softmmu_template.h
@@ -64,6 +64,7 @@ static inline DATA_TYPE glue(io_read, SU
cpu_io_recompile(env, retaddr);
}
+ env->mem_io_vaddr = addr;
#if SHIFT <= 2
res = io_mem_read[index][SHIFT](io_mem_opaque[index], physaddr);
#else
^ permalink raw reply [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH 05/12] Respect length of watchpoints
2008-11-03 10:35 [Qemu-devel] [PATCH 00/12] Enhance debugging support - 4th take Jan Kiszka
` (3 preceding siblings ...)
2008-11-03 10:36 ` [Qemu-devel] [PATCH 04/12] Set mem_io_vaddr on io_read Jan Kiszka
@ 2008-11-03 10:36 ` Jan Kiszka
2008-11-03 10:36 ` [Qemu-devel] [PATCH 06/12] Restore pc on watchpoint hits Jan Kiszka
` (8 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Jan Kiszka @ 2008-11-03 10:36 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
This adds length support for watchpoints. To keep things simple, only
aligned watchpoints are accepted.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
exec.c | 30 ++++++++++++++++++++----------
1 file changed, 20 insertions(+), 10 deletions(-)
Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -1295,14 +1295,21 @@ static void breakpoint_invalidate(CPUSta
int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
int flags, CPUWatchpoint **watchpoint)
{
+ target_ulong len_mask = ~(len - 1);
CPUWatchpoint *wp;
+ /* sanity checks: allow power-of-2 lengths, deny unaligned watchpoints */
+ if ((len != 1 && len != 2 && len != 4 && len != 8) || (addr & ~len_mask)) {
+ fprintf(stderr, "qemu: tried to set invalid watchpoint at "
+ TARGET_FMT_lx ", len=" TARGET_FMT_lu "\n", addr, len);
+ return -EINVAL;
+ }
wp = qemu_malloc(sizeof(*wp));
if (!wp)
return -ENOBUFS;
wp->vaddr = addr;
- wp->len_mask = 0;
+ wp->len_mask = len_mask;
wp->flags = flags;
wp->next = env->watchpoints;
@@ -1326,10 +1333,12 @@ int cpu_watchpoint_insert(CPUState *env,
int cpu_watchpoint_remove(CPUState *env, target_ulong addr, target_ulong len,
int flags)
{
+ target_ulong len_mask = ~(len - 1);
CPUWatchpoint *wp;
for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
- if (addr == wp->vaddr && flags == wp->flags) {
+ if (addr == wp->vaddr && len_mask == wp->len_mask
+ && flags == wp->flags) {
cpu_watchpoint_remove_by_ref(env, wp);
return 0;
}
@@ -2485,7 +2494,7 @@ static CPUWriteMemoryFunc *notdirty_mem_
};
/* Generate a debug exception if a watchpoint has been hit. */
-static void check_watchpoint(int offset, int flags)
+static void check_watchpoint(int offset, int len_mask, int flags)
{
CPUState *env = cpu_single_env;
target_ulong vaddr;
@@ -2493,7 +2502,8 @@ static void check_watchpoint(int offset,
vaddr = (env->mem_io_vaddr & TARGET_PAGE_MASK) + offset;
for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
- if (vaddr == wp->vaddr && (wp->flags & flags)) {
+ if ((vaddr == (wp->vaddr & len_mask) ||
+ (vaddr & wp->len_mask) == wp->vaddr) && (wp->flags & flags)) {
env->watchpoint_hit = wp;
cpu_interrupt(env, CPU_INTERRUPT_DEBUG);
break;
@@ -2506,40 +2516,40 @@ static void check_watchpoint(int offset,
phys routines. */
static uint32_t watch_mem_readb(void *opaque, target_phys_addr_t addr)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x0, BP_MEM_READ);
return ldub_phys(addr);
}
static uint32_t watch_mem_readw(void *opaque, target_phys_addr_t addr)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x1, BP_MEM_READ);
return lduw_phys(addr);
}
static uint32_t watch_mem_readl(void *opaque, target_phys_addr_t addr)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x3, BP_MEM_READ);
return ldl_phys(addr);
}
static void watch_mem_writeb(void *opaque, target_phys_addr_t addr,
uint32_t val)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x0, BP_MEM_WRITE);
stb_phys(addr, val);
}
static void watch_mem_writew(void *opaque, target_phys_addr_t addr,
uint32_t val)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x1, BP_MEM_WRITE);
stw_phys(addr, val);
}
static void watch_mem_writel(void *opaque, target_phys_addr_t addr,
uint32_t val)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x3, BP_MEM_WRITE);
stl_phys(addr, val);
}
^ permalink raw reply [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH 06/12] Restore pc on watchpoint hits
2008-11-03 10:35 [Qemu-devel] [PATCH 00/12] Enhance debugging support - 4th take Jan Kiszka
` (4 preceding siblings ...)
2008-11-03 10:36 ` [Qemu-devel] [PATCH 05/12] Respect length of watchpoints Jan Kiszka
@ 2008-11-03 10:36 ` Jan Kiszka
2008-11-03 10:36 ` [Qemu-devel] [PATCH 07/12] Remove premature memop TB terminations Jan Kiszka
` (7 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Jan Kiszka @ 2008-11-03 10:36 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
In order to provide accurate information about the triggering
instruction, this patch adds the required bits to restore the pc if the
access happened inside a TB. With the BP_STOP_BEFORE_ACCESS flag, the
watchpoint user can control if the debug trap should be issued on or
after the accessing instruction.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
cpu-all.h | 1 +
exec.c | 24 ++++++++++++++++++++++--
2 files changed, 23 insertions(+), 2 deletions(-)
Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -2497,16 +2497,36 @@ static CPUWriteMemoryFunc *notdirty_mem_
static void check_watchpoint(int offset, int len_mask, int flags)
{
CPUState *env = cpu_single_env;
+ TBCPUState cpu_state;
+ TranslationBlock *tb;
target_ulong vaddr;
CPUWatchpoint *wp;
+ if (env->watchpoint_hit) {
+ /* We re-entered the check after replacing the TB. Now raise
+ * the debug interrupt so that is will trigger after the
+ * current instruction. */
+ cpu_interrupt(env, CPU_INTERRUPT_DEBUG);
+ return;
+ }
vaddr = (env->mem_io_vaddr & TARGET_PAGE_MASK) + offset;
for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
if ((vaddr == (wp->vaddr & len_mask) ||
(vaddr & wp->len_mask) == wp->vaddr) && (wp->flags & flags)) {
env->watchpoint_hit = wp;
- cpu_interrupt(env, CPU_INTERRUPT_DEBUG);
- break;
+ tb = tb_find_pc(env->mem_io_pc);
+ if (!tb) {
+ cpu_abort(env, "check_watchpoint: could not find TB for pc=%p",
+ (void *)env->mem_io_pc);
+ }
+ cpu_restore_state(tb, env, env->mem_io_pc, NULL);
+ tb_phys_invalidate(tb, -1);
+ if (wp->flags & BP_STOP_BEFORE_ACCESS)
+ env->exception_index = EXCP_DEBUG;
+ else {
+ cpu_get_tb_cpu_state(env, &cpu_state);
+ tb_gen_code(env, &cpu_state, 1);
+ } cpu_resume_from_signal(env, NULL);
}
}
}
Index: b/cpu-all.h
===================================================================
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -765,6 +765,7 @@ void cpu_reset_interrupt(CPUState *env,
#define BP_MEM_READ 0x01
#define BP_MEM_WRITE 0x02
#define BP_MEM_ACCESS (BP_MEM_READ | BP_MEM_WRITE)
+#define BP_STOP_BEFORE_ACCESS 0x04
#define BP_GDB 0x10
int cpu_breakpoint_insert(CPUState *env, target_ulong pc, int flags,
^ permalink raw reply [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH 07/12] Remove premature memop TB terminations
2008-11-03 10:35 [Qemu-devel] [PATCH 00/12] Enhance debugging support - 4th take Jan Kiszka
` (5 preceding siblings ...)
2008-11-03 10:36 ` [Qemu-devel] [PATCH 06/12] Restore pc on watchpoint hits Jan Kiszka
@ 2008-11-03 10:36 ` Jan Kiszka
2008-11-03 10:36 ` [Qemu-devel] [PATCH 08/12] qemu: gdbstub: manage CPUs as threads Jan Kiszka
` (6 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Jan Kiszka @ 2008-11-03 10:36 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
Now that we can properly restore the pc on watchpoint hits, there is no
more need for prematurely terminating TBs if watchpoints are present.
Remove all related bits.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
exec.c | 4 ----
target-arm/translate.c | 6 ------
target-m68k/translate.c | 6 ------
3 files changed, 16 deletions(-)
Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -1319,10 +1319,6 @@ int cpu_watchpoint_insert(CPUState *env,
env->watchpoints = wp;
tlb_flush_page(env, addr);
- /* FIXME: This flush is needed because of the hack to make memory ops
- terminate the TB. It can be removed once the proper IO trap and
- re-execute bits are in. */
- tb_flush(env);
if (watchpoint)
*watchpoint = wp;
Index: b/target-arm/translate.c
===================================================================
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -8715,12 +8715,6 @@ static inline void gen_intermediate_code
gen_set_label(dc->condlabel);
dc->condjmp = 0;
}
- /* Terminate the TB on memory ops if watchpoints are present. */
- /* FIXME: This should be replacd by the deterministic execution
- * IRQ raising bits. */
- if (dc->is_mem && env->watchpoints)
- break;
-
/* Translation stops when a conditional branch is enoutered.
* Otherwise the subsequent code could get translated several times.
* Also stop translation when a page boundary is reached. This
Index: b/target-m68k/translate.c
===================================================================
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -2979,12 +2979,6 @@ gen_intermediate_code_internal(CPUState
dc->insn_pc = dc->pc;
disas_m68k_insn(env, dc);
num_insns++;
-
- /* Terminate the TB on memory ops if watchpoints are present. */
- /* FIXME: This should be replaced by the deterministic execution
- * IRQ raising bits. */
- if (dc->is_mem && env->watchpoints)
- break;
} while (!dc->is_jmp && gen_opc_ptr < gen_opc_end &&
!env->singlestep_enabled &&
(pc_offset) < (TARGET_PAGE_SIZE - 32) &&
^ permalink raw reply [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH 08/12] qemu: gdbstub: manage CPUs as threads
2008-11-03 10:35 [Qemu-devel] [PATCH 00/12] Enhance debugging support - 4th take Jan Kiszka
` (6 preceding siblings ...)
2008-11-03 10:36 ` [Qemu-devel] [PATCH 07/12] Remove premature memop TB terminations Jan Kiszka
@ 2008-11-03 10:36 ` Jan Kiszka
2008-11-03 10:36 ` [Qemu-devel] [PATCH 09/12] Introduce BP_WATCHPOINT_HIT flag Jan Kiszka
` (5 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Jan Kiszka @ 2008-11-03 10:36 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
This patch enhances QEMU's built-in debugger for SMP guest debugging.
Using the thread support of the gdb remote protocol, each VCPU is mapped
on a pseudo thread and exposed to the gdb frontend. This way you can
easy switch the focus of gdb between the VCPUs and observe their states.
On breakpoint hit, the focus is automatically adjusted just as for
normal multi-threaded application under gdb control.
Furthermore, the patch propagates breakpoint and watchpoint insertions
or removals to all CPUs, not just the current one as it was the case so
far. Without this, SMP guest debugging was practically unfeasible.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
gdbstub.c | 273 +++++++++++++++++++++++++++++++++++++++++++-------------------
gdbstub.h | 1
vl.c | 1
3 files changed, 194 insertions(+), 81 deletions(-)
Index: b/gdbstub.h
===================================================================
--- a/gdbstub.h
+++ b/gdbstub.h
@@ -8,6 +8,7 @@ typedef void (*gdb_syscall_complete_cb)(
void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...);
int use_gdb_syscalls(void);
+void gdb_set_stop_cpu(CPUState *env);
#ifdef CONFIG_USER_ONLY
int gdb_handlesig (CPUState *, int);
void gdb_exit(CPUState *, int);
Index: b/vl.c
===================================================================
--- a/vl.c
+++ b/vl.c
@@ -4595,6 +4595,7 @@ static int main_loop(void)
ret = EXCP_INTERRUPT;
}
if (unlikely(ret == EXCP_DEBUG)) {
+ gdb_set_stop_cpu(cur_cpu);
vm_stop(EXCP_DEBUG);
}
/* If all cpus are halted then wait until the next IRQ */
Index: b/gdbstub.c
===================================================================
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -69,7 +69,9 @@ enum RSState {
RS_SYSCALL,
};
typedef struct GDBState {
- CPUState *env; /* current CPU */
+ CPUState *c_cpu; /* current CPU for step/continue ops */
+ CPUState *g_cpu; /* current CPU for other ops */
+ CPUState *query_cpu; /* for q{f|s}ThreadInfo */
enum RSState state; /* parsing state */
char line_buf[MAX_PACKET_LENGTH];
int line_buf_index;
@@ -90,6 +92,8 @@ typedef struct GDBState {
*/
static int sstep_flags = SSTEP_ENABLE|SSTEP_NOIRQ|SSTEP_NOTIMER;
+static GDBState *gdbserver_state;
+
/* This is an ugly hack to cope with both new and old gdb.
If gdb sends qXfer:features:read then assume we're talking to a newish
gdb that understands target descriptions. */
@@ -99,9 +103,6 @@ static int gdb_has_xml;
/* XXX: This is not thread safe. Do we care? */
static int gdbserver_fd = -1;
-/* XXX: remove this hack. */
-static GDBState gdbserver_state;
-
static int get_char(GDBState *s)
{
uint8_t ch;
@@ -126,8 +127,6 @@ static int get_char(GDBState *s)
}
#endif
-/* GDB stub state for use by semihosting syscalls. */
-static GDBState *gdb_syscall_state;
static gdb_syscall_complete_cb gdb_current_syscall_cb;
enum {
@@ -141,8 +140,8 @@ enum {
int use_gdb_syscalls(void)
{
if (gdb_syscall_mode == GDB_SYS_UNKNOWN) {
- gdb_syscall_mode = (gdb_syscall_state ? GDB_SYS_ENABLED
- : GDB_SYS_DISABLED);
+ gdb_syscall_mode = (gdbserver_state ? GDB_SYS_ENABLED
+ : GDB_SYS_DISABLED);
}
return gdb_syscall_mode == GDB_SYS_ENABLED;
}
@@ -1031,7 +1030,7 @@ static int memtox(char *buf, const char
return p - buf;
}
-const char *get_feature_xml(CPUState *env, const char *p, const char **newp)
+const char *get_feature_xml(const char *p, const char **newp)
{
extern const char *const xml_builtin[][2];
size_t len;
@@ -1057,7 +1056,7 @@ const char *get_feature_xml(CPUState *en
"<xi:include href=\"%s\"/>",
GDB_CORE_XML);
- for (r = env->gdb_regs; r; r = r->next) {
+ for (r = first_cpu->gdb_regs; r; r = r->next) {
strcat(target_xml, "<xi:include href=\"");
strcat(target_xml, r->xml);
strcat(target_xml, "\"/>");
@@ -1160,49 +1159,78 @@ static const int xlat_gdb_type[] = {
};
#endif
-static int gdb_breakpoint_insert(CPUState *env, target_ulong addr,
- target_ulong len, int type)
+static int gdb_breakpoint_insert(target_ulong addr, target_ulong len, int type)
{
+ CPUState *env;
+ int err = 0;
+
switch (type) {
case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
- return cpu_breakpoint_insert(env, addr, BP_GDB, NULL);
+ for (env = first_cpu; env != NULL; env = env->next_cpu) {
+ err = cpu_breakpoint_insert(env, addr, BP_GDB, NULL);
+ if (err)
+ break;
+ }
+ return err;
#ifndef CONFIG_USER_ONLY
case GDB_WATCHPOINT_WRITE ... GDB_WATCHPOINT_ACCESS:
- return cpu_watchpoint_insert(env, addr, len, xlat_gdb_type[type],
- NULL);
+ for (env = first_cpu; env != NULL; env = env->next_cpu) {
+ err = cpu_watchpoint_insert(env, addr, len, xlat_gdb_type[type],
+ NULL);
+ if (err)
+ break;
+ }
+ return err;
#endif
default:
return -ENOSYS;
}
}
-static int gdb_breakpoint_remove(CPUState *env, target_ulong addr,
- target_ulong len, int type)
+static int gdb_breakpoint_remove(target_ulong addr, target_ulong len, int type)
{
+ CPUState *env;
+ int err = 0;
+
switch (type) {
case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
- return cpu_breakpoint_remove(env, addr, BP_GDB);
+ for (env = first_cpu; env != NULL; env = env->next_cpu) {
+ err = cpu_breakpoint_remove(env, addr, BP_GDB);
+ if (err)
+ break;
+ }
+ return err;
#ifndef CONFIG_USER_ONLY
case GDB_WATCHPOINT_WRITE ... GDB_WATCHPOINT_ACCESS:
- return cpu_watchpoint_remove(env, addr, len, xlat_gdb_type[type]);
+ for (env = first_cpu; env != NULL; env = env->next_cpu) {
+ err = cpu_watchpoint_remove(env, addr, len, xlat_gdb_type[type]);
+ if (err)
+ break;
+ }
+ return err;
#endif
default:
return -ENOSYS;
}
}
-static void gdb_breakpoint_remove_all(CPUState *env)
+static void gdb_breakpoint_remove_all(void)
{
- cpu_breakpoint_remove_all(env, BP_GDB);
+ CPUState *env;
+
+ for (env = first_cpu; env != NULL; env = env->next_cpu) {
+ cpu_breakpoint_remove_all(env, BP_GDB);
#ifndef CONFIG_USER_ONLY
- cpu_watchpoint_remove_all(env, BP_GDB);
+ cpu_watchpoint_remove_all(env, BP_GDB);
#endif
+ }
}
-static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
+static int gdb_handle_packet(GDBState *s, const char *line_buf)
{
+ CPUState *env;
const char *p;
- int ch, reg_size, type, res;
+ int ch, reg_size, type, res, thread;
char buf[MAX_PACKET_LENGTH];
uint8_t mem_buf[MAX_PACKET_LENGTH];
uint8_t *registers;
@@ -1222,26 +1250,26 @@ static int gdb_handle_packet(GDBState *s
* because gdb is doing and initial connect and the state
* should be cleaned up.
*/
- gdb_breakpoint_remove_all(env);
+ gdb_breakpoint_remove_all();
break;
case 'c':
if (*p != '\0') {
addr = strtoull(p, (char **)&p, 16);
#if defined(TARGET_I386)
- env->eip = addr;
+ s->c_cpu->eip = addr;
#elif defined (TARGET_PPC)
- env->nip = addr;
+ s->c_cpu->nip = addr;
#elif defined (TARGET_SPARC)
- env->pc = addr;
- env->npc = addr + 4;
+ s->c_cpu->pc = addr;
+ s->c_cpu->npc = addr + 4;
#elif defined (TARGET_ARM)
- env->regs[15] = addr;
+ s->c_cpu->regs[15] = addr;
#elif defined (TARGET_SH4)
- env->pc = addr;
+ s->c_cpu->pc = addr;
#elif defined (TARGET_MIPS)
- env->active_tc.PC = addr;
+ s->c_cpu->active_tc.PC = addr;
#elif defined (TARGET_CRIS)
- env->pc = addr;
+ s->c_cpu->pc = addr;
#endif
}
gdb_continue(s);
@@ -1256,7 +1284,7 @@ static int gdb_handle_packet(GDBState *s
exit(0);
case 'D':
/* Detach packet */
- gdb_breakpoint_remove_all(env);
+ gdb_breakpoint_remove_all();
gdb_continue(s);
put_packet(s, "OK");
break;
@@ -1264,23 +1292,23 @@ static int gdb_handle_packet(GDBState *s
if (*p != '\0') {
addr = strtoull(p, (char **)&p, 16);
#if defined(TARGET_I386)
- env->eip = addr;
+ s->c_cpu->eip = addr;
#elif defined (TARGET_PPC)
- env->nip = addr;
+ s->c_cpu->nip = addr;
#elif defined (TARGET_SPARC)
- env->pc = addr;
- env->npc = addr + 4;
+ s->c_cpu->pc = addr;
+ s->c_cpu->npc = addr + 4;
#elif defined (TARGET_ARM)
- env->regs[15] = addr;
+ s->c_cpu->regs[15] = addr;
#elif defined (TARGET_SH4)
- env->pc = addr;
+ s->c_cpu->pc = addr;
#elif defined (TARGET_MIPS)
- env->active_tc.PC = addr;
+ s->c_cpu->active_tc.PC = addr;
#elif defined (TARGET_CRIS)
- env->pc = addr;
+ s->c_cpu->pc = addr;
#endif
}
- cpu_single_step(env, sstep_flags);
+ cpu_single_step(s->c_cpu, sstep_flags);
gdb_continue(s);
return RS_IDLE;
case 'F':
@@ -1299,7 +1327,7 @@ static int gdb_handle_packet(GDBState *s
p++;
type = *p;
if (gdb_current_syscall_cb)
- gdb_current_syscall_cb(s->env, ret, err);
+ gdb_current_syscall_cb(s->c_cpu, ret, err);
if (type == 'C') {
put_packet(s, "T02");
} else {
@@ -1310,7 +1338,7 @@ static int gdb_handle_packet(GDBState *s
case 'g':
len = 0;
for (addr = 0; addr < num_g_regs; addr++) {
- reg_size = gdb_read_register(env, mem_buf + len, addr);
+ reg_size = gdb_read_register(s->g_cpu, mem_buf + len, addr);
len += reg_size;
}
memtohex(buf, mem_buf, len);
@@ -1321,7 +1349,7 @@ static int gdb_handle_packet(GDBState *s
len = strlen(p) / 2;
hextomem((uint8_t *)registers, p, len);
for (addr = 0; addr < num_g_regs && len > 0; addr++) {
- reg_size = gdb_write_register(env, registers, addr);
+ reg_size = gdb_write_register(s->g_cpu, registers, addr);
len -= reg_size;
registers += reg_size;
}
@@ -1332,7 +1360,7 @@ static int gdb_handle_packet(GDBState *s
if (*p == ',')
p++;
len = strtoull(p, NULL, 16);
- if (cpu_memory_rw_debug(env, addr, mem_buf, len, 0) != 0) {
+ if (cpu_memory_rw_debug(s->g_cpu, addr, mem_buf, len, 0) != 0) {
put_packet (s, "E14");
} else {
memtohex(buf, mem_buf, len);
@@ -1347,7 +1375,7 @@ static int gdb_handle_packet(GDBState *s
if (*p == ':')
p++;
hextomem(mem_buf, p, len);
- if (cpu_memory_rw_debug(env, addr, mem_buf, len, 1) != 0)
+ if (cpu_memory_rw_debug(s->g_cpu, addr, mem_buf, len, 1) != 0)
put_packet(s, "E14");
else
put_packet(s, "OK");
@@ -1359,7 +1387,7 @@ static int gdb_handle_packet(GDBState *s
if (!gdb_has_xml)
goto unknown_command;
addr = strtoull(p, (char **)&p, 16);
- reg_size = gdb_read_register(env, mem_buf, addr);
+ reg_size = gdb_read_register(s->g_cpu, mem_buf, addr);
if (reg_size) {
memtohex(buf, mem_buf, reg_size);
put_packet(s, buf);
@@ -1375,7 +1403,7 @@ static int gdb_handle_packet(GDBState *s
p++;
reg_size = strlen(p) / 2;
hextomem(mem_buf, p, reg_size);
- gdb_write_register(env, mem_buf, addr);
+ gdb_write_register(s->g_cpu, mem_buf, addr);
put_packet(s, "OK");
break;
case 'Z':
@@ -1388,9 +1416,9 @@ static int gdb_handle_packet(GDBState *s
p++;
len = strtoull(p, (char **)&p, 16);
if (ch == 'Z')
- res = gdb_breakpoint_insert(env, addr, len, type);
+ res = gdb_breakpoint_insert(addr, len, type);
else
- res = gdb_breakpoint_remove(env, addr, len, type);
+ res = gdb_breakpoint_remove(addr, len, type);
if (res >= 0)
put_packet(s, "OK");
else if (res == -ENOSYS)
@@ -1398,6 +1426,45 @@ static int gdb_handle_packet(GDBState *s
else
put_packet(s, "E22");
break;
+ case 'H':
+ type = *p++;
+ thread = strtoull(p, (char **)&p, 16);
+ if (thread == -1 || thread == 0) {
+ put_packet(s, "OK");
+ break;
+ }
+ for (env = first_cpu; env != NULL; env = env->next_cpu)
+ if (env->cpu_index + 1 == thread)
+ break;
+ if (env == NULL) {
+ put_packet(s, "E22");
+ break;
+ }
+ switch (type) {
+ case 'c':
+ s->c_cpu = env;
+ put_packet(s, "OK");
+ break;
+ case 'g':
+ s->g_cpu = env;
+ put_packet(s, "OK");
+ break;
+ default:
+ put_packet(s, "E22");
+ break;
+ }
+ break;
+ case 'T':
+ thread = strtoull(p, (char **)&p, 16);
+#ifndef CONFIG_USER_ONLY
+ if (thread > 0 && thread < smp_cpus + 1)
+#else
+ if (thread == 1)
+#endif
+ put_packet(s, "OK");
+ else
+ put_packet(s, "E22");
+ break;
case 'q':
case 'Q':
/* parse any 'q' packets here */
@@ -1423,10 +1490,39 @@ static int gdb_handle_packet(GDBState *s
sstep_flags = type;
put_packet(s, "OK");
break;
+ } else if (strcmp(p,"C") == 0) {
+ /* "Current thread" remains vague in the spec, so always return
+ * the first CPU (gdb returns the first thread). */
+ put_packet(s, "QC1");
+ break;
+ } else if (strcmp(p,"fThreadInfo") == 0) {
+ s->query_cpu = first_cpu;
+ goto report_cpuinfo;
+ } else if (strcmp(p,"sThreadInfo") == 0) {
+ report_cpuinfo:
+ if (s->query_cpu) {
+ snprintf(buf, sizeof(buf), "m%x", s->query_cpu->cpu_index+1);
+ put_packet(s, buf);
+ s->query_cpu = s->query_cpu->next_cpu;
+ } else
+ put_packet(s, "l");
+ break;
+ } else if (strncmp(p,"ThreadExtraInfo,", 16) == 0) {
+ thread = strtoull(p+16, (char **)&p, 16);
+ for (env = first_cpu; env != NULL; env = env->next_cpu)
+ if (env->cpu_index + 1 == thread) {
+ len = snprintf((char *)mem_buf, sizeof(mem_buf),
+ "CPU#%d [%s]", env->cpu_index,
+ env->halted ? "halted " : "running");
+ memtohex(buf, mem_buf, len);
+ put_packet(s, buf);
+ break;
+ }
+ break;
}
#ifdef CONFIG_LINUX_USER
else if (strncmp(p, "Offsets", 7) == 0) {
- TaskState *ts = env->opaque;
+ TaskState *ts = s->c_cpu->opaque;
snprintf(buf, sizeof(buf),
"Text=" TARGET_ABI_FMT_lx ";Data=" TARGET_ABI_FMT_lx
@@ -1453,7 +1549,7 @@ static int gdb_handle_packet(GDBState *s
gdb_has_xml = 1;
p += 19;
- xml = get_feature_xml(env, p, &p);
+ xml = get_feature_xml(p, &p);
if (!xml) {
snprintf(buf, sizeof(buf), "E00");
put_packet(s, buf);
@@ -1501,10 +1597,17 @@ static int gdb_handle_packet(GDBState *s
extern void tb_flush(CPUState *env);
+void gdb_set_stop_cpu(CPUState *env)
+{
+ gdbserver_state->c_cpu = env;
+ gdbserver_state->g_cpu = env;
+}
+
#ifndef CONFIG_USER_ONLY
static void gdb_vm_stopped(void *opaque, int reason)
{
- GDBState *s = opaque;
+ GDBState *s = gdbserver_state;
+ CPUState *env = s->c_cpu;
char buf[256];
const char *type;
int ret;
@@ -1513,11 +1616,11 @@ static void gdb_vm_stopped(void *opaque,
return;
/* disable single step if it was enable */
- cpu_single_step(s->env, 0);
+ cpu_single_step(env, 0);
if (reason == EXCP_DEBUG) {
- if (s->env->watchpoint_hit) {
- switch (s->env->watchpoint_hit->flags & BP_MEM_ACCESS) {
+ if (env->watchpoint_hit) {
+ switch (env->watchpoint_hit->flags & BP_MEM_ACCESS) {
case BP_MEM_READ:
type = "r";
break;
@@ -1528,20 +1631,22 @@ static void gdb_vm_stopped(void *opaque,
type = "";
break;
}
- snprintf(buf, sizeof(buf), "T%02x%swatch:" TARGET_FMT_lx ";",
- SIGTRAP, type, s->env->watchpoint_hit->vaddr);
+ snprintf(buf, sizeof(buf),
+ "T%02xthread:%02x;%swatch:" TARGET_FMT_lx ";",
+ SIGTRAP, env->cpu_index+1, type,
+ env->watchpoint_hit->vaddr);
put_packet(s, buf);
- s->env->watchpoint_hit = NULL;
+ env->watchpoint_hit = NULL;
return;
}
- tb_flush(s->env);
+ tb_flush(env);
ret = SIGTRAP;
} else if (reason == EXCP_INTERRUPT) {
ret = SIGINT;
} else {
ret = 0;
}
- snprintf(buf, sizeof(buf), "S%02x", ret);
+ snprintf(buf, sizeof(buf), "T%02xthread:%02x;", ret, env->cpu_index+1);
put_packet(s, buf);
}
#endif
@@ -1560,7 +1665,7 @@ void gdb_do_syscall(gdb_syscall_complete
uint64_t i64;
GDBState *s;
- s = gdb_syscall_state;
+ s = gdbserver_state;
if (!s)
return;
gdb_current_syscall_cb = cb;
@@ -1605,15 +1710,14 @@ void gdb_do_syscall(gdb_syscall_complete
va_end(va);
put_packet(s, buf);
#ifdef CONFIG_USER_ONLY
- gdb_handlesig(s->env, 0);
+ gdb_handlesig(s->c_cpu, 0);
#else
- cpu_interrupt(s->env, CPU_INTERRUPT_EXIT);
+ cpu_interrupt(s->c_cpu, CPU_INTERRUPT_EXIT);
#endif
}
static void gdb_read_byte(GDBState *s, int ch)
{
- CPUState *env = s->env;
int i, csum;
uint8_t reply;
@@ -1679,7 +1783,7 @@ static void gdb_read_byte(GDBState *s, i
} else {
reply = '+';
put_buffer(s, &reply, 1);
- s->state = gdb_handle_packet(s, env, s->line_buf);
+ s->state = gdb_handle_packet(s, s->line_buf);
}
break;
default:
@@ -1696,7 +1800,7 @@ gdb_handlesig (CPUState *env, int sig)
char buf[256];
int n;
- s = &gdbserver_state;
+ s = gdbserver_state;
if (gdbserver_fd < 0 || s->fd < 0)
return sig;
@@ -1744,7 +1848,7 @@ void gdb_exit(CPUState *env, int code)
GDBState *s;
char buf[4];
- s = &gdbserver_state;
+ s = gdbserver_state;
if (gdbserver_fd < 0 || s->fd < 0)
return;
@@ -1753,7 +1857,7 @@ void gdb_exit(CPUState *env, int code)
}
-static void gdb_accept(void *opaque)
+static void gdb_accept(void)
{
GDBState *s;
struct sockaddr_in sockaddr;
@@ -1775,13 +1879,20 @@ static void gdb_accept(void *opaque)
val = 1;
setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (char *)&val, sizeof(val));
- s = &gdbserver_state;
+ s = qemu_mallocz(sizeof(GDBState));
+ if (!s) {
+ errno = ENOMEM;
+ perror("accept");
+ return;
+ }
+
memset (s, 0, sizeof (GDBState));
- s->env = first_cpu; /* XXX: allow to change CPU */
+ s->c_cpu = first_cpu;
+ s->g_cpu = first_cpu;
s->fd = fd;
gdb_has_xml = 0;
- gdb_syscall_state = s;
+ gdbserver_state = s;
fcntl(fd, F_SETFL, O_NONBLOCK);
}
@@ -1823,7 +1934,7 @@ int gdbserver_start(int port)
if (gdbserver_fd < 0)
return -1;
/* accept connections */
- gdb_accept (NULL);
+ gdb_accept();
return 0;
}
#else
@@ -1836,11 +1947,10 @@ static int gdb_chr_can_receive(void *opa
static void gdb_chr_receive(void *opaque, const uint8_t *buf, int size)
{
- GDBState *s = opaque;
int i;
for (i = 0; i < size; i++) {
- gdb_read_byte(s, buf[i]);
+ gdb_read_byte(gdbserver_state, buf[i]);
}
}
@@ -1849,7 +1959,6 @@ static void gdb_chr_event(void *opaque,
switch (event) {
case CHR_EVENT_RESET:
vm_stop(EXCP_INTERRUPT);
- gdb_syscall_state = opaque;
gdb_has_xml = 0;
break;
default:
@@ -1884,11 +1993,13 @@ int gdbserver_start(const char *port)
if (!s) {
return -1;
}
- s->env = first_cpu; /* XXX: allow to change CPU */
+ s->c_cpu = first_cpu;
+ s->g_cpu = first_cpu;
s->chr = chr;
+ gdbserver_state = s;
qemu_chr_add_handlers(chr, gdb_chr_can_receive, gdb_chr_receive,
- gdb_chr_event, s);
- qemu_add_vm_stop_handler(gdb_vm_stopped, s);
+ gdb_chr_event, NULL);
+ qemu_add_vm_stop_handler(gdb_vm_stopped, NULL);
return 0;
}
#endif
^ permalink raw reply [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH 09/12] Introduce BP_WATCHPOINT_HIT flag
2008-11-03 10:35 [Qemu-devel] [PATCH 00/12] Enhance debugging support - 4th take Jan Kiszka
` (7 preceding siblings ...)
2008-11-03 10:36 ` [Qemu-devel] [PATCH 08/12] qemu: gdbstub: manage CPUs as threads Jan Kiszka
@ 2008-11-03 10:36 ` Jan Kiszka
2008-11-03 10:36 ` [Qemu-devel] [PATCH 10/12] Add debug exception hook Jan Kiszka
` (4 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Jan Kiszka @ 2008-11-03 10:36 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
When one watchpoint is hit, others might have triggered as well. To
support users of the watchpoint API which need to detect such cases,
the BP_WATCHPOINT_HIT flag is introduced and maintained.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
cpu-all.h | 1 +
cpu-exec.c | 11 +++++++++++
exec.c | 34 ++++++++++++++++++++--------------
3 files changed, 32 insertions(+), 14 deletions(-)
Index: b/cpu-all.h
===================================================================
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -766,6 +766,7 @@ void cpu_reset_interrupt(CPUState *env,
#define BP_MEM_WRITE 0x02
#define BP_MEM_ACCESS (BP_MEM_READ | BP_MEM_WRITE)
#define BP_STOP_BEFORE_ACCESS 0x04
+#define BP_WATCHPOINT_HIT 0x08
#define BP_GDB 0x10
int cpu_breakpoint_insert(CPUState *env, target_ulong pc, int flags,
Index: b/cpu-exec.c
===================================================================
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -181,6 +181,15 @@ static inline TranslationBlock *tb_find_
return tb;
}
+static void cpu_handle_debug_exception(CPUState *env)
+{
+ CPUWatchpoint *wp;
+
+ if (!env->watchpoint_hit)
+ for (wp = env->watchpoints; wp != NULL; wp = wp->next)
+ wp->flags &= ~BP_WATCHPOINT_HIT;
+}
+
/* main execution loop */
int cpu_exec(CPUState *env1)
@@ -235,6 +244,8 @@ int cpu_exec(CPUState *env1)
if (env->exception_index >= EXCP_INTERRUPT) {
/* exit request from the cpu execution loop */
ret = env->exception_index;
+ if (ret == EXCP_DEBUG)
+ cpu_handle_debug_exception(env);
break;
} else if (env->user_mode_only) {
/* if user mode only, we simulate a fake exception
Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -1334,7 +1334,7 @@ int cpu_watchpoint_remove(CPUState *env,
for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
if (addr == wp->vaddr && len_mask == wp->len_mask
- && flags == wp->flags) {
+ && flags == (wp->flags & ~BP_WATCHPOINT_HIT)) {
cpu_watchpoint_remove_by_ref(env, wp);
return 0;
}
@@ -2509,20 +2509,26 @@ static void check_watchpoint(int offset,
for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
if ((vaddr == (wp->vaddr & len_mask) ||
(vaddr & wp->len_mask) == wp->vaddr) && (wp->flags & flags)) {
- env->watchpoint_hit = wp;
- tb = tb_find_pc(env->mem_io_pc);
- if (!tb) {
- cpu_abort(env, "check_watchpoint: could not find TB for pc=%p",
- (void *)env->mem_io_pc);
+ wp->flags |= BP_WATCHPOINT_HIT;
+ if (!env->watchpoint_hit) {
+ env->watchpoint_hit = wp;
+ tb = tb_find_pc(env->mem_io_pc);
+ if (!tb) {
+ cpu_abort(env, "check_watchpoint: could not find TB for "
+ "pc=%p", (void *)env->mem_io_pc);
+ }
+ cpu_restore_state(tb, env, env->mem_io_pc, NULL);
+ tb_phys_invalidate(tb, -1);
+ if (wp->flags & BP_STOP_BEFORE_ACCESS)
+ env->exception_index = EXCP_DEBUG;
+ else {
+ cpu_get_tb_cpu_state(env, &cpu_state);
+ tb_gen_code(env, &cpu_state, 1);
+ }
+ cpu_resume_from_signal(env, NULL);
}
- cpu_restore_state(tb, env, env->mem_io_pc, NULL);
- tb_phys_invalidate(tb, -1);
- if (wp->flags & BP_STOP_BEFORE_ACCESS)
- env->exception_index = EXCP_DEBUG;
- else {
- cpu_get_tb_cpu_state(env, &cpu_state);
- tb_gen_code(env, &cpu_state, 1);
- } cpu_resume_from_signal(env, NULL);
+ } else {
+ wp->flags &= ~BP_WATCHPOINT_HIT;
}
}
}
^ permalink raw reply [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH 10/12] Add debug exception hook
2008-11-03 10:35 [Qemu-devel] [PATCH 00/12] Enhance debugging support - 4th take Jan Kiszka
` (8 preceding siblings ...)
2008-11-03 10:36 ` [Qemu-devel] [PATCH 09/12] Introduce BP_WATCHPOINT_HIT flag Jan Kiszka
@ 2008-11-03 10:36 ` Jan Kiszka
2008-11-03 10:36 ` [Qemu-devel] [PATCH 11/12] Introduce BP_CPU as a breakpoint type Jan Kiszka
` (3 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Jan Kiszka @ 2008-11-03 10:36 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
This patch allows to hook into the delivery of EXCP_DEBUG so that other
use beyond guest debugging becomes possible.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
cpu-exec.c | 13 +++++++++++++
exec-all.h | 4 ++++
2 files changed, 17 insertions(+)
Index: b/cpu-exec.c
===================================================================
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -181,6 +181,16 @@ static inline TranslationBlock *tb_find_
return tb;
}
+static CPUDebugExcpHandler *debug_excp_handler;
+
+CPUDebugExcpHandler *cpu_set_debug_excp_handler(CPUDebugExcpHandler *handler)
+{
+ CPUDebugExcpHandler *old_handler = debug_excp_handler;
+
+ debug_excp_handler = handler;
+ return old_handler;
+}
+
static void cpu_handle_debug_exception(CPUState *env)
{
CPUWatchpoint *wp;
@@ -188,6 +198,9 @@ static void cpu_handle_debug_exception(C
if (!env->watchpoint_hit)
for (wp = env->watchpoints; wp != NULL; wp = wp->next)
wp->flags &= ~BP_WATCHPOINT_HIT;
+
+ if (debug_excp_handler)
+ debug_excp_handler(env);
}
/* main execution loop */
Index: b/exec-all.h
===================================================================
--- a/exec-all.h
+++ b/exec-all.h
@@ -390,4 +390,8 @@ static inline int kqemu_is_ok(CPUState *
}
#endif
+
+typedef void (CPUDebugExcpHandler)(CPUState *env);
+
+CPUDebugExcpHandler *cpu_set_debug_excp_handler(CPUDebugExcpHandler *handler);
#endif
^ permalink raw reply [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH 11/12] Introduce BP_CPU as a breakpoint type
2008-11-03 10:35 [Qemu-devel] [PATCH 00/12] Enhance debugging support - 4th take Jan Kiszka
` (9 preceding siblings ...)
2008-11-03 10:36 ` [Qemu-devel] [PATCH 10/12] Add debug exception hook Jan Kiszka
@ 2008-11-03 10:36 ` Jan Kiszka
2008-11-03 10:36 ` [Qemu-devel] [PATCH 12/12] x86: Debug register emulation Jan Kiszka
` (2 subsequent siblings)
13 siblings, 0 replies; 28+ messages in thread
From: Jan Kiszka @ 2008-11-03 10:36 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
Add another breakpoint/watchpoint type to BP_GDB: BP_CPU. This type is
intended for hardware-assisted break/watchpoint emulations like the x86
architecture requires.
To keep the highest priority for BP_GDB breakpoints, this type is
always inserted at the head of break/watchpoint lists, thus is found
first when looking up the origin of a debug interruption.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
cpu-all.h | 1 +
exec.c | 46 ++++++++++++++++++++++++++++++++++++++--------
2 files changed, 39 insertions(+), 8 deletions(-)
Index: b/cpu-all.h
===================================================================
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -768,6 +768,7 @@ void cpu_reset_interrupt(CPUState *env,
#define BP_STOP_BEFORE_ACCESS 0x04
#define BP_WATCHPOINT_HIT 0x08
#define BP_GDB 0x10
+#define BP_CPU 0x20
int cpu_breakpoint_insert(CPUState *env, target_ulong pc, int flags,
CPUBreakpoint **breakpoint);
Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -1296,7 +1296,7 @@ int cpu_watchpoint_insert(CPUState *env,
int flags, CPUWatchpoint **watchpoint)
{
target_ulong len_mask = ~(len - 1);
- CPUWatchpoint *wp;
+ CPUWatchpoint *wp, *prev_wp;
/* sanity checks: allow power-of-2 lengths, deny unaligned watchpoints */
if ((len != 1 && len != 2 && len != 4 && len != 8) || (addr & ~len_mask)) {
@@ -1312,11 +1312,26 @@ int cpu_watchpoint_insert(CPUState *env,
wp->len_mask = len_mask;
wp->flags = flags;
- wp->next = env->watchpoints;
- wp->prev = NULL;
+ /* keep all GDB-injected watchpoints in front */
+ if (!(flags & BP_GDB) && env->watchpoints) {
+ prev_wp = env->watchpoints;
+ while (prev_wp->next != NULL && (prev_wp->next->flags & BP_GDB))
+ prev_wp = prev_wp->next;
+ } else {
+ prev_wp = NULL;
+ }
+
+ /* Insert new watchpoint */
+ if (prev_wp) {
+ wp->next = prev_wp->next;
+ prev_wp->next = wp;
+ } else {
+ wp->next = env->watchpoints;
+ env->watchpoints = wp;
+ }
if (wp->next)
wp->next->prev = wp;
- env->watchpoints = wp;
+ wp->prev = prev_wp;
tlb_flush_page(env, addr);
@@ -1372,7 +1387,7 @@ int cpu_breakpoint_insert(CPUState *env,
CPUBreakpoint **breakpoint)
{
#if defined(TARGET_HAS_ICE)
- CPUBreakpoint *bp;
+ CPUBreakpoint *bp, *prev_bp;
bp = qemu_malloc(sizeof(*bp));
if (!bp)
@@ -1381,11 +1396,26 @@ int cpu_breakpoint_insert(CPUState *env,
bp->pc = pc;
bp->flags = flags;
- bp->next = env->breakpoints;
- bp->prev = NULL;
+ /* keep all GDB-injected breakpoints in front */
+ if (!(flags & BP_GDB) && env->breakpoints) {
+ prev_bp = env->breakpoints;
+ while (prev_bp->next != NULL && (prev_bp->next->flags & BP_GDB))
+ prev_bp = prev_bp->next;
+ } else {
+ prev_bp = NULL;
+ }
+
+ /* Insert new breakpoint */
+ if (prev_bp) {
+ bp->next = prev_bp->next;
+ prev_bp->next = bp;
+ } else {
+ bp->next = env->breakpoints;
+ env->breakpoints = bp;
+ }
if (bp->next)
bp->next->prev = bp;
- env->breakpoints = bp;
+ bp->prev = prev_bp;
breakpoint_invalidate(env, pc);
^ permalink raw reply [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH 12/12] x86: Debug register emulation
2008-11-03 10:35 [Qemu-devel] [PATCH 00/12] Enhance debugging support - 4th take Jan Kiszka
` (10 preceding siblings ...)
2008-11-03 10:36 ` [Qemu-devel] [PATCH 11/12] Introduce BP_CPU as a breakpoint type Jan Kiszka
@ 2008-11-03 10:36 ` Jan Kiszka
2008-11-13 20:38 ` [Qemu-devel] [PATCH 00/12] Enhance debugging support - 4th take Anthony Liguori
2008-11-13 22:06 ` [Qemu-devel] " Fabrice Bellard
13 siblings, 0 replies; 28+ messages in thread
From: Jan Kiszka @ 2008-11-03 10:36 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
Built on top of previously enhanced breakpoint/watchpoint support, this
patch adds full debug register emulation for the x86 architecture.
Many corner cases were considered, and the result was successfully
tested inside a Linux guest with gdb, but I won't be surprised if one
or two scenarios still behave differently in reality.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
linux-user/main.c | 4 -
target-i386/cpu.h | 34 +++++++++++++
target-i386/helper.c | 118 ++++++++++++++++++++++++++++++++++++------------
target-i386/op_helper.c | 79 ++++++++++++++++++++++++++++++--
4 files changed, 199 insertions(+), 36 deletions(-)
Index: b/linux-user/main.c
===================================================================
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -405,7 +405,7 @@ void cpu_loop(CPUX86State *env)
queue_signal(env, info.si_signo, &info);
}
break;
- case EXCP01_SSTP:
+ case EXCP01_DB:
case EXCP03_INT3:
#ifndef TARGET_X86_64
if (env->eflags & VM_MASK) {
@@ -415,7 +415,7 @@ void cpu_loop(CPUX86State *env)
{
info.si_signo = SIGTRAP;
info.si_errno = 0;
- if (trapnr == EXCP01_SSTP) {
+ if (trapnr == EXCP01_DB) {
info.si_code = TARGET_TRAP_BRKPT;
info._sifields._sigfault._addr = env->eip;
} else {
Index: b/target-i386/cpu.h
===================================================================
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -199,6 +199,16 @@
#define CR4_OSFXSR_MASK (1 << 9)
#define CR4_OSXMMEXCPT_MASK (1 << 10)
+#define DR6_BD (1 << 13)
+#define DR6_BS (1 << 14)
+#define DR6_BT (1 << 15)
+#define DR6_FIXED_1 0xffff0ff0
+
+#define DR7_GD (1 << 13)
+#define DR7_TYPE_SHIFT 16
+#define DR7_LEN_SHIFT 18
+#define DR7_FIXED_1 0x00000400
+
#define PG_PRESENT_BIT 0
#define PG_RW_BIT 1
#define PG_USER_BIT 2
@@ -355,7 +365,7 @@
#define CPUID_MWAIT_EMX (1 << 0) /* enumeration supported */
#define EXCP00_DIVZ 0
-#define EXCP01_SSTP 1
+#define EXCP01_DB 1
#define EXCP02_NMI 2
#define EXCP03_INT3 3
#define EXCP04_INTO 4
@@ -587,6 +597,10 @@ typedef struct CPUX86State {
int exception_is_int;
target_ulong exception_next_eip;
target_ulong dr[8]; /* debug registers */
+ union {
+ CPUBreakpoint *cpu_breakpoint[4];
+ CPUWatchpoint *cpu_watchpoint[4];
+ }; /* break/watchpoints for dr[0..3] */
uint32_t smbase;
int old_exception; /* exception in flight */
@@ -774,6 +788,24 @@ static inline void cpu_clone_regs(CPUSta
}
#endif
+static inline int hw_breakpoint_enabled(unsigned long dr7, int index)
+{
+ return (dr7 >> (index * 2)) & 3;
+}
+
+static inline int hw_breakpoint_type(unsigned long dr7, int index)
+{
+ return (dr7 >> (DR7_TYPE_SHIFT + (index * 2))) & 3;
+}
+
+static inline int hw_breakpoint_len(unsigned long dr7, int index)
+{
+ int len = ((dr7 >> (DR7_LEN_SHIFT + (index * 2))) & 3);
+ return (len == 2) ? 8 : len + 1;
+}
+
+int check_hw_breakpoints(CPUState *env, int force_dr6_update);
+
#include "cpu-all.h"
#include "exec-all.h"
#include "svm.h"
Index: b/target-i386/helper.c
===================================================================
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -32,8 +32,6 @@
//#define DEBUG_MMU
-static int cpu_x86_register (CPUX86State *env, const char *cpu_model);
-
static void add_flagname_to_bitmaps(char *flagname, uint32_t *features,
uint32_t *ext_features,
uint32_t *ext2_features,
@@ -91,33 +89,6 @@ static void add_flagname_to_bitmaps(char
fprintf(stderr, "CPU feature %s not found\n", flagname);
}
-CPUX86State *cpu_x86_init(const char *cpu_model)
-{
- CPUX86State *env;
- static int inited;
-
- env = qemu_mallocz(sizeof(CPUX86State));
- if (!env)
- return NULL;
- cpu_exec_init(env);
- env->cpu_model_str = cpu_model;
-
- /* init various static tables */
- if (!inited) {
- inited = 1;
- optimize_flags_init();
- }
- if (cpu_x86_register(env, cpu_model) < 0) {
- cpu_x86_close(env);
- return NULL;
- }
- cpu_reset(env);
-#ifdef USE_KQEMU
- kqemu_init(env);
-#endif
- return env;
-}
-
typedef struct x86_def_t {
const char *name;
uint32_t level;
@@ -490,6 +461,12 @@ void cpu_reset(CPUX86State *env)
env->fpuc = 0x37f;
env->mxcsr = 0x1f80;
+
+ memset(env->dr, 0, sizeof(env->dr));
+ env->dr[6] = DR6_FIXED_1;
+ env->dr[7] = DR7_FIXED_1;
+ cpu_breakpoint_remove_all(env, BP_CPU);
+ cpu_watchpoint_remove_all(env, BP_CPU);
}
void cpu_x86_close(CPUX86State *env)
@@ -1286,4 +1263,87 @@ target_phys_addr_t cpu_get_phys_page_deb
paddr = (pte & TARGET_PAGE_MASK) + page_offset;
return paddr;
}
+
+int check_hw_breakpoints(CPUState *env, int force_dr6_update)
+{
+ target_ulong dr6;
+ int reg, type;
+ int hit_enabled = 0;
+
+ dr6 = env->dr[6] & ~0xf;
+ for (reg = 0; reg < 4; reg++) {
+ type = hw_breakpoint_type(env->dr[7], reg);
+ if ((type == 0 && env->dr[reg] == env->eip) ||
+ ((type & 1) && env->cpu_watchpoint[reg] &&
+ (env->cpu_watchpoint[reg]->flags & BP_WATCHPOINT_HIT))) {
+ dr6 |= 1 << reg;
+ if (hw_breakpoint_enabled(env->dr[7], reg))
+ hit_enabled = 1;
+ }
+ }
+ if (hit_enabled || force_dr6_update)
+ env->dr[6] = dr6;
+ return hit_enabled;
+}
+
+static CPUDebugExcpHandler *prev_debug_excp_handler;
+
+void raise_exception(int exception_index);
+
+static void breakpoint_handler(CPUState *env)
+{
+ CPUBreakpoint *bp;
+
+ if (env->watchpoint_hit) {
+ if (env->watchpoint_hit->flags & BP_CPU) {
+ env->watchpoint_hit = NULL;
+ if (check_hw_breakpoints(env, 0))
+ raise_exception(EXCP01_DB);
+ else
+ cpu_resume_from_signal(env, NULL);
+ }
+ } else {
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next)
+ if (bp->pc == env->eip) {
+ if (bp->flags & BP_CPU) {
+ check_hw_breakpoints(env, 1);
+ raise_exception(EXCP01_DB);
+ }
+ break;
+ }
+ }
+ if (prev_debug_excp_handler)
+ prev_debug_excp_handler(env);
+}
#endif /* !CONFIG_USER_ONLY */
+
+CPUX86State *cpu_x86_init(const char *cpu_model)
+{
+ CPUX86State *env;
+ static int inited;
+
+ env = qemu_mallocz(sizeof(CPUX86State));
+ if (!env)
+ return NULL;
+ cpu_exec_init(env);
+ env->cpu_model_str = cpu_model;
+
+ /* init various static stuff */
+ if (!inited) {
+ inited = 1;
+ optimize_flags_init();
+#ifndef CONFIG_USER_ONLY
+ prev_debug_excp_handler =
+ cpu_set_debug_excp_handler(breakpoint_handler);
+#endif
+ }
+ if (cpu_x86_register(env, cpu_model) < 0) {
+ cpu_x86_close(env);
+ return NULL;
+ }
+ cpu_reset(env);
+#ifdef USE_KQEMU
+ kqemu_init(env);
+#endif
+ return env;
+}
Index: b/target-i386/op_helper.c
===================================================================
--- a/target-i386/op_helper.c
+++ b/target-i386/op_helper.c
@@ -94,6 +94,53 @@ const CPU86_LDouble f15rk[7] =
3.32192809488736234781L, /*l2t*/
};
+static void hw_breakpoint_insert(int index)
+{
+ int type, err = 0;
+
+ switch (hw_breakpoint_type(env->dr[7], index)) {
+ case 0:
+ if (hw_breakpoint_enabled(env->dr[7], index))
+ err = cpu_breakpoint_insert(env, env->dr[index], BP_CPU,
+ &env->cpu_breakpoint[index]);
+ break;
+ case 1:
+ type = BP_CPU | BP_MEM_WRITE;
+ goto insert_wp;
+ case 2:
+ /* No support for I/O watchpoints yet */
+ break;
+ case 3:
+ type = BP_CPU | BP_MEM_ACCESS;
+ insert_wp:
+ err = cpu_watchpoint_insert(env, env->dr[index],
+ hw_breakpoint_len(env->dr[7], index),
+ type, &env->cpu_watchpoint[index]);
+ break;
+ }
+ if (err)
+ env->cpu_breakpoint[index] = NULL;
+}
+
+static void hw_breakpoint_remove(int index)
+{
+ if (!env->cpu_breakpoint[index])
+ return;
+ switch (hw_breakpoint_type(env->dr[7], index)) {
+ case 0:
+ if (hw_breakpoint_enabled(env->dr[7], index))
+ cpu_breakpoint_remove_by_ref(env, env->cpu_breakpoint[index]);
+ break;
+ case 1:
+ case 3:
+ cpu_watchpoint_remove_by_ref(env, env->cpu_watchpoint[index]);
+ break;
+ case 2:
+ /* No support for I/O watchpoints yet */
+ break;
+ }
+}
+
/* broken thread support */
static spinlock_t global_cpu_lock = SPIN_LOCK_UNLOCKED;
@@ -496,6 +543,15 @@ static void switch_tss(int tss_selector,
/* XXX: different exception if CALL ? */
raise_exception_err(EXCP0D_GPF, 0);
}
+
+ /* reset local breakpoints */
+ if (env->dr[7] & 0x55) {
+ for (i = 0; i < 4; i++) {
+ if (hw_breakpoint_enabled(env->dr[7], i) == 0x1)
+ hw_breakpoint_remove(i);
+ }
+ env->dr[7] &= ~0x55;
+ }
}
/* check if Port I/O is allowed in TSS */
@@ -1879,8 +1935,11 @@ void helper_cmpxchg16b(target_ulong a0)
void helper_single_step(void)
{
- env->dr[6] |= 0x4000;
- raise_exception(EXCP01_SSTP);
+#ifndef CONFIG_USER_ONLY
+ check_hw_breakpoints(env, 1);
+#endif
+ env->dr[6] |= DR6_BS;
+ raise_exception(EXCP01_DB);
}
void helper_cpuid(void)
@@ -3085,10 +3144,22 @@ void helper_clts(void)
env->hflags &= ~HF_TS_MASK;
}
-/* XXX: do more */
void helper_movl_drN_T0(int reg, target_ulong t0)
{
- env->dr[reg] = t0;
+ int i;
+
+ if (reg < 4) {
+ hw_breakpoint_remove(reg);
+ env->dr[reg] = t0;
+ hw_breakpoint_insert(reg);
+ } else if (reg == 7) {
+ for (i = 0; i < 4; i++)
+ hw_breakpoint_remove(i);
+ env->dr[7] = t0;
+ for (i = 0; i < 4; i++)
+ hw_breakpoint_insert(i);
+ } else
+ env->dr[reg] = t0;
}
void helper_invlpg(target_ulong addr)
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH 00/12] Enhance debugging support - 4th take
2008-11-03 10:35 [Qemu-devel] [PATCH 00/12] Enhance debugging support - 4th take Jan Kiszka
` (11 preceding siblings ...)
2008-11-03 10:36 ` [Qemu-devel] [PATCH 12/12] x86: Debug register emulation Jan Kiszka
@ 2008-11-13 20:38 ` Anthony Liguori
2008-11-13 22:55 ` [Qemu-devel] " Jan Kiszka
2008-11-13 22:06 ` [Qemu-devel] " Fabrice Bellard
13 siblings, 1 reply; 28+ messages in thread
From: Anthony Liguori @ 2008-11-13 20:38 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
Hi Jan,
Jan Kiszka wrote:
> Yet another round for the gdbstub and debug regs patches.
>
First, thanks for your patience with this series. Unfortunately, there
isn't a very clear target-i386 maintainer right now so this series
hasn't gotten the feedback it should have gotten. I feel a little
uneasy about committing it because I don't have complete confidence in
the impact with TCG but after reviewing it and I don't have a setup
prepared to reliably test it all, I feel pretty certain that it's not
going to blow things up.
I have some minor comments for some of the patches. Once you address
them, I'll apply the series unless there is objection before you post
the next round.
In terms of my exec.c split, I'll wait to commit the code motion until
after your series has been merged to keep rebasing easy for you.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH 00/12] Enhance debugging support - 4th take
2008-11-03 10:35 [Qemu-devel] [PATCH 00/12] Enhance debugging support - 4th take Jan Kiszka
` (12 preceding siblings ...)
2008-11-13 20:38 ` [Qemu-devel] [PATCH 00/12] Enhance debugging support - 4th take Anthony Liguori
@ 2008-11-13 22:06 ` Fabrice Bellard
2008-11-13 22:55 ` [Qemu-devel] " Jan Kiszka
13 siblings, 1 reply; 28+ messages in thread
From: Fabrice Bellard @ 2008-11-13 22:06 UTC (permalink / raw)
To: jan.kiszka; +Cc: qemu-devel
I had a quick look at the patch serie (I don't have the time to look at
it carefully). I find the patch globally acceptable, but I have two remarks:
- Patch 01/12 may introduce a performance regression due to the change
in tb_find_fast(). If gcc does not optimizes the code correctly, your
change will introduce many unneeded memory accesses and a call to
memcmp() in the fast path, which is not acceptable.
- Patch 12/12 needs improvements (load/save VM) and possibly more
analysis to see if it complies with the x86 spec, so it could be applied
later.
Regards,
Fabrice.
Jan Kiszka wrote:
> Yet another round for the gdbstub and debug regs patches.
>
> This version addresses the concerns about the next_cflags approach
> brought up by Fabrice and Glauber. Instead of this, the first patch in
> this series takes an alternative path to obtain the required parameters
> for TB generation, see its description for details.
>
> Besides rebasing the series on top of this patch and the latest changes
> in SVN, I addressed Glauber's review comments on take 3 (proper
> description for patch 4, introduce CPUWatchpoint.len_mask directly).
>
> Jan
>
> --
> Siemens AG, Corporate Technology, CT SE 2
> Corporate Competence Center Embedded Linux
>
>
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread* [Qemu-devel] Re: [PATCH 00/12] Enhance debugging support - 4th take
2008-11-13 22:06 ` [Qemu-devel] " Fabrice Bellard
@ 2008-11-13 22:55 ` Jan Kiszka
2008-11-13 23:32 ` Anthony Liguori
0 siblings, 1 reply; 28+ messages in thread
From: Jan Kiszka @ 2008-11-13 22:55 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka
Fabrice Bellard wrote:
> I had a quick look at the patch serie (I don't have the time to look at
> it carefully). I find the patch globally acceptable, but I have two remarks:
>
> - Patch 01/12 may introduce a performance regression due to the change
> in tb_find_fast(). If gcc does not optimizes the code correctly, your
> change will introduce many unneeded memory accesses and a call to
> memcmp() in the fast path, which is not acceptable.
OK, will remove the memcmp to play safe. Beyond that, there is only a
static inline cpu_get_tb_cpu_state involved in the fast path which
dereferences addresses. If that gets blown up to a real function, the
compiler is not configured for performance anyway. But I can check the
results again with older and current compilers, comparing object sizes.
> - Patch 12/12 needs improvements (load/save VM)
Good point, will check and add missing bits.
> and possibly more
> analysis to see if it complies with the x86 spec, so it could be applied
> later.
No problem (as long as "later" doesn't mean another 5 months ;) ),
Please keep in mind that this patch allows to run a lot of
debug-register-using guest code that broke so far. So, unless you or
someone else with deep x86 knowledge find issues during that code
inspection, hunting them via concrete test cases may be helpful as well.
Thanks for having a look!
Jan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 00/12] Enhance debugging support - 4th take
2008-11-13 22:55 ` [Qemu-devel] " Jan Kiszka
@ 2008-11-13 23:32 ` Anthony Liguori
0 siblings, 0 replies; 28+ messages in thread
From: Anthony Liguori @ 2008-11-13 23:32 UTC (permalink / raw)
To: qemu-devel; +Cc: jan.kiszka
Jan Kiszka wrote:
> No problem (as long as "later" doesn't mean another 5 months ;) ),
>
> Please keep in mind that this patch allows to run a lot of
> debug-register-using guest code that broke so far. So, unless you or
> someone else with deep x86 knowledge find issues during that code
> inspection, hunting them via concrete test cases may be helpful as well.
>
Yeah, I think this is reasonable. As long as the patch is well isolated
and easy to identify with a git bisect, I think it's okay.
Regards,
Anthony Liguori
> Thanks for having a look!
> Jan
>
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread