| Message ID | 20250317142819.900029-7-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| 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.
On Fri, 25 Apr 2025 at 15:42, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > 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(): Thanks for finding this. The problem is that gdb_init_cpu() is being called as part of the base-class instance init, so the CPU object isn't fully instantiated yet -- it hasn't even run the aarch64_max_initfn() function yet. So gdb_init_cpu() ends up calling CPU-specific methods on a half-instantiated object, which is why it crashes. But we do need to move things so they happen after realize, because up til realize the CPU properties might be changed (otherwise we give the wrong answer for the aarch64=off case). Except that part of the CPU subclass realize involves calling gdb_register_coprocessor(), which needs to happen after we've set up the core regs for gdb. I think that moving the call to cpu_exec_realizefn() would work (this gets called near the start of realize, so before the subclass realize decides to add more gdb registers). But I'll have to check whether that would be wrong for some other architecture, and maybe there's a neater way to do this. -- PMM
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(-)