[PATCH] tcg/arm: Don't emit UNPREDICTABLE LDRD with Rm == Rt or Rt+1

[PATCH] tcg/arm: Don't emit UNPREDICTABLE LDRD with Rm == Rt or Rt+1

[Richard Henderson]

The LDRD (register) instruction is UNPREDICTABLE if the Rm register
is the same as either Rt or Rt+1 (the two registers being loaded to).
We weren't making sure we avoided this, with the result that on some
host CPUs like the Cortex-A7 we would get a SIGILL because the CPU
chooses to UNDEF for this particular UNPREDICTABLE case.

Since we've already checked that datalo is aligned, we can simplify
the test vs the Rm operand by aligning it before comparison.  Check
for the two orderings before falling back to two ldr instructions.

We don't bother to do anything similar for tcg_out_ldrd_rwb(),
because it is only used in tcg_out_tlb_read() with a fixed set of
registers which don't overlap.

There is no equivalent UNPREDICTABLE case for STRD.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/896
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/arm/tcg-target.c.inc | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/tcg/arm/tcg-target.c.inc b/tcg/arm/tcg-target.c.inc
index e1ea69669c..4bc0420f4d 100644
--- a/tcg/arm/tcg-target.c.inc
+++ b/tcg/arm/tcg-target.c.inc
@@ -1689,8 +1689,21 @@ static void tcg_out_qemu_ld_index(TCGContext *s, MemOp opc,
         /* LDRD requires alignment; double-check that. */
         if (get_alignment_bits(opc) >= MO_64
             && (datalo & 1) == 0 && datahi == datalo + 1) {
-            tcg_out_ldrd_r(s, COND_AL, datalo, addrlo, addend);
-        } else if (scratch_addend) {
+            /*
+             * Rm (the second address op) must not overlap Rt or Rt + 1.
+             * Since datalo is aligned, we can simplify the test via alignment.
+             * Flip the two address arguments if that works.
+             */
+            if ((addend & ~1) != datalo) {
+                tcg_out_ldrd_r(s, COND_AL, datalo, addrlo, addend);
+                break;
+            }
+            if ((addrlo & ~1) != datalo) {
+                tcg_out_ldrd_r(s, COND_AL, datalo, addend, addrlo);
+                break;
+            }
+        }
+        if (scratch_addend) {
             tcg_out_ld32_rwb(s, COND_AL, datalo, addend, addrlo);
             tcg_out_ld32_12(s, COND_AL, datahi, addend, 4);
         } else {
-- 
2.25.1

Re: [PATCH] tcg/arm: Don't emit UNPREDICTABLE LDRD with Rm == Rt or Rt+1

[Alex Bennée]

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

> The LDRD (register) instruction is UNPREDICTABLE if the Rm register
> is the same as either Rt or Rt+1 (the two registers being loaded to).
> We weren't making sure we avoided this, with the result that on some
> host CPUs like the Cortex-A7 we would get a SIGILL because the CPU
> chooses to UNDEF for this particular UNPREDICTABLE case.
>
> Since we've already checked that datalo is aligned, we can simplify
> the test vs the Rm operand by aligning it before comparison.  Check
> for the two orderings before falling back to two ldr instructions.
>
> We don't bother to do anything similar for tcg_out_ldrd_rwb(),
> because it is only used in tcg_out_tlb_read() with a fixed set of
> registers which don't overlap.
>
> There is no equivalent UNPREDICTABLE case for STRD.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/896
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

The fix looks sane to me (although I can't test because it seems my
aarch32 on the SynQuacer does try it's best). So:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

A wider question. Is this something that can be handled the constraints
done by the register allocator? I assume that avoid direct aliasing if
needed?

-- 
Alex Bennée

Re: [PATCH] tcg/arm: Don't emit UNPREDICTABLE LDRD with Rm == Rt or Rt+1

[Richard Henderson]

On 3/11/22 06:02, Alex Bennée wrote:
> A wider question. Is this something that can be handled the constraints
> done by the register allocator? I assume that avoid direct aliasing if
> needed?

No.  We do have "allocate a non-overlapping register"; we don't have "allocate an aligned 
register pair", which *would* be helpful.

However, in this specific case "addend" is completely invisible to the register allocator, 
coming entirely from the backend's tlb implementation (or guest_base).


r~