1. Patchew
  2. QEMU
  3. [PATCH v3 00/11] gdbstub: Implement follow-fork-mode child
  4. View patch

[PATCH v3 01/11] gdbstub: Support disablement in a multi-threaded process

Ilya Leoshkevich posted 11 patches 8 months, 2 weeks ago
Maintainers: Warner Losh <imp@bsdimp.com>, Kyle Evans <kevans@freebsd.org>, "Alex Bennée" <alex.bennee@linaro.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Laurent Vivier <laurent@vivier.eu>
There is a newer version of this series
[PATCH v3 01/11] gdbstub: Support disablement in a multi-threaded process
Posted by Ilya Leoshkevich 8 months, 2 weeks ago 
The upcoming follow-fork-mode child support will require disabling
gdbstub in the parent process, which may have multiple threads (which
are represented as CPUs).

Loop over all CPUs in order to remove breakpoints and disable
single-step. Move the respective code into a separate function.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 gdbstub/user.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/gdbstub/user.c b/gdbstub/user.c
index 14918d1a217..e17f7ece908 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -356,16 +356,27 @@ int gdbserver_start(const char *port_or_path)
     return -1;
 }
 
+static void disable_gdbstub(void)
+{
+    CPUState *cpu;
+
+    close(gdbserver_user_state.fd);
+    gdbserver_user_state.fd = -1;
+    CPU_FOREACH(cpu) {
+        cpu_breakpoint_remove_all(cpu, BP_GDB);
+        /* no cpu_watchpoint_remove_all for user-mode */
+        cpu_single_step(cpu, 0);
+        tb_flush(cpu);
+    }
+}
+
 /* Disable gdb stub for child processes.  */
 void gdbserver_fork(CPUState *cpu)
 {
     if (!gdbserver_state.init || gdbserver_user_state.fd < 0) {
         return;
     }
-    close(gdbserver_user_state.fd);
-    gdbserver_user_state.fd = -1;
-    cpu_breakpoint_remove_all(cpu, BP_GDB);
-    /* no cpu_watchpoint_remove_all for user-mode */
+    disable_gdbstub();
 }
 
 /*
-- 
2.43.0
Re: [PATCH v3 01/11] gdbstub: Support disablement in a multi-threaded process
Posted by Richard Henderson 8 months, 2 weeks ago 
On 2/16/24 03:05, Ilya Leoshkevich wrote:
> The upcoming follow-fork-mode child support will require disabling
> gdbstub in the parent process, which may have multiple threads (which
> are represented as CPUs).
> 
> Loop over all CPUs in order to remove breakpoints and disable
> single-step. Move the respective code into a separate function.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   gdbstub/user.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/gdbstub/user.c b/gdbstub/user.c
> index 14918d1a217..e17f7ece908 100644
> --- a/gdbstub/user.c
> +++ b/gdbstub/user.c
> @@ -356,16 +356,27 @@ int gdbserver_start(const char *port_or_path)
>       return -1;
>   }
>   
> +static void disable_gdbstub(void)
> +{
> +    CPUState *cpu;
> +
> +    close(gdbserver_user_state.fd);
> +    gdbserver_user_state.fd = -1;
> +    CPU_FOREACH(cpu) {
> +        cpu_breakpoint_remove_all(cpu, BP_GDB);
> +        /* no cpu_watchpoint_remove_all for user-mode */
> +        cpu_single_step(cpu, 0);
> +        tb_flush(cpu);

