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

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

[Ard Biesheuvel]

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;
+}
+
 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);
     }
-- 
2.30.2

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

[Richard Henderson]

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~

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

[Philippe Mathieu-Daudé]

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>

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

[Alex Bennée]

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>

-- 
Alex Bennée

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

[Peter Maydell]

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

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

[Ard Biesheuvel]

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.