[Qemu-devel] [RESEND][PATCH] gdbstub: Add vCont support

[Qemu-devel] [RESEND][PATCH] gdbstub: Add vCont support

[Jan Kiszka]

[ Also available via git://git.kiszka.org/qemu.git queue/gdb ]

In order to set the VCPU for the next single-step command, you need gdb
6.8 or better - and this patch. It enhances the existing support for
representing VCPUs as threads to the gdb frontend by introducing the
vCont remote gdb command. This is used by gdb to switch the debugging
focus for single-stepping multi-threaded targets.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 gdbstub.c |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 0bcd5d5..1cb20b7 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1542,6 +1542,62 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
             s->signal = 0;
         gdb_continue(s);
         return RS_IDLE;
+    case 'v':
+        if (strncmp(p, "Cont", 4) == 0) {
+            int res_signal, res_thread;
+
+            p += 4;
+            if (*p == '?') {
+                put_packet(s, "vCont;c;C;s;S");
+                break;
+            }
+            res = 0;
+            res_signal = 0;
+            res_thread = 0;
+            while (*p) {
+                int action, signal;
+
+                if (*p++ != ';') {
+                    res = 0;
+                    break;
+                }
+                action = *p++;
+                signal = 0;
+                if (action == 'C' || action == 'S')
+                    signal = strtoul(p, (char **)&p, 16);
+                else if (action != 'c' && action != 's') {
+                    res = 0;
+                    break;
+                }
+                thread = 0;
+                if (*p == ':')
+                    thread = strtoull(p+1, (char **)&p, 16);
+
+                action = tolower(action);
+                if (res == 0 || (res == 'c' && action == 's')) {
+                    res = action;
+                    res_signal = signal;
+                    res_thread = thread;
+                }
+            }
+            if (res) {
+                if (res_thread != -1 && res_thread != 0) {
+                    for (env = first_cpu; env != NULL; env = env->next_cpu)
+                        if (env->cpu_index + 1 == res_thread)
+                            break;
+                    if (env == NULL) {
+                        put_packet(s, "E22");
+                        break;
+                    }
+                    s->c_cpu = env;
+                }
+                if (res == 's')
+                    cpu_single_step(s->c_cpu, sstep_flags);
+                gdb_continue(s);
+                return RS_IDLE;
+            }
+            break;
+        }
     case 'k':
         /* Kill the target */
         fprintf(stderr, "\nQEMU: Terminated via GDBstub\n");

RE: [Qemu-devel] [RESEND][PATCH] gdbstub: Add vCont support

[Krumme, Chris]

 

> -----Original Message-----
> From: 
> qemu-devel-bounces+chris.krumme=windriver.com@nongnu.org 
> [mailto:qemu-devel-bounces+chris.krumme=windriver.com@nongnu.o
> rg] On Behalf Of Jan Kiszka
> Sent: Wednesday, January 14, 2009 8:44 AM
> To: qemu-devel@nongnu.org
> Subject: [Qemu-devel] [RESEND][PATCH] gdbstub: Add vCont support
> 
> [ Also available via git://git.kiszka.org/qemu.git queue/gdb ]
> 
> In order to set the VCPU for the next single-step command, 
> you need gdb
> 6.8 or better - and this patch. It enhances the existing support for
> representing VCPUs as threads to the gdb frontend by introducing the
> vCont remote gdb command. This is used by gdb to switch the debugging
> focus for single-stepping multi-threaded targets.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
>  gdbstub.c |   56 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 56 insertions(+), 0 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index 0bcd5d5..1cb20b7 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1542,6 +1542,62 @@ static int gdb_handle_packet(GDBState 
> *s, const char *line_buf)
>              s->signal = 0;
>          gdb_continue(s);
>          return RS_IDLE;
> +    case 'v':
> +        if (strncmp(p, "Cont", 4) == 0) {
> +            int res_signal, res_thread;
> +
> +            p += 4;
> +            if (*p == '?') {
> +                put_packet(s, "vCont;c;C;s;S");
> +                break;
> +            }
> +            res = 0;
> +            res_signal = 0;
> +            res_thread = 0;
> +            while (*p) {
> +                int action, signal;
> +
> +                if (*p++ != ';') {
> +                    res = 0;
> +                    break;
> +                }
> +                action = *p++;
> +                signal = 0;
> +                if (action == 'C' || action == 'S')
> +                    signal = strtoul(p, (char **)&p, 16);
> +                else if (action != 'c' && action != 's') {
> +                    res = 0;
> +                    break;
> +                }
> +                thread = 0;
> +                if (*p == ':')
> +                    thread = strtoull(p+1, (char **)&p, 16);
> +
> +                action = tolower(action);
> +                if (res == 0 || (res == 'c' && action == 's')) {
> +                    res = action;
> +                    res_signal = signal;
> +                    res_thread = thread;
> +                }
> +            }
> +            if (res) {
> +                if (res_thread != -1 && res_thread != 0) {
> +                    for (env = first_cpu; env != NULL; env = 
> env->next_cpu)
> +                        if (env->cpu_index + 1 == res_thread)
> +                            break;
> +                    if (env == NULL) {
> +                        put_packet(s, "E22");
> +                        break;
> +                    }
> +                    s->c_cpu = env;
> +                }
> +                if (res == 's')
> +                    cpu_single_step(s->c_cpu, sstep_flags);
> +                gdb_continue(s);

Where did res_signal go?  (btw: some OS use signal 0 along with the
rest.)

> +                return RS_IDLE;
> +            }

If the command is not vCont do you need to return an error?

Thanks

Chris

> +            break;
> +        }
>      case 'k':
>          /* Kill the target */
>          fprintf(stderr, "\nQEMU: Terminated via GDBstub\n");
> 
> 
> 

[Qemu-devel] Re: [RESEND][PATCH] gdbstub: Add vCont support

[Jan Kiszka]

Krumme, Chris wrote:
>  
> 
>> -----Original Message-----
>> From: 
>> qemu-devel-bounces+chris.krumme=windriver.com@nongnu.org 
>> [mailto:qemu-devel-bounces+chris.krumme=windriver.com@nongnu.o
>> rg] On Behalf Of Jan Kiszka
>> Sent: Wednesday, January 14, 2009 8:44 AM
>> To: qemu-devel@nongnu.org
>> Subject: [Qemu-devel] [RESEND][PATCH] gdbstub: Add vCont support
>>
>> [ Also available via git://git.kiszka.org/qemu.git queue/gdb ]
>>
>> In order to set the VCPU for the next single-step command, 
>> you need gdb
>> 6.8 or better - and this patch. It enhances the existing support for
>> representing VCPUs as threads to the gdb frontend by introducing the
>> vCont remote gdb command. This is used by gdb to switch the debugging
>> focus for single-stepping multi-threaded targets.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>>  gdbstub.c |   56 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 56 insertions(+), 0 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index 0bcd5d5..1cb20b7 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -1542,6 +1542,62 @@ static int gdb_handle_packet(GDBState 
>> *s, const char *line_buf)
>>              s->signal = 0;
>>          gdb_continue(s);
>>          return RS_IDLE;
>> +    case 'v':
>> +        if (strncmp(p, "Cont", 4) == 0) {
>> +            int res_signal, res_thread;
>> +
>> +            p += 4;
>> +            if (*p == '?') {
>> +                put_packet(s, "vCont;c;C;s;S");
>> +                break;
>> +            }
>> +            res = 0;
>> +            res_signal = 0;
>> +            res_thread = 0;
>> +            while (*p) {
>> +                int action, signal;
>> +
>> +                if (*p++ != ';') {
>> +                    res = 0;
>> +                    break;
>> +                }
>> +                action = *p++;
>> +                signal = 0;
>> +                if (action == 'C' || action == 'S')
>> +                    signal = strtoul(p, (char **)&p, 16);
>> +                else if (action != 'c' && action != 's') {
>> +                    res = 0;
>> +                    break;
>> +                }
>> +                thread = 0;
>> +                if (*p == ':')
>> +                    thread = strtoull(p+1, (char **)&p, 16);
>> +
>> +                action = tolower(action);
>> +                if (res == 0 || (res == 'c' && action == 's')) {
>> +                    res = action;
>> +                    res_signal = signal;
>> +                    res_thread = thread;
>> +                }
>> +            }
>> +            if (res) {
>> +                if (res_thread != -1 && res_thread != 0) {
>> +                    for (env = first_cpu; env != NULL; env = 
>> env->next_cpu)
>> +                        if (env->cpu_index + 1 == res_thread)
>> +                            break;
>> +                    if (env == NULL) {
>> +                        put_packet(s, "E22");
>> +                        break;
>> +                    }
>> +                    s->c_cpu = env;
>> +                }
>> +                if (res == 's')
>> +                    cpu_single_step(s->c_cpu, sstep_flags);
>> +                gdb_continue(s);
> 
> Where did res_signal go?

Good question... Looks like I forgot some 's->signal = res_signal;'
here. Will add this.

>  (btw: some OS use signal 0 along with the
> rest.)

In case of the 'c' command, gdbstub also sets s->signal to 0, so I don't
think this default value can be problematic.

> 
>> +                return RS_IDLE;
>> +            }
> 
> If the command is not vCont do you need to return an error?

True, this screams for 'goto unknown_command;'

Thanks for reviewing!

Jan

-- 
Siemens AG, Corporate Technology, CT SE 26
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [RESEND][PATCH] gdbstub: Add vCont support

[Anthony Liguori]

Jan Kiszka wrote:
> [ Also available via git://git.kiszka.org/qemu.git queue/gdb ]
>
> In order to set the VCPU for the next single-step command, you need gdb
> 6.8 or better - and this patch. It enhances the existing support for
> representing VCPUs as threads to the gdb frontend by introducing the
> vCont remote gdb command. This is used by gdb to switch the debugging
> focus for single-stepping multi-threaded targets.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>   

I think the consensus from the last posting of this was that modeling 
threads was pretty broken and that we should model as processes.  Did I 
miss something there?

Regards,

Anthony Liguori

> ---
>
>  gdbstub.c |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 56 insertions(+), 0 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 0bcd5d5..1cb20b7 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1542,6 +1542,62 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>              s->signal = 0;
>          gdb_continue(s);
>          return RS_IDLE;
> +    case 'v':
> +        if (strncmp(p, "Cont", 4) == 0) {
> +            int res_signal, res_thread;
> +
> +            p += 4;
> +            if (*p == '?') {
> +                put_packet(s, "vCont;c;C;s;S");
> +                break;
> +            }
> +            res = 0;
> +            res_signal = 0;
> +            res_thread = 0;
> +            while (*p) {
> +                int action, signal;
> +
> +                if (*p++ != ';') {
> +                    res = 0;
> +                    break;
> +                }
> +                action = *p++;
> +                signal = 0;
> +                if (action == 'C' || action == 'S')
> +                    signal = strtoul(p, (char **)&p, 16);
> +                else if (action != 'c' && action != 's') {
> +                    res = 0;
> +                    break;
> +                }
> +                thread = 0;
> +                if (*p == ':')
> +                    thread = strtoull(p+1, (char **)&p, 16);
> +
> +                action = tolower(action);
> +                if (res == 0 || (res == 'c' && action == 's')) {
> +                    res = action;
> +                    res_signal = signal;
> +                    res_thread = thread;
> +                }
> +            }
> +            if (res) {
> +                if (res_thread != -1 && res_thread != 0) {
> +                    for (env = first_cpu; env != NULL; env = env->next_cpu)
> +                        if (env->cpu_index + 1 == res_thread)
> +                            break;
> +                    if (env == NULL) {
> +                        put_packet(s, "E22");
> +                        break;
> +                    }
> +                    s->c_cpu = env;
> +                }
> +                if (res == 's')
> +                    cpu_single_step(s->c_cpu, sstep_flags);
> +                gdb_continue(s);
> +                return RS_IDLE;
> +            }
> +            break;
> +        }
>      case 'k':
>          /* Kill the target */
>          fprintf(stderr, "\nQEMU: Terminated via GDBstub\n");
>
>
>   

[Qemu-devel] Re: [RESEND][PATCH] gdbstub: Add vCont support

[Jan Kiszka]

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

Anthony Liguori wrote:
> Jan Kiszka wrote:
>> [ Also available via git://git.kiszka.org/qemu.git queue/gdb ]
>>
>> In order to set the VCPU for the next single-step command, you need gdb
>> 6.8 or better - and this patch. It enhances the existing support for
>> representing VCPUs as threads to the gdb frontend by introducing the
>> vCont remote gdb command. This is used by gdb to switch the debugging
>> focus for single-stepping multi-threaded targets.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>   
> 
> I think the consensus from the last posting of this was that modeling
> threads was pretty broken and that we should model as processes.  Did I
> miss something there?

There was this concern brought up by Paul Brook. I looked into this
again, but the picture didn't change for me:

 a) Modeling cpus as processes buys us nothing compared to threads given
    the fact that we cannot provide a stable memory mapping to the gdb
    frontend anyway. (*)

 b) The model is already part of mainline qemu. This patch is just
    about adding even more usefulness to it.

Jan

(*) Process abstraction is, if used at all, guest business. At best we
could try to invent (likely OS-specific) heuristics to identify
identical mappings and call them processes. I don't see a reasonable
benefit compared to the expected effort and unreliability.


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

Re: [Qemu-devel] Re: [RESEND][PATCH] gdbstub: Add vCont support

[Paul Brook]

>  a) Modeling cpus as processes buys us nothing compared to threads given
>     the fact that we cannot provide a stable memory mapping to the gdb
>     frontend anyway. (*)

I disagree. The process model fits perfectly. The whole point is that each CPU 
has its own virtual address space. Separate address spaces is the fundamental 
difference between a process and a thread.

If you have a multicore system where several cores share a MMU[1] then 
modelling these as threads probably make sense.

Don't confuse this with OS awareness in GDB (i.e. implementing a userspace 
debug environment via a bare metal kernel level debug interface). That's a 
completely separate issue.

>  b) The model is already part of mainline qemu. This patch is just
>     about adding even more usefulness to it.

I have no problems with ripping out the bogus "thread" support once (or even 
before) proper process support is implemented.  Likewise I've no problem 
requiring a recent GDB if you want to do multicore debugging.

Paul

[1] or have no MMU at all.

Re: [Qemu-devel] Re: [RESEND][PATCH] gdbstub: Add vCont support

[Jan Kiszka]

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

Paul Brook wrote:
>>  a) Modeling cpus as processes buys us nothing compared to threads given
>>     the fact that we cannot provide a stable memory mapping to the gdb
>>     frontend anyway. (*)
> 
> I disagree. The process model fits perfectly. The whole point is that each CPU 
> has its own virtual address space. Separate address spaces is the fundamental 
> difference between a process and a thread.
> 
> If you have a multicore system where several cores share a MMU[1] then 
> modelling these as threads probably make sense.
> 
> Don't confuse this with OS awareness in GDB (i.e. implementing a userspace 
> debug environment via a bare metal kernel level debug interface). That's a 
> completely separate issue.

