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 <i...@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.hender...@linaro.org>
>
>
> r~
