[Qemu-devel] [PATCH v2 0/4] some gdbstub fixes for debug and vcont

[Qemu-devel] [PATCH v2 0/4] some gdbstub fixes for debug and vcont

[Alex Bennée]

Hi,

This is a follow up v2 of the patches I posted yesterday. There are
two new patches. The first is a simple name change to make the purpose
of the function clearer. The second new patch now fixes the bug by
making the thread-id consistent when reporting it via the gdbstub. It
also reduces the size of CPUState by avoiding duplication of
information already available in TaskStruct. The original fix is still
in the series although its theoretical (but still correct) for my test
case which now uses the correct thread-id.

Alex Bennée (4):
  gdbstub: modernise DEBUG_GDB
  gdbstub: rename cpu_index -> cpu_gdb_index
  qom/cpu: remove host_tid field
  gdbstub: don't fail on vCont;C04:0;c packets

 gdbstub.c              | 117 ++++++++++++++++++++++++-------------------------
 include/exec/gdbstub.h |   9 ----
 include/qom/cpu.h      |   2 -
 linux-user/syscall.c   |   1 -
 4 files changed, 57 insertions(+), 72 deletions(-)

-- 
2.13.0

[Qemu-devel] [PATCH v2 1/4] gdbstub: modernise DEBUG_GDB

[Alex Bennée]

Convert the a gdb_debug helper which compiles away to nothing when not
used but still ensures the format strings are checked. There is some
minor code motion for the incorrect checksum message to report it
before we attempt to send the reply.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
 gdbstub.c | 77 +++++++++++++++++++++++++++------------------------------------
 1 file changed, 33 insertions(+), 44 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 86eed4f97c..a249846954 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -271,7 +271,20 @@ static int gdb_signal_to_target (int sig)
         return -1;
 }
 
