diff mbox series

[v3] Hexagon: add PC alignment check and exception

Message ID 5c90567ec28723865e144f386b36f5b676b7a5d3.1714486874.git.quic_mathbern@quicinc.com
State New
Headers show
Series [v3] Hexagon: add PC alignment check and exception | expand

Commit Message

Matheus Tavares Bernardino April 30, 2024, 2:25 p.m. UTC 
The Hexagon Programmer's Reference Manual says that the exception 0x1e
should be raised upon an unaligned program counter. Let's implement that
and also add some tests.

Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
---
v2: https://lore.kernel.org/qemu-devel/e559b521d1920f804df10244c8c07564431aeba5.1714419461.git.quic_mathbern@quicinc.com/

Thanks for the comments, Richard and Taylor!

Changed in v3:
- Removed now unnecessary pkt_raises_exception addition.
- Added HEX_EXCP_PC_NOT_ALIGNED handling at
  linux-user/hexagon/cpu_loop.c.
- Merged all tests into a C file that uses signal handler to check
  that the exception was raised.

 target/hexagon/cpu.h                       |  7 ++
 target/hexagon/cpu_bits.h                  |  4 +
 target/hexagon/macros.h                    |  3 -
 linux-user/hexagon/cpu_loop.c              |  4 +
 target/hexagon/op_helper.c                 |  9 +--
 tests/tcg/hexagon/unaligned_pc.c           | 85 ++++++++++++++++++++++
 tests/tcg/hexagon/Makefile.target          |  4 +
 tests/tcg/hexagon/unaligned_pc_multi_cof.S |  5 ++
 8 files changed, 113 insertions(+), 8 deletions(-)
 create mode 100644 tests/tcg/hexagon/unaligned_pc.c
 create mode 100644 tests/tcg/hexagon/unaligned_pc_multi_cof.S

Comments

Richard Henderson April 30, 2024, 3:45 p.m. UTC | #1 
On 4/30/24 07:25, Matheus Tavares Bernardino wrote:
> The Hexagon Programmer's Reference Manual says that the exception 0x1e
> should be raised upon an unaligned program counter. Let's implement that
> and also add some tests.
> 
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---
> v2: https://lore.kernel.org/qemu-devel/e559b521d1920f804df10244c8c07564431aeba5.1714419461.git.quic_mathbern@quicinc.com/
> 
> Thanks for the comments, Richard and Taylor!
> 
> Changed in v3:
> - Removed now unnecessary pkt_raises_exception addition.
> - Added HEX_EXCP_PC_NOT_ALIGNED handling at
>    linux-user/hexagon/cpu_loop.c.
> - Merged all tests into a C file that uses signal handler to check
>    that the exception was raised.


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~
Richard Henderson April 30, 2024, 3:52 p.m. UTC | #2 
On 4/30/24 07:25, Matheus Tavares Bernardino wrote:
> +void test_multi_cof(void)
> +{
> +    asm volatile(
> +        "p0 = cmp.eq(r0, r0)\n"
> +        "{\n"
> +        "    if (p0) jump test_multi_cof_unaligned\n"
> +        "    jump 1f\n"
> +        "}\n"
> +        "1: nop\n"
> +        : : : "p0");
> +}

I will say you could just add the label to the end of the asm here, like

	.byte 0
test_multi_cof_unaligned:

rather than use a separate source file.


r~
Brian Cain April 30, 2024, 6:31 p.m. UTC | #3 
> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Tuesday, April 30, 2024 10:53 AM
> To: Matheus Bernardino (QUIC) <quic_mathbern@quicinc.com>; qemu-
> devel@nongnu.org
> Cc: Brian Cain <bcain@quicinc.com>; Sid Manning <sidneym@quicinc.com>;
> ale@rev.ng; anjo@rev.ng; ltaylorsimpson@gmail.com; Laurent Vivier
> <laurent@vivier.eu>
> Subject: Re: [PATCH v3] Hexagon: add PC alignment check and exception
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
> 
> On 4/30/24 07:25, Matheus Tavares Bernardino wrote:
> > +void test_multi_cof(void)
> > +{
> > +    asm volatile(
> > +        "p0 = cmp.eq(r0, r0)\n"
> > +        "{\n"
> > +        "    if (p0) jump test_multi_cof_unaligned\n"
> > +        "    jump 1f\n"
> > +        "}\n"
> > +        "1: nop\n"
> > +        : : : "p0");
> > +}
> 
> I will say you could just add the label to the end of the asm here, like
> 
>         .byte 0
> test_multi_cof_unaligned:
> 
> rather than use a separate source file.

