]> xenbits.xensource.com Git - qemu-xen.git/commitdiff
Hexagon (target/hexagon) fix bug in mem_noshuf load exception
authorTaylor Simpson <tsimpson@quicinc.com>
Thu, 7 Jul 2022 21:05:46 +0000 (14:05 -0700)
committerTaylor Simpson <tsimpson@quicinc.com>
Tue, 19 Jul 2022 21:20:08 +0000 (14:20 -0700)
The semantics of a mem_noshuf packet are that the store effectively
happens before the load.  However, in cases where the load raises an
exception, we cannot simply execute the store first.

This change adds a probe to check that the load will not raise an
exception before executing the store.

If the load is predicated, this requires special handling.  We check
the condition before performing the probe.  Since, we need the EA to
perform the check, we move the GET_EA portion inside CHECK_NOSHUF_PRED.

Test case added in tests/tcg/hexagon/mem_noshuf_exception.c

Suggested-by: Alessandro Di Federico <ale@rev.ng>
Suggested-by: Anton Johansson <anjo@rev.ng>
Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20220707210546.15985-3-tsimpson@quicinc.com>

target/hexagon/gen_tcg.h
target/hexagon/genptr.c
target/hexagon/helper.h
target/hexagon/macros.h
target/hexagon/op_helper.c
tests/tcg/hexagon/Makefile.target
tests/tcg/hexagon/mem_noshuf_exception.c [new file with mode: 0644]

index b0b6b3644e4817e15cd52f35e26dc73eb8bef966..50634ac4593d00458531fcb60ef20467202f67f9 100644 (file)
     do { \
         TCGv LSB = tcg_temp_local_new(); \
         TCGLabel *label = gen_new_label(); \
-        GET_EA; \
+        tcg_gen_movi_tl(EA, 0); \
         PRED;  \
+        CHECK_NOSHUF_PRED(GET_EA, SIZE, LSB); \
         PRED_LOAD_CANCEL(LSB, EA); \
         tcg_gen_movi_tl(RdV, 0); \
-        CHECK_NOSHUF; \
         tcg_gen_brcondi_tl(TCG_COND_EQ, LSB, 0, label); \
-            fLOAD(1, SIZE, SIGN, EA, RdV); \
+        fLOAD(1, SIZE, SIGN, EA, RdV); \
         gen_set_label(label); \
         tcg_temp_free(LSB); \
     } while (0)
     do { \
         TCGv LSB = tcg_temp_local_new(); \
         TCGLabel *label = gen_new_label(); \
-        GET_EA; \
+        tcg_gen_movi_tl(EA, 0); \
         PRED;  \
+        CHECK_NOSHUF_PRED(GET_EA, 8, LSB); \
         PRED_LOAD_CANCEL(LSB, EA); \
         tcg_gen_movi_i64(RddV, 0); \
-        CHECK_NOSHUF; \
         tcg_gen_brcondi_tl(TCG_COND_EQ, LSB, 0, label); \
-            fLOAD(1, 8, u, EA, RddV); \
+        fLOAD(1, 8, u, EA, RddV); \
         gen_set_label(label); \
         tcg_temp_free(LSB); \
     } while (0)
index cd6af4bcebf395982af27ebb99b27ce1d9544ca8..8a334ba07b63a803e88a5fded410769decdfd1b3 100644 (file)
@@ -638,5 +638,12 @@ static void vec_to_qvec(size_t size, intptr_t dstoff, intptr_t srcoff)
     tcg_temp_free_i64(mask);
 }
 
+static void probe_noshuf_load(TCGv va, int s, int mi)
+{
+    TCGv size = tcg_constant_tl(s);
+    TCGv mem_idx = tcg_constant_tl(mi);
+    gen_helper_probe_noshuf_load(cpu_env, va, size, mem_idx);
+}
+
 #include "tcg_funcs_generated.c.inc"
 #include "tcg_func_table_generated.c.inc"
index c89aa4ed4d6d9d61b2f212dae2815df345ed5835..368f0b57085bdf9044ffe74d757a999aa22ae780 100644 (file)
@@ -104,6 +104,7 @@ DEF_HELPER_1(vwhist128q, void, env)
 DEF_HELPER_2(vwhist128m, void, env, s32)
 DEF_HELPER_2(vwhist128qm, void, env, s32)
 
