This patch adds alignment checks in the load operations in the VLLDM
instruction, and in the store operations in the VLSTM instruction.

Manual alignment checks in the both helpers are retained because they
enforce an 8-byte alignment requirement (instead of the 4-byte alignment for
ordinary long loads/stores). References to cpu_*_data_* are still replaced
with cpu_*_mmu(), so that the individual word accesses themselves also
perform the standard alignment checks, in keeping with the ARM pseudocode.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1154
Signed-off-by: William Kosasih <kosasihwilliam4@gmail.com>
---
 target/arm/tcg/m_helper.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/target/arm/tcg/m_helper.c b/target/arm/tcg/m_helper.c
index 6614719832..251e12edf9 100644
--- a/target/arm/tcg/m_helper.c
+++ b/target/arm/tcg/m_helper.c
@@ -1048,6 +1048,9 @@ void HELPER(v7m_vlstm)(CPUARMState *env, uint32_t fptr)
     bool s = env->v7m.fpccr[M_REG_S] & R_V7M_FPCCR_S_MASK;
     bool lspact = env->v7m.fpccr[s] & R_V7M_FPCCR_LSPACT_MASK;
     uintptr_t ra = GETPC();
+    ARMMMUIdx mmu_idx = arm_mmu_idx(env);
+    MemOpIdx oi = make_memop_idx(MO_TEUL | MO_ALIGN,
+                                 arm_to_core_mmu_idx(mmu_idx));
 
     assert(env->v7m.secure);
 
@@ -1073,7 +1076,7 @@ void HELPER(v7m_vlstm)(CPUARMState *env, uint32_t fptr)
      * Note that we do not use v7m_stack_write() here, because the
      * accesses should not set the FSR bits for stacking errors if they
      * fail. (In pseudocode terms, they are AccType_NORMAL, not AccType_STACK
-     * or AccType_LAZYFP). Faults in cpu_stl_data_ra() will throw exceptions
+     * or AccType_LAZYFP). Faults in cpu_stl_mmu() will throw exceptions
      * and longjmp out.
      */
     if (!(env->v7m.fpccr[M_REG_S] & R_V7M_FPCCR_LSPEN_MASK)) {
@@ -1089,12 +1092,12 @@ void HELPER(v7m_vlstm)(CPUARMState *env, uint32_t fptr)
             if (i >= 16) {
                 faddr += 8; /* skip the slot for the FPSCR */
             }
-            cpu_stl_data_ra(env, faddr, slo, ra);
-            cpu_stl_data_ra(env, faddr + 4, shi, ra);
+            cpu_stl_mmu(env, faddr, slo, oi, ra);
+            cpu_stl_mmu(env, faddr + 4, shi, oi, ra);
         }
-        cpu_stl_data_ra(env, fptr + 0x40, vfp_get_fpscr(env), ra);
+        cpu_stl_mmu(env, fptr + 0x40, vfp_get_fpscr(env), oi, ra);
         if (cpu_isar_feature(aa32_mve, cpu)) {
-            cpu_stl_data_ra(env, fptr + 0x44, env->v7m.vpr, ra);
+            cpu_stl_mmu(env, fptr + 0x44, env->v7m.vpr, oi, ra);
         }
 
         /*
@@ -1121,6 +1124,9 @@ void HELPER(v7m_vlldm)(CPUARMState *env, uint32_t fptr)
 {
     ARMCPU *cpu = env_archcpu(env);
     uintptr_t ra = GETPC();
+    ARMMMUIdx mmu_idx = arm_mmu_idx(env);
+    MemOpIdx oi = make_memop_idx(MO_TEUL | MO_ALIGN,
+                                 arm_to_core_mmu_idx(mmu_idx));
 
     /* fptr is the value of Rn, the frame pointer we load the FP regs from */
     assert(env->v7m.secure);
@@ -1155,16 +1161,16 @@ void HELPER(v7m_vlldm)(CPUARMState *env, uint32_t fptr)
                 faddr += 8; /* skip the slot for the FPSCR and VPR */
             }
 
