[PATCH] target/arm: Introduce aarch64_set_svcr

Richard Henderson posted 1 patch 1 year, 2 months ago
There is a newer version of this series
target/arm/cpu.h              |  2 +-
target/arm/helper-sme.h       |  3 +--
linux-user/aarch64/cpu_loop.c | 11 ++--------
linux-user/aarch64/signal.c   | 13 ++---------
target/arm/helper.c           | 41 ++++++++++++++++++++++++++++++++---
target/arm/sme_helper.c       | 37 ++-----------------------------
target/arm/translate-a64.c    | 19 ++++++----------
7 files changed, 53 insertions(+), 73 deletions(-)
[PATCH] target/arm: Introduce aarch64_set_svcr
Posted by Richard Henderson 1 year, 2 months ago
Unify the two helper_set_pstate_{sm,za} in this function.
Do not call helper_* functions from svcr_write.
Cleans up linux-user usage by consolodating logic.

Cc: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---

Fabiano, I expect this to replace much of your

  [RFC PATCH v2 07/19] target/arm: Move helper_set_pstate_* into cpregs.c

r~
---
 target/arm/cpu.h              |  2 +-
 target/arm/helper-sme.h       |  3 +--
 linux-user/aarch64/cpu_loop.c | 11 ++--------
 linux-user/aarch64/signal.c   | 13 ++---------
 target/arm/helper.c           | 41 ++++++++++++++++++++++++++++++++---
 target/arm/sme_helper.c       | 37 ++-----------------------------
 target/arm/translate-a64.c    | 19 ++++++----------
 7 files changed, 53 insertions(+), 73 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index bf2bce046d..a471add499 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1123,7 +1123,7 @@ int aarch64_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
 void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
 void aarch64_sve_change_el(CPUARMState *env, int old_el,
                            int new_el, bool el0_a64);
-void arm_reset_sve_state(CPUARMState *env);
+void aarch64_set_svcr(CPUARMState *env, uint64_t new, uint64_t mask);
 
 /*
  * SVE registers are encoded in KVM's memory in an endianness-invariant format.
diff --git a/target/arm/helper-sme.h b/target/arm/helper-sme.h
index d2d544a696..27eef49a11 100644
--- a/target/arm/helper-sme.h
+++ b/target/arm/helper-sme.h
@@ -17,8 +17,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 
-DEF_HELPER_FLAGS_2(set_pstate_sm, TCG_CALL_NO_RWG, void, env, i32)
-DEF_HELPER_FLAGS_2(set_pstate_za, TCG_CALL_NO_RWG, void, env, i32)
+DEF_HELPER_FLAGS_3(set_svcr, TCG_CALL_NO_RWG, void, env, i32, i32)
 
 DEF_HELPER_FLAGS_3(sme_zero, TCG_CALL_NO_RWG, void, env, i32, i32)
 
diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
index 9875d609a9..2e2f7cf218 100644
--- a/linux-user/aarch64/cpu_loop.c
+++ b/linux-user/aarch64/cpu_loop.c
@@ -89,15 +89,8 @@ void cpu_loop(CPUARMState *env)
 
         switch (trapnr) {
         case EXCP_SWI:
-            /*
-             * On syscall, PSTATE.ZA is preserved, along with the ZA matrix.
-             * PSTATE.SM is cleared, per SMSTOP, which does ResetSVEState.
-             */
-            if (FIELD_EX64(env->svcr, SVCR, SM)) {
-                env->svcr = FIELD_DP64(env->svcr, SVCR, SM, 0);
-                arm_rebuild_hflags(env);
-                arm_reset_sve_state(env);
-            }
+            /* On syscall, PSTATE.ZA is preserved, PSTATE.SM is cleared. */
+            aarch64_set_svcr(env, 0, R_SVCR_SM_MASK);
             ret = do_syscall(env,
                              env->xregs[8],
                              env->xregs[0],
diff --git a/linux-user/aarch64/signal.c b/linux-user/aarch64/signal.c
index 6a2c6e06d2..b265cfd470 100644
--- a/linux-user/aarch64/signal.c
+++ b/linux-user/aarch64/signal.c
@@ -665,17 +665,8 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
         env->btype = 2;
     }
 
