Re: [PATCH v3] Hexagon: add PC alignment check and exception

2024-05-02 Thread Matheus Tavares Bernardino
On Tue, 30 Apr 2024 08:52:36 -0700 Richard Henderson 
 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 :
   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 

   20dcc:   04 c0 00 58 5800c004jump 0x20dd0 
 }
   20dd0:   00 c0 00 7f 7f00c000 {  nop }
   20dd4:   00 00 c0 00 



RE: [PATCH v3] Hexagon: add PC alignment check and exception

2024-04-30 Thread ltaylorsimpson



> -Original Message-
> From: Matheus Tavares Bernardino 
> Sent: Tuesday, April 30, 2024 9:25 AM
> To: qemu-devel@nongnu.org
> Cc: bc...@quicinc.com; sidn...@quicinc.com; a...@rev.ng; a...@rev.ng;
> ltaylorsimp...@gmail.com; richard.hender...@linaro.org; Laurent Vivier
> 
> 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 
> ---
> v2: https://lore.kernel.org/qemu-
> devel/e559b521d1920f804df10244c8c07564431aeba5.1714419461.git.quic_ma
> thb...@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 00..1add2d0d99
> --- /dev/null
> +++ b/tests/tcg/hexagon/unaligned_pc.c
> @@ -0,0 +1,85 @@
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* 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 = #0x\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, , 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
> 

RE: [PATCH v3] Hexagon: add PC alignment check and exception

2024-04-30 Thread Brian Cain


> -Original Message-
> From: Richard Henderson 
> Sent: Tuesday, April 30, 2024 10:53 AM
> To: Matheus Bernardino (QUIC) ; qemu-
> de...@nongnu.org
> Cc: Brian Cain ; Sid Manning ;
> a...@rev.ng; a...@rev.ng; ltaylorsimp...@gmail.com; Laurent Vivier
> 
> 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


Re: [PATCH v3] Hexagon: add PC alignment check and exception

2024-04-30 Thread Richard Henderson

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~



Re: [PATCH v3] Hexagon: add PC alignment check and exception

2024-04-30 Thread Richard Henderson

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 
---
v2: 
https://lore.kernel.org/qemu-devel/e559b521d1920f804df10244c8c07564431aeba5.1714419461.git.quic_mathb...@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 


r~