All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] Hexagon (target/hexagon) probe the stores in a packet at start of commit
@ 2021-09-30 21:16 Taylor Simpson
  2021-09-30 21:41 ` Philippe Mathieu-Daudé
  2021-10-01 15:55 ` Richard Henderson
  0 siblings, 2 replies; 5+ messages in thread
From: Taylor Simpson @ 2021-09-30 21:16 UTC (permalink / raw)
  To: qemu-devel; +Cc: ale, bcain, tsimpson, richard.henderson, f4bug

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>

*** Changes in v2 ***
Address feedback from Richard Henderson <richard.henderson@linaro.org>
- Since we know the value of all the mask at translation time, call
  specialized helper
- dczeroa has to be the only store operation in a packet, so we go
  ahead and process that first
- When there are two scalar stores, we probe the one in slot 0 - the
  call to process_store_log will do slot 1 first, so we don't need
  to probe
---
 target/hexagon/helper.h           |   2 +
 target/hexagon/op_helper.c        |  16 ++++++
 target/hexagon/translate.c        |  32 +++++++++++-
 tests/tcg/hexagon/hex_sigsegv.c   | 106 ++++++++++++++++++++++++++++++++++++++
 tests/tcg/hexagon/Makefile.target |   1 +
 5 files changed, 155 insertions(+), 2 deletions(-)
 create mode 100644 tests/tcg/hexagon/hex_sigsegv.c

diff --git a/target/hexagon/helper.h b/target/hexagon/helper.h
index ca201fb..89de2a3 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_2(probe_pkt_scalar_store_s0, void, env, int)
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 61d5cde..af32de4 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -377,6 +377,22 @@ int32_t HELPER(vacsh_pred)(CPUHexagonState *env,
     return PeV;
 }
 
+static 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);
+    }
+}
+
+/* Called during packet commit when there are two scalar stores */
+void HELPER(probe_pkt_scalar_store_s0)(CPUHexagonState *env, int mmu_idx)
+{
+    probe_store(env, 0, mmu_idx);
+}
+
 /*
  * 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..8fc2c83 100644
--- a/target/hexagon/translate.c
+++ b/target/hexagon/translate.c
@@ -471,10 +471,38 @@ 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.
+     *
+     * dczeroa has to be the only store operation in the packet, so we go
+     * ahead and process that first.
+     *
+     * When there are two scalar stores, we probe the one in slot 0.
+     *
+     * 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.
+     */
+    bool has_store_s0 = pkt->pkt_has_store_s0;
+    bool has_store_s1 = (pkt->pkt_has_store_s1 && !ctx->s1_store_processed);
+    if (pkt->pkt_has_dczeroa) {
+        /*
+         * The dczeroa will be the store in slot 0, check that we don't have
+         * a store in slot 1.
+         */
+        g_assert(has_store_s0 && !has_store_s1);
+        process_dczeroa(ctx, pkt);
+    } else if (has_store_s0 && has_store_s1) {
+        TCGv mem_idx = tcg_const_tl(ctx->mem_idx);
+        gen_helper_probe_pkt_scalar_store_s0(cpu_env, mem_idx);
+        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) {
         TCGv has_st0 =
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
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] Hexagon (target/hexagon) probe the stores in a packet at start of commit
  2021-09-30 21:16 [PATCH v2] Hexagon (target/hexagon) probe the stores in a packet at start of commit Taylor Simpson
@ 2021-09-30 21:41 ` Philippe Mathieu-Daudé
  2021-10-01 15:55 ` Richard Henderson
  1 sibling, 0 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-30 21:41 UTC (permalink / raw)
  To: Taylor Simpson, qemu-devel; +Cc: ale, bcain, richard.henderson

Hi Taylor,

On 9/30/21 23:16, Taylor Simpson wrote:
> 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>
> 
> *** Changes in v2 ***
> Address feedback from Richard Henderson <richard.henderson@linaro.org>
> - Since we know the value of all the mask at translation time, call
>   specialized helper
> - dczeroa has to be the only store operation in a packet, so we go
>   ahead and process that first
> - When there are two scalar stores, we probe the one in slot 0 - the
>   call to process_store_log will do slot 1 first, so we don't need
>   to probe
> ---
>  target/hexagon/helper.h           |   2 +
>  target/hexagon/op_helper.c        |  16 ++++++
>  target/hexagon/translate.c        |  32 +++++++++++-
>  tests/tcg/hexagon/hex_sigsegv.c   | 106 ++++++++++++++++++++++++++++++++++++++
>  tests/tcg/hexagon/Makefile.target |   1 +
>  5 files changed, 155 insertions(+), 2 deletions(-)
>  create mode 100644 tests/tcg/hexagon/hex_sigsegv.c
> 
> diff --git a/target/hexagon/helper.h b/target/hexagon/helper.h
> index ca201fb..89de2a3 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_2(probe_pkt_scalar_store_s0, void, env, int)
> diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
> index 61d5cde..af32de4 100644
> --- a/target/hexagon/op_helper.c
> +++ b/target/hexagon/op_helper.c
> @@ -377,6 +377,22 @@ int32_t HELPER(vacsh_pred)(CPUHexagonState *env,
>      return PeV;
>  }
>  
> +static 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);
> +    }

Matter of taste probably:

       if (env->slot_cancelled & (1 << slot) {
           return;
       }
       probe_write(env, env->mem_log_stores[slot].va,
                   env->mem_log_stores[slot].width, mmu_idx, GETPC());

> +}
> +
> +/* Called during packet commit when there are two scalar stores */
> +void HELPER(probe_pkt_scalar_store_s0)(CPUHexagonState *env, int mmu_idx)
> +{
> +    probe_store(env, 0, mmu_idx);
> +}
> +
>  /*
>   * 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..8fc2c83 100644
> --- a/target/hexagon/translate.c
> +++ b/target/hexagon/translate.c
> @@ -471,10 +471,38 @@ 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.
> +     *
> +     * dczeroa has to be the only store operation in the packet, so we go
> +     * ahead and process that first.
> +     *
> +     * When there are two scalar stores, we probe the one in slot 0.
> +     *
> +     * 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.
> +     */
> +    bool has_store_s0 = pkt->pkt_has_store_s0;
> +    bool has_store_s1 = (pkt->pkt_has_store_s1 && !ctx->s1_store_processed);
> +    if (pkt->pkt_has_dczeroa) {
> +        /*
> +         * The dczeroa will be the store in slot 0, check that we don't have
> +         * a store in slot 1.
> +         */
> +        g_assert(has_store_s0 && !has_store_s1);
> +        process_dczeroa(ctx, pkt);
> +    } else if (has_store_s0 && has_store_s1) {
> +        TCGv mem_idx = tcg_const_tl(ctx->mem_idx);
> +        gen_helper_probe_pkt_scalar_store_s0(cpu_env, mem_idx);
> +        tcg_temp_free(mem_idx);

The index is read-only, so you can avoid the temporary:

   gen_helper_probe_pkt_scalar_store_s0(cpu_env,
                                        tcg_constant_tl(ctx->mem_idx));

Maybe add a (better) comment here:

       } else {

           /* default path, all constraints OK, we are good */
> +    }
> +
> +    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) {
>          TCGv has_st0 =
> 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

hex_sigsegv is a generic test name ...

> @@ -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

... but you are testing a very specific case.

Maybe rename as "multi_pkt_stores" (or better)?

> + *
> + * 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.
> + */

Otherwise LGTM.

Regards,

Phil.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] Hexagon (target/hexagon) probe the stores in a packet at start of commit
  2021-09-30 21:16 [PATCH v2] Hexagon (target/hexagon) probe the stores in a packet at start of commit Taylor Simpson
  2021-09-30 21:41 ` Philippe Mathieu-Daudé
@ 2021-10-01 15:55 ` Richard Henderson
  2021-10-01 16:14   ` Taylor Simpson
  1 sibling, 1 reply; 5+ messages in thread
From: Richard Henderson @ 2021-10-01 15:55 UTC (permalink / raw)
  To: Taylor Simpson, qemu-devel; +Cc: ale, bcain, f4bug

On 9/30/21 5:16 PM, Taylor Simpson wrote:
> +    } else if (has_store_s0 && has_store_s1) {
> +        TCGv mem_idx = tcg_const_tl(ctx->mem_idx);
> +        gen_helper_probe_pkt_scalar_store_s0(cpu_env, mem_idx);
> +        tcg_temp_free(mem_idx);
> +    }

So we're assuming that the s1 store happens first?
If so, you could expand the comment above.

Otherwise, it looks good.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~


^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH v2] Hexagon (target/hexagon) probe the stores in a packet at start of commit
  2021-10-01 15:55 ` Richard Henderson
