[PATCH v2 8/9] target/arm: Add aarch64_tcg_ops

Richard Henderson posted 9 patches 5 months, 3 weeks ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Yanan Wang <wangyanan55@huawei.com>, "Alex Bennée" <alex.bennee@linaro.org>, Alexandre Iooss <erdnaxe@crans.org>, Mahmoud Mandour <ma.mandourr@gmail.com>, Pierrick Bouvier <pierrick.bouvier@linaro.org>, Peter Maydell <peter.maydell@linaro.org>
[PATCH v2 8/9] target/arm: Add aarch64_tcg_ops
Posted by Richard Henderson 5 months, 3 weeks ago
For the moment, this is an exact copy of arm_tcg_ops.
Export arm_cpu_exec_interrupt for the cross-file reference.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/internals.h |  1 +
 target/arm/cpu.c       |  2 +-
 target/arm/cpu64.c     | 30 ++++++++++++++++++++++++++++++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 11b5da2562..dc53d86249 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -364,6 +364,7 @@ void arm_restore_state_to_opc(CPUState *cs,
 
 #ifdef CONFIG_TCG
 void arm_cpu_synchronize_from_tb(CPUState *cs, const TranslationBlock *tb);
+bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request);
 #endif /* CONFIG_TCG */
 
 typedef enum ARMFPRounding {
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 35fa281f1b..3cd4711064 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -824,7 +824,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
     return unmasked || pstate_unmasked;
 }
 
-static bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
+bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
     CPUClass *cc = CPU_GET_CLASS(cs);
     CPUARMState *env = cpu_env(cs);
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 262a1d6c0b..7ba80099af 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -31,6 +31,9 @@
 #include "hvf_arm.h"
 #include "qapi/visitor.h"
 #include "hw/qdev-properties.h"
+#ifdef CONFIG_TCG
+#include "hw/core/tcg-cpu-ops.h"
+#endif
 #include "internals.h"
 #include "cpu-features.h"
 #include "cpregs.h"
@@ -793,6 +796,29 @@ static const gchar *aarch64_gdb_arch_name(CPUState *cs)
     return "aarch64";
 }
 
+#ifdef CONFIG_TCG
+static const TCGCPUOps aarch64_tcg_ops = {
+    .initialize = arm_translate_init,
+    .synchronize_from_tb = arm_cpu_synchronize_from_tb,
+    .debug_excp_handler = arm_debug_excp_handler,
+    .restore_state_to_opc = arm_restore_state_to_opc,
+
+#ifdef CONFIG_USER_ONLY
+    .record_sigsegv = arm_cpu_record_sigsegv,
+    .record_sigbus = arm_cpu_record_sigbus,
+#else
+    .tlb_fill = arm_cpu_tlb_fill,
+    .cpu_exec_interrupt = arm_cpu_exec_interrupt,
+    .do_interrupt = arm_cpu_do_interrupt,
+    .do_transaction_failed = arm_cpu_do_transaction_failed,
+    .do_unaligned_access = arm_cpu_do_unaligned_access,
+    .adjust_watchpoint_address = arm_adjust_watchpoint_address,
+    .debug_check_watchpoint = arm_debug_check_watchpoint,
+    .debug_check_breakpoint = arm_debug_check_breakpoint,
+#endif /* !CONFIG_USER_ONLY */
+};
+#endif /* CONFIG_TCG */
+
 static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
 {
     CPUClass *cc = CPU_CLASS(oc);
@@ -802,6 +828,10 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_core_xml_file = "aarch64-core.xml";
     cc->gdb_arch_name = aarch64_gdb_arch_name;
 
+#ifdef CONFIG_TCG
+    cc->tcg_ops = &aarch64_tcg_ops;
+#endif
+
     object_class_property_add_bool(oc, "aarch64", aarch64_cpu_get_aarch64,
                                    aarch64_cpu_set_aarch64);
     object_class_property_set_description(oc, "aarch64",
-- 
2.34.1
Re: [PATCH v2 8/9] target/arm: Add aarch64_tcg_ops
Posted by Alex Bennée 5 months, 2 weeks ago
Richard Henderson <richard.henderson@linaro.org> writes:

> For the moment, this is an exact copy of arm_tcg_ops.
> Export arm_cpu_exec_interrupt for the cross-file reference.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/internals.h |  1 +
>  target/arm/cpu.c       |  2 +-
>  target/arm/cpu64.c     | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 11b5da2562..dc53d86249 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -364,6 +364,7 @@ void arm_restore_state_to_opc(CPUState *cs,
>  
>  #ifdef CONFIG_TCG
>  void arm_cpu_synchronize_from_tb(CPUState *cs, const TranslationBlock *tb);
> +bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request);
>  #endif /* CONFIG_TCG */
>  
>  typedef enum ARMFPRounding {
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 35fa281f1b..3cd4711064 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -824,7 +824,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
>      return unmasked || pstate_unmasked;
>  }
>  
> -static bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
> +bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cs);
>      CPUARMState *env = cpu_env(cs);
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 262a1d6c0b..7ba80099af 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -31,6 +31,9 @@
>  #include "hvf_arm.h"
>  #include "qapi/visitor.h"
>  #include "hw/qdev-properties.h"
> +#ifdef CONFIG_TCG
> +#include "hw/core/tcg-cpu-ops.h"
> +#endif
>  #include "internals.h"
>  #include "cpu-features.h"
>  #include "cpregs.h"
> @@ -793,6 +796,29 @@ static const gchar *aarch64_gdb_arch_name(CPUState *cs)
>      return "aarch64";
>  }
>  
> +#ifdef CONFIG_TCG
> +static const TCGCPUOps aarch64_tcg_ops = {
> +    .initialize = arm_translate_init,
> +    .synchronize_from_tb = arm_cpu_synchronize_from_tb,
> +    .debug_excp_handler = arm_debug_excp_handler,
> +    .restore_state_to_opc = arm_restore_state_to_opc,
> +
> +#ifdef CONFIG_USER_ONLY
> +    .record_sigsegv = arm_cpu_record_sigsegv,
> +    .record_sigbus = arm_cpu_record_sigbus,
> +#else
> +    .tlb_fill = arm_cpu_tlb_fill,
> +    .cpu_exec_interrupt = arm_cpu_exec_interrupt,
> +    .do_interrupt = arm_cpu_do_interrupt,
> +    .do_transaction_failed = arm_cpu_do_transaction_failed,
> +    .do_unaligned_access = arm_cpu_do_unaligned_access,
> +    .adjust_watchpoint_address = arm_adjust_watchpoint_address,
> +    .debug_check_watchpoint = arm_debug_check_watchpoint,
> +    .debug_check_breakpoint = arm_debug_check_breakpoint,
> +#endif /* !CONFIG_USER_ONLY */
> +};
> +#endif /* CONFIG_TCG */
> +