-    /*
-     * Invoke the signal handler with both SM and ZA disabled.
-     * When clearing SM, ResetSVEState, per SMSTOP.
-     */
-    if (FIELD_EX64(env->svcr, SVCR, SM)) {
-        arm_reset_sve_state(env);
-    }
-    if (env->svcr) {
-        env->svcr = 0;
-        arm_rebuild_hflags(env);
-    }
+    /* Invoke the signal handler with both SM and ZA disabled. */
+    aarch64_set_svcr(env, 0, R_SVCR_SM_MASK | R_SVCR_ZA_MASK);
 
     if (info) {
         tswap_siginfo(&frame->info, info);
diff --git a/target/arm/helper.c b/target/arm/helper.c
index cee3804354..1d74b95971 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6722,12 +6722,47 @@ static CPAccessResult access_esm(CPUARMState *env, const ARMCPRegInfo *ri,
     return CP_ACCESS_OK;
 }
 
+/* ResetSVEState */
+static void arm_reset_sve_state(CPUARMState *env)
+{
+    memset(env->vfp.zregs, 0, sizeof(env->vfp.zregs));
+    /* Recall that FFR is stored as pregs[16]. */
+    memset(env->vfp.pregs, 0, sizeof(env->vfp.pregs));
+    vfp_set_fpcr(env, 0x0800009f);
+}
+
+void aarch64_set_svcr(CPUARMState *env, uint64_t new, uint64_t mask)
+{
+    uint64_t change = (env->svcr ^ new) & mask;
+
+    if (change == 0) {
+        return;
+    }
+    env->svcr ^= change;
+
+    if (change & R_SVCR_SM_MASK) {
+        arm_reset_sve_state(env);
+    }
+
+    /*
+     * ResetSMEState.
+     *
+     * SetPSTATE_ZA zeros on enable and disable.  We can zero this only
+     * on enable: while disabled, the storage is inaccessible and the
+     * value does not matter.  We're not saving the storage in vmstate
+     * when disabled either.
+     */
+    if (change & new & R_SVCR_ZA_MASK) {
+        memset(env->zarray, 0, sizeof(env->zarray));
+    }
+
+    arm_rebuild_hflags(env);
+}
+
 static void svcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                        uint64_t value)
 {
-    helper_set_pstate_sm(env, FIELD_EX64(value, SVCR, SM));
-    helper_set_pstate_za(env, FIELD_EX64(value, SVCR, ZA));
-    arm_rebuild_hflags(env);
+    aarch64_set_svcr(env, value, -1);
 }
 
 static void smcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
diff --git a/target/arm/sme_helper.c b/target/arm/sme_helper.c
index f891306bb9..1e67fcac30 100644
--- a/target/arm/sme_helper.c
+++ b/target/arm/sme_helper.c
@@ -29,42 +29,9 @@
 #include "vec_internal.h"
 #include "sve_ldst_internal.h"
 
