From: Richard Henderson <richard.henderson@linaro.org>
To: "Philippe Mathieu-Daudé" <f4bug@amsat.org>, qemu-devel@nongnu.org
Subject: Re: [PATCH] target/mips: Advance pc after semihosting exception
Date: Tue, 2 Aug 2022 07:11:35 -0700 [thread overview]
Message-ID: <ea8ad12b-66fb-7173-3e29-ffd7be3cd2b0@linaro.org> (raw)
In-Reply-To: <7fb76578-6cf0-3b25-63ea-2e9dce7b3716@amsat.org>
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);
>
next prev parent reply other threads:[~2022-08-02 14:12 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-30 2:18 [PATCH] target/mips: Advance pc after semihosting exception Richard Henderson
2022-08-02 6:48 ` Philippe Mathieu-Daudé via
2022-08-02 14:11 ` Richard Henderson [this message]
2022-08-02 14:27 ` Philippe Mathieu-Daudé via
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ea8ad12b-66fb-7173-3e29-ffd7be3cd2b0@linaro.org \
--to=richard.henderson@linaro.org \
--cc=f4bug@amsat.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).