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