-//#define DEBUG_GDB
+/* #define DEBUG_GDB */
+
+#ifdef DEBUG_GDB
+# define DEBUG_GDB_GATE 1
+#else
+# define DEBUG_GDB_GATE 0
+#endif
+
+#define gdb_debug(fmt, ...) do { \
+    if (DEBUG_GDB_GATE) { \
+        fprintf(stderr, "%s: " fmt, __func__, ## __VA_ARGS__); \
+    } \
+} while (0)
+
 
 typedef struct GDBRegisterState {
     int base_reg;
@@ -547,9 +560,7 @@ static int put_packet_binary(GDBState *s, const char *buf, int len)
 /* return -1 if error, 0 if OK */
 static int put_packet(GDBState *s, const char *buf)
 {
-#ifdef DEBUG_GDB
-    printf("reply='%s'\n", buf);
-#endif
+    gdb_debug("reply='%s'\n", buf);
 
     return put_packet_binary(s, buf, strlen(buf));
 }
@@ -955,9 +966,9 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
     uint8_t *registers;
     target_ulong addr, len;
 
-#ifdef DEBUG_GDB
-    printf("command='%s'\n", line_buf);
-#endif
+
+    gdb_debug("command='%s'\n", line_buf);
+
     p = line_buf;
     ch = *p++;
     switch(ch) {
@@ -1518,17 +1529,14 @@ static void gdb_read_byte(GDBState *s, int ch)
         /* Waiting for a response to the last packet.  If we see the start
            of a new command then abandon the previous response.  */
         if (ch == '-') {
-#ifdef DEBUG_GDB
-            printf("Got NACK, retransmitting\n");
-#endif
+            gdb_debug("Got NACK, retransmitting\n");
             put_buffer(s, (uint8_t *)s->last_packet, s->last_packet_len);
+        } else if (ch == '+') {
+            gdb_debug("Got ACK\n");
+        } else {
+            gdb_debug("Got '%c' when expecting ACK/NACK\n", ch);
         }
-#ifdef DEBUG_GDB
-        else if (ch == '+')
-            printf("Got ACK\n");
-        else
-            printf("Got '%c' when expecting ACK/NACK\n", ch);
-#endif
+
         if (ch == '+' || ch == '$')
             s->last_packet_len = 0;
         if (ch != '$')
@@ -1549,9 +1557,7 @@ static void gdb_read_byte(GDBState *s, int ch)
                 s->line_sum = 0;
                 s->state = RS_GETLINE;
             } else {
-#ifdef DEBUG_GDB
-                printf("gdbstub received garbage between packets: 0x%x\n", ch);
-#endif
+                gdb_debug("received garbage between packets: 0x%x\n", ch);
             }
             break;
         case RS_GETLINE:
@@ -1567,9 +1573,7 @@ static void gdb_read_byte(GDBState *s, int ch)
                 /* end of command, start of checksum*/
                 s->state = RS_CHKSUM1;
             } else if (s->line_buf_index >= sizeof(s->line_buf) - 1) {
-#ifdef DEBUG_GDB
-                printf("gdbstub command buffer overrun, dropping command\n");
-#endif
+                gdb_debug("command buffer overrun, dropping command\n");
                 s->state = RS_IDLE;
             } else {
                 /* unescaped command character */
@@ -1583,9 +1587,7 @@ static void gdb_read_byte(GDBState *s, int ch)
                 s->state = RS_CHKSUM1;
             } else if (s->line_buf_index >= sizeof(s->line_buf) - 1) {
                 /* command buffer overrun */
-#ifdef DEBUG_GDB
-                printf("gdbstub command buffer overrun, dropping command\n");
-#endif
+                gdb_debug("command buffer overrun, dropping command\n");
                 s->state = RS_IDLE;
             } else {
                 /* parse escaped character and leave escape state */
@@ -1597,25 +1599,18 @@ static void gdb_read_byte(GDBState *s, int ch)
         case RS_GETLINE_RLE:
             if (ch < ' ') {
                 /* invalid RLE count encoding */
-#ifdef DEBUG_GDB
-                printf("gdbstub got invalid RLE count: 0x%x\n", ch);
-#endif
+                gdb_debug("got invalid RLE count: 0x%x\n", ch);
                 s->state = RS_GETLINE;
             } else {
                 /* decode repeat length */
                 int repeat = (unsigned char)ch - ' ' + 3;
                 if (s->line_buf_index + repeat >= sizeof(s->line_buf) - 1) {
                     /* that many repeats would overrun the command buffer */
-#ifdef DEBUG_GDB
-                    printf("gdbstub command buffer overrun,"
-                           " dropping command\n");
-#endif
+                    gdb_debug("command buffer overrun, dropping command\n");
                     s->state = RS_IDLE;
                 } else if (s->line_buf_index < 1) {
                     /* got a repeat but we have nothing to repeat */
-#ifdef DEBUG_GDB
-                    printf("gdbstub got invalid RLE sequence\n");
-#endif
+                    gdb_debug("got invalid RLE sequence\n");
                     s->state = RS_GETLINE;
                 } else {
                     /* repeat the last character */
@@ -1630,9 +1625,7 @@ static void gdb_read_byte(GDBState *s, int ch)
         case RS_CHKSUM1:
             /* get high hex digit of checksum */
             if (!isxdigit(ch)) {
-#ifdef DEBUG_GDB
-                printf("gdbstub got invalid command checksum digit\n");
-#endif
+                gdb_debug("got invalid command checksum digit\n");
                 s->state = RS_GETLINE;
                 break;
             }
@@ -1643,21 +1636,17 @@ static void gdb_read_byte(GDBState *s, int ch)
         case RS_CHKSUM2:
             /* get low hex digit of checksum */
             if (!isxdigit(ch)) {
-#ifdef DEBUG_GDB
-                printf("gdbstub got invalid command checksum digit\n");
-#endif
+                gdb_debug("got invalid command checksum digit\n");
                 s->state = RS_GETLINE;
                 break;
             }
             s->line_csum |= fromhex(ch);
 
             if (s->line_csum != (s->line_sum & 0xff)) {
+                gdb_debug("got command packet with incorrect checksum\n");
                 /* send NAK reply */
                 reply = '-';
                 put_buffer(s, &reply, 1);
-#ifdef DEBUG_GDB
-                printf("gdbstub got command packet with incorrect checksum\n");
-#endif
                 s->state = RS_IDLE;
             } else {
                 /* send ACK reply */
-- 
2.13.0

[Qemu-devel] [PATCH v2 2/4] gdbstub: rename cpu_index -> cpu_gdb_index

[Alex Bennée]

This is to make it clear the index is purely a gdbstub function and
should not be confused with the value of cpu->cpu_index.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 gdbstub.c              | 12 ++++++------
 include/exec/gdbstub.h |  7 ++++++-
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index a249846954..026d1fe6bb 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -837,7 +837,7 @@ static CPUState *find_cpu(uint32_t thread_id)
     CPUState *cpu;
 
     CPU_FOREACH(cpu) {
-        if (cpu_index(cpu) == thread_id) {
+        if (cpu_gdb_index(cpu) == thread_id) {
             return cpu;
         }
     }
@@ -925,7 +925,7 @@ static int gdb_handle_vcont(GDBState *s, const char *p)
             idx = tmp;
             /* 0 means any thread, so we pick the first valid CPU */
             if (!idx) {
-                idx = cpu_index(first_cpu);
+                idx = cpu_gdb_index(first_cpu);
             }
 
             /*
@@ -975,7 +975,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
     case '?':
         /* TODO: Make this return the correct value for user-mode.  */
         snprintf(buf, sizeof(buf), "T%02xthread:%02x;", GDB_SIGNAL_TRAP,
-                 cpu_index(s->c_cpu));
+                 cpu_gdb_index(s->c_cpu));
         put_packet(s, buf);
         /* Remove all the breakpoints when this query is issued,
          * because gdb is doing and initial connect and the state
@@ -1243,7 +1243,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         } else if (strcmp(p,"sThreadInfo") == 0) {
         report_cpuinfo:
             if (s->query_cpu) {
-                snprintf(buf, sizeof(buf), "m%x", cpu_index(s->query_cpu));
+                snprintf(buf, sizeof(buf), "m%x", cpu_gdb_index(s->query_cpu));
                 put_packet(s, buf);
                 s->query_cpu = CPU_NEXT(s->query_cpu);
             } else
@@ -1400,7 +1400,7 @@ static void gdb_vm_state_change(void *opaque, int running, RunState state)
             }
             snprintf(buf, sizeof(buf),
                      "T%02xthread:%02x;%swatch:" TARGET_FMT_lx ";",
-                     GDB_SIGNAL_TRAP, cpu_index(cpu), type,
+                     GDB_SIGNAL_TRAP, cpu_gdb_index(cpu), type,
                      (target_ulong)cpu->watchpoint_hit->vaddr);
             cpu->watchpoint_hit = NULL;
             goto send_packet;
@@ -1434,7 +1434,7 @@ static void gdb_vm_state_change(void *opaque, int running, RunState state)
         break;
     }
     gdb_set_stop_cpu(cpu);
-    snprintf(buf, sizeof(buf), "T%02xthread:%02x;", ret, cpu_index(cpu));
+    snprintf(buf, sizeof(buf), "T%02xthread:%02x;", ret, cpu_gdb_index(cpu));
 
 send_packet:
     put_packet(s, buf);
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index f9708bbcd6..c4fe567600 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -58,7 +58,12 @@ void gdb_register_coprocessor(CPUState *cpu,
                               gdb_reg_cb get_reg, gdb_reg_cb set_reg,
                               int num_regs, const char *xml, int g_pos);
 
-static inline int cpu_index(CPUState *cpu)
+/* Return the GDB index for a given vCPU state.
+ *
+ * For user mode this is simply the thread id. In system mode GDB
+ * numbers CPUs from 1 as 0 is reserved as an "any cpu" index.
+ */
+static inline int cpu_gdb_index(CPUState *cpu)
 {
 #if defined(CONFIG_USER_ONLY)
     return cpu->host_tid;
-- 
2.13.0

Re: [Qemu-devel] [PATCH v2 2/4] gdbstub: rename cpu_index -> cpu_gdb_index

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 3865 bytes --]

On Thu,  1 Jun 2017 15:49:13 +0100
Alex Bennée <alex.bennee@linaro.org> wrote:

> This is to make it clear the index is purely a gdbstub function and
> should not be confused with the value of cpu->cpu_index.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  gdbstub.c              | 12 ++++++------
>  include/exec/gdbstub.h |  7 ++++++-
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index a249846954..026d1fe6bb 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -837,7 +837,7 @@ static CPUState *find_cpu(uint32_t thread_id)
>      CPUState *cpu;
>  
>      CPU_FOREACH(cpu) {
> -        if (cpu_index(cpu) == thread_id) {
> +        if (cpu_gdb_index(cpu) == thread_id) {
>              return cpu;
>          }
>      }
> @@ -925,7 +925,7 @@ static int gdb_handle_vcont(GDBState *s, const char *p)
>              idx = tmp;
>              /* 0 means any thread, so we pick the first valid CPU */
>              if (!idx) {
> -                idx = cpu_index(first_cpu);
> +                idx = cpu_gdb_index(first_cpu);
>              }
>  
>              /*
> @@ -975,7 +975,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>      case '?':
>          /* TODO: Make this return the correct value for user-mode.  */
>          snprintf(buf, sizeof(buf), "T%02xthread:%02x;", GDB_SIGNAL_TRAP,
> -                 cpu_index(s->c_cpu));
> +                 cpu_gdb_index(s->c_cpu));
>          put_packet(s, buf);
>          /* Remove all the breakpoints when this query is issued,
>           * because gdb is doing and initial connect and the state
> @@ -1243,7 +1243,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          } else if (strcmp(p,"sThreadInfo") == 0) {
>          report_cpuinfo:
>              if (s->query_cpu) {
> -                snprintf(buf, sizeof(buf), "m%x", cpu_index(s->query_cpu));
> +                snprintf(buf, sizeof(buf), "m%x", cpu_gdb_index(s->query_cpu));
>                  put_packet(s, buf);
>                  s->query_cpu = CPU_NEXT(s->query_cpu);
>              } else
> @@ -1400,7 +1400,7 @@ static void gdb_vm_state_change(void *opaque, int running, RunState state)
>              }
>              snprintf(buf, sizeof(buf),
>                       "T%02xthread:%02x;%swatch:" TARGET_FMT_lx ";",
> -                     GDB_SIGNAL_TRAP, cpu_index(cpu), type,
> +                     GDB_SIGNAL_TRAP, cpu_gdb_index(cpu), type,
>                       (target_ulong)cpu->watchpoint_hit->vaddr);
>              cpu->watchpoint_hit = NULL;
>              goto send_packet;
> @@ -1434,7 +1434,7 @@ static void gdb_vm_state_change(void *opaque, int running, RunState state)
>          break;
>      }
>      gdb_set_stop_cpu(cpu);
> -    snprintf(buf, sizeof(buf), "T%02xthread:%02x;", ret, cpu_index(cpu));
> +    snprintf(buf, sizeof(buf), "T%02xthread:%02x;", ret, cpu_gdb_index(cpu));
>  
>  send_packet:
>      put_packet(s, buf);
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index f9708bbcd6..c4fe567600 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -58,7 +58,12 @@ void gdb_register_coprocessor(CPUState *cpu,
>                                gdb_reg_cb get_reg, gdb_reg_cb set_reg,
>                                int num_regs, const char *xml, int g_pos);
>  
> -static inline int cpu_index(CPUState *cpu)
> +/* Return the GDB index for a given vCPU state.
> + *
> + * For user mode this is simply the thread id. In system mode GDB
> + * numbers CPUs from 1 as 0 is reserved as an "any cpu" index.
> + */
> +static inline int cpu_gdb_index(CPUState *cpu)
>  {
>  #if defined(CONFIG_USER_ONLY)
>      return cpu->host_tid;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH v2 2/4] gdbstub: rename cpu_index -> cpu_gdb_index

[Claudio Imbrenda]

On Thu,  1 Jun 2017 15:49:13 +0100
Alex Bennée <alex.bennee@linaro.org> wrote:

> This is to make it clear the index is purely a gdbstub function and
> should not be confused with the value of cpu->cpu_index.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  gdbstub.c              | 12 ++++++------
>  include/exec/gdbstub.h |  7 ++++++-
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index a249846954..026d1fe6bb 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -837,7 +837,7 @@ static CPUState *find_cpu(uint32_t thread_id)
>      CPUState *cpu;
> 
>      CPU_FOREACH(cpu) {
> -        if (cpu_index(cpu) == thread_id) {
> +        if (cpu_gdb_index(cpu) == thread_id) {
>              return cpu;
>          }
>      }
> @@ -925,7 +925,7 @@ static int gdb_handle_vcont(GDBState *s, const
> char *p) idx = tmp;
>              /* 0 means any thread, so we pick the first valid CPU */
>              if (!idx) {
> -                idx = cpu_index(first_cpu);
> +                idx = cpu_gdb_index(first_cpu);
>              }
> 
>              /*
> @@ -975,7 +975,7 @@ static int gdb_handle_packet(GDBState *s, const
> char *line_buf) case '?':
>          /* TODO: Make this return the correct value for user-mode.
> */ snprintf(buf, sizeof(buf), "T%02xthread:%02x;", GDB_SIGNAL_TRAP,
> -                 cpu_index(s->c_cpu));
> +                 cpu_gdb_index(s->c_cpu));
>          put_packet(s, buf);
>          /* Remove all the breakpoints when this query is issued,
>           * because gdb is doing and initial connect and the state
> @@ -1243,7 +1243,7 @@ static int gdb_handle_packet(GDBState *s, const
> char *line_buf) } else if (strcmp(p,"sThreadInfo") == 0) {
>          report_cpuinfo:
>              if (s->query_cpu) {
> -                snprintf(buf, sizeof(buf), "m%x",
> cpu_index(s->query_cpu));
> +                snprintf(buf, sizeof(buf), "m%x",
> cpu_gdb_index(s->query_cpu)); put_packet(s, buf);
>                  s->query_cpu = CPU_NEXT(s->query_cpu);
>              } else
> @@ -1400,7 +1400,7 @@ static void gdb_vm_state_change(void *opaque,
> int running, RunState state) }
>              snprintf(buf, sizeof(buf),
>                       "T%02xthread:%02x;%swatch:" TARGET_FMT_lx ";",
> -                     GDB_SIGNAL_TRAP, cpu_index(cpu), type,
> +                     GDB_SIGNAL_TRAP, cpu_gdb_index(cpu), type,
>                       (target_ulong)cpu->watchpoint_hit->vaddr);
>              cpu->watchpoint_hit = NULL;
>              goto send_packet;
> @@ -1434,7 +1434,7 @@ static void gdb_vm_state_change(void *opaque,
> int running, RunState state) break;
>      }
>      gdb_set_stop_cpu(cpu);
> -    snprintf(buf, sizeof(buf), "T%02xthread:%02x;", ret,
> cpu_index(cpu));
> +    snprintf(buf, sizeof(buf), "T%02xthread:%02x;", ret,
> cpu_gdb_index(cpu));
> 
>  send_packet:
>      put_packet(s, buf);
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index f9708bbcd6..c4fe567600 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -58,7 +58,12 @@ void gdb_register_coprocessor(CPUState *cpu,
>                                gdb_reg_cb get_reg, gdb_reg_cb set_reg,
>                                int num_regs, const char *xml, int
> g_pos);
> 
> -static inline int cpu_index(CPUState *cpu)
> +/* Return the GDB index for a given vCPU state.
> + *
> + * For user mode this is simply the thread id. In system mode GDB
> + * numbers CPUs from 1 as 0 is reserved as an "any cpu" index.
> + */
> +static inline int cpu_gdb_index(CPUState *cpu)
>  {
>  #if defined(CONFIG_USER_ONLY)
>      return cpu->host_tid;

I like this solution

Reviewed-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH v2 2/4] gdbstub: rename cpu_index -> cpu_gdb_index

[Philippe Mathieu-Daudé]

On 06/01/2017 11:49 AM, Alex Bennée wrote:
> This is to make it clear the index is purely a gdbstub function and
> should not be confused with the value of cpu->cpu_index.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  gdbstub.c              | 12 ++++++------
>  include/exec/gdbstub.h |  7 ++++++-
>  2 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index a249846954..026d1fe6bb 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -837,7 +837,7 @@ static CPUState *find_cpu(uint32_t thread_id)
>      CPUState *cpu;
>
>      CPU_FOREACH(cpu) {
> -        if (cpu_index(cpu) == thread_id) {
> +        if (cpu_gdb_index(cpu) == thread_id) {
>              return cpu;
>          }
>      }
> @@ -925,7 +925,7 @@ static int gdb_handle_vcont(GDBState *s, const char *p)
>              idx = tmp;
>              /* 0 means any thread, so we pick the first valid CPU */
>              if (!idx) {
> -                idx = cpu_index(first_cpu);
> +                idx = cpu_gdb_index(first_cpu);
>              }
>
>              /*
> @@ -975,7 +975,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>      case '?':
>          /* TODO: Make this return the correct value for user-mode.  */
>          snprintf(buf, sizeof(buf), "T%02xthread:%02x;", GDB_SIGNAL_TRAP,
> -                 cpu_index(s->c_cpu));
> +                 cpu_gdb_index(s->c_cpu));
>          put_packet(s, buf);
>          /* Remove all the breakpoints when this query is issued,
>           * because gdb is doing and initial connect and the state
> @@ -1243,7 +1243,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>          } else if (strcmp(p,"sThreadInfo") == 0) {
>          report_cpuinfo:
>              if (s->query_cpu) {
> -                snprintf(buf, sizeof(buf), "m%x", cpu_index(s->query_cpu));
> +                snprintf(buf, sizeof(buf), "m%x", cpu_gdb_index(s->query_cpu));
>                  put_packet(s, buf);
>                  s->query_cpu = CPU_NEXT(s->query_cpu);
>              } else
> @@ -1400,7 +1400,7 @@ static void gdb_vm_state_change(void *opaque, int running, RunState state)
>              }
>              snprintf(buf, sizeof(buf),
>                       "T%02xthread:%02x;%swatch:" TARGET_FMT_lx ";",
> -                     GDB_SIGNAL_TRAP, cpu_index(cpu), type,
> +                     GDB_SIGNAL_TRAP, cpu_gdb_index(cpu), type,
>                       (target_ulong)cpu->watchpoint_hit->vaddr);
>              cpu->watchpoint_hit = NULL;
>              goto send_packet;
> @@ -1434,7 +1434,7 @@ static void gdb_vm_state_change(void *opaque, int running, RunState state)
>          break;
>      }
>      gdb_set_stop_cpu(cpu);
> -    snprintf(buf, sizeof(buf), "T%02xthread:%02x;", ret, cpu_index(cpu));
> +    snprintf(buf, sizeof(buf), "T%02xthread:%02x;", ret, cpu_gdb_index(cpu));
>
>  send_packet:
>      put_packet(s, buf);
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index f9708bbcd6..c4fe567600 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -58,7 +58,12 @@ void gdb_register_coprocessor(CPUState *cpu,
>                                gdb_reg_cb get_reg, gdb_reg_cb set_reg,
>                                int num_regs, const char *xml, int g_pos);
>
> -static inline int cpu_index(CPUState *cpu)
> +/* Return the GDB index for a given vCPU state.
> + *
> + * For user mode this is simply the thread id. In system mode GDB
> + * numbers CPUs from 1 as 0 is reserved as an "any cpu" index.
> + */
> +static inline int cpu_gdb_index(CPUState *cpu)
>  {
>  #if defined(CONFIG_USER_ONLY)
>      return cpu->host_tid;
>

[Qemu-devel] [PATCH v2 3/4] qom/cpu: remove host_tid field

[Alex Bennée]

This was only used by the gdbstub and even then was only being set for
subsequent threads. Rather the continue duplicating the number just
make the gdbstub get the information from TaskState structure.

Now the tid is correctly reported for all threads the bug I was seeing
with "vCont;C04:0;c" packets is fixed as the correct tid is reported
to gdb.

I moved cpu_gdb_index into the gdbstub to facilitate easy access to
the TaskState which is used elsewhere in gdbstub.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 gdbstub.c              | 15 +++++++++++++++
 include/exec/gdbstub.h | 14 --------------
 include/qom/cpu.h      |  2 --
 linux-user/syscall.c   |  1 -
 4 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 026d1fe6bb..45a3a0b16b 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -55,6 +55,21 @@ static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr,
     return cpu_memory_rw_debug(cpu, addr, buf, len, is_write);
 }
 
+/* Return the GDB index for a given vCPU state.
+ *
+ * For user mode this is simply the thread id. In system mode GDB
+ * numbers CPUs from 1 as 0 is reserved as an "any cpu" index.
+ */
+static inline int cpu_gdb_index(CPUState *cpu)
+{
+#if defined(CONFIG_USER_ONLY)
+    TaskState *ts = (TaskState *) cpu->opaque;
+    return ts->ts_tid;
+#else
+    return cpu->cpu_index + 1;
+#endif
+}
+
 enum {
     GDB_SIGNAL_0 = 0,
     GDB_SIGNAL_INT = 2,
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index c4fe567600..9aa7756d92 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -58,20 +58,6 @@ void gdb_register_coprocessor(CPUState *cpu,
                               gdb_reg_cb get_reg, gdb_reg_cb set_reg,
                               int num_regs, const char *xml, int g_pos);
 
-/* Return the GDB index for a given vCPU state.
- *
- * For user mode this is simply the thread id. In system mode GDB
- * numbers CPUs from 1 as 0 is reserved as an "any cpu" index.
- */
-static inline int cpu_gdb_index(CPUState *cpu)
-{
-#if defined(CONFIG_USER_ONLY)
-    return cpu->host_tid;
-#else
-    return cpu->cpu_index + 1;
-#endif
-}
-
 /* The GDB remote protocol transfers values in target byte order.  This means
  * we can use the raw memory access routines to access the value buffer.
  * Conveniently, these also handle the case where the buffer is mis-aligned.
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 55214ce131..909e7ae994 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -266,7 +266,6 @@ struct qemu_work_item;
  * @nr_cores: Number of cores within this CPU package.
  * @nr_threads: Number of threads within this CPU.
  * @numa_node: NUMA node this CPU is belonging to.
- * @host_tid: Host thread ID.
  * @running: #true if CPU is currently running (lockless).
  * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;
  * valid under cpu_list_lock.
@@ -321,7 +320,6 @@ struct CPUState {
     HANDLE hThread;
 #endif
     int thread_id;
-    uint32_t host_tid;
     bool running, has_waiter;
     struct QemuCond *halt_cond;
     bool thread_kicked;
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index cec8428589..cada188e58 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -6216,7 +6216,6 @@ static void *clone_func(void *arg)
     thread_cpu = cpu;
     ts = (TaskState *)cpu->opaque;
     info->tid = gettid();
-    cpu->host_tid = info->tid;
     task_settid(ts);
     if (info->child_tidptr)
         put_user_u32(info->tid, info->child_tidptr);
-- 
2.13.0

Re: [Qemu-devel] [PATCH v2 3/4] qom/cpu: remove host_tid field

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 4218 bytes --]

On Thu,  1 Jun 2017 15:49:14 +0100
Alex Bennée <alex.bennee@linaro.org> wrote:

> This was only used by the gdbstub and even then was only being set for
> subsequent threads. Rather the continue duplicating the number just
> make the gdbstub get the information from TaskState structure.
> 
> Now the tid is correctly reported for all threads the bug I was seeing
> with "vCont;C04:0;c" packets is fixed as the correct tid is reported
> to gdb.
> 
> I moved cpu_gdb_index into the gdbstub to facilitate easy access to
> the TaskState which is used elsewhere in gdbstub.
> 

FWIW, this change would make more sense in patch 2 since all users
are in gdbstub.c and it would avoid to change things twice. No big
deal compared to the benefit of dropping the broken @host_tid :)

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---

In any case.

Reviewed-by: Greg Kurz <groug@kaod.org>

>  gdbstub.c              | 15 +++++++++++++++
>  include/exec/gdbstub.h | 14 --------------
>  include/qom/cpu.h      |  2 --
>  linux-user/syscall.c   |  1 -
>  4 files changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index 026d1fe6bb..45a3a0b16b 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -55,6 +55,21 @@ static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr,
>      return cpu_memory_rw_debug(cpu, addr, buf, len, is_write);
>  }
>  
> +/* Return the GDB index for a given vCPU state.
> + *
> + * For user mode this is simply the thread id. In system mode GDB
> + * numbers CPUs from 1 as 0 is reserved as an "any cpu" index.
> + */
> +static inline int cpu_gdb_index(CPUState *cpu)
> +{
> +#if defined(CONFIG_USER_ONLY)
> +    TaskState *ts = (TaskState *) cpu->opaque;
> +    return ts->ts_tid;
> +#else
> +    return cpu->cpu_index + 1;
> +#endif
> +}
> +
>  enum {
>      GDB_SIGNAL_0 = 0,
>      GDB_SIGNAL_INT = 2,
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index c4fe567600..9aa7756d92 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -58,20 +58,6 @@ void gdb_register_coprocessor(CPUState *cpu,
>                                gdb_reg_cb get_reg, gdb_reg_cb set_reg,
>                                int num_regs, const char *xml, int g_pos);
>  
> -/* Return the GDB index for a given vCPU state.
> - *
> - * For user mode this is simply the thread id. In system mode GDB
> - * numbers CPUs from 1 as 0 is reserved as an "any cpu" index.
> - */
> -static inline int cpu_gdb_index(CPUState *cpu)
> -{
> -#if defined(CONFIG_USER_ONLY)
> -    return cpu->host_tid;
> -#else
> -    return cpu->cpu_index + 1;
> -#endif
> -}
> -
>  /* The GDB remote protocol transfers values in target byte order.  This means
>   * we can use the raw memory access routines to access the value buffer.
>   * Conveniently, these also handle the case where the buffer is mis-aligned.
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 55214ce131..909e7ae994 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -266,7 +266,6 @@ struct qemu_work_item;
>   * @nr_cores: Number of cores within this CPU package.
>   * @nr_threads: Number of threads within this CPU.
>   * @numa_node: NUMA node this CPU is belonging to.
> - * @host_tid: Host thread ID.
>   * @running: #true if CPU is currently running (lockless).
>   * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;
>   * valid under cpu_list_lock.
> @@ -321,7 +320,6 @@ struct CPUState {
>      HANDLE hThread;
>  #endif
>      int thread_id;
> -    uint32_t host_tid;
>      bool running, has_waiter;
>      struct QemuCond *halt_cond;
>      bool thread_kicked;
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index cec8428589..cada188e58 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -6216,7 +6216,6 @@ static void *clone_func(void *arg)
>      thread_cpu = cpu;
>      ts = (TaskState *)cpu->opaque;
>      info->tid = gettid();
> -    cpu->host_tid = info->tid;
>      task_settid(ts);
>      if (info->child_tidptr)
>          put_user_u32(info->tid, info->child_tidptr);


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH v2 3/4] qom/cpu: remove host_tid field

[Claudio Imbrenda]

On Thu, 1 Jun 2017 17:32:13 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Thu,  1 Jun 2017 15:49:14 +0100
> Alex Bennée <alex.bennee@linaro.org> wrote:
> 
> > This was only used by the gdbstub and even then was only being set
> > for subsequent threads. Rather the continue duplicating the number
> > just make the gdbstub get the information from TaskState structure.
> > 
> > Now the tid is correctly reported for all threads the bug I was
> > seeing with "vCont;C04:0;c" packets is fixed as the correct tid is
> > reported to gdb.
> > 
> > I moved cpu_gdb_index into the gdbstub to facilitate easy access to
> > the TaskState which is used elsewhere in gdbstub.
> >   
> 
> FWIW, this change would make more sense in patch 2 since all users
> are in gdbstub.c and it would avoid to change things twice. No big
> deal compared to the benefit of dropping the broken @host_tid :)

I agree with this

> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > ---  
> 
> In any case.
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>

and me too

Reviewed-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
 
> >  gdbstub.c              | 15 +++++++++++++++
> >  include/exec/gdbstub.h | 14 --------------
> >  include/qom/cpu.h      |  2 --
> >  linux-user/syscall.c   |  1 -
> >  4 files changed, 15 insertions(+), 17 deletions(-)
> > 
> > diff --git a/gdbstub.c b/gdbstub.c
> > index 026d1fe6bb..45a3a0b16b 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -55,6 +55,21 @@ static inline int
> > target_memory_rw_debug(CPUState *cpu, target_ulong addr, return
> > cpu_memory_rw_debug(cpu, addr, buf, len, is_write); }
> >  
> > +/* Return the GDB index for a given vCPU state.
> > + *
> > + * For user mode this is simply the thread id. In system mode GDB
> > + * numbers CPUs from 1 as 0 is reserved as an "any cpu" index.
> > + */
> > +static inline int cpu_gdb_index(CPUState *cpu)
> > +{
> > +#if defined(CONFIG_USER_ONLY)
> > +    TaskState *ts = (TaskState *) cpu->opaque;
> > +    return ts->ts_tid;
> > +#else
> > +    return cpu->cpu_index + 1;
> > +#endif
> > +}
> > +
> >  enum {
> >      GDB_SIGNAL_0 = 0,
> >      GDB_SIGNAL_INT = 2,
> > diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> > index c4fe567600..9aa7756d92 100644
> > --- a/include/exec/gdbstub.h
> > +++ b/include/exec/gdbstub.h
> > @@ -58,20 +58,6 @@ void gdb_register_coprocessor(CPUState *cpu,
> >                                gdb_reg_cb get_reg, gdb_reg_cb
> > set_reg, int num_regs, const char *xml, int g_pos);
> >  
> > -/* Return the GDB index for a given vCPU state.
> > - *
> > - * For user mode this is simply the thread id. In system mode GDB
> > - * numbers CPUs from 1 as 0 is reserved as an "any cpu" index.
> > - */
> > -static inline int cpu_gdb_index(CPUState *cpu)
> > -{
> > -#if defined(CONFIG_USER_ONLY)
> > -    return cpu->host_tid;
> > -#else
> > -    return cpu->cpu_index + 1;
> > -#endif
> > -}
> > -
> >  /* The GDB remote protocol transfers values in target byte order.
> > This means
> >   * we can use the raw memory access routines to access the value
> > buffer.
> >   * Conveniently, these also handle the case where the buffer is
> > mis-aligned. diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > index 55214ce131..909e7ae994 100644
> > --- a/include/qom/cpu.h
> > +++ b/include/qom/cpu.h
> > @@ -266,7 +266,6 @@ struct qemu_work_item;
> >   * @nr_cores: Number of cores within this CPU package.
> >   * @nr_threads: Number of threads within this CPU.
> >   * @numa_node: NUMA node this CPU is belonging to.
> > - * @host_tid: Host thread ID.
> >   * @running: #true if CPU is currently running (lockless).
> >   * @has_waiter: #true if a CPU is currently waiting for the
> > cpu_exec_end;
> >   * valid under cpu_list_lock.
> > @@ -321,7 +320,6 @@ struct CPUState {
> >      HANDLE hThread;
> >  #endif
> >      int thread_id;
> > -    uint32_t host_tid;
> >      bool running, has_waiter;
> >      struct QemuCond *halt_cond;
> >      bool thread_kicked;
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index cec8428589..cada188e58 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -6216,7 +6216,6 @@ static void *clone_func(void *arg)
> >      thread_cpu = cpu;
> >      ts = (TaskState *)cpu->opaque;
> >      info->tid = gettid();
> > -    cpu->host_tid = info->tid;
> >      task_settid(ts);
> >      if (info->child_tidptr)
> >          put_user_u32(info->tid, info->child_tidptr);  
> 

Re: [Qemu-devel] [PATCH v2 3/4] qom/cpu: remove host_tid field

[Philippe Mathieu-Daudé]

On 06/01/2017 11:49 AM, Alex Bennée wrote:
> This was only used by the gdbstub and even then was only being set for
> subsequent threads. Rather the continue duplicating the number just
> make the gdbstub get the information from TaskState structure.
>
> Now the tid is correctly reported for all threads the bug I was seeing
> with "vCont;C04:0;c" packets is fixed as the correct tid is reported
> to gdb.
>
> I moved cpu_gdb_index into the gdbstub to facilitate easy access to
> the TaskState which is used elsewhere in gdbstub.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  gdbstub.c              | 15 +++++++++++++++
>  include/exec/gdbstub.h | 14 --------------
>  include/qom/cpu.h      |  2 --
>  linux-user/syscall.c   |  1 -
>  4 files changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 026d1fe6bb..45a3a0b16b 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -55,6 +55,21 @@ static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr,
>      return cpu_memory_rw_debug(cpu, addr, buf, len, is_write);
>  }
>
> +/* Return the GDB index for a given vCPU state.
> + *
> + * For user mode this is simply the thread id. In system mode GDB
> + * numbers CPUs from 1 as 0 is reserved as an "any cpu" index.
> + */
> +static inline int cpu_gdb_index(CPUState *cpu)
> +{
> +#if defined(CONFIG_USER_ONLY)
> +    TaskState *ts = (TaskState *) cpu->opaque;
> +    return ts->ts_tid;
> +#else
> +    return cpu->cpu_index + 1;
> +#endif
> +}
> +
>  enum {
>      GDB_SIGNAL_0 = 0,
>      GDB_SIGNAL_INT = 2,
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index c4fe567600..9aa7756d92 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -58,20 +58,6 @@ void gdb_register_coprocessor(CPUState *cpu,
>                                gdb_reg_cb get_reg, gdb_reg_cb set_reg,
>                                int num_regs, const char *xml, int g_pos);
>
> -/* Return the GDB index for a given vCPU state.
> - *
> - * For user mode this is simply the thread id. In system mode GDB
> - * numbers CPUs from 1 as 0 is reserved as an "any cpu" index.
> - */
> -static inline int cpu_gdb_index(CPUState *cpu)
> -{
> -#if defined(CONFIG_USER_ONLY)
> -    return cpu->host_tid;
> -#else
> -    return cpu->cpu_index + 1;
> -#endif
> -}
> -
>  /* The GDB remote protocol transfers values in target byte order.  This means
>   * we can use the raw memory access routines to access the value buffer.
>   * Conveniently, these also handle the case where the buffer is mis-aligned.
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 55214ce131..909e7ae994 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -266,7 +266,6 @@ struct qemu_work_item;
>   * @nr_cores: Number of cores within this CPU package.
>   * @nr_threads: Number of threads within this CPU.
>   * @numa_node: NUMA node this CPU is belonging to.
> - * @host_tid: Host thread ID.
>   * @running: #true if CPU is currently running (lockless).
>   * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;
>   * valid under cpu_list_lock.
> @@ -321,7 +320,6 @@ struct CPUState {
>      HANDLE hThread;
>  #endif
>      int thread_id;
> -    uint32_t host_tid;
>      bool running, has_waiter;
>      struct QemuCond *halt_cond;
>      bool thread_kicked;
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index cec8428589..cada188e58 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -6216,7 +6216,6 @@ static void *clone_func(void *arg)
>      thread_cpu = cpu;
>      ts = (TaskState *)cpu->opaque;
>      info->tid = gettid();
> -    cpu->host_tid = info->tid;
>      task_settid(ts);
>      if (info->child_tidptr)
>          put_user_u32(info->tid, info->child_tidptr);
>

Re: [Qemu-devel] [PATCH v2 3/4] qom/cpu: remove host_tid field

[Laurent Vivier]

Le 01/06/2017 à 16:49, Alex Bennée a écrit :
> This was only used by the gdbstub and even then was only being set for
> subsequent threads. Rather the continue duplicating the number just
> make the gdbstub get the information from TaskState structure.
> 
> Now the tid is correctly reported for all threads the bug I was seeing
> with "vCont;C04:0;c" packets is fixed as the correct tid is reported
> to gdb.
> 
> I moved cpu_gdb_index into the gdbstub to facilitate easy access to
> the TaskState which is used elsewhere in gdbstub.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

> ---
>  gdbstub.c              | 15 +++++++++++++++
>  include/exec/gdbstub.h | 14 --------------
>  include/qom/cpu.h      |  2 --
>  linux-user/syscall.c   |  1 -
>  4 files changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index 026d1fe6bb..45a3a0b16b 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -55,6 +55,21 @@ static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr,
>      return cpu_memory_rw_debug(cpu, addr, buf, len, is_write);
>  }
>  
> +/* Return the GDB index for a given vCPU state.
> + *
> + * For user mode this is simply the thread id. In system mode GDB
> + * numbers CPUs from 1 as 0 is reserved as an "any cpu" index.
> + */
> +static inline int cpu_gdb_index(CPUState *cpu)
> +{
> +#if defined(CONFIG_USER_ONLY)
> +    TaskState *ts = (TaskState *) cpu->opaque;
> +    return ts->ts_tid;
> +#else
> +    return cpu->cpu_index + 1;
> +#endif
> +}
> +
>  enum {
>      GDB_SIGNAL_0 = 0,
>      GDB_SIGNAL_INT = 2,
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index c4fe567600..9aa7756d92 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -58,20 +58,6 @@ void gdb_register_coprocessor(CPUState *cpu,
>                                gdb_reg_cb get_reg, gdb_reg_cb set_reg,
>                                int num_regs, const char *xml, int g_pos);
>  
> -/* Return the GDB index for a given vCPU state.
> - *
> - * For user mode this is simply the thread id. In system mode GDB
> - * numbers CPUs from 1 as 0 is reserved as an "any cpu" index.
> - */
> -static inline int cpu_gdb_index(CPUState *cpu)
> -{
> -#if defined(CONFIG_USER_ONLY)
> -    return cpu->host_tid;
> -#else
> -    return cpu->cpu_index + 1;
> -#endif
> -}
> -
>  /* The GDB remote protocol transfers values in target byte order.  This means
>   * we can use the raw memory access routines to access the value buffer.
>   * Conveniently, these also handle the case where the buffer is mis-aligned.
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 55214ce131..909e7ae994 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -266,7 +266,6 @@ struct qemu_work_item;
>   * @nr_cores: Number of cores within this CPU package.
>   * @nr_threads: Number of threads within this CPU.
>   * @numa_node: NUMA node this CPU is belonging to.
> - * @host_tid: Host thread ID.
>   * @running: #true if CPU is currently running (lockless).
>   * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;
>   * valid under cpu_list_lock.
> @@ -321,7 +320,6 @@ struct CPUState {
>      HANDLE hThread;
>  #endif
>      int thread_id;
> -    uint32_t host_tid;
>      bool running, has_waiter;
>      struct QemuCond *halt_cond;
>      bool thread_kicked;
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index cec8428589..cada188e58 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -6216,7 +6216,6 @@ static void *clone_func(void *arg)
>      thread_cpu = cpu;
>      ts = (TaskState *)cpu->opaque;
>      info->tid = gettid();
> -    cpu->host_tid = info->tid;
>      task_settid(ts);
>      if (info->child_tidptr)
>          put_user_u32(info->tid, info->child_tidptr);
> 

[Qemu-devel] [PATCH v2 4/4] gdbstub: don't fail on vCont; C04:0; c packets

[Alex Bennée]

The thread-id of 0 means any CPU but we then ignore the fact we find
the first_cpu in this case who can have an index of 0. Instead of
bailing out just test if we have managed to match up thread-id to a
CPU.

Otherwise you get:
  gdb_handle_packet: command='vCont;C04:0;c'
  put_packet: reply='E22'

The actual reason for gdb sending vCont;C04:0;c was fixed in a
previous commit where we ensure the first_cpu's tid is correctly
reported to gdb however we should still behave correctly next time it
does send 0.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Greg Kurz <groug@kaod.org>

---
v2
  - used Greg's less convoluted suggestion
  - expand commit message
---
 gdbstub.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 45a3a0b16b..6b1e72e9f7 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -937,23 +937,16 @@ static int gdb_handle_vcont(GDBState *s, const char *p)
             if (res) {
                 goto out;
             }
-            idx = tmp;
+
             /* 0 means any thread, so we pick the first valid CPU */
-            if (!idx) {
-                idx = cpu_gdb_index(first_cpu);
-            }
+            cpu = tmp ? find_cpu(tmp) : first_cpu;
 
-            /*
-             * If we are in user mode, the thread specified is actually a
-             * thread id, and not an index. We need to find the actual
-             * CPU first, and only then we can use its index.
-             */
-            cpu = find_cpu(idx);
             /* invalid CPU/thread specified */
-            if (!idx || !cpu) {
+            if (!cpu) {
                 res = -EINVAL;
                 goto out;
             }
+
             /* only use if no previous match occourred */
             if (newstates[cpu->cpu_index] == 1) {
                 newstates[cpu->cpu_index] = cur_action;
-- 
2.13.0

Re: [Qemu-devel] [PATCH v2 4/4] gdbstub: don't fail on vCont; C04:0; c packets

[Philippe Mathieu-Daudé]

On 06/01/2017 11:49 AM, Alex Bennée wrote:
> The thread-id of 0 means any CPU but we then ignore the fact we find
> the first_cpu in this case who can have an index of 0. Instead of
> bailing out just test if we have managed to match up thread-id to a
> CPU.
>
> Otherwise you get:
>   gdb_handle_packet: command='vCont;C04:0;c'
>   put_packet: reply='E22'
>
> The actual reason for gdb sending vCont;C04:0;c was fixed in a
> previous commit where we ensure the first_cpu's tid is correctly
> reported to gdb however we should still behave correctly next time it
> does send 0.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>
> ---
> v2
>   - used Greg's less convoluted suggestion
>   - expand commit message
> ---
>  gdbstub.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 45a3a0b16b..6b1e72e9f7 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -937,23 +937,16 @@ static int gdb_handle_vcont(GDBState *s, const char *p)
>              if (res) {
>                  goto out;
>              }
> -            idx = tmp;
> +
>              /* 0 means any thread, so we pick the first valid CPU */
> -            if (!idx) {
> -                idx = cpu_gdb_index(first_cpu);
> -            }
> +            cpu = tmp ? find_cpu(tmp) : first_cpu;
>
> -            /*
> -             * If we are in user mode, the thread specified is actually a
> -             * thread id, and not an index. We need to find the actual
> -             * CPU first, and only then we can use its index.
> -             */
> -            cpu = find_cpu(idx);
>              /* invalid CPU/thread specified */
> -            if (!idx || !cpu) {
> +            if (!cpu) {
>                  res = -EINVAL;
>                  goto out;
>              }
> +
>              /* only use if no previous match occourred */
>              if (newstates[cpu->cpu_index] == 1) {
>                  newstates[cpu->cpu_index] = cur_action;
>

Re: [Qemu-devel] [PATCH v2 4/4] gdbstub: don't fail on vCont; C04:0; c packets

[Claudio Imbrenda]

On Thu,  1 Jun 2017 15:49:15 +0100
Alex Bennée <alex.bennee@linaro.org> wrote:

> The thread-id of 0 means any CPU but we then ignore the fact we find
> the first_cpu in this case who can have an index of 0. Instead of
> bailing out just test if we have managed to match up thread-id to a
> CPU.
> 
> Otherwise you get:
>   gdb_handle_packet: command='vCont;C04:0;c'
>   put_packet: reply='E22'
> 
> The actual reason for gdb sending vCont;C04:0;c was fixed in a
> previous commit where we ensure the first_cpu's tid is correctly
> reported to gdb however we should still behave correctly next time it
> does send 0.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> ---
> v2
>   - used Greg's less convoluted suggestion
>   - expand commit message
> ---
>  gdbstub.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index 45a3a0b16b..6b1e72e9f7 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -937,23 +937,16 @@ static int gdb_handle_vcont(GDBState *s, const
> char *p) if (res) {
>                  goto out;
>              }
> -            idx = tmp;
> +
>              /* 0 means any thread, so we pick the first valid CPU */
> -            if (!idx) {
> -                idx = cpu_gdb_index(first_cpu);
> -            }
> +            cpu = tmp ? find_cpu(tmp) : first_cpu;
> 
> -            /*
> -             * If we are in user mode, the thread specified is
> actually a
> -             * thread id, and not an index. We need to find the
> actual
> -             * CPU first, and only then we can use its index.
> -             */
> -            cpu = find_cpu(idx);
>              /* invalid CPU/thread specified */
> -            if (!idx || !cpu) {
> +            if (!cpu) {
>                  res = -EINVAL;
>                  goto out;
>              }
> +
>              /* only use if no previous match occourred */
>              if (newstates[cpu->cpu_index] == 1) {
>                  newstates[cpu->cpu_index] = cur_action;


Reviewed-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>