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

[Salil Mehta]

>  From: Alex Bennée <alex.bennee@linaro.org>
>  Sent: Tuesday, May 21, 2024 4:23 PM
>  To: Salil Mehta <salil.mehta@huawei.com>
>  
>  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.
>  

Ok.

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

Sure, I will also add it in the header of this patch. Though, I did mention
about such dependency in the cover letter for this entire series.


Pasting from the cover-letter:

[*] https://github.com/salil-mehta/qemu.git 
virt-cpuhp-armv8/rfc-v3.arch.agnostic.v10

NOTE: For ARM, above will work in combination of the architecture specific part 
based on
RFC V2 [1]. This architecture specific patch-set RFC V3 shall be floated soon 
and is present
at below location

[*] https://github.com/salil-mehta/qemu/tree/virt-cpuhp-armv8/rfc-v3-rc1


Thanks
Salil.

>  
>  --
>  Alex Bennée
>  Virtualisation Tech Lead @ Linaro