1. Patchew
  2. QEMU
  3. [PATCH V10 0/8] Add architecture agnostic code to support vCPU Hotplug
  4. View patch

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

Salil Mehta via posted 8 patches 1 year, 8 months ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <anisinha@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Yanan Wang <wangyanan55@huawei.com>, Richard Henderson <richard.henderson@linaro.org>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>
There is a newer version of this series
[PATCH V10 7/8] gdbstub: Add helper function to unregister GDB register space
Posted by Salil Mehta via 1 year, 8 months ago 
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);
     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
-- 
2.34.1
Re: [PATCH V10 7/8] gdbstub: Add helper function to unregister GDB register space
Posted by Alex Bennée 1 year, 8 months ago 
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.

>      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
RE: [PATCH V10 7/8] gdbstub: Add helper function to unregister GDB register space
Posted by Salil Mehta via 1 year, 8 months ago 
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
Re: [PATCH V10 7/8] gdbstub: Add helper function to unregister GDB register space
Posted by Alex Bennée 1 year, 8 months ago 
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
RE: [PATCH V10 7/8] gdbstub: Add helper function to unregister GDB register space
Posted by Salil Mehta via 1 year, 8 months ago 
>  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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.