You only need to flush once.  The cpu argument is used to determine if we can perform the 
flush immediately or need to queue it.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~
Re: [PATCH v3 01/11] gdbstub: Support disablement in a multi-threaded process
Posted by Ilya Leoshkevich 8 months, 2 weeks ago 
On Sat, 2024-02-17 at 10:21 -1000, Richard Henderson wrote:
> On 2/16/24 03:05, Ilya Leoshkevich wrote:
> > The upcoming follow-fork-mode child support will require disabling
> > gdbstub in the parent process, which may have multiple threads
> > (which
> > are represented as CPUs).
> > 
> > Loop over all CPUs in order to remove breakpoints and disable
> > single-step. Move the respective code into a separate function.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >   gdbstub/user.c | 19 +++++++++++++++----
> >   1 file changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/gdbstub/user.c b/gdbstub/user.c
> > index 14918d1a217..e17f7ece908 100644
> > --- a/gdbstub/user.c
> > +++ b/gdbstub/user.c
> > @@ -356,16 +356,27 @@ int gdbserver_start(const char *port_or_path)
> >       return -1;
> >   }
> >   
> > +static void disable_gdbstub(void)
> > +{
> > +    CPUState *cpu;
> > +
> > +    close(gdbserver_user_state.fd);
> > +    gdbserver_user_state.fd = -1;
> > +    CPU_FOREACH(cpu) {
> > +        cpu_breakpoint_remove_all(cpu, BP_GDB);
> > +        /* no cpu_watchpoint_remove_all for user-mode */
> > +        cpu_single_step(cpu, 0);
> > +        tb_flush(cpu);
> 
> You only need to flush once.  The cpu argument is used to determine
> if we can perform the 
> flush immediately or need to queue it.

I thought we needed to flush jump caches on all CPUs, but I see now
that do_tb_flush() already does this, so this loop is unnecessarily
quadratic.

Btw, shouldn't do_tb_flush() have cpu as a local variable, and not as
a parameter?

> 
> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> 
> r~
Re: [PATCH v3 01/11] gdbstub: Support disablement in a multi-threaded process
Posted by Ilya Leoshkevich 8 months, 2 weeks ago 
On Mon, 2024-02-19 at 14:05 +0100, Ilya Leoshkevich wrote:
> On Sat, 2024-02-17 at 10:21 -1000, Richard Henderson wrote:
> > On 2/16/24 03:05, Ilya Leoshkevich wrote:
> > > The upcoming follow-fork-mode child support will require
> > > disabling
> > > gdbstub in the parent process, which may have multiple threads
> > > (which
> > > are represented as CPUs).
> > > 
> > > Loop over all CPUs in order to remove breakpoints and disable
> > > single-step. Move the respective code into a separate function.
> > > 
> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > ---
> > >   gdbstub/user.c | 19 +++++++++++++++----
> > >   1 file changed, 15 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/gdbstub/user.c b/gdbstub/user.c
> > > index 14918d1a217..e17f7ece908 100644
> > > --- a/gdbstub/user.c
> > > +++ b/gdbstub/user.c
> > > @@ -356,16 +356,27 @@ int gdbserver_start(const char
> > > *port_or_path)
> > >       return -1;
> > >   }
> > >   
> > > +static void disable_gdbstub(void)
> > > +{
> > > +    CPUState *cpu;
> > > +
> > > +    close(gdbserver_user_state.fd);
> > > +    gdbserver_user_state.fd = -1;
> > > +    CPU_FOREACH(cpu) {
> > > +        cpu_breakpoint_remove_all(cpu, BP_GDB);
> > > +        /* no cpu_watchpoint_remove_all for user-mode */
> > > +        cpu_single_step(cpu, 0);
> > > +        tb_flush(cpu);
> > 
> > You only need to flush once.  The cpu argument is used to determine
> > if we can perform the 
> > flush immediately or need to queue it.
> 
> I thought we needed to flush jump caches on all CPUs, but I see now
> that do_tb_flush() already does this, so this loop is unnecessarily
> quadratic.
> 
> Btw, shouldn't do_tb_flush() have cpu as a local variable, and not as
> a parameter?

Never mind, the dummy parameter is needed for usage with
async_safe_run_on_cpu().


[...]
Re: [PATCH v3 01/11] gdbstub: Support disablement in a multi-threaded process
Posted by Alex Bennée 8 months, 2 weeks ago 
Ilya Leoshkevich <iii@linux.ibm.com> writes:

> The upcoming follow-fork-mode child support will require disabling
> gdbstub in the parent process, which may have multiple threads (which
> are represented as CPUs).
>
> Loop over all CPUs in order to remove breakpoints and disable
> single-step. Move the respective code into a separate function.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.