You snipped away my argument under (*):

> (*) Process abstraction is, if used at all, guest business. At best we
> could try to invent (likely OS-specific) heuristics to identify
> identical mappings and call them processes. I don't see a reasonable
> benefit compared to the expected effort and unreliability.

You cannot simply assign some CPU n to a virtual process n because the
mapping you see on that CPU at some point in time may next pop up on
some other CPU - and vice versa. You start to make gdb believe it sees
consistent processes while you have no clue at all about the scheduling
of your guest. So what do you gain?

I can tell you what you loose: If gdb starts to think that there are n
separate processes, you will have to set separate breakpoints as well.
Bad. And if some breakpoint assigned to process n suddenly hits you on
process (CPU) m, only chaos is the result. E.g. kvm would re-inject it
as guest-originated. Even worse.

> 
>>  b) The model is already part of mainline qemu. This patch is just
>>     about adding even more usefulness to it.
> 
> I have no problems with ripping out the bogus "thread" support once (or even 
> before) proper process support is implemented.  Likewise I've no problem 
> requiring a recent GDB if you want to do multicore debugging.

There was zero practical need for this so far. But maybe you can
describe a _concrete_ debugging use case and the related steps in gdb -
based on a potential process interface. I'm surely willing to learn
about it if there is really such a great practical improvement feasible.
What will be the user's benefit?

