[Qemu-devel] [PATCH RFT qom-cpu for-next] gdbstub: Fix gdb_register_coprocessor() register counting

[Qemu-devel] [PATCH RFT qom-cpu for-next] gdbstub: Fix gdb_register_coprocessor() register counting

[Andreas Färber]

Commit a0e372f0c49ac01faeaeb73a6e8f50e8ac615f34 reorganized the register
counting for GDB. While it seems correct not to let the total number of
registers skyrocket in an SMP scenario through a static variable, the
distinction between total register count and 'g' packet register count
(last_reg vs. num_g_regs) got lost among the way.

Fix this by introducing CPUState::gdb_num_g_regs and using that in
gdb_handle_packet().

Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Cc: qemu-stable@nongnu.org (for 1.6)
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 gdbstub.c         | 6 ++++--
 include/qom/cpu.h | 2 ++
 qom/cpu.c         | 2 +-
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 1af25a6..9d067d6 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -621,6 +621,8 @@ void gdb_register_coprocessor(CPUState *cpu,
         if (g_pos != s->base_reg) {
             fprintf(stderr, "Error: Bad gdb register numbering for '%s'\n"
                     "Expected %d got %d\n", xml, g_pos, s->base_reg);
+        } else {
+            cpu->gdb_num_g_regs = cpu->gdb_num_regs;
         }
     }
 }
@@ -902,7 +904,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
     case 'g':
         cpu_synchronize_state(s->g_cpu);
         len = 0;