+DEF_HELPER_4(probe_noshuf_load, void, env, i32, int, int)
 DEF_HELPER_2(probe_pkt_scalar_store_s0, void, env, int)
 DEF_HELPER_2(probe_hvx_stores, void, env, int)
 DEF_HELPER_3(probe_pkt_scalar_hvx_stores, void, env, int, int)
index a78e84faa430d2eef77c258689ec8a0d0a209a90..92eb8bbf05c464d0012b58b7e634877b3ff008d2 100644 (file)
  *
  *
  * For qemu, we look for a load in slot 0 when there is  a store in slot 1
- * in the same packet.  When we see this, we call a helper that merges the
- * bytes from the store buffer with the value loaded from memory.
+ * in the same packet.  When we see this, we call a helper that probes the
+ * load to make sure it doesn't fault.  Then, we process the store ahead of
+ * the actual load.
+
  */
-#define CHECK_NOSHUF \
+#define CHECK_NOSHUF(VA, SIZE) \
     do { \
+        if (insn->slot == 0 && pkt->pkt_has_store_s1) { \
+            probe_noshuf_load(VA, SIZE, ctx->mem_idx); \
+            process_store(ctx, pkt, 1); \
+        } \
+    } while (0)
+
+#define CHECK_NOSHUF_PRED(GET_EA, SIZE, PRED) \
+    do { \
+        TCGLabel *label = gen_new_label(); \
+        tcg_gen_brcondi_tl(TCG_COND_EQ, PRED, 0, label); \
+        GET_EA; \
+        if (insn->slot == 0 && pkt->pkt_has_store_s1) { \
+            probe_noshuf_load(EA, SIZE, ctx->mem_idx); \
+        } \
+        gen_set_label(label); \
         if (insn->slot == 0 && pkt->pkt_has_store_s1) { \
             process_store(ctx, pkt, 1); \
         } \
 
 #define MEM_LOAD1s(DST, VA) \
     do { \
-        CHECK_NOSHUF; \
+        CHECK_NOSHUF(VA, 1); \
         tcg_gen_qemu_ld8s(DST, VA, ctx->mem_idx); \
     } while (0)
 #define MEM_LOAD1u(DST, VA) \
     do { \
-        CHECK_NOSHUF; \
+        CHECK_NOSHUF(VA, 1); \
         tcg_gen_qemu_ld8u(DST, VA, ctx->mem_idx); \
     } while (0)
 #define MEM_LOAD2s(DST, VA) \
     do { \
-        CHECK_NOSHUF; \
+        CHECK_NOSHUF(VA, 2); \
         tcg_gen_qemu_ld16s(DST, VA, ctx->mem_idx); \
     } while (0)
 #define MEM_LOAD2u(DST, VA) \
     do { \
-        CHECK_NOSHUF; \
+        CHECK_NOSHUF(VA, 2); \
         tcg_gen_qemu_ld16u(DST, VA, ctx->mem_idx); \
     } while (0)
 #define MEM_LOAD4s(DST, VA) \
     do { \
-        CHECK_NOSHUF; \
+        CHECK_NOSHUF(VA, 4); \
         tcg_gen_qemu_ld32s(DST, VA, ctx->mem_idx); \
     } while (0)
 #define MEM_LOAD4u(DST, VA) \
     do { \
-        CHECK_NOSHUF; \
+        CHECK_NOSHUF(VA, 4); \
         tcg_gen_qemu_ld32s(DST, VA, ctx->mem_idx); \
     } while (0)
 #define MEM_LOAD8u(DST, VA) \
     do { \
-        CHECK_NOSHUF; \
+        CHECK_NOSHUF(VA, 8); \
         tcg_gen_qemu_ld64(DST, VA, ctx->mem_idx); \
     } while (0)
 
index a5ed819c045189cebcf22bc1f32ee896a31da732..085afc32746310dc3de6478e41c8a872ddad347d 100644 (file)
@@ -442,6 +442,17 @@ static void probe_store(CPUHexagonState *env, int slot, int mmu_idx)
     }
 }
 