Jan


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

Re: [Qemu-devel] Re: [RESEND][PATCH] gdbstub: Add vCont support

[Jan Kiszka]

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

Jan Kiszka wrote:
> Paul Brook wrote:
>>>  a) Modeling cpus as processes buys us nothing compared to threads given
>>>     the fact that we cannot provide a stable memory mapping to the gdb
>>>     frontend anyway. (*)
>> I disagree. The process model fits perfectly. The whole point is that each CPU 
>> has its own virtual address space. Separate address spaces is the fundamental 
>> difference between a process and a thread.
>>
>> If you have a multicore system where several cores share a MMU[1] then 
>> modelling these as threads probably make sense.
>>
>> Don't confuse this with OS awareness in GDB (i.e. implementing a userspace 
>> debug environment via a bare metal kernel level debug interface). That's a 
>> completely separate issue.
> 
> You snipped away my argument under (*):
> 
>> (*) Process abstraction is, if used at all, guest business. At best we
>> could try to invent (likely OS-specific) heuristics to identify
>> identical mappings and call them processes. I don't see a reasonable
>> benefit compared to the expected effort and unreliability.
> 
> You cannot simply assign some CPU n to a virtual process n because the
> mapping you see on that CPU at some point in time may next pop up on
> some other CPU - and vice versa. You start to make gdb believe it sees
> consistent processes while you have no clue at all about the scheduling
> of your guest. So what do you gain?
> 
> I can tell you what you loose: If gdb starts to think that there are n
> separate processes, you will have to set separate breakpoints as well.
> Bad. And if some breakpoint assigned to process n suddenly hits you on
> process (CPU) m, only chaos is the result. E.g. kvm would re-inject it
> as guest-originated. Even worse.
> 
>>>  b) The model is already part of mainline qemu. This patch is just
>>>     about adding even more usefulness to it.
>> I have no problems with ripping out the bogus "thread" support once (or even 
>> before) proper process support is implemented.  Likewise I've no problem 
>> requiring a recent GDB if you want to do multicore debugging.
> 
> There was zero practical need for this so far. But maybe you can
> describe a _concrete_ debugging use case and the related steps in gdb -
> based on a potential process interface. I'm surely willing to learn
> about it if there is really such a great practical improvement feasible.
> What will be the user's benefit?

