diff mbox series

[v1,13/28] target/i386: use gdb_get_reg helpers

Message ID 20200316172155.971-14-alex.bennee@linaro.org
State New
Headers show
Series testing and gdbstub | expand

Commit Message

Alex Bennée March 16, 2020, 5:21 p.m. UTC 
This is cleaner than poking memory directly and will make later
clean-ups easier.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v7
  - remove stray space
  - fixup the floatx80 set/get routines
---
 target/i386/gdbstub.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

Comments

Philippe Mathieu-Daudé March 17, 2020, 9:53 a.m. UTC | #1 
On 3/16/20 6:21 PM, Alex Bennée wrote:
> This is cleaner than poking memory directly and will make later
> clean-ups easier.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v7
>    - remove stray space
>    - fixup the floatx80 set/get routines
> ---
>   target/i386/gdbstub.c | 27 +++++++++++----------------
>   1 file changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
> index 572ead641ca..e4d8cb66c00 100644
> --- a/target/i386/gdbstub.c
> +++ b/target/i386/gdbstub.c
> @@ -98,26 +98,22 @@ int x86_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
>                   return gdb_get_reg64(mem_buf,
>                                        env->regs[gpr_map[n]] & 0xffffffffUL);
>               } else {
> -                memset(mem_buf, 0, sizeof(target_ulong));
> -                return sizeof(target_ulong);
> +                return gdb_get_regl(mem_buf, 0);
>               }
>           } else {
>               return gdb_get_reg32(mem_buf, 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. */
> -        memcpy(mem_buf, &env->fpregs[n - IDX_FP_REGS], 10);
> -#else
> -        memset(mem_buf, 0, 10);
> -#endif
> -        return 10;
> +        floatx80 *fp = (floatx80 *) &env->fpregs[n - IDX_FP_REGS];
> +        int len = gdb_get_reg64(mem_buf, cpu_to_le64(fp->low));
> +        len += gdb_get_reg16(mem_buf + len, cpu_to_le16(fp->high));
> +        return len;
>       } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
>           n -= IDX_XMM_REGS;
>           if (n < CPU_NB_REGS32 || TARGET_LONG_BITS == 64) {
> -            stq_p(mem_buf, env->xmm_regs[n].ZMM_Q(0));
> -            stq_p(mem_buf + 8, env->xmm_regs[n].ZMM_Q(1));
> -            return 16;
> +            return gdb_get_reg128(mem_buf,
> +                                  env->xmm_regs[n].ZMM_Q(0),
> +                                  env->xmm_regs[n].ZMM_Q(1));
>           }
>       } else {
>           switch (n) {
> @@ -290,10 +286,9 @@ int x86_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>               return 4;
>           }
>       } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
> -#ifdef USE_X86LDOUBLE
> -        /* FIXME: byteswap float values - after fixing fpregs layout. */
> -        memcpy(&env->fpregs[n - IDX_FP_REGS], mem_buf, 10);
> -#endif
> +        floatx80 *fp = (floatx80 *) &env->fpregs[n - IDX_FP_REGS];
> +        fp->low = le64_to_cpu(* (uint64_t *) mem_buf);
> +        fp->high = le16_to_cpu(* (uint16_t *) (mem_buf + 8));
>           return 10;
>       } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
>           n -= IDX_XMM_REGS;
> 

