* [PATCH] Hexagon: add PC alignment check and exception
@ 2024-04-26 18:15 Matheus Tavares Bernardino
2024-04-27 3:50 ` Brian Cain
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Matheus Tavares Bernardino @ 2024-04-26 18:15 UTC (permalink / raw)
To: qemu-devel; +Cc: bcain, sidneym, ale, anjo, ltaylorsimpson
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 tests for both the most common case as well as packets with
multiple change-of-flow instructions.
Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
---
target/hexagon/cpu_bits.h | 1 +
target/hexagon/translate.h | 2 ++
target/hexagon/genptr.c | 21 ++++++++++++++++-----
target/hexagon/translate.c | 2 +-
tests/tcg/hexagon/Makefile.target | 13 +++++++++++++
tests/tcg/hexagon/unaligned_pc.S | 10 ++++++++++
tests/tcg/hexagon/unaligned_pc_multi_cof.S | 13 +++++++++++++
7 files changed, 56 insertions(+), 6 deletions(-)
create mode 100644 tests/tcg/hexagon/unaligned_pc.S
create mode 100644 tests/tcg/hexagon/unaligned_pc_multi_cof.S
diff --git a/target/hexagon/cpu_bits.h b/target/hexagon/cpu_bits.h
index 96fef71729..d6900c8bda 100644
--- a/target/hexagon/cpu_bits.h
+++ b/target/hexagon/cpu_bits.h
@@ -23,6 +23,7 @@
#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/translate.h b/target/hexagon/translate.h
index 4dd59c6726..daf11eb584 100644
--- a/target/hexagon/translate.h
+++ b/target/hexagon/translate.h
@@ -75,6 +75,8 @@ typedef struct DisasContext {
TCGv dczero_addr;
} DisasContext;
+void gen_exception_end_tb(DisasContext *ctx, int excp);
+
static inline void ctx_log_pred_write(DisasContext *ctx, int pnum)
{
if (!test_bit(pnum, ctx->pregs_written)) {
diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
index dbae6c570a..c96edd9379 100644
--- a/target/hexagon/genptr.c
+++ b/target/hexagon/genptr.c
@@ -473,6 +473,7 @@ static void gen_write_new_pc_addr(DisasContext *ctx, TCGv addr,
TCGCond cond, TCGv pred)
{
TCGLabel *pred_false = NULL;
+ TCGLabel *branch_taken = NULL;
if (cond != TCG_COND_ALWAYS) {
pred_false = gen_new_label();
tcg_gen_brcondi_tl(cond, pred, 0, pred_false);
@@ -480,12 +481,22 @@ static void gen_write_new_pc_addr(DisasContext *ctx, TCGv addr,
if (ctx->pkt->pkt_has_multi_cof) {
/* If there are multiple branches in a packet, ignore the second one */
- tcg_gen_movcond_tl(TCG_COND_NE, hex_gpr[HEX_REG_PC],
- ctx->branch_taken, tcg_constant_tl(0),
- hex_gpr[HEX_REG_PC], addr);
+ branch_taken = gen_new_label();
+ tcg_gen_brcondi_tl(TCG_COND_NE, ctx->branch_taken, 0, branch_taken);
tcg_gen_movi_tl(ctx->branch_taken, 1);
- } else {
- tcg_gen_mov_tl(hex_gpr[HEX_REG_PC], addr);
+ }
+
+ TCGLabel *pc_aligned = gen_new_label();
+ TCGv pc_remainder = tcg_temp_new();
+ tcg_gen_andi_tl(pc_remainder, addr, PCALIGN_MASK);
+ tcg_gen_brcondi_tl(TCG_COND_EQ, pc_remainder, 0, pc_aligned);
+ gen_exception_end_tb(ctx, HEX_EXCP_PC_NOT_ALIGNED);
+ gen_set_label(pc_aligned);
+
+ tcg_gen_mov_tl(hex_gpr[HEX_REG_PC], addr);
+
+ if (ctx->pkt->pkt_has_multi_cof) {
+ gen_set_label(branch_taken);
}
if (cond != TCG_COND_ALWAYS) {
diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
index f163eefe97..e6ee63a53e 100644
--- a/target/hexagon/translate.c
+++ b/target/hexagon/translate.c
@@ -185,7 +185,7 @@ static void gen_end_tb(DisasContext *ctx)
ctx->base.is_jmp = DISAS_NORETURN;
}
-static void gen_exception_end_tb(DisasContext *ctx, int excp)
+void gen_exception_end_tb(DisasContext *ctx, int excp)
{
gen_exec_counters(ctx);
tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], ctx->next_PC);
diff --git a/tests/tcg/hexagon/Makefile.target b/tests/tcg/hexagon/Makefile.target
index f839b2c0d5..02d7fff34c 100644
--- a/tests/tcg/hexagon/Makefile.target
+++ b/tests/tcg/hexagon/Makefile.target
@@ -51,6 +51,19 @@ HEX_TESTS += scatter_gather
HEX_TESTS += hvx_misc
HEX_TESTS += hvx_histogram
HEX_TESTS += invalid-slots
+HEX_TESTS += unaligned_pc
+HEX_TESTS += unaligned_pc_multi_cof
+
+run-unaligned_pc: unaligned_pc
+run-unaligned_pc_multi_cof: unaligned_pc_multi_cof
+run-unaligned_pc run-unaligned_pc_multi_cof:
+ $(call run-test, $<, $(QEMU) $< 2> $<.stderr,"$< on $(TARGET_NAME)"); \
+ if [ $$? -ne 1 ] ; then \
+ return 1; \
+ fi
+ $(call quiet-command, \
+ grep -q "exception 0x1e" $<.stderr, \
+ "GREP", "exception 0x1e");
run-and-check-exception = $(call run-test,$2,$3 2>$2.stderr; \
test $$? -eq 1 && grep -q "exception $(strip $1)" $2.stderr)
diff --git a/tests/tcg/hexagon/unaligned_pc.S b/tests/tcg/hexagon/unaligned_pc.S
new file mode 100644
index 0000000000..39d6b2060b
--- /dev/null
+++ b/tests/tcg/hexagon/unaligned_pc.S
@@ -0,0 +1,10 @@
+test:
+ allocframe(#0x8)
+ r0 = #0xffffffff
+ framekey = r0
+ dealloc_return
+
+.global _start
+_start:
+ call test
+ jump pass
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..a83e248ece
--- /dev/null
+++ b/tests/tcg/hexagon/unaligned_pc_multi_cof.S
@@ -0,0 +1,13 @@
+.org 0x3
+test:
+ nop
+ jumpr r31
+
+.global _start
+_start:
+ p0 = cmp.eq(r0, r0)
+ {
+ if (p0) jump test
+ jump pass
+ }
+ jump pass
--
2.37.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* RE: [PATCH] Hexagon: add PC alignment check and exception
2024-04-26 18:15 [PATCH] Hexagon: add PC alignment check and exception Matheus Tavares Bernardino
@ 2024-04-27 3:50 ` Brian Cain
2024-04-27 14:56 ` Richard Henderson
2024-04-29 14:40 ` ltaylorsimpson
2 siblings, 0 replies; 7+ messages in thread
From: Brian Cain @ 2024-04-27 3:50 UTC (permalink / raw)
To: Matheus Bernardino (QUIC), qemu-devel@nongnu.org
Cc: Sid Manning, ale@rev.ng, anjo@rev.ng, ltaylorsimpson@gmail.com
> -----Original Message-----
> From: Matheus Bernardino (QUIC) <quic_mathbern@quicinc.com>
> Sent: Friday, April 26, 2024 1:16 PM
> To: qemu-devel@nongnu.org
> Cc: Brian Cain <bcain@quicinc.com>; Sid Manning <sidneym@quicinc.com>;
> ale@rev.ng; anjo@rev.ng; ltaylorsimpson@gmail.com
> Subject: [PATCH] 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 tests for both the most common case as well as packets with
> multiple change-of-flow instructions.
>
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
Reviewed-by: Brian Cain <bcain@quicinc.com>
> ---
> target/hexagon/cpu_bits.h | 1 +
> target/hexagon/translate.h | 2 ++
> target/hexagon/genptr.c | 21 ++++++++++++++++-----
> target/hexagon/translate.c | 2 +-
> tests/tcg/hexagon/Makefile.target | 13 +++++++++++++
> tests/tcg/hexagon/unaligned_pc.S | 10 ++++++++++
> tests/tcg/hexagon/unaligned_pc_multi_cof.S | 13 +++++++++++++
> 7 files changed, 56 insertions(+), 6 deletions(-)
> create mode 100644 tests/tcg/hexagon/unaligned_pc.S
> create mode 100644 tests/tcg/hexagon/unaligned_pc_multi_cof.S
>
> diff --git a/target/hexagon/cpu_bits.h b/target/hexagon/cpu_bits.h
> index 96fef71729..d6900c8bda 100644
> --- a/target/hexagon/cpu_bits.h
> +++ b/target/hexagon/cpu_bits.h
> @@ -23,6 +23,7 @@
> #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/translate.h b/target/hexagon/translate.h
> index 4dd59c6726..daf11eb584 100644
> --- a/target/hexagon/translate.h
> +++ b/target/hexagon/translate.h
> @@ -75,6 +75,8 @@ typedef struct DisasContext {
> TCGv dczero_addr;
> } DisasContext;
>
> +void gen_exception_end_tb(DisasContext *ctx, int excp);
> +
> static inline void ctx_log_pred_write(DisasContext *ctx, int pnum)
> {
> if (!test_bit(pnum, ctx->pregs_written)) {
> diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
> index dbae6c570a..c96edd9379 100644
> --- a/target/hexagon/genptr.c
> +++ b/target/hexagon/genptr.c
> @@ -473,6 +473,7 @@ static void gen_write_new_pc_addr(DisasContext
> *ctx, TCGv addr,
> TCGCond cond, TCGv pred)
> {
> TCGLabel *pred_false = NULL;
> + TCGLabel *branch_taken = NULL;
> if (cond != TCG_COND_ALWAYS) {
> pred_false = gen_new_label();
> tcg_gen_brcondi_tl(cond, pred, 0, pred_false);
> @@ -480,12 +481,22 @@ static void gen_write_new_pc_addr(DisasContext
> *ctx, TCGv addr,
>
> if (ctx->pkt->pkt_has_multi_cof) {
> /* If there are multiple branches in a packet, ignore the second one */
> - tcg_gen_movcond_tl(TCG_COND_NE, hex_gpr[HEX_REG_PC],
> - ctx->branch_taken, tcg_constant_tl(0),
> - hex_gpr[HEX_REG_PC], addr);
> + branch_taken = gen_new_label();
> + tcg_gen_brcondi_tl(TCG_COND_NE, ctx->branch_taken, 0,
> branch_taken);
> tcg_gen_movi_tl(ctx->branch_taken, 1);
> - } else {
> - tcg_gen_mov_tl(hex_gpr[HEX_REG_PC], addr);
> + }
> +
> + TCGLabel *pc_aligned = gen_new_label();
> + TCGv pc_remainder = tcg_temp_new();
> + tcg_gen_andi_tl(pc_remainder, addr, PCALIGN_MASK);
> + tcg_gen_brcondi_tl(TCG_COND_EQ, pc_remainder, 0, pc_aligned);
> + gen_exception_end_tb(ctx, HEX_EXCP_PC_NOT_ALIGNED);
> + gen_set_label(pc_aligned);
> +
> + tcg_gen_mov_tl(hex_gpr[HEX_REG_PC], addr);
> +
> + if (ctx->pkt->pkt_has_multi_cof) {
> + gen_set_label(branch_taken);
> }
>
> if (cond != TCG_COND_ALWAYS) {
> diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
> index f163eefe97..e6ee63a53e 100644
> --- a/target/hexagon/translate.c
> +++ b/target/hexagon/translate.c
> @@ -185,7 +185,7 @@ static void gen_end_tb(DisasContext *ctx)
> ctx->base.is_jmp = DISAS_NORETURN;
> }
>
> -static void gen_exception_end_tb(DisasContext *ctx, int excp)
> +void gen_exception_end_tb(DisasContext *ctx, int excp)
> {
> gen_exec_counters(ctx);
> tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], ctx->next_PC);
> diff --git a/tests/tcg/hexagon/Makefile.target
> b/tests/tcg/hexagon/Makefile.target
> index f839b2c0d5..02d7fff34c 100644
> --- a/tests/tcg/hexagon/Makefile.target
> +++ b/tests/tcg/hexagon/Makefile.target
> @@ -51,6 +51,19 @@ HEX_TESTS += scatter_gather
> HEX_TESTS += hvx_misc
> HEX_TESTS += hvx_histogram
> HEX_TESTS += invalid-slots
> +HEX_TESTS += unaligned_pc
> +HEX_TESTS += unaligned_pc_multi_cof
> +
> +run-unaligned_pc: unaligned_pc
> +run-unaligned_pc_multi_cof: unaligned_pc_multi_cof
> +run-unaligned_pc run-unaligned_pc_multi_cof:
> + $(call run-test, $<, $(QEMU) $< 2> $<.stderr,"$< on
> $(TARGET_NAME)"); \
> + if [ $$? -ne 1 ] ; then \
> + return 1; \
> + fi
> + $(call quiet-command, \
> + grep -q "exception 0x1e" $<.stderr, \
> + "GREP", "exception 0x1e");
>
> run-and-check-exception = $(call run-test,$2,$3 2>$2.stderr; \
> test $$? -eq 1 && grep -q "exception $(strip $1)" $2.stderr)
> diff --git a/tests/tcg/hexagon/unaligned_pc.S
> b/tests/tcg/hexagon/unaligned_pc.S
> new file mode 100644
> index 0000000000..39d6b2060b
> --- /dev/null
> +++ b/tests/tcg/hexagon/unaligned_pc.S
> @@ -0,0 +1,10 @@
> +test:
> + allocframe(#0x8)
> + r0 = #0xffffffff
> + framekey = r0
> + dealloc_return
> +
> +.global _start
> +_start:
> + call test
> + jump pass
> 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..a83e248ece
> --- /dev/null
> +++ b/tests/tcg/hexagon/unaligned_pc_multi_cof.S
> @@ -0,0 +1,13 @@
> +.org 0x3
> +test:
> + nop
> + jumpr r31
> +
> +.global _start
> +_start:
> + p0 = cmp.eq(r0, r0)
> + {
> + if (p0) jump test
> + jump pass
> + }
> + jump pass
> --
> 2.37.2
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Hexagon: add PC alignment check and exception
2024-04-26 18:15 [PATCH] Hexagon: add PC alignment check and exception Matheus Tavares Bernardino
2024-04-27 3:50 ` Brian Cain
@ 2024-04-27 14:56 ` Richard Henderson
2024-04-29 18:19 ` Richard Henderson
2024-04-29 14:40 ` ltaylorsimpson
2 siblings, 1 reply; 7+ messages in thread
From: Richard Henderson @ 2024-04-27 14:56 UTC (permalink / raw)
To: Matheus Tavares Bernardino, qemu-devel
Cc: bcain, sidneym, ale, anjo, ltaylorsimpson
On 4/26/24 11:15, 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 tests for both the most common case as well as packets with
> multiple change-of-flow instructions.
>
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---
> target/hexagon/cpu_bits.h | 1 +
> target/hexagon/translate.h | 2 ++
> target/hexagon/genptr.c | 21 ++++++++++++++++-----
> target/hexagon/translate.c | 2 +-
> tests/tcg/hexagon/Makefile.target | 13 +++++++++++++
> tests/tcg/hexagon/unaligned_pc.S | 10 ++++++++++
> tests/tcg/hexagon/unaligned_pc_multi_cof.S | 13 +++++++++++++
> 7 files changed, 56 insertions(+), 6 deletions(-)
> create mode 100644 tests/tcg/hexagon/unaligned_pc.S
> create mode 100644 tests/tcg/hexagon/unaligned_pc_multi_cof.S
>
> diff --git a/target/hexagon/cpu_bits.h b/target/hexagon/cpu_bits.h
> index 96fef71729..d6900c8bda 100644
> --- a/target/hexagon/cpu_bits.h
> +++ b/target/hexagon/cpu_bits.h
> @@ -23,6 +23,7 @@
> #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/translate.h b/target/hexagon/translate.h
> index 4dd59c6726..daf11eb584 100644
> --- a/target/hexagon/translate.h
> +++ b/target/hexagon/translate.h
> @@ -75,6 +75,8 @@ typedef struct DisasContext {
> TCGv dczero_addr;
> } DisasContext;
>
> +void gen_exception_end_tb(DisasContext *ctx, int excp);
> +
> static inline void ctx_log_pred_write(DisasContext *ctx, int pnum)
> {
> if (!test_bit(pnum, ctx->pregs_written)) {
> diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
> index dbae6c570a..c96edd9379 100644
> --- a/target/hexagon/genptr.c
> +++ b/target/hexagon/genptr.c
> @@ -473,6 +473,7 @@ static void gen_write_new_pc_addr(DisasContext *ctx, TCGv addr,
> TCGCond cond, TCGv pred)
> {
> TCGLabel *pred_false = NULL;
> + TCGLabel *branch_taken = NULL;
> if (cond != TCG_COND_ALWAYS) {
> pred_false = gen_new_label();
> tcg_gen_brcondi_tl(cond, pred, 0, pred_false);
> @@ -480,12 +481,22 @@ static void gen_write_new_pc_addr(DisasContext *ctx, TCGv addr,
>
> if (ctx->pkt->pkt_has_multi_cof) {
> /* If there are multiple branches in a packet, ignore the second one */
> - tcg_gen_movcond_tl(TCG_COND_NE, hex_gpr[HEX_REG_PC],
> - ctx->branch_taken, tcg_constant_tl(0),
> - hex_gpr[HEX_REG_PC], addr);
> + branch_taken = gen_new_label();
> + tcg_gen_brcondi_tl(TCG_COND_NE, ctx->branch_taken, 0, branch_taken);
> tcg_gen_movi_tl(ctx->branch_taken, 1);
> - } else {
> - tcg_gen_mov_tl(hex_gpr[HEX_REG_PC], addr);
> + }
> +
> + TCGLabel *pc_aligned = gen_new_label();
> + TCGv pc_remainder = tcg_temp_new();
> + tcg_gen_andi_tl(pc_remainder, addr, PCALIGN_MASK);
> + tcg_gen_brcondi_tl(TCG_COND_EQ, pc_remainder, 0, pc_aligned);
> + gen_exception_end_tb(ctx, HEX_EXCP_PC_NOT_ALIGNED);
> + gen_set_label(pc_aligned);
> +
> + tcg_gen_mov_tl(hex_gpr[HEX_REG_PC], addr);
I am suspicious that the exception is raised without the pc being assigned.
How does the exception handler see the incorrect value?
Also, this is a perfect place to use the new TCG_COND_TSTEQ condition, eliminating the
separate andi step and the variable.
r~
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Hexagon: add PC alignment check and exception
2024-04-27 14:56 ` Richard Henderson
@ 2024-04-29 18:19 ` Richard Henderson
0 siblings, 0 replies; 7+ messages in thread
From: Richard Henderson @ 2024-04-29 18:19 UTC (permalink / raw)
To: Matheus Tavares Bernardino, qemu-devel
Cc: bcain, sidneym, ale, anjo, ltaylorsimpson
On 4/27/24 07:56, Richard Henderson wrote:
> On 4/26/24 11:15, 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 tests for both the most common case as well as packets with
>> multiple change-of-flow instructions.
>>
>> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
>> ---
>> target/hexagon/cpu_bits.h | 1 +
>> target/hexagon/translate.h | 2 ++
>> target/hexagon/genptr.c | 21 ++++++++++++++++-----
>> target/hexagon/translate.c | 2 +-
>> tests/tcg/hexagon/Makefile.target | 13 +++++++++++++
>> tests/tcg/hexagon/unaligned_pc.S | 10 ++++++++++
>> tests/tcg/hexagon/unaligned_pc_multi_cof.S | 13 +++++++++++++
>> 7 files changed, 56 insertions(+), 6 deletions(-)
>> create mode 100644 tests/tcg/hexagon/unaligned_pc.S
>> create mode 100644 tests/tcg/hexagon/unaligned_pc_multi_cof.S
>>
>> diff --git a/target/hexagon/cpu_bits.h b/target/hexagon/cpu_bits.h
>> index 96fef71729..d6900c8bda 100644
>> --- a/target/hexagon/cpu_bits.h
>> +++ b/target/hexagon/cpu_bits.h
>> @@ -23,6 +23,7 @@
>> #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/translate.h b/target/hexagon/translate.h
>> index 4dd59c6726..daf11eb584 100644
>> --- a/target/hexagon/translate.h
>> +++ b/target/hexagon/translate.h
>> @@ -75,6 +75,8 @@ typedef struct DisasContext {
>> TCGv dczero_addr;
>> } DisasContext;
>> +void gen_exception_end_tb(DisasContext *ctx, int excp);
>> +
>> static inline void ctx_log_pred_write(DisasContext *ctx, int pnum)
>> {
>> if (!test_bit(pnum, ctx->pregs_written)) {
>> diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
>> index dbae6c570a..c96edd9379 100644
>> --- a/target/hexagon/genptr.c
>> +++ b/target/hexagon/genptr.c
>> @@ -473,6 +473,7 @@ static void gen_write_new_pc_addr(DisasContext *ctx, TCGv addr,
>> TCGCond cond, TCGv pred)
>> {
>> TCGLabel *pred_false = NULL;
>> + TCGLabel *branch_taken = NULL;
>> if (cond != TCG_COND_ALWAYS) {
>> pred_false = gen_new_label();
>> tcg_gen_brcondi_tl(cond, pred, 0, pred_false);
>> @@ -480,12 +481,22 @@ static void gen_write_new_pc_addr(DisasContext *ctx, TCGv addr,
>> if (ctx->pkt->pkt_has_multi_cof) {
>> /* If there are multiple branches in a packet, ignore the second one */
>> - tcg_gen_movcond_tl(TCG_COND_NE, hex_gpr[HEX_REG_PC],
>> - ctx->branch_taken, tcg_constant_tl(0),
>> - hex_gpr[HEX_REG_PC], addr);
>> + branch_taken = gen_new_label();
>> + tcg_gen_brcondi_tl(TCG_COND_NE, ctx->branch_taken, 0, branch_taken);
>> tcg_gen_movi_tl(ctx->branch_taken, 1);
>> - } else {
>> - tcg_gen_mov_tl(hex_gpr[HEX_REG_PC], addr);
>> + }
>> +
>> + TCGLabel *pc_aligned = gen_new_label();
>> + TCGv pc_remainder = tcg_temp_new();
>> + tcg_gen_andi_tl(pc_remainder, addr, PCALIGN_MASK);
>> + tcg_gen_brcondi_tl(TCG_COND_EQ, pc_remainder, 0, pc_aligned);
>> + gen_exception_end_tb(ctx, HEX_EXCP_PC_NOT_ALIGNED);
>> + gen_set_label(pc_aligned);
>> +
>> + tcg_gen_mov_tl(hex_gpr[HEX_REG_PC], addr);
>
> I am suspicious that the exception is raised without the pc being assigned.
> How does the exception handler see the incorrect value?
S390x does not check this with the branch, but when beginning to translate the next
instruction, in cpu_get_tb_cpu_state.
ARM does not check this with the branch, but just before translating each block, in
aarch64_tr_translate_insn, just before reading the instruction itself.
Alternately, RISC-V, whose exception handler expects to see pc still pointing at the
branch, but raises a special misaligned-pc exception, and places the branch target in
BADADDR where the exception handler can see it.
All of these arrange for the target address to be seen.
The hexagon manual I have at my fingertips is instructions only, and doesn't have any
exception handling details...
r~
r~
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] Hexagon: add PC alignment check and exception
2024-04-26 18:15 [PATCH] Hexagon: add PC alignment check and exception Matheus Tavares Bernardino
2024-04-27 3:50 ` Brian Cain
2024-04-27 14:56 ` Richard Henderson
@ 2024-04-29 14:40 ` ltaylorsimpson
2024-04-29 14:51 ` ltaylorsimpson
2 siblings, 1 reply; 7+ messages in thread
From: ltaylorsimpson @ 2024-04-29 14:40 UTC (permalink / raw)
To: 'Matheus Tavares Bernardino', qemu-devel
Cc: bcain, sidneym, ale, anjo
> -----Original Message-----
> From: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> Sent: Friday, April 26, 2024 1:16 PM
> To: qemu-devel@nongnu.org
> Cc: bcain@quicinc.com; sidneym@quicinc.com; ale@rev.ng; anjo@rev.ng;
> ltaylorsimpson@gmail.com
> Subject: [PATCH] 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 tests for both the most common case as well as packets with
> multiple change-of-flow instructions.
>
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---
> --- a/target/hexagon/genptr.c
> +++ b/target/hexagon/genptr.c
> @@ -473,6 +473,7 @@ static void gen_write_new_pc_addr(DisasContext
You haven't added the check to gen_write_new_pc_pcrel. It's not needed
there because the encoding guarantees the target is always aligned - right?
However, there is a call to gen_write_new_pc_addr inside that function. In
this case, we'll add a check that isn't necessary. Consider adding a
parameter to indicate if the check can be avoided.
> a/tests/tcg/hexagon/Makefile.target b/tests/tcg/hexagon/Makefile.target
> index f839b2c0d5..02d7fff34c 100644
> --- a/tests/tcg/hexagon/Makefile.target
> +++ b/tests/tcg/hexagon/Makefile.target
> @@ -51,6 +51,19 @@ HEX_TESTS += scatter_gather HEX_TESTS += hvx_misc
> HEX_TESTS += hvx_histogram HEX_TESTS += invalid-slots
> +HEX_TESTS += unaligned_pc
> +HEX_TESTS += unaligned_pc_multi_cof
> +
> +run-unaligned_pc: unaligned_pc
> +run-unaligned_pc_multi_cof: unaligned_pc_multi_cof run-unaligned_pc
> +run-unaligned_pc_multi_cof:
> + $(call run-test, $<, $(QEMU) $< 2> $<.stderr,"$< on
> $(TARGET_NAME)"); \
> + if [ $$? -ne 1 ] ; then \
> + return 1; \
> + fi
> + $(call quiet-command, \
> + grep -q "exception 0x1e" $<.stderr, \
> + "GREP", "exception 0x1e");
We should also test endloop instructions.
Thanks,
Taylor
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] Hexagon: add PC alignment check and exception
2024-04-29 14:40 ` ltaylorsimpson
@ 2024-04-29 14:51 ` ltaylorsimpson
0 siblings, 0 replies; 7+ messages in thread
From: ltaylorsimpson @ 2024-04-29 14:51 UTC (permalink / raw)
To: 'Matheus Tavares Bernardino', qemu-devel
Cc: bcain, sidneym, ale, anjo
PS You should also update the pkt_raises_exception function in translate.c
to return true for packets that contain these instructions. This will
ensure that none of the machine state is changed before the check is
complete.
Taylor
> -----Original Message-----
> From: ltaylorsimpson@gmail.com <ltaylorsimpson@gmail.com>
> Sent: Monday, April 29, 2024 9:41 AM
> To: 'Matheus Tavares Bernardino' <quic_mathbern@quicinc.com>; qemu-
> devel@nongnu.org
> Cc: bcain@quicinc.com; sidneym@quicinc.com; ale@rev.ng; anjo@rev.ng
> Subject: RE: [PATCH] Hexagon: add PC alignment check and exception
>
>
>
> > -----Original Message-----
> > From: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> > Sent: Friday, April 26, 2024 1:16 PM
> > To: qemu-devel@nongnu.org
> > Cc: bcain@quicinc.com; sidneym@quicinc.com; ale@rev.ng; anjo@rev.ng;
> > ltaylorsimpson@gmail.com
> > Subject: [PATCH] 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 tests for both the most common case as well as
> > packets with multiple change-of-flow instructions.
> >
> > Signed-off-by: Matheus Tavares Bernardino
> <quic_mathbern@quicinc.com>
> > ---
>
>
> > --- a/target/hexagon/genptr.c
> > +++ b/target/hexagon/genptr.c
> > @@ -473,6 +473,7 @@ static void gen_write_new_pc_addr(DisasContext
>
> You haven't added the check to gen_write_new_pc_pcrel. It's not needed
> there because the encoding guarantees the target is always aligned -
right?
> However, there is a call to gen_write_new_pc_addr inside that function.
In
> this case, we'll add a check that isn't necessary. Consider adding a
parameter
> to indicate if the check can be avoided.
>
>
> > a/tests/tcg/hexagon/Makefile.target
> > b/tests/tcg/hexagon/Makefile.target
> > index f839b2c0d5..02d7fff34c 100644
> > --- a/tests/tcg/hexagon/Makefile.target
> > +++ b/tests/tcg/hexagon/Makefile.target
> > @@ -51,6 +51,19 @@ HEX_TESTS += scatter_gather HEX_TESTS +=
> hvx_misc
> > HEX_TESTS += hvx_histogram HEX_TESTS += invalid-slots
> > +HEX_TESTS += unaligned_pc
> > +HEX_TESTS += unaligned_pc_multi_cof
> > +
> > +run-unaligned_pc: unaligned_pc
> > +run-unaligned_pc_multi_cof: unaligned_pc_multi_cof run-unaligned_pc
> > +run-unaligned_pc_multi_cof:
> > + $(call run-test, $<, $(QEMU) $< 2> $<.stderr,"$< on
> > $(TARGET_NAME)"); \
> > + if [ $$? -ne 1 ] ; then \
> > + return 1; \
> > + fi
> > + $(call quiet-command, \
> > + grep -q "exception 0x1e" $<.stderr, \
> > + "GREP", "exception 0x1e");
>
> We should also test endloop instructions.
>
> Thanks,
> Taylor
>
^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <028e01da9a44_b05d2940_11177bc0_@gmail.com>]
* RE: [PATCH] Hexagon: add PC alignment check and exception
[not found] <028e01da9a44_b05d2940_11177bc0_@gmail.com>
@ 2024-04-29 18:02 ` Matheus Tavares Bernardino
0 siblings, 0 replies; 7+ messages in thread
From: Matheus Tavares Bernardino @ 2024-04-29 18:02 UTC (permalink / raw)
To: ltaylorsimpson; +Cc: quic_mathbern, qemu-devel, bcain, sidneym, ale, anjo
Hi, Taylor
On Mon, 29 Apr 2024 09:51:16 -0500 <ltaylorsimpson@gmail.com> wrote:
>
> PS You should also update the pkt_raises_exception function in translate.c
> to return true for packets that contain these instructions. This will
> ensure that none of the machine state is changed before the check is
> complete.
Will do, thanks.
> > -----Original Message-----
> > From: ltaylorsimpson@gmail.com <ltaylorsimpson@gmail.com>
> >
> >
> >
> > > -----Original Message-----
> > > From: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> > >
> > >
> > > --- a/target/hexagon/genptr.c
> > > +++ b/target/hexagon/genptr.c
> > > @@ -473,6 +473,7 @@ static void gen_write_new_pc_addr(DisasContext
> >
> > You haven't added the check to gen_write_new_pc_pcrel. It's not needed
> > there because the encoding guarantees the target is always aligned -
> right?
> > However, there is a call to gen_write_new_pc_addr inside that function.
> In
> > this case, we'll add a check that isn't necessary. Consider adding a
> parameter
> > to indicate if the check can be avoided.
Actually, I had missed this spot and I think we do need the check at
gen_write_new_pc_pcrel too. The added test `unaligned_pc_multi_cof.S` will
exercise it with a relative PC addr that is not aligned. I'll fix that, thanks.
> > > a/tests/tcg/hexagon/Makefile.target
> > > b/tests/tcg/hexagon/Makefile.target
> > > index f839b2c0d5..02d7fff34c 100644
> > > --- a/tests/tcg/hexagon/Makefile.target
> > > +++ b/tests/tcg/hexagon/Makefile.target
> > > @@ -51,6 +51,19 @@ HEX_TESTS += scatter_gather HEX_TESTS +> hvx_misc
> > > HEX_TESTS += hvx_histogram HEX_TESTS += invalid-slots
> > > +HEX_TESTS += unaligned_pc
> > > +HEX_TESTS += unaligned_pc_multi_cof
> > > +
> > > +run-unaligned_pc: unaligned_pc
> > > +run-unaligned_pc_multi_cof: unaligned_pc_multi_cof run-unaligned_pc
> > > +run-unaligned_pc_multi_cof:
> > > + $(call run-test, $<, $(QEMU) $< 2> $<.stderr,"$< on
> > > $(TARGET_NAME)"); \
> > > + if [ $$? -ne 1 ] ; then \
> > > + return 1; \
> > > + fi
> > > + $(call quiet-command, \
> > > + grep -q "exception 0x1e" $<.stderr, \
> > > + "GREP", "exception 0x1e");
> >
> > We should also test endloop instructions.
Good idea, will do.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-04-29 18:20 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-26 18:15 [PATCH] Hexagon: add PC alignment check and exception Matheus Tavares Bernardino
2024-04-27 3:50 ` Brian Cain
2024-04-27 14:56 ` Richard Henderson
2024-04-29 18:19 ` Richard Henderson
2024-04-29 14:40 ` ltaylorsimpson
2024-04-29 14:51 ` ltaylorsimpson
[not found] <028e01da9a44_b05d2940_11177bc0_@gmail.com>
2024-04-29 18:02 ` Matheus Tavares Bernardino
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).