Maybe, if there are OSes in the field where a process model fits (surely
not Linux), we could find the compromise to add alternative support for
that model and let the user decide which one to use via some command
line switch or maintenance command. There are still a few 'ifs' for me
in this equation, including that gdb properly supports the model. But if
they can be removed, I'm surely open for such enhancements!

For now I would be very glad if we could agree on completing the thread
model implementation so that users can benefit from it without having to
patch qemu or kvm.

Jan


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

Re: [Qemu-devel] Re: [RESEND][PATCH] gdbstub: Add vCont support

[Paul Brook]

On Friday 16 January 2009, Jan Kiszka wrote:
> Paul Brook wrote:
> >>  a) Modeling cpus as processes buys us nothing compared to threads given
> >>     the fact that we cannot provide a stable memory mapping to the gdb
> >>     frontend anyway. (*)
> >
> > I disagree. The process model fits perfectly. The whole point is that
> > each CPU has its own virtual address space. Separate address spaces is
> > the fundamental difference between a process and a thread.
> >
> > If you have a multicore system where several cores share a MMU[1] then
> > modelling these as threads probably make sense.
> >
> > Don't confuse this with OS awareness in GDB (i.e. implementing a
> > userspace debug environment via a bare metal kernel level debug
> > interface). That's a completely separate issue.
>
> You snipped away my argument under (*):
> > (*) Process abstraction is, if used at all, guest business. At best we
> > could try to invent (likely OS-specific) heuristics to identify
> > identical mappings and call them processes. I don't see a reasonable
> > benefit compared to the expected effort and unreliability.