Agreed: that would simplify this test case definition and the patch a bit.

-Brian
Taylor Simpson April 30, 2024, 7:41 p.m. UTC | #4 
> -----Original Message-----
> From: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> Sent: Tuesday, April 30, 2024 9:25 AM
> To: qemu-devel@nongnu.org
> Cc: bcain@quicinc.com; sidneym@quicinc.com; ale@rev.ng; anjo@rev.ng;
> ltaylorsimpson@gmail.com; richard.henderson@linaro.org; Laurent Vivier
> <laurent@vivier.eu>
> Subject: [PATCH v3] Hexagon: add PC alignment check and exception
> 
> The Hexagon Programmer's Reference Manual says that the exception 0x1e
> should be raised upon an unaligned program counter. Let's implement that
> and also add some tests.
> 
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---
> v2: https://lore.kernel.org/qemu-
> devel/e559b521d1920f804df10244c8c07564431aeba5.1714419461.git.quic_ma
> thbern@quicinc.com/
> 
> Thanks for the comments, Richard and Taylor!
> 
> Changed in v3:
> - Removed now unnecessary pkt_raises_exception addition.
> - Added HEX_EXCP_PC_NOT_ALIGNED handling at
>   linux-user/hexagon/cpu_loop.c.
> - Merged all tests into a C file that uses signal handler to check
>   that the exception was raised.
> 
>  target/hexagon/cpu.h                       |  7 ++
>  target/hexagon/cpu_bits.h                  |  4 +
>  target/hexagon/macros.h                    |  3 -
>  linux-user/hexagon/cpu_loop.c              |  4 +
>  target/hexagon/op_helper.c                 |  9 +--
>  tests/tcg/hexagon/unaligned_pc.c           | 85 ++++++++++++++++++++++
>  tests/tcg/hexagon/Makefile.target          |  4 +
>  tests/tcg/hexagon/unaligned_pc_multi_cof.S |  5 ++
>  8 files changed, 113 insertions(+), 8 deletions(-)  create mode 100644
> tests/tcg/hexagon/unaligned_pc.c  create mode 100644
> tests/tcg/hexagon/unaligned_pc_multi_cof.S
> 



> a/tests/tcg/hexagon/unaligned_pc.c b/tests/tcg/hexagon/unaligned_pc.c
> new file mode 100644
> index 0000000000..1add2d0d99
> --- /dev/null
> +++ b/tests/tcg/hexagon/unaligned_pc.c
> @@ -0,0 +1,85 @@
> +#include <stdio.h>
> +#include <signal.h>
> +#include <setjmp.h>
> +#include <stdlib.h>
> +
> +/* will be changed in signal handler */ volatile sig_atomic_t
> +completed_tests; static jmp_buf after_test; static int nr_tests;
> +
> +void __attribute__((naked)) test_return(void) {
> +    asm volatile(
> +        "allocframe(#0x8)\n"
> +        "r0 = #0xffffffff\n"
> +        "framekey = r0\n"
> +        "dealloc_return\n"
> +        : : : "r0");

Add r29, r30, r31 to clobbers list.
Add framekey to clobbers list (assuming the compiler will take it).

> +}
> +
> +void test_endloop(void)
> +{
> +    asm volatile(
> +        "loop0(1f, #2)\n"
> +        "1: r0 = #0x3\n"
> +        "sa0 = r0\n"
> +        "{ nop }:endloop0\n"
> +        : : : "r0");
> +}

Add sa0, lc0, usr to the clobbers list.

