Re: [PATCH v2 3/7] hw/core/cpu-common: Don't init gdbstub until cpu_exec_realizefn()

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Tue, Apr 29, 2025 at 02:21:56PM +0100, Peter Maydell wrote:
> Currently we call gdb_init_cpu() in cpu_common_initfn(), which is
> very early in the CPU object's init->realize creation sequence.  In
> particular this happens before the architecture-specific subclass's
> init fn has even run.  This means that gdb_init_cpu() can only do
> things that depend strictly on the class, not on the object, because
> the CPUState* that it is passed is currently half-initialized.
> 
> In commit a1f728ecc90cf6c6 we accidentally broke this rule, by adding
> a call to the gdb_get_core_xml_file method which takes the CPUState.
> At the moment we get away with this because the only implementation
> doesn't actually look at the pointer it is passed.  However the whole
> reason we created that method was so that we could make the "which
> XML file?" decision based on a property of the CPU object, and we
> currently can't change the Arm implementation of the method to do
> what we want without causing wrong behaviour or a crash.
> 
> The ordering restrictions here are:
>  * we must call gdb_init_cpu before:
>    - any call to gdb_register_coprocessor()
>    - any use of the gdb_num_regs field (this is only used
>      in code that's about to call gdb_register_coprocessor()
>      and wants to know the first register number of the
>      set of registers it's about to add)
>  * we must call gdb_init_cpu after CPU properties have been
>    set, which is to say somewhere in realize
> 
> The function cpu_exec_realizefn() meets both of these requirements,
> as it is called by the architecture-specific CPU realize function
> early in realize, before any calls ot gdb_register_coprocessor().
> Move the gdb_init_cpu() call to there.

Sounds good to me:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>



> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/core/cpu-common.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index 92c40b6bf83..39e674aca21 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -234,6 +234,8 @@ bool cpu_exec_realizefn(CPUState *cpu, Error **errp)
>          return false;
>      }
>  
> +    gdb_init_cpu(cpu);
> +
>      /* Wait until cpu initialization complete before exposing cpu. */
>      cpu_list_add(cpu);
>  
> @@ -304,7 +306,6 @@ static void cpu_common_initfn(Object *obj)
>      /* cache the cpu class for the hotpath */
>      cpu->cc = CPU_GET_CLASS(cpu);
>  
> -    gdb_init_cpu(cpu);
>      cpu->cpu_index = UNASSIGNED_CPU_INDEX;
>      cpu->cluster_index = UNASSIGNED_CLUSTER_INDEX;
>      cpu->as = NULL;
> -- 
> 2.43.0
>