You're doing exactly what I said not do do. You are confusing a GDB process 
model (i.e. separate address spaces) with actual OS processes.

The point is that each CPU has its own distinct virtual address space. GDB 
assumes that all threads use the same memory map. To handle these distinct 
address spaces you need to model CPUs as processes. Your thread hack is 
dependent on, and limited to, address ranges that happen to mapped the same 
on all CPUs. This may be sufficient for simple linux kernel debugging, but 
fails very rapidly when you start trying to do anything clever.

> You cannot simply assign some CPU n to a virtual process n because the
> mapping you see on that CPU at some point in time may next pop up on
> some other CPU - and vice versa. You start to make gdb believe it sees
> consistent processes while you have no clue at all about the scheduling
> of your guest. So what do you gain?

Mapping current CPU state to an OS process is the debugger's problem. A 
sufficiently smart debugger is able to interpret the OS process table, and 
match that to the address space that's present on a particular CPU. I don't 
think GDB is capable of doing this right now, but I've seen it done in other 
debuggers.

> I can tell you what you loose: If gdb starts to think that there are n
> separate processes, you will have to set separate breakpoints as well.
> Bad. And if some breakpoint assigned to process n suddenly hits you on
> process (CPU) m, only chaos is the result. E.g. kvm would re-inject it
> as guest-originated. Even worse.