> +
> +void test_multi_cof(void)
> +{
> +    asm volatile(
> +        "p0 = cmp.eq(r0, r0)\n"
> +        "{\n"
> +        "    if (p0) jump test_multi_cof_unaligned\n"
> +        "    jump 1f\n"
> +        "}\n"
> +        "1: nop\n"
> +        : : : "p0");
> +}
> +
> +void sigbus_handler(int signum)
> +{
> +    /* retore framekey after test_return */
> +    asm volatile(
> +        "r0 = #0\n"
> +        "framekey = r0\n"
> +        : : : "r0");

Add framekey to the clobbers list.

> +    printf("Test %d complete\n", completed_tests);
> +    completed_tests++;
> +    siglongjmp(after_test, 1);
> +}
> +
> +void test_done(void)
> +{
> +    int err = (completed_tests != nr_tests);
> +    puts(err ? "FAIL" : "PASS");
> +    exit(err);
> +}
> +
> +typedef void (*test_fn)(void);
> +
> +int main()
> +{
> +    test_fn tests[] = { test_return, test_endloop, test_multi_cof,
test_done
> };
> +    nr_tests = (sizeof(tests) / sizeof(tests[0])) - 1;
> +
> +    struct sigaction sa = {
> +        .sa_sigaction = sigbus_handler,
> +        .sa_flags = SA_SIGINFO
> +    };
> +
> +    if (sigaction(SIGBUS, &sa, NULL) < 0) {
> +        perror("sigaction");
> +        return EXIT_FAILURE;
> +    }
> +
> +    sigsetjmp(after_test, 1);
> +    tests[completed_tests]();
> +
> +    /* should never get here */
> +    puts("FAIL");
> +    return 1;
> +}
> diff --git a/tests/tcg/hexagon/Makefile.target
> b/tests/tcg/hexagon/Makefile.target
> index f839b2c0d5..75139e731c 100644
> --- a/tests/tcg/hexagon/Makefile.target
> +++ b/tests/tcg/hexagon/Makefile.target
> @@ -51,6 +51,7 @@ HEX_TESTS += scatter_gather  HEX_TESTS += hvx_misc
> HEX_TESTS += hvx_histogram  HEX_TESTS += invalid-slots
> +HEX_TESTS += unaligned_pc
> 
>  run-and-check-exception = $(call run-test,$2,$3 2>$2.stderr; \
>  	test $$? -eq 1 && grep -q "exception $(strip $1)" $2.stderr) @@ -
> 108,6 +109,9 @@ preg_alias: preg_alias.c hex_test.h
>  read_write_overlap: read_write_overlap.c hex_test.h
>  reg_mut: reg_mut.c hex_test.h
> 
> +unaligned_pc: unaligned_pc.c unaligned_pc_multi_cof.S
> +	$(CC) $(CFLAGS) $(CROSS_CC_GUEST_CFLAGS) -mv73 $^ -o $@
> $(LDFLAGS)
> +
>  # This test has to be compiled for the -mv67t target
>  usr: usr.c hex_test.h
>  	$(CC) $(CFLAGS) -mv67t -O2 -Wno-inline-asm -Wno-expansion-to-
> defined $< -o $@ $(LDFLAGS) diff --git
> a/tests/tcg/hexagon/unaligned_pc_multi_cof.S
> b/tests/tcg/hexagon/unaligned_pc_multi_cof.S
> new file mode 100644
> index 0000000000..10accd0057
> --- /dev/null
> +++ b/tests/tcg/hexagon/unaligned_pc_multi_cof.S
> @@ -0,0 +1,5 @@
> +.org 0x3
> +.global test_multi_cof_unaligned
> +test_multi_cof_unaligned:
> +	nop
> +	jumpr r31
> --
> 2.37.2
Matheus Tavares Bernardino May 2, 2024, 2:35 p.m. UTC | #5 
On Tue, 30 Apr 2024 08:52:36 -0700 Richard Henderson <richard.henderson@linaro.org> wrote:
>
> On 4/30/24 07:25, Matheus Tavares Bernardino wrote:
> > +void test_multi_cof(void)
> > +{
> > +    asm volatile(
> > +        "p0 = cmp.eq(r0, r0)\n"
> > +        "{\n"
> > +        "    if (p0) jump test_multi_cof_unaligned\n"
> > +        "    jump 1f\n"
> > +        "}\n"
> > +        "1: nop\n"
> > +        : : : "p0");
> > +}
> 
> I will say you could just add the label to the end of the asm here, like
> 
> 	.byte 0
> test_multi_cof_unaligned:
> 
> rather than use a separate source file.
>

That would be nice, but unfortunately that doesn't work because the label gets
aligned by the assembler :( 

diff --git a/tests/tcg/hexagon/unaligned_pc.c b/tests/tcg/hexagon/unaligned_pc.c
index 1add2d0d99..3772947a86 100644
--- a/tests/tcg/hexagon/unaligned_pc.c
+++ b/tests/tcg/hexagon/unaligned_pc.c
@@ -33,10 +33,12 @@ void test_multi_cof(void)
     asm volatile(
         "p0 = cmp.eq(r0, r0)\n"
         "{\n"
-        "    if (p0) jump test_multi_cof_unaligned\n"
+        "    if (p0) jump 2f\n"
         "    jump 1f\n"
         "}\n"
         "1: nop\n"
+        ".byte 0\n"
+        "2: nop\n"
         : : : "p0");
 }

Ends up producing:

