diff mbox series

[v2,06/14] target/arm: use gdb_get_reg helpers

Message ID 20191130084602.10818-7-alex.bennee@linaro.org
State Superseded
Headers show
Series gdbstub refactor and SVE support | expand

Commit Message

Alex Bennée Nov. 30, 2019, 8:45 a.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>


---
v2
  - make sure we pass hi/lo correctly as quads are stored in LE order
---
 target/arm/helper.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

-- 
2.20.1

Comments

Philippe Mathieu-Daudé Dec. 1, 2019, 8:05 p.m. UTC | #1 
On 11/30/19 9:45 AM, 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>

> 

> ---

> v2

>    - make sure we pass hi/lo correctly as quads are stored in LE order

> ---

>   target/arm/helper.c | 18 +++++++-----------

>   1 file changed, 7 insertions(+), 11 deletions(-)

> 

> diff --git a/target/arm/helper.c b/target/arm/helper.c

> index 0bf8f53d4b8..0ac950d6c71 100644

> --- a/target/arm/helper.c

> +++ b/target/arm/helper.c

> @@ -105,21 +105,17 @@ static int aarch64_fpu_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)

>   {

>       switch (reg) {

>       case 0 ... 31:

> -        /* 128 bit FP register */

> -        {

> -            uint64_t *q = aa64_vfp_qreg(env, reg);

> -            stq_le_p(buf, q[0]);

> -            stq_le_p(buf + 8, q[1]);

> -            return 16;

> -        }

> +    {

> +        /* 128 bit FP register - quads are in LE order */


Oh, this was always wrong on BE :(

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>


> +        uint64_t *q = aa64_vfp_qreg(env, reg);

> +        return gdb_get_reg128(buf, q[1], q[0]);

> +    }

>       case 32:

>           /* FPSR */

> -        stl_p(buf, vfp_get_fpsr(env));

> -        return 4;

> +        return gdb_get_reg32(buf, vfp_get_fpsr(env));

>       case 33:

>           /* FPCR */

> -        stl_p(buf, vfp_get_fpcr(env));

> -        return 4;

> +        return gdb_get_reg32(buf,vfp_get_fpcr(env));

>       default:

>           return 0;

>       }

>
Richard Henderson Dec. 2, 2019, 2:20 a.m. UTC | #2 
On 11/30/19 8:45 AM, 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>

> 

> ---

> v2

>   - make sure we pass hi/lo correctly as quads are stored in LE order

> ---

>  target/arm/helper.c | 18 +++++++-----------

>  1 file changed, 7 insertions(+), 11 deletions(-)


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~
Alan Hayward Dec. 2, 2019, 10:05 a.m. UTC | #3 
> On 1 Dec 2019, at 20:05, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> 

> On 11/30/19 9:45 AM, 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>

>> ---

>> v2

>>   - make sure we pass hi/lo correctly as quads are stored in LE order

>> ---

>>  target/arm/helper.c | 18 +++++++-----------

>>  1 file changed, 7 insertions(+), 11 deletions(-)

>> diff --git a/target/arm/helper.c b/target/arm/helper.c

>> index 0bf8f53d4b8..0ac950d6c71 100644

>> --- a/target/arm/helper.c

>> +++ b/target/arm/helper.c

>> @@ -105,21 +105,17 @@ static int aarch64_fpu_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)

>>  {

>>      switch (reg) {

>>      case 0 ... 31:

>> -        /* 128 bit FP register */

>> -        {

>> -            uint64_t *q = aa64_vfp_qreg(env, reg);

>> -            stq_le_p(buf, q[0]);

>> -            stq_le_p(buf + 8, q[1]);

>> -            return 16;

>> -        }

>> +    {

>> +        /* 128 bit FP register - quads are in LE order */

> 

> Oh, this was always wrong on BE :(


Am I right in thinking this patch correctly matches the SVE BE changes from June?

Specifically, this patch:
http://lists.infradead.org/pipermail/linux-arm-kernel/2019-June/657826.html


Alan.

> 

> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 

>> +        uint64_t *q = aa64_vfp_qreg(env, reg);

>> +        return gdb_get_reg128(buf, q[1], q[0]);

>> +    }

>>      case 32:

>>          /* FPSR */

>> -        stl_p(buf, vfp_get_fpsr(env));

>> -        return 4;

>> +        return gdb_get_reg32(buf, vfp_get_fpsr(env));

>>      case 33:

>>          /* FPCR */

>> -        stl_p(buf, vfp_get_fpcr(env));

>> -        return 4;

>> +        return gdb_get_reg32(buf,vfp_get_fpcr(env));

>>      default:

>>          return 0;

>>      }
Alex Bennée Dec. 5, 2019, 5:58 p.m. UTC | #4 
Alan Hayward <Alan.Hayward@arm.com> writes:

>> On 1 Dec 2019, at 20:05, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

>> 

>> On 11/30/19 9:45 AM, 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>

>>> ---

>>> v2

>>>   - make sure we pass hi/lo correctly as quads are stored in LE order

>>> ---

>>>  target/arm/helper.c | 18 +++++++-----------

>>>  1 file changed, 7 insertions(+), 11 deletions(-)

>>> diff --git a/target/arm/helper.c b/target/arm/helper.c

>>> index 0bf8f53d4b8..0ac950d6c71 100644

>>> --- a/target/arm/helper.c

>>> +++ b/target/arm/helper.c

>>> @@ -105,21 +105,17 @@ static int aarch64_fpu_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)

>>>  {

>>>      switch (reg) {

>>>      case 0 ... 31:

>>> -        /* 128 bit FP register */

>>> -        {

>>> -            uint64_t *q = aa64_vfp_qreg(env, reg);

>>> -            stq_le_p(buf, q[0]);

>>> -            stq_le_p(buf + 8, q[1]);

>>> -            return 16;

>>> -        }

>>> +    {

>>> +        /* 128 bit FP register - quads are in LE order */

>> 

>> Oh, this was always wrong on BE :(

>

> Am I right in thinking this patch correctly matches the SVE BE changes from June?

>

> Specifically, this patch:

> http://lists.infradead.org/pipermail/linux-arm-kernel/2019-June/657826.html


Not quite. This is just taking into account the way we store the data
internally in cpu.h. The gdb_get_reg128 helper will then ensure stuff is
in target endian format which is what gdbstub defines.

There aren't any actual kernel to userspace transfers going on here.

>

>

> Alan.

>

>> 

>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>> 

>>> +        uint64_t *q = aa64_vfp_qreg(env, reg);

>>> +        return gdb_get_reg128(buf, q[1], q[0]);

>>> +    }

>>>      case 32:

>>>          /* FPSR */

>>> -        stl_p(buf, vfp_get_fpsr(env));

>>> -        return 4;

>>> +        return gdb_get_reg32(buf, vfp_get_fpsr(env));

>>>      case 33:

>>>          /* FPCR */

>>> -        stl_p(buf, vfp_get_fpcr(env));

>>> -        return 4;

>>> +        return gdb_get_reg32(buf,vfp_get_fpcr(env));

>>>      default:

>>>          return 0;

>>>      }



-- 
Alex Bennée
diff mbox series

Patch

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 0bf8f53d4b8..0ac950d6c71 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -105,21 +105,17 @@  static int aarch64_fpu_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
 {
     switch (reg) {
     case 0 ... 31:
-        /* 128 bit FP register */
-        {
-            uint64_t *q = aa64_vfp_qreg(env, reg);
-            stq_le_p(buf, q[0]);
-            stq_le_p(buf + 8, q[1]);
-            return 16;
-        }
+    {
+        /* 128 bit FP register - quads are in LE order */
+        uint64_t *q = aa64_vfp_qreg(env, reg);
+        return gdb_get_reg128(buf, q[1], q[0]);
+    }
     case 32:
         /* FPSR */
-        stl_p(buf, vfp_get_fpsr(env));
-        return 4;
+        return gdb_get_reg32(buf, vfp_get_fpsr(env));
     case 33:
         /* FPCR */
-        stl_p(buf, vfp_get_fpcr(env));
-        return 4;
+        return gdb_get_reg32(buf,vfp_get_fpcr(env));
     default:
         return 0;
     }