You need to use multi-process breakpoints.

An OS aware debugger will take care of migrating userspace breakpoints when OS 
context switches occur.

> There was zero practical need for this so far. But maybe you can
> describe a _concrete_ debugging use case and the related steps in gdb -
> based on a potential process interface. I'm surely willing to learn
> about it if there is really such a great practical improvement feasible.
> What will be the user's benefit?

In embedded systems it's fairly common to run entirely separate operating 
systems on different cores. e.g. you may run linux on one core and a RTOS or 
dedicated codec engine on the other.

The rest of my examples are hypothetical. I don't have actual examples, 
however it's things that I know people are interested in, and in some cases 
actively working on. GDB already has a python plugin interface, though 
currently there aren't any useful hooks for doingn OS awareness stuff.

I'm not sure offhand how highmem mappings work. Are these per-cpu or global?
My guess is linux makes them global, but I can certainly imagine them being 
per-cpu.

A kernel with a 4g/4g split is a fairly pathological case. Your assumption 
that there is a useful common mapping across all cpus fails and the debugger 
absolutely needs to be able to specify which virtual address space it is 
accessing. A relatively small amount of debugger OS awareness is required to 
figure out whether a particular core is in the kernel or user space, but 
that's not QEMU's problem.

Paul

Re: [Qemu-devel] Re: [RESEND][PATCH] gdbstub: Add vCont support

[Jan Kiszka]

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

Paul Brook wrote:
> On Friday 16 January 2009, Jan Kiszka wrote:
>> Paul Brook wrote:
>>>>  a) Modeling cpus as processes buys us nothing compared to threads given
>>>>     the fact that we cannot provide a stable memory mapping to the gdb
>>>>     frontend anyway. (*)
>>> I disagree. The process model fits perfectly. The whole point is that
>>> each CPU has its own virtual address space. Separate address spaces is
>>> the fundamental difference between a process and a thread.
>>>
>>> If you have a multicore system where several cores share a MMU[1] then
>>> modelling these as threads probably make sense.
>>>
>>> Don't confuse this with OS awareness in GDB (i.e. implementing a
>>> userspace debug environment via a bare metal kernel level debug
>>> interface). That's a completely separate issue.
>> You snipped away my argument under (*):
>>> (*) Process abstraction is, if used at all, guest business. At best we
>>> could try to invent (likely OS-specific) heuristics to identify
>>> identical mappings and call them processes. I don't see a reasonable
>>> benefit compared to the expected effort and unreliability.
> 
> You're doing exactly what I said not do do. You are confusing a GDB process 
> model (i.e. separate address spaces) with actual OS processes.

Well, if gdb can tell them apart... My concern is that gdb's process
model implementation was likely not done with the qemu scenario in mind
(otherwise we would talk about "gdb thread model vs. address space model").

> 
> The point is that each CPU has its own distinct virtual address space. GDB 
> assumes that all threads use the same memory map. To handle these distinct 
> address spaces you need to model CPUs as processes. Your thread hack is 
> dependent on, and limited to, address ranges that happen to mapped the same 
> on all CPUs. This may be sufficient for simple linux kernel debugging, but 
> fails very rapidly when you start trying to do anything clever.

So far it hasn't because the thread model implementation is a bit
smarter than a "hack".

However, you still don't tell us what prevents to _add_ the multiprocess
support to qemu once gdb or some other debugger can handle it according
to our needs. It is an _addon_ feature to the gdb protocol anyway, and
qemu will have to check if the frontend supports it. If not, qemu could
fall back to the thread model to allow at least "basic" debugging and
would not have to tell the frontend "sorry, I'm too smart for you".