My principle concern is duplicating an otherwise identical structure
just gives us more opportunity to miss a change. 

>  static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      CPUClass *cc = CPU_CLASS(oc);
> @@ -802,6 +828,10 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
>      cc->gdb_core_xml_file = "aarch64-core.xml";
>      cc->gdb_arch_name = aarch64_gdb_arch_name;
>  
> +#ifdef CONFIG_TCG
> +    cc->tcg_ops = &aarch64_tcg_ops;
> +#endif
> +

What happens when the CPU is running mixed mode code and jumping between
64 and 32 bit? Wouldn't it be easier to have a helper that routes to the
correct unwinder, c.f. gen_intermediate_code

  #ifdef TARGET_AARCH64
      if (EX_TBFLAG_ANY(tb_flags, AARCH64_STATE)) {
          ops = &aarch64_translator_ops;
      }
  #endif

which I guess for a runtime helper would be:

   if (is_a64(cpu_env(cs))) {
      aarch64_plugin_need_unwind_for_reg(...);
   } else {
      arm_plugin_need_unwind_for_reg(...);
   }


etc...


>      object_class_property_add_bool(oc, "aarch64", aarch64_cpu_get_aarch64,
>                                     aarch64_cpu_set_aarch64);
>      object_class_property_set_description(oc, "aarch64",

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro
Re: [PATCH v2 8/9] target/arm: Add aarch64_tcg_ops
Posted by Richard Henderson 5 months, 2 weeks ago
On 6/12/24 07:36, Alex Bennée wrote:
> What happens when the CPU is running mixed mode code and jumping between
> 64 and 32 bit? Wouldn't it be easier to have a helper that routes to the
> correct unwinder, c.f. gen_intermediate_code

GDB can't switch modes, so there is *never* any mode switching.


r~

Re: [PATCH v2 8/9] target/arm: Add aarch64_tcg_ops
Posted by Alex Bennée 5 months, 2 weeks ago
Richard Henderson <richard.henderson@linaro.org> writes:

> On 6/12/24 07:36, Alex Bennée wrote:
>> What happens when the CPU is running mixed mode code and jumping between
>> 64 and 32 bit? Wouldn't it be easier to have a helper that routes to the
>> correct unwinder, c.f. gen_intermediate_code
>
> GDB can't switch modes, so there is *never* any mode switching.

Hmm but code can. We do want to solve the gdb mode switching case as
well although that requires some work on the gdbstub.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro