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

[Ard Biesheuvel <ardb@kernel.org>]

On Tue, 11 Jan 2022 at 15:11, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> 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.
>

Right.

> > --- 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.)
>

So how is this fundamentally going to work then? Which GDB stub should
we choose in such a case?


> >      /* 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...
>

Oops, my bad. I spotted this locally as well but failed to fold it
into the patch.

> >      } 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.
>

Ack.

So I can fix most of these issues, but the fundamental one remains, so
I'll hold off on a v3 until we can settle that.

Thanks,
Ard.