@ 2021-10-01 16:14   ` Taylor Simpson
  0 siblings, 0 replies; 5+ messages in thread
From: Taylor Simpson @ 2021-10-01 16:14 UTC (permalink / raw)
  To: Richard Henderson, qemu-devel; +Cc: ale, Brian Cain, f4bug



> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Friday, October 1, 2021 10:55 AM
> 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 v2] Hexagon (target/hexagon) probe the stores in a
> packet at start of commit
> 
> On 9/30/21 5:16 PM, Taylor Simpson wrote:
> > +    } else if (has_store_s0 && has_store_s1) {
> > +        TCGv mem_idx = tcg_const_tl(ctx->mem_idx);
> > +        gen_helper_probe_pkt_scalar_store_s0(cpu_env, mem_idx);
> > +        tcg_temp_free(mem_idx);
> > +    }
> 
> So we're assuming that the s1 store happens first?
> If so, you could expand the comment above.

Yes, there's a comment in process_store_log (with a typo fixed here).
    /*
     *  When a packet has two stores, the hardware processes
     *  slot 1 and then slot 0.  This will be important when
     *  the memory accesses overlap.
     */
I'll fix the typo and expand the comment in the above code.

Also, tests/tcg/hexagon/dual_stores.c tests for this behavior.

> Otherwise, it looks good.
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks!



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2] Hexagon (target/hexagon) probe the stores in a packet at start of commit
@ 2021-09-22 23:11 Taylor Simpson
  0 siblings, 0 replies; 5+ messages in thread
From: Taylor Simpson @ 2021-09-22 23:11 UTC (permalink / raw)
  To: qemu-devel; +Cc: ale, bcain, tsimpson, richard.henderson, f4bug

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

*** Changes in v2 ***
Reduce the overhead of probe_pkt_stores - pass a bit mask instead of
multiple booleans

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

diff --git a/target/hexagon/helper.h b/target/hexagon/helper.h
index ca201fb..412aaf9 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_3(probe_pkt_stores, void, env, int, int)
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 61d5cde..fa65980 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -377,6 +377,39 @@ int32_t HELPER(vacsh_pred)(CPUHexagonState *env,
     return PeV;
 }
 
+static 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 mask, int mmu_idx)
+{
+    bool has_st0        = (mask >> 0) & 1;
+    bool has_st1        = (mask >> 1) & 1;
+    bool has_dczeroa    = (mask >> 2) & 1;
+
+    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..9e8d27a 100644
--- a/target/hexagon/translate.c
+++ b/target/hexagon/translate.c
@@ -471,9 +471,40 @@ 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 && !ctx->s1_store_processed)) ||
+        pkt->pkt_has_dczeroa) {
+        TCGv mem_idx = tcg_const_tl(ctx->mem_idx);
+        int mask = 0;
+        TCGv mask_tcgv;
+
+        if (pkt->pkt_has_store_s0) {
+            mask |= (1 << 0);
+        }
+        if (pkt->pkt_has_store_s1 && !ctx->s1_store_processed) {
+            mask |= (1 << 1);
+        }
+        if (pkt->pkt_has_dczeroa) {
+            mask |= (1 << 2);
+        }
+        mask_tcgv = tcg_const_tl(mask);
+        gen_helper_probe_pkt_stores(cpu_env, mask_tcgv, mem_idx);
+        tcg_temp_free(mem_idx);
+        tcg_temp_free(mask_tcgv);
+    }
+
+    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
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-10-01 16:18 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-30 21:16 [PATCH v2] Hexagon (target/hexagon) probe the stores in a packet at start of commit Taylor Simpson
2021-09-30 21:41 ` Philippe Mathieu-Daudé
2021-10-01 15:55 ` Richard Henderson
2021-10-01 16:14   ` Taylor Simpson
  -- strict thread matches above, loose matches on Subject: below --
2021-09-22 23:11 Taylor Simpson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.