diff mbox series

Hexagon (target/hexagon) probe the stores in a packet at start of commit

Message ID 1632335718-13541-1-git-send-email-tsimpson@quicinc.com
State New
Headers show
Series Hexagon (target/hexagon) probe the stores in a packet at start of commit | expand

Commit Message

Taylor Simpson Sept. 22, 2021, 6:35 p.m. UTC 
When a packet has 2 stores, either both commit or neither commit.
At the beginning of gen_commit_packet, we check for multiple stores.
If there are multiple stores, call a helper that will probe each of
them before proceeding with the commit.

Note that we don't call the probe helper for packets with only one
store.  Therefore, we call process_store_log before anything else
involved in committing the packet.

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

Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
---
 target/hexagon/helper.h           |   2 +
 target/hexagon/op_helper.c        |  30 +++++++++++
 target/hexagon/translate.c        |  27 +++++++++-
 tests/tcg/hexagon/hex_sigsegv.c   | 106 ++++++++++++++++++++++++++++++++++++++
 tests/tcg/hexagon/Makefile.target |   1 +
 5 files changed, 165 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/hexagon/hex_sigsegv.c

Comments

Richard Henderson Sept. 23, 2021, 5:05 p.m. UTC | #1 
On 9/22/21 11:35 AM, Taylor Simpson wrote:
> +static inline void probe_store(CPUHexagonState *env, int slot, int mmu_idx)
> +{
> +    if (!(env->slot_cancelled & (1 << slot))) {
> +        size1u_t width = env->mem_log_stores[slot].width;
> +        target_ulong va = env->mem_log_stores[slot].va;
> +        uintptr_t ra = GETPC();
> +        probe_write(env, va, width, mmu_idx, ra);
> +    }
> +}
> +
> +void HELPER(probe_pkt_stores)(CPUHexagonState *env, int has_st0, int has_st1,
> +                              int has_dczeroa, int mmu_idx)
> +{
> +    if (has_st0 && !has_dczeroa) {
> +        probe_store(env, 0, mmu_idx);
> +    }
> +    if (has_st1 && !has_dczeroa) {
> +        probe_store(env, 1, mmu_idx);
> +    }
> +    if (has_dczeroa) {
> +        /* Probe 32 bytes starting at (dczero_addr & ~0x1f) */
> +        target_ulong va = env->dczero_addr & ~0x1f;
> +        uintptr_t ra = GETPC();
> +        probe_write(env, va +  0, 8, mmu_idx, ra);
> +        probe_write(env, va +  8, 8, mmu_idx, ra);
> +        probe_write(env, va + 16, 8, mmu_idx, ra);
> +        probe_write(env, va + 24, 8, mmu_idx, ra);
> +    }
> +}

You know at translate time the value of all of these has_* variables.

Since has_dczeroa disables the other two probes, surely probe_pkt_dczeroa should be its 
own helper.

That said, if dczeroa (apparently) cannot be paired with other stores, why do you need to 
probe for it at all?  Since the operation is 32-byte aligned, surely the first real store 
will validate the write for the entire block.

Once you eliminate dczeroa from this helper, the only time it will be called is with both 
has_st0 and has_st1 true, at which point you don't need to pass the arguments in at all.


