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

[Richard Henderson]

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~