| Message ID | 20250317142819.900029-7-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Remove TYPE_AARCH64_CPU class | expand |
On 17/3/25 15:28, Peter Maydell wrote: > Currently we provide an AArch64 gdbstub for CPUs which are > TYPE_AARCH64_CPU, and an AArch32 gdbstub for those which are only > TYPE_ARM_CPU. This mostly does the right thing, except in the > corner case of KVM with -cpu host,aarch64=off. That produces a CPU > which is TYPE_AARCH64_CPU but which has ARM_FEATURE_AARCH64 removed > and which to the guest is in AArch32 mode. > > Now we have moved all the handling of AArch64-vs-AArch32 gdbstub > behaviour into TYPE_ARM_CPU we can change the condition we use for > whether to select the AArch64 gdbstub to look at ARM_FEATURE_AARCH64. > This will mean that we now correctly provide an AArch32 gdbstub for > aarch64=off CPUs. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > target/arm/internals.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On 17/3/25 15:28, Peter Maydell wrote: > Currently we provide an AArch64 gdbstub for CPUs which are > TYPE_AARCH64_CPU, and an AArch32 gdbstub for those which are only > TYPE_ARM_CPU. This mostly does the right thing, except in the > corner case of KVM with -cpu host,aarch64=off. That produces a CPU > which is TYPE_AARCH64_CPU but which has ARM_FEATURE_AARCH64 removed > and which to the guest is in AArch32 mode. > > Now we have moved all the handling of AArch64-vs-AArch32 gdbstub > behaviour into TYPE_ARM_CPU we can change the condition we use for > whether to select the AArch64 gdbstub to look at ARM_FEATURE_AARCH64. > This will mean that we now correctly provide an AArch32 gdbstub for > aarch64=off CPUs. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > target/arm/internals.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/arm/internals.h b/target/arm/internals.h > index a14c269fa5a..a18d87fa28b 100644 > --- a/target/arm/internals.h > +++ b/target/arm/internals.h > @@ -1694,7 +1694,7 @@ void aarch64_add_sme_properties(Object *obj); > /* Return true if the gdbstub is presenting an AArch64 CPU */ > static inline bool arm_gdbstub_is_aarch64(ARMCPU *cpu) 4 uses, maybe worth removing (directly inlining) as a cleanup patch on top of this conversion series. > { > - return object_dynamic_cast(OBJECT(cpu), TYPE_AARCH64_CPU); > + return arm_feature(&cpu->env, ARM_FEATURE_AARCH64); > } > > /* Read the CONTROL register as the MRS instruction would. */
Hi Peter, On 17/3/25 15:28, Peter Maydell wrote: > Currently we provide an AArch64 gdbstub for CPUs which are > TYPE_AARCH64_CPU, and an AArch32 gdbstub for those which are only > TYPE_ARM_CPU. This mostly does the right thing, except in the > corner case of KVM with -cpu host,aarch64=off. That produces a CPU > which is TYPE_AARCH64_CPU but which has ARM_FEATURE_AARCH64 removed > and which to the guest is in AArch32 mode. > > Now we have moved all the handling of AArch64-vs-AArch32 gdbstub > behaviour into TYPE_ARM_CPU we can change the condition we use for > whether to select the AArch64 gdbstub to look at ARM_FEATURE_AARCH64. > This will mean that we now correctly provide an AArch32 gdbstub for > aarch64=off CPUs. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > target/arm/internals.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/arm/internals.h b/target/arm/internals.h > index a14c269fa5a..a18d87fa28b 100644 > --- a/target/arm/internals.h > +++ b/target/arm/internals.h > @@ -1694,7 +1694,7 @@ void aarch64_add_sme_properties(Object *obj); > /* Return true if the gdbstub is presenting an AArch64 CPU */ > static inline bool arm_gdbstub_is_aarch64(ARMCPU *cpu) > { > - return object_dynamic_cast(OBJECT(cpu), TYPE_AARCH64_CPU); > + return arm_feature(&cpu->env, ARM_FEATURE_AARCH64); Unfortunately this doesn't work well: while a Aarch64 CPU is of type TYPE_AARCH64_CPU right after being instantiated (not yet QOM realized), the features are only finalized during arm_cpu_instance_init(): static void arm_cpu_instance_init(Object *obj) { ARMCPUClass *acc = ARM_CPU_GET_CLASS(obj); acc->info->initfn(obj); arm_cpu_post_init(obj); } void arm_cpu_post_init(Object *obj) { ARMCPU *cpu = ARM_CPU(obj); /* * Some features imply others. Figure this out now, because we * are going to look at the feature bits in deciding which * properties to add. */ arm_cpu_propagate_feature_implications(cpu); ... } The GDB feature checks are done earlier: object_init_with_type -> cpu_common_initfn -> gdb_init_cpu -> gdb_get_core_xml_file -> arm_gdb_get_core_xml_file -> arm_gdbstub_is_aarch64 At this point the feature set is empty, triggering the assertion in gdb_find_static_feature(): $ ./build/qemu-aarch64 build/tests/tcg/aarch64-linux-user/semihosting ** ERROR:../../gdbstub/gdbstub.c:494:gdb_find_static_feature: code should not be reached Bail out! ERROR:../../gdbstub/gdbstub.c:494:gdb_find_static_feature: code should not be reached Aborted (core dumped) I suppose gdb_init_cpu() needs more splitting work. For now I'll drop this patches 5+ from my queue. Regards, Phil.
diff --git a/target/arm/internals.h b/target/arm/internals.h index a14c269fa5a..a18d87fa28b 100644 --- a/target/arm/internals.h +++ b/target/arm/internals.h @@ -1694,7 +1694,7 @@ void aarch64_add_sme_properties(Object *obj); /* Return true if the gdbstub is presenting an AArch64 CPU */ static inline bool arm_gdbstub_is_aarch64(ARMCPU *cpu) { - return object_dynamic_cast(OBJECT(cpu), TYPE_AARCH64_CPU); + return arm_feature(&cpu->env, ARM_FEATURE_AARCH64); } /* Read the CONTROL register as the MRS instruction would. */
Currently we provide an AArch64 gdbstub for CPUs which are TYPE_AARCH64_CPU, and an AArch32 gdbstub for those which are only TYPE_ARM_CPU. This mostly does the right thing, except in the corner case of KVM with -cpu host,aarch64=off. That produces a CPU which is TYPE_AARCH64_CPU but which has ARM_FEATURE_AARCH64 removed and which to the guest is in AArch32 mode. Now we have moved all the handling of AArch64-vs-AArch32 gdbstub behaviour into TYPE_ARM_CPU we can change the condition we use for whether to select the AArch64 gdbstub to look at ARM_FEATURE_AARCH64. This will mean that we now correctly provide an AArch32 gdbstub for aarch64=off CPUs. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/internals.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)