r~
Taylor Simpson Sept. 24, 2021, 2:19 p.m. UTC | #2 
> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Thursday, September 23, 2021 12:05 PM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: f4bug@amsat.org; ale@rev.ng; Brian Cain <bcain@quicinc.com>
> Subject: Re: [PATCH] Hexagon (target/hexagon) probe the stores in a packet
> at start of commit
> 
> On 9/22/21 11:35 AM, Taylor Simpson wrote:
> > +static inline void probe_store(CPUHexagonState *env, int slot, int
> > +mmu_idx) {
> > +    if (!(env->slot_cancelled & (1 << slot))) {
> > +        size1u_t width = env->mem_log_stores[slot].width;
> > +        target_ulong va = env->mem_log_stores[slot].va;
> > +        uintptr_t ra = GETPC();
> > +        probe_write(env, va, width, mmu_idx, ra);
> > +    }
> > +}
> > +
> > +void HELPER(probe_pkt_stores)(CPUHexagonState *env, int has_st0, int
> has_st1,
> > +                              int has_dczeroa, int mmu_idx) {
> > +    if (has_st0 && !has_dczeroa) {
> > +        probe_store(env, 0, mmu_idx);
> > +    }
> > +    if (has_st1 && !has_dczeroa) {
> > +        probe_store(env, 1, mmu_idx);
> > +    }
> > +    if (has_dczeroa) {
> > +        /* Probe 32 bytes starting at (dczero_addr & ~0x1f) */
> > +        target_ulong va = env->dczero_addr & ~0x1f;
> > +        uintptr_t ra = GETPC();
> > +        probe_write(env, va +  0, 8, mmu_idx, ra);
> > +        probe_write(env, va +  8, 8, mmu_idx, ra);
> > +        probe_write(env, va + 16, 8, mmu_idx, ra);
> > +        probe_write(env, va + 24, 8, mmu_idx, ra);
> > +    }
> > +}
> 
> You know at translate time the value of all of these has_* variables.
> 
> Since has_dczeroa disables the other two probes, surely probe_pkt_dczeroa
> should be its own helper.
> 
> That said, if dczeroa (apparently) cannot be paired with other stores, why do
> you need to probe for it at all?  Since the operation is 32-byte aligned, surely
> the first real store will validate the write for the entire block.
> 
> Once you eliminate dczeroa from this helper, the only time it will be called is
> with both
> has_st0 and has_st1 true, at which point you don't need to pass the
> arguments in at all.

You are correct, dczeroa can't be in a packet with any other memory op, so it could be its own helper.

We also need to account for HVX stores in the other patch series under review.  With HVX, we could have an HVX store and a scalar store in the same packet.  I'll go ahead and make the changes you suggest here.  I'll create a more general helper in the HVX series.  For efficiency, I'd like to only call a single helper that will do all the probes.  So, we'll either end up with one for each possible combination or a one for scalar only and one for the other combinations with a mask.


Thanks,
Taylor
diff mbox series

Patch

diff --git a/target/hexagon/helper.h b/target/hexagon/helper.h
index ca201fb..992017c 100644
--- a/target/hexagon/helper.h
+++ b/target/hexagon/helper.h
@@ -89,3 +89,5 @@  DEF_HELPER_4(sffms_lib, f32, env, f32, f32, f32)
 
 DEF_HELPER_3(dfmpyfix, f64, env, f64, f64)
 DEF_HELPER_4(dfmpyhh, f64, env, f64, f64, f64)
+
+DEF_HELPER_5(probe_pkt_stores, void, env, int, int, int, int)
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 61d5cde..2be3e01 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -377,6 +377,36 @@  int32_t HELPER(vacsh_pred)(CPUHexagonState *env,
     return PeV;
 }
 
+static inline void probe_store(CPUHexagonState *env, int slot, int mmu_idx)
+{
+    if (!(env->slot_cancelled & (1 << slot))) {
+        size1u_t width = env->mem_log_stores[slot].width;
+        target_ulong va = env->mem_log_stores[slot].va;
+        uintptr_t ra = GETPC();
+        probe_write(env, va, width, mmu_idx, ra);
+    }
+}
+
+void HELPER(probe_pkt_stores)(CPUHexagonState *env, int has_st0, int has_st1,
+                              int has_dczeroa, int mmu_idx)
+{
+    if (has_st0 && !has_dczeroa) {
+        probe_store(env, 0, mmu_idx);
+    }
+    if (has_st1 && !has_dczeroa) {
+        probe_store(env, 1, mmu_idx);
+    }
+    if (has_dczeroa) {
+        /* Probe 32 bytes starting at (dczero_addr & ~0x1f) */
+        target_ulong va = env->dczero_addr & ~0x1f;
+        uintptr_t ra = GETPC();
+        probe_write(env, va +  0, 8, mmu_idx, ra);
+        probe_write(env, va +  8, 8, mmu_idx, ra);
+        probe_write(env, va + 16, 8, mmu_idx, ra);
+        probe_write(env, va + 24, 8, mmu_idx, ra);
+    }
+}
+
 /*
  * mem_noshuf
  * Section 5.5 of the Hexagon V67 Programmer's Reference Manual
diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
index 6fb4e68..61b015c 100644
--- a/target/hexagon/translate.c
+++ b/target/hexagon/translate.c
@@ -471,9 +471,34 @@  static void update_exec_counters(DisasContext *ctx, Packet *pkt)
 
 static void gen_commit_packet(DisasContext *ctx, Packet *pkt)
 {
+    /*
+     * If there is more than one store in a packet, make sure they are all OK
+     * before proceeding with the rest of the packet commit.
+     *
+     * Note that we don't call the probe helper for packets with only one
+     * store.  Therefore, we call process_store_log before anything else
+     * involved in committing the packet.
+     */
+    if ((pkt->pkt_has_store_s0 && pkt->pkt_has_store_s1) ||
+        pkt->pkt_has_dczeroa) {
+        TCGv has_st0 = tcg_const_tl(pkt->pkt_has_store_s0);
+        TCGv has_st1 = tcg_const_tl(pkt->pkt_has_store_s1);
+        TCGv has_dczeroa = tcg_const_tl(pkt->pkt_has_dczeroa);
+        TCGv mem_idx = tcg_const_tl(ctx->mem_idx);
+
+        gen_helper_probe_pkt_stores(cpu_env, has_st0, has_st1,
+                                    has_dczeroa, mem_idx);
+
+        tcg_temp_free(has_st0);
+        tcg_temp_free(has_st1);
+        tcg_temp_free(has_dczeroa);
+        tcg_temp_free(mem_idx);
+    }
+
+    process_store_log(ctx, pkt);
+
     gen_reg_writes(ctx);
     gen_pred_writes(ctx, pkt);
-    process_store_log(ctx, pkt);
     process_dczeroa(ctx, pkt);
     update_exec_counters(ctx, pkt);
     if (HEX_DEBUG) {
diff --git a/tests/tcg/hexagon/hex_sigsegv.c b/tests/tcg/hexagon/hex_sigsegv.c
new file mode 100644
index 0000000..dc2b349
--- /dev/null
+++ b/tests/tcg/hexagon/hex_sigsegv.c
@@ -0,0 +1,106 @@ 
+/*
+ *  Copyright(c) 2021 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 two stores in a packet
+ *
+ * When a packet has 2 stores, either both commit or neither commit.
+ * We test this with a packet that does stores to both NULL and a global
+ * variable, "should_not_change".  After the SIGSEGV is caught, we check
+ * that the "should_not_change" value is the same.
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <fcntl.h>
+#include <setjmp.h>
+#include <signal.h>
+
+typedef unsigned char uint8_t;
+
+int err;
+int segv_caught;
+
+#define SHOULD_NOT_CHANGE_VAL        5
+int should_not_change = SHOULD_NOT_CHANGE_VAL;
+
+#define BUF_SIZE        300
+unsigned char buf[BUF_SIZE];
+
+
+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;
+
+    /* 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"
+                     "    memw(r19) = #0\n\t"
+                     "}\n\t"
+                      : : : "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);
+
+    puts(err ? "FAIL" : "PASS");
+    return err ? EXIT_FAILURE : EXIT_SUCCESS;
+}
diff --git a/tests/tcg/hexagon/Makefile.target b/tests/tcg/hexagon/Makefile.target
index 050cd61..c1e1650 100644
--- a/tests/tcg/hexagon/Makefile.target
+++ b/tests/tcg/hexagon/Makefile.target
@@ -28,6 +28,7 @@  first: $(HEX_SRC)/first.S
 	$(CC) -static -mv67 -nostdlib $^ -o $@
 
 HEX_TESTS = first
+HEX_TESTS += hex_sigsegv
 HEX_TESTS += misc
 HEX_TESTS += preg_alias
 HEX_TESTS += dual_stores