Re: [PATCH v2 04/10] gdbstub: add gdb_register_internal_coprocessor()

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

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

> Add support for registering coprocessor features that are only visible
> to QEMU's internal APIs (HMP monitor $register expressions, plugins) but
> hidden from GDB's target description XML.
>
> When a feature is registered as internal:
> - It is excluded from the target.xml sent to GDB
> - Its XML is not served via qXfer:features:read
> - 'p'/'P' packets for internal register numbers return E14
>
> This is useful for exposing system registers to the HMP monitor without
> affecting GDB's register namespace. Exposing aliases or synthetic
> registers through GDB's target description can cause problems: name
> collisions with GDB's pseudo-registers, independent register caching,
> and missing DWARF unwind rules.

Hmm are they *that* useful for HMP then?

The front-ends do limit what they export to gdb (see for example
ARM_CP_NO_GDB) generally because reading them causes side-effects. Those
restrictions are also a reason to limit them from the HMP interface.

>
> Internal features must be registered after all GDB-visible features to
> keep remote register numbering consistent. This ordering is enforced
> with an assertion.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  gdbstub/gdbstub.c      | 71 ++++++++++++++++++++++++++++++++++++++++++++------
>  include/exec/gdbstub.h | 21 +++++++++++++++
>  2 files changed, 84 insertions(+), 8 deletions(-)
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index 912e31b66744..ac0f76d62333 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -54,6 +54,7 @@ typedef struct GDBRegisterState {
>      gdb_get_reg_cb get_reg;
>      gdb_set_reg_cb set_reg;
>      const GDBFeature *feature;
> +    bool internal; /* hidden from GDB, accessible via API/monitor */
>  } GDBRegisterState;
>  
>  GDBState gdbserver_state;
> @@ -385,6 +386,11 @@ static const char *get_feature_xml(const char *p, const char **newp,
>              }
>              for (guint i = 0; i < cpu->gdb_regs->len; i++) {
>                  r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
> +                /* The core target.xml feature must never be internal. */
> +                g_assert(i > 0 || !r->internal);
> +                if (r->internal) {
> +                    continue;
> +                }
>                  g_ptr_array_add(
>                      xml,
>                      g_markup_printf_escaped("<xi:include href=\"%s\"/>",
> @@ -400,6 +406,9 @@ static const char *get_feature_xml(const char *p, const char **newp,
>      /* Is it one of the features? */
>      for (guint i = 0; i < cpu->gdb_regs->len; i++) {
>          r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
> +        if (r->internal) {
> +            continue;
> +        }
>          if (strncmp(p, r->feature->xmlname, len) == 0) {
>              return r->feature->xml;
>          }
> @@ -563,13 +572,14 @@ int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
>  
>  static void gdb_register_feature(CPUState *cpu, int base_reg,
>                                   gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
> -                                 const GDBFeature *feature)
> +                                 const GDBFeature *feature, bool internal)
>  {
>      GDBRegisterState s = {
>          .base_reg = base_reg,
>          .get_reg = get_reg,
>          .set_reg = set_reg,
> -        .feature = feature
> +        .feature = feature,
> +        .internal = internal
>      };
>  
>      trace_gdbxml_register_feature(feature->name, feature->xmlname,
> @@ -605,7 +615,7 @@ void gdb_init_cpu(CPUState *cpu)
>          assert(feature->base_reg == 0);
>          gdb_register_feature(cpu, 0,
>                               cc->gdb_read_register, cc->gdb_write_register,
> -                             feature);
> +                             feature, false);
>          cpu->gdb_next_base_reg = cpu->gdb_num_g_regs = feature->num_regs;
>      } else {
>          cpu->gdb_next_base_reg = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
> @@ -616,20 +626,26 @@ void gdb_init_cpu(CPUState *cpu)
>                            cc->gdb_num_core_regs);
>  }
>  
> -void gdb_register_coprocessor(CPUState *cpu,
> -                              gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
> -                              const GDBFeature *feature)
> +static void do_gdb_register_coprocessor(CPUState *cpu,
> +                                        gdb_get_reg_cb get_reg,
> +                                        gdb_set_reg_cb set_reg,
> +                                        const GDBFeature *feature,
> +                                        bool internal)
>  {
>      GDBRegisterState *s;
>      guint i;
>      int base_reg = cpu->gdb_next_base_reg;
>  
>      for (i = 0; i < cpu->gdb_regs->len; i++) {
> -        /* Check for duplicates.  */
>          s = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
> +        /* Check for duplicates.  */
>          if (s->feature == feature) {
>              return;
>          }
> +        /* GDB-visible features must not follow internal ones.  */
> +        if (!internal) {
> +            g_assert(!s->internal);
> +        }
>      }
>  
>      if (base_reg < feature->base_reg) {
> @@ -637,12 +653,27 @@ void gdb_register_coprocessor(CPUState *cpu,
>                                                feature->base_reg);
>          base_reg = feature->base_reg;
>      }
> -    gdb_register_feature(cpu, base_reg, get_reg, set_reg, feature);
> +    gdb_register_feature(cpu, base_reg, get_reg, set_reg, feature, internal);
>  
>      /* Add to end of list.  */
>      cpu->gdb_next_base_reg = base_reg + feature->num_regs;
>  }
>  
> +void gdb_register_coprocessor(CPUState *cpu,
> +                              gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
> +                              const GDBFeature *feature)
> +{
> +    do_gdb_register_coprocessor(cpu, get_reg, set_reg, feature, false);
> +}
> +
> +void gdb_register_internal_coprocessor(CPUState *cpu,
> +                                       gdb_get_reg_cb get_reg,
> +                                       gdb_set_reg_cb set_reg,
> +                                       const GDBFeature *feature)
> +{
> +    do_gdb_register_coprocessor(cpu, get_reg, set_reg, feature, true);
> +}
> +
>  void gdb_unregister_coprocessor_all(CPUState *cpu)
>  {
>      /*
> @@ -1223,6 +1254,18 @@ static void handle_remove_bp(GArray *params, void *user_ctx)
>   * the remote gdb to fallback to older methods.
>   */
>  
> +static bool gdb_register_is_internal(CPUState *cpu, int reg)
> +{
> +    for (guint i = 0; i < cpu->gdb_regs->len; i++) {
> +        GDBRegisterState *r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
> +
> +        if (r->base_reg <= reg && reg < r->base_reg + r->feature->num_regs) {
> +            return r->internal;
> +        }
> +    }
> +    return false;
> +}
> +
>  static void handle_set_reg(GArray *params, void *user_ctx)
>  {
>      int reg_size;
> @@ -1232,6 +1275,12 @@ static void handle_set_reg(GArray *params, void *user_ctx)
>          return;
>      }
>  
> +    if (gdb_register_is_internal(gdbserver_state.g_cpu,
> +                                 gdb_get_cmd_param(params, 0)->val_ull)) {
> +        gdb_put_packet("E14");
> +        return;
> +    }
> +
>      reg_size = strlen(gdb_get_cmd_param(params, 1)->data) / 2;
>      gdb_hextomem(gdbserver_state.mem_buf, gdb_get_cmd_param(params, 1)->data, reg_size);
>      gdb_write_register(gdbserver_state.g_cpu, gdbserver_state.mem_buf->data,
> @@ -1248,6 +1297,12 @@ static void handle_get_reg(GArray *params, void *user_ctx)
>          return;
>      }
>  
> +    if (gdb_register_is_internal(gdbserver_state.g_cpu,
> +                                 gdb_get_cmd_param(params, 0)->val_ull)) {
> +        gdb_put_packet("E14");
> +        return;
> +    }
> +
>      reg_size = gdb_read_register(gdbserver_state.g_cpu,
>                                   gdbserver_state.mem_buf,
>                                   gdb_get_cmd_param(params, 0)->val_ull);
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index 75eb4d9c365e..1bc778324758 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -40,6 +40,27 @@ void gdb_register_coprocessor(CPUState *cpu,
>                                gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
>                                const GDBFeature *feature);
>  
> +/**
> + * gdb_register_internal_coprocessor() - register internal registers
> + *
> + * Like gdb_register_coprocessor(), but the feature is NOT exposed to
> + * GDB via the target description XML. The registers are only
> + * accessible through QEMU's internal APIs (HMP monitor ``$register``
> + * expressions via gdb_get_register_list()).
> + *
> + * Internal features must be registered after all GDB-visible features
> + * to avoid register numbering mismatches with GDB.
> + *
> + * @cpu - the CPU associated with registers
> + * @get_reg - get function
> + * @set_reg - set function
> + * @feature - the GDBFeature describing the registers
> + */
> +void gdb_register_internal_coprocessor(CPUState *cpu,
> +                                       gdb_get_reg_cb get_reg,
> +                                       gdb_set_reg_cb set_reg,
> +                                       const GDBFeature *feature);
> +
>  /**
>   * gdb_unregister_coprocessor_all() - unregisters supplemental set of registers
>   * @cpu - the CPU associated with registers

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro