Patchwork [3/4] target-arm: Fix VFP register byte order in GDB remote

login
register
mail settings
Submitter Fabien Chouteau
Date March 1, 2013, 5:21 p.m.
Message ID <1362158507-19310-4-git-send-email-chouteau@adacore.com>
Download mbox | patch
Permalink /patch/224418/
State New
Headers show

Comments

Fabien Chouteau - March 1, 2013, 5:21 p.m.
From GDB Remote Serial Protocol doc:

"The bytes with the register are transmitted in target byte order."

Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
---
 target-arm/helper.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)
Paul Brook - March 1, 2013, 8:51 p.m.
> From GDB Remote Serial Protocol doc:
> 
> "The bytes with the register are transmitted in target byte order."

>          /* Aliases for Q regs.  */
>          nregs += 16;
>          if (reg < nregs) {
> 
> -            stfq_le_p(buf, env->vfp.regs[(reg - 32) * 2]);
> -            stfq_le_p(buf + 8, env->vfp.regs[(reg - 32) * 2 + 1]);
> +            stfq_p(buf, env->vfp.regs[(reg - 32) * 2]);
> +            stfq_p(buf + 8, env->vfp.regs[(reg - 32) * 2 + 1]);

This is wrong. You're still using little-endian ordering of words.

Paul
Fabien Chouteau - March 4, 2013, 10:03 a.m.
On 03/01/2013 09:51 PM, Paul Brook wrote:
>> From GDB Remote Serial Protocol doc:
>>
>> "The bytes with the register are transmitted in target byte order."
> 
>>          /* Aliases for Q regs.  */
>>          nregs += 16;
>>          if (reg < nregs) {
>>
>> -            stfq_le_p(buf, env->vfp.regs[(reg - 32) * 2]);
>> -            stfq_le_p(buf + 8, env->vfp.regs[(reg - 32) * 2 + 1]);
>> +            stfq_p(buf, env->vfp.regs[(reg - 32) * 2]);
>> +            stfq_p(buf + 8, env->vfp.regs[(reg - 32) * 2 + 1]);
> 
> This is wrong. You're still using little-endian ordering of words.
> 

Can you explain a little bit further? If I'm in big-endian mode, stfq_p() will be stfq_be_p(), right?

Thanks,
Paul Brook - March 4, 2013, 1:30 p.m.
> >> "The bytes with the register are transmitted in target byte order."
> >> 
> >>          /* Aliases for Q regs.  */
> >>          nregs += 16;
> >>          if (reg < nregs) {
> >> 
> >> -            stfq_le_p(buf, env->vfp.regs[(reg - 32) * 2]);
> >> -            stfq_le_p(buf + 8, env->vfp.regs[(reg - 32) * 2 + 1]);
> >> +            stfq_p(buf, env->vfp.regs[(reg - 32) * 2]);
> >> +            stfq_p(buf + 8, env->vfp.regs[(reg - 32) * 2 + 1]);
> > 
> > This is wrong. You're still using little-endian ordering of words.
> 
> Can you explain a little bit further? If I'm in big-endian mode, stfq_p()
> will be stfq_be_p(), right?

Because we're actually storing two halves of a 128-bit value.   You still 
store the least significant half first.

Paul
Fabien Chouteau - March 4, 2013, 5:31 p.m.
On 03/04/2013 02:30 PM, Paul Brook wrote:
>>>> "The bytes with the register are transmitted in target byte order."
>>>>
>>>>          /* Aliases for Q regs.  */
>>>>          nregs += 16;
>>>>          if (reg < nregs) {
>>>>
>>>> -            stfq_le_p(buf, env->vfp.regs[(reg - 32) * 2]);
>>>> -            stfq_le_p(buf + 8, env->vfp.regs[(reg - 32) * 2 + 1]);
>>>> +            stfq_p(buf, env->vfp.regs[(reg - 32) * 2]);
>>>> +            stfq_p(buf + 8, env->vfp.regs[(reg - 32) * 2 + 1]);
>>>
>>> This is wrong. You're still using little-endian ordering of words.
>>
>> Can you explain a little bit further? If I'm in big-endian mode, stfq_p()
>> will be stfq_be_p(), right?
> 
> Because we're actually storing two halves of a 128-bit value.   You still 
> store the least significant half first.
> 

Right, I'm sorry I didn't see you comment was only about the Q registers.
What would be the solution then?

#ifdef TARGET_WORDS_BIGENDIAN
            stfq_p(buf, env->vfp.regs[(reg - 32) * 2 + 1]);
            stfq_p(buf + 8, env->vfp.regs[(reg - 32) * 2]);
#else
            stfq_p(buf, env->vfp.regs[(reg - 32) * 2]);
            stfq_p(buf + 8, env->vfp.regs[(reg - 32) * 2 + 1]);
#endif  /* TARGET_WORDS_BIGENDIAN */

Regards,
Fabien Chouteau - March 5, 2013, 10:59 a.m.
On 03/05/2013 12:34 AM, Paul Brook wrote:
>>> Because we're actually storing two halves of a 128-bit value.   You still
>>> store the least significant half first.
>>
>> Right, I'm sorry I didn't see you comment was only about the Q registers.
>> What would be the solution then?
>>
>> #ifdef TARGET_WORDS_BIGENDIAN
>>             stfq_p(buf, env->vfp.regs[(reg - 32) * 2 + 1]);
>>             stfq_p(buf + 8, env->vfp.regs[(reg - 32) * 2]);
>> #else
>>             stfq_p(buf, env->vfp.regs[(reg - 32) * 2]);
>>             stfq_p(buf + 8, env->vfp.regs[(reg - 32) * 2 + 1]);
>> #endif  /* TARGET_WORDS_BIGENDIAN */
> 
> Yes, something like that.
> 

Alright, I'll do that for V2.

Thanks,

Patch

diff --git a/target-arm/helper.c b/target-arm/helper.c
index e97e1a5..75ee0dc 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -16,18 +16,17 @@  static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
 {
     int nregs;
 
-    /* VFP data registers are always little-endian.  */
     nregs = arm_feature(env, ARM_FEATURE_VFP3) ? 32 : 16;
     if (reg < nregs) {
-        stfq_le_p(buf, env->vfp.regs[reg]);
+        stfq_p(buf, env->vfp.regs[reg]);
         return 8;
     }
     if (arm_feature(env, ARM_FEATURE_NEON)) {
         /* Aliases for Q regs.  */
         nregs += 16;
         if (reg < nregs) {
-            stfq_le_p(buf, env->vfp.regs[(reg - 32) * 2]);
-            stfq_le_p(buf + 8, env->vfp.regs[(reg - 32) * 2 + 1]);
+            stfq_p(buf, env->vfp.regs[(reg - 32) * 2]);
+            stfq_p(buf + 8, env->vfp.regs[(reg - 32) * 2 + 1]);
             return 16;
         }
     }
@@ -45,14 +44,14 @@  static int vfp_gdb_set_reg(CPUARMState *env, uint8_t *buf, int reg)
 
     nregs = arm_feature(env, ARM_FEATURE_VFP3) ? 32 : 16;
     if (reg < nregs) {
-        env->vfp.regs[reg] = ldfq_le_p(buf);
+        env->vfp.regs[reg] = ldfq_p(buf);
         return 8;
     }
     if (arm_feature(env, ARM_FEATURE_NEON)) {
         nregs += 16;
         if (reg < nregs) {
-            env->vfp.regs[(reg - 32) * 2] = ldfq_le_p(buf);
-            env->vfp.regs[(reg - 32) * 2 + 1] = ldfq_le_p(buf + 8);
+            env->vfp.regs[(reg - 32) * 2] = ldfq_p(buf);
+            env->vfp.regs[(reg - 32) * 2 + 1] = ldfq_p(buf + 8);
             return 16;
         }
     }