Re: [Qemu-devel] [PATCH v6 07/16] gdbstub: add multiprocess support to (f|s)ThreadInfo and ThreadExtraInfo
On Mon, Nov 19, 2018 at 11:12:45AM +0100, Luc Michel wrote:
>
>
> On 11/16/18 11:04 AM, Edgar E. Iglesias wrote:
> > On Thu, Nov 15, 2018 at 10:41:58AM +0100, Luc Michel wrote:
> >> Change the thread info related packets handling to support multiprocess
> >> extension.
> >>
> >> Add the CPUs class name in the extra info to help differentiate
> >> them in multiprocess mode.
> >>
> >> Signed-off-by: Luc Michel
> >> Reviewed-by: Philippe Mathieu-Daudé
> >> ---
> >> gdbstub.c | 35 +--
> >> 1 file changed, 25 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/gdbstub.c b/gdbstub.c
> >> index d19b0137e8..292dee8914 100644
> >> --- a/gdbstub.c
> >> +++ b/gdbstub.c
> >> @@ -1260,11 +1260,10 @@ out:
> >> static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >> {
> >> CPUState *cpu;
> >> CPUClass *cc;
> >> const char *p;
> >> -uint32_t thread;
> >> uint32_t pid, tid;
> >> int ch, reg_size, type, res;
> >> uint8_t mem_buf[MAX_PACKET_LENGTH];
> >> char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
> >> char thread_id[16];
> >> @@ -1556,30 +1555,46 @@ static int gdb_handle_packet(GDBState *s, const
> >> char *line_buf)
> >> snprintf(buf, sizeof(buf), "QC%s",
> >> gdb_fmt_thread_id(s, cpu, thread_id,
> >> sizeof(thread_id)));
> >> put_packet(s, buf);
> >> break;
> >> } else if (strcmp(p,"fThreadInfo") == 0) {
> >> -s->query_cpu = first_cpu;
> >> +s->query_cpu = gdb_first_cpu(s);
> >> goto report_cpuinfo;
> >> } else if (strcmp(p,"sThreadInfo") == 0) {
> >> report_cpuinfo:
> >> if (s->query_cpu) {
> >> -snprintf(buf, sizeof(buf), "m%x",
> >> cpu_gdb_index(s->query_cpu));
> >> +snprintf(buf, sizeof(buf), "m%s",
> >> + gdb_fmt_thread_id(s, s->query_cpu,
> >> + thread_id, sizeof(thread_id)));
> >> put_packet(s, buf);
> >> -s->query_cpu = CPU_NEXT(s->query_cpu);
> >> +s->query_cpu = gdb_next_cpu(s, s->query_cpu);
> >> } else
> >> put_packet(s, "l");
> >> break;
> >> } else if (strncmp(p,"ThreadExtraInfo,", 16) == 0) {
> >> -thread = strtoull(p+16, (char **), 16);
> >> -cpu = find_cpu(thread);
> >> +if (read_thread_id(p + 16, , , ) ==
> >> GDB_READ_THREAD_ERR) {
> >> +put_packet(s, "E22");
> >> +break;
> >> +}
> >> +cpu = gdb_get_cpu(s, pid, tid);
> >> if (cpu != NULL) {
> >> cpu_synchronize_state(cpu);
> >> -/* memtohex() doubles the required space */
> >> -len = snprintf((char *)mem_buf, sizeof(buf) / 2,
> >> - "CPU#%d [%s]", cpu->cpu_index,
> >> - cpu->halted ? "halted " : "running");
> >> +
> >> +if (s->multiprocess && (s->process_num > 1)) {
> >> +/* Print the CPU model in multiprocess mode */
> >> +ObjectClass *oc = object_get_class(OBJECT(cpu));
> >> +const char *cpu_model = object_class_get_name(oc);
> >> +len = snprintf((char *)mem_buf, sizeof(buf) / 2,
> >> + "CPU#%d %s [%s]", cpu->cpu_index,
> >> + cpu_model,
> >> + cpu->halted ? "halted " : "running");
> >
> >
> >
> > I wonder if we could also print a friendly name here deducted from QOM?
> > In some of our use-cases we have an array of MicroBlazes that all live
> > in different HW subsystems and are named differently (e.g CSU, PMU, PMC,
> > PSM etc).
> >
> > Instead of just seeing a list of MicroBlaze cores it may be more useful
> > to see the actual core name of some sort, e.g:
> >
> > Instead of:
> > CPU#0 MicroBlaze [running]
> > CPU#1 MicroBlaze [running]
> > CPU#2 MicroBlaze [running]
> > CPU#3 MicroBlaze [running]
> >
> > Perhaps something like:
> > CPU#0 MicroBlaze PMU [running]
> > CPU#1 MicroBlaze PMC-PPU0 [running]
> > CPU#2 MicroBlaze PMC-PPU1 [running]
> > CPU#3 MicroBlaze PSM [running]
> >
> > Any thoughts on that?
> I wanted to avoid the ThreadExtraInfo packet to become too much cluttered.
>
> Here are some tests adding the component part of the CPU canonical name:
>
> (gdb) info threads
> Id Target Id Frame
> 1.1 Thread 1.1 (CPU#0 cortex-a53-arm-cpu apu-cpu[0] [running])
> 0x in ?? ()
> 1.2 Thread 1.2 (CPU#1 cortex-a53-arm-cpu apu-cpu[1] [halted ])
> 0x in ?? ()
> 1.3 Thread 1.3 (CPU#2 cortex-a53-arm-cpu apu-cpu[2] [halted ])
> 0x in ?? ()
> 1.4 Thread 1.4 (CPU#3 cortex-a53-arm-cpu apu-cpu[3] [halted ])
> 0x in ?? ()
> *
Re: [Qemu-devel] [PATCH v6 07/16] gdbstub: add multiprocess support to (f|s)ThreadInfo and ThreadExtraInfo
On 11/16/18 11:04 AM, Edgar E. Iglesias wrote:
> On Thu, Nov 15, 2018 at 10:41:58AM +0100, Luc Michel wrote:
>> Change the thread info related packets handling to support multiprocess
>> extension.
>>
>> Add the CPUs class name in the extra info to help differentiate
>> them in multiprocess mode.
>>
>> Signed-off-by: Luc Michel
>> Reviewed-by: Philippe Mathieu-Daudé
>> ---
>> gdbstub.c | 35 +--
>> 1 file changed, 25 insertions(+), 10 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index d19b0137e8..292dee8914 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -1260,11 +1260,10 @@ out:
>> static int gdb_handle_packet(GDBState *s, const char *line_buf)
>> {
>> CPUState *cpu;
>> CPUClass *cc;
>> const char *p;
>> -uint32_t thread;
>> uint32_t pid, tid;
>> int ch, reg_size, type, res;
>> uint8_t mem_buf[MAX_PACKET_LENGTH];
>> char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
>> char thread_id[16];
>> @@ -1556,30 +1555,46 @@ static int gdb_handle_packet(GDBState *s, const char
>> *line_buf)
>> snprintf(buf, sizeof(buf), "QC%s",
>> gdb_fmt_thread_id(s, cpu, thread_id,
>> sizeof(thread_id)));
>> put_packet(s, buf);
>> break;
>> } else if (strcmp(p,"fThreadInfo") == 0) {
>> -s->query_cpu = first_cpu;
>> +s->query_cpu = gdb_first_cpu(s);
>> goto report_cpuinfo;
>> } else if (strcmp(p,"sThreadInfo") == 0) {
>> report_cpuinfo:
>> if (s->query_cpu) {
>> -snprintf(buf, sizeof(buf), "m%x",
>> cpu_gdb_index(s->query_cpu));
>> +snprintf(buf, sizeof(buf), "m%s",
>> + gdb_fmt_thread_id(s, s->query_cpu,
>> + thread_id, sizeof(thread_id)));
>> put_packet(s, buf);
>> -s->query_cpu = CPU_NEXT(s->query_cpu);
>> +s->query_cpu = gdb_next_cpu(s, s->query_cpu);
>> } else
>> put_packet(s, "l");
>> break;
>> } else if (strncmp(p,"ThreadExtraInfo,", 16) == 0) {
>> -thread = strtoull(p+16, (char **), 16);
>> -cpu = find_cpu(thread);
>> +if (read_thread_id(p + 16, , , ) ==
>> GDB_READ_THREAD_ERR) {
>> +put_packet(s, "E22");
>> +break;
>> +}
>> +cpu = gdb_get_cpu(s, pid, tid);
>> if (cpu != NULL) {
>> cpu_synchronize_state(cpu);
>> -/* memtohex() doubles the required space */
>> -len = snprintf((char *)mem_buf, sizeof(buf) / 2,
>> - "CPU#%d [%s]", cpu->cpu_index,
>> - cpu->halted ? "halted " : "running");
>> +
>> +if (s->multiprocess && (s->process_num > 1)) {
>> +/* Print the CPU model in multiprocess mode */
>> +ObjectClass *oc = object_get_class(OBJECT(cpu));
>> +const char *cpu_model = object_class_get_name(oc);
>> +len = snprintf((char *)mem_buf, sizeof(buf) / 2,
>> + "CPU#%d %s [%s]", cpu->cpu_index,
>> + cpu_model,
>> + cpu->halted ? "halted " : "running");
>
>
>
> I wonder if we could also print a friendly name here deducted from QOM?
> In some of our use-cases we have an array of MicroBlazes that all live
> in different HW subsystems and are named differently (e.g CSU, PMU, PMC,
> PSM etc).
>
> Instead of just seeing a list of MicroBlaze cores it may be more useful
> to see the actual core name of some sort, e.g:
>
> Instead of:
> CPU#0 MicroBlaze [running]
> CPU#1 MicroBlaze [running]
> CPU#2 MicroBlaze [running]
> CPU#3 MicroBlaze [running]
>
> Perhaps something like:
> CPU#0 MicroBlaze PMU [running]
> CPU#1 MicroBlaze PMC-PPU0 [running]
> CPU#2 MicroBlaze PMC-PPU1 [running]
> CPU#3 MicroBlaze PSM [running]
>
> Any thoughts on that?
I wanted to avoid the ThreadExtraInfo packet to become too much cluttered.
Here are some tests adding the component part of the CPU canonical name:
(gdb) info threads
Id Target Id Frame
1.1 Thread 1.1 (CPU#0 cortex-a53-arm-cpu apu-cpu[0] [running])
0x in ?? ()
1.2 Thread 1.2 (CPU#1 cortex-a53-arm-cpu apu-cpu[1] [halted ])
0x in ?? ()
1.3 Thread 1.3 (CPU#2 cortex-a53-arm-cpu apu-cpu[2] [halted ])
0x in ?? ()
1.4 Thread 1.4 (CPU#3 cortex-a53-arm-cpu apu-cpu[3] [halted ])
0x in ?? ()
* 2.1 Thread 2.5 (CPU#4 cortex-r5f-arm-cpu rpu-cpu[0] [halted ])
0x in ?? ()
2.2 Thread 2.6 (CPU#5 cortex-r5f-arm-cpu rpu-cpu[1] [halted ])
0x in ?? ()
The model name takes quite some room. The interesting info are `arm` and
`cortex-xxx`, but AFAIK there is no way of extracting
Re: [Qemu-devel] [PATCH v6 07/16] gdbstub: add multiprocess support to (f|s)ThreadInfo and ThreadExtraInfo
On Thu, Nov 15, 2018 at 10:41:58AM +0100, Luc Michel wrote:
> Change the thread info related packets handling to support multiprocess
> extension.
>
> Add the CPUs class name in the extra info to help differentiate
> them in multiprocess mode.
>
> Signed-off-by: Luc Michel
> Reviewed-by: Philippe Mathieu-Daudé
> ---
> gdbstub.c | 35 +--
> 1 file changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index d19b0137e8..292dee8914 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1260,11 +1260,10 @@ out:
> static int gdb_handle_packet(GDBState *s, const char *line_buf)
> {
> CPUState *cpu;
> CPUClass *cc;
> const char *p;
> -uint32_t thread;
> uint32_t pid, tid;
> int ch, reg_size, type, res;
> uint8_t mem_buf[MAX_PACKET_LENGTH];
> char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
> char thread_id[16];
> @@ -1556,30 +1555,46 @@ static int gdb_handle_packet(GDBState *s, const char
> *line_buf)
> snprintf(buf, sizeof(buf), "QC%s",
> gdb_fmt_thread_id(s, cpu, thread_id,
> sizeof(thread_id)));
> put_packet(s, buf);
> break;
> } else if (strcmp(p,"fThreadInfo") == 0) {
> -s->query_cpu = first_cpu;
> +s->query_cpu = gdb_first_cpu(s);
> goto report_cpuinfo;
> } else if (strcmp(p,"sThreadInfo") == 0) {
> report_cpuinfo:
> if (s->query_cpu) {
> -snprintf(buf, sizeof(buf), "m%x",
> cpu_gdb_index(s->query_cpu));
> +snprintf(buf, sizeof(buf), "m%s",
> + gdb_fmt_thread_id(s, s->query_cpu,
> + thread_id, sizeof(thread_id)));
> put_packet(s, buf);
> -s->query_cpu = CPU_NEXT(s->query_cpu);
> +s->query_cpu = gdb_next_cpu(s, s->query_cpu);
> } else
> put_packet(s, "l");
> break;
> } else if (strncmp(p,"ThreadExtraInfo,", 16) == 0) {
> -thread = strtoull(p+16, (char **), 16);
> -cpu = find_cpu(thread);
> +if (read_thread_id(p + 16, , , ) ==
> GDB_READ_THREAD_ERR) {
> +put_packet(s, "E22");
> +break;
> +}
> +cpu = gdb_get_cpu(s, pid, tid);
> if (cpu != NULL) {
> cpu_synchronize_state(cpu);
> -/* memtohex() doubles the required space */
> -len = snprintf((char *)mem_buf, sizeof(buf) / 2,
> - "CPU#%d [%s]", cpu->cpu_index,
> - cpu->halted ? "halted " : "running");
> +
> +if (s->multiprocess && (s->process_num > 1)) {
> +/* Print the CPU model in multiprocess mode */
> +ObjectClass *oc = object_get_class(OBJECT(cpu));
> +const char *cpu_model = object_class_get_name(oc);
> +len = snprintf((char *)mem_buf, sizeof(buf) / 2,
> + "CPU#%d %s [%s]", cpu->cpu_index,
> + cpu_model,
> + cpu->halted ? "halted " : "running");
I wonder if we could also print a friendly name here deducted from QOM?
In some of our use-cases we have an array of MicroBlazes that all live
in different HW subsystems and are named differently (e.g CSU, PMU, PMC,
PSM etc).
Instead of just seeing a list of MicroBlaze cores it may be more useful
to see the actual core name of some sort, e.g:
Instead of:
CPU#0 MicroBlaze [running]
CPU#1 MicroBlaze [running]
CPU#2 MicroBlaze [running]
CPU#3 MicroBlaze [running]
Perhaps something like:
CPU#0 MicroBlaze PMU [running]
CPU#1 MicroBlaze PMC-PPU0 [running]
CPU#2 MicroBlaze PMC-PPU1 [running]
CPU#3 MicroBlaze PSM [running]
Any thoughts on that?
Thanks,
Edgar
> +} else {
> +/* memtohex() doubles the required space */
> +len = snprintf((char *)mem_buf, sizeof(buf) / 2,
> + "CPU#%d [%s]", cpu->cpu_index,
> + cpu->halted ? "halted " : "running");
> +}
> trace_gdbstub_op_extra_info((char *)mem_buf);
> memtohex(buf, mem_buf, len);
> put_packet(s, buf);
> }
> break;
> --
> 2.19.1
>
[Qemu-devel] [PATCH v6 07/16] gdbstub: add multiprocess support to (f|s)ThreadInfo and ThreadExtraInfo
Change the thread info related packets handling to support multiprocess
extension.
Add the CPUs class name in the extra info to help differentiate
them in multiprocess mode.
Signed-off-by: Luc Michel
Reviewed-by: Philippe Mathieu-Daudé
---
gdbstub.c | 35 +--
1 file changed, 25 insertions(+), 10 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index d19b0137e8..292dee8914 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1260,11 +1260,10 @@ out:
static int gdb_handle_packet(GDBState *s, const char *line_buf)
{
CPUState *cpu;
CPUClass *cc;
const char *p;
-uint32_t thread;
uint32_t pid, tid;
int ch, reg_size, type, res;
uint8_t mem_buf[MAX_PACKET_LENGTH];
char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
char thread_id[16];
@@ -1556,30 +1555,46 @@ static int gdb_handle_packet(GDBState *s, const char
*line_buf)
snprintf(buf, sizeof(buf), "QC%s",
gdb_fmt_thread_id(s, cpu, thread_id, sizeof(thread_id)));
put_packet(s, buf);
break;
} else if (strcmp(p,"fThreadInfo") == 0) {
-s->query_cpu = first_cpu;
+s->query_cpu = gdb_first_cpu(s);
goto report_cpuinfo;
} else if (strcmp(p,"sThreadInfo") == 0) {
report_cpuinfo:
if (s->query_cpu) {
-snprintf(buf, sizeof(buf), "m%x", cpu_gdb_index(s->query_cpu));
+snprintf(buf, sizeof(buf), "m%s",
+ gdb_fmt_thread_id(s, s->query_cpu,
+ thread_id, sizeof(thread_id)));
put_packet(s, buf);
-s->query_cpu = CPU_NEXT(s->query_cpu);
+s->query_cpu = gdb_next_cpu(s, s->query_cpu);
} else
put_packet(s, "l");
break;
} else if (strncmp(p,"ThreadExtraInfo,", 16) == 0) {
-thread = strtoull(p+16, (char **), 16);
-cpu = find_cpu(thread);
+if (read_thread_id(p + 16, , , ) == GDB_READ_THREAD_ERR)
{
+put_packet(s, "E22");
+break;
+}
+cpu = gdb_get_cpu(s, pid, tid);
if (cpu != NULL) {
cpu_synchronize_state(cpu);
-/* memtohex() doubles the required space */
-len = snprintf((char *)mem_buf, sizeof(buf) / 2,
- "CPU#%d [%s]", cpu->cpu_index,
- cpu->halted ? "halted " : "running");
+
+if (s->multiprocess && (s->process_num > 1)) {
+/* Print the CPU model in multiprocess mode */
+ObjectClass *oc = object_get_class(OBJECT(cpu));
+const char *cpu_model = object_class_get_name(oc);
+len = snprintf((char *)mem_buf, sizeof(buf) / 2,
+ "CPU#%d %s [%s]", cpu->cpu_index,
+ cpu_model,
+ cpu->halted ? "halted " : "running");
+} else {
+/* memtohex() doubles the required space */
+len = snprintf((char *)mem_buf, sizeof(buf) / 2,
+ "CPU#%d [%s]", cpu->cpu_index,
+ cpu->halted ? "halted " : "running");
+}
trace_gdbstub_op_extra_info((char *)mem_buf);
memtohex(buf, mem_buf, len);
put_packet(s, buf);
}
break;
--
2.19.1
