[Qemu-devel] [PATCH] PPC/GDB: handle read and write of fpscr

[Qemu-devel] [PATCH] PPC/GDB: handle read and write of fpscr

[Fabien Chouteau]

Although the support of this register may be uncomplete, there are no
reason to prevent the debugger from reading or writing it.

Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
---
 gdbstub.c                   |    3 ++-
 target-ppc/translate_init.c |    2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index e414ad9..d23d9c5 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -781,7 +781,8 @@ static int cpu_gdb_write_register(CPUPPCState *env, uint8_t *mem_buf, int n)
             /* fpscr */
             if (gdb_has_xml)
                 return 0;
-            return 4;
+            env->fpscr = ldtul_p(mem_buf);
+            return sizeof(target_ulong);
         }
     }
     return 0;
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 09ad4ba..a5d2cc3 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -7693,7 +7693,7 @@ static int gdb_set_float_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
         return 8;
     }
     if (n == 32) {
-        /* FPSCR not implemented  */
+        env->fpscr = ldl_p(mem_buf);
         return 4;
     }
     return 0;
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] PPC/GDB: handle read and write of fpscr

[Alexander Graf]

On 19.03.2013, at 13:03, Fabien Chouteau wrote:

> Although the support of this register may be uncomplete, there are no
> reason to prevent the debugger from reading or writing it.
> 
> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
> ---
> gdbstub.c                   |    3 ++-
> target-ppc/translate_init.c |    2 +-
> 2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index e414ad9..d23d9c5 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -781,7 +781,8 @@ static int cpu_gdb_write_register(CPUPPCState *env, uint8_t *mem_buf, int n)
>             /* fpscr */
>             if (gdb_has_xml)
>                 return 0;
> -            return 4;
> +            env->fpscr = ldtul_p(mem_buf);

Check out helper_store_fpscr() in target-ppc/fpu_helper.c. Storing fpscr has a bunch of side effects that won't happen when you just set the env variable. I'd prefer not to enable users to set fpscr when we can't guarantee that the updated values are actually used.

Can't you just call the helper function here?


Alex

> +            return sizeof(target_ulong);
>         }
>     }
>     return 0;
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 09ad4ba..a5d2cc3 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -7693,7 +7693,7 @@ static int gdb_set_float_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
>         return 8;
>     }
>     if (n == 32) {
> -        /* FPSCR not implemented  */
> +        env->fpscr = ldl_p(mem_buf);
>         return 4;
>     }
>     return 0;
> -- 
> 1.7.9.5
> 

Re: [Qemu-devel] [PATCH] PPC/GDB: handle read and write of fpscr

[Peter Maydell]

On 19 March 2013 12:10, Alexander Graf <agraf@suse.de> wrote:
> Check out helper_store_fpscr() in target-ppc/fpu_helper.c. Storing
> fpscr has a bunch of side effects that won't happen when you just
> set the env variable.

Speaking of which, am I missing something, or does the code
in machine.c do nothing to ensure that we call set_float_rounding_mode
to set the fpu/ emulation code up with the right rounding mode
when loading CPU state on incoming migration?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] PPC/GDB: handle read and write of fpscr

[Alexander Graf]

On 19.03.2013, at 13:13, Peter Maydell wrote:

> On 19 March 2013 12:10, Alexander Graf <agraf@suse.de> wrote:
>> Check out helper_store_fpscr() in target-ppc/fpu_helper.c. Storing
>> fpscr has a bunch of side effects that won't happen when you just
>> set the env variable.
> 
> Speaking of which, am I missing something, or does the code
> in machine.c do nothing to ensure that we call set_float_rounding_mode
> to set the fpu/ emulation code up with the right rounding mode
> when loading CPU state on incoming migration?

I very much doubt anyone tests migration with TCG :)


Alex

Re: [Qemu-devel] [PATCH] PPC/GDB: handle read and write of fpscr

[Fabien Chouteau]

On 03/19/2013 01:10 PM, Alexander Graf wrote:
> 
> On 19.03.2013, at 13:03, Fabien Chouteau wrote:
> 
>> Although the support of this register may be uncomplete, there are no
>> reason to prevent the debugger from reading or writing it.
>>
>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
>> ---
>> gdbstub.c                   |    3 ++-
>> target-ppc/translate_init.c |    2 +-
>> 2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index e414ad9..d23d9c5 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -781,7 +781,8 @@ static int cpu_gdb_write_register(CPUPPCState *env, uint8_t *mem_buf, int n)
>>             /* fpscr */
>>             if (gdb_has_xml)
>>                 return 0;
>> -            return 4;
>> +            env->fpscr = ldtul_p(mem_buf);
> 
> Check out helper_store_fpscr() in target-ppc/fpu_helper.c. Storing fpscr has a bunch of side effects that won't happen when you just set the env variable. I'd prefer not to enable users to set fpscr when we can't guarantee that the updated values are actually used.
> 
> Can't you just call the helper function here?
> 