00020dc0 <test_multi_cof>:
   20dc0:       00 c0 9d a0     a09dc000 {      allocframe(#0x0) }
   20dc4:       00 c0 00 f2     f200c000 {      p0 = cmp.eq(r0,r0) }
   20dc8:       06 40 00 5c     5c004006 {      if (p0) jump:nt 0x20dd4 <test_multi_cof+0x14>
   20dcc:       04 c0 00 58     5800c004        jump 0x20dd0 <test_multi_cof+0x10> }
   20dd0:       00 c0 00 7f     7f00c000 {      nop }
   20dd4:       00 00 c0 00 <unknown>
diff mbox series

Patch

diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h
index 3eef58fe8f..764f3c38cc 100644
--- a/target/hexagon/cpu.h
+++ b/target/hexagon/cpu.h
@@ -134,6 +134,10 @@  struct ArchCPU {
 
 FIELD(TB_FLAGS, IS_TIGHT_LOOP, 0, 1)
 
+G_NORETURN void hexagon_raise_exception_err(CPUHexagonState *env,
+                                            uint32_t exception,
+                                            uintptr_t pc);
+
 static inline void cpu_get_tb_cpu_state(CPUHexagonState *env, vaddr *pc,
                                         uint64_t *cs_base, uint32_t *flags)
 {
@@ -144,6 +148,9 @@  static inline void cpu_get_tb_cpu_state(CPUHexagonState *env, vaddr *pc,
         hex_flags = FIELD_DP32(hex_flags, TB_FLAGS, IS_TIGHT_LOOP, 1);
     }
     *flags = hex_flags;
+    if (*pc & PCALIGN_MASK) {
+        hexagon_raise_exception_err(env, HEX_EXCP_PC_NOT_ALIGNED, 0);
+    }
 }
 
 typedef HexagonCPU ArchCPU;
diff --git a/target/hexagon/cpu_bits.h b/target/hexagon/cpu_bits.h
index 96fef71729..4279281a71 100644
--- a/target/hexagon/cpu_bits.h
+++ b/target/hexagon/cpu_bits.h
@@ -20,9 +20,13 @@ 
 
 #include "qemu/bitops.h"
 
+#define PCALIGN 4
+#define PCALIGN_MASK (PCALIGN - 1)
+
 #define HEX_EXCP_FETCH_NO_UPAGE  0x012
 #define HEX_EXCP_INVALID_PACKET  0x015
 #define HEX_EXCP_INVALID_OPCODE  0x015
+#define HEX_EXCP_PC_NOT_ALIGNED  0x01e
 #define HEX_EXCP_PRIV_NO_UREAD   0x024
 #define HEX_EXCP_PRIV_NO_UWRITE  0x025
 
diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
index 1376d6ccc1..f375471a98 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
@@ -22,9 +22,6 @@ 
 #include "hex_regs.h"
 #include "reg_fields.h"
 
-#define PCALIGN 4
-#define PCALIGN_MASK (PCALIGN - 1)
-
 #define GET_FIELD(FIELD, REGIN) \
     fEXTRACTU_BITS(REGIN, reg_field_info[FIELD].width, \
                    reg_field_info[FIELD].offset)
diff --git a/linux-user/hexagon/cpu_loop.c b/linux-user/hexagon/cpu_loop.c
index 7f1499ed28..d41159e52a 100644
--- a/linux-user/hexagon/cpu_loop.c
+++ b/linux-user/hexagon/cpu_loop.c
@@ -60,6 +60,10 @@  void cpu_loop(CPUHexagonState *env)
                 env->gpr[0] = ret;
             }
             break;
+        case HEX_EXCP_PC_NOT_ALIGNED:
+            force_sig_fault(TARGET_SIGBUS, TARGET_BUS_ADRALN,
+                            env->gpr[HEX_REG_R31]);
+            break;
         case EXCP_ATOMIC:
             cpu_exec_step_atomic(cs);
             break;
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index da10ac5847..ae5a605513 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -36,10 +36,9 @@ 
 #define SF_MANTBITS    23
 
 /* Exceptions processing helpers */
-static G_NORETURN
-void do_raise_exception_err(CPUHexagonState *env,
-                            uint32_t exception,
-                            uintptr_t pc)
+G_NORETURN void hexagon_raise_exception_err(CPUHexagonState *env,
+                                            uint32_t exception,
+                                            uintptr_t pc)
 {
     CPUState *cs = env_cpu(env);
     qemu_log_mask(CPU_LOG_INT, "%s: %d\n", __func__, exception);
@@ -49,7 +48,7 @@  void do_raise_exception_err(CPUHexagonState *env,
 
 G_NORETURN void HELPER(raise_exception)(CPUHexagonState *env, uint32_t excp)
 {
-    do_raise_exception_err(env, excp, 0);
+    hexagon_raise_exception_err(env, excp, 0);
 }
 
 void log_store32(CPUHexagonState *env, target_ulong addr,
diff --git a/tests/tcg/hexagon/unaligned_pc.c b/tests/tcg/hexagon/unaligned_pc.c
new file mode 100644
index 0000000000..1add2d0d99
--- /dev/null
+++ b/tests/tcg/hexagon/unaligned_pc.c
@@ -0,0 +1,85 @@ 
+#include <stdio.h>
+#include <signal.h>
+#include <setjmp.h>
+#include <stdlib.h>
+
+/* will be changed in signal handler */
+volatile sig_atomic_t completed_tests;
+static jmp_buf after_test;
+static int nr_tests;
+
+void __attribute__((naked)) test_return(void)
+{
+    asm volatile(
+        "allocframe(#0x8)\n"
+        "r0 = #0xffffffff\n"
+        "framekey = r0\n"
+        "dealloc_return\n"
+        : : : "r0");
+}
+
+void test_endloop(void)
+{
+    asm volatile(
+        "loop0(1f, #2)\n"
+        "1: r0 = #0x3\n"
+        "sa0 = r0\n"
+        "{ nop }:endloop0\n"
+        : : : "r0");
+}
+
+void test_multi_cof(void)
+{
+    asm volatile(
+        "p0 = cmp.eq(r0, r0)\n"
+        "{\n"
+        "    if (p0) jump test_multi_cof_unaligned\n"
+        "    jump 1f\n"
+        "}\n"
+        "1: nop\n"
+        : : : "p0");
+}
+
+void sigbus_handler(int signum)
+{
+    /* retore framekey after test_return */
+    asm volatile(
+        "r0 = #0\n"
+        "framekey = r0\n"
+        : : : "r0");
+    printf("Test %d complete\n", completed_tests);
+    completed_tests++;
+    siglongjmp(after_test, 1);
+}
+
+void test_done(void)
+{
+    int err = (completed_tests != nr_tests);
+    puts(err ? "FAIL" : "PASS");
+    exit(err);
+}
+
+typedef void (*test_fn)(void);
+
+int main()
+{
+    test_fn tests[] = { test_return, test_endloop, test_multi_cof, test_done };
+    nr_tests = (sizeof(tests) / sizeof(tests[0])) - 1;
+
+    struct sigaction sa = {
+        .sa_sigaction = sigbus_handler,
+        .sa_flags = SA_SIGINFO
+    };
+
+    if (sigaction(SIGBUS, &sa, NULL) < 0) {
+        perror("sigaction");
+        return EXIT_FAILURE;
+    }
+
+    sigsetjmp(after_test, 1);
+    tests[completed_tests]();
+
+    /* should never get here */
+    puts("FAIL");
+    return 1;
+}
diff --git a/tests/tcg/hexagon/Makefile.target b/tests/tcg/hexagon/Makefile.target
index f839b2c0d5..75139e731c 100644
--- a/tests/tcg/hexagon/Makefile.target
+++ b/tests/tcg/hexagon/Makefile.target
@@ -51,6 +51,7 @@  HEX_TESTS += scatter_gather
 HEX_TESTS += hvx_misc
 HEX_TESTS += hvx_histogram
 HEX_TESTS += invalid-slots
+HEX_TESTS += unaligned_pc
 
 run-and-check-exception = $(call run-test,$2,$3 2>$2.stderr; \
 	test $$? -eq 1 && grep -q "exception $(strip $1)" $2.stderr)
@@ -108,6 +109,9 @@  preg_alias: preg_alias.c hex_test.h
 read_write_overlap: read_write_overlap.c hex_test.h
 reg_mut: reg_mut.c hex_test.h
 
+unaligned_pc: unaligned_pc.c unaligned_pc_multi_cof.S
+	$(CC) $(CFLAGS) $(CROSS_CC_GUEST_CFLAGS) -mv73 $^ -o $@ $(LDFLAGS)
+
 # This test has to be compiled for the -mv67t target
 usr: usr.c hex_test.h
 	$(CC) $(CFLAGS) -mv67t -O2 -Wno-inline-asm -Wno-expansion-to-defined $< -o $@ $(LDFLAGS)
diff --git a/tests/tcg/hexagon/unaligned_pc_multi_cof.S b/tests/tcg/hexagon/unaligned_pc_multi_cof.S
new file mode 100644
index 0000000000..10accd0057
--- /dev/null
+++ b/tests/tcg/hexagon/unaligned_pc_multi_cof.S
@@ -0,0 +1,5 @@ 
+.org 0x3
+.global test_multi_cof_unaligned
+test_multi_cof_unaligned:
+	nop
+	jumpr r31