Re: [PATCH 02/10] gdbstub: fix next register base after register gaps

["Alex Bennée" <alex.bennee@linaro.org>]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Commit b3e88abb200c ("gdbstub: Consider GDBFeature::base_reg in
> gdb_register_coprocessor()") identified that register numbers may
> have gaps. We also need to adjust the next base_reg correctly
> to avoid overlapping registers. Rename the field to be more
> explicit about its usage.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Acked-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  gdbstub/gdbstub.c      | 13 ++++++-------
>  include/hw/core/cpu.h  |  2 +-
>  target/arm/gdbstub.c   |  8 +++++---
>  target/arm/gdbstub64.c | 10 ++++++----
>  target/ppc/gdbstub.c   |  2 +-
>  target/riscv/gdbstub.c |  9 ++++++---
>  target/xtensa/cpu.c    |  2 +-
>  7 files changed, 26 insertions(+), 20 deletions(-)
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index 0a328b0dd488..0ce871d74929 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -605,13 +605,13 @@ void gdb_init_cpu(CPUState *cpu)
>          gdb_register_feature(cpu, 0,
>                               cc->gdb_read_register, cc->gdb_write_register,
>                               feature);
> -        cpu->gdb_num_regs = cpu->gdb_num_g_regs = feature->num_regs;
> +        cpu->gdb_next_base_reg = cpu->gdb_num_g_regs = feature->num_regs;
>      } else {
> -        cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
> +        cpu->gdb_next_base_reg = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
>      }
>  
>      trace_gdbxml_init_cpu(object_get_typename(OBJECT(cpu)), cpu->cpu_index,
> -                          cpu->gdb_num_regs, cpu->gdb_num_g_regs,
> +                          cpu->gdb_next_base_reg, cpu->gdb_num_g_regs,
>                            cc->gdb_num_core_regs);
>  }
>  
> @@ -621,7 +621,7 @@ void gdb_register_coprocessor(CPUState *cpu,
>  {
>      GDBRegisterState *s;
>      guint i;
> -    int base_reg = cpu->gdb_num_regs;
> +    int base_reg = cpu->gdb_next_base_reg;
>  
>      for (i = 0; i < cpu->gdb_regs->len; i++) {
>          /* Check for duplicates.  */
> @@ -639,7 +639,7 @@ void gdb_register_coprocessor(CPUState *cpu,
>      gdb_register_feature(cpu, base_reg, get_reg, set_reg, feature);
>  
>      /* Add to end of list.  */
> -    cpu->gdb_num_regs += feature->num_regs;
> +    cpu->gdb_next_base_reg = base_reg + feature->num_regs;
>  }
>  
>  void gdb_unregister_coprocessor_all(CPUState *cpu)
> @@ -651,7 +651,7 @@ void gdb_unregister_coprocessor_all(CPUState *cpu)
>      g_array_free(cpu->gdb_regs, true);
>  
>      cpu->gdb_regs = NULL;
> -    cpu->gdb_num_regs = 0;
> +    cpu->gdb_next_base_reg = 0;
>      cpu->gdb_num_g_regs = 0;
>  }
>  
> @@ -2522,4 +2522,3 @@ void gdb_create_default_process(GDBState *s)
>      process->attached = false;
>      process->target_xml = NULL;
>  }
> -
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 328e30045ce1..02212f555c2e 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -521,7 +521,7 @@ struct CPUState {
>      struct CPUJumpCache *tb_jmp_cache;
>  
>      GArray *gdb_regs;
> -    int gdb_num_regs;
> +    int gdb_next_base_reg;
>      int gdb_num_g_regs;
>      QTAILQ_ENTRY(CPUState) node;
>  
> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
> index d6e29c4cf467..e5cbe4d951ef 100644
> --- a/target/arm/gdbstub.c
> +++ b/target/arm/gdbstub.c
> @@ -555,19 +555,21 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
>          gdb_register_coprocessor(cs, mve_gdb_get_reg, mve_gdb_set_reg,
>                                   gdb_find_static_feature("arm-m-profile-mve.xml"));
>      }
> +    GDBFeature *feature =
> +        arm_gen_dynamic_sysreg_feature(cs, cs->gdb_next_base_reg);
>      gdb_register_coprocessor(cs, arm_gdb_get_sysreg, arm_gdb_set_sysreg,
> -                             arm_gen_dynamic_sysreg_feature(cs, cs->gdb_num_regs));
> +                             feature);
>  
>  #ifdef CONFIG_TCG
>      if (arm_feature(env, ARM_FEATURE_M) && tcg_enabled()) {
>          gdb_register_coprocessor(cs,
>              arm_gdb_get_m_systemreg, arm_gdb_set_m_systemreg,
> -            arm_gen_dynamic_m_systemreg_feature(cs, cs->gdb_num_regs));
> +            arm_gen_dynamic_m_systemreg_feature(cs, cs->gdb_next_base_reg));
>  #ifndef CONFIG_USER_ONLY
>          if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
>              gdb_register_coprocessor(cs,
>                  arm_gdb_get_m_secextreg, arm_gdb_set_m_secextreg,
> -                arm_gen_dynamic_m_secextreg_feature(cs, cs->gdb_num_regs));
> +                arm_gen_dynamic_m_secextreg_feature(cs, cs->gdb_next_base_reg));
>          }
>  #endif
>      }
> diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
> index 0c3e5b30bd6a..cf5b95b54641 100644
> --- a/target/arm/gdbstub64.c
> +++ b/target/arm/gdbstub64.c
> @@ -885,7 +885,8 @@ void aarch64_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
>      CPUState *cs = CPU(cpu);
>      if (isar_feature_aa64_sve(&cpu->isar) ||
>          isar_feature_aa64_sme(&cpu->isar)) {
> -        GDBFeature *feature = arm_gen_dynamic_svereg_feature(cs, cs->gdb_num_regs);
> +        GDBFeature *feature =
> +            arm_gen_dynamic_svereg_feature(cs, cs->gdb_next_base_reg);
>          gdb_register_coprocessor(cs, aarch64_gdb_get_sve_reg,
>                                   aarch64_gdb_set_sve_reg, feature);
>      } else {
> @@ -896,7 +897,7 @@ void aarch64_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
>  
>      if (isar_feature_aa64_sme(&cpu->isar)) {
>          GDBFeature *sme_feature =
> -            arm_gen_dynamic_smereg_feature(cs, cs->gdb_num_regs);
> +            arm_gen_dynamic_smereg_feature(cs, cs->gdb_next_base_reg);
>          gdb_register_coprocessor(cs, aarch64_gdb_get_sme_reg,
>                                   aarch64_gdb_set_sme_reg, sme_feature);
>          if (isar_feature_aa64_sme2(&cpu->isar)) {
> @@ -927,7 +928,8 @@ void aarch64_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
>  #endif
>  
>      /* All AArch64 CPUs have at least TPIDR */
> +    GDBFeature *tls_feature =
> +        arm_gen_dynamic_tls_feature(cs, cs->gdb_next_base_reg);
>      gdb_register_coprocessor(cs, aarch64_gdb_get_tls_reg,
> -                             aarch64_gdb_set_tls_reg,
> -                             arm_gen_dynamic_tls_feature(cs, cs->gdb_num_regs));
> +                             aarch64_gdb_set_tls_reg, tls_feature);
>  }
> diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
> index 4d622c5cad59..ec25c541c685 100644
> --- a/target/ppc/gdbstub.c
> +++ b/target/ppc/gdbstub.c
> @@ -206,7 +206,7 @@ static void gdb_gen_spr_feature(CPUState *cs)
>  
>      gdb_feature_builder_init(&builder, &pcc->gdb_spr,
>                               "org.qemu.power.spr", "power-spr.xml",
> -                             cs->gdb_num_regs);
> +                             cs->gdb_next_base_reg);
>  
>      for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
>          ppc_spr_t *spr = &env->spr_cb[i];
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index 9abbf5bcdf0e..f279f4ab2461 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -348,9 +348,10 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
>                                   gdb_find_static_feature("riscv-32bit-fpu.xml"));
>      }
>      if (cpu->cfg.ext_zve32x) {
> +        GDBFeature *feature =
> +            ricsv_gen_dynamic_vector_feature(cs, cs->gdb_next_base_reg);
>          gdb_register_coprocessor(cs, riscv_gdb_get_vector,
> -                                 riscv_gdb_set_vector,
> -                                 ricsv_gen_dynamic_vector_feature(cs, cs->gdb_num_regs));
> +                                 riscv_gdb_set_vector, feature);
>      }
>  
>  #ifdef CONFIG_TCG
> @@ -373,8 +374,10 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
>      }
>  
>      if (cpu->cfg.ext_zicsr) {
> +        GDBFeature *feature =
> +            riscv_gen_dynamic_csr_feature(cs, cs->gdb_next_base_reg);
>          gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
> -                                 riscv_gen_dynamic_csr_feature(cs, cs->gdb_num_regs));
> +                                 feature);
>      }
>  #endif
>  }
> diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
> index 7c25b9ab707a..48863a1859d4 100644
> --- a/target/xtensa/cpu.c
> +++ b/target/xtensa/cpu.c
> @@ -263,7 +263,7 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    cs->gdb_num_regs = xcc->config->gdb_regmap.num_regs;
> +    cs->gdb_next_base_reg = xcc->config->gdb_regmap.num_regs;
>  
>      qemu_init_vcpu(cs);

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro