[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3] target-ppc: gdbstub allow byte swapping for,
|
From: |
Alexander Graf |
|
Subject: |
Re: [Qemu-devel] [PATCH v3] target-ppc: gdbstub allow byte swapping for, reading/writing registers |
|
Date: |
Mon, 20 Jan 2014 19:39:03 +0100 |
On 20.01.2014, at 18:49, Thomas Falcon <address@hidden> wrote:
> On 01/20/2014 08:33 AM, Alexander Graf wrote:
>> On 17.01.2014, at 22:02, Thomas Falcon <address@hidden> 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 <address@hidden>
>>> ---
>>> 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