-/* ResetSVEState */
-void arm_reset_sve_state(CPUARMState *env)
+void helper_set_svcr(CPUARMState *env, uint32_t val, uint32_t mask)
 {
-    memset(env->vfp.zregs, 0, sizeof(env->vfp.zregs));
-    /* Recall that FFR is stored as pregs[16]. */
-    memset(env->vfp.pregs, 0, sizeof(env->vfp.pregs));
-    vfp_set_fpcr(env, 0x0800009f);
-}
-
-void helper_set_pstate_sm(CPUARMState *env, uint32_t i)
-{
-    if (i == FIELD_EX64(env->svcr, SVCR, SM)) {
-        return;
-    }
-    env->svcr ^= R_SVCR_SM_MASK;
-    arm_reset_sve_state(env);
-}
-
-void helper_set_pstate_za(CPUARMState *env, uint32_t i)
-{
-    if (i == FIELD_EX64(env->svcr, SVCR, ZA)) {
-        return;
-    }
-    env->svcr ^= R_SVCR_ZA_MASK;
-
-    /*
-     * ResetSMEState.
-     *
-     * SetPSTATE_ZA zeros on enable and disable.  We can zero this only
-     * on enable: while disabled, the storage is inaccessible and the
-     * value does not matter.  We're not saving the storage in vmstate
-     * when disabled either.
-     */
-    if (i) {
-        memset(env->zarray, 0, sizeof(env->zarray));
-    }
+    aarch64_set_svcr(env, val, mask);
 }
 
 void helper_sme_zero(CPUARMState *env, uint32_t imm, uint32_t svl)
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 2ee171f249..19cf371c4c 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1841,19 +1841,14 @@ static void handle_msr_i(DisasContext *s, uint32_t insn,
             goto do_unallocated;
         }
         if (sme_access_check(s)) {
-            bool i = crm & 1;
-            bool changed = false;
+            int old = s->pstate_sm | (s->pstate_za << 1);
+            int new = (crm & 1) * 3;
+            int msk = (crm >> 1) & 3;
 
-            if ((crm & 2) && i != s->pstate_sm) {
-                gen_helper_set_pstate_sm(cpu_env, tcg_constant_i32(i));
-                changed = true;
-            }
-            if ((crm & 4) && i != s->pstate_za) {
-                gen_helper_set_pstate_za(cpu_env, tcg_constant_i32(i));
-                changed = true;
-            }
-            if (changed) {
-                gen_rebuild_hflags(s);
+            if ((old ^ new) & msk) {
+                /* At least one bit changes. */
+                gen_helper_set_svcr(cpu_env, tcg_constant_i32(new),
+                                    tcg_constant_i32(msk));
             } else {
                 s->base.is_jmp = DISAS_NEXT;
             }
-- 
2.34.1
Re: [PATCH] target/arm: Introduce aarch64_set_svcr
Posted by Fabiano Rosas 1 year, 2 months ago
Richard Henderson <richard.henderson@linaro.org> writes:

> Unify the two helper_set_pstate_{sm,za} in this function.
> Do not call helper_* functions from svcr_write.
> Cleans up linux-user usage by consolodating logic.
>
> Cc: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>
> Fabiano, I expect this to replace much of your
>
>   [RFC PATCH v2 07/19] target/arm: Move helper_set_pstate_* into cpregs.c

Thanks!

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Re: [PATCH] target/arm: Introduce aarch64_set_svcr
Posted by Philippe Mathieu-Daudé 1 year, 2 months ago
On 12/1/23 01:43, Richard Henderson wrote:
> Unify the two helper_set_pstate_{sm,za} in this function.
> Do not call helper_* functions from svcr_write.
> Cleans up linux-user usage by consolodating logic.
> 
> Cc: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> 
> Fabiano, I expect this to replace much of your
> 
>    [RFC PATCH v2 07/19] target/arm: Move helper_set_pstate_* into cpregs.c
> 
> r~
> ---
>   target/arm/cpu.h              |  2 +-
>   target/arm/helper-sme.h       |  3 +--
>   linux-user/aarch64/cpu_loop.c | 11 ++--------
>   linux-user/aarch64/signal.c   | 13 ++---------
>   target/arm/helper.c           | 41 ++++++++++++++++++++++++++++++++---
>   target/arm/sme_helper.c       | 37 ++-----------------------------
>   target/arm/translate-a64.c    | 19 ++++++----------
>   7 files changed, 53 insertions(+), 73 deletions(-)

Since this patch was a bit too hard to digest at once, I split it
in trivial steps here:
https://lore.kernel.org/qemu-devel/20230112102436.1913-1-philmd@linaro.org/

For whichever version you prefer:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>