I'd prefer both USE_X86LDOUBLE hunks in a separate patch, anyway:

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Philippe Mathieu-Daudé March 17, 2020, 10:05 a.m. UTC | #2 
On 3/17/20 10:53 AM, Philippe Mathieu-Daudé wrote:
> On 3/16/20 6:21 PM, Alex Bennée wrote:
>> This is cleaner than poking memory directly and will make later
>> clean-ups easier.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> ---
>> v7
>>    - remove stray space
>>    - fixup the floatx80 set/get routines
>> ---
>>   target/i386/gdbstub.c | 27 +++++++++++----------------
>>   1 file changed, 11 insertions(+), 16 deletions(-)
>>
>> diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
>> index 572ead641ca..e4d8cb66c00 100644
>> --- a/target/i386/gdbstub.c
>> +++ b/target/i386/gdbstub.c
>> @@ -98,26 +98,22 @@ int x86_cpu_gdb_read_register(CPUState *cs, 
>> uint8_t *mem_buf, int n)
>>                   return gdb_get_reg64(mem_buf,
>>                                        env->regs[gpr_map[n]] & 
>> 0xffffffffUL);
>>               } else {
>> -                memset(mem_buf, 0, sizeof(target_ulong));
>> -                return sizeof(target_ulong);
>> +                return gdb_get_regl(mem_buf, 0);
>>               }
>>           } else {
>>               return gdb_get_reg32(mem_buf, 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. */
>> -        memcpy(mem_buf, &env->fpregs[n - IDX_FP_REGS], 10);
>> -#else
>> -        memset(mem_buf, 0, 10);
>> -#endif
>> -        return 10;
>> +        floatx80 *fp = (floatx80 *) &env->fpregs[n - IDX_FP_REGS];
>> +        int len = gdb_get_reg64(mem_buf, cpu_to_le64(fp->low));
>> +        len += gdb_get_reg16(mem_buf + len, cpu_to_le16(fp->high));
>> +        return len;
>>       } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
>>           n -= IDX_XMM_REGS;
>>           if (n < CPU_NB_REGS32 || TARGET_LONG_BITS == 64) {
>> -            stq_p(mem_buf, env->xmm_regs[n].ZMM_Q(0));
>> -            stq_p(mem_buf + 8, env->xmm_regs[n].ZMM_Q(1));
>> -            return 16;
>> +            return gdb_get_reg128(mem_buf,
>> +                                  env->xmm_regs[n].ZMM_Q(0),
>> +                                  env->xmm_regs[n].ZMM_Q(1));
>>           }
>>       } else {
>>           switch (n) {
>> @@ -290,10 +286,9 @@ int x86_cpu_gdb_write_register(CPUState *cs, 
>> uint8_t *mem_buf, int n)
>>               return 4;
>>           }
>>       } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
>> -#ifdef USE_X86LDOUBLE
>> -        /* FIXME: byteswap float values - after fixing fpregs layout. */
>> -        memcpy(&env->fpregs[n - IDX_FP_REGS], mem_buf, 10);
>> -#endif
>> +        floatx80 *fp = (floatx80 *) &env->fpregs[n - IDX_FP_REGS];
>> +        fp->low = le64_to_cpu(* (uint64_t *) mem_buf);
>> +        fp->high = le16_to_cpu(* (uint16_t *) (mem_buf + 8));

Note, checkpatch complains for both lines:
ERROR: space prohibited after that '*' (ctx:BxW)

>>           return 10;
>>       } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
>>           n -= IDX_XMM_REGS;
>>
> 
> I'd prefer both USE_X86LDOUBLE hunks in a separate patch, anyway:
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
diff mbox series

Patch

diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
index 572ead641ca..e4d8cb66c00 100644
--- a/target/i386/gdbstub.c
+++ b/target/i386/gdbstub.c
@@ -98,26 +98,22 @@  int x86_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
                 return gdb_get_reg64(mem_buf,
                                      env->regs[gpr_map[n]] & 0xffffffffUL);
             } else {
-                memset(mem_buf, 0, sizeof(target_ulong));
-                return sizeof(target_ulong);
+                return gdb_get_regl(mem_buf, 0);
             }
         } else {
             return gdb_get_reg32(mem_buf, 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. */
-        memcpy(mem_buf, &env->fpregs[n - IDX_FP_REGS], 10);
-#else
-        memset(mem_buf, 0, 10);
-#endif
-        return 10;
+        floatx80 *fp = (floatx80 *) &env->fpregs[n - IDX_FP_REGS];
+        int len = gdb_get_reg64(mem_buf, cpu_to_le64(fp->low));
+        len += gdb_get_reg16(mem_buf + len, cpu_to_le16(fp->high));
+        return len;
     } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
         n -= IDX_XMM_REGS;
         if (n < CPU_NB_REGS32 || TARGET_LONG_BITS == 64) {
-            stq_p(mem_buf, env->xmm_regs[n].ZMM_Q(0));
-            stq_p(mem_buf + 8, env->xmm_regs[n].ZMM_Q(1));
-            return 16;
+            return gdb_get_reg128(mem_buf,
+                                  env->xmm_regs[n].ZMM_Q(0),
+                                  env->xmm_regs[n].ZMM_Q(1));
         }
     } else {
         switch (n) {
@@ -290,10 +286,9 @@  int x86_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
             return 4;
         }
     } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
-#ifdef USE_X86LDOUBLE
-        /* FIXME: byteswap float values - after fixing fpregs layout. */
-        memcpy(&env->fpregs[n - IDX_FP_REGS], mem_buf, 10);
-#endif
+        floatx80 *fp = (floatx80 *) &env->fpregs[n - IDX_FP_REGS];
+        fp->low = le64_to_cpu(* (uint64_t *) mem_buf);
+        fp->high = le16_to_cpu(* (uint16_t *) (mem_buf + 8));
         return 10;
     } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
         n -= IDX_XMM_REGS;