> 
>> You cannot simply assign some CPU n to a virtual process n because the
>> mapping you see on that CPU at some point in time may next pop up on
>> some other CPU - and vice versa. You start to make gdb believe it sees
>> consistent processes while you have no clue at all about the scheduling
>> of your guest. So what do you gain?
> 
> Mapping current CPU state to an OS process is the debugger's problem. A 
> sufficiently smart debugger is able to interpret the OS process table, and 
> match that to the address space that's present on a particular CPU. I don't 
> think GDB is capable of doing this right now, but I've seen it done in other 
> debuggers.

So this is still a future requirement on qemu, no?

Let's assume we had multiprocess support implemented in qemu already.
Would I still be able to debug the "corner case" Linux without
additional pain?

> 
>> I can tell you what you loose: If gdb starts to think that there are n
>> separate processes, you will have to set separate breakpoints as well.
>> Bad. And if some breakpoint assigned to process n suddenly hits you on
>> process (CPU) m, only chaos is the result. E.g. kvm would re-inject it
>> as guest-originated. Even worse.
> 
> You need to use multi-process breakpoints.

Does gdb provide them already? Or is there some other free debuggers
that do so?

> 
> An OS aware debugger will take care of migrating userspace breakpoints when OS 
> context switches occur.
> 
>> There was zero practical need for this so far. But maybe you can
>> describe a _concrete_ debugging use case and the related steps in gdb -
>> based on a potential process interface. I'm surely willing to learn
>> about it if there is really such a great practical improvement feasible.
>> What will be the user's benefit?
> 
> In embedded systems it's fairly common to run entirely separate operating 
> systems on different cores. e.g. you may run linux on one core and a RTOS or 
> dedicated codec engine on the other.

Accepted, that can be a valid use case.

Right now the debugger could already handle this scenario /wrt soft
breakpoints by leaving the focus on those CPUs for which it has the code
image.

> 
> The rest of my examples are hypothetical. I don't have actual examples, 
> however it's things that I know people are interested in, and in some cases 
> actively working on. GDB already has a python plugin interface, though 
> currently there aren't any useful hooks for doingn OS awareness stuff.
> 
> I'm not sure offhand how highmem mappings work. Are these per-cpu or global?
> My guess is linux makes them global, but I can certainly imagine them being 
> per-cpu.

highmem is global. I'm not sure if anyone will bother changing this in
the future. Multicore CPU need an increasing amount of memory, thus
generally come with a sufficiently large virtual address spaces.


I admit: If you add OS awareness to the debugger or if you have a fixed
CPU schedule, the multiprocess protocol extension can be useful as it
allows to direct break/watchpoints to certain CPUs.

My suggestion still stands: Add multiprocess support on top of the
thread model, but keep threads as foundation. The former is upcoming
technology while the latter already covers most of todays' kernel
debugging scenarios without problems.

But I do have my problems with providing interfaces _only_ for users
still under development, neglecting what is already possible with
todays' versions. Right now we face a similar issue with x86 due to gdb
not yet being as far as a certain change to gdbstub assumed. I'm working
on this, but it was confirmed by gdb people that there is easy quick
fix. And that currently leaves qemu unusable for certain scenarios
unless you patch it.

So please focus on what is available today - and please let us merge
this patch. vCont is mandatory for the multiprocess feature anyway, just
the thread-id encoding will vary.

Jan


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

Re: [Qemu-devel] Re: [RESEND][PATCH] gdbstub: Add vCont support

[Daniel Jacobowitz]

On Fri, Jan 16, 2009 at 12:15:44AM +0000, Paul Brook wrote:
> >  b) The model is already part of mainline qemu. This patch is just
> >     about adding even more usefulness to it.
> 
> I have no problems with ripping out the bogus "thread" support once (or even 
> before) proper process support is implemented.  Likewise I've no problem 
> requiring a recent GDB if you want to do multicore debugging.

This is a bit of a different case than the x86 32-bit / 64-bit
problem.  The hack is contained entirely in the qemu implementation
and once we have a GDB that supports process model - or specifically
a multi-core model - it should be easy to adapt qemu.  In the mean
time, I don't see a problem with Jan's patch; it's how hardware
debuggers generally present cores to GDB at the present time.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [Qemu-devel] Re: [RESEND][PATCH] gdbstub: Add vCont support

[Jamie Lokier]

