Re: [Qemu-devel] [PATCH] [SPARC] Gdbstub: Fix back-trace on SPARC32

2011-09-08 Thread Fabien Chouteau
On 07/09/2011 21:02, Blue Swirl wrote:
 On Tue, Sep 6, 2011 at 10:38 AM, Fabien Chouteau chout...@adacore.com wrote:
 On 05/09/2011 21:22, Blue Swirl wrote:
 On Mon, Sep 5, 2011 at 9:33 AM, Fabien Chouteau chout...@adacore.com 
 wrote:
 On 03/09/2011 11:25, Blue Swirl wrote:
 On Thu, Sep 1, 2011 at 2:17 PM, Fabien Chouteau chout...@adacore.com 
 wrote:
 Gdb expects all registers windows to be flushed in ram, which is not the 
 case
 in Qemu. Therefore the back-trace generation doesn't work. This patch 
 adds a
 function to handle reads/writes in stack frames as if windows were 
 flushed.

 Signed-off-by: Fabien Chouteau chout...@adacore.com
 ---
  gdbstub.c |   10 --
  target-sparc/cpu.h|7 
  target-sparc/helper.c |   85 
 +
  3 files changed, 99 insertions(+), 3 deletions(-)

 diff --git a/gdbstub.c b/gdbstub.c
 index 3b87c27..85d5ad7 100644
 --- a/gdbstub.c
 +++ b/gdbstub.c
 @@ -41,6 +41,9 @@
  #include qemu_socket.h
  #include kvm.h

 +#ifndef TARGET_CPU_MEMORY_RW_DEBUG
 +#define TARGET_CPU_MEMORY_RW_DEBUG cpu_memory_rw_debug

 These days, inline functions are preferred over macros.


 This is to allow target-specific implementation of the function.

 That can be done with inline functions too.

 OK, how do you do that?
 
 #ifndef TARGET_CPU_MEMORY_RW_DEBUG
 int target_memory_rw_debug(CPUState *env, target_ulong addr,
   uint8_t *buf, int len, int is_write)
 {
 return cpu_memory_rw_debug(env, addr, buf, len, is_write);
 }
 #else
 /* target_memory_rw_debug() defined in cpu.h */
 #endif
 

