Re: [PATCH] target/mips: Advance pc after semihosting exception

[Richard Henderson]

On 8/1/22 23:48, Philippe Mathieu-Daudé wrote:
> Hi Richard,
>
> On 30/7/22 04:18, Richard Henderson wrote:
> > Delay generating the exception until after we know the
> > insn length, and record that length in env->error_code.
> >
> > Fixes: 8ec7e3c53d4 ("target/mips: Use an exception for semihosting")
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1126
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >   target/mips/tcg/translate.h               |  4 ++++
> >   target/mips/tcg/sysemu/tlb_helper.c       |  1 +
> >   target/mips/tcg/translate.c               | 10 +++++-----
> >   target/mips/tcg/micromips_translate.c.inc |  6 +++---
> >   target/mips/tcg/mips16e_translate.c.inc   |  2 +-
> >   target/mips/tcg/nanomips_translate.c.inc  |  4 ++--
> >   6 files changed, 16 insertions(+), 11 deletions(-)
> >
> > diff --git a/target/mips/tcg/translate.h b/target/mips/tcg/translate.h
> > index 55053226ae..69f85841d2 100644
> > --- a/target/mips/tcg/translate.h
> > +++ b/target/mips/tcg/translate.h
> > @@ -51,6 +51,10 @@ typedef struct DisasContext {
> >       int gi;
> >   } DisasContext;
> > +#define DISAS_STOP       DISAS_TARGET_0
> > +#define DISAS_EXIT       DISAS_TARGET_1
> > +#define DISAS_SEMIHOST   DISAS_TARGET_2
> > +
> >   /* MIPS major opcodes */
> >   #define MASK_OP_MAJOR(op)   (op & (0x3F << 26))
> > diff --git a/target/mips/tcg/sysemu/tlb_helper.c 
> > b/target/mips/tcg/sysemu/tlb_helper.c
> > index 57ffad2902..9d16859c0a 100644
> > --- a/target/mips/tcg/sysemu/tlb_helper.c
> > +++ b/target/mips/tcg/sysemu/tlb_helper.c
> > @@ -1056,6 +1056,7 @@ void mips_cpu_do_interrupt(CPUState *cs)
> >       case EXCP_SEMIHOST:
> >           cs->exception_index = EXCP_NONE;
> >           mips_semihosting(env);
> > +        env->active_tc.PC += env->error_code;
> >           return;
> >       case EXCP_DSS:
> >           env->CP0_Debug |= 1 << CP0DB_DSS;
> > diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
> > index 1f6a779808..de1511baaf 100644
> > --- a/target/mips/tcg/translate.c
> > +++ b/target/mips/tcg/translate.c
> > @@ -1213,9 +1213,6 @@ TCGv_i64 fpu_f64[32];
> >   #include "exec/gen-icount.h"
> > -#define DISAS_STOP       DISAS_TARGET_0
> > -#define DISAS_EXIT       DISAS_TARGET_1
> > -
> >   static const char regnames_HI[][4] = {
> >       "HI0", "HI1", "HI2", "HI3",
> >   };
> > @@ -13902,7 +13899,7 @@ static void decode_opc_special_r6(CPUMIPSState *env, 
> > DisasContext *ctx)
> >           break;
> >       case R6_OPC_SDBBP:
> >           if (is_uhi(extract32(ctx->opcode, 6, 20))) {
> > -            generate_exception_end(ctx, EXCP_SEMIHOST);
> > +            ctx->base.is_jmp = DISAS_SEMIHOST;
> >           } else {
> >               if (ctx->hflags & MIPS_HFLAG_SBRI) {
> >                   gen_reserved_instruction(ctx);
> > @@ -14314,7 +14311,7 @@ static void decode_opc_special2_legacy(CPUMIPSState *env, 
> > DisasContext *ctx)
> >           break;
> >       case OPC_SDBBP:
> >           if (is_uhi(extract32(ctx->opcode, 6, 20))) {
> > -            generate_exception_end(ctx, EXCP_SEMIHOST);
> > +            ctx->base.is_jmp = DISAS_SEMIHOST;
> >           } else {
> >               /*
> >                * XXX: not clear which exception should be raised
> > @@ -16098,6 +16095,9 @@ static void mips_tr_translate_insn(DisasContextBase *dcbase, 
> > CPUState *cs)
> >       if (is_slot) {
> >           gen_branch(ctx, insn_bytes);
> >       }
> > +    if (ctx->base.is_jmp == DISAS_SEMIHOST) {
> > +        generate_exception_err(ctx, EXCP_SEMIHOST, insn_bytes);
>
> Hmm this API takes an error_code argument:
>
>    int error_code;
>    #define EXCP_TLB_NOMATCH   0x1
>    #define EXCP_INST_NOTAVAIL 0x2 /* No valid instruction word for BadInstr */
>
> Now we pass bytes. What about adding an extra helper?
>
>    void generate_exception_err_displace(DisasContext *ctx,
>                                         int excp, int err,
>                                         target_long displacement);

These error codes are specific to the matching EXCP.
We don't need to introduce extra storage, I don't think.


r~

> Otherwise LGTM.
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> > +    }
> >       ctx->base.pc_next += insn_bytes;
> >       if (ctx->base.is_jmp != DISAS_NEXT) {
> > diff --git a/target/mips/tcg/micromips_translate.c.inc 
> > b/target/mips/tcg/micromips_translate.c.inc
> > index 274caf2c3c..b2c696f891 100644
> > --- a/target/mips/tcg/micromips_translate.c.inc
> > +++ b/target/mips/tcg/micromips_translate.c.inc
> > @@ -826,7 +826,7 @@ static void gen_pool16c_insn(DisasContext *ctx)
> >           break;
> >       case SDBBP16:
> >           if (is_uhi(extract32(ctx->opcode, 0, 4))) {
> > -            generate_exception_end(ctx, EXCP_SEMIHOST);
> > +            ctx->base.is_jmp = DISAS_SEMIHOST;
> >           } else {
> >               /*
> >                * XXX: not clear which exception should be raised
> > @@ -942,7 +942,7 @@ static void gen_pool16c_r6_insn(DisasContext *ctx)
> >           case R6_SDBBP16:
> >               /* SDBBP16 */
> >               if (is_uhi(extract32(ctx->opcode, 6, 4))) {
> > -                generate_exception_end(ctx, EXCP_SEMIHOST);
> > +                ctx->base.is_jmp = DISAS_SEMIHOST;
> >               } else {
> >                   if (ctx->hflags & MIPS_HFLAG_SBRI) {
> >                       generate_exception(ctx, EXCP_RI);
> > @@ -1311,7 +1311,7 @@ static void gen_pool32axf(CPUMIPSState *env, DisasContext *ctx, 
> > int rt, int rs)
> >               break;
> >           case SDBBP:
> >               if (is_uhi(extract32(ctx->opcode, 16, 10))) {
> > -                generate_exception_end(ctx, EXCP_SEMIHOST);
> > +                ctx->base.is_jmp = DISAS_SEMIHOST;
> >               } else {
> >                   check_insn(ctx, ISA_MIPS_R1);
> >                   if (ctx->hflags & MIPS_HFLAG_SBRI) {
> > diff --git a/target/mips/tcg/mips16e_translate.c.inc 
> > b/target/mips/tcg/mips16e_translate.c.inc
> > index 0a3ba252e4..7568933e23 100644
> > --- a/target/mips/tcg/mips16e_translate.c.inc
> > +++ b/target/mips/tcg/mips16e_translate.c.inc
> > @@ -952,7 +952,7 @@ static int decode_ase_mips16e(CPUMIPSState *env, 
> > DisasContext *ctx)
> >               break;
> >           case RR_SDBBP:
> >               if (is_uhi(extract32(ctx->opcode, 5, 6))) {
> > -                generate_exception_end(ctx, EXCP_SEMIHOST);
> > +                ctx->base.is_jmp = DISAS_SEMIHOST;
> >               } else {
> >                   /*
> >                    * XXX: not clear which exception should be raised
> > diff --git a/target/mips/tcg/nanomips_translate.c.inc 
> > b/target/mips/tcg/nanomips_translate.c.inc
> > index ecb0ebed57..b3aff22c18 100644
> > --- a/target/mips/tcg/nanomips_translate.c.inc
> > +++ b/target/mips/tcg/nanomips_translate.c.inc
> > @@ -3695,7 +3695,7 @@ static int decode_nanomips_32_48_opc(CPUMIPSState *env, 
> > DisasContext *ctx)
> >                   break;
> >               case NM_SDBBP:
> >                   if (is_uhi(extract32(ctx->opcode, 0, 19))) {
> > -                    generate_exception_end(ctx, EXCP_SEMIHOST);
> > +                    ctx->base.is_jmp = DISAS_SEMIHOST;
> >                   } else {
> >                       if (ctx->hflags & MIPS_HFLAG_SBRI) {
> >                           gen_reserved_instruction(ctx);
> > @@ -4634,7 +4634,7 @@ static int decode_isa_nanomips(CPUMIPSState *env, 
> > DisasContext *ctx)
> >                   break;
> >               case NM_SDBBP16:
> >                   if (is_uhi(extract32(ctx->opcode, 0, 3))) {
> > -                    generate_exception_end(ctx, EXCP_SEMIHOST);
> > +                    ctx->base.is_jmp = DISAS_SEMIHOST;
> >                   } else {
> >                       if (ctx->hflags & MIPS_HFLAG_SBRI) {
> >                           gen_reserved_instruction(ctx);