| Message ID | 20220108150952.1483911-1-ardb@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] target/arm/cpu64: Use 32-bit GDBstub when running in 32-bit KVM mode | expand |
On 1/8/22 7:09 AM, Ard Biesheuvel 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(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 1/8/22 16:09, Ard Biesheuvel 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 Way cleaner. Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Ard Biesheuvel <ardb@kernel.org> writes: > 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> Reviewed-by: Alex Bennée <alex.bennee@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
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.
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; +} + static void arm_cpu_class_init(ObjectClass *oc, void *data) { ARMCPUClass *acc = ARM_CPU_CLASS(oc); @@ -2061,18 +2070,15 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data) device_class_set_props(dc, arm_cpu_properties); device_class_set_parent_reset(dc, arm_cpu_reset, &acc->parent_reset); + arm_cpu_class_gdb_init(cc); + cc->class_by_name = arm_cpu_class_by_name; cc->has_work = arm_cpu_has_work; cc->dump_state = arm_cpu_dump_state; cc->set_pc = arm_cpu_set_pc; - cc->gdb_read_register = arm_cpu_gdb_read_register; - cc->gdb_write_register = arm_cpu_gdb_write_register; #ifndef CONFIG_USER_ONLY cc->sysemu_ops = &arm_sysemu_ops; #endif - cc->gdb_num_core_regs = 26; - cc->gdb_core_xml_file = "arm-core.xml"; - cc->gdb_arch_name = arm_gdb_arch_name; cc->gdb_get_dynamic_xml = arm_gdb_get_dynamic_xml; cc->gdb_stop_before_watchpoint = true; cc->disas_set_info = arm_disas_set_info; diff --git a/target/arm/cpu.h b/target/arm/cpu.h index e33f37b70ada..208da8e35697 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -1064,6 +1064,8 @@ int arm_gen_dynamic_svereg_xml(CPUState *cpu, int base_reg); */ const char *arm_gdb_get_dynamic_xml(CPUState *cpu, const char *xmlname); +void arm_cpu_class_gdb_init(CPUClass *cc); + int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs, int cpuid, void *opaque); int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs, diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c index 15245a60a8c7..df7667864e11 100644 --- 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); /* 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) } else { set_feature(&cpu->env, ARM_FEATURE_AARCH64); }
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(-)