RE: [PATCH V10 7/8] gdbstub: Add helper function to unregister GDB register space

[Salil Mehta via <qemu-devel@nongnu.org>]

Hi Alex,

>  From: Alex Bennée <alex.bennee@linaro.org>
>  Sent: Tuesday, May 21, 2024 1:45 PM
>  To: Salil Mehta <salil.mehta@huawei.com>
>  
>  Salil Mehta <salil.mehta@huawei.com> writes:
>  
>  > Add common function to help unregister the GDB register space. This
>  > shall be done in context to the CPU unrealization.
>  >
>  > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>  > Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
>  > Reviewed-by: Gavin Shan <gshan@redhat.com>
>  > Tested-by: Xianglai Li <lixianglai@loongson.cn>
>  > Tested-by: Miguel Luis <miguel.luis@oracle.com>
>  > Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
>  > Reviewed-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
>  > ---
>  >  gdbstub/gdbstub.c      | 13 +++++++++++++
>  >  hw/core/cpu-common.c   |  1 -
>  >  include/exec/gdbstub.h |  6 ++++++
>  >  3 files changed, 19 insertions(+), 1 deletion(-)
>  >
>  > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c index
>  > b3574997ea..1949b09240 100644
>  > --- a/gdbstub/gdbstub.c
>  > +++ b/gdbstub/gdbstub.c
>  > @@ -617,6 +617,19 @@ void gdb_register_coprocessor(CPUState *cpu,
>  >      }
>  >  }
>  >
>  > +void gdb_unregister_coprocessor_all(CPUState *cpu) {
>  > +    /*
>  > +     * Safe to nuke everything. GDBRegisterState::xml is static const char
>  so
>  > +     * it won't be freed
>  > +     */
>  > +    g_array_free(cpu->gdb_regs, true);
>  > +
>  > +    cpu->gdb_regs = NULL;
>  > +    cpu->gdb_num_regs = 0;
>  > +    cpu->gdb_num_g_regs = 0;
>  > +}
>  > +
>  >  static void gdb_process_breakpoint_remove_all(GDBProcess *p)  {
>  >      CPUState *cpu = gdb_get_first_cpu_in_process(p); diff --git
>  > a/hw/core/cpu-common.c b/hw/core/cpu-common.c index
>  > 0f0a247f56..e5140b4bc1 100644
>  > --- a/hw/core/cpu-common.c
>  > +++ b/hw/core/cpu-common.c
>  > @@ -274,7 +274,6 @@ static void cpu_common_finalize(Object *obj)  {
>  >      CPUState *cpu = CPU(obj);
>  >
>  > -    g_array_free(cpu->gdb_regs, TRUE);
>  
>  Is this patch missing something? As far as I can tell the new function never
>  gets called.


Above was causing double free because eventually this free'ng of 'gdb_regs' is being
done in context to un-realization of ARM CPU. Function ' gdb_unregister_coprocessor_all'
will be used by loongson arch as well. Hence, I placed this newly added function
in the arch agnostic patch-set 

https://lore.kernel.org/qemu-devel/20230926103654.34424-1-salil.mehta@huawei.com/

Another approach could be to keep it but make above free'ing as conditional?

/* in case architecture specific code did not do its job */
if (cpu->gdb_regs)
    g_array_free(cpu->gdb_regs, TRUE);


Best regards
Salil.


>  
>  >      qemu_lockcnt_destroy(&cpu->in_ioctl_lock);
>  >      qemu_mutex_destroy(&cpu->work_mutex);
>  >  }
>  > diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h index
>  > eb14b91139..249d4d4bc8 100644
>  > --- a/include/exec/gdbstub.h
>  > +++ b/include/exec/gdbstub.h
>  > @@ -49,6 +49,12 @@ void gdb_register_coprocessor(CPUState *cpu,
>  >                                gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
>  >                                const GDBFeature *feature, int g_pos);
>  >
>  > +/**
>  > + * gdb_unregister_coprocessor_all() - unregisters supplemental set of
>  > +registers
>  > + * @cpu - the CPU associated with registers  */ void
>  > +gdb_unregister_coprocessor_all(CPUState *cpu);
>  > +
>  >  /**
>  >   * gdbserver_start: start the gdb server
>  >   * @port_or_device: connection spec for gdb
>  
>  --
>  Alex Bennée
>  Virtualisation Tech Lead @ Linaro