Re: [PATCH] align gdbstub with QEMU

Jan Kiszka
2007-12-14T11:57:03Z

> We've noticed some problems with current gdbstub in kvm's qemu:
>
> # qemu-system-x86_64 -hda myimage -S -s &
> # gdb
> (gdb) tar re :1234
> Remote debugging using :1234
> Remote 'g' packet reply is too long:
> 000000000000000000000000000000000000[...]
>
> This issue did not occur with QEMU from CVS. As I was aware of an
> x86_64-related problem in QEMU's gdbstub, I merged its current state
> into kvm - and things start to work again. Find the update patch below.

Any feedback on this? Any chance to see this patch (or some similar
version) in kvm-57?

Thanks,
Jan

>
> Signed-off-by: Jan Kiszka <jan.kiszka-kv7WeFo6aLtBDgjK7y7TUQ@public.gmane.org>
>
> ---
> qemu/gdbstub.c | 290 ++++++++++++++++++++++++++++-----------------------------
> 1 file changed, 143 insertions(+), 147 deletions(-)
>
> Index: kvm-54/qemu/gdbstub.c
> ===================================================================
> --- kvm-54.orig/qemu/gdbstub.c
> +++ kvm-54/qemu/gdbstub.c
> @@ -222,146 +222,60 @@ static int put_packet(GDBState *s, char
> return 0;
> }
>
> -#if defined(TARGET_X86_64)
> +#if defined(TARGET_I386)
>
> static int cpu_gdb_read_registers(CPUState *env, uint8_t *mem_buf)
> {
> - uint8_t *p = mem_buf;
> int i, fpus;
> + uint32_t *registers = (uint32_t *)mem_buf;
>
> -#define PUTREG(x) do { \
> - target_ulong reg = tswapl(x); \
> - memcpy(p, &reg, sizeof reg); \
> - p += sizeof reg; \
> - } while (0)
> -#define PUTREG32(x) do { \
> - uint32_t reg = tswap32(x); \
> - memcpy(p, &reg, sizeof reg); \
> - p += sizeof reg; \
> - } while (0)
> -#define PUTREGF(x) do { \
> - memcpy(p, &(x), 10); \
> - p += sizeof (x); \
> - } while (0)
> -
> - PUTREG(env->regs[R_EAX]);
> - PUTREG(env->regs[R_EBX]);
> - PUTREG(env->regs[R_ECX]);
> - PUTREG(env->regs[R_EDX]);
> - PUTREG(env->regs[R_ESI]);
> - PUTREG(env->regs[R_EDI]);
> - PUTREG(env->regs[R_EBP]);
> - PUTREG(env->regs[R_ESP]);
> - PUTREG(env->regs[8]);
> - PUTREG(env->regs[9]);
> - PUTREG(env->regs[10]);
> - PUTREG(env->regs[11]);
> - PUTREG(env->regs[12]);
> - PUTREG(env->regs[13]);
> - PUTREG(env->regs[14]);
> - PUTREG(env->regs[15]);
> -
> - PUTREG(env->eip);
> - PUTREG32(env->eflags);
> - PUTREG32(env->segs[R_CS].selector);
> - PUTREG32(env->segs[R_SS].selector);
> - PUTREG32(env->segs[R_DS].selector);
> - PUTREG32(env->segs[R_ES].selector);
> - PUTREG32(env->segs[R_FS].selector);
> - PUTREG32(env->segs[R_GS].selector);
> - /* XXX: convert floats */
> - for(i = 0; i < 8; i++) {
> - PUTREGF(env->fpregs[i]);
> - }
> - PUTREG32(env->fpuc);
> - fpus = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
> - PUTREG32(fpus);
> - PUTREG32(0); /* XXX: convert tags */
> - PUTREG32(0); /* fiseg */
> - PUTREG32(0); /* fioff */
> - PUTREG32(0); /* foseg */
> - PUTREG32(0); /* fooff */
> - PUTREG32(0); /* fop */
> -
> -#undef PUTREG
> -#undef PUTREG32
> -#undef PUTREGF
> -
> - return p - mem_buf;
> -}
> +#ifdef TARGET_X86_64
> + /* This corresponds with amd64_register_info[] in gdb/amd64-tdep.c */
> + uint64_t *registers64 = (uint64_t *)mem_buf;
> +
> + if (env->hflags & HF_CS64_MASK) {
> + registers64[0] = tswap64(env->regs[R_EAX]);
> + registers64[1] = tswap64(env->regs[R_EBX]);
> + registers64[2] = tswap64(env->regs[R_ECX]);
> + registers64[3] = tswap64(env->regs[R_EDX]);
> + registers64[4] = tswap64(env->regs[R_ESI]);
> + registers64[5] = tswap64(env->regs[R_EDI]);
> + registers64[6] = tswap64(env->regs[R_EBP]);
> + registers64[7] = tswap64(env->regs[R_ESP]);
> + for(i = 8; i < 16; i++) {
> + registers64[i] = tswap64(env->regs[i]);
> + }
> + registers64[16] = tswap64(env->eip);
>
> -static void cpu_gdb_write_registers(CPUState *env, uint8_t *mem_buf, int size)
> -{
> - uint8_t *p = mem_buf;
> - uint32_t junk;
> - int i, fpus;
> + registers = (uint32_t *)&registers64[17];
> + registers[0] = tswap32(env->eflags);
> + registers[1] = tswap32(env->segs[R_CS].selector);
> + registers[2] = tswap32(env->segs[R_SS].selector);
> + registers[3] = tswap32(env->segs[R_DS].selector);
> + registers[4] = tswap32(env->segs[R_ES].selector);
> + registers[5] = tswap32(env->segs[R_FS].selector);
> + registers[6] = tswap32(env->segs[R_GS].selector);
> + /* XXX: convert floats */
> + for(i = 0; i < 8; i++) {
> + memcpy(mem_buf + 16 * 8 + 7 * 4 + i * 10, &env->fpregs[i], 10);
> + }
> + registers[27] = tswap32(env->fpuc); /* fctrl */
> + fpus = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
> + registers[28] = tswap32(fpus); /* fstat */
> + registers[29] = 0; /* ftag */
> + registers[30] = 0; /* fiseg */
> + registers[31] = 0; /* fioff */
> + registers[32] = 0; /* foseg */
> + registers[33] = 0; /* fooff */
> + registers[34] = 0; /* fop */
> + for(i = 0; i < 16; i++) {
> + memcpy(mem_buf + 16 * 8 + 35 * 4 + i * 16, &env->xmm_regs[i], 16);
> + }
> + registers[99] = tswap32(env->mxcsr);
>
> -#define GETREG(x) do { \
> - target_ulong reg; \
> - memcpy(&reg, p, sizeof reg); \
> - x = tswapl(reg); \
> - p += sizeof reg; \
> - } while (0)
> -#define GETREG32(x) do { \
> - uint32_t reg; \
> - memcpy(&reg, p, sizeof reg); \
> - x = tswap32(reg); \
> - p += sizeof reg; \
> - } while (0)
> -#define GETREGF(x) do { \
> - memcpy(&(x), p, 10); \
> - p += 10; \
> - } while (0)
> -
> - GETREG(env->regs[R_EAX]);
> - GETREG(env->regs[R_EBX]);
> - GETREG(env->regs[R_ECX]);
> - GETREG(env->regs[R_EDX]);
> - GETREG(env->regs[R_ESI]);
> - GETREG(env->regs[R_EDI]);
> - GETREG(env->regs[R_EBP]);
> - GETREG(env->regs[R_ESP]);
> - GETREG(env->regs[8]);
> - GETREG(env->regs[9]);
> - GETREG(env->regs[10]);
> - GETREG(env->regs[11]);
> - GETREG(env->regs[12]);
> - GETREG(env->regs[13]);
> - GETREG(env->regs[14]);
> - GETREG(env->regs[15]);
> -
> - GETREG(env->eip);
> - GETREG32(env->eflags);
> - GETREG32(env->segs[R_CS].selector);
> - GETREG32(env->segs[R_SS].selector);
> - GETREG32(env->segs[R_DS].selector);
> - GETREG32(env->segs[R_ES].selector);
> - GETREG32(env->segs[R_FS].selector);
> - GETREG32(env->segs[R_GS].selector);
> - /* XXX: convert floats */
> - for(i = 0; i < 8; i++) {
> - GETREGF(env->fpregs[i]);
> + return 8 * 17 + 4 * 7 + 10 * 8 + 4 * 8 + 16 * 16 + 4;
> }
> - GETREG32(env->fpuc);
> - GETREG32(fpus); /* XXX: convert fpus */
> - GETREG32(junk); /* XXX: convert tags */
> - GETREG32(junk); /* fiseg */
> - GETREG32(junk); /* fioff */
> - GETREG32(junk); /* foseg */
> - GETREG32(junk); /* fooff */
> - GETREG32(junk); /* fop */
> -
> -#undef GETREG
> -#undef GETREG32
> -#undef GETREGF
> -}
> -
> -#elif defined(TARGET_I386)
> -
> -static int cpu_gdb_read_registers(CPUState *env, uint8_t *mem_buf)
> -{
> - uint32_t *registers = (uint32_t *)mem_buf;
> - int i, fpus;
> +#endif
>
> for(i = 0; i < 8; i++) {
> registers[i] = env->regs[i];
> @@ -435,7 +349,7 @@ static int cpu_gdb_read_registers(CPUSta
> }
> /* nip, msr, ccr, lnk, ctr, xer, mq */
> registers[96] = tswapl(env->nip);
> - registers[97] = tswapl(do_load_msr(env));
> + registers[97] = tswapl(env->msr);
> tmp = 0;
> for (i = 0; i < 8; i++)
> tmp |= env->crf[i] << (32 - ((i + 1) * 4));
> @@ -464,7 +378,7 @@ static void cpu_gdb_write_registers(CPUS
> }
> /* nip, msr, ccr, lnk, ctr, xer, mq */
> env->nip = tswapl(registers[96]);
> - do_store_msr(env, tswapl(registers[97]));
> + ppc_store_msr(env, tswapl(registers[97]));
> registers[98] = tswapl(registers[98]);
> for (i = 0; i < 8; i++)
> env->crf[i] = (registers[98] >> (32 - ((i + 1) * 4))) & 0xF;
> @@ -698,7 +612,7 @@ static int cpu_gdb_read_registers(CPUSta
> ptr += sizeof(target_ulong);
> }
>
> - *(target_ulong *)ptr = tswapl(env->CP0_Status);
> + *(target_ulong *)ptr = (int32_t)tswap32(env->CP0_Status);
> ptr += sizeof(target_ulong);
>
> *(target_ulong *)ptr = tswapl(env->LO[0][env->current_tc]);
> @@ -710,7 +624,7 @@ static int cpu_gdb_read_registers(CPUSta
> *(target_ulong *)ptr = tswapl(env->CP0_BadVAddr);
> ptr += sizeof(target_ulong);
>
> - *(target_ulong *)ptr = tswapl(env->CP0_Cause);
> + *(target_ulong *)ptr = (int32_t)tswap32(env->CP0_Cause);
> ptr += sizeof(target_ulong);
>
> *(target_ulong *)ptr = tswapl(env->PC[env->current_tc]);
> @@ -720,19 +634,34 @@ static int cpu_gdb_read_registers(CPUSta
> {
> for (i = 0; i < 32; i++)
> {
> - *(target_ulong *)ptr = tswapl(env->fpu->fpr[i].fs[FP_ENDIAN_IDX]);
> + if (env->CP0_Status & (1 << CP0St_FR))
> + *(target_ulong *)ptr = tswapl(env->fpu->fpr[i].d);
> + else
> + *(target_ulong *)ptr = tswap32(env->fpu->fpr[i].w[FP_ENDIAN_IDX]);
> ptr += sizeof(target_ulong);
> }
>
> - *(target_ulong *)ptr = tswapl(env->fpu->fcr31);
> + *(target_ulong *)ptr = (int32_t)tswap32(env->fpu->fcr31);
> + ptr += sizeof(target_ulong);
> +
> + *(target_ulong *)ptr = (int32_t)tswap32(env->fpu->fcr0);
> ptr += sizeof(target_ulong);
> + }
> +
> + /* "fp", pseudo frame pointer. Not yet implemented in gdb. */
> + *(target_ulong *)ptr = 0;
> + ptr += sizeof(target_ulong);
>
> - *(target_ulong *)ptr = tswapl(env->fpu->fcr0);
> + /* Registers for embedded use, we just pad them. */
> + for (i = 0; i < 16; i++)
> + {
> + *(target_ulong *)ptr = 0;
> ptr += sizeof(target_ulong);
> }
>
> - /* 32 FP registers, fsr, fir, fp. Not yet implemented. */
> - /* what's 'fp' mean here? */
> + /* Processor ID. */
> + *(target_ulong *)ptr = (int32_t)tswap32(env->CP0_PRid);
> + ptr += sizeof(target_ulong);
>
> return ptr - mem_buf;
> }
> @@ -782,15 +711,17 @@ static void cpu_gdb_write_registers(CPUS
> {
> for (i = 0; i < 32; i++)
> {
> - env->fpu->fpr[i].fs[FP_ENDIAN_IDX] = tswapl(*(target_ulong *)ptr);
> + if (env->CP0_Status & (1 << CP0St_FR))
> + env->fpu->fpr[i].d = tswapl(*(target_ulong *)ptr);
> + else
> + env->fpu->fpr[i].w[FP_ENDIAN_IDX] = tswapl(*(target_ulong *)ptr);
> ptr += sizeof(target_ulong);
> }
>
> - env->fpu->fcr31 = tswapl(*(target_ulong *)ptr) & 0x0183FFFF;
> + env->fpu->fcr31 = tswapl(*(target_ulong *)ptr) & 0xFF83FFFF;
> ptr += sizeof(target_ulong);
>
> - env->fpu->fcr0 = tswapl(*(target_ulong *)ptr);
> - ptr += sizeof(target_ulong);
> + /* The remaining registers are assumed to be read-only. */
>
> /* set rounding mode */
> RESTORE_ROUNDING_MODE;
> @@ -863,6 +794,66 @@ static void cpu_gdb_write_registers(CPUS
> for (i = 0; i < 8; i++) LOAD(env->gregs[i]);
> for (i = 0; i < 8; i++) LOAD(env->gregs[i + 16]);
> }
> +#elif defined (TARGET_CRIS)
> +
> +static int cris_save_32 (unsigned char *d, uint32_t value)
> +{
> + *d++ = (value);
> + *d++ = (value >>= 8);
> + *d++ = (value >>= 8);
> + *d++ = (value >>= 8);
> + return 4;
> +}
> +static int cris_save_16 (unsigned char *d, uint32_t value)
> +{
> + *d++ = (value);
> + *d++ = (value >>= 8);
> + return 2;
> +}
> +static int cris_save_8 (unsigned char *d, uint32_t value)
> +{
> + *d++ = (value);
> + return 1;
> +}
> +
> +/* FIXME: this will bug on archs not supporting unaligned word accesses. */
> +static int cpu_gdb_read_registers(CPUState *env, uint8_t *mem_buf)
> +{
> + uint8_t *ptr = mem_buf;
> + uint8_t srs;
> + int i;
> +
> + for (i = 0; i < 16; i++)
> + ptr += cris_save_32 (ptr, env->regs[i]);
> +
> + srs = env->pregs[SR_SRS];
> +
> + ptr += cris_save_8 (ptr, env->pregs[0]);
> + ptr += cris_save_8 (ptr, env->pregs[1]);
> + ptr += cris_save_32 (ptr, env->pregs[2]);
> + ptr += cris_save_8 (ptr, srs);
> + ptr += cris_save_16 (ptr, env->pregs[4]);
> +
> + for (i = 5; i < 16; i++)
> + ptr += cris_save_32 (ptr, env->pregs[i]);
> +
> + ptr += cris_save_32 (ptr, env->pc);
> +
> + for (i = 0; i < 16; i++)
> + ptr += cris_save_32 (ptr, env->sregs[srs][i]);
> +
> + return ((uint8_t *)ptr - mem_buf);
> +}
> +
> +static void cpu_gdb_write_registers(CPUState *env, uint8_t *mem_buf, int size)
> +{
> + uint32_t *ptr = (uint32_t *)mem_buf;
> + int i;
> +
> +#define LOAD(x) (x)=*ptr++;
> + for (i = 0; i < 16; i++) LOAD(env->regs[i]);
> + LOAD (env->pc);
> +}
> #else
> static int cpu_gdb_read_registers(CPUState *env, uint8_t *mem_buf)
> {
> @@ -880,7 +871,7 @@ static int gdb_handle_packet(GDBState *s
> const char *p;
> int ch, reg_size, type;
> char buf[4096];
> - uint8_t mem_buf[2000];
> + uint8_t mem_buf[4096];
> uint32_t *registers;
> target_ulong addr, len;
>
> @@ -914,6 +905,8 @@ static int gdb_handle_packet(GDBState *s
> env->pc = addr;
> #elif defined (TARGET_MIPS)
> env->PC[env->current_tc] = addr;
> +#elif defined (TARGET_CRIS)
> + env->pc = addr;
> #endif
> }
> #ifdef CONFIG_USER_ONLY
> @@ -941,6 +934,8 @@ static int gdb_handle_packet(GDBState *s
> env->pc = addr;
> #elif defined (TARGET_MIPS)
> env->PC[env->current_tc] = addr;
> +#elif defined (TARGET_CRIS)
> + env->pc = addr;
> #endif
> }
> cpu_single_step(env, 1);
> @@ -1070,7 +1065,8 @@ static int gdb_handle_packet(GDBState *s
> TaskState *ts = env->opaque;
>
> sprintf(buf,
> - "Text=" TARGET_FMT_lx ";Data=" TARGET_FMT_lx ";Bss=" TARGET_FMT_lx,
> + "Text=" TARGET_ABI_FMT_lx ";Data=" TARGET_ABI_FMT_lx
> + ";Bss=" TARGET_ABI_FMT_lx,
> ts->info->code_offset,
> ts->info->data_offset,
> ts->info->data_offset);
>
>

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