From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47469) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gOgfy-00079b-EG for qemu-devel@nongnu.org; Mon, 19 Nov 2018 05:21:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gOgYF-00042b-HF for qemu-devel@nongnu.org; Mon, 19 Nov 2018 05:13:09 -0500 References: <20181115094207.22846-1-luc.michel@greensocs.com> <20181115094207.22846-8-luc.michel@greensocs.com> <20181116100407.GQ7447@toto> From: Luc Michel Message-ID: Date: Mon, 19 Nov 2018 11:12:45 +0100 MIME-Version: 1.0 In-Reply-To: <20181116100407.GQ7447@toto> Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v6 07/16] gdbstub: add multiprocess support to (f|s)ThreadInfo and ThreadExtraInfo List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Edgar E. Iglesias" Cc: qemu-devel@nongnu.org, qemu-arm@nongnu.org, Peter Maydell , saipava@xilinx.com, edgari@xilinx.com, alistair@alistair23.me, =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , mark.burton@greensocs.com, Eduardo Habkost 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 multiproces= s >> 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=C3=A9 >> --- >> 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, cons= t char *line_buf) >> snprintf(buf, sizeof(buf), "QC%s", >> gdb_fmt_thread_id(s, cpu, thread_id, sizeof(thre= ad_id))); >> put_packet(s, buf); >> break; >> } else if (strcmp(p,"fThreadInfo") =3D=3D 0) { >> - s->query_cpu =3D first_cpu; >> + s->query_cpu =3D gdb_first_cpu(s); >> goto report_cpuinfo; >> } else if (strcmp(p,"sThreadInfo") =3D=3D 0) { >> report_cpuinfo: >> if (s->query_cpu) { >> - snprintf(buf, sizeof(buf), "m%x", cpu_gdb_index(s->qu= ery_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 =3D CPU_NEXT(s->query_cpu); >> + s->query_cpu =3D gdb_next_cpu(s, s->query_cpu); >> } else >> put_packet(s, "l"); >> break; >> } else if (strncmp(p,"ThreadExtraInfo,", 16) =3D=3D 0) { >> - thread =3D strtoull(p+16, (char **)&p, 16); >> - cpu =3D find_cpu(thread); >> + if (read_thread_id(p + 16, &p, &pid, &tid) =3D=3D GDB_REA= D_THREAD_ERR) { >> + put_packet(s, "E22"); >> + break; >> + } >> + cpu =3D gdb_get_cpu(s, pid, tid); >> if (cpu !=3D NULL) { >> cpu_synchronize_state(cpu); >> - /* memtohex() doubles the required space */ >> - len =3D 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 =3D object_get_class(OBJECT(cpu))= ; >> + const char *cpu_model =3D object_class_get_name(o= c); >> + len =3D snprintf((char *)mem_buf, sizeof(buf) / 2= , >> + "CPU#%d %s [%s]", cpu->cpu_index, >> + cpu_model, >> + cpu->halted ? "halted " : "running= "); >=20 >=20 >=20 > 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). >=20 > 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: >=20 > Instead of: > CPU#0 MicroBlaze [running] > CPU#1 MicroBlaze [running] > CPU#2 MicroBlaze [running] > CPU#3 MicroBlaze [running] >=20 > 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] >=20 > 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]) 0x0000000000000000 in ?? () 1.2 Thread 1.2 (CPU#1 cortex-a53-arm-cpu apu-cpu[1] [halted ]) 0x0000000000000000 in ?? () 1.3 Thread 1.3 (CPU#2 cortex-a53-arm-cpu apu-cpu[2] [halted ]) 0x0000000000000000 in ?? () 1.4 Thread 1.4 (CPU#3 cortex-a53-arm-cpu apu-cpu[3] [halted ]) 0x0000000000000000 in ?? () * 2.1 Thread 2.5 (CPU#4 cortex-r5f-arm-cpu rpu-cpu[0] [halted ]) 0xffff0000 in ?? () 2.2 Thread 2.6 (CPU#5 cortex-r5f-arm-cpu rpu-cpu[1] [halted ]) 0xffff0000 in ?? () The model name takes quite some room. The interesting info are `arm` and `cortex-xxx`, but AFAIK there is no way of extracting that for a CPU generically. In this case, having the component part of the canonical name is ok because self-explanatory. However we could encounter cases where the parent name would be necessary to discriminate the CPUs, something like: cluster[0]/cpu[0] /cpu[1] cluster[1]/cpu[0] /cpu[1] ... The "safest" way would be to have the whole path: (gdb) info threads Id Target Id Frame 1.1 Thread 1.1 (CPU#0 cortex-a53-arm-cpu /machine/soc/apu-cluster/apu-cpu[0] [running]) 0x0000000000000000 in ?? (= ) 1.2 Thread 1.2 (CPU#1 cortex-a53-arm-cpu /machine/soc/apu-cluster/apu-cpu[1] [halted ]) 0x0000000000000000 in ?? (= ) 1.3 Thread 1.3 (CPU#2 cortex-a53-arm-cpu /machine/soc/apu-cluster/apu-cpu[2] [halted ]) 0x0000000000000000 in ?? (= ) 1.4 Thread 1.4 (CPU#3 cortex-a53-arm-cpu /machine/soc/apu-cluster/apu-cpu[3] [halted ]) 0x0000000000000000 in ?? (= ) * 2.1 Thread 2.5 (CPU#4 cortex-r5f-arm-cpu /machine/soc/rpu-cluster/rpu-cpu[0] [halted ]) 0xffff0000 in ?? () 2.2 Thread 2.6 (CPU#5 cortex-r5f-arm-cpu /machine/soc/rpu-cluster/rpu-cpu[1] [halted ]) 0xffff0000 in ?? () But that becomes really cluttered... We could also remove the CPU model completely. What are your thoughts? Thanks, Luc >=20 > Thanks, > Edgar >=20 >> + } else { >> + /* memtohex() doubles the required space */ >> + len =3D 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; >> --=20 >> 2.19.1 >>