-        for (addr = 0; addr < s->g_cpu->gdb_num_regs; addr++) {
+        for (addr = 0; addr < s->g_cpu->gdb_num_g_regs; addr++) {
             reg_size = gdb_read_register(s->g_cpu, mem_buf + len, addr);
             len += reg_size;
         }
@@ -914,7 +916,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         registers = mem_buf;
         len = strlen(p) / 2;
         hextomem((uint8_t *)registers, p, len);
-        for (addr = 0; addr < s->g_cpu->gdb_num_regs && len > 0; addr++) {
+        for (addr = 0; addr < s->g_cpu->gdb_num_g_regs && len > 0; addr++) {
             reg_size = gdb_write_register(s->g_cpu, registers, addr);
             len -= reg_size;
             registers += reg_size;
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 0d6e95c..3e49936 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -152,6 +152,7 @@ struct kvm_run;
  * @current_tb: Currently executing TB.
  * @gdb_regs: Additional GDB registers.
  * @gdb_num_regs: Number of total registers accessible to GDB.
+ * @gdb_num_g_regs: Number of registers in GDB 'g' packets.
  * @next_cpu: Next CPU sharing TB cache.
  * @kvm_fd: vCPU file descriptor for KVM.
  *
@@ -188,6 +189,7 @@ struct CPUState {
     struct TranslationBlock *current_tb;
     struct GDBRegisterState *gdb_regs;
     int gdb_num_regs;
+    int gdb_num_g_regs;
     CPUState *next_cpu;
 
     int kvm_fd;
diff --git a/qom/cpu.c b/qom/cpu.c
index aa95108..e71e57b 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -240,7 +240,7 @@ static void cpu_common_initfn(Object *obj)
     CPUState *cpu = CPU(obj);
     CPUClass *cc = CPU_GET_CLASS(obj);
 
-    cpu->gdb_num_regs = cc->gdb_num_core_regs;
+    cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
 }
 
 static int64_t cpu_common_get_arch_id(CPUState *cpu)
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH RFT qom-cpu for-next] gdbstub: Fix gdb_register_coprocessor() register counting

[Aneesh Kumar K.V]

Andreas Färber <afaerber@suse.de> writes:

> Commit a0e372f0c49ac01faeaeb73a6e8f50e8ac615f34 reorganized the register
> counting for GDB. While it seems correct not to let the total number of
> registers skyrocket in an SMP scenario through a static variable, the
> distinction between total register count and 'g' packet register count
> (last_reg vs. num_g_regs) got lost among the way.
>
> Fix this by introducing CPUState::gdb_num_g_regs and using that in
> gdb_handle_packet().
>
> Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> Cc: qemu-stable@nongnu.org (for 1.6)
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>

Tested-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

> ---
>  gdbstub.c         | 6 ++++--
>  include/qom/cpu.h | 2 ++
>  qom/cpu.c         | 2 +-
>  3 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 1af25a6..9d067d6 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -621,6 +621,8 @@ void gdb_register_coprocessor(CPUState *cpu,
>          if (g_pos != s->base_reg) {
>              fprintf(stderr, "Error: Bad gdb register numbering for '%s'\n"
>                      "Expected %d got %d\n", xml, g_pos, s->base_reg);
> +        } else {
> +            cpu->gdb_num_g_regs = cpu->gdb_num_regs;
>          }
>      }
>  }
> @@ -902,7 +904,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>      case 'g':
>          cpu_synchronize_state(s->g_cpu);
>          len = 0;
> -        for (addr = 0; addr < s->g_cpu->gdb_num_regs; addr++) {
> +        for (addr = 0; addr < s->g_cpu->gdb_num_g_regs; addr++) {
>              reg_size = gdb_read_register(s->g_cpu, mem_buf + len, addr);
>              len += reg_size;
>          }
> @@ -914,7 +916,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          registers = mem_buf;
>          len = strlen(p) / 2;
>          hextomem((uint8_t *)registers, p, len);
> -        for (addr = 0; addr < s->g_cpu->gdb_num_regs && len > 0; addr++) {
> +        for (addr = 0; addr < s->g_cpu->gdb_num_g_regs && len > 0; addr++) {
>              reg_size = gdb_write_register(s->g_cpu, registers, addr);
>              len -= reg_size;
>              registers += reg_size;
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 0d6e95c..3e49936 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -152,6 +152,7 @@ struct kvm_run;
>   * @current_tb: Currently executing TB.
>   * @gdb_regs: Additional GDB registers.
>   * @gdb_num_regs: Number of total registers accessible to GDB.
> + * @gdb_num_g_regs: Number of registers in GDB 'g' packets.
>   * @next_cpu: Next CPU sharing TB cache.
>   * @kvm_fd: vCPU file descriptor for KVM.
>   *
> @@ -188,6 +189,7 @@ struct CPUState {
>      struct TranslationBlock *current_tb;
>      struct GDBRegisterState *gdb_regs;
>      int gdb_num_regs;
> +    int gdb_num_g_regs;
>      CPUState *next_cpu;
>
>      int kvm_fd;
> diff --git a/qom/cpu.c b/qom/cpu.c
> index aa95108..e71e57b 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -240,7 +240,7 @@ static void cpu_common_initfn(Object *obj)
>      CPUState *cpu = CPU(obj);
>      CPUClass *cc = CPU_GET_CLASS(obj);
>
> -    cpu->gdb_num_regs = cc->gdb_num_core_regs;
> +    cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
>  }
>
>  static int64_t cpu_common_get_arch_id(CPUState *cpu)
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH RFT qom-cpu for-next] gdbstub: Fix gdb_register_coprocessor() register counting

[Andreas Färber]

Am 12.08.2013 19:22, schrieb Andreas Färber:
> Commit a0e372f0c49ac01faeaeb73a6e8f50e8ac615f34 reorganized the register
> counting for GDB. While it seems correct not to let the total number of
> registers skyrocket in an SMP scenario through a static variable, the
> distinction between total register count and 'g' packet register count
> (last_reg vs. num_g_regs) got lost among the way.
> 
> Fix this by introducing CPUState::gdb_num_g_regs and using that in
> gdb_handle_packet().
> 
> Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> Cc: qemu-stable@nongnu.org (for 1.6)
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  gdbstub.c         | 6 ++++--
>  include/qom/cpu.h | 2 ++
>  qom/cpu.c         | 2 +-
>  3 files changed, 7 insertions(+), 3 deletions(-)

Ping!

Peter, could you verify the ARM FP registers are now as expected?

Max, could you test xtensa as the special case?

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH RFT qom-cpu for-next] gdbstub: Fix gdb_register_coprocessor() register counting

[Max Filippov]

On Fri, Aug 16, 2013 at 2:59 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 12.08.2013 19:22, schrieb Andreas Färber:
>> Commit a0e372f0c49ac01faeaeb73a6e8f50e8ac615f34 reorganized the register
>> counting for GDB. While it seems correct not to let the total number of
>> registers skyrocket in an SMP scenario through a static variable, the
>> distinction between total register count and 'g' packet register count
>> (last_reg vs. num_g_regs) got lost among the way.
>>
>> Fix this by introducing CPUState::gdb_num_g_regs and using that in
>> gdb_handle_packet().
>>
>> Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> Cc: qemu-stable@nongnu.org (for 1.6)
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Max Filippov <jcmvbkbc@gmail.com>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  gdbstub.c         | 6 ++++--
>>  include/qom/cpu.h | 2 ++
>>  qom/cpu.c         | 2 +-
>>  3 files changed, 7 insertions(+), 3 deletions(-)
>
> Ping!
>
> Peter, could you verify the ARM FP registers are now as expected?
>
> Max, could you test xtensa as the special case?

Tested, appears to be good.

-- 
Thanks.
-- Max

Re: [Qemu-devel] [PATCH RFT qom-cpu for-next] gdbstub: Fix gdb_register_coprocessor() register counting

[Peter Maydell]

On 16 August 2013 11:59, Andreas Färber <afaerber@suse.de> wrote:
> Peter, could you verify the ARM FP registers are now as expected?

Yes, we seem to be back to sending them separately rather than
in the 'g' packet with the integer registers.

-- PMM