| Message ID | 20240216130817.369377-2-iii@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | gdbstub: Implement follow-fork-mode child | expand |
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>
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~
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~
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(). [...]
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(); } /*
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(-)