-            slo = cpu_ldl_data_ra(env, faddr, ra);
-            shi = cpu_ldl_data_ra(env, faddr + 4, ra);
+            slo = cpu_ldl_mmu(env, faddr, oi, ra);
+            shi = cpu_ldl_mmu(env, faddr + 4, oi, ra);
 
             dn = (uint64_t) shi << 32 | slo;
             *aa32_vfp_dreg(env, i / 2) = dn;
         }
-        fpscr = cpu_ldl_data_ra(env, fptr + 0x40, ra);
+        fpscr = cpu_ldl_mmu(env, fptr + 0x40, oi, ra);
         vfp_set_fpscr(env, fpscr);
         if (cpu_isar_feature(aa32_mve, cpu)) {
-            env->v7m.vpr = cpu_ldl_data_ra(env, fptr + 0x44, ra);
+            env->v7m.vpr = cpu_ldl_mmu(env, fptr + 0x44, oi, ra);
         }
     }
 
-- 
2.48.1

On 7/1/25 04:31, William Kosasih wrote:
> This patch adds alignment checks in the load operations in the VLLDM
> instruction, and in the store operations in the VLSTM instruction.
> 
> Manual alignment checks in the both helpers are retained because they
> enforce an 8-byte alignment requirement (instead of the 4-byte alignment for
> ordinary long loads/stores). References to cpu_*_data_* are still replaced
> with cpu_*_mmu(), so that the individual word accesses themselves also
> perform the standard alignment checks, in keeping with the ARM pseudocode.

So... this merely makes this function match the pseudocode, it doesn't actually fix a bug.
This description should be fixed to reflect that.

> diff --git a/target/arm/tcg/m_helper.c b/target/arm/tcg/m_helper.c
> index 6614719832..251e12edf9 100644
> --- a/target/arm/tcg/m_helper.c
> +++ b/target/arm/tcg/m_helper.c
> @@ -1048,6 +1048,9 @@ void HELPER(v7m_vlstm)(CPUARMState *env, uint32_t fptr)
>       bool s = env->v7m.fpccr[M_REG_S] & R_V7M_FPCCR_S_MASK;
>       bool lspact = env->v7m.fpccr[s] & R_V7M_FPCCR_LSPACT_MASK;
>       uintptr_t ra = GETPC();
> +    ARMMMUIdx mmu_idx = arm_mmu_idx(env);
> +    MemOpIdx oi = make_memop_idx(MO_TEUL | MO_ALIGN,
> +                                 arm_to_core_mmu_idx(mmu_idx));
>   
>       assert(env->v7m.secure);
>   
> @@ -1073,7 +1076,7 @@ void HELPER(v7m_vlstm)(CPUARMState *env, uint32_t fptr)
>        * Note that we do not use v7m_stack_write() here, because the
>        * accesses should not set the FSR bits for stacking errors if they
>        * fail. (In pseudocode terms, they are AccType_NORMAL, not AccType_STACK
> -     * or AccType_LAZYFP). Faults in cpu_stl_data_ra() will throw exceptions
> +     * or AccType_LAZYFP). Faults in cpu_stl_mmu() will throw exceptions
>        * and longjmp out.
>        */
>       if (!(env->v7m.fpccr[M_REG_S] & R_V7M_FPCCR_LSPEN_MASK)) {
> @@ -1089,12 +1092,12 @@ void HELPER(v7m_vlstm)(CPUARMState *env, uint32_t fptr)
>               if (i >= 16) {
>                   faddr += 8; /* skip the slot for the FPSCR */
>               }
> -            cpu_stl_data_ra(env, faddr, slo, ra);
> -            cpu_stl_data_ra(env, faddr + 4, shi, ra);
> +            cpu_stl_mmu(env, faddr, slo, oi, ra);
> +            cpu_stl_mmu(env, faddr + 4, shi, oi, ra);

This is an improvement because the mmu index is resolved once, instead of within every 
call to cpu_stl_data_ra.


r~