+/*
+ * Called from a mem_noshuf packet to make sure the load doesn't
+ * raise an exception
+ */
+void HELPER(probe_noshuf_load)(CPUHexagonState *env, target_ulong va,
+                               int size, int mmu_idx)
+{
+    uintptr_t retaddr = GETPC();
+    probe_read(env, va, size, mmu_idx, retaddr);
+}
+
 /* Called during packet commit when there are two scalar stores */
 void HELPER(probe_pkt_scalar_store_s0)(CPUHexagonState *env, int mmu_idx)
 {
@@ -514,10 +525,12 @@ void HELPER(probe_pkt_scalar_hvx_stores)(CPUHexagonState *env, int mask,
  * If the load is in slot 0 and there is a store in slot1 (that
  * wasn't cancelled), we have to do the store first.
  */
-static void check_noshuf(CPUHexagonState *env, uint32_t slot)
+static void check_noshuf(CPUHexagonState *env, uint32_t slot,
+                         target_ulong vaddr, int size)
 {
     if (slot == 0 && env->pkt_has_store_s1 &&
         ((env->slot_cancelled & (1 << 1)) == 0)) {
+        HELPER(probe_noshuf_load)(env, vaddr, size, MMU_USER_IDX);
         HELPER(commit_store)(env, 1);
     }
 }
@@ -526,7 +539,7 @@ static uint8_t mem_load1(CPUHexagonState *env, uint32_t slot,
                          target_ulong vaddr)
 {
     uintptr_t ra = GETPC();
-    check_noshuf(env, slot);
+    check_noshuf(env, slot, vaddr, 1);
     return cpu_ldub_data_ra(env, vaddr, ra);
 }
 
@@ -534,7 +547,7 @@ static uint16_t mem_load2(CPUHexagonState *env, uint32_t slot,
                           target_ulong vaddr)
 {
     uintptr_t ra = GETPC();
-    check_noshuf(env, slot);
+    check_noshuf(env, slot, vaddr, 2);
     return cpu_lduw_data_ra(env, vaddr, ra);
 }
 
@@ -542,7 +555,7 @@ static uint32_t mem_load4(CPUHexagonState *env, uint32_t slot,
                           target_ulong vaddr)
 {
     uintptr_t ra = GETPC();
-    check_noshuf(env, slot);
+    check_noshuf(env, slot, vaddr, 4);
     return cpu_ldl_data_ra(env, vaddr, ra);
 }
 
@@ -550,7 +563,7 @@ static uint64_t mem_load8(CPUHexagonState *env, uint32_t slot,
                           target_ulong vaddr)
 {
     uintptr_t ra = GETPC();
-    check_noshuf(env, slot);
+    check_noshuf(env, slot, vaddr, 8);
     return cpu_ldq_data_ra(env, vaddr, ra);
 }
 
index 23b987053491c3001a0f13d450a974c33a839746..96a4d7a614da4ca0363de717322b9d01f2c581c5 100644 (file)
@@ -35,6 +35,7 @@ HEX_TESTS += preg_alias
 HEX_TESTS += dual_stores
 HEX_TESTS += multi_result
 HEX_TESTS += mem_noshuf
+HEX_TESTS += mem_noshuf_exception
 HEX_TESTS += circ
 HEX_TESTS += brev
 HEX_TESTS += load_unpack
diff --git a/tests/tcg/hexagon/mem_noshuf_exception.c b/tests/tcg/hexagon/mem_noshuf_exception.c
new file mode 100644 (file)
index 0000000..08660ea
--- /dev/null
@@ -0,0 +1,146 @@
+/*
+ *  Copyright(c) 2022 Qualcomm Innovation Center, Inc. All Rights Reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * Test the VLIW semantics of exceptions with mem_noshuf
+ *
+ * When a packet has the :mem_noshuf attribute, the semantics dictate
+ * That the load will get the data from the store if the addresses overlap.
+ * To accomplish this, we perform the store first.  However, we have to
+ * handle the case where the store raises an exception.  In that case, the
+ * store should not alter the machine state.
+ *
+ * We test this with a mem_noshuf packet with a store to a global variable,
+ * "should_not_change" and a load from NULL.  After the SIGSEGV is caught,
+ * we check * that the "should_not_change" value is the same.
+ *
+ * We also check that a predicated load where the predicate is false doesn't
+ * raise an exception and allows the store to happen.
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <fcntl.h>
+#include <setjmp.h>
+#include <signal.h>
+
+int err;
+int segv_caught;
+
+#define SHOULD_NOT_CHANGE_VAL        5
+int should_not_change = SHOULD_NOT_CHANGE_VAL;
+
+#define OK_TO_CHANGE_VAL        13
+int ok_to_change = OK_TO_CHANGE_VAL;
+
+static void __check(const char *filename, int line, int x, int expect)
+{
+    if (x != expect) {
+        printf("ERROR %s:%d - %d != %d\n",
+               filename, line, x, expect);
+        err++;
+    }
+}
+
+#define check(x, expect) __check(__FILE__, __LINE__, (x), (expect))
+
+static void __chk_error(const char *filename, int line, int ret)
+{
+    if (ret < 0) {
+        printf("ERROR %s:%d - %d\n", filename, line, ret);
+        err++;
+    }
+}
+
+#define chk_error(ret) __chk_error(__FILE__, __LINE__, (ret))
+
+jmp_buf jmp_env;
+
+static void sig_segv(int sig, siginfo_t *info, void *puc)
+{
+    check(sig, SIGSEGV);
+    segv_caught = 1;
+    longjmp(jmp_env, 1);
+}
+
+int main()
+{
+    struct sigaction act;
+    int dummy32;
+    long long dummy64;
+    void *p;
+
+    /* SIGSEGV test */
+    act.sa_sigaction = sig_segv;
+    sigemptyset(&act.sa_mask);
+    act.sa_flags = SA_SIGINFO;
+    chk_error(sigaction(SIGSEGV, &act, NULL));
+    if (setjmp(jmp_env) == 0) {
+        asm volatile("r18 = ##should_not_change\n\t"
+                     "r19 = #0\n\t"
+                     "{\n\t"
+                     "    memw(r18) = #7\n\t"
+                     "    %0 = memw(r19)\n\t"
+                     "}:mem_noshuf\n\t"
+                      : "=r"(dummy32) : : "r18", "r19", "memory");
+    }
+
+    act.sa_handler = SIG_DFL;
+    sigemptyset(&act.sa_mask);
+    act.sa_flags = 0;
+    chk_error(sigaction(SIGSEGV, &act, NULL));
+
+    check(segv_caught, 1);
+    check(should_not_change, SHOULD_NOT_CHANGE_VAL);
+
+    /*
+     * Check that a predicated load where the predicate is false doesn't
+     * raise an exception and allows the store to happen.
+     */
+    asm volatile("r18 = ##ok_to_change\n\t"
+                 "r19 = #0\n\t"
+                 "p0 = cmp.gt(r0, r0)\n\t"
+                 "{\n\t"
+                 "    memw(r18) = #7\n\t"
+                 "    if (p0) %0 = memw(r19)\n\t"
+                 "}:mem_noshuf\n\t"
+                  : "=r"(dummy32) : : "r18", "r19", "p0", "memory");
+
+    check(ok_to_change, 7);
+
+    /*
+     * Also check that the post-increment doesn't happen when the
+     * predicate is false.
+     */
+    ok_to_change = OK_TO_CHANGE_VAL;
+    p = NULL;
+    asm volatile("r18 = ##ok_to_change\n\t"
+                 "p0 = cmp.gt(r0, r0)\n\t"
+                 "{\n\t"
+                 "    memw(r18) = #9\n\t"
+                 "    if (p0) %1 = memd(%0 ++ #8)\n\t"
+                 "}:mem_noshuf\n\t"
+                  : "+r"(p), "=r"(dummy64) : : "r18", "p0", "memory");
+
+    check(ok_to_change, 9);
+    check((int)p, (int)NULL);
+
+    puts(err ? "FAIL" : "PASS");
+    return err ? EXIT_FAILURE : EXIT_SUCCESS;
+}