[Qemu-devel] [RESEND][PATCH 0/4] Long pending gdbstub patches

[Qemu-devel] [RESEND][PATCH 0/4] Long pending gdbstub patches

[Jan Kiszka]

Before the 0.11 window closes, another attempt to bring up this
never-ending story of gdbstub enhancements: These patches are all resent
versions (with minor rebases) of partly 7 month old proposals to improve
the gdbstub usability for x86.

The first patch (just recently reposted) add vCont support and should no
longer be controversial, specifically as we have more users now with the
qemu-user world.

Patch 2 and 3 refactor the x86 gdbstub code and add support for setting
segment register content, also nothing problematic IMHO.

Patch 4 used to raise some concerns by Paul that the underlying issue is
rather a gdb one. No question, that's right. However, the resolution
requires more work and will not see the light of day within the
foreseeable future. Moreover, we have lots of gdb versions out there
(including the to-be-released 7.0) that do suffer from this shortcoming
in its x86 support, and we need to deal with them.

All patches are in daily use since they have been written, not just by
me or Siemens-internal users. I've a lot of external users pulling this
series regularly from my tree as it plugs some annoying holes /wrt x86
guest debugging. I bet they all would be happy to find this in 0.11.

Find this series also at git://git.kiszka.org/qemu.git queues/gdb

Jan Kiszka (4):
      gdbstub: Add vCont support
      gdbstub: x86: Refactor register access
      gdbstub: x86: Support for setting segment registers
      gdbstub: x86: Switch 64/32 bit registers dynamically

 gdbstub.c            |  267 +++++++++++++++++++++++++++++++++++---------------
 target-i386/cpu.h    |   11 ++-
 target-i386/helper.c |   30 ++++++
 3 files changed, 227 insertions(+), 81 deletions(-)

[Qemu-devel] [PATCH 2/4] gdbstub: x86: Refactor register access

[Jan Kiszka]

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 74713fd..c17d14f 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -511,111 +511,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.  */

[Qemu-devel] [PATCH 3/4] gdbstub: x86: Support for setting segment registers

[Jan Kiszka]

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 c17d14f..24297ba 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -563,6 +563,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;
@@ -590,23 +615,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 a50f059..aa35987 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -747,6 +747,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 8a76abd..fef5c5b 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1692,6 +1692,36 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
     }
 }
 
+
+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;

[Qemu-devel] [PATCH 1/4] gdbstub: Add vCont support

[Jan Kiszka]

This patch adds support for the vCont remote gdb command. It is used by
gdb 6.8 or better to switch the debugging focus for single-stepping
multi-threaded targets, ie. multi-threaded application in user mode
emulation or VCPUs in system emulation.

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

 gdbstub.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 9bd4375..74713fd 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1631,6 +1631,64 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
             s->signal = 0;
         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) {
+                    env = find_cpu(res_thread);
+                    if (env == NULL) {
+                        put_packet(s, "E22");
+                        break;
+                    }
+                    s->c_cpu = env;
+                }
+                if (res == 's') {
+                    cpu_single_step(s->c_cpu, sstep_flags);
+                }
+                s->signal = res_signal;
+                gdb_continue(s);
+                return RS_IDLE;
+            }
+            break;
+        } else {
+            goto unknown_command;
+        }
     case 'k':
         /* Kill the target */
         fprintf(stderr, "\nQEMU: Terminated via GDBstub\n");

[Qemu-devel] [PATCH 4/4] gdbstub: x86: Switch 64/32 bit registers dynamically

[Jan Kiszka]

Commit 56aebc891674cd2d07b3f64183415697be200084 changed gdbstub in way
that debugging 32 or 16-bit guest code is no longer possible with qemu
for x86_64 guest CPUs. Since that commit, qemu only provides registers
sets for 64-bit, forcing current and foreseeable gdb to also switch its
architecture to 64-bit. And this breaks if the inferior is 32 or 16 bit.

No question, this is a gdb issue. But, as it was confirmed in several
discusssions with gdb people, it is a non-trivial thing to fix. So until
qemu finds a gdb version attach with a rework x86 support, we have to
work around it by switching the register layout as the guest switches
its execution mode between 16/32 and 64 bit.

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

 gdbstub.c         |   53 ++++++++++++++++++++++++++++++++++++++++-------------
 target-i386/cpu.h |    7 +++++--
 2 files changed, 45 insertions(+), 15 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 24297ba..618edc8 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -506,8 +506,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)
 