Paul Brook wrote:
> >  a) Modeling cpus as processes buys us nothing compared to threads given
> >     the fact that we cannot provide a stable memory mapping to the gdb
> >     frontend anyway. (*)
> 
> I disagree. The process model fits perfectly. The whole point is
> that each CPU has its own virtual address space. Separate address
> spaces is the fundamental difference between a process and a thread.

That isn't so fundamental.

Distinct CPUs in an OS like Linux have most of the kernel address
space shared between them - except when in special modes like 4G.  But
some of the kernel address space is per-CPU.

Threads do not necessarily share the whole virtual address space:
sometimes thread-specific storage is mapped into the same address in
each thread.

In Windows processes, historically it's been possible to have a
mixture of process-specific mapped segments and system-wide shared
segments, which looks more like threads.

Of course there's the no-MMU architectures too.

So the distinction which really matters is, surely, with which model
does GDB behave most usefully with multiple CPUs having their own
MMUs?  Does GDB _assume_ all threads have exactly the same address
space, or does GDB allow for threads which have some thread-local
mappings, and therefore always use the correct thread when examining
memory etc.?

-- Jamie

Re: [Qemu-devel] Re: [RESEND][PATCH] gdbstub: Add vCont support

[Paul Brook]

> So the distinction which really matters is, surely, with which model
> does GDB behave most usefully with multiple CPUs having their own
> MMUs?  Does GDB _assume_ all threads have exactly the same address
> space, or does GDB allow for threads which have some thread-local
> mappings, and therefore always use the correct thread when examining
> memory etc.?

GDB assumes all threads share a common address space.

Paul

[Qemu-devel] [RESEND][PATCH] gdbstub: Add vCont support

[Jan Kiszka]

In order to set the VCPU for the next single-step command, you need gdb
6.8 or better - and this patch. It enhances the existing support for
representing VCPUs as threads to the gdb frontend by introducing the
vCont remote gdb command. This is used by gdb to switch the debugging
focus for single-stepping multi-threaded targets.


There was quite some discussion around this patch in the past, dealing
with the model for presenting VCPU as threads to the gdb front-end. This
patch should be merged nevertheless because

- this patch does not introduce the threading model, it only introduces
  vCont according to the exiting model used by qemu 0.10.x.

- current gdb provides no alternative yet, but we already have lots of
  use cases that are covered by the basic threading model.

- enhancing qemu later on with a true multicore model once gdb supports
  it will not obsolete this patch.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 gdbstub.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 8876c1d..1191dc2 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1570,6 +1570,64 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
             s->signal = 0;
         gdb_continue(s);
         return RS_IDLE;
+    case 'v':
+        if (strncmp(p, "Cont", 4) == 0) {
+            int res_signal, res_thread;
+
+            p += 4;
+            if (*p == '?') {
+                put_packet(s, "vCont;c;C;s;S");
+                break;
+            }
+            res = 0;
+            res_signal = 0;
+            res_thread = 0;
+            while (*p) {
+                int action, signal;
+
+                if (*p++ != ';') {
+                    res = 0;
+                    break;
+                }
+                action = *p++;
+                signal = 0;
+                if (action == 'C' || action == 'S')
+                    signal = strtoul(p, (char **)&p, 16);
+                else if (action != 'c' && action != 's') {
+                    res = 0;
+                    break;
+                }
+                thread = 0;
+                if (*p == ':')
+                    thread = strtoull(p+1, (char **)&p, 16);
+
+                action = tolower(action);
+                if (res == 0 || (res == 'c' && action == 's')) {
+                    res = action;
+                    res_signal = signal;
+                    res_thread = thread;
+                }
+            }
+            if (res) {
+                if (res_thread != -1 && res_thread != 0) {
+                    for (env = first_cpu; env != NULL; env = env->next_cpu)
+                        if (env->cpu_index + 1 == res_thread)
+                            break;
+                    if (env == NULL) {
+                        put_packet(s, "E22");
+                        break;
+                    }
+                    s->c_cpu = env;
+                }
+                if (res == 's')
+                    cpu_single_step(s->c_cpu, sstep_flags);
+                s->signal = res_signal;
+                gdb_continue(s);
+                return RS_IDLE;
+            }
+            break;
+        } else
+            goto unknown_command;
     case 'k':
         /* Kill the target */
         fprintf(stderr, "\nQEMU: Terminated via GDBstub\n");