[PATCH 0/5] target/arm: Some CONFIG_TCG code movement

[PATCH 0/5] target/arm: Some CONFIG_TCG code movement

[Fabiano Rosas]

Hi,

This is the second round of rebasing the patches from:
https://lore.kernel.org/r/20210416162824.25131-1-cfontana@suse.de

These are the simpler ones that move code under
CONFIG_TCG/tcg_enabled. No new directories or files.

Claudio Fontana (5):
  target/arm: only build psci for TCG
  target/arm: rename handle_semihosting to tcg_handle_semihosting
  target/arm: wrap semihosting and psci calls with tcg_enabled
  target/arm: wrap call to aarch64_sve_change_el in tcg_enabled()
  target/arm: only perform TCG cpu and machine inits if TCG enabled

 target/arm/cpu.c       | 31 ++++++++++++---------
 target/arm/helper.c    | 45 +++++++++++++++++--------------
 target/arm/kvm.c       | 18 ++++++-------
 target/arm/kvm_arm.h   |  3 +--
 target/arm/machine.c   | 61 +++++++++++++++++++++++-------------------
 target/arm/meson.build |  5 +++-
 6 files changed, 91 insertions(+), 72 deletions(-)

-- 
2.35.3

[PATCH 1/5] target/arm: only build psci for TCG

[Fabiano Rosas]

From: Claudio Fontana <cfontana@suse.de>

Signed-off-by: Claudio Fontana <cfontana@suse.de>
Cc: Alexander Graf <agraf@csgraf.de>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
Originally from:
[RFC v14 09/80] target/arm: only build psci for TCG
https://lore.kernel.org/r/20210416162824.25131-10-cfontana@suse.de
---
 target/arm/meson.build | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/target/arm/meson.build b/target/arm/meson.build
index 87e911b27f..26e425418f 100644
--- a/target/arm/meson.build
+++ b/target/arm/meson.build
@@ -61,10 +61,13 @@ arm_softmmu_ss.add(files(
   'arm-powerctl.c',
   'machine.c',
   'monitor.c',
-  'psci.c',
   'ptw.c',
 ))
 
+arm_softmmu_ss.add(when: 'CONFIG_TCG', if_true: files(
+  'psci.c',
+))
+
 subdir('hvf')
 
 target_arch += {'arm': arm_ss}
-- 
2.35.3

[PATCH 2/5] target/arm: rename handle_semihosting to tcg_handle_semihosting

[Fabiano Rosas]

From: Claudio Fontana <cfontana@suse.de>

make it clearer from the name that this is a tcg-only function.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
This function moves elsewhere in the original series, but the name
change doesn't need to wait.

Originally from:
[RFC v14 38/80] target/arm: rename handle_semihosting to tcg_handle_semihosting
https://lore.kernel.org/r/20210416162824.25131-39-cfontana@suse.de
---
 target/arm/helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index d8c8223ec3..45bf164a07 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10252,7 +10252,7 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
  * trapped to the hypervisor in KVM.
  */
 #ifdef CONFIG_TCG
-static void handle_semihosting(CPUState *cs)
+static void tcg_handle_semihosting(CPUState *cs)
 {
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
@@ -10313,7 +10313,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
      */
 #ifdef CONFIG_TCG
     if (cs->exception_index == EXCP_SEMIHOST) {
-        handle_semihosting(cs);
+        tcg_handle_semihosting(cs);
         return;
     }
 #endif
-- 
2.35.3

[PATCH 3/5] target/arm: wrap semihosting and psci calls with tcg_enabled

[Fabiano Rosas]

From: Claudio Fontana <cfontana@suse.de>

for "all" builds (tcg + kvm), we want to avoid doing
the psci and semihosting checks if tcg is built-in, but not enabled.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
Originally from:
[RFC v14 39/80] target/arm: replace CONFIG_TCG with tcg_enabled
https://lore.kernel.org/r/20210416162824.25131-40-cfontana@suse.de
---
 target/arm/helper.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 45bf164a07..9d0a53cb00 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -26,6 +26,7 @@
 #include "sysemu/cpus.h"
 #include "sysemu/cpu-timers.h"
 #include "sysemu/kvm.h"
+#include "sysemu/tcg.h"
 #include "qemu/range.h"
 #include "qapi/qapi-commands-machine-target.h"
 #include "qapi/error.h"
@@ -10300,23 +10301,25 @@ void arm_cpu_do_interrupt(CPUState *cs)
                       env->exception.syndrome);
     }
 
-    if (arm_is_psci_call(cpu, cs->exception_index)) {
-        arm_handle_psci_call(cpu);
-        qemu_log_mask(CPU_LOG_INT, "...handled as PSCI call\n");
-        return;
-    }
+    if (tcg_enabled()) {
+        if (arm_is_psci_call(cpu, cs->exception_index)) {
+            arm_handle_psci_call(cpu);
+            qemu_log_mask(CPU_LOG_INT, "...handled as PSCI call\n");
+            return;
+        }
 
-    /*
-     * Semihosting semantics depend on the register width of the code
-     * that caused the exception, not the target exception level, so
-     * must be handled here.
-     */
+        /*
+         * Semihosting semantics depend on the register width of the code
+         * that caused the exception, not the target exception level, so
+         * must be handled here.
+         */
 #ifdef CONFIG_TCG
-    if (cs->exception_index == EXCP_SEMIHOST) {
-        tcg_handle_semihosting(cs);
-        return;
-    }
+        if (cs->exception_index == EXCP_SEMIHOST) {
+            tcg_handle_semihosting(cs);
+            return;
+        }
 #endif
+    }
 
     /* Hooks may change global state so BQL should be held, also the
      * BQL needs to be held for any modification of
-- 
2.35.3

[PATCH 4/5] target/arm: wrap call to aarch64_sve_change_el in tcg_enabled()

[Fabiano Rosas]

From: Claudio Fontana <cfontana@suse.de>

Signed-off-by: Claudio Fontana <cfontana@suse.de>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
Originally from:
[RFC v14 42/80] target/arm: wrap call to aarch64_sve_change_el in tcg_enabled()
https://lore.kernel.org/r/20210416162824.25131-43-cfontana@suse.de
---
 target/arm/helper.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 9d0a53cb00..92624e2910 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10066,11 +10066,13 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
     unsigned int cur_el = arm_current_el(env);
     int rt;
 
-    /*
-     * Note that new_el can never be 0.  If cur_el is 0, then
-     * el0_a64 is is_a64(), else el0_a64 is ignored.
-     */
-    aarch64_sve_change_el(env, cur_el, new_el, is_a64(env));
+    if (tcg_enabled()) {
+        /*
+         * Note that new_el can never be 0.  If cur_el is 0, then
+         * el0_a64 is is_a64(), else el0_a64 is ignored.
+         */
+        aarch64_sve_change_el(env, cur_el, new_el, is_a64(env));
+    }
 
     if (cur_el < new_el) {
         /* Entry vector offset depends on whether the implemented EL
-- 
2.35.3

[PATCH 5/5] target/arm: only perform TCG cpu and machine inits if TCG enabled

[Fabiano Rosas]

From: Claudio Fontana <cfontana@suse.de>

of note, cpreg lists were previously initialized by TCG first,
and then thrown away and replaced with the data coming from KVM.

Now we just initialize once, either for TCG or for KVM.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
[moved arm_cpu_register_gdb_regs_for_features out of tcg_enabled]
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
We still need to tell GDB about the register sets even when running
with KVM.

Originally from:
[RFC v14 16/80] target/arm: only perform TCG cpu and machine inits if
TCG enabled
https://lore.kernel.org/r/20210416162824.25131-17-cfontana@suse.de
---
 target/arm/cpu.c     | 31 ++++++++++++----------
 target/arm/kvm.c     | 18 ++++++-------
 target/arm/kvm_arm.h |  3 +--
 target/arm/machine.c | 61 ++++++++++++++++++++++++--------------------
 4 files changed, 62 insertions(+), 51 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 38d066c294..a0c77ba153 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -523,9 +523,11 @@ static void arm_cpu_reset(DeviceState *dev)
     }
 #endif
 
-    hw_breakpoint_update_all(cpu);
-    hw_watchpoint_update_all(cpu);
-    arm_rebuild_hflags(env);
+    if (tcg_enabled()) {
+        hw_breakpoint_update_all(cpu);
+        hw_watchpoint_update_all(cpu);
+        arm_rebuild_hflags(env);
+    }
 }
 
 #ifndef CONFIG_USER_ONLY
@@ -1597,6 +1599,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         }
     }
 
+#ifdef CONFIG_TCG
     {
         uint64_t scale;
 
@@ -1622,7 +1625,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         cpu->gt_timer[GTIMER_HYPVIRT] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
                                                   arm_gt_hvtimer_cb, cpu);
     }
-#endif
+#endif /* CONFIG_TCG */
+#endif /* !CONFIG_USER_ONLY */
 
     cpu_exec_realizefn(cs, &local_err);
     if (local_err != NULL) {
@@ -1940,17 +1944,16 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         unset_feature(env, ARM_FEATURE_PMU);
     }
     if (arm_feature(env, ARM_FEATURE_PMU)) {
-        pmu_init(cpu);
-
-        if (!kvm_enabled()) {
+        if (tcg_enabled()) {
+            pmu_init(cpu);
             arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0);
             arm_register_el_change_hook(cpu, &pmu_post_el_change, 0);
-        }
 
 #ifndef CONFIG_USER_ONLY
-        cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, arm_pmu_timer_cb,
-                cpu);
+            cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, arm_pmu_timer_cb,
+                                          cpu);
 #endif
+        }
     } else {
         cpu->isar.id_aa64dfr0 =
             FIELD_DP64(cpu->isar.id_aa64dfr0, ID_AA64DFR0, PMUVER, 0);
@@ -2046,10 +2049,12 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         set_feature(env, ARM_FEATURE_VBAR);
     }
 
-    register_cp_regs_for_features(cpu);
-    arm_cpu_register_gdb_regs_for_features(cpu);
+    if (tcg_enabled()) {
+        register_cp_regs_for_features(cpu);
+        init_cpreg_list(cpu);
+    }
 
-    init_cpreg_list(cpu);
+    arm_cpu_register_gdb_regs_for_features(cpu);
 
 #ifndef CONFIG_USER_ONLY
     MachineState *ms = MACHINE(qdev_get_machine());
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index f022c644d2..2f01c26f54 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -438,9 +438,11 @@ static uint64_t *kvm_arm_get_cpreg_ptr(ARMCPU *cpu, uint64_t regidx)
     return &cpu->cpreg_values[res - cpu->cpreg_indexes];
 }
 
-/* Initialize the ARMCPU cpreg list according to the kernel's
- * definition of what CPU registers it knows about (and throw away
- * the previous TCG-created cpreg list).
+/*
+ * Initialize the ARMCPU cpreg list according to the kernel's
+ * definition of what CPU registers it knows about.
+ *
+ * The parallel for TCG is init_cpreg_list()
  */
 int kvm_arm_init_cpreg_list(ARMCPU *cpu)
 {
@@ -482,12 +484,10 @@ int kvm_arm_init_cpreg_list(ARMCPU *cpu)
         arraylen++;
     }
 
-    cpu->cpreg_indexes = g_renew(uint64_t, cpu->cpreg_indexes, arraylen);
-    cpu->cpreg_values = g_renew(uint64_t, cpu->cpreg_values, arraylen);
-    cpu->cpreg_vmstate_indexes = g_renew(uint64_t, cpu->cpreg_vmstate_indexes,
-                                         arraylen);
-    cpu->cpreg_vmstate_values = g_renew(uint64_t, cpu->cpreg_vmstate_values,
-                                        arraylen);
+    cpu->cpreg_indexes = g_new(uint64_t, arraylen);
+    cpu->cpreg_values = g_new(uint64_t, arraylen);
+    cpu->cpreg_vmstate_indexes = g_new(uint64_t, arraylen);
+    cpu->cpreg_vmstate_values = g_new(uint64_t, arraylen);
     cpu->cpreg_array_len = arraylen;
     cpu->cpreg_vmstate_array_len = arraylen;
 
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index 99017b635c..41de2a7cf1 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -70,8 +70,7 @@ void kvm_arm_register_device(MemoryRegion *mr, uint64_t devid, uint64_t group,
  * @cpu: ARMCPU
  *
  * Initialize the ARMCPU cpreg list according to the kernel's
- * definition of what CPU registers it knows about (and throw away
- * the previous TCG-created cpreg list).
+ * definition of what CPU registers it knows about.
  *
  * Returns: 0 if success, else < 0 error code
  */
diff --git a/target/arm/machine.c b/target/arm/machine.c
index 54c5c62433..4cc4468d3e 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -2,6 +2,7 @@
 #include "cpu.h"
 #include "qemu/error-report.h"
 #include "sysemu/kvm.h"
+#include "sysemu/tcg.h"
 #include "kvm_arm.h"
 #include "internals.h"
 #include "migration/cpu.h"
@@ -687,7 +688,7 @@ static int cpu_pre_save(void *opaque)
 {
     ARMCPU *cpu = opaque;
 
-    if (!kvm_enabled()) {
+    if (tcg_enabled()) {
         pmu_op_start(&cpu->env);
     }
 
@@ -722,7 +723,7 @@ static int cpu_post_save(void *opaque)
 {
     ARMCPU *cpu = opaque;
 
-    if (!kvm_enabled()) {
+    if (tcg_enabled()) {
         pmu_op_finish(&cpu->env);
     }
 
@@ -741,7 +742,7 @@ static int cpu_pre_load(void *opaque)
      */
     env->irq_line_state = UINT32_MAX;
 
-    if (!kvm_enabled()) {
+    if (tcg_enabled()) {
         pmu_op_start(&cpu->env);
     }
 
@@ -811,36 +812,37 @@ static int cpu_post_load(void *opaque, int version_id)
         }
     }
 
-    hw_breakpoint_update_all(cpu);
-    hw_watchpoint_update_all(cpu);
+    if (tcg_enabled()) {
+        hw_breakpoint_update_all(cpu);
+        hw_watchpoint_update_all(cpu);
 
-    /*
-     * TCG gen_update_fp_context() relies on the invariant that
-     * FPDSCR.LTPSIZE is constant 4 for M-profile with the LOB extension;
-     * forbid bogus incoming data with some other value.
-     */
-    if (arm_feature(env, ARM_FEATURE_M) && cpu_isar_feature(aa32_lob, cpu)) {
-        if (extract32(env->v7m.fpdscr[M_REG_NS],
-                      FPCR_LTPSIZE_SHIFT, FPCR_LTPSIZE_LENGTH) != 4 ||
-            extract32(env->v7m.fpdscr[M_REG_S],
-                      FPCR_LTPSIZE_SHIFT, FPCR_LTPSIZE_LENGTH) != 4) {
-            return -1;
+        /*
+         * TCG gen_update_fp_context() relies on the invariant that
+         * FPDSCR.LTPSIZE is constant 4 for M-profile with the LOB extension;
+         * forbid bogus incoming data with some other value.
+         */
+        if (arm_feature(env, ARM_FEATURE_M) &&
+            cpu_isar_feature(aa32_lob, cpu)) {
+            if (extract32(env->v7m.fpdscr[M_REG_NS],
+                          FPCR_LTPSIZE_SHIFT, FPCR_LTPSIZE_LENGTH) != 4 ||
+                extract32(env->v7m.fpdscr[M_REG_S],
+                          FPCR_LTPSIZE_SHIFT, FPCR_LTPSIZE_LENGTH) != 4) {
+                return -1;
+            }
         }
-    }
 
-    /*
-     * Misaligned thumb pc is architecturally impossible.
-     * We have an assert in thumb_tr_translate_insn to verify this.
-     * Fail an incoming migrate to avoid this assert.
-     */
-    if (!is_a64(env) && env->thumb && (env->regs[15] & 1)) {
-        return -1;
-    }
+        /*
+         * Misaligned thumb pc is architecturally impossible.
+         * We have an assert in thumb_tr_translate_insn to verify this.
+         * Fail an incoming migrate to avoid this assert.
+         */
+        if (!is_a64(env) && env->thumb && (env->regs[15] & 1)) {
+            return -1;
+        }
 
-    if (!kvm_enabled()) {
         pmu_op_finish(&cpu->env);
+        arm_rebuild_hflags(&cpu->env);
     }
-    arm_rebuild_hflags(&cpu->env);
 
     return 0;
 }
@@ -890,8 +892,13 @@ const VMStateDescription vmstate_arm_cpu = {
         VMSTATE_UINT32(env.exception.syndrome, ARMCPU),
         VMSTATE_UINT32(env.exception.fsr, ARMCPU),
         VMSTATE_UINT64(env.exception.vaddress, ARMCPU),
+#ifdef CONFIG_TCG
         VMSTATE_TIMER_PTR(gt_timer[GTIMER_PHYS], ARMCPU),
         VMSTATE_TIMER_PTR(gt_timer[GTIMER_VIRT], ARMCPU),
+#else
+        VMSTATE_UNUSED(sizeof(QEMUTimer *)),
+        VMSTATE_UNUSED(sizeof(QEMUTimer *)),
+#endif /* CONFIG_TCG */
         {
             .name = "power_state",
             .version_id = 0,
-- 
2.35.3

Re: [PATCH 1/5] target/arm: only build psci for TCG

[Alexander Graf]

Hi Claudio,

If the PSCI implementation becomes TCG only, can we also move to a tcg accel directory? It slowly gets super confusing to keep track of which files are supposed to be generic target code and which ones TCG specific.

Alex

> Am 16.12.2022 um 22:37 schrieb Fabiano Rosas <farosas@suse.de>:
> 
> From: Claudio Fontana <cfontana@suse.de>
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> Cc: Alexander Graf <agraf@csgraf.de>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> Originally from:
> [RFC v14 09/80] target/arm: only build psci for TCG
> https://lore.kernel.org/r/20210416162824.25131-10-cfontana@suse.de
> ---
> target/arm/meson.build | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/meson.build b/target/arm/meson.build
> index 87e911b27f..26e425418f 100644
> --- a/target/arm/meson.build
> +++ b/target/arm/meson.build
> @@ -61,10 +61,13 @@ arm_softmmu_ss.add(files(
>   'arm-powerctl.c',
>   'machine.c',
>   'monitor.c',
> -  'psci.c',
>   'ptw.c',
> ))
> 
> +arm_softmmu_ss.add(when: 'CONFIG_TCG', if_true: files(
> +  'psci.c',
> +))
> +
> subdir('hvf')
> 
> target_arch += {'arm': arm_ss}
> -- 
> 2.35.3
> 

Re: [PATCH 2/5] target/arm: rename handle_semihosting to tcg_handle_semihosting

[Richard Henderson]

On 12/16/22 13:29, Fabiano Rosas wrote:
> From: Claudio Fontana <cfontana@suse.de>
> 
> make it clearer from the name that this is a tcg-only function.
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> This function moves elsewhere in the original series, but the name
> change doesn't need to wait.
> 
> Originally from:
> [RFC v14 38/80] target/arm: rename handle_semihosting to tcg_handle_semihosting
> https://lore.kernel.org/r/20210416162824.25131-39-cfontana@suse.de

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 3/5] target/arm: wrap semihosting and psci calls with tcg_enabled

[Richard Henderson]

On 12/16/22 13:29, Fabiano Rosas wrote:
> -    if (arm_is_psci_call(cpu, cs->exception_index)) {
> -        arm_handle_psci_call(cpu);
> -        qemu_log_mask(CPU_LOG_INT, "...handled as PSCI call\n");
> -        return;
> -    }
> +    if (tcg_enabled()) {
> +        if (arm_is_psci_call(cpu, cs->exception_index)) {

This could be

     if (tcg_enabled() && arm_is_psci_call(...))

because...

> -    /*
> -     * Semihosting semantics depend on the register width of the code
> -     * that caused the exception, not the target exception level, so
> -     * must be handled here.
> -     */
> +        /*
> +         * Semihosting semantics depend on the register width of the code
> +         * that caused the exception, not the target exception level, so
> +         * must be handled here.
> +         */
>   #ifdef CONFIG_TCG
> -    if (cs->exception_index == EXCP_SEMIHOST) {
> -        tcg_handle_semihosting(cs);
> -        return;
> -    }
> +        if (cs->exception_index == EXCP_SEMIHOST) {

If you were able to replace the ifdef, that would be one thing, but since you aren't I 
don't think this requires a separate check.  There is no way to generate EXCP_SEMIHOST 
except via TCG.

I guess I don't insist, since you're working toward Claudio's much larger patch set, so

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 5/5] target/arm: only perform TCG cpu and machine inits if TCG enabled

[Richard Henderson]

On 12/16/22 13:29, Fabiano Rosas wrote:
> -    /*
> -     * Misaligned thumb pc is architecturally impossible.
> -     * We have an assert in thumb_tr_translate_insn to verify this.
> -     * Fail an incoming migrate to avoid this assert.
> -     */
> -    if (!is_a64(env) && env->thumb && (env->regs[15] & 1)) {
> -        return -1;
> -    }
> +        /*
> +         * Misaligned thumb pc is architecturally impossible.
> +         * We have an assert in thumb_tr_translate_insn to verify this.
> +         * Fail an incoming migrate to avoid this assert.
> +         */
> +        if (!is_a64(env) && env->thumb && (env->regs[15] & 1)) {
> +            return -1;
> +        }

This is a sanity check rejecting malformed vmsave.  While hw virt won't have the same 
assert as mentioned in the comment, it won't be happy and will raise some sort of cpu 
exception later.  I think it's better to reject the bad vmload early.  I suppose we could 
expand the comment to that effect, so that it doesn't appear to be wholly tcg inspired.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 1/5] target/arm: only build psci for TCG

[Claudio Fontana]

On 12/16/22 22:59, Alexander Graf wrote:
> Hi Claudio,
> 
> If the PSCI implementation becomes TCG only, can we also move to a tcg accel directory? It slowly gets super confusing to keep track of which files are supposed to be generic target code and which ones TCG specific> 
> Alex

Hi Alex, Fabiano, Peter and all,

that was the plan but at the time of:

https://lore.kernel.org/all/20210416162824.25131-1-cfontana@suse.de/

Peter mentioned that HVF AArch64 might use that code too:

https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg00509.html

so from v2 to v3 the series changed to not move the code under tcg/ , expecting HVF to be reusing that code "soon".

I see that your hvf code in hvf/ implements psci, is there some plan to reuse pieces from the tcg implementation now?

Ciao,

Claudio


> 
>> Am 16.12.2022 um 22:37 schrieb Fabiano Rosas <farosas@suse.de>:
>>
>> From: Claudio Fontana <cfontana@suse.de>
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> Cc: Alexander Graf <agraf@csgraf.de>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>> Originally from:
>> [RFC v14 09/80] target/arm: only build psci for TCG
>> https://lore.kernel.org/r/20210416162824.25131-10-cfontana@suse.de
>> ---
>> target/arm/meson.build | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/arm/meson.build b/target/arm/meson.build
>> index 87e911b27f..26e425418f 100644
>> --- a/target/arm/meson.build
>> +++ b/target/arm/meson.build
>> @@ -61,10 +61,13 @@ arm_softmmu_ss.add(files(
>>   'arm-powerctl.c',
>>   'machine.c',
>>   'monitor.c',
>> -  'psci.c',
>>   'ptw.c',
>> ))
>>
>> +arm_softmmu_ss.add(when: 'CONFIG_TCG', if_true: files(
>> +  'psci.c',
>> +))
>> +
>> subdir('hvf')
>>
>> target_arch += {'arm': arm_ss}
>> -- 
>> 2.35.3
>>

Re: [PATCH 1/5] target/arm: only build psci for TCG

[Alexander Graf]

Hey Claudio,

On 19.12.22 09:37, Claudio Fontana wrote:
>
> On 12/16/22 22:59, Alexander Graf wrote:
>> Hi Claudio,
>>
>> If the PSCI implementation becomes TCG only, can we also move to a tcg accel directory? It slowly gets super confusing to keep track of which files are supposed to be generic target code and which ones TCG specific>
>> Alex
> Hi Alex, Fabiano, Peter and all,
>
> that was the plan but at the time of:
>
> https://lore.kernel.org/all/20210416162824.25131-1-cfontana@suse.de/
>
> Peter mentioned that HVF AArch64 might use that code too:
>
> https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg00509.html
>
> so from v2 to v3 the series changed to not move the code under tcg/ , expecting HVF to be reusing that code "soon".
>
> I see that your hvf code in hvf/ implements psci, is there some plan to reuse pieces from the tcg implementation now?

I originally reused the PSCI code in earlier versions of my hvf patch 
set, but then we realized that some functions like remote CPU reset are 
wired in a TCG specific view of the world with full target CPU register 
ownership. So if we want to actually share it, we'll need to abstract it 
up a level.

Hence I'd suggest to move it to a TCG directory for now and then later 
move it back into a generic helper if we want / need to. The code just 
simply isn't generic yet.

Or alternatively, you create a patch (set) to actually merge the 2 
implementations into a generic one again which then can live at a 
generic place :)


Alex

Re: [PATCH 1/5] target/arm: only build psci for TCG

[Claudio Fontana]

Ciao Alex,

On 12/19/22 11:47, Alexander Graf wrote:
> Hey Claudio,
> 
> On 19.12.22 09:37, Claudio Fontana wrote:
>>
>> On 12/16/22 22:59, Alexander Graf wrote:
>>> Hi Claudio,
>>>
>>> If the PSCI implementation becomes TCG only, can we also move to a tcg accel directory? It slowly gets super confusing to keep track of which files are supposed to be generic target code and which ones TCG specific>
>>> Alex
>> Hi Alex, Fabiano, Peter and all,
>>
>> that was the plan but at the time of:
>>
>> https://lore.kernel.org/all/20210416162824.25131-1-cfontana@suse.de/
>>
>> Peter mentioned that HVF AArch64 might use that code too:
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg00509.html
>>
>> so from v2 to v3 the series changed to not move the code under tcg/ , expecting HVF to be reusing that code "soon".
>>
>> I see that your hvf code in hvf/ implements psci, is there some plan to reuse pieces from the tcg implementation now?
> 
> I originally reused the PSCI code in earlier versions of my hvf patch 
> set, but then we realized that some functions like remote CPU reset are 
> wired in a TCG specific view of the world with full target CPU register 
> ownership. So if we want to actually share it, we'll need to abstract it 
> up a level.
> 
> Hence I'd suggest to move it to a TCG directory for now and then later 
> move it back into a generic helper if we want / need to. The code just 
> simply isn't generic yet.
> 
> Or alternatively, you create a patch (set) to actually merge the 2 
> implementations into a generic one again which then can live at a 
> generic place :)
> 
> 
> Alex

Thanks for the clarification, I'll leave the choice up to Fabiano now, since he is working on the series currently :-)

Ciao,

Claudio

Re: [PATCH 1/5] target/arm: only build psci for TCG

[Fabiano Rosas]

Claudio Fontana <cfontana@suse.de> writes:

> Ciao Alex,
>
> On 12/19/22 11:47, Alexander Graf wrote:
>> Hey Claudio,
>> 
>> On 19.12.22 09:37, Claudio Fontana wrote:
>>>
>>> On 12/16/22 22:59, Alexander Graf wrote:
>>>> Hi Claudio,
>>>>
>>>> If the PSCI implementation becomes TCG only, can we also move to a tcg accel directory? It slowly gets super confusing to keep track of which files are supposed to be generic target code and which ones TCG specific>
>>>> Alex
>>> Hi Alex, Fabiano, Peter and all,
>>>
>>> that was the plan but at the time of:
>>>
>>> https://lore.kernel.org/all/20210416162824.25131-1-cfontana@suse.de/
>>>
>>> Peter mentioned that HVF AArch64 might use that code too:
>>>
>>> https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg00509.html
>>>
>>> so from v2 to v3 the series changed to not move the code under tcg/ , expecting HVF to be reusing that code "soon".
>>>
>>> I see that your hvf code in hvf/ implements psci, is there some plan to reuse pieces from the tcg implementation now?
>> 
>> I originally reused the PSCI code in earlier versions of my hvf patch 
>> set, but then we realized that some functions like remote CPU reset are 
>> wired in a TCG specific view of the world with full target CPU register 
>> ownership. So if we want to actually share it, we'll need to abstract it 
>> up a level.
>> 
>> Hence I'd suggest to move it to a TCG directory for now and then later 
>> move it back into a generic helper if we want / need to. The code just 
>> simply isn't generic yet.
>> 
>> Or alternatively, you create a patch (set) to actually merge the 2 
>> implementations into a generic one again which then can live at a 
>> generic place :)
>> 
>> 
>> Alex
>
> Thanks for the clarification, I'll leave the choice up to Fabiano now, since he is working on the series currently :-)
>
> Ciao,
>
> Claudio

Hello, thank you all for the comments.

I like the idea of merging the two implementations. However, I won't get
to it anytime soon. There's still ~70 patches in the original series
that I need to understand, rebase and test, including the introduction
of the tcg directory.

I'd say we merge this as is now, since this patch has no
dependencies. Later when I introduce the tcg directory I can move the
code there along with the other tcg-only files. I'll take note to come
back to the PSCI code as well.

Re: [PATCH 3/5] target/arm: wrap semihosting and psci calls with tcg_enabled

[Fabiano Rosas]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 12/16/22 13:29, Fabiano Rosas wrote:
>> -    if (arm_is_psci_call(cpu, cs->exception_index)) {
>> -        arm_handle_psci_call(cpu);
>> -        qemu_log_mask(CPU_LOG_INT, "...handled as PSCI call\n");
>> -        return;
>> -    }
>> +    if (tcg_enabled()) {
>> +        if (arm_is_psci_call(cpu, cs->exception_index)) {
>
> This could be
>
>      if (tcg_enabled() && arm_is_psci_call(...))
>
> because...
>
>> -    /*
>> -     * Semihosting semantics depend on the register width of the code
>> -     * that caused the exception, not the target exception level, so
>> -     * must be handled here.
>> -     */
>> +        /*
>> +         * Semihosting semantics depend on the register width of the code
>> +         * that caused the exception, not the target exception level, so
>> +         * must be handled here.
>> +         */
>>   #ifdef CONFIG_TCG
>> -    if (cs->exception_index == EXCP_SEMIHOST) {
>> -        tcg_handle_semihosting(cs);
>> -        return;
>> -    }
>> +        if (cs->exception_index == EXCP_SEMIHOST) {
>
> If you were able to replace the ifdef, that would be one thing, but since you aren't I 
> don't think this requires a separate check.  There is no way to generate EXCP_SEMIHOST 
> except via TCG.

Right, I had to keep the ifdef that was being removed in the original
patch because tcg_handle_semihosting had moved elsewhere in Claudio's
series.

>
> I guess I don't insist, since you're working toward Claudio's much larger patch set, so
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

If you don't mind I'll leave like this then, unless this comes to a v2.

Thank you

Re: [PATCH 5/5] target/arm: only perform TCG cpu and machine inits if TCG enabled

[Fabiano Rosas]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 12/16/22 13:29, Fabiano Rosas wrote:
>> -    /*
>> -     * Misaligned thumb pc is architecturally impossible.
>> -     * We have an assert in thumb_tr_translate_insn to verify this.
>> -     * Fail an incoming migrate to avoid this assert.
>> -     */
>> -    if (!is_a64(env) && env->thumb && (env->regs[15] & 1)) {
>> -        return -1;
>> -    }
>> +        /*
>> +         * Misaligned thumb pc is architecturally impossible.
>> +         * We have an assert in thumb_tr_translate_insn to verify this.
>> +         * Fail an incoming migrate to avoid this assert.
>> +         */
>> +        if (!is_a64(env) && env->thumb && (env->regs[15] & 1)) {
>> +            return -1;
>> +        }
>
> This is a sanity check rejecting malformed vmsave.  While hw virt won't have the same 
> assert as mentioned in the comment, it won't be happy and will raise some sort of cpu 
> exception later.  I think it's better to reject the bad vmload early.  I suppose we could 
> expand the comment to that effect, so that it doesn't appear to be wholly tcg inspired.

I see, I'll leave it out of tcg_enabled() and update the comment.

> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
>
> r~

Re: [PATCH 1/5] target/arm: only build psci for TCG

[Alexander Graf]

Hey Fabiano,

On 19.12.22 12:42, Fabiano Rosas wrote:
> Claudio Fontana <cfontana@suse.de> writes:
>
>> Ciao Alex,
>>
>> On 12/19/22 11:47, Alexander Graf wrote:
>>> Hey Claudio,
>>>
>>> On 19.12.22 09:37, Claudio Fontana wrote:
>>>> On 12/16/22 22:59, Alexander Graf wrote:
>>>>> Hi Claudio,
>>>>>
>>>>> If the PSCI implementation becomes TCG only, can we also move to a tcg accel directory? It slowly gets super confusing to keep track of which files are supposed to be generic target code and which ones TCG specific>
>>>>> Alex
>>>> Hi Alex, Fabiano, Peter and all,
>>>>
>>>> that was the plan but at the time of:
>>>>
>>>> https://lore.kernel.org/all/20210416162824.25131-1-cfontana@suse.de/
>>>>
>>>> Peter mentioned that HVF AArch64 might use that code too:
>>>>
>>>> https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg00509.html
>>>>
>>>> so from v2 to v3 the series changed to not move the code under tcg/ , expecting HVF to be reusing that code "soon".
>>>>
>>>> I see that your hvf code in hvf/ implements psci, is there some plan to reuse pieces from the tcg implementation now?
>>> I originally reused the PSCI code in earlier versions of my hvf patch
>>> set, but then we realized that some functions like remote CPU reset are
>>> wired in a TCG specific view of the world with full target CPU register
>>> ownership. So if we want to actually share it, we'll need to abstract it
>>> up a level.
>>>
>>> Hence I'd suggest to move it to a TCG directory for now and then later
>>> move it back into a generic helper if we want / need to. The code just
>>> simply isn't generic yet.
>>>
>>> Or alternatively, you create a patch (set) to actually merge the 2
>>> implementations into a generic one again which then can live at a
>>> generic place :)
>>>
>>>
>>> Alex
>> Thanks for the clarification, I'll leave the choice up to Fabiano now, since he is working on the series currently :-)
>>
>> Ciao,
>>
>> Claudio
> Hello, thank you all for the comments.
>
> I like the idea of merging the two implementations. However, I won't get
> to it anytime soon. There's still ~70 patches in the original series
> that I need to understand, rebase and test, including the introduction
> of the tcg directory.


Sure, I am definitely fine with leaving them separate for now as well :).


> I'd say we merge this as is now, since this patch has no
> dependencies. Later when I introduce the tcg directory I can move the
> code there along with the other tcg-only files. I'll take note to come
> back to the PSCI code as well.

I'm confused about the patch ordering :). Why is it easier to move the 
psci.c compilation target from generic to an if(CONFIG_TCG) only to 
later move it into a tcg/ directory? Wouldn't it be easier to create a 
tcg/ directory from the start and just put it there?

The current approach just looks like duplicate effort to me.


Alex

Re: [PATCH 1/5] target/arm: only build psci for TCG

[Fabiano Rosas]

Alexander Graf <agraf@csgraf.de> writes:

> Hey Fabiano,
>
> On 19.12.22 12:42, Fabiano Rosas wrote:
>> Claudio Fontana <cfontana@suse.de> writes:
>>
>>> Ciao Alex,
>>>
>>> On 12/19/22 11:47, Alexander Graf wrote:
>>>> Hey Claudio,
>>>>
>>>> On 19.12.22 09:37, Claudio Fontana wrote:
>>>>> On 12/16/22 22:59, Alexander Graf wrote:
>>>>>> Hi Claudio,
>>>>>>
>>>>>> If the PSCI implementation becomes TCG only, can we also move to a tcg accel directory? It slowly gets super confusing to keep track of which files are supposed to be generic target code and which ones TCG specific>
>>>>>> Alex
>>>>> Hi Alex, Fabiano, Peter and all,
>>>>>
>>>>> that was the plan but at the time of:
>>>>>
>>>>> https://lore.kernel.org/all/20210416162824.25131-1-cfontana@suse.de/
>>>>>
>>>>> Peter mentioned that HVF AArch64 might use that code too:
>>>>>
>>>>> https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg00509.html
>>>>>
>>>>> so from v2 to v3 the series changed to not move the code under tcg/ , expecting HVF to be reusing that code "soon".
>>>>>
>>>>> I see that your hvf code in hvf/ implements psci, is there some plan to reuse pieces from the tcg implementation now?
>>>> I originally reused the PSCI code in earlier versions of my hvf patch
>>>> set, but then we realized that some functions like remote CPU reset are
>>>> wired in a TCG specific view of the world with full target CPU register
>>>> ownership. So if we want to actually share it, we'll need to abstract it
>>>> up a level.
>>>>
>>>> Hence I'd suggest to move it to a TCG directory for now and then later
>>>> move it back into a generic helper if we want / need to. The code just
>>>> simply isn't generic yet.
>>>>
>>>> Or alternatively, you create a patch (set) to actually merge the 2
>>>> implementations into a generic one again which then can live at a
>>>> generic place :)
>>>>
>>>>
>>>> Alex
>>> Thanks for the clarification, I'll leave the choice up to Fabiano now, since he is working on the series currently :-)
>>>
>>> Ciao,
>>>
>>> Claudio
>> Hello, thank you all for the comments.
>>
>> I like the idea of merging the two implementations. However, I won't get
>> to it anytime soon. There's still ~70 patches in the original series
>> that I need to understand, rebase and test, including the introduction
>> of the tcg directory.
>
>
> Sure, I am definitely fine with leaving them separate for now as well :).
>
>
>> I'd say we merge this as is now, since this patch has no
>> dependencies. Later when I introduce the tcg directory I can move the
>> code there along with the other tcg-only files. I'll take note to come
>> back to the PSCI code as well.
>
> I'm confused about the patch ordering :). Why is it easier to move the 
> psci.c compilation target from generic to an if(CONFIG_TCG) only to 
> later move it into a tcg/ directory?

It's a simple patch, so the overhead didn't cross my mind. But you are
right, this could go directly into tcg/ without having to put it under
CONFIG_TCG first.

> Wouldn't it be easier to create a 
> tcg/ directory from the start and just put it there?

I don't know about "from the start". At this point we cannot have a
single commit moving everything into the tcg/ directory because some
files still contain tcg + non-tcg code. We would end up with several
commits moving files into tcg/ along the history, which I think results
in a poor experience when inspecting the log later on (git blame and so
on). So my idea was to split as much as I can upfront and only later
move everything into the directory.

I'm also rebasing this series [1] from 2021, which means I'd rather have
small chunks of code moved under CONFIG_TCG that I can build-test with
--disable-tcg (even though the build doesn't finish, I can see the
number of errors going down), than to move non-tcg code into tcg/ and
then pull it out later like in the original series.

1- https://lore.kernel.org/r/20210416162824.25131-1-cfontana@suse.de

But hey, that's just my reasoning, no strong feelings about it.

Re: [PATCH 1/5] target/arm: only build psci for TCG

[Alexander Graf]

On 20.12.22 14:53, Fabiano Rosas wrote:
> Alexander Graf <agraf@csgraf.de> writes:
>
>> Hey Fabiano,
>>
>> On 19.12.22 12:42, Fabiano Rosas wrote:
>>> Claudio Fontana <cfontana@suse.de> writes:
>>>
>>>> Ciao Alex,
>>>>
>>>> On 12/19/22 11:47, Alexander Graf wrote:
>>>>> Hey Claudio,
>>>>>
>>>>> On 19.12.22 09:37, Claudio Fontana wrote:
>>>>>> On 12/16/22 22:59, Alexander Graf wrote:
>>>>>>> Hi Claudio,
>>>>>>>
>>>>>>> If the PSCI implementation becomes TCG only, can we also move to a tcg accel directory? It slowly gets super confusing to keep track of which files are supposed to be generic target code and which ones TCG specific>
>>>>>>> Alex
>>>>>> Hi Alex, Fabiano, Peter and all,
>>>>>>
>>>>>> that was the plan but at the time of:
>>>>>>
>>>>>> https://lore.kernel.org/all/20210416162824.25131-1-cfontana@suse.de/
>>>>>>
>>>>>> Peter mentioned that HVF AArch64 might use that code too:
>>>>>>
>>>>>> https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg00509.html
>>>>>>
>>>>>> so from v2 to v3 the series changed to not move the code under tcg/ , expecting HVF to be reusing that code "soon".
>>>>>>
>>>>>> I see that your hvf code in hvf/ implements psci, is there some plan to reuse pieces from the tcg implementation now?
>>>>> I originally reused the PSCI code in earlier versions of my hvf patch
>>>>> set, but then we realized that some functions like remote CPU reset are
>>>>> wired in a TCG specific view of the world with full target CPU register
>>>>> ownership. So if we want to actually share it, we'll need to abstract it
>>>>> up a level.
>>>>>
>>>>> Hence I'd suggest to move it to a TCG directory for now and then later
>>>>> move it back into a generic helper if we want / need to. The code just
>>>>> simply isn't generic yet.
>>>>>
>>>>> Or alternatively, you create a patch (set) to actually merge the 2
>>>>> implementations into a generic one again which then can live at a
>>>>> generic place :)
>>>>>
>>>>>
>>>>> Alex
>>>> Thanks for the clarification, I'll leave the choice up to Fabiano now, since he is working on the series currently :-)
>>>>
>>>> Ciao,
>>>>
>>>> Claudio
>>> Hello, thank you all for the comments.
>>>
>>> I like the idea of merging the two implementations. However, I won't get
>>> to it anytime soon. There's still ~70 patches in the original series
>>> that I need to understand, rebase and test, including the introduction
>>> of the tcg directory.
>>
>> Sure, I am definitely fine with leaving them separate for now as well :).
>>
>>
>>> I'd say we merge this as is now, since this patch has no
>>> dependencies. Later when I introduce the tcg directory I can move the
>>> code there along with the other tcg-only files. I'll take note to come
>>> back to the PSCI code as well.
>> I'm confused about the patch ordering :). Why is it easier to move the
>> psci.c compilation target from generic to an if(CONFIG_TCG) only to
>> later move it into a tcg/ directory?
> It's a simple patch, so the overhead didn't cross my mind. But you are
> right, this could go directly into tcg/ without having to put it under
> CONFIG_TCG first.


I'm sure more like this will follow, and it will be a lot easier on 
everyone if the pattern is going to be "move tcg specific code to tcg/ 
and leave generic code in the main directory".


>
>> Wouldn't it be easier to create a
>> tcg/ directory from the start and just put it there?
> I don't know about "from the start". At this point we cannot have a
> single commit moving everything into the tcg/ directory because some
> files still contain tcg + non-tcg code.


Yes, the only thing the initial commit at the start would do is create 
the directory and ninja config, pretty much nothing else. All follow-on 
commits then split the currently entangled code into tcg/ once code is 
clearly separate :).


I believe this was also the approach Claudio took in his patch set last 
year, and I find it very reasonable. It allows you to stop at any point 
mid-way.


> We would end up with several
> commits moving files into tcg/ along the history, which I think results
> in a poor experience when inspecting the log later on (git blame and so
> on). So my idea was to split as much as I can upfront and only later
> move everything into the directory.


Quite the opposite: Please make sure to move everything slowly at a 
digestible pace. There is no way you will get 100 patches in at once. 
Make sure you can cut off at any point in between.

What you basically want is to move from "target/arm is tcg+generic code" 
to "target/arm is generic, target/arm/tcg is tcg code". You will be in a 
transitional phase along the way whatever you do, so just make it 
"target/arm is tcg+generic code, target/arm/tcg is tcg code" while 
things are in flight and have a final commit that indicates the 
conversion is done.


> I'm also rebasing this series [1] from 2021, which means I'd rather have
> small chunks of code moved under CONFIG_TCG that I can build-test with
> --disable-tcg (even though the build doesn't finish, I can see the
> number of errors going down), than to move non-tcg code into tcg/ and
> then pull it out later like in the original series.


I think we're saying the same thing. Please don't move non-tcg code into 
tcg/. Just move files that are absolutely clearly TCG into tcg/ right 
from the start. The psci.c is a good example for that. translate*.c and 
op-helper.c would be another.


Alex


> 1- https://lore.kernel.org/r/20210416162824.25131-1-cfontana@suse.de
>
> But hey, that's just my reasoning, no strong feelings about it.

Re: [PATCH 1/5] target/arm: only build psci for TCG

[Fabiano Rosas]

Alexander Graf <agraf@csgraf.de> writes:

>>> I'm confused about the patch ordering :). Why is it easier to move the
>>> psci.c compilation target from generic to an if(CONFIG_TCG) only to
>>> later move it into a tcg/ directory?
>> It's a simple patch, so the overhead didn't cross my mind. But you are
>> right, this could go directly into tcg/ without having to put it under
>> CONFIG_TCG first.
>
>
> I'm sure more like this will follow, and it will be a lot easier on 
> everyone if the pattern is going to be "move tcg specific code to tcg/ 
> and leave generic code in the main directory".

Ok, so I'll drop this patch from this series and resend it along with
the rest of the code movement to the tcg/ directory.

> Quite the opposite: Please make sure to move everything slowly at a 
> digestible pace. There is no way you will get 100 patches in at once. 
> Make sure you can cut off at any point in between.

I meant having code under CONFIG_TCG first and later moving to tcg/. So
we separate moving the code from figuring out if it should be
moved. There was no implication of speed, size or indigestibility =).

>
> What you basically want is to move from "target/arm is tcg+generic code" 
> to "target/arm is generic, target/arm/tcg is tcg code". You will be in a 
> transitional phase along the way whatever you do, so just make it 
> "target/arm is tcg+generic code, target/arm/tcg is tcg code" while 
> things are in flight and have a final commit that indicates the 
> conversion is done.
>
>
>> I'm also rebasing this series [1] from 2021, which means I'd rather have
>> small chunks of code moved under CONFIG_TCG that I can build-test with
>> --disable-tcg (even though the build doesn't finish, I can see the
>> number of errors going down), than to move non-tcg code into tcg/ and
>> then pull it out later like in the original series.
>
>
> I think we're saying the same thing. Please don't move non-tcg code into 
> tcg/. Just move files that are absolutely clearly TCG into tcg/ right 
> from the start. The psci.c is a good example for that. translate*.c and 
> op-helper.c would be another.

Yeah, I think we agree. Thanks for taking the time to spell it out.