@@ -521,7 +522,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. */
@@ -532,12 +536,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);
@@ -593,8 +604,15 @@ 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) {
+            n = gpr_map32[n];
+            env->regs[n] &= ~0xffffffffUL;
+            env->regs[n] |= (uint32_t)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. */
@@ -603,14 +621,23 @@ 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 &= ~0xffffffffUL;
+                env->eip |= (uint32_t)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 aa35987..c01ebea 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -538,10 +538,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

[Qemu-devel] Re: [PATCH 4/4] gdbstub: x86: Switch 64/32 bit registers dynamically

[Paul Brook]

On Saturday 27 June 2009, Jan Kiszka wrote:
> Commit 56aebc891674cd2d07b3f64183415697be200084 changed gdbstub in way
> that debugging 32 or 16-bit guest code is no longer possible with qemu
> for x86_64 guest CPUs. Since that commit, qemu only provides registers
> sets for 64-bit, forcing current and foreseeable gdb to also switch its
> architecture to 64-bit. And this breaks if the inferior is 32 or 16 bit.
>
> No question, this is a gdb issue. But, as it was confirmed in several
> discusssions with gdb people, it is a non-trivial thing to fix. So until
> qemu finds a gdb version attach with a rework x86 support, we have to
> work around it by switching the register layout as the guest switches
> its execution mode between 16/32 and 64 bit.

I still object to this patch. Especially as there's no indication that it's a 
harroble hack to workaround broken GDB.

Paul

[Qemu-devel] Re: [PATCH 4/4] gdbstub: x86: Switch 64/32 bit registers dynamically

[Jan Kiszka]

Paul Brook wrote:
> On Saturday 27 June 2009, Jan Kiszka wrote:
>> Commit 56aebc891674cd2d07b3f64183415697be200084 changed gdbstub in way
>> that debugging 32 or 16-bit guest code is no longer possible with qemu
>> for x86_64 guest CPUs. Since that commit, qemu only provides registers
>> sets for 64-bit, forcing current and foreseeable gdb to also switch its
>> architecture to 64-bit. And this breaks if the inferior is 32 or 16 bit.
>>
>> No question, this is a gdb issue. But, as it was confirmed in several
>> discusssions with gdb people, it is a non-trivial thing to fix. So until
>> qemu finds a gdb version attach with a rework x86 support, we have to
>> work around it by switching the register layout as the guest switches
>> its execution mode between 16/32 and 64 bit.
> 
> I still object to this patch. Especially as there's no indication that it's a 
> harroble hack to workaround broken GDB.

Sorry, last sentence doesn't parse for me.

So what do you suggest for fixing this bug? Do you have a patch at hand
to fix gdb before 7.0? And a solution for older gdbs?

Jan

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

Re: [Qemu-devel] Re: [PATCH 4/4] gdbstub: x86: Switch 64/32 bit registers dynamically

[Paul Brook]

> >> No question, this is a gdb issue. But, as it was confirmed in several
> >> discusssions with gdb people, it is a non-trivial thing to fix. So until
> >> qemu finds a gdb version attach with a rework x86 support, we have to
> >> work around it by switching the register layout as the guest switches
> >> its execution mode between 16/32 and 64 bit.
> >
> > I still object to this patch. Especially as there's no indication that
> > it's a harroble hack to workaround broken GDB.
>
> Sorry, last sentence doesn't parse for me.

You've just deliberately broken the qemu gdbstub. After your patch it is 
impossible to debug mixed 32/64-bit code.

> So what do you suggest for fixing this bug? 

Fix gdb.

> Do you have a patch at hand to fix gdb before 7.0?

No. I'm unconvinced by any argument that requires a specific GDB version. 
You've known about this bug for a long time now.

> And a solution for older gdbs?

If you really care about old gdb, then you get to backport the changes.

Paul

Re: [Qemu-devel] Re: [PATCH 4/4] gdbstub: x86: Switch 64/32 bit registers dynamically

[Jan Kiszka]

Paul Brook wrote:
>>>> No question, this is a gdb issue. But, as it was confirmed in several
>>>> discusssions with gdb people, it is a non-trivial thing to fix. So until
>>>> qemu finds a gdb version attach with a rework x86 support, we have to
>>>> work around it by switching the register layout as the guest switches
>>>> its execution mode between 16/32 and 64 bit.
>>> I still object to this patch. Especially as there's no indication that
>>> it's a harroble hack to workaround broken GDB.
>> Sorry, last sentence doesn't parse for me.
> 
> You've just deliberately broken the qemu gdbstub. After your patch it is 
> impossible to debug mixed 32/64-bit code.

Please give it a try as it's the other way around: You can't properly
debug mixed target code without my patch. You can only debug 32/16 bit
code with qemu (for i386) and 64 bit code with qemu-system-x86_64. But
those scenarios are not affected by my patch in any way.

> 
>> So what do you suggest for fixing this bug? 
> 
> Fix gdb.

It's not a one-liner, far more complex than this intermediate workaround.

> 
>> Do you have a patch at hand to fix gdb before 7.0?
> 
> No. I'm unconvinced by any argument that requires a specific GDB version. 
> You've known about this bug for a long time now.
> 

Please don't forget that it wasn't me to remove the workaround from qemu
before fixing gdb first. That workaround used to be there for a reason.

>> And a solution for older gdbs?
> 
> If you really care about old gdb, then you get to backport the changes.

To all standard distros out there in the field... (this is not just
about fixing my personal debug environment)

Jan

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

Re: [Qemu-devel] Re: [PATCH 4/4] gdbstub: x86: Switch 64/32 bit registers dynamically

[Paul Brook]

> > You've just deliberately broken the qemu gdbstub. After your patch it is
> > impossible to debug mixed 32/64-bit code.
>
> Please give it a try as it's the other way around: You can't properly
> debug mixed target code without my patch. 

Sure you can, you just need a gdb that doesn't suck.  As soon as you encounter 
actual mixed code (rather than just running the wrong qemu) your patch causes 
things to die horribly.

Paul

Re: [Qemu-devel] Re: [PATCH 4/4] gdbstub: x86: Switch 64/32 bit registers dynamically

[Jan Kiszka]

Paul Brook wrote:
>>> You've just deliberately broken the qemu gdbstub. After your patch it is
>>> impossible to debug mixed 32/64-bit code.
>> Please give it a try as it's the other way around: You can't properly
>> debug mixed target code without my patch. 
> 
> Sure you can, you just need a gdb that doesn't suck.  As soon as you encounter 
> actual mixed code (rather than just running the wrong qemu) your patch causes 
> things to die horribly.

Just to recall the situation (again, please actually try it): if you
have to debug code that switches between 16/32 bit and 64 bit, you
_can't_ debug the 16 or 32 bit part as gdb will stumble and fall over
qemu sending 64-bit register layout for 16/32 bit code. That is a gdb
limitation, but this patch is about dealing with it until it's resolved
in gdb.

Jan

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

Re: [Qemu-devel] Re: [PATCH 4/4] gdbstub: x86: Switch 64/32 bit registers dynamically

[Daniel Jacobowitz]

On Mon, Jun 29, 2009 at 04:53:45PM +0200, Jan Kiszka wrote:
> Just to recall the situation (again, please actually try it): if you
> have to debug code that switches between 16/32 bit and 64 bit, you
> _can't_ debug the 16 or 32 bit part as gdb will stumble and fall over
> qemu sending 64-bit register layout for 16/32 bit code. That is a gdb
> limitation, but this patch is about dealing with it until it's resolved
> in gdb.

Remind me why you can't just tell GDB that the target is 64-bit
despite whatever file you've given it?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [Qemu-devel] Re: [PATCH 4/4] gdbstub: x86: Switch 64/32 bit registers dynamically

[Jan Kiszka]

Daniel Jacobowitz wrote:
> On Mon, Jun 29, 2009 at 04:53:45PM +0200, Jan Kiszka wrote:
>> Just to recall the situation (again, please actually try it): if you
>> have to debug code that switches between 16/32 bit and 64 bit, you
>> _can't_ debug the 16 or 32 bit part as gdb will stumble and fall over
>> qemu sending 64-bit register layout for 16/32 bit code. That is a gdb
>> limitation, but this patch is about dealing with it until it's resolved
>> in gdb.
> 
> Remind me why you can't just tell GDB that the target is 64-bit
> despite whatever file you've given it?

Because gdb mixes up arch capability and current operation mode on x86.
It always tries to disassemble according to the set arch. Moreover, it
will misinterpret the registers as being valid across all 64 bits, not
just 16 or 32. I haven't looked into further side effects, but I bet
there are more.

Jan

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

Re: [Qemu-devel] Re: [PATCH 4/4] gdbstub: x86: Switch 64/32 bit registers dynamically

[Jamie Lokier]

Jan Kiszka wrote:
> Daniel Jacobowitz wrote:
> > On Mon, Jun 29, 2009 at 04:53:45PM +0200, Jan Kiszka wrote:
> >> Just to recall the situation (again, please actually try it): if you
> >> have to debug code that switches between 16/32 bit and 64 bit, you
> >> _can't_ debug the 16 or 32 bit part as gdb will stumble and fall over
> >> qemu sending 64-bit register layout for 16/32 bit code. That is a gdb
> >> limitation, but this patch is about dealing with it until it's resolved
> >> in gdb.
> > 
> > Remind me why you can't just tell GDB that the target is 64-bit
> > despite whatever file you've given it?
> 
> Because gdb mixes up arch capability and current operation mode on x86.
> It always tries to disassemble according to the set arch. Moreover, it
> will misinterpret the registers as being valid across all 64 bits, not
> just 16 or 32. I haven't looked into further side effects, but I bet
> there are more.

On a 64-bit CPU in 16/32-bit mode, all 64 register bits _are_ valid
aren't they?  (But not useful, as far as I know.  Unless there's a
64-bit equivalent to i386's "big real" modes and such).

-- Jamie

Re: [Qemu-devel] Re: [PATCH 4/4] gdbstub: x86: Switch 64/32 bit registers dynamically

[Jan Kiszka]

Jamie Lokier wrote:
> Jan Kiszka wrote:
>> Daniel Jacobowitz wrote:
>>> On Mon, Jun 29, 2009 at 04:53:45PM +0200, Jan Kiszka wrote:
>>>> Just to recall the situation (again, please actually try it): if you
>>>> have to debug code that switches between 16/32 bit and 64 bit, you
>>>> _can't_ debug the 16 or 32 bit part as gdb will stumble and fall over
>>>> qemu sending 64-bit register layout for 16/32 bit code. That is a gdb
>>>> limitation, but this patch is about dealing with it until it's resolved
>>>> in gdb.
>>> Remind me why you can't just tell GDB that the target is 64-bit
>>> despite whatever file you've given it?
>> Because gdb mixes up arch capability and current operation mode on x86.
>> It always tries to disassemble according to the set arch. Moreover, it
>> will misinterpret the registers as being valid across all 64 bits, not
>> just 16 or 32. I haven't looked into further side effects, but I bet
>> there are more.
> 
> On a 64-bit CPU in 16/32-bit mode, all 64 register bits _are_ valid
> aren't they?  (But not useful, as far as I know.  Unless there's a
> 64-bit equivalent to i386's "big real" modes and such).

I meant they are invalid in the sense that, e.g., 32 bit code will not
modify the bits 32..63, thus r*x may not always equal e*x.

But even more problematic is the fact that frame unwinding does not work
if gdb applies 64 bit mode while the target is doing something
completely different. A "set arch i386:x86-64" workaround for this
problem simply does not work.

Jan

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

Re: [Qemu-devel] Re: [PATCH 4/4] gdbstub: x86: Switch 64/32 bit registers dynamically

[Gerd Hoffmann]

On 06/29/09 16:43, Paul Brook wrote:
>>> You've just deliberately broken the qemu gdbstub. After your patch it is
>>> impossible to debug mixed 32/64-bit code.
>> Please give it a try as it's the other way around: You can't properly
>> debug mixed target code without my patch.
>
> Sure you can, you just need a gdb that doesn't suck.  As soon as you encounter
> actual mixed code (rather than just running the wrong qemu) your patch causes
> things to die horribly.

i.e. the latest gdb release (6.8) works better *with* the workaround,
and the latest gdb bits from cvs work better *without* the workaround.
Is that correct?

How about making it runtime-switchable then, so qemu can deal with both 
cases?

cheers,
   Gerd

Re: [Qemu-devel] Re: [PATCH 4/4] gdbstub: x86: Switch 64/32 bit registers dynamically

[Jan Kiszka]

Gerd Hoffmann wrote:
> On 06/29/09 16:43, Paul Brook wrote:
>>>> You've just deliberately broken the qemu gdbstub. After your patch
>>>> it is
>>>> impossible to debug mixed 32/64-bit code.
>>> Please give it a try as it's the other way around: You can't properly
>>> debug mixed target code without my patch.
>>
>> Sure you can, you just need a gdb that doesn't suck.  As soon as you
>> encounter
>> actual mixed code (rather than just running the wrong qemu) your patch
>> causes
>> things to die horribly.
> 
> i.e. the latest gdb release (6.8) works better *with* the workaround,
> and the latest gdb bits from cvs work better *without* the workaround.
> Is that correct?

Nope, even today's gdb requires my patch to work properly in these mixed
scenarios.

> 
> How about making it runtime-switchable then, so qemu can deal with both
> cases?

I hope we will once be able to automatically detected improved gdb
versions. For now I see no need for a command line or whatever switch.
But I'm always open to learn about steps that work without it and fail
when it's applied. I'm not aware of any.

Jan

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

Re: [Qemu-devel] Re: [PATCH 4/4] gdbstub: x86: Switch 64/32 bit registers dynamically

[Paul Brook]

> Please don't forget that it wasn't me to remove the workaround from qemu
> before fixing gdb first. That workaround used to be there for a reason.

I removed completely undocumented and obviously wrong code.

Paul