On Sat, 8 Jan 2022 at 15:10, Ard Biesheuvel <a...@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.hender...@linaro.org>
> Cc: Peter Maydell <peter.mayd...@linaro.org>
> Cc: Alex Bennee <alex.ben...@linaro.org>
> Signed-off-by: Ard Biesheuvel <a...@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