OK, understood.


 +#endif

  enum {
 GDB_SIGNAL_0 = 0,
 @@ -2013,7 +2016,7 @@ static int gdb_handle_packet(GDBState *s, const 
 char *line_buf)
 if (*p == ',')
 p++;
 len = strtoull(p, NULL, 16);
 -if (cpu_memory_rw_debug(s-g_cpu, addr, mem_buf, len, 0) != 0) {
 +if (TARGET_CPU_MEMORY_RW_DEBUG(s-g_cpu, addr, mem_buf, len, 0) 
 != 0) {

 cpu_memory_rw_debug() could remain unwrapped with a generic function
 like cpu_gdb_sync_memory() which gdbstub should explicitly call.

 Maybe the lazy condition codes etc. could be handled in similar way,
 cpu_gdb_sync_registers().


 Excuse me, I don't understand here.

 cpu_gdb_{read,write}_register needs to force calculation of lazy
 condition codes. On Sparc this is handled by cpu_get_psr(), so it is
 not explicit.

 I still don't understand you point. Do you suggest a cpu_gdb_sync_memory() 
 that
 will flush register windows?

 Not really but nevermind.

 +
 +/* Gdb expects all registers windows to be flushed in ram. This 
 function handles
 + * reads/writes in stack frames as if windows were flushed. We assume 
 that the
 + * sparc ABI is followed.
 + */

 We can't assume that, it depends on what we are executing (BIOS, OS,
 even application).

 Well, maybe the statement is too strong. The ABI is required to get a valid
 result. Gdb cannot build back-traces if the ABI is not followed anyway.

 But if the ABI assumption happens to be wrong (for example registers
 contain random values), memory may be corrupted because this would
 happily use whatever the registers contain.

 This cannot corrupt memory, the point is to read/write in registers instead 
 of
 memory.

 Sorry, I misread a part of the patch, guest memory is not written
 unlike I mistakenly assumed (simple register to memory flush).
 However, wrong ABI assumption may instead corrupt the registers.

 Another way to fix this would be that GDB would tell QEMU what ABI to
 use for flushing. But how would one tell GDB about a non-standard ABI?

 For user emulators we can make ABI assumptions, there similar patch
 could make sense. But system emulators can't assume anything about the
 guest OS, it could be Linux, *BSD, a commercial OS or even a toy OS.

 I think all of these kernels follow the SPARC32 ABI, and if they don't Gdb
 cannot handle them anyway.

 This solution covers 99% of the problem.

 As is, it's not 100% correct and the failure case is destructive. But
 would it make sense if the registers were not touched on write? Then
 to GDB the windows would appear as if flushed to memory, but like real
 hardware the registers would not automatically get updated from memory
 if it's changed by GDB. I don't think corruption would be possible in
 that case, though GDB (or the user) could get temporarily confused if
 a read from memory location would not return its true value.


I think this might be the best compromise. So I'll just handle reads in
register windows.

 BTW, cpu_cwp_inc() is called but there is no effort to restore CWP afterward.


The CWP in CPUState is never modified by cpu_cpw_inc().

Version 2 is on its way...

Regards,

-- 
Fabien Chouteau



Re: [Qemu-devel] [PATCH] [SPARC] Gdbstub: Fix back-trace on SPARC32

2011-09-07 Thread Blue Swirl
On Tue, Sep 6, 2011 at 10:38 AM, Fabien Chouteau chout...@adacore.com wrote:
 On 05/09/2011 21:22, Blue Swirl wrote:
 On Mon, Sep 5, 2011 at 9:33 AM, Fabien Chouteau chout...@adacore.com wrote:
 On 03/09/2011 11:25, Blue Swirl wrote:
 On Thu, Sep 1, 2011 at 2:17 PM, Fabien Chouteau chout...@adacore.com 
 wrote:
 Gdb expects all registers windows to be flushed in ram, which is not the 
 case
 in Qemu. Therefore the back-trace generation doesn't work. This patch 
 adds a
 function to handle reads/writes in stack frames as if windows were 
 flushed.

 Signed-off-by: Fabien Chouteau chout...@adacore.com
 ---
  gdbstub.c             |   10 --
  target-sparc/cpu.h    |    7 
  target-sparc/helper.c |   85 
 +
  3 files changed, 99 insertions(+), 3 deletions(-)

 diff --git a/gdbstub.c b/gdbstub.c
 index 3b87c27..85d5ad7 100644
 --- a/gdbstub.c
 +++ b/gdbstub.c
 @@ -41,6 +41,9 @@
  #include qemu_socket.h
  #include kvm.h

 +#ifndef TARGET_CPU_MEMORY_RW_DEBUG
 +#define TARGET_CPU_MEMORY_RW_DEBUG cpu_memory_rw_debug

 These days, inline functions are preferred over macros.


 This is to allow target-specific implementation of the function.

 That can be done with inline functions too.

 OK, how do you do that?

#ifndef TARGET_CPU_MEMORY_RW_DEBUG
int target_memory_rw_debug(CPUState *env, target_ulong addr,
  uint8_t *buf, int len, int is_write)
{
return cpu_memory_rw_debug(env, addr, buf, len, is_write);
}
#else
/* target_memory_rw_debug() defined in cpu.h */
#endif

 +#endif

  enum {
     GDB_SIGNAL_0 = 0,
 @@ -2013,7 +2016,7 @@ static int gdb_handle_packet(GDBState *s, const 
 char *line_buf)
         if (*p == ',')
             p++;
         len = strtoull(p, NULL, 16);
 -        if (cpu_memory_rw_debug(s-g_cpu, addr, mem_buf, len, 0) != 0) {
 +        if (TARGET_CPU_MEMORY_RW_DEBUG(s-g_cpu, addr, mem_buf, len, 0) 
 != 0) {

 cpu_memory_rw_debug() could remain unwrapped with a generic function
 like cpu_gdb_sync_memory() which gdbstub should explicitly call.

 Maybe the lazy condition codes etc. could be handled in similar way,
 cpu_gdb_sync_registers().


 Excuse me, I don't understand here.

 cpu_gdb_{read,write}_register needs to force calculation of lazy
 condition codes. On Sparc this is handled by cpu_get_psr(), so it is
 not explicit.

 I still don't understand you point. Do you suggest a cpu_gdb_sync_memory() 
 that
 will flush register windows?

Not really but nevermind.

 +
 +/* Gdb expects all registers windows to be flushed in ram. This function 
 handles
 + * reads/writes in stack frames as if windows were flushed. We assume 
 that the
 + * sparc ABI is followed.
 + */

 We can't assume that, it depends on what we are executing (BIOS, OS,
 even application).

 Well, maybe the statement is too strong. The ABI is required to get a valid
 result. Gdb cannot build back-traces if the ABI is not followed anyway.

 But if the ABI assumption happens to be wrong (for example registers
 contain random values), memory may be corrupted because this would
 happily use whatever the registers contain.

 This cannot corrupt memory, the point is to read/write in registers instead of
 memory.

Sorry, I misread a part of the patch, guest memory is not written
unlike I mistakenly assumed (simple register to memory flush).
However, wrong ABI assumption may instead corrupt the registers.

 Another way to fix this would be that GDB would tell QEMU what ABI to
 use for flushing. But how would one tell GDB about a non-standard ABI?

 For user emulators we can make ABI assumptions, there similar patch
 could make sense. But system emulators can't assume anything about the
 guest OS, it could be Linux, *BSD, a commercial OS or even a toy OS.

 I think all of these kernels follow the SPARC32 ABI, and if they don't Gdb
 cannot handle them anyway.

 This solution covers 99% of the problem.

As is, it's not 100% correct and the failure case is destructive. But
would it make sense if the registers were not touched on write? Then
to GDB the windows would appear as if flushed to memory, but like real
hardware the registers would not automatically get updated from memory
if it's changed by GDB. I don't think corruption would be possible in
that case, though GDB (or the user) could get temporarily confused if
a read from memory location would not return its true value.

BTW, cpu_cwp_inc() is called but there is no effort to restore CWP afterward.

 On Sparc64 there are two ABIs (32 bit and 64 bit
 with offset of -2047), though calling flushw instruction could handle
 that.

 This solution is for SPARC32 only.

 If the flush happens to trigger a fault, we're in big trouble.


 That's why it's safer/easier to use this hackish read/write in the 
 registers.

 No, if the fault happens here, handling it may be tricky. See for
 example what paranoia Linux has to do for user window flushing, it
 involves the no-fault mode in MMU.

 There's no possible 

Re: [Qemu-devel] [PATCH] [SPARC] Gdbstub: Fix back-trace on SPARC32

2011-09-06 Thread Fabien Chouteau
On 05/09/2011 21:22, Blue Swirl wrote:
 On Mon, Sep 5, 2011 at 9:33 AM, Fabien Chouteau chout...@adacore.com wrote:
 On 03/09/2011 11:25, Blue Swirl wrote:
 On Thu, Sep 1, 2011 at 2:17 PM, Fabien Chouteau chout...@adacore.com 
 wrote:
 Gdb expects all registers windows to be flushed in ram, which is not the 
 case
 in Qemu. Therefore the back-trace generation doesn't work. This patch adds 
 a
 function to handle reads/writes in stack frames as if windows were flushed.

 Signed-off-by: Fabien Chouteau chout...@adacore.com
 ---
  gdbstub.c |   10 --
  target-sparc/cpu.h|7 
  target-sparc/helper.c |   85 
 +
  3 files changed, 99 insertions(+), 3 deletions(-)

 diff --git a/gdbstub.c b/gdbstub.c
 index 3b87c27..85d5ad7 100644
 --- a/gdbstub.c
 +++ b/gdbstub.c
 @@ -41,6 +41,9 @@
  #include qemu_socket.h
  #include kvm.h

 +#ifndef TARGET_CPU_MEMORY_RW_DEBUG
 +#define TARGET_CPU_MEMORY_RW_DEBUG cpu_memory_rw_debug

 These days, inline functions are preferred over macros.


 This is to allow target-specific implementation of the function.

 That can be done with inline functions too.

OK, how do you do that?

 +#endif

  enum {
 GDB_SIGNAL_0 = 0,
 @@ -2013,7 +2016,7 @@ static int gdb_handle_packet(GDBState *s, const char 
 *line_buf)
 if (*p == ',')
 p++;
 len = strtoull(p, NULL, 16);
 -if (cpu_memory_rw_debug(s-g_cpu, addr, mem_buf, len, 0) != 0) {
 +if (TARGET_CPU_MEMORY_RW_DEBUG(s-g_cpu, addr, mem_buf, len, 0) 
 != 0) {

 cpu_memory_rw_debug() could remain unwrapped with a generic function
 like cpu_gdb_sync_memory() which gdbstub should explicitly call.

 Maybe the lazy condition codes etc. could be handled in similar way,
 cpu_gdb_sync_registers().


 Excuse me, I don't understand here.

 cpu_gdb_{read,write}_register needs to force calculation of lazy
 condition codes. On Sparc this is handled by cpu_get_psr(), so it is
 not explicit.

I still don't understand you point. Do you suggest a cpu_gdb_sync_memory() that
will flush register windows?


 +
 +/* Gdb expects all registers windows to be flushed in ram. This function 
 handles
 + * reads/writes in stack frames as if windows were flushed. We assume 
 that the
 + * sparc ABI is followed.
 + */

 We can't assume that, it depends on what we are executing (BIOS, OS,
 even application).

 Well, maybe the statement is too strong. The ABI is required to get a valid
 result. Gdb cannot build back-traces if the ABI is not followed anyway.

 But if the ABI assumption happens to be wrong (for example registers
 contain random values), memory may be corrupted because this would
 happily use whatever the registers contain.

This cannot corrupt memory, the point is to read/write in registers instead of
memory.


 Another way to fix this would be that GDB would tell QEMU what ABI to
 use for flushing. But how would one tell GDB about a non-standard ABI?

 For user emulators we can make ABI assumptions, there similar patch
 could make sense. But system emulators can't assume anything about the
 guest OS, it could be Linux, *BSD, a commercial OS or even a toy OS.

I think all of these kernels follow the SPARC32 ABI, and if they don't Gdb
cannot handle them anyway.

This solution covers 99% of the problem.


 On Sparc64 there are two ABIs (32 bit and 64 bit
 with offset of -2047), though calling flushw instruction could handle
 that.

 This solution is for SPARC32 only.

 If the flush happens to trigger a fault, we're in big trouble.


 That's why it's safer/easier to use this hackish read/write in the 
 registers.

 No, if the fault happens here, handling it may be tricky. See for
 example what paranoia Linux has to do for user window flushing, it
 involves the no-fault mode in MMU.

There's no possible fault, as we do not read or write in memory, that's the
point of this implementation.


 Overall, I think this is too hackish. Maybe this is a bug in GDB
 instead, information from backtrace is not reliable if ABI is not
 known.


 It's not a bug in Gdb. To build back-traces you have to read stack frames. To
 read stack frames, register windows must be flushed.

 Yes, but the flusher should be GDB, assuming that flushing is even a
 good idea which I doubt.

 Back traces are not reliable in any case. The code could be compiled
 to omit the frame pointer.

This is a corner case. And again, something not supported by Gdb.


 In Qemu we can avoid
 flushing with this little trick.

 This doesn't avoid flushing but performs it magically during GDB memory 
 access.


...instead of writing and reading all register windows each time Gdb stops Qemu.
That's what I call avoid flushing.

Regards,

-- 
Fabien Chouteau



Re: [Qemu-devel] [PATCH] [SPARC] Gdbstub: Fix back-trace on SPARC32

2011-09-05 Thread Fabien Chouteau
On 03/09/2011 11:25, Blue Swirl wrote:
 On Thu, Sep 1, 2011 at 2:17 PM, Fabien Chouteau chout...@adacore.com wrote:
 Gdb expects all registers windows to be flushed in ram, which is not the case
 in Qemu. Therefore the back-trace generation doesn't work. This patch adds a
 function to handle reads/writes in stack frames as if windows were flushed.

 Signed-off-by: Fabien Chouteau chout...@adacore.com
 ---
  gdbstub.c |   10 --
  target-sparc/cpu.h|7 
  target-sparc/helper.c |   85 
 +
  3 files changed, 99 insertions(+), 3 deletions(-)

 diff --git a/gdbstub.c b/gdbstub.c
 index 3b87c27..85d5ad7 100644
 --- a/gdbstub.c
 +++ b/gdbstub.c
 @@ -41,6 +41,9 @@
  #include qemu_socket.h
  #include kvm.h

 +#ifndef TARGET_CPU_MEMORY_RW_DEBUG
 +#define TARGET_CPU_MEMORY_RW_DEBUG cpu_memory_rw_debug

 These days, inline functions are preferred over macros.


This is to allow target-specific implementation of the function.

 +#endif

  enum {
 GDB_SIGNAL_0 = 0,
 @@ -2013,7 +2016,7 @@ static int gdb_handle_packet(GDBState *s, const char 
 *line_buf)
 if (*p == ',')
 p++;
 len = strtoull(p, NULL, 16);
 -if (cpu_memory_rw_debug(s-g_cpu, addr, mem_buf, len, 0) != 0) {
 +if (TARGET_CPU_MEMORY_RW_DEBUG(s-g_cpu, addr, mem_buf, len, 0) != 
 0) {

 cpu_memory_rw_debug() could remain unwrapped with a generic function
 like cpu_gdb_sync_memory() which gdbstub should explicitly call.

 Maybe the lazy condition codes etc. could be handled in similar way,
 cpu_gdb_sync_registers().


Excuse me, I don't understand here.

 put_packet (s, E14);
 } else {
 memtohex(buf, mem_buf, len);
 @@ -2028,10 +2031,11 @@ static int gdb_handle_packet(GDBState *s, const char 
 *line_buf)
 if (*p == ':')
 p++;
 hextomem(mem_buf, p, len);
 -if (cpu_memory_rw_debug(s-g_cpu, addr, mem_buf, len, 1) != 0)
 +if (TARGET_CPU_MEMORY_RW_DEBUG(s-g_cpu, addr, mem_buf, len, 1) != 
 0) {
 put_packet(s, E14);
 -else
 +} else {
 put_packet(s, OK);
 +}
 break;
 case 'p':
 /* Older gdb are really dumb, and don't use 'g' if 'p' is avaialable.
 diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h
 index 8654f26..3f76eaf 100644
 --- a/target-sparc/cpu.h
 +++ b/target-sparc/cpu.h
 @@ -495,6 +495,13 @@ int cpu_sparc_handle_mmu_fault(CPUSPARCState *env1, 
 target_ulong address, int rw
  target_ulong mmu_probe(CPUSPARCState *env, target_ulong address, int 
 mmulev);
  void dump_mmu(FILE *f, fprintf_function cpu_fprintf, CPUState *env);

 +#if !defined(TARGET_SPARC64)
 +int sparc_cpu_memory_rw_debug(CPUState *env, target_ulong addr,
 +  uint8_t *buf, int len, int is_write);
 +#define TARGET_CPU_MEMORY_RW_DEBUG sparc_cpu_memory_rw_debug
 +#endif
 +
 +
  /* translate.c */
  void gen_intermediate_code_init(CPUSPARCState *env);

 diff --git a/target-sparc/helper.c b/target-sparc/helper.c
 index 1fe1f07..2cf4e8b 100644
 --- a/target-sparc/helper.c
 +++ b/target-sparc/helper.c
 @@ -358,6 +358,91 @@ void dump_mmu(FILE *f, fprintf_function cpu_fprintf, 
 CPUState *env)
 }
  }

 +
 +/* Gdb expects all registers windows to be flushed in ram. This function 
 handles
 + * reads/writes in stack frames as if windows were flushed. We assume that 
 the
 + * sparc ABI is followed.
 + */

 We can't assume that, it depends on what we are executing (BIOS, OS,
 even application).

Well, maybe the statement is too strong. The ABI is required to get a valid
result. Gdb cannot build back-traces if the ABI is not followed anyway.

 On Sparc64 there are two ABIs (32 bit and 64 bit
 with offset of -2047), though calling flushw instruction could handle
 that.

This solution is for SPARC32 only.

 If the flush happens to trigger a fault, we're in big trouble.


That's why it's safer/easier to use this hackish read/write in the registers.

 Overall, I think this is too hackish. Maybe this is a bug in GDB
 instead, information from backtrace is not reliable if ABI is not
 known.


It's not a bug in Gdb. To build back-traces you have to read stack frames. To
read stack frames, register windows must be flushed. In Qemu we can avoid
flushing with this little trick.

Regards,

-- 
Fabien Chouteau



Re: [Qemu-devel] [PATCH] [SPARC] Gdbstub: Fix back-trace on SPARC32

2011-09-05 Thread Blue Swirl
On Mon, Sep 5, 2011 at 9:33 AM, Fabien Chouteau chout...@adacore.com wrote:
 On 03/09/2011 11:25, Blue Swirl wrote:
 On Thu, Sep 1, 2011 at 2:17 PM, Fabien Chouteau chout...@adacore.com wrote:
 Gdb expects all registers windows to be flushed in ram, which is not the 
 case
 in Qemu. Therefore the back-trace generation doesn't work. This patch adds a
 function to handle reads/writes in stack frames as if windows were flushed.

 Signed-off-by: Fabien Chouteau chout...@adacore.com
 ---
  gdbstub.c             |   10 --
  target-sparc/cpu.h    |    7 
  target-sparc/helper.c |   85 
 +
  3 files changed, 99 insertions(+), 3 deletions(-)

 diff --git a/gdbstub.c b/gdbstub.c
 index 3b87c27..85d5ad7 100644
 --- a/gdbstub.c
 +++ b/gdbstub.c
 @@ -41,6 +41,9 @@
  #include qemu_socket.h
  #include kvm.h

 +#ifndef TARGET_CPU_MEMORY_RW_DEBUG
 +#define TARGET_CPU_MEMORY_RW_DEBUG cpu_memory_rw_debug

 These days, inline functions are preferred over macros.


 This is to allow target-specific implementation of the function.

That can be done with inline functions too.

 +#endif

  enum {
     GDB_SIGNAL_0 = 0,
 @@ -2013,7 +2016,7 @@ static int gdb_handle_packet(GDBState *s, const char 
 *line_buf)
         if (*p == ',')
             p++;
         len = strtoull(p, NULL, 16);
 -        if (cpu_memory_rw_debug(s-g_cpu, addr, mem_buf, len, 0) != 0) {
 +        if (TARGET_CPU_MEMORY_RW_DEBUG(s-g_cpu, addr, mem_buf, len, 0) != 
 0) {

 cpu_memory_rw_debug() could remain unwrapped with a generic function
 like cpu_gdb_sync_memory() which gdbstub should explicitly call.

 Maybe the lazy condition codes etc. could be handled in similar way,
 cpu_gdb_sync_registers().


 Excuse me, I don't understand here.

cpu_gdb_{read,write}_register needs to force calculation of lazy
condition codes. On Sparc this is handled by cpu_get_psr(), so it is
not explicit.

             put_packet (s, E14);
         } else {
             memtohex(buf, mem_buf, len);
 @@ -2028,10 +2031,11 @@ static int gdb_handle_packet(GDBState *s, const 
 char *line_buf)
         if (*p == ':')
             p++;
         hextomem(mem_buf, p, len);
 -        if (cpu_memory_rw_debug(s-g_cpu, addr, mem_buf, len, 1) != 0)
 +        if (TARGET_CPU_MEMORY_RW_DEBUG(s-g_cpu, addr, mem_buf, len, 1) != 
 0) {
             put_packet(s, E14);
 -        else
 +        } else {
             put_packet(s, OK);
 +        }
         break;
     case 'p':
         /* Older gdb are really dumb, and don't use 'g' if 'p' is 
 avaialable.
 diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h
 index 8654f26..3f76eaf 100644
 --- a/target-sparc/cpu.h
 +++ b/target-sparc/cpu.h
 @@ -495,6 +495,13 @@ int cpu_sparc_handle_mmu_fault(CPUSPARCState *env1, 
 target_ulong address, int rw
  target_ulong mmu_probe(CPUSPARCState *env, target_ulong address, int 
 mmulev);
  void dump_mmu(FILE *f, fprintf_function cpu_fprintf, CPUState *env);

 +#if !defined(TARGET_SPARC64)
 +int sparc_cpu_memory_rw_debug(CPUState *env, target_ulong addr,
 +                              uint8_t *buf, int len, int is_write);
 +#define TARGET_CPU_MEMORY_RW_DEBUG sparc_cpu_memory_rw_debug
 +#endif
 +
 +
  /* translate.c */
  void gen_intermediate_code_init(CPUSPARCState *env);

 diff --git a/target-sparc/helper.c b/target-sparc/helper.c
 index 1fe1f07..2cf4e8b 100644
 --- a/target-sparc/helper.c
 +++ b/target-sparc/helper.c
 @@ -358,6 +358,91 @@ void dump_mmu(FILE *f, fprintf_function cpu_fprintf, 
 CPUState *env)
     }
  }

 +
 +/* Gdb expects all registers windows to be flushed in ram. This function 
 handles
 + * reads/writes in stack frames as if windows were flushed. We assume that 
 the
 + * sparc ABI is followed.
 + */

 We can't assume that, it depends on what we are executing (BIOS, OS,
 even application).

 Well, maybe the statement is too strong. The ABI is required to get a valid
 result. Gdb cannot build back-traces if the ABI is not followed anyway.

But if the ABI assumption happens to be wrong (for example registers
contain random values), memory may be corrupted because this would
happily use whatever the registers contain.

Another way to fix this would be that GDB would tell QEMU what ABI to
use for flushing. But how would one tell GDB about a non-standard ABI?

For user emulators we can make ABI assumptions, there similar patch
could make sense. But system emulators can't assume anything about the
guest OS, it could be Linux, *BSD, a commercial OS or even a toy OS.

 On Sparc64 there are two ABIs (32 bit and 64 bit
 with offset of -2047), though calling flushw instruction could handle
 that.

 This solution is for SPARC32 only.

 If the flush happens to trigger a fault, we're in big trouble.


 That's why it's safer/easier to use this hackish read/write in the 
 registers.

No, if the fault happens here, handling it may be tricky. See for
example what paranoia Linux has to do for user window flushing, it
involves the no-fault mode in MMU.

 

Re: [Qemu-devel] [PATCH] [SPARC] Gdbstub: Fix back-trace on SPARC32

2011-09-03 Thread Blue Swirl
On Thu, Sep 1, 2011 at 2:17 PM, Fabien Chouteau chout...@adacore.com wrote:
 Gdb expects all registers windows to be flushed in ram, which is not the case
 in Qemu. Therefore the back-trace generation doesn't work. This patch adds a
 function to handle reads/writes in stack frames as if windows were flushed.

 Signed-off-by: Fabien Chouteau chout...@adacore.com
 ---
  gdbstub.c             |   10 --
  target-sparc/cpu.h    |    7 
  target-sparc/helper.c |   85 
 +
  3 files changed, 99 insertions(+), 3 deletions(-)

 diff --git a/gdbstub.c b/gdbstub.c
 index 3b87c27..85d5ad7 100644
 --- a/gdbstub.c
 +++ b/gdbstub.c
 @@ -41,6 +41,9 @@
  #include qemu_socket.h
  #include kvm.h

 +#ifndef TARGET_CPU_MEMORY_RW_DEBUG
 +#define TARGET_CPU_MEMORY_RW_DEBUG cpu_memory_rw_debug

These days, inline functions are preferred over macros.

 +#endif

  enum {
     GDB_SIGNAL_0 = 0,
 @@ -2013,7 +2016,7 @@ static int gdb_handle_packet(GDBState *s, const char 
 *line_buf)
         if (*p == ',')
             p++;
         len = strtoull(p, NULL, 16);
 -        if (cpu_memory_rw_debug(s-g_cpu, addr, mem_buf, len, 0) != 0) {
 +        if (TARGET_CPU_MEMORY_RW_DEBUG(s-g_cpu, addr, mem_buf, len, 0) != 
 0) {

cpu_memory_rw_debug() could remain unwrapped with a generic function
like cpu_gdb_sync_memory() which gdbstub should explicitly call.

Maybe the lazy condition codes etc. could be handled in similar way,
cpu_gdb_sync_registers().

             put_packet (s, E14);
         } else {
             memtohex(buf, mem_buf, len);
 @@ -2028,10 +2031,11 @@ static int gdb_handle_packet(GDBState *s, const char 
 *line_buf)
         if (*p == ':')
             p++;
         hextomem(mem_buf, p, len);
 -        if (cpu_memory_rw_debug(s-g_cpu, addr, mem_buf, len, 1) != 0)
 +        if (TARGET_CPU_MEMORY_RW_DEBUG(s-g_cpu, addr, mem_buf, len, 1) != 
 0) {
             put_packet(s, E14);
 -        else
 +        } else {
             put_packet(s, OK);
 +        }
         break;
     case 'p':
         /* Older gdb are really dumb, and don't use 'g' if 'p' is avaialable.
 diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h
 index 8654f26..3f76eaf 100644
 --- a/target-sparc/cpu.h
 +++ b/target-sparc/cpu.h
 @@ -495,6 +495,13 @@ int cpu_sparc_handle_mmu_fault(CPUSPARCState *env1, 
 target_ulong address, int rw
  target_ulong mmu_probe(CPUSPARCState *env, target_ulong address, int mmulev);
  void dump_mmu(FILE *f, fprintf_function cpu_fprintf, CPUState *env);

 +#if !defined(TARGET_SPARC64)
 +int sparc_cpu_memory_rw_debug(CPUState *env, target_ulong addr,
 +                              uint8_t *buf, int len, int is_write);
 +#define TARGET_CPU_MEMORY_RW_DEBUG sparc_cpu_memory_rw_debug
 +#endif
 +
 +
  /* translate.c */
  void gen_intermediate_code_init(CPUSPARCState *env);

 diff --git a/target-sparc/helper.c b/target-sparc/helper.c
 index 1fe1f07..2cf4e8b 100644
 --- a/target-sparc/helper.c
 +++ b/target-sparc/helper.c
 @@ -358,6 +358,91 @@ void dump_mmu(FILE *f, fprintf_function cpu_fprintf, 
 CPUState *env)
     }
  }

 +
 +/* Gdb expects all registers windows to be flushed in ram. This function 
 handles
 + * reads/writes in stack frames as if windows were flushed. We assume that 
 the
 + * sparc ABI is followed.
 + */

We can't assume that, it depends on what we are executing (BIOS, OS,
even application). On Sparc64 there are two ABIs (32 bit and 64 bit
with offset of -2047), though calling flushw instruction could handle
that. If the flush happens to trigger a fault, we're in big trouble.

Overall, I think this is too hackish. Maybe this is a bug in GDB
instead, information from backtrace is not reliable if ABI is not
known.

 +int sparc_cpu_memory_rw_debug(CPUState *env, target_ulong addr,
 +                              uint8_t *buf, int len, int is_write)
 +{
 +    int i;
 +    int len1;
 +    int cwp = env-cwp;
 +
 +    for (i = 0; i  env-nwindows; i++) {
 +        int off;
 +        target_ulong fp = env-regbase[cwp * 16 + 22];
 +
 +        /* Assume fp == 0 means end of frame.  */
 +        if (fp == 0) {
 +            break;
 +        }
 +
 +        cwp = cpu_cwp_inc(env, cwp + 1);
 +
 +        /* Invalid window ? */
 +        if (env-wim  (1  cwp)) {
 +            break;
 +        }
 +
 +        /* According to the ABI, the stack is growing downward.  */
 +        if (addr + len  fp) {
 +            break;
 +        }
 +
 +        /* Not in this frame.  */
 +        if (addr  fp + 64) {
 +            continue;
 +        }
 +
 +        /* Handle access before this window.  */
 +        if (addr  fp) {
 +            len1 = fp - addr;
 +            if (cpu_memory_rw_debug(env, addr, buf, len1, is_write) != 0) {
 +                return -1;
 +            }
 +            addr += len1;
 +            len -= len1;
 +            buf += len1;
 +        }
 +
 +        /* Access byte per byte to registers. Not very efficient but speed is
 +         * not critical.
 +         */
 + 

[Qemu-devel] [PATCH] [SPARC] Gdbstub: Fix back-trace on SPARC32

2011-09-01 Thread Fabien Chouteau
Gdb expects all registers windows to be flushed in ram, which is not the case
in Qemu. Therefore the back-trace generation doesn't work. This patch adds a
function to handle reads/writes in stack frames as if windows were flushed.

Signed-off-by: Fabien Chouteau chout...@adacore.com
---
 gdbstub.c |   10 --
 target-sparc/cpu.h|7 
 target-sparc/helper.c |   85 +
 3 files changed, 99 insertions(+), 3 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 3b87c27..85d5ad7 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -41,6 +41,9 @@
 #include qemu_socket.h
 #include kvm.h
 
+#ifndef TARGET_CPU_MEMORY_RW_DEBUG
+#define TARGET_CPU_MEMORY_RW_DEBUG cpu_memory_rw_debug
+#endif
 
 enum {
 GDB_SIGNAL_0 = 0,
@@ -2013,7 +2016,7 @@ static int gdb_handle_packet(GDBState *s, const char 
*line_buf)
 if (*p == ',')
 p++;
 len = strtoull(p, NULL, 16);
-if (cpu_memory_rw_debug(s-g_cpu, addr, mem_buf, len, 0) != 0) {
+if (TARGET_CPU_MEMORY_RW_DEBUG(s-g_cpu, addr, mem_buf, len, 0) != 0) {
 put_packet (s, E14);
 } else {
 memtohex(buf, mem_buf, len);
@@ -2028,10 +2031,11 @@ static int gdb_handle_packet(GDBState *s, const char 
*line_buf)
 if (*p == ':')
 p++;
 hextomem(mem_buf, p, len);
-if (cpu_memory_rw_debug(s-g_cpu, addr, mem_buf, len, 1) != 0)
+if (TARGET_CPU_MEMORY_RW_DEBUG(s-g_cpu, addr, mem_buf, len, 1) != 0) {
 put_packet(s, E14);
-else
+} else {
 put_packet(s, OK);
+}
 break;
 case 'p':
 /* Older gdb are really dumb, and don't use 'g' if 'p' is avaialable.
diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h
index 8654f26..3f76eaf 100644
--- a/target-sparc/cpu.h
+++ b/target-sparc/cpu.h
@@ -495,6 +495,13 @@ int cpu_sparc_handle_mmu_fault(CPUSPARCState *env1, 
target_ulong address, int rw
 target_ulong mmu_probe(CPUSPARCState *env, target_ulong address, int mmulev);
 void dump_mmu(FILE *f, fprintf_function cpu_fprintf, CPUState *env);
 
+#if !defined(TARGET_SPARC64)
+int sparc_cpu_memory_rw_debug(CPUState *env, target_ulong addr,
+  uint8_t *buf, int len, int is_write);
+#define TARGET_CPU_MEMORY_RW_DEBUG sparc_cpu_memory_rw_debug
+#endif
+
+
 /* translate.c */
 void gen_intermediate_code_init(CPUSPARCState *env);
 
diff --git a/target-sparc/helper.c b/target-sparc/helper.c
index 1fe1f07..2cf4e8b 100644
--- a/target-sparc/helper.c
+++ b/target-sparc/helper.c
@@ -358,6 +358,91 @@ void dump_mmu(FILE *f, fprintf_function cpu_fprintf, 
CPUState *env)
 }
 }
 
+
+/* Gdb expects all registers windows to be flushed in ram. This function 
handles
+ * reads/writes in stack frames as if windows were flushed. We assume that the
+ * sparc ABI is followed.
+ */
+int sparc_cpu_memory_rw_debug(CPUState *env, target_ulong addr,
+  uint8_t *buf, int len, int is_write)
+{
+int i;
+int len1;
+int cwp = env-cwp;
+
+for (i = 0; i  env-nwindows; i++) {
+int off;
+target_ulong fp = env-regbase[cwp * 16 + 22];
+
+/* Assume fp == 0 means end of frame.  */
+if (fp == 0) {
+break;
+}
+
+cwp = cpu_cwp_inc(env, cwp + 1);
+
+/* Invalid window ? */
+if (env-wim  (1  cwp)) {
+break;
+}
+
+/* According to the ABI, the stack is growing downward.  */
+if (addr + len  fp) {
+break;
+}
+
+/* Not in this frame.  */
+if (addr  fp + 64) {
+continue;
+}
+
+/* Handle access before this window.  */
+if (addr  fp) {
+len1 = fp - addr;
+if (cpu_memory_rw_debug(env, addr, buf, len1, is_write) != 0) {
+return -1;
+}
+addr += len1;
+len -= len1;
+buf += len1;
+}
+
+/* Access byte per byte to registers. Not very efficient but speed is
+ * not critical.
+ */
+off = addr - fp;
+len1 = 64 - off;
+
+if (len1  len) {
+len1 = len;
+}
+
+for (; len1; len1--) {
+int reg = cwp * 16 + 8 + (off  2);
+union {
+uint32_t v;
+uint8_t c[4];
+} u;
+u.v = cpu_to_be32(env-regbase[reg]);
+if (is_write) {
+u.c[off  3] = *buf++;
+env-regbase[reg] = be32_to_cpu(u.v);
+} else {
+*buf++ = u.c[off  3];
+}
+addr++;
+len--;
+off++;
+}
+
+if (len == 0) {
+return 0;
+}
+}
+return cpu_memory_rw_debug(env, addr, buf, len, is_write);
+}
+
+
 #else /* !TARGET_SPARC64 */
 
 // 41 bit physical address space
-- 
1.7.4.1