I can if I include helper.h, unfortunately this file is named helpers.h
(note the 's') for m68k so I wont be able to include it for all
platforms.

Either I put an include in the middle of the source (in a #if defined
(TARGET_PPC) section), I don't know if this is allowed in Qemu:

@@ -691,6 +695,8 @@ static int cpu_gdb_write_register(CPUX86State *env, uint8_t *mem_buf, int n)

 #elif defined (TARGET_PPC)

+#include "helper.h"
+
 /* Old gdb always expects FP registers.  Newer (xml-aware) gdb only
    expects whatever the target description contains.  Due to a
    historical mishap the FP registers appear in between core integer


or I just add a new PPC section after the others includes:

@@ -42,6 +42,10 @@
 #include "sysemu/kvm.h"
 #include "qemu/bitops.h"

+#if defined (TARGET_PPC)
+#include "helper.h"
+#endif
+
 #ifndef TARGET_CPU_MEMORY_RW_DEBUG
 static inline int target_memory_rw_debug(CPUArchState *env, target_ulong addr,
                                          uint8_t *buf, int len, int is_write)


-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH] PPC/GDB: handle read and write of fpscr

[Peter Maydell]

On 19 March 2013 15:14, Fabien Chouteau <chouteau@adacore.com> wrote:
> I can if I include helper.h, unfortunately this file is named helpers.h
> (note the 's') for m68k so I wont be able to include it for all
> platforms.

The way we do this on ARM for cpsr_read is:
 * cpsr_read() is defined in target-arm/cpu.h
 * gdbstub.c calls cpsr_read()
 * HELPER(cpsr_read) is mostly a wrapper calling cpsr_read()
 * helper.h declares HELPER(cpsr_read) as usual

I'm not sure you want to include helper.h.

(m68k should have helpers.h renamed to bring it into line with
everything else, though.)

-- PMM

Re: [Qemu-devel] [PATCH] PPC/GDB: handle read and write of fpscr

[Fabien Chouteau]

On 03/19/2013 04:23 PM, Peter Maydell wrote:
> On 19 March 2013 15:14, Fabien Chouteau <chouteau@adacore.com> wrote:
>> I can if I include helper.h, unfortunately this file is named helpers.h
>> (note the 's') for m68k so I wont be able to include it for all
>> platforms.
> 
> The way we do this on ARM for cpsr_read is:
>  * cpsr_read() is defined in target-arm/cpu.h
>  * gdbstub.c calls cpsr_read()
>  * HELPER(cpsr_read) is mostly a wrapper calling cpsr_read()
>  * helper.h declares HELPER(cpsr_read) as usual
> 
> I'm not sure you want to include helper.h.
> 

Cleaner solution indeed. I'll do the opposite, store_fpscr calls helper_store_fpscr.

Thanks,

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH] PPC/GDB: handle read and write of fpscr

[Peter Maydell]

On 19 March 2013 17:20, Fabien Chouteau <chouteau@adacore.com> wrote:
> On 03/19/2013 04:23 PM, Peter Maydell wrote:
>> The way we do this on ARM for cpsr_read is:
>>  * cpsr_read() is defined in target-arm/cpu.h
>>  * gdbstub.c calls cpsr_read()
>>  * HELPER(cpsr_read) is mostly a wrapper calling cpsr_read()
>>  * helper.h declares HELPER(cpsr_read) as usual
>>
>> I'm not sure you want to include helper.h.
>>
>
> Cleaner solution indeed. I'll do the opposite, store_fpscr
> calls helper_store_fpscr.

I think that way round is kind of backwards personally (HELPER
functions are for TCG generated code to call) but it doesn't
make much difference really.

-- PMM

Re: [Qemu-devel] [PATCH] PPC/GDB: handle read and write of fpscr

[Richard Henderson]

On 03/19/2013 10:20 AM, Fabien Chouteau wrote:
> Cleaner solution indeed. I'll do the opposite, store_fpscr calls helper_store_fpscr.

The declarations won't be in the right places for that to work right.

Canonically, cpu_store_fpscr should be declared in cpu.h, have the leading
"cpu_" prefix, and then helper_store_fpscr is declared (via macro) in helper.h
and its definition should call the cpu.h function.


r~

Re: [Qemu-devel] [PATCH] PPC/GDB: handle read and write of fpscr

[Alexander Graf]

On 19.03.2013, at 19:32, Richard Henderson wrote:

> On 03/19/2013 10:20 AM, Fabien Chouteau wrote:
>> Cleaner solution indeed. I'll do the opposite, store_fpscr calls helper_store_fpscr.
> 
> The declarations won't be in the right places for that to work right.
> 
> Canonically, cpu_store_fpscr should be declared in cpu.h, have the leading
> "cpu_" prefix, and then helper_store_fpscr is declared (via macro) in helper.h
> and its definition should call the cpu.h function.

Heh :) Fabien did a clever workaround that one in his v2. Mind to check whether it's ok for you?


Alex