| Message ID | 1363694598-19799-1-git-send-email-chouteau@adacore.com |
|---|---|
| State | New |
| Headers | show |
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 >
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
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
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)
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
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,
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
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~
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
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;
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(-)