[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 7/7] target/microblaze: Diagnose invalid insns in delay slots
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH 7/7] target/microblaze: Diagnose invalid insns in delay slots |
Date: |
Wed, 2 Sep 2020 19:13:51 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 |
On 9/2/20 6:35 PM, Richard Henderson wrote:
> These cases result in undefined and undocumented behaviour but the
> behaviour is deterministic, i.e cores will not lock-up or expose
> security issues. However, RTL will not raise exceptions either.
>
> Therefore, log a GUEST_ERROR and treat these cases as nops, to
> avoid corner cases which could put qemu into an invalid state.
Excellent! This is the same issue I have with TX39 MIPS cores :)
Thanks for the clean fix.
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/microblaze/translate.c | 47 +++++++++++++++++++++++++++++------
> 1 file changed, 40 insertions(+), 7 deletions(-)
>
> diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
> index 4416361d08..caa4831aed 100644
> --- a/target/microblaze/translate.c
> +++ b/target/microblaze/translate.c
> @@ -179,6 +179,20 @@ static bool trap_userspace(DisasContext *dc, bool cond)
> return cond_user;
> }
>
> +/*
> + * Return true, and log an error, if the current insn is
> + * within a delay slot.
> + */
> +static bool invalid_delay_slot(DisasContext *dc, const char *insn_type)
> +{
> + if (dc->tb_flags & D_FLAG) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "Invalid insn in delay slot: %s\n", insn_type);
> + return true;
> + }
> + return false;
> +}
> +
> static TCGv_i32 reg_for_read(DisasContext *dc, int reg)
> {
> if (likely(reg != 0)) {
> @@ -500,6 +514,9 @@ DO_TYPEA_CFG(idivu, use_div, true, gen_idivu)
>
> static bool trans_imm(DisasContext *dc, arg_imm *arg)
> {
> + if (invalid_delay_slot(dc, "imm")) {
> + return true;
> + }
> dc->ext_imm = arg->imm << 16;
> tcg_gen_movi_i32(cpu_imm, dc->ext_imm);
> dc->tb_flags_to_set = IMM_FLAG;
> @@ -1067,6 +1084,9 @@ static bool do_branch(DisasContext *dc, int dest_rb,
> int dest_imm,
> {
> uint32_t add_pc;
>
> + if (invalid_delay_slot(dc, "branch")) {
> + return true;
> + }
> if (delay) {
> setup_dslot(dc, dest_rb < 0);
> }
> @@ -1106,6 +1126,9 @@ static bool do_bcc(DisasContext *dc, int dest_rb, int
> dest_imm,
> {
> TCGv_i32 zero, next;
>
> + if (invalid_delay_slot(dc, "bcc")) {
> + return true;
> + }
> if (delay) {
> setup_dslot(dc, dest_rb < 0);
> }
> @@ -1158,6 +1181,10 @@ static bool trans_brk(DisasContext *dc, arg_typea_br
> *arg)
> if (trap_userspace(dc, true)) {
> return true;
> }
> + if (invalid_delay_slot(dc, "brk")) {
> + return true;
> + }
> +
> tcg_gen_mov_i32(cpu_pc, reg_for_read(dc, arg->rb));
> if (arg->rd) {
> tcg_gen_movi_i32(cpu_R[arg->rd], dc->base.pc_next);
> @@ -1176,6 +1203,10 @@ static bool trans_brki(DisasContext *dc, arg_typeb_br
> *arg)
> if (trap_userspace(dc, imm != 0x8 && imm != 0x18)) {
> return true;
> }
> + if (invalid_delay_slot(dc, "brki")) {
> + return true;
> + }
> +
> tcg_gen_movi_i32(cpu_pc, imm);
> if (arg->rd) {
> tcg_gen_movi_i32(cpu_R[arg->rd], dc->base.pc_next);
> @@ -1216,6 +1247,11 @@ static bool trans_mbar(DisasContext *dc, arg_mbar *arg)
> {
> int mbar_imm = arg->imm;
>
> + /* Note that mbar is a specialized branch instruction. */
> + if (invalid_delay_slot(dc, "mbar")) {
> + return true;
> + }
> +
> /* Data access memory barrier. */
> if ((mbar_imm & 2) == 0) {
> tcg_gen_mb(TCG_BAR_SC | TCG_MO_ALL);
> @@ -1263,6 +1299,10 @@ static bool do_rts(DisasContext *dc, arg_typeb_bc
> *arg, int to_set)
> if (trap_userspace(dc, to_set)) {
> return true;
> }
> + if (invalid_delay_slot(dc, "rts")) {
> + return true;
> + }
> +
> dc->tb_flags_to_set |= to_set;
> setup_dslot(dc, true);
>
> @@ -1695,7 +1735,6 @@ static void mb_tr_translate_insn(DisasContextBase *dcb,
> CPUState *cs)
> if (dc->jmp_cond != TCG_COND_NEVER && !(dc->tb_flags & D_FLAG)) {
> /*
> * Finish any return-from branch.
> - * TODO: Diagnose rtXd in delay slot of rtYd earlier.
> */
> uint32_t rt_ibe = dc->tb_flags & (DRTI_FLAG | DRTB_FLAG | DRTE_FLAG);
> if (unlikely(rt_ibe != 0)) {
> @@ -1717,12 +1756,6 @@ static void mb_tr_translate_insn(DisasContextBase
> *dcb, CPUState *cs)
> * and will handle D_FLAG in mb_cpu_do_interrupt.
> */
> break;
> - case DISAS_EXIT:
> - /*
> - * TODO: diagnose brk/brki in delay slot earlier.
> - * This would then fold into the illegal insn case above.
> - */
> - break;
> case DISAS_NEXT:
> /*
> * Normal insn a delay slot.
>
- [PATCH 0/7] target/microblaze improvements, Richard Henderson, 2020/09/02
- [PATCH 1/7] target/microblaze: Rename DISAS_UPDATE to DISAS_EXIT, Richard Henderson, 2020/09/02
- [PATCH 2/7] target/microblaze: Introduce DISAS_EXIT_NEXT, DISAS_EXIT_JUMP, Richard Henderson, 2020/09/02
- [PATCH 4/7] target/microblaze: Handle DISAS_EXIT_NEXT in delay slot, Richard Henderson, 2020/09/02
- [PATCH 3/7] target/microblaze: Replace cpustate_changed with DISAS_EXIT_NEXT, Richard Henderson, 2020/09/02
- [PATCH 6/7] target/microblaze: Use tcg_gen_lookup_and_goto_ptr, Richard Henderson, 2020/09/02
- [PATCH 7/7] target/microblaze: Diagnose invalid insns in delay slots, Richard Henderson, 2020/09/02
- [PATCH 5/7] target/microblaze: Force rtid, rted, rtbd to exit, Richard Henderson, 2020/09/02