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

[Alex Bennée]

Salil Mehta <salil.mehta@huawei.com> writes:

> 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);

No I don't object to moving it to a function. But I would expect the
patch that adds the function and plumbs it in to also be the patch that
removes the inline call. Otherwise the tree will be broken in behaviour
between patches.

Just make it clear in the header that the series needs the pre-requisite
patches.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro