Re: [PATCH v2] target/arm/cpu64: Use 32-bit GDBstub when running in 32-bit KVM mode

[Peter Maydell <peter.maydell@linaro.org>]

On Sat, 8 Jan 2022 at 15:10, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> When running under KVM, we may decide to run the CPU in 32-bit mode, by
> setting the 'aarch64=off' CPU option. In this case, we need to switch to
> the 32-bit version of the GDB stub too, so that GDB has the correct view
> of the CPU state. Without this, GDB debugging does not work at all, and
> errors out upon connecting to the target with a mysterious 'g' packet
> length error.
>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Alex Bennee <alex.bennee@linaro.org>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> v2: refactor existing CPUClass::gdb_... member assignments for the
>     32-bit code so we can reuse it for the 64-bit code
>
>  target/arm/cpu.c   | 16 +++++++++++-----
>  target/arm/cpu.h   |  2 ++
>  target/arm/cpu64.c |  3 +++
>  3 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index a211804fd3df..ae8e78fc1472 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -2049,6 +2049,15 @@ static const struct TCGCPUOps arm_tcg_ops = {
>  };
>  #endif /* CONFIG_TCG */
>
> +void arm_cpu_class_gdb_init(CPUClass *cc)
> +{
> +    cc->gdb_read_register = arm_cpu_gdb_read_register;
> +    cc->gdb_write_register = arm_cpu_gdb_write_register;
> +    cc->gdb_num_core_regs = 26;
> +    cc->gdb_core_xml_file = "arm-core.xml";
> +    cc->gdb_arch_name = arm_gdb_arch_name;
> +}

Most of these fields are not used by the gdbstub until
runtime, but cc->gdb_num_core_regs is used earlier.
In particular, in cpu_common_initfn() we copy that value
into cpu->gdb_num_regs and cpu->gdb_num_g_regs (this happens
at the CPU object's instance_init time, ie before the
aarch64_cpu_set_aarch64 function is called), and these are the
values that are then used when registering dynamic sysreg
XML, coprocessor registers, etc.

> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -906,6 +906,7 @@ static bool aarch64_cpu_get_aarch64(Object *obj, Error **errp)
>  static void aarch64_cpu_set_aarch64(Object *obj, bool value, Error **errp)
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
> +    CPUClass *cc = CPU_GET_CLASS(obj);

This is called to change the property for a specific CPU
object -- you can't change the values of the *class* here.
(Consider a system with 2 CPUs, one of which has aarch64=yes
and one of which has aarch64=no.)

>      /* At this time, this property is only allowed if KVM is enabled.  This
>       * restriction allows us to avoid fixing up functionality that assumes a
> @@ -919,6 +920,8 @@ static void aarch64_cpu_set_aarch64(Object *obj, bool value, Error **errp)
>              return;
>          }
>          unset_feature(&cpu->env, ARM_FEATURE_AARCH64);
> +
> +        arm_cpu_class_gdb_init(cc)

This fails to compile because of the missing semicolon...

>      } else {
>          set_feature(&cpu->env, ARM_FEATURE_AARCH64);

If the user (admittedly slightly perversely) toggles the
aarch64 flag from on to off to on again, we should reset the
gdb function pointers to the aarch64 versions again.

>      }

thanks
-- PMM