On 20.01.2014, at 18:49, Thomas Falcon <tlfal...@linux.vnet.ibm.com> wrote:
> On 01/20/2014 08:33 AM, Alexander Graf wrote: >> On 17.01.2014, at 22:02, Thomas Falcon <tlfal...@linux.vnet.ibm.com> wrote: >> >>> This patch allows registers to be properly read from and written to >>> when using the gdbstub to debug a ppc guest running in little >>> endian mode. It accomplishes this goal by byte swapping the values of >>> any registers if the MSR:LE value is set. >>> >>> Signed-off-by: Thomas Falcon <tlfal...@linux.vnet.ibm.com> >>> --- >>> Differences from v2: >>> >>> Fixed formatting issues >>> Added logic to ensure only FP registers have a guaranteed size of 8 bytes >> >> I don't really like how the write case has to know about the size of a >> register (maybe we could factor this out into a single function for all >> reads and writes?), but this is good enough for now :). However, I can't >> apply the patch as your email client seems to have broken the patch >> formatting. >> >> >> Alex >> >> > I'm not sure of a way to swap the value without knowing its size. In both > read and write, the size needs to be known and is hardcoded in some cases. > The write case cannot know the size without a conditional since we need to > swap in mem_buf before we call ppc_cpu_gdb_write_register. Maybe we could > get around this by hanging ppc_cpu_gdb_write_register so that it returns a > pointer to the register being overwritten, and then we could swap that > instead of mem_buf? But even then I guess we would still need to check the > size of the register before we called bswap32/64. Well, the easiest way would be to factor out the size from the content reads and writes. Something like this: diff --git a/target-ppc/gdbstub.c b/target-ppc/gdbstub.c index 1c91090..17243cb 100644 --- a/target-ppc/gdbstub.c +++ b/target-ppc/gdbstub.c @@ -28,27 +28,67 @@ * FP regs zero size when talking to a newer gdb. */ +static int ppc_cpu_gdb_register_len(int n) +{ + switch (n) { + case 0 ... 31: + /* gprs */ + return sizeof(target_ulong); + case 32 ... 63: + /* fprs */ + if (gdb_has_xml) { + return 0; + } + return 8; + case 66: + /* cr */ + return 4; + case 64: + /* nip */ + case 65: + /* msr */ + case 67: + /* lr */ + case 68: + /* ctr */ + case 69: + /* xer */ + return sizeof(target_ulong); + case 70: + /* fpscr */ + if (gdb_has_xml) { + return 0; + } + return sizeof(target_ulong); + default: + return 0; + } +} + int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n) { PowerPCCPU *cpu = POWERPC_CPU(cs); CPUPPCState *env = &cpu->env; + int r = ppc_cpu_gdb_register_len(n); + + if (!r) { + return r; + } if (n < 32) { /* gprs */ - return gdb_get_regl(mem_buf, env->gpr[n]); + gdb_get_regl(mem_buf, env->gpr[n]); } else if (n < 64) { /* fprs */ - if (gdb_has_xml) { - return 0; - } stfq_p(mem_buf, env->fpr[n-32]); - return 8; } else { switch (n) { case 64: - return gdb_get_regl(mem_buf, env->nip); + gdb_get_regl(mem_buf, env->nip); + break; case 65: - return gdb_get_regl(mem_buf, env->msr); + gdb_get_regl(mem_buf, env->msr); + break; case 66: { uint32_t cr = 0; @@ -56,50 +96,51 @@ int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n) for (i = 0; i < 8; i++) { cr |= env->crf[i] << (32 - ((i + 1) * 4)); } - return gdb_get_reg32(mem_buf, cr); + gdb_get_reg32(mem_buf, cr); + break; } case 67: - return gdb_get_regl(mem_buf, env->lr); + gdb_get_regl(mem_buf, env->lr); + break; case 68: - return gdb_get_regl(mem_buf, env->ctr); + gdb_get_regl(mem_buf, env->ctr); + break; case 69: - return gdb_get_regl(mem_buf, env->xer); + gdb_get_regl(mem_buf, env->xer); + break; case 70: - { - if (gdb_has_xml) { - return 0; - } - return gdb_get_reg32(mem_buf, env->fpscr); - } + gdb_get_reg32(mem_buf, env->fpscr); + break; } } - return 0; + + return r; } int ppc_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n) { PowerPCCPU *cpu = POWERPC_CPU(cs); CPUPPCState *env = &cpu->env; + int r = ppc_cpu_gdb_register_len(n); + + if (!r) { + return r; + } if (n < 32) { /* gprs */ env->gpr[n] = ldtul_p(mem_buf); - return sizeof(target_ulong); } else if (n < 64) { /* fprs */ - if (gdb_has_xml) { - return 0; - } env->fpr[n-32] = ldfq_p(mem_buf); - return 8; } else { switch (n) { case 64: env->nip = ldtul_p(mem_buf); - return sizeof(target_ulong); + break; case 65: ppc_store_msr(env, ldtul_p(mem_buf)); - return sizeof(target_ulong); + break; case 66: { uint32_t cr = ldl_p(mem_buf); @@ -107,25 +148,23 @@ int ppc_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n) for (i = 0; i < 8; i++) { env->crf[i] = (cr >> (32 - ((i + 1) * 4))) & 0xF; } - return 4; + break; } case 67: env->lr = ldtul_p(mem_buf); - return sizeof(target_ulong); + break; case 68: env->ctr = ldtul_p(mem_buf); - return sizeof(target_ulong); + break; case 69: env->xer = ldtul_p(mem_buf); - return sizeof(target_ulong); + break; case 70: /* fpscr */ - if (gdb_has_xml) { - return 0; - } store_fpscr(env, ldtul_p(mem_buf), 0xffffffff); - return sizeof(target_ulong); + break; } } - return 0; + + return r; } > > Anyway, sorry about the formatting issues again. Should I just resubmit the > patch as is? Whichever way you prefer. The way it is now I can't apply it :). Alex