* [Qemu-devel] [PATCH v5 07/18] Restore pc on watchpoint hits
2008-11-17 16:18 [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Jan Kiszka
@ 2008-11-17 16:18 ` Jan Kiszka
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 03/18] gdbstub: Return appropriate watch message to gdb Jan Kiszka
` (17 subsequent siblings)
18 siblings, 0 replies; 37+ messages in thread
From: Jan Kiszka @ 2008-11-17 16:18 UTC (permalink / raw)
To: qemu-devel
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 | 26 ++++++++++++++++++++++++--
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/cpu-all.h b/cpu-all.h
index 1f4f1e7..0322955 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -765,6 +765,7 @@ void cpu_reset_interrupt(CPUState *env, int mask);
#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,
diff --git a/exec.c b/exec.c
index a9ac1ad..0dd4aa3 100644
--- a/exec.c
+++ b/exec.c
@@ -2506,16 +2506,38 @@ static CPUWriteMemoryFunc *notdirty_mem_write[3] = {
static void check_watchpoint(int offset, int len_mask, int flags)
{
CPUState *env = cpu_single_env;
+ target_ulong pc, cs_base;
+ TranslationBlock *tb;
target_ulong vaddr;
CPUWatchpoint *wp;
+ int cpu_flags;
+ 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, &pc, &cs_base, &cpu_flags);
+ tb_gen_code(env, pc, cs_base, cpu_flags, 1);
+ }
+ cpu_resume_from_signal(env, NULL);
}
}
}
^ permalink raw reply related [flat|nested] 37+ messages in thread* [Qemu-devel] [PATCH v5 03/18] gdbstub: Return appropriate watch message to gdb
2008-11-17 16:18 [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Jan Kiszka
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 07/18] Restore pc on watchpoint hits Jan Kiszka
@ 2008-11-17 16:18 ` Jan Kiszka
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 05/18] Set mem_io_vaddr on io_read Jan Kiszka
` (16 subsequent siblings)
18 siblings, 0 replies; 37+ messages in thread
From: Jan Kiszka @ 2008-11-17 16:18 UTC (permalink / raw)
To: qemu-devel
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 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index 15d38f0..aff8189 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1493,6 +1493,7 @@ static void gdb_vm_stopped(void *opaque, int reason)
{
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, int reason)
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].type &
+ (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 related [flat|nested] 37+ messages in thread* [Qemu-devel] [PATCH v5 05/18] Set mem_io_vaddr on io_read
2008-11-17 16:18 [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Jan Kiszka
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 07/18] Restore pc on watchpoint hits Jan Kiszka
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 03/18] gdbstub: Return appropriate watch message to gdb Jan Kiszka
@ 2008-11-17 16:18 ` Jan Kiszka
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 01/18] Convert CPU_PC_FROM_TB to static inline Jan Kiszka
` (15 subsequent siblings)
18 siblings, 0 replies; 37+ messages in thread
From: Jan Kiszka @ 2008-11-17 16:18 UTC (permalink / raw)
To: qemu-devel
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 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/softmmu_template.h b/softmmu_template.h
index 98dd378..cf91804 100644
--- a/softmmu_template.h
+++ b/softmmu_template.h
@@ -64,6 +64,7 @@ static inline DATA_TYPE glue(io_read, SUFFIX)(target_phys_addr_t physaddr,
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 related [flat|nested] 37+ messages in thread* [Qemu-devel] [PATCH v5 01/18] Convert CPU_PC_FROM_TB to static inline
2008-11-17 16:18 [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Jan Kiszka
` (2 preceding siblings ...)
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 05/18] Set mem_io_vaddr on io_read Jan Kiszka
@ 2008-11-17 16:18 ` Jan Kiszka
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 02/18] Refactor translation block CPU state handling Jan Kiszka
` (14 subsequent siblings)
18 siblings, 0 replies; 37+ messages in thread
From: Jan Kiszka @ 2008-11-17 16:18 UTC (permalink / raw)
To: qemu-devel
...as macros should be avoided when possible.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
cpu-exec.c | 4 ++--
target-alpha/cpu.h | 6 +++++-
target-arm/cpu.h | 8 ++++++--
target-cris/cpu.h | 9 +++++++--
target-i386/cpu.h | 8 ++++++--
target-m68k/cpu.h | 8 ++++++--
target-mips/cpu.h | 12 +++++++-----
target-ppc/cpu.h | 8 ++++++--
target-sh4/cpu.h | 12 +++++++-----
target-sparc/cpu.h | 12 +++++++-----
10 files changed, 59 insertions(+), 28 deletions(-)
diff --git a/cpu-exec.c b/cpu-exec.c
index 9265fc1..322af0d 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -110,7 +110,7 @@ static void cpu_exec_nocache(int max_cycles, TranslationBlock *orig_tb)
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);
@@ -654,7 +654,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. */
diff --git a/target-alpha/cpu.h b/target-alpha/cpu.h
index 210cc55..f606fac 100644
--- a/target-alpha/cpu.h
+++ b/target-alpha/cpu.h
@@ -318,6 +318,7 @@ static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
#endif
#include "cpu-all.h"
+#include "exec-all.h"
enum {
FEATURE_ASN = 0x00000001,
@@ -416,6 +417,9 @@ 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->pc;
+}
#endif /* !defined (__CPU_ALPHA_H__) */
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index c182245..79e51ac 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -415,8 +415,12 @@ static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
}
#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->pc;
+}
#endif
diff --git a/target-cris/cpu.h b/target-cris/cpu.h
index 1a8c884..19cb209 100644
--- a/target-cris/cpu.h
+++ b/target-cris/cpu.h
@@ -237,7 +237,12 @@ static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
#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->pc;
+}
+
#endif
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 2e21bc3..ead073c 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -789,10 +789,14 @@ static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
}
#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->pc - tb->cs_base;
+}
+
#endif /* CPU_I386_H */
diff --git a/target-m68k/cpu.h b/target-m68k/cpu.h
index a6687b1..48c752c 100644
--- a/target-m68k/cpu.h
+++ b/target-m68k/cpu.h
@@ -231,8 +231,12 @@ static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
}
#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->pc;
+}
#endif
diff --git a/target-mips/cpu.h b/target-mips/cpu.h
index d686f8e..a2544b1 100644
--- a/target-mips/cpu.h
+++ b/target-mips/cpu.h
@@ -502,6 +502,7 @@ static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
}
#include "cpu-all.h"
+#include "exec-all.h"
/* Memory access type :
* may be needed for precise access rights control and precise exceptions.
@@ -563,10 +564,11 @@ CPUMIPSState *cpu_mips_init(const char *cpu_model);
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->pc;
+ env->hflags &= ~MIPS_HFLAG_BMASK;
+ env->hflags |= tb->flags & MIPS_HFLAG_BMASK;
+}
#endif /* !defined (__MIPS_CPU_H__) */
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 00eac07..1ffbc30 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -826,9 +826,8 @@ static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
}
#endif
-#define CPU_PC_FROM_TB(env, tb) env->nip = tb->pc
-
#include "cpu-all.h"
+#include "exec-all.h"
/*****************************************************************************/
/* CRF definitions */
@@ -1432,4 +1431,9 @@ enum {
/*****************************************************************************/
+static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
+{
+ env->nip = tb->pc;
+}
+
#endif /* !defined (__CPU_PPC_H__) */
diff --git a/target-sh4/cpu.h b/target-sh4/cpu.h
index 2f42e60..ed7c105 100644
--- a/target-sh4/cpu.h
+++ b/target-sh4/cpu.h
@@ -173,12 +173,8 @@ static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
}
#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,10 @@ static inline int cpu_ptel_pr (uint32_t ptel)
#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->pc;
+ env->flags = tb->flags;
+}
+
#endif /* _CPU_SH4_H */
diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h
index c13926d..4779e34 100644
--- a/target-sparc/cpu.h
+++ b/target-sparc/cpu.h
@@ -491,12 +491,8 @@ static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
}
#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,10 @@ 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->pc;
+ env->npc = tb->cs_base;
+}
+
#endif
^ permalink raw reply related [flat|nested] 37+ messages in thread* [Qemu-devel] [PATCH v5 02/18] Refactor translation block CPU state handling
2008-11-17 16:18 [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Jan Kiszka
` (3 preceding siblings ...)
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 01/18] Convert CPU_PC_FROM_TB to static inline Jan Kiszka
@ 2008-11-17 16:18 ` Jan Kiszka
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 04/18] Refactor and enhance break/watchpoint API Jan Kiszka
` (13 subsequent siblings)
18 siblings, 0 replies; 37+ messages in thread
From: Jan Kiszka @ 2008-11-17 16:18 UTC (permalink / raw)
To: qemu-devel
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 | 63 ++--------------------------------------------------
exec.c | 56 ++++++++++++++++++----------------------------
target-alpha/cpu.h | 8 +++++++
target-arm/cpu.h | 13 +++++++++++
target-cris/cpu.h | 9 +++++++
target-i386/cpu.h | 8 +++++++
target-m68k/cpu.h | 10 ++++++++
target-mips/cpu.h | 8 +++++++
target-ppc/cpu.h | 8 +++++++
target-sh4/cpu.h | 11 +++++++++
target-sparc/cpu.h | 16 +++++++++++++
11 files changed, 115 insertions(+), 95 deletions(-)
diff --git a/cpu-exec.c b/cpu-exec.c
index 322af0d..1955d24 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -169,71 +169,12 @@ static inline TranslationBlock *tb_find_fast(void)
{
TranslationBlock *tb;
target_ulong cs_base, pc;
- uint64_t flags;
+ int flags;
/* 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
+ cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags);
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)) {
diff --git a/exec.c b/exec.c
index 1edc737..772b1c1 100644
--- a/exec.c
+++ b/exec.c
@@ -886,12 +886,19 @@ TranslationBlock *tb_gen_code(CPUState *env,
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;
+ target_ulong current_pc = 0;
+ target_ulong current_cs_base = 0;
+ int current_flags = 0;
+#endif /* TARGET_HAS_PRECISE_SMC */
p = page_find(start >> TARGET_PAGE_BITS);
if (!p)
@@ -905,12 +912,6 @@ void tb_invalidate_phys_page_range(target_phys_addr_t start, target_phys_addr_t
/* 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;
@@ -947,14 +948,8 @@ void tb_invalidate_phys_page_range(target_phys_addr_t start, target_phys_addr_t
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, ¤t_pc, ¤t_cs_base,
+ ¤t_flags);
}
#endif /* TARGET_HAS_PRECISE_SMC */
/* we need to do that to handle the case where a signal
@@ -1027,12 +1022,16 @@ static inline void tb_invalidate_phys_page_fast(target_phys_addr_t start, int le
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;
+ target_ulong current_pc = 0;
+ target_ulong current_cs_base = 0;
+ int current_flags = 0;
#endif
addr &= TARGET_PAGE_MASK;
@@ -1040,11 +1039,6 @@ static void tb_invalidate_phys_page(target_phys_addr_t addr,
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 +1058,8 @@ static void tb_invalidate_phys_page(target_phys_addr_t addr,
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, ¤t_pc, ¤t_cs_base,
+ ¤t_flags);
}
#endif /* TARGET_HAS_PRECISE_SMC */
tb_phys_invalidate(tb, addr);
diff --git a/target-alpha/cpu.h b/target-alpha/cpu.h
index f606fac..122e2c2 100644
--- a/target-alpha/cpu.h
+++ b/target-alpha/cpu.h
@@ -422,4 +422,12 @@ static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
env->pc = tb->pc;
}
+static inline void cpu_get_tb_cpu_state(CPUState *env, target_ulong *pc,
+ target_ulong *cs_base, int *flags)
+{
+ *pc = env->pc;
+ *cs_base = 0;
+ *flags = env->ps;
+}
+
#endif /* !defined (__CPU_ALPHA_H__) */
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 79e51ac..d6cb116 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -423,4 +423,17 @@ static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
env->regs[15] = tb->pc;
}
+static inline void cpu_get_tb_cpu_state(CPUState *env, target_ulong *pc,
+ target_ulong *cs_base, int *flags)
+{
+ *pc = env->regs[15];
+ *cs_base = 0;
+ *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)
+ *flags |= (1 << 6);
+ if (env->vfp.xregs[ARM_VFP_FPEXC] & (1 << 30))
+ *flags |= (1 << 7);
+}
+
#endif
diff --git a/target-cris/cpu.h b/target-cris/cpu.h
index 19cb209..44a98e4 100644
--- a/target-cris/cpu.h
+++ b/target-cris/cpu.h
@@ -245,4 +245,13 @@ static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
env->pc = tb->pc;
}
+static inline void cpu_get_tb_cpu_state(CPUState *env, target_ulong *pc,
+ target_ulong *cs_base, int *flags)
+{
+ *pc = env->pc;
+ *cs_base = 0;
+ *flags = env->dslot |
+ (env->pregs[PR_CCS] & (S_FLAG | P_FLAG | U_FLAG | X_FLAG));
+}
+
#endif
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index ead073c..ebbf1b1 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -799,4 +799,12 @@ static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
env->eip = tb->pc - tb->cs_base;
}
+static inline void cpu_get_tb_cpu_state(CPUState *env, target_ulong *pc,
+ target_ulong *cs_base, int *flags)
+{
+ *cs_base = env->segs[R_CS].base;
+ *pc = *cs_base + env->eip;
+ *flags = env->hflags | (env->eflags & (IOPL_MASK | TF_MASK | VM_MASK));
+}
+
#endif /* CPU_I386_H */
diff --git a/target-m68k/cpu.h b/target-m68k/cpu.h
index 48c752c..bdef9b8 100644
--- a/target-m68k/cpu.h
+++ b/target-m68k/cpu.h
@@ -239,4 +239,14 @@ static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
env->pc = tb->pc;
}
+static inline void cpu_get_tb_cpu_state(CPUState *env, target_ulong *pc,
+ target_ulong *cs_base, int *flags)
+{
+ *pc = env->pc;
+ *cs_base = 0;
+ *flags = (env->fpcr & M68K_FPCR_PREC) /* Bit 6 */
+ | (env->sr & SR_S) /* Bit 13 */
+ | ((env->macsr >> 4) & 0xf); /* Bits 0-3 */
+}
+
#endif
diff --git a/target-mips/cpu.h b/target-mips/cpu.h
index a2544b1..86a7680 100644
--- a/target-mips/cpu.h
+++ b/target-mips/cpu.h
@@ -571,4 +571,12 @@ static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
env->hflags |= tb->flags & MIPS_HFLAG_BMASK;
}
+static inline void cpu_get_tb_cpu_state(CPUState *env, target_ulong *pc,
+ target_ulong *cs_base, int *flags)
+{
+ *pc = env->active_tc.PC;
+ *cs_base = 0;
+ *flags = env->hflags & (MIPS_HFLAG_TMASK | MIPS_HFLAG_BMASK);
+}
+
#endif /* !defined (__MIPS_CPU_H__) */
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 1ffbc30..35c824a 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1436,4 +1436,12 @@ static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
env->nip = tb->pc;
}
+static inline void cpu_get_tb_cpu_state(CPUState *env, target_ulong *pc,
+ target_ulong *cs_base, int *flags)
+{
+ *pc = env->nip;
+ *cs_base = 0;
+ *flags = env->hflags;
+}
+
#endif /* !defined (__CPU_PPC_H__) */
diff --git a/target-sh4/cpu.h b/target-sh4/cpu.h
index ed7c105..23ecded 100644
--- a/target-sh4/cpu.h
+++ b/target-sh4/cpu.h
@@ -271,4 +271,15 @@ static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
env->flags = tb->flags;
}
+static inline void cpu_get_tb_cpu_state(CPUState *env, target_ulong *pc,
+ target_ulong *cs_base, int *flags)
+{
+ *pc = env->pc;
+ *cs_base = 0;
+ *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 */
diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h
index 4779e34..93c1be5 100644
--- a/target-sparc/cpu.h
+++ b/target-sparc/cpu.h
@@ -510,4 +510,20 @@ static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
env->npc = tb->cs_base;
}
+static inline void cpu_get_tb_cpu_state(CPUState *env, target_ulong *pc,
+ target_ulong *cs_base, int *flags)
+{
+ *pc = env->pc;
+ *cs_base = env->npc;
+#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
+}
+
#endif
^ permalink raw reply related [flat|nested] 37+ messages in thread* [Qemu-devel] [PATCH v5 04/18] Refactor and enhance break/watchpoint API
2008-11-17 16:18 [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Jan Kiszka
` (4 preceding siblings ...)
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 02/18] Refactor translation block CPU state handling Jan Kiszka
@ 2008-11-17 16:18 ` Jan Kiszka
2008-11-18 19:59 ` Anthony Liguori
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 06/18] Respect length of watchpoints Jan Kiszka
` (12 subsequent siblings)
18 siblings, 1 reply; 37+ messages in thread
From: Jan Kiszka @ 2008-11-17 16:18 UTC (permalink / raw)
To: qemu-devel
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 | 195 ++++++++++++++++++++++++++++------------------
gdbstub.c | 137 ++++++++++++++++++--------------
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, 267 insertions(+), 183 deletions(-)
diff --git a/cpu-all.h b/cpu-all.h
index cdd79bc..1f4f1e7 100644
--- 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 */
diff --git a/cpu-defs.h b/cpu-defs.h
index 46d4487..069d312 100644
--- 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)
@@ -145,6 +143,19 @@ typedef struct icount_decr_u16 {
struct kvm_run;
struct KVMState;
+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 */ \
@@ -177,16 +188,11 @@ struct KVMState;
\
/* 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; \
\
diff --git a/exec.c b/exec.c
index 772b1c1..4619e49 100644
--- 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,
@@ -1299,107 +1298,150 @@ static void breakpoint_invalidate(CPUState *env, target_ulong pc)
#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;
+
+ if (watchpoint)
+ *watchpoint = wp;
+ return 0;
}
-/* Remove a watchpoint. */
-int cpu_watchpoint_remove(CPUState *env, target_ulong addr)
+/* Remove a specific watchpoint. */
+int cpu_watchpoint_remove(CPUState *env, target_ulong addr, target_ulong len,
+ int flags)
{
- int i;
+ CPUWatchpoint *wp;
- 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);
+ 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);
+}
+
+/* Remove all matching watchpoints. */
+void cpu_watchpoint_remove_all(CPUState *env, int mask)
+{
+ CPUWatchpoint *wp;
+
+ for (wp = env->watchpoints; wp != NULL; wp = wp->next)
+ if (wp->flags & mask)
+ cpu_watchpoint_remove_by_ref(env, wp);
}
-/* 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)
+/* Add a breakpoint. */
+int cpu_breakpoint_insert(CPUState *env, target_ulong pc, int flags,
+ CPUBreakpoint **breakpoint)
{
#if defined(TARGET_HAS_ICE)
- int i;
+ CPUBreakpoint *bp;
- for(i = 0; i < env->nb_breakpoints; i++) {
- if (env->breakpoints[i] == pc)
- return 0;
- }
+ bp = qemu_malloc(sizeof(*bp));
+ if (!bp)
+ return -ENOBUFS;
- if (env->nb_breakpoints >= MAX_BREAKPOINTS)
- return -1;
- env->breakpoints[env->nb_breakpoints++] = pc;
+ 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
}
@@ -1881,7 +1923,7 @@ int tlb_set_page_exec(CPUState *env, target_ulong vaddr,
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);
@@ -1922,8 +1964,8 @@ int tlb_set_page_exec(CPUState *env, target_ulong vaddr,
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. */
@@ -2456,13 +2498,12 @@ static void check_watchpoint(int offset, int flags)
{
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;
}
@@ -2474,40 +2515,40 @@ static void check_watchpoint(int offset, int flags)
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);
}
diff --git a/gdbstub.c b/gdbstub.c
index aff8189..a0fc62d 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1145,10 +1145,70 @@ void gdb_register_coprocessor(CPUState * env,
}
}
+/* 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:
+ case GDB_BREAKPOINT_HW:
+ return cpu_breakpoint_insert(env, addr, BP_GDB, NULL);
+#ifndef CONFIG_USER_ONLY
+ case GDB_WATCHPOINT_WRITE:
+ case GDB_WATCHPOINT_READ:
+ case 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:
+ case GDB_BREAKPOINT_HW:
+ return cpu_breakpoint_remove(env, addr, BP_GDB);
+#ifndef CONFIG_USER_ONLY
+ case GDB_WATCHPOINT_WRITE:
+ case GDB_WATCHPOINT_READ:
+ case 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 +1228,7 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
* 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 +1262,7 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
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 +1385,6 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
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 +1393,16 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
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 +1523,11 @@ static void gdb_vm_stopped(void *opaque, int reason)
if (reason == EXCP_DEBUG) {
if (s->env->watchpoint_hit) {
- switch (s->env->watchpoint[s->env->watchpoint_hit - 1].type &
- (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 +1535,9 @@ static void gdb_vm_stopped(void *opaque, int reason)
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);
diff --git a/target-alpha/translate.c b/target-alpha/translate.c
index 8fc9766..3b90f62 100644
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -2340,6 +2340,7 @@ static always_inline void gen_intermediate_code_internal (CPUState *env,
target_ulong pc_start;
uint32_t insn;
uint16_t *gen_opc_end;
+ CPUBreakpoint *bp;
int j, lj = -1;
int ret;
int num_insns;
@@ -2362,9 +2363,9 @@ static always_inline void gen_intermediate_code_internal (CPUState *env,
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;
}
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 9d52991..237c5f6 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -8600,6 +8600,7 @@ static inline void gen_intermediate_code_internal(CPUState *env,
int search_pc)
{
DisasContext dc1, *dc = &dc1;
+ CPUBreakpoint *bp;
uint16_t *gen_opc_end;
int j, lj;
target_ulong pc_start;
@@ -8676,9 +8677,9 @@ static inline void gen_intermediate_code_internal(CPUState *env,
}
#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);
@@ -8731,7 +8732,7 @@ static inline void gen_intermediate_code_internal(CPUState *env,
/* 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.
diff --git a/target-cris/translate.c b/target-cris/translate.c
index a10ccb6..ac258a9 100644
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -3187,10 +3187,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);
diff --git a/target-i386/translate.c b/target-i386/translate.c
index 5006106..0de238b 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -7522,6 +7522,7 @@ static inline void gen_intermediate_code_internal(CPUState *env,
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;
@@ -7605,9 +7606,9 @@ static inline void gen_intermediate_code_internal(CPUState *env,
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;
}
diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index 07f0640..49e2cb2 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -2965,6 +2965,7 @@ gen_intermediate_code_internal(CPUState *env, TranslationBlock *tb,
{
DisasContext dc1, *dc = &dc1;
uint16_t *gen_opc_end;
+ CPUBreakpoint *bp;
int j, lj;
target_ulong pc_start;
int pc_offset;
@@ -2998,9 +2999,9 @@ gen_intermediate_code_internal(CPUState *env, TranslationBlock *tb,
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;
@@ -3030,7 +3031,7 @@ gen_intermediate_code_internal(CPUState *env, TranslationBlock *tb,
/* 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 &&
diff --git a/target-mips/translate.c b/target-mips/translate.c
index 590c2b6..cc7e71c 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -8246,6 +8246,7 @@ gen_intermediate_code_internal (CPUState *env, TranslationBlock *tb,
DisasContext ctx;
target_ulong pc_start;
uint16_t *gen_opc_end;
+ CPUBreakpoint *bp;
int j, lj = -1;
int num_insns;
int max_insns;
@@ -8285,9 +8286,9 @@ gen_intermediate_code_internal (CPUState *env, TranslationBlock *tb,
#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;
gen_helper_0i(raise_exception, EXCP_DEBUG);
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index cceb434..6faffa1 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -7170,6 +7170,7 @@ static always_inline void gen_intermediate_code_internal (CPUState *env,
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;
@@ -7224,9 +7225,9 @@ static always_inline void gen_intermediate_code_internal (CPUState *env,
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;
diff --git a/target-sh4/translate.c b/target-sh4/translate.c
index b05e8fc..eb3366c 100644
--- a/target-sh4/translate.c
+++ b/target-sh4/translate.c
@@ -1803,6 +1803,7 @@ gen_intermediate_code_internal(CPUState * env, TranslationBlock * tb,
DisasContext ctx;
target_ulong pc_start;
static uint16_t *gen_opc_end;
+ CPUBreakpoint *bp;
int i, ii;
int num_insns;
int max_insns;
@@ -1836,9 +1837,9 @@ gen_intermediate_code_internal(CPUState * env, TranslationBlock * tb,
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);
gen_helper_debug();
diff --git a/target-sparc/translate.c b/target-sparc/translate.c
index 77fb723..e94e3c5 100644
--- a/target-sparc/translate.c
+++ b/target-sparc/translate.c
@@ -4778,6 +4778,7 @@ static inline void gen_intermediate_code_internal(TranslationBlock * tb,
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;
@@ -4815,9 +4816,9 @@ static inline void gen_intermediate_code_internal(TranslationBlock * tb,
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);
gen_helper_debug();
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [Qemu-devel] [PATCH v5 04/18] Refactor and enhance break/watchpoint API
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 04/18] Refactor and enhance break/watchpoint API Jan Kiszka
@ 2008-11-18 19:59 ` Anthony Liguori
2008-11-18 22:24 ` [Qemu-devel] " Jan Kiszka
0 siblings, 1 reply; 37+ messages in thread
From: Anthony Liguori @ 2008-11-18 19:59 UTC (permalink / raw)
To: qemu-devel
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.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>
I noticed you use an open coded doubly linked list. Can you submit a
follow-up patch to convert the code to use a TAILQ from sys-queue.h?
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 37+ messages in thread
* [Qemu-devel] Re: [PATCH v5 04/18] Refactor and enhance break/watchpoint API
2008-11-18 19:59 ` Anthony Liguori
@ 2008-11-18 22:24 ` Jan Kiszka
0 siblings, 0 replies; 37+ messages in thread
From: Jan Kiszka @ 2008-11-18 22:24 UTC (permalink / raw)
To: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1385 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.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>
>
> I noticed you use an open coded doubly linked list. Can you submit a
> follow-up patch to convert the code to use a TAILQ from sys-queue.h?
>
WIP. Takes a few changes, so a bit more testing is also required. But
this refactoring already uncovered a use-after-release bug in my
original patch.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v5 06/18] Respect length of watchpoints
2008-11-17 16:18 [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Jan Kiszka
` (5 preceding siblings ...)
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 04/18] Refactor and enhance break/watchpoint API Jan Kiszka
@ 2008-11-17 16:18 ` Jan Kiszka
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 13/18] x86: Debug register emulation Jan Kiszka
` (11 subsequent siblings)
18 siblings, 0 replies; 37+ messages in thread
From: Jan Kiszka @ 2008-11-17 16:18 UTC (permalink / raw)
To: qemu-devel
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 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/exec.c b/exec.c
index 4619e49..a9ac1ad 100644
--- a/exec.c
+++ b/exec.c
@@ -1301,14 +1301,21 @@ static void breakpoint_invalidate(CPUState *env, target_ulong pc)
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;
@@ -1332,10 +1339,12 @@ int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
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;
}
@@ -2494,7 +2503,7 @@ static CPUWriteMemoryFunc *notdirty_mem_write[3] = {
};
/* 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;
@@ -2502,7 +2511,8 @@ static void check_watchpoint(int offset, int flags)
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;
@@ -2515,40 +2525,40 @@ static void check_watchpoint(int offset, int flags)
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 related [flat|nested] 37+ messages in thread* [Qemu-devel] [PATCH v5 13/18] x86: Debug register emulation
2008-11-17 16:18 [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Jan Kiszka
` (6 preceding siblings ...)
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 06/18] Respect length of watchpoints Jan Kiszka
@ 2008-11-17 16:18 ` Jan Kiszka
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 09/18] gdbstub: manage CPUs as threads Jan Kiszka
` (10 subsequent siblings)
18 siblings, 0 replies; 37+ messages in thread
From: Jan Kiszka @ 2008-11-17 16:18 UTC (permalink / raw)
To: qemu-devel
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 | 36 ++++++++++
target-i386/helper.c | 169 ++++++++++++++++++++++++++++++++++++++---------
target-i386/machine.c | 4 +
target-i386/op_helper.c | 46 +++++++++++--
5 files changed, 217 insertions(+), 42 deletions(-)
diff --git a/linux-user/main.c b/linux-user/main.c
index f17d012..66be107 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -403,7 +403,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) {
@@ -413,7 +413,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 {
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index ebbf1b1..eed1f62 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -205,6 +205,16 @@
#define CR4_OSFXSR_MASK (1 << CR4_OSFXSR_SHIFT)
#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
@@ -362,7 +372,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
@@ -596,6 +606,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 */
@@ -789,6 +803,26 @@ static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
}
#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;
+}
+
+void hw_breakpoint_insert(CPUState *env, int index);
+void hw_breakpoint_remove(CPUState *env, int index);
+int check_hw_breakpoints(CPUState *env, int force_dr6_update);
+
#include "cpu-all.h"
#include "exec-all.h"
diff --git a/target-i386/helper.c b/target-i386/helper.c
index baa99c9..0cfef44 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -34,8 +34,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,
@@ -93,35 +91,6 @@ static void add_flagname_to_bitmaps(char *flagname, uint32_t *features,
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
- if (kvm_enabled())
- kvm_init_vcpu(env);
- return env;
-}
-
typedef struct x86_def_t {
const char *name;
uint32_t level;
@@ -499,6 +468,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)
@@ -1295,6 +1270,105 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr)
paddr = (pte & TARGET_PAGE_MASK) + page_offset;
return paddr;
}
+
+void hw_breakpoint_insert(CPUState *env, 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;
+}
+
+void hw_breakpoint_remove(CPUState *env, 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;
+ }
+}
+
+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 */
static void host_cpuid(uint32_t function, uint32_t *eax, uint32_t *ebx,
@@ -1532,3 +1606,36 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
break;
}
}
+
+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();
+#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
+ if (kvm_enabled())
+ kvm_init_vcpu(env);
+ return env;
+}
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 7f78b0d..faab2eb 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -259,6 +259,10 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
for(i = 0; i < 8; i++)
qemu_get_betls(f, &env->dr[i]);
+ cpu_breakpoint_remove_all(env, BP_CPU);
+ cpu_watchpoint_remove_all(env, BP_CPU);
+ for (i = 0; i < 4; i++)
+ hw_breakpoint_insert(env, i);
/* MMU */
qemu_get_sbe32s(f, &a20_mask);
diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
index 90f685d..6dc0802 100644
--- a/target-i386/op_helper.c
+++ b/target-i386/op_helper.c
@@ -496,6 +496,17 @@ static void switch_tss(int tss_selector,
/* XXX: different exception if CALL ? */
raise_exception_err(EXCP0D_GPF, 0);
}
+
+#ifndef CONFIG_USER_ONLY
+ /* 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(env, i);
+ }
+ env->dr[7] &= ~0x55;
+ }
+#endif
}
/* check if Port I/O is allowed in TSS */
@@ -1879,8 +1890,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);
+ env->dr[6] |= DR6_BS;
+#endif
+ raise_exception(EXCP01_DB);
}
void helper_cpuid(void)
@@ -2868,6 +2882,10 @@ target_ulong helper_read_crN(int reg)
void helper_write_crN(int reg, target_ulong t0)
{
}
+
+void helper_movl_drN_T0(int reg, target_ulong t0)
+{
+}
#else
target_ulong helper_read_crN(int reg)
{
@@ -2913,6 +2931,24 @@ void helper_write_crN(int reg, target_ulong t0)
break;
}
}
+
+void helper_movl_drN_T0(int reg, target_ulong t0)
+{
+ int i;
+
+ if (reg < 4) {
+ hw_breakpoint_remove(env, reg);
+ env->dr[reg] = t0;
+ hw_breakpoint_insert(env, reg);
+ } else if (reg == 7) {
+ for (i = 0; i < 4; i++)
+ hw_breakpoint_remove(env, i);
+ env->dr[7] = t0;
+ for (i = 0; i < 4; i++)
+ hw_breakpoint_insert(env, i);
+ } else
+ env->dr[reg] = t0;
+}
#endif
void helper_lmsw(target_ulong t0)
@@ -2929,12 +2965,6 @@ 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;
-}
-
void helper_invlpg(target_ulong addr)
{
helper_svm_check_intercept_param(SVM_EXIT_INVLPG, 0);
^ permalink raw reply related [flat|nested] 37+ messages in thread* [Qemu-devel] [PATCH v5 09/18] gdbstub: manage CPUs as threads
2008-11-17 16:18 [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Jan Kiszka
` (7 preceding siblings ...)
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 13/18] x86: Debug register emulation Jan Kiszka
@ 2008-11-17 16:18 ` Jan Kiszka
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 18/18] gdbstub: x86: Switch 64/32 bit registers dynamically Jan Kiszka
` (9 subsequent siblings)
18 siblings, 0 replies; 37+ messages in thread
From: Jan Kiszka @ 2008-11-17 16:18 UTC (permalink / raw)
To: qemu-devel
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 | 276 +++++++++++++++++++++++++++++++++++++++++++------------------
gdbstub.h | 1
vl.c | 1
3 files changed, 196 insertions(+), 82 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index a0fc62d..b522a89 100644
--- 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 *mem, int len)
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 *env, const char *p, const char **newp)
"<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,55 +1159,84 @@ 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:
case 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:
case GDB_WATCHPOINT_READ:
case 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:
case 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:
case GDB_WATCHPOINT_READ:
case 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,32 +1250,33 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
switch(ch) {
case '?':
/* TODO: Make this return the correct value for user-mode. */
- snprintf(buf, sizeof(buf), "S%02x", SIGTRAP);
+ snprintf(buf, sizeof(buf), "T%02xthread:%02x;", SIGTRAP,
+ s->c_cpu->cpu_index+1);
put_packet(s, buf);
/* Remove all the breakpoints when this query is issued,
* 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);
@@ -1262,7 +1291,7 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
exit(0);
case 'D':
/* Detach packet */
- gdb_breakpoint_remove_all(env);
+ gdb_breakpoint_remove_all();
gdb_continue(s);
put_packet(s, "OK");
break;
@@ -1270,23 +1299,23 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
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':
@@ -1305,7 +1334,7 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
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 {
@@ -1316,7 +1345,7 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
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);
@@ -1327,7 +1356,7 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
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;
}
@@ -1338,7 +1367,7 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
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);
@@ -1353,7 +1382,7 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
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");
@@ -1365,7 +1394,7 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
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);
@@ -1381,7 +1410,7 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
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':
@@ -1394,9 +1423,9 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
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)
@@ -1404,6 +1433,45 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
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 */
@@ -1429,10 +1497,39 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
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
@@ -1459,7 +1556,7 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
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);
@@ -1507,10 +1604,17 @@ static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
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;
@@ -1519,11 +1623,11 @@ static void gdb_vm_stopped(void *opaque, int reason)
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;
@@ -1534,20 +1638,22 @@ static void gdb_vm_stopped(void *opaque, int reason)
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
@@ -1566,7 +1672,7 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
uint64_t i64;
GDBState *s;
- s = gdb_syscall_state;
+ s = gdbserver_state;
if (!s)
return;
gdb_current_syscall_cb = cb;
@@ -1611,15 +1717,14 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
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;
@@ -1685,7 +1790,7 @@ static void gdb_read_byte(GDBState *s, int ch)
} 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:
@@ -1702,7 +1807,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;
@@ -1750,7 +1855,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;
@@ -1759,7 +1864,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;
@@ -1781,13 +1886,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);
}
@@ -1829,7 +1941,7 @@ int gdbserver_start(int port)
if (gdbserver_fd < 0)
return -1;
/* accept connections */
- gdb_accept (NULL);
+ gdb_accept();
return 0;
}
#else
@@ -1842,11 +1954,10 @@ static int gdb_chr_can_receive(void *opaque)
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]);
}
}
@@ -1855,7 +1966,6 @@ static void gdb_chr_event(void *opaque, int event)
switch (event) {
case CHR_EVENT_RESET:
vm_stop(EXCP_INTERRUPT);
- gdb_syscall_state = opaque;
gdb_has_xml = 0;
break;
default:
@@ -1890,11 +2000,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
diff --git a/gdbstub.h b/gdbstub.h
index 76e9482..1ba7931 100644
--- a/gdbstub.h
+++ b/gdbstub.h
@@ -8,6 +8,7 @@ typedef void (*gdb_syscall_complete_cb)(CPUState *env,
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);
diff --git a/vl.c b/vl.c
index 2cfe23d..ef531df 100644
--- a/vl.c
+++ b/vl.c
@@ -3720,6 +3720,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 */
^ permalink raw reply related [flat|nested] 37+ messages in thread* [Qemu-devel] [PATCH v5 18/18] gdbstub: x86: Switch 64/32 bit registers dynamically
2008-11-17 16:18 [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Jan Kiszka
` (8 preceding siblings ...)
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 09/18] gdbstub: manage CPUs as threads Jan Kiszka
@ 2008-11-17 16:18 ` Jan Kiszka
2008-11-18 21:21 ` Anthony Liguori
2008-11-18 21:33 ` Anthony Liguori
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 12/18] Introduce BP_CPU as a breakpoint type Jan Kiszka
` (8 subsequent siblings)
18 siblings, 2 replies; 37+ messages in thread
From: Jan Kiszka @ 2008-11-17 16:18 UTC (permalink / raw)
To: qemu-devel
Commit 5459 broke gdbstub's dynamic register set switching between
x86-64 and i386. That prevents setting the correct architecture in gdb
when debugging 32 or 16-bit code in a 64-bit emulator. This patch
reintroduces the feature over previous refactorings.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
gdbstub.c | 50 +++++++++++++++++++++++++++++++++++++-------------
target-i386/cpu.h | 7 +++++--
2 files changed, 42 insertions(+), 15 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index c83b76a..2ed1695 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -301,8 +301,9 @@ static const int gpr_map[16] = {
8, 9, 10, 11, 12, 13, 14, 15
};
#else
-static const int gpr_map[8] = {0, 1, 2, 3, 4, 5, 6, 7};
+#define gpr_map gpr_map32
#endif
+static const int gpr_map32[8] = { 0, 1, 2, 3, 4, 5, 6, 7 };
#define NUM_CORE_REGS (CPU_NB_REGS * 2 + 25)
@@ -316,7 +317,10 @@ static const int gpr_map[8] = {0, 1, 2, 3, 4, 5, 6, 7};
static int cpu_gdb_read_register(CPUState *env, uint8_t *mem_buf, int n)
{
if (n < CPU_NB_REGS) {
- GET_REGL(env->regs[gpr_map[n]]);
+ if (TARGET_LONG_BITS == 64 && (env->hflags & HF_CS64_MASK))
+ GET_REGL(env->regs[gpr_map[n]]);
+ else if (n < CPU_NB_REGS32)
+ GET_REG32(env->regs[gpr_map32[n]]);
} else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
#ifdef USE_X86LDOUBLE
/* FIXME: byteswap float values - after fixing fpregs layout. */
@@ -327,12 +331,19 @@ static int cpu_gdb_read_register(CPUState *env, uint8_t *mem_buf, int n)
return 10;
} else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
n -= IDX_XMM_REGS;
- stq_p(mem_buf, env->xmm_regs[n].XMM_Q(0));
- stq_p(mem_buf + 8, env->xmm_regs[n].XMM_Q(1));
- return 16;
+ if (n < CPU_NB_REGS32
+ || (TARGET_LONG_BITS == 64 && (env->hflags & HF_CS64_MASK))) {
+ stq_p(mem_buf, env->xmm_regs[n].XMM_Q(0));
+ stq_p(mem_buf + 8, env->xmm_regs[n].XMM_Q(1));
+ return 16;
+ }
} else {
switch (n) {
- case IDX_IP_REG: GET_REGL(env->eip);
+ case IDX_IP_REG:
+ if (TARGET_LONG_BITS == 64 && (env->hflags & HF_CS64_MASK))
+ GET_REG64(env->eip);
+ else
+ GET_REG32(env->eip);
case IDX_FLAGS_REG: GET_REG32(env->eflags);
case IDX_SEG_REGS: GET_REG32(env->segs[R_CS].selector);
@@ -400,8 +411,13 @@ static int cpu_gdb_write_register(CPUState *env, uint8_t *mem_buf, int n)
uint32_t tmp;
if (n < CPU_NB_REGS) {
- env->regs[gpr_map[n]] = ldtul_p(mem_buf);
- return sizeof(target_ulong);
+ if (TARGET_LONG_BITS == 64 && (env->hflags & HF_CS64_MASK)) {
+ env->regs[gpr_map[n]] = ldtul_p(mem_buf);
+ return sizeof(target_ulong);
+ } else if (n < CPU_NB_REGS32) {
+ env->regs[gpr_map32[n]] = ldl_p(mem_buf);
+ return 4;
+ }
} else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
#ifdef USE_X86LDOUBLE
/* FIXME: byteswap float values - after fixing fpregs layout. */
@@ -410,14 +426,22 @@ static int cpu_gdb_write_register(CPUState *env, uint8_t *mem_buf, int n)
return 10;
} else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
n -= IDX_XMM_REGS;
- env->xmm_regs[n].XMM_Q(0) = ldq_p(mem_buf);
- env->xmm_regs[n].XMM_Q(1) = ldq_p(mem_buf + 8);
- return 16;
+ if (n < CPU_NB_REGS32
+ || (TARGET_LONG_BITS == 64 && (env->hflags & HF_CS64_MASK))) {
+ env->xmm_regs[n].XMM_Q(0) = ldq_p(mem_buf);
+ env->xmm_regs[n].XMM_Q(1) = ldq_p(mem_buf + 8);
+ return 16;
+ }
} else {
switch (n) {
case IDX_IP_REG:
- env->eip = ldtul_p(mem_buf);
- return sizeof(target_ulong);
+ if (TARGET_LONG_BITS == 64 && (env->hflags & HF_CS64_MASK)) {
+ env->eip = ldq_p(mem_buf);
+ return 8;
+ } else {
+ env->eip = ldl_p(mem_buf);
+ return 4;
+ }
case IDX_FLAGS_REG:
env->eflags = ldl_p(mem_buf);
return 4;
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index b7c8a2f..01884ec 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -511,10 +511,13 @@ typedef union {
#endif
#define MMX_Q(n) q
+#define CPU_NB_REGS64 16
+#define CPU_NB_REGS32 8
+
#ifdef TARGET_X86_64
-#define CPU_NB_REGS 16
+#define CPU_NB_REGS CPU_NB_REGS64
#else
-#define CPU_NB_REGS 8
+#define CPU_NB_REGS CPU_NB_REGS32
#endif
#define NB_MMU_MODES 2
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [Qemu-devel] [PATCH v5 18/18] gdbstub: x86: Switch 64/32 bit registers dynamically
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 18/18] gdbstub: x86: Switch 64/32 bit registers dynamically Jan Kiszka
@ 2008-11-18 21:21 ` Anthony Liguori
2008-11-18 21:33 ` Anthony Liguori
1 sibling, 0 replies; 37+ messages in thread
From: Anthony Liguori @ 2008-11-18 21:21 UTC (permalink / raw)
To: qemu-devel
Jan Kiszka wrote:
> Commit 5459 broke gdbstub's dynamic register set switching between
> x86-64 and i386. That prevents setting the correct architecture in gdb
> when debugging 32 or 16-bit code in a 64-bit emulator. This patch
> reintroduces the feature over previous refactorings.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>
This patch looks good to me. If you resubmit against SVN, I'll apply
it. It is rejected right now because it depends on the previous patches
and I didn't want to manually fix the rejects.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v5 18/18] gdbstub: x86: Switch 64/32 bit registers dynamically
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 18/18] gdbstub: x86: Switch 64/32 bit registers dynamically Jan Kiszka
2008-11-18 21:21 ` Anthony Liguori
@ 2008-11-18 21:33 ` Anthony Liguori
2008-11-18 21:45 ` Anthony Liguori
1 sibling, 1 reply; 37+ messages in thread
From: Anthony Liguori @ 2008-11-18 21:33 UTC (permalink / raw)
To: qemu-devel
Jan Kiszka wrote:
> Commit 5459 broke gdbstub's dynamic register set switching between
> x86-64 and i386. That prevents setting the correct architecture in gdb
> when debugging 32 or 16-bit code in a 64-bit emulator. This patch
> reintroduces the feature over previous refactorings.
>
How does this interact with SMP? If you have one VCPU in 32-bit mode
and another in 64-bit mode, won't that confuse GDB?
Regards,
Anthony Liguori
>
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [PATCH v5 18/18] gdbstub: x86: Switch 64/32 bit registers dynamically
2008-11-18 21:33 ` Anthony Liguori
@ 2008-11-18 21:45 ` Anthony Liguori
2008-11-18 22:37 ` [Qemu-devel] " Jan Kiszka
0 siblings, 1 reply; 37+ messages in thread
From: Anthony Liguori @ 2008-11-18 21:45 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka, Paul Brook
Anthony Liguori wrote:
> Jan Kiszka wrote:
>> Commit 5459 broke gdbstub's dynamic register set switching between
>> x86-64 and i386. That prevents setting the correct architecture in gdb
>> when debugging 32 or 16-bit code in a 64-bit emulator. This patch
>> reintroduces the feature over previous refactorings.
>>
>
> How does this interact with SMP? If you have one VCPU in 32-bit mode
> and another in 64-bit mode, won't that confuse GDB?
After talking to Paul in IRC, it seems that GDB is going to make
assumptions that all threads share an address space and potentially
cache memory accesses or at least be sloppy with how it does memory
accesses.
I think this is a pretty strong argument for using fork instead of
threads. I would think you should be able to provoke this pretty easily
with an SMP guest.
Regards,
Anthony Liguori
> Regards,
>
> Anthony Liguori
>
>
>>
>>
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [Qemu-devel] Re: [PATCH v5 18/18] gdbstub: x86: Switch 64/32 bit registers dynamically
2008-11-18 21:45 ` Anthony Liguori
@ 2008-11-18 22:37 ` Jan Kiszka
2008-11-18 22:46 ` Paul Brook
0 siblings, 1 reply; 37+ messages in thread
From: Jan Kiszka @ 2008-11-18 22:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Paul Brook
[-- Attachment #1: Type: text/plain, Size: 1902 bytes --]
Anthony Liguori wrote:
> Anthony Liguori wrote:
>> Jan Kiszka wrote:
>>> Commit 5459 broke gdbstub's dynamic register set switching between
>>> x86-64 and i386. That prevents setting the correct architecture in gdb
>>> when debugging 32 or 16-bit code in a 64-bit emulator. This patch
>>> reintroduces the feature over previous refactorings.
>>>
>>
>> How does this interact with SMP? If you have one VCPU in 32-bit mode
>> and another in 64-bit mode, won't that confuse GDB?
Each mode switch already confuses GDB: Either you see borken disassembly
and are unable to match what you see with your source code, or you
manually have to issue 'set arch ...' when you switch between CPUs in
different addressing modes. Fortunately, the latter is not a very common
case, at least to my experience.
The best approach, definitely, would be to teach GDB how to switch the
disassembler mode depending on the thread's (or VCPUs) state. But so
there is neither a mechanism in GDB for this, nor is GDB even aware of
the x86 modes (no tracking of privileged registers). We have some
preliminary patches for this, but they are still far away from GDB mainline.
>
> After talking to Paul in IRC, it seems that GDB is going to make
> assumptions that all threads share an address space and potentially
> cache memory accesses or at least be sloppy with how it does memory
> accesses.
>
> I think this is a pretty strong argument for using fork instead of
> threads. I would think you should be able to provoke this pretty easily
> with an SMP guest.
As I explained in another reply, this is not the primary use case for
the gdbstub as I see it. However, I'm open to learn how fork could help
us while keeping the usability we now have via threads plus opening more
of the guest code for this kind of debugging (though I don't see a need
for the latter - yet).
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] Re: [PATCH v5 18/18] gdbstub: x86: Switch 64/32 bit registers dynamically
2008-11-18 22:37 ` [Qemu-devel] " Jan Kiszka
@ 2008-11-18 22:46 ` Paul Brook
2008-11-18 23:07 ` Jan Kiszka
0 siblings, 1 reply; 37+ messages in thread
From: Paul Brook @ 2008-11-18 22:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
> The best approach, definitely, would be to teach GDB how to switch the
> disassembler mode depending on the thread's (or VCPUs) state. But so
> there is neither a mechanism in GDB for this, nor is GDB even aware of
> the x86 modes (no tracking of privileged registers). We have some
> preliminary patches for this, but they are still far away from GDB
> mainline.
I'm pretty sure all the infrastructure is there. gdb is able to natively debug
32-bit binaries on a 64-bit host and is able to switch disassembler modes for
ARM vs. Thumb.
Paul
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] Re: [PATCH v5 18/18] gdbstub: x86: Switch 64/32 bit registers dynamically
2008-11-18 22:46 ` Paul Brook
@ 2008-11-18 23:07 ` Jan Kiszka
2008-11-18 23:23 ` Paul Brook
0 siblings, 1 reply; 37+ messages in thread
From: Jan Kiszka @ 2008-11-18 23:07 UTC (permalink / raw)
To: Paul Brook; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 934 bytes --]
Paul Brook wrote:
>> The best approach, definitely, would be to teach GDB how to switch the
>> disassembler mode depending on the thread's (or VCPUs) state. But so
>> there is neither a mechanism in GDB for this, nor is GDB even aware of
>> the x86 modes (no tracking of privileged registers). We have some
>> preliminary patches for this, but they are still far away from GDB
>> mainline.
>
> I'm pretty sure all the infrastructure is there. gdb is able to natively debug
> 32-bit binaries on a 64-bit host and is able to switch disassembler modes for
> ARM vs. Thumb.
How is it done on ARM? Maybe that will provide the right pointer for x86.
For fun I recently tried to debug a 32-bit binary on a 64-bit host via
gdbserver - the fun ended as quickly as with qemu. The only way for
switching disassembly mode on x86 that I found is via set arch, and that
switching the register packet format as well.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] Re: [PATCH v5 18/18] gdbstub: x86: Switch 64/32 bit registers dynamically
2008-11-18 23:07 ` Jan Kiszka
@ 2008-11-18 23:23 ` Paul Brook
2008-11-18 23:38 ` Jan Kiszka
0 siblings, 1 reply; 37+ messages in thread
From: Paul Brook @ 2008-11-18 23:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
On Tuesday 18 November 2008, Jan Kiszka wrote:
> Paul Brook wrote:
> >> The best approach, definitely, would be to teach GDB how to switch the
> >> disassembler mode depending on the thread's (or VCPUs) state. But so
> >> there is neither a mechanism in GDB for this, nor is GDB even aware of
> >> the x86 modes (no tracking of privileged registers). We have some
> >> preliminary patches for this, but they are still far away from GDB
> >> mainline.
> >
> > I'm pretty sure all the infrastructure is there. gdb is able to natively
> > debug 32-bit binaries on a 64-bit host and is able to switch disassembler
> > modes for ARM vs. Thumb.
>
> How is it done on ARM? Maybe that will provide the right pointer for x86.
Anything you have symbols for you know what type of code it is from the
binary. On ARM there's an EABI defined scheme for identifying arm/thumb/data
regions. On x86 the ELF class of the image is probably sufficient.
In the absence of real information gdb falls back to the current CPU mode,
which is a bit in the CPU status register. Exactly which register/bit depends
whether you're talking to an M-profile device. M-profile cores are identified
based on the XML register descriptions. If you don't have an XML capable
target then you don't get to debug M-profile devices.
IIRC There's also a gdb option to override the fallback mode.
Paul
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] Re: [PATCH v5 18/18] gdbstub: x86: Switch 64/32 bit registers dynamically
2008-11-18 23:23 ` Paul Brook
@ 2008-11-18 23:38 ` Jan Kiszka
2008-11-19 0:06 ` Paul Brook
0 siblings, 1 reply; 37+ messages in thread
From: Jan Kiszka @ 2008-11-18 23:38 UTC (permalink / raw)
To: Paul Brook; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1938 bytes --]
Paul Brook wrote:
> On Tuesday 18 November 2008, Jan Kiszka wrote:
>> Paul Brook wrote:
>>>> The best approach, definitely, would be to teach GDB how to switch the
>>>> disassembler mode depending on the thread's (or VCPUs) state. But so
>>>> there is neither a mechanism in GDB for this, nor is GDB even aware of
>>>> the x86 modes (no tracking of privileged registers). We have some
>>>> preliminary patches for this, but they are still far away from GDB
>>>> mainline.
>>> I'm pretty sure all the infrastructure is there. gdb is able to natively
>>> debug 32-bit binaries on a 64-bit host and is able to switch disassembler
>>> modes for ARM vs. Thumb.
>> How is it done on ARM? Maybe that will provide the right pointer for x86.
>
> Anything you have symbols for you know what type of code it is from the
> binary. On ARM there's an EABI defined scheme for identifying arm/thumb/data
> regions. On x86 the ELF class of the image is probably sufficient.
ELF-based detection can only work as good the underlying 'raw' switching
works.
>
> In the absence of real information gdb falls back to the current CPU mode,
> which is a bit in the CPU status register. Exactly which register/bit depends
> whether you're talking to an M-profile device. M-profile cores are identified
> based on the XML register descriptions. If you don't have an XML capable
> target then you don't get to debug M-profile devices.
...and this surely doesn't map on x86 (yet): gdb has no clue at all
about the CPU mode as it has no clue about segments or control registers.
>
> IIRC There's also a gdb option to override the fallback mode.
For x86, the core of the issue is a decoupled control of the gdb remote
protocol and the disassembly mode. I guess I have to dig a bit in the
code to see if the hard coupling we see in practice can be broken up.
Not according to the command help I found so far.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] Re: [PATCH v5 18/18] gdbstub: x86: Switch 64/32 bit registers dynamically
2008-11-18 23:38 ` Jan Kiszka
@ 2008-11-19 0:06 ` Paul Brook
2008-11-19 9:38 ` Jan Kiszka
0 siblings, 1 reply; 37+ messages in thread
From: Paul Brook @ 2008-11-19 0:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
> > In the absence of real information gdb falls back to the current CPU
> > mode, which is a bit in the CPU status register. Exactly which
> > register/bit depends whether you're talking to an M-profile device.
> > M-profile cores are identified based on the XML register descriptions. If
> > you don't have an XML capable target then you don't get to debug
> > M-profile devices.
>
> ...and this surely doesn't map on x86 (yet): gdb has no clue at all
> about the CPU mode as it has no clue about segments or control registers.
>
> > IIRC There's also a gdb option to override the fallback mode.
>
> For x86, the core of the issue is a decoupled control of the gdb remote
> protocol and the disassembly mode. I guess I have to dig a bit in the
> code to see if the hard coupling we see in practice can be broken up.
> Not according to the command help I found so far.
This is sounding a lot like "we can't be bothered fixing gdb, so we're going
to add ugly hacks to qemu instead". I don't find this a particularly
convincing argument.
Paul
^ permalink raw reply [flat|nested] 37+ messages in thread
* [Qemu-devel] Re: [PATCH v5 18/18] gdbstub: x86: Switch 64/32 bit registers dynamically
2008-11-19 0:06 ` Paul Brook
@ 2008-11-19 9:38 ` Jan Kiszka
0 siblings, 0 replies; 37+ messages in thread
From: Jan Kiszka @ 2008-11-19 9:38 UTC (permalink / raw)
To: qemu-devel
Paul Brook wrote:
>>> In the absence of real information gdb falls back to the current CPU
>>> mode, which is a bit in the CPU status register. Exactly which
>>> register/bit depends whether you're talking to an M-profile device.
>>> M-profile cores are identified based on the XML register descriptions. If
>>> you don't have an XML capable target then you don't get to debug
>>> M-profile devices.
>> ...and this surely doesn't map on x86 (yet): gdb has no clue at all
>> about the CPU mode as it has no clue about segments or control registers.
>>
>>> IIRC There's also a gdb option to override the fallback mode.
>> For x86, the core of the issue is a decoupled control of the gdb remote
>> protocol and the disassembly mode. I guess I have to dig a bit in the
>> code to see if the hard coupling we see in practice can be broken up.
>> Not according to the command help I found so far.
>
> This is sounding a lot like "we can't be bothered fixing gdb, so we're going
> to add ugly hacks to qemu instead". I don't find this a particularly
> convincing argument.
I'll be the last to refuse getting rid of this switching once there is a
released gdb version which is smarter. But right now there is not even a
smart CVS revision.
Although gdb does have separate current_gdbarch (used by the
disassembler) and target_gdbarch (used by the remote connector),
gdbarch_update_p always changes both of them when invoking 'set arch'.
There are comments indicating that they are working on making gdb more
flexible here for at least 5 years now. Frankly, this doesn't suggest
that we'll see an improvement tomorrow nor that it would be easy for us
to provide some ad-hoc gdb patches to accelerate this. Nevertheless,
this topic will surely come up for us again when we'll try to integrate
our segmentation enhancements for gdb.
But for now we face a regression of qemu-system-x86_64 that only allows
to use it in long mode. And that is what my patch fixes again. I really
don't see why we should keep it broken until gdb is fixed just because
the concept is a bit "ugly".
Jan
--
Siemens AG, Corporate Technology, CT SE 2 ES-OS
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v5 12/18] Introduce BP_CPU as a breakpoint type
2008-11-17 16:18 [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Jan Kiszka
` (9 preceding siblings ...)
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 18/18] gdbstub: x86: Switch 64/32 bit registers dynamically Jan Kiszka
@ 2008-11-17 16:18 ` Jan Kiszka
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 11/18] Add debug exception hook Jan Kiszka
` (7 subsequent siblings)
18 siblings, 0 replies; 37+ messages in thread
From: Jan Kiszka @ 2008-11-17 16:18 UTC (permalink / raw)
To: qemu-devel
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(-)
diff --git a/cpu-all.h b/cpu-all.h
index e97507f..aa4b770 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -768,6 +768,7 @@ void cpu_reset_interrupt(CPUState *env, int mask);
#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);
diff --git a/exec.c b/exec.c
index 0fe18b0..a90d076 100644
--- a/exec.c
+++ b/exec.c
@@ -1302,7 +1302,7 @@ int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
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)) {
@@ -1318,11 +1318,26 @@ int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
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);
@@ -1378,7 +1393,7 @@ int cpu_breakpoint_insert(CPUState *env, target_ulong pc, int flags,
CPUBreakpoint **breakpoint)
{
#if defined(TARGET_HAS_ICE)
- CPUBreakpoint *bp;
+ CPUBreakpoint *bp, *prev_bp;
bp = qemu_malloc(sizeof(*bp));
if (!bp)
@@ -1387,11 +1402,26 @@ int cpu_breakpoint_insert(CPUState *env, target_ulong pc, int flags,
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 related [flat|nested] 37+ messages in thread* [Qemu-devel] [PATCH v5 11/18] Add debug exception hook
2008-11-17 16:18 [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Jan Kiszka
` (10 preceding siblings ...)
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 12/18] Introduce BP_CPU as a breakpoint type Jan Kiszka
@ 2008-11-17 16:18 ` Jan Kiszka
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 17/18] gdbstub: x86: Support for setting segment registers Jan Kiszka
` (6 subsequent siblings)
18 siblings, 0 replies; 37+ messages in thread
From: Jan Kiszka @ 2008-11-17 16:18 UTC (permalink / raw)
To: qemu-devel
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(+), 0 deletions(-)
diff --git a/cpu-exec.c b/cpu-exec.c
index 191d9e4..8d86527 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -183,6 +183,16 @@ static inline TranslationBlock *tb_find_fast(void)
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;
@@ -190,6 +200,9 @@ static void cpu_handle_debug_exception(CPUState *env)
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 */
diff --git a/exec-all.h b/exec-all.h
index e3da98a..aec318b 100644
--- a/exec-all.h
+++ b/exec-all.h
@@ -387,4 +387,8 @@ static inline int kqemu_is_ok(CPUState *env)
}
#endif
+
+typedef void (CPUDebugExcpHandler)(CPUState *env);
+
+CPUDebugExcpHandler *cpu_set_debug_excp_handler(CPUDebugExcpHandler *handler);
#endif
^ permalink raw reply related [flat|nested] 37+ messages in thread* [Qemu-devel] [PATCH v5 17/18] gdbstub: x86: Support for setting segment registers
2008-11-17 16:18 [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Jan Kiszka
` (11 preceding siblings ...)
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 11/18] Add debug exception hook Jan Kiszka
@ 2008-11-17 16:18 ` Jan Kiszka
2008-11-18 21:19 ` Anthony Liguori
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 14/18] x86: Dump debug registers Jan Kiszka
` (5 subsequent siblings)
18 siblings, 1 reply; 37+ messages in thread
From: Jan Kiszka @ 2008-11-17 16:18 UTC (permalink / raw)
To: qemu-devel
This allows to set segment registers via gdb also in system emulation
mode. Basic sanity checks are applied and nothing is changed if they
fail. But screwing up the target via this interface will never be
complicated, so I avoided being too paranoid here.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
gdbstub.c | 60 ++++++++++++++++++++++++++++++++++-------------
target-i386/cpu.h | 14 +++++++++++
target-i386/op_helper.c | 14 -----------
3 files changed, 57 insertions(+), 31 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index fd4d5db..c83b76a 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -358,6 +358,43 @@ static int cpu_gdb_read_register(CPUState *env, uint8_t *mem_buf, int n)
return 0;
}
+static int cpu_x86_gdb_load_seg(CPUState *env, int sreg, uint8_t *mem_buf)
+{
+ uint16_t selector = ldl_p(mem_buf);
+
+ if (selector != env->segs[sreg].selector) {
+#if defined(CONFIG_USER_ONLY)
+ cpu_x86_load_seg(env, sreg, selector);
+#else
+ SegmentCache *dt;
+ target_ulong ptr;
+ uint32_t e1, e2;
+ int index;
+
+ if (!(env->cr[0] & CR0_PE_MASK) || (env->eflags & VM_MASK)) {
+ cpu_x86_load_seg_cache(env, sreg, selector, selector << 4,
+ 0xffff, 0);
+ } else {
+ if (selector & 0x4)
+ dt = &env->ldt;
+ else
+ dt = &env->gdt;
+ index = selector & ~7;
+ ptr = dt->base + index;
+ if ((index + 7) <= dt->limit &&
+ cpu_memory_rw_debug(env, ptr, (uint8_t *)&e1,
+ sizeof(e1), 0) == 0 &&
+ cpu_memory_rw_debug(env, ptr + 4, (uint8_t *)&e2,
+ sizeof(e2), 0) == 0)
+ cpu_x86_load_seg_cache(env, sreg, selector,
+ get_seg_base(e1, e2),
+ get_seg_limit(e1, e2), e2);
+ }
+#endif
+ }
+ return 4;
+}
+
static int cpu_gdb_write_register(CPUState *env, uint8_t *mem_buf, int n)
{
uint32_t tmp;
@@ -385,23 +422,12 @@ static int cpu_gdb_write_register(CPUState *env, uint8_t *mem_buf, int n)
env->eflags = ldl_p(mem_buf);
return 4;
-#if defined(CONFIG_USER_ONLY)
-#define LOAD_SEG(index, sreg)\
- tmp = ldl_p(mem_buf);\
- if (tmp != env->segs[sreg].selector)\
- cpu_x86_load_seg(env, sreg, tmp);\
- return 4
-#else
-/* FIXME: Honor segment registers. Needs to avoid raising an exception
- when the selector is invalid. */
-#define LOAD_SEG(index, sreg) return 4
-#endif
- case IDX_SEG_REGS: LOAD_SEG(10, R_CS);
- case IDX_SEG_REGS + 1: LOAD_SEG(11, R_SS);
- case IDX_SEG_REGS + 2: LOAD_SEG(12, R_DS);
- case IDX_SEG_REGS + 3: LOAD_SEG(13, R_ES);
- case IDX_SEG_REGS + 4: LOAD_SEG(14, R_FS);
- case IDX_SEG_REGS + 5: LOAD_SEG(15, R_GS);
+ case IDX_SEG_REGS: return cpu_x86_gdb_load_seg(env, R_CS, mem_buf);
+ case IDX_SEG_REGS + 1: return cpu_x86_gdb_load_seg(env, R_SS, mem_buf);
+ case IDX_SEG_REGS + 2: return cpu_x86_gdb_load_seg(env, R_DS, mem_buf);
+ case IDX_SEG_REGS + 3: return cpu_x86_gdb_load_seg(env, R_ES, mem_buf);
+ case IDX_SEG_REGS + 4: return cpu_x86_gdb_load_seg(env, R_FS, mem_buf);
+ case IDX_SEG_REGS + 5: return cpu_x86_gdb_load_seg(env, R_GS, mem_buf);
case IDX_FP_REGS + 8:
env->fpuc = ldl_p(mem_buf);
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index eed1f62..b7c8a2f 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -651,6 +651,20 @@ int cpu_get_pic_interrupt(CPUX86State *s);
/* MSDOS compatibility mode FPU exception support */
void cpu_set_ferr(CPUX86State *s);
+static inline unsigned int get_seg_limit(uint32_t e1, uint32_t e2)
+{
+ unsigned int limit;
+ limit = (e1 & 0xffff) | (e2 & 0x000f0000);
+ if (e2 & DESC_G_MASK)
+ limit = (limit << 12) | 0xfff;
+ return limit;
+}
+
+static inline uint32_t get_seg_base(uint32_t e1, uint32_t e2)
+{
+ return ((e1 >> 16) | ((e2 & 0xff) << 16) | (e2 & 0xff000000));
+}
+
/* this function must always be used to load data in the segment
cache: it synchronizes the hflags with the segment cache values */
static inline void cpu_x86_load_seg_cache(CPUX86State *env,
diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
index 6dc0802..091dc97 100644
--- a/target-i386/op_helper.c
+++ b/target-i386/op_helper.c
@@ -143,20 +143,6 @@ static inline int load_segment(uint32_t *e1_ptr, uint32_t *e2_ptr,
return 0;
}
-static inline unsigned int get_seg_limit(uint32_t e1, uint32_t e2)
-{
- unsigned int limit;
- limit = (e1 & 0xffff) | (e2 & 0x000f0000);
- if (e2 & DESC_G_MASK)
- limit = (limit << 12) | 0xfff;
- return limit;
-}
-
-static inline uint32_t get_seg_base(uint32_t e1, uint32_t e2)
-{
- return ((e1 >> 16) | ((e2 & 0xff) << 16) | (e2 & 0xff000000));
-}
-
static inline void load_seg_cache_raw_dt(SegmentCache *sc, uint32_t e1, uint32_t e2)
{
sc->base = get_seg_base(e1, e2);
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [Qemu-devel] [PATCH v5 17/18] gdbstub: x86: Support for setting segment registers
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 17/18] gdbstub: x86: Support for setting segment registers Jan Kiszka
@ 2008-11-18 21:19 ` Anthony Liguori
2008-11-18 23:15 ` [Qemu-devel] " Jan Kiszka
0 siblings, 1 reply; 37+ messages in thread
From: Anthony Liguori @ 2008-11-18 21:19 UTC (permalink / raw)
To: qemu-devel
Jan Kiszka wrote:
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index eed1f62..b7c8a2f 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -651,6 +651,20 @@ int cpu_get_pic_interrupt(CPUX86State *s);
> /* MSDOS compatibility mode FPU exception support */
> void cpu_set_ferr(CPUX86State *s);
>
> +static inline unsigned int get_seg_limit(uint32_t e1, uint32_t e2)
> +{
> + unsigned int limit;
> + limit = (e1 & 0xffff) | (e2 & 0x000f0000);
> + if (e2 & DESC_G_MASK)
> + limit = (limit << 12) | 0xfff;
> + return limit;
> +}
> +
> +static inline uint32_t get_seg_base(uint32_t e1, uint32_t e2)
> +{
> + return ((e1 >> 16) | ((e2 & 0xff) << 16) | (e2 & 0xff000000));
> +}
> +
>
I like this patch but if you're going to export new x86 helper
functions, please prefix them with cpu_x86. In this case, these helpers
are awfully low level (they're taking a GDT entry split into two 32-bit
words). I would rather see an interface that took a CPUState and a
segment register index. In the very least, it won't be very obvious to
anyone what this API expects to take so a comment would be required.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 37+ messages in thread* [Qemu-devel] Re: [PATCH v5 17/18] gdbstub: x86: Support for setting segment registers
2008-11-18 21:19 ` Anthony Liguori
@ 2008-11-18 23:15 ` Jan Kiszka
2008-11-19 14:24 ` Jan Kiszka
0 siblings, 1 reply; 37+ messages in thread
From: Jan Kiszka @ 2008-11-18 23:15 UTC (permalink / raw)
To: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1439 bytes --]
Anthony Liguori wrote:
> Jan Kiszka wrote:
>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>> index eed1f62..b7c8a2f 100644
>> --- a/target-i386/cpu.h
>> +++ b/target-i386/cpu.h
>> @@ -651,6 +651,20 @@ int cpu_get_pic_interrupt(CPUX86State *s);
>> /* MSDOS compatibility mode FPU exception support */
>> void cpu_set_ferr(CPUX86State *s);
>>
>> +static inline unsigned int get_seg_limit(uint32_t e1, uint32_t e2)
>> +{
>> + unsigned int limit;
>> + limit = (e1 & 0xffff) | (e2 & 0x000f0000);
>> + if (e2 & DESC_G_MASK)
>> + limit = (limit << 12) | 0xfff;
>> + return limit;
>> +}
>> +
>> +static inline uint32_t get_seg_base(uint32_t e1, uint32_t e2)
>> +{
>> + return ((e1 >> 16) | ((e2 & 0xff) << 16) | (e2 & 0xff000000));
>> +}
>> +
>>
>
> I like this patch but if you're going to export new x86 helper
> functions, please prefix them with cpu_x86. In this case, these helpers
> are awfully low level (they're taking a GDT entry split into two 32-bit
> words). I would rather see an interface that took a CPUState and a
> segment register index. In the very least, it won't be very obvious to
> anyone what this API expects to take so a comment would be required.
That was a result of the decision process "quick feature enhancement or
also some more refactoring?". :)
Will think out a new interface instead, leaving those two where they
came from.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread* [Qemu-devel] Re: [PATCH v5 17/18] gdbstub: x86: Support for setting segment registers
2008-11-18 23:15 ` [Qemu-devel] " Jan Kiszka
@ 2008-11-19 14:24 ` Jan Kiszka
0 siblings, 0 replies; 37+ messages in thread
From: Jan Kiszka @ 2008-11-19 14:24 UTC (permalink / raw)
To: qemu-devel
Jan Kiszka wrote:
> Anthony Liguori wrote:
>> Jan Kiszka wrote:
>>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>>> index eed1f62..b7c8a2f 100644
>>> --- a/target-i386/cpu.h
>>> +++ b/target-i386/cpu.h
>>> @@ -651,6 +651,20 @@ int cpu_get_pic_interrupt(CPUX86State *s);
>>> /* MSDOS compatibility mode FPU exception support */
>>> void cpu_set_ferr(CPUX86State *s);
>>>
>>> +static inline unsigned int get_seg_limit(uint32_t e1, uint32_t e2)
>>> +{
>>> + unsigned int limit;
>>> + limit = (e1 & 0xffff) | (e2 & 0x000f0000);
>>> + if (e2 & DESC_G_MASK)
>>> + limit = (limit << 12) | 0xfff;
>>> + return limit;
>>> +}
>>> +
>>> +static inline uint32_t get_seg_base(uint32_t e1, uint32_t e2)
>>> +{
>>> + return ((e1 >> 16) | ((e2 & 0xff) << 16) | (e2 & 0xff000000));
>>> +}
>>> +
>>>
>> I like this patch but if you're going to export new x86 helper
>> functions, please prefix them with cpu_x86. In this case, these helpers
>> are awfully low level (they're taking a GDT entry split into two 32-bit
>> words). I would rather see an interface that took a CPUState and a
>> segment register index. In the very least, it won't be very obvious to
>> anyone what this API expects to take so a comment would be required.
>
> That was a result of the decision process "quick feature enhancement or
> also some more refactoring?". :)
>
> Will think out a new interface instead, leaving those two where they
> came from.
>
[Now with new, shiny cpu_x86_get_descr_debug interface.]
This allows to set segment registers via gdb also in system emulation
mode. Basic sanity checks are applied and nothing is changed if they
fail. But screwing up the target via this interface will never be
complicated, so I avoided being too paranoid here.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
gdbstub.c | 48 +++++++++++++++++++++++++++++++-----------------
target-i386/cpu.h | 4 ++++
target-i386/helper.c | 30 ++++++++++++++++++++++++++++++
3 files changed, 65 insertions(+), 17 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index fd4d5db..89efa3f 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -358,6 +358,31 @@ static int cpu_gdb_read_register(CPUState *env, uint8_t *mem_buf, int n)
return 0;
}
+static int cpu_x86_gdb_load_seg(CPUState *env, int sreg, uint8_t *mem_buf)
+{
+ uint16_t selector = ldl_p(mem_buf);
+
+ if (selector != env->segs[sreg].selector) {
+#if defined(CONFIG_USER_ONLY)
+ cpu_x86_load_seg(env, sreg, selector);
+#else
+ unsigned int limit, flags;
+ target_ulong base;
+
+ if (!(env->cr[0] & CR0_PE_MASK) || (env->eflags & VM_MASK)) {
+ base = selector << 4;
+ limit = 0xffff;
+ flags = 0;
+ } else {
+ if (!cpu_x86_get_descr_debug(env, selector, &base, &limit, &flags))
+ return 4;
+ }
+ cpu_x86_load_seg_cache(env, sreg, selector, base, limit, flags);
+#endif
+ }
+ return 4;
+}
+
static int cpu_gdb_write_register(CPUState *env, uint8_t *mem_buf, int n)
{
uint32_t tmp;
@@ -385,23 +410,12 @@ static int cpu_gdb_write_register(CPUState *env, uint8_t *mem_buf, int n)
env->eflags = ldl_p(mem_buf);
return 4;
-#if defined(CONFIG_USER_ONLY)
-#define LOAD_SEG(index, sreg)\
- tmp = ldl_p(mem_buf);\
- if (tmp != env->segs[sreg].selector)\
- cpu_x86_load_seg(env, sreg, tmp);\
- return 4
-#else
-/* FIXME: Honor segment registers. Needs to avoid raising an exception
- when the selector is invalid. */
-#define LOAD_SEG(index, sreg) return 4
-#endif
- case IDX_SEG_REGS: LOAD_SEG(10, R_CS);
- case IDX_SEG_REGS + 1: LOAD_SEG(11, R_SS);
- case IDX_SEG_REGS + 2: LOAD_SEG(12, R_DS);
- case IDX_SEG_REGS + 3: LOAD_SEG(13, R_ES);
- case IDX_SEG_REGS + 4: LOAD_SEG(14, R_FS);
- case IDX_SEG_REGS + 5: LOAD_SEG(15, R_GS);
+ case IDX_SEG_REGS: return cpu_x86_gdb_load_seg(env, R_CS, mem_buf);
+ case IDX_SEG_REGS + 1: return cpu_x86_gdb_load_seg(env, R_SS, mem_buf);
+ case IDX_SEG_REGS + 2: return cpu_x86_gdb_load_seg(env, R_DS, mem_buf);
+ case IDX_SEG_REGS + 3: return cpu_x86_gdb_load_seg(env, R_ES, mem_buf);
+ case IDX_SEG_REGS + 4: return cpu_x86_gdb_load_seg(env, R_FS, mem_buf);
+ case IDX_SEG_REGS + 5: return cpu_x86_gdb_load_seg(env, R_GS, mem_buf);
case IDX_FP_REGS + 8:
env->fpuc = ldl_p(mem_buf);
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index eed1f62..947c178 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -710,6 +710,10 @@ static inline void cpu_x86_load_seg_cache(CPUX86State *env,
}
}
+int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
+ target_ulong *base, unsigned int *limit,
+ unsigned int *flags);
+
/* wrapper, just in case memory mappings must be changed */
static inline void cpu_x86_set_cpl(CPUX86State *s, int cpl)
{
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 037540d..4194be9 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1614,6 +1614,36 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
}
}
+
+int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
+ target_ulong *base, unsigned int *limit,
+ unsigned int *flags)
+{
+ SegmentCache *dt;
+ target_ulong ptr;
+ uint32_t e1, e2;
+ int index;
+
+ if (selector & 0x4)
+ dt = &env->ldt;
+ else
+ dt = &env->gdt;
+ index = selector & ~7;
+ ptr = dt->base + index;
+ if ((index + 7) > dt->limit
+ || cpu_memory_rw_debug(env, ptr, (uint8_t *)&e1, sizeof(e1), 0) != 0
+ || cpu_memory_rw_debug(env, ptr+4, (uint8_t *)&e2, sizeof(e2), 0) != 0)
+ return 0;
+
+ *base = ((e1 >> 16) | ((e2 & 0xff) << 16) | (e2 & 0xff000000));
+ *limit = (e1 & 0xffff) | (e2 & 0x000f0000);
+ if (e2 & DESC_G_MASK)
+ *limit = (*limit << 12) | 0xfff;
+ *flags = e2;
+
+ return 1;
+}
+
CPUX86State *cpu_x86_init(const char *cpu_model)
{
CPUX86State *env;
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v5 14/18] x86: Dump debug registers
2008-11-17 16:18 [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Jan Kiszka
` (12 preceding siblings ...)
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 17/18] gdbstub: x86: Support for setting segment registers Jan Kiszka
@ 2008-11-17 16:18 ` Jan Kiszka
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 15/18] gdbstub: Add vCont support Jan Kiszka
` (4 subsequent siblings)
18 siblings, 0 replies; 37+ messages in thread
From: Jan Kiszka @ 2008-11-17 16:18 UTC (permalink / raw)
To: qemu-devel
As the debug registers are no longer dummies, let's include their
current state into the 'info registers' output and other register dumps.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
target-i386/helper.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 0cfef44..2a61cb0 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -644,6 +644,10 @@ void cpu_dump_state(CPUState *env, FILE *f,
env->cr[2],
env->cr[3],
(uint32_t)env->cr[4]);
+ for(i = 0; i < 4; i++)
+ cpu_fprintf(f, "DR%d=%016" PRIx64 " ", i, env->dr[i]);
+ cpu_fprintf(f, "\nDR6=%016" PRIx64 " DR7=%016" PRIx64 "\n",
+ env->dr[6], env->cr[7]);
} else
#endif
{
@@ -675,6 +679,9 @@ void cpu_dump_state(CPUState *env, FILE *f,
(uint32_t)env->cr[2],
(uint32_t)env->cr[3],
(uint32_t)env->cr[4]);
+ for(i = 0; i < 4; i++)
+ cpu_fprintf(f, "DR%d=%08x ", i, env->dr[i]);
+ cpu_fprintf(f, "\nDR6=%08x DR7=%08x\n", env->dr[6], env->cr[7]);
}
if (flags & X86_DUMP_CCOP) {
if ((unsigned)env->cc_op < CC_OP_NB)
^ permalink raw reply related [flat|nested] 37+ messages in thread* [Qemu-devel] [PATCH v5 15/18] gdbstub: Add vCont support
2008-11-17 16:18 [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Jan Kiszka
` (13 preceding siblings ...)
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 14/18] x86: Dump debug registers Jan Kiszka
@ 2008-11-17 16:18 ` Jan Kiszka
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 10/18] Introduce BP_WATCHPOINT_HIT flag Jan Kiszka
` (3 subsequent siblings)
18 siblings, 0 replies; 37+ messages in thread
From: Jan Kiszka @ 2008-11-17 16:18 UTC (permalink / raw)
To: qemu-devel
In order to set the thread (CPU) for the next single-step command, you
need gdb 6.8 or better - and this patch. It introduces vCont support
which gdb uses to switch the debugging focus for single-stepping
multi-threaded targets.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
gdbstub.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 56 insertions(+), 0 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index b522a89..11c54eb 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1285,6 +1285,62 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
s->signal = strtoul(p, (char **)&p, 16);
gdb_continue(s);
return RS_IDLE;
+ case 'v':
+ if (strncmp(p, "Cont", 4) == 0) {
+ int res_signal, res_thread;
+
+ p += 4;
+ if (*p == '?') {
+ put_packet(s, "vCont;c;C;s;S");
+ break;
+ }
+ res = 0;
+ res_signal = 0;
+ res_thread = 0;
+ while (*p) {
+ int action, signal;
+
+ if (*p++ != ';') {
+ res = 0;
+ break;
+ }
+ action = *p++;
+ signal = 0;
+ if (action == 'C' || action == 'S')
+ signal = strtoul(p, (char **)&p, 16);
+ else if (action != 'c' && action != 's') {
+ res = 0;
+ break;
+ }
+ thread = 0;
+ if (*p == ':')
+ thread = strtoull(p+1, (char **)&p, 16);
+
+ action = tolower(action);
+ if (res == 0 || (res == 'c' && action == 's')) {
+ res = action;
+ res_signal = signal;
+ res_thread = thread;
+ }
+ }
+ if (res) {
+ if (res_thread != -1 && res_thread != 0) {
+ for (env = first_cpu; env != NULL; env = env->next_cpu)
+ if (env->cpu_index + 1 == res_thread)
+ break;
+ if (env == NULL) {
+ put_packet(s, "E22");
+ break;
+ }
+ s->c_cpu = env;
+ }
+ if (res == 's')
+ cpu_single_step(s->c_cpu, sstep_flags);
+ gdb_continue(s);
+ return RS_IDLE;
+ }
+ break;
+ }
case 'k':
/* Kill the target */
fprintf(stderr, "\nQEMU: Terminated via GDBstub\n");
^ permalink raw reply related [flat|nested] 37+ messages in thread* [Qemu-devel] [PATCH v5 10/18] Introduce BP_WATCHPOINT_HIT flag
2008-11-17 16:18 [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Jan Kiszka
` (14 preceding siblings ...)
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 15/18] gdbstub: Add vCont support Jan Kiszka
@ 2008-11-17 16:18 ` Jan Kiszka
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 16/18] gdbstub: x86: Refactor register access Jan Kiszka
` (2 subsequent siblings)
18 siblings, 0 replies; 37+ messages in thread
From: Jan Kiszka @ 2008-11-17 16:18 UTC (permalink / raw)
To: qemu-devel
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 | 35 ++++++++++++++++++++---------------
3 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/cpu-all.h b/cpu-all.h
index 0322955..e97507f 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -766,6 +766,7 @@ void cpu_reset_interrupt(CPUState *env, int mask);
#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,
diff --git a/cpu-exec.c b/cpu-exec.c
index 1955d24..191d9e4 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -183,6 +183,15 @@ static inline TranslationBlock *tb_find_fast(void)
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)
@@ -237,6 +246,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
diff --git a/exec.c b/exec.c
index b49162c..0fe18b0 100644
--- a/exec.c
+++ b/exec.c
@@ -1340,7 +1340,7 @@ int cpu_watchpoint_remove(CPUState *env, target_ulong addr, target_ulong len,
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;
}
@@ -2519,21 +2519,26 @@ static void check_watchpoint(int offset, int len_mask, int flags)
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);
- }
- 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, &pc, &cs_base, &cpu_flags);
- tb_gen_code(env, pc, cs_base, cpu_flags, 1);
+ 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, &pc, &cs_base, &cpu_flags);
+ tb_gen_code(env, pc, cs_base, cpu_flags, 1);
+ }
+ cpu_resume_from_signal(env, NULL);
}
- cpu_resume_from_signal(env, NULL);
+ } else {
+ wp->flags &= ~BP_WATCHPOINT_HIT;
}
}
}
^ permalink raw reply related [flat|nested] 37+ messages in thread* [Qemu-devel] [PATCH v5 16/18] gdbstub: x86: Refactor register access
2008-11-17 16:18 [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Jan Kiszka
` (15 preceding siblings ...)
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 10/18] Introduce BP_WATCHPOINT_HIT flag Jan Kiszka
@ 2008-11-17 16:18 ` Jan Kiszka
2008-11-18 21:15 ` Anthony Liguori
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 08/18] Remove premature memop TB terminations Jan Kiszka
2008-11-18 21:26 ` [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Anthony Liguori
18 siblings, 1 reply; 37+ messages in thread
From: Jan Kiszka @ 2008-11-17 16:18 UTC (permalink / raw)
To: qemu-devel
Clarify gdb's register set layout by using constants for
cpu_gdb_read/write_register.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
gdbstub.c | 156 ++++++++++++++++++++++++++++++++-----------------------------
1 files changed, 83 insertions(+), 73 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index 11c54eb..fd4d5db 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -306,111 +306,121 @@ static const int gpr_map[8] = {0, 1, 2, 3, 4, 5, 6, 7};
#define NUM_CORE_REGS (CPU_NB_REGS * 2 + 25)
+#define IDX_IP_REG CPU_NB_REGS
+#define IDX_FLAGS_REG (IDX_IP_REG + 1)
+#define IDX_SEG_REGS (IDX_FLAGS_REG + 1)
+#define IDX_FP_REGS (IDX_SEG_REGS + 6)
+#define IDX_XMM_REGS (IDX_FP_REGS + 16)
+#define IDX_MXCSR_REG (IDX_XMM_REGS + CPU_NB_REGS)
+
static int cpu_gdb_read_register(CPUState *env, uint8_t *mem_buf, int n)
{
if (n < CPU_NB_REGS) {
GET_REGL(env->regs[gpr_map[n]]);
- } else if (n >= CPU_NB_REGS + 8 && n < CPU_NB_REGS + 16) {
- /* FIXME: byteswap float values. */
+ } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
#ifdef USE_X86LDOUBLE
- memcpy(mem_buf, &env->fpregs[n - (CPU_NB_REGS + 8)], 10);
+ /* FIXME: byteswap float values - after fixing fpregs layout. */
+ memcpy(mem_buf, &env->fpregs[n - IDX_FP_REGS], 10);
#else
memset(mem_buf, 0, 10);
#endif
return 10;
- } else if (n >= CPU_NB_REGS + 24) {
- n -= CPU_NB_REGS + 24;
- if (n < CPU_NB_REGS) {
- stq_p(mem_buf, env->xmm_regs[n].XMM_Q(0));
- stq_p(mem_buf + 8, env->xmm_regs[n].XMM_Q(1));
- return 16;
- } else if (n == CPU_NB_REGS) {
- GET_REG32(env->mxcsr);
- }
+ } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
+ n -= IDX_XMM_REGS;
+ stq_p(mem_buf, env->xmm_regs[n].XMM_Q(0));
+ stq_p(mem_buf + 8, env->xmm_regs[n].XMM_Q(1));
+ return 16;
} else {
- n -= CPU_NB_REGS;
switch (n) {
- case 0: GET_REGL(env->eip);
- case 1: GET_REG32(env->eflags);
- case 2: GET_REG32(env->segs[R_CS].selector);
- case 3: GET_REG32(env->segs[R_SS].selector);
- case 4: GET_REG32(env->segs[R_DS].selector);
- case 5: GET_REG32(env->segs[R_ES].selector);
- case 6: GET_REG32(env->segs[R_FS].selector);
- case 7: GET_REG32(env->segs[R_GS].selector);
- /* 8...15 x87 regs. */
- case 16: GET_REG32(env->fpuc);
- case 17: GET_REG32((env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11);
- case 18: GET_REG32(0); /* ftag */
- case 19: GET_REG32(0); /* fiseg */
- case 20: GET_REG32(0); /* fioff */
- case 21: GET_REG32(0); /* foseg */
- case 22: GET_REG32(0); /* fooff */
- case 23: GET_REG32(0); /* fop */
- /* 24+ xmm regs. */
+ case IDX_IP_REG: GET_REGL(env->eip);
+ case IDX_FLAGS_REG: GET_REG32(env->eflags);
+
+ case IDX_SEG_REGS: GET_REG32(env->segs[R_CS].selector);
+ case IDX_SEG_REGS + 1: GET_REG32(env->segs[R_SS].selector);
+ case IDX_SEG_REGS + 2: GET_REG32(env->segs[R_DS].selector);
+ case IDX_SEG_REGS + 3: GET_REG32(env->segs[R_ES].selector);
+ case IDX_SEG_REGS + 4: GET_REG32(env->segs[R_FS].selector);
+ case IDX_SEG_REGS + 5: GET_REG32(env->segs[R_GS].selector);
+
+ case IDX_FP_REGS + 8: GET_REG32(env->fpuc);
+ case IDX_FP_REGS + 9: GET_REG32((env->fpus & ~0x3800) |
+ (env->fpstt & 0x7) << 11);
+ case IDX_FP_REGS + 10: GET_REG32(0); /* ftag */
+ case IDX_FP_REGS + 11: GET_REG32(0); /* fiseg */
+ case IDX_FP_REGS + 12: GET_REG32(0); /* fioff */
+ case IDX_FP_REGS + 13: GET_REG32(0); /* foseg */
+ case IDX_FP_REGS + 14: GET_REG32(0); /* fooff */
+ case IDX_FP_REGS + 15: GET_REG32(0); /* fop */
+
+ case IDX_MXCSR_REG: GET_REG32(env->mxcsr);
}
}
return 0;
}
-static int cpu_gdb_write_register(CPUState *env, uint8_t *mem_buf, int i)
+static int cpu_gdb_write_register(CPUState *env, uint8_t *mem_buf, int n)
{
uint32_t tmp;
- if (i < CPU_NB_REGS) {
- env->regs[gpr_map[i]] = ldtul_p(mem_buf);
+ if (n < CPU_NB_REGS) {
+ env->regs[gpr_map[n]] = ldtul_p(mem_buf);
return sizeof(target_ulong);
- } else if (i >= CPU_NB_REGS + 8 && i < CPU_NB_REGS + 16) {
- i -= CPU_NB_REGS + 8;
+ } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
#ifdef USE_X86LDOUBLE
- memcpy(&env->fpregs[i], mem_buf, 10);
+ /* FIXME: byteswap float values - after fixing fpregs layout. */
+ memcpy(&env->fpregs[n - IDX_FP_REGS], mem_buf, 10);
#endif
return 10;
- } else if (i >= CPU_NB_REGS + 24) {
- i -= CPU_NB_REGS + 24;
- if (i < CPU_NB_REGS) {
- env->xmm_regs[i].XMM_Q(0) = ldq_p(mem_buf);
- env->xmm_regs[i].XMM_Q(1) = ldq_p(mem_buf + 8);
- return 16;
- } else if (i == CPU_NB_REGS) {
- env->mxcsr = ldl_p(mem_buf);
- return 4;
- }
+ } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
+ n -= IDX_XMM_REGS;
+ env->xmm_regs[n].XMM_Q(0) = ldq_p(mem_buf);
+ env->xmm_regs[n].XMM_Q(1) = ldq_p(mem_buf + 8);
+ return 16;
} else {
- i -= CPU_NB_REGS;
- switch (i) {
- case 0: env->eip = ldtul_p(mem_buf); return sizeof(target_ulong);
- case 1: env->eflags = ldl_p(mem_buf); return 4;
+ switch (n) {
+ case IDX_IP_REG:
+ env->eip = ldtul_p(mem_buf);
+ return sizeof(target_ulong);
+ case IDX_FLAGS_REG:
+ env->eflags = ldl_p(mem_buf);
+ return 4;
+
#if defined(CONFIG_USER_ONLY)
#define LOAD_SEG(index, sreg)\
tmp = ldl_p(mem_buf);\
if (tmp != env->segs[sreg].selector)\
- cpu_x86_load_seg(env, sreg, tmp);
+ cpu_x86_load_seg(env, sreg, tmp);\
+ return 4
#else
/* FIXME: Honor segment registers. Needs to avoid raising an exception
when the selector is invalid. */
-#define LOAD_SEG(index, sreg) do {} while(0)
+#define LOAD_SEG(index, sreg) return 4
#endif
- case 2: LOAD_SEG(10, R_CS); return 4;
- case 3: LOAD_SEG(11, R_SS); return 4;
- case 4: LOAD_SEG(12, R_DS); return 4;
- case 5: LOAD_SEG(13, R_ES); return 4;
- case 6: LOAD_SEG(14, R_FS); return 4;
- case 7: LOAD_SEG(15, R_GS); return 4;
- /* 8...15 x87 regs. */
- case 16: env->fpuc = ldl_p(mem_buf); return 4;
- case 17:
- tmp = ldl_p(mem_buf);
- env->fpstt = (tmp >> 11) & 7;
- env->fpus = tmp & ~0x3800;
- return 4;
- case 18: /* ftag */ return 4;
- case 19: /* fiseg */ return 4;
- case 20: /* fioff */ return 4;
- case 21: /* foseg */ return 4;
- case 22: /* fooff */ return 4;
- case 23: /* fop */ return 4;
- /* 24+ xmm regs. */
+ case IDX_SEG_REGS: LOAD_SEG(10, R_CS);
+ case IDX_SEG_REGS + 1: LOAD_SEG(11, R_SS);
+ case IDX_SEG_REGS + 2: LOAD_SEG(12, R_DS);
+ case IDX_SEG_REGS + 3: LOAD_SEG(13, R_ES);
+ case IDX_SEG_REGS + 4: LOAD_SEG(14, R_FS);
+ case IDX_SEG_REGS + 5: LOAD_SEG(15, R_GS);
+
+ case IDX_FP_REGS + 8:
+ env->fpuc = ldl_p(mem_buf);
+ return 4;
+ case IDX_FP_REGS + 9:
+ tmp = ldl_p(mem_buf);
+ env->fpstt = (tmp >> 11) & 7;
+ env->fpus = tmp & ~0x3800;
+ return 4;
+ case IDX_FP_REGS + 10: /* ftag */ return 4;
+ case IDX_FP_REGS + 11: /* fiseg */ return 4;
+ case IDX_FP_REGS + 12: /* fioff */ return 4;
+ case IDX_FP_REGS + 13: /* foseg */ return 4;
+ case IDX_FP_REGS + 14: /* fooff */ return 4;
+ case IDX_FP_REGS + 15: /* fop */ return 4;
+
+ case IDX_MXCSR_REG:
+ env->mxcsr = ldl_p(mem_buf);
+ return 4;
}
}
/* Unrecognised register. */
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [Qemu-devel] [PATCH v5 16/18] gdbstub: x86: Refactor register access
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 16/18] gdbstub: x86: Refactor register access Jan Kiszka
@ 2008-11-18 21:15 ` Anthony Liguori
2008-11-18 23:12 ` [Qemu-devel] " Jan Kiszka
0 siblings, 1 reply; 37+ messages in thread
From: Anthony Liguori @ 2008-11-18 21:15 UTC (permalink / raw)
To: qemu-devel
Jan Kiszka wrote:
> return 10;
> - } else if (n >= CPU_NB_REGS + 24) {
> - n -= CPU_NB_REGS + 24;
> - if (n < CPU_NB_REGS) {
> - stq_p(mem_buf, env->xmm_regs[n].XMM_Q(0));
> - stq_p(mem_buf + 8, env->xmm_regs[n].XMM_Q(1));
> - return 16;
> - } else if (n == CPU_NB_REGS) {
> - GET_REG32(env->mxcsr);
> - }
> + } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
> + n -= IDX_XMM_REGS;
> + stq_p(mem_buf, env->xmm_regs[n].XMM_Q(0));
> + stq_p(mem_buf + 8, env->xmm_regs[n].XMM_Q(1));
> + return 16;
>
I think you have too much going on in this patch to review it properly.
It's not immediately obvious to me that this change results in identical
code.
> } else {
> - n -= CPU_NB_REGS;
> switch (n) {
> - case 0: GET_REGL(env->eip);
> - case 1: GET_REG32(env->eflags);
> - case 2: GET_REG32(env->segs[R_CS].selector);
> - case 3: GET_REG32(env->segs[R_SS].selector);
> - case 4: GET_REG32(env->segs[R_DS].selector);
> - case 5: GET_REG32(env->segs[R_ES].selector);
> - case 6: GET_REG32(env->segs[R_FS].selector);
> - case 7: GET_REG32(env->segs[R_GS].selector);
> - /* 8...15 x87 regs. */
> - case 16: GET_REG32(env->fpuc);
> - case 17: GET_REG32((env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11);
> - case 18: GET_REG32(0); /* ftag */
> - case 19: GET_REG32(0); /* fiseg */
> - case 20: GET_REG32(0); /* fioff */
> - case 21: GET_REG32(0); /* foseg */
> - case 22: GET_REG32(0); /* fooff */
> - case 23: GET_REG32(0); /* fop */
> - /* 24+ xmm regs. */
> + case IDX_IP_REG: GET_REGL(env->eip);
> + case IDX_FLAGS_REG: GET_REG32(env->eflags);
> +
> + case IDX_SEG_REGS: GET_REG32(env->segs[R_CS].selector);
> + case IDX_SEG_REGS + 1: GET_REG32(env->segs[R_SS].selector);
> + case IDX_SEG_REGS + 2: GET_REG32(env->segs[R_DS].selector);
> + case IDX_SEG_REGS + 3: GET_REG32(env->segs[R_ES].selector);
> + case IDX_SEG_REGS + 4: GET_REG32(env->segs[R_FS].selector);
> + case IDX_SEG_REGS + 5: GET_REG32(env->segs[R_GS].selector);
> +
> + case IDX_FP_REGS + 8: GET_REG32(env->fpuc);
> + case IDX_FP_REGS + 9: GET_REG32((env->fpus & ~0x3800) |
> + (env->fpstt & 0x7) << 11);
> + case IDX_FP_REGS + 10: GET_REG32(0); /* ftag */
> + case IDX_FP_REGS + 11: GET_REG32(0); /* fiseg */
> + case IDX_FP_REGS + 12: GET_REG32(0); /* fioff */
> + case IDX_FP_REGS + 13: GET_REG32(0); /* foseg */
> + case IDX_FP_REGS + 14: GET_REG32(0); /* fooff */
> + case IDX_FP_REGS + 15: GET_REG32(0); /* fop */
> +
> + case IDX_MXCSR_REG: GET_REG32(env->mxcsr);
> }
> }
> return 0;
> }
>
> -static int cpu_gdb_write_register(CPUState *env, uint8_t *mem_buf, int i)
> +static int cpu_gdb_write_register(CPUState *env, uint8_t *mem_buf, int n)
>
Is this rename really necessary?
> #define LOAD_SEG(index, sreg)\
> tmp = ldl_p(mem_buf);\
> if (tmp != env->segs[sreg].selector)\
> - cpu_x86_load_seg(env, sreg, tmp);
> + cpu_x86_load_seg(env, sreg, tmp);\
> + return 4
> #else
> /* FIXME: Honor segment registers. Needs to avoid raising an exception
> when the selector is invalid. */
> -#define LOAD_SEG(index, sreg) do {} while(0)
> +#define LOAD_SEG(index, sreg) return 4
> #endif
>
I don't like the idea of hiding a return in a LOAD_SEG thing. You would
expect for these cases to fall through unless you look at the macro
definition.
Code cleanup patches are good, but please try and split them in such a
way that they are easier to review. Things like variable renames or
introductions of #define's should be completely mechanical. If you want
to reswizzle code, please separate that into a separate patch. That
keeps the later smaller which requires a more fine review.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 37+ messages in thread* [Qemu-devel] Re: [PATCH v5 16/18] gdbstub: x86: Refactor register access
2008-11-18 21:15 ` Anthony Liguori
@ 2008-11-18 23:12 ` Jan Kiszka
0 siblings, 0 replies; 37+ messages in thread
From: Jan Kiszka @ 2008-11-18 23:12 UTC (permalink / raw)
To: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5259 bytes --]
Anthony Liguori wrote:
> Jan Kiszka wrote:
>> return 10;
>> - } else if (n >= CPU_NB_REGS + 24) {
>> - n -= CPU_NB_REGS + 24;
>> - if (n < CPU_NB_REGS) {
>> - stq_p(mem_buf, env->xmm_regs[n].XMM_Q(0));
>> - stq_p(mem_buf + 8, env->xmm_regs[n].XMM_Q(1));
>> - return 16;
>> - } else if (n == CPU_NB_REGS) {
>> - GET_REG32(env->mxcsr);
>> - } + } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS +
>> CPU_NB_REGS) {
>> + n -= IDX_XMM_REGS;
>> + stq_p(mem_buf, env->xmm_regs[n].XMM_Q(0));
>> + stq_p(mem_buf + 8, env->xmm_regs[n].XMM_Q(1));
>> + return 16;
>>
>
> I think you have too much going on in this patch to review it properly.
> It's not immediately obvious to me that this change results in identical
> code.
I would say this is not only because of the changes...
>
>> } else {
>> - n -= CPU_NB_REGS;
>> switch (n) {
>> - case 0: GET_REGL(env->eip);
>> - case 1: GET_REG32(env->eflags);
>> - case 2: GET_REG32(env->segs[R_CS].selector);
>> - case 3: GET_REG32(env->segs[R_SS].selector);
>> - case 4: GET_REG32(env->segs[R_DS].selector);
>> - case 5: GET_REG32(env->segs[R_ES].selector);
>> - case 6: GET_REG32(env->segs[R_FS].selector);
>> - case 7: GET_REG32(env->segs[R_GS].selector);
>> - /* 8...15 x87 regs. */
>> - case 16: GET_REG32(env->fpuc);
>> - case 17: GET_REG32((env->fpus & ~0x3800) | (env->fpstt & 0x7)
>> << 11);
>> - case 18: GET_REG32(0); /* ftag */
>> - case 19: GET_REG32(0); /* fiseg */
>> - case 20: GET_REG32(0); /* fioff */
>> - case 21: GET_REG32(0); /* foseg */
>> - case 22: GET_REG32(0); /* fooff */
>> - case 23: GET_REG32(0); /* fop */
>> - /* 24+ xmm regs. */
>> + case IDX_IP_REG: GET_REGL(env->eip);
>> + case IDX_FLAGS_REG: GET_REG32(env->eflags);
>> +
>> + case IDX_SEG_REGS: GET_REG32(env->segs[R_CS].selector);
>> + case IDX_SEG_REGS + 1: GET_REG32(env->segs[R_SS].selector);
>> + case IDX_SEG_REGS + 2: GET_REG32(env->segs[R_DS].selector);
>> + case IDX_SEG_REGS + 3: GET_REG32(env->segs[R_ES].selector);
>> + case IDX_SEG_REGS + 4: GET_REG32(env->segs[R_FS].selector);
>> + case IDX_SEG_REGS + 5: GET_REG32(env->segs[R_GS].selector);
>> +
>> + case IDX_FP_REGS + 8: GET_REG32(env->fpuc);
>> + case IDX_FP_REGS + 9: GET_REG32((env->fpus & ~0x3800) |
>> + (env->fpstt & 0x7) << 11);
>> + case IDX_FP_REGS + 10: GET_REG32(0); /* ftag */
>> + case IDX_FP_REGS + 11: GET_REG32(0); /* fiseg */
>> + case IDX_FP_REGS + 12: GET_REG32(0); /* fioff */
>> + case IDX_FP_REGS + 13: GET_REG32(0); /* foseg */
>> + case IDX_FP_REGS + 14: GET_REG32(0); /* fooff */
>> + case IDX_FP_REGS + 15: GET_REG32(0); /* fop */
>> +
>> + case IDX_MXCSR_REG: GET_REG32(env->mxcsr);
>> }
>> }
>> return 0;
>> }
>>
>> -static int cpu_gdb_write_register(CPUState *env, uint8_t *mem_buf,
>> int i)
>> +static int cpu_gdb_write_register(CPUState *env, uint8_t *mem_buf,
>> int n)
>>
>
> Is this rename really necessary?
It improves consistency of this code, readability ("Why was it called
'i' here, but 'n' above?").
>
>> #define LOAD_SEG(index, sreg)\
>> tmp = ldl_p(mem_buf);\
>> if (tmp != env->segs[sreg].selector)\
>> - cpu_x86_load_seg(env, sreg, tmp);
>> + cpu_x86_load_seg(env, sreg, tmp);\
>> + return 4
>> #else
>> /* FIXME: Honor segment registers. Needs to avoid raising an exception
>> when the selector is invalid. */
>> -#define LOAD_SEG(index, sreg) do {} while(0)
>> +#define LOAD_SEG(index, sreg) return 4
>> #endif
>>
>
> I don't like the idea of hiding a return in a LOAD_SEG thing. You would
> expect for these cases to fall through unless you look at the macro
> definition.
The macro fortunately dies with the next patch. So you may argue I
should leave that part alone and simply remove it later...
BTW, there are more of such macros. Changing them would have caused more
churn than I felt like I should cause.
>
> Code cleanup patches are good, but please try and split them in such a
> way that they are easier to review. Things like variable renames or
> introductions of #define's should be completely mechanical. If you want
> to reswizzle code, please separate that into a separate patch. That
> keeps the later smaller which requires a more fine review.
Well, this is always a trade-off: Leave the code as-is, just add the
functionality that I need right now, or also attempt to clean up what
caused confusing or nagged me otherwise. But the latter can easily cost
much more than the former. Up to a certain point I agree with your aim,
but up from a certain point of split up I would have to fall back to the
first approach due to time constraints.
So please let me know what you think this one should be split up into.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread
* [Qemu-devel] [PATCH v5 08/18] Remove premature memop TB terminations
2008-11-17 16:18 [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Jan Kiszka
` (16 preceding siblings ...)
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 16/18] gdbstub: x86: Refactor register access Jan Kiszka
@ 2008-11-17 16:18 ` Jan Kiszka
2008-11-18 21:26 ` [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Anthony Liguori
18 siblings, 0 replies; 37+ messages in thread
From: Jan Kiszka @ 2008-11-17 16:18 UTC (permalink / raw)
To: qemu-devel
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, 0 insertions(+), 16 deletions(-)
diff --git a/exec.c b/exec.c
index 0dd4aa3..b49162c 100644
--- a/exec.c
+++ b/exec.c
@@ -1325,10 +1325,6 @@ int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
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;
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 237c5f6..54eb067 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -8729,12 +8729,6 @@ static inline void gen_intermediate_code_internal(CPUState *env,
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
diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index 49e2cb2..a14f6c5 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -3027,12 +3027,6 @@ gen_intermediate_code_internal(CPUState *env, TranslationBlock *tb,
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 related [flat|nested] 37+ messages in thread* Re: [Qemu-devel] [PATCH v5 00/18] Enhance debugging support
2008-11-17 16:18 [Qemu-devel] [PATCH v5 00/18] Enhance debugging support Jan Kiszka
` (17 preceding siblings ...)
2008-11-17 16:18 ` [Qemu-devel] [PATCH v5 08/18] Remove premature memop TB terminations Jan Kiszka
@ 2008-11-18 21:26 ` Anthony Liguori
18 siblings, 0 replies; 37+ messages in thread
From: Anthony Liguori @ 2008-11-18 21:26 UTC (permalink / raw)
To: qemu-devel
Jan Kiszka wrote:
> Here is the latest update after the recent review round. Addressed
> issues:
>
Applied 1-14. Thanks for all your work!
I took a pretty detailed look at patch 13. It seems pretty unlikely to
me that it could cause regressions since we were basically ignoring
debug registers before. I did some extra testing with it too.
15 looks fine but I want to see what the decision is wrt threads vs
processes. I think vCont is a good example of why using threads may
prove better than processes.
I've provided additional comments on 16-18. Nothing major and if you
update them, they can be applied quickly.
> PS: If you happen to use git internally, you can also pull the series
> from my private repository:
>
> git://git.kiszka.org/qemu.git gdb-queue
>
Son of a gun, I wish I would have seen that early. FYI, in general, if
people have git repos based on the kernel.org repo, please let me know.
It's a lot easier for me to pull those patches and git-svn the result
instead of dealing with exporting mails and pulling them into SVN.
Regards,
Anthony Liguori
> --
> Siemens AG, Corporate Technology, CT SE 2 ES-OS
> Corporate Competence Center Embedded Linux
>
>
>
^ permalink raw reply [flat|nested] 37+ messages in thread